Re: [RFC PATCH 1/1] soc: renesas: rcar-rst: Add support to set rproc boot address

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Julien,

On Tue, Sep 14, 2021 at 11:56 AM Julien Massot <julien.massot@iot.bzh> wrote:
> R-Car Gen3 SoC series has a realtime processor, the boot
> address of this processor can be set thanks to CR7BAR register
> of the reset module.
>
> Export this function so that it's possible to set the boot
> address from a remoteproc driver.
>
> Also drop the __initdata qualifier on rcar_rst_base,
> since we will use this address later than init time.
>
> Signed-off-by: Julien Massot <julien.massot@iot.bzh>

Thanks for your patch!

> --- a/drivers/soc/renesas/rcar-rst.c
> +++ b/drivers/soc/renesas/rcar-rst.c
> @@ -12,6 +12,8 @@
>
>  #define WDTRSTCR_RESET         0xA55A0002
>  #define WDTRSTCR               0x0054
> +#define CR7BAR                 0x0070
> +#define CR7BAREN               BIT(4)
>
>  static int rcar_rst_enable_wdt_reset(void __iomem *base)
>  {
> @@ -76,7 +78,7 @@ static const struct of_device_id rcar_rst_matches[] __initconst = {
>         { /* sentinel */ }
>  };
>
> -static void __iomem *rcar_rst_base __initdata;
> +static void __iomem *rcar_rst_base;
>  static u32 saved_mode __initdata;
>
>  static int __init rcar_rst_init(void)
> @@ -130,3 +132,27 @@ int __init rcar_rst_read_mode_pins(u32 *mode)
>         *mode = saved_mode;
>         return 0;
>  }
> +
> +/*
> + * Most of R-Car Gen3 SoCs have an ARM Realtime Core.
> + * Firmware boot address can be set before starting,
> + * the realtime core thanks to CR7BAR register.
> + * Boot address is on 32bit, and should be aligned on
> + * 4k boundary.
> + */
> +int rcar_rst_set_rproc_boot_addr(u32 boot_addr)
> +{
> +       if (!rcar_rst_base)
> +               return -EIO;
> +
> +       if (boot_addr % SZ_4K) {
> +               pr_warn("Invalid boot address for remote processor, should be aligned on 4k\n");
> +               boot_addr -= boot_addr % SZ_4K;

I think it would be safer to just return -EINVAL.

> +       }
> +
> +       boot_addr |= CR7BAREN;
> +       iowrite32(boot_addr, rcar_rst_base + CR7BAR);

According to Note 2 for the CR7BAR register, you must do this in two steps,
first without CR7BAREN set, then with CR7BAREN set.
See also how CA7BAR and CA15BAR are handled in
arch/arm/mach-shmobile/pm-rcar-gen2.c.

Note that CA15/CA7 on R-Car Gen2 (and CA57/CA53 on Gen3, but
that's hidden by ACPI), unlike CR7, also need RESCNT handling.

> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(rcar_rst_set_rproc_boot_addr);
> diff --git a/include/linux/soc/renesas/rcar-rst.h b/include/linux/soc/renesas/rcar-rst.h
> index 7899a5b8c247..7c97c2c4bba6 100644
> --- a/include/linux/soc/renesas/rcar-rst.h
> +++ b/include/linux/soc/renesas/rcar-rst.h
> @@ -4,8 +4,10 @@
>
>  #ifdef CONFIG_RST_RCAR
>  int rcar_rst_read_mode_pins(u32 *mode);
> +int rcar_rst_set_rproc_boot_addr(u32 boot_addr);
>  #else
>  static inline int rcar_rst_read_mode_pins(u32 *mode) { return -ENODEV; }
> +static inline int rcar_rst_set_rproc_boot_addr(u32 boot_addr) { return -ENODEV; }
>  #endif
>
>  #endif /* __LINUX_SOC_RENESAS_RCAR_RST_H__ */

In general, I think this looks like a good abstraction, which we can
also use for further abstraction of R-Car Gen2 (cfr. [1]).

I'm just wondering if we should pass the BAR offset to
rcar_rst_set_rproc_boot_addr() explicitly (and rename the function),
or have multiple functions for the different BARs.

Comments?

[1] "[PATCH/RFC 0/6] ARM: r8a73a4: Add SMP support"
    https://lore.kernel.org/linux-renesas-soc/20210204135409.1652604-1-geert+renesas@glider.be/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds