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

[Roger Quadros <rogerq@kernel.org>]

On 02/12/2024 07:42, Siddharth Vadapalli wrote:
> On Fri, Nov 29, 2024 at 02:28:36PM +0200, Roger Quadros wrote:
>>
>>
>> On 28/11/2024 19:20, Siddharth Vadapalli wrote:
> 
> [...]
> 
>>> USB DFU boot utilizes this driver when the "dr_mode" is set to "otg" in
>>> the device-tree for AM62A. This driver is invoked both for the host and
>>> device commands corresponding to USB. I came up with this patch while
>>> trying to enable USB DFU boot on AM62A with "dr_mode" as "otg" in the
>>> device-tree. Additionally, USB DFU boot works on AM62A with the change
>>> made by this patch and it doesn't work without this change. I have
>>> traced the call sequence to see that the driver is probed and have come
>>> up with the change performed in this patch on the basis of this observation.
>>>
>>
>> Although the glue driver portion of the dwc3-generic driver is not used by AM62
>> it still uses the generic_probe() portion via
>> U_BOOT_DRIVER(dwc3_generic_host) and U_BOOT_DRIVER(dwc3_generic_peripheral)
>>
>> here is the dm tree output on am62-sk after executing "usb start" command.
>>
>>
>>  simple_bus    5  [ + ]   dwc3-am62             |   |-- dwc3-usb@f900000
>>  usb_gadget    0  [   ]   dwc3-generic-periphe  |   |   |-- usb@31000000
>>  usb           0  [ + ]   xhci-dwc3             |   |   `-- usb@31000000
>>  usb_hub       0  [ + ]   usb_hub               |   |       `-- usb_hub
>>  simple_bus    6  [ + ]   dwc3-am62             |   |-- dwc3-usb@f910000
>>  usb           1  [   ]   dwc3-generic-host     |   |   |-- usb@31100000
>>  usb           1  [ + ]   xhci-dwc3             |   |   `-- usb@31100000
>>  usb_hub       1  [ + ]   usb_hub               |   |       `-- usb_hub
>>
>> what is strange is that even though usb@31000000 is configured as
>> "peripheral" (in k3-am625-sk-u-boot.dtsi), xhci-dwc3 and usb_hub drivers
>> are still initialized for it.
> 
> CONFIG_OF_UPSTREAM is enabled in am62x_evm_a53_defconfig. So dr_mode
> might be "otg" as specified in:
> dts/upstream/src/arm64/ti/k3-am62-main.dtsi

Not it is not "otg". it is "peripheral". I verified from the generated 
u-boot.dtb

Isn't k3-am625-sk-u-boot.dtsi still used even if it is CONFIG_OF_UPSTREAM?

> I'm not sure about this however. Additionally, this series isn't required
> for AM62. DFU Boot works on AM62 without this series as well. This series
> is required for AM62A.

It works on am62-sk because USB mode is forced to "peripheral" in
k3-am625-sk-u-boot.dtsi.

That hack can be removed once your series fixes it the right way.

> 
> [...]
> 
>>> I didn't test the case where both are used. I validated that peripheral
>>> mode works across various stages of boot with "otg" mode via USB DFU
>>> boot. This isn't the case without this patch.
>>>
>>> If we forget the reason that this patch is introduced and take a moment
>>> to analyze the existing code prior to this patch, we see that:
>>> 1. Both dwc3_generic_peripheral_probe() and dwc3_generic_host_probe()
>>> invoke dwc3_generic_probe().
>>> 2. dwc3_generic_probe() sets the role based on the role specified in the/
>>> device-tree and not what the caller wants the role to be.
>>> This seems to be strange to me considering that dwc3_generic_probe()
>>> simply proceeds to configure without comparing the role supported by the
>>> platform with the role requested by the caller.
>>>
>>> So even if we entirely forget the "otg" fix being attempted by this
>>> patch, the same patch is fixing something else i.e. ensuring that
>>> dwc3_generic_probe() does what the caller wants it to do by using the
>>> role that the caller expects. Isn't it incorrect that the existing code
>>> might end up with:
>>> a) Caller being dwc3_generic_host_probe() with dr_mode being set to
>>> "peripheral" within dwc3_generic_probe() based on the device-tree and
>>> proceeding to configure the controller
>>
>> Maye this is the reason why host controller is initialized for peripheral
>> only port in the dm tree output I listed above. This needs to be fixed.
> 
> Sure, I will post a patch to do the following:
> If dr_mode specified in device-tree is "host" then return -EINVAL within
> dwc3_generic_probe() if the caller is "dwc3_generic_peripheral_probe()".
> Similarly, if dr_mode specified in the device-tree is "peripheral" then
> return -EINVAL within dwc3_generic_probe() if the caller is
> "dwc3_generic_host_probe()". When dr_mode is specified as "otg" in the
> device-tree, then configure the mode based on the caller of
> dwc3_generic_probe().
> 
> Please let me know if the above implementation is acceptable.

Looks OK to me.
> 
>>> b) Caller being dwc3_generic_peripheral_probe() with dr_mode being set
>>> to "host" within dwc3_generic_probe() based on the device-tree and
>>> proceeding to configure the controller.
>>
>> This too needs to be fixed.
>>
>>> Shouldn't this inconsistency be fixed? This patch fixes that by setting
>>> dr_mode to what the caller expects. However, based on your suggestion, I
>>> agree that there should have been a check within dwc3_generic_probe() to
>>> ensure that the platform supports what the caller is requesting. I will
>>> update this patch by adding the check.
>>
>> Apart from the platform supported mode check when not "otg", if platform
>> supports "otg" we need to check and prevent role setting if hardware is
>> not in the correct state based on connected USB cable.
>> i.e. if host mode cable is used then prevent it from running as a peripheral.
>> if peripheral mode cable is used then prevent it from running as a host.
>> (based on ID status?)
> 
> Yes, I agree that using the ID status to determine the role is accurate.
> At the same time, I think that in addition to this, if the user runs a
> command corresponding to "peripheral" mode of operation with the ID
> status indicating "host", then it will be better to take this
> inconsistency into account and error out rather than simply proceeding
> to configure on the basis of the ID status. If you have any feedback,
> kindly let me know.

The approach looks OK to me.

-- 
cheers,
-roger