[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Paulius Zaleckas]

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

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

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Paulius Zaleckas]

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

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

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Paulius Zaleckas]

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

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

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

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

Acked-by: Nicolas Pitre <nico@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;
>  	}
> 

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

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

Acked-by: Nicolas Pitre <nico@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;
>  	}
> 

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

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

Acked-by: Nicolas Pitre <nico@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;
>  	}
> 

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Jamie Lokier]

Paulius Zaleckas wrote:
> Signed-off-by: Paulius Zaleckas <paulius.zaleckas@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

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Jamie Lokier]

Paulius Zaleckas wrote:
> Signed-off-by: Paulius Zaleckas <paulius.zaleckas@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

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Jamie Lokier]

Paulius Zaleckas wrote:
> Signed-off-by: Paulius Zaleckas <paulius.zaleckas@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

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Paulius Zaleckas]

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@gmail.com>
>
> Acked-by: Nicolas Pitre<nico@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;
>>   	}
>>

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Paulius Zaleckas]

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@gmail.com>
>
> Acked-by: Nicolas Pitre<nico@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;
>>   	}
>>

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Paulius Zaleckas]

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@gmail.com>
>
> Acked-by: Nicolas Pitre<nico@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;
>>   	}
>>

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Artem Bityutskiy]

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@gmail.com>
> >
> > Acked-by: Nicolas Pitre<nico@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 (Артём Битюцкий)

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Artem Bityutskiy]

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@gmail.com>
> >
> > Acked-by: Nicolas Pitre<nico@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 (Артём Битюцкий)

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Artem Bityutskiy]

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@gmail.com>
> >
> > Acked-by: Nicolas Pitre<nico@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 (????? ????????)

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

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@gmail.com>
> > >
> > > Acked-by: Nicolas Pitre<nico@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

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

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@gmail.com>
> > >
> > > Acked-by: Nicolas Pitre<nico@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

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

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@gmail.com>
> > >
> > > Acked-by: Nicolas Pitre<nico@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

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Artem Bityutskiy]

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@gmail.com>
> > > >
> > > > Acked-by: Nicolas Pitre<nico@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 (Артём Битюцкий)

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Artem Bityutskiy]

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@gmail.com>
> > > >
> > > > Acked-by: Nicolas Pitre<nico@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 (Артём Битюцкий)

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Artem Bityutskiy]

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@gmail.com>
> > > >
> > > > Acked-by: Nicolas Pitre<nico@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 (????? ????????)

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Fri, 23 Apr 2010, Artem Bityutskiy wrote:

> 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@gmail.com>
> > > > >
> > > > > Acked-by: Nicolas Pitre<nico@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.

The patch is providing a fix to my own code, and I acked it already. The 
root of the problem is a defficiency in gcc, and dwmw2 asked that a PR 
be filled for that and a reference to it added to the patch.  This has 
been done.


Nicolas

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Fri, 23 Apr 2010, Artem Bityutskiy wrote:

> 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@gmail.com>
> > > > >
> > > > > Acked-by: Nicolas Pitre<nico@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.

The patch is providing a fix to my own code, and I acked it already. The 
root of the problem is a defficiency in gcc, and dwmw2 asked that a PR 
be filled for that and a reference to it added to the patch.  This has 
been done.


Nicolas

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Fri, 23 Apr 2010, Artem Bityutskiy wrote:

> 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@gmail.com>
> > > > >
> > > > > Acked-by: Nicolas Pitre<nico@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.

The patch is providing a fix to my own code, and I acked it already. The 
root of the problem is a defficiency in gcc, and dwmw2 asked that a PR 
be filled for that and a reference to it added to the patch.  This has 
been done.


Nicolas

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Andrew Morton]

On Fri, 23 Apr 2010 13:50:10 -0400 (EDT)
Nicolas Pitre <nico@fluxnic.net> wrote:

> > Fine with me, but not up to me. But I guess Andrew could merge it.
> 
> The patch is providing a fix to my own code, and I acked it already. The 
> root of the problem is a defficiency in gcc, and dwmw2 asked that a PR 
> be filled for that and a reference to it added to the patch.  This has 
> been done.

Well, I merged it and can send it upstream.  But Jamie's observations
give me pause?

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Andrew Morton]

On Fri, 23 Apr 2010 13:50:10 -0400 (EDT)
Nicolas Pitre <nico@fluxnic.net> wrote:

> > Fine with me, but not up to me. But I guess Andrew could merge it.
> 
> The patch is providing a fix to my own code, and I acked it already. The 
> root of the problem is a defficiency in gcc, and dwmw2 asked that a PR 
> be filled for that and a reference to it added to the patch.  This has 
> been done.

Well, I merged it and can send it upstream.  But Jamie's observations
give me pause?

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Andrew Morton]

On Fri, 23 Apr 2010 13:50:10 -0400 (EDT)
Nicolas Pitre <nico@fluxnic.net> wrote:

> > Fine with me, but not up to me. But I guess Andrew could merge it.
> 
> The patch is providing a fix to my own code, and I acked it already. The 
> root of the problem is a defficiency in gcc, and dwmw2 asked that a PR 
> be filled for that and a reference to it added to the patch.  This has 
> been done.

Well, I merged it and can send it upstream.  But Jamie's observations
give me pause?

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Fri, 23 Apr 2010, Andrew Morton wrote:

> On Fri, 23 Apr 2010 13:50:10 -0400 (EDT)
> Nicolas Pitre <nico@fluxnic.net> wrote:
> 
> > > Fine with me, but not up to me. But I guess Andrew could merge it.
> > 
> > The patch is providing a fix to my own code, and I acked it already. The 
> > root of the problem is a defficiency in gcc, and dwmw2 asked that a PR 
> > be filled for that and a reference to it added to the patch.  This has 
> > been done.
> 
> Well, I merged it and can send it upstream.  But Jamie's observations
> give me pause?

What observation?  Let me find that.


Nicolas

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Fri, 23 Apr 2010, Andrew Morton wrote:

> On Fri, 23 Apr 2010 13:50:10 -0400 (EDT)
> Nicolas Pitre <nico@fluxnic.net> wrote:
> 
> > > Fine with me, but not up to me. But I guess Andrew could merge it.
> > 
> > The patch is providing a fix to my own code, and I acked it already. The 
> > root of the problem is a defficiency in gcc, and dwmw2 asked that a PR 
> > be filled for that and a reference to it added to the patch.  This has 
> > been done.
> 
> Well, I merged it and can send it upstream.  But Jamie's observations
> give me pause?

What observation?  Let me find that.


Nicolas

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Fri, 23 Apr 2010, Andrew Morton wrote:

> On Fri, 23 Apr 2010 13:50:10 -0400 (EDT)
> Nicolas Pitre <nico@fluxnic.net> wrote:
> 
> > > Fine with me, but not up to me. But I guess Andrew could merge it.
> > 
> > The patch is providing a fix to my own code, and I acked it already. The 
> > root of the problem is a defficiency in gcc, and dwmw2 asked that a PR 
> > be filled for that and a reference to it added to the patch.  This has 
> > been done.
> 
> Well, I merged it and can send it upstream.  But Jamie's observations
> give me pause?

What observation?  Let me find that.


Nicolas

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Thu, 25 Mar 2010, Jamie Lokier wrote:

> Paulius Zaleckas wrote:
> > Signed-off-by: Paulius Zaleckas <paulius.zaleckas@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.

I don't see how that could happen.  The store into buf[] puts a 
dependency on the output constraint of the inline asm statement.  And by 
vertue of being volatile, gcc cannot cache the result of the output from 
the asm as if it was a pure function.

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

The LDRD has special range constraints on its addressing mode which is 
not expressable with any of the available gcc memory constraints.

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

But yet it is not all ARM variants that can do 64-bit accesses.  
Anything pre ARMv5 doesn't have the LDRD instruction, and the equivalent 
LDM is not a possible substitute with regard to memory bus access 
either.

So I'd prefer to keep it as an obvious local exception that happens to 
exploit some specifics of the actual hardware implementation rather than 
something that was architecturally defined.


Nicolas

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Thu, 25 Mar 2010, Jamie Lokier wrote:

> Paulius Zaleckas wrote:
> > Signed-off-by: Paulius Zaleckas <paulius.zaleckas@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.

I don't see how that could happen.  The store into buf[] puts a 
dependency on the output constraint of the inline asm statement.  And by 
vertue of being volatile, gcc cannot cache the result of the output from 
the asm as if it was a pure function.

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

The LDRD has special range constraints on its addressing mode which is 
not expressable with any of the available gcc memory constraints.

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

But yet it is not all ARM variants that can do 64-bit accesses.  
Anything pre ARMv5 doesn't have the LDRD instruction, and the equivalent 
LDM is not a possible substitute with regard to memory bus access 
either.

So I'd prefer to keep it as an obvious local exception that happens to 
exploit some specifics of the actual hardware implementation rather than 
something that was architecturally defined.


Nicolas

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Thu, 25 Mar 2010, Jamie Lokier wrote:

> Paulius Zaleckas wrote:
> > Signed-off-by: Paulius Zaleckas <paulius.zaleckas@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.

I don't see how that could happen.  The store into buf[] puts a 
dependency on the output constraint of the inline asm statement.  And by 
vertue of being volatile, gcc cannot cache the result of the output from 
the asm as if it was a pure function.

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

The LDRD has special range constraints on its addressing mode which is 
not expressable with any of the available gcc memory constraints.

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

But yet it is not all ARM variants that can do 64-bit accesses.  
Anything pre ARMv5 doesn't have the LDRD instruction, and the equivalent 
LDM is not a possible substitute with regard to memory bus access 
either.

So I'd prefer to keep it as an obvious local exception that happens to 
exploit some specifics of the actual hardware implementation rather than 
something that was architecturally defined.


Nicolas

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Jamie Lokier]

Nicolas Pitre wrote:
> On Thu, 25 Mar 2010, Jamie Lokier wrote:
> > >  		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.
> 
> I don't see how that could happen.  The store into buf[] puts a 
> dependency on the output constraint of the inline asm statement.  And by 
> vertue of being volatile, gcc cannot cache the result of the output from 
> the asm as if it was a pure function.

The store into buf[] dependency doesn't stop this, after unrolling:

    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
    buf64[i++] = x;
    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
    buf64[i++] = x;

from being reordered as this

    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x2) : "r" (io_base));
    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x1) : "r" (io_base));
    buf64[i++] = x1;
    buf64[i++] = x2;

because the asm doesn't depend on memory, just register inputs and
outputs;

I'm not sure what you mean about the volatile stopping gcc from
treating the asm as a pure function.  Is that meaning of volatile in
the asm documentation?  (volatile on asm doesn't mean the same as
volatile on a function, or volatile on a pointer).

> > 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).
> 
> The LDRD has special range constraints on its addressing mode which is 
> not expressable with any of the available gcc memory constraints.

    'Q'
          A memory reference where the exact address is in a single
          register (''m'' is preferable for 'asm' statements)

If 'r' is good enough for io_base, 'Q' should be good enough for *io_base.

-- Jamie

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Jamie Lokier]

Nicolas Pitre wrote:
> On Thu, 25 Mar 2010, Jamie Lokier wrote:
> > >  		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.
> 
> I don't see how that could happen.  The store into buf[] puts a 
> dependency on the output constraint of the inline asm statement.  And by 
> vertue of being volatile, gcc cannot cache the result of the output from 
> the asm as if it was a pure function.

The store into buf[] dependency doesn't stop this, after unrolling:

    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
    buf64[i++] = x;
    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
    buf64[i++] = x;

from being reordered as this

    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x2) : "r" (io_base));
    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x1) : "r" (io_base));
    buf64[i++] = x1;
    buf64[i++] = x2;

because the asm doesn't depend on memory, just register inputs and
outputs;

I'm not sure what you mean about the volatile stopping gcc from
treating the asm as a pure function.  Is that meaning of volatile in
the asm documentation?  (volatile on asm doesn't mean the same as
volatile on a function, or volatile on a pointer).

> > 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).
> 
> The LDRD has special range constraints on its addressing mode which is 
> not expressable with any of the available gcc memory constraints.

    'Q'
          A memory reference where the exact address is in a single
          register (''m'' is preferable for 'asm' statements)

If 'r' is good enough for io_base, 'Q' should be good enough for *io_base.

-- Jamie

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Jamie Lokier]

Nicolas Pitre wrote:
> On Thu, 25 Mar 2010, Jamie Lokier wrote:
> > >  		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.
> 
> I don't see how that could happen.  The store into buf[] puts a 
> dependency on the output constraint of the inline asm statement.  And by 
> vertue of being volatile, gcc cannot cache the result of the output from 
> the asm as if it was a pure function.

The store into buf[] dependency doesn't stop this, after unrolling:

    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
    buf64[i++] = x;
    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
    buf64[i++] = x;

from being reordered as this

    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x2) : "r" (io_base));
    asm volatile ("ldrd\t%0, [%1]" : "=&r" (x1) : "r" (io_base));
    buf64[i++] = x1;
    buf64[i++] = x2;

because the asm doesn't depend on memory, just register inputs and
outputs;

I'm not sure what you mean about the volatile stopping gcc from
treating the asm as a pure function.  Is that meaning of volatile in
the asm documentation?  (volatile on asm doesn't mean the same as
volatile on a function, or volatile on a pointer).

> > 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).
> 
> The LDRD has special range constraints on its addressing mode which is 
> not expressable with any of the available gcc memory constraints.

    'Q'
          A memory reference where the exact address is in a single
          register (''m'' is preferable for 'asm' statements)

If 'r' is good enough for io_base, 'Q' should be good enough for *io_base.

-- Jamie

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Jamie Lokier]

Jamie Lokier wrote:
> Nicolas Pitre wrote:
> > On Thu, 25 Mar 2010, Jamie Lokier wrote:
> > > 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).
> > 
> > The LDRD has special range constraints on its addressing mode which is 
> > not expressable with any of the available gcc memory constraints.
> 
>     'Q'
>           A memory reference where the exact address is in a single
>           register (''m'' is preferable for 'asm' statements)
> 
> If 'r' is good enough for io_base, 'Q' should be good enough for *io_base.

And if that doesn't work, it's possible to pass both the pointer, and
the dereference, as separate input operands, and only use the pointer
in the asm template.  GCC will still treat the dereference input as a
dependency.

-- Jamie

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Jamie Lokier]

Jamie Lokier wrote:
> Nicolas Pitre wrote:
> > On Thu, 25 Mar 2010, Jamie Lokier wrote:
> > > 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).
> > 
> > The LDRD has special range constraints on its addressing mode which is 
> > not expressable with any of the available gcc memory constraints.
> 
>     'Q'
>           A memory reference where the exact address is in a single
>           register (''m'' is preferable for 'asm' statements)
> 
> If 'r' is good enough for io_base, 'Q' should be good enough for *io_base.

And if that doesn't work, it's possible to pass both the pointer, and
the dereference, as separate input operands, and only use the pointer
in the asm template.  GCC will still treat the dereference input as a
dependency.

-- Jamie

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Sat, 24 Apr 2010, Jamie Lokier wrote:

> Nicolas Pitre wrote:
> > On Thu, 25 Mar 2010, Jamie Lokier wrote:
> > > >  		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.
> > 
> > I don't see how that could happen.  The store into buf[] puts a 
> > dependency on the output constraint of the inline asm statement.  And by 
> > vertue of being volatile, gcc cannot cache the result of the output from 
> > the asm as if it was a pure function.
> 
> The store into buf[] dependency doesn't stop this, after unrolling:
> 
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
>     buf64[i++] = x;
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
>     buf64[i++] = x;
> 
> from being reordered as this
> 
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x2) : "r" (io_base));
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x1) : "r" (io_base));
>     buf64[i++] = x1;
>     buf64[i++] = x2;
> 
> because the asm doesn't depend on memory, just register inputs and
> outputs;

I disagree.  The volatile tells gcc that the asm has side effects, and 
therefore 1) they can't be optimized away, and 2) can't be swapped with 
regards to each other like you do in your example.

>     'Q'
>           A memory reference where the exact address is in a single
>           register (''m'' is preferable for 'asm' statements)

Hmmm... Is this something new?  I must have missed it before.


Nicolas

Re: [PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Sat, 24 Apr 2010, Jamie Lokier wrote:

> Nicolas Pitre wrote:
> > On Thu, 25 Mar 2010, Jamie Lokier wrote:
> > > >  		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.
> > 
> > I don't see how that could happen.  The store into buf[] puts a 
> > dependency on the output constraint of the inline asm statement.  And by 
> > vertue of being volatile, gcc cannot cache the result of the output from 
> > the asm as if it was a pure function.
> 
> The store into buf[] dependency doesn't stop this, after unrolling:
> 
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
>     buf64[i++] = x;
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
>     buf64[i++] = x;
> 
> from being reordered as this
> 
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x2) : "r" (io_base));
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x1) : "r" (io_base));
>     buf64[i++] = x1;
>     buf64[i++] = x2;
> 
> because the asm doesn't depend on memory, just register inputs and
> outputs;

I disagree.  The volatile tells gcc that the asm has side effects, and 
therefore 1) they can't be optimized away, and 2) can't be swapped with 
regards to each other like you do in your example.

>     'Q'
>           A memory reference where the exact address is in a single
>           register (''m'' is preferable for 'asm' statements)

Hmmm... Is this something new?  I must have missed it before.


Nicolas

[PATCH v2] MTD: Fix Orion NAND driver compilation with ARM OABI

[Nicolas Pitre]

On Sat, 24 Apr 2010, Jamie Lokier wrote:

> Nicolas Pitre wrote:
> > On Thu, 25 Mar 2010, Jamie Lokier wrote:
> > > >  		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.
> > 
> > I don't see how that could happen.  The store into buf[] puts a 
> > dependency on the output constraint of the inline asm statement.  And by 
> > vertue of being volatile, gcc cannot cache the result of the output from 
> > the asm as if it was a pure function.
> 
> The store into buf[] dependency doesn't stop this, after unrolling:
> 
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
>     buf64[i++] = x;
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x) : "r" (io_base));
>     buf64[i++] = x;
> 
> from being reordered as this
> 
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x2) : "r" (io_base));
>     asm volatile ("ldrd\t%0, [%1]" : "=&r" (x1) : "r" (io_base));
>     buf64[i++] = x1;
>     buf64[i++] = x2;
> 
> because the asm doesn't depend on memory, just register inputs and
> outputs;

I disagree.  The volatile tells gcc that the asm has side effects, and 
therefore 1) they can't be optimized away, and 2) can't be swapped with 
regards to each other like you do in your example.

>     'Q'
>           A memory reference where the exact address is in a single
>           register (''m'' is preferable for 'asm' statements)

Hmmm... Is this something new?  I must have missed it before.


Nicolas