[PATCH 0/1] spi: aspeed: Update SPI clock frequency configuration

[PATCH 0/1] spi: aspeed: Update SPI clock frequency configuration

[Chin-Ting Kuo]

This patch series aims to update SPI clock frequency
configuration rule and add SPI clock frequency
calculation method for different ASPEED platforms.

This patch is based on [1] and the patch series [2].
[1]: https://github.com/legoater/linux/commits/openbmc-5.15
[2]: https://patchwork.kernel.org/project/spi-devel-general/list/?series=626283


Chin-Ting Kuo (1):
  spi: aspeed: Update SPI clock frequency configuration

 drivers/spi/spi-aspeed-smc.c | 166 +++++++++++++++++++++++++++++++----
 1 file changed, 149 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH 1/1] spi: aspeed: Update SPI clock frequency configuration

[Chin-Ting Kuo]

Instead of using the slowest one, the maximum clock
frequency configured in the device tree should be kept
when no timing calibration process is executed.
Besides, an extra callback function is added in order
to calculate clock frequency configuration for
different ASPEED platforms.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 drivers/spi/spi-aspeed-smc.c | 166 +++++++++++++++++++++++++++++++----
 1 file changed, 149 insertions(+), 17 deletions(-)

diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
index 227797e13997..728163d0045d 100644
--- a/drivers/spi/spi-aspeed-smc.c
+++ b/drivers/spi/spi-aspeed-smc.c
@@ -3,7 +3,7 @@
  * ASPEED FMC/SPI Memory Controller Driver
  *
  * Copyright (c) 2015-2022, IBM Corporation.
- * Copyright (c) 2020, ASPEED Corporation.
+ * Copyright (c) 2020-2022, ASPEED Corporation.
  */
 
 #include <linux/clk.h>
@@ -84,6 +84,7 @@ struct aspeed_spi_data {
 	u32 (*segment_reg)(struct aspeed_spi *aspi, u32 start, u32 end);
 	int (*calibrate)(struct aspeed_spi_chip *chip, u32 hdiv,
 			 const u8 *golden_buf, u8 *test_buf);
+	u32 (*clk_config)(struct aspeed_spi_chip *chip, u32 max_hz);
 };
 
 #define ASPEED_SPI_MAX_NUM_CS	5
@@ -959,7 +960,10 @@ static int aspeed_spi_do_calibration(struct aspeed_spi_chip *chip)
 	u32 ctl_val;
 	u8 *golden_buf = NULL;
 	u8 *test_buf = NULL;
-	int i, rc, best_div = -1;
+	int i, rc;
+	u32 best_freq = 0;
+	u32 freq;
+	u32 clk_conf;
 
 	dev_dbg(aspi->dev, "calculate timing compensation - AHB freq: %d MHz",
 		ahb_freq / 1000000);
@@ -980,7 +984,7 @@ static int aspeed_spi_do_calibration(struct aspeed_spi_chip *chip)
 	memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
 	if (!aspeed_spi_check_calib_data(golden_buf, CALIBRATE_BUF_SIZE)) {
 		dev_info(aspi->dev, "Calibration area too uniform, using low speed");
-		goto no_calib;
+		goto end_calib;
 	}
 
 #if defined(VERBOSE_DEBUG)
@@ -990,7 +994,7 @@ static int aspeed_spi_do_calibration(struct aspeed_spi_chip *chip)
 
 	/* Now we iterate the HCLK dividers until we find our breaking point */
 	for (i = ARRAY_SIZE(aspeed_spi_hclk_divs); i > data->hdiv_max - 1; i--) {
-		u32 tv, freq;
+		u32 tv;
 
 		freq = ahb_freq / i;
 		if (freq > max_freq)
@@ -1002,27 +1006,149 @@ static int aspeed_spi_do_calibration(struct aspeed_spi_chip *chip)
 		dev_dbg(aspi->dev, "Trying HCLK/%d [%08x] ...", i, tv);
 		rc = data->calibrate(chip, i, golden_buf, test_buf);
 		if (rc == 0)
-			best_div = i;
+			best_freq = freq;
 	}
 
 	/* Nothing found ? */
-	if (best_div < 0) {
-		dev_warn(aspi->dev, "No good frequency, using dumb slow");
-	} else {
-		dev_dbg(aspi->dev, "Found good read timings at HCLK/%d", best_div);
+	if (best_freq == 0)
+		dev_warn(aspi->dev, "Use the default timing setting");
+	else
+		dev_dbg(aspi->dev, "Found good read timings at HCLK/%d", i);
 
-		/* Record the freq */
-		for (i = 0; i < ASPEED_SPI_MAX; i++)
-			chip->ctl_val[i] = (chip->ctl_val[i] & data->hclk_mask) |
-				ASPEED_SPI_HCLK_DIV(best_div);
-	}
 
-no_calib:
+end_calib:
+	if (best_freq == 0)
+		best_freq = max_freq;
+
+	clk_conf = data->clk_config(chip, best_freq);
+	/* Record the freq */
+	for (i = 0; i < ASPEED_SPI_MAX; i++)
+		chip->ctl_val[i] = (chip->ctl_val[i] & data->hclk_mask) | clk_conf;
+
 	writel(chip->ctl_val[ASPEED_SPI_READ], chip->ctl);
 	kfree(test_buf);
 	return 0;
 }
 
+/* HCLK/1 ..	HCLK/16 */
+static const u32 aspeed_spi_hclk_masks[] = {
+	15, 7, 14, 6, 13, 5, 12, 4,
+	11, 3, 10, 2, 9,  1, 8,  0
+};
+
+static u32 aspeed_spi_ast2400_clk_config(struct aspeed_spi_chip *chip,
+					 u32 max_hz)
+{
+	struct aspeed_spi *aspi = chip->aspi;
+	u32 ahb_freq = aspi->clk_freq;
+	u32 hclk_div = 0; /* default value */
+	u32 i;
+	bool found = false;
+
+	/* FMC/SPIR10[11:8] */
+	for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
+		if (ahb_freq / (i + 1) <= max_hz) {
+			found = true;
+			break;
+		}
+	}
+
+	if (found)
+		hclk_div = aspeed_spi_hclk_masks[i] << 8;
+
+	dev_dbg(aspi->dev, "found: %s, hclk: %d, max_clk: %d\n",
+		found ? "yes" : "no", ahb_freq, max_hz);
+
+	if (found) {
+		dev_dbg(aspi->dev, "h_div: %d (mask %x)\n",
+			i + 1, aspeed_spi_hclk_masks[i]);
+	}
+
+	return hclk_div;
+}
+
+static u32 aspeed_spi_ast2500_clk_config(struct aspeed_spi_chip *chip,
+					 u32 max_hz)
+{
+	struct aspeed_spi *aspi = chip->aspi;
+	u32 ahb_freq = aspi->clk_freq;
+	u32 hclk_div = 0; /* default value */
+	u32 i;
+	bool found = false;
+
+	/* FMC/SPIR10[11:8] */
+	for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
+		if (ahb_freq / (i + 1) <= max_hz) {
+			found = true;
+			break;
+		}
+	}
+
+	if (found) {
+		hclk_div = aspeed_spi_hclk_masks[i] << 8;
+	} else {
+		/* If FMC10[13] is set, an extra div_4 can be introduced. */
+		for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
+			if (ahb_freq / ((i + 1) * 4) <= max_hz) {
+				found = true;
+				break;
+			}
+		}
+
+		if (found)
+			hclk_div = BIT(13) | (aspeed_spi_hclk_masks[i] << 8);
+	}
+
+	dev_dbg(aspi->dev, "found: %s, hclk: %d, max_clk: %d\n",
+		found ? "yes" : "no", ahb_freq, max_hz);
+
+	if (found) {
+		dev_dbg(aspi->dev, "h_div: %d (mask %x)\n",
+			i + 1, aspeed_spi_hclk_masks[i]);
+	}
+
+	return hclk_div;
+}
+
+static u32 aspeed_spi_ast2600_clk_config(struct aspeed_spi_chip *chip,
+					 u32 max_hz)
+{
+	struct aspeed_spi *aspi = chip->aspi;
+	u32 ahb_freq = aspi->clk_freq;
+	u32 hclk_div = 0x400; /* default value */
+	u32 i, j;
+	bool found = false;
+
+	/* FMC/SPIR10[27:24] */
+	for (j = 0; j < 0xf; j++) {
+		/* FMC/SPIR10[11:8] */
+		for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
+			if (i == 0 && j == 0)
+				continue;
+
+			if (ahb_freq / (i + 1 + (j * 16)) <= max_hz) {
+				found = true;
+				break;
+			}
+		}
+
+		if (found) {
+			hclk_div = ((j << 24) | aspeed_spi_hclk_masks[i] << 8);
+			break;
+		}
+	}
+
+	dev_dbg(aspi->dev, "found: %s, hclk: %d, max_clk: %d\n",
+		found ? "yes" : "no", ahb_freq, max_hz);
+
+	if (found) {
+		dev_dbg(aspi->dev, "base_clk: %d, h_div: %d (mask %x)\n",
+			j, i + 1, aspeed_spi_hclk_masks[i]);
+	}
+
+	return hclk_div;
+}
+
 #define TIMING_DELAY_DI		BIT(3)
 #define TIMING_DELAY_HCYCLE_MAX	5
 #define TIMING_REG_AST2600(chip)				\
@@ -1097,6 +1223,7 @@ static const struct aspeed_spi_data ast2400_fmc_data = {
 	.segment_start = aspeed_spi_segment_start,
 	.segment_end   = aspeed_spi_segment_end,
 	.segment_reg   = aspeed_spi_segment_reg,
+	.clk_config    = aspeed_spi_ast2400_clk_config,
 };
 
 static const struct aspeed_spi_data ast2400_spi_data = {
@@ -1109,6 +1236,7 @@ static const struct aspeed_spi_data ast2400_spi_data = {
 	.hdiv_max      = 1,
 	.calibrate     = aspeed_spi_calibrate,
 	/* No segment registers */
+	.clk_config    = aspeed_spi_ast2400_clk_config,
 };
 
 static const struct aspeed_spi_data ast2500_fmc_data = {
@@ -1117,12 +1245,13 @@ static const struct aspeed_spi_data ast2500_fmc_data = {
 	.we0	       = 16,
 	.ctl0	       = CE0_CTRL_REG,
 	.timing	       = CE0_TIMING_COMPENSATION_REG,
-	.hclk_mask     = 0xfffff0ff,
+	.hclk_mask     = 0xffffd0ff,
 	.hdiv_max      = 1,
 	.calibrate     = aspeed_spi_calibrate,
 	.segment_start = aspeed_spi_segment_start,
 	.segment_end   = aspeed_spi_segment_end,
 	.segment_reg   = aspeed_spi_segment_reg,
+	.clk_config    = aspeed_spi_ast2500_clk_config,
 };
 
 static const struct aspeed_spi_data ast2500_spi_data = {
@@ -1131,12 +1260,13 @@ static const struct aspeed_spi_data ast2500_spi_data = {
 	.we0	       = 16,
 	.ctl0	       = CE0_CTRL_REG,
 	.timing	       = CE0_TIMING_COMPENSATION_REG,
-	.hclk_mask     = 0xfffff0ff,
+	.hclk_mask     = 0xffffd0ff,
 	.hdiv_max      = 1,
 	.calibrate     = aspeed_spi_calibrate,
 	.segment_start = aspeed_spi_segment_start,
 	.segment_end   = aspeed_spi_segment_end,
 	.segment_reg   = aspeed_spi_segment_reg,
+	.clk_config    = aspeed_spi_ast2500_clk_config,
 };
 
 static const struct aspeed_spi_data ast2600_fmc_data = {
@@ -1152,6 +1282,7 @@ static const struct aspeed_spi_data ast2600_fmc_data = {
 	.segment_start = aspeed_spi_segment_ast2600_start,
 	.segment_end   = aspeed_spi_segment_ast2600_end,
 	.segment_reg   = aspeed_spi_segment_ast2600_reg,
+	.clk_config    = aspeed_spi_ast2600_clk_config,
 };
 
 static const struct aspeed_spi_data ast2600_spi_data = {
@@ -1167,6 +1298,7 @@ static const struct aspeed_spi_data ast2600_spi_data = {
 	.segment_start = aspeed_spi_segment_ast2600_start,
 	.segment_end   = aspeed_spi_segment_ast2600_end,
 	.segment_reg   = aspeed_spi_segment_ast2600_reg,
+	.clk_config    = aspeed_spi_ast2600_clk_config,
 };
 
 static const struct of_device_id aspeed_spi_matches[] = {
-- 
2.25.1

Re: [PATCH 1/1] spi: aspeed: Update SPI clock frequency configuration

[Cédric Le Goater]

Hello Chin-Ting,

On 4/14/22 12:28, Chin-Ting Kuo wrote:
> Instead of using the slowest one, the maximum clock
> frequency configured in the device tree should be kept
> when no timing calibration process is executed.
> Besides, an extra callback function is added in order
> to calculate clock frequency configuration for
> different ASPEED platforms.
> 
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>

I gave this patch a try on an AST2600 A3 EVB and an AST2500 EVB and it
behaved as expected. The default setting from the DT is chosen when the
flash contents are too uniform.


> ---
>   drivers/spi/spi-aspeed-smc.c | 166 +++++++++++++++++++++++++++++++----
>   1 file changed, 149 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/spi/spi-aspeed-smc.c b/drivers/spi/spi-aspeed-smc.c
> index 227797e13997..728163d0045d 100644
> --- a/drivers/spi/spi-aspeed-smc.c
> +++ b/drivers/spi/spi-aspeed-smc.c
> @@ -3,7 +3,7 @@
>    * ASPEED FMC/SPI Memory Controller Driver
>    *
>    * Copyright (c) 2015-2022, IBM Corporation.
> - * Copyright (c) 2020, ASPEED Corporation.
> + * Copyright (c) 2020-2022, ASPEED Corporation.
>    */
>   #include <linux/clk.h>
> @@ -84,6 +84,7 @@ struct aspeed_spi_data {
>   	u32 (*segment_reg)(struct aspeed_spi *aspi, u32 start, u32 end);
>   	int (*calibrate)(struct aspeed_spi_chip *chip, u32 hdiv,
>   			 const u8 *golden_buf, u8 *test_buf);
> +	u32 (*clk_config)(struct aspeed_spi_chip *chip, u32 max_hz);
>   };
>   
>   #define ASPEED_SPI_MAX_NUM_CS	5
> @@ -959,7 +960,10 @@ static int aspeed_spi_do_calibration(struct aspeed_spi_chip *chip)
>   	u32 ctl_val;
>   	u8 *golden_buf = NULL;
>   	u8 *test_buf = NULL;
> -	int i, rc, best_div = -1;
> +	int i, rc;
> +	u32 best_freq = 0;
> +	u32 freq;
> +	u32 clk_conf;
>   
>   	dev_dbg(aspi->dev, "calculate timing compensation - AHB freq: %d MHz",
>   		ahb_freq / 1000000);
> @@ -980,7 +984,7 @@ static int aspeed_spi_do_calibration(struct aspeed_spi_chip *chip)
>   	memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
>   	if (!aspeed_spi_check_calib_data(golden_buf, CALIBRATE_BUF_SIZE)) {
>   		dev_info(aspi->dev, "Calibration area too uniform, using low speed");

may be change dev_info() to "..., using default freq"

> -		goto no_calib;
> +		goto end_calib;
>   	}
>   
>   #if defined(VERBOSE_DEBUG)
> @@ -990,7 +994,7 @@ static int aspeed_spi_do_calibration(struct aspeed_spi_chip *chip)
>   
>   	/* Now we iterate the HCLK dividers until we find our breaking point */
>   	for (i = ARRAY_SIZE(aspeed_spi_hclk_divs); i > data->hdiv_max - 1; i--) {
> -		u32 tv, freq;
> +		u32 tv;
>   
>   		freq = ahb_freq / i;
>   		if (freq > max_freq)
> @@ -1002,27 +1006,149 @@ static int aspeed_spi_do_calibration(struct aspeed_spi_chip *chip)
>   		dev_dbg(aspi->dev, "Trying HCLK/%d [%08x] ...", i, tv);
>   		rc = data->calibrate(chip, i, golden_buf, test_buf);
>   		if (rc == 0)
> -			best_div = i;
> +			best_freq = freq;
>   	}
>   
>   	/* Nothing found ? */
> -	if (best_div < 0) {
> -		dev_warn(aspi->dev, "No good frequency, using dumb slow");
> -	} else {
> -		dev_dbg(aspi->dev, "Found good read timings at HCLK/%d", best_div);
> +	if (best_freq == 0)
> +		dev_warn(aspi->dev, "Use the default timing setting");
> +	else
> +		dev_dbg(aspi->dev, "Found good read timings at HCLK/%d", i);
>   
> -		/* Record the freq */
> -		for (i = 0; i < ASPEED_SPI_MAX; i++)
> -			chip->ctl_val[i] = (chip->ctl_val[i] & data->hclk_mask) |
> -				ASPEED_SPI_HCLK_DIV(best_div);
> -	}
>   
> -no_calib:
> +end_calib:
> +	if (best_freq == 0)
> +		best_freq = max_freq;
> +
> +	clk_conf = data->clk_config(chip, best_freq);
> +	/* Record the freq */
> +	for (i = 0; i < ASPEED_SPI_MAX; i++)
> +		chip->ctl_val[i] = (chip->ctl_val[i] & data->hclk_mask) | clk_conf;
> +
>   	writel(chip->ctl_val[ASPEED_SPI_READ], chip->ctl);
>   	kfree(test_buf);
>   	return 0;
>   }
>   
> +/* HCLK/1 ..	HCLK/16 */
> +static const u32 aspeed_spi_hclk_masks[] = {
> +	15, 7, 14, 6, 13, 5, 12, 4,
> +	11, 3, 10, 2, 9,  1, 8,  0
> +};

This new array is a bit redundant with aspeed_spi_hclk_divs[]

> +
> +static u32 aspeed_spi_ast2400_clk_config(struct aspeed_spi_chip *chip,
> +					 u32 max_hz)
> +{
> +	struct aspeed_spi *aspi = chip->aspi;
> +	u32 ahb_freq = aspi->clk_freq;
> +	u32 hclk_div = 0; /* default value */
> +	u32 i;
> +	bool found = false;
> +
> +	/* FMC/SPIR10[11:8] */
> +	for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
> +		if (ahb_freq / (i + 1) <= max_hz) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (found)
> +		hclk_div = aspeed_spi_hclk_masks[i] << 8;
> +
> +	dev_dbg(aspi->dev, "found: %s, hclk: %d, max_clk: %d\n",
> +		found ? "yes" : "no", ahb_freq, max_hz);
> +
> +	if (found) {
> +		dev_dbg(aspi->dev, "h_div: %d (mask %x)\n",
> +			i + 1, aspeed_spi_hclk_masks[i]);
> +	}
> +
> +	return hclk_div;
> +}
> +
> +static u32 aspeed_spi_ast2500_clk_config(struct aspeed_spi_chip *chip,
> +					 u32 max_hz)
> +{
> +	struct aspeed_spi *aspi = chip->aspi;
> +	u32 ahb_freq = aspi->clk_freq;
> +	u32 hclk_div = 0; /* default value */
> +	u32 i;
> +	bool found = false;
> +
> +	/* FMC/SPIR10[11:8] */
> +	for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
> +		if (ahb_freq / (i + 1) <= max_hz) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	if (found) {
> +		hclk_div = aspeed_spi_hclk_masks[i] << 8;
> +	} else {
> +		/* If FMC10[13] is set, an extra div_4 can be introduced. */
> +		for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
> +			if (ahb_freq / ((i + 1) * 4) <= max_hz) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (found)
> +			hclk_div = BIT(13) | (aspeed_spi_hclk_masks[i] << 8);
> +	}
> +
> +	dev_dbg(aspi->dev, "found: %s, hclk: %d, max_clk: %d\n",
> +		found ? "yes" : "no", ahb_freq, max_hz);
> +
> +	if (found) {
> +		dev_dbg(aspi->dev, "h_div: %d (mask %x)\n",
> +			i + 1, aspeed_spi_hclk_masks[i]);
> +	}
> +
> +	return hclk_div;
> +}
> +
> +static u32 aspeed_spi_ast2600_clk_config(struct aspeed_spi_chip *chip,
> +					 u32 max_hz)
> +{
> +	struct aspeed_spi *aspi = chip->aspi;
> +	u32 ahb_freq = aspi->clk_freq;
> +	u32 hclk_div = 0x400; /* default value */
> +	u32 i, j;
> +	bool found = false;
> +
> +	/* FMC/SPIR10[27:24] */
> +	for (j = 0; j < 0xf; j++) {
> +		/* FMC/SPIR10[11:8] */
> +		for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
> +			if (i == 0 && j == 0)
> +				continue;
> +
> +			if (ahb_freq / (i + 1 + (j * 16)) <= max_hz) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (found) {
> +			hclk_div = ((j << 24) | aspeed_spi_hclk_masks[i] << 8);
> +			break;
> +		}
> +	}
> +
> +	dev_dbg(aspi->dev, "found: %s, hclk: %d, max_clk: %d\n",
> +		found ? "yes" : "no", ahb_freq, max_hz);
> +
> +	if (found) {
> +		dev_dbg(aspi->dev, "base_clk: %d, h_div: %d (mask %x)\n",
> +			j, i + 1, aspeed_spi_hclk_masks[i]);
> +	}
> +
> +	return hclk_div;
> +}
> +
>   #define TIMING_DELAY_DI		BIT(3)
>   #define TIMING_DELAY_HCYCLE_MAX	5
>   #define TIMING_REG_AST2600(chip)				\
> @@ -1097,6 +1223,7 @@ static const struct aspeed_spi_data ast2400_fmc_data = {
>   	.segment_start = aspeed_spi_segment_start,
>   	.segment_end   = aspeed_spi_segment_end,
>   	.segment_reg   = aspeed_spi_segment_reg,
> +	.clk_config    = aspeed_spi_ast2400_clk_config,
>   };
>   
>   static const struct aspeed_spi_data ast2400_spi_data = {
> @@ -1109,6 +1236,7 @@ static const struct aspeed_spi_data ast2400_spi_data = {
>   	.hdiv_max      = 1,
>   	.calibrate     = aspeed_spi_calibrate,
>   	/* No segment registers */
> +	.clk_config    = aspeed_spi_ast2400_clk_config,
>   };
>   
>   static const struct aspeed_spi_data ast2500_fmc_data = {
> @@ -1117,12 +1245,13 @@ static const struct aspeed_spi_data ast2500_fmc_data = {
>   	.we0	       = 16,
>   	.ctl0	       = CE0_CTRL_REG,
>   	.timing	       = CE0_TIMING_COMPENSATION_REG,
> -	.hclk_mask     = 0xfffff0ff,
> +	.hclk_mask     = 0xffffd0ff,

I think this change of. hclk_mask and the one below should be included
in the initial patchset. I will when I publish v5.

Thanks,

C.

>   	.hdiv_max      = 1,
>   	.calibrate     = aspeed_spi_calibrate,
>   	.segment_start = aspeed_spi_segment_start,
>   	.segment_end   = aspeed_spi_segment_end,
>   	.segment_reg   = aspeed_spi_segment_reg,
> +	.clk_config    = aspeed_spi_ast2500_clk_config,
>   };
>   
>   static const struct aspeed_spi_data ast2500_spi_data = {
> @@ -1131,12 +1260,13 @@ static const struct aspeed_spi_data ast2500_spi_data = {
>   	.we0	       = 16,
>   	.ctl0	       = CE0_CTRL_REG,
>   	.timing	       = CE0_TIMING_COMPENSATION_REG,
> -	.hclk_mask     = 0xfffff0ff,
> +	.hclk_mask     = 0xffffd0ff,
>   	.hdiv_max      = 1,
>   	.calibrate     = aspeed_spi_calibrate,
>   	.segment_start = aspeed_spi_segment_start,
>   	.segment_end   = aspeed_spi_segment_end,
>   	.segment_reg   = aspeed_spi_segment_reg,
> +	.clk_config    = aspeed_spi_ast2500_clk_config,
>   };
>   
>   static const struct aspeed_spi_data ast2600_fmc_data = {
> @@ -1152,6 +1282,7 @@ static const struct aspeed_spi_data ast2600_fmc_data = {
>   	.segment_start = aspeed_spi_segment_ast2600_start,
>   	.segment_end   = aspeed_spi_segment_ast2600_end,
>   	.segment_reg   = aspeed_spi_segment_ast2600_reg,
> +	.clk_config    = aspeed_spi_ast2600_clk_config,
>   };
>   
>   static const struct aspeed_spi_data ast2600_spi_data = {
> @@ -1167,6 +1298,7 @@ static const struct aspeed_spi_data ast2600_spi_data = {
>   	.segment_start = aspeed_spi_segment_ast2600_start,
>   	.segment_end   = aspeed_spi_segment_ast2600_end,
>   	.segment_reg   = aspeed_spi_segment_ast2600_reg,
> +	.clk_config    = aspeed_spi_ast2600_clk_config,
>   };
>   
>   static const struct of_device_id aspeed_spi_matches[] = {

RE: [PATCH 1/1] spi: aspeed: Update SPI clock frequency configuration

[Chin-Ting Kuo]

Hi Cédric,

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Tuesday, April 19, 2022 4:42 AM
> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>;
> linux-aspeed@lists.ozlabs.org; openbmc@lists.ozlabs.org
> Subject: Re: [PATCH 1/1] spi: aspeed: Update SPI clock frequency configuration
> 
> Hello Chin-Ting,
> 
> On 4/14/22 12:28, Chin-Ting Kuo wrote:
> > Instead of using the slowest one, the maximum clock frequency
> > configured in the device tree should be kept when no timing
> > calibration process is executed.
> > Besides, an extra callback function is added in order to calculate
> > clock frequency configuration for different ASPEED platforms.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> 
> I gave this patch a try on an AST2600 A3 EVB and an AST2500 EVB and it
> behaved as expected. The default setting from the DT is chosen when the flash
> contents are too uniform.
> 
> 
> > ---
> >   drivers/spi/spi-aspeed-smc.c | 166
> +++++++++++++++++++++++++++++++----
> >   1 file changed, 149 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/spi/spi-aspeed-smc.c
> > b/drivers/spi/spi-aspeed-smc.c index 227797e13997..728163d0045d 100644
> > --- a/drivers/spi/spi-aspeed-smc.c
> > +++ b/drivers/spi/spi-aspeed-smc.c
> > @@ -3,7 +3,7 @@
> >    * ASPEED FMC/SPI Memory Controller Driver
> >    *
> >    * Copyright (c) 2015-2022, IBM Corporation.
> > - * Copyright (c) 2020, ASPEED Corporation.
> > + * Copyright (c) 2020-2022, ASPEED Corporation.
> >    */
> >   #include <linux/clk.h>
> > @@ -84,6 +84,7 @@ struct aspeed_spi_data {
> >   	u32 (*segment_reg)(struct aspeed_spi *aspi, u32 start, u32 end);
> >   	int (*calibrate)(struct aspeed_spi_chip *chip, u32 hdiv,
> >   			 const u8 *golden_buf, u8 *test_buf);
> > +	u32 (*clk_config)(struct aspeed_spi_chip *chip, u32 max_hz);
> >   };
> >
> >   #define ASPEED_SPI_MAX_NUM_CS	5
> > @@ -959,7 +960,10 @@ static int aspeed_spi_do_calibration(struct
> aspeed_spi_chip *chip)
> >   	u32 ctl_val;
> >   	u8 *golden_buf = NULL;
> >   	u8 *test_buf = NULL;
> > -	int i, rc, best_div = -1;
> > +	int i, rc;
> > +	u32 best_freq = 0;
> > +	u32 freq;
> > +	u32 clk_conf;
> >
> >   	dev_dbg(aspi->dev, "calculate timing compensation - AHB freq: %d
> MHz",
> >   		ahb_freq / 1000000);
> > @@ -980,7 +984,7 @@ static int aspeed_spi_do_calibration(struct
> aspeed_spi_chip *chip)
> >   	memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
> >   	if (!aspeed_spi_check_calib_data(golden_buf, CALIBRATE_BUF_SIZE))
> {
> >   		dev_info(aspi->dev, "Calibration area too uniform, using low
> > speed");
> 
> may be change dev_info() to "..., using default freq"

Okay.

> 
> > -		goto no_calib;
> > +		goto end_calib;
> >   	}
> >
> >   #if defined(VERBOSE_DEBUG)
> > @@ -990,7 +994,7 @@ static int aspeed_spi_do_calibration(struct
> > aspeed_spi_chip *chip)
> >
> >   	/* Now we iterate the HCLK dividers until we find our breaking point
> */
> >   	for (i = ARRAY_SIZE(aspeed_spi_hclk_divs); i > data->hdiv_max - 1;
> i--) {
> > -		u32 tv, freq;
> > +		u32 tv;
> >
> >   		freq = ahb_freq / i;
> >   		if (freq > max_freq)
> > @@ -1002,27 +1006,149 @@ static int aspeed_spi_do_calibration(struct
> aspeed_spi_chip *chip)
> >   		dev_dbg(aspi->dev, "Trying HCLK/%d [%08x] ...", i, tv);
> >   		rc = data->calibrate(chip, i, golden_buf, test_buf);
> >   		if (rc == 0)
> > -			best_div = i;
> > +			best_freq = freq;
> >   	}
> >
> >   	/* Nothing found ? */
> > -	if (best_div < 0) {
> > -		dev_warn(aspi->dev, "No good frequency, using dumb slow");
> > -	} else {
> > -		dev_dbg(aspi->dev, "Found good read timings at HCLK/%d",
> best_div);
> > +	if (best_freq == 0)
> > +		dev_warn(aspi->dev, "Use the default timing setting");
> > +	else
> > +		dev_dbg(aspi->dev, "Found good read timings at HCLK/%d", i);
> >
> > -		/* Record the freq */
> > -		for (i = 0; i < ASPEED_SPI_MAX; i++)
> > -			chip->ctl_val[i] = (chip->ctl_val[i] & data->hclk_mask) |
> > -				ASPEED_SPI_HCLK_DIV(best_div);
> > -	}
> >
> > -no_calib:
> > +end_calib:
> > +	if (best_freq == 0)
> > +		best_freq = max_freq;
> > +
> > +	clk_conf = data->clk_config(chip, best_freq);
> > +	/* Record the freq */
> > +	for (i = 0; i < ASPEED_SPI_MAX; i++)
> > +		chip->ctl_val[i] = (chip->ctl_val[i] & data->hclk_mask) | clk_conf;
> > +
> >   	writel(chip->ctl_val[ASPEED_SPI_READ], chip->ctl);
> >   	kfree(test_buf);
> >   	return 0;
> >   }
> >
> > +/* HCLK/1 ..	HCLK/16 */
> > +static const u32 aspeed_spi_hclk_masks[] = {
> > +	15, 7, 14, 6, 13, 5, 12, 4,
> > +	11, 3, 10, 2, 9,  1, 8,  0
> > +};
> 
> This new array is a bit redundant with aspeed_spi_hclk_divs[]
> 

Yes, I will try to merge this array into aspeed_spi_hclk_divs.

> > +
> > +static u32 aspeed_spi_ast2400_clk_config(struct aspeed_spi_chip *chip,
> > +					 u32 max_hz)
> > +{
> > +	struct aspeed_spi *aspi = chip->aspi;
> > +	u32 ahb_freq = aspi->clk_freq;
> > +	u32 hclk_div = 0; /* default value */
> > +	u32 i;
> > +	bool found = false;
> > +
> > +	/* FMC/SPIR10[11:8] */
> > +	for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
> > +		if (ahb_freq / (i + 1) <= max_hz) {
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (found)
> > +		hclk_div = aspeed_spi_hclk_masks[i] << 8;
> > +
> > +	dev_dbg(aspi->dev, "found: %s, hclk: %d, max_clk: %d\n",
> > +		found ? "yes" : "no", ahb_freq, max_hz);
> > +
> > +	if (found) {
> > +		dev_dbg(aspi->dev, "h_div: %d (mask %x)\n",
> > +			i + 1, aspeed_spi_hclk_masks[i]);
> > +	}
> > +
> > +	return hclk_div;
> > +}
> > +
> > +static u32 aspeed_spi_ast2500_clk_config(struct aspeed_spi_chip *chip,
> > +					 u32 max_hz)
> > +{
> > +	struct aspeed_spi *aspi = chip->aspi;
> > +	u32 ahb_freq = aspi->clk_freq;
> > +	u32 hclk_div = 0; /* default value */
> > +	u32 i;
> > +	bool found = false;
> > +
> > +	/* FMC/SPIR10[11:8] */
> > +	for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
> > +		if (ahb_freq / (i + 1) <= max_hz) {
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (found) {
> > +		hclk_div = aspeed_spi_hclk_masks[i] << 8;
> > +	} else {
> > +		/* If FMC10[13] is set, an extra div_4 can be introduced. */
> > +		for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
> > +			if (ahb_freq / ((i + 1) * 4) <= max_hz) {
> > +				found = true;
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (found)
> > +			hclk_div = BIT(13) | (aspeed_spi_hclk_masks[i] << 8);
> > +	}
> > +
> > +	dev_dbg(aspi->dev, "found: %s, hclk: %d, max_clk: %d\n",
> > +		found ? "yes" : "no", ahb_freq, max_hz);
> > +
> > +	if (found) {
> > +		dev_dbg(aspi->dev, "h_div: %d (mask %x)\n",
> > +			i + 1, aspeed_spi_hclk_masks[i]);
> > +	}
> > +
> > +	return hclk_div;
> > +}
> > +
> > +static u32 aspeed_spi_ast2600_clk_config(struct aspeed_spi_chip *chip,
> > +					 u32 max_hz)
> > +{
> > +	struct aspeed_spi *aspi = chip->aspi;
> > +	u32 ahb_freq = aspi->clk_freq;
> > +	u32 hclk_div = 0x400; /* default value */
> > +	u32 i, j;
> > +	bool found = false;
> > +
> > +	/* FMC/SPIR10[27:24] */
> > +	for (j = 0; j < 0xf; j++) {
> > +		/* FMC/SPIR10[11:8] */
> > +		for (i = 0; i < ARRAY_SIZE(aspeed_spi_hclk_masks); i++) {
> > +			if (i == 0 && j == 0)
> > +				continue;
> > +
> > +			if (ahb_freq / (i + 1 + (j * 16)) <= max_hz) {
> > +				found = true;
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (found) {
> > +			hclk_div = ((j << 24) | aspeed_spi_hclk_masks[i] << 8);
> > +			break;
> > +		}
> > +	}
> > +
> > +	dev_dbg(aspi->dev, "found: %s, hclk: %d, max_clk: %d\n",
> > +		found ? "yes" : "no", ahb_freq, max_hz);
> > +
> > +	if (found) {
> > +		dev_dbg(aspi->dev, "base_clk: %d, h_div: %d (mask %x)\n",
> > +			j, i + 1, aspeed_spi_hclk_masks[i]);
> > +	}
> > +
> > +	return hclk_div;
> > +}
> > +
> >   #define TIMING_DELAY_DI		BIT(3)
> >   #define TIMING_DELAY_HCYCLE_MAX	5
> >   #define TIMING_REG_AST2600(chip)				\
> > @@ -1097,6 +1223,7 @@ static const struct aspeed_spi_data
> ast2400_fmc_data = {
> >   	.segment_start = aspeed_spi_segment_start,
> >   	.segment_end   = aspeed_spi_segment_end,
> >   	.segment_reg   = aspeed_spi_segment_reg,
> > +	.clk_config    = aspeed_spi_ast2400_clk_config,
> >   };
> >
> >   static const struct aspeed_spi_data ast2400_spi_data = { @@ -1109,6
> > +1236,7 @@ static const struct aspeed_spi_data ast2400_spi_data = {
> >   	.hdiv_max      = 1,
> >   	.calibrate     = aspeed_spi_calibrate,
> >   	/* No segment registers */
> > +	.clk_config    = aspeed_spi_ast2400_clk_config,
> >   };
> >
> >   static const struct aspeed_spi_data ast2500_fmc_data = { @@ -1117,12
> > +1245,13 @@ static const struct aspeed_spi_data ast2500_fmc_data = {
> >   	.we0	       = 16,
> >   	.ctl0	       = CE0_CTRL_REG,
> >   	.timing	       = CE0_TIMING_COMPENSATION_REG,
> > -	.hclk_mask     = 0xfffff0ff,
> > +	.hclk_mask     = 0xffffd0ff,
> 
> I think this change of. hclk_mask and the one below should be included in the
> initial patchset. I will when I publish v5.
> 

Okay.


Thanks.

Chin-Ting

> Thanks,
> 
> C.
> 
> >   	.hdiv_max      = 1,
> >   	.calibrate     = aspeed_spi_calibrate,
> >   	.segment_start = aspeed_spi_segment_start,
> >   	.segment_end   = aspeed_spi_segment_end,
> >   	.segment_reg   = aspeed_spi_segment_reg,
> > +	.clk_config    = aspeed_spi_ast2500_clk_config,
> >   };
> >
> >   static const struct aspeed_spi_data ast2500_spi_data = { @@ -1131,12
> > +1260,13 @@ static const struct aspeed_spi_data ast2500_spi_data = {
> >   	.we0	       = 16,
> >   	.ctl0	       = CE0_CTRL_REG,
> >   	.timing	       = CE0_TIMING_COMPENSATION_REG,
> > -	.hclk_mask     = 0xfffff0ff,
> > +	.hclk_mask     = 0xffffd0ff,
> >   	.hdiv_max      = 1,
> >   	.calibrate     = aspeed_spi_calibrate,
> >   	.segment_start = aspeed_spi_segment_start,
> >   	.segment_end   = aspeed_spi_segment_end,
> >   	.segment_reg   = aspeed_spi_segment_reg,
> > +	.clk_config    = aspeed_spi_ast2500_clk_config,
> >   };
> >
> >   static const struct aspeed_spi_data ast2600_fmc_data = { @@ -1152,6
> > +1282,7 @@ static const struct aspeed_spi_data ast2600_fmc_data = {
> >   	.segment_start = aspeed_spi_segment_ast2600_start,
> >   	.segment_end   = aspeed_spi_segment_ast2600_end,
> >   	.segment_reg   = aspeed_spi_segment_ast2600_reg,
> > +	.clk_config    = aspeed_spi_ast2600_clk_config,
> >   };
> >
> >   static const struct aspeed_spi_data ast2600_spi_data = { @@ -1167,6
> > +1298,7 @@ static const struct aspeed_spi_data ast2600_spi_data = {
> >   	.segment_start = aspeed_spi_segment_ast2600_start,
> >   	.segment_end   = aspeed_spi_segment_ast2600_end,
> >   	.segment_reg   = aspeed_spi_segment_ast2600_reg,
> > +	.clk_config    = aspeed_spi_ast2600_clk_config,
> >   };
> >
> >   static const struct of_device_id aspeed_spi_matches[] = {