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

[Marek Vasut <marex@xxxxxxx>]

On 12/12/19 5:12 AM, Masahiro Yamada wrote:
[...]
>>>>>>>> @@ -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's definitely two separate issues.

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

This patch at least retains the current behavior, so if someone wants to
restore the original behavior before 4.19, it can be even done on top of
this patch.

[...]

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