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

[Masahiro Yamada <masahiroy@xxxxxxxxxx>]

On Thu, Dec 12, 2019 at 10:06 AM Marek Vasut <marex@xxxxxxx> wrote:
>
> On 12/10/19 7:30 AM, Masahiro Yamada wrote:
> > 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 it is broken now ?

I do not know.
As I already said, there is no perfect solution about what
to do when SPARE_AREA_SKIP_BYTES is zero.

I received various feedback from SOCFPGA board users
about this driver, but nothing from denali_pci platfrom users.
Absolutely zero question/complaint.

I suspect there is no user of that platform, but who knows.


> If so, then that fix is for another patch.

So, do you want me to get back the original behavior,
then you want to send a new patch based on that?

I think it is a waste of time, but
it would be less worse than continuing this thread.



> >> 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.
>
> Or maybe
>
> commit 0d55c668b218a1db68b5044bce4de74e1bd0f0c8
>     mtd: rawnand: denali: set SPARE_AREA_SKIP_BYTES register to 8 if unset
>
> should be reverted, since it changed the behavior ?
>
> [...]



-- 
Best Regards
Masahiro Yamada

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