[U-Boot] [PATCH v5 0/2] DW SPI: Get clock value from Device Tree

[U-Boot] [PATCH v5 0/2] DW SPI: Get clock value from Device Tree

[Eugeniy Paltsev]

As discussed with Marek during the LINUX-PITER here is v4 patch:

Add option to set spi controller clock frequency via device tree
using standard clock bindings.

Define dw_spi_get_clk function as 'weak' as some targets
(like SOCFPGA_GEN5 and SOCFPGA_ARRIA10) fon't use standard clock API
and implement dw_spi_get_clk their own way in their clock manager.

Get rid of clock_manager.h include in designware_spi.c as we don't use
cm_get_spi_controller_clk_hz function anymore - we use redefined
dw_spi_get_clk in SOCFPGA clock managers (clock_manager_gen5.c and
clock_manager_arria10.c) instead.

Changes v4->v5:
  * Get rid of usless ifdef in dw_spi_get_clk function

Eugeniy Paltsev (2):
  SOCFPGA: clock manager: implement dw_spi_get_clk function
  DW SPI: Get clock value from Device Tree

 arch/arm/mach-socfpga/clock_manager_arria10.c |  9 +++++
 arch/arm/mach-socfpga/clock_manager_gen5.c    |  9 +++++
 drivers/spi/designware_spi.c                  | 47 +++++++++++++++++++++++++--
 3 files changed, 63 insertions(+), 2 deletions(-)

-- 
2.9.3

[U-Boot] [PATCH v5 1/2] SOCFPGA: clock manager: implement dw_spi_get_clk function

[Eugeniy Paltsev]

Implement dw_spi_get_clk function to override its weak
implementation in designware_spi.c driver.

We need this change to get rid of cm_get_spi_controller_clk_hz
function and clock_manager.h include in designware_spi.c driver.

Reviewed-by: Marek Vasut <marex@denx.de>
Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 arch/arm/mach-socfpga/clock_manager_arria10.c | 9 +++++++++
 arch/arm/mach-socfpga/clock_manager_gen5.c    | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/arm/mach-socfpga/clock_manager_arria10.c b/arch/arm/mach-socfpga/clock_manager_arria10.c
index 482b854..623a266 100644
--- a/arch/arm/mach-socfpga/clock_manager_arria10.c
+++ b/arch/arm/mach-socfpga/clock_manager_arria10.c
@@ -7,6 +7,7 @@
 #include <common.h>
 #include <fdtdec.h>
 #include <asm/io.h>
+#include <dm.h>
 #include <asm/arch/clock_manager.h>
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -1076,6 +1077,14 @@ unsigned int cm_get_qspi_controller_clk_hz(void)
 	return  cm_get_l4_noc_hz(CLKMGR_MAINPLL_NOCDIV_L4MAINCLK_LSB);
 }
 
+/* Override weak dw_spi_get_clk implementation in designware_spi.c driver */
+int dw_spi_get_clk(struct udevice *bus, ulong *rate)
+{
+	*rate = cm_get_spi_controller_clk_hz();
+
+	return 0;
+}
+
 void cm_print_clock_quick_summary(void)
 {
 	printf("MPU       %10ld kHz\n", cm_get_mpu_clk_hz() / 1000);
diff --git a/arch/arm/mach-socfpga/clock_manager_gen5.c b/arch/arm/mach-socfpga/clock_manager_gen5.c
index 31fd510..a371d83 100644
--- a/arch/arm/mach-socfpga/clock_manager_gen5.c
+++ b/arch/arm/mach-socfpga/clock_manager_gen5.c
@@ -6,6 +6,7 @@
 
 #include <common.h>
 #include <asm/io.h>
+#include <dm.h>
 #include <asm/arch/clock_manager.h>
 #include <wait_bit.h>
 
@@ -509,6 +510,14 @@ unsigned int cm_get_spi_controller_clk_hz(void)
 	return clock;
 }
 
+/* Override weak dw_spi_get_clk implementation in designware_spi.c driver */
+int dw_spi_get_clk(struct udevice *bus, ulong *rate)
+{
+	*rate = cm_get_spi_controller_clk_hz();
+
+	return 0;
+}
+
 void cm_print_clock_quick_summary(void)
 {
 	printf("MPU       %10ld kHz\n", cm_get_mpu_clk_hz() / 1000);
-- 
2.9.3

[U-Boot] [PATCH v5 2/2] DW SPI: Get clock value from Device Tree

[Eugeniy Paltsev]

Add option to set spi controller clock frequency via device tree
using standard clock bindings.

Define dw_spi_get_clk function as 'weak' as some targets
(like SOCFPGA_GEN5 and SOCFPGA_ARRIA10) don't use standard clock API
and implement dw_spi_get_clk their own way in their clock manager.

Get rid of clock_manager.h include as we don't use
cm_get_spi_controller_clk_hz function anymore. (we use redefined
dw_spi_get_clk in SOCFPGA clock managers instead)

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 drivers/spi/designware_spi.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 6cc4f51..470a3a7 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -12,6 +12,7 @@
 
 #include <asm-generic/gpio.h>
 #include <common.h>
+#include <clk.h>
 #include <dm.h>
 #include <errno.h>
 #include <malloc.h>
@@ -19,7 +20,6 @@
 #include <fdtdec.h>
 #include <linux/compat.h>
 #include <asm/io.h>
-#include <asm/arch/clock_manager.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -95,6 +95,7 @@ struct dw_spi_priv {
 	void __iomem *regs;
 	unsigned int freq;		/* Default frequency */
 	unsigned int mode;
+	unsigned long bus_clk_rate;
 
 	struct gpio_desc cs_gpio;	/* External chip-select gpio */
 
@@ -195,14 +196,51 @@ static void spi_hw_init(struct dw_spi_priv *priv)
 	debug("%s: fifo_len=%d\n", __func__, priv->fifo_len);
 }
 
+/*
+ * We define dw_spi_get_clk function as 'weak' as some targets
+ * (like SOCFPGA_GEN5 and SOCFPGA_ARRIA10) fon't use standard clock API
+ * and implement dw_spi_get_clk their own way in their clock manager.
+ */
+__weak int dw_spi_get_clk(struct udevice *bus, ulong *rate)
+{
+	struct clk clk;
+	int ret;
+
+	ret = clk_get_by_index(bus, 0, &clk);
+	if (ret)
+		return -EINVAL;
+
+	ret = clk_enable(&clk);
+	if (ret && ret != -ENOSYS)
+		return ret;
+
+	*rate = clk_get_rate(&clk);
+	if (!*rate) {
+		clk_disable(&clk);
+		return -EINVAL;
+	}
+
+	debug("%s: get spi controller clk via device tree: %lu Hz\n",
+	      __func__, *rate);
+
+	clk_free(&clk);
+
+	return 0;
+}
+
 static int dw_spi_probe(struct udevice *bus)
 {
 	struct dw_spi_platdata *plat = dev_get_platdata(bus);
 	struct dw_spi_priv *priv = dev_get_priv(bus);
+	int ret;
 
 	priv->regs = plat->regs;
 	priv->freq = plat->frequency;
 
+	ret = dw_spi_get_clk(bus, &priv->bus_clk_rate);
+	if (ret)
+		return ret;
+
 	/* Currently only bits_per_word == 8 supported */
 	priv->bits_per_word = 8;
 
@@ -411,7 +449,7 @@ static int dw_spi_set_speed(struct udevice *bus, uint speed)
 	spi_enable_chip(priv, 0);
 
 	/* clk_div doesn't support odd number */
-	clk_div = cm_get_spi_controller_clk_hz() / speed;
+	clk_div = priv->bus_clk_rate / speed;
 	clk_div = (clk_div + 1) & 0xfffe;
 	dw_write(priv, DW_SPI_BAUDR, clk_div);
 
-- 
2.9.3

[U-Boot] [PATCH v5 2/2] DW SPI: Get clock value from Device Tree

[Marek Vasut]

On 11/14/2017 04:33 PM, Eugeniy Paltsev wrote:
> Add option to set spi controller clock frequency via device tree
> using standard clock bindings.
> 
> Define dw_spi_get_clk function as 'weak' as some targets
> (like SOCFPGA_GEN5 and SOCFPGA_ARRIA10) don't use standard clock API
> and implement dw_spi_get_clk their own way in their clock manager.
> 
> Get rid of clock_manager.h include as we don't use
> cm_get_spi_controller_clk_hz function anymore. (we use redefined
> dw_spi_get_clk in SOCFPGA clock managers instead)
> 
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
>  drivers/spi/designware_spi.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> index 6cc4f51..470a3a7 100644
> --- a/drivers/spi/designware_spi.c
> +++ b/drivers/spi/designware_spi.c
> @@ -12,6 +12,7 @@
>  
>  #include <asm-generic/gpio.h>
>  #include <common.h>
> +#include <clk.h>
>  #include <dm.h>
>  #include <errno.h>
>  #include <malloc.h>
> @@ -19,7 +20,6 @@
>  #include <fdtdec.h>
>  #include <linux/compat.h>
>  #include <asm/io.h>
> -#include <asm/arch/clock_manager.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -95,6 +95,7 @@ struct dw_spi_priv {
>  	void __iomem *regs;
>  	unsigned int freq;		/* Default frequency */
>  	unsigned int mode;
> +	unsigned long bus_clk_rate;
>  
>  	struct gpio_desc cs_gpio;	/* External chip-select gpio */
>  
> @@ -195,14 +196,51 @@ static void spi_hw_init(struct dw_spi_priv *priv)
>  	debug("%s: fifo_len=%d\n", __func__, priv->fifo_len);
>  }
>  
> +/*
> + * We define dw_spi_get_clk function as 'weak' as some targets
> + * (like SOCFPGA_GEN5 and SOCFPGA_ARRIA10) fon't use standard clock API
> + * and implement dw_spi_get_clk their own way in their clock manager.
> + */
> +__weak int dw_spi_get_clk(struct udevice *bus, ulong *rate)
> +{
> +	struct clk clk;
> +	int ret;
> +
> +	ret = clk_get_by_index(bus, 0, &clk);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = clk_enable(&clk);
> +	if (ret && ret != -ENOSYS)
> +		return ret;
> +
> +	*rate = clk_get_rate(&clk);
> +	if (!*rate) {
> +		clk_disable(&clk);
> +		return -EINVAL;
> +	}
> +
> +	debug("%s: get spi controller clk via device tree: %lu Hz\n",
> +	      __func__, *rate);
> +
> +	clk_free(&clk);

You probably want to retain the handle to these clock in the private
data, since otherwise you won't be able to turn the clock off in
.remove() callback of the driver (if/once it's implemented)

Otherwise the patch look good, thanks !

> +	return 0;
> +}
> +
>  static int dw_spi_probe(struct udevice *bus)
>  {
>  	struct dw_spi_platdata *plat = dev_get_platdata(bus);
>  	struct dw_spi_priv *priv = dev_get_priv(bus);
> +	int ret;
>  
>  	priv->regs = plat->regs;
>  	priv->freq = plat->frequency;
>  
> +	ret = dw_spi_get_clk(bus, &priv->bus_clk_rate);
> +	if (ret)
> +		return ret;
> +
>  	/* Currently only bits_per_word == 8 supported */
>  	priv->bits_per_word = 8;
>  
> @@ -411,7 +449,7 @@ static int dw_spi_set_speed(struct udevice *bus, uint speed)
>  	spi_enable_chip(priv, 0);
>  
>  	/* clk_div doesn't support odd number */
> -	clk_div = cm_get_spi_controller_clk_hz() / speed;
> +	clk_div = priv->bus_clk_rate / speed;
>  	clk_div = (clk_div + 1) & 0xfffe;
>  	dw_write(priv, DW_SPI_BAUDR, clk_div);
>  
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [uboot-snps-arc] Re: [PATCH v5 2/2] DW SPI: Get clock value from Device Tree

[Eugeniy Paltsev]

On Wed, 2017-11-15 at 10:24 +0100, Marek Vasut wrote:
> On 11/14/2017 04:33 PM, Eugeniy Paltsev wrote:
> > Add option to set spi controller clock frequency via device tree
> > using standard clock bindings.
> > 
> > Define dw_spi_get_clk function as 'weak' as some targets
> > (like SOCFPGA_GEN5 and SOCFPGA_ARRIA10) don't use standard clock API
> > and implement dw_spi_get_clk their own way in their clock manager.
> > 
> > Get rid of clock_manager.h include as we don't use
> > cm_get_spi_controller_clk_hz function anymore. (we use redefined
> > dw_spi_get_clk in SOCFPGA clock managers instead)
> > 
> > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> > ---
> >  drivers/spi/designware_spi.c | 42 ++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 40 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> > index 6cc4f51..470a3a7 100644
> > --- a/drivers/spi/designware_spi.c
> > +++ b/drivers/spi/designware_spi.c
> > @@ -12,6 +12,7 @@
> >  
> > +/*
> > + * We define dw_spi_get_clk function as 'weak' as some targets
> > + * (like SOCFPGA_GEN5 and SOCFPGA_ARRIA10) fon't use standard clock API
> > + * and implement dw_spi_get_clk their own way in their clock manager.
> > + */
> > +__weak int dw_spi_get_clk(struct udevice *bus, ulong *rate)
> > +{
> > +	struct clk clk;
> > +	int ret;
> > +
> > +	ret = clk_get_by_index(bus, 0, &clk);
> > +	if (ret)
> > +		return -EINVAL;
> > +
> > +	ret = clk_enable(&clk);
> > +	if (ret && ret != -ENOSYS)
> > +		return ret;
> > +
> > +	*rate = clk_get_rate(&clk);
> > +	if (!*rate) {
> > +		clk_disable(&clk);
> > +		return -EINVAL;
> > +	}
> > +
> > +	debug("%s: get spi controller clk via device tree: %lu Hz\n",
> > +	      __func__, *rate);
> > +
> > +	clk_free(&clk);
> 
> You probably want to retain the handle to these clock in the private
> data, since otherwise you won't be able to turn the clock off in
> .remove() callback of the driver (if/once it's implemented)

No, .remove() callback isn't implemented in this driver, so it isn't necessary.

> Otherwise the patch look good, thanks !
> 
> 
-- 
 Eugeniy Paltsev

[U-Boot] [uboot-snps-arc] Re: [PATCH v5 2/2] DW SPI: Get clock value from Device Tree

[Marek Vasut]

On 12/09/2017 04:23 PM, Eugeniy Paltsev wrote:
> On Wed, 2017-11-15 at 10:24 +0100, Marek Vasut wrote:
>> On 11/14/2017 04:33 PM, Eugeniy Paltsev wrote:
>>> Add option to set spi controller clock frequency via device tree
>>> using standard clock bindings.
>>>
>>> Define dw_spi_get_clk function as 'weak' as some targets
>>> (like SOCFPGA_GEN5 and SOCFPGA_ARRIA10) don't use standard clock API
>>> and implement dw_spi_get_clk their own way in their clock manager.
>>>
>>> Get rid of clock_manager.h include as we don't use
>>> cm_get_spi_controller_clk_hz function anymore. (we use redefined
>>> dw_spi_get_clk in SOCFPGA clock managers instead)
>>>
>>> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
>>> ---
>>>  drivers/spi/designware_spi.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
>>> index 6cc4f51..470a3a7 100644
>>> --- a/drivers/spi/designware_spi.c
>>> +++ b/drivers/spi/designware_spi.c
>>> @@ -12,6 +12,7 @@
>>>  
>>> +/*
>>> + * We define dw_spi_get_clk function as 'weak' as some targets
>>> + * (like SOCFPGA_GEN5 and SOCFPGA_ARRIA10) fon't use standard clock API
>>> + * and implement dw_spi_get_clk their own way in their clock manager.
>>> + */
>>> +__weak int dw_spi_get_clk(struct udevice *bus, ulong *rate)
>>> +{
>>> +	struct clk clk;
>>> +	int ret;
>>> +
>>> +	ret = clk_get_by_index(bus, 0, &clk);
>>> +	if (ret)
>>> +		return -EINVAL;
>>> +
>>> +	ret = clk_enable(&clk);
>>> +	if (ret && ret != -ENOSYS)
>>> +		return ret;
>>> +
>>> +	*rate = clk_get_rate(&clk);
>>> +	if (!*rate) {
>>> +		clk_disable(&clk);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	debug("%s: get spi controller clk via device tree: %lu Hz\n",
>>> +	      __func__, *rate);
>>> +
>>> +	clk_free(&clk);
>>
>> You probably want to retain the handle to these clock in the private
>> data, since otherwise you won't be able to turn the clock off in
>> .remove() callback of the driver (if/once it's implemented)
> 
> No, .remove() callback isn't implemented in this driver, so it isn't necessary.

Either implement it, or if you're lazy, put the clock handle into the
private data, since that's a good practice.

>> Otherwise the patch look good, thanks !
>>
>>


-- 
Best regards,
Marek Vasut