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

[Masahiro Yamada <masahiroy@xxxxxxxxxx>]

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.


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.





> >> @@ -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;


-- 
Best Regards
Masahiro Yamada

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