[PATCH 22/23] ARM: cci: driver need big endian fixes in asm code

[Victor Kamensky]

On 15 October 2013 03:41, Ben Dooks <ben.dooks at 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 at 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 at arm.com>
>>>>> Signed-off-by: Victor Kamensky<victor.kamensky at linaro.org>
>>>>> Signed-off-by: Ben Dooks<ben.dooks at 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