[PATCH linux 0/3] ASPEED SPI driver support for dual IO mode

[PATCH linux 0/3] ASPEED SPI driver support for dual IO mode

[Robert Lippert]

Adds support for dual IO SPI mode.  This speeds up SPI accesses, dropping
time to dump an entire 64MB Macronix chip from ~12s to ~6s using:
	dd if=/dev/mtdblock0 bs=64MB count=1 of=/dev/null

The dual IO bit mode should not depend on any board-specific properties
as it uses the same IO lines as single bit mode.  Only issue could be
if a board used a one-way level tranlator or mux on the MOSI line
but that seems unlikely to me.

Tested on Zaius board with AST2500 chip, not tested on AST2400 but I
expect it to work since registers are identical.

Note dropped TODO about quad mode since ASPEED SPI controller does
not support quad mode.


Robert Lippert (3):
  mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l
  mtd: spi-nor: aspeed: add support for SPI dual IO read mode
  mtd: spi-nor: aspeed: fix DMA access on AST2500

 drivers/mtd/spi-nor/aspeed-smc.c | 30 +++++++++++++++++++++++-------
 drivers/mtd/spi-nor/spi-nor.c    |  2 +-
 2 files changed, 24 insertions(+), 8 deletions(-)

-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH linux 1/3] mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l

[Robert Lippert]

Signed-off-by: Robert Lippert <rlippert@google.com>
---

 drivers/mtd/spi-nor/spi-nor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index a68073afe8a8..e508c25a47f7 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -857,7 +857,7 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
 	{ "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
 	{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
-	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_QUAD_READ) },
+	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "mx66l1g45g",  INFO(0xc2201b, 0, 64 * 1024, 2048, SECT_4K | SPI_NOR_QUAD_READ) },
 	{ "mx66l1g55g",  INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) },
 
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode

[Robert Lippert]

Implements support for the dual IO read mode on aspeed SMC/FMC
controllers which uses both MISO and MOSI lines for data during a read
to double the read bandwidth.

Signed-off-by: Robert Lippert <rlippert@google.com>
---

 drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index d3bed34f5aa0..a8ca2ab308d7 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
 {
 	struct aspeed_smc_chip *chip = nor->priv;
 	int ret;
+	u32 ctl;
+	int i;
+	u8 dummy = 0;
 
 	mutex_lock(&chip->controller->mutex);
 
@@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
 
 	aspeed_smc_start_user(nor);
 	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
+
+	/* Send dummy bytes */
+	for (i = 0; i < (chip->nor.read_dummy / 8); ++i)
+		aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
+
+	if (chip->nor.flash_read == SPI_NOR_DUAL) {
+		/* Switch to dual I/O mode for data cycle */
+		ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
+		ctl |= CONTROL_SPI_IO_DUAL_DATA;
+		writel(ctl, chip->ctl);
+	}
+
 	aspeed_smc_read_from_ahb(read_buf, chip->base, len);
 	aspeed_smc_stop_user(nor);
 
@@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 		cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
 		break;
 	case SPI_NOR_FAST:
+	case SPI_NOR_DUAL:
 		cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
 		break;
 	default:
@@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 	}
 
 	chip->ctl_val[smc_read] |= cmd |
+		spi_control_fill_opcode(chip->nor.read_opcode) |
 		CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
 
-	dev_dbg(controller->dev, "base control register: %08x\n",
+	if (chip->nor.flash_read == SPI_NOR_DUAL)
+		chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;
+
+	dev_info(controller->dev, "read control register: %08x\n",
 		 chip->ctl_val[smc_read]);
 	return 0;
 }
@@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
 		if (err)
 			continue;
 
-		/*
-		 * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
-		 * when board support is present as determined by of property.
-		 */
-		err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
+		err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
 		if (err)
 			continue;
 
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH linux 3/3] mtd: spi-nor: aspeed: fix DMA access on AST2500

[Robert Lippert]

AST2500 has additional bits in the dma_addr field.  Its easier
to just write the full address into the register as the hardware
will handle the masking properly.

Signed-off-by: Robert Lippert <rlippert@google.com>
---

 drivers/mtd/spi-nor/aspeed-smc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index a8ca2ab308d7..6cf159741acd 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -349,7 +349,7 @@ static int aspeed_smc_dma_wait(struct aspeed_smc_chip *chip)
 }
 
 #define DMA_LENGTH(x) (((x) - 4) & ~0xFE000003)
-#define DMA_ADDR(x) ((x) & ~0xE0000003)
+#define DMA_ADDR(x) ((x) & ~0x00000003)
 
 static inline void aspeed_smc_chip_configure(struct aspeed_smc_chip *chip,
 					     u32 ctl)
-- 
2.11.0.390.gc69c2f50cf-goog

Re: [PATCH linux 1/3] mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l

[Cédric Le Goater]

On 01/10/2017 02:05 AM, Robert Lippert wrote:
> Signed-off-by: Robert Lippert <rlippert@google.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

This would be good to upstream I think. As I need to send
a little patchset for the label support in mainline Linux,
I can include it if you want. 

C.

> ---
> 
>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index a68073afe8a8..e508c25a47f7 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -857,7 +857,7 @@ static const struct flash_info spi_nor_ids[] = {
>  	{ "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
>  	{ "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
>  	{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
> -	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_QUAD_READ) },
> +	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>  	{ "mx66l1g45g",  INFO(0xc2201b, 0, 64 * 1024, 2048, SECT_4K | SPI_NOR_QUAD_READ) },
>  	{ "mx66l1g55g",  INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) },
>  
> 

Re: [PATCH linux 3/3] mtd: spi-nor: aspeed: fix DMA access on AST2500

[Cédric Le Goater]

On 01/10/2017 02:05 AM, Robert Lippert wrote:
> AST2500 has additional bits in the dma_addr field.  Its easier
> to just write the full address into the register as the hardware
> will handle the masking properly.

Yes definitely, 

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Not for mainline yet though.

C.

> Signed-off-by: Robert Lippert <rlippert@google.com>
> ---
> 
>  drivers/mtd/spi-nor/aspeed-smc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index a8ca2ab308d7..6cf159741acd 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -349,7 +349,7 @@ static int aspeed_smc_dma_wait(struct aspeed_smc_chip *chip)
>  }
>  
>  #define DMA_LENGTH(x) (((x) - 4) & ~0xFE000003)
> -#define DMA_ADDR(x) ((x) & ~0xE0000003)
> +#define DMA_ADDR(x) ((x) & ~0x00000003)
>  
>  static inline void aspeed_smc_chip_configure(struct aspeed_smc_chip *chip,
>  					     u32 ctl)
> 

Re: [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode

[Cédric Le Goater]

Hello Robert,

On 01/10/2017 02:05 AM, Robert Lippert wrote:
> Implements support for the dual IO read mode on aspeed SMC/FMC
> controllers which uses both MISO and MOSI lines for data during a read
> to double the read bandwidth.
> 
> Signed-off-by: Robert Lippert <rlippert@google.com>
> ---
> 
>  drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index d3bed34f5aa0..a8ca2ab308d7 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>  {
>  	struct aspeed_smc_chip *chip = nor->priv;
>  	int ret;
> +	u32 ctl;
> +	int i;
> +	u8 dummy = 0;

OxFF would be a better value.
  
>  	mutex_lock(&chip->controller->mutex);
>  
> @@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>  
>  	aspeed_smc_start_user(nor);
>  	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
> +
> +	/* Send dummy bytes */
> +	for (i = 0; i < (chip->nor.read_dummy / 8); ++i)

the parentheses are not needed I think.
 
> +		aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
> +
> +	if (chip->nor.flash_read == SPI_NOR_DUAL) {
> +		/* Switch to dual I/O mode for data cycle */
> +		ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
> +		ctl |= CONTROL_SPI_IO_DUAL_DATA;
> +		writel(ctl, chip->ctl);
> +	}

So why can not we set the controller in aspeed_smc_chip_setup_finish() 
once and for all ? 

> +
>  	aspeed_smc_read_from_ahb(read_buf, chip->base, len);
>  	aspeed_smc_stop_user(nor);
>  
> @@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>  		cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
>  		break;
>  	case SPI_NOR_FAST:
> +	case SPI_NOR_DUAL:
>  		cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
>  		break;
>  	default:
> @@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>  	}
>  
>  	chip->ctl_val[smc_read] |= cmd |
> +		spi_control_fill_opcode(chip->nor.read_opcode) |
>  		CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
>  
> -	dev_dbg(controller->dev, "base control register: %08x\n",
> +	if (chip->nor.flash_read == SPI_NOR_DUAL)
> +		chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;

may be we can move that part in the switch setting above  ? 

> +	dev_info(controller->dev, "read control register: %08x\n",
>  		 chip->ctl_val[smc_read]);

may be keep dev_dbg, but the change of 'base' by 'read' is ok.

Thanks,

C. 

>  	return 0;
>  }
> @@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
>  		if (err)
>  			continue;
>  
> -		/*
> -		 * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
> -		 * when board support is present as determined by of property.
> -		 */
> -		err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
> +		err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
>  		if (err)
>  			continue;
>  
> 

Re: [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode

[Rob Lippert]

On Tue, Jan 10, 2017 at 12:10 AM, Cédric Le Goater <clg@kaod.org> wrote:
>
> Hello Robert,
>
> On 01/10/2017 02:05 AM, Robert Lippert wrote:
> > Implements support for the dual IO read mode on aspeed SMC/FMC
> > controllers which uses both MISO and MOSI lines for data during a read
> > to double the read bandwidth.
> >
> > Signed-off-by: Robert Lippert <rlippert@google.com>
> > ---
> >
> >  drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> > index d3bed34f5aa0..a8ca2ab308d7 100644
> > --- a/drivers/mtd/spi-nor/aspeed-smc.c
> > +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> > @@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
> >  {
> >       struct aspeed_smc_chip *chip = nor->priv;
> >       int ret;
> > +     u32 ctl;
> > +     int i;
> > +     u8 dummy = 0;
>
> OxFF would be a better value.

OK.

>
> >       mutex_lock(&chip->controller->mutex);
> >
> > @@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
> >
> >       aspeed_smc_start_user(nor);
> >       aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
> > +
> > +     /* Send dummy bytes */
> > +     for (i = 0; i < (chip->nor.read_dummy / 8); ++i)
>
> the parentheses are not needed I think.

Yes not needed, removed.

>
> > +             aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
> > +
> > +     if (chip->nor.flash_read == SPI_NOR_DUAL) {
> > +             /* Switch to dual I/O mode for data cycle */
> > +             ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
> > +             ctl |= CONTROL_SPI_IO_DUAL_DATA;
> > +             writel(ctl, chip->ctl);
> > +     }
>
> So why can not we set the controller in aspeed_smc_chip_setup_finish()
> once and for all ?

It needs to make sure to only effect the data part of the operation
(the command+addr writes to the device should not be dual-IO mode),
and also needs to not interfere with the aspeed_smc_read_reg read
calls in which the data phase cannot be dual IO mode.

>
> > +
> >       aspeed_smc_read_from_ahb(read_buf, chip->base, len);
> >       aspeed_smc_stop_user(nor);
> >
> > @@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
> >               cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
> >               break;
> >       case SPI_NOR_FAST:
> > +     case SPI_NOR_DUAL:
> >               cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
> >               break;
> >       default:
> > @@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
> >       }
> >
> >       chip->ctl_val[smc_read] |= cmd |
> > +             spi_control_fill_opcode(chip->nor.read_opcode) |
> >               CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
> >
> > -     dev_dbg(controller->dev, "base control register: %08x\n",
> > +     if (chip->nor.flash_read == SPI_NOR_DUAL)
> > +             chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;
>
> may be we can move that part in the switch setting above  ?

Done.

>
> > +     dev_info(controller->dev, "read control register: %08x\n",
> >                chip->ctl_val[smc_read]);
>
> may be keep dev_dbg, but the change of 'base' by 'read' is ok.

OK.

>
> Thanks,
>
> C.
>
> >       return 0;
> >  }
> > @@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
> >               if (err)
> >                       continue;
> >
> > -             /*
> > -              * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
> > -              * when board support is present as determined by of property.
> > -              */
> > -             err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
> > +             err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
> >               if (err)
> >                       continue;
> >
> >
>

Re: [PATCH linux 1/3] mtd: spi-nor: add SPI_NOR_DUAL_READ to mx66l51235l

[Rob Lippert]

On Mon, Jan 9, 2017 at 11:54 PM, Cédric Le Goater <clg@kaod.org> wrote:
> On 01/10/2017 02:05 AM, Robert Lippert wrote:
>> Signed-off-by: Robert Lippert <rlippert@google.com>
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
> This would be good to upstream I think. As I need to send
> a little patchset for the label support in mainline Linux,
> I can include it if you want.

Sounds good to me, no issue with including this change in your patchset.

>
> C.
>
>> ---
>>
>>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index a68073afe8a8..e508c25a47f7 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -857,7 +857,7 @@ static const struct flash_info spi_nor_ids[] = {
>>       { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
>>       { "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
>>       { "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
>> -     { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_QUAD_READ) },
>> +     { "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>>       { "mx66l1g45g",  INFO(0xc2201b, 0, 64 * 1024, 2048, SECT_4K | SPI_NOR_QUAD_READ) },
>>       { "mx66l1g55g",  INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) },
>>
>>
>

Re: [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode

[Cédric Le Goater]

On 01/10/2017 11:54 PM, Rob Lippert wrote:
> On Tue, Jan 10, 2017 at 12:10 AM, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Hello Robert,
>>
>> On 01/10/2017 02:05 AM, Robert Lippert wrote:
>>> Implements support for the dual IO read mode on aspeed SMC/FMC
>>> controllers which uses both MISO and MOSI lines for data during a read
>>> to double the read bandwidth.
>>>
>>> Signed-off-by: Robert Lippert <rlippert@google.com>
>>> ---
>>>
>>>  drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
>>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>> index d3bed34f5aa0..a8ca2ab308d7 100644
>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>> @@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>>  {
>>>       struct aspeed_smc_chip *chip = nor->priv;
>>>       int ret;
>>> +     u32 ctl;
>>> +     int i;
>>> +     u8 dummy = 0;
>>
>> OxFF would be a better value.
> 
> OK.
> 
>>
>>>       mutex_lock(&chip->controller->mutex);
>>>
>>> @@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>>
>>>       aspeed_smc_start_user(nor);
>>>       aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>> +
>>> +     /* Send dummy bytes */
>>> +     for (i = 0; i < (chip->nor.read_dummy / 8); ++i)
>>
>> the parentheses are not needed I think.
> 
> Yes not needed, removed.
> 
>>
>>> +             aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
>>> +
>>> +     if (chip->nor.flash_read == SPI_NOR_DUAL) {
>>> +             /* Switch to dual I/O mode for data cycle */
>>> +             ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
>>> +             ctl |= CONTROL_SPI_IO_DUAL_DATA;
>>> +             writel(ctl, chip->ctl);
>>> +     }
>>
>> So why can not we set the controller in aspeed_smc_chip_setup_finish()
>> once and for all ?
> 
> It needs to make sure to only effect the data part of the operation
> (the command+addr writes to the device should not be dual-IO mode),
> and also needs to not interfere with the aspeed_smc_read_reg read
> calls in which the data phase cannot be dual IO mode.

ok. it makes sense. Is it much faster ? 

Thanks, 

C.

>>
>>> +
>>>       aspeed_smc_read_from_ahb(read_buf, chip->base, len);
>>>       aspeed_smc_stop_user(nor);
>>>
>>> @@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>               cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
>>>               break;
>>>       case SPI_NOR_FAST:
>>> +     case SPI_NOR_DUAL:
>>>               cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
>>>               break;
>>>       default:
>>> @@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>       }
>>>
>>>       chip->ctl_val[smc_read] |= cmd |
>>> +             spi_control_fill_opcode(chip->nor.read_opcode) |
>>>               CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
>>>
>>> -     dev_dbg(controller->dev, "base control register: %08x\n",
>>> +     if (chip->nor.flash_read == SPI_NOR_DUAL)
>>> +             chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;
>>
>> may be we can move that part in the switch setting above  ?
> 
> Done.
> 
>>
>>> +     dev_info(controller->dev, "read control register: %08x\n",
>>>                chip->ctl_val[smc_read]);
>>
>> may be keep dev_dbg, but the change of 'base' by 'read' is ok.
> 
> OK.
> 
>>
>> Thanks,
>>
>> C.
>>
>>>       return 0;
>>>  }
>>> @@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
>>>               if (err)
>>>                       continue;
>>>
>>> -             /*
>>> -              * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
>>> -              * when board support is present as determined by of property.
>>> -              */
>>> -             err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
>>> +             err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
>>>               if (err)
>>>                       continue;
>>>
>>>
>>

Re: [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode

[Rob Lippert]

On Wed, Jan 11, 2017 at 3:43 AM, Cédric Le Goater <clg@kaod.org> wrote:
> On 01/10/2017 11:54 PM, Rob Lippert wrote:
>> On Tue, Jan 10, 2017 at 12:10 AM, Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>> Hello Robert,
>>>
>>> On 01/10/2017 02:05 AM, Robert Lippert wrote:
>>>> Implements support for the dual IO read mode on aspeed SMC/FMC
>>>> controllers which uses both MISO and MOSI lines for data during a read
>>>> to double the read bandwidth.
>>>>
>>>> Signed-off-by: Robert Lippert <rlippert@google.com>
>>>> ---
>>>>
>>>>  drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
>>>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>>> index d3bed34f5aa0..a8ca2ab308d7 100644
>>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>>> @@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>>>  {
>>>>       struct aspeed_smc_chip *chip = nor->priv;
>>>>       int ret;
>>>> +     u32 ctl;
>>>> +     int i;
>>>> +     u8 dummy = 0;
>>>
>>> OxFF would be a better value.
>>
>> OK.
>>
>>>
>>>>       mutex_lock(&chip->controller->mutex);
>>>>
>>>> @@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>>>
>>>>       aspeed_smc_start_user(nor);
>>>>       aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>>> +
>>>> +     /* Send dummy bytes */
>>>> +     for (i = 0; i < (chip->nor.read_dummy / 8); ++i)
>>>
>>> the parentheses are not needed I think.
>>
>> Yes not needed, removed.
>>
>>>
>>>> +             aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
>>>> +
>>>> +     if (chip->nor.flash_read == SPI_NOR_DUAL) {
>>>> +             /* Switch to dual I/O mode for data cycle */
>>>> +             ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
>>>> +             ctl |= CONTROL_SPI_IO_DUAL_DATA;
>>>> +             writel(ctl, chip->ctl);
>>>> +     }
>>>
>>> So why can not we set the controller in aspeed_smc_chip_setup_finish()
>>> once and for all ?
>>
>> It needs to make sure to only effect the data part of the operation
>> (the command+addr writes to the device should not be dual-IO mode),
>> and also needs to not interfere with the aspeed_smc_read_reg read
>> calls in which the data phase cannot be dual IO mode.
>
> ok. it makes sense. Is it much faster ?

Yea it is about twice as much bandwidth, as I mentioned in the cover
letter it drops the time to dump an entire 64MB flash from 12s to 7s.

With a companion change to u-boot to double the SPI frequency to
104Mhz that time drops to 3s and speeds up the entire bmc boot process
(including uboot) by about 15s.

Thanks,
-Rob

>
> Thanks,
>
> C.
>
>>>
>>>> +
>>>>       aspeed_smc_read_from_ahb(read_buf, chip->base, len);
>>>>       aspeed_smc_stop_user(nor);
>>>>
>>>> @@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>>               cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
>>>>               break;
>>>>       case SPI_NOR_FAST:
>>>> +     case SPI_NOR_DUAL:
>>>>               cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
>>>>               break;
>>>>       default:
>>>> @@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>>       }
>>>>
>>>>       chip->ctl_val[smc_read] |= cmd |
>>>> +             spi_control_fill_opcode(chip->nor.read_opcode) |
>>>>               CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
>>>>
>>>> -     dev_dbg(controller->dev, "base control register: %08x\n",
>>>> +     if (chip->nor.flash_read == SPI_NOR_DUAL)
>>>> +             chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;
>>>
>>> may be we can move that part in the switch setting above  ?
>>
>> Done.
>>
>>>
>>>> +     dev_info(controller->dev, "read control register: %08x\n",
>>>>                chip->ctl_val[smc_read]);
>>>
>>> may be keep dev_dbg, but the change of 'base' by 'read' is ok.
>>
>> OK.
>>
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>>       return 0;
>>>>  }
>>>> @@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
>>>>               if (err)
>>>>                       continue;
>>>>
>>>> -             /*
>>>> -              * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
>>>> -              * when board support is present as determined by of property.
>>>> -              */
>>>> -             err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
>>>> +             err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
>>>>               if (err)
>>>>                       continue;
>>>>
>>>>
>>>
>

Re: [PATCH linux 2/3] mtd: spi-nor: aspeed: add support for SPI dual IO read mode

[Cédric Le Goater]

On 01/11/2017 07:34 PM, Rob Lippert wrote:
> On Wed, Jan 11, 2017 at 3:43 AM, Cédric Le Goater <clg@kaod.org> wrote:
>> On 01/10/2017 11:54 PM, Rob Lippert wrote:
>>> On Tue, Jan 10, 2017 at 12:10 AM, Cédric Le Goater <clg@kaod.org> wrote:
>>>>
>>>> Hello Robert,
>>>>
>>>> On 01/10/2017 02:05 AM, Robert Lippert wrote:
>>>>> Implements support for the dual IO read mode on aspeed SMC/FMC
>>>>> controllers which uses both MISO and MOSI lines for data during a read
>>>>> to double the read bandwidth.
>>>>>
>>>>> Signed-off-by: Robert Lippert <rlippert@google.com>
>>>>> ---
>>>>>
>>>>>  drivers/mtd/spi-nor/aspeed-smc.c | 28 ++++++++++++++++++++++------
>>>>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>>>> index d3bed34f5aa0..a8ca2ab308d7 100644
>>>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>>>> @@ -540,6 +540,9 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>>>>  {
>>>>>       struct aspeed_smc_chip *chip = nor->priv;
>>>>>       int ret;
>>>>> +     u32 ctl;
>>>>> +     int i;
>>>>> +     u8 dummy = 0;
>>>>
>>>> OxFF would be a better value.
>>>
>>> OK.
>>>
>>>>
>>>>>       mutex_lock(&chip->controller->mutex);
>>>>>
>>>>> @@ -557,6 +560,18 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>>>>>
>>>>>       aspeed_smc_start_user(nor);
>>>>>       aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>>>> +
>>>>> +     /* Send dummy bytes */
>>>>> +     for (i = 0; i < (chip->nor.read_dummy / 8); ++i)
>>>>
>>>> the parentheses are not needed I think.
>>>
>>> Yes not needed, removed.
>>>
>>>>
>>>>> +             aspeed_smc_write_to_ahb(chip->base, &dummy, 1);
>>>>> +
>>>>> +     if (chip->nor.flash_read == SPI_NOR_DUAL) {
>>>>> +             /* Switch to dual I/O mode for data cycle */
>>>>> +             ctl = readl(chip->ctl) & ~CONTROL_SPI_IO_MODE_MASK;
>>>>> +             ctl |= CONTROL_SPI_IO_DUAL_DATA;
>>>>> +             writel(ctl, chip->ctl);
>>>>> +     }
>>>>
>>>> So why can not we set the controller in aspeed_smc_chip_setup_finish()
>>>> once and for all ?
>>>
>>> It needs to make sure to only effect the data part of the operation
>>> (the command+addr writes to the device should not be dual-IO mode),
>>> and also needs to not interfere with the aspeed_smc_read_reg read
>>> calls in which the data phase cannot be dual IO mode.
>>
>> ok. it makes sense. Is it much faster ?
> 
> Yea it is about twice as much bandwidth, as I mentioned in the cover
> letter it drops the time to dump an entire 64MB flash from 12s to 7s.

Good. Sorry for asking, I scanned the cover a bit quickly it seems. 

> With a companion change to u-boot to double the SPI frequency to
> 104Mhz that time drops to 3s and speeds up the entire bmc boot process
> (including uboot) by about 15s.

yes we need to add that to the driver also. I have a few other patches
improving the timing settings but I have been focusing on upstreaming 
first.

Thanks,

C. 

 
> Thanks,
> -Rob
> 
>>
>> Thanks,
>>
>> C.
>>
>>>>
>>>>> +
>>>>>       aspeed_smc_read_from_ahb(read_buf, chip->base, len);
>>>>>       aspeed_smc_stop_user(nor);
>>>>>
>>>>> @@ -868,6 +883,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>>>               cmd = CONTROL_SPI_COMMAND_MODE_NORMAL;
>>>>>               break;
>>>>>       case SPI_NOR_FAST:
>>>>> +     case SPI_NOR_DUAL:
>>>>>               cmd = CONTROL_SPI_COMMAND_MODE_FREAD;
>>>>>               break;
>>>>>       default:
>>>>> @@ -876,9 +892,13 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>>>       }
>>>>>
>>>>>       chip->ctl_val[smc_read] |= cmd |
>>>>> +             spi_control_fill_opcode(chip->nor.read_opcode) |
>>>>>               CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8);
>>>>>
>>>>> -     dev_dbg(controller->dev, "base control register: %08x\n",
>>>>> +     if (chip->nor.flash_read == SPI_NOR_DUAL)
>>>>> +             chip->ctl_val[smc_read] |= CONTROL_SPI_IO_DUAL_DATA;
>>>>
>>>> may be we can move that part in the switch setting above  ?
>>>
>>> Done.
>>>
>>>>
>>>>> +     dev_info(controller->dev, "read control register: %08x\n",
>>>>>                chip->ctl_val[smc_read]);
>>>>
>>>> may be keep dev_dbg, but the change of 'base' by 'read' is ok.
>>>
>>> OK.
>>>
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>>>>       return 0;
>>>>>  }
>>>>> @@ -980,11 +1000,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
>>>>>               if (err)
>>>>>                       continue;
>>>>>
>>>>> -             /*
>>>>> -              * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach
>>>>> -              * when board support is present as determined by of property.
>>>>> -              */
>>>>> -             err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL);
>>>>> +             err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_DUAL);
>>>>>               if (err)
>>>>>                       continue;
>>>>>
>>>>>
>>>>
>>