RE: [PATCH net-next v2 1/5] net: enetc: Add pseudo-MAC support for ENETCv4 Ports via a fixed-link node

[Claudiu Manoil <claudiu.manoil@nxp.com>]

From: Wei Fang <wei.fang@nxp.com>
Sent: Thursday, October 15, 2026 5:17 AM
[...]
> Subject: RE: [PATCH net-next v2 1/5] net: enetc: Add pseudo-MAC support for
> ENETCv4 Ports via a fixed-link node
> 
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h
> > b/drivers/net/ethernet/freescale/enetc/enetc.h
> > index d9e91832a9c1..ed5402f1b73c 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> > @@ -500,6 +500,7 @@ struct enetc_ndev_priv {
> >
> >  	struct clk *ref_clk; /* RGMII/RMII reference clock */
> >  	u64 sysclk_freq; /* NETC system clock frequency */
> > +	struct of_changeset *ppm_ocs; /* pseudo-MAC fixed-link changeset
> */
> 
> struct enetc_ndev_priv is shared by both PF and VF, but ppm_ocs is only
> used by PF. Would it be better to move it to struct enetc_pf?
> 

Ok, will try this out.

> >  };
[...]
> > +static int enetc4_add_ppm_fixed_link(struct enetc_ndev_priv *priv)
> > +{
> > +	struct device_node *np = dev_of_node(priv->dev);
> > +	struct device_node *fixed_link;
> > +	struct of_changeset *ocs;
> > +	int speed, err;
> > +
> > +	if (!np)
> > +		return dev_err_probe(priv->dev, -ENODEV,
> > +				     "Missing of_node for Pseudo-MAC port\n");
> > +
> > +	enetc4_get_pcr_speed(&priv->si->hw, &speed);
> > +	speed = enetc_phylink_match_pseudo_mac_speed(speed);
> > +
> > +	ocs = kzalloc_obj(*ocs);
> > +	if (!ocs)
> > +		return -ENOMEM;
> > +
> > +	of_changeset_init(ocs);
> > +
> > +	fixed_link = of_changeset_create_node(ocs, np, "fixed-link");
> > +	if (!fixed_link) {
> > +		err = -ENOMEM;
> > +		goto err_destroy;
> > +	}
> > +
> > +	err = of_changeset_add_prop_u32(ocs, fixed_link, "speed", speed);
> > +	if (err)
> > +		goto err_put_node;
> > +
> > +	err = of_changeset_add_prop_bool(ocs, fixed_link, "full-duplex");
> > +	if (err)
> > +		goto err_put_node;
> > +
> > +	err = of_changeset_apply(ocs);
> > +	if (err)
> > +		goto err_put_node;
> > +
> > +	of_node_put(fixed_link);
> > +	priv->ppm_ocs = ocs;
> > +
> > +	return 0;
> > +
> > +err_put_node:
> > +	of_node_put(fixed_link);
> > +err_destroy:
> > +	of_changeset_destroy(ocs);
> > +	kfree(ocs);
> > +
> > +	return dev_err_probe(priv->dev, err,
> > +			     "Failed to add fixed-link node\n");
> > +}
> > +
> >  static int enetc4_link_init(struct enetc_ndev_priv *priv,
> >  			    struct device_node *node)
> >  {
> > +	bool dynamic = node && of_node_check_flag(node, OF_DYNAMIC);
> >  	struct enetc_pf *pf = enetc_si_priv(priv->si);
> >  	struct device *dev = priv->dev;
> >  	int err;
> >
> > -	err = of_get_phy_mode(node, &pf->if_mode);
> > -	if (err) {
> > -		dev_err(dev, "Failed to get PHY mode\n");
> > -		return err;
[...]
> > +	if (enetc_is_pseudo_mac(priv->si) && (!node || dynamic)) {
> 
> This indicates that the node pointer can be NULL, but
> enetc4_add_ppm_fixed_link() subsequently does not allow node
> to be NULL. This is puzzling.

enetc4_add_ppm_fixed_link() checks dev_of_node(priv->dev) and
if NULL, it returns error and prints following err message:
"Missing of_node for Pseudo-MAC port\n".
Which is fine for the enetc_is_pseudo_mac() case.

One alternative would be to take the common 'else' branch in case of NULL
node for the pseudo-MAC, and let of_get_phy_mode() return with error, but 
in that case "Failed to get PHY mode\n" is not a good error indication.

So, nothing to change here, the 'if' branch check for the pseudo-MAC case is
correct as it is.

> 
> > +		pf->if_mode = PHY_INTERFACE_MODE_INTERNAL;
> > +
> > +		err = enetc4_add_ppm_fixed_link(priv);
> > +		if (err)
> > +			return err;
> > +
> > +		/* The fixed-link node was spliced onto dev's own of_node, so
> > +		 * phylink will find it via dev_fwnode(dev). Use that node for
> > +		 * the subsequent MDIO/phylink setup below.
> > +		 */
> > +		node = dev_of_node(dev);
> 
> Based on the implementation of enetc4_add_ppm_fixed_link(), the
> input parameter dev->of_node cannot be NULL. I'm not familiar
> with the of_changeset_* APIs. Will enetc4_add_ppm_fixed_link()
> update this pointer?
> 

Sure, all these functions take 'np' as argument, and 'np' is
dev_of_node(priv->dev), which is the same as dev->of_node.

> > +	} else {
> > +		err = of_get_phy_mode(node, &pf->if_mode);
> > +		if (err) {
> > +			dev_err(dev, "Failed to get PHY mode\n");
> > +			return err;
> > +		}
> >  	}
> >
> >  	err = enetc_mdiobus_create(pf, node);
> >  	if (err) {
> >  		dev_err(dev, "Failed to create MDIO bus\n");
> > -		return err;
> > +		goto err_mdiobus_create;
> >  	}
> >
> >  	err = enetc_phylink_create(priv, node, &enetc_pl_mac_ops);