Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 17 Nov 2019 11:02:14 +0000
From:   Marc Zyngier <maz@kernel.org>
To:     Andreas =?UTF-8?Q?F=C3=A4rber?= <afaerber@suse.de>
Cc:     linux-realtek-soc@lists.infradead.org,
        Russell King <linux@armlinux.org.uk>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 8/8] ARM: realtek: Enable RTD1195 arch timer
Message-ID: <20191117110214.6b160b2e@why>
In-Reply-To: <20191117072109.20402-9-afaerber@suse.de>
References: <20191117072109.20402-1-afaerber@suse.de>
        <20191117072109.20402-9-afaerber@suse.de>
Organization: Approximate
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.
-- 
Jazz is not dead. It just smells funny...