Re: [PATCH] mtd: rawnand: denali_dt: Add support for configuring SPARE_AREA_SKIP_BYTES

[Masahiro Yamada <masahiroy@xxxxxxxxxx>]

On Tue, Dec 10, 2019 at 12:35 PM Marek Vasut <marex@xxxxxxx> wrote:
>
> On 12/10/19 4:15 AM, Masahiro Yamada wrote:
> > On Mon, Dec 9, 2019 at 9:57 PM Marek Vasut <marex@xxxxxxx> wrote:
> >>
> >> On 12/9/19 6:38 AM, Masahiro Yamada wrote:
> >> [...]
> >>
> >>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> >>>> index 3102ddbd8abdb..b6c463d021677 100644
> >>>> --- a/drivers/mtd/nand/raw/denali.c
> >>>> +++ b/drivers/mtd/nand/raw/denali.c
> >>>> @@ -1302,14 +1302,21 @@ int denali_init(struct denali_controller *denali)
> >>>>
> >>>>         /*
> >>>>          * Set how many bytes should be skipped before writing data in OOB.
> >>>> +        * If a non-zero value has already been configured, update it in HW.
> >>>>          * If a non-zero value has already been set (by firmware or something),
> >>>>          * just use it. Otherwise, set the driver's default.
> >>>>          */
> >>>> -       denali->oob_skip_bytes = ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>> -       if (!denali->oob_skip_bytes) {
> >>>> -               denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> >>>> +       if (denali->oob_skip_bytes) {
> >>>>                 iowrite32(denali->oob_skip_bytes,
> >>>>                           denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>> +       } else {
> >>>> +               denali->oob_skip_bytes =
> >>>> +                       ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>> +               if (!denali->oob_skip_bytes) {
> >>>> +                       denali->oob_skip_bytes = DENALI_DEFAULT_OOB_SKIP_BYTES;
> >>>> +                       iowrite32(denali->oob_skip_bytes,
> >>>> +                                 denali->reg + SPARE_AREA_SKIP_BYTES);
> >>>
> >>> This fallback is ugly, and should be removed, I think.
> >>> It is only reachable by PCI platform (Intel MRST), where
> >>> DENALI_DEFAULT_OOB_SKIP_BYTES is probably useless.
> >>
> >> This fallback retains the original behavior on all platforms. It might
> >> not be to your liking, but it does not break other platforms while
> >> fixing SoCFPGA. We don't know what other platforms might be depending on
> >> this behavior, do we ?
> >
> >      if (denali->oob_skip_bytes) {
> >                  iowrite32(denali->oob_skip_bytes,
> >                                  denali->reg + SPARE_AREA_SKIP_BYTES);
> >      else
> >                 denali->oob_skip_bytes =
> >                                  ioread32(denali->reg + SPARE_AREA_SKIP_BYTES);
> >
> >
> > ... retains the original behavior.
>
> It does not, because if the readback in the else branch sets
> oob_skip_bytes to 0, the controller is not updated with the default value.
>
> > For denali_dt.c, it now explicitly sets SPARE_AREA_SKIP_BYTES
> > to the well-defined value.
> >
> > denali_pci.c is the only platform that can read back the
> > register value.
> >
> > See, how Intel originally wrote the code.
> >
> > https://github.com/torvalds/linux/blob/v3.0/drivers/mtd/nand/denali.c#L1345
> >
> >
> > Please notice the part "if this value is 0, just let it be."
> > The Intel MRST platform happily accepts
> > SPARE_AREA_SKIP_BYTES being set to 0.
> >
> > I am not sure how many people are using this platform,
> > but anyway it is how it has worked for a long time.
>
> The intel platform might accept 0 happily, but that's not how the
> controller was configured for a long time.

It is.
It worked like that for 9 years. (i.e. v2.6.35 - v4.19)


> So if I were to change the
> code as you suggest, it would likely break some setups.

There is no perfect solution here
when SPARE_AREA_SKIP_BYTES was set to 0
before booting the kernel.

[A] Keep SPARE_AREA_SKIP_BYTES as it is.
     This might affect the factory-recorded BBM,
     but it should at least work if the firmware or the boot-loader
     has set up the controller this way.

[B] Override SPARE_AREA_SKIP_BYTES with
      a different value (8).
      This can keep the factory-based BBM, but
     this is very unlikely to work across software stages
     if the NAND device was formatted by the firmware or
     the boot-loader.


We need to give up something.
[A] was the original, 9 years' default, and cleaner.




> >>>> @@ -209,6 +213,8 @@ static int denali_dt_probe(struct platform_device *pdev)
> >>>>         denali->clk_rate = clk_get_rate(dt->clk);
> >>>>         denali->clk_x_rate = clk_get_rate(dt->clk_x);
> >>>>
> >>>> +       denali->oob_skip_bytes = data->oob_skip_bytes;
> >>>> +
> >>>
> >>> Please move this to the relevant hunk.
> >>> Preferably, based on this:
> >>> http://patchwork.ozlabs.org/patch/1205912/
> >>
> >> I don't understand what you want me to change ? Shall I rebase this on
> >> top of your patch from today and repost ?
> >
> > Yes.
> >
> > I do not want to scatter the relevant code
> > (coping struct fields) to random places.
> >
> > I want the code to look like this:
> >
> >        denali->revision = data->revision;
> >        denali->caps = data->caps;
> > +     denali->oob_skip_bytes = data->oob_skip_bytes;
> >        denali->ecc_caps = data->ecc_caps;
>
> Maybe you can rebase your patch on top of this one then ? It was posted
> later after all.

Neither is applied yet.
If you have a chance for v2,
it would not be so annoying to move the code
to the relevant place.

If you really do not like rebase,
the second best would be:



        if (data) {
                  denali->revision = data->revision;
                  denali->caps = data->caps;
+                denali->oob_skip_bytes = data->oob_skip_bytes;
                  denali->ecc_caps = data->ecc_caps;
        }

--
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/