[PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Bjorn Andersson]

From: Bjorn Andersson <bjorn.andersson@linaro.org>

Introduce a binding for GPIO-based mux hardware used for connecting,
disconnecting and switching orientation of the SBU lines in USB Type-C
applications.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---

Changes since v1:
- Expanded the example to indicate how this fits with the TCPM
- Updated maintainer email address.

 .../devicetree/bindings/usb/gpio-sbu-mux.yaml | 110 ++++++++++++++++++
 1 file changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml

diff --git a/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml b/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml
new file mode 100644
index 000000000000..bf4b1d016e1f
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml
@@ -0,0 +1,110 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/usb/gpio-sbu-mux.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: GPIO-based SBU mux
+
+maintainers:
+  - Bjorn Andersson <andersson@kernel.org>
+
+description:
+  In USB Type-C applications the SBU lines needs to be connected, disconnected
+  and swapped depending on the altmode and orientation. This binding describes
+  a family of hardware solutions which switches between these modes using GPIO
+  signals.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - onnn,fsusb43l10x
+          - pericom,pi3usb102
+      - const: gpio-sbu-mux
+
+  enable-gpios:
+    description: Switch enable GPIO
+
+  select-gpios:
+    description: Orientation select
+
+  vcc-supply:
+    description: power supply
+
+  mode-switch:
+    description: Flag the port as possible handle of altmode switching
+    type: boolean
+
+  orientation-switch:
+    description: Flag the port as possible handler of orientation switching
+    type: boolean
+
+  port:
+    $ref: /schemas/graph.yaml#/properties/port
+    description:
+      A port node to link the SBU mux to a TypeC controller for the purpose of
+      handling altmode muxing and orientation switching.
+
+required:
+  - compatible
+  - enable-gpios
+  - select-gpios
+  - mode-switch
+  - orientation-switch
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    tcpm {
+        connector {
+            compatible = "usb-c-connector";
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    tcpm_hs_out: endpoint {
+                        remote-endpoint = <&usb_hs_phy_in>;
+                    };
+                };
+
+                port@1 {
+                    reg = <1>;
+                    tcpm_ss_out: endpoint {
+                        remote-endpoint = <&usb_ss_phy_in>;
+                    };
+                };
+
+                port@2 {
+                    reg = <2>;
+                    tcpm_sbu_out: endpoint {
+                        remote-endpoint = <&sbu_mux_in>;
+                    };
+                };
+            };
+        };
+    };
+
+    sbu-mux {
+        compatible = "pericom,pi3usb102", "gpio-sbu-mux";
+
+        enable-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
+        select-gpios = <&tlmm 164 GPIO_ACTIVE_HIGH>;
+
+        mode-switch;
+        orientation-switch;
+
+        port {
+            sbu_mux_in: endpoint {
+                remote-endpoint = <&tcpm_sbu_out>;
+            };
+        };
+    };
+...
-- 
2.37.3

[PATCH v2 2/2] usb: typec: mux: Introduce GPIO-based SBU mux

[Bjorn Andersson]

From: Bjorn Andersson <bjorn.andersson@linaro.org>

A design found in various Qualcomm-based boards is to use a USB switch,
controlled through a pair of GPIO lines to connect, disconnect and
switch the orientation of the SBU lines in USB Type-C applications.

This introduces a generic driver, which implements the typec_switch and
typec_mux interfaces to perform these operations.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---

Changes since v1:
- Fixed "swapped" being uninitialized for TYPEC_ORIENTATION_NONE in switch_set

 drivers/usb/typec/mux/Kconfig        |   6 +
 drivers/usb/typec/mux/Makefile       |   1 +
 drivers/usb/typec/mux/gpio-sbu-mux.c | 172 +++++++++++++++++++++++++++
 3 files changed, 179 insertions(+)
 create mode 100644 drivers/usb/typec/mux/gpio-sbu-mux.c

diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index 5eb2c17d72c1..c46fa4f9d3df 100644
--- a/drivers/usb/typec/mux/Kconfig
+++ b/drivers/usb/typec/mux/Kconfig
@@ -12,6 +12,12 @@ config TYPEC_MUX_FSA4480
 	  common USB Type-C connector.
 	  If compiled as a module, the module will be named fsa4480.
 
+config TYPEC_MUX_GPIO_SBU
+	tristate "Generic GPIO based SBU mux for USB Type-C applications"
+	help
+	  Say Y or M if your system uses a GPIO based mux for managing the
+	  connected state and the swapping of the SBU lines in a Type-C port.
+
 config TYPEC_MUX_PI3USB30532
 	tristate "Pericom PI3USB30532 Type-C cross switch driver"
 	depends on I2C
diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
index e52a56c16bfb..dda67e19b58b 100644
--- a/drivers/usb/typec/mux/Makefile
+++ b/drivers/usb/typec/mux/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_TYPEC_MUX_FSA4480)		+= fsa4480.o
+obj-$(CONFIG_TYPEC_MUX_GPIO_SBU)	+= gpio-sbu-mux.o
 obj-$(CONFIG_TYPEC_MUX_PI3USB30532)	+= pi3usb30532.o
 obj-$(CONFIG_TYPEC_MUX_INTEL_PMC)	+= intel_pmc_mux.o
diff --git a/drivers/usb/typec/mux/gpio-sbu-mux.c b/drivers/usb/typec/mux/gpio-sbu-mux.c
new file mode 100644
index 000000000000..f62516dafe8f
--- /dev/null
+++ b/drivers/usb/typec/mux/gpio-sbu-mux.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Linaro Ltd.
+ */
+
+#include <linux/bits.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/gpio/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/usb/typec_dp.h>
+#include <linux/usb/typec_mux.h>
+
+struct gpio_sbu_mux {
+	struct gpio_desc *enable_gpio;
+	struct gpio_desc *select_gpio;
+
+	struct typec_switch_dev *sw;
+	struct typec_mux_dev *mux;
+
+	struct mutex lock; /* protect enabled and swapped */
+	bool enabled;
+	bool swapped;
+};
+
+static int gpio_sbu_switch_set(struct typec_switch_dev *sw,
+			       enum typec_orientation orientation)
+{
+	struct gpio_sbu_mux *sbu_mux = typec_switch_get_drvdata(sw);
+	bool enabled;
+	bool swapped;
+
+	mutex_lock(&sbu_mux->lock);
+
+	enabled = sbu_mux->enabled;
+	swapped = sbu_mux->swapped;
+
+	switch (orientation) {
+	case TYPEC_ORIENTATION_NONE:
+		enabled = false;
+		break;
+	case TYPEC_ORIENTATION_NORMAL:
+		swapped = false;
+		break;
+	case TYPEC_ORIENTATION_REVERSE:
+		swapped = true;
+		break;
+	}
+
+	if (enabled != sbu_mux->enabled)
+		gpiod_set_value(sbu_mux->enable_gpio, enabled);
+
+	if (swapped != sbu_mux->swapped)
+		gpiod_set_value(sbu_mux->select_gpio, swapped);
+
+	sbu_mux->enabled = enabled;
+	sbu_mux->swapped = swapped;
+
+	mutex_unlock(&sbu_mux->lock);
+
+	return 0;
+}
+
+static int gpio_sbu_mux_set(struct typec_mux_dev *mux,
+			    struct typec_mux_state *state)
+{
+	struct gpio_sbu_mux *sbu_mux = typec_mux_get_drvdata(mux);
+
+	mutex_lock(&sbu_mux->lock);
+
+	switch (state->mode) {
+	case TYPEC_STATE_SAFE:
+	case TYPEC_STATE_USB:
+		sbu_mux->enabled = false;
+		break;
+	case TYPEC_DP_STATE_C:
+	case TYPEC_DP_STATE_D:
+	case TYPEC_DP_STATE_E:
+		sbu_mux->enabled = true;
+		break;
+	default:
+		break;
+	}
+
+	gpiod_set_value(sbu_mux->enable_gpio, sbu_mux->enabled);
+
+	mutex_unlock(&sbu_mux->lock);
+
+	return 0;
+}
+
+static int gpio_sbu_mux_probe(struct platform_device *pdev)
+{
+	struct typec_switch_desc sw_desc = { };
+	struct typec_mux_desc mux_desc = { };
+	struct device *dev = &pdev->dev;
+	struct gpio_sbu_mux *sbu_mux;
+
+	sbu_mux = devm_kzalloc(dev, sizeof(*sbu_mux), GFP_KERNEL);
+	if (!sbu_mux)
+		return -ENOMEM;
+
+	mutex_init(&sbu_mux->lock);
+
+	sbu_mux->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(sbu_mux->enable_gpio))
+		return dev_err_probe(dev, PTR_ERR(sbu_mux->enable_gpio),
+				     "unable to acquire enable gpio\n");
+
+	sbu_mux->select_gpio = devm_gpiod_get(dev, "select", GPIOD_OUT_LOW);
+	if (IS_ERR(sbu_mux->select_gpio))
+		return dev_err_probe(dev, PTR_ERR(sbu_mux->select_gpio),
+				     "unable to acquire select gpio\n");
+
+	sw_desc.drvdata = sbu_mux;
+	sw_desc.fwnode = dev_fwnode(dev);
+	sw_desc.set = gpio_sbu_switch_set;
+
+	sbu_mux->sw = typec_switch_register(dev, &sw_desc);
+	if (IS_ERR(sbu_mux->sw))
+		return dev_err_probe(dev, PTR_ERR(sbu_mux->sw),
+				     "failed to register typec switch\n");
+
+	mux_desc.drvdata = sbu_mux;
+	mux_desc.fwnode = dev_fwnode(dev);
+	mux_desc.set = gpio_sbu_mux_set;
+
+	sbu_mux->mux = typec_mux_register(dev, &mux_desc);
+	if (IS_ERR(sbu_mux->mux)) {
+		typec_switch_unregister(sbu_mux->sw);
+		return dev_err_probe(dev, PTR_ERR(sbu_mux->mux),
+				     "failed to register typec mux\n");
+	}
+
+	platform_set_drvdata(pdev, sbu_mux);
+
+	return 0;
+}
+
+static int gpio_sbu_mux_remove(struct platform_device *pdev)
+{
+	struct gpio_sbu_mux *sbu_mux = platform_get_drvdata(pdev);
+
+	gpiod_set_value(sbu_mux->enable_gpio, 0);
+
+	typec_mux_unregister(sbu_mux->mux);
+	typec_switch_unregister(sbu_mux->sw);
+
+	return 0;
+}
+
+static const struct of_device_id gpio_sbu_mux_match[] = {
+	{ .compatible = "gpio-sbu-mux", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, gpio_sbu_mux_match);
+
+static struct platform_driver gpio_sbu_mux_driver = {
+	.probe = gpio_sbu_mux_probe,
+	.remove = gpio_sbu_mux_remove,
+	.driver = {
+		.name = "gpio_sbu_mux",
+		.of_match_table = gpio_sbu_mux_match,
+	},
+};
+module_platform_driver(gpio_sbu_mux_driver);
+
+MODULE_DESCRIPTION("GPIO based SBU mux driver");
+MODULE_LICENSE("GPL");
-- 
2.37.3

Re: [PATCH v2 2/2] usb: typec: mux: Introduce GPIO-based SBU mux

[Heikki Krogerus]

On Thu, Jan 12, 2023 at 08:11:15PM -0800, Bjorn Andersson wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> A design found in various Qualcomm-based boards is to use a USB switch,
> controlled through a pair of GPIO lines to connect, disconnect and
> switch the orientation of the SBU lines in USB Type-C applications.
> 
> This introduces a generic driver, which implements the typec_switch and
> typec_mux interfaces to perform these operations.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>

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

> ---
> 
> Changes since v1:
> - Fixed "swapped" being uninitialized for TYPEC_ORIENTATION_NONE in switch_set
> 
>  drivers/usb/typec/mux/Kconfig        |   6 +
>  drivers/usb/typec/mux/Makefile       |   1 +
>  drivers/usb/typec/mux/gpio-sbu-mux.c | 172 +++++++++++++++++++++++++++
>  3 files changed, 179 insertions(+)
>  create mode 100644 drivers/usb/typec/mux/gpio-sbu-mux.c
> 
> diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
> index 5eb2c17d72c1..c46fa4f9d3df 100644
> --- a/drivers/usb/typec/mux/Kconfig
> +++ b/drivers/usb/typec/mux/Kconfig
> @@ -12,6 +12,12 @@ config TYPEC_MUX_FSA4480
>  	  common USB Type-C connector.
>  	  If compiled as a module, the module will be named fsa4480.
>  
> +config TYPEC_MUX_GPIO_SBU
> +	tristate "Generic GPIO based SBU mux for USB Type-C applications"
> +	help
> +	  Say Y or M if your system uses a GPIO based mux for managing the
> +	  connected state and the swapping of the SBU lines in a Type-C port.
> +
>  config TYPEC_MUX_PI3USB30532
>  	tristate "Pericom PI3USB30532 Type-C cross switch driver"
>  	depends on I2C
> diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
> index e52a56c16bfb..dda67e19b58b 100644
> --- a/drivers/usb/typec/mux/Makefile
> +++ b/drivers/usb/typec/mux/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  obj-$(CONFIG_TYPEC_MUX_FSA4480)		+= fsa4480.o
> +obj-$(CONFIG_TYPEC_MUX_GPIO_SBU)	+= gpio-sbu-mux.o
>  obj-$(CONFIG_TYPEC_MUX_PI3USB30532)	+= pi3usb30532.o
>  obj-$(CONFIG_TYPEC_MUX_INTEL_PMC)	+= intel_pmc_mux.o
> diff --git a/drivers/usb/typec/mux/gpio-sbu-mux.c b/drivers/usb/typec/mux/gpio-sbu-mux.c
> new file mode 100644
> index 000000000000..f62516dafe8f
> --- /dev/null
> +++ b/drivers/usb/typec/mux/gpio-sbu-mux.c
> @@ -0,0 +1,172 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Linaro Ltd.
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/usb/typec_dp.h>
> +#include <linux/usb/typec_mux.h>
> +
> +struct gpio_sbu_mux {
> +	struct gpio_desc *enable_gpio;
> +	struct gpio_desc *select_gpio;
> +
> +	struct typec_switch_dev *sw;
> +	struct typec_mux_dev *mux;
> +
> +	struct mutex lock; /* protect enabled and swapped */
> +	bool enabled;
> +	bool swapped;
> +};
> +
> +static int gpio_sbu_switch_set(struct typec_switch_dev *sw,
> +			       enum typec_orientation orientation)
> +{
> +	struct gpio_sbu_mux *sbu_mux = typec_switch_get_drvdata(sw);
> +	bool enabled;
> +	bool swapped;
> +
> +	mutex_lock(&sbu_mux->lock);
> +
> +	enabled = sbu_mux->enabled;
> +	swapped = sbu_mux->swapped;
> +
> +	switch (orientation) {
> +	case TYPEC_ORIENTATION_NONE:
> +		enabled = false;
> +		break;
> +	case TYPEC_ORIENTATION_NORMAL:
> +		swapped = false;
> +		break;
> +	case TYPEC_ORIENTATION_REVERSE:
> +		swapped = true;
> +		break;
> +	}
> +
> +	if (enabled != sbu_mux->enabled)
> +		gpiod_set_value(sbu_mux->enable_gpio, enabled);
> +
> +	if (swapped != sbu_mux->swapped)
> +		gpiod_set_value(sbu_mux->select_gpio, swapped);
> +
> +	sbu_mux->enabled = enabled;
> +	sbu_mux->swapped = swapped;
> +
> +	mutex_unlock(&sbu_mux->lock);
> +
> +	return 0;
> +}
> +
> +static int gpio_sbu_mux_set(struct typec_mux_dev *mux,
> +			    struct typec_mux_state *state)
> +{
> +	struct gpio_sbu_mux *sbu_mux = typec_mux_get_drvdata(mux);
> +
> +	mutex_lock(&sbu_mux->lock);
> +
> +	switch (state->mode) {
> +	case TYPEC_STATE_SAFE:
> +	case TYPEC_STATE_USB:
> +		sbu_mux->enabled = false;
> +		break;
> +	case TYPEC_DP_STATE_C:
> +	case TYPEC_DP_STATE_D:
> +	case TYPEC_DP_STATE_E:
> +		sbu_mux->enabled = true;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	gpiod_set_value(sbu_mux->enable_gpio, sbu_mux->enabled);
> +
> +	mutex_unlock(&sbu_mux->lock);
> +
> +	return 0;
> +}
> +
> +static int gpio_sbu_mux_probe(struct platform_device *pdev)
> +{
> +	struct typec_switch_desc sw_desc = { };
> +	struct typec_mux_desc mux_desc = { };
> +	struct device *dev = &pdev->dev;
> +	struct gpio_sbu_mux *sbu_mux;
> +
> +	sbu_mux = devm_kzalloc(dev, sizeof(*sbu_mux), GFP_KERNEL);
> +	if (!sbu_mux)
> +		return -ENOMEM;
> +
> +	mutex_init(&sbu_mux->lock);
> +
> +	sbu_mux->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(sbu_mux->enable_gpio))
> +		return dev_err_probe(dev, PTR_ERR(sbu_mux->enable_gpio),
> +				     "unable to acquire enable gpio\n");
> +
> +	sbu_mux->select_gpio = devm_gpiod_get(dev, "select", GPIOD_OUT_LOW);
> +	if (IS_ERR(sbu_mux->select_gpio))
> +		return dev_err_probe(dev, PTR_ERR(sbu_mux->select_gpio),
> +				     "unable to acquire select gpio\n");
> +
> +	sw_desc.drvdata = sbu_mux;
> +	sw_desc.fwnode = dev_fwnode(dev);
> +	sw_desc.set = gpio_sbu_switch_set;
> +
> +	sbu_mux->sw = typec_switch_register(dev, &sw_desc);
> +	if (IS_ERR(sbu_mux->sw))
> +		return dev_err_probe(dev, PTR_ERR(sbu_mux->sw),
> +				     "failed to register typec switch\n");
> +
> +	mux_desc.drvdata = sbu_mux;
> +	mux_desc.fwnode = dev_fwnode(dev);
> +	mux_desc.set = gpio_sbu_mux_set;
> +
> +	sbu_mux->mux = typec_mux_register(dev, &mux_desc);
> +	if (IS_ERR(sbu_mux->mux)) {
> +		typec_switch_unregister(sbu_mux->sw);
> +		return dev_err_probe(dev, PTR_ERR(sbu_mux->mux),
> +				     "failed to register typec mux\n");
> +	}
> +
> +	platform_set_drvdata(pdev, sbu_mux);
> +
> +	return 0;
> +}
> +
> +static int gpio_sbu_mux_remove(struct platform_device *pdev)
> +{
> +	struct gpio_sbu_mux *sbu_mux = platform_get_drvdata(pdev);
> +
> +	gpiod_set_value(sbu_mux->enable_gpio, 0);
> +
> +	typec_mux_unregister(sbu_mux->mux);
> +	typec_switch_unregister(sbu_mux->sw);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id gpio_sbu_mux_match[] = {
> +	{ .compatible = "gpio-sbu-mux", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, gpio_sbu_mux_match);
> +
> +static struct platform_driver gpio_sbu_mux_driver = {
> +	.probe = gpio_sbu_mux_probe,
> +	.remove = gpio_sbu_mux_remove,
> +	.driver = {
> +		.name = "gpio_sbu_mux",
> +		.of_match_table = gpio_sbu_mux_match,
> +	},
> +};
> +module_platform_driver(gpio_sbu_mux_driver);
> +
> +MODULE_DESCRIPTION("GPIO based SBU mux driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.37.3

-- 
heikki

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Rob Herring]

On Thu, Jan 12, 2023 at 08:11:14PM -0800, Bjorn Andersson wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Introduce a binding for GPIO-based mux hardware used for connecting,
> disconnecting and switching orientation of the SBU lines in USB Type-C
> applications.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
> 
> Changes since v1:
> - Expanded the example to indicate how this fits with the TCPM
> - Updated maintainer email address.
> 
>  .../devicetree/bindings/usb/gpio-sbu-mux.yaml | 110 ++++++++++++++++++
>  1 file changed, 110 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml b/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml
> new file mode 100644
> index 000000000000..bf4b1d016e1f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/usb/gpio-sbu-mux.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: GPIO-based SBU mux
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +
> +description:
> +  In USB Type-C applications the SBU lines needs to be connected, disconnected
> +  and swapped depending on the altmode and orientation. This binding describes
> +  a family of hardware solutions which switches between these modes using GPIO
> +  signals.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - onnn,fsusb43l10x
> +          - pericom,pi3usb102
> +      - const: gpio-sbu-mux
> +
> +  enable-gpios:
> +    description: Switch enable GPIO
> +
> +  select-gpios:
> +    description: Orientation select
> +
> +  vcc-supply:
> +    description: power supply
> +
> +  mode-switch:
> +    description: Flag the port as possible handle of altmode switching
> +    type: boolean
> +
> +  orientation-switch:
> +    description: Flag the port as possible handler of orientation switching
> +    type: boolean
> +
> +  port:
> +    $ref: /schemas/graph.yaml#/properties/port
> +    description:
> +      A port node to link the SBU mux to a TypeC controller for the purpose of
> +      handling altmode muxing and orientation switching.
> +
> +required:
> +  - compatible
> +  - enable-gpios
> +  - select-gpios
> +  - mode-switch
> +  - orientation-switch
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    tcpm {
> +        connector {
> +            compatible = "usb-c-connector";
> +
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                    reg = <0>;
> +                    tcpm_hs_out: endpoint {
> +                        remote-endpoint = <&usb_hs_phy_in>;
> +                    };
> +                };
> +
> +                port@1 {
> +                    reg = <1>;
> +                    tcpm_ss_out: endpoint {
> +                        remote-endpoint = <&usb_ss_phy_in>;
> +                    };
> +                };
> +
> +                port@2 {
> +                    reg = <2>;
> +                    tcpm_sbu_out: endpoint {
> +                        remote-endpoint = <&sbu_mux_in>;
> +                    };
> +                };
> +            };
> +        };
> +    };
> +
> +    sbu-mux {
> +        compatible = "pericom,pi3usb102", "gpio-sbu-mux";
> +
> +        enable-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
> +        select-gpios = <&tlmm 164 GPIO_ACTIVE_HIGH>;
> +
> +        mode-switch;
> +        orientation-switch;
> +
> +        port {
> +            sbu_mux_in: endpoint {
> +                remote-endpoint = <&tcpm_sbu_out>;
> +            };

Don't you need a connection to whatever drives SBU? Maybe your case is 
fixed because the phy does the DP/USB muxing? But the binding needs to 
support the worst case which I guess would be all the muxing/switching 
is done by separate board level components.

Rob

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Bjorn Andersson]

On Tue, Jan 17, 2023 at 11:56:57AM -0600, Rob Herring wrote:
> On Thu, Jan 12, 2023 at 08:11:14PM -0800, Bjorn Andersson wrote:
> > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > Introduce a binding for GPIO-based mux hardware used for connecting,
> > disconnecting and switching orientation of the SBU lines in USB Type-C
> > applications.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> > 
> > Changes since v1:
> > - Expanded the example to indicate how this fits with the TCPM
> > - Updated maintainer email address.
> > 
> >  .../devicetree/bindings/usb/gpio-sbu-mux.yaml | 110 ++++++++++++++++++
> >  1 file changed, 110 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml b/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml
> > new file mode 100644
> > index 000000000000..bf4b1d016e1f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml
> > @@ -0,0 +1,110 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/usb/gpio-sbu-mux.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: GPIO-based SBU mux
> > +
> > +maintainers:
> > +  - Bjorn Andersson <andersson@kernel.org>
> > +
> > +description:
> > +  In USB Type-C applications the SBU lines needs to be connected, disconnected
> > +  and swapped depending on the altmode and orientation. This binding describes
> > +  a family of hardware solutions which switches between these modes using GPIO
> > +  signals.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - onnn,fsusb43l10x
> > +          - pericom,pi3usb102
> > +      - const: gpio-sbu-mux
> > +
> > +  enable-gpios:
> > +    description: Switch enable GPIO
> > +
> > +  select-gpios:
> > +    description: Orientation select
> > +
> > +  vcc-supply:
> > +    description: power supply
> > +
> > +  mode-switch:
> > +    description: Flag the port as possible handle of altmode switching
> > +    type: boolean
> > +
> > +  orientation-switch:
> > +    description: Flag the port as possible handler of orientation switching
> > +    type: boolean
> > +
> > +  port:
> > +    $ref: /schemas/graph.yaml#/properties/port
> > +    description:
> > +      A port node to link the SBU mux to a TypeC controller for the purpose of
> > +      handling altmode muxing and orientation switching.
> > +
> > +required:
> > +  - compatible
> > +  - enable-gpios
> > +  - select-gpios
> > +  - mode-switch
> > +  - orientation-switch
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    tcpm {
> > +        connector {
> > +            compatible = "usb-c-connector";
> > +
> > +            ports {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +
> > +                port@0 {
> > +                    reg = <0>;
> > +                    tcpm_hs_out: endpoint {
> > +                        remote-endpoint = <&usb_hs_phy_in>;
> > +                    };
> > +                };
> > +
> > +                port@1 {
> > +                    reg = <1>;
> > +                    tcpm_ss_out: endpoint {
> > +                        remote-endpoint = <&usb_ss_phy_in>;
> > +                    };
> > +                };
> > +
> > +                port@2 {
> > +                    reg = <2>;
> > +                    tcpm_sbu_out: endpoint {
> > +                        remote-endpoint = <&sbu_mux_in>;
> > +                    };
> > +                };
> > +            };
> > +        };
> > +    };
> > +
> > +    sbu-mux {
> > +        compatible = "pericom,pi3usb102", "gpio-sbu-mux";
> > +
> > +        enable-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
> > +        select-gpios = <&tlmm 164 GPIO_ACTIVE_HIGH>;
> > +
> > +        mode-switch;
> > +        orientation-switch;
> > +
> > +        port {
> > +            sbu_mux_in: endpoint {
> > +                remote-endpoint = <&tcpm_sbu_out>;
> > +            };
> 
> Don't you need a connection to whatever drives SBU? Maybe your case is 
> fixed because the phy does the DP/USB muxing? But the binding needs to 
> support the worst case which I guess would be all the muxing/switching 
> is done by separate board level components.
> 

Perhaps I'm misunderstanding your request, but I think this is the worst
case you're talking about.

&usb_ss_phy_in is a reference to the PHY, which does switching/muxing of
the SuperSpeed lanes in the connector, but the PHY provides no control
over the SBU signals.

So this sbu-mux is a separate component between the SBU-pads on the SoC
and the usb-c-connector, referenced through he &sbu_mux_in reference.


So upon e.g. a orientation switch, the typec_switch_set() call the tcpm
implementation will request orientation switching from port@1 and port@2
(no orientation-switch on port@0/HS pins).

Regards,
Bjorn

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Rob Herring]

On Wed, Jan 18, 2023 at 10:08:11AM -0800, Bjorn Andersson wrote:
> On Tue, Jan 17, 2023 at 11:56:57AM -0600, Rob Herring wrote:
> > On Thu, Jan 12, 2023 at 08:11:14PM -0800, Bjorn Andersson wrote:
> > > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > 
> > > Introduce a binding for GPIO-based mux hardware used for connecting,
> > > disconnecting and switching orientation of the SBU lines in USB Type-C
> > > applications.
> > > 
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > ---


> > > +    tcpm {
> > > +        connector {
> > > +            compatible = "usb-c-connector";
> > > +
> > > +            ports {
> > > +                #address-cells = <1>;
> > > +                #size-cells = <0>;
> > > +
> > > +                port@0 {
> > > +                    reg = <0>;
> > > +                    tcpm_hs_out: endpoint {
> > > +                        remote-endpoint = <&usb_hs_phy_in>;
> > > +                    };
> > > +                };
> > > +
> > > +                port@1 {
> > > +                    reg = <1>;
> > > +                    tcpm_ss_out: endpoint {
> > > +                        remote-endpoint = <&usb_ss_phy_in>;
> > > +                    };
> > > +                };
> > > +
> > > +                port@2 {
> > > +                    reg = <2>;
> > > +                    tcpm_sbu_out: endpoint {
> > > +                        remote-endpoint = <&sbu_mux_in>;
> > > +                    };
> > > +                };
> > > +            };
> > > +        };
> > > +    };
> > > +
> > > +    sbu-mux {
> > > +        compatible = "pericom,pi3usb102", "gpio-sbu-mux";
> > > +
> > > +        enable-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
> > > +        select-gpios = <&tlmm 164 GPIO_ACTIVE_HIGH>;
> > > +
> > > +        mode-switch;
> > > +        orientation-switch;
> > > +
> > > +        port {
> > > +            sbu_mux_in: endpoint {
> > > +                remote-endpoint = <&tcpm_sbu_out>;
> > > +            };
> > 
> > Don't you need a connection to whatever drives SBU? Maybe your case is 
> > fixed because the phy does the DP/USB muxing? But the binding needs to 
> > support the worst case which I guess would be all the muxing/switching 
> > is done by separate board level components.
> > 
> 
> Perhaps I'm misunderstanding your request, but I think this is the worst
> case you're talking about.
> 
> &usb_ss_phy_in is a reference to the PHY, which does switching/muxing of
> the SuperSpeed lanes in the connector, but the PHY provides no control
> over the SBU signals.
> 
> So this sbu-mux is a separate component between the SBU-pads on the SoC
> and the usb-c-connector, referenced through he &sbu_mux_in reference.
> 
> 
> So upon e.g. a orientation switch, the typec_switch_set() call the tcpm
> implementation will request orientation switching from port@1 and port@2
> (no orientation-switch on port@0/HS pins).

'port@2' is supposed to define the connection to what controls SBU. The 
mux here switches the signals, but it doesn't control them. The mux 
should sit in the middle, but the graph terminates at the mux. You don't 
have a connection presumably because you know what the connection. 
Perhaps because there is only 1 connector and controller.

Suppose you have 2 connectors and 2 controllers which drive SBU 
signals. Also assume that the SBU signals are completely independent 
from what's driving the altmode SS signals. How would you describe that?

Rob

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Bjorn Andersson]

On Thu, Jan 19, 2023 at 10:11:32AM -0600, Rob Herring wrote:
> On Wed, Jan 18, 2023 at 10:08:11AM -0800, Bjorn Andersson wrote:
> > On Tue, Jan 17, 2023 at 11:56:57AM -0600, Rob Herring wrote:
> > > On Thu, Jan 12, 2023 at 08:11:14PM -0800, Bjorn Andersson wrote:
> > > > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > 
> > > > Introduce a binding for GPIO-based mux hardware used for connecting,
> > > > disconnecting and switching orientation of the SBU lines in USB Type-C
> > > > applications.
> > > > 
> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > > ---
> 
> 
> > > > +    tcpm {
> > > > +        connector {
> > > > +            compatible = "usb-c-connector";
> > > > +
> > > > +            ports {
> > > > +                #address-cells = <1>;
> > > > +                #size-cells = <0>;
> > > > +
> > > > +                port@0 {
> > > > +                    reg = <0>;
> > > > +                    tcpm_hs_out: endpoint {
> > > > +                        remote-endpoint = <&usb_hs_phy_in>;
> > > > +                    };
> > > > +                };
> > > > +
> > > > +                port@1 {
> > > > +                    reg = <1>;
> > > > +                    tcpm_ss_out: endpoint {
> > > > +                        remote-endpoint = <&usb_ss_phy_in>;
> > > > +                    };
> > > > +                };
> > > > +
> > > > +                port@2 {
> > > > +                    reg = <2>;
> > > > +                    tcpm_sbu_out: endpoint {
> > > > +                        remote-endpoint = <&sbu_mux_in>;
> > > > +                    };
> > > > +                };
> > > > +            };
> > > > +        };
> > > > +    };
> > > > +
> > > > +    sbu-mux {
> > > > +        compatible = "pericom,pi3usb102", "gpio-sbu-mux";
> > > > +
> > > > +        enable-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
> > > > +        select-gpios = <&tlmm 164 GPIO_ACTIVE_HIGH>;
> > > > +
> > > > +        mode-switch;
> > > > +        orientation-switch;
> > > > +
> > > > +        port {
> > > > +            sbu_mux_in: endpoint {
> > > > +                remote-endpoint = <&tcpm_sbu_out>;
> > > > +            };
> > > 
> > > Don't you need a connection to whatever drives SBU? Maybe your case is 
> > > fixed because the phy does the DP/USB muxing? But the binding needs to 
> > > support the worst case which I guess would be all the muxing/switching 
> > > is done by separate board level components.
> > > 
> > 
> > Perhaps I'm misunderstanding your request, but I think this is the worst
> > case you're talking about.
> > 
> > &usb_ss_phy_in is a reference to the PHY, which does switching/muxing of
> > the SuperSpeed lanes in the connector, but the PHY provides no control
> > over the SBU signals.
> > 
> > So this sbu-mux is a separate component between the SBU-pads on the SoC
> > and the usb-c-connector, referenced through he &sbu_mux_in reference.
> > 
> > 
> > So upon e.g. a orientation switch, the typec_switch_set() call the tcpm
> > implementation will request orientation switching from port@1 and port@2
> > (no orientation-switch on port@0/HS pins).
> 
> 'port@2' is supposed to define the connection to what controls SBU. The 
> mux here switches the signals, but it doesn't control them.

The SBU signals are driven by the SS PHY, on behalf of the DisplayPort
controller. These signals are  turned on/off as a result of the TCPM
indicating the HPD state to the DisplayPort controller.

There's a such not really a direct representation today of the entity
that drives the SBU lines. It happens to be a sub-block in
&usb_ss_phy_in, but I don't envision that we need/want any signaling
between the TCPM and the SBU-"driver".


I see that I missed that in the example above, your suggestion on how to
model that relationship (TCPM - DP controller) was to add an additional
endpoint in port@1. So that's the current design (but neither ports nor
endpoints are significant from an implementation point of view).

> The mux should sit in the middle, but the graph terminates at the mux.
> You don't have a connection presumably because you know what the
> connection.

But do you suggest that the graph should reference the entity that
drives the SBU signals? What about the discrete mux?

> Perhaps because there is only 1 connector and controller.
> 

There is one SBU mux, one DP controller and one SS PHY per
usb-c-connector.

> Suppose you have 2 connectors and 2 controllers which drive SBU 
> signals. Also assume that the SBU signals are completely independent 
> from what's driving the altmode SS signals. How would you describe that?
> 

This is the setup we have on e.g. SC8280XP CRD; where the TCPM has two
usb-c-connectors defined, each with their graph referencing the SS PHY,
DP controller and respective sbu-mux.

There's an incomplete example of this published at [1] (where the SS phy
isn't represented yet - and hence there's no control over the SS lanes,
nor is the HS lanes connected to the dwc3 for role switching).

Perhaps I'm misunderstanding your concerns though?

[1] https://github.com/andersson/kernel/blob/wip/sc8280xp-next-20220720/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts#L37

Regards,
Bjorn

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Rob Herring]

On Thu, Jan 19, 2023 at 11:40 AM Bjorn Andersson
<quic_bjorande@quicinc.com> wrote:
>
> On Thu, Jan 19, 2023 at 10:11:32AM -0600, Rob Herring wrote:
> > On Wed, Jan 18, 2023 at 10:08:11AM -0800, Bjorn Andersson wrote:
> > > On Tue, Jan 17, 2023 at 11:56:57AM -0600, Rob Herring wrote:
> > > > On Thu, Jan 12, 2023 at 08:11:14PM -0800, Bjorn Andersson wrote:
> > > > > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > >
> > > > > Introduce a binding for GPIO-based mux hardware used for connecting,
> > > > > disconnecting and switching orientation of the SBU lines in USB Type-C
> > > > > applications.
> > > > >
> > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > > > ---
> >
> >
> > > > > +    tcpm {
> > > > > +        connector {
> > > > > +            compatible = "usb-c-connector";
> > > > > +
> > > > > +            ports {
> > > > > +                #address-cells = <1>;
> > > > > +                #size-cells = <0>;
> > > > > +
> > > > > +                port@0 {
> > > > > +                    reg = <0>;
> > > > > +                    tcpm_hs_out: endpoint {
> > > > > +                        remote-endpoint = <&usb_hs_phy_in>;
> > > > > +                    };
> > > > > +                };
> > > > > +
> > > > > +                port@1 {
> > > > > +                    reg = <1>;
> > > > > +                    tcpm_ss_out: endpoint {
> > > > > +                        remote-endpoint = <&usb_ss_phy_in>;
> > > > > +                    };
> > > > > +                };
> > > > > +
> > > > > +                port@2 {
> > > > > +                    reg = <2>;
> > > > > +                    tcpm_sbu_out: endpoint {
> > > > > +                        remote-endpoint = <&sbu_mux_in>;
> > > > > +                    };
> > > > > +                };
> > > > > +            };
> > > > > +        };
> > > > > +    };
> > > > > +
> > > > > +    sbu-mux {
> > > > > +        compatible = "pericom,pi3usb102", "gpio-sbu-mux";
> > > > > +
> > > > > +        enable-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
> > > > > +        select-gpios = <&tlmm 164 GPIO_ACTIVE_HIGH>;
> > > > > +
> > > > > +        mode-switch;
> > > > > +        orientation-switch;
> > > > > +
> > > > > +        port {
> > > > > +            sbu_mux_in: endpoint {
> > > > > +                remote-endpoint = <&tcpm_sbu_out>;
> > > > > +            };
> > > >
> > > > Don't you need a connection to whatever drives SBU? Maybe your case is
> > > > fixed because the phy does the DP/USB muxing? But the binding needs to
> > > > support the worst case which I guess would be all the muxing/switching
> > > > is done by separate board level components.
> > > >
> > >
> > > Perhaps I'm misunderstanding your request, but I think this is the worst
> > > case you're talking about.
> > >
> > > &usb_ss_phy_in is a reference to the PHY, which does switching/muxing of
> > > the SuperSpeed lanes in the connector, but the PHY provides no control
> > > over the SBU signals.
> > >
> > > So this sbu-mux is a separate component between the SBU-pads on the SoC
> > > and the usb-c-connector, referenced through he &sbu_mux_in reference.
> > >
> > >
> > > So upon e.g. a orientation switch, the typec_switch_set() call the tcpm
> > > implementation will request orientation switching from port@1 and port@2
> > > (no orientation-switch on port@0/HS pins).
> >
> > 'port@2' is supposed to define the connection to what controls SBU. The
> > mux here switches the signals, but it doesn't control them.
>
> The SBU signals are driven by the SS PHY, on behalf of the DisplayPort
> controller. These signals are  turned on/off as a result of the TCPM
> indicating the HPD state to the DisplayPort controller.
>
> There's a such not really a direct representation today of the entity
> that drives the SBU lines. It happens to be a sub-block in
> &usb_ss_phy_in, but I don't envision that we need/want any signaling
> between the TCPM and the SBU-"driver".
>
>
> I see that I missed that in the example above, your suggestion on how to
> model that relationship (TCPM - DP controller) was to add an additional
> endpoint in port@1. So that's the current design (but neither ports nor
> endpoints are significant from an implementation point of view).
>
> > The mux should sit in the middle, but the graph terminates at the mux.
> > You don't have a connection presumably because you know what the
> > connection.
>
> But do you suggest that the graph should reference the entity that
> drives the SBU signals?

Yes, that was the original intent.

> What about the discrete mux?

You mean the mux in this binding, right? That should be in the middle:

DPaux --> SBUmux --> connector

Maybe the SS phy is in there too.

>
> > Perhaps because there is only 1 connector and controller.
> >
>
> There is one SBU mux, one DP controller and one SS PHY per
> usb-c-connector.
>
> > Suppose you have 2 connectors and 2 controllers which drive SBU
> > signals. Also assume that the SBU signals are completely independent
> > from what's driving the altmode SS signals. How would you describe that?
> >
>
> This is the setup we have on e.g. SC8280XP CRD; where the TCPM has two
> usb-c-connectors defined, each with their graph referencing the SS PHY,
> DP controller and respective sbu-mux.
>
> There's an incomplete example of this published at [1] (where the SS phy
> isn't represented yet - and hence there's no control over the SS lanes,
> nor is the HS lanes connected to the dwc3 for role switching).
>
> Perhaps I'm misunderstanding your concerns though?

That looks like you can assume who drives SBU based on the DP
controller. Probably a safe assumption for DP (that DP-aux is part of
the DP controller), but I was more worried about if you can't assume
that relationship. Take HDMI for example where the DDC signals can
come from anywhere. They could be part of the HDMI bridge, a general
purpose I2C bus off the SoC, or bitbanged GPIOs. Though from what I've
read, HDMI Altmode is dead. I don't know if the need to describe the
SBU connection would apply to anything else.

I guess this all boils down to whether the SBU mux should have a 2nd
optional port as the input for what drives it.

Rob

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Bjorn Andersson]

On Tue, Jan 24, 2023 at 10:00:12AM -0600, Rob Herring wrote:
> On Thu, Jan 19, 2023 at 11:40 AM Bjorn Andersson
> <quic_bjorande@quicinc.com> wrote:
> >
> > On Thu, Jan 19, 2023 at 10:11:32AM -0600, Rob Herring wrote:
> > > On Wed, Jan 18, 2023 at 10:08:11AM -0800, Bjorn Andersson wrote:
> > > > On Tue, Jan 17, 2023 at 11:56:57AM -0600, Rob Herring wrote:
> > > > > On Thu, Jan 12, 2023 at 08:11:14PM -0800, Bjorn Andersson wrote:
> > > > > > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > >
> > > > > > Introduce a binding for GPIO-based mux hardware used for connecting,
> > > > > > disconnecting and switching orientation of the SBU lines in USB Type-C
> > > > > > applications.
> > > > > >
> > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > > > > ---
> > >
> > >
> > > > > > +    tcpm {
> > > > > > +        connector {
> > > > > > +            compatible = "usb-c-connector";
> > > > > > +
> > > > > > +            ports {
> > > > > > +                #address-cells = <1>;
> > > > > > +                #size-cells = <0>;
> > > > > > +
> > > > > > +                port@0 {
> > > > > > +                    reg = <0>;
> > > > > > +                    tcpm_hs_out: endpoint {
> > > > > > +                        remote-endpoint = <&usb_hs_phy_in>;
> > > > > > +                    };
> > > > > > +                };
> > > > > > +
> > > > > > +                port@1 {
> > > > > > +                    reg = <1>;
> > > > > > +                    tcpm_ss_out: endpoint {
> > > > > > +                        remote-endpoint = <&usb_ss_phy_in>;
> > > > > > +                    };
> > > > > > +                };
> > > > > > +
> > > > > > +                port@2 {
> > > > > > +                    reg = <2>;
> > > > > > +                    tcpm_sbu_out: endpoint {
> > > > > > +                        remote-endpoint = <&sbu_mux_in>;
> > > > > > +                    };
> > > > > > +                };
> > > > > > +            };
> > > > > > +        };
> > > > > > +    };
> > > > > > +
> > > > > > +    sbu-mux {
> > > > > > +        compatible = "pericom,pi3usb102", "gpio-sbu-mux";
> > > > > > +
> > > > > > +        enable-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
> > > > > > +        select-gpios = <&tlmm 164 GPIO_ACTIVE_HIGH>;
> > > > > > +
> > > > > > +        mode-switch;
> > > > > > +        orientation-switch;
> > > > > > +
> > > > > > +        port {
> > > > > > +            sbu_mux_in: endpoint {
> > > > > > +                remote-endpoint = <&tcpm_sbu_out>;
> > > > > > +            };
> > > > >
> > > > > Don't you need a connection to whatever drives SBU? Maybe your case is
> > > > > fixed because the phy does the DP/USB muxing? But the binding needs to
> > > > > support the worst case which I guess would be all the muxing/switching
> > > > > is done by separate board level components.
> > > > >
> > > >
> > > > Perhaps I'm misunderstanding your request, but I think this is the worst
> > > > case you're talking about.
> > > >
> > > > &usb_ss_phy_in is a reference to the PHY, which does switching/muxing of
> > > > the SuperSpeed lanes in the connector, but the PHY provides no control
> > > > over the SBU signals.
> > > >
> > > > So this sbu-mux is a separate component between the SBU-pads on the SoC
> > > > and the usb-c-connector, referenced through he &sbu_mux_in reference.
> > > >
> > > >
> > > > So upon e.g. a orientation switch, the typec_switch_set() call the tcpm
> > > > implementation will request orientation switching from port@1 and port@2
> > > > (no orientation-switch on port@0/HS pins).
> > >
> > > 'port@2' is supposed to define the connection to what controls SBU. The
> > > mux here switches the signals, but it doesn't control them.
> >
> > The SBU signals are driven by the SS PHY, on behalf of the DisplayPort
> > controller. These signals are  turned on/off as a result of the TCPM
> > indicating the HPD state to the DisplayPort controller.
> >
> > There's a such not really a direct representation today of the entity
> > that drives the SBU lines. It happens to be a sub-block in
> > &usb_ss_phy_in, but I don't envision that we need/want any signaling
> > between the TCPM and the SBU-"driver".
> >
> >
> > I see that I missed that in the example above, your suggestion on how to
> > model that relationship (TCPM - DP controller) was to add an additional
> > endpoint in port@1. So that's the current design (but neither ports nor
> > endpoints are significant from an implementation point of view).
> >
> > > The mux should sit in the middle, but the graph terminates at the mux.
> > > You don't have a connection presumably because you know what the
> > > connection.
> >
> > But do you suggest that the graph should reference the entity that
> > drives the SBU signals?
> 
> Yes, that was the original intent.
> 

Directly from the connector, or just indirectly?

> > What about the discrete mux?
> 
> You mean the mux in this binding, right? That should be in the middle:
> 
> DPaux --> SBUmux --> connector
> 
> Maybe the SS phy is in there too.
> 

The signal originally comes from the DP controller, the analog
electronics lives in the SS phy, then the signal goes to the SBU mux and
finally to the connector.

> >
> > > Perhaps because there is only 1 connector and controller.
> > >
> >
> > There is one SBU mux, one DP controller and one SS PHY per
> > usb-c-connector.
> >
> > > Suppose you have 2 connectors and 2 controllers which drive SBU
> > > signals. Also assume that the SBU signals are completely independent
> > > from what's driving the altmode SS signals. How would you describe that?
> > >
> >
> > This is the setup we have on e.g. SC8280XP CRD; where the TCPM has two
> > usb-c-connectors defined, each with their graph referencing the SS PHY,
> > DP controller and respective sbu-mux.
> >
> > There's an incomplete example of this published at [1] (where the SS phy
> > isn't represented yet - and hence there's no control over the SS lanes,
> > nor is the HS lanes connected to the dwc3 for role switching).
> >
> > Perhaps I'm misunderstanding your concerns though?
> 
> That looks like you can assume who drives SBU based on the DP
> controller. Probably a safe assumption for DP (that DP-aux is part of
> the DP controller), but I was more worried about if you can't assume
> that relationship. Take HDMI for example where the DDC signals can
> come from anywhere. They could be part of the HDMI bridge, a general
> purpose I2C bus off the SoC, or bitbanged GPIOs. Though from what I've
> read, HDMI Altmode is dead. I don't know if the need to describe the
> SBU connection would apply to anything else.
> 
> I guess this all boils down to whether the SBU mux should have a 2nd
> optional port as the input for what drives it.
> 

Are you saying that the connector should link with the mux and then the
source of the signal should be daisy chained? Or that we should just
link both of them as two separate endpoints from the connector?

Regards,
Bjorn

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Rob Herring]

On Tue, Jan 24, 2023 at 11:04 AM Bjorn Andersson
<quic_bjorande@quicinc.com> wrote:
>
> On Tue, Jan 24, 2023 at 10:00:12AM -0600, Rob Herring wrote:
> > On Thu, Jan 19, 2023 at 11:40 AM Bjorn Andersson
> > <quic_bjorande@quicinc.com> wrote:
> > >
> > > On Thu, Jan 19, 2023 at 10:11:32AM -0600, Rob Herring wrote:
> > > > On Wed, Jan 18, 2023 at 10:08:11AM -0800, Bjorn Andersson wrote:
> > > > > On Tue, Jan 17, 2023 at 11:56:57AM -0600, Rob Herring wrote:
> > > > > > On Thu, Jan 12, 2023 at 08:11:14PM -0800, Bjorn Andersson wrote:
> > > > > > > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > > >
> > > > > > > Introduce a binding for GPIO-based mux hardware used for connecting,
> > > > > > > disconnecting and switching orientation of the SBU lines in USB Type-C
> > > > > > > applications.
> > > > > > >
> > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > > > > > ---
> > > >
> > > >
> > > > > > > +    tcpm {
> > > > > > > +        connector {
> > > > > > > +            compatible = "usb-c-connector";
> > > > > > > +
> > > > > > > +            ports {
> > > > > > > +                #address-cells = <1>;
> > > > > > > +                #size-cells = <0>;
> > > > > > > +
> > > > > > > +                port@0 {
> > > > > > > +                    reg = <0>;
> > > > > > > +                    tcpm_hs_out: endpoint {
> > > > > > > +                        remote-endpoint = <&usb_hs_phy_in>;
> > > > > > > +                    };
> > > > > > > +                };
> > > > > > > +
> > > > > > > +                port@1 {
> > > > > > > +                    reg = <1>;
> > > > > > > +                    tcpm_ss_out: endpoint {
> > > > > > > +                        remote-endpoint = <&usb_ss_phy_in>;
> > > > > > > +                    };
> > > > > > > +                };
> > > > > > > +
> > > > > > > +                port@2 {
> > > > > > > +                    reg = <2>;
> > > > > > > +                    tcpm_sbu_out: endpoint {
> > > > > > > +                        remote-endpoint = <&sbu_mux_in>;
> > > > > > > +                    };
> > > > > > > +                };
> > > > > > > +            };
> > > > > > > +        };
> > > > > > > +    };
> > > > > > > +
> > > > > > > +    sbu-mux {
> > > > > > > +        compatible = "pericom,pi3usb102", "gpio-sbu-mux";
> > > > > > > +
> > > > > > > +        enable-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
> > > > > > > +        select-gpios = <&tlmm 164 GPIO_ACTIVE_HIGH>;
> > > > > > > +
> > > > > > > +        mode-switch;
> > > > > > > +        orientation-switch;
> > > > > > > +
> > > > > > > +        port {
> > > > > > > +            sbu_mux_in: endpoint {
> > > > > > > +                remote-endpoint = <&tcpm_sbu_out>;
> > > > > > > +            };
> > > > > >
> > > > > > Don't you need a connection to whatever drives SBU? Maybe your case is
> > > > > > fixed because the phy does the DP/USB muxing? But the binding needs to
> > > > > > support the worst case which I guess would be all the muxing/switching
> > > > > > is done by separate board level components.
> > > > > >
> > > > >
> > > > > Perhaps I'm misunderstanding your request, but I think this is the worst
> > > > > case you're talking about.
> > > > >
> > > > > &usb_ss_phy_in is a reference to the PHY, which does switching/muxing of
> > > > > the SuperSpeed lanes in the connector, but the PHY provides no control
> > > > > over the SBU signals.
> > > > >
> > > > > So this sbu-mux is a separate component between the SBU-pads on the SoC
> > > > > and the usb-c-connector, referenced through he &sbu_mux_in reference.
> > > > >
> > > > >
> > > > > So upon e.g. a orientation switch, the typec_switch_set() call the tcpm
> > > > > implementation will request orientation switching from port@1 and port@2
> > > > > (no orientation-switch on port@0/HS pins).
> > > >
> > > > 'port@2' is supposed to define the connection to what controls SBU. The
> > > > mux here switches the signals, but it doesn't control them.
> > >
> > > The SBU signals are driven by the SS PHY, on behalf of the DisplayPort
> > > controller. These signals are  turned on/off as a result of the TCPM
> > > indicating the HPD state to the DisplayPort controller.
> > >
> > > There's a such not really a direct representation today of the entity
> > > that drives the SBU lines. It happens to be a sub-block in
> > > &usb_ss_phy_in, but I don't envision that we need/want any signaling
> > > between the TCPM and the SBU-"driver".
> > >
> > >
> > > I see that I missed that in the example above, your suggestion on how to
> > > model that relationship (TCPM - DP controller) was to add an additional
> > > endpoint in port@1. So that's the current design (but neither ports nor
> > > endpoints are significant from an implementation point of view).
> > >
> > > > The mux should sit in the middle, but the graph terminates at the mux.
> > > > You don't have a connection presumably because you know what the
> > > > connection.
> > >
> > > But do you suggest that the graph should reference the entity that
> > > drives the SBU signals?
> >
> > Yes, that was the original intent.
> >
>
> Directly from the connector, or just indirectly?
>
> > > What about the discrete mux?
> >
> > You mean the mux in this binding, right? That should be in the middle:
> >
> > DPaux --> SBUmux --> connector
> >
> > Maybe the SS phy is in there too.
> >
>
> The signal originally comes from the DP controller, the analog
> electronics lives in the SS phy, then the signal goes to the SBU mux and
> finally to the connector.
>
> > >
> > > > Perhaps because there is only 1 connector and controller.
> > > >
> > >
> > > There is one SBU mux, one DP controller and one SS PHY per
> > > usb-c-connector.
> > >
> > > > Suppose you have 2 connectors and 2 controllers which drive SBU
> > > > signals. Also assume that the SBU signals are completely independent
> > > > from what's driving the altmode SS signals. How would you describe that?
> > > >
> > >
> > > This is the setup we have on e.g. SC8280XP CRD; where the TCPM has two
> > > usb-c-connectors defined, each with their graph referencing the SS PHY,
> > > DP controller and respective sbu-mux.
> > >
> > > There's an incomplete example of this published at [1] (where the SS phy
> > > isn't represented yet - and hence there's no control over the SS lanes,
> > > nor is the HS lanes connected to the dwc3 for role switching).
> > >
> > > Perhaps I'm misunderstanding your concerns though?
> >
> > That looks like you can assume who drives SBU based on the DP
> > controller. Probably a safe assumption for DP (that DP-aux is part of
> > the DP controller), but I was more worried about if you can't assume
> > that relationship. Take HDMI for example where the DDC signals can
> > come from anywhere. They could be part of the HDMI bridge, a general
> > purpose I2C bus off the SoC, or bitbanged GPIOs. Though from what I've
> > read, HDMI Altmode is dead. I don't know if the need to describe the
> > SBU connection would apply to anything else.
> >
> > I guess this all boils down to whether the SBU mux should have a 2nd
> > optional port as the input for what drives it.
> >
>
> Are you saying that the connector should link with the mux and then the
> source of the signal should be daisy chained? Or that we should just
> link both of them as two separate endpoints from the connector?

The former. The data path of the signal in h/w should match in the DT
graph. The caveat being we don't normally show PHYs in that mix
because they are somewhat transparent. That's maybe becoming less true
with routing or muxing included in PHYs.

Rob

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Bjorn Andersson]

On Tue, Jan 24, 2023 at 08:31:13PM -0600, Rob Herring wrote:
> On Tue, Jan 24, 2023 at 11:04 AM Bjorn Andersson
> <quic_bjorande@quicinc.com> wrote:
> >
> > On Tue, Jan 24, 2023 at 10:00:12AM -0600, Rob Herring wrote:
> > > On Thu, Jan 19, 2023 at 11:40 AM Bjorn Andersson
> > > <quic_bjorande@quicinc.com> wrote:
> > > >
> > > > On Thu, Jan 19, 2023 at 10:11:32AM -0600, Rob Herring wrote:
> > > > > On Wed, Jan 18, 2023 at 10:08:11AM -0800, Bjorn Andersson wrote:
> > > > > > On Tue, Jan 17, 2023 at 11:56:57AM -0600, Rob Herring wrote:
> > > > > > > On Thu, Jan 12, 2023 at 08:11:14PM -0800, Bjorn Andersson wrote:
> > > > > > > > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > > > >
> > > > > > > > Introduce a binding for GPIO-based mux hardware used for connecting,
> > > > > > > > disconnecting and switching orientation of the SBU lines in USB Type-C
> > > > > > > > applications.
> > > > > > > >
> > > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > > > > > > ---
> > > > >
> > > > >
> > > > > > > > +    tcpm {
> > > > > > > > +        connector {
> > > > > > > > +            compatible = "usb-c-connector";
> > > > > > > > +
> > > > > > > > +            ports {
> > > > > > > > +                #address-cells = <1>;
> > > > > > > > +                #size-cells = <0>;
> > > > > > > > +
> > > > > > > > +                port@0 {
> > > > > > > > +                    reg = <0>;
> > > > > > > > +                    tcpm_hs_out: endpoint {
> > > > > > > > +                        remote-endpoint = <&usb_hs_phy_in>;
> > > > > > > > +                    };
> > > > > > > > +                };
> > > > > > > > +
> > > > > > > > +                port@1 {
> > > > > > > > +                    reg = <1>;
> > > > > > > > +                    tcpm_ss_out: endpoint {
> > > > > > > > +                        remote-endpoint = <&usb_ss_phy_in>;
> > > > > > > > +                    };
> > > > > > > > +                };
> > > > > > > > +
> > > > > > > > +                port@2 {
> > > > > > > > +                    reg = <2>;
> > > > > > > > +                    tcpm_sbu_out: endpoint {
> > > > > > > > +                        remote-endpoint = <&sbu_mux_in>;
> > > > > > > > +                    };
> > > > > > > > +                };
> > > > > > > > +            };
> > > > > > > > +        };
> > > > > > > > +    };
> > > > > > > > +
> > > > > > > > +    sbu-mux {
> > > > > > > > +        compatible = "pericom,pi3usb102", "gpio-sbu-mux";
> > > > > > > > +
> > > > > > > > +        enable-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
> > > > > > > > +        select-gpios = <&tlmm 164 GPIO_ACTIVE_HIGH>;
> > > > > > > > +
> > > > > > > > +        mode-switch;
> > > > > > > > +        orientation-switch;
> > > > > > > > +
> > > > > > > > +        port {
> > > > > > > > +            sbu_mux_in: endpoint {
> > > > > > > > +                remote-endpoint = <&tcpm_sbu_out>;
> > > > > > > > +            };
> > > > > > >
> > > > > > > Don't you need a connection to whatever drives SBU? Maybe your case is
> > > > > > > fixed because the phy does the DP/USB muxing? But the binding needs to
> > > > > > > support the worst case which I guess would be all the muxing/switching
> > > > > > > is done by separate board level components.
> > > > > > >
> > > > > >
> > > > > > Perhaps I'm misunderstanding your request, but I think this is the worst
> > > > > > case you're talking about.
> > > > > >
> > > > > > &usb_ss_phy_in is a reference to the PHY, which does switching/muxing of
> > > > > > the SuperSpeed lanes in the connector, but the PHY provides no control
> > > > > > over the SBU signals.
> > > > > >
> > > > > > So this sbu-mux is a separate component between the SBU-pads on the SoC
> > > > > > and the usb-c-connector, referenced through he &sbu_mux_in reference.
> > > > > >
> > > > > >
> > > > > > So upon e.g. a orientation switch, the typec_switch_set() call the tcpm
> > > > > > implementation will request orientation switching from port@1 and port@2
> > > > > > (no orientation-switch on port@0/HS pins).
> > > > >
> > > > > 'port@2' is supposed to define the connection to what controls SBU. The
> > > > > mux here switches the signals, but it doesn't control them.
> > > >
> > > > The SBU signals are driven by the SS PHY, on behalf of the DisplayPort
> > > > controller. These signals are  turned on/off as a result of the TCPM
> > > > indicating the HPD state to the DisplayPort controller.
> > > >
> > > > There's a such not really a direct representation today of the entity
> > > > that drives the SBU lines. It happens to be a sub-block in
> > > > &usb_ss_phy_in, but I don't envision that we need/want any signaling
> > > > between the TCPM and the SBU-"driver".
> > > >
> > > >
> > > > I see that I missed that in the example above, your suggestion on how to
> > > > model that relationship (TCPM - DP controller) was to add an additional
> > > > endpoint in port@1. So that's the current design (but neither ports nor
> > > > endpoints are significant from an implementation point of view).
> > > >
> > > > > The mux should sit in the middle, but the graph terminates at the mux.
> > > > > You don't have a connection presumably because you know what the
> > > > > connection.
> > > >
> > > > But do you suggest that the graph should reference the entity that
> > > > drives the SBU signals?
> > >
> > > Yes, that was the original intent.
> > >
> >
> > Directly from the connector, or just indirectly?
> >
> > > > What about the discrete mux?
> > >
> > > You mean the mux in this binding, right? That should be in the middle:
> > >
> > > DPaux --> SBUmux --> connector
> > >
> > > Maybe the SS phy is in there too.
> > >
> >
> > The signal originally comes from the DP controller, the analog
> > electronics lives in the SS phy, then the signal goes to the SBU mux and
> > finally to the connector.
> >
> > > >
> > > > > Perhaps because there is only 1 connector and controller.
> > > > >
> > > >
> > > > There is one SBU mux, one DP controller and one SS PHY per
> > > > usb-c-connector.
> > > >
> > > > > Suppose you have 2 connectors and 2 controllers which drive SBU
> > > > > signals. Also assume that the SBU signals are completely independent
> > > > > from what's driving the altmode SS signals. How would you describe that?
> > > > >
> > > >
> > > > This is the setup we have on e.g. SC8280XP CRD; where the TCPM has two
> > > > usb-c-connectors defined, each with their graph referencing the SS PHY,
> > > > DP controller and respective sbu-mux.
> > > >
> > > > There's an incomplete example of this published at [1] (where the SS phy
> > > > isn't represented yet - and hence there's no control over the SS lanes,
> > > > nor is the HS lanes connected to the dwc3 for role switching).
> > > >
> > > > Perhaps I'm misunderstanding your concerns though?
> > >
> > > That looks like you can assume who drives SBU based on the DP
> > > controller. Probably a safe assumption for DP (that DP-aux is part of
> > > the DP controller), but I was more worried about if you can't assume
> > > that relationship. Take HDMI for example where the DDC signals can
> > > come from anywhere. They could be part of the HDMI bridge, a general
> > > purpose I2C bus off the SoC, or bitbanged GPIOs. Though from what I've
> > > read, HDMI Altmode is dead. I don't know if the need to describe the
> > > SBU connection would apply to anything else.
> > >
> > > I guess this all boils down to whether the SBU mux should have a 2nd
> > > optional port as the input for what drives it.
> > >
> >
> > Are you saying that the connector should link with the mux and then the
> > source of the signal should be daisy chained? Or that we should just
> > link both of them as two separate endpoints from the connector?
> 
> The former. The data path of the signal in h/w should match in the DT
> graph. The caveat being we don't normally show PHYs in that mix
> because they are somewhat transparent. That's maybe becoming less true
> with routing or muxing included in PHYs.
> 

We have discussed and prototyped this a few times now in the Qualcomm
community, and I am not a fan of having to add forwarding-logic to each
implementation of a Type-C mux/switch, which in some configuration might
have an entity behind it that needs the notifications.

But I don't think there's a concern for this binding (in my
implementation), there's currently no mode/orientation switching
happening beyond this entity.



That said, if we're to represent each signal path to the connector,
I would like to bring up this problem again:
https://lore.kernel.org/all/20220520164810.141400-1-bjorn.andersson@linaro.org/

port@0 represent the HS signals going to the connector, port@1 the SS
signals going to the connector, port@2 the SBU signals going to the
connector.

But I need some representation of the HPD (hot plug detection) "signal"
(there is no actual signal path in hardware, this is a virtual
notification) going _from_ the connector to the DisplayPort controller.

We discussed this (perhaps in person, as there's no trace on lkml?) and
you suggested that I use a second endpoint under port@1, instead of
introducing a fourth port.


I'm fine either way, but I don't think it would be convenient nor
representable to daisy chain this behind any of the existing endpoints;
none of the other endpoints should deal with the HPD signal and the
direct of_graph-link between the usb-c-connector and the DP controller
mimics that of e.g. dp-connector very nicely, both in description and
implementation.

Regards,
Bjorn

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Rob Herring]

On Wed, Jan 25, 2023 at 03:40:13PM -0800, Bjorn Andersson wrote:
> On Tue, Jan 24, 2023 at 08:31:13PM -0600, Rob Herring wrote:
> > On Tue, Jan 24, 2023 at 11:04 AM Bjorn Andersson
> > <quic_bjorande@quicinc.com> wrote:
> > >
> > > On Tue, Jan 24, 2023 at 10:00:12AM -0600, Rob Herring wrote:
> > > > On Thu, Jan 19, 2023 at 11:40 AM Bjorn Andersson
> > > > <quic_bjorande@quicinc.com> wrote:
> > > > >
> > > > > On Thu, Jan 19, 2023 at 10:11:32AM -0600, Rob Herring wrote:
> > > > > > On Wed, Jan 18, 2023 at 10:08:11AM -0800, Bjorn Andersson wrote:
> > > > > > > On Tue, Jan 17, 2023 at 11:56:57AM -0600, Rob Herring wrote:
> > > > > > > > On Thu, Jan 12, 2023 at 08:11:14PM -0800, Bjorn Andersson wrote:
> > > > > > > > > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > > > > >
> > > > > > > > > Introduce a binding for GPIO-based mux hardware used for connecting,
> > > > > > > > > disconnecting and switching orientation of the SBU lines in USB Type-C
> > > > > > > > > applications.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > > > > > > > ---
> > > > > >
> > > > > >
> > > > > > > > > +    tcpm {
> > > > > > > > > +        connector {
> > > > > > > > > +            compatible = "usb-c-connector";
> > > > > > > > > +
> > > > > > > > > +            ports {
> > > > > > > > > +                #address-cells = <1>;
> > > > > > > > > +                #size-cells = <0>;
> > > > > > > > > +
> > > > > > > > > +                port@0 {
> > > > > > > > > +                    reg = <0>;
> > > > > > > > > +                    tcpm_hs_out: endpoint {
> > > > > > > > > +                        remote-endpoint = <&usb_hs_phy_in>;
> > > > > > > > > +                    };
> > > > > > > > > +                };
> > > > > > > > > +
> > > > > > > > > +                port@1 {
> > > > > > > > > +                    reg = <1>;
> > > > > > > > > +                    tcpm_ss_out: endpoint {
> > > > > > > > > +                        remote-endpoint = <&usb_ss_phy_in>;
> > > > > > > > > +                    };
> > > > > > > > > +                };
> > > > > > > > > +
> > > > > > > > > +                port@2 {
> > > > > > > > > +                    reg = <2>;
> > > > > > > > > +                    tcpm_sbu_out: endpoint {
> > > > > > > > > +                        remote-endpoint = <&sbu_mux_in>;
> > > > > > > > > +                    };
> > > > > > > > > +                };
> > > > > > > > > +            };
> > > > > > > > > +        };
> > > > > > > > > +    };
> > > > > > > > > +
> > > > > > > > > +    sbu-mux {
> > > > > > > > > +        compatible = "pericom,pi3usb102", "gpio-sbu-mux";
> > > > > > > > > +
> > > > > > > > > +        enable-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
> > > > > > > > > +        select-gpios = <&tlmm 164 GPIO_ACTIVE_HIGH>;
> > > > > > > > > +
> > > > > > > > > +        mode-switch;
> > > > > > > > > +        orientation-switch;
> > > > > > > > > +
> > > > > > > > > +        port {
> > > > > > > > > +            sbu_mux_in: endpoint {
> > > > > > > > > +                remote-endpoint = <&tcpm_sbu_out>;
> > > > > > > > > +            };
> > > > > > > >
> > > > > > > > Don't you need a connection to whatever drives SBU? Maybe your case is
> > > > > > > > fixed because the phy does the DP/USB muxing? But the binding needs to
> > > > > > > > support the worst case which I guess would be all the muxing/switching
> > > > > > > > is done by separate board level components.
> > > > > > > >
> > > > > > >
> > > > > > > Perhaps I'm misunderstanding your request, but I think this is the worst
> > > > > > > case you're talking about.
> > > > > > >
> > > > > > > &usb_ss_phy_in is a reference to the PHY, which does switching/muxing of
> > > > > > > the SuperSpeed lanes in the connector, but the PHY provides no control
> > > > > > > over the SBU signals.
> > > > > > >
> > > > > > > So this sbu-mux is a separate component between the SBU-pads on the SoC
> > > > > > > and the usb-c-connector, referenced through he &sbu_mux_in reference.
> > > > > > >
> > > > > > >
> > > > > > > So upon e.g. a orientation switch, the typec_switch_set() call the tcpm
> > > > > > > implementation will request orientation switching from port@1 and port@2
> > > > > > > (no orientation-switch on port@0/HS pins).
> > > > > >
> > > > > > 'port@2' is supposed to define the connection to what controls SBU. The
> > > > > > mux here switches the signals, but it doesn't control them.
> > > > >
> > > > > The SBU signals are driven by the SS PHY, on behalf of the DisplayPort
> > > > > controller. These signals are  turned on/off as a result of the TCPM
> > > > > indicating the HPD state to the DisplayPort controller.
> > > > >
> > > > > There's a such not really a direct representation today of the entity
> > > > > that drives the SBU lines. It happens to be a sub-block in
> > > > > &usb_ss_phy_in, but I don't envision that we need/want any signaling
> > > > > between the TCPM and the SBU-"driver".
> > > > >
> > > > >
> > > > > I see that I missed that in the example above, your suggestion on how to
> > > > > model that relationship (TCPM - DP controller) was to add an additional
> > > > > endpoint in port@1. So that's the current design (but neither ports nor
> > > > > endpoints are significant from an implementation point of view).
> > > > >
> > > > > > The mux should sit in the middle, but the graph terminates at the mux.
> > > > > > You don't have a connection presumably because you know what the
> > > > > > connection.
> > > > >
> > > > > But do you suggest that the graph should reference the entity that
> > > > > drives the SBU signals?
> > > >
> > > > Yes, that was the original intent.
> > > >
> > >
> > > Directly from the connector, or just indirectly?
> > >
> > > > > What about the discrete mux?
> > > >
> > > > You mean the mux in this binding, right? That should be in the middle:
> > > >
> > > > DPaux --> SBUmux --> connector
> > > >
> > > > Maybe the SS phy is in there too.
> > > >
> > >
> > > The signal originally comes from the DP controller, the analog
> > > electronics lives in the SS phy, then the signal goes to the SBU mux and
> > > finally to the connector.
> > >
> > > > >
> > > > > > Perhaps because there is only 1 connector and controller.
> > > > > >
> > > > >
> > > > > There is one SBU mux, one DP controller and one SS PHY per
> > > > > usb-c-connector.
> > > > >
> > > > > > Suppose you have 2 connectors and 2 controllers which drive SBU
> > > > > > signals. Also assume that the SBU signals are completely independent
> > > > > > from what's driving the altmode SS signals. How would you describe that?
> > > > > >
> > > > >
> > > > > This is the setup we have on e.g. SC8280XP CRD; where the TCPM has two
> > > > > usb-c-connectors defined, each with their graph referencing the SS PHY,
> > > > > DP controller and respective sbu-mux.
> > > > >
> > > > > There's an incomplete example of this published at [1] (where the SS phy
> > > > > isn't represented yet - and hence there's no control over the SS lanes,
> > > > > nor is the HS lanes connected to the dwc3 for role switching).
> > > > >
> > > > > Perhaps I'm misunderstanding your concerns though?
> > > >
> > > > That looks like you can assume who drives SBU based on the DP
> > > > controller. Probably a safe assumption for DP (that DP-aux is part of
> > > > the DP controller), but I was more worried about if you can't assume
> > > > that relationship. Take HDMI for example where the DDC signals can
> > > > come from anywhere. They could be part of the HDMI bridge, a general
> > > > purpose I2C bus off the SoC, or bitbanged GPIOs. Though from what I've
> > > > read, HDMI Altmode is dead. I don't know if the need to describe the
> > > > SBU connection would apply to anything else.
> > > >
> > > > I guess this all boils down to whether the SBU mux should have a 2nd
> > > > optional port as the input for what drives it.
> > > >
> > >
> > > Are you saying that the connector should link with the mux and then the
> > > source of the signal should be daisy chained? Or that we should just
> > > link both of them as two separate endpoints from the connector?
> > 
> > The former. The data path of the signal in h/w should match in the DT
> > graph. The caveat being we don't normally show PHYs in that mix
> > because they are somewhat transparent. That's maybe becoming less true
> > with routing or muxing included in PHYs.
> > 
> 
> We have discussed and prototyped this a few times now in the Qualcomm
> community, and I am not a fan of having to add forwarding-logic to each
> implementation of a Type-C mux/switch, which in some configuration might
> have an entity behind it that needs the notifications.

I don't know if we can really escape that.


> But I don't think there's a concern for this binding (in my
> implementation), there's currently no mode/orientation switching
> happening beyond this entity.
> 
> 
> 
> That said, if we're to represent each signal path to the connector,
> I would like to bring up this problem again:
> https://lore.kernel.org/all/20220520164810.141400-1-bjorn.andersson@linaro.org/
> 
> port@0 represent the HS signals going to the connector, port@1 the SS
> signals going to the connector, port@2 the SBU signals going to the
> connector.
> 
> But I need some representation of the HPD (hot plug detection) "signal"
> (there is no actual signal path in hardware, this is a virtual
> notification) going _from_ the connector to the DisplayPort controller.

I would assume whatever entity is deciding to enable the DP signals on 
the connector would be what generates the HPD notification. I think you 
want to describe the DP signal connections and muxing, but HPD itself 
wouldn't be in the DT.

> We discussed this (perhaps in person, as there's no trace on lkml?) and
> you suggested that I use a second endpoint under port@1, instead of
> introducing a fourth port.

Multiple endpoints on a port are typically a mux or fanout depending on 
the data direction. But the muxing is not really in the connector, so 
that should probably be represented somewhere else. I think a new port 
suffers from the same issue. Maybe that's close enough? Depends if there 
are cases of more alt modes or more complicated muxing/switching.

> I'm fine either way, but I don't think it would be convenient nor
> representable to daisy chain this behind any of the existing endpoints;
> none of the other endpoints should deal with the HPD signal and the
> direct of_graph-link between the usb-c-connector and the DP controller
> mimics that of e.g. dp-connector very nicely, both in description and
> implementation.

That would be nice, but the reality is there's a lot more between DP and 
the connector with USB-C and the graph should reflect that. I guess the 
problem there is being able to walk the graph. Suppose we have:

DP out port -> altmode mux in port -> altmode mux out port -> type-c 
port 1

The issue walking the graph here would be generic code doesn't know 
altmode mux port numbering as that's not a generic thing (could be 
perhaps?). Maybe you can walk from each end and see if you end up in 
the same device.

Of course, I haven't even considered the split cases where it's not 
just either USB3 OR DP, but combinations. 


What I'd really like to see for all this USB-C stuff is block diagrams 
of the h/w components and then what the corresponding DT looks like. 
Trying to extend things one piece at a time is hard to review and not 
likely going to end with a flexible design.

Rob

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Bjorn Andersson]

On Mon, Jan 30, 2023 at 10:48:13AM -0600, Rob Herring wrote:
> On Wed, Jan 25, 2023 at 03:40:13PM -0800, Bjorn Andersson wrote:
> > On Tue, Jan 24, 2023 at 08:31:13PM -0600, Rob Herring wrote:
> > > On Tue, Jan 24, 2023 at 11:04 AM Bjorn Andersson
> > > <quic_bjorande@quicinc.com> wrote:
> > > >
> > > > On Tue, Jan 24, 2023 at 10:00:12AM -0600, Rob Herring wrote:
> > > > > On Thu, Jan 19, 2023 at 11:40 AM Bjorn Andersson
> > > > > <quic_bjorande@quicinc.com> wrote:
[..]
> > > > Are you saying that the connector should link with the mux and then the
> > > > source of the signal should be daisy chained? Or that we should just
> > > > link both of them as two separate endpoints from the connector?
> > > 
> > > The former. The data path of the signal in h/w should match in the DT
> > > graph. The caveat being we don't normally show PHYs in that mix
> > > because they are somewhat transparent. That's maybe becoming less true
> > > with routing or muxing included in PHYs.
> > > 
> > 
> > We have discussed and prototyped this a few times now in the Qualcomm
> > community, and I am not a fan of having to add forwarding-logic to each
> > implementation of a Type-C mux/switch, which in some configuration might
> > have an entity behind it that needs the notifications.
> 
> I don't know if we can really escape that.
> 

Okay, we'll have to figure out how to implement that when such need come...

> 
> > But I don't think there's a concern for this binding (in my
> > implementation), there's currently no mode/orientation switching
> > happening beyond this entity.
> > 
> > 
> > 
> > That said, if we're to represent each signal path to the connector,
> > I would like to bring up this problem again:
> > https://lore.kernel.org/all/20220520164810.141400-1-bjorn.andersson@linaro.org/
> > 
> > port@0 represent the HS signals going to the connector, port@1 the SS
> > signals going to the connector, port@2 the SBU signals going to the
> > connector.
> > 
> > But I need some representation of the HPD (hot plug detection) "signal"
> > (there is no actual signal path in hardware, this is a virtual
> > notification) going _from_ the connector to the DisplayPort controller.
> 
> I would assume whatever entity is deciding to enable the DP signals on 
> the connector would be what generates the HPD notification.

The HPD notification comes from the display/connector, and is
conceptually equivalent to hpd-gpios in e.g. the dp-connector binding.

> I think you want to describe the DP signal connections and muxing, but
> HPD itself wouldn't be in the DT.
> 

Okay, so you're saying that the display driver needs to traverse the
graph representing the display-signal path, in hope to find someone
generating a HPD notification?

> > We discussed this (perhaps in person, as there's no trace on lkml?) and
> > you suggested that I use a second endpoint under port@1, instead of
> > introducing a fourth port.
> 
> Multiple endpoints on a port are typically a mux or fanout depending on 
> the data direction. But the muxing is not really in the connector, so 
> that should probably be represented somewhere else. I think a new port 
> suffers from the same issue. Maybe that's close enough? Depends if there 
> are cases of more alt modes or more complicated muxing/switching.
> 
> > I'm fine either way, but I don't think it would be convenient nor
> > representable to daisy chain this behind any of the existing endpoints;
> > none of the other endpoints should deal with the HPD signal and the
> > direct of_graph-link between the usb-c-connector and the DP controller
> > mimics that of e.g. dp-connector very nicely, both in description and
> > implementation.
> 
> That would be nice, but the reality is there's a lot more between DP and 
> the connector with USB-C and the graph should reflect that.

Not when it comes to delivering the HPD notification, afaict.

The TCPM will configure muxes & switches to ensure that the USB
connector is wired up according to what has been decided over USB PD.

The HPD signal is orthogonal to that configuration, and should not be
picked up by any of the other components.

> I guess the 
> problem there is being able to walk the graph. Suppose we have:
> 
> DP out port -> altmode mux in port -> altmode mux out port -> type-c 
> port 1
> 
> The issue walking the graph here would be generic code doesn't know 
> altmode mux port numbering as that's not a generic thing (could be 
> perhaps?). Maybe you can walk from each end and see if you end up in 
> the same device.
> 
> Of course, I haven't even considered the split cases where it's not 
> just either USB3 OR DP, but combinations. 
> 

The implementation that toggles between the DP-only and USB/DP-combo is
not stable, so we currently only support USB/DP-combo upstream.

Again, the TCPM will handle the muxing and orientation switching in the
PHY and sbu-mux, then once a datapath capable of delivering DP-altmode
data is established, it might send HPD notifications - to the display
driver.

> 
> What I'd really like to see for all this USB-C stuff is block diagrams 
> of the h/w components and then what the corresponding DT looks like. 
> Trying to extend things one piece at a time is hard to review and not 
> likely going to end with a flexible design.
> 

This is the design we have in a range of different boards:

                     +------------- - -
 USB connector       | SoC
 +-+                 |   +--------+    +-------+
 | |                 |   |        |    |       |
 |*|<------- HS -----|-->| HS phy |<-->| (EUD) |<--+
 | |                 |   |        |    |       |   |   +--------+
 | |                 |   +--------+    +-------+   +-->|        |
 | |                 |                                 |  dwc3  |
 | |                 |   +--------+        /---------->|        |
 | |   +----------+  |   |        |<------/            +--------+
 |*|<--|(redriver)|<-|-->| SS phy |
 | |   +----------+  |   |        |<-\   +------------+
 | |                 |   +--------+   \->|            |
 | |                 |                   |     DP     |
 | |     +-----+     |                   | controller |
 |*|<--->| SBU |<----|------------------>|            |
 | |     | mux |     |                   |            |
 | |     +----+      |                   +------------+
 +-+                 |
                     +------------- - -

The EUD and redriver are only found/used in some designs.  My proposed
representation of this (without those) is:

/soc {
    usb-controller {
        dwc3 {
            port {
                dwc0-out: endpoint {
                    remote-endpoint = <&connector0_hs>;
            };
        };
    };

    ss_phy: phy {
        port {
            ss_phy_out: endpoint {
                remote-endpoint = <&connector0_ss>;
            };
        };
    };

    display-subsystem {
        displayport-controller {
            phys = <&ss_phy>;
            ports {
                port@1 {
                    reg = <1>;
                    dp0_out: endpoint {
                        remote-endpoint = <&connector0_hpd>;
                    };
                };
            };
        };
    };
};

usb0-sbu-mux {
    compatible = "gpio-sbu-mux";

    port {
        sbu0_out: endpoint {
            remote-endpoint = <&connector_sbu>;
        };
    };
};

tcpm {
    connector@0 {
        compatible = "usb-c-connector";
        reg = <0>;
        ports {
            port@0 {
                reg = <0>;
                connector0_hs: endpoint {
                    remote-endpoint = <&dwc0_out>;
                };
            };

            port@1 {
                reg = <1>;
                connector0_ss: endpoint@0 {
                    remote-endpoint = <&ss_phy_out>;
                };
                connector0_hpd: endpoint@1 {
                    remote-endpoint = <&dp0_out>;
                };
            };

            port@2 {
                reg = <2>;
                    connector_sbu: endpoint {
                        remote-endpoint = <&sbu0_out>;
                };
            };
        };
    };
};

The EUD needs to be able to override the role-switch state, so the design that
was accepted was to implement the role-switch forwarding logic in the driver
and daisy chain the of-graph.

No redriver has made it to LKML, but the this is where the daisy chain vs
reference all instances from port@1 comes in.

The Type-C port manager (tcpm) might be handling multiple usb-c-connectors, in
which case the reg of the connector identifies which instance is being
described.


So I think that all these pieces fits your model, except the port@1/endpoint@1
and the fact that displayport-controller has a of_graph.


In particular we have a number of these:

/dp-connector {
    compatible = "dp-connector";

    port {
        connector: endpoint {
            remote-endpoint = <&dp_out>;
        };
    };
};

/soc {
    display-subsystem {
        displayport-controller {
            phys = <&some_dp_phy>;
            ports {
                port@1 {
                    reg = <1>;
                    dp_out: endpoint {
                        remote-endpoint = <&connector>;
                    };
                };
            };
        };
    };
};

As you said previously, it doesn't make sense to represent the phy in this
graph. We just define the output of the controller as port@1 and link that to
the connector.

So what is the output of the dp controller in the USB case - if we're not
representing that as the HPD link directly between the connector and the
controller?

Note that it's not a matter of traversing the graph, because we don't use a
graph for the connection between the controller and the PHY.

Regards,
Bjorn

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Rob Herring]

On Mon, Jan 30, 2023 at 01:42:14PM -0800, Bjorn Andersson wrote:
> On Mon, Jan 30, 2023 at 10:48:13AM -0600, Rob Herring wrote:
> > On Wed, Jan 25, 2023 at 03:40:13PM -0800, Bjorn Andersson wrote:
> > > On Tue, Jan 24, 2023 at 08:31:13PM -0600, Rob Herring wrote:
> > > > On Tue, Jan 24, 2023 at 11:04 AM Bjorn Andersson
> > > > <quic_bjorande@quicinc.com> wrote:
> > > > >
> > > > > On Tue, Jan 24, 2023 at 10:00:12AM -0600, Rob Herring wrote:
> > > > > > On Thu, Jan 19, 2023 at 11:40 AM Bjorn Andersson
> > > > > > <quic_bjorande@quicinc.com> wrote:
> [..]
> > > > > Are you saying that the connector should link with the mux and then the
> > > > > source of the signal should be daisy chained? Or that we should just
> > > > > link both of them as two separate endpoints from the connector?
> > > > 
> > > > The former. The data path of the signal in h/w should match in the DT
> > > > graph. The caveat being we don't normally show PHYs in that mix
> > > > because they are somewhat transparent. That's maybe becoming less true
> > > > with routing or muxing included in PHYs.
> > > > 
> > > 
> > > We have discussed and prototyped this a few times now in the Qualcomm
> > > community, and I am not a fan of having to add forwarding-logic to each
> > > implementation of a Type-C mux/switch, which in some configuration might
> > > have an entity behind it that needs the notifications.
> > 
> > I don't know if we can really escape that.
> > 
> 
> Okay, we'll have to figure out how to implement that when such need come...
> 
> > 
> > > But I don't think there's a concern for this binding (in my
> > > implementation), there's currently no mode/orientation switching
> > > happening beyond this entity.
> > > 
> > > 
> > > 
> > > That said, if we're to represent each signal path to the connector,
> > > I would like to bring up this problem again:
> > > https://lore.kernel.org/all/20220520164810.141400-1-bjorn.andersson@linaro.org/
> > > 
> > > port@0 represent the HS signals going to the connector, port@1 the SS
> > > signals going to the connector, port@2 the SBU signals going to the
> > > connector.
> > > 
> > > But I need some representation of the HPD (hot plug detection) "signal"
> > > (there is no actual signal path in hardware, this is a virtual
> > > notification) going _from_ the connector to the DisplayPort controller.
> > 
> > I would assume whatever entity is deciding to enable the DP signals on 
> > the connector would be what generates the HPD notification.
> 
> The HPD notification comes from the display/connector, and is
> conceptually equivalent to hpd-gpios in e.g. the dp-connector binding.

Except that it is software based rather than a h/w signal (ignoring the 
s/w generating a h/w HPD signal for you).

> 
> > I think you want to describe the DP signal connections and muxing, but
> > HPD itself wouldn't be in the DT.
> > 
> 
> Okay, so you're saying that the display driver needs to traverse the
> graph representing the display-signal path, in hope to find someone
> generating a HPD notification?

After further discussion, I think it is still the immediate neighbor, it 
is just that the immediate neighbor is some other component, not the 
type-c connector. 

> > > We discussed this (perhaps in person, as there's no trace on lkml?) and
> > > you suggested that I use a second endpoint under port@1, instead of
> > > introducing a fourth port.
> > 
> > Multiple endpoints on a port are typically a mux or fanout depending on 
> > the data direction. But the muxing is not really in the connector, so 
> > that should probably be represented somewhere else. I think a new port 
> > suffers from the same issue. Maybe that's close enough? Depends if there 
> > are cases of more alt modes or more complicated muxing/switching.
> > 
> > > I'm fine either way, but I don't think it would be convenient nor
> > > representable to daisy chain this behind any of the existing endpoints;
> > > none of the other endpoints should deal with the HPD signal and the
> > > direct of_graph-link between the usb-c-connector and the DP controller
> > > mimics that of e.g. dp-connector very nicely, both in description and
> > > implementation.
> > 
> > That would be nice, but the reality is there's a lot more between DP and 
> > the connector with USB-C and the graph should reflect that.
> 
> Not when it comes to delivering the HPD notification, afaict.
> 
> The TCPM will configure muxes & switches to ensure that the USB
> connector is wired up according to what has been decided over USB PD.
> 
> The HPD signal is orthogonal to that configuration, and should not be
> picked up by any of the other components.

Agreed as HPD is not a h/w signal routed between components.


> > I guess the 
> > problem there is being able to walk the graph. Suppose we have:
> > 
> > DP out port -> altmode mux in port -> altmode mux out port -> type-c 
> > port 1
> > 
> > The issue walking the graph here would be generic code doesn't know 
> > altmode mux port numbering as that's not a generic thing (could be 
> > perhaps?). Maybe you can walk from each end and see if you end up in 
> > the same device.
> > 
> > Of course, I haven't even considered the split cases where it's not 
> > just either USB3 OR DP, but combinations. 
> > 
> 
> The implementation that toggles between the DP-only and USB/DP-combo is
> not stable, so we currently only support USB/DP-combo upstream.

Okay, but I don't care about that from a binding standpoint. All 
possibilities need to be considered whether your h/w can support it or 
not.
 
> Again, the TCPM will handle the muxing and orientation switching in the
> PHY and sbu-mux, then once a datapath capable of delivering DP-altmode
> data is established, it might send HPD notifications - to the display
> driver.
> 
> > 
> > What I'd really like to see for all this USB-C stuff is block diagrams 
> > of the h/w components and then what the corresponding DT looks like. 
> > Trying to extend things one piece at a time is hard to review and not 
> > likely going to end with a flexible design.
> > 
> 
> This is the design we have in a range of different boards:

*This* is what I need for every Type-C binding.

> 
>                      +------------- - -
>  USB connector       | SoC
>  +-+                 |   +--------+    +-------+
>  | |                 |   |        |    |       |
>  |*|<------- HS -----|-->| HS phy |<-->| (EUD) |<--+
>  | |                 |   |        |    |       |   |   +--------+
>  | |                 |   +--------+    +-------+   +-->|        |
>  | |                 |                                 |  dwc3  |
>  | |                 |   +--------+        /---------->|        |
>  | |   +----------+  |   |        |<------/            +--------+
>  |*|<--|(redriver)|<-|-->| SS phy |
>  | |   +----------+  |   |        |<-\   +------------+
>  | |                 |   +--------+   \->|            |
>  | |                 |                   |     DP     |
>  | |     +-----+     |                   | controller |
>  |*|<--->| SBU |<----|------------------>|            |
>  | |     | mux |     |                   |            |
>  | |     +----+      |                   +------------+
>  +-+                 |
>                      +------------- - -

Where's the TCPM?


> The EUD and redriver are only found/used in some designs.  My proposed
> representation of this (without those) is:

I'd assume a redriver is mostly transparent to s/w?


> 
> /soc {
>     usb-controller {
>         dwc3 {
>             port {
>                 dwc0-out: endpoint {
>                     remote-endpoint = <&connector0_hs>;
>             };
>         };
>     };
> 
>     ss_phy: phy {
>         port {
>             ss_phy_out: endpoint {
>                 remote-endpoint = <&connector0_ss>;
>             };
>         };
>     };
> 
>     display-subsystem {
>         displayport-controller {
>             phys = <&ss_phy>;
>             ports {
>                 port@1 {
>                     reg = <1>;
>                     dp0_out: endpoint {
>                         remote-endpoint = <&connector0_hpd>;
>                     };
>                 };
>             };
>         };
>     };
> };
> 
> usb0-sbu-mux {
>     compatible = "gpio-sbu-mux";
> 
>     port {
>         sbu0_out: endpoint {
>             remote-endpoint = <&connector_sbu>;
>         };
>     };
> };
> 
> tcpm {
>     connector@0 {
>         compatible = "usb-c-connector";
>         reg = <0>;
>         ports {
>             port@0 {
>                 reg = <0>;
>                 connector0_hs: endpoint {
>                     remote-endpoint = <&dwc0_out>;
>                 };
>             };
> 
>             port@1 {
>                 reg = <1>;
>                 connector0_ss: endpoint@0 {
>                     remote-endpoint = <&ss_phy_out>;
>                 };
>                 connector0_hpd: endpoint@1 {
>                     remote-endpoint = <&dp0_out>;
>                 };

This just looks wrong to me because one connection is the output of the 
phy's mux and one is the input. The USB SS connection is implicit, but I 
think it should be explicit from dwc3 to ss_phy. It would need an output 
port and 2 input ports. I want to say we already have bindings doing 
this.

>             };
> 
>             port@2 {
>                 reg = <2>;
>                     connector_sbu: endpoint {
>                         remote-endpoint = <&sbu0_out>;
>                 };
>             };
>         };
>     };
> };
> 
> The EUD needs to be able to override the role-switch state, so the design that
> was accepted was to implement the role-switch forwarding logic in the driver
> and daisy chain the of-graph.

Given EUD is a Qualcomm only thing, I'm not all that worried about it.

> 
> No redriver has made it to LKML, but the this is where the daisy chain vs
> reference all instances from port@1 comes in.
> 
> The Type-C port manager (tcpm) might be handling multiple usb-c-connectors, in
> which case the reg of the connector identifies which instance is being
> described.
> 
> 
> So I think that all these pieces fits your model, except the port@1/endpoint@1
> and the fact that displayport-controller has a of_graph.
> 
> 
> In particular we have a number of these:
> 
> /dp-connector {
>     compatible = "dp-connector";
> 
>     port {
>         connector: endpoint {
>             remote-endpoint = <&dp_out>;
>         };
>     };
> };
> 
> /soc {
>     display-subsystem {
>         displayport-controller {
>             phys = <&some_dp_phy>;
>             ports {
>                 port@1 {
>                     reg = <1>;
>                     dp_out: endpoint {
>                         remote-endpoint = <&connector>;
>                     };
>                 };
>             };
>         };
>     };
> };
> 
> As you said previously, it doesn't make sense to represent the phy in this
> graph. We just define the output of the controller as port@1 and link that to
> the connector.

What I said (or meant) was we don't represent phys which are just 
providing the electrical interface. Your 'phy' is also a mux/switch, so 
it does make sense to represent it in the graph.

> 
> So what is the output of the dp controller in the USB case - if we're not
> representing that as the HPD link directly between the connector and the
> controller?

The answer lies in your block diagram... 

The question I think is whether we could standardize the mux/switch 
graph ports. Say 'port@0' is the output to type-C connector port@1, 
and port@[1-9] are altmode connections to USB/DP/TB. If we did that, 
then generic code can walk the graph from a controller to the connector. 
We only need to know that port@0 goes to the connector. However, that 
assumes there is only 1 entity in the middle and I don't know if that 
holds true.

Rob

Re: [PATCH v2 1/2] dt-bindings: usb: Introduce GPIO-based SBU mux

[Bjorn Andersson]

On Tue, Jan 31, 2023 at 01:44:05PM -0600, Rob Herring wrote:
> On Mon, Jan 30, 2023 at 01:42:14PM -0800, Bjorn Andersson wrote:
> > On Mon, Jan 30, 2023 at 10:48:13AM -0600, Rob Herring wrote:
> > > On Wed, Jan 25, 2023 at 03:40:13PM -0800, Bjorn Andersson wrote:
> > > > On Tue, Jan 24, 2023 at 08:31:13PM -0600, Rob Herring wrote:
> > > > > On Tue, Jan 24, 2023 at 11:04 AM Bjorn Andersson
> > > > > <quic_bjorande@quicinc.com> wrote:
[..]
> > This is the design we have in a range of different boards:
> 
> *This* is what I need for every Type-C binding.
> 

Glad you like it.

> > 
> >                      +------------- - -
> >  USB connector       | SoC
> >  +-+                 |   +--------+    +-------+
> >  | |                 |   |        |    |       |
> >  |*|<------- HS -----|-->| HS phy |<-->| (EUD) |<--+
> >  | |                 |   |        |    |       |   |   +--------+
> >  | |                 |   +--------+    +-------+   +-->|        |
> >  | |                 |                                 |  dwc3  |
> >  | |                 |   +--------+        /---------->|        |
> >  | |   +----------+  |   |        |<------/            +--------+
> >  |*|<--|(redriver)|<-|-->| SS phy |
> >  | |   +----------+  |   |        |<-\   +------------+
> >  | |                 |   +--------+   \->|            |
> >  | |                 |                   |     DP     |
> >  | |     +-----+     |                   | controller |
> >  |*|<--->| SBU |<----|------------------>|            |
> >  | |     | mux |     |                   |            |
> >  | |     +----+      |                   +------------+
> >  +-+                 |
> >                      +------------- - -
> 
> Where's the TCPM?
> 

The TCPM here becomes the implementation behind one more more
USB connectors.

> 
> > The EUD and redriver are only found/used in some designs.  My proposed
> > representation of this (without those) is:
> 
> I'd assume a redriver is mostly transparent to s/w?
> 

There are both cases. But per our discussion (summarized below), each
entity in the graph should represent the actual signal path.  So in the
case where it need to be represented in the signal path, the
implementation would have to deal with it being the port@1
remote-endpoint.

> 
> > 
> > /soc {
> >     usb-controller {
> >         dwc3 {
> >             port {
> >                 dwc0-out: endpoint {
> >                     remote-endpoint = <&connector0_hs>;
> >             };
> >         };
> >     };
> > 
> >     ss_phy: phy {
> >         port {
> >             ss_phy_out: endpoint {
> >                 remote-endpoint = <&connector0_ss>;
> >             };
> >         };
> >     };
> > 
> >     display-subsystem {
> >         displayport-controller {
> >             phys = <&ss_phy>;
> >             ports {
> >                 port@1 {
> >                     reg = <1>;
> >                     dp0_out: endpoint {
> >                         remote-endpoint = <&connector0_hpd>;
> >                     };
> >                 };
> >             };
> >         };
> >     };
> > };
> > 
> > usb0-sbu-mux {
> >     compatible = "gpio-sbu-mux";
> > 
> >     port {
> >         sbu0_out: endpoint {
> >             remote-endpoint = <&connector_sbu>;
> >         };
> >     };
> > };
> > 
> > tcpm {
> >     connector@0 {
> >         compatible = "usb-c-connector";
> >         reg = <0>;
> >         ports {
> >             port@0 {
> >                 reg = <0>;
> >                 connector0_hs: endpoint {
> >                     remote-endpoint = <&dwc0_out>;
> >                 };
> >             };
> > 
> >             port@1 {
> >                 reg = <1>;
> >                 connector0_ss: endpoint@0 {
> >                     remote-endpoint = <&ss_phy_out>;
> >                 };
> >                 connector0_hpd: endpoint@1 {
> >                     remote-endpoint = <&dp0_out>;
> >                 };
> 
> This just looks wrong to me because one connection is the output of the 
> phy's mux and one is the input. The USB SS connection is implicit, but I 
> think it should be explicit from dwc3 to ss_phy. It would need an output 
> port and 2 input ports. I want to say we already have bindings doing 
> this.
> 

Right, endpoint@0 represents the actual signal path, while endpoint@1
represents the display signal source or HPD destination.

It does look weird, but that's what we agreed upon in a previous
iteration.

> >             };
> > 
> >             port@2 {
> >                 reg = <2>;
> >                     connector_sbu: endpoint {
> >                         remote-endpoint = <&sbu0_out>;
> >                 };
> >             };
> >         };
> >     };
> > };
> > 
[..]
> > 
> > /dp-connector {
> >     compatible = "dp-connector";
> > 
> >     port {
> >         connector: endpoint {
> >             remote-endpoint = <&dp_out>;
> >         };
> >     };
> > };
> > 
> > /soc {
> >     display-subsystem {
> >         displayport-controller {
> >             phys = <&some_dp_phy>;
> >             ports {
> >                 port@1 {
> >                     reg = <1>;
> >                     dp_out: endpoint {
> >                         remote-endpoint = <&connector>;
> >                     };
> >                 };
> >             };
> >         };
> >     };
> > };
> > 
> > As you said previously, it doesn't make sense to represent the phy in this
> > graph. We just define the output of the controller as port@1 and link that to
> > the connector.
> 
> What I said (or meant) was we don't represent phys which are just 
> providing the electrical interface. Your 'phy' is also a mux/switch, so 
> it does make sense to represent it in the graph.
> 

Attempting to summarize our lengthy discussion on IRC.

The output port of the display block represents the signal path.

In the even that the associated phy is merely a dumb D/A converter, the
next logical entity on that path is the connector, such as the
dp-connector example above.

If, on the other hand, the phy, or any other component, on the signal path
is doing more than just electrical conversion, it should be represented
in the DT and linked using the of-graph.

As such, in the case where the phy is involved in e.g. orientation
switching, the output (port@1 in the Qualcomm DP binding) of the
display block should be tied to the phy, and then the phy should be
connected to the next entity on the path (e.g. the usb-c-connector).

In both cases, the phys property can be seen as representing the
"control interface", and the graph is used to represent the signal path.

> > 
> > So what is the output of the dp controller in the USB case - if we're not
> > representing that as the HPD link directly between the connector and the
> > controller?
> 
> The answer lies in your block diagram... 
> 

So, each (active) component in the diagram should be represented in the
Devicetree and the links between them should be represented by
of-graphs.

> The question I think is whether we could standardize the mux/switch 
> graph ports. Say 'port@0' is the output to type-C connector port@1, 
> and port@[1-9] are altmode connections to USB/DP/TB. If we did that, 
> then generic code can walk the graph from a controller to the connector. 
> We only need to know that port@0 goes to the connector.

In the display bindings today, we use port@0 for in and port@1 for
out, but it doesn't some universal.

> However, that assumes there is only 1 entity in the middle and I don't
> know if that holds true.
> 

We've seen examples of redrivers or the ChromeOS switch, where this
doesn't hold. In the latter we have two outputs (one being active at a
time)...

Regards,
Bjorn