* [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
* New big-endian patch series against 3.12-rc4 @ 2013-10-08 22:34 Ben Dooks 2013-10-08 22:34 ` [PATCH 22/23] ARM: cci: driver need big endian fixes in asm code Ben Dooks 0 siblings, 1 reply; 7+ messages in thread From: Ben Dooks @ 2013-10-08 22:34 UTC (permalink / raw) To: linux-arm-kernel This is a new series for big-endian on ARM re-based onto 3.12-rc4 and updated with new patches from Victor to fix regressions since 3.11. I have not been able to test this further than building it as I do not currently have access to any hardware, however there where no big issues when re-basing. Thanks to Marc Zyngier for pointing out the need for a rebase. The branch is at: git://git.baserock.org/delta/linux baserock/bjdooks/3.12-rc4/core Russell, if possible could this go into -next and then if everything I can issue a pull request for it. Changes since last: - Updated to 3.12-rc4 to be closer to mainline - Added Victor's patches for CCI and TC2 boot - Added Victor's patch for ASID extraction - Updated Victor's patch with fix for building on older kernels ^ 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 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
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).