From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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>
---
.../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 230 ++++++++++++++++++---
1 file changed, 201 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 446f06b591a0b70992c7a431b56cf88c1b6718fd..d2922b4fb4bad1bb8dd77f1feaa7a260b97f0ae8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -7,6 +7,8 @@
#include <linux/platform_device.h>
#include <linux/phy.h>
#include <linux/phy/phy.h>
+#include <linux/pm_opp.h>
+#include <linux/pm_domain.h>
#include "stmmac.h"
#include "stmmac_platform.h"
@@ -81,6 +83,13 @@
#define SGMII_10M_RX_CLK_DVDR 0x31
+enum ethqos_pd_selector {
+ ETHQOS_PD_CORE = 0,
+ ETHQOS_PD_MDIO,
+ ETHQOS_PD_SERDES,
+ ETHQOS_NUM_PDS,
+};
+
struct ethqos_emac_por {
unsigned int offset;
unsigned int value;
@@ -98,6 +107,9 @@ struct ethqos_emac_driver_data {
struct ethqos_emac_pm_data {
const char *link_clk_name;
+ bool use_domains;
+ struct dev_pm_domain_attach_data pd;
+ unsigned int clk_ptp_rate;
};
struct ethqos_emac_match_data {
@@ -111,13 +123,20 @@ struct ethqos_emac_pm_ctx {
struct phy *serdes_phy;
};
+struct ethqos_emac_pd_ctx {
+ struct dev_pm_domain_list *pd_list;
+};
+
struct qcom_ethqos {
struct platform_device *pdev;
void __iomem *rgmii_base;
void __iomem *mac_base;
int (*configure_func)(struct qcom_ethqos *ethqos, int speed);
- struct ethqos_emac_pm_ctx pm;
+ union {
+ struct ethqos_emac_pm_ctx pm;
+ struct ethqos_emac_pd_ctx pd;
+ };
phy_interface_t phy_mode;
int serdes_speed;
int (*set_serdes_speed)(struct qcom_ethqos *ethqos);
@@ -330,6 +349,25 @@ static const struct ethqos_emac_match_data emac_sa8775p_data = {
.pm_data = &emac_sa8775p_pm_data,
};
+static const char * const emac_sa8255p_pd_names[] = {
+ "core", "mdio", "serdes"
+};
+
+static const struct ethqos_emac_pm_data emac_sa8255p_pm_data = {
+ .pd = {
+ .pd_flags = PD_FLAG_NO_DEV_LINK,
+ .pd_names = emac_sa8255p_pd_names,
+ .num_pd_names = ETHQOS_NUM_PDS,
+ },
+ .use_domains = true,
+ .clk_ptp_rate = 230400000,
+};
+
+static const struct ethqos_emac_match_data emac_sa8255p_data = {
+ .drv_data = &emac_v4_0_0_data,
+ .pm_data = &emac_sa8255p_pm_data,
+};
+
static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
{
struct device *dev = ðqos->pdev->dev;
@@ -411,6 +449,28 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
return 0;
}
+static int qcom_ethqos_domain_on(struct qcom_ethqos *ethqos,
+ enum ethqos_pd_selector sel)
+{
+ struct device *dev = ethqos->pd.pd_list->pd_devs[sel];
+ int ret;
+
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0)
+ dev_err(ðqos->pdev->dev,
+ "Failed to enable the power domain for %s\n",
+ dev_name(dev));
+ return ret;
+}
+
+static void qcom_ethqos_domain_off(struct qcom_ethqos *ethqos,
+ enum ethqos_pd_selector sel)
+{
+ struct device *dev = ethqos->pd.pd_list->pd_devs[sel];
+
+ pm_runtime_put_sync(dev);
+}
+
static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
{
struct device *dev = ðqos->pdev->dev;
@@ -646,6 +706,13 @@ static int ethqos_set_serdes_speed_phy(struct qcom_ethqos *ethqos)
return phy_set_speed(ethqos->pm.serdes_phy, ethqos->serdes_speed);
}
+static int ethqos_set_serdes_speed_pd(struct qcom_ethqos *ethqos)
+{
+ struct device *dev = ethqos->pd.pd_list->pd_devs[ETHQOS_PD_SERDES];
+
+ return dev_pm_opp_set_level(dev, ethqos->serdes_speed);
+}
+
static void ethqos_set_serdes_speed(struct qcom_ethqos *ethqos, int speed)
{
if (ethqos->serdes_speed != speed) {
@@ -737,6 +804,27 @@ static void qcom_ethqos_serdes_powerdown(struct net_device *ndev, void *priv)
phy_exit(ethqos->pm.serdes_phy);
}
+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->serdes_speed);
+}
+
+static void qcom_ethqos_pd_serdes_powerdown(struct net_device *ndev, void *priv)
+{
+ struct qcom_ethqos *ethqos = priv;
+
+ /* TODO set level */
+ qcom_ethqos_domain_off(ethqos, ETHQOS_PD_SERDES);
+}
+
static int ethqos_clks_config(void *priv, bool enabled)
{
struct qcom_ethqos *ethqos = priv;
@@ -769,6 +857,61 @@ static void ethqos_clks_disable(void *data)
ethqos_clks_config(ethqos, false);
}
+static int ethqos_pd_clks_config(void *priv, bool enabled)
+{
+ struct qcom_ethqos *ethqos = priv;
+ int ret = 0;
+
+ if (enabled) {
+ ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_MDIO);
+ if (ret < 0) {
+ dev_err(ðqos->pdev->dev,
+ "Failed to enable the MDIO power domain\n");
+ return ret;
+ }
+
+ ethqos_set_func_clk_en(ethqos);
+ } else {
+ qcom_ethqos_domain_off(ethqos, ETHQOS_PD_MDIO);
+ }
+
+ return ret;
+}
+
+static int qcom_ethqos_pd_init(struct platform_device *pdev, void *priv)
+{
+ struct qcom_ethqos *ethqos = priv;
+ int ret;
+
+ /*
+ * Enable functional clock to prevent DMA reset after timeout due
+ * to no PHY clock being enabled after the hardware block has been
+ * power cycled. The actual configuration will be adjusted once
+ * ethqos_fix_mac_speed() is called.
+ */
+ ethqos_set_func_clk_en(ethqos);
+
+ ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_CORE);
+ if (ret)
+ return ret;
+
+ ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_MDIO);
+ if (ret) {
+ qcom_ethqos_domain_off(ethqos, ETHQOS_PD_CORE);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void qcom_ethqos_pd_exit(struct platform_device *pdev, void *data)
+{
+ struct qcom_ethqos *ethqos = data;
+
+ qcom_ethqos_domain_off(ethqos, ETHQOS_PD_MDIO);
+ qcom_ethqos_domain_off(ethqos, ETHQOS_PD_CORE);
+}
+
static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
{
struct plat_stmmacenet_data *plat_dat = priv->plat;
@@ -809,8 +952,6 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
"dt configuration failed\n");
}
- plat_dat->clks_config = ethqos_clks_config;
-
ethqos = devm_kzalloc(dev, sizeof(*ethqos), GFP_KERNEL);
if (!ethqos)
return -ENOMEM;
@@ -852,28 +993,63 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
ethqos->rgmii_config_loopback_en = drv_data->rgmii_config_loopback_en;
ethqos->has_emac_ge_3 = drv_data->has_emac_ge_3;
ethqos->needs_sgmii_loopback = drv_data->needs_sgmii_loopback;
-
- ethqos->pm.link_clk = devm_clk_get(dev, clk_name);
- if (IS_ERR(ethqos->pm.link_clk))
- return dev_err_probe(dev, PTR_ERR(ethqos->pm.link_clk),
- "Failed to get link_clk\n");
-
- ret = ethqos_clks_config(ethqos, true);
- if (ret)
- return ret;
-
- ret = devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos);
- if (ret)
- return ret;
-
- ethqos->pm.serdes_phy = devm_phy_optional_get(dev, "serdes");
- if (IS_ERR(ethqos->pm.serdes_phy))
- return dev_err_probe(dev, PTR_ERR(ethqos->pm.serdes_phy),
- "Failed to get serdes phy\n");
-
- ethqos->set_serdes_speed = ethqos_set_serdes_speed_phy;
ethqos->serdes_speed = SPEED_1000;
- ethqos_update_link_clk(ethqos, SPEED_1000);
+
+ if (pm_data && pm_data->use_domains) {
+ ethqos->set_serdes_speed = ethqos_set_serdes_speed_pd;
+
+ ret = devm_pm_domain_attach_list(dev, &pm_data->pd,
+ ðqos->pd.pd_list);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to attach power domains\n");
+
+ plat_dat->clks_config = ethqos_pd_clks_config;
+ plat_dat->serdes_powerup = qcom_ethqos_pd_serdes_powerup;
+ plat_dat->serdes_powerdown = qcom_ethqos_pd_serdes_powerdown;
+ plat_dat->exit = qcom_ethqos_pd_exit;
+ plat_dat->init = qcom_ethqos_pd_init;
+ plat_dat->clk_ptp_rate = pm_data->clk_ptp_rate;
+
+ ret = qcom_ethqos_pd_init(pdev, ethqos);
+ if (ret)
+ return ret;
+
+ ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_SERDES);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to enable the serdes power domain\n");
+ } else {
+ ethqos->set_serdes_speed = ethqos_set_serdes_speed_phy;
+
+ ethqos->pm.link_clk = devm_clk_get(dev, clk_name);
+ if (IS_ERR(ethqos->pm.link_clk))
+ return dev_err_probe(dev, PTR_ERR(ethqos->pm.link_clk),
+ "Failed to get link_clk\n");
+
+ ret = ethqos_clks_config(ethqos, true);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos);
+ if (ret)
+ return ret;
+
+ ethqos->pm.serdes_phy = devm_phy_optional_get(dev, "serdes");
+ if (IS_ERR(ethqos->pm.serdes_phy))
+ return dev_err_probe(dev, PTR_ERR(ethqos->pm.serdes_phy),
+ "Failed to get serdes phy\n");
+
+ ethqos_update_link_clk(ethqos, SPEED_1000);
+
+ plat_dat->clks_config = ethqos_clks_config;
+ plat_dat->ptp_clk_freq_config = ethqos_ptp_clk_freq_config;
+
+ if (ethqos->pm.serdes_phy) {
+ plat_dat->serdes_powerup = qcom_ethqos_serdes_powerup;
+ plat_dat->serdes_powerdown = qcom_ethqos_serdes_powerdown;
+ }
+ }
+
ethqos_set_func_clk_en(ethqos);
plat_dat->bsp_priv = ethqos;
@@ -891,11 +1067,6 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
if (drv_data->dma_addr_width)
plat_dat->host_dma_width = drv_data->dma_addr_width;
- if (ethqos->pm.serdes_phy) {
- plat_dat->serdes_powerup = qcom_ethqos_serdes_powerup;
- plat_dat->serdes_powerdown = qcom_ethqos_serdes_powerdown;
- }
-
/* Enable TSO on queue0 and enable TBS on rest of the queues */
for (i = 1; i < plat_dat->tx_queues_to_use; i++)
plat_dat->tx_queues_cfg[i].tbs_en = 1;
@@ -905,6 +1076,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
static const struct of_device_id qcom_ethqos_match[] = {
{ .compatible = "qcom,qcs404-ethqos", .data = &emac_qcs404_data},
+ { .compatible = "qcom,sa8255p-ethqos", .data = &emac_sa8255p_data},
{ .compatible = "qcom,sa8775p-ethqos", .data = &emac_sa8775p_data},
{ .compatible = "qcom,sc8280xp-ethqos", .data = &emac_sc8280xp_data},
{ .compatible = "qcom,sm8150-ethqos", .data = &emac_sm8150_data},
--
2.51.0
On Fri, Nov 07, 2025 at 11:29:58AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
...
> +static int qcom_ethqos_pd_init(struct platform_device *pdev, void *priv)
> +{
> + struct qcom_ethqos *ethqos = priv;
> + int ret;
> +
> + /*
> + * Enable functional clock to prevent DMA reset after timeout due
> + * to no PHY clock being enabled after the hardware block has been
> + * power cycled. The actual configuration will be adjusted once
> + * ethqos_fix_mac_speed() is called.
> + */
> + ethqos_set_func_clk_en(ethqos);
> +
> + ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_CORE);
> + if (ret)
> + return ret;
> +
> + ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_MDIO);
> + if (ret) {
> + qcom_ethqos_domain_off(ethqos, ETHQOS_PD_CORE);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void qcom_ethqos_pd_exit(struct platform_device *pdev, void *data)
> +{
> + struct qcom_ethqos *ethqos = data;
> +
> + qcom_ethqos_domain_off(ethqos, ETHQOS_PD_MDIO);
> + qcom_ethqos_domain_off(ethqos, ETHQOS_PD_CORE);
> +}
> +
> static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
> {
> struct plat_stmmacenet_data *plat_dat = priv->plat;
...
> @@ -852,28 +993,63 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
> ethqos->rgmii_config_loopback_en = drv_data->rgmii_config_loopback_en;
> ethqos->has_emac_ge_3 = drv_data->has_emac_ge_3;
> ethqos->needs_sgmii_loopback = drv_data->needs_sgmii_loopback;
> -
> - ethqos->pm.link_clk = devm_clk_get(dev, clk_name);
> - if (IS_ERR(ethqos->pm.link_clk))
> - return dev_err_probe(dev, PTR_ERR(ethqos->pm.link_clk),
> - "Failed to get link_clk\n");
> -
> - ret = ethqos_clks_config(ethqos, true);
> - if (ret)
> - return ret;
> -
> - ret = devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos);
> - if (ret)
> - return ret;
> -
> - ethqos->pm.serdes_phy = devm_phy_optional_get(dev, "serdes");
> - if (IS_ERR(ethqos->pm.serdes_phy))
> - return dev_err_probe(dev, PTR_ERR(ethqos->pm.serdes_phy),
> - "Failed to get serdes phy\n");
> -
> - ethqos->set_serdes_speed = ethqos_set_serdes_speed_phy;
> ethqos->serdes_speed = SPEED_1000;
> - ethqos_update_link_clk(ethqos, SPEED_1000);
> +
> + if (pm_data && pm_data->use_domains) {
> + ethqos->set_serdes_speed = ethqos_set_serdes_speed_pd;
> +
> + ret = devm_pm_domain_attach_list(dev, &pm_data->pd,
> + ðqos->pd.pd_list);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to attach power domains\n");
> +
> + plat_dat->clks_config = ethqos_pd_clks_config;
> + plat_dat->serdes_powerup = qcom_ethqos_pd_serdes_powerup;
> + plat_dat->serdes_powerdown = qcom_ethqos_pd_serdes_powerdown;
> + plat_dat->exit = qcom_ethqos_pd_exit;
Hi Bartosz,
It seems that the intention of this is to ensure
that domains turned on by qcom_ethqos_pd_init()
are turned off again on exit or clean-up in error paths.
> + plat_dat->init = qcom_ethqos_pd_init;
> + plat_dat->clk_ptp_rate = pm_data->clk_ptp_rate;
> +
> + ret = qcom_ethqos_pd_init(pdev, ethqos);
> + if (ret)
> + return ret;
And here those domains are turned on.
> +
> + ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_SERDES);
> + if (ret)
But it seems that if we reach this error path then the cleanup is not
performed. This is because plat_dat and thus it's exit callback are
registered until the call to devm_stmmac_pltfr_probe() towards the end of
this function.
Sorry if I'm on the wrong track here. I did dig into it.
But this was flagged by Claude Code running
https://github.com/masoncl/review-prompts/
> + return dev_err_probe(dev, ret,
> + "Failed to enable the serdes power domain\n");
...
On Tue, Nov 11, 2025 at 12:48 PM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Nov 07, 2025 at 11:29:58AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> ...
>
> > +static int qcom_ethqos_pd_init(struct platform_device *pdev, void *priv)
> > +{
> > + struct qcom_ethqos *ethqos = priv;
> > + int ret;
> > +
> > + /*
> > + * Enable functional clock to prevent DMA reset after timeout due
> > + * to no PHY clock being enabled after the hardware block has been
> > + * power cycled. The actual configuration will be adjusted once
> > + * ethqos_fix_mac_speed() is called.
> > + */
> > + ethqos_set_func_clk_en(ethqos);
> > +
> > + ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_CORE);
> > + if (ret)
> > + return ret;
> > +
> > + ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_MDIO);
> > + if (ret) {
> > + qcom_ethqos_domain_off(ethqos, ETHQOS_PD_CORE);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void qcom_ethqos_pd_exit(struct platform_device *pdev, void *data)
> > +{
> > + struct qcom_ethqos *ethqos = data;
> > +
> > + qcom_ethqos_domain_off(ethqos, ETHQOS_PD_MDIO);
> > + qcom_ethqos_domain_off(ethqos, ETHQOS_PD_CORE);
> > +}
> > +
> > static void ethqos_ptp_clk_freq_config(struct stmmac_priv *priv)
> > {
> > struct plat_stmmacenet_data *plat_dat = priv->plat;
>
> ...
>
> > @@ -852,28 +993,63 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
> > ethqos->rgmii_config_loopback_en = drv_data->rgmii_config_loopback_en;
> > ethqos->has_emac_ge_3 = drv_data->has_emac_ge_3;
> > ethqos->needs_sgmii_loopback = drv_data->needs_sgmii_loopback;
> > -
> > - ethqos->pm.link_clk = devm_clk_get(dev, clk_name);
> > - if (IS_ERR(ethqos->pm.link_clk))
> > - return dev_err_probe(dev, PTR_ERR(ethqos->pm.link_clk),
> > - "Failed to get link_clk\n");
> > -
> > - ret = ethqos_clks_config(ethqos, true);
> > - if (ret)
> > - return ret;
> > -
> > - ret = devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos);
> > - if (ret)
> > - return ret;
> > -
> > - ethqos->pm.serdes_phy = devm_phy_optional_get(dev, "serdes");
> > - if (IS_ERR(ethqos->pm.serdes_phy))
> > - return dev_err_probe(dev, PTR_ERR(ethqos->pm.serdes_phy),
> > - "Failed to get serdes phy\n");
> > -
> > - ethqos->set_serdes_speed = ethqos_set_serdes_speed_phy;
> > ethqos->serdes_speed = SPEED_1000;
> > - ethqos_update_link_clk(ethqos, SPEED_1000);
> > +
> > + if (pm_data && pm_data->use_domains) {
> > + ethqos->set_serdes_speed = ethqos_set_serdes_speed_pd;
> > +
> > + ret = devm_pm_domain_attach_list(dev, &pm_data->pd,
> > + ðqos->pd.pd_list);
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret, "Failed to attach power domains\n");
> > +
> > + plat_dat->clks_config = ethqos_pd_clks_config;
> > + plat_dat->serdes_powerup = qcom_ethqos_pd_serdes_powerup;
> > + plat_dat->serdes_powerdown = qcom_ethqos_pd_serdes_powerdown;
> > + plat_dat->exit = qcom_ethqos_pd_exit;
>
> Hi Bartosz,
>
> It seems that the intention of this is to ensure
> that domains turned on by qcom_ethqos_pd_init()
> are turned off again on exit or clean-up in error paths.
>
> > + plat_dat->init = qcom_ethqos_pd_init;
> > + plat_dat->clk_ptp_rate = pm_data->clk_ptp_rate;
> > +
> > + ret = qcom_ethqos_pd_init(pdev, ethqos);
> > + if (ret)
> > + return ret;
>
> And here those domains are turned on.
>
> > +
> > + ret = qcom_ethqos_domain_on(ethqos, ETHQOS_PD_SERDES);
> > + if (ret)
>
> But it seems that if we reach this error path then the cleanup is not
> performed. This is because plat_dat and thus it's exit callback are
> registered until the call to devm_stmmac_pltfr_probe() towards the end of
> this function.
We can only reach this if devm_stmmac_pltfr_probe() fails. Yeah it
probably warrants a devres action.
Bartosz
On 11/7/25 11:29 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> 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>
> ---
[...]
> +static void qcom_ethqos_pd_serdes_powerdown(struct net_device *ndev, void *priv)
> +{
> + struct qcom_ethqos *ethqos = priv;
> +
> + /* TODO set level */
> + qcom_ethqos_domain_off(ethqos, ETHQOS_PD_SERDES);
dev_pm_opp_set_level(dev, 0);?
perhaps with _index?
Konrad
© 2016 - 2025 Red Hat, Inc.