[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Guennadi Liakhovetski]

Hi

As I remarked in the opening email to the SMDK6400 patch-series, I've got 
a strange problem, I cannot understand.

Here're the c-code:

typedef struct {
	volatile u32	TCNTB;
	volatile u32	TCMPB;
	volatile u32	TCNTO;
} s3c64xx_timer;

typedef struct {
	volatile u32	TCFG0;
	volatile u32	TCFG1;
	volatile u32	TCON;
	s3c64xx_timer	ch[4];
	volatile u32	TCNTB4;
	volatile u32	TCNTO4;
} s3c64xx_timers;


static s3c64xx_timers *s3c64xx_get_base_timers(void)
{
	return (s3c64xx_timers *)ELFIN_TIMER_BASE;
}

/* macro to read the 16 bit timer */
static inline ulong read_timer(void)
{
	s3c64xx_timers *const timers = s3c64xx_get_base_timers();

	return timers->TCNTO4;
}

The respective assembly code is quite simple:

00000000 <read_timer>:
   0:	e59f3004 	ldr	r3, [pc, #4]	; c <timer_load_val>
   4:	e5930040 	ldr	r0, [r3, #64]
   8:	e12fff1e 	bx	lr
   c:	7f006000 	swivc	0x00006000

Perfect. Now, if I make the two structs above packed, I get this assembly:

00000000 <read_timer>:
   0:	e59f301c 	ldr	r3, [pc, #28]	; 24 <.text+0x24>
   4:	e5d30040 	ldrb	r0, [r3, #64]
   8:	e5d32041 	ldrb	r2, [r3, #65]
   c:	e5d31042 	ldrb	r1, [r3, #66]
  10:	e1800402 	orr	r0, r0, r2, lsl #8
  14:	e5d33043 	ldrb	r3, [r3, #67]
  18:	e1800801 	orr	r0, r0, r1, lsl #16
  1c:	e1800c03 	orr	r0, r0, r3, lsl #24
  20:	e12fff1e 	bx	lr
  24:	7f006000 	swivc	0x00006000

which converts all accesses to the structure to 8-bit... Ideas? Toolchain 
eldk-4.1. U-Boot from nand/testing git with the patches I just sent to the 
list, configured for smdk6400_config.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Wolfgang Denk]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0808111737330.30591@axis700.grange> you wrote:
> 
> As I remarked in the opening email to the SMDK6400 patch-series, I've got 
> a strange problem, I cannot understand.

What exactly is your problem?

> which converts all accesses to the structure to 8-bit... Ideas? Toolchain 
> eldk-4.1. U-Boot from nand/testing git with the patches I just sent to the 
> list, configured for smdk6400_config.

Note that you did not exactly ask a question, you just describe  your
observations so we have to guess what might be puzzeling you. I think
that  some  people  with  more ARM experience than me will simply not
understand what you are asking about, because they are  used  to  GCC
turning  perfectly  valid  looking 32 bit accesses into a sequence of
four 8 bit accesses.

They consider this "normal".

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
IMPORTANT NOTICE TO PURCHASERS: The Entire Physical Universe,  Inclu-
ding  This Product, May One Day Collapse Back into an Infinitesimally
Small Space. Should  Another  Universe  Subsequently  Re-emerge,  the
Existence of This Product in That Universe Cannot Be Guaranteed.

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Guennadi Liakhovetski]

On Mon, 11 Aug 2008, Wolfgang Denk wrote:

> Dear Guennadi Liakhovetski,
> 
> In message <Pine.LNX.4.64.0808111737330.30591@axis700.grange> you wrote:
> > 
> > As I remarked in the opening email to the SMDK6400 patch-series, I've got 
> > a strange problem, I cannot understand.
> 
> What exactly is your problem?

Right, sorry, the problem is simple: it doesn't work with the byte access.

> > which converts all accesses to the structure to 8-bit... Ideas? Toolchain 
> > eldk-4.1. U-Boot from nand/testing git with the patches I just sent to the 
> > list, configured for smdk6400_config.
> 
> Note that you did not exactly ask a question, you just describe  your
> observations so we have to guess what might be puzzeling you. I think
> that  some  people  with  more ARM experience than me will simply not
> understand what you are asking about, because they are  used  to  GCC
> turning  perfectly  valid  looking 32 bit accesses into a sequence of
> four 8 bit accesses.
> 
> They consider this "normal".

I cannot think of any such examples... I know access-types can be changed 
on the hardware, say, on a 16-bit bus a 32-bit access can be split into 
two 16-bit accesses, but I currently cannot think of any examples of the 
compiler doing such things. But, I think, I can imagine _why_ the compiler 
might decide to do that - it cannot guarantee, that the structure is 
32-bit aligned, so, it is opting for the "safe" variant... So, the 
question is: is the compiler right and the c-code was wrong, or the other 
way round?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Kenneth Johansson]

On Mon, 2008-08-11 at 18:23 +0200, Guennadi Liakhovetski wrote:

> I cannot think of any such examples... I know access-types can be changed 
> on the hardware, say, on a 16-bit bus a 32-bit access can be split into 
> two 16-bit accesses, but I currently cannot think of any examples of the 
> compiler doing such things. But, I think, I can imagine _why_ the compiler 
> might decide to do that - it cannot guarantee, that the structure is 
> 32-bit aligned, so, it is opting for the "safe" variant... So, the 
> question is: is the compiler right and the c-code was wrong, or the other 
> way round?

Both are "right" :). C has a lot of implementation specific stuff that
can make life interesting. Simple thing like if char is signed or
unsigned sometimes is really important. 

If you have to make sure that the read/write is done in 16 bit the only
option is to use assembler code. 

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Guennadi Liakhovetski]

On Mon, 11 Aug 2008, Kenneth Johansson wrote:

> 
> On Mon, 2008-08-11 at 18:23 +0200, Guennadi Liakhovetski wrote:
> 
> > I cannot think of any such examples... I know access-types can be changed 
> > on the hardware, say, on a 16-bit bus a 32-bit access can be split into 
> > two 16-bit accesses, but I currently cannot think of any examples of the 
> > compiler doing such things. But, I think, I can imagine _why_ the compiler 
> > might decide to do that - it cannot guarantee, that the structure is 
> > 32-bit aligned, so, it is opting for the "safe" variant... So, the 
> > question is: is the compiler right and the c-code was wrong, or the other 
> > way round?
> 
> Both are "right" :). C has a lot of implementation specific stuff that
> can make life interesting. Simple thing like if char is signed or
> unsigned sometimes is really important. 
> 
> If you have to make sure that the read/write is done in 16 bit the only
> option is to use assembler code. 

I think, properly written mmapped-register access functions like readl() 
work perfectly too.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:48 Mon 11 Aug     , Guennadi Liakhovetski wrote:
> Hi
> 
> As I remarked in the opening email to the SMDK6400 patch-series, I've got 
> a strange problem, I cannot understand.
> 
> Here're the c-code:
> 
> typedef struct {
> 	volatile u32	TCNTB;
> 	volatile u32	TCMPB;
> 	volatile u32	TCNTO;
> } s3c64xx_timer;
> 
> typedef struct {
> 	volatile u32	TCFG0;
> 	volatile u32	TCFG1;
> 	volatile u32	TCON;
> 	s3c64xx_timer	ch[4];
> 	volatile u32	TCNTB4;
> 	volatile u32	TCNTO4;
> } s3c64xx_timers;
IIRC in gcc 4

you're supposed to declare the struct as packet and not the typedef

struct {
	volatile u32	TCNTB;
	volatile u32	TCMPB;
	volatile u32	TCNTO;
} _s3c64xx_timer __attribute__ ((__packed__));
typedef struct _s3c64xx_timer s3c64xx_timer;

struct {
	volatile u32	TCFG0;
	volatile u32	TCFG1;
	volatile u32	TCON;
	s3c64xx_timer	ch[4];
	volatile u32	TCNTB4;
	volatile u32	TCNTO4;
} _s3c64xx_timers __attribute__ ((__packed__));
typedef struct _s3c64xx_timers s3c64xx_timers;

Best Regards,
J.

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Kenneth Johansson]

On Mon, 2008-08-11 at 19:00 +0200, Guennadi Liakhovetski wrote:
> On Mon, 11 Aug 2008, Kenneth Johansson wrote:
> 
> > 
> > On Mon, 2008-08-11 at 18:23 +0200, Guennadi Liakhovetski wrote:
> > 
> > > I cannot think of any such examples... I know access-types can be changed 
> > > on the hardware, say, on a 16-bit bus a 32-bit access can be split into 
> > > two 16-bit accesses, but I currently cannot think of any examples of the 
> > > compiler doing such things. But, I think, I can imagine _why_ the compiler 
> > > might decide to do that - it cannot guarantee, that the structure is 
> > > 32-bit aligned, so, it is opting for the "safe" variant... So, the 
> > > question is: is the compiler right and the c-code was wrong, or the other 
> > > way round?
> > 
> > Both are "right" :). C has a lot of implementation specific stuff that
> > can make life interesting. Simple thing like if char is signed or
> > unsigned sometimes is really important. 
> > 
> > If you have to make sure that the read/write is done in 16 bit the only
> > option is to use assembler code. 
> 
> I think, properly written mmapped-register access functions like readl() 
> work perfectly too.
> 

what does the compiler do if you change the code  to 

        return *(ulong*)&timers->TCNTO4;

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Guennadi Liakhovetski]

On Mon, 11 Aug 2008, Jean-Christophe PLAGNIOL-VILLARD wrote:

> On 17:48 Mon 11 Aug     , Guennadi Liakhovetski wrote:
> > Hi
> > 
> > As I remarked in the opening email to the SMDK6400 patch-series, I've got 
> > a strange problem, I cannot understand.
> > 
> > Here're the c-code:
> > 
> > typedef struct {
> > 	volatile u32	TCNTB;
> > 	volatile u32	TCMPB;
> > 	volatile u32	TCNTO;
> > } s3c64xx_timer;
> > 
> > typedef struct {
> > 	volatile u32	TCFG0;
> > 	volatile u32	TCFG1;
> > 	volatile u32	TCON;
> > 	s3c64xx_timer	ch[4];
> > 	volatile u32	TCNTB4;
> > 	volatile u32	TCNTO4;
> > } s3c64xx_timers;
> IIRC in gcc 4
> 
> you're supposed to declare the struct as packet and not the typedef
> 
> struct {
> 	volatile u32	TCNTB;
> 	volatile u32	TCMPB;
> 	volatile u32	TCNTO;
> } _s3c64xx_timer __attribute__ ((__packed__));
> typedef struct _s3c64xx_timer s3c64xx_timer;

I believe you meant

struct _s3c64xx_timer {
	volatile u32	TCNTB;
	volatile u32	TCMPB;
	volatile u32	TCNTO;
} __attribute__ ((__packed__));

but this doesn't change a thing, I checked it too. Even this

} __attribute__((__packed__,aligned(4)));

doesn't help.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Guennadi Liakhovetski]

On Mon, 11 Aug 2008, Kenneth Johansson wrote:

> On Mon, 2008-08-11 at 19:00 +0200, Guennadi Liakhovetski wrote:
> > On Mon, 11 Aug 2008, Kenneth Johansson wrote:
> > 
> > > On Mon, 2008-08-11 at 18:23 +0200, Guennadi Liakhovetski wrote:
> > > 
> > > > I cannot think of any such examples... I know access-types can be changed 
> > > > on the hardware, say, on a 16-bit bus a 32-bit access can be split into 
> > > > two 16-bit accesses, but I currently cannot think of any examples of the 
> > > > compiler doing such things. But, I think, I can imagine _why_ the compiler 
> > > > might decide to do that - it cannot guarantee, that the structure is 
> > > > 32-bit aligned, so, it is opting for the "safe" variant... So, the 
> > > > question is: is the compiler right and the c-code was wrong, or the other 
> > > > way round?
> > > 
> > > Both are "right" :). C has a lot of implementation specific stuff that
> > > can make life interesting. Simple thing like if char is signed or
> > > unsigned sometimes is really important. 
> > > 
> > > If you have to make sure that the read/write is done in 16 bit the only
> > > option is to use assembler code. 
> > 
> > I think, properly written mmapped-register access functions like readl() 
> > work perfectly too.
> 
> what does the compiler do if you change the code  to 
> 
>         return *(ulong*)&timers->TCNTO4;

Strangely, this doesn't help either... Nor does using 
readl(&timers->TCNTO4).

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Scott Wood]

On Mon, Aug 11, 2008 at 05:48:21PM +0200, Guennadi Liakhovetski wrote:
> The respective assembly code is quite simple:
> 
> 00000000 <read_timer>:
>    0:	e59f3004 	ldr	r3, [pc, #4]	; c <timer_load_val>
>    4:	e5930040 	ldr	r0, [r3, #64]
>    8:	e12fff1e 	bx	lr
>    c:	7f006000 	swivc	0x00006000
> 
> Perfect. Now, if I make the two structs above packed, I get this assembly:
> 
> 00000000 <read_timer>:
>    0:	e59f301c 	ldr	r3, [pc, #28]	; 24 <.text+0x24>
>    4:	e5d30040 	ldrb	r0, [r3, #64]
>    8:	e5d32041 	ldrb	r2, [r3, #65]
>    c:	e5d31042 	ldrb	r1, [r3, #66]
>   10:	e1800402 	orr	r0, r0, r2, lsl #8
>   14:	e5d33043 	ldrb	r3, [r3, #67]
>   18:	e1800801 	orr	r0, r0, r1, lsl #16
>   1c:	e1800c03 	orr	r0, r0, r3, lsl #24
>   20:	e12fff1e 	bx	lr
>   24:	7f006000 	swivc	0x00006000
> 
> which converts all accesses to the structure to 8-bit... Ideas? Toolchain 
> eldk-4.1. U-Boot from nand/testing git with the patches I just sent to the 
> list, configured for smdk6400_config.

Packed implies a lack of alignment.  Since ARM can't do unaligned
accesses in a "normal" way, the compiler avoids them.

Why do you need to pack the struct?

-Scott

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Guennadi Liakhovetski]

On Mon, 11 Aug 2008, Scott Wood wrote:

> On Mon, Aug 11, 2008 at 05:48:21PM +0200, Guennadi Liakhovetski wrote:
> > The respective assembly code is quite simple:
> > 
> > 00000000 <read_timer>:
> >    0:	e59f3004 	ldr	r3, [pc, #4]	; c <timer_load_val>
> >    4:	e5930040 	ldr	r0, [r3, #64]
> >    8:	e12fff1e 	bx	lr
> >    c:	7f006000 	swivc	0x00006000
> > 
> > Perfect. Now, if I make the two structs above packed, I get this assembly:
> > 
> > 00000000 <read_timer>:
> >    0:	e59f301c 	ldr	r3, [pc, #28]	; 24 <.text+0x24>
> >    4:	e5d30040 	ldrb	r0, [r3, #64]
> >    8:	e5d32041 	ldrb	r2, [r3, #65]
> >    c:	e5d31042 	ldrb	r1, [r3, #66]
> >   10:	e1800402 	orr	r0, r0, r2, lsl #8
> >   14:	e5d33043 	ldrb	r3, [r3, #67]
> >   18:	e1800801 	orr	r0, r0, r1, lsl #16
> >   1c:	e1800c03 	orr	r0, r0, r3, lsl #24
> >   20:	e12fff1e 	bx	lr
> >   24:	7f006000 	swivc	0x00006000
> > 
> > which converts all accesses to the structure to 8-bit... Ideas? Toolchain 
> > eldk-4.1. U-Boot from nand/testing git with the patches I just sent to the 
> > list, configured for smdk6400_config.
> 
> Packed implies a lack of alignment.  Since ARM can't do unaligned
> accesses in a "normal" way, the compiler avoids them.
> 
> Why do you need to pack the struct?

I don't. These structs are already filled with dummy bytes. So it also 
works without the packed attribute. Why I tried to enable it was because 
the attribute was in the original code from Samsung, but was commented 
out, so, I wanted to find out what happens if I re-enable it. Besides, the 
packed attribute serves as a hint in the source, that this struct has 
special requirements for its in-memory representation.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] __attribute__((__packed__)) switching to byte-access on ARM

[Guennadi Liakhovetski]

On Mon, 11 Aug 2008, Guennadi Liakhovetski wrote:

> but this doesn't change a thing, I checked it too. Even this
> 
> } __attribute__((__packed__,aligned(4)));
> 
> doesn't help.

Sorry, this last statement is wrong, adding aligned(4) does help.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de