[PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer

[PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer

[Abel Vesa]

The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
via I2C. It provides altmode and orientation handling and usually sits
between the Type-C port and the PHY.

It is currently used alongside Qualcomm Snapdragon X Elite SoCs on quite
a few laptops already.

This new driver adds support for the following 3 modes:
 - DP 4lanes (pin assignments C and E)
 - DP 2lanes + USB3 (pin assignment D)
 - USB3

This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
it can support link training from source to itself. This means that the
DP driver needs to be aware of the repeater presence and to handle
the link training accordingly. This is currently missing from msm dp
driver, but there is already effort going on to add it. Once done,
full external DP will be working on all X1E laptops that make use of
this retimer.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
Changes in v2:
- Addressed all comments from Johan and Konrad.
- Reworked the handling of the vregs so it would be more cleaner.
  Dropped the usage of bulk regulators API and handled them separately.
  Also discribed all regulators according to data sheet.
- Added all delays according to data sheet.
- Fixed coldplug (on boot) orientation detection.
- Didn't pick Krzysztof's R-b tag because the bindings changed w.r.t
  supplies.
- Link to v1: https://lore.kernel.org/r/20240829-x1e80100-ps8830-v1-0-bcc4790b1d45@linaro.org

---
Abel Vesa (2):
      dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings
      usb: typec: Add support for Parade PS8830 Type-C Retimer

 .../devicetree/bindings/usb/parade,ps8830.yaml     | 129 +++++++
 drivers/usb/typec/mux/Kconfig                      |  10 +
 drivers/usb/typec/mux/Makefile                     |   1 +
 drivers/usb/typec/mux/ps8830.c                     | 424 +++++++++++++++++++++
 4 files changed, 564 insertions(+)
---
base-commit: c02d24a5af66a9806922391493205a344749f2c4
change-id: 20240521-x1e80100-ps8830-d5ccca95b557

Best regards,
-- 
Abel Vesa <abel.vesa@linaro.org>

[PATCH v2 1/2] dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings

[Abel Vesa]

Document bindings for the Parade PS8830 Type-C retimer. This retimer is
currently found on all boards featuring Qualcomm Snapdragon X Elite SoCs
and it is needed to provide altmode muxing between DP and USB, but also
connector orientation handling between.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 .../devicetree/bindings/usb/parade,ps8830.yaml     | 129 +++++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/parade,ps8830.yaml b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..f6721d6eee26c6d4590a12c19791b3d47a8145f3
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
@@ -0,0 +1,129 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/parade,ps8830.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Parade PS8830 USB and DisplayPort Retimer
+
+maintainers:
+  - Abel Vesa <abel.vesa@linaro.org>
+
+properties:
+  compatible:
+    enum:
+      - parade,ps8830
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: XO Clock
+
+  clock-names:
+    items:
+      - const: xo
+
+  reset-gpios:
+    maxItems: 1
+
+  vdd-supply:
+    description: power supply (1.07V)
+
+  vdd33-supply:
+    description: power supply (3.3V)
+
+  vdd33-cap-supply:
+    description: power supply (3.3V)
+
+  vddar-supply:
+    description: power supply (1.07V)
+
+  vddat-supply:
+    description: power supply (1.07V)
+
+  vddio-supply:
+    description: power supply (1.2V or 1.8V)
+
+  orientation-switch: true
+  retimer-switch: true
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Super Speed (SS) Output endpoint to the Type-C connector
+
+      port@1:
+        $ref: /schemas/graph.yaml#/$defs/port-base
+        description: Super Speed (SS) Input endpoint from the Super-Speed PHY
+        unevaluatedProperties: false
+
+      port@2:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          Sideband Use (SBU) AUX lines endpoint to the Type-C connector for the purpose of
+          handling altmode muxing and orientation switching.
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: usb-switch.yaml#
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        typec-mux@8 {
+            compatible = "parade,ps8830";
+            reg = <0x8>;
+
+            vdd-supply = <&vreg_rtmr_1p15>;
+            vdd33-supply = <&vreg_rtmr_3p3>;
+            vdd33-cap-supply = <&vreg_rtmr_3p3>;
+            vddar-supply = <&vreg_rtmr_1p15>;
+            vddat-supply = <&vreg_rtmr_1p15>;
+            vddio-supply = <&vreg_rtmr_1p8>;
+
+            reset-gpios = <&pm8550_gpios 10 GPIO_ACTIVE_HIGH>;
+
+            retimer-switch;
+            orientation-switch;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    usb_con_ss: endpoint {
+                        remote-endpoint = <&typec_con_ss>;
+                    };
+                };
+                port@1 {
+                    reg = <1>;
+                    phy_con_ss: endpoint {
+                        remote-endpoint = <&usb_phy_ss>;
+                        data-lanes = <3 2 1 0>;
+                    };
+                };
+                port@2 {
+                    reg = <2>;
+                    usb_con_sbu: endpoint {
+                        remote-endpoint = <&typec_dp_aux>;
+                    };
+                };
+            };
+        };
+    };
+...

-- 
2.34.1

[PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Abel Vesa]

The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
It provides both altmode and orientation handling.

Add a driver with support for the following modes:
 - DP 4lanes
 - DP 2lanes + USB3
 - USB3

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/usb/typec/mux/Kconfig  |  10 +
 drivers/usb/typec/mux/Makefile |   1 +
 drivers/usb/typec/mux/ps8830.c | 424 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 435 insertions(+)

diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
index ce7db6ad30572a0a74890f5f11944fb3ff07f635..48613b67f1c5dacd14d54baf91c3066377cf97be 100644
--- a/drivers/usb/typec/mux/Kconfig
+++ b/drivers/usb/typec/mux/Kconfig
@@ -56,6 +56,16 @@ config TYPEC_MUX_NB7VPQ904M
 	  Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
 	  redriver chip found on some devices with a Type-C port.
 
+config TYPEC_MUX_PS8830
+	tristate "Parade PS8830 Type-C retimer driver"
+	depends on I2C
+	depends on DRM || DRM=n
+	select DRM_AUX_BRIDGE if DRM_BRIDGE && OF
+	select REGMAP_I2C
+	help
+	  Say Y or M if your system has a Parade PS8830 Type-C retimer chip
+	  found on some devices with a Type-C port.
+
 config TYPEC_MUX_PTN36502
 	tristate "NXP PTN36502 Type-C redriver driver"
 	depends on I2C
diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
index bb96f30267af05b33b9277dcf1cc0e1527d2dcdd..4b23b12cfe45a0ff8a37f38c7ba050f572d556e7 100644
--- a/drivers/usb/typec/mux/Makefile
+++ b/drivers/usb/typec/mux/Makefile
@@ -6,5 +6,6 @@ obj-$(CONFIG_TYPEC_MUX_PI3USB30532)	+= pi3usb30532.o
 obj-$(CONFIG_TYPEC_MUX_INTEL_PMC)	+= intel_pmc_mux.o
 obj-$(CONFIG_TYPEC_MUX_IT5205)		+= it5205.o
 obj-$(CONFIG_TYPEC_MUX_NB7VPQ904M)	+= nb7vpq904m.o
+obj-$(CONFIG_TYPEC_MUX_PS8830)		+= ps8830.o
 obj-$(CONFIG_TYPEC_MUX_PTN36502)	+= ptn36502.o
 obj-$(CONFIG_TYPEC_MUX_WCD939X_USBSS)	+= wcd939x-usbss.o
diff --git a/drivers/usb/typec/mux/ps8830.c b/drivers/usb/typec/mux/ps8830.c
new file mode 100644
index 0000000000000000000000000000000000000000..58990f7860bf77ec12d00f0d3df3a40735bbf9d8
--- /dev/null
+++ b/drivers/usb/typec/mux/ps8830.c
@@ -0,0 +1,424 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Parade PS8830 usb retimer driver
+ *
+ * Copyright (C) 2024 Linaro Ltd.
+ */
+
+#include <drm/bridge/aux-bridge.h>
+#include <linux/clk.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/usb/typec_altmode.h>
+#include <linux/usb/typec_dp.h>
+#include <linux/usb/typec_mux.h>
+#include <linux/usb/typec_retimer.h>
+
+struct ps8830_retimer {
+	struct i2c_client *client;
+	struct gpio_desc *reset_gpio;
+	struct regmap *regmap;
+	struct typec_switch_dev *sw;
+	struct typec_retimer *retimer;
+	struct clk *xo_clk;
+	struct regulator *vdd_supply;
+	struct regulator *vdd33_supply;
+	struct regulator *vdd33_cap_supply;
+	struct regulator *vddat_supply;
+	struct regulator *vddar_supply;
+	struct regulator *vddio_supply;
+
+	struct typec_switch *typec_switch;
+	struct typec_mux *typec_mux;
+
+	struct mutex lock; /* protect non-concurrent retimer & switch */
+
+	enum typec_orientation orientation;
+	unsigned long mode;
+	int cfg[3];
+};
+
+static void ps8830_write(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2)
+{
+	if (cfg0 == retimer->cfg[0] &&
+	    cfg1 == retimer->cfg[1] &&
+	    cfg2 == retimer->cfg[2])
+		return;
+
+	retimer->cfg[0] = cfg0;
+	retimer->cfg[1] = cfg1;
+	retimer->cfg[2] = cfg2;
+
+	regmap_write(retimer->regmap, 0x0, cfg0);
+	regmap_write(retimer->regmap, 0x1, cfg1);
+	regmap_write(retimer->regmap, 0x2, cfg2);
+}
+
+static void ps8830_configure(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2)
+{
+	/* Write safe-mode config before switching to new config */
+	ps8830_write(retimer, 0x1, 0x0, 0x0);
+
+	ps8830_write(retimer, cfg0, cfg1, cfg2);
+}
+
+static int ps8380_set(struct ps8830_retimer *retimer)
+{
+	int cfg0 = 0x00;
+	int cfg1 = 0x00;
+	int cfg2 = 0x00;
+
+	if (retimer->orientation == TYPEC_ORIENTATION_NONE ||
+	    retimer->mode == TYPEC_STATE_SAFE) {
+		ps8830_write(retimer, 0x1, 0x0, 0x0);
+		return 0;
+	}
+
+	if (retimer->orientation == TYPEC_ORIENTATION_NORMAL)
+		cfg0 = 0x01;
+	else
+		cfg0 = 0x03;
+
+	switch (retimer->mode) {
+	/* USB3 Only */
+	case TYPEC_STATE_USB:
+		cfg0 |= 0x20;
+		break;
+
+	/* DP Only */
+	case TYPEC_DP_STATE_C:
+		cfg1 = 0x85;
+		break;
+
+	case TYPEC_DP_STATE_E:
+		cfg1 = 0x81;
+		break;
+
+	/* DP + USB */
+	case TYPEC_DP_STATE_D:
+		cfg0 |= 0x20;
+		cfg1 = 0x85;
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	ps8830_configure(retimer, cfg0, cfg1, cfg2);
+
+	return 0;
+}
+
+static int ps8830_sw_set(struct typec_switch_dev *sw,
+			 enum typec_orientation orientation)
+{
+	struct ps8830_retimer *retimer = typec_switch_get_drvdata(sw);
+	int ret = 0;
+
+	ret = typec_switch_set(retimer->typec_switch, orientation);
+	if (ret)
+		return ret;
+
+	mutex_lock(&retimer->lock);
+
+	if (retimer->orientation != orientation) {
+		retimer->orientation = orientation;
+
+		ret = ps8380_set(retimer);
+	}
+
+	mutex_unlock(&retimer->lock);
+
+	return ret;
+}
+
+static int ps8830_retimer_set(struct typec_retimer *rtmr,
+			      struct typec_retimer_state *state)
+{
+	struct ps8830_retimer *retimer = typec_retimer_get_drvdata(rtmr);
+	struct typec_mux_state mux_state;
+	int ret = 0;
+
+	mutex_lock(&retimer->lock);
+
+	if (state->mode != retimer->mode) {
+		retimer->mode = state->mode;
+
+		ret = ps8380_set(retimer);
+	}
+
+	mutex_unlock(&retimer->lock);
+
+	if (ret)
+		return ret;
+
+	mux_state.alt = state->alt;
+	mux_state.data = state->data;
+	mux_state.mode = state->mode;
+
+	return typec_mux_set(retimer->typec_mux, &mux_state);
+}
+
+static int ps8830_enable_vregs(struct ps8830_retimer *retimer)
+{
+	struct device *dev = &retimer->client->dev;
+	int ret;
+
+	ret = regulator_enable(retimer->vdd33_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD 3.3V regulator: %d\n", ret);
+		return ret;
+	}
+
+	ret = regulator_enable(retimer->vdd33_cap_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD 3.3V CAP regulator: %d\n", ret);
+		goto err_vdd33_disable;
+	}
+
+	usleep_range(4000, 10000);
+
+	ret = regulator_enable(retimer->vdd_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD regulator: %d\n", ret);
+		goto err_vdd33_cap_disable;
+	}
+
+	ret = regulator_enable(retimer->vddar_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD AR regulator: %d\n", ret);
+		goto err_vdd_disable;
+	}
+
+	ret = regulator_enable(retimer->vddat_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD AT regulator: %d\n", ret);
+		goto err_vddar_disable;
+	}
+
+	ret = regulator_enable(retimer->vddio_supply);
+	if (ret) {
+		dev_err(dev, "cannot enable VDD IO regulator: %d\n", ret);
+		goto err_vddat_disable;
+	}
+
+	return 0;
+
+err_vddat_disable:
+	regulator_disable(retimer->vddat_supply);
+
+err_vddar_disable:
+	regulator_disable(retimer->vddar_supply);
+
+err_vdd_disable:
+	regulator_disable(retimer->vdd_supply);
+
+err_vdd33_cap_disable:
+	regulator_disable(retimer->vdd33_cap_supply);
+
+err_vdd33_disable:
+	regulator_disable(retimer->vdd33_supply);
+
+	return ret;
+}
+
+static int ps8830_get_vregs(struct ps8830_retimer *retimer)
+{
+	struct device *dev = &retimer->client->dev;
+
+	retimer->vdd_supply = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(retimer->vdd_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vdd_supply),
+				     "failed to get VDD\n");
+
+	retimer->vdd33_supply = devm_regulator_get(dev, "vdd33");
+	if (IS_ERR(retimer->vdd33_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vdd33_supply),
+				     "failed to get VDD 3.3V\n");
+
+	retimer->vdd33_cap_supply = devm_regulator_get(dev, "vdd33-cap");
+	if (IS_ERR(retimer->vdd33_cap_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vdd33_cap_supply),
+				     "failed to get VDD CAP 3.3V\n");
+
+	retimer->vddat_supply = devm_regulator_get(dev, "vddat");
+	if (IS_ERR(retimer->vddat_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vddat_supply),
+				     "failed to get VDD AT\n");
+
+	retimer->vddar_supply = devm_regulator_get(dev, "vddar");
+	if (IS_ERR(retimer->vddar_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vddar_supply),
+				     "failed to get VDD AR\n");
+
+	retimer->vddio_supply = devm_regulator_get(dev, "vddio");
+	if (IS_ERR(retimer->vddio_supply))
+		return dev_err_probe(dev, PTR_ERR(retimer->vddio_supply),
+				     "failed to get VDD IO\n");
+
+	return 0;
+}
+
+static const struct regmap_config ps8830_retimer_regmap = {
+	.max_register = 0x1f,
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int ps8830_retimer_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct typec_switch_desc sw_desc = { };
+	struct typec_retimer_desc rtmr_desc = { };
+	struct ps8830_retimer *retimer;
+	int ret;
+
+	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
+	if (!retimer)
+		return -ENOMEM;
+
+	retimer->client = client;
+
+	mutex_init(&retimer->lock);
+
+	retimer->regmap = devm_regmap_init_i2c(client, &ps8830_retimer_regmap);
+	if (IS_ERR(retimer->regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(retimer->regmap);
+	}
+
+	ret = ps8830_get_vregs(retimer);
+	if (ret)
+		return ret;
+
+	retimer->xo_clk = devm_clk_get(dev, "xo");
+	if (IS_ERR(retimer->xo_clk))
+		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
+				     "failed to get xo clock\n");
+
+	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(retimer->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(retimer->reset_gpio),
+				     "failed to get reset gpio\n");
+
+	retimer->typec_switch = fwnode_typec_switch_get(dev->fwnode);
+	if (IS_ERR(retimer->typec_switch)) {
+		dev_err(dev, "failed to acquire orientation-switch\n");
+		return PTR_ERR(retimer->typec_switch);
+	}
+
+	retimer->typec_mux = fwnode_typec_mux_get(dev->fwnode);
+	if (IS_ERR(retimer->typec_mux)) {
+		dev_err(dev, "failed to acquire mode-mux\n");
+		goto err_switch_put;
+	}
+
+	sw_desc.drvdata = retimer;
+	sw_desc.fwnode = dev_fwnode(dev);
+	sw_desc.set = ps8830_sw_set;
+
+	ret = drm_aux_bridge_register(dev);
+	if (ret)
+		goto err_mux_put;
+
+	retimer->sw = typec_switch_register(dev, &sw_desc);
+	if (IS_ERR(retimer->sw)) {
+		dev_err(dev, "failed to register typec switch\n");
+		goto err_aux_bridge_unregister;
+	}
+
+	rtmr_desc.drvdata = retimer;
+	rtmr_desc.fwnode = dev_fwnode(dev);
+	rtmr_desc.set = ps8830_retimer_set;
+
+	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
+	if (IS_ERR(retimer->retimer)) {
+		dev_err(dev, "failed to register typec retimer\n");
+		goto err_switch_unregister;
+	}
+
+	ret = clk_prepare_enable(retimer->xo_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable XO: %d\n", ret);
+		goto err_retimer_unregister;
+	}
+
+	ret = ps8830_enable_vregs(retimer);
+	if (ret)
+		goto err_clk_disable;
+
+	/* delay needed as per datasheet */
+	usleep_range(4000, 14000);
+
+	gpiod_set_value(retimer->reset_gpio, 1);
+
+	return 0;
+
+err_clk_disable:
+	clk_disable_unprepare(retimer->xo_clk);
+
+err_retimer_unregister:
+	typec_retimer_unregister(retimer->retimer);
+
+err_switch_unregister:
+	typec_switch_unregister(retimer->sw);
+
+err_aux_bridge_unregister:
+	gpiod_set_value(retimer->reset_gpio, 0);
+	clk_disable_unprepare(retimer->xo_clk);
+
+err_mux_put:
+	typec_mux_put(retimer->typec_mux);
+
+err_switch_put:
+	typec_switch_put(retimer->typec_switch);
+
+	return ret;
+}
+
+static void ps8830_retimer_remove(struct i2c_client *client)
+{
+	struct ps8830_retimer *retimer = i2c_get_clientdata(client);
+
+	typec_retimer_unregister(retimer->retimer);
+	typec_switch_unregister(retimer->sw);
+
+	gpiod_set_value(retimer->reset_gpio, 0);
+
+	regulator_disable(retimer->vddio_supply);
+	regulator_disable(retimer->vddat_supply);
+	regulator_disable(retimer->vddar_supply);
+	regulator_disable(retimer->vdd_supply);
+	regulator_disable(retimer->vdd33_cap_supply);
+	regulator_disable(retimer->vdd33_supply);
+
+	clk_disable_unprepare(retimer->xo_clk);
+
+	typec_mux_put(retimer->typec_mux);
+	typec_switch_put(retimer->typec_switch);
+}
+
+static const struct of_device_id ps8830_retimer_of_table[] = {
+	{ .compatible = "parade,ps8830" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ps8830_retimer_of_table);
+
+static struct i2c_driver ps8830_retimer_driver = {
+	.driver = {
+		.name = "ps8830_retimer",
+		.of_match_table = ps8830_retimer_of_table,
+	},
+	.probe		= ps8830_retimer_probe,
+	.remove		= ps8830_retimer_remove,
+};
+
+module_i2c_driver(ps8830_retimer_driver);
+
+MODULE_DESCRIPTION("Parade PS8830 Type-C Retimer driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH v2 1/2] dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings

[Rob Herring]

On Fri, Oct 04, 2024 at 04:57:37PM +0300, Abel Vesa wrote:
> Document bindings for the Parade PS8830 Type-C retimer. This retimer is
> currently found on all boards featuring Qualcomm Snapdragon X Elite SoCs
> and it is needed to provide altmode muxing between DP and USB, but also
> connector orientation handling between.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  .../devicetree/bindings/usb/parade,ps8830.yaml     | 129 +++++++++++++++++++++
>  1 file changed, 129 insertions(+)

Missing R-by from Krzysztof?

Re: [PATCH v2 1/2] dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings

[Dmitry Baryshkov]

On Sat, Oct 05, 2024 at 12:36:47PM GMT, Rob Herring wrote:
> On Fri, Oct 04, 2024 at 04:57:37PM +0300, Abel Vesa wrote:
> > Document bindings for the Parade PS8830 Type-C retimer. This retimer is
> > currently found on all boards featuring Qualcomm Snapdragon X Elite SoCs
> > and it is needed to provide altmode muxing between DP and USB, but also
> > connector orientation handling between.
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> >  .../devicetree/bindings/usb/parade,ps8830.yaml     | 129 +++++++++++++++++++++
> >  1 file changed, 129 insertions(+)
> 
> Missing R-by from Krzysztof?

Quoting the changelog:

Didn't pick Krzysztof's R-b tag because the bindings changed w.r.t
supplies

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings

[Dmitry Baryshkov]

On Fri, Oct 04, 2024 at 04:57:37PM GMT, Abel Vesa wrote:
> Document bindings for the Parade PS8830 Type-C retimer. This retimer is
> currently found on all boards featuring Qualcomm Snapdragon X Elite SoCs
> and it is needed to provide altmode muxing between DP and USB, but also
> connector orientation handling between.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  .../devicetree/bindings/usb/parade,ps8830.yaml     | 129 +++++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/parade,ps8830.yaml b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..f6721d6eee26c6d4590a12c19791b3d47a8145f3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
> @@ -0,0 +1,129 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/parade,ps8830.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Parade PS8830 USB and DisplayPort Retimer
> +
> +maintainers:
> +  - Abel Vesa <abel.vesa@linaro.org>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - parade,ps8830
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: XO Clock
> +
> +  clock-names:
> +    items:
> +      - const: xo
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: power supply (1.07V)
> +
> +  vdd33-supply:
> +    description: power supply (3.3V)
> +
> +  vdd33-cap-supply:
> +    description: power supply (3.3V)
> +
> +  vddar-supply:
> +    description: power supply (1.07V)
> +
> +  vddat-supply:
> +    description: power supply (1.07V)

Any additional details?

> +
> +  vddio-supply:
> +    description: power supply (1.2V or 1.8V)
> +
> +  orientation-switch: true
> +  retimer-switch: true
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Super Speed (SS) Output endpoint to the Type-C connector
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        description: Super Speed (SS) Input endpoint from the Super-Speed PHY

or from another SS signal source, which can be a mux, a switch or
anything else. I'd say, just 'Input Super Speed (SS)'

> +        unevaluatedProperties: false
> +
> +      port@2:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          Sideband Use (SBU) AUX lines endpoint to the Type-C connector for the purpose of
> +          handling altmode muxing and orientation switching.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +allOf:
> +  - $ref: usb-switch.yaml#
> +
> +additionalProperties: false
> +

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings

[Dmitry Baryshkov]

On Fri, Oct 04, 2024 at 04:57:37PM GMT, Abel Vesa wrote:
> Document bindings for the Parade PS8830 Type-C retimer. This retimer is
> currently found on all boards featuring Qualcomm Snapdragon X Elite SoCs
> and it is needed to provide altmode muxing between DP and USB, but also
> connector orientation handling between.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  .../devicetree/bindings/usb/parade,ps8830.yaml     | 129 +++++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/parade,ps8830.yaml b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..f6721d6eee26c6d4590a12c19791b3d47a8145f3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
> @@ -0,0 +1,129 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/parade,ps8830.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Parade PS8830 USB and DisplayPort Retimer
> +
> +maintainers:
> +  - Abel Vesa <abel.vesa@linaro.org>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - parade,ps8830
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: XO Clock
> +
> +  clock-names:
> +    items:
> +      - const: xo
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: power supply (1.07V)
> +
> +  vdd33-supply:
> +    description: power supply (3.3V)
> +
> +  vdd33-cap-supply:
> +    description: power supply (3.3V)
> +
> +  vddar-supply:
> +    description: power supply (1.07V)
> +
> +  vddat-supply:
> +    description: power supply (1.07V)
> +
> +  vddio-supply:
> +    description: power supply (1.2V or 1.8V)
> +
> +  orientation-switch: true
> +  retimer-switch: true
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Super Speed (SS) Output endpoint to the Type-C connector
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/$defs/port-base
> +        description: Super Speed (SS) Input endpoint from the Super-Speed PHY
> +        unevaluatedProperties: false

BTW: could you please also use usb/usb-switch.yaml for these ports ?
Maybe we should also add port@2 to that schema.

> +
> +      port@2:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description:
> +          Sideband Use (SBU) AUX lines endpoint to the Type-C connector for the purpose of
> +          handling altmode muxing and orientation switching.

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Dmitry Baryshkov]

On Fri, Oct 04, 2024 at 04:57:38PM GMT, Abel Vesa wrote:
> The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
> It provides both altmode and orientation handling.
> 
> Add a driver with support for the following modes:
>  - DP 4lanes
>  - DP 2lanes + USB3
>  - USB3
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
>  drivers/usb/typec/mux/Kconfig  |  10 +
>  drivers/usb/typec/mux/Makefile |   1 +
>  drivers/usb/typec/mux/ps8830.c | 424 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 435 insertions(+)
> 
> diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
> index ce7db6ad30572a0a74890f5f11944fb3ff07f635..48613b67f1c5dacd14d54baf91c3066377cf97be 100644
> --- a/drivers/usb/typec/mux/Kconfig
> +++ b/drivers/usb/typec/mux/Kconfig
> @@ -56,6 +56,16 @@ config TYPEC_MUX_NB7VPQ904M
>  	  Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
>  	  redriver chip found on some devices with a Type-C port.
>  
> +config TYPEC_MUX_PS8830
> +	tristate "Parade PS8830 Type-C retimer driver"
> +	depends on I2C
> +	depends on DRM || DRM=n
> +	select DRM_AUX_BRIDGE if DRM_BRIDGE && OF
> +	select REGMAP_I2C
> +	help
> +	  Say Y or M if your system has a Parade PS8830 Type-C retimer chip
> +	  found on some devices with a Type-C port.
> +
>  config TYPEC_MUX_PTN36502
>  	tristate "NXP PTN36502 Type-C redriver driver"
>  	depends on I2C
> diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
> index bb96f30267af05b33b9277dcf1cc0e1527d2dcdd..4b23b12cfe45a0ff8a37f38c7ba050f572d556e7 100644
> --- a/drivers/usb/typec/mux/Makefile
> +++ b/drivers/usb/typec/mux/Makefile
> @@ -6,5 +6,6 @@ obj-$(CONFIG_TYPEC_MUX_PI3USB30532)	+= pi3usb30532.o
>  obj-$(CONFIG_TYPEC_MUX_INTEL_PMC)	+= intel_pmc_mux.o
>  obj-$(CONFIG_TYPEC_MUX_IT5205)		+= it5205.o
>  obj-$(CONFIG_TYPEC_MUX_NB7VPQ904M)	+= nb7vpq904m.o
> +obj-$(CONFIG_TYPEC_MUX_PS8830)		+= ps8830.o
>  obj-$(CONFIG_TYPEC_MUX_PTN36502)	+= ptn36502.o
>  obj-$(CONFIG_TYPEC_MUX_WCD939X_USBSS)	+= wcd939x-usbss.o
> diff --git a/drivers/usb/typec/mux/ps8830.c b/drivers/usb/typec/mux/ps8830.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..58990f7860bf77ec12d00f0d3df3a40735bbf9d8
> --- /dev/null
> +++ b/drivers/usb/typec/mux/ps8830.c
> @@ -0,0 +1,424 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Parade PS8830 usb retimer driver
> + *
> + * Copyright (C) 2024 Linaro Ltd.
> + */
> +
> +#include <drm/bridge/aux-bridge.h>
> +#include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/usb/typec_altmode.h>
> +#include <linux/usb/typec_dp.h>
> +#include <linux/usb/typec_mux.h>
> +#include <linux/usb/typec_retimer.h>
> +
> +struct ps8830_retimer {
> +	struct i2c_client *client;
> +	struct gpio_desc *reset_gpio;
> +	struct regmap *regmap;
> +	struct typec_switch_dev *sw;
> +	struct typec_retimer *retimer;
> +	struct clk *xo_clk;
> +	struct regulator *vdd_supply;
> +	struct regulator *vdd33_supply;
> +	struct regulator *vdd33_cap_supply;
> +	struct regulator *vddat_supply;
> +	struct regulator *vddar_supply;
> +	struct regulator *vddio_supply;
> +
> +	struct typec_switch *typec_switch;
> +	struct typec_mux *typec_mux;
> +
> +	struct mutex lock; /* protect non-concurrent retimer & switch */
> +
> +	enum typec_orientation orientation;
> +	unsigned long mode;
> +	int cfg[3];
> +};
> +
> +static void ps8830_write(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2)
> +{
> +	if (cfg0 == retimer->cfg[0] &&
> +	    cfg1 == retimer->cfg[1] &&
> +	    cfg2 == retimer->cfg[2])
> +		return;
> +
> +	retimer->cfg[0] = cfg0;
> +	retimer->cfg[1] = cfg1;
> +	retimer->cfg[2] = cfg2;
> +
> +	regmap_write(retimer->regmap, 0x0, cfg0);
> +	regmap_write(retimer->regmap, 0x1, cfg1);
> +	regmap_write(retimer->regmap, 0x2, cfg2);
> +}

This looks like a reimplementation of regcache. Is it really necessary?

> +
> +static void ps8830_configure(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2)
> +{
> +	/* Write safe-mode config before switching to new config */

Why is this required?

> +	ps8830_write(retimer, 0x1, 0x0, 0x0);
> +
> +	ps8830_write(retimer, cfg0, cfg1, cfg2);
> +}
> +
> +static int ps8380_set(struct ps8830_retimer *retimer)
> +{
> +	int cfg0 = 0x00;
> +	int cfg1 = 0x00;
> +	int cfg2 = 0x00;
> +
> +	if (retimer->orientation == TYPEC_ORIENTATION_NONE ||
> +	    retimer->mode == TYPEC_STATE_SAFE) {
> +		ps8830_write(retimer, 0x1, 0x0, 0x0);
> +		return 0;
> +	}
> +
> +	if (retimer->orientation == TYPEC_ORIENTATION_NORMAL)
> +		cfg0 = 0x01;
> +	else
> +		cfg0 = 0x03;
> +
> +	switch (retimer->mode) {
> +	/* USB3 Only */
> +	case TYPEC_STATE_USB:
> +		cfg0 |= 0x20;
> +		break;
> +

The TYPEC_DP clauses should only be used if state->alt->swid is set to
USB_TYPEC_DP_SID. Other altmodes share id space for their states.

> +	/* DP Only */
> +	case TYPEC_DP_STATE_C:
> +		cfg1 = 0x85;
> +		break;
> +
> +	case TYPEC_DP_STATE_E:
> +		cfg1 = 0x81;
> +		break;
> +
> +	/* DP + USB */
> +	case TYPEC_DP_STATE_D:
> +		cfg0 |= 0x20;
> +		cfg1 = 0x85;
> +		break;

CDE, please.

> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ps8830_configure(retimer, cfg0, cfg1, cfg2);
> +
> +	return 0;
> +}
> +

-- 
With best wishes
Dmitry

Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer

[Johan Hovold]

On Fri, Oct 04, 2024 at 04:57:36PM +0300, Abel Vesa wrote:
> The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
> via I2C. It provides altmode and orientation handling and usually sits
> between the Type-C port and the PHY.
> 
> It is currently used alongside Qualcomm Snapdragon X Elite SoCs on quite
> a few laptops already.
> 
> This new driver adds support for the following 3 modes:
>  - DP 4lanes (pin assignments C and E)
>  - DP 2lanes + USB3 (pin assignment D)
>  - USB3
> 
> This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
> it can support link training from source to itself. This means that the
> DP driver needs to be aware of the repeater presence and to handle
> the link training accordingly. This is currently missing from msm dp
> driver, but there is already effort going on to add it. Once done,
> full external DP will be working on all X1E laptops that make use of
> this retimer.

I was gonna ask you to include the devicetree changes that enables the
retimers as part of this series (to facilitate review and testing), but
perhaps you should indeed not post them again until LTTPR support is in
place.

> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
> Changes in v2:
> - Addressed all comments from Johan and Konrad.
> - Reworked the handling of the vregs so it would be more cleaner.
>   Dropped the usage of bulk regulators API and handled them separately.
>   Also discribed all regulators according to data sheet.
> - Added all delays according to data sheet.
> - Fixed coldplug (on boot) orientation detection.

Coldplug orientation detection still does not work here with this series
applied.

I'm not entirely sure this whether worked better with v1, but with v2
my SuperSpeed ethernet device shows up as a HighSpeed device in one
orientation. It is also not disconnected an re-enumerated as SS as is
the case on the X13s (and possibly with v1):

	usb 1-1: new high-speed USB device number 2 using xhci-hcd

> - Didn't pick Krzysztof's R-b tag because the bindings changed w.r.t
>   supplies.
> - Link to v1: https://lore.kernel.org/r/20240829-x1e80100-ps8830-v1-0-bcc4790b1d45@linaro.org
> 
> ---
> Abel Vesa (2):
>       dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings
>       usb: typec: Add support for Parade PS8830 Type-C Retimer

Johan

Re: [PATCH v2 1/2] dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings

[Johan Hovold]

On Fri, Oct 04, 2024 at 04:57:37PM +0300, Abel Vesa wrote:
> Document bindings for the Parade PS8830 Type-C retimer. This retimer is
> currently found on all boards featuring Qualcomm Snapdragon X Elite SoCs
> and it is needed to provide altmode muxing between DP and USB, but also
> connector orientation handling between.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>

> +  clocks:
> +    items:
> +      - description: XO Clock
> +
> +  clock-names:
> +    items:
> +      - const: xo

> +  vdd-supply:
> +    description: power supply (1.07V)
> +
> +  vdd33-supply:
> +    description: power supply (3.3V)
> +
> +  vdd33-cap-supply:
> +    description: power supply (3.3V)
> +
> +  vddar-supply:
> +    description: power supply (1.07V)
> +
> +  vddat-supply:
> +    description: power supply (1.07V)
> +
> +  vddio-supply:
> +    description: power supply (1.2V or 1.8V)

> +required:
> +  - compatible
> +  - reg

Presumably all of the supplies are also required.

Similar for clocks, etc.

> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        typec-mux@8 {
> +            compatible = "parade,ps8830";
> +            reg = <0x8>;
> +
> +            vdd-supply = <&vreg_rtmr_1p15>;
> +            vdd33-supply = <&vreg_rtmr_3p3>;
> +            vdd33-cap-supply = <&vreg_rtmr_3p3>;
> +            vddar-supply = <&vreg_rtmr_1p15>;
> +            vddat-supply = <&vreg_rtmr_1p15>;
> +            vddio-supply = <&vreg_rtmr_1p8>;
> +
> +            reset-gpios = <&pm8550_gpios 10 GPIO_ACTIVE_HIGH>;

The reset line is active low.

> +
> +            retimer-switch;
> +            orientation-switch;
> +
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                port@0 {
> +                    reg = <0>;

Please add a newline before each child node for readability.

> +                    usb_con_ss: endpoint {

We typically avoid adding unused labels to the examples, but perhaps
here it serves as documentation?

> +                        remote-endpoint = <&typec_con_ss>;
> +                    };
> +                };

Add newline here too, and similar below.

> +                port@1 {
> +                    reg = <1>;
> +                    phy_con_ss: endpoint {
> +                        remote-endpoint = <&usb_phy_ss>;
> +                        data-lanes = <3 2 1 0>;
> +                    };
> +                };
> +                port@2 {
> +                    reg = <2>;
> +                    usb_con_sbu: endpoint {
> +                        remote-endpoint = <&typec_dp_aux>;
> +                    };
> +                };
> +            };
> +        };
> +    };
> +...

Johan

Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Johan Hovold]

On Fri, Oct 04, 2024 at 04:57:38PM +0300, Abel Vesa wrote:

> +static int ps8830_enable_vregs(struct ps8830_retimer *retimer)
> +{

> +	return 0;
> +
> +err_vddat_disable:
> +	regulator_disable(retimer->vddat_supply);
> +

Nit: I'd drop the empty lines between the errors cases here.

> +err_vddar_disable:
> +	regulator_disable(retimer->vddar_supply);
> +
> +err_vdd_disable:
> +	regulator_disable(retimer->vdd_supply);
> +
> +err_vdd33_cap_disable:
> +	regulator_disable(retimer->vdd33_cap_supply);
> +
> +err_vdd33_disable:
> +	regulator_disable(retimer->vdd33_supply);
> +
> +	return ret;
> +}

> +static int ps8830_retimer_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct typec_switch_desc sw_desc = { };
> +	struct typec_retimer_desc rtmr_desc = { };
> +	struct ps8830_retimer *retimer;
> +	int ret;
> +
> +	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
> +	if (!retimer)
> +		return -ENOMEM;
> +
> +	retimer->client = client;
> +
> +	mutex_init(&retimer->lock);
> +
> +	retimer->regmap = devm_regmap_init_i2c(client, &ps8830_retimer_regmap);
> +	if (IS_ERR(retimer->regmap)) {
> +		dev_err(dev, "failed to allocate register map\n");

Please make sure to log the errno as well here and below.

> +		return PTR_ERR(retimer->regmap);
> +	}
> +
> +	ret = ps8830_get_vregs(retimer);
> +	if (ret)
> +		return ret;
> +
> +	retimer->xo_clk = devm_clk_get(dev, "xo");
> +	if (IS_ERR(retimer->xo_clk))
> +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> +				     "failed to get xo clock\n");
> +
> +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);

The reset line is active low and should be described as such in DT. So
here you want to request it as logically low if you want to deassert
reset.

Is there now timing requirements on when you deassert reset after
enabling the supplies?

> +	if (IS_ERR(retimer->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(retimer->reset_gpio),
> +				     "failed to get reset gpio\n");
> +
> +	retimer->typec_switch = fwnode_typec_switch_get(dev->fwnode);
> +	if (IS_ERR(retimer->typec_switch)) {
> +		dev_err(dev, "failed to acquire orientation-switch\n");

I saw the driver fail here once, but not sure what the errno was since
it was not printed. Presumably it was a probe deferral and then this
should be a dev_err_probe() as well:

	ps8830_retimer 2-0008: failed to acquire orientation-switch

> +		return PTR_ERR(retimer->typec_switch);
> +	}
> +
> +	retimer->typec_mux = fwnode_typec_mux_get(dev->fwnode);
> +	if (IS_ERR(retimer->typec_mux)) {
> +		dev_err(dev, "failed to acquire mode-mux\n");

Similar here perhaps?

> +		goto err_switch_put;
> +	}
> +
> +	sw_desc.drvdata = retimer;
> +	sw_desc.fwnode = dev_fwnode(dev);
> +	sw_desc.set = ps8830_sw_set;
> +
> +	ret = drm_aux_bridge_register(dev);
> +	if (ret)
> +		goto err_mux_put;
> +
> +	retimer->sw = typec_switch_register(dev, &sw_desc);
> +	if (IS_ERR(retimer->sw)) {
> +		dev_err(dev, "failed to register typec switch\n");
> +		goto err_aux_bridge_unregister;
> +	}
> +
> +	rtmr_desc.drvdata = retimer;
> +	rtmr_desc.fwnode = dev_fwnode(dev);
> +	rtmr_desc.set = ps8830_retimer_set;
> +
> +	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
> +	if (IS_ERR(retimer->retimer)) {
> +		dev_err(dev, "failed to register typec retimer\n");
> +		goto err_switch_unregister;
> +	}
> +
> +	ret = clk_prepare_enable(retimer->xo_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable XO: %d\n", ret);
> +		goto err_retimer_unregister;
> +	}

Should you really enable the clock before the regulators?

> +
> +	ret = ps8830_enable_vregs(retimer);
> +	if (ret)
> +		goto err_clk_disable;
> +
> +	/* delay needed as per datasheet */
> +	usleep_range(4000, 14000);
> +
> +	gpiod_set_value(retimer->reset_gpio, 1);

Here you only deassert reset in case the line is incorrectly described
as active high in DT.

> +	return 0;
> +
> +err_clk_disable:
> +	clk_disable_unprepare(retimer->xo_clk);
> +
> +err_retimer_unregister:
> +	typec_retimer_unregister(retimer->retimer);
> +
> +err_switch_unregister:
> +	typec_switch_unregister(retimer->sw);
> +
> +err_aux_bridge_unregister:
> +	gpiod_set_value(retimer->reset_gpio, 0);
> +	clk_disable_unprepare(retimer->xo_clk);
> +
> +err_mux_put:
> +	typec_mux_put(retimer->typec_mux);
> +
> +err_switch_put:
> +	typec_switch_put(retimer->typec_switch);

Drop newlines before labels?

> +
> +	return ret;
> +}

Johan

Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer

[Abel Vesa]

On 24-10-15 14:41:25, Johan Hovold wrote:
> On Fri, Oct 04, 2024 at 04:57:36PM +0300, Abel Vesa wrote:
> > The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
> > via I2C. It provides altmode and orientation handling and usually sits
> > between the Type-C port and the PHY.
> > 
> > It is currently used alongside Qualcomm Snapdragon X Elite SoCs on quite
> > a few laptops already.
> > 
> > This new driver adds support for the following 3 modes:
> >  - DP 4lanes (pin assignments C and E)
> >  - DP 2lanes + USB3 (pin assignment D)
> >  - USB3
> > 
> > This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
> > it can support link training from source to itself. This means that the
> > DP driver needs to be aware of the repeater presence and to handle
> > the link training accordingly. This is currently missing from msm dp
> > driver, but there is already effort going on to add it. Once done,
> > full external DP will be working on all X1E laptops that make use of
> > this retimer.
> 
> I was gonna ask you to include the devicetree changes that enables the
> retimers as part of this series (to facilitate review and testing), but
> perhaps you should indeed not post them again until LTTPR support is in
> place.

I was thinking maybe we should not wait for LTTPR support as this series
brings orientation support as is. I still need to figure out how to
strip out the DP parts of it in such a way that orientation should still
be working but DP should not (until LTTPR is in).

> 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> > Changes in v2:
> > - Addressed all comments from Johan and Konrad.
> > - Reworked the handling of the vregs so it would be more cleaner.
> >   Dropped the usage of bulk regulators API and handled them separately.
> >   Also discribed all regulators according to data sheet.
> > - Added all delays according to data sheet.
> > - Fixed coldplug (on boot) orientation detection.
> 
> Coldplug orientation detection still does not work here with this series
> applied.
> 
> I'm not entirely sure this whether worked better with v1, but with v2
> my SuperSpeed ethernet device shows up as a HighSpeed device in one
> orientation. It is also not disconnected an re-enumerated as SS as is
> the case on the X13s (and possibly with v1):
> 
> 	usb 1-1: new high-speed USB device number 2 using xhci-hcd

For coldplug, this series does the right thing as it leaves the retimer
initialized if it was left enabled at boot. There is a second part
needed for the coldplug to work. That is the regulator-boot-on property
in retimer's vregs nodes. That will ensure that the regulator is not
disabled until retimer driver probes and will keep the retimer initialized
until USB device is enumerated.

> 
> > - Didn't pick Krzysztof's R-b tag because the bindings changed w.r.t
> >   supplies.
> > - Link to v1: https://lore.kernel.org/r/20240829-x1e80100-ps8830-v1-0-bcc4790b1d45@linaro.org
> > 
> > ---
> > Abel Vesa (2):
> >       dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings
> >       usb: typec: Add support for Parade PS8830 Type-C Retimer
> 
> Johan

Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer

[Konrad Dybcio]

On 10/15/24 15:03, Abel Vesa wrote:
> On 24-10-15 14:41:25, Johan Hovold wrote:
>> On Fri, Oct 04, 2024 at 04:57:36PM +0300, Abel Vesa wrote:
>>> The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
>>> via I2C. It provides altmode and orientation handling and usually sits
>>> between the Type-C port and the PHY.
>>>
>>> It is currently used alongside Qualcomm Snapdragon X Elite SoCs on quite
>>> a few laptops already.
>>>
>>> This new driver adds support for the following 3 modes:
>>>   - DP 4lanes (pin assignments C and E)
>>>   - DP 2lanes + USB3 (pin assignment D)
>>>   - USB3
>>>
>>> This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
>>> it can support link training from source to itself. This means that the
>>> DP driver needs to be aware of the repeater presence and to handle
>>> the link training accordingly. This is currently missing from msm dp
>>> driver, but there is already effort going on to add it. Once done,
>>> full external DP will be working on all X1E laptops that make use of
>>> this retimer.
>>
>> I was gonna ask you to include the devicetree changes that enables the
>> retimers as part of this series (to facilitate review and testing), but
>> perhaps you should indeed not post them again until LTTPR support is in
>> place.
> 
> I was thinking maybe we should not wait for LTTPR support as this series
> brings orientation support as is.

It also happens to bring an undesired crash-on-unplug feature when
DP is enabled.. I suppose it's fine to bring this series in if you
separate enabling the retimer on devices from wiring DP up.

Konrad

Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer

[Johan Hovold]

On Tue, Oct 15, 2024 at 04:03:53PM +0300, Abel Vesa wrote:
> On 24-10-15 14:41:25, Johan Hovold wrote:
> > On Fri, Oct 04, 2024 at 04:57:36PM +0300, Abel Vesa wrote:
> > > The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
> > > via I2C. It provides altmode and orientation handling and usually sits
> > > between the Type-C port and the PHY.

> > > This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
> > > it can support link training from source to itself. This means that the
> > > DP driver needs to be aware of the repeater presence and to handle
> > > the link training accordingly. This is currently missing from msm dp
> > > driver, but there is already effort going on to add it. Once done,
> > > full external DP will be working on all X1E laptops that make use of
> > > this retimer.
> > 
> > I was gonna ask you to include the devicetree changes that enables the
> > retimers as part of this series (to facilitate review and testing), but
> > perhaps you should indeed not post them again until LTTPR support is in
> > place.
> 
> I was thinking maybe we should not wait for LTTPR support as this series
> brings orientation support as is. I still need to figure out how to
> strip out the DP parts of it in such a way that orientation should still
> be working but DP should not (until LTTPR is in).

Yeah, possible, or you can at least include the DT patches here but mark
them as do-not-merge-yet or similar.

> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > ---
> > > Changes in v2:

> > > - Fixed coldplug (on boot) orientation detection.
> > 
> > Coldplug orientation detection still does not work here with this series
> > applied.
> > 
> > I'm not entirely sure this whether worked better with v1, but with v2
> > my SuperSpeed ethernet device shows up as a HighSpeed device in one
> > orientation. It is also not disconnected an re-enumerated as SS as is
> > the case on the X13s (and possibly with v1):
> > 
> > 	usb 1-1: new high-speed USB device number 2 using xhci-hcd
> 
> For coldplug, this series does the right thing as it leaves the retimer
> initialized if it was left enabled at boot. There is a second part
> needed for the coldplug to work. That is the regulator-boot-on property
> in retimer's vregs nodes. That will ensure that the regulator is not
> disabled until retimer driver probes and will keep the retimer initialized
> until USB device is enumerated.

I can confirm that marking the regulators as having been left on by the
bootloader so that they are not disabled temporarily during boot indeed
fixes the coldplug issue here.

That however makes me wonder whether something is missing in the driver
so that it still relies on setup having been done by the boot firmware.

Have you tried actually asserting reset during probe to verify that
driver can configure the retimers itself without relying on the boot
firmware?

Johan

Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer

[Abel Vesa]

On 24-10-17 08:00:44, Johan Hovold wrote:
> On Tue, Oct 15, 2024 at 04:03:53PM +0300, Abel Vesa wrote:
> > On 24-10-15 14:41:25, Johan Hovold wrote:
> > > On Fri, Oct 04, 2024 at 04:57:36PM +0300, Abel Vesa wrote:
> > > > The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
> > > > via I2C. It provides altmode and orientation handling and usually sits
> > > > between the Type-C port and the PHY.
> 
> > > > This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
> > > > it can support link training from source to itself. This means that the
> > > > DP driver needs to be aware of the repeater presence and to handle
> > > > the link training accordingly. This is currently missing from msm dp
> > > > driver, but there is already effort going on to add it. Once done,
> > > > full external DP will be working on all X1E laptops that make use of
> > > > this retimer.
> > > 
> > > I was gonna ask you to include the devicetree changes that enables the
> > > retimers as part of this series (to facilitate review and testing), but
> > > perhaps you should indeed not post them again until LTTPR support is in
> > > place.
> > 
> > I was thinking maybe we should not wait for LTTPR support as this series
> > brings orientation support as is. I still need to figure out how to
> > strip out the DP parts of it in such a way that orientation should still
> > be working but DP should not (until LTTPR is in).
> 
> Yeah, possible, or you can at least include the DT patches here but mark
> them as do-not-merge-yet or similar.

Sure, will do that. Will have to split the DP part of it into separate
patches and mark only those as do-not-merge-yet. 

> 
> > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > > ---
> > > > Changes in v2:
> 
> > > > - Fixed coldplug (on boot) orientation detection.
> > > 
> > > Coldplug orientation detection still does not work here with this series
> > > applied.
> > > 
> > > I'm not entirely sure this whether worked better with v1, but with v2
> > > my SuperSpeed ethernet device shows up as a HighSpeed device in one
> > > orientation. It is also not disconnected an re-enumerated as SS as is
> > > the case on the X13s (and possibly with v1):
> > > 
> > > 	usb 1-1: new high-speed USB device number 2 using xhci-hcd
> > 
> > For coldplug, this series does the right thing as it leaves the retimer
> > initialized if it was left enabled at boot. There is a second part
> > needed for the coldplug to work. That is the regulator-boot-on property
> > in retimer's vregs nodes. That will ensure that the regulator is not
> > disabled until retimer driver probes and will keep the retimer initialized
> > until USB device is enumerated.
> 
> I can confirm that marking the regulators as having been left on by the
> bootloader so that they are not disabled temporarily during boot indeed
> fixes the coldplug issue here.
> 
> That however makes me wonder whether something is missing in the driver
> so that it still relies on setup having been done by the boot firmware.
> 
> Have you tried actually asserting reset during probe to verify that
> driver can configure the retimers itself without relying on the boot
> firmware?

We do not want to reset the retimers on probe because we won't be able
to figure out the orientation config until next pmic glink notify comes.
The pmic glink notify only triggers on USB event, which never comes
until you replug the device. So in order to have coldplug orientation
configured correctly in the retimer, we need to make sure the retimer
holds state until unplug.

> 
> Johan

Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Abel Vesa]

On 24-10-06 18:40:51, Dmitry Baryshkov wrote:
> On Fri, Oct 04, 2024 at 04:57:38PM GMT, Abel Vesa wrote:
> > The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
> > It provides both altmode and orientation handling.
> > 
> > Add a driver with support for the following modes:
> >  - DP 4lanes
> >  - DP 2lanes + USB3
> >  - USB3
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> >  drivers/usb/typec/mux/Kconfig  |  10 +
> >  drivers/usb/typec/mux/Makefile |   1 +
> >  drivers/usb/typec/mux/ps8830.c | 424 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 435 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/mux/Kconfig b/drivers/usb/typec/mux/Kconfig
> > index ce7db6ad30572a0a74890f5f11944fb3ff07f635..48613b67f1c5dacd14d54baf91c3066377cf97be 100644
> > --- a/drivers/usb/typec/mux/Kconfig
> > +++ b/drivers/usb/typec/mux/Kconfig
> > @@ -56,6 +56,16 @@ config TYPEC_MUX_NB7VPQ904M
> >  	  Say Y or M if your system has a On Semiconductor NB7VPQ904M Type-C
> >  	  redriver chip found on some devices with a Type-C port.
> >  
> > +config TYPEC_MUX_PS8830
> > +	tristate "Parade PS8830 Type-C retimer driver"
> > +	depends on I2C
> > +	depends on DRM || DRM=n
> > +	select DRM_AUX_BRIDGE if DRM_BRIDGE && OF
> > +	select REGMAP_I2C
> > +	help
> > +	  Say Y or M if your system has a Parade PS8830 Type-C retimer chip
> > +	  found on some devices with a Type-C port.
> > +
> >  config TYPEC_MUX_PTN36502
> >  	tristate "NXP PTN36502 Type-C redriver driver"
> >  	depends on I2C
> > diff --git a/drivers/usb/typec/mux/Makefile b/drivers/usb/typec/mux/Makefile
> > index bb96f30267af05b33b9277dcf1cc0e1527d2dcdd..4b23b12cfe45a0ff8a37f38c7ba050f572d556e7 100644
> > --- a/drivers/usb/typec/mux/Makefile
> > +++ b/drivers/usb/typec/mux/Makefile
> > @@ -6,5 +6,6 @@ obj-$(CONFIG_TYPEC_MUX_PI3USB30532)	+= pi3usb30532.o
> >  obj-$(CONFIG_TYPEC_MUX_INTEL_PMC)	+= intel_pmc_mux.o
> >  obj-$(CONFIG_TYPEC_MUX_IT5205)		+= it5205.o
> >  obj-$(CONFIG_TYPEC_MUX_NB7VPQ904M)	+= nb7vpq904m.o
> > +obj-$(CONFIG_TYPEC_MUX_PS8830)		+= ps8830.o
> >  obj-$(CONFIG_TYPEC_MUX_PTN36502)	+= ptn36502.o
> >  obj-$(CONFIG_TYPEC_MUX_WCD939X_USBSS)	+= wcd939x-usbss.o
> > diff --git a/drivers/usb/typec/mux/ps8830.c b/drivers/usb/typec/mux/ps8830.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..58990f7860bf77ec12d00f0d3df3a40735bbf9d8
> > --- /dev/null
> > +++ b/drivers/usb/typec/mux/ps8830.c
> > @@ -0,0 +1,424 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Parade PS8830 usb retimer driver
> > + *
> > + * Copyright (C) 2024 Linaro Ltd.
> > + */
> > +
> > +#include <drm/bridge/aux-bridge.h>
> > +#include <linux/clk.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/usb/typec_altmode.h>
> > +#include <linux/usb/typec_dp.h>
> > +#include <linux/usb/typec_mux.h>
> > +#include <linux/usb/typec_retimer.h>
> > +
> > +struct ps8830_retimer {
> > +	struct i2c_client *client;
> > +	struct gpio_desc *reset_gpio;
> > +	struct regmap *regmap;
> > +	struct typec_switch_dev *sw;
> > +	struct typec_retimer *retimer;
> > +	struct clk *xo_clk;
> > +	struct regulator *vdd_supply;
> > +	struct regulator *vdd33_supply;
> > +	struct regulator *vdd33_cap_supply;
> > +	struct regulator *vddat_supply;
> > +	struct regulator *vddar_supply;
> > +	struct regulator *vddio_supply;
> > +
> > +	struct typec_switch *typec_switch;
> > +	struct typec_mux *typec_mux;
> > +
> > +	struct mutex lock; /* protect non-concurrent retimer & switch */
> > +
> > +	enum typec_orientation orientation;
> > +	unsigned long mode;
> > +	int cfg[3];
> > +};
> > +
> > +static void ps8830_write(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2)
> > +{
> > +	if (cfg0 == retimer->cfg[0] &&
> > +	    cfg1 == retimer->cfg[1] &&
> > +	    cfg2 == retimer->cfg[2])
> > +		return;
> > +
> > +	retimer->cfg[0] = cfg0;
> > +	retimer->cfg[1] = cfg1;
> > +	retimer->cfg[2] = cfg2;
> > +
> > +	regmap_write(retimer->regmap, 0x0, cfg0);
> > +	regmap_write(retimer->regmap, 0x1, cfg1);
> > +	regmap_write(retimer->regmap, 0x2, cfg2);
> > +}
> 
> This looks like a reimplementation of regcache. Is it really necessary?

Sure, will use regcache.

> 
> > +
> > +static void ps8830_configure(struct ps8830_retimer *retimer, int cfg0, int cfg1, int cfg2)
> > +{
> > +	/* Write safe-mode config before switching to new config */
> 
> Why is this required?

Data sheet says it needs to be configured into safe mode before
switching modes.

> 
> > +	ps8830_write(retimer, 0x1, 0x0, 0x0);
> > +
> > +	ps8830_write(retimer, cfg0, cfg1, cfg2);
> > +}
> > +
> > +static int ps8380_set(struct ps8830_retimer *retimer)
> > +{
> > +	int cfg0 = 0x00;
> > +	int cfg1 = 0x00;
> > +	int cfg2 = 0x00;
> > +
> > +	if (retimer->orientation == TYPEC_ORIENTATION_NONE ||
> > +	    retimer->mode == TYPEC_STATE_SAFE) {
> > +		ps8830_write(retimer, 0x1, 0x0, 0x0);
> > +		return 0;
> > +	}
> > +
> > +	if (retimer->orientation == TYPEC_ORIENTATION_NORMAL)
> > +		cfg0 = 0x01;
> > +	else
> > +		cfg0 = 0x03;
> > +
> > +	switch (retimer->mode) {
> > +	/* USB3 Only */
> > +	case TYPEC_STATE_USB:
> > +		cfg0 |= 0x20;
> > +		break;
> > +
> 
> The TYPEC_DP clauses should only be used if state->alt->swid is set to
> USB_TYPEC_DP_SID. Other altmodes share id space for their states.

Make sense. Will check svid for DP.

> 
> > +	/* DP Only */
> > +	case TYPEC_DP_STATE_C:
> > +		cfg1 = 0x85;
> > +		break;
> > +
> > +	case TYPEC_DP_STATE_E:
> > +		cfg1 = 0x81;
> > +		break;
> > +
> > +	/* DP + USB */
> > +	case TYPEC_DP_STATE_D:
> > +		cfg0 |= 0x20;
> > +		cfg1 = 0x85;
> > +		break;
> 
> CDE, please.

Will do.

> 
> > +
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	ps8830_configure(retimer, cfg0, cfg1, cfg2);
> > +
> > +	return 0;
> > +}
> > +
> 
> -- 
> With best wishes
> Dmitry

Thanks for reviewing.

Abel

Re: [PATCH v2 1/2] dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings

[Abel Vesa]

On 24-10-06 18:28:52, Dmitry Baryshkov wrote:
> On Fri, Oct 04, 2024 at 04:57:37PM GMT, Abel Vesa wrote:
> > Document bindings for the Parade PS8830 Type-C retimer. This retimer is
> > currently found on all boards featuring Qualcomm Snapdragon X Elite SoCs
> > and it is needed to provide altmode muxing between DP and USB, but also
> > connector orientation handling between.
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> >  .../devicetree/bindings/usb/parade,ps8830.yaml     | 129 +++++++++++++++++++++
> >  1 file changed, 129 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/parade,ps8830.yaml b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..f6721d6eee26c6d4590a12c19791b3d47a8145f3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/parade,ps8830.yaml
> > @@ -0,0 +1,129 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/parade,ps8830.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Parade PS8830 USB and DisplayPort Retimer
> > +
> > +maintainers:
> > +  - Abel Vesa <abel.vesa@linaro.org>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - parade,ps8830
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: XO Clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: xo
> > +
> > +  reset-gpios:
> > +    maxItems: 1
> > +
> > +  vdd-supply:
> > +    description: power supply (1.07V)
> > +
> > +  vdd33-supply:
> > +    description: power supply (3.3V)
> > +
> > +  vdd33-cap-supply:
> > +    description: power supply (3.3V)
> > +
> > +  vddar-supply:
> > +    description: power supply (1.07V)
> > +
> > +  vddat-supply:
> > +    description: power supply (1.07V)
> 
> Any additional details?

Documentation doesn't say anything more than this.

> 
> > +
> > +  vddio-supply:
> > +    description: power supply (1.2V or 1.8V)
> > +
> > +  orientation-switch: true
> > +  retimer-switch: true
> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: Super Speed (SS) Output endpoint to the Type-C connector
> > +
> > +      port@1:
> > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > +        description: Super Speed (SS) Input endpoint from the Super-Speed PHY
> 
> or from another SS signal source, which can be a mux, a switch or
> anything else. I'd say, just 'Input Super Speed (SS)'

Will use that.

> 
> > +        unevaluatedProperties: false
> > +
> > +      port@2:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description:
> > +          Sideband Use (SBU) AUX lines endpoint to the Type-C connector for the purpose of
> > +          handling altmode muxing and orientation switching.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +allOf:
> > +  - $ref: usb-switch.yaml#
> > +
> > +additionalProperties: false
> > +
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer

[Johan Hovold]

On Thu, Oct 17, 2024 at 11:25:20AM +0300, Abel Vesa wrote:
> On 24-10-17 08:00:44, Johan Hovold wrote:

> > I can confirm that marking the regulators as having been left on by the
> > bootloader so that they are not disabled temporarily during boot indeed
> > fixes the coldplug issue here.
> > 
> > That however makes me wonder whether something is missing in the driver
> > so that it still relies on setup having been done by the boot firmware.
> > 
> > Have you tried actually asserting reset during probe to verify that
> > driver can configure the retimers itself without relying on the boot
> > firmware?
> 
> We do not want to reset the retimers on probe because we won't be able
> to figure out the orientation config until next pmic glink notify comes.
> The pmic glink notify only triggers on USB event, which never comes
> until you replug the device. So in order to have coldplug orientation
> configured correctly in the retimer, we need to make sure the retimer
> holds state until unplug.

Ok, thanks for clarifying. As we've discussed off-list it should be
possible to retrieve the orientation from the orientation gpios, but I'm
sure there are bits missing in the kernel for propagating that
information to the retimers currently.

If I understood you correctly you did reset and reinitialise the
retimers in v1, which is useful during development at least to make sure
that the driver is complete.

Johan

Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Christophe JAILLET]

Le 04/10/2024 à 15:57, Abel Vesa a écrit :
> The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
> It provides both altmode and orientation handling.
> 
> Add a driver with support for the following modes:
>   - DP 4lanes
>   - DP 2lanes + USB3
>   - USB3
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---

Hi,

> +static int ps8830_retimer_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct typec_switch_desc sw_desc = { };
> +	struct typec_retimer_desc rtmr_desc = { };
> +	struct ps8830_retimer *retimer;
> +	int ret;
> +
> +	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
> +	if (!retimer)
> +		return -ENOMEM;
> +
> +	retimer->client = client;
> +
> +	mutex_init(&retimer->lock);
> +
> +	retimer->regmap = devm_regmap_init_i2c(client, &ps8830_retimer_regmap);
> +	if (IS_ERR(retimer->regmap)) {
> +		dev_err(dev, "failed to allocate register map\n");
> +		return PTR_ERR(retimer->regmap);
> +	}
> +
> +	ret = ps8830_get_vregs(retimer);
> +	if (ret)
> +		return ret;
> +
> +	retimer->xo_clk = devm_clk_get(dev, "xo");
> +	if (IS_ERR(retimer->xo_clk))
> +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> +				     "failed to get xo clock\n");
> +
> +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(retimer->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(retimer->reset_gpio),
> +				     "failed to get reset gpio\n");
> +
> +	retimer->typec_switch = fwnode_typec_switch_get(dev->fwnode);
> +	if (IS_ERR(retimer->typec_switch)) {
> +		dev_err(dev, "failed to acquire orientation-switch\n");
> +		return PTR_ERR(retimer->typec_switch);
> +	}
> +
> +	retimer->typec_mux = fwnode_typec_mux_get(dev->fwnode);
> +	if (IS_ERR(retimer->typec_mux)) {
> +		dev_err(dev, "failed to acquire mode-mux\n");
> +		goto err_switch_put;
> +	}
> +
> +	sw_desc.drvdata = retimer;
> +	sw_desc.fwnode = dev_fwnode(dev);
> +	sw_desc.set = ps8830_sw_set;
> +
> +	ret = drm_aux_bridge_register(dev);
> +	if (ret)
> +		goto err_mux_put;
> +
> +	retimer->sw = typec_switch_register(dev, &sw_desc);
> +	if (IS_ERR(retimer->sw)) {
> +		dev_err(dev, "failed to register typec switch\n");
> +		goto err_aux_bridge_unregister;
> +	}
> +
> +	rtmr_desc.drvdata = retimer;
> +	rtmr_desc.fwnode = dev_fwnode(dev);
> +	rtmr_desc.set = ps8830_retimer_set;
> +
> +	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
> +	if (IS_ERR(retimer->retimer)) {
> +		dev_err(dev, "failed to register typec retimer\n");
> +		goto err_switch_unregister;
> +	}
> +
> +	ret = clk_prepare_enable(retimer->xo_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable XO: %d\n", ret);
> +		goto err_retimer_unregister;
> +	}
> +
> +	ret = ps8830_enable_vregs(retimer);
> +	if (ret)
> +		goto err_clk_disable;
> +
> +	/* delay needed as per datasheet */
> +	usleep_range(4000, 14000);
> +
> +	gpiod_set_value(retimer->reset_gpio, 1);
> +
> +	return 0;
> +
> +err_clk_disable:
> +	clk_disable_unprepare(retimer->xo_clk);
> +
> +err_retimer_unregister:
> +	typec_retimer_unregister(retimer->retimer);
> +
> +err_switch_unregister:
> +	typec_switch_unregister(retimer->sw);
> +
> +err_aux_bridge_unregister:
> +	gpiod_set_value(retimer->reset_gpio, 0);

Is this called useful here?
gpiod_set_value(, 1) has not been called yet.

It made sense to have something like that in v1, but it looks strange in v2.

CJ

> +	clk_disable_unprepare(retimer->xo_clk);
> +
> +err_mux_put:
> +	typec_mux_put(retimer->typec_mux);
> +
> +err_switch_put:
> +	typec_switch_put(retimer->typec_switch);
> +
> +	return ret;
> +}

...

Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Abel Vesa]

On 24-10-22 09:41:42, Christophe JAILLET wrote:
> Le 04/10/2024 à 15:57, Abel Vesa a écrit :
> > The Parade PS8830 is a Type-C muti-protocol retimer controlled over I2C.
> > It provides both altmode and orientation handling.
> > 
> > Add a driver with support for the following modes:
> >   - DP 4lanes
> >   - DP 2lanes + USB3
> >   - USB3
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> 
> Hi,
> 
> > +static int ps8830_retimer_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct typec_switch_desc sw_desc = { };
> > +	struct typec_retimer_desc rtmr_desc = { };
> > +	struct ps8830_retimer *retimer;
> > +	int ret;
> > +
> > +	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
> > +	if (!retimer)
> > +		return -ENOMEM;
> > +
> > +	retimer->client = client;
> > +
> > +	mutex_init(&retimer->lock);
> > +
> > +	retimer->regmap = devm_regmap_init_i2c(client, &ps8830_retimer_regmap);
> > +	if (IS_ERR(retimer->regmap)) {
> > +		dev_err(dev, "failed to allocate register map\n");
> > +		return PTR_ERR(retimer->regmap);
> > +	}
> > +
> > +	ret = ps8830_get_vregs(retimer);
> > +	if (ret)
> > +		return ret;
> > +
> > +	retimer->xo_clk = devm_clk_get(dev, "xo");
> > +	if (IS_ERR(retimer->xo_clk))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> > +				     "failed to get xo clock\n");
> > +
> > +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(retimer->reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->reset_gpio),
> > +				     "failed to get reset gpio\n");
> > +
> > +	retimer->typec_switch = fwnode_typec_switch_get(dev->fwnode);
> > +	if (IS_ERR(retimer->typec_switch)) {
> > +		dev_err(dev, "failed to acquire orientation-switch\n");
> > +		return PTR_ERR(retimer->typec_switch);
> > +	}
> > +
> > +	retimer->typec_mux = fwnode_typec_mux_get(dev->fwnode);
> > +	if (IS_ERR(retimer->typec_mux)) {
> > +		dev_err(dev, "failed to acquire mode-mux\n");
> > +		goto err_switch_put;
> > +	}
> > +
> > +	sw_desc.drvdata = retimer;
> > +	sw_desc.fwnode = dev_fwnode(dev);
> > +	sw_desc.set = ps8830_sw_set;
> > +
> > +	ret = drm_aux_bridge_register(dev);
> > +	if (ret)
> > +		goto err_mux_put;
> > +
> > +	retimer->sw = typec_switch_register(dev, &sw_desc);
> > +	if (IS_ERR(retimer->sw)) {
> > +		dev_err(dev, "failed to register typec switch\n");
> > +		goto err_aux_bridge_unregister;
> > +	}
> > +
> > +	rtmr_desc.drvdata = retimer;
> > +	rtmr_desc.fwnode = dev_fwnode(dev);
> > +	rtmr_desc.set = ps8830_retimer_set;
> > +
> > +	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
> > +	if (IS_ERR(retimer->retimer)) {
> > +		dev_err(dev, "failed to register typec retimer\n");
> > +		goto err_switch_unregister;
> > +	}
> > +
> > +	ret = clk_prepare_enable(retimer->xo_clk);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable XO: %d\n", ret);
> > +		goto err_retimer_unregister;
> > +	}
> > +
> > +	ret = ps8830_enable_vregs(retimer);
> > +	if (ret)
> > +		goto err_clk_disable;
> > +
> > +	/* delay needed as per datasheet */
> > +	usleep_range(4000, 14000);
> > +
> > +	gpiod_set_value(retimer->reset_gpio, 1);
> > +
> > +	return 0;
> > +
> > +err_clk_disable:
> > +	clk_disable_unprepare(retimer->xo_clk);
> > +
> > +err_retimer_unregister:
> > +	typec_retimer_unregister(retimer->retimer);
> > +
> > +err_switch_unregister:
> > +	typec_switch_unregister(retimer->sw);
> > +
> > +err_aux_bridge_unregister:
> > +	gpiod_set_value(retimer->reset_gpio, 0);
> 
> Is this called useful here?
> gpiod_set_value(, 1) has not been called yet.
> 
> It made sense to have something like that in v1, but it looks strange in v2.
> 

The devm_gpiod_get() flag sets it to HIGH.

Anyway, this will be reworked in v3 as the reset gpio is active low.

> CJ
> 
> > +	clk_disable_unprepare(retimer->xo_clk);
> > +
> > +err_mux_put:
> > +	typec_mux_put(retimer->typec_mux);
> > +
> > +err_switch_put:
> > +	typec_switch_put(retimer->typec_switch);
> > +
> > +	return ret;
> > +}
> 
> ...

Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Abel Vesa]

On 24-10-15 15:03:15, Johan Hovold wrote:
> On Fri, Oct 04, 2024 at 04:57:38PM +0300, Abel Vesa wrote:
> 
> > +static int ps8830_enable_vregs(struct ps8830_retimer *retimer)
> > +{
> 
> > +	return 0;
> > +
> > +err_vddat_disable:
> > +	regulator_disable(retimer->vddat_supply);
> > +
> 
> Nit: I'd drop the empty lines between the errors cases here.

Will drop.

> 
> > +err_vddar_disable:
> > +	regulator_disable(retimer->vddar_supply);
> > +
> > +err_vdd_disable:
> > +	regulator_disable(retimer->vdd_supply);
> > +
> > +err_vdd33_cap_disable:
> > +	regulator_disable(retimer->vdd33_cap_supply);
> > +
> > +err_vdd33_disable:
> > +	regulator_disable(retimer->vdd33_supply);
> > +
> > +	return ret;
> > +}
> 
> > +static int ps8830_retimer_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct typec_switch_desc sw_desc = { };
> > +	struct typec_retimer_desc rtmr_desc = { };
> > +	struct ps8830_retimer *retimer;
> > +	int ret;
> > +
> > +	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
> > +	if (!retimer)
> > +		return -ENOMEM;
> > +
> > +	retimer->client = client;
> > +
> > +	mutex_init(&retimer->lock);
> > +
> > +	retimer->regmap = devm_regmap_init_i2c(client, &ps8830_retimer_regmap);
> > +	if (IS_ERR(retimer->regmap)) {
> > +		dev_err(dev, "failed to allocate register map\n");
> 
> Please make sure to log the errno as well here and below.

Will add.

> 
> > +		return PTR_ERR(retimer->regmap);
> > +	}
> > +
> > +	ret = ps8830_get_vregs(retimer);
> > +	if (ret)
> > +		return ret;
> > +
> > +	retimer->xo_clk = devm_clk_get(dev, "xo");
> > +	if (IS_ERR(retimer->xo_clk))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> > +				     "failed to get xo clock\n");
> > +
> > +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> 
> The reset line is active low and should be described as such in DT. So
> here you want to request it as logically low if you want to deassert
> reset.

This is being reworked in v3 as we need to support cases where the
retimer has been left enabled and initialized by bootloader and we want
to keep that state until unplug event for the cold-plug orientation
to work properly.

On top of that, we don't want to deassert the reset here. We do that
via gpiod_set_value() call below, after the clocks and regulators have
been enabled.

> 
> Is there now timing requirements on when you deassert reset after
> enabling the supplies?

So based on my comment above, this is actually asserting the reset.
No timing requirements for that.

> 
> > +	if (IS_ERR(retimer->reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(retimer->reset_gpio),
> > +				     "failed to get reset gpio\n");
> > +
> > +	retimer->typec_switch = fwnode_typec_switch_get(dev->fwnode);
> > +	if (IS_ERR(retimer->typec_switch)) {
> > +		dev_err(dev, "failed to acquire orientation-switch\n");
> 
> I saw the driver fail here once, but not sure what the errno was since
> it was not printed. Presumably it was a probe deferral and then this
> should be a dev_err_probe() as well:
> 
> 	ps8830_retimer 2-0008: failed to acquire orientation-switch

Will use dev_err_probe.

> 
> > +		return PTR_ERR(retimer->typec_switch);
> > +	}
> > +
> > +	retimer->typec_mux = fwnode_typec_mux_get(dev->fwnode);
> > +	if (IS_ERR(retimer->typec_mux)) {
> > +		dev_err(dev, "failed to acquire mode-mux\n");
> 
> Similar here perhaps?

Same.

> 
> > +		goto err_switch_put;
> > +	}
> > +
> > +	sw_desc.drvdata = retimer;
> > +	sw_desc.fwnode = dev_fwnode(dev);
> > +	sw_desc.set = ps8830_sw_set;
> > +
> > +	ret = drm_aux_bridge_register(dev);
> > +	if (ret)
> > +		goto err_mux_put;
> > +
> > +	retimer->sw = typec_switch_register(dev, &sw_desc);
> > +	if (IS_ERR(retimer->sw)) {
> > +		dev_err(dev, "failed to register typec switch\n");
> > +		goto err_aux_bridge_unregister;
> > +	}
> > +
> > +	rtmr_desc.drvdata = retimer;
> > +	rtmr_desc.fwnode = dev_fwnode(dev);
> > +	rtmr_desc.set = ps8830_retimer_set;
> > +
> > +	retimer->retimer = typec_retimer_register(dev, &rtmr_desc);
> > +	if (IS_ERR(retimer->retimer)) {
> > +		dev_err(dev, "failed to register typec retimer\n");
> > +		goto err_switch_unregister;
> > +	}
> > +
> > +	ret = clk_prepare_enable(retimer->xo_clk);
> > +	if (ret) {
> > +		dev_err(dev, "failed to enable XO: %d\n", ret);
> > +		goto err_retimer_unregister;
> > +	}
> 
> Should you really enable the clock before the regulators?

So maybe in this case it might not really matter. But in principle,
the HW might be affected by clock glitches and such when IP block
is powered up but unclocked. Even more so if the clock enabling
(prepare, to be more exact) involves switching to a new PLL.

So clock first, then power up. At least that's my understanding of HW
in general.

> 
> > +
> > +	ret = ps8830_enable_vregs(retimer);
> > +	if (ret)
> > +		goto err_clk_disable;
> > +
> > +	/* delay needed as per datasheet */
> > +	usleep_range(4000, 14000);
> > +
> > +	gpiod_set_value(retimer->reset_gpio, 1);
> 
> Here you only deassert reset in case the line is incorrectly described
> as active high in DT.

Yes, this needs to be 0 instead of 1. And in v3 it will depend on
a DT property called ps8830,boot-on, meaning if we want to keep it
enabled and configured as left by bootloader, by using that property
we will skip the resetting altogether.

> 
> > +	return 0;
> > +
> > +err_clk_disable:
> > +	clk_disable_unprepare(retimer->xo_clk);
> > +
> > +err_retimer_unregister:
> > +	typec_retimer_unregister(retimer->retimer);
> > +
> > +err_switch_unregister:
> > +	typec_switch_unregister(retimer->sw);
> > +
> > +err_aux_bridge_unregister:
> > +	gpiod_set_value(retimer->reset_gpio, 0);
> > +	clk_disable_unprepare(retimer->xo_clk);
> > +
> > +err_mux_put:
> > +	typec_mux_put(retimer->typec_mux);
> > +
> > +err_switch_put:
> > +	typec_switch_put(retimer->typec_switch);
> 
> Drop newlines before labels?

Will do.

> 
> > +
> > +	return ret;
> > +}
> 
> Johan

Thanks for reviewing.

Abel

Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Johan Hovold]

On Tue, Oct 22, 2024 at 12:01:14PM +0300, Abel Vesa wrote:
> On 24-10-15 15:03:15, Johan Hovold wrote:
> > On Fri, Oct 04, 2024 at 04:57:38PM +0300, Abel Vesa wrote:

> > > +	ret = ps8830_get_vregs(retimer);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	retimer->xo_clk = devm_clk_get(dev, "xo");
> > > +	if (IS_ERR(retimer->xo_clk))
> > > +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> > > +				     "failed to get xo clock\n");
> > > +
> > > +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > 
> > The reset line is active low and should be described as such in DT. So
> > here you want to request it as logically low if you want to deassert
> > reset.
> 
> This is being reworked in v3 as we need to support cases where the
> retimer has been left enabled and initialized by bootloader and we want
> to keep that state until unplug event for the cold-plug orientation
> to work properly.
> 
> On top of that, we don't want to deassert the reset here. We do that
> via gpiod_set_value() call below, after the clocks and regulators have
> been enabled.

Ok, but you should generally not drive an input high before powering on
the device as that can damage the IC (more below).

That is, in this case, you should not deassert reset before making sure
the supplies are enabled.

> > > +	ret = clk_prepare_enable(retimer->xo_clk);
> > > +	if (ret) {
> > > +		dev_err(dev, "failed to enable XO: %d\n", ret);
> > > +		goto err_retimer_unregister;
> > > +	}
> > 
> > Should you really enable the clock before the regulators?
> 
> So maybe in this case it might not really matter. But in principle,
> the HW might be affected by clock glitches and such when IP block
> is powered up but unclocked. Even more so if the clock enabling
> (prepare, to be more exact) involves switching to a new PLL.
> 
> So clock first, then power up. At least that's my understanding of HW
> in general.

I think you got that backwards as inputs are typically rated for some
maximum voltage based on the supply voltage. That applies also to the
reset line as I also mentioned above.

What does the datasheet say?

> > > +
> > > +	ret = ps8830_enable_vregs(retimer);
> > > +	if (ret)
> > > +		goto err_clk_disable;

Johan

Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Abel Vesa]

On 24-10-23 09:04:10, Johan Hovold wrote:
> On Tue, Oct 22, 2024 at 12:01:14PM +0300, Abel Vesa wrote:
> > On 24-10-15 15:03:15, Johan Hovold wrote:
> > > On Fri, Oct 04, 2024 at 04:57:38PM +0300, Abel Vesa wrote:
> 
> > > > +	ret = ps8830_get_vregs(retimer);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	retimer->xo_clk = devm_clk_get(dev, "xo");
> > > > +	if (IS_ERR(retimer->xo_clk))
> > > > +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> > > > +				     "failed to get xo clock\n");
> > > > +
> > > > +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > > 
> > > The reset line is active low and should be described as such in DT. So
> > > here you want to request it as logically low if you want to deassert
> > > reset.
> > 
> > This is being reworked in v3 as we need to support cases where the
> > retimer has been left enabled and initialized by bootloader and we want
> > to keep that state until unplug event for the cold-plug orientation
> > to work properly.
> > 
> > On top of that, we don't want to deassert the reset here. We do that
> > via gpiod_set_value() call below, after the clocks and regulators have
> > been enabled.
> 
> Ok, but you should generally not drive an input high before powering on
> the device as that can damage the IC (more below).

This is just not true, generally. Think of top level XTALs which feed in
clocks (and can't be disabled) before ICs are enabled.

> 
> That is, in this case, you should not deassert reset before making sure
> the supplies are enabled.

Wrong. Even the data sheet of this retimer shows in the timigs plot the
reset as being asserted before the supplies are enabled.

And generally speaking, the reset needs to be asserted before the
supplies are up, so that the IC doesn't start doing any work until
the SW decides it needs to.

> 
> > > > +	ret = clk_prepare_enable(retimer->xo_clk);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "failed to enable XO: %d\n", ret);
> > > > +		goto err_retimer_unregister;
> > > > +	}
> > > 
> > > Should you really enable the clock before the regulators?
> > 
> > So maybe in this case it might not really matter. But in principle,
> > the HW might be affected by clock glitches and such when IP block
> > is powered up but unclocked. Even more so if the clock enabling
> > (prepare, to be more exact) involves switching to a new PLL.
> > 
> > So clock first, then power up. At least that's my understanding of HW
> > in general.
> 
> I think you got that backwards as inputs are typically rated for some
> maximum voltage based on the supply voltage. 

Yes, but that's done at board design stage.

> That applies also to the
> reset line as I also mentioned above.
> 
> What does the datasheet say?

As mentioned above, datasheet shows reset asserted before the supplies
are being enabled.

> 
> > > > +
> > > > +	ret = ps8830_enable_vregs(retimer);
> > > > +	if (ret)
> > > > +		goto err_clk_disable;
> 
> Johan

Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Johan Hovold]

On Wed, Oct 23, 2024 at 10:32:09AM +0300, Abel Vesa wrote:
> On 24-10-23 09:04:10, Johan Hovold wrote:
> > On Tue, Oct 22, 2024 at 12:01:14PM +0300, Abel Vesa wrote:
> > > On 24-10-15 15:03:15, Johan Hovold wrote:
> > > > On Fri, Oct 04, 2024 at 04:57:38PM +0300, Abel Vesa wrote:
> > 
> > > > > +	ret = ps8830_get_vregs(retimer);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	retimer->xo_clk = devm_clk_get(dev, "xo");
> > > > > +	if (IS_ERR(retimer->xo_clk))
> > > > > +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> > > > > +				     "failed to get xo clock\n");
> > > > > +
> > > > > +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > > > 
> > > > The reset line is active low and should be described as such in DT. So
> > > > here you want to request it as logically low if you want to deassert
> > > > reset.
> > > 
> > > This is being reworked in v3 as we need to support cases where the
> > > retimer has been left enabled and initialized by bootloader and we want
> > > to keep that state until unplug event for the cold-plug orientation
> > > to work properly.
> > > 
> > > On top of that, we don't want to deassert the reset here. We do that
> > > via gpiod_set_value() call below, after the clocks and regulators have
> > > been enabled.
> > 
> > Ok, but you should generally not drive an input high before powering on
> > the device as that can damage the IC (more below).
> 
> This is just not true, generally. Think of top level XTALs which feed in
> clocks (and can't be disabled) before ICs are enabled.

I'm talking about an I/O pin here, you must generally not drive those
high before powering on the IC.

And AFAIU the same applies to clocks even though the risk of damage
there is lower.

> > That is, in this case, you should not deassert reset before making sure
> > the supplies are enabled.
> 
> Wrong. Even the data sheet of this retimer shows in the timigs plot the
> reset as being asserted before the supplies are enabled.

Reset *asserted*, yes (i.e. pull to ground). Not *deasserted* (i.e.
drive high) as you are doing here.

> And generally speaking, the reset needs to be asserted before the
> supplies are up, so that the IC doesn't start doing any work until
> the SW decides it needs to.

Again, the problem is that you are *deasserting* reset before enabling
the supplies.

Johan

Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Abel Vesa]

On 24-10-23 09:52:19, Johan Hovold wrote:
> On Wed, Oct 23, 2024 at 10:32:09AM +0300, Abel Vesa wrote:
> > On 24-10-23 09:04:10, Johan Hovold wrote:
> > > On Tue, Oct 22, 2024 at 12:01:14PM +0300, Abel Vesa wrote:
> > > > On 24-10-15 15:03:15, Johan Hovold wrote:
> > > > > On Fri, Oct 04, 2024 at 04:57:38PM +0300, Abel Vesa wrote:
> > > 
> > > > > > +	ret = ps8830_get_vregs(retimer);
> > > > > > +	if (ret)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	retimer->xo_clk = devm_clk_get(dev, "xo");
> > > > > > +	if (IS_ERR(retimer->xo_clk))
> > > > > > +		return dev_err_probe(dev, PTR_ERR(retimer->xo_clk),
> > > > > > +				     "failed to get xo clock\n");
> > > > > > +
> > > > > > +	retimer->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > > > > 
> > > > > The reset line is active low and should be described as such in DT. So
> > > > > here you want to request it as logically low if you want to deassert
> > > > > reset.
> > > > 
> > > > This is being reworked in v3 as we need to support cases where the
> > > > retimer has been left enabled and initialized by bootloader and we want
> > > > to keep that state until unplug event for the cold-plug orientation
> > > > to work properly.
> > > > 
> > > > On top of that, we don't want to deassert the reset here. We do that
> > > > via gpiod_set_value() call below, after the clocks and regulators have
> > > > been enabled.
> > > 
> > > Ok, but you should generally not drive an input high before powering on
> > > the device as that can damage the IC (more below).
> > 
> > This is just not true, generally. Think of top level XTALs which feed in
> > clocks (and can't be disabled) before ICs are enabled.
> 
> I'm talking about an I/O pin here, you must generally not drive those
> high before powering on the IC.
> 
> And AFAIU the same applies to clocks even though the risk of damage
> there is lower.

As I stated before, enabling (or rather preparing, from kernel's point
of view) will definitely glitch due to PLL switcing (unless the mux is
glitchless from design). And there is literally no risk of enabling or
keeping a clock enabled even if the consumer is powered off.

> 
> > > That is, in this case, you should not deassert reset before making sure
> > > the supplies are enabled.
> > 
> > Wrong. Even the data sheet of this retimer shows in the timigs plot the
> > reset as being asserted before the supplies are enabled.
> 
> Reset *asserted*, yes (i.e. pull to ground). Not *deasserted* (i.e.
> drive high) as you are doing here.

Oh, yeah, that has been fixed in v3 already. Also, this v2 version was
also wrong from active level point of view. Which is also fixed in v3.

> 
> > And generally speaking, the reset needs to be asserted before the
> > supplies are up, so that the IC doesn't start doing any work until
> > the SW decides it needs to.
> 
> Again, the problem is that you are *deasserting* reset before enabling
> the supplies.

Yep.

> 
> Johan

Re: [PATCH v2 2/2] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Johan Hovold]

On Wed, Oct 23, 2024 at 11:04:37AM +0300, Abel Vesa wrote:
> On 24-10-23 09:52:19, Johan Hovold wrote:

> > I'm talking about an I/O pin here, you must generally not drive those
> > high before powering on the IC.
> > 
> > And AFAIU the same applies to clocks even though the risk of damage
> > there is lower.
> 
> As I stated before, enabling (or rather preparing, from kernel's point
> of view) will definitely glitch due to PLL switcing (unless the mux is
> glitchless from design). And there is literally no risk of enabling or
> keeping a clock enabled even if the consumer is powered off.

That's a separate discussion from whether you should supply clocks to an
unpowered IC, and you can get around that by making sure the IC is held
in reset until the clock is stable.

What does the datasheet say about the XTALO_REFCLK_IN pin? What's the
max voltage specified as?

And since the machine we are currently working on do not using a crystal
oscillator here, do you need to configure the device to use a clock
instead somehow?

Is there any mention of the refclk in the power-on sequence?

Johan