[PATCH] spi: spi-qpic-snand: use correct CW_PER_PAGE value for OOB write

[PATCH] spi: spi-qpic-snand: use correct CW_PER_PAGE value for OOB write

[Gabor Juhos]

The qcom_spi_program_oob() function uses only the last codeword to write
the OOB data into the flash, but it sets the CW_PER_PAGE field in the
CFG0 register as it would use all codewords.

It seems that this confuses the hardware somehow, and any access to the
flash fails with a timeout error after the function is called. The problem
can be easily reproduced with the following commands:

    # dd if=/dev/zero bs=2176 count=1 > /tmp/test.bin
    1+0 records in
    1+0 records out
    # flash_erase /dev/mtd4 0 0
    Erasing 128 Kibyte @ 0 -- 100 % complete
    # nandwrite -O /dev/mtd4 /tmp/test.bin
    Writing data to block 0 at offset 0x0
    # nanddump -o /dev/mtd4 >/dev/null
    ECC failed: 0
    ECC corrected: 0
    Number of bad blocks: 0
    Number of bbt blocks: 0
    Block size 131072, page size 2048, OOB size 128
    Dumping data starting at 0x00000000 and ending at 0x00020000...
    [   33.197605] qcom_snand 79b0000.spi: failure to read oob
    libmtd: error!: MEMREADOOB64 ioctl failed for mtd4, offset 0 (eraseblock 0)
            error 110 (Operation timed out)
    [   35.277582] qcom_snand 79b0000.spi: failure in submitting cmd descriptor
    libmtd: error!: cannot read 2048 bytes from mtd4 (eraseblock 0, offset 2048)
            error 110 (Operation timed out)
    nanddump: error!: mtd_read

Change the code to use the correct CW_PER_PAGE value to avoid this.

Fixes: 7304d1909080 ("spi: spi-qpic: add driver for QCOM SPI NAND flash Interface")
Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
---
 drivers/spi/spi-qpic-snand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index 0cfa0d960fd3c245c2bbf4f5e02d0fc0b13e7696..5216d60e01aab26f927baaea24296571a77527cb 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -1196,7 +1196,7 @@ static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
 	u32 cfg0, cfg1, ecc_bch_cfg, ecc_buf_cfg;
 
 	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
-	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
+	       FIELD_PREP(CW_PER_PAGE_MASK, 0);
 	cfg1 = ecc_cfg->cfg1;
 	ecc_bch_cfg = ecc_cfg->ecc_bch_cfg;
 	ecc_buf_cfg = ecc_cfg->ecc_buf_cfg;

---
base-commit: 926406a85ad895fbe6ee4577cdbc4f55245a0742
change-id: 20250731-qpic-snand-oob-cwpp-fix-6b26ee45c743

Best regards,
-- 
Gabor Juhos <j4g8y7@gmail.com>

Re: [PATCH] spi: spi-qpic-snand: use correct CW_PER_PAGE value for OOB write

[Konrad Dybcio]

On 8/1/25 9:58 AM, Gabor Juhos wrote:
> The qcom_spi_program_oob() function uses only the last codeword to write
> the OOB data into the flash, but it sets the CW_PER_PAGE field in the
> CFG0 register as it would use all codewords.
> 
> It seems that this confuses the hardware somehow, and any access to the
> flash fails with a timeout error after the function is called. The problem
> can be easily reproduced with the following commands:
> 
>     # dd if=/dev/zero bs=2176 count=1 > /tmp/test.bin
>     1+0 records in
>     1+0 records out
>     # flash_erase /dev/mtd4 0 0
>     Erasing 128 Kibyte @ 0 -- 100 % complete
>     # nandwrite -O /dev/mtd4 /tmp/test.bin
>     Writing data to block 0 at offset 0x0
>     # nanddump -o /dev/mtd4 >/dev/null
>     ECC failed: 0
>     ECC corrected: 0
>     Number of bad blocks: 0
>     Number of bbt blocks: 0
>     Block size 131072, page size 2048, OOB size 128
>     Dumping data starting at 0x00000000 and ending at 0x00020000...
>     [   33.197605] qcom_snand 79b0000.spi: failure to read oob
>     libmtd: error!: MEMREADOOB64 ioctl failed for mtd4, offset 0 (eraseblock 0)
>             error 110 (Operation timed out)
>     [   35.277582] qcom_snand 79b0000.spi: failure in submitting cmd descriptor
>     libmtd: error!: cannot read 2048 bytes from mtd4 (eraseblock 0, offset 2048)
>             error 110 (Operation timed out)
>     nanddump: error!: mtd_read
> 
> Change the code to use the correct CW_PER_PAGE value to avoid this.
> 
> Fixes: 7304d1909080 ("spi: spi-qpic: add driver for QCOM SPI NAND flash Interface")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> ---
>  drivers/spi/spi-qpic-snand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
> index 0cfa0d960fd3c245c2bbf4f5e02d0fc0b13e7696..5216d60e01aab26f927baaea24296571a77527cb 100644
> --- a/drivers/spi/spi-qpic-snand.c
> +++ b/drivers/spi/spi-qpic-snand.c
> @@ -1196,7 +1196,7 @@ static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
>  	u32 cfg0, cfg1, ecc_bch_cfg, ecc_buf_cfg;
>  
>  	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
> -	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
> +	       FIELD_PREP(CW_PER_PAGE_MASK, 0);

FWIW I'm just a fly-by reviewer for this driver, but the docs say:

The value is the number of codewords per page minus one
"NOTE: This field must be cleared for block erase operation"

Konrad

Re: [PATCH] spi: spi-qpic-snand: use correct CW_PER_PAGE value for OOB write

[Gabor Juhos]

Hi Konrad,

2025. 08. 01. 13:08 keltezéssel, Konrad Dybcio írta:

...

>> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
>> index 0cfa0d960fd3c245c2bbf4f5e02d0fc0b13e7696..5216d60e01aab26f927baaea24296571a77527cb 100644
>> --- a/drivers/spi/spi-qpic-snand.c
>> +++ b/drivers/spi/spi-qpic-snand.c
>> @@ -1196,7 +1196,7 @@ static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
>>  	u32 cfg0, cfg1, ecc_bch_cfg, ecc_buf_cfg;
>>  
>>  	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
>> -	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>> +	       FIELD_PREP(CW_PER_PAGE_MASK, 0);
> 
> FWIW I'm just a fly-by reviewer for this driver, but the docs say:
> 
> The value is the number of codewords per page minus one

Well, the driver uses that differently even without the patch. See below.

> "NOTE: This field must be cleared for block erase operation"

    $ git grep -hp 'FIELD_PREP(CW_PER_PAGE_MASK,.*;' drivers/spi/spi-qpic-snand.c
    static int qcom_spi_block_erase(struct qcom_nand_controller *snandc)
                                             FIELD_PREP(CW_PER_PAGE_MASK, 0));
  
This function implements the block erase operation and it corresponds to the
documentation. So far so good.

    static int qcom_spi_read_last_cw(struct qcom_nand_controller *snandc,
                   FIELD_PREP(CW_PER_PAGE_MASK, 0);
    static int qcom_spi_read_cw_raw(struct qcom_nand_controller *snandc, u8 *data_buf,
                   FIELD_PREP(CW_PER_PAGE_MASK, 0);
  

These two functions are using a single codeword (with zero CW_PER_PAGE value).
So, it seems that in reality the CW_PER_PAGE value means the number of codewords
(minus one) used within a single operation executed. Of course it is possible
that the existing code is wrong here.

    static int qcom_spi_read_page_ecc(struct qcom_nand_controller *snandc,
                   FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
    static int qcom_spi_read_page_oob(struct qcom_nand_controller *snandc,
                   FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
    static int qcom_spi_program_raw(struct qcom_nand_controller *snandc,
                   FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
    static int qcom_spi_program_ecc(struct qcom_nand_controller *snandc,
                   FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);


The previous functions are operating on whole pages, so those are using all codewords
within a page thus 'num_cw - 1' is getting set in the register field. This also matches
with the documentation.

    static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
                   FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);

This is the function fixed by the patch. As it is indicated in the commit description
this also uses a single codeword similarly to the qcom_spi_read_(last_cw,cw_raw) functions
described above so the CW_PER_PAGE value should be set to zero.

Regards,
Gabor

Re: [PATCH] spi: spi-qpic-snand: use correct CW_PER_PAGE value for OOB write

[Konrad Dybcio]

On 8/4/25 9:40 AM, Gabor Juhos wrote:
> Hi Konrad,
> 
> 2025. 08. 01. 13:08 keltezéssel, Konrad Dybcio írta:
> 
> ...
> 
>>> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
>>> index 0cfa0d960fd3c245c2bbf4f5e02d0fc0b13e7696..5216d60e01aab26f927baaea24296571a77527cb 100644
>>> --- a/drivers/spi/spi-qpic-snand.c
>>> +++ b/drivers/spi/spi-qpic-snand.c
>>> @@ -1196,7 +1196,7 @@ static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
>>>  	u32 cfg0, cfg1, ecc_bch_cfg, ecc_buf_cfg;
>>>  
>>>  	cfg0 = (ecc_cfg->cfg0 & ~CW_PER_PAGE_MASK) |
>>> -	       FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>>> +	       FIELD_PREP(CW_PER_PAGE_MASK, 0);
>>
>> FWIW I'm just a fly-by reviewer for this driver, but the docs say:
>>
>> The value is the number of codewords per page minus one
> 
> Well, the driver uses that differently even without the patch. See below.
> 
>> "NOTE: This field must be cleared for block erase operation"
> 
>     $ git grep -hp 'FIELD_PREP(CW_PER_PAGE_MASK,.*;' drivers/spi/spi-qpic-snand.c
>     static int qcom_spi_block_erase(struct qcom_nand_controller *snandc)
>                                              FIELD_PREP(CW_PER_PAGE_MASK, 0));
>   
> This function implements the block erase operation and it corresponds to the
> documentation. So far so good.
> 
>     static int qcom_spi_read_last_cw(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, 0);
>     static int qcom_spi_read_cw_raw(struct qcom_nand_controller *snandc, u8 *data_buf,
>                    FIELD_PREP(CW_PER_PAGE_MASK, 0);
>   
> 
> These two functions are using a single codeword (with zero CW_PER_PAGE value).
> So, it seems that in reality the CW_PER_PAGE value means the number of codewords
> (minus one) used within a single operation executed. Of course it is possible
> that the existing code is wrong here.
> 
>     static int qcom_spi_read_page_ecc(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>     static int qcom_spi_read_page_oob(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>     static int qcom_spi_program_raw(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
>     static int qcom_spi_program_ecc(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
> 
> 
> The previous functions are operating on whole pages, so those are using all codewords
> within a page thus 'num_cw - 1' is getting set in the register field. This also matches
> with the documentation.
> 
>     static int qcom_spi_program_oob(struct qcom_nand_controller *snandc,
>                    FIELD_PREP(CW_PER_PAGE_MASK, num_cw - 1);
> 
> This is the function fixed by the patch. As it is indicated in the commit description
> this also uses a single codeword similarly to the qcom_spi_read_(last_cw,cw_raw) functions
> described above so the CW_PER_PAGE value should be set to zero.

I didn't mean to dispute what you said :)
Simply included some context for other reviewers

But thanks for the insight, this seems to indeed make sense
the way you presented it

Konrad

Re: [PATCH] spi: spi-qpic-snand: use correct CW_PER_PAGE value for OOB write

[Mark Brown]

On Fri, 01 Aug 2025 09:58:35 +0200, Gabor Juhos wrote:
> The qcom_spi_program_oob() function uses only the last codeword to write
> the OOB data into the flash, but it sets the CW_PER_PAGE field in the
> CFG0 register as it would use all codewords.
> 
> It seems that this confuses the hardware somehow, and any access to the
> flash fails with a timeout error after the function is called. The problem
> can be easily reproduced with the following commands:
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: spi-qpic-snand: use correct CW_PER_PAGE value for OOB write
      commit: 6bc829220b33da8522572cc50fdf5067c51d3bf3

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