| Message ID | 20220331110819.133392-1-matthias.schiffer@ew.tq-group.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 97e4827d775faa9a32b5e1a97959c69dd77d17a3 |
| Headers | show |
| Series | spi: cadence-quadspi: fix protocol setup for non-1-1-X operations | expand |
Hi Matthias, On 31/03/22 01:08PM, Matthias Schiffer wrote: > cqspi_set_protocol() only set the data width, but ignored the command > and address width (except for 8-8-8 DTR ops), leading to corruption of > all transfers using 1-X-X or X-X-X ops. Fix by setting the other two > widths as well. > > While we're at it, simplify the code a bit by replacing the > CQSPI_INST_TYPE_* constants with ilog2(). > > Tested on a TI AM64x with a Macronix MX25U51245G QSPI flash with 1-4-4 > read and write operations. > > Fixes: a314f6367787 ("mtd: spi-nor: Convert cadence-quadspi to use spi-mem framework") I think a fixes tag is wrong here. The old driver did not support 1-X-X modes either. So you are not fixing anything, you are adding a new feature. I don't think we should backport this patch to stable. > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > --- > drivers/spi/spi-cadence-quadspi.c | 46 ++++++++----------------------- > 1 file changed, 12 insertions(+), 34 deletions(-) > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > index b0c9f62ccefb..616ada891974 100644 > --- a/drivers/spi/spi-cadence-quadspi.c > +++ b/drivers/spi/spi-cadence-quadspi.c > @@ -19,6 +19,7 @@ > #include <linux/iopoll.h> > #include <linux/jiffies.h> > #include <linux/kernel.h> > +#include <linux/log2.h> > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/of.h> > @@ -102,12 +103,6 @@ struct cqspi_driver_platdata { > #define CQSPI_TIMEOUT_MS 500 > #define CQSPI_READ_TIMEOUT_MS 10 > > -/* Instruction type */ > -#define CQSPI_INST_TYPE_SINGLE 0 > -#define CQSPI_INST_TYPE_DUAL 1 > -#define CQSPI_INST_TYPE_QUAD 2 > -#define CQSPI_INST_TYPE_OCTAL 3 > - > #define CQSPI_DUMMY_CLKS_PER_BYTE 8 > #define CQSPI_DUMMY_BYTES_MAX 4 > #define CQSPI_DUMMY_CLKS_MAX 31 > @@ -376,10 +371,6 @@ static unsigned int cqspi_calc_dummy(const struct spi_mem_op *op, bool dtr) > static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, > const struct spi_mem_op *op) > { > - f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE; > - f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE; > - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; > - > /* > * For an op to be DTR, cmd phase along with every other non-empty > * phase should have dtr field set to 1. If an op phase has zero > @@ -389,32 +380,23 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, > (!op->addr.nbytes || op->addr.dtr) && > (!op->data.nbytes || op->data.dtr); > > - switch (op->data.buswidth) { > - case 0: > - break; > - case 1: > - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; > - break; > - case 2: > - f_pdata->data_width = CQSPI_INST_TYPE_DUAL; > - break; > - case 4: > - f_pdata->data_width = CQSPI_INST_TYPE_QUAD; > - break; > - case 8: > - f_pdata->data_width = CQSPI_INST_TYPE_OCTAL; > - break; > - default: > - return -EINVAL; > - } > + f_pdata->inst_width = 0; > + if (op->cmd.buswidth) > + f_pdata->inst_width = ilog2(op->cmd.buswidth); > + > + f_pdata->addr_width = 0; > + if (op->addr.buswidth) > + f_pdata->addr_width = ilog2(op->addr.buswidth); > + > + f_pdata->data_width = 0; > + if (op->data.buswidth) > + f_pdata->data_width = ilog2(op->data.buswidth); Honestly, I think we should get rid of cqspi_set_protocol() entirely. I see no need to store f_pdata->{instr,addr,data}_width since we recalculate those for each op execution anyway. So why not just use the spi_mem_op to get those values directly and be rid of all this mess? > > /* Right now we only support 8-8-8 DTR mode. */ > if (f_pdata->dtr) { > switch (op->cmd.buswidth) { > case 0: > - break; > case 8: > - f_pdata->inst_width = CQSPI_INST_TYPE_OCTAL; > break; > default: > return -EINVAL; > @@ -422,9 +404,7 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, > > switch (op->addr.buswidth) { > case 0: > - break; > case 8: > - f_pdata->addr_width = CQSPI_INST_TYPE_OCTAL; > break; > default: > return -EINVAL; > @@ -432,9 +412,7 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, > > switch (op->data.buswidth) { > case 0: > - break; > case 8: > - f_pdata->data_width = CQSPI_INST_TYPE_OCTAL; > break; > default: > return -EINVAL; > -- > 2.25.1 >
On Fri, 2022-04-01 at 15:36 +0530, Pratyush Yadav wrote: > Hi Matthias, > > On 31/03/22 01:08PM, Matthias Schiffer wrote: > > cqspi_set_protocol() only set the data width, but ignored the > > command > > and address width (except for 8-8-8 DTR ops), leading to corruption > > of > > all transfers using 1-X-X or X-X-X ops. Fix by setting the other > > two > > widths as well. > > > > While we're at it, simplify the code a bit by replacing the > > CQSPI_INST_TYPE_* constants with ilog2(). > > > > Tested on a TI AM64x with a Macronix MX25U51245G QSPI flash with 1- > > 4-4 > > read and write operations. > > > > Fixes: a314f6367787 ("mtd: spi-nor: Convert cadence-quadspi to use > > spi-mem framework") > > I think a fixes tag is wrong here. The old driver did not support 1- > X-X > modes either. So you are not fixing anything, you are adding a new > feature. I don't think we should backport this patch to stable. Giving a precise fixes tag is a bit difficult. The referenced commit made the driver (accidentally) accept commands like 1-4-4 without handing them correctly, causing data corruption for flashs that support these modes. The data corruption is fixed by my patch. As the change was unintended, one option would be to split this patch into two parts: One fix patch that makes cqspi_set_protocol() -EINVAL again for all commands that are not 1-1-X, and one feature patch that adds actual support for these commands. My thought process was that making these commands work correctly can't break anything that is not already broken in current stable kernels. But if you prefer the minimal change, I can send a v2 that splits the patch. Regards, Matthias > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com > > > > > --- > > drivers/spi/spi-cadence-quadspi.c | 46 ++++++++------------------- > > ---- > > 1 file changed, 12 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi- > > cadence-quadspi.c > > index b0c9f62ccefb..616ada891974 100644 > > --- a/drivers/spi/spi-cadence-quadspi.c > > +++ b/drivers/spi/spi-cadence-quadspi.c > > @@ -19,6 +19,7 @@ > > #include <linux/iopoll.h> > > #include <linux/jiffies.h> > > #include <linux/kernel.h> > > +#include <linux/log2.h> > > #include <linux/module.h> > > #include <linux/of_device.h> > > #include <linux/of.h> > > @@ -102,12 +103,6 @@ struct cqspi_driver_platdata { > > #define CQSPI_TIMEOUT_MS 500 > > #define CQSPI_READ_TIMEOUT_MS 10 > > > > -/* Instruction type */ > > -#define CQSPI_INST_TYPE_SINGLE 0 > > -#define CQSPI_INST_TYPE_DUAL 1 > > -#define CQSPI_INST_TYPE_QUAD 2 > > -#define CQSPI_INST_TYPE_OCTAL 3 > > - > > #define CQSPI_DUMMY_CLKS_PER_BYTE 8 > > #define CQSPI_DUMMY_BYTES_MAX 4 > > #define CQSPI_DUMMY_CLKS_MAX 31 > > @@ -376,10 +371,6 @@ static unsigned int cqspi_calc_dummy(const > > struct spi_mem_op *op, bool dtr) > > static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, > > const struct spi_mem_op *op) > > { > > - f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE; > > - f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE; > > - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; > > - > > /* > > * For an op to be DTR, cmd phase along with every other non- > > empty > > * phase should have dtr field set to 1. If an op phase has > > zero > > @@ -389,32 +380,23 @@ static int cqspi_set_protocol(struct > > cqspi_flash_pdata *f_pdata, > > (!op->addr.nbytes || op->addr.dtr) && > > (!op->data.nbytes || op->data.dtr); > > > > - switch (op->data.buswidth) { > > - case 0: > > - break; > > - case 1: > > - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; > > - break; > > - case 2: > > - f_pdata->data_width = CQSPI_INST_TYPE_DUAL; > > - break; > > - case 4: > > - f_pdata->data_width = CQSPI_INST_TYPE_QUAD; > > - break; > > - case 8: > > - f_pdata->data_width = CQSPI_INST_TYPE_OCTAL; > > - break; > > - default: > > - return -EINVAL; > > - } > > + f_pdata->inst_width = 0; > > + if (op->cmd.buswidth) > > + f_pdata->inst_width = ilog2(op->cmd.buswidth); > > + > > + f_pdata->addr_width = 0; > > + if (op->addr.buswidth) > > + f_pdata->addr_width = ilog2(op->addr.buswidth); > > + > > + f_pdata->data_width = 0; > > + if (op->data.buswidth) > > + f_pdata->data_width = ilog2(op->data.buswidth); > > Honestly, I think we should get rid of cqspi_set_protocol() entirely. > I > see no need to store f_pdata->{instr,addr,data}_width since we > recalculate those for each op execution anyway. So why not just use > the > spi_mem_op to get those values directly and be rid of all this mess? > > > > > /* Right now we only support 8-8-8 DTR mode. */ > > if (f_pdata->dtr) { > > switch (op->cmd.buswidth) { > > case 0: > > - break; > > case 8: > > - f_pdata->inst_width = CQSPI_INST_TYPE_OCTAL; > > break; > > default: > > return -EINVAL; > > @@ -422,9 +404,7 @@ static int cqspi_set_protocol(struct > > cqspi_flash_pdata *f_pdata, > > > > switch (op->addr.buswidth) { > > case 0: > > - break; > > case 8: > > - f_pdata->addr_width = CQSPI_INST_TYPE_OCTAL; > > break; > > default: > > return -EINVAL; > > @@ -432,9 +412,7 @@ static int cqspi_set_protocol(struct > > cqspi_flash_pdata *f_pdata, > > > > switch (op->data.buswidth) { > > case 0: > > - break; > > case 8: > > - f_pdata->data_width = CQSPI_INST_TYPE_OCTAL; > > break; > > default: > > return -EINVAL; > > -- > > 2.25.1 > >
On 01/04/22 12:20PM, Matthias Schiffer wrote: > On Fri, 2022-04-01 at 15:36 +0530, Pratyush Yadav wrote: > > Hi Matthias, > > > > On 31/03/22 01:08PM, Matthias Schiffer wrote: > > > cqspi_set_protocol() only set the data width, but ignored the > > > command > > > and address width (except for 8-8-8 DTR ops), leading to corruption > > > of > > > all transfers using 1-X-X or X-X-X ops. Fix by setting the other > > > two > > > widths as well. > > > > > > While we're at it, simplify the code a bit by replacing the > > > CQSPI_INST_TYPE_* constants with ilog2(). > > > > > > Tested on a TI AM64x with a Macronix MX25U51245G QSPI flash with 1- > > > 4-4 > > > read and write operations. > > > > > > Fixes: a314f6367787 ("mtd: spi-nor: Convert cadence-quadspi to use > > > spi-mem framework") > > > > I think a fixes tag is wrong here. The old driver did not support 1- > > X-X > > modes either. So you are not fixing anything, you are adding a new > > feature. I don't think we should backport this patch to stable. > > > Giving a precise fixes tag is a bit difficult. The referenced commit > made the driver (accidentally) accept commands like 1-4-4 without > handing them correctly, causing data corruption for flashs that support > these modes. The data corruption is fixed by my patch. Ah, you're right. I missed the fact that the original driver explicitly checked for 1-1-1, 1-1-4, and 1-1-8, and returned an error for the rest. So your patch does indeed fix a bug. > > As the change was unintended, one option would be to split this patch > into two parts: One fix patch that makes cqspi_set_protocol() -EINVAL > again for all commands that are not 1-1-X, and one feature patch that > adds actual support for these commands. > > My thought process was that making these commands work correctly can't > break anything that is not already broken in current stable kernels. > But if you prefer the minimal change, I can send a v2 that splits the > patch. Yes, but as I commented below, I would prefer you rework the driver to drop cqspi_set_protocol() entirely. For doing that the 2 patch approach would work best so we don't end up getting the new code backported to stable as well. > > Regards, > Matthias > > > > > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com > > > > > > > --- > > > drivers/spi/spi-cadence-quadspi.c | 46 ++++++++------------------- > > > ---- > > > 1 file changed, 12 insertions(+), 34 deletions(-) > > > > > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi- > > > cadence-quadspi.c > > > index b0c9f62ccefb..616ada891974 100644 > > > --- a/drivers/spi/spi-cadence-quadspi.c > > > +++ b/drivers/spi/spi-cadence-quadspi.c > > > @@ -19,6 +19,7 @@ > > > #include <linux/iopoll.h> > > > #include <linux/jiffies.h> > > > #include <linux/kernel.h> > > > +#include <linux/log2.h> > > > #include <linux/module.h> > > > #include <linux/of_device.h> > > > #include <linux/of.h> > > > @@ -102,12 +103,6 @@ struct cqspi_driver_platdata { > > > #define CQSPI_TIMEOUT_MS 500 > > > #define CQSPI_READ_TIMEOUT_MS 10 > > > > > > -/* Instruction type */ > > > -#define CQSPI_INST_TYPE_SINGLE 0 > > > -#define CQSPI_INST_TYPE_DUAL 1 > > > -#define CQSPI_INST_TYPE_QUAD 2 > > > -#define CQSPI_INST_TYPE_OCTAL 3 > > > - > > > #define CQSPI_DUMMY_CLKS_PER_BYTE 8 > > > #define CQSPI_DUMMY_BYTES_MAX 4 > > > #define CQSPI_DUMMY_CLKS_MAX 31 > > > @@ -376,10 +371,6 @@ static unsigned int cqspi_calc_dummy(const > > > struct spi_mem_op *op, bool dtr) > > > static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, > > > const struct spi_mem_op *op) > > > { > > > - f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE; > > > - f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE; > > > - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; > > > - > > > /* > > > * For an op to be DTR, cmd phase along with every other non- > > > empty > > > * phase should have dtr field set to 1. If an op phase has > > > zero > > > @@ -389,32 +380,23 @@ static int cqspi_set_protocol(struct > > > cqspi_flash_pdata *f_pdata, > > > (!op->addr.nbytes || op->addr.dtr) && > > > (!op->data.nbytes || op->data.dtr); > > > > > > - switch (op->data.buswidth) { > > > - case 0: > > > - break; > > > - case 1: > > > - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; > > > - break; > > > - case 2: > > > - f_pdata->data_width = CQSPI_INST_TYPE_DUAL; > > > - break; > > > - case 4: > > > - f_pdata->data_width = CQSPI_INST_TYPE_QUAD; > > > - break; > > > - case 8: > > > - f_pdata->data_width = CQSPI_INST_TYPE_OCTAL; > > > - break; > > > - default: > > > - return -EINVAL; > > > - } > > > + f_pdata->inst_width = 0; > > > + if (op->cmd.buswidth) > > > + f_pdata->inst_width = ilog2(op->cmd.buswidth); > > > + > > > + f_pdata->addr_width = 0; > > > + if (op->addr.buswidth) > > > + f_pdata->addr_width = ilog2(op->addr.buswidth); > > > + > > > + f_pdata->data_width = 0; > > > + if (op->data.buswidth) > > > + f_pdata->data_width = ilog2(op->data.buswidth); > > > > Honestly, I think we should get rid of cqspi_set_protocol() entirely. > > I > > see no need to store f_pdata->{instr,addr,data}_width since we > > recalculate those for each op execution anyway. So why not just use > > the > > spi_mem_op to get those values directly and be rid of all this mess? > > > > > > > > /* Right now we only support 8-8-8 DTR mode. */ > > > if (f_pdata->dtr) { > > > switch (op->cmd.buswidth) { > > > case 0: > > > - break; > > > case 8: > > > - f_pdata->inst_width = CQSPI_INST_TYPE_OCTAL; > > > break; > > > default: > > > return -EINVAL; > > > @@ -422,9 +404,7 @@ static int cqspi_set_protocol(struct > > > cqspi_flash_pdata *f_pdata, > > > > > > switch (op->addr.buswidth) { > > > case 0: > > > - break; > > > case 8: > > > - f_pdata->addr_width = CQSPI_INST_TYPE_OCTAL; > > > break; > > > default: > > > return -EINVAL; > > > @@ -432,9 +412,7 @@ static int cqspi_set_protocol(struct > > > cqspi_flash_pdata *f_pdata, > > > > > > switch (op->data.buswidth) { > > > case 0: > > > - break; > > > case 8: > > > - f_pdata->data_width = CQSPI_INST_TYPE_OCTAL; > > > break; > > > default: > > > return -EINVAL; > > > -- > > > 2.25.1 > > > >
On Wed, 2022-04-06 at 01:07 +0530, Pratyush Yadav wrote: > On 01/04/22 12:20PM, Matthias Schiffer wrote: > > On Fri, 2022-04-01 at 15:36 +0530, Pratyush Yadav wrote: > > > Hi Matthias, > > > > > > On 31/03/22 01:08PM, Matthias Schiffer wrote: > > > > cqspi_set_protocol() only set the data width, but ignored the > > > > command > > > > and address width (except for 8-8-8 DTR ops), leading to > > > > corruption > > > > of > > > > all transfers using 1-X-X or X-X-X ops. Fix by setting the > > > > other > > > > two > > > > widths as well. > > > > > > > > While we're at it, simplify the code a bit by replacing the > > > > CQSPI_INST_TYPE_* constants with ilog2(). > > > > > > > > Tested on a TI AM64x with a Macronix MX25U51245G QSPI flash > > > > with 1- > > > > 4-4 > > > > read and write operations. > > > > > > > > Fixes: a314f6367787 ("mtd: spi-nor: Convert cadence-quadspi to > > > > use > > > > spi-mem framework") > > > > > > I think a fixes tag is wrong here. The old driver did not support > > > 1- > > > X-X > > > modes either. So you are not fixing anything, you are adding a > > > new > > > feature. I don't think we should backport this patch to stable. > > > > Giving a precise fixes tag is a bit difficult. The referenced > > commit > > made the driver (accidentally) accept commands like 1-4-4 without > > handing them correctly, causing data corruption for flashs that > > support > > these modes. The data corruption is fixed by my patch. > > Ah, you're right. I missed the fact that the original driver > explicitly > checked for 1-1-1, 1-1-4, and 1-1-8, and returned an error for the > rest. > So your patch does indeed fix a bug. > > > As the change was unintended, one option would be to split this > > patch > > into two parts: One fix patch that makes cqspi_set_protocol() > > -EINVAL > > again for all commands that are not 1-1-X, and one feature patch > > that > > adds actual support for these commands. > > > > My thought process was that making these commands work correctly > > can't > > break anything that is not already broken in current stable > > kernels. > > But if you prefer the minimal change, I can send a v2 that splits > > the > > patch. > > Yes, but as I commented below, I would prefer you rework the driver > to > drop cqspi_set_protocol() entirely. For doing that the 2 patch > approach > would work best so we don't end up getting the new code backported > to > stable as well. Okay, I tried to split my patch, however I found that cqspi_set_protocol() is not the right place to check for unsupported operations at all - if the driver ever reaches a path where cqspi_set_protocol() returns -EINVAL, something has already gone wrong (and the attemption operation will fail altogether rather than falling back to a slower command). I think that these checks - including the check that only 8-8-8 is supported with DTR - should actually happen in cqspi_supports_mem_op(). Does that sound right? Regards, Matthias > > > Regards, > > Matthias > > > > > > > > > > Signed-off-by: Matthias Schiffer < > > > > matthias.schiffer@ew.tq-group.com > > > > --- > > > > drivers/spi/spi-cadence-quadspi.c | 46 ++++++++--------------- > > > > ---- > > > > ---- > > > > 1 file changed, 12 insertions(+), 34 deletions(-) > > > > > > > > diff --git a/drivers/spi/spi-cadence-quadspi.c > > > > b/drivers/spi/spi- > > > > cadence-quadspi.c > > > > index b0c9f62ccefb..616ada891974 100644 > > > > --- a/drivers/spi/spi-cadence-quadspi.c > > > > +++ b/drivers/spi/spi-cadence-quadspi.c > > > > @@ -19,6 +19,7 @@ > > > > #include <linux/iopoll.h> > > > > #include <linux/jiffies.h> > > > > #include <linux/kernel.h> > > > > +#include <linux/log2.h> > > > > #include <linux/module.h> > > > > #include <linux/of_device.h> > > > > #include <linux/of.h> > > > > @@ -102,12 +103,6 @@ struct cqspi_driver_platdata { > > > > #define CQSPI_TIMEOUT_MS 500 > > > > #define CQSPI_READ_TIMEOUT_MS 10 > > > > > > > > -/* Instruction type */ > > > > -#define CQSPI_INST_TYPE_SINGLE 0 > > > > -#define CQSPI_INST_TYPE_DUAL 1 > > > > -#define CQSPI_INST_TYPE_QUAD 2 > > > > -#define CQSPI_INST_TYPE_OCTAL 3 > > > > - > > > > #define CQSPI_DUMMY_CLKS_PER_BYTE 8 > > > > #define CQSPI_DUMMY_BYTES_MAX 4 > > > > #define CQSPI_DUMMY_CLKS_MAX 31 > > > > @@ -376,10 +371,6 @@ static unsigned int cqspi_calc_dummy(const > > > > struct spi_mem_op *op, bool dtr) > > > > static int cqspi_set_protocol(struct cqspi_flash_pdata > > > > *f_pdata, > > > > const struct spi_mem_op *op) > > > > { > > > > - f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE; > > > > - f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE; > > > > - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; > > > > - > > > > /* > > > > * For an op to be DTR, cmd phase along with every > > > > other non- > > > > empty > > > > * phase should have dtr field set to 1. If an op phase > > > > has > > > > zero > > > > @@ -389,32 +380,23 @@ static int cqspi_set_protocol(struct > > > > cqspi_flash_pdata *f_pdata, > > > > (!op->addr.nbytes || op->addr.dtr) && > > > > (!op->data.nbytes || op->data.dtr); > > > > > > > > - switch (op->data.buswidth) { > > > > - case 0: > > > > - break; > > > > - case 1: > > > > - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; > > > > - break; > > > > - case 2: > > > > - f_pdata->data_width = CQSPI_INST_TYPE_DUAL; > > > > - break; > > > > - case 4: > > > > - f_pdata->data_width = CQSPI_INST_TYPE_QUAD; > > > > - break; > > > > - case 8: > > > > - f_pdata->data_width = CQSPI_INST_TYPE_OCTAL; > > > > - break; > > > > - default: > > > > - return -EINVAL; > > > > - } > > > > + f_pdata->inst_width = 0; > > > > + if (op->cmd.buswidth) > > > > + f_pdata->inst_width = ilog2(op->cmd.buswidth); > > > > + > > > > + f_pdata->addr_width = 0; > > > > + if (op->addr.buswidth) > > > > + f_pdata->addr_width = ilog2(op->addr.buswidth); > > > > + > > > > + f_pdata->data_width = 0; > > > > + if (op->data.buswidth) > > > > + f_pdata->data_width = ilog2(op->data.buswidth); > > > > > > Honestly, I think we should get rid of cqspi_set_protocol() > > > entirely. > > > I > > > see no need to store f_pdata->{instr,addr,data}_width since we > > > recalculate those for each op execution anyway. So why not just > > > use > > > the > > > spi_mem_op to get those values directly and be rid of all this > > > mess? > > > > > > > > > > > /* Right now we only support 8-8-8 DTR mode. */ > > > > if (f_pdata->dtr) { > > > > switch (op->cmd.buswidth) { > > > > case 0: > > > > - break; > > > > case 8: > > > > - f_pdata->inst_width = > > > > CQSPI_INST_TYPE_OCTAL; > > > > break; > > > > default: > > > > return -EINVAL; > > > > @@ -422,9 +404,7 @@ static int cqspi_set_protocol(struct > > > > cqspi_flash_pdata *f_pdata, > > > > > > > > switch (op->addr.buswidth) { > > > > case 0: > > > > - break; > > > > case 8: > > > > - f_pdata->addr_width = > > > > CQSPI_INST_TYPE_OCTAL; > > > > break; > > > > default: > > > > return -EINVAL; > > > > @@ -432,9 +412,7 @@ static int cqspi_set_protocol(struct > > > > cqspi_flash_pdata *f_pdata, > > > > > > > > switch (op->data.buswidth) { > > > > case 0: > > > > - break; > > > > case 8: > > > > - f_pdata->data_width = > > > > CQSPI_INST_TYPE_OCTAL; > > > > break; > > > > default: > > > > return -EINVAL; > > > > -- > > > > 2.25.1 > > > >
On Thu, 31 Mar 2022 13:08:19 +0200, Matthias Schiffer wrote: > cqspi_set_protocol() only set the data width, but ignored the command > and address width (except for 8-8-8 DTR ops), leading to corruption of > all transfers using 1-X-X or X-X-X ops. Fix by setting the other two > widths as well. > > While we're at it, simplify the code a bit by replacing the > CQSPI_INST_TYPE_* constants with ilog2(). > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations commit: 97e4827d775faa9a32b5e1a97959c69dd77d17a3 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index b0c9f62ccefb..616ada891974 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -19,6 +19,7 @@ #include <linux/iopoll.h> #include <linux/jiffies.h> #include <linux/kernel.h> +#include <linux/log2.h> #include <linux/module.h> #include <linux/of_device.h> #include <linux/of.h> @@ -102,12 +103,6 @@ struct cqspi_driver_platdata { #define CQSPI_TIMEOUT_MS 500 #define CQSPI_READ_TIMEOUT_MS 10 -/* Instruction type */ -#define CQSPI_INST_TYPE_SINGLE 0 -#define CQSPI_INST_TYPE_DUAL 1 -#define CQSPI_INST_TYPE_QUAD 2 -#define CQSPI_INST_TYPE_OCTAL 3 - #define CQSPI_DUMMY_CLKS_PER_BYTE 8 #define CQSPI_DUMMY_BYTES_MAX 4 #define CQSPI_DUMMY_CLKS_MAX 31 @@ -376,10 +371,6 @@ static unsigned int cqspi_calc_dummy(const struct spi_mem_op *op, bool dtr) static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, const struct spi_mem_op *op) { - f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE; - f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE; - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; - /* * For an op to be DTR, cmd phase along with every other non-empty * phase should have dtr field set to 1. If an op phase has zero @@ -389,32 +380,23 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, (!op->addr.nbytes || op->addr.dtr) && (!op->data.nbytes || op->data.dtr); - switch (op->data.buswidth) { - case 0: - break; - case 1: - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; - break; - case 2: - f_pdata->data_width = CQSPI_INST_TYPE_DUAL; - break; - case 4: - f_pdata->data_width = CQSPI_INST_TYPE_QUAD; - break; - case 8: - f_pdata->data_width = CQSPI_INST_TYPE_OCTAL; - break; - default: - return -EINVAL; - } + f_pdata->inst_width = 0; + if (op->cmd.buswidth) + f_pdata->inst_width = ilog2(op->cmd.buswidth); + + f_pdata->addr_width = 0; + if (op->addr.buswidth) + f_pdata->addr_width = ilog2(op->addr.buswidth); + + f_pdata->data_width = 0; + if (op->data.buswidth) + f_pdata->data_width = ilog2(op->data.buswidth); /* Right now we only support 8-8-8 DTR mode. */ if (f_pdata->dtr) { switch (op->cmd.buswidth) { case 0: - break; case 8: - f_pdata->inst_width = CQSPI_INST_TYPE_OCTAL; break; default: return -EINVAL; @@ -422,9 +404,7 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, switch (op->addr.buswidth) { case 0: - break; case 8: - f_pdata->addr_width = CQSPI_INST_TYPE_OCTAL; break; default: return -EINVAL; @@ -432,9 +412,7 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, switch (op->data.buswidth) { case 0: - break; case 8: - f_pdata->data_width = CQSPI_INST_TYPE_OCTAL; break; default: return -EINVAL;
cqspi_set_protocol() only set the data width, but ignored the command and address width (except for 8-8-8 DTR ops), leading to corruption of all transfers using 1-X-X or X-X-X ops. Fix by setting the other two widths as well. While we're at it, simplify the code a bit by replacing the CQSPI_INST_TYPE_* constants with ilog2(). Tested on a TI AM64x with a Macronix MX25U51245G QSPI flash with 1-4-4 read and write operations. Fixes: a314f6367787 ("mtd: spi-nor: Convert cadence-quadspi to use spi-mem framework") Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- drivers/spi/spi-cadence-quadspi.c | 46 ++++++++----------------------- 1 file changed, 12 insertions(+), 34 deletions(-)