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

From: Gabor Juhos <j4g8y7@gmail.com>
To: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>,
	Mark Brown <broonie@kernel.org>,
	Md Sadre Alam <quic_mdalam@quicinc.com>,
	Varadarajan Narayanan <quic_varada@quicinc.com>,
	Sricharan Ramabadhran <quic_srichara@quicinc.com>
Cc: linux-spi@vger.kernel.org, linux-mtd@lists.infradead.org,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] spi: spi-qpic-snand: use correct CW_PER_PAGE value for OOB write
Date: Mon, 4 Aug 2025 09:40:54 +0200	[thread overview]
Message-ID: <1ba00a38-7293-4f72-9aee-f87f41a3dcc6@gmail.com> (raw)
In-Reply-To: <b2e4d6b1-25bc-4b2e-ae54-6588f1573131@oss.qualcomm.com>

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