[U-Boot] [PATCH] Macronix MX25xx MTD SPI flash driver

[Prafulla Wadaskar]

> -----Original Message-----
> From: Mike Frysinger [mailto:vapier at gentoo.org] 
> Sent: Friday, April 03, 2009 7:44 PM
> To: u-boot at lists.denx.de
> Cc: Prafulla Wadaskar; Ronen Shitrit
> Subject: Re: [U-Boot] [PATCH] Macronix MX25xx MTD SPI flash driver
> 
> On Friday 03 April 2009 07:49:19 Prafulla Wadaskar wrote:
> > --- a/drivers/mtd/spi/Makefile
> > +++ b/drivers/mtd/spi/Makefile
> > @@ -28,6 +28,7 @@ LIB	:= $(obj)libspi_flash.a
> >  COBJS-$(CONFIG_SPI_FLASH)	+= spi_flash.o
> >  COBJS-$(CONFIG_SPI_FLASH_ATMEL)	+= atmel.o
> >  COBJS-$(CONFIG_SPI_FLASH_STMICRO)	+= stmicro.o
> > +COBJS-$(CONFIG_SPI_FLASH_MACRONIX)	+= macronix.o
> 
> please keep the list sorted
Done..

> 
> > --- /dev/null
> > +++ b/drivers/mtd/spi/macronix.c
> > @@ -0,0 +1,319 @@
> > + * Based on drivers/mtd/spi/stmicor.c
> 
> typo in file name
Corrected..

> 
> > + * 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., 51 Franklin Street, Fifth Floor, Boston,
> > + * MA 02110-1301 USA
> > + */
> > +#include <common.h>
> 
> please add a new line there
Added..

> 
> > +/* MX25Pxx-specific commands */
> > +#define CMD_MX25PXX_WREN	0x06	/* Write Enable */
> > +#define CMD_MX25PXX_WRDI	0x04	/* Write Disable */
> > +#define CMD_MX25PXX_RDSR	0x05	/* Read Status Register */
> > +#define CMD_MX25PXX_WRSR	0x01	/* Write Status Register */
> > +#define CMD_MX25PXX_READ	0x03	/* Read Data Bytes */
> > +#define CMD_MX25PXX_FAST_READ	0x0b	/* Read Data 
> Bytes at Higher Speed */
> > +#define CMD_MX25PXX_PP		0x02	/* Page Program */
> > +#define CMD_MX25PXX_SE		0x20	/* Sector Erase */
> > +#define CMD_MX25PXX_BE		0xD8	/* Block Erase */
> > +#define CMD_MX25PXX_CE		0xc7	/* Chip Erase */
> > +#define CMD_MX25PXX_DP		0xb9	/* Deep Power-down */
> > +#define CMD_MX25PXX_RES		0xab	/* Release from 
> DP, and Read Signature */
> 
> is it really MX25PXX ?  or should it be MX25XX ?  the P looks 
> like a hold over from the stmicro driver.
You are right
It should be MX25LXX for 3.0 volt operation And it should be 
MX27VXX for 2.5 volt operation To address a common driver for 
both, I am making it MX25XX

> 
> > +struct macronix_spi_flash {
> > +	const struct macronix_spi_flash_params *params;
> > +	struct spi_flash flash;
> > +};
> 
> the spi_flash struct needs to be the first member in order to 
> prevent crashes when working with the spi flash layer
Done..

> 
> > +	u8 cmd[4] = { CMD_MX25PXX_RDSR, 0xff, 0xff, 0xff };
> > +
> > +	ret = spi_xfer(spi, 32, &cmd[0], NULL, SPI_XFER_BEGIN);
> 
> do you actually need to read/write 4 bytes here ?  shouldnt it be:
> u8 cmd = CMD_MX25PXX_RDSR;
> ret = spi_xfer(spi, 8, &cmd, NULL, SPI_XFER_BEGIN);
> 
Yes, I have checked specs too, we need only 8bit transfer here
Done...

> > +	if (ret) {
> > +		debug("SF: Failed to send command %02x: %d\n", 
> cmd[0], ret);
> 
> then you'll need to change to "cmd"
> 
> > +			debug("SF: STMicro Page Program failed\n");
> > +	debug("SF: STMicro: Successfully programmed %u bytes @ 0x%x\n",
> > +	      len, offset);
> > +			debug("SF: STMicro page erase failed\n");
> > +			debug("SF: STMicro page erase timed out\n");
> > +	debug("SF: STMicro: Successfully erased %u bytes @ 0x%x\n",
> > +	      len * sector_size, offset);
> > +		debug("SF: Unsupported STMicro ID %02x\n", id[1]);
> 
> this isnt STMicro anymore ...
Corrected... Very sorry for this :-(

> 
> > +		/* Up to 2 seconds */
> > +		ret = macronix_wait_ready(flash, 2 * CONFIG_SYS_HZ);
> 
> there's a common flash erase timeout define
Block erase time for Micronix are different than specified in spi_flash_internals.h
Hence MICRONIX spefic timouts defined and used in macronix.c
 
> 
> > +*spi_flash_probe_macronix(struct spi_slave *spi, u8 * idcode)
> 
> no space between "*" and "idcode"
Done..

> 
> > +	u8 id[3];
> > +
> > +	ret = spi_flash_cmd(spi, CMD_READ_ID, id, sizeof(id));
> > +	if (ret)
> > +		return NULL;
> 
> no need to read id here ... use the idcode passed in to you
> 
Done..

> > +		if (params->idcode1 == idcode[2]) {
> > +			break;
> > +		}
> 
> drop the {} braces
Dropped..

> 
> > --- a/drivers/mtd/spi/spi_flash.c
> > +++ b/drivers/mtd/spi/spi_flash.c
> > @@ -3,7 +3,6 @@
> >   *
> >   * Copyright (C) 2008 Atmel Corporation
> >   */
> > -#define DEBUG
> >  #include <common.h>
> >  #include <malloc.h>
> >  #include <spi.h>
> 
> please drop this hunk ... it doesnt belong in this patch (and 
> it's already been merged elsewhere) -mike
Okay Dropped

I am ready with the updated patch,
How should I release it the community?
1. new patch with review feedback (with same name)
Or
2. Delta on the top of previous patch

Any suggestion...?

Regards..
Prafulla . .

>