Linux-PHY Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v7 10/10] iio: adc: qcom-spmi-iadc: Remove regmap R/W wrapper functions
From: AngeloGioacchino Del Regno @ 2026-01-14  8:39 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, arnd, gregkh, srini, vkoul,
	neil.armstrong, sre, sboyd, angelogioacchino.delregno, krzk,
	dmitry.baryshkov, quic_wcheng, melody.olvera, quic_nsekar,
	ivo.ivanov.ivanov1, abelvesa, luca.weiss, konrad.dybcio,
	mitltlatltl, krishna.kurapati, linux-arm-msm, linux-iio,
	linux-kernel, linux-phy, linux-pm, kernel, Jonathan Cameron
In-Reply-To: <20260114083957.9945-1-angelogioacchino.delregno@collabora.com>

This driver doesn't need to add any register base address to any
regmap call anymore since it was migrated to register as a SPMI
subdevice with its own regmap reg_base, which makes the regmap
API to automatically add such base address internally.

Since the iadc_{read,write,read_result}() functions now only do
call regmap_{read,write,bulk_read}() and nothing else, simplify
the driver by removing them and by calling regmap APIs directly.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iio/adc/qcom-spmi-iadc.c | 83 ++++++++++++--------------------
 1 file changed, 30 insertions(+), 53 deletions(-)

diff --git a/drivers/iio/adc/qcom-spmi-iadc.c b/drivers/iio/adc/qcom-spmi-iadc.c
index 67096952b229..7d46ec2d1a30 100644
--- a/drivers/iio/adc/qcom-spmi-iadc.c
+++ b/drivers/iio/adc/qcom-spmi-iadc.c
@@ -113,77 +113,59 @@ struct iadc_chip {
 	struct completion complete;
 };
 
-static int iadc_read(struct iadc_chip *iadc, u16 offset, u8 *data)
-{
-	unsigned int val;
-	int ret;
-
-	ret = regmap_read(iadc->regmap, offset, &val);
-	if (ret < 0)
-		return ret;
-
-	*data = val;
-	return 0;
-}
-
-static int iadc_write(struct iadc_chip *iadc, u16 offset, u8 data)
-{
-	return regmap_write(iadc->regmap, offset, data);
-}
-
 static int iadc_reset(struct iadc_chip *iadc)
 {
-	u8 data;
+	u32 data;
 	int ret;
 
-	ret = iadc_write(iadc, IADC_SEC_ACCESS, IADC_SEC_ACCESS_DATA);
+	ret = regmap_write(iadc->regmap, IADC_SEC_ACCESS, IADC_SEC_ACCESS_DATA);
 	if (ret < 0)
 		return ret;
 
-	ret = iadc_read(iadc, IADC_PERH_RESET_CTL3, &data);
+	ret = regmap_read(iadc->regmap, IADC_PERH_RESET_CTL3, &data);
 	if (ret < 0)
 		return ret;
 
-	ret = iadc_write(iadc, IADC_SEC_ACCESS, IADC_SEC_ACCESS_DATA);
+	ret = regmap_write(iadc->regmap, IADC_SEC_ACCESS, IADC_SEC_ACCESS_DATA);
 	if (ret < 0)
 		return ret;
 
 	data |= IADC_FOLLOW_WARM_RB;
 
-	return iadc_write(iadc, IADC_PERH_RESET_CTL3, data);
+	return regmap_write(iadc->regmap, IADC_PERH_RESET_CTL3, data);
 }
 
 static int iadc_set_state(struct iadc_chip *iadc, bool state)
 {
-	return iadc_write(iadc, IADC_EN_CTL1, state ? IADC_EN_CTL1_SET : 0);
+	return regmap_write(iadc->regmap, IADC_EN_CTL1, state ? IADC_EN_CTL1_SET : 0);
 }
 
 static void iadc_status_show(struct iadc_chip *iadc)
 {
-	u8 mode, sta1, chan, dig, en, req;
+	u32 mode, sta1, chan, dig, en, req;
 	int ret;
 
-	ret = iadc_read(iadc, IADC_MODE_CTL, &mode);
+	ret = regmap_read(iadc->regmap, IADC_MODE_CTL, &mode);
 	if (ret < 0)
 		return;
 
-	ret = iadc_read(iadc, IADC_DIG_PARAM, &dig);
+	ret = regmap_read(iadc->regmap, IADC_DIG_PARAM, &dig);
 	if (ret < 0)
 		return;
 
-	ret = iadc_read(iadc, IADC_CH_SEL_CTL, &chan);
+	ret = regmap_read(iadc->regmap, IADC_CH_SEL_CTL, &chan);
 	if (ret < 0)
 		return;
 
-	ret = iadc_read(iadc, IADC_CONV_REQ, &req);
+	ret = regmap_read(iadc->regmap, IADC_CONV_REQ, &req);
 	if (ret < 0)
 		return;
 
-	ret = iadc_read(iadc, IADC_STATUS1, &sta1);
+	ret = regmap_read(iadc->regmap, IADC_STATUS1, &sta1);
 	if (ret < 0)
 		return;
 
-	ret = iadc_read(iadc, IADC_EN_CTL1, &en);
+	ret = regmap_read(iadc->regmap, IADC_EN_CTL1, &en);
 	if (ret < 0)
 		return;
 
@@ -199,34 +181,34 @@ static int iadc_configure(struct iadc_chip *iadc, int channel)
 
 	/* Mode selection */
 	mode = (IADC_OP_MODE_NORMAL << IADC_OP_MODE_SHIFT) | IADC_TRIM_EN;
-	ret = iadc_write(iadc, IADC_MODE_CTL, mode);
+	ret = regmap_write(iadc->regmap, IADC_MODE_CTL, mode);
 	if (ret < 0)
 		return ret;
 
 	/* Channel selection */
-	ret = iadc_write(iadc, IADC_CH_SEL_CTL, channel);
+	ret = regmap_write(iadc->regmap, IADC_CH_SEL_CTL, channel);
 	if (ret < 0)
 		return ret;
 
 	/* Digital parameter setup */
 	decim = IADC_DEF_DECIMATION << IADC_DIG_DEC_RATIO_SEL_SHIFT;
-	ret = iadc_write(iadc, IADC_DIG_PARAM, decim);
+	ret = regmap_write(iadc->regmap, IADC_DIG_PARAM, decim);
 	if (ret < 0)
 		return ret;
 
 	/* HW settle time delay */
-	ret = iadc_write(iadc, IADC_HW_SETTLE_DELAY, IADC_DEF_HW_SETTLE_TIME);
+	ret = regmap_write(iadc->regmap, IADC_HW_SETTLE_DELAY, IADC_DEF_HW_SETTLE_TIME);
 	if (ret < 0)
 		return ret;
 
-	ret = iadc_write(iadc, IADC_FAST_AVG_CTL, IADC_DEF_AVG_SAMPLES);
+	ret = regmap_write(iadc->regmap, IADC_FAST_AVG_CTL, IADC_DEF_AVG_SAMPLES);
 	if (ret < 0)
 		return ret;
 
 	if (IADC_DEF_AVG_SAMPLES)
-		ret = iadc_write(iadc, IADC_FAST_AVG_EN, IADC_FAST_AVG_EN_SET);
+		ret = regmap_write(iadc->regmap, IADC_FAST_AVG_EN, IADC_FAST_AVG_EN_SET);
 	else
-		ret = iadc_write(iadc, IADC_FAST_AVG_EN, 0);
+		ret = regmap_write(iadc->regmap, IADC_FAST_AVG_EN, 0);
 
 	if (ret < 0)
 		return ret;
@@ -239,19 +221,19 @@ static int iadc_configure(struct iadc_chip *iadc, int channel)
 		return ret;
 
 	/* Request conversion */
-	return iadc_write(iadc, IADC_CONV_REQ, IADC_CONV_REQ_SET);
+	return regmap_write(iadc->regmap, IADC_CONV_REQ, IADC_CONV_REQ_SET);
 }
 
 static int iadc_poll_wait_eoc(struct iadc_chip *iadc, unsigned int interval_us)
 {
 	unsigned int count, retry;
 	int ret;
-	u8 sta1;
+	u32 sta1;
 
 	retry = interval_us / IADC_CONV_TIME_MIN_US;
 
 	for (count = 0; count < retry; count++) {
-		ret = iadc_read(iadc, IADC_STATUS1, &sta1);
+		ret = regmap_read(iadc->regmap, IADC_STATUS1, &sta1);
 		if (ret < 0)
 			return ret;
 
@@ -267,11 +249,6 @@ static int iadc_poll_wait_eoc(struct iadc_chip *iadc, unsigned int interval_us)
 	return -ETIMEDOUT;
 }
 
-static int iadc_read_result(struct iadc_chip *iadc, u16 *data)
-{
-	return regmap_bulk_read(iadc->regmap, IADC_DATA, data, 2);
-}
-
 static int iadc_do_conversion(struct iadc_chip *iadc, int chan, u16 *data)
 {
 	unsigned int wait;
@@ -296,7 +273,7 @@ static int iadc_do_conversion(struct iadc_chip *iadc, int chan, u16 *data)
 	}
 
 	if (!ret)
-		ret = iadc_read_result(iadc, data);
+		ret = regmap_bulk_read(iadc->regmap, IADC_DATA, data, sizeof(*data));
 exit:
 	iadc_set_state(iadc, false);
 	if (ret < 0)
@@ -392,10 +369,10 @@ static int iadc_update_offset(struct iadc_chip *iadc)
 
 static int iadc_version_check(struct iadc_chip *iadc)
 {
-	u8 val;
+	u32 val;
 	int ret;
 
-	ret = iadc_read(iadc, IADC_PERPH_TYPE, &val);
+	ret = regmap_read(iadc->regmap, IADC_PERPH_TYPE, &val);
 	if (ret < 0)
 		return ret;
 
@@ -404,7 +381,7 @@ static int iadc_version_check(struct iadc_chip *iadc)
 		return -EINVAL;
 	}
 
-	ret = iadc_read(iadc, IADC_PERPH_SUBTYPE, &val);
+	ret = regmap_read(iadc->regmap, IADC_PERPH_SUBTYPE, &val);
 	if (ret < 0)
 		return ret;
 
@@ -413,7 +390,7 @@ static int iadc_version_check(struct iadc_chip *iadc)
 		return -EINVAL;
 	}
 
-	ret = iadc_read(iadc, IADC_REVISION2, &val);
+	ret = regmap_read(iadc->regmap, IADC_REVISION2, &val);
 	if (ret < 0)
 		return ret;
 
@@ -428,7 +405,7 @@ static int iadc_version_check(struct iadc_chip *iadc)
 static int iadc_rsense_read(struct iadc_chip *iadc, struct device_node *node)
 {
 	int ret, sign, int_sense;
-	u8 deviation;
+	u32 deviation;
 
 	ret = of_property_read_u32(node, "qcom,external-resistor-micro-ohms",
 				   &iadc->rsense[IADC_EXT_RSENSE]);
@@ -440,7 +417,7 @@ static int iadc_rsense_read(struct iadc_chip *iadc, struct device_node *node)
 		return -EINVAL;
 	}
 
-	ret = iadc_read(iadc, IADC_NOMINAL_RSENSE, &deviation);
+	ret = regmap_read(iadc->regmap, IADC_NOMINAL_RSENSE, &deviation);
 	if (ret < 0)
 		return ret;
 
-- 
2.52.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH v7 09/10] iio: adc: qcom-spmi-iadc: Migrate to devm_spmi_subdevice_alloc_and_add()
From: AngeloGioacchino Del Regno @ 2026-01-14  8:39 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, arnd, gregkh, srini, vkoul,
	neil.armstrong, sre, sboyd, angelogioacchino.delregno, krzk,
	dmitry.baryshkov, quic_wcheng, melody.olvera, quic_nsekar,
	ivo.ivanov.ivanov1, abelvesa, luca.weiss, konrad.dybcio,
	mitltlatltl, krishna.kurapati, linux-arm-msm, linux-iio,
	linux-kernel, linux-phy, linux-pm, kernel, Jonathan Cameron
In-Reply-To: <20260114083957.9945-1-angelogioacchino.delregno@collabora.com>

Some Qualcomm PMICs integrate an Current ADC device, reachable
in a specific address range over SPMI.

Instead of using the parent SPMI device (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device and
initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.

This allows to stop manually adding the register base address to
every R/W call in this driver, as this can be, and is now, handled
by the regmap API instead.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iio/adc/qcom-spmi-iadc.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/qcom-spmi-iadc.c b/drivers/iio/adc/qcom-spmi-iadc.c
index b64a8a407168..67096952b229 100644
--- a/drivers/iio/adc/qcom-spmi-iadc.c
+++ b/drivers/iio/adc/qcom-spmi-iadc.c
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/spmi.h>
 
 /* IADC register and bit definition */
 #define IADC_REVISION2				0x1
@@ -94,7 +95,6 @@
  * struct iadc_chip - IADC Current ADC device structure.
  * @regmap: regmap for register read/write.
  * @dev: This device pointer.
- * @base: base offset for the ADC peripheral.
  * @rsense: Values of the internal and external sense resister in micro Ohms.
  * @poll_eoc: Poll for end of conversion instead of waiting for IRQ.
  * @offset: Raw offset values for the internal and external channels.
@@ -105,7 +105,6 @@
 struct iadc_chip {
 	struct regmap	*regmap;
 	struct device	*dev;
-	u16		base;
 	bool		poll_eoc;
 	u32		rsense[2];
 	u16		offset[2];
@@ -119,7 +118,7 @@ static int iadc_read(struct iadc_chip *iadc, u16 offset, u8 *data)
 	unsigned int val;
 	int ret;
 
-	ret = regmap_read(iadc->regmap, iadc->base + offset, &val);
+	ret = regmap_read(iadc->regmap, offset, &val);
 	if (ret < 0)
 		return ret;
 
@@ -129,7 +128,7 @@ static int iadc_read(struct iadc_chip *iadc, u16 offset, u8 *data)
 
 static int iadc_write(struct iadc_chip *iadc, u16 offset, u8 data)
 {
-	return regmap_write(iadc->regmap, iadc->base + offset, data);
+	return regmap_write(iadc->regmap, offset, data);
 }
 
 static int iadc_reset(struct iadc_chip *iadc)
@@ -270,7 +269,7 @@ static int iadc_poll_wait_eoc(struct iadc_chip *iadc, unsigned int interval_us)
 
 static int iadc_read_result(struct iadc_chip *iadc, u16 *data)
 {
-	return regmap_bulk_read(iadc->regmap, iadc->base + IADC_DATA, data, 2);
+	return regmap_bulk_read(iadc->regmap, IADC_DATA, data, 2);
 }
 
 static int iadc_do_conversion(struct iadc_chip *iadc, int chan, u16 *data)
@@ -483,12 +482,19 @@ static const struct iio_chan_spec iadc_channels[] = {
 
 static int iadc_probe(struct platform_device *pdev)
 {
+	struct regmap_config iadc_regmap_config = {
+		.reg_bits = 16,
+		.val_bits = 8,
+		.max_register = 0x100,
+		.fast_io = true,
+	};
 	struct device_node *node = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
+	struct spmi_subdevice *sub_sdev;
+	struct spmi_device *sparent;
 	struct iio_dev *indio_dev;
 	struct iadc_chip *iadc;
 	int ret, irq_eoc;
-	u32 res;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*iadc));
 	if (!indio_dev)
@@ -497,18 +503,21 @@ static int iadc_probe(struct platform_device *pdev)
 	iadc = iio_priv(indio_dev);
 	iadc->dev = dev;
 
-	iadc->regmap = dev_get_regmap(dev->parent, NULL);
-	if (!iadc->regmap)
-		return -ENODEV;
+	sparent = to_spmi_device(dev->parent);
+	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
+	if (IS_ERR(sub_sdev))
+		return PTR_ERR(sub_sdev);
 
 	init_completion(&iadc->complete);
 	mutex_init(&iadc->lock);
 
-	ret = of_property_read_u32(node, "reg", &res);
+	ret = of_property_read_u32(node, "reg", &iadc_regmap_config.reg_base);
 	if (ret < 0)
 		return -ENODEV;
 
-	iadc->base = res;
+	iadc->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &iadc_regmap_config);
+	if (IS_ERR(iadc->regmap))
+		return PTR_ERR(iadc->regmap);
 
 	ret = iadc_version_check(iadc);
 	if (ret < 0)
@@ -584,3 +593,4 @@ MODULE_ALIAS("platform:qcom-spmi-iadc");
 MODULE_DESCRIPTION("Qualcomm SPMI PMIC current ADC driver");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Ivan T. Ivanov <iivanov@mm-sol.com>");
+MODULE_IMPORT_NS("SPMI");
-- 
2.52.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH v7 08/10] misc: qcom-coincell: Migrate to devm_spmi_subdevice_alloc_and_add()
From: AngeloGioacchino Del Regno @ 2026-01-14  8:39 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, arnd, gregkh, srini, vkoul,
	neil.armstrong, sre, sboyd, angelogioacchino.delregno, krzk,
	dmitry.baryshkov, quic_wcheng, melody.olvera, quic_nsekar,
	ivo.ivanov.ivanov1, abelvesa, luca.weiss, konrad.dybcio,
	mitltlatltl, krishna.kurapati, linux-arm-msm, linux-iio,
	linux-kernel, linux-phy, linux-pm, kernel
In-Reply-To: <20260114083957.9945-1-angelogioacchino.delregno@collabora.com>

Some Qualcomm PMICs integrate a charger for coincells, usually
powering an RTC when external (or main battery) power is missing.

Instead of using the parent SPMI device (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device and
initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.

This allows to stop manually adding the register base address to
every R/W call in this driver, as this can be, and is now, handled
by the regmap API instead.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/misc/Kconfig         |  2 ++
 drivers/misc/qcom-coincell.c | 38 +++++++++++++++++++++++++-----------
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index d7d41b054b98..bd4fe4ec893c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -290,6 +290,8 @@ config HP_ILO
 config QCOM_COINCELL
 	tristate "Qualcomm coincell charger support"
 	depends on MFD_SPMI_PMIC || COMPILE_TEST
+	depends on SPMI
+	select REGMAP_SPMI
 	help
 	  This driver supports the coincell block found inside of
 	  Qualcomm PMICs.  The coincell charger provides a means to
diff --git a/drivers/misc/qcom-coincell.c b/drivers/misc/qcom-coincell.c
index 3c57f7429147..49e38442b289 100644
--- a/drivers/misc/qcom-coincell.c
+++ b/drivers/misc/qcom-coincell.c
@@ -9,11 +9,11 @@
 #include <linux/of.h>
 #include <linux/regmap.h>
 #include <linux/platform_device.h>
+#include <linux/spmi.h>
 
 struct qcom_coincell {
 	struct device	*dev;
 	struct regmap	*regmap;
-	u32		base_addr;
 };
 
 #define QCOM_COINCELL_REG_RSET		0x44
@@ -35,7 +35,7 @@ static int qcom_coincell_chgr_config(struct qcom_coincell *chgr, int rset,
 	/* if disabling, just do that and skip other operations */
 	if (!enable)
 		return regmap_write(chgr->regmap,
-			  chgr->base_addr + QCOM_COINCELL_REG_ENABLE, 0);
+			  QCOM_COINCELL_REG_ENABLE, 0);
 
 	/* find index for current-limiting resistor */
 	for (i = 0; i < ARRAY_SIZE(qcom_rset_map); i++)
@@ -58,7 +58,7 @@ static int qcom_coincell_chgr_config(struct qcom_coincell *chgr, int rset,
 	}
 
 	rc = regmap_write(chgr->regmap,
-			  chgr->base_addr + QCOM_COINCELL_REG_RSET, i);
+			  QCOM_COINCELL_REG_RSET, i);
 	if (rc) {
 		/*
 		 * This is mainly to flag a bad base_addr (reg) from dts.
@@ -71,19 +71,28 @@ static int qcom_coincell_chgr_config(struct qcom_coincell *chgr, int rset,
 	}
 
 	rc = regmap_write(chgr->regmap,
-		chgr->base_addr + QCOM_COINCELL_REG_VSET, j);
+		QCOM_COINCELL_REG_VSET, j);
 	if (rc)
 		return rc;
 
 	/* set 'enable' register */
 	return regmap_write(chgr->regmap,
-			    chgr->base_addr + QCOM_COINCELL_REG_ENABLE,
+			    QCOM_COINCELL_REG_ENABLE,
 			    QCOM_COINCELL_ENABLE);
 }
 
 static int qcom_coincell_probe(struct platform_device *pdev)
 {
-	struct device_node *node = pdev->dev.of_node;
+	struct regmap_config qcom_coincell_regmap_config = {
+		.reg_bits = 16,
+		.val_bits = 8,
+		.max_register = 0x100,
+		.fast_io = true,
+	};
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct spmi_subdevice *sub_sdev;
+	struct spmi_device *sparent;
 	struct qcom_coincell chgr;
 	u32 rset = 0;
 	u32 vset = 0;
@@ -92,16 +101,22 @@ static int qcom_coincell_probe(struct platform_device *pdev)
 
 	chgr.dev = &pdev->dev;
 
-	chgr.regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	rc = of_property_read_u32(node, "reg", &qcom_coincell_regmap_config.reg_base);
+	if (rc)
+		return rc;
+
+	sparent = to_spmi_device(dev->parent);
+	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
+	if (IS_ERR(sub_sdev))
+		return PTR_ERR(sub_sdev);
+
+	chgr.regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev,
+						&qcom_coincell_regmap_config);
 	if (!chgr.regmap) {
 		dev_err(chgr.dev, "Unable to get regmap\n");
 		return -EINVAL;
 	}
 
-	rc = of_property_read_u32(node, "reg", &chgr.base_addr);
-	if (rc)
-		return rc;
-
 	enable = !of_property_read_bool(node, "qcom,charger-disable");
 
 	if (enable) {
@@ -142,3 +157,4 @@ module_platform_driver(qcom_coincell_driver);
 
 MODULE_DESCRIPTION("Qualcomm PMIC coincell charger driver");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS("SPMI");
-- 
2.52.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH v7 07/10] phy: qualcomm: eusb2-repeater: Migrate to devm_spmi_subdevice_alloc_and_add()
From: AngeloGioacchino Del Regno @ 2026-01-14  8:39 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, arnd, gregkh, srini, vkoul,
	neil.armstrong, sre, sboyd, angelogioacchino.delregno, krzk,
	dmitry.baryshkov, quic_wcheng, melody.olvera, quic_nsekar,
	ivo.ivanov.ivanov1, abelvesa, luca.weiss, konrad.dybcio,
	mitltlatltl, krishna.kurapati, linux-arm-msm, linux-iio,
	linux-kernel, linux-phy, linux-pm, kernel, Abel Vesa
In-Reply-To: <20260114083957.9945-1-angelogioacchino.delregno@collabora.com>

Some Qualcomm PMICs integrate an USB Repeater device, used to
convert between eUSB2 and USB 2.0 signaling levels, reachable
in a specific address range over SPMI.

Instead of using the parent SPMI device (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device for EUSB2
and initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.

This allows to stop manually adding the register base address to
every R/W call in this driver, as this can be, and is now, handled
by the regmap API instead.

Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/qualcomm/Kconfig                  |  2 +
 .../phy/qualcomm/phy-qcom-eusb2-repeater.c    | 55 ++++++++++++-------
 2 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
index 60a0ead127fa..902a788f35f1 100644
--- a/drivers/phy/qualcomm/Kconfig
+++ b/drivers/phy/qualcomm/Kconfig
@@ -128,7 +128,9 @@ config PHY_QCOM_QUSB2
 config PHY_QCOM_EUSB2_REPEATER
 	tristate "Qualcomm PMIC eUSB2 Repeater Driver"
 	depends on OF && (ARCH_QCOM || COMPILE_TEST)
+	depends on SPMI
 	select GENERIC_PHY
+	select REGMAP_SPMI
 	help
 	  Enable support for the USB high-speed eUSB2 repeater on Qualcomm
 	  PMICs. The repeater is paired with a Synopsys or M31 eUSB2 Phy
diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
index efeec4709a15..88e10020f958 100644
--- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
+++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
@@ -9,6 +9,7 @@
 #include <linux/regmap.h>
 #include <linux/of.h>
 #include <linux/phy/phy.h>
+#include <linux/spmi.h>
 
 /* eUSB2 status registers */
 #define EUSB2_RPTR_STATUS		0x08
@@ -66,7 +67,6 @@ struct eusb2_repeater {
 	struct phy *phy;
 	struct regulator_bulk_data *vregs;
 	const struct eusb2_repeater_cfg *cfg;
-	u32 base;
 	enum phy_mode mode;
 };
 
@@ -143,7 +143,6 @@ static int eusb2_repeater_init(struct phy *phy)
 	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
 	struct device_node *np = rptr->dev->of_node;
 	struct regmap *regmap = rptr->regmap;
-	u32 base = rptr->base;
 	u32 poll_val;
 	s32 dt_val;
 	int ret;
@@ -154,37 +153,37 @@ static int eusb2_repeater_init(struct phy *phy)
 	if (ret)
 		return ret;
 
-	regmap_write(regmap, base + EUSB2_EN_CTL1, EUSB2_RPTR_EN);
+	regmap_write(regmap, EUSB2_EN_CTL1, EUSB2_RPTR_EN);
 
 	/* Write registers from init table */
 	for (int i = 0; i < rptr->cfg->init_tbl_num; i++)
-		regmap_write(regmap, base + rptr->cfg->init_tbl[i].reg,
+		regmap_write(regmap, rptr->cfg->init_tbl[i].reg,
 			     rptr->cfg->init_tbl[i].value);
 
 	/* Override registers from devicetree values */
 	if (!of_property_read_u8(np, "qcom,tune-usb2-preem", &val))
-		regmap_write(regmap, base + EUSB2_TUNE_USB2_PREEM, val);
+		regmap_write(regmap, EUSB2_TUNE_USB2_PREEM, val);
 
 	if (!of_property_read_u8(np, "qcom,tune-usb2-disc-thres", &val))
-		regmap_write(regmap, base + EUSB2_TUNE_HSDISC, val);
+		regmap_write(regmap, EUSB2_TUNE_HSDISC, val);
 
 	if (!of_property_read_u8(np, "qcom,tune-usb2-amplitude", &val))
-		regmap_write(regmap, base + EUSB2_TUNE_IUSB2, val);
+		regmap_write(regmap, EUSB2_TUNE_IUSB2, val);
 
 	if (!of_property_read_u8(np, "qcom,tune-res-fsdif", &val))
-		regmap_write(regmap, base + EUSB2_TUNE_RES_FSDIF, val);
+		regmap_write(regmap, EUSB2_TUNE_RES_FSDIF, val);
 
 	if (!of_property_read_s32(np, "qcom,squelch-detector-bp", &dt_val)) {
 		for (i = 0; i < ARRAY_SIZE(squelch_detector); i++) {
 			if (squelch_detector[i] == dt_val) {
-				regmap_write(regmap, base + EUSB2_TUNE_SQUELCH_U, i);
+				regmap_write(regmap, EUSB2_TUNE_SQUELCH_U, i);
 				break;
 			}
 		}
 	}
 
 	/* Wait for status OK */
-	ret = regmap_read_poll_timeout(regmap, base + EUSB2_RPTR_STATUS, poll_val,
+	ret = regmap_read_poll_timeout(regmap, EUSB2_RPTR_STATUS, poll_val,
 				       poll_val & RPTR_OK, 10, 5);
 	if (ret)
 		dev_err(rptr->dev, "initialization timed-out\n");
@@ -197,7 +196,6 @@ static int eusb2_repeater_set_mode(struct phy *phy,
 {
 	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
 	struct regmap *regmap = rptr->regmap;
-	u32 base = rptr->base;
 
 	switch (mode) {
 	case PHY_MODE_USB_HOST:
@@ -206,8 +204,8 @@ static int eusb2_repeater_set_mode(struct phy *phy,
 		 * per eUSB 1.2 Spec. Below implement software workaround until
 		 * PHY and controller is fixing seen observation.
 		 */
-		regmap_write(regmap, base + EUSB2_FORCE_EN_5, F_CLK_19P2M_EN);
-		regmap_write(regmap, base + EUSB2_FORCE_VAL_5, V_CLK_19P2M_EN);
+		regmap_write(regmap, EUSB2_FORCE_EN_5, F_CLK_19P2M_EN);
+		regmap_write(regmap, EUSB2_FORCE_VAL_5, V_CLK_19P2M_EN);
 		break;
 	case PHY_MODE_USB_DEVICE:
 		/*
@@ -216,8 +214,8 @@ static int eusb2_repeater_set_mode(struct phy *phy,
 		 * repeater doesn't clear previous value due to shared
 		 * regulators (say host <-> device mode switch).
 		 */
-		regmap_write(regmap, base + EUSB2_FORCE_EN_5, 0);
-		regmap_write(regmap, base + EUSB2_FORCE_VAL_5, 0);
+		regmap_write(regmap, EUSB2_FORCE_EN_5, 0);
+		regmap_write(regmap, EUSB2_FORCE_VAL_5, 0);
 		break;
 	default:
 		return -EINVAL;
@@ -242,13 +240,23 @@ static const struct phy_ops eusb2_repeater_ops = {
 
 static int eusb2_repeater_probe(struct platform_device *pdev)
 {
+	struct regmap_config eusb2_regmap_config = {
+		.reg_bits = 16,
+		.val_bits = 8,
+		.max_register = 0x100,
+		.fast_io = true,
+	};
+	struct spmi_device *sparent;
 	struct eusb2_repeater *rptr;
+	struct spmi_subdevice *sub_sdev;
 	struct device *dev = &pdev->dev;
 	struct phy_provider *phy_provider;
 	struct device_node *np = dev->of_node;
-	u32 res;
 	int ret;
 
+	if (!dev->parent)
+		return -ENODEV;
+
 	rptr = devm_kzalloc(dev, sizeof(*rptr), GFP_KERNEL);
 	if (!rptr)
 		return -ENOMEM;
@@ -260,15 +268,21 @@ static int eusb2_repeater_probe(struct platform_device *pdev)
 	if (!rptr->cfg)
 		return -EINVAL;
 
-	rptr->regmap = dev_get_regmap(dev->parent, NULL);
-	if (!rptr->regmap)
+	sparent = to_spmi_device(dev->parent);
+	if (!sparent)
 		return -ENODEV;
 
-	ret = of_property_read_u32(np, "reg", &res);
+	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
+	if (IS_ERR(sub_sdev))
+		return PTR_ERR(sub_sdev);
+
+	ret = of_property_read_u32(np, "reg", &eusb2_regmap_config.reg_base);
 	if (ret < 0)
 		return ret;
 
-	rptr->base = res;
+	rptr->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &eusb2_regmap_config);
+	if (IS_ERR(rptr->regmap))
+		return -ENODEV;
 
 	ret = eusb2_repeater_init_vregs(rptr);
 	if (ret < 0) {
@@ -335,3 +349,4 @@ module_platform_driver(eusb2_repeater_driver);
 
 MODULE_DESCRIPTION("Qualcomm PMIC eUSB2 Repeater driver");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("SPMI");
-- 
2.52.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH v7 06/10] power: reset: qcom-pon: Migrate to devm_spmi_subdevice_alloc_and_add()
From: AngeloGioacchino Del Regno @ 2026-01-14  8:39 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, arnd, gregkh, srini, vkoul,
	neil.armstrong, sre, sboyd, angelogioacchino.delregno, krzk,
	dmitry.baryshkov, quic_wcheng, melody.olvera, quic_nsekar,
	ivo.ivanov.ivanov1, abelvesa, luca.weiss, konrad.dybcio,
	mitltlatltl, krishna.kurapati, linux-arm-msm, linux-iio,
	linux-kernel, linux-phy, linux-pm, kernel, Sebastian Reichel
In-Reply-To: <20260114083957.9945-1-angelogioacchino.delregno@collabora.com>

Some Qualcomm PMICs integrates a Power On device supporting pwrkey
and resin along with the Android reboot reason action identifier.

Instead of using the parent SPMI device (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device for PON
and initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.

This allows to stop manually adding the register base address to
every R/W call in this driver, as this can be, and is now, handled
by the regmap API instead.

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/power/reset/qcom-pon.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
index 7e108982a582..0e075a2e5e48 100644
--- a/drivers/power/reset/qcom-pon.c
+++ b/drivers/power/reset/qcom-pon.c
@@ -11,6 +11,7 @@
 #include <linux/reboot.h>
 #include <linux/reboot-mode.h>
 #include <linux/regmap.h>
+#include <linux/spmi.h>
 
 #define PON_SOFT_RB_SPARE		0x8f
 
@@ -22,7 +23,6 @@
 struct qcom_pon {
 	struct device *dev;
 	struct regmap *regmap;
-	u32 baseaddr;
 	struct reboot_mode_driver reboot_mode;
 	long reason_shift;
 };
@@ -35,7 +35,7 @@ static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot,
 	int ret;
 
 	ret = regmap_update_bits(pon->regmap,
-				 pon->baseaddr + PON_SOFT_RB_SPARE,
+				 PON_SOFT_RB_SPARE,
 				 GENMASK(7, pon->reason_shift),
 				 magic << pon->reason_shift);
 	if (ret < 0)
@@ -46,27 +46,42 @@ static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot,
 
 static int qcom_pon_probe(struct platform_device *pdev)
 {
+	struct regmap_config qcom_pon_regmap_config = {
+		.reg_bits = 16,
+		.val_bits = 8,
+		.max_register = 0x100,
+		.fast_io = true,
+	};
+	struct device *dev = &pdev->dev;
+	struct spmi_subdevice *sub_sdev;
+	struct spmi_device *sparent;
 	struct qcom_pon *pon;
 	long reason_shift;
 	int error;
 
+	if (!dev->parent)
+		return -ENODEV;
+
 	pon = devm_kzalloc(&pdev->dev, sizeof(*pon), GFP_KERNEL);
 	if (!pon)
 		return -ENOMEM;
 
 	pon->dev = &pdev->dev;
 
-	pon->regmap = dev_get_regmap(pdev->dev.parent, NULL);
-	if (!pon->regmap) {
-		dev_err(&pdev->dev, "failed to locate regmap\n");
-		return -ENODEV;
-	}
+	sparent = to_spmi_device(dev->parent);
+	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
+	if (IS_ERR(sub_sdev))
+		return PTR_ERR(sub_sdev);
 
-	error = of_property_read_u32(pdev->dev.of_node, "reg",
-				     &pon->baseaddr);
+	error = of_property_read_u32(dev->of_node, "reg",
+				     &qcom_pon_regmap_config.reg_base);
 	if (error)
 		return error;
 
+	pon->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &qcom_pon_regmap_config);
+	if (IS_ERR(pon->regmap))
+		return PTR_ERR(pon->regmap);
+
 	reason_shift = (long)of_device_get_match_data(&pdev->dev);
 
 	if (reason_shift != NO_REASON_SHIFT) {
@@ -106,3 +121,4 @@ module_platform_driver(qcom_pon_driver);
 
 MODULE_DESCRIPTION("Qualcomm Power On driver");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS("SPMI");
-- 
2.52.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH v7 05/10] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()
From: AngeloGioacchino Del Regno @ 2026-01-14  8:39 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, arnd, gregkh, srini, vkoul,
	neil.armstrong, sre, sboyd, angelogioacchino.delregno, krzk,
	dmitry.baryshkov, quic_wcheng, melody.olvera, quic_nsekar,
	ivo.ivanov.ivanov1, abelvesa, luca.weiss, konrad.dybcio,
	mitltlatltl, krishna.kurapati, linux-arm-msm, linux-iio,
	linux-kernel, linux-phy, linux-pm, kernel
In-Reply-To: <20260114083957.9945-1-angelogioacchino.delregno@collabora.com>

Some Qualcomm PMICs integrate a SDAM device, internally located in
a specific address range reachable through SPMI communication.

Instead of using the parent SPMI device (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device for SDAM
and initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.

This allows to stop manually adding the register base address to
every R/W call in this driver, as this can be, and is now, handled
by the regmap API instead.

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/nvmem/Kconfig          |  1 +
 drivers/nvmem/qcom-spmi-sdam.c | 36 +++++++++++++++++++++++-----------
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index bf47a982cf62..5e924c869e77 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -368,6 +368,7 @@ config NVMEM_SNVS_LPGPR
 config NVMEM_SPMI_SDAM
 	tristate "SPMI SDAM Support"
 	depends on SPMI
+	select REGMAP_SPMI
 	help
 	  This driver supports the Shared Direct Access Memory Module on
 	  Qualcomm Technologies, Inc. PMICs. It provides the clients
diff --git a/drivers/nvmem/qcom-spmi-sdam.c b/drivers/nvmem/qcom-spmi-sdam.c
index 4f1cca6eab71..2bb7b3bc497e 100644
--- a/drivers/nvmem/qcom-spmi-sdam.c
+++ b/drivers/nvmem/qcom-spmi-sdam.c
@@ -9,6 +9,7 @@
 #include <linux/nvmem-provider.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/spmi.h>
 
 #define SDAM_MEM_START			0x40
 #define REGISTER_MAP_ID			0x40
@@ -20,7 +21,6 @@
 struct sdam_chip {
 	struct regmap			*regmap;
 	struct nvmem_config		sdam_config;
-	unsigned int			base;
 	unsigned int			size;
 };
 
@@ -73,7 +73,7 @@ static int sdam_read(void *priv, unsigned int offset, void *val,
 		return -EINVAL;
 	}
 
-	rc = regmap_bulk_read(sdam->regmap, sdam->base + offset, val, bytes);
+	rc = regmap_bulk_read(sdam->regmap, offset, val, bytes);
 	if (rc < 0)
 		dev_err(dev, "Failed to read SDAM offset %#x len=%zd, rc=%d\n",
 						offset, bytes, rc);
@@ -100,7 +100,7 @@ static int sdam_write(void *priv, unsigned int offset, void *val,
 		return -EINVAL;
 	}
 
-	rc = regmap_bulk_write(sdam->regmap, sdam->base + offset, val, bytes);
+	rc = regmap_bulk_write(sdam->regmap, offset, val, bytes);
 	if (rc < 0)
 		dev_err(dev, "Failed to write SDAM offset %#x len=%zd, rc=%d\n",
 						offset, bytes, rc);
@@ -110,8 +110,17 @@ static int sdam_write(void *priv, unsigned int offset, void *val,
 
 static int sdam_probe(struct platform_device *pdev)
 {
+	struct regmap_config sdam_regmap_config = {
+		.reg_bits = 16,
+		.val_bits = 8,
+		.max_register = 0x100,
+		.fast_io = true,
+	};
 	struct sdam_chip *sdam;
 	struct nvmem_device *nvmem;
+	struct spmi_device *sparent;
+	struct spmi_subdevice *sub_sdev;
+	struct device *dev = &pdev->dev;
 	unsigned int val;
 	int rc;
 
@@ -119,19 +128,23 @@ static int sdam_probe(struct platform_device *pdev)
 	if (!sdam)
 		return -ENOMEM;
 
-	sdam->regmap = dev_get_regmap(pdev->dev.parent, NULL);
-	if (!sdam->regmap) {
-		dev_err(&pdev->dev, "Failed to get regmap handle\n");
-		return -ENXIO;
-	}
+	sparent = to_spmi_device(dev->parent);
+	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
+	if (IS_ERR(sub_sdev))
+		return PTR_ERR(sub_sdev);
 
-	rc = of_property_read_u32(pdev->dev.of_node, "reg", &sdam->base);
+	rc = of_property_read_u32(dev->of_node, "reg", &sdam_regmap_config.reg_base);
 	if (rc < 0) {
 		dev_err(&pdev->dev, "Failed to get SDAM base, rc=%d\n", rc);
 		return -EINVAL;
 	}
 
-	rc = regmap_read(sdam->regmap, sdam->base + SDAM_SIZE, &val);
+	sdam->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &sdam_regmap_config);
+	if (IS_ERR(sdam->regmap))
+		return dev_err_probe(&pdev->dev, PTR_ERR(sdam->regmap),
+				     "Failed to get regmap handle\n");
+
+	rc = regmap_read(sdam->regmap, SDAM_SIZE, &val);
 	if (rc < 0) {
 		dev_err(&pdev->dev, "Failed to read SDAM_SIZE rc=%d\n", rc);
 		return -EINVAL;
@@ -159,7 +172,7 @@ static int sdam_probe(struct platform_device *pdev)
 	}
 	dev_dbg(&pdev->dev,
 		"SDAM base=%#x size=%u registered successfully\n",
-		sdam->base, sdam->size);
+		sdam_regmap_config.reg_base, sdam->size);
 
 	return 0;
 }
@@ -181,3 +194,4 @@ module_platform_driver(sdam_driver);
 
 MODULE_DESCRIPTION("QCOM SPMI SDAM driver");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS("SPMI");
-- 
2.52.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH v7 04/10] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant
From: AngeloGioacchino Del Regno @ 2026-01-14  8:39 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, arnd, gregkh, srini, vkoul,
	neil.armstrong, sre, sboyd, angelogioacchino.delregno, krzk,
	dmitry.baryshkov, quic_wcheng, melody.olvera, quic_nsekar,
	ivo.ivanov.ivanov1, abelvesa, luca.weiss, konrad.dybcio,
	mitltlatltl, krishna.kurapati, linux-arm-msm, linux-iio,
	linux-kernel, linux-phy, linux-pm, kernel, Jonathan Cameron
In-Reply-To: <20260114083957.9945-1-angelogioacchino.delregno@collabora.com>

Some devices connected over the SPMI bus may be big, in the sense
that those may be a complex of devices managed by a single chip
over the SPMI bus, reachable through a single SID.

Add new functions aimed at managing sub-devices of a SPMI device
spmi_subdevice_alloc_and_add() and a spmi_subdevice_put_and_remove()
for adding a new subdevice and removing it respectively, and also
add their devm_* variants.

The need for such functions comes from the existence of	those
complex Power Management ICs (PMICs), which feature one or many
sub-devices, in some cases with these being even addressable on
the chip in form of SPMI register ranges.

Examples of those devices can be found in both Qualcomm platforms
with their PMICs having PON, RTC, SDAM, GPIO controller, and other
sub-devices, and in newer MediaTek platforms showing similar HW
features and a similar layout with those also having many subdevs.

Also, instead of generally exporting symbols, export them with a
new "SPMI" namespace: all users will have to import this namespace
to make use of the newly introduced exports.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/spmi/spmi-devres.c | 24 ++++++++++++
 drivers/spmi/spmi.c        | 78 ++++++++++++++++++++++++++++++++++++++
 include/linux/spmi.h       | 16 ++++++++
 3 files changed, 118 insertions(+)

diff --git a/drivers/spmi/spmi-devres.c b/drivers/spmi/spmi-devres.c
index 62c4b3f24d06..8feebab0365b 100644
--- a/drivers/spmi/spmi-devres.c
+++ b/drivers/spmi/spmi-devres.c
@@ -60,5 +60,29 @@ int devm_spmi_controller_add(struct device *parent, struct spmi_controller *ctrl
 }
 EXPORT_SYMBOL_GPL(devm_spmi_controller_add);
 
+static void devm_spmi_subdevice_remove(void *res)
+{
+	spmi_subdevice_remove(res);
+}
+
+struct spmi_subdevice *devm_spmi_subdevice_alloc_and_add(struct device *dev,
+							 struct spmi_device *sparent)
+{
+	struct spmi_subdevice *sub_sdev;
+	int ret;
+
+	sub_sdev = spmi_subdevice_alloc_and_add(sparent);
+	if (IS_ERR(sub_sdev))
+		return sub_sdev;
+
+	ret = devm_add_action_or_reset(dev, devm_spmi_subdevice_remove, sub_sdev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return sub_sdev;
+}
+EXPORT_SYMBOL_NS_GPL(devm_spmi_subdevice_alloc_and_add, "SPMI");
+
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("SPMI devres helpers");
+MODULE_IMPORT_NS("SPMI");
diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
index 29567c0620ae..6644c0f065f1 100644
--- a/drivers/spmi/spmi.c
+++ b/drivers/spmi/spmi.c
@@ -19,6 +19,7 @@
 
 static bool is_registered;
 static DEFINE_IDA(ctrl_ida);
+static DEFINE_IDA(spmi_subdevice_ida);
 
 static void spmi_dev_release(struct device *dev)
 {
@@ -31,6 +32,19 @@ static const struct device_type spmi_dev_type = {
 	.release	= spmi_dev_release,
 };
 
+static void spmi_subdev_release(struct device *dev)
+{
+	struct spmi_device *sdev = to_spmi_device(dev);
+	struct spmi_subdevice *sub_sdev = container_of(sdev, struct spmi_subdevice, sdev);
+
+	ida_free(&spmi_subdevice_ida, sub_sdev->devid);
+	kfree(sub_sdev);
+}
+
+static const struct device_type spmi_subdev_type = {
+	.release	= spmi_subdev_release,
+};
+
 static void spmi_ctrl_release(struct device *dev)
 {
 	struct spmi_controller *ctrl = to_spmi_controller(dev);
@@ -87,6 +101,18 @@ void spmi_device_remove(struct spmi_device *sdev)
 }
 EXPORT_SYMBOL_GPL(spmi_device_remove);
 
+/**
+ * spmi_subdevice_remove() - Remove an SPMI subdevice
+ * @sub_sdev:	spmi_device to be removed
+ */
+void spmi_subdevice_remove(struct spmi_subdevice *sub_sdev)
+{
+	struct spmi_device *sdev = &sub_sdev->sdev;
+
+	device_unregister(&sdev->dev);
+}
+EXPORT_SYMBOL_NS_GPL(spmi_subdevice_remove, "SPMI");
+
 static inline int
 spmi_cmd(struct spmi_controller *ctrl, u8 opcode, u8 sid)
 {
@@ -428,6 +454,58 @@ struct spmi_device *spmi_device_alloc(struct spmi_controller *ctrl)
 }
 EXPORT_SYMBOL_GPL(spmi_device_alloc);
 
+/**
+ * spmi_subdevice_alloc_and_add(): Allocate and add a new SPMI sub-device
+ * @sparent:	SPMI parent device with previously registered SPMI controller
+ *
+ * Returns:
+ * Pointer to newly allocated SPMI sub-device for success or negative ERR_PTR.
+ */
+struct spmi_subdevice *spmi_subdevice_alloc_and_add(struct spmi_device *sparent)
+{
+	struct spmi_subdevice *sub_sdev;
+	struct spmi_device *sdev;
+	int ret;
+
+	sub_sdev = kzalloc(sizeof(*sub_sdev), GFP_KERNEL);
+	if (!sub_sdev)
+		return ERR_PTR(-ENOMEM);
+
+	ret = ida_alloc(&spmi_subdevice_ida, GFP_KERNEL);
+	if (ret < 0) {
+		kfree(sub_sdev);
+		return ERR_PTR(ret);
+	}
+
+	sdev = &sub_sdev->sdev;
+	sdev->ctrl = sparent->ctrl;
+	device_initialize(&sdev->dev);
+	sdev->dev.parent = &sparent->dev;
+	sdev->dev.bus = &spmi_bus_type;
+	sdev->dev.type = &spmi_subdev_type;
+
+	sub_sdev->devid = ret;
+	sdev->usid = sparent->usid;
+
+	ret = dev_set_name(&sdev->dev, "%d-%02x.%d.auto",
+			   sdev->ctrl->nr, sdev->usid, sub_sdev->devid);
+	if (ret)
+		goto err_put_dev;
+
+	ret = device_add(&sdev->dev);
+	if (ret) {
+		dev_err(&sdev->dev, "Can't add device, status %pe\n", ERR_PTR(ret));
+		goto err_put_dev;
+	}
+
+	return sub_sdev;
+
+err_put_dev:
+	put_device(&sdev->dev);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_NS_GPL(spmi_subdevice_alloc_and_add, "SPMI");
+
 /**
  * spmi_controller_alloc() - Allocate a new SPMI controller
  * @parent:	parent device
diff --git a/include/linux/spmi.h b/include/linux/spmi.h
index 28e8c8bd3944..7cea0a5b034b 100644
--- a/include/linux/spmi.h
+++ b/include/linux/spmi.h
@@ -69,6 +69,22 @@ int spmi_device_add(struct spmi_device *sdev);
 
 void spmi_device_remove(struct spmi_device *sdev);
 
+/**
+ * struct spmi_subdevice - Basic representation of an SPMI sub-device
+ * @sdev:	Sub-device representation of an SPMI device
+ * @devid:	Platform Device ID of an SPMI sub-device
+ */
+struct spmi_subdevice {
+	struct spmi_device	sdev;
+	unsigned int		devid;
+};
+
+struct spmi_subdevice *spmi_subdevice_alloc_and_add(struct spmi_device *sparent);
+void spmi_subdevice_remove(struct spmi_subdevice *sdev);
+
+struct spmi_subdevice *devm_spmi_subdevice_alloc_and_add(struct device *dev,
+							 struct spmi_device *sparent);
+
 /**
  * struct spmi_controller - interface to the SPMI master controller
  * @dev:	Driver model representation of the device.
-- 
2.52.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH v7 03/10] spmi: Remove unneeded goto in spmi_device_add() error path
From: AngeloGioacchino Del Regno @ 2026-01-14  8:39 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, arnd, gregkh, srini, vkoul,
	neil.armstrong, sre, sboyd, angelogioacchino.delregno, krzk,
	dmitry.baryshkov, quic_wcheng, melody.olvera, quic_nsekar,
	ivo.ivanov.ivanov1, abelvesa, luca.weiss, konrad.dybcio,
	mitltlatltl, krishna.kurapati, linux-arm-msm, linux-iio,
	linux-kernel, linux-phy, linux-pm, kernel
In-Reply-To: <20260114083957.9945-1-angelogioacchino.delregno@collabora.com>

If any error happens during device_add() just return inside of the
conditional, as the goto path doesn't do anything else if not just
returning.

While at it, to improve readability, also change this function to
explicitly return 0 (for success) at the end.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/spmi/spmi.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
index c21789c6b59f..29567c0620ae 100644
--- a/drivers/spmi/spmi.c
+++ b/drivers/spmi/spmi.c
@@ -69,13 +69,11 @@ int spmi_device_add(struct spmi_device *sdev)
 	err = device_add(&sdev->dev);
 	if (err < 0) {
 		dev_err(&sdev->dev, "Can't add device, status %pe\n", ERR_PTR(err));
-		goto err_device_add;
+		return err;
 	}
 
 	dev_dbg(&sdev->dev, "device registered\n");
-
-err_device_add:
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(spmi_device_add);
 
-- 
2.52.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH v7 02/10] spmi: Remove redundant dev_name() print in spmi_device_add()
From: AngeloGioacchino Del Regno @ 2026-01-14  8:39 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, arnd, gregkh, srini, vkoul,
	neil.armstrong, sre, sboyd, angelogioacchino.delregno, krzk,
	dmitry.baryshkov, quic_wcheng, melody.olvera, quic_nsekar,
	ivo.ivanov.ivanov1, abelvesa, luca.weiss, konrad.dybcio,
	mitltlatltl, krishna.kurapati, linux-arm-msm, linux-iio,
	linux-kernel, linux-phy, linux-pm, kernel
In-Reply-To: <20260114083957.9945-1-angelogioacchino.delregno@collabora.com>

Function spmi_device_add() uses dev_{dbg,err}() for respectively
debug and error prints, and passes the same device pointer as both
the dev_{dbg,err}() parameters and to a dev_name() that is part of
the actual message.
This means that the device name gets printed twice!

Remove the redundant dev_name() from the messages.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/spmi/spmi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
index bcb71f25194f..c21789c6b59f 100644
--- a/drivers/spmi/spmi.c
+++ b/drivers/spmi/spmi.c
@@ -68,12 +68,11 @@ int spmi_device_add(struct spmi_device *sdev)
 
 	err = device_add(&sdev->dev);
 	if (err < 0) {
-		dev_err(&sdev->dev, "Can't add %s, status %pe\n",
-			dev_name(&sdev->dev), ERR_PTR(err));
+		dev_err(&sdev->dev, "Can't add device, status %pe\n", ERR_PTR(err));
 		goto err_device_add;
 	}
 
-	dev_dbg(&sdev->dev, "device %s registered\n", dev_name(&sdev->dev));
+	dev_dbg(&sdev->dev, "device registered\n");
 
 err_device_add:
 	return err;
-- 
2.52.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH RESEND v7 00/10] SPMI: Implement sub-devices and migrate drivers
From: AngeloGioacchino Del Regno @ 2026-01-14  8:39 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, arnd, gregkh, srini, vkoul,
	neil.armstrong, sre, sboyd, angelogioacchino.delregno, krzk,
	dmitry.baryshkov, quic_wcheng, melody.olvera, quic_nsekar,
	ivo.ivanov.ivanov1, abelvesa, luca.weiss, konrad.dybcio,
	mitltlatltl, krishna.kurapati, linux-arm-msm, linux-iio,
	linux-kernel, linux-phy, linux-pm, kernel

Changes in v7:
 - Added commit to cleanup redundant dev_name() in the pre-existing
   spmi_device_add() function
 - Added commit removing unneeded goto and improving spmi_device_add()
   readability by returning error in error path, and explicitly zero
   for success at the end.

Changes in v6:
 - Added commit to convert spmi.c to %pe error format and used
   %pe error format in spmi_subdevice code as wanted by Uwe Kleine-Konig

Changes in v5:
 - Changed dev_err to dev_err_probe in qcom-spmi-sdam (and done
   that even though I disagree - because I wanted this series to
   *exclusively* introduce the minimum required changes to
   migrate to the new API, but okay, whatever....!);
 - Added missing REGMAP dependency in Kconfig for qcom-spmi-sdam,
   phy-qcom-eusb2-repeater and qcom-coincell to resolve build
   issues when the already allowed COMPILE_TEST is enabled
   as pointed out by the test robot's randconfig builds.

Changes in v4:
 - Added selection of REGMAP_SPMI in Kconfig for qcom-coincell and
   for phy-qcom-eusb2-repeater to resolve undefined references when
   compiled with some randconfig

Changes in v3:
 - Fixed importing "SPMI" namespace in spmi-devres.c
 - Removed all instances of defensive programming, as pointed out by
   jic23 and Sebastian
 - Removed explicit casting as pointed out by jic23
 - Moved ida_free call to spmi_subdev_release() and simplified error
   handling in spmi_subdevice_alloc_and_add() as pointed out by jic23

Changes in v2:
 - Fixed missing `sparent` initialization in phy-qcom-eusb2-repeater
 - Changed val_bits to 8 in all Qualcomm drivers to ensure
   compatibility as suggested by Casey
 - Added struct device pointer in all conversion commits as suggested
   by Andy
 - Exported newly introduced functions with a new "SPMI" namespace
   and imported the same in all converted drivers as suggested by Andy
 - Added missing error checking for dev_set_name() call in spmi.c
   as suggested by Andy
 - Added comma to last entry of regmap_config as suggested by Andy

While adding support for newer MediaTek platforms, featuring complex
SPMI PMICs, I've seen that those SPMI-connected chips are internally
divided in various IP blocks, reachable in specific contiguous address
ranges... more or less like a MMIO, but over a slow SPMI bus instead.

I recalled that Qualcomm had something similar... and upon checking a
couple of devicetrees, yeah - indeed it's the same over there.

What I've seen then is a common pattern of reading the "reg" property
from devicetree in a struct member and then either
 A. Wrapping regmap_{read/write/etc}() calls in a function that adds
    the register base with "base + ..register", like it's done with
    writel()/readl() calls; or
 B. Doing the same as A. but without wrapper functions.

Even though that works just fine, in my opinion it's wrong.

The regmap API is way more complex than MMIO-only readl()/writel()
functions for multiple reasons (including supporting multiple busses
like SPMI, of course) - but everyone seemed to forget that regmap
can manage register base offsets transparently and automatically in
its API functions by simply adding a `reg_base` to the regmap_config
structure, which is used for initializing a `struct regmap`.

So, here we go: this series implements the software concept of an SPMI
Sub-Device (which, well, also reflects how Qualcomm and MediaTek's
actual hardware is laid out anyway).

               SPMI Controller
                     |                ______
                     |               /       Sub-Device 1
                     V              /
              SPMI Device (PMIC) ----------- Sub-Device 2
                                    \
                                     \______ Sub-Device 3

As per this implementation, an SPMI Sub-Device can be allocated/created
and added in any driver that implements a... well.. subdevice (!) with
an SPMI "main" device as its parent: this allows to create and finally
to correctly configure a regmap that is specific to the sub-device,
operating on its specific address range and reading, and writing, to
its registers with the regmap API taking care of adding the base address
of a sub-device's registers as per regmap API design.

All of the SPMI Sub-Devices are therefore added as children of the SPMI
Device (usually a PMIC), as communication depends on the PMIC's SPMI bus
to be available (and the PMIC to be up and running, of course).

Summarizing the dependency chain (which is obvious to whoever knows what
is going on with Qualcomm and/or MediaTek SPMI PMICs):
    "SPMI Sub-Device x...N" are children "SPMI Device"
    "SPMI Device" is a child of "SPMI Controller"

(that was just another way to say the same thing as the graph above anyway).

Along with the new SPMI Sub-Device registration functions, I have also
performed a conversion of some Qualcomm SPMI drivers and only where the
actual conversion was trivial.

I haven't included any conversion of more complex Qualcomm SPMI drivers
because I don't have the required bandwidth to do so (and besides, I think,
but haven't exactly verified, that some of those require SoCs that I don't
have for testing anyway).

AngeloGioacchino Del Regno (10):
  spmi: Print error status with %pe format
  spmi: Remove redundant dev_name() print in spmi_device_add()
  spmi: Remove unneeded goto in spmi_device_add() error path
  spmi: Implement spmi_subdevice_alloc_and_add() and devm variant
  nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()
  power: reset: qcom-pon: Migrate to devm_spmi_subdevice_alloc_and_add()
  phy: qualcomm: eusb2-repeater: Migrate to
    devm_spmi_subdevice_alloc_and_add()
  misc: qcom-coincell: Migrate to devm_spmi_subdevice_alloc_and_add()
  iio: adc: qcom-spmi-iadc: Migrate to
    devm_spmi_subdevice_alloc_and_add()
  iio: adc: qcom-spmi-iadc: Remove regmap R/W wrapper functions

 drivers/iio/adc/qcom-spmi-iadc.c              | 109 ++++++++----------
 drivers/misc/Kconfig                          |   2 +
 drivers/misc/qcom-coincell.c                  |  38 ++++--
 drivers/nvmem/Kconfig                         |   1 +
 drivers/nvmem/qcom-spmi-sdam.c                |  36 ++++--
 drivers/phy/qualcomm/Kconfig                  |   2 +
 .../phy/qualcomm/phy-qcom-eusb2-repeater.c    |  55 +++++----
 drivers/power/reset/qcom-pon.c                |  34 ++++--
 drivers/spmi/spmi-devres.c                    |  24 ++++
 drivers/spmi/spmi.c                           |  95 +++++++++++++--
 include/linux/spmi.h                          |  16 +++
 11 files changed, 290 insertions(+), 122 deletions(-)

-- 
2.52.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply

* [PATCH v7 01/10] spmi: Print error status with %pe format
From: AngeloGioacchino Del Regno @ 2026-01-14  8:39 UTC (permalink / raw)
  To: jic23
  Cc: dlechner, nuno.sa, andy, arnd, gregkh, srini, vkoul,
	neil.armstrong, sre, sboyd, angelogioacchino.delregno, krzk,
	dmitry.baryshkov, quic_wcheng, melody.olvera, quic_nsekar,
	ivo.ivanov.ivanov1, abelvesa, luca.weiss, konrad.dybcio,
	mitltlatltl, krishna.kurapati, linux-arm-msm, linux-iio,
	linux-kernel, linux-phy, linux-pm, kernel
In-Reply-To: <20260114083957.9945-1-angelogioacchino.delregno@collabora.com>

Instead of printing just a number, use the %pe format for error
status, increasing readability of error prints.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/spmi/spmi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
index 3cf8d9bd4566..bcb71f25194f 100644
--- a/drivers/spmi/spmi.c
+++ b/drivers/spmi/spmi.c
@@ -68,8 +68,8 @@ int spmi_device_add(struct spmi_device *sdev)
 
 	err = device_add(&sdev->dev);
 	if (err < 0) {
-		dev_err(&sdev->dev, "Can't add %s, status %d\n",
-			dev_name(&sdev->dev), err);
+		dev_err(&sdev->dev, "Can't add %s, status %pe\n",
+			dev_name(&sdev->dev), ERR_PTR(err));
 		goto err_device_add;
 	}
 
@@ -494,8 +494,8 @@ static void of_spmi_register_devices(struct spmi_controller *ctrl)
 		err = of_property_read_u32_array(node, "reg", reg, 2);
 		if (err) {
 			dev_err(&ctrl->dev,
-				"node %pOF err (%d) does not have 'reg' property\n",
-				node, err);
+				"node %pOF err (%pe) does not have 'reg' property\n",
+				node, ERR_PTR(err));
 			continue;
 		}
 
@@ -523,7 +523,7 @@ static void of_spmi_register_devices(struct spmi_controller *ctrl)
 		err = spmi_device_add(sdev);
 		if (err) {
 			dev_err(&sdev->dev,
-				"failure adding device. status %d\n", err);
+				"failure adding device. status %pe\n", ERR_PTR(err));
 			spmi_device_put(sdev);
 		}
 	}
-- 
2.52.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH 3/3] MAINTAINERS: Add ASPEED USB3 PHY driver
From: Ryan Chen @ 2026-01-14  7:13 UTC (permalink / raw)
  To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
  Cc: linux-phy, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, Ryan Chen
In-Reply-To: <20260114-upstream_usb3phy-v1-0-2e59590be2d7@aspeedtech.com>

Add maintainer entry for ASPEED USB3 PHY driver.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5b11839cba9d..ad1d12f346f2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3894,6 +3894,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/usb/aspeed,ast2600-udc.yaml
 F:	drivers/usb/gadget/udc/aspeed_udc.c
 
+ASPEED USB3 PHY DRIVER
+M:	Ryan Chen <ryan_chen@aspeedtech.com>
+L:	linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
+L:	linux-phy@lists.infradead.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/phy/aspeed,ast2700-usb3-phy.yaml
+F:	drivers/phy/aspeed/phy-aspeed-usb3.c
+
 ASPEED VIDEO ENGINE DRIVER
 M:	Eddie James <eajames@linux.ibm.com>
 L:	linux-media@vger.kernel.org

-- 
2.34.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH 2/3] phy: add AST2700 usb3.2 phy driver
From: Ryan Chen @ 2026-01-14  7:13 UTC (permalink / raw)
  To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
  Cc: linux-phy, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, Ryan Chen
In-Reply-To: <20260114-upstream_usb3phy-v1-0-2e59590be2d7@aspeedtech.com>

Add AST2700 USB3.2 PHY driver support.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 drivers/phy/aspeed/Kconfig           |  13 ++
 drivers/phy/aspeed/Makefile          |   2 +
 drivers/phy/aspeed/phy-aspeed-usb3.c | 236 +++++++++++++++++++++++++++++++++++
 3 files changed, 251 insertions(+)

diff --git a/drivers/phy/aspeed/Kconfig b/drivers/phy/aspeed/Kconfig
new file mode 100644
index 000000000000..93bb5a913633
--- /dev/null
+++ b/drivers/phy/aspeed/Kconfig
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+#
+# PHY drivers for ASPEED
+#
+
+config PHY_ASPEED_USB3
+	tristate "ASPEED USB3 PHY driver"
+	select GENERIC_PHY
+	depends on ARCH_ASPEED
+	default n
+	help
+	  Enable driver support for Aspeed AST2700 USB3 PHY.
diff --git a/drivers/phy/aspeed/Makefile b/drivers/phy/aspeed/Makefile
new file mode 100644
index 000000000000..20b5ac7b7e64
--- /dev/null
+++ b/drivers/phy/aspeed/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_ASPEED_USB3_PHY)		+= phy-aspeed-usb3.o
diff --git a/drivers/phy/aspeed/phy-aspeed-usb3.c b/drivers/phy/aspeed/phy-aspeed-usb3.c
new file mode 100644
index 000000000000..872d2163fcf5
--- /dev/null
+++ b/drivers/phy/aspeed/phy-aspeed-usb3.c
@@ -0,0 +1,236 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2026 Aspeed Technology Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+#define PHY3S00		0x00
+#define PHY3S00_INIT_DONE		BIT(15)
+#define PHY3S00_SRAM_BYPASS		BIT(7)
+#define PHY3S00_SRAM_EXT_LOAD	BIT(6)
+#define PHY3S04		0x04
+#define PHY3C00		0x08
+#define PHY3C04		0x0C
+#define PHY3P00		0x10
+#define PHY3P00_RX_ADAPT_AFE_EN_G1	BIT(0)
+#define PHY3P00_RX_ADAPT_AFE_EN_G2	BIT(1)
+#define PHY3P00_RX_ADAPT_DFE_EN_G1	BIT(2)
+#define PHY3P00_RX_ADAPT_DFE_EN_G2	BIT(3)
+#define PHY3P00_RX_CDR_VCO_LOWFREQ_G1	BIT(4)
+#define PHY3P00_RX_CDR_VCO_LOWFREQ_G2	BIT(5)
+#define PHY3P00_RX_EQ_AFE_GAIN_G1	GENMASK(9, 6)
+#define PHY3P00_RX_EQ_AFE_GAIN_G2	GENMASK(13, 10)
+#define PHY3P00_RX_EQ_ATT_LVL_G1	GENMASK(16, 14)
+#define PHY3P00_RX_EQ_ATT_LVL_G2	GENMASK(19, 17)
+#define PHY3P00_RX_EQ_CTLE_BOOST_G1	GENMASK(24, 20)
+#define PHY3P00_RX_EQ_CTLE_BOOST_G2	GENMASK(29, 25)
+#define PHY3P00_RX_EQ_DELTA_IQ_G1_LO	GENMASK(31, 30)
+
+#define PHY3P04		0x14
+#define PHY3P04_RX_EQ_DELTA_IQ_G1_HI	GENMASK(1, 0)
+#define PHY3P04_RX_EQ_DELTA_IQ_G2	GENMASK(5, 2)
+#define PHY3P04_RX_EQ_DFE_TAP1_G1	GENMASK(13, 6)
+#define PHY3P04_RX_EQ_DFE_TAP1_G2	GENMASK(21, 14)
+#define PHY3P04_RX_LOS_LFPS_EN		BIT(22)
+#define PHY3P04_RX_LOS_THRESHOLD	GENMASK(25, 23)
+#define PHY3P04_RX_TERM_CTRL		GENMASK(28, 26)
+#define PHY3P04_TX_EQ_MAIN_G1_LO	GENMASK(31, 29)
+
+#define PHY3P08		0x18
+#define PHY3P08_TX_EQ_MAIN_G1_HI	GENMASK(1, 0)
+#define PHY3P08_TX_EQ_MAIN_G2		GENMASK(6, 2)
+#define PHY3P08_TX_EQ_OVRD		BIT(7)
+#define PHY3P08_TX_EQ_POST_G1		GENMASK(12, 9)
+#define PHY3P08_TX_EQ_POST_G2		GENMASK(16, 13)
+#define PHY3P08_TX_EQ_PRE_G1		GENMASK(20, 17)
+#define PHY3P08_TX_EQ_PRE_G2		GENMASK(24, 21)
+#define PHY3P08_TX_IBOOST_LVL		GENMASK(28, 25)
+#define PHY3P08_TX_TERM_CTRL		GENMASK(31, 29)
+
+#define PHY3P0C		0x1C
+#define PHY3P0C_TX_VBOOST_EN		BIT(0)
+
+#define PHY3CMD		0x40
+
+#define PHY3P_RX_EQ_CTLE_BOOST_G1_DEFAULT	0x7
+#define PHY3P_RX_EQ_CTLE_BOOST_G2_DEFAULT	0x7
+#define PHY3P_RX_EQ_DELTA_IQ_G1_DEFAULT	0x3
+#define PHY3P_RX_EQ_DELTA_IQ_G2_DEFAULT	0x5
+#define PHY3P_RX_LOS_THRESHOLD_DEFAULT		0x3
+#define PHY3P_RX_TERM_CTRL_DEFAULT		0x2
+#define PHY3P_TX_EQ_MAIN_G1_DEFAULT		0xa
+#define PHY3P_TX_EQ_MAIN_G2_DEFAULT		0x9
+#define PHY3P_TX_EQ_POST_G1_DEFAULT		0x4
+#define PHY3P_TX_EQ_POST_G2_DEFAULT		0x3
+#define PHY3P_TX_EQ_PRE_G2_DEFAULT		0x2
+#define PHY3P_TX_IBOOST_LVL_DEFAULT		0xf
+#define PHY3P_TX_TERM_CTRL_DEFAULT		0x2
+
+#define PHY3P00_DEFAULT ( \
+	PHY3P00_RX_ADAPT_AFE_EN_G1 | \
+	PHY3P00_RX_ADAPT_AFE_EN_G2 | \
+	PHY3P00_RX_ADAPT_DFE_EN_G1 | \
+	PHY3P00_RX_ADAPT_DFE_EN_G2 | \
+	FIELD_PREP(PHY3P00_RX_EQ_CTLE_BOOST_G1, PHY3P_RX_EQ_CTLE_BOOST_G1_DEFAULT) | \
+	FIELD_PREP(PHY3P00_RX_EQ_CTLE_BOOST_G2, PHY3P_RX_EQ_CTLE_BOOST_G2_DEFAULT) | \
+	FIELD_PREP(PHY3P00_RX_EQ_DELTA_IQ_G1_LO, \
+		   PHY3P_RX_EQ_DELTA_IQ_G1_DEFAULT & 0x3) \
+)
+
+#define PHY3P04_DEFAULT ( \
+	FIELD_PREP(PHY3P04_RX_EQ_DELTA_IQ_G1_HI, \
+		   PHY3P_RX_EQ_DELTA_IQ_G1_DEFAULT >> 2) | \
+	FIELD_PREP(PHY3P04_RX_EQ_DELTA_IQ_G2, PHY3P_RX_EQ_DELTA_IQ_G2_DEFAULT) | \
+	PHY3P04_RX_LOS_LFPS_EN | \
+	FIELD_PREP(PHY3P04_RX_LOS_THRESHOLD, PHY3P_RX_LOS_THRESHOLD_DEFAULT) | \
+	FIELD_PREP(PHY3P04_RX_TERM_CTRL, PHY3P_RX_TERM_CTRL_DEFAULT) | \
+	FIELD_PREP(PHY3P04_TX_EQ_MAIN_G1_LO, \
+		   PHY3P_TX_EQ_MAIN_G1_DEFAULT & 0x7) \
+)
+
+#define PHY3P08_DEFAULT ( \
+	FIELD_PREP(PHY3P08_TX_EQ_MAIN_G1_HI, PHY3P_TX_EQ_MAIN_G1_DEFAULT >> 3) | \
+	FIELD_PREP(PHY3P08_TX_EQ_MAIN_G2, PHY3P_TX_EQ_MAIN_G2_DEFAULT) | \
+	FIELD_PREP(PHY3P08_TX_EQ_POST_G1, PHY3P_TX_EQ_POST_G1_DEFAULT) | \
+	FIELD_PREP(PHY3P08_TX_EQ_POST_G2, PHY3P_TX_EQ_POST_G2_DEFAULT) | \
+	FIELD_PREP(PHY3P08_TX_EQ_PRE_G2, PHY3P_TX_EQ_PRE_G2_DEFAULT) | \
+	FIELD_PREP(PHY3P08_TX_IBOOST_LVL, PHY3P_TX_IBOOST_LVL_DEFAULT) | \
+	FIELD_PREP(PHY3P08_TX_TERM_CTRL, PHY3P_TX_TERM_CTRL_DEFAULT) \
+)
+
+#define PHY3P0C_DEFAULT \
+	PHY3P0C_TX_VBOOST_EN
+
+struct aspeed_usb3_phy {
+	void __iomem *regs;
+	struct reset_control *rst;
+	struct device *dev;
+	struct clk *clk;
+};
+
+static int aspeed_usb3_phy_init(struct phy *phy)
+{
+	struct aspeed_usb3_phy *aspeed_phy = phy_get_drvdata(phy);
+	u32 val;
+	int ret;
+
+	ret = clk_prepare_enable(aspeed_phy->clk);
+	if (ret) {
+		dev_err(aspeed_phy->dev, "Failed to enable clock %d\n", ret);
+		return ret;
+	}
+
+	ret = reset_control_deassert(aspeed_phy->rst);
+	if (ret) {
+		clk_disable_unprepare(aspeed_phy->clk);
+		return ret;
+	}
+
+	/* Wait for USB3 PHY internal SRAM initialization done */
+	ret = readl_poll_timeout(aspeed_phy->regs + PHY3S00, val,
+				 val & PHY3S00_INIT_DONE,
+				 USEC_PER_MSEC, 10 * USEC_PER_MSEC);
+	if (ret) {
+		dev_err(aspeed_phy->dev, "SRAM init timeout\n");
+		goto err_assert_reset;
+	}
+
+	val = readl(aspeed_phy->regs + PHY3S00);
+	val |= PHY3S00_SRAM_BYPASS;
+	writel(val, aspeed_phy->regs + PHY3S00);
+
+	/* Set protocol1_ext signals as default PHY3 settings based on SNPS documents.
+	 * Including PCFGI[54]: protocol1_ext_rx_los_lfps_en for better compatibility
+	 */
+	writel(PHY3P00_DEFAULT, aspeed_phy->regs + PHY3P00);
+	writel(PHY3P04_DEFAULT, aspeed_phy->regs + PHY3P04);
+	writel(PHY3P08_DEFAULT, aspeed_phy->regs + PHY3P08);
+	writel(PHY3P0C_DEFAULT, aspeed_phy->regs + PHY3P0C);
+
+	return 0;
+
+err_assert_reset:
+	reset_control_assert(aspeed_phy->rst);
+	clk_disable_unprepare(aspeed_phy->clk);
+	return ret;
+}
+
+static int aspeed_usb3_phy_exit(struct phy *phy)
+{
+	struct aspeed_usb3_phy *aspeed_phy = phy_get_drvdata(phy);
+
+	reset_control_assert(aspeed_phy->rst);
+	clk_disable_unprepare(aspeed_phy->clk);
+
+	return 0;
+}
+
+static const struct phy_ops aspeed_usb3_phy_ops = {
+	.init		= aspeed_usb3_phy_init,
+	.exit		= aspeed_usb3_phy_exit,
+	.owner		= THIS_MODULE,
+};
+
+static int aspeed_usb3_phy_probe(struct platform_device *pdev)
+{
+	struct aspeed_usb3_phy *aspeed_phy;
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+
+	aspeed_phy = devm_kzalloc(dev, sizeof(*aspeed_phy), GFP_KERNEL);
+	if (!aspeed_phy)
+		return -ENOMEM;
+
+	aspeed_phy->dev = dev;
+
+	aspeed_phy->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(aspeed_phy->clk))
+		return PTR_ERR(aspeed_phy->clk);
+
+	aspeed_phy->rst = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(aspeed_phy->rst))
+		return PTR_ERR(aspeed_phy->rst);
+
+	aspeed_phy->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(aspeed_phy->regs))
+		return PTR_ERR(aspeed_phy->regs);
+
+	phy = devm_phy_create(dev, NULL, &aspeed_usb3_phy_ops);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	phy_set_drvdata(phy, aspeed_phy);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id aspeed_usb3_phy_match_table[] = {
+	{
+		.compatible = "aspeed,ast2700-usb3-phy",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, aspeed_usb3_phy_match_table);
+
+static struct platform_driver aspeed_usb3_phy_driver = {
+	.probe		= aspeed_usb3_phy_probe,
+	.driver		= {
+		.name	= KBUILD_MODNAME,
+		.of_match_table	= aspeed_usb3_phy_match_table,
+	},
+};
+module_platform_driver(aspeed_usb3_phy_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ASPEED USB3.0 PHY Driver");

-- 
2.34.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH 1/3] dt-bindings: phy: aspeed: Document AST2700 USB3.0 PHY
From: Ryan Chen @ 2026-01-14  7:13 UTC (permalink / raw)
  To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
  Cc: linux-phy, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, Ryan Chen
In-Reply-To: <20260114-upstream_usb3phy-v1-0-2e59590be2d7@aspeedtech.com>

Document AST2700 USB3.2 PHY. This IP is connected between
USB3 controller and PHY module.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 .../bindings/phy/aspeed,ast2700-usb3-phy.yaml      | 50 ++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/aspeed,ast2700-usb3-phy.yaml b/Documentation/devicetree/bindings/phy/aspeed,ast2700-usb3-phy.yaml
new file mode 100644
index 000000000000..83da224d99b1
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/aspeed,ast2700-usb3-phy.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/aspeed,ast2700-usb3-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED AST2700 USB 3.2 PHY
+
+maintainers:
+  - Ryan Chen <ryan_chen@aspeedtech.com>
+
+properties:
+  compatible:
+    const: aspeed,ast2700-usb3-phy
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: USB 3.0 PHY clock
+
+  resets:
+    maxItems: 1
+    description: USB 3.0 PHY reset
+
+  '#phy-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+  - '#phy-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/aspeed,ast2700-scu.h>
+    #include <dt-bindings/reset/aspeed,ast2700-scu.h>
+
+    usb-phy@12010000 {
+        compatible = "aspeed,ast2700-usb3-phy";
+        reg = <0x12010000 0xc0>;
+        clocks = <&syscon0 SCU0_CLK_GATE_PORTAUSB2CLK>;
+        resets = <&syscon0 SCU0_RESET_PORTA_PHY3>;
+        #phy-cells = <0>;
+    };

-- 
2.34.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply related

* [PATCH 0/3] Add AST2700 USB3.2 PHY driver
From: Ryan Chen @ 2026-01-14  7:13 UTC (permalink / raw)
  To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
  Cc: linux-phy, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, Ryan Chen

Add AST2700 USB3.2 PHY support.
 - Supports Super Speed Plus Gen2x1 (10 Gbps), Super Speed (5 Gbps),
   High Speed (480 Mbps), Full Speed (12Mbps), and Low Speed (1.5 Mbps).

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
Ryan Chen (3):
      dt-bindings: phy: aspeed: Document AST2700 USB3.0 PHY
      phy: add AST2700 usb3.2 phy driver
      MAINTAINERS: Add ASPEED USB3 PHY driver

 .../bindings/phy/aspeed,ast2700-usb3-phy.yaml      |  50 +++++
 MAINTAINERS                                        |   8 +
 drivers/phy/aspeed/Kconfig                         |  13 ++
 drivers/phy/aspeed/Makefile                        |   2 +
 drivers/phy/aspeed/phy-aspeed-usb3.c               | 236 +++++++++++++++++++++
 5 files changed, 309 insertions(+)
---
base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
change-id: 20260112-upstream_usb3phy-7116f8dfe779

Best regards,
-- 
Ryan Chen <ryan_chen@aspeedtech.com>


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply

* Re: [PATCH] drm/msm/dp: Correct LeMans/Monaco DP phy Swing/Emphasis setting
From: Dmitry Baryshkov @ 2026-01-13 18:55 UTC (permalink / raw)
  To: Yongxing Mou
  Cc: Konrad Dybcio, Vinod Koul, Neil Armstrong, linux-arm-msm,
	linux-phy, linux-kernel
In-Reply-To: <33d57a53-93b4-4ea3-a7bb-0b2015501bad@oss.qualcomm.com>

On Tue, Jan 13, 2026 at 08:04:06PM +0800, Yongxing Mou wrote:
> 
> 
> On 1/9/2026 5:58 PM, Konrad Dybcio wrote:
> > On 1/9/26 9:30 AM, Yongxing Mou wrote:
> > > Currently, the LeMans/Monaco DP PHY operates in DP mode rather than eDP
> > > mode. Per the PHY HPG, the Swing and Emphasis settings have been corrected
> > > to the appropriate DP-mode values.
> > > 
> > > Additionally, the HPG specifies that the LDO value should be set to 0 when
> > > in DP mode. These misconfigurations can lead to link training failures on
> > > certain dongles.
> > > 
> > > Signed-off-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
> > > ---
> > >   drivers/phy/qualcomm/phy-qcom-edp.c | 27 ++++++++++++++++++++++++---
> > >   1 file changed, 24 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
> > > index 13feab99feec..5b0d774bd715 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-edp.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c
> > > @@ -122,6 +122,13 @@ static const u8 dp_swing_hbr_rbr[4][4] = {
> > >   	{ 0x1f, 0xff, 0xff, 0xff }
> > >   };
> > > +static const u8 dp_swing_hbr_rbr_v1[4][4] = {
> > > +	{ 0x07, 0x0f, 0x16, 0x1f },
> > > +	{ 0x11, 0x1e, 0x1f, 0xff },
> > > +	{ 0x16, 0x1f, 0xff, 0xff },
> > > +	{ 0x1f, 0xff, 0xff, 0xff }
> > > +};
> > 
> > For these platforms, I see 4 tables of settings:
> > 
> > (Low/High) Swing/Pre-em for (Low/High) HBR
> > 
> > None of them exactly match your change
> > 
> Emm, this table is in LeMans eDP HPG, here are 6 tables. 4 of them use for
> eDP mode and reset 2 tables used for DP mode. If my understanding is
> incorrect, please correct me. Thanks ~~~ > [...]
> > 
> > > -	ldo_config = edp->is_edp ? 0x0 : 0x1;
> > > +	ldo_config = !edp->is_edp ? 0x0 : 0x1;
> > 
> > You'll notice that this is further wrong, because for eDP, it should be
> > 0x81 at low-swing-high-HBR and 0xc1 at low-swing-low-HBR, and 0 at both
> > cases of high-swing
> > 
> > Please split the LDO change into a separate commit because it touches
> > more platforms
> > 
> > Konrad
> > 
> 
> Yes, you are right, here seems something not correct. i will separate this
> change into single one.Here are some parts I don't fully understand here.
> Could you please point it? How do we know whether it is in low‑swing or
> high‑swing. I didn’t see any logic in the current code that determines this.
> Also, the value in Hamoa seems not same with LeMans,it is 0x51 and 0x91.
> 
> While going through the Hamoa HPG, I noticed a potential issue.
> 
>  static struct qcom_edp_phy_cfg x1e80100_phy_cfg = {
> 	.aux_cfg = edp_phy_aux_cfg_v4,
> 	.vco_div_cfg = edp_phy_vco_div_cfg_v4,
> 	.swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg,...It use
> dp_phy_swing_pre_emph_cfg not edp_phy_swing_pre_emph_cfg, but Hamoa really
> use edp-panel here.. so does this phy cfg correct here?

All PHYs should support eDP and DP modes, so most of the configuration
tables need to be updated/fixed. I tried going through all the tables,
but I never had time to do it in a sane and complete way. As you started
looking into it, would you please review programming for all chipsets
starting from SC8180X?

-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply

* Re: [PATCH 3/5] phy: tegra: xusb: Fix ordering issue when switching roles on USB2 ports
From: Jon Hunter @ 2026-01-13 16:36 UTC (permalink / raw)
  To: Diogo Ivo, Mathias Nyman, Greg Kroah-Hartman, Thierry Reding,
	JC Kuo, Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: linux-usb, linux-tegra, linux-kernel, linux-phy, devicetree
In-Reply-To: <c3f54ffb-cf4b-47ae-871a-6dd60b17c9cb@tecnico.ulisboa.pt>


On 13/01/2026 15:10, Diogo Ivo wrote:

...

>>>>> -    if (tegra->host_mode)
>>>>> -        phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_HOST);
>>>>> -    else
>>>>> -        phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_NONE);
>>>>> +    if (tegra->host_mode) {
>>>>> +        if (!tegra_xusb_usb2_port_wait_role_none(tegra->padctl,
>>>>> +                             tegra->otg_usb2_port))
>>>>> +            return;
>>>>> +        role = USB_ROLE_HOST;
>>>>> +    }
>>>>> +
>>>>> +    mutex_lock(&tegra->lock);
>>>>> +    phy_set_mode_ext(phy, PHY_MODE_USB_OTG, role);
>>>>>       mutex_unlock(&tegra->lock);
>>>>
>>>> I am trying to understand why you opted to implement it this way 
>>>> around and not add the wait loop after setting to the mode to 
>>>> USB_ROLE_NONE in the original code all within the context of the mutex?
>>>
>>> I did that to minimize the amount of time we wait while holding the
>>> mutex, as we can now possibly wait a significant amount of time for the
>>> role switch. Is this an unneccessary optimization?
>>
>> Do you mean it will be longer than a few 100us?
> 
> Currently the worst case in wait_role_none() is around 300us but again
> this is simply because I chose the values with no criteria except that
> in my testing they have worked thus far. Do you have access to any
> internal documentation where the transition length is documented?
> 
> In any case I think that the underlying principle of minimizing the time
> we hold the mutex is solid, no?

Yes, but it was unclear to me if there could be a case where we are 
waiting for USB_ROLE_NONE but we have already transitioned to host. May 
be that will never happen, but it was not clear.

Jon

-- 
nvpublic


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply

* Re: [PATCH 5/5] arm64: tegra: smaug: Add usb-role-switch support
From: Diogo Ivo @ 2026-01-13 15:11 UTC (permalink / raw)
  To: Jon Hunter, Mathias Nyman, Greg Kroah-Hartman, Thierry Reding,
	JC Kuo, Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: linux-usb, linux-tegra, linux-kernel, linux-phy, devicetree
In-Reply-To: <42df7bb1-c587-429f-88ac-1e5ffa7c2b3f@nvidia.com>



On 1/13/26 14:49, Jon Hunter wrote:
> 
> On 13/01/2026 14:20, Diogo Ivo wrote:
>>
>>
>> On 1/12/26 22:03, Jon Hunter wrote:
>>>
>>> On 04/12/2025 21:27, Diogo Ivo wrote:
>>>> The USB2 port on Smaug is configured for OTG operation but lacked the
>>>> required 'usb-role-switch' property, leading to a failed probe and a
>>>> non-functioning USB port. Add the property along with setting the 
>>>> default
>>>> role to host.
>>>>
>>>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
>>>> ---
>>>>   arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/ 
>>>> arm64/boot/dts/nvidia/tegra210-smaug.dts
>>>> index b8d854f90be7..49bf23d6f593 100644
>>>> --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
>>>> +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
>>>> @@ -1809,6 +1809,8 @@ usb2-0 {
>>>>                   status = "okay";
>>>>                   vbus-supply = <&usbc_vbus>;
>>>>                   mode = "otg";
>>>> +                usb-role-switch;
>>>> +                role-switch-default-mode = "host";
>>>>               };
>>>
>>>
>>> This change does add the following warning when building with CHECK_DTBS
>>> ...
>>>
>>> arch/arm64/boot/dts/nvidia/tegra210-smaug.dtb: padctl@7009f000 
>>> (nvidia,tegra210-xusb-padctl): ports:usb2-0: 'role-switch-default- 
>>> mode' does not match any of the regexes: '^pinctrl-[0-9]+$'
>>>      from schema $id: http://devicetree.org/schemas/phy/ 
>>> nvidia,tegra210- xusb-padctl.yaml
>>>
>>> I know that there are many warnings seen for the smaug DTB, but it would
>>> be good to ensure we don't add more.
>>
>> The 'role-switch-default-mode' property is read by the driver to set the
>> initial role for the port [0] and is needed in order for the port to work
>> when booting so in order to fix the warning this property needs to be 
>> added
>> to the binding.
> 
> Correct.
> 
>> As for the other warning ('connector' is a dependency of 'usb-role- 
>> switch')
>> again I think the binding needs to be adjusted since in the Pixel C the
>> connector node should be under the (as for now not present)
>> cros-ec-typec node and the usb2-0 is then modeled as a remote-endpoint
>> for the full connector. I am currently working on fixing the cros-ec- 
>> typec
>> driver and already have a working fix for automatic role switching but
>> in any case I think the binding is what needs changing. If you agree
>> with this then I will add the necessary changes to the DT binding in v2.
> 
> Yes in both cases we need fixes to the DT binding that's all.

Perfect, will do for v2.

Diogo

> Jon
> 

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply

* Re: [PATCH 3/5] phy: tegra: xusb: Fix ordering issue when switching roles on USB2 ports
From: Diogo Ivo @ 2026-01-13 15:10 UTC (permalink / raw)
  To: Jon Hunter, Mathias Nyman, Greg Kroah-Hartman, Thierry Reding,
	JC Kuo, Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: linux-usb, linux-tegra, linux-kernel, linux-phy, devicetree
In-Reply-To: <703a470d-87fc-4580-a743-952e422984d4@nvidia.com>



On 1/13/26 14:48, Jon Hunter wrote:
> 
> On 13/01/2026 14:05, Diogo Ivo wrote:
>>
>>
>> On 1/13/26 11:56, Jon Hunter wrote:
>>>
>>> On 04/12/2025 21:27, Diogo Ivo wrote:
>>>> The current implementation of USB2 role switching on Tegra relies on
>>>> whichever the previous USB controller driver was using the PHY to first
>>>> "yield" it back to USB_ROLE_NONE before the next controller configures
>>>> it for the new role. However, no mechanism to guarantee this ordering
>>>> was implemented, and currently, in the general case, the configuration
>>>> functions tegra_xhci_id_work() and tegra_xudc_usb_role_sw_work() end up
>>>> running in the same order regardless of the transition being HOST- 
>>>> >DEVICE
>>>> or DEVICE->HOST, leading to one of these transitions ending up in a
>>>> non-working state due to the new configuration being clobbered by the
>>>> previous controller driver setting USB_ROLE_NONE after the fact.
>>>>
>>>> Fix this by introducing a helper that waits for the USB2 port’s current
>>>> role to become USB_ROLE_NONE and add it in the configuration functions
>>>> above before setting the role to either USB_ROLE_HOST or
>>>> USB_ROLE_DEVICE. The specific parameters of the helper function are
>>>> choices that seem reasonable in my testing and have no other basis.
>>>
>>> This is no information here about why 6 * 50/60us is deemed to be 
>>> sufficient? May be it is, but a comment would be nice.
>>>
>>>> This was tested on a Tegra210 platform (Smaug). However, due to the 
>>>> similar
>>>> approach in Tegra186 it is likely that not only this problem exists 
>>>> there
>>>> but that this patch also fixes it.
>>>>
>>>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
>>>> ---
>>>>   drivers/phy/tegra/xusb.c            | 23 +++++++++++++++++++++++
>>>>   drivers/usb/gadget/udc/tegra-xudc.c |  4 ++++
>>>>   drivers/usb/host/xhci-tegra.c       | 15 ++++++++++-----
>>>>   include/linux/phy/tegra/xusb.h      |  1 +
>>>>   4 files changed, 38 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
>>>> index c89df95aa6ca..e05c3f2d1421 100644
>>>> --- a/drivers/phy/tegra/xusb.c
>>>> +++ b/drivers/phy/tegra/xusb.c
>>>> @@ -740,6 +740,29 @@ static void 
>>>> tegra_xusb_parse_usb_role_default_mode(struct tegra_xusb_port *port)
>>>>       }
>>>>   }
>>>> +bool tegra_xusb_usb2_port_wait_role_none(struct tegra_xusb_padctl 
>>>> *padctl, int index)
>>>> +{
>>>> +    struct tegra_xusb_usb2_port *usb2 = 
>>>> tegra_xusb_find_usb2_port(padctl,
>>>> +                                      index);
>>>> +    int retries = 5;
>>>> +
>>>> +    if (!usb2) {
>>>> +        dev_err(&usb2->base.dev, "no port found for USB2 lane 
>>>> %u\n", index);
>>>
>>> This appears to be a bug. If !usb2 then dereference usb2->base anyway.
>>
>> It is a bug, will fix in v2.
>>
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    do {
>>>> +        if (usb2->role == USB_ROLE_NONE)
>>>> +            return true;
>>>> +
>>>> +        usleep_range(50, 60);
>>>> +    } while (retries--);
>>>> +
>>>> +    dev_err(&usb2->base.dev, "timed out waiting for USB_ROLE_NONE");
>>>> +
>>>> +    return false;
>>>> +}
>>>> +
>>>>   static int tegra_xusb_usb2_port_parse_dt(struct 
>>>> tegra_xusb_usb2_port *usb2)
>>>>   {
>>>>       struct tegra_xusb_port *port = &usb2->base;
>>>> diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/ 
>>>> gadget/ udc/tegra-xudc.c
>>>> index 0c38fc37b6e6..72d725659e5f 100644
>>>> --- a/drivers/usb/gadget/udc/tegra-xudc.c
>>>> +++ b/drivers/usb/gadget/udc/tegra-xudc.c
>>>> @@ -698,8 +698,12 @@ static void 
>>>> tegra_xudc_restore_port_speed(struct tegra_xudc *xudc)
>>>>   static void tegra_xudc_device_mode_on(struct tegra_xudc *xudc)
>>>>   {
>>>> +    int port = tegra_xusb_padctl_get_port_number(xudc->curr_utmi_phy);
>>>>       int err;
>>>> +    if (!tegra_xusb_usb2_port_wait_role_none(xudc->padctl, port))
>>>> +        return;
>>>> +
>>>>       pm_runtime_get_sync(xudc->dev);
>>>>       tegra_phy_xusb_utmi_pad_power_on(xudc->curr_utmi_phy);
>>>> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci- 
>>>> tegra.c
>>>> index 9c69fccdc6e8..9944593166a3 100644
>>>> --- a/drivers/usb/host/xhci-tegra.c
>>>> +++ b/drivers/usb/host/xhci-tegra.c
>>>> @@ -1352,18 +1352,23 @@ static void tegra_xhci_id_work(struct 
>>>> work_struct *work)
>>>>       struct tegra_xusb_mbox_msg msg;
>>>>       struct phy *phy = tegra_xusb_get_phy(tegra, "usb2",
>>>>                               tegra->otg_usb2_port);
>>>> +    enum usb_role role = USB_ROLE_NONE;
>>>>       u32 status;
>>>>       int ret;
>>>>       dev_dbg(tegra->dev, "host mode %s\n", str_on_off(tegra- 
>>>> >host_mode));
>>>> -    mutex_lock(&tegra->lock);
>>>
>>> Extra blank line here.
>>
>> Will fix in v2.
>>
>>>> -    if (tegra->host_mode)
>>>> -        phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_HOST);
>>>> -    else
>>>> -        phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_NONE);
>>>> +    if (tegra->host_mode) {
>>>> +        if (!tegra_xusb_usb2_port_wait_role_none(tegra->padctl,
>>>> +                             tegra->otg_usb2_port))
>>>> +            return;
>>>> +        role = USB_ROLE_HOST;
>>>> +    }
>>>> +
>>>> +    mutex_lock(&tegra->lock);
>>>> +    phy_set_mode_ext(phy, PHY_MODE_USB_OTG, role);
>>>>       mutex_unlock(&tegra->lock);
>>>
>>> I am trying to understand why you opted to implement it this way 
>>> around and not add the wait loop after setting to the mode to 
>>> USB_ROLE_NONE in the original code all within the context of the mutex?
>>
>> I did that to minimize the amount of time we wait while holding the
>> mutex, as we can now possibly wait a significant amount of time for the
>> role switch. Is this an unneccessary optimization?
> 
> Do you mean it will be longer than a few 100us?

Currently the worst case in wait_role_none() is around 300us but again
this is simply because I chose the values with no criteria except that
in my testing they have worked thus far. Do you have access to any
internal documentation where the transition length is documented?

In any case I think that the underlying principle of minimizing the time
we hold the mutex is solid, no?

Diogo

> Jon
> 

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply

* Re: [PATCH 2/5] phy: tegra: xusb: Fix USB2 port regulator disable logic
From: Diogo Ivo @ 2026-01-13 15:05 UTC (permalink / raw)
  To: Jon Hunter, Mathias Nyman, Greg Kroah-Hartman, Thierry Reding,
	JC Kuo, Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: linux-usb, linux-tegra, linux-kernel, linux-phy, devicetree
In-Reply-To: <043663d0-d592-432b-8550-10669674d17a@nvidia.com>



On 1/13/26 14:42, Jon Hunter wrote:
> 
> On 13/01/2026 13:59, Diogo Ivo wrote:
>>
>>
>> On 1/13/26 12:01, Jon Hunter wrote:
>>>
>>> On 04/12/2025 21:27, Diogo Ivo wrote:
>>>> The USB2 PHY mode handling on Tegra210 incorrectly relied on
>>>> regulator_is_enabled() when determining whether the VBUS supply should
>>>> be disabled during role changes. This is because regulator_is_enabled()
>>>> reports exactly what is states and not if there is an unbalanced number
>>>> of calls between regulator_enable() and regulator_disable(). For
>>>> example, regulator_is_enabled() always reports true on a fixed- 
>>>> regulator
>>>> with no enable gpio, which is the case on the Pixel C.
>>>>
>>>> This then leads to the PHY driver wrongfully calling 
>>>> regulator_disable()
>>>> when transitioning from USB_ROLE_DEVICE to USB_ROLE_NONE since the 
>>>> driver
>>>> did not previously call the corresponding regulator_enable().
>>>>
>>>> Fix this by keeping track of the current role and updating the logic to
>>>> disable the regulator only when the previous role was USB_ROLE_HOST.
>>>>
>>>> While at it fix a small typo in a comment.
>>>>
>>>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
>>>> ---
>>>>   drivers/phy/tegra/xusb-tegra210.c | 5 +++--
>>>>   drivers/phy/tegra/xusb.h          | 1 +
>>>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/ 
>>>> xusb-tegra210.c
>>>> index 3409924498e9..63ad57d95514 100644
>>>> --- a/drivers/phy/tegra/xusb-tegra210.c
>>>> +++ b/drivers/phy/tegra/xusb-tegra210.c
>>>> @@ -1934,9 +1934,9 @@ static int tegra210_usb2_phy_set_mode(struct 
>>>> phy *phy, enum phy_mode mode,
>>>>               /*
>>>>                * When port is peripheral only or role transitions to
>>>>                * USB_ROLE_NONE from USB_ROLE_DEVICE, regulator is not
>>>> -             * be enabled.
>>>> +             * enabled.
>>>>                */
>>>> -            if (regulator_is_enabled(port->supply))
>>>> +            if (port->role == USB_ROLE_HOST)
>>>>                   regulator_disable(port->supply);
>>>>               tegra210_xusb_padctl_id_override(padctl, false);
>>>> @@ -1944,6 +1944,7 @@ static int tegra210_usb2_phy_set_mode(struct 
>>>> phy *phy, enum phy_mode mode,
>>>>           }
>>>>       }
>>>> +    port->role = submode;
>>>>       mutex_unlock(&padctl->lock);
>>>>       return err;
>>>> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
>>>> index d2b5f9565132..273af147dfd3 100644
>>>> --- a/drivers/phy/tegra/xusb.h
>>>> +++ b/drivers/phy/tegra/xusb.h
>>>> @@ -317,6 +317,7 @@ struct tegra_xusb_usb2_port {
>>>>       enum usb_dr_mode mode;
>>>>       bool internal;
>>>>       int usb3_port_fake;
>>>> +    enum usb_role role;
>>>>   };
>>>
>>>
>>> A similar fix was made to the Tegra186 code by commit cefc1caee9dd 
>>> ("phy: tegra: xusb: Fix unbalanced regulator disable in UTMI PHY 
>>> mode"). Although the above looks simpler, I am wondering if we should 
>>> make a similar change to the Tegra210 code so that they both are 
>>> implemented in the same way?
>>
>> Looking at cefc1caee9dd my approach leads to less changes but I do agree
>> that standardization benefits us here. However in that case I think we
>> can take it a step further and actually just have a single function
>> tegra_xusb_padctl_id_override() (and likewise for vbus_override() and
>> set_mode()) since they all seem to do the same thing in both platforms.
> 
> Yes I think that would be fine. I can't say I have looked at that in 
> detail but that would seem like the logical way to go.

Ok, then I'll do just that.

> Jon

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply

* Re: [PATCH 5/5] arm64: tegra: smaug: Add usb-role-switch support
From: Jon Hunter @ 2026-01-13 14:49 UTC (permalink / raw)
  To: Diogo Ivo, Mathias Nyman, Greg Kroah-Hartman, Thierry Reding,
	JC Kuo, Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: linux-usb, linux-tegra, linux-kernel, linux-phy, devicetree
In-Reply-To: <ea4ee289-fb46-44be-b5e6-9d3beca7c0d8@tecnico.ulisboa.pt>


On 13/01/2026 14:20, Diogo Ivo wrote:
> 
> 
> On 1/12/26 22:03, Jon Hunter wrote:
>>
>> On 04/12/2025 21:27, Diogo Ivo wrote:
>>> The USB2 port on Smaug is configured for OTG operation but lacked the
>>> required 'usb-role-switch' property, leading to a failed probe and a
>>> non-functioning USB port. Add the property along with setting the 
>>> default
>>> role to host.
>>>
>>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
>>> ---
>>>   arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/ 
>>> arm64/boot/dts/nvidia/tegra210-smaug.dts
>>> index b8d854f90be7..49bf23d6f593 100644
>>> --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
>>> +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
>>> @@ -1809,6 +1809,8 @@ usb2-0 {
>>>                   status = "okay";
>>>                   vbus-supply = <&usbc_vbus>;
>>>                   mode = "otg";
>>> +                usb-role-switch;
>>> +                role-switch-default-mode = "host";
>>>               };
>>
>>
>> This change does add the following warning when building with CHECK_DTBS
>> ...
>>
>> arch/arm64/boot/dts/nvidia/tegra210-smaug.dtb: padctl@7009f000 
>> (nvidia,tegra210-xusb-padctl): ports:usb2-0: 'role-switch-default- 
>> mode' does not match any of the regexes: '^pinctrl-[0-9]+$'
>>      from schema $id: http://devicetree.org/schemas/phy/ 
>> nvidia,tegra210- xusb-padctl.yaml
>>
>> I know that there are many warnings seen for the smaug DTB, but it would
>> be good to ensure we don't add more.
> 
> The 'role-switch-default-mode' property is read by the driver to set the
> initial role for the port [0] and is needed in order for the port to work
> when booting so in order to fix the warning this property needs to be added
> to the binding.

Correct.

> As for the other warning ('connector' is a dependency of 'usb-role-switch')
> again I think the binding needs to be adjusted since in the Pixel C the
> connector node should be under the (as for now not present)
> cros-ec-typec node and the usb2-0 is then modeled as a remote-endpoint
> for the full connector. I am currently working on fixing the cros-ec-typec
> driver and already have a working fix for automatic role switching but
> in any case I think the binding is what needs changing. If you agree
> with this then I will add the necessary changes to the DT binding in v2.

Yes in both cases we need fixes to the DT binding that's all.

Jon

-- 
nvpublic


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply

* Re: [PATCH 3/5] phy: tegra: xusb: Fix ordering issue when switching roles on USB2 ports
From: Jon Hunter @ 2026-01-13 14:48 UTC (permalink / raw)
  To: Diogo Ivo, Mathias Nyman, Greg Kroah-Hartman, Thierry Reding,
	JC Kuo, Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: linux-usb, linux-tegra, linux-kernel, linux-phy, devicetree
In-Reply-To: <ae36f759-e889-4371-8c08-b8ffd1b69250@tecnico.ulisboa.pt>


On 13/01/2026 14:05, Diogo Ivo wrote:
> 
> 
> On 1/13/26 11:56, Jon Hunter wrote:
>>
>> On 04/12/2025 21:27, Diogo Ivo wrote:
>>> The current implementation of USB2 role switching on Tegra relies on
>>> whichever the previous USB controller driver was using the PHY to first
>>> "yield" it back to USB_ROLE_NONE before the next controller configures
>>> it for the new role. However, no mechanism to guarantee this ordering
>>> was implemented, and currently, in the general case, the configuration
>>> functions tegra_xhci_id_work() and tegra_xudc_usb_role_sw_work() end up
>>> running in the same order regardless of the transition being HOST- 
>>> >DEVICE
>>> or DEVICE->HOST, leading to one of these transitions ending up in a
>>> non-working state due to the new configuration being clobbered by the
>>> previous controller driver setting USB_ROLE_NONE after the fact.
>>>
>>> Fix this by introducing a helper that waits for the USB2 port’s current
>>> role to become USB_ROLE_NONE and add it in the configuration functions
>>> above before setting the role to either USB_ROLE_HOST or
>>> USB_ROLE_DEVICE. The specific parameters of the helper function are
>>> choices that seem reasonable in my testing and have no other basis.
>>
>> This is no information here about why 6 * 50/60us is deemed to be 
>> sufficient? May be it is, but a comment would be nice.
>>
>>> This was tested on a Tegra210 platform (Smaug). However, due to the 
>>> similar
>>> approach in Tegra186 it is likely that not only this problem exists 
>>> there
>>> but that this patch also fixes it.
>>>
>>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
>>> ---
>>>   drivers/phy/tegra/xusb.c            | 23 +++++++++++++++++++++++
>>>   drivers/usb/gadget/udc/tegra-xudc.c |  4 ++++
>>>   drivers/usb/host/xhci-tegra.c       | 15 ++++++++++-----
>>>   include/linux/phy/tegra/xusb.h      |  1 +
>>>   4 files changed, 38 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
>>> index c89df95aa6ca..e05c3f2d1421 100644
>>> --- a/drivers/phy/tegra/xusb.c
>>> +++ b/drivers/phy/tegra/xusb.c
>>> @@ -740,6 +740,29 @@ static void 
>>> tegra_xusb_parse_usb_role_default_mode(struct tegra_xusb_port *port)
>>>       }
>>>   }
>>> +bool tegra_xusb_usb2_port_wait_role_none(struct tegra_xusb_padctl 
>>> *padctl, int index)
>>> +{
>>> +    struct tegra_xusb_usb2_port *usb2 = 
>>> tegra_xusb_find_usb2_port(padctl,
>>> +                                      index);
>>> +    int retries = 5;
>>> +
>>> +    if (!usb2) {
>>> +        dev_err(&usb2->base.dev, "no port found for USB2 lane %u\n", 
>>> index);
>>
>> This appears to be a bug. If !usb2 then dereference usb2->base anyway.
> 
> It is a bug, will fix in v2.
> 
>>> +        return false;
>>> +    }
>>> +
>>> +    do {
>>> +        if (usb2->role == USB_ROLE_NONE)
>>> +            return true;
>>> +
>>> +        usleep_range(50, 60);
>>> +    } while (retries--);
>>> +
>>> +    dev_err(&usb2->base.dev, "timed out waiting for USB_ROLE_NONE");
>>> +
>>> +    return false;
>>> +}
>>> +
>>>   static int tegra_xusb_usb2_port_parse_dt(struct 
>>> tegra_xusb_usb2_port *usb2)
>>>   {
>>>       struct tegra_xusb_port *port = &usb2->base;
>>> diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/ 
>>> gadget/ udc/tegra-xudc.c
>>> index 0c38fc37b6e6..72d725659e5f 100644
>>> --- a/drivers/usb/gadget/udc/tegra-xudc.c
>>> +++ b/drivers/usb/gadget/udc/tegra-xudc.c
>>> @@ -698,8 +698,12 @@ static void tegra_xudc_restore_port_speed(struct 
>>> tegra_xudc *xudc)
>>>   static void tegra_xudc_device_mode_on(struct tegra_xudc *xudc)
>>>   {
>>> +    int port = tegra_xusb_padctl_get_port_number(xudc->curr_utmi_phy);
>>>       int err;
>>> +    if (!tegra_xusb_usb2_port_wait_role_none(xudc->padctl, port))
>>> +        return;
>>> +
>>>       pm_runtime_get_sync(xudc->dev);
>>>       tegra_phy_xusb_utmi_pad_power_on(xudc->curr_utmi_phy);
>>> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci- 
>>> tegra.c
>>> index 9c69fccdc6e8..9944593166a3 100644
>>> --- a/drivers/usb/host/xhci-tegra.c
>>> +++ b/drivers/usb/host/xhci-tegra.c
>>> @@ -1352,18 +1352,23 @@ static void tegra_xhci_id_work(struct 
>>> work_struct *work)
>>>       struct tegra_xusb_mbox_msg msg;
>>>       struct phy *phy = tegra_xusb_get_phy(tegra, "usb2",
>>>                               tegra->otg_usb2_port);
>>> +    enum usb_role role = USB_ROLE_NONE;
>>>       u32 status;
>>>       int ret;
>>>       dev_dbg(tegra->dev, "host mode %s\n", str_on_off(tegra- 
>>> >host_mode));
>>> -    mutex_lock(&tegra->lock);
>>
>> Extra blank line here.
> 
> Will fix in v2.
> 
>>> -    if (tegra->host_mode)
>>> -        phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_HOST);
>>> -    else
>>> -        phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_NONE);
>>> +    if (tegra->host_mode) {
>>> +        if (!tegra_xusb_usb2_port_wait_role_none(tegra->padctl,
>>> +                             tegra->otg_usb2_port))
>>> +            return;
>>> +        role = USB_ROLE_HOST;
>>> +    }
>>> +
>>> +    mutex_lock(&tegra->lock);
>>> +    phy_set_mode_ext(phy, PHY_MODE_USB_OTG, role);
>>>       mutex_unlock(&tegra->lock);
>>
>> I am trying to understand why you opted to implement it this way 
>> around and not add the wait loop after setting to the mode to 
>> USB_ROLE_NONE in the original code all within the context of the mutex?
> 
> I did that to minimize the amount of time we wait while holding the
> mutex, as we can now possibly wait a significant amount of time for the
> role switch. Is this an unneccessary optimization?

Do you mean it will be longer than a few 100us?

Jon

-- 
nvpublic


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply

* Re: [PATCH 2/5] phy: tegra: xusb: Fix USB2 port regulator disable logic
From: Jon Hunter @ 2026-01-13 14:42 UTC (permalink / raw)
  To: Diogo Ivo, Mathias Nyman, Greg Kroah-Hartman, Thierry Reding,
	JC Kuo, Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: linux-usb, linux-tegra, linux-kernel, linux-phy, devicetree
In-Reply-To: <54afff11-df9b-4c25-bd1d-8134196ce093@tecnico.ulisboa.pt>


On 13/01/2026 13:59, Diogo Ivo wrote:
> 
> 
> On 1/13/26 12:01, Jon Hunter wrote:
>>
>> On 04/12/2025 21:27, Diogo Ivo wrote:
>>> The USB2 PHY mode handling on Tegra210 incorrectly relied on
>>> regulator_is_enabled() when determining whether the VBUS supply should
>>> be disabled during role changes. This is because regulator_is_enabled()
>>> reports exactly what is states and not if there is an unbalanced number
>>> of calls between regulator_enable() and regulator_disable(). For
>>> example, regulator_is_enabled() always reports true on a fixed-regulator
>>> with no enable gpio, which is the case on the Pixel C.
>>>
>>> This then leads to the PHY driver wrongfully calling regulator_disable()
>>> when transitioning from USB_ROLE_DEVICE to USB_ROLE_NONE since the 
>>> driver
>>> did not previously call the corresponding regulator_enable().
>>>
>>> Fix this by keeping track of the current role and updating the logic to
>>> disable the regulator only when the previous role was USB_ROLE_HOST.
>>>
>>> While at it fix a small typo in a comment.
>>>
>>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
>>> ---
>>>   drivers/phy/tegra/xusb-tegra210.c | 5 +++--
>>>   drivers/phy/tegra/xusb.h          | 1 +
>>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/phy/tegra/xusb-tegra210.c b/drivers/phy/tegra/ 
>>> xusb-tegra210.c
>>> index 3409924498e9..63ad57d95514 100644
>>> --- a/drivers/phy/tegra/xusb-tegra210.c
>>> +++ b/drivers/phy/tegra/xusb-tegra210.c
>>> @@ -1934,9 +1934,9 @@ static int tegra210_usb2_phy_set_mode(struct 
>>> phy *phy, enum phy_mode mode,
>>>               /*
>>>                * When port is peripheral only or role transitions to
>>>                * USB_ROLE_NONE from USB_ROLE_DEVICE, regulator is not
>>> -             * be enabled.
>>> +             * enabled.
>>>                */
>>> -            if (regulator_is_enabled(port->supply))
>>> +            if (port->role == USB_ROLE_HOST)
>>>                   regulator_disable(port->supply);
>>>               tegra210_xusb_padctl_id_override(padctl, false);
>>> @@ -1944,6 +1944,7 @@ static int tegra210_usb2_phy_set_mode(struct 
>>> phy *phy, enum phy_mode mode,
>>>           }
>>>       }
>>> +    port->role = submode;
>>>       mutex_unlock(&padctl->lock);
>>>       return err;
>>> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
>>> index d2b5f9565132..273af147dfd3 100644
>>> --- a/drivers/phy/tegra/xusb.h
>>> +++ b/drivers/phy/tegra/xusb.h
>>> @@ -317,6 +317,7 @@ struct tegra_xusb_usb2_port {
>>>       enum usb_dr_mode mode;
>>>       bool internal;
>>>       int usb3_port_fake;
>>> +    enum usb_role role;
>>>   };
>>
>>
>> A similar fix was made to the Tegra186 code by commit cefc1caee9dd 
>> ("phy: tegra: xusb: Fix unbalanced regulator disable in UTMI PHY 
>> mode"). Although the above looks simpler, I am wondering if we should 
>> make a similar change to the Tegra210 code so that they both are 
>> implemented in the same way?
> 
> Looking at cefc1caee9dd my approach leads to less changes but I do agree
> that standardization benefits us here. However in that case I think we
> can take it a step further and actually just have a single function
> tegra_xusb_padctl_id_override() (and likewise for vbus_override() and
> set_mode()) since they all seem to do the same thing in both platforms.

Yes I think that would be fine. I can't say I have looked at that in 
detail but that would seem like the logical way to go.

Jon

-- 
nvpublic


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply

* Re: [PATCH 1/5] PCI: dw-rockchip: Add phy_calibrate() to check PHY lock status
From: Manivannan Sadhasivam @ 2026-01-13 14:34 UTC (permalink / raw)
  To: Shawn Lin
  Cc: Bjorn Helgaas, Vinod Koul, linux-pci, linux-rockchip, linux-phy,
	Heiko Stuebner, Neil Armstrong, Sebastian Reichel
In-Reply-To: <1766560210-100883-2-git-send-email-shawn.lin@rock-chips.com>

On Wed, Dec 24, 2025 at 03:10:06PM +0800, Shawn Lin wrote:
> Current we keep controller in reset state when initializing PHY which
> is the right thing to do. But this case, the PHY is also reset because
> it refers to a signal from controller. Now we check PHY lock status
> inside .phy_init() callback which may be bogus for certain type of PHY,
> because of the fact above. Add phy_calibrate() to better check PHY lock
> status if provided.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

I guess this patch can get merged separately without the PHY patches and not
cause any functional issue. But I'd like the PHY patches to get reviewed so that
we know that the API usage is correct.

- Mani

> ---
> 
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index f8605fe..75d6306 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -705,6 +705,12 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto deinit_phy;
>  
> +	ret = phy_calibrate(rockchip->phy);
> +	if (ret) {
> +		dev_err(dev, "phy lock failed\n");
> +		goto assert_controller;
> +	}
> +
>  	ret = rockchip_pcie_clk_init(rockchip);
>  	if (ret)
>  		goto deinit_phy;
> @@ -727,7 +733,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  	}
>  
>  	return 0;
> -
> +assert_controller:
> +	reset_control_assert(rockchip->rst);
>  deinit_clk:
>  	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
>  deinit_phy:
> -- 
> 2.7.4
> 

-- 
மணிவண்ணன் சதாசிவம்

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply

* Re: [PATCH 5/5] arm64: tegra: smaug: Add usb-role-switch support
From: Diogo Ivo @ 2026-01-13 14:20 UTC (permalink / raw)
  To: Jon Hunter, Mathias Nyman, Greg Kroah-Hartman, Thierry Reding,
	JC Kuo, Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley
  Cc: linux-usb, linux-tegra, linux-kernel, linux-phy, devicetree
In-Reply-To: <ea60f024-1f39-483c-87e3-36624bd58d49@nvidia.com>



On 1/12/26 22:03, Jon Hunter wrote:
> 
> On 04/12/2025 21:27, Diogo Ivo wrote:
>> The USB2 port on Smaug is configured for OTG operation but lacked the
>> required 'usb-role-switch' property, leading to a failed probe and a
>> non-functioning USB port. Add the property along with setting the default
>> role to host.
>>
>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
>> ---
>>   arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/ 
>> arm64/boot/dts/nvidia/tegra210-smaug.dts
>> index b8d854f90be7..49bf23d6f593 100644
>> --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
>> +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
>> @@ -1809,6 +1809,8 @@ usb2-0 {
>>                   status = "okay";
>>                   vbus-supply = <&usbc_vbus>;
>>                   mode = "otg";
>> +                usb-role-switch;
>> +                role-switch-default-mode = "host";
>>               };
> 
> 
> This change does add the following warning when building with CHECK_DTBS
> ...
> 
> arch/arm64/boot/dts/nvidia/tegra210-smaug.dtb: padctl@7009f000 
> (nvidia,tegra210-xusb-padctl): ports:usb2-0: 'role-switch-default-mode' 
> does not match any of the regexes: '^pinctrl-[0-9]+$'
>      from schema $id: http://devicetree.org/schemas/phy/nvidia,tegra210- 
> xusb-padctl.yaml
> 
> I know that there are many warnings seen for the smaug DTB, but it would
> be good to ensure we don't add more.

The 'role-switch-default-mode' property is read by the driver to set the
initial role for the port [0] and is needed in order for the port to work
when booting so in order to fix the warning this property needs to be added
to the binding.

As for the other warning ('connector' is a dependency of 'usb-role-switch')
again I think the binding needs to be adjusted since in the Pixel C the
connector node should be under the (as for now not present)
cros-ec-typec node and the usb2-0 is then modeled as a remote-endpoint
for the full connector. I am currently working on fixing the cros-ec-typec
driver and already have a working fix for automatic role switching but
in any case I think the binding is what needs changing. If you agree
with this then I will add the necessary changes to the DT binding in v2.

Thanks,
Diogo

[0]: 
https://elixir.bootlin.com/linux/v6.18.4/source/drivers/phy/tegra/xusb.c#L730

> Cheers
> Jon

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox