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

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

[Arnd Bergmann]

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 <pali.rohar@gmail.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>

+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

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

[Pali Rohár]

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 <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> 
> +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).

-- 
Pali Rohár
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

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 <pali.rohar@gmail.com>
> > > Acked-by: Pavel Machek <pavel@ucw.cz>
> > 
> > +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

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

[Pali Rohár]

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 <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> > > 
> > > +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

-- 
Pali Rohár
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tony Lindgren]

* Pali Rohár <pali.rohar@gmail.com> [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

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

[Pali Rohár]

On Wednesday 24 June 2015 22:01:38 Tony Lindgren wrote:
> * Pali Rohár <pali.rohar@gmail.com> [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?

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Tony Lindgren]

* Pali Rohár <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pali Rohár]

On Thursday 25 June 2015 00:22:05 Tony Lindgren wrote:
> * Pali Rohár <pali.rohar@gmail.com> [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?

-- 
Pali Rohár
pali.rohar@gmail.com

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

[Tony Lindgren]

* Pali Rohár <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [150625 00:29]:
> On Thursday 25 June 2015 00:22:05 Tony Lindgren wrote:
> > * Pali Rohár <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1965 bytes --]

On Thursday 25 June 2015 07:01:38 Tony Lindgren wrote:
> * Pali Rohár <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [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?

-- 
Pali Rohár
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Tony Lindgren]

* Pali Rohár <pali.rohar@gmail.com> [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

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1100 bytes --]

On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
> * Pali Rohár <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [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"
...

-- 
Pali Rohár
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Tony Lindgren]

* Pali Rohár <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [150706 06:14]:
> On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
> > * Pali Rohár <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [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
> pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Rob Herring]

On Mon, Jul 6, 2015 at 7:31 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Pali Rohár <pali.rohar@gmail.com> [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

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

[Rob Herring]

On Mon, Jul 6, 2015 at 8:12 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
>> * Pali Rohár <pali.rohar@gmail.com> [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
> pali.rohar@gmail.com

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

[Tony Lindgren]

* Rob Herring <robherring2@gmail.com> [150706 08:23]:
> On Mon, Jul 6, 2015 at 7:31 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Pali Rohár <pali.rohar@gmail.com> [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

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1842 bytes --]

On Monday 06 July 2015 17:22:58 Rob Herring wrote:
> On Mon, Jul 6, 2015 at 8:12 AM, Pali Rohár <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
> >> * Pali Rohár <pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [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
> > pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org


-- 
Pali Rohár
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 845 bytes --]

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)

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Rob Herring]

On Mon, Jul 6, 2015 at 11:20 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Monday 06 July 2015 17:22:58 Rob Herring wrote:
>> On Mon, Jul 6, 2015 at 8:12 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
>> > On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
>> >> * Pali Rohár <pali.rohar@gmail.com> [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