From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 44788C4332F for ; Sun, 11 Dec 2022 12:23:09 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 0FDAA8538D; Sun, 11 Dec 2022 13:23:07 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=tinet.cat Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 4478A8538C; Sun, 11 Dec 2022 13:23:05 +0100 (CET) Received: from mx1.tinet.cat (mx1.tinet.org [195.77.216.146]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id A9CD88538D for ; Sun, 11 Dec 2022 13:23:02 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=tinet.cat Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=xdrudis@tinet.cat X-ASG-Debug-ID: 1670761380-163e7b330c5cade0001-4l7tJC Received: from smtp01.tinet.cat (smtp.tinet.cat [195.77.216.131]) by mx1.tinet.cat with ESMTP id D700vfEu0BGV9YOV; Sun, 11 Dec 2022 13:23:00 +0100 (CET) X-Barracuda-Envelope-From: xdrudis@tinet.cat X-Barracuda-Effective-Source-IP: smtp.tinet.cat[195.77.216.131] X-Barracuda-Apparent-Source-IP: 195.77.216.131 Received: from localhost (190.red-79-152-181.dynamicip.rima-tde.net [79.152.181.190]) by smtp01.tinet.cat (Postfix) with ESMTPSA id A0F7E605D0AC; Sun, 11 Dec 2022 13:23:00 +0100 (CET) Date: Sun, 11 Dec 2022 13:22:47 +0100 From: Xavier Drudis Ferran To: Marek Vasut Cc: Xavier Drudis Ferran , u-boot@lists.denx.de, Simon Glass , Philipp Tomsich , Kever Yang , Lukasz Majewski , Sean Anderson Subject: Re: [PATCH v4 2/2] arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations for the 480MHz usb2phy clock in rk3399. Message-ID: X-ASG-Orig-Subj: Re: [PATCH v4 2/2] arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations for the 480MHz usb2phy clock in rk3399. References: <1f0e75c7-e4f3-5332-03fa-6f08f7378b08@denx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1f0e75c7-e4f3-5332-03fa-6f08f7378b08@denx.de> X-Barracuda-Connect: smtp.tinet.cat[195.77.216.131] X-Barracuda-Start-Time: 1670761380 X-Barracuda-URL: https://webmail.tinet.cat:443/cgi-mod/mark.cgi X-Barracuda-Scan-Msg-Size: 4445 X-Barracuda-BRTS-Status: 1 X-Barracuda-Bayes: SPAM GLOBAL 0.9999 1.0000 4.3422 X-Barracuda-Spam-Score: 4.34 X-Barracuda-Spam-Status: No, SCORE=4.34 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=6.0 KILL_LEVEL=8.0 tests= X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.102758 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean El Sun, Dec 11, 2022 at 06:20:41AM +0100, Marek Vasut deia: > On 12/9/22 16:47, Xavier Drudis Ferran wrote: > > This clock has no users but appears in a phandle list used by > > ehci-generic.c to bulk enable it. The phandle list comes from linux, > > where it is needed for suspend/resume to work [1]. > > > > My tests give the same results with or without this patch, but Marek > > Vasut found it weird to declare an empty clk_ops[2]. > > > > So I adapted the code from linux 6.1-rc8 so that it hopefully works > > if it ever has some user. For now, without real users, it seems to > > at least not give any errors. > > You might want to squash 1/2 and 2/2 together, since it's one change (add > clock operations to a phy driver). > I prefer to send them separate, but I don't mind if they are squashed when applied. In my mind it is better if the second patch can be reverted easily if it ever is found to give any trouble (or just increase code size). But I'm not the most knowledgeable person in this list by a huge margin, so if mantainers apply it squashed I'll be happy with my 2 USB2 ports working. In fact I don't mind if u-boot takes v4, v3, v2 or v1 of this, or any other solution, I just want 2 USB2 ports in Rock Pi 4B to be usable in u-boot. As it is now only the USB3 ports work in u-boot, unless one enables CONFIG_OCHI_GENERIC, and then the USB2 ports would work slowly. > Since 1/2 works without any clock operations, who does enable these usb > clock on this SoC ? Is there any driver or platform code for that ? If so, > you can drop that platform code with this driver-side implementation in > place (which is nice). > I'm sorry if I'm not being clear. I can't drop that code, it needs to enable 2 clocks, but it is enabling 3. I can remove one clock from the list of enabled clocks (I did so in v1) or I can leave it and ignore the error (I did so in v2). But since that didn't seem to please the list, I tried v3 and v4. Maybe I just added noise and should have waited further opinions. ehci-generic.c bulk enables all clocks in the phandle list assigned to the usb host (usb_host_0ehci). The arch/arm/dts/rk3399.dtsi provides such a list. But the last clock is the usb phy &u2phy0. The phy is otherwise enabled, and the output clock associated with it seems not to serve any purpose, since u-boot works when I remove &u2phy0 from the clock phandle list (v1), and also when I leave it there but ignore the error for the missing clock (v2). In master, v1 and v2 &u2phy0 is UCLASS_PHY, but not UCLASS_CLK. With version v3 the &u2phy0 is bound as UCLASS_CLK too, so the bulk enable from ehci-generic succeeds, although it does nothing to the hardware (the clock driver having no operations). This does not have any bad effect as far as I can see. The simple fact that ehci-generic doesn't fail makes the 2 USB2 ports usable in u-boot. With v4 the &u2phy0 is bound as UCLASS_CLK, it is bulk enabled, and that ends up calling an operation that does touch the hardware. This still doesn't have any bad effect, but I can't tell that it works, because it does the same as in v3 or v2 or v1. So when I say there are no users, I don't mean the code isn't called. It is, but the behaviour I can see is the same when it is and when it isn't, so whatever the enabled clock does, it does not seem that u-boot is using it. Why is it in the phandle list then ? Because it apparently helps linux resume or suspend. When the output clock is not enabled, something seems to block linux resume. But I'm not testing linux resume, and it doesn't matter, because in linux the output clock is enabled anyway, because &u2phy0 provides a clk, and is in the clock phandle list of usb_host0_ehci). Since u-boot and linux share rk3399.dtsi the &u2phy0 is in the phandle list in u-boot too, but since in master it isn't UCLASS_CLK, ehci-generic fails and USB2 is not available in that port. Maybe there is some use for the clock in u-boot that I'm not seeing because I don't test good enough or I don't know enough about USB, or lack USB testing equipment, or something. > btw I am still hoping some of the rockchip people will have a look at this > series. Yes, I'm looking forward to it too. So far Kever Yang reacted in late August to v1 by proposing v2 (as far as I understand). Thanks for listening to my walls of text. -- Xavi