Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670187 > unrolled thread

Re: [PATCH 4/9] clk: imx: add pllv4 support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/9] clk: imx: add pllv4 support Stephen Boyd <sboyd@codeaurora.org> - 2017-06-20 04:00 +0200
    Re: [PATCH 4/9] clk: imx: add pllv4 support Dong Aisheng <dongas86@gmail.com> - 2017-06-20 11:40 +0200
      Re: [PATCH 4/9] clk: imx: add pllv4 support Stephen Boyd <sboyd@codeaurora.org> - 2017-07-01 02:40 +0200
        RE: [PATCH 4/9] clk: imx: add pllv4 support "A.s. Dong" <aisheng.dong@nxp.com> - 2017-07-03 05:30 +0200

#1670187 — Re: [PATCH 4/9] clk: imx: add pllv4 support

On 05/15, Dong Aisheng wrote:
>  obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> diff --git a/drivers/clk/imx/clk-pllv4.c b/drivers/clk/imx/clk-pllv4.c
> new file mode 100644
> index 0000000..502da64
> --- /dev/null
> +++ b/drivers/clk/imx/clk-pllv4.c
> @@ -0,0 +1,196 @@
> +/*
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + * Copyright 2017 NXP
> + *
> + * Author: Dong Aisheng <aisheng.dong@nxp.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/clk.h>

Is this include used?

> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +
> +/* PLL Control Status Register (xPLLCSR) */
> +#define PLL_CSR_OFFSET		0x0
> +#define PLL_VLD			BIT(24)
> +#define PLL_EN			BIT(0)
> +
> +/* PLL Configuration Register (xPLLCFG) */
> +#define PLL_CFG_OFFSET		0x08
> +#define BP_PLL_MULT		16
> +#define BM_PLL_MULT		(0x7f << 16)
> +
> +/* PLL Numerator Register (xPLLNUM) */
> +#define PLL_NUM_OFFSET		0x10
> +
> +/* PLL Denominator Register (xPLLDENOM) */
> +#define PLL_DENOM_OFFSET	0x14
> +
> +struct clk_pllv4 {
> +	struct clk_hw	hw;
> +	void __iomem	*base;
> +};
> +
> +/* Valid PLL MULT Table */
> +static const int pllv4_mult_table[] = {33, 27, 22, 20, 17, 16};
> +
> +#define to_clk_pllv4(__hw) container_of(__hw, struct clk_pllv4, hw)
> +
> +static int clk_pllv4_wait_lock(struct clk_pllv4 *pll)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(5);
> +
> +	/* Wait for PLL to lock */
> +	do {
> +		if (readl_relaxed(pll->base  + PLL_CSR_OFFSET) & PLL_VLD)
> +			break;
> +		if (time_after(jiffies, timeout))
> +			break;
> +			usleep_range(50, 500);
> +	} while (1);
> +
> +	return readl_relaxed(pll->base + PLL_CSR_OFFSET) & PLL_VLD ?
> +	       0 : -ETIMEDOUT;

Use readl_poll_timeout() instead?

> +}
> +
> +static int clk_pllv4_is_enabled(struct clk_hw *hw)
> +{
> +	struct clk_pllv4 *pll = to_clk_pllv4(hw);
> +
> +	if (readl_relaxed(pll->base) & PLL_EN)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static unsigned long clk_pllv4_recalc_rate(struct clk_hw *hw,
> +					   unsigned long parent_rate)
> +{
> +	struct clk_pllv4 *pll = to_clk_pllv4(hw);
> +	u32 div;
> +
> +	div = (readl_relaxed(pll->base + PLL_CFG_OFFSET)
> +	       & BM_PLL_MULT) >> BP_PLL_MULT;

Put that on multiple lines please:

	div = readl_relaxed(pll->base + PLL_CFG_OFFSET);
	div &= BM_PLL_MULT;
	div >>= BP_PLL_MULT; /* Why BP this time? */

> +
> +	return parent_rate * div;
> +}
> +
> +static long clk_pllv4_round_rate(struct clk_hw *hw, unsigned long rate,
> +				 unsigned long *prate)
> +{
> +	unsigned long parent_rate = *prate;
> +	unsigned long round_rate, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(pllv4_mult_table); i++) {
> +		round_rate = parent_rate * pllv4_mult_table[i];
> +		if (rate >= round_rate)
> +			return round_rate;
> +	}
> +
> +	return round_rate;
> +}
> +
> +static bool clk_pllv4_is_valid_mult(unsigned int mult)
> +{
> +	int i;
> +
> +	/* check if mult is in valid MULT table */
> +	for (i = 0; i < ARRAY_SIZE(pllv4_mult_table); i++) {
> +		if (pllv4_mult_table[i] == mult)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int clk_pllv4_set_rate(struct clk_hw *hw, unsigned long rate,
> +			      unsigned long parent_rate)
> +{
> +	struct clk_pllv4 *pll = to_clk_pllv4(hw);
> +	u32 val, mult;
> +
> +	if (clk_pllv4_is_enabled(hw)) {
> +		WARN(1, "clk_pllv4: can't change rate when pll is enabled");
> +		return -EINVAL;

Sad, CLK_SET_RATE_GATE isn't working for you I suppose?

> +	}
> +
> +	mult = rate / parent_rate;
> +
> +	if (clk_pllv4_is_valid_mult(mult))
> +		return -EINVAL;
> +
> +	val = readl_relaxed(pll->base + PLL_CFG_OFFSET);
> +	val &= ~BM_PLL_MULT;
> +	val |= mult << BP_PLL_MULT;
> +	writel_relaxed(val, pll->base + PLL_CFG_OFFSET);
> +
> +	return 0;
> +}
> +

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1670557

On Mon, Jun 19, 2017 at 06:59:17PM -0700, Stephen Boyd wrote:
> On 05/15, Dong Aisheng wrote:
> >  obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> > diff --git a/drivers/clk/imx/clk-pllv4.c b/drivers/clk/imx/clk-pllv4.c
> > new file mode 100644
> > index 0000000..502da64
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-pllv4.c
> > @@ -0,0 +1,196 @@
> > +/*
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Copyright 2017 NXP
> > + *
> > + * Author: Dong Aisheng <aisheng.dong@nxp.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/clk.h>
> 
> Is this include used?
> 

Nice catch. Will remove.

> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +
> > +/* PLL Control Status Register (xPLLCSR) */
> > +#define PLL_CSR_OFFSET		0x0
> > +#define PLL_VLD			BIT(24)
> > +#define PLL_EN			BIT(0)
> > +
> > +/* PLL Configuration Register (xPLLCFG) */
> > +#define PLL_CFG_OFFSET		0x08
> > +#define BP_PLL_MULT		16
> > +#define BM_PLL_MULT		(0x7f << 16)
> > +
> > +/* PLL Numerator Register (xPLLNUM) */
> > +#define PLL_NUM_OFFSET		0x10
> > +
> > +/* PLL Denominator Register (xPLLDENOM) */
> > +#define PLL_DENOM_OFFSET	0x14
> > +
> > +struct clk_pllv4 {
> > +	struct clk_hw	hw;
> > +	void __iomem	*base;
> > +};
> > +
> > +/* Valid PLL MULT Table */
> > +static const int pllv4_mult_table[] = {33, 27, 22, 20, 17, 16};
> > +
> > +#define to_clk_pllv4(__hw) container_of(__hw, struct clk_pllv4, hw)
> > +
> > +static int clk_pllv4_wait_lock(struct clk_pllv4 *pll)
> > +{
> > +	unsigned long timeout = jiffies + msecs_to_jiffies(5);
> > +
> > +	/* Wait for PLL to lock */
> > +	do {
> > +		if (readl_relaxed(pll->base  + PLL_CSR_OFFSET) & PLL_VLD)
> > +			break;
> > +		if (time_after(jiffies, timeout))
> > +			break;
> > +			usleep_range(50, 500);
> > +	} while (1);
> > +
> > +	return readl_relaxed(pll->base + PLL_CSR_OFFSET) & PLL_VLD ?
> > +	       0 : -ETIMEDOUT;
> 
> Use readl_poll_timeout() instead?
> 

Good idea.

> > +}
> > +
> > +static int clk_pllv4_is_enabled(struct clk_hw *hw)
> > +{
> > +	struct clk_pllv4 *pll = to_clk_pllv4(hw);
> > +
> > +	if (readl_relaxed(pll->base) & PLL_EN)
> > +		return 1;
> > +
> > +	return 0;
> > +}
> > +
> > +static unsigned long clk_pllv4_recalc_rate(struct clk_hw *hw,
> > +					   unsigned long parent_rate)
> > +{
> > +	struct clk_pllv4 *pll = to_clk_pllv4(hw);
> > +	u32 div;
> > +
> > +	div = (readl_relaxed(pll->base + PLL_CFG_OFFSET)
> > +	       & BM_PLL_MULT) >> BP_PLL_MULT;
> 
> Put that on multiple lines please:
> 
> 	div = readl_relaxed(pll->base + PLL_CFG_OFFSET);
> 	div &= BM_PLL_MULT;
> 	div >>= BP_PLL_MULT; /* Why BP this time? */
> 

Of course.

> > +
> > +	return parent_rate * div;
> > +}
> > +
> > +static long clk_pllv4_round_rate(struct clk_hw *hw, unsigned long rate,
> > +				 unsigned long *prate)
> > +{
> > +	unsigned long parent_rate = *prate;
> > +	unsigned long round_rate, i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(pllv4_mult_table); i++) {
> > +		round_rate = parent_rate * pllv4_mult_table[i];
> > +		if (rate >= round_rate)
> > +			return round_rate;
> > +	}
> > +
> > +	return round_rate;
> > +}
> > +
> > +static bool clk_pllv4_is_valid_mult(unsigned int mult)
> > +{
> > +	int i;
> > +
> > +	/* check if mult is in valid MULT table */
> > +	for (i = 0; i < ARRAY_SIZE(pllv4_mult_table); i++) {
> > +		if (pllv4_mult_table[i] == mult)
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +static int clk_pllv4_set_rate(struct clk_hw *hw, unsigned long rate,
> > +			      unsigned long parent_rate)
> > +{
> > +	struct clk_pllv4 *pll = to_clk_pllv4(hw);
> > +	u32 val, mult;
> > +
> > +	if (clk_pllv4_is_enabled(hw)) {
> > +		WARN(1, "clk_pllv4: can't change rate when pll is enabled");
> > +		return -EINVAL;
> 
> Sad, CLK_SET_RATE_GATE isn't working for you I suppose?
> 

CLK_SET_RATE_GATE can't work in early stage before running clk_disable_unused.
At that point, the clock tree state is still not consistent with HW.
e.g. prepare/enable count is still zero but it's actually enabled due to
reset state or bootloader.

The code here is adding a double check in case user sets rate in early stage.

However, probably it could also be moved into clock core as it's not platform
dependant behavior?

Regards
Dong Aisheng

> > +	}
> > +
> > +	mult = rate / parent_rate;
> > +
> > +	if (clk_pllv4_is_valid_mult(mult))
> > +		return -EINVAL;
> > +
> > +	val = readl_relaxed(pll->base + PLL_CFG_OFFSET);
> > +	val &= ~BM_PLL_MULT;
> > +	val |= mult << BP_PLL_MULT;
> > +	writel_relaxed(val, pll->base + PLL_CFG_OFFSET);
> > +
> > +	return 0;
> > +}
> > +
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1679223

On 06/20, Dong Aisheng wrote:
> On Mon, Jun 19, 2017 at 06:59:17PM -0700, Stephen Boyd wrote:
> > On 05/15, Dong Aisheng wrote:
> > > +
> > > +	if (clk_pllv4_is_enabled(hw)) {
> > > +		WARN(1, "clk_pllv4: can't change rate when pll is enabled");
> > > +		return -EINVAL;
> > 
> > Sad, CLK_SET_RATE_GATE isn't working for you I suppose?
> > 
> 
> CLK_SET_RATE_GATE can't work in early stage before running clk_disable_unused.
> At that point, the clock tree state is still not consistent with HW.
> e.g. prepare/enable count is still zero but it's actually enabled due to
> reset state or bootloader.
> 
> The code here is adding a double check in case user sets rate in early stage.
> 
> However, probably it could also be moved into clock core as it's not platform
> dependant behavior?
> 

Ok. It would be good to fix the core framework to synchronize the
prepared/enabled state at registration time so we don't need this
check in the driver.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1679622

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@codeaurora.org]
> Sent: Saturday, July 01, 2017 8:37 AM
> To: Dong Aisheng
> Cc: A.s. Dong; linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com;
> shawnguo@kernel.org; Anson Huang; Jacky Bai
> Subject: Re: [PATCH 4/9] clk: imx: add pllv4 support
> 
> On 06/20, Dong Aisheng wrote:
> > On Mon, Jun 19, 2017 at 06:59:17PM -0700, Stephen Boyd wrote:
> > > On 05/15, Dong Aisheng wrote:
> > > > +
> > > > +	if (clk_pllv4_is_enabled(hw)) {
> > > > +		WARN(1, "clk_pllv4: can't change rate when pll is
> enabled");
> > > > +		return -EINVAL;
> > >
> > > Sad, CLK_SET_RATE_GATE isn't working for you I suppose?
> > >
> >
> > CLK_SET_RATE_GATE can't work in early stage before running
> clk_disable_unused.
> > At that point, the clock tree state is still not consistent with HW.
> > e.g. prepare/enable count is still zero but it's actually enabled due
> > to reset state or bootloader.
> >
> > The code here is adding a double check in case user sets rate in early
> stage.
> >
> > However, probably it could also be moved into clock core as it's not
> > platform dependant behavior?
> >
> 
> Ok. It would be good to fix the core framework to synchronize the
> prepared/enabled state at registration time so we don't need this check
> in the driver.
> 

I will prepare a core fix later and remove these checks first.

Regards
Dong Aisheng

> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web