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

[Jonas Jelonek <jelonek.jonas@gmail.com>]

Hi Paolo,

On 03.07.26 09:55, Paolo Abeni wrote:
> 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
>
Thanks for your remarks, I'll address them in v5.

Best regards,
Jonas