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

[Marek Vasut <marex@xxxxxxx>]

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 ?

If so, then that fix is for another patch.

>> 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 ?

[...]

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