| Message ID | 20210311191216.7363-4-p.yadav@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: Add OSPI PHY calibration support for spi-cadence-quadspi | expand |
Hi Pratyush, p.yadav@ti.com 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@ti.com> > --- > 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? > + spi_mem_do_calibration(nor->spimem, &op); A warning/info upon calibration error (not on the absence of the hook) would be nice? > + > + return 0; > } > > static int spi_nor_remove(struct spi_mem *spimem) Otherwise I like the overall idea. Thanks, Miquèl
+Cedric On 17/05/22 04:02PM, Miquel Raynal wrote: > Hi Pratyush, > > p.yadav@ti.com 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@ti.com> > > --- > > 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? 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(). > > > + 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
Hi Pratyush, p.yadav@ti.com wrote on Wed, 18 May 2022 11:37:05 +0530: > +Cedric > > On 17/05/22 04:02PM, Miquel Raynal wrote: > > Hi Pratyush, > > > > p.yadav@ti.com 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@ti.com> > > > --- > > > 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. 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. > > > > > > + 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
On 18/05/22 09:19AM, Miquel Raynal wrote: > Hi Pratyush, > > p.yadav@ti.com wrote on Wed, 18 May 2022 11:37:05 +0530: > > > +Cedric > > > > On 17/05/22 04:02PM, Miquel Raynal wrote: > > > Hi Pratyush, > > > > > > p.yadav@ti.com 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@ti.com> > > > > --- > > > > 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
Hello, On 5/18/22 09:56, Pratyush Yadav wrote: > On 18/05/22 09:19AM, Miquel Raynal wrote: >> Hi Pratyush, >> >> p.yadav@ti.com wrote on Wed, 18 May 2022 11:37:05 +0530: >> >>> +Cedric >>> >>> On 17/05/22 04:02PM, Miquel Raynal wrote: >>>> Hi Pratyush, >>>> >>>> p.yadav@ti.com 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@ti.com> >>>>> --- >>>>> 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. > 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. > 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. Thanks, C.
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@ti.com wrote on Wed, 18 May 2022 11:37:05 +0530: > > > > > > > +Cedric > > > > > > > > On 17/05/22 04:02PM, Miquel Raynal wrote: > > > > > Hi Pratyush, > > > > > > > > > > p.yadav@ti.com 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@ti.com> > > > > > > --- > > > > > > 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.
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@ti.com wrote on Wed, 18 May 2022 11:37:05 +0530: >>>> >>>>> +Cedric >>>>> >>>>> On 17/05/22 04:02PM, Miquel Raynal wrote: >>>>>> Hi Pratyush, >>>>>> >>>>>> p.yadav@ti.com 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@ti.com> >>>>>>> --- >>>>>>> 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.
On 27/06/22 11:43AM, Cédric Le Goater wrote: > On 6/27/22 11:14, Pratyush Yadav wrote: > > On 18/05/22 10:51AM, Cédric Le Goater wrote: [...] > > > > > 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. Hmm, yes but I've got this feedback from multiple people that we should not do the calibration after mtd_device_register() since at that point the device is usable from userspace. Maybe we can come up with an API from MTD parsers that can just return us a list of partitions but not actually register them? > > > > > > > > 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.
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); + spi_mem_do_calibration(nor->spimem, &op); + + return 0; } static int spi_nor_remove(struct spi_mem *spimem)
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@ti.com> --- drivers/mtd/spi-nor/core.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)