Re: [PATCH 2/3] rockchip: fix wrong PCI range address in rk3568 dtsi
Le 25/06/2024 à 13:31, Etienne Dublé a écrit : The bus addresses should be isolated to each pci controller if I am not mistaken, so changing the bus address was probably just done as a workaround because of some other unknown bug. OK. Hum, could be an issue in pci core of U-Boot that expect unique bus addresses across all pci controllers. Will run some quick tests on my R5C later. OK, thanks for looking at it. Hello Jonas, Could you have a look at this issue with the 2nd interface on the nanopi r5c, and your idea about a possible issue in U-Boot core about non-unique PCI bus addresses? Thanks, Etienne -- Etienne Dublé CNRS / LIG - Bâtiment IMAG 700 avenue Centrale - 38401 St Martin d'Hères Bureau 426 - Tel 0457421431
Re: [PATCH 1/3] net: give a different name to rtl8169 interfaces
Hi Quentin, Le 27/06/2024 à 14:27, Quentin Schulz a écrit : Hi Etienne, On 6/25/24 1:42 PM, Etienne Dublé wrote: Hello, Le 25/06/2024 à 12:25, Dragan Simic a écrit : Another option may be to name them "rtl8169@", with "" reflecting the PCI region address (so it is unique and stable). What do you think? I guess that's one way, I'm also wondering how systemd renames those to be unique but stable on the same machine, maybe we could take some inspiration from them for that? Systemd also allows renaming of network interfaces using some rules predefined by the user, so there's also the possibility to rename the interfaces in U-Boot to ethernet0 and ethernet1, using fixed rules that would be based on the PCI region address. OK but the renaming occurs in the rtl8169 driver, that is used by several boards, whereas the PCI region addresses come from the device-tree, so they differ from board to board. I'm of the opinion that we only care about stability for the same product, not for different products with the same Ethernet PHY on different SoCs/products. I agree, but my comment was more about how u-boot code is structured (network interface naming is done in the driver, and the driver code should probably not include board-specific code, so we are quite limited). In theory we could just name the interfaces ethernet0, ethernet1, etc., by using the PCI region address as the sort key, and maybe this is what Dragan was suggesting. But in practice when driver->bind() is called we don't know if more interfaces will be discovered next; and renaming the 1st interface when the 2nd one is discovered with a lower address is probably not the kind of code u-boot code experts would like... Cheers, Etienne Dublé -- Etienne Dublé CNRS / LIG - Bâtiment IMAG 700 avenue Centrale - 38401 St Martin d'Hères Bureau 426 - Tel 0457421431
Re: [PATCH 1/3] net: give a different name to rtl8169 interfaces
Hello, Le 25/06/2024 à 12:25, Dragan Simic a écrit : Another option may be to name them "rtl8169@", with "" reflecting the PCI region address (so it is unique and stable). What do you think? I guess that's one way, I'm also wondering how systemd renames those to be unique but stable on the same machine, maybe we could take some inspiration from them for that? Systemd also allows renaming of network interfaces using some rules predefined by the user, so there's also the possibility to rename the interfaces in U-Boot to ethernet0 and ethernet1, using fixed rules that would be based on the PCI region address. OK but the renaming occurs in the rtl8169 driver, that is used by several boards, whereas the PCI region addresses come from the device-tree, so they differ from board to board. Regards, Etienne -- Etienne Dublé CNRS / LIG - Bâtiment IMAG 700 avenue Centrale - 38401 St Martin d'Hères Bureau 426 - Tel 0457421431
Re: [PATCH 2/3] rockchip: fix wrong PCI range address in rk3568 dtsi
Hello Jonas, Le 25/06/2024 à 12:46, Jonas Karlman a écrit : Ahh, linux is still missing a patch to be able to use full address ranges as a root complex. Will re-run some tests on my R5C on both u-boot and linux to see if I can replicate your issue. OK. To use the second interface in u-boot, you first need to provide a ".bind()" member function in the rtl8169 driver, otherwise both interfaces have the same name (this is my [PATCH 1/3]). Then : Hit any key to stop autoboot: 0 => pci enum => net list eth0 : RTL8169#0 52:e8:a1:60:81:e7 active eth1 : RTL8169#1 52:e8:a1:60:81:e6 => setenv ethact "RTL8169#1" => dhcp The issue appears when sending the first packet. When activating DEBUG_RTL8169_TX in rtl8169.c it prints "tx timeout/error". The issue may be that U-Boot is not fully capable of having overlapping bus addresses for multiple pci controllers. To my knowledge these addresses should be internal to the pci controller and its devices. The addresses below tells us that system memory address 0x34000+, and 0x38000+ is mapped to bus address 0x4000+ of each pci controller. I see. [...] So I verified in the downstream repository of the board vendor (https://github.com/friendlyarm/uboot-rockchip/blob/nanopi5-v2017.09/arch/arm/dts/rk3568.dtsi#L1754C21-L1754C31 and the second address there was replaced with "*0x0* *0x8000*". Then, updating this was enough to make the second interface work in u-boot. The bus addresses should be isolated to each pci controller if I am not mistaken, so changing the bus address was probably just done as a workaround because of some other unknown bug. OK. Hum, could be an issue in pci core of U-Boot that expect unique bus addresses across all pci controllers. Will run some quick tests on my R5C later. OK, thanks for looking at it. Regards, Etienne -- Etienne Dublé CNRS / LIG - Bâtiment IMAG 700 avenue Centrale - 38401 St Martin d'Hères Bureau 426 - Tel 0457421431
Re: [PATCH 2/3] rockchip: fix wrong PCI range address in rk3568 dtsi
Hello Jonas,
Le 25/06/2024 à 10:29, Jonas Karlman a écrit :
I do not understand the need for such/this patch. The changed address is
the internal io address that is shared with the pci controller and
devices.
Do you have any issue in linux or is it only in U-Boot?, I suspect this
change is only a workaround to an issue only found in U-Boot.
On my board (Nanopi R5C with two RTL-8125B interfaces) both RTL-8125B
interfaces work in Linux, and only the first one works in u-boot.
But Linux is apparently using the second PCI region and u-boot is using
the third (with the suspected address). These PCI regions are of the
same type.
The rtl8169 driver or pci system of U-Boot may be of fault and should
probably be fixed instead of changing io addresses to work around issues
in software. E.g rtl8169 has a static ioaddr that is shared between all
drivers, something that may cause issues.
I am not an expert, but I really believe the issue comes from this
address in the device tree.
We have these pcie entries in rk3568.dtsi:
pcie3x1: pcie@fe27 {
[...]
ranges = <0x0100 0x0 0xf210 0x0 0xf210 0x0
0x0010>,
<0x0200 0x0 0xf220 0x0 0xf220 0x0
0x01e0>,
<0x0300 *0x0* *0x4000* 0x3 0x4000
0x0 0x4000>;
[...]
}
[...]
pcie3x2: pcie@fe28 {
[...]
ranges = <0x0100 0x0 0xf010 0x0 0xf010 0x0
0x0010>,
<0x0200 0x0 0xf020 0x0 0xf020 0x0
0x01e0>,
<0x0300 *0x0* *0x4000* 0x3 0x8000
0x0 0x4000>;
[...]
}
Again, I am not an expert, but it seemed suspicious to me that those two
entries were sharing the same PCI address.
So I verified in the downstream repository of the board vendor
(https://github.com/friendlyarm/uboot-rockchip/blob/nanopi5-v2017.09/arch/arm/dts/rk3568.dtsi#L1754C21-L1754C31),
and the second address there was replaced with "*0x0* *0x8000*".
Then, updating this was enough to make the second interface work in u-boot.
Like you, I initially suspected the code of rtl8169.c, which has many
global & static variables, so I actually spent quite a lot of time on
refactoring it, moving things to the private struct, but could never
make it work until I found this suspecting PCI address.
Cheers,
Etienne
--
Etienne Dublé
CNRS / LIG - Bâtiment IMAG
700 avenue Centrale - 38401 St Martin d'Hères
Bureau 426 - Tel 0457421431
Re: [PATCH 2/3] rockchip: fix wrong PCI range address in rk3568 dtsi
Hi Quentin, Le 24/06/2024 à 15:15, Quentin Schulz a écrit : Hi Etienne, On 6/24/24 2:40 PM, ETIENNE DUBLE wrote: One of the PCI ranges was wrong in this device tree. When testing with a FriendlyElec Nanopi R5C board, the 2nd ethernet interface (labelled "wan") was not working in u-boot because of that. With the correct value (found in FriendlyElec's downstream u-boot repository), this 2nd ethernet interface now works. Signed-off-by: Etienne Dublé --- dts/upstream/src/arm64/rockchip/rk3568.dtsi | 2 +- dts/upstream is only for patches coming from **merged** Linux kernel (i.e. releases or -rc releases or master branch from Linus Torvalds). We do not accept U-Boot-only patches in dts/upstream. Please submit the patch to the Linux kernel first and it will eventually make it to that directory either via a dts/update-dts-subtree.sh pull or dts/update-dts-subtree.sh pick. Depending on maintainer's opinion, fixing the range in arch/arm/dts/rk3568-u-boot.dtsi could be an option, but we really need the patch sent to upstream Linux kernel community first. c.f. https://www.kernel.org/doc/html/v6.5/process/submitting-patches.html I see, I will look at it. In version 2 of the series I will remove this second patch and just mention that we are waiting for this problem to be fixed upstream, in the cover letter. Cheers, Etienne -- Etienne Dublé CNRS / LIG - Bâtiment IMAG 700 avenue Centrale - 38401 St Martin d'Hères Bureau 426 - Tel 0457421431
Re: [PATCH 1/3] net: give a different name to rtl8169 interfaces
Hi Quentin,
Thanks for reviewing my patches.
Le 24/06/2024 à 15:08, Quentin Schulz a écrit :
[...]
1 file changed, 11 insertions(+)
diff --git a/drivers/net/rtl8169.c b/drivers/net/rtl8169.c
index 93e83661ce..b30d51731f 100644
--- a/drivers/net/rtl8169.c
+++ b/drivers/net/rtl8169.c
@@ -1091,6 +1091,16 @@ static int rtl8169_eth_probe(struct udevice *dev)
return 0;
}
+static int rtl8169_eth_bind(struct udevice *dev)
+{
+ static int card_number;
+ char name[16];
+
+ sprintf(name, "RTL8169#%u", card_number++);
+
+ return device_set_name(dev, name);
+}
+
I don't think we can guarantee bind order so this may not be stable
over time.
I'm wondering if there isn't a way to use the "ethernet" (ethernet0,
ethernet1) alias in DT instead?
Actually the ethernet interfaces are not declared in the device tree,
only PCI buses are (at least on this Nanopi board).
The ethernet interfaces are only detected when running "pci enum".
Another option may be to name them "rtl8169@", with ""
reflecting the PCI region address (so it is unique and stable). What do
you think?
Cheers,
Etienne
--
Etienne Dublé
CNRS / LIG - Bâtiment IMAG
700 avenue Centrale - 38401 St Martin d'Hères
Bureau 426 - Tel 0457421431
Re: [PATCH v2] watchdog: Add a watchdog driver for Raspberry Pi boards
Hi Stefan,
+/* The hardware supports a maximum timeout value of 0xf ticks
+ * (just below 16 seconds).
+ * U-boot users specify the timeout as a number of milliseconds
+ * by using variable CONFIG_WATCHDOG_TIMEOUT_MSECS.
+ * The maximum value should be 15999 ms in our case.
+ * However, u-boot internally converts this config value to seconds,
+ * thus specifying 15999 actually means 15000 ms (0xf ticks).
means 15999 ms?
No, 15000 ms.
Actually I observed that whatever value 15xxx I set in .config, the
driver code is called with value 15000.
I suppose this is due to the code in wdt-uclass.c which converts the
config value to an integer number of seconds.
So the limit I wrote in the code (15000 ms, 0xf ticks) is actually
"the max value the user can specify in configuration", whereas the
hardware limit is a little higher (0xf ticks).
However, thinking twice it is probably not a good idea to deal with the
behavior of higher layers here.
So I will change back the driver limit to 0xf ticks and replace this
long explanation with a minimal comment.
+
+ writel(PM_PASSWORD | timeout_ticks, base + PM_WDOG);
+ cur = readl(base + PM_RSTC);
+ writel(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
+ PM_RSTC_WRCFG_FULL_RESET, base + PM_RSTC);
This does not seem to be aligned with the "(" from the line above.
BTW: Please use scripts/checkpatch.pl to see, if the patch has no more
issues.
I did run it, but apparently it did not catch this one.
I will fix it.
+
+ if (priv->timeout_ticks > PM_WDOG_MAX_TICKS) {
+ printf("WARNING: bcm2835_wdt cannot handle large timeout
values.\n");
+ printf(" Setting the max value of 15000 ms instead.\n");
+ printf(" Set CONFIG_WATCHDOG_TIMEOUT_MSECS=15000 at most "
+ "to avoid this warning.\n");
This seems way too long for a warning from this driver. Please shorten
it to one line. The comment above already covers this limitation AFAICT.
OK.
Thanks,
Etienne
--
Etienne Dublé
CNRS / LIG - Bâtiment IMAG
700 avenue Centrale - 38401 St Martin d'Hères
Bureau 426 - Tel 0457421431
Re: [PATCH] watchdog: Add a watchdog driver for Raspberry Pi boards
Hi Stefan,
Thanks for the feedback.
I will send a v2 to fix those issues.
Regards,
Etienne
Le 24/01/2023 à 14:38, Stefan Roese a écrit :
Hi Etienne,
On 1/24/23 14:20, Etienne Dublé wrote:
This driver supports the bcm2835 watchdog found on
Raspberry Pi boards.
It is derived from the Linux driver and was tested
on two Raspberry Pi board versions (B+ and 3B+).
Signed-off-by: Etienne Dublé
---
drivers/watchdog/Kconfig | 9 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/bcm2835_wdt.c | 135 +
3 files changed, 145 insertions(+)
create mode 100644 drivers/watchdog/bcm2835_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f1b1cf63ca..06c0d630c8 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -30,6 +30,7 @@ config WATCHDOG_TIMEOUT_MSECS
default 128000 if ARCH_MX7 || ARCH_VF610
default 3 if ARCH_SOCFPGA
default 16000 if ARCH_SUNXI
+ default 15000 if ARCH_BCM283X
default 6
help
Watchdog timeout in msec
@@ -326,6 +327,14 @@ config WDT_SUNXI
help
Enable support for the watchdog timer in Allwinner sunxi SoCs.
+config WDT_BCM2835
+ bool "Broadcom 2835 watchdog timer support"
Your patch seems to have whitespace problems. Please make sure to send
it e.g. via git send-email.
More comments below...
+ depends on WDT && ARCH_BCM283X
+ default y
+ help
+ Enable support for the watchdog timer in Broadcom 283X SoCs such
+ as Raspberry Pi boards.
+
config XILINX_TB_WATCHDOG
bool "Xilinx Axi watchdog timer support"
depends on WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 446d961d7d..f99915960c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_WDT_APPLE) += apple_wdt.o
obj-$(CONFIG_WDT_ARMADA_37XX) += armada-37xx-wdt.o
obj-$(CONFIG_WDT_ASPEED) += ast_wdt.o
obj-$(CONFIG_WDT_AST2600) += ast2600_wdt.o
+obj-$(CONFIG_WDT_BCM2835) += bcm2835_wdt.o
obj-$(CONFIG_WDT_BCM6345) += bcm6345_wdt.o
obj-$(CONFIG_WDT_BOOKE) += booke_wdt.o
obj-$(CONFIG_WDT_CORTINA) += cortina_wdt.o
diff --git a/drivers/watchdog/bcm2835_wdt.c
b/drivers/watchdog/bcm2835_wdt.c
new file mode 100644
index 00..48535c003f
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2013 Lubomir Rintel
+ * Copyright (C) 2023 Etienne Dublé (CNRS)
+ *
+ * This code is mostly derived from the linux driver.
+ */
+
+#include
+#include
+#include
+#include
+
+#define PM_RSTC 0x1c
+#define PM_WDOG 0x24
+
+#define PM_PASSWORD 0x5a00
+
+/* The hardware supports a maximum timeout value of 0xf ticks
+ * (just below 16 seconds).
+ * U-boot users specify the timeout as a number of milliseconds
+ * by using variable CONFIG_WATCHDOG_TIMEOUT_MSECS.
+ * The maximum value should be 15999 ms in our case.
+ * However, u-boot internally converts this config value to seconds,
+ * thus specifying 15999 actually means 15000 ms (0xf ticks).
+ */
+#define PM_WDOG_MAX_TICKS 0x000f
+#define PM_RSTC_WRCFG_CLR 0xffcf
+#define PM_RSTC_WRCFG_FULL_RESET 0x0020
+#define PM_RSTC_RESET 0x0102
+
+#define MS_TO_WDOG_TICKS(x) (((x) << 16) / 1000)
+
+struct bcm2835_wdt_priv {
+ void __iomem *base;
+};
+
+static u64 timeout_ticks = PM_WDOG_MAX_TICKS;
Is this static variable really needed? Why not add this variable (if
needed) into the priv struct above instead?
+
+static int bcm2835_wdt_start_ticks(struct udevice *dev,
+ u64 timeout_ticks, ulong flags)
+{
+ struct bcm2835_wdt_priv *priv = dev_get_priv(dev);
+ void __iomem *base = priv->base;
+ u32 cur;
+
+ writel(PM_PASSWORD | timeout_ticks, base + PM_WDOG);
+ cur = readl(base + PM_RSTC);
+ writel(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
+ PM_RSTC_WRCFG_FULL_RESET, base + PM_RSTC);
+
+ return 0;
+}
+
+static int bcm2835_wdt_start(struct udevice *dev, u64 timeout_ms,
ulong flags)
+{
+ timeout_ticks = MS_TO_WDOG_TICKS(timeout_ms);
Again, there should be no need for this static variable. Please move it
to the priv struct instead.
Thanks,
Stefan
+
+ if (timeout_ticks > PM_WDOG_MAX_TICKS) {
+ printf("WARNING: bcm2835_wdt cannot handle large timeout
values.\n");
+ printf(" Setting the max value of 15000 ms instead.\n");
+ printf(" Set CONFIG_WATCHDOG_TIMEOUT_MSECS=15000 at most "
+ "to avoid this warning.\n");
+ timeout_ticks = PM_WDOG_MAX_TICKS;
+ }
+
+ return bcm2835_wdt_start_ticks(dev, timeout_ticks, flags);
+}
+
+static int bcm2835_wdt_reset(struct udevice *dev)
+{
+ /* restart the timer with the static variable '
[PATCH] watchdog: Add a watchdog driver for Raspberry Pi boards
This driver supports the bcm2835 watchdog found on
Raspberry Pi boards.
It is derived from the Linux driver and was tested
on two Raspberry Pi board versions (B+ and 3B+).
Signed-off-by: Etienne Dublé
---
drivers/watchdog/Kconfig | 9 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/bcm2835_wdt.c | 135 +
3 files changed, 145 insertions(+)
create mode 100644 drivers/watchdog/bcm2835_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f1b1cf63ca..06c0d630c8 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -30,6 +30,7 @@ config WATCHDOG_TIMEOUT_MSECS
default 128000 if ARCH_MX7 || ARCH_VF610
default 3 if ARCH_SOCFPGA
default 16000 if ARCH_SUNXI
+ default 15000 if ARCH_BCM283X
default 6
help
Watchdog timeout in msec
@@ -326,6 +327,14 @@ config WDT_SUNXI
help
Enable support for the watchdog timer in Allwinner sunxi SoCs.
+config WDT_BCM2835
+ bool "Broadcom 2835 watchdog timer support"
+ depends on WDT && ARCH_BCM283X
+ default y
+ help
+ Enable support for the watchdog timer in Broadcom 283X SoCs such
+ as Raspberry Pi boards.
+
config XILINX_TB_WATCHDOG
bool "Xilinx Axi watchdog timer support"
depends on WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 446d961d7d..f99915960c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_WDT_APPLE) += apple_wdt.o
obj-$(CONFIG_WDT_ARMADA_37XX) += armada-37xx-wdt.o
obj-$(CONFIG_WDT_ASPEED) += ast_wdt.o
obj-$(CONFIG_WDT_AST2600) += ast2600_wdt.o
+obj-$(CONFIG_WDT_BCM2835) += bcm2835_wdt.o
obj-$(CONFIG_WDT_BCM6345) += bcm6345_wdt.o
obj-$(CONFIG_WDT_BOOKE) += booke_wdt.o
obj-$(CONFIG_WDT_CORTINA) += cortina_wdt.o
diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
new file mode 100644
index 00..48535c003f
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2013 Lubomir Rintel
+ * Copyright (C) 2023 Etienne Dublé (CNRS)
+ *
+ * This code is mostly derived from the linux driver.
+ */
+
+#include
+#include
+#include
+#include
+
+#define PM_RSTC0x1c
+#define PM_WDOG0x24
+
+#define PM_PASSWORD0x5a00
+
+/* The hardware supports a maximum timeout value of 0xf ticks
+ * (just below 16 seconds).
+ * U-boot users specify the timeout as a number of milliseconds
+ * by using variable CONFIG_WATCHDOG_TIMEOUT_MSECS.
+ * The maximum value should be 15999 ms in our case.
+ * However, u-boot internally converts this config value to seconds,
+ * thus specifying 15999 actually means 15000 ms (0xf ticks).
+ */
+#define PM_WDOG_MAX_TICKS 0x000f
+#define PM_RSTC_WRCFG_CLR 0xffcf
+#define PM_RSTC_WRCFG_FULL_RESET 0x0020
+#define PM_RSTC_RESET 0x0102
+
+#define MS_TO_WDOG_TICKS(x) (((x) << 16) / 1000)
+
+struct bcm2835_wdt_priv {
+ void __iomem *base;
+};
+
+static u64 timeout_ticks = PM_WDOG_MAX_TICKS;
+
+static int bcm2835_wdt_start_ticks(struct udevice *dev,
+ u64 timeout_ticks, ulong flags)
+{
+ struct bcm2835_wdt_priv *priv = dev_get_priv(dev);
+ void __iomem *base = priv->base;
+ u32 cur;
+
+ writel(PM_PASSWORD | timeout_ticks, base + PM_WDOG);
+ cur = readl(base + PM_RSTC);
+ writel(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
+ PM_RSTC_WRCFG_FULL_RESET, base + PM_RSTC);
+
+ return 0;
+}
+
+static int bcm2835_wdt_start(struct udevice *dev, u64 timeout_ms, ulong
flags)
+{
+ timeout_ticks = MS_TO_WDOG_TICKS(timeout_ms);
+
+ if (timeout_ticks > PM_WDOG_MAX_TICKS) {
+ printf("WARNING: bcm2835_wdt cannot handle large timeout
values.\n");
+ printf("Setting the max value of 15000 ms instead.\n");
+ printf("Set CONFIG_WATCHDOG_TIMEOUT_MSECS=15000 at most "
+ "to avoid this warning.\n");
+ timeout_ticks = PM_WDOG_MAX_TICKS;
+ }
+
+ return bcm2835_wdt_start_ticks(dev, timeout_ticks, flags);
+}
+
+static int bcm2835_wdt_reset(struct udevice *dev)
+{
+ /* restart the timer with the static variable 'timeout_ticks'
+* saved from the last bcm2835_wdt_start() call.
+*/
+ return bcm2835_wdt_start_ticks(dev, timeout_ticks, 0);
+}
+
+static int bcm2835_wdt_stop(struct udevice *dev)
+{
+ struct bcm2835_wdt_priv *priv = dev_get_priv(dev);
+ void __iomem *base = priv->base;
+
+ writel(PM_PASSWORD | PM_RSTC_R
Re: [PATCH] bcmgenet: fix DMA buffer management
Hi Jason,
Thanks for testing and for the additional fix.
In my case (with dhcp followed by 3 TFTP transfers), the board booted fine 20
times in a row with only my fix applied. Previously it was booting once in 10
trials or so.
The driver resets tx_index in bcmgenet_gmac_eth_send() if it is higher than
256, and the same occurs for rx_index in bcmgenet_gmac_free_pkt(), so I suppose
the issue you faced only occurs when the boundary case is reached between two
commands?
Anyway, thanks for the update.
Cheers
Etienne
De: "Jason Wessel"
À: "Etienne DUBLE" , "joe hershberger"
Cc: u-boot@lists.denx.de, "ETIENNE DUBLE"
Envoyé: Jeudi 16 Juillet 2020 18:02:11
Objet: Re: [PATCH] bcmgenet: fix DMA buffer management
On 7/16/20 7:02 AM, Jason Wessel wrote:
> On 7/9/20 3:11 AM, etienne.du...@gmail.com wrote:
>> From: Etienne Dublé
>>
>> This commit fixes a serious issue occuring when several network
>> commands are run on a raspberry pi 4 board: for instance a "dhcp"
>> command and then one or several "tftp" commands. In this case,
>> packet recv callbacks were called several times on the same packets,
>> and send function was failing most of the time.
>>
>> note: if the boot procedure is made of a single network
>> command, the issue is not visible.
>>
>> The issue is related to management of the packet ring buffers
>> (producer / consumer) and DMA.
>> Each time a packet is received, the ethernet device stores it
>> in the buffer and increments an index called RDMA_PROD_INDEX.
>> Each time the driver outputs a received packet, it increments
>> another index called RDMA_CONS_INDEX.
>>
>> Between each pair of network commands, as part of the driver
>> 'start' function, previous code tried to reset both RDMA_CONS_INDEX
>> and RDMA_PROD_INDEX to 0. But RDMA_PROD_INDEX cannot be written from
>> driver side, thus its value was actually not updated, and only
>> RDMA_CONS_INDEX was reset to 0. This was resulting in a major
>> synchronization issue between the driver and the device. Most
>> visible bahavior was that the driver seemed to receive again the
>> packets from the previous commands (e.g. DHCP response packets
>> "received" again when performing the first TFTP command).
>>
>> This fix consists in setting RDMA_CONS_INDEX to the same
>> value as RDMA_PROD_INDEX, when resetting the driver.
>>
>> The same kind of fix was needed on the TX side, and a few variables
>> had to be reset accordingly (c_index, tx_index, rx_index).
>
>
> While there is some kind of problem with the driver, because I too
> have observed a problem with multiple requests timing out or failing,
> this patch makes the problem much worse. I was only able to complete
> a single tftp request.
>
> In my case I am using a static IP address and serverip.
>
> Also your patch was missing the sign-off line. Please consider
> running your patches through scripts/checkpatch.pl.
>
> Cheers,
> Jason.
>
>> ---
>> drivers/net/bcmgenet.c | 15 +++
>> 1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/bcmgenet.c b/drivers/net/bcmgenet.c
>> index 11b6148ab6..a4facfd63f 100644
>> --- a/drivers/net/bcmgenet.c
>> +++ b/drivers/net/bcmgenet.c
>> @@ -378,8 +378,6 @@ static void rx_descs_init(struct bcmgenet_eth_priv
>> *priv)
>> u32 len_stat, i;
>> void *desc_base = priv->rx_desc_base;
>>
>> - priv->c_index = 0;
>> -
>> len_stat = (RX_BUF_LENGTH << DMA_BUFLENGTH_SHIFT) | DMA_OWN;
>>
>> for (i = 0; i < RX_DESCS; i++) {
>> @@ -403,8 +401,10 @@ static void rx_ring_init(struct bcmgenet_eth_priv
>> *priv)
>> writel(RX_DESCS * DMA_DESC_SIZE / 4 - 1,
>> priv->mac_reg + RDMA_RING_REG_BASE + DMA_END_ADDR);
>>
>> - writel(0x0, priv->mac_reg + RDMA_PROD_INDEX);
>> - writel(0x0, priv->mac_reg + RDMA_CONS_INDEX);
>> + /* cannot init RDMA_PROD_INDEX to 0, so align RDMA_CONS_INDEX on it
>> instead */
>> + priv->c_index = readl(priv->mac_reg + RDMA_PROD_INDEX);
>> + writel(priv->c_index, priv->mac_reg + RDMA_CONS_INDEX);
>> + priv->rx_index = priv->c_index;
printf("before RX_IDX: 0x%x\n", priv->rx_index);
I added a printf() like above for the RX and TX to see what is going on when
I try and transfer a kernel Image file the second time.
U-Boot> tftp ${loadaddr} bootfs/Image
before RX_IDX: 0x0
before TX_IDX: 0x0
Using ethernet@7d58 device
Filename 'bootfs/Image'.
Load add
