Re: [PATCH 1/7] clk: sunxi: Add post clk divider for factor clocks

Hi Emilio,

On Sat, Sep 13, 2014 at 11:43:46AM -0300, Emilio L?pez wrote:

Hi,

El 06/09/14 a las 07:47, Chen-Yu Tsai escibi?:

quoted

Some factor clocks, mostly PLLs, have an extra fixed divider just before
the clock output. Add an option to the factor clk driver config data to
specify this divider.

Signed-off-by: Chen-Yu Tsai <redacted>
---
 drivers/clk/sunxi/clk-factors.c | 3 +++
 drivers/clk/sunxi/clk-factors.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-factors.c
index 2057c8a..435111d 100644
--- a/drivers/clk/sunxi/clk-factors.c
+++ b/drivers/clk/sunxi/clk-factors.c

@@ -64,6 +64,9 @@ static unsigned long clk_factors_recalc_rate(struct clk_hw *hw,
 	/* Calculate the rate */
 	rate = (parent_rate * (n + config->n_start) * (k + 1) >> p) / (m + 1);

+	if (config->post_div)
+		rate /= config->post_div;
+
 	return rate;
 }

diff --git a/drivers/clk/sunxi/clk-factors.h b/drivers/clk/sunxi/clk-factors.h
index d2d0efa..ce70c65 100644
--- a/drivers/clk/sunxi/clk-factors.h
+++ b/drivers/clk/sunxi/clk-factors.h

@@ -16,6 +16,7 @@ struct clk_factors_config {
 	u8 pshift;
 	u8 pwidth;
 	u8 n_start;
+	u8 post_div;
 };

 struct clk_factors {

For the record, I liked your solution on[1] more, as it's in line
with what we're doing on the other sunxi platforms, instead of
adding features in factors to cover for some cases. But it's your
and Maxime's call, as I haven't written any of the sun6i code so
far.

No, you still wrote most of the clock support, so your opinion is
always valuable (and valued).

Thing is, unlike what was done in the sun4i driver where there was a
"real" technical issue that was preventing us from using only
fixed-factor, we're not in such a case in sun6i (and later,
apparently).

PLL6 has only one output, which is then directly multiplied by
fixed-factors, without any (pre|post)-dividers for any of them.

That means that following what you did for the sun4i would just
register 3 "dumbs" fixed-factors, that we couldn't reference from DT,
or through a cryptic index (which is not even documented in our
bindings).

I'd be fine either way, I just prefer the solution that has less code
and is more explicit.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140916/202624f8/attachment.sig>

Hi Maxime, Emilio,

On Tue, Sep 16, 2014 at 11:57 PM, Maxime Ripard
[off-list ref] wrote:

Hi Emilio,

On Sat, Sep 13, 2014 at 11:43:46AM -0300, Emilio L?pez wrote:

quoted

Hi,

El 06/09/14 a las 07:47, Chen-Yu Tsai escibi?:

quoted

Some factor clocks, mostly PLLs, have an extra fixed divider just before
the clock output. Add an option to the factor clk driver config data to
specify this divider.

Signed-off-by: Chen-Yu Tsai <redacted>
---
 drivers/clk/sunxi/clk-factors.c | 3 +++
 drivers/clk/sunxi/clk-factors.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/clk/sunxi/clk-factors.c b/drivers/clk/sunxi/clk-factors.c
index 2057c8a..435111d 100644
--- a/drivers/clk/sunxi/clk-factors.c
+++ b/drivers/clk/sunxi/clk-factors.c

@@ -64,6 +64,9 @@ static unsigned long clk_factors_recalc_rate(struct clk_hw *hw,
    /* Calculate the rate */
    rate = (parent_rate * (n + config->n_start) * (k + 1) >> p) / (m + 1);

+    if (config->post_div)
+            rate /= config->post_div;
+
    return rate;
 }

diff --git a/drivers/clk/sunxi/clk-factors.h b/drivers/clk/sunxi/clk-factors.h
index d2d0efa..ce70c65 100644
--- a/drivers/clk/sunxi/clk-factors.h
+++ b/drivers/clk/sunxi/clk-factors.h

@@ -16,6 +16,7 @@ struct clk_factors_config {
    u8 pshift;
    u8 pwidth;
    u8 n_start;
+    u8 post_div;
 };

 struct clk_factors {

For the record, I liked your solution on[1] more, as it's in line
with what we're doing on the other sunxi platforms, instead of
adding features in factors to cover for some cases. But it's your
and Maxime's call, as I haven't written any of the sun6i code so
far.

No, you still wrote most of the clock support, so your opinion is
always valuable (and valued).

Thing is, unlike what was done in the sun4i driver where there was a
"real" technical issue that was preventing us from using only
fixed-factor, we're not in such a case in sun6i (and later,
apparently).

PLL6 has only one output, which is then directly multiplied by
fixed-factors, without any (pre|post)-dividers for any of them.

That means that following what you did for the sun4i would just
register 3 "dumbs" fixed-factors, that we couldn't reference from DT,
or through a cryptic index (which is not even documented in our
bindings).

I'd be fine either way, I just prefer the solution that has less code
and is more explicit.

What's the verdict on this series?

ChenYu