[PATCH net-next 00/12] Add support for PSE port priority

[PATCH net-next 00/12] Add support for PSE port priority

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

This series brings support for port priority in the PSE subsystem.
PSE controllers can set priorities to decide which ports should be
turned off in case of special events like over-current.

This series also adds support for the devm_pse_irq_helper() helper,
similarly to devm_regulator_irq_helper(), to report events and errors.
Wrappers are used to avoid regulator naming in PSE drivers to prevent
confusion.

Patches 1-3: Cosmetics.
Patch 4: Adds support for last supported features in the TPS23881 drivers.
Patches 5-7: Add support for port priority in PSE core and ethtool.
Patches 8-9: Add support for port priority in PD692x0 and TPS23881 drivers.
Patches 10-11: Add support for devm_pse_irq_helper() helper in PSE core and
               ethtool.
Patch 12: Adds support for interrupt and event report in TPS23881 driver.

This patch series is based on the fix sent recently:
https://lore.kernel.org/netdev/20241002121706.246143-1-kory.maincent@bootlin.com/T/#u

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
Kory Maincent (12):
      net: pse-pd: Remove unused pse_ethtool_get_pw_limit function declaration
      net: pse-pd: tps23881: Correct boolean evaluation for bitmask checks
      net: pse-pd: tps23881: Simplify function returns by removing redundant checks
      net: pse-pd: tps23881: Add support for power limit and measurement features
      net: pse-pd: Add support for getting and setting port priority
      net: ethtool: Add PSE new port priority support feature
      netlink: specs: Expand the PSE netlink command with C33 prio attributes
      net: pse-pd: pd692x0: Add support for PSE PI priority feature
      net: pse-pd: tps23881: Add support for PSE PI priority feature
      net: pse-pd: Register regulator even for undescribed PSE PIs
      net: pse-pd: Add support for event reporting using devm_regulator_irq_helper
      net: pse-pd: tps23881: Add support for PSE events and interrupts

 Documentation/netlink/specs/ethtool.yaml     |  11 +
 Documentation/networking/ethtool-netlink.rst |  16 +
 drivers/net/pse-pd/pd692x0.c                 |  23 ++
 drivers/net/pse-pd/pse_core.c                |  66 +++-
 drivers/net/pse-pd/tps23881.c                | 532 +++++++++++++++++++++++++--
 include/linux/pse-pd/pse.h                   |  43 ++-
 include/uapi/linux/ethtool_netlink.h         |   2 +
 net/ethtool/pse-pd.c                         |  18 +
 8 files changed, 674 insertions(+), 37 deletions(-)
---
base-commit: 8052e7ff851b33e77f23800f8d15bafae9f97d17
change-id: 20240913-feature_poe_port_prio-a51aed7332ec

Best regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net-next 01/12] net: pse-pd: Remove unused pse_ethtool_get_pw_limit function declaration

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

Removed the unused pse_ethtool_get_pw_limit() function declaration from
pse.h. This function was declared but never implemented or used,
making the declaration unnecessary.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 include/linux/pse-pd/pse.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index 591a53e082e6..85a08c349256 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -184,8 +184,6 @@ int pse_ethtool_set_config(struct pse_control *psec,
 int pse_ethtool_set_pw_limit(struct pse_control *psec,
 			     struct netlink_ext_ack *extack,
 			     const unsigned int pw_limit);
-int pse_ethtool_get_pw_limit(struct pse_control *psec,
-			     struct netlink_ext_ack *extack);
 
 bool pse_has_podl(struct pse_control *psec);
 bool pse_has_c33(struct pse_control *psec);
@@ -222,12 +220,6 @@ static inline int pse_ethtool_set_pw_limit(struct pse_control *psec,
 	return -EOPNOTSUPP;
 }
 
-static inline int pse_ethtool_get_pw_limit(struct pse_control *psec,
-					   struct netlink_ext_ack *extack)
-{
-	return -EOPNOTSUPP;
-}
-
 static inline bool pse_has_podl(struct pse_control *psec)
 {
 	return false;

-- 
2.34.1

Re: [PATCH net-next 01/12] net: pse-pd: Remove unused pse_ethtool_get_pw_limit function declaration

[Andrew Lunn]

On Wed, Oct 02, 2024 at 06:27:57PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Removed the unused pse_ethtool_get_pw_limit() function declaration from
> pse.h. This function was declared but never implemented or used,
> making the declaration unnecessary.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 01/12] net: pse-pd: Remove unused pse_ethtool_get_pw_limit function declaration

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

On Wed, Oct 2, 2024 at 9:59 PM Kory Maincent <kory.maincent@bootlin.com> wrote:
>
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
>
> Removed the unused pse_ethtool_get_pw_limit() function declaration from
> pse.h. This function was declared but never implemented or used,
> making the declaration unnecessary.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

LGTM,
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>


-- 
Regards,
Kalesh A P

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

Re: [PATCH net-next 01/12] net: pse-pd: Remove unused pse_ethtool_get_pw_limit function declaration

[Oleksij Rempel]

On Wed, Oct 02, 2024 at 06:27:57PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Removed the unused pse_ethtool_get_pw_limit() function declaration from
> pse.h. This function was declared but never implemented or used,
> making the declaration unnecessary.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next 02/12] net: pse-pd: tps23881: Correct boolean evaluation for bitmask checks

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

Update misleading boolean evaluation when checking bitmask values.
The existing code directly assigned the result of bitwise operations
to a boolean variable, which is not consistent with later assignments.
This has been corrected by explicitly converting the bitmask results
to boolean using the !! operator, ensuring proper code consistency

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/tps23881.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index 5c4e88be46ee..1a57c55f8577 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -139,9 +139,9 @@ static int tps23881_pi_is_enabled(struct pse_controller_dev *pcdev, int id)
 
 	chan = priv->port[id].chan[0];
 	if (chan < 4)
-		enabled = ret & BIT(chan);
+		enabled = !!(ret & BIT(chan));
 	else
-		enabled = ret & BIT(chan + 4);
+		enabled = !!(ret & BIT(chan + 4));
 
 	if (priv->port[id].is_4p) {
 		chan = priv->port[id].chan[1];
@@ -172,11 +172,11 @@ static int tps23881_ethtool_get_status(struct pse_controller_dev *pcdev,
 
 	chan = priv->port[id].chan[0];
 	if (chan < 4) {
-		enabled = ret & BIT(chan);
-		delivering = ret & BIT(chan + 4);
+		enabled = !!(ret & BIT(chan));
+		delivering = !!(ret & BIT(chan + 4));
 	} else {
-		enabled = ret & BIT(chan + 4);
-		delivering = ret & BIT(chan + 8);
+		enabled = !!(ret & BIT(chan + 4));
+		delivering = !!(ret & BIT(chan + 8));
 	}
 
 	if (priv->port[id].is_4p) {

-- 
2.34.1

[PATCH net-next 03/12] net: pse-pd: tps23881: Simplify function returns by removing redundant checks

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

Cleaned up several functions in tps23881 by removing redundant checks on
return values at the end of functions. These check has been removed, and
the return statement now directly returns the function result, reducing
the code's complexity and making it more concise.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/tps23881.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index 1a57c55f8577..fdf996f5d1f8 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -118,11 +118,7 @@ static int tps23881_pi_disable(struct pse_controller_dev *pcdev, int id)
 			val |= BIT(chan + 8);
 	}
 
-	ret = i2c_smbus_write_word_data(client, TPS23881_REG_PW_EN, val);
-	if (ret)
-		return ret;
-
-	return 0;
+	return i2c_smbus_write_word_data(client, TPS23881_REG_PW_EN, val);
 }
 
 static int tps23881_pi_is_enabled(struct pse_controller_dev *pcdev, int id)
@@ -488,7 +484,7 @@ tps23881_write_port_matrix(struct tps23881_priv *priv,
 	struct i2c_client *client = priv->client;
 	u8 pi_id, lgcl_chan, hw_chan;
 	u16 val = 0;
-	int i, ret;
+	int i;
 
 	for (i = 0; i < port_cnt; i++) {
 		pi_id = port_matrix[i].pi_id;
@@ -519,11 +515,7 @@ tps23881_write_port_matrix(struct tps23881_priv *priv,
 	}
 
 	/* Write hardware ports matrix */
-	ret = i2c_smbus_write_word_data(client, TPS23881_REG_PORT_MAP, val);
-	if (ret)
-		return ret;
-
-	return 0;
+	return i2c_smbus_write_word_data(client, TPS23881_REG_PORT_MAP, val);
 }
 
 static int
@@ -572,11 +564,7 @@ tps23881_set_ports_conf(struct tps23881_priv *priv,
 			val |= BIT(port_matrix[i].lgcl_chan[1]) |
 			       BIT(port_matrix[i].lgcl_chan[1] + 4);
 	}
-	ret = i2c_smbus_write_word_data(client, TPS23881_REG_DET_CLA_EN, val);
-	if (ret)
-		return ret;
-
-	return 0;
+	return i2c_smbus_write_word_data(client, TPS23881_REG_DET_CLA_EN, val);
 }
 
 static int
@@ -602,11 +590,7 @@ tps23881_set_ports_matrix(struct tps23881_priv *priv,
 	if (ret)
 		return ret;
 
-	ret = tps23881_set_ports_conf(priv, port_matrix);
-	if (ret)
-		return ret;
-
-	return 0;
+	return tps23881_set_ports_conf(priv, port_matrix);
 }
 
 static int tps23881_setup_pi_matrix(struct pse_controller_dev *pcdev)

-- 
2.34.1

Re: [PATCH net-next 03/12] net: pse-pd: tps23881: Simplify function returns by removing redundant checks

[Andrew Lunn]

On Wed, Oct 02, 2024 at 06:27:59PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Cleaned up several functions in tps23881 by removing redundant checks on
> return values at the end of functions. These check has been removed, and
> the return statement now directly returns the function result, reducing
> the code's complexity and making it more concise.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 03/12] net: pse-pd: tps23881: Simplify function returns by removing redundant checks

[Oleksij Rempel]

On Wed, Oct 02, 2024 at 06:27:59PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Cleaned up several functions in tps23881 by removing redundant checks on
> return values at the end of functions. These check has been removed, and
> the return statement now directly returns the function result, reducing
> the code's complexity and making it more concise.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
 
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next 04/12] net: pse-pd: tps23881: Add support for power limit and measurement features

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

Expand PSE callbacks to support the newly introduced
pi_get/set_current_limit() and pi_get_voltage() functions. These callbacks
allow for power limit configuration in the TPS23881 controller.

Additionally, the patch includes the detected class, the current power
delivered and the power limit ranges in the status returned, providing more
comprehensive PoE status reporting.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/tps23881.c | 314 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 314 insertions(+)

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index fdf996f5d1f8..e05b45cdc9f8 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -25,17 +25,29 @@
 #define TPS23881_REG_GEN_MASK	0x17
 #define TPS23881_REG_NBITACC	BIT(5)
 #define TPS23881_REG_PW_EN	0x19
+#define TPS23881_REG_2PAIR_POL1	0x1e
 #define TPS23881_REG_PORT_MAP	0x26
 #define TPS23881_REG_PORT_POWER	0x29
+#define TPS23881_REG_4PAIR_POL1	0x2a
+#define TPS23881_REG_INPUT_V	0x2e
+#define TPS23881_REG_CHAN1_A	0x30
+#define TPS23881_REG_CHAN1_V	0x32
 #define TPS23881_REG_POEPLUS	0x40
 #define TPS23881_REG_TPON	BIT(0)
 #define TPS23881_REG_FWREV	0x41
 #define TPS23881_REG_DEVID	0x43
 #define TPS23881_REG_DEVID_MASK	0xF0
 #define TPS23881_DEVICE_ID	0x02
+#define TPS23881_REG_CHAN1_CLASS	0x4c
 #define TPS23881_REG_SRAM_CTRL	0x60
 #define TPS23881_REG_SRAM_DATA	0x61
 
+#define TPS23881_UV_STEP	3662
+#define TPS23881_MAX_UV		60000000
+#define TPS23881_NA_STEP	70190
+#define TPS23881_MAX_UA		1150000
+#define TPS23881_MW_STEP	500
+
 struct tps23881_port_desc {
 	u8 chan[2];
 	bool is_4p;
@@ -151,6 +163,175 @@ static int tps23881_pi_is_enabled(struct pse_controller_dev *pcdev, int id)
 	return enabled;
 }
 
+static int tps23881_pi_get_voltage(struct pse_controller_dev *pcdev, int id)
+{
+	struct tps23881_priv *priv = to_tps23881_priv(pcdev);
+	struct i2c_client *client = priv->client;
+	int ret, reg;
+	u8 chan;
+	u64 uV;
+
+	/* Read Voltage only at one of the 2-pair ports */
+	chan = priv->port[id].chan[0];
+	if (chan < 4)
+		/* Registers 0x32 0x36 0x3a 0x3e */
+		reg = TPS23881_REG_CHAN1_V + chan * 4;
+	else
+		/* Registers 0x33 0x37 0x3b 0x3f */
+		reg = TPS23881_REG_CHAN1_V + 1 + (chan % 4) * 4;
+
+	ret = i2c_smbus_read_word_data(client, reg);
+	if (ret < 0)
+		return ret;
+
+	uV = ret;
+	uV *= TPS23881_UV_STEP;
+	if (uV > TPS23881_MAX_UV) {
+		dev_err(&client->dev, "voltage read out of range\n");
+		return -ERANGE;
+	}
+
+	return (int)uV;
+}
+
+static int
+tps23881_pi_get_chan_current(struct tps23881_priv *priv, u8 chan)
+{
+	struct i2c_client *client = priv->client;
+	int reg, ret;
+	u64 tmp_64;
+
+	if (chan < 4)
+		/* Registers 0x30 0x34 0x38 0x3c */
+		reg = TPS23881_REG_CHAN1_A + chan * 4;
+	else
+		/* Registers 0x31 0x35 0x39 0x3d */
+		reg = TPS23881_REG_CHAN1_A + 1 + (chan % 4) * 4;
+
+	ret = i2c_smbus_read_word_data(client, reg);
+	if (ret < 0)
+		return ret;
+
+	tmp_64 = ret;
+	tmp_64 *= TPS23881_NA_STEP;
+	/* uA = nA / 1000 */
+	tmp_64 = DIV_ROUND_CLOSEST_ULL(tmp_64, 1000);
+	if (tmp_64 > TPS23881_MAX_UA) {
+		dev_err(&client->dev, "current read out of range\n");
+		return -ERANGE;
+	}
+	return (int)tmp_64;
+}
+
+static int
+tps23881_pi_get_power(struct tps23881_priv *priv, unsigned long id)
+{
+	int ret, uV, uA;
+	u64 tmp_64;
+	u8 chan;
+
+	ret = tps23881_pi_get_voltage(&priv->pcdev, id);
+	if (ret < 0)
+		return ret;
+	uV = ret;
+
+	chan = priv->port[id].chan[0];
+	ret = tps23881_pi_get_chan_current(priv, chan);
+	if (ret < 0)
+		return ret;
+	uA = ret;
+
+	if (priv->port[id].is_4p) {
+		chan = priv->port[id].chan[1];
+		ret = tps23881_pi_get_chan_current(priv, chan);
+		if (ret < 0)
+			return ret;
+		uA += ret;
+	}
+
+	tmp_64 = uV;
+	tmp_64 *= uA;
+	/* mW = uV * uA / 1000000000 */
+	return DIV_ROUND_CLOSEST_ULL(tmp_64, 1000000000);
+}
+
+static int
+tps23881_pi_get_pw_limit_chan(struct tps23881_priv *priv, u8 chan)
+{
+	struct i2c_client *client = priv->client;
+	int ret, reg, mW;
+
+	reg = TPS23881_REG_2PAIR_POL1 + (chan % 4);
+	ret = i2c_smbus_read_word_data(client, reg);
+	if (ret < 0)
+		return ret;
+
+	if (chan < 4)
+		mW = (ret & 0xff) * TPS23881_MW_STEP;
+	else
+		mW = (ret >> 8) * TPS23881_MW_STEP;
+
+	return mW;
+}
+
+static int tps23881_pi_get_pw_limit(struct tps23881_priv *priv, int id)
+{
+	int ret, mW;
+	u8 chan;
+
+	chan = priv->port[id].chan[0];
+	ret = tps23881_pi_get_pw_limit_chan(priv, chan);
+	if (ret < 0)
+		return ret;
+
+	mW = ret;
+	if (priv->port[id].is_4p) {
+		chan = priv->port[id].chan[1];
+		ret = tps23881_pi_get_pw_limit_chan(priv, chan);
+		if (ret < 0)
+			return ret;
+		mW += ret;
+	}
+
+	return mW;
+}
+
+static int tps23881_pi_get_max_pw_limit(struct tps23881_priv *priv, int id)
+{
+	int ret, uV;
+	u64 tmp_64;
+
+	ret = tps23881_pi_get_voltage(&priv->pcdev, id);
+	if (ret < 0)
+		return ret;
+	uV = ret;
+
+	tmp_64 = uV;
+	tmp_64 *= MAX_PI_CURRENT;
+	/* mW = uV * uA / 1000000000 */
+	return DIV_ROUND_CLOSEST_ULL(tmp_64, 1000000000);
+}
+
+static int tps23881_pi_get_class(struct tps23881_priv *priv, int id)
+{
+	struct i2c_client *client = priv->client;
+	int ret, reg, class;
+	u8 chan;
+
+	chan = priv->port[id].chan[0];
+	reg = TPS23881_REG_CHAN1_CLASS + (chan % 4);
+	ret = i2c_smbus_read_word_data(client, reg);
+	if (ret < 0)
+		return ret;
+
+	if (chan < 4)
+		class = ret >> 4;
+	else
+		class = ret >> 12;
+
+	return class;
+}
+
 static int tps23881_ethtool_get_status(struct pse_controller_dev *pcdev,
 				       unsigned long id,
 				       struct netlink_ext_ack *extack,
@@ -198,6 +379,35 @@ static int tps23881_ethtool_get_status(struct pse_controller_dev *pcdev,
 	else
 		status->c33_admin_state = ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED;
 
+	ret = tps23881_pi_get_power(priv, id);
+	if (ret < 0)
+		return ret;
+	status->c33_actual_pw = ret;
+
+	status->c33_pw_limit_ranges = kzalloc(sizeof(*status->c33_pw_limit_ranges),
+					      GFP_KERNEL);
+	if (!status->c33_pw_limit_ranges)
+		return -ENOMEM;
+
+	status->c33_actual_pw = ret;
+
+	ret = tps23881_pi_get_max_pw_limit(priv, id);
+	if (ret < 0)
+		return ret;
+	status->c33_pw_limit_nb_ranges = 1;
+	status->c33_pw_limit_ranges->min = 2000;
+	status->c33_pw_limit_ranges->max = ret;
+
+	ret = tps23881_pi_get_pw_limit(priv, id);
+	if (ret < 0)
+		return ret;
+	status->c33_avail_pw_limit = ret;
+
+	ret = tps23881_pi_get_class(priv, id);
+	if (ret < 0)
+		return ret;
+	status->c33_pw_class = ret;
+
 	return 0;
 }
 
@@ -614,12 +824,116 @@ static int tps23881_setup_pi_matrix(struct pse_controller_dev *pcdev)
 	return ret;
 }
 
+static int tps23881_pi_get_current_limit(struct pse_controller_dev *pcdev,
+					 int id)
+{
+	struct tps23881_priv *priv = to_tps23881_priv(pcdev);
+	int ret, mW, uV;
+	u64 tmp_64;
+
+	ret = tps23881_pi_get_pw_limit(priv, id);
+	if (ret < 0)
+		return ret;
+	mW = ret;
+
+	ret = tps23881_pi_get_voltage(pcdev, id);
+	if (ret < 0)
+		return ret;
+	uV = ret;
+
+	tmp_64 = mW;
+	tmp_64 *= 1000000000ull;
+	/* uA = mW * 1000000000 / uV */
+	return DIV_ROUND_CLOSEST_ULL(tmp_64, uV);
+}
+
+static int
+tps23881_pi_set_2p_pw_limit(struct tps23881_priv *priv, u8 chan, u8 pol)
+{
+	struct i2c_client *client = priv->client;
+	int ret, reg;
+	u16 val;
+
+	reg = TPS23881_REG_2PAIR_POL1 + (chan % 4);
+	ret = i2c_smbus_read_word_data(client, reg);
+	if (ret < 0)
+		return ret;
+
+	if (chan < 4)
+		val = (ret & 0xff00) | pol;
+	else
+		val = (ret & 0xff) | (pol << 8);
+
+	return i2c_smbus_write_word_data(client, reg, val);
+}
+
+static int
+tps23881_pi_set_4p_pw_limit(struct tps23881_priv *priv, u8 chan, u8 pol)
+{
+	struct i2c_client *client = priv->client;
+	int ret, reg;
+	u16 val;
+
+	if ((chan % 4) < 2)
+		reg = TPS23881_REG_4PAIR_POL1;
+	else
+		reg = TPS23881_REG_4PAIR_POL1 + 1;
+
+	ret = i2c_smbus_read_word_data(client, reg);
+	if (ret < 0)
+		return ret;
+
+	if (chan < 4)
+		val = (ret & 0xff00) | pol;
+	else
+		val = (ret & 0xff) | (pol << 8);
+
+	return i2c_smbus_write_word_data(client, reg, val);
+}
+
+static int tps23881_pi_set_current_limit(struct pse_controller_dev *pcdev,
+					 int id, int max_uA)
+{
+	struct tps23881_priv *priv = to_tps23881_priv(pcdev);
+	u8 chan, pw_pol;
+	int ret, mW;
+	u64 tmp_64;
+
+	ret = tps23881_pi_get_voltage(pcdev, id);
+	if (ret < 0)
+		return ret;
+
+	tmp_64 = ret;
+	tmp_64 *= max_uA;
+	/* mW = uV * uA / 1000000000 */
+	mW = DIV_ROUND_CLOSEST_ULL(tmp_64, 1000000000);
+	pw_pol = DIV_ROUND_CLOSEST_ULL(mW, TPS23881_MW_STEP);
+
+	if (priv->port[id].is_4p) {
+		chan = priv->port[id].chan[0];
+		/* One chan is enough to configure the PI power limit */
+		ret = tps23881_pi_set_4p_pw_limit(priv, chan, pw_pol);
+		if (ret < 0)
+			return ret;
+	} else {
+		chan = priv->port[id].chan[0];
+		ret = tps23881_pi_set_2p_pw_limit(priv, chan, pw_pol);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 static const struct pse_controller_ops tps23881_ops = {
 	.setup_pi_matrix = tps23881_setup_pi_matrix,
 	.pi_enable = tps23881_pi_enable,
 	.pi_disable = tps23881_pi_disable,
 	.pi_is_enabled = tps23881_pi_is_enabled,
 	.ethtool_get_status = tps23881_ethtool_get_status,
+	.pi_get_voltage = tps23881_pi_get_voltage,
+	.pi_get_current_limit = tps23881_pi_get_current_limit,
+	.pi_set_current_limit = tps23881_pi_set_current_limit,
 };
 
 static const char fw_parity_name[] = "ti/tps23881/tps23881-parity-14.bin";

-- 
2.34.1

Re: [PATCH net-next 04/12] net: pse-pd: tps23881: Add support for power limit and measurement features

[Andrew Lunn]

On Wed, Oct 02, 2024 at 06:28:00PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Expand PSE callbacks to support the newly introduced
> pi_get/set_current_limit() and pi_get_voltage() functions. These callbacks
> allow for power limit configuration in the TPS23881 controller.
> 
> Additionally, the patch includes the detected class, the current power
> delivered and the power limit ranges in the status returned, providing more
> comprehensive PoE status reporting.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 04/12] net: pse-pd: tps23881: Add support for power limit and measurement features

[Oleksij Rempel]

On Wed, Oct 02, 2024 at 06:28:00PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Expand PSE callbacks to support the newly introduced
> pi_get/set_current_limit() and pi_get_voltage() functions. These callbacks
> allow for power limit configuration in the TPS23881 controller.
> 
> Additionally, the patch includes the detected class, the current power
> delivered and the power limit ranges in the status returned, providing more
> comprehensive PoE status reporting.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

> +static int tps23881_pi_get_class(struct tps23881_priv *priv, int id)
> +{
....
> +	if (chan < 4)
> +		class = ret >> 4;
> +	else
> +		class = ret >> 12;

....
> +tps23881_pi_set_2p_pw_limit(struct tps23881_priv *priv, u8 chan, u8 pol)
> +{
....
> +	reg = TPS23881_REG_2PAIR_POL1 + (chan % 4);
> +	ret = i2c_smbus_read_word_data(client, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (chan < 4)
> +		val = (ret & 0xff00) | pol;
> +	else
> +		val = (ret & 0xff) | (pol << 8);

This is a common pattern in this driver, we read and write two registers
in one run and then calculate bit offset for the channel, can you please
move it in to separate function. This can be done in a separate patch if
you like.

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 04/12] net: pse-pd: tps23881: Add support for power limit and measurement features

[Kory Maincent]

On Wed, 9 Oct 2024 07:02:38 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> On Wed, Oct 02, 2024 at 06:28:00PM +0200, Kory Maincent wrote:
> > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> > 
> > Expand PSE callbacks to support the newly introduced
> > pi_get/set_current_limit() and pi_get_voltage() functions. These callbacks
> > allow for power limit configuration in the TPS23881 controller.
> > 
> > Additionally, the patch includes the detected class, the current power
> > delivered and the power limit ranges in the status returned, providing more
> > comprehensive PoE status reporting.
> > 
> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>  
> 
> > +static int tps23881_pi_get_class(struct tps23881_priv *priv, int id)
> > +{  
> ....
> > +	if (chan < 4)
> > +		class = ret >> 4;
> > +	else
> > +		class = ret >> 12;  
> 
> ....
> > +tps23881_pi_set_2p_pw_limit(struct tps23881_priv *priv, u8 chan, u8 pol)
> > +{  
> ....
> > +	reg = TPS23881_REG_2PAIR_POL1 + (chan % 4);
> > +	ret = i2c_smbus_read_word_data(client, reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (chan < 4)
> > +		val = (ret & 0xff00) | pol;
> > +	else
> > +		val = (ret & 0xff) | (pol << 8);  
> 
> This is a common pattern in this driver, we read and write two registers
> in one run and then calculate bit offset for the channel, can you please
> move it in to separate function. This can be done in a separate patch if
> you like.

The pattern is common but the operations are always different so I didn't found
a clean way of doing it.
Here is a listing of it:
	if (chan < 4)
		class = ret >> 4;
	else
		class = ret >> 12;

	if (chan < 4)
		val = (ret & 0xff00) | pol;
	else
		val = (ret & 0xff) | (pol << 8);  

        if (chan < 4)                                                           
                val = (u16)(ret | BIT(chan));                                   
        else                                                                    
                val = (u16)(ret | BIT(chan + 4));

	if (chan < 4)
		mW = (ret & 0xff) * TPS23881_MW_STEP;
	else
		mW = (ret >> 8) * TPS23881_MW_STEP;


Any idea?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 04/12] net: pse-pd: tps23881: Add support for power limit and measurement features

[Oleksij Rempel]

On Wed, Oct 09, 2024 at 11:05:01AM +0200, Kory Maincent wrote:
> On Wed, 9 Oct 2024 07:02:38 +0200
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > On Wed, Oct 02, 2024 at 06:28:00PM +0200, Kory Maincent wrote:
> > > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> > > 
> > > Expand PSE callbacks to support the newly introduced
> > > pi_get/set_current_limit() and pi_get_voltage() functions. These callbacks
> > > allow for power limit configuration in the TPS23881 controller.
> > > 
> > > Additionally, the patch includes the detected class, the current power
> > > delivered and the power limit ranges in the status returned, providing more
> > > comprehensive PoE status reporting.
> > > 
> > > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>  
> > 
> > > +static int tps23881_pi_get_class(struct tps23881_priv *priv, int id)
> > > +{  
> > ....
> > > +	if (chan < 4)
> > > +		class = ret >> 4;
> > > +	else
> > > +		class = ret >> 12;  
> > 
> > ....
> > > +tps23881_pi_set_2p_pw_limit(struct tps23881_priv *priv, u8 chan, u8 pol)
> > > +{  
> > ....
> > > +	reg = TPS23881_REG_2PAIR_POL1 + (chan % 4);
> > > +	ret = i2c_smbus_read_word_data(client, reg);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	if (chan < 4)
> > > +		val = (ret & 0xff00) | pol;
> > > +	else
> > > +		val = (ret & 0xff) | (pol << 8);  
> > 
> > This is a common pattern in this driver, we read and write two registers
> > in one run and then calculate bit offset for the channel, can you please
> > move it in to separate function. This can be done in a separate patch if
> > you like.
> 
> The pattern is common but the operations are always different so I didn't found
> a clean way of doing it.
> Here is a listing of it:
> 	if (chan < 4)
> 		class = ret >> 4;
> 	else
> 		class = ret >> 12;
> 
> 	if (chan < 4)
> 		val = (ret & 0xff00) | pol;
> 	else
> 		val = (ret & 0xff) | (pol << 8);  
> 
>         if (chan < 4)                                                           
>                 val = (u16)(ret | BIT(chan));                                   
>         else                                                                    
>                 val = (u16)(ret | BIT(chan + 4));
> 
> 	if (chan < 4)
> 		mW = (ret & 0xff) * TPS23881_MW_STEP;
> 	else
> 		mW = (ret >> 8) * TPS23881_MW_STEP;
> 
> 
> Any idea?
> 

something like this:

/*
 * Helper to extract a value from a u16 register value, which is made of two u8 registers.
 * The function calculates the bit offset based on the channel and extracts the relevant
 * bits using a provided field mask.
 *
 * @param reg_val: The u16 register value (composed of two u8 registers).
 * @param chan: The channel number (0-7).
 * @param field_offset: The base bit offset to apply (e.g., 0 or 4).
 * @param field_mask: The mask to apply to extract the required bits.
 * @return: The extracted value for the specific channel.
 */
static u16 tps23881_calc_val(u16 reg_val, u8 chan, u8 field_offset, u16 field_mask)
{
        u8 bit_offset;

        if (chan < 4) {
                bit_offset = field_offset;
        } else {
                bit_offset = field_offset;
                reg_val >>= 8;
        }

        return (reg_val >> bit_offset) & field_mask;
}

/*
 * Helper to combine individual channel values into a u16 register value.
 * The function sets the value for a specific channel in the appropriate position.
 *
 * @param reg_val: The current u16 register value.
 * @param chan: The channel number (0-7).
 * @param field_offset: The base bit offset to apply (e.g., 0 or 4).
 * @param field_mask: The mask to apply for the field (e.g., 0x0F).
 * @param field_val: The value to set for the specific channel (masked by field_mask).
 * @return: The updated u16 register value with the channel value set.
 */
static u16 tps23881_set_val(u16 reg_val, u8 chan, u8 field_offset, u16 field_mask, u16 field_val)
{
        u8 bit_offset;

        field_val &= field_mask;

        if (chan < 4) {
                bit_offset = field_offset;
                reg_val &= ~(field_mask << bit_offset);
                reg_val |= (field_val << bit_offset);
        } else {
                bit_offset = field_offset;
                reg_val &= ~(field_mask << (bit_offset + 8));
                reg_val |= (field_val << (bit_offset + 8));
        }

        return reg_val;
}
 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 04/12] net: pse-pd: tps23881: Add support for power limit and measurement features

[Kory Maincent]

On Wed, 9 Oct 2024 17:16:20 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> > > This is a common pattern in this driver, we read and write two registers
> > > in one run and then calculate bit offset for the channel, can you please
> > > move it in to separate function. This can be done in a separate patch if
> > > you like.  
> > 
> > The pattern is common but the operations are always different so I didn't
> > found a clean way of doing it.
> > Here is a listing of it:
> > 	if (chan < 4)
> > 		class = ret >> 4;
> > 	else
> > 		class = ret >> 12;
> > 
> > 	if (chan < 4)
> > 		val = (ret & 0xff00) | pol;
> > 	else
> > 		val = (ret & 0xff) | (pol << 8);  
> > 
> >         if (chan < 4)
> > val = (u16)(ret | BIT(chan));                                   
> >         else
> > val = (u16)(ret | BIT(chan + 4));
> > 
> > 	if (chan < 4)
> > 		mW = (ret & 0xff) * TPS23881_MW_STEP;
> > 	else
> > 		mW = (ret >> 8) * TPS23881_MW_STEP;
> > 
> > 
> > Any idea?
> >   
> 
> something like this:

Oh thanks, you rock!!
Indeed this should work, thanks for sorting this out.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net-next 05/12] net: pse-pd: Add support for getting and setting port priority

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

This patch introduces the ability to configure the PSE PI port priority.
Port priority is utilized by PSE controllers to determine which ports to
turn off first in scenarios such as power budget exceedance.

The pis_prio_max value is used to define the maximum priority level
supported by the controller. Both the current priority and the maximum
priority are exposed to the user through the pse_ethtool_get_status call.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/pse_core.c | 30 ++++++++++++++++++++++++++++++
 include/linux/pse-pd/pse.h    | 19 +++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index f8e6854781e6..6b3893a3381c 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -750,6 +750,7 @@ static int _pse_ethtool_get_status(struct pse_controller_dev *pcdev,
 		return -EOPNOTSUPP;
 	}
 
+	status->c33_prio_max = pcdev->pis_prio_max;
 	return ops->ethtool_get_status(pcdev, id, extack, status);
 }
 
@@ -898,6 +899,35 @@ int pse_ethtool_set_pw_limit(struct pse_control *psec,
 }
 EXPORT_SYMBOL_GPL(pse_ethtool_set_pw_limit);
 
+int pse_ethtool_set_prio(struct pse_control *psec,
+			 struct netlink_ext_ack *extack,
+			 unsigned int prio)
+{
+	const struct pse_controller_ops *ops;
+	int ret;
+
+	ops = psec->pcdev->ops;
+	if (!ops->pi_set_prio) {
+		NL_SET_ERR_MSG(extack,
+			       "pse driver does not support port priority");
+		return -EOPNOTSUPP;
+	}
+
+	if (prio > psec->pcdev->pis_prio_max) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "priority %d exceed priority max %d",
+				   prio, psec->pcdev->pis_prio_max);
+		return -ERANGE;
+	}
+
+	mutex_lock(&psec->pcdev->lock);
+	ret = ops->pi_set_prio(psec->pcdev, psec->id, prio);
+	mutex_unlock(&psec->pcdev->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pse_ethtool_set_prio);
+
 bool pse_has_podl(struct pse_control *psec)
 {
 	return psec->pcdev->types & ETHTOOL_PSE_PODL;
diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index 85a08c349256..b60fc56923bd 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -50,6 +50,8 @@ struct pse_control_config {
  *	is in charge of the memory allocation.
  * @c33_pw_limit_nb_ranges: number of supported power limit configuration
  *	ranges
+ * @c33_prio_max: max priority allowed for the c33_prio variable value
+ * @c33_prio: priority of the PSE
  */
 struct pse_control_status {
 	enum ethtool_podl_pse_admin_state podl_admin_state;
@@ -62,6 +64,8 @@ struct pse_control_status {
 	u32 c33_avail_pw_limit;
 	struct ethtool_c33_pse_pw_limit_range *c33_pw_limit_ranges;
 	u32 c33_pw_limit_nb_ranges;
+	u32 c33_prio_max;
+	u32 c33_prio;
 };
 
 /**
@@ -81,6 +85,7 @@ struct pse_control_status {
  *			  set_current_limit regulator callback.
  *			  Should not return an error in case of MAX_PI_CURRENT
  *			  current value set.
+ * @pi_set_prio: Configure the PSE PI priority
  */
 struct pse_controller_ops {
 	int (*ethtool_get_status)(struct pse_controller_dev *pcdev,
@@ -95,6 +100,8 @@ struct pse_controller_ops {
 				    int id);
 	int (*pi_set_current_limit)(struct pse_controller_dev *pcdev,
 				    int id, int max_uA);
+	int (*pi_set_prio)(struct pse_controller_dev *pcdev, int id,
+			   unsigned int prio);
 };
 
 struct module;
@@ -150,6 +157,7 @@ struct pse_pi {
  * @types: types of the PSE controller
  * @pi: table of PSE PIs described in this controller device
  * @no_of_pse_pi: flag set if the pse_pis devicetree node is not used
+ * @pis_prio_max: Maximum value allowed for the PSE PIs priority
  */
 struct pse_controller_dev {
 	const struct pse_controller_ops *ops;
@@ -163,6 +171,7 @@ struct pse_controller_dev {
 	enum ethtool_pse_types types;
 	struct pse_pi *pi;
 	bool no_of_pse_pi;
+	unsigned int pis_prio_max;
 };
 
 #if IS_ENABLED(CONFIG_PSE_CONTROLLER)
@@ -184,6 +193,9 @@ int pse_ethtool_set_config(struct pse_control *psec,
 int pse_ethtool_set_pw_limit(struct pse_control *psec,
 			     struct netlink_ext_ack *extack,
 			     const unsigned int pw_limit);
+int pse_ethtool_set_prio(struct pse_control *psec,
+			 struct netlink_ext_ack *extack,
+			 unsigned int prio);
 
 bool pse_has_podl(struct pse_control *psec);
 bool pse_has_c33(struct pse_control *psec);
@@ -220,6 +232,13 @@ static inline int pse_ethtool_set_pw_limit(struct pse_control *psec,
 	return -EOPNOTSUPP;
 }
 
+static inline int pse_ethtool_set_prio(struct pse_control *psec,
+				       struct netlink_ext_ack *extack,
+				       unsigned int prio)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline bool pse_has_podl(struct pse_control *psec)
 {
 	return false;

-- 
2.34.1

Re: [PATCH net-next 05/12] net: pse-pd: Add support for getting and setting port priority

[Andrew Lunn]

On Wed, Oct 02, 2024 at 06:28:01PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> This patch introduces the ability to configure the PSE PI port priority.
> Port priority is utilized by PSE controllers to determine which ports to
> turn off first in scenarios such as power budget exceedance.
> 
> The pis_prio_max value is used to define the maximum priority level
> supported by the controller. Both the current priority and the maximum
> priority are exposed to the user through the pse_ethtool_get_status call.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 05/12] net: pse-pd: Add support for getting and setting port priority

[Oleksij Rempel]

On Wed, Oct 02, 2024 at 06:28:01PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> This patch introduces the ability to configure the PSE PI port priority.
> Port priority is utilized by PSE controllers to determine which ports to
> turn off first in scenarios such as power budget exceedance.
> 
> The pis_prio_max value is used to define the maximum priority level
> supported by the controller. Both the current priority and the maximum
> priority are exposed to the user through the pse_ethtool_get_status call.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next 06/12] net: ethtool: Add PSE new port priority support feature

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

This patch expands the status information provided by ethtool for PSE c33
with current port priority and max port priority. It also adds a call to
pse_ethtool_set_prio() to configure the PSE port priority.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 Documentation/networking/ethtool-netlink.rst | 16 ++++++++++++++++
 include/uapi/linux/ethtool_netlink.h         |  2 ++
 net/ethtool/pse-pd.c                         | 18 ++++++++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 295563e91082..15208429a973 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1763,6 +1763,10 @@ Kernel response contents:
                                                       limit of the PoE PSE.
   ``ETHTOOL_A_C33_PSE_PW_LIMIT_RANGES``       nested  Supported power limit
                                                       configuration ranges.
+  ``ETHTOOL_A_C33_PSE_PRIO_MAX``                 u32  priority maximum configurable
+                                                      on the PoE PSE
+  ``ETHTOOL_A_C33_PSE_PRIO``                     u32  priority of the PoE PSE
+                                                      currently configured
   ==========================================  ======  =============================
 
 When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_STATE`` attribute identifies
@@ -1836,6 +1840,12 @@ identifies the C33 PSE power limit ranges through
 If the controller works with fixed classes, the min and max values will be
 equal.
 
+When set, the optional ``ETHTOOL_A_C33_PSE_PRIO_MAX`` attribute identifies
+the C33 PSE maximum priority value.
+
+When set, the optional ``ETHTOOL_A_C33_PSE_PRIO`` attributes is used to
+identifies the currently configured C33 PSE priority.
+
 PSE_SET
 =======
 
@@ -1849,6 +1859,8 @@ Request contents:
   ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL``        u32  Control PSE Admin state
   ``ETHTOOL_A_C33_PSE_AVAIL_PWR_LIMIT``      u32  Control PoE PSE available
                                                   power limit
+  ``ETHTOOL_A_C33_PSE_PRIO``                 u32  Control priority of the
+                                                  PoE PSE
   ======================================  ======  =============================
 
 When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` attribute is used
@@ -1871,6 +1883,10 @@ various existing products that document power consumption in watts rather than
 classes. If power limit configuration based on classes is needed, the
 conversion can be done in user space, for example by ethtool.
 
+When set, the optional ``ETHTOOL_A_C33_PSE_PRIO`` attributes is used to
+control the C33 PSE priority. Allowed priority value are between zero
+and the value of ``ETHTOOL_A_C33_PSE_PRIO_MAX`` attribute.
+
 RSS_GET
 =======
 
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 283305f6b063..874a4bca2e19 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -970,6 +970,8 @@ enum {
 	ETHTOOL_A_C33_PSE_EXT_SUBSTATE,		/* u32 */
 	ETHTOOL_A_C33_PSE_AVAIL_PW_LIMIT,	/* u32 */
 	ETHTOOL_A_C33_PSE_PW_LIMIT_RANGES,	/* nest - _C33_PSE_PW_LIMIT_* */
+	ETHTOOL_A_C33_PSE_PRIO_MAX,		/* u32 */
+	ETHTOOL_A_C33_PSE_PRIO,			/* u32 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_PSE_CNT,
diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index a0705edca22a..439739eaf2ed 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -109,6 +109,9 @@ static int pse_reply_size(const struct ethnl_req_info *req_base,
 		len += st->c33_pw_limit_nb_ranges *
 		       (nla_total_size(0) +
 			nla_total_size(sizeof(u32)) * 2);
+	if (st->c33_prio_max)
+		/* _C33_PSE_PRIO_MAX + _C33_PSE_PRIO */
+		len += nla_total_size(sizeof(u32)) * 2;
 
 	return len;
 }
@@ -198,6 +201,11 @@ static int pse_fill_reply(struct sk_buff *skb,
 	    pse_put_pw_limit_ranges(skb, st))
 		return -EMSGSIZE;
 
+	if (st->c33_prio_max > 0 &&
+	    (nla_put_u32(skb, ETHTOOL_A_C33_PSE_PRIO_MAX, st->c33_prio_max) ||
+	     nla_put_u32(skb, ETHTOOL_A_C33_PSE_PRIO, st->c33_prio)))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
@@ -219,6 +227,7 @@ const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1] = {
 		NLA_POLICY_RANGE(NLA_U32, ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED,
 				 ETHTOOL_C33_PSE_ADMIN_STATE_ENABLED),
 	[ETHTOOL_A_C33_PSE_AVAIL_PW_LIMIT] = { .type = NLA_U32 },
+	[ETHTOOL_A_C33_PSE_PRIO] = { .type = NLA_U32 },
 };
 
 static int
@@ -267,6 +276,15 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 	if (ret)
 		return ret;
 
+	if (tb[ETHTOOL_A_C33_PSE_PRIO]) {
+		unsigned int prio;
+
+		prio = nla_get_u32(tb[ETHTOOL_A_C33_PSE_PRIO]);
+		ret = pse_ethtool_set_prio(phydev->psec, info->extack, prio);
+		if (ret)
+			return ret;
+	}
+
 	if (tb[ETHTOOL_A_C33_PSE_AVAIL_PW_LIMIT]) {
 		unsigned int pw_limit;
 

-- 
2.34.1

Re: [PATCH net-next 06/12] net: ethtool: Add PSE new port priority support feature

[Andrew Lunn]

On Wed, Oct 02, 2024 at 06:28:02PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> This patch expands the status information provided by ethtool for PSE c33
> with current port priority and max port priority. It also adds a call to
> pse_ethtool_set_prio() to configure the PSE port priority.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 06/12] net: ethtool: Add PSE new port priority support feature

[Oleksij Rempel]

On Wed, Oct 02, 2024 at 06:28:02PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> This patch expands the status information provided by ethtool for PSE c33
> with current port priority and max port priority. It also adds a call to
> pse_ethtool_set_prio() to configure the PSE port priority.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  Documentation/networking/ethtool-netlink.rst | 16 ++++++++++++++++
>  include/uapi/linux/ethtool_netlink.h         |  2 ++
>  net/ethtool/pse-pd.c                         | 18 ++++++++++++++++++
>  3 files changed, 36 insertions(+)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 295563e91082..15208429a973 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -1763,6 +1763,10 @@ Kernel response contents:
>                                                        limit of the PoE PSE.
>    ``ETHTOOL_A_C33_PSE_PW_LIMIT_RANGES``       nested  Supported power limit
>                                                        configuration ranges.
> +  ``ETHTOOL_A_C33_PSE_PRIO_MAX``                 u32  priority maximum configurable
> +                                                      on the PoE PSE
> +  ``ETHTOOL_A_C33_PSE_PRIO``                     u32  priority of the PoE PSE
> +                                                      currently configured
>    ==========================================  ======  =============================
>  
>  When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_STATE`` attribute identifies
> @@ -1836,6 +1840,12 @@ identifies the C33 PSE power limit ranges through
>  If the controller works with fixed classes, the min and max values will be
>  equal.
>  
> +When set, the optional ``ETHTOOL_A_C33_PSE_PRIO_MAX`` attribute identifies
> +the C33 PSE maximum priority value.
> +
> +When set, the optional ``ETHTOOL_A_C33_PSE_PRIO`` attributes is used to
> +identifies the currently configured C33 PSE priority.
> +
>  PSE_SET
>  =======
>  
> @@ -1849,6 +1859,8 @@ Request contents:
>    ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL``        u32  Control PSE Admin state
>    ``ETHTOOL_A_C33_PSE_AVAIL_PWR_LIMIT``      u32  Control PoE PSE available
>                                                    power limit
> +  ``ETHTOOL_A_C33_PSE_PRIO``                 u32  Control priority of the
> +                                                  PoE PSE
>    ======================================  ======  =============================
>  
>  When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` attribute is used
> @@ -1871,6 +1883,10 @@ various existing products that document power consumption in watts rather than
>  classes. If power limit configuration based on classes is needed, the
>  conversion can be done in user space, for example by ethtool.
>  
> +When set, the optional ``ETHTOOL_A_C33_PSE_PRIO`` attributes is used to
> +control the C33 PSE priority. Allowed priority value are between zero
> +and the value of ``ETHTOOL_A_C33_PSE_PRIO_MAX`` attribute.
 
We need to introduce a new attribute to effectively manage PSE priorities. With
the addition of the `ETHTOOL_A_C33_PSE_PRIO` attribute for setting priorities,
it's important to know which PSE controller or domain each port belongs to.

Initially, we might consider using a PSE controller index, such as
`ETHTOOL_A_PSE_CONTROLLER_ID`, to identify the specific PSE controller
associated with each port.

However, using just the PSE controller index is too limiting. Here's why:

- Typical PSE controllers handle priorities only within themselves. They
usually can't manage prioritization across different controllers unless they
are part of the same power domain. In systems where multiple PSE controllers
cooperate—either directly or through software mechanisms like the regulator
framework—controllers might share power domains or manage priorities together.
This means priorities are not confined to individual controllers but are
relevant within shared power domains.

- As systems become more complex, with controllers that can work together,
relying solely on a controller index won't accommodate these cooperative
scenarios.

To address these issues, we should use a power domain identifier instead. I
suggest introducing a new attribute called `ETHTOOL_A_PSE_POWER_DOMAIN_ID`.

- It specifies the power domain to which each port belongs, ensuring that
priorities are managed correctly within that domain.

- It accommodates systems where controllers cooperate and share power
resources, allowing for proper coordination of priorities across controllers
within the same power domain.

- It provides flexibility for future developments where controllers might work
together in new ways, preventing limitations that would arise from using a
strict controller index.

However, to provide comprehensive information, it would be beneficial to use
both attributes:

- `ETHTOOL_A_PSE_CONTROLLER_ID` to identify the specific PSE controller
associated with each port.

- `ETHTOOL_A_PSE_POWER_DOMAIN_ID` to specify the power domain to which each
port belongs.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 06/12] net: ethtool: Add PSE new port priority support feature

[Kory Maincent]

On Sat, 5 Oct 2024 08:26:36 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> >  When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` attribute is
> > used @@ -1871,6 +1883,10 @@ various existing products that document power
> > consumption in watts rather than classes. If power limit configuration
> > based on classes is needed, the conversion can be done in user space, for
> > example by ethtool. 
> > +When set, the optional ``ETHTOOL_A_C33_PSE_PRIO`` attributes is used to
> > +control the C33 PSE priority. Allowed priority value are between zero
> > +and the value of ``ETHTOOL_A_C33_PSE_PRIO_MAX`` attribute.  
>  
> We need to introduce a new attribute to effectively manage PSE priorities.
> With the addition of the `ETHTOOL_A_C33_PSE_PRIO` attribute for setting
> priorities, it's important to know which PSE controller or domain each port
> belongs to.
> 
> Initially, we might consider using a PSE controller index, such as
> `ETHTOOL_A_PSE_CONTROLLER_ID`, to identify the specific PSE controller
> associated with each port.
> 
> However, using just the PSE controller index is too limiting. Here's why:
> 
> - Typical PSE controllers handle priorities only within themselves. They
> usually can't manage prioritization across different controllers unless they
> are part of the same power domain. In systems where multiple PSE controllers
> cooperate—either directly or through software mechanisms like the regulator
> framework—controllers might share power domains or manage priorities together.
> This means priorities are not confined to individual controllers but are
> relevant within shared power domains.
> 
> - As systems become more complex, with controllers that can work together,
> relying solely on a controller index won't accommodate these cooperative
> scenarios.
> 
> To address these issues, we should use a power domain identifier instead. I
> suggest introducing a new attribute called `ETHTOOL_A_PSE_POWER_DOMAIN_ID`.
> 
> - It specifies the power domain to which each port belongs, ensuring that
> priorities are managed correctly within that domain.
> 
> - It accommodates systems where controllers cooperate and share power
> resources, allowing for proper coordination of priorities across controllers
> within the same power domain.
> 
> - It provides flexibility for future developments where controllers might work
> together in new ways, preventing limitations that would arise from using a
> strict controller index.
> 
> However, to provide comprehensive information, it would be beneficial to use
> both attributes:
> 
> - `ETHTOOL_A_PSE_CONTROLLER_ID` to identify the specific PSE controller
> associated with each port.
> 
> - `ETHTOOL_A_PSE_POWER_DOMAIN_ID` to specify the power domain to which each
> port belongs.

Currently the priority is managed by the PSE controller so the port is the only
information needed. The user interface is ethtool, and I don't see why he would
need such things like controller id or power domain id. Instead, it could be
managed by the PSE core depending on the power domains described in the
devicetree. The user only wants to know if he can allow a specific power budget
on a Ethernet port and configure port priority in case of over power-budget
event.

I don't have hardware with several PSE controllers. Is there already such
hardware existing in the market?
This seems like an interesting idea but I think it would belong in another patch
series.
Still, it is good to talk about it for future development idea.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 06/12] net: ethtool: Add PSE new port priority support feature

[Oleksij Rempel]

On Mon, Oct 07, 2024 at 11:30:26AM +0200, Kory Maincent wrote:
> On Sat, 5 Oct 2024 08:26:36 +0200
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > >  When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` attribute is
> > > used @@ -1871,6 +1883,10 @@ various existing products that document power
> > > consumption in watts rather than classes. If power limit configuration
> > > based on classes is needed, the conversion can be done in user space, for
> > > example by ethtool. 
> > > +When set, the optional ``ETHTOOL_A_C33_PSE_PRIO`` attributes is used to
> > > +control the C33 PSE priority. Allowed priority value are between zero
> > > +and the value of ``ETHTOOL_A_C33_PSE_PRIO_MAX`` attribute.  
> >  
> > We need to introduce a new attribute to effectively manage PSE priorities.
> > With the addition of the `ETHTOOL_A_C33_PSE_PRIO` attribute for setting
> > priorities, it's important to know which PSE controller or domain each port
> > belongs to.
> > 
> > Initially, we might consider using a PSE controller index, such as
> > `ETHTOOL_A_PSE_CONTROLLER_ID`, to identify the specific PSE controller
> > associated with each port.
> > 
> > However, using just the PSE controller index is too limiting. Here's why:
> > 
> > - Typical PSE controllers handle priorities only within themselves. They
> > usually can't manage prioritization across different controllers unless they
> > are part of the same power domain. In systems where multiple PSE controllers
> > cooperate—either directly or through software mechanisms like the regulator
> > framework—controllers might share power domains or manage priorities together.
> > This means priorities are not confined to individual controllers but are
> > relevant within shared power domains.
> > 
> > - As systems become more complex, with controllers that can work together,
> > relying solely on a controller index won't accommodate these cooperative
> > scenarios.
> > 
> > To address these issues, we should use a power domain identifier instead. I
> > suggest introducing a new attribute called `ETHTOOL_A_PSE_POWER_DOMAIN_ID`.
> > 
> > - It specifies the power domain to which each port belongs, ensuring that
> > priorities are managed correctly within that domain.
> > 
> > - It accommodates systems where controllers cooperate and share power
> > resources, allowing for proper coordination of priorities across controllers
> > within the same power domain.
> > 
> > - It provides flexibility for future developments where controllers might work
> > together in new ways, preventing limitations that would arise from using a
> > strict controller index.
> > 
> > However, to provide comprehensive information, it would be beneficial to use
> > both attributes:
> > 
> > - `ETHTOOL_A_PSE_CONTROLLER_ID` to identify the specific PSE controller
> > associated with each port.
> > 
> > - `ETHTOOL_A_PSE_POWER_DOMAIN_ID` to specify the power domain to which each
> > port belongs.
> 
> Currently the priority is managed by the PSE controller so the port is the only
> information needed. The user interface is ethtool, and I don't see why he would
> need such things like controller id or power domain id. Instead, it could be
> managed by the PSE core depending on the power domains described in the
> devicetree. The user only wants to know if he can allow a specific power budget
> on a Ethernet port and configure port priority in case of over power-budget
> event.

Budget is important but different topic. If user do not know how much
the budget is, there is nothing usable user can configure. Imagine you
do not know how much money can spend and the only way to find it out is
by baying things.

But, budget is the secondary topic withing context of this patch set.
The primer topic here is the prioritization, so the information user
need to know it the context: do A has higher prio in relation to B? Do A
and B actually in the same domain?


> I don't have hardware with several PSE controllers. Is there already such
> hardware existing in the market?

Please correct me if i'm wrong, but in case of pd692x0 based devices,
every manager (for example PD69208M) is own power domain. There are
following limiting factors:
                          PI 1
                   L4    /
		 PD69208M - PI 2
              L3 //      \
 L1      L2     //        PI 3
PSU ============'
                \\        PI 4
                 \\      /
		 PD69208M - PI 5
                         \
			  PI 6

L1 - limits defined by Power Supply Unit
L2 - Limits defined by main supply rail ob PCB
L3 - Limits defined by rail attached to one specific manager
L4 - Limits defined by manager. In case of PD69208M it is Max 0.627A
(for all or per port?)

Assuming PSU provides enough budget to covert Max supported current for
every manager, then the limiting factor is actual manager. It means,
setting prio for PI 4 in relation to PI 1 makes no real sense, because
it is in different power domain.

User will not understand why devices fail to provide enough power by
attaching two device to one domain and not failing by attaching to
different domains. Except we provide this information to the user space.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 06/12] net: ethtool: Add PSE new port priority support feature

[Kory Maincent]

On Mon, 7 Oct 2024 16:10:33 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> > 
> > Currently the priority is managed by the PSE controller so the port is the
> > only information needed. The user interface is ethtool, and I don't see why
> > he would need such things like controller id or power domain id. Instead,
> > it could be managed by the PSE core depending on the power domains
> > described in the devicetree. The user only wants to know if he can allow a
> > specific power budget on a Ethernet port and configure port priority in
> > case of over power-budget event.  
> 
> Budget is important but different topic. If user do not know how much
> the budget is, there is nothing usable user can configure. Imagine you
> do not know how much money can spend and the only way to find it out is
> by baying things.

Yes I agree, but I thought this could be done at the driver level specified in
the power limit ranges for now.
I don't really know the Power Domain API but I don't think it can currently
support what you are expecting for PSE. Maybe through the regulator API, or
something specific to PSE API.
Maybe we should define the power domain PSE concept as it seems something PSE
specific.

> But, budget is the secondary topic withing context of this patch set.
> The primer topic here is the prioritization, so the information user
> need to know it the context: do A has higher prio in relation to B? Do A
> and B actually in the same domain?
> 
> 
> > I don't have hardware with several PSE controllers. Is there already such
> > hardware existing in the market?  
> 
> Please correct me if i'm wrong, but in case of pd692x0 based devices,
> every manager (for example PD69208M) is own power domain. There are
> following limiting factors:
>                           PI 1
>                    L4    /
> 		 PD69208M - PI 2
>               L3 //      \
>  L1      L2     //        PI 3
> PSU ============'
>                 \\        PI 4
>                  \\      /
> 		 PD69208M - PI 5
>                          \
> 			  PI 6
> 
> L1 - limits defined by Power Supply Unit
> L2 - Limits defined by main supply rail ob PCB
> L3 - Limits defined by rail attached to one specific manager
> L4 - Limits defined by manager. In case of PD69208M it is Max 0.627A
> (for all or per port?)

Should the rail really have an impact on power limit? I am not a hardware
designer but having limit defined by the rails seems the best way to create
magic smoke.
Don't know how you find this 0.627A value but it seems a bit low. Port current
limit is 1300mA according to the datasheet.

I first though that hardware should support all ports being powered at the same
time. Indeed this might not be the case be and there is a command to configure
the power bank (PD69208M) power limit.
 
> Assuming PSU provides enough budget to covert Max supported current for
> every manager, then the limiting factor is actual manager. It means,
> setting prio for PI 4 in relation to PI 1 makes no real sense, because
> it is in different power domain.

In fact it does for our case as the PD692x0 consider all the ports in the same
power domain. There is no mention of port priority per PD69208M.
We can only get PD69208M events and status.

> User will not understand why devices fail to provide enough power by
> attaching two device to one domain and not failing by attaching to
> different domains. Except we provide this information to the user space.

What you are explaining seems neat on the paper but I don't know the best way
to implement it. It needs more brainstorming.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 06/12] net: ethtool: Add PSE new port priority support feature

[Kory Maincent]

On Tue, 8 Oct 2024 12:23:00 +0200
Kory Maincent <kory.maincent@bootlin.com> wrote:

> On Mon, 7 Oct 2024 16:10:33 +0200
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > User will not understand why devices fail to provide enough power by
> > attaching two device to one domain and not failing by attaching to
> > different domains. Except we provide this information to the user space.
> 
> What you are explaining seems neat on the paper but I don't know the best way
> to implement it. It needs more brainstorming.

Is it ok for you if we go further with this patch series and continue talking
about PSE power domain alongside?
It should not be necessary to be supported with port priority as the two PSE
supported controller can behave autonomously on a power domain.
I hope I will have time in the project to add its support when we will have a
more precise idea of how.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 06/12] net: ethtool: Add PSE new port priority support feature

[Oleksij Rempel]

On Tue, Oct 08, 2024 at 02:56:17PM +0200, Kory Maincent wrote:
> On Tue, 8 Oct 2024 12:23:00 +0200
> Kory Maincent <kory.maincent@bootlin.com> wrote:
> 
> > On Mon, 7 Oct 2024 16:10:33 +0200
> > Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > 
> > > User will not understand why devices fail to provide enough power by
> > > attaching two device to one domain and not failing by attaching to
> > > different domains. Except we provide this information to the user space.
> > 
> > What you are explaining seems neat on the paper but I don't know the best way
> > to implement it. It needs more brainstorming.
> 
> Is it ok for you if we go further with this patch series and continue talking
> about PSE power domain alongside?
> It should not be necessary to be supported with port priority as the two PSE
> supported controller can behave autonomously on a power domain.
> I hope I will have time in the project to add its support when we will have a
> more precise idea of how.

Ok.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 06/12] net: ethtool: Add PSE new port priority support feature

[Oleksij Rempel]

On Wed, Oct 02, 2024 at 06:28:02PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
...
>  PSE_SET
>  =======
>  
> @@ -1849,6 +1859,8 @@ Request contents:
>    ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL``        u32  Control PSE Admin state
>    ``ETHTOOL_A_C33_PSE_AVAIL_PWR_LIMIT``      u32  Control PoE PSE available
>                                                    power limit
> +  ``ETHTOOL_A_C33_PSE_PRIO``                 u32  Control priority of the
> +                                                  PoE PSE
>    ======================================  ======  =============================
>  
>  When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` attribute is used
> @@ -1871,6 +1883,10 @@ various existing products that document power consumption in watts rather than
>  classes. If power limit configuration based on classes is needed, the
>  conversion can be done in user space, for example by ethtool.
>  
> +When set, the optional ``ETHTOOL_A_C33_PSE_PRIO`` attributes is used to
> +control the C33 PSE priority. Allowed priority value are between zero
> +and the value of ``ETHTOOL_A_C33_PSE_PRIO_MAX`` attribute.
 
Please extend the documentation with expected system behavior: the
meaning of lowest and highest values, power up and shutdown behaviors,
etc.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next 07/12] netlink: specs: Expand the PSE netlink command with C33 prio attributes

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

Expand the c33 PSE attributes with priority and priority max to be able to
set and get the PSE Power Interface priority.

./ynl/cli.py --spec netlink/specs/ethtool.yaml --no-schema --do pse-get
             --json '{"header":{"dev-name":"eth1"}}'
{'c33-pse-actual-pw': 1700,
 'c33-pse-admin-state': 3,
 'c33-pse-avail-pw-limit': 97500,
 'c33-pse-prio': 2,
 'c33-pse-prio-max': 2,
 'c33-pse-pw-class': 4,
 'c33-pse-pw-d-status': 4,
 'c33-pse-pw-limit-ranges': [{'max': 18100, 'min': 15000},
                             {'max': 38000, 'min': 30000},
                             {'max': 65000, 'min': 60000},
                             {'max': 97500, 'min': 90000}],
 'header': {'dev-index': 5, 'dev-name': 'eth1'}}

./ynl/cli.py --spec netlink/specs/ethtool.yaml --no-schema --do pse-set
             --json '{"header":{"dev-name":"eth1"},
                      "c33-pse-prio":1}'
None

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 6a050d755b9c..e2967645fbf2 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -1020,6 +1020,14 @@ attribute-sets:
         type: nest
         multi-attr: true
         nested-attributes: c33-pse-pw-limit
+      -
+        name: c33-pse-prio-max
+        type: u32
+        name-prefix: ethtool-a-
+      -
+        name: c33-pse-prio
+        type: u32
+        name-prefix: ethtool-a-
   -
     name: rss
     attributes:
@@ -1776,6 +1784,8 @@ operations:
             - c33-pse-ext-substate
             - c33-pse-avail-pw-limit
             - c33-pse-pw-limit-ranges
+            - c33-pse-prio-max
+            - c33-pse-prio
       dump: *pse-get-op
     -
       name: pse-set
@@ -1790,6 +1800,7 @@ operations:
             - podl-pse-admin-control
             - c33-pse-admin-control
             - c33-pse-avail-pw-limit
+            - c33-pse-prio
     -
       name: rss-get
       doc: Get RSS params.

-- 
2.34.1

Re: [PATCH net-next 07/12] netlink: specs: Expand the PSE netlink command with C33 prio attributes

[Donald Hunter]

Kory Maincent <kory.maincent@bootlin.com> writes:

> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
>
> Expand the c33 PSE attributes with priority and priority max to be able to
> set and get the PSE Power Interface priority.
>
> ./ynl/cli.py --spec netlink/specs/ethtool.yaml --no-schema --do pse-get
>              --json '{"header":{"dev-name":"eth1"}}'
> {'c33-pse-actual-pw': 1700,
>  'c33-pse-admin-state': 3,
>  'c33-pse-avail-pw-limit': 97500,
>  'c33-pse-prio': 2,
>  'c33-pse-prio-max': 2,
>  'c33-pse-pw-class': 4,
>  'c33-pse-pw-d-status': 4,
>  'c33-pse-pw-limit-ranges': [{'max': 18100, 'min': 15000},
>                              {'max': 38000, 'min': 30000},
>                              {'max': 65000, 'min': 60000},
>                              {'max': 97500, 'min': 90000}],
>  'header': {'dev-index': 5, 'dev-name': 'eth1'}}
>
> ./ynl/cli.py --spec netlink/specs/ethtool.yaml --no-schema --do pse-set
>              --json '{"header":{"dev-name":"eth1"},
>                       "c33-pse-prio":1}'
> None
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

[PATCH net-next 08/12] net: pse-pd: pd692x0: Add support for PSE PI priority feature

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

This patch extends the PSE callbacks by adding support for the newly
introduced pi_set_prio() callback, enabling the configuration of PSE PI
priorities. The current port priority is now also included in the status
information returned to users.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/pd692x0.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
index 0af7db80b2f8..3a4a9836d621 100644
--- a/drivers/net/pse-pd/pd692x0.c
+++ b/drivers/net/pse-pd/pd692x0.c
@@ -685,6 +685,8 @@ static int pd692x0_ethtool_get_status(struct pse_controller_dev *pcdev,
 	if (ret < 0)
 		return ret;
 	status->c33_avail_pw_limit = ret;
+	/* PSE core priority start at 0 */
+	status->c33_prio = buf.data[2] - 1;
 
 	memset(&buf, 0, sizeof(buf));
 	msg = pd692x0_msg_template_list[PD692X0_MSG_GET_PORT_CLASS];
@@ -1061,6 +1063,25 @@ static int pd692x0_pi_set_current_limit(struct pse_controller_dev *pcdev,
 	return pd692x0_sendrecv_msg(priv, &msg, &buf);
 }
 
+static int pd692x0_pi_set_prio(struct pse_controller_dev *pcdev, int id,
+			       unsigned int prio)
+{
+	struct pd692x0_priv *priv = to_pd692x0_priv(pcdev);
+	struct pd692x0_msg msg, buf = {0};
+	int ret;
+
+	ret = pd692x0_fw_unavailable(priv);
+	if (ret)
+		return ret;
+
+	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_PORT_PARAM];
+	msg.sub[2] = id;
+	/* Controller priority from 1 to 3 */
+	msg.data[4] = prio + 1;
+
+	return pd692x0_sendrecv_msg(priv, &msg, &buf);
+}
+
 static const struct pse_controller_ops pd692x0_ops = {
 	.setup_pi_matrix = pd692x0_setup_pi_matrix,
 	.ethtool_get_status = pd692x0_ethtool_get_status,
@@ -1070,6 +1091,7 @@ static const struct pse_controller_ops pd692x0_ops = {
 	.pi_get_voltage = pd692x0_pi_get_voltage,
 	.pi_get_current_limit = pd692x0_pi_get_current_limit,
 	.pi_set_current_limit = pd692x0_pi_set_current_limit,
+	.pi_set_prio = pd692x0_pi_set_prio,
 };
 
 #define PD692X0_FW_LINE_MAX_SZ 0xff
@@ -1486,6 +1508,7 @@ static int pd692x0_i2c_probe(struct i2c_client *client)
 	priv->pcdev.ops = &pd692x0_ops;
 	priv->pcdev.dev = dev;
 	priv->pcdev.types = ETHTOOL_PSE_C33;
+	priv->pcdev.pis_prio_max = 2;
 	ret = devm_pse_controller_register(dev, &priv->pcdev);
 	if (ret)
 		return dev_err_probe(dev, ret,

-- 
2.34.1

Re: [PATCH net-next 08/12] net: pse-pd: pd692x0: Add support for PSE PI priority feature

[Andrew Lunn]

> +	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_PORT_PARAM];
> +	msg.sub[2] = id;
> +	/* Controller priority from 1 to 3 */
> +	msg.data[4] = prio + 1;

Does 0 have a meaning? It just seems an odd design if it does not.

	Andrew

Re: [PATCH net-next 08/12] net: pse-pd: pd692x0: Add support for PSE PI priority feature

[Kory Maincent]

On Thu, 3 Oct 2024 01:41:02 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > +	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_PORT_PARAM];
> > +	msg.sub[2] = id;
> > +	/* Controller priority from 1 to 3 */
> > +	msg.data[4] = prio + 1;  
> 
> Does 0 have a meaning? It just seems an odd design if it does not.

PD692x0 has an odd firmware design from the beginning. ;)
Yes, the priority available are from 1 to 3. Setting it to 0 does nothing.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 08/12] net: pse-pd: pd692x0: Add support for PSE PI priority feature

[Oleksij Rempel]

On Thu, Oct 03, 2024 at 01:41:02AM +0200, Andrew Lunn wrote:
> > +	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_PORT_PARAM];
> > +	msg.sub[2] = id;
> > +	/* Controller priority from 1 to 3 */
> > +	msg.data[4] = prio + 1;
> 
> Does 0 have a meaning? It just seems an odd design if it does not.

0 is not documented. But there are sub-priority which are not directly
configured by user, but affect the system behavior.

Priority#: Critical – 1; high – 2; low – 3
 For ports with the same priority, the PoE Controller sets the
 sub-priority according to the logic port number. (Lower number gets
 higher priority).

Port priority affects:
1. Power-up order: After a reset, the ports are powered up according to
 their priority, highest to lowest, highest priority will power up first.
2. Shutdown order: When exceeding the power budget, lowest priority
 ports will turn off first.

Should we return sub priorities on the prio get request?

If i see it correctly, even if user do not actively configures priorities,
they are always present. For example port 0 will have always a Prio
higher than Port 10.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 08/12] net: pse-pd: pd692x0: Add support for PSE PI priority feature

[Kory Maincent]

On Tue, 8 Oct 2024 15:57:22 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> On Thu, Oct 03, 2024 at 01:41:02AM +0200, Andrew Lunn wrote:
> > > +	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_PORT_PARAM];
> > > +	msg.sub[2] = id;
> > > +	/* Controller priority from 1 to 3 */
> > > +	msg.data[4] = prio + 1;  
> > 
> > Does 0 have a meaning? It just seems an odd design if it does not.  
> 
> 0 is not documented. But there are sub-priority which are not directly
> configured by user, but affect the system behavior.
> 
> Priority#: Critical – 1; high – 2; low – 3
>  For ports with the same priority, the PoE Controller sets the
>  sub-priority according to the logic port number. (Lower number gets
>  higher priority).
> 
> Port priority affects:
> 1. Power-up order: After a reset, the ports are powered up according to
>  their priority, highest to lowest, highest priority will power up first.
> 2. Shutdown order: When exceeding the power budget, lowest priority
>  ports will turn off first.
> 
> Should we return sub priorities on the prio get request?
> 
> If i see it correctly, even if user do not actively configures priorities,
> they are always present. For example port 0 will have always a Prio
> higher than Port 10.

We could add a subprio ehtool attribute, but it won't be configurable.
In fact it could be configurable by changing the port matrix order but it is not
a good idea. Applying a new port matrix turn off all the ports.

I am not sure if it is specific to Microchip controller or if it is generic
enough to add the attribute.
I would say not to return it for now.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 08/12] net: pse-pd: pd692x0: Add support for PSE PI priority feature

[Oleksij Rempel]

On Tue, Oct 08, 2024 at 04:21:20PM +0200, Kory Maincent wrote:
> On Tue, 8 Oct 2024 15:57:22 +0200
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > On Thu, Oct 03, 2024 at 01:41:02AM +0200, Andrew Lunn wrote:
> > > > +	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_PORT_PARAM];
> > > > +	msg.sub[2] = id;
> > > > +	/* Controller priority from 1 to 3 */
> > > > +	msg.data[4] = prio + 1;  
> > > 
> > > Does 0 have a meaning? It just seems an odd design if it does not.  
> > 
> > 0 is not documented. But there are sub-priority which are not directly
> > configured by user, but affect the system behavior.
> > 
> > Priority#: Critical – 1; high – 2; low – 3
> >  For ports with the same priority, the PoE Controller sets the
> >  sub-priority according to the logic port number. (Lower number gets
> >  higher priority).
> > 
> > Port priority affects:
> > 1. Power-up order: After a reset, the ports are powered up according to
> >  their priority, highest to lowest, highest priority will power up first.
> > 2. Shutdown order: When exceeding the power budget, lowest priority
> >  ports will turn off first.
> > 
> > Should we return sub priorities on the prio get request?
> > 
> > If i see it correctly, even if user do not actively configures priorities,
> > they are always present. For example port 0 will have always a Prio
> > higher than Port 10.
> 
> We could add a subprio ehtool attribute, but it won't be configurable.
> In fact it could be configurable by changing the port matrix order but it is not
> a good idea. Applying a new port matrix turn off all the ports.
> 
> I am not sure if it is specific to Microchip controller or if it is generic
> enough to add the attribute.
> I would say not to return it for now.

The generic attribute do not reflect the behavior of two different
controllers. Currently implemented prio attribute is in this case TI
specific and do not work for Microchip case.

Please note, I do not care about configurability in this case, I only
care about information the user get.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 08/12] net: pse-pd: pd692x0: Add support for PSE PI priority feature

[Andrew Lunn]

On Tue, Oct 08, 2024 at 03:57:22PM +0200, Oleksij Rempel wrote:
> On Thu, Oct 03, 2024 at 01:41:02AM +0200, Andrew Lunn wrote:
> > > +	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_PORT_PARAM];
> > > +	msg.sub[2] = id;
> > > +	/* Controller priority from 1 to 3 */
> > > +	msg.data[4] = prio + 1;
> > 
> > Does 0 have a meaning? It just seems an odd design if it does not.
> 
> 0 is not documented. But there are sub-priority which are not directly
> configured by user, but affect the system behavior.
> 
> Priority#: Critical – 1; high – 2; low – 3
>  For ports with the same priority, the PoE Controller sets the
>  sub-priority according to the logic port number. (Lower number gets
>  higher priority).

With less priorities than ports, there is always going to be something
like this.

> 
> Port priority affects:
> 1. Power-up order: After a reset, the ports are powered up according to
>  their priority, highest to lowest, highest priority will power up first.
> 2. Shutdown order: When exceeding the power budget, lowest priority
>  ports will turn off first.
> 
> Should we return sub priorities on the prio get request?

I should be optional, since we might not actually know what a
particular device is doing. It could pick at random, it could pick a
port which is consuming just enough to cover the shortfall if it was
turned off, it could pick the highest consumer of the lowest priority
etc. Some of these conditions are not going to be easy to describe
even if we do know it.

	Andrew

Re: [PATCH net-next 08/12] net: pse-pd: pd692x0: Add support for PSE PI priority feature

[Oleksij Rempel]

Hi Andrew,

On Tue, Oct 08, 2024 at 06:50:25PM +0200, Andrew Lunn wrote:
> On Tue, Oct 08, 2024 at 03:57:22PM +0200, Oleksij Rempel wrote:
> > On Thu, Oct 03, 2024 at 01:41:02AM +0200, Andrew Lunn wrote:
> > > > +	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_PORT_PARAM];
> > > > +	msg.sub[2] = id;
> > > > +	/* Controller priority from 1 to 3 */
> > > > +	msg.data[4] = prio + 1;
> > > 
> > > Does 0 have a meaning? It just seems an odd design if it does not.
> > 
> > 0 is not documented. But there are sub-priority which are not directly
> > configured by user, but affect the system behavior.
> > 
> > Priority#: Critical – 1; high – 2; low – 3
> >  For ports with the same priority, the PoE Controller sets the
> >  sub-priority according to the logic port number. (Lower number gets
> >  higher priority).
> 
> With less priorities than ports, there is always going to be something
> like this.
> 
> > 
> > Port priority affects:
> > 1. Power-up order: After a reset, the ports are powered up according to
> >  their priority, highest to lowest, highest priority will power up first.
> > 2. Shutdown order: When exceeding the power budget, lowest priority
> >  ports will turn off first.
> > 
> > Should we return sub priorities on the prio get request?
> 
> I should be optional, since we might not actually know what a
> particular device is doing. It could pick at random, it could pick a
> port which is consuming just enough to cover the shortfall if it was
> turned off, it could pick the highest consumer of the lowest priority
> etc. Some of these conditions are not going to be easy to describe
> even if we do know it.

After reviewing the manuals for LTC4266 and TPS2388x, I realized that these
controllers expose interfaces, but they don't implement prioritization concepts
themselves.

The LTC4266 and TPS2388x controllers provide only interfaces that allow the
kernel to manage shutdown and prioritization policies. For TPS2388x, fast
shutdown is implemented as a port bitmask with only two priorities, handled via
the OSS pin. Fast shutdown is triggered by the kernel on request by toggling
the corresponding pin, and the policy - when and why this pin is toggled - is
defined by the kernel or user space. Slow shutdown, on the other hand, is
managed via the I2C bus and allows for more refined control, enabling a wider
range of priorities and more granular policies.

I'll tend to hope we can reuse the proposed ETHTOOL_A_C33_PSE_PRIO interface
across different PSE controllers. However, it is already being mapped to
different shutdown concepts: PD692x0 firmware seems to rely on a slow shutdown
backed by internal policies, while TPS2388x maps it to fast shutdown with
driver specific policy. This inconsistency could force us to either break the
UAPI or introduce a new, inconsistent one once we realize TPS2388x fast
shutdown isn't what we actually need.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 08/12] net: pse-pd: pd692x0: Add support for PSE PI priority feature

[Andrew Lunn]

On Wed, Oct 09, 2024 at 09:16:33AM +0200, Oleksij Rempel wrote:
> Hi Andrew,
> 
> On Tue, Oct 08, 2024 at 06:50:25PM +0200, Andrew Lunn wrote:
> > On Tue, Oct 08, 2024 at 03:57:22PM +0200, Oleksij Rempel wrote:
> > > On Thu, Oct 03, 2024 at 01:41:02AM +0200, Andrew Lunn wrote:
> > > > > +	msg = pd692x0_msg_template_list[PD692X0_MSG_SET_PORT_PARAM];
> > > > > +	msg.sub[2] = id;
> > > > > +	/* Controller priority from 1 to 3 */
> > > > > +	msg.data[4] = prio + 1;
> > > > 
> > > > Does 0 have a meaning? It just seems an odd design if it does not.
> > > 
> > > 0 is not documented. But there are sub-priority which are not directly
> > > configured by user, but affect the system behavior.
> > > 
> > > Priority#: Critical – 1; high – 2; low – 3
> > >  For ports with the same priority, the PoE Controller sets the
> > >  sub-priority according to the logic port number. (Lower number gets
> > >  higher priority).
> > 
> > With less priorities than ports, there is always going to be something
> > like this.
> > 
> > > 
> > > Port priority affects:
> > > 1. Power-up order: After a reset, the ports are powered up according to
> > >  their priority, highest to lowest, highest priority will power up first.
> > > 2. Shutdown order: When exceeding the power budget, lowest priority
> > >  ports will turn off first.
> > > 
> > > Should we return sub priorities on the prio get request?
> > 
> > I should be optional, since we might not actually know what a
> > particular device is doing. It could pick at random, it could pick a
> > port which is consuming just enough to cover the shortfall if it was
> > turned off, it could pick the highest consumer of the lowest priority
> > etc. Some of these conditions are not going to be easy to describe
> > even if we do know it.
> 
> After reviewing the manuals for LTC4266 and TPS2388x, I realized that these
> controllers expose interfaces, but they don't implement prioritization concepts
> themselves.
> 
> The LTC4266 and TPS2388x controllers provide only interfaces that allow the
> kernel to manage shutdown and prioritization policies. For TPS2388x, fast
> shutdown is implemented as a port bitmask with only two priorities, handled via
> the OSS pin. Fast shutdown is triggered by the kernel on request by toggling
> the corresponding pin, and the policy - when and why this pin is toggled - is
> defined by the kernel or user space. Slow shutdown, on the other hand, is
> managed via the I2C bus and allows for more refined control, enabling a wider
> range of priorities and more granular policies.
> 
> I'll tend to hope we can reuse the proposed ETHTOOL_A_C33_PSE_PRIO interface
> across different PSE controllers. However, it is already being mapped to
> different shutdown concepts: PD692x0 firmware seems to rely on a slow shutdown
> backed by internal policies, while TPS2388x maps it to fast shutdown with
> driver specific policy. This inconsistency could force us to either break the
> UAPI or introduce a new, inconsistent one once we realize TPS2388x fast
> shutdown isn't what we actually need.

We should try to avoid fragmentation of the API, but given there is no
standardisation here, vendors are free to do whatever they want, this
may be difficult. Still, we should try to avoid it.

Maybe we want to consider a generic simple prioritization in the
kernel, plus export a generic model of PSE to user space to allow a
user space manager? This is really policy, and ideally we don't want
policy in the kernel. The in kernel implementation can use the
hardware prioritisation if it supports it, otherwise provide a library
of code which a driver can use to implement simple software
prioritisation?

For a user space manager, we already talked about an event to signal
that we are entering overload. I guess we additionally need an API to
get the current runtime state, what each consumer is actually
consuming, and a management API to shutdown or enable a port due to
overload, which is separate to the administrative state of a port.

The hardware/firmware might provide lots of options and capabilities,
but sometimes it is better it ignore them. Export a basic set which we
expect most PSE controllers can offer, and do the rest in our software
which we have full insight into, and debug and share, unlike firmware
which is a broken black box.

If we decide this is the way we want to go, we should not need to
extend the API for LTC4266 and TPS2388x, you just implement a basic
prioritisation in the driver. If it turns out to be not ideal, it does
not matter so much, so long as we have the understanding that
eventually we can have something better in userspace.

We can maybe do a rough sketch of what the kAPI looks like for a user
space manager, but we can kick the implementation down the road while
the in kernel prioritization is good enough.

	Andrew

[PATCH net-next 09/12] net: pse-pd: tps23881: Add support for PSE PI priority feature

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

This patch extends the PSE callbacks by adding support for the newly
introduced pi_set_prio() callback, enabling the configuration of PSE PI
priorities. The current port priority is now also included in the status
information returned to users.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/tps23881.c | 57 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index e05b45cdc9f8..ddb44a17218a 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -22,6 +22,7 @@
 #define TPS23881_OP_MODE_SEMIAUTO	0xaaaa
 #define TPS23881_REG_DIS_EN	0x13
 #define TPS23881_REG_DET_CLA_EN	0x14
+#define TPS23881_REG_PW_PRIO	0x15
 #define TPS23881_REG_GEN_MASK	0x17
 #define TPS23881_REG_NBITACC	BIT(5)
 #define TPS23881_REG_PW_EN	0x19
@@ -408,6 +409,24 @@ static int tps23881_ethtool_get_status(struct pse_controller_dev *pcdev,
 		return ret;
 	status->c33_pw_class = ret;
 
+	ret = i2c_smbus_read_word_data(client, TPS23881_REG_PW_PRIO);
+	if (ret < 0)
+		return ret;
+
+	chan = priv->port[id].chan[0];
+	if (chan < 4)
+		status->c33_prio = !!(ret & BIT(chan + 4));
+	else
+		status->c33_prio = !!(ret & BIT(chan + 8));
+
+	if (priv->port[id].is_4p) {
+		chan = priv->port[id].chan[1];
+		if (chan < 4)
+			status->c33_prio &= !!(ret & BIT(chan + 4));
+		else
+			status->c33_prio &= !!(ret & BIT(chan + 8));
+	}
+
 	return 0;
 }
 
@@ -925,6 +944,42 @@ static int tps23881_pi_set_current_limit(struct pse_controller_dev *pcdev,
 	return 0;
 }
 
+static int tps23881_pi_set_prio(struct pse_controller_dev *pcdev, int id,
+				unsigned int prio)
+{
+	struct tps23881_priv *priv = to_tps23881_priv(pcdev);
+	struct i2c_client *client = priv->client;
+	u8 chan, bit;
+	u16 val;
+	int ret;
+
+	ret = i2c_smbus_read_word_data(client, TPS23881_REG_PW_PRIO);
+	if (ret < 0)
+		return ret;
+
+	chan = priv->port[id].chan[0];
+	if (chan < 4)
+		bit = chan + 4;
+	else
+		bit = chan + 8;
+
+	val = (u16)(ret & ~BIT(bit));
+	val |= prio << (bit);
+
+	if (priv->port[id].is_4p) {
+		chan = priv->port[id].chan[1];
+		if (chan < 4)
+			bit = chan + 4;
+		else
+			bit = chan + 8;
+
+		val &= ~BIT(bit);
+		val |= prio << (bit);
+	}
+
+	return i2c_smbus_write_word_data(client, TPS23881_REG_PW_PRIO, val);
+}
+
 static const struct pse_controller_ops tps23881_ops = {
 	.setup_pi_matrix = tps23881_setup_pi_matrix,
 	.pi_enable = tps23881_pi_enable,
@@ -934,6 +989,7 @@ static const struct pse_controller_ops tps23881_ops = {
 	.pi_get_voltage = tps23881_pi_get_voltage,
 	.pi_get_current_limit = tps23881_pi_get_current_limit,
 	.pi_set_current_limit = tps23881_pi_set_current_limit,
+	.pi_set_prio = tps23881_pi_set_prio,
 };
 
 static const char fw_parity_name[] = "ti/tps23881/tps23881-parity-14.bin";
@@ -1106,6 +1162,7 @@ static int tps23881_i2c_probe(struct i2c_client *client)
 	priv->pcdev.dev = dev;
 	priv->pcdev.types = ETHTOOL_PSE_C33;
 	priv->pcdev.nr_lines = TPS23881_MAX_CHANS;
+	priv->pcdev.pis_prio_max = 1;
 	ret = devm_pse_controller_register(dev, &priv->pcdev);
 	if (ret) {
 		return dev_err_probe(dev, ret,

-- 
2.34.1

Re: [PATCH net-next 09/12] net: pse-pd: tps23881: Add support for PSE PI priority feature

[Andrew Lunn]

On Wed, Oct 02, 2024 at 06:28:05PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> This patch extends the PSE callbacks by adding support for the newly
> introduced pi_set_prio() callback, enabling the configuration of PSE PI
> priorities. The current port priority is now also included in the status
> information returned to users.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 09/12] net: pse-pd: tps23881: Add support for PSE PI priority feature

[Oleksij Rempel]

On Wed, Oct 02, 2024 at 06:28:05PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> This patch extends the PSE callbacks by adding support for the newly
> introduced pi_set_prio() callback, enabling the configuration of PSE PI
> priorities. The current port priority is now also included in the status
> information returned to users.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  drivers/net/pse-pd/tps23881.c | 57 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)

....
> 
>  static const char fw_parity_name[] = "ti/tps23881/tps23881-parity-14.bin";
> @@ -1106,6 +1162,7 @@ static int tps23881_i2c_probe(struct i2c_client *client)
>  	priv->pcdev.dev = dev;
>  	priv->pcdev.types = ETHTOOL_PSE_C33;
>  	priv->pcdev.nr_lines = TPS23881_MAX_CHANS;
> +	priv->pcdev.pis_prio_max = 1;

This controller supports 1 bit and 3 bit prios, it will be good to know
why 1 bit mode is used.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next 10/12] net: pse-pd: Register regulator even for undescribed PSE PIs

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

Ensure that regulators are registered for all PSE PIs, even those not
explicitly described in the device tree. This change lays the
groundwork for future support of regulator notifiers. Maintaining
consistent ordering between the PSE PIs regulator table and the
regulator notifier table will prevent added complexity in future
implementations.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/pse_core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 6b3893a3381c..d365fb7c8a98 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -463,10 +463,6 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 	for (i = 0; i < pcdev->nr_lines; i++) {
 		char *reg_name;
 
-		/* Do not register regulator for PIs not described */
-		if (!pcdev->no_of_pse_pi && !pcdev->pi[i].np)
-			continue;
-
 		reg_name = devm_kzalloc(pcdev->dev, reg_name_len, GFP_KERNEL);
 		if (!reg_name)
 			return -ENOMEM;

-- 
2.34.1

Re: [PATCH net-next 10/12] net: pse-pd: Register regulator even for undescribed PSE PIs

[Andrew Lunn]

On Wed, Oct 02, 2024 at 06:28:06PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Ensure that regulators are registered for all PSE PIs, even those not
> explicitly described in the device tree. This change lays the
> groundwork for future support of regulator notifiers. Maintaining
> consistent ordering between the PSE PIs regulator table and the
> regulator notifier table will prevent added complexity in future
> implementations.

Does this change anything visible to the user?

Is it guaranteed that these unused regulators are disabled?  Not that
they were before i guess. But now they exist, should we disable them?

	Andrew

Re: [PATCH net-next 10/12] net: pse-pd: Register regulator even for undescribed PSE PIs

[Kory Maincent]

Hello Andrew,

Thanks for your review!

On Thu, 3 Oct 2024 01:46:22 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Wed, Oct 02, 2024 at 06:28:06PM +0200, Kory Maincent wrote:
> > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> > 
> > Ensure that regulators are registered for all PSE PIs, even those not
> > explicitly described in the device tree. This change lays the
> > groundwork for future support of regulator notifiers. Maintaining
> > consistent ordering between the PSE PIs regulator table and the
> > regulator notifier table will prevent added complexity in future
> > implementations.  
> 
> Does this change anything visible to the user?

No it doesn't.
 
> Is it guaranteed that these unused regulators are disabled?  Not that
> they were before i guess. But now they exist, should we disable them?

Indeed we could disable PI not described in the devicetree.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH net-next 11/12] net: pse-pd: Add support for event reporting using devm_regulator_irq_helper

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

Add support for devm_pse_irq_helper(), a wrapper for
devm_regulator_irq_helper(). This aims to report events such as
over-current or over-temperature conditions similarly to how the regulator
API handles them. Additionally, this patch introduces several define
wrappers to keep regulator naming conventions out of PSE drivers.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/pse_core.c | 32 +++++++++++++++++++++++++++++++-
 include/linux/pse-pd/pse.h    | 24 ++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index d365fb7c8a98..a9f102507f5e 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -8,7 +8,6 @@
 #include <linux/device.h>
 #include <linux/of.h>
 #include <linux/pse-pd/pse.h>
-#include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 
 static DEFINE_MUTEX(pse_list_mutex);
@@ -536,6 +535,37 @@ int devm_pse_controller_register(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_pse_controller_register);
 
+int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq,
+			int irq_flags, int supported_errs,
+			const struct pse_irq_desc *d)
+{
+	struct regulator_dev **rdevs;
+	void *irq_helper;
+	int i;
+
+	rdevs = devm_kcalloc(pcdev->dev, pcdev->nr_lines,
+			     sizeof(struct regulator_dev *), GFP_KERNEL);
+	if (!rdevs)
+		return -ENOMEM;
+
+	for (i = 0; i < pcdev->nr_lines; i++)
+		rdevs[i] = pcdev->pi[i].rdev;
+
+	/* Register notifiers - can fail if IRQ is not given */
+	irq_helper = devm_regulator_irq_helper(pcdev->dev, d, irq,
+					       0, supported_errs, NULL,
+					       &rdevs[0], pcdev->nr_lines);
+	if (IS_ERR(irq_helper)) {
+		if (PTR_ERR(irq_helper) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		dev_warn(pcdev->dev, "IRQ disabled %pe\n", irq_helper);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_pse_irq_helper);
+
 /* PSE control section */
 
 static void __pse_control_release(struct kref *kref)
diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index b60fc56923bd..ba3d6630d768 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -8,6 +8,7 @@
 #include <linux/ethtool.h>
 #include <linux/list.h>
 #include <uapi/linux/ethtool.h>
+#include <linux/regulator/driver.h>
 
 /* Maximum current in uA according to IEEE 802.3-2022 Table 145-1 */
 #define MAX_PI_CURRENT 1920000
@@ -15,6 +16,26 @@
 struct phy_device;
 struct pse_controller_dev;
 
+/* structure and define wrappers from PSE to regulator */
+#define pse_irq_desc regulator_irq_desc
+#define pse_irq_data regulator_irq_data
+#define pse_err_data regulator_err_data
+#define pse_err_state regulator_err_state
+
+#define PSE_EVENT_TABLE(event)	REGULATOR_EVENT_##event
+#define PSE_ERROR_TABLE(error)	REGULATOR_ERROR_##error
+
+#define PSE_EVENT_OVER_CURRENT		PSE_EVENT_TABLE(OVER_CURRENT)
+#define PSE_EVENT_OVER_TEMP		PSE_EVENT_TABLE(OVER_TEMP)
+
+#define PSE_ERROR_OVER_CURRENT		PSE_ERROR_TABLE(OVER_CURRENT)
+#define PSE_ERROR_OVER_TEMP		PSE_ERROR_TABLE(OVER_TEMP)
+
+/* Return values for PSE IRQ helpers */
+#define PSE_ERROR_CLEARED	PSE_ERROR_TABLE(CLEARED)
+#define PSE_FAILED_RETRY	REGULATOR_FAILED_RETRY
+#define PSE_ERROR_ON		PSE_ERROR_TABLE(ON)
+
 /**
  * struct pse_control_config - PSE control/channel configuration.
  *
@@ -180,6 +201,9 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev);
 struct device;
 int devm_pse_controller_register(struct device *dev,
 				 struct pse_controller_dev *pcdev);
+int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq,
+			int irq_flags, int supported_errs,
+			const struct pse_irq_desc *d);
 
 struct pse_control *of_pse_control_get(struct device_node *node);
 void pse_control_put(struct pse_control *psec);

-- 
2.34.1

Re: [PATCH net-next 11/12] net: pse-pd: Add support for event reporting using devm_regulator_irq_helper

[Andrew Lunn]

> +int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq,
> +			int irq_flags, int supported_errs,
> +			const struct pse_irq_desc *d)
> +{
> +	struct regulator_dev **rdevs;
> +	void *irq_helper;
> +	int i;
> +
> +	rdevs = devm_kcalloc(pcdev->dev, pcdev->nr_lines,
> +			     sizeof(struct regulator_dev *), GFP_KERNEL);
> +	if (!rdevs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < pcdev->nr_lines; i++)
> +		rdevs[i] = pcdev->pi[i].rdev;
> +
> +	/* Register notifiers - can fail if IRQ is not given */
> +	irq_helper = devm_regulator_irq_helper(pcdev->dev, d, irq,
> +					       0, supported_errs, NULL,
> +					       &rdevs[0], pcdev->nr_lines);

Should irq_flags be passed through? I'm guessing one usage of it will
be IRQF_SHARED when there is one interrupt shared by a number of
controllers.

	Andrew

Re: [PATCH net-next 11/12] net: pse-pd: Add support for event reporting using devm_regulator_irq_helper

[Kory Maincent]

On Thu, 3 Oct 2024 01:52:20 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > +int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq,
> > +			int irq_flags, int supported_errs,
> > +			const struct pse_irq_desc *d)
> > +{
> > +	struct regulator_dev **rdevs;
> > +	void *irq_helper;
> > +	int i;
> > +
> > +	rdevs = devm_kcalloc(pcdev->dev, pcdev->nr_lines,
> > +			     sizeof(struct regulator_dev *), GFP_KERNEL);
> > +	if (!rdevs)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < pcdev->nr_lines; i++)
> > +		rdevs[i] = pcdev->pi[i].rdev;
> > +
> > +	/* Register notifiers - can fail if IRQ is not given */
> > +	irq_helper = devm_regulator_irq_helper(pcdev->dev, d, irq,
> > +					       0, supported_errs, NULL,
> > +					       &rdevs[0],
> > pcdev->nr_lines);  
> 
> Should irq_flags be passed through? I'm guessing one usage of it will
> be IRQF_SHARED when there is one interrupt shared by a number of
> controllers.

Oh yes, you are right! Thanks for spotting it!

> > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> > 
> > Add support for devm_pse_irq_helper(), a wrapper for
> > devm_regulator_irq_helper(). This aims to report events such as
> > over-current or over-temperature conditions similarly to how the regulator
> > API handles them. Additionally, this patch introduces several define
> > wrappers to keep regulator naming conventions out of PSE drivers.  
> 
> I'm missing something here, i think.
> 
> https://docs.kernel.org/power/regulator/consumer.html#regulator-events
> 
> Suggests these are internal events, using a notification chain. How
> does user space get to know about such events?

When events appears, _notifier_call_chain() is called which can generate netlink
messages alongside the internal events:
https://elixir.bootlin.com/linux/v6.11.1/source/drivers/regulator/core.c#L4898

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 11/12] net: pse-pd: Add support for event reporting using devm_regulator_irq_helper

[Andrew Lunn]

> > https://docs.kernel.org/power/regulator/consumer.html#regulator-events
> > 
> > Suggests these are internal events, using a notification chain. How
> > does user space get to know about such events?
> 
> When events appears, _notifier_call_chain() is called which can generate netlink
> messages alongside the internal events:
> https://elixir.bootlin.com/linux/v6.11.1/source/drivers/regulator/core.c#L4898

Ah, O.K.

But is this in the correct 'address space' for the want of a better
term. Everything else to do with PSE is in the networking domain of
netlink. ethtool is used to configure PSE. Shouldn't the notification
also close by to ethtool? When an interface changes state, there is a
notification sent. Maybe we want to piggyback on that?

Also, how do regulator events work in combination with network
namespaces? If you move the interface into a different network
namespace, do the regulator events get delivered to the root namespace
or the namespace the interface is in?

	Andrew

Re: [PATCH net-next 11/12] net: pse-pd: Add support for event reporting using devm_regulator_irq_helper

[Kory Maincent]

On Thu, 3 Oct 2024 14:56:21 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > https://docs.kernel.org/power/regulator/consumer.html#regulator-events
> > > 
> > > Suggests these are internal events, using a notification chain. How
> > > does user space get to know about such events?  
> > 
> > When events appears, _notifier_call_chain() is called which can generate
> > netlink messages alongside the internal events:
> > https://elixir.bootlin.com/linux/v6.11.1/source/drivers/regulator/core.c#L4898
> >  
> 
> Ah, O.K.
> 
> But is this in the correct 'address space' for the want of a better
> term. Everything else to do with PSE is in the networking domain of
> netlink. ethtool is used to configure PSE. Shouldn't the notification
> also close by to ethtool? When an interface changes state, there is a
> notification sent. Maybe we want to piggyback on that?

Indeed, but regulator API already provide such events, which will even be sent
when we enable or disable the PSE. Should we write a second event management.
Using regulator event API allows to report over current internal events to the
parents regulator the power supply of the PSE which could also do something to
avoid smoke.

Or maybe we should add another wrapper which will send PSE ethtool netlink
notification alongside the regulator notifications supported by this patch.

> Also, how do regulator events work in combination with network
> namespaces? If you move the interface into a different network
> namespace, do the regulator events get delivered to the root namespace
> or the namespace the interface is in?

regulator events are sent in root namespace.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 11/12] net: pse-pd: Add support for event reporting using devm_regulator_irq_helper

[Andrew Lunn]

> Indeed, but regulator API already provide such events, which will even be sent
> when we enable or disable the PSE. Should we write a second event management.
> Using regulator event API allows to report over current internal events to the
> parents regulator the power supply of the PSE which could also do something to
> avoid smoke.
> 
> Or maybe we should add another wrapper which will send PSE ethtool netlink
> notification alongside the regulator notifications supported by this patch.
> 
> > Also, how do regulator events work in combination with network
> > namespaces? If you move the interface into a different network
> > namespace, do the regulator events get delivered to the root namespace
> > or the namespace the interface is in?
> 
> regulator events are sent in root namespace.

I think we will need two event, the base regulator event, and a
networking event. Since it is a regulator, sending a normal regulator
event makes a lot of sense. But mapping that regulator event to a
netns:ifnam is going to be hard. Anything wanting to take an action is
probably going to want to use ethtool, and so needs to be in the
correct netns, etc. But it does get messy if there is some sort of
software driven prioritisation going on, some daemon needs to pick a
victim to reduce power to, and the interfaces are spread over multiple
network namespaces.

What i don't know is if we can use an existing event, or we should add
a new one. Often rtnetlink_event() is used:

https://elixir.bootlin.com/linux/v6.12-rc1/source/net/core/rtnetlink.c#L6679

but without some PSE information in it, it would be hard to know why
it was sent. So we probably either want a generic ethtool event, or a
PSE event.

    Andrew

Re: [PATCH net-next 11/12] net: pse-pd: Add support for event reporting using devm_regulator_irq_helper

[Oleksij Rempel]

On Thu, Oct 03, 2024 at 05:22:58PM +0200, Andrew Lunn wrote:
> > Indeed, but regulator API already provide such events, which will even be sent
> > when we enable or disable the PSE. Should we write a second event management.
> > Using regulator event API allows to report over current internal events to the
> > parents regulator the power supply of the PSE which could also do something to
> > avoid smoke.
> > 
> > Or maybe we should add another wrapper which will send PSE ethtool netlink
> > notification alongside the regulator notifications supported by this patch.
> > 
> > > Also, how do regulator events work in combination with network
> > > namespaces? If you move the interface into a different network
> > > namespace, do the regulator events get delivered to the root namespace
> > > or the namespace the interface is in?
> > 
> > regulator events are sent in root namespace.
> 
> I think we will need two event, the base regulator event, and a
> networking event. Since it is a regulator, sending a normal regulator
> event makes a lot of sense. But mapping that regulator event to a
> netns:ifnam is going to be hard. Anything wanting to take an action is
> probably going to want to use ethtool, and so needs to be in the
> correct netns, etc. But it does get messy if there is some sort of
> software driven prioritisation going on, some daemon needs to pick a
> victim to reduce power to, and the interfaces are spread over multiple
> network namespaces.
> 
> What i don't know is if we can use an existing event, or we should add
> a new one. Often rtnetlink_event() is used:
> 
> https://elixir.bootlin.com/linux/v6.12-rc1/source/net/core/rtnetlink.c#L6679
> 
> but without some PSE information in it, it would be hard to know why
> it was sent. So we probably either want a generic ethtool event, or a
> PSE event.

Hm... assuming we have following scenario:

                                  .---------   PI 1
                                 / .---------  PI 2
                   .========= PSE /----------( PI 3 ) NNS red
                  //              \----------( PI 4 ) NNS blue
Main supply      //                `---------( PI 5 ) NNS blue
o================´--- System, CPU

In this case we seems to have a new challenge:

On one side, a system wide power manager should see and mange all ports.
On other side, withing a name space, we should be able to play in a
isolated sand box. There is a reason why it is isolated. So, we should
be able to sandbox power delivery and port prios too. Means, by creating
network names space, we will need a power names space. 

I can even imagine a use case: an admin limited access to a switch for
developer. A developer name space is created with PSE budget and max
prios available for this name space. This will prevent users from DoSing
system critical ports.

At this point, creating a power name space will an overkill for this
patch set, so it should be enough to allow controlling prios over
ethtool per port and isolation support if needed.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 11/12] net: pse-pd: Add support for event reporting using devm_regulator_irq_helper

[Oleksij Rempel]

On Fri, Oct 04, 2024 at 03:56:33PM +0200, Oleksij Rempel wrote:
> On Thu, Oct 03, 2024 at 05:22:58PM +0200, Andrew Lunn wrote:
> > > Indeed, but regulator API already provide such events, which will even be sent
> > > when we enable or disable the PSE. Should we write a second event management.
> > > Using regulator event API allows to report over current internal events to the
> > > parents regulator the power supply of the PSE which could also do something to
> > > avoid smoke.
> > > 
> > > Or maybe we should add another wrapper which will send PSE ethtool netlink
> > > notification alongside the regulator notifications supported by this patch.
> > > 
> > > > Also, how do regulator events work in combination with network
> > > > namespaces? If you move the interface into a different network
> > > > namespace, do the regulator events get delivered to the root namespace
> > > > or the namespace the interface is in?
> > > 
> > > regulator events are sent in root namespace.
> > 
> > I think we will need two event, the base regulator event, and a
> > networking event. Since it is a regulator, sending a normal regulator
> > event makes a lot of sense. But mapping that regulator event to a
> > netns:ifnam is going to be hard. Anything wanting to take an action is
> > probably going to want to use ethtool, and so needs to be in the
> > correct netns, etc. But it does get messy if there is some sort of
> > software driven prioritisation going on, some daemon needs to pick a
> > victim to reduce power to, and the interfaces are spread over multiple
> > network namespaces.
> > 
> > What i don't know is if we can use an existing event, or we should add
> > a new one. Often rtnetlink_event() is used:
> > 
> > https://elixir.bootlin.com/linux/v6.12-rc1/source/net/core/rtnetlink.c#L6679
> > 
> > but without some PSE information in it, it would be hard to know why
> > it was sent. So we probably either want a generic ethtool event, or a
> > PSE event.
> 
> Hm... assuming we have following scenario:
> 
>                                   .---------   PI 1
>                                  / .---------  PI 2
>                    .========= PSE /----------( PI 3 ) NNS red
>                   //              \----------( PI 4 ) NNS blue
> Main supply      //                `---------( PI 5 ) NNS blue
> o================´--- System, CPU
> 
> In this case we seems to have a new challenge:
> 
> On one side, a system wide power manager should see and mange all ports.
> On other side, withing a name space, we should be able to play in a
> isolated sand box. There is a reason why it is isolated. So, we should
> be able to sandbox power delivery and port prios too. Means, by creating
> network names space, we will need a power names space. 
> 
> I can even imagine a use case: an admin limited access to a switch for
> developer. A developer name space is created with PSE budget and max
> prios available for this name space. This will prevent users from DoSing
> system critical ports.
> 
> At this point, creating a power name space will an overkill for this
> patch set, so it should be enough to allow controlling prios over
> ethtool per port and isolation support if needed.

Oh, sorry, i'm too tired. Too many words are missing in my answer ...

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 11/12] net: pse-pd: Add support for event reporting using devm_regulator_irq_helper

[Kory Maincent]

On Fri, 4 Oct 2024 16:02:15 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> On Fri, Oct 04, 2024 at 03:56:33PM +0200, Oleksij Rempel wrote:
> > On Thu, Oct 03, 2024 at 05:22:58PM +0200, Andrew Lunn wrote:  
>  [...]  
>  [...]  
>  [...]  
> > > 
> > > I think we will need two event, the base regulator event, and a
> > > networking event. Since it is a regulator, sending a normal regulator
> > > event makes a lot of sense. But mapping that regulator event to a
> > > netns:ifnam is going to be hard. Anything wanting to take an action is
> > > probably going to want to use ethtool, and so needs to be in the
> > > correct netns, etc. But it does get messy if there is some sort of
> > > software driven prioritisation going on, some daemon needs to pick a
> > > victim to reduce power to, and the interfaces are spread over multiple
> > > network namespaces.
> > > 
> > > What i don't know is if we can use an existing event, or we should add
> > > a new one. Often rtnetlink_event() is used:
> > > 
> > > https://elixir.bootlin.com/linux/v6.12-rc1/source/net/core/rtnetlink.c#L6679
> > > 
> > > but without some PSE information in it, it would be hard to know why
> > > it was sent. So we probably either want a generic ethtool event, or a
> > > PSE event.  
> > 
> > Hm... assuming we have following scenario:
> > 
> >                                   .---------   PI 1
> >                                  / .---------  PI 2
> >                    .========= PSE /----------( PI 3 ) NNS red
> >                   //              \----------( PI 4 ) NNS blue
> > Main supply      //                `---------( PI 5 ) NNS blue
> > o================´--- System, CPU
> > 
> > In this case we seems to have a new challenge:
> > 
> > On one side, a system wide power manager should see and mange all ports.
> > On other side, withing a name space, we should be able to play in a
> > isolated sand box. There is a reason why it is isolated. So, we should
> > be able to sandbox power delivery and port prios too. Means, by creating
> > network names space, we will need a power names space. 
> > 
> > I can even imagine a use case: an admin limited access to a switch for
> > developer. A developer name space is created with PSE budget and max
> > prios available for this name space. This will prevent users from DoSing
> > system critical ports.
> > 
> > At this point, creating a power name space will an overkill for this
> > patch set, so it should be enough to allow controlling prios over
> > ethtool per port and isolation support if needed. 

Yes, I will add simple ethtool notification for now to report events on each
interfaces.

> Oh, sorry, i'm too tired. Too many words are missing in my answer ...

Nearly the weekend!! Rest well!

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 11/12] net: pse-pd: Add support for event reporting using devm_regulator_irq_helper

[Andrew Lunn]

On Wed, Oct 02, 2024 at 06:28:07PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Add support for devm_pse_irq_helper(), a wrapper for
> devm_regulator_irq_helper(). This aims to report events such as
> over-current or over-temperature conditions similarly to how the regulator
> API handles them. Additionally, this patch introduces several define
> wrappers to keep regulator naming conventions out of PSE drivers.

I'm missing something here, i think.

https://docs.kernel.org/power/regulator/consumer.html#regulator-events

Suggests these are internal events, using a notification chain. How
does user space get to know about such events?

	Andrew

[PATCH net-next 12/12] net: pse-pd: tps23881: Add support for PSE events and interrupts

[Kory Maincent]

From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>

Add support for PSE event reporting through interrupts. Set up the newly
introduced devm_pse_irq_helper helper to register the interrupt. Events are
reported for over-current and over-temperature conditions.

This patch also adds support for an OSS GPIO line to turn off all low
priority ports in case of an over-current event.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/tps23881.c | 123 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 122 insertions(+), 1 deletion(-)

diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
index ddb44a17218a..03f36b641bb4 100644
--- a/drivers/net/pse-pd/tps23881.c
+++ b/drivers/net/pse-pd/tps23881.c
@@ -17,6 +17,13 @@
 
 #define TPS23881_MAX_CHANS 8
 
+#define TPS23881_REG_IT		0x0
+#define TPS23881_REG_IT_MASK	0x1
+#define TPS23881_REG_IT_IFAULT	BIT(5)
+#define TPS23881_REG_IT_SUPF	BIT(7)
+#define TPS23881_REG_FAULT	0x7
+#define TPS23881_REG_SUPF_EVENT	0xb
+#define TPS23881_REG_TSD	BIT(7)
 #define TPS23881_REG_PW_STATUS	0x10
 #define TPS23881_REG_OP_MODE	0x12
 #define TPS23881_OP_MODE_SEMIAUTO	0xaaaa
@@ -25,6 +32,7 @@
 #define TPS23881_REG_PW_PRIO	0x15
 #define TPS23881_REG_GEN_MASK	0x17
 #define TPS23881_REG_NBITACC	BIT(5)
+#define TPS23881_REG_INTEN	BIT(7)
 #define TPS23881_REG_PW_EN	0x19
 #define TPS23881_REG_2PAIR_POL1	0x1e
 #define TPS23881_REG_PORT_MAP	0x26
@@ -59,6 +67,7 @@ struct tps23881_priv {
 	struct pse_controller_dev pcdev;
 	struct device_node *np;
 	struct tps23881_port_desc port[TPS23881_MAX_CHANS];
+	struct gpio_desc *oss;
 };
 
 static struct tps23881_priv *to_tps23881_priv(struct pse_controller_dev *pcdev)
@@ -1088,11 +1097,112 @@ static int tps23881_flash_sram_fw(struct i2c_client *client)
 	return 0;
 }
 
+static void tps23881_turn_off_low_prio(struct tps23881_priv *priv)
+{
+	dev_info(&priv->client->dev,
+		 "turn off low priority ports due to over current event.\n");
+	gpiod_set_value_cansleep(priv->oss, 1);
+
+	/* TPS23880 datasheet (Rev G) indicates minimum OSS pulse is 5us */
+	usleep_range(5, 10);
+	gpiod_set_value_cansleep(priv->oss, 0);
+}
+
+static int tps23881_irq_handler(int irq, struct pse_irq_data *pid,
+				unsigned long *dev_mask)
+{
+	struct tps23881_priv *priv = (struct tps23881_priv *)pid->data;
+	struct i2c_client *client = priv->client;
+	struct pse_err_state *stat;
+	int ret, i;
+	u16 val;
+
+	*dev_mask = 0;
+	for (i = 0; i < TPS23881_MAX_CHANS; i++) {
+		stat = &pid->states[i];
+		stat->notifs = 0;
+		stat->errors = 0;
+	}
+
+	ret = i2c_smbus_read_word_data(client, TPS23881_REG_IT);
+	if (ret < 0)
+		return PSE_FAILED_RETRY;
+
+	val = (u16)ret;
+	if (val & TPS23881_REG_IT_SUPF) {
+		ret = i2c_smbus_read_word_data(client, TPS23881_REG_SUPF_EVENT);
+		if (ret < 0)
+			return PSE_FAILED_RETRY;
+
+		if (ret & TPS23881_REG_TSD) {
+			for (i = 0; i < TPS23881_MAX_CHANS; i++) {
+				stat = &pid->states[i];
+				*dev_mask |= 1 << i;
+				stat->notifs = PSE_EVENT_OVER_TEMP;
+				stat->errors = PSE_ERROR_OVER_TEMP;
+			}
+		}
+	}
+
+	if (val & (TPS23881_REG_IT_IFAULT | TPS23881_REG_IT_IFAULT << 8)) {
+		ret = i2c_smbus_read_word_data(client, TPS23881_REG_FAULT);
+		if (ret < 0)
+			return PSE_FAILED_RETRY;
+
+		val = (u16)(ret & 0xf0f);
+
+		/* Power cut detected, shutdown low priority port */
+		if (val && priv->oss)
+			tps23881_turn_off_low_prio(priv);
+
+		*dev_mask |= val;
+		for (i = 0; i < TPS23881_MAX_CHANS; i++) {
+			if (val & BIT(i)) {
+				stat = &pid->states[i];
+				stat->notifs = PSE_EVENT_OVER_CURRENT;
+				stat->errors = PSE_ERROR_OVER_CURRENT;
+			}
+		}
+	}
+
+	return PSE_ERROR_CLEARED;
+}
+
+static int tps23881_setup_irq(struct tps23881_priv *priv, int irq)
+{
+	int errs = PSE_ERROR_OVER_CURRENT | PSE_ERROR_OVER_TEMP;
+	struct i2c_client *client = priv->client;
+	struct pse_irq_desc irq_desc = {
+		.name = "tps23881-irq",
+		.map_event = tps23881_irq_handler,
+		.data = priv,
+	};
+	int ret;
+	u16 val;
+
+	val = TPS23881_REG_IT_IFAULT | TPS23881_REG_IT_SUPF |
+	      TPS23881_REG_IT_IFAULT << 8 | TPS23881_REG_IT_SUPF << 8;
+	ret = i2c_smbus_write_word_data(client, TPS23881_REG_IT_MASK, val);
+	if (ret)
+		return ret;
+
+	ret = i2c_smbus_read_word_data(client, TPS23881_REG_GEN_MASK);
+	if (ret < 0)
+		return ret;
+
+	val = (u16)(ret | TPS23881_REG_INTEN | TPS23881_REG_INTEN << 8);
+	ret = i2c_smbus_write_word_data(client, TPS23881_REG_GEN_MASK, val);
+	if (ret < 0)
+		return ret;
+
+	return devm_pse_irq_helper(&priv->pcdev, irq, 0, errs, &irq_desc);
+}
+
 static int tps23881_i2c_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct tps23881_priv *priv;
-	struct gpio_desc *reset;
+	struct gpio_desc *reset, *oss;
 	int ret;
 	u8 val;
 
@@ -1169,6 +1279,17 @@ static int tps23881_i2c_probe(struct i2c_client *client)
 				     "failed to register PSE controller\n");
 	}
 
+	oss = devm_gpiod_get_optional(dev, "oss", GPIOD_OUT_LOW);
+	if (IS_ERR(oss))
+		return dev_err_probe(&client->dev, PTR_ERR(oss), "Failed to get OSS GPIO\n");
+	priv->oss = oss;
+
+	if (client->irq) {
+		ret = tps23881_setup_irq(priv, client->irq);
+		if (ret)
+			return ret;
+	}
+
 	return ret;
 }
 

-- 
2.34.1

Re: [PATCH net-next 12/12] net: pse-pd: tps23881: Add support for PSE events and interrupts

[Andrew Lunn]

> This patch also adds support for an OSS GPIO line to turn off all low
> priority ports in case of an over-current event.

Does this need a binding update? Or is the GPIO already listed?

	Andrew

Re: [PATCH net-next 12/12] net: pse-pd: tps23881: Add support for PSE events and interrupts

[Kory Maincent]

On Thu, 3 Oct 2024 01:57:08 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > This patch also adds support for an OSS GPIO line to turn off all low
> > priority ports in case of an over-current event.  
> 
> Does this need a binding update? Or is the GPIO already listed?

Indeed and it is missing. Oops! /o\

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 12/12] net: pse-pd: tps23881: Add support for PSE events and interrupts

[Oleksij Rempel]

On Wed, Oct 02, 2024 at 06:28:08PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Add support for PSE event reporting through interrupts. Set up the newly
> introduced devm_pse_irq_helper helper to register the interrupt. Events are
> reported for over-current and over-temperature conditions.
> 
> This patch also adds support for an OSS GPIO line to turn off all low
> priority ports in case of an over-current event.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  drivers/net/pse-pd/tps23881.c | 123 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 122 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
> index ddb44a17218a..03f36b641bb4 100644
> --- a/drivers/net/pse-pd/tps23881.c
> +++ b/drivers/net/pse-pd/tps23881.c
> @@ -17,6 +17,13 @@
>  
>  #define TPS23881_MAX_CHANS 8
>  
> +#define TPS23881_REG_IT		0x0
> +#define TPS23881_REG_IT_MASK	0x1
> +#define TPS23881_REG_IT_IFAULT	BIT(5)
> +#define TPS23881_REG_IT_SUPF	BIT(7)
> +#define TPS23881_REG_FAULT	0x7
> +#define TPS23881_REG_SUPF_EVENT	0xb
> +#define TPS23881_REG_TSD	BIT(7)
>  #define TPS23881_REG_PW_STATUS	0x10
>  #define TPS23881_REG_OP_MODE	0x12
>  #define TPS23881_OP_MODE_SEMIAUTO	0xaaaa
> @@ -25,6 +32,7 @@
>  #define TPS23881_REG_PW_PRIO	0x15
>  #define TPS23881_REG_GEN_MASK	0x17
>  #define TPS23881_REG_NBITACC	BIT(5)
> +#define TPS23881_REG_INTEN	BIT(7)
>  #define TPS23881_REG_PW_EN	0x19
>  #define TPS23881_REG_2PAIR_POL1	0x1e
>  #define TPS23881_REG_PORT_MAP	0x26
> @@ -59,6 +67,7 @@ struct tps23881_priv {
>  	struct pse_controller_dev pcdev;
>  	struct device_node *np;
>  	struct tps23881_port_desc port[TPS23881_MAX_CHANS];
> +	struct gpio_desc *oss;
>  };
>  
>  static struct tps23881_priv *to_tps23881_priv(struct pse_controller_dev *pcdev)
> @@ -1088,11 +1097,112 @@ static int tps23881_flash_sram_fw(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static void tps23881_turn_off_low_prio(struct tps23881_priv *priv)
> +{
> +	dev_info(&priv->client->dev,
> +		 "turn off low priority ports due to over current event.\n");
> +	gpiod_set_value_cansleep(priv->oss, 1);
> +
> +	/* TPS23880 datasheet (Rev G) indicates minimum OSS pulse is 5us */
> +	usleep_range(5, 10);
> +	gpiod_set_value_cansleep(priv->oss, 0);

Ah, now I understand why 1 bit priority mode is used. The "fast" shutdown
path is done over interrupt and gpio bitbang.

It is not your fault...

> +}
> +
> +static int tps23881_irq_handler(int irq, struct pse_irq_data *pid,
> +				unsigned long *dev_mask)
> +{
> +	struct tps23881_priv *priv = (struct tps23881_priv *)pid->data;
> +	struct i2c_client *client = priv->client;
> +	struct pse_err_state *stat;
> +	int ret, i;
> +	u16 val;
> +
> +	*dev_mask = 0;
> +	for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +		stat = &pid->states[i];
> +		stat->notifs = 0;
> +		stat->errors = 0;
> +	}
> +

Please add comment that two registers are read here in one run.

> +	ret = i2c_smbus_read_word_data(client, TPS23881_REG_IT);
> +	if (ret < 0)
> +		return PSE_FAILED_RETRY;
> +
> +	val = (u16)ret;
> +	if (val & TPS23881_REG_IT_SUPF) {
> +		ret = i2c_smbus_read_word_data(client, TPS23881_REG_SUPF_EVENT);
> +		if (ret < 0)
> +			return PSE_FAILED_RETRY;
> +
> +		if (ret & TPS23881_REG_TSD) {
> +			for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +				stat = &pid->states[i];
> +				*dev_mask |= 1 << i;
> +				stat->notifs = PSE_EVENT_OVER_TEMP;
> +				stat->errors = PSE_ERROR_OVER_TEMP;
> +			}
> +		}
> +	}
> +
> +	if (val & (TPS23881_REG_IT_IFAULT | TPS23881_REG_IT_IFAULT << 8)) {

Ok, i see, you are reading two registers in one run and wont to test if
mask and status bits are active. In this code you will get true every
time the interrupt handler is executed.

> +		ret = i2c_smbus_read_word_data(client, TPS23881_REG_FAULT);
> +		if (ret < 0)
> +			return PSE_FAILED_RETRY;
> +
> +		val = (u16)(ret & 0xf0f);
> +
> +		/* Power cut detected, shutdown low priority port */
> +		if (val && priv->oss)
> +			tps23881_turn_off_low_prio(priv);
> +
> +		*dev_mask |= val;
> +		for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +			if (val & BIT(i)) {
> +				stat = &pid->states[i];
> +				stat->notifs = PSE_EVENT_OVER_CURRENT;
> +				stat->errors = PSE_ERROR_OVER_CURRENT;
> +			}
> +		}
> +	}
> +
> +	return PSE_ERROR_CLEARED;
> +}
> +
> +static int tps23881_setup_irq(struct tps23881_priv *priv, int irq)
> +{
> +	int errs = PSE_ERROR_OVER_CURRENT | PSE_ERROR_OVER_TEMP;
> +	struct i2c_client *client = priv->client;
> +	struct pse_irq_desc irq_desc = {
> +		.name = "tps23881-irq",
> +		.map_event = tps23881_irq_handler,
> +		.data = priv,
> +	};
> +	int ret;
> +	u16 val;
> +
> +	val = TPS23881_REG_IT_IFAULT | TPS23881_REG_IT_SUPF |
> +	      TPS23881_REG_IT_IFAULT << 8 | TPS23881_REG_IT_SUPF << 8;
> +	ret = i2c_smbus_write_word_data(client, TPS23881_REG_IT_MASK, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_smbus_read_word_data(client, TPS23881_REG_GEN_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (u16)(ret | TPS23881_REG_INTEN | TPS23881_REG_INTEN << 8);
> +	ret = i2c_smbus_write_word_data(client, TPS23881_REG_GEN_MASK, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_pse_irq_helper(&priv->pcdev, irq, 0, errs, &irq_desc);
> +}
> +
>  static int tps23881_i2c_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
>  	struct tps23881_priv *priv;
> -	struct gpio_desc *reset;
> +	struct gpio_desc *reset, *oss;
>  	int ret;
>  	u8 val;
>  
> @@ -1169,6 +1279,17 @@ static int tps23881_i2c_probe(struct i2c_client *client)
>  				     "failed to register PSE controller\n");
>  	}
>  
> +	oss = devm_gpiod_get_optional(dev, "oss", GPIOD_OUT_LOW);
> +	if (IS_ERR(oss))
> +		return dev_err_probe(&client->dev, PTR_ERR(oss), "Failed to get OSS GPIO\n");
> +	priv->oss = oss;
> +
> +	if (client->irq) {
> +		ret = tps23881_setup_irq(priv, client->irq);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return ret;
>  }
>  
> 
> -- 
> 2.34.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 12/12] net: pse-pd: tps23881: Add support for PSE events and interrupts

[Oleksij Rempel]

Hi Kory,

On Wed, Oct 02, 2024 at 06:28:08PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> Add support for PSE event reporting through interrupts. Set up the newly
> introduced devm_pse_irq_helper helper to register the interrupt. Events are
> reported for over-current and over-temperature conditions.
> 
> This patch also adds support for an OSS GPIO line to turn off all low
> priority ports in case of an over-current event.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>

...
> +static int tps23881_irq_handler(int irq, struct pse_irq_data *pid,
> +				unsigned long *dev_mask)
> +{
> +	struct tps23881_priv *priv = (struct tps23881_priv *)pid->data;
> +	struct i2c_client *client = priv->client;
> +	struct pse_err_state *stat;
> +	int ret, i;
> +	u16 val;
> +
> +	*dev_mask = 0;
> +	for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +		stat = &pid->states[i];
> +		stat->notifs = 0;
> +		stat->errors = 0;
> +	}
> +
> +	ret = i2c_smbus_read_word_data(client, TPS23881_REG_IT);
> +	if (ret < 0)
> +		return PSE_FAILED_RETRY;
> +
> +	val = (u16)ret;
> +	if (val & TPS23881_REG_IT_SUPF) {
> +		ret = i2c_smbus_read_word_data(client, TPS23881_REG_SUPF_EVENT);
> +		if (ret < 0)
> +			return PSE_FAILED_RETRY;
> +
> +		if (ret & TPS23881_REG_TSD) {
> +			for (i = 0; i < TPS23881_MAX_CHANS; i++) {
> +				stat = &pid->states[i];
> +				*dev_mask |= 1 << i;
> +				stat->notifs = PSE_EVENT_OVER_TEMP;
> +				stat->errors = PSE_ERROR_OVER_TEMP;
> +			}
> +		}
> +	}
> +
> +	if (val & (TPS23881_REG_IT_IFAULT | TPS23881_REG_IT_IFAULT << 8)) {
> +		ret = i2c_smbus_read_word_data(client, TPS23881_REG_FAULT);
> +		if (ret < 0)
> +			return PSE_FAILED_RETRY;
> +
> +		val = (u16)(ret & 0xf0f);
> +
> +		/* Power cut detected, shutdown low priority port */
> +		if (val && priv->oss)
> +			tps23881_turn_off_low_prio(priv);

Sorry, this is policy and even not the best one.
The priority concept is related to the power budget, but this
implementation will shutdown all low prios ports only if some
port/channel has over-current event. It means, in case high prio port
has over-current event, it will be not shut down.
 
I'll propose not to add prio support for this chip right now, it will
need more software infrastructure to handle it nearly in similar way as
it is done by pd692x0.
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 12/12] net: pse-pd: tps23881: Add support for PSE events and interrupts

[Kory Maincent]

On Wed, 9 Oct 2024 09:25:10 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> > +
> > +	if (val & (TPS23881_REG_IT_IFAULT | TPS23881_REG_IT_IFAULT << 8)) {
> > +		ret = i2c_smbus_read_word_data(client, TPS23881_REG_FAULT);
> > +		if (ret < 0)
> > +			return PSE_FAILED_RETRY;
> > +
> > +		val = (u16)(ret & 0xf0f);
> > +
> > +		/* Power cut detected, shutdown low priority port */
> > +		if (val && priv->oss)
> > +			tps23881_turn_off_low_prio(priv);  
> 
> Sorry, this is policy and even not the best one.
> The priority concept is related to the power budget, but this
> implementation will shutdown all low prios ports only if some
> port/channel has over-current event. It means, in case high prio port
> has over-current event, it will be not shut down.
>  
> I'll propose not to add prio support for this chip right now, it will
> need more software infrastructure to handle it nearly in similar way as
> it is done by pd692x0.

Yes, I have expected some debate around this support.
I was not sure of the policy while developing it.
Ok, let's remove it for now.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 00/12] Add support for PSE port priority

[Kyle Swenson]

Hello Kory,

On Wed, Oct 02, 2024 at 06:27:56PM +0200, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> This series brings support for port priority in the PSE subsystem.
> PSE controllers can set priorities to decide which ports should be
> turned off in case of special events like over-current.

First off, great work here.  I've read through the patches in the series and
have a pretty good idea of what you're trying to achieve- use the PSE
controller's idea of "port priority" and expose this to userspace via ethtool.

I think this is probably sufficient but I wanted to share my experience
supporting a system level PSE power budget with PSE port priorities across
different PSE controllers through the same userspace interface such that
userspace doesn't know or care about the underlying PSE controller.

Out of the three PSE controllers I'm aware of (Microchip's PD692x0, TI's
TPS2388x, and LTC's LT4266), the PD692x0 definitely has the most advanced
configuration, supporting concepts like a system (well, manager) level budget
and powering off lower priority ports in the event that the port power
consumption is greater than the system budget.

When we experimented with this feature in our routers, we found it to be using
the dynamic power consumed by a particular port- literally, the summation of
port current * port voltage across all the ports.  While this behavior
technically saves the system from resetting or worse, it causes a bit of a
problem with lower priority ports getting powered off depending on the behavior
(power consumption) of unrelated devices.  

As an example, let's say we've got 4 devices, all powered, and we're close to
the power budget.  One of the devices starts consuming more power (perhaps it's
modem just powered on), but not more than it's class limit.  Say this device
consumes enough power to exceed the configured power budget, causing the lowest
priority device to be powered off.  This is the documented and intended
behavior of the PD692x0 chipset, but causes an unpleasant user experience
because it's not really clear why some device was powered down all the sudden.
Was it because someone unplugged it? Or because the modem on the high priority
device turned on?  Or maybe that device had an overcurrent?  It'd be impossible
to tell, and even worse, by the time someone is able to physically look at the
switch, the low priority device might be back online (perhaps the modem on
the high priority device powered off).

This behavior is unique to the PD692x0- I'm much less familiar with the
TPS2388x's idea of port priority but it is very different from the PD692x0.
Frankly the behavior of the OSS pin is confusing and since we don't use the PSE
controllers' idea of port priority, it was safe to ignore it. Finally, the
LTC4266 has a "masked shutdown" ability where a predetermined set of ports are
shutdown when a specific pin (MSD) is driven low.  Like the TPS2388x's OSS pin,
We ignore this feature on the LTC4266.

If the end-goal here is to have a device-independent idea of "port priority" I
think we need to add a level of indirection between the port priority concept and the
actual PSE hardware.  The indirection would enable a system with multiple
(possibly heterogeneous even) PSE chips to have a unified idea of port
priority.  The way we've implemented this in our routers is by putting the PSE
controllers in "semi-auto" mode, where they continually detect and classify PDs
(powered device), but do not power them until instructed to do so.  The
mechanism that decides to power a particular port or not (for lack of a better
term, "budgeting logic") uses the available system power budget (configured
from userspace), the relative port priorities (also configured from userspace)
and the class of a detected PD.  The classification result is used to determine
the _maximum_ power a particular PD might draw, and that is the value that is
subtracted from the power budget.

Using the PD's classification and then allocating it the maximum power for that
class enables a non-technical installer to plug in all the PDs at the switch,
and observe if all the PDs are powered (or not).  But the important part is
(unless the port priorities or power budget are changed from userspace) the
devices that are powered won't change due to dynamic power consumption of the
other devices.

I'm not sure what the right path is for the kernel, and I'm not sure how this
would look with the regulator integration, nor am I sure what the userspace API
should look like (we used sysfs, but that's probably not ideal for upstream).
It's also not clear how much of the budgeting logic should be in the kernel, if
any. Despite that, hopefully sharing our experience is insightful and/or
helpful.  If not, feel free to ignore it.  In any case, you've got my

Reviewed-by: Kyle Swenson <kyle.swenson@est.tech>

for all the patches in the series.

Thanks,
Kyle Swenson

Re: [PATCH net-next 00/12] Add support for PSE port priority

[Kory Maincent]

Hello Kyle,

On Wed, 9 Oct 2024 13:54:51 +0000
Kyle Swenson <kyle.swenson@est.tech> wrote:

> Hello Kory,
> 
> On Wed, Oct 02, 2024 at 06:27:56PM +0200, Kory Maincent wrote:
> > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> > 
> > This series brings support for port priority in the PSE subsystem.
> > PSE controllers can set priorities to decide which ports should be
> > turned off in case of special events like over-current.  
> 
> First off, great work here.  I've read through the patches in the series and
> have a pretty good idea of what you're trying to achieve- use the PSE
> controller's idea of "port priority" and expose this to userspace via ethtool.
> 
> I think this is probably sufficient but I wanted to share my experience
> supporting a system level PSE power budget with PSE port priorities across
> different PSE controllers through the same userspace interface such that
> userspace doesn't know or care about the underlying PSE controller.
> 
> Out of the three PSE controllers I'm aware of (Microchip's PD692x0, TI's
> TPS2388x, and LTC's LT4266), the PD692x0 definitely has the most advanced
> configuration, supporting concepts like a system (well, manager) level budget
> and powering off lower priority ports in the event that the port power
> consumption is greater than the system budget.
> 
> When we experimented with this feature in our routers, we found it to be using
> the dynamic power consumed by a particular port- literally, the summation of
> port current * port voltage across all the ports.  While this behavior
> technically saves the system from resetting or worse, it causes a bit of a
> problem with lower priority ports getting powered off depending on the
> behavior (power consumption) of unrelated devices.  
> 
> As an example, let's say we've got 4 devices, all powered, and we're close to
> the power budget.  One of the devices starts consuming more power (perhaps
> it's modem just powered on), but not more than it's class limit.  Say this
> device consumes enough power to exceed the configured power budget, causing
> the lowest priority device to be powered off.  This is the documented and
> intended behavior of the PD692x0 chipset, but causes an unpleasant user
> experience because it's not really clear why some device was powered down all
> the sudden. Was it because someone unplugged it? Or because the modem on the
> high priority device turned on?  Or maybe that device had an overcurrent?
> It'd be impossible to tell, and even worse, by the time someone is able to
> physically look at the switch, the low priority device might be back online
> (perhaps the modem on the high priority device powered off).
> 
> This behavior is unique to the PD692x0- I'm much less familiar with the
> TPS2388x's idea of port priority but it is very different from the PD692x0.
> Frankly the behavior of the OSS pin is confusing and since we don't use the
> PSE controllers' idea of port priority, it was safe to ignore it. Finally, the
> LTC4266 has a "masked shutdown" ability where a predetermined set of ports are
> shutdown when a specific pin (MSD) is driven low.  Like the TPS2388x's OSS
> pin, We ignore this feature on the LTC4266.
> 
> If the end-goal here is to have a device-independent idea of "port priority" I
> think we need to add a level of indirection between the port priority concept
> and the actual PSE hardware.  The indirection would enable a system with
> multiple (possibly heterogeneous even) PSE chips to have a unified idea of
> port priority.  The way we've implemented this in our routers is by putting
> the PSE controllers in "semi-auto" mode, where they continually detect and
> classify PDs (powered device), but do not power them until instructed to do
> so.  The mechanism that decides to power a particular port or not (for lack
> of a better term, "budgeting logic") uses the available system power budget
> (configured from userspace), the relative port priorities (also configured
> from userspace) and the class of a detected PD.  The classification result is
> used to determine the _maximum_ power a particular PD might draw, and that is
> the value that is subtracted from the power budget.
> 
> Using the PD's classification and then allocating it the maximum power for
> that class enables a non-technical installer to plug in all the PDs at the
> switch, and observe if all the PDs are powered (or not).  But the important
> part is (unless the port priorities or power budget are changed from
> userspace) the devices that are powered won't change due to dynamic power
> consumption of the other devices.
> 
> I'm not sure what the right path is for the kernel, and I'm not sure how this
> would look with the regulator integration, nor am I sure what the userspace
> API should look like (we used sysfs, but that's probably not ideal for
> upstream). It's also not clear how much of the budgeting logic should be in
> the kernel, if any. Despite that, hopefully sharing our experience is
> insightful and/or helpful.  If not, feel free to ignore it.  In any case,
> you've got my

Thanks for your review and for sharing your PSE experience.
It indeed is insightful for further development and update of this series.

So you are saying that from a use experience the port priority feature is not
user-friendly as we don't know why a port has been shutdown.
Even if we can report the over-current event of which port caused it, you still
thinks it is not useful?

We could have several cases for over power budget event:
- The power limit exceeded is the one configured for the ports.
  We should shutdown only that port without taking care about priority.
  TPS23881 has this behavior when power exceed Pcut.
  I think the PD692x0 does the same. Need to verify.
- The power limit exceeded is the global (or manager PD69208M) power budget.
  Here port priority is interesting.
  Is there a way to know which port create this global power limit excess?
  Should we turn off this port even if he don't exceed his own power limit or
  should we turn off low priority ports?
  I can't find global power budget concept for the TPS23881. 
  I could't test this case because I don't have enough load. In fact, maybe by
  setting the PD692x0 power bank limit low it could work.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 00/12] Add support for PSE port priority

[Kyle Swenson]

Hello Kory,

On Wed, Oct 09, 2024 at 05:04:00PM +0200, Kory Maincent wrote:
> Hello Kyle,
> 
> On Wed, 9 Oct 2024 13:54:51 +0000
> Kyle Swenson <kyle.swenson@est.tech> wrote:
> 
> > Hello Kory,
> > 
> > On Wed, Oct 02, 2024 at 06:27:56PM +0200, Kory Maincent wrote:
> > > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> > > 
> > > This series brings support for port priority in the PSE subsystem.
> > > PSE controllers can set priorities to decide which ports should be
> > > turned off in case of special events like over-current.  
> > 
> > First off, great work here.  I've read through the patches in the series and
> > have a pretty good idea of what you're trying to achieve- use the PSE
> > controller's idea of "port priority" and expose this to userspace via ethtool.
> > 
> > I think this is probably sufficient but I wanted to share my experience
> > supporting a system level PSE power budget with PSE port priorities across
> > different PSE controllers through the same userspace interface such that
> > userspace doesn't know or care about the underlying PSE controller.
> > 
> > Out of the three PSE controllers I'm aware of (Microchip's PD692x0, TI's
> > TPS2388x, and LTC's LT4266), the PD692x0 definitely has the most advanced
> > configuration, supporting concepts like a system (well, manager) level budget
> > and powering off lower priority ports in the event that the port power
> > consumption is greater than the system budget.
> > 
> > When we experimented with this feature in our routers, we found it to be using
> > the dynamic power consumed by a particular port- literally, the summation of
> > port current * port voltage across all the ports.  While this behavior
> > technically saves the system from resetting or worse, it causes a bit of a
> > problem with lower priority ports getting powered off depending on the
> > behavior (power consumption) of unrelated devices.  
> > 
> > As an example, let's say we've got 4 devices, all powered, and we're close to
> > the power budget.  One of the devices starts consuming more power (perhaps
> > it's modem just powered on), but not more than it's class limit.  Say this
> > device consumes enough power to exceed the configured power budget, causing
> > the lowest priority device to be powered off.  This is the documented and
> > intended behavior of the PD692x0 chipset, but causes an unpleasant user
> > experience because it's not really clear why some device was powered down all
> > the sudden. Was it because someone unplugged it? Or because the modem on the
> > high priority device turned on?  Or maybe that device had an overcurrent?
> > It'd be impossible to tell, and even worse, by the time someone is able to
> > physically look at the switch, the low priority device might be back online
> > (perhaps the modem on the high priority device powered off).
> > 
> > This behavior is unique to the PD692x0- I'm much less familiar with the
> > TPS2388x's idea of port priority but it is very different from the PD692x0.
> > Frankly the behavior of the OSS pin is confusing and since we don't use the
> > PSE controllers' idea of port priority, it was safe to ignore it. Finally, the
> > LTC4266 has a "masked shutdown" ability where a predetermined set of ports are
> > shutdown when a specific pin (MSD) is driven low.  Like the TPS2388x's OSS
> > pin, We ignore this feature on the LTC4266.
> > 
> > If the end-goal here is to have a device-independent idea of "port priority" I
> > think we need to add a level of indirection between the port priority concept
> > and the actual PSE hardware.  The indirection would enable a system with
> > multiple (possibly heterogeneous even) PSE chips to have a unified idea of
> > port priority.  The way we've implemented this in our routers is by putting
> > the PSE controllers in "semi-auto" mode, where they continually detect and
> > classify PDs (powered device), but do not power them until instructed to do
> > so.  The mechanism that decides to power a particular port or not (for lack
> > of a better term, "budgeting logic") uses the available system power budget
> > (configured from userspace), the relative port priorities (also configured
> > from userspace) and the class of a detected PD.  The classification result is
> > used to determine the _maximum_ power a particular PD might draw, and that is
> > the value that is subtracted from the power budget.
> > 
> > Using the PD's classification and then allocating it the maximum power for
> > that class enables a non-technical installer to plug in all the PDs at the
> > switch, and observe if all the PDs are powered (or not).  But the important
> > part is (unless the port priorities or power budget are changed from
> > userspace) the devices that are powered won't change due to dynamic power
> > consumption of the other devices.
> > 
> > I'm not sure what the right path is for the kernel, and I'm not sure how this
> > would look with the regulator integration, nor am I sure what the userspace
> > API should look like (we used sysfs, but that's probably not ideal for
> > upstream). It's also not clear how much of the budgeting logic should be in
> > the kernel, if any. Despite that, hopefully sharing our experience is
> > insightful and/or helpful.  If not, feel free to ignore it.  In any case,
> > you've got my
> 
> Thanks for your review and for sharing your PSE experience.
> It indeed is insightful for further development and update of this series.

Excellent, glad to hear it.

> So you are saying that from a use experience the port priority feature is not
> user-friendly as we don't know why a port has been shutdown.
> Even if we can report the over-current event of which port caused it, you still
> thinks it is not useful?

Well, not quite.  I think the concept of a "port priority" is useful,
but I don't know that the PD692xx's concept of "port priority" is what
we want.  The issue is the PD692xx's budgeting algorithm is based on
dynamic power used (i.e. the total power used at any given time).  Since
this is, well, dynamic, it makes it confusing when a lower priority port
is powered off due to the runtime behavior of higher-priority ports.
It's even more confusing if the implicit or default port priorities are
used.

Instead, we found that using the maximum power that is allowed be drawn
by a particular PD's class (set by the IEEE standard) is more user
friendly, because the set of devices that are powered won't change
(unless priorities are changed, or the system budget is changed).
For example, if we've got 4 devices plugged in, and the three highest
priority devices consume all the power budget, the lowest priority
device won't ever be powered.  There isn't a case where the lowest
priority device will be shut down because a higher priority device
starts consuming more power at some point in the future.

> We could have several cases for over power budget event:
> - The power limit exceeded is the one configured for the ports.
>   We should shutdown only that port without taking care about priority.
>   TPS23881 has this behavior when power exceed Pcut.
>   I think the PD692x0 does the same. Need to verify.

These conditions I'd not call "over power budget events".  I'd call them
"port overcurrent events" and I agree, those only affect the specific
problem port.

> - The power limit exceeded is the global (or manager PD69208M) power budget.
>   Here port priority is interesting.
>   Is there a way to know which port create this global power limit excess?
>   Should we turn off this port even if he don't exceed his own power limit or
>   should we turn off low priority ports?

I think it's important to make a distinction between an "overcurrent"
condition and the condition where we've exceeded the system power
budget.  An "overcurrent" is port-specific, and can happen if the PD
consumes more power than the classification of the device allows.  For
example, if a Class 3 PD (i.e. 802.3at, also referred to as a Type II
PD) consumes more than 15.4 W at the PSE, it will be shutdown
immediately.  This support is required by all the IEEE 802.3 standards
around PoE (.af, .at. and .bt) and is a safety thing.  The TPS2388x
implements this with Pcut, the LTC4266 impliments this with Icut
register, and the PD692xx implements it with the port power limit
registers.  

The condition where we've exceeded our system-level power
budget is a little different, in that it causes a port to be shutdown
despite that port not exceeding it's class power limit.  This condition
is the case I'm concerned we're solving in this series, and solving it
for the PD692xx case only, and it's based off dynamic power consumption.

So I guess I'm suggesting that we take the power budgeting concept out
of the PSE drivers, and put it into software (either kernel, userspace)
instead of the PSE hardware.  

>   I can't find global power budget concept for the TPS23881. 

This is because this idea doesn't exist on the TPS2388x.  

>   I could't test this case because I don't have enough load. In fact, maybe by
>   setting the PD692x0 power bank limit low it could work.

Hopefully this helps clarify.

> 
> Regards,
> -- 
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com

Thanks,
Kyle

Re: [PATCH net-next 00/12] Add support for PSE port priority

[Oleksij Rempel]

Hello Kyle,

On Wed, Oct 09, 2024 at 05:42:30PM +0000, Kyle Swenson wrote:
> Hello Kory,
> 
> On Wed, Oct 09, 2024 at 05:04:00PM +0200, Kory Maincent wrote:
> > Hello Kyle,
> > 
> > On Wed, 9 Oct 2024 13:54:51 +0000
> > Kyle Swenson <kyle.swenson@est.tech> wrote:
> > 
> > > Hello Kory,
> > > 
> > > On Wed, Oct 02, 2024 at 06:27:56PM +0200, Kory Maincent wrote:
> > > > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> > > > 
> > > > This series brings support for port priority in the PSE subsystem.
> > > > PSE controllers can set priorities to decide which ports should be
> > > > turned off in case of special events like over-current.  
> > > 
> > > First off, great work here.  I've read through the patches in the series and
> > > have a pretty good idea of what you're trying to achieve- use the PSE
> > > controller's idea of "port priority" and expose this to userspace via ethtool.
> > > 
> > > I think this is probably sufficient but I wanted to share my experience
> > > supporting a system level PSE power budget with PSE port priorities across
> > > different PSE controllers through the same userspace interface such that
> > > userspace doesn't know or care about the underlying PSE controller.
> > > 
> > > Out of the three PSE controllers I'm aware of (Microchip's PD692x0, TI's
> > > TPS2388x, and LTC's LT4266), the PD692x0 definitely has the most advanced
> > > configuration, supporting concepts like a system (well, manager) level budget
> > > and powering off lower priority ports in the event that the port power
> > > consumption is greater than the system budget.
> > > 
> > > When we experimented with this feature in our routers, we found it to be using
> > > the dynamic power consumed by a particular port- literally, the summation of
> > > port current * port voltage across all the ports.  While this behavior
> > > technically saves the system from resetting or worse, it causes a bit of a
> > > problem with lower priority ports getting powered off depending on the
> > > behavior (power consumption) of unrelated devices.  
> > > 
> > > As an example, let's say we've got 4 devices, all powered, and we're close to
> > > the power budget.  One of the devices starts consuming more power (perhaps
> > > it's modem just powered on), but not more than it's class limit.  Say this
> > > device consumes enough power to exceed the configured power budget, causing
> > > the lowest priority device to be powered off.  This is the documented and
> > > intended behavior of the PD692x0 chipset, but causes an unpleasant user
> > > experience because it's not really clear why some device was powered down all
> > > the sudden. Was it because someone unplugged it? Or because the modem on the
> > > high priority device turned on?  Or maybe that device had an overcurrent?
> > > It'd be impossible to tell, and even worse, by the time someone is able to
> > > physically look at the switch, the low priority device might be back online
> > > (perhaps the modem on the high priority device powered off).
> > > 
> > > This behavior is unique to the PD692x0- I'm much less familiar with the
> > > TPS2388x's idea of port priority but it is very different from the PD692x0.
> > > Frankly the behavior of the OSS pin is confusing and since we don't use the
> > > PSE controllers' idea of port priority, it was safe to ignore it. Finally, the
> > > LTC4266 has a "masked shutdown" ability where a predetermined set of ports are
> > > shutdown when a specific pin (MSD) is driven low.  Like the TPS2388x's OSS
> > > pin, We ignore this feature on the LTC4266.
> > > 
> > > If the end-goal here is to have a device-independent idea of "port priority" I
> > > think we need to add a level of indirection between the port priority concept
> > > and the actual PSE hardware.  The indirection would enable a system with
> > > multiple (possibly heterogeneous even) PSE chips to have a unified idea of
> > > port priority.  The way we've implemented this in our routers is by putting
> > > the PSE controllers in "semi-auto" mode, where they continually detect and
> > > classify PDs (powered device), but do not power them until instructed to do
> > > so.  The mechanism that decides to power a particular port or not (for lack
> > > of a better term, "budgeting logic") uses the available system power budget
> > > (configured from userspace), the relative port priorities (also configured
> > > from userspace) and the class of a detected PD.  The classification result is
> > > used to determine the _maximum_ power a particular PD might draw, and that is
> > > the value that is subtracted from the power budget.
> > > 
> > > Using the PD's classification and then allocating it the maximum power for
> > > that class enables a non-technical installer to plug in all the PDs at the
> > > switch, and observe if all the PDs are powered (or not).  But the important
> > > part is (unless the port priorities or power budget are changed from
> > > userspace) the devices that are powered won't change due to dynamic power
> > > consumption of the other devices.
> > > 
> > > I'm not sure what the right path is for the kernel, and I'm not sure how this
> > > would look with the regulator integration, nor am I sure what the userspace
> > > API should look like (we used sysfs, but that's probably not ideal for
> > > upstream). It's also not clear how much of the budgeting logic should be in
> > > the kernel, if any. Despite that, hopefully sharing our experience is
> > > insightful and/or helpful.  If not, feel free to ignore it.  In any case,
> > > you've got my
> > 
> > Thanks for your review and for sharing your PSE experience.
> > It indeed is insightful for further development and update of this series.
> 
> Excellent, glad to hear it.
> 
> > So you are saying that from a use experience the port priority feature is not
> > user-friendly as we don't know why a port has been shutdown.
> > Even if we can report the over-current event of which port caused it, you still
> > thinks it is not useful?
> 
> Well, not quite.  I think the concept of a "port priority" is useful,
> but I don't know that the PD692xx's concept of "port priority" is what
> we want.  The issue is the PD692xx's budgeting algorithm is based on
> dynamic power used (i.e. the total power used at any given time).  Since
> this is, well, dynamic, it makes it confusing when a lower priority port
> is powered off due to the runtime behavior of higher-priority ports.
> It's even more confusing if the implicit or default port priorities are
> used.
> 
> Instead, we found that using the maximum power that is allowed be drawn
> by a particular PD's class (set by the IEEE standard) is more user
> friendly, because the set of devices that are powered won't change
> (unless priorities are changed, or the system budget is changed).
> For example, if we've got 4 devices plugged in, and the three highest
> priority devices consume all the power budget, the lowest priority
> device won't ever be powered.  There isn't a case where the lowest
> priority device will be shut down because a higher priority device
> starts consuming more power at some point in the future.
> 
> > We could have several cases for over power budget event:
> > - The power limit exceeded is the one configured for the ports.
> >   We should shutdown only that port without taking care about priority.
> >   TPS23881 has this behavior when power exceed Pcut.
> >   I think the PD692x0 does the same. Need to verify.
> 
> These conditions I'd not call "over power budget events".  I'd call them
> "port overcurrent events" and I agree, those only affect the specific
> problem port.
> 
> > - The power limit exceeded is the global (or manager PD69208M) power budget.
> >   Here port priority is interesting.
> >   Is there a way to know which port create this global power limit excess?
> >   Should we turn off this port even if he don't exceed his own power limit or
> >   should we turn off low priority ports?
> 
> I think it's important to make a distinction between an "overcurrent"
> condition and the condition where we've exceeded the system power
> budget.  An "overcurrent" is port-specific, and can happen if the PD
> consumes more power than the classification of the device allows.  For
> example, if a Class 3 PD (i.e. 802.3at, also referred to as a Type II
> PD) consumes more than 15.4 W at the PSE, it will be shutdown
> immediately.  This support is required by all the IEEE 802.3 standards
> around PoE (.af, .at. and .bt) and is a safety thing.  The TPS2388x
> implements this with Pcut, the LTC4266 impliments this with Icut
> register, and the PD692xx implements it with the port power limit
> registers.  
> 
> The condition where we've exceeded our system-level power
> budget is a little different, in that it causes a port to be shutdown
> despite that port not exceeding it's class power limit.  This condition
> is the case I'm concerned we're solving in this series, and solving it
> for the PD692xx case only, and it's based off dynamic power consumption.
> 
> So I guess I'm suggesting that we take the power budgeting concept out
> of the PSE drivers, and put it into software (either kernel, userspace)
> instead of the PSE hardware.  
> 
> >   I can't find global power budget concept for the TPS23881. 
> 
> This is because this idea doesn't exist on the TPS2388x.  
> 
> >   I could't test this case because I don't have enough load. In fact, maybe by
> >   setting the PD692x0 power bank limit low it could work.
> 
> Hopefully this helps clarify.


Thank you for your detailed insights. Before we dive deeper into policies and
implementations, I’d like to clarify an important point to avoid confusion
later. When comparing different PSE components, it's crucial to note that the
Microchip PD692x0 operates in two distinct categories:
1. PoE controller (PD692x0)
2. PoE manager (PD6920x)

Comparing the PoE controller (PD692x0) with TPS2388x or LTC4266 isn't entirely
fair, as TPS2388x and LTC4266 are more comparable to the PoE manager (PD6920x).
The functionalities provided by the PoE controller (PD692x0) are things we
would need to implement ourselves on the software stack (kernel or userspace).
The budget heuristic that is implemented in the PD692x0's firmware is absent in
TPS2388x and LTC4266.

Policy Variants and Implementation

In cases where we are discussing prioritization, we are fundamentally talking
about over-provisioning. This typically means that while a device advertises a
certain maximum per-port power capacity (e.g., 95W), the total system power
budget (e.g., 300W) is insufficient to supply maximum power to all ports
simultaneously. This is often due to various system limitations, and if there
were no power limits, prioritization wouldn't be necessary.

The challenge then becomes how to squeeze more Powered Devices (PDs) onto one
PSE system. Here are two methods for over-provisioning:

1. Static Method:
 
   This method involves distributing power based on PD classification. It’s
   straightforward and stable, with the software (probably within the PSE
   framework) keeping track of the budget and subtracting the power requested by
   each PD’s class. 
 
   Advantages: Every PD gets its promised power at any time, which guarantees
   reliability. 

   Disadvantages: PD classification steps are large, meaning devices request
   much more power than they actually need. As a result, the power supply may
   only operate at, say, 50% capacity, which is inefficient and wastes money.

2. Dynamic Method:  

   To address the inefficiencies of the static method, vendors like Microchip
   have introduced dynamic power budgeting, as seen in the PD692x0 firmware.
   This method monitors the current consumption per port and subtracts it from
   the available power budget. When the budget is exceeded, lower-priority
   ports are shut down.  

   Advantages: This method optimizes resource utilization, saving costs.

   Disadvantages: Low-priority devices may experience instability. A possible
   improvement could involve using LLDP protocols to dynamically configure
   power limits per port, thus allowing us to reduce power on over-consuming
   ports rather than shutting them down entirely.

Recommendations for Software Handling

Both methods have their pros and cons. Since the dynamic method is not always
desirable, and if there's no way to disable it in the PD692x0's firmware, one
potential workaround could be handling the budget in software and dynamically
setting per-port limits. For instance, with a total budget of 300W and unused
ports, we could initially set 95W limits per port. As high-priority PDs (e.g.,
three 95W devices) are powered, we could dynamically reduce the power limit on
the remaining ports to 15W, ensuring that no device exceeds that classification
threshold.

This is just one idea, and there are likely other policy variants we could
explore. Importantly, I believe these heuristics don’t belong in the kernel
itself. Instead, the kernel should simply provide the necessary interfaces,
leaving the policy implementation to userspace management software. At least
this is a lesson learned from Thermal Management talk at LPC :D

Best regards,  
Oleksij

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 00/12] Add support for PSE port priority

[Kory Maincent]

Hello,

On Thu, 10 Oct 2024 07:42:25 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> > The condition where we've exceeded our system-level power
> > budget is a little different, in that it causes a port to be shutdown
> > despite that port not exceeding it's class power limit.  This condition
> > is the case I'm concerned we're solving in this series, and solving it
> > for the PD692xx case only, and it's based off dynamic power consumption.
> > 
> > So I guess I'm suggesting that we take the power budgeting concept out
> > of the PSE drivers, and put it into software (either kernel, userspace)
> > instead of the PSE hardware.  
> >   
> > >   I can't find global power budget concept for the TPS23881.   
> > 
> > This is because this idea doesn't exist on the TPS2388x.  
> >   
> > >   I could't test this case because I don't have enough load. In fact,
> > > maybe by setting the PD692x0 power bank limit low it could work.  
> > 
> > Hopefully this helps clarify.  
> 
> 
> Thank you for your detailed insights. Before we dive deeper into policies and
> implementations, I’d like to clarify an important point to avoid confusion
> later. When comparing different PSE components, it's crucial to note that the
> Microchip PD692x0 operates in two distinct categories:
> 1. PoE controller (PD692x0)
> 2. PoE manager (PD6920x)
> 
> Comparing the PoE controller (PD692x0) with TPS2388x or LTC4266 isn't entirely
> fair, as TPS2388x and LTC4266 are more comparable to the PoE manager
> (PD6920x). The functionalities provided by the PoE controller (PD692x0) are
> things we would need to implement ourselves on the software stack (kernel or
> userspace). The budget heuristic that is implemented in the PD692x0's
> firmware is absent in TPS2388x and LTC4266.
> 
> Policy Variants and Implementation
> 
> In cases where we are discussing prioritization, we are fundamentally talking
> about over-provisioning. This typically means that while a device advertises a
> certain maximum per-port power capacity (e.g., 95W), the total system power
> budget (e.g., 300W) is insufficient to supply maximum power to all ports
> simultaneously. This is often due to various system limitations, and if there
> were no power limits, prioritization wouldn't be necessary.
> 
> The challenge then becomes how to squeeze more Powered Devices (PDs) onto one
> PSE system. Here are two methods for over-provisioning:
> 
> 1. Static Method:
>  
>    This method involves distributing power based on PD classification. It’s
>    straightforward and stable, with the software (probably within the PSE
>    framework) keeping track of the budget and subtracting the power requested
> by each PD’s class. 
>  
>    Advantages: Every PD gets its promised power at any time, which guarantees
>    reliability. 
> 
>    Disadvantages: PD classification steps are large, meaning devices request
>    much more power than they actually need. As a result, the power supply may
>    only operate at, say, 50% capacity, which is inefficient and wastes money.
> 
> 2. Dynamic Method:  
> 
>    To address the inefficiencies of the static method, vendors like Microchip
>    have introduced dynamic power budgeting, as seen in the PD692x0 firmware.
>    This method monitors the current consumption per port and subtracts it from
>    the available power budget. When the budget is exceeded, lower-priority
>    ports are shut down.  
> 
>    Advantages: This method optimizes resource utilization, saving costs.
> 
>    Disadvantages: Low-priority devices may experience instability. A possible
>    improvement could involve using LLDP protocols to dynamically configure
>    power limits per port, thus allowing us to reduce power on over-consuming
>    ports rather than shutting them down entirely.

Indeed we will have only static method for PSE controllers not supporting system
power budget management like the TPS2388x or LTC426.
Both method could be supported for "smart" PSE controller like PD692x0.

Let's begin with the static method implementation in the PSE framework for now.
It will need the power domain notion you have talked about.

> Recommendations for Software Handling
> 
> Both methods have their pros and cons. Since the dynamic method is not always
> desirable, and if there's no way to disable it in the PD692x0's firmware, one
> potential workaround could be handling the budget in software and dynamically
> setting per-port limits. For instance, with a total budget of 300W and unused
> ports, we could initially set 95W limits per port. As high-priority PDs (e.g.,
> three 95W devices) are powered, we could dynamically reduce the power limit on
> the remaining ports to 15W, ensuring that no device exceeds that
> classification threshold.
> 
> This is just one idea, and there are likely other policy variants we could
> explore. Importantly, I believe these heuristics don’t belong in the kernel
> itself. Instead, the kernel should simply provide the necessary interfaces,
> leaving the policy implementation to userspace management software. At least
> this is a lesson learned from Thermal Management talk at LPC :D

I think the kernel is only missing the PSE notification events to be ready to
leave the port priority policy to the userspace.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 00/12] Add support for PSE port priority

[Kory Maincent]

On Tue, 15 Oct 2024 11:43:52 +0200
Kory Maincent <kory.maincent@bootlin.com> wrote:

> > Policy Variants and Implementation
> > 
> > In cases where we are discussing prioritization, we are fundamentally
> > talking about over-provisioning. This typically means that while a device
> > advertises a certain maximum per-port power capacity (e.g., 95W), the total
> > system power budget (e.g., 300W) is insufficient to supply maximum power to
> > all ports simultaneously. This is often due to various system limitations,
> > and if there were no power limits, prioritization wouldn't be necessary.
> > 
> > The challenge then becomes how to squeeze more Powered Devices (PDs) onto
> > one PSE system. Here are two methods for over-provisioning:
> > 
> > 1. Static Method:
> >  
> >    This method involves distributing power based on PD classification. It’s
> >    straightforward and stable, with the software (probably within the PSE
> >    framework) keeping track of the budget and subtracting the power
> > requested by each PD’s class. 
> >  
> >    Advantages: Every PD gets its promised power at any time, which
> > guarantees reliability. 
> > 
> >    Disadvantages: PD classification steps are large, meaning devices request
> >    much more power than they actually need. As a result, the power supply
> > may only operate at, say, 50% capacity, which is inefficient and wastes
> > money.
> > 
> > 2. Dynamic Method:  
> > 
> >    To address the inefficiencies of the static method, vendors like
> > Microchip have introduced dynamic power budgeting, as seen in the PD692x0
> > firmware. This method monitors the current consumption per port and
> > subtracts it from the available power budget. When the budget is exceeded,
> > lower-priority ports are shut down.  
> > 
> >    Advantages: This method optimizes resource utilization, saving costs.
> > 
> >    Disadvantages: Low-priority devices may experience instability. A
> > possible improvement could involve using LLDP protocols to dynamically
> > configure power limits per port, thus allowing us to reduce power on
> > over-consuming ports rather than shutting them down entirely.  
> 
> Indeed we will have only static method for PSE controllers not supporting
> system power budget management like the TPS2388x or LTC426.
> Both method could be supported for "smart" PSE controller like PD692x0.
> 
> Let's begin with the static method implementation in the PSE framework for
> now. It will need the power domain notion you have talked about.

While developing the software support for port priority in static method, I
faced an issue.

Supposing we are exceeding the power budget when we plug a new PD.
The port power should not be enabled directly or magic smoke will appear.
So we have to separate the detection part to know the needs of the PD from the
power enable part.

Currently the port power is enabled on the hardware automatically after the
detection process. There is no way to separate power port process and detection
process with the PD692x0 controller and it could be done on the TPS23881 by
configuring it to manual mode but: "The use of this mode is intended for system
diagnostic purposes only in the event that ports cannot be powered in
accordance with the IEEE 802.3bt standard from semiauto or auto modes."
Not sure we want that.

So in fact the workaround you talked about above will be needed for the two PSE
controllers.
 
> Both methods have their pros and cons. Since the dynamic method is not always
> desirable, and if there's no way to disable it in the PD692x0's firmware, one
> potential workaround could be handling the budget in software and dynamically
> setting per-port limits. For instance, with a total budget of 300W and unused
> ports, we could initially set 95W limits per port. As high-priority PDs (e.g.,
> three 95W devices) are powered, we could dynamically reduce the power limit on
> the remaining ports to 15W, ensuring that no device exceeds that
> classification threshold.

We would set port overcurrent limit for all unpowered ports when the power
budget available is less than max PI power 100W as you described.
If a new PD plugged exceed the overcurrent limit then it will raise an interrupt
and we could deal with the power budget to turn off low priority ports at that
time. 

Mmh in fact I could not know if the overcurrent event interrupt comes from a
newly plugged PD or not.

An option: When we get new PD device plug interrupt event, we wait the end of
classification time (Tpon 400ms) and read the interrupt states again to know if
there is an overcurrent or not on the port.

What do you think?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next 00/12] Add support for PSE port priority

[Oleksij Rempel]

On Thu, Oct 17, 2024 at 12:35:57PM +0200, Kory Maincent wrote:
> On Tue, 15 Oct 2024 11:43:52 +0200
> Kory Maincent <kory.maincent@bootlin.com> wrote:
> 
> > > Policy Variants and Implementation
> > > 
> > > In cases where we are discussing prioritization, we are fundamentally
> > > talking about over-provisioning. This typically means that while a device
> > > advertises a certain maximum per-port power capacity (e.g., 95W), the total
> > > system power budget (e.g., 300W) is insufficient to supply maximum power to
> > > all ports simultaneously. This is often due to various system limitations,
> > > and if there were no power limits, prioritization wouldn't be necessary.
> > > 
> > > The challenge then becomes how to squeeze more Powered Devices (PDs) onto
> > > one PSE system. Here are two methods for over-provisioning:
> > > 
> > > 1. Static Method:
> > >  
> > >    This method involves distributing power based on PD classification. It’s
> > >    straightforward and stable, with the software (probably within the PSE
> > >    framework) keeping track of the budget and subtracting the power
> > > requested by each PD’s class. 
> > >  
> > >    Advantages: Every PD gets its promised power at any time, which
> > > guarantees reliability. 
> > > 
> > >    Disadvantages: PD classification steps are large, meaning devices request
> > >    much more power than they actually need. As a result, the power supply
> > > may only operate at, say, 50% capacity, which is inefficient and wastes
> > > money.
> > > 
> > > 2. Dynamic Method:  
> > > 
> > >    To address the inefficiencies of the static method, vendors like
> > > Microchip have introduced dynamic power budgeting, as seen in the PD692x0
> > > firmware. This method monitors the current consumption per port and
> > > subtracts it from the available power budget. When the budget is exceeded,
> > > lower-priority ports are shut down.  
> > > 
> > >    Advantages: This method optimizes resource utilization, saving costs.
> > > 
> > >    Disadvantages: Low-priority devices may experience instability. A
> > > possible improvement could involve using LLDP protocols to dynamically
> > > configure power limits per port, thus allowing us to reduce power on
> > > over-consuming ports rather than shutting them down entirely.  
> > 
> > Indeed we will have only static method for PSE controllers not supporting
> > system power budget management like the TPS2388x or LTC426.
> > Both method could be supported for "smart" PSE controller like PD692x0.
> > 
> > Let's begin with the static method implementation in the PSE framework for
> > now. It will need the power domain notion you have talked about.
> 
> While developing the software support for port priority in static method, I
> faced an issue.
> 
> Supposing we are exceeding the power budget when we plug a new PD.
> The port power should not be enabled directly or magic smoke will appear.
> So we have to separate the detection part to know the needs of the PD from the
> power enable part.
> 
> Currently the port power is enabled on the hardware automatically after the
> detection process. There is no way to separate power port process and detection
> process with the PD692x0 controller and it could be done on the TPS23881 by
> configuring it to manual mode but: "The use of this mode is intended for system
> diagnostic purposes only in the event that ports cannot be powered in
> accordance with the IEEE 802.3bt standard from semiauto or auto modes."
> Not sure we want that.
> 
> So in fact the workaround you talked about above will be needed for the two PSE
> controllers.

For the TPS23881, "9.1.1.2 Semiauto", seems to be exactly what we wont:
"The port performs detection and classification (if valid detection
occurs) continuously. Registers are updated each time a detection or
classification occurs. The port power is not automatically turned on. A
Power Enable command is required to turn on the port"

For PD692x0 controller, i'm not 100% sure. There is "4.3.5 Set Enable/Disable
Channels" command, "Sets individual port Enable (Delivering power
enable) or Disable (Delivering power disable)." 

For my understanding, "Delivering power" is the state after
classification. So, it is what we wont too.

If, it works in both cases, it would be a more elegant way to go. THe
controller do auto- detection and classification, what we should do in
the software is do decide if the PD can be enabled based on
classification results, priority and available budget.

> > Both methods have their pros and cons. Since the dynamic method is not always
> > desirable, and if there's no way to disable it in the PD692x0's firmware, one
> > potential workaround could be handling the budget in software and dynamically
> > setting per-port limits. For instance, with a total budget of 300W and unused
> > ports, we could initially set 95W limits per port. As high-priority PDs (e.g.,
> > three 95W devices) are powered, we could dynamically reduce the power limit on
> > the remaining ports to 15W, ensuring that no device exceeds that
> > classification threshold.
> 
> We would set port overcurrent limit for all unpowered ports when the power
> budget available is less than max PI power 100W as you described.
> If a new PD plugged exceed the overcurrent limit then it will raise an interrupt
> and we could deal with the power budget to turn off low priority ports at that
> time. 

> Mmh in fact I could not know if the overcurrent event interrupt comes from a
> newly plugged PD or not.

Hm..  in case of PD692x0, may be using event counters?

> An option: When we get new PD device plug interrupt event, we wait the end of
> classification time (Tpon 400ms) and read the interrupt states again to know if
> there is an overcurrent or not on the port.

Let's try Semiauto mode for TPS23881 first, I assume it is designed
exactly for this use case.

And then, test if PD692x0 supports a way to disable auto power delivery
in the 4.3.5 command.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 00/12] Add support for PSE port priority

[Kory Maincent]

On Fri, 18 Oct 2024 08:14:26 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> On Thu, Oct 17, 2024 at 12:35:57PM +0200, Kory Maincent wrote:
> > On Tue, 15 Oct 2024 11:43:52 +0200
> > Kory Maincent <kory.maincent@bootlin.com> wrote:
> >   
>  [...]  
> > > 
> > > Indeed we will have only static method for PSE controllers not supporting
> > > system power budget management like the TPS2388x or LTC426.
> > > Both method could be supported for "smart" PSE controller like PD692x0.
> > > 
> > > Let's begin with the static method implementation in the PSE framework for
> > > now. It will need the power domain notion you have talked about.  
> > 
> > While developing the software support for port priority in static method, I
> > faced an issue.
> > 
> > Supposing we are exceeding the power budget when we plug a new PD.
> > The port power should not be enabled directly or magic smoke will appear.
> > So we have to separate the detection part to know the needs of the PD from
> > the power enable part.
> > 
> > Currently the port power is enabled on the hardware automatically after the
> > detection process. There is no way to separate power port process and
> > detection process with the PD692x0 controller and it could be done on the
> > TPS23881 by configuring it to manual mode but: "The use of this mode is
> > intended for system diagnostic purposes only in the event that ports cannot
> > be powered in accordance with the IEEE 802.3bt standard from semiauto or
> > auto modes." Not sure we want that.
> > 
> > So in fact the workaround you talked about above will be needed for the two
> > PSE controllers.  
> 
> For the TPS23881, "9.1.1.2 Semiauto", seems to be exactly what we wont:
> "The port performs detection and classification (if valid detection
> occurs) continuously. Registers are updated each time a detection or
> classification occurs. The port power is not automatically turned on. A
> Power Enable command is required to turn on the port"

I tested reading the assigned class and not the requested class register so I
thought it was not working but indeed it detects the class even if the port
power is off. That's what I was looking for, nice!
Just figured out also that calling pwoff is reseting detection, classification,
power policy... So the port need to be setup again after a pwoff.
 
> For PD692x0 controller, i'm not 100% sure. There is "4.3.5 Set Enable/Disable
> Channels" command, "Sets individual port Enable (Delivering power
> enable) or Disable (Delivering power disable)." 
> 
> For my understanding, "Delivering power" is the state after
> classification. So, it is what we wont too.

On the PD692x0 there is also a requested class and power value but it stay "to
no class detected value" (0xc) if the port is not enabled.
It did not find a way to detect the class and keep port power off.
 
> If, it works in both cases, it would be a more elegant way to go. THe
> controller do auto- detection and classification, what we should do in
> the software is do decide if the PD can be enabled based on
> classification results, priority and available budget.
> 
> > > Both methods have their pros and cons. Since the dynamic method is not
> > > always desirable, and if there's no way to disable it in the PD692x0's
> > > firmware, one potential workaround could be handling the budget in
> > > software and dynamically setting per-port limits. For instance, with a
> > > total budget of 300W and unused ports, we could initially set 95W limits
> > > per port. As high-priority PDs (e.g., three 95W devices) are powered, we
> > > could dynamically reduce the power limit on the remaining ports to 15W,
> > > ensuring that no device exceeds that classification threshold.  
> > 
> > We would set port overcurrent limit for all unpowered ports when the power
> > budget available is less than max PI power 100W as you described.
> > If a new PD plugged exceed the overcurrent limit then it will raise an
> > interrupt and we could deal with the power budget to turn off low priority
> > ports at that time.   
> 
> > Mmh in fact I could not know if the overcurrent event interrupt comes from a
> > newly plugged PD or not.  
> 
> Hm..  in case of PD692x0, may be using event counters?

Counters? I don't see how.

> > An option: When we get new PD device plug interrupt event, we wait the end
> > of classification time (Tpon 400ms) and read the interrupt states again to
> > know if there is an overcurrent or not on the port.  
> 
> Let's try Semiauto mode for TPS23881 first, I assume it is designed
> exactly for this use case.

Yes,

> And then, test if PD692x0 supports a way to disable auto power delivery
> in the 4.3.5 command.

I don't have this 4.3.5 command. Are you refering to another document than the
communication protocol version 3.55 document?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com