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

[Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>]

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