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

From: Cédric Le Goater
Date: Mon Jun 27 2022 - 06:01:57 EST

On 6/27/22 11:14, Pratyush Yadav wrote:
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.

True. I don't exactly remember how your proposal worked from the
driver side but I imagine having a specific DT property to locate
that pattern in the setup handler and to use it later on is not
too complex.

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.

OK. and that it would become more generic then.

I have not thought this through yet though.

If a partition is required, that's a dependency on mtdpart.

It could be done from spi_nor_probe() after mtd_device_register() with
some spimem handler using the 'struct mtd_partition' for the {size,offset}
parameters.


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.

I know you would prefer a specific handler and that can still be done.

Thanks,

C.