[RESEND] [PATCH v2 0/2] ARM: /proc/cpuinfo: DT: Add support for Revision

[RESEND] [PATCH v2 0/2] ARM: /proc/cpuinfo: DT: Add support for Revision

[Pali Rohár]

This patch adds support for DT "/revision" and convert ATAG_REVISION to DT.

Pali Roh?r (2):
  arm: devtree: Set system_rev from DT revision
  arm: boot: convert ATAG_REVISION to DT revision field

 arch/arm/boot/compressed/atags_to_fdt.c |   37 +++++++++++++++++++++++++++++++
 arch/arm/kernel/devtree.c               |   20 ++++++++++++-----
 2 files changed, 52 insertions(+), 5 deletions(-)

-- 
1.7.9.5

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

[Pali Rohár]

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>
---
 arch/arm/kernel/devtree.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

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;
 }
-- 
1.7.9.5

[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

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

> 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 at gmail.com

[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

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

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 at gmail.com

[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

[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 at gmail.com

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

[Tony Lindgren]

* 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

[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 at gmail.com

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

[Tony Lindgren]

* Pali Roh?r <pali.rohar@gmail.com> [150625 00:29]:
> 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.
> 
> So /revision DT property will be (binary) value, right?

Right just u32 value.

Regards,

Tony

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

[Pali Rohár]

On Thursday 25 June 2015 07: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

Hello Tony,

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

-- 
Pali Roh?r
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150706/930acdaf/attachment.sig>

[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

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

[Pali Rohár]

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"
"/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 at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150706/a874ec87/attachment.sig>

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

[Tony Lindgren]

* Pali Roh?r <pali.rohar@gmail.com> [150706 06:14]:
> 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?
>
> 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 at gmail.com

[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 at gmail.com

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

[Pali Rohár]

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"...

Any idea?

> > "/arm,atags"
> > "/chosen/arm,atags"
> > ...
> > 
> > --
> > Pali Roh?r
> > pali.rohar at gmail.com


-- 
Pali Roh?r
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150706/4b4fe11c/attachment-0001.sig>

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

[Pali Rohár]

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 at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150706/320d18e1/attachment.sig>

[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

[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

[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

[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.

[RESEND] [PATCH v2 2/2] arm: boot: convert ATAG_REVISION to DT revision field

[Pali Rohár]

ATAG_REVISION is unsigned number and revision in DT is stored as hexadecimal
string value. It means that it will be correctly parsed by kernel.

Signed-off-by: Pali Roh?r <pali.rohar@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 arch/arm/boot/compressed/atags_to_fdt.c |   37 +++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
index 9448aa0..b23748e 100644
--- a/arch/arm/boot/compressed/atags_to_fdt.c
+++ b/arch/arm/boot/compressed/atags_to_fdt.c
@@ -97,6 +97,39 @@ static void merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
 	setprop_string(fdt, "/chosen", "bootargs", cmdline);
 }
 
+static void tohexstr(char * str, int size, unsigned int num)
+{
+	int len = 0;
+	int i, tmp;
+
+	if (size < 4) {
+		if (size > 0)
+			str[0] = 0;
+		return;
+	}
+
+	str[len++] = '0';
+	str[len++] = 'x';
+
+	while (len-1 < size && num) {
+		tmp = num % 16;
+		if (tmp >= 10)
+			tmp += 'A';
+		else
+			tmp += '0';
+		str[len++] = tmp;
+		num /= 16;
+	}
+
+	str[len] = 0;
+
+	for (i = 2; i < 2+(len-2)/2; ++i) {
+		tmp = str[i];
+		str[i] = str[len-i+1];
+		str[len-i+1] = tmp;
+	}
+}
+
 /*
  * Convert and fold provided ATAGs into the provided FDT.
  *
@@ -171,6 +204,10 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
 					cpu_to_fdt32(atag->u.mem.size);
 			}
 
+		} else if (atag->hdr.tag == ATAG_REVISION) {
+			char revision[11];
+			tohexstr(revision, sizeof(revision), atag->u.revision.rev);
+			setprop_string(fdt, "/", "revision", revision);
 		} else if (atag->hdr.tag == ATAG_INITRD2) {
 			uint32_t initrd_start, initrd_size;
 			initrd_start = atag->u.initrd.start;
-- 
1.7.9.5

[PATCH 0/2] ARM: /proc/atags: Export also for DT

[Pali Rohár]

This patch adds support for DT "/atags" and stores ATAG structure to DT.

It depends on "ARM: /proc/cpuinfo: DT: Add support for Revision" patches.

Pali Roh?r (2):
  arm: devtree: Save atags if are in DT atags field
  arm: boot: store ATAG structure into DT atag field

 arch/arm/boot/compressed/atags_to_fdt.c |    6 +++++-
 arch/arm/kernel/devtree.c               |    6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

-- 
1.7.9.5

[PATCH 1/2] arm: devtree: Save atags if are in DT atags field

[Pali Rohár]

This patch creates /proc/atags from DT /atags field.

Signed-off-by: Pali Roh?r <pali.rohar@gmail.com>
---
 arch/arm/kernel/devtree.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index 7e13e27..dd98322 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -29,6 +29,7 @@
 #include <asm/mach-types.h>
 #include <asm/system_info.h>
 
+#include "atags.h"
 
 #ifdef CONFIG_SMP
 extern struct of_cpu_method __cpu_method_of_table[];
@@ -256,5 +257,10 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
 			system_rev = 0;
 	}
 
+	/* Save atags */
+	prop = of_get_flat_dt_prop(dt_root, "atags", NULL);
+	if (prop)
+		save_atags((void *)prop);
+
 	return mdesc;
 }
-- 
1.7.9.5

[PATCH 1/2] arm: devtree: Save atags if are in DT atags field

[Arnd Bergmann]

On Friday 15 May 2015 21:50:06 Pali Roh?r wrote:
> @@ -256,5 +257,10 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
>                         system_rev = 0;
>         }
>  
> +       /* Save atags */
> +       prop = of_get_flat_dt_prop(dt_root, "atags", NULL);
> +       if (prop)
> +               save_atags((void *)prop);
> +
>         return mdesc;
> 

How about checking whether this is actually running on the one board
that needs it first?

I'd rather not introduce something that may end up being considered
an ABI on other machines.

	Arnd

[PATCH 1/2] arm: devtree: Save atags if are in DT atags field

[Tony Lindgren]

* Arnd Bergmann <arnd@arndb.de> [150515 13:11]:
> On Friday 15 May 2015 21:50:06 Pali Roh?r wrote:
> > @@ -256,5 +257,10 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
> >                         system_rev = 0;
> >         }
> >  
> > +       /* Save atags */
> > +       prop = of_get_flat_dt_prop(dt_root, "atags", NULL);
> > +       if (prop)
> > +               save_atags((void *)prop);
> > +
> >         return mdesc;
> > 
> 
> How about checking whether this is actually running on the one board
> that needs it first?
> 
> I'd rather not introduce something that may end up being considered
> an ABI on other machines.

It seems having this within CONFIG_ARM_ATAG_DTB_COMPAT should be
enough here.

Regards,

Tony

[PATCH 2/2] arm: boot: store ATAG structure into DT atags field

[Pali Rohár]

This patch stores existing ATAG structure into DT /atags field
and so previous patch exports it for userspace via /proc/atags.

Signed-off-by: Pali Roh?r <pali.rohar@gmail.com>
---
 arch/arm/boot/compressed/atags_to_fdt.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
index b23748e..4a4a70c 100644
--- a/arch/arm/boot/compressed/atags_to_fdt.c
+++ b/arch/arm/boot/compressed/atags_to_fdt.c
@@ -145,7 +145,7 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
 	 * address and size for each bank */
 	uint32_t mem_reg_property[2 * 2 * NR_BANKS];
 	int memcount = 0;
-	int ret, memsize;
+	int ret, memsize, atag_size;
 
 	/* make sure we've got an aligned pointer */
 	if ((u32)atag_list & 0x3)
@@ -219,6 +219,10 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
 		}
 	}
 
+	/* include the terminating ATAG_NONE */
+	atag_size = (char *)atag - (char *)atag_list + sizeof(struct tag_header);
+	setprop(fdt, "/", "atags", atag_list, atag_size);
+
 	if (memcount) {
 		setprop(fdt, "/memory", "reg", mem_reg_property,
 			4 * memcount * memsize);
-- 
1.7.9.5

[PATCH 2/2] arm: boot: store ATAG structure into DT atags field

[Arnd Bergmann]

On Friday 15 May 2015 21:50:07 Pali Roh?r wrote:
>                 }
>         }
>  
> +       /* include the terminating ATAG_NONE */
> +       atag_size = (char *)atag - (char *)atag_list + sizeof(struct tag_header);
> +       setprop(fdt, "/", "atags", atag_list, atag_size);
> +
>         if (memcount) {
>                 setprop(fdt, "/memory", "reg", mem_reg_property,
>                         4 * memcount * memsize);
> 

The property should probably have a DT binding, and be named "linux,atags".

It may also help to check if the "linux,atags" property already exists and not
create it otherwise. That way we can put it into the n900 dts file and have
it updated by the compat code, but not expose the atags on other platforms
unless they opt in.

	Arnd

[PATCH 2/2] arm: boot: store ATAG structure into DT atags field

[Pali Rohár]

On Friday 15 May 2015 22:12:41 Arnd Bergmann wrote:
> On Friday 15 May 2015 21:50:07 Pali Roh?r wrote:
> >                 }
> >         
> >         }
> > 
> > +       /* include the terminating ATAG_NONE */
> > +       atag_size = (char *)atag - (char *)atag_list +
> > sizeof(struct tag_header); +       setprop(fdt, "/", "atags",
> > atag_list, atag_size);
> > +
> > 
> >         if (memcount) {
> >         
> >                 setprop(fdt, "/memory", "reg", mem_reg_property,
> >                 
> >                         4 * memcount * memsize);
> 
> The property should probably have a DT binding, and be named
> "linux,atags".
> 
> It may also help to check if the "linux,atags" property already
> exists and not create it otherwise. That way we can put it into the
> n900 dts file and have it updated by the compat code, but not expose
> the atags on other platforms unless they opt in.
> 
> 	Arnd

Maybe what would help: Is there a way to tell decompressor/kernel to not 
touch atag memory and then after kernel/board-code starts it save copy 
of atags? I think it is not possible right now, but correct me if I'm 
wrong...

-- 
Pali Roh?r
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150515/bf454953/attachment.sig>

[PATCH 2/2] arm: boot: store ATAG structure into DT atags field

[Arnd Bergmann]

On Friday 15 May 2015 22:16:24 Pali Roh?r wrote:
> On Friday 15 May 2015 22:12:41 Arnd Bergmann wrote:
> > On Friday 15 May 2015 21:50:07 Pali Roh?r wrote:
> > >                 }
> > >         
> > >         }
> > > 
> > > +       /* include the terminating ATAG_NONE */
> > > +       atag_size = (char *)atag - (char *)atag_list +
> > > sizeof(struct tag_header); +       setprop(fdt, "/", "atags",
> > > atag_list, atag_size);
> > > +
> > > 
> > >         if (memcount) {
> > >         
> > >                 setprop(fdt, "/memory", "reg", mem_reg_property,
> > >                 
> > >                         4 * memcount * memsize);
> > 
> > The property should probably have a DT binding, and be named
> > "linux,atags".
> > 
> > It may also help to check if the "linux,atags" property already
> > exists and not create it otherwise. That way we can put it into the
> > n900 dts file and have it updated by the compat code, but not expose
> > the atags on other platforms unless they opt in.
> > 
> >       Arnd
> 
> Maybe what would help: Is there a way to tell decompressor/kernel to not 
> touch atag memory and then after kernel/board-code starts it save copy 
> of atags? I think it is not possible right now, but correct me if I'm 
> wrong...
> 

I don't think that is possible without an incompatible change to the
boot protocol.

	Arnd

[PATCH 2/2] arm: boot: store ATAG structure into DT atags field

[Tony Lindgren]

* Arnd Bergmann <arnd@arndb.de> [150515 13:23]:
> On Friday 15 May 2015 22:16:24 Pali Roh?r wrote:
> > On Friday 15 May 2015 22:12:41 Arnd Bergmann wrote:
> > > On Friday 15 May 2015 21:50:07 Pali Roh?r wrote:
> > > >                 }
> > > >         
> > > >         }
> > > > 
> > > > +       /* include the terminating ATAG_NONE */
> > > > +       atag_size = (char *)atag - (char *)atag_list +
> > > > sizeof(struct tag_header); +       setprop(fdt, "/", "atags",
> > > > atag_list, atag_size);
> > > > +
> > > > 
> > > >         if (memcount) {
> > > >         
> > > >                 setprop(fdt, "/memory", "reg", mem_reg_property,
> > > >                 
> > > >                         4 * memcount * memsize);
> > > 
> > > The property should probably have a DT binding, and be named
> > > "linux,atags".
> > > 
> > > It may also help to check if the "linux,atags" property already
> > > exists and not create it otherwise. That way we can put it into the
> > > n900 dts file and have it updated by the compat code, but not expose
> > > the atags on other platforms unless they opt in.

Using "linux,atags" sounds good to me. And yes checking it with
getprop before doing setprop makes sense.

> > Maybe what would help: Is there a way to tell decompressor/kernel to not 
> > touch atag memory and then after kernel/board-code starts it save copy 
> > of atags? I think it is not possible right now, but correct me if I'm 
> > wrong...
> > 
> 
> I don't think that is possible without an incompatible change to the
> boot protocol.

Agreed, let's keep the changes to minimum.

Looks like with the comments posted all the pending four patches
from Pali become quite a minimal set of three patches if we keep
the rev string as hex.

Regrds,

Tony

[PATCH 0/2] ARM: /proc/atags: Export also for DT

[Russell King - ARM Linux]

On Fri, May 15, 2015 at 09:50:05PM +0200, Pali Roh?r wrote:
> This patch adds support for DT "/atags" and stores ATAG structure to DT.
> 
> It depends on "ARM: /proc/cpuinfo: DT: Add support for Revision" patches.
> 
> Pali Roh?r (2):
>   arm: devtree: Save atags if are in DT atags field
>   arm: boot: store ATAG structure into DT atag field
> 
>  arch/arm/boot/compressed/atags_to_fdt.c |    6 +++++-
>  arch/arm/kernel/devtree.c               |    6 ++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)

What these patches are missing is a description of _why_ this is required
in any of the commit messages.  The commit messages seem to be describing
_what_ the change is doing, which is something that can be seen by reading
the patches, but it leaves the question of why this change is necessary
entirely open.

Please update the commit messages on the next patch revision.

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