Patchwork [U-Boot] Decreases code size of the nand_spl

login
register
mail settings
Submitter Alex
Date May 4, 2011, 1:10 p.m.
Message ID <4DC15037.70109@dawning.com>
Download mbox | patch
Permalink /patch/94037/
State Accepted
Commit 8370978318337df3979aa0852529d53d77ed736e
Headers show

Comments

Alex - May 4, 2011, 1:10 p.m.
From b59f1e5b0bc3684615756c12fd5c5f9fcaa4c812 Mon Sep 17 00:00:00 2001
From: Alex Waterman <awaterman@dawning.com>
Date: Tue, 3 May 2011 15:00:23 -0400
Subject: [PATCH] Decreases code size of the nand_spl

The canyonland boards nand_spl size is just under the maximum 4KByte size. This
patch decreases the size of the nand_spl to make a previous commit - commit
65a9db7be0868be91ba81b9b5bf821de82e6d9b0 - fit in the nand_spl.

Signed-off-by: Alex Waterman <awaterman@dawning.com>
---
This patch uses a function pointer declared as a local variable; checkpatch didn't
mind but this seems like it could be (stylistically) a very bad idea. Any thoughts?

 nand_spl/nand_boot.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)
Stefan Roese - May 4, 2011, 1:47 p.m.
Hi Alex,

On Wednesday 04 May 2011 15:10:15 Alex Waterman wrote:
> From b59f1e5b0bc3684615756c12fd5c5f9fcaa4c812 Mon Sep 17 00:00:00 2001
> From: Alex Waterman <awaterman@dawning.com>
> Date: Tue, 3 May 2011 15:00:23 -0400
> Subject: [PATCH] Decreases code size of the nand_spl
> 
> The canyonland boards nand_spl size is just under the maximum 4KByte size.
> This patch decreases the size of the nand_spl to make a previous commit -
> commit 65a9db7be0868be91ba81b9b5bf821de82e6d9b0 - fit in the nand_spl.
> 
> Signed-off-by: Alex Waterman <awaterman@dawning.com>
> ---
> This patch uses a function pointer declared as a local variable; checkpatch
> didn't mind but this seems like it could be (stylistically) a very bad
> idea. Any thoughts?

Please see my patches sent a few hours ago:

"nand_spl: nand_boot.c: Init nand_chip.options to 0"
"nand_spl: nand_boot.c: Remove CONFIG_SYS_NAND_READ_DELAY"

The 2nd patch fixes the size problem as well. So no need for your patch any 
more.

But thanks anyway.

BTW: What platform/SoC are you using?

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
Alex - May 4, 2011, 2:03 p.m.
Stefan,

> The 2nd patch fixes the size problem as well. So no need for your patch any 
> more.

Ahh, I saw the first but missed the second I guess. 

> BTW: What platform/SoC are you using?

I am using a custom board based on the AMCC sequoia board. At some point I
plan to try and mainline our port for that board if possible. But I have
more work to go on that.

- Alex
Scott Wood - May 4, 2011, 5:46 p.m.
On Wed, 4 May 2011 15:47:27 +0200
Stefan Roese <sr@denx.de> wrote:

> Hi Alex,
> 
> On Wednesday 04 May 2011 15:10:15 Alex Waterman wrote:
> > From b59f1e5b0bc3684615756c12fd5c5f9fcaa4c812 Mon Sep 17 00:00:00 2001
> > From: Alex Waterman <awaterman@dawning.com>
> > Date: Tue, 3 May 2011 15:00:23 -0400
> > Subject: [PATCH] Decreases code size of the nand_spl
> > 
> > The canyonland boards nand_spl size is just under the maximum 4KByte size.
> > This patch decreases the size of the nand_spl to make a previous commit -
> > commit 65a9db7be0868be91ba81b9b5bf821de82e6d9b0 - fit in the nand_spl.
> > 
> > Signed-off-by: Alex Waterman <awaterman@dawning.com>
> > ---
> > This patch uses a function pointer declared as a local variable; checkpatch
> > didn't mind but this seems like it could be (stylistically) a very bad
> > idea. Any thoughts?

I don't see what's wrong with a local function pointer.

> Please see my patches sent a few hours ago:
> 
> "nand_spl: nand_boot.c: Init nand_chip.options to 0"
> "nand_spl: nand_boot.c: Remove CONFIG_SYS_NAND_READ_DELAY"
> 
> The 2nd patch fixes the size problem as well. So no need for your patch any 
> more.

Or we could apply both and save even more space, delaying the next time we
run into trouble. :-)

-Scott
Alex - May 4, 2011, 6:24 p.m.
Scott,

> Or we could apply both and save even more space, delaying the next time we
> run into trouble. :-)

Since the spl is so limited in space, would it be worth making every function
in the spl use a local function pointer instead of lots of dereferences?

- Alex
Scott Wood - May 4, 2011, 6:43 p.m.
On Wed, 4 May 2011 14:24:15 -0400
Alex Waterman <awaterman@dawning.com> wrote:

> 
> Scott,
> 
> > Or we could apply both and save even more space, delaying the next time we
> > run into trouble. :-)
> 
> Since the spl is so limited in space, would it be worth making every function
> in the spl use a local function pointer instead of lots of dereferences?

It'll only help when the same pointer is called more than once in the
function, but I'd be fine with doing it in those cases.

-Scott
Scott Wood - May 13, 2011, 4:16 p.m.
On Wed, May 04, 2011 at 09:10:15AM -0400, Alex Waterman wrote:
> From b59f1e5b0bc3684615756c12fd5c5f9fcaa4c812 Mon Sep 17 00:00:00 2001
> From: Alex Waterman <awaterman@dawning.com>
> Date: Tue, 3 May 2011 15:00:23 -0400
> Subject: [PATCH] Decreases code size of the nand_spl
> 
> The canyonland boards nand_spl size is just under the maximum 4KByte size. This
> patch decreases the size of the nand_spl to make a previous commit - commit
> 65a9db7be0868be91ba81b9b5bf821de82e6d9b0 - fit in the nand_spl.
> 
> Signed-off-by: Alex Waterman <awaterman@dawning.com>
> ---
> This patch uses a function pointer declared as a local variable; checkpatch didn't
> mind but this seems like it could be (stylistically) a very bad idea. Any thoughts?
> 
>  nand_spl/nand_boot.c |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)

Applied to u-boot-nand-flash

-Scott

Patch

diff --git a/nand_spl/nand_boot.c b/nand_spl/nand_boot.c
index 4a96878..79048b3 100644
--- a/nand_spl/nand_boot.c
+++ b/nand_spl/nand_boot.c
@@ -77,6 +77,8 @@  static int nand_command(struct mtd_info *mtd, int block, int page, int offs, u8
 {
 	struct nand_chip *this = mtd->priv;
 	int page_addr = page + block * CONFIG_SYS_NAND_PAGE_COUNT;
+	void (*hwctrl)(struct mtd_info *mtd, int cmd,
+			unsigned int ctrl) = this->cmd_ctrl;
 
 	if (this->dev_ready)
 		while (!this->dev_ready(mtd))
@@ -95,25 +97,25 @@  static int nand_command(struct mtd_info *mtd, int block, int page, int offs, u8
 		offs >>= 1;
 
 	/* Begin command latch cycle */
-	this->cmd_ctrl(mtd, cmd, NAND_CTRL_CLE | NAND_CTRL_CHANGE);
+	hwctrl(mtd, cmd, NAND_CTRL_CLE | NAND_CTRL_CHANGE);
 	/* Set ALE and clear CLE to start address cycle */
 	/* Column address */
-	this->cmd_ctrl(mtd, offs & 0xff,
+	hwctrl(mtd, offs & 0xff,
 		       NAND_CTRL_ALE | NAND_CTRL_CHANGE); /* A[7:0] */
-	this->cmd_ctrl(mtd, (offs >> 8) & 0xff, NAND_CTRL_ALE); /* A[11:9] */
+	hwctrl(mtd, (offs >> 8) & 0xff, NAND_CTRL_ALE); /* A[11:9] */
 	/* Row address */
-	this->cmd_ctrl(mtd, (page_addr & 0xff), NAND_CTRL_ALE); /* A[19:12] */
-	this->cmd_ctrl(mtd, ((page_addr >> 8) & 0xff),
+	hwctrl(mtd, (page_addr & 0xff), NAND_CTRL_ALE); /* A[19:12] */
+	hwctrl(mtd, ((page_addr >> 8) & 0xff),
 		       NAND_CTRL_ALE); /* A[27:20] */
 #ifdef CONFIG_SYS_NAND_5_ADDR_CYCLE
 	/* One more address cycle for devices > 128MiB */
-	this->cmd_ctrl(mtd, (page_addr >> 16) & 0x0f,
+	hwctrl(mtd, (page_addr >> 16) & 0x0f,
 		       NAND_CTRL_ALE); /* A[31:28] */
 #endif
 	/* Latch in address */
-	this->cmd_ctrl(mtd, NAND_CMD_READSTART,
+	hwctrl(mtd, NAND_CMD_READSTART,
 		       NAND_CTRL_CLE | NAND_CTRL_CHANGE);
-	this->cmd_ctrl(mtd, NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE);
+	hwctrl(mtd, NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE);
 
 	/*
 	 * Wait a while for the data to be ready