ср, 23 серп. 2023 р. о 13:49 Thierry Reding <thierry.red...@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 <ma...@denx.de> > > > написав(-ла): > > > > On 8/20/23 09:13, Svyatoslav Ryhel wrote: > > > > > 20 серпня 2023 р. 05:23:14 GMT+03:00, Marek Vasut <ma...@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 <clamo...@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 <clamo...@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