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? > >
