On 8/23/23 12:49, Thierry Reding wrote:
On Sun, Aug 20, 2023 at 09:10:17PM +0200, Marek Vasut wrote:
On 8/20/23 20:32, Svyatoslav Ryhel wrote:
20 серпня 2023 р. 21:14:15 GMT+03:00, Marek Vasut <[email protected]> написав(-ла):
On 8/20/23 09:13, Svyatoslav Ryhel wrote:
20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <[email protected]> написав(-ла):
On 8/19/23 17:06, Svyatoslav Ryhel wrote:
Some devices (like ASUS TF201) may not have fuse based xcvr setup
which will result in not working USB line. To fix this situation
allow xcvr setup to be performed from device tree values if
nvidia,xcvr-setup-use-fuses is not defined.
Tested-by: Svyatoslav Ryhel <[email protected]> # ASUS TF201
I would hope so. Usually T-B tags are not added by the patch author, but that's
a detail, and here it has the added model value, so keep it.
Signed-off-by: Svyatoslav Ryhel <[email protected]>
---
drivers/usb/host/ehci-tegra.c | 53 +++++++++++++++++++++++++++++++----
1 file changed, 48 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 2cf1625670..f24baf8f0c 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -81,6 +81,8 @@ struct fdt_usb {
enum periph_id periph_id;/* peripheral id */
struct gpio_desc vbus_gpio; /* GPIO for vbus enable */
struct gpio_desc phy_reset_gpio; /* GPIO to reset ULPI phy */
+ bool xcvr_setup_use_fuses; /* Indicates that the value is read from the
on-chip fuses */
+ u32 xcvr_setup; /* Input of XCVR cell, HS driver output control */
};
/*
@@ -464,10 +466,21 @@ static int init_utmi_usb_controller(struct fdt_usb
*config,
/* Recommended PHY settings for EYE diagram */
val = readl(&usbctlr->utmip_xcvr_cfg0);
- clrsetbits_le32(&val, UTMIP_XCVR_SETUP_MASK,
- 0x4 << UTMIP_XCVR_SETUP_SHIFT);
- clrsetbits_le32(&val, UTMIP_XCVR_SETUP_MSB_MASK,
- 0x3 << UTMIP_XCVR_SETUP_MSB_SHIFT);
+
+ if (!config->xcvr_setup_use_fuses) {
+ clrsetbits_le32(&val, UTMIP_XCVR_SETUP_MASK,
+ config->xcvr_setup <<
+ UTMIP_XCVR_SETUP_SHIFT);
+ clrsetbits_le32(&val, UTMIP_XCVR_SETUP_MSB_MASK,
+ config->xcvr_setup <<
+ UTMIP_XCVR_SETUP_MSB_SHIFT);
+ } else {
+ clrsetbits_le32(&val, UTMIP_XCVR_SETUP_MASK,
+ 0x4 << UTMIP_XCVR_SETUP_SHIFT);
What is this hard-coded value ?
0x4 and 0x3 (not same) but those are not in the device tree. Mainline linux
driver seems not set this at all if use_fuses is enabled but I decided to keep
original setup just to not cause regressions.
https://github.com/clamor-s/linux/blob/transformer/drivers/usb/phy/phy-tegra-usb.c#L587-L590
Why not place this value into config->xcvr_setup in e.g. probe() and drop this
if/else statement ?
Because config->xcvr_setup should matter only if use_fuses is disabled
Can it matter always instead ?
No, it cannot. You are inversing hw design. Xcvr_setup matters only if
use_fuses is disabled. If use_fuses is on (which is default state) xcvr values
are taken from chip fuse.
The way I read this block of code is, some value is written into the
register if config->xcvr_setup_use_fuses is false, another value if
config->xcvr_setup_use_fuses is true . Why not do this determination once in
probe and then just program the appropriate value instead ?
+ clrsetbits_le32(&val, UTMIP_XCVR_SETUP_MSB_MASK,
+ 0x3 << UTMIP_XCVR_SETUP_MSB_SHIFT);
+ }
+
clrsetbits_le32(&val, UTMIP_XCVR_HSSLEW_MSB_MASK,
0x8 << UTMIP_XCVR_HSSLEW_MSB_SHIFT);
writel(val, &usbctlr->utmip_xcvr_cfg0);
@@ -522,7 +535,9 @@ static int init_utmi_usb_controller(struct fdt_usb *config,
setbits_le32(&usbctlr->utmip_bat_chrg_cfg0, UTMIP_PD_CHRG);
clrbits_le32(&usbctlr->utmip_xcvr_cfg0, UTMIP_XCVR_LSBIAS_SE);
- setbits_le32(&usbctlr->utmip_spare_cfg0, FUSE_SETUP_SEL);
+
+ if (config->xcvr_setup_use_fuses)
+ setbits_le32(&usbctlr->utmip_spare_cfg0, FUSE_SETUP_SEL);
/*
* Configure the UTMIP_IDLE_WAIT and UTMIP_ELASTIC_LIMIT
@@ -843,6 +858,8 @@ static const struct ehci_ops tegra_ehci_ops = {
static int ehci_usb_of_to_plat(struct udevice *dev)
{
struct fdt_usb *priv = dev_get_priv(dev);
+ u32 usb_phy_phandle;
+ ofnode usb_phy_node;
int ret;
ret = fdt_decode_usb(dev, priv);
@@ -851,6 +868,32 @@ static int ehci_usb_of_to_plat(struct udevice *dev)
priv->type = dev_get_driver_data(dev);
+ ret = ofnode_read_u32(dev_ofnode(dev), "nvidia,phy", &usb_phy_phandle);
+ if (ret) {
+ log_debug("%s: required usb phy node isn't provided\n", __func__);
+ priv->xcvr_setup_use_fuses = true;
+ return 0;
+ }
+
+ usb_phy_node = ofnode_get_by_phandle(usb_phy_phandle);
+ if (!ofnode_valid(usb_phy_node)) {
+ log_err("%s: failed to find usb phy node\n", __func__);
+ return -EINVAL;
+ }
dev_read_phandle() should replace the above
Goal of this piece is to get phy of_node by the phandle provided in the
controller node. Controller node has not only single nvidia,phy phandle
reference but also clock and reset reference. How will dev_read_phandle
return me a phandle of "nvidia,phy"?
https://github.com/clamor-s/u-boot/blob/master/arch/arm/dts/tegra30.dtsi#L798-L834
+ priv->xcvr_setup_use_fuses = ofnode_read_bool(
+ usb_phy_node, "nvidia,xcvr-setup-use-fuses");
dev_read_bool()
This value is set in phy node, not controllers unfortunately.
The question that comes to mind is, would it make sense to implement a PHY
driver similar to what e.g. imx does -- drivers/phy/phy-imx8mq-usb.c -- for the
tegra PHY ?
Yes, but not by me or at least not now. I have already invested too much of my
time and effort in this and I will not invest even more into refactoring all
this code into 2 separate drivers. Existing code satisfies me apart from this
small tweak.
The PHY driver implementation is trivial, example is the imx driver above,
then just call phy on/off in the right place. Linux also has a PHY driver
for tegra, maybe you can reuse it ?
The Linux USB PHY driver is something that in retrospect I would've done
differently. The problem is that it shares resources (registers and
clock/reset lines) with the USB controller and it's caused a lot of
headaches to support it in Linux.
Clock and reset are not a problem, since they are abstracted by clock
and reset subsystems respectively. Shared registers can be handled by
syscon too. As far as I can tell, the chipidea HDRC core should be well
isolated, wherever I came in contact with it, it was. Which shared
registers are you talking about ?
Maybe within U-Boot this isn't such a big deal, but for Linux I would've
preferred a single driver that is both a USB controller and a USB PHY. I
suppose it could expose some sort of PHY object for abstraction, but
that would probably be a bit of overkill since this is really only used
within the USB controller driver.
This isn't about providing a PHY abstraction to the outside, but about
keeping the CI HDRC control code (which is basically EHCI-HCD) separate
from multiple disparate PHYs which are separate IPs.
