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: Jamie Lokier on 25 Mar 2010 16:50 Paulius Zaleckas wrote: > Signed-off-by: Paulius Zaleckas <paulius.zaleckas(a)gmail.com> It's probably worth including the people who weighed in on the discussion with 'Cc:' headers. > - 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; The "register...asm" looks fine, but it occurs to me the constraints are too weak (and they were before), so GCC could optimise that to the wrong behaviour. The "volatile" prevents GCC deleting the asm if it's output isn't used, but it doesn't stop GCC from reordering the asms, for example if it decides to unroll the loop. It probably won't reorder in that case, but it could. The result would be out of order values stored into buf[]. It could even move the ldrd earlier than the prior byte accesses, or after the later byte accesses. Any one of these should fix it: - Make io_base a pointer-to-volatile-u64 or cast it in the asm, and make sure to dereference it and use an "m" constraint (or tighter, such as "Q", if ldrd needs it). It must be u64, not pointer-to-void, to tell GCC the size. That tells GCC which memory the asm accesses, and the volatile dereference should tell GCC not to reorder them in principle (but the GCC manual doesn't make a specific promise about this for asms). With a proper memory input with the correct size, in principle "asm volatile" can be changed to just "asm", but I'm not entirely convinced GCC will honour the volatile on the pointer, so I'd leave it on the asm too. - Add "memory" to the asm's clobbers. Although it doesn't write, it does change the visible memory that *io_base sees, and anyway GCC's manual says to use "memory" clobber when the asm does unpredictable memory reads too. With that added, you still need the volatile keyword after asm, because the memory is not listed in the inputs or outputs (only the address is). The GCC manual explains that "asm volatile" is needed in that case. This is slightly less good because it'd prevent reordering writes to buf[i++] if GCC unrolled the loop. - Put barrier() before and after the asm, which is equivalent to adding a "memory" clobber (least good). You aren't supposed to dereference pointers used with read{b,w,l} anyway. It doesn't matter in this driver because we "know" it's only used on an SoC where read{b,w,l} don't do any address translation. But will that always be true? I suppose the cleanest approach is to define readq, the 64-bit analogue of readl, and use that here. x86 already defines readq, so it's got precedent. -- 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: Paulius Zaleckas on 31 Mar 2010 08:20 On 03/25/2010 06:26 PM, Nicolas Pitre wrote: > 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> David, Will you take this patch? Or you are waiting till I will add all Cc as Jamie suggested? > >> --- >> >> 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/
From: Artem Bityutskiy on 23 Apr 2010 08:10 On Wed, 2010-03-31 at 15:01 +0300, Paulius Zaleckas wrote: > On 03/25/2010 06:26 PM, Nicolas Pitre wrote: > > 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> > > David, > > Will you take this patch? > Or you are waiting till I will add all Cc as Jamie suggested? Meanwhile, I've pushed your patch to my l2-mtd-2.6.git / dunno. -- Best Regards, Artem Bityutskiy (Артём Битюцкий) -- 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 23 Apr 2010 09:00 On Fri, 23 Apr 2010, Artem Bityutskiy wrote: > On Wed, 2010-03-31 at 15:01 +0300, Paulius Zaleckas wrote: > > On 03/25/2010 06:26 PM, Nicolas Pitre wrote: > > > 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> > > > > David, > > > > Will you take this patch? > > Or you are waiting till I will add all Cc as Jamie suggested? > > Meanwhile, I've pushed your patch to my l2-mtd-2.6.git / dunno. I think it should go to mainline. It is not perfect, but still better than the current situation. 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: Artem Bityutskiy on 23 Apr 2010 11:20
On Fri, 2010-04-23 at 08:54 -0400, Nicolas Pitre wrote: > On Fri, 23 Apr 2010, Artem Bityutskiy wrote: > > > On Wed, 2010-03-31 at 15:01 +0300, Paulius Zaleckas wrote: > > > On 03/25/2010 06:26 PM, Nicolas Pitre wrote: > > > > 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> > > > > > > David, > > > > > > Will you take this patch? > > > Or you are waiting till I will add all Cc as Jamie suggested? > > > > Meanwhile, I've pushed your patch to my l2-mtd-2.6.git / dunno. > > I think it should go to mainline. It is not perfect, but still better > than the current situation. Fine with me, but not up to me. But I guess Andrew could merge it. -- Best Regards, Artem Bityutskiy (Артём Битюцкий) -- 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/ |