[PATCH 00/13] Switch realtek target to upstream platform

[Sander Vanheule]

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