Message ID | 20221219191038.1973807-2-robh@kernel.org |
---|---|
State | Superseded, archived |
Headers | show |
Series | [1/2] dt-bindings: usb: snps,dwc3: Allow power-domains property | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | warning | total: 0 errors, 1 warnings, 151 lines checked |
robh/patch-applied | success | |
robh/dtbs-check | warning | build log |
robh/dt-meta-schema | success |
Rob Herring <robh@kernel.org> writes: > The rockchip,dwc3.yaml schema defines a single DWC3 node, but the RK3399 > uses the discouraged parent wrapper node and child 'generic' DWC3 node. Why discouraged? Splitting those two separate devices (yes, they are separate physical modules) has greatly simplified e.g. power management and encapsulation of the core module.
On Tue, Dec 20, 2022 at 1:37 AM Felipe Balbi <balbi@kernel.org> wrote: > > Rob Herring <robh@kernel.org> writes: > > > The rockchip,dwc3.yaml schema defines a single DWC3 node, but the RK3399 > > uses the discouraged parent wrapper node and child 'generic' DWC3 node. > > Why discouraged? Splitting those two separate devices (yes, they are > separate physical modules) has greatly simplified e.g. power management > and encapsulation of the core module. Sometimes they are separate and that's fine, but often it's just different clocks, resets, etc. and that's no different from every other block. If there's wrapper registers or something clearly extra, then I agree a wrapper parent node makes sense. Otherwise, for cases like RK3399, I don't think it does, but we're stuck with it now. Also, we have this pattern pretty much nowhere else and DWC3 is not special. Rob
Rob Herring <robh@kernel.org> writes: > On Tue, Dec 20, 2022 at 1:37 AM Felipe Balbi <balbi@kernel.org> wrote: >> >> Rob Herring <robh@kernel.org> writes: >> >> > The rockchip,dwc3.yaml schema defines a single DWC3 node, but the RK3399 >> > uses the discouraged parent wrapper node and child 'generic' DWC3 node. >> >> Why discouraged? Splitting those two separate devices (yes, they are >> separate physical modules) has greatly simplified e.g. power management >> and encapsulation of the core module. > > Sometimes they are separate and that's fine, but often it's just > different clocks, resets, etc. and that's no different from every > other block. Right, then the argument is that all other blocks are not modelling the HW as they should :) > If there's wrapper registers or something clearly extra, then I agree > a wrapper parent node makes sense. There's always wrapper-specific registers. Some wrappers even add custom functionality. IIRC Qcom added a HW-based URB "scheduler" or some sort. > Otherwise, for cases like RK3399, I don't think it does, but we're > stuck with it now. > > Also, we have this pattern pretty much nowhere else and DWC3 is not > special. No, it's not. But it could just be the first example of the driver actually modelling the underlying HW. In any case, I was just curious with your opinion that this model is discouraged, as it's not stated anywhere in the kernel's documentation. Happy holidays
diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml b/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml index b3798d94d2fd..edb130c780e4 100644 --- a/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml +++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml @@ -29,7 +29,6 @@ select: contains: enum: - rockchip,rk3328-dwc3 - - rockchip,rk3399-dwc3 - rockchip,rk3568-dwc3 required: - compatible @@ -39,7 +38,6 @@ properties: items: - enum: - rockchip,rk3328-dwc3 - - rockchip,rk3399-dwc3 - rockchip,rk3568-dwc3 - const: snps,dwc3 @@ -90,7 +88,7 @@ required: examples: - | - #include <dt-bindings/clock/rk3399-cru.h> + #include <dt-bindings/clock/rk3328-cru.h> #include <dt-bindings/interrupt-controller/arm-gic.h> bus { @@ -98,11 +96,11 @@ examples: #size-cells = <2>; usbdrd3_0: usb@fe800000 { - compatible = "rockchip,rk3399-dwc3", "snps,dwc3"; + compatible = "rockchip,rk3328-dwc3", "snps,dwc3"; reg = <0x0 0xfe800000 0x0 0x100000>; interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>; - clocks = <&cru SCLK_USB3OTG0_REF>, <&cru SCLK_USB3OTG0_SUSPEND>, - <&cru ACLK_USB3OTG0>, <&cru ACLK_USB3_GRF>; + clocks = <&cru SCLK_USB3OTG_REF>, <&cru SCLK_USB3OTG_SUSPEND>, + <&cru ACLK_USB3OTG>; clock-names = "ref_clk", "suspend_clk", "bus_clk", "grf_clk"; dr_mode = "otg"; diff --git a/Documentation/devicetree/bindings/usb/rockchip,rk3399-dwc3.yaml b/Documentation/devicetree/bindings/usb/rockchip,rk3399-dwc3.yaml new file mode 100644 index 000000000000..e39a8a3a7ab3 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/rockchip,rk3399-dwc3.yaml @@ -0,0 +1,115 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/usb/rockchip,rk3399-dwc3.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Rockchip RK3399 SuperSpeed DWC3 USB SoC controller + +maintainers: + - Heiko Stuebner <heiko@sntech.de> + +properties: + compatible: + const: rockchip,rk3399-dwc3 + + '#address-cells': + const: 2 + + '#size-cells': + const: 2 + + ranges: true + + clocks: + items: + - description: + Controller reference clock, must to be 24 MHz + - description: + Controller suspend clock, must to be 24 MHz or 32 KHz + - description: + Master/Core clock, must to be >= 62.5 MHz for SS + operation and >= 30MHz for HS operation + - description: + USB3 aclk peri + - description: + USB3 aclk + - description: + Controller grf clock + + clock-names: + items: + - const: ref_clk + - const: suspend_clk + - const: bus_clk + - const: aclk_usb3_rksoc_axi_perf + - const: aclk_usb3 + - const: grf_clk + + resets: + maxItems: 1 + + reset-names: + const: usb3-otg + +patternProperties: + '^usb@': + $ref: snps,dwc3.yaml# + +additionalProperties: false + +required: + - compatible + - '#address-cells' + - '#size-cells' + - ranges + - clocks + - clock-names + - resets + - reset-names + +examples: + - | + #include <dt-bindings/clock/rk3399-cru.h> + #include <dt-bindings/power/rk3399-power.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + + bus { + #address-cells = <2>; + #size-cells = <2>; + + usb { + compatible = "rockchip,rk3399-dwc3"; + #address-cells = <2>; + #size-cells = <2>; + ranges; + clocks = <&cru SCLK_USB3OTG0_REF>, <&cru SCLK_USB3OTG0_SUSPEND>, + <&cru ACLK_USB3OTG0>, <&cru ACLK_USB3_RKSOC_AXI_PERF>, + <&cru ACLK_USB3>, <&cru ACLK_USB3_GRF>; + clock-names = "ref_clk", "suspend_clk", + "bus_clk", "aclk_usb3_rksoc_axi_perf", + "aclk_usb3", "grf_clk"; + resets = <&cru SRST_A_USB3_OTG0>; + reset-names = "usb3-otg"; + + usb@fe800000 { + compatible = "snps,dwc3"; + reg = <0x0 0xfe800000 0x0 0x100000>; + interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH 0>; + clocks = <&cru SCLK_USB3OTG0_REF>, <&cru ACLK_USB3OTG0>, + <&cru SCLK_USB3OTG0_SUSPEND>; + clock-names = "ref", "bus_early", "suspend"; + dr_mode = "otg"; + phys = <&u2phy0_otg>, <&tcphy0_usb3>; + phy-names = "usb2-phy", "usb3-phy"; + phy_type = "utmi_wide"; + snps,dis_enblslpm_quirk; + snps,dis-u2-freeclk-exists-quirk; + snps,dis_u2_susphy_quirk; + snps,dis-del-phy-power-chg-quirk; + snps,dis-tx-ipgap-linecheck-quirk; + power-domains = <&power RK3399_PD_USB3>; + }; + }; + }; +...
The rockchip,dwc3.yaml schema defines a single DWC3 node, but the RK3399 uses the discouraged parent wrapper node and child 'generic' DWC3 node. The intent was to modify the RK3399 DTs to use a single node, but the DT changes were rejected for ABI reasons. However, the schema was accepted as-is. To fix this, we need to move the RK3399 binding to its own schema file. The RK3328 and RK3568 bindings are correct and use a single node. Cc: Johan Jonker <jbx6244@gmail.com> Signed-off-by: Rob Herring <robh@kernel.org> --- .../bindings/usb/rockchip,dwc3.yaml | 10 +- .../bindings/usb/rockchip,rk3399-dwc3.yaml | 115 ++++++++++++++++++ 2 files changed, 119 insertions(+), 6 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/rockchip,rk3399-dwc3.yaml