Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <4bc8533b-cc60-a524-551c-5edd29905506@wolfvision.net>
Date:   Fri, 25 Feb 2022 19:25:57 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [PATCH v1 7/8] arm64: dts: rockchip: add rk356x dwc3 usb3 nodes
Content-Language: en-US
To:     Peter Geis <pgwipeout@gmail.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        Heiko Stuebner <heiko@sntech.de>,
        devicetree <devicetree@vger.kernel.org>,
        arm-mail-list <linux-arm-kernel@lists.infradead.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20220225145432.422130-1-pgwipeout@gmail.com>
 <20220225145432.422130-8-pgwipeout@gmail.com>
 <2f1f09c0-9b7e-5145-fb25-a5f1fe4d0ab4@wolfvision.net>
 <CAMdYzYqRsOji3PfxK_FgOUbg4KomC+LnWgkTXS7=YRKTuPYVow@mail.gmail.com>
From:   Michael Riesch <michael.riesch@wolfvision.net>
In-Reply-To: <CAMdYzYqRsOji3PfxK_FgOUbg4KomC+LnWgkTXS7=YRKTuPYVow@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?akRzdXBQQ1FENmpnQUpIajdBWVhpd2hpUC9NS2dTVzZEb2F5cjA5ZkljZFdv?=
 =?utf-8?B?TncxQzN0R1hZck5vLzhKcXErSjdRNXVuU1Z2Z0FFNVFTcm5QaC9maENWN3py?=
 =?utf-8?B?NkxPTzNGQlhZeWIrNGNHK2FUN0xudVlocGV3SXppKzFnSjk0K2JNSHlDYTF3?=
 =?utf-8?B?OWZzaVY5cHVnbHlwYWllamk5VVNwUXJhV0dONUNjYU1ZcTc5RndBempjTkxP?=
 =?utf-8?B?KzAvR0o0ZC9nczM3T3ZmZHhhVStiYmJEbEJlNXcwb1BMc2dKNmlQNkRxY0Ey?=
 =?utf-8?B?SjJiUzVZQ2FFRTJCa2l3enNSaTdrbEdZc0lEOEs3S1hQb0VsdXI5dkNlRXBL?=
 =?utf-8?B?V2R3eGUwUlhkelN0MWVIaTFzOWNwbUtYUU5JT3dKT2lxRzJYRFhtK2J6NUFJ?=
 =?utf-8?B?N083dVpTMXlPano3UE8xODdsWTNTb1hTRUNONUE3ZGVuaTEzcUR1cnpDaEs4?=
 =?utf-8?B?UmxSYzNhWllhSjBiQ0NIYXFoUVVKUWlKYkp6ME5JU2dLT3RGdDFlNDVCKzRv?=
 =?utf-8?B?c1RxNDQrU0lQNUh1bVVyUjBJNzd4aE5RU2RVVXBZVSswWnp3QzRadGFCUFov?=
 =?utf-8?B?dUc1VCtzc1Q2Y0FCeWdqSWtpbVV2S0ttd2dYMWwxTjNEeVdldm1SUXNTcGZw?=
 =?utf-8?B?U0YyaFN2a28rVitLcmNVbXZKKzVDSHR1M1FXM3VpdU5UT3FEb2YyaWhvU3VZ?=
 =?utf-8?B?dFF5aGZmdnQza1BoWVhkTnJ4cUJmUy9tOEl5b3N5RHZ5YXl0TDVaaXBmM0R6?=
 =?utf-8?B?SWJrT1ZOUmVYUC95UnJsU25UMEdybFluWE5xV3c1OExZc3FKSGYzdlhZOHZj?=
 =?utf-8?B?dWZLbUVmVHVFY2lsc0VrRitvU3hCZGVVQXZRWGEwdFFPRHFsYURNTjdObWcr?=
 =?utf-8?B?ZWNDTy9pWUFvbmVVdE1GZ3NiR01FSllDZHlIUWRSL2luZEp6cjZMWjMycjdv?=
 =?utf-8?B?RlMrbktVYktlOTg2TTVVa0QvOFc5RXEwVU84Q0JjQlE0WW5ObStybGU5ekFu?=
 =?utf-8?B?NVU2S21PYXhrV2ZLclFaSWViOEZZb1BmSm5hMTVnU2Y0dlpndXV1WGpzQ0pV?=
 =?utf-8?B?OGhwVndta0V5TGFJZVZGZGdqTllLMllWOTJYYlYvcWE1NkRWR2o1R25qZTlO?=
 =?utf-8?B?NUtSTmRXMlVOVjBFUk81cFpQRE1aTXVlUGFrOTE2aW1hMnM3UGxHRFVxSHI0?=
 =?utf-8?B?M053OVRrMzNnT3doZGlYOEd2bVR2SzY2YkxFNXVSNmF6RXNvaENwaGk0TTMx?=
 =?utf-8?B?WFhDbDMwb2JuT3lnQ3ZKS1RlVGNoN3BFV0t1TWpQVllrRTZKS0NTK1VSQWNa?=
 =?utf-8?B?cDlTS0l0dTBnNjRKeEthN1I1TjV4T3RKb1VScXArNER6RnBBRnN0YUdYVDAx?=
 =?utf-8?B?c3ZaVEZqWlFHNDYvRHBXVjJwZU1Cc2lMdThuSTVSbHRFcnhjOThLbW8zeUZJ?=
 =?utf-8?B?NGs2Q1l2anJzNldHZWp5NFFwQ21maE1xMnF1TWJZeE1NbFFVUUc3Rjk3UjNp?=
 =?utf-8?B?TC9KUUVaN1g1QWR4SjJMMmFoS1FPT2RwdlkzZ3N6WklEZG0wa3lkWXFUY1Jv?=
 =?utf-8?B?cmMzeXVOaG1oenlLd1dtVjQ0RU5vSjcrWnZHaXBJd0YwazdpTkc3VHZHM05p?=
 =?utf-8?B?QUwyVk5lKzFDbktra3dKOVZhd1ZiWWlDc2gvN3hUVXQ5eURUMWticHZYRmdo?=
 =?utf-8?B?RVRtWHhYMzJHZnkzZ1NYTi9qVUdqYUdEU1RTSys2RUZYNzU3U295MXRTaStT?=
 =?utf-8?B?MHRMbHFQL0szWlJFdEo0dGZ0dDQ2dDhoZi9iLyt0dnk5S0UrdG80RlVzZWlU?=
 =?utf-8?B?NS9IMTA1OGhvVE9iSVlRM0hQU1FZWGVHSElTVHlGTyt5bWpRYU5FNzdFSS9n?=
 =?utf-8?B?VVdvMGlHSmpqSkZrbVhWc0tUSG9pK0s3M0lxZkFpM0t4NlVjYXEzaGswWG5s?=
 =?utf-8?B?eEZtLy96VFkySVhyUVd6eWFLN0k0b3FBNDVrclpQV1Zsd2tTMm0zdTBQMTF4?=
 =?utf-8?B?VlBaR2xwYWh1ZmZZWWMwaGwrOU5JMVovOGIxK21MeWdVcmZUUUZPWGJoOEgx?=
 =?utf-8?B?OU1zcmNLQnBuVG1EQmMrZ3E2YWUyMGRacEt6bEU5cVJxMkZEMkhTOGExY1JG?=
 =?utf-8?B?cmw5MUFEczJoL0RYOENwSDhxS3lDYUFIUGZNL0J1dUw1ZnVjRUk1VVpVSk1r?=
 =?utf-8?B?b3l4NWJHM3lyanJ2THN2K1RsR0tOY3plRE0wekFJc3VteVd6aVZ4aFZWbFZo?=
 =?utf-8?B?M1d6WE52UTBlenNHZnE5T3cvSzM4K1lhOHl4RGF3K1BYczRwYURkTlVCYzBW?=
 =?utf-8?B?L3UrbStCd3p1SitjbnJ4NE9seHNYU3pJUnEvM0dWOCs2RXNPRUJSdnlMUWdk?=
 =?utf-8?Q?UdoFfiiv4efySRCw=3D?=
X-OriginatorOrg: wolfvision.net
X-MS-Exchange-CrossTenant-Network-Message-Id: 6d382b78-b9b7-4282-bfaf-08d9f88c4563
X-MS-Exchange-CrossTenant-AuthSource: VI1PR0802MB2430.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Feb 2022 18:25:59.4629
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: e94ec9da-9183-471e-83b3-51baa8eb804f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ZJyweyvIrkhNpBOwtmcssyS/KHYATvXfeUi+xjelfCMbz5dV0Emlx8+kY+5U/6QRQrjVFcRDBFjcOjgq2FUwAPkK6MP4O6X+A8GT+nEOb0E=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR08MB6537
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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