| Message ID | 20220628124441.2385023-4-martin.blumenstingl@googlemail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | reset: replace reset-lantiq with reset-intel-gw | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Tue, Jun 28, 2022 at 02:44:35PM +0200, Martin Blumenstingl wrote: > The Lantiq Amazon-SE, Danube, xRX100 and xRX200 SoCs have up to two USB2 > PHYs which are part of the RCU register space. The RCU registers on > these SoCs are using big endian. Update the binding for these SoCs to > properly describe this IP: > - Add compatible strings for Amazon-SE, Danube and xRX100 > - Rename the xRX200 compatible string (which is not used anywhere) and > switch to the one previously documented in mips/lantiq/rcu.txt > - Allow usage of "simple-mfd" and "syscon" in the compatible string so the > child devices (USB2 PHYs) can be described > - Allow #address-cells and #size-cells to be set to 1 for describing the > child devices (USB2 PHYs) > - #reset-cells must always be 3 (offset, reset bit and status bit) on the > legacy SoCs while LGM uses a fixed value of 2 (offset and reset bit - > status bit is always identical to the reset bit). > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > .../bindings/reset/intel,rcu-gw.yaml | 84 +++++++++++++++++-- > 1 file changed, 79 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml b/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml > index be64f8597710..b90913c7b7d3 100644 > --- a/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml > +++ b/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml > @@ -11,9 +11,16 @@ maintainers: > > properties: > compatible: > - enum: > - - intel,rcu-lgm > - - intel,rcu-xrx200 It is okay to remove/change this because ? > + oneOf: > + - items: > + - enum: > + - lantiq,ase-rcu > + - lantiq,danube-rcu > + - lantiq,xrx100-rcu > + - lantiq,xrx200-rcu > + - const: simple-mfd This says child nodes have 0 dependence on anything in the parent node. Such as a clock in the parent needing to be enabled. > + - const: syscon Given the child nodes depend on this, I find the combination to be a contradiction. But it's widely used, so oh well. > + - const: intel,rcu-lgm > > reg: > description: Reset controller registers. > @@ -33,8 +40,6 @@ properties: > maximum: 31 > > "#reset-cells": > - minimum: 2 > - maximum: 3 > description: | > First cell is reset request register offset. > Second cell is bit offset in reset request register. > @@ -43,6 +48,43 @@ properties: > reset request and reset status registers is same. Whereas > 3 for legacy SoCs as bit offset differs. > > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 1 > + > + big-endian: true > + > +patternProperties: > + "^usb2-phy@[0-9a-f]+$": > + type: object > + $ref: "../phy/lantiq,xway-rcu-usb2-phy.yaml" > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: intel,rcu-lgm > + then: > + properties: > + "#reset-cells": > + const: 2 else: properties: "#reset-cells": const: 3 > + - if: > + properties: > + compatible: > + contains: > + enum: > + - lantiq,ase-rcu > + - lantiq,danube-rcu > + - lantiq,xrx100-rcu > + - lantiq,xrx200-rcu > + then: > + properties: > + "#reset-cells": > + const: 3 > + > required: > - compatible > - reg > @@ -67,3 +109,35 @@ examples: > #pwm-cells = <2>; > resets = <&rcu0 0x30 21>; > }; > + - | > + rcu_xrx200: rcu@203000 { > + compatible = "lantiq,xrx200-rcu", "simple-mfd", "syscon"; > + reg = <0x203000 0x100>; > + big-endian; > + > + #address-cells = <1>; > + #size-cells = <1>; > + > + #reset-cells = <3>; > + intel,global-reset = <0x10 30 29>; > + > + usb_phy0: usb2-phy@18 { > + compatible = "lantiq,xrx200-usb2-phy"; > + reg = <0x18 4>, <0x38 4>; > + status = "disabled"; Why is your example disabled? Don't use 'status' in examples. > + > + resets = <&rcu_xrx200 0x48 4 4>, <&rcu_xrx200 0x10 4 4>; Humm, a dependency on the parent. Not a 'simple-mfd'. > + reset-names = "phy", "ctrl"; > + #phy-cells = <0>; > + }; > + > + usb_phy1: usb2-phy@34 { > + compatible = "lantiq,xrx200-usb2-phy"; > + reg = <0x34 4>, <0x3c 4>; > + status = "disabled"; > + > + resets = <&rcu_xrx200 0x48 5 5>, <&rcu_xrx200 0x10 4 4>; > + reset-names = "phy", "ctrl"; > + #phy-cells = <0>; > + }; > + }; > -- > 2.36.1 > >
Hi Rob, On Fri, Jul 1, 2022 at 6:33 PM Rob Herring <robh@kernel.org> wrote: > > On Tue, Jun 28, 2022 at 02:44:35PM +0200, Martin Blumenstingl wrote: > > The Lantiq Amazon-SE, Danube, xRX100 and xRX200 SoCs have up to two USB2 > > PHYs which are part of the RCU register space. The RCU registers on > > these SoCs are using big endian. Update the binding for these SoCs to > > properly describe this IP: > > - Add compatible strings for Amazon-SE, Danube and xRX100 > > - Rename the xRX200 compatible string (which is not used anywhere) and > > switch to the one previously documented in mips/lantiq/rcu.txt > > - Allow usage of "simple-mfd" and "syscon" in the compatible string so the > > child devices (USB2 PHYs) can be described > > - Allow #address-cells and #size-cells to be set to 1 for describing the > > child devices (USB2 PHYs) > > - #reset-cells must always be 3 (offset, reset bit and status bit) on the > > legacy SoCs while LGM uses a fixed value of 2 (offset and reset bit - > > status bit is always identical to the reset bit). > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > --- > > .../bindings/reset/intel,rcu-gw.yaml | 84 +++++++++++++++++-- > > 1 file changed, 79 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml b/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml > > index be64f8597710..b90913c7b7d3 100644 > > --- a/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml > > +++ b/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml > > @@ -11,9 +11,16 @@ maintainers: > > > > properties: > > compatible: > > - enum: > > - - intel,rcu-lgm > > - - intel,rcu-xrx200 > > It is okay to remove/change this because ? I'll update the description in v2. The "intel,rcu-xrx200" compatible string isn't used anywhere (upstream or downstream in OpenWrt). u-boot on Lantiq xRX200 SoCs is too old to pass a dtb to the kernel, so we're appending the DTB to the kernel image. > > + oneOf: > > + - items: > > + - enum: > > + - lantiq,ase-rcu > > + - lantiq,danube-rcu > > + - lantiq,xrx100-rcu > > + - lantiq,xrx200-rcu > > + - const: simple-mfd > > This says child nodes have 0 dependence on anything in the parent node. > Such as a clock in the parent needing to be enabled. > > > + - const: syscon > > Given the child nodes depend on this, I find the combination to be a > contradiction. But it's widely used, so oh well. I can think of two ways to solve this: 1) remove the simple-mfd compatible string and make the driver also discover child nodes 2) remove the simple-mfd compatible string and remove the USB PHY child nodes - then add add #phy-cells = <1> to the RCU node itself (and somehow update the RCU and USB PHY drivers accordingly) 3) introduce a separate child node for the reset-controller, then the child nodes depend on each other (but there's no strict dependency on the parent anymore other than the fact that the parent needs a "syscon" compatible string). My understanding of this IP block is that it was initially designed as a reset controller, hence its name "reset controller unit" (RCU). Then additional logic was added after the fact. So I think 1) (dropping the simple-mfd compatible string) or 2) (dropping the simple-mfd compatible string and the child nodes altogether) is the right way to go here. Which route would you go and why? [...] > > +patternProperties: > > + "^usb2-phy@[0-9a-f]+$": > > + type: object > > + $ref: "../phy/lantiq,xway-rcu-usb2-phy.yaml" > > + > > +allOf: > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: intel,rcu-lgm > > + then: > > + properties: > > + "#reset-cells": > > + const: 2 > > else: > properties: > "#reset-cells": > const: 3 much shorter, thanks - I'll take care of this in v2. [...] > > + usb_phy0: usb2-phy@18 { > > + compatible = "lantiq,xrx200-usb2-phy"; > > + reg = <0x18 4>, <0x38 4>; > > + status = "disabled"; > > Why is your example disabled? Don't use 'status' in examples. I should know this better - I'll fix this in v2. Best regards, Martin
On Sun, Jul 03, 2022 at 01:04:20AM +0200, Martin Blumenstingl wrote: > Hi Rob, > > On Fri, Jul 1, 2022 at 6:33 PM Rob Herring <robh@kernel.org> wrote: > > > > On Tue, Jun 28, 2022 at 02:44:35PM +0200, Martin Blumenstingl wrote: > > > The Lantiq Amazon-SE, Danube, xRX100 and xRX200 SoCs have up to two USB2 > > > PHYs which are part of the RCU register space. The RCU registers on > > > these SoCs are using big endian. Update the binding for these SoCs to > > > properly describe this IP: > > > - Add compatible strings for Amazon-SE, Danube and xRX100 > > > - Rename the xRX200 compatible string (which is not used anywhere) and > > > switch to the one previously documented in mips/lantiq/rcu.txt > > > - Allow usage of "simple-mfd" and "syscon" in the compatible string so the > > > child devices (USB2 PHYs) can be described > > > - Allow #address-cells and #size-cells to be set to 1 for describing the > > > child devices (USB2 PHYs) > > > - #reset-cells must always be 3 (offset, reset bit and status bit) on the > > > legacy SoCs while LGM uses a fixed value of 2 (offset and reset bit - > > > status bit is always identical to the reset bit). > > > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > --- > > > .../bindings/reset/intel,rcu-gw.yaml | 84 +++++++++++++++++-- > > > 1 file changed, 79 insertions(+), 5 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml b/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml > > > index be64f8597710..b90913c7b7d3 100644 > > > --- a/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml > > > +++ b/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml > > > @@ -11,9 +11,16 @@ maintainers: > > > > > > properties: > > > compatible: > > > - enum: > > > - - intel,rcu-lgm > > > - - intel,rcu-xrx200 > > > > It is okay to remove/change this because ? > I'll update the description in v2. The "intel,rcu-xrx200" compatible > string isn't used anywhere (upstream or downstream in OpenWrt). > u-boot on Lantiq xRX200 SoCs is too old to pass a dtb to the kernel, > so we're appending the DTB to the kernel image. > > > > + oneOf: > > > + - items: > > > + - enum: > > > + - lantiq,ase-rcu > > > + - lantiq,danube-rcu > > > + - lantiq,xrx100-rcu > > > + - lantiq,xrx200-rcu > > > + - const: simple-mfd > > > > This says child nodes have 0 dependence on anything in the parent node. > > Such as a clock in the parent needing to be enabled. > > > > > + - const: syscon > > > > Given the child nodes depend on this, I find the combination to be a > > contradiction. But it's widely used, so oh well. > I can think of two ways to solve this: > 1) remove the simple-mfd compatible string and make the driver also > discover child nodes > 2) remove the simple-mfd compatible string and remove the USB PHY > child nodes - then add add #phy-cells = <1> to the RCU node itself > (and somehow update the RCU and USB PHY drivers accordingly) > 3) introduce a separate child node for the reset-controller, then the > child nodes depend on each other (but there's no strict dependency on > the parent anymore other than the fact that the parent needs a > "syscon" compatible string). > > My understanding of this IP block is that it was initially designed as > a reset controller, hence its name "reset controller unit" (RCU). Then > additional logic was added after the fact. > So I think 1) (dropping the simple-mfd compatible string) or 2) > (dropping the simple-mfd compatible string and the child nodes > altogether) is the right way to go here. Which route would you go and > why? 2 would be my choice. That's the simplest binding. Unless the phy registers show up in different places on multiple devices, then maybe it's worth keeping the child node. Rob
diff --git a/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml b/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml index be64f8597710..b90913c7b7d3 100644 --- a/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml +++ b/Documentation/devicetree/bindings/reset/intel,rcu-gw.yaml @@ -11,9 +11,16 @@ maintainers: properties: compatible: - enum: - - intel,rcu-lgm - - intel,rcu-xrx200 + oneOf: + - items: + - enum: + - lantiq,ase-rcu + - lantiq,danube-rcu + - lantiq,xrx100-rcu + - lantiq,xrx200-rcu + - const: simple-mfd + - const: syscon + - const: intel,rcu-lgm reg: description: Reset controller registers. @@ -33,8 +40,6 @@ properties: maximum: 31 "#reset-cells": - minimum: 2 - maximum: 3 description: | First cell is reset request register offset. Second cell is bit offset in reset request register. @@ -43,6 +48,43 @@ properties: reset request and reset status registers is same. Whereas 3 for legacy SoCs as bit offset differs. + "#address-cells": + const: 1 + + "#size-cells": + const: 1 + + big-endian: true + +patternProperties: + "^usb2-phy@[0-9a-f]+$": + type: object + $ref: "../phy/lantiq,xway-rcu-usb2-phy.yaml" + +allOf: + - if: + properties: + compatible: + contains: + const: intel,rcu-lgm + then: + properties: + "#reset-cells": + const: 2 + - if: + properties: + compatible: + contains: + enum: + - lantiq,ase-rcu + - lantiq,danube-rcu + - lantiq,xrx100-rcu + - lantiq,xrx200-rcu + then: + properties: + "#reset-cells": + const: 3 + required: - compatible - reg @@ -67,3 +109,35 @@ examples: #pwm-cells = <2>; resets = <&rcu0 0x30 21>; }; + - | + rcu_xrx200: rcu@203000 { + compatible = "lantiq,xrx200-rcu", "simple-mfd", "syscon"; + reg = <0x203000 0x100>; + big-endian; + + #address-cells = <1>; + #size-cells = <1>; + + #reset-cells = <3>; + intel,global-reset = <0x10 30 29>; + + usb_phy0: usb2-phy@18 { + compatible = "lantiq,xrx200-usb2-phy"; + reg = <0x18 4>, <0x38 4>; + status = "disabled"; + + resets = <&rcu_xrx200 0x48 4 4>, <&rcu_xrx200 0x10 4 4>; + reset-names = "phy", "ctrl"; + #phy-cells = <0>; + }; + + usb_phy1: usb2-phy@34 { + compatible = "lantiq,xrx200-usb2-phy"; + reg = <0x34 4>, <0x3c 4>; + status = "disabled"; + + resets = <&rcu_xrx200 0x48 5 5>, <&rcu_xrx200 0x10 4 4>; + reset-names = "phy", "ctrl"; + #phy-cells = <0>; + }; + };
The Lantiq Amazon-SE, Danube, xRX100 and xRX200 SoCs have up to two USB2 PHYs which are part of the RCU register space. The RCU registers on these SoCs are using big endian. Update the binding for these SoCs to properly describe this IP: - Add compatible strings for Amazon-SE, Danube and xRX100 - Rename the xRX200 compatible string (which is not used anywhere) and switch to the one previously documented in mips/lantiq/rcu.txt - Allow usage of "simple-mfd" and "syscon" in the compatible string so the child devices (USB2 PHYs) can be described - Allow #address-cells and #size-cells to be set to 1 for describing the child devices (USB2 PHYs) - #reset-cells must always be 3 (offset, reset bit and status bit) on the legacy SoCs while LGM uses a fixed value of 2 (offset and reset bit - status bit is always identical to the reset bit). Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- .../bindings/reset/intel,rcu-gw.yaml | 84 +++++++++++++++++-- 1 file changed, 79 insertions(+), 5 deletions(-)