| Message ID | 20211217161654.367782-7-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | External ECC engines & Macronix support | expand |
On 17/12/21 05:16PM, Miquel Raynal wrote: > Now that spi_mem_default_supports_op() has access to the static > controller capabilities (related to memory operations), and now that > these capabilities have been filled by the impacted controllers, there > is no need for a specific helper checking only DTR operations, so let's > just kill spi_mem_dtr_supports_op() and simply use > spi_mem_default_supports_op() instead. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/spi/spi-cadence-quadspi.c | 5 +---- > drivers/spi/spi-mem.c | 10 ---------- > drivers/spi/spi-mxic.c | 10 +--------- > include/linux/spi/spi-mem.h | 11 ----------- > 4 files changed, 2 insertions(+), 34 deletions(-) > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > index e137b1ec85d4..67d05ee8d6a0 100644 > --- a/drivers/spi/spi-mxic.c > +++ b/drivers/spi/spi-mxic.c > @@ -335,8 +335,6 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf, > static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > const struct spi_mem_op *op) > { > - bool all_false; > - > if (op->data.buswidth > 8 || op->addr.buswidth > 8 || > op->dummy.buswidth > 8 || op->cmd.buswidth > 8) > return false; > @@ -348,13 +346,7 @@ 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; > - > - if (all_false) > - return spi_mem_default_supports_op(mem, op); > - else > - return spi_mem_dtr_supports_op(mem, op); > + return spi_mem_default_supports_op(mem, op); Does this controller support mixed DTR modes? If it doesn't then it should reject mixed DTR ops before calling spi_mem_default_supports_op(). Anyway, the current driver doesn't check for it either so this change does not make anything worse at the very least. Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
Hi Pratyush, p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:28:42 +0530: > On 17/12/21 05:16PM, Miquel Raynal wrote: > > Now that spi_mem_default_supports_op() has access to the static > > controller capabilities (related to memory operations), and now that > > these capabilities have been filled by the impacted controllers, there > > is no need for a specific helper checking only DTR operations, so let's > > just kill spi_mem_dtr_supports_op() and simply use > > spi_mem_default_supports_op() instead. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/spi/spi-cadence-quadspi.c | 5 +---- > > drivers/spi/spi-mem.c | 10 ---------- > > drivers/spi/spi-mxic.c | 10 +--------- > > include/linux/spi/spi-mem.h | 11 ----------- > > 4 files changed, 2 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > > index e137b1ec85d4..67d05ee8d6a0 100644 > > --- a/drivers/spi/spi-mxic.c > > +++ b/drivers/spi/spi-mxic.c > > @@ -335,8 +335,6 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf, > > static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > const struct spi_mem_op *op) > > { > > - bool all_false; > > - > > if (op->data.buswidth > 8 || op->addr.buswidth > 8 || > > op->dummy.buswidth > 8 || op->cmd.buswidth > 8) > > return false; > > @@ -348,13 +346,7 @@ 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; > > - > > - if (all_false) > > - return spi_mem_default_supports_op(mem, op); > > - else > > - return spi_mem_dtr_supports_op(mem, op); > > + return spi_mem_default_supports_op(mem, op); > > Does this controller support mixed DTR modes? If it doesn't then it > should reject mixed DTR ops before calling > spi_mem_default_supports_op(). Anyway, the current driver doesn't check > for it either so this change does not make anything worse at the very > least. The Cadence SPI driver does something like: all_true = cmd.dtr && addr.dtr ...; all_false = !cmd.dtr && !addr.dtr ...; if (!all_true || !all_false) return false; This basically rules out any mixed DTR operation. I believe Macronix code is inspired from this function, but they intentionally dropped the all_true check, making the situation boolean: either there is at least one DTR op, or there is none. So I believe this was done on purpose and this controller supports mixed DTR ops. > Reviewed-by: Pratyush Yadav <p.yadav@ti.com> Thanks, Miquèl
On 21/12/21 10:58AM, Miquel Raynal wrote: > Hi Pratyush, > > p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:28:42 +0530: > > > On 17/12/21 05:16PM, Miquel Raynal wrote: > > > Now that spi_mem_default_supports_op() has access to the static > > > controller capabilities (related to memory operations), and now that > > > these capabilities have been filled by the impacted controllers, there > > > is no need for a specific helper checking only DTR operations, so let's > > > just kill spi_mem_dtr_supports_op() and simply use > > > spi_mem_default_supports_op() instead. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/spi/spi-cadence-quadspi.c | 5 +---- > > > drivers/spi/spi-mem.c | 10 ---------- > > > drivers/spi/spi-mxic.c | 10 +--------- > > > include/linux/spi/spi-mem.h | 11 ----------- > > > 4 files changed, 2 insertions(+), 34 deletions(-) > > > > > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > > > index e137b1ec85d4..67d05ee8d6a0 100644 > > > --- a/drivers/spi/spi-mxic.c > > > +++ b/drivers/spi/spi-mxic.c > > > @@ -335,8 +335,6 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf, > > > static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > > const struct spi_mem_op *op) > > > { > > > - bool all_false; > > > - > > > if (op->data.buswidth > 8 || op->addr.buswidth > 8 || > > > op->dummy.buswidth > 8 || op->cmd.buswidth > 8) > > > return false; > > > @@ -348,13 +346,7 @@ 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; > > > - > > > - if (all_false) > > > - return spi_mem_default_supports_op(mem, op); > > > - else > > > - return spi_mem_dtr_supports_op(mem, op); > > > + return spi_mem_default_supports_op(mem, op); > > > > Does this controller support mixed DTR modes? If it doesn't then it > > should reject mixed DTR ops before calling > > spi_mem_default_supports_op(). Anyway, the current driver doesn't check > > for it either so this change does not make anything worse at the very > > least. > > The Cadence SPI driver does something like: > > all_true = cmd.dtr && addr.dtr ...; > all_false = !cmd.dtr && !addr.dtr ...; > if (!all_true || !all_false) > return false; > > This basically rules out any mixed DTR operation. > > I believe Macronix code is inspired from this function, but they > intentionally dropped the all_true check, making the situation boolean: > either there is at least one DTR op, or there is none. So I believe > this was done on purpose and this controller supports mixed DTR ops. I see that your other patches in this series touch this driver so you should have the datasheet right? Can you look and see for sure if it does? I am fine with the patch as it is but if we can make the check stricter it would be nice. > > > Reviewed-by: Pratyush Yadav <p.yadav@ti.com> > > Thanks, > Miquèl
Hi Pratyush, p.yadav@ti.com wrote on Tue, 21 Dec 2021 15:40:54 +0530: > On 21/12/21 10:58AM, Miquel Raynal wrote: > > Hi Pratyush, > > > > p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:28:42 +0530: > > > > > On 17/12/21 05:16PM, Miquel Raynal wrote: > > > > Now that spi_mem_default_supports_op() has access to the static > > > > controller capabilities (related to memory operations), and now that > > > > these capabilities have been filled by the impacted controllers, there > > > > is no need for a specific helper checking only DTR operations, so let's > > > > just kill spi_mem_dtr_supports_op() and simply use > > > > spi_mem_default_supports_op() instead. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/spi/spi-cadence-quadspi.c | 5 +---- > > > > drivers/spi/spi-mem.c | 10 ---------- > > > > drivers/spi/spi-mxic.c | 10 +--------- > > > > include/linux/spi/spi-mem.h | 11 ----------- > > > > 4 files changed, 2 insertions(+), 34 deletions(-) > > > > > > > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > > > > index e137b1ec85d4..67d05ee8d6a0 100644 > > > > --- a/drivers/spi/spi-mxic.c > > > > +++ b/drivers/spi/spi-mxic.c > > > > @@ -335,8 +335,6 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf, > > > > static bool mxic_spi_mem_supports_op(struct spi_mem *mem, > > > > const struct spi_mem_op *op) > > > > { > > > > - bool all_false; > > > > - > > > > if (op->data.buswidth > 8 || op->addr.buswidth > 8 || > > > > op->dummy.buswidth > 8 || op->cmd.buswidth > 8) > > > > return false; > > > > @@ -348,13 +346,7 @@ 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; > > > > - > > > > - if (all_false) > > > > - return spi_mem_default_supports_op(mem, op); > > > > - else > > > > - return spi_mem_dtr_supports_op(mem, op); > > > > + return spi_mem_default_supports_op(mem, op); > > > > > > Does this controller support mixed DTR modes? If it doesn't then it > > > should reject mixed DTR ops before calling > > > spi_mem_default_supports_op(). Anyway, the current driver doesn't check > > > for it either so this change does not make anything worse at the very > > > least. > > > > The Cadence SPI driver does something like: > > > > all_true = cmd.dtr && addr.dtr ...; > > all_false = !cmd.dtr && !addr.dtr ...; > > if (!all_true || !all_false) > > return false; > > > > This basically rules out any mixed DTR operation. > > > > I believe Macronix code is inspired from this function, but they > > intentionally dropped the all_true check, making the situation boolean: > > either there is at least one DTR op, or there is none. So I believe > > this was done on purpose and this controller supports mixed DTR ops. > > I see that your other patches in this series touch this driver so you > should have the datasheet right? Can you look and see for sure if it > does? I am fine with the patch as it is but if we can make the check > stricter it would be nice. As discussed on IRC, I've checked the datasheet and indeed the following modes are supported: * 1S-1D-1D * 1S-2D-2D * 1S-4D-4D * 4S-4D-4D So yes, even if there is no use of these modes yet, certain mixed modes are supported by this controller. > > > > > > Reviewed-by: Pratyush Yadav <p.yadav@ti.com> Thanks, Miquèl
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index 98e0cc4236e3..bc6b33bce0bb 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -1252,10 +1252,7 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem, if (!(all_true || all_false)) return false; - if (all_true) - return spi_mem_dtr_supports_op(mem, op); - else - return spi_mem_default_supports_op(mem, op); + return spi_mem_default_supports_op(mem, op); } static int cqspi_of_get_flash_pdata(struct platform_device *pdev, diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 677e54221ebc..cfe1c99db5f3 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -160,16 +160,6 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem, return true; } -bool spi_mem_dtr_supports_op(struct spi_mem *mem, - const struct spi_mem_op *op) -{ - if (op->cmd.dtr && op->cmd.nbytes != 2) - return false; - - return spi_mem_check_buswidth(mem, op); -} -EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op); - bool spi_mem_default_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) { diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c index e137b1ec85d4..67d05ee8d6a0 100644 --- a/drivers/spi/spi-mxic.c +++ b/drivers/spi/spi-mxic.c @@ -335,8 +335,6 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf, static bool mxic_spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) { - bool all_false; - if (op->data.buswidth > 8 || op->addr.buswidth > 8 || op->dummy.buswidth > 8 || op->cmd.buswidth > 8) return false; @@ -348,13 +346,7 @@ 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; - - if (all_false) - return spi_mem_default_supports_op(mem, op); - else - return spi_mem_dtr_supports_op(mem, op); + return spi_mem_default_supports_op(mem, op); } static int mxic_spi_mem_exec_op(struct spi_mem *mem, diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h index 045ecb7c6f50..d7787c8f3746 100644 --- a/include/linux/spi/spi-mem.h +++ b/include/linux/spi/spi-mem.h @@ -332,10 +332,6 @@ void spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr, bool spi_mem_default_supports_op(struct spi_mem *mem, const struct spi_mem_op *op); - -bool spi_mem_dtr_supports_op(struct spi_mem *mem, - const struct spi_mem_op *op); - #else static inline int spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr, @@ -358,13 +354,6 @@ bool spi_mem_default_supports_op(struct spi_mem *mem, { return false; } - -static inline -bool spi_mem_dtr_supports_op(struct spi_mem *mem, - const struct spi_mem_op *op) -{ - return false; -} #endif /* CONFIG_SPI_MEM */ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op);
Now that spi_mem_default_supports_op() has access to the static controller capabilities (related to memory operations), and now that these capabilities have been filled by the impacted controllers, there is no need for a specific helper checking only DTR operations, so let's just kill spi_mem_dtr_supports_op() and simply use spi_mem_default_supports_op() instead. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/spi/spi-cadence-quadspi.c | 5 +---- drivers/spi/spi-mem.c | 10 ---------- drivers/spi/spi-mxic.c | 10 +--------- include/linux/spi/spi-mem.h | 11 ----------- 4 files changed, 2 insertions(+), 34 deletions(-)