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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: qca8k: Add support for force mode for fixed link topology
    
A fixed link topology is commonly used to connect this switch (on port
0 or 6) to a SoC's MAC over SGMII. When inband negotiation is not used,
the switch needs to be configured to operate in force mode. As such,
enable support for force mode.

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index a36b8b07030e3..3dbd18ab86380 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
[ ... ]
> @@ -1611,10 +1611,14 @@ static int qca8k_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>  	if (priv->ports_config.sgmii_tx_clk_falling_edge)
>  		val |= QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE;
>  
> +	if (neg_mode == PHYLINK_PCS_NEG_OUTBAND)
> +		val |= QCA8K_PORT_PAD_SGMII_FORCE_MODE;
> +

[Severity: High]
Could there be an issue with configuring port 6 on QCA8327 and QCA8337
hardware?

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?

>  	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?

>  
>  	return 0;
-- 
pw-bot: cr