On Thu, Jan 16, 2025 at 8:08 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: > > On 15/01/2025 02:26, Peter Geis wrote: > > Add documentation for the usb3 phy as implemented on the rk3328 SoC. > > > > Signed-off-by: Peter Geis <pgwipeout@xxxxxxxxx> > > --- > > > > .../bindings/phy/rockchip,inno-usb3phy.yaml | 166 ++++++++++++++++++ > > 1 file changed, 166 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml > > > > diff --git a/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml > > new file mode 100644 > > index 000000000000..cde489ca87ab > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml > > @@ -0,0 +1,166 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > Wrong license. > > Please run scripts/checkpatch.pl and fix reported warnings. After that, > run also `scripts/checkpatch.pl --strict` and (probably) fix more > warnings. Some warnings can be ignored, especially from --strict run, > but the code here looks like it needs a fix. Feel free to get in touch > if the warning is not clear. Checkpatch literally told me to change this. Ran against my original dev binding: ./scripts/checkpatch.pl --strict Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml CHECK: DT binding documents should be licensed (GPL-2.0-only OR BSD-2-Clause) #1: FILE: Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml:1: +# SPDX-License-Identifier: GPL-2.0 > > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/phy/rockchip,inno-usb3phy.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Rockchip USB 3.0 phy with Innosilicon IP block > > > + > > +maintainers: > > + - Heiko Stuebner <heiko@xxxxxxxxx> > > + > > +properties: > > + compatible: > > + enum: > > + - rockchip,rk3328-usb3phy > > Why is this binding entirely different than existing usb2 phy? Nothing > in common? This looks like made for driver and both - driver and binding > - duplicating a lot. Hmm, I hadn't considered merging it into the usb2 binding as it is a unique (and uniquely broken) device. I'd like Heiko's thoughts on this. > > > + > > + reg: > > + maxItems: 1 > > + > > + clocks: > > + minItems: 3 > > Drop > > > + maxItems: 3 > > + > > + clock-names: > > + items: > > + - const: refclk-usb3otg > > ref > > > + - const: usb3phy-otg > > otg > > > + - const: usb3phy-pipe > > pipe > > > + > > + interrupts: > > + minItems: 4 > > You code here randomly. reg has only maxItems, clocks have both and this > have only minItems. Does not make sense. If you get it wrong, I would > assume you repeat the same mistake but here it is like randomly chosen > pieces. And this is making me wonder whether this was not sent too fast. I admit, I dread writing bindings as even now I'm weak in YAML and it seems to pick and choose what rules it wants to follow. > > Anyway: only maxItems. > > > > + > > + interrupt-names: > > + items: > > + - const: bvalid > > + - const: id > > + - const: linestate > > + - const: rxdet > > + > > + resets: > > + minItems: 6 > > maxItems instead > > > + > > + reset-names: > > + items: > > + - const: usb3phy-u2-por > > + - const: usb3phy-u3-por > > + - const: usb3phy-pipe-mac > > + - const: usb3phy-utmi-mac > > + - const: usb3phy-utmi-apb > > + - const: usb3phy-pipe-apb > > + > > + "#address-cells": > > + const: 2 > > + > > + "#size-cells": > > + const: 2 > > + > > + ranges: true > > + > > +patternProperties: > > + > > Drop blank line > > > + utmi-port@[0-9a-f]+$: > > This wasn't tested. Missing quotes, missing starting anchor. make W=1 dt_binding_check didn't complain about it, using the dt-schema from pip3 from about a week ago. > > > + type: object > > Explain what are the children here - description. Fair, will do. > > > > + additionalProperties: false > > + > > + properties: > > + compatible: > > + enum: > > + - rockchip,rk3328-usb3phy-utmi > > + > > + reg: > > + maxItems: 1 > > + > > + "#phy-cells": > > + const: 0 > > Does not look correct. Your parent device is the phy, not child. Why do > you create children per each type of phy? Because that's how it's done elsewhere in Rockchip's phys [1]. How should it be done? > > > + > > + phy-supply: > > + description: > > + Phandle to a regulator that provides power to VBUS. > > + See ./phy-bindings.txt for details. > > + > > + required: > > + - compatible > > + - reg > > + - "#phy-cells" > > + > > + pipe-port@[0-9a-f]+$: > > + type: object > > + additionalProperties: false > > + > > + properties: > > + compatible: > > + enum: > > + - rockchip,rk3328-usb3phy-pipe > > + > > + reg: > > + maxItems: 1 > > + > > + "#phy-cells": > > + const: 0 > > + > > + phy-supply: > > + description: > > + Phandle to a regulator that provides power to VBUS. > > + See ./phy-bindings.txt for details. > > Drop "see ....". I was tempted to convert phy-bindings.txt over but as above I dread writing bindings. Will drop. > > > + > > + required: > > + - compatible > > + - reg > > + - "#phy-cells" > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - clock-names > > + - interrupts > > + - interrupt-names > > + - resets > > + - reset-names > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/clock/rk3328-cru.h> > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > + soc { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + > > + usb3phy: usb3-phy@ff460000 { > > + compatible = "rockchip,rk3328-usb3phy"; > > + reg = <0x0 0xff460000 0x0 0x10000>; > > + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>; > > That's way over the limit. Wrapping is at 80. Will correct. > > > + clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe"; > > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "bvalid", "id", "linestate", "rxdet"; > > > I appreciate all the feedback, I'll incorporate the changes you've recommended. Very Respectfully, Peter Geis > > Best regards, > Krzysztof [1] https://elixir.bootlin.com/linux/v6.13-rc3/source/arch/arm64/boot/dts/rockchip/rk3328.dtsi#L887
