Re: [PATCH next-next] net: dsa: qca8k: Add support for force mode for fixed link topology

[George Moussalem]

On 6/5/26 05:24, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
...
> 
> Earlier in qca8k_pcs_config(), reg is overridden for these models:
> 
> 	if (priv->switch_id == QCA8K_ID_QCA8327 ||
> 	    priv->switch_id == QCA8K_ID_QCA8337)
> 		reg = QCA8K_REG_PORT0_PAD_CTRL;
> 
> Since the SGMII force mode configuration uses this same reg variable,
> won't this cause the FORCE_MODE bit for port 6 to be incorrectly written
> to port 0's PAD control register on these specific switches?

I've taken another look at the vendor SDK and this property is always
set on PORT0_PAD_CTRL regardless of whether it's physically wired on
port 0 or 6. I did see it is only set on the QCA8337 version, so I'll
check for that in v2.

> 
>>  	if (val)
>>  		ret = qca8k_rmw(priv, reg,
>>  				QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE |
>> -				QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE,
>> +				QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE |
>> +				QCA8K_PORT_PAD_SGMII_FORCE_MODE,
>>  				val);
> 
> [Severity: High]
> Is FORCE_MODE properly cleared if it was previously set by a bootloader,
> but we don't use outband negotiation and no clock falling edge properties
> are configured?
> 
> If neg_mode != PHYLINK_PCS_NEG_OUTBAND and the clock phase bits are not set,
> val will be 0. This causes the if (val) check to fail, skipping the
> qca8k_rmw() call entirely, leaving FORCE_MODE unchanged in hardware.
> 
> However, if clock phases are configured in the device tree, val will be
> non-zero, and qca8k_rmw() will clear FORCE_MODE. Does this create a
> scenario where clearing this mode unpredictably depends on unrelated clock
> configurations?

No, the default value, as for all documented bits on PORT0_PAD_CTRL, is
0 (disabled) after a reset. So it will be disabled by default and only
enabled if a fixed link is defined in the port node in the DT.

> 
>>  
>>  	return 0;

Best regards,
George