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

[Svyatoslav Ryhel]

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

>> +                    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.