Date:   Fri, 25 Feb 2022 12:59:26 -0500
From:   Peter Geis <pgwipeout@...il.com>
To:     Michael Riesch <michael.riesch@...fvision.net>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Heiko Stuebner <heiko@...ech.de>,
        devicetree <devicetree@...r.kernel.org>,
        arm-mail-list <linux-arm-kernel@...ts.infradead.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 7/8] arm64: dts: rockchip: add rk356x dwc3 usb3 nodes

On Fri, Feb 25, 2022 at 12:01 PM Michael Riesch
<michael.riesch@...fvision.net> wrote:
>
> Hi Peter,
>
> (It should be noted that there was a slight mishap in communications
> between the two of us resulting in two series with the same goal. Now
> let's clean up the mess :-)
>
> Thanks for your series. Seeing that it contains more patches than mine
> it probably makes sense to use your series as basis. Please Cc: me in
> future iterations of this patch series and consider my comments below.

Will do.
If you'd like I can also pull your enablement patch.

>
> On 2/25/22 15:54, Peter Geis wrote:
> > Add the dwc3 device nodes to the rk356x device trees.
> > The rk3566 has one usb2 capable dwc3 otg controller and one usb3 capable
> > dwc3 host controller.
> > The rk3568 has one usb3 capable dwc3 otg controller and one usb3 capable
> > dwc3 host controller.
> >
> > Signed-off-by: Peter Geis <pgwipeout@...il.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3566.dtsi | 12 +++++++
> >  arch/arm64/boot/dts/rockchip/rk3568.dtsi |  9 +++++
> >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 45 +++++++++++++++++++++++-
> >  3 files changed, 65 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3566.dtsi b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> > index 3839eef5e4f7..8e8b52f58f44 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> > @@ -6,6 +6,10 @@ / {
> >       compatible = "rockchip,rk3566";
> >  };
> >
> > +&pipegrf {
> > +     compatible = "rockchip,rk3566-pipe-grf", "syscon";
> > +};
> > +
> >  &power {
> >       power-domain@...568_PD_PIPE {
> >               reg = <RK3568_PD_PIPE>;
> > @@ -18,3 +22,11 @@ power-domain@...568_PD_PIPE {
> >               #power-domain-cells = <0>;
> >       };
> >  };
> > +
> > +&usbdrd30 {
>
> I would really love to have some alignment with the other USB controllers
>
> usb_host{0,1}_{e,o}hci
>
> here. I am aware that older SoCs and the SDK are using these names and
> it might be painful to have different versions to maintain at the
> moment, but can we please agree on
>
> usb_host0_xhci
> usb_host1_xhci
>
> or something like that?

I agree, I like your naming better.

>
> > +     phys = <&usb2phy0_otg>;
> > +     phy-names = "usb2-phy";
> > +     extcon = <&usb2phy0>;
> > +     maximum-speed = "high-speed";
> > +     snps,dis_u2_susphy_quirk;
> > +};
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> > index 5b0f528d6818..77c044cbaaad 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> > @@ -99,6 +99,10 @@ opp-1992000000 {
> >       };
> >  };
> >
> > +&pipegrf {
> > +     compatible = "rockchip,rk3568-pipe-grf", "syscon";
> > +};
> > +
> >  &power {
> >       power-domain@...568_PD_PIPE {
> >               reg = <RK3568_PD_PIPE>;
> > @@ -114,3 +118,8 @@ power-domain@...568_PD_PIPE {
> >               #power-domain-cells = <0>;
> >       };
> >  };
> > +
> > +&usbdrd30 {
> > +     phys = <&usb2phy0_otg>, <&combphy0 PHY_TYPE_USB3>;
> > +     phy-names = "usb2-phy", "usb3-phy";
> > +};
> > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > index 84d5d607e693..4fae5b3b326e 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > @@ -230,6 +230,50 @@ scmi_shmem: sram@0 {
> >               };
> >       };
> >
> > +     usbdrd30: usbdrd@...00000 {
> > +             compatible = "rockchip,rk3568-dwc3", "snps,dwc3";
> > +             reg = <0x0 0xfcc00000 0x0 0x400000>;
> > +             interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>;
> > +             clocks = <&cru CLK_USB3OTG0_REF>, <&cru CLK_USB3OTG0_SUSPEND>,
> > +                      <&cru ACLK_USB3OTG0>, <&cru PCLK_PIPE>;
> > +             clock-names = "ref_clk", "suspend_clk",
> > +                           "bus_clk", "grf_clk";
>
> Please consider Johan's comments on my first series. In my tests
> removing the PCLK_PIPE clock did not make any difference.

I'd like to test that this isn't being used, or isn't just working
because it's enabled elsewhere.
If both of those are false, then I'll be happy to drop this.

>
> > +             dr_mode = "host";
>
> Based on the description in the commit message above it should be "otg",
> right? Boards are free to overrule this, of course.

Currently the usb2phy does not support OTG mode correctly.
There are patches in the works for this, but at the moment it's safer
to default to host.

>
> > +             phy_type = "utmi_wide";
> > +             power-domains = <&power RK3568_PD_PIPE>;
> > +             resets = <&cru SRST_USB3OTG0>;
> > +             reset-names = "usb3-otg";
> > +             snps,dis_enblslpm_quirk;
> > +             snps,dis-u2-freeclk-exists-quirk;
> > +             snps,dis-del-phy-power-chg-quirk;
> > +             snps,dis-tx-ipgap-linecheck-quirk;
> > +             snps,xhci-trb-ent-quirk;
>
> In my first version I had all those quirks as well, but are they
> actually necessary? I decided to remove them all to have a fresh start
> (also activating them did not seem to affect my test setup).

I'm now curious about this, can someone weigh in on valid ways of
testing each one of these in a way that is definite?

>
> > +             status = "disabled";
> > +     };
> > +
> > +     usbhost30: usbhost@...00000 {
>
> Please reconsider the this name as well.
>
> > +             compatible = "rockchip,rk3568-dwc3", "snps,dwc3";
> > +             reg = <0x0 0xfd000000 0x0 0x400000>;
> > +             interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>;
> > +             clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>,
> > +                      <&cru ACLK_USB3OTG1>, <&cru PCLK_PIPE>;
> > +             clock-names = "ref_clk", "suspend_clk",
> > +                           "bus_clk", "grf_clk";
> > +             dr_mode = "host";
>
> Here "host" clearly makes sense, as this controller is not capable of otg.
>
> > +             phys = <&usb2phy0_host>, <&combphy1 PHY_TYPE_USB3>;
> > +             phy-names = "usb2-phy", "usb3-phy";
> > +             phy_type = "utmi_wide";
> > +             power-domains = <&power RK3568_PD_PIPE>;
> > +             resets = <&cru SRST_USB3OTG1>;
> > +             reset-names = "usb3-host";
> > +             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;
>
> What was said about quirks above holds here as well (although one quirk
> not documented in the bindings is missing here).

Same thing here, I'd like absolute testing to determine that these are
not necessary, since downstream (the oem) felt they were.

>
> Best regards,
> Michael
>
> > +             status = "disabled";
> > +     };
> > +
> >       gic: interrupt-controller@...00000 {
> >               compatible = "arm,gic-v3";
> >               reg = <0x0 0xfd400000 0 0x10000>, /* GICD */
> > @@ -297,7 +341,6 @@ pmu_io_domains: io-domains {
> >       };
> >
> >       pipegrf: syscon@...50000 {
> > -             compatible = "rockchip,rk3568-pipe-grf", "syscon";
> >               reg = <0x0 0xfdc50000 0x0 0x1000>;
> >       };
> >

Powered by blists - more mailing lists