Message-ID: <Z0WJAzkgq4Qr-xLU@pengutronix.de>
Date: Tue, 26 Nov 2024 09:38:27 +0100
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Kory Maincent <kory.maincent@...tlin.com>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jonathan Corbet <corbet@....net>,
	Donald Hunter <donald.hunter@...il.com>,
	Rob Herring <robh@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>,
	Simon Horman <horms@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, Kyle Swenson <kyle.swenson@....tech>,
	Dent Project <dentproject@...uxfoundation.org>,
	kernel@...gutronix.de,
	Maxime Chevallier <maxime.chevallier@...tlin.com>
Subject: Re: [PATCH RFC net-next v3 21/27] net: pse-pd: Add support for
 getting and setting port priority

On Thu, Nov 21, 2024 at 03:42:47PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@...tlin.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.
> 
> This patch add support for two mode of port priority modes.

Priorit mode is too abstract for me, in this case we are talking about
Budget Evaluation Strategy.

> 1. Static Method:
> 
>    This method involves distributing power based on PD classification.
>    It’s straightforward and stable, the PSE core 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.
> 
>    Priority max value is matching the number of PSE PIs within the PSE.
> 
> 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.
> 
>    Priority max value is set by the PSE controller driver.
> 
> Signed-off-by: Kory Maincent <kory.maincent@...tlin.com>
> ---
> 
> Change in v3:
> - Add disconnection policy.
> - Add management of disabled port priority in the interrupt handler.
> - Move port prio mode in the power domain instead of the PSE.
> 
> Change in v2:
> - Rethink the port priority support.
> ---
>  drivers/net/pse-pd/pse_core.c | 550 +++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pse-pd/pse.h    |  63 +++++
>  include/uapi/linux/ethtool.h  |  73 ++++++
>  3 files changed, 676 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index ff0ffbccf139..f15a693692ae 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -40,10 +40,17 @@ struct pse_control {
>   * struct pse_power_domain - a PSE power domain
>   * @id: ID of the power domain
>   * @supply: Power supply the Power Domain
> + * @port_prio_mode: Current port priority mode of the power domain

Same here, it is Budget Evaluation Strategy. May be: budget_eval_strategy

> + * @discon_pol: Current disonnection policy of the power domain
> + * @pi_lrc_id: ID of the last recently connected PI. -1 if none. Relevant
> + *	       for static port priority mode.
>   */
>  struct pse_power_domain {
>  	int id;
>  	struct regulator *supply;
> +	u32 port_prio_mode;
> +	u32 discon_pol;
> +	int pi_lrc_id;

We should store all ports withing the domain in a list and process the
list backwards or forwards, depending on disconnection policy.

>  };
>  
>  static int of_load_single_pse_pi_pairset(struct device_node *node,
> @@ -222,6 +229,33 @@ static int of_load_pse_pis(struct pse_controller_dev *pcdev)
>  	return ret;
>  }
>  
> +static void pse_pi_deallocate_pw_budget(struct pse_pi *pi)
> +{
> +	if (!pi->pw_d)
> +		return;
> +
> +	regulator_free_power_budget(pi->pw_d->supply, pi->pw_allocated);
> +}
> +
> +/* Helper returning true if the power control is managed from the software
> + * in the interrupt handler
> + */

Please use function comment format. I would really love to have comments
on all new functions.

> +static bool pse_pw_d_is_sw_pw_control(struct pse_controller_dev *pcdev,
> +				      struct pse_power_domain *pw_d)
> +{
> +	if (!pw_d)
> +		return false;
> +
> +	if (pw_d->port_prio_mode & ETHTOOL_PSE_PORT_PRIO_STATIC)

here should be pw_d->port_prio_mode == ETHTOOL_PSE_PORT_PRIO_STATIC

We can't support multiple evaluation strategies per port.

> +		return true;
> +	if (pw_d->port_prio_mode == ETHTOOL_PSE_PORT_PRIO_DISABLED &&
> +	    pcdev->ops->pi_enable && pcdev->ops->pi_get_pw_req &&
> +	    pcdev->irq)
> +		return true;
> +
> +	return false;
> +}
> +

....

> +int pse_ethtool_set_prio_mode(struct pse_control *psec,
> +			      struct netlink_ext_ack *extack,
> +			      u32 prio_mode)
> +{
> +	struct pse_controller_dev *pcdev = psec->pcdev;
> +	const struct pse_controller_ops *ops;
> +	int ret = 0, i;
> +
> +	if (!(prio_mode & pcdev->port_prio_supp_modes)) {
> +		NL_SET_ERR_MSG(extack, "priority mode not supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (!pcdev->pi[psec->id].pw_d) {
> +		NL_SET_ERR_MSG(extack, "no power domain attached");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/* ETHTOOL_PSE_PORT_PRIO_DISABLED can't be ORed with another mode */
> +	if (prio_mode & ETHTOOL_PSE_PORT_PRIO_DISABLED &&
> +	    prio_mode & ~ETHTOOL_PSE_PORT_PRIO_DISABLED) {
> +		NL_SET_ERR_MSG(extack,
> +			       "port priority can't be enabled and disabled simultaneously");
> +		return -EINVAL;
> +	}
> +
> +	ops = psec->pcdev->ops;
> +
> +	/* We don't want priority mode change in the middle of an
> +	 * enable/disable call
> +	 */
> +	mutex_lock(&pcdev->lock);
> +	pcdev->pi[psec->id].pw_d->port_prio_mode = prio_mode;

In proposed implementation we have can set policies per port, but it
will affect complete domain. This is not good. It feels like a separate
challenge with extra discussion and work. I would recommend not to
implement policy setting right now.

If you will decide to implement setting of policies anyway, then we need
to discuss the interface.
- If the policy should be done per domain, then we will need a separate
  interface to interact with domains.
  Pro: seems to be easier to implement.
- If we will go with policy per port, wich would make sense too, then
  some rework of this patch is needed.
  Pro: can combine best of both strategies: set ports with wide load
  range to static strategy and use dynamic strategy on other ports.

Right now we do not have software implementation for dynamic mode,
implementing configuration of the policies from user space can be
implemented later. It is enough to provide information about what
hard coded policy is currently used.

>  /* Maximum current in uA according to IEEE 802.3-2022 Table 145-1 */
>  #define MAX_PI_CURRENT 1920000
> @@ -62,6 +63,13 @@ 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_supp_modes: PSE port priority modes supported. Set by PSE core.
> + * @c33_prio_mode: PSE port priority mode selected. Set by PSE core.
> + * @c33_prio_max: max priority allowed for the c33_prio variable value. Set
> + *	by PSE core.
> + * @c33_prio: priority of the PSE. Set by PSE core in case of static port
> + *	priority mode.
> + * @c33_discon_pol: PSE disconnection policy selected. Set by PSE core.

Priority configuration is not port of IEEE 802.3 specification. c33_
prefix should be removed here.

>   */
>  struct pse_control_status {
>  	u32 pse_id;
> @@ -76,6 +84,11 @@ 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_supp_modes;
> +	enum pse_port_prio_modes c33_prio_mode;
> +	u32 c33_prio_max;
> +	u32 c33_prio;
> +	u32 c33_discon_pol;
>  };

....

>  struct module;
> @@ -143,6 +163,12 @@ struct pse_pi_pairset {
>   * @rdev: regulator represented by the PSE PI
>   * @admin_state_enabled: PI enabled state
>   * @pw_d: Power domain of the PSE PI
> + * @prio: Priority of the PSE PI. Used in static port priority mode
> + * @isr_enabled: PSE PI power status managed by the interruption handler.
> + *		 This variable is relevant when the power enabled management
> + *		 is a managed in software like the static port priority mode.
> + * @pw_allocated: Power allocated to a PSE PI to manage power budget in
> + *	static port priority mode
>   */
>  struct pse_pi {
>  	struct pse_pi_pairset pairset[2];
> @@ -150,6 +176,9 @@ struct pse_pi {
>  	struct regulator_dev *rdev;
>  	bool admin_state_enabled;
>  	struct pse_power_domain *pw_d;
> +	int prio;
> +	bool isr_enabled;
> +	int pw_allocated;

s/pw_allocated/pw_allocated_mw/

>  	return false;
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index a4c93d6de218..b6727049840c 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1002,11 +1002,84 @@ enum ethtool_c33_pse_pw_d_status {
>   * enum ethtool_pse_events - event list of the PSE controller.
>   * @ETHTOOL_PSE_EVENT_OVER_CURRENT: PSE output current is too high.
>   * @ETHTOOL_PSE_EVENT_OVER_TEMP: PSE in over temperature state.
> + * @ETHTOOL_C33_PSE_EVENT_DETECTION: detection process occur on the PSE.
> + *	IEEE 802.3-2022 145.2.6 PSE detection of PDs

33.2.5 and 145.2.6 -> 30.9.1.1.5 aPSEPowerDetectionStatus

> + * @ETHTOOL_C33_PSE_EVENT_CLASSIFICATION: classification process occur on
> + *	the PSE. IEEE 802.3-2022 145.2.8 PSE classification of PDs and
> + *	mutual identification

33.2.6 and 145.2.8 -> 30.9.1.1.8 aPSEPowerClassification

> + * @ETHTOOL_C33_PSE_EVENT_DISCONNECTION: PD has been disconnected on the PSE.

This one seems to be related to following parts of specification:
33.3.8 and 145.3.9 PD Maintain Power Signature
33.5.1.2.9 MPS Absent
30.9.1.1.20 aPSEMPSAbsentCounter

> + * @ETHTOOL_PSE_EVENT_SW_PW_CONTROL_ERROR: PSE faced an error managing the
> + *	power control from software.
>   */
>  
>  enum ethtool_pse_events {
>  	ETHTOOL_PSE_EVENT_OVER_CURRENT =	1 << 0,
>  	ETHTOOL_PSE_EVENT_OVER_TEMP =		1 << 1,
> +	ETHTOOL_C33_PSE_EVENT_DETECTION =	1 << 2,
> +	ETHTOOL_C33_PSE_EVENT_CLASSIFICATION =	1 << 3,
> +	ETHTOOL_C33_PSE_EVENT_DISCONNECTION =	1 << 4,
> +	ETHTOOL_PSE_EVENT_SW_PW_CONTROL_ERROR =	1 << 5,
> +};
-- 
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 |

Powered by blists - more mailing lists