Re: [PATCH v2 11/35] mtd: spi-nor: winbond: Use manufacturer late_init() for OTP ops

Pratyush Yadav
2021-10-11T06:56:05Z

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.
> >
>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.