Hi,

On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:

This patch provides to enabled the prints on erase and write functions to make sure that how many bytes erase/write into flash device.

Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com

drivers/mtd/spi/spi_flash.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 00aece9..464c2ab 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -115,7 +115,7 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, byte_addr = 0; }

•   debug("SF: program %s %zu bytes @ %#x\n",
  

•   printf("SF: program %s %zu bytes @ %#x\n",
          ret ? "failure" : "success", len, offset);
  

I don't think we want this - it will make programming very slow and verbose.

    spi_release_bus(flash->spi);

@@ -235,7 +235,7 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) goto out; }

•   debug("SF: Successfully erased %zu bytes @ %#x\n", len, start);
  

•   printf("SF: Successfully erased %zu bytes @ %#x\n", len, start);
  

You may want to put this code into cmd_sf instead, where it is reasonable to add messages. You are changing core spi code which might be used from many places.

out: spi_release_bus(flash->spi); -- 1.7.0.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon

Hi Jagan,

On Mon, Dec 10, 2012 at 10:37 PM, Jagan Teki jagannadh.teki@gmail.com wrote:

Hi Simon,

I understand your concern.

But currently there is no prints a/f reading/writing/erasing the SPI flash. User's are unable to confirm whether that particular sf commands are properly done/not.

Well if there is no error, then I suppose it worked ok. We should definitely print errors, and progress information can sometimes be useful for very long operations. But serial and LCD output is slow, so it can affect run time, quite apart from the verbosity, so IMO the less the better.

Would it not be possible to put this message into cmd_sf.c?

Regards, Simon

Thanks, Jagan.

On Tue, Dec 11, 2012 at 12:56 AM, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:

...

This patch provides to enabled the prints on erase and write functions to make sure that how many bytes erase/write into flash device.

Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com

drivers/mtd/spi/spi_flash.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 00aece9..464c2ab 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -115,7 +115,7 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, byte_addr = 0; }

•   debug("SF: program %s %zu bytes @ %#x\n",
  

•   printf("SF: program %s %zu bytes @ %#x\n",
          ret ? "failure" : "success", len, offset);
  

I don't think we want this - it will make programming very slow and verbose.

...

    spi_release_bus(flash->spi);

@@ -235,7 +235,7 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) goto out; }

•   debug("SF: Successfully erased %zu bytes @ %#x\n", len, start);
  

•   printf("SF: Successfully erased %zu bytes @ %#x\n", len, start);
  

You may want to put this code into cmd_sf instead, where it is reasonable to add messages. You are changing core spi code which might be used from many places.

...

out: spi_release_bus(flash->spi); -- 1.7.0.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon

Hi Jagan,

On Wed, Dec 12, 2012 at 6:43 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

Hi Simon,

On Wed, Dec 12, 2012 at 12:01 PM, Simon Glass sjg@chromium.org wrote:

...

Hi Jagan,

On Mon, Dec 10, 2012 at 10:37 PM, Jagan Teki jagannadh.teki@gmail.com wrote:

...

Hi Simon,

I understand your concern.

But currently there is no prints a/f reading/writing/erasing the SPI flash. User's are unable to confirm whether that particular sf commands are properly done/not.

Well if there is no error, then I suppose it worked ok. We should definitely print errors, and progress information can sometimes be useful for very long operations. But serial and LCD output is slow, so it can affect run time, quite apart from the verbosity, so IMO the less the better.

Would it not be possible to put this message into cmd_sf.c?

Yes it will possible to do this on cmd_sf. But I am not understanding what is the side effect, if we put in this area.. will you please elaborate.

Well if someone writes some code that calls the spi_flash interface from else where, such as:

http://patchwork.ozlabs.org/patch/190164/

or defines CONFIG_ENV_IS_IN_SPI_FLASH

then your patch will start printing messages when none are required.

By putting it in cmd_sf.c, and perhaps evening making it optional through a CONFIG_SF_VERBOSE, you make it possible for people to keep the existing behavior.

Regards, Simon

Thanks, Jagan.

...

Regards, Simon

...

Thanks, Jagan.

On Tue, Dec 11, 2012 at 12:56 AM, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:

...

This patch provides to enabled the prints on erase and write functions to make sure that how many bytes erase/write into flash device.

Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com

drivers/mtd/spi/spi_flash.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 00aece9..464c2ab 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -115,7 +115,7 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, byte_addr = 0; }

•   debug("SF: program %s %zu bytes @ %#x\n",
  

•   printf("SF: program %s %zu bytes @ %#x\n",
          ret ? "failure" : "success", len, offset);
  

I don't think we want this - it will make programming very slow and verbose.

...

    spi_release_bus(flash->spi);

@@ -235,7 +235,7 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) goto out; }

•   debug("SF: Successfully erased %zu bytes @ %#x\n", len, start);
  

•   printf("SF: Successfully erased %zu bytes @ %#x\n", len, start);
  

You may want to put this code into cmd_sf instead, where it is reasonable to add messages. You are changing core spi code which might be used from many places.

...

out: spi_release_bus(flash->spi); -- 1.7.0.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon

+Mike who is the maintainer

On Wed, Dec 12, 2012 at 7:16 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

Hi Simon,

On Wed, Dec 12, 2012 at 8:31 PM, Simon Glass sjg@chromium.org wrote:

...

Hi Jagan,

On Wed, Dec 12, 2012 at 6:43 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

...

Hi Simon,

On Wed, Dec 12, 2012 at 12:01 PM, Simon Glass sjg@chromium.org wrote:

...

Hi Jagan,

On Mon, Dec 10, 2012 at 10:37 PM, Jagan Teki jagannadh.teki@gmail.com wrote:

...

Hi Simon,

I understand your concern.

But currently there is no prints a/f reading/writing/erasing the SPI flash. User's are unable to confirm whether that particular sf commands are properly done/not.

Well if there is no error, then I suppose it worked ok. We should definitely print errors, and progress information can sometimes be useful for very long operations. But serial and LCD output is slow, so it can affect run time, quite apart from the verbosity, so IMO the less the better.

Would it not be possible to put this message into cmd_sf.c?

Yes it will possible to do this on cmd_sf. But I am not understanding what is the side effect, if we put in this area.. will you please elaborate.

Well if someone writes some code that calls the spi_flash interface from else where, such as:

http://patchwork.ozlabs.org/patch/190164/

or defines CONFIG_ENV_IS_IN_SPI_FLASH

then your patch will start printing messages when none are required.

By putting it in cmd_sf.c, and perhaps evening making it optional through a CONFIG_SF_VERBOSE, you make it possible for people to keep the existing behavior.

Thanks for your information. Now I understood the whole scenario's.

OK, can I move the prints on cmd_sf.c with CONFIG_SF_VERBOSE?

That would be my preference, but Mike might have thoughts on this.

Regards, Simon

Thanks, Jagan.

...

Regards, Simon

...

Thanks, Jagan.

...

Regards, Simon

...

Thanks, Jagan.

On Tue, Dec 11, 2012 at 12:56 AM, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote: > This patch provides to enabled the prints on erase and write > functions to make sure that how many bytes erase/write into flash > device. > > Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com > --- > drivers/mtd/spi/spi_flash.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c > index 00aece9..464c2ab 100644 > --- a/drivers/mtd/spi/spi_flash.c > +++ b/drivers/mtd/spi/spi_flash.c > @@ -115,7 +115,7 @@ int spi_flash_cmd_write_multi(struct spi_flash > *flash, u32 offset, > byte_addr = 0; > } > > - debug("SF: program %s %zu bytes @ %#x\n", > + printf("SF: program %s %zu bytes @ %#x\n", > ret ? "failure" : "success", len, offset);

I don't think we want this - it will make programming very slow and verbose.

> > spi_release_bus(flash->spi); > @@ -235,7 +235,7 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 > offset, size_t len) > goto out; > } > > - debug("SF: Successfully erased %zu bytes @ %#x\n", len, start); > + printf("SF: Successfully erased %zu bytes @ %#x\n", len, start);

You may want to put this code into cmd_sf instead, where it is reasonable to add messages. You are changing core spi code which might be used from many places.

> > out: > spi_release_bus(flash->spi); > -- > 1.7.0.4 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon

Hi,

On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:

This patch adds a print message on spi_flash_cmd_read_fast() to make sure that how many bytes read from flash device.

Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com

I have the same verbosity comment on this patch, BTW.

Regards, Simon

---

drivers/mtd/spi/spi_flash.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 464c2ab..800ed8b 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -139,12 +139,17 @@ int spi_flash_cmd_read_fast(struct spi_flash *flash, u32 offset, size_t len, void *data) { u8 cmd[5];

•   int ret;
  
    cmd[0] = CMD_READ_ARRAY_FAST;
    spi_flash_addr(offset, cmd);
    cmd[4] = 0x00;
  
  

•   return spi_flash_read_common(flash, cmd, sizeof(cmd), data, len);
  

•   ret = spi_flash_read_common(flash, cmd, sizeof(cmd), data, len);
  

•   printf("SF: re-program %s %zu bytes @ %#x\n",
  

•           ret ? "failure" : "success", len, offset);
  

•   return ret;
  

}

int spi_flash_cmd_poll_bit(struct spi_flash *flash, unsigned long timeout,

1.7.0.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

On Wed, Dec 12, 2012 at 6:46 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

Hi Simon,

On Wed, Dec 12, 2012 at 12:03 PM, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:

...

This patch adds a print message on spi_flash_cmd_read_fast() to make sure that how many bytes read from flash device.

Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com

I have the same verbosity comment on this patch, BTW.

This is also needs to put it on cmd_sf,c?

I would say so, yes.

Thanks, Jagan.

...

Regards, Simon

...

---

drivers/mtd/spi/spi_flash.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 464c2ab..800ed8b 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -139,12 +139,17 @@ int spi_flash_cmd_read_fast(struct spi_flash *flash, u32 offset, size_t len, void *data) { u8 cmd[5];

•   int ret;
  
    cmd[0] = CMD_READ_ARRAY_FAST;
    spi_flash_addr(offset, cmd);
    cmd[4] = 0x00;
  
  

•   return spi_flash_read_common(flash, cmd, sizeof(cmd), data, len);
  

•   ret = spi_flash_read_common(flash, cmd, sizeof(cmd), data, len);
  

•   printf("SF: re-program %s %zu bytes @ %#x\n",
  

•           ret ? "failure" : "success", len, offset);
  

•   return ret;
  

}

int spi_flash_cmd_poll_bit(struct spi_flash *flash, unsigned long timeout,

1.7.0.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Hi,

On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:

This patch provides support to program a flash config register.

Configuration register contains the control bits used to configure the different configurations and security features of a device.

User need to set these bits through spi_flash_cmd_write_config() based on their usage.

Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com

drivers/mtd/spi/spi_flash.c | 27 +++++++++++++++++++++++++++ drivers/mtd/spi/spi_flash_internal.h | 3 +++ 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 800ed8b..a8f0af0 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -274,6 +274,33 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr) return 0; }

+int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr) +{

•   u8 cmd;
  

•   int ret;
  

•   ret = spi_flash_cmd_write_enable(flash);
  

•   if (ret < 0) {
  

•           debug("SF: enabling write failed\n");
  

•           return ret;
  

•   }
  

•   cmd = CMD_WRITE_STATUS;
  

•   ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);
  

Does this assume a particular endianness? Perhaps should put the values into a byte array instead?

•   if (ret) {
  

•           debug("SF: fail to write config register\n");
  

•           return ret;
  

•   }
  

•   ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
  

•   if (ret < 0) {
  

•           debug("SF: write config register timed out\n");
  

•           return ret;
  

•   }
  

•   return 0;
  

+}

/*

• The following table holds all device probe functions

diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h index 141cfa8..9287778 100644 --- a/drivers/mtd/spi/spi_flash_internal.h +++ b/drivers/mtd/spi/spi_flash_internal.h @@ -77,6 +77,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash) /* Program the status register. */ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);

+/* Program the config register. */ +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr);

/*

• Same as spi_flash_cmd_read() except it also claims/releases the SPI
• bus. Used as common part of the ->read() operation.

-- 1.7.0.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon

Hi Jagan,

On Wed, Dec 12, 2012 at 7:20 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

Hi Simon,

On Wed, Dec 12, 2012 at 12:05 PM, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:

...

This patch provides support to program a flash config register.

Configuration register contains the control bits used to configure the different configurations and security features of a device.

User need to set these bits through spi_flash_cmd_write_config() based on their usage.

Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com

drivers/mtd/spi/spi_flash.c | 27 +++++++++++++++++++++++++++ drivers/mtd/spi/spi_flash_internal.h | 3 +++ 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 800ed8b..a8f0af0 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -274,6 +274,33 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr) return 0; }

+int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr) +{

•   u8 cmd;
  

•   int ret;
  

•   ret = spi_flash_cmd_write_enable(flash);
  

•   if (ret < 0) {
  

•           debug("SF: enabling write failed\n");
  

•           return ret;
  

•   }
  

•   cmd = CMD_WRITE_STATUS;
  

•   ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);
  

Does this assume a particular endianness? Perhaps should put the values into a byte array instead?

Yes it follows the endianness.

My concern was more that u16 is being used to send two bytes. How about:

u8 data[2];

put_unaligned_le16(cr, data)

On my QPP patch, ret = spi_flash_cmd_write_config(flash, 0x0200);

where 02 is config and 00 is status register WRR have CMD+status+config => for CMD+16 data format.

Let me know if you need any info.

OK.

Regards, Simon

Thanks, Jagan.

...

...

•   if (ret) {
  

•           debug("SF: fail to write config register\n");
  

•           return ret;
  

•   }
  

•   ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
  

•   if (ret < 0) {
  

•           debug("SF: write config register timed out\n");
  

•           return ret;
  

•   }
  

•   return 0;
  

+}

/*

• The following table holds all device probe functions

diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h index 141cfa8..9287778 100644 --- a/drivers/mtd/spi/spi_flash_internal.h +++ b/drivers/mtd/spi/spi_flash_internal.h @@ -77,6 +77,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash) /* Program the status register. */ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);

+/* Program the config register. */ +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr);

/*

• Same as spi_flash_cmd_read() except it also claims/releases the SPI
• bus. Used as common part of the ->read() operation.

-- 1.7.0.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon

Hi Jagan,

On Wed, Dec 12, 2012 at 8:21 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

Hi Simon,

On Wed, Dec 12, 2012 at 8:53 PM, Simon Glass sjg@chromium.org wrote:

...

Hi Jagan,

On Wed, Dec 12, 2012 at 7:20 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

...

Hi Simon,

On Wed, Dec 12, 2012 at 12:05 PM, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:

...

This patch provides support to program a flash config register.

Configuration register contains the control bits used to configure the different configurations and security features of a device.

User need to set these bits through spi_flash_cmd_write_config() based on their usage.

Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com

drivers/mtd/spi/spi_flash.c | 27 +++++++++++++++++++++++++++ drivers/mtd/spi/spi_flash_internal.h | 3 +++ 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 800ed8b..a8f0af0 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -274,6 +274,33 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr) return 0; }

+int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr) +{

•   u8 cmd;
  

•   int ret;
  

•   ret = spi_flash_cmd_write_enable(flash);
  

•   if (ret < 0) {
  

•           debug("SF: enabling write failed\n");
  

•           return ret;
  

•   }
  

•   cmd = CMD_WRITE_STATUS;
  

•   ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);
  

Does this assume a particular endianness? Perhaps should put the values into a byte array instead?

Yes it follows the endianness.

My concern was more that u16 is being used to send two bytes. How about:

u8 data[2];

Means I need to send status on data[1] and config on data[0].?

...

put_unaligned_le16(cr, data)

I couldn't understand this, may be for endianness.?

Yes that's right. Just checking that your code will work correctly on a big-endian machine. Will it? It is normally not a good idea to cast a short into a char[2].

Regards, Simon

Thanks, Jagan.

...

...

On my QPP patch, ret = spi_flash_cmd_write_config(flash, 0x0200);

where 02 is config and 00 is status register WRR have CMD+status+config => for CMD+16 data format.

Let me know if you need any info.

OK.

Regards, Simon

...

Thanks, Jagan.

...

...

•   if (ret) {
  

•           debug("SF: fail to write config register\n");
  

•           return ret;
  

•   }
  

•   ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
  

•   if (ret < 0) {
  

•           debug("SF: write config register timed out\n");
  

•           return ret;
  

•   }
  

•   return 0;
  

+}

/*

• The following table holds all device probe functions

diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h index 141cfa8..9287778 100644 --- a/drivers/mtd/spi/spi_flash_internal.h +++ b/drivers/mtd/spi/spi_flash_internal.h @@ -77,6 +77,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash) /* Program the status register. */ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);

+/* Program the config register. */ +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr);

/*

• Same as spi_flash_cmd_read() except it also claims/releases the SPI
• bus. Used as common part of the ->read() operation.

-- 1.7.0.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon

Hi Jagan,

On Thu, Dec 13, 2012 at 8:08 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

Hi Simon,

On Thu, Dec 13, 2012 at 4:03 AM, Simon Glass sjg@chromium.org wrote:

...

Hi Jagan,

On Wed, Dec 12, 2012 at 8:21 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

...

Hi Simon,

On Wed, Dec 12, 2012 at 8:53 PM, Simon Glass sjg@chromium.org wrote:

...

Hi Jagan,

On Wed, Dec 12, 2012 at 7:20 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

...

Hi Simon,

On Wed, Dec 12, 2012 at 12:05 PM, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote: > This patch provides support to program a flash config register. > > Configuration register contains the control bits used to configure > the different configurations and security features of a device. > > User need to set these bits through spi_flash_cmd_write_config() > based on their usage. > > Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com > --- > drivers/mtd/spi/spi_flash.c | 27 +++++++++++++++++++++++++++ > drivers/mtd/spi/spi_flash_internal.h | 3 +++ > 2 files changed, 30 insertions(+), 0 deletions(-) > > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c > index 800ed8b..a8f0af0 100644 > --- a/drivers/mtd/spi/spi_flash.c > +++ b/drivers/mtd/spi/spi_flash.c > @@ -274,6 +274,33 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr) > return 0; > } > > +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr) > +{ > + u8 cmd; > + int ret; > + > + ret = spi_flash_cmd_write_enable(flash); > + if (ret < 0) { > + debug("SF: enabling write failed\n"); > + return ret; > + } > + > + cmd = CMD_WRITE_STATUS; > + ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);

Does this assume a particular endianness? Perhaps should put the values into a byte array instead?

Yes it follows the endianness.

My concern was more that u16 is being used to send two bytes. How about:

u8 data[2];

Means I need to send status on data[1] and config on data[0].?

...

put_unaligned_le16(cr, data)

I couldn't understand this, may be for endianness.?

Yes that's right. Just checking that your code will work correctly on a big-endian machine. Will it? It is normally not a good idea to cast a short into a char[2].

OK.

Let me explain my senario here. in below function I am passing 0x0200 spi_flash_cmd_write_config(flash, 0x0200);

out of 2-byte data 0x0200 first 00 will pass as a status register value and 0x02 will pass as a config value.it's took the bytes from LSB onwards.

Can you please explain how put_unaligned_le16(cr, data will work for this case?

Possibly...

The prototype is:

int spi_flash_cmd_write(struct spi_slave *spi, const u8 *cmd, size_t cmd_len, const void *data, size_t data_len);

You are doing:

u16 value;

ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);

Passing a u16 cast to a u8 * is probably not endian safe.

Instead I think you should do:

u8 cr[2];

(get bytes into cr) ret = spi_flash_cmd_write(flash->spi, &cmd, 1, cr, 2);

Regards, Simon

Thanks, Jagan.

...

Regards, Simon

...

Thanks, Jagan.

...

...

On my QPP patch, ret = spi_flash_cmd_write_config(flash, 0x0200);

where 02 is config and 00 is status register WRR have CMD+status+config => for CMD+16 data format.

Let me know if you need any info.

OK.

Regards, Simon

...

Thanks, Jagan.

...

> + if (ret) { > + debug("SF: fail to write config register\n"); > + return ret; > + } > + > + ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT); > + if (ret < 0) { > + debug("SF: write config register timed out\n"); > + return ret; > + } > + > + return 0; > +} > + > /* > * The following table holds all device probe functions > * > diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h > index 141cfa8..9287778 100644 > --- a/drivers/mtd/spi/spi_flash_internal.h > +++ b/drivers/mtd/spi/spi_flash_internal.h > @@ -77,6 +77,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash) > /* Program the status register. */ > int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr); > > +/* Program the config register. */ > +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr); > + > /* > * Same as spi_flash_cmd_read() except it also claims/releases the SPI > * bus. Used as common part of the ->read() operation. > -- > 1.7.0.4 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon

Hello Jagannadha,

I have some remarks and questions, as I currently work on a hw platform, which also would allow to support dual- or quad-io accesses.

So my first question: why is this restricted to write only? If you have a hardware, which is capable of supporting this, the read will definitely benefit from it, while the speedup for write depends on the internal programming time of the flash.

The other questions: On which hardware platform was this tested and for which flashes? Patches for both components are missing from this patch series. And for both I have more remarks below.

Jagannadha Sutradharudu Teki wrote on 2012-12-10:

This patch provides support to program a flash using Quad-input Page Program(32h) instruction.

This will effectively increases the data transfer rate by up to four times, as compared to the Page Program(PP) instruction.

Respective flash drivers need to use spi_flash_cmd_write_multi_qpp() based on their usage.

+#ifdef CONFIG_CMD_SF_QPP

• else if (strcmp(argv[0], "write.qpp") == 0)

• ret = spi_flash_write_qpp(flash, offset, len, buf);
  

+#endif

Is it really necessary to have a dedicated command here? Wouldn't it be better, if the SF layer or the driver will use it automatically, if the hardware supports it and the driver has detected the feature of the flash?

+#ifdef CONFIG_CMD_SF_QPP +int spi_flash_set_quad_enable_bit(struct spi_flash *flash) +{

[...]

• if (data & 0x2) {

• debug("SF: quad enable bit is already set.\n");
  

Is this bit common for all flashes? Otherwise add some comments on tested flashed and/or TODO for an extension to provide this info from the flash driver.

• return ret;
  

• } else {

• debug("SF: need to set quad enable bit\n");
  

• ret = spi_flash_cmd_write_config(flash, 0x0200);
  

Same here. And why is this a 16-bit value here?

+int spi_flash_cmd_write_multi_qpp(struct spi_flash *flash, u32 offset,

• size_t len, const void *buf)
  

+{

• unsigned long page_addr, byte_addr, page_size;
• size_t chunk_len, actual;
• int ret;
• u8 cmd[4];
• page_size = flash->page_size;
• page_addr = offset / page_size;
• byte_addr = offset % page_size;
• ret = spi_claim_bus(flash->spi);
• if (ret) {

• debug("SF: unable to claim SPI bus\n");
  

• return ret;
  

• }
• ret = spi_flash_set_quad_enable_bit(flash);

I don't like this implicit setting here. And as far as I know, this bit is sticky/non-volatile. It is not necessary to write this each time. Maybe it make more sense to have an interactive command to write this bit (enabled or disabled) to the flash? And then the flash probe function can check the bit and map to the appropriate read and write functions.

• if (ret) {

• debug("SF: set quad enable bit failed\n");
  

• return ret;
  

• }
• cmd[0] = CMD_QUAD_PAGE_PROGRAM;
• for (actual = 0; actual < len; actual += chunk_len) {

• chunk_len = min(len - actual, page_size - byte_addr);
  

• cmd[1] = page_addr >> 8;
  

• cmd[2] = page_addr;
  

• cmd[3] = byte_addr;
  

• debug("PP: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x }
  

chunk_len = %zu\n",

•       buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len);
  

• ret = spi_flash_cmd_write_enable(flash);
  

• if (ret < 0) {
  

• 	debug("SF: enabling write failed\n");
  

• 	break;
  

• }
  

• ret = spi_flash_cmd_write(flash->spi, cmd, 4,
  

• 			  buf + actual, chunk_len);
  

• if (ret < 0) {
  

• 	debug("SF: write failed\n");
  

• 	break;
  

• }
  

Except for the config bit and the different command code, I don't see any difference to the "regular" spi_flash_cmd_write_multi function. So the question is: How does the SPI framework knows when to send 4 bits in parallel to 4 pins instead of the serialization to one signal pin?

This extension to the SPI framework is completely missing!

diff --git a/include/spi_flash.h b/include/spi_flash.h index 9da9062..61f0c19 100644 --- a/include/spi_flash.h +++ b/include/spi_flash.h @@ -43,6 +43,10 @@ struct spi_flash { size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, size_t len, const void *buf); +#ifdef CONFIG_CMD_SF_QPP

• int (*write_qpp)(struct spi_flash *flash, u32 offset,

• 		size_t len, const void *buf);
  

The flash probe should detect, if QPP is possible, and then map the existing (*read) and (*write) pointers to the relevant functions. No new pointers required here.

+#endif int (*erase)(struct spi_flash *flash, u32 offset, size_t len); };

+#ifdef CONFIG_CMD_SF_QPP +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset,

• size_t len, const void *buf)
  

+{

• return flash->write_qpp(flash, offset, len, buf);

+} +#endif

See above, this is also not required.

I would be happy to support you by adapting and testing this extension on my platform.

The plan on my side is to send the pieces to support my platform in the near future (not sure if my schedule will fit for the next merge window, but anyway..)

Adding this feature on top would be very nice ;-)

Best Regards, Thomas

Hi,

On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:

This patch provides support to program a flash using Quad-input Page Program(32h) instruction.

This will effectively increases the data transfer rate by up to four times, as compared to the Page Program(PP) instruction.

Respective flash drivers need to use spi_flash_cmd_write_multi_qpp() based on their usage.

Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com

It's great to have this support. A few comments below.

---

README | 1 + common/cmd_sf.c | 12 +++- drivers/mtd/spi/spi_flash.c | 108 ++++++++++++++++++++++++++++++++++ drivers/mtd/spi/spi_flash_internal.h | 13 ++++ include/spi_flash.h | 12 ++++ 5 files changed, 144 insertions(+), 2 deletions(-)

diff --git a/README b/README index 5a86ae9..a01de13 100644 --- a/README +++ b/README @@ -869,6 +869,7 @@ The following options need to be configured: CONFIG_CMD_SETGETDCR Support for DCR Register access (4xx only) CONFIG_CMD_SF * Read/write/erase SPI NOR flash

•           CONFIG_CMD_SF_QPP       * Program SPI flash using Quad-input Page Program
            CONFIG_CMD_SHA1SUM        print sha1 memory digest
                                      (requires CONFIG_CMD_MEMORY)
            CONFIG_CMD_SOURCE         "source" command Support
  

diff --git a/common/cmd_sf.c b/common/cmd_sf.c index 5ac1d0c..a449d2c 100644 --- a/common/cmd_sf.c +++ b/common/cmd_sf.c @@ -228,6 +228,10 @@ static int do_spi_flash_read_write(int argc, char * const argv[]) ret = spi_flash_update(flash, offset, len, buf); else if (strcmp(argv[0], "read") == 0) ret = spi_flash_read(flash, offset, len, buf); +#ifdef CONFIG_CMD_SF_QPP

•   else if (strcmp(argv[0], "write.qpp") == 0)
  

•           ret = spi_flash_write_qpp(flash, offset, len, buf);
  

+#endif else ret = spi_flash_write(flash, offset, len, buf);

@@ -300,7 +304,7 @@ static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ }

    if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0 ||

•       strcmp(cmd, "update") == 0)
  

•       strcmp(cmd, "update") == 0 || strcmp(cmd, "write.qpp") == 0)
            ret = do_spi_flash_read_write(argc, argv);
    else if (strcmp(cmd, "erase") == 0)
            ret = do_spi_flash_erase(argc, argv);
  

@@ -327,5 +331,9 @@ U_BOOT_CMD( "sf erase offset [+]len - erase `len' bytes from `offset'\n" " `+len' round up `len' to block size\n" "sf update addr offset len - erase and write `len' bytes from memory\n"

•   "                                 at `addr' to flash at `offset'"
  

•   "                                 at `addr' to flash at `offset'\n"
  

+#ifdef CONFIG_CMD_SF_QPP

•   "sf write.qpp addr offset len   - write `len' bytes from memory\n"
  

•   "                                 at `addr' to flash at `offset' using Quad-input Page Program(32h)"
  

+#endif ); diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index a8f0af0..3545f59 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -122,6 +122,114 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, return ret; }

+#ifdef CONFIG_CMD_SF_QPP +int spi_flash_set_quad_enable_bit(struct spi_flash *flash) +{

•   u8 cmd = CMD_READ_CONFIG;
  

•   u8 data = 0;
  

•   int ret;
  

•   ret = spi_flash_read_common(flash,
  

•                   &cmd, sizeof(cmd), &data, sizeof(data));
  

•   if (ret < 0) {
  

•           debug("SF: fail to read config register\n");
  

•           return ret;
  

•   }
  

•   if (data & 0x2) {
  

Can we have defines/enums for this please?

•           debug("SF: quad enable bit is already set.\n");
  

•           return ret;
  

•   } else {
  

•           debug("SF: need to set quad enable bit\n");
  

•           ret = spi_flash_cmd_write_config(flash, 0x0200);
  

and here?

•           if (ret < 0) {
  

•                   debug("SF: fail to write quad enable bit\n");
  

•                   return ret;
  

•           }
  

•           ret = spi_flash_read_common(flash,
  

•                           &cmd, sizeof(cmd), &data, sizeof(data));
  

•           if (ret < 0) {
  

•                   debug("SF: fail to read config register\n");
  

•                   return ret;
  

•           }
  

It almost seems like you could have a loop here: read it, return if ok, write it, then loop again (up to two times). Might reduce the code length, but perhaps your way is better.

•           if (data & 0x2)
  

•                   debug("SF: successfully set quad enable bit\n");
  

•           else {
  

•                   printf("SF: fail to set quad enable bit\n");
  

•                   return -1;
  

•           }
  

•   }
  

•   return ret;
  

+}

+int spi_flash_cmd_write_multi_qpp(struct spi_flash *flash, u32 offset,

•           size_t len, const void *buf)
  

+{

•   unsigned long page_addr, byte_addr, page_size;
  

•   size_t chunk_len, actual;
  

•   int ret;
  

•   u8 cmd[4];
  

•   page_size = flash->page_size;
  

•   page_addr = offset / page_size;
  

•   byte_addr = offset % page_size;
  

•   ret = spi_claim_bus(flash->spi);
  

•   if (ret) {
  

•           debug("SF: unable to claim SPI bus\n");
  

•           return ret;
  

•   }
  

•   ret = spi_flash_set_quad_enable_bit(flash);
  

•   if (ret) {
  

•           debug("SF: set quad enable bit failed\n");
  

•           return ret;
  

•   }
  

•   cmd[0] = CMD_QUAD_PAGE_PROGRAM;
  

•   for (actual = 0; actual < len; actual += chunk_len) {
  

•           chunk_len = min(len - actual, page_size - byte_addr);
  

•           cmd[1] = page_addr >> 8;
  

•           cmd[2] = page_addr;
  

•           cmd[3] = byte_addr;
  

•           debug("PP: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = %zu\n",
  

•                 buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len);
  

•           ret = spi_flash_cmd_write_enable(flash);
  

•           if (ret < 0) {
  

•                   debug("SF: enabling write failed\n");
  

•                   break;
  

•           }
  

•           ret = spi_flash_cmd_write(flash->spi, cmd, 4,
  

•                                     buf + actual, chunk_len);
  

•           if (ret < 0) {
  

•                   debug("SF: write failed\n");
  

•                   break;
  

•           }
  

•           ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
  

•           if (ret)
  

•                   break;
  

•           page_addr++;
  

•           byte_addr = 0;
  

This seems very similar to the existing write function. Can you pull out the common code?

•   }
  

•   printf("SF: program %s %zu bytes @ %#x\n",
  

•         ret ? "failure" : "success", len, offset);
  

I think this should go in the cmdline code, not here. I may have mentioned that already :-)

•   spi_release_bus(flash->spi);
  

•   return ret;
  

+} +#endif

int spi_flash_read_common(struct spi_flash *flash, const u8 *cmd, size_t cmd_len, void *data, size_t data_len) { diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h index 9287778..cb157ea 100644 --- a/drivers/mtd/spi/spi_flash_internal.h +++ b/drivers/mtd/spi/spi_flash_internal.h @@ -20,8 +20,10 @@

#define CMD_WRITE_STATUS 0x01 #define CMD_PAGE_PROGRAM 0x02 +#define CMD_QUAD_PAGE_PROGRAM 0x32 #define CMD_WRITE_DISABLE 0x04 #define CMD_READ_STATUS 0x05 +#define CMD_READ_CONFIG 0x35 #define CMD_WRITE_ENABLE 0x06 #define CMD_ERASE_4K 0x20 #define CMD_ERASE_32K 0x52 @@ -54,10 +56,21 @@ int spi_flash_cmd_write(struct spi_slave *spi, const u8 *cmd, size_t cmd_len, /*

• Write the requested data out breaking it up into multiple write
• commands as needed per the write size.

• • Programming a flash using Page Program(PP, 02h)
  */

int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, size_t len, const void *buf);

+#ifdef CONFIG_CMD_SF_QPP +/*

• • Write the requested data out breaking it up into multiple write
• • commands as needed per the write size.
• • Programming a flash using Quad-input Page Program(QPP, 32h)
• */

+int spi_flash_cmd_write_multi_qpp(struct spi_flash *flash, u32 offset,

•           size_t len, const void *buf);
  

+#endif

/*

• Enable writing on the SPI flash.

*/ diff --git a/include/spi_flash.h b/include/spi_flash.h index 9da9062..61f0c19 100644 --- a/include/spi_flash.h +++ b/include/spi_flash.h @@ -43,6 +43,10 @@ struct spi_flash { size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, size_t len, const void *buf); +#ifdef CONFIG_CMD_SF_QPP

•   int             (*write_qpp)(struct spi_flash *flash, u32 offset,
  

•                           size_t len, const void *buf);
  

+#endif

This is ok, but it almost feels like we should add a flags parameter here?

    int             (*erase)(struct spi_flash *flash, u32 offset,
                            size_t len);

}; @@ -63,6 +67,14 @@ static inline int spi_flash_write(struct spi_flash *flash, u32 offset, return flash->write(flash, offset, len, buf); }

+#ifdef CONFIG_CMD_SF_QPP +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset,

•           size_t len, const void *buf)
  

+{

•   return flash->write_qpp(flash, offset, len, buf);
  

+} +#endif

static inline int spi_flash_erase(struct spi_flash *flash, u32 offset, size_t len) { -- 1.7.0.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon

Hi Jagan,

On Wed, Dec 12, 2012 at 8:52 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

Hi Simon,

On Wed, Dec 12, 2012 at 12:11 PM, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:

...

This patch provides support to program a flash using Quad-input Page Program(32h) instruction.

This will effectively increases the data transfer rate by up to four times, as compared to the Page Program(PP) instruction.

Respective flash drivers need to use spi_flash_cmd_write_multi_qpp() based on their usage.

Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com

It's great to have this support. A few comments below.

Thanks.

...

...

---

README | 1 + common/cmd_sf.c | 12 +++- drivers/mtd/spi/spi_flash.c | 108 ++++++++++++++++++++++++++++++++++ drivers/mtd/spi/spi_flash_internal.h | 13 ++++ include/spi_flash.h | 12 ++++ 5 files changed, 144 insertions(+), 2 deletions(-)

diff --git a/README b/README index 5a86ae9..a01de13 100644 --- a/README +++ b/README @@ -869,6 +869,7 @@ The following options need to be configured: CONFIG_CMD_SETGETDCR Support for DCR Register access (4xx only) CONFIG_CMD_SF * Read/write/erase SPI NOR flash

•           CONFIG_CMD_SF_QPP       * Program SPI flash using Quad-input Page Program
            CONFIG_CMD_SHA1SUM        print sha1 memory digest
                                      (requires CONFIG_CMD_MEMORY)
            CONFIG_CMD_SOURCE         "source" command Support
  

diff --git a/common/cmd_sf.c b/common/cmd_sf.c index 5ac1d0c..a449d2c 100644 --- a/common/cmd_sf.c +++ b/common/cmd_sf.c @@ -228,6 +228,10 @@ static int do_spi_flash_read_write(int argc, char * const argv[]) ret = spi_flash_update(flash, offset, len, buf); else if (strcmp(argv[0], "read") == 0) ret = spi_flash_read(flash, offset, len, buf); +#ifdef CONFIG_CMD_SF_QPP

•   else if (strcmp(argv[0], "write.qpp") == 0)
  

•           ret = spi_flash_write_qpp(flash, offset, len, buf);
  

+#endif else ret = spi_flash_write(flash, offset, len, buf);

@@ -300,7 +304,7 @@ static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ }

    if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0 ||

•       strcmp(cmd, "update") == 0)
  

•       strcmp(cmd, "update") == 0 || strcmp(cmd, "write.qpp") == 0)
            ret = do_spi_flash_read_write(argc, argv);
    else if (strcmp(cmd, "erase") == 0)
            ret = do_spi_flash_erase(argc, argv);
  

@@ -327,5 +331,9 @@ U_BOOT_CMD( "sf erase offset [+]len - erase `len' bytes from `offset'\n" " `+len' round up `len' to block size\n" "sf update addr offset len - erase and write `len' bytes from memory\n"

•   "                                 at `addr' to flash at `offset'"
  

•   "                                 at `addr' to flash at `offset'\n"
  

+#ifdef CONFIG_CMD_SF_QPP

•   "sf write.qpp addr offset len   - write `len' bytes from memory\n"
  

•   "                                 at `addr' to flash at `offset' using Quad-input Page Program(32h)"
  

+#endif ); diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index a8f0af0..3545f59 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -122,6 +122,114 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, return ret; }

+#ifdef CONFIG_CMD_SF_QPP +int spi_flash_set_quad_enable_bit(struct spi_flash *flash) +{

•   u8 cmd = CMD_READ_CONFIG;
  

•   u8 data = 0;
  

•   int ret;
  

•   ret = spi_flash_read_common(flash,
  

•                   &cmd, sizeof(cmd), &data, sizeof(data));
  

•   if (ret < 0) {
  

•           debug("SF: fail to read config register\n");
  

•           return ret;
  

•   }
  

•   if (data & 0x2) {
  

Can we have defines/enums for this please?

Yes I will do.

...

...

•           debug("SF: quad enable bit is already set.\n");
  

•           return ret;
  

•   } else {
  

•           debug("SF: need to set quad enable bit\n");
  

•           ret = spi_flash_cmd_write_config(flash, 0x0200);
  

and here?

Ok.

...

...

•           if (ret < 0) {
  

•                   debug("SF: fail to write quad enable bit\n");
  

•                   return ret;
  

•           }
  

•           ret = spi_flash_read_common(flash,
  

•                           &cmd, sizeof(cmd), &data, sizeof(data));
  

•           if (ret < 0) {
  

•                   debug("SF: fail to read config register\n");
  

•                   return ret;
  

•           }
  

It almost seems like you could have a loop here: read it, return if ok, write it, then loop again (up to two times). Might reduce the code length, but perhaps your way is better.

I just check the config reg for quad bit if it is set already, return. otherwise set the bit and read it again.

Any better way we need to do?

Very rough code:

for (pass = 0; pass < 2; pass++) { spi_flash_read_common() if (data & 0x2) return 0; spi_flash_cmd_write_config(flash, 0x0200); } debug("could not set it...."); return -some error;

Please ignore this if you can't make it work, just trying to reduce duplication.

...

...

•           if (data & 0x2)
  

•                   debug("SF: successfully set quad enable bit\n");
  

•           else {
  

•                   printf("SF: fail to set quad enable bit\n");
  

•                   return -1;
  

•           }
  

•   }
  

•   return ret;
  

+}

+int spi_flash_cmd_write_multi_qpp(struct spi_flash *flash, u32 offset,

•           size_t len, const void *buf)
  

+{

•   unsigned long page_addr, byte_addr, page_size;
  

•   size_t chunk_len, actual;
  

•   int ret;
  

•   u8 cmd[4];
  

•   page_size = flash->page_size;
  

•   page_addr = offset / page_size;
  

•   byte_addr = offset % page_size;
  

•   ret = spi_claim_bus(flash->spi);
  

•   if (ret) {
  

•           debug("SF: unable to claim SPI bus\n");
  

•           return ret;
  

•   }
  

•   ret = spi_flash_set_quad_enable_bit(flash);
  

•   if (ret) {
  

•           debug("SF: set quad enable bit failed\n");
  

•           return ret;
  

•   }
  

•   cmd[0] = CMD_QUAD_PAGE_PROGRAM;
  

•   for (actual = 0; actual < len; actual += chunk_len) {
  

•           chunk_len = min(len - actual, page_size - byte_addr);
  

•           cmd[1] = page_addr >> 8;
  

•           cmd[2] = page_addr;
  

•           cmd[3] = byte_addr;
  

•           debug("PP: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = %zu\n",
  

•                 buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len);
  

•           ret = spi_flash_cmd_write_enable(flash);
  

•           if (ret < 0) {
  

•                   debug("SF: enabling write failed\n");
  

•                   break;
  

•           }
  

•           ret = spi_flash_cmd_write(flash->spi, cmd, 4,
  

•                                     buf + actual, chunk_len);
  

•           if (ret < 0) {
  

•                   debug("SF: write failed\n");
  

•                   break;
  

•           }
  

•           ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
  

•           if (ret)
  

•                   break;
  

•           page_addr++;
  

•           byte_addr = 0;
  

This seems very similar to the existing write function. Can you pull out the common code?

Yes, most of the code is redundant.. But my plan will short it up in near future a/f more testing. (not touch the existing write call as of now)

make sense?

OK, in that case you should probably do your testing after you shorten the code, and then submit again? Or are you just looking for overall comments so far? Overall to me it seems like a useful feature and we should have it.

...

...

•   }
  

•   printf("SF: program %s %zu bytes @ %#x\n",
  

•         ret ? "failure" : "success", len, offset);
  

I think this should go in the cmdline code, not here. I may have mentioned that already :-)

...

•   spi_release_bus(flash->spi);
  

•   return ret;
  

+} +#endif

int spi_flash_read_common(struct spi_flash *flash, const u8 *cmd, size_t cmd_len, void *data, size_t data_len) { diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h index 9287778..cb157ea 100644 --- a/drivers/mtd/spi/spi_flash_internal.h +++ b/drivers/mtd/spi/spi_flash_internal.h @@ -20,8 +20,10 @@

#define CMD_WRITE_STATUS 0x01 #define CMD_PAGE_PROGRAM 0x02 +#define CMD_QUAD_PAGE_PROGRAM 0x32 #define CMD_WRITE_DISABLE 0x04 #define CMD_READ_STATUS 0x05 +#define CMD_READ_CONFIG 0x35 #define CMD_WRITE_ENABLE 0x06 #define CMD_ERASE_4K 0x20 #define CMD_ERASE_32K 0x52 @@ -54,10 +56,21 @@ int spi_flash_cmd_write(struct spi_slave *spi, const u8 *cmd, size_t cmd_len, /*

• Write the requested data out breaking it up into multiple write
• commands as needed per the write size.

• • Programming a flash using Page Program(PP, 02h)
  */

int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, size_t len, const void *buf);

+#ifdef CONFIG_CMD_SF_QPP +/*

• • Write the requested data out breaking it up into multiple write
• • commands as needed per the write size.
• • Programming a flash using Quad-input Page Program(QPP, 32h)
• */

+int spi_flash_cmd_write_multi_qpp(struct spi_flash *flash, u32 offset,

•           size_t len, const void *buf);
  

+#endif

/*

• Enable writing on the SPI flash.

*/ diff --git a/include/spi_flash.h b/include/spi_flash.h index 9da9062..61f0c19 100644 --- a/include/spi_flash.h +++ b/include/spi_flash.h @@ -43,6 +43,10 @@ struct spi_flash { size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, size_t len, const void *buf); +#ifdef CONFIG_CMD_SF_QPP

•   int             (*write_qpp)(struct spi_flash *flash, u32 offset,
  

•                           size_t len, const void *buf);
  

+#endif

This is ok, but it almost feels like we should add a flags parameter here?

I couldn't understand the flag parameter here..can u elaborate?

Well the only difference between the standard write() and your write_qpp() is the status write and the command byte that is sent. Is that right?

If so, then perhaps a flags word as a 5th parameter to write() would make some sense - you could have a QUAD flag. But there is a lot of code to change, so perhaps we should wait until there is a second need for flags.

Regards, Simon

Thanks, Jagan.

...

...

    int             (*erase)(struct spi_flash *flash, u32 offset,
                            size_t len);

}; @@ -63,6 +67,14 @@ static inline int spi_flash_write(struct spi_flash *flash, u32 offset, return flash->write(flash, offset, len, buf); }

+#ifdef CONFIG_CMD_SF_QPP +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset,

•           size_t len, const void *buf)
  

+{

•   return flash->write_qpp(flash, offset, len, buf);
  

+} +#endif

static inline int spi_flash_erase(struct spi_flash *flash, u32 offset, size_t len) { -- 1.7.0.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon

+Mike

Hi,

On Thu, Dec 13, 2012 at 8:21 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

Hi Simon,

On Thu, Dec 13, 2012 at 4:08 AM, Simon Glass sjg@chromium.org wrote:

...

Hi Jagan,

On Wed, Dec 12, 2012 at 8:52 AM, Jagan Teki jagannadh.teki@gmail.com wrote:

...

Hi Simon,

On Wed, Dec 12, 2012 at 12:11 PM, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com wrote:

...

This patch provides support to program a flash using Quad-input Page Program(32h) instruction.

This will effectively increases the data transfer rate by up to four times, as compared to the Page Program(PP) instruction.

Respective flash drivers need to use spi_flash_cmd_write_multi_qpp() based on their usage.

Signed-off-by: Jagannadha Sutradharudu Teki jagannadh.teki@gmail.com

It's great to have this support. A few comments below.

Thanks.

...

...

---

README | 1 + common/cmd_sf.c | 12 +++- drivers/mtd/spi/spi_flash.c | 108 ++++++++++++++++++++++++++++++++++ drivers/mtd/spi/spi_flash_internal.h | 13 ++++ include/spi_flash.h | 12 ++++ 5 files changed, 144 insertions(+), 2 deletions(-)

diff --git a/README b/README index 5a86ae9..a01de13 100644 --- a/README +++ b/README @@ -869,6 +869,7 @@ The following options need to be configured: CONFIG_CMD_SETGETDCR Support for DCR Register access (4xx only) CONFIG_CMD_SF * Read/write/erase SPI NOR flash

•           CONFIG_CMD_SF_QPP       * Program SPI flash using Quad-input Page Program
            CONFIG_CMD_SHA1SUM        print sha1 memory digest
                                      (requires CONFIG_CMD_MEMORY)
            CONFIG_CMD_SOURCE         "source" command Support
  

diff --git a/common/cmd_sf.c b/common/cmd_sf.c index 5ac1d0c..a449d2c 100644 --- a/common/cmd_sf.c +++ b/common/cmd_sf.c @@ -228,6 +228,10 @@ static int do_spi_flash_read_write(int argc, char * const argv[]) ret = spi_flash_update(flash, offset, len, buf); else if (strcmp(argv[0], "read") == 0) ret = spi_flash_read(flash, offset, len, buf); +#ifdef CONFIG_CMD_SF_QPP

•   else if (strcmp(argv[0], "write.qpp") == 0)
  

•           ret = spi_flash_write_qpp(flash, offset, len, buf);
  

+#endif else ret = spi_flash_write(flash, offset, len, buf);

@@ -300,7 +304,7 @@ static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ }

    if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0 ||

•       strcmp(cmd, "update") == 0)
  

•       strcmp(cmd, "update") == 0 || strcmp(cmd, "write.qpp") == 0)
            ret = do_spi_flash_read_write(argc, argv);
    else if (strcmp(cmd, "erase") == 0)
            ret = do_spi_flash_erase(argc, argv);
  

@@ -327,5 +331,9 @@ U_BOOT_CMD( "sf erase offset [+]len - erase `len' bytes from `offset'\n" " `+len' round up `len' to block size\n" "sf update addr offset len - erase and write `len' bytes from memory\n"

•   "                                 at `addr' to flash at `offset'"
  

•   "                                 at `addr' to flash at `offset'\n"
  

+#ifdef CONFIG_CMD_SF_QPP

•   "sf write.qpp addr offset len   - write `len' bytes from memory\n"
  

•   "                                 at `addr' to flash at `offset' using Quad-input Page Program(32h)"
  

+#endif ); diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index a8f0af0..3545f59 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -122,6 +122,114 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, return ret; }

+#ifdef CONFIG_CMD_SF_QPP +int spi_flash_set_quad_enable_bit(struct spi_flash *flash) +{

•   u8 cmd = CMD_READ_CONFIG;
  

•   u8 data = 0;
  

•   int ret;
  

•   ret = spi_flash_read_common(flash,
  

•                   &cmd, sizeof(cmd), &data, sizeof(data));
  

•   if (ret < 0) {
  

•           debug("SF: fail to read config register\n");
  

•           return ret;
  

•   }
  

•   if (data & 0x2) {
  

Can we have defines/enums for this please?

Yes I will do.

...

...

•           debug("SF: quad enable bit is already set.\n");
  

•           return ret;
  

•   } else {
  

•           debug("SF: need to set quad enable bit\n");
  

•           ret = spi_flash_cmd_write_config(flash, 0x0200);
  

and here?

Ok.

...

...

•           if (ret < 0) {
  

•                   debug("SF: fail to write quad enable bit\n");
  

•                   return ret;
  

•           }
  

•           ret = spi_flash_read_common(flash,
  

•                           &cmd, sizeof(cmd), &data, sizeof(data));
  

•           if (ret < 0) {
  

•                   debug("SF: fail to read config register\n");
  

•                   return ret;
  

•           }
  

It almost seems like you could have a loop here: read it, return if ok, write it, then loop again (up to two times). Might reduce the code length, but perhaps your way is better.

I just check the config reg for quad bit if it is set already, return. otherwise set the bit and read it again.

Any better way we need to do?

Very rough code:

for (pass = 0; pass < 2; pass++) { spi_flash_read_common() if (data & 0x2) return 0; spi_flash_cmd_write_config(flash, 0x0200); } debug("could not set it...."); return -some error;

I liked this logic, may be I will try.

...

Please ignore this if you can't make it work, just trying to reduce duplication.

...

...

...

•           if (data & 0x2)
  

•                   debug("SF: successfully set quad enable bit\n");
  

•           else {
  

•                   printf("SF: fail to set quad enable bit\n");
  

•                   return -1;
  

•           }
  

•   }
  

•   return ret;
  

+}

+int spi_flash_cmd_write_multi_qpp(struct spi_flash *flash, u32 offset,

•           size_t len, const void *buf)
  

+{

•   unsigned long page_addr, byte_addr, page_size;
  

•   size_t chunk_len, actual;
  

•   int ret;
  

•   u8 cmd[4];
  

•   page_size = flash->page_size;
  

•   page_addr = offset / page_size;
  

•   byte_addr = offset % page_size;
  

•   ret = spi_claim_bus(flash->spi);
  

•   if (ret) {
  

•           debug("SF: unable to claim SPI bus\n");
  

•           return ret;
  

•   }
  

•   ret = spi_flash_set_quad_enable_bit(flash);
  

•   if (ret) {
  

•           debug("SF: set quad enable bit failed\n");
  

•           return ret;
  

•   }
  

•   cmd[0] = CMD_QUAD_PAGE_PROGRAM;
  

•   for (actual = 0; actual < len; actual += chunk_len) {
  

•           chunk_len = min(len - actual, page_size - byte_addr);
  

•           cmd[1] = page_addr >> 8;
  

•           cmd[2] = page_addr;
  

•           cmd[3] = byte_addr;
  

•           debug("PP: 0x%p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = %zu\n",
  

•                 buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len);
  

•           ret = spi_flash_cmd_write_enable(flash);
  

•           if (ret < 0) {
  

•                   debug("SF: enabling write failed\n");
  

•                   break;
  

•           }
  

•           ret = spi_flash_cmd_write(flash->spi, cmd, 4,
  

•                                     buf + actual, chunk_len);
  

•           if (ret < 0) {
  

•                   debug("SF: write failed\n");
  

•                   break;
  

•           }
  

•           ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
  

•           if (ret)
  

•                   break;
  

•           page_addr++;
  

•           byte_addr = 0;
  

This seems very similar to the existing write function. Can you pull out the common code?

Yes, most of the code is redundant.. But my plan will short it up in near future a/f more testing. (not touch the existing write call as of now)

make sense?

OK, in that case you should probably do your testing after you shorten the code, and then submit again? Or are you just looking for overall comments so far? Overall to me it seems like a useful feature and we should have it.

...

...

...

•   }
  

•   printf("SF: program %s %zu bytes @ %#x\n",
  

•         ret ? "failure" : "success", len, offset);
  

I think this should go in the cmdline code, not here. I may have mentioned that already :-)

...

•   spi_release_bus(flash->spi);
  

•   return ret;
  

+} +#endif

int spi_flash_read_common(struct spi_flash *flash, const u8 *cmd, size_t cmd_len, void *data, size_t data_len) { diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h index 9287778..cb157ea 100644 --- a/drivers/mtd/spi/spi_flash_internal.h +++ b/drivers/mtd/spi/spi_flash_internal.h @@ -20,8 +20,10 @@

#define CMD_WRITE_STATUS 0x01 #define CMD_PAGE_PROGRAM 0x02 +#define CMD_QUAD_PAGE_PROGRAM 0x32 #define CMD_WRITE_DISABLE 0x04 #define CMD_READ_STATUS 0x05 +#define CMD_READ_CONFIG 0x35 #define CMD_WRITE_ENABLE 0x06 #define CMD_ERASE_4K 0x20 #define CMD_ERASE_32K 0x52 @@ -54,10 +56,21 @@ int spi_flash_cmd_write(struct spi_slave *spi, const u8 *cmd, size_t cmd_len, /*

• Write the requested data out breaking it up into multiple write
• commands as needed per the write size.

• • Programming a flash using Page Program(PP, 02h)
  */

int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, size_t len, const void *buf);

+#ifdef CONFIG_CMD_SF_QPP +/*

• • Write the requested data out breaking it up into multiple write
• • commands as needed per the write size.
• • Programming a flash using Quad-input Page Program(QPP, 32h)
• */

+int spi_flash_cmd_write_multi_qpp(struct spi_flash *flash, u32 offset,

•           size_t len, const void *buf);
  

+#endif

/*

• Enable writing on the SPI flash.

*/ diff --git a/include/spi_flash.h b/include/spi_flash.h index 9da9062..61f0c19 100644 --- a/include/spi_flash.h +++ b/include/spi_flash.h @@ -43,6 +43,10 @@ struct spi_flash { size_t len, void *buf); int (*write)(struct spi_flash *flash, u32 offset, size_t len, const void *buf); +#ifdef CONFIG_CMD_SF_QPP

•   int             (*write_qpp)(struct spi_flash *flash, u32 offset,
  

•                           size_t len, const void *buf);
  

+#endif

This is ok, but it almost feels like we should add a flags parameter here?

I couldn't understand the flag parameter here..can u elaborate?

Well the only difference between the standard write() and your write_qpp() is the status write and the command byte that is sent. Is that right?

If so, then perhaps a flags word as a 5th parameter to write() would make some sense - you could have a QUAD flag. But there is a lot of code to change, so perhaps we should wait until there is a second need for flags.

OK. I agree with your comments.

BTW. I have an other plan to pass the instruction(PP, QPP) to spi_flash_write with a separate flag and only common standard write call can use for both the instructions. may be this way we can reduce the duplicate.

Some how i have liked this, what do you say..any side effects...?

And also I am planning to create a separate commands for QUAD bit enable and disable.. so if any one use the quad write(qpp) and quad read(will add in future) he should enable the QUAD bit first. please comment?

Actually I wonder whether the single/dual/quad setting should be kept in struct spi_flash, and you could add a new function to set the width in bits. Then the existing read/write functions can use the selected width. That way you don't need to change the read()/write() functions, although you will need to add a new function to set the width.

Regards, Simon

Thanks, Jagan.

...

Regards, Simon

...

Thanks, Jagan.

...

...

    int             (*erase)(struct spi_flash *flash, u32 offset,
                            size_t len);

}; @@ -63,6 +67,14 @@ static inline int spi_flash_write(struct spi_flash *flash, u32 offset, return flash->write(flash, offset, len, buf); }

+#ifdef CONFIG_CMD_SF_QPP +static inline int spi_flash_write_qpp(struct spi_flash *flash, u32 offset,

•           size_t len, const void *buf)
  

+{

•   return flash->write_qpp(flash, offset, len, buf);
  

+} +#endif

static inline int spi_flash_erase(struct spi_flash *flash, u32 offset, size_t len) { -- 1.7.0.4

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

Regards, Simon