linux-kernel - Re: [PATCH 3/6] dt-bindings: pinctrl: Add fsl,ls1012a-pinctrl yaml file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6ddcd97d-7d60-8e4f-fede-42bf7f99e2b0@digi.com>
Date: Wed, 28 Aug 2024 10:43:15 +1000 (AEST)
From: David Leonard <David.Leonard@...i.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
cc: linux-arm-kernel@...ts.infradead.org, Dong Aisheng <aisheng.dong@....com>, 
    Fabio Estevam <festevam@...il.com>, Shawn Guo <shawnguo@...nel.org>, 
    Jacky Bai <ping.bai@....com>, 
    Pengutronix Kernel Team <kernel@...gutronix.de>, 
    Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>, 
    Krzysztof Kozlowski <krzk+dt@...nel.org>, 
    Conor Dooley <conor+dt@...nel.org>, linux-gpio@...r.kernel.org, 
    devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] dt-bindings: pinctrl: Add fsl,ls1012a-pinctrl yaml
 file

On Tue, 27 Aug 2024, Krzysztof Kozlowski wrote:

> On 27/08/2024 18:51, David Leonard wrote:
>>>> +properties:
>>>> +  compatible:
>>>> +    const: fsl,ls1012a-pinctrl
>>>> +
>>>> +  reg:
>>>> +    description: Specifies the base address of the PMUXCR0 register.
>>>> +    maxItems: 2
>>>
>>> Instead list and describe the items.
>>
>> Changed to
>>
>>     reg:
>>       items:
>>         - description: Physical base address of the PMUXCR0 register.
>>         - description: Size of the PMUXCR0 register (4).
>>
>> Is this what you meant?
>
> Almost, second reg is not a size. You claim there are two IO address
> spaces. Each address space contains base address and size. Look at other
> bindings how they do it.

Previously, dt_binding_check was giving me a warning that 'maxItems' needed
to be supplied. Which confused me because I thought of reg as an opaque subspace
descriptor, but I was just trying to placate the checks. After tool upgrades,
that warning doesn't appear any more.

So the neatest documentation would be:

   reg:
     description: Specifies the address of the PMUXCR0 register.

>>>> +  big-endian:
>>>> +    description: If present, the PMUXCR0 register is implemented in big-endian.
>>>
>>> Why is this here? Either it is or it is not?
>>
>> You're right. Changed to
>>
>>     big-endian: true
>>
>> (This also lead to some code simplification)
>
> OK, but I still wonder why is it here. Without it the hardware will work
> in little-endian?

Well, it's here firstly because I was trying to follow a perceived convention in
dts/freescale/fsl,ls1012a.dtsi. That DT uses big-endian in child
nodes of /soc that match up with memory map tables from the datasheet.
(Not only do different and adjacent parts of the register map have
opposing endianess, some register layouts also seem to be reversed
bitwise, others bytewise.)

The second reason is practical/dodgy. The pinctrl driver should logically
be a child of the scfg node, but the syscon driver doesn't populate its
child nodes. To get the pinctrl driver to work meant making it a sibling
node with an unsatisfactory overlap with the scfg's address region
0x1570000+0x10000. (No driver binds to "fsl,ls1012a-scfg".)

         soc: soc {
                 compatible = "simple-bus";
                 #address-cells = <2>;
                 #size-cells = <2>;
 		...
                 scfg: scfg@...0000 {
                         compatible = "fsl,ls1012a-scfg", "syscon";
                         reg = <0x0 0x1570000 0x0 0x10000>;
                         big-endian;
 		};
 		pinmux: pinmux@...040c {
                         compatible = "fsl,ls1046a-pinctrl";
                         reg = <0 0x157040c 0 4>;
                         big-endian;
 			...
 		};
 	};

The better device tree would be:

         soc: soc {
                 compatible = "simple-bus";
 		...
                 scfg: scfg@...0000 {
                         compatible = "fsl,ls1046a-scfg", "syscon";
                         big-endian;
                         #address-cells = <1>;
                         #size-cells = <1>;
 			...
 			pinmux: pinmux@40c {
 				compatible = "fsl,ls1046a-pinctrl";
 				reg = <0x40c 4>;
 				...
 			};
 		};
 	};

And this would resolve the big-endian property issue.

There was a discussion of syscon populating its child nodes at
https://lore.kernel.org/lkml/1403513950.4136.34.camel@paszta.hi.pengutronix.de/T/

Cheers,

David