[PATCH v1 2/6] clk: rockchip: fix up the frac clk get rate error

[PATCH v1 2/6] clk: rockchip: fix up the frac clk get rate error

[Elaine Zhang]

support fractional divider with only one level parent clock

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
---
 drivers/clk/rockchip/clk.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index e04bf300ea0a..0256a99f06f3 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -195,16 +195,21 @@ static void rockchip_fractional_approximation(struct clk_hw *hw,
 	if (((rate * 20 > p_rate) && (p_rate % rate != 0)) ||
 	    (fd->max_prate && fd->max_prate < p_rate)) {
 		p_parent = clk_hw_get_parent(clk_hw_get_parent(hw));
-		p_parent_rate = clk_hw_get_rate(p_parent);
-		*parent_rate = p_parent_rate;
-		if (fd->max_prate && p_parent_rate > fd->max_prate) {
-			div = DIV_ROUND_UP(p_parent_rate, fd->max_prate);
-			*parent_rate = p_parent_rate / div;
+		if (!p_parent) {
+			*parent_rate = p_rate;
+		} else {
+			p_parent_rate = clk_hw_get_rate(p_parent);
+			*parent_rate = p_parent_rate;
+			if (fd->max_prate && p_parent_rate > fd->max_prate) {
+				div = DIV_ROUND_UP(p_parent_rate,
+						   fd->max_prate);
+				*parent_rate = p_parent_rate / div;
+			}
 		}
 
 		if (*parent_rate < rate * 20) {
-			pr_err("%s parent_rate(%ld) is low than rate(%ld)*20, fractional div is not allowed\n",
-			       clk_hw_get_name(hw), *parent_rate, rate);
+			pr_warn("%s p_rate(%ld) is low than rate(%ld)*20, use integer or half-div\n",
+				clk_hw_get_name(hw), *parent_rate, rate);
 			*m = 0;
 			*n = 1;
 			return;
-- 
1.9.1




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/6] clk: rockchip: add a COMPOSITE_DIV_OFFSET clock-type

[Elaine Zhang]

From: Finley Xiao <finley.xiao@rock-chips.com>

The div offset of some clocks are different from their mux offset
and the COMPOSITE clock-type require that div and mux offset are
the same, so add a new COMPOSITE_DIV_OFFSET clock-type to handle that.

Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
---
 drivers/clk/rockchip/clk.c |  9 ++++++---
 drivers/clk/rockchip/clk.h | 23 +++++++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 0256a99f06f3..0a8a694a41ab 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -46,7 +46,7 @@ static struct clk *rockchip_clk_register_branch(const char *name,
 		const char *const *parent_names, u8 num_parents,
 		void __iomem *base,
 		int muxdiv_offset, u8 mux_shift, u8 mux_width, u8 mux_flags,
-		u8 div_shift, u8 div_width, u8 div_flags,
+		int div_offset, u8 div_shift, u8 div_width, u8 div_flags,
 		struct clk_div_table *div_table, int gate_offset,
 		u8 gate_shift, u8 gate_flags, unsigned long flags,
 		spinlock_t *lock)
@@ -95,7 +95,10 @@ static struct clk *rockchip_clk_register_branch(const char *name,
 		}
 
 		div->flags = div_flags;
-		div->reg = base + muxdiv_offset;
+		if (div_offset)
+			div->reg = base + div_offset;
+		else
+			div->reg = base + muxdiv_offset;
 		div->shift = div_shift;
 		div->width = div_width;
 		div->lock = lock;
@@ -536,7 +539,7 @@ void __init rockchip_clk_register_branches(
 				ctx->reg_base, list->muxdiv_offset,
 				list->mux_shift,
 				list->mux_width, list->mux_flags,
-				list->div_shift, list->div_width,
+				list->div_offset, list->div_shift, list->div_width,
 				list->div_flags, list->div_table,
 				list->gate_offset, list->gate_shift,
 				list->gate_flags, flags, &ctx->lock);
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index 3c827ec0965c..20200a707611 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -407,6 +407,7 @@ struct rockchip_clk_branch {
 	u8				mux_shift;
 	u8				mux_width;
 	u8				mux_flags;
+	int				div_offset;
 	u8				div_shift;
 	u8				div_width;
 	u8				div_flags;
@@ -439,6 +440,28 @@ struct rockchip_clk_branch {
 		.gate_flags	= gf,				\
 	}
 
+#define COMPOSITE_DIV_OFFSET(_id, cname, pnames, f, mo, ms, mw,	\
+			     mf, do, ds, dw, df, go, gs, gf)	\
+	{							\
+		.id		= _id,				\
+		.branch_type	= branch_composite,		\
+		.name		= cname,			\
+		.parent_names	= pnames,			\
+		.num_parents	= ARRAY_SIZE(pnames),		\
+		.flags		= f,				\
+		.muxdiv_offset	= mo,				\
+		.mux_shift	= ms,				\
+		.mux_width	= mw,				\
+		.mux_flags	= mf,				\
+		.div_offset	= do,				\
+		.div_shift	= ds,				\
+		.div_width	= dw,				\
+		.div_flags	= df,				\
+		.gate_offset	= go,				\
+		.gate_shift	= gs,				\
+		.gate_flags	= gf,				\
+	}
+
 #define COMPOSITE_NOMUX(_id, cname, pname, f, mo, ds, dw, df,	\
 			go, gs, gf)				\
 	{							\
-- 
1.9.1




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 5/6] clk: rockchip: add pll up and down when change pll freq

[Elaine Zhang]

set pll sequence:
	->set pll to slow mode or other plls
	->set pll down
	->set pll params
	->set pll up
	->wait pll lock status
	->set pll to normal mode

To slove the system error:
wait_pll_lock: timeout waiting for pll to lock
pll_set_params: pll update unsucessful,
		trying to restore old params

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
---
 drivers/clk/rockchip/clk-pll.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index dd0433d4753e..9fe1227e77e9 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -208,6 +208,11 @@ static int rockchip_rk3036_pll_set_params(struct rockchip_clk_pll *pll,
 		rate_change_remuxed = 1;
 	}
 
+	/* set pll power down */
+	writel(HIWORD_UPDATE(1,
+			     RK3036_PLLCON1_PWRDOWN, 13),
+	       pll->reg_base + RK3036_PLLCON(1));
+
 	/* update pll values */
 	writel_relaxed(HIWORD_UPDATE(rate->fbdiv, RK3036_PLLCON0_FBDIV_MASK,
 					  RK3036_PLLCON0_FBDIV_SHIFT) |
@@ -229,6 +234,10 @@ static int rockchip_rk3036_pll_set_params(struct rockchip_clk_pll *pll,
 	pllcon |= rate->frac << RK3036_PLLCON2_FRAC_SHIFT;
 	writel_relaxed(pllcon, pll->reg_base + RK3036_PLLCON(2));
 
+	/* set pll power up */
+	writel(HIWORD_UPDATE(0, RK3036_PLLCON1_PWRDOWN, 13),
+	       pll->reg_base + RK3036_PLLCON(1));
+
 	/* wait for the pll to lock */
 	ret = rockchip_pll_wait_lock(pll);
 	if (ret) {
@@ -685,6 +694,11 @@ static int rockchip_rk3399_pll_set_params(struct rockchip_clk_pll *pll,
 		rate_change_remuxed = 1;
 	}
 
+	/* set pll power down */
+	writel(HIWORD_UPDATE(RK3399_PLLCON3_PWRDOWN,
+			     RK3399_PLLCON3_PWRDOWN, 0),
+	       pll->reg_base + RK3399_PLLCON(3));
+
 	/* update pll values */
 	writel_relaxed(HIWORD_UPDATE(rate->fbdiv, RK3399_PLLCON0_FBDIV_MASK,
 						  RK3399_PLLCON0_FBDIV_SHIFT),
@@ -708,6 +722,11 @@ static int rockchip_rk3399_pll_set_params(struct rockchip_clk_pll *pll,
 					    RK3399_PLLCON3_DSMPD_SHIFT),
 		       pll->reg_base + RK3399_PLLCON(3));
 
+	/* set pll power up */
+	writel(HIWORD_UPDATE(0,
+			     RK3399_PLLCON3_PWRDOWN, 0),
+	       pll->reg_base + RK3399_PLLCON(3));
+
 	/* wait for the pll to lock */
 	ret = rockchip_rk3399_pll_wait_lock(pll);
 	if (ret) {
-- 
1.9.1




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/6] clk: rockchip: add a COMPOSITE_DIV_OFFSET clock-type

[Heiko Stübner]

Am Mittwoch, 3. April 2019, 11:42:26 CEST schrieb Elaine Zhang:
> From: Finley Xiao <finley.xiao@rock-chips.com>
> 
> The div offset of some clocks are different from their mux offset
> and the COMPOSITE clock-type require that div and mux offset are
> the same, so add a new COMPOSITE_DIV_OFFSET clock-type to handle that.
> 
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>

applied for 5.2

Thanks
Heiko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 4/6] clk: rockchip: add a clock-type for muxes based in the pmugrf

[Heiko Stübner]

Hi Elaine,

Am Mittwoch, 3. April 2019, 11:42:27 CEST schrieb Elaine Zhang:
> Rockchip socs often have some tiny number of muxes not controlled from
> the core clock controller but through bits set in the pmugrf.
> Use MUXPMUGRF() to cover this special clock-type.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>

Do you have an example for such a clock?

I.e. on rk3399 we have the pmucru which already links
to the pmugrf as its "regular" rockchip,grf.
Similarly the main-cru links to the main grf.

So I don't really see where a clock needs to be defined
in the main-grf and use a bit from the pmugrf.

Similarly I wasn't really able to find anything that looks like
a clock-mux in the PX30's (rk3326) pmugrf.

So I'd really like an example beforehand :-D

Thanks
Heiko

> ---
>  drivers/clk/rockchip/clk.c |  9 +++++++++
>  drivers/clk/rockchip/clk.h | 17 +++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
> index 0a8a694a41ab..875412043dd7 100644
> --- a/drivers/clk/rockchip/clk.c
> +++ b/drivers/clk/rockchip/clk.c
> @@ -415,6 +415,8 @@ struct rockchip_clk_provider * __init rockchip_clk_init(struct device_node *np,
>  
>  	ctx->grf = syscon_regmap_lookup_by_phandle(ctx->cru_node,
>  						   "rockchip,grf");
> +	ctx->pmugrf = syscon_regmap_lookup_by_phandle(ctx->cru_node,
> +						   "rockchip,pmugrf");
>  
>  	return ctx;
>  
> @@ -490,6 +492,13 @@ void __init rockchip_clk_register_branches(
>  				list->mux_shift, list->mux_width,
>  				list->mux_flags);
>  			break;
> +		case branch_muxpmugrf:
> +			clk = rockchip_clk_register_muxgrf(list->name,
> +				list->parent_names, list->num_parents,
> +				flags, ctx->pmugrf, list->muxdiv_offset,
> +				list->mux_shift, list->mux_width,
> +				list->mux_flags);
> +			break;
>  		case branch_divider:
>  			if (list->div_table)
>  				clk = clk_register_divider_table(NULL,
> diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
> index 20200a707611..1b30346f11e1 100644
> --- a/drivers/clk/rockchip/clk.h
> +++ b/drivers/clk/rockchip/clk.h
> @@ -234,6 +234,7 @@ struct rockchip_clk_provider {
>  	struct clk_onecell_data clk_data;
>  	struct device_node *cru_node;
>  	struct regmap *grf;
> +	struct regmap *pmugrf;
>  	spinlock_t lock;
>  };
>  
> @@ -386,6 +387,7 @@ enum rockchip_clk_branch_type {
>  	branch_composite,
>  	branch_mux,
>  	branch_muxgrf,
> +	branch_muxpmugrf,
>  	branch_divider,
>  	branch_fraction_divider,
>  	branch_gate,
> @@ -658,6 +660,21 @@ struct rockchip_clk_branch {
>  		.gate_offset	= -1,				\
>  	}
>  
> +#define MUXPMUGRF(_id, cname, pnames, f, o, s, w, mf)		\
> +	{							\
> +		.id		= _id,				\
> +		.branch_type	= branch_muxpmugrf,		\
> +		.name		= cname,			\
> +		.parent_names	= pnames,			\
> +		.num_parents	= ARRAY_SIZE(pnames),		\
> +		.flags		= f,				\
> +		.muxdiv_offset	= o,				\
> +		.mux_shift	= s,				\
> +		.mux_width	= w,				\
> +		.mux_flags	= mf,				\
> +		.gate_offset	= -1,				\
> +	}
> +
>  #define DIV(_id, cname, pname, f, o, s, w, df)			\
>  	{							\
>  		.id		= _id,				\
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/6] clk: rockchip: Add supprot to limit input rate for fractional divider

[Heiko Stübner]

Hi Elaine,

Am Mittwoch, 3. April 2019, 11:42:24 CEST schrieb Elaine Zhang:
> From: Finley Xiao <finley.xiao@rock-chips.com>
> 
> From Rockchips fractional divider usage, some clocks can be generated
> by fractional divider, but the input clock frequency of fractional
> divider should be less than a specified value.
> 
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>

can you tell me where these maximum input values come from?

I talked to Christoph from Theobroma (Cc'ed) last week and he mentioned
that they're using the fractional divider with a higher input frequency
to create a very specific frequency [some details are gone from my memory
though] they can't get otherwise.

So I really don't want to break their working setup by introducing barriers
that are not strictly necessary.

@Christoph: can you describe the bits from your fractional setup that
I've forgotten please?


Thanks
Heiko


> diff --git a/drivers/clk/rockchip/clk-px30.c b/drivers/clk/rockchip/clk-px30.c
> index 601a77f1af78..ccabce35580b 100644
> --- a/drivers/clk/rockchip/clk-px30.c
> +++ b/drivers/clk/rockchip/clk-px30.c
> @@ -21,6 +21,7 @@
>  #include "clk.h"
>  
>  #define PX30_GRF_SOC_STATUS0		0x480
> +#define PX30_FRAC_MAX_PRATE		600000000


> diff --git a/drivers/clk/rockchip/clk-rk3368.c b/drivers/clk/rockchip/clk-rk3368.c
> index 7c4d242f19c1..67c2da5e7d61 100644
> --- a/drivers/clk/rockchip/clk-rk3368.c
> +++ b/drivers/clk/rockchip/clk-rk3368.c
> @@ -20,6 +20,9 @@
>  #include "clk.h"
>  
>  #define RK3368_GRF_SOC_STATUS0	0x480
> +#define RK3368_I2S_FRAC_MAX_PRATE       600000000
> +#define RK3368_UART_FRAC_MAX_PRATE	600000000
> +#define RK3368_SPDIF_FRAC_MAX_PRATE	600000000

> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> index 5a628148f3f0..1d81382bd3e0 100644
> --- a/drivers/clk/rockchip/clk-rk3399.c
> +++ b/drivers/clk/rockchip/clk-rk3399.c
> @@ -21,6 +21,12 @@
>  #include <dt-bindings/clock/rk3399-cru.h>
>  #include "clk.h"
>  
> +#define RK3399_I2S_FRAC_MAX_PRATE       600000000
> +#define RK3399_UART_FRAC_MAX_PRATE	600000000
> +#define RK3399_SPDIF_FRAC_MAX_PRATE	600000000
> +#define RK3399_VOP_FRAC_MAX_PRATE	600000000
> +#define RK3399_WIFI_FRAC_MAX_PRATE	600000000
> +




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/6] clk: rockchip: Add supprot to limit input rate for fractional divider

[Christoph Müllner]

Hi Heiko,

On 12.04.19 13:52, Heiko Stübner wrote:
> Hi Elaine,
> 
> Am Mittwoch, 3. April 2019, 11:42:24 CEST schrieb Elaine Zhang:
>> From: Finley Xiao <finley.xiao@rock-chips.com>
>>
>> From Rockchips fractional divider usage, some clocks can be generated
>> by fractional divider, but the input clock frequency of fractional
>> divider should be less than a specified value.
>>
>> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> 
> can you tell me where these maximum input values come from?
> 
> I talked to Christoph from Theobroma (Cc'ed) last week and he mentioned
> that they're using the fractional divider with a higher input frequency
> to create a very specific frequency [some details are gone from my memory
> though] they can't get otherwise.
> 
> So I really don't want to break their working setup by introducing barriers
> that are not strictly necessary.
> 
> @Christoph: can you describe the bits from your fractional setup that
> I've forgotten please?

We need to set the I2S0 clock to 24.56 MHz.

When restricting the input frequency to a maximum of 600 Mhz,
we could use the integer divider to get 400 Mhz (dividing by 2).
However, with the 400 Mhz as input to the frac divider,
we run into the problem, that the maximum possible output frequency
is 20 MHz (there is another restriction which states that the
fraction input : output frequency must be >= 20).

Thanks,
Christoph

> 
>> diff --git a/drivers/clk/rockchip/clk-px30.c b/drivers/clk/rockchip/clk-px30.c
>> index 601a77f1af78..ccabce35580b 100644
>> --- a/drivers/clk/rockchip/clk-px30.c
>> +++ b/drivers/clk/rockchip/clk-px30.c
>> @@ -21,6 +21,7 @@
>>  #include "clk.h"
>>  
>>  #define PX30_GRF_SOC_STATUS0		0x480
>> +#define PX30_FRAC_MAX_PRATE		600000000
> 
> 
>> diff --git a/drivers/clk/rockchip/clk-rk3368.c b/drivers/clk/rockchip/clk-rk3368.c
>> index 7c4d242f19c1..67c2da5e7d61 100644
>> --- a/drivers/clk/rockchip/clk-rk3368.c
>> +++ b/drivers/clk/rockchip/clk-rk3368.c
>> @@ -20,6 +20,9 @@
>>  #include "clk.h"
>>  
>>  #define RK3368_GRF_SOC_STATUS0	0x480
>> +#define RK3368_I2S_FRAC_MAX_PRATE       600000000
>> +#define RK3368_UART_FRAC_MAX_PRATE	600000000
>> +#define RK3368_SPDIF_FRAC_MAX_PRATE	600000000
> 
>> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
>> index 5a628148f3f0..1d81382bd3e0 100644
>> --- a/drivers/clk/rockchip/clk-rk3399.c
>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>> @@ -21,6 +21,12 @@
>>  #include <dt-bindings/clock/rk3399-cru.h>
>>  #include "clk.h"
>>  
>> +#define RK3399_I2S_FRAC_MAX_PRATE       600000000
>> +#define RK3399_UART_FRAC_MAX_PRATE	600000000
>> +#define RK3399_SPDIF_FRAC_MAX_PRATE	600000000
>> +#define RK3399_VOP_FRAC_MAX_PRATE	600000000
>> +#define RK3399_WIFI_FRAC_MAX_PRATE	600000000
>> +
> 
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 5/6] clk: rockchip: add pll up and down when change pll freq

[Heiko Stübner]

Hi Elaine,

Am Mittwoch, 3. April 2019, 11:44:09 CEST schrieb Elaine Zhang:
> set pll sequence:
> 	->set pll to slow mode or other plls
> 	->set pll down
> 	->set pll params
> 	->set pll up
> 	->wait pll lock status
> 	->set pll to normal mode
> 
> To slove the system error:
> wait_pll_lock: timeout waiting for pll to lock
> pll_set_params: pll update unsucessful,
> 		trying to restore old params

Can you tell me on what soc this was experienced?

The patch includes rk3399, but I don't think the CrOS kernel
does powerdown the pll when changing the cpu-frequency
[added Doug and Brian for clarification and possible testing :-) ]

But I did find that the M0 code in ATF does actually power-down the
PLL and follow your outline from above. So essentially I'd just like
a thumbs up from chromeos people if they have the time.


Heiko


> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
>  drivers/clk/rockchip/clk-pll.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
> index dd0433d4753e..9fe1227e77e9 100644
> --- a/drivers/clk/rockchip/clk-pll.c
> +++ b/drivers/clk/rockchip/clk-pll.c
> @@ -208,6 +208,11 @@ static int rockchip_rk3036_pll_set_params(struct rockchip_clk_pll *pll,
>  		rate_change_remuxed = 1;
>  	}
>  
> +	/* set pll power down */
> +	writel(HIWORD_UPDATE(1,
> +			     RK3036_PLLCON1_PWRDOWN, 13),
> +	       pll->reg_base + RK3036_PLLCON(1));
> +
>  	/* update pll values */
>  	writel_relaxed(HIWORD_UPDATE(rate->fbdiv, RK3036_PLLCON0_FBDIV_MASK,
>  					  RK3036_PLLCON0_FBDIV_SHIFT) |
> @@ -229,6 +234,10 @@ static int rockchip_rk3036_pll_set_params(struct rockchip_clk_pll *pll,
>  	pllcon |= rate->frac << RK3036_PLLCON2_FRAC_SHIFT;
>  	writel_relaxed(pllcon, pll->reg_base + RK3036_PLLCON(2));
>  
> +	/* set pll power up */
> +	writel(HIWORD_UPDATE(0, RK3036_PLLCON1_PWRDOWN, 13),
> +	       pll->reg_base + RK3036_PLLCON(1));
> +
>  	/* wait for the pll to lock */
>  	ret = rockchip_pll_wait_lock(pll);
>  	if (ret) {
> @@ -685,6 +694,11 @@ static int rockchip_rk3399_pll_set_params(struct rockchip_clk_pll *pll,
>  		rate_change_remuxed = 1;
>  	}
>  
> +	/* set pll power down */
> +	writel(HIWORD_UPDATE(RK3399_PLLCON3_PWRDOWN,
> +			     RK3399_PLLCON3_PWRDOWN, 0),
> +	       pll->reg_base + RK3399_PLLCON(3));
> +
>  	/* update pll values */
>  	writel_relaxed(HIWORD_UPDATE(rate->fbdiv, RK3399_PLLCON0_FBDIV_MASK,
>  						  RK3399_PLLCON0_FBDIV_SHIFT),
> @@ -708,6 +722,11 @@ static int rockchip_rk3399_pll_set_params(struct rockchip_clk_pll *pll,
>  					    RK3399_PLLCON3_DSMPD_SHIFT),
>  		       pll->reg_base + RK3399_PLLCON(3));
>  
> +	/* set pll power up */
> +	writel(HIWORD_UPDATE(0,
> +			     RK3399_PLLCON3_PWRDOWN, 0),
> +	       pll->reg_base + RK3399_PLLCON(3));
> +
>  	/* wait for the pll to lock */
>  	ret = rockchip_rk3399_pll_wait_lock(pll);
>  	if (ret) {
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/6] clk: rockchip: Add supprot to limit input rate for fractional divider

[Heiko Stübner]

Hi Christoph,

Am Freitag, 12. April 2019, 14:12:52 CEST schrieb Christoph Müllner:
> On 12.04.19 13:52, Heiko Stübner wrote:
> > Am Mittwoch, 3. April 2019, 11:42:24 CEST schrieb Elaine Zhang:
> >> From: Finley Xiao <finley.xiao@rock-chips.com>
> >>
> >> From Rockchips fractional divider usage, some clocks can be generated
> >> by fractional divider, but the input clock frequency of fractional
> >> divider should be less than a specified value.
> >>
> >> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> >> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> > 
> > can you tell me where these maximum input values come from?
> > 
> > I talked to Christoph from Theobroma (Cc'ed) last week and he mentioned
> > that they're using the fractional divider with a higher input frequency
> > to create a very specific frequency [some details are gone from my memory
> > though] they can't get otherwise.
> > 
> > So I really don't want to break their working setup by introducing barriers
> > that are not strictly necessary.
> > 
> > @Christoph: can you describe the bits from your fractional setup that
> > I've forgotten please?
> 
> We need to set the I2S0 clock to 24.56 MHz.
> 
> When restricting the input frequency to a maximum of 600 Mhz,
> we could use the integer divider to get 400 Mhz (dividing by 2).
> However, with the 400 Mhz as input to the frac divider,
> we run into the problem, that the maximum possible output frequency
> is 20 MHz (there is another restriction which states that the
> fraction input : output frequency must be >= 20).

just for clarification, what is the current input frequency you
already use sucessfully?


Heiko

> > 
> >> diff --git a/drivers/clk/rockchip/clk-px30.c b/drivers/clk/rockchip/clk-px30.c
> >> index 601a77f1af78..ccabce35580b 100644
> >> --- a/drivers/clk/rockchip/clk-px30.c
> >> +++ b/drivers/clk/rockchip/clk-px30.c
> >> @@ -21,6 +21,7 @@
> >>  #include "clk.h"
> >>  
> >>  #define PX30_GRF_SOC_STATUS0		0x480
> >> +#define PX30_FRAC_MAX_PRATE		600000000
> > 
> > 
> >> diff --git a/drivers/clk/rockchip/clk-rk3368.c b/drivers/clk/rockchip/clk-rk3368.c
> >> index 7c4d242f19c1..67c2da5e7d61 100644
> >> --- a/drivers/clk/rockchip/clk-rk3368.c
> >> +++ b/drivers/clk/rockchip/clk-rk3368.c
> >> @@ -20,6 +20,9 @@
> >>  #include "clk.h"
> >>  
> >>  #define RK3368_GRF_SOC_STATUS0	0x480
> >> +#define RK3368_I2S_FRAC_MAX_PRATE       600000000
> >> +#define RK3368_UART_FRAC_MAX_PRATE	600000000
> >> +#define RK3368_SPDIF_FRAC_MAX_PRATE	600000000
> > 
> >> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> >> index 5a628148f3f0..1d81382bd3e0 100644
> >> --- a/drivers/clk/rockchip/clk-rk3399.c
> >> +++ b/drivers/clk/rockchip/clk-rk3399.c
> >> @@ -21,6 +21,12 @@
> >>  #include <dt-bindings/clock/rk3399-cru.h>
> >>  #include "clk.h"
> >>  
> >> +#define RK3399_I2S_FRAC_MAX_PRATE       600000000
> >> +#define RK3399_UART_FRAC_MAX_PRATE	600000000
> >> +#define RK3399_SPDIF_FRAC_MAX_PRATE	600000000
> >> +#define RK3399_VOP_FRAC_MAX_PRATE	600000000
> >> +#define RK3399_WIFI_FRAC_MAX_PRATE	600000000
> >> +
> > 
> > 
> > 
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/6] clk: rockchip: Add supprot to limit input rate for fractional divider

[Christoph Müllner]

On 12.04.19 14:21, Heiko Stübner wrote:
> Hi Christoph,
> 
> Am Freitag, 12. April 2019, 14:12:52 CEST schrieb Christoph Müllner:
>> On 12.04.19 13:52, Heiko Stübner wrote:
>>> Am Mittwoch, 3. April 2019, 11:42:24 CEST schrieb Elaine Zhang:
>>>> From: Finley Xiao <finley.xiao@rock-chips.com>
>>>>
>>>> From Rockchips fractional divider usage, some clocks can be generated
>>>> by fractional divider, but the input clock frequency of fractional
>>>> divider should be less than a specified value.
>>>>
>>>> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
>>>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>>>
>>> can you tell me where these maximum input values come from?
>>>
>>> I talked to Christoph from Theobroma (Cc'ed) last week and he mentioned
>>> that they're using the fractional divider with a higher input frequency
>>> to create a very specific frequency [some details are gone from my memory
>>> though] they can't get otherwise.
>>>
>>> So I really don't want to break their working setup by introducing barriers
>>> that are not strictly necessary.
>>>
>>> @Christoph: can you describe the bits from your fractional setup that
>>> I've forgotten please?
>>
>> We need to set the I2S0 clock to 24.56 MHz.
>>
>> When restricting the input frequency to a maximum of 600 Mhz,
>> we could use the integer divider to get 400 Mhz (dividing by 2).
>> However, with the 400 Mhz as input to the frac divider,
>> we run into the problem, that the maximum possible output frequency
>> is 20 MHz (there is another restriction which states that the
>> fraction input : output frequency must be >= 20).
> 
> just for clarification, what is the current input frequency you
> already use sucessfully?

Our working setup uses the integer divider to reduce to 400 MHz
and uses the frac divider to get something near 24.56 MHz.
I have to admit I have never measured what's on the clock line.

>>>
>>>> diff --git a/drivers/clk/rockchip/clk-px30.c b/drivers/clk/rockchip/clk-px30.c
>>>> index 601a77f1af78..ccabce35580b 100644
>>>> --- a/drivers/clk/rockchip/clk-px30.c
>>>> +++ b/drivers/clk/rockchip/clk-px30.c
>>>> @@ -21,6 +21,7 @@
>>>>  #include "clk.h"
>>>>  
>>>>  #define PX30_GRF_SOC_STATUS0		0x480
>>>> +#define PX30_FRAC_MAX_PRATE		600000000
>>>
>>>
>>>> diff --git a/drivers/clk/rockchip/clk-rk3368.c b/drivers/clk/rockchip/clk-rk3368.c
>>>> index 7c4d242f19c1..67c2da5e7d61 100644
>>>> --- a/drivers/clk/rockchip/clk-rk3368.c
>>>> +++ b/drivers/clk/rockchip/clk-rk3368.c
>>>> @@ -20,6 +20,9 @@
>>>>  #include "clk.h"
>>>>  
>>>>  #define RK3368_GRF_SOC_STATUS0	0x480
>>>> +#define RK3368_I2S_FRAC_MAX_PRATE       600000000
>>>> +#define RK3368_UART_FRAC_MAX_PRATE	600000000
>>>> +#define RK3368_SPDIF_FRAC_MAX_PRATE	600000000
>>>
>>>> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
>>>> index 5a628148f3f0..1d81382bd3e0 100644
>>>> --- a/drivers/clk/rockchip/clk-rk3399.c
>>>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>>>> @@ -21,6 +21,12 @@
>>>>  #include <dt-bindings/clock/rk3399-cru.h>
>>>>  #include "clk.h"
>>>>  
>>>> +#define RK3399_I2S_FRAC_MAX_PRATE       600000000
>>>> +#define RK3399_UART_FRAC_MAX_PRATE	600000000
>>>> +#define RK3399_SPDIF_FRAC_MAX_PRATE	600000000
>>>> +#define RK3399_VOP_FRAC_MAX_PRATE	600000000
>>>> +#define RK3399_WIFI_FRAC_MAX_PRATE	600000000
>>>> +
>>>
>>>
>>>
>>
> 
> 
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/6] clk: rockchip: Add supprot to limit input rate for fractional divider

[Heiko Stübner]

Am Freitag, 12. April 2019, 14:32:08 CEST schrieb Christoph Müllner:
> 
> On 12.04.19 14:21, Heiko Stübner wrote:
> > Hi Christoph,
> > 
> > Am Freitag, 12. April 2019, 14:12:52 CEST schrieb Christoph Müllner:
> >> On 12.04.19 13:52, Heiko Stübner wrote:
> >>> Am Mittwoch, 3. April 2019, 11:42:24 CEST schrieb Elaine Zhang:
> >>>> From: Finley Xiao <finley.xiao@rock-chips.com>
> >>>>
> >>>> From Rockchips fractional divider usage, some clocks can be generated
> >>>> by fractional divider, but the input clock frequency of fractional
> >>>> divider should be less than a specified value.
> >>>>
> >>>> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> >>>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> >>>
> >>> can you tell me where these maximum input values come from?
> >>>
> >>> I talked to Christoph from Theobroma (Cc'ed) last week and he mentioned
> >>> that they're using the fractional divider with a higher input frequency
> >>> to create a very specific frequency [some details are gone from my memory
> >>> though] they can't get otherwise.
> >>>
> >>> So I really don't want to break their working setup by introducing barriers
> >>> that are not strictly necessary.
> >>>
> >>> @Christoph: can you describe the bits from your fractional setup that
> >>> I've forgotten please?
> >>
> >> We need to set the I2S0 clock to 24.56 MHz.
> >>
> >> When restricting the input frequency to a maximum of 600 Mhz,
> >> we could use the integer divider to get 400 Mhz (dividing by 2).
> >> However, with the 400 Mhz as input to the frac divider,
> >> we run into the problem, that the maximum possible output frequency
> >> is 20 MHz (there is another restriction which states that the
> >> fraction input : output frequency must be >= 20).
> > 
> > just for clarification, what is the current input frequency you
> > already use sucessfully?
> 
> Our working setup uses the integer divider to reduce to 400 MHz
> and uses the frac divider to get something near 24.56 MHz.
> I have to admit I have never measured what's on the clock line.

Ah ok, so the 600MHz input maximum is ok for your setup and the "< 20"
ratio is the question. I thought I remembered you using a higher than
600MHz input rate. [bad memory on my side]




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 5/6] clk: rockchip: add pll up and down when change pll freq

[Doug Anderson]

Hi,

On Fri, Apr 12, 2019 at 5:16 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi Elaine,
>
> Am Mittwoch, 3. April 2019, 11:44:09 CEST schrieb Elaine Zhang:
> > set pll sequence:
> >       ->set pll to slow mode or other plls
> >       ->set pll down
> >       ->set pll params
> >       ->set pll up
> >       ->wait pll lock status
> >       ->set pll to normal mode
> >
> > To slove the system error:
> > wait_pll_lock: timeout waiting for pll to lock
> > pll_set_params: pll update unsucessful,
> >               trying to restore old params
>
> Can you tell me on what soc this was experienced?
>
> The patch includes rk3399, but I don't think the CrOS kernel
> does powerdown the pll when changing the cpu-frequency
> [added Doug and Brian for clarification and possible testing :-) ]

As far as I can tell you're right.  We don't seem to have it and I'm
not aware of problems.


> But I did find that the M0 code in ATF does actually power-down the
> PLL and follow your outline from above. So essentially I'd just like
> a thumbs up from chromeos people if they have the time.

It does seem like it should be fine in general to do it.  It's one
extra step but presumably it should be fine.

In general the Rockchip PLL programming guidelines have always been a
bit funny.  Looking at the version of the doc I have, I see phrases
like "The PLL programming support changed on-the-fly and the PLL will
simply slew to the new frequency" which makes me feel like you're
supposed to be able to change the PLL frequency without powering down.
This is repeated in another part of the manual which talks about the
glitches that can happen when changing the PLL on the fly: it doesn't
say not to do it, it just says to expect glitches (which can be
avoided by changing the parent first).

...but then in another section of the doc it talks about asserting PD
before doing a frequency change!  :-P

Though in that same section it says: "Release PD after no less than
1us from the time it was asserted."  Even though probably 1 us has
passed, I'd still expect a udelay(1) to be explicit here.


One other thing that concerns me a little about this patch is that I
wonder if it is legal to call rockchip_rk3399_pll_set_params() while
the PLL is off.  AKA is it OK to change the rate of a PLL while it is
not enabled?  I'm not saying that this would have worked before
(actually, you might end up hitting the exact error "timeout waiting
for pll to lock"), but now it seems even worse because we'll
implicitly turning on the PLL.  ...a part of me wonders if this is the
root cause of the problem Elaine's patch is trying to solve: that some
code was trying to set the rate of a PLL before enabling it.


So, tl; dr:
* I doubt this patch is needed on rk3399, but it probably won't hurt.
* If you're going to do the power down, you should add the udelay()
* There's a bug on 3036.  See below.
* You should change your patch so it doesn't enable the PLL if it
wasn't already enabled.


> > Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> > ---
> >  drivers/clk/rockchip/clk-pll.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
> > index dd0433d4753e..9fe1227e77e9 100644
> > --- a/drivers/clk/rockchip/clk-pll.c
> > +++ b/drivers/clk/rockchip/clk-pll.c
> > @@ -208,6 +208,11 @@ static int rockchip_rk3036_pll_set_params(struct rockchip_clk_pll *pll,
> >               rate_change_remuxed = 1;
> >       }
> >
> > +     /* set pll power down */
> > +     writel(HIWORD_UPDATE(1,
> > +                          RK3036_PLLCON1_PWRDOWN, 13),

This does not do what you think it does.  It should be:

HIWORD_UPDATE(RK3036_PLLCON1_PWRDOWN,
     RK3036_PLLCON1_PWRDOWN, 0)

...without that my compiler yells at me:

signed shift result (0x40000000000) requires 44 bits to represent

...and the compiler is, indeed, correct.


> > +            pll->reg_base + RK3036_PLLCON(1));
> > +
> >       /* update pll values */
> >       writel_relaxed(HIWORD_UPDATE(rate->fbdiv, RK3036_PLLCON0_FBDIV_MASK,
> >                                         RK3036_PLLCON0_FBDIV_SHIFT) |
> > @@ -229,6 +234,10 @@ static int rockchip_rk3036_pll_set_params(struct rockchip_clk_pll *pll,
> >       pllcon |= rate->frac << RK3036_PLLCON2_FRAC_SHIFT;
> >       writel_relaxed(pllcon, pll->reg_base + RK3036_PLLCON(2));
> >
> > +     /* set pll power up */
> > +     writel(HIWORD_UPDATE(0, RK3036_PLLCON1_PWRDOWN, 13),
> > +            pll->reg_base + RK3036_PLLCON(1));

In a similar vein, the above should be:

writel(HIWORD_UPDATE(0, RK3036_PLLCON1_PWRDOWN, 0),

...since RK3036_PLLCON1_PWRDOWN already has the shift.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel