[RFC PATCH 0/1] Add support for setting Cortex R7 boot address

[RFC PATCH 0/1] Add support for setting Cortex R7 boot address

[Julien Massot]

Hi,
I plan to submit a small driver to remoteproc subsystem in order
to manage the Cortex R7 found on various SoCs of the R-Car Gen3
SoC series.

Enabling this processor from the remoteproc subsystem requires to:
- loading a firmware to a RAM area.
- setting the boot address thanks to CR7BAR register
- enable power domains
- deassert CR7 reset

The goal here is to export a function from rcar-rst.c to allow a
a remoteproc driver to set this boot address.

I'm not so much used to manipulate init data so don't hesitate to
comment.

Thanks !

Julien Massot (1):
  soc: renesas: rcar-rst: Add support to set rproc boot address

 drivers/soc/renesas/rcar-rst.c       | 28 +++++++++++++++++++++++++++-
 include/linux/soc/renesas/rcar-rst.h |  2 ++
 2 files changed, 29 insertions(+), 1 deletion(-)

-- 
2.31.1

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

[Julien Massot]

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>
---
 drivers/soc/renesas/rcar-rst.c       | 28 +++++++++++++++++++++++++++-
 include/linux/soc/renesas/rcar-rst.h |  2 ++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/renesas/rcar-rst.c b/drivers/soc/renesas/rcar-rst.c
index 8a1e402ea799..7f8452b07cdf 100644
--- 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;
+	}
+
+	boot_addr |= CR7BAREN;
+	iowrite32(boot_addr, rcar_rst_base + CR7BAR);
+
+	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__ */
-- 
2.31.1

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

[Geert Uytterhoeven]

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

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

[Julien Massot]

Hi Geert,

>> +
>> +/*
>> + * 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.
Indeed, I should better fix my firmware or my remoteproc driver to give correct input to this
function. will return -EINVAL in case of bad alignment.
> 
>> +       }
>> +
>> +       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.
You're right will fix.
> 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]).
Yes I was also thinking about future generation like Gen4, but I don't have the documentation
at this point.
 From what I understand in [1]: CA7BAR in Gen2 is managed by the SYSC module and not by the RST module
as for CR7BAR in Gen3. So despite the fact that the procedure is similar, we won't be able to set CA7BAR in
rcar-rst.c.

> 
> 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.
Passing BAR offset will imply to spread RST module offsets across different subsystems,
and the second question will be to be able to do the correct boundary check for the targeted
processor: CR7BAR is aligned on 4k an it looks like CA7BAR is on 256k. It looks like it's
manageable thanks to the driver data which already holds the 'mode monitor register' offset per SoC generation.

One missing point will be for future R-Car SoCs: trends on others SoC seems to be to go to have
several 'realtime', or remote processor. In this case this function will not scale up.

Thanks for the review !

-- 
Julien Massot [IoT.bzh]

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

[Geert Uytterhoeven]

Hi Julien,

On Wed, Sep 22, 2021 at 11:54 AM Julien Massot <julien.massot@iot.bzh> wrote:
> > In general, I think this looks like a good abstraction, which we can
> > also use for further abstraction of R-Car Gen2 (cfr. [1]).
> Yes I was also thinking about future generation like Gen4, but I don't have the documentation
> at this point.
>  From what I understand in [1]: CA7BAR in Gen2 is managed by the SYSC module and not by the RST module
> as for CR7BAR in Gen3. So despite the fact that the procedure is similar, we won't be able to set CA7BAR in
> rcar-rst.c.

On R-Car Gen2, CA7BAR is managed by the RST module.
On R-Mobile APE6, it is managed by the SYSC module.

> > 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.
> Passing BAR offset will imply to spread RST module offsets across different subsystems,
> and the second question will be to be able to do the correct boundary check for the targeted
> processor: CR7BAR is aligned on 4k an it looks like CA7BAR is on 256k. It looks like it's
> manageable thanks to the driver data which already holds the 'mode monitor register' offset per SoC generation.
>
> One missing point will be for future R-Car SoCs: trends on others SoC seems to be to go to have
> several 'realtime', or remote processor. In this case this function will not scale up.

On R-Car V3U, CR52BAR is managed by the APMU module.

There's not much we can do about future processors yet ;-)

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