> -----Original Message-----
> From: Mike Frysinger [mailto:vap...@gentoo.org]
> Sent: Friday, April 03, 2009 7:44 PM
> To: u-boot@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 . .
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
