Re: [PATCH net-next v4 2/2] net: pse-pd: add Realtek/Broadcom PSE MCU driver

[Paolo Abeni <pabeni@redhat.com>]

On 6/30/26 12:56 PM, Jonas Jelonek wrote:
> A range of PoE switches use a small microcontroller on the PCB to front
> the actual PSE silicon. The host CPU talks to that MCU over I2C/SMBus or
> UART using a fixed 12-byte request/response protocol with a trailing
> checksum; the PSE chips are managed by the MCU and are not accessed
> directly. The same protocol family is spoken by Realtek and Broadcom PSE
> MCUs, diverging in opcode numbering and a few response layouts, which the
> driver abstracts behind a per-dialect opcode table and parser hooks
> selected by the compatible. The specific PSE chip behind the MCU is
> detected at runtime and only influences per-chip constants (power scaling
> and the per-port cap).
> 
> The driver is split into a shared core and two transport modules:
> 
> - PSE_REALTEK_MCU: protocol, message framing, dialect machinery, and the
>   pse_controller_ops glue.
> - PSE_REALTEK_MCU_I2C / PSE_REALTEK_MCU_UART: transport modules
>   registering the MCU on an I2C bus or a serdev port respectively.
> 
> The realtek-pse-mcu-* files and PSE_REALTEK_MCU* symbols match the
> realtek,pse-mcu-rtk / realtek,pse-mcu-brcm compatibles: all name the
> Realtek PSE-MCU front-end, not the MCU silicon or the PSE chip behind
> it (see the binding for the prefix rationale). Broadcom PSE MCUs speak
> the same protocol family and are handled by the same shared core
> through the dialect abstraction selected by the '-brcm' compatible.
> 
> Power budgeting is left to the MCU firmware; the driver advertises
> PSE_BUDGET_EVAL_STRAT_DYNAMIC (controller-managed budget) accordingly.
> 
> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
> ---
>  MAINTAINERS                               |    7 +
>  drivers/net/pse-pd/Kconfig                |   28 +
>  drivers/net/pse-pd/Makefile               |    3 +
>  drivers/net/pse-pd/realtek-pse-mcu-core.c | 1019 +++++++++++++++++++++
>  drivers/net/pse-pd/realtek-pse-mcu-i2c.c  |  163 ++++
>  drivers/net/pse-pd/realtek-pse-mcu-uart.c |  156 ++++
>  drivers/net/pse-pd/realtek-pse-mcu.h      |   87 ++
>  7 files changed, 1463 insertions(+)
>  create mode 100644 drivers/net/pse-pd/realtek-pse-mcu-core.c
>  create mode 100644 drivers/net/pse-pd/realtek-pse-mcu-i2c.c
>  create mode 100644 drivers/net/pse-pd/realtek-pse-mcu-uart.c
>  create mode 100644 drivers/net/pse-pd/realtek-pse-mcu.h

This is quite large, and shouls be split in smaller patches to help
reviewers.

[...]
> +struct rtpse_mcu_dialect {
> +	struct rtpse_mcu_opcode opcode[RTPSE_MCU_NUM_CMDS];
> +
> +	/*
> +	 * Response parsers. Each dialect must supply its own; the core calls
> +	 * these unconditionally rather than carrying a default that would
> +	 * silently mis-decode bytes from a dialect that forgot to set them.
> +	 */
> +	int (*parse_system_info)(const u8 *payload, struct rtpse_mcu_info *info);

The 2 existing implementation always return 0; you may consider change
it to a void function.

> +static int rtpse_mcu_port_get_voltage(struct pse_controller_dev *pcdev, int id)
> +{
> +	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
> +	struct rtpse_mcu_port_measurement measurement;
> +	int ret;
> +	u32 uV;
> +
> +	ret = rtpse_mcu_port_get_measurement(pse, id, &measurement);
> +	if (ret)
> +		return ret;
> +
> +	/* 64.45mV per LSB */
> +	uV = (u32)measurement.voltage_raw * 64450U;

This cast    ^^^^^ should be unneeded.

> +	return min_t(u32, uV, INT_MAX);
> +}
> +
> +static int rtpse_mcu_port_enable(struct pse_controller_dev *pcdev, int id)
> +{
> +	return rtpse_mcu_port_set_state(to_rtpse_mcu_ctrl(pcdev), id, true);
> +}
> +
> +static int rtpse_mcu_port_disable(struct pse_controller_dev *pcdev, int id)
> +{
> +	return rtpse_mcu_port_set_state(to_rtpse_mcu_ctrl(pcdev), id, false);
> +}
> +
> +static int rtpse_mcu_port_get_pw_limit(struct pse_controller_dev *pcdev, int id)
> +{
> +	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
> +	struct rtpse_mcu_port_ext_config config;
> +	int ret;
> +
> +	ret = rtpse_mcu_port_get_ext_config(pse, id, &config);
> +	if (ret)
> +		return ret;
> +
> +	return config.max_power * pse->chip->pw_read_lsb_mW;
> +}
> +
> +static int rtpse_mcu_port_set_pw_limit(struct pse_controller_dev *pcdev, int id, int max_mW)
> +{
> +	const struct rtpse_mcu_opcode *type_opc, *val_opc;
> +	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
> +	const struct rtpse_mcu_chip_info *chip = pse->chip;
> +	unsigned int prg_val;
> +	int ret;
> +
> +	if (max_mW < 0 || max_mW > chip->max_mW_per_port)
> +		return -ERANGE;
> +
> +	type_opc = &pse->dialect->opcode[RTPSE_MCU_CMD_PORT_SET_POWER_LIMIT_TYPE];
> +	val_opc = &pse->dialect->opcode[chip->pw_set_cmd];
> +	if (!type_opc->valid || !val_opc->valid)
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * Switch the port to user-defined limit mode first, then program the
> +	 * limit value. If the second cmd fails, the port is left in
> +	 * user-defined mode but with the previous limit value; the next
> +	 * successful set_pw_limit call recovers it.
> +	 */
> +	ret = rtpse_mcu_port_cmd(pse, id, type_opc->op, RTPSE_MCU_PORT_PW_LIMIT_TYPE_USER);
> +	if (ret)
> +		return ret;
> +
> +	prg_val = min_t(unsigned int, max_mW / chip->pw_set_lsb_mW, 0xff);
> +
> +	return rtpse_mcu_port_cmd(pse, id, val_opc->op, prg_val);
> +}
> +
> +static int rtpse_mcu_port_get_pw_limit_ranges(struct pse_controller_dev *pcdev, int id,
> +					      struct pse_pw_limit_ranges *out)
> +{
> +	struct ethtool_c33_pse_pw_limit_range *range;
> +	struct rtpse_mcu_ctrl *pse = to_rtpse_mcu_ctrl(pcdev);
> +
> +	range = kzalloc_obj(*range, GFP_KERNEL);

or just:

	range = kzalloc_obj(*range);


> +static int rtpse_mcu_discover(struct rtpse_mcu_ctrl *pse, struct rtpse_mcu_info *info)
> +{
> +	struct rtpse_mcu_ext_config ext_config;
> +	unsigned long deadline;
> +	int ret;
> +
> +	/*
> +	 * The MCU may not answer on the bus yet right after power-up or
> +	 * enable-gpios assertion: depending on the transport it either stays
> +	 * silent (-ETIMEDOUT) or does not ACK its address at all (-ENXIO /
> +	 * -EREMOTEIO). Retry within a bounded wall-time window so a slow boot
> +	 * still probes, while a genuinely unresponsive MCU fails with its real
> +	 * error instead of deferring forever and masking it.
> +	 */
> +	deadline = jiffies + msecs_to_jiffies(RTPSE_MCU_BOOT_TIMEOUT_MS);
> +	do {
> +		ret = rtpse_mcu_get_info(pse, info);
> +		if (ret != -ETIMEDOUT && ret != -ENXIO && ret != -EREMOTEIO &&
> +		    ret != -EAGAIN)
> +			break;
> +		msleep(RTPSE_MCU_BOOT_RETRY_MS);
> +	} while (time_before(jiffies, deadline));
> +	if (ret)
> +		return dev_err_probe(pse->dev, ret, "failed to read MCU info\n");
> +
> +	switch (info->device_id) {
> +	case RTPSE_MCU_DEVICE_ID_RTL8238B:
> +		pse->chip = &rtl8238b_info;
> +		break;
> +	case RTPSE_MCU_DEVICE_ID_RTL8239:
> +		pse->chip = &rtl8239_info;
> +		break;
> +	case RTPSE_MCU_DEVICE_ID_RTL8239C:
> +		pse->chip = &rtl8239c_info;
> +		break;
> +	case RTPSE_MCU_DEVICE_ID_BCM59111:
> +		pse->chip = &bcm59111_info;
> +		break;
> +	case RTPSE_MCU_DEVICE_ID_BCM59121:
> +		pse->chip = &bcm59121_info;
> +		break;
> +	default:
> +		return dev_err_probe(pse->dev, -EINVAL, "unknown PSE id 0x%x\n",
> +				     info->device_id);
> +	}
> +
> +	if (!info->max_ports || info->max_ports > RTPSE_MCU_MAX_PORTS)
> +		return dev_err_probe(pse->dev, -EINVAL,
> +				     "MCU reports invalid port count %u\n", info->max_ports);
> +
> +	ret = rtpse_mcu_get_ext_config(pse, &ext_config);
> +	if (ret)
> +		return dev_err_probe(pse->dev, ret, "failed to read MCU ext config\n");
> +
> +	dev_info(pse->dev, "%s MCU, %s (id 0x%04x), %u ports across %u PSE chip(s)\n",
> +		 pse->dialect->mcu_type_str(info->mcu_type), pse->chip->name,
> +		 info->device_id, info->max_ports, ext_config.num_of_pses);

The general guidance is to try to avoid unneeded print on dmsg, as they
tend to scary admins, but I personally agree on message on modprobe.

No strong opionion either ways.

/P