[U-Boot] [PATCH 3/4] sf: Add extended address access support

[Langer Thomas (LQDE RD ST PON SW)]

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 at 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