Re: [PATCH net-next 3/4] net: pse-pd: realtek-pse-mcu: configure global power budget

[Jonas Jelonek]

Hi Sander,

On 05.11.26 22:51, Sander Vanheule wrote:
> ...
>>  
>> +static int rtpse_mcu_get_global_budget_idx(struct rtpse_mcu_ctrl *pse, u8
>> *budget_idx)
>> +{
>> +	struct rtpse_mcu_msg req, resp;
>> +	const struct rtpse_mcu_opcode *opc;
>> +	int ret;
>> +
>> +	opc = &pse->dialect->opcode[RTPSE_MCU_CMD_GET_POWER_STATUS];
>> +	if (!opc->valid)
>> +		return -EOPNOTSUPP;
>> +
>> +	rtpse_mcu_msg_init(&req, opc->op);
>> +	ret = rtpse_mcu_do_xfer(pse, &req, &resp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (resp.payload[4] > 0x7)
>> +		return -ERANGE;
>> +
>> +	*budget_idx = resp.payload[4];
>> +	return 0;
>> +}
>> +
>> +static int rtpse_mcu_set_global_budget(struct rtpse_mcu_ctrl *pse, u8
>> budget_idx,
>> +				       u16 budget_raw)
>> +{
>> +	const struct rtpse_mcu_opcode *opc;
>> +	struct rtpse_mcu_msg req, resp;
>> +	int ret;
>> +
>> +	opc = &pse->dialect->opcode[RTPSE_MCU_CMD_SET_GLOBAL_BUDGET];
>> +	if (!opc->valid)
>> +		return -EOPNOTSUPP;
>> +
>> +	rtpse_mcu_msg_init(&req, opc->op);
>> +	req.payload[0] = budget_idx;
>> +	put_unaligned_be16(budget_raw, &req.payload[1]);
>> +	put_unaligned_be16(0, &req.payload[3]);
> You don't want to configure a guard band? Can this lead to flapping of devices,
> where a port is rapidly switched off and on by the consumed power going above
> and below the power limit?

Maybe I got the idea of that guard band wrong, haven't found a good
explanation of that setting either. I just wanted to avoid setting any
guessed value here which reduces the global budget from e.g. 400W to
380W without specific reason.

>> +
>> +	ret = rtpse_mcu_do_xfer(pse, &req, &resp);
>> +	if (ret)
>> +		return ret;
>> +	if (resp.payload[0] != budget_idx || resp.payload[1] != 0)
>> +		return -EIO;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rtpse_mcu_apply_global_budget(struct rtpse_mcu_ctrl *pse,
>> +					 struct regulator *vmain)
>> +{
>> +	int budget, ret;
>> +	u16 budget_raw;
>> +	u8 budget_idx;
>> +
>> +	budget = regulator_get_unclaimed_power_budget(vmain);
>> +	if (budget == INT_MAX)
>> +		return -ENODATA;
>> +	if (budget < 0 || budget > U16_MAX * RTPSE_MCU_GLOBAL_BUDGET_LSB_MW)
>> +		return -ERANGE;
>> +
>> +	budget = rounddown(budget, RTPSE_MCU_GLOBAL_BUDGET_LSB_MW);
>> +	if (!budget)
>> +		return -ERANGE;
>> +	budget_raw = budget / RTPSE_MCU_GLOBAL_BUDGET_LSB_MW;
>> +
>> +	ret = regulator_request_power_budget(vmain, budget);
>> +	if (ret)
>> +		return ret;
>> +	pse->requested_budget_mW = budget;
>> +
>> +	ret = rtpse_mcu_get_global_budget_idx(pse, &budget_idx);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Retain the programmed MCU limit even if later probe steps fail. */
>> +	return rtpse_mcu_set_global_budget(pse, budget_idx, budget_raw);
> This index is different from an earlier version of this patch, no? 
>
> My (old) documentation [1] describes the retrieved index as:
>     "Last updated PSE device ID. Also the PSE device the budget belongs to."
>
> I'm afraid I don't remember what I meant with the second part, but I suspect the
> power budget needs to be configured on all PSE devices. Does this work
> consistently on switches with multiple PSE chips? If "last updated" means it
> might sometimes return a lower index, I'm worried this might also lead to some
> cases where you query the MCU in the middle of an update cycle.

It works on several devices though I haven't pushed done tests to push those
devices to a specific global limit. I should probably do that soon to have more
of an idea about this behavior.

On Gen2, this doesn't refer to PSE indices but to a bank ID, and the whole system
apparently only uses one bank. It seems the bank is not changeable too, and
vendor firmware I've seen also just sets the power on a single bank.

For Gen1, the assumptions are based on that part "the PSE device the budget
belongs to" and on the userspace reference implementation [2] (grep for
pse_id_set_budget_mask). It varies for different devices there but most seem to
work also with a single index used? Also setting this budget on multiple
downstream managers sounds to me like we would grant each manager this
budget, unless there is some weird inter-manager communication going on.

Users in the OpenWrt forum came up with that issue and ran some own analysis
on vendor firmware. At least on the devices tested there also the vendor
justs sets the budget on a single index [3]. If some specific devices really need
this on multiple indices (like GS1900-24HPv1 in realtek-poe?) then I guess the
switch-specific compatibles need to be used for that purpose and a slightly
adjusted implementation.

> Otherwise this looks okay to me. I'll try to test it over the weekend.
>
> Best,
> Sander
>
> [1] https://svanheule.net/switches/software/broadcom_poe_control_protocol#get_power_statistics

Best,
Jonas

[2] https://github.com/Hurricos/realtek-poe/blob/realtek-poe/src/main.c
[3] https://forum.openwrt.org/t/support-for-zyxel-gs1920-series-gs1920-24hp/155683/170