Subject: U-Boot Digest, Vol 9, Issue 39
From: [email-masked]
To: [email-masked]
Date: Wed, 04 Feb 2009 21:45:52 +0100
List-Id: U-Boot discussion <u-boot.lists.denx.de>

Send U-Boot mailing list submissions to
	[email-masked]

To subscribe or unsubscribe via the World Wide Web, visit
	http://lists.denx.de/mailman/listinfo/u-boot
or, via email, send a message with subject or body 'help' to
	[email-masked]

You can reach the person managing the list at
	[email-masked]

When replying, please edit your Subject line so it is more specific
than "Re: Contents of U-Boot digest..."

Today's Topics:

   1. Re: [PATCH 6/8] MPC8641HPCN: Enable CONFIG_ADDR_MAP (Jon Loeliger)
   2. [PATCH] nand_base: Add timeout for NAND reset command
      (Peter Tyser)
   3. Re: [PATCH] nand_base: Add timeout for NAND reset command
      (Scott Wood)
   4. Re: [PATCH 2/9] i.MX31: add a simple gpio driver
      (Anatolij Gustschin)
   5. Re: [PATCH 8/9] video: add a i.MX31 framebuffer driver	only
      for bitmaps so far (Magnus Lilja)
   6. Re: [PATCH] nand_base: Add timeout for NAND reset command
      (Peter Tyser)
   7. Re: [PATCH 3/9] i.MX31: support GPIO as a chip-select in the
      mxc_spi driver (Anatolij Gustschin)
   8. Re: [PATCH 3/9] i.MX31: support GPIO as a chip-select in the
      mxc_spi driver (Guennadi Liakhovetski)
   9. Re: [PATCH 2/9] i.MX31: add a simple gpio driver
      (Guennadi Liakhovetski)
  10. Re: [PATCH] nand_base: Add timeout for NAND reset command
      (Scott Wood)

----------------------------------------------------------------------

Message: 1
Date: Wed, 04 Feb 2009 13:43:11 -0600
From: Jon Loeliger <[email-masked]>
Subject: Re: [U-Boot] [PATCH 6/8] MPC8641HPCN: Enable CONFIG_ADDR_MAP
To: Becky Bruce <[email-masked]>
Cc: [email-masked]
Message-ID: <[email-masked]>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed

Becky Bruce wrote:
> Signed-off-by: Becky Bruce <[email-masked]>
> ---

Acked-by: Jon Loeliger <[email-masked]>

Assuming someone else will now pick up this series.

Thanks,
jdl

------------------------------

Message: 2
Date: Wed,  4 Feb 2009 13:47:22 -0600
From: Peter Tyser <[email-masked]>
Subject: [U-Boot] [PATCH] nand_base: Add timeout for NAND reset
	command
To: [email-masked]
Cc: [email-masked], Peter Tyser <[email-masked]>
Message-ID: <[email-masked]>

Without the timeout present an infinite loop can occur if the
NAND device is broken or not present.

Signed-off-by: Peter Tyser <[email-masked]>
---
 drivers/mtd/nand/nand_base.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index bfa7874..a2656eb 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -75,6 +75,17 @@
 #include <jffs2/jffs2.h>
 #endif
 
+/*
+ * CONFIG_SYS_NAND_RESET_CNT is used as a timeout mechanism when resetting
+ * a flash.  NAND flash is initialized prior to interrupts so standard timers
+ * can't be used.  CONFIG_SYS_NAND_RESET_CNT should be set to a value
+ * which is greater than (max NAND reset time / NAND status read time).
+ * A conservative default of 200000 (500 us / 25 ns) is used as a default.
+ */
+#ifndef CONFIG_SYS_NAND_RESET_CNT
+#define CONFIG_SYS_NAND_RESET_CNT 200000
+#endif
+
 /* Define default oob placement schemes for large and small page devices */
 static struct nand_ecclayout nand_oob_8 = {
 	.eccbytes = 3,
@@ -524,6 +535,7 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 {
 	register struct nand_chip *chip = mtd->priv;
 	int ctrl = NAND_CTRL_CLE | NAND_CTRL_CHANGE;
+	uint32_t rst_sts_cnt = CONFIG_SYS_NAND_RESET_CNT;
 
 	/*
 	 * Write out the command to the device.
@@ -590,7 +602,8 @@ static void nand_command(struct mtd_info *mtd, unsigned int command,
 			       NAND_CTRL_CLE | NAND_CTRL_CHANGE);
 		chip->cmd_ctrl(mtd,
 			       NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE);
-		while (!(chip->read_byte(mtd) & NAND_STATUS_READY)) ;
+		while (!(chip->read_byte(mtd) & NAND_STATUS_READY) &&
+			(rst_sts_cnt--));
 		return;
 
 		/* This applies to read commands */
@@ -626,6 +639,7 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 			    int column, int page_addr)
 {
 	register struct nand_chip *chip = mtd->priv;
+	uint32_t rst_sts_cnt = CONFIG_SYS_NAND_RESET_CNT;
 
 	/* Emulate NAND_CMD_READOOB */
 	if (command == NAND_CMD_READOOB) {
@@ -696,7 +710,8 @@ static void nand_command_lp(struct mtd_info *mtd, unsigned int command,
 			       NAND_NCE | NAND_CLE | NAND_CTRL_CHANGE);
 		chip->cmd_ctrl(mtd, NAND_CMD_NONE,
 			       NAND_NCE | NAND_CTRL_CHANGE);
-		while (!(chip->read_byte(mtd) & NAND_STATUS_READY)) ;
+		while (!(chip->read_byte(mtd) & NAND_STATUS_READY) &&
+			(rst_sts_cnt--));
 		return;
 
 	case NAND_CMD_RNDOUT:
-- 
1.6.0.2.GIT

------------------------------

Message: 3
Date: Wed, 04 Feb 2009 13:54:02 -0600
From: Scott Wood <[email-masked]>
Subject: Re: [U-Boot] [PATCH] nand_base: Add timeout for NAND reset
	command
To: Peter Tyser <[email-masked]>
Cc: [email-masked]
Message-ID: <[email-masked]>
Content-Type: text/plain; charset=UTF-8; format=flowed

Peter Tyser wrote:
> +/*
> + * CONFIG_SYS_NAND_RESET_CNT is used as a timeout mechanism when resetting
> + * a flash.  NAND flash is initialized prior to interrupts so standard timers
> + * can't be used.  CONFIG_SYS_NAND_RESET_CNT should be set to a value
> + * which is greater than (max NAND reset time / NAND status read time).
> + * A conservative default of 200000 (500 us / 25 ns) is used as a default.
> + */
> +#ifndef CONFIG_SYS_NAND_RESET_CNT
> +#define CONFIG_SYS_NAND_RESET_CNT 200000
> +#endif

Where does 25 ns come from?  Should the timeout be in terms of real time 
rather than iterations (we use get_ticks() for this purpose in 
fsl_elbc_nand.c)?

-Scott

------------------------------

Message: 4
Date: Wed, 04 Feb 2009 20:54:51 +0100
From: Anatolij Gustschin <[email-masked]>
Subject: Re: [U-Boot] [PATCH 2/9] i.MX31: add a simple gpio driver
To: Guennadi Liakhovetski <[email-masked]>
Cc: [email-masked]
Message-ID: <[email-masked]>
Content-Type: text/plain; charset=ISO-8859-1

Hello Guennadi,

> diff --git a/drivers/gpio/mx31_gpio.c b/drivers/gpio/mx31_gpio.c
> new file mode 100644
> index 0000000..871601f
> --- /dev/null
> +++ b/drivers/gpio/mx31_gpio.c
> @@ -0,0 +1,71 @@
> +/*
> + * Copyright (C) 2009
> + * Guennadi Liakhovetski, DEXN Software Engineering, <[email-masked]>

please fix: s/DEXN/DENX/

Best regards,
Anatolij

------------------------------

Message: 5
Date: Wed, 4 Feb 2009 21:22:39 +0100
From: Magnus Lilja <[email-masked]>
Subject: Re: [U-Boot] [PATCH 8/9] video: add a i.MX31 framebuffer
	driver	only for bitmaps so far
To: Guennadi Liakhovetski <[email-masked]>
Cc: [email-masked]
Message-ID:
	<[email-masked]>
Content-Type: text/plain; charset=ISO-8859-1

Hi

2009/2/4 Guennadi Liakhovetski <[email-masked]>:
> On Wed, 4 Feb 2009, Magnus Lilja wrote:
>
>> > +       switch (channel) {
>> > +       case IDMAC_SDC_0:
>> > +               ipu_channel_set_priority(channel, 1);
>>
>> Missing break?
>
> You don't mean inserting a break here would change anything, do you?

Not in this code but when the next developer adds a case
IDMAC_MMM_NNN: after the existing case it's easy to forget to add that
missing break.

>
>> > +       default:
>> > +               break;
>> > +       }
>
> [snip]
>
>> > +/* References in this function refer to respective Linux kernel sources */
>> > +void lcd_enable (void)
>> > +{
>> > +       u32 reg;
>> > +
>> > +       /* pcm037.c::mxc_board_init() */
>>
>> Strance comment in a generic file.
>
> "Strance"? See just 5 lines above.

Ok, I missed that comment. ("Strance" => "Strange")

Thanks, Magnus

------------------------------

Message: 6
Date: Wed, 04 Feb 2009 14:22:52 -0600
From: Peter Tyser <[email-masked]>
Subject: Re: [U-Boot] [PATCH] nand_base: Add timeout for NAND reset
	command
To: Scott Wood <[email-masked]>
Cc: [email-masked]
Message-ID: <[email-masked]>
Content-Type: text/plain

On Wed, 2009-02-04 at 13:54 -0600, Scott Wood wrote:
> Peter Tyser wrote:
> > +/*
> > + * CONFIG_SYS_NAND_RESET_CNT is used as a timeout mechanism when resetting
> > + * a flash.  NAND flash is initialized prior to interrupts so standard timers
> > + * can't be used.  CONFIG_SYS_NAND_RESET_CNT should be set to a value
> > + * which is greater than (max NAND reset time / NAND status read time).
> > + * A conservative default of 200000 (500 us / 25 ns) is used as a default.
> > + */
> > +#ifndef CONFIG_SYS_NAND_RESET_CNT
> > +#define CONFIG_SYS_NAND_RESET_CNT 200000
> > +#endif
> 
> Where does 25 ns come from?  Should the timeout be in terms of real time 
> rather than iterations (we use get_ticks() for this purpose in 
> fsl_elbc_nand.c)?

The 25ns was calculated based on the addition of trp and trhoh from the
Micron MT29F8G08 datasheet.  Based on the timing diagram for a "Read
Status" cycle I thought this would be the minimum cycle time needed to
read the chip's status.  Other chips (ST, Samsung) I glanced at had > 25
ns read status times as well.

I had tried using get_timer() (I believe nand_wait() would have been
perfect to use), but that didn't work due to interrupts being disabled
when NAND is probed.  I didn't consider using get_ticks()...  That seems
much better.  Is get_ticks() available for all platforms when NAND is
initialized?

Assuming get_ticks() is available for all platforms, would you prefer I:
1. re-do the patch using get_ticks()
2. update nand_wait() to use get_ticks instead of get_timer() and use it

Thanks,
Peter

------------------------------

Message: 7
Date: Wed, 04 Feb 2009 21:28:51 +0100
From: Anatolij Gustschin <[email-masked]>
Subject: Re: [U-Boot] [PATCH 3/9] i.MX31: support GPIO as a
	chip-select in the mxc_spi driver
To: Guennadi Liakhovetski <[email-masked]>
Cc: [email-masked]
Message-ID: <[email-masked]>
Content-Type: text/plain; charset=ISO-8859-1

Hello Guennadi,

> diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
> index 9267341..eacd36c 100644
> --- a/drivers/spi/mxc_spi.c
> +++ b/drivers/spi/mxc_spi.c
<snip>
> @@ -91,6 +94,9 @@ static u32 spi_xchg_single(struct spi_slave *slave, u32 data,
>  	struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave);
>  	unsigned int cfg_reg = reg_read(mxcs->base + MXC_CSPICTRL);
>  
> +	if (mxcs->gpio > 0 && (flags & SPI_XFER_BEGIN))
> +		mx31_gpio_set(mxcs->gpio, !!(mxcs->ctrl_reg & MXC_CSPICTRL_SSPOL));

above line to long (> 80 chars).

Also you add mx31_gpio_* calls from the new mx31 GPIO driver to
this driver unconditionally. I think, this will break linking
for imx31_litekit and mx31ads boards as these boards include the
mxc_spi driver but do not define CONFIG_MX31_GPIO. How can we
separate this? Or am I missing something?

> @@ -100,6 +106,9 @@ static u32 spi_xchg_single(struct spi_slave *slave, u32 data,
>  	while (reg_read(mxcs->base + MXC_CSPICTRL) & MXC_CSPICTRL_XCH)
>  		;
>  
> +	if (mxcs->gpio > 0 && (flags & SPI_XFER_END))
> +		mx31_gpio_set(mxcs->gpio, !(mxcs->ctrl_reg & MXC_CSPICTRL_SSPOL));

above line to long (> 80 chars).

Best regargs,
Anatolij

------------------------------

Message: 8
Date: Wed, 4 Feb 2009 21:40:09 +0100 (CET)
From: Guennadi Liakhovetski <[email-masked]>
Subject: Re: [U-Boot] [PATCH 3/9] i.MX31: support GPIO as a
	chip-select in the mxc_spi driver
To: Anatolij Gustschin <[email-masked]>
Cc: [email-masked]
Message-ID: <[email-masked]>
Content-Type: TEXT/PLAIN; charset=US-ASCII

On Wed, 4 Feb 2009, Anatolij Gustschin wrote:

> Hello Guennadi,
> 
> > diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
> > index 9267341..eacd36c 100644
> > --- a/drivers/spi/mxc_spi.c
> > +++ b/drivers/spi/mxc_spi.c
> <snip>
> > @@ -91,6 +94,9 @@ static u32 spi_xchg_single(struct spi_slave *slave, u32 data,
> >  	struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave);
> >  	unsigned int cfg_reg = reg_read(mxcs->base + MXC_CSPICTRL);
> >  
> > +	if (mxcs->gpio > 0 && (flags & SPI_XFER_BEGIN))
> > +		mx31_gpio_set(mxcs->gpio, !!(mxcs->ctrl_reg & MXC_CSPICTRL_SSPOL));
> 
> above line to long (> 80 chars).
> 
> Also you add mx31_gpio_* calls from the new mx31 GPIO driver to
> this driver unconditionally. I think, this will break linking
> for imx31_litekit and mx31ads boards as these boards include the
> mxc_spi driver but do not define CONFIG_MX31_GPIO. How can we
> separate this? Or am I missing something?
> 
> > @@ -100,6 +106,9 @@ static u32 spi_xchg_single(struct spi_slave *slave, u32 data,
> >  	while (reg_read(mxcs->base + MXC_CSPICTRL) & MXC_CSPICTRL_XCH)
> >  		;
> >  
> > +	if (mxcs->gpio > 0 && (flags & SPI_XFER_END))
> > +		mx31_gpio_set(mxcs->gpio, !(mxcs->ctrl_reg & MXC_CSPICTRL_SSPOL));
> 
> above line to long (> 80 chars).

Right, will fix all above.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

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: [email-masked]

------------------------------

Message: 9
Date: Wed, 4 Feb 2009 21:41:21 +0100 (CET)
From: Guennadi Liakhovetski <[email-masked]>
Subject: Re: [U-Boot] [PATCH 2/9] i.MX31: add a simple gpio driver
To: Anatolij Gustschin <[email-masked]>
Cc: [email-masked]
Message-ID: <[email-masked]>
Content-Type: TEXT/PLAIN; charset=US-ASCII

On Wed, 4 Feb 2009, Anatolij Gustschin wrote:

> Hello Guennadi,
> 
> > diff --git a/drivers/gpio/mx31_gpio.c b/drivers/gpio/mx31_gpio.c
> > new file mode 100644
> > index 0000000..871601f
> > --- /dev/null
> > +++ b/drivers/gpio/mx31_gpio.c
> > @@ -0,0 +1,71 @@
> > +/*
> > + * Copyright (C) 2009
> > + * Guennadi Liakhovetski, DEXN Software Engineering, <[email-masked]>
> 
> please fix: s/DEXN/DENX/

Oops, * copypaste = 4 occurrences in 3 patches:-(

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

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: [email-masked]

------------------------------

Message: 10
Date: Wed, 04 Feb 2009 14:45:11 -0600
From: Scott Wood <[email-masked]>
Subject: Re: [U-Boot] [PATCH] nand_base: Add timeout for NAND reset
	command
To: Peter Tyser <[email-masked]>
Cc: [email-masked]
Message-ID: <[email-masked]>
Content-Type: text/plain; charset=UTF-8; format=flowed

Peter Tyser wrote:
> The 25ns was calculated based on the addition of trp and trhoh from the
> Micron MT29F8G08 datasheet.  Based on the timing diagram for a "Read
> Status" cycle I thought this would be the minimum cycle time needed to
> read the chip's status.  Other chips (ST, Samsung) I glanced at had > 25
> ns read status times as well.
> 
> I had tried using get_timer() (I believe nand_wait() would have been
> perfect to use), but that didn't work due to interrupts being disabled
> when NAND is probed.  I didn't consider using get_ticks()...  That seems
> much better.  Is get_ticks() available for all platforms when NAND is
> initialized?

Probably, but who knows what weirdness is out there.

> Assuming get_ticks() is available for all platforms, would you prefer I:
> 1. re-do the patch using get_ticks()
> 2. update nand_wait() to use get_ticks instead of get_timer() and use it

#2 looks better.

-Scott

------------------------------

_______________________________________________
U-Boot mailing list
[email-masked]
http://lists.denx.de/mailman/listinfo/u-boot

End of U-Boot Digest, Vol 9, Issue 39
*************************************