Re: [PATCH v4 2/2] Embedded USB Debugger (EUD) driver

Bryan O'Donoghue
2020-02-18T14:48:14Z

On 18/02/2020 13:23, Dwivedi, Avaneesh Kumar (avani) wrote:
>
> On 2/18/2020 1:14 AM, Bryan O'Donoghue wrote:
>> On 16/02/2020 16:07, Dwivedi, Avaneesh Kumar (avani) wrote:
>>>
>>> On 2/4/2020 8:40 AM, Bryan O'Donoghue wrote:
>>>> On 03/02/2020 19:35, Bjorn Andersson wrote:
>>>>> On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
>>>>
>>>> Hi Avaneesh.
>>>
>>> Hello Bryan, Thank you very much for your review comments.
>>>
>>> Will be replying to your comments and will be posting new patchset
>>> soon as per review comments.
>>>
>>>>
>>>>> Please aim for keeping the sort order in this file (ignore QCOM_APR
>>>>> which obviously is in the wrong place)
>>>>>
>>>>>> +       tristate "QTI Embedded USB Debugger (EUD)"
>>>>>> +       depends on ARCH_QCOM
>>>>
>>>> If we persist with the model of EXTCON you should "select EXTCON" here.
>>
>>> I have asked this query with Bjorn Also against his review comments,
>>> whether we need to persist with extcon or need to switch to usb role
>>> switch framework, as we are notifying not only to usb controller but
>>> also to pmic charger so in case we adopt usb role switch then how we
>>> will notify to pmic charger to enable charging battery ? Also as i
>>> mentioned there my dilema is it does not look very apt to model EUD
>>> hw IP as c type connector, so please let me know your views.
>>
>> I think there's a desire to model USB ports as connector child nodes
>> of a USB controllers as opposed to the more generic extcon so, I think
>> the effort should probably be made to model it up as typec.
> this comment is irrespective of your below comment (If we were to
> support Control Peripheral where the local DWC3 controller has the
> signals routed away entirely, then I think we would need to look into
> modelling that in device tree - and using an overlay to show the DWC3
> controller going away in Control Peripheral mode and coming back. )?

Yes, I think irrespective we should model this as a connector not an
extcon and I think you could do think you could do that as a typec

1. Using role-switch
2. Use the regulator API to capture EUD related charger messages
and trigger changes in the PMIC as opposed to using extcon
to notify.

I could be wrong about #2

>> Can that work for you ?
> Did not comprehend this comment fully. if possible can you give some
> example.

My understanding is we are generally being encouraged to model ports as
connectors instead of extcon. I think it is possible to model your port
driver as a typec connector using USB role-switching and the regulator
API i.e. I don't think you really need extcon here.

>> Ah so, the EUD is a mux, that sits between the connector and the
>> controller, routing UTMI signals to an internal USB hub, which in-turn
>> has debug functions attached to the hub...
> Yes that is correct understanding.
>>
>> Can the Arm core see the hub ? I assume not ?
> Not sure what is it mean by "Can the Arm core see the hub"?

In Debug mode will a DWC3 controller in host mode enumerate the internal
hub ? If so, is that a supported use-case ?

>> There are a few different modes - you should probably be clear on
>> which mode it is you are supporting.
>>
>> Normal mode: (Bypass)
>> Port | EUD | Controller
>>
>> Normal + debug hub mode: (Debug)
>> Port | EUD | Controller + HUB -> debug functions
>>
>> Debug hub mode: (Control Peripheral)
>> Port | EUD | HUB -> debug functions
>>
>> its not clear to me from the documentation or the code which mode it
>> is we are targeting to be supported here.
> Its debug mode which we are supporting in driver.
>>
>> I think you should support Debug mode only here, so that the Arm core
>> never has to deal with the situation where the USB connector "goes away".
> Can you please help what you mean by "so that the Arm core never has to
> deal with the situation where the USB connector "goes away""

So my thinking is

- DWC3 in host mode
For argument sake, lets say an external self-powered hub is connected
and a number of USB devices are enumerated
- EUD switches to Control Peripheral mode

In this case what would happen ?

>>
>> If we were to support Control Peripheral where the local DWC3
>> controller has the signals routed away entirely, then I think we would
>> need to look into modelling that in device tree - and using an overlay
>> to show the DWC3 controller going away in Control Peripheral mode and
>> coming back.
> debug mode is set run time via user, i will check how we can model such
> scenario where device tree corresponding to a h/w module is only valid
> in some scenario at run time. if possible please elaborate bit more on
> your suggestion

If Debug mode is all you are trying to do support then I don't think you
really need to model that in DT.

However if intend to support Control Peripheral mode which as I
understand it, switches the UTMI signals away from a DWC3 controller in
Host mode, then I think you would need to use a DT overlay to switch off
the controller, before switching.

That's why I'm asking you about Control Peripheral mode - do you want to
support it - and if so, then what happens to DWC3 in host mode when the
UTMI signals go away ?

I think you've said you only want to support Debug mode, which makes
more sense to me.

Is Debug mode only valid when the DWC3 controller is in
peripheral/device mode and if so, should we be checking/enforcing that
somewhere - DT or EUD-driver code ?

>> Also final thought since the EUD can operate in different modes, it
>> really should be a string that gets passed in - with the string name
>> aligning to the documentation "bypass", "debug" and so on, so that the
>> mode we are switching to is obvious to anybody who has the spec and
>> the driver.
>
> you mean we should document that this driver works in debug mode only?
> not clear on where one should pass "debug" and "bypass" string?

You have a routine to switch to debug mode that takes a parameter from
user-space right ?

Bjorn mentioned you could write 42. My question/suggestion is why isn't
the value written a string which corresponds to the supported modes from
the EUD spec ?
"bypass" as default "debug" the mode you want to add, at a later time
you could optionally add in "control-periperhal" mode.

Makes a little more sense to me than writing just 0, 1 or 42 :) into
your store routine.

---
bod