Hi Simon, On 2025-04-06 00:12, Simon Glass wrote: > Several drivers make use of the designware Ethernet driver but do not > implement the remove() method. Add this so that DMA is stopped when the > OS is booted to avoid memory corruption, etc.
The designware Ethernet driver core should not need the remove() ops as the eth_ops.stop() ops already should stop DMA. And the eth-uclass pre_remove() ops already call the eth_ops.stop() ops. All these drivers seem to use designware_eth_ops and thus have correct stop() ops configured and should really not need the remove() ops. Side note, this also misses the gmac rockchip driver that uses its own eth_ops instead of designware_eth_ops, however it also use same stop() ops as all these drivers. > > Signed-off-by: Simon Glass <[email protected]> > Reported-by: Christian Kohlschütter <[email protected]> > --- > > drivers/net/designware.c | 2 +- > drivers/net/designware.h | 12 ++++++++++++ > drivers/net/dwmac_meson8b.c | 6 ++++++ > drivers/net/dwmac_s700.c | 6 ++++++ > drivers/net/dwmac_socfpga.c | 6 ++++++ > 5 files changed, 31 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/designware.c b/drivers/net/designware.c > index eebf14bd51a..d4d31925fca 100644 > --- a/drivers/net/designware.c > +++ b/drivers/net/designware.c > @@ -805,7 +805,7 @@ clk_err: > return err; > } > > -static int designware_eth_remove(struct udevice *dev) > +int designware_eth_remove(struct udevice *dev) > { > struct dw_eth_dev *priv = dev_get_priv(dev); > > diff --git a/drivers/net/designware.h b/drivers/net/designware.h > index e47101ccaf6..8c9d0190e03 100644 > --- a/drivers/net/designware.h > +++ b/drivers/net/designware.h > @@ -247,6 +247,18 @@ struct dw_eth_dev { > > int designware_eth_of_to_plat(struct udevice *dev); > int designware_eth_probe(struct udevice *dev); > + > +/** > + * designware_eth_remove() - Remove the device > + * > + * Disables DMA and marks the device as remove. This must be called before > + * booting an OS, to ensure that DMA is inactive. > + * > + * @dev: Device to remove > + * Return 0 if OK, -ve on error > + */ > +int designware_eth_remove(struct udevice *dev); > + > extern const struct eth_ops designware_eth_ops; > > struct dw_eth_pdata { > diff --git a/drivers/net/dwmac_meson8b.c b/drivers/net/dwmac_meson8b.c > index fde4aabbace..b2152f8da9b 100644 > --- a/drivers/net/dwmac_meson8b.c > +++ b/drivers/net/dwmac_meson8b.c > @@ -145,6 +145,11 @@ static int dwmac_meson8b_probe(struct udevice *dev) > return designware_eth_probe(dev); > } > > +static int dwmac_meson8b_remove(struct udevice *dev) > +{ > + return designware_eth_remove(dev); > +} > + > static const struct udevice_id dwmac_meson8b_ids[] = { > { .compatible = "amlogic,meson-gxbb-dwmac", .data = > (ulong)dwmac_setup_gx }, > { .compatible = "amlogic,meson-g12a-dwmac", .data = > (ulong)dwmac_setup_axg }, > @@ -158,6 +163,7 @@ U_BOOT_DRIVER(dwmac_meson8b) = { > .of_match = dwmac_meson8b_ids, > .of_to_plat = dwmac_meson8b_of_to_plat, > .probe = dwmac_meson8b_probe, > + .remove = dwmac_meson8b_remove, There is no reason to add the dwmac_meson8b_remove function above, you could just use designware_eth_remove directly here. Same for remaining drivers below. Regards, Jonas > .ops = &designware_eth_ops, > .priv_auto = sizeof(struct dw_eth_dev), > .plat_auto = sizeof(struct dwmac_meson8b_plat), > diff --git a/drivers/net/dwmac_s700.c b/drivers/net/dwmac_s700.c > index 969d247b4f3..cfb37c3aa71 100644 > --- a/drivers/net/dwmac_s700.c > +++ b/drivers/net/dwmac_s700.c > @@ -44,6 +44,11 @@ static int dwmac_s700_probe(struct udevice *dev) > return designware_eth_probe(dev); > } > > +static int dwmac_s700_remove(struct udevice *dev) > +{ > + return designware_eth_remove(dev); > +} > + > static int dwmac_s700_of_to_plat(struct udevice *dev) > { > return designware_eth_of_to_plat(dev); > @@ -60,6 +65,7 @@ U_BOOT_DRIVER(dwmac_s700) = { > .of_match = dwmac_s700_ids, > .of_to_plat = dwmac_s700_of_to_plat, > .probe = dwmac_s700_probe, > + .remove = dwmac_s700_remove, > .ops = &designware_eth_ops, > .priv_auto = sizeof(struct dw_eth_dev), > .plat_auto = sizeof(struct eth_pdata), > diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c > index a9e2d8c0972..8e7a9975d28 100644 > --- a/drivers/net/dwmac_socfpga.c > +++ b/drivers/net/dwmac_socfpga.c > @@ -130,6 +130,11 @@ static int dwmac_socfpga_probe(struct udevice *dev) > return designware_eth_probe(dev); > } > > +static int dwmac_socfpga_remove(struct udevice *dev) > +{ > + return designware_eth_remove(dev); > +} > + > static const struct udevice_id dwmac_socfpga_ids[] = { > { .compatible = "altr,socfpga-stmmac" }, > { } > @@ -141,6 +146,7 @@ U_BOOT_DRIVER(dwmac_socfpga) = { > .of_match = dwmac_socfpga_ids, > .of_to_plat = dwmac_socfpga_of_to_plat, > .probe = dwmac_socfpga_probe, > + .remove = dwmac_socfpga_remove, > .ops = &designware_eth_ops, > .priv_auto = sizeof(struct dw_eth_dev), > .plat_auto = sizeof(struct dwmac_socfpga_plat),
