* [PATCH v1 0/1] tegra-usb: support dts based xcvr setup @ 2023-08-19 15:06 Svyatoslav Ryhel 2023-08-19 15:06 ` [PATCH v1 1/1] usb: host: tegra: implement " Svyatoslav Ryhel 0 siblings, 1 reply; 13+ messages in thread From: Svyatoslav Ryhel @ 2023-08-19 15:06 UTC (permalink / raw) To: Marek Vasut, Simon Glass, Svyatoslav Ryhel; +Cc: u-boot This patch makes tegra usb driver a bit more flexible by passing xcvr setup values from device tree. In most cases default setup of USB works perfectly fine, but for some devices (like ASUS TF201) fuses used for xcvr setup are quite different which brakes some usb lines (in TF201 case dock USB port). To fix this xcvr-setup value can be passed from device tree. Svyatoslav Ryhel (1): usb: host: tegra: implement dts based xcvr setup drivers/usb/host/ehci-tegra.c | 53 +++++++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 5 deletions(-) -- 2.39.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-19 15:06 [PATCH v1 0/1] tegra-usb: support dts based xcvr setup Svyatoslav Ryhel @ 2023-08-19 15:06 ` Svyatoslav Ryhel 2023-08-20 2:23 ` Marek Vasut 0 siblings, 1 reply; 13+ messages in thread From: Svyatoslav Ryhel @ 2023-08-19 15:06 UTC (permalink / raw) To: Marek Vasut, Simon Glass, Svyatoslav Ryhel; +Cc: u-boot 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 <clamor95@gmail.com> # ASUS TF201 Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> --- 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); + 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; + } + + priv->xcvr_setup_use_fuses = ofnode_read_bool( + usb_phy_node, "nvidia,xcvr-setup-use-fuses"); + + if (!priv->xcvr_setup_use_fuses) { + ret = ofnode_read_u32(usb_phy_node, "nvidia,xcvr-setup", + &priv->xcvr_setup); + if (ret) + return ret; + } + + log_debug("%s: xcvr_setup_use_fuses %d, xcvr_setup %d\n", + __func__, priv->xcvr_setup_use_fuses, priv->xcvr_setup); + return 0; } -- 2.39.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-19 15:06 ` [PATCH v1 1/1] usb: host: tegra: implement " Svyatoslav Ryhel @ 2023-08-20 2:23 ` Marek Vasut 2023-08-20 7:13 ` Svyatoslav Ryhel 0 siblings, 1 reply; 13+ messages in thread From: Marek Vasut @ 2023-08-20 2:23 UTC (permalink / raw) To: Svyatoslav Ryhel, Simon Glass; +Cc: u-boot 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 <clamor95@gmail.com> # 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 <clamor95@gmail.com> > --- > 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 ? Why not place this value into config->xcvr_setup in e.g. probe() and drop this if/else statement ? > + 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 > + priv->xcvr_setup_use_fuses = ofnode_read_bool( > + usb_phy_node, "nvidia,xcvr-setup-use-fuses"); dev_read_bool() > + if (!priv->xcvr_setup_use_fuses) { > + ret = ofnode_read_u32(usb_phy_node, "nvidia,xcvr-setup", > + &priv->xcvr_setup); > + if (ret) > + return ret; > + } > + > + log_debug("%s: xcvr_setup_use_fuses %d, xcvr_setup %d\n", > + __func__, priv->xcvr_setup_use_fuses, priv->xcvr_setup); Is xcvr_setup always initialized, e.g. in case priv->xcvr_setup_use_fuses=true ? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-20 2:23 ` Marek Vasut @ 2023-08-20 7:13 ` Svyatoslav Ryhel 2023-08-20 18:14 ` Marek Vasut 0 siblings, 1 reply; 13+ messages in thread From: Svyatoslav Ryhel @ 2023-08-20 7:13 UTC (permalink / raw) To: Marek Vasut, Simon Glass; +Cc: u-boot 20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >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 <clamor95@gmail.com> # 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 <clamor95@gmail.com> >> --- >> 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 >> + 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. >> + if (!priv->xcvr_setup_use_fuses) { >> + ret = ofnode_read_u32(usb_phy_node, "nvidia,xcvr-setup", >> + &priv->xcvr_setup); >> + if (ret) >> + return ret; >> + } >> + >> + log_debug("%s: xcvr_setup_use_fuses %d, xcvr_setup %d\n", >> + __func__, priv->xcvr_setup_use_fuses, priv->xcvr_setup); > >Is xcvr_setup always initialized, e.g. in case priv->xcvr_setup_use_fuses=true ? Yes, xcvr_setup is always initialized, unless the device needs it to be disabled. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-20 7:13 ` Svyatoslav Ryhel @ 2023-08-20 18:14 ` Marek Vasut 2023-08-20 18:32 ` Svyatoslav Ryhel 0 siblings, 1 reply; 13+ messages in thread From: Marek Vasut @ 2023-08-20 18:14 UTC (permalink / raw) To: Svyatoslav Ryhel, Simon Glass; +Cc: u-boot On 8/20/23 09:13, Svyatoslav Ryhel wrote: > 20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >> 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 <clamor95@gmail.com> # 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 <clamor95@gmail.com> >>> --- >>> 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 ? >>> + 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 ? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-20 18:14 ` Marek Vasut @ 2023-08-20 18:32 ` Svyatoslav Ryhel 2023-08-20 19:10 ` Marek Vasut 0 siblings, 1 reply; 13+ messages in thread From: Svyatoslav Ryhel @ 2023-08-20 18:32 UTC (permalink / raw) To: Marek Vasut, Simon Glass; +Cc: u-boot 20 серпня 2023 р. 21:14:15 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >On 8/20/23 09:13, Svyatoslav Ryhel wrote: >> 20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >>> 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 <clamor95@gmail.com> # 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 <clamor95@gmail.com> >>>> --- >>>> 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. >>>> + 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-20 18:32 ` Svyatoslav Ryhel @ 2023-08-20 19:10 ` Marek Vasut 2023-08-20 19:23 ` Svyatoslav Ryhel 2023-08-23 10:49 ` Thierry Reding 0 siblings, 2 replies; 13+ messages in thread From: Marek Vasut @ 2023-08-20 19:10 UTC (permalink / raw) To: Svyatoslav Ryhel, Simon Glass; +Cc: u-boot On 8/20/23 20:32, Svyatoslav Ryhel wrote: > 20 серпня 2023 р. 21:14:15 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >> On 8/20/23 09:13, Svyatoslav Ryhel wrote: >>> 20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >>>> 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 <clamor95@gmail.com> # 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 <clamor95@gmail.com> >>>>> --- >>>>> 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 ? Let's not add ad-hoc hacks, those are difficult to maintain in the long run. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-20 19:10 ` Marek Vasut @ 2023-08-20 19:23 ` Svyatoslav Ryhel 2023-08-20 20:48 ` Marek Vasut 2023-08-23 10:49 ` Thierry Reding 1 sibling, 1 reply; 13+ messages in thread From: Svyatoslav Ryhel @ 2023-08-20 19:23 UTC (permalink / raw) To: Marek Vasut, Simon Glass; +Cc: u-boot 20 серпня 2023 р. 22:10:17 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >On 8/20/23 20:32, Svyatoslav Ryhel wrote: >> 20 серпня 2023 р. 21:14:15 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >>> On 8/20/23 09:13, Svyatoslav Ryhel wrote: >>>> 20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >>>>> 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 <clamor95@gmail.com> # 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 <clamor95@gmail.com> >>>>>> --- >>>>>> 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 ? You are correct, I will remove those previous stuff. >>>>>> + 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 ? > >Let's not add ad-hoc hacks, those are difficult to maintain in the long run. Nope, no intention to do it. Hense I assume this patch can be dropped. Best regards, Svyatoslav R. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-20 19:23 ` Svyatoslav Ryhel @ 2023-08-20 20:48 ` Marek Vasut 0 siblings, 0 replies; 13+ messages in thread From: Marek Vasut @ 2023-08-20 20:48 UTC (permalink / raw) To: Svyatoslav Ryhel, Simon Glass; +Cc: u-boot On 8/20/23 21:23, Svyatoslav Ryhel wrote: > > > 20 серпня 2023 р. 22:10:17 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >> On 8/20/23 20:32, Svyatoslav Ryhel wrote: >>> 20 серпня 2023 р. 21:14:15 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >>>> On 8/20/23 09:13, Svyatoslav Ryhel wrote: >>>>> 20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >>>>>> 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 <clamor95@gmail.com> # 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 <clamor95@gmail.com> >>>>>>> --- >>>>>>> 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 ? > > You are correct, I will remove those previous stuff. > >>>>>>> + 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 ? >> >> Let's not add ad-hoc hacks, those are difficult to maintain in the long run. > > Nope, no intention to do it. Hense I assume this patch can be dropped. Right, using DM PHY driver for controlling the PHY seems to be the right approach. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-20 19:10 ` Marek Vasut 2023-08-20 19:23 ` Svyatoslav Ryhel @ 2023-08-23 10:49 ` Thierry Reding 2023-08-23 11:28 ` Svyatoslav Ryhel ` (2 more replies) 1 sibling, 3 replies; 13+ messages in thread From: Thierry Reding @ 2023-08-23 10:49 UTC (permalink / raw) To: Marek Vasut; +Cc: Svyatoslav Ryhel, Simon Glass, u-boot [-- Attachment #1: Type: text/plain, Size: 8564 bytes --] 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 <marex@denx.de> написав(-ла): > > > On 8/20/23 09:13, Svyatoslav Ryhel wrote: > > > > 20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): > > > > > 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 <clamor95@gmail.com> # 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 <clamor95@gmail.com> > > > > > > --- > > > > > > 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. 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. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-23 10:49 ` Thierry Reding @ 2023-08-23 11:28 ` Svyatoslav Ryhel 2023-08-23 11:46 ` Marek Vasut 2023-08-25 17:34 ` Svyatoslav Ryhel 2 siblings, 0 replies; 13+ messages in thread From: Svyatoslav Ryhel @ 2023-08-23 11:28 UTC (permalink / raw) To: Thierry Reding, Marek Vasut; +Cc: Simon Glass, u-boot 23 серпня 2023 р. 13:49:18 GMT+03:00, Thierry Reding <thierry.reding@gmail.com> написав(-ла): >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 <marex@denx.de> написав(-ла): >> > > On 8/20/23 09:13, Svyatoslav Ryhel wrote: >> > > > 20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >> > > > > 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 <clamor95@gmail.com> # 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 <clamor95@gmail.com> >> > > > > > --- >> > > > > > 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. > >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. PHY node can be parsed by controller for values used in setup without creating separate driver. I would even agree to implement this. >Thierry ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-23 10:49 ` Thierry Reding 2023-08-23 11:28 ` Svyatoslav Ryhel @ 2023-08-23 11:46 ` Marek Vasut 2023-08-25 17:34 ` Svyatoslav Ryhel 2 siblings, 0 replies; 13+ messages in thread From: Marek Vasut @ 2023-08-23 11:46 UTC (permalink / raw) To: Thierry Reding; +Cc: Svyatoslav Ryhel, Simon Glass, u-boot 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 <marex@denx.de> написав(-ла): >>>> On 8/20/23 09:13, Svyatoslav Ryhel wrote: >>>>> 20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >>>>>> 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 <clamor95@gmail.com> # 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 <clamor95@gmail.com> >>>>>>> --- >>>>>>> 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup 2023-08-23 10:49 ` Thierry Reding 2023-08-23 11:28 ` Svyatoslav Ryhel 2023-08-23 11:46 ` Marek Vasut @ 2023-08-25 17:34 ` Svyatoslav Ryhel 2 siblings, 0 replies; 13+ messages in thread From: Svyatoslav Ryhel @ 2023-08-25 17:34 UTC (permalink / raw) To: Thierry Reding, Marek Vasut; +Cc: Simon Glass, u-boot ср, 23 серп. 2023 р. о 13:49 Thierry Reding <thierry.reding@gmail.com> пише: > > 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 <marex@denx.de> написав(-ла): > > > > On 8/20/23 09:13, Svyatoslav Ryhel wrote: > > > > > 20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): > > > > > > 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 <clamor95@gmail.com> # 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 <clamor95@gmail.com> > > > > > > > --- > > > > > > > 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. > > 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. Marek, sorry but at this point I am with Thierry, this stuff will be more painful to maintain as 2 separate parts then as it is now. Moreover, it will not provide any benefit, ony issues like waste time to split, maintain 2 files and literally no code reusability or reduction. I have made a rough sketch of how phy node parsing may look, but I cannot load it here as a patch because it requires first that follow up patches related to boards I have sent to be merged (yes, hard to explain, Thierry should know). You may check commit in my personal fork here https://github.com/clamor-s/u-boot/commit/c8b896192ab9ce3dd578cf224e2c81c8b4049ea1 > Thierry ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-08-25 17:35 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-19 15:06 [PATCH v1 0/1] tegra-usb: support dts based xcvr setup Svyatoslav Ryhel 2023-08-19 15:06 ` [PATCH v1 1/1] usb: host: tegra: implement " Svyatoslav Ryhel 2023-08-20 2:23 ` Marek Vasut 2023-08-20 7:13 ` Svyatoslav Ryhel 2023-08-20 18:14 ` Marek Vasut 2023-08-20 18:32 ` Svyatoslav Ryhel 2023-08-20 19:10 ` Marek Vasut 2023-08-20 19:23 ` Svyatoslav Ryhel 2023-08-20 20:48 ` Marek Vasut 2023-08-23 10:49 ` Thierry Reding 2023-08-23 11:28 ` Svyatoslav Ryhel 2023-08-23 11:46 ` Marek Vasut 2023-08-25 17:34 ` Svyatoslav Ryhel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox