From mboxrd@z Thu Jan 1 00:00:00 1970 From: Date: Thu, 11 Oct 2012 05:38:59 -0000 Subject: No subject Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 ag= ain pointing to 0x0. Because we have 3-byte addressing able to access only first 16MB of flash b= ut 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 of= fset 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 =3D 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 fo= r > manufacturer ID) should be done only once once during probing of the flas= h. > 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 simi= lar > 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 exp= lain. > > > + ret =3D spi_flash_check_extaddr_access(flash, &offset); > > + if (ret) { > > + debug("SF: fail to acess ext_addr\n"); > > + return ret; > > + } > > + } > > + > > page_size =3D flash->page_size; > > page_addr =3D offset / page_size; > > byte_addr =3D 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 =3D spi_flash_check_extaddr_access(flash, &offset); > > + if (ret) { > > + debug("SF: fail to acess ext_addr\n"); > > + return ret; > > + } > > + } > > > > cmd[0] =3D 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 =3D spi_flash_check_extaddr_access(flash, &offset); > > + if (ret) { > > + debug("SF: fail to acess ext_addr\n"); > > + return ret; > > + } > > + } > > + > > erase_size =3D 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 *flas= h, > 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 =3D 0, write_done =3D 0; > > + > > + for (pass =3D 0; pass < 2; pass++) { > Why this is required?? > > > + ret =3D spi_flash_cmd_extaddr_read(flash, (void *)&data); > > + if (ret < 0) { > > + debug("SF: fail to read ext addr register\n"); > > + return ret; > > + } > > + > > + if ((data !=3D status) & !write_done) { > > + debug("SF: need to %s ext addr bit\n", > > + status ? "set" : "reset"); > > + > > + write_done =3D 1; > > + ret =3D 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), plea= se > cache the actual value inside struct spi_flash. > Initial read should be done during probing and then writing only if the v= alue > 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 h= appened 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 r= ecipient(s) and contain(s) confidential information that may be proprietary= , privileged or copyrighted under applicable law. If you are not the intend= ed recipient, do not read, copy, or forward this email message or any attac= hments. Delete this email message and any attachments immediately.