Re: [PATCH v2 5/5] arm64: dts: uniphier: Add NX1 SoC and boards support
From: Kunihiko Hayashi
Date: Fri Nov 11 2022 - 11:42:26 EST
On 2022/11/11 23:40, Krzysztof Kozlowski wrote:
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@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@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.
This structure follow the existing UniPhier SoCs, so it is necessary
to re-think the structure of all SoCs, not just this NX1 SoC.
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.
In this same area, mode selector, clock selector, phy configuration etc,
exist together in a mixed manner. There is a kind of design issues.
From this point of view, I should separate the new devicetree once
from this patch series, and it is necessary to consider syscon DT schema
and related drivers for existing SoCs.
+ };
+ };
+
+ soc-glue@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.
It takes more time to review existing structures, especially patch 1/5.
Thank you,
---
Best Regards
Kunihiko Hayashi