| Message ID | 1376885403-12156-4-git-send-email-b32955@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Adding others (keep devicetree@vger.kernel.org in the loop) On Mon, Aug 19, 2013 at 12:10:01PM +0800, Huang Shijie wrote: > This patch adds the quad read support: > > (1) Add the relative commands: > OPCODE_QIOR, OPCODE_4QIOR, OPCODE_RDCR, > > also add the relative macro for the Configuartion Register. > > (2) add the "m25p,quad-read" property for the m25p80 driver > If the dts has the "m25p,quad-read" property, the kernel will > set the Quad bit of the configuration register, and when the > setting is suceeded, we set the read opcode with OPCODE_QIOR. > > Signed-off-by: Huang Shijie <b32955@freescale.com> > --- > Documentation/devicetree/bindings/mtd/m25p80.txt | 5 ++ > drivers/mtd/devices/m25p80.c | 51 ++++++++++++++++++++++ > include/linux/mtd/spi-nor.h | 6 +++ > 3 files changed, 62 insertions(+), 0 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt b/Documentation/devicetree/bindings/mtd/m25p80.txt > index 6d3d576..b33313f 100644 > --- a/Documentation/devicetree/bindings/mtd/m25p80.txt > +++ b/Documentation/devicetree/bindings/mtd/m25p80.txt > @@ -17,6 +17,11 @@ Optional properties: > Refer to your chips' datasheet to check if this is supported > by your chip. > > +- m25p,quad-read : Use the "quad read" opcode to read data from the chip instead > + of the usual "read" opcode. This opcode is not supported by > + all chips and support for it can not be detected at runtime. > + Refer to your chips' datasheet to check if this is supported > + by your chip. Why can't this be detected at runtime? We added a "no fast read" flag to the device table, so why not "dual/quad mode supported"? And believe it or not, not all m25p80 users have device tree. So it isn't very logical to tie this support to device-tree only. > Example: > > flash: m25p80@0 { > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index f3598c1..4bc9b1b 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -103,6 +103,40 @@ static int write_sr(struct m25p *flash, u8 val) > } > > /* > + * Read the configuration register, returning its value in the location > + * Return the configuration register value. > + * Returns negative if error occurred. > + */ > +static int read_cr(struct m25p *flash) > +{ > + u8 code = OPCODE_RDCR; > + int ret; > + u8 val; > + > + ret = spi_write_then_read(flash->spi, &code, 1, &val, 1); > + if (ret < 0) { > + dev_err(&flash->spi->dev, "error %d reading CR\n", ret); > + return ret; > + } > + return val; > +} > + > +/* > + * Write status register and configuration register with 2 bytes > + * The first byte will be written to the status register, while the second byte > + * will be written to the configuration register. > + * Returns negative if error occurred. > + */ > +static int write_sr_cr(struct m25p *flash, u16 val) > +{ > + flash->command[0] = OPCODE_WRSR; > + flash->command[1] = 0; > + flash->command[2] = (val >> 8); > + > + return spi_write(flash->spi, flash->command, 3); > +} > + > +/* > * Set write enable latch with Write Enable command. > * Returns negative if error occurred. > */ > @@ -880,6 +914,8 @@ static int m25p_probe(struct spi_device *spi) > unsigned i; > struct mtd_part_parser_data ppdata; > struct device_node __maybe_unused *np = spi->dev.of_node; > + u16 sr_cr; > + int ret; > > #ifdef CONFIG_MTD_OF_PARTS > if (!of_device_is_available(np)) > @@ -1014,6 +1050,21 @@ static int m25p_probe(struct spi_device *spi) > else > flash->read_opcode = OPCODE_NORM_READ; > > + /* Try to enable the Quad Read */ > + if (np && of_property_read_bool(np, "m25p,quad-read")) { > + /* The configuration register is set by the second byte. */ > + sr_cr = CR_QUAD << 8; > + > + /* Write the QUAD bit to the Configuration Register. */ > + write_enable(flash); > + if (write_sr_cr(flash, sr_cr) == 0) { > + /* read back and check it */ > + ret = read_cr(flash); > + if (ret > 0 && (ret & CR_QUAD)) > + flash->read_opcode = OPCODE_QIOR; This is not correct. You are assuming that the SPI master knows to read with 4 IO lines, instead of the traditional 1 line; IOW, you are assuming that: (1) if the slave DT node has "quad-read", then the whole system supports it (bad design; you're putting assumptions about the "parent" node in the child) (2) the controller driver will act as your new driver does -- that it will decode these commands and recognize that the quad read command should be run on 4 IO lines, not just 1 What you're really missing from device-tree (and the SPI subystem in general) is how to detect those SPI controllers which support dual and quad mode transfers, and how to communicate that a particular SPI transaction should be performed on 1 or 4 IO lines. We shouldn't have this just hacked in via assumptions. See Wang Yuhang's patch set for adding proper dual/quad support to the SPI subsystem. It seems to be addressing (1) and (2) in a better way. http://thread.gmane.org/gmane.linux.kernel.spi.devel/14420 (I can't find patch 2/2) > + } > + } > + > flash->program_opcode = OPCODE_PP; > > if (info->addr_width) > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index b420a5b..d5b189d 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -39,6 +39,9 @@ > > /* Used for Spansion flashes only. */ > #define OPCODE_BRWR 0x17 /* Bank register write */ > +#define OPCODE_QIOR 0xeb /* Quad read */ > +#define OPCODE_4QIOR 0xec /* Quad read */ Use tab between 'define' and 'OPCODE...', like OPCODE_RDCR below. > +#define OPCODE_RDCR 0x35 /* Read configuration register */ > > /* Status Register bits. */ > #define SR_WIP 1 /* Write in progress */ > @@ -49,4 +52,7 @@ > #define SR_BP2 0x10 /* Block protect 2 */ > #define SR_SRWD 0x80 /* SR write protect */ > > +/* Configuration Register bits. */ > +#define CR_QUAD 0x2 /* Quad I/O */ > + > #endif /* __LINUX_MTD_SPI_NOR_H */ Brian
On Thu, Aug 22, 2013 at 12:34:53PM -0700, Brian Norris wrote: > On Mon, Aug 19, 2013 at 12:10:01PM +0800, Huang Shijie wrote: > > +- m25p,quad-read : Use the "quad read" opcode to read data from the chip instead > > + of the usual "read" opcode. This opcode is not supported by > > + all chips and support for it can not be detected at runtime. > > + Refer to your chips' datasheet to check if this is supported > > + by your chip. > Why can't this be detected at runtime? We added a "no fast read" flag to > the device table, so why not "dual/quad mode supported"? And believe it > or not, not all m25p80 users have device tree. So it isn't very logical > to tie this support to device-tree only. There needs to be some way of saying if the additional data lines are actually wired up or not; it could be a negative property (flagging if the lines are not present) but that runs the risk of breaking systems if a driver acquires the ability to support extra data lines but a system doesn't have it. This should be a generic property for all quad devices to use, though, since the same thing applies everywhere. > This is not correct. You are assuming that the SPI master knows to read > with 4 IO lines, instead of the traditional 1 line; IOW, you are > assuming that: > (1) if the slave DT node has "quad-read", then the whole system supports > it (bad design; you're putting assumptions about the "parent" node > in the child) This is fine, the device tree is for the board as a whole not for the individual chips - if the board doesn't support quad read the device tree shouldn't configure the chip for quad mode. It really needs to be a slave property since a system could opt to connect some devices with extra data lines and some without on the same SPI bus. > What you're really missing from device-tree (and the SPI subystem in > general) is how to detect those SPI controllers which support dual and > quad mode transfers, and how to communicate that a particular SPI > transaction should be performed on 1 or 4 IO lines. We shouldn't have > this just hacked in via assumptions. That bit does need to be fixed in this driver, yes. The SPI core now has quad mode support.
Dear Mark Brown, > On Thu, Aug 22, 2013 at 12:34:53PM -0700, Brian Norris wrote: > > On Mon, Aug 19, 2013 at 12:10:01PM +0800, Huang Shijie wrote: > > > +- m25p,quad-read : Use the "quad read" opcode to read data from the > > > chip instead + of the usual "read" opcode. This > > > opcode is not supported by + all chips and support > > > for it can not be detected at runtime. + Refer to > > > your chips' datasheet to check if this is supported + > > > by your chip. > > > > Why can't this be detected at runtime? We added a "no fast read" flag to > > the device table, so why not "dual/quad mode supported"? And believe it > > or not, not all m25p80 users have device tree. So it isn't very logical > > to tie this support to device-tree only. > > There needs to be some way of saying if the additional data lines are > actually wired up or not; it could be a negative property (flagging if > the lines are not present) but that runs the risk of breaking systems > if a driver acquires the ability to support extra data lines but a > system doesn't have it. > > This should be a generic property for all quad devices to use, though, > since the same thing applies everywhere. Full ACK, "m25p,dual-read" and "m25p,quad-read" sound like a good prop names? Best regards, Marek Vasut
On Thu, Aug 22, 2013 at 10:29:01PM +0200, Marek Vasut wrote: > > On Thu, Aug 22, 2013 at 12:34:53PM -0700, Brian Norris wrote: > > This should be a generic property for all quad devices to use, though, > > since the same thing applies everywhere. > Full ACK, "m25p,dual-read" and "m25p,quad-read" sound like a good prop names? I was actually thinking something more generic than that - putting the property at the SPI generic bindings level. Though if all flashes with this dual/quad read functionality have the prefix m25p the above would work also, at the minute this does seem to be mostly used by flash (I bet someone's got some DSPs or something though).
Dear Mark Brown, > On Thu, Aug 22, 2013 at 10:29:01PM +0200, Marek Vasut wrote: > > > On Thu, Aug 22, 2013 at 12:34:53PM -0700, Brian Norris wrote: > > > > > > This should be a generic property for all quad devices to use, though, > > > since the same thing applies everywhere. > > > > Full ACK, "m25p,dual-read" and "m25p,quad-read" sound like a good prop > > names? > > I was actually thinking something more generic than that - putting the > property at the SPI generic bindings level. Though if all flashes with > this dual/quad read functionality have the prefix m25p the above would > work also, at the minute this does seem to be mostly used by flash (I > bet someone's got some DSPs or something though). Ah! So you mean the SPI controller would provide information that it can do dual/quad transfers? But then, the additional pins can only be wired to certain chips (controller by certain CS lines). Best regards, Marek Vasut
? 2013?08?23? 03:55, Mark Brown ??: > On Thu, Aug 22, 2013 at 12:34:53PM -0700, Brian Norris wrote: >> On Mon, Aug 19, 2013 at 12:10:01PM +0800, Huang Shijie wrote: > >> Why can't this be detected at runtime? We added a "no fast read" flag to >> the device table, so why not "dual/quad mode supported"? And believe it >> or not, not all m25p80 users have device tree. So it isn't very logical >> to tie this support to device-tree only. > Hi Brian: My only question here is do we need to add some flags , such as "quad read", to the device table? If you think we need to, i will add a patch for it. (Just as Brown said, when the DTS configurates the "m25p,quad-read" property, it means : Both the board and the NOR support the Quad read.) >> What you're really missing from device-tree (and the SPI subystem in >> general) is how to detect those SPI controllers which support dual and >> quad mode transfers, and how to communicate that a particular SPI >> transaction should be performed on 1 or 4 IO lines. We shouldn't have >> this just hacked in via assumptions. > That bit does need to be fixed in this driver, yes. The SPI core now > has quad mode support. yes, i do not need these bits in this driver. thanks Huang Shijie
Hi, Shijie 2013/8/19 Huang Shijie <b32955@freescale.com>: > This patch adds the quad read support: > > (1) Add the relative commands: > OPCODE_QIOR, OPCODE_4QIOR, OPCODE_RDCR, > > also add the relative macro for the Configuartion Register. > > (2) add the "m25p,quad-read" property for the m25p80 driver > If the dts has the "m25p,quad-read" property, the kernel will > set the Quad bit of the configuration register, and when the > setting is suceeded, we set the read opcode with OPCODE_QIOR. > > Signed-off-by: Huang Shijie <b32955@freescale.com> > --- > Documentation/devicetree/bindings/mtd/m25p80.txt | 5 ++ > drivers/mtd/devices/m25p80.c | 51 ++++++++++++++++++++++ > include/linux/mtd/spi-nor.h | 6 +++ > 3 files changed, 62 insertions(+), 0 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt b/Documentation/devicetree/bindings/mtd/m25p80.txt > index 6d3d576..b33313f 100644 > --- a/Documentation/devicetree/bindings/mtd/m25p80.txt > +++ b/Documentation/devicetree/bindings/mtd/m25p80.txt > @@ -17,6 +17,11 @@ Optional properties: > Refer to your chips' datasheet to check if this is supported > by your chip. > > +- m25p,quad-read : Use the "quad read" opcode to read data from the chip instead > + of the usual "read" opcode. This opcode is not supported by > + all chips and support for it can not be detected at runtime. > + Refer to your chips' datasheet to check if this is supported > + by your chip. > Example: > > flash: m25p80@0 { > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index f3598c1..4bc9b1b 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -103,6 +103,40 @@ static int write_sr(struct m25p *flash, u8 val) > } > > /* > + * Read the configuration register, returning its value in the location > + * Return the configuration register value. > + * Returns negative if error occurred. > + */ > +static int read_cr(struct m25p *flash) > +{ > + u8 code = OPCODE_RDCR; > + int ret; > + u8 val; > + > + ret = spi_write_then_read(flash->spi, &code, 1, &val, 1); > + if (ret < 0) { > + dev_err(&flash->spi->dev, "error %d reading CR\n", ret); > + return ret; > + } > + return val; > +} > + > +/* > + * Write status register and configuration register with 2 bytes > + * The first byte will be written to the status register, while the second byte > + * will be written to the configuration register. > + * Returns negative if error occurred. > + */ > +static int write_sr_cr(struct m25p *flash, u16 val) > +{ > + flash->command[0] = OPCODE_WRSR; > + flash->command[1] = 0; > + flash->command[2] = (val >> 8); > + > + return spi_write(flash->spi, flash->command, 3); > +} > + > +/* > * Set write enable latch with Write Enable command. > * Returns negative if error occurred. > */ > @@ -880,6 +914,8 @@ static int m25p_probe(struct spi_device *spi) > unsigned i; > struct mtd_part_parser_data ppdata; > struct device_node __maybe_unused *np = spi->dev.of_node; > + u16 sr_cr; > + int ret; > > #ifdef CONFIG_MTD_OF_PARTS > if (!of_device_is_available(np)) > @@ -1014,6 +1050,21 @@ static int m25p_probe(struct spi_device *spi) > else > flash->read_opcode = OPCODE_NORM_READ; > > + /* Try to enable the Quad Read */ > + if (np && of_property_read_bool(np, "m25p,quad-read")) { > + /* The configuration register is set by the second byte. */ > + sr_cr = CR_QUAD << 8; > + > + /* Write the QUAD bit to the Configuration Register. */ > + write_enable(flash); > + if (write_sr_cr(flash, sr_cr) == 0) { > + /* read back and check it */ > + ret = read_cr(flash); > + if (ret > 0 && (ret & CR_QUAD)) > + flash->read_opcode = OPCODE_QIOR; > + } > + } > + Well, M25p80.c support lots of flash devices, so driver should be as general as possible. Firstly not all the devices m25p80 supports set quad mode as your sequence, perhaps write_sr_cr can not match all the m25p80 flash. Secondly, why you only support QIOR(high performance) not QOR or DOR. Maybe QIOR seems too special, so what if user want to use QOR if he set quad mode in DTS. Another point, if command changed to OPCODE_QIOR, there should also should be some correct in m25p_read. such as the number of dummy data. QIOR can support read without read command if set the certain bit in transfer, these aspects did not reflect in your patch. > flash->program_opcode = OPCODE_PP; > > if (info->addr_width) > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h > index b420a5b..d5b189d 100644 > --- a/include/linux/mtd/spi-nor.h > +++ b/include/linux/mtd/spi-nor.h > @@ -39,6 +39,9 @@ > > /* Used for Spansion flashes only. */ > #define OPCODE_BRWR 0x17 /* Bank register write */ > +#define OPCODE_QIOR 0xeb /* Quad read */ > +#define OPCODE_4QIOR 0xec /* Quad read */ > +#define OPCODE_RDCR 0x35 /* Read configuration register */ > > /* Status Register bits. */ > #define SR_WIP 1 /* Write in progress */ > @@ -49,4 +52,7 @@ > #define SR_BP2 0x10 /* Block protect 2 */ > #define SR_SRWD 0x80 /* SR write protect */ > > +/* Configuration Register bits. */ > +#define CR_QUAD 0x2 /* Quad I/O */ > + > #endif /* __LINUX_MTD_SPI_NOR_H */ > -- > 1.7.1 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-spi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
? 2013?08?23? 17:05, yuhang wang ??: >> + u16 sr_cr; >> > + int ret; >> > >> > #ifdef CONFIG_MTD_OF_PARTS >> > if (!of_device_is_available(np)) >> > @@ -1014,6 +1050,21 @@ static int m25p_probe(struct spi_device *spi) >> > else >> > flash->read_opcode = OPCODE_NORM_READ; >> > >> > + /* Try to enable the Quad Read */ >> > + if (np&& of_property_read_bool(np, "m25p,quad-read")) { >> > + /* The configuration register is set by the second byte. */ >> > + sr_cr = CR_QUAD<< 8; >> > + >> > + /* Write the QUAD bit to the Configuration Register. */ >> > + write_enable(flash); >> > + if (write_sr_cr(flash, sr_cr) == 0) { >> > + /* read back and check it */ >> > + ret = read_cr(flash); >> > + if (ret> 0&& (ret& CR_QUAD)) >> > + flash->read_opcode = OPCODE_QIOR; >> > + } >> > + } >> > + > Well, M25p80.c support lots of flash devices, so driver should be as > general as possible. Firstly not all the devices m25p80 supports set > quad mode as your sequence, perhaps write_sr_cr can not match all the It does not matter the NOR flash supports the write_sr_cr() or not, If the NOR flash does not support the write_sr_cr(), it may fails, and you will not set the OPCODE_QIOR for the m25p80_read. > m25p80 flash. Secondly, why you only support QIOR(high performance) > not QOR or DOR. Maybe QIOR seems too special, so what if user want to > use QOR if he set quad mode in DTS. > Frankly speaking, i am reluctant to support the QIOR, it is a little slow. :) So the the QIOR is lowest speed for QUADSPI controller, and i do not want to support the DOR. In my new version, i add the support for DDR QIOR read which is the double rate of the QIOR. The user should knows if the NOR flash supports the quad-read or not, and set the proper DT. > Another point, if command changed to OPCODE_QIOR, there should also > should be some correct in m25p_read. such as the number of dummy data. I only need to change the read opcode. > QIOR can support read without read command if set the certain bit in > transfer, these aspects did not reflect in your patch. > For the Quadspi, it will handle the dummy by the LUT sequence, such as DDR QUAD read, the LUT sequence will set proper dummy (6 cycles for S25FL128S). I do not need the m25p_read to set the dummy. thanks Huang Shijie
On Fri, Aug 23, 2013 at 01:58:05AM +0200, Marek Vasut wrote: > > I was actually thinking something more generic than that - putting the > > property at the SPI generic bindings level. Though if all flashes with > > this dual/quad read functionality have the prefix m25p the above would > > work also, at the minute this does seem to be mostly used by flash (I > > bet someone's got some DSPs or something though). > Ah! So you mean the SPI controller would provide information that it can do > dual/quad transfers? But then, the additional pins can only be wired to certain > chips (controller by certain CS lines). No, not exactly - I just meant that the property on the child node should be one that's consistent over all chips and could hopefully be implemented in the SPI core as part of instantiating the device in DT. Which probably just means stripping or changing the vendor prefix.
On Friday 23 August 2013 02:55 PM, Huang Shijie wrote: > ? 2013?08?23? 17:05, yuhang wang ??: >>> + u16 sr_cr; >>> > + int ret; >>> > >>> > #ifdef CONFIG_MTD_OF_PARTS >>> > if (!of_device_is_available(np)) >>> > @@ -1014,6 +1050,21 @@ static int m25p_probe(struct spi_device *spi) >>> > else >>> > flash->read_opcode = OPCODE_NORM_READ; >>> > >>> > + /* Try to enable the Quad Read */ >>> > + if (np&& of_property_read_bool(np, "m25p,quad-read")) { >>> > + /* The configuration register is set by the >>> second byte. */ >>> > + sr_cr = CR_QUAD<< 8; >>> > + >>> > + /* Write the QUAD bit to the Configuration >>> Register. */ >>> > + write_enable(flash); >>> > + if (write_sr_cr(flash, sr_cr) == 0) { >>> > + /* read back and check it */ >>> > + ret = read_cr(flash); >>> > + if (ret> 0&& (ret& CR_QUAD)) >>> > + flash->read_opcode = OPCODE_QIOR; >>> > + } >>> > + } >>> > + >> Well, M25p80.c support lots of flash devices, so driver should be as >> general as possible. Firstly not all the devices m25p80 supports set >> quad mode as your sequence, perhaps write_sr_cr can not match all the > It does not matter the NOR flash supports the write_sr_cr() or not, > If the NOR flash does not support the write_sr_cr(), it may fails, and > you will not set the OPCODE_QIOR for the > m25p80_read. > >> m25p80 flash. Secondly, why you only support QIOR(high performance) >> not QOR or DOR. Maybe QIOR seems too special, so what if user want to >> use QOR if he set quad mode in DTS. >> > Frankly speaking, i am reluctant to support the QIOR, it is a little > slow. :) > You should add QOR opcodes also in your patch, so we have the complete set. > So the the QIOR is lowest speed for QUADSPI controller, and i do not > want to support the DOR. > > In my new version, i add the support for DDR QIOR read which is the > double rate of the QIOR. > > The user should knows if the NOR flash supports the quad-read or not, > and set the proper DT. > >> Another point, if command changed to OPCODE_QIOR, there should also >> should be some correct in m25p_read. such as the number of dummy data. > I only need to change the read opcode. >> QIOR can support read without read command if set the certain bit in >> transfer, these aspects did not reflect in your patch. >> > For the Quadspi, it will handle the dummy by the LUT sequence, such as > DDR QUAD read, the LUT sequence will > set proper dummy (6 cycles for S25FL128S). I do not need the m25p_read > to set the dummy. > > > thanks > Huang Shijie > > > > > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
Dear Mark Brown, > On Fri, Aug 23, 2013 at 01:58:05AM +0200, Marek Vasut wrote: > > > I was actually thinking something more generic than that - putting the > > > property at the SPI generic bindings level. Though if all flashes with > > > this dual/quad read functionality have the prefix m25p the above would > > > work also, at the minute this does seem to be mostly used by flash (I > > > bet someone's got some DSPs or something though). > > > > Ah! So you mean the SPI controller would provide information that it can > > do dual/quad transfers? But then, the additional pins can only be wired > > to certain chips (controller by certain CS lines). > > No, not exactly - I just meant that the property on the child node > should be one that's consistent over all chips and could hopefully be > implemented in the SPI core as part of instantiating the device in DT. > Which probably just means stripping or changing the vendor prefix. Ah right, got you now. Thanks! Best regards, Marek Vasut
On 08/22/2013 12:55 PM, Mark Brown wrote: > On Thu, Aug 22, 2013 at 12:34:53PM -0700, Brian Norris wrote: >> What you're really missing from device-tree (and the SPI subystem in >> general) is how to detect those SPI controllers which support dual and >> quad mode transfers, and how to communicate that a particular SPI >> transaction should be performed on 1 or 4 IO lines. We shouldn't have >> this just hacked in via assumptions. > > That bit does need to be fixed in this driver, yes. The SPI core now > has quad mode support. Where? Perhaps I'm missing the obvious, but I don't see SPI core quad support in linux-next or in Linus' tree. Brian
Hi, On Friday 23 August 2013 04:53 PM, Brian Norris wrote: > On 08/22/2013 12:55 PM, Mark Brown wrote: >> On Thu, Aug 22, 2013 at 12:34:53PM -0700, Brian Norris wrote: >>> What you're really missing from device-tree (and the SPI subystem in >>> general) is how to detect those SPI controllers which support dual and >>> quad mode transfers, and how to communicate that a particular SPI >>> transaction should be performed on 1 or 4 IO lines. We shouldn't have >>> this just hacked in via assumptions. >> >> That bit does need to be fixed in this driver, yes. The SPI core now >> has quad mode support. > > Where? Perhaps I'm missing the obvious, but I don't see SPI core quad > support in linux-next or in Linus' tree. > You can find it here in Mark's tree: git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git branch: remotes/brownspi/spi-next > Brian > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Fri, Aug 23, 2013 at 04:23:44AM -0700, Brian Norris wrote: > On 08/22/2013 12:55 PM, Mark Brown wrote: > >That bit does need to be fixed in this driver, yes. The SPI core now > >has quad mode support. > Where? Perhaps I'm missing the obvious, but I don't see SPI core > quad support in linux-next or in Linus' tree. It went in yesterday, -next doesn't seem to have been updated today (I suspect Stephen may be on vacation, I'd need to check his announcement mail from yesterday to confirm). The SPI tree is: git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
On 08/23/2013 02:41 AM, Mark Brown wrote: > On Fri, Aug 23, 2013 at 01:58:05AM +0200, Marek Vasut wrote: >>> I was actually thinking something more generic than that - putting the >>> property at the SPI generic bindings level. Though if all flashes with >>> this dual/quad read functionality have the prefix m25p the above would >>> work also, at the minute this does seem to be mostly used by flash (I >>> bet someone's got some DSPs or something though). > >> Ah! So you mean the SPI controller would provide information that it can do >> dual/quad transfers? But then, the additional pins can only be wired to certain >> chips (controller by certain CS lines). > > No, not exactly - I just meant that the property on the child node > should be one that's consistent over all chips and could hopefully be > implemented in the SPI core as part of instantiating the device in DT. > Which probably just means stripping or changing the vendor prefix. (Now that I've been pointed to the support merged into the SPI tree...) Aren't the following new DT properties (for the SPI slave) sufficient? spi-rx-nbits spi-tx-nbits We can leave the detection of which flash chips support which modes to software (m25p80.c) where it belongs, IMO. They're already in the following commit: commit f477b7fb13df2b843997559ff34e87d054ba6538 Author: wangyuhang <wangyuhang2014@gmail.com> Date: Sun Aug 11 18:15:17 2013 +0800 spi: DUAL and QUAD support fix the previous patch some mistake below: 1. DT in slave node, use "spi-tx-nbits = <1/2/4>" in place of using "spi-tx-dual, spi-tx-quad" directly, same to rx. So correct the previous way to get the property in @of_register_spi_devices(). 2. Change the value of transfer bit macro(SPI_NBITS_SINGLE, SPI_NBITS_DUAL SPI_NBITS_QUAD) to 0x01, 0x02 and 0x04 to match the actual wires. 3. Add the following check (1)keep the tx_nbits and rx_nbits in spi_transfer is not beyond the single, dual and quad. (2)keep tx_nbits and rx_nbits are contained by @spi_device->mode example: if @spi_device->mode = DUAL, then tx/rx_nbits can not be set to QUAD(SPI_NBITS_QUAD) (3)if "@spi_device->mode & SPI_3WIRE", then tx/rx_nbits should be in single(SPI_NBITS_SINGLE) Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> Signed-off-by: Mark Brown <broonie@linaro.org> Brian
On 08/23/2013 04:46 AM, Brian Norris wrote: > (Now that I've been pointed to the support merged into the SPI tree...) > > Aren't the following new DT properties (for the SPI slave) sufficient? > > spi-rx-nbits > spi-tx-nbits ... > They're already in the following commit: > > commit f477b7fb13df2b843997559ff34e87d054ba6538 > Author: wangyuhang <wangyuhang2014@gmail.com> > Date: Sun Aug 11 18:15:17 2013 +0800 > > spi: DUAL and QUAD support > > fix the previous patch some mistake below: > 1. DT in slave node, use "spi-tx-nbits = <1/2/4>" in place of using > "spi-tx-dual, spi-tx-quad" directly, same to rx. So correct the > previous way to get the property in @of_register_spi_devices(). > 2. Change the value of transfer bit macro(SPI_NBITS_SINGLE, SPI_NBITS_DUAL > SPI_NBITS_QUAD) to 0x01, 0x02 and 0x04 to match the actual wires. > 3. Add the following check > (1)keep the tx_nbits and rx_nbits in spi_transfer is not beyond the > single, dual and quad. > (2)keep tx_nbits and rx_nbits are contained by @spi_device->mode > example: if @spi_device->mode = DUAL, then tx/rx_nbits can not be set > to QUAD(SPI_NBITS_QUAD) > (3)if "@spi_device->mode & SPI_3WIRE", then tx/rx_nbits should be in > single(SPI_NBITS_SINGLE) > > Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> > Signed-off-by: Mark Brown <broonie@linaro.org> Speaking of which, the new device-tree properties are not documented in Documentation/devicetree/bindings/spi/spi-bus.txt yet. Brian
On Fri, Aug 23, 2013 at 04:53:09AM -0700, Brian Norris wrote: > On 08/23/2013 04:46 AM, Brian Norris wrote: > > Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> > > Signed-off-by: Mark Brown <broonie@linaro.org> > Speaking of which, the new device-tree properties are not documented > in Documentation/devicetree/bindings/spi/spi-bus.txt yet. Yes, indeed - if you look at the review you'll see I asked for a followup patch doing that.
Hi, 2013/8/23 Brian Norris <computersforpeace@gmail.com>: > On 08/23/2013 04:46 AM, Brian Norris wrote: >> >> (Now that I've been pointed to the support merged into the SPI tree...) >> >> Aren't the following new DT properties (for the SPI slave) sufficient? >> >> spi-rx-nbits >> spi-tx-nbits > > ... > >> They're already in the following commit: >> >> commit f477b7fb13df2b843997559ff34e87d054ba6538 >> Author: wangyuhang <wangyuhang2014@gmail.com> >> Date: Sun Aug 11 18:15:17 2013 +0800 >> >> spi: DUAL and QUAD support >> >> fix the previous patch some mistake below: >> 1. DT in slave node, use "spi-tx-nbits = <1/2/4>" in place of using >> "spi-tx-dual, spi-tx-quad" directly, same to rx. So correct the >> previous way to get the property in @of_register_spi_devices(). >> 2. Change the value of transfer bit macro(SPI_NBITS_SINGLE, >> SPI_NBITS_DUAL >> SPI_NBITS_QUAD) to 0x01, 0x02 and 0x04 to match the actual wires. >> 3. Add the following check >> (1)keep the tx_nbits and rx_nbits in spi_transfer is not beyond >> the >> single, dual and quad. >> (2)keep tx_nbits and rx_nbits are contained by @spi_device->mode >> example: if @spi_device->mode = DUAL, then tx/rx_nbits can not >> be set >> to QUAD(SPI_NBITS_QUAD) >> (3)if "@spi_device->mode & SPI_3WIRE", then tx/rx_nbits should be >> in >> single(SPI_NBITS_SINGLE) >> >> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> >> Signed-off-by: Mark Brown <broonie@linaro.org> > > > Speaking of which, the new device-tree properties are not documented in > Documentation/devicetree/bindings/spi/spi-bus.txt yet. > > Brian Sorry, because I don't have the entire environment in my own PC, so I will update the document patch as soon as possible when I go back to company.
Hi, Shijie 2013/8/23 Huang Shijie <b32955@freescale.com>: > ? 2013?08?23? 17:05, yuhang wang ??: >>> >>> + u16 sr_cr; >>> > + int ret; >>> > >>> > #ifdef CONFIG_MTD_OF_PARTS >>> > if (!of_device_is_available(np)) >>> > @@ -1014,6 +1050,21 @@ static int m25p_probe(struct spi_device *spi) >>> > else >>> > flash->read_opcode = OPCODE_NORM_READ; >>> > >>> > + /* Try to enable the Quad Read */ >>> > + if (np&& of_property_read_bool(np, "m25p,quad-read")) { >>> >>> > + /* The configuration register is set by the second >>> > byte. */ >>> > + sr_cr = CR_QUAD<< 8; >>> > + >>> > + /* Write the QUAD bit to the Configuration Register. >>> > */ >>> > + write_enable(flash); >>> > + if (write_sr_cr(flash, sr_cr) == 0) { >>> > + /* read back and check it */ >>> > + ret = read_cr(flash); >>> > + if (ret> 0&& (ret& CR_QUAD)) >>> >>> > + flash->read_opcode = OPCODE_QIOR; >>> > + } >>> > + } >>> > + >> >> Well, M25p80.c support lots of flash devices, so driver should be as >> general as possible. Firstly not all the devices m25p80 supports set >> quad mode as your sequence, perhaps write_sr_cr can not match all the > > It does not matter the NOR flash supports the write_sr_cr() or not, > If the NOR flash does not support the write_sr_cr(), it may fails, and you > will not set the OPCODE_QIOR for the > m25p80_read. > > So your purpose of the patch is to make m25p80 support quad read or just support QIOR? if it's the previous one, when set quad support in DT, but it is possible that quad mode set failed and m25p80 driver still read in single mode. In such case, user won't get any error message, so user won't know what transfer mode the flash works in. Or you just aimed to support QIOR, so the name in DT(quad read) seems not appropriate. >> m25p80 flash. Secondly, why you only support QIOR(high performance) >> not QOR or DOR. Maybe QIOR seems too special, so what if user want to >> use QOR if he set quad mode in DTS. >> > Frankly speaking, i am reluctant to support the QIOR, it is a little slow. > :) > > So the the QIOR is lowest speed for QUADSPI controller, and i do not want to > support the DOR. > > In my new version, i add the support for DDR QIOR read which is the double > rate of the QIOR. > > The user should knows if the NOR flash supports the quad-read or not, and > set the proper DT. > > It is slow in your spi system, but to m25p80 it should be general. Maybe some others will use that function. So I think it is better to supplement the other operations. >> Another point, if command changed to OPCODE_QIOR, there should also >> should be some correct in m25p_read. such as the number of dummy data. > > I only need to change the read opcode. > >> QIOR can support read without read command if set the certain bit in >> transfer, these aspects did not reflect in your patch. >> > For the Quadspi, it will handle the dummy by the LUT sequence, such as DDR > QUAD read, the LUT sequence will > set proper dummy (6 cycles for S25FL128S). I do not need the m25p_read to > set the dummy. > > Also the same point to above, it is OK to your spi controller, but your current m25p80 patch can not content others. If I don't have the SPI controller which support LUT sequence, so my spi controller driver rely on the info that m25p80 provides, then your patch won't work. Best regards
On Saturday 24 August 2013 08:15 AM, Huang Shijie wrote: > On Fri, Aug 23, 2013 at 03:27:43PM +0530, Sourav Poddar wrote: >> On Friday 23 August 2013 02:55 PM, Huang Shijie wrote: >>> ? 2013?08?23? 17:05, yuhang wang ??: >>> >> You should add QOR opcodes also in your patch, so we have the complete set. > yes, i have already supported the QIOR opcode in my driver. > I meant opcode 0x6b Quad output read which spansion flash supports. > thanks > Huang Shijie
On Fri, Aug 23, 2013 at 03:27:43PM +0530, Sourav Poddar wrote: > On Friday 23 August 2013 02:55 PM, Huang Shijie wrote: > >? 2013?08?23? 17:05, yuhang wang ??: > > > You should add QOR opcodes also in your patch, so we have the complete set. yes, i have already supported the QIOR opcode in my driver. thanks Huang Shijie
On Fri, Aug 23, 2013 at 09:59:09PM +0800, yuhang wang wrote: > Hi, Shijie > > >>> > + > >> > >> Well, M25p80.c support lots of flash devices, so driver should be as > >> general as possible. Firstly not all the devices m25p80 supports set > >> quad mode as your sequence, perhaps write_sr_cr can not match all the > > > > It does not matter the NOR flash supports the write_sr_cr() or not, > > If the NOR flash does not support the write_sr_cr(), it may fails, and you > > will not set the OPCODE_QIOR for the > > m25p80_read. > > > > > So your purpose of the patch is to make m25p80 support quad read or > just support QIOR? if it's the previous one, when set quad support in The patch makes the m25p80 could supports the Quad read. it is okay if the quad-read mode set fails. > DT, but it is possible that quad mode set failed and m25p80 driver > still read in single mode. In such case, user won't get any error For the Quadspi driver, if the quad read mode set failed, it will still run in the Fast Read mode. > message, so user won't know what transfer mode the flash works in. Or do you need a warning when the quad read set fails? > you just aimed to support QIOR, so the name in DT(quad read) seems not > appropriate. sorry, could you explain a little more? how can make the DT seems more appropriate ? thanks > > >> m25p80 flash. Secondly, why you only support QIOR(high performance) > >> not QOR or DOR. Maybe QIOR seems too special, so what if user want to > >> use QOR if he set quad mode in DTS. > >> > > Frankly speaking, i am reluctant to support the QIOR, it is a little slow. > > :) > > > > So the the QIOR is lowest speed for QUADSPI controller, and i do not want to > > support the DOR. > > > > In my new version, i add the support for DDR QIOR read which is the double > > rate of the QIOR. > > > > The user should knows if the NOR flash supports the quad-read or not, and > > set the proper DT. > > > > > It is slow in your spi system, but to m25p80 it should be general. > Maybe some others will use that function. So I think it is better to > supplement the other operations. it is the other driver's responsibility to add the bits info or the dummy info to the m25p80 code. > > >> Another point, if command changed to OPCODE_QIOR, there should also > >> should be some correct in m25p_read. such as the number of dummy data. > > > > I only need to change the read opcode. > > > >> QIOR can support read without read command if set the certain bit in > >> transfer, these aspects did not reflect in your patch. > >> > > For the Quadspi, it will handle the dummy by the LUT sequence, such as DDR > > QUAD read, the LUT sequence will > > set proper dummy (6 cycles for S25FL128S). I do not need the m25p_read to > > set the dummy. > > > > > Also the same point to above, it is OK to your spi controller, but > your current m25p80 patch can not content others. If I don't have the > SPI controller which support LUT sequence, so my spi controller driver > rely on the info that m25p80 provides, then your patch won't work. dido. you can submit a patch to fix this issue if your SPI controller can not works on this patch. It is over-design for this patch set to add the bits info/dummy info to the m25p80 code. thanks Huang Shijie
diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt b/Documentation/devicetree/bindings/mtd/m25p80.txt index 6d3d576..b33313f 100644 --- a/Documentation/devicetree/bindings/mtd/m25p80.txt +++ b/Documentation/devicetree/bindings/mtd/m25p80.txt @@ -17,6 +17,11 @@ Optional properties: Refer to your chips' datasheet to check if this is supported by your chip. +- m25p,quad-read : Use the "quad read" opcode to read data from the chip instead + of the usual "read" opcode. This opcode is not supported by + all chips and support for it can not be detected at runtime. + Refer to your chips' datasheet to check if this is supported + by your chip. Example: flash: m25p80@0 { diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index f3598c1..4bc9b1b 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -103,6 +103,40 @@ static int write_sr(struct m25p *flash, u8 val) } /* + * Read the configuration register, returning its value in the location + * Return the configuration register value. + * Returns negative if error occurred. + */ +static int read_cr(struct m25p *flash) +{ + u8 code = OPCODE_RDCR; + int ret; + u8 val; + + ret = spi_write_then_read(flash->spi, &code, 1, &val, 1); + if (ret < 0) { + dev_err(&flash->spi->dev, "error %d reading CR\n", ret); + return ret; + } + return val; +} + +/* + * Write status register and configuration register with 2 bytes + * The first byte will be written to the status register, while the second byte + * will be written to the configuration register. + * Returns negative if error occurred. + */ +static int write_sr_cr(struct m25p *flash, u16 val) +{ + flash->command[0] = OPCODE_WRSR; + flash->command[1] = 0; + flash->command[2] = (val >> 8); + + return spi_write(flash->spi, flash->command, 3); +} + +/* * Set write enable latch with Write Enable command. * Returns negative if error occurred. */ @@ -880,6 +914,8 @@ static int m25p_probe(struct spi_device *spi) unsigned i; struct mtd_part_parser_data ppdata; struct device_node __maybe_unused *np = spi->dev.of_node; + u16 sr_cr; + int ret; #ifdef CONFIG_MTD_OF_PARTS if (!of_device_is_available(np)) @@ -1014,6 +1050,21 @@ static int m25p_probe(struct spi_device *spi) else flash->read_opcode = OPCODE_NORM_READ; + /* Try to enable the Quad Read */ + if (np && of_property_read_bool(np, "m25p,quad-read")) { + /* The configuration register is set by the second byte. */ + sr_cr = CR_QUAD << 8; + + /* Write the QUAD bit to the Configuration Register. */ + write_enable(flash); + if (write_sr_cr(flash, sr_cr) == 0) { + /* read back and check it */ + ret = read_cr(flash); + if (ret > 0 && (ret & CR_QUAD)) + flash->read_opcode = OPCODE_QIOR; + } + } + flash->program_opcode = OPCODE_PP; if (info->addr_width) diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index b420a5b..d5b189d 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -39,6 +39,9 @@ /* Used for Spansion flashes only. */ #define OPCODE_BRWR 0x17 /* Bank register write */ +#define OPCODE_QIOR 0xeb /* Quad read */ +#define OPCODE_4QIOR 0xec /* Quad read */ +#define OPCODE_RDCR 0x35 /* Read configuration register */ /* Status Register bits. */ #define SR_WIP 1 /* Write in progress */ @@ -49,4 +52,7 @@ #define SR_BP2 0x10 /* Block protect 2 */ #define SR_SRWD 0x80 /* SR write protect */ +/* Configuration Register bits. */ +#define CR_QUAD 0x2 /* Quad I/O */ + #endif /* __LINUX_MTD_SPI_NOR_H */
This patch adds the quad read support: (1) Add the relative commands: OPCODE_QIOR, OPCODE_4QIOR, OPCODE_RDCR, also add the relative macro for the Configuartion Register. (2) add the "m25p,quad-read" property for the m25p80 driver If the dts has the "m25p,quad-read" property, the kernel will set the Quad bit of the configuration register, and when the setting is suceeded, we set the read opcode with OPCODE_QIOR. Signed-off-by: Huang Shijie <b32955@freescale.com> --- Documentation/devicetree/bindings/mtd/m25p80.txt | 5 ++ drivers/mtd/devices/m25p80.c | 51 ++++++++++++++++++++++ include/linux/mtd/spi-nor.h | 6 +++ 3 files changed, 62 insertions(+), 0 deletions(-)