[U-Boot] [PATCH 4/4] sf: Add Quad-input Page Program(32h) instruction support

[Langer Thomas (LQDE RD ST PON SW)]

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