Re: [PATCH net-next 5/5] net: stmmac: support the TC9564 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: stmmac: tc9564_reset_names mismatch with DT binding reset-names
- [High] net: stmmac: stmmac_get_platform_resources() zeroes DT MAC address
- [Medium] net: stmmac: tc9564: unconditional reset and clock disable in suspend breaks WoL
--

commit d94523eb7296bd3578512ea2f1d63c42d3751631
Author: Daniel Thompson <daniel@riscstar.com>

net: stmmac: support the TC9564 SoC

This commit adds stmmac glue layer support for the Ethernet XGMAC
functionality on the Toshiba TC9564 SoC via a platform driver bound to a PCI
endpoint bus node.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tc9564.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tc9564.c
> new file mode 100644
> index 0000000000000..21098f91e3bbf
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tc9564.c
> @@ -0,0 +1,590 @@

[ ... ]

> +enum {
> +	RESET_ID_MAC,
> +	RESET_ID_XPCS,
> +	RESET_ID_PMA,
> +};
> +
> +static const char * const tc9564_reset_names[] = {
> +	[RESET_ID_MAC] = "mac",
> +	[RESET_ID_XPCS] = "xpcs",
> +	[RESET_ID_PMA] = "pma",
> +};

[Severity: High]
Should the entries in tc9564_reset_names include the "-reset" suffix?

In Documentation/devicetree/bindings/net/toshiba,tc9564-xgmac.yaml, the
required reset-names property items are defined as:

  reset-names:
    items:
      - const: mac-reset
      - const: pma-reset
      - const: xpcs-reset

When tc9564_dwmac_probe() calls tc9564_reset_init() ->
devm_reset_control_bulk_get_exclusive(), the reset lookup matches each
td->resets[i].id against the device tree node's reset-names property. With
"mac", "xpcs", and "pma", won't reset lookup fail during probe on a
binding-compliant device tree?

[ ... ]

> +static void tc9564_mac_disable(struct tc9564_data *td)
> +{
> +	WARN_ON(reset_control_bulk_assert(ARRAY_SIZE(td->resets), td->resets));
> +
> +	clk_bulk_disable_unprepare(td->clock_count, td->clocks);
> +}

[ ... ]

> +static int tc9564_dwmac_suspend(struct device *dev, void *bsp_priv)
> +{
> +	struct tc9564_data *td = bsp_priv;
> +
> +	tc9564_mac_disable(td);
> +
> +	return 0;
> +}
> +
> +static int tc9564_dwmac_resume(struct device *dev, void *bsp_priv)
> +{
> +	struct tc9564_data *td = bsp_priv;
> +
> +	tc9564_mac_enable(td);
> +
> +	return 0;
> +}
> +
> +static int tc9564_probe_config_dt(struct tc9564_data *td)
> +{
> +	struct platform_device *pdev = to_platform_device(td->dev);
> +	struct device_node *np = td->dev->of_node;
> +	struct plat_stmmacenet_data *plat;
> +	phy_interface_t phy_interface;
> +	struct device *dev = td->dev;
> +	int ret;
> +
> +	ret = device_get_phy_mode(dev);
> +	if (ret < 0)
> +		return -ENODEV;
> +	phy_interface = ret;
> +
> +	/* The platform structure is allocated with devm_kzalloc() */
> +	plat = devm_stmmac_probe_config_dt(pdev, td->res.mac);
> +	if (IS_ERR(plat))
> +		return PTR_ERR(plat);
> +
> +	/*
> +	 * Core driver cannot deduce or validate fifo size because the bits
> +	 * that provide FIFO size only worth in powers of two.
> +	 */
> +	if (plat->rx_fifo_size == 0 || plat->tx_fifo_size == 0)
> +		return dev_err_probe(dev, -EINVAL, "fifo size is not set\n");
> +	if (plat->rx_fifo_size > TC9564_RX_FIFO_SIZE ||
> +	    plat->tx_fifo_size > TC9564_TX_FIFO_SIZE)
> +		return dev_err_probe(dev, -EINVAL, "fifo size too big (%u/%u)\n",
> +				     plat->rx_fifo_size, plat->tx_fifo_size);
> +
> +	/*
> +	 * toshiba,tc9564-xgmac is *not* compatible with snps,dwxgmac because
> +	 * generic drivers cannot possibly work on TC9564 (without clocks
> +	 * reading registers wedges the bus). However, without snps,dwxgmac,
> +	 * devm_stmmac_probe_config_dt(), can't figure of the core type and
> +	 * won't parse a couple of extra properties. Let's handle that here!
> +	 */
> +	plat->core_type = DWMAC_CORE_XGMAC;
> +	plat->pmt = true;

[Severity: Medium]
Does calling tc9564_mac_disable() unconditionally in tc9564_dwmac_suspend()
break Wake-on-LAN?

In tc9564_probe_config_dt(), plat->pmt is set to true. When Wake-on-LAN is
enabled via ethtool and the system suspends, stmmac_suspend() programs the
XGMAC receiver and PMT wakeup registers via stmmac_pmt() and keeps the link
active via phylink_suspend(priv->phylink, true), before calling
priv->plat->suspend().

Because tc9564_dwmac_suspend() unconditionally calls tc9564_mac_disable(),
all three resets (RESET_ID_MAC, RESET_ID_XPCS, RESET_ID_PMA) are asserted
and all clocks are disabled without checking device_may_wakeup(dev), which
clears the programmed PMT state and shuts down the receive path.

[ ... ]

> +static int tc9564_dwmac_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = tc9564_probe_config_dt(td);
> +	if (ret)
> +		return ret;
> +
> +	ret = stmmac_get_platform_resources(pdev, &td->res);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to initialize stmmac resources\n");

[Severity: High]
Does calling stmmac_get_platform_resources() after tc9564_probe_config_dt()
wipe out the MAC address read from the device tree?

In tc9564_probe_config_dt(), devm_stmmac_probe_config_dt(pdev, td->res.mac)
populates td->res.mac from the device tree. However,
stmmac_get_platform_resources(pdev, &td->res) is called immediately
afterward, and it begins with:

drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c:stmmac_get_platform_resources() {
	int ret;

	memset(stmmac_res, 0, sizeof(*stmmac_res));

This zeroes the entire td->res structure, including td->res.mac, before it
is passed to stmmac_dvr_probe(). Should stmmac_get_platform_resources() be
called before tc9564_probe_config_dt()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019030129.1470307-1-elder@riscstar.com?part=5