[PATCH v1 1/1] usb: host: tegra: implement dts based xcvr setup
Svyatoslav Ryhel
clamor95 at gmail.com
Sun Aug 20 20:32:34 CEST 2023
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.
>>>> + 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.
More information about the U-Boot
mailing list