[PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Simon Horman]

Boot mode register driver for R-Car Gen2.

If running on a supported platform it reads the boot mode register and
records it using the boot mode register infrastructure established by an
earlier patch.

rcar_gen2_init_boot_mode() is exported allow it to be explicitly called in
cases where the boot mode register is needed before init calls are made.

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/misc/boot-mode-reg/Kconfig     |  8 +++++
 drivers/misc/boot-mode-reg/Makefile    |  1 +
 drivers/misc/boot-mode-reg/rcar-gen2.c | 61 ++++++++++++++++++++++++++++++++++
 include/misc/boot-mode-reg.h           |  3 ++
 4 files changed, 73 insertions(+)
 create mode 100644 drivers/misc/boot-mode-reg/rcar-gen2.c

diff --git a/drivers/misc/boot-mode-reg/Kconfig b/drivers/misc/boot-mode-reg/Kconfig
index 806eba24238f..4731edf8a9db 100644
--- a/drivers/misc/boot-mode-reg/Kconfig
+++ b/drivers/misc/boot-mode-reg/Kconfig
@@ -9,3 +9,11 @@ config BOOT_MODE_REG_CORE
 	help
 	  Say Y here to allow support for drivers to read boot mode
 	  registers and make the value available to other subsystems.
+
+config BOOT_MODE_REG_RCAR_GEN2
+	tristate "Boot Mode Register Driver for Renesas R-Car Gen2 SoCs"
+	default n
+	select BOOT_MODE_REG_CORE
+	help
+	  Say Y here to allow support for reading the boot mode register
+	  on Renesas R-Car Gen2 SoCs.
diff --git a/drivers/misc/boot-mode-reg/Makefile b/drivers/misc/boot-mode-reg/Makefile
index 19134b20a7f1..d097fd0164aa 100644
--- a/drivers/misc/boot-mode-reg/Makefile
+++ b/drivers/misc/boot-mode-reg/Makefile
@@ -4,3 +4,4 @@
 #
 
 obj-$(CONFIG_BOOT_MODE_REG_CORE)	+= core.o
+obj-$(CONFIG_BOOT_MODE_REG_RCAR_GEN2)	+= rcar-gen2.o
diff --git a/drivers/misc/boot-mode-reg/rcar-gen2.c b/drivers/misc/boot-mode-reg/rcar-gen2.c
new file mode 100644
index 000000000000..0f1a06fcf094
--- /dev/null
+++ b/drivers/misc/boot-mode-reg/rcar-gen2.c
@@ -0,0 +1,61 @@
+/*
+ * R-Car Gen2 Boot Mode Register Driver
+ *
+ * Copyright (C) 2015 Simon Horman
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include <misc/boot-mode-reg.h>
+
+#define MODEMR 0xe6160060
+
+static int __init rcar_gen2_read_mode_pins(void)
+{
+	void __iomem *modemr;
+	int err = -ENOMEM;
+	static u32 mode;
+
+	modemr = ioremap_nocache(MODEMR, 4);
+	if (!modemr) {
+		pr_err("failed to map boot mode register");
+		goto err;
+	}
+	mode = ioread32(modemr);
+	iounmap(modemr);
+
+	err = boot_mode_reg_set(mode);
+err:
+	if (err)
+		pr_err("failed to initialise boot mode");
+	return err;
+}
+
+int __init rcar_gen2_init_boot_mode(void)
+{
+	if (of_machine_is_compatible("renesas,r8a7790") ||
+	    of_machine_is_compatible("renesas,r8a7791") ||
+	    of_machine_is_compatible("renesas,r8a7792") ||
+	    of_machine_is_compatible("renesas,r8a7793") ||
+	    of_machine_is_compatible("renesas,r8a7794"))
+		return rcar_gen2_read_mode_pins();
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(boot_mode_set);
+early_initcall(rcar_gen2_init_boot_mode);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Simon Horman <horms@verge.net.au>");
+MODULE_DESCRIPTION("R-Car Gen2 Boot Mode Register Driver");
diff --git a/include/misc/boot-mode-reg.h b/include/misc/boot-mode-reg.h
index 34ee653279a4..f8fea0ea5a3e 100644
--- a/include/misc/boot-mode-reg.h
+++ b/include/misc/boot-mode-reg.h
@@ -21,4 +21,7 @@
 int boot_mode_reg_get(u32 *mode);
 int boot_mode_reg_set(u32 mode);
 
+/* Allow explicit initialisation before initcalls */
+int rcar_gen2_init_boot_mode(void);
+
 #endif
-- 
2.1.4

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Geert Uytterhoeven]

On Thu, Oct 15, 2015 at 8:59 AM, Simon Horman
<horms+renesas@verge.net.au> wrote:
> --- /dev/null
> +++ b/drivers/misc/boot-mode-reg/rcar-gen2.c
> @@ -0,0 +1,61 @@
> +/*
> + * R-Car Gen2 Boot Mode Register Driver

> +#define MODEMR 0xe6160060

Shouldn't this come from DT?

> +static int __init rcar_gen2_read_mode_pins(void)
> +{
> +       void __iomem *modemr;
> +       int err = -ENOMEM;
> +       static u32 mode;
> +
> +       modemr = ioremap_nocache(MODEMR, 4);
> +       if (!modemr) {
> +               pr_err("failed to map boot mode register");
> +               goto err;
> +       }
> +       mode = ioread32(modemr);
> +       iounmap(modemr);
> +
> +       err = boot_mode_reg_set(mode);
> +err:
> +       if (err)
> +               pr_err("failed to initialise boot mode");
> +       return err;
> +}

> --- a/include/misc/boot-mode-reg.h
> +++ b/include/misc/boot-mode-reg.h
> @@ -21,4 +21,7 @@
>  int boot_mode_reg_get(u32 *mode);
>  int boot_mode_reg_set(u32 mode);
>
> +/* Allow explicit initialisation before initcalls */
> +int rcar_gen2_init_boot_mode(void);

When using explicit initialisation before initcalls, the second call will
trigger a new ioremap/ioread32/iounmap cycle.

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: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Khiem Nguyen]

Hi Simon,

Thanks for your patch.

On 10/15/2015 1:59 PM, Simon Horman wrote:
> Boot mode register driver for R-Car Gen2.
>
> If running on a supported platform it reads the boot mode register and
> records it using the boot mode register infrastructure established by an
> earlier patch.
>
> rcar_gen2_init_boot_mode() is exported allow it to be explicitly called in
> cases where the boot mode register is needed before init calls are made.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>   drivers/misc/boot-mode-reg/Kconfig     |  8 +++++
>   drivers/misc/boot-mode-reg/Makefile    |  1 +
>   drivers/misc/boot-mode-reg/rcar-gen2.c | 61 ++++++++++++++++++++++++++++++++++
>   include/misc/boot-mode-reg.h           |  3 ++
>   4 files changed, 73 insertions(+)
>   create mode 100644 drivers/misc/boot-mode-reg/rcar-gen2.c
>
> diff --git a/drivers/misc/boot-mode-reg/Kconfig b/drivers/misc/boot-mode-reg/Kconfig
> index 806eba24238f..4731edf8a9db 100644
> --- a/drivers/misc/boot-mode-reg/Kconfig
> +++ b/drivers/misc/boot-mode-reg/Kconfig
> @@ -9,3 +9,11 @@ config BOOT_MODE_REG_CORE
>   	help
>   	  Say Y here to allow support for drivers to read boot mode
>   	  registers and make the value available to other subsystems.
> +
> +config BOOT_MODE_REG_RCAR_GEN2
> +	tristate "Boot Mode Register Driver for Renesas R-Car Gen2 SoCs"
> +	default n
> +	select BOOT_MODE_REG_CORE
> +	help
> +	  Say Y here to allow support for reading the boot mode register
> +	  on Renesas R-Car Gen2 SoCs.
> diff --git a/drivers/misc/boot-mode-reg/Makefile b/drivers/misc/boot-mode-reg/Makefile
> index 19134b20a7f1..d097fd0164aa 100644
> --- a/drivers/misc/boot-mode-reg/Makefile
> +++ b/drivers/misc/boot-mode-reg/Makefile
> @@ -4,3 +4,4 @@
>   #
>
>   obj-$(CONFIG_BOOT_MODE_REG_CORE)	+= core.o
> +obj-$(CONFIG_BOOT_MODE_REG_RCAR_GEN2)	+= rcar-gen2.o
> diff --git a/drivers/misc/boot-mode-reg/rcar-gen2.c b/drivers/misc/boot-mode-reg/rcar-gen2.c
> new file mode 100644
> index 000000000000..0f1a06fcf094
> --- /dev/null
> +++ b/drivers/misc/boot-mode-reg/rcar-gen2.c
> @@ -0,0 +1,61 @@
> +/*
> + * R-Car Gen2 Boot Mode Register Driver
> + *
> + * Copyright (C) 2015 Simon Horman
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#include <misc/boot-mode-reg.h>
> +
> +#define MODEMR 0xe6160060
> +
> +static int __init rcar_gen2_read_mode_pins(void)
> +{
> +	void __iomem *modemr;
> +	int err = -ENOMEM;
> +	static u32 mode;
> +
> +	modemr = ioremap_nocache(MODEMR, 4);
> +	if (!modemr) {
> +		pr_err("failed to map boot mode register");
> +		goto err;
> +	}
> +	mode = ioread32(modemr);
> +	iounmap(modemr);
> +
> +	err = boot_mode_reg_set(mode);
> +err:
> +	if (err)
> +		pr_err("failed to initialise boot mode");
> +	return err;
> +}
> +
> +int __init rcar_gen2_init_boot_mode(void)
> +{
> +	if (of_machine_is_compatible("renesas,r8a7790") ||
> +	    of_machine_is_compatible("renesas,r8a7791") ||
> +	    of_machine_is_compatible("renesas,r8a7792") ||
> +	    of_machine_is_compatible("renesas,r8a7793") ||
> +	    of_machine_is_compatible("renesas,r8a7794"))
> +		return rcar_gen2_read_mode_pins();
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(boot_mode_set);

Could you tell me the purpose of this ?
There's no such function name in this file.

> +early_initcall(rcar_gen2_init_boot_mode);
> +
> +MODULE_LICENSE("GPL");

The license should be GPLv2 to match with the paragraph at top of this 
file, right ?

> +MODULE_AUTHOR("Simon Horman <horms@verge.net.au>");
> +MODULE_DESCRIPTION("R-Car Gen2 Boot Mode Register Driver");
> diff --git a/include/misc/boot-mode-reg.h b/include/misc/boot-mode-reg.h
> index 34ee653279a4..f8fea0ea5a3e 100644
> --- a/include/misc/boot-mode-reg.h
> +++ b/include/misc/boot-mode-reg.h
> @@ -21,4 +21,7 @@
>   int boot_mode_reg_get(u32 *mode);
>   int boot_mode_reg_set(u32 mode);
>
> +/* Allow explicit initialisation before initcalls */
> +int rcar_gen2_init_boot_mode(void);
> +
>   #endif
>

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Khiem Nguyen]

On 10/15/2015 2:34 PM, Khiem Nguyen wrote:
> Hi Simon,
>
> Thanks for your patch.
>
> On 10/15/2015 1:59 PM, Simon Horman wrote:
>> Boot mode register driver for R-Car Gen2.
>>
>> If running on a supported platform it reads the boot mode register and
>> records it using the boot mode register infrastructure established by an
>> earlier patch.
>>
>> rcar_gen2_init_boot_mode() is exported allow it to be explicitly called in
>> cases where the boot mode register is needed before init calls are made.
>>
>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

[snip]

>> +int __init rcar_gen2_init_boot_mode(void)
>> +{
>> +	if (of_machine_is_compatible("renesas,r8a7790") ||
>> +	    of_machine_is_compatible("renesas,r8a7791") ||
>> +	    of_machine_is_compatible("renesas,r8a7792") ||
>> +	    of_machine_is_compatible("renesas,r8a7793") ||
>> +	    of_machine_is_compatible("renesas,r8a7794"))
>> +		return rcar_gen2_read_mode_pins();
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(boot_mode_set);
>
> Could you tell me the purpose of this ?
> There's no such function name in this file.

Read again the commit log, this export symbol should be 
rcar_gen2_init_boot_mode.

>> +early_initcall(rcar_gen2_init_boot_mode);
>> +
>> +MODULE_LICENSE("GPL");
>
> The license should be GPLv2 to match with the paragraph at top of this
> file, right ?
>
>> +MODULE_AUTHOR("Simon Horman <horms@verge.net.au>");
>> +MODULE_DESCRIPTION("R-Car Gen2 Boot Mode Register Driver");
>> diff --git a/include/misc/boot-mode-reg.h b/include/misc/boot-mode-reg.h
>> index 34ee653279a4..f8fea0ea5a3e 100644
>> --- a/include/misc/boot-mode-reg.h
>> +++ b/include/misc/boot-mode-reg.h
>> @@ -21,4 +21,7 @@
>>    int boot_mode_reg_get(u32 *mode);
>>    int boot_mode_reg_set(u32 mode);
>>
>> +/* Allow explicit initialisation before initcalls */
>> +int rcar_gen2_init_boot_mode(void);
>> +
>>    #endif
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Simon Horman]

On Thu, Oct 15, 2015 at 09:09:13AM +0200, Geert Uytterhoeven wrote:
> On Thu, Oct 15, 2015 at 8:59 AM, Simon Horman
> <horms+renesas@verge.net.au> wrote:
> > --- /dev/null
> > +++ b/drivers/misc/boot-mode-reg/rcar-gen2.c
> > @@ -0,0 +1,61 @@
> > +/*
> > + * R-Car Gen2 Boot Mode Register Driver
> 
> > +#define MODEMR 0xe6160060
> 
> Shouldn't this come from DT?

If its a property of the SoC then I'm not sure that it needs to as its a
known value for the supported SoCs.

> > +static int __init rcar_gen2_read_mode_pins(void)
> > +{
> > +       void __iomem *modemr;
> > +       int err = -ENOMEM;
> > +       static u32 mode;
> > +
> > +       modemr = ioremap_nocache(MODEMR, 4);
> > +       if (!modemr) {
> > +               pr_err("failed to map boot mode register");
> > +               goto err;
> > +       }
> > +       mode = ioread32(modemr);
> > +       iounmap(modemr);
> > +
> > +       err = boot_mode_reg_set(mode);
> > +err:
> > +       if (err)
> > +               pr_err("failed to initialise boot mode");
> > +       return err;
> > +}
> 
> > --- a/include/misc/boot-mode-reg.h
> > +++ b/include/misc/boot-mode-reg.h
> > @@ -21,4 +21,7 @@
> >  int boot_mode_reg_get(u32 *mode);
> >  int boot_mode_reg_set(u32 mode);
> >
> > +/* Allow explicit initialisation before initcalls */
> > +int rcar_gen2_init_boot_mode(void);
> 
> When using explicit initialisation before initcalls, the second call will
> trigger a new ioremap/ioread32/iounmap cycle.

Thanks, I'll fix that.

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Simon Horman]

On Thu, Oct 15, 2015 at 02:34:05PM +0700, Khiem Nguyen wrote:
> Hi Simon,
> 
> Thanks for your patch.
> 
> On 10/15/2015 1:59 PM, Simon Horman wrote:
> >Boot mode register driver for R-Car Gen2.
> >
> >If running on a supported platform it reads the boot mode register and
> >records it using the boot mode register infrastructure established by an
> >earlier patch.
> >
> >rcar_gen2_init_boot_mode() is exported allow it to be explicitly called in
> >cases where the boot mode register is needed before init calls are made.
> >
> >Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >---
> >  drivers/misc/boot-mode-reg/Kconfig     |  8 +++++
> >  drivers/misc/boot-mode-reg/Makefile    |  1 +
> >  drivers/misc/boot-mode-reg/rcar-gen2.c | 61 ++++++++++++++++++++++++++++++++++
> >  include/misc/boot-mode-reg.h           |  3 ++
> >  4 files changed, 73 insertions(+)
> >  create mode 100644 drivers/misc/boot-mode-reg/rcar-gen2.c
> >
> >diff --git a/drivers/misc/boot-mode-reg/Kconfig b/drivers/misc/boot-mode-reg/Kconfig
> >index 806eba24238f..4731edf8a9db 100644
> >--- a/drivers/misc/boot-mode-reg/Kconfig
> >+++ b/drivers/misc/boot-mode-reg/Kconfig
> >@@ -9,3 +9,11 @@ config BOOT_MODE_REG_CORE
> >  	help
> >  	  Say Y here to allow support for drivers to read boot mode
> >  	  registers and make the value available to other subsystems.
> >+
> >+config BOOT_MODE_REG_RCAR_GEN2
> >+	tristate "Boot Mode Register Driver for Renesas R-Car Gen2 SoCs"
> >+	default n
> >+	select BOOT_MODE_REG_CORE
> >+	help
> >+	  Say Y here to allow support for reading the boot mode register
> >+	  on Renesas R-Car Gen2 SoCs.
> >diff --git a/drivers/misc/boot-mode-reg/Makefile b/drivers/misc/boot-mode-reg/Makefile
> >index 19134b20a7f1..d097fd0164aa 100644
> >--- a/drivers/misc/boot-mode-reg/Makefile
> >+++ b/drivers/misc/boot-mode-reg/Makefile
> >@@ -4,3 +4,4 @@
> >  #
> >
> >  obj-$(CONFIG_BOOT_MODE_REG_CORE)	+= core.o
> >+obj-$(CONFIG_BOOT_MODE_REG_RCAR_GEN2)	+= rcar-gen2.o
> >diff --git a/drivers/misc/boot-mode-reg/rcar-gen2.c b/drivers/misc/boot-mode-reg/rcar-gen2.c
> >new file mode 100644
> >index 000000000000..0f1a06fcf094
> >--- /dev/null
> >+++ b/drivers/misc/boot-mode-reg/rcar-gen2.c
> >@@ -0,0 +1,61 @@
> >+/*
> >+ * R-Car Gen2 Boot Mode Register Driver
> >+ *
> >+ * Copyright (C) 2015 Simon Horman
> >+ *
> >+ * This program is free software; you can redistribute it and/or modify
> >+ * it under the terms of the GNU General Public License as published by
> >+ * the Free Software Foundation; version 2 of the License.
> >+ *
> >+ * This program is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >+ * GNU General Public License for more details.
> >+ */
> >+
> >+#include <linux/io.h>
> >+#include <linux/module.h>
> >+#include <linux/of.h>
> >+
> >+#include <misc/boot-mode-reg.h>
> >+
> >+#define MODEMR 0xe6160060
> >+
> >+static int __init rcar_gen2_read_mode_pins(void)
> >+{
> >+	void __iomem *modemr;
> >+	int err = -ENOMEM;
> >+	static u32 mode;
> >+
> >+	modemr = ioremap_nocache(MODEMR, 4);
> >+	if (!modemr) {
> >+		pr_err("failed to map boot mode register");
> >+		goto err;
> >+	}
> >+	mode = ioread32(modemr);
> >+	iounmap(modemr);
> >+
> >+	err = boot_mode_reg_set(mode);
> >+err:
> >+	if (err)
> >+		pr_err("failed to initialise boot mode");
> >+	return err;
> >+}
> >+
> >+int __init rcar_gen2_init_boot_mode(void)
> >+{
> >+	if (of_machine_is_compatible("renesas,r8a7790") ||
> >+	    of_machine_is_compatible("renesas,r8a7791") ||
> >+	    of_machine_is_compatible("renesas,r8a7792") ||
> >+	    of_machine_is_compatible("renesas,r8a7793") ||
> >+	    of_machine_is_compatible("renesas,r8a7794"))
> >+		return rcar_gen2_read_mode_pins();
> >+
> >+	return 0;
> >+}
> >+EXPORT_SYMBOL_GPL(boot_mode_set);
> 
> Could you tell me the purpose of this ?
> There's no such function name in this file.

Sorry about that, it is supposed to be
EXPORT_SYMBOL_GPL(rcar_gen2_init_boot_mode);

> >+early_initcall(rcar_gen2_init_boot_mode);
> >+
> >+MODULE_LICENSE("GPL");
> 
> The license should be GPLv2 to match with the paragraph at top of this file,
> right ?
> 
> >+MODULE_AUTHOR("Simon Horman <horms@verge.net.au>");
> >+MODULE_DESCRIPTION("R-Car Gen2 Boot Mode Register Driver");
> >diff --git a/include/misc/boot-mode-reg.h b/include/misc/boot-mode-reg.h
> >index 34ee653279a4..f8fea0ea5a3e 100644
> >--- a/include/misc/boot-mode-reg.h
> >+++ b/include/misc/boot-mode-reg.h
> >@@ -21,4 +21,7 @@
> >  int boot_mode_reg_get(u32 *mode);
> >  int boot_mode_reg_set(u32 mode);
> >
> >+/* Allow explicit initialisation before initcalls */
> >+int rcar_gen2_init_boot_mode(void);
> >+
> >  #endif
> >
> 

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Laurent Pinchart]

Hi Simon,

On Thursday 15 October 2015 16:59:18 Simon Horman wrote:
> On Thu, Oct 15, 2015 at 09:09:13AM +0200, Geert Uytterhoeven wrote:
> > On Thu, Oct 15, 2015 at 8:59 AM, Simon Horman wrote:
> >> --- /dev/null
> >> +++ b/drivers/misc/boot-mode-reg/rcar-gen2.c
> >> @@ -0,0 +1,61 @@
> >> +/*
> >> + * R-Car Gen2 Boot Mode Register Driver
> >> 
> >> +#define MODEMR 0xe6160060
> > 
> > Shouldn't this come from DT?
> 
> If its a property of the SoC then I'm not sure that it needs to as its a
> known value for the supported SoCs.

Hypervisors (at least Xen) use DT to initialize memory mappings. I believe we 
should thus describe the RST IP in DT and create an rcar-rst driver that 
includes the code from this patch.

> >> +static int __init rcar_gen2_read_mode_pins(void)
> >> +{
> >> +       void __iomem *modemr;
> >> +       int err = -ENOMEM;
> >> +       static u32 mode;
> >> +
> >> +       modemr = ioremap_nocache(MODEMR, 4);
> >> +       if (!modemr) {
> >> +               pr_err("failed to map boot mode register");
> >> +               goto err;
> >> +       }
> >> +       mode = ioread32(modemr);
> >> +       iounmap(modemr);
> >> +
> >> +       err = boot_mode_reg_set(mode);
> >> +err:
> >> +       if (err)
> >> +               pr_err("failed to initialise boot mode");
> >> +       return err;
> >> +}
> >> 
> >> --- a/include/misc/boot-mode-reg.h
> >> +++ b/include/misc/boot-mode-reg.h
> >> @@ -21,4 +21,7 @@
> >>  int boot_mode_reg_get(u32 *mode);
> >>  int boot_mode_reg_set(u32 mode);
> >> 
> >> +/* Allow explicit initialisation before initcalls */
> >> +int rcar_gen2_init_boot_mode(void);
> > 
> > When using explicit initialisation before initcalls, the second call will
> > trigger a new ioremap/ioread32/iounmap cycle.
> 
> Thanks, I'll fix that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Laurent Pinchart]

Hi Simon,

Thank you for the patch.

On Thursday 15 October 2015 15:59:33 Simon Horman wrote:
> Boot mode register driver for R-Car Gen2.
> 
> If running on a supported platform it reads the boot mode register and
> records it using the boot mode register infrastructure established by an
> earlier patch.
> 
> rcar_gen2_init_boot_mode() is exported allow it to be explicitly called in
> cases where the boot mode register is needed before init calls are made.
> 
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>  drivers/misc/boot-mode-reg/Kconfig     |  8 +++++
>  drivers/misc/boot-mode-reg/Makefile    |  1 +
>  drivers/misc/boot-mode-reg/rcar-gen2.c | 61 +++++++++++++++++++++++++++++++
>  include/misc/boot-mode-reg.h           |  3 ++
>  4 files changed, 73 insertions(+)
>  create mode 100644 drivers/misc/boot-mode-reg/rcar-gen2.c
> 
> diff --git a/drivers/misc/boot-mode-reg/Kconfig
> b/drivers/misc/boot-mode-reg/Kconfig index 806eba24238f..4731edf8a9db
> 100644
> --- a/drivers/misc/boot-mode-reg/Kconfig
> +++ b/drivers/misc/boot-mode-reg/Kconfig
> @@ -9,3 +9,11 @@ config BOOT_MODE_REG_CORE
>  	help
>  	  Say Y here to allow support for drivers to read boot mode
>  	  registers and make the value available to other subsystems.
> +
> +config BOOT_MODE_REG_RCAR_GEN2
> +	tristate "Boot Mode Register Driver for Renesas R-Car Gen2 SoCs"
> +	default n
> +	select BOOT_MODE_REG_CORE
> +	help
> +	  Say Y here to allow support for reading the boot mode register
> +	  on Renesas R-Car Gen2 SoCs.
> diff --git a/drivers/misc/boot-mode-reg/Makefile
> b/drivers/misc/boot-mode-reg/Makefile index 19134b20a7f1..d097fd0164aa
> 100644
> --- a/drivers/misc/boot-mode-reg/Makefile
> +++ b/drivers/misc/boot-mode-reg/Makefile
> @@ -4,3 +4,4 @@
>  #
> 
>  obj-$(CONFIG_BOOT_MODE_REG_CORE)	+= core.o
> +obj-$(CONFIG_BOOT_MODE_REG_RCAR_GEN2)	+= rcar-gen2.o
> diff --git a/drivers/misc/boot-mode-reg/rcar-gen2.c
> b/drivers/misc/boot-mode-reg/rcar-gen2.c new file mode 100644
> index 000000000000..0f1a06fcf094
> --- /dev/null
> +++ b/drivers/misc/boot-mode-reg/rcar-gen2.c
> @@ -0,0 +1,61 @@
> +/*
> + * R-Car Gen2 Boot Mode Register Driver
> + *
> + * Copyright (C) 2015 Simon Horman
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#include <misc/boot-mode-reg.h>
> +
> +#define MODEMR 0xe6160060
> +
> +static int __init rcar_gen2_read_mode_pins(void)
> +{
> +	void __iomem *modemr;
> +	int err = -ENOMEM;
> +	static u32 mode;
> +
> +	modemr = ioremap_nocache(MODEMR, 4);
> +	if (!modemr) {
> +		pr_err("failed to map boot mode register");
> +		goto err;
> +	}
> +	mode = ioread32(modemr);
> +	iounmap(modemr);
> +
> +	err = boot_mode_reg_set(mode);
> +err:
> +	if (err)
> +		pr_err("failed to initialise boot mode");
> +	return err;
> +}
> +
> +int __init rcar_gen2_init_boot_mode(void)
> +{
> +	if (of_machine_is_compatible("renesas,r8a7790") ||
> +	    of_machine_is_compatible("renesas,r8a7791") ||
> +	    of_machine_is_compatible("renesas,r8a7792") ||
> +	    of_machine_is_compatible("renesas,r8a7793") ||
> +	    of_machine_is_compatible("renesas,r8a7794"))
> +		return rcar_gen2_read_mode_pins();
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(boot_mode_set);
> +early_initcall(rcar_gen2_init_boot_mode);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Simon Horman <horms@verge.net.au>");
> +MODULE_DESCRIPTION("R-Car Gen2 Boot Mode Register Driver");
> diff --git a/include/misc/boot-mode-reg.h b/include/misc/boot-mode-reg.h
> index 34ee653279a4..f8fea0ea5a3e 100644
> --- a/include/misc/boot-mode-reg.h
> +++ b/include/misc/boot-mode-reg.h
> @@ -21,4 +21,7 @@
>  int boot_mode_reg_get(u32 *mode);
>  int boot_mode_reg_set(u32 mode);
> 
> +/* Allow explicit initialisation before initcalls */
> +int rcar_gen2_init_boot_mode(void);
> +

I would move this to a separate header file.

And I'd like to also get rid of it :-) Do we need this function for any 
purpose other than arch timer initialization in arch/arm/mach-shmobile/setup-
rcar-gen2.c ? Quickly looking it that code I wonder whether we couldn't get 
the extal frequency from DT instead of the boot mode pins, which would then 
remove the dependency.

In the longer term we should try to get rid of setup-rcar-gen2.c, but I wonder 
how to do so. It looks like a workaround due to a broken boot loader to kernel 
contract. We should have fixed that in u-boot :-/

>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Geert Uytterhoeven]

Hi Laurent,

On Fri, Oct 23, 2015 at 2:49 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> --- a/include/misc/boot-mode-reg.h
>> +++ b/include/misc/boot-mode-reg.h
>> @@ -21,4 +21,7 @@
>>  int boot_mode_reg_get(u32 *mode);
>>  int boot_mode_reg_set(u32 mode);
>>
>> +/* Allow explicit initialisation before initcalls */
>> +int rcar_gen2_init_boot_mode(void);
>> +
>
> I would move this to a separate header file.
>
> And I'd like to also get rid of it :-) Do we need this function for any
> purpose other than arch timer initialization in arch/arm/mach-shmobile/setup-
> rcar-gen2.c ? Quickly looking it that code I wonder whether we couldn't get
> the extal frequency from DT instead of the boot mode pins, which would then
> remove the dependency.

We do have the extal frequency in DT.

The boot mode pins does not control the extal frequency, but a few dividers
internal to the CPG.

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: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Laurent Pinchart]

Hi Geert,

On Saturday 24 October 2015 19:46:11 Geert Uytterhoeven wrote:
> On Fri, Oct 23, 2015 at 2:49 PM, Laurent Pinchart wrote:
> >> --- a/include/misc/boot-mode-reg.h
> >> +++ b/include/misc/boot-mode-reg.h
> >> @@ -21,4 +21,7 @@
> >> 
> >>  int boot_mode_reg_get(u32 *mode);
> >>  int boot_mode_reg_set(u32 mode);
> >> 
> >> +/* Allow explicit initialisation before initcalls */
> >> +int rcar_gen2_init_boot_mode(void);
> >> +
> > 
> > I would move this to a separate header file.
> > 
> > And I'd like to also get rid of it :-) Do we need this function for any
> > purpose other than arch timer initialization in
> > arch/arm/mach-shmobile/setup-rcar-gen2.c ? Quickly looking it that code
> > I wonder whether we couldn't get the extal frequency from DT instead of
> > the boot mode pins, which would then remove the dependency.
> 
> We do have the extal frequency in DT.
> 
> The boot mode pins does not control the extal frequency, but a few dividers
> internal to the CPG.

Agreed, but in rcar_gen2_read_mode_pins() it's used to get the external clock 
frequency as each PLL setting is specific to one external frequency.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Simon Horman]

On Fri, Oct 23, 2015 at 03:37:46PM +0300, Laurent Pinchart wrote:
> Hi Simon,
> 
> On Thursday 15 October 2015 16:59:18 Simon Horman wrote:
> > On Thu, Oct 15, 2015 at 09:09:13AM +0200, Geert Uytterhoeven wrote:
> > > On Thu, Oct 15, 2015 at 8:59 AM, Simon Horman wrote:
> > >> --- /dev/null
> > >> +++ b/drivers/misc/boot-mode-reg/rcar-gen2.c
> > >> @@ -0,0 +1,61 @@
> > >> +/*
> > >> + * R-Car Gen2 Boot Mode Register Driver
> > >> 
> > >> +#define MODEMR 0xe6160060
> > > 
> > > Shouldn't this come from DT?
> > 
> > If its a property of the SoC then I'm not sure that it needs to as its a
> > known value for the supported SoCs.
> 
> Hypervisors (at least Xen) use DT to initialize memory mappings. I believe we 
> should thus describe the RST IP in DT and create an rcar-rst driver that 
> includes the code from this patch.

So we would add a binding, say a compat string and a register range.
That might look like this:


	rst: reset-controller@e6160000 {
		compatible = "renesas,rst-r8a7795", "syscon";
		reg = <0 0xe6160000 0 0x0200>;
	};

The above is copped from Geerts earlier work
"[PATCH 1/6]  reset: Add renesas,rst DT bindings"

http://www.spinics.net/lists/linux-sh/msg44800.html:

Is that binding what we are after?

Would the driver do anything beyond what it currently does +
using an offset to the base of its register range instead of the
hardcoded MODEMR value (and changes discussed elswhere in this thread,
e.g. to use soemthing like CLK_OF_DECLARE) ?

> > >> +static int __init rcar_gen2_read_mode_pins(void)
> > >> +{
> > >> +       void __iomem *modemr;
> > >> +       int err = -ENOMEM;
> > >> +       static u32 mode;
> > >> +
> > >> +       modemr = ioremap_nocache(MODEMR, 4);
> > >> +       if (!modemr) {
> > >> +               pr_err("failed to map boot mode register");
> > >> +               goto err;
> > >> +       }
> > >> +       mode = ioread32(modemr);
> > >> +       iounmap(modemr);
> > >> +
> > >> +       err = boot_mode_reg_set(mode);
> > >> +err:
> > >> +       if (err)
> > >> +               pr_err("failed to initialise boot mode");
> > >> +       return err;
> > >> +}
> > >> 
> > >> --- a/include/misc/boot-mode-reg.h
> > >> +++ b/include/misc/boot-mode-reg.h
> > >> @@ -21,4 +21,7 @@
> > >>  int boot_mode_reg_get(u32 *mode);
> > >>  int boot_mode_reg_set(u32 mode);
> > >> 
> > >> +/* Allow explicit initialisation before initcalls */
> > >> +int rcar_gen2_init_boot_mode(void);
> > > 
> > > When using explicit initialisation before initcalls, the second call will
> > > trigger a new ioremap/ioread32/iounmap cycle.
> > 
> > Thanks, I'll fix that.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Laurent Pinchart]

Hi Simon,

On Monday 26 October 2015 14:50:08 Simon Horman wrote:
> On Fri, Oct 23, 2015 at 03:37:46PM +0300, Laurent Pinchart wrote:
> > On Thursday 15 October 2015 16:59:18 Simon Horman wrote:
> >> On Thu, Oct 15, 2015 at 09:09:13AM +0200, Geert Uytterhoeven wrote:
> >>> On Thu, Oct 15, 2015 at 8:59 AM, Simon Horman wrote:
> >>>> --- /dev/null
> >>>> +++ b/drivers/misc/boot-mode-reg/rcar-gen2.c
> >>>> @@ -0,0 +1,61 @@
> >>>> +/*
> >>>> + * R-Car Gen2 Boot Mode Register Driver
> >>>> 
> >>>> +#define MODEMR 0xe6160060
> >>> 
> >>> Shouldn't this come from DT?
> >> 
> >> If its a property of the SoC then I'm not sure that it needs to as its a
> >> known value for the supported SoCs.
> > 
> > Hypervisors (at least Xen) use DT to initialize memory mappings. I believe
> > we should thus describe the RST IP in DT and create an rcar-rst driver
> > that includes the code from this patch.
> 
> So we would add a binding, say a compat string and a register range.
> That might look like this:
> 
> 	rst: reset-controller@e6160000 {
> 		compatible = "renesas,rst-r8a7795", "syscon";
> 		reg = <0 0xe6160000 0 0x0200>;
> 	};
> 
> The above is copped from Geerts earlier work
> "[PATCH 1/6]  reset: Add renesas,rst DT bindings"
> 
> http://www.spinics.net/lists/linux-sh/msg44800.html:
> 
> Is that binding what we are after?

That's exactly it, except that as we'll have a proper API to retrieve the boot 
mode we won't need the "syscon" compatible string.

> Would the driver do anything beyond what it currently does + using an offset
> to the base of its register range instead of the hardcoded MODEMR value (and
> changes discussed elswhere in this thread, e.g. to use soemthing like
> CLK_OF_DECLARE) ?

It should handle CPU reset in the future, but for now your description is all 
that would need to be implemented.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Simon Horman]

On Mon, Oct 26, 2015 at 04:08:04AM +0200, Laurent Pinchart wrote:
> Hi Geert,
> 
> On Saturday 24 October 2015 19:46:11 Geert Uytterhoeven wrote:
> > On Fri, Oct 23, 2015 at 2:49 PM, Laurent Pinchart wrote:
> > >> --- a/include/misc/boot-mode-reg.h
> > >> +++ b/include/misc/boot-mode-reg.h
> > >> @@ -21,4 +21,7 @@
> > >> 
> > >>  int boot_mode_reg_get(u32 *mode);
> > >>  int boot_mode_reg_set(u32 mode);
> > >> 
> > >> +/* Allow explicit initialisation before initcalls */
> > >> +int rcar_gen2_init_boot_mode(void);
> > >> +
> > > 
> > > I would move this to a separate header file.
> > > 
> > > And I'd like to also get rid of it :-) Do we need this function for any
> > > purpose other than arch timer initialization in
> > > arch/arm/mach-shmobile/setup-rcar-gen2.c ? Quickly looking it that code
> > > I wonder whether we couldn't get the extal frequency from DT instead of
> > > the boot mode pins, which would then remove the dependency.
> > 
> > We do have the extal frequency in DT.
> > 
> > The boot mode pins does not control the extal frequency, but a few dividers
> > internal to the CPG.
> 
> Agreed, but in rcar_gen2_read_mode_pins() it's used to get the external clock 
> frequency as each PLL setting is specific to one external frequency.

I think that Laurent has a good point here and if extal frequency
was taken from DT then we probably wouldn't need early access to mode pins
in rcar_gen2_read_mode_pins().

However, early access to mode pins is also seems to be required by
rcar_gen2_cpg_register_clock().

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Geert Uytterhoeven]

On Tue, Dec 15, 2015 at 8:58 AM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Oct 26, 2015 at 04:08:04AM +0200, Laurent Pinchart wrote:
>> On Saturday 24 October 2015 19:46:11 Geert Uytterhoeven wrote:
>> > On Fri, Oct 23, 2015 at 2:49 PM, Laurent Pinchart wrote:
>> > >> --- a/include/misc/boot-mode-reg.h
>> > >> +++ b/include/misc/boot-mode-reg.h
>> > >> @@ -21,4 +21,7 @@
>> > >>
>> > >>  int boot_mode_reg_get(u32 *mode);
>> > >>  int boot_mode_reg_set(u32 mode);
>> > >>
>> > >> +/* Allow explicit initialisation before initcalls */
>> > >> +int rcar_gen2_init_boot_mode(void);
>> > >> +
>> > >
>> > > I would move this to a separate header file.
>> > >
>> > > And I'd like to also get rid of it :-) Do we need this function for any
>> > > purpose other than arch timer initialization in
>> > > arch/arm/mach-shmobile/setup-rcar-gen2.c ? Quickly looking it that code
>> > > I wonder whether we couldn't get the extal frequency from DT instead of
>> > > the boot mode pins, which would then remove the dependency.
>> >
>> > We do have the extal frequency in DT.
>> >
>> > The boot mode pins does not control the extal frequency, but a few dividers
>> > internal to the CPG.
>>
>> Agreed, but in rcar_gen2_read_mode_pins() it's used to get the external clock
>> frequency as each PLL setting is specific to one external frequency.
>
> I think that Laurent has a good point here and if extal frequency
> was taken from DT then we probably wouldn't need early access to mode pins
> in rcar_gen2_read_mode_pins().

Indeed.

Can we make use of Documentation/devicetree/bindings/arm/arch_timer.txt
for r8a7794?

"- clock-frequency : The frequency of the main counter, in Hz. Should be present
   only where necessary to work around broken firmware which does not configure
   CNTFRQ on all CPUs to a uniform correct value. Use of this property is
   strongly discouraged; fix your firmware unless absolutely impossible."

> However, early access to mode pins is also seems to be required by
> rcar_gen2_cpg_register_clock().

Which is not that early...
Which can easily use the rst node and renesas,modemr?

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: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Magnus Damm]

Hi Geert,

On Tue, Dec 15, 2015 at 5:16 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Dec 15, 2015 at 8:58 AM, Simon Horman <horms@verge.net.au> wrote:
>> On Mon, Oct 26, 2015 at 04:08:04AM +0200, Laurent Pinchart wrote:
>>> On Saturday 24 October 2015 19:46:11 Geert Uytterhoeven wrote:
>>> > On Fri, Oct 23, 2015 at 2:49 PM, Laurent Pinchart wrote:
>>> > >> --- a/include/misc/boot-mode-reg.h
>>> > >> +++ b/include/misc/boot-mode-reg.h
>>> > >> @@ -21,4 +21,7 @@
>>> > >>
>>> > >>  int boot_mode_reg_get(u32 *mode);
>>> > >>  int boot_mode_reg_set(u32 mode);
>>> > >>
>>> > >> +/* Allow explicit initialisation before initcalls */
>>> > >> +int rcar_gen2_init_boot_mode(void);
>>> > >> +
>>> > >
>>> > > I would move this to a separate header file.
>>> > >
>>> > > And I'd like to also get rid of it :-) Do we need this function for any
>>> > > purpose other than arch timer initialization in
>>> > > arch/arm/mach-shmobile/setup-rcar-gen2.c ? Quickly looking it that code
>>> > > I wonder whether we couldn't get the extal frequency from DT instead of
>>> > > the boot mode pins, which would then remove the dependency.
>>> >
>>> > We do have the extal frequency in DT.
>>> >
>>> > The boot mode pins does not control the extal frequency, but a few dividers
>>> > internal to the CPG.
>>>
>>> Agreed, but in rcar_gen2_read_mode_pins() it's used to get the external clock
>>> frequency as each PLL setting is specific to one external frequency.
>>
>> I think that Laurent has a good point here and if extal frequency
>> was taken from DT then we probably wouldn't need early access to mode pins
>> in rcar_gen2_read_mode_pins().
>
> Indeed.
>
> Can we make use of Documentation/devicetree/bindings/arm/arch_timer.txt
> for r8a7794?
>
> "- clock-frequency : The frequency of the main counter, in Hz. Should be present
>    only where necessary to work around broken firmware which does not configure
>    CNTFRQ on all CPUs to a uniform correct value. Use of this property is
>    strongly discouraged; fix your firmware unless absolutely impossible."

Just one random comment:

The clock for the arch timer on Gen2 was documented to be fixed, but
in reality it varied depending on MD pin setting. In my opinion it
would make sense that the arch timer DT node would follow the same
style as the rest of the devices and tie in to the clock topology via
DT instead of using a quasi-fixed frequency.

Thanks,

/ magnus

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Geert Uytterhoeven]

On Tue, Dec 15, 2015 at 9:59 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Tue, Dec 15, 2015 at 5:16 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Tue, Dec 15, 2015 at 8:58 AM, Simon Horman <horms@verge.net.au> wrote:
>>> On Mon, Oct 26, 2015 at 04:08:04AM +0200, Laurent Pinchart wrote:
>>>> On Saturday 24 October 2015 19:46:11 Geert Uytterhoeven wrote:
>>>> > On Fri, Oct 23, 2015 at 2:49 PM, Laurent Pinchart wrote:
>>>> > >> --- a/include/misc/boot-mode-reg.h
>>>> > >> +++ b/include/misc/boot-mode-reg.h
>>>> > >> @@ -21,4 +21,7 @@
>>>> > >>
>>>> > >>  int boot_mode_reg_get(u32 *mode);
>>>> > >>  int boot_mode_reg_set(u32 mode);
>>>> > >>
>>>> > >> +/* Allow explicit initialisation before initcalls */
>>>> > >> +int rcar_gen2_init_boot_mode(void);
>>>> > >> +
>>>> > >
>>>> > > I would move this to a separate header file.
>>>> > >
>>>> > > And I'd like to also get rid of it :-) Do we need this function for any
>>>> > > purpose other than arch timer initialization in
>>>> > > arch/arm/mach-shmobile/setup-rcar-gen2.c ? Quickly looking it that code
>>>> > > I wonder whether we couldn't get the extal frequency from DT instead of
>>>> > > the boot mode pins, which would then remove the dependency.
>>>> >
>>>> > We do have the extal frequency in DT.
>>>> >
>>>> > The boot mode pins does not control the extal frequency, but a few dividers
>>>> > internal to the CPG.
>>>>
>>>> Agreed, but in rcar_gen2_read_mode_pins() it's used to get the external clock
>>>> frequency as each PLL setting is specific to one external frequency.
>>>
>>> I think that Laurent has a good point here and if extal frequency
>>> was taken from DT then we probably wouldn't need early access to mode pins
>>> in rcar_gen2_read_mode_pins().
>>
>> Indeed.
>>
>> Can we make use of Documentation/devicetree/bindings/arm/arch_timer.txt
>> for r8a7794?
>>
>> "- clock-frequency : The frequency of the main counter, in Hz. Should be present
>>    only where necessary to work around broken firmware which does not configure
>>    CNTFRQ on all CPUs to a uniform correct value. Use of this property is
>>    strongly discouraged; fix your firmware unless absolutely impossible."
>
> Just one random comment:
>
> The clock for the arch timer on Gen2 was documented to be fixed, but
> in reality it varied depending on MD pin setting. In my opinion it
> would make sense that the arch timer DT node would follow the same
> style as the rest of the devices and tie in to the clock topology via
> DT instead of using a quasi-fixed frequency.

Yeah, I was actually looking for a "clocks" property in that document,
when I found "clock-frequency". This clearly follows the spirit of ePAPR,
which predates CCF.

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: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Simon Horman]

On Tue, Dec 15, 2015 at 09:16:27AM +0100, Geert Uytterhoeven wrote:
> On Tue, Dec 15, 2015 at 8:58 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, Oct 26, 2015 at 04:08:04AM +0200, Laurent Pinchart wrote:
> >> On Saturday 24 October 2015 19:46:11 Geert Uytterhoeven wrote:
> >> > On Fri, Oct 23, 2015 at 2:49 PM, Laurent Pinchart wrote:
> >> > >> --- a/include/misc/boot-mode-reg.h
> >> > >> +++ b/include/misc/boot-mode-reg.h
> >> > >> @@ -21,4 +21,7 @@
> >> > >>
> >> > >>  int boot_mode_reg_get(u32 *mode);
> >> > >>  int boot_mode_reg_set(u32 mode);
> >> > >>
> >> > >> +/* Allow explicit initialisation before initcalls */
> >> > >> +int rcar_gen2_init_boot_mode(void);
> >> > >> +
> >> > >
> >> > > I would move this to a separate header file.
> >> > >
> >> > > And I'd like to also get rid of it :-) Do we need this function for any
> >> > > purpose other than arch timer initialization in
> >> > > arch/arm/mach-shmobile/setup-rcar-gen2.c ? Quickly looking it that code
> >> > > I wonder whether we couldn't get the extal frequency from DT instead of
> >> > > the boot mode pins, which would then remove the dependency.
> >> >
> >> > We do have the extal frequency in DT.
> >> >
> >> > The boot mode pins does not control the extal frequency, but a few dividers
> >> > internal to the CPG.
> >>
> >> Agreed, but in rcar_gen2_read_mode_pins() it's used to get the external clock
> >> frequency as each PLL setting is specific to one external frequency.
> >
> > I think that Laurent has a good point here and if extal frequency
> > was taken from DT then we probably wouldn't need early access to mode pins
> > in rcar_gen2_read_mode_pins().
> 
> Indeed.
> 
> Can we make use of Documentation/devicetree/bindings/arm/arch_timer.txt
> for r8a7794?
> 
> "- clock-frequency : The frequency of the main counter, in Hz. Should be present
>    only where necessary to work around broken firmware which does not configure
>    CNTFRQ on all CPUs to a uniform correct value. Use of this property is
>    strongly discouraged; fix your firmware unless absolutely impossible."
> 
> > However, early access to mode pins is also seems to be required by
> > rcar_gen2_cpg_register_clock().
> 
> Which is not that early...
> Which can easily use the rst node and renesas,modemr?

It seems early enough that the initcall to initialise the boot mode pin
driver would not have kicked in. I can try fiddling the initcall level.
But I am missing the point?

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Geert Uytterhoeven]

Hi Simon,

On Wed, Dec 16, 2015 at 5:32 AM, Simon Horman <horms@verge.net.au> wrote:
> On Tue, Dec 15, 2015 at 09:16:27AM +0100, Geert Uytterhoeven wrote:
>> On Tue, Dec 15, 2015 at 8:58 AM, Simon Horman <horms@verge.net.au> wrote:
>> > On Mon, Oct 26, 2015 at 04:08:04AM +0200, Laurent Pinchart wrote:
>> >> On Saturday 24 October 2015 19:46:11 Geert Uytterhoeven wrote:
>> >> > On Fri, Oct 23, 2015 at 2:49 PM, Laurent Pinchart wrote:
>> >> > >> --- a/include/misc/boot-mode-reg.h
>> >> > >> +++ b/include/misc/boot-mode-reg.h
>> >> > >> @@ -21,4 +21,7 @@
>> >> > >>
>> >> > >>  int boot_mode_reg_get(u32 *mode);
>> >> > >>  int boot_mode_reg_set(u32 mode);
>> >> > >>
>> >> > >> +/* Allow explicit initialisation before initcalls */
>> >> > >> +int rcar_gen2_init_boot_mode(void);
>> >> > >> +
>> >> > >
>> >> > > I would move this to a separate header file.
>> >> > >
>> >> > > And I'd like to also get rid of it :-) Do we need this function for any
>> >> > > purpose other than arch timer initialization in
>> >> > > arch/arm/mach-shmobile/setup-rcar-gen2.c ? Quickly looking it that code
>> >> > > I wonder whether we couldn't get the extal frequency from DT instead of
>> >> > > the boot mode pins, which would then remove the dependency.
>> >> >
>> >> > We do have the extal frequency in DT.
>> >> >
>> >> > The boot mode pins does not control the extal frequency, but a few dividers
>> >> > internal to the CPG.
>> >>
>> >> Agreed, but in rcar_gen2_read_mode_pins() it's used to get the external clock
>> >> frequency as each PLL setting is specific to one external frequency.
>> >
>> > I think that Laurent has a good point here and if extal frequency
>> > was taken from DT then we probably wouldn't need early access to mode pins
>> > in rcar_gen2_read_mode_pins().
>>
>> Indeed.
>>
>> Can we make use of Documentation/devicetree/bindings/arm/arch_timer.txt
>> for r8a7794?
>>
>> "- clock-frequency : The frequency of the main counter, in Hz. Should be present
>>    only where necessary to work around broken firmware which does not configure
>>    CNTFRQ on all CPUs to a uniform correct value. Use of this property is
>>    strongly discouraged; fix your firmware unless absolutely impossible."
>>
>> > However, early access to mode pins is also seems to be required by
>> > rcar_gen2_cpg_register_clock().
>>
>> Which is not that early...
>> Which can easily use the rst node and renesas,modemr?
>
> It seems early enough that the initcall to initialise the boot mode pin
> driver would not have kicked in. I can try fiddling the initcall level.
> But I am missing the point?

I meant rcar_gen2_timer_init() runs earlier than rcar_gen2_cpg_clocks_init().

On Gen3, the clocks are initialized much later, as cpg_mssr_init()
uses subsys_initcall(), while Gen2 uses CLK_OF_DECLARE().
We can convert Gen2 to CPG/MSSR, though.

An advantage of using the rst node and renesas,modemr is that all ordering
is resolved automatically, through syscon_regmap_lookup_by_phandle()
and of_syscon_register().

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: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Simon Horman]

On Wed, Dec 16, 2015 at 08:40:23AM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Wed, Dec 16, 2015 at 5:32 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Tue, Dec 15, 2015 at 09:16:27AM +0100, Geert Uytterhoeven wrote:
> >> On Tue, Dec 15, 2015 at 8:58 AM, Simon Horman <horms@verge.net.au> wrote:
> >> > On Mon, Oct 26, 2015 at 04:08:04AM +0200, Laurent Pinchart wrote:
> >> >> On Saturday 24 October 2015 19:46:11 Geert Uytterhoeven wrote:
> >> >> > On Fri, Oct 23, 2015 at 2:49 PM, Laurent Pinchart wrote:
> >> >> > >> --- a/include/misc/boot-mode-reg.h
> >> >> > >> +++ b/include/misc/boot-mode-reg.h
> >> >> > >> @@ -21,4 +21,7 @@
> >> >> > >>
> >> >> > >>  int boot_mode_reg_get(u32 *mode);
> >> >> > >>  int boot_mode_reg_set(u32 mode);
> >> >> > >>
> >> >> > >> +/* Allow explicit initialisation before initcalls */
> >> >> > >> +int rcar_gen2_init_boot_mode(void);
> >> >> > >> +
> >> >> > >
> >> >> > > I would move this to a separate header file.
> >> >> > >
> >> >> > > And I'd like to also get rid of it :-) Do we need this function for any
> >> >> > > purpose other than arch timer initialization in
> >> >> > > arch/arm/mach-shmobile/setup-rcar-gen2.c ? Quickly looking it that code
> >> >> > > I wonder whether we couldn't get the extal frequency from DT instead of
> >> >> > > the boot mode pins, which would then remove the dependency.
> >> >> >
> >> >> > We do have the extal frequency in DT.
> >> >> >
> >> >> > The boot mode pins does not control the extal frequency, but a few dividers
> >> >> > internal to the CPG.
> >> >>
> >> >> Agreed, but in rcar_gen2_read_mode_pins() it's used to get the external clock
> >> >> frequency as each PLL setting is specific to one external frequency.
> >> >
> >> > I think that Laurent has a good point here and if extal frequency
> >> > was taken from DT then we probably wouldn't need early access to mode pins
> >> > in rcar_gen2_read_mode_pins().
> >>
> >> Indeed.
> >>
> >> Can we make use of Documentation/devicetree/bindings/arm/arch_timer.txt
> >> for r8a7794?
> >>
> >> "- clock-frequency : The frequency of the main counter, in Hz. Should be present
> >>    only where necessary to work around broken firmware which does not configure
> >>    CNTFRQ on all CPUs to a uniform correct value. Use of this property is
> >>    strongly discouraged; fix your firmware unless absolutely impossible."
> >>
> >> > However, early access to mode pins is also seems to be required by
> >> > rcar_gen2_cpg_register_clock().
> >>
> >> Which is not that early...
> >> Which can easily use the rst node and renesas,modemr?
> >
> > It seems early enough that the initcall to initialise the boot mode pin
> > driver would not have kicked in. I can try fiddling the initcall level.
> > But I am missing the point?
> 
> I meant rcar_gen2_timer_init() runs earlier than rcar_gen2_cpg_clocks_init().

Got it.

In rcar_gen2_timer_init() I think we can replace the use of boot mode pins
with use of extal frequency from DT, as suggested earlier from Laurent.

An implication of that is that we could infer the boot mode settings
from the extal frequency and then use that in rcar_gen2_cpg_clocks_init().
Though I'm not sure how we would transmit the information from
rcar_gen2_timer_init() to rcar_gen2_cpg_clocks_init().

> On Gen3, the clocks are initialized much later, as cpg_mssr_init()
> uses subsys_initcall(), while Gen2 uses CLK_OF_DECLARE().
> We can convert Gen2 to CPG/MSSR, though.

Ok, so we might have better luck with this approach if we were to try
it against a platform that used CPG/MSSR. e.g. Gen3.

I for one don't think its worth converting Gen2 to CPG/MSSR to allow
use of this prototype API. But if the conversion happens for other reasons
then things may start to fall into place.

> An advantage of using the rst node and renesas,modemr is that all ordering
> is resolved automatically, through syscon_regmap_lookup_by_phandle()
> and of_syscon_register().

I for one thought using the rst node and renesas,modemr was a nice clean
approach.

Re: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Geert Uytterhoeven]

Hi Simon,

On Tue, Jan 19, 2016 at 1:30 AM, Simon Horman <horms@verge.net.au> wrote:
> In rcar_gen2_timer_init() I think we can replace the use of boot mode pins
> with use of extal frequency from DT, as suggested earlier from Laurent.

Indeed. And it's really the extal frequency we need there, not the mode pin
configuration.

> An implication of that is that we could infer the boot mode settings
> from the extal frequency and then use that in rcar_gen2_cpg_clocks_init().
> Though I'm not sure how we would transmit the information from
> rcar_gen2_timer_init() to rcar_gen2_cpg_clocks_init().

Deriving the mode pin configuration from the extal frequency sounds backwards
to me. What if a non-standard crystal is used, like on Salvator-X?

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: [PATCH/RFC 2/6] boot-mode-reg: Add R-Car Gen2 driver

[Simon Horman]

On Tue, Jan 19, 2016 at 09:01:38AM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Tue, Jan 19, 2016 at 1:30 AM, Simon Horman <horms@verge.net.au> wrote:
> > In rcar_gen2_timer_init() I think we can replace the use of boot mode pins
> > with use of extal frequency from DT, as suggested earlier from Laurent.
> 
> Indeed. And it's really the extal frequency we need there, not the mode pin
> configuration.

Agreed.

> > An implication of that is that we could infer the boot mode settings
> > from the extal frequency and then use that in rcar_gen2_cpg_clocks_init().
> > Though I'm not sure how we would transmit the information from
> > rcar_gen2_timer_init() to rcar_gen2_cpg_clocks_init().
> 
> Deriving the mode pin configuration from the extal frequency sounds backwards
> to me. What if a non-standard crystal is used, like on Salvator-X?

I meant to say that it seems a bit awkward in my previous email.
You've pointed out a good reason why.