| Message ID | 06d144fc-5065-497f-b4a0-dc5481c702a5@CH1EHSMHS039.ehs.local |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
Hello Jagan, Am 23.02.2013 12:39, schrieb Jagannadha Sutradharudu Teki: > This patch provides support to access an extended addressing > in 3-byte address mode. > > The current implementation in spi_flash supports 3-byte address mode > due to this up to 16MB amount of flash is able to access for those > flashes which has an actual size of > 16MB. > > extended/bank address register contains an information to access the > 4th byte addressing hence the flashes which has > 16MB can be accessible. > > Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com> > --- > drivers/mtd/spi/spi_flash.c | 81 ++++++++++++++++++++++++++++++++++ > drivers/mtd/spi/spi_flash_internal.h | 8 +++ > 2 files changed, 89 insertions(+), 0 deletions(-) > > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c > index c168c1c..16e5f59 100644 > --- a/drivers/mtd/spi/spi_flash.c > +++ b/drivers/mtd/spi/spi_flash.c > @@ -23,6 +23,30 @@ static void spi_flash_addr(u32 addr, u8 *cmd) > cmd[3] = addr >> 0; > } > > +static int spi_flash_check_extaddr_access(struct spi_flash *flash, u32 *offset) > +{ > + int ret; > + > + if (*offset >= 0x1000000) { > + ret = spi_flash_extaddr_access(flash, STATUS_EXTADDR_ENABLE); Restricting this to a single bit here would give the next size limit at 32M. Please make it future-prov as much as possible: Why not directly use the upper byte of the offset as parameter? > + if (ret) { > + debug("SF: fail to %s ext addr bit\n", > + STATUS_EXTADDR_ENABLE ? "set" : "reset"); > + return ret; > + } > + *offset -= 0x1000000; Are you sure that manipulating the value of the caller has no side-effect? Is it even necessary, if the callers only do 3-byte-addressing and probably ignore the upper byte? > + } else { > + ret = spi_flash_extaddr_access(flash, STATUS_EXTADDR_DISABLE); > + if (ret) { > + debug("SF: fail to %s ext addr bit\n", > + STATUS_EXTADDR_DISABLE ? "set" : "reset"); > + return ret; > + } > + } > + > + return ret; > +} > + > static int spi_flash_read_write(struct spi_slave *spi, > const u8 *cmd, size_t cmd_len, > const u8 *data_out, u8 *data_in, > @@ -73,6 +97,14 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, > int ret; > u8 cmd[4]; > > + if (flash->size > 0x1000000) { As already said in my comments to patch 1/4, this check (and the check for manufacturer ID) should be done only once once during probing of the flash. Then, if necessary, adapted functions for read, write and erase should be assigned to the function-pointers. Or use an additional function-pointer, which can be set to this or a similar function. Then the call of this function should be included in the loops, which all these functions have. Otherwise, as with your current code, you have a problem if the current accessed block crosses the boundary at 16M. Have you ever tried this? > + ret = spi_flash_check_extaddr_access(flash, &offset); > + if (ret) { > + debug("SF: fail to acess ext_addr\n"); > + return ret; > + } > + } > + > page_size = flash->page_size; > page_addr = offset / page_size; > byte_addr = offset % page_size; > @@ -139,6 +171,15 @@ int spi_flash_cmd_read_fast(struct spi_flash *flash, u32 offset, > size_t len, void *data) > { > u8 cmd[5]; > + int ret; > + > + if (flash->size > 0x1000000) { > + ret = spi_flash_check_extaddr_access(flash, &offset); > + if (ret) { > + debug("SF: fail to acess ext_addr\n"); > + return ret; > + } > + } > > cmd[0] = CMD_READ_ARRAY_FAST; > spi_flash_addr(offset, cmd); > @@ -196,6 +237,14 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) > int ret; > u8 cmd[4]; > > + if (flash->size > 0x1000000) { > + ret = spi_flash_check_extaddr_access(flash, &offset); > + if (ret) { > + debug("SF: fail to acess ext_addr\n"); > + return ret; > + } > + } > + > erase_size = flash->sector_size; > if (offset % erase_size || len % erase_size) { > debug("SF: Erase offset/length not multiple of erase size\n"); > @@ -333,6 +382,38 @@ int spi_flash_cmd_extaddr_read(struct spi_flash *flash, void *data) > return spi_flash_read_common(flash, &cmd, 1, data, 1); > } > > +int spi_flash_extaddr_access(struct spi_flash *flash, u8 status) > +{ > + int ret, pass; > + u8 data = 0, write_done = 0; > + > + for (pass = 0; pass < 2; pass++) { Why this is required?? > + ret = spi_flash_cmd_extaddr_read(flash, (void *)&data); > + if (ret < 0) { > + debug("SF: fail to read ext addr register\n"); > + return ret; > + } > + > + if ((data != status) & !write_done) { > + debug("SF: need to %s ext addr bit\n", > + status ? "set" : "reset"); > + > + write_done = 1; > + ret = spi_flash_cmd_extaddr_write(flash, status); > + if (ret < 0) { > + debug("SF: fail to write ext addr bit\n"); > + return ret; > + } > + } else { > + debug("SF: ext addr bit is %s.\n", > + status ? "set" : "reset"); Instead of reading and writing this register each time (which will happen often, if this function is called in the loops as suggested above), please cache the actual value inside struct spi_flash. Initial read should be done during probing and then writing only if the value needs to change. This is something depending on this design rule: http://www.denx.de/wiki/U-Boot/DesignPrinciples#2_Keep_it_Fast > + return ret; > + } > + } > + > + return -1; > +} > + > /* > * 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 65960ad..a6b04d7 100644 > --- a/drivers/mtd/spi/spi_flash_internal.h > +++ b/drivers/mtd/spi/spi_flash_internal.h > @@ -34,6 +34,8 @@ > > /* Common status */ > #define STATUS_WIP 0x01 > +#define STATUS_EXTADDR_ENABLE 0x01 > +#define STATUS_EXTADDR_DISABLE 0x00 As said above, don't restrict to a single bit! > > /* Send a single-byte command to the device and read the response */ > int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response, size_t len); > @@ -86,6 +88,12 @@ int spi_flash_cmd_extaddr_write(struct spi_flash *flash, u8 ear); > > /* Read the extended address register */ > int spi_flash_cmd_extaddr_read(struct spi_flash *flash, void *data); > +/* > + * Extended address access > + * access 4th byte address in 3-byte addessing mode for flashes > + * which has an actual size of > 16MB. > + */ > +int spi_flash_extaddr_access(struct spi_flash *flash, u8 status); > > /* > * Same as spi_flash_cmd_read() except it also claims/releases the SPI > Best Regards, Thomas
> -----Original Message----- > From: Langer Thomas (LQDE RD ST PON SW) > [mailto:thomas.langer@lantiq.com] > Sent: 24 February 2013 21:21 > To: Jagannadha Sutradharudu Teki; u-boot@lists.denx.de > Cc: Michal@theia.denx.de; Jagannadha Sutradharudu Teki > Subject: AW: [U-Boot] [PATCH 3/4] sf: Add extended address access support > > Hello Jagan, > > Am 23.02.2013 12:39, schrieb Jagannadha Sutradharudu Teki: > > This patch provides support to access an extended addressing in 3-byte > > address mode. > > > > The current implementation in spi_flash supports 3-byte address mode > > due to this up to 16MB amount of flash is able to access for those > > flashes which has an actual size of > 16MB. > > > > extended/bank address register contains an information to access the > > 4th byte addressing hence the flashes which has > 16MB can be accessible. > > > > Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com> > > --- > > drivers/mtd/spi/spi_flash.c | 81 > ++++++++++++++++++++++++++++++++++ > > drivers/mtd/spi/spi_flash_internal.h | 8 +++ > > 2 files changed, 89 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c > > index c168c1c..16e5f59 100644 > > --- a/drivers/mtd/spi/spi_flash.c > > +++ b/drivers/mtd/spi/spi_flash.c > > @@ -23,6 +23,30 @@ static void spi_flash_addr(u32 addr, u8 *cmd) > > cmd[3] = addr >> 0; > > } > > > > +static int spi_flash_check_extaddr_access(struct spi_flash *flash, > > +u32 *offset) { > > + int ret; > > + > > + if (*offset >= 0x1000000) { > > + ret = spi_flash_extaddr_access(flash, > STATUS_EXTADDR_ENABLE); > Restricting this to a single bit here would give the next size limit at 32M. > Please make it future-prov as much as possible: Why not directly use the > upper byte of the offset as parameter? I didn't get you clearly, can you please elaborate. > > > + if (ret) { > > + debug("SF: fail to %s ext addr bit\n", > > + STATUS_EXTADDR_ENABLE ? "set" : "reset"); > > + return ret; > > + } > > + *offset -= 0x1000000; > Are you sure that manipulating the value of the caller has no side-effect? > Is it even necessary, if the callers only do 3-byte-addressing and probably > ignore the upper byte? May be you are not getting my exact code context. From the current implementation on spi, we have 3-byte addressing means we are able to access only up to 16MB--> means 0 to 0x FFFFFF If the flash has 32MB, if the user is giving an offset 0x1000000 it will again pointing to 0x0. Because we have 3-byte addressing able to access only first 16MB of flash but the actual flash size is 32MB. So, from my implementation if user is giving an offset of 0x1000000 then we have enable the bank register for pointing to second 16MB area on 32 MB flash and we must subtract the offset from 16MB as we are using 3-byte addressing. Means we are accessing 4-byte addressing in 3-byte address mode. Ie is the reason I am subtracting it from16MB. Please comment and let me know if you're not clear. > > > + } else { > > + ret = spi_flash_extaddr_access(flash, > STATUS_EXTADDR_DISABLE); > > + if (ret) { > > + debug("SF: fail to %s ext addr bit\n", > > + STATUS_EXTADDR_DISABLE ? "set" : "reset"); > > + return ret; > > + } > > + } > > + > > + return ret; > > +} > > + > > static int spi_flash_read_write(struct spi_slave *spi, > > const u8 *cmd, size_t cmd_len, > > const u8 *data_out, u8 *data_in, @@ -73,6 > +97,14 @@ int > > spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, > > int ret; > > u8 cmd[4]; > > > > + if (flash->size > 0x1000000) { > > As already said in my comments to patch 1/4, this check (and the check for > manufacturer ID) should be done only once once during probing of the flash. > Then, if necessary, adapted functions for read, write and erase should be > assigned to the function-pointers. > Or use an additional function-pointer, which can be set to this or a similar > function. > Then the call of this function should be included in the loops, which all these > functions have. > Otherwise, as with your current code, you have a problem if the current > accessed block crosses the boundary at 16M. Have you ever tried this? Means this check should be in probe call, by adding one member in spi_flash structure, is it?. I am not understanding extra function pointers concept, will you please explain. > > > + ret = spi_flash_check_extaddr_access(flash, &offset); > > + if (ret) { > > + debug("SF: fail to acess ext_addr\n"); > > + return ret; > > + } > > + } > > + > > page_size = flash->page_size; > > page_addr = offset / page_size; > > byte_addr = offset % page_size; > > @@ -139,6 +171,15 @@ int spi_flash_cmd_read_fast(struct spi_flash > *flash, u32 offset, > > size_t len, void *data) > > { > > u8 cmd[5]; > > + int ret; > > + > > + if (flash->size > 0x1000000) { > > + ret = spi_flash_check_extaddr_access(flash, &offset); > > + if (ret) { > > + debug("SF: fail to acess ext_addr\n"); > > + return ret; > > + } > > + } > > > > cmd[0] = CMD_READ_ARRAY_FAST; > > spi_flash_addr(offset, cmd); > > @@ -196,6 +237,14 @@ int spi_flash_cmd_erase(struct spi_flash *flash, > u32 offset, size_t len) > > int ret; > > u8 cmd[4]; > > > > + if (flash->size > 0x1000000) { > > + ret = spi_flash_check_extaddr_access(flash, &offset); > > + if (ret) { > > + debug("SF: fail to acess ext_addr\n"); > > + return ret; > > + } > > + } > > + > > erase_size = flash->sector_size; > > if (offset % erase_size || len % erase_size) { > > debug("SF: Erase offset/length not multiple of erase size\n"); > @@ > > -333,6 +382,38 @@ int spi_flash_cmd_extaddr_read(struct spi_flash *flash, > void *data) > > return spi_flash_read_common(flash, &cmd, 1, data, 1); > > } > > > > +int spi_flash_extaddr_access(struct spi_flash *flash, u8 status) { > > + int ret, pass; > > + u8 data = 0, write_done = 0; > > + > > + for (pass = 0; pass < 2; pass++) { > Why this is required?? > > > + ret = spi_flash_cmd_extaddr_read(flash, (void *)&data); > > + if (ret < 0) { > > + debug("SF: fail to read ext addr register\n"); > > + return ret; > > + } > > + > > + if ((data != status) & !write_done) { > > + debug("SF: need to %s ext addr bit\n", > > + status ? "set" : "reset"); > > + > > + write_done = 1; > > + ret = spi_flash_cmd_extaddr_write(flash, status); > > + if (ret < 0) { > > + debug("SF: fail to write ext addr bit\n"); > > + return ret; > > + } > > + } else { > > + debug("SF: ext addr bit is %s.\n", > > + status ? "set" : "reset"); > Instead of reading and writing this register each time (which will happen > often, if this function is called in the loops as suggested above), please > cache the actual value inside struct spi_flash. > Initial read should be done during probing and then writing only if the value > needs to change. > This is something depending on this design rule: > http://www.denx.de/wiki/U-Boot/DesignPrinciples#2_Keep_it_Fast But reading is also required a/f writing, whether the particular write is happened properly or not? The reason for 2 times loop is for code reduction. The actual flow is first need to read the value and then write and finally read back whether the written value is fine or not. So this entire flow requires 2 reads and 1 write. I am adding loop for reduce the extra read code. Any suggestions. Thanks, Jagan. > > > > + return ret; > > + } > > + } > > + > > + return -1; > > +} > > + > > /* > > * 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 65960ad..a6b04d7 100644 > > --- a/drivers/mtd/spi/spi_flash_internal.h > > +++ b/drivers/mtd/spi/spi_flash_internal.h > > @@ -34,6 +34,8 @@ > > > > /* Common status */ > > #define STATUS_WIP 0x01 > > +#define STATUS_EXTADDR_ENABLE 0x01 > > +#define STATUS_EXTADDR_DISABLE 0x00 > As said above, don't restrict to a single bit! > > > > > /* Send a single-byte command to the device and read the response */ > > int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response, > > size_t len); @@ -86,6 +88,12 @@ int spi_flash_cmd_extaddr_write(struct > > spi_flash *flash, u8 ear); > > > > /* Read the extended address register */ > > int spi_flash_cmd_extaddr_read(struct spi_flash *flash, void *data); > > +/* > > + * Extended address access > > + * access 4th byte address in 3-byte addessing mode for flashes > > + * which has an actual size of > 16MB. > > + */ > > +int spi_flash_extaddr_access(struct spi_flash *flash, u8 status); > > > > /* > > * Same as spi_flash_cmd_read() except it also claims/releases the > > SPI > > > > Best Regards, > Thomas This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index c168c1c..16e5f59 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -23,6 +23,30 @@ static void spi_flash_addr(u32 addr, u8 *cmd) cmd[3] = addr >> 0; } +static int spi_flash_check_extaddr_access(struct spi_flash *flash, u32 *offset) +{ + int ret; + + if (*offset >= 0x1000000) { + ret = spi_flash_extaddr_access(flash, STATUS_EXTADDR_ENABLE); + if (ret) { + debug("SF: fail to %s ext addr bit\n", + STATUS_EXTADDR_ENABLE ? "set" : "reset"); + return ret; + } + *offset -= 0x1000000; + } else { + ret = spi_flash_extaddr_access(flash, STATUS_EXTADDR_DISABLE); + if (ret) { + debug("SF: fail to %s ext addr bit\n", + STATUS_EXTADDR_DISABLE ? "set" : "reset"); + return ret; + } + } + + return ret; +} + static int spi_flash_read_write(struct spi_slave *spi, const u8 *cmd, size_t cmd_len, const u8 *data_out, u8 *data_in, @@ -73,6 +97,14 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset, int ret; u8 cmd[4]; + if (flash->size > 0x1000000) { + ret = spi_flash_check_extaddr_access(flash, &offset); + if (ret) { + debug("SF: fail to acess ext_addr\n"); + return ret; + } + } + page_size = flash->page_size; page_addr = offset / page_size; byte_addr = offset % page_size; @@ -139,6 +171,15 @@ int spi_flash_cmd_read_fast(struct spi_flash *flash, u32 offset, size_t len, void *data) { u8 cmd[5]; + int ret; + + if (flash->size > 0x1000000) { + ret = spi_flash_check_extaddr_access(flash, &offset); + if (ret) { + debug("SF: fail to acess ext_addr\n"); + return ret; + } + } cmd[0] = CMD_READ_ARRAY_FAST; spi_flash_addr(offset, cmd); @@ -196,6 +237,14 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len) int ret; u8 cmd[4]; + if (flash->size > 0x1000000) { + ret = spi_flash_check_extaddr_access(flash, &offset); + if (ret) { + debug("SF: fail to acess ext_addr\n"); + return ret; + } + } + erase_size = flash->sector_size; if (offset % erase_size || len % erase_size) { debug("SF: Erase offset/length not multiple of erase size\n"); @@ -333,6 +382,38 @@ int spi_flash_cmd_extaddr_read(struct spi_flash *flash, void *data) return spi_flash_read_common(flash, &cmd, 1, data, 1); } +int spi_flash_extaddr_access(struct spi_flash *flash, u8 status) +{ + int ret, pass; + u8 data = 0, write_done = 0; + + for (pass = 0; pass < 2; pass++) { + ret = spi_flash_cmd_extaddr_read(flash, (void *)&data); + if (ret < 0) { + debug("SF: fail to read ext addr register\n"); + return ret; + } + + if ((data != status) & !write_done) { + debug("SF: need to %s ext addr bit\n", + status ? "set" : "reset"); + + write_done = 1; + ret = spi_flash_cmd_extaddr_write(flash, status); + if (ret < 0) { + debug("SF: fail to write ext addr bit\n"); + return ret; + } + } else { + debug("SF: ext addr bit is %s.\n", + status ? "set" : "reset"); + return ret; + } + } + + return -1; +} + /* * 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 65960ad..a6b04d7 100644 --- a/drivers/mtd/spi/spi_flash_internal.h +++ b/drivers/mtd/spi/spi_flash_internal.h @@ -34,6 +34,8 @@ /* Common status */ #define STATUS_WIP 0x01 +#define STATUS_EXTADDR_ENABLE 0x01 +#define STATUS_EXTADDR_DISABLE 0x00 /* Send a single-byte command to the device and read the response */ int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response, size_t len); @@ -86,6 +88,12 @@ int spi_flash_cmd_extaddr_write(struct spi_flash *flash, u8 ear); /* Read the extended address register */ int spi_flash_cmd_extaddr_read(struct spi_flash *flash, void *data); +/* + * Extended address access + * access 4th byte address in 3-byte addessing mode for flashes + * which has an actual size of > 16MB. + */ +int spi_flash_extaddr_access(struct spi_flash *flash, u8 status); /* * Same as spi_flash_cmd_read() except it also claims/releases the SPI
This patch provides support to access an extended addressing in 3-byte address mode. The current implementation in spi_flash supports 3-byte address mode due to this up to 16MB amount of flash is able to access for those flashes which has an actual size of > 16MB. extended/bank address register contains an information to access the 4th byte addressing hence the flashes which has > 16MB can be accessible. Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com> --- drivers/mtd/spi/spi_flash.c | 81 ++++++++++++++++++++++++++++++++++ drivers/mtd/spi/spi_flash_internal.h | 8 +++ 2 files changed, 89 insertions(+), 0 deletions(-)