[PATCH v3 0/8] MIPS: Boston: Fix syscon devicetree binding and node

[PATCH v3 0/8] MIPS: Boston: Fix syscon devicetree binding and node

[Jiaxun Yang]

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
Changes in v3:
- Drop CPU bindings in question
- See individual patches
- Link to v2: https://lore.kernel.org/r/20240612-boston-syscon-v2-0-9f8e1a07fa63@flygoat.com

Changes in v2:
- Write a new binding for this device (Krzysztof)
- Sweep other warnings
- Link to v1: https://lore.kernel.org/r/20240513-boston-syscon-v1-0-93ff557d3548@flygoat.com

---
Jiaxun Yang (8):
      MIPS: dts: Boston: Add simple-mfd compatible for syscon
      MIPS: dts: Boston: Move syscon-reboot node under syscon
      MIPS: dts: Boston: Rename clock node as clock-controller
      MIPS: dts: Boston: Remove unused #interrupt-cells
      MIPS: dts: Boston: Add model property
      MIPS: dts: Boston: Rename uart node
      dt-bindings: mfd: Add img,boston-platform-regs
      dt-bindings: mips: img: Add devices binding

 .../devicetree/bindings/clock/img,boston-clock.txt | 31 --------
 .../bindings/mfd/img,boston-platform-regs.yaml     | 74 +++++++++++++++++++
 .../devicetree/bindings/mips/img/devices.yaml      | 33 +++++++++
 .../bindings/mips/img/pistachio-marduk.txt         | 10 ---
 .../devicetree/bindings/mips/img/pistachio.txt     | 42 -----------
 .../devicetree/bindings/mips/img/xilfpga.txt       | 83 ----------------------
 arch/mips/boot/dts/img/boston.dts                  | 25 ++++---
 7 files changed, 119 insertions(+), 179 deletions(-)
---
base-commit: 6ba6c795dc73c22ce2c86006f17c4aa802db2a60
change-id: 20240513-boston-syscon-8e315e94a894

Best regards,
-- 
Jiaxun Yang <jiaxun.yang@flygoat.com>

[PATCH v3 1/8] MIPS: dts: Boston: Add simple-mfd compatible for syscon

[Jiaxun Yang]

We certainly want subnodes of system-controller node to
be populated, add simple-mfd compatible to make that happen.

This is required by U-Boot.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
v3: Update commit message.
---
 arch/mips/boot/dts/img/boston.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/img/boston.dts b/arch/mips/boot/dts/img/boston.dts
index 72f7605d2e31..f7a2c46f1eb8 100644
--- a/arch/mips/boot/dts/img/boston.dts
+++ b/arch/mips/boot/dts/img/boston.dts
@@ -203,7 +203,7 @@ cpc@16200000 {
 	};
 
 	plat_regs: system-controller@17ffd000 {
-		compatible = "img,boston-platform-regs", "syscon";
+		compatible = "img,boston-platform-regs", "syscon", "simple-mfd";
 		reg = <0x17ffd000 0x1000>;
 
 		clk_boston: clock {

-- 
2.43.0

[PATCH v3 2/8] MIPS: dts: Boston: Move syscon-reboot node under syscon

[Jiaxun Yang]

To match actual hardware topology.

Also rename node to "reboot".

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/boot/dts/img/boston.dts | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/mips/boot/dts/img/boston.dts b/arch/mips/boot/dts/img/boston.dts
index f7a2c46f1eb8..f465079d3085 100644
--- a/arch/mips/boot/dts/img/boston.dts
+++ b/arch/mips/boot/dts/img/boston.dts
@@ -210,13 +210,13 @@ clk_boston: clock {
 			compatible = "img,boston-clock";
 			#clock-cells = <1>;
 		};
-	};
 
-	reboot: syscon-reboot {
-		compatible = "syscon-reboot";
-		regmap = <&plat_regs>;
-		offset = <0x10>;
-		mask = <0x10>;
+		reboot: reboot {
+			compatible = "syscon-reboot";
+			regmap = <&plat_regs>;
+			offset = <0x10>;
+			mask = <0x10>;
+		};
 	};
 
 	uart0: uart@17ffe000 {

-- 
2.43.0

[PATCH v3 3/8] MIPS: dts: Boston: Rename clock node as clock-controller

[Jiaxun Yang]

Better naming.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
v3: New patch (Rob)
---
 arch/mips/boot/dts/img/boston.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/img/boston.dts b/arch/mips/boot/dts/img/boston.dts
index f465079d3085..337dac462e6a 100644
--- a/arch/mips/boot/dts/img/boston.dts
+++ b/arch/mips/boot/dts/img/boston.dts
@@ -206,7 +206,7 @@ plat_regs: system-controller@17ffd000 {
 		compatible = "img,boston-platform-regs", "syscon", "simple-mfd";
 		reg = <0x17ffd000 0x1000>;
 
-		clk_boston: clock {
+		clk_boston: clock-controller {
 			compatible = "img,boston-clock";
 			#clock-cells = <1>;
 		};

-- 
2.43.0

[PATCH v3 4/8] MIPS: dts: Boston: Remove unused #interrupt-cells

[Jiaxun Yang]

There is no reason to have #interrupt-cells node for a
pci device.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/boot/dts/img/boston.dts | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/mips/boot/dts/img/boston.dts b/arch/mips/boot/dts/img/boston.dts
index 337dac462e6a..3863a7bab03b 100644
--- a/arch/mips/boot/dts/img/boston.dts
+++ b/arch/mips/boot/dts/img/boston.dts
@@ -131,7 +131,6 @@ pci2_root@0,0 {
 
 			#address-cells = <3>;
 			#size-cells = <2>;
-			#interrupt-cells = <1>;
 
 			eg20t_bridge@1,0,0 {
 				compatible = "pci8086,8800";
@@ -139,7 +138,6 @@ eg20t_bridge@1,0,0 {
 
 				#address-cells = <3>;
 				#size-cells = <2>;
-				#interrupt-cells = <1>;
 
 				eg20t_phub@2,0,0 {
 					compatible = "pci8086,8801";

-- 
2.43.0

[PATCH v3 5/8] MIPS: dts: Boston: Add model property

[Jiaxun Yang]

Using string "IMG Boston" from bootrom.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/boot/dts/img/boston.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/boot/dts/img/boston.dts b/arch/mips/boot/dts/img/boston.dts
index 3863a7bab03b..5e825f34fcfa 100644
--- a/arch/mips/boot/dts/img/boston.dts
+++ b/arch/mips/boot/dts/img/boston.dts
@@ -10,6 +10,7 @@ / {
 	#address-cells = <1>;
 	#size-cells = <1>;
 	compatible = "img,boston";
+	model = "IMG Boston";
 
 	chosen {
 		stdout-path = "uart0:115200";

-- 
2.43.0

[PATCH v3 6/8] MIPS: dts: Boston: Rename uart node

[Jiaxun Yang]

Remove it as serial node.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 arch/mips/boot/dts/img/boston.dts | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/mips/boot/dts/img/boston.dts b/arch/mips/boot/dts/img/boston.dts
index 5e825f34fcfa..7d1870aae47f 100644
--- a/arch/mips/boot/dts/img/boston.dts
+++ b/arch/mips/boot/dts/img/boston.dts
@@ -13,11 +13,11 @@ / {
 	model = "IMG Boston";
 
 	chosen {
-		stdout-path = "uart0:115200";
+		stdout-path = "serial0:115200";
 	};
 
 	aliases {
-		uart0 = &uart0;
+		serial0 = &uart0;
 	};
 
 	cpus {
@@ -218,7 +218,7 @@ reboot: reboot {
 		};
 	};
 
-	uart0: uart@17ffe000 {
+	uart0: serial@17ffe000 {
 		compatible = "ns16550a";
 		reg = <0x17ffe000 0x1000>;
 		reg-shift = <2>;

-- 
2.43.0

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

[Jiaxun Yang]

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(-)

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>;
+      };
+    };

-- 
2.43.0

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

[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.

> +
> +  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

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

[Jiaxun Yang]

在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

-- 
- Jiaxun

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

[Krzysztof Kozlowski]

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

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

[Jiaxun Yang]

在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

-- 
- Jiaxun

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

[Krzysztof Kozlowski]

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

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

[Jiaxun Yang]

在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

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

[Jiaxun Yang]

在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 


-- 
- Jiaxun

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

[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.

> 
> 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

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

[Jiaxun Yang]

在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

-- 
- Jiaxun

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

[Rob Herring]

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

[PATCH v3 8/8] dt-bindings: mips: img: Add devices binding

[Jiaxun Yang]

Add devices binding for various Imagination Technologies
MIPS based Platforms.

Remove old documents.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
v3: Remove old bindings, they are not relavant any more.
    Booting protocol stuff is covered by spec.
---
 .../devicetree/bindings/mips/img/devices.yaml      | 33 +++++++++
 .../bindings/mips/img/pistachio-marduk.txt         | 10 ---
 .../devicetree/bindings/mips/img/pistachio.txt     | 42 -----------
 .../devicetree/bindings/mips/img/xilfpga.txt       | 83 ----------------------
 4 files changed, 33 insertions(+), 135 deletions(-)

diff --git a/Documentation/devicetree/bindings/mips/img/devices.yaml b/Documentation/devicetree/bindings/mips/img/devices.yaml
new file mode 100644
index 000000000000..460ca96577ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/mips/img/devices.yaml
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mips/img/devices.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Imagination Technologies MIPS based Platforms
+
+maintainers:
+  - Jiaxun Yang <jiaxun.yang@flygoat.com>
+
+properties:
+  $nodename:
+    const: '/'
+  compatible:
+    oneOf:
+
+      - description: IMG Boston
+        const: img,boston
+
+      - description: IMG Marduk (Creator Ci40)
+        items:
+          - const: img,pistachio-marduk
+          - const: img,pistachio
+
+      - description: Imagination University Program MIPSfpga
+        items:
+          - const: img,xilfpga
+          - const: digilent,nexys4ddr
+
+additionalProperties: true
+
+...
diff --git a/Documentation/devicetree/bindings/mips/img/pistachio-marduk.txt b/Documentation/devicetree/bindings/mips/img/pistachio-marduk.txt
deleted file mode 100644
index 2d5126d529a2..000000000000
--- a/Documentation/devicetree/bindings/mips/img/pistachio-marduk.txt
+++ /dev/null
@@ -1,10 +0,0 @@
-Imagination Technologies' Pistachio SoC based Marduk Board
-==========================================================
-
-Compatible string must be "img,pistachio-marduk", "img,pistachio"
-
-Hardware and other related documentation is available at
-https://docs.creatordev.io/ci40/
-
-It is also known as Creator Ci40. Marduk is legacy name and will
-be there for decades.
diff --git a/Documentation/devicetree/bindings/mips/img/pistachio.txt b/Documentation/devicetree/bindings/mips/img/pistachio.txt
deleted file mode 100644
index a736d889c2b8..000000000000
--- a/Documentation/devicetree/bindings/mips/img/pistachio.txt
+++ /dev/null
@@ -1,42 +0,0 @@
-Imagination Pistachio SoC
-=========================
-
-Required properties:
---------------------
- - compatible: Must include "img,pistachio".
-
-CPU nodes:
-----------
-A "cpus" node is required.  Required properties:
- - #address-cells: Must be 1.
- - #size-cells: Must be 0.
-A CPU sub-node is also required for at least CPU 0.  Since the topology may
-be probed via CPS, it is not necessary to specify secondary CPUs.  Required
-propertis:
- - device_type: Must be "cpu".
- - compatible: Must be "mti,interaptiv".
- - reg: CPU number.
- - clocks: Must include the CPU clock.  See ../../clock/clock-bindings.txt for
-   details on clock bindings.
-Example:
-	cpus {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		cpu0: cpu@0 {
-			device_type = "cpu";
-			compatible = "mti,interaptiv";
-			reg = <0>;
-			clocks = <&clk_core CLK_MIPS>;
-		};
-	};
-
-
-Boot protocol:
---------------
-In accordance with the MIPS UHI specification[1], the bootloader must pass the
-following arguments to the kernel:
- - $a0: -2.
- - $a1: KSEG0 address of the flattened device-tree blob.
-
-[1] http://prplfoundation.org/wiki/MIPS_documentation
diff --git a/Documentation/devicetree/bindings/mips/img/xilfpga.txt b/Documentation/devicetree/bindings/mips/img/xilfpga.txt
deleted file mode 100644
index 57e7ee942166..000000000000
--- a/Documentation/devicetree/bindings/mips/img/xilfpga.txt
+++ /dev/null
@@ -1,83 +0,0 @@
-Imagination University Program MIPSfpga
-=======================================
-
-Under the Imagination University Program, a microAptiv UP core has been
-released for academic usage.
-
-As we are dealing with a MIPS core instantiated on an FPGA, specifications
-are fluid and can be varied in RTL.
-
-This binding document is provided as baseline guidance for the example
-project provided by IMG.
-
-The example project runs on the Nexys4DDR board by Digilent powered by
-the ARTIX-7 FPGA by Xilinx.
-
-Relevant details about the example project and the Nexys4DDR board:
-
-- microAptiv UP core m14Kc
-- 50MHz clock speed
-- 128Mbyte DDR RAM	at 0x0000_0000
-- 8Kbyte RAM		at 0x1000_0000
-- axi_intc		at 0x1020_0000
-- axi_uart16550		at 0x1040_0000
-- axi_gpio		at 0x1060_0000
-- axi_i2c		at 0x10A0_0000
-- custom_gpio		at 0x10C0_0000
-- axi_ethernetlite	at 0x10E0_0000
-- 8Kbyte BootRAM	at 0x1FC0_0000
-
-Required properties:
---------------------
- - compatible: Must include "digilent,nexys4ddr","img,xilfpga".
-
-CPU nodes:
-----------
-A "cpus" node is required.  Required properties:
- - #address-cells: Must be 1.
- - #size-cells: Must be 0.
-A CPU sub-node is also required for at least CPU 0. Required properties:
- - device_type: Must be "cpu".
- - compatible: Must be "mips,m14Kc".
- - reg: Must be <0>.
- - clocks: phandle to ext clock for fixed-clock received by MIPS core.
-
-Example:
-
-	compatible = "img,xilfpga","digilent,nexys4ddr";
-	cpus {
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		cpu0: cpu@0 {
-			device_type = "cpu";
-			compatible = "mips,m14Kc";
-			reg = <0>;
-			clocks	= <&ext>;
-		};
-	};
-
-	ext: ext {
-		compatible = "fixed-clock";
-		#clock-cells = <0>;
-		clock-frequency = <50000000>;
-	};
-
-Boot protocol:
---------------
-
-The BootRAM is a writeable "RAM" in FPGA at 0x1FC0_0000.
-This is for easy reprogrammibility via JTAG.
-
-The BootRAM initializes the cache and the axi_uart peripheral.
-
-DDR initialization is already handled by a HW IP block.
-
-When the example project bitstream is loaded, the cpu_reset button
-needs to be pressed.
-
-The bootram initializes the cache and axi_uart.
-Then outputs MIPSFPGA\n\r on the serial port on the Nexys4DDR board.
-
-At this point, the board is ready to load the Linux kernel
-vmlinux file via JTAG.

-- 
2.43.0

Re: [PATCH v3 8/8] dt-bindings: mips: img: Add devices binding

[Krzysztof Kozlowski]

On 18/06/2024 17:11, Jiaxun Yang wrote:
> Add devices binding for various Imagination Technologies
> MIPS based Platforms.
> 
> Remove old documents.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> v3: Remove old bindings, they are not relavant any more.
>     Booting protocol stuff is covered by spec.
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof