[PATCH AUTOSEL 5.17 34/49] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[PATCH AUTOSEL 5.17 34/49] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[Sasha Levin]

From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

[ Upstream commit 97e4827d775faa9a32b5e1a97959c69dd77d17a3 ]

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.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Link: https://lore.kernel.org/r/20220331110819.133392-1-matthias.schiffer@ew.tq-group.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 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 b808c94641fa..75f356041138 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;
-- 
2.35.1

Re: [PATCH AUTOSEL 5.17 34/49] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[Matthias Schiffer]

Hi Mark,

what's your plan regarding this patch and the other patch I sent [1]? I
think there has been some confusion regarding which solution we want to
backport to stable kernels (well, at least I'm confused...)

I'm fine with this patch getting backported, but in that case [1]
doesn't make sense anymore (in fact I expected this patch to be dropped
for now when I submitted [1], due to Pratyush Yadav's concerns).

Regards,
Matthias


[1] https://patchwork.kernel.org/project/spi-devel-general/patch/20220406132832.199777-1-matthias.schiffer@ew.tq-group.com/



On Mon, 2022-04-11 at 20:43 -0400, Sasha Levin wrote:
> From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> 
> [ Upstream commit 97e4827d775faa9a32b5e1a97959c69dd77d17a3 ]
> 
> 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.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Link: 
> https://lore.kernel.org/r/20220331110819.133392-1-matthias.schiffer@ew.tq-group.com
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  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 b808c94641fa..75f356041138 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;

Re: [PATCH AUTOSEL 5.17 34/49] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]

On Tue, Apr 12, 2022 at 01:49:19PM +0200, Matthias Schiffer wrote:

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

> what's your plan regarding this patch and the other patch I sent [1]? I
> think there has been some confusion regarding which solution we want to
> backport to stable kernels (well, at least I'm confused...)

Well, it's up to the stable people what they choose to backport -
they're generally fairly aggressive about what they pick up so I guess
they want to take this one?

> I'm fine with this patch getting backported, but in that case [1]
> doesn't make sense anymore (in fact I expected this patch to be dropped
> for now when I submitted [1], due to Pratyush Yadav's concerns).

> [1] https://patchwork.kernel.org/project/spi-devel-general/patch/20220406132832.199777-1-matthias.schiffer@ew.tq-group.com/

For the benefit of those playing at home that's "spi: cadence-quadspi:
fix incorrect supports_op() return value".  It's much more the sort of
thing I'd expect to see backported to stable so it seems good from that
point of view.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH AUTOSEL 5.17 34/49] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[Sasha Levin]

On Tue, Apr 12, 2022 at 01:07:09PM +0100, Mark Brown wrote:
>On Tue, Apr 12, 2022 at 01:49:19PM +0200, Matthias Schiffer wrote:
>
>Please don't top post, reply in line with needed context.  This allows
>readers to readily follow the flow of conversation and understand what
>you are talking about and also helps ensure that everything in the
>discussion is being addressed.
>
>> what's your plan regarding this patch and the other patch I sent [1]? I
>> think there has been some confusion regarding which solution we want to
>> backport to stable kernels (well, at least I'm confused...)
>
>Well, it's up to the stable people what they choose to backport -
>they're generally fairly aggressive about what they pick up so I guess
>they want to take this one?
>
>> I'm fine with this patch getting backported, but in that case [1]
>> doesn't make sense anymore (in fact I expected this patch to be dropped
>> for now when I submitted [1], due to Pratyush Yadav's concerns).
>
>> [1] https://patchwork.kernel.org/project/spi-devel-general/patch/20220406132832.199777-1-matthias.schiffer@ew.tq-group.com/
>
>For the benefit of those playing at home that's "spi: cadence-quadspi:
>fix incorrect supports_op() return value".  It's much more the sort of
>thing I'd expect to see backported to stable so it seems good from that
>point of view.

I'm a bit confused as I don't see the other patch in Linus's tree?

I'll queue this one up then...

-- 
Thanks,
Sasha

Re: [PATCH AUTOSEL 5.17 34/49] spi: cadence-quadspi: fix protocol setup for non-1-1-X operations

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

On Sun, Apr 17, 2022 at 05:33:28PM -0400, Sasha Levin wrote:
> On Tue, Apr 12, 2022 at 01:07:09PM +0100, Mark Brown wrote:

> > For the benefit of those playing at home that's "spi: cadence-quadspi:
> > fix incorrect supports_op() return value".  It's much more the sort of
> > thing I'd expect to see backported to stable so it seems good from that
> > point of view.

> I'm a bit confused as I don't see the other patch in Linus's tree?

> I'll queue this one up then...

I've only recently applied the above commit, it's not sent to Linus yet.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]