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

[Jonas Karlman]

Hi Simon,

On 2025-04-06 23:10, Simon Glass wrote:
> 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?

Ahh, I see, remove() is only called for devices with ACTIVE_DMA or
OS_PREPARE. So I guess that was probably the issue all along?

There is some confusion that dm_remove_devices_active() does not remove
device_active()=true devices and instead only remove ACTIVE_DMA and/or
OS_PREPARE flagged devices.

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

Maybe eth_post_probe() or similar should add the OS_PREPARE or
ACTIVE_DMA flag for all eth-uclass, to ensure remove() and thus stop()
gets called when dm_remove_devices_active() is called.

There is probably no need to add all these remove() ops, as they would
still not have been called by dm_remove_devices_active().

Regards,
Jonas

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