| Message ID | 1397641641-11430-1-git-send-email-b32955@freescale.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wednesday, April 16, 2014 at 11:47:21 AM, Huang Shijie wrote: [...] > @@ -74,6 +74,11 @@ static int read_cr(struct spi_nor *nor) > static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) > { > switch (nor->flash_read) { > + case SPI_NOR_DDR_QUAD: > + /* > + * We set 8 for the DDR Quad read, the SPI NOR controller > + * can change it to 6 or 4 with DeviceTree property. > + */ Isn't the number of dummy cycles a property of the chip ? [...] > +static int set_ddr_quad_mode(struct spi_nor *nor, u32 jedec_id) > +{ > + int status; > + > + switch (JEDEC_MFR(jedec_id)) { > + default: /* Spansion */ This should really _check_ the jedec ID here and only enable the quad mode for spansion on spansion flashes. The default case should just return -ENOTSUP or something. [...] > @@ -1016,8 +1038,15 @@ int spi_nor_scan(struct spi_nor *nor, const struct > spi_device_id *id, if (info->flags & SPI_NOR_NO_FR) > nor->flash_read = SPI_NOR_NORMAL; > > - /* Quad/Dual-read mode takes precedence over fast/normal */ > - if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) { > + /* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */ > + if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) { > + ret = set_ddr_quad_mode(nor, info->jedec_id); > + if (ret) { > + dev_err(dev, "DDR quad mode not supported\n"); > + return ret; Is it really necessary to fail here? Can we not fall back to some "lower" mode ? [...] Best regards, Marek Vasut
On Thu, Apr 17, 2014 at 04:24:36PM +0200, Marek Vasut wrote: > On Wednesday, April 16, 2014 at 11:47:21 AM, Huang Shijie wrote: > [...] > > > @@ -74,6 +74,11 @@ static int read_cr(struct spi_nor *nor) > > static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) > > { > > switch (nor->flash_read) { > > + case SPI_NOR_DDR_QUAD: > > + /* > > + * We set 8 for the DDR Quad read, the SPI NOR controller > > + * can change it to 6 or 4 with DeviceTree property. > > + */ > > Isn't the number of dummy cycles a property of the chip ? I want to add DT node, such as "ddr_quad_read_dummy". But I do not know which Document file to put the DT node to. the spi-bus.txt ? or the spi-nor.txt? > [...] > > > +static int set_ddr_quad_mode(struct spi_nor *nor, u32 jedec_id) > > +{ > > + int status; > > + > > + switch (JEDEC_MFR(jedec_id)) { > > + default: /* Spansion */ > > This should really _check_ the jedec ID here and only enable the quad mode for > spansion on spansion flashes. The default case should just return -ENOTSUP or > something. > [...] ok. change it in next version. > > > @@ -1016,8 +1038,15 @@ int spi_nor_scan(struct spi_nor *nor, const struct > > spi_device_id *id, if (info->flags & SPI_NOR_NO_FR) > > nor->flash_read = SPI_NOR_NORMAL; > > > > - /* Quad/Dual-read mode takes precedence over fast/normal */ > > - if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) { > > + /* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */ > > + if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) { > > + ret = set_ddr_quad_mode(nor, info->jedec_id); > > + if (ret) { > > + dev_err(dev, "DDR quad mode not supported\n"); > > + return ret; > > Is it really necessary to fail here? Can we not fall back to some "lower" mode ? I perfer to fail here. If we fail to enable the quad read, we also do not fall back to fast read mode. thanks Huang Shijie
On Thu, Apr 17, 2014 at 4:24 PM, Marek Vasut <marex@denx.de> wrote: >> @@ -1016,8 +1038,15 @@ int spi_nor_scan(struct spi_nor *nor, const struct >> spi_device_id *id, if (info->flags & SPI_NOR_NO_FR) >> nor->flash_read = SPI_NOR_NORMAL; >> >> - /* Quad/Dual-read mode takes precedence over fast/normal */ >> - if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) { >> + /* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */ >> + if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) { >> + ret = set_ddr_quad_mode(nor, info->jedec_id); >> + if (ret) { >> + dev_err(dev, "DDR quad mode not supported\n"); >> + return ret; > > Is it really necessary to fail here? Can we not fall back to some "lower" mode ? IIRC, the original m25p80 also fails, FWIW... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thursday, April 17, 2014 at 08:41:50 PM, Geert Uytterhoeven wrote: > On Thu, Apr 17, 2014 at 4:24 PM, Marek Vasut <marex@denx.de> wrote: > >> @@ -1016,8 +1038,15 @@ int spi_nor_scan(struct spi_nor *nor, const > >> struct spi_device_id *id, if (info->flags & SPI_NOR_NO_FR) > >> > >> nor->flash_read = SPI_NOR_NORMAL; > >> > >> - /* Quad/Dual-read mode takes precedence over fast/normal */ > >> - if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) { > >> + /* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal > >> */ + if (mode == SPI_NOR_DDR_QUAD && info->flags & > >> SPI_NOR_DDR_QUAD_READ) { + ret = set_ddr_quad_mode(nor, > >> info->jedec_id); > >> + if (ret) { > >> + dev_err(dev, "DDR quad mode not supported\n"); > >> + return ret; > > > > Is it really necessary to fail here? Can we not fall back to some "lower" > > mode ? > > IIRC, the original m25p80 also fails, FWIW... OK, so let's keep consistent. But maybe we should add a note for future generations that this can be improved. Best regards, Marek Vasut
On Thursday, April 17, 2014 at 06:05:17 PM, Huang Shijie wrote: > On Thu, Apr 17, 2014 at 04:24:36PM +0200, Marek Vasut wrote: > > On Wednesday, April 16, 2014 at 11:47:21 AM, Huang Shijie wrote: > > [...] > > > > > @@ -74,6 +74,11 @@ static int read_cr(struct spi_nor *nor) > > > > > > static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) > > > { > > > > > > switch (nor->flash_read) { > > > > > > + case SPI_NOR_DDR_QUAD: > > > + /* > > > + * We set 8 for the DDR Quad read, the SPI NOR controller > > > + * can change it to 6 or 4 with DeviceTree property. > > > + */ > > > > Isn't the number of dummy cycles a property of the chip ? > > I want to add DT node, such as "ddr_quad_read_dummy". > But I do not know which Document file to put the DT node to. > > the spi-bus.txt ? or the spi-nor.txt? It's a property of the chip, therefore spi-nor.txt , no ? [...] I replied about the failing-to-lower-mode in the other email. Best regards, Marek Vasut
On Mon, Apr 21, 2014 at 05:26:07PM +0200, Marek Vasut wrote: > On Thursday, April 17, 2014 at 06:05:17 PM, Huang Shijie wrote: > > On Thu, Apr 17, 2014 at 04:24:36PM +0200, Marek Vasut wrote: > > > On Wednesday, April 16, 2014 at 11:47:21 AM, Huang Shijie wrote: > > > [...] > > > > > > > @@ -74,6 +74,11 @@ static int read_cr(struct spi_nor *nor) > > > > > > > > static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) > > > > { > > > > > > > > switch (nor->flash_read) { > > > > > > > > + case SPI_NOR_DDR_QUAD: > > > > + /* > > > > + * We set 8 for the DDR Quad read, the SPI NOR controller > > > > + * can change it to 6 or 4 with DeviceTree property. > > > > + */ > > > > > > Isn't the number of dummy cycles a property of the chip ? > > > > I want to add DT node, such as "ddr_quad_read_dummy". > > But I do not know which Document file to put the DT node to. > > > > the spi-bus.txt ? or the spi-nor.txt? > > It's a property of the chip, therefore spi-nor.txt , no ? [1] For the m25p80.c, the @nor->dev points to a spi_device{}. If the spi core can support the DDR QUAD read, we can add the DT node to the spi-bus.txt. Please see the spi-bus.txt: ---------------------------------------------------------------------- SPI slave nodes must be children of the SPI master node and can contain the following properties. - reg - (required) chip select address of device. - compatible - (required) name of SPI device following generic names recommended practice - spi-max-frequency - (required) Maximum SPI clocking speed of device in Hz - spi-cpol - (optional) Empty property indicating device requires inverse clock polarity (CPOL) mode - spi-cpha - (optional) Empty property indicating device requires shifted clock phase (CPHA) mode - spi-cs-high - (optional) Empty property indicating device requires chip select active high - spi-3wire - (optional) Empty property indicating device requires ---------------------------------------------------------------------- As a SPI slave DT node, we can put the "ddr_quad_read_dummy" to the spi-bus.txt. But unfortunately, I am not sure if the SPI core can support the DDR quad read or not. :( IMHO, it can _NOT_ support. [2] For the SPI NOR controller(such as fsl-quadspi.c), the @nor->dev points to a platform_device{}. I did not allocate a device{} for the NOR flash. so i could add the "ddr_quad_read_dummy" to the fsl-quadspi.txt. what's your opinion? thanks Huang Shijie
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index b88cc7e..6858e36 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -74,6 +74,11 @@ static int read_cr(struct spi_nor *nor) static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) { switch (nor->flash_read) { + case SPI_NOR_DDR_QUAD: + /* + * We set 8 for the DDR Quad read, the SPI NOR controller + * can change it to 6 or 4 with DeviceTree property. + */ case SPI_NOR_FAST: case SPI_NOR_DUAL: case SPI_NOR_QUAD: @@ -402,6 +407,7 @@ struct flash_info { #define SECT_4K_PMC 0x10 /* SPINOR_OP_BE_4K_PMC works uniformly */ #define SPI_NOR_DUAL_READ 0x20 /* Flash supports Dual Read */ #define SPI_NOR_QUAD_READ 0x40 /* Flash supports Quad Read */ +#define SPI_NOR_DDR_QUAD_READ 0x80 /* Flash supports DDR Quad Read */ }; #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \ @@ -846,6 +852,22 @@ static int spansion_quad_enable(struct spi_nor *nor) return 0; } +static int set_ddr_quad_mode(struct spi_nor *nor, u32 jedec_id) +{ + int status; + + switch (JEDEC_MFR(jedec_id)) { + default: /* Spansion */ + status = spansion_quad_enable(nor); + if (status) { + dev_err(nor->dev, + "Spansion DDR quad-read not enabled\n"); + return -EINVAL; + } + return status; + } +} + static int set_quad_mode(struct spi_nor *nor, u32 jedec_id) { int status; @@ -1016,8 +1038,15 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, if (info->flags & SPI_NOR_NO_FR) nor->flash_read = SPI_NOR_NORMAL; - /* Quad/Dual-read mode takes precedence over fast/normal */ - if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) { + /* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */ + if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) { + ret = set_ddr_quad_mode(nor, info->jedec_id); + if (ret) { + dev_err(dev, "DDR quad mode not supported\n"); + return ret; + } + nor->flash_read = SPI_NOR_DDR_QUAD; + } else if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) { ret = set_quad_mode(nor, info->jedec_id); if (ret) { dev_err(dev, "quad mode not supported\n"); @@ -1030,6 +1059,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, /* Default commands */ switch (nor->flash_read) { + case SPI_NOR_DDR_QUAD: + nor->read_opcode = SPINOR_OP_READ_1_4_4_D; + break; case SPI_NOR_QUAD: nor->read_opcode = SPINOR_OP_READ_1_1_4; break; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 5324184..b3ada3e 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -12,10 +12,11 @@ /* * Note on opcode nomenclature: some opcodes have a format like - * SPINOR_OP_FUNCTION{4,}_x_y_z. The numbers x, y, and z stand for the number + * SPINOR_OP_FUNCTION{4,}_x_y_z{_D}. The numbers x, y, and z stand for the number * of I/O lines used for the opcode, address, and data (respectively). The * FUNCTION has an optional suffix of '4', to represent an opcode which - * requires a 4-byte (32-bit) address. + * requires a 4-byte (32-bit) address. The suffix of 'D' stands for the + * DDR mode. */ /* Flash opcodes. */ @@ -26,6 +27,7 @@ #define SPINOR_OP_READ_FAST 0x0b /* Read data bytes (high frequency) */ #define SPINOR_OP_READ_1_1_2 0x3b /* Read data bytes (Dual SPI) */ #define SPINOR_OP_READ_1_1_4 0x6b /* Read data bytes (Quad SPI) */ +#define SPINOR_OP_READ_1_4_4_D 0xed /* Read data bytes (DDR Quad SPI) */ #define SPINOR_OP_PP 0x02 /* Page program (up to 256 bytes) */ #define SPINOR_OP_BE_4K 0x20 /* Erase 4KiB block */ #define SPINOR_OP_BE_4K_PMC 0xd7 /* Erase 4KiB block on PMC chips */ @@ -74,6 +76,7 @@ enum read_mode { SPI_NOR_FAST, SPI_NOR_DUAL, SPI_NOR_QUAD, + SPI_NOR_DDR_QUAD, }; /**
This patch adds the DDR quad read support by the following: [1] add SPI_NOR_DDR_QUAD read mode. [2] add DDR Quad read opcode: SPINOR_OP_READ_1_4_4_D. [3] add set_ddr_quad_mode() to initialize for the DDR quad read. Currently it only works for Spansion NOR. [3] set the dummy with 8 for DDR quad read. _ Signed-off-by: Huang Shijie <b32955@freescale.com> --- drivers/mtd/spi-nor/spi-nor.c | 36 ++++++++++++++++++++++++++++++++++-- include/linux/mtd/spi-nor.h | 7 +++++-- 2 files changed, 39 insertions(+), 4 deletions(-)