All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paul Cercueil <paul@crapouillou.net>
To: Zhou Yanjie <zhouyanjie@zoho.com>
Cc: linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
	robh+dt@kernel.org, paul.burton@mips.com, paulburton@kernel.org,
	mturquette@baylibre.com, sboyd@kernel.org, mark.rutland@arm.com,
	syq@debian.org, sernia.zhou@foxmail.com, zhenwenjin@gmail.com
Subject: Re: [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830.
Date: Wed, 27 Nov 2019 18:37:33 +0100	[thread overview]
Message-ID: <1574876253.3.4@crapouillou.net> (raw)
In-Reply-To: <1574825576-91028-2-git-send-email-zhouyanjie@zoho.com>

Hi Zhou,


Le mer., nov. 27, 2019 at 11:32, Zhou Yanjie <zhouyanjie@zoho.com> a 
écrit :
> 1.Adjust the PLL related code in "cgu.c" and "cgu.h" to make it
>   compatible with the X1830 Soc from Ingenic.
> 2.Adjust the code in "jz4740-cgu.c" to be compatible with the
>   new cgu code.
> 3.Adjust the code in "jz4725b-cgu.c" to be compatible with the
>   new cgu code.
> 4.Adjust the code in "jz4770-cgu.c" to be compatible with the
>   new cgu code.
> 5.Adjust the code in "jz4780-cgu.c" to be compatible with the
>   new cgu code.
> 6.Adjust the code in "x1000-cgu.c" to be compatible with the
>   new cgu code.
> 
> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com>
> ---
>  drivers/clk/ingenic/cgu.c         | 55 
> +++++++++++++++++++++++++++++----------
>  drivers/clk/ingenic/cgu.h         | 12 ++++++++-
>  drivers/clk/ingenic/jz4725b-cgu.c |  3 ++-
>  drivers/clk/ingenic/jz4740-cgu.c  |  3 ++-
>  drivers/clk/ingenic/jz4770-cgu.c  |  6 +++--
>  drivers/clk/ingenic/jz4780-cgu.c  |  3 ++-
>  drivers/clk/ingenic/x1000-cgu.c   |  6 +++--
>  7 files changed, 66 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> index 6e96303..c3c69a8 100644
> --- a/drivers/clk/ingenic/cgu.c
> +++ b/drivers/clk/ingenic/cgu.c
> @@ -84,7 +84,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned 
> long parent_rate)
>  	pll_info = &clk_info->pll;
> 
>  	spin_lock_irqsave(&cgu->lock, flags);
> -	ctl = readl(cgu->base + pll_info->reg);
> +	ctl = readl(cgu->base + pll_info->reg[1]);

I really don't like this patch. There is no info on what reg[1] and 
reg[0] are. First, don't use hardcoded numbers, use macros with 
meaningful names. Second, why not just have two fields instead of one 
2-values array? That would remove a lot of the noise.


>  	spin_unlock_irqrestore(&cgu->lock, flags);
> 
>  	m = (ctl >> pll_info->m_shift) & GENMASK(pll_info->m_bits - 1, 0);
> @@ -93,8 +93,17 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
> unsigned long parent_rate)
>  	n += pll_info->n_offset;
>  	od_enc = ctl >> pll_info->od_shift;
>  	od_enc &= GENMASK(pll_info->od_bits - 1, 0);
> -	bypass = !pll_info->no_bypass_bit &&
> -		 !!(ctl & BIT(pll_info->bypass_bit));
> +
> +	if (pll_info->version >= CGU_X1830) {
> +		spin_lock_irqsave(&cgu->lock, flags);
> +		ctl = readl(cgu->base + pll_info->reg[0]);
> +		spin_unlock_irqrestore(&cgu->lock, flags);

Why the spinlock?


> +
> +		bypass = !pll_info->no_bypass_bit &&
> +			 !!(ctl & BIT(pll_info->bypass_bit));
> +	} else

Please comply to the kernel coding style - use brackets after the else.

> +		bypass = !pll_info->no_bypass_bit &&
> +			 !!(ctl & BIT(pll_info->bypass_bit));
> 
>  	if (bypass)
>  		return parent_rate;
> @@ -106,7 +115,10 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, 
> unsigned long parent_rate)
>  	BUG_ON(od == pll_info->od_max);
>  	od++;
> 
> -	return div_u64((u64)parent_rate * m, n * od);
> +	if (pll_info->version >= CGU_X1830)
> +		return div_u64((u64)parent_rate * m * 2, n * od);

Where does that *2 come from?

> +	else
> +		return div_u64((u64)parent_rate * m, n * od);
>  }
> 
>  static unsigned long
> @@ -139,7 +151,10 @@ ingenic_pll_calc(const struct 
> ingenic_cgu_clk_info *clk_info,
>  	if (pod)
>  		*pod = od;
> 
> -	return div_u64((u64)parent_rate * m, n * od);
> +	if (pll_info->version >= CGU_X1830)
> +		return div_u64((u64)parent_rate * m * 2, n * od);
> +	else
> +		return div_u64((u64)parent_rate * m, n * od);
>  }
> 
>  static inline const struct ingenic_cgu_clk_info *to_clk_info(
> @@ -183,7 +198,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned 
> long req_rate,
>  			clk_info->name, req_rate, rate);
> 
>  	spin_lock_irqsave(&cgu->lock, flags);
> -	ctl = readl(cgu->base + pll_info->reg);
> +	ctl = readl(cgu->base + pll_info->reg[1]);
> 
>  	ctl &= ~(GENMASK(pll_info->m_bits - 1, 0) << pll_info->m_shift);
>  	ctl |= (m - pll_info->m_offset) << pll_info->m_shift;
> @@ -194,7 +209,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned 
> long req_rate,
>  	ctl &= ~(GENMASK(pll_info->od_bits - 1, 0) << pll_info->od_shift);
>  	ctl |= pll_info->od_encoding[od - 1] << pll_info->od_shift;
> 
> -	writel(ctl, cgu->base + pll_info->reg);
> +	writel(ctl, cgu->base + pll_info->reg[1]);
>  	spin_unlock_irqrestore(&cgu->lock, flags);
> 
>  	return 0;
> @@ -212,16 +227,28 @@ static int ingenic_pll_enable(struct clk_hw *hw)
>  	u32 ctl;
> 
>  	spin_lock_irqsave(&cgu->lock, flags);
> -	ctl = readl(cgu->base + pll_info->reg);
> 
> -	ctl &= ~BIT(pll_info->bypass_bit);
> +	if (pll_info->version >= CGU_X1830) {
> +		ctl = readl(cgu->base + pll_info->reg[0]);
> +
> +		ctl &= ~BIT(pll_info->bypass_bit);
> +
> +		writel(ctl, cgu->base + pll_info->reg[0]);
> +
> +		ctl = readl(cgu->base + pll_info->reg[1]);
> +	} else {
> +		ctl = readl(cgu->base + pll_info->reg[1]);
> +
> +		ctl &= ~BIT(pll_info->bypass_bit);
> +	}
> +
>  	ctl |= BIT(pll_info->enable_bit);
> 
> -	writel(ctl, cgu->base + pll_info->reg);
> +	writel(ctl, cgu->base + pll_info->reg[1]);
> 
>  	/* wait for the PLL to stabilise */
>  	for (i = 0; i < timeout; i++) {
> -		ctl = readl(cgu->base + pll_info->reg);
> +		ctl = readl(cgu->base + pll_info->reg[1]);
>  		if (ctl & BIT(pll_info->stable_bit))
>  			break;
>  		mdelay(1);
> @@ -245,11 +272,11 @@ static void ingenic_pll_disable(struct clk_hw 
> *hw)
>  	u32 ctl;
> 
>  	spin_lock_irqsave(&cgu->lock, flags);
> -	ctl = readl(cgu->base + pll_info->reg);
> +	ctl = readl(cgu->base + pll_info->reg[1]);
> 
>  	ctl &= ~BIT(pll_info->enable_bit);
> 
> -	writel(ctl, cgu->base + pll_info->reg);
> +	writel(ctl, cgu->base + pll_info->reg[1]);
>  	spin_unlock_irqrestore(&cgu->lock, flags);
>  }
> 
> @@ -263,7 +290,7 @@ static int ingenic_pll_is_enabled(struct clk_hw 
> *hw)
>  	u32 ctl;
> 
>  	spin_lock_irqsave(&cgu->lock, flags);
> -	ctl = readl(cgu->base + pll_info->reg);
> +	ctl = readl(cgu->base + pll_info->reg[1]);
>  	spin_unlock_irqrestore(&cgu->lock, flags);
> 
>  	return !!(ctl & BIT(pll_info->enable_bit));
> diff --git a/drivers/clk/ingenic/cgu.h b/drivers/clk/ingenic/cgu.h
> index 0dc8004..5f87be4 100644
> --- a/drivers/clk/ingenic/cgu.h
> +++ b/drivers/clk/ingenic/cgu.h
> @@ -42,8 +42,18 @@
>   * @stable_bit: the index of the stable bit in the PLL control 
> register
>   * @no_bypass_bit: if set, the PLL has no bypass functionality
>   */
> +enum ingenic_cgu_version {
> +	CGU_JZ4740,
> +	CGU_JZ4725B,
> +	CGU_JZ4770,
> +	CGU_JZ4780,
> +	CGU_X1000,
> +	CGU_X1830,
> +};
> +
>  struct ingenic_cgu_pll_info {
> -	unsigned reg;
> +	enum ingenic_cgu_version version;
> +	unsigned reg[2];
>  	const s8 *od_encoding;
>  	u8 m_shift, m_bits, m_offset;
>  	u8 n_shift, n_bits, n_offset;
> diff --git a/drivers/clk/ingenic/jz4725b-cgu.c 
> b/drivers/clk/ingenic/jz4725b-cgu.c
> index a3b4635..6da7b41 100644
> --- a/drivers/clk/ingenic/jz4725b-cgu.c
> +++ b/drivers/clk/ingenic/jz4725b-cgu.c
> @@ -53,7 +53,8 @@ static const struct ingenic_cgu_clk_info 
> jz4725b_cgu_clocks[] = {
>  		"pll", CGU_CLK_PLL,
>  		.parents = { JZ4725B_CLK_EXT, -1, -1, -1 },
>  		.pll = {
> -			.reg = CGU_REG_CPPCR,
> +			.version = CGU_JZ4725B,
> +			.reg = { -1, CGU_REG_CPPCR },
>  			.m_shift = 23,
>  			.m_bits = 9,
>  			.m_offset = 2,
> diff --git a/drivers/clk/ingenic/jz4740-cgu.c 
> b/drivers/clk/ingenic/jz4740-cgu.c
> index 4f0e92c..3cf800d 100644
> --- a/drivers/clk/ingenic/jz4740-cgu.c
> +++ b/drivers/clk/ingenic/jz4740-cgu.c
> @@ -68,7 +68,8 @@ static const struct ingenic_cgu_clk_info 
> jz4740_cgu_clocks[] = {
>  		"pll", CGU_CLK_PLL,
>  		.parents = { JZ4740_CLK_EXT, -1, -1, -1 },
>  		.pll = {
> -			.reg = CGU_REG_CPPCR,
> +			.version = CGU_JZ4740,
> +			.reg = { -1, CGU_REG_CPPCR },
>  			.m_shift = 23,
>  			.m_bits = 9,
>  			.m_offset = 2,
> diff --git a/drivers/clk/ingenic/jz4770-cgu.c 
> b/drivers/clk/ingenic/jz4770-cgu.c
> index 956dd65..a62dfb1 100644
> --- a/drivers/clk/ingenic/jz4770-cgu.c
> +++ b/drivers/clk/ingenic/jz4770-cgu.c
> @@ -101,7 +101,8 @@ static const struct ingenic_cgu_clk_info 
> jz4770_cgu_clocks[] = {
>  		"pll0", CGU_CLK_PLL,
>  		.parents = { JZ4770_CLK_EXT },
>  		.pll = {
> -			.reg = CGU_REG_CPPCR0,
> +			.version = CGU_JZ4770,
> +			.reg = { -1, CGU_REG_CPPCR0 },
>  			.m_shift = 24,
>  			.m_bits = 7,
>  			.m_offset = 1,
> @@ -123,7 +124,8 @@ static const struct ingenic_cgu_clk_info 
> jz4770_cgu_clocks[] = {
>  		"pll1", CGU_CLK_PLL,
>  		.parents = { JZ4770_CLK_EXT },
>  		.pll = {
> -			.reg = CGU_REG_CPPCR1,
> +			.version = CGU_JZ4770,
> +			.reg = { -1, CGU_REG_CPPCR1 },
>  			.m_shift = 24,
>  			.m_bits = 7,
>  			.m_offset = 1,
> diff --git a/drivers/clk/ingenic/jz4780-cgu.c 
> b/drivers/clk/ingenic/jz4780-cgu.c
> index ea905ff..59356d1b 100644
> --- a/drivers/clk/ingenic/jz4780-cgu.c
> +++ b/drivers/clk/ingenic/jz4780-cgu.c
> @@ -220,7 +220,8 @@ static const struct ingenic_cgu_clk_info 
> jz4780_cgu_clocks[] = {
>  	/* PLLs */
> 
>  #define DEF_PLL(name) { \
> -	.reg = CGU_REG_ ## name, \
> +	.version = CGU_JZ4780, \
> +	.reg = { -1, CGU_REG_ ## name }, \
>  	.m_shift = 19, \
>  	.m_bits = 13, \
>  	.m_offset = 1, \
> diff --git a/drivers/clk/ingenic/x1000-cgu.c 
> b/drivers/clk/ingenic/x1000-cgu.c
> index b22d87b..7179b9f 100644
> --- a/drivers/clk/ingenic/x1000-cgu.c
> +++ b/drivers/clk/ingenic/x1000-cgu.c
> @@ -57,7 +57,8 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
>  		"apll", CGU_CLK_PLL,
>  		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
>  		.pll = {
> -			.reg = CGU_REG_APLL,
> +			.version = CGU_X1000,
> +			.reg = { -1, CGU_REG_APLL },
>  			.m_shift = 24,
>  			.m_bits = 7,
>  			.m_offset = 1,
> @@ -78,7 +79,8 @@ static const struct ingenic_cgu_clk_info 
> x1000_cgu_clocks[] = {
>  		"mpll", CGU_CLK_PLL,
>  		.parents = { X1000_CLK_EXCLK, -1, -1, -1 },
>  		.pll = {
> -			.reg = CGU_REG_MPLL,
> +			.version = CGU_X1000,
> +			.reg = { -1, CGU_REG_MPLL },
>  			.m_shift = 24,
>  			.m_bits = 7,
>  			.m_offset = 1,
> --
> 2.7.4
> 
> 



  reply	other threads:[~2019-11-27 17:37 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-27  3:32 clk: Ingenic: Add support for the X1830 and add USB clk for X1000 Zhou Yanjie
2019-11-27  3:32 ` [PATCH 1/5] clk: Ingenic: Adjust code to make it compatible with X1830 Zhou Yanjie
2019-11-27 17:37   ` Paul Cercueil [this message]
2019-11-28  6:29     ` Zhou Yanjie
2019-11-29 11:23       ` Paul Cercueil
2019-12-10 22:55         ` Paul Burton
2019-11-27  3:32 ` [PATCH 2/5] dt-bindings: clock: Add X1830 bindings Zhou Yanjie
2019-12-05 20:38   ` Rob Herring
2019-11-27  3:32 ` [PATCH 3/5] clk: Ingenic: Add CGU driver for X1830 Zhou Yanjie
2019-11-27  3:32 ` [PATCH 4/5] dt-bindings: clock: Add USB OTG clock for X1000 Zhou Yanjie
2019-11-27 17:19   ` Paul Cercueil
2019-11-28  5:44     ` Zhou Yanjie
2019-11-27  3:32 ` [PATCH 5/5] clk: Ingenic: " Zhou Yanjie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1574876253.3.4@crapouillou.net \
    --to=paul@crapouillou.net \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mturquette@baylibre.com \
    --cc=paul.burton@mips.com \
    --cc=paulburton@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=sernia.zhou@foxmail.com \
    --cc=syq@debian.org \
    --cc=zhenwenjin@gmail.com \
    --cc=zhouyanjie@zoho.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.