Message-ID: <c015bb20-6a57-4f63-8102-34b3d83e0f5b@microchip.com>
Date: Wed, 2 Jul 2025 10:36:00 +0000
From: <Balamanikandan.Gunasundar@...rochip.com>
To: <ada.coupriediaz@....com>, <admin@...iphile.com>
CC: <claudiu.beznea@...on.dev>, <alexandre.belloni@...tlin.com>,
	<vigneshr@...com>, <richard@....at>, <Nicolas.Ferre@...rochip.com>,
	<linux-kernel@...r.kernel.org>, <tudor.ambarus@...aro.org>,
	<linux-mtd@...ts.infradead.org>, <miquel.raynal@...tlin.com>,
	<linux-arm-kernel@...ts.infradead.org>,
	<Balamanikandan.Gunasundar@...rochip.com>
Subject: Re: mtd: rawnand: atmel: ECC error after update to kernel 6.6

Hi,

Thanks Ada for the quick investigation. I too checked the history of the 
change. As you mentioned this could have been lost in the porting.

This patch is required for SAM9x60, SAM9x7, SAM9G25 as it is recommended 
by the datasheet. For 133MHz this must be set to 2, indicating the setup 
time as 3 clock cycles. I will investigate this more and send a patch asap.

I will also try to get more missing details in the datasheet.

Thanks,
Bala

On 01/07/25 9:18 pm, Ada Couprie Diaz wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
> 
> Hi,
> 
> I'm not really involved in this part of the kernel or drivers,
> but I took some time to look into it. Hopefully I'm not completely
> missing something !
> 
> On 30/06/2025 15:38, Zixun LI wrote:
>> Hi,
>>
>> After updating our SAM9G25 device from kernel 3.16 to 6.6, we have
>> encountered UBIFS failures with ECC errors
>>
>> [...]
>>
>> After some diagnostics, I used devmem2 to compare SMC and PMECC
>> registers between the two kernels. All values match except for the
>> PMECC_CLK register, which is 2 in kernel 3.16 [1] and 0 in kernel 6.6
>> [2]. It appears the clock setting is missing since the kernel 4.14
>> refactor.
>>
>> According to the SAM9G25 datasheet this field must be programmed with 2.
>>
>> Manually setting PMECC_CLK to 2 (devmem2 0xffffe010 w 2) resolves the
>> nandtest issue.
>>
>> Is the clock setting moved to elsewhere after the refactor ?
> 
> As far as I can tell, the setting of the *PMECC_CLK* was lost in
> f88fc122cc34 (mtd: nand: Cleanup/rework the atmel_nand driver),
> which was merged for 4.12.
> 
> The register offset and values are  part of the defines it introduces
> but are unused, so they might have been forgotten.
> I couldn't find another place where this was done, and I think it would
> make sense for it to be done here still.
> 
> The tricky part is that now the driver handles other PMECCs that do not 
> have
> this register at all in their datasheets :
>   - SAMA5D2 series[3], page 713, 36.20 Register summary :
>       0x80 (0x10 offset) is marked reserved,
>   - SAMA5D4 series[4], page 512, Table 30-20 :
>       0x080 (0x10 offset) is marked reserved,
> 
> So I think it's best not to add this write back for all cases.
> 
> I also checked, as some other SoCs' PMECCs are marked compatible with
> the one used in your SAM9G25 (`atmel,at91sam9g45-pmecc`) :
>   - SAM9x60 series[5], page 256, 21.6 Register Summary :
>       0x10 offset has PMECC_CLK present, recommends setting to 2 at 133MHz,
>   - SAM9x7 series[6], page 263, 21.6 Register Summary :
>       0x10 offset has PMECC_CLK present, reccomends setting to 2 at 133MHz.
> 
> So it should be safe to write 2 to this register for all those matching
> this /compatible/ string.
> 
> The datasheet itself is not clear as to what to do for other MCK 
> frequencies,
> though as 133MHz is the highest supported frequency and it worked before,
> hopefully it just means a cycle or two of useless delay at worse,
> but at least it keeps it simple.
> 
> I've written a small patch below that I think should fix the issue, but 
> again
> I don't know if that's upstreamable as-is.
> 
>> Best regards,
>> Zixun LI
>>
>> [1] 
>> https://github.com/torvalds/linux/blob/19583ca584d6f574384e17fe7613dfaeadcdc4a6/drivers/mtd/nand/atmel_nand.c#L1058
>> [2] 
>> https://github.com/torvalds/linux/blob/ffc253263a1375a65fa6c9f62a893e9767fbebfa/drivers/mtd/nand/raw/atmel/pmecc.c#L772
>>
> Hopefully that can help !
> Best regards,
> Ada
> 
> 
> [3]: 
> https://ww1.microchip.com/downloads/en/DeviceDoc/SAMA5D2-Series-Data-Sheet-DS60001476C.pdf
> [4]: 
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAMA5D4-Series-Data-Sheet-DS60001525.pdf
> [5]: 
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAM9X60-Data-Sheet-DS60001579.pdf
> [6]: 
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAM9X7-Series-Data-Sheet-DS60001813.pdf
> 
> ------------------------ >8 ------------------------
> 
> diff --git a/drivers/mtd/nand/raw/atmel/pmecc.c 
> b/drivers/mtd/nand/raw/atmel/pmecc.c
> index 3c7dee1be21d..cd96b44f3f30 100644
> --- a/drivers/mtd/nand/raw/atmel/pmecc.c
> +++ b/drivers/mtd/nand/raw/atmel/pmecc.c
> @@ -143,6 +143,7 @@ struct atmel_pmecc_caps {
>         int nstrengths;
>         int el_offset;
>         bool correct_erased_chunks;
> +       bool need_clk_config;
>   };
> 
>   struct atmel_pmecc {
> @@ -787,6 +788,9 @@ int atmel_pmecc_enable(struct atmel_pmecc_user 
> *user, int op)
>         writel(PMECC_CTRL_ENABLE, pmecc->regs.base + ATMEL_PMECC_CTRL);
>         writel(PMECC_CTRL_DATA, pmecc->regs.base + ATMEL_PMECC_CTRL);
> 
> +       if (pmecc->caps->need_clk_config)
> +               writel(PMECC_CLK_133MHZ, pmecc->regs.base + 
> ATMEL_PMECC_CLK);
> +
>         return 0;
>   }
>   EXPORT_SYMBOL_GPL(atmel_pmecc_enable);
> @@ -896,6 +900,7 @@ static struct atmel_pmecc_caps at91sam9g45_caps = {
>         .strengths = atmel_pmecc_strengths,
>         .nstrengths = 5,
>         .el_offset = 0x8c,
> +       .need_clk_config = true,
>   };
> 
>   static struct atmel_pmecc_caps sama5d4_caps = {
> 
> 

Powered by blists - more mailing lists