[PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Add properties for mode, orientation and USB data role switches for
Type C connectors. When available, these will allow the Type C connector
class port driver to configure the various switches according to USB PD
information (like orientation, alt mode etc.) provided by the Chrome OS
EC controller.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
index 6d7396ab8bee..b5814640aa32 100644
--- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
+++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
@@ -21,7 +21,21 @@ properties:
     const: google,cros-ec-typec
 
   connector:
-    $ref: /schemas/connector/usb-connector.yaml#
+    allOf:
+      - $ref: /schemas/connector/usb-connector.yaml#
+      - type: object
+        properties:
+          mode-switch:
+            description: Reference to a DT node for the USB Type C Multiplexer
+              controlling the data lines routing for this connector.
+
+          orientation-switch:
+            description: Reference to a DT node for the USB Type C orientation
+              switch for this connector.
+
+          usb-role-switch:
+            description: Reference to a DT node for the USB Data role switch
+              for this connector.
 
 required:
   - compatible
@@ -49,6 +63,17 @@ examples:
             data-role = "dual";
             try-power-role = "source";
           };
+
+          connector@1 {
+            compatible = "usb-c-connector";
+            reg = <1>;
+            power-role = "dual";
+            data-role = "host";
+            try-power-role = "source";
+            mode-switch = <&typec_mux>;
+            orientation-switch = <&typec_orientation_switch>;
+            usb-role-switch = <&typec_mux>;
+          };
         };
       };
     };
-- 
2.26.1.301.g55bc3eb7cb9-goog

[PATCH 2/2] platform/chrome: typec: Register Type C switches

[Prashant Malani]

Register Type C mux and switch handles, when provided via firmware
bindings. These will allow the cros-ec-typec driver, and also alternate
mode drivers to configure connected Muxes correctly, according to PD
information retrieved from the Chrome OS EC.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/platform/chrome/cros_ec_typec.c | 47 +++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index eda57db26f8d..324ead297c4d 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -14,6 +14,8 @@
 #include <linux/platform_data/cros_usbpd_notify.h>
 #include <linux/platform_device.h>
 #include <linux/usb/typec.h>
+#include <linux/usb/typec_mux.h>
+#include <linux/usb/role.h>
 
 #define DRV_NAME "cros-ec-typec"
 
@@ -25,6 +27,9 @@ struct cros_typec_port {
 	struct typec_partner *partner;
 	/* Port partner PD identity info. */
 	struct usb_pd_identity p_identity;
+	struct typec_switch *ori_sw;
+	struct typec_mux *mux;
+	struct usb_role_switch *role_sw;
 };
 
 /* Platform-specific data for the Chrome OS EC Type C controller. */
@@ -84,6 +89,40 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
 	return 0;
 }
 
+static int cros_typec_get_switch_handles(struct cros_typec_port *port,
+					 struct fwnode_handle *fwnode,
+					 struct device *dev)
+{
+	port->mux = fwnode_typec_mux_get(fwnode, NULL);
+	if (IS_ERR(port->mux)) {
+		dev_info(dev, "Mux handle not found.\n");
+		goto mux_err;
+	}
+
+	port->ori_sw = fwnode_typec_switch_get(fwnode);
+	if (IS_ERR(port->ori_sw)) {
+		dev_info(dev, "Orientation switch handle not found.\n");
+		goto ori_sw_err;
+	}
+
+	port->role_sw = fwnode_usb_role_switch_get(fwnode);
+	if (IS_ERR(port->role_sw)) {
+		dev_info(dev, "USB role switch handle not found.\n");
+		goto role_sw_err;
+	}
+
+	return 0;
+
+role_sw_err:
+	usb_role_switch_put(port->role_sw);
+ori_sw_err:
+	typec_switch_put(port->ori_sw);
+mux_err:
+	typec_mux_put(port->mux);
+
+	return -ENODEV;
+}
+
 static void cros_unregister_ports(struct cros_typec_data *typec)
 {
 	int i;
@@ -91,6 +130,9 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
 	for (i = 0; i < typec->num_ports; i++) {
 		if (!typec->ports[i])
 			continue;
+		usb_role_switch_put(typec->ports[i]->role_sw);
+		typec_switch_put(typec->ports[i]->ori_sw);
+		typec_mux_put(typec->ports[i]->mux);
 		typec_unregister_port(typec->ports[i]->port);
 	}
 }
@@ -153,6 +195,11 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
 			ret = PTR_ERR(cros_port->port);
 			goto unregister_ports;
 		}
+
+		ret = cros_typec_get_switch_handles(cros_port, fwnode, dev);
+		if (ret)
+			dev_info(dev, "No switch control for port %d\n",
+				 port_num);
 	}
 
 	return 0;
-- 
2.26.1.301.g55bc3eb7cb9-goog

Re: [PATCH 2/2] platform/chrome: typec: Register Type C switches

[Heikki Krogerus]

On Wed, Apr 22, 2020 at 03:22:41PM -0700, Prashant Malani wrote:
> Register Type C mux and switch handles, when provided via firmware
> bindings. These will allow the cros-ec-typec driver, and also alternate
> mode drivers to configure connected Muxes correctly, according to PD
> information retrieved from the Chrome OS EC.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@intel.linux.com>

> ---
>  drivers/platform/chrome/cros_ec_typec.c | 47 +++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index eda57db26f8d..324ead297c4d 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -14,6 +14,8 @@
>  #include <linux/platform_data/cros_usbpd_notify.h>
>  #include <linux/platform_device.h>
>  #include <linux/usb/typec.h>
> +#include <linux/usb/typec_mux.h>
> +#include <linux/usb/role.h>
>  
>  #define DRV_NAME "cros-ec-typec"
>  
> @@ -25,6 +27,9 @@ struct cros_typec_port {
>  	struct typec_partner *partner;
>  	/* Port partner PD identity info. */
>  	struct usb_pd_identity p_identity;
> +	struct typec_switch *ori_sw;
> +	struct typec_mux *mux;
> +	struct usb_role_switch *role_sw;
>  };
>  
>  /* Platform-specific data for the Chrome OS EC Type C controller. */
> @@ -84,6 +89,40 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
>  	return 0;
>  }
>  
> +static int cros_typec_get_switch_handles(struct cros_typec_port *port,
> +					 struct fwnode_handle *fwnode,
> +					 struct device *dev)
> +{
> +	port->mux = fwnode_typec_mux_get(fwnode, NULL);
> +	if (IS_ERR(port->mux)) {
> +		dev_info(dev, "Mux handle not found.\n");
> +		goto mux_err;
> +	}
> +
> +	port->ori_sw = fwnode_typec_switch_get(fwnode);
> +	if (IS_ERR(port->ori_sw)) {
> +		dev_info(dev, "Orientation switch handle not found.\n");
> +		goto ori_sw_err;
> +	}
> +
> +	port->role_sw = fwnode_usb_role_switch_get(fwnode);
> +	if (IS_ERR(port->role_sw)) {
> +		dev_info(dev, "USB role switch handle not found.\n");
> +		goto role_sw_err;
> +	}
> +
> +	return 0;
> +
> +role_sw_err:
> +	usb_role_switch_put(port->role_sw);
> +ori_sw_err:
> +	typec_switch_put(port->ori_sw);
> +mux_err:
> +	typec_mux_put(port->mux);
> +
> +	return -ENODEV;
> +}
> +
>  static void cros_unregister_ports(struct cros_typec_data *typec)
>  {
>  	int i;
> @@ -91,6 +130,9 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
>  	for (i = 0; i < typec->num_ports; i++) {
>  		if (!typec->ports[i])
>  			continue;
> +		usb_role_switch_put(typec->ports[i]->role_sw);
> +		typec_switch_put(typec->ports[i]->ori_sw);
> +		typec_mux_put(typec->ports[i]->mux);
>  		typec_unregister_port(typec->ports[i]->port);
>  	}
>  }
> @@ -153,6 +195,11 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
>  			ret = PTR_ERR(cros_port->port);
>  			goto unregister_ports;
>  		}
> +
> +		ret = cros_typec_get_switch_handles(cros_port, fwnode, dev);
> +		if (ret)
> +			dev_info(dev, "No switch control for port %d\n",
> +				 port_num);
>  	}
>  
>  	return 0;
> -- 
> 2.26.1.301.g55bc3eb7cb9-goog

thanks,

-- 
heikki

Re: [PATCH 2/2] platform/chrome: typec: Register Type C switches

[Enric Balletbo i Serra]

Hi Prashant,

Thank you for your patch.

On 23/4/20 0:22, Prashant Malani wrote:
> Register Type C mux and switch handles, when provided via firmware
> bindings. These will allow the cros-ec-typec driver, and also alternate
> mode drivers to configure connected Muxes correctly, according to PD
> information retrieved from the Chrome OS EC.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 47 +++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index eda57db26f8d..324ead297c4d 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -14,6 +14,8 @@
>  #include <linux/platform_data/cros_usbpd_notify.h>
>  #include <linux/platform_device.h>
>  #include <linux/usb/typec.h>
> +#include <linux/usb/typec_mux.h>
> +#include <linux/usb/role.h>
>  
>  #define DRV_NAME "cros-ec-typec"
>  
> @@ -25,6 +27,9 @@ struct cros_typec_port {
>  	struct typec_partner *partner;
>  	/* Port partner PD identity info. */
>  	struct usb_pd_identity p_identity;
> +	struct typec_switch *ori_sw;
> +	struct typec_mux *mux;
> +	struct usb_role_switch *role_sw;
>  };
>  
>  /* Platform-specific data for the Chrome OS EC Type C controller. */
> @@ -84,6 +89,40 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
>  	return 0;
>  }
>  
> +static int cros_typec_get_switch_handles(struct cros_typec_port *port,
> +					 struct fwnode_handle *fwnode,
> +					 struct device *dev)
> +{
> +	port->mux = fwnode_typec_mux_get(fwnode, NULL);
> +	if (IS_ERR(port->mux)) {

Should you return an error if NULL is returned (IS_ERR_OR_NULL) ? I think that
fwnode_typec_mux_get can return NULL too.


> +		dev_info(dev, "Mux handle not found.\n");
> +		goto mux_err;
> +	}
> +
> +	port->ori_sw = fwnode_typec_switch_get(fwnode);
> +	if (IS_ERR(port->ori_sw)) {

ditto

> +		dev_info(dev, "Orientation switch handle not found.\n");
> +		goto ori_sw_err;
> +	}
> +
> +	port->role_sw = fwnode_usb_role_switch_get(fwnode);
> +	if (IS_ERR(port->role_sw)) {

ditto

> +		dev_info(dev, "USB role switch handle not found.\n");
> +		goto role_sw_err;
> +	}
> +
> +	return 0;
> +
> +role_sw_err:
> +	usb_role_switch_put(port->role_sw);
> +ori_sw_err:
> +	typec_switch_put(port->ori_sw);
> +mux_err:
> +	typec_mux_put(port->mux);
> +
> +	return -ENODEV;
> +}
> +
>  static void cros_unregister_ports(struct cros_typec_data *typec)
>  {
>  	int i;
> @@ -91,6 +130,9 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
>  	for (i = 0; i < typec->num_ports; i++) {
>  		if (!typec->ports[i])
>  			continue;
> +		usb_role_switch_put(typec->ports[i]->role_sw);
> +		typec_switch_put(typec->ports[i]->ori_sw);
> +		typec_mux_put(typec->ports[i]->mux);
>  		typec_unregister_port(typec->ports[i]->port);
>  	}
>  }
> @@ -153,6 +195,11 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
>  			ret = PTR_ERR(cros_port->port);
>  			goto unregister_ports;
>  		}
> +
> +		ret = cros_typec_get_switch_handles(cros_port, fwnode, dev);
> +		if (ret)
> +			dev_info(dev, "No switch control for port %d\n",
> +				 port_num);

When drivers are working, they should not spit out any messages, make
this dev_dbg() at the most. Be quiet, please.


>  	}
>  
>  	return 0;
> 

Re: [PATCH 2/2] platform/chrome: typec: Register Type C switches

[Prashant Malani]

Hi Enric,

Thanks for your review. Kindly see inline:

On Wed, Apr 29, 2020 at 3:22 PM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Prashant,
>
> Thank you for your patch.
>
> On 23/4/20 0:22, Prashant Malani wrote:
> > Register Type C mux and switch handles, when provided via firmware
> > bindings. These will allow the cros-ec-typec driver, and also alternate
> > mode drivers to configure connected Muxes correctly, according to PD
> > information retrieved from the Chrome OS EC.
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >  drivers/platform/chrome/cros_ec_typec.c | 47 +++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index eda57db26f8d..324ead297c4d 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -14,6 +14,8 @@
> >  #include <linux/platform_data/cros_usbpd_notify.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/usb/typec.h>
> > +#include <linux/usb/typec_mux.h>
> > +#include <linux/usb/role.h>
> >
> >  #define DRV_NAME "cros-ec-typec"
> >
> > @@ -25,6 +27,9 @@ struct cros_typec_port {
> >       struct typec_partner *partner;
> >       /* Port partner PD identity info. */
> >       struct usb_pd_identity p_identity;
> > +     struct typec_switch *ori_sw;
> > +     struct typec_mux *mux;
> > +     struct usb_role_switch *role_sw;
> >  };
> >
> >  /* Platform-specific data for the Chrome OS EC Type C controller. */
> > @@ -84,6 +89,40 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
> >       return 0;
> >  }
> >
> > +static int cros_typec_get_switch_handles(struct cros_typec_port *port,
> > +                                      struct fwnode_handle *fwnode,
> > +                                      struct device *dev)
> > +{
> > +     port->mux = fwnode_typec_mux_get(fwnode, NULL);
> > +     if (IS_ERR(port->mux)) {
>
> Should you return an error if NULL is returned (IS_ERR_OR_NULL) ? I think that
> fwnode_typec_mux_get can return NULL too.
I think returning NULL can be considered "not an error" for devices
that don't have kernel-controlled muxes (which won't have this
property defined).
So this check should be fine as is.
>
>
> > +             dev_info(dev, "Mux handle not found.\n");
> > +             goto mux_err;
> > +     }
> > +
> > +     port->ori_sw = fwnode_typec_switch_get(fwnode);
> > +     if (IS_ERR(port->ori_sw)) {
>
> ditto
>
> > +             dev_info(dev, "Orientation switch handle not found.\n");
> > +             goto ori_sw_err;
> > +     }
> > +
> > +     port->role_sw = fwnode_usb_role_switch_get(fwnode);
> > +     if (IS_ERR(port->role_sw)) {
>
> ditto
>
> > +             dev_info(dev, "USB role switch handle not found.\n");
> > +             goto role_sw_err;
> > +     }
> > +
> > +     return 0;
> > +
> > +role_sw_err:
> > +     usb_role_switch_put(port->role_sw);
> > +ori_sw_err:
> > +     typec_switch_put(port->ori_sw);
> > +mux_err:
> > +     typec_mux_put(port->mux);
> > +
> > +     return -ENODEV;
> > +}
> > +
> >  static void cros_unregister_ports(struct cros_typec_data *typec)
> >  {
> >       int i;
> > @@ -91,6 +130,9 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
> >       for (i = 0; i < typec->num_ports; i++) {
> >               if (!typec->ports[i])
> >                       continue;
> > +             usb_role_switch_put(typec->ports[i]->role_sw);
> > +             typec_switch_put(typec->ports[i]->ori_sw);
> > +             typec_mux_put(typec->ports[i]->mux);
> >               typec_unregister_port(typec->ports[i]->port);
> >       }
> >  }
> > @@ -153,6 +195,11 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
> >                       ret = PTR_ERR(cros_port->port);
> >                       goto unregister_ports;
> >               }
> > +
> > +             ret = cros_typec_get_switch_handles(cros_port, fwnode, dev);
> > +             if (ret)
> > +                     dev_info(dev, "No switch control for port %d\n",
> > +                              port_num);
>
> When drivers are working, they should not spit out any messages, make
> this dev_dbg() at the most. Be quiet, please.
Ack. Will update this in the next version.
>
>
> >       }
> >
> >       return 0;
> >

Re: [PATCH 2/2] platform/chrome: typec: Register Type C switches

[Enric Balletbo i Serra]

Hi Prashant,


On 30/4/20 1:02, Prashant Malani wrote:
> Hi Enric,
> 
> Thanks for your review. Kindly see inline:
> 
> On Wed, Apr 29, 2020 at 3:22 PM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>>
>> Hi Prashant,
>>
>> Thank you for your patch.
>>
>> On 23/4/20 0:22, Prashant Malani wrote:
>>> Register Type C mux and switch handles, when provided via firmware
>>> bindings. These will allow the cros-ec-typec driver, and also alternate
>>> mode drivers to configure connected Muxes correctly, according to PD
>>> information retrieved from the Chrome OS EC.
>>>
>>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
>>> ---
>>>  drivers/platform/chrome/cros_ec_typec.c | 47 +++++++++++++++++++++++++
>>>  1 file changed, 47 insertions(+)
>>>
>>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
>>> index eda57db26f8d..324ead297c4d 100644
>>> --- a/drivers/platform/chrome/cros_ec_typec.c
>>> +++ b/drivers/platform/chrome/cros_ec_typec.c
>>> @@ -14,6 +14,8 @@
>>>  #include <linux/platform_data/cros_usbpd_notify.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/usb/typec.h>
>>> +#include <linux/usb/typec_mux.h>
>>> +#include <linux/usb/role.h>
>>>
>>>  #define DRV_NAME "cros-ec-typec"
>>>
>>> @@ -25,6 +27,9 @@ struct cros_typec_port {
>>>       struct typec_partner *partner;
>>>       /* Port partner PD identity info. */
>>>       struct usb_pd_identity p_identity;
>>> +     struct typec_switch *ori_sw;
>>> +     struct typec_mux *mux;
>>> +     struct usb_role_switch *role_sw;
>>>  };
>>>
>>>  /* Platform-specific data for the Chrome OS EC Type C controller. */
>>> @@ -84,6 +89,40 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
>>>       return 0;
>>>  }
>>>
>>> +static int cros_typec_get_switch_handles(struct cros_typec_port *port,
>>> +                                      struct fwnode_handle *fwnode,
>>> +                                      struct device *dev)
>>> +{
>>> +     port->mux = fwnode_typec_mux_get(fwnode, NULL);
>>> +     if (IS_ERR(port->mux)) {
>>
>> Should you return an error if NULL is returned (IS_ERR_OR_NULL) ? I think that
>> fwnode_typec_mux_get can return NULL too.
> I think returning NULL can be considered "not an error" for devices
> that don't have kernel-controlled muxes (which won't have this
> property defined).
> So this check should be fine as is.
>>
>>
>>> +             dev_info(dev, "Mux handle not found.\n");
>>> +             goto mux_err;
>>> +     }
>>> +
>>> +     port->ori_sw = fwnode_typec_switch_get(fwnode);
>>> +     if (IS_ERR(port->ori_sw)) {
>>
>> ditto
>>
>>> +             dev_info(dev, "Orientation switch handle not found.\n");
>>> +             goto ori_sw_err;
>>> +     }
>>> +
>>> +     port->role_sw = fwnode_usb_role_switch_get(fwnode);
>>> +     if (IS_ERR(port->role_sw)) {
>>
>> ditto
>>
>>> +             dev_info(dev, "USB role switch handle not found.\n");
>>> +             goto role_sw_err;
>>> +     }
>>> +
>>> +     return 0;
>>> +
>>> +role_sw_err:
>>> +     usb_role_switch_put(port->role_sw);

I see, and put checks for NULL, so it's fine. Thanks for the clarification.

>>> +ori_sw_err:
>>> +     typec_switch_put(port->ori_sw);
>>> +mux_err:
>>> +     typec_mux_put(port->mux);
>>> +
>>> +     return -ENODEV;
>>> +}
>>> +
>>>  static void cros_unregister_ports(struct cros_typec_data *typec)
>>>  {
>>>       int i;
>>> @@ -91,6 +130,9 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
>>>       for (i = 0; i < typec->num_ports; i++) {
>>>               if (!typec->ports[i])
>>>                       continue;
>>> +             usb_role_switch_put(typec->ports[i]->role_sw);
>>> +             typec_switch_put(typec->ports[i]->ori_sw);
>>> +             typec_mux_put(typec->ports[i]->mux);
>>>               typec_unregister_port(typec->ports[i]->port);
>>>       }
>>>  }
>>> @@ -153,6 +195,11 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
>>>                       ret = PTR_ERR(cros_port->port);
>>>                       goto unregister_ports;
>>>               }
>>> +
>>> +             ret = cros_typec_get_switch_handles(cros_port, fwnode, dev);
>>> +             if (ret)
>>> +                     dev_info(dev, "No switch control for port %d\n",
>>> +                              port_num);
>>
>> When drivers are working, they should not spit out any messages, make
>> this dev_dbg() at the most. Be quiet, please.
> Ack. Will update this in the next version.
>>
>>
>>>       }
>>>
>>>       return 0;
>>>

Re: [PATCH 2/2] platform/chrome: typec: Register Type C switches

[Enric Balletbo i Serra]

Hi Prashant,

Following my previous comments ...

On 23/4/20 0:22, Prashant Malani wrote:
> Register Type C mux and switch handles, when provided via firmware
> bindings. These will allow the cros-ec-typec driver, and also alternate
> mode drivers to configure connected Muxes correctly, according to PD
> information retrieved from the Chrome OS EC.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 47 +++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index eda57db26f8d..324ead297c4d 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -14,6 +14,8 @@
>  #include <linux/platform_data/cros_usbpd_notify.h>
>  #include <linux/platform_device.h>
>  #include <linux/usb/typec.h>
> +#include <linux/usb/typec_mux.h>
> +#include <linux/usb/role.h>
>  
>  #define DRV_NAME "cros-ec-typec"
>  
> @@ -25,6 +27,9 @@ struct cros_typec_port {
>  	struct typec_partner *partner;
>  	/* Port partner PD identity info. */
>  	struct usb_pd_identity p_identity;
> +	struct typec_switch *ori_sw;
> +	struct typec_mux *mux;
> +	struct usb_role_switch *role_sw;
>  };
>  
>  /* Platform-specific data for the Chrome OS EC Type C controller. */
> @@ -84,6 +89,40 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
>  	return 0;
>  }
>  
> +static int cros_typec_get_switch_handles(struct cros_typec_port *port,
> +					 struct fwnode_handle *fwnode,
> +					 struct device *dev)
> +{
> +	port->mux = fwnode_typec_mux_get(fwnode, NULL);
> +	if (IS_ERR(port->mux)) {
> +		dev_info(dev, "Mux handle not found.\n");

Be quiet also here, dev_dbg at most, as you're ignoring the error anyway at the end.

> +		goto mux_err;
> +	}
> +
> +	port->ori_sw = fwnode_typec_switch_get(fwnode);
> +	if (IS_ERR(port->ori_sw)) {
> +		dev_info(dev, "Orientation switch handle not found.\n");

Same here

> +		goto ori_sw_err;
> +	}
> +
> +	port->role_sw = fwnode_usb_role_switch_get(fwnode);
> +	if (IS_ERR(port->role_sw)) {
> +		dev_info(dev, "USB role switch handle not found.\n");

And here.

> +		goto role_sw_err;
> +	}
> +
> +	return 0;
> +
> +role_sw_err:
> +	usb_role_switch_put(port->role_sw);
> +ori_sw_err:
> +	typec_switch_put(port->ori_sw);
> +mux_err:
> +	typec_mux_put(port->mux);
> +
> +	return -ENODEV;
> +}
> +
>  static void cros_unregister_ports(struct cros_typec_data *typec)
>  {
>  	int i;
> @@ -91,6 +130,9 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
>  	for (i = 0; i < typec->num_ports; i++) {
>  		if (!typec->ports[i])
>  			continue;
> +		usb_role_switch_put(typec->ports[i]->role_sw);
> +		typec_switch_put(typec->ports[i]->ori_sw);
> +		typec_mux_put(typec->ports[i]->mux);
>  		typec_unregister_port(typec->ports[i]->port);
>  	}
>  }
> @@ -153,6 +195,11 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
>  			ret = PTR_ERR(cros_port->port);
>  			goto unregister_ports;
>  		}
> +
> +		ret = cros_typec_get_switch_handles(cros_port, fwnode, dev);
> +		if (ret)
> +			dev_info(dev, "No switch control for port %d\n",
> +				 port_num);
>  	}
>  
>  	return 0;
> 

Re: [PATCH 2/2] platform/chrome: typec: Register Type C switches

[Prashant Malani]

Hi Enric,

On Thu, Apr 30, 2020 at 12:25:10AM +0200, Enric Balletbo i Serra wrote:
> Hi Prashant,
> 
> Following my previous comments ...
> 
> On 23/4/20 0:22, Prashant Malani wrote:
> > Register Type C mux and switch handles, when provided via firmware
> > bindings. These will allow the cros-ec-typec driver, and also alternate
> > mode drivers to configure connected Muxes correctly, according to PD
> > information retrieved from the Chrome OS EC.
> > 
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >  drivers/platform/chrome/cros_ec_typec.c | 47 +++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index eda57db26f8d..324ead297c4d 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -14,6 +14,8 @@
> >  #include <linux/platform_data/cros_usbpd_notify.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/usb/typec.h>
> > +#include <linux/usb/typec_mux.h>
> > +#include <linux/usb/role.h>
> >  
> >  #define DRV_NAME "cros-ec-typec"
> >  
> > @@ -25,6 +27,9 @@ struct cros_typec_port {
> >  	struct typec_partner *partner;
> >  	/* Port partner PD identity info. */
> >  	struct usb_pd_identity p_identity;
> > +	struct typec_switch *ori_sw;
> > +	struct typec_mux *mux;
> > +	struct usb_role_switch *role_sw;
> >  };
> >  
> >  /* Platform-specific data for the Chrome OS EC Type C controller. */
> > @@ -84,6 +89,40 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
> >  	return 0;
> >  }
> >  
> > +static int cros_typec_get_switch_handles(struct cros_typec_port *port,
> > +					 struct fwnode_handle *fwnode,
> > +					 struct device *dev)
> > +{
> > +	port->mux = fwnode_typec_mux_get(fwnode, NULL);
> > +	if (IS_ERR(port->mux)) {
> > +		dev_info(dev, "Mux handle not found.\n");
> 
> Be quiet also here, dev_dbg at most, as you're ignoring the error anyway at the end.
> 

Done.
> > +		goto mux_err;
> > +	}
> > +
> > +	port->ori_sw = fwnode_typec_switch_get(fwnode);
> > +	if (IS_ERR(port->ori_sw)) {
> > +		dev_info(dev, "Orientation switch handle not found.\n");
> 
> Same here
> 
Done.
> > +		goto ori_sw_err;
> > +	}
> > +
> > +	port->role_sw = fwnode_usb_role_switch_get(fwnode);
> > +	if (IS_ERR(port->role_sw)) {
> > +		dev_info(dev, "USB role switch handle not found.\n");
> 
> And here.
> 

Done.

Thanks for the review. I've uploaded a new version of the patches.

Best regards,

-Prashant
> > +		goto role_sw_err;
> > +	}
> > +
> > +	return 0;
> > +
> > +role_sw_err:
> > +	usb_role_switch_put(port->role_sw);
> > +ori_sw_err:
> > +	typec_switch_put(port->ori_sw);
> > +mux_err:
> > +	typec_mux_put(port->mux);
> > +
> > +	return -ENODEV;
> > +}
> > +
> >  static void cros_unregister_ports(struct cros_typec_data *typec)
> >  {
> >  	int i;
> > @@ -91,6 +130,9 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
> >  	for (i = 0; i < typec->num_ports; i++) {
> >  		if (!typec->ports[i])
> >  			continue;
> > +		usb_role_switch_put(typec->ports[i]->role_sw);
> > +		typec_switch_put(typec->ports[i]->ori_sw);
> > +		typec_mux_put(typec->ports[i]->mux);
> >  		typec_unregister_port(typec->ports[i]->port);
> >  	}
> >  }
> > @@ -153,6 +195,11 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
> >  			ret = PTR_ERR(cros_port->port);
> >  			goto unregister_ports;
> >  		}
> > +
> > +		ret = cros_typec_get_switch_handles(cros_port, fwnode, dev);
> > +		if (ret)
> > +			dev_info(dev, "No switch control for port %d\n",
> > +				 port_num);
> >  	}
> >  
> >  	return 0;
> > 

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

(Forgot to add swboyd to the email, so adding now)

On Wed, Apr 22, 2020 at 3:22 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> Add properties for mode, orientation and USB data role switches for
> Type C connectors. When available, these will allow the Type C connector
> class port driver to configure the various switches according to USB PD
> information (like orientation, alt mode etc.) provided by the Chrome OS
> EC controller.
>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> index 6d7396ab8bee..b5814640aa32 100644
> --- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> @@ -21,7 +21,21 @@ properties:
>      const: google,cros-ec-typec
>
>    connector:
> -    $ref: /schemas/connector/usb-connector.yaml#
> +    allOf:
> +      - $ref: /schemas/connector/usb-connector.yaml#
> +      - type: object
> +        properties:
> +          mode-switch:
> +            description: Reference to a DT node for the USB Type C Multiplexer
> +              controlling the data lines routing for this connector.
> +
> +          orientation-switch:
> +            description: Reference to a DT node for the USB Type C orientation
> +              switch for this connector.
> +
> +          usb-role-switch:
> +            description: Reference to a DT node for the USB Data role switch
> +              for this connector.
>
>  required:
>    - compatible
> @@ -49,6 +63,17 @@ examples:
>              data-role = "dual";
>              try-power-role = "source";
>            };
> +
> +          connector@1 {
> +            compatible = "usb-c-connector";
> +            reg = <1>;
> +            power-role = "dual";
> +            data-role = "host";
> +            try-power-role = "source";
> +            mode-switch = <&typec_mux>;
> +            orientation-switch = <&typec_orientation_switch>;
> +            usb-role-switch = <&typec_mux>;
> +          };
>          };
>        };
>      };
> --
> 2.26.1.301.g55bc3eb7cb9-goog
>

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Enric Balletbo i Serra]

Hi Prashant,

Thank you for the patch.

On 23/4/20 0:22, Prashant Malani wrote:
> Add properties for mode, orientation and USB data role switches for
> Type C connectors. When available, these will allow the Type C connector
> class port driver to configure the various switches according to USB PD
> information (like orientation, alt mode etc.) provided by the Chrome OS
> EC controller.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

This patch still needs a review from Rob.

> ---
>  .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> index 6d7396ab8bee..b5814640aa32 100644
> --- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> @@ -21,7 +21,21 @@ properties:
>      const: google,cros-ec-typec
>  
>    connector:
> -    $ref: /schemas/connector/usb-connector.yaml#
> +    allOf:
> +      - $ref: /schemas/connector/usb-connector.yaml#
> +      - type: object
> +        properties:
> +          mode-switch:
> +            description: Reference to a DT node for the USB Type C Multiplexer
> +              controlling the data lines routing for this connector.
> +
> +          orientation-switch:
> +            description: Reference to a DT node for the USB Type C orientation
> +              switch for this connector.
> +
> +          usb-role-switch:
> +            description: Reference to a DT node for the USB Data role switch
> +              for this connector.
>  
>  required:
>    - compatible
> @@ -49,6 +63,17 @@ examples:
>              data-role = "dual";
>              try-power-role = "source";
>            };
> +
> +          connector@1 {
> +            compatible = "usb-c-connector";
> +            reg = <1>;
> +            power-role = "dual";
> +            data-role = "host";
> +            try-power-role = "source";
> +            mode-switch = <&typec_mux>;
> +            orientation-switch = <&typec_orientation_switch>;
> +            usb-role-switch = <&typec_mux>;
> +          };
>          };
>        };
>      };
> 

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 2570 bytes --]

Hi Prashant,

On Wed, Apr 22, 2020 at 03:22:39PM -0700, Prashant Malani wrote:
> Add properties for mode, orientation and USB data role switches for
> Type C connectors. When available, these will allow the Type C connector
> class port driver to configure the various switches according to USB PD
> information (like orientation, alt mode etc.) provided by the Chrome OS
> EC controller.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

Acked-By: Benson Leung <bleung@chromium.org>


Thanks,
Benson

> ---
>  .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> index 6d7396ab8bee..b5814640aa32 100644
> --- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> @@ -21,7 +21,21 @@ properties:
>      const: google,cros-ec-typec
>  
>    connector:
> -    $ref: /schemas/connector/usb-connector.yaml#
> +    allOf:
> +      - $ref: /schemas/connector/usb-connector.yaml#
> +      - type: object
> +        properties:
> +          mode-switch:
> +            description: Reference to a DT node for the USB Type C Multiplexer
> +              controlling the data lines routing for this connector.
> +
> +          orientation-switch:
> +            description: Reference to a DT node for the USB Type C orientation
> +              switch for this connector.
> +
> +          usb-role-switch:
> +            description: Reference to a DT node for the USB Data role switch
> +              for this connector.
>  
>  required:
>    - compatible
> @@ -49,6 +63,17 @@ examples:
>              data-role = "dual";
>              try-power-role = "source";
>            };
> +
> +          connector@1 {
> +            compatible = "usb-c-connector";
> +            reg = <1>;
> +            power-role = "dual";
> +            data-role = "host";
> +            try-power-role = "source";
> +            mode-switch = <&typec_mux>;
> +            orientation-switch = <&typec_orientation_switch>;
> +            usb-role-switch = <&typec_mux>;
> +          };
>          };
>        };
>      };
> -- 
> 2.26.1.301.g55bc3eb7cb9-goog
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Hi Rob,

Apologies in case you've already looked at this, but could you kindly review this patch?

Thanks,

-Prashant

On Wed, Apr 22, 2020 at 03:22:39PM -0700, Prashant Malani wrote:
> Add properties for mode, orientation and USB data role switches for
> Type C connectors. When available, these will allow the Type C connector
> class port driver to configure the various switches according to USB PD
> information (like orientation, alt mode etc.) provided by the Chrome OS
> EC controller.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> index 6d7396ab8bee..b5814640aa32 100644
> --- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> @@ -21,7 +21,21 @@ properties:
>      const: google,cros-ec-typec
>  
>    connector:
> -    $ref: /schemas/connector/usb-connector.yaml#
> +    allOf:
> +      - $ref: /schemas/connector/usb-connector.yaml#
> +      - type: object
> +        properties:
> +          mode-switch:
> +            description: Reference to a DT node for the USB Type C Multiplexer
> +              controlling the data lines routing for this connector.
> +
> +          orientation-switch:
> +            description: Reference to a DT node for the USB Type C orientation
> +              switch for this connector.
> +
> +          usb-role-switch:
> +            description: Reference to a DT node for the USB Data role switch
> +              for this connector.
>  
>  required:
>    - compatible
> @@ -49,6 +63,17 @@ examples:
>              data-role = "dual";
>              try-power-role = "source";
>            };
> +
> +          connector@1 {
> +            compatible = "usb-c-connector";
> +            reg = <1>;
> +            power-role = "dual";
> +            data-role = "host";
> +            try-power-role = "source";
> +            mode-switch = <&typec_mux>;
> +            orientation-switch = <&typec_orientation_switch>;
> +            usb-role-switch = <&typec_mux>;
> +          };
>          };
>        };
>      };
> -- 
> 2.26.1.301.g55bc3eb7cb9-goog
> 

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Rob Herring]

On Wed, Apr 22, 2020 at 03:22:39PM -0700, Prashant Malani wrote:
> Add properties for mode, orientation and USB data role switches for
> Type C connectors. When available, these will allow the Type C connector
> class port driver to configure the various switches according to USB PD
> information (like orientation, alt mode etc.) provided by the Chrome OS
> EC controller.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> index 6d7396ab8bee..b5814640aa32 100644
> --- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> @@ -21,7 +21,21 @@ properties:
>      const: google,cros-ec-typec
>  
>    connector:
> -    $ref: /schemas/connector/usb-connector.yaml#
> +    allOf:
> +      - $ref: /schemas/connector/usb-connector.yaml#
> +      - type: object
> +        properties:

These don't seem CrOS EC specific, so why document them as such. 

> +          mode-switch:
> +            description: Reference to a DT node for the USB Type C Multiplexer
> +              controlling the data lines routing for this connector.

This is for alternate mode muxing I presume.

We already have a mux-control binding. Why not use that here?

> +
> +          orientation-switch:
> +            description: Reference to a DT node for the USB Type C orientation
> +              switch for this connector.

What's in this node?

> +
> +          usb-role-switch:
> +            description: Reference to a DT node for the USB Data role switch
> +              for this connector.
>  
>  required:
>    - compatible
> @@ -49,6 +63,17 @@ examples:
>              data-role = "dual";
>              try-power-role = "source";
>            };
> +
> +          connector@1 {
> +            compatible = "usb-c-connector";
> +            reg = <1>;
> +            power-role = "dual";
> +            data-role = "host";
> +            try-power-role = "source";
> +            mode-switch = <&typec_mux>;
> +            orientation-switch = <&typec_orientation_switch>;
> +            usb-role-switch = <&typec_mux>;
> +          };
>          };
>        };
>      };
> -- 
> 2.26.1.301.g55bc3eb7cb9-goog
> 

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Hi Rob,

Thank you for reviewing the patch. Kindly see my comments inline:

On Mon, May 11, 2020 at 02:28:00PM -0500, Rob Herring wrote:
> On Wed, Apr 22, 2020 at 03:22:39PM -0700, Prashant Malani wrote:
> > Add properties for mode, orientation and USB data role switches for
> > Type C connectors. When available, these will allow the Type C connector
> > class port driver to configure the various switches according to USB PD
> > information (like orientation, alt mode etc.) provided by the Chrome OS
> > EC controller.
> > 
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >  .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
> >  1 file changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > index 6d7396ab8bee..b5814640aa32 100644
> > --- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > @@ -21,7 +21,21 @@ properties:
> >      const: google,cros-ec-typec
> >  
> >    connector:
> > -    $ref: /schemas/connector/usb-connector.yaml#
> > +    allOf:
> > +      - $ref: /schemas/connector/usb-connector.yaml#
> > +      - type: object
> > +        properties:
> 
> These don't seem CrOS EC specific, so why document them as such. 

Are you referring to the "mode-switch", "orientation-switch" and
"usb-role-switch" properties? If so, then yes, they aren't Cros EC
specific. The Type C connector class framework requires the nodes to be
named like this, and the cros-ec-typec driver uses this framework, hence
the description here (the Type C connector class framework doesn't have
any bindings).

Would it be better to add in the description string that Type Connector
class expects these switches to be named this way? :

" Reference to a DT node for the USB Type C Multiplexer controlling the
data lines routing for this connector. This switch is assumed registered
with the Type C connector class framework, which requires it to be named
this way."
> 
> > +          mode-switch:
> > +            description: Reference to a DT node for the USB Type C Multiplexer
> > +              controlling the data lines routing for this connector.
> 
> This is for alternate mode muxing I presume.

Yes, that's right.
> 
> We already have a mux-control binding. Why not use that here?

Heikki might be able to offer more insight into why this is the case,
since the connector class framework seems to expect a phandle and for
the device driver to implement a "set" command. Heikki, would you happen to know?

> 
> > +
> > +          orientation-switch:
> > +            description: Reference to a DT node for the USB Type C orientation
> > +              switch for this connector.
> 
> What's in this node?

Similar to the other "-switch", this will contain a phandle to a device
which can control orientation settings for the Type C Mux. The connector
class API assumes the switches are named this way. For example:

orientation-switch:
https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/usb/typec/mux.c#L64

mode-switch:
https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/usb/typec/mux.c#L258

> 
> > +
> > +          usb-role-switch:
> > +            description: Reference to a DT node for the USB Data role switch
> > +              for this connector.
> >  
> >  required:
> >    - compatible
> > @@ -49,6 +63,17 @@ examples:
> >              data-role = "dual";
> >              try-power-role = "source";
> >            };
> > +
> > +          connector@1 {
> > +            compatible = "usb-c-connector";
> > +            reg = <1>;
> > +            power-role = "dual";
> > +            data-role = "host";
> > +            try-power-role = "source";
> > +            mode-switch = <&typec_mux>;
> > +            orientation-switch = <&typec_orientation_switch>;
> > +            usb-role-switch = <&typec_mux>;
> > +          };
> >          };
> >        };
> >      };
> > -- 
> > 2.26.1.301.g55bc3eb7cb9-goog
> > 

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Heikki Krogerus]

Hi guys,

On Mon, May 11, 2020 at 01:46:35PM -0700, Prashant Malani wrote:
> Hi Rob,
> 
> Thank you for reviewing the patch. Kindly see my comments inline:
> 
> On Mon, May 11, 2020 at 02:28:00PM -0500, Rob Herring wrote:
> > On Wed, Apr 22, 2020 at 03:22:39PM -0700, Prashant Malani wrote:
> > > Add properties for mode, orientation and USB data role switches for
> > > Type C connectors. When available, these will allow the Type C connector
> > > class port driver to configure the various switches according to USB PD
> > > information (like orientation, alt mode etc.) provided by the Chrome OS
> > > EC controller.
> > > 
> > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > ---
> > >  .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
> > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > index 6d7396ab8bee..b5814640aa32 100644
> > > --- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > @@ -21,7 +21,21 @@ properties:
> > >      const: google,cros-ec-typec
> > >  
> > >    connector:
> > > -    $ref: /schemas/connector/usb-connector.yaml#
> > > +    allOf:
> > > +      - $ref: /schemas/connector/usb-connector.yaml#
> > > +      - type: object
> > > +        properties:
> > 
> > These don't seem CrOS EC specific, so why document them as such. 
> 
> Are you referring to the "mode-switch", "orientation-switch" and
> "usb-role-switch" properties? If so, then yes, they aren't Cros EC
> specific. The Type C connector class framework requires the nodes to be
> named like this, and the cros-ec-typec driver uses this framework, hence
> the description here (the Type C connector class framework doesn't have
> any bindings).
> 
> Would it be better to add in the description string that Type Connector
> class expects these switches to be named this way? :
> 
> " Reference to a DT node for the USB Type C Multiplexer controlling the
> data lines routing for this connector. This switch is assumed registered
> with the Type C connector class framework, which requires it to be named
> this way."
> > 
> > > +          mode-switch:
> > > +            description: Reference to a DT node for the USB Type C Multiplexer
> > > +              controlling the data lines routing for this connector.
> > 
> > This is for alternate mode muxing I presume.
> 
> Yes, that's right.
> > 
> > We already have a mux-control binding. Why not use that here?
> 
> Heikki might be able to offer more insight into why this is the case,
> since the connector class framework seems to expect a phandle and for
> the device driver to implement a "set" command. Heikki, would you happen to know?

The mode-switch here would actually represent the "consumer" part in
the mux-control bindings. So the mux-controls would describe the
relationship between the "mode-switch" and the mux controller(s),
while the mode-switch property describes the relationship between
something like USB Type-C Port Manager (or this cros_ec function) and
the "mux consumer".

> > > +
> > > +          orientation-switch:
> > > +            description: Reference to a DT node for the USB Type C orientation
> > > +              switch for this connector.
> > 
> > What's in this node?
> 
> Similar to the other "-switch", this will contain a phandle to a device
> which can control orientation settings for the Type C Mux. The connector
> class API assumes the switches are named this way. For example:
> 
> orientation-switch:
> https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/usb/typec/mux.c#L64
> 
> mode-switch:
> https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/usb/typec/mux.c#L258
> 
> > 
> > > +
> > > +          usb-role-switch:
> > > +            description: Reference to a DT node for the USB Data role switch
> > > +              for this connector.
> > >  
> > >  required:
> > >    - compatible
> > > @@ -49,6 +63,17 @@ examples:
> > >              data-role = "dual";
> > >              try-power-role = "source";
> > >            };
> > > +
> > > +          connector@1 {
> > > +            compatible = "usb-c-connector";
> > > +            reg = <1>;
> > > +            power-role = "dual";
> > > +            data-role = "host";
> > > +            try-power-role = "source";
> > > +            mode-switch = <&typec_mux>;
> > > +            orientation-switch = <&typec_orientation_switch>;
> > > +            usb-role-switch = <&typec_mux>;
> > > +          };
> > >          };
> > >        };
> > >      };
> > > -- 
> > > 2.26.1.301.g55bc3eb7cb9-goog
> > > 

thanks,

-- 
heikki

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Hi folks,

On Tue, May 12, 2020 at 04:41:54PM +0300, Heikki Krogerus wrote:
> Hi guys,
> 
> On Mon, May 11, 2020 at 01:46:35PM -0700, Prashant Malani wrote:
> > Hi Rob,
> > 
> > Thank you for reviewing the patch. Kindly see my comments inline:
> > 
> > On Mon, May 11, 2020 at 02:28:00PM -0500, Rob Herring wrote:
> > > On Wed, Apr 22, 2020 at 03:22:39PM -0700, Prashant Malani wrote:
> > > > Add properties for mode, orientation and USB data role switches for
> > > > Type C connectors. When available, these will allow the Type C connector
> > > > class port driver to configure the various switches according to USB PD
> > > > information (like orientation, alt mode etc.) provided by the Chrome OS
> > > > EC controller.
> > > > 
> > > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > > ---
> > > >  .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
> > > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > > index 6d7396ab8bee..b5814640aa32 100644
> > > > --- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > > +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > > @@ -21,7 +21,21 @@ properties:
> > > >      const: google,cros-ec-typec
> > > >  
> > > >    connector:
> > > > -    $ref: /schemas/connector/usb-connector.yaml#
> > > > +    allOf:
> > > > +      - $ref: /schemas/connector/usb-connector.yaml#
> > > > +      - type: object
> > > > +        properties:
> > > 
> > > These don't seem CrOS EC specific, so why document them as such. 
> > 
> > Are you referring to the "mode-switch", "orientation-switch" and
> > "usb-role-switch" properties? If so, then yes, they aren't Cros EC
> > specific. The Type C connector class framework requires the nodes to be
> > named like this, and the cros-ec-typec driver uses this framework, hence
> > the description here (the Type C connector class framework doesn't have
> > any bindings).
> > 
> > Would it be better to add in the description string that Type Connector
> > class expects these switches to be named this way? :
> > 
> > " Reference to a DT node for the USB Type C Multiplexer controlling the
> > data lines routing for this connector. This switch is assumed registered
> > with the Type C connector class framework, which requires it to be named
> > this way."
> > > 
> > > > +          mode-switch:
> > > > +            description: Reference to a DT node for the USB Type C Multiplexer
> > > > +              controlling the data lines routing for this connector.
> > > 
> > > This is for alternate mode muxing I presume.
> > 
> > Yes, that's right.
> > > 
> > > We already have a mux-control binding. Why not use that here?
> > 
> > Heikki might be able to offer more insight into why this is the case,
> > since the connector class framework seems to expect a phandle and for
> > the device driver to implement a "set" command. Heikki, would you happen to know?
> 
> The mode-switch here would actually represent the "consumer" part in
> the mux-control bindings. So the mux-controls would describe the
> relationship between the "mode-switch" and the mux controller(s),
> while the mode-switch property describes the relationship between
> something like USB Type-C Port Manager (or this cros_ec function) and
> the "mux consumer".
> 

Thanks for the explanation, Heikki.

Hi Rob,

Does the above explanation help clarify the usage here?

If so, shall I upload a new patch version with the additional text
(referencing Type C connector class framework) added to the *-switch
descriptions?

Best regards,

-Prashant
> > > > +
> > > > +          orientation-switch:
> > > > +            description: Reference to a DT node for the USB Type C orientation
> > > > +              switch for this connector.
> > > 
> > > What's in this node?
> > 
> > Similar to the other "-switch", this will contain a phandle to a device
> > which can control orientation settings for the Type C Mux. The connector
> > class API assumes the switches are named this way. For example:
> > 
> > orientation-switch:
> > https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/usb/typec/mux.c#L64
> > 
> > mode-switch:
> > https://elixir.bootlin.com/linux/v5.7-rc2/source/drivers/usb/typec/mux.c#L258
> > 
> > > 
> > > > +
> > > > +          usb-role-switch:
> > > > +            description: Reference to a DT node for the USB Data role switch
> > > > +              for this connector.
> > > >  
> > > >  required:
> > > >    - compatible
> > > > @@ -49,6 +63,17 @@ examples:
> > > >              data-role = "dual";
> > > >              try-power-role = "source";
> > > >            };
> > > > +
> > > > +          connector@1 {
> > > > +            compatible = "usb-c-connector";
> > > > +            reg = <1>;
> > > > +            power-role = "dual";
> > > > +            data-role = "host";
> > > > +            try-power-role = "source";
> > > > +            mode-switch = <&typec_mux>;
> > > > +            orientation-switch = <&typec_orientation_switch>;
> > > > +            usb-role-switch = <&typec_mux>;
> > > > +          };
> > > >          };
> > > >        };
> > > >      };
> > > > -- 
> > > > 2.26.1.301.g55bc3eb7cb9-goog
> > > > 
> 
> thanks,
> 
> -- 
> heikki

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Rob Herring]

On Tue, May 12, 2020 at 7:42 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi guys,
>
> On Mon, May 11, 2020 at 01:46:35PM -0700, Prashant Malani wrote:
> > Hi Rob,
> >
> > Thank you for reviewing the patch. Kindly see my comments inline:
> >
> > On Mon, May 11, 2020 at 02:28:00PM -0500, Rob Herring wrote:
> > > On Wed, Apr 22, 2020 at 03:22:39PM -0700, Prashant Malani wrote:
> > > > Add properties for mode, orientation and USB data role switches for
> > > > Type C connectors. When available, these will allow the Type C connector
> > > > class port driver to configure the various switches according to USB PD
> > > > information (like orientation, alt mode etc.) provided by the Chrome OS
> > > > EC controller.
> > > >
> > > > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > > > ---
> > > >  .../bindings/chrome/google,cros-ec-typec.yaml | 27 ++++++++++++++++++-
> > > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > > index 6d7396ab8bee..b5814640aa32 100644
> > > > --- a/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > > +++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml
> > > > @@ -21,7 +21,21 @@ properties:
> > > >      const: google,cros-ec-typec
> > > >
> > > >    connector:
> > > > -    $ref: /schemas/connector/usb-connector.yaml#
> > > > +    allOf:
> > > > +      - $ref: /schemas/connector/usb-connector.yaml#
> > > > +      - type: object
> > > > +        properties:
> > >
> > > These don't seem CrOS EC specific, so why document them as such.
> >
> > Are you referring to the "mode-switch", "orientation-switch" and
> > "usb-role-switch" properties? If so, then yes, they aren't Cros EC
> > specific. The Type C connector class framework requires the nodes to be
> > named like this, and the cros-ec-typec driver uses this framework, hence
> > the description here (the Type C connector class framework doesn't have
> > any bindings).
> >
> > Would it be better to add in the description string that Type Connector
> > class expects these switches to be named this way? :
> >
> > " Reference to a DT node for the USB Type C Multiplexer controlling the
> > data lines routing for this connector. This switch is assumed registered
> > with the Type C connector class framework, which requires it to be named
> > this way."
> > >
> > > > +          mode-switch:
> > > > +            description: Reference to a DT node for the USB Type C Multiplexer
> > > > +              controlling the data lines routing for this connector.
> > >
> > > This is for alternate mode muxing I presume.
> >
> > Yes, that's right.
> > >
> > > We already have a mux-control binding. Why not use that here?
> >
> > Heikki might be able to offer more insight into why this is the case,
> > since the connector class framework seems to expect a phandle and for
> > the device driver to implement a "set" command. Heikki, would you happen to know?
>
> The mode-switch here would actually represent the "consumer" part in
> the mux-control bindings. So the mux-controls would describe the
> relationship between the "mode-switch" and the mux controller(s),
> while the mode-switch property describes the relationship between
> something like USB Type-C Port Manager (or this cros_ec function) and
> the "mux consumer".

The "USB Type-C Port Manager" is not just the parent node in your case?

Can you point me to what you expect your DT to look like showing the
mode switch node, the connector, the USB host(s), and the DP/HDMI
bridge/output?

Rob

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Hi Rob,

Thanks for reviewing the patch! Kindly see inline:

On Fri, May 29, 2020 at 2:55 PM Rob Herring <robh@kernel.org> wrote:
>
> > > " Reference to a DT node for the USB Type C Multiplexer controlling the
> > > data lines routing for this connector. This switch is assumed registered
> > > with the Type C connector class framework, which requires it to be named
> > > this way."
> > > >
> > > > > +          mode-switch:
> > > > > +            description: Reference to a DT node for the USB Type C Multiplexer
> > > > > +              controlling the data lines routing for this connector.
> > > >
> > > > This is for alternate mode muxing I presume.
> > >
> > > Yes, that's right.
> > > >
> > > > We already have a mux-control binding. Why not use that here?
> > >
> > > Heikki might be able to offer more insight into why this is the case,
> > > since the connector class framework seems to expect a phandle and for
> > > the device driver to implement a "set" command. Heikki, would you happen to know?
> >
> > The mode-switch here would actually represent the "consumer" part in
> > the mux-control bindings. So the mux-controls would describe the
> > relationship between the "mode-switch" and the mux controller(s),
> > while the mode-switch property describes the relationship between
> > something like USB Type-C Port Manager (or this cros_ec function) and
> > the "mux consumer".
>
> The "USB Type-C Port Manager" is not just the parent node in your case?
>
> Can you point me to what you expect your DT to look like showing the
> mode switch node, the connector, the USB host(s), and the DP/HDMI
> bridge/output?

Caveat: I'm not a DT expert and not well-versed with the mux-control
bindings, so Heikki may be able to describe these better.
That said, here is my attempt to show the nodes you requested, cobbled
together from the Rockchip rk3399 DTSI[1] and
swboyd's connector binding example [2].

Nodes truncated and unrelated fields omitted in the interest of brevity:

// Chrome OS EC Type C Port Manager.
typec {
    compatible = "google,cros-ec-typec";
    #address-cells = <1>;
    #size-cells = <0>;

    connector@0 {
        compatible = "usb-c-connector";
        reg = <0>;
        power-role = "dual";
        data-role = "dual";
        try-power-role = "source";
        mode-switch = <&foo_mux>;
        // Other switches can point to the same mux.
        ....
    };
};

// Mux switch
// TODO: Can possibly embed this in the PHY controller node itself?
foo_mux {
    compatible = "vendor,typec-mux";
    mux-gpios = <&gpio_controller 23 GPIO_ACTIVE_HIGH>;

    ports {
        #address-cells = <1>;
        #size-cells = <0>;
        port@0 {
            reg = <0>;
            mux_dp_in: endpoint {
                remote-endpoint = <&dp_phy_out>;
            };
        };

        port@1 {
            reg = <1>;
            mux_usb_in: endpoint1 {
                remote-endpoint = <&usb3_phy_out>;
            };
        };
    };
};

// Type C PHY Controller.
tcphy0: phy@ff7c0000 {
    compatible = "rockchip,rk3399-typec-phy";
    reg = <0x0 0xff7c0000 0x0 0x40000>;
    ...
    tcphy0_dp: phy@dc00000 {
        compatible = "soc,dp-phy";
        reg = <0xdc00000 0x1000>;
        ports {
            port@0 {
                reg = <0>;
                dp_phy_out: endpoint {
                    remote-endpoint = <&mux_dp_in>;
                };
            };
        };
    };

    tcphy0_usb3: phy@db00000 {
        compatible = "soc,usb3-phy";
        reg = <0xdb00000 0x1000>;
        ports {
            port@0 {
                reg = <0>;
                usb3_phy_out: endpoint {
                    remote-endpoint = <&mux_usb3_in>;
                };
            };
        };
    };
};


// USB3 Host controller
usbdrd3_0: usb@fe800000 {
    compatible = "rockchip,rk3399-dwc3";
    #address-cells = <2>;
    #size-cells = <2>;
    clocks = ...;
    clock-names = ...;
    status = "disabled";

    usbdrd_dwc3_0: usb@fe800000 {
        compatible = "snps,dwc3";
        reg = <0x0 0xfe800000 0x0 0x100000>;
        interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH 0>;
        clocks = ...;
        clock-names = ...;
        dr_mode = "otg";
        phys = <&tcphy0_usb3>;
        phy-names = "usb3-phy";
        phy_type = "utmi_wide";
        power-domains = <&power RK3399_PD_USB3>;
        status = "disabled";
    };
};

// DP controller
cdn_dp: dp@fec00000 {
    compatible = "rockchip,rk3399-cdn-dp";
    reg = <0x0 0xfec00000 0x0 0x100000>;
    interrupts = ...;
    clocks = ...;
    clock-names = ...;
    phys = <&tcphy0_dp>;
    ...
    ports {
        dp_in: port {
            #address-cells = <1>;
            #size-cells = <0>;

            dp_in_vopb: endpoint@0 {
                reg = <0>;
                remote-endpoint = <&vopb_out_dp>;
            };

            dp_in_vopl: endpoint@1 {
                reg = <1>;
                remote-endpoint = <&vopl_out_dp>;
            };
        };
    };
};

[1] : https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads/chromeos-5.4/arch/arm64/boot/dts/rockchip/rk3399.dtsi
[2]: https://lkml.org/lkml/2020/2/28/1081

Hope this helps, and my apologies in advance for any errors.

Best regards,

-Prashant

>
> Rob

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Rob Herring]

On Fri, May 29, 2020 at 5:30 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Rob,
>
> Thanks for reviewing the patch! Kindly see inline:
>
> On Fri, May 29, 2020 at 2:55 PM Rob Herring <robh@kernel.org> wrote:
> >
> > > > " Reference to a DT node for the USB Type C Multiplexer controlling the
> > > > data lines routing for this connector. This switch is assumed registered
> > > > with the Type C connector class framework, which requires it to be named
> > > > this way."
> > > > >
> > > > > > +          mode-switch:
> > > > > > +            description: Reference to a DT node for the USB Type C Multiplexer
> > > > > > +              controlling the data lines routing for this connector.
> > > > >
> > > > > This is for alternate mode muxing I presume.
> > > >
> > > > Yes, that's right.
> > > > >
> > > > > We already have a mux-control binding. Why not use that here?
> > > >
> > > > Heikki might be able to offer more insight into why this is the case,
> > > > since the connector class framework seems to expect a phandle and for
> > > > the device driver to implement a "set" command. Heikki, would you happen to know?
> > >
> > > The mode-switch here would actually represent the "consumer" part in
> > > the mux-control bindings. So the mux-controls would describe the
> > > relationship between the "mode-switch" and the mux controller(s),
> > > while the mode-switch property describes the relationship between
> > > something like USB Type-C Port Manager (or this cros_ec function) and
> > > the "mux consumer".
> >
> > The "USB Type-C Port Manager" is not just the parent node in your case?
> >
> > Can you point me to what you expect your DT to look like showing the
> > mode switch node, the connector, the USB host(s), and the DP/HDMI
> > bridge/output?
>
> Caveat: I'm not a DT expert and not well-versed with the mux-control
> bindings, so Heikki may be able to describe these better.
> That said, here is my attempt to show the nodes you requested, cobbled
> together from the Rockchip rk3399 DTSI[1] and
> swboyd's connector binding example [2].
>
> Nodes truncated and unrelated fields omitted in the interest of brevity:
>
> // Chrome OS EC Type C Port Manager.
> typec {
>     compatible = "google,cros-ec-typec";
>     #address-cells = <1>;
>     #size-cells = <0>;
>
>     connector@0 {
>         compatible = "usb-c-connector";
>         reg = <0>;
>         power-role = "dual";
>         data-role = "dual";
>         try-power-role = "source";
>         mode-switch = <&foo_mux>;
>         // Other switches can point to the same mux.
>         ....

The connector is supposed to have 'ports' for USB2, USB3, and Aux
unless the parent is the USB controller.

>     };
> };
>
> // Mux switch
> // TODO: Can possibly embed this in the PHY controller node itself?
> foo_mux {
>     compatible = "vendor,typec-mux";
>     mux-gpios = <&gpio_controller 23 GPIO_ACTIVE_HIGH>;
>
>     ports {
>         #address-cells = <1>;
>         #size-cells = <0>;
>         port@0 {
>             reg = <0>;
>             mux_dp_in: endpoint {
>                 remote-endpoint = <&dp_phy_out>;
>             };
>         };
>
>         port@1 {
>             reg = <1>;
>             mux_usb_in: endpoint1 {
>                 remote-endpoint = <&usb3_phy_out>;
>             };
>         };

This all goes away if you have ports in the connector node. More below.

>     };
> };
>
> // Type C PHY Controller.
> tcphy0: phy@ff7c0000 {
>     compatible = "rockchip,rk3399-typec-phy";
>     reg = <0x0 0xff7c0000 0x0 0x40000>;
>     ...
>     tcphy0_dp: phy@dc00000 {
>         compatible = "soc,dp-phy";
>         reg = <0xdc00000 0x1000>;
>         ports {
>             port@0 {
>                 reg = <0>;
>                 dp_phy_out: endpoint {
>                     remote-endpoint = <&mux_dp_in>;
>                 };
>             };

This is wrong in that 'phys' are not part of the graph. It's the DP
and USB controllers that should be part of the graph. Any phys are
referenced with the phys binding and are not part of the graph.

>         };
>     };
>
>     tcphy0_usb3: phy@db00000 {
>         compatible = "soc,usb3-phy";
>         reg = <0xdb00000 0x1000>;
>         ports {
>             port@0 {
>                 reg = <0>;
>                 usb3_phy_out: endpoint {
>                     remote-endpoint = <&mux_usb3_in>;
>                 };
>             };
>         };
>     };
> };
>
>
> // USB3 Host controller
> usbdrd3_0: usb@fe800000 {
>     compatible = "rockchip,rk3399-dwc3";
>     #address-cells = <2>;
>     #size-cells = <2>;
>     clocks = ...;
>     clock-names = ...;
>     status = "disabled";
>
>     usbdrd_dwc3_0: usb@fe800000 {
>         compatible = "snps,dwc3";
>         reg = <0x0 0xfe800000 0x0 0x100000>;
>         interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH 0>;
>         clocks = ...;
>         clock-names = ...;
>         dr_mode = "otg";
>         phys = <&tcphy0_usb3>;
>         phy-names = "usb3-phy";
>         phy_type = "utmi_wide";
>         power-domains = <&power RK3399_PD_USB3>;
>         status = "disabled";
>     };
> };
>
> // DP controller
> cdn_dp: dp@fec00000 {
>     compatible = "rockchip,rk3399-cdn-dp";
>     reg = <0x0 0xfec00000 0x0 0x100000>;
>     interrupts = ...;
>     clocks = ...;
>     clock-names = ...;
>     phys = <&tcphy0_dp>;
>     ...
>     ports {
>         dp_in: port {
>             #address-cells = <1>;
>             #size-cells = <0>;
>
>             dp_in_vopb: endpoint@0 {
>                 reg = <0>;
>                 remote-endpoint = <&vopb_out_dp>;
>             };
>
>             dp_in_vopl: endpoint@1 {
>                 reg = <1>;
>                 remote-endpoint = <&vopl_out_dp>;
>             };
>         };

This should have an output port and then that is connected to the USB
connector. Given that DP is muxed with the USB SS signals, port@1
(defined as USB SS) should then have 2 endpoints.

Then the only new thing here is how to represent the GPIO line
controlling the mux. Normally, I'd expect this to be in the parent of
the connector (the type-C controller), but since you have multiple
connectors, that doesn't work so well. So you can put 'mux-gpios' in
the port@1 node. (Or maybe it should be 'mux-controls' with a gpio mux
defined elsewhere).

Rob

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Hi Rob,

Thanks again for the comments and feedback. Kindly see responses inline:

(Trimming unrelated text from thread):

On Tue, Jun 09, 2020 at 02:30:11PM -0600, Rob Herring wrote:
> On Fri, May 29, 2020 at 5:30 PM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > Nodes truncated and unrelated fields omitted in the interest of brevity:
> >
> > // Chrome OS EC Type C Port Manager.
> > typec {
> >     compatible = "google,cros-ec-typec";
> >     #address-cells = <1>;
> >     #size-cells = <0>;
> >
> >     connector@0 {
> >         compatible = "usb-c-connector";
> >         reg = <0>;
> >         power-role = "dual";
> >         data-role = "dual";
> >         try-power-role = "source";
> >         mode-switch = <&foo_mux>;
> >         // Other switches can point to the same mux.
> >         ....
> 
> The connector is supposed to have 'ports' for USB2, USB3, and Aux
> unless the parent is the USB controller.
Understood; so, coupled with Heikki's explanation (see below for where
I've pasted it), would it be something like so? (adding inline to the connector
node definition):

            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;
                    usb_con_hs: endpoint {
                        remote-endpoint = <&foo_usb_hs_controller>;
                    };
                };

                port@1 {
                    reg = <1>;
                    usb_con0_ss: endpoint@0 {
                        remote-endpoint = <&mode_mux_in>;
                    };
                };

                port@2 {
                    reg = <2>;
                    usb_con_sbu: endpoint {
                        remote-endpoint = <&foo_dp_aux>;
                    };
                };
            };

> 
> >     };
> > };
> >
> > // Mux switch
> > // TODO: Can possibly embed this in the PHY controller node itself?
> > foo_mux {
> >     compatible = "vendor,typec-mux";
> >     mux-gpios = <&gpio_controller 23 GPIO_ACTIVE_HIGH>;
> >
> >     ports {
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> >         port@0 {
> >             reg = <0>;
> >             mux_dp_in: endpoint {
> >                 remote-endpoint = <&dp_phy_out>;
> >             };
> >         };
> >
> >         port@1 {
> >             reg = <1>;
> >             mux_usb_in: endpoint1 {
> >                 remote-endpoint = <&usb3_phy_out>;
> >             };
> >         };
> 
> This all goes away if you have ports in the connector node. More below.

I think I my earlier example may have been a bit incorrect. Per Heikki's
explanation of who the consumer of the mux-control bindings is, the
foo_mux definition would now be like so:

foo_mux {
    compatible = "typec-mux-consumer";
    // This can be expanded to add more mux-controls for orientation,
    // data role etc.
    mux-controls = <&mode_mux_controller>;
    mux-control-names = "mode";
    #address-cells = <1>;
    #size-cells = <0>;

    port@0 {
        reg = <0>;
        mode_mux_in: endpoint {
            remote-endpoint = <&usb_con0_ss>
        };
    };

    port@1 {
        reg = <1>;
        mode_mux_out_usb3: endpoint {
            remote-endpoint = <&usb3_0_ep>
        };
    };

    port@2 {
        reg = <2>;
        mode_mux_out_dp: endpoint {
            remote-endpoint = <&dp0_out_ep>
        };
    };
};

and we add the definition for the mux-controller, where the mux-gpio
control actually resides:

mode_mux_controller: mux-controller {
    compatible = "vendor,typec-mode-mux";
    mux-gpios = <&gpio_controller 23 GPIO_ACTIVE_HIGH>;
};

> 
> >     };
> > };
> >
> > // Type C PHY Controller.
> > tcphy0: phy@ff7c0000 {
> >     compatible = "rockchip,rk3399-typec-phy";
> >     reg = <0x0 0xff7c0000 0x0 0x40000>;
> >     ...
> >     tcphy0_dp: phy@dc00000 {
> >         compatible = "soc,dp-phy";
> >         reg = <0xdc00000 0x1000>;
> >         ports {
> >             port@0 {
> >                 reg = <0>;
> >                 dp_phy_out: endpoint {
> >                     remote-endpoint = <&mux_dp_in>;
> >                 };
> >             };
> 
> This is wrong in that 'phys' are not part of the graph. It's the DP
> and USB controllers that should be part of the graph. Any phys are
> referenced with the phys binding and are not part of the graph.

Got it. So the new PHY definition would be just the same, but no
"ports".

> 
> >         };
> >     };
> >
> >     tcphy0_usb3: phy@db00000 {
> >         compatible = "soc,usb3-phy";
> >         reg = <0xdb00000 0x1000>;
> >         ports {
> >             port@0 {
> >                 reg = <0>;
> >                 usb3_phy_out: endpoint {
> >                     remote-endpoint = <&mux_usb3_in>;
> >                 };
> >             };
> >         };
> >     };
> > };
> >
> >
> > // USB3 Host controller
> > usbdrd3_0: usb@fe800000 {
> >     compatible = "rockchip,rk3399-dwc3";
> >     #address-cells = <2>;
> >     #size-cells = <2>;
> >     clocks = ...;
> >     clock-names = ...;
> >     status = "disabled";
> >
> >     usbdrd_dwc3_0: usb@fe800000 {
> >         compatible = "snps,dwc3";
> >         reg = <0x0 0xfe800000 0x0 0x100000>;
> >         interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH 0>;
> >         clocks = ...;
> >         clock-names = ...;
> >         dr_mode = "otg";
> >         phys = <&tcphy0_usb3>;
> >         phy-names = "usb3-phy";
> >         phy_type = "utmi_wide";
> >         power-domains = <&power RK3399_PD_USB3>;
> >         status = "disabled";

This means a port definition here:
            ports {
                #address-cells = <1>;
                #size-cells = <0>;

                port@0 {
                    reg = <0>;
                    usb3_0_ep: endpoint {
                        remote-endpoint = <&mode_mux_out_usb3>;
                    };
                };
            };

> >     };
> > };
> >
> > // DP controller
> > cdn_dp: dp@fec00000 {
> >     compatible = "rockchip,rk3399-cdn-dp";
> >     reg = <0x0 0xfec00000 0x0 0x100000>;
> >     interrupts = ...;
> >     clocks = ...;
> >     clock-names = ...;
> >     phys = <&tcphy0_dp>;
> >     ...
> >     ports {
> >         dp_in: port {
> >             #address-cells = <1>;
> >             #size-cells = <0>;
> >
> >             dp_in_vopb: endpoint@0 {
> >                 reg = <0>;
> >                 remote-endpoint = <&vopb_out_dp>;
> >             };
> >
> >             dp_in_vopl: endpoint@1 {
> >                 reg = <1>;
> >                 remote-endpoint = <&vopl_out_dp>;
> >             };
> >         };
> 
> This should have an output port and then that is connected to the USB
> connector. Given that DP is muxed with the USB SS signals, port@1
> (defined as USB SS) should then have 2 endpoints.

So, completing the example, another port here:

            dp_out: port@1 {
                reg = <1>;
                dp0_out_ep: endpoint {
                    remote-endpoint = <&mode_mux_out_dp>;
                };
            };

> 
> Then the only new thing here is how to represent the GPIO line
> controlling the mux. Normally, I'd expect this to be in the parent of
> the connector (the type-C controller), but since you have multiple
> connectors, that doesn't work so well. So you can put 'mux-gpios' in
> the port@1 node. (Or maybe it should be 'mux-controls' with a gpio mux
> defined elsewhere).

I think the updated example better helps delineate what we're trying to
achieve. Per Heikki's earlier explanation (re-quoting here) :

"
> > > > The mode-switch here would actually represent the "consumer" part in
> > > > the mux-control bindings. So the mux-controls would describe the
> > > > relationship between the "mode-switch" and the mux controller(s),
> > > > while the mode-switch property describes the relationship between
> > > > something like USB Type-C Port Manager (or this cros_ec function) and
> > > > the "mux consumer".
"

So the device that "mode-switch" points to (in this case, foo_mux) is the "consumer" of
the mux-control direct consumer). Heikki, does this example better
demonstrate what the Type C connector class expects?

Also, I think it might be better to move these *-switch properties into the
usb-connector.yaml binding in the event that we decide to take this route,
since the "Chrome EC Type C port driver" isn't expected to use them directly.
Rob, kindly LMK if you'd prefer that, and I can make the change in the
next patch version.

I can re-write this example in a non-inline form if that would be easier
to interpret. Kindly LMK, and also LMK if this graph connection scheme
example is acceptable.

Thanks again, and best regards,

-Prashant

> 
> Rob

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Heikki Krogerus]

On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> Hi Rob,
> 
> Thanks again for the comments and feedback. Kindly see responses inline:
> 
> (Trimming unrelated text from thread):
> 
> On Tue, Jun 09, 2020 at 02:30:11PM -0600, Rob Herring wrote:
> > On Fri, May 29, 2020 at 5:30 PM Prashant Malani <pmalani@chromium.org> wrote:
> > >
> > > Nodes truncated and unrelated fields omitted in the interest of brevity:
> > >
> > > // Chrome OS EC Type C Port Manager.
> > > typec {
> > >     compatible = "google,cros-ec-typec";
> > >     #address-cells = <1>;
> > >     #size-cells = <0>;
> > >
> > >     connector@0 {
> > >         compatible = "usb-c-connector";
> > >         reg = <0>;
> > >         power-role = "dual";
> > >         data-role = "dual";
> > >         try-power-role = "source";
> > >         mode-switch = <&foo_mux>;
> > >         // Other switches can point to the same mux.
> > >         ....
> > 
> > The connector is supposed to have 'ports' for USB2, USB3, and Aux
> > unless the parent is the USB controller.
> Understood; so, coupled with Heikki's explanation (see below for where
> I've pasted it), would it be something like so? (adding inline to the connector
> node definition):
> 
>             ports {
>                 #address-cells = <1>;
>                 #size-cells = <0>;
> 
>                 port@0 {
>                     reg = <0>;
>                     usb_con_hs: endpoint {
>                         remote-endpoint = <&foo_usb_hs_controller>;
>                     };
>                 };
> 
>                 port@1 {
>                     reg = <1>;
>                     usb_con0_ss: endpoint@0 {
>                         remote-endpoint = <&mode_mux_in>;
>                     };
>                 };
> 
>                 port@2 {
>                     reg = <2>;
>                     usb_con_sbu: endpoint {
>                         remote-endpoint = <&foo_dp_aux>;
>                     };
>                 };
>             };

The pins that can be reassigned can in practice go anywhere. We can't
group them in any way. What do we do for example when the two sideband
pins go to different locations?

It looks like the OF graph for the USB Type-C connectors expects the
pins to be grouped like that, which is too bad, because unfortunately
it will not work. It would require that we have a separate port for
every pin that can be reassigned on the connector, and let's not even
consider that.

We need higher level description of the connections for the USB Type-C
connectors. For example, a connector can be connected to this (or
these) DisplayPort(s), this USB 2.0 port, this USB 3.x port, this USB4
port, etc. And this is the mux that handles the pins on this
connector, and these are the retimers, etc. etc.

We also need a way to identify those connections, and relying on
something like fixed port node addresses, so indexes in practice,
feels really risky to me. A conflict may seem unlikely now, but we
seen those so many times in the past other things like GPIOs, IRQs,
etc. We really need to define string identifiers for these
connections.


thanks,

-- 
heikki

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Rob Herring]

On Wed, Jun 10, 2020 at 9:34 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> > Hi Rob,
> >
> > Thanks again for the comments and feedback. Kindly see responses inline:
> >
> > (Trimming unrelated text from thread):
> >
> > On Tue, Jun 09, 2020 at 02:30:11PM -0600, Rob Herring wrote:
> > > On Fri, May 29, 2020 at 5:30 PM Prashant Malani <pmalani@chromium.org> wrote:
> > > >
> > > > Nodes truncated and unrelated fields omitted in the interest of brevity:
> > > >
> > > > // Chrome OS EC Type C Port Manager.
> > > > typec {
> > > >     compatible = "google,cros-ec-typec";
> > > >     #address-cells = <1>;
> > > >     #size-cells = <0>;
> > > >
> > > >     connector@0 {
> > > >         compatible = "usb-c-connector";
> > > >         reg = <0>;
> > > >         power-role = "dual";
> > > >         data-role = "dual";
> > > >         try-power-role = "source";
> > > >         mode-switch = <&foo_mux>;
> > > >         // Other switches can point to the same mux.
> > > >         ....
> > >
> > > The connector is supposed to have 'ports' for USB2, USB3, and Aux
> > > unless the parent is the USB controller.
> > Understood; so, coupled with Heikki's explanation (see below for where
> > I've pasted it), would it be something like so? (adding inline to the connector
> > node definition):
> >
> >             ports {
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >
> >                 port@0 {
> >                     reg = <0>;
> >                     usb_con_hs: endpoint {
> >                         remote-endpoint = <&foo_usb_hs_controller>;
> >                     };
> >                 };
> >
> >                 port@1 {
> >                     reg = <1>;
> >                     usb_con0_ss: endpoint@0 {
> >                         remote-endpoint = <&mode_mux_in>;
> >                     };
> >                 };
> >
> >                 port@2 {
> >                     reg = <2>;
> >                     usb_con_sbu: endpoint {
> >                         remote-endpoint = <&foo_dp_aux>;
> >                     };
> >                 };
> >             };
>
> The pins that can be reassigned can in practice go anywhere. We can't
> group them in any way. What do we do for example when the two sideband
> pins go to different locations?

The sideband pins from the connector go to multiple places or the
sideband signal from a controller go to multiple connectors? Either
way, that's solved with multiple endpoints. In the former case, port@2
would have multiple endpoints with all the possible connections. The
general model of the graph is each port is a separate data channel and
multiple endpoints are either a mux or fanout depending on the data
direction.

> It looks like the OF graph for the USB Type-C connectors expects the
> pins to be grouped like that, which is too bad, because unfortunately
> it will not work. It would require that we have a separate port for
> every pin that can be reassigned on the connector, and let's not even
> consider that.

I guess you are referring to the 4 SS signal pairs and that they could
be 2 USB links, 1 USB link and 1-2 Alt mode links, or 4 Alt mode
links. I think the grouping of SS signals is fine as I'd expect
there's a single entity deciding the routing. That would be 'mux' node
I think, but 'mux' is the wrong abstraction here. I guess we could
have 4 muxes (1 for each signal), but I'd hope we don't need that
level of detail in DT. I think we're in agreement on that.

> We need higher level description of the connections for the USB Type-C
> connectors. For example, a connector can be connected to this (or
> these) DisplayPort(s), this USB 2.0 port, this USB 3.x port, this USB4
> port, etc. And this is the mux that handles the pins on this
> connector, and these are the retimers, etc. etc.
>
> We also need a way to identify those connections, and relying on
> something like fixed port node addresses, so indexes in practice,
> feels really risky to me. A conflict may seem unlikely now, but we
> seen those so many times in the past other things like GPIOs, IRQs,
> etc. We really need to define string identifiers for these
> connections.

I assume for IRQs you are referring to cases where we have a variable
number such as 'interrupts = <1 2 3>;' where 'interrupts = <1 3>'
doesn't work because we can't describe interrupt 2 is not present? The
graph binding doesn't suffer from that issue since we can easily omit
port or endpoint nodes.

Also, the numbering is specific to a compatible string. If we need
different numbering, then we can do a new compatible.

Rob

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Hi Rob,

On Wed, Jun 10, 2020 at 9:53 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jun 10, 2020 at 9:34 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> > > Hi Rob,
> > >
> > > Thanks again for the comments and feedback. Kindly see responses inline:
> > >
> > > (Trimming unrelated text from thread):
> > >
> > >
> > >             ports {
> > >                 #address-cells = <1>;
> > >                 #size-cells = <0>;
> > >
> > >                 port@0 {
> > >                     reg = <0>;
> > >                     usb_con_hs: endpoint {
> > >                         remote-endpoint = <&foo_usb_hs_controller>;
> > >                     };
> > >                 };
> > >
> > >                 port@1 {
> > >                     reg = <1>;
> > >                     usb_con0_ss: endpoint@0 {
> > >                         remote-endpoint = <&mode_mux_in>;
> > >                     };
> > >                 };
> > >
> > >                 port@2 {
> > >                     reg = <2>;
> > >                     usb_con_sbu: endpoint {
> > >                         remote-endpoint = <&foo_dp_aux>;
> > >                     };
> > >                 };
> > >             };
> >
> > The pins that can be reassigned can in practice go anywhere. We can't
> > group them in any way. What do we do for example when the two sideband
> > pins go to different locations?
>
> The sideband pins from the connector go to multiple places or the
> sideband signal from a controller go to multiple connectors? Either
> way, that's solved with multiple endpoints. In the former case, port@2
> would have multiple endpoints with all the possible connections. The
> general model of the graph is each port is a separate data channel and
> multiple endpoints are either a mux or fanout depending on the data
> direction.
>
> > It looks like the OF graph for the USB Type-C connectors expects the
> > pins to be grouped like that, which is too bad, because unfortunately
> > it will not work. It would require that we have a separate port for
> > every pin that can be reassigned on the connector, and let's not even
> > consider that.
>
> I guess you are referring to the 4 SS signal pairs and that they could
> be 2 USB links, 1 USB link and 1-2 Alt mode links, or 4 Alt mode
> links. I think the grouping of SS signals is fine as I'd expect
> there's a single entity deciding the routing. That would be 'mux' node
> I think, but 'mux' is the wrong abstraction here. I guess we could
> have 4 muxes (1 for each signal), but I'd hope we don't need that
> level of detail in DT. I think we're in agreement on that.

I think the updated example handles this grouping (port@1 going to a
"SS mux") although as you said it should probably be a group of muxes,
but I think the example illustrates the point. Is that assessment
correct?

>
> > We need higher level description of the connections for the USB Type-C
> > connectors. For example, a connector can be connected to this (or
> > these) DisplayPort(s), this USB 2.0 port, this USB 3.x port, this USB4
> > port, etc. And this is the mux that handles the pins on this
> > connector, and these are the retimers, etc. etc.
> >
> > We also need a way to identify those connections, and relying on
> > something like fixed port node addresses, so indexes in practice,
> > feels really risky to me. A conflict may seem unlikely now, but we
> > seen those so many times in the past other things like GPIOs, IRQs,
> > etc. We really need to define string identifiers for these
> > connections.
>
> I assume for IRQs you are referring to cases where we have a variable
> number such as 'interrupts = <1 2 3>;' where 'interrupts = <1 3>'
> doesn't work because we can't describe interrupt 2 is not present? The
> graph binding doesn't suffer from that issue since we can easily omit
> port or endpoint nodes.
>
> Also, the numbering is specific to a compatible string. If we need
> different numbering, then we can do a new compatible.
>
> Rob

Would this block the addition of the "*-switch" properties? IIUC the
two are related but not dependent on each other.

The *-switch properties are phandles which the Type C connector class
framework expects (and uses to get handles to those switches).
These would point to the "mux" or "group of mux" abstractions as noted earlier.

I'd suggest we work on updating the Type C connector class to a model
that can describe connections for both DT (using OF graph) and ACPI,
if something
like that exists, but let it not block the addition of these switches
to usb-connector.yaml; they will be needed by the Type C connector
class framework
regardless of the form the connection description takes.

Best regards,

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Rob Herring]

On Wed, Jun 10, 2020 at 11:49 AM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Rob,
>
> On Wed, Jun 10, 2020 at 9:53 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Jun 10, 2020 at 9:34 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> > > > Hi Rob,
> > > >
> > > > Thanks again for the comments and feedback. Kindly see responses inline:
> > > >
> > > > (Trimming unrelated text from thread):
> > > >
> > > >
> > > >             ports {
> > > >                 #address-cells = <1>;
> > > >                 #size-cells = <0>;
> > > >
> > > >                 port@0 {
> > > >                     reg = <0>;
> > > >                     usb_con_hs: endpoint {
> > > >                         remote-endpoint = <&foo_usb_hs_controller>;
> > > >                     };
> > > >                 };
> > > >
> > > >                 port@1 {
> > > >                     reg = <1>;
> > > >                     usb_con0_ss: endpoint@0 {
> > > >                         remote-endpoint = <&mode_mux_in>;
> > > >                     };
> > > >                 };
> > > >
> > > >                 port@2 {
> > > >                     reg = <2>;
> > > >                     usb_con_sbu: endpoint {
> > > >                         remote-endpoint = <&foo_dp_aux>;
> > > >                     };
> > > >                 };
> > > >             };
> > >
> > > The pins that can be reassigned can in practice go anywhere. We can't
> > > group them in any way. What do we do for example when the two sideband
> > > pins go to different locations?
> >
> > The sideband pins from the connector go to multiple places or the
> > sideband signal from a controller go to multiple connectors? Either
> > way, that's solved with multiple endpoints. In the former case, port@2
> > would have multiple endpoints with all the possible connections. The
> > general model of the graph is each port is a separate data channel and
> > multiple endpoints are either a mux or fanout depending on the data
> > direction.
> >
> > > It looks like the OF graph for the USB Type-C connectors expects the
> > > pins to be grouped like that, which is too bad, because unfortunately
> > > it will not work. It would require that we have a separate port for
> > > every pin that can be reassigned on the connector, and let's not even
> > > consider that.
> >
> > I guess you are referring to the 4 SS signal pairs and that they could
> > be 2 USB links, 1 USB link and 1-2 Alt mode links, or 4 Alt mode
> > links. I think the grouping of SS signals is fine as I'd expect
> > there's a single entity deciding the routing. That would be 'mux' node
> > I think, but 'mux' is the wrong abstraction here. I guess we could
> > have 4 muxes (1 for each signal), but I'd hope we don't need that
> > level of detail in DT. I think we're in agreement on that.
>
> I think the updated example handles this grouping (port@1 going to a
> "SS mux") although as you said it should probably be a group of muxes,
> but I think the example illustrates the point. Is that assessment
> correct?

Yes, but let's stop calling it a mux. It's a "USB Type C signal routing blob".

> > > We need higher level description of the connections for the USB Type-C
> > > connectors. For example, a connector can be connected to this (or
> > > these) DisplayPort(s), this USB 2.0 port, this USB 3.x port, this USB4
> > > port, etc. And this is the mux that handles the pins on this
> > > connector, and these are the retimers, etc. etc.
> > >
> > > We also need a way to identify those connections, and relying on
> > > something like fixed port node addresses, so indexes in practice,
> > > feels really risky to me. A conflict may seem unlikely now, but we
> > > seen those so many times in the past other things like GPIOs, IRQs,
> > > etc. We really need to define string identifiers for these
> > > connections.
> >
> > I assume for IRQs you are referring to cases where we have a variable
> > number such as 'interrupts = <1 2 3>;' where 'interrupts = <1 3>'
> > doesn't work because we can't describe interrupt 2 is not present? The
> > graph binding doesn't suffer from that issue since we can easily omit
> > port or endpoint nodes.
> >
> > Also, the numbering is specific to a compatible string. If we need
> > different numbering, then we can do a new compatible.
> >
> > Rob
>
> Would this block the addition of the "*-switch" properties? IIUC the
> two are related but not dependent on each other.
>
> The *-switch properties are phandles which the Type C connector class
> framework expects (and uses to get handles to those switches).
> These would point to the "mux" or "group of mux" abstractions as noted earlier.

You don't need them though. Walk the graph. You get the connector
port@1 remote endpoint and then get its parent.

> I'd suggest we work on updating the Type C connector class to a model
> that can describe connections for both DT (using OF graph) and ACPI,
> if something
> like that exists, but let it not block the addition of these switches
> to usb-connector.yaml; they will be needed by the Type C connector
> class framework
> regardless of the form the connection description takes.

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Hi Rob,

Thanks as always for your help in reviewing this proposal!

Kindly see inline

(Trimming text);
On Thu, Jun 11, 2020 at 02:00:47PM -0600, Rob Herring wrote:
> On Wed, Jun 10, 2020 at 11:49 AM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > Hi Rob,
> >
> > On Wed, Jun 10, 2020 at 9:53 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > > On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> >
> > I think the updated example handles this grouping (port@1 going to a
> > "SS mux") although as you said it should probably be a group of muxes,
> > but I think the example illustrates the point. Is that assessment
> > correct?
>
> Yes, but let's stop calling it a mux. It's a "USB Type C signal routing blob".

Ack.

Let's go with "-switch" ? That's what the connector class uses and it
conveys the meaning (unless that is a reserved keyword in DT).

>
> > Would this block the addition of the "*-switch" properties? IIUC the
> > two are related but not dependent on each other.
> >
> > The *-switch properties are phandles which the Type C connector class
> > framework expects (and uses to get handles to those switches).
> > These would point to the "mux" or "group of mux" abstractions as noted earlier.
>
> You don't need them though. Walk the graph. You get the connector
> port@1 remote endpoint and then get its parent.
>

I see; would it be something along the lines of this? (DT example
follows; search for "example_end" to jump to bottom):

<example_start>

connector@0 {
    compatible = "usb-c-connector";
    reg = <0>;
    power-role = "dual";
    data-role = "dual";
    try-power-role = "source";
    ....
    ports {
        #address-cells = <1>;
        #size-cells = <0>;

        port@0 {
            reg = <0>;
            usb_con_hs: endpoint {
                remote-endpoint = <&foo_usb_hs_controller>;
            };
        };

        port@1 {
            reg = <1>;
            #address-cells = <1>;
            #size-cells = <0>;

            usb_con0_ss_mode: endpoint@0 {
                reg = <0>
                remote-endpoint = <&mode_switch_ss_in>;
            };

            usb_con0_ss_orientation: endpoint@1 {
                        reg = <1>
                        remote-endpoint = <&orientation_switch_ss_in>;
            };

            usb_con0_ss_data_role: endpoint@2 {
                        reg = <2>
                        remote-endpoint = <&data_role_switch_in>;
            };
        };

        port@2 {
            reg = <2>;
            #address-cells = <1>;
            #size-cells = <0>;
            usb_con0_sbu_mode: endpoint@0 {
                        reg = <0>
                        remote-endpoint = <&mode_switch_sbu_in>;
            };
            usb_con0_sbu_orientation: endpoint@1 {
                        reg = <1>
                        remote-endpoint = <&orientation_switch_sbu_in>;
            };
        };
    };
};

mode_switch {
    compatible = "typec-mode-switch";
    mux-controls = <&mode_mux_controller>;
    mux-control-names = "mode";
    #address-cells = <1>;
    #size-cells = <0>;

    port@0 {
        reg = <0>;
        mode_switch_ss_in: endpoint {
            remote-endpoint = <&usb_con0_ss_mode>
        };
    };

    port@1 {
        reg = <1>;
        mode_switch_out_usb3: endpoint {
            remote-endpoint = <&usb3_0_ep>
        };
    };

    port@2 {
        reg = <2>;
        mode_switch_out_dp: endpoint {
            remote-endpoint = <&dp0_out_ep>
        };
    };

    port@3 {
        reg = <3>;
        mode_switch_sbu_in: endpoint {
            remote-endpoint = <&usb_con0_sbu_mode>
        };
    };
    // ... other ports similarly defined.
};

orientation_switch {
    compatible = "typec-orientation-switch";
    mux-controls = <&orientation_mux_controller>;
    mux-control-names = "orientation";
    #address-cells = <1>;
    #size-cells = <0>;

    port@0 {
        reg = <0>;
        orientation_switch_ss_in: endpoint {
            remote-endpoint = <&usb_con0_ss_orientation>
        };
    };

    port@1
        reg = <1>;
        orientation_switch_sbu_in: endpoint {
            remote-endpoint = <&usb_con0_sbu_orientation>
        };
    };
    // ... other ports similarly defined.
};

data_role_switch {
    compatible = "typec-data-role-switch";
    mux-controls = <&data_role_switch_controller>;
    mux-control-names = "data_role";

    port {
        data_role_switch_in: endpoint {
            remote-endpoint = <&usb_con0_ss_data_role>
        };
    };
};

<example_end>

Would this be conformant to OF graph and usb-connector bindings
requirements? We'll certainly send out a format PATCH/RFC series for
this, but I was hoping to gauge whether we're thinking along the right lines.

So, in effect this would mean:
- New bindings(and compatible strings) to be added for:
  typec-{orientation,data-role,mode}-switch.
- Handling in Type C connector class to parse switches from OF graph.
- Handling in Type C connector class for distinct switches for port@1
  (SS lines) and port@2 (SBU lines).

The only thing I'm confused about is how we can define these switch
remote-endpoint bindings in usb-connector.yaml; the port can have an
remote-endpoint, but can we specify what the parent of the remote-endpoint
should have as a compatible string? Or do we not need to?

Best regards,

-Prashant

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Heikki Krogerus]

On Fri, Jun 12, 2020 at 10:34:06AM -0700, Prashant Malani wrote:
> Hi Rob,
> 
> Thanks as always for your help in reviewing this proposal!
> 
> Kindly see inline
> 
> (Trimming text);
> On Thu, Jun 11, 2020 at 02:00:47PM -0600, Rob Herring wrote:
> > On Wed, Jun 10, 2020 at 11:49 AM Prashant Malani <pmalani@chromium.org> wrote:
> > >
> > > Hi Rob,
> > >
> > > On Wed, Jun 10, 2020 at 9:53 AM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > > On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> > >
> > > I think the updated example handles this grouping (port@1 going to a
> > > "SS mux") although as you said it should probably be a group of muxes,
> > > but I think the example illustrates the point. Is that assessment
> > > correct?
> >
> > Yes, but let's stop calling it a mux. It's a "USB Type C signal routing blob".
> 
> Ack.
> 
> Let's go with "-switch" ? That's what the connector class uses and it
> conveys the meaning (unless that is a reserved keyword in DT).

Just as a clarification here, we should not be even talking about
signal routing here. We are talking about functions that an external
components perform from the connector's perspective. It depends on the
platform does that function require changing the routing of the lines
from the connector. For example, data role swapping does not require
muxing on platforms that have single dual-role USB controller, but
platforms that have separate IPs for the USB host and USB device
controllers will need a mux.

Note, that it is even possible that switching from USB to DisplayPort
mode does not require any pin reconfiguration from the mux, even if
the platform has one, because the PHY can be shared between USB3 and
DP. Then the PHY just needs to be told that it is now in DP mode when
DP alt mode is entered instead of the mux.

> > > Would this block the addition of the "*-switch" properties? IIUC the
> > > two are related but not dependent on each other.
> > >
> > > The *-switch properties are phandles which the Type C connector class
> > > framework expects (and uses to get handles to those switches).
> > > These would point to the "mux" or "group of mux" abstractions as noted earlier.
> >
> > You don't need them though. Walk the graph. You get the connector
> > port@1 remote endpoint and then get its parent.
> >
> 
> I see; would it be something along the lines of this? (DT example
> follows; search for "example_end" to jump to bottom):

I just realized that you have in practice placed the mux-agent into
the graph below, right? That we can not do, because it is not
physically connected to the connector.

> <example_start>
> 
> connector@0 {
>     compatible = "usb-c-connector";
>     reg = <0>;
>     power-role = "dual";
>     data-role = "dual";
>     try-power-role = "source";
>     ....
>     ports {
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         port@0 {
>             reg = <0>;
>             usb_con_hs: endpoint {
>                 remote-endpoint = <&foo_usb_hs_controller>;
>             };
>         };
> 
>         port@1 {
>             reg = <1>;
>             #address-cells = <1>;
>             #size-cells = <0>;
> 
>             usb_con0_ss_mode: endpoint@0 {
>                 reg = <0>
>                 remote-endpoint = <&mode_switch_ss_in>;
>             };
> 
>             usb_con0_ss_orientation: endpoint@1 {
>                         reg = <1>
>                         remote-endpoint = <&orientation_switch_ss_in>;
>             };
> 
>             usb_con0_ss_data_role: endpoint@2 {
>                         reg = <2>
>                         remote-endpoint = <&data_role_switch_in>;
>             };
>         };
> 
>         port@2 {
>             reg = <2>;
>             #address-cells = <1>;
>             #size-cells = <0>;
>             usb_con0_sbu_mode: endpoint@0 {
>                         reg = <0>
>                         remote-endpoint = <&mode_switch_sbu_in>;
>             };
>             usb_con0_sbu_orientation: endpoint@1 {
>                         reg = <1>
>                         remote-endpoint = <&orientation_switch_sbu_in>;
>             };
>         };
>     };
> };
> 
> mode_switch {
>     compatible = "typec-mode-switch";
>     mux-controls = <&mode_mux_controller>;
>     mux-control-names = "mode";
>     #address-cells = <1>;
>     #size-cells = <0>;
> 
>     port@0 {
>         reg = <0>;
>         mode_switch_ss_in: endpoint {
>             remote-endpoint = <&usb_con0_ss_mode>
>         };
>     };
> 
>     port@1 {
>         reg = <1>;
>         mode_switch_out_usb3: endpoint {
>             remote-endpoint = <&usb3_0_ep>
>         };
>     };
> 
>     port@2 {
>         reg = <2>;
>         mode_switch_out_dp: endpoint {
>             remote-endpoint = <&dp0_out_ep>
>         };
>     };
> 
>     port@3 {
>         reg = <3>;
>         mode_switch_sbu_in: endpoint {
>             remote-endpoint = <&usb_con0_sbu_mode>
>         };
>     };
>     // ... other ports similarly defined.
> };
> 
> orientation_switch {
>     compatible = "typec-orientation-switch";
>     mux-controls = <&orientation_mux_controller>;
>     mux-control-names = "orientation";
>     #address-cells = <1>;
>     #size-cells = <0>;
> 
>     port@0 {
>         reg = <0>;
>         orientation_switch_ss_in: endpoint {
>             remote-endpoint = <&usb_con0_ss_orientation>
>         };
>     };
> 
>     port@1
>         reg = <1>;
>         orientation_switch_sbu_in: endpoint {
>             remote-endpoint = <&usb_con0_sbu_orientation>
>         };
>     };
>     // ... other ports similarly defined.
> };
> 
> data_role_switch {
>     compatible = "typec-data-role-switch";
>     mux-controls = <&data_role_switch_controller>;
>     mux-control-names = "data_role";
> 
>     port {
>         data_role_switch_in: endpoint {
>             remote-endpoint = <&usb_con0_ss_data_role>
>         };
>     };
> };
> 
> <example_end>
> 
> Would this be conformant to OF graph and usb-connector bindings
> requirements? We'll certainly send out a format PATCH/RFC series for
> this, but I was hoping to gauge whether we're thinking along the right lines.
> 
> So, in effect this would mean:
> - New bindings(and compatible strings) to be added for:
>   typec-{orientation,data-role,mode}-switch.
> - Handling in Type C connector class to parse switches from OF graph.
> - Handling in Type C connector class for distinct switches for port@1
>   (SS lines) and port@2 (SBU lines).
> 
> The only thing I'm confused about is how we can define these switch
> remote-endpoint bindings in usb-connector.yaml; the port can have an
> remote-endpoint, but can we specify what the parent of the remote-endpoint
> should have as a compatible string? Or do we not need to?

thanks,

-- 
heikki

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Hi Heikki and Rob,

(trimming text):
On Mon, Jun 15, 2020 at 04:22:07PM +0300, Heikki Krogerus wrote:
> On Fri, Jun 12, 2020 at 10:34:06AM -0700, Prashant Malani wrote:
> > Hi Rob,
> > > Yes, but let's stop calling it a mux. It's a "USB Type C signal routing blob".
> > 
> > Ack.
> > 
> > Let's go with "-switch" ? That's what the connector class uses and it
> > conveys the meaning (unless that is a reserved keyword in DT).
> 
> Just as a clarification here, we should not be even talking about
> signal routing here. We are talking about functions that an external
> components perform from the connector's perspective. It depends on the
> platform does that function require changing the routing of the lines
> from the connector. For example, data role swapping does not require
> muxing on platforms that have single dual-role USB controller, but
> platforms that have separate IPs for the USB host and USB device
> controllers will need a mux.
> 
> Note, that it is even possible that switching from USB to DisplayPort
> mode does not require any pin reconfiguration from the mux, even if
> the platform has one, because the PHY can be shared between USB3 and
> DP. Then the PHY just needs to be told that it is now in DP mode when
> DP alt mode is entered instead of the mux.
> 
> > > > Would this block the addition of the "*-switch" properties? IIUC the
> > > > two are related but not dependent on each other.
> > > >
> > > > The *-switch properties are phandles which the Type C connector class
> > > > framework expects (and uses to get handles to those switches).
> > > > These would point to the "mux" or "group of mux" abstractions as noted earlier.
> > >
> > > You don't need them though. Walk the graph. You get the connector
> > > port@1 remote endpoint and then get its parent.
> > >
> > 
> > I see; would it be something along the lines of this? (DT example
> > follows; search for "example_end" to jump to bottom):
> 
> I just realized that you have in practice placed the mux-agent into
> the graph below, right? That we can not do, because it is not
> physically connected to the connector.

Is this a requirement? I read through the graph.txt file [1] and I couldn't
find anything stating that a physical connection between two devices was
required (but I may be misinterpreting that document)

[1]:
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/graph.txt

> 
> > <example_start>
> > 
> > <example_end>
> > 
> > Would this be conformant to OF graph and usb-connector bindings
> > requirements? We'll certainly send out a format PATCH/RFC series for
> > this, but I was hoping to gauge whether we're thinking along the right lines.
> > 
> > So, in effect this would mean:
> > - New bindings(and compatible strings) to be added for:
> >   typec-{orientation,data-role,mode}-switch.
> > - Handling in Type C connector class to parse switches from OF graph.
> > - Handling in Type C connector class for distinct switches for port@1
> >   (SS lines) and port@2 (SBU lines).
> > 
> > The only thing I'm confused about is how we can define these switch
> > remote-endpoint bindings in usb-connector.yaml; the port can have an
> > remote-endpoint, but can we specify what the parent of the remote-endpoint
> > should have as a compatible string? Or do we not need to?
> 
> thanks,
> 
> -- 
> heikki

Best regards,

-Prashant

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Hi Rob,

Just following up on this. Would the below example align better with
OF graph requirements?

Example begins at <example_start>, but in summary:
- port@1 (Superspeed) of usb-c-connector will have 3 endpoints (0 =
goes to mode switch, 1 = goes to orientation switch, 2 = goes to data
role switch)
- port@2 (SBU) of usb-c-connector will have 2 endpoints (0 = goes to
mode switch, 1 = goes to orientation switch)
-These end points can go through arbitrarily long paths (including
retimers) as long as they end up at the following devices:
    a. device with compatible string "typec-mode-switch" for endpoint 0.
    b. device with compatible string "typec-orientation-switch" for endpoint 1.
    c. device with compatible string "typec-data-role-switch" for endpoint 2.
- Connector class framework will perform the traversal from
usb-c-connector port endpoints to the "*-switch" devices.

Best regards,

On Fri, Jun 12, 2020 at 10:34 AM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Rob,
>
> Thanks as always for your help in reviewing this proposal!
>
> Kindly see inline
>
> (Trimming text);
> On Thu, Jun 11, 2020 at 02:00:47PM -0600, Rob Herring wrote:
> > On Wed, Jun 10, 2020 at 11:49 AM Prashant Malani <pmalani@chromium.org> wrote:
> > >
> > > Hi Rob,
> > >
> > > On Wed, Jun 10, 2020 at 9:53 AM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > > On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> > >
> > > I think the updated example handles this grouping (port@1 going to a
> > > "SS mux") although as you said it should probably be a group of muxes,
> > > but I think the example illustrates the point. Is that assessment
> > > correct?
> >
> > Yes, but let's stop calling it a mux. It's a "USB Type C signal routing blob".
>
> Ack.
>
> Let's go with "-switch" ? That's what the connector class uses and it
> conveys the meaning (unless that is a reserved keyword in DT).
>
> >
> > > Would this block the addition of the "*-switch" properties? IIUC the
> > > two are related but not dependent on each other.
> > >
> > > The *-switch properties are phandles which the Type C connector class
> > > framework expects (and uses to get handles to those switches).
> > > These would point to the "mux" or "group of mux" abstractions as noted earlier.
> >
> > You don't need them though. Walk the graph. You get the connector
> > port@1 remote endpoint and then get its parent.
> >
>
> I see; would it be something along the lines of this? (DT example
> follows; search for "example_end" to jump to bottom):
>
> <example_start>
>
> connector@0 {
>     compatible = "usb-c-connector";
>     reg = <0>;
>     power-role = "dual";
>     data-role = "dual";
>     try-power-role = "source";
>     ....
>     ports {
>         #address-cells = <1>;
>         #size-cells = <0>;
>
>         port@0 {
>             reg = <0>;
>             usb_con_hs: endpoint {
>                 remote-endpoint = <&foo_usb_hs_controller>;
>             };
>         };
>
>         port@1 {
>             reg = <1>;
>             #address-cells = <1>;
>             #size-cells = <0>;
>
>             usb_con0_ss_mode: endpoint@0 {
>                 reg = <0>
>                 remote-endpoint = <&mode_switch_ss_in>;
>             };
>
>             usb_con0_ss_orientation: endpoint@1 {
>                         reg = <1>
>                         remote-endpoint = <&orientation_switch_ss_in>;
>             };
>
>             usb_con0_ss_data_role: endpoint@2 {
>                         reg = <2>
>                         remote-endpoint = <&data_role_switch_in>;
>             };
>         };
>
>         port@2 {
>             reg = <2>;
>             #address-cells = <1>;
>             #size-cells = <0>;
>             usb_con0_sbu_mode: endpoint@0 {
>                         reg = <0>
>                         remote-endpoint = <&mode_switch_sbu_in>;
>             };
>             usb_con0_sbu_orientation: endpoint@1 {
>                         reg = <1>
>                         remote-endpoint = <&orientation_switch_sbu_in>;
>             };
>         };
>     };
> };
>
> mode_switch {
>     compatible = "typec-mode-switch";
>     mux-controls = <&mode_mux_controller>;
>     mux-control-names = "mode";
>     #address-cells = <1>;
>     #size-cells = <0>;
>
>     port@0 {
>         reg = <0>;
>         mode_switch_ss_in: endpoint {
>             remote-endpoint = <&usb_con0_ss_mode>
>         };
>     };
>
>     port@1 {
>         reg = <1>;
>         mode_switch_out_usb3: endpoint {
>             remote-endpoint = <&usb3_0_ep>
>         };
>     };
>
>     port@2 {
>         reg = <2>;
>         mode_switch_out_dp: endpoint {
>             remote-endpoint = <&dp0_out_ep>
>         };
>     };
>
>     port@3 {
>         reg = <3>;
>         mode_switch_sbu_in: endpoint {
>             remote-endpoint = <&usb_con0_sbu_mode>
>         };
>     };
>     // ... other ports similarly defined.
> };
>
> orientation_switch {
>     compatible = "typec-orientation-switch";
>     mux-controls = <&orientation_mux_controller>;
>     mux-control-names = "orientation";
>     #address-cells = <1>;
>     #size-cells = <0>;
>
>     port@0 {
>         reg = <0>;
>         orientation_switch_ss_in: endpoint {
>             remote-endpoint = <&usb_con0_ss_orientation>
>         };
>     };
>
>     port@1
>         reg = <1>;
>         orientation_switch_sbu_in: endpoint {
>             remote-endpoint = <&usb_con0_sbu_orientation>
>         };
>     };
>     // ... other ports similarly defined.
> };
>
> data_role_switch {
>     compatible = "typec-data-role-switch";
>     mux-controls = <&data_role_switch_controller>;
>     mux-control-names = "data_role";
>
>     port {
>         data_role_switch_in: endpoint {
>             remote-endpoint = <&usb_con0_ss_data_role>
>         };
>     };
> };
>
> <example_end>
>
> Would this be conformant to OF graph and usb-connector bindings
> requirements? We'll certainly send out a format PATCH/RFC series for
> this, but I was hoping to gauge whether we're thinking along the right lines.
>
> So, in effect this would mean:
> - New bindings(and compatible strings) to be added for:
>   typec-{orientation,data-role,mode}-switch.
> - Handling in Type C connector class to parse switches from OF graph.
> - Handling in Type C connector class for distinct switches for port@1
>   (SS lines) and port@2 (SBU lines).
>
> The only thing I'm confused about is how we can define these switch
> remote-endpoint bindings in usb-connector.yaml; the port can have an
> remote-endpoint, but can we specify what the parent of the remote-endpoint
> should have as a compatible string? Or do we not need to?
>
> Best regards,
>
> -Prashant
>

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Hi Rob,

Thought I'd check in again to see if you've had a chance to look at
this proposal.

Since Type C connector class framework assumes the existing
"{mode,orientation,data-role}-switch" bindings for non-DT platforms
already, as I see it, we can either:

1. Implement a different handling for DT platforms which utilizes port
end-points and update the Type C connector class framework to parse
those accordingly; this is what the above proposal suggests. It
reserves some end-points for the "switches" that the Type C connector
class framework expects and just follows the OF graph till it finds
the various switches. Other schemas that use usb-connector.yaml schema
can add more end-points as their use case deems needed, as long as
they're not the reserved ones.

<or>

2. Let various schemas that use usb-connector.schema add their own
bindings according to their requirements (in the example of
cros-ec-typec, it is adding the "*-switch" nodes directly under each
connector instead of using OF graph so that Type C connector class
framework can detect the switches, but there other examples for other
use cases).

I'm fine with either, but since this thread is now nearly 3 months
old, it would be nice to arrive at a decision.

Best regards,

On Mon, Jun 29, 2020 at 1:41 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Rob,
>
> Just following up on this. Would the below example align better with
> OF graph requirements?
>
> Example begins at <example_start>, but in summary:
> - port@1 (Superspeed) of usb-c-connector will have 3 endpoints (0 =
> goes to mode switch, 1 = goes to orientation switch, 2 = goes to data
> role switch)
> - port@2 (SBU) of usb-c-connector will have 2 endpoints (0 = goes to
> mode switch, 1 = goes to orientation switch)
> -These end points can go through arbitrarily long paths (including
> retimers) as long as they end up at the following devices:
>     a. device with compatible string "typec-mode-switch" for endpoint 0.
>     b. device with compatible string "typec-orientation-switch" for endpoint 1.
>     c. device with compatible string "typec-data-role-switch" for endpoint 2.
> - Connector class framework will perform the traversal from
> usb-c-connector port endpoints to the "*-switch" devices.
>
> Best regards,
>
> On Fri, Jun 12, 2020 at 10:34 AM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > Hi Rob,
> >
> > Thanks as always for your help in reviewing this proposal!
> >
> > Kindly see inline
> >
> > (Trimming text);
> > On Thu, Jun 11, 2020 at 02:00:47PM -0600, Rob Herring wrote:
> > > On Wed, Jun 10, 2020 at 11:49 AM Prashant Malani <pmalani@chromium.org> wrote:
> > > >
> > > > Hi Rob,
> > > >
> > > > On Wed, Jun 10, 2020 at 9:53 AM Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > > On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> > > >
> > > > I think the updated example handles this grouping (port@1 going to a
> > > > "SS mux") although as you said it should probably be a group of muxes,
> > > > but I think the example illustrates the point. Is that assessment
> > > > correct?
> > >
> > > Yes, but let's stop calling it a mux. It's a "USB Type C signal routing blob".
> >
> > Ack.
> >
> > Let's go with "-switch" ? That's what the connector class uses and it
> > conveys the meaning (unless that is a reserved keyword in DT).
> >
> > >
> > > > Would this block the addition of the "*-switch" properties? IIUC the
> > > > two are related but not dependent on each other.
> > > >
> > > > The *-switch properties are phandles which the Type C connector class
> > > > framework expects (and uses to get handles to those switches).
> > > > These would point to the "mux" or "group of mux" abstractions as noted earlier.
> > >
> > > You don't need them though. Walk the graph. You get the connector
> > > port@1 remote endpoint and then get its parent.
> > >
> >
> > I see; would it be something along the lines of this? (DT example
> > follows; search for "example_end" to jump to bottom):
> >
> > <example_start>
> >
> > connector@0 {
> >     compatible = "usb-c-connector";
> >     reg = <0>;
> >     power-role = "dual";
> >     data-role = "dual";
> >     try-power-role = "source";
> >     ....
> >     ports {
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> >
> >         port@0 {
> >             reg = <0>;
> >             usb_con_hs: endpoint {
> >                 remote-endpoint = <&foo_usb_hs_controller>;
> >             };
> >         };
> >
> >         port@1 {
> >             reg = <1>;
> >             #address-cells = <1>;
> >             #size-cells = <0>;
> >
> >             usb_con0_ss_mode: endpoint@0 {
> >                 reg = <0>
> >                 remote-endpoint = <&mode_switch_ss_in>;
> >             };
> >
> >             usb_con0_ss_orientation: endpoint@1 {
> >                         reg = <1>
> >                         remote-endpoint = <&orientation_switch_ss_in>;
> >             };
> >
> >             usb_con0_ss_data_role: endpoint@2 {
> >                         reg = <2>
> >                         remote-endpoint = <&data_role_switch_in>;
> >             };
> >         };
> >
> >         port@2 {
> >             reg = <2>;
> >             #address-cells = <1>;
> >             #size-cells = <0>;
> >             usb_con0_sbu_mode: endpoint@0 {
> >                         reg = <0>
> >                         remote-endpoint = <&mode_switch_sbu_in>;
> >             };
> >             usb_con0_sbu_orientation: endpoint@1 {
> >                         reg = <1>
> >                         remote-endpoint = <&orientation_switch_sbu_in>;
> >             };
> >         };
> >     };
> > };
> >
> > mode_switch {
> >     compatible = "typec-mode-switch";
> >     mux-controls = <&mode_mux_controller>;
> >     mux-control-names = "mode";
> >     #address-cells = <1>;
> >     #size-cells = <0>;
> >
> >     port@0 {
> >         reg = <0>;
> >         mode_switch_ss_in: endpoint {
> >             remote-endpoint = <&usb_con0_ss_mode>
> >         };
> >     };
> >
> >     port@1 {
> >         reg = <1>;
> >         mode_switch_out_usb3: endpoint {
> >             remote-endpoint = <&usb3_0_ep>
> >         };
> >     };
> >
> >     port@2 {
> >         reg = <2>;
> >         mode_switch_out_dp: endpoint {
> >             remote-endpoint = <&dp0_out_ep>
> >         };
> >     };
> >
> >     port@3 {
> >         reg = <3>;
> >         mode_switch_sbu_in: endpoint {
> >             remote-endpoint = <&usb_con0_sbu_mode>
> >         };
> >     };
> >     // ... other ports similarly defined.
> > };
> >
> > orientation_switch {
> >     compatible = "typec-orientation-switch";
> >     mux-controls = <&orientation_mux_controller>;
> >     mux-control-names = "orientation";
> >     #address-cells = <1>;
> >     #size-cells = <0>;
> >
> >     port@0 {
> >         reg = <0>;
> >         orientation_switch_ss_in: endpoint {
> >             remote-endpoint = <&usb_con0_ss_orientation>
> >         };
> >     };
> >
> >     port@1
> >         reg = <1>;
> >         orientation_switch_sbu_in: endpoint {
> >             remote-endpoint = <&usb_con0_sbu_orientation>
> >         };
> >     };
> >     // ... other ports similarly defined.
> > };
> >
> > data_role_switch {
> >     compatible = "typec-data-role-switch";
> >     mux-controls = <&data_role_switch_controller>;
> >     mux-control-names = "data_role";
> >
> >     port {
> >         data_role_switch_in: endpoint {
> >             remote-endpoint = <&usb_con0_ss_data_role>
> >         };
> >     };
> > };
> >
> > <example_end>
> >
> > Would this be conformant to OF graph and usb-connector bindings
> > requirements? We'll certainly send out a format PATCH/RFC series for
> > this, but I was hoping to gauge whether we're thinking along the right lines.
> >
> > So, in effect this would mean:
> > - New bindings(and compatible strings) to be added for:
> >   typec-{orientation,data-role,mode}-switch.
> > - Handling in Type C connector class to parse switches from OF graph.
> > - Handling in Type C connector class for distinct switches for port@1
> >   (SS lines) and port@2 (SBU lines).
> >
> > The only thing I'm confused about is how we can define these switch
> > remote-endpoint bindings in usb-connector.yaml; the port can have an
> > remote-endpoint, but can we specify what the parent of the remote-endpoint
> > should have as a compatible string? Or do we not need to?
> >
> > Best regards,
> >
> > -Prashant
> >

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Prashant Malani]

Hi Rob,

Just checking in again to see if you have any thoughts about the
proposal outlined in previous emails in this thread.

Best regards,

On Fri, Jul 10, 2020 at 1:51 AM Prashant Malani <pmalani@chromium.org> wrote:
>
> Hi Rob,
>
> Thought I'd check in again to see if you've had a chance to look at
> this proposal.
>
> Since Type C connector class framework assumes the existing
> "{mode,orientation,data-role}-switch" bindings for non-DT platforms
> already, as I see it, we can either:
>
> 1. Implement a different handling for DT platforms which utilizes port
> end-points and update the Type C connector class framework to parse
> those accordingly; this is what the above proposal suggests. It
> reserves some end-points for the "switches" that the Type C connector
> class framework expects and just follows the OF graph till it finds
> the various switches. Other schemas that use usb-connector.yaml schema
> can add more end-points as their use case deems needed, as long as
> they're not the reserved ones.
>
> <or>
>
> 2. Let various schemas that use usb-connector.schema add their own
> bindings according to their requirements (in the example of
> cros-ec-typec, it is adding the "*-switch" nodes directly under each
> connector instead of using OF graph so that Type C connector class
> framework can detect the switches, but there other examples for other
> use cases).
>
> I'm fine with either, but since this thread is now nearly 3 months
> old, it would be nice to arrive at a decision.
>
> Best regards,
>
> On Mon, Jun 29, 2020 at 1:41 PM Prashant Malani <pmalani@chromium.org> wrote:
> >
> > Hi Rob,
> >
> > Just following up on this. Would the below example align better with
> > OF graph requirements?
> >
> > Example begins at <example_start>, but in summary:
> > - port@1 (Superspeed) of usb-c-connector will have 3 endpoints (0 =
> > goes to mode switch, 1 = goes to orientation switch, 2 = goes to data
> > role switch)
> > - port@2 (SBU) of usb-c-connector will have 2 endpoints (0 = goes to
> > mode switch, 1 = goes to orientation switch)
> > -These end points can go through arbitrarily long paths (including
> > retimers) as long as they end up at the following devices:
> >     a. device with compatible string "typec-mode-switch" for endpoint 0.
> >     b. device with compatible string "typec-orientation-switch" for endpoint 1.
> >     c. device with compatible string "typec-data-role-switch" for endpoint 2.
> > - Connector class framework will perform the traversal from
> > usb-c-connector port endpoints to the "*-switch" devices.
> >
> > Best regards,
> >
> > On Fri, Jun 12, 2020 at 10:34 AM Prashant Malani <pmalani@chromium.org> wrote:
> > >
> > > Hi Rob,
> > >
> > > Thanks as always for your help in reviewing this proposal!
> > >
> > > Kindly see inline
> > >
> > > (Trimming text);
> > > On Thu, Jun 11, 2020 at 02:00:47PM -0600, Rob Herring wrote:
> > > > On Wed, Jun 10, 2020 at 11:49 AM Prashant Malani <pmalani@chromium.org> wrote:
> > > > >
> > > > > Hi Rob,
> > > > >
> > > > > On Wed, Jun 10, 2020 at 9:53 AM Rob Herring <robh@kernel.org> wrote:
> > > > > >
> > > > > > > On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> > > > >
> > > > > I think the updated example handles this grouping (port@1 going to a
> > > > > "SS mux") although as you said it should probably be a group of muxes,
> > > > > but I think the example illustrates the point. Is that assessment
> > > > > correct?
> > > >
> > > > Yes, but let's stop calling it a mux. It's a "USB Type C signal routing blob".
> > >
> > > Ack.
> > >
> > > Let's go with "-switch" ? That's what the connector class uses and it
> > > conveys the meaning (unless that is a reserved keyword in DT).
> > >
> > > >
> > > > > Would this block the addition of the "*-switch" properties? IIUC the
> > > > > two are related but not dependent on each other.
> > > > >
> > > > > The *-switch properties are phandles which the Type C connector class
> > > > > framework expects (and uses to get handles to those switches).
> > > > > These would point to the "mux" or "group of mux" abstractions as noted earlier.
> > > >
> > > > You don't need them though. Walk the graph. You get the connector
> > > > port@1 remote endpoint and then get its parent.
> > > >
> > >
> > > I see; would it be something along the lines of this? (DT example
> > > follows; search for "example_end" to jump to bottom):
> > >
> > > <example_start>
> > >
> > > connector@0 {
> > >     compatible = "usb-c-connector";
> > >     reg = <0>;
> > >     power-role = "dual";
> > >     data-role = "dual";
> > >     try-power-role = "source";
> > >     ....
> > >     ports {
> > >         #address-cells = <1>;
> > >         #size-cells = <0>;
> > >
> > >         port@0 {
> > >             reg = <0>;
> > >             usb_con_hs: endpoint {
> > >                 remote-endpoint = <&foo_usb_hs_controller>;
> > >             };
> > >         };
> > >
> > >         port@1 {
> > >             reg = <1>;
> > >             #address-cells = <1>;
> > >             #size-cells = <0>;
> > >
> > >             usb_con0_ss_mode: endpoint@0 {
> > >                 reg = <0>
> > >                 remote-endpoint = <&mode_switch_ss_in>;
> > >             };
> > >
> > >             usb_con0_ss_orientation: endpoint@1 {
> > >                         reg = <1>
> > >                         remote-endpoint = <&orientation_switch_ss_in>;
> > >             };
> > >
> > >             usb_con0_ss_data_role: endpoint@2 {
> > >                         reg = <2>
> > >                         remote-endpoint = <&data_role_switch_in>;
> > >             };
> > >         };
> > >
> > >         port@2 {
> > >             reg = <2>;
> > >             #address-cells = <1>;
> > >             #size-cells = <0>;
> > >             usb_con0_sbu_mode: endpoint@0 {
> > >                         reg = <0>
> > >                         remote-endpoint = <&mode_switch_sbu_in>;
> > >             };
> > >             usb_con0_sbu_orientation: endpoint@1 {
> > >                         reg = <1>
> > >                         remote-endpoint = <&orientation_switch_sbu_in>;
> > >             };
> > >         };
> > >     };
> > > };
> > >
> > > mode_switch {
> > >     compatible = "typec-mode-switch";
> > >     mux-controls = <&mode_mux_controller>;
> > >     mux-control-names = "mode";
> > >     #address-cells = <1>;
> > >     #size-cells = <0>;
> > >
> > >     port@0 {
> > >         reg = <0>;
> > >         mode_switch_ss_in: endpoint {
> > >             remote-endpoint = <&usb_con0_ss_mode>
> > >         };
> > >     };
> > >
> > >     port@1 {
> > >         reg = <1>;
> > >         mode_switch_out_usb3: endpoint {
> > >             remote-endpoint = <&usb3_0_ep>
> > >         };
> > >     };
> > >
> > >     port@2 {
> > >         reg = <2>;
> > >         mode_switch_out_dp: endpoint {
> > >             remote-endpoint = <&dp0_out_ep>
> > >         };
> > >     };
> > >
> > >     port@3 {
> > >         reg = <3>;
> > >         mode_switch_sbu_in: endpoint {
> > >             remote-endpoint = <&usb_con0_sbu_mode>
> > >         };
> > >     };
> > >     // ... other ports similarly defined.
> > > };
> > >
> > > orientation_switch {
> > >     compatible = "typec-orientation-switch";
> > >     mux-controls = <&orientation_mux_controller>;
> > >     mux-control-names = "orientation";
> > >     #address-cells = <1>;
> > >     #size-cells = <0>;
> > >
> > >     port@0 {
> > >         reg = <0>;
> > >         orientation_switch_ss_in: endpoint {
> > >             remote-endpoint = <&usb_con0_ss_orientation>
> > >         };
> > >     };
> > >
> > >     port@1
> > >         reg = <1>;
> > >         orientation_switch_sbu_in: endpoint {
> > >             remote-endpoint = <&usb_con0_sbu_orientation>
> > >         };
> > >     };
> > >     // ... other ports similarly defined.
> > > };
> > >
> > > data_role_switch {
> > >     compatible = "typec-data-role-switch";
> > >     mux-controls = <&data_role_switch_controller>;
> > >     mux-control-names = "data_role";
> > >
> > >     port {
> > >         data_role_switch_in: endpoint {
> > >             remote-endpoint = <&usb_con0_ss_data_role>
> > >         };
> > >     };
> > > };
> > >
> > > <example_end>
> > >
> > > Would this be conformant to OF graph and usb-connector bindings
> > > requirements? We'll certainly send out a format PATCH/RFC series for
> > > this, but I was hoping to gauge whether we're thinking along the right lines.
> > >
> > > So, in effect this would mean:
> > > - New bindings(and compatible strings) to be added for:
> > >   typec-{orientation,data-role,mode}-switch.
> > > - Handling in Type C connector class to parse switches from OF graph.
> > > - Handling in Type C connector class for distinct switches for port@1
> > >   (SS lines) and port@2 (SBU lines).
> > >
> > > The only thing I'm confused about is how we can define these switch
> > > remote-endpoint bindings in usb-connector.yaml; the port can have an
> > > remote-endpoint, but can we specify what the parent of the remote-endpoint
> > > should have as a compatible string? Or do we not need to?
> > >
> > > Best regards,
> > >
> > > -Prashant
> > >

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Heikki Krogerus]

On Wed, Jun 10, 2020 at 10:53:45AM -0600, Rob Herring wrote:
> On Wed, Jun 10, 2020 at 9:34 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> > > Hi Rob,
> > >
> > > Thanks again for the comments and feedback. Kindly see responses inline:
> > >
> > > (Trimming unrelated text from thread):
> > >
> > > On Tue, Jun 09, 2020 at 02:30:11PM -0600, Rob Herring wrote:
> > > > On Fri, May 29, 2020 at 5:30 PM Prashant Malani <pmalani@chromium.org> wrote:
> > > > >
> > > > > Nodes truncated and unrelated fields omitted in the interest of brevity:
> > > > >
> > > > > // Chrome OS EC Type C Port Manager.
> > > > > typec {
> > > > >     compatible = "google,cros-ec-typec";
> > > > >     #address-cells = <1>;
> > > > >     #size-cells = <0>;
> > > > >
> > > > >     connector@0 {
> > > > >         compatible = "usb-c-connector";
> > > > >         reg = <0>;
> > > > >         power-role = "dual";
> > > > >         data-role = "dual";
> > > > >         try-power-role = "source";
> > > > >         mode-switch = <&foo_mux>;
> > > > >         // Other switches can point to the same mux.
> > > > >         ....
> > > >
> > > > The connector is supposed to have 'ports' for USB2, USB3, and Aux
> > > > unless the parent is the USB controller.
> > > Understood; so, coupled with Heikki's explanation (see below for where
> > > I've pasted it), would it be something like so? (adding inline to the connector
> > > node definition):
> > >
> > >             ports {
> > >                 #address-cells = <1>;
> > >                 #size-cells = <0>;
> > >
> > >                 port@0 {
> > >                     reg = <0>;
> > >                     usb_con_hs: endpoint {
> > >                         remote-endpoint = <&foo_usb_hs_controller>;
> > >                     };
> > >                 };
> > >
> > >                 port@1 {
> > >                     reg = <1>;
> > >                     usb_con0_ss: endpoint@0 {
> > >                         remote-endpoint = <&mode_mux_in>;
> > >                     };
> > >                 };
> > >
> > >                 port@2 {
> > >                     reg = <2>;
> > >                     usb_con_sbu: endpoint {
> > >                         remote-endpoint = <&foo_dp_aux>;
> > >                     };
> > >                 };
> > >             };
> >
> > The pins that can be reassigned can in practice go anywhere. We can't
> > group them in any way. What do we do for example when the two sideband
> > pins go to different locations?
> 
> The sideband pins from the connector go to multiple places or the
> sideband signal from a controller go to multiple connectors? Either
> way, that's solved with multiple endpoints. In the former case, port@2
> would have multiple endpoints with all the possible connections. The
> general model of the graph is each port is a separate data channel and
> multiple endpoints are either a mux or fanout depending on the data
> direction.

No, that's not what I'm trying to ask here. Bad example, sorry. I'm
trying to understand why is it necessary to slit the connector into
three separate interfaces? There does not seem to be anything in the
kernel that would benefit from that. Why isn't the connector described
as a single interface in devicetree?

My concern with the three separate interfaces is that they may force
us to know in kernel which of the three interfaces are association
with a mode, and actually not just the mode, but the possible the pin
configurations of the mode. That is something that we may end up
having to hard code into the drivers, even though it does not provide
any useful information to us, and that would not be OK.

Right now they also allow making assumptions regarding the alternate
modes since there are no "bindings" for those, for example, if these
OF graph ports have an endpoint to the DP, it means DP alt mode is
supported. But that is of course not true. DisplayPort is used also
with other alternate modes. We must never make any assumptions based
on those interfaces. So again, why do we have them?

Either I'm missing something, or the devicetree description of the
Type-C connectors really is way too complex, way too "low level",
causing us potential problems without providing anything that we could
actually ever use in the operating system.


thanks,

-- 
heikki

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Rob Herring]

On Fri, Jun 12, 2020 at 6:46 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Jun 10, 2020 at 10:53:45AM -0600, Rob Herring wrote:
> > On Wed, Jun 10, 2020 at 9:34 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Tue, Jun 09, 2020 at 04:57:40PM -0700, Prashant Malani wrote:
> > > > Hi Rob,
> > > >
> > > > Thanks again for the comments and feedback. Kindly see responses inline:
> > > >
> > > > (Trimming unrelated text from thread):
> > > >
> > > > On Tue, Jun 09, 2020 at 02:30:11PM -0600, Rob Herring wrote:
> > > > > On Fri, May 29, 2020 at 5:30 PM Prashant Malani <pmalani@chromium.org> wrote:
> > > > > >
> > > > > > Nodes truncated and unrelated fields omitted in the interest of brevity:
> > > > > >
> > > > > > // Chrome OS EC Type C Port Manager.
> > > > > > typec {
> > > > > >     compatible = "google,cros-ec-typec";
> > > > > >     #address-cells = <1>;
> > > > > >     #size-cells = <0>;
> > > > > >
> > > > > >     connector@0 {
> > > > > >         compatible = "usb-c-connector";
> > > > > >         reg = <0>;
> > > > > >         power-role = "dual";
> > > > > >         data-role = "dual";
> > > > > >         try-power-role = "source";
> > > > > >         mode-switch = <&foo_mux>;
> > > > > >         // Other switches can point to the same mux.
> > > > > >         ....
> > > > >
> > > > > The connector is supposed to have 'ports' for USB2, USB3, and Aux
> > > > > unless the parent is the USB controller.
> > > > Understood; so, coupled with Heikki's explanation (see below for where
> > > > I've pasted it), would it be something like so? (adding inline to the connector
> > > > node definition):
> > > >
> > > >             ports {
> > > >                 #address-cells = <1>;
> > > >                 #size-cells = <0>;
> > > >
> > > >                 port@0 {
> > > >                     reg = <0>;
> > > >                     usb_con_hs: endpoint {
> > > >                         remote-endpoint = <&foo_usb_hs_controller>;
> > > >                     };
> > > >                 };
> > > >
> > > >                 port@1 {
> > > >                     reg = <1>;
> > > >                     usb_con0_ss: endpoint@0 {
> > > >                         remote-endpoint = <&mode_mux_in>;
> > > >                     };
> > > >                 };
> > > >
> > > >                 port@2 {
> > > >                     reg = <2>;
> > > >                     usb_con_sbu: endpoint {
> > > >                         remote-endpoint = <&foo_dp_aux>;
> > > >                     };
> > > >                 };
> > > >             };
> > >
> > > The pins that can be reassigned can in practice go anywhere. We can't
> > > group them in any way. What do we do for example when the two sideband
> > > pins go to different locations?
> >
> > The sideband pins from the connector go to multiple places or the
> > sideband signal from a controller go to multiple connectors? Either
> > way, that's solved with multiple endpoints. In the former case, port@2
> > would have multiple endpoints with all the possible connections. The
> > general model of the graph is each port is a separate data channel and
> > multiple endpoints are either a mux or fanout depending on the data
> > direction.
>
> No, that's not what I'm trying to ask here. Bad example, sorry. I'm
> trying to understand why is it necessary to slit the connector into
> three separate interfaces?

Because it is easily 3 separate h/w components (nodes) that have a
link to the connector.

> There does not seem to be anything in the
> kernel that would benefit from that. Why isn't the connector described
> as a single interface in devicetree?

The connector was designed pretty much before there was any TypeC
support in the kernel. Bindings shouldn't be designed around the
*current* needs of a particular OS.

The simplest case for the connector would be:

usb@1234 {
  compatible = "vendor,some-usb-2and3-with-typec-controller";
  ...
  connector {
    compatible = "usb-type-c-connector";
    /* No ports! */
  };

In this case, the h/w for
"vendor,some-usb-2and3-with-typec-controller" can handle everything
for the connector. Doesn't need anything for alt modes because either
it is not supported or there's only one possible source.

> My concern with the three separate interfaces is that they may force
> us to know in kernel which of the three interfaces are association
> with a mode, and actually not just the mode, but the possible the pin
> configurations of the mode. That is something that we may end up
> having to hard code into the drivers, even though it does not provide
> any useful information to us, and that would not be OK.

Either you hard-code things in DT with "generic", low-level binding or
you hard-code things in a driver. Or maybe in your case, things are
hard-coded in the EC? But most platforms don't have that.

> Right now they also allow making assumptions regarding the alternate
> modes since there are no "bindings" for those, for example, if these
> OF graph ports have an endpoint to the DP, it means DP alt mode is
> supported. But that is of course not true. DisplayPort is used also
> with other alternate modes. We must never make any assumptions based
> on those interfaces. So again, why do we have them?

I'm pretty sure we have cases where the alt mode is HDMI. Maybe
there's not yet been a case of multiple alt modes til now. So now the
binding needs to be extended.

> Either I'm missing something, or the devicetree description of the
> Type-C connectors really is way too complex, way too "low level",
> causing us potential problems without providing anything that we could
> actually ever use in the operating system.

Well, all bindings are a balancing act of being flexible enough vs.
high-level enough to be stable. What I need is something that's going
to work for everyone, not just CrOS. Adding a new property at time is
death by 1000 cuts and usually a sign of someone only fixing their own
immediate problem.

Rob

Re: [PATCH 1/2] dt-bindings: chrome: Add cros-ec-typec mux props

[Heikki Krogerus]

Hi Rob,

> > Either I'm missing something, or the devicetree description of the
> > Type-C connectors really is way too complex, way too "low level",
> > causing us potential problems without providing anything that we could
> > actually ever use in the operating system.
> 
> Well, all bindings are a balancing act of being flexible enough vs.
> high-level enough to be stable. What I need is something that's going
> to work for everyone, not just CrOS. Adding a new property at time is
> death by 1000 cuts and usually a sign of someone only fixing their own
> immediate problem.

If you referring to the phandles that are related to the muxes, then
those we will need. Those phandles point to the components that can
configure the muxes, but those components are not the muxes
themselves. On these platforms the muxes can not be configured
directly, and this is by the way the normal setup these days. I have
even alternate mode adapters that do not configure the mux directly
from the microcontroller. So we are not talking about the first
platform with this setup here.

The problem is that these components are not physically connected to
the connector, so we can't place them to the OF graph. The mux should
be placed into the graph (we may not be able to configure the muxes,
but we may still be able to read their status), but these components
should not.

I was really hoping that we could follow the "mux-controller"
bindings, but it just did not feel it would work perfectly with these
components that are not exactly the mux-controllers, but more like
proxies to the actual mux-controllers. We could probable ignore that
fact if the real mux-controllers were not visible to us, but
unfortunately they are visible to us. More importantly, the "muxes"
that we need to use with USB Type-C connectors will not always be
actual muxes at all. Depending on the platform, for example the USB
role switching will be handled by a mux, or a dual-role capable USB
controller.

But I'm open for suggestions here. The only thing that I can say for
sure about this is that we can't rely on OF graph with the muxes.
Right now I actually only have a wish that we had a reference array
that would hold all the phandles to the components that can configure
the lines behind the connector a bit like in mux bindings, but
regardless of were they real muxes, "proxy" to the muxes, or
anything else. Then we would need to define also somekind of device
property for each known function, like "orientation", "role" and so
on, that would return index to the component (mux, or what ever it is)
in the reference array that can handle that particular function.

I also don't feel comfortable using the name "mux" with these because,
they really will not always be muxes. That's why I talk about switches,
though I'm not sure if that's any better.

Sorry about the long mail.

thanks,

-- 
heikki