[U-Boot] Make preparatory patches that initially have no effect?
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
participants (4)
-
Mike Frysinger -
Reinhard Meyer -
Scott Wood -
Wolfgang Denk