No subject

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.