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

[Michael Riesch <michael.riesch@wolfvision.net>]

Hi Peter,

On 2/25/22 18:59, Peter Geis wrote:
> On Fri, Feb 25, 2022 at 12:01 PM Michael Riesch
> <michael.riesch@wolfvision.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@fd000000 {
>>
>> 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@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>;
>>>       };
>>>