[PATCH] mtd: spi-nor: core: replace dummy buswidth from addr to data

[PATCH] mtd: spi-nor: core: replace dummy buswidth from addr to data

[Cheng Ming Lin]

From: Cheng Ming Lin <chengminglin@mxic.com.tw>

The default dummy cycle for Macronix SPI NOR flash in Octal Output
Read Mode(1-1-8) is 20.

Currently, the dummy buswidth is set according to the address bus width.
In the 1-1-8 mode, this means the dummy buswidth is 1. When converting
dummy cycles to bytes, this results in 20 x 1 / 8 = 2 bytes, causing the
host to read data 4 cycles too early.

Since the protocol data buswidth is always greater than or equal to the
address buswidth. Setting the dummy buswidth to match the data buswidth
increases the likelihood that the dummy cycle-to-byte conversion will be
divisible, preventing the host from reading data prematurely.

Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
---
 drivers/mtd/spi-nor/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f9c189ed7353..c7aceaa8a43f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -89,7 +89,7 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
 		op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
 
 	if (op->dummy.nbytes)
-		op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
+		op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);
 
 	if (op->data.nbytes)
 		op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);
-- 
2.25.1

Re: [PATCH] mtd: spi-nor: core: replace dummy buswidth from addr to data

[Pratyush Yadav]

On Thu, Nov 07 2024, Cheng Ming Lin wrote:

> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
>
> The default dummy cycle for Macronix SPI NOR flash in Octal Output
> Read Mode(1-1-8) is 20.
>
> Currently, the dummy buswidth is set according to the address bus width.
> In the 1-1-8 mode, this means the dummy buswidth is 1. When converting
> dummy cycles to bytes, this results in 20 x 1 / 8 = 2 bytes, causing the
> host to read data 4 cycles too early.
>
> Since the protocol data buswidth is always greater than or equal to the
> address buswidth. Setting the dummy buswidth to match the data buswidth
> increases the likelihood that the dummy cycle-to-byte conversion will be
> divisible, preventing the host from reading data prematurely.

Makes sense.

Reviewed-by: Pratyush Yadav <pratyush@kernel.org>

-- 
Regards,
Pratyush Yadav

Re: [PATCH] mtd: spi-nor: core: replace dummy buswidth from addr to data

[Tudor Ambarus]

On 11/7/24 9:30 AM, Cheng Ming Lin wrote:
> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
> 
> The default dummy cycle for Macronix SPI NOR flash in Octal Output
> Read Mode(1-1-8) is 20.
> 
> Currently, the dummy buswidth is set according to the address bus width.
> In the 1-1-8 mode, this means the dummy buswidth is 1. When converting
> dummy cycles to bytes, this results in 20 x 1 / 8 = 2 bytes, causing the
> host to read data 4 cycles too early.
> 
> Since the protocol data buswidth is always greater than or equal to the
> address buswidth. Setting the dummy buswidth to match the data buswidth
> increases the likelihood that the dummy cycle-to-byte conversion will be
> divisible, preventing the host from reading data prematurely.

This is still very wrong and the `fix` is working just by chance.
Consider what happens when one requires 10 dummy cycles. BTW, does this
fix a real problem, or it's just a theoretical fix?

I don't like that we're chasing our tails here. The fix is to pass
directly cycles instead of bytes and update MTD and SPI to use cycles. I
had an attempt to fix this in the past, but I can't allocate time to
respin. Are you interested in taking over and fixing MTD and SPI?

> Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
> ---
>  drivers/mtd/spi-nor/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index f9c189ed7353..c7aceaa8a43f 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -89,7 +89,7 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
>  		op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
>  
>  	if (op->dummy.nbytes)
> -		op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> +		op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);
>  
>  	if (op->data.nbytes)
>  		op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);

Re: [PATCH] mtd: spi-nor: core: replace dummy buswidth from addr to data

[Cheng Ming Lin]

Hi Tudor,

Tudor Ambarus <tudor.ambarus@linaro.org> 於 2024年11月11日 週一 下午6:18寫道：
>
>
>
> On 11/7/24 9:30 AM, Cheng Ming Lin wrote:
> > From: Cheng Ming Lin <chengminglin@mxic.com.tw>
> >
> > The default dummy cycle for Macronix SPI NOR flash in Octal Output
> > Read Mode(1-1-8) is 20.
> >
> > Currently, the dummy buswidth is set according to the address bus width.
> > In the 1-1-8 mode, this means the dummy buswidth is 1. When converting
> > dummy cycles to bytes, this results in 20 x 1 / 8 = 2 bytes, causing the
> > host to read data 4 cycles too early.
> >
> > Since the protocol data buswidth is always greater than or equal to the
> > address buswidth. Setting the dummy buswidth to match the data buswidth
> > increases the likelihood that the dummy cycle-to-byte conversion will be
> > divisible, preventing the host from reading data prematurely.
>
> This is still very wrong and the `fix` is working just by chance.
> Consider what happens when one requires 10 dummy cycles. BTW, does this
> fix a real problem, or it's just a theoretical fix?

In 1-1-8 mode, setting the dummy buswidth to match the data
buswidth ensures a dummy buswidth of 8, which can accommodate
all types of dummy cycles.

This patch resolves a significant issue in 1-1-8 mode, as described above.

>
> I don't like that we're chasing our tails here. The fix is to pass
> directly cycles instead of bytes and update MTD and SPI to use cycles. I
> had an attempt to fix this in the past, but I can't allocate time to
> respin. Are you interested in taking over and fixing MTD and SPI?
>

I understand your concerns and agree with your approach. However,
this fix may require some time to implement, and we're currently facing
this issue with a pressing need for resolution. If I can allocate time in
the near feature, I'd be interested in taking on the work to address this
for both MTD and SPI.

> > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
> > ---
> >  drivers/mtd/spi-nor/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index f9c189ed7353..c7aceaa8a43f 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -89,7 +89,7 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
> >               op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> >
> >       if (op->dummy.nbytes)
> > -             op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> > +             op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);
> >
> >       if (op->data.nbytes)
> >               op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);

Re: [PATCH] mtd: spi-nor: core: replace dummy buswidth from addr to data

[Tudor Ambarus]

On 11/12/24 2:42 AM, Cheng Ming Lin wrote:
> Hi Tudor,
> 
> Tudor Ambarus <tudor.ambarus@linaro.org> 於 2024年11月11日 週一 下午6:18寫道：
>>
>>
>>
>> On 11/7/24 9:30 AM, Cheng Ming Lin wrote:
>>> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
>>>
>>> The default dummy cycle for Macronix SPI NOR flash in Octal Output
>>> Read Mode(1-1-8) is 20.
>>>
>>> Currently, the dummy buswidth is set according to the address bus width.
>>> In the 1-1-8 mode, this means the dummy buswidth is 1. When converting
>>> dummy cycles to bytes, this results in 20 x 1 / 8 = 2 bytes, causing the
>>> host to read data 4 cycles too early.
>>>
>>> Since the protocol data buswidth is always greater than or equal to the
>>> address buswidth. Setting the dummy buswidth to match the data buswidth
>>> increases the likelihood that the dummy cycle-to-byte conversion will be
>>> divisible, preventing the host from reading data prematurely.
>>
>> This is still very wrong and the `fix` is working just by chance.
>> Consider what happens when one requires 10 dummy cycles. BTW, does this
>> fix a real problem, or it's just a theoretical fix?
> 
> In 1-1-8 mode, setting the dummy buswidth to match the data
> buswidth ensures a dummy buswidth of 8, which can accommodate
> all types of dummy cycles.
> 
> This patch resolves a significant issue in 1-1-8 mode, as described above.
> 

shall we add a fixes tag then, and a cc to stable?

Re: [PATCH] mtd: spi-nor: core: replace dummy buswidth from addr to data

[Tudor Ambarus]

On 11/12/24 6:45 AM, Tudor Ambarus wrote:
> 
> 
> On 11/12/24 2:42 AM, Cheng Ming Lin wrote:
>> Hi Tudor,
>>
>> Tudor Ambarus <tudor.ambarus@linaro.org> 於 2024年11月11日 週一 下午6:18寫道：
>>>
>>>
>>>
>>> On 11/7/24 9:30 AM, Cheng Ming Lin wrote:
>>>> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
>>>>
>>>> The default dummy cycle for Macronix SPI NOR flash in Octal Output
>>>> Read Mode(1-1-8) is 20.
>>>>
>>>> Currently, the dummy buswidth is set according to the address bus width.
>>>> In the 1-1-8 mode, this means the dummy buswidth is 1. When converting
>>>> dummy cycles to bytes, this results in 20 x 1 / 8 = 2 bytes, causing the
>>>> host to read data 4 cycles too early.
>>>>
>>>> Since the protocol data buswidth is always greater than or equal to the
>>>> address buswidth. Setting the dummy buswidth to match the data buswidth
>>>> increases the likelihood that the dummy cycle-to-byte conversion will be
>>>> divisible, preventing the host from reading data prematurely.
>>>
>>> This is still very wrong and the `fix` is working just by chance.
>>> Consider what happens when one requires 10 dummy cycles. BTW, does this
>>> fix a real problem, or it's just a theoretical fix?
>>
>> In 1-1-8 mode, setting the dummy buswidth to match the data
>> buswidth ensures a dummy buswidth of 8, which can accommodate
>> all types of dummy cycles.
>>
>> This patch resolves a significant issue in 1-1-8 mode, as described above.
>>
> 
> shall we add a fixes tag then, and a cc to stable?

I need a patch today if you want it integrated in the SPI NOR PR.
Otherwise we'll queue one in the -rc phase.

Re: [PATCH] mtd: spi-nor: core: replace dummy buswidth from addr to data

[Cheng Ming Lin]

Hi Tudor,

Tudor Ambarus <tudor.ambarus@linaro.org> 於 2024年11月12日 週二 下午3:06寫道：
>
>
>
> On 11/12/24 6:45 AM, Tudor Ambarus wrote:
> >
> >
> > On 11/12/24 2:42 AM, Cheng Ming Lin wrote:
> >> Hi Tudor,
> >>
> >> Tudor Ambarus <tudor.ambarus@linaro.org> 於 2024年11月11日 週一 下午6:18寫道：
> >>>
> >>>
> >>>
> >>> On 11/7/24 9:30 AM, Cheng Ming Lin wrote:
> >>>> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
> >>>>
> >>>> The default dummy cycle for Macronix SPI NOR flash in Octal Output
> >>>> Read Mode(1-1-8) is 20.
> >>>>
> >>>> Currently, the dummy buswidth is set according to the address bus width.
> >>>> In the 1-1-8 mode, this means the dummy buswidth is 1. When converting
> >>>> dummy cycles to bytes, this results in 20 x 1 / 8 = 2 bytes, causing the
> >>>> host to read data 4 cycles too early.
> >>>>
> >>>> Since the protocol data buswidth is always greater than or equal to the
> >>>> address buswidth. Setting the dummy buswidth to match the data buswidth
> >>>> increases the likelihood that the dummy cycle-to-byte conversion will be
> >>>> divisible, preventing the host from reading data prematurely.
> >>>
> >>> This is still very wrong and the `fix` is working just by chance.
> >>> Consider what happens when one requires 10 dummy cycles. BTW, does this
> >>> fix a real problem, or it's just a theoretical fix?
> >>
> >> In 1-1-8 mode, setting the dummy buswidth to match the data
> >> buswidth ensures a dummy buswidth of 8, which can accommodate
> >> all types of dummy cycles.
> >>
> >> This patch resolves a significant issue in 1-1-8 mode, as described above.
> >>
> >
> > shall we add a fixes tag then, and a cc to stable?
>
> I need a patch today if you want it integrated in the SPI NOR PR.
> Otherwise we'll queue one in the -rc phase.

Sure, so do I need to submit a second version of the patch
with the Fixes tag and cc it to stable?

Thanks,
Cheng Ming Lin