[PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup

Marek Vasut marex at denx.de
Wed Aug 23 13:46:22 CEST 2023


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


More information about the U-Boot mailing list