Re: [PATCH 1/2] mtd: rawnand: atmel: set pmecc data setup time

[Alexander Dahl <ada@thorsis.com>]

Hello Eugen,

Am Fri, Nov 07, 2025 at 12:52:05PM +0200 schrieb Eugen Hristev:
> Hello Zixun, Alexander,
> 
> On 11/6/25 13:12, Zixun LI wrote:
> > Setup the pmecc data setup time as 3 clock cycles for 133MHz as
> > recommended by the datasheet.
> > 
> > Backported from Linux: f552a7c7 ("mtd: rawnand: atmel: set pmecc data
> > setup time")
> 
> Maybe use 12 digits commit hash ? I cannot find this commit in Linux

v6.16-rc4-10-gf552a7c7e0a14 aka
f552a7c7e0a14215cb8a6fd89e60fa3932a74786 from July 2025.

> 
> > 
> > Fixes: a490e1b7c017c ("nand: atmel: Add pmecc driver")
> > 
> > Signed-off-by: Zixun LI <admin@hifiphile.com>
> > ---
> >  drivers/mtd/nand/raw/atmel/pmecc.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/raw/atmel/pmecc.c b/drivers/mtd/nand/raw/atmel/pmecc.c
> > index e500a0fe3f8671ff08195c94c3383696a7731eb3..7c4e9bd5f9967b119239adf2b39e683ea62b71fd 100644
> > --- a/drivers/mtd/nand/raw/atmel/pmecc.c
> > +++ b/drivers/mtd/nand/raw/atmel/pmecc.c
> > @@ -142,6 +142,7 @@ struct atmel_pmecc_caps {
> >  	int nstrengths;
> >  	int el_offset;
> >  	bool correct_erased_chunks;
> > +	bool clk_ctrl;
> >  };
> >  
> >  struct atmel_pmecc_user_conf_cache {
> > @@ -840,6 +841,10 @@ atmel_pmecc_create(struct udevice *dev,
> >  
> >  	pmecc->regs.timing = 0;
> >  
> > +	/* pmecc data setup time */
> > +	if (caps->clk_ctrl)
> > +		writel(PMECC_CLK_133MHZ, pmecc->regs.base + ATMEL_PMECC_CLK);
> > +
> >  	/* Disable all interrupts before registering the PMECC handler. */
> >  	writel(0xffffffff, pmecc->regs.base + ATMEL_PMECC_IDR);
> >  	atmel_pmecc_reset(pmecc);
> > @@ -884,6 +889,7 @@ static struct atmel_pmecc_caps at91sam9g45_caps = {
> >  	.strengths = atmel_pmecc_strengths,
> >  	.nstrengths = 5,
> >  	.el_offset = 0x8c,
> > +	.clk_ctrl = true,
> 
> This appears to be done only for sam9g45.
> 
> So, Alexander, this should not impact sam9x60 .

Yes it should.  According to arch/arm/boot/dts/microchip/sam9x60.dtsi
the compatible used on sam9x60 is "atmel,at91sam9g45-pmecc", see:

	pmecc: ecc-engine@ffffe000 {
			compatible = "microchip,sam9x60-pmecc", "atmel,at91sam9g45-pmecc";
			reg = <0xffffe000 0x300>,
					<0xffffe600 0x100>;
	};

> I looked a bit in the Linux driver and this appears to be just as bad:
> 133 Mhz clock config hardcoded. I would expect this clock to be taken
> from a clock input and not hardcoded. But oh well, if this clock is to
> be changed in another device, it would be exposed somewhere...

I did not look into that deeply, but if it is like you said, someone
should.  This sounds bad.

Greets
Alex

> 
> Eugen
> 
> >  };
> >  
> >  static struct atmel_pmecc_caps sama5d4_caps = {
> > 
>