2017-03-29 23:16:40

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 2/3] usb: add DT bindings for farady fotg2 host controller

On Tue, Feb 21, 2017 at 3:43 PM, Rob Herring <[email protected]> wrote:
> On Fri, Feb 17, 2017 at 4:07 AM, Hans Ulli Kroll
> <[email protected]> wrote:
>> Hi Rob,
>>
>> On Wed, 15 Feb 2017, Rob Herring wrote:
>>
>>> On Wed, Feb 08, 2017 at 09:00:09PM +0100, Hans Ulli Kroll wrote:
>>> > This adds DT bindings for the Faraday FOTG2 host controller.
>>> >
>>> > Signed-off-by: Hans Ulli Kroll <[email protected]>
>>> > ---
>>> > Documentation/devicetree/bindings/usb/fotg2-host.txt | 15 +++++++++++++++
>>> > 1 file changed, 15 insertions(+)
>>> > create mode 100644 Documentation/devicetree/bindings/usb/fotg2-host.txt
>>> >
>>> > diff --git a/Documentation/devicetree/bindings/usb/fotg2-host.txt b/Documentation/devicetree/bindings/usb/fotg2-host.txt
>>> > new file mode 100644
>>> > index 000000000000..4c07566a4bf5
>>> > --- /dev/null
>>> > +++ b/Documentation/devicetree/bindings/usb/fotg2-host.txt
>>> > @@ -0,0 +1,15 @@
>>> > +Faraday FOTG Host controller
>>> > +
>>> > +Required properties:
>>> > +
>>> > +- compatible: should be "faraday,fotg210-hcd"
>>>
>>> hcd as in "host controller driver"? Bindings describe h/w not drivers.
>>>
>>> It's an OTG controller or host controller?
>>>
>>
>> here only the host controller part used.
>>
>> faraday fotg2 is a dual role hcd/otg device and here is only the
>> host part used.
>
> Because you don't care about device mode or restricted by the IP
> configuration or SoC integration? The former is a user choice and
> shouldn't be part of DT. The latter should be implied by an SoC
> specific compatible string. Using only a compatible string for a
> licensed IP is not specific enough as vendors use differing versions
> and integrate them in different ways.

Hans can you add:

compatible = "cortina,gemini-fotg", "faraday,fotg210-hcd" or something
as composite compatible for our controller?

It's OK for the driver to only match on the latter for now, but for the
future it's nice for the driver to know what silicon it is on.

This is what I've done for most other Cortina/Gemini stuff.

Yours,
Linus Walleij


2017-03-30 18:31:58

by Hans Ulli Kroll

[permalink] [raw]
Subject: Re: [PATCH 2/3] usb: add DT bindings for farady fotg2 host controller

Hi Linus,

On Thu, 30 Mar 2017, Linus Walleij wrote:

> On Tue, Feb 21, 2017 at 3:43 PM, Rob Herring <[email protected]> wrote:
> > On Fri, Feb 17, 2017 at 4:07 AM, Hans Ulli Kroll
> > <[email protected]> wrote:
> >> Hi Rob,
> >>
> >> On Wed, 15 Feb 2017, Rob Herring wrote:
> >>
> >>> On Wed, Feb 08, 2017 at 09:00:09PM +0100, Hans Ulli Kroll wrote:
> >>> > This adds DT bindings for the Faraday FOTG2 host controller.
> >>> >
> >>> > Signed-off-by: Hans Ulli Kroll <[email protected]>
> >>> > ---
> >>> > Documentation/devicetree/bindings/usb/fotg2-host.txt | 15 +++++++++++++++
> >>> > 1 file changed, 15 insertions(+)
> >>> > create mode 100644 Documentation/devicetree/bindings/usb/fotg2-host.txt
> >>> >
> >>> > diff --git a/Documentation/devicetree/bindings/usb/fotg2-host.txt b/Documentation/devicetree/bindings/usb/fotg2-host.txt
> >>> > new file mode 100644
> >>> > index 000000000000..4c07566a4bf5
> >>> > --- /dev/null
> >>> > +++ b/Documentation/devicetree/bindings/usb/fotg2-host.txt
> >>> > @@ -0,0 +1,15 @@
> >>> > +Faraday FOTG Host controller
> >>> > +
> >>> > +Required properties:
> >>> > +
> >>> > +- compatible: should be "faraday,fotg210-hcd"
> >>>
> >>> hcd as in "host controller driver"? Bindings describe h/w not drivers.
> >>>
> >>> It's an OTG controller or host controller?
> >>>
> >>
> >> here only the host controller part used.
> >>
> >> faraday fotg2 is a dual role hcd/otg device and here is only the
> >> host part used.
> >
> > Because you don't care about device mode or restricted by the IP
> > configuration or SoC integration? The former is a user choice and
> > shouldn't be part of DT. The latter should be implied by an SoC
> > specific compatible string. Using only a compatible string for a
> > licensed IP is not specific enough as vendors use differing versions
> > and integrate them in different ways.
>
> Hans can you add:
>
> compatible = "cortina,gemini-fotg", "faraday,fotg210-hcd" or something
> as composite compatible for our controller?
>

I prefer
"faraday,fotg210-usb2"

I've got rejected by Rob due the fact this is an dual role controller,
which supports both host and device mode. And DT must reflect this desgn
pattern.

Currently I'm wrappingt my head around the design of the fsl-mph-dr-of.c
driver to use this as a blueprint for the Faraday driver.

Greetings
Hans Ulli Kroll

2017-04-22 00:22:35

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 2/3] usb: add DT bindings for farady fotg2 host controller

On Thu, Mar 30, 2017 at 8:31 PM, Hans Ulli Kroll
<[email protected]> wrote:
>> On Tue, Feb 21, 2017 at 3:43 PM, Rob Herring <[email protected]> wrote:
>> > On Fri, Feb 17, 2017 at 4:07 AM, Hans Ulli Kroll
>> > <[email protected]> wrote:
>> >> Hi Rob,
>> >>
>> >> On Wed, 15 Feb 2017, Rob Herring wrote:
>> >>
>> >>> On Wed, Feb 08, 2017 at 09:00:09PM +0100, Hans Ulli Kroll wrote:
>> >>> > This adds DT bindings for the Faraday FOTG2 host controller.
>> >>> >
>> >>> > Signed-off-by: Hans Ulli Kroll <[email protected]>
>> >>> > ---
>> >>> > Documentation/devicetree/bindings/usb/fotg2-host.txt | 15 +++++++++++++++
>> >>> > 1 file changed, 15 insertions(+)
>> >>> > create mode 100644 Documentation/devicetree/bindings/usb/fotg2-host.txt
>> >>> >
>> >>> > diff --git a/Documentation/devicetree/bindings/usb/fotg2-host.txt b/Documentation/devicetree/bindings/usb/fotg2-host.txt
>> >>> > new file mode 100644
>> >>> > index 000000000000..4c07566a4bf5
>> >>> > --- /dev/null
>> >>> > +++ b/Documentation/devicetree/bindings/usb/fotg2-host.txt
>> >>> > @@ -0,0 +1,15 @@
>> >>> > +Faraday FOTG Host controller
>> >>> > +
>> >>> > +Required properties:
>> >>> > +
>> >>> > +- compatible: should be "faraday,fotg210-hcd"
>> >>>
>> >>> hcd as in "host controller driver"? Bindings describe h/w not drivers.
>> >>>
>> >>> It's an OTG controller or host controller?
>> >>>
>> >>
>> >> here only the host controller part used.
>> >>
>> >> faraday fotg2 is a dual role hcd/otg device and here is only the
>> >> host part used.
>> >
>> > Because you don't care about device mode or restricted by the IP
>> > configuration or SoC integration? The former is a user choice and
>> > shouldn't be part of DT. The latter should be implied by an SoC
>> > specific compatible string. Using only a compatible string for a
>> > licensed IP is not specific enough as vendors use differing versions
>> > and integrate them in different ways.
>>
>> Hans can you add:
>>
>> compatible = "cortina,gemini-fotg", "faraday,fotg210-hcd" or something
>> as composite compatible for our controller?

I hacked on it a bit and sent out. Hope you don't hate it too much.

> I prefer
> "faraday,fotg210-usb2"

I simply named it after the IP core name, which is just "faraday,fotg210".

> I've got rejected by Rob due the fact this is an dual role controller,
> which supports both host and device mode. And DT must reflect this desgn
> pattern.
>
> Currently I'm wrappingt my head around the design of the fsl-mph-dr-of.c
> driver to use this as a blueprint for the Faraday driver.

I don't know how much more of the dual-mode we need to reflect,
I guess it comes up in OTG mode since we don't parse the
host-only or device-only attributes (yet).

Yours,
Linus Walleij