Re: [PATCH v2 1/2] usb: dwc3-generic: set "mode" based on caller of dwc3_generic_probe()

[Roger Quadros <rogerq@kernel.org>]

On 04/12/2024 12:05, Siddharth Vadapalli wrote:
> On Wed, Dec 04, 2024 at 11:57:14AM +0200, Roger Quadros wrote:
>>
>>
>> On 04/12/2024 09:47, Siddharth Vadapalli wrote:
>>> On Wed, Dec 04, 2024 at 09:32:23AM +0200, Roger Quadros wrote:
>>>>
>>>>
>>>> On 04/12/2024 07:16, Siddharth Vadapalli wrote:
>>>>> On Tue, Dec 03, 2024 at 09:23:11PM +0200, Roger Quadros wrote:
>>>
>>> [...]
>>>
>>>>>>> +++ b/drivers/usb/dwc3/dwc3-generic.c
>>>>>>> @@ -51,7 +51,8 @@ struct dwc3_generic_host_priv {
>>>>>>>  };
>>>>>>>  
>>>>>>>  static int dwc3_generic_probe(struct udevice *dev,
>>>>>>> -			      struct dwc3_generic_priv *priv)
>>>>>>> +			      struct dwc3_generic_priv *priv,
>>>>>>> +			      enum usb_dr_mode mode)
>>>>>>
>>>>>> This is not necessary as dwc3_generic_host_probe will only be
>>>>>> registered if dr_mode == "host" and dwc3_generic_peripheral_probe
>>>>>> will only be registered if dr_mode == "host" or "peripheral"
>>>>>
>>>>> But I don't see it happening that way. dwc3_generic_host_probe() or
>>>>> dwc3_generic_peripheral_probe() seem to be invoked based on the command
>>>>> executed - USB Host command will trigger generic_host_probe() while USB
>>>>> Device/Gadget command will trigger generic_peripheral_probe().
>>>>>
>>>>
>>>> But, when dr_mode is "otg" generic host driver is *not* bound to the
>>>> DWC3 device. So dwc3_generic_host_probe() will never be called for it.
>>>
>>> In USB DFU boot, "dwc3_generic_peripheral_boot()" is invoked. Please
>>> test it out on AM625-SK. I was able to observe this on AM62A7-SK.
>>
>> We do want dwc3_generic_peripheral_boot() to be invoked with dr_mode is "otg".
>> As we still do not support dual-role switching in u-boot.
>>
>> And you need that for DFU as well. Did I miss something?
> 
> You were mentioning that "dwc3_generic_host_probe()" will not be called,
> but that is unrelated to the issue. I was pointing out that
> "dwc3_generic_peripheral_probe()" is invoked, with plat->dr_mode set to
> "otg" which currently results in PRTCAPDIR set to 11b which results in
> the issue. So the patch aims to address this by setting mode based on
> caller rather than using what is mentioned in the device-tree. This
> should be in sync when "dr_mode" is either "host"/"peripheral", but in
> the case of "otg", we run into issues which is fixed by this patch.
> 
> [...]
> 
>>>>> 2. dr_mode = "otg" results in the "PRTCAPDIR" field of the "GCTL"
>>>>> register of the USB Controller being set to 11b. According to the
>>>>> datasheet of the Designware USB Dual Role Controller, "PRTCAPDIR"
>>>>> should never be set to any value other than 01b (Host) and 10b (Device).
>>>>> Quoting the datasheet:
>>>>> "Programming this field with random data causes the controller
>>>>> to keep toggling between the host mode and the device mode."
>>>>>
>>>>> Therefore, in order to avoid programming 11b in "PRTCAPDIR", and, given
>>>>> that the caller specifies the intended role, rather than simply using
>>>>> the "dr_mode" property to set the role, set the role on the basis of the
>>>>> caller (intended role) and the device-tree (platform support).
>>>>> ----------------------------------------------------------------------------
>>>>>
>>>>> Point #2 above is the main reason why dr_mode cannot be set to "otg" in
>>>>> dwc3_generic_probe(). The only difference now is that the check can be
>>>>> dropped as you have indicated, so the last paragraph above will change
>>>>
>>>> Finally we are at the root of the problem. But the solution is not to
>>>> change the dr_mode but instead handle it correctly in dwc3_core_init_mode()
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index a35b8c2f646..7f42b6e62c6 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -752,13 +752,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>>>  		}
>>>>  		break;
>>>>  	case USB_DR_MODE_OTG:
>>>> -		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
>>>> -		ret = dwc3_host_init(dwc);
>>>> -		if (ret) {
>>>> -			dev_err(dwc->dev, "failed to initialize host\n");
>>>> -			return ret;
>>>> -		}
>>>> -
>>>> +		/* We don't support dual-role so restrict to Device mode */
>>>> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>>>
>>> But is this acceptable? Why not default to Host? I went through a series
>>
>> I will let others chime in if it is acceptable or not.
>>
>>> Why not default to host? 
>>
>> Because we don't bind the generic_host driver at all when dr_mode = "otg".
>> So XHCI driver will not be probed and host mode will not function.
>>
>>> I went through a series
>>> for defaulting OTG to Device for the Cadence Controller which was
>>> objected at:
>>> https://lore.kernel.org/r/62c5fe13-7f0e-e736-273e-31a8bbddbf13@kernel.org/
>>> with the suggestion to use the cable to determine the role.
>>>
>>
>> dual-role is a new feature for this driver in u-boot which can be added
>> in the future if needed.
>> The current problem is to get DFU/device mode to work when dr_mode is "otg".
>>
>> As the dwc3-generic driver only registers the device controller when
>> dr_mode is "otg" I don't see any other neater way to fix it than to
>> limit the controller to device mode if dr_mode is "otg".
> 
> Why not do what this patch does? What is the issue with the current
> patch (apart from the checks, which I could retain as well if others
> wish so)? The caller clearly mentions the expected role, so why not use
> that?

Because the patch is more complicated than it needs to be, now that we know:
1) generic_host_probe() will only be called when dr_mode is "host",
2) generic_peripheral_probe() will only be called when dr_mode is "peripheral"
or "otg".

The problem case is generic_peripheral_probe() being called with when
dr_mode is "otg".

Another alternative could be to override dr_mode to "peripheral" if
it was "otg" in dwc3_generic_probe before calling dwc3_init() but
I don't prefer that as we are loosing platform information here
and it will have to be reverted when dual-role support is added in core.c.

So better to deal with this in core.c and add a note why we are doing it.

> 
>>
>> The only only other driver calling dwc3_init() is dwc3-layerscape.c. whose
>> commit log states
>> "OTG mode is not supported yet. The dr_mode in the devicetree will either
>>  have to be set to peripheral or host."
>>
>> Or you can go all the way and get the dual-role mode working the right way
>> where you check user request along with cable status and allow/prevent a
>> certain role.
> 
> The current patch seems to be an intermediate solution (actually, it
> might be what should have been done when the code was first introduced
> itself. Why use plat->dr_mode at all, except maybe to check if the
> platform supports what is requested by the caller?)
> 
> Regards,
> Siddharth.

-- 
cheers,
-roger