[PATCH 2/3] designware: Use the remove() method with related drivers

[Simon Glass]

Hi Jonas,

On Sun, 6 Apr 2025 at 21:02, Jonas Karlman <jonas at kwiboo.se> wrote:
>
> 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.
>

That's interesting. I wrote that code about 9 years ago so perhaps can
be forgiven for forgetting.

Very few drivers set the DM_FLAG_ACTIVE_DMA flag, certainly not the
designware ones. So how would they be removed?

> 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.

Hmm yes I saw that but then apparently forgot to include it.

I can perhaps redo this patch to add the DMA flag.

>
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > Reported-by: Christian Kohlschütter <christian at kohlschutter.com>
> > ---
> >
> >  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.

Yes, I wasn't sure which way to go on that. I'll remove them.

>
> 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),
>

Regards,
Simon