[v3,8/8] ARM: realtek: Enable RTD1195 arch timer
diff mbox series

Message ID 20191117072109.20402-9-afaerber@suse.de
State New
Headers show
Series
  • ARM: Initial RTD1195 and MeLE X1000 support
Related show

Commit Message

Andreas Färber Nov. 17, 2019, 7:21 a.m. UTC
Without this magic write the timer doesn't work and boot gets stuck.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 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?
 
 v2 -> v3: Unchanged
 
 v2: New
 
 arch/arm/mach-realtek/rtd1195.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Marc Zyngier Nov. 17, 2019, 11:02 a.m. UTC | #1
On Sun, 17 Nov 2019 08:21:09 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Without this magic write the timer doesn't work and boot gets stuck.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  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?
>  
>  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?

	M.
Andreas Färber Nov. 17, 2019, 5:08 p.m. UTC | #2
Am 17.11.19 um 12:02 schrieb Marc Zyngier:
> On Sun, 17 Nov 2019 08:21:09 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Without this magic write the timer doesn't work and boot gets stuck.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  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?

    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

>>  
>>  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.

v2 did not get any review from Realtek, so for this v3 I explicitly
spelled out my register questions above, in case the term "magic" was
not enough to prompt an actual explanation of what this is doing...

Only change that I can apply right now will be to turn this writel()
into a writel_relaxed(). Tested OK.

Original one-line BSP code:
https://github.com/BPI-SINOVOIP/BPI-M4-bsp/blob/master/linux-rtk/arch/arm/mach-rtd119x/rtd119x.c#L105

In my testing, all I can tell is that on both X1000 and Horseradish the
register value is 0x0 before above BSP-inspired write of 0x1.
So BIT(0) might be a clock gate enable or a reset deassert, or it might
be a larger field meaning something else.

For now, my small Busybox initrd is not capable of testing kexec, but I
don't foresee a problem here, given the observed register values.
(Unlike RTD1295, RTD1195 does not have native AHCI support for rootfs.)

Given the odd location of this register right after GICV rather than on
their r-bus, can you rule out that this is some standard Arm register?

Note that this patch is intentionally separate and last in this series,
precisely due to this expected contentious discussion. :) But as there
seems no realistic chance of anyone implementing a new U-Boot anytime
soon, we'll have to live with some such ugly solution to unblock boot.


Slightly related to this .init_time hook, I am facing an issue for SMP
where my ioremap() fails in .smp_init_cpus if I don't implement a
.map_io hook here, providing an old-style fixed mapping for r-bus. Later
ioremap()s in actual drivers in that same r-bus space do succeed.

And for the record, RTD1295 and RTD1395 still don't have SMP in mainline
either because they're not implementing it via PSCI; RTD1619 appears to
be the first to do that in BL31. No public BL31 code [1] that we might
fix, nor any public documentation on how we might experimentally replace
BL31 with one written from scratch, so I'm carrying non-upstreamable
patches (marked "HACK:") hacking up arm64 spin-table to use different
addresses and widths to bring them up. :/

Regards,
Andreas

[1] https://github.com/ARM-software/arm-trusted-firmware/tree/master/plat
Marc Zyngier Nov. 18, 2019, 9:27 a.m. UTC | #3
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@suse.de> wrote:
>>
>>> Without this magic write the timer doesn't work and boot gets 
>>> stuck.
>>>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>>  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.
Andreas Färber Nov. 18, 2019, 10:48 p.m. UTC | #4
Hi Marc,

Am 18.11.19 um 10:27 schrieb 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@suse.de> wrote:
>>>
>>>> Without this magic write the timer doesn't work and boot gets stuck.
>>>>
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>>  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?

No, I was pointing out that I myself had already asked pretty much the
same questions you just asked me. How did you expect me to have answers
to your "Shouldn't this be a read/modify/write sequence?" then? It
seemed like you missed my questions up there:

Without knowing how the register is structured, I can't implement a
read/modify/write sequence - for that we'd need to know whether it's a
single bit we can just set or a field that we would need to mask first
before writing into it.

And ideally we would be getting its address from DT, so we would need to
know whether it's just this one register or some contiguous IP block
such as "arm,armv7-timer-mem". Same issue whether we implement it in the
kernel or in U-Boot btw. If it's no standard Arm register, we'll need to
have a binding with a sensible compatible string, some REG_ constant to
replace the magic offset number and some meaningful constant for 0x1.

>>>> 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
[...]
>>>> @@ -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. 

Dispute that with U-Boot maintainers then, please, and let me know the
outcome. I just had an argument with Neil that it's not okay to import
from linux-next.git and that it must be an -rc1 or other stable tag. (My
VIM3 patch submission before Plumbers was not accepted for that reason,
and I haven't found time for U-Boot since.)

> If you're able to support all of this in the
> kernel,

No, I'm not, that was my point! :( We do _not_ have all those drivers
needed for a useful U-Boot implemented for the mainline kernel yet. Like
I said, I'm using a Busybox initrd for testing RTD1195 (also RTD1293,
RTD1296 and RTD1395). And I've already amassed 140+ patches on top of
-next without those core drivers - I really need to get that queue down
for sensible collaboration with Realtek and others.

I need people to be able to help contribute those drivers to the kernel,
therefore I need to get the basics merged, so that Realtek and community
members can base patches on top, so that we get there over time.
If you're asking that someone do a new U-Boot before we can even get a
timer or GIC DT node merged, then we're doomed and I'm wasting my time
here. Rob did give a Reviewed-by for the DT, so I would've thought it
can't be that terrible!

As I pointed out above, we need to get the .dts[i] into some -rc1 kernel
release for U-Boot to pull them. If you're blocking to merge RTD1195
into v5.6-rc1 here, we're not gonna get even a driver-less mainline
U-Boot for it in about 8 weeks. Chicken-and-egg problem.

And my experience with U-Boot has been that if U-Boot patches don't get
merged immediately, other people will duplicate that work later without
crediting me, so that my time spent on U-Boot would be wasted. And the
same is happening with the kernel if we don't get my patches merged in
some form - you already witnessed Aleix doing his own irqchip driver
instead of collaborating on the one that went through three rounds of
review already (and now he's disappeared again without input for me, so
I'm preparing to send you a v4 tonight or tomorrow). STM32 was another
bad experience (and I still have two further v7-M ports to clean up).

With the GeekBox, that I'm the official U-Boot maintainer of, I managed
to shoot myself in the foot, unable to recover from a U-Boot I flashed.
So I am very hesitant to repeat that, especially with RTD1195 hardware
that's no longer sold (X1000).

> 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.

And the work of the sunXi community - including Bootlin and Arm people -
is commendable and inspiring (e.g., meson-tools).

My time as single person is limited for this night-time project though,
so it's unrealistic to ask me to reverse-engineer a new BL31 without
sources, write a U-Boot implementation and port the kernel, all at once.
Especially when I don't know how to even test those other components.
Maybe that's clear to you - for me it isn't. The RTD1395, where I do
have U-Boot sources for BPi-M4 [2] and get it loaded as file from SD by
a previous bootloader, is not affected by this RTD1195 timer problem;
yet I don't see how from any given U-Boot (or LK) I would be able to
load a new BL31 on RTD1295/RTD1395, nor how to load a HYP capable U-Boot
on RTD1195. I thought it's impossible to escape to higher ELs? And for
ARMv7 I thought U-Boot was neither relocatable not reentrant?

Now, I can continue carrying this commit in my GitHub tree for myself,
like I do for spin-table, if Olof and Arnd agree with you that this 8/8
should not be merged, even if we clarify this register. But I just don't
see the time right now for doing alone what the sunxi community have
achieved together over the course of many years, with Realtek in my neck
for new kernel patches.

The context here is that most of the drivers seen on RTD1295 and later
originated in RTD1195, so for sane compatible strings (cf. watchdog [3],
rtc [4] discussions!) we need to be able to test RTD1195 and to base
patches, such as upcoming irqchip mux v4, on all four+ SoC families.
Realtek appears to be focused on the new RTD1619/RTD1319, and I would
really like to avoid merging drivers for those new chips only and caring
about bindings and compatibility with the three earlier SoC families as
afterthought, repeating the mistakes I made with RTD1295 vs. RTD1195 for
lack of hardware at the time.

I don't think that I'm pushing into a wrong direction; we just disagree
about the steps to get there. I would like people to be able to take a
mainline kernel and boot on RTD1195 without getting stuck; if they have
to use a GitHub tree to successfully boot, then it's not really mainline
yet. Booting with only one CPU up is much less severe by comparison, as
it allows to probe all drivers and to explore sysfs and debugfs.

Keep in mind that I started RTD1295 mainlining without having any BSP
sources at all, which is less than the sunxi folks had to go on.

The key on sunxi, I thought, was that you had the FEL tools to just load
bootloader binaries dynamically over USB. For Amlogic we at least had
binary blobs and binary tools integrated into U-Boot build process and
an idea where to place them on SD to boot them - here I have only what
is on eMMC/SPI, no docs. Also, I do recall a long time where sunxi had
GitHub repos with downstream BSP forks - I'd rather not get into that
business and immediately skip to making mainline more and more usable.

If of course Realtek or one of their OEMs or some other community member
wanted to work on providing a rebased U-Boot in a Git repo, that would
be great. Once we have it working, we could drop workarounds like these.

A quick git-grep finds 277 occurrences of .init_time in arch/arm/mach-*,
so hardly a callback about to get dropped.

Regards,
Andreas

[1] https://github.com/BPI-SINOVOIP/BPI-W2-bsp/tree/master/u-boot-rtk
[2] https://github.com/BPI-SINOVOIP/BPI-M4-bsp/tree/master/u-boot-rtk
[3] https://patchwork.kernel.org/patch/11200563/
[4] https://patchwork.kernel.org/patch/11200561/

Patch
diff mbox series

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();
+}
+
 static const char *const rtd1195_dt_compat[] __initconst = {
 	"realtek,rtd1195",
 	NULL
@@ -31,6 +46,7 @@  static const char *const rtd1195_dt_compat[] __initconst = {
 
 DT_MACHINE_START(rtd1195, "Realtek RTD1195")
 	.dt_compat = rtd1195_dt_compat,
+	.init_time = rtd1195_init_time,
 	.reserve = rtd1195_reserve,
 	.l2c_aux_val = 0x0,
 	.l2c_aux_mask = ~0x0,