MTD: Fix Orion NAND driver compilation with ARM OABI
in [Kernel]
|
Prev: [RFC] Yet another (third) dt3155 driver PCI/video4linux compliant
Next: [PATCH] intel-iommu: don't call domain_exit if can not attach with iommu
From: David Woodhouse on 20 Mar 2010 05:50 On Sat, 2010-03-20 at 10:55 +0200, Paulius Zaleckas wrote: > We must tell GCC to use even register for variable passed > to ldrd instruction. Without this patch GCC 4.2.1 puts this > variable to r2/r3 on EABI and r3/r4 on OABI, so force it to > r2/r3. This does not change anything when EABI and OABI > compilation works OK. > > Without this patch and with OABI I get: > /tmp/ccMkwOCs.s:63: Error: first destination register must be even -- `ldrd r3,[ip]' > make[5]: *** [drivers/mtd/nand/orion_nand.o] Error 1 ... > - uint64_t x; > + /* > + * force x variable to r2/r3 registers since ldrd instruction > + * requires first register to be even. > + */ > + register uint64_t x asm ("r2"); > + > asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base)); Hm, isn't there an asm constraint which will force it into an appropriate register pair? Failing that, "=&2,4,6,8" ought to work. (Um, and why the earlyclobber? Why can't io_base be passed in in one of the same registers?) We should try to avoid making our constraints more restrictive than they need to be. -- David Woodhouse Open Source Technology Centre David.Woodhouse(a)intel.com Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Paulius Zaleckas on 20 Mar 2010 07:30 On Sat, Mar 20, 2010 at 11:41 AM, David Woodhouse <dwmw2(a)infradead.org> wrote: > On Sat, 2010-03-20 at 10:55 +0200, Paulius Zaleckas wrote: >> We must tell GCC to use even register for variable passed >> to ldrd instruction. Without this patch GCC 4.2.1 puts this >> variable to r2/r3 on EABI and r3/r4 on OABI, so force it to >> r2/r3. This does not change anything when EABI and OABI >> compilation works OK. >> >> Without this patch and with OABI I get: >> /tmp/ccMkwOCs.s:63: Error: first destination register must be even -- `ldrd r3,[ip]' >> make[5]: *** [drivers/mtd/nand/orion_nand.o] Error 1 > > �... >> - � � � � � � uint64_t x; >> + � � � � � � /* >> + � � � � � � �* force x variable to r2/r3 registers since ldrd instruction >> + � � � � � � �* requires first register to be even. >> + � � � � � � �*/ >> + � � � � � � register uint64_t x asm ("r2"); >> + >> � � � � � � � asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base)); > > Hm, isn't there an asm constraint which will force it into an > appropriate register pair? Not that I know of... > Failing that, "=&2,4,6,8" ought to work. No, fails with error: matching constraint not valid in output operand > (Um, and why the earlyclobber? Why can't io_base be passed in in one of > the same registers?) > > We should try to avoid making our constraints more restrictive than they > need to be. > > -- > David Woodhouse � � � � � � � � � � � � � �Open Source Technology Centre > David.Woodhouse(a)intel.com � � � � � � � � � � � � � � �Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Nicolas Pitre on 20 Mar 2010 11:40 On Sat, 20 Mar 2010, Mikael Pettersson wrote: > I missed the start of this thread, but looking at orion_nand.c I fail to > see why you'd need to mess with inline asm for reading a sequence of u64 > values from an I/O location to an array. The reason for doing so with inline asm in the first place was not discussed in this thread. > Rewriting that to proper C generates a nice ldrd;strd loop with my gcc-4.4.3. Sure, GCC version 4.4.3 does the right thing. Not all of them do. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jamie Lokier on 21 Mar 2010 13:20 David Woodhouse wrote: > Strictly speaking, I think your version is wrong -- although you force > the variable 'x' to be stored in r2/r3, you don't actually force GCC to > use r2/r3 as the output registers for the asm statement -- it could > happily use other registers, then move their contents into r2/r3 > afterwards. We used to do that a lot in the syscall macros in <asm/unistd.h>, on a lot of architectures. Were they all broken? > Obviously it _won't_ do that most of the time, but it _could_. GCC PR > #15089 was filed for the fact that sometimes it does, but I think Nico > was missing the point -- GCC is _allowed_ to do that, and if it makes > you sad then you should be asking for better assembly constraints which > would allow you to tell it not to. From the GCC info documentation: Sometimes you need to make an `asm' operand be a specific register, but there's no matching constraint letter for that register _by itself_. To force the operand into that register, use a local variable for the operand and specify the register in the variable declaration. *Note Explicit Reg Vars::. Then for the `asm' operand, use any register constraint letter that matches the register: register int *p1 asm ("r0") = ...; register int *p2 asm ("r1") = ...; register int *result asm ("r0"); asm ("sysint" : "=r" (result) : "0" (p1), "r" (p2)); Fwiw, that note is present in GCC-4.0.1, but not GCC-3.3.6. But we've depended on that behaviour for a long time. Note that we've depended on GCC not copying values with a dereferenced memory location for a long time too: E.g. "+m" (*ptr) is used a lot in spinlocks. I'm sure I've read email confirmation (on these very lists) that GCC will always work with a memory constraint used in that way, without copying the value to/from a different location such as a stack slot. But suprisingly, the GCC documentation says: Extended asm supports input-output or read-write operands. Use the constraint character `+' to indicate such an operand and list it with the output operands. You should only use read-write operands when the constraints for the operand (or the operand in which only some of the bits are to be changed) allow a register. ^^^^^^^^^^^^^^^^ Maybe we're relying on undefined GCC behaviour for the "+m" constraint? > See the __asmeq() macro in <asm/system.h> for a dirty hack which will > check which registers are used and abort at compile time, although your > compilation is going to fail anyway so I'm not sure it makes much of a > difference in this particular case. > > The real fix here is to add an asm constraint to GCC which allows you to > specify "any even GPR" (or whatever's most suitable for the ldrd > instruction). Being able to give specific registers, like you can on > other architectures, would be useful too. See above GCC documentation for using register variables to designate specific registers. Many supported architectures don't have asm letter constraints for each register - hence so many of the old _syscallN macros in <asm/unistd.h> having to use register variables. I am surprised GCC doesn't have a constraint for "any even register suitable for ldrd" on ARM, but I've just checked gcc-4.4.3 and it doesn't. However, if I'm reading the source correctly, if not compiling for Thumb-1, and GCC believes the target machine supports ldrd, then all doubleword values are constrained to an even register pair anyway. That's why GCC itself does not need an even-register constraint letter. ....Which is I guess why it throws up only with OABI, or with pre-arm5e archs: GCC doesn't consider OABI targets to support ldrd. (It's actually some more obscure condition, let's not go there). Something else from the lovely GCC source: mfix-cortex-m3-ldrd Target Report Var(fix_cm3_ldrd) Init(2) Avoid overlapping destination and address registers on LDRD instructions that may trigger Cortex-M3 errata. In other words, the "=&" earlyclobber *is* needed on Cortex-M3. Enjoy! -- Jamie -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Nicolas Pitre on 25 Mar 2010 12:30
On Thu, 25 Mar 2010, Paulius Zaleckas wrote: > We must tell GCC to use even register for variable passed > to ldrd instruction. Without this patch GCC 4.2.1 puts this > variable to r2/r3 on EABI and r3/r4 on OABI, so force it to > r2/r3. This does not change anything when EABI and OABI > compilation works OK. > > Without this patch and with OABI I get: > CC drivers/mtd/nand/orion_nand.o > /tmp/ccMkwOCs.s: Assembler messages: > /tmp/ccMkwOCs.s:63: Error: first destination register must be even -- `ldrd r3,[ip]' > make[5]: *** [drivers/mtd/nand/orion_nand.o] Error 1 > > Signed-off-by: Paulius Zaleckas <paulius.zaleckas(a)gmail.com> Acked-by: Nicolas Pitre <nico(a)fluxnic.net> > --- > > drivers/mtd/nand/orion_nand.c | 8 +++++++- > 1 files changed, 7 insertions(+), 1 deletions(-) > > diff --git a/drivers/mtd/nand/orion_nand.c b/drivers/mtd/nand/orion_nand.c > index f59c074..d60fc57 100644 > --- a/drivers/mtd/nand/orion_nand.c > +++ b/drivers/mtd/nand/orion_nand.c > @@ -60,7 +60,13 @@ static void orion_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) > } > buf64 = (uint64_t *)buf; > while (i < len/8) { > - uint64_t x; > + /* > + * Since GCC has no proper constraint (PR 43518) > + * force x variable to r2/r3 registers as ldrd instruction > + * requires first register to be even. > + */ > + register uint64_t x asm ("r2"); > + > asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base)); > buf64[i++] = x; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |