Re: [PATCH v4 10/20] tegra: fdt: Add additional USB binding

Re: [PATCH v4 10/20] tegra: fdt: Add additional USB binding

[Stephen Warren]

On 01/11/2012 09:32 PM, Simon Glass wrote:
> This adds a property to indicate a port which can switch between host and device
> mode.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> 
>  doc/device-tree-bindings/usb/tegra-usb.txt |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/doc/device-tree-bindings/usb/tegra-usb.txt b/doc/device-tree-bindings/usb/tegra-usb.txt
> index 035d63d..96fd022 100644
> --- a/doc/device-tree-bindings/usb/tegra-usb.txt
> +++ b/doc/device-tree-bindings/usb/tegra-usb.txt
> @@ -11,3 +11,7 @@ Required properties :
>   - phy_type : Should be one of "ulpi" or "utmi".
>   - nvidia,vbus-gpio : If present, specifies a gpio that needs to be
>     activated for the bus to be powered.
> +
> +Optional properties:
> + - support-host-mode : If present then this peripheral can switch between
> +   host and device mode

All of Tegra's USB ports support host mode, and it's the typical mode of
operation. The TRM also indicates that all USB ports support device mode
(possibly depending on the PHY type that's been selected for or attached
to port). Port 2 explicitly doesn't support run-time switching, but by
omission, ports 1 and 3 do.

Hence, "support-host-mode" is not an accurate name, and doesn't match
the description given either.

I think that it'd be better to:
* Add properties to explicitly specify whether the port is
intended/allowed to operate (per board design) in each of host or device
mode.
* Default to static host mode if no properties are present.

i.e.:

+Optional properties:
+ - enable-host-mode : Indicate that the port is intended to operate in
+   host mode.
+ - enable-device-mode : Indicate that the port is intended to operate
+   in device mode.

We should error out if both properties are set for USB 2, since it
doesn't support run-time switching. Perhaps you can skip this
error-checking for now since we aren't implementing device mode anywhere
anyway...

Then in the later patch in your series that was using support-host-mode,

* Do VBUS detection only if both enable-host-mode and enable-device-mode
are present.
* Do VBUS GPIO enable if enable-host-mode is present (or assumed by
default) and VBUS detection passed (if it ran).

-- 
nvpublic

Re: [PATCH v4 10/20] tegra: fdt: Add additional USB binding

[Olof Johansson]

On Wed, Jan 18, 2012 at 03:48:30PM -0700, Stephen Warren wrote:
> On 01/11/2012 09:32 PM, Simon Glass wrote:
> > This adds a property to indicate a port which can switch between host and device
> > mode.
> > 
> > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > ---
> > 
> >  doc/device-tree-bindings/usb/tegra-usb.txt |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/doc/device-tree-bindings/usb/tegra-usb.txt b/doc/device-tree-bindings/usb/tegra-usb.txt
> > index 035d63d..96fd022 100644
> > --- a/doc/device-tree-bindings/usb/tegra-usb.txt
> > +++ b/doc/device-tree-bindings/usb/tegra-usb.txt
> > @@ -11,3 +11,7 @@ Required properties :
> >   - phy_type : Should be one of "ulpi" or "utmi".
> >   - nvidia,vbus-gpio : If present, specifies a gpio that needs to be
> >     activated for the bus to be powered.
> > +
> > +Optional properties:
> > + - support-host-mode : If present then this peripheral can switch between
> > +   host and device mode
> 
> All of Tegra's USB ports support host mode, and it's the typical mode of
> operation. The TRM also indicates that all USB ports support device mode
> (possibly depending on the PHY type that's been selected for or attached
> to port). Port 2 explicitly doesn't support run-time switching, but by
> omission, ports 1 and 3 do.
> 
> Hence, "support-host-mode" is not an accurate name, and doesn't match
> the description given either.
> 
> I think that it'd be better to:
> * Add properties to explicitly specify whether the port is
> intended/allowed to operate (per board design) in each of host or device
> mode.
> * Default to static host mode if no properties are present.
> 
> i.e.:
> 
> +Optional properties:
> + - enable-host-mode : Indicate that the port is intended to operate in
> +   host mode.
> + - enable-device-mode : Indicate that the port is intended to operate
> +   in device mode.

fsl-usb.txt uses "dr_mode" here, which might make sense to reuse given some of
the shared IP in question for device mode:

 - dr_mode : indicates the working mode for "fsl-usb2-dr" compatible
   controllers.  Can be "host", "peripheral", or "otg".  Default to
   "host" if not defined for backward compatibility.


-Olof

Re: [PATCH v4 10/20] tegra: fdt: Add additional USB binding

[Simon Glass]

Hi Olof,

On Wed, Jan 18, 2012 at 9:35 PM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> On Wed, Jan 18, 2012 at 03:48:30PM -0700, Stephen Warren wrote:
>> On 01/11/2012 09:32 PM, Simon Glass wrote:
>> > This adds a property to indicate a port which can switch between host and device
>> > mode.
>> >
>> > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> > ---
>> >
>> >  doc/device-tree-bindings/usb/tegra-usb.txt |    4 ++++
>> >  1 files changed, 4 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/doc/device-tree-bindings/usb/tegra-usb.txt b/doc/device-tree-bindings/usb/tegra-usb.txt
>> > index 035d63d..96fd022 100644
>> > --- a/doc/device-tree-bindings/usb/tegra-usb.txt
>> > +++ b/doc/device-tree-bindings/usb/tegra-usb.txt
>> > @@ -11,3 +11,7 @@ Required properties :
>> >   - phy_type : Should be one of "ulpi" or "utmi".
>> >   - nvidia,vbus-gpio : If present, specifies a gpio that needs to be
>> >     activated for the bus to be powered.
>> > +
>> > +Optional properties:
>> > + - support-host-mode : If present then this peripheral can switch between
>> > +   host and device mode
>>
>> All of Tegra's USB ports support host mode, and it's the typical mode of
>> operation. The TRM also indicates that all USB ports support device mode
>> (possibly depending on the PHY type that's been selected for or attached
>> to port). Port 2 explicitly doesn't support run-time switching, but by
>> omission, ports 1 and 3 do.
>>
>> Hence, "support-host-mode" is not an accurate name, and doesn't match
>> the description given either.
>>
>> I think that it'd be better to:
>> * Add properties to explicitly specify whether the port is
>> intended/allowed to operate (per board design) in each of host or device
>> mode.
>> * Default to static host mode if no properties are present.
>>
>> i.e.:
>>
>> +Optional properties:
>> + - enable-host-mode : Indicate that the port is intended to operate in
>> +   host mode.
>> + - enable-device-mode : Indicate that the port is intended to operate
>> +   in device mode.
>
> fsl-usb.txt uses "dr_mode" here, which might make sense to reuse given some of
> the shared IP in question for device mode:
>
>  - dr_mode : indicates the working mode for "fsl-usb2-dr" compatible
>   controllers.  Can be "host", "peripheral", or "otg".  Default to
>   "host" if not defined for backward compatibility.

What does 'dr' mean?

Regards,
Simon

>
>
> -Olof

Re: [PATCH v4 10/20] tegra: fdt: Add additional USB binding

[Olof Johansson]

Hi,

On Wed, Jan 18, 2012 at 9:55 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:

>> fsl-usb.txt uses "dr_mode" here, which might make sense to reuse given some of
>> the shared IP in question for device mode:
>>
>>  - dr_mode : indicates the working mode for "fsl-usb2-dr" compatible
>>   controllers.  Can be "host", "peripheral", or "otg".  Default to
>>   "host" if not defined for backward compatibility.
>
> What does 'dr' mean?

After some searching, it looks like it means "dual-role", which seems
appropriate terminology for the tegra SoC as well.


-Olof

Re: [PATCH v4 10/20] tegra: fdt: Add additional USB binding

[Simon Glass]

Hi Olof,

On Wed, Jan 18, 2012 at 10:41 PM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> Hi,
>
> On Wed, Jan 18, 2012 at 9:55 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
>>> fsl-usb.txt uses "dr_mode" here, which might make sense to reuse given some of
>>> the shared IP in question for device mode:
>>>
>>>  - dr_mode : indicates the working mode for "fsl-usb2-dr" compatible
>>>   controllers.  Can be "host", "peripheral", or "otg".  Default to
>>>   "host" if not defined for backward compatibility.
>>
>> What does 'dr' mean?
>
> After some searching, it looks like it means "dual-role", which seems
> appropriate terminology for the tegra SoC as well.

Thanks for looking that up.

Well maybe, but it could mean data reverse mode or distinctive
ring...I like Stephen's nice device-tree-friendly naming and
explanation :-) Are we stuck with this?

Regards,
Simon

>
>
> -Olof

Re: [PATCH v4 10/20] tegra: fdt: Add additional USB binding

[Olof Johansson]

On Wed, Jan 18, 2012 at 10:59 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Olof,
>
> On Wed, Jan 18, 2012 at 10:41 PM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
>> Hi,
>>
>> On Wed, Jan 18, 2012 at 9:55 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>
>>>> fsl-usb.txt uses "dr_mode" here, which might make sense to reuse given some of
>>>> the shared IP in question for device mode:
>>>>
>>>>  - dr_mode : indicates the working mode for "fsl-usb2-dr" compatible
>>>>   controllers.  Can be "host", "peripheral", or "otg".  Default to
>>>>   "host" if not defined for backward compatibility.
>>>
>>> What does 'dr' mean?
>>
>> After some searching, it looks like it means "dual-role", which seems
>> appropriate terminology for the tegra SoC as well.
>
> Thanks for looking that up.
>
> Well maybe, but it could mean data reverse mode or distinctive
> ring...I like Stephen's nice device-tree-friendly naming and
> explanation :-) Are we stuck with this?

I think there's value in staying common with similar bindings on other
platforms, yes. Sorry. :)


-Olof

RE: [PATCH v4 10/20] tegra: fdt: Add additional USB binding

[Stephen Warren]

Olof Johansson wrote at Thursday, January 19, 2012 12:13 AM:
> On Wed, Jan 18, 2012 at 10:59 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> > Hi Olof,
> >
> > On Wed, Jan 18, 2012 at 10:41 PM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> >> Hi,
> >>
> >> On Wed, Jan 18, 2012 at 9:55 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> >>
> >>>> fsl-usb.txt uses "dr_mode" here, which might make sense to reuse given some of
> >>>> the shared IP in question for device mode:
> >>>>
> >>>>  - dr_mode : indicates the working mode for "fsl-usb2-dr" compatible
> >>>>   controllers.  Can be "host", "peripheral", or "otg".  Default to
> >>>>   "host" if not defined for backward compatibility.
> >>>
> >>> What does 'dr' mean?
> >>
> >> After some searching, it looks like it means "dual-role", which seems
> >> appropriate terminology for the tegra SoC as well.
> >
> > Thanks for looking that up.
> >
> > Well maybe, but it could mean data reverse mode or distinctive
> > ring...I like Stephen's nice device-tree-friendly naming and
> > explanation :-) Are we stuck with this?
> 
> I think there's value in staying common with similar bindings on other
> platforms, yes. Sorry. :)

Ack from me too. If there's an existing convention, I'd prefer to stick
with it. Sorry, I should have looked this up before coming up with
something new.

-- 
nvpublic

Re: [PATCH v4 10/20] tegra: fdt: Add additional USB binding

[Simon Glass]

Hi,

On Thu, Jan 19, 2012 at 9:33 AM, Stephen Warren <swarren@nvidia.com> wrote:
> Olof Johansson wrote at Thursday, January 19, 2012 12:13 AM:
>> On Wed, Jan 18, 2012 at 10:59 PM, Simon Glass <sjg@chromium.org> wrote:
>> > Hi Olof,
>> >
>> > On Wed, Jan 18, 2012 at 10:41 PM, Olof Johansson <olof@lixom.net> wrote:
>> >> Hi,
>> >>
>> >> On Wed, Jan 18, 2012 at 9:55 PM, Simon Glass <sjg@chromium.org> wrote:
>> >>
>> >>>> fsl-usb.txt uses "dr_mode" here, which might make sense to reuse given some of
>> >>>> the shared IP in question for device mode:
>> >>>>
>> >>>>  - dr_mode : indicates the working mode for "fsl-usb2-dr" compatible
>> >>>>   controllers.  Can be "host", "peripheral", or "otg".  Default to
>> >>>>   "host" if not defined for backward compatibility.
>> >>>
>> >>> What does 'dr' mean?
>> >>
>> >> After some searching, it looks like it means "dual-role", which seems
>> >> appropriate terminology for the tegra SoC as well.
>> >
>> > Thanks for looking that up.
>> >
>> > Well maybe, but it could mean data reverse mode or distinctive
>> > ring...I like Stephen's nice device-tree-friendly naming and
>> > explanation :-) Are we stuck with this?
>>
>> I think there's value in staying common with similar bindings on other
>> platforms, yes. Sorry. :)
>
> Ack from me too. If there's an existing convention, I'd prefer to stick
> with it. Sorry, I should have looked this up before coming up with
> something new.

OK thanks for that. Will take a look.

Regards,
Simon

>
> --
> nvpublic
>