[PATCH 1/4] base: soc: Early register bus when needed

[PATCH 1/4] base: soc: Early register bus when needed

[Arnd Bergmann]

On Tuesday, October 4, 2016 11:09:24 AM CEST Geert Uytterhoeven wrote:
> If soc_device_register() is called before soc_bus_register(), it crashes
> with a NULL pointer dereference.
> 
> soc_bus_register() is already a core_initcall(), but drivers/base/ is
> entered later than e.g. drivers/pinctrl/ and drivers/soc/. Hence there
> are several subsystems that may need to know SoC revision information,
> while it's not so easy to initialize the SoC bus even earlier using an
> initcall.
> 
> To fix this, let soc_device_register() register the bus early if that
> hasn't happened yet.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 

Not nice, but I can't think of a better alternative, so

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the SoC bus

[Dirk Behme]

Hi Geert,

I've been offline some weeks, so sorry if I'm not completely up to date, 
yet, or miss anything.

Overall, having a quick look, the proposal in this patch series and your 
second series "arm64: renesas: r8a7795: R-Car H3 ES2.0 Prototype" looks 
nice to me. At least much better than encoding the ESx.x in the device 
tree as discussed some month ago ;)

Two minor comments below:


On 04.10.2016 11:09, Geert Uytterhoeven wrote:
> Identify the SoC type and revision, and register this information with
> the SoC bus, so it is available under /sys/devices/soc0/, and can be
> checked where needed using soc_device_match().
>
> In addition, on SoCs that support it, the product ID is read from a
> hardware register and validated, to catch accidental use of a DTB for a
> different SoC.
>
> Example:
>
>     Detected Renesas r8a7791 ES1.0
>     ...
>     # cat /sys/devices/soc0/{family,machine,soc_id,revision}
>     R-Car Gen2
>     Koelsch
>     r8a7791
>     ES1.0
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> This patch does NOT add a call to
>
>         of_platform_default_populate(NULL, NULL,
>                                      soc_device_to_device(soc_dev));
>
> Contrary to suggested by commit 74d1d82cdaaec727 ("drivers/base: add bus
> for System-on-Chip devices), doing so would not only move on-SoC devices
> from /sys/devices/platform/ to /sys/devices/soc0/, but also all other
> board (off-SoC) devices specified in the DTB.
> ---
>  arch/arm/mach-shmobile/Kconfig    |   1 +
>  arch/arm64/Kconfig.platforms      |   1 +
>  drivers/soc/renesas/Makefile      |   2 +
>  drivers/soc/renesas/renesas-soc.c | 266 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 270 insertions(+)
>  create mode 100644 drivers/soc/renesas/renesas-soc.c
>
> diff --git a/arch/arm/mach-shmobile/Kconfig b/arch/arm/mach-shmobile/Kconfig
> index b5a3cbe81dd1d1f0..e41d2cbb2c825981 100644
> --- a/arch/arm/mach-shmobile/Kconfig
> +++ b/arch/arm/mach-shmobile/Kconfig
> @@ -42,6 +42,7 @@ menuconfig ARCH_RENESAS
>  	select HAVE_ARM_TWD if SMP
>  	select NO_IOPORT_MAP
>  	select PINCTRL
> +	select SOC_BUS
>  	select ZONE_DMA if ARM_LPAE
>
>  if ARCH_RENESAS
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index be5d824ebdba2dab..a2675afc61baba8d 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -131,6 +131,7 @@ config ARCH_RENESAS
>  	select PM
>  	select PM_GENERIC_DOMAINS
>  	select RENESAS_IRQC
> +	select SOC_BUS
>  	help
>  	  This enables support for the ARMv8 based Renesas SoCs.
>
> diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile
> index 623039c3514cdc34..ae6ae8a11f98aba1 100644
> --- a/drivers/soc/renesas/Makefile
> +++ b/drivers/soc/renesas/Makefile
> @@ -1,3 +1,5 @@
> +obj-y				+= renesas-soc.o
> +
>  obj-$(CONFIG_ARCH_R8A7779)	+= rcar-sysc.o r8a7779-sysc.o
>  obj-$(CONFIG_ARCH_R8A7790)	+= rcar-sysc.o r8a7790-sysc.o
>  obj-$(CONFIG_ARCH_R8A7791)	+= rcar-sysc.o r8a7791-sysc.o
> diff --git a/drivers/soc/renesas/renesas-soc.c b/drivers/soc/renesas/renesas-soc.c
> new file mode 100644
> index 0000000000000000..74b72e4112b8889e
> --- /dev/null
> +++ b/drivers/soc/renesas/renesas-soc.c
> @@ -0,0 +1,266 @@
> +/*
> + * Renesas SoC Identification
> + *
> + * Copyright (C) 2014-2016 Glider bvba
> + *
> + * 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/of.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/sys_soc.h>
> +
> +
> +struct renesas_family {
> +	const char name[16];
> +	u32 reg;			/* CCCR, PVR, or PRR */


I'm wondering if we want to encode this information in the device tree?

 From the structs below it looks like this is information would be 
typically given in the device tree, and not hard coded in this C code?

On the other hand, above you mention

"catch accidental use of a DTB for a different SoC"

which is a nice feature, too.

So I just want to talk about the pros & cons, most probably both ways 
are fine.


> +};
> +
> +static const struct renesas_family fam_emev2 __initconst = {
> +	.name	= "Emma Mobile EV2",
> +};
> +
> +static const struct renesas_family fam_rmobile __initconst = {
> +	.name	= "R-Mobile",
> +	.reg	= 0xe600101c,		/* CCCR (Common Chip Code Register) */
> +};
> +
> +static const struct renesas_family fam_rcar_gen1 __initconst = {
> +	.name	= "R-Car Gen1",
> +	.reg	= 0xff000044,		/* PRR (Product Register) */
> +};
> +
> +static const struct renesas_family fam_rcar_gen2 __initconst = {
> +	.name	= "R-Car Gen2",
> +	.reg	= 0xff000044,		/* PRR (Product Register) */
> +};
> +
> +static const struct renesas_family fam_rcar_gen3 __initconst = {
> +	.name	= "R-Car Gen3",
> +	.reg	= 0xfff00044,		/* PRR (Product Register) */
> +};
> +
> +static const struct renesas_family fam_rza __initconst = {
> +	.name	= "RZ/A",
> +};
> +
> +static const struct renesas_family fam_rzg __initconst = {
> +	.name	= "RZ/G",
> +	.reg	= 0xff000044,		/* PRR (Product Register) */
> +};
> +
> +static const struct renesas_family fam_shmobile __initconst = {
> +	.name	= "SH-Mobile",
> +	.reg	= 0xe600101c,		/* CCCR (Common Chip Code Register) */
> +};
> +
> +
> +struct renesas_soc {
> +	const struct renesas_family *family;
> +	u8 id;
> +};
> +
> +static const struct renesas_soc soc_emev2 __initconst = {
> +	.family	= &fam_emev2,
> +};
> +
> +static const struct renesas_soc soc_rz_a1h __initconst = {
> +	.family	= &fam_rza,
> +};
> +
> +static const struct renesas_soc soc_rmobile_ape6 __initconst = {
> +	.family	= &fam_rmobile,
> +	.id	= 0x3f,
> +};
> +
> +static const struct renesas_soc soc_rmobile_a1 __initconst = {
> +	.family	= &fam_rmobile,
> +	.id	= 0x40,
> +};
> +
> +static const struct renesas_soc soc_rz_g1m __initconst = {
> +	.family	= &fam_rzg,
> +	.id	= 0x47,
> +};
> +
> +static const struct renesas_soc soc_rz_g1e __initconst = {
> +	.family	= &fam_rzg,
> +	.id	= 0x4c,
> +};
> +
> +static const struct renesas_soc soc_rcar_m1a __initconst = {
> +	.family	= &fam_rcar_gen1,
> +};
> +
> +static const struct renesas_soc soc_rcar_h1 __initconst = {
> +	.family	= &fam_rcar_gen1,
> +	.id	= 0x3b,
> +};
> +
> +static const struct renesas_soc soc_rcar_h2 __initconst = {
> +	.family	= &fam_rcar_gen2,
> +	.id	= 0x45,
> +};
> +
> +static const struct renesas_soc soc_rcar_m2_w __initconst = {
> +	.family	= &fam_rcar_gen2,
> +	.id	= 0x47,
> +};
> +
> +static const struct renesas_soc soc_rcar_v2h __initconst = {
> +	.family	= &fam_rcar_gen2,
> +	.id	= 0x4a,
> +};
> +
> +static const struct renesas_soc soc_rcar_m2_n __initconst = {
> +	.family	= &fam_rcar_gen2,
> +	.id	= 0x4b,
> +};
> +
> +static const struct renesas_soc soc_rcar_e2 __initconst = {
> +	.family	= &fam_rcar_gen2,
> +	.id	= 0x4c,
> +};
> +
> +static const struct renesas_soc soc_rcar_h3 __initconst = {
> +	.family	= &fam_rcar_gen3,
> +	.id	= 0x4f,
> +};
> +
> +static const struct renesas_soc soc_rcar_m3_w __initconst = {
> +	.family	= &fam_rcar_gen3,
> +	.id	= 0x52,
> +};
> +
> +static const struct renesas_soc soc_shmobile_ag5 __initconst = {
> +	.family	= &fam_shmobile,
> +	.id	= 0x37,
> +};
> +
> +static const struct of_device_id renesas_socs[] __initconst = {
> +#ifdef CONFIG_ARCH_EMEV2
> +	{ .compatible = "renesas,emev2",	.data = &soc_emev2 },
> +#endif
> +#ifdef CONFIG_ARCH_R7S72100
> +	{ .compatible = "renesas,r7s72100",	.data = &soc_rz_a1h },
> +#endif
> +#ifdef CONFIG_ARCH_R8A73A4
> +	{ .compatible = "renesas,r8a73a4",	.data = &soc_rmobile_ape6 },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7740
> +	{ .compatible = "renesas,r8a7740",	.data = &soc_rmobile_a1 },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7743
> +	{ .compatible = "renesas,r8a7743",	.data = &soc_rz_g1m },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7745
> +	{ .compatible = "renesas,r8a7745",	.data = &soc_rz_g1e },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7778
> +	{ .compatible = "renesas,r8a7778",	.data = &soc_rcar_m1a },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7779
> +	{ .compatible = "renesas,r8a7779",	.data = &soc_rcar_h1 },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7790
> +	{ .compatible = "renesas,r8a7790",	.data = &soc_rcar_h2 },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7791
> +	{ .compatible = "renesas,r8a7791",	.data = &soc_rcar_m2_w },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7792
> +	{ .compatible = "renesas,r8a7792",	.data = &soc_rcar_v2h },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7793
> +	{ .compatible = "renesas,r8a7793",	.data = &soc_rcar_m2_n },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7794
> +	{ .compatible = "renesas,r8a7794",	.data = &soc_rcar_e2 },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7795
> +	{ .compatible = "renesas,r8a7795",	.data = &soc_rcar_h3 },
> +#endif
> +#ifdef CONFIG_ARCH_R8A7796
> +	{ .compatible = "renesas,r8a7796",	.data = &soc_rcar_m3_w },
> +#endif
> +#ifdef CONFIG_ARCH_SH73A0
> +	{ .compatible = "renesas,sh73a0",	.data = &soc_shmobile_ag5 },
> +#endif
> +	{ /* sentinel */ }
> +};
> +
> +static int __init renesas_soc_init(void)
> +{
> +	struct soc_device_attribute *soc_dev_attr;
> +	const struct renesas_family *family;
> +	unsigned int product, esi = 0, esf;
> +	const struct of_device_id *match;
> +	const struct renesas_soc *soc;
> +	struct soc_device *soc_dev;
> +	struct device_node *np;
> +	void __iomem *mapped;
> +
> +	np = of_find_matching_node_and_match(NULL, renesas_socs, &match);
> +	if (!np)
> +		return -ENODEV;
> +
> +	of_node_put(np);
> +	soc = match->data;
> +	family = soc->family;
> +
> +	if (soc->id) {
> +		mapped = ioremap(family->reg, 4);
> +		if (!mapped)
> +			return -ENOMEM;
> +
> +		product = readl(mapped);
> +		iounmap(mapped);
> +
> +		if (((product >> 8) & 0xff) != soc->id) {
> +			pr_crit("SoC mismatch (product = 0x%x)\n", product);
> +			return -ENODEV;
> +		}
> +
> +		esi = ((product >> 4) & 0x0f) + 1;
> +		esf = product & 0xf;


I'm somehow surprised to see that all SoCs covered here use the same way 
to encode esi and esf? I would have expected that we need different 
decoding for different SoCs. But if this isn't the case, even better :)


Best regards

Dirk

[PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the SoC bus

[Arnd Bergmann]

On Tuesday, October 4, 2016 11:09:27 AM CEST Geert Uytterhoeven wrote:
> Identify the SoC type and revision, and register this information with
> the SoC bus, so it is available under /sys/devices/soc0/, and can be
> checked where needed using soc_device_match().
> 
> In addition, on SoCs that support it, the product ID is read from a
> hardware register and validated, to catch accidental use of a DTB for a
> different SoC.
> 
> Example:
> 
>     Detected Renesas r8a7791 ES1.0
>     ...
>     # cat /sys/devices/soc0/{family,machine,soc_id,revision}
>     R-Car Gen2
>     Koelsch
>     r8a7791
>     ES1.0
> 

Seems all reasonable.

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> This patch does NOT add a call to
> 
>         of_platform_default_populate(NULL, NULL,
>                                      soc_device_to_device(soc_dev));
> 
> Contrary to suggested by commit 74d1d82cdaaec727 ("drivers/base: add bus
> for System-on-Chip devices), doing so would not only move on-SoC devices
> from /sys/devices/platform/ to /sys/devices/soc0/, but also all other
> board (off-SoC) devices specified in the DTB.

Right, we have moved away from that a while ago, and now just
use the device for identification, not to model the device
hierarchy.

> diff --git a/drivers/soc/renesas/renesas-soc.c b/drivers/soc/renesas/renesas-soc.c
> new file mode 100644
> index 0000000000000000..74b72e4112b8889e
> --- /dev/null
> +++ b/drivers/soc/renesas/renesas-soc.c
> @@ -0,0 +1,266 @@
> +/*
> + * Renesas SoC Identification
> + *
> + * Copyright (C) 2014-2016 Glider bvba
> + *
> + * 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/of.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/sys_soc.h>
> +
> +
> +struct renesas_family {
> +	const char name[16];
> +	u32 reg;			/* CCCR, PVR, or PRR */
> +};
> +
> +static const struct renesas_family fam_emev2 __initconst = {
> +	.name	= "Emma Mobile EV2",
> +};

As this is not related to the others and doesn't have the respective
register, I'd leave the platform out of this, and possibly have
a separate driver for it.

> +static const struct renesas_family fam_rza __initconst = {
> +	.name	= "RZ/A",
> +};

I'm not sure about the relationship between this one and the others,
maybe it should be treated in the same way as emev2 and left out from
this driver?

> +static const struct renesas_family fam_rmobile __initconst = {
> +	.name	= "R-Mobile",
> +	.reg	= 0xe600101c,		/* CCCR (Common Chip Code Register) */
> +};
> +
> +static const struct renesas_family fam_rcar_gen1 __initconst = {
> +	.name	= "R-Car Gen1",
> +	.reg	= 0xff000044,		/* PRR (Product Register) */
> +};
> +
> +static const struct renesas_family fam_rcar_gen2 __initconst = {
> +	.name	= "R-Car Gen2",
> +	.reg	= 0xff000044,		/* PRR (Product Register) */
> +};
> +
> +static const struct renesas_family fam_rcar_gen3 __initconst = {
> +	.name	= "R-Car Gen3",
> +	.reg	= 0xfff00044,		/* PRR (Product Register) */
> +};
> +
> +static const struct renesas_family fam_rzg __initconst = {
> +	.name	= "RZ/G",
> +	.reg	= 0xff000044,		/* PRR (Product Register) */
> +};
> +
> +static const struct renesas_family fam_shmobile __initconst = {
> +	.name	= "SH-Mobile",
> +	.reg	= 0xe600101c,		/* CCCR (Common Chip Code Register) */
> +};

These seem to fall into two distinct categories, maybe there is a
better way to group them. What device contain the two kinds of
registers (PRR, CCCR)?

Hardcoding the register address seems rather ugly here, so maybe
there is a way to have two separate probe methods based on the
surrounding register range, and then bind to that?

> +static const struct of_device_id renesas_socs[] __initconst = {
> +#ifdef CONFIG_ARCH_EMEV2
> +	{ .compatible = "renesas,emev2",	.data = &soc_emev2 },
> +#endif
> +#ifdef CONFIG_ARCH_R7S72100
> +	{ .compatible = "renesas,r7s72100",	.data = &soc_rz_a1h },
> +#endif
> +#ifdef CONFIG_ARCH_R8A73A4

I think the #ifdefs here will result in warnings for unused symbols 
when the Kconfig symbols are disabled.

	Arnd

[PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the SoC bus

[Geert Uytterhoeven]

Hi Arnd,

On Mon, Oct 10, 2016 at 4:23 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday, October 4, 2016 11:09:27 AM CEST Geert Uytterhoeven wrote:
>> Identify the SoC type and revision, and register this information with
>> the SoC bus, so it is available under /sys/devices/soc0/, and can be
>> checked where needed using soc_device_match().
>>
>> In addition, on SoCs that support it, the product ID is read from a
>> hardware register and validated, to catch accidental use of a DTB for a
>> different SoC.
>>
>> Example:
>>
>>     Detected Renesas r8a7791 ES1.0
>>     ...
>>     # cat /sys/devices/soc0/{family,machine,soc_id,revision}
>>     R-Car Gen2
>>     Koelsch
>>     r8a7791
>>     ES1.0
>>
>
> Seems all reasonable.
>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> This patch does NOT add a call to
>>
>>         of_platform_default_populate(NULL, NULL,
>>                                      soc_device_to_device(soc_dev));
>>
>> Contrary to suggested by commit 74d1d82cdaaec727 ("drivers/base: add bus
>> for System-on-Chip devices), doing so would not only move on-SoC devices
>> from /sys/devices/platform/ to /sys/devices/soc0/, but also all other
>> board (off-SoC) devices specified in the DTB.
>
> Right, we have moved away from that a while ago, and now just
> use the device for identification, not to model the device
> hierarchy.

OK.

>> diff --git a/drivers/soc/renesas/renesas-soc.c b/drivers/soc/renesas/renesas-soc.c
>> new file mode 100644
>> index 0000000000000000..74b72e4112b8889e
>> --- /dev/null
>> +++ b/drivers/soc/renesas/renesas-soc.c
>> @@ -0,0 +1,266 @@
>> +/*
>> + * Renesas SoC Identification
>> + *
>> + * Copyright (C) 2014-2016 Glider bvba
>> + *
>> + * 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/of.h>
>> +#include <linux/slab.h>
>> +#include <linux/string.h>
>> +#include <linux/sys_soc.h>
>> +
>> +
>> +struct renesas_family {
>> +     const char name[16];
>> +     u32 reg;                        /* CCCR, PVR, or PRR */
>> +};
>> +
>> +static const struct renesas_family fam_emev2 __initconst = {
>> +     .name   = "Emma Mobile EV2",
>> +};
>
> As this is not related to the others and doesn't have the respective
> register, I'd leave the platform out of this, and possibly have
> a separate driver for it.

OK. Emma Mobile is special, as it doesn't share any drivers with the
other SoCs (NEC vs. Hitachi origin).

>> +static const struct renesas_family fam_rza __initconst = {
>> +     .name   = "RZ/A",
>> +};
>
> I'm not sure about the relationship between this one and the others,
> maybe it should be treated in the same way as emev2 and left out from
> this driver?

While RZ/A doesn't have a version registers (AFAIK), it shares several
drivers with the other SoCs (SH/R-Mobile, R-Car).
Hence I'd like to keep it, so we can match for it in these drivers when
needed. It has e.g. a different variant of the serial port (SCIF), more
closely to the one on SH2 rather than SH4.

>> +static const struct renesas_family fam_rmobile __initconst = {
>> +     .name   = "R-Mobile",
>> +     .reg    = 0xe600101c,           /* CCCR (Common Chip Code Register) */
>> +};
>> +
>> +static const struct renesas_family fam_rcar_gen1 __initconst = {
>> +     .name   = "R-Car Gen1",
>> +     .reg    = 0xff000044,           /* PRR (Product Register) */
>> +};
>> +
>> +static const struct renesas_family fam_rcar_gen2 __initconst = {
>> +     .name   = "R-Car Gen2",
>> +     .reg    = 0xff000044,           /* PRR (Product Register) */
>> +};
>> +
>> +static const struct renesas_family fam_rcar_gen3 __initconst = {
>> +     .name   = "R-Car Gen3",
>> +     .reg    = 0xfff00044,           /* PRR (Product Register) */
>> +};
>> +
>> +static const struct renesas_family fam_rzg __initconst = {
>> +     .name   = "RZ/G",
>> +     .reg    = 0xff000044,           /* PRR (Product Register) */
>> +};
>> +
>> +static const struct renesas_family fam_shmobile __initconst = {
>> +     .name   = "SH-Mobile",
>> +     .reg    = 0xe600101c,           /* CCCR (Common Chip Code Register) */
>> +};
>
> These seem to fall into two distinct categories, maybe there is a
> better way to group them. What device contain the two kinds of
> registers (PRR, CCCR)?

Actually there are three (notice the extra "f" on R-Car Gen3 ;-)

Some SoCs have only CCCR, others have only PRR, some have both.
On some SoCs one of them can be accessed from the RealTime CPU
core (SH) only.
On some SoCs the register is not documented, but present.
If the PRR exists, it's a better choice, as it contains additional information
in the high order bits (representing the presence of each big (CA15/CA57),
little (CA7/CA53), and RT (CR7) CPU core). Currently we don't use that
information, though.

Grouping them in some other way means we would loose the family name,
which is exposed through soc_dev_attr->family.
The usefulness of family names is debatable though, as this is more an
issue of marketing business.

> Hardcoding the register address seems rather ugly here, so maybe
> there is a way to have two separate probe methods based on the
> surrounding register range, and then bind to that?

There's no simple relation between CCCR/PRR and other register blocks.
I prefer not to add these to DT, as that would add one more worm to the
backwards compatibility can.

>> +static const struct of_device_id renesas_socs[] __initconst = {
>> +#ifdef CONFIG_ARCH_EMEV2
>> +     { .compatible = "renesas,emev2",        .data = &soc_emev2 },
>> +#endif
>> +#ifdef CONFIG_ARCH_R7S72100
>> +     { .compatible = "renesas,r7s72100",     .data = &soc_rz_a1h },
>> +#endif
>> +#ifdef CONFIG_ARCH_R8A73A4
>
> I think the #ifdefs here will result in warnings for unused symbols
> when the Kconfig symbols are disabled.

Originally I had __maybe_unused, but it didn't seem to be needed.
Do you know which compiler needs it, so I can check?

Thanks for your comments!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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

[PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the SoC bus

[Arnd Bergmann]

On Wednesday, October 19, 2016 10:02:57 AM CEST Geert Uytterhoeven wrote:
> On Mon, Oct 10, 2016 at 4:23 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday, October 4, 2016 11:09:27 AM CEST Geert Uytterhoeven wrote:

> >> +static const struct renesas_family fam_rza __initconst = {
> >> +     .name   = "RZ/A",
> >> +};
> >
> > I'm not sure about the relationship between this one and the others,
> > maybe it should be treated in the same way as emev2 and left out from
> > this driver?
> 
> While RZ/A doesn't have a version registers (AFAIK), it shares several
> drivers with the other SoCs (SH/R-Mobile, R-Car).
> Hence I'd like to keep it, so we can match for it in these drivers when
> needed. It has e.g. a different variant of the serial port (SCIF), more
> closely to the one on SH2 rather than SH4.

I'd prefer seeing a separate soc driver for that one.

> >> +static const struct renesas_family fam_rmobile __initconst = {
> >> +     .name   = "R-Mobile",
> >> +     .reg    = 0xe600101c,           /* CCCR (Common Chip Code Register) */
> >> +};
> >> +
> >> +static const struct renesas_family fam_rcar_gen1 __initconst = {
> >> +     .name   = "R-Car Gen1",
> >> +     .reg    = 0xff000044,           /* PRR (Product Register) */
> >> +};
> >> +
> >> +static const struct renesas_family fam_rcar_gen2 __initconst = {
> >> +     .name   = "R-Car Gen2",
> >> +     .reg    = 0xff000044,           /* PRR (Product Register) */
> >> +};
> >> +
> >> +static const struct renesas_family fam_rcar_gen3 __initconst = {
> >> +     .name   = "R-Car Gen3",
> >> +     .reg    = 0xfff00044,           /* PRR (Product Register) */
> >> +};
> >> +
> >> +static const struct renesas_family fam_rzg __initconst = {
> >> +     .name   = "RZ/G",
> >> +     .reg    = 0xff000044,           /* PRR (Product Register) */
> >> +};
> >> +
> >> +static const struct renesas_family fam_shmobile __initconst = {
> >> +     .name   = "SH-Mobile",
> >> +     .reg    = 0xe600101c,           /* CCCR (Common Chip Code Register) */
> >> +};
> >
> > These seem to fall into two distinct categories, maybe there is a
> > better way to group them. What device contain the two kinds of
> > registers (PRR, CCCR)?
> 
> Actually there are three (notice the extra "f" on R-Car Gen3 ;-)

I see. Hopefully this is just the same register block at a different
location though.

> Some SoCs have only CCCR, others have only PRR, some have both.
> On some SoCs one of them can be accessed from the RealTime CPU
> core (SH) only.
> On some SoCs the register is not documented, but present.
> If the PRR exists, it's a better choice, as it contains additional information
> in the high order bits (representing the presence of each big (CA15/CA57),
> little (CA7/CA53), and RT (CR7) CPU core). Currently we don't use that
> information, though.
> 
> Grouping them in some other way means we would loose the family name,
> which is exposed through soc_dev_attr->family.
> The usefulness of family names is debatable though, as this is more an
> issue of marketing business.

How about having a table to look up the family name by the value
of the PRR or CCCR then?

> > Hardcoding the register address seems rather ugly here, so maybe
> > there is a way to have two separate probe methods based on the
> > surrounding register range, and then bind to that?
> 
> There's no simple relation between CCCR/PRR and other register blocks.
> I prefer not to add these to DT, as that would add one more worm to the
> backwards compatibility can.

Hmm, I understand the concern about compatibility with existing DT files,
but I also really hate to see hardcoded register addresses.

Any reason against requiring the DT node for future chips though?
How about this:

The driver could report the hardcoded strings for the SoCs it already
knows about (you have the table anyway) and not report the revision
unless there is a regmap containing the CCCR or the PRR, in which
case you use that. Future SoCs will provide the PRR (I assume
CCCR is only used on the older ones) through a syscon regmap
that we can use to find out the exact revision as well.

The existing DT files can gain the syscon device so you can report
the revision on those machines as well, unless you use an old DTB.

> >> +static const struct of_device_id renesas_socs[] __initconst = {
> >> +#ifdef CONFIG_ARCH_EMEV2
> >> +     { .compatible = "renesas,emev2",        .data = &soc_emev2 },
> >> +#endif
> >> +#ifdef CONFIG_ARCH_R7S72100
> >> +     { .compatible = "renesas,r7s72100",     .data = &soc_rz_a1h },
> >> +#endif
> >> +#ifdef CONFIG_ARCH_R8A73A4
> >
> > I think the #ifdefs here will result in warnings for unused symbols
> > when the Kconfig symbols are disabled.
> 
> Originally I had __maybe_unused, but it didn't seem to be needed.
> Do you know which compiler needs it, so I can check?

Ah, I remember now: gcc doesn't warn for 'static const' variables
unless we pass -Wunused-const, which should be enabled with "make W=1",
and we might make that the default in the future (after fixing the
handful of drivers currently relying on this).

Why not just drop all the #ifdef here? There should be very little
overhead in size, especially if all the data is __initconst.

	Arnd

[PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the SoC bus

[Geert Uytterhoeven]

Hi Arnd,

On Wed, Oct 19, 2016 at 12:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday, October 19, 2016 10:02:57 AM CEST Geert Uytterhoeven wrote:
>> On Mon, Oct 10, 2016 at 4:23 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Tuesday, October 4, 2016 11:09:27 AM CEST Geert Uytterhoeven wrote:
>> >> +static const struct renesas_family fam_rza __initconst = {
>> >> +     .name   = "RZ/A",
>> >> +};
>> >
>> > I'm not sure about the relationship between this one and the others,
>> > maybe it should be treated in the same way as emev2 and left out from
>> > this driver?
>>
>> While RZ/A doesn't have a version registers (AFAIK), it shares several
>> drivers with the other SoCs (SH/R-Mobile, R-Car).
>> Hence I'd like to keep it, so we can match for it in these drivers when
>> needed. It has e.g. a different variant of the serial port (SCIF), more
>> closely to the one on SH2 rather than SH4.
>
> I'd prefer seeing a separate soc driver for that one.

OK, that can be done.

>> >> +static const struct renesas_family fam_rmobile __initconst = {
>> >> +     .name   = "R-Mobile",
>> >> +     .reg    = 0xe600101c,           /* CCCR (Common Chip Code Register) */
>> >> +};
>> >> +
>> >> +static const struct renesas_family fam_rcar_gen1 __initconst = {
>> >> +     .name   = "R-Car Gen1",
>> >> +     .reg    = 0xff000044,           /* PRR (Product Register) */
>> >> +};
>> >> +
>> >> +static const struct renesas_family fam_rcar_gen2 __initconst = {
>> >> +     .name   = "R-Car Gen2",
>> >> +     .reg    = 0xff000044,           /* PRR (Product Register) */
>> >> +};
>> >> +
>> >> +static const struct renesas_family fam_rcar_gen3 __initconst = {
>> >> +     .name   = "R-Car Gen3",
>> >> +     .reg    = 0xfff00044,           /* PRR (Product Register) */
>> >> +};
>> >> +
>> >> +static const struct renesas_family fam_rzg __initconst = {
>> >> +     .name   = "RZ/G",
>> >> +     .reg    = 0xff000044,           /* PRR (Product Register) */
>> >> +};
>> >> +
>> >> +static const struct renesas_family fam_shmobile __initconst = {
>> >> +     .name   = "SH-Mobile",
>> >> +     .reg    = 0xe600101c,           /* CCCR (Common Chip Code Register) */
>> >> +};
>> >
>> > These seem to fall into two distinct categories, maybe there is a
>> > better way to group them. What device contain the two kinds of
>> > registers (PRR, CCCR)?
>>
>> Actually there are three (notice the extra "f" on R-Car Gen3 ;-)
>
> I see. Hopefully this is just the same register block at a different
> location though.

More or less.

>> Some SoCs have only CCCR, others have only PRR, some have both.
>> On some SoCs one of them can be accessed from the RealTime CPU
>> core (SH) only.
>> On some SoCs the register is not documented, but present.
>> If the PRR exists, it's a better choice, as it contains additional information
>> in the high order bits (representing the presence of each big (CA15/CA57),
>> little (CA7/CA53), and RT (CR7) CPU core). Currently we don't use that
>> information, though.
>>
>> Grouping them in some other way means we would loose the family name,
>> which is exposed through soc_dev_attr->family.
>> The usefulness of family names is debatable though, as this is more an
>> issue of marketing business.
>
> How about having a table to look up the family name by the value
> of the PRR or CCCR then?

Unfortunately there exist SoCs from different families using the same
product ID.

And different SoCs from the same family may have a revision register
or not (e.g. R-Car H1 has, M1A hasn't).

>> > Hardcoding the register address seems rather ugly here, so maybe
>> > there is a way to have two separate probe methods based on the
>> > surrounding register range, and then bind to that?
>>
>> There's no simple relation between CCCR/PRR and other register blocks.
>> I prefer not to add these to DT, as that would add one more worm to the
>> backwards compatibility can.
>
> Hmm, I understand the concern about compatibility with existing DT files,
> but I also really hate to see hardcoded register addresses.
>
> Any reason against requiring the DT node for future chips though?

For future SoCs, we  can easily make it mandatory.

> How about this:
>
> The driver could report the hardcoded strings for the SoCs it already
> knows about (you have the table anyway) and not report the revision
> unless there is a regmap containing the CCCR or the PRR, in which
> case you use that. Future SoCs will provide the PRR (I assume
> CCCR is only used on the older ones) through a syscon regmap
> that we can use to find out the exact revision as well.
>
> The existing DT files can gain the syscon device so you can report
> the revision on those machines as well, unless you use an old DTB.

Hmm... That means that if we have to add a driver quirk to distinguish
between different revisions of the same SoC, we have to update the
DTB anyway, to add the CCCR/PRR device node.
We might as well just change the compatible value in that DTB for the
device that needs the quirk. Which is what we'd like to avoid in the
first place.

>> >> +static const struct of_device_id renesas_socs[] __initconst = {
>> >> +#ifdef CONFIG_ARCH_EMEV2
>> >> +     { .compatible = "renesas,emev2",        .data = &soc_emev2 },
>> >> +#endif
>> >> +#ifdef CONFIG_ARCH_R7S72100
>> >> +     { .compatible = "renesas,r7s72100",     .data = &soc_rz_a1h },
>> >> +#endif
>> >> +#ifdef CONFIG_ARCH_R8A73A4
>> >
>> > I think the #ifdefs here will result in warnings for unused symbols
>> > when the Kconfig symbols are disabled.
>>
>> Originally I had __maybe_unused, but it didn't seem to be needed.
>> Do you know which compiler needs it, so I can check?
>
> Ah, I remember now: gcc doesn't warn for 'static const' variables
> unless we pass -Wunused-const, which should be enabled with "make W=1",
> and we might make that the default in the future (after fixing the
> handful of drivers currently relying on this).

OK.

> Why not just drop all the #ifdef here? There should be very little
> overhead in size, especially if all the data is __initconst.

It still saves ca. 3 KiB for a kernel for a single SoC.

It's been a while I've tried multi_v7_defconfig, but I'm afraid there's no
Renesas SoC left that can boot it, due to sheer size.
We're facing the same issue with arm64_defconfig soon (it already
fails now if you add a small (1.5 MiB) initrd).

Not to mention RZ/A1H, which is used on some boards running with
its 10 MiB of internal SRAM only.

Kernel size does matter, despite no progress on linux-tiny.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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

[PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the SoC bus

[Arnd Bergmann]

On Friday, October 21, 2016 8:16:00 PM CEST Geert Uytterhoeven wrote:
> On Wed, Oct 19, 2016 at 12:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday, October 19, 2016 10:02:57 AM CEST Geert Uytterhoeven wrote:
> >> On Mon, Oct 10, 2016 at 4:23 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > I'd prefer seeing a separate soc driver for that one.
> >> Some SoCs have only CCCR, others have only PRR, some have both.
> >> On some SoCs one of them can be accessed from the RealTime CPU
> >> core (SH) only.
> >> On some SoCs the register is not documented, but present.
> >> If the PRR exists, it's a better choice, as it contains additional information
> >> in the high order bits (representing the presence of each big (CA15/CA57),
> >> little (CA7/CA53), and RT (CR7) CPU core). Currently we don't use that
> >> information, though.
> >>
> >> Grouping them in some other way means we would loose the family name,
> >> which is exposed through soc_dev_attr->family.
> >> The usefulness of family names is debatable though, as this is more an
> >> issue of marketing business.
> >
> > How about having a table to look up the family name by the value
> > of the PRR or CCCR then?
> 
> Unfortunately there exist SoCs from different families using the same
> product ID.
> 
> And different SoCs from the same family may have a revision register
> or not (e.g. R-Car H1 has, M1A hasn't).

Is this something we expect to see more of in the future, or can
we expect future chips to handle this more consistently?

> > How about this:
> >
> > The driver could report the hardcoded strings for the SoCs it already
> > knows about (you have the table anyway) and not report the revision
> > unless there is a regmap containing the CCCR or the PRR, in which
> > case you use that. Future SoCs will provide the PRR (I assume
> > CCCR is only used on the older ones) through a syscon regmap
> > that we can use to find out the exact revision as well.
> >
> > The existing DT files can gain the syscon device so you can report
> > the revision on those machines as well, unless you use an old DTB.
> 
> Hmm... That means that if we have to add a driver quirk to distinguish
> between different revisions of the same SoC, we have to update the
> DTB anyway, to add the CCCR/PRR device node.
> We might as well just change the compatible value in that DTB for the
> device that needs the quirk. Which is what we'd like to avoid in the
> first place.

Do you have a specific example in mind? If this is only a theoretical
problem, we can worry about it when we get there, and then decide
if we add a hardcoded register after all.

> > Why not just drop all the #ifdef here? There should be very little
> > overhead in size, especially if all the data is __initconst.
> 
> It still saves ca. 3 KiB for a kernel for a single SoC.

Fair enough, that is more than I was expecting from looking at the
source. It's probably the of_device_id structures for the most part.

Please just add the __maybe_unused then, to save us a patch in case
we make -Wunused-const the default in the future.

	Arnd

[PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the SoC bus

[Geert Uytterhoeven]

Hi Arnd,

On Fri, Oct 21, 2016 at 11:16 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday, October 21, 2016 8:16:00 PM CEST Geert Uytterhoeven wrote:
>> On Wed, Oct 19, 2016 at 12:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Wednesday, October 19, 2016 10:02:57 AM CEST Geert Uytterhoeven wrote:
>> >> On Mon, Oct 10, 2016 at 4:23 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > I'd prefer seeing a separate soc driver for that one.
>> >> Some SoCs have only CCCR, others have only PRR, some have both.
>> >> On some SoCs one of them can be accessed from the RealTime CPU
>> >> core (SH) only.
>> >> On some SoCs the register is not documented, but present.
>> >> If the PRR exists, it's a better choice, as it contains additional information
>> >> in the high order bits (representing the presence of each big (CA15/CA57),
>> >> little (CA7/CA53), and RT (CR7) CPU core). Currently we don't use that
>> >> information, though.
>> >>
>> >> Grouping them in some other way means we would loose the family name,
>> >> which is exposed through soc_dev_attr->family.
>> >> The usefulness of family names is debatable though, as this is more an
>> >> issue of marketing business.
>> >
>> > How about having a table to look up the family name by the value
>> > of the PRR or CCCR then?
>>
>> Unfortunately there exist SoCs from different families using the same
>> product ID.
>>
>> And different SoCs from the same family may have a revision register
>> or not (e.g. R-Car H1 has, M1A hasn't).
>
> Is this something we expect to see more of in the future, or can
> we expect future chips to handle this more consistently?

I expect to see more of these in the future.

Perhaps I just should forget about the product IDs and (marketing) families,
and just stick the CCCR/PRR addresses in the of_device_ids?
Then we'll have SoC names (e.g. "r8a7791") and (optional) revisions
(e.g. "ES1.0") to match on.

>> > How about this:
>> >
>> > The driver could report the hardcoded strings for the SoCs it already
>> > knows about (you have the table anyway) and not report the revision
>> > unless there is a regmap containing the CCCR or the PRR, in which
>> > case you use that. Future SoCs will provide the PRR (I assume
>> > CCCR is only used on the older ones) through a syscon regmap
>> > that we can use to find out the exact revision as well.
>> >
>> > The existing DT files can gain the syscon device so you can report
>> > the revision on those machines as well, unless you use an old DTB.
>>
>> Hmm... That means that if we have to add a driver quirk to distinguish
>> between different revisions of the same SoC, we have to update the
>> DTB anyway, to add the CCCR/PRR device node.
>> We might as well just change the compatible value in that DTB for the
>> device that needs the quirk. Which is what we'd like to avoid in the
>> first place.
>
> Do you have a specific example in mind? If this is only a theoretical
> problem, we can worry about it when we get there, and then decide
> if we add a hardcoded register after all.

For R-Car H3, there are small differences between ES1.0 and ES1.1,
and more and larger differences between ES1.x and ES2.0, which
need different handling (patches already floating around).

For (old) R-Car H1, the SATA driver already handles "renesas,sata-r8a7790-es1",
but so far there didn't exist an established process to specify how that
compatible value would end up in the DTB (the in-kernel DTS doesn't have it).

There may be more differences I'm not aware of.

>> > Why not just drop all the #ifdef here? There should be very little
>> > overhead in size, especially if all the data is __initconst.
>>
>> It still saves ca. 3 KiB for a kernel for a single SoC.
>
> Fair enough, that is more than I was expecting from looking at the
> source. It's probably the of_device_id structures for the most part.

Yep, ca. 200 bytes per ID.

> Please just add the __maybe_unused then, to save us a patch in case
> we make -Wunused-const the default in the future.

Sure.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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

[PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the SoC bus

[Arnd Bergmann]

On Saturday, October 22, 2016 9:44:11 AM CEST Geert Uytterhoeven wrote:
> Hi Arnd,
> 
> On Fri, Oct 21, 2016 at 11:16 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday, October 21, 2016 8:16:00 PM CEST Geert Uytterhoeven wrote:
> >> On Wed, Oct 19, 2016 at 12:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > On Wednesday, October 19, 2016 10:02:57 AM CEST Geert Uytterhoeven wrote:
> >> >> On Mon, Oct 10, 2016 at 4:23 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > I'd prefer seeing a separate soc driver for that one.
> >> >> Some SoCs have only CCCR, others have only PRR, some have both.
> >> >> On some SoCs one of them can be accessed from the RealTime CPU
> >> >> core (SH) only.
> >> >> On some SoCs the register is not documented, but present.
> >> >> If the PRR exists, it's a better choice, as it contains additional information
> >> >> in the high order bits (representing the presence of each big (CA15/CA57),
> >> >> little (CA7/CA53), and RT (CR7) CPU core). Currently we don't use that
> >> >> information, though.
> >> >>
> >> >> Grouping them in some other way means we would loose the family name,
> >> >> which is exposed through soc_dev_attr->family.
> >> >> The usefulness of family names is debatable though, as this is more an
> >> >> issue of marketing business.
> >> >
> >> > How about having a table to look up the family name by the value
> >> > of the PRR or CCCR then?
> >>
> >> Unfortunately there exist SoCs from different families using the same
> >> product ID.
> >>
> >> And different SoCs from the same family may have a revision register
> >> or not (e.g. R-Car H1 has, M1A hasn't).
> >
> > Is this something we expect to see more of in the future, or can
> > we expect future chips to handle this more consistently?
> 
> I expect to see more of these in the future.
> 
> Perhaps I just should forget about the product IDs and (marketing) families,
> and just stick the CCCR/PRR addresses in the of_device_ids?
> Then we'll have SoC names (e.g. "r8a7791") and (optional) revisions
> (e.g. "ES1.0") to match on.

I don't think listing the marketing names is a problem if we need a
full list of all chips in of_device_ids anyway.

I'm still hoping to be able to limit the need for specifying the
register addresses in the driver instead.

> >> > How about this:
> >> >
> >> > The driver could report the hardcoded strings for the SoCs it already
> >> > knows about (you have the table anyway) and not report the revision
> >> > unless there is a regmap containing the CCCR or the PRR, in which
> >> > case you use that. Future SoCs will provide the PRR (I assume
> >> > CCCR is only used on the older ones) through a syscon regmap
> >> > that we can use to find out the exact revision as well.
> >> >
> >> > The existing DT files can gain the syscon device so you can report
> >> > the revision on those machines as well, unless you use an old DTB.
> >>
> >> Hmm... That means that if we have to add a driver quirk to distinguish
> >> between different revisions of the same SoC, we have to update the
> >> DTB anyway, to add the CCCR/PRR device node.
> >> We might as well just change the compatible value in that DTB for the
> >> device that needs the quirk. Which is what we'd like to avoid in the
> >> first place.
> >
> > Do you have a specific example in mind? If this is only a theoretical
> > problem, we can worry about it when we get there, and then decide
> > if we add a hardcoded register after all.
> 
> For R-Car H3, there are small differences between ES1.0 and ES1.1,
> and more and larger differences between ES1.x and ES2.0, which
> need different handling (patches already floating around).
> 
> For (old) R-Car H1, the SATA driver already handles "renesas,sata-r8a7790-es1",
> but so far there didn't exist an established process to specify how that
> compatible value would end up in the DTB (the in-kernel DTS doesn't have it).
> 
> There may be more differences I'm not aware of.

Ok, so for R-Car H1, I assume we don't need the driver, it would just
be a way to replace the current workaround with a different one, right?

For R-Car H3, do we just require driver changes to work with ES2.0,
or also DT changes? If the new chip version already implies a new DT,
we can require the presence of a device node that has the correct
register number.

	Arnd

[PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the SoC bus

[Geert Uytterhoeven]

Hi Arnd,

On Sat, Oct 29, 2016 at 11:27 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday, October 22, 2016 9:44:11 AM CEST Geert Uytterhoeven wrote:
>> On Fri, Oct 21, 2016 at 11:16 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Friday, October 21, 2016 8:16:00 PM CEST Geert Uytterhoeven wrote:
>> >> On Wed, Oct 19, 2016 at 12:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> > On Wednesday, October 19, 2016 10:02:57 AM CEST Geert Uytterhoeven wrote:
>> >> >> On Mon, Oct 10, 2016 at 4:23 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> > I'd prefer seeing a separate soc driver for that one.
>> >> >> Some SoCs have only CCCR, others have only PRR, some have both.
>> >> >> On some SoCs one of them can be accessed from the RealTime CPU
>> >> >> core (SH) only.
>> >> >> On some SoCs the register is not documented, but present.
>> >> >> If the PRR exists, it's a better choice, as it contains additional information
>> >> >> in the high order bits (representing the presence of each big (CA15/CA57),
>> >> >> little (CA7/CA53), and RT (CR7) CPU core). Currently we don't use that
>> >> >> information, though.
>> >> >>
>> >> >> Grouping them in some other way means we would loose the family name,
>> >> >> which is exposed through soc_dev_attr->family.
>> >> >> The usefulness of family names is debatable though, as this is more an
>> >> >> issue of marketing business.
>> >> >
>> >> > How about having a table to look up the family name by the value
>> >> > of the PRR or CCCR then?
>> >>
>> >> Unfortunately there exist SoCs from different families using the same
>> >> product ID.
>> >>
>> >> And different SoCs from the same family may have a revision register
>> >> or not (e.g. R-Car H1 has, M1A hasn't).
>> >
>> > Is this something we expect to see more of in the future, or can
>> > we expect future chips to handle this more consistently?
>>
>> I expect to see more of these in the future.
>>
>> Perhaps I just should forget about the product IDs and (marketing) families,
>> and just stick the CCCR/PRR addresses in the of_device_ids?
>> Then we'll have SoC names (e.g. "r8a7791") and (optional) revisions
>> (e.g. "ES1.0") to match on.
>
> I don't think listing the marketing names is a problem if we need a
> full list of all chips in of_device_ids anyway.

I'm removing the marketing names. We don't match them anyway (and probably
shouldn't, as we don't control them anyway, cfr.
https://www.renesas.com/en-us/solutions/automotive/products.html).

> I'm still hoping to be able to limit the need for specifying the
> register addresses in the driver instead.

Adding DT binding...

>> >> > How about this:
>> >> >
>> >> > The driver could report the hardcoded strings for the SoCs it already
>> >> > knows about (you have the table anyway) and not report the revision
>> >> > unless there is a regmap containing the CCCR or the PRR, in which
>> >> > case you use that. Future SoCs will provide the PRR (I assume
>> >> > CCCR is only used on the older ones) through a syscon regmap
>> >> > that we can use to find out the exact revision as well.
>> >> >
>> >> > The existing DT files can gain the syscon device so you can report
>> >> > the revision on those machines as well, unless you use an old DTB.
>> >>
>> >> Hmm... That means that if we have to add a driver quirk to distinguish
>> >> between different revisions of the same SoC, we have to update the
>> >> DTB anyway, to add the CCCR/PRR device node.
>> >> We might as well just change the compatible value in that DTB for the
>> >> device that needs the quirk. Which is what we'd like to avoid in the
>> >> first place.
>> >
>> > Do you have a specific example in mind? If this is only a theoretical
>> > problem, we can worry about it when we get there, and then decide
>> > if we add a hardcoded register after all.
>>
>> For R-Car H3, there are small differences between ES1.0 and ES1.1,
>> and more and larger differences between ES1.x and ES2.0, which
>> need different handling (patches already floating around).
>>
>> For (old) R-Car H1, the SATA driver already handles "renesas,sata-r8a7790-es1",
>> but so far there didn't exist an established process to specify how that
>> compatible value would end up in the DTB (the in-kernel DTS doesn't have it).
>>
>> There may be more differences I'm not aware of.
>
> Ok, so for R-Car H1, I assume we don't need the driver, it would just
> be a way to replace the current workaround with a different one, right?
>
> For R-Car H3, do we just require driver changes to work with ES2.0,
> or also DT changes? If the new chip version already implies a new DT,
> we can require the presence of a device node that has the correct
> register number.

H3 also needs DT changes for some features (e.g. different number of USB
channels, different topology for graphics).

soc_device_match() would mostly (only?) be used to handle limitations and
quirks in early revisions. These are intended to be removed once production
has been ramped up, and there's no longer a need to support them.
However, that also means soc_device_match() would be used to match against
early revisions, not against late revisions. I.e. the early SoCs need the chip
ID registers declared, not the new ones.

Stay tuned for v2...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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

[PATCH 0/4] soc: renesas: Identify SoC and register with the SoC bus

[Arnd Bergmann]

On Tuesday, October 4, 2016 11:09:23 AM CEST Geert Uytterhoeven wrote:
> 	Hi all,
> 
> Some Renesas SoCs may exist in different revisions, providing slightly
> different functionalities (e.g. R-Car H3 ES1.x and ES2.0). This needs to
> be catered for by drivers and/or platform code.  The recently proposed
> soc_device_match() API seems like a good fit to handle this.
> 
> This patch series implements the core infrastructure to provide SoC and
> revision information through the SoC bus for Renesas ARM SoCs. It
> consists of 4 patches:
>   - Patch 1 avoids a crash when SoC revision information is needed and
>     provided early,
>   - Patch 2 (from Arnd) introduces the soc_device_match() API.
>     I don't know if, when, and through which channel this patch is
>     planned to go upstream,
>   - Patch 3 fixes a bug in soc_device_match(), causing a crash when
>     trying to match on an SoC attribute that is not provided (seen on
>     EMEV2, RZ/A, and R-Car M1A, which lack revision information),
>   - Patch 4 identifies Renesas SoCs and registers them with the SoC bus.
> 
> Tested on (family, machine, soc_id, optional revision):
> 
>     Emma Mobile EV2, EMEV2 KZM9D Board, emev2
>     RZ/A, Genmai, r7s72100
>     R-Mobile, APE6EVM, r8a73a4, ES1.0
>     R-Mobile, armadillo 800 eva, r8a7740, ES2.0
>     R-Car Gen1, bockw, r8a7778
>     R-Car Gen1, marzen, r8a7779, ES1.0
>     R-Car Gen2, Lager, r8a7790, ES1.0
>     R-Car Gen2, Koelsch, r8a7791, ES1.0
>     R-Car Gen2, Gose, r8a7793, ES1.0
>     R-Car Gen2, Alt, r8a7794, ES1.0
>     R-Car Gen3, Renesas Salvator-X board based on r8a7795, r8a7795, ES1.0
>     R-Car Gen3, Renesas Salvator-X board based on r8a7796, r8a7796, ES1.0
>     SH-Mobile, KZM-A9-GT, sh73a0, ES2.0

As mentioned in the comment for the driver patch, I think this makes
a lot of sense for the machines that have a revision register, in
particular when the interpretation of that register is always done
the same way, but I'm a bit skeptical about doing it in the same driver
for machines that don't have the register.

Matching by a device rather than the SoC platform also has the advantage
that there is no need to maintain a list of compatible numbers in the
driver.

	Arnd

[PATCH 0/4] soc: renesas: Identify SoC and register with the SoC bus

[Geert Uytterhoeven]

Hi Arnd,

On Mon, Oct 10, 2016 at 4:28 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday, October 4, 2016 11:09:23 AM CEST Geert Uytterhoeven wrote:
>> Some Renesas SoCs may exist in different revisions, providing slightly
>> different functionalities (e.g. R-Car H3 ES1.x and ES2.0). This needs to
>> be catered for by drivers and/or platform code.  The recently proposed
>> soc_device_match() API seems like a good fit to handle this.
>>
>> This patch series implements the core infrastructure to provide SoC and
>> revision information through the SoC bus for Renesas ARM SoCs. It
>> consists of 4 patches:
>>   - Patch 1 avoids a crash when SoC revision information is needed and
>>     provided early,
>>   - Patch 2 (from Arnd) introduces the soc_device_match() API.
>>     I don't know if, when, and through which channel this patch is
>>     planned to go upstream,
>>   - Patch 3 fixes a bug in soc_device_match(), causing a crash when
>>     trying to match on an SoC attribute that is not provided (seen on
>>     EMEV2, RZ/A, and R-Car M1A, which lack revision information),
>>   - Patch 4 identifies Renesas SoCs and registers them with the SoC bus.
>>
>> Tested on (family, machine, soc_id, optional revision):
>>
>>     Emma Mobile EV2, EMEV2 KZM9D Board, emev2
>>     RZ/A, Genmai, r7s72100
>>     R-Mobile, APE6EVM, r8a73a4, ES1.0
>>     R-Mobile, armadillo 800 eva, r8a7740, ES2.0
>>     R-Car Gen1, bockw, r8a7778
>>     R-Car Gen1, marzen, r8a7779, ES1.0
>>     R-Car Gen2, Lager, r8a7790, ES1.0
>>     R-Car Gen2, Koelsch, r8a7791, ES1.0
>>     R-Car Gen2, Gose, r8a7793, ES1.0
>>     R-Car Gen2, Alt, r8a7794, ES1.0
>>     R-Car Gen3, Renesas Salvator-X board based on r8a7795, r8a7795, ES1.0
>>     R-Car Gen3, Renesas Salvator-X board based on r8a7796, r8a7796, ES1.0
>>     SH-Mobile, KZM-A9-GT, sh73a0, ES2.0
>
> As mentioned in the comment for the driver patch, I think this makes
> a lot of sense for the machines that have a revision register, in
> particular when the interpretation of that register is always done
> the same way, but I'm a bit skeptical about doing it in the same driver
> for machines that don't have the register.
>
> Matching by a device rather than the SoC platform also has the advantage
> that there is no need to maintain a list of compatible numbers in the
> driver.

Currently we (usually) use:
  - SoC-specific compatible values, to handle known differences within the
    same family now, and handle future unknown differences,
  - Family-specific compatible values, which we define ourselves.

Usually drivers match on the latter.

Every time a new SoC is introduced, we have to update lots of DT binding
docs, to add the new SoC-specific compatible values.

Two-phase matching (driver code matches against "renesas,<foo>",
driver matches against SoC using soc_device_match()) would allow to
remove the burden of updating DT documentation all the time.
The drivers would need updates, though.
Another advantage would be that we can reuse .dtsi snippets for SoCs in
the same family, which we currently can't easily do due to the SoC-specific
compatible values.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 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

[PATCH 0/4] soc: renesas: Identify SoC and register with the SoC bus

[Arnd Bergmann]

On Wednesday, October 19, 2016 10:10:38 AM CEST Geert Uytterhoeven wrote:
> Hi Arnd,
> 
> On Mon, Oct 10, 2016 at 4:28 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday, October 4, 2016 11:09:23 AM CEST Geert Uytterhoeven wrote:
> >> Some Renesas SoCs may exist in different revisions, providing slightly
> >> different functionalities (e.g. R-Car H3 ES1.x and ES2.0). This needs to
> >> be catered for by drivers and/or platform code.  The recently proposed
> >> soc_device_match() API seems like a good fit to handle this.
> >>
> >> This patch series implements the core infrastructure to provide SoC and
> >> revision information through the SoC bus for Renesas ARM SoCs. It
> >> consists of 4 patches:
> >>   - Patch 1 avoids a crash when SoC revision information is needed and
> >>     provided early,
> >>   - Patch 2 (from Arnd) introduces the soc_device_match() API.
> >>     I don't know if, when, and through which channel this patch is
> >>     planned to go upstream,
> >>   - Patch 3 fixes a bug in soc_device_match(), causing a crash when
> >>     trying to match on an SoC attribute that is not provided (seen on
> >>     EMEV2, RZ/A, and R-Car M1A, which lack revision information),
> >>   - Patch 4 identifies Renesas SoCs and registers them with the SoC bus.
> >>
> >> Tested on (family, machine, soc_id, optional revision):
> >>
> >>     Emma Mobile EV2, EMEV2 KZM9D Board, emev2
> >>     RZ/A, Genmai, r7s72100
> >>     R-Mobile, APE6EVM, r8a73a4, ES1.0
> >>     R-Mobile, armadillo 800 eva, r8a7740, ES2.0
> >>     R-Car Gen1, bockw, r8a7778
> >>     R-Car Gen1, marzen, r8a7779, ES1.0
> >>     R-Car Gen2, Lager, r8a7790, ES1.0
> >>     R-Car Gen2, Koelsch, r8a7791, ES1.0
> >>     R-Car Gen2, Gose, r8a7793, ES1.0
> >>     R-Car Gen2, Alt, r8a7794, ES1.0
> >>     R-Car Gen3, Renesas Salvator-X board based on r8a7795, r8a7795, ES1.0
> >>     R-Car Gen3, Renesas Salvator-X board based on r8a7796, r8a7796, ES1.0
> >>     SH-Mobile, KZM-A9-GT, sh73a0, ES2.0
> >
> > As mentioned in the comment for the driver patch, I think this makes
> > a lot of sense for the machines that have a revision register, in
> > particular when the interpretation of that register is always done
> > the same way, but I'm a bit skeptical about doing it in the same driver
> > for machines that don't have the register.
> >
> > Matching by a device rather than the SoC platform also has the advantage
> > that there is no need to maintain a list of compatible numbers in the
> > driver.
> 
> Currently we (usually) use:
>   - SoC-specific compatible values, to handle known differences within the
>     same family now, and handle future unknown differences,
>   - Family-specific compatible values, which we define ourselves.
> 
> Usually drivers match on the latter.
> 
> Every time a new SoC is introduced, we have to update lots of DT binding
> docs, to add the new SoC-specific compatible values.
>
> Two-phase matching (driver code matches against "renesas,<foo>",
> driver matches against SoC using soc_device_match()) would allow to
> remove the burden of updating DT documentation all the time.
> The drivers would need updates, though.
> Another advantage would be that we can reuse .dtsi snippets for SoCs in
> the same family, which we currently can't easily do due to the SoC-specific
> compatible values.

Interesting idea, but unrelated to my comment above, which was about
the soc driver in particular, rather the drivers using it.

	Arnd

[PATCH 2/4] base: soc: Introduce soc_device_match() interface

[Greg Kroah-Hartman]

On Tue, Oct 04, 2016 at 11:09:25AM +0200, Geert Uytterhoeven wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> We keep running into cases where device drivers want to know the exact
> version of the a SoC they are currently running on. In the past, this has
> usually been done through a vendor specific API that can be called by a
> driver, or by directly accessing some kind of version register that is
> not part of the device itself but that belongs to a global register area
> of the chip.
> 
> Common reasons for doing this include:
> 
> - A machine is not using devicetree or similar for passing data about
>   on-chip devices, but just announces their presence using boot-time
>   platform devices, and the machine code itself does not care about the
>   revision.
> 
> - There is existing firmware or boot loaders with existing DT binaries
>   with generic compatible strings that do not identify the particular
>   revision of each device, but the driver knows which SoC revisions
>   include which part.
> 
> - A prerelease version of a chip has some quirks and we are using the same
>   version of the bootloader and the DT blob on both the prerelease and the
>   final version. An update of the DT binding seems inappropriate because
>   that would involve maintaining multiple copies of the dts and/or
>   bootloader.
> 
> This patch introduces the soc_device_match() interface that is meant to
> work like of_match_node() but instead of identifying the version of a
> device, it identifies the SoC itself using a vendor-agnostic interface.
> 
> Unlike of_match_node(), we do not do an exact string compare but instead
> use glob_match() to allow wildcards in strings.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/base/Kconfig    |  1 +
>  drivers/base/soc.c      | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sys_soc.h |  3 +++
>  3 files changed, 70 insertions(+)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 3/4] base: soc: Check for NULL SoC device attributes

[Arnd Bergmann]

On Tuesday, October 4, 2016 11:09:26 AM CEST Geert Uytterhoeven wrote:
> If soc_device_match() is used to check the value of a specific
> attribute that is not present for the current SoC, the kernel crashes
> with a NULL pointer dereference.
> 
> Fix this by explicitly checking for the absence of a needed property,
> and considering this a non-match.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH 3/4] base: soc: Check for NULL SoC device attributes

[Greg Kroah-Hartman]

On Tue, Oct 04, 2016 at 11:09:26AM +0200, Geert Uytterhoeven wrote:
> If soc_device_match() is used to check the value of a specific
> attribute that is not present for the current SoC, the kernel crashes
> with a NULL pointer dereference.
> 
> Fix this by explicitly checking for the absence of a needed property,
> and considering this a non-match.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>