RE: [PATCH/RFC v4 2/4] usb: gadget: udc: renesas_usb3: Add register of usb role switch

2018-05-24 Thread Yoshihiro Shimoda 
Hi Rob, Geert-san,

> From: Geert Uytterhoeven, Sent: Thursday, May 24, 2018 5:18 PM
> 
> Hi Rob,
> 
> On Wed, May 23, 2018 at 5:00 PM, Rob Herring  wrote:

> >>> >  Optional properties:
> >>> >- phys: phandle + phy specifier pair
> >>> >- phy-names: must be "usb"
> >>> > +  - The connection to a usb3.0 host node needs by using OF graph 
> >>> > bindings for
> >>> > +usb role switch.
> >>> > +   - port@0 = USB3.0 host port.
> >>>
> >>> On the host side, this might conflict with the USB connector binding.
> >>>
> >>> I would either make sure this can work with the connector binding by
> >>> having 2 endpoints on the HS or SS port or just use the 'companion'
> >>> property defined in usb-generic.txt.
> >>
> >> I don't understand the first one now... This means the renesas_usb3 should 
> >> follow
> >> USB connector binding and have 2 endpoints for the usb role switch to avoid
> >> the conflict like below?
> >>  - port1@0: Super Speed (SS), present in SS capable connectors (from 
> >> usb-connector.txt).
> >>  - port1@1: USB3.0 host port.
> >
> > I'm confused, SS and USB3.0 are the same essentially. It would be:
> >
> > port@1/endpoint@0: SS host port
> > port@1/endpoint@1: SS device port

Thank you for the comment. It's better than my description.

> >> About the 'companion' in usb-generic.txt, the property intends to be used 
> >> for EHCI or host side
> >> like the commit log [1]. If there is accept to use 'companion' for this 
> >> patch, I think it will
> >> be simple to achieve this role switch feature. However, in last month, I 
> >> submitted a similar patch [2]
> >> that has "renesas,host" property, but I got reply from Andy [3] and Heikki 
> >> [4]. So, I'm
> >> trying to improve the device connection framework [5] now.
> >
> > I think this case is rare enough that we don't need a general solution
> > using OF graph, so I'm fine with a simple, single property to link the
> > 2 nodes. Either reusing "companion" or "renesas,host" is fine by me.
> 
> I'd go for the standard "companion" over "renesas,host"[*].
> 
> [*] Doh, we have another one ("renesas,bonding"), invented when I wasn't
> aware of the existence of "companion" yet...

Thank you for the comments. So, I'll reuse "companion" for it.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

Re: [PATCH/RFC v4 2/4] usb: gadget: udc: renesas_usb3: Add register of usb role switch

2018-05-24 Thread Geert Uytterhoeven 
Hi Rob,

On Wed, May 23, 2018 at 5:00 PM, Rob Herring  wrote:
> On Wed, May 23, 2018 at 1:52 AM, Yoshihiro Shimoda
>  wrote:
>>> From: Rob Herring, Sent: Wednesday, May 23, 2018 2:13 AM
>>> On Tue, May 22, 2018 at 09:01:07PM +0900, Yoshihiro Shimoda wrote:
>>> > --- a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
>>> > +++ b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
>>> > @@ -19,6 +19,9 @@ Required properties:
>>> >  Optional properties:
>>> >- phys: phandle + phy specifier pair
>>> >- phy-names: must be "usb"
>>> > +  - The connection to a usb3.0 host node needs by using OF graph 
>>> > bindings for
>>> > +usb role switch.
>>> > +   - port@0 = USB3.0 host port.
>>>
>>> On the host side, this might conflict with the USB connector binding.
>>>
>>> I would either make sure this can work with the connector binding by
>>> having 2 endpoints on the HS or SS port or just use the 'companion'
>>> property defined in usb-generic.txt.
>>
>> I don't understand the first one now... This means the renesas_usb3 should 
>> follow
>> USB connector binding and have 2 endpoints for the usb role switch to avoid
>> the conflict like below?
>>  - port1@0: Super Speed (SS), present in SS capable connectors (from 
>> usb-connector.txt).
>>  - port1@1: USB3.0 host port.
>
> I'm confused, SS and USB3.0 are the same essentially. It would be:
>
> port@1/endpoint@0: SS host port
> port@1/endpoint@1: SS device port
>
>> About the 'companion' in usb-generic.txt, the property intends to be used 
>> for EHCI or host side
>> like the commit log [1]. If there is accept to use 'companion' for this 
>> patch, I think it will
>> be simple to achieve this role switch feature. However, in last month, I 
>> submitted a similar patch [2]
>> that has "renesas,host" property, but I got reply from Andy [3] and Heikki 
>> [4]. So, I'm
>> trying to improve the device connection framework [5] now.
>
> I think this case is rare enough that we don't need a general solution
> using OF graph, so I'm fine with a simple, single property to link the
> 2 nodes. Either reusing "companion" or "renesas,host" is fine by me.

I'd go for the standard "companion" over "renesas,host"[*].

[*] Doh, we have another one ("renesas,bonding"), invented when I wasn't
aware of the existence of "companion" yet...

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH/RFC v4 2/4] usb: gadget: udc: renesas_usb3: Add register of usb role switch

2018-05-23 Thread Rob Herring 
On Wed, May 23, 2018 at 1:52 AM, Yoshihiro Shimoda
 wrote:
> Hi Rob,
>
> Thank you for the review!
>
>> From: Rob Herring, Sent: Wednesday, May 23, 2018 2:13 AM
>>
>> On Tue, May 22, 2018 at 09:01:07PM +0900, Yoshihiro Shimoda wrote:
>> > This patch adds role switch support for R-Car SoCs into the USB 3.0
>> > peripheral driver. Some R-Car SoCs (e.g. R-Car H3) have USB 3.0
>> > dual-role device controller which has the USB 3.0 xHCI host and
>> > Renesas USB 3.0 peripheral.
>> >
>> > Unfortunately, the mode change register contains the USB 3.0 peripheral
>> > controller side only. So, the USB 3.0 peripheral driver (renesas_usb3)
>> > manages this register now. However, in peripheral mode, the host
>> > should stop. Also the host hardware needs to reinitialize its own
>> > registers when the mode changes from peripheral to host mode.
>> > Otherwise, the host cannot work correctly (e.g. detect a device as
>> > high-speed).
>> >
>> > To achieve this by a driver, this role switch driver manages
>> > the mode change register and attach/release the xhci-plat driver.
>> >
>> > Signed-off-by: Yoshihiro Shimoda 
>> > ---
>> >  .../devicetree/bindings/usb/renesas_usb3.txt   | 15 
>>
>> Please split bindings to a separate patch.
>
> Oops. I got it.
>
>> >  drivers/usb/gadget/udc/Kconfig |  1 +
>> >  drivers/usb/gadget/udc/renesas_usb3.c  | 82 
>> > ++
>> >  3 files changed, 98 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
>> b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
>> > index 2c071bb5..f6105aa 100644
>> > --- a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
>> > +++ b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
>> > @@ -19,6 +19,9 @@ Required properties:
>> >  Optional properties:
>> >- phys: phandle + phy specifier pair
>> >- phy-names: must be "usb"
>> > +  - The connection to a usb3.0 host node needs by using OF graph bindings 
>> > for
>> > +usb role switch.
>> > +   - port@0 = USB3.0 host port.
>>
>> On the host side, this might conflict with the USB connector binding.
>>
>> I would either make sure this can work with the connector binding by
>> having 2 endpoints on the HS or SS port or just use the 'companion'
>> property defined in usb-generic.txt.
>
> I don't understand the first one now... This means the renesas_usb3 should 
> follow
> USB connector binding and have 2 endpoints for the usb role switch to avoid
> the conflict like below?
>  - port1@0: Super Speed (SS), present in SS capable connectors (from 
> usb-connector.txt).
>  - port1@1: USB3.0 host port.

I'm confused, SS and USB3.0 are the same essentially. It would be:

port@1/endpoint@0: SS host port
port@1/endpoint@1: SS device port

> About the 'companion' in usb-generic.txt, the property intends to be used for 
> EHCI or host side
> like the commit log [1]. If there is accept to use 'companion' for this 
> patch, I think it will
> be simple to achieve this role switch feature. However, in last month, I 
> submitted a similar patch [2]
> that has "renesas,host" property, but I got reply from Andy [3] and Heikki 
> [4]. So, I'm
> trying to improve the device connection framework [5] now.

I think this case is rare enough that we don't need a general solution
using OF graph, so I'm fine with a simple, single property to link the
2 nodes. Either reusing "companion" or "renesas,host" is fine by me.

Rob

>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/usb/generic.txt?h=v4.17-rc6=5095cb89c62acc78b4cfaeb9a4072979d010510a
>
> [2]
> https://www.spinics.net/lists/linux-usb/msg167773.html
>
> [3]
> https://www.spinics.net/lists/linux-usb/msg167780.html
>
> [4]
> https://www.spinics.net/lists/linux-usb/msg167806.html
>
> [5]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/driver-api/device_connection.rst
>
> Best regards,
> Yoshihiro Shimoda
>
>> Rob

Re: [PATCH/RFC v4 2/4] usb: gadget: udc: renesas_usb3: Add register of usb role switch

2018-05-22 Thread Rob Herring 
On Tue, May 22, 2018 at 09:01:07PM +0900, Yoshihiro Shimoda wrote:
> This patch adds role switch support for R-Car SoCs into the USB 3.0
> peripheral driver. Some R-Car SoCs (e.g. R-Car H3) have USB 3.0
> dual-role device controller which has the USB 3.0 xHCI host and
> Renesas USB 3.0 peripheral.
> 
> Unfortunately, the mode change register contains the USB 3.0 peripheral
> controller side only. So, the USB 3.0 peripheral driver (renesas_usb3)
> manages this register now. However, in peripheral mode, the host
> should stop. Also the host hardware needs to reinitialize its own
> registers when the mode changes from peripheral to host mode.
> Otherwise, the host cannot work correctly (e.g. detect a device as
> high-speed).
> 
> To achieve this by a driver, this role switch driver manages
> the mode change register and attach/release the xhci-plat driver.
> 
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  .../devicetree/bindings/usb/renesas_usb3.txt   | 15 

Please split bindings to a separate patch.

>  drivers/usb/gadget/udc/Kconfig |  1 +
>  drivers/usb/gadget/udc/renesas_usb3.c  | 82 
> ++
>  3 files changed, 98 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt 
> b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> index 2c071bb5..f6105aa 100644
> --- a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> +++ b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> @@ -19,6 +19,9 @@ Required properties:
>  Optional properties:
>- phys: phandle + phy specifier pair
>- phy-names: must be "usb"
> +  - The connection to a usb3.0 host node needs by using OF graph bindings for
> +usb role switch.
> +   - port@0 = USB3.0 host port.

On the host side, this might conflict with the USB connector binding. 

I would either make sure this can work with the connector binding by 
having 2 endpoints on the HS or SS port or just use the 'companion' 
property defined in usb-generic.txt.

Rob