Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1503598 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the  SoC bus Geert Uytterhoeven <geert@linux-m68k.org> - 2016-10-19 11:10 +0200
    Re: [PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the SoC bus Arnd Bergmann <arnd@arndb.de> - 2016-10-19 17:30 +0200
      Re: [PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the  SoC bus Geert Uytterhoeven <geert@linux-m68k.org> - 2016-10-21 20:20 +0200
        Re: [PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the SoC bus Arnd Bergmann <arnd@arndb.de> - 2016-10-21 23:20 +0200
          Re: [PATCH/RFC 4/4] soc: renesas: Identify SoC and register with the  SoC bus Geert Uytterhoeven <geert@linux-m68k.org> - 2016-10-22 09:50 +0200

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

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@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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1506202

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@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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1506396

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@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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web