| 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 |
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
在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
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
在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
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
在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
在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
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
在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
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 --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>; + }; + };
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(-)