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

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;
> +}

...

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;
> > +}
> 
> ...

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

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

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

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

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

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

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

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

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