linux-kernel - Re: [PATCH v1 7/8] arm64: dts: rockchip: add rk356x dwc3 usb3 nodes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4bc8533b-cc60-a524-551c-5edd29905506@wolfvision.net>
Date:   Fri, 25 Feb 2022 19:25:57 +0100
From:   Michael Riesch <michael.riesch@...fvision.net>
To:     Peter Geis <pgwipeout@...il.com>
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

Hi Peter,

On 2/25/22 18:59, Peter Geis wrote:
> 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.

That'd be great, thanks! I'll help testing of course :-)

>> On 2/25/22 15:54, Peter Geis wrote:
> [...]
>>> +             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.

Ah OK, makes sense!

Best regards,
Michael

> 
>>
>>> +             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>;
>>>       };
>>>