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

[U-Boot] [PATCH v6 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 v5->v6:
  * Put the clock handle into the private data

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 v6 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.

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 v6 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) 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 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 | 43 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 5aa507b..a037376 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -11,6 +11,7 @@
  */
 
 #include <common.h>
+#include <clk.h>
 #include <dm.h>
 #include <errno.h>
 #include <malloc.h>
@@ -18,7 +19,6 @@
 #include <fdtdec.h>
 #include <linux/compat.h>
 #include <asm/io.h>
-#include <asm/arch/clock_manager.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -94,6 +94,8 @@ struct dw_spi_priv {
 	void __iomem *regs;
 	unsigned int freq;		/* Default frequency */
 	unsigned int mode;
+	struct clk clk;
+	unsigned long bus_clk_rate;
 
 	int bits_per_word;
 	u8 cs;			/* chip select pin */
@@ -176,14 +178,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 dw_spi_priv *priv = dev_get_priv(bus);
+	int ret;
+
+	ret = clk_get_by_index(bus, 0, &priv->clk);
+	if (ret)
+		return -EINVAL;
+
+	ret = clk_enable(&priv->clk);
+	if (ret && ret != -ENOSYS && ret != -ENOSYS && ret != -ENOTSUPP)
+		return ret;
+
+	*rate = clk_get_rate(&priv->clk);
+	if (!*rate) {
+		clk_disable(&priv->clk);
+		return -EINVAL;
+	}
+
+	debug("%s: get spi controller clk via device tree: %lu Hz\n",
+	      __func__, *rate);
+
+	clk_free(&priv->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;
 
@@ -369,7 +408,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_writel(priv, DW_SPI_BAUDR, clk_div);
 
-- 
2.9.3

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

[Marek Vasut]

On 12/11/2017 05:18 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) 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 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 | 43 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> index 5aa507b..a037376 100644
> --- a/drivers/spi/designware_spi.c
> +++ b/drivers/spi/designware_spi.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include <common.h>
> +#include <clk.h>
>  #include <dm.h>
>  #include <errno.h>
>  #include <malloc.h>
> @@ -18,7 +19,6 @@
>  #include <fdtdec.h>
>  #include <linux/compat.h>
>  #include <asm/io.h>
> -#include <asm/arch/clock_manager.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -94,6 +94,8 @@ struct dw_spi_priv {
>  	void __iomem *regs;
>  	unsigned int freq;		/* Default frequency */
>  	unsigned int mode;
> +	struct clk clk;
> +	unsigned long bus_clk_rate;
>  
>  	int bits_per_word;
>  	u8 cs;			/* chip select pin */
> @@ -176,14 +178,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 dw_spi_priv *priv = dev_get_priv(bus);
> +	int ret;
> +
> +	ret = clk_get_by_index(bus, 0, &priv->clk);
> +	if (ret)
> +		return -EINVAL;

if (ret)
 return ret;

> +	ret = clk_enable(&priv->clk);
> +	if (ret && ret != -ENOSYS && ret != -ENOSYS && ret != -ENOTSUPP)

You have ENOSYS twice in there, but you can do just if (ret) return ret
again.

> +		return ret;
> +
> +	*rate = clk_get_rate(&priv->clk);
> +	if (!*rate) {
> +		clk_disable(&priv->clk);

You didn't do clk_free() here, just implement a failpath (ie. goto
err_rate).

> +		return -EINVAL;
> +	}
> +
> +	debug("%s: get spi controller clk via device tree: %lu Hz\n",
> +	      __func__, *rate);
> +
> +	clk_free(&priv->clk);

If anyone accesses priv->clk outside of this function, the code will
crash, so remove this.

> +	return 0;

err_rate:
 clk_free() ...

> +}
> +
>  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;
>  
> @@ -369,7 +408,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_writel(priv, DW_SPI_BAUDR, clk_div);
>  
> 


-- 
Best regards,
Marek Vasut

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

[Eugeniy Paltsev]

On Mon, 2017-12-11 at 17:21 +0100, Marek Vasut wrote:
> On 12/11/2017 05:18 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) 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 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 | 43 +++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 41 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> > index 5aa507b..a037376 100644
> > --- a/drivers/spi/designware_spi.c
> > +++ b/drivers/spi/designware_spi.c
> > @@ -11,6 +11,7 @@
> >   */
> >  
> >  #include <common.h>
> > +#include <clk.h>
> >  #include <dm.h>
> >  #include <errno.h>
> >  #include <malloc.h>
> > @@ -18,7 +19,6 @@
> >  #include <fdtdec.h>
> >  #include <linux/compat.h>
> >  #include <asm/io.h>
> > -#include <asm/arch/clock_manager.h>
> >  
> >  DECLARE_GLOBAL_DATA_PTR;
> >  
> > @@ -94,6 +94,8 @@ struct dw_spi_priv {
> >  	void __iomem *regs;
> >  	unsigned int freq;		/* Default frequency */
> >  	unsigned int mode;
> > +	struct clk clk;
> > +	unsigned long bus_clk_rate;
> >  
> >  	int bits_per_word;
> >  	u8 cs;			/* chip select pin */
> > @@ -176,14 +178,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 dw_spi_priv *priv = dev_get_priv(bus);
> > +	int ret;
> > +
> > +	ret = clk_get_by_index(bus, 0, &priv->clk);
> > +	if (ret)
> > +		return -EINVAL;
> 
> if (ret)
>  return ret;
Ok.

> 
> > +	ret = clk_enable(&priv->clk);
> > +	if (ret && ret != -ENOSYS && ret != -ENOSYS && ret != -ENOTSUPP)
> 
> You have ENOSYS twice in there, but you can do just if (ret) return ret
> again.

The idea is to skip error if error code is -ENOSYS or -ENOTSUPP
as it is OK situation: some clock drivers don't implement .enable/.disable
callbacks so clk_enable returns -ENOSYS.
Also some clock drivers implement .enable/.disable callbacks not for all
clock IDs and return -ENOSYS or -ENOTSUPP for others.

But definitely I should remove second ENOSYS here.

> > +		return ret;
> > +
> > +	*rate = clk_get_rate(&priv->clk);
> > +	if (!*rate) {
> > +		clk_disable(&priv->clk);
> 
> You didn't do clk_free() here, just implement a failpath (ie. goto
> err_rate).
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	debug("%s: get spi controller clk via device tree: %lu Hz\n",
> > +	      __func__, *rate);
> > +
> > +	clk_free(&priv->clk);
> 
> If anyone accesses priv->clk outside of this function, the code will
> crash, so remove this.
> 
> > +	return 0;
> 
> err_rate:
>  clk_free() ...

Sure, thanks.

> 
> > +}
> > +
> >  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;
> >  
> > @@ -369,7 +408,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_writel(priv, DW_SPI_BAUDR, clk_div);
> >  
> > 
> 
> 
-- 
 Eugeniy Paltsev

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

[Marek Vasut]

On 12/11/2017 05:37 PM, Eugeniy Paltsev wrote:
> On Mon, 2017-12-11 at 17:21 +0100, Marek Vasut wrote:
>> On 12/11/2017 05:18 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) 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 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 | 43 +++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 41 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
>>> index 5aa507b..a037376 100644
>>> --- a/drivers/spi/designware_spi.c
>>> +++ b/drivers/spi/designware_spi.c
>>> @@ -11,6 +11,7 @@
>>>   */
>>>  
>>>  #include <common.h>
>>> +#include <clk.h>
>>>  #include <dm.h>
>>>  #include <errno.h>
>>>  #include <malloc.h>
>>> @@ -18,7 +19,6 @@
>>>  #include <fdtdec.h>
>>>  #include <linux/compat.h>
>>>  #include <asm/io.h>
>>> -#include <asm/arch/clock_manager.h>
>>>  
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>  
>>> @@ -94,6 +94,8 @@ struct dw_spi_priv {
>>>  	void __iomem *regs;
>>>  	unsigned int freq;		/* Default frequency */
>>>  	unsigned int mode;
>>> +	struct clk clk;
>>> +	unsigned long bus_clk_rate;
>>>  
>>>  	int bits_per_word;
>>>  	u8 cs;			/* chip select pin */
>>> @@ -176,14 +178,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 dw_spi_priv *priv = dev_get_priv(bus);
>>> +	int ret;
>>> +
>>> +	ret = clk_get_by_index(bus, 0, &priv->clk);
>>> +	if (ret)
>>> +		return -EINVAL;
>>
>> if (ret)
>>  return ret;
> Ok.
> 
>>
>>> +	ret = clk_enable(&priv->clk);
>>> +	if (ret && ret != -ENOSYS && ret != -ENOSYS && ret != -ENOTSUPP)
>>
>> You have ENOSYS twice in there, but you can do just if (ret) return ret
>> again.
> 
> The idea is to skip error if error code is -ENOSYS or -ENOTSUPP
> as it is OK situation: some clock drivers don't implement .enable/.disable
> callbacks so clk_enable returns -ENOSYS.
> Also some clock drivers implement .enable/.disable callbacks not for all
> clock IDs and return -ENOSYS or -ENOTSUPP for others.
> 
> But definitely I should remove second ENOSYS here.
+CC Simon, that's somewhat iffy ...

-- 
Best regards,
Marek Vasut

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

[Eugeniy Paltsev]

On Mon, 2017-12-11 at 17:41 +0100, Marek Vasut wrote:
> On 12/11/2017 05:37 PM, Eugeniy Paltsev wrote:
> > On Mon, 2017-12-11 at 17:21 +0100, Marek Vasut wrote:
> > > On 12/11/2017 05:18 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) 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 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 | 43 +++++++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 41 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> > > > index 5aa507b..a037376 100644
> > > > --- a/drivers/spi/designware_spi.c
> > > > +++ b/drivers/spi/designware_spi.c
> > > > @@ -11,6 +11,7 @@
> > > >   */
> > > >  
> > > >  #include <common.h>
> > > > +#include <clk.h>
> > > >  #include <dm.h>
> > > >  #include <errno.h>
> > > >  #include <malloc.h>
> > > > @@ -18,7 +19,6 @@
> > > >  #include <fdtdec.h>
> > > >  #include <linux/compat.h>
> > > >  #include <asm/io.h>
> > > > -#include <asm/arch/clock_manager.h>
> > > >  
> > > >  DECLARE_GLOBAL_DATA_PTR;
> > > >  
> > > > @@ -94,6 +94,8 @@ struct dw_spi_priv {
> > > >  	void __iomem *regs;
> > > >  	unsigned int freq;		/* Default frequency */
> > > >  	unsigned int mode;
> > > > +	struct clk clk;
> > > > +	unsigned long bus_clk_rate;
> > > >  
> > > >  	int bits_per_word;
> > > >  	u8 cs;			/* chip select pin */
> > > > @@ -176,14 +178,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 dw_spi_priv *priv = dev_get_priv(bus);
> > > > +	int ret;
> > > > +
> > > > +	ret = clk_get_by_index(bus, 0, &priv->clk);
> > > > +	if (ret)
> > > > +		return -EINVAL;
> > > 
> > > if (ret)
> > >  return ret;
> > 
> > Ok.
> > 
> > > 
> > > > +	ret = clk_enable(&priv->clk);
> > > > +	if (ret && ret != -ENOSYS && ret != -ENOSYS && ret != -ENOTSUPP)
> > > 
> > > You have ENOSYS twice in there, but you can do just if (ret) return ret
> > > again.
> > 
> > The idea is to skip error if error code is -ENOSYS or -ENOTSUPP
> > as it is OK situation: some clock drivers don't implement .enable/.disable
> > callbacks so clk_enable returns -ENOSYS.
> > Also some clock drivers implement .enable/.disable callbacks not for all
> > clock IDs and return -ENOSYS or -ENOTSUPP for others.
> > 
> > But definitely I should remove second ENOSYS here.
> 
> +CC Simon, that's somewhat iffy ...

Looks like it is quite common approach, I can see it in some uboot drivers:

For example: drivers/mmc/zynq_sdhci.c
--------------->8------------
	ret = clk_enable(&clk);
	if (ret && ret != -ENOSYS) {
		dev_err(dev, "failed to enable clock\n");
		return ret;
	}
--------------->8------------

-- 
 Eugeniy Paltsev

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

[Simon Glass]

On 11 December 2017 at 09:41, Marek Vasut <marex@denx.de> wrote:
> On 12/11/2017 05:37 PM, Eugeniy Paltsev wrote:
>> On Mon, 2017-12-11 at 17:21 +0100, Marek Vasut wrote:
>>> On 12/11/2017 05:18 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) 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 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 | 43 +++++++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 41 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
>>>> index 5aa507b..a037376 100644
>>>> --- a/drivers/spi/designware_spi.c
>>>> +++ b/drivers/spi/designware_spi.c
>>>> @@ -11,6 +11,7 @@
>>>>   */
>>>>
>>>>  #include <common.h>
>>>> +#include <clk.h>
>>>>  #include <dm.h>
>>>>  #include <errno.h>
>>>>  #include <malloc.h>
>>>> @@ -18,7 +19,6 @@
>>>>  #include <fdtdec.h>
>>>>  #include <linux/compat.h>
>>>>  #include <asm/io.h>
>>>> -#include <asm/arch/clock_manager.h>
>>>>
>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>
>>>> @@ -94,6 +94,8 @@ struct dw_spi_priv {
>>>>     void __iomem *regs;
>>>>     unsigned int freq;              /* Default frequency */
>>>>     unsigned int mode;
>>>> +   struct clk clk;
>>>> +   unsigned long bus_clk_rate;
>>>>
>>>>     int bits_per_word;
>>>>     u8 cs;                  /* chip select pin */
>>>> @@ -176,14 +178,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 dw_spi_priv *priv = dev_get_priv(bus);
>>>> +   int ret;
>>>> +
>>>> +   ret = clk_get_by_index(bus, 0, &priv->clk);
>>>> +   if (ret)
>>>> +           return -EINVAL;
>>>
>>> if (ret)
>>>  return ret;
>> Ok.
>>
>>>
>>>> +   ret = clk_enable(&priv->clk);
>>>> +   if (ret && ret != -ENOSYS && ret != -ENOSYS && ret != -ENOTSUPP)
>>>
>>> You have ENOSYS twice in there, but you can do just if (ret) return ret
>>> again.
>>
>> The idea is to skip error if error code is -ENOSYS or -ENOTSUPP
>> as it is OK situation: some clock drivers don't implement .enable/.disable
>> callbacks so clk_enable returns -ENOSYS.
>> Also some clock drivers implement .enable/.disable callbacks not for all
>> clock IDs and return -ENOSYS or -ENOTSUPP for others.
>>
>> But definitely I should remove second ENOSYS here.
> +CC Simon, that's somewhat iffy ...

I think it is OK.

Regards,
Simon