Message ID | 20150501090108.GA7205@amd |
---|---|
State | Superseded |
Delegated to: | Jagannadha Sutradharudu Teki |
Headers | show |
On Friday, May 01, 2015 at 11:01:09 AM, Pavel Machek wrote: > This is needed for the SoCFPGA booting from SPI NOR flash > e.g. (N25Q256A) as long as u-boot-spl 2013 is used (newer one is not > available). > > Signed-off-by: Stefan Roese <sr@denx.de> > Signed-off-by: Pavel Machek <pavel@denx.de> > > --- > > Ported to today's u-boot version. > > diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c > index 201471c..f7cfbd9 100644 > --- a/drivers/mtd/spi/sf_probe.c > +++ b/drivers/mtd/spi/sf_probe.c > @@ -347,6 +348,36 @@ int spi_flash_probe_slave(struct spi_slave *spi, > struct spi_flash *flash) } > } > > +#ifdef CONFIG_SPI_N25Q256A_RESET Should be CONFIG_SPI_MICRON_RESET, since other parts which can also be used would have similar issue. This should also be documented in README I believe. It'd be nice if you added diffstat into your patches as it makes things easier during review. > +#define CMD_RESET_ENABLE 0x66 > +#define CMD_RESET_MEMORY 0x99 > + /* > + * This is needed for the SoCFPGA booting from SPI NOR flash > + * e.g. (N25Q256A), as U-Boot SPL 2013-socfpga (only version > + * working on that board) sets 4-byt addressing mode. > + * > + * Additionally it may be good idea to change > + * this line in the Linux SPI NOR flash driver: Please submit a patch for Linux then. But this will be extremely crappy and unreliable solution, so you should at least make the kernel spit some warning upon shutdown so people are aware they are doing something terribly wrong. > + * { "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, > + * SECT_4K | SHUTDOWN_3BYTE) }, > + * > + * Add SHUTDOWN_3BYTE here. > + */ > + ret = spi_flash_cmd(spi, CMD_RESET_ENABLE, NULL, 0); > + if (ret) { > + printf("SF: Failed issue reset command\n"); I thought this was just a reset-enable command. If this command fails, user won't be able to tell which of these two failed, so it's a bad idea to use the same error message for both. > + goto err_read_id; > + } > + > + ret = spi_flash_cmd(spi, CMD_RESET_MEMORY, NULL, 0); > + if (ret) { > + printf("SF: Failed issue reset command\n"); > + goto err_read_id; > + } > + > + printf("SF: Device software reset\n"); > +#endif > #ifdef CONFIG_OF_CONTROL > if (spi_flash_decode_fdt(gd->fdt_blob, flash)) { > debug("SF: FDT decode error\n"); Best regards, Marek Vasut
On Fri 2015-05-01 16:24:45, Marek Vasut wrote: > On Friday, May 01, 2015 at 11:01:09 AM, Pavel Machek wrote: > > diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c > > index 201471c..f7cfbd9 100644 > > --- a/drivers/mtd/spi/sf_probe.c > > +++ b/drivers/mtd/spi/sf_probe.c > > @@ -347,6 +348,36 @@ int spi_flash_probe_slave(struct spi_slave *spi, > > struct spi_flash *flash) } > > } > > > > +#ifdef CONFIG_SPI_N25Q256A_RESET > > Should be CONFIG_SPI_MICRON_RESET, since other parts which can also be > used would have similar issue. I'm pretty sure some Micron parts use different interface. > It'd be nice if you > added diffstat into your patches as it makes things easier during > review. Yes, it also makes patch harder to create (as it is tricky to hand-edit the patches), and having diffstat for a patch that fits on a screen is just stupid. > > + * { "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, > > + * SECT_4K | SHUTDOWN_3BYTE) }, > > + * > > + * Add SHUTDOWN_3BYTE here. > > + */ > > + ret = spi_flash_cmd(spi, CMD_RESET_ENABLE, NULL, 0); > > + if (ret) { > > + printf("SF: Failed issue reset command\n"); > > I thought this was just a reset-enable command. If this command > fails, user won't be able to tell which of these two failed, so > it's a bad idea to use the same error message for both. Ok. Pavel
On Friday, May 01, 2015 at 04:49:37 PM, Pavel Machek wrote: > On Fri 2015-05-01 16:24:45, Marek Vasut wrote: > > On Friday, May 01, 2015 at 11:01:09 AM, Pavel Machek wrote: > > > diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c > > > index 201471c..f7cfbd9 100644 > > > --- a/drivers/mtd/spi/sf_probe.c > > > +++ b/drivers/mtd/spi/sf_probe.c > > > @@ -347,6 +348,36 @@ int spi_flash_probe_slave(struct spi_slave *spi, > > > struct spi_flash *flash) } > > > > > > } > > > > > > +#ifdef CONFIG_SPI_N25Q256A_RESET > > > > Should be CONFIG_SPI_MICRON_RESET, since other parts which can also be > > used would have similar issue. > > I'm pretty sure some Micron parts use different interface. Which ones ? > > It'd be nice if you > > added diffstat into your patches as it makes things easier during > > review. > > Yes, it also makes patch harder to create (as it is tricky to > hand-edit the patches) git format-patch automatically inserts the diffstat for you. > , and having diffstat for a patch that fits on a > screen is just stupid. It's a 2-line diffstat for this patch. If you get a 1-screen big diffstat, then your patch is most likely wrong. > > > + * { "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, > > > + * SECT_4K | SHUTDOWN_3BYTE) }, > > > + * > > > + * Add SHUTDOWN_3BYTE here. > > > + */ > > > + ret = spi_flash_cmd(spi, CMD_RESET_ENABLE, NULL, 0); > > > + if (ret) { > > > + printf("SF: Failed issue reset command\n"); > > > > I thought this was just a reset-enable command. If this command > > fails, user won't be able to tell which of these two failed, so > > it's a bad idea to use the same error message for both. > > Ok. > Pavel Best regards, Marek Vasut
On Fri 2015-05-01 19:26:34, Marek Vasut wrote: > On Friday, May 01, 2015 at 04:49:37 PM, Pavel Machek wrote: > > On Fri 2015-05-01 16:24:45, Marek Vasut wrote: > > > On Friday, May 01, 2015 at 11:01:09 AM, Pavel Machek wrote: > > > > diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c > > > > index 201471c..f7cfbd9 100644 > > > > --- a/drivers/mtd/spi/sf_probe.c > > > > +++ b/drivers/mtd/spi/sf_probe.c > > > > @@ -347,6 +348,36 @@ int spi_flash_probe_slave(struct spi_slave *spi, > > > > struct spi_flash *flash) } > > > > > > > > } > > > > > > > > +#ifdef CONFIG_SPI_N25Q256A_RESET > > > > > > Should be CONFIG_SPI_MICRON_RESET, since other parts which can also be > > > used would have similar issue. > > > > I'm pretty sure some Micron parts use different interface. > > Which ones ? N25Q128? > > > It'd be nice if you > > > added diffstat into your patches as it makes things easier during > > > review. > > > > Yes, it also makes patch harder to create (as it is tricky to > > hand-edit the patches) > > git format-patch automatically inserts the diffstat for you. As I have already told you, I'm not using git to submit my patches. > > , and having diffstat for a patch that fits on a > > screen is just stupid. > > It's a 2-line diffstat for this patch. If you get a 1-screen big > diffstat, then your patch is most likely wrong. As I said, diffstat for patch that fits on screen is useless and stupid. Pavel
On Sunday, May 10, 2015 at 11:07:38 AM, Pavel Machek wrote: > On Fri 2015-05-01 19:26:34, Marek Vasut wrote: > > On Friday, May 01, 2015 at 04:49:37 PM, Pavel Machek wrote: > > > On Fri 2015-05-01 16:24:45, Marek Vasut wrote: > > > > On Friday, May 01, 2015 at 11:01:09 AM, Pavel Machek wrote: > > > > > diff --git a/drivers/mtd/spi/sf_probe.c > > > > > b/drivers/mtd/spi/sf_probe.c index 201471c..f7cfbd9 100644 > > > > > --- a/drivers/mtd/spi/sf_probe.c > > > > > +++ b/drivers/mtd/spi/sf_probe.c > > > > > @@ -347,6 +348,36 @@ int spi_flash_probe_slave(struct spi_slave > > > > > *spi, struct spi_flash *flash) } > > > > > > > > > > } > > > > > > > > > > +#ifdef CONFIG_SPI_N25Q256A_RESET > > > > > > > > Should be CONFIG_SPI_MICRON_RESET, since other parts which can also > > > > be used would have similar issue. > > > > > > I'm pretty sure some Micron parts use different interface. > > > > Which ones ? > > N25Q128? According to [1] page 28, the interface is the same. Did I miss something? [1] https://www.micron.com/~/media/documents/products/data-sheet/nor- flash/serial-nor/n25q/n25q_128_3_volt_with_boot_sector.pdf > > > > It'd be nice if you > > > > added diffstat into your patches as it makes things easier during > > > > review. > > > > > > Yes, it also makes patch harder to create (as it is tricky to > > > hand-edit the patches) > > > > git format-patch automatically inserts the diffstat for you. > > As I have already told you, I'm not using git to submit my patches. Please fix your tools then. [...] Best regards, Marek Vasut
diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index 201471c..f7cfbd9 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -347,6 +348,36 @@ int spi_flash_probe_slave(struct spi_slave *spi, struct spi_flash *flash) } } +#ifdef CONFIG_SPI_N25Q256A_RESET +#define CMD_RESET_ENABLE 0x66 +#define CMD_RESET_MEMORY 0x99 + /* + * This is needed for the SoCFPGA booting from SPI NOR flash + * e.g. (N25Q256A), as U-Boot SPL 2013-socfpga (only version + * working on that board) sets 4-byt addressing mode. + * + * Additionally it may be good idea to change + * this line in the Linux SPI NOR flash driver: + * + * { "n25q256a", INFO(0x20ba19, 0, 64 * 1024, 512, + * SECT_4K | SHUTDOWN_3BYTE) }, + * + * Add SHUTDOWN_3BYTE here. + */ + ret = spi_flash_cmd(spi, CMD_RESET_ENABLE, NULL, 0); + if (ret) { + printf("SF: Failed issue reset command\n"); + goto err_read_id; + } + + ret = spi_flash_cmd(spi, CMD_RESET_MEMORY, NULL, 0); + if (ret) { + printf("SF: Failed issue reset command\n"); + goto err_read_id; + } + + printf("SF: Device software reset\n"); +#endif #ifdef CONFIG_OF_CONTROL if (spi_flash_decode_fdt(gd->fdt_blob, flash)) { debug("SF: FDT decode error\n");