[U-Boot] [PATCH 1/5] altera_qspi: call callback even if the erase failed

[U-Boot] [PATCH 1/5] altera_qspi: call callback even if the erase failed

[Thomas Chou]

Erase is an asynchronous operation.  Device drivers are supposed
to call instr->callback() whenever the operation completes, even
if it completes with a failure.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
 drivers/mtd/altera_qspi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
index c7e37ad..627a8cc 100644
--- a/drivers/mtd/altera_qspi.c
+++ b/drivers/mtd/altera_qspi.c
@@ -146,6 +146,7 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
 			debug("erase %08x fail %x\n", sect, stat);
 			writel(stat, &regs->isr); /* clear isr */
 			instr->state = MTD_ERASE_FAILED;
+			mtd_erase_callback(instr);
 			return -EIO;
 		}
 		addr += mtd->erasesize;
-- 
2.5.0

[U-Boot] [PATCH 2/5] altera_qspi: set fail_addr for erase ops

[Thomas Chou]

If the erase fails, fail_addr might indicate exactly which block
failed. If fail_addr = MTD_FAIL_ADDR_UNKNOWN, the failure was not
at the device level or was not specific to any particular block.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
 drivers/mtd/altera_qspi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
index 627a8cc..b0d4f2c 100644
--- a/drivers/mtd/altera_qspi.c
+++ b/drivers/mtd/altera_qspi.c
@@ -145,6 +145,7 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
 			/* erase failed, sector might be protected */
 			debug("erase %08x fail %x\n", sect, stat);
 			writel(stat, &regs->isr); /* clear isr */
+			instr->fail_addr = addr;
 			instr->state = MTD_ERASE_FAILED;
 			mtd_erase_callback(instr);
 			return -EIO;
-- 
2.5.0

[U-Boot] [PATCH 3/5] altera_qspi: skip erase if the sector is blank

[Thomas Chou]

Skip erase if the sector is blank. The sector erase is slow, and
may take 0.7 sec typically or up to 3 sec worst-case.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
 drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
index b0d4f2c..8a630a6 100644
--- a/drivers/mtd/altera_qspi.c
+++ b/drivers/mtd/altera_qspi.c
@@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
 	size_t end = addr + len;
 	u32 sect;
 	u32 stat;
+	u32 *flash, *last;
 
 	instr->state = MTD_ERASING;
 	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
 	while (addr < end) {
-		sect = addr / mtd->erasesize;
-		sect <<= 8;
-		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
-		debug("erase %08x\n", sect);
-		writel(sect, &regs->mem_op);
-		stat = readl(&regs->isr);
-		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
-			/* erase failed, sector might be protected */
-			debug("erase %08x fail %x\n", sect, stat);
-			writel(stat, &regs->isr); /* clear isr */
-			instr->fail_addr = addr;
-			instr->state = MTD_ERASE_FAILED;
-			mtd_erase_callback(instr);
-			return -EIO;
+		flash = pdata->base + addr;
+		last = pdata->base + addr + mtd->erasesize;
+		/* skip erase if sector is blank */
+		while (flash < last) {
+			if (readl(flash) != 0xffffffff)
+				break;
+			flash++;
+		}
+		if (flash < last) {
+			sect = addr / mtd->erasesize;
+			sect <<= 8;
+			sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
+			debug("erase %08x\n", sect);
+			writel(sect, &regs->mem_op);
+			stat = readl(&regs->isr);
+			if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
+				/* erase failed, sector might be protected */
+				debug("erase %08x fail %x\n", sect, stat);
+				writel(stat, &regs->isr); /* clear isr */
+				instr->fail_addr = addr;
+				instr->state = MTD_ERASE_FAILED;
+				mtd_erase_callback(instr);
+				return -EIO;
+			}
 		}
 		addr += mtd->erasesize;
 	}
-- 
2.5.0

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Thomas Chou]

Show sector erase progress with dot and comma.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
 drivers/mtd/altera_qspi.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
index 8a630a6..0624ff4 100644
--- a/drivers/mtd/altera_qspi.c
+++ b/drivers/mtd/altera_qspi.c
@@ -52,6 +52,7 @@ struct altera_qspi_platdata {
 	unsigned long size;
 };
 
+static uint flash_verbose;
 flash_info_t flash_info[CONFIG_SYS_MAX_FLASH_BANKS];	/* FLASH chips info */
 
 static void altera_qspi_get_locked_range(struct mtd_info *mtd, loff_t *ofs,
@@ -74,6 +75,11 @@ void flash_print_info(flash_info_t *info)
 	putc('\n');
 }
 
+void flash_set_verbose(uint v)
+{
+	flash_verbose = v;
+}
+
 int flash_erase(flash_info_t *info, int s_first, int s_last)
 {
 	struct mtd_info *mtd = info->mtd;
@@ -84,10 +90,13 @@ int flash_erase(flash_info_t *info, int s_first, int s_last)
 	instr.mtd = mtd;
 	instr.addr = mtd->erasesize * s_first;
 	instr.len = mtd->erasesize * (s_last + 1 - s_first);
+	flash_set_verbose(1);
 	ret = mtd_erase(mtd, &instr);
+	flash_set_verbose(0);
 	if (ret)
 		return ERR_PROTECTED;
 
+	puts(" done\n");
 	return 0;
 }
 
@@ -160,6 +169,11 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
 				mtd_erase_callback(instr);
 				return -EIO;
 			}
+			if (flash_verbose)
+				putc('.');
+		} else {
+			if (flash_verbose)
+				putc(',');
 		}
 		addr += mtd->erasesize;
 	}
-- 
2.5.0

[U-Boot] [PATCH 5/5] altera_qspi: allow ctrl-c to abort the erase ops

[Thomas Chou]

Allow ctrl-c to abort the erase ops.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
 drivers/mtd/altera_qspi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
index 0624ff4..a9148a7 100644
--- a/drivers/mtd/altera_qspi.c
+++ b/drivers/mtd/altera_qspi.c
@@ -5,6 +5,7 @@
  */
 
 #include <common.h>
+#include <console.h>
 #include <dm.h>
 #include <errno.h>
 #include <fdt_support.h>
@@ -145,6 +146,14 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
 	instr->state = MTD_ERASING;
 	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
 	while (addr < end) {
+		if (ctrlc()) {
+			if (flash_verbose)
+				putc('\n');
+			instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
+			instr->state = MTD_ERASE_FAILED;
+			mtd_erase_callback(instr);
+			return -EIO;
+		}
 		flash = pdata->base + addr;
 		last = pdata->base + addr + mtd->erasesize;
 		/* skip erase if sector is blank */
-- 
2.5.0

[U-Boot] [PATCH 3/5] altera_qspi: skip erase if the sector is blank

[Marek Vasut]

On Thursday, December 24, 2015 at 01:51:22 AM, Thomas Chou wrote:
> Skip erase if the sector is blank. The sector erase is slow, and
> may take 0.7 sec typically or up to 3 sec worst-case.
> 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>  drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
> index b0d4f2c..8a630a6 100644
> --- a/drivers/mtd/altera_qspi.c
> +++ b/drivers/mtd/altera_qspi.c
> @@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info *mtd,
> struct erase_info *instr) size_t end = addr + len;
>  	u32 sect;
>  	u32 stat;
> +	u32 *flash, *last;
> 
>  	instr->state = MTD_ERASING;
>  	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
>  	while (addr < end) {
> -		sect = addr / mtd->erasesize;
> -		sect <<= 8;
> -		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
> -		debug("erase %08x\n", sect);
> -		writel(sect, &regs->mem_op);
> -		stat = readl(&regs->isr);
> -		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
> -			/* erase failed, sector might be protected */
> -			debug("erase %08x fail %x\n", sect, stat);
> -			writel(stat, &regs->isr); /* clear isr */
> -			instr->fail_addr = addr;
> -			instr->state = MTD_ERASE_FAILED;
> -			mtd_erase_callback(instr);
> -			return -EIO;
> +		flash = pdata->base + addr;
> +		last = pdata->base + addr + mtd->erasesize;
> +		/* skip erase if sector is blank */
> +		while (flash < last) {
> +			if (readl(flash) != 0xffffffff)
> +				break;
> +			flash++;

Shouldn't $last be divided by 4 ? $flash is u32 * afterall .

> +		}
> +		if (flash < last) {
> +			sect = addr / mtd->erasesize;
> +			sect <<= 8;
> +			sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
> +			debug("erase %08x\n", sect);
> +			writel(sect, &regs->mem_op);
> +			stat = readl(&regs->isr);
> +			if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
> +				/* erase failed, sector might be protected */
> +				debug("erase %08x fail %x\n", sect, stat);
> +				writel(stat, &regs->isr); /* clear isr */
> +				instr->fail_addr = addr;
> +				instr->state = MTD_ERASE_FAILED;
> +				mtd_erase_callback(instr);
> +				return -EIO;
> +			}
>  		}
>  		addr += mtd->erasesize;
>  	}

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Marek Vasut]

On Thursday, December 24, 2015 at 01:51:23 AM, Thomas Chou wrote:
> Show sector erase progress with dot and comma.
> 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---

Shouldn't this go into common code ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 3/5] altera_qspi: skip erase if the sector is blank

[Thomas Chou]

Hi Marek,

On 2015?12?24? 09:28, Marek Vasut wrote:
> On Thursday, December 24, 2015 at 01:51:22 AM, Thomas Chou wrote:
>> Skip erase if the sector is blank. The sector erase is slow, and
>> may take 0.7 sec typically or up to 3 sec worst-case.
>>
>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
>> ---
>>   drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
>>   1 file changed, 25 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
>> index b0d4f2c..8a630a6 100644
>> --- a/drivers/mtd/altera_qspi.c
>> +++ b/drivers/mtd/altera_qspi.c
>> @@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info *mtd,
>> struct erase_info *instr) size_t end = addr + len;
>>   	u32 sect;
>>   	u32 stat;
>> +	u32 *flash, *last;
>>
>>   	instr->state = MTD_ERASING;
>>   	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
>>   	while (addr < end) {
>> -		sect = addr / mtd->erasesize;
>> -		sect <<= 8;
>> -		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
>> -		debug("erase %08x\n", sect);
>> -		writel(sect, &regs->mem_op);
>> -		stat = readl(&regs->isr);
>> -		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
>> -			/* erase failed, sector might be protected */
>> -			debug("erase %08x fail %x\n", sect, stat);
>> -			writel(stat, &regs->isr); /* clear isr */
>> -			instr->fail_addr = addr;
>> -			instr->state = MTD_ERASE_FAILED;
>> -			mtd_erase_callback(instr);
>> -			return -EIO;
>> +		flash = pdata->base + addr;
>> +		last = pdata->base + addr + mtd->erasesize;
>> +		/* skip erase if sector is blank */
>> +		while (flash < last) {
>> +			if (readl(flash) != 0xffffffff)
>> +				break;
>> +			flash++;
>
> Shouldn't $last be divided by 4 ? $flash is u32 * afterall .

No. Both flash and last are assigned with byte addressing from 
pdata->base, which is void *.

Best regards,
Thomas

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Thomas Chou]

Hi Marek,

On 2015?12?24? 09:29, Marek Vasut wrote:
> On Thursday, December 24, 2015 at 01:51:23 AM, Thomas Chou wrote:
>> Show sector erase progress with dot and comma.
>>
>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
>> ---
>
> Shouldn't this go into common code ?
>

The code to print a dot is minimal. It will cost more if this go into 
common code.

Alternatively, we might set the flash_verbose directly and remove the 
flash_set_verbose().

Best regards,
Thomas

[U-Boot] [PATCH 3/5] altera_qspi: skip erase if the sector is blank

[Marek Vasut]

On Thursday, December 24, 2015 at 03:23:05 AM, Thomas Chou wrote:
> Hi Marek,
> 
> On 2015?12?24? 09:28, Marek Vasut wrote:
> > On Thursday, December 24, 2015 at 01:51:22 AM, Thomas Chou wrote:
> >> Skip erase if the sector is blank. The sector erase is slow, and
> >> may take 0.7 sec typically or up to 3 sec worst-case.
> >> 
> >> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> >> ---
> >> 
> >>   drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
> >>   1 file changed, 25 insertions(+), 14 deletions(-)
> >> 
> >> diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
> >> index b0d4f2c..8a630a6 100644
> >> --- a/drivers/mtd/altera_qspi.c
> >> +++ b/drivers/mtd/altera_qspi.c
> >> @@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info *mtd,
> >> struct erase_info *instr) size_t end = addr + len;
> >> 
> >>   	u32 sect;
> >>   	u32 stat;
> >> 
> >> +	u32 *flash, *last;
> >> 
> >>   	instr->state = MTD_ERASING;
> >>   	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
> >>   	while (addr < end) {
> >> 
> >> -		sect = addr / mtd->erasesize;
> >> -		sect <<= 8;
> >> -		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
> >> -		debug("erase %08x\n", sect);
> >> -		writel(sect, &regs->mem_op);
> >> -		stat = readl(&regs->isr);
> >> -		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
> >> -			/* erase failed, sector might be protected */
> >> -			debug("erase %08x fail %x\n", sect, stat);
> >> -			writel(stat, &regs->isr); /* clear isr */
> >> -			instr->fail_addr = addr;
> >> -			instr->state = MTD_ERASE_FAILED;
> >> -			mtd_erase_callback(instr);
> >> -			return -EIO;
> >> +		flash = pdata->base + addr;
> >> +		last = pdata->base + addr + mtd->erasesize;
> >> +		/* skip erase if sector is blank */
> >> +		while (flash < last) {
> >> +			if (readl(flash) != 0xffffffff)
> >> +				break;
> >> +			flash++;
> > 
> > Shouldn't $last be divided by 4 ? $flash is u32 * afterall .
> 
> No. Both flash and last are assigned with byte addressing from
> pdata->base, which is void *.

The data type of both $flash and $last is u32 * though?

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Marek Vasut]

On Thursday, December 24, 2015 at 03:50:57 AM, Thomas Chou wrote:
> Hi Marek,
> 
> On 2015?12?24? 09:29, Marek Vasut wrote:
> > On Thursday, December 24, 2015 at 01:51:23 AM, Thomas Chou wrote:
> >> Show sector erase progress with dot and comma.
> >> 
> >> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> >> ---
> > 
> > Shouldn't this go into common code ?
> 
> The code to print a dot is minimal. It will cost more if this go into
> common code.

But then this driver behaves in a non-standard manner AND noone benefits
from this functionality but this driver.

> Alternatively, we might set the flash_verbose directly and remove the
> flash_set_verbose().
> 
> Best regards,
> Thomas

Best regards,
Marek Vasut

[U-Boot] [PATCH 3/5] altera_qspi: skip erase if the sector is blank

[Thomas Chou]

Hi Marek,

On 2015?12?24? 11:01, Marek Vasut wrote:
> On Thursday, December 24, 2015 at 03:23:05 AM, Thomas Chou wrote:
>> Hi Marek,
>>
>> On 2015?12?24? 09:28, Marek Vasut wrote:
>>> On Thursday, December 24, 2015 at 01:51:22 AM, Thomas Chou wrote:
>>>> Skip erase if the sector is blank. The sector erase is slow, and
>>>> may take 0.7 sec typically or up to 3 sec worst-case.
>>>>
>>>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
>>>> ---
>>>>
>>>>    drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
>>>>    1 file changed, 25 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
>>>> index b0d4f2c..8a630a6 100644
>>>> --- a/drivers/mtd/altera_qspi.c
>>>> +++ b/drivers/mtd/altera_qspi.c
>>>> @@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info *mtd,
>>>> struct erase_info *instr) size_t end = addr + len;
>>>>
>>>>    	u32 sect;
>>>>    	u32 stat;
>>>>
>>>> +	u32 *flash, *last;
>>>>
>>>>    	instr->state = MTD_ERASING;
>>>>    	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
>>>>    	while (addr < end) {
>>>>
>>>> -		sect = addr / mtd->erasesize;
>>>> -		sect <<= 8;
>>>> -		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
>>>> -		debug("erase %08x\n", sect);
>>>> -		writel(sect, &regs->mem_op);
>>>> -		stat = readl(&regs->isr);
>>>> -		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
>>>> -			/* erase failed, sector might be protected */
>>>> -			debug("erase %08x fail %x\n", sect, stat);
>>>> -			writel(stat, &regs->isr); /* clear isr */
>>>> -			instr->fail_addr = addr;
>>>> -			instr->state = MTD_ERASE_FAILED;
>>>> -			mtd_erase_callback(instr);
>>>> -			return -EIO;
>>>> +		flash = pdata->base + addr;
>>>> +		last = pdata->base + addr + mtd->erasesize;
>>>> +		/* skip erase if sector is blank */
>>>> +		while (flash < last) {
>>>> +			if (readl(flash) != 0xffffffff)
>>>> +				break;
>>>> +			flash++;
>>>
>>> Shouldn't $last be divided by 4 ? $flash is u32 * afterall .
>>
>> No. Both flash and last are assigned with byte addressing from
>> pdata->base, which is void *.
>
> The data type of both $flash and $last is u32 * though?

Yes.

flash = pdata->base + addr;

will be the same as,

flash = pdata->base;
flash += addr / 4;

Best regards,
Thomas

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Thomas Chou]

Hi Marek,

On 2015?12?24? 11:02, Marek Vasut wrote:
> On Thursday, December 24, 2015 at 03:50:57 AM, Thomas Chou wrote:
>> Hi Marek,
>>
>> On 2015?12?24? 09:29, Marek Vasut wrote:
>>> On Thursday, December 24, 2015 at 01:51:23 AM, Thomas Chou wrote:
>>>> Show sector erase progress with dot and comma.
>>>>
>>>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
>>>> ---
>>>
>>> Shouldn't this go into common code ?
>>
>> The code to print a dot is minimal. It will cost more if this go into
>> common code.
>
> But then this driver behaves in a non-standard manner AND noone benefits
> from this functionality but this driver.

The sector erase might take very long when there are a lot of sectors to 
erase, say 12 min to 50 min for 1024 sectors on the 10m50 board. Without 
the display of progress, it will look like the board hangs.

The dotting code comes from the old cfi_flash.c. Some other parallel 
flash have this, but not spi-flash. It is trivial to add though.

Best regards,
Thomas

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Marek Vasut]

On Thursday, December 24, 2015 at 04:26:57 AM, Thomas Chou wrote:
> Hi Marek,

Hi Thomas,

> On 2015?12?24? 11:02, Marek Vasut wrote:
> > On Thursday, December 24, 2015 at 03:50:57 AM, Thomas Chou wrote:
> >> Hi Marek,
> >> 
> >> On 2015?12?24? 09:29, Marek Vasut wrote:
> >>> On Thursday, December 24, 2015 at 01:51:23 AM, Thomas Chou wrote:
> >>>> Show sector erase progress with dot and comma.
> >>>> 
> >>>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> >>>> ---
> >>> 
> >>> Shouldn't this go into common code ?
> >> 
> >> The code to print a dot is minimal. It will cost more if this go into
> >> common code.
> > 
> > But then this driver behaves in a non-standard manner AND noone benefits
> > from this functionality but this driver.
> 
> The sector erase might take very long when there are a lot of sectors to
> erase, say 12 min to 50 min for 1024 sectors on the 10m50 board. Without
> the display of progress, it will look like the board hangs.

Yeah, that I do understand and I agree this is a good idea :)

> The dotting code comes from the old cfi_flash.c. Some other parallel
> flash have this, but not spi-flash. It is trivial to add though.

Urm, altera_qspi is CFI, right ? So can we stuff this into common code or not?
Sorry if I am confused and off the mark.

Best regards,
Marek Vasut

[U-Boot] [PATCH 3/5] altera_qspi: skip erase if the sector is blank

[Marek Vasut]

On Thursday, December 24, 2015 at 04:08:33 AM, Thomas Chou wrote:
> Hi Marek,
> 
> On 2015?12?24? 11:01, Marek Vasut wrote:
> > On Thursday, December 24, 2015 at 03:23:05 AM, Thomas Chou wrote:
> >> Hi Marek,
> >> 
> >> On 2015?12?24? 09:28, Marek Vasut wrote:
> >>> On Thursday, December 24, 2015 at 01:51:22 AM, Thomas Chou wrote:
> >>>> Skip erase if the sector is blank. The sector erase is slow, and
> >>>> may take 0.7 sec typically or up to 3 sec worst-case.
> >>>> 
> >>>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> >>>> ---
> >>>> 
> >>>>    drivers/mtd/altera_qspi.c | 39
> >>>>    +++++++++++++++++++++++++-------------- 1 file changed, 25
> >>>>    insertions(+), 14 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/mtd/altera_qspi.c b/drivers/mtd/altera_qspi.c
> >>>> index b0d4f2c..8a630a6 100644
> >>>> --- a/drivers/mtd/altera_qspi.c
> >>>> +++ b/drivers/mtd/altera_qspi.c
> >>>> @@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info
> >>>> *mtd, struct erase_info *instr) size_t end = addr + len;
> >>>> 
> >>>>    	u32 sect;
> >>>>    	u32 stat;
> >>>> 
> >>>> +	u32 *flash, *last;
> >>>> 
> >>>>    	instr->state = MTD_ERASING;
> >>>>    	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
> >>>>    	while (addr < end) {
> >>>> 
> >>>> -		sect = addr / mtd->erasesize;
> >>>> -		sect <<= 8;
> >>>> -		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
> >>>> -		debug("erase %08x\n", sect);
> >>>> -		writel(sect, &regs->mem_op);
> >>>> -		stat = readl(&regs->isr);
> >>>> -		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
> >>>> -			/* erase failed, sector might be protected */
> >>>> -			debug("erase %08x fail %x\n", sect, stat);
> >>>> -			writel(stat, &regs->isr); /* clear isr */
> >>>> -			instr->fail_addr = addr;
> >>>> -			instr->state = MTD_ERASE_FAILED;
> >>>> -			mtd_erase_callback(instr);
> >>>> -			return -EIO;
> >>>> +		flash = pdata->base + addr;
> >>>> +		last = pdata->base + addr + mtd->erasesize;
> >>>> +		/* skip erase if sector is blank */
> >>>> +		while (flash < last) {
> >>>> +			if (readl(flash) != 0xffffffff)
> >>>> +				break;
> >>>> +			flash++;
> >>> 
> >>> Shouldn't $last be divided by 4 ? $flash is u32 * afterall .
> >> 
> >> No. Both flash and last are assigned with byte addressing from
> >> pdata->base, which is void *.
> > 
> > The data type of both $flash and $last is u32 * though?
> 
> Yes.
> 
> flash = pdata->base + addr;
> 
> will be the same as,
> 
> flash = pdata->base;
> flash += addr / 4;

Please ignore what I said, sorry.

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Thomas Chou]

Hi Marek,

On 2015?12?24? 11:42, Marek Vasut wrote:
> On Thursday, December 24, 2015 at 04:26:57 AM, Thomas Chou wrote:
>> Hi Marek,
>
> Hi Thomas,
>
>> On 2015?12?24? 11:02, Marek Vasut wrote:
>>> On Thursday, December 24, 2015 at 03:50:57 AM, Thomas Chou wrote:
>>>> Hi Marek,
>>>>
>>>> On 2015?12?24? 09:29, Marek Vasut wrote:
>>>>> On Thursday, December 24, 2015 at 01:51:23 AM, Thomas Chou wrote:
>>>>>> Show sector erase progress with dot and comma.
>>>>>>
>>>>>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
>>>>>> ---
>>>>>
>>>>> Shouldn't this go into common code ?
>>>>
>>>> The code to print a dot is minimal. It will cost more if this go into
>>>> common code.
>>>
>>> But then this driver behaves in a non-standard manner AND noone benefits
>>> from this functionality but this driver.
>>
>> The sector erase might take very long when there are a lot of sectors to
>> erase, say 12 min to 50 min for 1024 sectors on the 10m50 board. Without
>> the display of progress, it will look like the board hangs.
>
> Yeah, that I do understand and I agree this is a good idea :)
>
>> The dotting code comes from the old cfi_flash.c. Some other parallel
>> flash have this, but not spi-flash. It is trivial to add though.
>
> Urm, altera_qspi is CFI, right ? So can we stuff this into common code or not?
> Sorry if I am confused and off the mark.

No worries. I think the dotting is best done per driver. We can have 
common behavior. But there is little advantage to make it common code.

Merry Xmas.

Thomas Chou

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Marek Vasut]

On Thursday, December 24, 2015 at 06:24:45 AM, Thomas Chou wrote:
> Hi Marek,
> 
> On 2015?12?24? 11:42, Marek Vasut wrote:
> > On Thursday, December 24, 2015 at 04:26:57 AM, Thomas Chou wrote:
> >> Hi Marek,
> > 
> > Hi Thomas,
> > 
> >> On 2015?12?24? 11:02, Marek Vasut wrote:
> >>> On Thursday, December 24, 2015 at 03:50:57 AM, Thomas Chou wrote:
> >>>> Hi Marek,
> >>>> 
> >>>> On 2015?12?24? 09:29, Marek Vasut wrote:
> >>>>> On Thursday, December 24, 2015 at 01:51:23 AM, Thomas Chou wrote:
> >>>>>> Show sector erase progress with dot and comma.
> >>>>>> 
> >>>>>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> >>>>>> ---
> >>>>> 
> >>>>> Shouldn't this go into common code ?
> >>>> 
> >>>> The code to print a dot is minimal. It will cost more if this go into
> >>>> common code.
> >>> 
> >>> But then this driver behaves in a non-standard manner AND noone
> >>> benefits from this functionality but this driver.
> >> 
> >> The sector erase might take very long when there are a lot of sectors to
> >> erase, say 12 min to 50 min for 1024 sectors on the 10m50 board. Without
> >> the display of progress, it will look like the board hangs.
> > 
> > Yeah, that I do understand and I agree this is a good idea :)
> > 
> >> The dotting code comes from the old cfi_flash.c. Some other parallel
> >> flash have this, but not spi-flash. It is trivial to add though.
> > 
> > Urm, altera_qspi is CFI, right ? So can we stuff this into common code or
> > not? Sorry if I am confused and off the mark.
> 
> No worries. I think the dotting is best done per driver.

Well why do you think so ?

> We can have
> common behavior. But there is little advantage to make it common code.
> 
> Merry Xmas.

Merry Xmas to you too!

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Thomas Chou]

Hi Marek,

On 2015?12?24? 13:37, Marek Vasut wrote:
> On Thursday, December 24, 2015 at 06:24:45 AM, Thomas Chou wrote:
>> Hi Marek,
>>
>> On 2015?12?24? 11:42, Marek Vasut wrote:
>>> On Thursday, December 24, 2015 at 04:26:57 AM, Thomas Chou wrote:
>>>> Hi Marek,
>>>
>>> Hi Thomas,
>>>
>>>> On 2015?12?24? 11:02, Marek Vasut wrote:
>>>>> On Thursday, December 24, 2015 at 03:50:57 AM, Thomas Chou wrote:
>>>>>> Hi Marek,
>>>>>>
>>>>>> On 2015?12?24? 09:29, Marek Vasut wrote:
>>>>>>> On Thursday, December 24, 2015 at 01:51:23 AM, Thomas Chou wrote:
>>>>>>>> Show sector erase progress with dot and comma.
>>>>>>>>
>>>>>>>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
>>>>>>>> ---
>>>>>>>
>>>>>>> Shouldn't this go into common code ?
>>>>>>
>>>>>> The code to print a dot is minimal. It will cost more if this go into
>>>>>> common code.
>>>>>
>>>>> But then this driver behaves in a non-standard manner AND noone
>>>>> benefits from this functionality but this driver.
>>>>
>>>> The sector erase might take very long when there are a lot of sectors to
>>>> erase, say 12 min to 50 min for 1024 sectors on the 10m50 board. Without
>>>> the display of progress, it will look like the board hangs.
>>>
>>> Yeah, that I do understand and I agree this is a good idea :)
>>>
>>>> The dotting code comes from the old cfi_flash.c. Some other parallel
>>>> flash have this, but not spi-flash. It is trivial to add though.
>>>
>>> Urm, altera_qspi is CFI, right ? So can we stuff this into common code or
>>> not? Sorry if I am confused and off the mark.
>>
>> No worries. I think the dotting is best done per driver.
>
> Well why do you think so ?

Well, it is only a putc() which is plain and simple.

Best regards,
Thomas

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Marek Vasut]

On Friday, December 25, 2015 at 01:12:28 AM, Thomas Chou wrote:
> Hi Marek,
> 
> On 2015?12?24? 13:37, Marek Vasut wrote:
> > On Thursday, December 24, 2015 at 06:24:45 AM, Thomas Chou wrote:
> >> Hi Marek,
> >> 
> >> On 2015?12?24? 11:42, Marek Vasut wrote:
> >>> On Thursday, December 24, 2015 at 04:26:57 AM, Thomas Chou wrote:
> >>>> Hi Marek,
> >>> 
> >>> Hi Thomas,
> >>> 
> >>>> On 2015?12?24? 11:02, Marek Vasut wrote:
> >>>>> On Thursday, December 24, 2015 at 03:50:57 AM, Thomas Chou wrote:
> >>>>>> Hi Marek,
> >>>>>> 
> >>>>>> On 2015?12?24? 09:29, Marek Vasut wrote:
> >>>>>>> On Thursday, December 24, 2015 at 01:51:23 AM, Thomas Chou wrote:
> >>>>>>>> Show sector erase progress with dot and comma.
> >>>>>>>> 
> >>>>>>>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> >>>>>>>> ---
> >>>>>>> 
> >>>>>>> Shouldn't this go into common code ?
> >>>>>> 
> >>>>>> The code to print a dot is minimal. It will cost more if this go
> >>>>>> into common code.
> >>>>> 
> >>>>> But then this driver behaves in a non-standard manner AND noone
> >>>>> benefits from this functionality but this driver.
> >>>> 
> >>>> The sector erase might take very long when there are a lot of sectors
> >>>> to erase, say 12 min to 50 min for 1024 sectors on the 10m50 board.
> >>>> Without the display of progress, it will look like the board hangs.
> >>> 
> >>> Yeah, that I do understand and I agree this is a good idea :)
> >>> 
> >>>> The dotting code comes from the old cfi_flash.c. Some other parallel
> >>>> flash have this, but not spi-flash. It is trivial to add though.
> >>> 
> >>> Urm, altera_qspi is CFI, right ? So can we stuff this into common code
> >>> or not? Sorry if I am confused and off the mark.
> >> 
> >> No worries. I think the dotting is best done per driver.
> > 
> > Well why do you think so ?
> 
> Well, it is only a putc() which is plain and simple.

Sure, but then I still don't understand why this cannot be in the common code.

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Thomas Chou]

Hi Marek,

On 2015?12?25? 12:08, Marek Vasut wrote:
>> Well, it is only a putc() which is plain and simple.
>
> Sure, but then I still don't understand why this cannot be in the common code.

Yes, it can be in the common code. Do you have an idea how should it 
look like?

Best regards,
Thomas

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Marek Vasut]

On Friday, December 25, 2015 at 09:33:52 AM, Thomas Chou wrote:
> Hi Marek,
> 
> On 2015?12?25? 12:08, Marek Vasut wrote:
> >> Well, it is only a putc() which is plain and simple.
> > 
> > Sure, but then I still don't understand why this cannot be in the common
> > code.
> 
> Yes, it can be in the common code. Do you have an idea how should it
> look like?

No, I don't, I am not a CFI expert, sorry.

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Thomas Chou]

Hi Marek,

On 2015?12?25? 17:58, Marek Vasut wrote:
> On Friday, December 25, 2015 at 09:33:52 AM, Thomas Chou wrote:
>> Hi Marek,
>>
>> On 2015?12?25? 12:08, Marek Vasut wrote:
>>>> Well, it is only a putc() which is plain and simple.
>>>
>>> Sure, but then I still don't understand why this cannot be in the common
>>> code.
>>
>> Yes, it can be in the common code. Do you have an idea how should it
>> look like?
>
> No, I don't, I am not a CFI expert, sorry.

Then I shall push these first and find way to put it to common code some 
time later. Thanks.

Best regards,
Thomas

[U-Boot] [PATCH 1/5] altera_qspi: call callback even if the erase failed

[Thomas Chou]

On 2015?12?24? 08:51, Thomas Chou wrote:
> Erase is an asynchronous operation.  Device drivers are supposed
> to call instr->callback() whenever the operation completes, even
> if it completes with a failure.
>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>   drivers/mtd/altera_qspi.c | 1 +
>   1 file changed, 1 insertion(+)
>

Applied to u-boot-nios.

[U-Boot] [PATCH 2/5] altera_qspi: set fail_addr for erase ops

[Thomas Chou]

On 2015?12?24? 08:51, Thomas Chou wrote:
> If the erase fails, fail_addr might indicate exactly which block
> failed. If fail_addr = MTD_FAIL_ADDR_UNKNOWN, the failure was not
> at the device level or was not specific to any particular block.
>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>   drivers/mtd/altera_qspi.c | 1 +
>   1 file changed, 1 insertion(+)
>

Applied to u-boot-nios.

[U-Boot] [PATCH 3/5] altera_qspi: skip erase if the sector is blank

[Thomas Chou]

On 2015?12?24? 08:51, Thomas Chou wrote:
> Skip erase if the sector is blank. The sector erase is slow, and
> may take 0.7 sec typically or up to 3 sec worst-case.
>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>   drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
>   1 file changed, 25 insertions(+), 14 deletions(-)
>

Applied to u-boot-nios.

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Thomas Chou]

On 2015?12?24? 08:51, Thomas Chou wrote:
> Show sector erase progress with dot and comma.
>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>   drivers/mtd/altera_qspi.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>

Applied to u-boot-nios.

[U-Boot] [PATCH 5/5] altera_qspi: allow ctrl-c to abort the erase ops

[Thomas Chou]

On 2015?12?24? 08:51, Thomas Chou wrote:
> Allow ctrl-c to abort the erase ops.
>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>   drivers/mtd/altera_qspi.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>

Applied to u-boot-nios.

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Marek Vasut]

On Monday, December 28, 2015 at 02:17:46 AM, Thomas Chou wrote:
> Hi Marek,
> 
> On 2015?12?25? 17:58, Marek Vasut wrote:
> > On Friday, December 25, 2015 at 09:33:52 AM, Thomas Chou wrote:
> >> Hi Marek,
> >> 
> >> On 2015?12?25? 12:08, Marek Vasut wrote:
> >>>> Well, it is only a putc() which is plain and simple.
> >>> 
> >>> Sure, but then I still don't understand why this cannot be in the
> >>> common code.
> >> 
> >> Yes, it can be in the common code. Do you have an idea how should it
> >> look like?
> > 
> > No, I don't, I am not a CFI expert, sorry.
> 
> Then I shall push these first and find way to put it to common code some
> time later. Thanks.

Is this any way to deal with feedback ? Why don't you put it into flash_erase()
for example, in drivers/mtd/cfi_flash.c ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Thomas Chou]

Hi Marek,

On 2015?12?28? 13:35, Marek Vasut wrote:
> On Monday, December 28, 2015 at 02:17:46 AM, Thomas Chou wrote:
>> Hi Marek,
>>
>> On 2015?12?25? 17:58, Marek Vasut wrote:
>>> On Friday, December 25, 2015 at 09:33:52 AM, Thomas Chou wrote:
>>>> Hi Marek,
>>>>
>>>> On 2015?12?25? 12:08, Marek Vasut wrote:
>>>>>> Well, it is only a putc() which is plain and simple.
>>>>>
>>>>> Sure, but then I still don't understand why this cannot be in the
>>>>> common code.
>>>>
>>>> Yes, it can be in the common code. Do you have an idea how should it
>>>> look like?
>>>
>>> No, I don't, I am not a CFI expert, sorry.
>>
>> Then I shall push these first and find way to put it to common code some
>> time later. Thanks.
>
> Is this any way to deal with feedback ? Why don't you put it into flash_erase()
> for example, in drivers/mtd/cfi_flash.c ?

Though altera-qspi works like CFI flash, it is not CFI flash as it does 
not have CFI info. It is a separate driver to cfi_flash.c.

Both drivers and many other parallel flash drivers under the board/arch 
directories are called by common/cmd_flash.c, which is very old and 
somewhat nasty. It could be the place for common feedback. But this is 
too far and deeply involved. I wanted to touch it a month ago, made some 
patches but withdrew then.

Best regards,
Thomas

[U-Boot] [PATCH 4/5] altera_qspi: show erase progress

[Marek Vasut]

On Monday, December 28, 2015 at 09:04:55 AM, Thomas Chou wrote:
> Hi Marek,

Hi!

> On 2015?12?28? 13:35, Marek Vasut wrote:
> > On Monday, December 28, 2015 at 02:17:46 AM, Thomas Chou wrote:
> >> Hi Marek,
> >> 
> >> On 2015?12?25? 17:58, Marek Vasut wrote:
> >>> On Friday, December 25, 2015 at 09:33:52 AM, Thomas Chou wrote:
> >>>> Hi Marek,
> >>>> 
> >>>> On 2015?12?25? 12:08, Marek Vasut wrote:
> >>>>>> Well, it is only a putc() which is plain and simple.
> >>>>> 
> >>>>> Sure, but then I still don't understand why this cannot be in the
> >>>>> common code.
> >>>> 
> >>>> Yes, it can be in the common code. Do you have an idea how should it
> >>>> look like?
> >>> 
> >>> No, I don't, I am not a CFI expert, sorry.
> >> 
> >> Then I shall push these first and find way to put it to common code some
> >> time later. Thanks.
> > 
> > Is this any way to deal with feedback ? Why don't you put it into
> > flash_erase() for example, in drivers/mtd/cfi_flash.c ?
> 
> Though altera-qspi works like CFI flash, it is not CFI flash as it does
> not have CFI info. It is a separate driver to cfi_flash.c.
> 
> Both drivers and many other parallel flash drivers under the board/arch
> directories are called by common/cmd_flash.c, which is very old and
> somewhat nasty. It could be the place for common feedback. But this is
> too far and deeply involved. I wanted to touch it a month ago, made some
> patches but withdrew then.

I see, thanks for the clarification. OK then.

It'd be nice if you managed to clean that part up eventually :)

Best regards,
Marek Vasut