[net-next,v15,9/9] net: stmmac: qcom-ethqos: add support for sa8255p
Commit Message
Extend the driver to support a new model - sa8255p. Unlike the previously
supported variants, this one's power management is done in the firmware
over SCMI. This is modeled in linux using power domains so add a new
emac data variant and a separate setup callback.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
.../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 107 +++++++++++++++++++++
1 file changed, 107 insertions(+)
Comments
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · Low: 2
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] With Wake-on-LAN armed, system suspend on sa8255p powers off
the MAC's SCMI "core"/"mdio" domains, and on resume the EMAC wrapper…
- [Medium] The SCMI variant has no CSR/application clock, and nothing
substitutes a rate for it.
- [Low] The new sa8255p (SCMI) variant accepts RGMII phy-modes but
silently misconfigures them.
- [Low] The new comment in ethqos_scmi_setup() and the commit message
misdescribe what the code does.
--
net: stmmac: qcom-ethqos: add support for sa8255p
This adds a new sa8255p variant to the qcom-ethqos glue driver. On this
variant, firmware handles power management over SCMI, and Linux models it
with power domains. It adds a new emac data variant with a fixed PTP clock
rate, plus a separate ethqos_scmi_setup() callback.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 509a5368e69a8..3d4bc950a72ac 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
[ ... ]
> @@ -756,6 +788,80 @@ static int ethqos_hlos_setup(struct qcom_ethqos *ethqos,
> return 0;
> }
>
> +static const char *const ethqos_scmi_pd_names[] = { "core", "mdio" };
> +
> +static int ethqos_scmi_setup(struct qcom_ethqos *ethqos,
> + struct plat_stmmacenet_data *plat_dat)
> +{
> + const struct dev_pm_domain_attach_data pd_data = {
> + .pd_names = ethqos_scmi_pd_names,
> + .num_pd_names = ARRAY_SIZE(ethqos_scmi_pd_names),
> + .pd_flags = PD_FLAG_DEV_LINK_ON,
> + };
> +
> + struct platform_device *pdev = ethqos->pdev;
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + ret = devm_pm_domain_attach_list(dev, &pd_data, ðqos->pds);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "Failed to attach power domains\n");
[Severity: Medium]
What happens to these two domains during system suspend when Wake-on-LAN
is armed?
devm_pm_domain_attach_list() creates virtual genpd devices through
genpd_dev_pm_attach_by_id(). Those devices have no parent:
drivers/pmdomain/core.c:genpd_dev_pm_attach_by_id() {
...
dev_set_name(virt_dev, "genpd:%u:%s", index, dev_name(dev));
virt_dev->bus = &genpd_bus_type;
virt_dev->release = genpd_release_dev;
virt_dev->of_node = of_node_get(dev->of_node);
...
}
dpm_propagate_wakeup_to_parent() only passes wakeup_path to dev->parent.
It does not reach device-link suppliers, so device_awake_path(virt_dev)
stays false.
SCMI domains set GENPD_FLAG_ACTIVE_WAKEUP. Even so, genpd_finish_suspend()
falls through to genpd_sync_power_off():
if (device_awake_path(dev) && genpd_is_active_wakeup(genpd) &&
!device_out_band_wakeup(dev))
return 0;
qcom_ethqos_probe() also sets plat_dat->pmt = true for this variant, so
WoL can be enabled with ethtool. With priv->wolopts set,
stmmac_pltfr_noirq_suspend() and stmmac_pltfr_noirq_resume() skip
pm_runtime_force_suspend() and pm_runtime_force_resume():
if (!priv->wolopts) {
/* enable the clk previously disabled */
ret = pm_runtime_force_resume(dev);
As a result, ethqos_clks_config(true) does not run on resume.
If firmware really power-cycles the MAC and wrapper when the "core" and
"mdio" domains are turned off, can WoL fail to wake the system? Would the
SGMII loopback and FUNC_CLK_EN bits at rgmii_base also be lost, so that
stmmac_resume()->stmmac_hw_setup() hits the DMA SW-reset timeout that the
comment below describes?
On the HLOS variants, the single GDSC is the MAC's own pm_domain, so the
awake-path check applies to the MAC device itself. This looks specific to
the multi-domain attach used here. The kernel tree can't show whether
firmware actually removes power on that request.
> +
> + /*
> + * The SerDes lane, its clocks and the MAC AXI/AHB clocks are owned by
> + * firmware and brought up through the SCMI power domains above. The
> + * MAC wrapper itself, however is in the kernel's register space: the
> + * wrapper bit that loops the PHY TX clock into the MAC's clk_rx_i -
> + * needed because no recovered RX clock exists yet - is not
> + * configured by firmware. Without it, clk_rx_i never toggles and the
> + * DMA SW-reset polled in dwmac4_dma_reset() never completes.
[Severity: Low]
Is this comment right about where the SerDes power comes from?
The sa8255p SerDes PHY binding (qcom,sa8255p-dwmac-sgmii-phy.yaml)
requires a power-domain of its own. phy-qcom-sgmii-eth-scmi.c votes for
it through the PHY device's runtime PM (devm_pm_runtime_enable() before
devm_phy_create()), and set_mode() sets its performance state.
qcom_ethqos_probe() still gets the "serdes" phy and installs
qcom_ethqos_serdes_powerup(). The PHY TX clock that is looped into
clk_rx_i therefore comes from serdes_powerup() in stmmac_open(), before
__stmmac_open()->stmmac_hw_setup(). It does not come from the "core" and
"mdio" domains attached above.
The current ordering works. Could the comment name the SerDes PHY's own
domain instead, so later changes to the SerDes or PM ordering are not
misled?
The commit message also says only:
Unlike the previously supported variants, this one's power management
is done in the firmware over SCMI. This is modeled in linux using power
domains so add a new emac data variant and a separate setup callback.
Could it also mention the following?
- The kernel still maps and programs the "rgmii" wrapper registers
(SGMII loopback and FUNC_CLK_EN) at probe and on every runtime resume.
- set_clk_tx_rate and dump_debug_regs are dropped for this variant.
- A fixed 230.4 MHz PTP reference rate is hardcoded in
emac_v4_0_0_scmi_data.
> + *
> + * Map the wrapper and program the same loopback/functional clock bits
> + * the non-firmware platforms rely on (see ethqos_clks_config) so the
> + * RX clock is present by the time the DMA engine is reset.
> + */
[ ... ]
> + ret = devm_add_action_or_reset(dev, ethqos_clks_disable, ethqos);
> + if (ret)
> + return ret;
> +
> + plat_dat->clk_ptp_rate = ethqos->data->ptp_clk_rate;
[Severity: Medium]
The PTP rate gets a fixed value here, but what about the CSR clock?
The sa8255p binding has no clocks property. stmmac_probe_config_dt()
therefore fails devm_clk_get("stmmaceth") and leaves plat->stmmac_clk
NULL:
plat->stmmac_clk = devm_clk_get(&pdev->dev,
STMMAC_RESOURCE_NAME);
if (IS_ERR(plat->stmmac_clk)) {
dev_warn(&pdev->dev, "Cannot get CSR clock\n");
plat->stmmac_clk = NULL;
dwmac4_core_init() then runs this on every stmmac_hw_setup():
clk_rate = clk_get_rate(priv->plat->stmmac_clk);
writel((clk_rate / 1000000) - 1, ioaddr + GMAC4_MAC_ONEUS_TIC_COUNTER);
With clk_rate == 0, won't this write 0xFFFFFFFF into
GMAC4_MAC_ONEUS_TIC_COUNTER? That would break the LPI/EEE timers, which
count in microseconds.
plat_dat->clk_ref_rate is not set either, so stmmac_usec2riwt() and
stmmac_riwt2usec() return 0:
unsigned long clk = clk_get_rate(priv->plat->stmmac_clk);
if (!clk) {
clk = priv->plat->clk_ref_rate;
if (!clk)
return 0;
Would ethtool -C rx-usecs then fail the MIN_DMA_RIWT check, and would
rx-usecs be reported as 0?
> +
> + switch (ethqos->phy_mode) {
> + case PHY_INTERFACE_MODE_2500BASEX:
> + case PHY_INTERFACE_MODE_SGMII:
> + plat_dat->fix_mac_speed = ethqos_fix_mac_speed_sgmii;
> + break;
> + default:
> + break;
> + }
[Severity: Low]
What happens here if the device tree sets an RGMII phy-mode on a
qcom,sa8255p-ethqos node?
qcom_ethqos_probe() accepts PHY_INTERFACE_MODE_RGMII, _ID, _RXID and _TXID
for every compatible, and the sa8255p binding does not constrain
phy-mode. For RGMII, this switch takes the default branch, so none of the
following happens:
- no fix_mac_speed is installed
- no set_clk_tx_rate is installed
- no RGMII IO macro or DLL programming is done (emac_v4_0_0_scmi_data
has no rgmii_por)
qcom_ethqos_set_sgmii_loopback() also returns early for non-SGMII modes.
The SCMI SerDes provider rejects RGMII as well:
drivers/phy/qualcomm/phy-qcom-sgmii-eth-scmi.c:qcom_dwmac_sgmii_phy_scmi_validate() {
switch (submode) {
case PHY_INTERFACE_MODE_SGMII:
case PHY_INTERFACE_MODE_1000BASEX:
case PHY_INTERFACE_MODE_2500BASEX:
return 0;
default:
return -EINVAL;
...
}
With a phys property, qcom_ethqos_serdes_powerup()->phy_set_mode_ext()
fails at open time. Without one, the MAC runs in RGMII mode with no
wrapper configuration. In both cases probe succeeds, but the interface
never works.
Would it be better for ethqos_scmi_setup() to return -EINVAL for modes
that don't use the SerDes, or for the binding to limit phy-mode to an
enum, or both?
> +
> + return 0;
> +}
[ ... ]
@@ -98,6 +98,11 @@ struct ethqos_emac_driver_data {
const char *link_clk_name;
struct dwmac4_addrs dwmac4_addrs;
bool needs_sgmii_loopback;
+ /* Fixed PTP reference clock rate, used on SCMI platforms where
+ * firmware does not implement the clock protocol allowing linux to
+ * query it.
+ */
+ unsigned long ptp_clk_rate;
int (*setup)(struct qcom_ethqos *ethqos,
struct plat_stmmacenet_data *plat_dat);
};
@@ -108,6 +113,7 @@ struct qcom_ethqos {
struct clk *link_clk;
struct phy *serdes_phy;
phy_interface_t phy_mode;
+ struct dev_pm_domain_list *pds;
const struct ethqos_emac_driver_data *data;
};
@@ -207,6 +213,8 @@ static void ethqos_set_func_clk_en(struct qcom_ethqos *ethqos)
static int ethqos_hlos_setup(struct qcom_ethqos *ethqos,
struct plat_stmmacenet_data *plat_dat);
+static int ethqos_scmi_setup(struct qcom_ethqos *ethqos,
+ struct plat_stmmacenet_data *plat_dat);
static const struct ethqos_emac_por emac_v2_3_0_por[] = {
{ .offset = RGMII_IO_MACRO_CONFIG, .value = 0x00C01343 },
@@ -311,6 +319,30 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
.setup = ethqos_hlos_setup,
};
+static const struct ethqos_emac_driver_data emac_v4_0_0_scmi_data = {
+ .has_emac_ge_3 = true,
+ .needs_sgmii_loopback = true,
+ .dma_addr_width = 36,
+ .ptp_clk_rate = 230400000,
+ .dwmac4_addrs = {
+ .dma_chan = 0x00008100,
+ .dma_chan_offset = 0x1000,
+ .mtl_chan = 0x00008000,
+ .mtl_chan_offset = 0x1000,
+ .mtl_ets_ctrl = 0x00008010,
+ .mtl_ets_ctrl_offset = 0x1000,
+ .mtl_txq_weight = 0x00008018,
+ .mtl_txq_weight_offset = 0x1000,
+ .mtl_send_slp_cred = 0x0000801c,
+ .mtl_send_slp_cred_offset = 0x1000,
+ .mtl_high_cred = 0x00008020,
+ .mtl_high_cred_offset = 0x1000,
+ .mtl_low_cred = 0x00008024,
+ .mtl_low_cred_offset = 0x1000,
+ },
+ .setup = ethqos_scmi_setup,
+};
+
static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
{
struct device *dev = ðqos->pdev->dev;
@@ -756,6 +788,80 @@ static int ethqos_hlos_setup(struct qcom_ethqos *ethqos,
return 0;
}
+static const char *const ethqos_scmi_pd_names[] = { "core", "mdio" };
+
+static int ethqos_scmi_setup(struct qcom_ethqos *ethqos,
+ struct plat_stmmacenet_data *plat_dat)
+{
+ const struct dev_pm_domain_attach_data pd_data = {
+ .pd_names = ethqos_scmi_pd_names,
+ .num_pd_names = ARRAY_SIZE(ethqos_scmi_pd_names),
+ .pd_flags = PD_FLAG_DEV_LINK_ON,
+ };
+
+ struct platform_device *pdev = ethqos->pdev;
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ ret = devm_pm_domain_attach_list(dev, &pd_data, ðqos->pds);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "Failed to attach power domains\n");
+
+ /*
+ * The SerDes lane, its clocks and the MAC AXI/AHB clocks are owned by
+ * firmware and brought up through the SCMI power domains above. The
+ * MAC wrapper itself, however is in the kernel's register space: the
+ * wrapper bit that loops the PHY TX clock into the MAC's clk_rx_i -
+ * needed because no recovered RX clock exists yet - is not
+ * configured by firmware. Without it, clk_rx_i never toggles and the
+ * DMA SW-reset polled in dwmac4_dma_reset() never completes.
+ *
+ * Map the wrapper and program the same loopback/functional clock bits
+ * the non-firmware platforms rely on (see ethqos_clks_config) so the
+ * RX clock is present by the time the DMA engine is reset.
+ */
+ ethqos->rgmii_base = devm_platform_ioremap_resource_byname(pdev, "rgmii");
+ if (IS_ERR(ethqos->rgmii_base))
+ return dev_err_probe(dev, PTR_ERR(ethqos->rgmii_base),
+ "Failed to map rgmii resource\n");
+
+ /*
+ * Register clks_config to run on every subsequent runtime resume,
+ * which stmmac performs after the power domains are on but before
+ * serdes_powerup() and the DMA reset. This alone isn't enough for
+ * the very first reset done from stmmac_open(), though: probe()
+ * fakes the device into RPM_ACTIVE without invoking the resume
+ * callback, so whether clks_config() has run by then depends on
+ * whether the device has already gone through one real suspend/resume
+ * cycle in the meantime. Call it here explicitly so the wrapper is
+ * always configured before the first DMA reset regardless of that
+ * timing.
+ */
+ plat_dat->clks_config = ethqos_clks_config;
+
+ 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;
+
+ plat_dat->clk_ptp_rate = ethqos->data->ptp_clk_rate;
+
+ switch (ethqos->phy_mode) {
+ case PHY_INTERFACE_MODE_2500BASEX:
+ case PHY_INTERFACE_MODE_SGMII:
+ plat_dat->fix_mac_speed = ethqos_fix_mac_speed_sgmii;
+ break;
+ default:
+ break;
+ }
+
+ return 0;
+}
+
static int qcom_ethqos_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
@@ -843,6 +949,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_v2_3_0_data},
+ { .compatible = "qcom,sa8255p-ethqos", .data = &emac_v4_0_0_scmi_data},
{ .compatible = "qcom,sa8775p-ethqos", .data = &emac_v4_0_0_data},
{ .compatible = "qcom,sc8280xp-ethqos", .data = &emac_v3_0_0_data},
{ .compatible = "qcom,sm8150-ethqos", .data = &emac_v2_1_0_data},