[U-Boot] [PATCH RESEND] DW SPI: Get clock value from Device Tree

[U-Boot] [PATCH RESEND] 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.
Old way of setting spi controller clock frequency (via implementation
of 'cm_get_spi_controller_clk_hz' function in platform specific code)
remains supported.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 Resending due to previously sent one was discarded by mailing list.

 drivers/spi/designware_spi.c | 68 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 5aa507b..c70697e 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>
@@ -94,6 +95,7 @@ struct dw_spi_priv {
 	void __iomem *regs;
 	unsigned int freq;		/* Default frequency */
 	unsigned int mode;
+	unsigned long bus_clk_rate;
 
 	int bits_per_word;
 	u8 cs;			/* chip select pin */
@@ -176,14 +178,78 @@ static void spi_hw_init(struct dw_spi_priv *priv)
 	debug("%s: fifo_len=%d\n", __func__, priv->fifo_len);
 }
 
+/*
+ * cm_get_spi_controller_clk_hz function is old way to set spi controller
+ * frequency. If it isn't implemented and spi controller frequency isn't set via
+ * device tree we will get into next default function.
+ */
+__weak unsigned int cm_get_spi_controller_clk_hz(void)
+{
+	error("SPI clock is defined neither via device tree nor via cm_get_spi_controller_clk_hz!");
+
+	return 0;
+}
+
+static int dw_spi_of_get_clk(struct udevice *bus)
+{
+#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK)
+	struct dw_spi_priv *priv = dev_get_priv(bus);
+	unsigned long clk_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;
+
+	clk_rate = clk_get_rate(&clk);
+	if (!clk_rate)
+		return -EINVAL;
+
+	priv->bus_clk_rate = clk_rate;
+
+	clk_free(&clk);
+
+	return 0;
+#endif
+
+	return -ENOSYS;
+}
+
+static int dw_spi_get_clk(struct udevice *bus)
+{
+	struct dw_spi_priv *priv = dev_get_priv(bus);
+
+	/* Firstly try to get clock frequency from device tree */
+	if (!dw_spi_of_get_clk(bus))
+		return 0;
+
+	/* In case of failure rollback to cm_get_spi_controller_clk_hz */
+	priv->bus_clk_rate = cm_get_spi_controller_clk_hz();
+
+	if (!priv->bus_clk_rate)
+		return -EINVAL;
+
+	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);
+	if (ret)
+		return ret;
+
 	/* Currently only bits_per_word == 8 supported */
 	priv->bits_per_word = 8;
 
@@ -369,7 +435,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 RESEND] DW SPI: Get clock value from Device Tree

[Eugeniy Paltsev]

Hi,

Maybe you have any comments or remarks about this patch? And if you don't could you please apply it. 

Thanks!

On Tue, 2017-09-26 at 16:10 +0300, Eugeniy Paltsev wrote:
> Add option to set spi controller clock frequency via device tree
> using standard clock bindings.
> Old way of setting spi controller clock frequency (via implementation
> of 'cm_get_spi_controller_clk_hz' function in platform specific code)
> remains supported.
> 
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
>  Resending due to previously sent one was discarded by mailing list.
> 
>  drivers/spi/designware_spi.c | 68 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> index 5aa507b..c70697e 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>
> @@ -94,6 +95,7 @@ struct dw_spi_priv {
>  	void __iomem *regs;
>  	unsigned int freq;		/* Default frequency */
>  	unsigned int mode;
> +	unsigned long bus_clk_rate;
>  
>  	int bits_per_word;
>  	u8 cs;			/* chip select pin */
> @@ -176,14 +178,78 @@ static void spi_hw_init(struct dw_spi_priv *priv)
>  	debug("%s: fifo_len=%d\n", __func__, priv->fifo_len);
>  }
>  
> +/*
> + * cm_get_spi_controller_clk_hz function is old way to set spi controller
> + * frequency. If it isn't implemented and spi controller frequency isn't set via
> + * device tree we will get into next default function.
> + */
> +__weak unsigned int cm_get_spi_controller_clk_hz(void)
> +{
> +	error("SPI clock is defined neither via device tree nor via cm_get_spi_controller_clk_hz!");
> +
> +	return 0;
> +}
> +
> +static int dw_spi_of_get_clk(struct udevice *bus)
> +{
> +#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK)
> +	struct dw_spi_priv *priv = dev_get_priv(bus);
> +	unsigned long clk_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;
> +
> +	clk_rate = clk_get_rate(&clk);
> +	if (!clk_rate)
> +		return -EINVAL;
> +
> +	priv->bus_clk_rate = clk_rate;
> +
> +	clk_free(&clk);
> +
> +	return 0;
> +#endif
> +
> +	return -ENOSYS;
> +}
> +
> +static int dw_spi_get_clk(struct udevice *bus)
> +{
> +	struct dw_spi_priv *priv = dev_get_priv(bus);
> +
> +	/* Firstly try to get clock frequency from device tree */
> +	if (!dw_spi_of_get_clk(bus))
> +		return 0;
> +
> +	/* In case of failure rollback to cm_get_spi_controller_clk_hz */
> +	priv->bus_clk_rate = cm_get_spi_controller_clk_hz();
> +
> +	if (!priv->bus_clk_rate)
> +		return -EINVAL;
> +
> +	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);
> +	if (ret)
> +		return ret;
> +
>  	/* Currently only bits_per_word == 8 supported */
>  	priv->bits_per_word = 8;
>  
> @@ -369,7 +435,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 RESEND] DW SPI: Get clock value from Device Tree

[Jagan Teki]

On Tue, Sep 26, 2017 at 6:40 PM, Eugeniy Paltsev
<Eugeniy.Paltsev@synopsys.com> wrote:
> Add option to set spi controller clock frequency via device tree
> using standard clock bindings.
> Old way of setting spi controller clock frequency (via implementation
> of 'cm_get_spi_controller_clk_hz' function in platform specific code)
> remains supported.
>
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
>  Resending due to previously sent one was discarded by mailing list.
>
>  drivers/spi/designware_spi.c | 68 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> index 5aa507b..c70697e 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>
> @@ -94,6 +95,7 @@ struct dw_spi_priv {
>         void __iomem *regs;
>         unsigned int freq;              /* Default frequency */
>         unsigned int mode;
> +       unsigned long bus_clk_rate;
>
>         int bits_per_word;
>         u8 cs;                  /* chip select pin */
> @@ -176,14 +178,78 @@ static void spi_hw_init(struct dw_spi_priv *priv)
>         debug("%s: fifo_len=%d\n", __func__, priv->fifo_len);
>  }
>
> +/*
> + * cm_get_spi_controller_clk_hz function is old way to set spi controller
> + * frequency. If it isn't implemented and spi controller frequency isn't set via
> + * device tree we will get into next default function.
> + */
> +__weak unsigned int cm_get_spi_controller_clk_hz(void)
> +{
> +       error("SPI clock is defined neither via device tree nor via cm_get_spi_controller_clk_hz!");
> +
> +       return 0;
> +}
> +
> +static int dw_spi_of_get_clk(struct udevice *bus)
> +{
> +#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK)
> +       struct dw_spi_priv *priv = dev_get_priv(bus);
> +       unsigned long clk_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;
> +
> +       clk_rate = clk_get_rate(&clk);
> +       if (!clk_rate)
> +               return -EINVAL;

Use bus_clk_rate instead of local clk_rate and disable the clk if it
can't get the rate.

> +
> +       priv->bus_clk_rate = clk_rate;
> +
> +       clk_free(&clk);
> +
> +       return 0;
> +#endif
> +
> +       return -ENOSYS;
> +}
> +
> +static int dw_spi_get_clk(struct udevice *bus)
> +{
> +       struct dw_spi_priv *priv = dev_get_priv(bus);
> +
> +       /* Firstly try to get clock frequency from device tree */
> +       if (!dw_spi_of_get_clk(bus))
> +               return 0;
> +
> +       /* In case of failure rollback to cm_get_spi_controller_clk_hz */
> +       priv->bus_clk_rate = cm_get_spi_controller_clk_hz();

Why weak function, we can just return with error message since weak
does the same job

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH RESEND] DW SPI: Get clock value from Device Tree

[Eugeniy Paltsev]

Hi Jagan,
Thanks for respond, my comments are given inline.

On Wed, 2017-10-11 at 16:36 +0530, Jagan Teki wrote:
> On Tue, Sep 26, 2017 at 6:40 PM, Eugeniy Paltsev
> <Eugeniy.Paltsev@synopsys.com> wrote:
> > Add option to set spi controller clock frequency via device tree
> > using standard clock bindings.
> > Old way of setting spi controller clock frequency (via implementation
> > of 'cm_get_spi_controller_clk_hz' function in platform specific code)
> > remains supported.
> > 
> > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> > ---
> > diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> > index 5aa507b..c70697e 100644
> > --- a/drivers/spi/designware_spi.c
> > +++ b/drivers/spi/designware_spi.c
> > @@ -11,6 +11,7 @@
> > +/*
> > + * cm_get_spi_controller_clk_hz function is old way to set spi controller
> > + * frequency. If it isn't implemented and spi controller frequency isn't set via
> > + * device tree we will get into next default function.
> > + */
> > +__weak unsigned int cm_get_spi_controller_clk_hz(void)
> > +{
> > +       error("SPI clock is defined neither via device tree nor via cm_get_spi_controller_clk_hz!");
> > +
> > +       return 0;
> > +}
> > +
> > +static int dw_spi_get_clk(struct udevice *bus)
> > +{
> > +       struct dw_spi_priv *priv = dev_get_priv(bus);
> > +
> > +       /* Firstly try to get clock frequency from device tree */
> > +       if (!dw_spi_of_get_clk(bus))
> > +               return 0;
> > +
> > +       /* In case of failure rollback to cm_get_spi_controller_clk_hz */
> > +       priv->bus_clk_rate = cm_get_spi_controller_clk_hz();
> 
> Why weak function, we can just return with error message since weak
> does the same job

I used weak function for backward compatibility with the previous driver version:
The previous driver version used cm_get_spi_controller_clk_hz() function which
is set outside of designware_spi.c
You can look at
   arch/arm/mach-socfpga/clock_manager_arria10.c
   arch/arm/mach-socfpga/clock_manager_gen5.c
   ...
as examples.

So I define cm_get_spi_controller_clk_hz as weak in this driver to handle both
use cases:
1) We set frequency via device tree and cm_get_spi_controller_clk_hz is NOT
   defined outside of designware_spi.c (new way)
2) We set frequency using cm_get_spi_controller_clk_hz, which is defined
   outside of designware_spi.c (old way)

I guess it is OK?
-- 
 Eugeniy Paltsev