On Thu, Jan 16, 2025 at 8:32 AM Peter Geis <pgwipeout@xxxxxxxxx> wrote:
>
> 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
I understand now, thank you. Perhaps checkpatch could put that in
quotes, instead saying ("GPL-2.0-only OR BSD-2-Clause") to make it
clear it's one thing.
>
> >
> > > +%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
