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
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 <[email protected]>
Acked-by: Pavel Machek <[email protected]>
---
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
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 <[email protected]>
Acked-by: Pavel Machek <[email protected]>
---
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
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 <[email protected]>
> Acked-by: Pavel Machek <[email protected]>
+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
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 <[email protected]>
> > Acked-by: Pavel Machek <[email protected]>
>
> +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
[email protected]
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 <[email protected]>
> > > Acked-by: Pavel Machek <[email protected]>
> >
> > +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
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 <[email protected]>
> > > > Acked-by: Pavel Machek <[email protected]>
> > >
> > > +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
[email protected]
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
This patch creates /proc/atags from DT /atags field.
Signed-off-by: Pali Rohár <[email protected]>
---
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
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 <[email protected]>
---
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
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
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
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
[email protected]
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
* Pali Rohár <[email protected]> [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
* Arnd Bergmann <[email protected]> [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
* Arnd Bergmann <[email protected]> [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
On Wednesday 24 June 2015 22:01:38 Tony Lindgren wrote:
> * Pali Rohár <[email protected]> [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
[email protected]
* Pali Rohár <[email protected]> [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
On Thursday 25 June 2015 00:22:05 Tony Lindgren wrote:
> * Pali Rohár <[email protected]> [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
[email protected]
* Pali Rohár <[email protected]> [150625 00:29]:
> On Thursday 25 June 2015 00:22:05 Tony Lindgren wrote:
> > * Pali Rohár <[email protected]> [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
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.
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.
On Thursday 25 June 2015 07:01:38 Tony Lindgren wrote:
> * Pali Rohár <[email protected]> [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
[email protected]
* Pali Rohár <[email protected]> [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
On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
> * Pali Rohár <[email protected]> [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
[email protected]
* Pali Rohár <[email protected]> [150706 06:14]:
> On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
> > * Pali Rohár <[email protected]> [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
> [email protected]
On Mon, Jul 6, 2015 at 7:31 AM, Tony Lindgren <[email protected]> wrote:
> * Pali Rohár <[email protected]> [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
On Mon, Jul 6, 2015 at 8:12 AM, Pali Rohár <[email protected]> wrote:
> On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
>> * Pali Rohár <[email protected]> [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
> [email protected]
* Rob Herring <[email protected]> [150706 08:23]:
> On Mon, Jul 6, 2015 at 7:31 AM, Tony Lindgren <[email protected]> wrote:
> > * Pali Rohár <[email protected]> [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
On Monday 06 July 2015 17:22:58 Rob Herring wrote:
> On Mon, Jul 6, 2015 at 8:12 AM, Pali Rohár <[email protected]> wrote:
> > On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
> >> * Pali Rohár <[email protected]> [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
> > [email protected]
--
Pali Rohár
[email protected]
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
[email protected]
On Mon, Jul 6, 2015 at 11:20 AM, Pali Rohár <[email protected]> wrote:
> On Monday 06 July 2015 17:22:58 Rob Herring wrote:
>> On Mon, Jul 6, 2015 at 8:12 AM, Pali Rohár <[email protected]> wrote:
>> > On Monday 06 July 2015 14:31:27 Tony Lindgren wrote:
>> >> * Pali Rohár <[email protected]> [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