-----Original Message----- From: Mike Frysinger [mailto:vapier@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 . .