| Message ID | 20210727045222.905056-12-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: spi-nor: Handle ID collisions and clean params init | expand |
On 27/07/21 07:51AM, Tudor Ambarus wrote: > OTP info is not yet discoverable via SFDP, use late_init() to init > the OTP ops. What do you mean by the "yet"? Does it mean that OTP info is planned to be added to the next SFDP version? Or does it mean that it is possible to discover it via SFDP but we just don't support it yet? If it is neither and it just means "SFDP does not mention OTP at all", like it is for locking, then you should just drop the "yet". I know this is very nitpicky but it just caught my eye. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/winbond.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c > index 96573f61caf5..6be45d2291c6 100644 > --- a/drivers/mtd/spi-nor/winbond.c > +++ b/drivers/mtd/spi-nor/winbond.c > @@ -147,17 +147,22 @@ static const struct spi_nor_otp_ops winbond_otp_ops = { > static void winbond_default_init(struct spi_nor *nor) > { > nor->params->set_4byte_addr_mode = winbond_set_4byte_addr_mode; Why not move this to late_init() as well? > - if (nor->params->otp.org->n_regions) > - nor->params->otp.ops = &winbond_otp_ops; > } > > static const struct spi_nor_fixups winbond_fixups = { > .default_init = winbond_default_init, > }; > > +static void winbond_late_init(struct spi_nor *nor) > +{ > + if (nor->params->otp.org->n_regions) > + nor->params->otp.ops = &winbond_otp_ops; > +} > + > const struct spi_nor_manufacturer spi_nor_winbond = { > .name = "winbond", > .parts = winbond_parts, > .nparts = ARRAY_SIZE(winbond_parts), > .fixups = &winbond_fixups, > + .late_init = winbond_late_init, > }; > -- > 2.25.1 >
Am 2021-08-16 21:17, schrieb Pratyush Yadav: > On 27/07/21 07:51AM, Tudor Ambarus wrote: >> OTP info is not yet discoverable via SFDP, use late_init() to init >> the OTP ops. > > What do you mean by the "yet"? Does it mean that OTP info is planned to > be added to the next SFDP version? Or does it mean that it is possible > to discover it via SFDP but we just don't support it yet? I've seen a bit (again in the XTX SFDP) which indicates it supports OTP. But thats all. It doesn't mention _how_ you can actually use it. And there are at least two different variants. -michael > If it is neither and it just means "SFDP does not mention OTP at all", > like it is for locking, then you should just drop the "yet". I know > this > is very nitpicky but it just caught my eye.
On 8/16/21 10:17 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 27/07/21 07:51AM, Tudor Ambarus wrote: >> OTP info is not yet discoverable via SFDP, use late_init() to init >> the OTP ops. > > What do you mean by the "yet"? Does it mean that OTP info is planned to > be added to the next SFDP version? Or does it mean that it is possible > to discover it via SFDP but we just don't support it yet? > > If it is neither and it just means "SFDP does not mention OTP at all", > like it is for locking, then you should just drop the "yet". I know this > is very nitpicky but it just caught my eye. I will update according to your suggestion. > >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> drivers/mtd/spi-nor/winbond.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c >> index 96573f61caf5..6be45d2291c6 100644 >> --- a/drivers/mtd/spi-nor/winbond.c >> +++ b/drivers/mtd/spi-nor/winbond.c >> @@ -147,17 +147,22 @@ static const struct spi_nor_otp_ops winbond_otp_ops = { >> static void winbond_default_init(struct spi_nor *nor) >> { >> nor->params->set_4byte_addr_mode = winbond_set_4byte_addr_mode; > > Why not move this to late_init() as well? 4byte mode is SFDP discoverable. Ideally we would get rid of the default_init() hook. Flashes that define SFDP will get the 4byte mode from SFDP, the others by explicitly setting the late_init() hook. All these should be done at flash level, not manufacturer level, otherwise it will be hard to guess who sets what, and we can end up with fixups for fixups. I'll parse the 4byte mode from SFDP soon, I think I have some patches somewhere. But the series is getting big, so maybe I'll keep it after this patch set. > >> - if (nor->params->otp.org->n_regions) >> - nor->params->otp.ops = &winbond_otp_ops; >> } >> >> static const struct spi_nor_fixups winbond_fixups = { >> .default_init = winbond_default_init, >> }; >> >> +static void winbond_late_init(struct spi_nor *nor) >> +{ >> + if (nor->params->otp.org->n_regions) >> + nor->params->otp.ops = &winbond_otp_ops; >> +} >> + >> const struct spi_nor_manufacturer spi_nor_winbond = { >> .name = "winbond", >> .parts = winbond_parts, >> .nparts = ARRAY_SIZE(winbond_parts), >> .fixups = &winbond_fixups, >> + .late_init = winbond_late_init, >> }; >> -- >> 2.25.1 >> > > -- > Regards, > Pratyush Yadav > Texas Instruments Inc. >
On 9/10/21 12:50 AM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2021-08-16 21:17, schrieb Pratyush Yadav: >> On 27/07/21 07:51AM, Tudor Ambarus wrote: >>> OTP info is not yet discoverable via SFDP, use late_init() to init >>> the OTP ops. >> >> What do you mean by the "yet"? Does it mean that OTP info is planned to >> be added to the next SFDP version? Or does it mean that it is possible >> to discover it via SFDP but we just don't support it yet? > > I've seen a bit (again in the XTX SFDP) which indicates it supports > OTP. But thats all. It doesn't mention _how_ you can actually use it. > And there are at least two different variants. > I've checked JEDEC Standard No. 216D.01, there are some OTP bits, but no OTP memory as we handle in SPI NOR. https://datasheet.lcsc.com/szlcsc/2005251034_XTX-XT25F128BSSIGT_C558844.pdf, Table 5, pg. 60 mentions something about Secured OTP, but that is a vendor table, we're good for now. cheers, ta
On 01/10/21 11:54AM, Tudor.Ambarus@microchip.com wrote: > On 8/16/21 10:17 PM, Pratyush Yadav wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On 27/07/21 07:51AM, Tudor Ambarus wrote: > >> OTP info is not yet discoverable via SFDP, use late_init() to init > >> the OTP ops. > > > > What do you mean by the "yet"? Does it mean that OTP info is planned to > > be added to the next SFDP version? Or does it mean that it is possible > > to discover it via SFDP but we just don't support it yet? > > > > If it is neither and it just means "SFDP does not mention OTP at all", > > like it is for locking, then you should just drop the "yet". I know this > > is very nitpicky but it just caught my eye. > > I will update according to your suggestion. > > > > >> > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > >> --- > >> drivers/mtd/spi-nor/winbond.c | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c > >> index 96573f61caf5..6be45d2291c6 100644 > >> --- a/drivers/mtd/spi-nor/winbond.c > >> +++ b/drivers/mtd/spi-nor/winbond.c > >> @@ -147,17 +147,22 @@ static const struct spi_nor_otp_ops winbond_otp_ops = { > >> static void winbond_default_init(struct spi_nor *nor) > >> { > >> nor->params->set_4byte_addr_mode = winbond_set_4byte_addr_mode; > > > > Why not move this to late_init() as well? > > 4byte mode is SFDP discoverable. Ideally we would get rid of the default_init() > hook. Flashes that define SFDP will get the 4byte mode from SFDP, the others > by explicitly setting the late_init() hook. All these should be done at flash > level, not manufacturer level, otherwise it will be hard to guess who sets what, > and we can end up with fixups for fixups. Ok, makes sense. > > I'll parse the 4byte mode from SFDP soon, I think I have some patches somewhere. > But the series is getting big, so maybe I'll keep it after this patch set. Which table has that information? BFPT DWORD 16? Anyway, I agree with keeping it after this patch set. > > > > >> - if (nor->params->otp.org->n_regions) > >> - nor->params->otp.ops = &winbond_otp_ops; > >> } > >> > >> static const struct spi_nor_fixups winbond_fixups = { > >> .default_init = winbond_default_init, > >> }; > >> > >> +static void winbond_late_init(struct spi_nor *nor) > >> +{ > >> + if (nor->params->otp.org->n_regions) > >> + nor->params->otp.ops = &winbond_otp_ops; > >> +} > >> + > >> const struct spi_nor_manufacturer spi_nor_winbond = { > >> .name = "winbond", > >> .parts = winbond_parts, > >> .nparts = ARRAY_SIZE(winbond_parts), > >> .fixups = &winbond_fixups, > >> + .late_init = winbond_late_init, > >> }; > >> -- > >> 2.25.1 > >> > > > > -- > > Regards, > > Pratyush Yadav > > Texas Instruments Inc. > > >
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c index 96573f61caf5..6be45d2291c6 100644 --- a/drivers/mtd/spi-nor/winbond.c +++ b/drivers/mtd/spi-nor/winbond.c @@ -147,17 +147,22 @@ static const struct spi_nor_otp_ops winbond_otp_ops = { static void winbond_default_init(struct spi_nor *nor) { nor->params->set_4byte_addr_mode = winbond_set_4byte_addr_mode; - if (nor->params->otp.org->n_regions) - nor->params->otp.ops = &winbond_otp_ops; } static const struct spi_nor_fixups winbond_fixups = { .default_init = winbond_default_init, }; +static void winbond_late_init(struct spi_nor *nor) +{ + if (nor->params->otp.org->n_regions) + nor->params->otp.ops = &winbond_otp_ops; +} + const struct spi_nor_manufacturer spi_nor_winbond = { .name = "winbond", .parts = winbond_parts, .nparts = ARRAY_SIZE(winbond_parts), .fixups = &winbond_fixups, + .late_init = winbond_late_init, };
OTP info is not yet discoverable via SFDP, use late_init() to init the OTP ops. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/winbond.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)