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

[Pratyush Yadav]

On 18/05/22 10:51AM, Cédric Le Goater wrote:
> Hello,
> 
> On 5/18/22 09:56, Pratyush Yadav wrote:
> > 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.
> 
> Which would mean calling it right after :
> 
> 	ret = spi_nor_create_read_dirmap(nor);
> 	if (ret)
> 		return ret;
> 
> 	ret = spi_nor_create_write_dirmap(nor);
> 	if (ret)
> 		return ret;
> 
> > 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.
> 
> Why don't you simply choose some random place, first 16KB for instance,
> and check that the data is random enough ? If not, declare calibration
> not possible and choose a default safe setting which is anyhow a
> requirement for calibration. Retry at reboot as data might have changed.

I did not come up with the pattern myself. But from what I can 
understand, the pattern is not random at all, but is carefully chosen to 
target certain ways a read can fail on the controller. So a random piece 
of data won't work as well as this carefully designed pattern.

> 
> > 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().
> 
> hmm, that might work for some boards. This is not at all the case for
> the BMC boards. Vendors can put any kind of flash model and/or layout
> and the driver needs to be more generic.

Yes, vendors can choose any layout, but one partition on that layout 
would be your calibration pattern. I think you can use a different 
compatible for that partition. I have not thought this through yet 
though.

> 
> > 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.
> 
> Perfect, we can move the calibration hook in spi_nor_create_read_dirmap()
> then, or in devm_spi_mem_dirmap_create(), which would make more sense IMHO.

Sorry, I still don't get why you want to tie dirmap and calibration 
together. Just let them be independent and let flash drivers take care 
of when to call what. SPI MEM should not care.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.