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

Submitted by Cédric Le Goater on April 20, 2017, 11:56 a.m.

Details

Message ID 1492689397-4704-5-git-send-email-clg@kaod.org
State New
Headers show

Commit Message

Cédric Le Goater April 20, 2017, 11:56 a.m.
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(-)

Comments

Marek Vasut April 20, 2017, 1:31 p.m.
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;
>
Richard Weinberger April 20, 2017, 1:33 p.m.
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
Cédric Le Goater April 20, 2017, 1:53 p.m.
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;
>>
> 
>
Marek Vasut April 20, 2017, 1:58 p.m.
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.

Patch hide | download patch | download mbox

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;