Re: [PATCH] ARM: mach-shmobile: Don't configure ARCH timer if PSCI is enabled

[Julien Grall]

Hi,

On 4/17/19 6:10 PM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> If CONFIG_PSCI is enabled then most likely we are running on
> PSCI-enabled U-Boot which, we assume, has already taken care of
> configuring ARCH timer stuff before switching to non-secure mode.
>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> ---
> A bit of context here...
>
> We are highly interested in Renesas "Stout" board support (r8a7790)
> in Xen hypervisor. The reason is to have fully supported HW for
> performing "OSSTEST" (Xen automatic test system) on ARM32.
>
> To reach that target we need a "generic way" for the secondary CPU cores
> bring up and switching them to non-secure hyp mode.
> So, the PSCI as a generic well-known way to bring up CPUs, was chosen
> for that purpose.
>
> You can find corresponding patches for U-Boot here:
> http://u-boot.10912.n7.nabble.com/PATCH-0-3-PSCI-support-for-r8a7790-SoC-Lager-Stout-boards-td357352.html
>
> You can find corresponding patches for Xen hypervisor here:
> https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxxxxxxxxx/msg43332.html
>
> To sumarize:
> Together with enabling CONFIG_PSCI in shmobile_defconfig, current patch
> is a minimal required change needed to run mainline Linux on top of
> PSCI-enabled U-Boot.
> There is no need to modify device tree. U-Boot will take care of inserting
> proper "enable-method" strings in CPU nodes.
>
> ---
>   arch/arm/mach-shmobile/setup-rcar-gen2.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-shmobile/setup-rcar-gen2.c b/arch/arm/mach-shmobile/setup-rcar-gen2.c
> index eea60b2..bac4490 100644
> --- a/arch/arm/mach-shmobile/setup-rcar-gen2.c
> +++ b/arch/arm/mach-shmobile/setup-rcar-gen2.c
> @@ -32,7 +32,7 @@ static const struct of_device_id cpg_matches[] __initconst = {
>   	{ /* sentinel */ }
>   };
>   
> -static unsigned int __init get_extal_freq(void)
> +static unsigned int __init __maybe_unused get_extal_freq(void)
>   {
>   	const struct of_device_id *match;
>   	struct device_node *cpg, *extal;
> @@ -60,6 +60,12 @@ static unsigned int __init get_extal_freq(void)
>   
>   void __init rcar_gen2_timer_init(void)
>   {
> +/*
> + * If CONFIG_PSCI is enabled then most likely we are running on PSCI-enabled
> + * U-Boot which, we assume, has already taken care of configuring ARCH timer
> + * stuff before switching to non-secure mode.
> + */

I disagree with that statement. CONFIG_ARM_PSCI may be selected in your 
.config for multiple reasons:
     - CONFIG_XEN will selected it
     - you build a .config targeting multiple platforms

Yet you may still want to boot the kernel on older U-boot. Such change 
would make it impossible.

If there are code that should not be called when PSCI is used, then you 
need to make sure the function is not called at runtime.

Cheers,

--
Julien Grall