| Message ID | 20211208214309.233041-1-sander@svanheule.net |
|---|---|
| Headers | show |
| Series | Switch realtek target to upstream platform | expand |
Hi Sander and Hiroshi, great work! A very big step to clean up this platform. Because this changes the fundamentals of the target I have some suggestions and questions. There are actually 4 RTL platforms which we now know how to support and you are taking care of the first 3 in your patch to make use of the generic MIPS platform: - RTL8380: 4Kec, single core - RTL8390: 34Kc, VSMP, MIPS timer works - RTL9300: 34Kc, VSMP, MIPS timer broken, but RTL9300 timer with SMP support - RTL9310: InterAptiv, SMP, GIC timer I would suggest to use sub-targets for these platforms with optimized compiler settings and SMP/single processor selected correctly for each, see https://github.com/BrainSlayer/pie/commits/pie-5.10-rtl9313 You already set up SMP build support in the config, but don't activate it. However with 2 small patches to the IRQ controller https://github.com/BrainSlayer/pie/commit/61860e04b89fa4c76a0e41af3e5d3480dabdf63c#diff-8b9c10bdba2282c6926cb9f7207ac869b011e248a6171f8132e79e1807c9f12a and the Ethernet driver https://github.com/BrainSlayer/pie/commit/cdbc6598a1e69f74aa5fbaac9c896595a06d21f4#diff-2adae55db41693c452fcc10fb3925ff8c5a38760243f0b77bbd54acc75170d0a SMP can be enabled on the RTL8390 and the 9310 platform. I am a bit worried of using the generic MIPS platform for the RTL9300. The SoC has a broken R4K Clock Event Interrupt and requires the RTL9300 timer to work plus SMP support for the IRQ controller. There are only 2 examples in Linux which have such a combination of dedicated Timer for SMP support and IRQ controller, the Sibyte SB1250 and the BCM1480 platforms. Both do not use the generic MIPS platform since they require lots of specific SMP code. I would like to understand how this would be supported with the generic MIPS platform, because I don't want to go there and then back again. Cheers, Birger On 08.12.21 22:42, Sander Vanheule wrote: > To reduce the maintenance burden of the realtek target in OpenWrt, Hiroshi and > I have worked on these patches to switch to the upstream platform. Some > downstream code is maintained, primarily for compatibility with the current > downstream-only networking drivers. There should be no functional changes for > users, although there will be some bootlog differences. > > We hope these patches will make it easier for developers to write upstream- > compatible code on OpenWrt. Vice versa, it should also be easier with these > changes to backport drivers and patches that were upstreamed. > > Sander Vanheule (13): > realtek: add missing gpio0 pinctrl properties > realtek: add sys-led disable pinctrl for rtl930x > realtek: remove hardcoded sys-led configurations > realtek: use fixed-clock as CPU clock > realtek: include io.h in mach-rtl83xx.h > realtek: update mach-rtl83xx.h includes > realtek: backport upstream platform > realtek: add board file for MACH_REALTEK_RTL > realtek: update driver platform dependencies > realtek: switch target to upstream platform > realtek: drop downstream platform > realtek: modernise devicetree console setup > realtek: use correct compatible for rtl930x SoCs > > target/linux/realtek/config-5.10 | 36 +++- > .../dts-5.10/rtl8380_netgear_gigabit.dtsi | 3 + > .../dts-5.10/rtl8382_d-link_dgs-1210.dtsi | 3 + > .../dts-5.10/rtl8382_inaba_aml2-17gp.dts | 3 + > target/linux/realtek/dts-5.10/rtl838x.dtsi | 16 +- > target/linux/realtek/dts-5.10/rtl930x.dtsi | 26 ++- > .../arch/mips/generic/board-realtek.c | 145 ++++++++++++++ > .../mips/include/asm/mach-rtl838x/ioremap.h | 29 --- > .../include/asm/mach-rtl838x/mach-rtl83xx.h | 1 + > .../files-5.10/arch/mips/rtl838x/Makefile | 5 - > .../files-5.10/arch/mips/rtl838x/Platform | 5 - > .../files-5.10/arch/mips/rtl838x/prom.c | 183 ------------------ > .../files-5.10/arch/mips/rtl838x/setup.c | 116 ----------- > .../drivers/clocksource/timer-rtl9300.c | 2 +- > .../drivers/net/dsa/rtl83xx/Kconfig | 2 +- > target/linux/realtek/image/Makefile | 4 +- > ...-add-realtek-rtl838x-rtl839x-support.patch | 45 +++++ > .../300-mips-add-rtl838x-platform.patch | 39 ---- > .../301-gpio-add-rtl8231-driver.patch | 2 +- > ...e-dependencies-for-gpio-realtek-otto.patch | 13 -- > ...pdate-dependency-for-spi-realtek-rtl.patch | 11 -- > ...pdate-dependency-for-irq-realtek-rtl.patch | 8 - > .../306-gpio-add-legacy-rtl838x-driver.patch | 2 +- > ...date-dependency-for-realtek-otto-wdt.patch | 15 -- > ...-in-board-realtek-for-generic-kernel.patch | 9 + > ...net-add-support-for-rtl838x-ethernet.patch | 2 +- > .../patches-5.10/705-add-rtl-phy.patch | 2 +- > 27 files changed, 285 insertions(+), 442 deletions(-) > create mode 100644 target/linux/realtek/files-5.10/arch/mips/generic/board-realtek.c > delete mode 100644 target/linux/realtek/files-5.10/arch/mips/include/asm/mach-rtl838x/ioremap.h > delete mode 100644 target/linux/realtek/files-5.10/arch/mips/rtl838x/Makefile > delete mode 100644 target/linux/realtek/files-5.10/arch/mips/rtl838x/Platform > delete mode 100644 target/linux/realtek/files-5.10/arch/mips/rtl838x/prom.c > delete mode 100644 target/linux/realtek/files-5.10/arch/mips/rtl838x/setup.c > create mode 100644 target/linux/realtek/patches-5.10/009-5.12-MIPS-add-realtek-rtl838x-rtl839x-support.patch > delete mode 100644 target/linux/realtek/patches-5.10/300-mips-add-rtl838x-platform.patch > delete mode 100644 target/linux/realtek/patches-5.10/303-gpio-update-dependencies-for-gpio-realtek-otto.patch > delete mode 100644 target/linux/realtek/patches-5.10/304-spi-update-dependency-for-spi-realtek-rtl.patch > delete mode 100644 target/linux/realtek/patches-5.10/305-irqchip-update-dependency-for-irq-realtek-rtl.patch > delete mode 100644 target/linux/realtek/patches-5.10/307-wdt-update-dependency-for-realtek-otto-wdt.patch > create mode 100644 target/linux/realtek/patches-5.10/309-mips-built-in-board-realtek-for-generic-kernel.patch >
Sander Vanheule <sander@svanheule.net> writes: > To reduce the maintenance burden of the realtek target in OpenWrt, Hiroshi and > I have worked on these patches to switch to the upstream platform. Some > downstream code is maintained, primarily for compatibility with the current > downstream-only networking drivers. There should be no functional changes for > users, although there will be some bootlog differences. > > We hope these patches will make it easier for developers to write upstream- > compatible code on OpenWrt. Vice versa, it should also be easier with these > changes to backport drivers and patches that were upstreamed. Nice! Tested the series on my Netgear GS108Tv3 and it works beautifully Tested-by: Bjørn Mork <bjorn@mork.no> Initial bootlog diff in case it's of interest to anyone: --- /tmp/a 2021-12-09 09:56:22.445327002 +0100 +++ /tmp/b 2021-12-09 09:56:24.041264026 +0100 @@ -13,22 +13,20 @@ ## Booting image from partition ... 0 ## Booting kernel from Legacy Image at 81000000 ... Version: MIPS OpenWrt Linux-5.10.82 - Created: 2021-11-29 13:09:16 UTC - Data Size: 2281063 Bytes = 2.2 MB + Created: 2021-12-09 8:18:28 UTC + Data Size: 2369255 Bytes = 2.3 MB Checksum ... OK Uncompressing ... OK Starting kernel ... -Linux version 5.10.82 (bjorn@canardo) (mips-openwrt-linux-musl-gcc (OpenWrt GCC 11.2.0 r17903+1-a2fcd3900c0c) 11.2.0, GNU ld (GNU Binutils) 2.37) #0 Mon Nov 29 13:09:16 2021 -RTL838X model is 83806800 -SoC Type: RTL8380 -Kernel command line: +Linux version 5.10.82 (bjorn@canardo) (mips-openwrt-linux-musl-gcc (OpenWrt GCC 11.2.0 r17903+1-a2fcd3900c0c) 11.2.0, GNU ld (GNU Binutils) 2.37) #0 SMP Thu Dec 9 08:18:28 2021 printk: bootconsole [early0] enabled CPU0 revision is: 00019070 (MIPS 4KEc) MIPS: machine is Netgear GS108T v3 +earlycon: ns16550a0 at MMIO 0x18002000 (options '115200n8') +printk: bootconsole [ns16550a0] enabled Initrd not found or empty - disabling initrd -Using appended Device Tree. Primary instruction cache 16kB, VIPT, 4-way, linesize 16 bytes. Primary data cache 16kB, 2-way, VIPT, cache aliases, linesize 16 bytes Zone ranges: @@ -41,46 +39,54 @@ Normal zone: 288 pages used for memmap Normal zone: 0 pages reserved Normal zone: 32768 pages, LIFO batch:7 -pcpu-alloc: s0 r0 d32768 u32768 alloc=1*32768 +percpu: Embedded 14 pages/cpu s26160 r8192 d22992 u57344 +pcpu-alloc: s26160 r8192 d22992 u57344 alloc=14*4096 pcpu-alloc: [0] 0 Built 1 zonelists, mobility grouping on. Total pages: 32480 -Kernel command line: console=ttyS0,115200 +Kernel command line: earlycon Dentry cache hash table entries: 16384 (order: 4, 65536 bytes, linear) Inode-cache hash table entries: 8192 (order: 3, 32768 bytes, linear) mem auto-init: stack:off, heap alloc:off, heap free:off -Memory: 120784K/131072K available (5623K kernel code, 636K rwdata, 1256K rodata, 1244K init, 203K bss, 10288K reserved, 0K cma-reserved) +Memory: 120484K/131072K available (5948K kernel code, 609K rwdata, 1236K rodata, 1224K init, 206K bss, 10588K reserved, 0K cma-reserved) SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1 +rcu: Hierarchical RCU implementation. +rcu: RCU restricting CPUs from NR_CPUS=2 to nr_cpu_ids=1. + Tracing variant of Tasks RCU enabled. +rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies. +rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=1 NR_IRQS: 256 -random: get_random_bytes called from start_kernel+0x31c/0x504 with crng_init=0 +random: get_random_bytes called from start_kernel+0x32c/0x544 with crng_init=0 timer_probe: no matching timers found -CPU frequency from device tree: 500MHz clocksource: MIPS: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 7645041786 ns sched_clock: 32 bits at 250MHz, resolution 4ns, wraps every 8589934590ns Calibrating delay loop... 498.89 BogoMIPS (lpj=2494464) pid_max: default: 32768 minimum: 301 Mount-cache hash table entries: 1024 (order: 0, 4096 bytes, linear) Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes, linear) +rcu: Hierarchical SRCU implementation. +dyndbg: Ignore empty _ddebug table in a CONFIG_DYNAMIC_DEBUG_CORE build +smp: Bringing up secondary CPUs ... +smp: Brought up 1 node, 1 CPU clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns -futex hash table entries: 256 (order: -1, 3072 bytes, linear) +futex hash table entries: 256 (order: 1, 8192 bytes, linear) pinctrl core: initialized pinctrl subsystem NET: Registered protocol family 16 +FPU Affinity set after 9960 emulations clocksource: Switched to clocksource MIPS NET: Registered protocol family 2 IP idents hash table entries: 2048 (order: 2, 16384 bytes, linear) -tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 4096 bytes, linear) +tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 6144 bytes, linear) TCP established hash table entries: 1024 (order: 0, 4096 bytes, linear) -TCP bind hash table entries: 1024 (order: 0, 4096 bytes, linear) +TCP bind hash table entries: 1024 (order: 1, 8192 bytes, linear) TCP: Hash tables configured (established 1024 bind 1024) -UDP hash table entries: 256 (order: 0, 4096 bytes, linear) -UDP-Lite hash table entries: 256 (order: 0, 4096 bytes, linear) +UDP hash table entries: 256 (order: 1, 8192 bytes, linear) +UDP-Lite hash table entries: 256 (order: 1, 8192 bytes, linear) NET: Registered protocol family 1 workingset: timestamp_bits=14 max_order=15 bucket_order=1 squashfs: version 4.0 (2009/01/31) Phillip Lougher jffs2: version 2.2 (NAND) (SUMMARY) (ZLIB) (LZMA) (RTIME) (CMODE_PRIORITY) (c) 2001-2006 Red Hat, Inc. pinctrl-single 1b001000.pinmux: 32 pins, size 4 pinctrl-single 1b00a000.pinmux: 32 pins, size 4 -Probing RTL8231 GPIOs -rtl8231_init called, MDIO bus ID: 31 Serial: 8250/16550 driver, 2 ports, IRQ sharing disabled printk: console [ttyS0] disabled 18002000.uart: ttyS0 at MMIO 0x18002000 (irq = 31, base_baud = 12500000) is a 16550A @@ -88,6 +94,8 @@ printk: console [ttyS0] enabled printk: bootconsole [early0] disabled printk: bootconsole [early0] disabled +printk: bootconsole [ns16550a0] disabled +printk: bootconsole [ns16550a0] disabled brd: module loaded spi-nor spi0.0: mx25l25635e (32768 Kbytes) 7 fixed-partitions partitions found on MTD device spi0.0
Hi Birger, On Thu, 2021-12-09 at 08:19 +0100, Birger Koblitz wrote: > Hi Sander and Hiroshi, > > great work! A very big step to clean up this platform. Because this changes > the fundamentals of the target I have some suggestions and questions. Thanks! I was expecting some comments from you :-) > > There are actually 4 RTL platforms which we now know how to support and you > are taking care of the first 3 in your patch to make use of the generic MIPS > platform: > - RTL8380: 4Kec, single core > - RTL8390: 34Kc, VSMP, MIPS timer works > - RTL9300: 34Kc, VSMP, MIPS timer broken, but RTL9300 timer with SMP support > - RTL9310: InterAptiv, SMP, GIC timer > > I would suggest to use sub-targets for these platforms with optimized > compiler settings and SMP/single processor selected correctly for each, > see https://github.com/BrainSlayer/pie/commits/pie-5.10-rtl9313 > > You already set up SMP build support in the config, but don't activate it. Currently there are no supported devices in OpenWrt that have one of the SMP- capable SoCs, which is the main reason it is not yet included here. I've had a very quick look on enabling SMP for RTL839x with a mainline build, but haven't gotten it to work yet. Hiroshi and I are still learning as we go along here. I don't know if we can reasonably expect to have one kernel that supports all SoCs, or if there are really conflicting build requirements. > However with 2 small patches to the IRQ controller > https://github.com/BrainSlayer/pie/commit/61860e04b89fa4c76a0e41af3e5d3480dabdf63c#diff-8b9c10bdba2282c6926cb9f7207ac869b011e248a6171f8132e79e1807c9f12a > The patch you link here doesn't seem to apply to the current IRQ controller, but to an already modified version of it. It also appears to implement interrupt balancing, but is that a hard requirement for SMP on RTL839x? > and the Ethernet driver > https://github.com/BrainSlayer/pie/commit/cdbc6598a1e69f74aa5fbaac9c896595a06d21f4#diff-2adae55db41693c452fcc10fb3925ff8c5a38760243f0b77bbd54acc75170d0a > SMP can be enabled on the RTL8390 and the 9310 platform. > > I am a bit worried of using the generic MIPS platform for the RTL9300. > The SoC has a broken R4K Clock Event Interrupt and requires the RTL9300 > timer to work plus SMP support for the IRQ controller. There are only > 2 examples in Linux which have such a combination of dedicated Timer > for SMP support and IRQ controller, the Sibyte SB1250 and the BCM1480 > platforms. A cursory search (and limited understanding of clocksources etc.) also turned up the Ingenic SoCs for me. These appear to have custom timers too, but are still based on MIPS_GENERIC. Since this Realtek timer is also available on RTL838x/RTL839x, we could use these platforms for testing as well. The kernel also supports the clocksource= parameter, which appears to allow for alternative clock selection. So if a (generic) kernel with R4K-CEVT support is running on RTL930x, shouldn't we be able to convince it to really use the Realtek timer? > Both do not use the generic MIPS platform since they require lots of > specific SMP code. I would like to understand how this would be supported > with the generic MIPS platform, because I don't want to go there and > then back again. I understand and agree with you, ping-ponging between custom/generic would be a waste of time. By using the upstream platform we should be less limited by the (human) resources available in OpenWrt alone. Bert got plenty of feedback when he submitted this platform upstream, and was basically able to replace all the custom code with the right selection of kernel config options. If Realtek did really funny stuff that makes MIPS_GENERIC incompatible with some of these SoCs, I still feel we should try going through upstream. But at least then we would be quite sure new code is the only way to support these chips. Best, Sander
Hi, thanks for the quick reply. On 09.12.21 10:38, Sander Vanheule wrote: > Hi Birger, > >> >> I would suggest to use sub-targets for these platforms with optimized >> compiler settings and SMP/single processor selected correctly for each, >> see https://github.com/BrainSlayer/pie/commits/pie-5.10-rtl9313 >> >> You already set up SMP build support in the config, but don't activate it. > > Currently there are no supported devices in OpenWrt that have one of the SMP- > capable SoCs, which is the main reason it is not yet included here. I've had a > very quick look on enabling SMP for RTL839x with a mainline build, but haven't > gotten it to work yet. I don't think you can pull the argument about that there are currently no supported devices in master as this concerns an architectural decision for all SoC generations. Also people are already using these devices, plus there are several developers trying to get support for the 839x up to the 931x into master bit-by-bit so it can see at least a bit of peer-review. We need to find a long-term solution, here and this change is very disruptive for development. As you can imagine, a lot of the code I use is never in a git, but instrumentation of core mips code to be able to debug deadlocks and RCU stalls. > Hiroshi and I are still learning as we go along here. I don't know if we can > reasonably expect to have one kernel that supports all SoCs, or if there are > really conflicting build requirements. > > >> However with 2 small patches to the IRQ controller >> https://github.com/BrainSlayer/pie/commit/61860e04b89fa4c76a0e41af3e5d3480dabdf63c#diff-8b9c10bdba2282c6926cb9f7207ac869b011e248a6171f8132e79e1807c9f12a >> > > The patch you link here doesn't seem to apply to the current IRQ controller, but > to an already modified version of it. It also appears to implement interrupt > balancing, but is that a hard requirement for SMP on RTL839x? The IRQ controller needs a lot of update and the patch is not on the backported patch for the controller but the actual file with the controller code. I really don't like to debug or do development on top of patches, it is much harder. I would suggest to just copy the new version into the tree, there was nothing there before. The IRQ balancing is not necessary strictly speaking for 8390, but if you want to use it with the RTL9300 timer, which needs to send IRQs to both CPUs, it is needed. So you would need it for your proposal below to test this on the 8390 with the timer. > >> and the Ethernet driver >> https://github.com/BrainSlayer/pie/commit/cdbc6598a1e69f74aa5fbaac9c896595a06d21f4#diff-2adae55db41693c452fcc10fb3925ff8c5a38760243f0b77bbd54acc75170d0a >> SMP can be enabled on the RTL8390 and the 9310 platform. >> >> I am a bit worried of using the generic MIPS platform for the RTL9300. >> The SoC has a broken R4K Clock Event Interrupt and requires the RTL9300 >> timer to work plus SMP support for the IRQ controller. There are only >> 2 examples in Linux which have such a combination of dedicated Timer >> for SMP support and IRQ controller, the Sibyte SB1250 and the BCM1480 >> platforms. > > A cursory search (and limited understanding of clocksources etc.) also turned up > the Ingenic SoCs for me. These appear to have custom timers too, but are still > based on MIPS_GENERIC. I had not noticed that one, it looks promising. There is now also a stand-alone CEVT-reimplementation of the RTL9300 timer here: https://github.com/BrainSlayer/pie/blob/pie-5.10-rtl9313/target/linux/realtek/files-5.10/arch/mips/kernel/cevt-rtl9300.c > > Since this Realtek timer is also available on RTL838x/RTL839x, we could use > these platforms for testing as well. The kernel also supports the clocksource= > parameter, which appears to allow for alternative clock selection. So if a > (generic) kernel with R4K-CEVT support is running on RTL930x, shouldn't we be > able to convince it to really use the Realtek timer? This should work. The RTL9300 event source actually pretends to be more accurate than the R4K-CEVT (it isn't but has more features such as one-shot), so that it is being picked up instead. > If Realtek did really funny stuff that makes MIPS_GENERIC incompatible with some > of these SoCs, I still feel we should try going through upstream. But at least > then we would be quite sure new code is the only way to support these chips. Good idea. Could you try using the RTL9300-CEVT timer and the new IRQ controller with MIPS_GENERIC and SMP? Maybe it works. Sebastian and I are quite sure the timer does what it is supposed to do and the IRQ controller works like a charm on the 8390 under SMP, so either MIPS_GENERIC works, or there are more issues with the SoC, including ones that make it so broken that it does not actually work with SMP. Realtek does not provide any support for SMP to device makers, so maybe the silicon is kaputt. In any case, could you look into the suggestion with the sub-targets? The different compile options should make quite a difference in performance. Cheers, Birger
Hi Birger, On Thu, 2021-12-09 at 13:26 +0100, Birger Koblitz wrote: > Hi, > > thanks for the quick reply. Thank you for the input! > On 09.12.21 10:38, Sander Vanheule wrote: > > Hi Birger, > > > > > > > > I would suggest to use sub-targets for these platforms with optimized > > > compiler settings and SMP/single processor selected correctly for each, > > > see https://github.com/BrainSlayer/pie/commits/pie-5.10-rtl9313 > > > > > > You already set up SMP build support in the config, but don't activate it. > > > > Currently there are no supported devices in OpenWrt that have one of the SMP- > > capable SoCs, which is the main reason it is not yet included here. I've had a > > very quick look on enabling SMP for RTL839x with a mainline build, but haven't > > gotten it to work yet. > I don't think you can pull the argument about that there are currently no supported > devices in master as this concerns an architectural decision for all SoC generations. > Also people are already using these devices, plus there are several developers trying > to get support for the 839x up to the 931x into master bit-by-bit so it can see at > least a bit of peer-review. We need to find a long-term solution, here and this > change is very disruptive for development. As you can imagine, a lot of > the code I use is never in a git, but instrumentation of core mips code to > be able to debug deadlocks and RCU stalls. This patch series is intended to reduce the maintenance burden, without introducing breaking changes to devices already in OpenWrt. No promises are made for out-of-tree code. Of course we need to discuss how we should move forward to implement working SMT support etc., but I feel that this patch series should not depend on that discussion. There is upstream code that runs on RTL8380 and RTL8390, and OpenWrt code provides the same features in a different way. If we want to support SMT upstream, we may have to submit code there anyway, so why do things twice? If you would like code to be supported, please submit patches we can review. Frequent small, incremental changes would be fine for me; maybe even preferable. IMHO you're not doing yourself a favour by keeping months worth of changes out-of-tree. > > > Hiroshi and I are still learning as we go along here. I don't know if we can > > reasonably expect to have one kernel that supports all SoCs, or if there are > > really conflicting build requirements. > > > > > > > However with 2 small patches to the IRQ controller > > > https://github.com/BrainSlayer/pie/commit/61860e04b89fa4c76a0e41af3e5d3480dabdf63c#diff-8b9c10bdba2282c6926cb9f7207ac869b011e248a6171f8132e79e1807c9f12a > > > > > > > The patch you link here doesn't seem to apply to the current IRQ controller, but > > to an already modified version of it. It also appears to implement interrupt > > balancing, but is that a hard requirement for SMP on RTL839x? > The IRQ controller needs a lot of update and the patch is not on the backported > patch for the controller but the actual file with the controller code. > I really don't like to debug or do development on top of patches, it is much > harder. I would suggest to just copy the new version into the tree, there was > nothing there before. > > The IRQ balancing is not necessary strictly speaking for 8390, but if you want > to use it with the RTL9300 timer, which needs to send IRQs to both CPUs, it is > needed. So you would need it for your proposal below to test this on the 8390 with the > timer. > > > > > > and the Ethernet driver > > > https://github.com/BrainSlayer/pie/commit/cdbc6598a1e69f74aa5fbaac9c896595a06d21f4#diff-2adae55db41693c452fcc10fb3925ff8c5a38760243f0b77bbd54acc75170d0a > > > SMP can be enabled on the RTL8390 and the 9310 platform. > > > > > > I am a bit worried of using the generic MIPS platform for the RTL9300. > > > The SoC has a broken R4K Clock Event Interrupt and requires the RTL9300 > > > timer to work plus SMP support for the IRQ controller. There are only > > > 2 examples in Linux which have such a combination of dedicated Timer > > > for SMP support and IRQ controller, the Sibyte SB1250 and the BCM1480 > > > platforms. > > > > A cursory search (and limited understanding of clocksources etc.) also turned up > > the Ingenic SoCs for me. These appear to have custom timers too, but are still > > based on MIPS_GENERIC. > I had not noticed that one, it looks promising. There is now also a stand-alone > CEVT-reimplementation of the RTL9300 timer here: > https://github.com/BrainSlayer/pie/blob/pie-5.10-rtl9313/target/linux/realtek/files-5.10/arch/mips/kernel/cevt-rtl9300.c > > > > > Since this Realtek timer is also available on RTL838x/RTL839x, we could use > > these platforms for testing as well. The kernel also supports the clocksource= > > parameter, which appears to allow for alternative clock selection. So if a > > (generic) kernel with R4K-CEVT support is running on RTL930x, shouldn't we be > > able to convince it to really use the Realtek timer? > This should work. The RTL9300 event source actually pretends to be more accurate than > the R4K-CEVT (it isn't but has more features such as one-shot), so that it is > being picked up instead. > > > If Realtek did really funny stuff that makes MIPS_GENERIC incompatible with some > > of these SoCs, I still feel we should try going through upstream. But at least > > then we would be quite sure new code is the only way to support these chips. > Good idea. Could you try using the RTL9300-CEVT timer and the new IRQ controller > with MIPS_GENERIC and SMP? Maybe it works. Sebastian and I are quite sure the > timer does what it is supposed to do and the IRQ controller works like a charm > on the 8390 under SMP, so either MIPS_GENERIC works, or there are more issues > with the SoC, including ones that make it so broken that it does not actually work > with SMP. Realtek does not provide any support for SMP to device makers, so > maybe the silicon is kaputt. > > In any case, could you look into the suggestion with the sub-targets? The different > compile options should make quite a difference in performance. Does enabling those different options on one build have that big an impact on SoCs that don't support all selected features? (I really don't know, don't normally do any performance testing.) I would be happy to continue discussing the interrupt controller, RTL9300 timer code, and SMT support, but maybe not in this thread. Cheers, Sander
Hi, On 09.12.21 22:26, Sander Vanheule wrote: > This patch series is intended to reduce the maintenance burden, without introducing > breaking changes to devices already in OpenWrt. No promises are made for out-of-tree code. This patch will unfortunately considerably increase the maintenance burden, because it pushes the code into a direction which does not reflect what these devices are. They use SMT, SMP and they have different cpu-architectures. This should be reflected by any new choice for the core architecture of the realtek code in OpenWRT. With regard to out-of tree code: This is where the features come from. They come from that development and it's the features users care for, the reason they are willing to test new stuff and the reason some users provide access to development devices. Also, the RTL9300 and RTL9310 code is in-tree. This is how we make sure that new developments are consistent across all architectures. > Of course we need to discuss how we should move forward to implement working SMT support > etc., but I feel that this patch series should not depend on that discussion. There is > upstream code that runs on RTL8380 and RTL8390, and OpenWrt code provides the same > features in a different way. If we want to support SMT upstream, we may have to submit > code there anyway, so why do things twice? It is much too early to submit this upstream. We have already provided code upstream before we had sufficient understanding of how things work several times. Example: the interrupt controller code was pushed to mainline before we understood that this actually is at the core of how to make VSMP work on the RTL9300. We did not even understand why there was a double set of registers already on the RTL8390, because of course there this is not necessary. But now you just told me that the patches for this IRQ controller do not apply to the patches that provide that controller from mainline? Patches on top of patches are not a maintenance burden? When in addition the controller needs a complete re-write because we had no clue what it really was supposed to do? And another example: the GPIO driver went to mainline before we understood how it works on the RTL9300. Again, in order to use it on these devices we have patches on top of patches and this is not a figure of speech, this is reality. And it still does not work on the RTL9300, at least not for me. But I need this to work reliably to be able to provide support. And don't let me get started how it works on the RTL931x. I am really happy I resisted the push to have the RTL9300 timer code upstreamed, because again, it is doing something completely different from what we thought at that time: the timer is not about being able to do precision cable diagnostics, it is about making VSMP work on the RTL9300. And for that it needs to work quite differently including a big re-write. > > If you would like code to be supported, please submit patches we can review. Frequent > small, incremental changes would be fine for me; maybe even preferable. IMHO you're not > doing yourself a favour by keeping months worth of changes out-of-tree. I have been providing code bit by bit in the past, such as the RTL9300 support and there are even bits of the RTL931x architecture which allow booting of such devices. But now this is not being recognised, since there are no devices in tree to use that code. Well, I tried to add those, I tried with the GS1900-48 using the RTL8390 and I tried with the XGS1210 using the RTL9300. But guess what? I was told to come back in a couple of months because the support was not complete and SFP/SFP+ and the like worked. I really would like to provide more often code, but something as simple as adding Routing Offload needed 3k lines of code and that needs time to develop. Providing half of it is not going to fly: which user wants half of their traffic end in nirvana, or all of their traffic being uncontrollable by any firewall? Add on top of that that routing offload works quite differently on the different platforms and the fact that you need to make a reasonable choice how to support them under one hat, and unfortunately this means months of work. I would love to provide 10GBit SFP+ support on the RTL9300 quickly at this point, but then I am having real trouble with the GPIO driver on that platform which I need for the SFP cages but is patches on top of mainline patches. > Does enabling those different options on one build have that big an impact on SoCs that > don't support all selected features? (I really don't know, don't normally do any > performance testing.) The differences to be expected are about 50-120% more speed for optimized architectures plus (V)SMP support. 50% for VSMP, 100% for SMP, plus maybe 20% for optimized compiler settings. I did some promising tests with dropbearkey. > > I would be happy to continue discussing the interrupt controller, RTL9300 timer code, and > SMT support, but maybe not in this thread. Fine, there is no need for that. I just would like from your side to take the RTL9300 architecture into account when you make a proposal to go MIPS_GENERIC so that we do not need to go back with all our code again or have to provide RTL9300 support by patches on top of patches. And I would like that this patch does not close its eyes on the fact that the architectures now support SMP and VSMP and takes this into account, too, even if it is based on more than 1 year old code that was pushed into mainline at a point in time we were lacking very basic understanding of the RTL SoCs. Cheers, Birger
Hi Birger, On Fri, 2021-12-10 at 13:30 +0100, Birger Koblitz wrote: > Hi, > > On 09.12.21 22:26, Sander Vanheule wrote: > > This patch series is intended to reduce the maintenance burden, without introducing > > breaking changes to devices already in OpenWrt. No promises are made for out-of-tree > > code. > This patch will unfortunately considerably increase the maintenance burden, because it > pushes the code into a direction which does not reflect what these devices are. They > use SMT, SMP and they have different cpu-architectures. This should be reflected > by any new choice for the core architecture of the realtek code in OpenWRT. > > With regard to out-of tree code: This is where the features come from. They come from > that development and it's the features users care for, the reason they are willing > to test new stuff and the reason some users provide access to development devices. Also, > the RTL9300 and RTL9310 code is in-tree. This is how we make sure that new developments > are consistent across all architectures. > > > Of course we need to discuss how we should move forward to implement working SMT > > support > > etc., but I feel that this patch series should not depend on that discussion. There is > > upstream code that runs on RTL8380 and RTL8390, and OpenWrt code provides the same > > features in a different way. If we want to support SMT upstream, we may have to submit > > code there anyway, so why do things twice? > It is much too early to submit this upstream. We have already provided code upstream > before we had sufficient understanding of how things work several times. Example: > the interrupt controller code was pushed to mainline before we understood that this > actually is at the core of how to make VSMP work on the RTL9300. We did not even > understand why there was a double set of registers already on the RTL8390, because of > course there this is not necessary. But now you just told me that the patches for this > IRQ controller do not apply to the patches that provide that controller from mainline? > Patches on top of patches are not a maintenance burden? When in addition the controller > needs a complete re-write because we had no clue what it really was supposed to do? > And another example: the GPIO driver went to mainline before we understood how it works > on the RTL9300. Again, in order to use it on these devices we have patches on top of > patches and this is not a figure of speech, this is reality. And it still does not work > on the RTL9300, at least not for me. But I need this to work reliably to be able to > provide support. And don't let me get started how it works on the RTL931x. > I am really happy I resisted the push to have the RTL9300 timer code upstreamed, because > again, it is doing something completely different from what we thought at that time: > the timer is not about being able to do precision cable diagnostics, it is about making > VSMP work on the RTL9300. And for that it needs to work quite differently including > a big re-write. I've provided a patch below that enables VPE support for RTL839x. Since RTL930x uses the same CPU architecture, I expect it to also bring up both threads there. Like you note RTL930x requires a specific clocksource driver, but it should be possible to run your current code for that on top of this (ammended) series. I will respin the series with some small changes, and add that VSMP-support patch. Please let us know if you can get your reworked IRQ driver and timer code working on top of the v2. That way, you wouldn't have to take a step back in your development, and we could continue providing support through upstream. > > > > If you would like code to be supported, please submit patches we can review. Frequent > > small, incremental changes would be fine for me; maybe even preferable. IMHO you're > > not > > doing yourself a favour by keeping months worth of changes out-of-tree. > I have been providing code bit by bit in the past, such as the RTL9300 support and there > are even bits of the RTL931x architecture which allow booting of such devices. But > now this is not being recognised, since there are no devices in tree to use that code. > Well, I tried to add those, I tried with the GS1900-48 using the RTL8390 and I tried > with the XGS1210 using the RTL9300. But guess what? I was told to come back in a couple > of months because the support was not complete and SFP/SFP+ and the like worked. > I really would like to provide more often code, but something as simple as adding > Routing > Offload needed 3k lines of code and that needs time to develop. Providing half of it is > not > going to fly: which user wants half of their traffic end in nirvana, or all of their > traffic being uncontrollable by any firewall? Add on top of that that routing offload > works > quite differently on the different platforms and the fact that you need to make a > reasonable > choice how to support them under one hat, and unfortunately this means months of work. > I would love to provide 10GBit SFP+ support on the RTL9300 quickly at this point, > but then I am having real trouble with the GPIO driver on that platform which I need for > the SFP cages but is patches on top of mainline patches. I was aware that you had device support prepared, but I didn't remember that it was rejected earlier for being incomplete. You're right that it's not always possible to submit patches quickly. Personally, I also haven't completed RTL9300 GPIO support, because I was not able to implement the IRQ balancing yet. I can submit a preliminary patch set to OpenWrt that doesn't do balancing, but at least provides the same features and IRQ support as RTL8380/RTL8390. > > Does enabling those different options on one build have that big an impact on SoCs > > that > > don't support all selected features? (I really don't know, don't normally do any > > performance testing.) > The differences to be expected are about 50-120% more speed for optimized architectures > plus (V)SMP support. > 50% for VSMP, 100% for SMP, plus maybe 20% for optimized compiler settings. I did some > promising > tests with dropbearkey. > > > > > I would be happy to continue discussing the interrupt controller, RTL9300 timer code, > > and > > SMT support, but maybe not in this thread. > Fine, there is no need for that. I just would like from your side to take the RTL9300 > architecture into account when you make a proposal to go MIPS_GENERIC so that we do not > need to go back with all our code again or have to provide RTL9300 support by patches on > top of patches. > And I would like that this patch does not close its eyes on the fact that the > architectures now support SMP and VSMP and takes this into account, too, even if it is > based on more than 1 year old code that was pushed into mainline at a point in time we > were lacking very basic understanding of the RTL SoCs. Hiroshi found that all we need to do to get VSMP working on RTL839x, is to somehow call register_vsmp_smp_ops(). The MIPS_GENERIC platform does not currently do this, but it can be added with a small patch, just like how MT7621 performs SMP initialisation: --- >8 --- --- a/arch/mips/generic/init.c +++ b/arch/mips/generic/init.c @@ -110,14 +110,17 @@ void __init plat_mem_setup(void) void __init device_tree_init(void) { - int err; - unflatten_and_copy_device_tree(); mips_cpc_probe(); - err = register_cps_smp_ops(); - if (err) - err = register_up_smp_ops(); + if (!register_cps_smp_ops()) + return; + if (!register_cmp_smp_ops()) + return; + if (!register_vsmp_smp_ops()) + return; + + register_up_smp_ops(); } int __init apply_mips_fdt_fixups(void *fdt_out, size_t fdt_out_size, --- 8< --- Best, Sander
> I've provided a patch below that enables VPE support for RTL839x. Since RTL930x uses the > same CPU architecture, I expect it to also bring up both threads there. Like you note > RTL930x requires a specific clocksource driver, but it should be possible to run your > current code for that on top of this (ammended) series. > > I will respin the series with some small changes, and add that VSMP-support patch. Please > let us know if you can get your reworked IRQ driver and timer code working on top of the > v2. That way, you wouldn't have to take a step back in your development, and we could > continue providing support through upstream. just enabling vsmp in that way is not enough. it looks simple and this is how we started too but you will quickly find out that it will cause lockups and hangs. if you do performance tests in addition the mainline still uses 4kc as cpu architecture, which is simply wrong for anything else but 838x
Hi Sebastian, On Sun, 2021-12-12 at 00:40 +0100, Sebastian Gottschall wrote: > > > I've provided a patch below that enables VPE support for RTL839x. Since RTL930x uses > > the > > same CPU architecture, I expect it to also bring up both threads there. Like you note > > RTL930x requires a specific clocksource driver, but it should be possible to run your > > current code for that on top of this (ammended) series. > > > > I will respin the series with some small changes, and add that VSMP-support patch. > > Please > > let us know if you can get your reworked IRQ driver and timer code working on top of > > the > > v2. That way, you wouldn't have to take a step back in your development, and we could > > continue providing support through upstream. > just enabling vsmp in that way is not enough. it looks simple and this > is how we started too but you will quickly find out that it will cause > lockups and hangs. I have not experienced a single lock-up when booting with this patch, but I also didn't stress-test the machine or even used the switch part. Do you guys have more details on why it locks up, or how exactly these lock-ups can be resolved? > if you do performance tests > in addition the mainline still uses 4kc as cpu architecture, which is > simply wrong for anything else but 838x Wrong or suboptimal? I don't currently experience any obvious issues, using the same toolchain for 8380, 8390 and 9300. Best, Sander
> I have not experienced a single lock-up when booting with this patch, but I also didn't > stress-test the machine or even used the switch part. Do you guys have more details on why > it locks up, or how exactly these lock-ups can be resolved? so far we found out it was related to the ethernet driver. but this is all part of the work we spended into it the last months and recently again since i'm back on this projects >> if you do performance tests >> in addition the mainline still uses 4kc as cpu architecture, which is >> simply wrong for anything else but 838x > Wrong or suboptimal? I don't currently experience any obvious issues, using the same > toolchain for 8380, 8390 and 9300. below suboptimal. i mean it decreases performance in a significant amount and fixing this issue is more than just easy > > Best, > Sander >
Hi Sebastian, On Sun, 2021-12-12 at 12:38 +0100, Sebastian Gottschall wrote: > > > I have not experienced a single lock-up when booting with this patch, but I also > > didn't > > stress-test the machine or even used the switch part. Do you guys have more details on > > why > > it locks up, or how exactly these lock-ups can be resolved? > so far we found out it was related to the ethernet driver. but this is > all part of the work we spended into it > the last months and recently again since i'm back on this projects If it's most likely caused by the ethernet driver, then I don't think complaining about vsmp_smp_ops not being sufficient is appropriate here. We didn't touch the ethernet driver with this patch series, so any of your changes there should still apply. > > > if you do performance tests > > > in addition the mainline still uses 4kc as cpu architecture, which is > > > simply wrong for anything else but 838x > > Wrong or suboptimal? I don't currently experience any obvious issues, using the same > > toolchain for 8380, 8390 and 9300. > below suboptimal. i mean it decreases performance in a significant > amount and fixing this issue is more than just easy If you know since a while that this will be necessary and is an easy win, then please submit a patch upstream to break up the target into the SoC-families. I'd actually love to see things like this being submitted by you or Birger! Generally speaking, I don't see a reason to wait with submitting patches until other parts have stopped breaking. At least then we can all work from a common code base. I obviously prefer submitting upstream where possible, but the networking part will be mostly (or only) OpenWrt for a while still. People could then enjoy a continous stream of improvements in the snapshot builds, and might be encouraged to try writing some code themselves. Best, Sander
Hi, On 12.12.21 20:51, Sander Vanheule wrote: > Hi Sebastian, > > On Sun, 2021-12-12 at 12:38 +0100, Sebastian Gottschall wrote: >> >>> I have not experienced a single lock-up when booting with this patch, but I also >>> didn't >>> stress-test the machine or even used the switch part. Do you guys have more details on >>> why >>> it locks up, or how exactly these lock-ups can be resolved? >> so far we found out it was related to the ethernet driver. but this is >> all part of the work we spended into it >> the last months and recently again since i'm back on this projects > > If it's most likely caused by the ethernet driver, then I don't think complaining about > vsmp_smp_ops not being sufficient is appropriate here. We didn't touch the ethernet driver > with this patch series, so any of your changes there should still apply. It has been mentioned that it is also other parts of the interplay between SMP code, clocksource and IRQ driver, especially for the RTL93xx. What you are proposing will likely make it much harder to tackle this, it might even mean that we have to revert everything back out. > >>>> if you do performance tests >>>> in addition the mainline still uses 4kc as cpu architecture, which is >>>> simply wrong for anything else but 838x >>> Wrong or suboptimal? I don't currently experience any obvious issues, using the same >>> toolchain for 8380, 8390 and 9300. >> below suboptimal. i mean it decreases performance in a significant >> amount and fixing this issue is more than just easy > > If you know since a while that this will be necessary and is an easy win, then please > submit a patch upstream to break up the target into the SoC-families. I'd actually love to > see things like this being submitted by you or Birger! There is a fundamental lack of understanding here. The break-up of the families is for OpenWRT and the way things are being built, including different configurations and code paths for targets. So this is nothing to upstream apart from maybe some additional driver code going there. We are trying to make OWRT do the right thing, talking about upstream is not getting us anywhere. From my point of view it looks like you are trying to pull the realtek architecture in OWRT into a direction that is based on our understanding more than 1 year ago when stuff started to go upstream. Our understanding has fundamentally shifted with SMP and the 93xx architectures. > > Generally speaking, I don't see a reason to wait with submitting patches until other parts > have stopped breaking. At least then we can all work from a common code base. I obviously > prefer submitting upstream where possible, but the networking part will be mostly (or > only) OpenWrt for a while still. People could then enjoy a continous stream of > improvements in the snapshot builds, and might be encouraged to try writing some code > themselves. Indeed. A common code base. And that is moving into a completely different direction than what you propose, based on current knowledge about the targets. If we don't have any possibility to have different code paths plus go for GENERIC_MIPS which is likely the wrong choice, then we essentially cut development off, because that is currently focussing on diversifying the different architectures. The network code is a long way away from going upstream. There is still a lot that is not understood. From fundamentals like SMI to the right architecture of the Ethernet driver in the face of SMP to how the SoC architectures can be put into one overall code-framework. My point of view is that we should not be upstreaming half-baked stuff. But what is even worse in my eyes is when we then take that half-baked stuff a year later and say: oh, but this is the way it needs to be done now even if this no longer has anything to do with our current understanding. My proposal to go forward: please provide a patch to split architectures and show that we can optimize compiler and code paths in the face of MIPS Generic for the 4 SoC families. Cheers, Birger