| Message ID | 1360365467-25056-11-git-send-email-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 8 Feb 2013, Ben Dooks wrote: > The atag-to-fdt code can now use the conversion function we introduced > previously to avoid it having to know about the endian-ness of the > environment which booted the processor. > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> Acked-by: Nicolas Pitre <nico@linaro.org> > --- > arch/arm/boot/compressed/atags_to_fdt.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c > index aabc02a..a64ec71 100644 > --- a/arch/arm/boot/compressed/atags_to_fdt.c > +++ b/arch/arm/boot/compressed/atags_to_fdt.c > @@ -108,9 +108,9 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space) > return 0; > > /* validate the ATAG */ > - if (atag->hdr.tag != ATAG_CORE || > - (atag->hdr.size != tag_size(tag_core) && > - atag->hdr.size != 2)) > + if (atag->hdr.tag != atag32_to_cpu(ATAG_CORE) || > + (atag->hdr.size != atag32_to_cpu(tag_size(tag_core)) && > + atag->hdr.size != atag32_to_cpu(2))) > return 1; > > /* let's give it all the room it could need */ > @@ -132,17 +132,17 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space) > else > setprop_string(fdt, "/chosen", "bootargs", > atag->u.cmdline.cmdline); > - } else if (atag->hdr.tag == ATAG_MEM) { > + } else if (atag->hdr.tag == atag32_to_cpu(ATAG_MEM)) { > if (memcount >= sizeof(mem_reg_property)/4) > continue; > - if (!atag->u.mem.size) > + if (!atag32_to_cpu(atag->u.mem.size)) > continue; > - mem_reg_property[memcount++] = cpu_to_fdt32(atag->u.mem.start); > - mem_reg_property[memcount++] = cpu_to_fdt32(atag->u.mem.size); > - } else if (atag->hdr.tag == ATAG_INITRD2) { > + mem_reg_property[memcount++] = cpu_to_fdt32(atag32_to_cpu(atag->u.mem.start)); > + mem_reg_property[memcount++] = cpu_to_fdt32(atag32_to_cpu(atag->u.mem.size)); > + } else if (atag->hdr.tag == atag32_to_cpu(ATAG_INITRD2)) { > uint32_t initrd_start, initrd_size; > - initrd_start = atag->u.initrd.start; > - initrd_size = atag->u.initrd.size; > + initrd_start = atag32_to_cpu(atag->u.initrd.start); > + initrd_size = atag32_to_cpu(____atag->u.initrd.size); > setprop_cell(fdt, "/chosen", "linux,initrd-start", > initrd_start); > setprop_cell(fdt, "/chosen", "linux,initrd-end", > -- > 1.7.10.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Fri, Feb 08, 2013 at 11:17:40PM +0000, Ben Dooks wrote: > - if (atag->hdr.tag != ATAG_CORE || > - (atag->hdr.size != tag_size(tag_core) && > - atag->hdr.size != 2)) > + if (atag->hdr.tag != atag32_to_cpu(ATAG_CORE) || > + (atag->hdr.size != atag32_to_cpu(tag_size(tag_core)) && > + atag->hdr.size != atag32_to_cpu(2))) This is wrong. You're saying that "ATAG_CORE" is in LE32 format. It isn't. It's in CPU endian format. If you want to do this kind of thing, you also need to define cpu_to_atag32() macros as well, otherwise this will cause sparse warnings. > - initrd_start = atag->u.initrd.start; > - initrd_size = atag->u.initrd.size; > + initrd_start = atag32_to_cpu(atag->u.initrd.start); > + initrd_size = atag32_to_cpu(____atag->u.initrd.size); Where did those four underscores come from? Has this been built?
On 09/02/13 12:05, Russell King - ARM Linux wrote: > On Fri, Feb 08, 2013 at 11:17:40PM +0000, Ben Dooks wrote: >> - if (atag->hdr.tag != ATAG_CORE || >> - (atag->hdr.size != tag_size(tag_core)&& >> - atag->hdr.size != 2)) >> + if (atag->hdr.tag != atag32_to_cpu(ATAG_CORE) || >> + (atag->hdr.size != atag32_to_cpu(tag_size(tag_core))&& >> + atag->hdr.size != atag32_to_cpu(2))) > > This is wrong. You're saying that "ATAG_CORE" is in LE32 format. It > isn't. It's in CPU endian format. If you want to do this kind of thing, > you also need to define cpu_to_atag32() macros as well, otherwise this > will cause sparse warnings. > >> - initrd_start = atag->u.initrd.start; >> - initrd_size = atag->u.initrd.size; >> + initrd_start = atag32_to_cpu(atag->u.initrd.start); >> + initrd_size = atag32_to_cpu(____atag->u.initrd.size); > > Where did those four underscores come from? Has this been built? I probably missed building this one. I've been mostly testing with DT based systems.
On Mon, 11 Feb 2013, Ben Dooks wrote: > On 09/02/13 12:05, Russell King - ARM Linux wrote: > > On Fri, Feb 08, 2013 at 11:17:40PM +0000, Ben Dooks wrote: > > > - if (atag->hdr.tag != ATAG_CORE || > > > - (atag->hdr.size != tag_size(tag_core)&& > > > - atag->hdr.size != 2)) > > > + if (atag->hdr.tag != atag32_to_cpu(ATAG_CORE) || > > > + (atag->hdr.size != atag32_to_cpu(tag_size(tag_core))&& > > > + atag->hdr.size != atag32_to_cpu(2))) > > > > This is wrong. You're saying that "ATAG_CORE" is in LE32 format. It > > isn't. It's in CPU endian format. If you want to do this kind of thing, > > you also need to define cpu_to_atag32() macros as well, otherwise this > > will cause sparse warnings. > > > > > - initrd_start = atag->u.initrd.start; > > > - initrd_size = atag->u.initrd.size; > > > + initrd_start = atag32_to_cpu(atag->u.initrd.start); > > > + initrd_size = atag32_to_cpu(____atag->u.initrd.size); > > > > Where did those four underscores come from? Has this been built? > > I probably missed building this one. I've been mostly testing with DT > based systems. Is this BE8 mode available on old systems? Or, will those BE8 capable old systems have BE userland compiled for them? Where I want to get to is: do we need to support BE8 mode for ATAG based systems at all, given that most if not all the modern ones should be DT based now? Making the ATAG code BE8 aware is looking to be quite invasive for potentially no users at all. Nicolas
On 12/02/13 21:53, Nicolas Pitre wrote: > On Mon, 11 Feb 2013, Ben Dooks wrote: > >> On 09/02/13 12:05, Russell King - ARM Linux wrote: >>> On Fri, Feb 08, 2013 at 11:17:40PM +0000, Ben Dooks wrote: >>>> - if (atag->hdr.tag != ATAG_CORE || >>>> - (atag->hdr.size != tag_size(tag_core)&& >>>> - atag->hdr.size != 2)) >>>> + if (atag->hdr.tag != atag32_to_cpu(ATAG_CORE) || >>>> + (atag->hdr.size != atag32_to_cpu(tag_size(tag_core))&& >>>> + atag->hdr.size != atag32_to_cpu(2))) >>> >>> This is wrong. You're saying that "ATAG_CORE" is in LE32 format. It >>> isn't. It's in CPU endian format. If you want to do this kind of thing, >>> you also need to define cpu_to_atag32() macros as well, otherwise this >>> will cause sparse warnings. >>> >>>> - initrd_start = atag->u.initrd.start; >>>> - initrd_size = atag->u.initrd.size; >>>> + initrd_start = atag32_to_cpu(atag->u.initrd.start); >>>> + initrd_size = atag32_to_cpu(____atag->u.initrd.size); >>> >>> Where did those four underscores come from? Has this been built? >> >> I probably missed building this one. I've been mostly testing with DT >> based systems. > > Is this BE8 mode available on old systems? Or, will those BE8 > capable old systems have BE userland compiled for them? BE8 is for ARMv6 and ARMv7 form of big endian. ARMv5 is BE32. > Where I want to get to is: do we need to support BE8 mode for ATAG based > systems at all, given that most if not all the modern ones should be DT > based now? Making the ATAG code BE8 aware is looking to be quite > invasive for potentially no users at all. The change is only useful when using a BE8 compiled kernel with a boot loader environment that is LE. Even if the ATAGs do not need fixing up, then we have a issue with uboot checking the zImage magic. I have pushed the ATAGs issues to a new series as it is not necessary for the current work we are doing with highbank primarily.
On Wed, 13 Feb 2013, Ben Dooks wrote: > On 12/02/13 21:53, Nicolas Pitre wrote: > > On Mon, 11 Feb 2013, Ben Dooks wrote: > > > > > On 09/02/13 12:05, Russell King - ARM Linux wrote: > > > > On Fri, Feb 08, 2013 at 11:17:40PM +0000, Ben Dooks wrote: > > > > > - if (atag->hdr.tag != ATAG_CORE || > > > > > - (atag->hdr.size != tag_size(tag_core)&& > > > > > - atag->hdr.size != 2)) > > > > > + if (atag->hdr.tag != atag32_to_cpu(ATAG_CORE) || > > > > > + (atag->hdr.size != atag32_to_cpu(tag_size(tag_core))&& > > > > > + atag->hdr.size != atag32_to_cpu(2))) > > > > > > > > This is wrong. You're saying that "ATAG_CORE" is in LE32 format. It > > > > isn't. It's in CPU endian format. If you want to do this kind of > > > > thing, > > > > you also need to define cpu_to_atag32() macros as well, otherwise this > > > > will cause sparse warnings. > > > > > > > > > - initrd_start = atag->u.initrd.start; > > > > > - initrd_size = atag->u.initrd.size; > > > > > + initrd_start = > > > > > atag32_to_cpu(atag->u.initrd.start); > > > > > + initrd_size = > > > > > atag32_to_cpu(____atag->u.initrd.size); > > > > > > > > Where did those four underscores come from? Has this been built? > > > > > > I probably missed building this one. I've been mostly testing with DT > > > based systems. > > > > Is this BE8 mode available on old systems? Or, will those BE8 > > capable old systems have BE userland compiled for them? > > BE8 is for ARMv6 and ARMv7 form of big endian. ARMv5 is BE32. I think that by now all ARMv6+ targets should be DT enabled. So you shouldn't have to care about ATAGs at all. > > Where I want to get to is: do we need to support BE8 mode for ATAG based > > systems at all, given that most if not all the modern ones should be DT > > based now? Making the ATAG code BE8 aware is looking to be quite > > invasive for potentially no users at all. > > The change is only useful when using a BE8 compiled kernel with a > boot loader environment that is LE. That's what CONFIG_ARM_ATAG_DTB_COMPAT is for. So only that code would need to swap the ATAG data. > Even if the ATAGs do not need > fixing up, then we have a issue with uboot checking the zImage magic. I gave you a patch for that. > I have pushed the ATAGs issues to a new series as it is not necessary > for the current work we are doing with highbank primarily. I don't think it is needed at all given that BE8 is available mainly on targets supporting DT, and is likely to be used on targets that do support only DT. Nicolas
On 14/02/13 04:43, Nicolas Pitre wrote: > On Wed, 13 Feb 2013, Ben Dooks wrote: > >> On 12/02/13 21:53, Nicolas Pitre wrote: >>> On Mon, 11 Feb 2013, Ben Dooks wrote: >>> >>>> On 09/02/13 12:05, Russell King - ARM Linux wrote: >>>>> On Fri, Feb 08, 2013 at 11:17:40PM +0000, Ben Dooks wrote: >>>>>> - if (atag->hdr.tag != ATAG_CORE || >>>>>> - (atag->hdr.size != tag_size(tag_core)&& >>>>>> - atag->hdr.size != 2)) >>>>>> + if (atag->hdr.tag != atag32_to_cpu(ATAG_CORE) || >>>>>> + (atag->hdr.size != atag32_to_cpu(tag_size(tag_core))&& >>>>>> + atag->hdr.size != atag32_to_cpu(2))) >>>>> >>>>> This is wrong. You're saying that "ATAG_CORE" is in LE32 format. It >>>>> isn't. It's in CPU endian format. If you want to do this kind of >>>>> thing, >>>>> you also need to define cpu_to_atag32() macros as well, otherwise this >>>>> will cause sparse warnings. >>>>> >>>>>> - initrd_start = atag->u.initrd.start; >>>>>> - initrd_size = atag->u.initrd.size; >>>>>> + initrd_start = >>>>>> atag32_to_cpu(atag->u.initrd.start); >>>>>> + initrd_size = >>>>>> atag32_to_cpu(____atag->u.initrd.size); >>>>> >>>>> Where did those four underscores come from? Has this been built? >>>> >>>> I probably missed building this one. I've been mostly testing with DT >>>> based systems. >>> >>> Is this BE8 mode available on old systems? Or, will those BE8 >>> capable old systems have BE userland compiled for them? >> >> BE8 is for ARMv6 and ARMv7 form of big endian. ARMv5 is BE32. > > I think that by now all ARMv6+ targets should be DT enabled. So you > shouldn't have to care about ATAGs at all. > >>> Where I want to get to is: do we need to support BE8 mode for ATAG based >>> systems at all, given that most if not all the modern ones should be DT >>> based now? Making the ATAG code BE8 aware is looking to be quite >>> invasive for potentially no users at all. >> >> The change is only useful when using a BE8 compiled kernel with a >> boot loader environment that is LE. > > That's what CONFIG_ARM_ATAG_DTB_COMPAT is for. So only that code would > need to swap the ATAG data. > >> Even if the ATAGs do not need >> fixing up, then we have a issue with uboot checking the zImage magic. > > I gave you a patch for that. > >> I have pushed the ATAGs issues to a new series as it is not necessary >> for the current work we are doing with highbank primarily. > > I don't think it is needed at all given that BE8 is available mainly on > targets supporting DT, and is likely to be used on targets that do > support only DT. Some devices we have still only have ATAG booting in u-boot, so it i possible that it may have to stick around to allow the fdt to be updated with any extra command line stuff...
diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c index aabc02a..a64ec71 100644 --- a/arch/arm/boot/compressed/atags_to_fdt.c +++ b/arch/arm/boot/compressed/atags_to_fdt.c @@ -108,9 +108,9 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space) return 0; /* validate the ATAG */ - if (atag->hdr.tag != ATAG_CORE || - (atag->hdr.size != tag_size(tag_core) && - atag->hdr.size != 2)) + if (atag->hdr.tag != atag32_to_cpu(ATAG_CORE) || + (atag->hdr.size != atag32_to_cpu(tag_size(tag_core)) && + atag->hdr.size != atag32_to_cpu(2))) return 1; /* let's give it all the room it could need */ @@ -132,17 +132,17 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space) else setprop_string(fdt, "/chosen", "bootargs", atag->u.cmdline.cmdline); - } else if (atag->hdr.tag == ATAG_MEM) { + } else if (atag->hdr.tag == atag32_to_cpu(ATAG_MEM)) { if (memcount >= sizeof(mem_reg_property)/4) continue; - if (!atag->u.mem.size) + if (!atag32_to_cpu(atag->u.mem.size)) continue; - mem_reg_property[memcount++] = cpu_to_fdt32(atag->u.mem.start); - mem_reg_property[memcount++] = cpu_to_fdt32(atag->u.mem.size); - } else if (atag->hdr.tag == ATAG_INITRD2) { + mem_reg_property[memcount++] = cpu_to_fdt32(atag32_to_cpu(atag->u.mem.start)); + mem_reg_property[memcount++] = cpu_to_fdt32(atag32_to_cpu(atag->u.mem.size)); + } else if (atag->hdr.tag == atag32_to_cpu(ATAG_INITRD2)) { uint32_t initrd_start, initrd_size; - initrd_start = atag->u.initrd.start; - initrd_size = atag->u.initrd.size; + initrd_start = atag32_to_cpu(atag->u.initrd.start); + initrd_size = atag32_to_cpu(____atag->u.initrd.size); setprop_cell(fdt, "/chosen", "linux,initrd-start", initrd_start); setprop_cell(fdt, "/chosen", "linux,initrd-end",
The atag-to-fdt code can now use the conversion function we introduced previously to avoid it having to know about the endian-ness of the environment which booted the processor. Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- arch/arm/boot/compressed/atags_to_fdt.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)