diff mbox series

[v3,7/8] dt-bindings: mfd: Add img,boston-platform-regs

Message ID 20240618-boston-syscon-v3-7-c47c06647a26@flygoat.com (mailing list archive)
State New
Headers show
Series MIPS: Boston: Fix syscon devicetree binding and node | expand

Commit Message

Jiaxun Yang June 18, 2024, 3:11 p.m. UTC
This compatible has been used in arch/mips/boot/dts/img/boston.dts
for a while but never documented properly.

Write a new binding for this device.
This also covers old img,boston-clock binding so remove that binding.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
v3: Rename clock and reboot node
---
 .../devicetree/bindings/clock/img,boston-clock.txt | 31 ---------
 .../bindings/mfd/img,boston-platform-regs.yaml     | 74 ++++++++++++++++++++++
 2 files changed, 74 insertions(+), 31 deletions(-)

Comments

Krzysztof Kozlowski June 19, 2024, 9:28 a.m. UTC | #1
On 18/06/2024 17:11, Jiaxun Yang wrote:
> This compatible has been used in arch/mips/boot/dts/img/boston.dts
> for a while but never documented properly.
> 

> diff --git a/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml b/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml
> new file mode 100644
> index 000000000000..79cae87c6758
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/img,boston-platform-regs.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Imagination Technologies Boston Platform Registers
> +
> +maintainers:
> +  - Jiaxun Yang <jiaxun.yang@flygoat.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: img,boston-platform-regs
> +      - const: syscon
> +      - const: simple-mfd


Fix U-boot to populate devices instead of relying on simple-mfd.

> +
> +  reg:
> +    maxItems: 1
> +
> +  clock-controller:
> +    type: object
> +
> +    description: Boston Clock Controller Device Node
> +      The clock consumer should specify the desired clock by having the clock
> +      ID in its "clocks" phandle cell.
> +      See include/dt-bindings/clock/boston-clock.h for the full list of boston
> +      clock IDs.
> +
> +    properties:
> +      "#clock-cells":
> +        const: 1
> +
> +      compatible:
> +        const: img,boston-clock

Please put compatible first in the list of properties (and follow the
same order in "required"). It's the most important piece, so we want it
to be the first to see. It also follows the convention of DTS, where
compatible is expected to be first.

> +
> +    required:
> +      - "#clock-cells"
> +      - compatible
> +
> +    additionalProperties: false
> +
> +  reboot:
> +    $ref: /schemas/power/reset/syscon-reboot.yaml#


Best regards,
Krzysztof
Jiaxun Yang June 19, 2024, 11:20 a.m. UTC | #2
在2024年6月19日六月 上午10:28,Krzysztof Kozlowski写道:
> On 18/06/2024 17:11, Jiaxun Yang wrote:
>> This compatible has been used in arch/mips/boot/dts/img/boston.dts
>> for a while but never documented properly.
>> 
>
>> diff --git a/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml b/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml
>> new file mode 100644
>> index 000000000000..79cae87c6758
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml
>> @@ -0,0 +1,74 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/mfd/img,boston-platform-regs.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Imagination Technologies Boston Platform Registers
>> +
>> +maintainers:
>> +  - Jiaxun Yang <jiaxun.yang@flygoat.com>
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: img,boston-platform-regs
>> +      - const: syscon
>> +      - const: simple-mfd
>
>
> Fix U-boot to populate devices instead of relying on simple-mfd.

Hi Krzysztof,

I believe U-Boot's implementation is correct. As per simple-mfd binding:

```
simple-mfd" - this signifies that the operating system should
  consider all subnodes of the MFD device as separate devices akin to how
  "simple-bus" indicates when to see subnodes as children for a simple
  memory-mapped bus.
```

This reads to me as "if you want sub nodes to be populated as devices
you need this."

In our case there are "clock" and "reset" node sub nodes which should be
probed as regular device, so it's true for us.

Linux managed to work without "simple-mfd" only because clock subsystem
is bypassing regular OF population process. Semantically we need this.

Besides Linux as upstream of devicetree source had accepted U-Boot
only stuff here, such as "bootph-all" property.

Thanks
- Jiaxun
>
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clock-controller:
>> +    type: object
>> +
>> +    description: Boston Clock Controller Device Node
>> +      The clock consumer should specify the desired clock by having the clock
>> +      ID in its "clocks" phandle cell.
>> +      See include/dt-bindings/clock/boston-clock.h for the full list of boston
>> +      clock IDs.
>> +
>> +    properties:
>> +      "#clock-cells":
>> +        const: 1
>> +
>> +      compatible:
>> +        const: img,boston-clock
>
> Please put compatible first in the list of properties (and follow the
> same order in "required"). It's the most important piece, so we want it
> to be the first to see. It also follows the convention of DTS, where
> compatible is expected to be first.
>
>> +
>> +    required:
>> +      - "#clock-cells"
>> +      - compatible
>> +
>> +    additionalProperties: false
>> +
>> +  reboot:
>> +    $ref: /schemas/power/reset/syscon-reboot.yaml#
>
>
> Best regards,
> Krzysztof
Krzysztof Kozlowski June 20, 2024, 6:40 a.m. UTC | #3
On 19/06/2024 13:20, Jiaxun Yang wrote:
> 
> 
> 在2024年6月19日六月 上午10:28,Krzysztof Kozlowski写道:
>> On 18/06/2024 17:11, Jiaxun Yang wrote:
>>> This compatible has been used in arch/mips/boot/dts/img/boston.dts
>>> for a while but never documented properly.
>>>
>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml b/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml
>>> new file mode 100644
>>> index 000000000000..79cae87c6758
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml
>>> @@ -0,0 +1,74 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/mfd/img,boston-platform-regs.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Imagination Technologies Boston Platform Registers
>>> +
>>> +maintainers:
>>> +  - Jiaxun Yang <jiaxun.yang@flygoat.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - const: img,boston-platform-regs
>>> +      - const: syscon
>>> +      - const: simple-mfd
>>
>>
>> Fix U-boot to populate devices instead of relying on simple-mfd.
> 
> Hi Krzysztof,
> 
> I believe U-Boot's implementation is correct. As per simple-mfd binding:
> 
> ```
> simple-mfd" - this signifies that the operating system should
>   consider all subnodes of the MFD device as separate devices akin to how
>   "simple-bus" indicates when to see subnodes as children for a simple
>   memory-mapped bus.
> ```
> 
> This reads to me as "if you want sub nodes to be populated as devices
> you need this."
> 
> In our case there are "clock" and "reset" node sub nodes which should be
> probed as regular device, so it's true for us.

No, you already got comment from Rob.

Your children depend on parent to provide IO address, so this is not
simple-mfd. Rule for simple-mfd is that children do not rely on parent
at all.

> 
> Linux managed to work without "simple-mfd" only because clock subsystem
> is bypassing regular OF population process. Semantically we need this.

Semantically? No, you need proper populate, not incorrect simple-mfd.

> 
> Besides Linux as upstream of devicetree source had accepted U-Boot
> only stuff here, such as "bootph-all" property.

Because bootph-all is valid and correct. Calling simple-mfd something
which is not entirely simple-mfd is not that valid.


Best regards,
Krzysztof
Jiaxun Yang June 20, 2024, 4:13 p.m. UTC | #4
在2024年6月20日六月 上午7:40,Krzysztof Kozlowski写道:
> On 19/06/2024 13:20, Jiaxun Yang wrote:
>> 
>> 
>> 在2024年6月19日六月 上午10:28,Krzysztof Kozlowski写道:
[...]
> Your children depend on parent to provide IO address, so this is not
> simple-mfd. Rule for simple-mfd is that children do not rely on parent
> at all.

I don't really get this.

As per syscon-rebbot.yaml:

The SYSCON registers map is normally retrieved from the
  parental dt-node. So the SYSCON reboot node should be represented as a
  sub-node of a "syscon", "simple-mfd" node.

syscon-reboot is certainly using parent node to provide I/O address,
even regmap property is deprecated. Without "simple-mfd", reboot node
won't be populated at all.

I'll remove the dependency of clock controller to parent node anyway.

Thanks
- Jiaxun

>
>> 
>> Linux managed to work without "simple-mfd" only because clock subsystem
>> is bypassing regular OF population process. Semantically we need this.
>
> Semantically? No, you need proper populate, not incorrect simple-mfd.
>
>> 
>> Besides Linux as upstream of devicetree source had accepted U-Boot
>> only stuff here, such as "bootph-all" property.
>
> Because bootph-all is valid and correct. Calling simple-mfd something
> which is not entirely simple-mfd is not that valid.
>
>
> Best regards,
> Krzysztof
Krzysztof Kozlowski June 20, 2024, 4:16 p.m. UTC | #5
On 20/06/2024 18:13, Jiaxun Yang wrote:
> 
> 
> 在2024年6月20日六月 上午7:40,Krzysztof Kozlowski写道:
>> On 19/06/2024 13:20, Jiaxun Yang wrote:
>>>
>>>
>>> 在2024年6月19日六月 上午10:28,Krzysztof Kozlowski写道:
> [...]
>> Your children depend on parent to provide IO address, so this is not
>> simple-mfd. Rule for simple-mfd is that children do not rely on parent
>> at all.
> 
> I don't really get this.
> 
> As per syscon-rebbot.yaml:
> 
> The SYSCON registers map is normally retrieved from the
>   parental dt-node. So the SYSCON reboot node should be represented as a
>   sub-node of a "syscon", "simple-mfd" node.
> 
> syscon-reboot is certainly using parent node to provide I/O address,
> even regmap property is deprecated. Without "simple-mfd", reboot node
> won't be populated at all.

That's why I asked you to populate children.

Best regards,
Krzysztof
Jiaxun Yang June 20, 2024, 4:32 p.m. UTC | #6
在2024年6月20日六月 下午5:16,Krzysztof Kozlowski写道:
[...]
>> syscon-reboot is certainly using parent node to provide I/O address,
>> even regmap property is deprecated. Without "simple-mfd", reboot node
>> won't be populated at all.
>
> That's why I asked you to populate children.

Do you mean I should write a driver for this?
This is a little bit overkilling, I'd rather fix the clock driver.

Thanks
>
> Best regards,
> Krzysztof
Jiaxun Yang June 21, 2024, 3:51 p.m. UTC | #7
在2024年6月20日六月 上午7:40,Krzysztof Kozlowski写道:
[...]
>> 
>> Hi Krzysztof,
>> 
>> I believe U-Boot's implementation is correct. As per simple-mfd binding:
>> 
>> ```
>> simple-mfd" - this signifies that the operating system should
>>   consider all subnodes of the MFD device as separate devices akin to how
>>   "simple-bus" indicates when to see subnodes as children for a simple
>>   memory-mapped bus.
>> ```
>> 
>> This reads to me as "if you want sub nodes to be populated as devices
>> you need this."
>> 
>> In our case there are "clock" and "reset" node sub nodes which should be
>> probed as regular device, so it's true for us.
>
> No, you already got comment from Rob.
>
> Your children depend on parent to provide IO address, so this is not
> simple-mfd. Rule for simple-mfd is that children do not rely on parent
> at all.
>
Hi Krzysztof,

Sorry but can I ask for clarification on "depend on parent to provide IO
address", do you mind explaining it a little bit? Does it mean children
should get regmap node from a phandle property, not the parent node? Or there
should be a reg property for child node to tell register offset etc?

There are way too much usage that children "depends" on parents somehow
in tree, so I want to confirm my understanding.

For boston-platform-regs there are some other PHYs that I may add drivers
for them in future, so I certainly want "simple-mfd" to be here
Krzysztof Kozlowski June 22, 2024, 6:12 p.m. UTC | #8
On 21/06/2024 17:51, Jiaxun Yang wrote:
> 
> 
> 在2024年6月20日六月 上午7:40,Krzysztof Kozlowski写道:
> [...]
>>>
>>> Hi Krzysztof,
>>>
>>> I believe U-Boot's implementation is correct. As per simple-mfd binding:
>>>
>>> ```
>>> simple-mfd" - this signifies that the operating system should
>>>   consider all subnodes of the MFD device as separate devices akin to how
>>>   "simple-bus" indicates when to see subnodes as children for a simple
>>>   memory-mapped bus.
>>> ```
>>>
>>> This reads to me as "if you want sub nodes to be populated as devices
>>> you need this."
>>>
>>> In our case there are "clock" and "reset" node sub nodes which should be
>>> probed as regular device, so it's true for us.
>>
>> No, you already got comment from Rob.
>>
>> Your children depend on parent to provide IO address, so this is not
>> simple-mfd. Rule for simple-mfd is that children do not rely on parent
>> at all.
>>
> Hi Krzysztof,
> 
> Sorry but can I ask for clarification on "depend on parent to provide IO
> address", do you mind explaining it a little bit? Does it mean children
> should get regmap node from a phandle property, not the parent node? Or there
> should be a reg property for child node to tell register offset etc?
> 
> There are way too much usage that children "depends" on parents somehow
> in tree, so I want to confirm my understanding.


Your driver relies on parent IO address to be provided - what's more to
explain here? If parent does not provide syscon, does the child work?
No. Therefore it is not suited for simple-mfd.

> 
> For boston-platform-regs there are some other PHYs that I may add drivers
> for them in future, so I certainly want "simple-mfd" to be here 

Well, I want a new Ducati, but we don't always get what we want, right?

Best regards,
Krzysztof
Jiaxun Yang June 22, 2024, 7:06 p.m. UTC | #9
在2024年6月22日六月 下午7:12,Krzysztof Kozlowski写道:
> On 21/06/2024 17:51, Jiaxun Yang wrote:
>> 
>> 
>> 在2024年6月20日六月 上午7:40,Krzysztof Kozlowski写道:
>> [...]
>>>>
>>>> Hi Krzysztof,
>>>>
>>>> I believe U-Boot's implementation is correct. As per simple-mfd binding:
>>>>
>>>> ```
>>>> simple-mfd" - this signifies that the operating system should
>>>>   consider all subnodes of the MFD device as separate devices akin to how
>>>>   "simple-bus" indicates when to see subnodes as children for a simple
>>>>   memory-mapped bus.
>>>> ```
>>>>
>>>> This reads to me as "if you want sub nodes to be populated as devices
>>>> you need this."
>>>>
>>>> In our case there are "clock" and "reset" node sub nodes which should be
>>>> probed as regular device, so it's true for us.
>>>
>>> No, you already got comment from Rob.
>>>
>>> Your children depend on parent to provide IO address, so this is not
>>> simple-mfd. Rule for simple-mfd is that children do not rely on parent
>>> at all.
>>>
>> Hi Krzysztof,
>> 
>> Sorry but can I ask for clarification on "depend on parent to provide IO
>> address", do you mind explaining it a little bit? Does it mean children
>> should get regmap node from a phandle property, not the parent node? Or there
>> should be a reg property for child node to tell register offset etc?
>> 
>> There are way too much usage that children "depends" on parents somehow
>> in tree, so I want to confirm my understanding.
>
>
> Your driver relies on parent IO address to be provided - what's more to
> explain here? If parent does not provide syscon, does the child work?
> No. Therefore it is not suited for simple-mfd.
>
I can name too much "simple-mfd" devices that depending on parent to get
it's syscon, in fact it's true for almost all "simple-mfd" users now.

I greped RISC-V's DTS, and all two users have child nodes depends on parent
node to get "IO address". For "canaan,k210-sysctl" it's both "canaan,k210-clk"
and "canaan,k210-rst", for "starfive,jh7110-sys-syscon" that's "starfive,jh7110-pll".

If that's something prohibited, then we may need a generic driver in kernel to
catch all those syscon devices lost eligibility to "simple-mfd" to get their childs
populated.

We also need to think about how to handle "syscon-reboot-mode" and "syscon-reboot".
"syscon-reboot-mode" is explicitly relying on parent IO address, for "syscon-reboot"
the ability of not relying on parent node (regmap property) is deprecated as well.

I think we need to make those rules explicit, I'm happy to write a document or update
Devicetree specification about that, but I need to make it crystal clear to myself first.

Thanks
[...]
>
> Best regards,
> Krzysztof
Rob Herring (Arm) June 27, 2024, 8:44 p.m. UTC | #10
On Wed, Jun 19, 2024 at 12:20:54PM +0100, Jiaxun Yang wrote:
> 
> 
> 在2024年6月19日六月 上午10:28,Krzysztof Kozlowski写道:
> > On 18/06/2024 17:11, Jiaxun Yang wrote:
> >> This compatible has been used in arch/mips/boot/dts/img/boston.dts
> >> for a while but never documented properly.
> >> 
> >
> >> diff --git a/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml b/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml
> >> new file mode 100644
> >> index 000000000000..79cae87c6758
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml
> >> @@ -0,0 +1,74 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/mfd/img,boston-platform-regs.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Imagination Technologies Boston Platform Registers
> >> +
> >> +maintainers:
> >> +  - Jiaxun Yang <jiaxun.yang@flygoat.com>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    items:
> >> +      - const: img,boston-platform-regs
> >> +      - const: syscon
> >> +      - const: simple-mfd
> >
> >
> > Fix U-boot to populate devices instead of relying on simple-mfd.
> 
> Hi Krzysztof,
> 
> I believe U-Boot's implementation is correct. As per simple-mfd binding:
> 
> ```
> simple-mfd" - this signifies that the operating system should
>   consider all subnodes of the MFD device as separate devices akin to how
>   "simple-bus" indicates when to see subnodes as children for a simple
>   memory-mapped bus.
> ```
> 
> This reads to me as "if you want sub nodes to be populated as devices
> you need this."
> 
> In our case there are "clock" and "reset" node sub nodes which should be
> probed as regular device, so it's true for us.
> 
> Linux managed to work without "simple-mfd" only because clock subsystem
> is bypassing regular OF population process. Semantically we need this.

I'm confused. Neither u-boot nor linux .dts files have 'simple-mfd' for 
this binding. So why do you need it? Why are we changing a platform 
that's had 1 dts change since 2018?

If anything, add it in a separate patch and we can discuss it there 
instead of a conversion.

> Besides Linux as upstream of devicetree source had accepted U-Boot
> only stuff here, such as "bootph-all" property.

Yes, and there are things we've rejected. See Arm FFA threads if you 
want to waste a few hours reading.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/img,boston-clock.txt b/Documentation/devicetree/bindings/clock/img,boston-clock.txt
deleted file mode 100644
index 7bc5e9ffb624..000000000000
--- a/Documentation/devicetree/bindings/clock/img,boston-clock.txt
+++ /dev/null
@@ -1,31 +0,0 @@ 
-Binding for Imagination Technologies MIPS Boston clock sources.
-
-This binding uses the common clock binding[1].
-
-[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
-
-The device node must be a child node of the syscon node corresponding to the
-Boston system's platform registers.
-
-Required properties:
-- compatible : Should be "img,boston-clock".
-- #clock-cells : Should be set to 1.
-  Values available for clock consumers can be found in the header file:
-    <dt-bindings/clock/boston-clock.h>
-
-Example:
-
-	system-controller@17ffd000 {
-		compatible = "img,boston-platform-regs", "syscon";
-		reg = <0x17ffd000 0x1000>;
-
-		clk_boston: clock {
-			compatible = "img,boston-clock";
-			#clock-cells = <1>;
-		};
-	};
-
-	uart0: uart@17ffe000 {
-		/* ... */
-		clocks = <&clk_boston BOSTON_CLK_SYS>;
-	};
diff --git a/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml b/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml
new file mode 100644
index 000000000000..79cae87c6758
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/img,boston-platform-regs.yaml
@@ -0,0 +1,74 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/img,boston-platform-regs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Imagination Technologies Boston Platform Registers
+
+maintainers:
+  - Jiaxun Yang <jiaxun.yang@flygoat.com>
+
+properties:
+  compatible:
+    items:
+      - const: img,boston-platform-regs
+      - const: syscon
+      - const: simple-mfd
+
+  reg:
+    maxItems: 1
+
+  clock-controller:
+    type: object
+
+    description: Boston Clock Controller Device Node
+      The clock consumer should specify the desired clock by having the clock
+      ID in its "clocks" phandle cell.
+      See include/dt-bindings/clock/boston-clock.h for the full list of boston
+      clock IDs.
+
+    properties:
+      "#clock-cells":
+        const: 1
+
+      compatible:
+        const: img,boston-clock
+
+    required:
+      - "#clock-cells"
+      - compatible
+
+    additionalProperties: false
+
+  reboot:
+    $ref: /schemas/power/reset/syscon-reboot.yaml#
+
+required:
+  - compatible
+  - reg
+  - clock-controller
+  - reboot
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/boston-clock.h>
+
+    plat_regs: system-controller@17ffd000 {
+      compatible = "img,boston-platform-regs", "syscon", "simple-mfd";
+      reg = <0x17ffd000 0x1000>;
+
+      clk_boston: clock-controller {
+        compatible = "img,boston-clock";
+        #clock-cells = <1>;
+      };
+
+      reboot: reboot {
+        compatible = "syscon-reboot";
+        regmap = <&plat_regs>;
+        offset = <0x10>;
+        mask = <0x10>;
+      };
+    };