[PATCH 0/4] clk: sunxi-ng: Fix issues with fractional mode

[PATCH 0/4] clk: sunxi-ng: Fix issues with fractional mode

[Jernej Skrabec]

During development of H3 HDMI driver, I found some issues with
setting video clock rate. It turned out that clock driver decided
to use fractional mode and selected right frequency, but it didn't
enable it. Additionally, fractional helpers don't wait on lock.

Patch 1 adds a call to N-M clock driver which enables fractional mode.
It also clears M factor before that as required by most SoCs.

Patch 2 adds a call to N clock driver which enables fractional mode.

Patch 3 makes fractional helpers less chatty.

Patch 4 adds a wait for lock to fractional helpers.

Jernej Skrabec (4):
  clk: sunxi-ng: Fix fractional mode for N-M clocks
  clk: sunxi-ng: multiplier: Fix fractional mode
  clk: sunxi-ng: Make fractional helper less chatty
  clk: sunxi-ng: Wait for lock when using fractional mode

 drivers/clk/sunxi-ng/ccu_frac.c | 14 ++++++++------
 drivers/clk/sunxi-ng/ccu_frac.h |  2 +-
 drivers/clk/sunxi-ng/ccu_mult.c | 10 +++++++---
 drivers/clk/sunxi-ng/ccu_nm.c   | 19 ++++++++++++++++---
 4 files changed, 32 insertions(+), 13 deletions(-)

-- 
2.13.3

[PATCH 1/4] clk: sunxi-ng: Fix fractional mode for N-M clocks

[Jernej Skrabec]

N-M factor clock driver is missing a call to ccu_frac_helper_enable()
when fractional mode is used. Additionally, most SoCs require that M
factor must be set to 0 when fractional mode is used.

Without this patch, clock keeps the old value and clk_set_rate() returns
without error.

Fixes: 6174a1e24b0d ("clk: sunxi-ng: Add N-M-factor clock support")
CC: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/clk/sunxi-ng/ccu_nm.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu_nm.c b/drivers/clk/sunxi-ng/ccu_nm.c
index 5e5e90a4a50c..c6ba866b99d3 100644
--- a/drivers/clk/sunxi-ng/ccu_nm.c
+++ b/drivers/clk/sunxi-ng/ccu_nm.c
@@ -117,10 +117,22 @@ static int ccu_nm_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long flags;
 	u32 reg;
 
-	if (ccu_frac_helper_has_rate(&nm->common, &nm->frac, rate))
+	if (ccu_frac_helper_has_rate(&nm->common, &nm->frac, rate)) {
+		spin_lock_irqsave(nm->common.lock, flags);
+
+		/* most SoCs require M to be 0 if fractional mode is used */
+		reg = readl(nm->common.base + nm->common.reg);
+		reg &= ~GENMASK(nm->m.width + nm->m.shift - 1, nm->m.shift);
+		writel(reg, nm->common.base + nm->common.reg);
+
+		spin_unlock_irqrestore(nm->common.lock, flags);
+
+		ccu_frac_helper_enable(&nm->common, &nm->frac);
+
 		return ccu_frac_helper_set_rate(&nm->common, &nm->frac, rate);
-	else
+	} else {
 		ccu_frac_helper_disable(&nm->common, &nm->frac);
+	}
 
 	_nm.min_n = nm->n.min ?: 1;
 	_nm.max_n = nm->n.max ?: 1 << nm->n.width;
-- 
2.13.3

[PATCH 2/4] clk: sunxi-ng: multiplier: Fix fractional mode

[Jernej Skrabec]

Driver for multiplier clock is missing a call to
ccu_frac_helper_enable() when fractional mode is selected.

Add a call to ccu_frac_helper_enable().

Fixes: d77e8135b340 ("clk: sunxi-ng: multiplier: Add fractional support")
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/clk/sunxi-ng/ccu_mult.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu_mult.c b/drivers/clk/sunxi-ng/ccu_mult.c
index 20d0300867f2..ee5e96222cb2 100644
--- a/drivers/clk/sunxi-ng/ccu_mult.c
+++ b/drivers/clk/sunxi-ng/ccu_mult.c
@@ -111,10 +111,13 @@ static int ccu_mult_set_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long flags;
 	u32 reg;
 
-	if (ccu_frac_helper_has_rate(&cm->common, &cm->frac, rate))
+	if (ccu_frac_helper_has_rate(&cm->common, &cm->frac, rate)) {
+		ccu_frac_helper_enable(&cm->common, &cm->frac);
+
 		return ccu_frac_helper_set_rate(&cm->common, &cm->frac, rate);
-	else
+	} else {
 		ccu_frac_helper_disable(&cm->common, &cm->frac);
+	}
 
 	parent_rate = ccu_mux_helper_apply_prediv(&cm->common, &cm->mux, -1,
 						  parent_rate);
-- 
2.13.3

[PATCH 3/4] clk: sunxi-ng: Make fractional helper less chatty

[Jernej Skrabec]

ccu_frac_helper_read_rate() prints some info which is not really
helpful except during debugging.

Replace printk() with pr_debug().

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/clk/sunxi-ng/ccu_frac.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu_frac.c b/drivers/clk/sunxi-ng/ccu_frac.c
index 8b5eb7756bf7..ff9e72dc5337 100644
--- a/drivers/clk/sunxi-ng/ccu_frac.c
+++ b/drivers/clk/sunxi-ng/ccu_frac.c
@@ -67,18 +67,18 @@ unsigned long ccu_frac_helper_read_rate(struct ccu_common *common,
 {
 	u32 reg;
 
-	printk("%s: Read fractional\n", clk_hw_get_name(&common->hw));
+	pr_debug("%s: Read fractional\n", clk_hw_get_name(&common->hw));
 
 	if (!(common->features & CCU_FEATURE_FRACTIONAL))
 		return 0;
 
-	printk("%s: clock is fractional (rates %lu and %lu)\n",
-	       clk_hw_get_name(&common->hw), cf->rates[0], cf->rates[1]);
+	pr_debug("%s: clock is fractional (rates %lu and %lu)\n",
+		 clk_hw_get_name(&common->hw), cf->rates[0], cf->rates[1]);
 
 	reg = readl(common->base + common->reg);
 
-	printk("%s: clock reg is 0x%x (select is 0x%x)\n",
-	       clk_hw_get_name(&common->hw), reg, cf->select);
+	pr_debug("%s: clock reg is 0x%x (select is 0x%x)\n",
+		 clk_hw_get_name(&common->hw), reg, cf->select);
 
 	return (reg & cf->select) ? cf->rates[1] : cf->rates[0];
 }
-- 
2.13.3

[PATCH 4/4] clk: sunxi-ng: Wait for lock when using fractional mode

[Jernej Skrabec]

Currently ccu_frac_helper_set_rate() doesn't wait for a lock bit to be
set before returning. Because of that, unstable clock may be used.

Add a wait for lock in the helper function.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/clk/sunxi-ng/ccu_frac.c | 4 +++-
 drivers/clk/sunxi-ng/ccu_frac.h | 2 +-
 drivers/clk/sunxi-ng/ccu_mult.c | 3 ++-
 drivers/clk/sunxi-ng/ccu_nm.c   | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu_frac.c b/drivers/clk/sunxi-ng/ccu_frac.c
index ff9e72dc5337..d1d168d4c4f0 100644
--- a/drivers/clk/sunxi-ng/ccu_frac.c
+++ b/drivers/clk/sunxi-ng/ccu_frac.c
@@ -85,7 +85,7 @@ unsigned long ccu_frac_helper_read_rate(struct ccu_common *common,
 
 int ccu_frac_helper_set_rate(struct ccu_common *common,
 			     struct ccu_frac_internal *cf,
-			     unsigned long rate)
+			     unsigned long rate, u32 lock)
 {
 	unsigned long flags;
 	u32 reg, sel;
@@ -106,5 +106,7 @@ int ccu_frac_helper_set_rate(struct ccu_common *common,
 	writel(reg | sel, common->base + common->reg);
 	spin_unlock_irqrestore(common->lock, flags);
 
+	ccu_helper_wait_for_lock(common, lock);
+
 	return 0;
 }
diff --git a/drivers/clk/sunxi-ng/ccu_frac.h b/drivers/clk/sunxi-ng/ccu_frac.h
index 7b1ee380156f..efe2dd6bac01 100644
--- a/drivers/clk/sunxi-ng/ccu_frac.h
+++ b/drivers/clk/sunxi-ng/ccu_frac.h
@@ -48,6 +48,6 @@ unsigned long ccu_frac_helper_read_rate(struct ccu_common *common,
 
 int ccu_frac_helper_set_rate(struct ccu_common *common,
 			     struct ccu_frac_internal *cf,
-			     unsigned long rate);
+			     unsigned long rate, u32 lock);
 
 #endif /* _CCU_FRAC_H_ */
diff --git a/drivers/clk/sunxi-ng/ccu_mult.c b/drivers/clk/sunxi-ng/ccu_mult.c
index ee5e96222cb2..12e0783caee6 100644
--- a/drivers/clk/sunxi-ng/ccu_mult.c
+++ b/drivers/clk/sunxi-ng/ccu_mult.c
@@ -114,7 +114,8 @@ static int ccu_mult_set_rate(struct clk_hw *hw, unsigned long rate,
 	if (ccu_frac_helper_has_rate(&cm->common, &cm->frac, rate)) {
 		ccu_frac_helper_enable(&cm->common, &cm->frac);
 
-		return ccu_frac_helper_set_rate(&cm->common, &cm->frac, rate);
+		return ccu_frac_helper_set_rate(&cm->common, &cm->frac,
+						rate, cm->lock);
 	} else {
 		ccu_frac_helper_disable(&cm->common, &cm->frac);
 	}
diff --git a/drivers/clk/sunxi-ng/ccu_nm.c b/drivers/clk/sunxi-ng/ccu_nm.c
index c6ba866b99d3..a32158e8f2e3 100644
--- a/drivers/clk/sunxi-ng/ccu_nm.c
+++ b/drivers/clk/sunxi-ng/ccu_nm.c
@@ -129,7 +129,8 @@ static int ccu_nm_set_rate(struct clk_hw *hw, unsigned long rate,
 
 		ccu_frac_helper_enable(&nm->common, &nm->frac);
 
-		return ccu_frac_helper_set_rate(&nm->common, &nm->frac, rate);
+		return ccu_frac_helper_set_rate(&nm->common, &nm->frac,
+						rate, nm->lock);
 	} else {
 		ccu_frac_helper_disable(&nm->common, &nm->frac);
 	}
-- 
2.13.3

[PATCH 1/4] clk: sunxi-ng: Fix fractional mode for N-M clocks

[Chen-Yu Tsai]

On Mon, Jul 31, 2017 at 12:41 AM, Jernej Skrabec
<jernej.skrabec@siol.net> wrote:
> N-M factor clock driver is missing a call to ccu_frac_helper_enable()
> when fractional mode is used. Additionally, most SoCs require that M
> factor must be set to 0 when fractional mode is used.
>
> Without this patch, clock keeps the old value and clk_set_rate() returns
> without error.
>
> Fixes: 6174a1e24b0d ("clk: sunxi-ng: Add N-M-factor clock support")
> CC: Maxime Ripard <maxime.ripard@free-electrons.com>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Queued as a fix for 4.13.

ChenYu

[PATCH 4/4] clk: sunxi-ng: Wait for lock when using fractional mode

[Chen-Yu Tsai]

On Mon, Jul 31, 2017 at 12:41 AM, Jernej Skrabec
<jernej.skrabec@siol.net> wrote:
> Currently ccu_frac_helper_set_rate() doesn't wait for a lock bit to be
> set before returning. Because of that, unstable clock may be used.
>
> Add a wait for lock in the helper function.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Can you provide a Fixes: tag?

Otherwise this looks good.

[PATCH 0/4] clk: sunxi-ng: Fix issues with fractional mode

[Chen-Yu Tsai]

Hi Jernej,

On Mon, Jul 31, 2017 at 12:41 AM, Jernej Skrabec
<jernej.skrabec@siol.net> wrote:
> During development of H3 HDMI driver, I found some issues with
> setting video clock rate. It turned out that clock driver decided
> to use fractional mode and selected right frequency, but it didn't
> enable it. Additionally, fractional helpers don't wait on lock.

What kind of resolution were you testing to actually hit this?

AFAIK the fractional mode is either 297 or 270 MHz. Even Full HD
1080p60 dot clocks aren't that high. And the clk drivers should
try to request a matching parent clk rate. So the PLL wouldn't
go that high. Are you testing 4k @ 30fps?

As it stands, I don't think any of the existing display support
can go that high, so I think we're safe as far as old kernels
go, i.e. we don't need to Cc stable for these.

Regards
ChenYu

> Patch 1 adds a call to N-M clock driver which enables fractional mode.
> It also clears M factor before that as required by most SoCs.
>
> Patch 2 adds a call to N clock driver which enables fractional mode.
>
> Patch 3 makes fractional helpers less chatty.
>
> Patch 4 adds a wait for lock to fractional helpers.
>
> Jernej Skrabec (4):
>   clk: sunxi-ng: Fix fractional mode for N-M clocks
>   clk: sunxi-ng: multiplier: Fix fractional mode
>   clk: sunxi-ng: Make fractional helper less chatty
>   clk: sunxi-ng: Wait for lock when using fractional mode
>
>  drivers/clk/sunxi-ng/ccu_frac.c | 14 ++++++++------
>  drivers/clk/sunxi-ng/ccu_frac.h |  2 +-
>  drivers/clk/sunxi-ng/ccu_mult.c | 10 +++++++---
>  drivers/clk/sunxi-ng/ccu_nm.c   | 19 ++++++++++++++++---
>  4 files changed, 32 insertions(+), 13 deletions(-)
>
> --
> 2.13.3
>

[PATCH 2/4] clk: sunxi-ng: multiplier: Fix fractional mode

[Chen-Yu Tsai]

On Mon, Jul 31, 2017 at 12:41 AM, Jernej Skrabec
<jernej.skrabec@siol.net> wrote:
> Driver for multiplier clock is missing a call to
> ccu_frac_helper_enable() when fractional mode is selected.
>
> Add a call to ccu_frac_helper_enable().
>
> Fixes: d77e8135b340 ("clk: sunxi-ng: multiplier: Add fractional support")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Queued as a fix for 4.13.

ChenYu

[PATCH 3/4] clk: sunxi-ng: Make fractional helper less chatty

[Chen-Yu Tsai]

()On Mon, Jul 31, 2017 at 12:41 AM, Jernej Skrabec
<jernej.skrabec@siol.net> wrote:
> ccu_frac_helper_read_rate() prints some info which is not really
> helpful except during debugging.
>
> Replace printk() with pr_debug().
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Queued as a fix for 4.13 with the following tag:

Fixes: 89a3dfb78707 ("clk: sunxi-ng: Add fractional lib")

The rationale being the previous two patches actually enable
the ccu_frac_helper_read_rate() code path, and we don't want
the users to be annoyed by all the noise.

ChenYu

[linux-sunxi] Re: [PATCH 0/4] clk: sunxi-ng: Fix issues with fractional mode

[Jernej Škrabec]

Hi Chen-Yu,

Dne ponedeljek, 31. julij 2017 ob 07:13:34 CEST je Chen-Yu Tsai napisal(a):
> Hi Jernej,
> 
> On Mon, Jul 31, 2017 at 12:41 AM, Jernej Skrabec
> 
> <jernej.skrabec@siol.net> wrote:
> > During development of H3 HDMI driver, I found some issues with
> > setting video clock rate. It turned out that clock driver decided
> > to use fractional mode and selected right frequency, but it didn't
> > enable it. Additionally, fractional helpers don't wait on lock.
> 
> What kind of resolution were you testing to actually hit this?

1920x1080p @ 60Hz

> 
> AFAIK the fractional mode is either 297 or 270 MHz. Even Full HD
> 1080p60 dot clocks aren't that high. And the clk drivers should
> try to request a matching parent clk rate. So the PLL wouldn't
> go that high. Are you testing 4k @ 30fps?

No, it is a bit more complicated than that. H3's HDMI PHY is proprietary and 
register meanings are not known well. Because of that, I'm using values found 
in BSP driver. Those values include pixel clock divider. BSP driver always use 
297 MHz as a base and uses dividers in PHY to prepare right pixel clock. So 
the case for 1080p is 297 MHz / 2 = 148.5 MHz.

> 
> As it stands, I don't think any of the existing display support
> can go that high, so I think we're safe as far as old kernels
> go, i.e. we don't need to Cc stable for these.

Ok.

Regards,
Jernej

[PATCH 4/4] clk: sunxi-ng: Wait for lock when using fractional mode

[Jernej Škrabec]

Hi Chen-Yu,

Dne ponedeljek, 31. julij 2017 ob 07:02:18 CEST je Chen-Yu Tsai napisal(a):
> On Mon, Jul 31, 2017 at 12:41 AM, Jernej Skrabec
> 
> <jernej.skrabec@siol.net> wrote:
> > Currently ccu_frac_helper_set_rate() doesn't wait for a lock bit to be
> > set before returning. Because of that, unstable clock may be used.
> > 
> > Add a wait for lock in the helper function.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> 
> Can you provide a Fixes: tag?

fixes: 89a3dfb78707 ("clk: sunxi-ng: Add fractional lib")

As we discussed on IRC, it is safe to call ccu_helper_wait_for_lock() even on 
platforms where lock bit is not present, since it just returns without effect.

Regards,
Jernej

> 
> Otherwise this looks good.

[linux-sunxi] Re: [PATCH 0/4] clk: sunxi-ng: Fix issues with fractional mode

[Chen-Yu Tsai]

On Tue, Aug 1, 2017 at 12:50 AM, Jernej ?krabec <jernej.skrabec@siol.net> wrote:
> Hi Chen-Yu,
>
> Dne ponedeljek, 31. julij 2017 ob 07:13:34 CEST je Chen-Yu Tsai napisal(a):
>> Hi Jernej,
>>
>> On Mon, Jul 31, 2017 at 12:41 AM, Jernej Skrabec
>>
>> <jernej.skrabec@siol.net> wrote:
>> > During development of H3 HDMI driver, I found some issues with
>> > setting video clock rate. It turned out that clock driver decided
>> > to use fractional mode and selected right frequency, but it didn't
>> > enable it. Additionally, fractional helpers don't wait on lock.
>>
>> What kind of resolution were you testing to actually hit this?
>
> 1920x1080p @ 60Hz
>
>>
>> AFAIK the fractional mode is either 297 or 270 MHz. Even Full HD
>> 1080p60 dot clocks aren't that high. And the clk drivers should
>> try to request a matching parent clk rate. So the PLL wouldn't
>> go that high. Are you testing 4k @ 30fps?
>
> No, it is a bit more complicated than that. H3's HDMI PHY is proprietary and
> register meanings are not known well. Because of that, I'm using values found
> in BSP driver. Those values include pixel clock divider. BSP driver always use
> 297 MHz as a base and uses dividers in PHY to prepare right pixel clock. So
> the case for 1080p is 297 MHz / 2 = 148.5 MHz.

I see. So for the current in kernel users, none would be able to
hit the fractional mode clock rates. HDMI on sun5i is limited to
1080p60, and we don't support pixel doubling. LCD dotclocks have
a minimal /6 divider, though it doesn't seem likely they will hit
this either, or we would have seen someone complain. And these
drivers were introduced well before 4.13-rc1. The clk maintainers
really like to have just critical fixes, and fixes for stuff
added in the current release. So I'm re-queuing these 4 patches
for 4.14.

Thanks
ChenYu

>
>>
>> As it stands, I don't think any of the existing display support
>> can go that high, so I think we're safe as far as old kernels
>> go, i.e. we don't need to Cc stable for these.
>
> Ok.
>
> Regards,
> Jernej
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.