Re: [PATCH v1] mtd: parsers: ofpart: Fix parsing when size-cells is 0

Marek Vasut
2022-12-02T17:08:27Z

On 12/2/22 17:57, Miquel Raynal wrote:
> Hi Marek,

Hi,

>> On 12/2/22 17:42, Miquel Raynal wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>> [...]
>>
>>>>> However, it should not be empty, at the very least a reg property
>>>>> should indicate on which CS it is wired, as expected there:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/Documentation/devicetree/bindings/mtd/nand-chip.yaml?h=mtd/next
>>>>
>>>> OK, I see your point. So basically this?
>>>>
>>>> &gpmi {
>>>> #size-cells = <1>;
>>>> ...
>>>> nand-chip@0 {
>>>> reg = <0>;
>>>> };
>>>> };
>>>>
>>>> btw. the GPMI NAND controller supports only one chipselect, so the reg in nand-chip node makes little sense.
>>>
>>> I randomly opened a reference manual (IMX6DQL.pdf), they say:
>>>
>>> "Up to four NAND devices, supported by four chip-selects and one
>>> ganged ready/ busy."
>>
>> Doh, and MX7D has the same controller, so size-cells = <1>; makes sense with nand-chip@N {} .
>
> Actually #address-cells is here for that. You need to point at one CS,
> so in most cases this is:
>
> controller {
> #address-cells = <1>;
> #size-cells = <0>;
> chip@N {
> reg = <N>;
> };
> };

Right ... thanks for spotting this.

But then the size-cells in the controller node should be zero. And
753395ea1e45 ("ARM: dts: imx7: Fix NAND controller size-cells") is
therefore correct ?

But that correction in 753395ea1e45 breaks setup where old U-Boot
injects partition@N directly into the nand-controller node, without
updating the nand-controller node size-cells, i.e. this:
nand-controller {
#address-cells = <1>;
#size-cells = <0>;

+ partition@0 { ... };
};
Because the size-cells is 0 in that case, and U-Boot does not update the
size-cells .

It used to work before because Linux DT contained size-cells=<1> in the
nand-controller node before 753395ea1e45 ("ARM: dts: imx7: Fix NAND
controller size-cells").

But here I would say this is a firmware bug and it might have to be
handled like a firmware bug, i.e. with fixup in the partition parser. I
seem to be changing my opinion here again.