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

[Oleksij Rempel <o.rempel@pengutronix.de>]

Hi Jonas,

On Tue, Jun 30, 2026 at 10:56:50AM +0000, 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>

LGTM, there are some nitpicks from my side
otherwise:
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

...
> +/* Shorthand for the designated-initializer entries in dialect opcode tables. */
> +#define RTPSE_MCU_OP(opc)	{ .op = (opc), .valid = true }
> +
> +/* Forward-declared so dialects can supply response parsers (defined below). */

No forward declarations please move the structs here.

> +struct rtpse_mcu_info;
> +struct rtpse_mcu_port_status;
> +
> +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);
> +	int (*parse_port_class)(const struct rtpse_mcu_port_status *status);
> +	const char *(*mcu_type_str)(unsigned int mcu_type);
> +};
> +struct rtpse_mcu_port_config {
> +	bool enable;

in this struct we use only enable, do you plan to wire it somewhere
later? I assume you wont to keep it as documentation. May be add some
debug traces? And add some comments what do you already know about this
fields.

> +	u8 function_mode;
> +	u8 detection_type;
> +	u8 cls_type;
> +	u8 disconnect_type;
> +	u8 pair_type;
> +};
> +
> +struct rtpse_mcu_port_ext_config {

Same here.

> +	u8 inrush_mode;
> +	u8 limit_type;
> +	u8 max_power;
> +	u8 priority;
> +	u8 chip_addr;
> +	u8 channel;
> +};


> +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);

Add some define for 0xff.
Can max_mW be zero?

> +
> +	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);

In the drivers/net, All declarations should be in reverse tree style.

> +	range = kzalloc_obj(*range, GFP_KERNEL);
> +	if (!range)
> +		return -ENOMEM;
> +
> +	range[0].min = 0;
> +	range[0].max = pse->chip->max_mW_per_port;
> +
> +	out->c33_pw_limit_ranges = range;
> +	return 1;
> +}
> +


> +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)

For this section sashiko has an opinion:
https://sashiko.dev/#/patchset/20260630105651.756058-1-jelonek.jonas%40gmail.com

> +			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);

Reduce it to debug level print.

> +	return 0;
> +}
> +

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 |