[PATCH v2 5/5] arm64: dts: uniphier: Add NX1 SoC and boards support

[Krzysztof Kozlowski]

On 11/11/2022 09:48, Kunihiko Hayashi wrote:
> Hi Krzysztof,
> 
> On 2022/11/09 0:11, Krzysztof Kozlowski wrote:
>> On 08/11/2022 15:30, Kunihiko Hayashi wrote:
>>> Hi Krzysztof,
>>>
>>> On 2022/11/08 20:13, Krzysztof Kozlowski wrote:
>>>> On 07/11/2022 11:34, Kunihiko Hayashi wrote:
>>>>> Initial version of devicetree sources for NX1 SoC and boards.
>>>>>
>>>>> NX1 SoC belongs to the UniPhier armv8 architecture platform, and is
>>>>> designed for IoT and AI/ML application fields.
>>>>>
>>>>
>>>>> +
>>>>> +		soc_glue: syscon at 1f800000 {
>>>>> +			compatible = "socionext,uniphier-nx1-soc-glue",
>>>>> +				     "simple-mfd", "syscon";
>>>>> +			reg = <0x1f800000 0x2000>;
>>>>> +
>>>>> +			pinctrl: pinctrl {
>>>>> +				compatible = "socionext,uniphier-nx1-pinctrl";
>>>>
>>>> So instead of documenting the hardware precisily, you have one big bag
>>>> for everything under simple-mfd. This is not how the SoC should be
>>>> described in DTS.
>>>
>>> Sorry I don't understand. This is inherited from the previous
>>> descriptions,
>>> but is there some example to express DTS correctly about that?
>>
>> I think yes, although it actually depends what is this hardware.
>> Generally speaking, do not use simple-mfd and syscon when these are not
>> really simple devices. There are quite many in your DTS, which got my
>> attention. Instead - have regular device with or without children.
>>
>> There is no real need to have this a simple-mfd with one children
>> without any resources (no address space, no clocks, no interrupts, nothing).
>>
>> Why this syscon/mfd and pinctrl is not a regular, one device?
> 
> The mfd/syscon.yaml says:
>    System controller node represents a register region containing a set
>    of miscellaneous registers.
> 
> The "soc-glue" is exactly this, it contains various register functions
> and might be referred to the drivers.
> 
> For example in this NX1 dts, ethernet node points to "soc-glue" node.
> 
>      eth: ethernet at 15000000 {
>          compatible = "socionext,uniphier-nx1-ave4";
>          ...
>          socionext,syscon-phy-mode = <&soc_glue 0>;
>      };
> 
> Since such register region is not often systematically designed,
> it is tough to cut out as specific memory region for "pinctrl".

So your choice is instead use entire address space as pinctrl - as a
child device without IO address space. That's also not a good solution.

> 
> And more, the existing pinctrl driver uses of_get_parent() and
> syscon_node_to_regmap(), so this change breaks compatibility.

This is a new DTS, so what compatibility is broken? With old kernel?
There was no compatibility with this Devicetree. Anyway using driver
implementation as reason for specific hardware description (DTS) is also
not correct.

> 
>>>>> +			};
>>>>> +		};
>>>>> +
>>>>> +		soc-glue at 1f900000 {
>>>>> +			compatible = "simple-mfd";
>>>>
>>>> No, it is not allowed on its own. You need a specific compatible and
>>>> bindings describing its children.
>>>
>>> I saw the definition of "simple-mfd" itself is only in mfd/mfd.txt.
>>>
>>> Currently there are only efuse devices as children, and this space means
>>> nothing. I think it had better define the devices directly.
>>
>> You need to start describe the hardware. efuse is an efuse, not MFD.
>> pinctrl is pinctrl not MFD + pinctrl.
> 
> This region also has multiple functions, though, the efuse might be
> cut out as specific region without "simple-mfd", unlike pinctrl.

simple-mfd itself does not mean region has multiple functions, but that
children do not depend on anything from the parent device.

You over-use syscon and simple-mfd in multiple places. of course some of
them will be reasonable, but now it does not.


Best regards,
Krzysztof