| Message ID | 20250309132959.19045-6-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | airoha: en7581: clk cleanup + USB support | expand |
On Sun, 09 Mar 2025 14:29:36 +0100, Christian Marangi wrote: > Add Documentation for Airoha EN7581 SCU. > > Airoha EN7581 SoC expose registers to control miscellaneous pheriperals > via the SCU (System Controller Unit). > > Example of these pheriperals are reset-controller, clock-controller, > PCIe line speed controller and bits to configure different Serdes ports > for USB or Ethernet usage. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > .../mfd/airoha,en7581-scu-sysctl.yaml | 68 +++++++++++++++++++ > 1 file changed, 68 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.example.dtb: system-controller@1fb00000: clock-controller: 'reg' is a required property from schema $id: http://devicetree.org/schemas/mfd/airoha,en7581-scu-sysctl.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.example.dtb: clock-controller: 'reg' is a required property from schema $id: http://devicetree.org/schemas/clock/airoha,en7523-scu.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250309132959.19045-6-ansuelsmth@gmail.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Sun, Mar 09, 2025 at 02:29:36PM +0100, Christian Marangi wrote: > + > +examples: > + - | > + #include <dt-bindings/soc/airoha,scu-ssr.h> > + > + system-controller@1fb00000 { > + compatible = "airoha,en7581-scu-sysctl", "syscon", "simple-mfd"; > + reg = <0x1fb00000 0x970>; > + > + clock-controller { > + compatible = "airoha,en7581-scu"; > + No resources here, so this is part of the parent. > + #clock-cells = <1>; > + #reset-cells = <1>; > + }; > + > + system-controller { > + compatible = "airoha,an7581-scu-ssr"; No, this was told many times - you do not have resources here, so no chhild. Best regards, Krzysztof
On 09/03/2025 14:29, Christian Marangi wrote: > Add Documentation for Airoha EN7581 SCU. > > Airoha EN7581 SoC expose registers to control miscellaneous pheriperals > via the SCU (System Controller Unit). > > Example of these pheriperals are reset-controller, clock-controller, > PCIe line speed controller and bits to configure different Serdes ports > for USB or Ethernet usage. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > .../mfd/airoha,en7581-scu-sysctl.yaml | 68 +++++++++++++++++++ > 1 file changed, 68 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml > > diff --git a/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml > new file mode 100644 > index 000000000000..d7dc66f912c1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml > @@ -0,0 +1,68 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/airoha,en7581-scu-sysctl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Airoha EN7581 SCU (System Controller Unit) > + > +maintainers: > + - Christian Marangi <ansuelsmth@gmail.com> > + > +description: > + Airoha EN7581 SoC expose registers to control miscellaneous > + pheriperals via the SCU (System Controller Unit). > + One more comment - there is no such thing as "sysctl" in your hardware. Look at the SCU binding which clearly says that it is the hardware you are duplicating here, so the "System Control Unit". So you have existing "This node defines the System Control Unit of the EN7523 SoC" and you add one more node which defines the "System Control Unit", so you have two "System Control Unit" device nodes? Look also what Stephen asked for: https://lore.kernel.org/all/20220106013100.842FCC36AEB@smtp.kernel.org/ so how system-controller can now became clock-controller? Now, it was the system controller since the beginning. Best regards, Krzysztof
On Mon, Mar 10, 2025 at 10:21:45AM +0100, Krzysztof Kozlowski wrote: > On 09/03/2025 14:29, Christian Marangi wrote: > > Add Documentation for Airoha EN7581 SCU. > > > > Airoha EN7581 SoC expose registers to control miscellaneous pheriperals > > via the SCU (System Controller Unit). > > > > Example of these pheriperals are reset-controller, clock-controller, > > PCIe line speed controller and bits to configure different Serdes ports > > for USB or Ethernet usage. > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > .../mfd/airoha,en7581-scu-sysctl.yaml | 68 +++++++++++++++++++ > > 1 file changed, 68 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml > > > > diff --git a/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml > > new file mode 100644 > > index 000000000000..d7dc66f912c1 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml > > @@ -0,0 +1,68 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mfd/airoha,en7581-scu-sysctl.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Airoha EN7581 SCU (System Controller Unit) > > + > > +maintainers: > > + - Christian Marangi <ansuelsmth@gmail.com> > > + > > +description: > > + Airoha EN7581 SoC expose registers to control miscellaneous > > + pheriperals via the SCU (System Controller Unit). > > + > One more comment - there is no such thing as "sysctl" in your hardware. > Look at the SCU binding which clearly says that it is the hardware you > are duplicating here, so the "System Control Unit". > > So you have existing "This node defines the System Control Unit of the > EN7523 SoC" and you add one more node which defines the "System Control > Unit", so you have two "System Control Unit" device nodes? > > Look also what Stephen asked for: > > https://lore.kernel.org/all/20220106013100.842FCC36AEB@smtp.kernel.org/ > > so how system-controller can now became clock-controller? Now, it was > the system controller since the beginning. > The main problem here (and we had a similar problem with GPIO and PWM) is that the Vendor (Airoha) wasn't so bright in placing the different registers for the SoC so we have case where everything is mixed and not one after another... Example we have - CLK register part 1 - Some bits that configure PCIe - CLK register part 2 - GPIO - CLK register part 3 - ... The driver solution for this is syscon and the simple-mfd node structure. Now the main problem is how to modle this in DT. There are lots of case where the simple-mfd model is used (like the one proposed) but probably this is not accepted anymore. But again this should be clearly stated or we have a chicken-egg problem when other devs implement similar thing and have to implement simple MFD driver to handle this. (and driver maintainers say "Use the simple-mfd model like it was already done) For this specific case (and to give an answer to the clock patch after this) the problem is that this register space was originally used only to control the clock and I wasn't aware that it was also used to control USB. Now that I'm implementing support for it, the disaster happened. So In short SCU is lots of thing, both a system-controller, a clock-controller and even a reset-controller. To make it short, 2 different solution: 1. We can keep the current node structure of the node-controller and add a child node for the SSR part (with a dedicated compatible). 2. Those property need to be be defined in the clock-controller node? The ideal solution is 1. Does it work for you? Sorry for the long post and hope you understand why this mess of reworking the binding.
On 10/03/2025 11:47, Christian Marangi wrote: > On Mon, Mar 10, 2025 at 10:21:45AM +0100, Krzysztof Kozlowski wrote: >> On 09/03/2025 14:29, Christian Marangi wrote: >>> Add Documentation for Airoha EN7581 SCU. >>> >>> Airoha EN7581 SoC expose registers to control miscellaneous pheriperals >>> via the SCU (System Controller Unit). >>> >>> Example of these pheriperals are reset-controller, clock-controller, >>> PCIe line speed controller and bits to configure different Serdes ports >>> for USB or Ethernet usage. >>> >>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> >>> --- >>> .../mfd/airoha,en7581-scu-sysctl.yaml | 68 +++++++++++++++++++ >>> 1 file changed, 68 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml >>> new file mode 100644 >>> index 000000000000..d7dc66f912c1 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml >>> @@ -0,0 +1,68 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/mfd/airoha,en7581-scu-sysctl.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Airoha EN7581 SCU (System Controller Unit) >>> + >>> +maintainers: >>> + - Christian Marangi <ansuelsmth@gmail.com> >>> + >>> +description: >>> + Airoha EN7581 SoC expose registers to control miscellaneous >>> + pheriperals via the SCU (System Controller Unit). >>> + >> One more comment - there is no such thing as "sysctl" in your hardware. >> Look at the SCU binding which clearly says that it is the hardware you >> are duplicating here, so the "System Control Unit". >> >> So you have existing "This node defines the System Control Unit of the >> EN7523 SoC" and you add one more node which defines the "System Control >> Unit", so you have two "System Control Unit" device nodes? >> >> Look also what Stephen asked for: >> >> https://lore.kernel.org/all/20220106013100.842FCC36AEB@smtp.kernel.org/ >> >> so how system-controller can now became clock-controller? Now, it was >> the system controller since the beginning. >> > > The main problem here (and we had a similar problem with GPIO and PWM) > is that the Vendor (Airoha) wasn't so bright in placing the different > registers for the SoC so we have case where everything is mixed and not > one after another... > > Example we have > - CLK register part 1 > - Some bits that configure PCIe > - CLK register part 2 > - GPIO > - CLK register part 3 > - ... This does not explain that binding said "This node defines the System Control Unit". So what are you adding here if not SCU? > > The driver solution for this is syscon and the simple-mfd node > structure. Let's keep driver entirely separate, we don't talk about them and mixing arguments won't make it easier. > > Now the main problem is how to modle this in DT. There are lots of case > where the simple-mfd model is used (like the one proposed) but probably > this is not accepted anymore. But again this should be clearly stated or > we have a chicken-egg problem when other devs implement similar thing and > have to implement simple MFD driver to handle this. (and driver > maintainers say "Use the simple-mfd model like it was already done) simple-mfd has nothing to do here. Describe the hardware - what is the SCU? > > For this specific case (and to give an answer to the clock patch after > this) the problem is that this register space was originally used only > to control the clock and I wasn't aware that it was also used to control > USB. Now that I'm implementing support for it, the disaster happened. > > So In short SCU is lots of thing, both a system-controller, a > clock-controller and even a reset-controller. And you have bindings for that already. Done. > > To make it short, 2 different solution: > 1. We can keep the current node structure of the node-controller and add a > child node for the SSR part (with a dedicated compatible). No, you do not add child nodes just because you want some drivers. What is SSR? How is it a device? > 2. Those property need to be be defined in the clock-controller node? In the SCU node. Do you have only one SCU or more? > > The ideal solution is 1. Does it work for you? > > Sorry for the long post and hope you understand why this mess of > reworking the binding. > Best regards, Krzysztof
On Mon, Mar 10, 2025 at 12:41:06PM +0100, Krzysztof Kozlowski wrote: > On 10/03/2025 11:47, Christian Marangi wrote: > > On Mon, Mar 10, 2025 at 10:21:45AM +0100, Krzysztof Kozlowski wrote: > >> On 09/03/2025 14:29, Christian Marangi wrote: > >>> Add Documentation for Airoha EN7581 SCU. > >>> > >>> Airoha EN7581 SoC expose registers to control miscellaneous pheriperals > >>> via the SCU (System Controller Unit). > >>> > >>> Example of these pheriperals are reset-controller, clock-controller, > >>> PCIe line speed controller and bits to configure different Serdes ports > >>> for USB or Ethernet usage. > >>> > >>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > >>> --- > >>> .../mfd/airoha,en7581-scu-sysctl.yaml | 68 +++++++++++++++++++ > >>> 1 file changed, 68 insertions(+) > >>> create mode 100644 Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml > >>> > >>> diff --git a/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml > >>> new file mode 100644 > >>> index 000000000000..d7dc66f912c1 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml > >>> @@ -0,0 +1,68 @@ > >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > >>> +%YAML 1.2 > >>> +--- > >>> +$id: http://devicetree.org/schemas/mfd/airoha,en7581-scu-sysctl.yaml# > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>> + > >>> +title: Airoha EN7581 SCU (System Controller Unit) > >>> + > >>> +maintainers: > >>> + - Christian Marangi <ansuelsmth@gmail.com> > >>> + > >>> +description: > >>> + Airoha EN7581 SoC expose registers to control miscellaneous > >>> + pheriperals via the SCU (System Controller Unit). > >>> + > >> One more comment - there is no such thing as "sysctl" in your hardware. > >> Look at the SCU binding which clearly says that it is the hardware you > >> are duplicating here, so the "System Control Unit". > >> > >> So you have existing "This node defines the System Control Unit of the > >> EN7523 SoC" and you add one more node which defines the "System Control > >> Unit", so you have two "System Control Unit" device nodes? > >> > >> Look also what Stephen asked for: > >> > >> https://lore.kernel.org/all/20220106013100.842FCC36AEB@smtp.kernel.org/ > >> > >> so how system-controller can now became clock-controller? Now, it was > >> the system controller since the beginning. > >> > > > > The main problem here (and we had a similar problem with GPIO and PWM) > > is that the Vendor (Airoha) wasn't so bright in placing the different > > registers for the SoC so we have case where everything is mixed and not > > one after another... > > > > Example we have > > - CLK register part 1 > > - Some bits that configure PCIe > > - CLK register part 2 > > - GPIO > > - CLK register part 3 > > - ... > > This does not explain that binding said "This node defines the System > Control Unit". > > So what are you adding here if not SCU? > With "This node defines the System Control Unit" I mean, the entire register space of the IP block is defined and each child specifically define each part of the IP block. > > > > The driver solution for this is syscon and the simple-mfd node > > structure. > > Let's keep driver entirely separate, we don't talk about them and mixing > arguments won't make it easier. > Ok. > > > > Now the main problem is how to modle this in DT. There are lots of case > > where the simple-mfd model is used (like the one proposed) but probably > > this is not accepted anymore. But again this should be clearly stated or > > we have a chicken-egg problem when other devs implement similar thing and > > have to implement simple MFD driver to handle this. (and driver > > maintainers say "Use the simple-mfd model like it was already done) > > simple-mfd has nothing to do here. Describe the hardware - what is the SCU? > > As I said below, SCU is just the name used in Airoha Documentation for this IP block. In this register space there are multiple things so it's not strictly a clock-controller (as it's currently defined) It was proposed as clock-controller previously as we weren't aware this IP block was used also for other usage that a strict clock controller. > > > > For this specific case (and to give an answer to the clock patch after > > this) the problem is that this register space was originally used only > > to control the clock and I wasn't aware that it was also used to control > > USB. Now that I'm implementing support for it, the disaster happened. > > > > So In short SCU is lots of thing, both a system-controller, a > > clock-controller and even a reset-controller. > > And you have bindings for that already. Done. > It's currently defined in DTS as clock-controller, should we change it to system-controller to make it more clear? > > > > To make it short, 2 different solution: > > 1. We can keep the current node structure of the node-controller and add a > > child node for the SSR part (with a dedicated compatible). > > No, you do not add child nodes just because you want some drivers. > > What is SSR? How is it a device? SSR is the name used in Documentation for the register used to configure the Serdes and PCIe port. > > > 2. Those property need to be be defined in the clock-controller node? > > In the SCU node. Do you have only one SCU or more? Strictly speaking it's one register space. One clock-controller, one reset-controller and one set of SSR registers, and from what I can understand it's ALWAYS "One device/compatible for Register space" The simple-mfd pattern can't really work for case like this where in one register space there are multiple stuff. Is everything clear now? To summarize: - no child nodes - add additional property for SSR in the SCU .yaml > > > > > The ideal solution is 1. Does it work for you? > > > > Sorry for the long post and hope you understand why this mess of > > reworking the binding. > > > > > Best regards, > Krzysztof
diff --git a/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml new file mode 100644 index 000000000000..d7dc66f912c1 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml @@ -0,0 +1,68 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/airoha,en7581-scu-sysctl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Airoha EN7581 SCU (System Controller Unit) + +maintainers: + - Christian Marangi <ansuelsmth@gmail.com> + +description: + Airoha EN7581 SoC expose registers to control miscellaneous + pheriperals via the SCU (System Controller Unit). + + Example of these pheriperals are reset-controller, clock-controller, + PCIe line speed controller and bits to configure different Serdes ports + for USB or Ethernet usage. + +properties: + compatible: + items: + - const: airoha,en7581-scu-sysctl + - const: syscon + - const: simple-mfd + + reg: + maxItems: 1 + + clock-controller: + type: object + $ref: /schemas/clock/airoha,en7523-scu.yaml + description: + Child node definition for EN7581 Clock controller + + system-controller: + type: object + $ref: /schemas/soc/airoha/airoha,an7581-scu-ssr.yaml + description: + Child node definition for EN7581 System Status Register + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + #include <dt-bindings/soc/airoha,scu-ssr.h> + + system-controller@1fb00000 { + compatible = "airoha,en7581-scu-sysctl", "syscon", "simple-mfd"; + reg = <0x1fb00000 0x970>; + + clock-controller { + compatible = "airoha,en7581-scu"; + + #clock-cells = <1>; + #reset-cells = <1>; + }; + + system-controller { + compatible = "airoha,an7581-scu-ssr"; + + airoha,serdes-usb2 = <AIROHA_SCU_SSR_USB2_PCIE2>; + }; + };
Add Documentation for Airoha EN7581 SCU. Airoha EN7581 SoC expose registers to control miscellaneous pheriperals via the SCU (System Controller Unit). Example of these pheriperals are reset-controller, clock-controller, PCIe line speed controller and bits to configure different Serdes ports for USB or Ethernet usage. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- .../mfd/airoha,en7581-scu-sysctl.yaml | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/airoha,en7581-scu-sysctl.yaml