Re: [PATCH v3 8/8] ARM: realtek: Enable RTD1195 arch timer

[Marc Zyngier]

On 2019-11-17 17:08, Andreas FÃrber wrote:
> Am 17.11.19 um 12:02 schrieb Marc Zyngier:
> > On Sun, 17 Nov 2019 08:21:09 +0100
> > Andreas FÃrber <afaerber@xxxxxxx> wrote:
> >
> > > Without this magic write the timer doesn't work and boot gets 
> > > stuck.
> > >
> > > Signed-off-by: Andreas FÃrber <afaerber@xxxxxxx>
> > > ---
> > >  What is the name of the register 0xff018000?
> > >  Is 0x1 a BIT(0) write, or how are the register bits defined?
> > >  Is this a reset or a clock gate? How should we model it in DT?
>
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Sorry? Do you expect me to come up with answer to these questions?

> > >  v2 -> v3: Unchanged
> > >
> > >  v2: New
> > >
> > >  arch/arm/mach-realtek/rtd1195.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/arch/arm/mach-realtek/rtd1195.c 
> > > b/arch/arm/mach-realtek/rtd1195.c
> > > index b31a4066be87..0532379c74f5 100644
> > > --- a/arch/arm/mach-realtek/rtd1195.c
> > > +++ b/arch/arm/mach-realtek/rtd1195.c
> > > @@ -5,6 +5,9 @@
> > >   * Copyright (c) 2017-2019 Andreas FÃrber
> > >   */
> > >
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/clocksource.h>
> > > +#include <linux/io.h>
> > >  #include <linux/memblock.h>
> > >  #include <asm/mach/arch.h>
> > >
> > > @@ -24,6 +27,18 @@ static void __init rtd1195_reserve(void)
> > >  	rtd1195_memblock_remove(0x18100000, 0x01000000);
> > >  }
> > >
> > > +static void __init rtd1195_init_time(void)
> > > +{
> > > +	void __iomem *base;
> > > +
> > > +	base = ioremap(0xff018000, 4);
> > > +	writel(0x1, base);
> > > +	iounmap(base);
> > > +
> > > +	of_clk_init(NULL);
> > > +	timer_probe();
> > > +}
> >
> > Gawd... Why isn't this set from the bootloader? By the time the 
> > kernel
> > starts, everything should be up and running. What is it going to do
> > when you kexec? Shouldn't this be a read/modify/write sequence?
>
> Again, I can't comment on why their BSP bootloaders don't do things 
> the
> expected way. The list of issues is long, and the newest U-Boot I've
> seen for RTD1395 was v2015.07 based, still downstream and pre-EBBR.
> And before we get a .dts merged into the kernel with all needed nodes
> (network, eMMC, etc.), there is zero chance of a mainline U-Boot 
> anyway.

[...]

I certainly dispute that. If you're able to support all of this in the
kernel, you're most probably able to do it in u-boot, and that's the 
right
place to do it (and that can be a pretty simple u-boot if you use the
original one as a first-stage loader).

I'm not trying to undermine your effort in supporting these platforms
in mainline. This is certainly commendable. But you're unfortunately
pushing in a direction that we've tried hard to move away from for
a good 8 years.

Look at what we did on the Allwinner front: we got a terrible piece of
crap, and turned it into one of the best supported platform, because
we've put the effort where it mattered. I personally wrote PSCI support
and HYP enablement in u-boot, addressing in one go most of the issues 
you
mention here. It didn't take that much time, and it is now there for 
you
to make use of.

Thanks,

        M.
--
Jazz is not dead. It just smells funny...