[2/2] mtd: spi-nor: aspeed: limit the maximum SPI frequency

Message ID 20180608130000.22627-3-clg@kaod.org
State Superseded, archived
Headers show
Series
  • mtd: spi-nor: aspeed: introduce a "spi-max-frequency" property
Related show

Commit Message

Cédric Le Goater June 8, 2018, 1 p.m.
The optimize read algo can choose a 100MHz SPI frequency which might
be a bit too high for dual output IO on some chips, for the W25Q256 on
palmetto for instance. The MX66L1G45G on witherspoon should be fine
though. Also, the second chip of the FMC controller does not get any
optimize settings for reads. Only the first is configured by U-Boot.

To fix these two issues, we introduce a "spi-max-frequency" property
in the device tree which will be used to cap the optimize read
algorithm and we run the algo on the FMC controller chips as well.

By default, the frequency setting is 50MHz.

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

Comments

Andrew Jeffery June 18, 2018, 3:38 a.m. | #1
On Fri, 8 Jun 2018, at 22:30, Cédric Le Goater wrote:
> The optimize read algo can choose a 100MHz SPI frequency which might
> be a bit too high for dual output IO on some chips, for the W25Q256 on
> palmetto for instance. The MX66L1G45G on witherspoon should be fine
> though. Also, the second chip of the FMC controller does not get any
> optimize settings for reads. Only the first is configured by U-Boot.
> 
> To fix these two issues, we introduce a "spi-max-frequency" property
> in the device tree which will be used to cap the optimize read
> algorithm and we run the algo on the FMC controller chips as well.

Might be a silly question, but why didn't you add support for optimized reads for the AST2400 FMC given we can limit the speed through the devicetree property?

> 
> By default, the frequency setting is 50MHz.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  drivers/mtd/spi-nor/aspeed-smc.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index a301895d1f06..e4a0123b12b3 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -83,6 +83,7 @@ static const struct aspeed_smc_info fmc_2500_info = {
>  	.ctl0 = 0x10,
>  	.timing = 0x94,
>  	.set_4b = aspeed_smc_chip_set_4b,
> +	.optimize_read = aspeed_smc_optimize_read,

i.e. here you add the support for the 2500 FMC, but the 2400 FMC info struct is unchanged.

>  };
>  
>  static const struct aspeed_smc_info spi_2500_info = {
> @@ -114,6 +115,7 @@ struct aspeed_smc_chip {
>  	u32 ctl_val[smc_max];			/* control settings */
>  	enum aspeed_smc_flash_type type;	/* what type of flash */
>  	struct spi_nor nor;
> +	u32 clk_rate;
>  };
>  
>  struct aspeed_smc_controller {
> @@ -130,6 +132,8 @@ struct aspeed_smc_controller {
>  	struct aspeed_smc_chip *chips[0];	/* pointers to attached chips */
>  };
>  
> +#define ASPEED_SPI_DEFAULT_FREQ		50000000
> +
>  /*
>   * SPI Flash Configuration Register (AST2500 SPI)
>   *     or
> @@ -993,11 +997,10 @@ static int aspeed_smc_chip_setup_finish(struct 
> aspeed_smc_chip *chip)
>  	dev_info(controller->dev, "read control register: %08x\n",
>  		chip->ctl_val[smc_read]);
>  
> -	/*
> -	 * TODO: get max freq from chip
> -	 */
> +
>  	if (optimize_read && info->optimize_read)
> -		info->optimize_read(chip, 104000000);
> +		info->optimize_read(chip, chip->clk_rate);
> +
>  	return 0;
>  }
>  
> @@ -1051,6 +1054,13 @@ static int aspeed_smc_setup_flash(struct 
> aspeed_smc_controller *controller,
>  			break;
>  		}
>  
> +		if (of_property_read_u32(child, "spi-max-frequency",
> +					 &chip->clk_rate)) {
> +			chip->clk_rate = ASPEED_SPI_DEFAULT_FREQ;
> +		}
> +		dev_info(dev, "Using %d MHz SPI frequency\n",
> +			 chip->clk_rate / 1000000);
> +
>  		chip->controller = controller;
>  		chip->ctl = controller->regs + info->ctl0 + cs * 4;
>  		chip->cs = cs;
> -- 
> 2.13.6
>
Cédric Le Goater June 19, 2018, 5:10 a.m. | #2
On 06/18/2018 05:38 AM, Andrew Jeffery wrote:
> On Fri, 8 Jun 2018, at 22:30, Cédric Le Goater wrote:
>> The optimize read algo can choose a 100MHz SPI frequency which might
>> be a bit too high for dual output IO on some chips, for the W25Q256 on
>> palmetto for instance. The MX66L1G45G on witherspoon should be fine
>> though. Also, the second chip of the FMC controller does not get any
>> optimize settings for reads. Only the first is configured by U-Boot.
>>
>> To fix these two issues, we introduce a "spi-max-frequency" property
>> in the device tree which will be used to cap the optimize read
>> algorithm and we run the algo on the FMC controller chips as well.
> 
> Might be a silly question, but why didn't you add support for optimized 
> reads for the AST2400 FMC given we can limit the speed through the 
> devicetree property ? >
>>
>> By default, the frequency setting is 50MHz.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  drivers/mtd/spi-nor/aspeed-smc.c | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>> index a301895d1f06..e4a0123b12b3 100644
>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>> @@ -83,6 +83,7 @@ static const struct aspeed_smc_info fmc_2500_info = {
>>  	.ctl0 = 0x10,
>>  	.timing = 0x94,
>>  	.set_4b = aspeed_smc_chip_set_4b,
>> +	.optimize_read = aspeed_smc_optimize_read,
> 
> i.e. here you add the support for the 2500 FMC, but the 2400 FMC info struct is unchanged.

Ah I understand now your remark. It is missing indeed. I didn't notice 
because I was targeting the alt fmc chip on the witherspoon and too 
high frequencies on the PNOR. I will check how it behaves on the new 
4.17 tree.

Thanks !

C. 
  
> 
>>  };
>>  
>>  static const struct aspeed_smc_info spi_2500_info = {
>> @@ -114,6 +115,7 @@ struct aspeed_smc_chip {
>>  	u32 ctl_val[smc_max];			/* control settings */
>>  	enum aspeed_smc_flash_type type;	/* what type of flash */
>>  	struct spi_nor nor;
>> +	u32 clk_rate;
>>  };
>>  
>>  struct aspeed_smc_controller {
>> @@ -130,6 +132,8 @@ struct aspeed_smc_controller {
>>  	struct aspeed_smc_chip *chips[0];	/* pointers to attached chips */
>>  };
>>  
>> +#define ASPEED_SPI_DEFAULT_FREQ		50000000
>> +
>>  /*
>>   * SPI Flash Configuration Register (AST2500 SPI)
>>   *     or
>> @@ -993,11 +997,10 @@ static int aspeed_smc_chip_setup_finish(struct 
>> aspeed_smc_chip *chip)
>>  	dev_info(controller->dev, "read control register: %08x\n",
>>  		chip->ctl_val[smc_read]);
>>  
>> -	/*
>> -	 * TODO: get max freq from chip
>> -	 */
>> +
>>  	if (optimize_read && info->optimize_read)
>> -		info->optimize_read(chip, 104000000);
>> +		info->optimize_read(chip, chip->clk_rate);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -1051,6 +1054,13 @@ static int aspeed_smc_setup_flash(struct 
>> aspeed_smc_controller *controller,
>>  			break;
>>  		}
>>  
>> +		if (of_property_read_u32(child, "spi-max-frequency",
>> +					 &chip->clk_rate)) {
>> +			chip->clk_rate = ASPEED_SPI_DEFAULT_FREQ;
>> +		}
>> +		dev_info(dev, "Using %d MHz SPI frequency\n",
>> +			 chip->clk_rate / 1000000);
>> +
>>  		chip->controller = controller;
>>  		chip->ctl = controller->regs + info->ctl0 + cs * 4;
>>  		chip->cs = cs;
>> -- 
>> 2.13.6
>>

Patch

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index a301895d1f06..e4a0123b12b3 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -83,6 +83,7 @@  static const struct aspeed_smc_info fmc_2500_info = {
 	.ctl0 = 0x10,
 	.timing = 0x94,
 	.set_4b = aspeed_smc_chip_set_4b,
+	.optimize_read = aspeed_smc_optimize_read,
 };
 
 static const struct aspeed_smc_info spi_2500_info = {
@@ -114,6 +115,7 @@  struct aspeed_smc_chip {
 	u32 ctl_val[smc_max];			/* control settings */
 	enum aspeed_smc_flash_type type;	/* what type of flash */
 	struct spi_nor nor;
+	u32 clk_rate;
 };
 
 struct aspeed_smc_controller {
@@ -130,6 +132,8 @@  struct aspeed_smc_controller {
 	struct aspeed_smc_chip *chips[0];	/* pointers to attached chips */
 };
 
+#define ASPEED_SPI_DEFAULT_FREQ		50000000
+
 /*
  * SPI Flash Configuration Register (AST2500 SPI)
  *     or
@@ -993,11 +997,10 @@  static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 	dev_info(controller->dev, "read control register: %08x\n",
 		chip->ctl_val[smc_read]);
 
-	/*
-	 * TODO: get max freq from chip
-	 */
+
 	if (optimize_read && info->optimize_read)
-		info->optimize_read(chip, 104000000);
+		info->optimize_read(chip, chip->clk_rate);
+
 	return 0;
 }
 
@@ -1051,6 +1054,13 @@  static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
 			break;
 		}
 
+		if (of_property_read_u32(child, "spi-max-frequency",
+					 &chip->clk_rate)) {
+			chip->clk_rate = ASPEED_SPI_DEFAULT_FREQ;
+		}
+		dev_info(dev, "Using %d MHz SPI frequency\n",
+			 chip->clk_rate / 1000000);
+
 		chip->controller = controller;
 		chip->ctl = controller->regs + info->ctl0 + cs * 4;
 		chip->cs = cs;