[RESEND,v2,1/2] arm: devtree: Set system_rev from DT revision

Commit Message

Pali Rohár May 6, 2015, 8:49 a.m. UTC

With this patch "revision" DT string entry is used to set global system_rev
variable. DT "revision" is expected to be string with one hexadecimal number.
So "Revision" line in /proc/cpuinfo will be same as "revision" DT value.

Signed-off-by: Pali Rohár <[e-mail]>
Acked-by: Pavel Machek <[e-mail]>
---
 arch/arm/kernel/devtree.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Arnd Bergmann May 6, 2015, 9:31 a.m. UTC | #1

On Wednesday 06 May 2015 10:49:01 Pali Rohár wrote:
> With this patch "revision" DT string entry is used to set global system_rev
> variable. DT "revision" is expected to be string with one hexadecimal number.
> So "Revision" line in /proc/cpuinfo will be same as "revision" DT value.
> 
> Signed-off-by: Pali Rohár <[e-mail]>
> Acked-by: Pavel Machek <[e-mail]>

+devicetree mailing list

The property needs to be specified in a binding somewhere.

> @@ -246,5 +247,14 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
>  	/* Change machine number to match the mdesc we're using */
>  	__machine_arch_type = mdesc->nr;
>  
> +	/* Set system revision from DT */
> +	prop = of_get_flat_dt_prop(dt_root, "revision", &size);
> +	if (prop && size > 0) {
> +		char revision[11];
> +		strlcpy(revision, prop, min(size, (int)sizeof(revision)));
> +		if (kstrtouint(revision, 16, &system_rev) != 0)
> +			system_rev = 0;
> +	}
> +
>  	return mdesc;
>  }
> 

What is the reason for doing it this early? I think it would be nicer to do
it after unflattening the DT.

Also, it seems strange to have a string property and then use kstrtouint
to convert it into a number. I think it should either be specified in a DT
binding to be a string and then have the kernel not assume that it is a number,
or we should define it to be binary.

	Arnd

Pali Rohár May 6, 2015, 10:37 a.m. UTC | #2

On Wednesday 06 May 2015 11:31:15 Arnd Bergmann wrote:
> On Wednesday 06 May 2015 10:49:01 Pali Rohár wrote:
> > With this patch "revision" DT string entry is used to set global system_rev
> > variable. DT "revision" is expected to be string with one hexadecimal number.
> > So "Revision" line in /proc/cpuinfo will be same as "revision" DT value.
> > 
> > Signed-off-by: Pali Rohár <[e-mail]>
> > Acked-by: Pavel Machek <[e-mail]>
> 
> +devicetree mailing list
> 
> The property needs to be specified in a binding somewhere.
> 
> > @@ -246,5 +247,14 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
> >  	/* Change machine number to match the mdesc we're using */
> >  	__machine_arch_type = mdesc->nr;
> >  
> > +	/* Set system revision from DT */
> > +	prop = of_get_flat_dt_prop(dt_root, "revision", &size);
> > +	if (prop && size > 0) {
> > +		char revision[11];
> > +		strlcpy(revision, prop, min(size, (int)sizeof(revision)));
> > +		if (kstrtouint(revision, 16, &system_rev) != 0)
> > +			system_rev = 0;
> > +	}
> > +
> >  	return mdesc;
> >  }
> > 
> 
> What is the reason for doing it this early? I think it would be nicer to do
> it after unflattening the DT.
> 

It needs to be done in this code, so "system_rev" variable is set
properly...

> Also, it seems strange to have a string property and then use kstrtouint
> to convert it into a number. I think it should either be specified in a DT
> binding to be a string and then have the kernel not assume that it is a number,
> or we should define it to be binary.
> 
> 	Arnd

Variable "system_rev" is number and it always was. So chaning type will
break more parts.

And it is string DT property to be human readable. Some other developers
suggested for v2 to change it to string (from number).

Arnd Bergmann May 6, 2015, 11:04 a.m. UTC | #3

On Wednesday 06 May 2015 12:37:52 Pali Rohár wrote:
> On Wednesday 06 May 2015 11:31:15 Arnd Bergmann wrote:
> > On Wednesday 06 May 2015 10:49:01 Pali Rohár wrote:
> > > With this patch "revision" DT string entry is used to set global system_rev
> > > variable. DT "revision" is expected to be string with one hexadecimal number.
> > > So "Revision" line in /proc/cpuinfo will be same as "revision" DT value.
> > > 
> > > Signed-off-by: Pali Rohár <[e-mail]>
> > > Acked-by: Pavel Machek <[e-mail]>
> > 
> > +devicetree mailing list
> > 
> > The property needs to be specified in a binding somewhere.
> > 
> > > @@ -246,5 +247,14 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
> > >     /* Change machine number to match the mdesc we're using */
> > >     __machine_arch_type = mdesc->nr;
> > >  
> > > +   /* Set system revision from DT */
> > > +   prop = of_get_flat_dt_prop(dt_root, "revision", &size);
> > > +   if (prop && size > 0) {
> > > +           char revision[11];
> > > +           strlcpy(revision, prop, min(size, (int)sizeof(revision)));
> > > +           if (kstrtouint(revision, 16, &system_rev) != 0)
> > > +                   system_rev = 0;
> > > +   }
> > > +
> > >     return mdesc;
> > >  }
> > > 
> > 
> > What is the reason for doing it this early? I think it would be nicer to do
> > it after unflattening the DT.
> > 
> 
> It needs to be done in this code, so "system_rev" variable is set
> properly...

What I mean is which code accesses this variable that early?

> > Also, it seems strange to have a string property and then use kstrtouint
> > to convert it into a number. I think it should either be specified in a DT
> > binding to be a string and then have the kernel not assume that it is a number,
> > or we should define it to be binary.
> > 
> >       Arnd
> 
> Variable "system_rev" is number and it always was. So chaning type will
> break more parts.
> 
> And it is string DT property to be human readable. Some other developers
> suggested for v2 to change it to string (from number).

Both of them would be human readable, you just use something else to
read them ;-)

If we have a string here, we should just change all uses of system_rev
in the kernel accordingly, there are only a few of them:

$ git grep -w system_rev
arch/arm/include/asm/system_info.h:extern unsigned int system_rev;
arch/arm/kernel/atags_parse.c:  system_rev = tag->u.revision.rev;
arch/arm/kernel/setup.c:unsigned int system_rev;
arch/arm/kernel/setup.c:EXPORT_SYMBOL(system_rev);
arch/arm/kernel/setup.c:        seq_printf(m, "Revision\t: %04x\n", system_rev);
arch/arm/mach-clps711x/devices.c:       system_rev = SYSFLG1_VERID(readl(base + SYSFLG1));
arch/arm/mach-clps711x/devices.c:       soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%u", system_rev);
arch/arm/mach-davinci/board-da850-evm.c:        switch (system_rev & 0xF) {
arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    system_rev = 0x27000;
arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    system_rev |= (mo_version << MOTHERBOARD_SHIFT);
arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    system_rev |= (exp_version << EXPBOARD_SHIFT);
arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    mo_version = (system_rev >> MOTHERBOARD_SHIFT) & VERSION_MASK;
arch/arm/mach-ixp4xx/goramo_mlr.c:              system_rev = __raw_readl(flash + CFG_REV);
arch/arm/mach-omap2/board-rx51-peripherals.c:   if ((system_rev >= SYSTEM_REV_S_USES_VAUX3 && system_rev < 0x100) ||
arch/arm/mach-omap2/board-rx51-peripherals.c:       system_rev >= SYSTEM_REV_B_USES_VAUX3) {
arch/arm/mach-orion5x/dns323-setup.c:   if (machine_is_dns323() && system_rev == DNS323_REV_A1)
arch/arm/mach-orion5x/dns323-setup.c:   system_rev = dns323_identify_rev();
arch/arm/mach-orion5x/dns323-setup.c:   pr_info("DNS-323: Identified HW revision %c1\n", 'A' + system_rev);
arch/arm/mach-orion5x/dns323-setup.c:   switch(system_rev) {
arch/arm/mach-orion5x/dns323-setup.c:   switch(system_rev) {
arch/arm/mach-orion5x/dns323-setup.c:   switch(system_rev) {
arch/arm/mach-pxa/cm-x300.c:    if (system_rev < 130)
arch/arm/mach-pxa/cm-x300.c:    if (system_rev < 130) {
arch/arm/mach-pxa/cm-x300.c:    if (system_rev < 130)
arch/arm/mach-pxa/magician.c:           if (system_rev < 3)
arch/arm/mach-pxa/magician.c:           if (system_rev < 3)
arch/arm/mach-pxa/magician.c:           system_rev = board_id & 0x7;
arch/arm/mach-pxa/magician.c:           if (lcd_select && (system_rev < 3))
arch/arm/mach-pxa/raumfeld.c:   if ((system_rev & 0xff) == 2) {
arch/arm/mach-pxa/raumfeld.c:   if ((system_rev & 0xff) > 1) {
arch/arm/mach-pxa/viper.c:              system_rev = (VIPER_BOARD_VERSION(version) << 8) |
arch/arm/mach-pxa/zeus.c:       system_rev = __raw_readw(ZEUS_CPLD_VERSION);
arch/arm/mach-pxa/zeus.c:       pr_info("Zeus CPLD V%dI%d\n", (system_rev & 0xf0) >> 4, (system_rev & 0x0f));
arch/arm/mach-zynq/common.c:    system_rev = zynq_get_revision();
arch/arm/mach-zynq/common.c:    soc_dev_attr->revision = kasprintf(GFP_KERNEL, "0x%x", system_rev);

In fact, half the uses of this actually assign the revision number themselves.
code outside of arch/arm/mach-* and /proc/cpuinfo currently uses the variable.

	Arnd

Pali Rohár May 6, 2015, 11:44 a.m. UTC | #4

On Wednesday 06 May 2015 13:04:01 Arnd Bergmann wrote:
> On Wednesday 06 May 2015 12:37:52 Pali Rohár wrote:
> > On Wednesday 06 May 2015 11:31:15 Arnd Bergmann wrote:
> > > On Wednesday 06 May 2015 10:49:01 Pali Rohár wrote:
> > > > With this patch "revision" DT string entry is used to set global system_rev
> > > > variable. DT "revision" is expected to be string with one hexadecimal number.
> > > > So "Revision" line in /proc/cpuinfo will be same as "revision" DT value.
> > > > 
> > > > Signed-off-by: Pali Rohár <[e-mail]>
> > > > Acked-by: Pavel Machek <[e-mail]>
> > > 
> > > +devicetree mailing list
> > > 
> > > The property needs to be specified in a binding somewhere.
> > > 
> > > > @@ -246,5 +247,14 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
> > > >     /* Change machine number to match the mdesc we're using */
> > > >     __machine_arch_type = mdesc->nr;
> > > >  
> > > > +   /* Set system revision from DT */
> > > > +   prop = of_get_flat_dt_prop(dt_root, "revision", &size);
> > > > +   if (prop && size > 0) {
> > > > +           char revision[11];
> > > > +           strlcpy(revision, prop, min(size, (int)sizeof(revision)));
> > > > +           if (kstrtouint(revision, 16, &system_rev) != 0)
> > > > +                   system_rev = 0;
> > > > +   }
> > > > +
> > > >     return mdesc;
> > > >  }
> > > > 
> > > 
> > > What is the reason for doing it this early? I think it would be nicer to do
> > > it after unflattening the DT.
> > > 
> > 
> > It needs to be done in this code, so "system_rev" variable is set
> > properly...
> 
> What I mean is which code accesses this variable that early?
> 

ATAG code is doing it at same early stage, so I added it to same early
stage...

> > > Also, it seems strange to have a string property and then use kstrtouint
> > > to convert it into a number. I think it should either be specified in a DT
> > > binding to be a string and then have the kernel not assume that it is a number,
> > > or we should define it to be binary.
> > > 
> > >       Arnd
> > 
> > Variable "system_rev" is number and it always was. So chaning type will
> > break more parts.
> > 
> > And it is string DT property to be human readable. Some other developers
> > suggested for v2 to change it to string (from number).
> 
> Both of them would be human readable, you just use something else to
> read them ;-)
> 
> If we have a string here, we should just change all uses of system_rev
> in the kernel accordingly, there are only a few of them:
> 
> $ git grep -w system_rev
> arch/arm/include/asm/system_info.h:extern unsigned int system_rev;
> arch/arm/kernel/atags_parse.c:  system_rev = tag->u.revision.rev;
> arch/arm/kernel/setup.c:unsigned int system_rev;
> arch/arm/kernel/setup.c:EXPORT_SYMBOL(system_rev);
> arch/arm/kernel/setup.c:        seq_printf(m, "Revision\t: %04x\n", system_rev);
> arch/arm/mach-clps711x/devices.c:       system_rev = SYSFLG1_VERID(readl(base + SYSFLG1));
> arch/arm/mach-clps711x/devices.c:       soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%u", system_rev);
> arch/arm/mach-davinci/board-da850-evm.c:        switch (system_rev & 0xF) {
> arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    system_rev = 0x27000;
> arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    system_rev |= (mo_version << MOTHERBOARD_SHIFT);
> arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    system_rev |= (exp_version << EXPBOARD_SHIFT);
> arch/arm/mach-imx/mach-imx27_visstrim_m10.c:    mo_version = (system_rev >> MOTHERBOARD_SHIFT) & VERSION_MASK;
> arch/arm/mach-ixp4xx/goramo_mlr.c:              system_rev = __raw_readl(flash + CFG_REV);
> arch/arm/mach-omap2/board-rx51-peripherals.c:   if ((system_rev >= SYSTEM_REV_S_USES_VAUX3 && system_rev < 0x100) ||
> arch/arm/mach-omap2/board-rx51-peripherals.c:       system_rev >= SYSTEM_REV_B_USES_VAUX3) {
> arch/arm/mach-orion5x/dns323-setup.c:   if (machine_is_dns323() && system_rev == DNS323_REV_A1)
> arch/arm/mach-orion5x/dns323-setup.c:   system_rev = dns323_identify_rev();
> arch/arm/mach-orion5x/dns323-setup.c:   pr_info("DNS-323: Identified HW revision %c1\n", 'A' + system_rev);
> arch/arm/mach-orion5x/dns323-setup.c:   switch(system_rev) {
> arch/arm/mach-orion5x/dns323-setup.c:   switch(system_rev) {
> arch/arm/mach-orion5x/dns323-setup.c:   switch(system_rev) {
> arch/arm/mach-pxa/cm-x300.c:    if (system_rev < 130)
> arch/arm/mach-pxa/cm-x300.c:    if (system_rev < 130) {
> arch/arm/mach-pxa/cm-x300.c:    if (system_rev < 130)
> arch/arm/mach-pxa/magician.c:           if (system_rev < 3)
> arch/arm/mach-pxa/magician.c:           if (system_rev < 3)
> arch/arm/mach-pxa/magician.c:           system_rev = board_id & 0x7;
> arch/arm/mach-pxa/magician.c:           if (lcd_select && (system_rev < 3))
> arch/arm/mach-pxa/raumfeld.c:   if ((system_rev & 0xff) == 2) {
> arch/arm/mach-pxa/raumfeld.c:   if ((system_rev & 0xff) > 1) {
> arch/arm/mach-pxa/viper.c:              system_rev = (VIPER_BOARD_VERSION(version) << 8) |
> arch/arm/mach-pxa/zeus.c:       system_rev = __raw_readw(ZEUS_CPLD_VERSION);
> arch/arm/mach-pxa/zeus.c:       pr_info("Zeus CPLD V%dI%d\n", (system_rev & 0xf0) >> 4, (system_rev & 0x0f));
> arch/arm/mach-zynq/common.c:    system_rev = zynq_get_revision();
> arch/arm/mach-zynq/common.c:    soc_dev_attr->revision = kasprintf(GFP_KERNEL, "0x%x", system_rev);
> 
> In fact, half the uses of this actually assign the revision number themselves.
> code outside of arch/arm/mach-* and /proc/cpuinfo currently uses the variable.
> 
> 	Arnd

Tony Lindgren June 25, 2015, 5:01 a.m. UTC | #5

* Pali Rohár <[e-mail]> [150506 04:45]:
> On Wednesday 06 May 2015 13:04:01 Arnd Bergmann wrote:
> > > 
> > > It needs to be done in this code, so "system_rev" variable is set
> > > properly...
> > 
> > What I mean is which code accesses this variable that early?
> > 
> 
> ATAG code is doing it at same early stage, so I added it to same early
> stage...

Yes we should do this early like the other atags.
 
> > > > Also, it seems strange to have a string property and then use kstrtouint
> > > > to convert it into a number. I think it should either be specified in a DT
> > > > binding to be a string and then have the kernel not assume that it is a number,
> > > > or we should define it to be binary.
> > > > 
> > > >       Arnd
> > > 
> > > Variable "system_rev" is number and it always was. So chaning type will
> > > break more parts.
> > > 
> > > And it is string DT property to be human readable. Some other developers
> > > suggested for v2 to change it to string (from number).
> > 
> > Both of them would be human readable, you just use something else to
> > read them ;-)
> > 
> > If we have a string here, we should just change all uses of system_rev
> > in the kernel accordingly, there are only a few of them:

Let's just keep it as a hex as it was. After all it's an existing
interface in /proc that user space programs may expect to be in
hex format already.

Pali, care to repost the whole set again right after -rc1 with
with rev property naming and documentation added? Just keep it
as hex and let's forget any string conversion.

Regards,

Tony

Pali Rohár June 25, 2015, 7:18 a.m. UTC | #6

On Wednesday 24 June 2015 22:01:38 Tony Lindgren wrote:
> * Pali Rohár <[e-mail]> [150506 04:45]:
> > On Wednesday 06 May 2015 13:04:01 Arnd Bergmann wrote:
> > > > 
> > > > It needs to be done in this code, so "system_rev" variable is set
> > > > properly...
> > > 
> > > What I mean is which code accesses this variable that early?
> > > 
> > 
> > ATAG code is doing it at same early stage, so I added it to same early
> > stage...
> 
> Yes we should do this early like the other atags.
>  
> > > > > Also, it seems strange to have a string property and then use kstrtouint
> > > > > to convert it into a number. I think it should either be specified in a DT
> > > > > binding to be a string and then have the kernel not assume that it is a number,
> > > > > or we should define it to be binary.
> > > > > 
> > > > >       Arnd
> > > > 
> > > > Variable "system_rev" is number and it always was. So chaning type will
> > > > break more parts.
> > > > 
> > > > And it is string DT property to be human readable. Some other developers
> > > > suggested for v2 to change it to string (from number).
> > > 
> > > Both of them would be human readable, you just use something else to
> > > read them ;-)
> > > 
> > > If we have a string here, we should just change all uses of system_rev
> > > in the kernel accordingly, there are only a few of them:
> 
> Let's just keep it as a hex as it was. After all it's an existing
> interface in /proc that user space programs may expect to be in
> hex format already.
> 
> Pali, care to repost the whole set again right after -rc1 with
> with rev property naming and documentation added? Just keep it
> as hex and let's forget any string conversion.
> 
> Regards,
> 
> Tony

Ok, but what do you mean to forget any string conversion?

Tony Lindgren June 25, 2015, 7:22 a.m. UTC | #7

* Pali Rohár <[e-mail]> [150625 00:21]:
> 
> Ok, but what do you mean to forget any string conversion?

No need for tohexstr() in the uncommpress code if the system_rev
value is a number coming from the dts.

Regards,

Tony

Pali Rohár June 25, 2015, 7:27 a.m. UTC | #8

On Thursday 25 June 2015 00:22:05 Tony Lindgren wrote:
> * Pali Rohár <[e-mail]> [150625 00:21]:
> > 
> > Ok, but what do you mean to forget any string conversion?
> 
> No need for tohexstr() in the uncommpress code if the system_rev
> value is a number coming from the dts.
> 
> Regards,
> 
> Tony

So /revision DT property will be (binary) value, right?

Tony Lindgren June 25, 2015, 7:41 a.m. UTC | #9

* Pali Rohár <[e-mail]> [150625 00:29]:
> On Thursday 25 June 2015 00:22:05 Tony Lindgren wrote:
> > * Pali Rohár <[e-mail]> [150625 00:21]:
> > > 
> > > Ok, but what do you mean to forget any string conversion?
> > 
> > No need for tohexstr() in the uncommpress code if the system_rev
> > value is a number coming from the dts.
> 
> So /revision DT property will be (binary) value, right?

Right just u32 value.

Regards,

Tony

Russell King - ARM Linux June 25, 2015, 10:03 a.m. UTC | #10

On Wed, May 06, 2015 at 01:44:17PM +0200, Pali Rohár wrote:
> On Wednesday 06 May 2015 13:04:01 Arnd Bergmann wrote:
> > What I mean is which code accesses this variable that early?
> > 
> 
> ATAG code is doing it at same early stage, so I added it to same early
> stage...

ATAG code does it early because ATAGs are only available early on, and
it's simpler to parse them all in one go, rather than having to do
multiple passes over the structure - especially when most instances are
just storing an integer to some BSS variable.

Pali Rohár July 6, 2015, 12:23 p.m. UTC | #11

On Thursday 25 June 2015 07:01:38 Tony Lindgren wrote:
> * Pali Rohár <[e-mail]> [150506 04:45]:
> > On Wednesday 06 May 2015 13:04:01 Arnd Bergmann wrote:
> > > > It needs to be done in this code, so "system_rev" variable is
> > > > set properly...
> > > 
> > > What I mean is which code accesses this variable that early?
> > 
> > ATAG code is doing it at same early stage, so I added it to same
> > early stage...
> 
> Yes we should do this early like the other atags.
> 
> > > > > Also, it seems strange to have a string property and then use
> > > > > kstrtouint to convert it into a number. I think it should
> > > > > either be specified in a DT binding to be a string and then
> > > > > have the kernel not assume that it is a number, or we should
> > > > > define it to be binary.
> > > > > 
> > > > >       Arnd
> > > > 
> > > > Variable "system_rev" is number and it always was. So chaning
> > > > type will break more parts.
> > > > 
> > > > And it is string DT property to be human readable. Some other
> > > > developers suggested for v2 to change it to string (from
> > > > number).
> > > 
> > > Both of them would be human readable, you just use something else
> > > to read them ;-)
> > > 
> > > If we have a string here, we should just change all uses of
> > > system_rev
> 
> > > in the kernel accordingly, there are only a few of them:
> Let's just keep it as a hex as it was. After all it's an existing
> interface in /proc that user space programs may expect to be in
> hex format already.
> 
> Pali, care to repost the whole set again right after -rc1 with
> with rev property naming and documentation added? Just keep it
> as hex and let's forget any string conversion.
> 
> Regards,
> 
> Tony

Hello Tony,

into which file should I put documentation about new DT properties?

Tony Lindgren July 6, 2015, 12:31 p.m. UTC | #12

* Pali Rohár <[e-mail]> [150706 05:25]:
>
> into which file should I put documentation about new DT properties?

If it's Linux generic like linux,revision, then how about
Documentation/devicetree/bindings/revision.txt?

For the ATAGs, Documentation/devicetree/bindings/arm/atag.txt?

Regards,

Tony

Pali Rohár July 6, 2015, 1:12 p.m. UTC | #13

On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
> * Pali Rohár <[e-mail]> [150706 05:25]:
> > into which file should I put documentation about new DT properties?
> 
> If it's Linux generic like linux,revision, then how about
> Documentation/devicetree/bindings/revision.txt?
> 
> For the ATAGs, Documentation/devicetree/bindings/arm/atag.txt?
> 
> Regards,
> 
> Tony

Hm... now I'm thinking into which DT field should I put atags and 
revision. In previous emails you wrote to use "linux,atags", now 
"arm,atags"? And put them into root node? Or other?

In arch/arm/boot/compressed/atags_to_fdt.c code I see that most atag 
properties are converted into "/chosen" node in DT...

So what do you prefer for "revision" and what for "atags"?

Some mentioned examples:

"/revision"
"/chosen/revision"
"/linux,revision"
"/chosen/linux,revision"
...

"/atags"
"/chosen/atags"
"/linux,atags"
"/chosen/linux,atags"
"/arm,atags"
"/chosen/arm,atags"
...

Tony Lindgren July 6, 2015, 1:55 p.m. UTC | #14

* Pali Rohár <[e-mail]> [150706 06:14]:
> On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
> > * Pali Rohár <[e-mail]> [150706 05:25]:
> > > into which file should I put documentation about new DT properties?
> > 
> > If it's Linux generic like linux,revision, then how about
> > Documentation/devicetree/bindings/revision.txt?
> > 
> > For the ATAGs, Documentation/devicetree/bindings/arm/atag.txt?
>
> Hm... now I'm thinking into which DT field should I put atags and 
> revision. In previous emails you wrote to use "linux,atags", now 
> "arm,atags"? And put them into root node? Or other?
> 
> In arch/arm/boot/compressed/atags_to_fdt.c code I see that most atag 
> properties are converted into "/chosen" node in DT...
> 
> So what do you prefer for "revision" and what for "atags"?

I'd prefer linux,revision and arm,atags. Chances are the ATAGs
won't be used on other architectures. I'm find with linux,atags
too if people prefer that.

Regards,

Tony
 
> Some mentioned examples:
> 
> "/revision"
> "/chosen/revision"
> "/linux,revision"
> "/chosen/linux,revision"
> ...
> 
> "/atags"
> "/chosen/atags"
> "/linux,atags"
> "/chosen/linux,atags"
> "/arm,atags"
> "/chosen/arm,atags"
> ...
> 
> -- 
> Pali Rohár
> [e-mail]

Rob Herring July 6, 2015, 3:20 p.m. UTC | #15

On Mon, Jul 6, 2015 at 7:31 AM, Tony Lindgren <[e-mail]> wrote:
> * Pali Rohár <[e-mail]> [150706 05:25]:
>>
>> into which file should I put documentation about new DT properties?
>
> If it's Linux generic like linux,revision, then how about

Just "revision" at the top level please. I'd prefer a string still,
but either is fine.

Rob

Rob Herring July 6, 2015, 3:22 p.m. UTC | #16

On Mon, Jul 6, 2015 at 8:12 AM, Pali Rohár <[e-mail]> wrote:
> On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
>> * Pali Rohár <[e-mail]> [150706 05:25]:
>> > into which file should I put documentation about new DT properties?
>>
>> If it's Linux generic like linux,revision, then how about
>> Documentation/devicetree/bindings/revision.txt?
>>
>> For the ATAGs, Documentation/devicetree/bindings/arm/atag.txt?
>>
>> Regards,
>>
>> Tony
>
> Hm... now I'm thinking into which DT field should I put atags and
> revision. In previous emails you wrote to use "linux,atags", now
> "arm,atags"? And put them into root node? Or other?
>
> In arch/arm/boot/compressed/atags_to_fdt.c code I see that most atag
> properties are converted into "/chosen" node in DT...
>
> So what do you prefer for "revision" and what for "atags"?
>
> Some mentioned examples:
>
> "/revision"

This one. This is a top level h/w property.

> "/chosen/revision"
> "/linux,revision"
> "/chosen/linux,revision"
> ...
>
> "/atags"
> "/chosen/atags"
> "/linux,atags"
> "/chosen/linux,atags"

This one. ATAGs are a Linux data struct.

Rob

> "/arm,atags"
> "/chosen/arm,atags"
> ...
>
> --
> Pali Rohár
> [e-mail]

Tony Lindgren July 6, 2015, 3:24 p.m. UTC | #17

* Rob Herring <[e-mail]> [150706 08:23]:
> On Mon, Jul 6, 2015 at 7:31 AM, Tony Lindgren <[e-mail]> wrote:
> > * Pali Rohár <[e-mail]> [150706 05:25]:
> >>
> >> into which file should I put documentation about new DT properties?
> >
> > If it's Linux generic like linux,revision, then how about
> 
> Just "revision" at the top level please. I'd prefer a string still,
> but either is fine.

OK works for me thanks.

Tony

Pali Rohár July 6, 2015, 4:20 p.m. UTC | #18

On Monday 06 July 2015 17:22:58 Rob Herring wrote:
> On Mon, Jul 6, 2015 at 8:12 AM, Pali Rohár <[e-mail]> wrote:
> > On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
> >> * Pali Rohár <[e-mail]> [150706 05:25]:
> >> > into which file should I put documentation about new DT
> >> > properties?
> >> 
> >> If it's Linux generic like linux,revision, then how about
> >> Documentation/devicetree/bindings/revision.txt?
> >> 
> >> For the ATAGs, Documentation/devicetree/bindings/arm/atag.txt?
> >> 
> >> Regards,
> >> 
> >> Tony
> > 
> > Hm... now I'm thinking into which DT field should I put atags and
> > revision. In previous emails you wrote to use "linux,atags", now
> > "arm,atags"? And put them into root node? Or other?
> > 
> > In arch/arm/boot/compressed/atags_to_fdt.c code I see that most
> > atag properties are converted into "/chosen" node in DT...
> > 
> > So what do you prefer for "revision" and what for "atags"?
> > 
> > Some mentioned examples:
> > 
> > "/revision"
> 
> This one. This is a top level h/w property.
> 
> > "/chosen/revision"
> > "/linux,revision"
> > "/chosen/linux,revision"
> > ...
> > 
> > "/atags"
> > "/chosen/atags"
> > "/linux,atags"
> > "/chosen/linux,atags"
> 
> This one. ATAGs are a Linux data struct.
> 
> Rob
> 

Ok, and how read that property "/chosen/linux,atags" in function
setup_machine_fdt() from file arch/arm/kernel/devtree.c ?

of_get_flat_dt_prop() cannot be used unless somebody get me offset to
node "/chosen"...

Any idea?

> > "/arm,atags"
> > "/chosen/arm,atags"
> > ...
> > 
> > --
> > Pali Rohár
> > [e-mail]

Pali Rohár July 6, 2015, 4:36 p.m. UTC | #19

On Monday 06 July 2015 18:20:35 Pali Rohár wrote:
> > > "/chosen/linux,atags"
> > 
> > This one. ATAGs are a Linux data struct.
> > 
> > Rob
> 
> Ok, and how read that property "/chosen/linux,atags" in function
> setup_machine_fdt() from file arch/arm/kernel/devtree.c ?
> 
> of_get_flat_dt_prop() cannot be used unless somebody get me offset to
> node "/chosen"...
> 
> Any idea?
> 

fdt_path_offset() from libfdt.h seems to work...

Is solution like this one acceptable?

#include <linux/libfdt.h>
#include "atags.h"

... setup_machine_fdt(unsigned int dt_phys) {

dt_virt = phys_to_virt(dt_phys);
dt_chosen = fdt_path_offset(dt_virt, "/chosen");
atags = of_get_flat_dt_prop(dt_chosen, "linux,atags", NULL);
save_atags(atags);

}

(this is without checks for errors)

Rob Herring July 6, 2015, 5:30 p.m. UTC | #20

On Mon, Jul 6, 2015 at 11:20 AM, Pali Rohár <[e-mail]> wrote:
> On Monday 06 July 2015 17:22:58 Rob Herring wrote:
>> On Mon, Jul 6, 2015 at 8:12 AM, Pali Rohár <[e-mail]> wrote:
>> > On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
>> >> * Pali Rohár <[e-mail]> [150706 05:25]:
>> >> > into which file should I put documentation about new DT
>> >> > properties?
>> >>
>> >> If it's Linux generic like linux,revision, then how about
>> >> Documentation/devicetree/bindings/revision.txt?
>> >>
>> >> For the ATAGs, Documentation/devicetree/bindings/arm/atag.txt?
>> >>
>> >> Regards,
>> >>
>> >> Tony
>> >
>> > Hm... now I'm thinking into which DT field should I put atags and
>> > revision. In previous emails you wrote to use "linux,atags", now
>> > "arm,atags"? And put them into root node? Or other?
>> >
>> > In arch/arm/boot/compressed/atags_to_fdt.c code I see that most
>> > atag properties are converted into "/chosen" node in DT...
>> >
>> > So what do you prefer for "revision" and what for "atags"?
>> >
>> > Some mentioned examples:
>> >
>> > "/revision"
>>
>> This one. This is a top level h/w property.
>>
>> > "/chosen/revision"
>> > "/linux,revision"
>> > "/chosen/linux,revision"
>> > ...
>> >
>> > "/atags"
>> > "/chosen/atags"
>> > "/linux,atags"
>> > "/chosen/linux,atags"
>>
>> This one. ATAGs are a Linux data struct.
>>
>> Rob
>>
>
> Ok, and how read that property "/chosen/linux,atags" in function
> setup_machine_fdt() from file arch/arm/kernel/devtree.c ?
>
> of_get_flat_dt_prop() cannot be used unless somebody get me offset to
> node "/chosen"...

Why can't you get the offset yourself?

However, why does this need to be early? It is only used to populate
/proc, right?

Rob

Patch

diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index 11c54de..7e13e27 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -19,6 +19,7 @@ 
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/smp.h>
+#include <linux/libfdt_env.h>
 
 #include <asm/cputype.h>
 #include <asm/setup.h>
@@ -26,6 +27,7 @@ 
 #include <asm/smp_plat.h>
 #include <asm/mach/arch.h>
 #include <asm/mach-types.h>
+#include <asm/system_info.h>
 
 
 #ifdef CONFIG_SMP
@@ -204,6 +206,9 @@  static const void * __init arch_get_next_mach(const char *const **match)
 const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
 {
 	const struct machine_desc *mdesc, *mdesc_best = NULL;
+	unsigned long dt_root;
+	const char *prop;
+	int size;
 
 #ifdef CONFIG_ARCH_MULTIPLATFORM
 	DT_MACHINE_START(GENERIC_DT, "Generic DT based system")
@@ -215,17 +220,13 @@  const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
 	if (!dt_phys || !early_init_dt_verify(phys_to_virt(dt_phys)))
 		return NULL;
 
+	dt_root = of_get_flat_dt_root();
 	mdesc = of_flat_dt_match_machine(mdesc_best, arch_get_next_mach);
 
 	if (!mdesc) {
-		const char *prop;
-		int size;
-		unsigned long dt_root;
-
 		early_print("\nError: unrecognized/unsupported "
 			    "device tree compatible list:\n[ ");
 
-		dt_root = of_get_flat_dt_root();
 		prop = of_get_flat_dt_prop(dt_root, "compatible", &size);
 		while (size > 0) {
 			early_print("'%s' ", prop);
@@ -246,5 +247,14 @@  const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
 	/* Change machine number to match the mdesc we're using */
 	__machine_arch_type = mdesc->nr;
 
+	/* Set system revision from DT */
+	prop = of_get_flat_dt_prop(dt_root, "revision", &size);
+	if (prop && size > 0) {
+		char revision[11];
+		strlcpy(revision, prop, min(size, (int)sizeof(revision)));
+		if (kstrtouint(revision, 16, &system_rev) != 0)
+			system_rev = 0;
+	}
+
 	return mdesc;
 }