diff mbox

[RFC,10/37] soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2

Message ID 1516903391-30467-11-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fabrizio Castro Jan. 25, 2018, 6:02 p.m. UTC
This patch allows for platform specific quirks as some of the SoC need
further customization for the watchdog to work properly, like for R-Car
Gen2 and for RZ/G.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
---
 drivers/soc/renesas/rcar-rst.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Comments

Sergei Shtylyov Jan. 26, 2018, 8:30 a.m. UTC | #1
Hello!

On 1/25/2018 9:02 PM, Fabrizio Castro wrote:

> This patch allows for platform specific quirks as some of the SoC need
> further customization for the watchdog to work properly, like for R-Car
> Gen2 and for RZ/G.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> ---
>   drivers/soc/renesas/rcar-rst.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/renesas/rcar-rst.c b/drivers/soc/renesas/rcar-rst.c
> index f7a0d54..5f52aea 100644
> --- a/drivers/soc/renesas/rcar-rst.c
> +++ b/drivers/soc/renesas/rcar-rst.c
> @@ -13,8 +13,18 @@
>   #include <linux/of_address.h>
>   #include <linux/soc/renesas/rcar-rst.h>
>   
> +#define WDTRSTCR_RESET		0xA55A0002
> +#define WDTRSTCR		0x0054
> +
> +static int gen2_configuration(void __iomem *base)
> +{
> +	iowrite32(WDTRSTCR_RESET, base + WDTRSTCR);
> +	return 0;
> +}
> +
>   struct rst_config {
> -	unsigned int modemr;	/* Mode Monitoring Register Offset */
> +	unsigned int modemr;		/* Mode Monitoring Register Offset */
> +	int (*configure)(void *base);	/* Platform specific configuration */
>   };
>   
>   static const struct rst_config rcar_rst_gen1 __initconst = {
> @@ -23,6 +33,7 @@ static const struct rst_config rcar_rst_gen1 __initconst = {
>   
>   static const struct rst_config rcar_rst_gen2 __initconst = {
>   	.modemr = 0x60,
> +	.configure = gen2_configuration,

    Why not call the method gen2_configure() then?

[...]

MBR, Sergei
Geert Uytterhoeven Jan. 26, 2018, 10:05 a.m. UTC | #2
Hi Fabrizio,

On Thu, Jan 25, 2018 at 7:02 PM, Fabrizio Castro
<fabrizio.castro@bp.renesas.com> wrote:
> This patch allows for platform specific quirks as some of the SoC need
> further customization for the watchdog to work properly, like for R-Car
> Gen2 and for RZ/G.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/soc/renesas/rcar-rst.c
> +++ b/drivers/soc/renesas/rcar-rst.c
> @@ -13,8 +13,18 @@
>  #include <linux/of_address.h>
>  #include <linux/soc/renesas/rcar-rst.h>
>
> +#define WDTRSTCR_RESET         0xA55A0002
> +#define WDTRSTCR               0x0054
> +
> +static int gen2_configuration(void __iomem *base)

rcar_rst_enable_wdt_reset()?

> +{
> +       iowrite32(WDTRSTCR_RESET, base + WDTRSTCR);
> +       return 0;
> +}
> +
>  struct rst_config {
> -       unsigned int modemr;    /* Mode Monitoring Register Offset */
> +       unsigned int modemr;            /* Mode Monitoring Register Offset */
> +       int (*configure)(void *base);   /* Platform specific configuration */
>  };
>
>  static const struct rst_config rcar_rst_gen1 __initconst = {
> @@ -23,6 +33,7 @@ static const struct rst_config rcar_rst_gen1 __initconst = {
>
>  static const struct rst_config rcar_rst_gen2 __initconst = {
>         .modemr = 0x60,
> +       .configure = gen2_configuration,

BTW, does it hurt to do this on R-Car Gen3, too?

Instead of a function pointer, you could also just have a feature bit.

>  };
>
>  static const struct rst_config rcar_rst_gen3 __initconst = {
> @@ -79,6 +90,14 @@ static int __init rcar_rst_init(void)
>         rcar_rst_base = base;
>         cfg = match->data;
>         saved_mode = ioread32(base + cfg->modemr);
> +       if (cfg->configure) {
> +               error = cfg->configure(base);
> +               if (error) {
> +                       pr_warn("%pOF: Cannot run SoC specific configuration\n",
> +                               np);
> +                       goto out_put;
> +               }
> +       }
>
>         pr_debug("%pOF: MODE = 0x%08x\n", np, saved_mode);

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
Fabrizio Castro Jan. 26, 2018, 4:59 p.m. UTC | #3
Hello Sergei,

thank you for your feedback.

> Subject: Re: [RFC 10/37] soc: renesas: rcar-rst: Enable watchdog as reset trigger for Gen2
>
> Hello!
>
> On 1/25/2018 9:02 PM, Fabrizio Castro wrote:
>
> > This patch allows for platform specific quirks as some of the SoC need
> > further customization for the watchdog to work properly, like for R-Car
> > Gen2 and for RZ/G.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> > ---
> >   drivers/soc/renesas/rcar-rst.c | 21 ++++++++++++++++++++-
> >   1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/renesas/rcar-rst.c b/drivers/soc/renesas/rcar-rst.c
> > index f7a0d54..5f52aea 100644
> > --- a/drivers/soc/renesas/rcar-rst.c
> > +++ b/drivers/soc/renesas/rcar-rst.c
> > @@ -13,8 +13,18 @@
> >   #include <linux/of_address.h>
> >   #include <linux/soc/renesas/rcar-rst.h>
> >
> > +#define WDTRSTCR_RESET0xA55A0002
> > +#define WDTRSTCR0x0054
> > +
> > +static int gen2_configuration(void __iomem *base)
> > +{
> > +iowrite32(WDTRSTCR_RESET, base + WDTRSTCR);
> > +return 0;
> > +}
> > +
> >   struct rst_config {
> > -unsigned int modemr;/* Mode Monitoring Register Offset */
> > +unsigned int modemr;/* Mode Monitoring Register Offset */
> > +int (*configure)(void *base);/* Platform specific configuration */
> >   };
> >
> >   static const struct rst_config rcar_rst_gen1 __initconst = {
> > @@ -23,6 +33,7 @@ static const struct rst_config rcar_rst_gen1 __initconst = {
> >
> >   static const struct rst_config rcar_rst_gen2 __initconst = {
> >   .modemr = 0x60,
> > +.configure = gen2_configuration,
>
>     Why not call the method gen2_configure() then?

Yeah, the name gen2_configuration isn't great!
As you may have noticed already, Geert suggested rcar_rst_enable_wdt_reset.
I am definitely going to change it (if I am keeping the function pointer at all!), expect to see another version of this patch in the next few days to address this.

Thanks,
Fab

>
> [...]
>
> MBR, Sergei



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
diff mbox

Patch

diff --git a/drivers/soc/renesas/rcar-rst.c b/drivers/soc/renesas/rcar-rst.c
index f7a0d54..5f52aea 100644
--- a/drivers/soc/renesas/rcar-rst.c
+++ b/drivers/soc/renesas/rcar-rst.c
@@ -13,8 +13,18 @@ 
 #include <linux/of_address.h>
 #include <linux/soc/renesas/rcar-rst.h>
 
+#define WDTRSTCR_RESET		0xA55A0002
+#define WDTRSTCR		0x0054
+
+static int gen2_configuration(void __iomem *base)
+{
+	iowrite32(WDTRSTCR_RESET, base + WDTRSTCR);
+	return 0;
+}
+
 struct rst_config {
-	unsigned int modemr;	/* Mode Monitoring Register Offset */
+	unsigned int modemr;		/* Mode Monitoring Register Offset */
+	int (*configure)(void *base);	/* Platform specific configuration */
 };
 
 static const struct rst_config rcar_rst_gen1 __initconst = {
@@ -23,6 +33,7 @@  static const struct rst_config rcar_rst_gen1 __initconst = {
 
 static const struct rst_config rcar_rst_gen2 __initconst = {
 	.modemr = 0x60,
+	.configure = gen2_configuration,
 };
 
 static const struct rst_config rcar_rst_gen3 __initconst = {
@@ -79,6 +90,14 @@  static int __init rcar_rst_init(void)
 	rcar_rst_base = base;
 	cfg = match->data;
 	saved_mode = ioread32(base + cfg->modemr);
+	if (cfg->configure) {
+		error = cfg->configure(base);
+		if (error) {
+			pr_warn("%pOF: Cannot run SoC specific configuration\n",
+				np);
+			goto out_put;
+		}
+	}
 
 	pr_debug("%pOF: MODE = 0x%08x\n", np, saved_mode);