Hi Jonas,
On Sun, 6 Apr 2025 at 21:02, Jonas Karlman <jonas@...> 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@...>
Reported-by: Christian Kohlschütter <christian@...>
---
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
