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 7D67EC4332F for ; Mon, 5 Dec 2022 19:02:32 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 9AB8C8567A; Mon, 5 Dec 2022 20:02:29 +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 E96BB851A4; Mon, 5 Dec 2022 20:02:27 +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 B06D58569E for ; Mon, 5 Dec 2022 20:02:21 +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: 1670266914-163e7b330c4268e0001-4l7tJC Received: from smtp01.tinet.cat (smtp01.tinet.cat [195.77.216.131]) by mx1.tinet.cat with ESMTP id 6uWnCJfCOE3n2TW2; Mon, 05 Dec 2022 20:01:54 +0100 (CET) X-Barracuda-Envelope-From: xdrudis@tinet.cat X-Barracuda-Effective-Source-IP: smtp01.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 E5B62605D0AC; Mon, 5 Dec 2022 19:54:45 +0100 (CET) Date: Mon, 5 Dec 2022 19:54:35 +0100 From: Xavier Drudis Ferran To: u-boot@lists.denx.de Cc: Simon Glass , Philipp Tomsich , Kever Yang , Lukasz Majewski , Sean Anderson , Marek Vasut Subject: [PATCH v2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2. Message-ID: X-ASG-Orig-Subj: [PATCH v2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2. MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline X-Barracuda-Connect: smtp01.tinet.cat[195.77.216.131] X-Barracuda-Start-Time: 1670266914 X-Barracuda-URL: https://webmail.tinet.cat:443/cgi-mod/mark.cgi X-Barracuda-Scan-Msg-Size: 7034 X-Barracuda-BRTS-Status: 1 X-Barracuda-Bayes: INNOCENT GLOBAL 0.8328 1.0000 2.5365 X-Barracuda-Spam-Score: 2.54 X-Barracuda-Spam-Status: No, SCORE=2.54 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.102626 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 arch/arm/dts/rk3399.dtsi has a node usb_host0_ehci: usb@fe380000 { compatible = "generic-ehci"; with clocks: clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>, <&u2phy0>; The first 2 refer to nodes with class UCLASS_CLK, but &u2phy0 has class UCLASS_PHY. u2phy0: usb2phy@e450 { compatible = "rockchip,rk3399-usb2phy"; Since clk_get_bulk() only looks for devices with UCLASS_CLK, it fails with -ENODEV and then ehci_usb_probe() aborts. The consequence is peripherals connected to a USB 2 port (e.g. in a Rock Pi 4 the white port, nearer the edge) not being detected. They're detected if CONFIG_USB_OHCI_GENERIC is selected in Kconfig, because ohci_usb_probe() does not abort when one clk_get_by_index() fails, but then they work in USB 1 mode,. rk3399.dtsi comes from linux and the u2phy0 was added[1] to the clock list in: commit b5d1c57299734f5b54035ef2e61706b83041f20c Author: William wu Date: Wed Dec 21 18:41:05 2016 +0800 arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399 We found that the suspend process was blocked when it run into ehci/ohci module due to clk-480m of usb2-phy was disabled. [...] Suspend concerns don't apply to U-Boot, and the problem with U-Boot failing to probe EHCI doesn't apply to linux, because in linux rockchip_usb2phy_clk480m_register makes u2phy0 a proper clock provider when called by rockchip_usb2phy_probe(). So I can think of a few alternative solutions: 1- Change ehci_usb_probe() to make it more similar to ohci_usb_probe(), and survive failure to get one clock. Looks a little harder, and I don't know whether it could break something if it ignored a clock that was important for something else than suspend. 2- Change rk3399.dtsi effecttively reverting the linux commit b5d1c57299734f5b54035ef2e61706b83041f20c. This dealigns the .dtsi from linux and seems fragile at the next synchronisation. 3- Change the clock list in rk3399-u-boot.dtsi or somewhere else. This survives .dts* sync but may survive "too much" and miss some change from linux that we might want. 4- Enable CONFIG_USB_OHCI_GENERIC and use the ports in USB 1 mode. This would need to be made for all boards using rk3399. In a simple test reading one file from USB storage it gave 769.5 KiB/s instead of 20.5 MiB/s with solution 2. 5- Trying to replicate linux and have usb2phy somehow provide a clk, or have a separate clock device for usb2phy in addition to the phy device. I just can't seem to imagine how to achieve this with the U-Boot driver model, maybe because of my limited familiarity with it. This patch is option 1 as Kever Yang requested on August 27th[2] when option 3 didn't get through. Sorry for the delay. Yet maybe the get_some_clks() function should be added to clk-uclass.c if it may be useful elsewhere. Or alternatively, clk_get_bulk() could be changed to the lenient behaviour of get_some_clks(), but that seems too invasive to me. Either of these changes can always be done in a later patch if needed. If so, one possibility would be to call it from ohci-generic.c. As it is now it looks like if it ever misses a clock but finds a subsequent clock, assigned to a higher index in the clock table, it may leave clock_count too low to release all found clocks. I don't know of a case where this happens, for rk3399 usb, even with non-default CONFIG_OHCI_GENERIC, the missing clock is the last one, and so the release iteration happens to find all found clocks. Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/ [2] https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut/#2954536 Cc: Simon Glass Cc: Philipp Tomsich Cc: Kever Yang Cc: Lukasz Majewski Cc: Sean Anderson Cc: Marek Vasut Signed-off-by: Xavier Drudis Ferran --- Changes: v2: implement option 1 (ehci-generic.c tolerates missing clocks) instead of option 3 (change dts node to remove the missing clock). --- drivers/usb/host/ehci-generic.c | 59 ++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c index a765a307a3..aa86dcc435 100644 --- a/drivers/usb/host/ehci-generic.c +++ b/drivers/usb/host/ehci-generic.c @@ -60,6 +60,63 @@ static int ehci_disable_vbus_supply(struct generic_ehci *priv) return 0; } +/** + * get_some_clks() - Get/request all available clocks of a + * device. Leave other as null. + * + * @dev: The client device. + * @bulk: A pointer to a clock bulk struct to initialize. + * + * This looks up and requests all clocks of the client device; each + * device is assumed to have n clocks associated with it somehow, and + * this function tries to find and request all of them in a separate + * structure. The mapping of client device clock indices to provider + * clocks may be via device-tree properties, board-provided mapping + * tables, or some other mechanism. + * + * This is like clk_get_bulk() in clk uclass, but failure to get some + * of the clocks is accepted and only the available ones are assigned + * to bulk->clks. So bulk->clks may contain invalid (zeroed) clk + * structs. clk_release_bulk(), clk_enable_bulk() and + * clk_disable_bulk() can deal with that. + * + * bulk->count will contain the number of attempted clocks (size of + * bulk->clks). Or an error when getting the clocks phandle if + * negative. + * + * Return: If some clocks could be successfully located and requested, + * it returns 0. If all failed, then returns the last error code. + */ +static int get_some_clks(struct udevice *dev, struct clk_bulk *bulk) +{ + int i, ret, count; + + count = 0; + + bulk->count = dev_count_phandle_with_args(dev, "clocks", "#clock-cells", 0); + if (bulk->count < 1) + return bulk->count; + + bulk->clks = devm_kcalloc(dev, bulk->count, sizeof(struct clk), GFP_KERNEL); + if (!bulk->clks) + return -ENOMEM; + + for (i = 0; i < bulk->count; i++) { + ret = clk_get_by_index(dev, i, &bulk->clks[i]); + if (ret < 0) { + dev_warn(dev, "Failed to get clock %i/%i (ret=%d)\n", i, bulk->count, ret); + break; + } + + ++count; + } + + if (!count) + return ret; + + return 0; +} + static int ehci_usb_probe(struct udevice *dev) { struct generic_ehci *priv = dev_get_priv(dev); @@ -68,7 +125,7 @@ static int ehci_usb_probe(struct udevice *dev) int err, ret; err = 0; - ret = clk_get_bulk(dev, &priv->clocks); + ret = get_some_clks(dev, &priv->clocks); if (ret && ret != -ENOENT) { dev_err(dev, "Failed to get clocks (ret=%d)\n", ret); return ret; -- 2.20.1