Re: [PATCH net-next v2 1/2] net: pcs: xpcs: Add hooks for xpcs configuration of rsfec

[Alexander H Duyck <alexander.duyck@gmail.com>]

On Mon, 2026-05-11 at 14:26 -0400, mike.marciniszyn@gmail.com wrote:
> From: "Mike Marciniszyn (Meta)" <mike.marciniszyn@gmail.com>
> 
> The DW PCS IP data sheet calls out the need to populate
> these vendor registers when operating at speeds above 10Gbps.
> This change enables the correct FEC settings to enable RS-FEC
> encoding on the link which is the standard used for most links
> at these higher speeds.
> 
> Note that the overwriting of the MDIO_PMA_RSFEC_CTRL register
> is intentional and consistent DW PCS IP requirements.
> 
> Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
> Signed-off-by: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>
> ---
> v2:
>   - Allow mdiobus probing for addr 0 and addr 1
>   - Add xpcs_find* helpers based on phy_find* to avoid hardcoded addresses
>   - Remove xpcs_bus write and use mdiodev_c45_write instead
>   - Address comment on use of MDIO_PMA_RSFEC_CTRL
> v1: https://lore.kernel.org/all/20260506190904.4029-2-mike.marciniszyn@gmail.com/
> 
>  drivers/net/ethernet/meta/fbnic/fbnmdiodev_c45_writeic_mdio.c |   3 +-
>  drivers/net/pcs/pcs-xpcs.c                   | 105 +++++++++++++++++++
>  drivers/net/pcs/pcs-xpcs.h                   |   6 ++
>  include/uapi/linux/mdio.h                    |   3 +
>  4 files changed, 115 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c b/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c
> index fe3a4ce88413..e29534dd1e0c 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_mdio.c
> @@ -263,8 +263,7 @@ int fbnic_mdiobus_create(struct fbnic_dev *fbd)
>  	bus->read_c45 = &fbnic_mdio_read_c45;
>  	bus->write_c45 = &fbnic_mdio_write_c45;
>  
> -	/* Disable PHY auto probing. We will add PCS manually */
> -	bus->phy_mask = ~0;
> +	bus->phy_mask = GENMASK(31, 2);
>  
>  	bus->parent = fbd->dev;
>  	bus->priv = fbd;

Why are you messing with the PHY autoprobing mask? That doesn't make
sense to add to this patch. The side effect is that it is going to
expose a phydev that won't be used as we access the PMD direcly.

[...]

> +static struct mdio_device *xpcs_find_first_mdev(struct dw_xpcs *xpcs)
> +{
> +	struct phy_device *p = phy_find_first(xpcs->mdiodev->bus);
> +
> +	if (!p)
> +		return NULL;
> +	return &p->mdio;
> +}
> +
> +static struct mdio_device *
> +xpcs_find_next_mdev(struct dw_xpcs *xpcs, struct mdio_device *mdev)
> +{
> +	struct phy_device *p = container_of(mdev, struct phy_device, mdio), *n;
> +
> +	n = phy_find_next(xpcs->mdiodev->bus, p);
> +	if (!n)
> +		return NULL;
> +	return &n->mdio;
> +}
> +

Okay, the use of these functions explains the issue. You shouldn't be
using phy_find_next. You know the bus will be located on mdio.addr + 1.
You should not need to have a phy_device assigned for each of the PMA
devices.

The general idea is you have mdio_devices you shouldn't be trying to
use the phy_device functions in the PCS driver.

> +static int
> +xpcs_config_rsfec_pma(struct dw_xpcs *xpcs, const struct pma_pcs_values *v)
> +{
> +	struct mdio_device *mdev;
> +	int ret = 0, i, pma_mmd;
> +
> +	pma_mmd = xpcs_get_pma_mmd(xpcs);
> +	if (pma_mmd < 1)
> +		return pma_mmd;
> +
> +	mdev = xpcs_find_first_mdev(xpcs);
> +	if (!mdev)
> +		return -ENODEV;
> +

There shouldn't be any need for a special for this. You already have
the capabilities to write to the devices you need to write to.

The only things you need to change would be to access the nearest PMA
to the PCS as that is the one that holds the RSFEC configuration. For
that you just need to know what devices are already enabled and you can
access them.

> +	for (i = 0; mdev && ret >= 0 && i < v->lanes;
> +	     i++, mdev = xpcs_find_next_mdev(xpcs, mdev)) {
> +		ret = mdiodev_c45_write(mdev, pma_mmd, MDIO_PMA_RSFEC_CTRL,
> +					v->rsfec_ctrl);
> +	}

You don't need this "find next" logic. It is just the device on the
adjacent bus. Also I am not sure we even need the loop logic since
there are only 2 devices you have to configure as currently only 2
lanes are supported.

[...]

> diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
> index b2541c948fc1..5219c877b2cf 100644
> --- a/include/uapi/linux/mdio.h
> +++ b/include/uapi/linux/mdio.h
> @@ -317,6 +317,9 @@
>  #define MDIO_PMA_10GBR_FECABLE_ABLE	0x0001	/* FEC ability */
>  #define MDIO_PMA_10GBR_FECABLE_ERRABLE	0x0002	/* FEC error indic. ability */
>  
> +/* RSFEC PMA Control register */
> +#define MDIO_PMA_RSFEC_CTRL_4LANE_PMD	BIT(3)
> +
>  /* PMA 10GBASE-R Fast Retrain status and control register. */
>  #define MDIO_PMA_10GBR_FSRT_ENABLE	0x0001	/* Fast retrain enable */
>  

The AI review complaint about this bit seems to be based on an outdated
version of the IEEE specification.

This is called out in 9802.3-2022 as "Four lane PMD indication" so
perhaps sashiko needs to be updated.