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 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?

> 
>>
>>>
>>>>
>>>>>  {
>>>>>  	int rc;
>>>>>  	struct dwc3_generic_plat *plat = dev_get_plat(dev);
>>>>> @@ -62,7 +63,19 @@ static int dwc3_generic_probe(struct udevice *dev,
>>>>>  
>>>>>  	dwc3->dev = dev;
>>>>>  	dwc3->maximum_speed = plat->maximum_speed;
>>>>> -	dwc3->dr_mode = plat->dr_mode;
>>>>> +
>>>>> +	/*
>>>>> +	 * If the controller supports OTG as indicated by plat->dr_mode,
>>>>> +	 * then either Host or Peripheral mode is acceptable.
>>>>> +	 * Otherwise, error out since the platform cannot support the mode
>>>>> +	 * being requested by the caller of dwc3_generic_probe().
>>>>> +	 */
>>>>> +	if (plat->dr_mode != mode && plat->dr_mode != USB_DR_MODE_OTG) {
>>>>> +		pr_err("Requested usb mode is not supported by platform\n");
>>>>
>>>> This is actually not necessary. Sorry for suggesting this approach before.
>>>
>>> While the check may not be necessary, dr_mode *should* be set based on
>>> the caller and not based on plat->dr_mode. The reason I say so is that
>>> plat->dr_mode could be "otg" in the device-tree. But "otg" shouldn't be
>>> used for further configuration. I had indicated the reason for this in
>>> the cover-letter, which I am pasting below for your reference:
>>>
>>> ---------------------------------------------------------------------------
>>> Currently, dwc3_generic_probe() sets the mode based on the device-tree
>>> rather than setting it on the basis of the caller. While this might be
>>> correct when the mode is "host" or "peripheral" in the device-tree, it is
>>> wrong when the mode is "otg" in the device-tree. It is wrong because of two
>>> reasons:
>>> 1. There is no OTG state machine in U-Boot. Hence the role will never
>>> switch to the correct one eventually among host or peripheral.
>>> 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".

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.

-- 
cheers,
-roger