| Message ID | 1506177052.15003206.1719232828107.JavaMail.zimbra@univ-grenoble-alpes.fr |
|---|---|
| State | Changes Requested |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | Improve networking support on FriendlyElec Nanopi R5C | expand |
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é <etienne.duble@imag.fr> > --- > > 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 Cheers, Quentin
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é <etienne.duble@imag.fr> >> --- >> >> 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
Hi Etienne, On 2024-06-25 08:47, Etienne Dublé wrote: > 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é <etienne.duble@imag.fr> >>> --- >>> >>> 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. 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. 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. Regards, Jonas > > Cheers, > Etienne >
> Date: Tue, 25 Jun 2024 10:29:48 +0200 > From: Jonas Karlman <jonas@kwiboo.se> Hi Jonas, > Hi Etienne, > > On 2024-06-25 08:47, Etienne Dublé wrote: > > 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é <etienne.duble@imag.fr> > >>> --- > >>> > >>> 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. > > 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. I do see similar issues on OpenBSD, where some configurations of the outbound address translation windows work fine and others don't. It does seem to depend on the PCIe device, but the only configuration that seems to work for everything is when the translation of the mmio windows is 1:1. So for OpenBSD we build U-Boot with a pacthed device tree. The downside of using a 1:1 translation for mmio is that this reduces the size of the 32-bit PCI mmio address space. Now I can't rule out that there are bugs in the OpenBSD driver for the RK3568 PCIe controller, but the driver seems to work fine on other PCIe controllers derived from the Synopsis DesignWare stuff. Etienne's diff isn't using 1:1 translation though. It just makes sure the lower 32 bits of the address are translated 1:1. I'll see if I can retest OpenBSD with such a setup. > 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. > > Regards, > Jonas > > > > > Cheers, > > Etienne > > > >
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@fe270000 { [...] ranges = <0x01000000 0x0 0xf2100000 0x0 0xf2100000 0x0 0x00100000>, <0x02000000 0x0 0xf2200000 0x0 0xf2200000 0x0 0x01e00000>, <0x03000000 *0x0* *0x40000000* 0x3 0x40000000 0x0 0x40000000>; [...] } [...] pcie3x2: pcie@fe280000 { [...] ranges = <0x01000000 0x0 0xf0100000 0x0 0xf0100000 0x0 0x00100000>, <0x02000000 0x0 0xf0200000 0x0 0xf0200000 0x0 0x01e00000>, <0x03000000 *0x0* *0x40000000* 0x3 0x80000000 0x0 0x40000000>; [...] } ---- 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* *0x80000000*". 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
Hello Etienne, On 2024-06-25 12:12, Etienne Dublé wrote: > > 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. 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. > >> 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. 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 0x340000000+, and 0x380000000+ is mapped to bus address 0x40000000+ of each pci controller. > We have these pcie entries in rk3568.dtsi: > > ---- > pcie3x1: pcie@fe270000 { > [...] > ranges = <0x01000000 0x0 0xf2100000 0x0 0xf2100000 0x0 > 0x00100000>, > <0x02000000 0x0 0xf2200000 0x0 0xf2200000 0x0 > 0x01e00000>, > <0x03000000 *0x0* *0x40000000* 0x3 0x40000000 > 0x0 0x40000000>; > [...] > } > [...] > pcie3x2: pcie@fe280000 { > [...] > ranges = <0x01000000 0x0 0xf0100000 0x0 0xf0100000 0x0 > 0x00100000>, > <0x02000000 0x0 0xf0200000 0x0 0xf0200000 0x0 > 0x01e00000>, > <0x03000000 *0x0* *0x40000000* 0x3 0x80000000 > 0x0 0x40000000>; > [...] > } > ---- > > 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* *0x80000000*". > 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. > > 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. 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. Regards, Jonas > > Cheers, > Etienne >
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 0x340000000+, > and 0x380000000+ is mapped to bus address 0x40000000+ 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* *0x80000000*". >> 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
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
diff --git a/dts/upstream/src/arm64/rockchip/rk3568.dtsi b/dts/upstream/src/arm64/rockchip/rk3568.dtsi index f1be76a54c..06aac034ca 100644 --- a/dts/upstream/src/arm64/rockchip/rk3568.dtsi +++ b/dts/upstream/src/arm64/rockchip/rk3568.dtsi @@ -150,7 +150,7 @@ <0x0 0xf0000000 0x0 0x00100000>; ranges = <0x01000000 0x0 0xf0100000 0x0 0xf0100000 0x0 0x00100000>, <0x02000000 0x0 0xf0200000 0x0 0xf0200000 0x0 0x01e00000>, - <0x03000000 0x0 0x40000000 0x3 0x80000000 0x0 0x40000000>; + <0x03000000 0x0 0x80000000 0x3 0x80000000 0x0 0x40000000>; reg-names = "dbi", "apb", "config"; resets = <&cru SRST_PCIE30X2_POWERUP>; reset-names = "pipe";
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é <etienne.duble@imag.fr> --- dts/upstream/src/arm64/rockchip/rk3568.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)