Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
Hi Marek,
On 14-09-2012 19:21, Marek Vasut wrote:
Dear José Miguel Gonçalves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: José Miguel Gonçalves jose.goncal...@inov.pt
[...]
+#include common.h
+#include nand.h
+#include asm/io.h
+#include asm/arch/s3c24xx_cpu.h
+#include asm/errno.h
+
+#define MAX_CHIPS 2
+static int nand_cs[MAX_CHIPS] = { 0, 1 };
+
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
This doesn't seem quite right ...
1) this should be in CPU directory
2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
3) should be inline function, not a macro
I'm having difficulties in following this suggestion. No problem if I migrate the
printf as a macro to a header in the CPU directory. The problem is when I try to
put it as an inline function. In this case if I define it like this;
#ifdef CONFIG_SPL_BUILD
static inline int printf(const char *fmt, ...)
{
return 0;
}
#endif
I will get an static declaration of `printf' follows non-static declaration
error due to the printf() declaration in common.h.
If I put this inline printf function (without the static) in a .c file in the CPU
directory, it will compile but, as I expected, it will not be inlined, but it will
be compiled as a normal function.
Can you detail on how to do this?
Best regards,
José Gonçalves
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On Sun, Sep 16, 2012 at 10:16:47AM +0100, Jos? Miguel Gon?alves wrote:
On 09/14/2012 08:01 PM, Tom Rini wrote:
On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos? Miguel Gon?alves wrote:
On 14-09-2012 19:21, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves jose.goncal...@inov.pt
[...]
+#include common.h
+#include nand.h
+#include asm/io.h
+#include asm/arch/s3c24xx_cpu.h
+#include asm/errno.h
+
+#define MAX_CHIPS2
+static int nand_cs[MAX_CHIPS] = { 0, 1 };
+
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
This doesn't seem quite right ...
1) this should be in CPU directory
2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
3) should be inline function, not a macro
1) and 3) OK.
Don't quite understand 2). I want to remove the printfs in the SPL
build, as it would blown up the internal SoC RAM space available.
So why add a condition with CONFIG_SPL_SERIAL_SUPPORT?
You've got 8KB, based on the final patch in the series. At least in my
SPL series that's still enough to get you printf/puts (I believe 4kb was
the cutoff where that had to be dropped).
Barely:
$ size u-boot-spl
text databssdechexfilename
3337 8588 3933f5du-boot-spl
$ size u-boot-spl-printf
text databssdechexfilename
7968 8604 8580 2184 u-boot-spl-printf
The printf is not so important that justifies exhausting the IRAM
space available and preventing any future SPL expansion...
There's two parts to this:
- What else can you do in a single binary, in theory? Is there boot
medium detection and you would want to have, for example, NAND and SD
support in the same binary? I would say memory is meant for using,
but this is a board maintainer decision and that's you :)
- We have a define today (CONFIG_SPL_LIBCOMMON_SUPPORT) that toggles
printf or no printf. If we really need to say yes to
LIBCOMMON_SUPPORT and no to printf, we need finer grained config
options and then a do-nothing printf is used for SPL. Doing the
opt-out driver by driver just punts this problem down the road to the
next developer and that's not very nice (and adding
CONFIG_SPL_PRINTF_SUPPORT shouldn't be a big patch, modify a few
Makefiles, update a bunch of config files, add
common/spl/dummy_funcs.c and a __weak printf).
--
Tom
signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On 09/17/2012 11:57:57 AM, Tom Rini wrote:
On Sun, Sep 16, 2012 at 10:16:47AM +0100, Jos? Miguel Gon?alves wrote:
On 09/14/2012 08:01 PM, Tom Rini wrote:
On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos? Miguel Gon?alves
wrote:
On 14-09-2012 19:21, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves jose.goncal...@inov.pt
[...]
+#include common.h
+#include nand.h
+#include asm/io.h
+#include asm/arch/s3c24xx_cpu.h
+#include asm/errno.h
+
+#define MAX_CHIPS 2
+static int nand_cs[MAX_CHIPS] = { 0, 1 };
+
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
This doesn't seem quite right ...
1) this should be in CPU directory
2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
3) should be inline function, not a macro
1) and 3) OK.
Don't quite understand 2). I want to remove the printfs in the SPL
build, as it would blown up the internal SoC RAM space available.
So why add a condition with CONFIG_SPL_SERIAL_SUPPORT?
You've got 8KB, based on the final patch in the series. At least
in my
SPL series that's still enough to get you printf/puts (I believe
4kb was
the cutoff where that had to be dropped).
Barely:
$ size u-boot-spl
text databssdechexfilename
3337 8588 3933f5du-boot-spl
$ size u-boot-spl-printf
text databssdechexfilename
7968 8604 8580 2184
u-boot-spl-printf
The printf is not so important that justifies exhausting the IRAM
space available and preventing any future SPL expansion...
There's two parts to this:
- What else can you do in a single binary, in theory? Is there boot
medium detection and you would want to have, for example, NAND and
SD
support in the same binary? I would say memory is meant for using,
but this is a board maintainer decision and that's you :)
- We have a define today (CONFIG_SPL_LIBCOMMON_SUPPORT) that toggles
printf or no printf. If we really need to say yes to
LIBCOMMON_SUPPORT and no to printf, we need finer grained config
options and then a do-nothing printf is used for SPL. Doing the
opt-out driver by driver just punts this problem down the road to
the
next developer and that's not very nice (and adding
CONFIG_SPL_PRINTF_SUPPORT shouldn't be a big patch, modify a few
Makefiles, update a bunch of config files, add
common/spl/dummy_funcs.c and a __weak printf).
Weak symbols are not OK for configuring printf out of the SPL, as
you'll still have all the format strings and caller code in the
binary. It should be a macro (or an inline function that replaces the
standard printf declaration), but it should be in a system header (not
the CPU directory -- not sure what Marek meant there) and be based on
an appropriate CONFIG symbol.
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On 17-09-2012 17:57, Tom Rini wrote:
On Sun, Sep 16, 2012 at 10:16:47AM +0100, Jos? Miguel Gon?alves wrote:
On 09/14/2012 08:01 PM, Tom Rini wrote:
On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos? Miguel Gon?alves wrote:
On 14-09-2012 19:21, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves jose.goncal...@inov.pt
[...]
+#include common.h
+#include nand.h
+#include asm/io.h
+#include asm/arch/s3c24xx_cpu.h
+#include asm/errno.h
+
+#define MAX_CHIPS 2
+static int nand_cs[MAX_CHIPS] = { 0, 1 };
+
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
This doesn't seem quite right ...
1) this should be in CPU directory
2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
3) should be inline function, not a macro
1) and 3) OK.
Don't quite understand 2). I want to remove the printfs in the SPL
build, as it would blown up the internal SoC RAM space available.
So why add a condition with CONFIG_SPL_SERIAL_SUPPORT?
You've got 8KB, based on the final patch in the series. At least in my
SPL series that's still enough to get you printf/puts (I believe 4kb was
the cutoff where that had to be dropped).
Barely:
$ size u-boot-spl
text databssdechexfilename
3337 8588 3933f5du-boot-spl
$ size u-boot-spl-printf
text databssdechexfilename
7968 8604 8580 2184 u-boot-spl-printf
The printf is not so important that justifies exhausting the IRAM
space available and preventing any future SPL expansion...
There's two parts to this:
- What else can you do in a single binary, in theory? Is there boot
medium detection and you would want to have, for example, NAND and SD
support in the same binary? I would say memory is meant for using,
but this is a board maintainer decision and that's you :)
That's exactly what I've got in mind when I talked about a future expansion! Being
able to boot also from an SD card.
With only 8KB for .text and .data, I can not use printfs in the SPL for this
platform (at least with the present printf support for SPL).
- We have a define today (CONFIG_SPL_LIBCOMMON_SUPPORT) that toggles
printf or no printf. If we really need to say yes to
LIBCOMMON_SUPPORT and no to printf, we need finer grained config
options and then a do-nothing printf is used for SPL. Doing the
opt-out driver by driver just punts this problem down the road to the
next developer and that's not very nice (and adding
CONFIG_SPL_PRINTF_SUPPORT shouldn't be a big patch, modify a few
Makefiles, update a bunch of config files, add
common/spl/dummy_funcs.c and a __weak printf).
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On 09/17/12 10:03, Scott Wood wrote:
On 09/17/2012 11:57:57 AM, Tom Rini wrote:
On Sun, Sep 16, 2012 at 10:16:47AM +0100, Jos? Miguel Gon?alves wrote:
On 09/14/2012 08:01 PM, Tom Rini wrote:
On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos? Miguel Gon?alves wrote:
On 14-09-2012 19:21, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves jose.goncal...@inov.pt
[...]
+#include common.h
+#include nand.h
+#include asm/io.h
+#include asm/arch/s3c24xx_cpu.h
+#include asm/errno.h
+
+#define MAX_CHIPS2
+static int nand_cs[MAX_CHIPS] = { 0, 1 };
+
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
This doesn't seem quite right ...
1) this should be in CPU directory
2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
3) should be inline function, not a macro
1) and 3) OK.
Don't quite understand 2). I want to remove the printfs in the SPL
build, as it would blown up the internal SoC RAM space available.
So why add a condition with CONFIG_SPL_SERIAL_SUPPORT?
You've got 8KB, based on the final patch in the series. At least
in my
SPL series that's still enough to get you printf/puts (I believe
4kb was
the cutoff where that had to be dropped).
Barely:
$ size u-boot-spl
text databssdechexfilename
3337 8588 3933f5du-boot-spl
$ size u-boot-spl-printf
text databssdechexfilename
7968 8604 8580 2184 u-boot-spl-printf
The printf is not so important that justifies exhausting the IRAM
space available and preventing any future SPL expansion...
There's two parts to this:
- What else can you do in a single binary, in theory? Is there boot
medium detection and you would want to have, for example, NAND and SD
support in the same binary? I would say memory is meant for using,
but this is a board maintainer decision and that's you :)
- We have a define today (CONFIG_SPL_LIBCOMMON_SUPPORT) that toggles
printf or no printf. If we really need to say yes to
LIBCOMMON_SUPPORT and no to printf, we need finer grained config
options and then a do-nothing printf is used for SPL. Doing the
opt-out driver by driver just punts this problem down the road to the
next developer and that's not very nice (and adding
CONFIG_SPL_PRINTF_SUPPORT shouldn't be a big patch, modify a few
Makefiles, update a bunch of config files, add
common/spl/dummy_funcs.c and a __weak printf).
Weak symbols are not OK for configuring printf out of the SPL, as you'll
still have all the format strings and caller code in the binary. It
should be a macro (or an inline function that replaces the standard
printf declaration), but it should be in a system header (not the CPU
directory -- not sure what Marek meant there) and be based on an
appropriate CONFIG symbol.
I'm a little leery of adding #if ... into common.h around printf. I'd
like to not worry about the branch/return bytes until we really really
have to again but yes, the strings are more of a concern since they
won't be collected out. Just top of my head thinking above.
--
Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On 09/17/2012 12:08:17 PM, Tom Rini wrote: On 09/17/12 10:03, Scott Wood wrote: Weak symbols are not OK for configuring printf out of the SPL, as you'll still have all the format strings and caller code in the binary. It should be a macro (or an inline function that replaces the standard printf declaration), but it should be in a system header (not the CPU directory -- not sure what Marek meant there) and be based on an appropriate CONFIG symbol. I'm a little leery of adding #if ... into common.h around printf. I'd like to not worry about the branch/return bytes until we really really have to again but yes, the strings are more of a concern since they won't be collected out. Just top of my head thinking above. Caller code won't be collected either. It's not just branch/return bytes but argument preparation -- possibly significant chunks of code to calculate values to be displayed, that might otherwise be optimized out. -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 09/17/12 10:08, José Miguel Gonçalves wrote:
On 17-09-2012 17:57, Tom Rini wrote:
On Sun, Sep 16, 2012 at 10:16:47AM +0100, Jos? Miguel Gon?alves
wrote:
On 09/14/2012 08:01 PM, Tom Rini wrote:
On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos? Miguel
Gon?alves wrote:
On 14-09-2012 19:21, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves
jose.goncal...@inov.pt
[...]
+#include common.h +#include nand.h +#include
asm/io.h +#include asm/arch/s3c24xx_cpu.h +#include
asm/errno.h + +#define MAX_CHIPS2 +static int
nand_cs[MAX_CHIPS] = { 0, 1 }; + +#ifdef
CONFIG_SPL_BUILD +#define printf(arg...) do {} while
(0)
This doesn't seem quite right ...
1) this should be in CPU directory 2) should be enabled
only if CONFIG_SPL_SERIAL_SUPPORT is not set 3) should be
inline function, not a macro
1) and 3) OK. Don't quite understand 2). I want to remove
the printfs in the SPL build, as it would blown up the
internal SoC RAM space available. So why add a condition
with CONFIG_SPL_SERIAL_SUPPORT?
You've got 8KB, based on the final patch in the series. At
least in my SPL series that's still enough to get you
printf/puts (I believe 4kb was the cutoff where that had to
be dropped).
Barely:
$ size u-boot-spl text databssdec
hexfilename 3337 8588 3933
f5du-boot-spl
$ size u-boot-spl-printf text databssdec
hexfilename 7968 8604 8580
2184 u-boot-spl-printf
The printf is not so important that justifies exhausting the
IRAM space available and preventing any future SPL
expansion...
There's two parts to this: - What else can you do in a single
binary, in theory? Is there boot medium detection and you would
want to have, for example, NAND and SD support in the same
binary? I would say memory is meant for using, but this is a
board maintainer decision and that's you :)
That's exactly what I've got in mind when I talked about a future
expansion! Being able to boot also from an SD card. With only 8KB
for .text and .data, I can not use printfs in the SPL for this
platform (at least with the present printf support for SPL).
- We have a define today (CONFIG_SPL_LIBCOMMON_SUPPORT) that
toggles printf or no printf. If we really need to say yes to
LIBCOMMON_SUPPORT and no to printf, we need finer grained config
options and then a do-nothing printf is used for SPL. Doing the
opt-out driver by driver just punts this problem down the road to
the next developer and that's not very nice (and adding
CONFIG_SPL_PRINTF_SUPPORT shouldn't be a big patch, modify a few
Makefiles, update a bunch of config files, add
common/spl/dummy_funcs.c and a __weak printf).
OK, so please take a stab at option two, on top of my SPL series,
keeping in mind what Scott has said (which makes sense) because
otherwise you'll be changing a lot of MMC files too to drop out printf :)
- --
Tom
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/
iQIcBAEBAgAGBQJQV2Q+AAoJENk4IS6UOR1W0VMP/1ToNzW5XmNApUGYIZi1d779
hJ6MieZoiTHOCnrHiRMMMAfOnPNnt+63TozmXWkhNhlPVRCs1Irx8nCpzMabjevr
ZENjnewtKsvgBsICak5rSQLbfyQBUG8tL3iBMPnYwyNhDf8CgED6rNCnxhV8Lr7j
o0gELaNHPRD7bJwglXo3TN0BxNtTrww3uSArSYh4WMVaOP908Mk7p7y8qEVSvNeh
BrdbVZK1oPmhlke9EUXfCQYqn/JdJ7mtdW1q5PdST7GFtnLZmqpj+FuOEwN3OioA
DE6J461Aqr95mGVUnUr7PxglytL6zLKJcE8YpIhu9nL6r9QRg0wm4HIKr3uTKLl5
WBs4ziJsLtm5IAZ7xg2FOsPrCkrAiL3bVQg0+7xVc1cVzKIGmtMR6oGVS+nI38Q6
lzmz0AQSuobeLkiP4+tL8C7kFkwMcj9I5LByN968ZMvbTftecIsdgYSkluOdGU5w
dwKPjplU4t8yy6d1TXbh0Xdw7q8cBZ3bjqbAKi3uo9BpEHgCDOwHp7oOTuJTDB/7
C6WxXHdQFHh7m0hxf1zkawOeh+oqd5MHAxjlckQ/zmg5UsmNWDA6RmmYgWrOBw9m
jCvN/lhe1soBnxaz2byUKwEkPIDwmBl+JgtSL7DMZ7bLfS59daXiaMNac2JPstG3
j0lBvj7SCVnQrE6SJxxa
=71I1
-END PGP SIGNATURE-
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On 17-09-2012 18:56, Tom Rini wrote:
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 09/17/12 10:08, José Miguel Gonçalves wrote:
On 17-09-2012 17:57, Tom Rini wrote:
On Sun, Sep 16, 2012 at 10:16:47AM +0100, Jos? Miguel Gon?alves
wrote:
On 09/14/2012 08:01 PM, Tom Rini wrote:
On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos? Miguel
Gon?alves wrote:
On 14-09-2012 19:21, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves
jose.goncal...@inov.pt
[...]
+#include common.h +#include nand.h +#include
asm/io.h +#include asm/arch/s3c24xx_cpu.h +#include
asm/errno.h + +#define MAX_CHIPS2 +static int
nand_cs[MAX_CHIPS] = { 0, 1 }; + +#ifdef
CONFIG_SPL_BUILD +#define printf(arg...) do {} while
(0)
This doesn't seem quite right ...
1) this should be in CPU directory 2) should be enabled
only if CONFIG_SPL_SERIAL_SUPPORT is not set 3) should be
inline function, not a macro
1) and 3) OK. Don't quite understand 2). I want to remove
the printfs in the SPL build, as it would blown up the
internal SoC RAM space available. So why add a condition
with CONFIG_SPL_SERIAL_SUPPORT?
You've got 8KB, based on the final patch in the series. At
least in my SPL series that's still enough to get you
printf/puts (I believe 4kb was the cutoff where that had to
be dropped).
Barely:
$ size u-boot-spl text databssdec
hexfilename 3337 8588 3933
f5du-boot-spl
$ size u-boot-spl-printf text databssdec
hexfilename 7968 8604 8580
2184 u-boot-spl-printf
The printf is not so important that justifies exhausting the
IRAM space available and preventing any future SPL
expansion...
There's two parts to this: - What else can you do in a single
binary, in theory? Is there boot medium detection and you would
want to have, for example, NAND and SD support in the same
binary? I would say memory is meant for using, but this is a
board maintainer decision and that's you :)
That's exactly what I've got in mind when I talked about a future
expansion! Being able to boot also from an SD card. With only 8KB
for .text and .data, I can not use printfs in the SPL for this
platform (at least with the present printf support for SPL).
- We have a define today (CONFIG_SPL_LIBCOMMON_SUPPORT) that
toggles printf or no printf. If we really need to say yes to
LIBCOMMON_SUPPORT and no to printf, we need finer grained config
options and then a do-nothing printf is used for SPL. Doing the
opt-out driver by driver just punts this problem down the road to
the next developer and that's not very nice (and adding
CONFIG_SPL_PRINTF_SUPPORT shouldn't be a big patch, modify a few
Makefiles, update a bunch of config files, add
common/spl/dummy_funcs.c and a __weak printf).
OK, so please take a stab at option two, on top of my SPL series,
keeping in mind what Scott has said (which makes sense) because
otherwise you'll be changing a lot of MMC files too to drop out printf :)
The solution that I sorted out on the current SPL framework was to add this:
#ifdef CONFIG_SPL_BUILD
#define printf(arg...) do {} while (0)
#ifdef CONFIG_SPL_SERIAL_SUPPORT
#define puts(arg) serial_puts(arg)
#endif
#endif
on a CPU specific header. Marek told me to not use macros, but to use inline
functions instead, but has I told earlier on this thread, I am unable to do that.
Suggestions for doing this in a better way are welcome...
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On Mon, Sep 17, 2012 at 07:05:48PM +0100, Jos? Miguel Gon?alves wrote:
On 17-09-2012 18:56, Tom Rini wrote:
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 09/17/12 10:08, Jos? Miguel Gon?alves wrote:
On 17-09-2012 17:57, Tom Rini wrote:
On Sun, Sep 16, 2012 at 10:16:47AM +0100, Jos? Miguel Gon?alves
wrote:
On 09/14/2012 08:01 PM, Tom Rini wrote:
On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos? Miguel
Gon?alves wrote:
On 14-09-2012 19:21, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves
jose.goncal...@inov.pt
[...]
+#include common.h +#include nand.h +#include
asm/io.h +#include asm/arch/s3c24xx_cpu.h +#include
asm/errno.h + +#define MAX_CHIPS2 +static int
nand_cs[MAX_CHIPS] = { 0, 1 }; + +#ifdef
CONFIG_SPL_BUILD +#define printf(arg...) do {} while
(0)
This doesn't seem quite right ...
1) this should be in CPU directory 2) should be enabled
only if CONFIG_SPL_SERIAL_SUPPORT is not set 3) should be
inline function, not a macro
1) and 3) OK. Don't quite understand 2). I want to remove
the printfs in the SPL build, as it would blown up the
internal SoC RAM space available. So why add a condition
with CONFIG_SPL_SERIAL_SUPPORT?
You've got 8KB, based on the final patch in the series. At
least in my SPL series that's still enough to get you
printf/puts (I believe 4kb was the cutoff where that had to
be dropped).
Barely:
$ size u-boot-spl text databssdec
hexfilename 3337 8588 3933
f5du-boot-spl
$ size u-boot-spl-printf text databssdec
hexfilename 7968 8604 8580
2184 u-boot-spl-printf
The printf is not so important that justifies exhausting the
IRAM space available and preventing any future SPL
expansion...
There's two parts to this: - What else can you do in a single
binary, in theory? Is there boot medium detection and you would
want to have, for example, NAND and SD support in the same
binary? I would say memory is meant for using, but this is a
board maintainer decision and that's you :)
That's exactly what I've got in mind when I talked about a future
expansion! Being able to boot also from an SD card. With only 8KB
for .text and .data, I can not use printfs in the SPL for this
platform (at least with the present printf support for SPL).
- We have a define today (CONFIG_SPL_LIBCOMMON_SUPPORT) that
toggles printf or no printf. If we really need to say yes to
LIBCOMMON_SUPPORT and no to printf, we need finer grained config
options and then a do-nothing printf is used for SPL. Doing the
opt-out driver by driver just punts this problem down the road to
the next developer and that's not very nice (and adding
CONFIG_SPL_PRINTF_SUPPORT shouldn't be a big patch, modify a few
Makefiles, update a bunch of config files, add
common/spl/dummy_funcs.c and a __weak printf).
OK, so please take a stab at option two, on top of my SPL series,
keeping in mind what Scott has said (which makes sense) because
otherwise you'll be changing a lot of MMC files too to drop out printf :)
The solution that I sorted out on the current SPL framework was to add this:
#ifdef CONFIG_SPL_BUILD
#define printf(arg...) do {} while (0)
#ifdef CONFIG_SPL_SERIAL_SUPPORT
#define puts(arg) serial_puts(arg)
#endif
#endif
on a CPU specific header. Marek told me to not use macros, but to
use inline functions instead, but has I told earlier on this thread,
I am unable to do that. Suggestions for doing this in a better way
are welcome...
It's gotta go in common.h, and something like
/* Big comment what / why */
#if !defined(CONFIG_SPL_BUILD) || \
(CONFIG_SPL_BUILD CONFIG_SPL_PRINTF_SUPPORT)
void putc(...);
void puts(...);
int printf();
#else
#define putc(c) serial_putc(c)
#define puts(s) serial_puts(s)
#define printf(arg...) do {} while (0)
#endif
--
Tom
signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On 17-09-2012 19:27, Tom Rini wrote:
On Mon, Sep 17, 2012 at 07:05:48PM +0100, Jos? Miguel Gon?alves wrote:
On 17-09-2012 18:56, Tom Rini wrote:
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 09/17/12 10:08, Jos? Miguel Gon?alves wrote:
On 17-09-2012 17:57, Tom Rini wrote:
On Sun, Sep 16, 2012 at 10:16:47AM +0100, Jos? Miguel Gon?alves
wrote:
On 09/14/2012 08:01 PM, Tom Rini wrote:
On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos? Miguel
Gon?alves wrote:
On 14-09-2012 19:21, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves
jose.goncal...@inov.pt
[...]
+#include common.h +#include nand.h +#include
asm/io.h +#include asm/arch/s3c24xx_cpu.h +#include
asm/errno.h + +#define MAX_CHIPS2 +static int
nand_cs[MAX_CHIPS] = { 0, 1 }; + +#ifdef
CONFIG_SPL_BUILD +#define printf(arg...) do {} while
(0)
This doesn't seem quite right ...
1) this should be in CPU directory 2) should be enabled
only if CONFIG_SPL_SERIAL_SUPPORT is not set 3) should be
inline function, not a macro
1) and 3) OK. Don't quite understand 2). I want to remove
the printfs in the SPL build, as it would blown up the
internal SoC RAM space available. So why add a condition
with CONFIG_SPL_SERIAL_SUPPORT?
You've got 8KB, based on the final patch in the series. At
least in my SPL series that's still enough to get you
printf/puts (I believe 4kb was the cutoff where that had to
be dropped).
Barely:
$ size u-boot-spl text databssdec
hexfilename 3337 8588 3933
f5du-boot-spl
$ size u-boot-spl-printf text databssdec
hexfilename 7968 8604 8580
2184 u-boot-spl-printf
The printf is not so important that justifies exhausting the
IRAM space available and preventing any future SPL
expansion...
There's two parts to this: - What else can you do in a single
binary, in theory? Is there boot medium detection and you would
want to have, for example, NAND and SD support in the same
binary? I would say memory is meant for using, but this is a
board maintainer decision and that's you :)
That's exactly what I've got in mind when I talked about a future
expansion! Being able to boot also from an SD card. With only 8KB
for .text and .data, I can not use printfs in the SPL for this
platform (at least with the present printf support for SPL).
- We have a define today (CONFIG_SPL_LIBCOMMON_SUPPORT) that
toggles printf or no printf. If we really need to say yes to
LIBCOMMON_SUPPORT and no to printf, we need finer grained config
options and then a do-nothing printf is used for SPL. Doing the
opt-out driver by driver just punts this problem down the road to
the next developer and that's not very nice (and adding
CONFIG_SPL_PRINTF_SUPPORT shouldn't be a big patch, modify a few
Makefiles, update a bunch of config files, add
common/spl/dummy_funcs.c and a __weak printf).
OK, so please take a stab at option two, on top of my SPL series,
keeping in mind what Scott has said (which makes sense) because
otherwise you'll be changing a lot of MMC files too to drop out printf :)
The solution that I sorted out on the current SPL framework was to add this:
#ifdef CONFIG_SPL_BUILD
#define printf(arg...) do {} while (0)
#ifdef CONFIG_SPL_SERIAL_SUPPORT
#define puts(arg) serial_puts(arg)
#endif
#endif
on a CPU specific header. Marek told me to not use macros, but to
use inline functions instead, but has I told earlier on this thread,
I am unable to do that. Suggestions for doing this in a better way
are welcome...
It's gotta go in common.h, and something like
/* Big comment what / why */
#if !defined(CONFIG_SPL_BUILD) || \
(CONFIG_SPL_BUILD CONFIG_SPL_PRINTF_SUPPORT)
void putc(...);
void puts(...);
int printf();
#else
#define putc(c) serial_putc(c)
#define puts(s) serial_puts(s)
#define printf(arg...) do {} while (0)
#endif
Are macros OK to remove printf() and to replace putc()/puts() by
serial_putc()/serial_puts() in the SPL?
Shouldn’t we be using inline functions instead?
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 09/17/12 11:34, José Miguel Gonçalves wrote:
On 17-09-2012 19:27, Tom Rini wrote:
On Mon, Sep 17, 2012 at 07:05:48PM +0100, Jos? Miguel Gon?alves
wrote:
On 17-09-2012 18:56, Tom Rini wrote:
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1
On 09/17/12 10:08, Jos? Miguel Gon?alves wrote:
On 17-09-2012 17:57, Tom Rini wrote:
On Sun, Sep 16, 2012 at 10:16:47AM +0100, Jos? Miguel
Gon?alves wrote:
On 09/14/2012 08:01 PM, Tom Rini wrote:
On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos?
Miguel Gon?alves wrote:
On 14-09-2012 19:21, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX
SoCs. Currently it only supports SLC NAND
chips.
Signed-off-by: Jos? Miguel Gon?alves
jose.goncal...@inov.pt
[...]
+#include common.h +#include nand.h
+#include asm/io.h +#include
asm/arch/s3c24xx_cpu.h +#include
asm/errno.h + +#define MAX_CHIPS2 +static
int nand_cs[MAX_CHIPS] = { 0, 1 }; + +#ifdef
CONFIG_SPL_BUILD +#define printf(arg...) do {}
while (0)
This doesn't seem quite right ...
1) this should be in CPU directory 2) should be
enabled only if CONFIG_SPL_SERIAL_SUPPORT is not
set 3) should be inline function, not a macro
1) and 3) OK. Don't quite understand 2). I want to
remove the printfs in the SPL build, as it would
blown up the internal SoC RAM space available. So
why add a condition with
CONFIG_SPL_SERIAL_SUPPORT?
You've got 8KB, based on the final patch in the
series. At least in my SPL series that's still
enough to get you printf/puts (I believe 4kb was the
cutoff where that had to be dropped).
Barely:
$ size u-boot-spl text databss
dec hexfilename 3337 8588
3933 f5du-boot-spl
$ size u-boot-spl-printf text databss
dec hexfilename 7968 8604
8580 2184 u-boot-spl-printf
The printf is not so important that justifies
exhausting the IRAM space available and preventing any
future SPL expansion...
There's two parts to this: - What else can you do in a
single binary, in theory? Is there boot medium detection
and you would want to have, for example, NAND and SD
support in the same binary? I would say memory is meant
for using, but this is a board maintainer decision and
that's you :)
That's exactly what I've got in mind when I talked about a
future expansion! Being able to boot also from an SD card.
With only 8KB for .text and .data, I can not use printfs in
the SPL for this platform (at least with the present printf
support for SPL).
- We have a define today (CONFIG_SPL_LIBCOMMON_SUPPORT)
that toggles printf or no printf. If we really need to
say yes to LIBCOMMON_SUPPORT and no to printf, we need
finer grained config options and then a do-nothing printf
is used for SPL. Doing the opt-out driver by driver just
punts this problem down the road to the next developer
and that's not very nice (and adding
CONFIG_SPL_PRINTF_SUPPORT shouldn't be a big patch,
modify a few Makefiles, update a bunch of config files,
add common/spl/dummy_funcs.c and a __weak printf).
OK, so please take a stab at option two, on top of my SPL
series, keeping in mind what Scott has said (which makes
sense) because otherwise you'll be changing a lot of MMC
files too to drop out printf :)
The solution that I sorted out on the current SPL framework was
to add this:
#ifdef CONFIG_SPL_BUILD #define printf(arg...) do {} while (0)
#ifdef CONFIG_SPL_SERIAL_SUPPORT #define puts(arg)
serial_puts(arg) #endif #endif
on a CPU specific header. Marek told me to not use macros, but
to use inline functions instead, but has I told earlier on this
thread, I am unable to do that. Suggestions for doing this in a
better way are welcome...
It's gotta go in common.h, and something like /* Big comment
what / why */ #if !defined(CONFIG_SPL_BUILD) || \
(CONFIG_SPL_BUILD CONFIG_SPL_PRINTF_SUPPORT) void putc(...);
void puts(...); int printf(); #else #define putc(c)
serial_putc(c) #define puts(s) serial_puts(s) #define
printf(arg...) do {} while (0) #endif
Are macros OK to remove printf() and to replace putc()/puts() by
serial_putc()/serial_puts() in the SPL? Shouldn’t we be using
inline functions instead?
As Scott pointed out, inline won't remove the string constants from
the binary so it will still be possibly too large, depending on all of
the code in question. And note the above isn't 100% right, we need a
test for SERIAL_SUPPORT in there too.
- --
Tom
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/
iQIcBAEBAgAGBQJQV3JcAAoJENk4IS6UOR1WMe0P/RlvfaE4J6ZxdIAIy77XmqGt
KKTEnJV+dTu8/a9jrMZFmZ/lGNPNrujLlphNUgGZr2W1+lriHuCgAZx1ObsMXhx9
XGRn05gshUuUtTWRCpOy3Nzo9jPF/LYrWttBiwDfOPWZ6+6G3zGPIXV3T9HHEFMM
ti0c0zCBMu1ci5RoQg4Zb6CJqJR/giBrBzegQZlL4x8t9p/GR03MSxdVPHx+NoQ7
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On 09/14/2012 08:01 PM, Tom Rini wrote:
On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos? Miguel Gon?alves wrote:
On 14-09-2012 19:21, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves jose.goncal...@inov.pt
[...]
+#include common.h
+#include nand.h
+#include asm/io.h
+#include asm/arch/s3c24xx_cpu.h
+#include asm/errno.h
+
+#define MAX_CHIPS 2
+static int nand_cs[MAX_CHIPS] = { 0, 1 };
+
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
This doesn't seem quite right ...
1) this should be in CPU directory
2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
3) should be inline function, not a macro
1) and 3) OK.
Don't quite understand 2). I want to remove the printfs in the SPL
build, as it would blown up the internal SoC RAM space available.
So why add a condition with CONFIG_SPL_SERIAL_SUPPORT?
You've got 8KB, based on the final patch in the series. At least in my
SPL series that's still enough to get you printf/puts (I believe 4kb was
the cutoff where that had to be dropped).
Barely:
$ size u-boot-spl
text databssdechexfilename
3337 8588 3933f5du-boot-spl
$ size u-boot-spl-printf
text databssdechexfilename
7968 8604 8580 2184 u-boot-spl-printf
The printf is not so important that justifies exhausting the IRAM space
available and preventing any future SPL expansion...
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: José Miguel Gonçalves jose.goncal...@inov.pt
---
Changes for v2:
- Coding style cleanup
- Use of clrsetbits_le32()
- Use of register bit macros instead of magic numbers
---
drivers/mtd/nand/Makefile |1 +
drivers/mtd/nand/s3c24xx_nand.c | 245 +++
2 files changed, 246 insertions(+)
create mode 100644 drivers/mtd/nand/s3c24xx_nand.c
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 29dc20e..791ec44 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -60,6 +60,7 @@ COBJS-$(CONFIG_NAND_MXS) += mxs_nand.o
COBJS-$(CONFIG_NAND_NDFC) += ndfc.o
COBJS-$(CONFIG_NAND_NOMADIK) += nomadik.o
COBJS-$(CONFIG_NAND_S3C2410) += s3c2410_nand.o
+COBJS-$(CONFIG_NAND_S3C24XX) += s3c24xx_nand.o
COBJS-$(CONFIG_NAND_S3C64XX) += s3c64xx.o
COBJS-$(CONFIG_NAND_SPEAR) += spr_nand.o
COBJS-$(CONFIG_NAND_OMAP_GPMC) += omap_gpmc.o
diff --git a/drivers/mtd/nand/s3c24xx_nand.c b/drivers/mtd/nand/s3c24xx_nand.c
new file mode 100644
index 000..9c0f6e2
--- /dev/null
+++ b/drivers/mtd/nand/s3c24xx_nand.c
@@ -0,0 +1,245 @@
+/*
+ * (C) Copyright 2012 INOV - INESC Inovacao
+ * Jose Goncalves jose.goncal...@inov.pt
+ *
+ * Based on drivers/mtd/nand/s3c64xx.c and U-Boot 1.3.4 from Samsung.
+ * Supports only SLC NAND Flash chips.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include common.h
+#include nand.h
+#include asm/io.h
+#include asm/arch/s3c24xx_cpu.h
+#include asm/errno.h
+
+#define MAX_CHIPS 2
+static int nand_cs[MAX_CHIPS] = { 0, 1 };
+
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
+#endif
+
+static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
+{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+ u_long nfcont;
+
+ nfcont = readl(nand-nfcont);
+
+ switch (chip) {
+ case -1:
+ nfcont |= NFCONT_NCE1 | NFCONT_NCE0;
+ break;
+ case 0:
+ nfcont = ~NFCONT_NCE0;
+ break;
+ case 1:
+ nfcont = ~NFCONT_NCE1;
+ break;
+ default:
+ return;
+ }
+
+ writel(nfcont, nand-nfcont);
+}
+
+/*
+ * Hardware specific access to control-lines function
+ */
+static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int
ctrl)
+{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+ struct nand_chip *this = mtd-priv;
+
+ if (ctrl NAND_CTRL_CHANGE) {
+ if (ctrl NAND_CLE)
+ this-IO_ADDR_W = nand-nfcmmd;
+ else if (ctrl NAND_ALE)
+ this-IO_ADDR_W = nand-nfaddr;
+ else
+ this-IO_ADDR_W = nand-nfdata;
+ if (ctrl NAND_NCE)
+ s3c_nand_select_chip(mtd, *(int *)this-priv);
+ else
+ s3c_nand_select_chip(mtd, -1);
+ }
+
+ if (cmd != NAND_CMD_NONE)
+ writeb(cmd, this-IO_ADDR_W);
+}
+
+/*
+ * Function for checking device ready pin
+ */
+static int s3c_nand_device_ready(struct mtd_info *mtdinfo)
+{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+
+ return readl(nand-nfstat) NFSTAT_RNB;
+}
+
+#ifdef CONFIG_S3C24XX_NAND_HWECC
+/*
+ * This function is called before encoding ECC codes to ready ECC engine.
+ */
+static void s3c_nand_enable_hwecc(struct mtd_info *mtd, int mode)
+{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+
+ /* Set 1-bit ECC */
+ clrsetbits_le32(nand-nfconf, NFCONF_ECCTYPE_MASK,
+ NFCONF_ECCTYPE_1BIT);
+
+ /* Initialize unlock ECC */
+ clrsetbits_le32(nand-nfcont, NFCONT_MECCLOCK, NFCONT_INITMECC);
+}
+
+/*
+ * This function is called immediately after encoding ECC codes.
+ * This function returns encoded ECC codes.
+ */
+static int s3c_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
+ u_char *ecc_code)
+{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+ u_long nfmecc0;
+
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
Dear José Miguel Gonçalves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: José Miguel Gonçalves jose.goncal...@inov.pt
[...]
+#include common.h
+#include nand.h
+#include asm/io.h
+#include asm/arch/s3c24xx_cpu.h
+#include asm/errno.h
+
+#define MAX_CHIPS2
+static int nand_cs[MAX_CHIPS] = { 0, 1 };
+
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
This doesn't seem quite right ...
1) this should be in CPU directory
2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
3) should be inline function, not a macro
+#endif
+
+static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
+{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+ u_long nfcont;
+
+ nfcont = readl(nand-nfcont);
+
+ switch (chip) {
+ case -1:
+ nfcont |= NFCONT_NCE1 | NFCONT_NCE0;
+ break;
+ case 0:
+ nfcont = ~NFCONT_NCE0;
+ break;
+ case 1:
+ nfcont = ~NFCONT_NCE1;
+ break;
+ default:
+ return;
+ }
+
+ writel(nfcont, nand-nfcont);
+}
+
+/*
+ * Hardware specific access to control-lines function
+ */
+static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int
ctrl) +{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+ struct nand_chip *this = mtd-priv;
+
+ if (ctrl NAND_CTRL_CHANGE) {
+ if (ctrl NAND_CLE)
+ this-IO_ADDR_W = nand-nfcmmd;
+ else if (ctrl NAND_ALE)
+ this-IO_ADDR_W = nand-nfaddr;
+ else
+ this-IO_ADDR_W = nand-nfdata;
Why don't you use local variable here?
+ if (ctrl NAND_NCE)
+ s3c_nand_select_chip(mtd, *(int *)this-priv);
Uh, how's this *(int *) supposed to work?
[...]
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On 14-09-2012 19:21, Marek Vasut wrote:
Dear José Miguel Gonçalves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: José Miguel Gonçalves jose.goncal...@inov.pt
[...]
+#include common.h
+#include nand.h
+#include asm/io.h
+#include asm/arch/s3c24xx_cpu.h
+#include asm/errno.h
+
+#define MAX_CHIPS 2
+static int nand_cs[MAX_CHIPS] = { 0, 1 };
+
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
This doesn't seem quite right ...
1) this should be in CPU directory
2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
3) should be inline function, not a macro
1) and 3) OK.
Don't quite understand 2). I want to remove the printfs in the SPL build, as it
would blown up the internal SoC RAM space available.
So why add a condition with CONFIG_SPL_SERIAL_SUPPORT?
+#endif
+
+static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
+{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+ u_long nfcont;
+
+ nfcont = readl(nand-nfcont);
+
+ switch (chip) {
+ case -1:
+ nfcont |= NFCONT_NCE1 | NFCONT_NCE0;
+ break;
+ case 0:
+ nfcont = ~NFCONT_NCE0;
+ break;
+ case 1:
+ nfcont = ~NFCONT_NCE1;
+ break;
+ default:
+ return;
+ }
+
+ writel(nfcont, nand-nfcont);
+}
+
+/*
+ * Hardware specific access to control-lines function
+ */
+static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int
ctrl) +{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+ struct nand_chip *this = mtd-priv;
+
+ if (ctrl NAND_CTRL_CHANGE) {
+ if (ctrl NAND_CLE)
+ this-IO_ADDR_W = nand-nfcmmd;
+ else if (ctrl NAND_ALE)
+ this-IO_ADDR_W = nand-nfaddr;
+ else
+ this-IO_ADDR_W = nand-nfdata;
Why don't you use local variable here?
Because you need to retain the NAND controller register to use between calls to
s3c_nand_hwcontrol().
If you call the function without the NAND_CTRL_CHANGE bit set in the parameter
'ctrl' you must use the register used on the last call on the next access.
+ if (ctrl NAND_NCE)
+ s3c_nand_select_chip(mtd, *(int *)this-priv);
Uh, how's this *(int *) supposed to work?
*(int *)this-priv contains an integer that is the chip id (0 or 1).
Best regards,
José Gonçalves
Best regards,
José Gonçalves
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On Fri, Sep 14, 2012 at 06:29:00PM +0100, Jos?? Miguel Gon??alves wrote:
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos?? Miguel Gon??alves jose.goncal...@inov.pt
[snip]
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
+#endif
As Marek pointed out, this isn't the right way to do this.
[snip]
+ case 2: /* Multiple error */
+ case 3: /* ECC area error */
+ printf(S3C24XX NAND: ECC uncorrectable error detected.\n);
Use puts() here.
--
Tom
signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos? Miguel Gon?alves wrote:
On 14-09-2012 19:21, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves jose.goncal...@inov.pt
[...]
+#include common.h
+#include nand.h
+#include asm/io.h
+#include asm/arch/s3c24xx_cpu.h
+#include asm/errno.h
+
+#define MAX_CHIPS 2
+static int nand_cs[MAX_CHIPS] = { 0, 1 };
+
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
This doesn't seem quite right ...
1) this should be in CPU directory
2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
3) should be inline function, not a macro
1) and 3) OK.
Don't quite understand 2). I want to remove the printfs in the SPL
build, as it would blown up the internal SoC RAM space available.
So why add a condition with CONFIG_SPL_SERIAL_SUPPORT?
You've got 8KB, based on the final patch in the series. At least in my
SPL series that's still enough to get you printf/puts (I believe 4kb was
the cutoff where that had to be dropped).
--
Tom
signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On Fri, Sep 14, 2012 at 08:21:11PM +0200, Marek Vasut wrote:
Dear José Miguel Gonçalves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: José Miguel Gonçalves jose.goncal...@inov.pt
[...]
+#include common.h
+#include nand.h
+#include asm/io.h
+#include asm/arch/s3c24xx_cpu.h
+#include asm/errno.h
+
+#define MAX_CHIPS 2
+static int nand_cs[MAX_CHIPS] = { 0, 1 };
+
+#ifdef CONFIG_SPL_BUILD
+#define printf(arg...) do {} while (0)
This doesn't seem quite right ...
1) this should be in CPU directory
2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
3) should be inline function, not a macro
What specifically should be in the CPU directory?
+#endif
+
+static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
+{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+ u_long nfcont;
+
+ nfcont = readl(nand-nfcont);
+
+ switch (chip) {
+ case -1:
+ nfcont |= NFCONT_NCE1 | NFCONT_NCE0;
+ break;
+ case 0:
+ nfcont = ~NFCONT_NCE0;
+ break;
+ case 1:
+ nfcont = ~NFCONT_NCE1;
+ break;
+ default:
+ return;
+ }
+
+ writel(nfcont, nand-nfcont);
+}
+
+/*
+ * Hardware specific access to control-lines function
+ */
+static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int
ctrl) +{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+ struct nand_chip *this = mtd-priv;
+
+ if (ctrl NAND_CTRL_CHANGE) {
+ if (ctrl NAND_CLE)
+ this-IO_ADDR_W = nand-nfcmmd;
+ else if (ctrl NAND_ALE)
+ this-IO_ADDR_W = nand-nfaddr;
+ else
+ this-IO_ADDR_W = nand-nfdata;
Why don't you use local variable here?
Because then you'll access the wrong data when the function is called
again without NAND_CTRL_CHANGE. This is a common way of writing the
hwcontrol function, though the way ndfc.c does it is simpler (you can use
a local variable if you ignore NAND_CTRL_CHANGE altogether).
+ if (ctrl NAND_NCE)
+ s3c_nand_select_chip(mtd, *(int *)this-priv);
Uh, how's this *(int *) supposed to work?
Usually driver-private data is a struct; apparently in this driver it's
an int.
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On Fri, Sep 14, 2012 at 02:24:48PM -0500, Scott Wood wrote:
On Fri, Sep 14, 2012 at 08:21:11PM +0200, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves jose.goncal...@inov.pt
[snip]
+/*
+ * Hardware specific access to control-lines function
+ */
+static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned
int
ctrl) +{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+ struct nand_chip *this = mtd-priv;
+
+ if (ctrl NAND_CTRL_CHANGE) {
+ if (ctrl NAND_CLE)
+ this-IO_ADDR_W = nand-nfcmmd;
+ else if (ctrl NAND_ALE)
+ this-IO_ADDR_W = nand-nfaddr;
+ else
+ this-IO_ADDR_W = nand-nfdata;
Why don't you use local variable here?
Because then you'll access the wrong data when the function is called
again without NAND_CTRL_CHANGE. This is a common way of writing the
hwcontrol function, though the way ndfc.c does it is simpler (you can use
a local variable if you ignore NAND_CTRL_CHANGE altogether).
+ if (ctrl NAND_NCE)
+ s3c_nand_select_chip(mtd, *(int *)this-priv);
Uh, how's this *(int *) supposed to work?
Usually driver-private data is a struct; apparently in this driver it's
an int.
Shall we take both of these comments as requests to do things
differently (struct like everyone else does, simpiler code like ndfc.c
does) unless there's good reason to not change?
--
Tom
signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
On Fri, Sep 14, 2012 at 01:20:32PM -0700, Tom Rini wrote:
On Fri, Sep 14, 2012 at 02:24:48PM -0500, Scott Wood wrote:
On Fri, Sep 14, 2012 at 08:21:11PM +0200, Marek Vasut wrote:
Dear Jos? Miguel Gon?alves,
NAND Flash driver with HW ECC for the S3C24XX SoCs.
Currently it only supports SLC NAND chips.
Signed-off-by: Jos? Miguel Gon?alves jose.goncal...@inov.pt
[snip]
+/*
+ * Hardware specific access to control-lines function
+ */
+static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned
int
ctrl) +{
+ struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
+ struct nand_chip *this = mtd-priv;
+
+ if (ctrl NAND_CTRL_CHANGE) {
+ if (ctrl NAND_CLE)
+ this-IO_ADDR_W = nand-nfcmmd;
+ else if (ctrl NAND_ALE)
+ this-IO_ADDR_W = nand-nfaddr;
+ else
+ this-IO_ADDR_W = nand-nfdata;
Why don't you use local variable here?
Because then you'll access the wrong data when the function is called
again without NAND_CTRL_CHANGE. This is a common way of writing the
hwcontrol function, though the way ndfc.c does it is simpler (you can use
a local variable if you ignore NAND_CTRL_CHANGE altogether).
+ if (ctrl NAND_NCE)
+ s3c_nand_select_chip(mtd, *(int *)this-priv);
Uh, how's this *(int *) supposed to work?
Usually driver-private data is a struct; apparently in this driver it's
an int.
Shall we take both of these comments as requests to do things
differently (struct like everyone else does, simpiler code like ndfc.c
does) unless there's good reason to not change?
Sure. :-)
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
