Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220226184147.769964-1-pgwipeout@gmail.com> <20220226184147.769964-10-pgwipeout@gmail.com>
 <4a49ba17-436b-c6b3-4a7d-42902781d2f4@gmail.com>
In-Reply-To: <4a49ba17-436b-c6b3-4a7d-42902781d2f4@gmail.com>
From:   Peter Geis <pgwipeout@gmail.com>
Date:   Sun, 27 Feb 2022 08:56:06 -0500
Message-ID: <CAMdYzYoKfyCOHrH-o1HDiMQqqQzvVrpY2TFVH4ANNzJvPg6rBQ@mail.gmail.com>
Subject: Re: [PATCH v2 09/11] arm64: dts: rockchip: add rk356x dwc3 usb3 nodes
To:     Johan Jonker <jbx6244@gmail.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        Heiko Stuebner <heiko@sntech.de>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
        Michael Riesch <michael.riesch@wolfvision.net>,
        devicetree <devicetree@vger.kernel.org>,
        arm-mail-list <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Feb 26, 2022 at 6:01 PM Johan Jonker <jbx6244@gmail.com> wrote:
>
>
>
> On 2/26/22 19:41, 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@gmail.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..a57eb68faba2 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@RK3568_PD_PIPE {
> >               reg = <RK3568_PD_PIPE>;
> > @@ -18,3 +22,11 @@ power-domain@RK3568_PD_PIPE {
> >               #power-domain-cells = <0>;
> >       };
> >  };
> > +
> > +&usb_host0_xhci {
> > +     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..8ba9334f9753 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@RK3568_PD_PIPE {
> >               reg = <RK3568_PD_PIPE>;
> > @@ -114,3 +118,8 @@ power-domain@RK3568_PD_PIPE {
> >               #power-domain-cells = <0>;
> >       };
> >  };
> > +
> > +&usb_host0_xhci {
> > +     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 7cdef800cb3c..b22e5a514ad7 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 {
> >               };
> >       };
> >
> > +     usb_host0_xhci: usb@fcc00000 {
> > +             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>;
> > +             clock-names = "ref_clk", "suspend_clk",
> > +                           "bus_clk";
> > +             dr_mode = "host";
> > +             phy_type = "utmi_wide";
>
> > +             power-domains = <&power RK3568_PD_PIPE>;
>
> When both usb_host0_xhci and usb_host1_xhci are connected to a usb2phy
> and the combphy's disabled there's no PCLK_PIPE enabled.
> Fix logic for RK3568_PD_PIPE by adding the USB3 clocks.
>
>
> > +             resets = <&cru SRST_USB3OTG0>;
>
> > +             reset-names = "usb3-otg";
>
> remove
>
> snps,dwc3.yaml only mentions the "resets" because
> devm_reset_control_array_get_optional_shared is used.
> reset-names is only a rk3399 legacy that I included due to the YAML
> conversion.
> With unevaluatedProperties now working "resets" also could be removed
> from rockchip,dwc3.yaml I think.

I've tested removing the reset entirely, and it seems the issues that
affected the rk3399 do not affect us here.

>
> https://github.com/torvalds/linux/commit/2f8e928408885dad5d8d6afefacb82100b6b62c7
> Added properties for rk3399 are:
>   power-domains
>   resets
>   reset-names
>
> > +             snps,dis_enblslpm_quirk;
> > +             snps,dis-u2-freeclk-exists-quirk;
> > +             snps,dis-del-phy-power-chg-quirk;
> > +             snps,dis-tx-ipgap-linecheck-quirk;
>
> sort
>
> > +             snps,xhci-trb-ent-quirk;
>
> ???
>
> check snps,dwc3.yaml

I've tried dropping all of these and testing all the configurations I
can think of.
The only one we seem to absolutely need is snps,dis_u2_susphy_quirk,
without which sometimes devices fail to enumerate after being removed.
The only weird behavior I found was on the Pinenote, where there is
only the OTG port and when a state change happens from host to device
mode the controller locks up the AXI bus for a few seconds until it
times out and gets reset by core.
OTG support is still a work in progress here though and is not supported yet.

TLDR: I'll be dropping all the quirks that have no apparent effect in
the v3, they can be added in if needed following broader testing.

Thanks for all the insight here!

>
> > +             status = "disabled";
> > +     };
> > +
> > +     usb_host1_xhci: usb@fd000000 {
> > +             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>;
> > +             clock-names = "ref_clk", "suspend_clk",
> > +                           "bus_clk";
> > +             dr_mode = "host";
> > +             phys = <&usb2phy0_host>, <&combphy1 PHY_TYPE_USB3>;
> > +             phy-names = "usb2-phy", "usb3-phy";
> > +             phy_type = "utmi_wide";
>
> > +             power-domains = <&power RK3568_PD_PIPE>;
>
> dito
>
> > +             resets = <&cru SRST_USB3OTG1>;
>
> > +             reset-names = "usb3-otg";
>
> remove
>
> > +             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;
>
> sort
>
> > +             status = "disabled";
> > +     };
> > +
> >       gic: interrupt-controller@fd400000 {
> >               compatible = "arm,gic-v3";
> >               reg = <0x0 0xfd400000 0 0x10000>, /* GICD */
> > @@ -297,7 +341,6 @@ pmu_io_domains: io-domains {
> >       };
> >
> >       pipegrf: syscon@fdc50000 {
> > -             compatible = "rockchip,rk3568-pipe-grf", "syscon";
> >               reg = <0x0 0xfdc50000 0x0 0x1000>;
> >       };
> >