Re: [PATCH v3 09/12] usb: cdns3: Handle otg mode as peripheral

[Roger Quadros <rogerq@kernel.org>]

On 19/08/2023 15:20, Marek Vasut wrote:
> On 8/19/23 09:00, Roger Quadros wrote:
>> +Peter & Pawel.
>>
>> On 19/08/2023 02:47, Marek Vasut wrote:
>>> On 8/18/23 21:26, Roger Quadros wrote:
>>>>
>>>>
>>>> On 18/08/2023 21:48, Tom Rini wrote:
>>>>> On Thu, Aug 17, 2023 at 11:15:17AM +0300, Roger Quadros wrote:
>>>>>> Hi Tom,
>>>>>>
>>>>>> On 14/08/2023 20:17, Tom Rini wrote:
>>>>>>> On Thu, Jul 13, 2023 at 03:45:42PM +0200, Julien Panis wrote:
>>>>>>>
>>>>>>>> Override 'otg' to 'peripheral' mode, since 'otg' mode
>>>>>>>> is not yet supported by u-boot.
>>>>>>>>
>>>>>>>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
>>>>>>>> Suggested-by: Roger Quadros <rogerq@kernel.org>
>>>>>>>> ---
>>>>>>>>    drivers/usb/cdns3/core.c | 4 ++++
>>>>>>>>    1 file changed, 4 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>>>>>>>> index 644a9791b9c9..bd763fc593e1 100644
>>>>>>>> --- a/drivers/usb/cdns3/core.c
>>>>>>>> +++ b/drivers/usb/cdns3/core.c
>>>>>>>> @@ -149,6 +149,10 @@ static int cdns3_core_init_role(struct cdns3 *cdns)
>>>>>>>>          dr_mode = best_dr_mode;
>>>>>>>>    +    /* u-boot doesn't yet support OTG so limit to PERIPHERAL */
>>>>>>>> +    if (dr_mode == USB_DR_MODE_OTG)
>>>>>>>> +        dr_mode = USB_DR_MODE_PERIPHERAL;
>>>>>>>> +
>>>>>>>>    #if defined(CONFIG_SPL_USB_HOST) || !defined(CONFIG_SPL_BUILD)
>>>>>>>>        if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>>>>>>>            ret = cdns3_host_init(cdns);
>>>>>>>
>>>>>>> Julien, why don't we support otg mode here?
>>>>>>>
>>>>>>
>>>>>> dr_mode will never be OTG at this point as the previous if condition
>>>>>> would have forced it to PERIPHERAL.
>>>>>>
>>>>>> My understanding was that u-boot USB framework doesn't support OTG mode
>>>>>> so we force it to PERIPHERAL.
>>>>>
>>>>> Well, the first part of this series is "make unknown state be OTG" for
>>>>> DWC3, so we do in general (and Marek told me off-list he's used it on
>>>>> DWC2 as well), so it sounds like these (CDNS3, MUSB) drivers need fixing
>>>>> / updating.
>>>>>
>>>>
>>>> CDNS3 does not have an internal OTG state machine. We mostly really care
>>>> about role switching and not the full OTG stack. There needs to be
>>>> some kind of SW framework to do that in u-boot.
>>>>
>>>> Marek could you please advise what can be done here?
>>>
>>> As far as I can tell, MX8MM (CI HDRC), STM32MP1 (DWC2) all can do the host/peripheral switching. Why is that a problem with CDNS3 ? I am not familiar with this controller btw.
>>
>> So for MX8MM and STM32MP1, do  we support on the fly host/peripheral switching based on USB plug type?
>> As we don't have interrupts, do you keep polling for ID status change?
> 
> Nope, you just plug in one cable or the other and run the matching command -- either 'usb reset' for Host or 'ums/dfu/...' for Peripheral .

OK.
> 
>> CDNS3 does support host/peripheral switching. i.e. cdsn3_hw_role_state_machine() and drd.c.
>> What we are missing is the interrupt logic to map ID pin status change to role change.
> 
> I don't think you need any interrupt logic for this to work ?

For the use case you described, no.
Julien, I think this can be easily achieved with the current driver.
Would be nice if driver prevents user from setting incorrect role than
what cable is set to.

-- 
cheers,
-roger