ARM: OMAP4: sleep: byteswap data for big-endian

From: Victor Kamensky <[email protected]>

Assembler functions defined in sleep44xx.S need to byteswap values after read / before write from h/w register if code compiled in big endian mode. Simple change to do 'rev x, x' before str instruction and after ldr instruction that deals with h/w registers.

Signed-off-by Victor Kamensky <[email protected]>

Signed-off-by Taras Kondratiuk <[email protected]>

---
This is a part of RFC series [1].
Based on v3.13-rc8.

[1] http://www.spinics.net/lists/linux-omap/msg99927.html

 arch/arm/mach-omap2/sleep44xx.S |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

arch/arm/mach-omap2/sleep44xx.S +17 -0

index 9086ce0..8017016 100644
@@ -12,6 +12,7 @@
 #include <linux/linkage.h>
 #include <asm/smp_scu.h>
 #include <asm/memory.h>
+#include <asm/assembler.h>
 #include <asm/hardware/cache-l2x0.h>
 
 #include "omap-secure.h"

@@ -74,6 +75,7 @@ ENTRY(omap4_finish_suspend)
 	 */
 	bl	omap4_get_sar_ram_base
 	ldr	r9, [r0, #OMAP_TYPE_OFFSET]
+ARM_BE8(rev	r9, r9)
 	cmp	r9, #0x1			@ Check for HS device
 	bne	skip_secure_l1_clean
 	mov	r0, #SCU_PM_NORMAL

@@ -113,12 +115,14 @@ skip_secure_l1_clean:
 	bl	omap4_get_sar_ram_base
 	mov	r8, r0
 	ldr	r9, [r8, #OMAP_TYPE_OFFSET]
+ARM_BE8(rev	r9, r9)
 	cmp	r9, #0x1			@ Check for HS device
 	bne	scu_gp_set
 	mrc	p15, 0, r0, c0, c0, 5		@ Read MPIDR
 	ands	r0, r0, #0x0f
 	ldreq	r0, [r8, #SCU_OFFSET0]
 	ldrne	r0, [r8, #SCU_OFFSET1]
+ARM_BE8(rev	r0, r0)
 	mov	r1, #0x00
 	stmfd   r13!, {r4-r12, r14}
 	ldr	r12, =OMAP4_MON_SCU_PWR_INDEX

@@ -130,6 +134,7 @@ scu_gp_set:
 	ands	r0, r0, #0x0f
 	ldreq	r1, [r8, #SCU_OFFSET0]
 	ldrne	r1, [r8, #SCU_OFFSET1]
+ARM_BE8(rev	r1, r1)
 	bl	omap4_get_scu_base
 	bl	scu_power_mode
 skip_scu_gp_set:

@@ -157,6 +162,7 @@ skip_scu_gp_set:
 	ands	r5, r5, #0x0f
 	ldreq	r0, [r8, #L2X0_SAVE_OFFSET0]	@ Retrieve L2 state from SAR
 	ldrne	r0, [r8, #L2X0_SAVE_OFFSET1]	@ memory.
+ARM_BE8(rev	r0, r0)
 	cmp	r0, #3
 	bne	do_WFI
 #ifdef CONFIG_PL310_ERRATA_727915

@@ -167,9 +173,11 @@ skip_scu_gp_set:
 	bl	omap4_get_l2cache_base
 	mov	r2, r0
 	ldr	r0, =0xffff
+ARM_BE8(rev	r0, r0)
 	str	r0, [r2, #L2X0_CLEAN_INV_WAY]
 wait:
 	ldr	r0, [r2, #L2X0_CLEAN_INV_WAY]
+ARM_BE8(rev	r0, r0)
 	ldr	r1, =0xffff
 	ands	r0, r0, r1
 	bne	wait

@@ -182,9 +190,11 @@ l2x_sync:
 	bl	omap4_get_l2cache_base
 	mov	r2, r0
 	mov	r0, #0x0
+ARM_BE8(rev	r0, r0)
 	str	r0, [r2, #L2X0_CACHE_SYNC]
 sync:
 	ldr	r0, [r2, #L2X0_CACHE_SYNC]
+ARM_BE8(rev	r0, r0)
 	ands	r0, r0, #0x1
 	bne	sync
 #endif

@@ -216,6 +226,7 @@ do_WFI:
 	bl	omap4_get_sar_ram_base
 	mov	r8, r0
 	ldr	r9, [r8, #OMAP_TYPE_OFFSET]
+ARM_BE8(rev	r9, r9)
 	cmp	r9, #0x1			@ Check for HS device
 	bne	scu_gp_clear
 	mov	r0, #SCU_PM_NORMAL

@@ -258,6 +269,7 @@ ENTRY(omap4_cpu_resume)
 	 */
 	ldr	r8, =OMAP44XX_SAR_RAM_BASE
 	ldr	r9, [r8, #OMAP_TYPE_OFFSET]
+ARM_BE8(rev	r9, r9)
 	cmp	r9, #0x1			@ Skip if GP device
 	bne	skip_ns_smp_enable
 	mrc     p15, 0, r0, c0, c0, 5

@@ -292,16 +304,19 @@ skip_ns_smp_enable:
 	 */
 	ldr	r2, =OMAP44XX_L2CACHE_BASE
 	ldr	r0, [r2, #L2X0_CTRL]
+ARM_BE8(rev	r0, r0)
 	and	r0, #0x0f
 	cmp	r0, #1
 	beq	skip_l2en			@ Skip if already enabled
 	ldr	r3, =OMAP44XX_SAR_RAM_BASE
 	ldr	r1, [r3, #OMAP_TYPE_OFFSET]
+ARM_BE8(rev	r1, r1)
 	cmp	r1, #0x1			@ Check for HS device
 	bne     set_gp_por
 	ldr     r0, =OMAP4_PPA_L2_POR_INDEX
 	ldr     r1, =OMAP44XX_SAR_RAM_BASE
 	ldr     r4, [r1, #L2X0_PREFETCH_CTRL_OFFSET]
+ARM_BE8(rev	r4, r4)
 	adr     r3, ppa_por_params
 	str     r4, [r3, #0x04]
 	mov	r1, #0x0			@ Process ID

@@ -313,11 +328,13 @@ skip_ns_smp_enable:
 set_gp_por:
 	ldr     r1, =OMAP44XX_SAR_RAM_BASE
 	ldr     r0, [r1, #L2X0_PREFETCH_CTRL_OFFSET]
+ARM_BE8(rev	r0, r0)
 	ldr	r12, =OMAP4_MON_L2X0_PREFETCH_INDEX	@ Setup L2 PREFETCH
 	DO_SMC
 set_aux_ctrl:
 	ldr     r1, =OMAP44XX_SAR_RAM_BASE
 	ldr	r0, [r1, #L2X0_AUXCTRL_OFFSET]
+ARM_BE8(rev	r0, r0)
 	ldr	r12, =OMAP4_MON_L2X0_AUXCTRL_INDEX	@ Setup L2 AUXCTRL
 	DO_SMC
 	mov	r0, #0x1

Quoted 15 lines · Taras Kondratiuk

From: Victor Kamensky <[email protected]>

Assembler functions defined in sleep44xx.S need to byteswap values after read / before write from h/w register if code compiled in big endian mode. Simple change to do 'rev x, x' before str instruction and after ldr instruction that deals with h/w registers.

Signed-off-by Victor Kamensky <[email protected]>

Signed-off-by Taras Kondratiuk <[email protected]>

---
This is a part of RFC series [1].
Based on v3.13-rc8.

[1] http://www.spinics.net/lists/linux-omap/msg99927.html

Quoted 3 lines

 arch/arm/mach-omap2/sleep44xx.S |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

OMAP4 is LE, and if there is a gcc flag for the same, is'nt it cleaner to deal with it in Makefile rather than trying to make an assembly meant only for LE by force building it for BE?

Quoted 20 lines · Nishanth Menon

Quoted 18 lines · Taras Kondratiuk

From: Victor Kamensky <[email protected]>

Assembler functions defined in sleep44xx.S need to byteswap values after read / before write from h/w register if code compiled in big endian mode. Simple change to do 'rev x, x' before str instruction and after ldr instruction that deals with h/w registers.

Signed-off-by Victor Kamensky <[email protected]>

Signed-off-by Taras Kondratiuk <[email protected]>

---
This is a part of RFC series [1].
Based on v3.13-rc8.

[1] http://www.spinics.net/lists/linux-omap/msg99927.html

 arch/arm/mach-omap2/sleep44xx.S |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Quoted 3 lines

OMAP4 is LE, and if there is a gcc flag for the same, is'nt it cleaner to deal with it in Makefile rather than trying to make an assembly meant only for LE by force building it for BE?

Hi Nishanth
I'm not sure I got your point.
Do you propose to build this file as LE while the rest of kernel is BE?

Quoted 21 lines · Taras Kondratiuk

Quoted 20 lines · Nishanth Menon

Quoted 18 lines · Taras Kondratiuk

From: Victor Kamensky <[email protected]>

Assembler functions defined in sleep44xx.S need to byteswap values after read / before write from h/w register if code compiled in big endian mode. Simple change to do 'rev x, x' before str instruction and after ldr instruction that deals with h/w registers.

Signed-off-by Victor Kamensky <[email protected]>

Signed-off-by Taras Kondratiuk <[email protected]>

---
This is a part of RFC series [1].
Based on v3.13-rc8.

[1] http://www.spinics.net/lists/linux-omap/msg99927.html

 arch/arm/mach-omap2/sleep44xx.S |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Quoted 8 lines

Quoted 3 lines

OMAP4 is LE, and if there is a gcc flag for the same, is'nt it cleaner to deal with it in Makefile rather than trying to make an assembly meant only for LE by force building it for BE?

Hi Nishanth
I'm not sure I got your point.
Do you propose to build this file as LE while the rest of kernel is BE?

I dont see why I should deal with the BE macro for every code change we have in omap4,am335x assembly. The hardware is LE and wont change just coz you are building it for BE. So I dont get the rationale for changing the assembly here - yes, if the assembly can be maintained as LE only mode and the build handling be adequately handled in Makefile (similar to SMC handling), that would be the best.

is the idea of BE build meant to deal with having a single BE kernel build work for all platforms (including LE ones)?

--
Regards,
Nishanth Menon

When BE kernel is built Makefile does take of compiling code in BE mode. I.e all proper flags like -mbig-endian and -Wl,--be8 will be set.

Quoted 3 lines

is the idea of BE build meant to deal with having a single BE kernel build work for all platforms (including LE ones)?

Sort of. The idea here to run BE image on OMAP4 chip, with kernel that would deals with LE periphery correctly, but ARM core run in BE with special kernel that compiled for BE case (i.e CONFIG_CPU_BIG_ENDIAN is set).

Quoted 3 lines · Victor Kamensky

When BE kernel is built Makefile does take of compiling code in BE mode. I.e all proper flags like -mbig-endian and -Wl,--be8 will be set.

Agreed, and I assume you cannot instead switch to LE mode when entering assembly assuming LE?

Quoted 3 lines · Victor Kamensky

When BE kernel is built Makefile does take of compiling code in BE mode. I.e all proper flags like -mbig-endian and -Wl,--be8 will be set.

Agreed, and I assume you cannot instead switch to LE mode when entering assembly assuming LE?

The reason I ask this is - most of our development is NOT in BE mode. we will continue to manipulate and add assembly - AM335x, DRA7/OMAP5 etc.. and obviously not every code change will indulge in ensuring right markers will be in place.

by ensuring readl_relaxed handles the variations, you have ensured that I dont need to care about drivers other than to ensure they use _relaxed variants. in the case of assembly, this does not seem long term manageable.

Quoted 8 lines

Quoted 3 lines

is the idea of BE build meant to deal with having a single BE kernel build work for all platforms (including LE ones)?

Sort of. The idea here to run BE image on OMAP4 chip, with kernel that would deals with LE periphery correctly, but ARM core run in BE with special kernel that compiled for BE case (i.e CONFIG_CPU_BIG_ENDIAN is set).

I still dont get the usecase - other than "hey, we do this coz we can do it!".. I mean, yep, it sounds great and all.. but 4 years down the line, is this still going to work? is this going to be interesting careabout? or we are just maintaining additional code for a passing fancy or proof-of-concept?

Regards,
Nishanth Menon

Quoted 8 lines · Nishanth Menon

Quoted 3 lines · Victor Kamensky

When BE kernel is built Makefile does take of compiling code in BE mode. I.e all proper flags like -mbig-endian and -Wl,--be8 will be set.

Agreed, and I assume you cannot instead switch to LE mode when entering assembly assuming LE?

Yes. Note that this asm interacts with other data in kernel that would be in BE form. And after all linker will not allow to put together files that were compiled in different endianity.

Quoted 9 lines

The reason I ask this is - most of our development is NOT in BE mode. we will continue to manipulate and add assembly - AM335x, DRA7/OMAP5 etc.. and obviously not every code change will indulge in ensuring right markers will be in place.

by ensuring readl_relaxed handles the variations, you have ensured that I dont need to care about drivers other than to ensure they use _relaxed variants. in the case of assembly, this does not seem long term manageable.

Yes, I agree if it is outside of main use case it will get broken if not attended to. Definitely universe entropy increases :) - if left without attention things will decay. Note we admit that even with ARM CPU core BE changes similar decay can happen eventually ...that is what LNG BE group trying to prevent. I think
the deal here is that next user of it can/need to fix things if they decayed.

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

is the idea of BE build meant to deal with having a single BE kernel build work for all platforms (including LE ones)?

Sort of. The idea here to run BE image on OMAP4 chip, with kernel that would deals with LE periphery correctly, but ARM core run in BE with special kernel that compiled for BE case (i.e CONFIG_CPU_BIG_ENDIAN is set).

Quoted 5 lines

I still dont get the usecase - other than "hey, we do this coz we can do it!".. I mean, yep, it sounds great and all.. but 4 years down the line, is this still going to work? is this going to be interesting careabout? or we are just maintaining additional code for a passing fancy or proof-of-concept?

Valid concern. From LNG BE group point of view it is not "we can do it". It is more like we've done it. We have Pandaboard ES running BE kernel for a while. It is in LNG BE tree. We used it as development and testing vehicle for BE work for a while. We are very grateful to the platform for that - it is affordable and easily available! Given, beyond ongoing BE testing on Pandaboard in LNG there may not be valid use case for further things on OMAP4 BE. We had discussion with Santosh Shilimkar from TI during last Linaro connect what to do with LNG BE Pandaboard series. Santosh suggested and we agreed that we would try to contribute them back to community. And that is what Taras is doing. IMHO even though there may not be real

product use case for BE OMAP4, it could serve in kernel source as good example that shows how to work with LE periphery from BE kernel. After all, these changes are noop for LE case and they don't introduce a lot of clutter: all BE asm rev and rev16 instructions wrapped around with ARM_BE8 macro.

Quoted 24 lines · Victor Kamensky

Quoted 8 lines · Nishanth Menon

Quoted 3 lines · Victor Kamensky

When BE kernel is built Makefile does take of compiling code in BE mode. I.e all proper flags like -mbig-endian and -Wl,--be8 will be set.

Agreed, and I assume you cannot instead switch to LE mode when entering assembly assuming LE?

Yes. Note that this asm interacts with other data in kernel that would be in BE form. And after all linker will not allow to put together files that were compiled in different endianity.

Quoted 9 lines

The reason I ask this is - most of our development is NOT in BE mode. we will continue to manipulate and add assembly - AM335x, DRA7/OMAP5 etc.. and obviously not every code change will indulge in ensuring right markers will be in place.

by ensuring readl_relaxed handles the variations, you have ensured that I dont need to care about drivers other than to ensure they use _relaxed variants. in the case of assembly, this does not seem long term manageable.

Quoted 8 lines

Yes, I agree if it is outside of main use case it will get broken if not attended to. Definitely universe entropy increases :) - if left without attention things will decay. Note we admit that even with ARM CPU core BE changes similar decay can happen eventually ...that is what LNG BE group trying to prevent. I think
the deal here is that next user of it can/need to fix things if they decayed.

Personally, I have no idea what "LNG BE" stands for.. I see the approach we are attempting here is to do any interaction external to ARM boundary to go through the ARM_BE8 macro.

If we want to make this something we can live with, then the platforms will have to ensure memory operations external to ARM must be operated upon by these macros. Instead, an approach that may be used is to introduce accessors macros that will provide right instruction based on BE/LE build and BE/LE SoC peripheral behavior.

Quoted 15 lines

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

is the idea of BE build meant to deal with having a single BE kernel build work for all platforms (including LE ones)?

Sort of. The idea here to run BE image on OMAP4 chip, with kernel that would deals with LE periphery correctly, but ARM core run in BE with special kernel that compiled for BE case (i.e CONFIG_CPU_BIG_ENDIAN is set).

Quoted 5 lines

I still dont get the usecase - other than "hey, we do this coz we can do it!".. I mean, yep, it sounds great and all.. but 4 years down the line, is this still going to work? is this going to be interesting careabout? or we are just maintaining additional code for a passing fancy or proof-of-concept?

Quoted 11 lines

Valid concern. From LNG BE group point of view it is not "we can do it". It is more like we've done it. We have Pandaboard ES running BE kernel for a while. It is in LNG BE tree. We used it as development and testing vehicle for BE work for a while. We are very grateful to the platform for that - it is affordable and easily available! Given, beyond ongoing BE testing on Pandaboard in LNG there may not be valid use case for further things on OMAP4 BE. We had discussion with Santosh Shilimkar from TI during last Linaro connect what to do with LNG BE Pandaboard series. Santosh suggested and we agreed that we would try to contribute them back to community. And that is what Taras is doing. IMHO even though there may not be real

ok.. some sort of Linaro thing about which I have no background about
- but dont really care in this context.

Quoted 4 lines

ok.. some sort of Linaro thing about which I have no background about
- but dont really care in this context.

Nothing related Linaro. Its just that platforms are supporting ARM BE mode and Linaro folks had working patches for Panda. So I suggested to get them on the lists.

Quoted 7 lines · Santosh Shilimkar

Quoted 4 lines

ok.. some sort of Linaro thing about which I have no background about
- but dont really care in this context.

Nothing related Linaro. Its just that platforms are supporting ARM BE mode and Linaro folks had working patches for Panda. So I suggested to get them on the lists.

I tend to think -> is this with OFF mode and CPUidle completely working? All context save and restore works with this? on HS and GP devices with BE mode builds? works on SDP4430,60 variations, considered reuse with AM43xx which could use parts of that logic?

I mean to indicate that terms like "works on panda" tends always to be relative.

It is nice to see it as a proof of concept, but I'd hate to see some dead code lying around in kernel and folks blindly following suit and introducing macros for new assembly for a feature that in practice just one group of folks care about and creates additional burden for rest of folks trying to keep that functionality going as we jump from one "device tree" style churn to another "framework"? Not to mean that good features should be kept away.. but personally, I could not find convincing arguments in this case..

Quoted 7 lines · Santosh Shilimkar

Quoted 4 lines

ok.. some sort of Linaro thing about which I have no background about
- but dont really care in this context.

Nothing related Linaro. Its just that platforms are supporting ARM BE mode and Linaro folks had working patches for Panda. So I suggested to get them on the lists.

I tend to think -> is this with OFF mode and CPUidle completely working? All context save and restore works with this? on HS and GP devices with BE mode builds? works on SDP4430,60 variations, considered reuse with AM43xx which could use parts of that logic?

I mean to indicate that terms like "works on panda" tends always to be relative.

It is nice to see it as a proof of concept, but I'd hate to see some dead code lying around in kernel and folks blindly following suit and introducing macros for new assembly for a feature that in practice just one group of folks care about and creates additional burden for rest of folks trying to keep that functionality going as we jump from one "device tree" style churn to another "framework"? Not to mean that good features should be kept away.. but personally, I could not find convincing arguments in this case..

On Tuesday 14 January 2014 04:13 PM, Nishanth Menon wrote:

I haven't looked at patch myself but as you pointed out if it adds dead code and makes the code un-readable then probably that something we shouldn't merge.