* [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code
2013-10-08 22:34 New big-endian patch series against 3.12-rc4 Ben Dooks
@ 2013-10-08 22:34 ` Ben Dooks
2013-10-09 14:35 ` Dave Martin
2013-10-14 20:53 ` Ben Dooks
0 siblings, 2 replies; 7+ messages in thread
From: Ben Dooks @ 2013-10-08 22:34 UTC (permalink / raw)
To: linux-arm-kernel
From: Victor Kamensky <victor.kamensky@linaro.org>
cci_enable_port_for_self written in asm and it works with h/w
registers that are in little endian format. When run in big
endian mode it needs byte swap before/after it writes/reads
to/from such registers
CC: Lorenzon Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
drivers/bus/arm-cci.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 2009266..6db173e 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
/* Enable the CCI port */
" ldr r0, [r0, %[offsetof_port_phys]] \n"
" mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
+#ifdef __ARMEB__
+" rev r3, r3 \n"
+#endif /* __ARMEB__ */
" str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
/* poll the status reg for completion */
@@ -288,6 +291,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
" ldr r0, [r1] \n"
" ldr r0, [r0, r1] @ cci_ctrl_base \n"
"4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
+#ifdef __ARMEB__
+" rev r1, r1 \n"
+#endif /* __ARMEB__ */
" tst r1, #1 \n"
" bne 4b \n"
--
1.8.4.rc3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code
2013-10-08 22:34 ` [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code Ben Dooks
@ 2013-10-09 14:35 ` Dave Martin
2013-10-14 20:53 ` Ben Dooks
1 sibling, 0 replies; 7+ messages in thread
From: Dave Martin @ 2013-10-09 14:35 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 09, 2013 at 12:34:38AM +0200, Ben Dooks wrote:
> From: Victor Kamensky <victor.kamensky@linaro.org>
>
> cci_enable_port_for_self written in asm and it works with h/w
> registers that are in little endian format. When run in big
> endian mode it needs byte swap before/after it writes/reads
> to/from such registers
I had a play with an alternate way of doing this that may be a bit
cleaner, posted as a separate mini-series. See:
ARM: Unify const-swabbing and conditional asm
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/203479.html
Cheers
---Dave
>
> CC: Lorenzon Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> drivers/bus/arm-cci.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 2009266..6db173e 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
> /* Enable the CCI port */
> " ldr r0, [r0, %[offsetof_port_phys]] \n"
> " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
> +#ifdef __ARMEB__
> +" rev r3, r3 \n"
> +#endif /* __ARMEB__ */
> " str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
>
> /* poll the status reg for completion */
> @@ -288,6 +291,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
> " ldr r0, [r1] \n"
> " ldr r0, [r0, r1] @ cci_ctrl_base \n"
> "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
> +#ifdef __ARMEB__
> +" rev r1, r1 \n"
> +#endif /* __ARMEB__ */
> " tst r1, #1 \n"
> " bne 4b \n"
>
> --
> 1.8.4.rc3
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code
2013-10-08 22:34 ` [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code Ben Dooks
2013-10-09 14:35 ` Dave Martin
@ 2013-10-14 20:53 ` Ben Dooks
2013-10-15 1:40 ` Nicolas Pitre
1 sibling, 1 reply; 7+ messages in thread
From: Ben Dooks @ 2013-10-14 20:53 UTC (permalink / raw)
To: linux-arm-kernel
On 08/10/13 23:34, Ben Dooks wrote:
> From: Victor Kamensky<victor.kamensky@linaro.org>
>
> cci_enable_port_for_self written in asm and it works with h/w
> registers that are in little endian format. When run in big
> endian mode it needs byte swap before/after it writes/reads
> to/from such registers
>
> CC: Lorenzon Pieralisi<lorenzo.pieralisi@arm.com>
> Signed-off-by: Victor Kamensky<victor.kamensky@linaro.org>
> Signed-off-by: Ben Dooks<ben.dooks@codethink.co.uk>
> ---
> drivers/bus/arm-cci.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 2009266..6db173e 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
> /* Enable the CCI port */
> " ldr r0, [r0, %[offsetof_port_phys]] \n"
> " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
> +#ifdef __ARMEB__
> +" rev r3, r3 \n"
> +#endif /* __ARMEB__ */
> " str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
>
> /* poll the status reg for completion */
> @@ -288,6 +291,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
> " ldr r0, [r1] \n"
> " ldr r0, [r0, r1] @ cci_ctrl_base \n"
> "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
> +#ifdef __ARMEB__
> +" rev r1, r1 \n"
> +#endif /* __ARMEB__ */
> " tst r1, #1 \n"
> " bne 4b \n"
I was just thinking if this would be a better way to change
the code:
diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 6db173e..2ad2511 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -280,10 +280,7 @@ asmlinkage void __naked cci_enable_port_for_self(void)
/* Enable the CCI port */
" ldr r0, [r0, %[offsetof_port_phys]] \n"
-" mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
-#ifdef __ARMEB__
-" rev r3, r3 \n"
-#endif /* __ARMEB__ */
+" mov r3, %[cci_enable_req]\n"
" str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
/* poll the status reg for completion */
@@ -291,10 +288,7 @@ asmlinkage void __naked cci_enable_port_for_self(void)
" ldr r0, [r1] \n"
" ldr r0, [r0, r1] @ cci_ctrl_base \n"
"4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
-#ifdef __ARMEB__
-" rev r1, r1 \n"
-#endif /* __ARMEB__ */
-" tst r1, #1 \n"
+" tst r1, %[cci_control_status_bits] \n"
" bne 4b \n"
" mov r0, #0 \n"
@@ -307,6 +301,8 @@ asmlinkage void __naked cci_enable_port_for_self(void)
"7: .word cci_ctrl_phys - . \n"
: :
[sizeof_cpu_port] "i" (sizeof(cpu_port)),
+ [cci_enable_req] "i" cpu_to_le32(CCI_ENABLE_REQ),
+ [cci_control_status_bits] "i" cpu_to_le32(1),
so swap the data bits before passing them into the code
which could also remove some of the use of the __stringify()
calls in there too.
It seems to compile ok, but is not complete.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code
2013-10-14 20:53 ` Ben Dooks
@ 2013-10-15 1:40 ` Nicolas Pitre
2013-10-15 9:58 ` Ben Dooks
0 siblings, 1 reply; 7+ messages in thread
From: Nicolas Pitre @ 2013-10-15 1:40 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 14 Oct 2013, Ben Dooks wrote:
> On 08/10/13 23:34, Ben Dooks wrote:
> > From: Victor Kamensky<victor.kamensky@linaro.org>
> >
> > cci_enable_port_for_self written in asm and it works with h/w
> > registers that are in little endian format. When run in big
> > endian mode it needs byte swap before/after it writes/reads
> > to/from such registers
> >
> > CC: Lorenzon Pieralisi<lorenzo.pieralisi@arm.com>
> > Signed-off-by: Victor Kamensky<victor.kamensky@linaro.org>
> > Signed-off-by: Ben Dooks<ben.dooks@codethink.co.uk>
> > ---
> > drivers/bus/arm-cci.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> > index 2009266..6db173e 100644
> > --- a/drivers/bus/arm-cci.c
> > +++ b/drivers/bus/arm-cci.c
> > @@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
> > /* Enable the CCI port */
> > " ldr r0, [r0, %[offsetof_port_phys]] \n"
> > " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
> > +#ifdef __ARMEB__
> > +" rev r3, r3 \n"
> > +#endif /* __ARMEB__ */
> > " str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
> >
> > /* poll the status reg for completion */
> > @@ -288,6 +291,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
> > " ldr r0, [r1] \n"
> > " ldr r0, [r0, r1] @ cci_ctrl_base \n"
> > "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
> > +#ifdef __ARMEB__
> > +" rev r1, r1 \n"
> > +#endif /* __ARMEB__ */
> > " tst r1, #1 \n"
> > " bne 4b \n"
>
> I was just thinking if this would be a better way to change
> the code:
>
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 6db173e..2ad2511 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -280,10 +280,7 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>
> /* Enable the CCI port */
> " ldr r0, [r0, %[offsetof_port_phys]] \n"
> -" mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
> -#ifdef __ARMEB__
> -" rev r3, r3 \n"
> -#endif /* __ARMEB__ */
> +" mov r3, %[cci_enable_req]\n"
> " str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
>
> /* poll the status reg for completion */
> @@ -291,10 +288,7 @@ asmlinkage void __naked cci_enable_port_for_self(void)
> " ldr r0, [r1] \n"
> " ldr r0, [r0, r1] @ cci_ctrl_base \n"
> "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
> -#ifdef __ARMEB__
> -" rev r1, r1 \n"
> -#endif /* __ARMEB__ */
> -" tst r1, #1 \n"
> +" tst r1, %[cci_control_status_bits] \n"
> " bne 4b \n"
>
> " mov r0, #0 \n"
> @@ -307,6 +301,8 @@ asmlinkage void __naked cci_enable_port_for_self(void)
> "7: .word cci_ctrl_phys - . \n"
> : :
> [sizeof_cpu_port] "i" (sizeof(cpu_port)),
> + [cci_enable_req] "i" cpu_to_le32(CCI_ENABLE_REQ),
> + [cci_control_status_bits] "i" cpu_to_le32(1),
>
> so swap the data bits before passing them into the code
> which could also remove some of the use of the __stringify()
> calls in there too.
That certainly looks nicer to me.
> It seems to compile ok, but is not complete.
What do you mean?
Nicolas
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code
2013-10-15 1:40 ` Nicolas Pitre
@ 2013-10-15 9:58 ` Ben Dooks
0 siblings, 0 replies; 7+ messages in thread
From: Ben Dooks @ 2013-10-15 9:58 UTC (permalink / raw)
To: linux-arm-kernel
On 15/10/13 02:40, Nicolas Pitre wrote:
> On Mon, 14 Oct 2013, Ben Dooks wrote:
>
>> On 08/10/13 23:34, Ben Dooks wrote:
>>> From: Victor Kamensky<victor.kamensky@linaro.org>
>>>
>>> cci_enable_port_for_self written in asm and it works with h/w
>>> registers that are in little endian format. When run in big
>>> endian mode it needs byte swap before/after it writes/reads
>>> to/from such registers
>>>
>>> CC: Lorenzon Pieralisi<lorenzo.pieralisi@arm.com>
>>> Signed-off-by: Victor Kamensky<victor.kamensky@linaro.org>
>>> Signed-off-by: Ben Dooks<ben.dooks@codethink.co.uk>
>>> ---
>>> drivers/bus/arm-cci.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
>>> index 2009266..6db173e 100644
>>> --- a/drivers/bus/arm-cci.c
>>> +++ b/drivers/bus/arm-cci.c
>>> @@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>>> /* Enable the CCI port */
>>> " ldr r0, [r0, %[offsetof_port_phys]] \n"
>>> " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
>>> +#ifdef __ARMEB__
>>> +" rev r3, r3 \n"
>>> +#endif /* __ARMEB__ */
>>> " str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
>>>
>>> /* poll the status reg for completion */
>>> @@ -288,6 +291,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>>> " ldr r0, [r1] \n"
>>> " ldr r0, [r0, r1] @ cci_ctrl_base \n"
>>> "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
>>> +#ifdef __ARMEB__
>>> +" rev r1, r1 \n"
>>> +#endif /* __ARMEB__ */
>>> " tst r1, #1 \n"
>>> " bne 4b \n"
>>
>> I was just thinking if this would be a better way to change
>> the code:
>>
>> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
>> index 6db173e..2ad2511 100644
>> --- a/drivers/bus/arm-cci.c
>> +++ b/drivers/bus/arm-cci.c
>> @@ -280,10 +280,7 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>>
>> /* Enable the CCI port */
>> " ldr r0, [r0, %[offsetof_port_phys]] \n"
>> -" mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
>> -#ifdef __ARMEB__
>> -" rev r3, r3 \n"
>> -#endif /* __ARMEB__ */
>> +" mov r3, %[cci_enable_req]\n"
>> " str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
>>
>> /* poll the status reg for completion */
>> @@ -291,10 +288,7 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>> " ldr r0, [r1] \n"
>> " ldr r0, [r0, r1] @ cci_ctrl_base \n"
>> "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
>> -#ifdef __ARMEB__
>> -" rev r1, r1 \n"
>> -#endif /* __ARMEB__ */
>> -" tst r1, #1 \n"
>> +" tst r1, %[cci_control_status_bits] \n"
>> " bne 4b \n"
>>
>> " mov r0, #0 \n"
>> @@ -307,6 +301,8 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>> "7: .word cci_ctrl_phys - . \n"
>> : :
>> [sizeof_cpu_port] "i" (sizeof(cpu_port)),
>> + [cci_enable_req] "i" cpu_to_le32(CCI_ENABLE_REQ),
>> + [cci_control_status_bits] "i" cpu_to_le32(1),
>>
>> so swap the data bits before passing them into the code
>> which could also remove some of the use of the __stringify()
>> calls in there too.
> That certainly looks nicer to me.
>
>> It seems to compile ok, but is not complete.
>
> What do you mean?
I didn't have time to check I caught all cases in this.
Victor, any chance you could have a look at this and re-do the patch?
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code
[not found] <20131015092637.GA2312@localhost.localdomain>
@ 2013-10-15 10:41 ` Ben Dooks
2013-10-15 14:02 ` Victor Kamensky
0 siblings, 1 reply; 7+ messages in thread
From: Ben Dooks @ 2013-10-15 10:41 UTC (permalink / raw)
To: linux-arm-kernel
On 15/10/13 10:27, Dave Martin wrote:
> On Mon, Oct 14, 2013 at 09:40:54PM -0400, Nicolas Pitre wrote:
>> On Mon, 14 Oct 2013, Ben Dooks wrote:
>>
>>> On 08/10/13 23:34, Ben Dooks wrote:
>>>> From: Victor Kamensky<victor.kamensky@linaro.org>
>>>>
>>>> cci_enable_port_for_self written in asm and it works with h/w
>>>> registers that are in little endian format. When run in big
>>>> endian mode it needs byte swap before/after it writes/reads
>>>> to/from such registers
>>>>
>>>> CC: Lorenzon Pieralisi<lorenzo.pieralisi@arm.com>
>>>> Signed-off-by: Victor Kamensky<victor.kamensky@linaro.org>
>>>> Signed-off-by: Ben Dooks<ben.dooks@codethink.co.uk>
>>>> ---
>>>> drivers/bus/arm-cci.c | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
>>>> index 2009266..6db173e 100644
>>>> --- a/drivers/bus/arm-cci.c
>>>> +++ b/drivers/bus/arm-cci.c
>>>> @@ -281,6 +281,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>>>> /* Enable the CCI port */
>>>> " ldr r0, [r0, %[offsetof_port_phys]] \n"
>>>> " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
>>>> +#ifdef __ARMEB__
>>>> +" rev r3, r3 \n"
>>>> +#endif /* __ARMEB__ */
>>>> " str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
>>>>
>>>> /* poll the status reg for completion */
>>>> @@ -288,6 +291,9 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>>>> " ldr r0, [r1] \n"
>>>> " ldr r0, [r0, r1] @ cci_ctrl_base \n"
>>>> "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
>>>> +#ifdef __ARMEB__
>>>> +" rev r1, r1 \n"
>>>> +#endif /* __ARMEB__ */
>>>> " tst r1, #1 \n"
>>>> " bne 4b \n"
>>>
>>> I was just thinking if this would be a better way to change
>>> the code:
>>>
>>> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
>>> index 6db173e..2ad2511 100644
>>> --- a/drivers/bus/arm-cci.c
>>> +++ b/drivers/bus/arm-cci.c
>>> @@ -280,10 +280,7 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>>>
>>> /* Enable the CCI port */
>>> " ldr r0, [r0, %[offsetof_port_phys]] \n"
>>> -" mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
>>> -#ifdef __ARMEB__
>>> -" rev r3, r3 \n"
>>> -#endif /* __ARMEB__ */
>>> +" mov r3, %[cci_enable_req]\n"
>>> " str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
>>>
>>> /* poll the status reg for completion */
>>> @@ -291,10 +288,7 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>>> " ldr r0, [r1] \n"
>>> " ldr r0, [r0, r1] @ cci_ctrl_base \n"
>>> "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
>>> -#ifdef __ARMEB__
>>> -" rev r1, r1 \n"
>>> -#endif /* __ARMEB__ */
>>> -" tst r1, #1 \n"
>>> +" tst r1, %[cci_control_status_bits] \n"
>>> " bne 4b \n"
>>>
>>> " mov r0, #0 \n"
>>> @@ -307,6 +301,8 @@ asmlinkage void __naked cci_enable_port_for_self(void)
>>> "7: .word cci_ctrl_phys - . \n"
>>> : :
>>> [sizeof_cpu_port] "i" (sizeof(cpu_port)),
>>> + [cci_enable_req] "i" cpu_to_le32(CCI_ENABLE_REQ),
>>> + [cci_control_status_bits] "i" cpu_to_le32(1),
>>>
>>> so swap the data bits before passing them into the code
>>> which could also remove some of the use of the __stringify()
>>> calls in there too.
>> That certainly looks nicer to me.
>>
>>> It seems to compile ok, but is not complete.
>>
>> What do you mean?
>
> Ditto -- this does look better that the other suggestions. I forget that
> there are something situations where the "i" constraint is actually
> useful.
Well, it was a quick idea late last night, whilst not-sleeping from
a nasty cold that hasn't quite gone away. In that state I'm not going
to guarantee anything.
If Victor could re-work and test, then that'd be great and I can swap
out the last patch of the BE series.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code
2013-10-15 10:41 ` [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code Ben Dooks
@ 2013-10-15 14:02 ` Victor Kamensky
0 siblings, 0 replies; 7+ messages in thread
From: Victor Kamensky @ 2013-10-15 14:02 UTC (permalink / raw)
To: linux-arm-kernel
On 15 October 2013 03:41, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 15/10/13 10:27, Dave Martin wrote:
>>
>> On Mon, Oct 14, 2013 at 09:40:54PM -0400, Nicolas Pitre wrote:
>>>
>>> On Mon, 14 Oct 2013, Ben Dooks wrote:
>>>
>>>> On 08/10/13 23:34, Ben Dooks wrote:
>>>>>
>>>>> From: Victor Kamensky<victor.kamensky@linaro.org>
>>>>>
>>>>> cci_enable_port_for_self written in asm and it works with h/w
>>>>> registers that are in little endian format. When run in big
>>>>> endian mode it needs byte swap before/after it writes/reads
>>>>> to/from such registers
>>>>>
>>>>> CC: Lorenzon Pieralisi<lorenzo.pieralisi@arm.com>
>>>>> Signed-off-by: Victor Kamensky<victor.kamensky@linaro.org>
>>>>> Signed-off-by: Ben Dooks<ben.dooks@codethink.co.uk>
>>>>> ---
>>>>> drivers/bus/arm-cci.c | 6 ++++++
>>>>> 1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
>>>>> index 2009266..6db173e 100644
>>>>> --- a/drivers/bus/arm-cci.c
>>>>> +++ b/drivers/bus/arm-cci.c
>>>>> @@ -281,6 +281,9 @@ asmlinkage void __naked
>>>>> cci_enable_port_for_self(void)
>>>>> /* Enable the CCI port */
>>>>> " ldr r0, [r0, %[offsetof_port_phys]] \n"
>>>>> " mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
>>>>> +#ifdef __ARMEB__
>>>>> +" rev r3, r3 \n"
>>>>> +#endif /* __ARMEB__ */
>>>>> " str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
>>>>>
>>>>> /* poll the status reg for completion */
>>>>> @@ -288,6 +291,9 @@ asmlinkage void __naked
>>>>> cci_enable_port_for_self(void)
>>>>> " ldr r0, [r1] \n"
>>>>> " ldr r0, [r0, r1] @ cci_ctrl_base \n"
>>>>> "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
>>>>> +#ifdef __ARMEB__
>>>>> +" rev r1, r1 \n"
>>>>> +#endif /* __ARMEB__ */
>>>>> " tst r1, #1 \n"
>>>>> " bne 4b \n"
>>>>
>>>>
>>>> I was just thinking if this would be a better way to change
>>>> the code:
>>>>
>>>> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
>>>> index 6db173e..2ad2511 100644
>>>> --- a/drivers/bus/arm-cci.c
>>>> +++ b/drivers/bus/arm-cci.c
>>>> @@ -280,10 +280,7 @@ asmlinkage void __naked
>>>> cci_enable_port_for_self(void)
>>>>
>>>> /* Enable the CCI port */
>>>> " ldr r0, [r0, %[offsetof_port_phys]] \n"
>>>> -" mov r3, #"__stringify(CCI_ENABLE_REQ)" \n"
>>>> -#ifdef __ARMEB__
>>>> -" rev r3, r3 \n"
>>>> -#endif /* __ARMEB__ */
>>>> +" mov r3, %[cci_enable_req]\n"
>>>> " str r3, [r0, #"__stringify(CCI_PORT_CTRL)"] \n"
>>>>
>>>> /* poll the status reg for completion */
>>>> @@ -291,10 +288,7 @@ asmlinkage void __naked
>>>> cci_enable_port_for_self(void)
>>>> " ldr r0, [r1] \n"
>>>> " ldr r0, [r0, r1] @ cci_ctrl_base \n"
>>>> "4: ldr r1, [r0, #"__stringify(CCI_CTRL_STATUS)"] \n"
>>>> -#ifdef __ARMEB__
>>>> -" rev r1, r1 \n"
>>>> -#endif /* __ARMEB__ */
>>>> -" tst r1, #1 \n"
>>>> +" tst r1, %[cci_control_status_bits] \n"
>>>> " bne 4b \n"
>>>>
>>>> " mov r0, #0 \n"
>>>> @@ -307,6 +301,8 @@ asmlinkage void __naked
>>>> cci_enable_port_for_self(void)
>>>> "7: .word cci_ctrl_phys - . \n"
>>>> : :
>>>> [sizeof_cpu_port] "i" (sizeof(cpu_port)),
>>>> + [cci_enable_req] "i" cpu_to_le32(CCI_ENABLE_REQ),
>>>> + [cci_control_status_bits] "i" cpu_to_le32(1),
>>>>
>>>> so swap the data bits before passing them into the code
>>>> which could also remove some of the use of the __stringify()
>>>> calls in there too.
>>>
>>> That certainly looks nicer to me.
>>>
>>>> It seems to compile ok, but is not complete.
>>>
>>>
>>> What do you mean?
>>
>>
>> Ditto -- this does look better that the other suggestions. I forget that
>> there are something situations where the "i" constraint is actually
>> useful.
>
>
> Well, it was a quick idea late last night, whilst not-sleeping from
> a nasty cold that hasn't quite gone away. In that state I'm not going
> to guarantee anything.
>
> If Victor could re-work and test, then that'd be great and I can swap
> out the last patch of the BE series.
I've tested it yesterday. It works fine. Agreed, much nicer than others.
Will code and post replacement patch today.
Thanks,
Victor
>
> --
> Ben Dooks http://www.codethink.co.uk/
> Senior Engineer Codethink - Providing Genius
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-10-15 14:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20131015092637.GA2312@localhost.localdomain>
2013-10-15 10:41 ` [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code Ben Dooks
2013-10-15 14:02 ` Victor Kamensky
2013-10-08 22:34 New big-endian patch series against 3.12-rc4 Ben Dooks
2013-10-08 22:34 ` [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code Ben Dooks
2013-10-09 14:35 ` Dave Martin
2013-10-14 20:53 ` Ben Dooks
2013-10-15 1:40 ` Nicolas Pitre
2013-10-15 9:58 ` Ben Dooks
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).