| Message ID | 20211214114140.54629-13-miquel.raynal@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Pipelined ECC engines & Macronix support | expand |
Hi Boris, boris.brezillon@collabora.com wrote on Tue, 14 Dec 2021 17:24:10 +0100: > On Tue, 14 Dec 2021 12:41:39 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > This driver can be simplified a little bit by using > > spi_mem_generic_supports_op() instead of the > > spi_mem_default/dtr_supports_op() couple. The all_false boolean is > > inverted to become a dtr boolean, which checks if at least one of the > > operation member uses dtr mode. The idea behind this change is to > > simplify the introduction of the pipelined ECC engine. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/spi/spi-mxic.c | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > > index 485a7f2afb44..5e71aa630504 100644 > > --- a/drivers/spi/spi-mxic.c > > +++ b/drivers/spi/spi-mxic.c > > @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc, > > static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > const struct spi_mem_op *op) > > { > > - bool all_false; > > + struct spi_mem_controller_caps caps = {}; > > > > if (op->data.buswidth > 8 || op->addr.buswidth > 8 || > > op->dummy.buswidth > 8 || op->cmd.buswidth > 8) > > @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > if (op->addr.nbytes > 7) > > return false; > > > > - all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr && > > - !op->data.dtr; > > + caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr; > > Are you sure that's what you want to do? spi_mem_controller_caps is > supposed to encode the controller capabilities, not whether the > operation contains a DTR cycle or not. I'd expect this caps object to be > statically defined, with possibly one instance per-compat if the caps > depend on the HW revision. In order to keep the series easy to review I decided to go for the following approach: * Introduce the spi_mem_generic_supports_op_helper() which takes a capabilities structure. This helper gathers all the checks from spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These two helpers now call the new one with either a NULL pointer in the former case, or a structure with the .dtr parameter set to true in the latter. * Change the API of spi_mem_default_supports_op(), this involves updating many different drivers so this change does only that in a very transparent way, with no functional changes at all. All the drivers provide a NULL parameter for the capabilities structure. * Actually make use of the new parameter of spi_mem_default_supports_op() in the drivers Cadence and Macronix, which do have DTR support. This kills the spi_mem_dtr_supports_op() helper. * Kill the temporary spi_mem_generic_supports_op() helper by moving all the logic back into spi_mem_default_supports_op(). This approach is really straightforward and easily bisectable if needed. While working on this, I fixed the check we discussed on IRC about the command parameter when in a DTR operation. I also reverted the logic in the various checks, as you suggested. Thanks, Miquèl
On Wed, 15 Dec 2021 18:44:26 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Boris, > > boris.brezillon@collabora.com wrote on Tue, 14 Dec 2021 17:24:10 +0100: > > > On Tue, 14 Dec 2021 12:41:39 +0100 > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > This driver can be simplified a little bit by using > > > spi_mem_generic_supports_op() instead of the > > > spi_mem_default/dtr_supports_op() couple. The all_false boolean is > > > inverted to become a dtr boolean, which checks if at least one of the > > > operation member uses dtr mode. The idea behind this change is to > > > simplify the introduction of the pipelined ECC engine. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/spi/spi-mxic.c | 10 +++------- > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > > > index 485a7f2afb44..5e71aa630504 100644 > > > --- a/drivers/spi/spi-mxic.c > > > +++ b/drivers/spi/spi-mxic.c > > > @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc, > > > static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > > const struct spi_mem_op *op) > > > { > > > - bool all_false; > > > + struct spi_mem_controller_caps caps = {}; > > > > > > if (op->data.buswidth > 8 || op->addr.buswidth > 8 || > > > op->dummy.buswidth > 8 || op->cmd.buswidth > 8) > > > @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > > if (op->addr.nbytes > 7) > > > return false; > > > > > > - all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr && > > > - !op->data.dtr; > > > + caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr; > > > > Are you sure that's what you want to do? spi_mem_controller_caps is > > supposed to encode the controller capabilities, not whether the > > operation contains a DTR cycle or not. I'd expect this caps object to be > > statically defined, with possibly one instance per-compat if the caps > > depend on the HW revision. > > In order to keep the series easy to review I decided to go for the > following approach: > * Introduce the spi_mem_generic_supports_op_helper() which takes a > capabilities structure. This helper gathers all the checks from > spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These > two helpers now call the new one with either a NULL pointer in the > former case, or a structure with the .dtr parameter set to true in > the latter. > * Change the API of spi_mem_default_supports_op(), this involves > updating many different drivers so this change does only that in a > very transparent way, with no functional changes at all. All the > drivers provide a NULL parameter for the capabilities structure. > * Actually make use of the new parameter of > spi_mem_default_supports_op() in the drivers Cadence and Macronix, > which do have DTR support. This kills the spi_mem_dtr_supports_op() > helper. > * Kill the temporary spi_mem_generic_supports_op() helper by moving > all the logic back into spi_mem_default_supports_op(). > > This approach is really straightforward and easily bisectable if > needed. I don't really see the benefit of converting drivers one by one for such a trivial/mechanical change to be honest. After all, we're talking about adding a caps argument to spi_mem_default_supports_op() and making all existing callers pass a caps object that's zero initialized. I hope we can get this right in one shot...
On Wed, 15 Dec 2021 18:44:26 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > In order to keep the series easy to review I decided to go for the > following approach: > * Introduce the spi_mem_generic_supports_op_helper() which takes a > capabilities structure. This helper gathers all the checks from > spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These > two helpers now call the new one with either a NULL pointer in the > former case, or a structure with the .dtr parameter set to true in > the latter. Is there a benefit adding an extra NULL check when you could make sure all callers pass a zero-initialized caps object when they don't support fancy features like DTR or ECC.
On Wed, 15 Dec 2021 18:44:26 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Boris, > > boris.brezillon@collabora.com wrote on Tue, 14 Dec 2021 17:24:10 +0100: > > > On Tue, 14 Dec 2021 12:41:39 +0100 > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > This driver can be simplified a little bit by using > > > spi_mem_generic_supports_op() instead of the > > > spi_mem_default/dtr_supports_op() couple. The all_false boolean is > > > inverted to become a dtr boolean, which checks if at least one of the > > > operation member uses dtr mode. The idea behind this change is to > > > simplify the introduction of the pipelined ECC engine. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/spi/spi-mxic.c | 10 +++------- > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > > > index 485a7f2afb44..5e71aa630504 100644 > > > --- a/drivers/spi/spi-mxic.c > > > +++ b/drivers/spi/spi-mxic.c > > > @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc, > > > static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > > const struct spi_mem_op *op) > > > { > > > - bool all_false; > > > + struct spi_mem_controller_caps caps = {}; > > > > > > if (op->data.buswidth > 8 || op->addr.buswidth > 8 || > > > op->dummy.buswidth > 8 || op->cmd.buswidth > 8) > > > @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > > if (op->addr.nbytes > 7) > > > return false; > > > > > > - all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr && > > > - !op->data.dtr; > > > + caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr; > > > > Are you sure that's what you want to do? spi_mem_controller_caps is > > supposed to encode the controller capabilities, not whether the > > operation contains a DTR cycle or not. I'd expect this caps object to be > > statically defined, with possibly one instance per-compat if the caps > > depend on the HW revision. > > In order to keep the series easy to review I decided to go for the > following approach: > * Introduce the spi_mem_generic_supports_op_helper() which takes a > capabilities structure. This helper gathers all the checks from > spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These > two helpers now call the new one with either a NULL pointer in the > former case, or a structure with the .dtr parameter set to true in > the latter. > * Change the API of spi_mem_default_supports_op(), this involves > updating many different drivers so this change does only that in a > very transparent way, with no functional changes at all. All the > drivers provide a NULL parameter for the capabilities structure. > * Actually make use of the new parameter of > spi_mem_default_supports_op() in the drivers Cadence and Macronix, > which do have DTR support. This kills the spi_mem_dtr_supports_op() > helper. > * Kill the temporary spi_mem_generic_supports_op() helper by moving > all the logic back into spi_mem_default_supports_op(). > > This approach is really straightforward and easily bisectable if > needed. There's also a second option that doesn't involve patching existing users: add a spi_mem_controller_caps to the spi_controller struct, and check this instance in your spi_mem_default_supports_op() implementation. Note that the buswidth check done in the generic helper is already based on caps exposed by the controller through spi_controller.mode_bits ({RX/TX}_{DUAL,QUAD,OCTAL} bits). > While working on this, I fixed the check we discussed on IRC > about the command parameter when in a DTR operation. I also reverted > the logic in the various checks, as you suggested. > > Thanks, > Miquèl
On Wed, Dec 15, 2021 at 08:05:48PM +0100, Boris Brezillon wrote: > There's also a second option that doesn't involve patching existing > users: add a spi_mem_controller_caps to the spi_controller struct, and > check this instance in your spi_mem_default_supports_op() > implementation. Note that the buswidth check done in the generic > helper is already based on caps exposed by the controller > through spi_controller.mode_bits ({RX/TX}_{DUAL,QUAD,OCTAL} bits). This approach is quite nice for things like this - having things as data rather than code. The only issue is if any of the caps end up varying by operation and we need different capabilities but that doesn't look too likely here I think?
Hi Boris, Mark, broonie@kernel.org wrote on Wed, 15 Dec 2021 19:19:11 +0000: > On Wed, Dec 15, 2021 at 08:05:48PM +0100, Boris Brezillon wrote: > > > There's also a second option that doesn't involve patching existing > > users: add a spi_mem_controller_caps to the spi_controller struct, and > > check this instance in your spi_mem_default_supports_op() > > implementation. Note that the buswidth check done in the generic > > helper is already based on caps exposed by the controller > > through spi_controller.mode_bits ({RX/TX}_{DUAL,QUAD,OCTAL} bits). > > This approach is quite nice for things like this - having things as data > rather than code. The only issue is if any of the caps end up varying > by operation and we need different capabilities but that doesn't look > too likely here I think? Sure, that's a good idea, I'll look into it. Thanks, Miquèl
Hi Boris, boris.brezillon@collabora.com wrote on Wed, 15 Dec 2021 19:45:38 +0100: > On Wed, 15 Dec 2021 18:44:26 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Hi Boris, > > > > boris.brezillon@collabora.com wrote on Tue, 14 Dec 2021 17:24:10 +0100: > > > > > On Tue, 14 Dec 2021 12:41:39 +0100 > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > This driver can be simplified a little bit by using > > > > spi_mem_generic_supports_op() instead of the > > > > spi_mem_default/dtr_supports_op() couple. The all_false boolean is > > > > inverted to become a dtr boolean, which checks if at least one of the > > > > operation member uses dtr mode. The idea behind this change is to > > > > simplify the introduction of the pipelined ECC engine. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/spi/spi-mxic.c | 10 +++------- > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > > > > index 485a7f2afb44..5e71aa630504 100644 > > > > --- a/drivers/spi/spi-mxic.c > > > > +++ b/drivers/spi/spi-mxic.c > > > > @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc, > > > > static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > > > const struct spi_mem_op *op) > > > > { > > > > - bool all_false; > > > > + struct spi_mem_controller_caps caps = {}; > > > > > > > > if (op->data.buswidth > 8 || op->addr.buswidth > 8 || > > > > op->dummy.buswidth > 8 || op->cmd.buswidth > 8) > > > > @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > > > if (op->addr.nbytes > 7) > > > > return false; > > > > > > > > - all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr && > > > > - !op->data.dtr; > > > > + caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr; > > > > > > Are you sure that's what you want to do? spi_mem_controller_caps is > > > supposed to encode the controller capabilities, not whether the > > > operation contains a DTR cycle or not. I'd expect this caps object to be > > > statically defined, with possibly one instance per-compat if the caps > > > depend on the HW revision. > > > > In order to keep the series easy to review I decided to go for the > > following approach: > > * Introduce the spi_mem_generic_supports_op_helper() which takes a > > capabilities structure. This helper gathers all the checks from > > spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These > > two helpers now call the new one with either a NULL pointer in the > > former case, or a structure with the .dtr parameter set to true in > > the latter. > > * Change the API of spi_mem_default_supports_op(), this involves > > updating many different drivers so this change does only that in a > > very transparent way, with no functional changes at all. All the > > drivers provide a NULL parameter for the capabilities structure. > > * Actually make use of the new parameter of > > spi_mem_default_supports_op() in the drivers Cadence and Macronix, > > which do have DTR support. This kills the spi_mem_dtr_supports_op() > > helper. > > * Kill the temporary spi_mem_generic_supports_op() helper by moving > > all the logic back into spi_mem_default_supports_op(). > > > > This approach is really straightforward and easily bisectable if > > needed. > > I don't really see the benefit of converting drivers one by one for > such a trivial/mechanical change to be honest. After all, we're talking > about adding a caps argument to spi_mem_default_supports_op() and > making all existing callers pass a caps object that's zero initialized. > I hope we can get this right in one shot... I'm not converting them one by one, but all in one patch. One by one is not even an option because it would completely break bisectability. What I meant is that in a single patch, I was converting all the drivers using spi_mem_default_supports_op() to pass a third parameter. In *all* cases, this parameter would be NULL (because I find this cleaner than providing empty structures for 95% of the drivers). In the next patch, I would actually convert the users of spi_mem_dtr_supports_op() to use spi_mem_default_supports_op() by providing their static capabilities definition. Thanks, Miquèl
Hi Boris, boris.brezillon@collabora.com wrote on Wed, 15 Dec 2021 19:52:19 +0100: > On Wed, 15 Dec 2021 18:44:26 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > In order to keep the series easy to review I decided to go for the > > following approach: > > * Introduce the spi_mem_generic_supports_op_helper() which takes a > > capabilities structure. This helper gathers all the checks from > > spi_mem_default_supports_op() and spi_mem_dtr_supports_op(). These > > two helpers now call the new one with either a NULL pointer in the > > former case, or a structure with the .dtr parameter set to true in > > the latter. > > Is there a benefit adding an extra NULL check when you could make sure > all callers pass a zero-initialized caps object when they don't support > fancy features like DTR or ECC. That's exactly my point, I really don't like the creation of 15 empty and useless structures while we could just have a check. If the controller provides no capabilities, we assure he has none. I don't think checking "if (caps && caps->PARAM)" hurts. Anyway, if we go for the spi_mem controller internal structure approach, we might just not need those. Thanks, Miquèl
Hi Mark, broonie@kernel.org wrote on Wed, 15 Dec 2021 19:19:11 +0000: > On Wed, Dec 15, 2021 at 08:05:48PM +0100, Boris Brezillon wrote: > > > There's also a second option that doesn't involve patching existing > > users: add a spi_mem_controller_caps to the spi_controller struct, and > > check this instance in your spi_mem_default_supports_op() > > implementation. Note that the buswidth check done in the generic > > helper is already based on caps exposed by the controller > > through spi_controller.mode_bits ({RX/TX}_{DUAL,QUAD,OCTAL} bits). > > This approach is quite nice for things like this - having things as data > rather than code. The only issue is if any of the caps end up varying > by operation and we need different capabilities but that doesn't look > too likely here I think? Indeed that was the main point of the original review from Boris, the capabilities should be fixed on the controller's lifetime. So I believe we are safe. I think I am going to propose the following: const struct spi_controller_mem_ops *mem_ops; + struct spi_controller_mem_caps mem_caps; As the structure is not supposed to enlarge dramatically in the near future, I guess it's fine to have it defined statically. Please tell me if you prefer a *mem_caps pointer. I'll send a proposal soon. Thanks, Miquèl
miquel.raynal@bootlin.com wrote on Thu, 16 Dec 2021 10:01:47 +0100: > Hi Mark, > > broonie@kernel.org wrote on Wed, 15 Dec 2021 19:19:11 +0000: > > > On Wed, Dec 15, 2021 at 08:05:48PM +0100, Boris Brezillon wrote: > > > > > There's also a second option that doesn't involve patching existing > > > users: add a spi_mem_controller_caps to the spi_controller struct, and > > > check this instance in your spi_mem_default_supports_op() > > > implementation. Note that the buswidth check done in the generic > > > helper is already based on caps exposed by the controller > > > through spi_controller.mode_bits ({RX/TX}_{DUAL,QUAD,OCTAL} bits). > > > > This approach is quite nice for things like this - having things as data > > rather than code. The only issue is if any of the caps end up varying > > by operation and we need different capabilities but that doesn't look > > too likely here I think? > > Indeed that was the main point of the original review from Boris, the > capabilities should be fixed on the controller's lifetime. So I believe > we are safe. > > I think I am going to propose the following: > const struct spi_controller_mem_ops *mem_ops; > + struct spi_controller_mem_caps mem_caps; > > As the structure is not supposed to enlarge dramatically in the near > future, I guess it's fine to have it defined statically. Please tell me > if you prefer a *mem_caps pointer. Actually as the spi-mem.h header is not included in spi.h, it makes defining a static mem_caps entry harder. I'll go for another approach. Maybe putting the capabilities within the spi_controller_mem_ops structure, as these are highly related. > I'll send a proposal soon. > > Thanks, > Miquèl Thanks, Miquèl
On Thu, Dec 16, 2021 at 10:57:39AM +0100, Miquel Raynal wrote: > Actually as the spi-mem.h header is not included in spi.h, it makes > defining a static mem_caps entry harder. I'll go for another approach. > Maybe putting the capabilities within the spi_controller_mem_ops > structure, as these are highly related. Yeah, or putting a pointer to a static declaration of the caps in there rather than the caps directly.
Hi Mark, broonie@kernel.org wrote on Thu, 16 Dec 2021 14:04:18 +0000: > On Thu, Dec 16, 2021 at 10:57:39AM +0100, Miquel Raynal wrote: > > > Actually as the spi-mem.h header is not included in spi.h, it makes > > defining a static mem_caps entry harder. I'll go for another approach. > > Maybe putting the capabilities within the spi_controller_mem_ops > > structure, as these are highly related. > > Yeah, or putting a pointer to a static declaration of the caps in there > rather than the caps directly. Yeah, that's what I ended up doing. Each controller driver supporting mem-ops must provide a capabilities structure. Drivers without specific capabilities can just reference the static &spi_mem_no_caps struct instead of defining their empty one. Thanks, Miquèl
diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c index 485a7f2afb44..5e71aa630504 100644 --- a/drivers/spi/spi-mxic.c +++ b/drivers/spi/spi-mxic.c @@ -452,7 +452,7 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc, static bool mxic_spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) { - bool all_false; + struct spi_mem_controller_caps caps = {}; if (op->data.buswidth > 8 || op->addr.buswidth > 8 || op->dummy.buswidth > 8 || op->cmd.buswidth > 8) @@ -465,13 +465,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem, if (op->addr.nbytes > 7) return false; - all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr && - !op->data.dtr; + caps.dtr = op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr; - if (all_false) - return spi_mem_default_supports_op(mem, op); - else - return spi_mem_dtr_supports_op(mem, op); + return spi_mem_generic_supports_op(mem, op, &caps); } static int mxic_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
This driver can be simplified a little bit by using spi_mem_generic_supports_op() instead of the spi_mem_default/dtr_supports_op() couple. The all_false boolean is inverted to become a dtr boolean, which checks if at least one of the operation member uses dtr mode. The idea behind this change is to simplify the introduction of the pipelined ECC engine. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/spi/spi-mxic.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)