[PATCH v2 0/3] spi: meson-axg: add few enhanced features

[PATCH v2 0/3] spi: meson-axg: add few enhanced features

[Sunny Luo]

add a few enhanced features for the SPICC controller of Meson-AXG SoC.

These patches are actually quite independent from each other, I send them
together in case to avoid the file conflicts.

Changes since v1 at [1]
- Add OF and COMMON_CLK dependence for SPICC in Kconfig to avoid compiling
  error.

[1] https://lore.kernel.org/lkml/20180503213645.20694-1-yixun.lan@amlogic.com

Sunny Luo (3):
  spi: meson-axg: support MAX 80M clock
  spi: meson-axg: enhance output enable feature
  spi: meson-axg: add a linear clock divider support

 drivers/spi/Kconfig           |   2 +-
 drivers/spi/spi-meson-spicc.c | 270 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 223 insertions(+), 49 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] spi: meson-axg: support MAX 80M clock

[Sunny Luo]

The SPICC controller in Meson-AXG is capable of running at 80M clock.
The ASIC IP is improved and the clock is actually running higher than
previous old SoCs.

Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/spi/spi-meson-spicc.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
index 7fe4488..b56249d 100644
--- a/drivers/spi/spi-meson-spicc.c
+++ b/drivers/spi/spi-meson-spicc.c
@@ -9,11 +9,13 @@
 
 #include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/spi/spi.h>
 #include <linux/types.h>
@@ -34,7 +36,6 @@
  *   to have a CS go down over the full transfer
  */
 
-#define SPICC_MAX_FREQ	30000000
 #define SPICC_MAX_BURST	128
 
 /* Register Map */
@@ -120,6 +121,10 @@
 #define SPICC_BURST_MAX	16
 #define SPICC_FIFO_HALF 10
 
+struct meson_spicc_data {
+	unsigned int			max_speed_hz;
+};
+
 struct meson_spicc_device {
 	struct spi_master		*master;
 	struct platform_device		*pdev;
@@ -127,6 +132,7 @@ struct meson_spicc_device {
 	struct clk			*core;
 	struct spi_message		*message;
 	struct spi_transfer		*xfer;
+	const struct meson_spicc_data	*data;
 	u8				*tx_buf;
 	u8				*rx_buf;
 	unsigned int			bytes_per_word;
@@ -517,6 +523,9 @@ static int meson_spicc_probe(struct platform_device *pdev)
 	spicc->pdev = pdev;
 	platform_set_drvdata(pdev, spicc);
 
+	spicc->data = (const struct meson_spicc_data *)
+		of_device_get_match_data(&pdev->dev);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	spicc->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(spicc->base)) {
@@ -567,11 +576,9 @@ static int meson_spicc_probe(struct platform_device *pdev)
 	master->unprepare_transfer_hardware = meson_spicc_unprepare_transfer;
 	master->transfer_one = meson_spicc_transfer_one;
 
-	/* Setup max rate according to the Meson GX datasheet */
-	if ((rate >> 2) > SPICC_MAX_FREQ)
-		master->max_speed_hz = SPICC_MAX_FREQ;
-	else
-		master->max_speed_hz = rate >> 2;
+	/* Setup max rate according to the Meson datasheet */
+	master->max_speed_hz = min_t(unsigned int, rate >> 1,
+				     spicc->data->max_speed_hz);
 
 	ret = devm_spi_register_master(&pdev->dev, master);
 	if (ret) {
@@ -602,9 +609,23 @@ static int meson_spicc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct meson_spicc_data meson_spicc_gx_data = {
+	.max_speed_hz	= 30000000,
+};
+
+static const struct meson_spicc_data meson_spicc_axg_data = {
+	.max_speed_hz	= 80000000,
+};
+
 static const struct of_device_id meson_spicc_of_match[] = {
-	{ .compatible = "amlogic,meson-gx-spicc", },
-	{ .compatible = "amlogic,meson-axg-spicc", },
+	{
+		.compatible	= "amlogic,meson-gx-spicc",
+		.data		= &meson_spicc_gx_data,
+	},
+	{
+		.compatible = "amlogic,meson-axg-spicc",
+		.data		= &meson_spicc_axg_data,
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, meson_spicc_of_match);
-- 
2.7.4

Re: [PATCH v2 1/3] spi: meson-axg: support MAX 80M clock

[Neil Armstrong]

Hi Sunny,

On 13/12/2018 09:39, Sunny Luo wrote:
> The SPICC controller in Meson-AXG is capable of running at 80M clock.
> The ASIC IP is improved and the clock is actually running higher than
> previous old SoCs.
> 
> Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  drivers/spi/spi-meson-spicc.c | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> index 7fe4488..b56249d 100644
> --- a/drivers/spi/spi-meson-spicc.c
> +++ b/drivers/spi/spi-meson-spicc.c
> @@ -9,11 +9,13 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/device.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/spi/spi.h>
>  #include <linux/types.h>
> @@ -34,7 +36,6 @@
>   *   to have a CS go down over the full transfer
>   */
>  
> -#define SPICC_MAX_FREQ	30000000
>  #define SPICC_MAX_BURST	128
>  
>  /* Register Map */
> @@ -120,6 +121,10 @@
>  #define SPICC_BURST_MAX	16
>  #define SPICC_FIFO_HALF 10
>  
> +struct meson_spicc_data {
> +	unsigned int			max_speed_hz;
> +};
> +
>  struct meson_spicc_device {
>  	struct spi_master		*master;
>  	struct platform_device		*pdev;
> @@ -127,6 +132,7 @@ struct meson_spicc_device {
>  	struct clk			*core;
>  	struct spi_message		*message;
>  	struct spi_transfer		*xfer;
> +	const struct meson_spicc_data	*data;
>  	u8				*tx_buf;
>  	u8				*rx_buf;
>  	unsigned int			bytes_per_word;
> @@ -517,6 +523,9 @@ static int meson_spicc_probe(struct platform_device *pdev)
>  	spicc->pdev = pdev;
>  	platform_set_drvdata(pdev, spicc);
>  
> +	spicc->data = (const struct meson_spicc_data *)
> +		of_device_get_match_data(&pdev->dev);
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	spicc->base = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(spicc->base)) {
> @@ -567,11 +576,9 @@ static int meson_spicc_probe(struct platform_device *pdev)
>  	master->unprepare_transfer_hardware = meson_spicc_unprepare_transfer;
>  	master->transfer_one = meson_spicc_transfer_one;
>  
> -	/* Setup max rate according to the Meson GX datasheet */
> -	if ((rate >> 2) > SPICC_MAX_FREQ)
> -		master->max_speed_hz = SPICC_MAX_FREQ;
> -	else
> -		master->max_speed_hz = rate >> 2;
> +	/* Setup max rate according to the Meson datasheet */
> +	master->max_speed_hz = min_t(unsigned int, rate >> 1,
> +				     spicc->data->max_speed_hz);

I think "rate >> 1" here depends on patch 3, either move patch 3 before this one
or keep "rate >> 2" and change it back to "rate >> 1" on patch 3.

>  
>  	ret = devm_spi_register_master(&pdev->dev, master);
>  	if (ret) {
> @@ -602,9 +609,23 @@ static int meson_spicc_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct meson_spicc_data meson_spicc_gx_data = {
> +	.max_speed_hz	= 30000000,
> +};
> +
> +static const struct meson_spicc_data meson_spicc_axg_data = {
> +	.max_speed_hz	= 80000000,
> +};
> +
>  static const struct of_device_id meson_spicc_of_match[] = {
> -	{ .compatible = "amlogic,meson-gx-spicc", },
> -	{ .compatible = "amlogic,meson-axg-spicc", },
> +	{
> +		.compatible	= "amlogic,meson-gx-spicc",
> +		.data		= &meson_spicc_gx_data,
> +	},
> +	{
> +		.compatible = "amlogic,meson-axg-spicc",
> +		.data		= &meson_spicc_axg_data,
> +	},
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, meson_spicc_of_match);
> 

Re: [PATCH v2 1/3] spi: meson-axg: support MAX 80M clock

[Sunny Luo]

Hi Neil,

On 2018/12/13 16:49, Neil Armstrong wrote:
> Hi Sunny,
> 
> On 13/12/2018 09:39, Sunny Luo wrote:
>> The SPICC controller in Meson-AXG is capable of running at 80M clock.
>> The ASIC IP is improved and the clock is actually running higher than
>> previous old SoCs.
>>
>> Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> ---
>>   drivers/spi/spi-meson-spicc.c | 37 +++++++++++++++++++++++++++++--------
>>   1 file changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
>> index 7fe4488..b56249d 100644
>> --- a/drivers/spi/spi-meson-spicc.c
>> +++ b/drivers/spi/spi-meson-spicc.c
>> @@ -9,11 +9,13 @@
>>   
>>   #include <linux/bitfield.h>
>>   #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>>   #include <linux/device.h>
>>   #include <linux/io.h>
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>> +#include <linux/of_device.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/spi/spi.h>
>>   #include <linux/types.h>
>> @@ -34,7 +36,6 @@
>>    *   to have a CS go down over the full transfer
>>    */
>>   
>> -#define SPICC_MAX_FREQ	30000000
>>   #define SPICC_MAX_BURST	128
>>   
>>   /* Register Map */
>> @@ -120,6 +121,10 @@
>>   #define SPICC_BURST_MAX	16
>>   #define SPICC_FIFO_HALF 10
>>   
>> +struct meson_spicc_data {
>> +	unsigned int			max_speed_hz;
>> +};
>> +
>>   struct meson_spicc_device {
>>   	struct spi_master		*master;
>>   	struct platform_device		*pdev;
>> @@ -127,6 +132,7 @@ struct meson_spicc_device {
>>   	struct clk			*core;
>>   	struct spi_message		*message;
>>   	struct spi_transfer		*xfer;
>> +	const struct meson_spicc_data	*data;
>>   	u8				*tx_buf;
>>   	u8				*rx_buf;
>>   	unsigned int			bytes_per_word;
>> @@ -517,6 +523,9 @@ static int meson_spicc_probe(struct platform_device *pdev)
>>   	spicc->pdev = pdev;
>>   	platform_set_drvdata(pdev, spicc);
>>   
>> +	spicc->data = (const struct meson_spicc_data *)
>> +		of_device_get_match_data(&pdev->dev);
>> +
>>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>   	spicc->base = devm_ioremap_resource(&pdev->dev, res);
>>   	if (IS_ERR(spicc->base)) {
>> @@ -567,11 +576,9 @@ static int meson_spicc_probe(struct platform_device *pdev)
>>   	master->unprepare_transfer_hardware = meson_spicc_unprepare_transfer;
>>   	master->transfer_one = meson_spicc_transfer_one;
>>   
>> -	/* Setup max rate according to the Meson GX datasheet */
>> -	if ((rate >> 2) > SPICC_MAX_FREQ)
>> -		master->max_speed_hz = SPICC_MAX_FREQ;
>> -	else
>> -		master->max_speed_hz = rate >> 2;
>> +	/* Setup max rate according to the Meson datasheet */
>> +	master->max_speed_hz = min_t(unsigned int, rate >> 1,
>> +				     spicc->data->max_speed_hz);
> 
> I think "rate >> 1" here depends on patch 3, either move patch 3 before this one
> or keep "rate >> 2" and change it back to "rate >> 1" on patch 3.
> 
Yes, this change should be in patch 3.

>>   
>>   	ret = devm_spi_register_master(&pdev->dev, master);
>>   	if (ret) {
>> @@ -602,9 +609,23 @@ static int meson_spicc_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +static const struct meson_spicc_data meson_spicc_gx_data = {
>> +	.max_speed_hz	= 30000000,
>> +};
>> +
>> +static const struct meson_spicc_data meson_spicc_axg_data = {
>> +	.max_speed_hz	= 80000000,
>> +};
>> +
>>   static const struct of_device_id meson_spicc_of_match[] = {
>> -	{ .compatible = "amlogic,meson-gx-spicc", },
>> -	{ .compatible = "amlogic,meson-axg-spicc", },
>> +	{
>> +		.compatible	= "amlogic,meson-gx-spicc",
>> +		.data		= &meson_spicc_gx_data,
>> +	},
>> +	{
>> +		.compatible = "amlogic,meson-axg-spicc",
>> +		.data		= &meson_spicc_axg_data,
>> +	},
>>   	{ /* sentinel */ }
>>   };
>>   MODULE_DEVICE_TABLE(of, meson_spicc_of_match);
>>
> 
> .
> 

[PATCH v2 2/3] spi: meson-axg: enhance output enable feature

[Sunny Luo]

The SPICC controller in Meson-AXG is capable of driving the CLK/MOSI/SS
signal lines through the idle state (between two transmission operation),
which avoid the signals floating in unexpected state.

Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/spi/spi-meson-spicc.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
index b56249d..0384c28 100644
--- a/drivers/spi/spi-meson-spicc.c
+++ b/drivers/spi/spi-meson-spicc.c
@@ -115,6 +115,13 @@
 
 #define SPICC_DWADDR	0x24	/* Write Address of DMA */
 
+#define SPICC_ENH_CTL0	0x38	/* Enhanced Feature */
+#define SPICC_ENH_MOSI_OEN		BIT(25)
+#define SPICC_ENH_CLK_OEN		BIT(26)
+#define SPICC_ENH_CS_OEN		BIT(27)
+#define SPICC_ENH_CLK_CS_DELAY_EN	BIT(28)
+#define SPICC_ENH_MAIN_CLK_AO		BIT(29)
+
 #define writel_bits_relaxed(mask, val, addr) \
 	writel_relaxed((readl_relaxed(addr) & ~(mask)) | (val), addr)
 
@@ -123,6 +130,7 @@
 
 struct meson_spicc_data {
 	unsigned int			max_speed_hz;
+	bool				has_oen;
 };
 
 struct meson_spicc_device {
@@ -145,6 +153,19 @@ struct meson_spicc_device {
 	bool				is_last_burst;
 };
 
+static void meson_spicc_oen_enable(struct meson_spicc_device *spicc)
+{
+	u32 conf;
+
+	if (!spicc->data->has_oen)
+		return;
+
+	conf = readl_relaxed(spicc->base + SPICC_ENH_CTL0) |
+		SPICC_ENH_MOSI_OEN | SPICC_ENH_CLK_OEN | SPICC_ENH_CS_OEN;
+
+	writel_relaxed(conf, spicc->base + SPICC_ENH_CTL0);
+}
+
 static inline bool meson_spicc_txfull(struct meson_spicc_device *spicc)
 {
 	return !!FIELD_GET(SPICC_TF,
@@ -453,6 +474,8 @@ static int meson_spicc_prepare_message(struct spi_master *master,
 
 	writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG);
 
+	meson_spicc_oen_enable(spicc);
+
 	return 0;
 }
 
@@ -610,11 +633,12 @@ static int meson_spicc_remove(struct platform_device *pdev)
 }
 
 static const struct meson_spicc_data meson_spicc_gx_data = {
-	.max_speed_hz	= 30000000,
+	.max_speed_hz		= 30000000,
 };
 
 static const struct meson_spicc_data meson_spicc_axg_data = {
-	.max_speed_hz	= 80000000,
+	.max_speed_hz		= 80000000,
+	.has_oen		= true,
 };
 
 static const struct of_device_id meson_spicc_of_match[] = {
-- 
2.7.4

Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature

[Neil Armstrong]

Hi Sunny,

On 13/12/2018 09:39, Sunny Luo wrote:
> The SPICC controller in Meson-AXG is capable of driving the CLK/MOSI/SS
> signal lines through the idle state (between two transmission operation),
> which avoid the signals floating in unexpected state.

This is welcome, because it's really missing on GX...
I tried implementing it with pinctrl at [1], but it's complex.

Can you provide more info on how we should implement in on GX to be on par ?

[1] https://github.com/superna9999/linux/commit/9c3a95659dd532d186556c1570c54d79ea5a4d45

> 
> Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  drivers/spi/spi-meson-spicc.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> index b56249d..0384c28 100644
> --- a/drivers/spi/spi-meson-spicc.c
> +++ b/drivers/spi/spi-meson-spicc.c
> @@ -115,6 +115,13 @@
>  
>  #define SPICC_DWADDR	0x24	/* Write Address of DMA */
>  
> +#define SPICC_ENH_CTL0	0x38	/* Enhanced Feature */
> +#define SPICC_ENH_MOSI_OEN		BIT(25)
> +#define SPICC_ENH_CLK_OEN		BIT(26)
> +#define SPICC_ENH_CS_OEN		BIT(27)
> +#define SPICC_ENH_CLK_CS_DELAY_EN	BIT(28)
> +#define SPICC_ENH_MAIN_CLK_AO		BIT(29)
> +
>  #define writel_bits_relaxed(mask, val, addr) \
>  	writel_relaxed((readl_relaxed(addr) & ~(mask)) | (val), addr)
>  
> @@ -123,6 +130,7 @@
>  
>  struct meson_spicc_data {
>  	unsigned int			max_speed_hz;
> +	bool				has_oen;
>  };
>  
>  struct meson_spicc_device {
> @@ -145,6 +153,19 @@ struct meson_spicc_device {
>  	bool				is_last_burst;
>  };
>  
> +static void meson_spicc_oen_enable(struct meson_spicc_device *spicc)
> +{
> +	u32 conf;
> +
> +	if (!spicc->data->has_oen)
> +		return;
> +
> +	conf = readl_relaxed(spicc->base + SPICC_ENH_CTL0) |
> +		SPICC_ENH_MOSI_OEN | SPICC_ENH_CLK_OEN | SPICC_ENH_CS_OEN;
> +
> +	writel_relaxed(conf, spicc->base + SPICC_ENH_CTL0);
> +}
> +
>  static inline bool meson_spicc_txfull(struct meson_spicc_device *spicc)
>  {
>  	return !!FIELD_GET(SPICC_TF,
> @@ -453,6 +474,8 @@ static int meson_spicc_prepare_message(struct spi_master *master,
>  
>  	writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG);
>  
> +	meson_spicc_oen_enable(spicc);
> +
>  	return 0;
>  }
>  
> @@ -610,11 +633,12 @@ static int meson_spicc_remove(struct platform_device *pdev)
>  }
>  
>  static const struct meson_spicc_data meson_spicc_gx_data = {
> -	.max_speed_hz	= 30000000,
> +	.max_speed_hz		= 30000000,

Nitpick, but I would have kept the indentation here ...

>  };
>  
>  static const struct meson_spicc_data meson_spicc_axg_data = {
> -	.max_speed_hz	= 80000000,
> +	.max_speed_hz		= 80000000,
> +	.has_oen		= true,

same here

>  };
>  
>  static const struct of_device_id meson_spicc_of_match[] = {
> 

Anywy it's nitpick,

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature

[Sunny Luo]

Hi Neil,

On 2018/12/13 16:53, Neil Armstrong wrote:
> Hi Sunny,
> 
> On 13/12/2018 09:39, Sunny Luo wrote:
>> The SPICC controller in Meson-AXG is capable of driving the CLK/MOSI/SS
>> signal lines through the idle state (between two transmission operation),
>> which avoid the signals floating in unexpected state.
> 
> This is welcome, because it's really missing on GX...
> I tried implementing it with pinctrl at [1], but it's complex.
> 
> Can you provide more info on how we should implement in on GX to be on par ?
> 
> [1] https://github.com/superna9999/linux/commit/9c3a95659dd532d186556c1570c54d79ea5a4d45
> 
GX is incapable of OEN. To be on par with it ,we have to pullup/down clk 
pin as
you did at[1].

>>   static const struct meson_spicc_data meson_spicc_gx_data = {
>> -	.max_speed_hz	= 30000000,
>> +	.max_speed_hz		= 30000000,
> 
> Nitpick, but I would have kept the indentation here ...
> 
>>   };
>>   
>>   static const struct meson_spicc_data meson_spicc_axg_data = {
>> -	.max_speed_hz	= 80000000,
>> +	.max_speed_hz		= 80000000,
>> +	.has_oen		= true,
> 
> same here
> 
> Anywy it's nitpick,
>  
ok, i will revert it next patch.

Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature

[Jerome Brunet]

On Thu, 2018-12-13 at 16:39 +0800, Sunny Luo wrote:
> The SPICC controller in Meson-AXG is capable of driving the CLK/MOSI/SS
> signal lines through the idle state (between two transmission operation),
> which avoid the signals floating in unexpected state.
> 
> Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  drivers/spi/spi-meson-spicc.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> index b56249d..0384c28 100644
> --- a/drivers/spi/spi-meson-spicc.c
> +++ b/drivers/spi/spi-meson-spicc.c
> @@ -115,6 +115,13 @@
>  
>  #define SPICC_DWADDR	0x24	/* Write Address of DMA */
>  
> +#define SPICC_ENH_CTL0	0x38	/* Enhanced Feature */
> +#define SPICC_ENH_MOSI_OEN		BIT(25)
> +#define SPICC_ENH_CLK_OEN		BIT(26)
> +#define SPICC_ENH_CS_OEN		BIT(27)
> +#define SPICC_ENH_CLK_CS_DELAY_EN	BIT(28)
> +#define SPICC_ENH_MAIN_CLK_AO		BIT(29)
> +
>  #define writel_bits_relaxed(mask, val, addr) \
>  	writel_relaxed((readl_relaxed(addr) & ~(mask)) | (val), addr)
>  
> @@ -123,6 +130,7 @@
>  
>  struct meson_spicc_data {
>  	unsigned int			max_speed_hz;
> +	bool				has_oen;
>  };
>  
>  struct meson_spicc_device {
> @@ -145,6 +153,19 @@ struct meson_spicc_device {
>  	bool				is_last_burst;
>  };
>  
> +static void meson_spicc_oen_enable(struct meson_spicc_device *spicc)
> +{
> +	u32 conf;
> +
> +	if (!spicc->data->has_oen)
> +		return;
> +
> +	conf = readl_relaxed(spicc->base + SPICC_ENH_CTL0) |
> +		SPICC_ENH_MOSI_OEN | SPICC_ENH_CLK_OEN | SPICC_ENH_CS_OEN;
> +
> +	writel_relaxed(conf, spicc->base + SPICC_ENH_CTL0);
> +}
> +
>  static inline bool meson_spicc_txfull(struct meson_spicc_device *spicc)
>  {
>  	return !!FIELD_GET(SPICC_TF,
> @@ -453,6 +474,8 @@ static int meson_spicc_prepare_message(struct spi_master
> *master,
>  
>  	writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG);
>  
> +	meson_spicc_oen_enable(spicc);
> +

Any specific reason for doing this in prepare_message() ? It looks like
something that could/should be done during the probe ?

>  	return 0;
>  }
>  
> @@ -610,11 +633,12 @@ static int meson_spicc_remove(struct platform_device
> *pdev)
>  }
>  
>  static const struct meson_spicc_data meson_spicc_gx_data = {
> -	.max_speed_hz	= 30000000,
> +	.max_speed_hz		= 30000000,
>  };
>  
>  static const struct meson_spicc_data meson_spicc_axg_data = {
> -	.max_speed_hz	= 80000000,
> +	.max_speed_hz		= 80000000,
> +	.has_oen		= true,
>  };
>  
>  static const struct of_device_id meson_spicc_of_match[] = {

Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 884 bytes --]

On Thu, Dec 13, 2018 at 10:04:56AM +0100, Jerome Brunet wrote:
> On Thu, 2018-12-13 at 16:39 +0800, Sunny Luo wrote:

> >  
> >  	writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG);
> >  
> > +	meson_spicc_oen_enable(spicc);
> > +

> Any specific reason for doing this in prepare_message() ? It looks like
> something that could/should be done during the probe ?

If it's for power management then there should be a matching disable in
unprepare_message() (or this should just be in the runtime PM code,
though it's possible there's stuff that's only needed while actually
doing transfers in which case this could make sense).

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature

[Sunny Luo]

Hi Mark&Jerome,

On 2018/12/13 19:53, Mark Brown wrote:
> On Thu, Dec 13, 2018 at 10:04:56AM +0100, Jerome Brunet wrote:
>> On Thu, 2018-12-13 at 16:39 +0800, Sunny Luo wrote:
> 
>>>   
>>>   	writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG);
>>>   
>>> +	meson_spicc_oen_enable(spicc);
>>> +
> 
>> Any specific reason for doing this in prepare_message() ? It looks like
>> something that could/should be done during the probe ?
> 
> If it's for power management then there should be a matching disable in
> unprepare_message() (or this should just be in the runtime PM code,
> though it's possible there's stuff that's only needed while actually
> doing transfers in which case this could make sense).
> 
OEN is only used to avoid the signals floating in unexpected state, i 
will move it into probe next patch.

> Please delete unneeded context from mails when replying.  Doing this
> makes it much easier to find your reply in the message, helping ensure
> it won't be missed by people scrolling through the irrelevant quoted
> material.
> 
OK.

Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature

[Sunny Luo]

Hi Jerome,

On 2018/12/13 17:04, Jerome Brunet wrote:
> On Thu, 2018-12-13 at 16:39 +0800, Sunny Luo wrote:
>> The SPICC controller in Meson-AXG is capable of driving the CLK/MOSI/SS
>> signal lines through the idle state (between two transmission operation),
>> which avoid the signals floating in unexpected state.
>>

>> @@ -453,6 +474,8 @@ static int meson_spicc_prepare_message(struct spi_master
>> *master,
>>   
>>   	writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG);
>>   
>> +	meson_spicc_oen_enable(spicc);
>> +
> 
> Any specific reason for doing this in prepare_message() ? It looks like
> something that could/should be done during the probe

Yes, as replied in Mark's mail, i will move it into probe next patch.

[PATCH v2 3/3] spi: meson-axg: add a linear clock divider support

[Sunny Luo]

The SPICC controller in Meson-AXG SoC is capable of using
a linear clock divider to reach a much fine tuned range of clocks,
while the old controller only use a power of two clock divider,
result at a more coarse clock range.

Also convert the clock registration into Common Clock Framework.

Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/spi/Kconfig           |   2 +-
 drivers/spi/spi-meson-spicc.c | 209 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 170 insertions(+), 41 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 9f89cb1..75f7362 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -384,7 +384,7 @@ config SPI_FSL_ESPI
 
 config SPI_MESON_SPICC
 	tristate "Amlogic Meson SPICC controller"
-	depends on ARCH_MESON || COMPILE_TEST
+	depends on OF && COMMON_CLK && (ARCH_MESON || COMPILE_TEST)
 	help
 	  This enables master mode support for the SPICC (SPI communication
 	  controller) available in Amlogic Meson SoCs.
diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
index 0384c28..59af18e 100644
--- a/drivers/spi/spi-meson-spicc.c
+++ b/drivers/spi/spi-meson-spicc.c
@@ -116,6 +116,9 @@
 #define SPICC_DWADDR	0x24	/* Write Address of DMA */
 
 #define SPICC_ENH_CTL0	0x38	/* Enhanced Feature */
+#define SPICC_ENH_CLK_CS_DELAY_MASK	GENMASK(15, 0)
+#define SPICC_ENH_DATARATE_MASK		GENMASK(23, 16)
+#define SPICC_ENH_DATARATE_EN		BIT(24)
 #define SPICC_ENH_MOSI_OEN		BIT(25)
 #define SPICC_ENH_CLK_OEN		BIT(26)
 #define SPICC_ENH_CS_OEN		BIT(27)
@@ -131,6 +134,7 @@
 struct meson_spicc_data {
 	unsigned int			max_speed_hz;
 	bool				has_oen;
+	bool				has_enhance_clk_div;
 };
 
 struct meson_spicc_device {
@@ -138,6 +142,7 @@ struct meson_spicc_device {
 	struct platform_device		*pdev;
 	void __iomem			*base;
 	struct clk			*core;
+	struct clk			*clk;
 	struct spi_message		*message;
 	struct spi_transfer		*xfer;
 	const struct meson_spicc_data	*data;
@@ -325,40 +330,6 @@ static irqreturn_t meson_spicc_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static u32 meson_spicc_setup_speed(struct meson_spicc_device *spicc, u32 conf,
-				   u32 speed)
-{
-	unsigned long parent, value;
-	unsigned int i, div;
-
-	parent = clk_get_rate(spicc->core);
-
-	/* Find closest inferior/equal possible speed */
-	for (i = 0 ; i < 7 ; ++i) {
-		/* 2^(data_rate+2) */
-		value = parent >> (i + 2);
-
-		if (value <= speed)
-			break;
-	}
-
-	/* If provided speed it lower than max divider, use max divider */
-	if (i > 7) {
-		div = 7;
-		dev_warn_once(&spicc->pdev->dev, "unable to get close to speed %u\n",
-			      speed);
-	} else
-		div = i;
-
-	dev_dbg(&spicc->pdev->dev, "parent %lu, speed %u -> %lu (%u)\n",
-		parent, speed, value, div);
-
-	conf &= ~SPICC_DATARATE_MASK;
-	conf |= FIELD_PREP(SPICC_DATARATE_MASK, div);
-
-	return conf;
-}
-
 static void meson_spicc_setup_xfer(struct meson_spicc_device *spicc,
 				   struct spi_transfer *xfer)
 {
@@ -367,9 +338,6 @@ static void meson_spicc_setup_xfer(struct meson_spicc_device *spicc,
 	/* Read original configuration */
 	conf = conf_orig = readl_relaxed(spicc->base + SPICC_CONREG);
 
-	/* Select closest divider */
-	conf = meson_spicc_setup_speed(spicc, conf, xfer->speed_hz);
-
 	/* Setup word width */
 	conf &= ~SPICC_BITLENGTH_MASK;
 	conf |= FIELD_PREP(SPICC_BITLENGTH_MASK,
@@ -378,6 +346,8 @@ static void meson_spicc_setup_xfer(struct meson_spicc_device *spicc,
 	/* Ignore if unchanged */
 	if (conf != conf_orig)
 		writel_relaxed(conf, spicc->base + SPICC_CONREG);
+
+	clk_set_rate(spicc->clk, xfer->speed_hz);
 }
 
 static int meson_spicc_transfer_one(struct spi_master *master,
@@ -486,9 +456,6 @@ static int meson_spicc_unprepare_transfer(struct spi_master *master)
 	/* Disable all IRQs */
 	writel(0, spicc->base + SPICC_INTREG);
 
-	/* Disable controller */
-	writel_bits_relaxed(SPICC_ENABLE, 0, spicc->base + SPICC_CONREG);
-
 	device_reset_optional(&spicc->pdev->dev);
 
 	return 0;
@@ -528,6 +495,157 @@ static void meson_spicc_cleanup(struct spi_device *spi)
 	spi->controller_state = NULL;
 }
 
+/*
+ * The Clock Mux
+ *            x-----------------x   x------------x    x------\
+ *        |---| 0) fixed factor |---| 1) old div |----|      |
+ *        |   x-----------------x   x------------x    |      |
+ * src ---|                                           |5) mux|-- out
+ *        |   x-----------------x   x------------x    |      |
+ *        |---| 2) fixed factor |---| 3) new div |0---|      |
+ *            x-----------------x   x------------x    x------/
+ *
+ * Clk path for GX series:
+ *    src -> 0 -> 1 -> out
+ *
+ * Clk path for AXG series:
+ *    src -> 0 -> 1 -> 5 -> out
+ *    src -> 2 -> 3 -> 5 -> out
+ */
+
+/* algorithm for div0 + div1: rate = freq / 4 / (2 ^ N) */
+static struct clk_fixed_factor meson_spicc_div0 = {
+	.mult	= 1,
+	.div	= 4,
+};
+
+static struct clk_divider meson_spicc_div1 = {
+	.reg	= (void *)SPICC_CONREG,
+	.shift	= 16,
+	.width	= 3,
+	.flags	= CLK_DIVIDER_POWER_OF_TWO,
+};
+
+/* algorithm for div2 + div3: rate = freq / 2 / (N + 1) */
+static struct clk_fixed_factor meson_spicc_div2 = {
+	.mult	= 1,
+	.div	= 2,
+};
+
+static struct clk_divider meson_spicc_div3 = {
+	.reg	= (void *)SPICC_ENH_CTL0,
+	.shift	= 16,
+	.width	= 8,
+};
+
+static struct clk_mux meson_spicc_sel = {
+	.reg	= (void *)SPICC_ENH_CTL0,
+	.mask	= 0x1,
+	.shift	= 24,
+};
+
+static int meson_spicc_clk_init(struct meson_spicc_device *spicc)
+{
+	struct device *dev = &spicc->pdev->dev;
+	struct clk_fixed_factor *div0;
+	struct clk_divider *div1;
+	struct clk_mux *mux;
+	struct clk_init_data init;
+	struct clk *clk;
+	const char *parent_names[1];
+	const char *mux_parent_names[2];
+	char name[32];
+
+	div0 = &meson_spicc_div0;
+	snprintf(name, sizeof(name), "%s#_div0", dev_name(dev));
+	init.name = name;
+	init.ops = &clk_fixed_factor_ops;
+	init.flags = 0;
+	parent_names[0] = __clk_get_name(spicc->core);
+	init.parent_names = parent_names;
+	init.num_parents = 1;
+
+	div0->hw.init = &init;
+
+	clk = devm_clk_register(dev, &div0->hw);
+	if (WARN_ON(IS_ERR(clk)))
+		return PTR_ERR(clk);
+
+	div1 = &meson_spicc_div1;
+	snprintf(name, sizeof(name), "%s#_div1", dev_name(dev));
+	init.name = name;
+	init.ops = &clk_divider_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	parent_names[0] = __clk_get_name(clk);
+	init.parent_names = parent_names;
+	init.num_parents = 1;
+
+	div1->reg = spicc->base + (u64)div1->reg;
+	div1->hw.init = &init;
+
+	clk = devm_clk_register(dev, &div1->hw);
+	if (WARN_ON(IS_ERR(clk)))
+		return PTR_ERR(clk);
+
+	if (!spicc->data->has_enhance_clk_div) {
+		spicc->clk = clk;
+		return 0;
+	}
+
+	mux_parent_names[0] = __clk_get_name(clk);
+
+	div0 = &meson_spicc_div2;
+	snprintf(name, sizeof(name), "%s#_div2", dev_name(dev));
+	init.name = name;
+	init.ops = &clk_fixed_factor_ops;
+	init.flags = 0;
+	parent_names[0] = __clk_get_name(spicc->core);
+	init.parent_names = parent_names;
+	init.num_parents = 1;
+
+	div0->hw.init = &init;
+
+	clk = devm_clk_register(dev, &div0->hw);
+	if (WARN_ON(IS_ERR(clk)))
+		return PTR_ERR(clk);
+
+	div1 = &meson_spicc_div3;
+	snprintf(name, sizeof(name), "%s#_div3", dev_name(dev));
+	init.name = name;
+	init.ops = &clk_divider_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	parent_names[0] = __clk_get_name(clk);
+	init.parent_names = parent_names;
+	init.num_parents = 1;
+
+	div1->reg = spicc->base + (u64)div1->reg;
+	div1->hw.init = &init;
+
+	clk = devm_clk_register(dev, &div1->hw);
+	if (WARN_ON(IS_ERR(clk)))
+		return PTR_ERR(clk);
+
+	mux_parent_names[1] = __clk_get_name(clk);
+
+	mux = &meson_spicc_sel;
+	snprintf(name, sizeof(name), "%s#_sel", dev_name(dev));
+	init.name = name;
+	init.ops = &clk_mux_ops;
+	init.parent_names = mux_parent_names;
+	init.num_parents = 2;
+	init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT;
+
+	mux->reg = spicc->base + (u64)mux->reg;
+	mux->hw.init = &init;
+
+	spicc->clk = devm_clk_register(dev, &mux->hw);
+	if (WARN_ON(IS_ERR(spicc->clk)))
+		return PTR_ERR(spicc->clk);
+
+	clk_set_parent(spicc->clk, clk);
+	return 0;
+}
+
 static int meson_spicc_probe(struct platform_device *pdev)
 {
 	struct spi_master *master;
@@ -557,6 +675,10 @@ static int meson_spicc_probe(struct platform_device *pdev)
 		goto out_master;
 	}
 
+	/* Set master mode and enable controller */
+	writel_relaxed(SPICC_ENABLE | SPICC_MODE_MASTER,
+		       spicc->base + SPICC_CONREG);
+
 	/* Disable all IRQs */
 	writel_relaxed(0, spicc->base + SPICC_INTREG);
 
@@ -603,6 +725,12 @@ static int meson_spicc_probe(struct platform_device *pdev)
 	master->max_speed_hz = min_t(unsigned int, rate >> 1,
 				     spicc->data->max_speed_hz);
 
+	ret = meson_spicc_clk_init(spicc);
+	if (ret) {
+		dev_err(&pdev->dev, "clock registration failed\n");
+		goto out_master;
+	}
+
 	ret = devm_spi_register_master(&pdev->dev, master);
 	if (ret) {
 		dev_err(&pdev->dev, "spi master registration failed\n");
@@ -639,6 +767,7 @@ static const struct meson_spicc_data meson_spicc_gx_data = {
 static const struct meson_spicc_data meson_spicc_axg_data = {
 	.max_speed_hz		= 80000000,
 	.has_oen		= true,
+	.has_enhance_clk_div	= true,
 };
 
 static const struct of_device_id meson_spicc_of_match[] = {
-- 
2.7.4

Re: [PATCH v2 3/3] spi: meson-axg: add a linear clock divider support

[Neil Armstrong]

Hi Sunny,

On 13/12/2018 09:39, Sunny Luo wrote:
> The SPICC controller in Meson-AXG SoC is capable of using
> a linear clock divider to reach a much fine tuned range of clocks,
> while the old controller only use a power of two clock divider,
> result at a more coarse clock range.

This patch should definitely go before patch 1.

> 
> Also convert the clock registration into Common Clock Framework.
> 
> Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  drivers/spi/Kconfig           |   2 +-
>  drivers/spi/spi-meson-spicc.c | 209 ++++++++++++++++++++++++++++++++++--------
>  2 files changed, 170 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 9f89cb1..75f7362 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -384,7 +384,7 @@ config SPI_FSL_ESPI
>  
>  config SPI_MESON_SPICC
>  	tristate "Amlogic Meson SPICC controller"
> -	depends on ARCH_MESON || COMPILE_TEST
> +	depends on OF && COMMON_CLK && (ARCH_MESON || COMPILE_TEST)
>  	help
>  	  This enables master mode support for the SPICC (SPI communication
>  	  controller) available in Amlogic Meson SoCs.
> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> index 0384c28..59af18e 100644
> --- a/drivers/spi/spi-meson-spicc.c
> +++ b/drivers/spi/spi-meson-spicc.c
> @@ -116,6 +116,9 @@
>  #define SPICC_DWADDR	0x24	/* Write Address of DMA */
>  
>  #define SPICC_ENH_CTL0	0x38	/* Enhanced Feature */
> +#define SPICC_ENH_CLK_CS_DELAY_MASK	GENMASK(15, 0)
> +#define SPICC_ENH_DATARATE_MASK		GENMASK(23, 16)
> +#define SPICC_ENH_DATARATE_EN		BIT(24)
>  #define SPICC_ENH_MOSI_OEN		BIT(25)
>  #define SPICC_ENH_CLK_OEN		BIT(26)
>  #define SPICC_ENH_CS_OEN		BIT(27)
> @@ -131,6 +134,7 @@
>  struct meson_spicc_data {
>  	unsigned int			max_speed_hz;
>  	bool				has_oen;
> +	bool				has_enhance_clk_div;
>  };
>  
>  struct meson_spicc_device {
> @@ -138,6 +142,7 @@ struct meson_spicc_device {
>  	struct platform_device		*pdev;
>  	void __iomem			*base;
>  	struct clk			*core;
> +	struct clk			*clk;
>  	struct spi_message		*message;
>  	struct spi_transfer		*xfer;
>  	const struct meson_spicc_data	*data;
> @@ -325,40 +330,6 @@ static irqreturn_t meson_spicc_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static u32 meson_spicc_setup_speed(struct meson_spicc_device *spicc, u32 conf,
> -				   u32 speed)
> -{
> -	unsigned long parent, value;
> -	unsigned int i, div;
> -
> -	parent = clk_get_rate(spicc->core);
> -
> -	/* Find closest inferior/equal possible speed */
> -	for (i = 0 ; i < 7 ; ++i) {
> -		/* 2^(data_rate+2) */
> -		value = parent >> (i + 2);
> -
> -		if (value <= speed)
> -			break;
> -	}
> -
> -	/* If provided speed it lower than max divider, use max divider */
> -	if (i > 7) {
> -		div = 7;
> -		dev_warn_once(&spicc->pdev->dev, "unable to get close to speed %u\n",
> -			      speed);
> -	} else
> -		div = i;
> -
> -	dev_dbg(&spicc->pdev->dev, "parent %lu, speed %u -> %lu (%u)\n",
> -		parent, speed, value, div);
> -
> -	conf &= ~SPICC_DATARATE_MASK;
> -	conf |= FIELD_PREP(SPICC_DATARATE_MASK, div);
> -
> -	return conf;
> -}
> -
>  static void meson_spicc_setup_xfer(struct meson_spicc_device *spicc,
>  				   struct spi_transfer *xfer)
>  {
> @@ -367,9 +338,6 @@ static void meson_spicc_setup_xfer(struct meson_spicc_device *spicc,
>  	/* Read original configuration */
>  	conf = conf_orig = readl_relaxed(spicc->base + SPICC_CONREG);
>  
> -	/* Select closest divider */
> -	conf = meson_spicc_setup_speed(spicc, conf, xfer->speed_hz);
> -
>  	/* Setup word width */
>  	conf &= ~SPICC_BITLENGTH_MASK;
>  	conf |= FIELD_PREP(SPICC_BITLENGTH_MASK,
> @@ -378,6 +346,8 @@ static void meson_spicc_setup_xfer(struct meson_spicc_device *spicc,
>  	/* Ignore if unchanged */
>  	if (conf != conf_orig)
>  		writel_relaxed(conf, spicc->base + SPICC_CONREG);
> +
> +	clk_set_rate(spicc->clk, xfer->speed_hz);
>  }
>  
>  static int meson_spicc_transfer_one(struct spi_master *master,
> @@ -486,9 +456,6 @@ static int meson_spicc_unprepare_transfer(struct spi_master *master)
>  	/* Disable all IRQs */
>  	writel(0, spicc->base + SPICC_INTREG);
>  
> -	/* Disable controller */
> -	writel_bits_relaxed(SPICC_ENABLE, 0, spicc->base + SPICC_CONREG);
> -
>  	device_reset_optional(&spicc->pdev->dev);
>  
>  	return 0;
> @@ -528,6 +495,157 @@ static void meson_spicc_cleanup(struct spi_device *spi)
>  	spi->controller_state = NULL;
>  }
>  
> +/*
> + * The Clock Mux
> + *            x-----------------x   x------------x    x------\
> + *        |---| 0) fixed factor |---| 1) old div |----|      |
> + *        |   x-----------------x   x------------x    |      |
> + * src ---|                                           |5) mux|-- out
> + *        |   x-----------------x   x------------x    |      |
> + *        |---| 2) fixed factor |---| 3) new div |0---|      |
> + *            x-----------------x   x------------x    x------/
> + *
> + * Clk path for GX series:
> + *    src -> 0 -> 1 -> out
> + *
> + * Clk path for AXG series:
> + *    src -> 0 -> 1 -> 5 -> out
> + *    src -> 2 -> 3 -> 5 -> out
> + */
> +
> +/* algorithm for div0 + div1: rate = freq / 4 / (2 ^ N) */
> +static struct clk_fixed_factor meson_spicc_div0 = {
> +	.mult	= 1,
> +	.div	= 4,
> +};
> +
> +static struct clk_divider meson_spicc_div1 = {
> +	.reg	= (void *)SPICC_CONREG,
> +	.shift	= 16,
> +	.width	= 3,
> +	.flags	= CLK_DIVIDER_POWER_OF_TWO,
> +};
> +
> +/* algorithm for div2 + div3: rate = freq / 2 / (N + 1) */
> +static struct clk_fixed_factor meson_spicc_div2 = {
> +	.mult	= 1,
> +	.div	= 2,
> +};
> +
> +static struct clk_divider meson_spicc_div3 = {
> +	.reg	= (void *)SPICC_ENH_CTL0,
> +	.shift	= 16,
> +	.width	= 8,
> +};
> +
> +static struct clk_mux meson_spicc_sel = {
> +	.reg	= (void *)SPICC_ENH_CTL0,
> +	.mask	= 0x1,
> +	.shift	= 24,
> +};
> +
> +static int meson_spicc_clk_init(struct meson_spicc_device *spicc)
> +{
> +	struct device *dev = &spicc->pdev->dev;
> +	struct clk_fixed_factor *div0;
> +	struct clk_divider *div1;
> +	struct clk_mux *mux;
> +	struct clk_init_data init;
> +	struct clk *clk;
> +	const char *parent_names[1];
> +	const char *mux_parent_names[2];
> +	char name[32];
> +
> +	div0 = &meson_spicc_div0;
> +	snprintf(name, sizeof(name), "%s#_div0", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_fixed_factor_ops;
> +	init.flags = 0;
> +	parent_names[0] = __clk_get_name(spicc->core);
> +	init.parent_names = parent_names;
> +	init.num_parents = 1;
> +
> +	div0->hw.init = &init;
> +
> +	clk = devm_clk_register(dev, &div0->hw);
> +	if (WARN_ON(IS_ERR(clk)))
> +		return PTR_ERR(clk);
> +
> +	div1 = &meson_spicc_div1;
> +	snprintf(name, sizeof(name), "%s#_div1", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_divider_ops;
> +	init.flags = CLK_SET_RATE_PARENT;
> +	parent_names[0] = __clk_get_name(clk);
> +	init.parent_names = parent_names;
> +	init.num_parents = 1;
> +
> +	div1->reg = spicc->base + (u64)div1->reg;
> +	div1->hw.init = &init;
> +
> +	clk = devm_clk_register(dev, &div1->hw);
> +	if (WARN_ON(IS_ERR(clk)))
> +		return PTR_ERR(clk);
> +
> +	if (!spicc->data->has_enhance_clk_div) {
> +		spicc->clk = clk;
> +		return 0;
> +	}
> +
> +	mux_parent_names[0] = __clk_get_name(clk);
> +
> +	div0 = &meson_spicc_div2;
> +	snprintf(name, sizeof(name), "%s#_div2", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_fixed_factor_ops;
> +	init.flags = 0;
> +	parent_names[0] = __clk_get_name(spicc->core);
> +	init.parent_names = parent_names;
> +	init.num_parents = 1;
> +
> +	div0->hw.init = &init;
> +
> +	clk = devm_clk_register(dev, &div0->hw);
> +	if (WARN_ON(IS_ERR(clk)))
> +		return PTR_ERR(clk);
> +
> +	div1 = &meson_spicc_div3;
> +	snprintf(name, sizeof(name), "%s#_div3", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_divider_ops;
> +	init.flags = CLK_SET_RATE_PARENT;
> +	parent_names[0] = __clk_get_name(clk);
> +	init.parent_names = parent_names;
> +	init.num_parents = 1;
> +
> +	div1->reg = spicc->base + (u64)div1->reg;
> +	div1->hw.init = &init;
> +
> +	clk = devm_clk_register(dev, &div1->hw);
> +	if (WARN_ON(IS_ERR(clk)))
> +		return PTR_ERR(clk);
> +
> +	mux_parent_names[1] = __clk_get_name(clk);
> +
> +	mux = &meson_spicc_sel;
> +	snprintf(name, sizeof(name), "%s#_sel", dev_name(dev));
> +	init.name = name;
> +	init.ops = &clk_mux_ops;
> +	init.parent_names = mux_parent_names;
> +	init.num_parents = 2;
> +	init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT;
> +
> +	mux->reg = spicc->base + (u64)mux->reg;
> +	mux->hw.init = &init;
> +
> +	spicc->clk = devm_clk_register(dev, &mux->hw);
> +	if (WARN_ON(IS_ERR(spicc->clk)))
> +		return PTR_ERR(spicc->clk);
> +
> +	clk_set_parent(spicc->clk, clk);
> +	return 0;
> +}

I'll let Jerome review this !

> +
>  static int meson_spicc_probe(struct platform_device *pdev)
>  {
>  	struct spi_master *master;
> @@ -557,6 +675,10 @@ static int meson_spicc_probe(struct platform_device *pdev)
>  		goto out_master;
>  	}
>  
> +	/* Set master mode and enable controller */
> +	writel_relaxed(SPICC_ENABLE | SPICC_MODE_MASTER,
> +		       spicc->base + SPICC_CONREG);

Please remove it from meson_spicc_prepare_message() now.

> +
>  	/* Disable all IRQs */
>  	writel_relaxed(0, spicc->base + SPICC_INTREG);
>  
> @@ -603,6 +725,12 @@ static int meson_spicc_probe(struct platform_device *pdev)
>  	master->max_speed_hz = min_t(unsigned int, rate >> 1,
>  				     spicc->data->max_speed_hz);
>  
> +	ret = meson_spicc_clk_init(spicc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "clock registration failed\n");
> +		goto out_master;
> +	}
> +
>  	ret = devm_spi_register_master(&pdev->dev, master);
>  	if (ret) {
>  		dev_err(&pdev->dev, "spi master registration failed\n");
> @@ -639,6 +767,7 @@ static const struct meson_spicc_data meson_spicc_gx_data = {
>  static const struct meson_spicc_data meson_spicc_axg_data = {
>  	.max_speed_hz		= 80000000,
>  	.has_oen		= true,
> +	.has_enhance_clk_div	= true,
>  };
>  
>  static const struct of_device_id meson_spicc_of_match[] = {
> 

Re: [PATCH v2 3/3] spi: meson-axg: add a linear clock divider support

[Jerome Brunet]

On Thu, 2018-12-13 at 09:55 +0100, Neil Armstrong wrote:
> Hi Sunny,
> 
> On 13/12/2018 09:39, Sunny Luo wrote:
> > The SPICC controller in Meson-AXG SoC is capable of using
> > a linear clock divider to reach a much fine tuned range of clocks,
> > while the old controller only use a power of two clock divider,
> > result at a more coarse clock range.
> 
> This patch should definitely go before patch 1.
> 
> > Also convert the clock registration into Common Clock Framework.
> > 
> > Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
> > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> > ---
> >  drivers/spi/Kconfig           |   2 +-
> >  drivers/spi/spi-meson-spicc.c | 209 ++++++++++++++++++++++++++++++++++---
> > -----
> >  2 files changed, 170 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index 9f89cb1..75f7362 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -384,7 +384,7 @@ config SPI_FSL_ESPI
> >  
> >  config SPI_MESON_SPICC
> >  	tristate "Amlogic Meson SPICC controller"
> > -	depends on ARCH_MESON || COMPILE_TEST
> > +	depends on OF && COMMON_CLK && (ARCH_MESON || COMPILE_TEST)

The purpose of this patch is clock, right ? Why does it add a dependency on OF
?

> >  	help
> >  	  This enables master mode support for the SPICC (SPI communication
> >  	  controller) available in Amlogic Meson SoCs.
> > diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> > index 0384c28..59af18e 100644
> > --- a/drivers/spi/spi-meson-spicc.c
> > +++ b/drivers/spi/spi-meson-spicc.c
> > @@ -116,6 +116,9 @@
> >  #define SPICC_DWADDR	0x24	/* Write Address of DMA */
> >  
> >  #define SPICC_ENH_CTL0	0x38	/* Enhanced Feature */
> > +#define SPICC_ENH_CLK_CS_DELAY_MASK	GENMASK(15, 0)
> > +#define SPICC_ENH_DATARATE_MASK		GENMASK(23, 16)
> > +#define SPICC_ENH_DATARATE_EN		BIT(24)
> >  #define SPICC_ENH_MOSI_OEN		BIT(25)
> >  #define SPICC_ENH_CLK_OEN		BIT(26)
> >  #define SPICC_ENH_CS_OEN		BIT(27)
> > @@ -131,6 +134,7 @@
> >  struct meson_spicc_data {
> >  	unsigned int			max_speed_hz;
> >  	bool				has_oen;
> > +	bool				has_enhance_clk_div;
> >  };
> >  
> >  struct meson_spicc_device {
> > @@ -138,6 +142,7 @@ struct meson_spicc_device {
> >  	struct platform_device		*pdev;
> >  	void __iomem			*base;
> >  	struct clk			*core;
> > +	struct clk			*clk;
> >  	struct spi_message		*message;
> >  	struct spi_transfer		*xfer;
> >  	const struct meson_spicc_data	*data;
> > @@ -325,40 +330,6 @@ static irqreturn_t meson_spicc_irq(int irq, void
> > *data)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > -static u32 meson_spicc_setup_speed(struct meson_spicc_device *spicc, u32
> > conf,
> > -				   u32 speed)
> > -{
> > -	unsigned long parent, value;
> > -	unsigned int i, div;
> > -
> > -	parent = clk_get_rate(spicc->core);
> > -
> > -	/* Find closest inferior/equal possible speed */
> > -	for (i = 0 ; i < 7 ; ++i) {
> > -		/* 2^(data_rate+2) */
> > -		value = parent >> (i + 2);
> > -
> > -		if (value <= speed)
> > -			break;
> > -	}
> > -
> > -	/* If provided speed it lower than max divider, use max divider */
> > -	if (i > 7) {
> > -		div = 7;
> > -		dev_warn_once(&spicc->pdev->dev, "unable to get close to speed
> > %u\n",
> > -			      speed);
> > -	} else
> > -		div = i;
> > -
> > -	dev_dbg(&spicc->pdev->dev, "parent %lu, speed %u -> %lu (%u)\n",
> > -		parent, speed, value, div);
> > -
> > -	conf &= ~SPICC_DATARATE_MASK;
> > -	conf |= FIELD_PREP(SPICC_DATARATE_MASK, div);
> > -
> > -	return conf;
> > -}
> > -
> >  static void meson_spicc_setup_xfer(struct meson_spicc_device *spicc,
> >  				   struct spi_transfer *xfer)
> >  {
> > @@ -367,9 +338,6 @@ static void meson_spicc_setup_xfer(struct
> > meson_spicc_device *spicc,
> >  	/* Read original configuration */
> >  	conf = conf_orig = readl_relaxed(spicc->base + SPICC_CONREG);
> >  
> > -	/* Select closest divider */
> > -	conf = meson_spicc_setup_speed(spicc, conf, xfer->speed_hz);
> > -
> >  	/* Setup word width */
> >  	conf &= ~SPICC_BITLENGTH_MASK;
> >  	conf |= FIELD_PREP(SPICC_BITLENGTH_MASK,
> > @@ -378,6 +346,8 @@ static void meson_spicc_setup_xfer(struct
> > meson_spicc_device *spicc,
> >  	/* Ignore if unchanged */
> >  	if (conf != conf_orig)
> >  		writel_relaxed(conf, spicc->base + SPICC_CONREG);
> > +
> > +	clk_set_rate(spicc->clk, xfer->speed_hz);
> >  }
> >  
> >  static int meson_spicc_transfer_one(struct spi_master *master,
> > @@ -486,9 +456,6 @@ static int meson_spicc_unprepare_transfer(struct
> > spi_master *master)
> >  	/* Disable all IRQs */
> >  	writel(0, spicc->base + SPICC_INTREG);
> >  
> > -	/* Disable controller */
> > -	writel_bits_relaxed(SPICC_ENABLE, 0, spicc->base + SPICC_CONREG);
> > -
> >  	device_reset_optional(&spicc->pdev->dev);
> >  
> >  	return 0;
> > @@ -528,6 +495,157 @@ static void meson_spicc_cleanup(struct spi_device
> > *spi)
> >  	spi->controller_state = NULL;
> >  }
> >  
> > +/*
> > + * The Clock Mux
> > + *            x-----------------x   x------------x    x------\
> > + *        |---| 0) fixed factor |---| 1) old div |----|      |
> > + *        |   x-----------------x   x------------x    |      |
> > + * src ---|                                           |5) mux|-- out
> > + *        |   x-----------------x   x------------x    |      |
> > + *        |---| 2) fixed factor |---| 3) new div |0---|      |
> > + *            x-----------------x   x------------x    x------/
> > + *
> > + * Clk path for GX series:
> > + *    src -> 0 -> 1 -> out
> > + *
> > + * Clk path for AXG series:
> > + *    src -> 0 -> 1 -> 5 -> out
> > + *    src -> 2 -> 3 -> 5 -> out
> > + */
> > +
> > +/* algorithm for div0 + div1: rate = freq / 4 / (2 ^ N) */
> > +static struct clk_fixed_factor meson_spicc_div0 = {
> > +	.mult	= 1,
> > +	.div	= 4,
> > +};
> > +
> > +static struct clk_divider meson_spicc_div1 = {
> > +	.reg	= (void *)SPICC_CONREG,
> > +	.shift	= 16,
> > +	.width	= 3,
> > +	.flags	= CLK_DIVIDER_POWER_OF_TWO,
> > +};
> > +
> > +/* algorithm for div2 + div3: rate = freq / 2 / (N + 1) */
> > +static struct clk_fixed_factor meson_spicc_div2 = {
> > +	.mult	= 1,
> > +	.div	= 2,
> > +};
> > +
> > +static struct clk_divider meson_spicc_div3 = {
> > +	.reg	= (void *)SPICC_ENH_CTL0,
> > +	.shift	= 16,
> > +	.width	= 8,
> > +};
> > +
> > +static struct clk_mux meson_spicc_sel = {
> > +	.reg	= (void *)SPICC_ENH_CTL0,
> > +	.mask	= 0x1,
> > +	.shift	= 24,
> > +};
> > +
> > +static int meson_spicc_clk_init(struct meson_spicc_device *spicc)
> > +{
> > +	struct device *dev = &spicc->pdev->dev;
> > +	struct clk_fixed_factor *div0;
> > +	struct clk_divider *div1;

Could you come up with something better than div0 and div1 ? it is confusing,
especially with the comment above about div3 and 4 ... fixed_factor, div maybe
?

> > +	struct clk_mux *mux;
> > +	struct clk_init_data init;
> > +	struct clk *clk;
> > +	const char *parent_names[1];
> > +	const char *mux_parent_names[2];
> > +	char name[32];
> > +
> > +	div0 = &meson_spicc_div0;
> > +	snprintf(name, sizeof(name), "%s#_div0", dev_name(dev));
> > +	init.name = name;
> > +	init.ops = &clk_fixed_factor_ops;
> > +	init.flags = 0;
> > +	parent_names[0] = __clk_get_name(spicc->core);
> > +	init.parent_names = parent_names;
> > +	init.num_parents = 1;
> > +
> > +	div0->hw.init = &init;
> > +
> > +	clk = devm_clk_register(dev, &div0->hw);
> > +	if (WARN_ON(IS_ERR(clk)))
> > +		return PTR_ERR(clk);
> > +
> > +	div1 = &meson_spicc_div1;
> > +	snprintf(name, sizeof(name), "%s#_div1", dev_name(dev));
> > +	init.name = name;
> > +	init.ops = &clk_divider_ops;
> > +	init.flags = CLK_SET_RATE_PARENT;
> > +	parent_names[0] = __clk_get_name(clk);
> > +	init.parent_names = parent_names;
> > +	init.num_parents = 1;
> > +
> > +	div1->reg = spicc->base + (u64)div1->reg;

So you have static data which you override here. This works only if there is a
single instance ... and does not really improve readability in your case.

IMO, you'd be better off without the static data above.

> > +	div1->hw.init = &init;
> > +
> > +	clk = devm_clk_register(dev, &div1->hw);
> > +	if (WARN_ON(IS_ERR(clk)))
> > +		return PTR_ERR(clk);
> > +
> > +	if (!spicc->data->has_enhance_clk_div) {

Do all SoC with 'has_oen' have 'has_enhance_clk_div' too ?
DO you really need two flags ?

> > +		spicc->clk = clk;
> > +		return 0;
> > +	}
> > +
> > +	mux_parent_names[0] = __clk_get_name(clk);
> > +
> > +	div0 = &meson_spicc_div2;
> > +	snprintf(name, sizeof(name), "%s#_div2", dev_name(dev));
> > +	init.name = name;
> > +	init.ops = &clk_fixed_factor_ops;
> > +	init.flags = 0;
> > +	parent_names[0] = __clk_get_name(spicc->core);
> > +	init.parent_names = parent_names;
> > +	init.num_parents = 1;
> > +
> > +	div0->hw.init = &init;
> > +
> > +	clk = devm_clk_register(dev, &div0->hw);
> > +	if (WARN_ON(IS_ERR(clk)))
> > +		return PTR_ERR(clk);
> > +
> > +	div1 = &meson_spicc_div3;
> > +	snprintf(name, sizeof(name), "%s#_div3", dev_name(dev));
> > +	init.name = name;
> > +	init.ops = &clk_divider_ops;
> > +	init.flags = CLK_SET_RATE_PARENT;
> > +	parent_names[0] = __clk_get_name(clk);
> > +	init.parent_names = parent_names;
> > +	init.num_parents = 1;
> > +
> > +	div1->reg = spicc->base + (u64)div1->reg;
> > +	div1->hw.init = &init;
> > +
> > +	clk = devm_clk_register(dev, &div1->hw);
> > +	if (WARN_ON(IS_ERR(clk)))
> > +		return PTR_ERR(clk);
> > +
> > +	mux_parent_names[1] = __clk_get_name(clk);
> > +
> > +	mux = &meson_spicc_sel;
> > +	snprintf(name, sizeof(name), "%s#_sel", dev_name(dev));
> > +	init.name = name;
> > +	init.ops = &clk_mux_ops;
> > +	init.parent_names = mux_parent_names;
> > +	init.num_parents = 2;
> > +	init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT;

Why CLK_SET_RATE_NO_REPARENT ? CCF should pick the parent that give the best
results, best to let it do its task ...

> > +
> > +	mux->reg = spicc->base + (u64)mux->reg;
> > +	mux->hw.init = &init;
> > +
> > +	spicc->clk = devm_clk_register(dev, &mux->hw);
> > +	if (WARN_ON(IS_ERR(spicc->clk)))
> > +		return PTR_ERR(spicc->clk);
> > +
> > +	clk_set_parent(spicc->clk, clk);

... then you can drop this.

> > +	return 0;
> > +}
> 
> I'll let Jerome review this !
> 
> > +
> >  static int meson_spicc_probe(struct platform_device *pdev)
> >  {
> >  	struct spi_master *master;
> > @@ -557,6 +675,10 @@ static int meson_spicc_probe(struct platform_device
> > *pdev)
> >  		goto out_master;
> >  	}
> >  
> > +	/* Set master mode and enable controller */
> > +	writel_relaxed(SPICC_ENABLE | SPICC_MODE_MASTER,
> > +		       spicc->base + SPICC_CONREG);
> 
> Please remove it from meson_spicc_prepare_message() now.
> 
> > +
> >  	/* Disable all IRQs */
> >  	writel_relaxed(0, spicc->base + SPICC_INTREG);
> >  
> > @@ -603,6 +725,12 @@ static int meson_spicc_probe(struct platform_device
> > *pdev)
> >  	master->max_speed_hz = min_t(unsigned int, rate >> 1,
> >  				     spicc->data->max_speed_hz);
> >  
> > +	ret = meson_spicc_clk_init(spicc);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "clock registration failed\n");
> > +		goto out_master;
> > +	}
> > +
> >  	ret = devm_spi_register_master(&pdev->dev, master);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "spi master registration failed\n");
> > @@ -639,6 +767,7 @@ static const struct meson_spicc_data
> > meson_spicc_gx_data = {
> >  static const struct meson_spicc_data meson_spicc_axg_data = {
> >  	.max_speed_hz		= 80000000,
> >  	.has_oen		= true,
> > +	.has_enhance_clk_div	= true,
> >  };
> >  
> >  static const struct of_device_id meson_spicc_of_match[] = {
> > 

Re: [PATCH v2 3/3] spi: meson-axg: add a linear clock divider support

[Sunny Luo]

Hi Jerome,

On 2018/12/13 17:28, Jerome Brunet wrote:
> On Thu, 2018-12-13 at 09:55 +0100, Neil Armstrong wrote:

>>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>>>   config SPI_MESON_SPICC
>>>   	tristate "Amlogic Meson SPICC controller"
>>> -	depends on ARCH_MESON || COMPILE_TEST
>>> +	depends on OF && COMMON_CLK && (ARCH_MESON || COMPILE_TEST)
> 
> The purpose of this patch is clock, right ? Why does it add a dependency on OF
> ?
> I did it by the way. Maybe it's better to add it in patch 1.


>>> +static int meson_spicc_clk_init(struct meson_spicc_device *spicc)
>>> +{
>>> +	struct device *dev = &spicc->pdev->dev;
>>> +	struct clk_fixed_factor *div0;
>>> +	struct clk_divider *div1;
> 
> Could you come up with something better than div0 and div1 ? it is confusing,
> especially with the comment above about div3 and 4 ... fixed_factor, div maybe
> ?
> Good, It is really confusing, I will change next patch.


>>> +	div1 = &meson_spicc_div1;
>>> +	div1->reg = spicc->base + (u64)div1->reg;
> 
> So you have static data which you override here. This works only if there is a
> single instance ... and does not really improve readability in your case.
> 
> IMO, you'd be better off without the static data above.

This is a terrible bug for more than one instances. I must overwrite it.


>>> +	if (!spicc->data->has_enhance_clk_div) {
> 
> Do all SoC with 'has_oen' have 'has_enhance_clk_div' too ?
> DO you really need two flags ?

Yes, all Soc with 'has_oen' must have 'has_enhance_clk_div' too.
It is distinct to use two flags here, what do you think of it?

>>> +	mux = &meson_spicc_sel;
>>> +	snprintf(name, sizeof(name), "%s#_sel", dev_name(dev));
>>> +	init.name = name;
>>> +	init.ops = &clk_mux_ops;
>>> +	init.parent_names = mux_parent_names;
>>> +	init.num_parents = 2;
>>> +	init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT;
> 
> Why CLK_SET_RATE_NO_REPARENT ? CCF should pick the parent that give the best
> results, best to let it do its task ...
> 
There are two div in AXG, one is the coarse old-div and the other is 
enhance-div. From SoCs designer's opinion, the ehance-div aims to take 
place of the old-div.

Re: [PATCH v2 3/3] spi: meson-axg: add a linear clock divider support

[Jerome Brunet]

On Thu, 2018-12-13 at 22:37 +0800, Sunny Luo wrote:
> Hi Jerome,
> 
> On 2018/12/13 17:28, Jerome Brunet wrote:
> > On Thu, 2018-12-13 at 09:55 +0100, Neil Armstrong wrote:
> > > > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > > >   config SPI_MESON_SPICC
> > > >   	tristate "Amlogic Meson SPICC controller"
> > > > -	depends on ARCH_MESON || COMPILE_TEST
> > > > +	depends on OF && COMMON_CLK && (ARCH_MESON || COMPILE_TEST)
> > 
> > The purpose of this patch is clock, right ? Why does it add a dependency
> > on OF
> > ?
> > I did it by the way. Maybe it's better to add it in patch 1.
> > > > +static int meson_spicc_clk_init(struct meson_spicc_device *spicc)
> > > > +{
> > > > +	struct device *dev = &spicc->pdev->dev;
> > > > +	struct clk_fixed_factor *div0;
> > > > +	struct clk_divider *div1;
> > 
> > Could you come up with something better than div0 and div1 ? it is
> > confusing,
> > especially with the comment above about div3 and 4 ... fixed_factor, div
> > maybe
> > ?
> > Good, It is really confusing, I will change next patch.
> > > > +	div1 = &meson_spicc_div1;
> > > > +	div1->reg = spicc->base + (u64)div1->reg;
> > 
> > So you have static data which you override here. This works only if there
> > is a
> > single instance ... and does not really improve readability in your case.
> > 
> > IMO, you'd be better off without the static data above.
> 
> This is a terrible bug for more than one instances. I must overwrite it.

You must set them properly, yes ... having these static data is not necessary

> 
> 
> > > > +	if (!spicc->data->has_enhance_clk_div) {
> > 
> > Do all SoC with 'has_oen' have 'has_enhance_clk_div' too ?
> > DO you really need two flags ?
> 
> Yes, all Soc with 'has_oen' must have 'has_enhance_clk_div' too.
> It is distinct to use two flags here, what do you think of it?

I wonder if you should be using of_device_is_compatible() instead of adding
these flags.

> 
> > > > +	mux = &meson_spicc_sel;
> > > > +	snprintf(name, sizeof(name), "%s#_sel", dev_name(dev));
> > > > +	init.name = name;
> > > > +	init.ops = &clk_mux_ops;
> > > > +	init.parent_names = mux_parent_names;
> > > > +	init.num_parents = 2;
> > > > +	init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT;
> > 
> > Why CLK_SET_RATE_NO_REPARENT ? CCF should pick the parent that give the
> > best
> > results, best to let it do its task ...
> > 
> There are two div in AXG, one is the coarse old-div and the other is 
> enhance-div. From SoCs designer's opinion, the ehance-div aims to take 
> place of the old-div.

... and all likelyhood, CCF will pick it BUT, unless the old path is broken,
please let the framework do its job. 

If the old path was broken you should not have described it ... but we have
been using it so far, so we know it works fine.

it's a very simple fix: drop CLK_SET_RATE_NO_REPARENT and your call
clk_set_parent()

> 
> 

Last but not least, I did not see it in my initial review, but:

I see you call clk_set_rate(spicc->clk, ...) but I don't see any call to
clk_prepare_enable() and clk_disable_unprepare().

It works because the input clock and your local tree does not gate, but it is
wrong. A driver should not assume that they clock is enabled by default.

Re: [PATCH v2 3/3] spi: meson-axg: add a linear clock divider support

[Sunny Luo]

Hi Neil,

On 2018/12/13 16:55, Neil Armstrong wrote:
> Hi Sunny,
> 
> On 13/12/2018 09:39, Sunny Luo wrote:
>> The SPICC controller in Meson-AXG SoC is capable of using
>> a linear clock divider to reach a much fine tuned range of clocks,
>> while the old controller only use a power of two clock divider,
>> result at a more coarse clock range.
> 
> This patch should definitely go before patch 1.
Would you please show the reason?
> 
>>  
>> +	/* Set master mode and enable controller */
>> +	writel_relaxed(SPICC_ENABLE | SPICC_MODE_MASTER,
>> +		       spicc->base + SPICC_CONREG);
> 
> Please remove it from meson_spicc_prepare_message() now.
>
Yes, I moved it here and forgot remove it at prepare_message().

Re: [PATCH v2 3/3] spi: meson-axg: add a linear clock divider support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5019 bytes --]

Hi Sunny,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on spi/for-next]
[also build test WARNING on v4.20-rc6 next-20181214]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sunny-Luo/spi-meson-axg-support-MAX-80M-clock/20181214-175627
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   drivers/spi/spi-meson-spicc.c: In function 'meson_spicc_clk_init':
>> drivers/spi/spi-meson-spicc.c:583:28: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     div1->reg = spicc->base + (u64)div1->reg;
                               ^
   drivers/spi/spi-meson-spicc.c:621:28: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     div1->reg = spicc->base + (u64)div1->reg;
                               ^
   drivers/spi/spi-meson-spicc.c:638:27: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
     mux->reg = spicc->base + (u64)mux->reg;
                              ^

vim +583 drivers/spi/spi-meson-spicc.c

   546	
   547	static int meson_spicc_clk_init(struct meson_spicc_device *spicc)
   548	{
   549		struct device *dev = &spicc->pdev->dev;
   550		struct clk_fixed_factor *div0;
   551		struct clk_divider *div1;
   552		struct clk_mux *mux;
   553		struct clk_init_data init;
   554		struct clk *clk;
   555		const char *parent_names[1];
   556		const char *mux_parent_names[2];
   557		char name[32];
   558	
   559		div0 = &meson_spicc_div0;
   560		snprintf(name, sizeof(name), "%s#_div0", dev_name(dev));
   561		init.name = name;
   562		init.ops = &clk_fixed_factor_ops;
   563		init.flags = 0;
   564		parent_names[0] = __clk_get_name(spicc->core);
   565		init.parent_names = parent_names;
   566		init.num_parents = 1;
   567	
   568		div0->hw.init = &init;
   569	
   570		clk = devm_clk_register(dev, &div0->hw);
   571		if (WARN_ON(IS_ERR(clk)))
   572			return PTR_ERR(clk);
   573	
   574		div1 = &meson_spicc_div1;
   575		snprintf(name, sizeof(name), "%s#_div1", dev_name(dev));
   576		init.name = name;
   577		init.ops = &clk_divider_ops;
   578		init.flags = CLK_SET_RATE_PARENT;
   579		parent_names[0] = __clk_get_name(clk);
   580		init.parent_names = parent_names;
   581		init.num_parents = 1;
   582	
 > 583		div1->reg = spicc->base + (u64)div1->reg;
   584		div1->hw.init = &init;
   585	
   586		clk = devm_clk_register(dev, &div1->hw);
   587		if (WARN_ON(IS_ERR(clk)))
   588			return PTR_ERR(clk);
   589	
   590		if (!spicc->data->has_enhance_clk_div) {
   591			spicc->clk = clk;
   592			return 0;
   593		}
   594	
   595		mux_parent_names[0] = __clk_get_name(clk);
   596	
   597		div0 = &meson_spicc_div2;
   598		snprintf(name, sizeof(name), "%s#_div2", dev_name(dev));
   599		init.name = name;
   600		init.ops = &clk_fixed_factor_ops;
   601		init.flags = 0;
   602		parent_names[0] = __clk_get_name(spicc->core);
   603		init.parent_names = parent_names;
   604		init.num_parents = 1;
   605	
   606		div0->hw.init = &init;
   607	
   608		clk = devm_clk_register(dev, &div0->hw);
   609		if (WARN_ON(IS_ERR(clk)))
   610			return PTR_ERR(clk);
   611	
   612		div1 = &meson_spicc_div3;
   613		snprintf(name, sizeof(name), "%s#_div3", dev_name(dev));
   614		init.name = name;
   615		init.ops = &clk_divider_ops;
   616		init.flags = CLK_SET_RATE_PARENT;
   617		parent_names[0] = __clk_get_name(clk);
   618		init.parent_names = parent_names;
   619		init.num_parents = 1;
   620	
   621		div1->reg = spicc->base + (u64)div1->reg;
   622		div1->hw.init = &init;
   623	
   624		clk = devm_clk_register(dev, &div1->hw);
   625		if (WARN_ON(IS_ERR(clk)))
   626			return PTR_ERR(clk);
   627	
   628		mux_parent_names[1] = __clk_get_name(clk);
   629	
   630		mux = &meson_spicc_sel;
   631		snprintf(name, sizeof(name), "%s#_sel", dev_name(dev));
   632		init.name = name;
   633		init.ops = &clk_mux_ops;
   634		init.parent_names = mux_parent_names;
   635		init.num_parents = 2;
   636		init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT;
   637	
   638		mux->reg = spicc->base + (u64)mux->reg;
   639		mux->hw.init = &init;
   640	
   641		spicc->clk = devm_clk_register(dev, &mux->hw);
   642		if (WARN_ON(IS_ERR(spicc->clk)))
   643			return PTR_ERR(spicc->clk);
   644	
   645		clk_set_parent(spicc->clk, clk);
   646		return 0;
   647	}
   648	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55675 bytes --]

[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/3] spi: meson-axg: add few enhanced features

[Neil Armstrong]

Hi Sunny,

On 13/12/2018 09:39, Sunny Luo wrote:
> add a few enhanced features for the SPICC controller of Meson-AXG SoC.
> 
> These patches are actually quite independent from each other, I send them
> together in case to avoid the file conflicts.
> 
> Changes since v1 at [1]
> - Add OF and COMMON_CLK dependence for SPICC in Kconfig to avoid compiling
>   error.
> 
> [1] https://lore.kernel.org/lkml/20180503213645.20694-1-yixun.lan-LpR1jeaWuhtBDgjK7y7TUQ@public.gmane.org
> 
> Sunny Luo (3):
>   spi: meson-axg: support MAX 80M clock
>   spi: meson-axg: enhance output enable feature
>   spi: meson-axg: add a linear clock divider support
> 
>  drivers/spi/Kconfig           |   2 +-
>  drivers/spi/spi-meson-spicc.c | 270 ++++++++++++++++++++++++++++++++++--------
>  2 files changed, 223 insertions(+), 49 deletions(-)
> 

Is there still a plan to push an updated version for AXG and G12A ?

Neil