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