[U-Boot] Make preparatory patches that initially have no effect?

[U-Boot] Make preparatory patches that initially have no effect?

[Reinhard Meyer]

Hello,

my current work in the AT91 SoCs will add several new drivers.

Some of them require new header files in arch-at91 or amendments
to existing header files there. Same for some at91 specific
header files in driver/*.

In existing header files it is adding adresses for
going-to-be-used devices and proper struct SoC defines for
them, the new header files will only allow struct SoC access
anyway.

If I add those discrete changes to each driver patch (where it
might actually belong), the incremental changes to some of
those files would require all those driver patches to be applied
in the right order to avoid conflicts.

Therefore I would like to put all new header files and all
changes to header files in one patch which would need to be
applied before the driver patches.

That patch would essentially cause no change to existing code.

Anyone find this idea bad?

Reinhard

[U-Boot] Make preparatory patches that initially have no effect?

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4C665CB9.2040406@emk-elektronik.de> you wrote:
> 
> If I add those discrete changes to each driver patch (where it
> might actually belong), the incremental changes to some of
> those files would require all those driver patches to be applied
> in the right order to avoid conflicts.

Yes, and? What's the problem with that?

> Therefore I would like to put all new header files and all
> changes to header files in one patch which would need to be
> applied before the driver patches.
> 
> That patch would essentially cause no change to existing code.
> 
> Anyone find this idea bad?

Yes,m that's a bad idea. Please re-read the "patches" wiki page.
Commits shall be atomic, and complete. Splitting stuff that
belongstogether is a bad idea, and your first patch that adds unused
stuff will be rejected because of that reason: adding unused stuff.

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
Perl already has an IDE.  It's called Unix.
                      -- Tom Christiansen in 375bd509 at cs.colorado.edu

[U-Boot] Make preparatory patches that initially have no effect?

[Reinhard Meyer]

Dear Wolfgang Denk,
>> If I add those discrete changes to each driver patch (where it
>> might actually belong), the incremental changes to some of
>> those files would require all those driver patches to be applied
>> in the right order to avoid conflicts.
> 
> Yes, and? What's the problem with that?

None for me. Only for people that want to try out a single (driver)
patch.

For example at91_gpbr.h is required by two independent patches.
Of course, I could base both patches such that each one introduces
that file.

> Yes,m that's a bad idea. Please re-read the "patches" wiki page.
> Commits shall be atomic, and complete. Splitting stuff that
> belongstogether is a bad idea, and your first patch that adds unused
> stuff will be rejected because of that reason: adding unused stuff.

I know that, however it could be argued that adding header files to
describe an architectures' hardware is not exactly specific to a driver.
Thats why I asked....

Best Regards,
Reinhard

[U-Boot] Make preparatory patches that initially have no effect?

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4C66CCCF.9080303@emk-elektronik.de> you wrote:
>
> None for me. Only for people that want to try out a single (driver)
> patch.

They can apply the patch series anyway (at least to the patch they are
interested in).

> For example at91_gpbr.h is required by two independent patches.
> Of course, I could base both patches such that each one introduces
> that file.

Have the first add that file, and the second assume it comes later in
the sequence.

> > Yes,m that's a bad idea. Please re-read the "patches" wiki page.
> > Commits shall be atomic, and complete. Splitting stuff that
> > belongstogether is a bad idea, and your first patch that adds unused
> > stuff will be rejected because of that reason: adding unused stuff.
> 
> I know that, however it could be argued that adding header files to
> describe an architectures' hardware is not exactly specific to a driver.
> Thats why I asked....

The wiki page does not talk about drivers... It's a general rule and
applies to all sorts of code. Only add what is really used (this also
refers, for example, to struct definitions for register blocks etc. -
don't try to provide a complete description of your SoC; add only
stuff that is actually used by the code).

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
"I like your game but we have to change the rules."

[U-Boot] Struct SoC access (was:Make preparatory patches that initially have no effect?)

[Reinhard Meyer]

Dear Wolfgang Denk,
> Have the first add that file, and the second assume it comes later in
> the sequence.

You don't mean by "sequence" PATCH 1/n, 2/n, etc? The drivers are so
independent that that would not really make sense...

> The wiki page does not talk about drivers... It's a general rule and
> applies to all sorts of code. Only add what is really used (this also
> refers, for example, to struct definitions for register blocks etc. -
> don't try to provide a complete description of your SoC; add only
> stuff that is actually used by the code).

That's a thin line. Although I need only one register of the DBU (for
example) I think its wise to define all registers in it, and not to
_reserve[] the unused ones....

Anyway, is the method of (for example!)

#define DBU_ADDR 0xsomething (in a SoC header file)

dbu_t *dbu = (dbu_t *)DBU_ADDR; (in a function)

OK?

Or do we need to further encapsulate that in a function like

dbut_t *get_dbu_addr(void) {return (dbu_t *)DBU_ADDR;}


I was even thinking of something like

struct soc {
	u32 xyz[0x80];	/* XYZ unit */
	u32 dbu[0x80];	/* Debug Unit */
	u32 rstc[0x80];	/* Reset Controller */
and so on.

Then in a driver one could write
dbu_t *dbu = (dbu_t *)soc.dbu;
or something along that line

Best Regards
Reinhard

[U-Boot] Struct SoC access (was:Make preparatory patches that initially have no effect?)

[Mike Frysinger]

On Sat, Aug 14, 2010 at 3:30 PM, Reinhard Meyer wrote:
> #define DBU_ADDR 0xsomething (in a SoC header file)
>
> dbu_t *dbu = (dbu_t *)DBU_ADDR; (in a function)

needs to be volatile ...
-mike

[U-Boot] Struct SoC access

[Reinhard Meyer]

Dear Mike Frysinger,
> On Sat, Aug 14, 2010 at 3:30 PM, Reinhard Meyer wrote:
>> #define DBU_ADDR 0xsomething (in a SoC header file)
>>
>> dbu_t *dbu = (dbu_t *)DBU_ADDR; (in a function)
> 
> needs to be volatile ...
> -mike
Why? The elements are used as parameters to readl/writel functions:
status = readl(&dbu->sr);

I am quite sure we are NOT supposed to directly access hardware:
status = dbu->sr;

Best Regards,
Reinhard

[U-Boot] Struct SoC access

[Mike Frysinger]

On Sat, Aug 14, 2010 at 3:40 PM, Reinhard Meyer wrote:
> Dear Mike Frysinger,
>> On Sat, Aug 14, 2010 at 3:30 PM, Reinhard Meyer wrote:
>>> #define DBU_ADDR 0xsomething (in a SoC header file)
>>>
>>> dbu_t *dbu = (dbu_t *)DBU_ADDR; (in a function)
>>
>> needs to be volatile ...
>
> Why? The elements are used as parameters to readl/writel functions:
> status = readl(&dbu->sr);

if you use accessor functions and those guarantee volatility, then you
dont need the markings on the pointer

> I am quite sure we are NOT supposed to directly access hardware:
> status = dbu->sr;

it depends on the hardware
-mike

[U-Boot] Struct SoC access (was:Make preparatory patches that initially have no effect?)

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4C66EECA.5020509@emk-elektronik.de> you wrote:
>
> > Have the first add that file, and the second assume it comes later in
> > the sequence.
> 
> You don't mean by "sequence" PATCH 1/n, 2/n, etc? The drivers are so
> independent that that would not really make sense...

Then just write in the comment part of the second patch that the other
one has to be applied first...

> That's a thin line. Although I need only one register of the DBU (for
> example) I think its wise to define all registers in it, and not to
> _reserve[] the unused ones....

Right. If you add a function, add all the registers in it.
But don't bother to explain each and every bit in the registers you
never refer to, nor add completely unrelated blocks.

> Anyway, is the method of (for example!)
> 
> #define DBU_ADDR 0xsomething (in a SoC header file)
> 
> dbu_t *dbu = (dbu_t *)DBU_ADDR; (in a function)
> 
> OK?

Yes.

> Or do we need to further encapsulate that in a function like

No.

> I was even thinking of something like
> 
> struct soc {
> 	u32 xyz[0x80];	/* XYZ unit */
> 	u32 dbu[0x80];	/* Debug Unit */
> 	u32 rstc[0x80];	/* Reset Controller */
> and so on.

This is what PPC used to do; I like that - but ARM people always
explained to me that it makes no sense because address space on ARM
SoC is only sparely populated.

> Then in a driver one could write
> dbu_t *dbu = (dbu_t *)soc.dbu;
> or something along that line

I think this looks nice, but as mentioned before - I'm not an ARM
expert. They tend to do it differently.

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
Q:  How many DEC repairman does it take to fix a flat ?
A:  Five; four to hold the car up and one to swap tires.

[U-Boot] Struct SoC access

[Reinhard Meyer]

Dear Wolfgang Denk,
> Then just write in the comment part of the second patch that the other
> one has to be applied first...

Thats fine with me.

>> Anyway, is the method of (for example!)
>>
>> #define DBU_ADDR 0xsomething (in a SoC header file)
>>
>> dbu_t *dbu = (dbu_t *)DBU_ADDR; (in a function)
>>
>> OK?
> 
> Yes.

Thats the way I do it currently

>> Or do we need to further encapsulate that in a function like
> 
> No.

I have seen that somewhere in u-boot.

>> I was even thinking of something like
>>
>> struct soc {
>> 	u32 xyz[0x80];	/* XYZ unit */
>> 	u32 dbu[0x80];	/* Debug Unit */
>> 	u32 rstc[0x80];	/* Reset Controller */
>> and so on.
> 
> This is what PPC used to do; I like that - but ARM people always
> explained to me that it makes no sense because address space on ARM
> SoC is only sparely populated.

Even if, that's no reason, on can write "u32 spi0[0x1000]", on AT91
the spacing of peripherals is 0x4000 bytes, in fact it repeats
times in its window. The system stuff is like one peripheral with
its components spaced by 0x200 bytes (hence the 0x80 above).

> I think this looks nice, but as mentioned before - I'm not an ARM
> expert. They tend to do it differently.

I can be done for AT91, but I'm not Don Quichote ;)

Would the toolchain "gulp" when one defines the whole 4 GB that way?

Best Regards,
Reinhard

[U-Boot] Struct SoC access

[Reinhard Meyer]

> Would the toolchain "gulp" when one defines the whole 4 GB that way?

In fact, a rather novel approach (just theorizing here):

#define SRAM_BASE offsetof(soc.sram)
#define SRAM_SIZE sizeof(soc.sram)

dbu_t *dbu = (dbu_t *)offsetof(soc.dbu);

without ever assigning soc the address 0...

Reinhard

[U-Boot] Struct SoC access

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4C6700E5.2070009@emk-elektronik.de> you wrote:
> > Would the toolchain "gulp" when one defines the whole 4 GB that way?
> 
> In fact, a rather novel approach (just theorizing here):
> 
> #define SRAM_BASE offsetof(soc.sram)
> #define SRAM_SIZE sizeof(soc.sram)
> 
> dbu_t *dbu = (dbu_t *)offsetof(soc.dbu);
> 
> without ever assigning soc the address 0...

Urgh... that's a log of pretty heavy assumptions, and not exactly
readable / understandable code either.  I gues your're not targeting
the next IOCCC?

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
Die Scheu vor Verantwortung ist die Krankheit unserer Zeit.
                                                 -- Otto von Bismarck

[U-Boot] Struct SoC access

[Wolfgang Denk]

Dear Reinhard Meyer,

In message <4C66F54D.2060701@emk-elektronik.de> you wrote:
>
> >> I was even thinking of something like
> >>
> >> struct soc {
> >> 	u32 xyz[0x80];	/* XYZ unit */
> >> 	u32 dbu[0x80];	/* Debug Unit */
> >> 	u32 rstc[0x80];	/* Reset Controller */
> >> and so on.
> > 
> > This is what PPC used to do; I like that - but ARM people always
> > explained to me that it makes no sense because address space on ARM
> > SoC is only sparely populated.
> 
> Even if, that's no reason, on can write "u32 spi0[0x1000]", on AT91
> the spacing of peripherals is 0x4000 bytes, in fact it repeats
> times in its window. The system stuff is like one peripheral with
> its components spaced by 0x200 bytes (hence the 0x80 above).

I think we have a misunderstaning ehre - I thought the entries like
"xyz" were indeed "u32" types - buut now I get the impression that
what you have in mind is that they are actually structs describing
hardware blocks.

Then it should be written like that.

For example, see file "arch/powerpc/include/asm/8xx_immap.h":

struct immap is what corresponds to your struct soc above.

> Would the toolchain "gulp" when one defines the whole 4 GB that way?

Why not?

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
It [being a Vulcan] means to adopt a philosophy, a way of life  which
is logical and beneficial. We cannot disregard that philosophy merely
for personal gain, no matter how important that gain might be.
	-- Spock, "Journey to Babel", stardate 3842.4

[U-Boot] Struct SoC access

[Reinhard Meyer]

Dear Wolfgang Denk,
> I think we have a misunderstaning ehre - I thought the entries like
> "xyz" were indeed "u32" types - buut now I get the impression that
> what you have in mind is that they are actually structs describing
> hardware blocks.

No, thats just spacemakers, otherwise one would have do declare all
structs for all hardware blocks in that file (or include a bunch of files).

> struct immap is what corresponds to your struct soc above.

Yes, with at least all currently used blocks declared as structs,
the currently unused ones made up of the proper amount of u32s
Since different AT91 SoC might have some blocks missing or at other
addresses in another amount etc.,
but with the same layout of each individual block, it would make sense
to keep the block structure definitions in separate files like
at91_dbu.h, at91_rstc.h, etc.
They would have to be included in the corresponding at91sam9260.h,
at91sam9261.h, etc. where the individual soc layouts would be defined.

>> Would the toolchain "gulp" when one defines the whole 4 GB that way?
> 
> Why not?

Since the AT91s have no base address registers at all, the memory layout
is completely fixed. Even chip selects have a fixed address and fixed
size of 256MB each. Therefore it _might_ make sense to completely
define the 4GB in the soc struct.
Then assign struct soc *soc = (struct soc *)0;

Did you read Mike's comment of hardware dependent direct usage of
struct members to access hardware? I thought that was generally
discouraged for several reasons (cache and access sequencing)?

Whats a IOCCC?

Reinhard

[U-Boot] Struct SoC access

[Scott Wood]

On Sun, Aug 15, 2010 at 12:15:29AM +0200, Reinhard Meyer wrote:
> >> Would the toolchain "gulp" when one defines the whole 4 GB that way?
> > 
> > Why not?
> 
> Since the AT91s have no base address registers at all, the memory layout
> is completely fixed. Even chip selects have a fixed address and fixed
> size of 256MB each. Therefore it _might_ make sense to completely
> define the 4GB in the soc struct.
> Then assign struct soc *soc = (struct soc *)0;

One snag you might hit is that dereferencing a NULL pointer is undefined,
and some versions of GCC assume you won't do this when optimizing.  Not sure
if this simple usage would be affected (it seems to mainly be an issue when
comparing a pointer to NULL after dereferencing), and
-fno-delete-null-pointer-checks may help.

> Did you read Mike's comment of hardware dependent direct usage of
> struct members to access hardware? I thought that was generally
> discouraged for several reasons (cache and access sequencing)?
> 
> Whats a IOCCC?

The International Obfuscated C Code Contest, possibly a more appropriate
venue for code that defines a 4GB struct. :-)

-Scott

[U-Boot] Struct SoC access

[Reinhard Meyer]

Dear Scott Wood,
>> Then assign struct soc *soc = (struct soc *)0;
> 
> One snag you might hit is that dereferencing a NULL pointer is undefined,
> and some versions of GCC assume you won't do this when optimizing.  Not sure
> if this simple usage would be affected (it seems to mainly be an issue when
> comparing a pointer to NULL after dereferencing), and
> -fno-delete-null-pointer-checks may help.
Its just an idea anyway...

Certainly the 1st way I proposed is the easiest to go:
#define BLOCK_BASE_ADDR 0xsomething
block_t *block = (block_t *)BLOCK_BASE_ADDR;

>> Whats a IOCCC?
> 
> The International Obfuscated C Code Contest, possibly a more appropriate
> venue for code that defines a 4GB struct. :-)
In that case I am quite sure the current AT91 way of defining the hardware
addresses to the drivers already qualifies for that ;)
(Try to follow the definition of SPI0_BASE...)

arch-at91/memory_map.h:
...
#ifndef __ASM_ARM_ARCH_MEMORYMAP_H__
#define __ASM_ARM_ARCH_MEMORYMAP_H__

#include <asm/arch/hardware.h>

#define USART0_BASE AT91_USART0
#define USART1_BASE AT91_USART1
#define USART2_BASE AT91_USART2
#define USART3_BASE (AT91_BASE_SYS + AT91_DBGU)
#define SPI0_BASE	AT91_BASE_SPI
#define SPI1_BASE	AT91_BASE_SPI1

#endif /* __ASM_ARM_ARCH_MEMORYMAP_H__ */
...

arch-at91/harware.h:
...
#if defined(CONFIG_AT91RM9200)
#include <asm/arch-at91/at91rm9200.h>
#elif defined(CONFIG_AT91SAM9260) || defined(CONFIG_AT91SAM9G20)
#include <asm/arch/at91sam9260.h>
#define AT91_BASE_SPI	AT91SAM9260_BASE_SPI0
#define AT91_BASE_SPI1	AT91SAM9260_BASE_SPI1
#define AT91_ID_UHP	AT91SAM9260_ID_UHP
#define AT91_PMC_UHP	AT91SAM926x_PMC_UHP
#define AT91_BASE_MMCI	AT91SAM9260_BASE_MCI
#elif defined(CONFIG_AT91SAM9261) || defined(CONFIG_AT91SAM9G10)
#include <asm/arch/at91sam9261.h>
#define AT91_BASE_SPI	AT91SAM9261_BASE_SPI0
#define AT91_ID_UHP	AT91SAM9261_ID_UHP
#define AT91_PMC_UHP	AT91SAM926x_PMC_UHP
#elif defined(CONFIG_AT91SAM9263)
...

arch-at91/at91sam9260.h:
...
#ifdef CONFIG_AT91_LEGACY

/*
  * User Peripheral physical base addresses.
  */
#define AT91SAM9260_BASE_TCB0		0xfffa0000
#define AT91SAM9260_BASE_TC0		0xfffa0000
#define AT91SAM9260_BASE_TC1		0xfffa0040
#define AT91SAM9260_BASE_TC2		0xfffa0080
#define AT91SAM9260_BASE_UDP		0xfffa4000
#define AT91SAM9260_BASE_MCI		0xfffa8000
#define AT91SAM9260_BASE_TWI		0xfffac000
#define AT91SAM9260_BASE_US0		0xfffb0000
#define AT91SAM9260_BASE_US1		0xfffb4000
#define AT91SAM9260_BASE_US2		0xfffb8000
#define AT91SAM9260_BASE_SSC		0xfffbc000
#define AT91SAM9260_BASE_ISI		0xfffc0000
#define AT91SAM9260_BASE_EMAC		0xfffc4000
#define AT91SAM9260_BASE_SPI0		0xfffc8000
#define AT91SAM9260_BASE_SPI1		0xfffcc000
#define AT91SAM9260_BASE_US3		0xfffd0000
#define AT91SAM9260_BASE_US4		0xfffd4000
#define AT91SAM9260_BASE_US5		0xfffd8000
#define AT91SAM9260_BASE_TCB1		0xfffdc000
#define AT91SAM9260_BASE_TC3		0xfffdc000
#define AT91SAM9260_BASE_TC4		0xfffdc040
#define AT91SAM9260_BASE_TC5		0xfffdc080
#define AT91SAM9260_BASE_ADC		0xfffe0000
#define AT91_BASE_SYS			0xffffe800
...

arch-at91/at91sam9261.h:
...
#ifdef CONFIG_AT91_LEGACY

/*
  * User Peripheral physical base addresses.
  */
#define AT91SAM9261_BASE_TCB0		0xfffa0000
#define AT91SAM9261_BASE_TC0		0xfffa0000
#define AT91SAM9261_BASE_TC1		0xfffa0040
#define AT91SAM9261_BASE_TC2		0xfffa0080
#define AT91SAM9261_BASE_UDP		0xfffa4000
#define AT91SAM9261_BASE_MCI		0xfffa8000
#define AT91SAM9261_BASE_TWI		0xfffac000
#define AT91SAM9261_BASE_US0		0xfffb0000
#define AT91SAM9261_BASE_US1		0xfffb4000
#define AT91SAM9261_BASE_US2		0xfffb8000
#define AT91SAM9261_BASE_SSC0		0xfffbc000
#define AT91SAM9261_BASE_SSC1		0xfffc0000
#define AT91SAM9261_BASE_SSC2		0xfffc4000
#define AT91SAM9261_BASE_SPI0		0xfffc8000
#define AT91SAM9261_BASE_SPI1		0xfffcc000
#define AT91_BASE_SYS			0xffffea00
...

isn't that obfuscated enough?

Reinhard