Re: [PATCH net-next v4 2/2] net: macb: add support for 1000BASE-X autonegotiation to PCS

On 8/11/26 21:54, 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.
> ---
> net: macb: add support for 1000BASE-X autonegotiation to PCS
>
> This decouples PCS enablement from SGMII mode selection so the macb PCS
> can also be used in 1000BASE-X mode, advertises 1000BASE-X to phylink,
> and adds an autonegotiation restart implementation for the PCS.
>
> A few questions on the changes below.
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index c15a9c7e69d3..2252e6a55072 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -583,7 +583,12 @@ static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
>>   
>>   static void macb_pcs_an_restart(struct phylink_pcs *pcs)
>>   {
>> -	/* Not supported */
>> +	struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
>> +	u32 old, new;
>> +
>> +	old = gem_readl(bp, PCSCNTRL);
>> +	new = old | BMCR_ANRESTART;
>> +	gem_writel(bp, PCSCNTRL, new);
>>   }
>>   
>>   static int macb_pcs_config(struct phylink_pcs *pcs,
>> @@ -609,8 +614,10 @@ static int macb_pcs_config(struct phylink_pcs *pcs,
>>   		new = old | BMCR_ANENABLE;
>>   	else
>>   		new = old & ~BMCR_ANENABLE;
>> -	if (old != new)
>> +	if (old != new) {
>>   		gem_writel(bp, PCSCNTRL, new);
>> +		return 1;
>> +	}
>>   
>>   	return 0;
>>   }
> [Severity: Medium]
> Is the positive return here attached to the right register write?  The
> kernel-doc for .pcs_config() in include/linux/phylink.h says:
>
>    * When an autonegotiation restart is required for 802.3z modes, .pcs_config()
>    * should return a positive non-zero integer (e.g. 1) to indicate to phylink
>    * to call the pcs_an_restart() method.
>
> and phylink_mii_c22_pcs_config() derives that value from the advertisement
> write, not from the BMCR autonegotiation-enable write:
>
> 	ret = mdiobus_modify_changed(pcs->bus, pcs->addr,
> 				     MII_ADVERTISE, 0xffff, adv);
> 	if (ret < 0)
> 		return ret;
> 	changed = ret;
>
> In macb_pcs_config() the earlier PCSANADV (clause 37 base page) write result
> is discarded:
>
> 	old = gem_readl(bp, PCSANADV);
> 	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising);
> 	if (new != -EINVAL && old != new)
> 		gem_writel(bp, PCSANADV, new);
>
> and 1 is returned only when the BMCR_ANENABLE bit in PCSCNTRL toggles.
>
> Does that mean an advertisement-only change never restarts autonegotiation?
> On an established in-band 1000BASE-X link, ethtool -A or ethtool -s ...
> advertise ... reaches phylink_change_inband_advert(), whose only trigger for
> the restart is the positive return:
>
> 	ret = phylink_pcs_config(pl->pcs, pl->pcs_neg_mode, &pl->link_config,
> 				 !!(pl->link_config.pause & MLO_PAUSE_AN));
> 	if (ret < 0)
> 		return ret;
>
> 	if (ret > 0)
> 		phylink_pcs_an_restart(pl);
>
> Since PCSAUTONEG is on out of reset and phylink keeps it on for in-band mode
> (default_an_inband is set in macb_mii_probe()), PCSCNTRL is unchanged, 0 is
> returned, and the freshly written base page is never put on the wire until a
> link bounce or a larger reconfiguration.  The newly added
> macb_pcs_an_restart() would then not be reached for the advertisement case
> the ops contract describes, while the case that does return 1 (the
> autonegotiation enable/disable toggle) is not the case a restart is asked
> for.
>
> Would recording a changed flag in the PCSANADV branch and returning that,
> as phylink_mii_c22_pcs_config() does, be more in line with the contract?
>
> Also, could the commit message mention the change of pcs_config() return
> semantics?
>
> [ ... ]

This is a good catch and the patch is indeed wrong; apologies. I will 
resend a corrected one, and also make the suggested change to the commit 
message.

>> @@ -957,7 +966,8 @@ static struct phylink_pcs *macb_mac_select_pcs(struct phylink_config *config,
>>   
>>   	if (interface == PHY_INTERFACE_MODE_10GBASER)
>>   		return &bp->phylink_usx_pcs;
>> -	else if (interface == PHY_INTERFACE_MODE_SGMII)
>> +	else if (interface == PHY_INTERFACE_MODE_1000BASEX ||
>> +	         interface == PHY_INTERFACE_MODE_SGMII)
>                   ^^^^^^^^^
>
> [Severity: Low]
> This isn't a bug, but the continuation line in macb_mac_select_pcs() is
> indented with a tab followed by spaces, which checkpatch.pl flags as "code
> indent should use tabs where possible".

Ah, OK, this can be fixed.

>
>>   		return &bp->phylink_sgmii_pcs;
>>   	else
>>   		return NULL;
> [ ... ]
>
>> @@ -1061,9 +1072,12 @@ static int macb_mii_probe(struct net_device *dev)
>>   			  bp->phylink_config.supported_interfaces);
>>   		phy_interface_set_rgmii(bp->phylink_config.supported_interfaces);
>>   
>> -		if (bp->caps & MACB_CAPS_PCS)
>> +		if (bp->caps & MACB_CAPS_PCS) {
>> +			__set_bit(PHY_INTERFACE_MODE_1000BASEX,
>> +				  bp->phylink_config.supported_interfaces);
>>   			__set_bit(PHY_INTERFACE_MODE_SGMII,
>>   				  bp->phylink_config.supported_interfaces);
>> +		}
>>   
>>   		if (bp->caps & MACB_CAPS_HIGH_SPEED) {
>>   			__set_bit(PHY_INTERFACE_MODE_10GBASER,
>> @@ -4932,7 +4946,9 @@ static int macb_init_dflt(struct platform_device *pdev)
>>   	/* Set MII management clock divider */
>>   	val = macb_mdc_clk_div(bp);
>>   	val |= macb_dbw(bp);
>> -	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>> +	if (bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX)
>> +		val |= GEM_BIT(PCSSEL);
>> +	else if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>>   		val |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
>>   	macb_writel(bp, NCFGR, val);
> [Severity: Medium]
> Does the serdes get brought up when phy-mode is "1000base-x"?
> init_reset_optional() still gates the serdes and ZynqMP GEM power-management
> setup on SGMII only:
>
> 	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> 		/* Ensure PHY device used in SGMII mode is ready */
> 		bp->phy = devm_phy_optional_get(&pdev->dev, NULL);
> 		...
> 			ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1);
>
> With phy-mode = "1000base-x" on a part with MACB_CAPS_PCS that uses
> init_reset_optional(), bp->phy stays NULL, so the later
> phy_set_mode_ext(bp->phy, ...) and phy_power_on(bp->phy) calls in
> macb_open() are no-ops, while macb_init_dflt() and macb_mac_config() still
> set GEM_BIT(PCSSEL) and macb_mii_probe() advertises 1000BASE-X.  Would that
> leave the interface up with an unconfigured, unpowered serdes and no
> diagnostic?

The equipment we are using this for (custom electronics for a particle 
physics experiment) operates with a phy-mode of "1000base-x" in its 
default device tree, so I can confirm this works fully correctly in that 
mode.

>
> I could not determine from the tree whether the GT serdes and the
> GEM_CONFIG_SGMII_MODE firmware call are strictly required for 1000BASE-X on
> those parts, so this may be fine on the affected hardware.  Should
> init_reset_optional() cover PHY_INTERFACE_MODE_1000BASEX as well, for
> consistency with the four sites updated here?

This particular block, per an earlier discussion 
(https://lore.kernel.org/netdev/[email protected]/), 
is ZynqMP-specific and onboard-SGMII-PHY specific, so does not apply to 
1000BASE-X mode and I had left it alone here. It certainly could be 
updated for consistency, but that doesn't seem to be needed, either. 
Happy to go either way on this.
-Nathan


-- 
Nathan Whitehorn (he/him)
Associate Professor
Department of Physics and Astronomy
Michigan State University
Biomedical and Physical Sciences 3225
East Lansing, MI 48824
(517) 884-5563