| Message ID | 20210922145212.333541-1-julien.massot@iot.bzh (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] soc: renesas: rcar-rst: Add support to set rproc boot address | expand |
Hi, > +/* > + * 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_gen3_rproc_boot_addr(u32 boot_addr) > +{ > + if (boot_addr % SZ_4K) { > + pr_warn("Invalid boot address for CR7 processor," > + "should be aligned on 4k got %x\n", boot_addr); > + return -EINVAL; > + } Just notice that this 4K value is wrong, CR7BAR should be aligned on 256K. Any comments about this v2 before I send an update ? Regards,
Hi Julien, On Wed, Sep 22, 2021 at 4:52 PM 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> > --- > > Change since RFC: > Introduce set_rproc_boot_addr function pointer, so that > it can be reused for other R-Car SoC generation. Thanks for the update! > --- 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) > { > @@ -19,25 +21,29 @@ static int rcar_rst_enable_wdt_reset(void __iomem *base) > return 0; > } > > +static int rcar_rst_set_gen3_rproc_boot_addr(u32 boot_addr); I think you can avoid this forward declaration, by reordering definitions. > + > struct rst_config { > unsigned int modemr; /* Mode Monitoring Register Offset */ > int (*configure)(void __iomem *base); /* Platform specific config */ > + int (*set_rproc_boot_addr)(u32 boot_addr); > }; > > -static const struct rst_config rcar_rst_gen1 __initconst = { > +static const struct rst_config rcar_rst_gen1 = { > .modemr = 0x20, > }; > > -static const struct rst_config rcar_rst_gen2 __initconst = { > +static const struct rst_config rcar_rst_gen2 = { > .modemr = 0x60, > .configure = rcar_rst_enable_wdt_reset, > }; > > -static const struct rst_config rcar_rst_gen3 __initconst = { > +static const struct rst_config rcar_rst_gen3 = { > .modemr = 0x60, > + .set_rproc_boot_addr = rcar_rst_set_gen3_rproc_boot_addr, > }; > > -static const struct rst_config rcar_rst_r8a779a0 __initconst = { > +static const struct rst_config rcar_rst_r8a779a0 = { > .modemr = 0x00, /* MODEMR0 and it has CPG related bits */ > }; I prefer to keep these in __init, as there are multiple instances. If you need to access some fields later, just make non-__init copies during initialization. > > @@ -76,13 +82,13 @@ 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 const struct rst_config *cfg; You don't need a pointer to the whole config struct, just a function pointer: static int int (*rcar_rst_set_rproc_boot_addr)(u32 boot_addr); > @@ -130,3 +136,33 @@ 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. That sentence sounds weird to me. > + * Boot address is on 32bit, and should be aligned on > + * 4k boundary. 256 KiB > + */ > +int rcar_rst_set_gen3_rproc_boot_addr(u32 boot_addr) Missing static? > +{ > + if (boot_addr % SZ_4K) { SZ_256K, as noted in your follow-up. > + pr_warn("Invalid boot address for CR7 processor," > + "should be aligned on 4k got %x\n", boot_addr); 256 KiB > + return -EINVAL; > + } > + > + iowrite32(boot_addr, rcar_rst_base + CR7BAR); > + iowrite32(boot_addr | CR7BAREN, rcar_rst_base + CR7BAR); > + > + return 0; > +} > + > +int rcar_rst_set_rproc_boot_addr(u32 boot_addr) > +{ > + if (!rcar_rst_base || !cfg->set_rproc_boot_addr) This can just check the rcar_rst_set_rproc_boot_addr pointer. > + return -EIO; > + > + return cfg->set_rproc_boot_addr(boot_addr); > +} > +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__ */ Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the review ! All the point will be addressed in v3. >> +static int rcar_rst_set_gen3_rproc_boot_addr(u32 boot_addr); > > I think you can avoid this forward declaration, by reordering definitions. Indeed I will reorder the definition. >> -static const struct rst_config rcar_rst_gen1 __initconst = { >> +static const struct rst_config rcar_rst_gen1 = { >> .modemr = 0x20, >> }; >> >> -static const struct rst_config rcar_rst_gen2 __initconst = { >> +static const struct rst_config rcar_rst_gen2 = { >> .modemr = 0x60, >> .configure = rcar_rst_enable_wdt_reset, >> }; >> >> -static const struct rst_config rcar_rst_gen3 __initconst = { >> +static const struct rst_config rcar_rst_gen3 = { >> .modemr = 0x60, >> + .set_rproc_boot_addr = rcar_rst_set_gen3_rproc_boot_addr, >> }; >> >> -static const struct rst_config rcar_rst_r8a779a0 __initconst = { >> +static const struct rst_config rcar_rst_r8a779a0 = { >> .modemr = 0x00, /* MODEMR0 and it has CPG related bits */ >> }; > > I prefer to keep these in __init, as there are multiple instances. > If you need to access some fields later, just make non-__init copies > during initialization. Ok > >> >> @@ -76,13 +82,13 @@ 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 const struct rst_config *cfg; > > You don't need a pointer to the whole config struct, just a function pointer: Will replace by a function pointer. > > static int int (*rcar_rst_set_rproc_boot_addr)(u32 boot_addr); > >> @@ -130,3 +136,33 @@ 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. > > That sentence sounds weird to me. Ok will improve this comment. > >> + * Boot address is on 32bit, and should be aligned on >> + * 4k boundary. > > 256 KiB Ok > >> + */ >> +int rcar_rst_set_gen3_rproc_boot_addr(u32 boot_addr) > > Missing static? Yes will be fixed in v3. > >> +{ >> + if (boot_addr % SZ_4K) { > > SZ_256K, as noted in your follow-up. > >> + pr_warn("Invalid boot address for CR7 processor," >> + "should be aligned on 4k got %x\n", boot_addr); > > 256 KiB ok > >> + return -EINVAL; >> + } >> + >> + iowrite32(boot_addr, rcar_rst_base + CR7BAR); >> + iowrite32(boot_addr | CR7BAREN, rcar_rst_base + CR7BAR); >> + >> + return 0; >> +} >> + >> +int rcar_rst_set_rproc_boot_addr(u32 boot_addr) >> +{ >> + if (!rcar_rst_base || !cfg->set_rproc_boot_addr) > > This can just check the rcar_rst_set_rproc_boot_addr pointer. Ok Regards,
diff --git a/drivers/soc/renesas/rcar-rst.c b/drivers/soc/renesas/rcar-rst.c index 8a1e402ea799..49200cdfe633 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) { @@ -19,25 +21,29 @@ static int rcar_rst_enable_wdt_reset(void __iomem *base) return 0; } +static int rcar_rst_set_gen3_rproc_boot_addr(u32 boot_addr); + struct rst_config { unsigned int modemr; /* Mode Monitoring Register Offset */ int (*configure)(void __iomem *base); /* Platform specific config */ + int (*set_rproc_boot_addr)(u32 boot_addr); }; -static const struct rst_config rcar_rst_gen1 __initconst = { +static const struct rst_config rcar_rst_gen1 = { .modemr = 0x20, }; -static const struct rst_config rcar_rst_gen2 __initconst = { +static const struct rst_config rcar_rst_gen2 = { .modemr = 0x60, .configure = rcar_rst_enable_wdt_reset, }; -static const struct rst_config rcar_rst_gen3 __initconst = { +static const struct rst_config rcar_rst_gen3 = { .modemr = 0x60, + .set_rproc_boot_addr = rcar_rst_set_gen3_rproc_boot_addr, }; -static const struct rst_config rcar_rst_r8a779a0 __initconst = { +static const struct rst_config rcar_rst_r8a779a0 = { .modemr = 0x00, /* MODEMR0 and it has CPG related bits */ }; @@ -76,13 +82,13 @@ 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 const struct rst_config *cfg; static int __init rcar_rst_init(void) { const struct of_device_id *match; - const struct rst_config *cfg; struct device_node *np; void __iomem *base; int error = 0; @@ -130,3 +136,33 @@ 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_gen3_rproc_boot_addr(u32 boot_addr) +{ + if (boot_addr % SZ_4K) { + pr_warn("Invalid boot address for CR7 processor," + "should be aligned on 4k got %x\n", boot_addr); + return -EINVAL; + } + + iowrite32(boot_addr, rcar_rst_base + CR7BAR); + iowrite32(boot_addr | CR7BAREN, rcar_rst_base + CR7BAR); + + return 0; +} + +int rcar_rst_set_rproc_boot_addr(u32 boot_addr) +{ + if (!rcar_rst_base || !cfg->set_rproc_boot_addr) + return -EIO; + + return cfg->set_rproc_boot_addr(boot_addr); +} +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__ */
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> --- Change since RFC: Introduce set_rproc_boot_addr function pointer, so that it can be reused for other R-Car SoC generation. --- drivers/soc/renesas/rcar-rst.c | 48 ++++++++++++++++++++++++---- include/linux/soc/renesas/rcar-rst.h | 2 ++ 2 files changed, 44 insertions(+), 6 deletions(-)