OneNAND board init support
PATCH 1/1 OneNAND board init support
Some CPU has internal OneNAND controller and use it for access OneNAND
To support these CPU, we provide the onenand_board_init Also we can override the onenand_read_page.
Some minor fixed:
- Remove unnecessary header file
- Fix wrong access at read interrupt
- The recent OneNAND has 4KiB pagesize
---
onenand_ipl/onenand_boot.c +0 -1
@@ -24,7 +24,6 @@*/ #include <common.h> -#include <version.h> #include "onenand_ipl.h"
onenand_ipl/onenand_ipl.h +8 -1
@@ -29,7 +29,14 @@#define THIS_ONENAND (a) (CONFIG_SYS_ONENAND_BASE + (a))#define READ_INTERRUPT () \ - onenand_readw(THIS_ONENAND(ONENAND_REG_INTERRUPT)) + onenand_readw(ONENAND_REG_INTERRUPT) +enum { +ONENAND_USE_DEFAULT , +ONENAND_USE_GENERIC , +}; + +extern int (*onenand_read_page)(ulong block,ulong page, +u_char *buf,int pagesize);extern int onenand_read_block (unsigned char *buf);#endif
onenand_ipl/onenand_read.c +44 -14
90 lines not on this page · raw message
PATCH 1/1 OneNAND board init support
onenand_ipl/onenand_ipl.h line 32
Quoted 3 lines · Kyungmin Park
#define
READ_INTERRUPT()\onenand_ipl/onenand_ipl.h +1
-1-onenand_readw (THIS_ONENAND (ONENAND_REG_INTERRUPT )) +onenand_readw (ONENAND_REG_INTERRUPT )
You could get rid of the newline now...
onenand_ipl/onenand_ipl.h line 37
Quoted 4 lines
onenand_ipl/onenand_ipl.h +4
-0+enum { +ONENAND_USE_DEFAULT , +ONENAND_USE_GENERIC , +};
What is this? Add a comment, and possibly more specific names.
onenand_ipl/onenand_ipl.h line 40
Quoted 2 lines
onenand_ipl/onenand_ipl.h +2
-0+extern int (*onenand_read_page)(ulong block,ulong page, +u_char *buf,int pagesize);
Maybe use a weak function instead? Or an #ifdef CONFIG_SYS_ONENAND_BOARD_READ_PAGE that will keep the code for the generic version from being in the image (it'd be nice if we could optimize out replaced weak functions). It seems especially odd that you use one method for init and another for read page.
onenand_ipl/onenand_read.c line 45
Quoted 4 lines
/* read a page with ECC */
onenand_ipl/onenand_read.c +1
-1-static inline int onenand_read_page (ulong block,ulong page, +static int generic_onenand_read_page (ulong block,ulong page,u_char * buf,int pagesize)
Is the "generic" code really generic, or is it just one specific controller?
Quoted 1 line
+#ifdef
CONFIG_ONENAND_BOARD_INIT
This should probably be CONFIG_SYS_ONENAND_BOARD_INIT -- it's not tweakable by the end user.
How is this different from the existing CONFIG_USE_ONENAND_BOARD_INIT?
Quoted 4 lines
onenand_ipl/onenand_read.c +4
-0+onenand_board_init (&page_is_4KiB, &page); +#else +onenand_generic_init (&page_is_4KiB, &page); +#endif
onenand_ipl/onenand_read.c static inline int onenand_read_page(ulong block, ulong page, · line 143
Quoted 10 lines
onenand_ipl/onenand_read.c +2
-5-if (onenand_readw (ONENAND_REG_TECHNOLOGY )) { - pagesize =4096 ;/* MLC OneNAND has 4KiB pagesize */ +if (page_is_4KiB) { + pagesize =4096 ;/* OneNAND has 4KiB pagesize */ erase_shift =18 ; - }else { - pagesize =2048 ; - erase_shift =17 ; }
I don't understand why you move the pagesize/erase_shift init before onenand_board_init, suggesting that the init code change it if it needs changing -- but then leave the page_is_4KiB stuff in the generic code.
This should probably just be filled in by the init code without anything here.
-Scott
PATCH 1/1 OneNAND board init support
Quoted 6 lines · Scott Wood
Quoted 3 lines · Kyungmin Park
?#define
READ_INTERRUPT()? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
- ? ? onenand_readw(THIS_ONENAND(ONENAND_REG_INTERRUPT))
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?onenand_readw(ONENAND_REG_INTERRUPT)You could get rid of the newline now...
It exceeds the 80 columns.
Quoted 7 lines
onenand_ipl/onenand_ipl.h line 37
Quoted 5 lines
onenand_ipl/onenand_ipl.h +4
-0+enum { + ? ? ONENAND_USE_DEFAULT, + ? ? ONENAND_USE_GENERIC, +};What is this? ?Add a comment, and possibly more specific names.
I see redefine the specific names and comments.
Quoted 9 lines
Quoted 3 lines
+extern int (*onenand_read_page)(ulong block, ulong page,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?u_char*buf, int pagesize);Maybe use a weak function instead? ?Or an #ifdef
CONFIG_SYS_ONENAND_BOARD_READ_PAGEthat will keep the code for the generic version from being in the image (it'd be nice if we could optimize out replaced weak functions). ?It seems especially odd that you use one method for init and another for read page.
I tried to use weak function but it produces more than expected. as you know it got size limitation.
When use the weak function. the apollon board will be broken. and I don't want to use #ifdef. since Now we support two different CPUs, s5pc100, s5pc110. these accesses different way. s5pc100 use own OneNAND controller. but s5pc110 use generic OneNAND method. That's reason to define the function pointer.
Quoted 8 lines
Quoted 5 lines
?/* read a page with ECC */
onenand_ipl/onenand_read.c +1
-1-static inline int onenand_read_page(ulong block, ulong page, +static int generic_onenand_read_page(ulong block, ulong page,? ? ? ? ? ? ? ? ? ? ? ? ? ? ?
u_char* buf, int pagesize)Is the "generic" code really generic, or is it just one specific controller?
The 'generic' means the original OneNAND access method. Use NOR interface and use OneNAND registers.
Most and Most generic method.
Only Samsung SoCs, s3c64xx series, and s5pc100 uses their own OneNAND controller.
Quoted 7 lines
Quoted 2 lines
+#ifdef
CONFIG_ONENAND_BOARD_INITThis should probably be
CONFIG_SYS_ONENAND_BOARD_INIT-- it's not tweakable by the end user.How is this different from the existing
CONFIG_USE_ONENAND_BOARD_INIT?
Okay l try to consider how to use same configurations.
Quoted 20 lines
Quoted 10 lines
+ ? ?
onenand_board_init(&page_is_4KiB, &page);(quoted diff) +5
-2+#else + ? ? onenand_generic_init(&page_is_4KiB, &page); +#endif - ? ? if (onenand_readw(ONENAND_REG_TECHNOLOGY)) { - ? ? ? ? ? ? pagesize = 4096; /* MLC OneNAND has 4KiB pagesize */ + ? ? if (page_is_4KiB) { + ? ? ? ? ? ? pagesize = 4096; /* OneNAND has 4KiB pagesize */Quoted 5 lines
? ? ? ? ? ? ?
erase_shift= 18;
- ? ? } else {
- ? ? ? ? ? ? pagesize = 2048;
- ? ? ? ? ? ?erase_shift= 17;
? ? ? }I don't understand why you move the pagesize/erase_shift init before
onenand_board_init, suggesting that the init code change it if it needs changing -- but then leave thepage_is_4KiBstuff in the generic code.
Quoted 2 lines
This should probably just be filled in by the init code without anything here.
No different. basically I assume OneNAND has 2KiB pagesize and In special case, MLC, and 4KiB pagesize OneNAND set the 4KiB pagesize.
If you want to leave as before. no problem.
Please consider the code size and don't want to break exsiting board support. That's reason I can't use weak function and use #ifdef at onenand_board_init.
Thank you,
Kyungmin Park
PATCH 1/1 OneNAND board init support
Quoted 9 lines · Kyungmin Park
Quoted 6 lines · Scott Wood
Quoted 3 lines · Kyungmin Park
?#define
READ_INTERRUPT()? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
- ? ? onenand_readw(THIS_ONENAND(ONENAND_REG_INTERRUPT))
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?onenand_readw(ONENAND_REG_INTERRUPT)You could get rid of the newline now...
It exceeds the 80 columns.
No it doesn't, anymore. You'll note that the start of onenand_readw() in to the right of the end of READ_INTERRUPT, so you're not saving any horizontal space with the newline.
Quoted 13 lines
Quoted 6 lines
onenand_ipl/onenand_ipl.h line 37
Quoted 4 lines
onenand_ipl/onenand_ipl.h +4
-0+enum { + ? ? ONENAND_USE_DEFAULT, + ? ? ONENAND_USE_GENERIC, +};What is this? ?Add a comment, and possibly more specific names.
I see redefine the specific names and comments.
Quoted 4 lines
Quoted 3 lines
+extern int (*onenand_read_page)(ulong block, ulong page,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?u_char*buf, int pagesize);
Quoted 7 lines
Quoted 5 lines
Maybe use a weak function instead? ?Or an #ifdef
CONFIG_SYS_ONENAND_BOARD_READ_PAGEthat will keep the code for the generic version from being in the image (it'd be nice if we could optimize out replaced weak functions). ?It seems especially odd that you use one method for init and another for read page.I tried to use weak function but it produces more than expected.
More than compiling both functions and choosing with a function pointer?
Quoted 2 lines
as you know it got size limitation. When use the weak function. the apollon board will be broken.
Broken how? Size?
Quoted 4 lines
and I don't want to use #ifdef. since Now we support two different CPUs, s5pc100, s5pc110. these accesses different way. s5pc100 use own OneNAND controller. but s5pc110 use generic OneNAND method. That's reason to define the function pointer.
Function pointers make sense if you want to override on a per-device basis (i.e. multiple controller types in the same system) or dynamically (different hardware handled by the same binary). Are you trying to generate one image that works on both s5pc100 and s5pc110? That sounds pretty luxurious for a space-constrained NAND bootstrap. :-)
Quoted 11 lines
Quoted 7 lines
Quoted 4 lines
?/* read a page with ECC */
onenand_ipl/onenand_read.c +1
-1-static inline int onenand_read_page(ulong block, ulong page, +static int generic_onenand_read_page(ulong block, ulong page,? ? ? ? ? ? ? ? ? ? ? ? ? ? ?
u_char* buf, int pagesize)Is the "generic" code really generic, or is it just one specific controller?
The 'generic' means the original OneNAND access method. Use NOR interface and use OneNAND registers.
Most and Most generic method.
OK, good; it was never clear to me just which hardware the existing onenand code has been targeting; I had gotten the impression that it was for one of the chips with a custom controller.
Better, though, would be to still have good separation between that implementation and the infrastructure that is truly generic even when you have a special controller. This is something I don't like about the current NAND code.
Quoted 22 lines
Quoted 19 lines
Quoted 9 lines
+ ? ?
onenand_board_init(&page_is_4KiB, &page);(quoted diff) +5
-2+#else + ? ? onenand_generic_init(&page_is_4KiB, &page); +#endif - ? ? if (onenand_readw(ONENAND_REG_TECHNOLOGY)) { - ? ? ? ? ? ? pagesize = 4096; /* MLC OneNAND has 4KiB pagesize */ + ? ? if (page_is_4KiB) { + ? ? ? ? ? ? pagesize = 4096; /* OneNAND has 4KiB pagesize */Quoted 5 lines
? ? ? ? ? ? ?
erase_shift= 18;
- ? ? } else {
- ? ? ? ? ? ? pagesize = 2048;
- ? ? ? ? ? ?erase_shift= 17;
? ? ? }I don't understand why you move the pagesize/erase_shift init before
onenand_board_init, suggesting that the init code change it if it needs changing -- but then leave thepage_is_4KiBstuff in the generic code.Quoted 2 lines
This should probably just be filled in by the init code without anything here.
Quoted 6 lines
No different. basically I assume OneNAND has 2KiB pagesize and In special case, MLC, and 4KiB pagesize OneNAND set the 4KiB pagesize.
If you want to leave as before. no problem.
Please consider the code size
It seems to me that just having the replaceable init function fill in the page size would be smaller than having non-replaceable code that passes a pointer in and then decides on the basis what was written there.
Quoted 1 line
and don't want to break exsiting board support.
I don't see how this additional bit of refactoring would put other boards at higher risk of breaking that what you're already doing. For any board that doesn't override onenand_board_init (and since that's a new capability, there should not be any that do yet), you're just moving something from one part of generic code to another.
-Scott
PATCH 1/1 OneNAND board init support
Quoted 7 lines · Scott Wood
Quoted 6 lines · Kyungmin Park
Quoted 5 lines · Scott Wood
Quoted 3 lines · Kyungmin Park
?#define
READ_INTERRUPT()? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
- ? ? onenand_readw(THIS_ONENAND(ONENAND_REG_INTERRUPT))
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?onenand_readw(ONENAND_REG_INTERRUPT)
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
You could get rid of the newline now...
It exceeds the 80 columns.
No it doesn't, anymore. ?You'll note that the start of
onenand_readw()in to the right of the end ofREAD_INTERRUPT, so you're not saving any horizontal space with the newline.
Right, change it.
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
onenand_ipl/onenand_ipl.h line 37
Quoted 5 lines
onenand_ipl/onenand_ipl.h +4
-0+enum { + ? ? ONENAND_USE_DEFAULT, + ? ? ONENAND_USE_GENERIC, +};What is this? ?Add a comment, and possibly more specific names.
I see redefine the specific names and comments.
At this time no need to it. remove it.
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 4 lines
+extern int (*onenand_read_page)(ulong block, ulong page,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?u_char*buf, int pagesize);Maybe use a weak function instead? ?Or an #ifdef
CONFIG_SYS_ONENAND_BOARD_READ_PAGEthat will keep the code for the generic version from being in the image (it'd be nice if we could optimize out replaced weak functions). ?It seems especially odd that you use one method for init and another for read page.
Quoted 3 lines
Quoted 1 line
I tried to use weak function but it produces more than expected.
More than compiling both functions and choosing with a function pointer?
Weak function itself. when using the weak function. it create more than function pointer.
Quoted 5 lines
Quoted 3 lines
as you know it got size limitation. When use the weak function. the apollon board will be broken.
Broken how? ?Size?
Yes, I'm under pressure of size limitation.
Quoted 11 lines
Quoted 5 lines
and I don't want to use #ifdef. since Now we support two different CPUs, s5pc100, s5pc110. these accesses different way. s5pc100 use own OneNAND controller. but s5pc110 use generic OneNAND method. That's reason to define the function pointer.
Function pointers make sense if you want to override on a per-device basis (i.e. multiple controller types in the same system) or dynamically (different hardware handled by the same binary). ?Are you trying to generate one image that works on both s5pc100 and s5pc110? ?That sounds pretty luxurious for a space-constrained NAND bootstrap. :-)
Now I used the two different CPUs. s5pc100 and s5pc110. also these cpus have different interface. Own controller and generic, respectively. Of course Now it supports both at one image.
That's reason to introduce the onenand_board_init at onenand IPL. Right. it's some luxurious but need to maintain several boards. It requires it.
Quoted 55 lines
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
?/* read a page with ECC */
onenand_ipl/onenand_read.c +1
-1-static inline int onenand_read_page(ulong block, ulong page, +static int generic_onenand_read_page(ulong block, ulong page,? ? ? ? ? ? ? ? ? ? ? ? ? ? ?
u_char* buf, int pagesize)Is the "generic" code really generic, or is it just one specific controller?
Quoted 3 lines
The 'generic' means the original OneNAND access method. Use NOR interface and use OneNAND registers.
Most and Most generic method.OK, good; it was never clear to me just which hardware the existing onenand code has been targeting; I had gotten the impression that it was for one of the chips with a custom controller.
Better, though, would be to still have good separation between that implementation and the infrastructure that is truly generic even when you have a special controller. ?This is something I don't like about the current NAND code.
Quoted 22 lines
Quoted 19 lines
Quoted 9 lines
+ ? ?
onenand_board_init(&page_is_4KiB, &page);(quoted diff) +5
-2+#else + ? ? onenand_generic_init(&page_is_4KiB, &page); +#endif - ? ? if (onenand_readw(ONENAND_REG_TECHNOLOGY)) { - ? ? ? ? ? ? pagesize = 4096; /* MLC OneNAND has 4KiB pagesize */ + ? ? if (page_is_4KiB) { + ? ? ? ? ? ? pagesize = 4096; /* OneNAND has 4KiB pagesize */Quoted 5 lines
? ? ? ? ? ? ?
erase_shift= 18;
- ? ? } else {
- ? ? ? ? ? ? pagesize = 2048;
- ? ? ? ? ? ?erase_shift= 17;
? ? ? }I don't understand why you move the pagesize/erase_shift init before
onenand_board_init, suggesting that the init code change it if it needs changing -- but then leave thepage_is_4KiBstuff in the generic code.Quoted 2 lines
This should probably just be filled in by the init code without anything here.
Quoted 6 lines
No different. basically I assume OneNAND has 2KiB pagesize and In special case, MLC, and 4KiB pagesize OneNAND set the 4KiB pagesize.
If you want to leave as before. no problem.
Please consider the code size
It seems to me that just having the replaceable init function fill in the page size would be smaller than having non-replaceable code that passes a pointer in and then decides on the basis what was written there.
Quoted 7 lines
Quoted 1 line
and don't want to break exsiting board support.
I don't see how this additional bit of refactoring would put other boards at higher risk of breaking that what you're already doing. ?For any board that doesn't override
onenand_board_init(and since that's a new capability, there should not be any that do yet), you're just moving something from one part of generic code to another.
At this time I just refactoring the code and then I will add board specific patch later.
Thank you,
Kyungmin Park