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

[Marek Vasut <marex@xxxxxxx>]

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. So if I were to change the
code as you suggest, it would likely break some setups.

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

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