* [PATCH] spi: spi-qpic-snand: fix calculating of ECC OOB regions' properties
@ 2025-07-31 18:11 Gabor Juhos
2025-08-01 11:56 ` Konrad Dybcio
0 siblings, 1 reply; 4+ messages in thread
From: Gabor Juhos @ 2025-07-31 18:11 UTC (permalink / raw)
To: Mark Brown, Varadarajan Narayanan, Sricharan Ramabadhran, Md Sadre Alam
Cc: linux-spi, linux-mtd, linux-arm-msm, linux-kernel, Gabor Juhos
The OOB layout used by the driver has two distinct regions which contains
hardware specific ECC data, yet the qcom_spi_ooblayout_ecc() function sets
the same offset and length values for both regions which is clearly wrong.
Change the code to calculate the correct values for both regions.
For reference, the following table shows the computed offset and length
values for various OOB size/ECC strength configurations:
+-----------------+-----------------+
|before the change| after the change|
+-------+----------+--------+--------+--------+--------+--------+
| OOB | ECC | region | region | region | region | region |
| size | strength | index | offset | length | offset | length |
+-------+----------+--------+--------+--------+--------+--------+
| 128 | 8 | 0 | 113 | 15 | 0 | 49 |
| | | 1 | 113 | 15 | 65 | 63 |
+-------+----------+--------+--------+--------+--------+--------+
| 128 | 4 | 0 | 117 | 11 | 0 | 37 |
| | | 1 | 117 | 11 | 53 | 75 |
+-------+----------+--------+--------+--------+--------+--------+
| 64 | 4 | 0 | 53 | 11 | 0 | 37 |
| | | 1 | 53 | 11 | 53 | 11 |
+-------+----------+--------+--------+--------+--------+--------+
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 | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
index 0cfa0d960fd3c245c2bbf4f5e02d0fc0b13e7696..37ddc48d2c17264499f821d235835c4ff5982873 100644
--- a/drivers/spi/spi-qpic-snand.c
+++ b/drivers/spi/spi-qpic-snand.c
@@ -213,8 +213,16 @@ static int qcom_spi_ooblayout_ecc(struct mtd_info *mtd, int section,
if (section > 1)
return -ERANGE;
- oobregion->length = qecc->ecc_bytes_hw + qecc->spare_bytes;
- oobregion->offset = mtd->oobsize - oobregion->length;
+ if (!section) {
+ oobregion->offset = 0;
+ oobregion->length = qecc->bytes * (qecc->steps - 1) +
+ qecc->bbm_size;
+ } else {
+ oobregion->offset = qecc->bytes * (qecc->steps - 1) +
+ qecc->bbm_size +
+ qecc->steps * 4;
+ oobregion->length = mtd->oobsize - oobregion->offset;
+ }
return 0;
}
---
base-commit: 926406a85ad895fbe6ee4577cdbc4f55245a0742
change-id: 20250731-qpic-snand-oob-ecc-fix-8d3d80cc8680
Best regards,
--
Gabor Juhos <j4g8y7@gmail.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] spi: spi-qpic-snand: fix calculating of ECC OOB regions' properties
2025-07-31 18:11 [PATCH] spi: spi-qpic-snand: fix calculating of ECC OOB regions' properties Gabor Juhos
@ 2025-08-01 11:56 ` Konrad Dybcio
2025-08-04 7:22 ` Gabor Juhos
0 siblings, 1 reply; 4+ messages in thread
From: Konrad Dybcio @ 2025-08-01 11:56 UTC (permalink / raw)
To: Gabor Juhos, Mark Brown, Varadarajan Narayanan,
Sricharan Ramabadhran, Md Sadre Alam
Cc: linux-spi, linux-mtd, linux-arm-msm, linux-kernel
On 7/31/25 8:11 PM, Gabor Juhos wrote:
> The OOB layout used by the driver has two distinct regions which contains
> hardware specific ECC data, yet the qcom_spi_ooblayout_ecc() function sets
> the same offset and length values for both regions which is clearly wrong.
>
> Change the code to calculate the correct values for both regions.
>
> For reference, the following table shows the computed offset and length
> values for various OOB size/ECC strength configurations:
>
> +-----------------+-----------------+
> |before the change| after the change|
> +-------+----------+--------+--------+--------+--------+--------+
> | OOB | ECC | region | region | region | region | region |
> | size | strength | index | offset | length | offset | length |
> +-------+----------+--------+--------+--------+--------+--------+
> | 128 | 8 | 0 | 113 | 15 | 0 | 49 |
> | | | 1 | 113 | 15 | 65 | 63 |
> +-------+----------+--------+--------+--------+--------+--------+
> | 128 | 4 | 0 | 117 | 11 | 0 | 37 |
> | | | 1 | 117 | 11 | 53 | 75 |
> +-------+----------+--------+--------+--------+--------+--------+
> | 64 | 4 | 0 | 53 | 11 | 0 | 37 |
> | | | 1 | 53 | 11 | 53 | 11 |
> +-------+----------+--------+--------+--------+--------+--------+
>
> 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 | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-qpic-snand.c b/drivers/spi/spi-qpic-snand.c
> index 0cfa0d960fd3c245c2bbf4f5e02d0fc0b13e7696..37ddc48d2c17264499f821d235835c4ff5982873 100644
> --- a/drivers/spi/spi-qpic-snand.c
> +++ b/drivers/spi/spi-qpic-snand.c
> @@ -213,8 +213,16 @@ static int qcom_spi_ooblayout_ecc(struct mtd_info *mtd, int section,
> if (section > 1)
> return -ERANGE;
>
> - oobregion->length = qecc->ecc_bytes_hw + qecc->spare_bytes;
> - oobregion->offset = mtd->oobsize - oobregion->length;
> + if (!section) {
> + oobregion->offset = 0;
> + oobregion->length = qecc->bytes * (qecc->steps - 1) +
> + qecc->bbm_size;
> + } else {
> + oobregion->offset = qecc->bytes * (qecc->steps - 1) +
> + qecc->bbm_size +
> + qecc->steps * 4;
> + oobregion->length = mtd->oobsize - oobregion->offset;
> + }
How about
if (section == 0) {
} else if (section == 1) {
} else { return -ERANGE }
?
FWIW the values match qcom_spi_ooblayout_free(), so the commit seems
sane
Konrad
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] spi: spi-qpic-snand: fix calculating of ECC OOB regions' properties
2025-08-01 11:56 ` Konrad Dybcio
@ 2025-08-04 7:22 ` Gabor Juhos
2025-08-04 9:04 ` Konrad Dybcio
0 siblings, 1 reply; 4+ messages in thread
From: Gabor Juhos @ 2025-08-04 7:22 UTC (permalink / raw)
To: Konrad Dybcio, Mark Brown, Varadarajan Narayanan,
Sricharan Ramabadhran, Md Sadre Alam
Cc: linux-spi, linux-mtd, linux-arm-msm, linux-kernel
Hi Konrad,
2025. 08. 01. 13:56 keltezéssel, Konrad Dybcio írta:
> On 7/31/25 8:11 PM, Gabor Juhos wrote:
...
>> --- a/drivers/spi/spi-qpic-snand.c
>> +++ b/drivers/spi/spi-qpic-snand.c
>> @@ -213,8 +213,16 @@ static int qcom_spi_ooblayout_ecc(struct mtd_info *mtd, int section,
>> if (section > 1)
>> return -ERANGE;
>>
>> - oobregion->length = qecc->ecc_bytes_hw + qecc->spare_bytes;
>> - oobregion->offset = mtd->oobsize - oobregion->length;
>> + if (!section) {
>> + oobregion->offset = 0;
>> + oobregion->length = qecc->bytes * (qecc->steps - 1) +
>> + qecc->bbm_size;
>> + } else {
>> + oobregion->offset = qecc->bytes * (qecc->steps - 1) +
>> + qecc->bbm_size +
>> + qecc->steps * 4;
>> + oobregion->length = mtd->oobsize - oobregion->offset;
>> + }
>
> How about
>
> if (section == 0) {
> } else if (section == 1) {
> } else { return -ERANGE }
>
> ?
The current way follows the implementation in the qcom_nandc driver, so it makes
it easier to compare the two, but it can be changed of course.
However, since the 'section' parameter is an integer can we agree up on using a
switch statement instead of multiple ifs?
Regards,
Gabor
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] spi: spi-qpic-snand: fix calculating of ECC OOB regions' properties
2025-08-04 7:22 ` Gabor Juhos
@ 2025-08-04 9:04 ` Konrad Dybcio
0 siblings, 0 replies; 4+ messages in thread
From: Konrad Dybcio @ 2025-08-04 9:04 UTC (permalink / raw)
To: Gabor Juhos, Mark Brown, Varadarajan Narayanan,
Sricharan Ramabadhran, Md Sadre Alam
Cc: linux-spi, linux-mtd, linux-arm-msm, linux-kernel
On 8/4/25 9:22 AM, Gabor Juhos wrote:
> Hi Konrad,
>
> 2025. 08. 01. 13:56 keltezéssel, Konrad Dybcio írta:
>> On 7/31/25 8:11 PM, Gabor Juhos wrote:
>
> ...
>
>>> --- a/drivers/spi/spi-qpic-snand.c
>>> +++ b/drivers/spi/spi-qpic-snand.c
>>> @@ -213,8 +213,16 @@ static int qcom_spi_ooblayout_ecc(struct mtd_info *mtd, int section,
>>> if (section > 1)
>>> return -ERANGE;
>>>
>>> - oobregion->length = qecc->ecc_bytes_hw + qecc->spare_bytes;
>>> - oobregion->offset = mtd->oobsize - oobregion->length;
>>> + if (!section) {
>>> + oobregion->offset = 0;
>>> + oobregion->length = qecc->bytes * (qecc->steps - 1) +
>>> + qecc->bbm_size;
>>> + } else {
>>> + oobregion->offset = qecc->bytes * (qecc->steps - 1) +
>>> + qecc->bbm_size +
>>> + qecc->steps * 4;
>>> + oobregion->length = mtd->oobsize - oobregion->offset;
>>> + }
>>
>> How about
>>
>> if (section == 0) {
>> } else if (section == 1) {
>> } else { return -ERANGE }
>>
>> ?
>
> The current way follows the implementation in the qcom_nandc driver, so it makes
> it easier to compare the two, but it can be changed of course.
>
> However, since the 'section' parameter is an integer can we agree up on using a
> switch statement instead of multiple ifs?
That works too
Konrad
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-08-04 9:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-07-31 18:11 [PATCH] spi: spi-qpic-snand: fix calculating of ECC OOB regions' properties Gabor Juhos
2025-08-01 11:56 ` Konrad Dybcio
2025-08-04 7:22 ` Gabor Juhos
2025-08-04 9:04 ` Konrad Dybcio
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®