Re: [PATCH net-next v9 6/6] net: stmmac: qcom-ethqos: add support for sa8255p

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Mar 16, 2026 at 01:05:11PM +0100, Bartosz Golaszewski wrote:
> Extend the driver to support a new model - sa8255p. Unlike the
> previously supported variants, this one's power management is done in
> the firmware using SCMI. This is modeled in linux using power domains so
> add support for them.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Shouldn't the SerDes driver be doing the power management rather than
the ethernet driver?

We already have the crappy situation with this driver that the stmmac
clocks are not running when they need to be, which is now causing
warnings with the VLAN code. The platform glue driver itself doesn't
_actually_ have enough information on its own to know when it needs
to ensure that the PCS and SerDes need to be operational which is what
is leading to this problem.

Also note that the core stmmac driver does runtime PM management which
covers both the stmmac MDIO block and the core MAC as well. How does
your implementation interact with those, when e.g. a MDIO bus on one
stmmac instance could be used to access a PHY on a different instance.

> +struct ethqos_emac_pd_ctx {
> +	struct dev_pm_domain_list *pd_list;
> +	int serdes_level;

I don't think serdes_level is appropriate nor correct (see below.)

> +static void ethqos_configure_sgmii_pd(struct qcom_ethqos *ethqos,
> +				      phy_interface_t interface, int speed)
> +{
> +	switch (speed) {
> +	case SPEED_2500:
> +	case SPEED_1000:
> +	case SPEED_100:
> +	case SPEED_10:
> +		ethqos->pd.serdes_level = speed;

This is called at mac_link_up(), after mac_finish() has done its
stuff...

> +	}
> +
> +	ethqos_configure_sgmii(ethqos, interface, speed);
> +}
> +
>  static void ethqos_configure(struct qcom_ethqos *ethqos,
>  			     phy_interface_t interface, int speed)
>  {
> @@ -710,6 +785,45 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
>  	return ret;
>  }
>  
> +static int ethqos_mac_finish_serdes_pd(struct net_device *ndev, void *priv,
> +				       unsigned int mode,
> +				       phy_interface_t interface)
> +{
> +	struct qcom_ethqos *ethqos = priv;
> +	struct device *dev = ethqos->pd.pd_list->pd_devs[ETHQOS_PD_SERDES];
> +	int ret = 0;
> +
> +	qcom_ethqos_set_sgmii_loopback(ethqos, false);
> +
> +	if (interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX)
> +		ret = dev_pm_opp_set_level(dev, ethqos->pd.serdes_level);

... which means this won't get called with anything but a stale speed
from the _previous_ link up event.

> +
> +	return ret;
> +}
> +
> +static int qcom_ethqos_pd_serdes_powerup(struct net_device *ndev, void *priv)
> +{
> +	struct qcom_ethqos *ethqos = priv;
> +	struct device *dev = ethqos->pd.pd_list->pd_devs[ETHQOS_PD_SERDES];
> +	int ret;
> +
> +	ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_SERDES);
> +	if (ret < 0)
> +		return ret;
> +
> +	return dev_pm_opp_set_level(dev, ethqos->pd.serdes_level);

and same here.

The fundamental question arises - why does the power domain need to know
the _media_ speed (which is completely unrelated to the speed at which
the SerDes link operates at) ?

For example, with SGMII, the link operates at 1.25GBaud irrespective of
whether it is operating at an underlying Ethernet data rate of 10M, 100M
or 1G speeds.

To me, the whole "serdes_level" stuff looks completely wrong.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!