[PATCH 0/6] PCI/pwrctrl: tc9563: Several fixes and improvements

[PATCH 0/6] PCI/pwrctrl: tc9563: Several fixes and improvements

[Manivannan Sadhasivam via B4 Relay]

Hi,

This series contains several fixes and improvements for the TC9563 Pwrctrl
driver and the switch binding. Most of the fixes are related to ensuring
correctness in parsing the DT properties for the switch ports and the
integrated Endpoint.

More details are in the individual patches.

Testing
=======

This series was validated on the RB3Gen2 board.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
Manivannan Sadhasivam (6):
      dt-bindings: PCI: toshiba,tc9563: Restrict Tx Amplitude, DFE and N_FTS to USP, DSP1 and DSP2
      PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node
      PCI/pwrctrl: tc9563: Power off only the external ports in tc9563_pwrctrl_disable_port()
      PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3
      PCI/pwrctrl: tc9563: Rename DSP3 to VDSP
      PCI/pwrctrl: tc9563: Move Integrated MAC Endpoint out of 'tc9563_pwrctrl_ports' enum

 .../devicetree/bindings/pci/toshiba,tc9563.yaml    |  21 +++-
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c           | 129 ++++++++++++++-------
 2 files changed, 104 insertions(+), 46 deletions(-)
---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260725-tc9563-fix-64eb7cbb1229

Best regards,
--  
மணிவண்ணன் சதாசிவம்

[PATCH 1/6] dt-bindings: PCI: toshiba,tc9563: Restrict Tx Amplitude, DFE and N_FTS to USP, DSP1 and DSP2

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

TC9563 switch allows configuring Tx Amplitude, DFE and N_FTS settings only
to USP, DSP1 and DSP2 ports. These settings don't apply to DSP3 and also
the integrated Ethernet MAC Endpoint.

So restrict these properties and also move the
'toshiba,tx-amplitude-microvolt' property to DSP2 in the example to conform
to the binding.

Fixes: 26f3ac884835 ("dt-bindings: PCI: Add binding for Toshiba TC9563 PCIe switch")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 .../devicetree/bindings/pci/toshiba,tc9563.yaml     | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml b/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
index b3ad05d90201..f084830c6d0c 100644
--- a/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
+++ b/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
@@ -54,7 +54,7 @@ properties:
           - description: I2C slave address
 
 patternProperties:
-  "^pcie@[1-3],0$":
+  "^pcie@[1-2],0$":
     description:
       child nodes describing the internal downstream ports of
       the tc9563 switch.
@@ -64,6 +64,21 @@ patternProperties:
       - $ref: /schemas/pci/pci-pci-bridge.yaml#
     unevaluatedProperties: false
 
+  "^pcie@[3],0$":
+    description:
+      child node describing the internal downstream port connected to the
+      integrated Ethernet MAC endpoint of the tc9563 switch.
+    type: object
+    $ref: /schemas/pci/pci-pci-bridge.yaml#
+    properties:
+      n-fts: false
+    patternProperties:
+      "^ethernet@0,[0-1]$":
+        type: object
+        properties:
+          n-fts: false
+    unevaluatedProperties: false
+
 $defs:
   tc9563-node:
     type: object
@@ -154,6 +169,8 @@ examples:
                     device_type = "pci";
                     ranges;
                     bus-range = <0x04 0xff>;
+
+                    toshiba,tx-amplitude-microvolt = <10>;
                 };
 
                 pcie@3,0 {
@@ -165,8 +182,6 @@ examples:
                     ranges;
                     bus-range = <0x05 0xff>;
 
-                    toshiba,tx-amplitude-microvolt = <10>;
-
                     ethernet@0,0 {
                         reg = <0x50000 0x0 0x0 0x0 0x0>;
                     };

-- 
2.43.0

[PATCH 2/6] PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

DSP3 has an integrated Ethernet MAC Endpoint which has its own set of
config registers for configuring settings such as ASPM. The Endpoint
device has 2 physical functions and those 2 functions share the same
settings.

Hence, parse the Endpoint node under DSP3 instead of parsing both the
functions. The existing parsing logic also has one OOB issue as parsing
both functions will result in accessing past the tc9563_pwrctrl->cfg array.

Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index 488e1ec34a7f..482e82e433dd 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
@@ -596,12 +596,17 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 		ret = tc9563_pwrctrl_parse_device_dt(tc9563, child, port);
 		if (ret)
 			break;
-		/* Embedded ethernet device are under DSP3 */
+
+		/*
+		 * The integrated Ethernet MAC Endpoint under DSP3 is a single
+		 * device whose functions share the same config registers.
+		 */
 		if (port == TC9563_DSP3) {
-			for_each_child_of_node_scoped(child, child1) {
-				port++;
+			struct device_node *eth __free(device_node) =
+					of_get_next_available_child(child, NULL);
+			if (eth) {
 				ret = tc9563_pwrctrl_parse_device_dt(tc9563,
-								child1, port);
+								eth, TC9563_ETHERNET);
 				if (ret)
 					break;
 			}

-- 
2.43.0

[PATCH 3/6] PCI/pwrctrl: tc9563: Power off only the external ports in tc9563_pwrctrl_disable_port()

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

TC9563 supports powering off only the external facing ports like DSP1 and
DSP2. It is not recommended to power off USP and DSP3 as they have fixed
ports/endpoint connected.

Hence, fix tc9563_pwrctrl_disable_port() to power off only DSP1 and DSP2.

Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index 482e82e433dd..ca19bf60d290 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
@@ -241,12 +241,18 @@ static int tc9563_pwrctrl_disable_port(struct tc9563_pwrctrl *tc9563,
 	if (!cfg->disable_port)
 		return 0;
 
-	if (port == TC9563_DSP1) {
+	switch (port) {
+	case TC9563_DSP1:
 		seq = dsp1_pwroff_seq;
 		len = ARRAY_SIZE(dsp1_pwroff_seq);
-	} else {
+		break;
+	case TC9563_DSP2:
 		seq = dsp2_pwroff_seq;
 		len = ARRAY_SIZE(dsp2_pwroff_seq);
+		break;
+	default:
+		/* Only external downstream ports DSP1/DSP2 can be powered off */
+		return 0;
 	}
 
 	ret = tc9563_pwrctrl_i2c_bulk_write(tc9563->client, seq, len);

-- 
2.43.0

[PATCH 4/6] PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

DSP3 doesn't support tuning Tx amplitude and DFE settings. So just skip
the setting if DT has passed the tuning properties. There is no need to
error out the whole driver because of it.

Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index ca19bf60d290..28e9ec3e172a 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
@@ -308,6 +308,7 @@ static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
 static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
 					   enum tc9563_pwrctrl_ports port)
 {
+	struct device *dev = tc9563->pwrctrl.dev;
 	u32 amp = tc9563->cfg[port].tx_amp;
 	int port_access;
 
@@ -327,6 +328,9 @@ static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
 	case TC9563_DSP2:
 		port_access = 0x8;
 		break;
+	case TC9563_DSP3:
+		dev_dbg(dev, "Tx amplitude tuning not supported for DSP3\n");
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -345,6 +349,7 @@ static int tc9563_pwrctrl_disable_dfe(struct tc9563_pwrctrl *tc9563,
 				      enum tc9563_pwrctrl_ports port)
 {
 	struct tc9563_pwrctrl_cfg *cfg = &tc9563->cfg[port];
+	struct device *dev = tc9563->pwrctrl.dev;
 	int port_access, lane_access = 0x3;
 	u32 phy_rate = 0x21;
 
@@ -363,6 +368,9 @@ static int tc9563_pwrctrl_disable_dfe(struct tc9563_pwrctrl *tc9563,
 		port_access = 0x8;
 		lane_access = 0x1;
 		break;
+	case TC9563_DSP3:
+		dev_dbg(dev, "DFE tuning not supported for DSP3\n");
+		return 0;
 	default:
 		return -EINVAL;
 	}

-- 
2.43.0

[PATCH 5/6] PCI/pwrctrl: tc9563: Rename DSP3 to VDSP

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

TC9563 Reference Manual calls DSP3 as VDSP (Virtual Downstream Port). Hence
name it as such to avoid ambiguity.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index 28e9ec3e172a..f1179737f221 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
@@ -80,7 +80,7 @@ enum tc9563_pwrctrl_ports {
 	TC9563_USP,
 	TC9563_DSP1,
 	TC9563_DSP2,
-	TC9563_DSP3,
+	TC9563_VDSP,
 	TC9563_ETHERNET,
 	TC9563_MAX
 };
@@ -328,8 +328,8 @@ static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
 	case TC9563_DSP2:
 		port_access = 0x8;
 		break;
-	case TC9563_DSP3:
-		dev_dbg(dev, "Tx amplitude tuning not supported for DSP3\n");
+	case TC9563_VDSP:
+		dev_dbg(dev, "Tx amplitude tuning not supported for VDSP\n");
 		return 0;
 	default:
 		return -EINVAL;
@@ -368,8 +368,8 @@ static int tc9563_pwrctrl_disable_dfe(struct tc9563_pwrctrl *tc9563,
 		port_access = 0x8;
 		lane_access = 0x1;
 		break;
-	case TC9563_DSP3:
-		dev_dbg(dev, "DFE tuning not supported for DSP3\n");
+	case TC9563_VDSP:
+		dev_dbg(dev, "DFE tuning not supported for VDSP\n");
 		return 0;
 	default:
 		return -EINVAL;
@@ -612,10 +612,10 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 			break;
 
 		/*
-		 * The integrated Ethernet MAC Endpoint under DSP3 is a single
+		 * The integrated Ethernet MAC Endpoint under VDSP is a single
 		 * device whose functions share the same config registers.
 		 */
-		if (port == TC9563_DSP3) {
+		if (port == TC9563_VDSP) {
 			struct device_node *eth __free(device_node) =
 					of_get_next_available_child(child, NULL);
 			if (eth) {

-- 
2.43.0

[PATCH 6/6] PCI/pwrctrl: tc9563: Move Integrated MAC Endpoint out of 'tc9563_pwrctrl_ports' enum

[Manivannan Sadhasivam via B4 Relay]

From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

'tc9563_pwrctrl_ports' is supposed to list only the internal ports of the
switch. But it currently lists the integrated MAC Endpoint as well, which
is wrong.

Hence, move it to a separate 'ep_cfg' struct and also configure/parse the
ports and the MAC Endpoint separately.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 96 ++++++++++++++++++++------------
 1 file changed, 60 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index f1179737f221..9d869483b6b3 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
@@ -81,7 +81,6 @@ enum tc9563_pwrctrl_ports {
 	TC9563_DSP1,
 	TC9563_DSP2,
 	TC9563_VDSP,
-	TC9563_ETHERNET,
 	TC9563_MAX
 };
 
@@ -109,6 +108,7 @@ struct tc9563_pwrctrl {
 	struct pci_pwrctrl pwrctrl;
 	struct regulator_bulk_data supplies[TC9563_PWRCTL_MAX_SUPPLY];
 	struct tc9563_pwrctrl_cfg cfg[TC9563_MAX];
+	struct tc9563_pwrctrl_cfg ep_cfg;
 	struct gpio_desc *reset_gpio;
 	struct i2c_adapter *adapter;
 	struct i2c_client *client;
@@ -263,11 +263,11 @@ static int tc9563_pwrctrl_disable_port(struct tc9563_pwrctrl *tc9563,
 					     ARRAY_SIZE(common_pwroff_seq));
 }
 
-static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
-						 enum tc9563_pwrctrl_ports port,
-						 bool is_l1, u32 ns)
+static int tc9563_pwrctrl_set_port_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
+						      enum tc9563_pwrctrl_ports port,
+						      bool is_l1, u32 ns)
 {
-	u32 rd_val, units;
+	u32 units;
 	int ret;
 
 	if (ns < TC9563_L0S_L1_DELAY_UNIT_NS)
@@ -276,25 +276,6 @@ static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
 	/* convert to units of 256ns */
 	units = ns / TC9563_L0S_L1_DELAY_UNIT_NS;
 
-	if (port == TC9563_ETHERNET) {
-		ret = tc9563_pwrctrl_i2c_read(tc9563->client,
-					      TC9563_EMBEDDED_ETH_DELAY,
-					      &rd_val);
-		if (ret)
-			return ret;
-
-		if (is_l1)
-			rd_val = u32_replace_bits(rd_val, units,
-						  TC9563_ETH_L1_DELAY_MASK);
-		else
-			rd_val = u32_replace_bits(rd_val, units,
-						  TC9563_ETH_L0S_DELAY_MASK);
-
-		return tc9563_pwrctrl_i2c_write(tc9563->client,
-						TC9563_EMBEDDED_ETH_DELAY,
-						rd_val);
-	}
-
 	ret = tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_PORT_SELECT,
 				       BIT(port));
 	if (ret)
@@ -305,6 +286,34 @@ static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
 			units);
 }
 
+static int tc9563_pwrctrl_set_eth_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
+						     bool is_l1, u32 ns)
+{
+	u32 rd_val, units;
+	int ret;
+
+	if (ns < TC9563_L0S_L1_DELAY_UNIT_NS)
+		return 0;
+
+	/* convert to units of 256ns */
+	units = ns / TC9563_L0S_L1_DELAY_UNIT_NS;
+
+	ret = tc9563_pwrctrl_i2c_read(tc9563->client, TC9563_EMBEDDED_ETH_DELAY,
+				      &rd_val);
+	if (ret)
+		return ret;
+
+	if (is_l1)
+		rd_val = u32_replace_bits(rd_val, units,
+					  TC9563_ETH_L1_DELAY_MASK);
+	else
+		rd_val = u32_replace_bits(rd_val, units,
+					  TC9563_ETH_L0S_DELAY_MASK);
+
+	return tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_EMBEDDED_ETH_DELAY,
+					rd_val);
+}
+
 static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
 					   enum tc9563_pwrctrl_ports port)
 {
@@ -430,11 +439,9 @@ static int tc9563_pwrctrl_assert_deassert_reset(struct tc9563_pwrctrl *tc9563,
 	return tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_RESET_GPIO, val);
 }
 
-static int tc9563_pwrctrl_parse_device_dt(struct tc9563_pwrctrl *tc9563,
-					  struct device_node *node,
-					  enum tc9563_pwrctrl_ports port)
+static int tc9563_pwrctrl_parse_device_dt(struct device_node *node,
+					  struct tc9563_pwrctrl_cfg *cfg)
 {
-	struct tc9563_pwrctrl_cfg *cfg = &tc9563->cfg[port];
 	int ret;
 
 	/* Disable port if the status of the port is disabled. */
@@ -505,13 +512,13 @@ static int tc9563_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
 			goto power_off;
 		}
 
-		ret = tc9563_pwrctrl_set_l0s_l1_entry_delay(tc9563, i, false, cfg->l0s_delay);
+		ret = tc9563_pwrctrl_set_port_l0s_l1_entry_delay(tc9563, i, false, cfg->l0s_delay);
 		if (ret) {
 			dev_err(dev, "Setting L0s entry delay failed\n");
 			goto power_off;
 		}
 
-		ret = tc9563_pwrctrl_set_l0s_l1_entry_delay(tc9563, i, true, cfg->l1_delay);
+		ret = tc9563_pwrctrl_set_port_l0s_l1_entry_delay(tc9563, i, true, cfg->l1_delay);
 		if (ret) {
 			dev_err(dev, "Setting L1 entry delay failed\n");
 			goto power_off;
@@ -536,6 +543,21 @@ static int tc9563_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
 		}
 	}
 
+	/* Configure the integrated Ethernet MAC endpoint */
+	ret = tc9563_pwrctrl_set_eth_l0s_l1_entry_delay(tc9563, false,
+							tc9563->ep_cfg.l0s_delay);
+	if (ret) {
+		dev_err(dev, "Setting Ethernet L0s entry delay failed\n");
+		goto power_off;
+	}
+
+	ret = tc9563_pwrctrl_set_eth_l0s_l1_entry_delay(tc9563, true,
+							tc9563->ep_cfg.l1_delay);
+	if (ret) {
+		dev_err(dev, "Setting Ethernet L1 entry delay failed\n");
+		goto power_off;
+	}
+
 	ret = tc9563_pwrctrl_assert_deassert_reset(tc9563, true);
 	if (!ret)
 		return 0;
@@ -593,8 +615,7 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 
 	pci_pwrctrl_init(&tc9563->pwrctrl, dev);
 
-	port = TC9563_USP;
-	ret = tc9563_pwrctrl_parse_device_dt(tc9563, node, port);
+	ret = tc9563_pwrctrl_parse_device_dt(node, &tc9563->cfg[TC9563_USP]);
 	if (ret) {
 		dev_err(dev, "failed to parse device tree properties: %d\n", ret);
 		goto remove_i2c;
@@ -605,9 +626,12 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 	 * The first node represents DSP1, the second node represents DSP2,
 	 * and so on.
 	 */
+	port = TC9563_USP;
 	for_each_child_of_node_scoped(node, child) {
-		port++;
-		ret = tc9563_pwrctrl_parse_device_dt(tc9563, child, port);
+		if (++port >= TC9563_MAX)
+			break;
+
+		ret = tc9563_pwrctrl_parse_device_dt(child, &tc9563->cfg[port]);
 		if (ret)
 			break;
 
@@ -619,8 +643,8 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
 			struct device_node *eth __free(device_node) =
 					of_get_next_available_child(child, NULL);
 			if (eth) {
-				ret = tc9563_pwrctrl_parse_device_dt(tc9563,
-								eth, TC9563_ETHERNET);
+				ret = tc9563_pwrctrl_parse_device_dt(eth,
+							&tc9563->ep_cfg);
 				if (ret)
 					break;
 			}

-- 
2.43.0

Re: [PATCH 2/6] PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node

[Bartosz Golaszewski]

On Sat, 25 Jul 2026 10:59:14 +0200, Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> said:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>
> DSP3 has an integrated Ethernet MAC Endpoint which has its own set of
> config registers for configuring settings such as ASPM. The Endpoint
> device has 2 physical functions and those 2 functions share the same
> settings.
>
> Hence, parse the Endpoint node under DSP3 instead of parsing both the
> functions. The existing parsing logic also has one OOB issue as parsing
> both functions will result in accessing past the tc9563_pwrctrl->cfg array.
>
> Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 488e1ec34a7f..482e82e433dd 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -596,12 +596,17 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>  		ret = tc9563_pwrctrl_parse_device_dt(tc9563, child, port);
>  		if (ret)
>  			break;
> -		/* Embedded ethernet device are under DSP3 */
> +
> +		/*
> +		 * The integrated Ethernet MAC Endpoint under DSP3 is a single
> +		 * device whose functions share the same config registers.
> +		 */
>  		if (port == TC9563_DSP3) {
> -			for_each_child_of_node_scoped(child, child1) {
> -				port++;
> +			struct device_node *eth __free(device_node) =
> +					of_get_next_available_child(child, NULL);
> +			if (eth) {
>  				ret = tc9563_pwrctrl_parse_device_dt(tc9563,
> -								child1, port);
> +								eth, TC9563_ETHERNET);
>  				if (ret)
>  					break;
>  			}
>
> --
> 2.43.0
>
>
>

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH 3/6] PCI/pwrctrl: tc9563: Power off only the external ports in tc9563_pwrctrl_disable_port()

[Bartosz Golaszewski]

On Sat, 25 Jul 2026 10:59:15 +0200, Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> said:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>
> TC9563 supports powering off only the external facing ports like DSP1 and
> DSP2. It is not recommended to power off USP and DSP3 as they have fixed
> ports/endpoint connected.
>
> Hence, fix tc9563_pwrctrl_disable_port() to power off only DSP1 and DSP2.
>
> Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 482e82e433dd..ca19bf60d290 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -241,12 +241,18 @@ static int tc9563_pwrctrl_disable_port(struct tc9563_pwrctrl *tc9563,
>  	if (!cfg->disable_port)
>  		return 0;
>
> -	if (port == TC9563_DSP1) {
> +	switch (port) {
> +	case TC9563_DSP1:
>  		seq = dsp1_pwroff_seq;
>  		len = ARRAY_SIZE(dsp1_pwroff_seq);
> -	} else {
> +		break;
> +	case TC9563_DSP2:
>  		seq = dsp2_pwroff_seq;
>  		len = ARRAY_SIZE(dsp2_pwroff_seq);
> +		break;
> +	default:
> +		/* Only external downstream ports DSP1/DSP2 can be powered off */
> +		return 0;
>  	}
>
>  	ret = tc9563_pwrctrl_i2c_bulk_write(tc9563->client, seq, len);
>
> --
> 2.43.0
>
>
>

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH 4/6] PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3

[Bartosz Golaszewski]

On Sat, 25 Jul 2026 10:59:16 +0200, Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> said:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>
> DSP3 doesn't support tuning Tx amplitude and DFE settings. So just skip
> the setting if DT has passed the tuning properties. There is no need to
> error out the whole driver because of it.
>
> Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index ca19bf60d290..28e9ec3e172a 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -308,6 +308,7 @@ static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
>  static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
>  					   enum tc9563_pwrctrl_ports port)
>  {
> +	struct device *dev = tc9563->pwrctrl.dev;
>  	u32 amp = tc9563->cfg[port].tx_amp;
>  	int port_access;
>
> @@ -327,6 +328,9 @@ static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
>  	case TC9563_DSP2:
>  		port_access = 0x8;
>  		break;
> +	case TC9563_DSP3:
> +		dev_dbg(dev, "Tx amplitude tuning not supported for DSP3\n");
> +		return 0;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -345,6 +349,7 @@ static int tc9563_pwrctrl_disable_dfe(struct tc9563_pwrctrl *tc9563,
>  				      enum tc9563_pwrctrl_ports port)
>  {
>  	struct tc9563_pwrctrl_cfg *cfg = &tc9563->cfg[port];
> +	struct device *dev = tc9563->pwrctrl.dev;
>  	int port_access, lane_access = 0x3;
>  	u32 phy_rate = 0x21;
>
> @@ -363,6 +368,9 @@ static int tc9563_pwrctrl_disable_dfe(struct tc9563_pwrctrl *tc9563,
>  		port_access = 0x8;
>  		lane_access = 0x1;
>  		break;
> +	case TC9563_DSP3:
> +		dev_dbg(dev, "DFE tuning not supported for DSP3\n");
> +		return 0;
>  	default:
>  		return -EINVAL;
>  	}
>
> --
> 2.43.0
>
>
>

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH 5/6] PCI/pwrctrl: tc9563: Rename DSP3 to VDSP

[Bartosz Golaszewski]

On Sat, 25 Jul 2026 10:59:17 +0200, Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> said:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>
> TC9563 Reference Manual calls DSP3 as VDSP (Virtual Downstream Port). Hence
> name it as such to avoid ambiguity.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 28e9ec3e172a..f1179737f221 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -80,7 +80,7 @@ enum tc9563_pwrctrl_ports {
>  	TC9563_USP,
>  	TC9563_DSP1,
>  	TC9563_DSP2,
> -	TC9563_DSP3,
> +	TC9563_VDSP,
>  	TC9563_ETHERNET,
>  	TC9563_MAX
>  };
> @@ -328,8 +328,8 @@ static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
>  	case TC9563_DSP2:
>  		port_access = 0x8;
>  		break;
> -	case TC9563_DSP3:
> -		dev_dbg(dev, "Tx amplitude tuning not supported for DSP3\n");
> +	case TC9563_VDSP:
> +		dev_dbg(dev, "Tx amplitude tuning not supported for VDSP\n");
>  		return 0;
>  	default:
>  		return -EINVAL;
> @@ -368,8 +368,8 @@ static int tc9563_pwrctrl_disable_dfe(struct tc9563_pwrctrl *tc9563,
>  		port_access = 0x8;
>  		lane_access = 0x1;
>  		break;
> -	case TC9563_DSP3:
> -		dev_dbg(dev, "DFE tuning not supported for DSP3\n");
> +	case TC9563_VDSP:
> +		dev_dbg(dev, "DFE tuning not supported for VDSP\n");
>  		return 0;
>  	default:
>  		return -EINVAL;
> @@ -612,10 +612,10 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>  			break;
>
>  		/*
> -		 * The integrated Ethernet MAC Endpoint under DSP3 is a single
> +		 * The integrated Ethernet MAC Endpoint under VDSP is a single
>  		 * device whose functions share the same config registers.
>  		 */
> -		if (port == TC9563_DSP3) {
> +		if (port == TC9563_VDSP) {
>  			struct device_node *eth __free(device_node) =
>  					of_get_next_available_child(child, NULL);
>  			if (eth) {
>
> --
> 2.43.0
>
>
>

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH 6/6] PCI/pwrctrl: tc9563: Move Integrated MAC Endpoint out of 'tc9563_pwrctrl_ports' enum

[Bartosz Golaszewski]

On Sat, 25 Jul 2026 10:59:18 +0200, Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.oss.qualcomm.com@kernel.org> said:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>
> 'tc9563_pwrctrl_ports' is supposed to list only the internal ports of the
> switch. But it currently lists the integrated MAC Endpoint as well, which
> is wrong.
>
> Hence, move it to a separate 'ep_cfg' struct and also configure/parse the
> ports and the MAC Endpoint separately.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---


Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH 1/6] dt-bindings: PCI: toshiba,tc9563: Restrict Tx Amplitude, DFE and N_FTS to USP, DSP1 and DSP2

[Alex Elder]

On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> TC9563 switch allows configuring Tx Amplitude, DFE and N_FTS settings only
> to USP, DSP1 and DSP2 ports. These settings don't apply to DSP3 and also
> the integrated Ethernet MAC Endpoint.

s/and also the/nor its/

Also I don't know if there is only one Ethernet MAC here, but
the TC9564 has two, so maybe make "MAC Endpoint" plural.

> 
> So restrict these properties and also move the
> 'toshiba,tx-amplitude-microvolt' property to DSP2 in the example to conform
> to the binding.
> 
> Fixes: 26f3ac884835 ("dt-bindings: PCI: Add binding for Toshiba TC9563 PCIe switch")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

I have some other questions with implied suggestions below, but
this looks good to me.

Acked-by: Alex Elder <elder@riscstar.com>

> ---
>   .../devicetree/bindings/pci/toshiba,tc9563.yaml     | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml b/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
> index b3ad05d90201..f084830c6d0c 100644
> --- a/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
> +++ b/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
> @@ -54,7 +54,7 @@ properties:
>             - description: I2C slave address
>   
>   patternProperties:
> -  "^pcie@[1-3],0$":
> +  "^pcie@[1-2],0$":
>       description:
>         child nodes describing the internal downstream ports of
>         the tc9563 switch.

Can you offer any explanation about why the settings apply to these
two downstream ports but not the third?  (I think it's because the
third has a different type of link to the endpoint.)

> @@ -64,6 +64,21 @@ patternProperties:
>         - $ref: /schemas/pci/pci-pci-bridge.yaml#
>       unevaluatedProperties: false
>   
> +  "^pcie@[3],0$":

Can this just be "^pcie@3,0$"?

Is there any need to define "^pcie@3,1" for the second PCI function?

> +    description:
> +      child node describing the internal downstream port connected to the
> +      integrated Ethernet MAC endpoint of the tc9563 switch.
> +    type: object
> +    $ref: /schemas/pci/pci-pci-bridge.yaml#
> +    properties:
> +      n-fts: false
> +    patternProperties:
> +      "^ethernet@0,[0-1]$":
> +        type: object
> +        properties:
> +          n-fts: false
> +    unevaluatedProperties: false
> +
>   $defs:
>     tc9563-node:
>       type: object
> @@ -154,6 +169,8 @@ examples:
>                       device_type = "pci";
>                       ranges;
>                       bus-range = <0x04 0xff>;
> +
> +                    toshiba,tx-amplitude-microvolt = <10>;
>                   };
>   
>                   pcie@3,0 {
> @@ -165,8 +182,6 @@ examples:
>                       ranges;
>                       bus-range = <0x05 0xff>;
>   
> -                    toshiba,tx-amplitude-microvolt = <10>;
> -
>                       ethernet@0,0 {
>                           reg = <0x50000 0x0 0x0 0x0 0x0>;
>                       };
> 

Re: [PATCH 2/6] PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node

[Alex Elder]

On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> DSP3 has an integrated Ethernet MAC Endpoint which has its own set of
> config registers for configuring settings such as ASPM. The Endpoint
> device has 2 physical functions and those 2 functions share the same
> settings.

This was not clear from the code.  So this explanation alone
represents an improvement.

> Hence, parse the Endpoint node under DSP3 instead of parsing both the
> functions. The existing parsing logic also has one OOB issue as parsing
> both functions will result in accessing past the tc9563_pwrctrl->cfg array.

The issue you mention is that the array has exactly one entry
available for the Ethernet MAC endpoints, so parsing more than
one (if defined in devicetree) references a non-existent entry.
Right?

But this constraint only applies to power control.  There can be
multiple MAC nodes (for their PCI functions) defined.  And it
seems the pwrctl settings (if any) *must* be specified for
function 0 and if specified for any others, they are quietly
ignored.

Is that accurate?

If so I think the DT binding could explain and possibly enforce
that, but I won't insist on that...

Reviewed-by: Alex Elder <elder@riscstar.com>

> 
> Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>   drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 488e1ec34a7f..482e82e433dd 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -596,12 +596,17 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>   		ret = tc9563_pwrctrl_parse_device_dt(tc9563, child, port);
>   		if (ret)
>   			break;
> -		/* Embedded ethernet device are under DSP3 */
> +
> +		/*
> +		 * The integrated Ethernet MAC Endpoint under DSP3 is a single
> +		 * device whose functions share the same config registers.
> +		 */
>   		if (port == TC9563_DSP3) {
> -			for_each_child_of_node_scoped(child, child1) {
> -				port++;
> +			struct device_node *eth __free(device_node) =
> +					of_get_next_available_child(child, NULL);

I'd add a blank line here.

> +			if (eth) {
>   				ret = tc9563_pwrctrl_parse_device_dt(tc9563,
> -								child1, port);
> +								eth, TC9563_ETHERNET);
>   				if (ret)
>   					break;
>   			}
> 

Re: [PATCH 3/6] PCI/pwrctrl: tc9563: Power off only the external ports in tc9563_pwrctrl_disable_port()

[Alex Elder]

On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> TC9563 supports powering off only the external facing ports like DSP1 and
> DSP2. It is not recommended to power off USP and DSP3 as they have fixed
> ports/endpoint connected.
> 
> Hence, fix tc9563_pwrctrl_disable_port() to power off only DSP1 and DSP2.
> 
> Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

This is very nice and makes that function a little less
confusing...

Reviewed-by: Alex Elder <elder@riscstar.com>

> ---
>   drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 482e82e433dd..ca19bf60d290 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -241,12 +241,18 @@ static int tc9563_pwrctrl_disable_port(struct tc9563_pwrctrl *tc9563,
>   	if (!cfg->disable_port)
>   		return 0;
>   
> -	if (port == TC9563_DSP1) {
> +	switch (port) {
> +	case TC9563_DSP1:
>   		seq = dsp1_pwroff_seq;
>   		len = ARRAY_SIZE(dsp1_pwroff_seq);
> -	} else {
> +		break;
> +	case TC9563_DSP2:
>   		seq = dsp2_pwroff_seq;
>   		len = ARRAY_SIZE(dsp2_pwroff_seq);
> +		break;
> +	default:
> +		/* Only external downstream ports DSP1/DSP2 can be powered off */
> +		return 0;
>   	}
>   
>   	ret = tc9563_pwrctrl_i2c_bulk_write(tc9563->client, seq, len);
> 

Re: [PATCH 4/6] PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3

[Alex Elder]

On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> DSP3 doesn't support tuning Tx amplitude and DFE settings. So just skip
> the setting if DT has passed the tuning properties. There is no need to
> error out the whole driver because of it.
> 
> Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

So to keep things simple, tc9563_pwrctrl_parse_device_dt() is called
for all downstream ports but you ignore the setting (with a warning)
for those that do not apply to DSP3.

What about tc9563_pwrctrl_set_nfts()?

Other than that (question) this looks good.

Reviewed-by: Alex Elder <elder@riscstar.com>

> ---
>   drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index ca19bf60d290..28e9ec3e172a 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -308,6 +308,7 @@ static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
>   static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
>   					   enum tc9563_pwrctrl_ports port)
>   {
> +	struct device *dev = tc9563->pwrctrl.dev;
>   	u32 amp = tc9563->cfg[port].tx_amp;
>   	int port_access;
>   
> @@ -327,6 +328,9 @@ static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
>   	case TC9563_DSP2:
>   		port_access = 0x8;
>   		break;
> +	case TC9563_DSP3:
> +		dev_dbg(dev, "Tx amplitude tuning not supported for DSP3\n");
> +		return 0;
>   	default:
>   		return -EINVAL;
>   	}
> @@ -345,6 +349,7 @@ static int tc9563_pwrctrl_disable_dfe(struct tc9563_pwrctrl *tc9563,
>   				      enum tc9563_pwrctrl_ports port)
>   {
>   	struct tc9563_pwrctrl_cfg *cfg = &tc9563->cfg[port];
> +	struct device *dev = tc9563->pwrctrl.dev;
>   	int port_access, lane_access = 0x3;
>   	u32 phy_rate = 0x21;
>   
> @@ -363,6 +368,9 @@ static int tc9563_pwrctrl_disable_dfe(struct tc9563_pwrctrl *tc9563,
>   		port_access = 0x8;
>   		lane_access = 0x1;
>   		break;
> +	case TC9563_DSP3:
> +		dev_dbg(dev, "DFE tuning not supported for DSP3\n");
> +		return 0;
>   	default:
>   		return -EINVAL;
>   	}
> 

Re: [PATCH 5/6] PCI/pwrctrl: tc9563: Rename DSP3 to VDSP

[Alex Elder]

On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> TC9563 Reference Manual calls DSP3 as VDSP (Virtual Downstream Port). Hence
> name it as such to avoid ambiguity.

I didn't really like the overloaded "virtual" term (though maybe
it is normal for PCI).  However I do like that this makes it
clearer that the third downstream port is different from the
first two.

Reviewed-by: Alex Elder <elder@riscstar.com>

> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>   drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 28e9ec3e172a..f1179737f221 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -80,7 +80,7 @@ enum tc9563_pwrctrl_ports {
>   	TC9563_USP,
>   	TC9563_DSP1,
>   	TC9563_DSP2,
> -	TC9563_DSP3,
> +	TC9563_VDSP,
>   	TC9563_ETHERNET,
>   	TC9563_MAX
>   };
> @@ -328,8 +328,8 @@ static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
>   	case TC9563_DSP2:
>   		port_access = 0x8;
>   		break;
> -	case TC9563_DSP3:
> -		dev_dbg(dev, "Tx amplitude tuning not supported for DSP3\n");
> +	case TC9563_VDSP:
> +		dev_dbg(dev, "Tx amplitude tuning not supported for VDSP\n");
>   		return 0;
>   	default:
>   		return -EINVAL;
> @@ -368,8 +368,8 @@ static int tc9563_pwrctrl_disable_dfe(struct tc9563_pwrctrl *tc9563,
>   		port_access = 0x8;
>   		lane_access = 0x1;
>   		break;
> -	case TC9563_DSP3:
> -		dev_dbg(dev, "DFE tuning not supported for DSP3\n");
> +	case TC9563_VDSP:
> +		dev_dbg(dev, "DFE tuning not supported for VDSP\n");
>   		return 0;
>   	default:
>   		return -EINVAL;
> @@ -612,10 +612,10 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>   			break;
>   
>   		/*
> -		 * The integrated Ethernet MAC Endpoint under DSP3 is a single
> +		 * The integrated Ethernet MAC Endpoint under VDSP is a single
>   		 * device whose functions share the same config registers.
>   		 */
> -		if (port == TC9563_DSP3) {
> +		if (port == TC9563_VDSP) {
>   			struct device_node *eth __free(device_node) =
>   					of_get_next_available_child(child, NULL);
>   			if (eth) {
> 

Re: [PATCH 6/6] PCI/pwrctrl: tc9563: Move Integrated MAC Endpoint out of 'tc9563_pwrctrl_ports' enum

[Alex Elder]

On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> 'tc9563_pwrctrl_ports' is supposed to list only the internal ports of the
> switch. But it currently lists the integrated MAC Endpoint as well, which
> is wrong.
> 
> Hence, move it to a separate 'ep_cfg' struct and also configure/parse the
> ports and the MAC Endpoint separately.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

I like this.  It cleans up some things I was going to suggest myself
at some point.

Reviewed-by: Alex Elder <elder@riscstar.com>

> ---
>   drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 96 ++++++++++++++++++++------------
>   1 file changed, 60 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index f1179737f221..9d869483b6b3 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -81,7 +81,6 @@ enum tc9563_pwrctrl_ports {
>   	TC9563_DSP1,
>   	TC9563_DSP2,
>   	TC9563_VDSP,
> -	TC9563_ETHERNET,
>   	TC9563_MAX
>   };
>   
> @@ -109,6 +108,7 @@ struct tc9563_pwrctrl {
>   	struct pci_pwrctrl pwrctrl;
>   	struct regulator_bulk_data supplies[TC9563_PWRCTL_MAX_SUPPLY];
>   	struct tc9563_pwrctrl_cfg cfg[TC9563_MAX];
> +	struct tc9563_pwrctrl_cfg ep_cfg;

maybe something with "eth" in the name?

>   	struct gpio_desc *reset_gpio;
>   	struct i2c_adapter *adapter;
>   	struct i2c_client *client;
> @@ -263,11 +263,11 @@ static int tc9563_pwrctrl_disable_port(struct tc9563_pwrctrl *tc9563,
>   					     ARRAY_SIZE(common_pwroff_seq));
>   }
>   
> -static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
> -						 enum tc9563_pwrctrl_ports port,
> -						 bool is_l1, u32 ns)
> +static int tc9563_pwrctrl_set_port_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
> +						      enum tc9563_pwrctrl_ports port,
> +						      bool is_l1, u32 ns)
>   {
> -	u32 rd_val, units;
> +	u32 units;
>   	int ret;
>   
>   	if (ns < TC9563_L0S_L1_DELAY_UNIT_NS)
> @@ -276,25 +276,6 @@ static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
>   	/* convert to units of 256ns */
>   	units = ns / TC9563_L0S_L1_DELAY_UNIT_NS;
>   
> -	if (port == TC9563_ETHERNET) {
> -		ret = tc9563_pwrctrl_i2c_read(tc9563->client,
> -					      TC9563_EMBEDDED_ETH_DELAY,
> -					      &rd_val);
> -		if (ret)
> -			return ret;
> -
> -		if (is_l1)
> -			rd_val = u32_replace_bits(rd_val, units,
> -						  TC9563_ETH_L1_DELAY_MASK);
> -		else
> -			rd_val = u32_replace_bits(rd_val, units,
> -						  TC9563_ETH_L0S_DELAY_MASK);
> -
> -		return tc9563_pwrctrl_i2c_write(tc9563->client,
> -						TC9563_EMBEDDED_ETH_DELAY,
> -						rd_val);
> -	}
> -
>   	ret = tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_PORT_SELECT,
>   				       BIT(port));
>   	if (ret)
> @@ -305,6 +286,34 @@ static int tc9563_pwrctrl_set_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
>   			units);
>   }
>   
> +static int tc9563_pwrctrl_set_eth_l0s_l1_entry_delay(struct tc9563_pwrctrl *tc9563,
> +						     bool is_l1, u32 ns)
> +{
> +	u32 rd_val, units;
> +	int ret;
> +
> +	if (ns < TC9563_L0S_L1_DELAY_UNIT_NS)
> +		return 0;
> +
> +	/* convert to units of 256ns */
> +	units = ns / TC9563_L0S_L1_DELAY_UNIT_NS;
> +
> +	ret = tc9563_pwrctrl_i2c_read(tc9563->client, TC9563_EMBEDDED_ETH_DELAY,
> +				      &rd_val);
> +	if (ret)
> +		return ret;
> +
> +	if (is_l1)
> +		rd_val = u32_replace_bits(rd_val, units,
> +					  TC9563_ETH_L1_DELAY_MASK);
> +	else
> +		rd_val = u32_replace_bits(rd_val, units,
> +					  TC9563_ETH_L0S_DELAY_MASK);
> +
> +	return tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_EMBEDDED_ETH_DELAY,
> +					rd_val);
> +}
> +
>   static int tc9563_pwrctrl_set_tx_amplitude(struct tc9563_pwrctrl *tc9563,
>   					   enum tc9563_pwrctrl_ports port)
>   {
> @@ -430,11 +439,9 @@ static int tc9563_pwrctrl_assert_deassert_reset(struct tc9563_pwrctrl *tc9563,
>   	return tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_RESET_GPIO, val);
>   }
>   
> -static int tc9563_pwrctrl_parse_device_dt(struct tc9563_pwrctrl *tc9563,
> -					  struct device_node *node,
> -					  enum tc9563_pwrctrl_ports port)
> +static int tc9563_pwrctrl_parse_device_dt(struct device_node *node,
> +					  struct tc9563_pwrctrl_cfg *cfg)
>   {
> -	struct tc9563_pwrctrl_cfg *cfg = &tc9563->cfg[port];
>   	int ret;
>   
>   	/* Disable port if the status of the port is disabled. */
> @@ -505,13 +512,13 @@ static int tc9563_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
>   			goto power_off;
>   		}
>   
> -		ret = tc9563_pwrctrl_set_l0s_l1_entry_delay(tc9563, i, false, cfg->l0s_delay);
> +		ret = tc9563_pwrctrl_set_port_l0s_l1_entry_delay(tc9563, i, false, cfg->l0s_delay);
>   		if (ret) {
>   			dev_err(dev, "Setting L0s entry delay failed\n");
>   			goto power_off;
>   		}
>   
> -		ret = tc9563_pwrctrl_set_l0s_l1_entry_delay(tc9563, i, true, cfg->l1_delay);
> +		ret = tc9563_pwrctrl_set_port_l0s_l1_entry_delay(tc9563, i, true, cfg->l1_delay);
>   		if (ret) {
>   			dev_err(dev, "Setting L1 entry delay failed\n");
>   			goto power_off;
> @@ -536,6 +543,21 @@ static int tc9563_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
>   		}
>   	}
>   
> +	/* Configure the integrated Ethernet MAC endpoint */
> +	ret = tc9563_pwrctrl_set_eth_l0s_l1_entry_delay(tc9563, false,
> +							tc9563->ep_cfg.l0s_delay);
> +	if (ret) {
> +		dev_err(dev, "Setting Ethernet L0s entry delay failed\n");
> +		goto power_off;
> +	}
> +
> +	ret = tc9563_pwrctrl_set_eth_l0s_l1_entry_delay(tc9563, true,
> +							tc9563->ep_cfg.l1_delay);
> +	if (ret) {
> +		dev_err(dev, "Setting Ethernet L1 entry delay failed\n");
> +		goto power_off;
> +	}
> +
>   	ret = tc9563_pwrctrl_assert_deassert_reset(tc9563, true);
>   	if (!ret)
>   		return 0;
> @@ -593,8 +615,7 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>   
>   	pci_pwrctrl_init(&tc9563->pwrctrl, dev);
>   
> -	port = TC9563_USP;
> -	ret = tc9563_pwrctrl_parse_device_dt(tc9563, node, port);
> +	ret = tc9563_pwrctrl_parse_device_dt(node, &tc9563->cfg[TC9563_USP]);
>   	if (ret) {
>   		dev_err(dev, "failed to parse device tree properties: %d\n", ret);
>   		goto remove_i2c;
> @@ -605,9 +626,12 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>   	 * The first node represents DSP1, the second node represents DSP2,
>   	 * and so on.
>   	 */
> +	port = TC9563_USP;
>   	for_each_child_of_node_scoped(node, child) {
> -		port++;
> -		ret = tc9563_pwrctrl_parse_device_dt(tc9563, child, port);
> +		if (++port >= TC9563_MAX)
> +			break;
> +
> +		ret = tc9563_pwrctrl_parse_device_dt(child, &tc9563->cfg[port]);
>   		if (ret)
>   			break;
>   
> @@ -619,8 +643,8 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>   			struct device_node *eth __free(device_node) =
>   					of_get_next_available_child(child, NULL);
>   			if (eth) {
> -				ret = tc9563_pwrctrl_parse_device_dt(tc9563,
> -								eth, TC9563_ETHERNET);
> +				ret = tc9563_pwrctrl_parse_device_dt(eth,
> +							&tc9563->ep_cfg);
>   				if (ret)
>   					break;
>   			}
> 

Re: [PATCH 0/6] PCI/pwrctrl: tc9563: Several fixes and improvements

[Bjorn Helgaas]

On Sat, Jul 25, 2026 at 10:59:12AM +0200, Manivannan Sadhasivam via B4 Relay wrote:
> Hi,
> 
> This series contains several fixes and improvements for the TC9563 Pwrctrl
> driver and the switch binding. Most of the fixes are related to ensuring
> correctness in parsing the DT properties for the switch ports and the
> integrated Endpoint.
> 
> More details are in the individual patches.
> 
> Testing
> =======
> 
> This series was validated on the RB3Gen2 board.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
> Manivannan Sadhasivam (6):
>       dt-bindings: PCI: toshiba,tc9563: Restrict Tx Amplitude, DFE and N_FTS to USP, DSP1 and DSP2
>       PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node
>       PCI/pwrctrl: tc9563: Power off only the external ports in tc9563_pwrctrl_disable_port()
>       PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3
>       PCI/pwrctrl: tc9563: Rename DSP3 to VDSP
>       PCI/pwrctrl: tc9563: Move Integrated MAC Endpoint out of 'tc9563_pwrctrl_ports' enum
> 
>  .../devicetree/bindings/pci/toshiba,tc9563.yaml    |  21 +++-
>  drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c           | 129 ++++++++++++++-------
>  2 files changed, 104 insertions(+), 46 deletions(-)

Added to pci/pwrctrl for v7.3, thanks!

Happy to revise if you respond to any of Alex's comments.

Re: [PATCH 1/6] dt-bindings: PCI: toshiba,tc9563: Restrict Tx Amplitude, DFE and N_FTS to USP, DSP1 and DSP2

[Manivannan Sadhasivam]

On Mon, Jul 27, 2026 at 10:59:02AM -0500, Alex Elder wrote:
> On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > TC9563 switch allows configuring Tx Amplitude, DFE and N_FTS settings only
> > to USP, DSP1 and DSP2 ports. These settings don't apply to DSP3 and also
> > the integrated Ethernet MAC Endpoint.
> 
> s/and also the/nor its/
> 
> Also I don't know if there is only one Ethernet MAC here, but
> the TC9564 has two, so maybe make "MAC Endpoint" plural.
> 
> > 
> > So restrict these properties and also move the
> > 'toshiba,tx-amplitude-microvolt' property to DSP2 in the example to conform
> > to the binding.
> > 
> > Fixes: 26f3ac884835 ("dt-bindings: PCI: Add binding for Toshiba TC9563 PCIe switch")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> I have some other questions with implied suggestions below, but
> this looks good to me.
> 
> Acked-by: Alex Elder <elder@riscstar.com>
> 
> > ---
> >   .../devicetree/bindings/pci/toshiba,tc9563.yaml     | 21 ++++++++++++++++++---
> >   1 file changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml b/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
> > index b3ad05d90201..f084830c6d0c 100644
> > --- a/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
> > +++ b/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml
> > @@ -54,7 +54,7 @@ properties:
> >             - description: I2C slave address
> >   patternProperties:
> > -  "^pcie@[1-3],0$":
> > +  "^pcie@[1-2],0$":
> >       description:
> >         child nodes describing the internal downstream ports of
> >         the tc9563 switch.
> 
> Can you offer any explanation about why the settings apply to these
> two downstream ports but not the third?  (I think it's because the
> third has a different type of link to the endpoint.)
> 

There was no explanation provided in the manual or in the application note. But
since these are PHY related tunings and the VDSP has an integrated MAC Endpoint,
these tunings are irrelevant.

> > @@ -64,6 +64,21 @@ patternProperties:
> >         - $ref: /schemas/pci/pci-pci-bridge.yaml#
> >       unevaluatedProperties: false
> > +  "^pcie@[3],0$":
> 
> Can this just be "^pcie@3,0$"?
> 
> Is there any need to define "^pcie@3,1" for the second PCI function?
> 

Good question. I tried "^pcie@3,0$" first, but it throwed the dt_binding_check
error:

/home/mani/tc9563-fix/Documentation/devicetree/bindings/pci/toshiba,tc9563.yaml:
patternProperties: '^pcie@3,0$' should not be valid under {'pattern': '^\\^[a-zA-Z0-9,\\-._#@]+\\$$'}
        hint: Fixed strings belong in 'properties', not 'patternProperties'

Looks like the schema doesn't allow fixed strings in patternProperties. So I
used regex, just to satisfy the requirement.

- Mani

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

Re: [PATCH 2/6] PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node

[Manivannan Sadhasivam]

On Mon, Jul 27, 2026 at 10:59:05AM -0500, Alex Elder wrote:
> On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > DSP3 has an integrated Ethernet MAC Endpoint which has its own set of
> > config registers for configuring settings such as ASPM. The Endpoint
> > device has 2 physical functions and those 2 functions share the same
> > settings.
> 
> This was not clear from the code.  So this explanation alone
> represents an improvement.
> 
> > Hence, parse the Endpoint node under DSP3 instead of parsing both the
> > functions. The existing parsing logic also has one OOB issue as parsing
> > both functions will result in accessing past the tc9563_pwrctrl->cfg array.
> 
> The issue you mention is that the array has exactly one entry
> available for the Ethernet MAC endpoints, so parsing more than
> one (if defined in devicetree) references a non-existent entry.
> Right?
> 

Yes.

> But this constraint only applies to power control.  There can be
> multiple MAC nodes (for their PCI functions) defined.  And it
> seems the pwrctl settings (if any) *must* be specified for
> function 0 and if specified for any others, they are quietly
> ignored.
> 
> Is that accurate?
> 

Exactly. DT can define both functions, but since there is no per-function
tuning, these properties cannot exist in both nodes, but only in function 0,
which also corresponds to the Endpoint.

> If so I think the DT binding could explain and possibly enforce
> that, but I won't insist on that...
> 

It is being enforced in Patch 1.

- Mani

> Reviewed-by: Alex Elder <elder@riscstar.com>
> 
> > 
> > Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >   drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > index 488e1ec34a7f..482e82e433dd 100644
> > --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > @@ -596,12 +596,17 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
> >   		ret = tc9563_pwrctrl_parse_device_dt(tc9563, child, port);
> >   		if (ret)
> >   			break;
> > -		/* Embedded ethernet device are under DSP3 */
> > +
> > +		/*
> > +		 * The integrated Ethernet MAC Endpoint under DSP3 is a single
> > +		 * device whose functions share the same config registers.
> > +		 */
> >   		if (port == TC9563_DSP3) {
> > -			for_each_child_of_node_scoped(child, child1) {
> > -				port++;
> > +			struct device_node *eth __free(device_node) =
> > +					of_get_next_available_child(child, NULL);
> 
> I'd add a blank line here.
> 
> > +			if (eth) {
> >   				ret = tc9563_pwrctrl_parse_device_dt(tc9563,
> > -								child1, port);
> > +								eth, TC9563_ETHERNET);
> >   				if (ret)
> >   					break;
> >   			}
> > 
> 

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

Re: [PATCH 4/6] PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3

[Manivannan Sadhasivam]

On Mon, Jul 27, 2026 at 10:59:11AM -0500, Alex Elder wrote:
> On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > DSP3 doesn't support tuning Tx amplitude and DFE settings. So just skip
> > the setting if DT has passed the tuning properties. There is no need to
> > error out the whole driver because of it.
> > 
> > Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> So to keep things simple, tc9563_pwrctrl_parse_device_dt() is called
> for all downstream ports but you ignore the setting (with a warning)
> for those that do not apply to DSP3.
> 
> What about tc9563_pwrctrl_set_nfts()?
> 

Good catch! I missed adding the check for N_FTS as well:

```
diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
index 9d869483b6b3..978907aa0ce5 100644
--- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
+++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
@@ -410,11 +410,17 @@ static int tc9563_pwrctrl_set_nfts(struct tc9563_pwrctrl *tc9563,
                {TC9563_NFTS_2_5_GT, nfts[0]},
                {TC9563_NFTS_5_GT, nfts[1]},
        };
+       struct device *dev = tc9563->pwrctrl.dev;
        int ret;

        if (!nfts[0])
                return 0;

+       if (port == TC9563_VDSP) {
+               dev_dbg(dev, "N_FTS tuning not supported for VDSP\n");
+               return 0;
+       }
+
        ret =  tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_PORT_SELECT,
                                        BIT(port));
        if (ret)
```

@Bjorn: Could you please amend the above diff with this commit?

- Mani

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

Re: [PATCH 4/6] PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3

[Alex Elder]

On 7/28/26 8:25 AM, Manivannan Sadhasivam wrote:
> On Mon, Jul 27, 2026 at 10:59:11AM -0500, Alex Elder wrote:
>> On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
>>> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>>
>>> DSP3 doesn't support tuning Tx amplitude and DFE settings. So just skip
>>> the setting if DT has passed the tuning properties. There is no need to
>>> error out the whole driver because of it.
>>>
>>> Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>
>> So to keep things simple, tc9563_pwrctrl_parse_device_dt() is called
>> for all downstream ports but you ignore the setting (with a warning)
>> for those that do not apply to DSP3.
>>
>> What about tc9563_pwrctrl_set_nfts()?
>>
> 
> Good catch! I missed adding the check for N_FTS as well:

In case you need it:

Reviewed-by: Alex Elder <elder@riscstar.com>

> 
> ```
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 9d869483b6b3..978907aa0ce5 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -410,11 +410,17 @@ static int tc9563_pwrctrl_set_nfts(struct tc9563_pwrctrl *tc9563,
>                  {TC9563_NFTS_2_5_GT, nfts[0]},
>                  {TC9563_NFTS_5_GT, nfts[1]},
>          };
> +       struct device *dev = tc9563->pwrctrl.dev;
>          int ret;
> 
>          if (!nfts[0])
>                  return 0;
> 
> +       if (port == TC9563_VDSP) {
> +               dev_dbg(dev, "N_FTS tuning not supported for VDSP\n");
> +               return 0;
> +       }
> +
>          ret =  tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_PORT_SELECT,
>                                          BIT(port));
>          if (ret)
> ```
> 
> @Bjorn: Could you please amend the above diff with this commit?
> 
> - Mani
> 

Re: [PATCH 2/6] PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node

[Manivannan Sadhasivam]

On Tue, Jul 28, 2026 at 03:12:49PM +0200, Manivannan Sadhasivam wrote:
> On Mon, Jul 27, 2026 at 10:59:05AM -0500, Alex Elder wrote:
> > On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > 
> > > DSP3 has an integrated Ethernet MAC Endpoint which has its own set of
> > > config registers for configuring settings such as ASPM. The Endpoint
> > > device has 2 physical functions and those 2 functions share the same
> > > settings.
> > 
> > This was not clear from the code.  So this explanation alone
> > represents an improvement.
> > 
> > > Hence, parse the Endpoint node under DSP3 instead of parsing both the
> > > functions. The existing parsing logic also has one OOB issue as parsing
> > > both functions will result in accessing past the tc9563_pwrctrl->cfg array.
> > 
> > The issue you mention is that the array has exactly one entry
> > available for the Ethernet MAC endpoints, so parsing more than
> > one (if defined in devicetree) references a non-existent entry.
> > Right?
> > 
> 
> Yes.
> 
> > But this constraint only applies to power control.  There can be
> > multiple MAC nodes (for their PCI functions) defined.  And it
> > seems the pwrctl settings (if any) *must* be specified for
> > function 0 and if specified for any others, they are quietly
> > ignored.
> > 
> > Is that accurate?
> > 
> 
> Exactly. DT can define both functions, but since there is no per-function
> tuning, these properties cannot exist in both nodes, but only in function 0,
> which also corresponds to the Endpoint.
> 
> > If so I think the DT binding could explain and possibly enforce
> > that, but I won't insist on that...
> > 
> 
> It is being enforced in Patch 1.
> 

Sorry, the current enforcement is only for PHY related properties like Tx
Amplitude, DFE and N_FTS. I didn't limit ASPM properties in function 1 though.
Doesn't seem to be strictly needed as no one would configure per-function ASPM
settings in DT (mostly unlikely).

- Mani

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

Re: [PATCH 2/6] PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node

[Alex Elder]

On 7/28/26 8:38 AM, Manivannan Sadhasivam wrote:
>>> If so I think the DT binding could explain and possibly enforce
>>> that, but I won't insist on that...
>>>
>> It is being enforced in Patch 1.
>>
> Sorry, the current enforcement is only for PHY related properties like Tx
> Amplitude, DFE and N_FTS. I didn't limit ASPM properties in function 1 though.
> Doesn't seem to be strictly needed as no one would configure per-function ASPM
> settings in DT (mostly unlikely).

I think it's fine.  You enforce what's needed at run time
regardless, right?

					-Alex

Re: [PATCH 2/6] PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node

[Manivannan Sadhasivam]

On Tue, Jul 28, 2026 at 08:41:02AM -0500, Alex Elder wrote:
> On 7/28/26 8:38 AM, Manivannan Sadhasivam wrote:
> > > > If so I think the DT binding could explain and possibly enforce
> > > > that, but I won't insist on that...
> > > > 
> > > It is being enforced in Patch 1.
> > > 
> > Sorry, the current enforcement is only for PHY related properties like Tx
> > Amplitude, DFE and N_FTS. I didn't limit ASPM properties in function 1 though.
> > Doesn't seem to be strictly needed as no one would configure per-function ASPM
> > settings in DT (mostly unlikely).
> 
> I think it's fine.  You enforce what's needed at run time
> regardless, right?
> 

Well, we don't parse the properties even if they are available in the function
DT nodes other than function 0.

- Mani

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

Re: [PATCH 4/6] PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3

[Bjorn Helgaas]

On Tue, Jul 28, 2026 at 03:25:29PM +0200, Manivannan Sadhasivam wrote:
> On Mon, Jul 27, 2026 at 10:59:11AM -0500, Alex Elder wrote:
> > On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > 
> > > DSP3 doesn't support tuning Tx amplitude and DFE settings. So just skip
> > > the setting if DT has passed the tuning properties. There is no need to
> > > error out the whole driver because of it.
> > > 
> > > Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > So to keep things simple, tc9563_pwrctrl_parse_device_dt() is called
> > for all downstream ports but you ignore the setting (with a warning)
> > for those that do not apply to DSP3.
> > 
> > What about tc9563_pwrctrl_set_nfts()?
> 
> Good catch! I missed adding the check for N_FTS as well:
> 
> ```
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 9d869483b6b3..978907aa0ce5 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -410,11 +410,17 @@ static int tc9563_pwrctrl_set_nfts(struct tc9563_pwrctrl *tc9563,
>                 {TC9563_NFTS_2_5_GT, nfts[0]},
>                 {TC9563_NFTS_5_GT, nfts[1]},
>         };
> +       struct device *dev = tc9563->pwrctrl.dev;
>         int ret;
> 
>         if (!nfts[0])
>                 return 0;
> 
> +       if (port == TC9563_VDSP) {
> +               dev_dbg(dev, "N_FTS tuning not supported for VDSP\n");
> +               return 0;
> +       }
> +
>         ret =  tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_PORT_SELECT,
>                                         BIT(port));
>         if (ret)
> ```
> 
> @Bjorn: Could you please amend the above diff with this commit?

It didn't apply cleanly (maybe whitespace damage?), but I applied it
by hand:
https://git.kernel.org/cgit/linux/kernel/git/pci/pci.git/commit/?id=b859b9366b44

Re: [PATCH 4/6] PCI/pwrctrl: tc9563: Skip Tx amplitude and DFE tuning for DSP3

[Manivannan Sadhasivam]

On Tue, Jul 28, 2026 at 11:04:57AM -0500, Bjorn Helgaas wrote:
> On Tue, Jul 28, 2026 at 03:25:29PM +0200, Manivannan Sadhasivam wrote:
> > On Mon, Jul 27, 2026 at 10:59:11AM -0500, Alex Elder wrote:
> > > On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > 
> > > > DSP3 doesn't support tuning Tx amplitude and DFE settings. So just skip
> > > > the setting if DT has passed the tuning properties. There is no need to
> > > > error out the whole driver because of it.
> > > > 
> > > > Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > 
> > > So to keep things simple, tc9563_pwrctrl_parse_device_dt() is called
> > > for all downstream ports but you ignore the setting (with a warning)
> > > for those that do not apply to DSP3.
> > > 
> > > What about tc9563_pwrctrl_set_nfts()?
> > 
> > Good catch! I missed adding the check for N_FTS as well:
> > 
> > ```
> > diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > index 9d869483b6b3..978907aa0ce5 100644
> > --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > @@ -410,11 +410,17 @@ static int tc9563_pwrctrl_set_nfts(struct tc9563_pwrctrl *tc9563,
> >                 {TC9563_NFTS_2_5_GT, nfts[0]},
> >                 {TC9563_NFTS_5_GT, nfts[1]},
> >         };
> > +       struct device *dev = tc9563->pwrctrl.dev;
> >         int ret;
> > 
> >         if (!nfts[0])
> >                 return 0;
> > 
> > +       if (port == TC9563_VDSP) {
> > +               dev_dbg(dev, "N_FTS tuning not supported for VDSP\n");
> > +               return 0;
> > +       }
> > +
> >         ret =  tc9563_pwrctrl_i2c_write(tc9563->client, TC9563_PORT_SELECT,
> >                                         BIT(port));
> >         if (ret)
> > ```
> > 
> > @Bjorn: Could you please amend the above diff with this commit?
> 
> It didn't apply cleanly (maybe whitespace damage?), but I applied it
> by hand:
> https://git.kernel.org/cgit/linux/kernel/git/pci/pci.git/commit/?id=b859b9366b44


LGTM, thanks!

- Mani

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