[PATCH 0/4] mtd: spi-nor: aspeed: add dual read and command mode support

[PATCH 0/4] mtd: spi-nor: aspeed: add dual read and command mode support

[Cédric Le Goater]

Hello,

Here is a short series adding support for Dual read and for the
controller mode "Command". The later is to perform direct memory
access of the flash contents on the AHB Bus. A prereq to use this mode
is to configure the segment registers of the controller which define
the mapping window of the chips on the AHB bus.

Thanks,

C.

Cédric Le Goater (3):
  mtd: spi-nor: aspeed: remove dummies from keep mask
  mtd: spi-nor: aspeed: configure chip window on AHB bus
  mtd: spi-nor: aspeed: use command mode for reads

Robert Lippert (1):
  mtd: spi-nor: aspeed: add support for SPI dual IO read mode

 drivers/mtd/spi-nor/aspeed-smc.c | 205 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 194 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH 1/4] mtd: spi-nor: aspeed: remove dummies from keep mask

[Cédric Le Goater]

There is no need to keep the dummy bytes in the control register if
the command mode is not kept also. This could lead to an inconsistent
setting : normal read mode (command 0x3) and dummy bytes. It is to be
noted that the HW allows such a configuration.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 56051d30f000..2940f2098420 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -180,8 +180,7 @@ struct aspeed_smc_controller {
 
 #define CONTROL_KEEP_MASK						\
 	(CONTROL_AAF_MODE | CONTROL_CE_INACTIVE_MASK | CONTROL_CLK_DIV4 | \
-	 CONTROL_IO_DUMMY_MASK | CONTROL_CLOCK_FREQ_SEL_MASK |		\
-	 CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
+	 CONTROL_CLOCK_FREQ_SEL_MASK | CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
 
 /*
  * The Segment Register uses a 8MB unit to encode the start address
-- 
2.7.4

Re: [PATCH 1/4] mtd: spi-nor: aspeed: remove dummies from keep mask

[Marek Vasut]

On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
> There is no need to keep the dummy bytes in the control register if
> the command mode is not kept also. This could lead to an inconsistent
> setting : normal read mode (command 0x3) and dummy bytes. It is to be
> noted that the HW allows such a configuration.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Marek Vasut <marek.vasut@gmail.com>

> ---
>  drivers/mtd/spi-nor/aspeed-smc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index 56051d30f000..2940f2098420 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -180,8 +180,7 @@ struct aspeed_smc_controller {
>  
>  #define CONTROL_KEEP_MASK						\
>  	(CONTROL_AAF_MODE | CONTROL_CE_INACTIVE_MASK | CONTROL_CLK_DIV4 | \
> -	 CONTROL_IO_DUMMY_MASK | CONTROL_CLOCK_FREQ_SEL_MASK |		\
> -	 CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
> +	 CONTROL_CLOCK_FREQ_SEL_MASK | CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
>  
>  /*
>   * The Segment Register uses a 8MB unit to encode the start address
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH 1/4] mtd: spi-nor: aspeed: remove dummies from keep mask

[Cyrille Pitchen]

Le 20/04/2017 à 15:28, Marek Vasut a écrit :
> On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
>> There is no need to keep the dummy bytes in the control register if
>> the command mode is not kept also. This could lead to an inconsistent
>> setting : normal read mode (command 0x3) and dummy bytes. It is to be
>> noted that the HW allows such a configuration.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Reviewed-by: Marek Vasut <marek.vasut@gmail.com>
>

Applied to the spi-nor/next branch of l2-mtd

Thanks!

>> ---
>>  drivers/mtd/spi-nor/aspeed-smc.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>> index 56051d30f000..2940f2098420 100644
>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>> @@ -180,8 +180,7 @@ struct aspeed_smc_controller {
>>  
>>  #define CONTROL_KEEP_MASK						\
>>  	(CONTROL_AAF_MODE | CONTROL_CE_INACTIVE_MASK | CONTROL_CLK_DIV4 | \
>> -	 CONTROL_IO_DUMMY_MASK | CONTROL_CLOCK_FREQ_SEL_MASK |		\
>> -	 CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
>> +	 CONTROL_CLOCK_FREQ_SEL_MASK | CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
>>  
>>  /*
>>   * The Segment Register uses a 8MB unit to encode the start address
>>
> 
> 

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

[Cédric Le Goater]

From: Robert Lippert <roblip@gmail.com>

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>
[clg: adapted to mainline driver ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 2940f2098420..183d950e621b 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -372,12 +372,20 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
 	struct aspeed_smc_chip *chip = nor->priv;
 	int i;
 	u8 dummy = 0xFF;
+	u32 ctl;
 
 	aspeed_smc_start_user(nor);
 	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
 	for (i = 0; i < chip->nor.read_dummy / 8; i++)
 		aspeed_smc_write_to_ahb(chip->ahb_base, &dummy, sizeof(dummy));
 
+	if (chip->nor.flash_read == SPI_NOR_DUAL) {
+		/* Switch to dual I/O mode for data cycle */
+		ctl = readl(chip->ctl) & ~CONTROL_IO_MODE_MASK;
+		ctl |= CONTROL_IO_DUAL_DATA;
+		writel(ctl, chip->ctl);
+	}
+
 	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
 	aspeed_smc_stop_user(nor);
 	return len;
@@ -591,6 +599,9 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 	case SPI_NOR_FAST:
 		cmd = CONTROL_COMMAND_MODE_FREAD;
 		break;
+	case SPI_NOR_DUAL:
+		cmd = CONTROL_COMMAND_MODE_FREAD | CONTROL_IO_DUAL_DATA;
+		break;
 	default:
 		dev_err(chip->nor.dev, "unsupported SPI read mode\n");
 		return -EINVAL;
@@ -599,7 +610,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 	chip->ctl_val[smc_read] |= cmd |
 		CONTROL_IO_DUMMY_SET(chip->nor.read_dummy / 8);
 
-	dev_dbg(controller->dev, "base control register: %08x\n",
+	dev_dbg(controller->dev, "read control register: %08x\n",
 		chip->ctl_val[smc_read]);
 	return 0;
 }
@@ -670,11 +681,9 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
 			break;
 
 		/*
-		 * TODO: Add support for SPI_NOR_QUAD and SPI_NOR_DUAL
-		 * attach when board support is present as determined
-		 * by of property.
+		 * Aspeed SoCs only support Dual mode
 		 */
-		ret = spi_nor_scan(nor, NULL, SPI_NOR_NORMAL);
+		ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL);
 		if (ret)
 			break;
 
-- 
2.7.4

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

[Marek Vasut]

On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
> From: Robert Lippert <roblip@gmail.com>
> 
> 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>
> [clg: adapted to mainline driver ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Marek Vasut <marek.vasut@gmail.com>

-- 
Best regards,
Marek Vasut

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

[Cyrille Pitchen]

Hi Cédric,


Le 20/04/2017 à 13:56, Cédric Le Goater a écrit :
> From: Robert Lippert <roblip@gmail.com>
> 
> 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>
> [clg: adapted to mainline driver ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  drivers/mtd/spi-nor/aspeed-smc.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index 2940f2098420..183d950e621b 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -372,12 +372,20 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>  	struct aspeed_smc_chip *chip = nor->priv;
>  	int i;
>  	u8 dummy = 0xFF;
> +	u32 ctl;
>  
>  	aspeed_smc_start_user(nor);
>  	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>  	for (i = 0; i < chip->nor.read_dummy / 8; i++)
>  		aspeed_smc_write_to_ahb(chip->ahb_base, &dummy, sizeof(dummy));
>  
> +	if (chip->nor.flash_read == SPI_NOR_DUAL) {
> +		/* Switch to dual I/O mode for data cycle */
> +		ctl = readl(chip->ctl) & ~CONTROL_IO_MODE_MASK;
> +		ctl |= CONTROL_IO_DUAL_DATA;
> +		writel(ctl, chip->ctl);
> +	}
> +

As expected this patch doesn't apply as is to the spi-nor/next branch of
l2-mtd because flash_read and SPI_NOR_{DUAL|QUAD} enums no longer exists.

Besides it looks like the Aspeed controller can support SPI 1-2-2 as
well as SPI 1-1-2.

I see:
#define CONTROL_IO_DUAL_DATA		BIT(29)
#define CONTROL_IO_DUAL_ADDR_DATA	(BIT(29) | BIT(28))

Now spi_nor_scan() makes the difference between these 2 SPI protocols.

Besides, you may also want to use the compatible DT string to make the
difference between Aspeed controllers also supporting SPI 1-1-4 and SPI
1-4-4 and other controllers supporting only Dual SPI protocols so you
could fill the hwcaps parameter of spi_nor_scan() as needed.

So I think we should spend a little more time to rework this patch.

Best regards,

Cyrille


>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>  	aspeed_smc_stop_user(nor);
>  	return len;
> @@ -591,6 +599,9 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>  	case SPI_NOR_FAST:
>  		cmd = CONTROL_COMMAND_MODE_FREAD;
>  		break;
> +	case SPI_NOR_DUAL:
> +		cmd = CONTROL_COMMAND_MODE_FREAD | CONTROL_IO_DUAL_DATA;
> +		break;
>  	default:
>  		dev_err(chip->nor.dev, "unsupported SPI read mode\n");
>  		return -EINVAL;
> @@ -599,7 +610,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>  	chip->ctl_val[smc_read] |= cmd |
>  		CONTROL_IO_DUMMY_SET(chip->nor.read_dummy / 8);
>  
> -	dev_dbg(controller->dev, "base control register: %08x\n",
> +	dev_dbg(controller->dev, "read control register: %08x\n",
>  		chip->ctl_val[smc_read]);
>  	return 0;
>  }
> @@ -670,11 +681,9 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>  			break;
>  
>  		/*
> -		 * TODO: Add support for SPI_NOR_QUAD and SPI_NOR_DUAL
> -		 * attach when board support is present as determined
> -		 * by of property.
> +		 * Aspeed SoCs only support Dual mode
>  		 */
> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_NORMAL);
> +		ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL);
>  		if (ret)
>  			break;
>  
> 

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

[Cédric Le Goater]

On 06/21/2017 12:50 AM, Cyrille Pitchen wrote:
> Hi Cédric,
> 
> 
> Le 20/04/2017 à 13:56, Cédric Le Goater a écrit :
>> From: Robert Lippert <roblip@gmail.com>
>>
>> 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>
>> [clg: adapted to mainline driver ]
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  drivers/mtd/spi-nor/aspeed-smc.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>> index 2940f2098420..183d950e621b 100644
>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>> @@ -372,12 +372,20 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>  	struct aspeed_smc_chip *chip = nor->priv;
>>  	int i;
>>  	u8 dummy = 0xFF;
>> +	u32 ctl;
>>  
>>  	aspeed_smc_start_user(nor);
>>  	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>  	for (i = 0; i < chip->nor.read_dummy / 8; i++)
>>  		aspeed_smc_write_to_ahb(chip->ahb_base, &dummy, sizeof(dummy));
>>  
>> +	if (chip->nor.flash_read == SPI_NOR_DUAL) {
>> +		/* Switch to dual I/O mode for data cycle */
>> +		ctl = readl(chip->ctl) & ~CONTROL_IO_MODE_MASK;
>> +		ctl |= CONTROL_IO_DUAL_DATA;
>> +		writel(ctl, chip->ctl);
>> +	}
>> +
> 
> As expected this patch doesn't apply as is to the spi-nor/next branch of
> l2-mtd because flash_read and SPI_NOR_{DUAL|QUAD} enums no longer exists.
> 
> Besides it looks like the Aspeed controller can support SPI 1-2-2 as
> well as SPI 1-1-2.
> 
> I see:
> #define CONTROL_IO_DUAL_DATA		BIT(29)
> #define CONTROL_IO_DUAL_ADDR_DATA	(BIT(29) | BIT(28))
> 
> Now spi_nor_scan() makes the difference between these 2 SPI protocols.
>
> Besides, you may also want to use the compatible DT string to make the
> difference between Aspeed controllers also supporting SPI 1-1-4 and SPI
> 1-4-4 and other controllers supporting only Dual SPI protocols so you
> could fill the hwcaps parameter of spi_nor_scan() as needed.
>
> So I think we should spend a little more time to rework this patch.

OK. I will take a look.

Please consider taking :

  [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

while I reconcile my patchset with the spi-nor tree.

Thanks,

C.

> 
>>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>  	aspeed_smc_stop_user(nor);
>>  	return len;
>> @@ -591,6 +599,9 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>  	case SPI_NOR_FAST:
>>  		cmd = CONTROL_COMMAND_MODE_FREAD;
>>  		break;
>> +	case SPI_NOR_DUAL:
>> +		cmd = CONTROL_COMMAND_MODE_FREAD | CONTROL_IO_DUAL_DATA;
>> +		break;
>>  	default:
>>  		dev_err(chip->nor.dev, "unsupported SPI read mode\n");
>>  		return -EINVAL;
>> @@ -599,7 +610,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>  	chip->ctl_val[smc_read] |= cmd |
>>  		CONTROL_IO_DUMMY_SET(chip->nor.read_dummy / 8);
>>  
>> -	dev_dbg(controller->dev, "base control register: %08x\n",
>> +	dev_dbg(controller->dev, "read control register: %08x\n",
>>  		chip->ctl_val[smc_read]);
>>  	return 0;
>>  }
>> @@ -670,11 +681,9 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>>  			break;
>>  
>>  		/*
>> -		 * TODO: Add support for SPI_NOR_QUAD and SPI_NOR_DUAL
>> -		 * attach when board support is present as determined
>> -		 * by of property.
>> +		 * Aspeed SoCs only support Dual mode
>>  		 */
>> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_NORMAL);
>> +		ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL);
>>  		if (ret)
>>  			break;
>>  
>>
> 

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

[Cédric Le Goater]

On 06/21/2017 09:32 AM, Cédric Le Goater wrote:
> On 06/21/2017 12:50 AM, Cyrille Pitchen wrote:
>> Hi Cédric,
>>
>>
>> Le 20/04/2017 à 13:56, Cédric Le Goater a écrit :
>>> From: Robert Lippert <roblip@gmail.com>
>>>
>>> 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>
>>> [clg: adapted to mainline driver ]
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>  drivers/mtd/spi-nor/aspeed-smc.c | 19 ++++++++++++++-----
>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>> index 2940f2098420..183d950e621b 100644
>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>> @@ -372,12 +372,20 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>>  	struct aspeed_smc_chip *chip = nor->priv;
>>>  	int i;
>>>  	u8 dummy = 0xFF;
>>> +	u32 ctl;
>>>  
>>>  	aspeed_smc_start_user(nor);
>>>  	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>>  	for (i = 0; i < chip->nor.read_dummy / 8; i++)
>>>  		aspeed_smc_write_to_ahb(chip->ahb_base, &dummy, sizeof(dummy));
>>>  
>>> +	if (chip->nor.flash_read == SPI_NOR_DUAL) {
>>> +		/* Switch to dual I/O mode for data cycle */
>>> +		ctl = readl(chip->ctl) & ~CONTROL_IO_MODE_MASK;
>>> +		ctl |= CONTROL_IO_DUAL_DATA;
>>> +		writel(ctl, chip->ctl);
>>> +	}
>>> +
>>
>> As expected this patch doesn't apply as is to the spi-nor/next branch of
>> l2-mtd because flash_read and SPI_NOR_{DUAL|QUAD} enums no longer exists.
>>
>> Besides it looks like the Aspeed controller can support SPI 1-2-2 as
>> well as SPI 1-1-2.
>>
>> I see:
>> #define CONTROL_IO_DUAL_DATA		BIT(29)
>> #define CONTROL_IO_DUAL_ADDR_DATA	(BIT(29) | BIT(28))
>>
>> Now spi_nor_scan() makes the difference between these 2 SPI protocols.
>>
>> Besides, you may also want to use the compatible DT string to make the
>> difference between Aspeed controllers also supporting SPI 1-1-4 and SPI
>> 1-4-4 and other controllers supporting only Dual SPI protocols so you
>> could fill the hwcaps parameter of spi_nor_scan() as needed.
>>
>> So I think we should spend a little more time to rework this patch.
> 
> OK. I will take a look.
> 
> Please consider taking :
> 
>   [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

Forget it. I am having issues with this patch on newer kernels. I will 
cook a patchset after the merge window.

Thanks,

C. 
   
 
> while I reconcile my patchset with the spi-nor tree.
> 
> Thanks,
> 
> C.
> 
>>
>>>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>>  	aspeed_smc_stop_user(nor);
>>>  	return len;
>>> @@ -591,6 +599,9 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>  	case SPI_NOR_FAST:
>>>  		cmd = CONTROL_COMMAND_MODE_FREAD;
>>>  		break;
>>> +	case SPI_NOR_DUAL:
>>> +		cmd = CONTROL_COMMAND_MODE_FREAD | CONTROL_IO_DUAL_DATA;
>>> +		break;
>>>  	default:
>>>  		dev_err(chip->nor.dev, "unsupported SPI read mode\n");
>>>  		return -EINVAL;
>>> @@ -599,7 +610,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>  	chip->ctl_val[smc_read] |= cmd |
>>>  		CONTROL_IO_DUMMY_SET(chip->nor.read_dummy / 8);
>>>  
>>> -	dev_dbg(controller->dev, "base control register: %08x\n",
>>> +	dev_dbg(controller->dev, "read control register: %08x\n",
>>>  		chip->ctl_val[smc_read]);
>>>  	return 0;
>>>  }
>>> @@ -670,11 +681,9 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>>>  			break;
>>>  
>>>  		/*
>>> -		 * TODO: Add support for SPI_NOR_QUAD and SPI_NOR_DUAL
>>> -		 * attach when board support is present as determined
>>> -		 * by of property.
>>> +		 * Aspeed SoCs only support Dual mode
>>>  		 */
>>> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_NORMAL);
>>> +		ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL);
>>>  		if (ret)
>>>  			break;
>>>  
>>>
>>
> 

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

[Cyrille Pitchen]

Le 21/06/2017 à 14:25, Cédric Le Goater a écrit :
> On 06/21/2017 09:32 AM, Cédric Le Goater wrote:
>> On 06/21/2017 12:50 AM, Cyrille Pitchen wrote:
>>> Hi Cédric,
>>>
>>>
>>> Le 20/04/2017 à 13:56, Cédric Le Goater a écrit :
>>>> From: Robert Lippert <roblip@gmail.com>
>>>>
>>>> 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>
>>>> [clg: adapted to mainline driver ]
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>  drivers/mtd/spi-nor/aspeed-smc.c | 19 ++++++++++++++-----
>>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>>> index 2940f2098420..183d950e621b 100644
>>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>>> @@ -372,12 +372,20 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>>>  	struct aspeed_smc_chip *chip = nor->priv;
>>>>  	int i;
>>>>  	u8 dummy = 0xFF;
>>>> +	u32 ctl;
>>>>  
>>>>  	aspeed_smc_start_user(nor);
>>>>  	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
>>>>  	for (i = 0; i < chip->nor.read_dummy / 8; i++)
>>>>  		aspeed_smc_write_to_ahb(chip->ahb_base, &dummy, sizeof(dummy));
>>>>  
>>>> +	if (chip->nor.flash_read == SPI_NOR_DUAL) {
>>>> +		/* Switch to dual I/O mode for data cycle */
>>>> +		ctl = readl(chip->ctl) & ~CONTROL_IO_MODE_MASK;
>>>> +		ctl |= CONTROL_IO_DUAL_DATA;
>>>> +		writel(ctl, chip->ctl);
>>>> +	}
>>>> +
>>>
>>> As expected this patch doesn't apply as is to the spi-nor/next branch of
>>> l2-mtd because flash_read and SPI_NOR_{DUAL|QUAD} enums no longer exists.
>>>
>>> Besides it looks like the Aspeed controller can support SPI 1-2-2 as
>>> well as SPI 1-1-2.
>>>
>>> I see:
>>> #define CONTROL_IO_DUAL_DATA		BIT(29)
>>> #define CONTROL_IO_DUAL_ADDR_DATA	(BIT(29) | BIT(28))
>>>
>>> Now spi_nor_scan() makes the difference between these 2 SPI protocols.
>>>
>>> Besides, you may also want to use the compatible DT string to make the
>>> difference between Aspeed controllers also supporting SPI 1-1-4 and SPI
>>> 1-4-4 and other controllers supporting only Dual SPI protocols so you
>>> could fill the hwcaps parameter of spi_nor_scan() as needed.
>>>
>>> So I think we should spend a little more time to rework this patch.
>>
>> OK. I will take a look.
>>
>> Please consider taking :
>>
>>   [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads
> 
> Forget it. I am having issues with this patch on newer kernels. I will 
> cook a patchset after the merge window.

OK, so I've updated the state of patch 4 to "rejected" in patchwork.

Best regards,

Cyrille
> 
> Thanks,
> 
> C. 
>    
>  
>> while I reconcile my patchset with the spi-nor tree.
>>
>> Thanks,
>>
>> C.
>>
>>>
>>>>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>>>  	aspeed_smc_stop_user(nor);
>>>>  	return len;
>>>> @@ -591,6 +599,9 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>>  	case SPI_NOR_FAST:
>>>>  		cmd = CONTROL_COMMAND_MODE_FREAD;
>>>>  		break;
>>>> +	case SPI_NOR_DUAL:
>>>> +		cmd = CONTROL_COMMAND_MODE_FREAD | CONTROL_IO_DUAL_DATA;
>>>> +		break;
>>>>  	default:
>>>>  		dev_err(chip->nor.dev, "unsupported SPI read mode\n");
>>>>  		return -EINVAL;
>>>> @@ -599,7 +610,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
>>>>  	chip->ctl_val[smc_read] |= cmd |
>>>>  		CONTROL_IO_DUMMY_SET(chip->nor.read_dummy / 8);
>>>>  
>>>> -	dev_dbg(controller->dev, "base control register: %08x\n",
>>>> +	dev_dbg(controller->dev, "read control register: %08x\n",
>>>>  		chip->ctl_val[smc_read]);
>>>>  	return 0;
>>>>  }
>>>> @@ -670,11 +681,9 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>>>>  			break;
>>>>  
>>>>  		/*
>>>> -		 * TODO: Add support for SPI_NOR_QUAD and SPI_NOR_DUAL
>>>> -		 * attach when board support is present as determined
>>>> -		 * by of property.
>>>> +		 * Aspeed SoCs only support Dual mode
>>>>  		 */
>>>> -		ret = spi_nor_scan(nor, NULL, SPI_NOR_NORMAL);
>>>> +		ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL);
>>>>  		if (ret)
>>>>  			break;
>>>>  
>>>>
>>>
>>
> 
> 

[PATCH 3/4] mtd: spi-nor: aspeed: configure chip window on AHB bus

[Cédric Le Goater]

The segment registers of the SMC controller provide a way to configure
the mapping windows of the chips on the AHB bus. The settings are
required to be correct when the controller operates in Command mode,
which is the case for DMAs and the LPC mapping.

This tries to set the segment registers of each chip depending on the
size of the flash device and depending on the previous segment
settings, in order to have a contiguous window across multiple chips.

Unfortunately, the AST2500 SPI controller has a bug and it is not
possible to configure a full 128MB window for a chip of the same
size. The window size needs to be restricted to 120MB. This issue only
applies to CE0.

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

 Changes since initial version :

 - fixed comments
 - renamed variables to be more explicit
 - introduced aspeed_smc_ahb_base_phy()
 - slightly reworked the code for the AST2400 SPI support 
 - introduced a 'ahb_window_size' field in the controller holding the
   size of the overall AHB window.
 
 drivers/mtd/spi-nor/aspeed-smc.c | 156 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 153 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 183d950e621b..60c020482946 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -97,6 +97,7 @@ struct aspeed_smc_chip {
 	struct aspeed_smc_controller *controller;
 	void __iomem *ctl;			/* control register */
 	void __iomem *ahb_base;			/* base of chip window */
+	u32 ahb_window_size;			/* chip mapping window size */
 	u32 ctl_val[smc_max];			/* control settings */
 	enum aspeed_smc_flash_type type;	/* what type of flash */
 	struct spi_nor nor;
@@ -109,6 +110,7 @@ struct aspeed_smc_controller {
 	const struct aspeed_smc_info *info;	/* type info of controller */
 	void __iomem *regs;			/* controller registers */
 	void __iomem *ahb_base;			/* per-chip windows resource */
+	u32 ahb_window_size;			/* full mapping window size */
 
 	struct aspeed_smc_chip *chips[0];	/* pointers to attached chips */
 };
@@ -193,6 +195,10 @@ struct aspeed_smc_controller {
 #define SEGMENT_ADDR_REG0		0x30
 #define SEGMENT_ADDR_START(_r)		((((_r) >> 16) & 0xFF) << 23)
 #define SEGMENT_ADDR_END(_r)		((((_r) >> 24) & 0xFF) << 23)
+#define SEGMENT_ADDR_VALUE(start, end)					\
+	(((((start) >> 23) & 0xFF) << 16) | ((((end) >> 23) & 0xFF) << 24))
+#define SEGMENT_ADDR_REG(controller, cs)	\
+	((controller)->regs + SEGMENT_ADDR_REG0 + (cs) * 4)
 
 /*
  * In user mode all data bytes read or written to the chip decode address
@@ -446,8 +452,7 @@ static void __iomem *aspeed_smc_chip_base(struct aspeed_smc_chip *chip,
 	u32 reg;
 
 	if (controller->info->nce > 1) {
-		reg = readl(controller->regs + SEGMENT_ADDR_REG0 +
-			    chip->cs * 4);
+		reg = readl(SEGMENT_ADDR_REG(controller, chip->cs));
 
 		if (SEGMENT_ADDR_START(reg) >= SEGMENT_ADDR_END(reg))
 			return NULL;
@@ -458,6 +463,146 @@ static void __iomem *aspeed_smc_chip_base(struct aspeed_smc_chip *chip,
 	return controller->ahb_base + offset;
 }
 
+static u32 aspeed_smc_ahb_base_phy(struct aspeed_smc_controller *controller)
+{
+	u32 seg0_val = readl(SEGMENT_ADDR_REG(controller, 0));
+
+	return SEGMENT_ADDR_START(seg0_val);
+}
+
+static u32 chip_set_segment(struct aspeed_smc_chip *chip, u32 cs, u32 start,
+			    u32 size)
+{
+	struct aspeed_smc_controller *controller = chip->controller;
+	void __iomem *seg_reg;
+	u32 seg_oldval, seg_newval, ahb_base_phy, end;
+
+	ahb_base_phy = aspeed_smc_ahb_base_phy(controller);
+
+	seg_reg = SEGMENT_ADDR_REG(controller, cs);
+	seg_oldval = readl(seg_reg);
+
+	/*
+	 * If the chip size is not specified, use the default segment
+	 * size, but take into account the possible overlap with the
+	 * previous segment
+	 */
+	if (!size)
+		size = SEGMENT_ADDR_END(seg_oldval) - start;
+
+	/*
+	 * The segment cannot exceed the maximum window size of the
+	 * controller.
+	 */
+	if (start + size > ahb_base_phy + controller->ahb_window_size) {
+		size = ahb_base_phy + controller->ahb_window_size - start;
+		dev_warn(chip->nor.dev, "CE%d window resized to %dMB",
+			 cs, size >> 20);
+	}
+
+	end = start + size;
+	seg_newval = SEGMENT_ADDR_VALUE(start, end);
+	writel(seg_newval, seg_reg);
+
+	/*
+	 * Restore default value if something goes wrong. The chip
+	 * might have set some bogus value and we would loose access
+	 * to the chip.
+	 */
+	if (seg_newval != readl(seg_reg)) {
+		dev_err(chip->nor.dev, "CE%d window invalid", cs);
+		writel(seg_oldval, seg_reg);
+		start = SEGMENT_ADDR_START(seg_oldval);
+		end = SEGMENT_ADDR_END(seg_oldval);
+		size = end - start;
+	}
+
+	dev_info(chip->nor.dev, "CE%d window [ 0x%.8x - 0x%.8x ] %dMB",
+		 cs, start, end, size >> 20);
+
+	return size;
+}
+
+/*
+ * The segment register defines the mapping window on the AHB bus and
+ * it needs to be configured depending on the chip size. The segment
+ * register of the following CE also needs to be tuned in order to
+ * provide a contiguous window across multiple chips.
+ *
+ * This is expected to be called in increasing CE order
+ */
+static u32 aspeed_smc_chip_set_segment(struct aspeed_smc_chip *chip)
+{
+	struct aspeed_smc_controller *controller = chip->controller;
+	u32 ahb_base_phy, start;
+	u32 size = chip->nor.mtd.size;
+
+	/*
+	 * Each controller has a chip size limit for direct memory
+	 * access
+	 */
+	if (size > controller->info->maxsize)
+		size = controller->info->maxsize;
+
+	/*
+	 * The AST2400 SPI controller only handles one chip and does
+	 * not have segment registers. Let's use the chip size for the
+	 * AHB window.
+	 */
+	if (controller->info == &spi_2400_info)
+		goto out;
+
+	/*
+	 * The AST2500 SPI controller has a HW bug when the CE0 chip
+	 * size reaches 128MB. Enforce a size limit of 120MB to
+	 * prevent the controller from using bogus settings in the
+	 * segment register.
+	 */
+	if (chip->cs == 0 && controller->info == &spi_2500_info &&
+	    size == SZ_128M) {
+		size = 120 << 20;
+		dev_info(chip->nor.dev,
+			 "CE%d window resized to %dMB (AST2500 HW quirk)",
+			 chip->cs, size >> 20);
+	}
+
+	ahb_base_phy = aspeed_smc_ahb_base_phy(controller);
+
+	/*
+	 * As a start address for the current segment, use the default
+	 * start address if we are handling CE0 or use the previous
+	 * segment ending address
+	 */
+	if (chip->cs) {
+		u32 prev = readl(SEGMENT_ADDR_REG(controller, chip->cs - 1));
+
+		start = SEGMENT_ADDR_END(prev);
+	} else {
+		start = ahb_base_phy;
+	}
+
+	size = chip_set_segment(chip, chip->cs, start, size);
+
+	/* Update chip base address on the AHB bus */
+	chip->ahb_base = controller->ahb_base + (start - ahb_base_phy);
+
+	/*
+	 * Now, make sure the next segment does not overlap with the
+	 * current one we just configured, even if there is no
+	 * available chip. That could break access in Command Mode.
+	 */
+	if (chip->cs < controller->info->nce - 1)
+		chip_set_segment(chip, chip->cs + 1, start + size, 0);
+
+out:
+	if (size < chip->nor.mtd.size)
+		dev_warn(chip->nor.dev,
+			 "CE%d window too small for chip %dMB",
+			 chip->cs, (u32)chip->nor.mtd.size >> 20);
+
+	return size;
+}
+
 static void aspeed_smc_chip_enable_write(struct aspeed_smc_chip *chip)
 {
 	struct aspeed_smc_controller *controller = chip->controller;
@@ -531,7 +676,7 @@ static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
 	 */
 	chip->ahb_base = aspeed_smc_chip_base(chip, res);
 	if (!chip->ahb_base) {
-		dev_warn(chip->nor.dev, "CE segment window closed.\n");
+		dev_warn(chip->nor.dev, "CE%d window closed", chip->cs);
 		return -EINVAL;
 	}
 
@@ -578,6 +723,9 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 	if (chip->nor.addr_width == 4 && info->set_4b)
 		info->set_4b(chip);
 
+	/* This is for direct AHB access when using Command Mode. */
+	chip->ahb_window_size = aspeed_smc_chip_set_segment(chip);
+
 	/*
 	 * base mode has not been optimized yet. use it for writes.
 	 */
@@ -739,6 +887,8 @@ static int aspeed_smc_probe(struct platform_device *pdev)
 	if (IS_ERR(controller->ahb_base))
 		return PTR_ERR(controller->ahb_base);
 
+	controller->ahb_window_size = resource_size(res);
+
 	ret = aspeed_smc_setup_flash(controller, np, res);
 	if (ret)
 		dev_err(dev, "Aspeed SMC probe failed %d\n", ret);
-- 
2.7.4

Re: [PATCH 3/4] mtd: spi-nor: aspeed: configure chip window on AHB bus

[Marek Vasut]

On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
> The segment registers of the SMC controller provide a way to configure
> the mapping windows of the chips on the AHB bus. The settings are
> required to be correct when the controller operates in Command mode,
> which is the case for DMAs and the LPC mapping.
> 
> This tries to set the segment registers of each chip depending on the
> size of the flash device and depending on the previous segment
> settings, in order to have a contiguous window across multiple chips.
> 
> Unfortunately, the AST2500 SPI controller has a bug and it is not
> possible to configure a full 128MB window for a chip of the same
> size. The window size needs to be restricted to 120MB. This issue only
> applies to CE0.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: Marek Vasut <marek.vasut@gmail.com>

-- 
Best regards,
Marek Vasut

Re: [PATCH 3/4] mtd: spi-nor: aspeed: configure chip window on AHB bus

[Cyrille Pitchen]

Le 20/04/2017 à 15:30, Marek Vasut a écrit :
> On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
>> The segment registers of the SMC controller provide a way to configure
>> the mapping windows of the chips on the AHB bus. The settings are
>> required to be correct when the controller operates in Command mode,
>> which is the case for DMAs and the LPC mapping.
>>
>> This tries to set the segment registers of each chip depending on the
>> size of the flash device and depending on the previous segment
>> settings, in order to have a contiguous window across multiple chips.
>>
>> Unfortunately, the AST2500 SPI controller has a bug and it is not
>> possible to configure a full 128MB window for a chip of the same
>> size. The window size needs to be restricted to 120MB. This issue only
>> applies to CE0.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Reviewed-by: Marek Vasut <marek.vasut@gmail.com>
> 

Applied to the spi-nor/next branch of l2-mtd

Thanks!

[PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

[Cédric Le Goater]

When reading flash contents, try to use the "command mode" if the AHB
window configured for the flash module is big enough. Else, just fall
back to the "user mode" to perform the read.

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

 Changes since initial version :

 - rebased on current patchset which removed DMA support

 drivers/mtd/spi-nor/aspeed-smc.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 60c020482946..43015aec4557 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -394,6 +394,31 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
 
 	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
 	aspeed_smc_stop_user(nor);
+	return 0;
+}
+
+static ssize_t aspeed_smc_read(struct spi_nor *nor, loff_t from, size_t len,
+			       u_char *read_buf)
+{
+	struct aspeed_smc_chip *chip = nor->priv;
+
+	/*
+	 * The AHB window configured for the chip is too small for the
+	 * read offset. Use the "User mode" of the controller to
+	 * perform the read.
+	 */
+	if (from >= chip->ahb_window_size) {
+		aspeed_smc_read_user(nor, from, len, read_buf);
+		goto out;
+	}
+
+	/*
+	 * Use the "Command mode" to do a direct read from the AHB
+	 * window configured for the chip. This should be the default.
+	 */
+	memcpy_fromio(read_buf, chip->ahb_base + from, len);
+
+out:
 	return len;
 }
 
@@ -817,7 +842,7 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
 		nor->dev = dev;
 		nor->priv = chip;
 		spi_nor_set_flash_node(nor, child);
-		nor->read = aspeed_smc_read_user;
+		nor->read = aspeed_smc_read;
 		nor->write = aspeed_smc_write_user;
 		nor->read_reg = aspeed_smc_read_reg;
 		nor->write_reg = aspeed_smc_write_reg;
-- 
2.7.4

Re: [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

[Marek Vasut]

On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
> When reading flash contents, try to use the "command mode" if the AHB
> window configured for the flash module is big enough. Else, just fall
> back to the "user mode" to perform the read.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  Changes since initial version :
> 
>  - rebased on current patchset which removed DMA support
> 
>  drivers/mtd/spi-nor/aspeed-smc.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index 60c020482946..43015aec4557 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -394,6 +394,31 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>  
>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>  	aspeed_smc_stop_user(nor);
> +	return 0;
> +}
> +
> +static ssize_t aspeed_smc_read(struct spi_nor *nor, loff_t from, size_t len,
> +			       u_char *read_buf)
> +{
> +	struct aspeed_smc_chip *chip = nor->priv;
> +
> +	/*
> +	 * The AHB window configured for the chip is too small for the
> +	 * read offset. Use the "User mode" of the controller to
> +	 * perform the read.
> +	 */
> +	if (from >= chip->ahb_window_size) {
> +		aspeed_smc_read_user(nor, from, len, read_buf);
> +		goto out;

What about turning this into dumb if () {} else {} and dropping the goto ?

> +	}
> +
> +	/*
> +	 * Use the "Command mode" to do a direct read from the AHB
> +	 * window configured for the chip. This should be the default.
> +	 */
> +	memcpy_fromio(read_buf, chip->ahb_base + from, len);
> +
> +out:
>  	return len;
>  }
>  
> @@ -817,7 +842,7 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>  		nor->dev = dev;
>  		nor->priv = chip;
>  		spi_nor_set_flash_node(nor, child);
> -		nor->read = aspeed_smc_read_user;
> +		nor->read = aspeed_smc_read;
>  		nor->write = aspeed_smc_write_user;
>  		nor->read_reg = aspeed_smc_read_reg;
>  		nor->write_reg = aspeed_smc_write_reg;
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

[Richard Weinberger]

Am 20.04.2017 um 15:31 schrieb Marek Vasut:
> On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
>> When reading flash contents, try to use the "command mode" if the AHB
>> window configured for the flash module is big enough. Else, just fall
>> back to the "user mode" to perform the read.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  Changes since initial version :
>>
>>  - rebased on current patchset which removed DMA support
>>
>>  drivers/mtd/spi-nor/aspeed-smc.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>> index 60c020482946..43015aec4557 100644
>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>> @@ -394,6 +394,31 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>  
>>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>  	aspeed_smc_stop_user(nor);
>> +	return 0;
>> +}
>> +
>> +static ssize_t aspeed_smc_read(struct spi_nor *nor, loff_t from, size_t len,
>> +			       u_char *read_buf)
>> +{
>> +	struct aspeed_smc_chip *chip = nor->priv;
>> +
>> +	/*
>> +	 * The AHB window configured for the chip is too small for the
>> +	 * read offset. Use the "User mode" of the controller to
>> +	 * perform the read.
>> +	 */
>> +	if (from >= chip->ahb_window_size) {
>> +		aspeed_smc_read_user(nor, from, len, read_buf);
>> +		goto out;
> 
> What about turning this into dumb if () {} else {} and dropping the goto ?

This is a matter of taste...

Thanks,
//richard

Re: [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

[Cédric Le Goater]

On 04/20/2017 03:31 PM, Marek Vasut wrote:
> On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
>> When reading flash contents, try to use the "command mode" if the AHB
>> window configured for the flash module is big enough. Else, just fall
>> back to the "user mode" to perform the read.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  Changes since initial version :
>>
>>  - rebased on current patchset which removed DMA support
>>
>>  drivers/mtd/spi-nor/aspeed-smc.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>> index 60c020482946..43015aec4557 100644
>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>> @@ -394,6 +394,31 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>  
>>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>  	aspeed_smc_stop_user(nor);
>> +	return 0;
>> +}
>> +
>> +static ssize_t aspeed_smc_read(struct spi_nor *nor, loff_t from, size_t len,
>> +			       u_char *read_buf)
>> +{
>> +	struct aspeed_smc_chip *chip = nor->priv;
>> +
>> +	/*
>> +	 * The AHB window configured for the chip is too small for the
>> +	 * read offset. Use the "User mode" of the controller to
>> +	 * perform the read.
>> +	 */
>> +	if (from >= chip->ahb_window_size) {
>> +		aspeed_smc_read_user(nor, from, len, read_buf);
>> +		goto out;
> 
> What about turning this into dumb if () {} else {} and dropping the goto ?

yes, well, this is because I have other patches adding DMAs :

	https://github.com/legoater/linux/commit/6880924dc2cf7a47c446a708df528b72d9bf9134#diff-e466368d609006970aab53a7b840221fR575

but as of today, the benefits in speed are to be proven and the current 
implementation does not support vmalloc'ed buffers. So I can drop the 
goto without too much regrets (if you insist :)

Cheers,

C.  
 
>> +	}
>> +
>> +	/*
>> +	 * Use the "Command mode" to do a direct read from the AHB
>> +	 * window configured for the chip. This should be the default.
>> +	 */
>> +	memcpy_fromio(read_buf, chip->ahb_base + from, len);
>> +
>> +out:
>>  	return len;
>>  }
>>  
>> @@ -817,7 +842,7 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>>  		nor->dev = dev;
>>  		nor->priv = chip;
>>  		spi_nor_set_flash_node(nor, child);
>> -		nor->read = aspeed_smc_read_user;
>> +		nor->read = aspeed_smc_read;
>>  		nor->write = aspeed_smc_write_user;
>>  		nor->read_reg = aspeed_smc_read_reg;
>>  		nor->write_reg = aspeed_smc_write_reg;
>>
> 
> 

Re: [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

[Marek Vasut]

On 04/20/2017 03:53 PM, Cédric Le Goater wrote:
> On 04/20/2017 03:31 PM, Marek Vasut wrote:
>> On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
>>> When reading flash contents, try to use the "command mode" if the AHB
>>> window configured for the flash module is big enough. Else, just fall
>>> back to the "user mode" to perform the read.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>
>>>  Changes since initial version :
>>>
>>>  - rebased on current patchset which removed DMA support
>>>
>>>  drivers/mtd/spi-nor/aspeed-smc.c | 27 ++++++++++++++++++++++++++-
>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>> index 60c020482946..43015aec4557 100644
>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>> @@ -394,6 +394,31 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>>  
>>>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>>  	aspeed_smc_stop_user(nor);
>>> +	return 0;
>>> +}
>>> +
>>> +static ssize_t aspeed_smc_read(struct spi_nor *nor, loff_t from, size_t len,
>>> +			       u_char *read_buf)
>>> +{
>>> +	struct aspeed_smc_chip *chip = nor->priv;
>>> +
>>> +	/*
>>> +	 * The AHB window configured for the chip is too small for the
>>> +	 * read offset. Use the "User mode" of the controller to
>>> +	 * perform the read.
>>> +	 */
>>> +	if (from >= chip->ahb_window_size) {
>>> +		aspeed_smc_read_user(nor, from, len, read_buf);
>>> +		goto out;
>>
>> What about turning this into dumb if () {} else {} and dropping the goto ?
> 
> yes, well, this is because I have other patches adding DMAs :
> 
> 	https://github.com/legoater/linux/commit/6880924dc2cf7a47c446a708df528b72d9bf9134#diff-e466368d609006970aab53a7b840221fR575
> 
> but as of today, the benefits in speed are to be proven and the current 
> implementation does not support vmalloc'ed buffers. So I can drop the 
> goto without too much regrets (if you insist :)

Nah, if you have further plans with this code, I don't mind either way.

-- 
Best regards,
Marek Vasut

Re: [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

[Cédric Le Goater]

On 04/20/2017 03:58 PM, Marek Vasut wrote:
> On 04/20/2017 03:53 PM, Cédric Le Goater wrote:
>> On 04/20/2017 03:31 PM, Marek Vasut wrote:
>>> On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
>>>> When reading flash contents, try to use the "command mode" if the AHB
>>>> window configured for the flash module is big enough. Else, just fall
>>>> back to the "user mode" to perform the read.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>
>>>>  Changes since initial version :
>>>>
>>>>  - rebased on current patchset which removed DMA support
>>>>
>>>>  drivers/mtd/spi-nor/aspeed-smc.c | 27 ++++++++++++++++++++++++++-
>>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>>> index 60c020482946..43015aec4557 100644
>>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>>> @@ -394,6 +394,31 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>>>  
>>>>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>>>  	aspeed_smc_stop_user(nor);
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static ssize_t aspeed_smc_read(struct spi_nor *nor, loff_t from, size_t len,
>>>> +			       u_char *read_buf)
>>>> +{
>>>> +	struct aspeed_smc_chip *chip = nor->priv;
>>>> +
>>>> +	/*
>>>> +	 * The AHB window configured for the chip is too small for the
>>>> +	 * read offset. Use the "User mode" of the controller to
>>>> +	 * perform the read.
>>>> +	 */
>>>> +	if (from >= chip->ahb_window_size) {
>>>> +		aspeed_smc_read_user(nor, from, len, read_buf);
>>>> +		goto out;
>>>
>>> What about turning this into dumb if () {} else {} and dropping the goto ?
>>
>> yes, well, this is because I have other patches adding DMAs :
>>
>> 	https://github.com/legoater/linux/commit/6880924dc2cf7a47c446a708df528b72d9bf9134#diff-e466368d609006970aab53a7b840221fR575
>>
>> but as of today, the benefits in speed are to be proven and the current 
>> implementation does not support vmalloc'ed buffers. So I can drop the 
>> goto without too much regrets (if you insist :)
> 
> Nah, if you have further plans with this code, I don't mind either way.
> 

OK let's keep that way.

Do you think we can merge these four patches in the next window ? 

Thanks,

C.

Re: [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

[Cédric Le Goater]

[ fixing Cyrille email ] 

On 04/20/2017 03:58 PM, Marek Vasut wrote:
> On 04/20/2017 03:53 PM, Cédric Le Goater wrote:
>> On 04/20/2017 03:31 PM, Marek Vasut wrote:
>>> On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
>>>> When reading flash contents, try to use the "command mode" if the AHB
>>>> window configured for the flash module is big enough. Else, just fall
>>>> back to the "user mode" to perform the read.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>
>>>>  Changes since initial version :
>>>>
>>>>  - rebased on current patchset which removed DMA support
>>>>
>>>>  drivers/mtd/spi-nor/aspeed-smc.c | 27 ++++++++++++++++++++++++++-
>>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>>> index 60c020482946..43015aec4557 100644
>>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>>> @@ -394,6 +394,31 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>>>  
>>>>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>>>  	aspeed_smc_stop_user(nor);
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static ssize_t aspeed_smc_read(struct spi_nor *nor, loff_t from, size_t len,
>>>> +			       u_char *read_buf)
>>>> +{
>>>> +	struct aspeed_smc_chip *chip = nor->priv;
>>>> +
>>>> +	/*
>>>> +	 * The AHB window configured for the chip is too small for the
>>>> +	 * read offset. Use the "User mode" of the controller to
>>>> +	 * perform the read.
>>>> +	 */
>>>> +	if (from >= chip->ahb_window_size) {
>>>> +		aspeed_smc_read_user(nor, from, len, read_buf);
>>>> +		goto out;
>>>
>>> What about turning this into dumb if () {} else {} and dropping the goto ?
>>
>> yes, well, this is because I have other patches adding DMAs :
>>
>> 	https://github.com/legoater/linux/commit/6880924dc2cf7a47c446a708df528b72d9bf9134#diff-e466368d609006970aab53a7b840221fR575
>>
>> but as of today, the benefits in speed are to be proven and the current 
>> implementation does not support vmalloc'ed buffers. So I can drop the 
>> goto without too much regrets (if you insist :)
> 
> Nah, if you have further plans with this code, I don't mind either way.
> 

OK let's keep that way.

Do you think we can merge these four patches in the next window ? 

Thanks,

C.

Re: [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

[Cédric Le Goater]

On 06/20/2017 03:42 PM, Cyrille Pitchen wrote:
> Hi Cédric,
> 
> Le 20/06/2017 à 11:07, Cédric Le Goater a écrit :
>> [ fixing Cyrille email ] 
>>
>> On 04/20/2017 03:58 PM, Marek Vasut wrote:
>>> On 04/20/2017 03:53 PM, Cédric Le Goater wrote:
>>>> On 04/20/2017 03:31 PM, Marek Vasut wrote:
>>>>> On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
>>>>>> When reading flash contents, try to use the "command mode" if the AHB
>>>>>> window configured for the flash module is big enough. Else, just fall
>>>>>> back to the "user mode" to perform the read.
>>>>>>
>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>> ---
>>>>>>
>>>>>>  Changes since initial version :
>>>>>>
>>>>>>  - rebased on current patchset which removed DMA support
>>>>>>
>>>>>>  drivers/mtd/spi-nor/aspeed-smc.c | 27 ++++++++++++++++++++++++++-
>>>>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>>>>> index 60c020482946..43015aec4557 100644
>>>>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>>>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>>>>> @@ -394,6 +394,31 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>>>>>  
>>>>>>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>>>>>  	aspeed_smc_stop_user(nor);
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t aspeed_smc_read(struct spi_nor *nor, loff_t from, size_t len,
>>>>>> +			       u_char *read_buf)
>>>>>> +{
>>>>>> +	struct aspeed_smc_chip *chip = nor->priv;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * The AHB window configured for the chip is too small for the
>>>>>> +	 * read offset. Use the "User mode" of the controller to
>>>>>> +	 * perform the read.
>>>>>> +	 */
>>>>>> +	if (from >= chip->ahb_window_size) {
>>>>>> +		aspeed_smc_read_user(nor, from, len, read_buf);
>>>>>> +		goto out;
>>>>>
>>>>> What about turning this into dumb if () {} else {} and dropping the goto ?
>>>>
>>>> yes, well, this is because I have other patches adding DMAs :
>>>>
>>>> 	https://github.com/legoater/linux/commit/6880924dc2cf7a47c446a708df528b72d9bf9134#diff-e466368d609006970aab53a7b840221fR575
>>>>
>>>> but as of today, the benefits in speed are to be proven and the current 
>>>> implementation does not support vmalloc'ed buffers. So I can drop the 
>>>> goto without too much regrets (if you insist :)
>>>
>>> Nah, if you have further plans with this code, I don't mind either way.
>>>
>>
>> OK let's keep that way.
>>
>> Do you think we can merge these four patches in the next window ? 
>>
> 
> I've planned to merge them in the spi-nor/next branch of l2-mtd within
> the next days if everything is OK.

That's the git repo : git://github.com/spi-nor/linux.git  ? 
 
> Last Friday I saw on patchwork that your patches were still waiting to
> be merged. I've just forgotten them sorry and last week-end I was not
> available to catch up.
> 
> I add a quick overview of the series, everything looks good. patch 2
> won't apply as is since flash_read no longer exists in 'struct spi_nor'
> so I will try to adapt your patch myself if I can.

Ah. I work on mainline generally, so I missed it.

> Otherwise I will ask you help to rebase it but I guess the modification
> will be quite straight forward.

Sure. Please don't hesitate to ping me. I will take a quick look
anyway.

Thanks,

C.

Re: [PATCH 4/4] mtd: spi-nor: aspeed: use command mode for reads

[Cédric Le Goater]

[ fixing Cyrille email. your "From" is still @atmel.com. Am I doing
  something wrong ? ]

On 06/20/2017 03:42 PM, Cyrille Pitchen wrote:
> Hi Cédric,
> 
> Le 20/06/2017 à 11:07, Cédric Le Goater a écrit :
>> [ fixing Cyrille email ] 
>>
>> On 04/20/2017 03:58 PM, Marek Vasut wrote:
>>> On 04/20/2017 03:53 PM, Cédric Le Goater wrote:
>>>> On 04/20/2017 03:31 PM, Marek Vasut wrote:
>>>>> On 04/20/2017 01:56 PM, Cédric Le Goater wrote:
>>>>>> When reading flash contents, try to use the "command mode" if the AHB
>>>>>> window configured for the flash module is big enough. Else, just fall
>>>>>> back to the "user mode" to perform the read.
>>>>>>
>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>> ---
>>>>>>
>>>>>>  Changes since initial version :
>>>>>>
>>>>>>  - rebased on current patchset which removed DMA support
>>>>>>
>>>>>>  drivers/mtd/spi-nor/aspeed-smc.c | 27 ++++++++++++++++++++++++++-
>>>>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>>>>> index 60c020482946..43015aec4557 100644
>>>>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>>>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>>>>> @@ -394,6 +394,31 @@ static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from,
>>>>>>  
>>>>>>  	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
>>>>>>  	aspeed_smc_stop_user(nor);
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t aspeed_smc_read(struct spi_nor *nor, loff_t from, size_t len,
>>>>>> +			       u_char *read_buf)
>>>>>> +{
>>>>>> +	struct aspeed_smc_chip *chip = nor->priv;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * The AHB window configured for the chip is too small for the
>>>>>> +	 * read offset. Use the "User mode" of the controller to
>>>>>> +	 * perform the read.
>>>>>> +	 */
>>>>>> +	if (from >= chip->ahb_window_size) {
>>>>>> +		aspeed_smc_read_user(nor, from, len, read_buf);
>>>>>> +		goto out;
>>>>>
>>>>> What about turning this into dumb if () {} else {} and dropping the goto ?
>>>>
>>>> yes, well, this is because I have other patches adding DMAs :
>>>>
>>>> 	https://github.com/legoater/linux/commit/6880924dc2cf7a47c446a708df528b72d9bf9134#diff-e466368d609006970aab53a7b840221fR575
>>>>
>>>> but as of today, the benefits in speed are to be proven and the current 
>>>> implementation does not support vmalloc'ed buffers. So I can drop the 
>>>> goto without too much regrets (if you insist :)
>>>
>>> Nah, if you have further plans with this code, I don't mind either way.
>>>
>>
>> OK let's keep that way.
>>
>> Do you think we can merge these four patches in the next window ? 
>>
> 
> I've planned to merge them in the spi-nor/next branch of l2-mtd within
> the next days if everything is OK.

That's the git repo : git://github.com/spi-nor/linux.git  ? 
 
> Last Friday I saw on patchwork that your patches were still waiting to
> be merged. I've just forgotten them sorry and last week-end I was not
> available to catch up.
> 
> I add a quick overview of the series, everything looks good. patch 2
> won't apply as is since flash_read no longer exists in 'struct spi_nor'
> so I will try to adapt your patch myself if I can.

Ah. I work on mainline generally, so I missed it.

> Otherwise I will ask you help to rebase it but I guess the modification
> will be quite straight forward.

Sure. Please don't hesitate to ping me. I will take a quick look
anyway.

Thanks,

C.