Re: [RFC PATCH 3/6] mtd: spi-nor: core: run calibration when initialization is done

[Pratyush Yadav <p.yadav@xxxxxx>]

On 18/05/22 09:19AM, Miquel Raynal wrote:
> Hi Pratyush,
> 
> p.yadav@xxxxxx wrote on Wed, 18 May 2022 11:37:05 +0530:
> 
> > +Cedric
> > 
> > On 17/05/22 04:02PM, Miquel Raynal wrote:
> > > Hi Pratyush,
> > > 
> > > p.yadav@xxxxxx wrote on Fri, 12 Mar 2021 00:42:13 +0530:
> > >   
> > > > Once the flash is initialized tell the controller it can run
> > > > calibration procedures if needed. This can be useful when calibration is
> > > > needed to run at higher clock speeds.
> > > > 
> > > > Signed-off-by: Pratyush Yadav <p.yadav@xxxxxx>
> > > > ---
> > > >  drivers/mtd/spi-nor/core.c | 12 ++++++++++--
> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > > > index 88888df009f0..e0cbcaf1be89 100644
> > > > --- a/drivers/mtd/spi-nor/core.c
> > > > +++ b/drivers/mtd/spi-nor/core.c
> > > > @@ -3650,6 +3650,7 @@ static int spi_nor_probe(struct spi_mem *spimem)
> > > >  	 * checking what's really supported using spi_mem_supports_op().
> > > >  	 */
> > > >  	const struct spi_nor_hwcaps hwcaps = { .mask = SNOR_HWCAPS_ALL };
> > > > +	struct spi_mem_op op;
> > > >  	char *flash_name;
> > > >  	int ret;
> > > >  
> > > > @@ -3709,8 +3710,15 @@ static int spi_nor_probe(struct spi_mem *spimem)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
> > > > -				   data ? data->nr_parts : 0);
> > > > +	ret = mtd_device_register(&nor->mtd, data ? data->parts : NULL,
> > > > +				  data ? data->nr_parts : 0);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	op = spi_nor_spimem_get_read_op(nor);  
> > > 
> > > Isn't this too specific? I really don't know much about spi-nors, but I
> > > find odd to have this op being created here, why not moving this into
> > > the _do_calibration() helper?  
> > 
> > Maybe the naming confused you but this is a function in the SPI NOR 
> > core, not in SPI MEM. SPI NOR supports both SPI MEM based controllers 
> > and "legacy" controllers, so the convention is to add the "spimem" 
> > prefix before SPI MEM specific functions. So I don't get the comment 
> > about it being too specific. It is too specific to what?
> 
> Mmh right, it's fine then.
> 
> > 
> > And how can spi_mem_do_calibration() know what op the flash uses to read 
> > data? SPI NOR or SPI NAND would know it, but not SPI MEM. That is why we 
> > pass in that information to spi_mem_do_calibration().
> 
> But here the op is "spi-nor wide", I would have expected a
> per-device op. But that is not a big deal, that is something that can
> also be updated later if needed I guess.

It is per-device. The op is generated using nor->read_opcode, 
nor->addr_width, nor->read_dummy, etc. So if you have 2 NOR flashes on 
your system with different opcodes, it would work for both.

> 
> One last question, is there something that mtd_device_register() does
> that is really needed for the calibration to work? Otherwise I would
> rather prefer to have that calibration happening before the user gets
> access to the device.

The calibration works by reading a known pattern that is already written 
to the flash again and again and seeing what delays work and what don't. 
For that to happen, the controller driver needs to know where the 
pattern is stored. This series does that by looking at the MTD 
partitions. For that to happen, we need to create those partitions 
first, which happens after mtd_device_register().

But I am planning to use device tree to get that information now so this 
should no longer be needed and we can do calibration before registering 
the device with MTD.

> 
> > 
> > >   
> > > > +	spi_mem_do_calibration(nor->spimem, &op);  
> > > 
> > > A warning/info upon calibration error (not on the absence of the hook)
> > > would be nice?  
> > 
> > Yes, agreed.
> > 
> > >   
> > > > +
> > > > +	return 0;
> > > >  }
> > > >  
> > > >  static int spi_nor_remove(struct spi_mem *spimem)  
> > > 
> > > Otherwise I like the overall idea.  
> > 
> > Thanks for reviewing.
> > 
> > > 
> > > Thanks,
> > > Miquèl  
> > 
> 
> 
> Thanks,
> Miquèl

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.