RE: [PATCH v2 2/7] dt-bindings: usb: renesas_usb3: add usb-role-switch property

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi All,

Thanks for the feedback.

> -----Original Message-----
> From: Rob Herring <robh@xxxxxxxxxx>
> Sent: 29 March 2019 13:58
> To: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Cc: Biju Das <biju.das@xxxxxxxxxxxxxx>; Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx>; Mark Rutland
> <mark.rutland@xxxxxxx>; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>; Felipe Balbi <balbi@xxxxxxxxxx>; linux-
> usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Simon Horman
> <horms@xxxxxxxxxxxx>; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>;
> Chris Paterson <Chris.Paterson2@xxxxxxxxxxx>; Fabrizio Castro
> <fabrizio.castro@xxxxxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 2/7] dt-bindings: usb: renesas_usb3: add usb-role-
> switch property
> 
> On Thu, Mar 28, 2019 at 12:48 PM Heikki Krogerus
> <heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Mar 28, 2019 at 10:33:53AM -0500, Rob Herring wrote:
> > > On Fri, Mar 15, 2019 at 12:51:46PM +0200, Heikki Krogerus wrote:
> > > > Thanks,
> > > >
> > > > On Fri, Mar 15, 2019 at 09:08:19AM +0000, Biju Das wrote:
> > > > > > Subject: Re: [PATCH v2 2/7] dt-bindings: usb: renesas_usb3:
> > > > > > add usb-role-
> > > > > > > > > a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > > > > > > > > b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > > > > > > > > index 35039e7..eecaf4c 100644
> > > > > > > > > ---
> > > > > > > > > a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > > > > > > > > +++
> b/Documentation/devicetree/bindings/usb/renesas_usb3
> > > > > > > > > +++ .txt
> > > > > > > > > @@ -22,6 +22,7 @@ Required properties:
> > > > > > > > >  Optional properties:
> > > > > > > > >    - phys: phandle + phy specifier pair
> > > > > > > > >    - phy-names: must be "usb"
> > > > > > > > > +  - usb-role-switch: use USB role switch to support
> > > > > > > > > + dual-role switch
> > > > > > > >
> > > > > > > > I don't think we can add such a property. At least, we
> > > > > > > > have to add
> > > > > > "renesas,"
> > > > > > > > prefix.
> > > > > > >
> > > > > > > usb_role_switch_get   api uses  "usb-role-switch"  property to get
> role
> > > > > > switch linked with the device.
> > > > > > >
> > > > > > > HD3SS3220 port controller driver gets role switch handle
> > > > > > > linked with the
> > > > > > device using usb_role_switch_get  api.
> > > > > > > That is the reason, I have added " usb-role-switch" property here.
> > > > > > >
> > > > > > > Do you have any other suggestion to get usb role switch handle?
> > > > > >
> > > > > > We can still change the API. Can we use the compatible for this?
> > > > >
> > > > > Do you mean usb_role_switch_get  API needs  to handle compatible
> "usb-x-connector"  wherex= a,b ,c ?
> > > > > Then it uses the graphs api to get the device linked with it and return
> the usb role switch handle.
> > > > > In that case, no need to add  generic "usb-role-switch"  property
> here.
> > > > >
> > > > > Can you please confirm my understanding is correct?
> > > >
> > > > No, I meant the compatible property would have the value
> > > > "usb-role-switch". Your compatible would probable look something
> > > > like this:
> > > >
> > > >         compatible = "renesas,r8a774c0-usb3-peri",
> > > >                      "usb-role-switch";
> > > >
> > > > So the idea would be that the device supplying USB role switch
> > > > functionality, in your case the USB controller, would need to have
> > > > the compatible property containing "usb-role-switch".
> > >
> > > That's not really something a driver could bind to nor provides any
> > > info as to what the h/w is (and how to interact with it).
> >
> > Fair enough. As I said, I don't know much about DT.
> >
> > The problem we are trying to solve here is, how to identify the USB
> > role switch from graph. It's primarily the USB Type-C connector
> > drivers that need to be able to get a handle to the role switch device
> > so they can tell it what to do. Basically the caller of
> > usb_role_switch_get() will have the compatible "usb-c-connector", so
> > it's of no use to us. We need the other endpoint, the role switch.
> >
> > Note: The USB role switch device will be a discrete (or integrated)
> > mux on platforms that have separate USB host and USB device
> > controllers, and on platforms with dual-role capable USB controller
> > (like this one) the USB controller will represent it.

Looks like we have a solution now. Heikki already proposed a new API
"add API to get usb_role_switch by node" https://patchwork.kernel.org/patch/10882555/

We can use this API to find remote endpoint of USB role switch device(renesas usb3) connected with type-c DRP port controller driver(TI HD3SS3220).
I believe in this case, we don't need generic Boolean property " usb-role-switch"

Apart from this, we can add Renesas specific property (renesas, usb-role-switch) to differentiate the
USB role switch associated with type C DRP controller driver from others.

I will send V3 based on this solution. Please let me know if you think otherwise.

Regards,
Biju

> Either way, it should just be the device connected to the connector's USB
> data port in the graph. Though maybe if USB2 and USB3 are different
> controllers that would complicate things.
> 
> > At the moment the function usb_role_switch_get() walks trough the
> > graph of the caller (so the connector) and expects that the
> > remote-endpoint representing the mux will have a boolean property
> > "usb-role-switch".
> 
> Assuming we have that I think it should be in the device
> (controller/mux) node rather than the endpoint node itself.
> 
> It could also be outside of DT in that the controller driver will know the h/w
> can support role switch and can register that capability. IOW, it can be implied
> by the compatible string.
> 
> >
> > I do agree that Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > should not be the file describing that boolean property, but if we had
> > a separate file describing the bindings for just the USB role switch
> > devices, would using it still be OK?
> 
> It should be described somewhere common, yes, but the property itself
> belongs in controller nodes. Generally, we still document where common
> properties are used.
> 
> Though, I prefer the implied by the compatible option. This should work
> unless there's a strong need to find the switch in DT without the switch driver
> being probed or if this is board specific (I wouldn't think so).
> 
> >
> > If not, then can you propose something else? How do we identify the
> > USB role switch endpoint from the graph? Would it be OK to expect the
> > the endpoint subnode to have that boolean property instead?
> >





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux