[V5,4/9] dt-bindings: usb: renesas_usb3: Add renesas,usb-role-switch property
diff mbox series

Message ID 1556097743-12717-5-git-send-email-biju.das@bp.renesas.com
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series
  • Add USB3.0 and TI HD3SS3220 driver support
Related show

Commit Message

Biju Das April 24, 2019, 9:22 a.m. UTC
Add an optional property renesas,usb-role-switch to support
dual role switch for USB Type-C DRP port controller devices
using USB role switch class framework.

Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 V4-->V5
  * No Change
 V3-->V4
  * No Change
 V2-->V3
  * Added optional renesas,usb-role-switch property.
 V1-->V2
  * Added usb-role-switch-property
  * Updated the example with usb-role-switch property.
---
 .../devicetree/bindings/usb/renesas_usb3.txt       | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Rob Herring April 26, 2019, 6:14 p.m. UTC | #1
On Wed, Apr 24, 2019 at 10:22:18AM +0100, Biju Das wrote:
> Add an optional property renesas,usb-role-switch to support
> dual role switch for USB Type-C DRP port controller devices
> using USB role switch class framework.
> 
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> ---
>  V4-->V5
>   * No Change
>  V3-->V4
>   * No Change
>  V2-->V3
>   * Added optional renesas,usb-role-switch property.
>  V1-->V2
>   * Added usb-role-switch-property
>   * Updated the example with usb-role-switch property.
> ---
>  .../devicetree/bindings/usb/renesas_usb3.txt       | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> index 35039e7..f1cb06a 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"
> +  - renesas,usb-role-switch: use USB role switch to handle role switch events

Mediatek and HiSilicon both have same or similar properties in patches 
under review. Please coordinate and document a common property.

Really, I'm wondering why this is needed. Can't you walk the graph to 
the connector and determine if dual role is supported by the connector 
type?

Rob
Biju Das April 29, 2019, 9:39 a.m. UTC | #2
Hi Rob,

Thanks for the feedback.

> Subject: Re: [PATCH V5 4/9] dt-bindings: usb: renesas_usb3: Add
> renesas,usb-role-switch property
> 
> On Wed, Apr 24, 2019 at 10:22:18AM +0100, Biju Das wrote:
> > Add an optional property renesas,usb-role-switch to support dual role
> > switch for USB Type-C DRP port controller devices using USB role
> > switch class framework.
> >
> > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > ---
> >  V4-->V5
> >   * No Change
> >  V3-->V4
> >   * No Change
> >  V2-->V3
> >   * Added optional renesas,usb-role-switch property.
> >  V1-->V2
> >   * Added usb-role-switch-property
> >   * Updated the example with usb-role-switch property.
> > ---
> >  .../devicetree/bindings/usb/renesas_usb3.txt       | 22
> ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > index 35039e7..f1cb06a 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"
> > +  - renesas,usb-role-switch: use USB role switch to handle role
> > + switch events
> 
> Mediatek and HiSilicon both have same or similar properties in patches under
> review. Please coordinate and document a common property.

As per R-Car Gen3 boards design. The USB3.0 port  on the boards (Salvator-xs and Ebisu) has a USB3.0 Type-A receptor.
For debug purpose , the same port can be used as peripheral  using force_b_device mode on debugfs.
Ie, we can use force_b_device to switch the role on this boards.

Where as RZ/G2E board is having USB 3.0 type-C connector.  So the driver needs to know whether to use debugfs based
dual role switch or non-debugfs based dual role switch(type-C). That is the reason I have added this property.

> Really, I'm wondering why this is needed. Can't you walk the graph to the
> connector and determine if dual role is supported by the connector type?

Yes, Basically we don't need this property. I could walk through the graph and determine 
the role supported by connector type

Please find the example code which will be used in the driver.

+static bool is_ext_dual_role_usb_connector_available(struct device *dev)
+{
+       struct device_node *np = dev->of_node;
+       struct device_node *parent;
+       struct device_node *child;
+       bool ret = false;
+       const char *role_type = NULL;
+
+       child = of_graph_get_endpoint_by_regs(np, -1, -1);
+       if (!child)
+               return ret;
+
+       parent = of_graph_get_remote_port_parent(child);
+       of_node_put(child);
+       child = of_get_child_by_name(parent, "connector");
+       of_node_put(parent);
+       if (!child)
+               return ret;
+
+       if (of_device_is_compatible(child, "usb-c-connector")) {
+               of_property_read_string(child, "data-role", &role_type);
+               if (role_type && (!strncmp(role_type, "dual", strlen("dual"))))
+                       ret = true;
+       }
+
+       of_node_put(child);
+       return ret;
+}

Regards,
Biju
Biju Das May 3, 2019, 6:39 a.m. UTC | #3
Hi Rob,


> Subject: RE: [PATCH V5 4/9] dt-bindings: usb: renesas_usb3: Add
> renesas,usb-role-switch property
> > On Wed, Apr 24, 2019 at 10:22:18AM +0100, Biju Das wrote:
> > > Add an optional property renesas,usb-role-switch to support dual
> > > role switch for USB Type-C DRP port controller devices using USB
> > > role switch class framework.
> > >
> > > Signed-off-by: Biju Das <biju.das@bp.renesas.com>
> > > ---
> > >  V4-->V5
> > >   * No Change
> > >  V3-->V4
> > >   * No Change
> > >  V2-->V3
> > >   * Added optional renesas,usb-role-switch property.
> > >  V1-->V2
> > >   * Added usb-role-switch-property
> > >   * Updated the example with usb-role-switch property.
> > > ---
> > >  .../devicetree/bindings/usb/renesas_usb3.txt       | 22
> > ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > > b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
> > > index 35039e7..f1cb06a 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"
> > > +  - renesas,usb-role-switch: use USB role switch to handle role
> > > + switch events
> >
> > Mediatek and HiSilicon both have same or similar properties in patches
> > under review. Please coordinate and document a common property.
> 
> As per R-Car Gen3 boards design. The USB3.0 port  on the boards (Salvator-xs
> and Ebisu) has a USB3.0 Type-A receptor.
> For debug purpose , the same port can be used as peripheral  using
> force_b_device mode on debugfs.
> Ie, we can use force_b_device to switch the role on this boards.
> 
> Where as RZ/G2E board is having USB 3.0 type-C connector.  So the driver
> needs to know whether to use debugfs based dual role switch or non-
> debugfs based dual role switch(type-C). That is the reason I have added this
> property.
> 
> > Really, I'm wondering why this is needed. Can't you walk the graph to
> > the connector and determine if dual role is supported by the connector
> type?
> 
> Yes, Basically we don't need this property. I could walk through the graph and
> determine the role supported by connector type
> 
> Please find the example code which will be used in the driver.
> 
> +static bool is_ext_dual_role_usb_connector_available(struct device
> +*dev) {
> +       struct device_node *np = dev->of_node;
> +       struct device_node *parent;
> +       struct device_node *child;
> +       bool ret = false;
> +       const char *role_type = NULL;
> +
> +       child = of_graph_get_endpoint_by_regs(np, -1, -1);
> +       if (!child)
> +               return ret;
> +
> +       parent = of_graph_get_remote_port_parent(child);
> +       of_node_put(child);
> +       child = of_get_child_by_name(parent, "connector");
> +       of_node_put(parent);
> +       if (!child)
> +               return ret;
> +
> +       if (of_device_is_compatible(child, "usb-c-connector")) {
> +               of_property_read_string(child, "data-role", &role_type);
> +               if (role_type && (!strncmp(role_type, "dual", strlen("dual"))))
> +                       ret = true;
> +       }
> +
> +       of_node_put(child);
> +       return ret;
> +}

Since we are introducing "usb-role-switch " common property[1],
I feel using the common-property make things simpler compared to walking through the graph. 

Are you happy with using the  common property?  Or  still prefer walk through the graph solution?
Please let me  know.

[1] https://patchwork.kernel.org/patch/10920909/

Regards,
Biju

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/usb/renesas_usb3.txt b/Documentation/devicetree/bindings/usb/renesas_usb3.txt
index 35039e7..f1cb06a 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"
+  - renesas,usb-role-switch: use USB role switch to handle role switch events
 
 Example of R-Car H3 ES1.x:
 	usb3_peri0: usb@ee020000 {
@@ -39,3 +40,24 @@  Example of R-Car H3 ES1.x:
 		interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cpg CPG_MOD 327>;
 	};
+
+Example of RZ/G2E:
+	usb3_peri0: usb@ee020000 {
+		compatible = "renesas,r8a774c0-usb3-peri",
+			     "renesas,rcar-gen3-usb3-peri";
+		reg = <0 0xee020000 0 0x400>;
+		interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cpg CPG_MOD 328>;
+		companion = <&xhci0>;
+		renesas,usb-role-switch;
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			usb3peri_role_switch: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&hd3ss3220_ep>;
+			};
+		};
+	};