| Message ID | 1398657227-20721-4-git-send-email-b32955@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday, April 28, 2014 at 05:53:40 AM, Huang Shijie wrote: > This patch adds the DDR quad read support by the following: > > [1] add SPI_NOR_DDR_QUAD read mode. > > [2] add DDR Quad read opcodes: > SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_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] about the dummy cycles. > We set the dummy with 8 for DDR quad read by default. > The m25p80.c can not support the DDR quad read, but the SPI NOR > controller can set the dummy value in its child DT node, and the SPI NOR > framework can parse it out. > > Test this patch for Spansion s25fl128s NOR flash. > > Signed-off-by: Huang Shijie <b32955@freescale.com> Acked-by: Marek Vasut <marex@denx.de> Best regards, Marek Vasut
Hi Huang, Sorry to address this series so late. I have a few questions about how you determine support for these DDR modes. On Mon, Apr 28, 2014 at 11:53:40AM +0800, Huang Shijie wrote: > This patch adds the DDR quad read support by the following: To Mark / linux-spi: Are DDR modes in the scope of drivers/spi/ at all, so that we could someday wire it up through m25p80.c? Or is 'DDR' such a distortion of the meaning of 'SPI' such that it will be restricted only to SPI NOR dedicated controllers? > [1] add SPI_NOR_DDR_QUAD read mode. > > [2] add DDR Quad read opcodes: > SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_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] about the dummy cycles. > We set the dummy with 8 for DDR quad read by default. Why? That seems wrong. You need to know for sure how many cycles should be used, not just guess a default. > The m25p80.c can not support the DDR quad read, but the SPI NOR controller > can set the dummy value in its child DT node, and the SPI NOR framework > can parse it out. Why does the dummy value belong in device tree? I think this can be handled in software. You might, however, want a few other hardware description parameters in device tree to help you. So I think spi-nor.c needs to know a few things: 1. Does the hardware/driver support DDR clocking? 2. What granularity of dummy cycles are supported? So m25p80.c needs to communicate that it only supports dummy cycles of multiples of 8, and fsl-quadspi supports single clock cycles. And spi-nor.c should be able to do the following: 3. Set how many dummy cycles should be used. 4. Write to the configuration register, to choose a Latency Code according to what the flash supports. I see modes that support 3, 6, 7, or 8. We'd probably just go for the fastest mode, which requires 8, right? So far, none of this seems to require a DT binding, unless there's something I'm missing about your fsl-quadspi controller. > Test this patch for Spansion s25fl128s NOR flash. > > Signed-off-by: Huang Shijie <b32955@freescale.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 54 +++++++++++++++++++++++++++++++++++++++- > include/linux/mtd/spi-nor.h | 8 ++++- > 2 files changed, 58 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index f374e44..e0bc11a 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -73,7 +73,20 @@ static int read_cr(struct spi_nor *nor) > */ > static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) > { > + u32 dummy; > + > switch (nor->flash_read) { > + case SPI_NOR_DDR_QUAD: > + /* > + * The m25p80.c can not support the DDR quad read. > + * We set the dummy cycles to 8 by default. The SPI NOR > + * controller driver can set it in its child DT node. > + * We parse it out here. > + */ > + if (nor->np && !of_property_read_u32(nor->np, > + "spi-nor,ddr-quad-read-dummy", &dummy)) { > + return dummy; > + } > case SPI_NOR_FAST: > case SPI_NOR_DUAL: > case SPI_NOR_QUAD: > @@ -402,6 +415,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 +860,24 @@ 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)) { > + case CFI_MFR_AMD: /* Spansion, actually */ > + status = spansion_quad_enable(nor); > + if (status) { > + dev_err(nor->dev, > + "Spansion DDR quad-read not enabled\n"); > + return status; > + } > + return status; > + default: > + return -EINVAL; > + } > +} > + > static int set_quad_mode(struct spi_nor *nor, u32 jedec_id) > { > int status; > @@ -1016,8 +1048,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) { Hmm, I think I should probably take another look at the design of spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The driver should be communicating which (multiple) modes it supports, not selecting a single mode. spi-nor.c is the only one which knows what the *flash* supports, so it should be combining knowledge from the controller driver with its own knowledge of the flash. > + ret = set_ddr_quad_mode(nor, info->jedec_id); > + if (ret) { > + dev_err(dev, "DDR quad mode not supported\n"); > + return ret; A ramification of my comment above is that we should not be returning an error in a situation like this; we should be able to fall back to another known-supported mode, like SDR QUAD, SDR DUAL, or just plain SPI -- if they're supported by the driver -- and spi-nor.c doesn't currently have enough information to do this. > + } > + 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 +1069,14 @@ 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: > + if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) { /* Spansion */ > + nor->read_opcode = SPINOR_OP_READ_1_4_4_D; > + } else { > + dev_err(dev, "DDR Quad Read is not supported.\n"); > + return -EINVAL; > + } > + break; > case SPI_NOR_QUAD: > nor->read_opcode = SPINOR_OP_READ_1_1_4; > break; > @@ -1057,6 +1104,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, > if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) { > /* Dedicated 4-byte command set */ > switch (nor->flash_read) { > + case SPI_NOR_DDR_QUAD: > + nor->read_opcode = SPINOR_OP_READ4_1_4_4_D; > + break; > case SPI_NOR_QUAD: > nor->read_opcode = SPINOR_OP_READ4_1_1_4; > break; > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index 48fe9fc..d191a6b 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 */ > @@ -40,6 +42,7 @@ > #define SPINOR_OP_READ4_FAST 0x0c /* Read data bytes (high frequency) */ > #define SPINOR_OP_READ4_1_1_2 0x3c /* Read data bytes (Dual SPI) */ > #define SPINOR_OP_READ4_1_1_4 0x6c /* Read data bytes (Quad SPI) */ > +#define SPINOR_OP_READ4_1_4_4_D 0xee /* Read data bytes (DDR Quad SPI) */ > #define SPINOR_OP_PP_4B 0x12 /* Page program (up to 256 bytes) */ > #define SPINOR_OP_SE_4B 0xdc /* Sector erase (usually 64KiB) */ > > @@ -74,6 +77,7 @@ enum read_mode { > SPI_NOR_FAST, > SPI_NOR_DUAL, > SPI_NOR_QUAD, > + SPI_NOR_DDR_QUAD, > }; > > /** So, I'll have to take another hard look at spi-nor.c soon. I think we may need to work on the abstractions here a bit more before I merge any new features like this. Regards, Brian P.S. Is there a good reason we've defined a whole read_xfer/write_xfer API that is completely unused?
On Tue, Jul 29, 2014 at 10:08:43PM -0700, Brian Norris wrote: > > On Mon, Apr 28, 2014 at 11:53:40AM +0800, Huang Shijie wrote: > > This patch adds the DDR quad read support by the following: > > To Mark / linux-spi: > > Are DDR modes in the scope of drivers/spi/ at all, so that we could > someday wire it up through m25p80.c? Or is 'DDR' such a distortion of > the meaning of 'SPI' such that it will be restricted only to SPI NOR > dedicated controllers? IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*. The SPI can only handles the byte streams. But the DDR mode may need to handle the cycles(such as the dummy cycles could be 7 cycles) which is not byte. So the DDR mode is handled by the SPI NOR controller now. Please correct me if I am wrong. :) > > > [1] add SPI_NOR_DDR_QUAD read mode. > > > > [2] add DDR Quad read opcodes: > > SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_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] about the dummy cycles. > > We set the dummy with 8 for DDR quad read by default. > > Why? That seems wrong. You need to know for sure how many cycles should > be used, not just guess a default. Do you mean that if people do not set the DT node for dummy, we should return an -EINVAL immediately? > > > The m25p80.c can not support the DDR quad read, but the SPI NOR controller > > can set the dummy value in its child DT node, and the SPI NOR framework > > can parse it out. > > Why does the dummy value belong in device tree? I think this can be > handled in software. You might, however, want a few other hardware > description parameters in device tree to help you. > > So I think spi-nor.c needs to know a few things: > > 1. Does the hardware/driver support DDR clocking? > 2. What granularity of dummy cycles are supported? So m25p80.c needs to > communicate that it only supports dummy cycles of multiples of 8, > and fsl-quadspi supports single clock cycles. I think you can send patches for these features. I does not clear about: for what does the spi-nor needs to know the above things. > And spi-nor.c should be able to do the following: > > 3. Set how many dummy cycles should be used. where can we get the number of the cycles? > 4. Write to the configuration register, to choose a Latency Code > according to what the flash supports. I see modes that support 3, 6, > 7, or 8. We'd probably just go for the fastest mode, which requires > 8, right? not right. The DDR mode can not work if we set a wrong dummy cycles for the flash. for some chips, the fastest mode may need 6 cycles, not 8. > > So far, none of this seems to require a DT binding, unless there's > something I'm missing about your fsl-quadspi controller. > > > Test this patch for Spansion s25fl128s NOR flash. > > > > Signed-off-by: Huang Shijie <b32955@freescale.com> > > --- > > + /* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */ > > + if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) { > > Hmm, I think I should probably take another look at the design of > spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The > driver should be communicating which (multiple) modes it supports, not > selecting a single mode. spi-nor.c is the only one which knows what the > *flash* supports, so it should be combining knowledge from the > controller driver with its own knowledge of the flash. It is okay for me to add multiples modes for the spi_nor_scan(). I added the single mode for spi_nor_scan is because that the fsl-quadspi does not want to support the low speed modes. (Of course, the fsl-quadspi controller does support the low speed modes.) > > > + ret = set_ddr_quad_mode(nor, info->jedec_id); > > + if (ret) { > > + dev_err(dev, "DDR quad mode not supported\n"); > > + return ret; > > A ramification of my comment above is that we should not be returning an > error in a situation like this; we should be able to fall back to > another known-supported mode, like SDR QUAD, SDR DUAL, or just plain > SPI -- if they're supported by the driver -- and spi-nor.c doesn't > currently have enough information to do this. ok. > > > + } > > + nor->flash_read = SPI_NOR_DDR_QUAD; > > > > /** > > So, I'll have to take another hard look at spi-nor.c soon. I think we > may need to work on the abstractions here a bit more before I merge any > new features like this. okay. no problem. > > Regards, > Brian > > P.S. Is there a good reason we've defined a whole read_xfer/write_xfer > API that is completely unused? These hooks are designed for other SPI NOR drivers, the fsl-quadspi does not use them. thanks Huang Shijie
+ Mark On Wed, Jul 30, 2014 at 02:44:13PM +0800, Huang Shijie wrote: > On Tue, Jul 29, 2014 at 10:08:43PM -0700, Brian Norris wrote: > > On Mon, Apr 28, 2014 at 11:53:40AM +0800, Huang Shijie wrote: > > > This patch adds the DDR quad read support by the following: > > > > To Mark / linux-spi: > > > > Are DDR modes in the scope of drivers/spi/ at all, so that we could > > someday wire it up through m25p80.c? Or is 'DDR' such a distortion of > > the meaning of 'SPI' such that it will be restricted only to SPI NOR > > dedicated controllers? > > IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*. I agree to some extent, but I wanted to confirm with the SPI guys that DDR is truly unique to SPI NOR. (I know it doesn't currently support it.) > The SPI can only handles the byte streams. Sure. > But the DDR mode may need to > handle the cycles(such as the dummy cycles could be 7 cycles) which is not byte. But that's the same story for some (but not all) of the dual and quad modes now; some have dummy cycles that are not multiples of 8 bits. Because there are some DDR modes which have 8 dummy cycles, it is theoretically possible for the SPI subsystem to handle them. > So the DDR mode is handled by the SPI NOR controller now. Right. BTW, does your quadspi controller unconditionally support DDR, or is there any dependency on board/clock configuration? I'm just curious whether you need a DT binding to describe DDR support, or if (as long as the flash supports it, and we get the dummy cycles right) you can always use DDR QUAD. > Please correct me if I am wrong. :) > > > > [1] add SPI_NOR_DDR_QUAD read mode. > > > > > > [2] add DDR Quad read opcodes: > > > SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_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] about the dummy cycles. > > > We set the dummy with 8 for DDR quad read by default. > > > > Why? That seems wrong. You need to know for sure how many cycles should > > be used, not just guess a default. > > Do you mean that if people do not set the DT node for dummy, we should > return an -EINVAL immediately? Possibly. But I actually don't think this belongs in DT at all. See below. > > > The m25p80.c can not support the DDR quad read, but the SPI NOR controller > > > can set the dummy value in its child DT node, and the SPI NOR framework > > > can parse it out. > > > > Why does the dummy value belong in device tree? I think this can be > > handled in software. Can you answer me this question? > > You might, however, want a few other hardware > > description parameters in device tree to help you. > > > > So I think spi-nor.c needs to know a few things: > > > > 1. Does the hardware/driver support DDR clocking? > > 2. What granularity of dummy cycles are supported? So m25p80.c needs to > > communicate that it only supports dummy cycles of multiples of 8, > > and fsl-quadspi supports single clock cycles. > > I think you can send patches for these features. I does not clear about: > for what does the spi-nor needs to know the above things. To properly abstract features between a driver and the spi-nor "library." For example, we need to make sure we don't try to use a command mode with 7 dummy cycles on m25p80.c; right now, each driver has to (implicitly) know the details of dummy cycles when selecting a 'mode' parameter for spi_nor_scan(). spi-nor.c should be selecting this for us, not forcing the driver to make the selection. > > And spi-nor.c should be able to do the following: > > > > 3. Set how many dummy cycles should be used. > where can we get the number of the cycles? This should be a property of the flash device, just like everything else we detect in spi-nor.c. > > 4. Write to the configuration register, to choose a Latency Code > > according to what the flash supports. I see modes that support 3, 6, > > 7, or 8. We'd probably just go for the fastest mode, which requires > > 8, right? > not right. > > The DDR mode can not work if we set a wrong dummy cycles for the flash. > > for some chips, the fastest mode may need 6 cycles, not 8. OK, but can spi-nor.c detect this instead of putting this in DT? e.g., associate this with the device ID? Or even better, we can support CFI detection for SPI NOR flash. I notice the datasheet for your Spansion flash [1] has an extensive set of CFI parameters defined, including the dummy cycle information. I think it might be more sustainable to try to support CFI [2] and SFDP [3] for detecting new flash, rather than adding new table entries ad-hoc. > > So far, none of this seems to require a DT binding, unless there's > > something I'm missing about your fsl-quadspi controller. > > > > > Test this patch for Spansion s25fl128s NOR flash. > > > > > > Signed-off-by: Huang Shijie <b32955@freescale.com> > > > --- > > > + /* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */ > > > + if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) { > > > > Hmm, I think I should probably take another look at the design of > > spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The > > driver should be communicating which (multiple) modes it supports, not > > selecting a single mode. spi-nor.c is the only one which knows what the > > *flash* supports, so it should be combining knowledge from the > > controller driver with its own knowledge of the flash. > > It is okay for me to add multiples modes for the spi_nor_scan(). > I added the single mode for spi_nor_scan is because that the fsl-quadspi > does not want to support the low speed modes. (Of course, the fsl-quadspi > controller does support the low speed modes.) Right, fsl-quadspi only supports one mode. But m25p80.c is more flexible, and I think we might have broken some of it in the process (e.g., if the SPI controller supports single/dual/quad but the flash only supports single, then we might fail to probe). I can take a look at this problem if you don't. I'd just recommend that we might take a step back on a few of these issues before blazing ahead with something irrevocable, like a DT binding. > > > + ret = set_ddr_quad_mode(nor, info->jedec_id); > > > + if (ret) { > > > + dev_err(dev, "DDR quad mode not supported\n"); > > > + return ret; > > > > A ramification of my comment above is that we should not be returning an > > error in a situation like this; we should be able to fall back to > > another known-supported mode, like SDR QUAD, SDR DUAL, or just plain > > SPI -- if they're supported by the driver -- and spi-nor.c doesn't > > currently have enough information to do this. > ok. > > > > > + } > > > + nor->flash_read = SPI_NOR_DDR_QUAD; > > > > > > /** > > > > So, I'll have to take another hard look at spi-nor.c soon. I think we > > may need to work on the abstractions here a bit more before I merge any > > new features like this. > > okay. no problem. > > > > > Regards, > > Brian > > > > P.S. Is there a good reason we've defined a whole read_xfer/write_xfer > > API that is completely unused? > These hooks are designed for other SPI NOR drivers, the fsl-quadspi does > not use them. Brian [1] http://www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf [2] http://www.jedec.org/sites/default/files/docs/jesd68-01.pdf plus some of the vendor extensions which are documented in their datasheets [3] http://www.macronix.com/Lists/ApplicationNote/Attachments/667/AN114v1-SFDP%20Introduction.pdf http://www.jedec.org/sites/default/files/docs/JESD216.pdf (login wall)
On Wed, Jul 30, 2014 at 12:45:00AM -0700, Brian Norris wrote: > On Wed, Jul 30, 2014 at 02:44:13PM +0800, Huang Shijie wrote: > > On Tue, Jul 29, 2014 at 10:08:43PM -0700, Brian Norris wrote: > > > On Mon, Apr 28, 2014 at 11:53:40AM +0800, Huang Shijie wrote: > > > > This patch adds the DDR quad read support by the following: > > > Are DDR modes in the scope of drivers/spi/ at all, so that we could > > > someday wire it up through m25p80.c? Or is 'DDR' such a distortion of > > > the meaning of 'SPI' such that it will be restricted only to SPI NOR > > > dedicated controllers? > > IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*. > I agree to some extent, but I wanted to confirm with the SPI guys that > DDR is truly unique to SPI NOR. (I know it doesn't currently support > it.) I don't know what DDR is in this context, sorry. I'm guessing you're right since it sounds like something to do with extra clocks and this is probably not something used by generic SPI devices at present (if it ends up being widely implemented by sufficiently generic controllers that might change but the trend seems to be to flash specific controllers).
On Wed, Jul 30, 2014 at 12:45:00AM -0700, Brian Norris wrote: > > BTW, does your quadspi controller unconditionally support DDR, or is > there any dependency on board/clock configuration? I'm just curious > whether you need a DT binding to describe DDR support, or if (as long as > the flash supports it, and we get the dummy cycles right) you can always > use DDR QUAD. The fsl-quadspi controller can support the DDR quad mode unconditionally. In the other word, this controller is _designed_ for the DDR quad mode. the driver does not needs a DT binding for the DDR. thanks. > > > Please correct me if I am wrong. :) > > > > > > [1] add SPI_NOR_DDR_QUAD read mode. > > > > > > > > [2] add DDR Quad read opcodes: > > > > SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_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] about the dummy cycles. > > > > We set the dummy with 8 for DDR quad read by default. > > > > > > Why? That seems wrong. You need to know for sure how many cycles should > > > be used, not just guess a default. > > > > Do you mean that if people do not set the DT node for dummy, we should > > return an -EINVAL immediately? > > Possibly. But I actually don't think this belongs in DT at all. See > below. > > > > > The m25p80.c can not support the DDR quad read, but the SPI NOR controller > > > > can set the dummy value in its child DT node, and the SPI NOR framework > > > > can parse it out. > > > > > > Why does the dummy value belong in device tree? I think this can be > > > handled in software. We can not handle it in the software way. Why? since there are too many modes,and each mode needs different dummy cycles. Even the same chips, different versions may uses different dummy cycles in the same mode. .. You can refer to the Spansion's S25fs128s. If you want to put these dummy cycles in the device ID, it will make you crazy in the end. :) > > Can you answer me this question? > > > > You might, however, want a few other hardware > > > description parameters in device tree to help you. > > > > > > So I think spi-nor.c needs to know a few things: > > > > > > 1. Does the hardware/driver support DDR clocking? > > > 2. What granularity of dummy cycles are supported? So m25p80.c needs to > > > communicate that it only supports dummy cycles of multiples of 8, > > > and fsl-quadspi supports single clock cycles. > > > > I think you can send patches for these features. I does not clear about: > > for what does the spi-nor needs to know the above things. > > To properly abstract features between a driver and the spi-nor > "library." For example, we need to make sure we don't try to use a > command mode with 7 dummy cycles on m25p80.c; right now, each driver has > to (implicitly) know the details of dummy cycles when selecting a 'mode' > parameter for spi_nor_scan(). spi-nor.c should be selecting this for us, > not forcing the driver to make the selection. > > > > And spi-nor.c should be able to do the following: > > > > > > 3. Set how many dummy cycles should be used. > > where can we get the number of the cycles? > > This should be a property of the flash device, just like everything else > we detect in spi-nor.c. This is indeed a probably of the flash device. and this is why I add a new DT binding file for the flash. > > > > 4. Write to the configuration register, to choose a Latency Code > > > according to what the flash supports. I see modes that support 3, 6, > > > 7, or 8. We'd probably just go for the fastest mode, which requires > > > 8, right? > > not right. > > > > The DDR mode can not work if we set a wrong dummy cycles for the flash. > > > > for some chips, the fastest mode may need 6 cycles, not 8. > > OK, but can spi-nor.c detect this instead of putting this in DT? e.g., > associate this with the device ID? see the my comment above. > > Or even better, we can support CFI detection for SPI NOR flash. I notice > the datasheet for your Spansion flash [1] has an extensive set of CFI > parameters defined, including the dummy cycle information. I think it > might be more sustainable to try to support CFI [2] and SFDP [3] for > detecting new flash, rather than adding new table entries ad-hoc. I think different chip vendors may have different format to store the info, just like the read-retry for nand chips. do you want to add the code only available for Spansion? What's more the code will be very long, i think. Add a new DT binding file for the flash maybe is the simplest way. > > > > So far, none of this seems to require a DT binding, unless there's > > > something I'm missing about your fsl-quadspi controller. > > > > > > > Test this patch for Spansion s25fl128s NOR flash. > > > > > > > > Signed-off-by: Huang Shijie <b32955@freescale.com> > > > > --- > > > > + /* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */ > > > > + if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) { > > > > > > Hmm, I think I should probably take another look at the design of > > > spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The > > > driver should be communicating which (multiple) modes it supports, not > > > selecting a single mode. spi-nor.c is the only one which knows what the > > > *flash* supports, so it should be combining knowledge from the > > > controller driver with its own knowledge of the flash. > > > > It is okay for me to add multiples modes for the spi_nor_scan(). > > I added the single mode for spi_nor_scan is because that the fsl-quadspi > > does not want to support the low speed modes. (Of course, the fsl-quadspi > > controller does support the low speed modes.) > > Right, fsl-quadspi only supports one mode. But m25p80.c is more > flexible, and I think we might have broken some of it in the process > (e.g., if the SPI controller supports single/dual/quad but the flash > only supports single, then we might fail to probe). > > I can take a look at this problem if you don't. I'd just recommend that > we might take a step back on a few of these issues before blazing ahead > with something irrevocable, like a DT binding. I am glad you can spend some time on this issue. Could you please also read this patch ?: http://lists.infradead.org/pipermail/linux-mtd/2014-May/054108.html thanks Huang Shijie
On Wed, Jul 30, 2014 at 11:46:07AM +0100, Mark Brown wrote: > On Wed, Jul 30, 2014 at 12:45:00AM -0700, Brian Norris wrote: > > On Wed, Jul 30, 2014 at 02:44:13PM +0800, Huang Shijie wrote: > > > IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*. > > > I agree to some extent, but I wanted to confirm with the SPI guys that > > DDR is truly unique to SPI NOR. (I know it doesn't currently support > > it.) > > I don't know what DDR is in this context, sorry. I think it's just the ability to latch data on both the rising and falling edges of the SPI clock. For SPI flash, it seems to be used for the data portion of the opcode/address/data sequence. > I'm guessing you're > right since it sounds like something to do with extra clocks and this is > probably not something used by generic SPI devices at present (if it > ends up being widely implemented by sufficiently generic controllers > that might change but the trend seems to be to flash specific > controllers). OK, thanks for chiming in. Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts a solution. Brian
On Sat, Aug 2, 2014 at 4:06 AM, Brian Norris <computersforpeace@gmail.com> wrote: > On Wed, Jul 30, 2014 at 11:46:07AM +0100, Mark Brown wrote: >> On Wed, Jul 30, 2014 at 12:45:00AM -0700, Brian Norris wrote: >> > On Wed, Jul 30, 2014 at 02:44:13PM +0800, Huang Shijie wrote: >> > > IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*. >> >> > I agree to some extent, but I wanted to confirm with the SPI guys that >> > DDR is truly unique to SPI NOR. (I know it doesn't currently support >> > it.) >> >> I don't know what DDR is in this context, sorry. > > I think it's just the ability to latch data on both the rising and > falling edges of the SPI clock. For SPI flash, it seems to be used for > the data portion of the opcode/address/data sequence. > >> I'm guessing you're >> right since it sounds like something to do with extra clocks and this is >> probably not something used by generic SPI devices at present (if it >> ends up being widely implemented by sufficiently generic controllers >> that might change but the trend seems to be to flash specific >> controllers). > > OK, thanks for chiming in. > > Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts > a solution. I think this can just be another SPI_* spi_device.mode flag. Do we need bindings for this in Documentation/devicetree/bindings/spi/spi-bus.txt? Unlike Quad SPI transfer support, this doesn't need special wiring, so DDR capability is an intrinsic property of the SPI slave, and the mode bit can just be set in the SPI slave driver, without any DT magic? 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 Sat, Aug 02, 2014 at 11:09:09AM +0200, Geert Uytterhoeven wrote: > On Sat, Aug 2, 2014 at 4:06 AM, Brian Norris > > On Wed, Jul 30, 2014 at 11:46:07AM +0100, Mark Brown wrote: > >> I don't know what DDR is in this context, sorry. > > I think it's just the ability to latch data on both the rising and > > falling edges of the SPI clock. For SPI flash, it seems to be used for > > the data portion of the opcode/address/data sequence. > > Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts > > a solution. > I think this can just be another SPI_* spi_device.mode flag. Sounds like it yes - I was wondering if this might be one of the modes with extra clock cycles that I've heard mentioned before which might be a little more fun. > Do we need bindings for this in > Documentation/devicetree/bindings/spi/spi-bus.txt? > Unlike Quad SPI transfer support, this doesn't need special wiring, so DDR > capability is an intrinsic property of the SPI slave, and the mode bit can just > be set in the SPI slave driver, without any DT magic? Right, unless we run into things like board design issues causing constraints this is something that can be enabled by the two drivers without needing DT configuration.
On Mon, Aug 4, 2014 at 9:25 AM, Mark Brown <broonie@kernel.org> wrote: > On Sat, Aug 02, 2014 at 11:09:09AM +0200, Geert Uytterhoeven wrote: >> On Sat, Aug 2, 2014 at 4:06 AM, Brian Norris >> > On Wed, Jul 30, 2014 at 11:46:07AM +0100, Mark Brown wrote: > >> >> I don't know what DDR is in this context, sorry. > >> > I think it's just the ability to latch data on both the rising and >> > falling edges of the SPI clock. For SPI flash, it seems to be used for >> > the data portion of the opcode/address/data sequence. > >> > Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts >> > a solution. > >> I think this can just be another SPI_* spi_device.mode flag. > > Sounds like it yes - I was wondering if this might be one of the modes > with extra clock cycles that I've heard mentioned before which might be > a little more fun. > >> Do we need bindings for this in >> Documentation/devicetree/bindings/spi/spi-bus.txt? >> Unlike Quad SPI transfer support, this doesn't need special wiring, so DDR >> capability is an intrinsic property of the SPI slave, and the mode bit can just >> be set in the SPI slave driver, without any DT magic? > > Right, unless we run into things like board design issues causing > constraints this is something that can be enabled by the two drivers > without needing DT configuration. All: we plan resume this work. I need direction how go ahead further. I go through this email thread. The discussion focus on how to get dummy cycle information. Shijie get it from DT. Brain suggest get from CFI or map id table if I understand correct. Accodring to spec http://www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf Table 8.10 Dummy cycle depend on frequency, read command. if information saved in driver, it will be huge table. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Wed, Jul 22, 2015 at 1:15 PM, Zhi Li <lznuaa@gmail.com> wrote: > On Mon, Aug 4, 2014 at 9:25 AM, Mark Brown <broonie@kernel.org> wrote: >> On Sat, Aug 02, 2014 at 11:09:09AM +0200, Geert Uytterhoeven wrote: >>> On Sat, Aug 2, 2014 at 4:06 AM, Brian Norris >>> > On Wed, Jul 30, 2014 at 11:46:07AM +0100, Mark Brown wrote: >> >>> >> I don't know what DDR is in this context, sorry. >> >>> > I think it's just the ability to latch data on both the rising and >>> > falling edges of the SPI clock. For SPI flash, it seems to be used for >>> > the data portion of the opcode/address/data sequence. >> >>> > Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts >>> > a solution. >> >>> I think this can just be another SPI_* spi_device.mode flag. >> >> Sounds like it yes - I was wondering if this might be one of the modes >> with extra clock cycles that I've heard mentioned before which might be >> a little more fun. >> >>> Do we need bindings for this in >>> Documentation/devicetree/bindings/spi/spi-bus.txt? >>> Unlike Quad SPI transfer support, this doesn't need special wiring, so DDR >>> capability is an intrinsic property of the SPI slave, and the mode bit can just >>> be set in the SPI slave driver, without any DT magic? >> >> Right, unless we run into things like board design issues causing >> constraints this is something that can be enabled by the two drivers >> without needing DT configuration. > > All: Just update shijie's email address. > > we plan resume this work. > I need direction how go ahead further. > > I go through this email thread. > > The discussion focus on how to get dummy cycle information. > > Shijie get it from DT. > > Brain suggest get from CFI or map id table if I understand correct. > > Accodring to spec > http://www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf > > Table 8.10 > > Dummy cycle depend on frequency, read command. > > if information saved in driver, it will be huge table. > >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index f374e44..e0bc11a 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -73,7 +73,20 @@ static int read_cr(struct spi_nor *nor) */ static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor) { + u32 dummy; + switch (nor->flash_read) { + case SPI_NOR_DDR_QUAD: + /* + * The m25p80.c can not support the DDR quad read. + * We set the dummy cycles to 8 by default. The SPI NOR + * controller driver can set it in its child DT node. + * We parse it out here. + */ + if (nor->np && !of_property_read_u32(nor->np, + "spi-nor,ddr-quad-read-dummy", &dummy)) { + return dummy; + } case SPI_NOR_FAST: case SPI_NOR_DUAL: case SPI_NOR_QUAD: @@ -402,6 +415,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 +860,24 @@ 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)) { + case CFI_MFR_AMD: /* Spansion, actually */ + status = spansion_quad_enable(nor); + if (status) { + dev_err(nor->dev, + "Spansion DDR quad-read not enabled\n"); + return status; + } + return status; + default: + return -EINVAL; + } +} + static int set_quad_mode(struct spi_nor *nor, u32 jedec_id) { int status; @@ -1016,8 +1048,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 +1069,14 @@ 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: + if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) { /* Spansion */ + nor->read_opcode = SPINOR_OP_READ_1_4_4_D; + } else { + dev_err(dev, "DDR Quad Read is not supported.\n"); + return -EINVAL; + } + break; case SPI_NOR_QUAD: nor->read_opcode = SPINOR_OP_READ_1_1_4; break; @@ -1057,6 +1104,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id, if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) { /* Dedicated 4-byte command set */ switch (nor->flash_read) { + case SPI_NOR_DDR_QUAD: + nor->read_opcode = SPINOR_OP_READ4_1_4_4_D; + break; case SPI_NOR_QUAD: nor->read_opcode = SPINOR_OP_READ4_1_1_4; break; diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 48fe9fc..d191a6b 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 */ @@ -40,6 +42,7 @@ #define SPINOR_OP_READ4_FAST 0x0c /* Read data bytes (high frequency) */ #define SPINOR_OP_READ4_1_1_2 0x3c /* Read data bytes (Dual SPI) */ #define SPINOR_OP_READ4_1_1_4 0x6c /* Read data bytes (Quad SPI) */ +#define SPINOR_OP_READ4_1_4_4_D 0xee /* Read data bytes (DDR Quad SPI) */ #define SPINOR_OP_PP_4B 0x12 /* Page program (up to 256 bytes) */ #define SPINOR_OP_SE_4B 0xdc /* Sector erase (usually 64KiB) */ @@ -74,6 +77,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 opcodes: SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_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] about the dummy cycles. We set the dummy with 8 for DDR quad read by default. The m25p80.c can not support the DDR quad read, but the SPI NOR controller can set the dummy value in its child DT node, and the SPI NOR framework can parse it out. Test this patch for Spansion s25fl128s NOR flash. Signed-off-by: Huang Shijie <b32955@freescale.com> --- drivers/mtd/spi-nor/spi-nor.c | 54 +++++++++++++++++++++++++++++++++++++++- include/linux/mtd/spi-nor.h | 8 ++++- 2 files changed, 58 insertions(+), 4 deletions(-)