[PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Chen-Yu Tsai]

Hi everyone,

This series unifies the mux and divider parts of the AHB1 clock found
on sun6i and sun8i, while also adding support for the pre-divider on
the PLL6 input.

The rate calculation logic must factor in which parent it is using to
calculate the rate, to decide whether to use the pre-divider or not.
This is beyond the original factors clk design in sunxi. To avoid
feature bloat, this is implemented as a seperate composite clk.

The new clock driver is registered with a separate OF_CLK_DECLARE.
This is done so that assigned-clocks* properties on the clk provider
node can actually work. The clock framework arranges the clock setup
order by checking whether all clock parents are available, by checking
the node matching OF_CLK_DECLARE.

However, the sunxi clk driver is based on the root node compatible,
has no defined dependencies (parents), and is setup before the fixed-rate
clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
There is no way to calculate the required clock factors to set a default
clock rate under these circumstances. This happens when we set the
defaults in the clock node (provider), rather than a clock consumer.

I can think of 2 ways to solve the dependency issue, but neither is
pretty. One would be to move the root fixed-rate clocks into the sunxi
clk driver. The other would be separating all the clocks into individual
OF_CLK_DECLARE statements, which adds a lot of boilerplate code.


The contents of this series are as follows:

Patch 1 adds support for a fixed divider on the output of factor clocks,
which is used by the next patch.

Patch 2 fixes PLL6 rate calculation error, due to one of the factor values
starting from 1, instead of 0. It also adds the /2 divider on the output.

Patch 3 adds the unified AHB1 clock driver.

Patch 4 and 5 unify the AHB1 clock nodes on sun6i and sun8i respectively.

Patch 6 sets the default parent and clock rate for AHB1, as required by
the DMA controller. Curiously I did not require this when I tried dmatest
on my A31 Hummingbird.

Patch 7 removes the clk_set_parent calls from the sun6i-dma driver, as
it no longer works, and is replaced by the previous patch.


Suggestions are more than welcome.


Cheers
ChenYu


Chen-Yu Tsai (7):
  clk: sunxi: Add post clk divider for factor clocks
  clk: sunxi: Fix PLL6 calculation on sun6i
  clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider
  ARM: dts: sun8i: Unify ahb1 clock nodes
  ARM: dts: sun6i: Unify ahb1 clock nodes
  ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma
    controller
  dmaengine: sun6i: Remove obsolete clk muxing code

 Documentation/devicetree/bindings/clock/sunxi.txt |   2 +-
 arch/arm/boot/dts/sun6i-a31.dtsi                  |  17 +-
 arch/arm/boot/dts/sun8i-a23.dtsi                  |  12 +-
 drivers/clk/sunxi/clk-factors.c                   |   3 +
 drivers/clk/sunxi/clk-factors.h                   |   1 +
 drivers/clk/sunxi/clk-sunxi.c                     | 215 +++++++++++++++++++++-
 drivers/dma/sun6i-dma.c                           |  23 ---
 7 files changed, 227 insertions(+), 46 deletions(-)

-- 
2.1.0

[PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Chen-Yu Tsai]

Hi everyone,

This series unifies the mux and divider parts of the AHB1 clock found
on sun6i and sun8i, while also adding support for the pre-divider on
the PLL6 input.

The rate calculation logic must factor in which parent it is using to
calculate the rate, to decide whether to use the pre-divider or not.
This is beyond the original factors clk design in sunxi. To avoid
feature bloat, this is implemented as a seperate composite clk.

The new clock driver is registered with a separate OF_CLK_DECLARE.
This is done so that assigned-clocks* properties on the clk provider
node can actually work. The clock framework arranges the clock setup
order by checking whether all clock parents are available, by checking
the node matching OF_CLK_DECLARE.

However, the sunxi clk driver is based on the root node compatible,
has no defined dependencies (parents), and is setup before the fixed-rate
clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
There is no way to calculate the required clock factors to set a default
clock rate under these circumstances. This happens when we set the
defaults in the clock node (provider), rather than a clock consumer.

I can think of 2 ways to solve the dependency issue, but neither is
pretty. One would be to move the root fixed-rate clocks into the sunxi
clk driver. The other would be separating all the clocks into individual
OF_CLK_DECLARE statements, which adds a lot of boilerplate code.


The contents of this series are as follows:

Patch 1 adds support for a fixed divider on the output of factor clocks,
which is used by the next patch.

Patch 2 fixes PLL6 rate calculation error, due to one of the factor values
starting from 1, instead of 0. It also adds the /2 divider on the output.

Patch 3 adds the unified AHB1 clock driver.

Patch 4 and 5 unify the AHB1 clock nodes on sun6i and sun8i respectively.

Patch 6 sets the default parent and clock rate for AHB1, as required by
the DMA controller. Curiously I did not require this when I tried dmatest
on my A31 Hummingbird.

Patch 7 removes the clk_set_parent calls from the sun6i-dma driver, as
it no longer works, and is replaced by the previous patch.


Suggestions are more than welcome.


Cheers
ChenYu


Chen-Yu Tsai (7):
  clk: sunxi: Add post clk divider for factor clocks
  clk: sunxi: Fix PLL6 calculation on sun6i
  clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider
  ARM: dts: sun8i: Unify ahb1 clock nodes
  ARM: dts: sun6i: Unify ahb1 clock nodes
  ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma
    controller
  dmaengine: sun6i: Remove obsolete clk muxing code

 Documentation/devicetree/bindings/clock/sunxi.txt |   2 +-
 arch/arm/boot/dts/sun6i-a31.dtsi                  |  17 +-
 arch/arm/boot/dts/sun8i-a23.dtsi                  |  12 +-
 drivers/clk/sunxi/clk-factors.c                   |   3 +
 drivers/clk/sunxi/clk-factors.h                   |   1 +
 drivers/clk/sunxi/clk-sunxi.c                     | 215 +++++++++++++++++++++-
 drivers/dma/sun6i-dma.c                           |  23 ---
 7 files changed, 227 insertions(+), 46 deletions(-)

-- 
2.1.0

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

[Chen-Yu Tsai]

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 <wens-jdAy2FN1RRM@public.gmane.org>
---
 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 {
-- 
2.1.0

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

[Chen-Yu Tsai]

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 <wens@csie.org>
---
 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 {
-- 
2.1.0

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

On Sat, Sep 06, 2014 at 06:47:22PM +0800, Chen-Yu Tsai wrote:
> 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 <wens-jdAy2FN1RRM@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Maxime Ripard]

On Sat, Sep 06, 2014 at 06:47:22PM +0800, Chen-Yu Tsai wrote:
> 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 <wens@csie.org>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
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/20140911/cf136ac2/attachment.sig>

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

[Emilio López]

Hi,

El 06/09/14 a las 07:47, Chen-Yu Tsai escibió:
> 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 <wens-jdAy2FN1RRM@public.gmane.org>
> ---
>   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.

Cheers!

Emilio

[1] https://patchwork.kernel.org/patch/4228541/

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Emilio López]

Hi,

El 06/09/14 a las 07:47, Chen-Yu Tsai escibi?:
> 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 <wens@csie.org>
> ---
>   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.

Cheers!

Emilio

[1] https://patchwork.kernel.org/patch/4228541/

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

[Chen-Yu Tsai]

Hi,

On Sat, Sep 13, 2014 at 10:43 PM, Emilio López <emilio-0Z03zUJReD5OxF6Tv1QG9Q@public.gmane.org> wrote:
> Hi,
>
> El 06/09/14 a las 07:47, Chen-Yu Tsai escibió:
>
>> 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 <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>>   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.

I'm OK either way. It's really up to Maxime whether he likes the DT
representation.

> Cheers!
>
> Emilio
>
> [1] https://patchwork.kernel.org/patch/4228541/

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Chen-Yu Tsai]

Hi,

On Sat, Sep 13, 2014 at 10:43 PM, Emilio L?pez <emilio@elopez.com.ar> wrote:
> Hi,
>
> El 06/09/14 a las 07:47, Chen-Yu Tsai escibi?:
>
>> 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 <wens@csie.org>
>> ---
>>   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.

I'm OK either way. It's really up to Maxime whether he likes the DT
representation.

> Cheers!
>
> Emilio
>
> [1] https://patchwork.kernel.org/patch/4228541/

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2564 bytes --]

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ó:
> >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 <wens-jdAy2FN1RRM@public.gmane.org>
> >---
> >  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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Maxime Ripard]

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?:
> >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 <wens@csie.org>
> >---
> >  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>

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

[Chen-Yu Tsai]

Hi Maxime, Emilio,

On Tue, Sep 16, 2014 at 11:57 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 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ó:
>> >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 <wens-jdAy2FN1RRM@public.gmane.org>
>> >---
>> >  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

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Chen-Yu Tsai]

Hi Maxime, Emilio,

On Tue, Sep 16, 2014 at 11:57 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> 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?:
>> >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 <wens@csie.org>
>> >---
>> >  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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3174 bytes --]

On Wed, Sep 24, 2014 at 11:35:58PM +0800, Chen-Yu Tsai wrote:
> Hi Maxime, Emilio,
> 
> On Tue, Sep 16, 2014 at 11:57 PM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > 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ó:
> >> >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 <wens-jdAy2FN1RRM@public.gmane.org>
> >> >---
> >> >  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?

If Emilio prefers to have a single clock node, fine. But please update
the DT bindings documentation.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Maxime Ripard]

On Wed, Sep 24, 2014 at 11:35:58PM +0800, Chen-Yu Tsai wrote:
> Hi Maxime, Emilio,
> 
> On Tue, Sep 16, 2014 at 11:57 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > 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?:
> >> >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 <wens@csie.org>
> >> >---
> >> >  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?

If Emilio prefers to have a single clock node, fine. But please update
the DT bindings documentation.

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/20140927/b6bd7df0/attachment.sig>

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

[Chen-Yu Tsai]

On Sat, Sep 27, 2014 at 3:07 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Wed, Sep 24, 2014 at 11:35:58PM +0800, Chen-Yu Tsai wrote:
>> Hi Maxime, Emilio,
>>
>> On Tue, Sep 16, 2014 at 11:57 PM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > 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ó:
>> >> >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 <wens-jdAy2FN1RRM@public.gmane.org>
>> >> >---
>> >> >  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?
>
> If Emilio prefers to have a single clock node, fine. But please update
> the DT bindings documentation.

OK. Then this series needs a bit of work. I'll pick the orignal
divs clocks patch from patchwork (as I didn't keep one) and fix
up the DT.


ChenYu

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

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

[Chen-Yu Tsai]

On Sat, Sep 27, 2014 at 3:07 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Sep 24, 2014 at 11:35:58PM +0800, Chen-Yu Tsai wrote:
>> Hi Maxime, Emilio,
>>
>> On Tue, Sep 16, 2014 at 11:57 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > 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?:
>> >> >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 <wens@csie.org>
>> >> >---
>> >> >  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?
>
> If Emilio prefers to have a single clock node, fine. But please update
> the DT bindings documentation.

OK. Then this series needs a bit of work. I'll pick the orignal
divs clocks patch from patchwork (as I didn't keep one) and fix
up the DT.


ChenYu

[PATCH 2/7] clk: sunxi: Fix PLL6 calculation on sun6i

[Chen-Yu Tsai]

The N factor for PLL6 counts from 1 to 32, as specified in the A23
manual, and shown in Allwinner's original A31 code.

Also the PLL6 factors alone calculate the clock rate for PLL6x2, not
the normal halved output for PLL6. This is what the factors clk
.recalc_rate callback expects.

This patch fixes the N factor in the clock driver, and adds a post
PLL divider of 2 to calculate the rate for PLL6.

A further patch (to the DT) should add a fixed-factor x2 clock as
the PLL6x2 output.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 drivers/clk/sunxi/clk-sunxi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index b654b7b..be9ac07 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -246,7 +246,7 @@ static void sun4i_get_pll5_factors(u32 *freq, u32 parent_rate,
 /**
  * sun6i_a31_get_pll6_factors() - calculates n, k factors for A31 PLL6
  * PLL6 rate is calculated as follows
- * rate = parent_rate * n * (k + 1) / 2
+ * rate = parent_rate * (n + 1) * (k + 1) / 2
  * parent_rate is always 24Mhz
  */
 
@@ -273,7 +273,7 @@ static void sun6i_a31_get_pll6_factors(u32 *freq, u32 parent_rate,
 	if (*k > 3)
 		*k = 3;
 
-	*n = DIV_ROUND_UP(div, (*k+1));
+	*n = DIV_ROUND_UP(div, (*k+1)) - 1;
 }
 
 /**
@@ -494,6 +494,8 @@ static struct clk_factors_config sun6i_a31_pll6_config = {
 	.nwidth = 5,
 	.kshift = 4,
 	.kwidth = 2,
+	.n_start = 1,
+	.post_div = 2,
 };
 
 static struct clk_factors_config sun4i_apb1_config = {
-- 
2.1.0

[PATCH 2/7] clk: sunxi: Fix PLL6 calculation on sun6i

[Chen-Yu Tsai]

The N factor for PLL6 counts from 1 to 32, as specified in the A23
manual, and shown in Allwinner's original A31 code.

Also the PLL6 factors alone calculate the clock rate for PLL6x2, not
the normal halved output for PLL6. This is what the factors clk
.recalc_rate callback expects.

This patch fixes the N factor in the clock driver, and adds a post
PLL divider of 2 to calculate the rate for PLL6.

A further patch (to the DT) should add a fixed-factor x2 clock as
the PLL6x2 output.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/clk/sunxi/clk-sunxi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index b654b7b..be9ac07 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -246,7 +246,7 @@ static void sun4i_get_pll5_factors(u32 *freq, u32 parent_rate,
 /**
  * sun6i_a31_get_pll6_factors() - calculates n, k factors for A31 PLL6
  * PLL6 rate is calculated as follows
- * rate = parent_rate * n * (k + 1) / 2
+ * rate = parent_rate * (n + 1) * (k + 1) / 2
  * parent_rate is always 24Mhz
  */
 
@@ -273,7 +273,7 @@ static void sun6i_a31_get_pll6_factors(u32 *freq, u32 parent_rate,
 	if (*k > 3)
 		*k = 3;
 
-	*n = DIV_ROUND_UP(div, (*k+1));
+	*n = DIV_ROUND_UP(div, (*k+1)) - 1;
 }
 
 /**
@@ -494,6 +494,8 @@ static struct clk_factors_config sun6i_a31_pll6_config = {
 	.nwidth = 5,
 	.kshift = 4,
 	.kwidth = 2,
+	.n_start = 1,
+	.post_div = 2,
 };
 
 static struct clk_factors_config sun4i_apb1_config = {
-- 
2.1.0

Re: [PATCH 2/7] clk: sunxi: Fix PLL6 calculation on sun6i

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

On Sat, Sep 06, 2014 at 06:47:23PM +0800, Chen-Yu Tsai wrote:
> The N factor for PLL6 counts from 1 to 32, as specified in the A23
> manual, and shown in Allwinner's original A31 code.
> 
> Also the PLL6 factors alone calculate the clock rate for PLL6x2, not
> the normal halved output for PLL6. This is what the factors clk
> .recalc_rate callback expects.
> 
> This patch fixes the N factor in the clock driver, and adds a post
> PLL divider of 2 to calculate the rate for PLL6.
> 
> A further patch (to the DT) should add a fixed-factor x2 clock as
> the PLL6x2 output.
> 
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/7] clk: sunxi: Fix PLL6 calculation on sun6i

[Maxime Ripard]

On Sat, Sep 06, 2014 at 06:47:23PM +0800, Chen-Yu Tsai wrote:
> The N factor for PLL6 counts from 1 to 32, as specified in the A23
> manual, and shown in Allwinner's original A31 code.
> 
> Also the PLL6 factors alone calculate the clock rate for PLL6x2, not
> the normal halved output for PLL6. This is what the factors clk
> .recalc_rate callback expects.
> 
> This patch fixes the N factor in the clock driver, and adds a post
> PLL divider of 2 to calculate the rate for PLL6.
> 
> A further patch (to the DT) should add a fixed-factor x2 clock as
> the PLL6x2 output.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
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/20140911/3f493df6/attachment.sig>

[PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Chen-Yu Tsai]

This patch unifies the sun6i AHB1 clock, originally supported
with separate mux and divider clks. It also adds support for
the pre-divider on the PLL6 input, thus allowing the clock to
be muxed to PLL6 with proper clock rate calculation.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 Documentation/devicetree/bindings/clock/sunxi.txt |   2 +-
 drivers/clk/sunxi/clk-sunxi.c                     | 209 ++++++++++++++++++++++
 2 files changed, 210 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
index d3a5c3c..3d531d6 100644
--- a/Documentation/devicetree/bindings/clock/sunxi.txt
+++ b/Documentation/devicetree/bindings/clock/sunxi.txt
@@ -23,7 +23,7 @@ Required properties:
 	"allwinner,sun5i-a10s-ahb-gates-clk" - for the AHB gates on A10s
 	"allwinner,sun7i-a20-ahb-gates-clk" - for the AHB gates on A20
 	"allwinner,sun6i-a31-ar100-clk" - for the AR100 on A31
-	"allwinner,sun6i-a31-ahb1-mux-clk" - for the AHB1 multiplexer on A31
+	"allwinner,sun6i-a31-ahb1-clk" - for the AHB1 clock on A31
 	"allwinner,sun6i-a31-ahb1-gates-clk" - for the AHB1 gates on A31
 	"allwinner,sun8i-a23-ahb1-gates-clk" - for the AHB1 gates on A23
 	"allwinner,sun4i-a10-apb0-clk" - for the APB0 clock
diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index be9ac07..87b7740 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -19,6 +19,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/reset-controller.h>
+#include <linux/log2.h>
 
 #include "clk-factors.h"
 
@@ -1339,3 +1340,211 @@ static void __init sun6i_init_clocks(struct device_node *node)
 }
 CLK_OF_DECLARE(sun6i_a31_clk_init, "allwinner,sun6i-a31", sun6i_init_clocks);
 CLK_OF_DECLARE(sun8i_a23_clk_init, "allwinner,sun8i-a23", sun6i_init_clocks);
+
+
+/**
+ * sun6i_a31_ahb1_clk_setup() - Setup function for a31 ahb1 composite clk
+ */
+
+#define SUN6I_AHB1_MAX_PARENTS		4
+#define SUN6I_AHB1_MUX_PARENT_PLL6	3
+#define SUN6I_AHB1_MUX_SHIFT		12
+#define SUN6I_AHB1_MUX_MASK		0x3
+#define SUN6I_AHB1_MUX_GET_PARENT(reg)	((reg >> SUN6I_AHB1_MUX_SHIFT) & \
+					 SUN6I_AHB1_MUX_MASK)
+#define SUN6I_AHB1_DIV_SHIFT		4
+#define SUN6I_AHB1_DIV_MASK		0x3
+#define SUN6I_AHB1_DIV_GET(reg)		((reg >> SUN6I_AHB1_DIV_SHIFT) & \
+					 SUN6I_AHB1_DIV_MASK)
+#define SUN6I_AHB1_DIV_SET(reg, div)	((reg & ~(SUN6I_AHB1_DIV_MASK << \
+						  SUN6I_AHB1_DIV_SHIFT)) | \
+					 (div << SUN6I_AHB1_DIV_SHIFT))
+#define SUN6I_AHB1_PLL6_DIV_SHIFT	6
+#define SUN6I_AHB1_PLL6_DIV_MASK	0x3
+#define SUN6I_AHB1_PLL6_DIV_GET(reg)	((reg >> SUN6I_AHB1_PLL6_DIV_SHIFT) & \
+					 SUN6I_AHB1_PLL6_DIV_MASK)
+#define SUN6I_AHB1_PLL6_DIV_SET(reg, div) ((reg & \
+					    ~(SUN6I_AHB1_PLL6_DIV_MASK << \
+					      SUN6I_AHB1_PLL6_DIV_SHIFT)) | \
+					   (div << SUN6I_AHB1_PLL6_DIV_SHIFT))
+
+struct sun6i_ahb1_clk {
+	struct clk_hw hw;
+	void __iomem *reg;
+};
+
+#define to_sun6i_ahb1_clk(_hw) container_of(_hw, struct sun6i_ahb1_clk, hw)
+
+static unsigned long sun6i_ahb1_clk_recalc_rate(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct sun6i_ahb1_clk *ahb1 = to_sun6i_ahb1_clk(hw);
+	unsigned long rate;
+	u32 reg;
+
+	/* Fetch the register value */
+	reg = readl(ahb1->reg);
+
+	/* apply pre-divider first if parent is pll6 */
+	if (SUN6I_AHB1_MUX_GET_PARENT(reg) == SUN6I_AHB1_MUX_PARENT_PLL6)
+		parent_rate /= SUN6I_AHB1_PLL6_DIV_GET(reg) + 1;
+
+	/* clk divider */
+	rate = parent_rate >> SUN6I_AHB1_DIV_GET(reg);
+
+	return rate;
+}
+
+static long sun6i_ahb1_clk_round(unsigned long rate, u8 *divp, u8 *pre_divp,
+				 u8 parent, unsigned long parent_rate)
+{
+	u8 div, calcp, calcm = 1;
+
+	/* clock can only divide, so we will never be able to achieve
+	 * frequencies higher than the parent frequency */
+	if (parent_rate && rate > parent_rate)
+		rate = parent_rate;
+
+	div = DIV_ROUND_UP(parent_rate, rate);
+
+	/* calculate pre-divider if parent is pll6 */
+	if (parent == SUN6I_AHB1_MUX_PARENT_PLL6) {
+		if (div < 4)
+			calcp = 0;
+		else if (div / 2 < 4)
+			calcp = 1;
+		else if (div / 4 < 4)
+			calcp = 2;
+		else
+			calcp = 3;
+
+		calcm = DIV_ROUND_UP(div, 1 << calcp);
+	} else {
+		calcp = __roundup_pow_of_two(div);
+		calcp = calcp > 3 ? 3 : calcp;
+	}
+
+	if (divp) {
+		*divp = calcp;
+		*pre_divp = calcm - 1;
+	}
+
+	return (parent_rate / calcm) >> calcp;
+}
+
+static long sun6i_ahb1_clk_determine_rate(struct clk_hw *hw, unsigned long rate,
+					  unsigned long *best_parent_rate,
+					  struct clk **best_parent_clk)
+{
+	struct clk *clk = hw->clk, *parent, *best_parent = NULL;
+	int i, num_parents;
+	unsigned long parent_rate, best = 0, child_rate, best_child_rate = 0;
+
+	/* find the parent that can help provide the fastest rate <= rate */
+	num_parents = __clk_get_num_parents(clk);
+	for (i = 0; i < num_parents; i++) {
+		parent = clk_get_parent_by_index(clk, i);
+		if (!parent)
+			continue;
+		if (__clk_get_flags(clk) & CLK_SET_RATE_PARENT)
+			parent_rate = __clk_round_rate(parent, rate);
+		else
+			parent_rate = __clk_get_rate(parent);
+
+		child_rate = sun6i_ahb1_clk_round(rate, NULL, NULL, i,
+						  parent_rate);
+
+		if (child_rate <= rate && child_rate > best_child_rate) {
+			best_parent = parent;
+			best = parent_rate;
+			best_child_rate = child_rate;
+		}
+	}
+
+	if (best_parent)
+		*best_parent_clk = best_parent;
+	*best_parent_rate = best;
+
+	return best_child_rate;
+}
+
+static int sun6i_ahb1_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+				   unsigned long parent_rate)
+{
+	struct sun6i_ahb1_clk *ahb1 = to_sun6i_ahb1_clk(hw);
+	unsigned long flags;
+	u8 div, pre_div, parent;
+	u32 reg;
+
+	spin_lock_irqsave(&clk_lock, flags);
+
+	reg = readl(ahb1->reg);
+
+	/* need to know which parent is used to apply pre-divider */
+	parent = SUN6I_AHB1_MUX_GET_PARENT(reg);
+	sun6i_ahb1_clk_round(rate, &div, &pre_div, parent, parent_rate);
+
+	reg = SUN6I_AHB1_DIV_SET(reg, div);
+	reg = SUN6I_AHB1_PLL6_DIV_SET(reg, pre_div);
+	writel(reg, ahb1->reg);
+
+	spin_unlock_irqrestore(&clk_lock, flags);
+
+	return 0;
+}
+
+static const struct clk_ops sun6i_ahb1_clk_ops = {
+	.determine_rate	= sun6i_ahb1_clk_determine_rate,
+	.recalc_rate	= sun6i_ahb1_clk_recalc_rate,
+	.set_rate	= sun6i_ahb1_clk_set_rate,
+};
+
+static void __init sun6i_ahb1_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	struct sun6i_ahb1_clk *ahb1;
+	struct clk_mux *mux;
+	const char *clk_name = node->name;
+	const char *parents[SUN6I_AHB1_MAX_PARENTS];
+	void __iomem *reg;
+	int i = 0;
+
+	reg = of_iomap(node, 0);
+
+	/* we have a mux, we will have >1 parents */
+	while (i < SUN6I_AHB1_MAX_PARENTS &&
+	       (parents[i] = of_clk_get_parent_name(node, i)) != NULL)
+		i++;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	ahb1 = kzalloc(sizeof(struct sun6i_ahb1_clk), GFP_KERNEL);
+	if (!ahb1)
+		return;
+
+	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
+	if (!mux) {
+		kfree(ahb1);
+		return;
+	}
+
+	/* set up clock properties */
+	mux->reg = reg;
+	mux->shift = SUN6I_AHB1_MUX_SHIFT;
+	mux->mask = SUN6I_AHB1_MUX_MASK;
+	mux->lock = &clk_lock;
+	ahb1->reg = reg;
+
+	clk = clk_register_composite(NULL, clk_name, parents, i,
+				     &mux->hw, &clk_mux_ops,
+				     &ahb1->hw, &sun6i_ahb1_clk_ops,
+				     NULL, NULL, 0);
+
+	if (!IS_ERR(clk)) {
+		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+		clk_register_clkdev(clk, clk_name, NULL);
+	}
+}
+
+CLK_OF_DECLARE(sun6i_a31_ahb1, "allwinner,sun6i-a31-ahb1-clk",
+		sun6i_ahb1_clk_setup);
-- 
2.1.0

[PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Chen-Yu Tsai]

This patch unifies the sun6i AHB1 clock, originally supported
with separate mux and divider clks. It also adds support for
the pre-divider on the PLL6 input, thus allowing the clock to
be muxed to PLL6 with proper clock rate calculation.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 Documentation/devicetree/bindings/clock/sunxi.txt |   2 +-
 drivers/clk/sunxi/clk-sunxi.c                     | 209 ++++++++++++++++++++++
 2 files changed, 210 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
index d3a5c3c..3d531d6 100644
--- a/Documentation/devicetree/bindings/clock/sunxi.txt
+++ b/Documentation/devicetree/bindings/clock/sunxi.txt
@@ -23,7 +23,7 @@ Required properties:
 	"allwinner,sun5i-a10s-ahb-gates-clk" - for the AHB gates on A10s
 	"allwinner,sun7i-a20-ahb-gates-clk" - for the AHB gates on A20
 	"allwinner,sun6i-a31-ar100-clk" - for the AR100 on A31
-	"allwinner,sun6i-a31-ahb1-mux-clk" - for the AHB1 multiplexer on A31
+	"allwinner,sun6i-a31-ahb1-clk" - for the AHB1 clock on A31
 	"allwinner,sun6i-a31-ahb1-gates-clk" - for the AHB1 gates on A31
 	"allwinner,sun8i-a23-ahb1-gates-clk" - for the AHB1 gates on A23
 	"allwinner,sun4i-a10-apb0-clk" - for the APB0 clock
diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index be9ac07..87b7740 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -19,6 +19,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/reset-controller.h>
+#include <linux/log2.h>
 
 #include "clk-factors.h"
 
@@ -1339,3 +1340,211 @@ static void __init sun6i_init_clocks(struct device_node *node)
 }
 CLK_OF_DECLARE(sun6i_a31_clk_init, "allwinner,sun6i-a31", sun6i_init_clocks);
 CLK_OF_DECLARE(sun8i_a23_clk_init, "allwinner,sun8i-a23", sun6i_init_clocks);
+
+
+/**
+ * sun6i_a31_ahb1_clk_setup() - Setup function for a31 ahb1 composite clk
+ */
+
+#define SUN6I_AHB1_MAX_PARENTS		4
+#define SUN6I_AHB1_MUX_PARENT_PLL6	3
+#define SUN6I_AHB1_MUX_SHIFT		12
+#define SUN6I_AHB1_MUX_MASK		0x3
+#define SUN6I_AHB1_MUX_GET_PARENT(reg)	((reg >> SUN6I_AHB1_MUX_SHIFT) & \
+					 SUN6I_AHB1_MUX_MASK)
+#define SUN6I_AHB1_DIV_SHIFT		4
+#define SUN6I_AHB1_DIV_MASK		0x3
+#define SUN6I_AHB1_DIV_GET(reg)		((reg >> SUN6I_AHB1_DIV_SHIFT) & \
+					 SUN6I_AHB1_DIV_MASK)
+#define SUN6I_AHB1_DIV_SET(reg, div)	((reg & ~(SUN6I_AHB1_DIV_MASK << \
+						  SUN6I_AHB1_DIV_SHIFT)) | \
+					 (div << SUN6I_AHB1_DIV_SHIFT))
+#define SUN6I_AHB1_PLL6_DIV_SHIFT	6
+#define SUN6I_AHB1_PLL6_DIV_MASK	0x3
+#define SUN6I_AHB1_PLL6_DIV_GET(reg)	((reg >> SUN6I_AHB1_PLL6_DIV_SHIFT) & \
+					 SUN6I_AHB1_PLL6_DIV_MASK)
+#define SUN6I_AHB1_PLL6_DIV_SET(reg, div) ((reg & \
+					    ~(SUN6I_AHB1_PLL6_DIV_MASK << \
+					      SUN6I_AHB1_PLL6_DIV_SHIFT)) | \
+					   (div << SUN6I_AHB1_PLL6_DIV_SHIFT))
+
+struct sun6i_ahb1_clk {
+	struct clk_hw hw;
+	void __iomem *reg;
+};
+
+#define to_sun6i_ahb1_clk(_hw) container_of(_hw, struct sun6i_ahb1_clk, hw)
+
+static unsigned long sun6i_ahb1_clk_recalc_rate(struct clk_hw *hw,
+						unsigned long parent_rate)
+{
+	struct sun6i_ahb1_clk *ahb1 = to_sun6i_ahb1_clk(hw);
+	unsigned long rate;
+	u32 reg;
+
+	/* Fetch the register value */
+	reg = readl(ahb1->reg);
+
+	/* apply pre-divider first if parent is pll6 */
+	if (SUN6I_AHB1_MUX_GET_PARENT(reg) == SUN6I_AHB1_MUX_PARENT_PLL6)
+		parent_rate /= SUN6I_AHB1_PLL6_DIV_GET(reg) + 1;
+
+	/* clk divider */
+	rate = parent_rate >> SUN6I_AHB1_DIV_GET(reg);
+
+	return rate;
+}
+
+static long sun6i_ahb1_clk_round(unsigned long rate, u8 *divp, u8 *pre_divp,
+				 u8 parent, unsigned long parent_rate)
+{
+	u8 div, calcp, calcm = 1;
+
+	/* clock can only divide, so we will never be able to achieve
+	 * frequencies higher than the parent frequency */
+	if (parent_rate && rate > parent_rate)
+		rate = parent_rate;
+
+	div = DIV_ROUND_UP(parent_rate, rate);
+
+	/* calculate pre-divider if parent is pll6 */
+	if (parent == SUN6I_AHB1_MUX_PARENT_PLL6) {
+		if (div < 4)
+			calcp = 0;
+		else if (div / 2 < 4)
+			calcp = 1;
+		else if (div / 4 < 4)
+			calcp = 2;
+		else
+			calcp = 3;
+
+		calcm = DIV_ROUND_UP(div, 1 << calcp);
+	} else {
+		calcp = __roundup_pow_of_two(div);
+		calcp = calcp > 3 ? 3 : calcp;
+	}
+
+	if (divp) {
+		*divp = calcp;
+		*pre_divp = calcm - 1;
+	}
+
+	return (parent_rate / calcm) >> calcp;
+}
+
+static long sun6i_ahb1_clk_determine_rate(struct clk_hw *hw, unsigned long rate,
+					  unsigned long *best_parent_rate,
+					  struct clk **best_parent_clk)
+{
+	struct clk *clk = hw->clk, *parent, *best_parent = NULL;
+	int i, num_parents;
+	unsigned long parent_rate, best = 0, child_rate, best_child_rate = 0;
+
+	/* find the parent that can help provide the fastest rate <= rate */
+	num_parents = __clk_get_num_parents(clk);
+	for (i = 0; i < num_parents; i++) {
+		parent = clk_get_parent_by_index(clk, i);
+		if (!parent)
+			continue;
+		if (__clk_get_flags(clk) & CLK_SET_RATE_PARENT)
+			parent_rate = __clk_round_rate(parent, rate);
+		else
+			parent_rate = __clk_get_rate(parent);
+
+		child_rate = sun6i_ahb1_clk_round(rate, NULL, NULL, i,
+						  parent_rate);
+
+		if (child_rate <= rate && child_rate > best_child_rate) {
+			best_parent = parent;
+			best = parent_rate;
+			best_child_rate = child_rate;
+		}
+	}
+
+	if (best_parent)
+		*best_parent_clk = best_parent;
+	*best_parent_rate = best;
+
+	return best_child_rate;
+}
+
+static int sun6i_ahb1_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+				   unsigned long parent_rate)
+{
+	struct sun6i_ahb1_clk *ahb1 = to_sun6i_ahb1_clk(hw);
+	unsigned long flags;
+	u8 div, pre_div, parent;
+	u32 reg;
+
+	spin_lock_irqsave(&clk_lock, flags);
+
+	reg = readl(ahb1->reg);
+
+	/* need to know which parent is used to apply pre-divider */
+	parent = SUN6I_AHB1_MUX_GET_PARENT(reg);
+	sun6i_ahb1_clk_round(rate, &div, &pre_div, parent, parent_rate);
+
+	reg = SUN6I_AHB1_DIV_SET(reg, div);
+	reg = SUN6I_AHB1_PLL6_DIV_SET(reg, pre_div);
+	writel(reg, ahb1->reg);
+
+	spin_unlock_irqrestore(&clk_lock, flags);
+
+	return 0;
+}
+
+static const struct clk_ops sun6i_ahb1_clk_ops = {
+	.determine_rate	= sun6i_ahb1_clk_determine_rate,
+	.recalc_rate	= sun6i_ahb1_clk_recalc_rate,
+	.set_rate	= sun6i_ahb1_clk_set_rate,
+};
+
+static void __init sun6i_ahb1_clk_setup(struct device_node *node)
+{
+	struct clk *clk;
+	struct sun6i_ahb1_clk *ahb1;
+	struct clk_mux *mux;
+	const char *clk_name = node->name;
+	const char *parents[SUN6I_AHB1_MAX_PARENTS];
+	void __iomem *reg;
+	int i = 0;
+
+	reg = of_iomap(node, 0);
+
+	/* we have a mux, we will have >1 parents */
+	while (i < SUN6I_AHB1_MAX_PARENTS &&
+	       (parents[i] = of_clk_get_parent_name(node, i)) != NULL)
+		i++;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	ahb1 = kzalloc(sizeof(struct sun6i_ahb1_clk), GFP_KERNEL);
+	if (!ahb1)
+		return;
+
+	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
+	if (!mux) {
+		kfree(ahb1);
+		return;
+	}
+
+	/* set up clock properties */
+	mux->reg = reg;
+	mux->shift = SUN6I_AHB1_MUX_SHIFT;
+	mux->mask = SUN6I_AHB1_MUX_MASK;
+	mux->lock = &clk_lock;
+	ahb1->reg = reg;
+
+	clk = clk_register_composite(NULL, clk_name, parents, i,
+				     &mux->hw, &clk_mux_ops,
+				     &ahb1->hw, &sun6i_ahb1_clk_ops,
+				     NULL, NULL, 0);
+
+	if (!IS_ERR(clk)) {
+		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+		clk_register_clkdev(clk, clk_name, NULL);
+	}
+}
+
+CLK_OF_DECLARE(sun6i_a31_ahb1, "allwinner,sun6i-a31-ahb1-clk",
+		sun6i_ahb1_clk_setup);
-- 
2.1.0

Re: [PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

Hi,

On Sat, Sep 06, 2014 at 06:47:24PM +0800, Chen-Yu Tsai wrote:
> This patch unifies the sun6i AHB1 clock, originally supported
> with separate mux and divider clks. It also adds support for
> the pre-divider on the PLL6 input, thus allowing the clock to
> be muxed to PLL6 with proper clock rate calculation.
> 
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

It looks fine, but I'd rather see this in a separate file, especially
since we don't seem to have any order dependency.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Maxime Ripard]

Hi,

On Sat, Sep 06, 2014 at 06:47:24PM +0800, Chen-Yu Tsai wrote:
> This patch unifies the sun6i AHB1 clock, originally supported
> with separate mux and divider clks. It also adds support for
> the pre-divider on the PLL6 input, thus allowing the clock to
> be muxed to PLL6 with proper clock rate calculation.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

It looks fine, but I'd rather see this in a separate file, especially
since we don't seem to have any order dependency.

Thanks!
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/20140911/03a0bc3f/attachment.sig>

Re: [PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Chen-Yu Tsai]

Hi,

On Fri, Sep 12, 2014 at 5:02 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> On Sat, Sep 06, 2014 at 06:47:24PM +0800, Chen-Yu Tsai wrote:
>> This patch unifies the sun6i AHB1 clock, originally supported
>> with separate mux and divider clks. It also adds support for
>> the pre-divider on the PLL6 input, thus allowing the clock to
>> be muxed to PLL6 with proper clock rate calculation.
>>
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>
> It looks fine, but I'd rather see this in a separate file, especially
> since we don't seem to have any order dependency.

Sorry, just to be clear, separate file under clk/sunxi?

This cannot be in a separate file, as it shares a spinlock with apb1
divider. They share the same register.

We could move apb1 out though. But i would prefer to do that when
we split out all the clocks into individual OF_CLK_DECLAREs.

ChenYu

[PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Chen-Yu Tsai]

Hi,

On Fri, Sep 12, 2014 at 5:02 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Sat, Sep 06, 2014 at 06:47:24PM +0800, Chen-Yu Tsai wrote:
>> This patch unifies the sun6i AHB1 clock, originally supported
>> with separate mux and divider clks. It also adds support for
>> the pre-divider on the PLL6 input, thus allowing the clock to
>> be muxed to PLL6 with proper clock rate calculation.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>
> It looks fine, but I'd rather see this in a separate file, especially
> since we don't seem to have any order dependency.

Sorry, just to be clear, separate file under clk/sunxi?

This cannot be in a separate file, as it shares a spinlock with apb1
divider. They share the same register.

We could move apb1 out though. But i would prefer to do that when
we split out all the clocks into individual OF_CLK_DECLAREs.

ChenYu

Re: [PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1512 bytes --]

On Fri, Sep 12, 2014 at 11:16:26AM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Fri, Sep 12, 2014 at 5:02 AM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > Hi,
> >
> > On Sat, Sep 06, 2014 at 06:47:24PM +0800, Chen-Yu Tsai wrote:
> >> This patch unifies the sun6i AHB1 clock, originally supported
> >> with separate mux and divider clks. It also adds support for
> >> the pre-divider on the PLL6 input, thus allowing the clock to
> >> be muxed to PLL6 with proper clock rate calculation.
> >>
> >> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> >
> > It looks fine, but I'd rather see this in a separate file, especially
> > since we don't seem to have any order dependency.
> 
> Sorry, just to be clear, separate file under clk/sunxi?

Yes

> This cannot be in a separate file, as it shares a spinlock with apb1
> divider. They share the same register.
> 
> We could move apb1 out though. But i would prefer to do that when
> we split out all the clocks into individual OF_CLK_DECLAREs.

Ah right, my bad :)

My plan on the long term is to kill clk-sunxi as a place where all the
clocks are defined, and only leave the "policy" there, for example the
clock protection code (even if that should probably be removed too,
together with clkdev), the various rates / parenting enforcements,
etc.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Maxime Ripard]

On Fri, Sep 12, 2014 at 11:16:26AM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Fri, Sep 12, 2014 at 5:02 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi,
> >
> > On Sat, Sep 06, 2014 at 06:47:24PM +0800, Chen-Yu Tsai wrote:
> >> This patch unifies the sun6i AHB1 clock, originally supported
> >> with separate mux and divider clks. It also adds support for
> >> the pre-divider on the PLL6 input, thus allowing the clock to
> >> be muxed to PLL6 with proper clock rate calculation.
> >>
> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >
> > It looks fine, but I'd rather see this in a separate file, especially
> > since we don't seem to have any order dependency.
> 
> Sorry, just to be clear, separate file under clk/sunxi?

Yes

> This cannot be in a separate file, as it shares a spinlock with apb1
> divider. They share the same register.
> 
> We could move apb1 out though. But i would prefer to do that when
> we split out all the clocks into individual OF_CLK_DECLAREs.

Ah right, my bad :)

My plan on the long term is to kill clk-sunxi as a place where all the
clocks are defined, and only leave the "policy" there, for example the
clock protection code (even if that should probably be removed too,
together with clkdev), the various rates / parenting enforcements,
etc.

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/20140913/364cc588/attachment.sig>

Re: [PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Mike Turquette]

Quoting Maxime Ripard (2014-09-13 03:26:03)
> On Fri, Sep 12, 2014 at 11:16:26AM +0800, Chen-Yu Tsai wrote:
> > Hi,
> > 
> > On Fri, Sep 12, 2014 at 5:02 AM, Maxime Ripard
> > <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > > Hi,
> > >
> > > On Sat, Sep 06, 2014 at 06:47:24PM +0800, Chen-Yu Tsai wrote:
> > >> This patch unifies the sun6i AHB1 clock, originally supported
> > >> with separate mux and divider clks. It also adds support for
> > >> the pre-divider on the PLL6 input, thus allowing the clock to
> > >> be muxed to PLL6 with proper clock rate calculation.
> > >>
> > >> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> > >
> > > It looks fine, but I'd rather see this in a separate file, especially
> > > since we don't seem to have any order dependency.
> > 
> > Sorry, just to be clear, separate file under clk/sunxi?
> 
> Yes
> 
> > This cannot be in a separate file, as it shares a spinlock with apb1
> > divider. They share the same register.
> > 
> > We could move apb1 out though. But i would prefer to do that when
> > we split out all the clocks into individual OF_CLK_DECLAREs.
> 
> Ah right, my bad :)
> 
> My plan on the long term is to kill clk-sunxi as a place where all the
> clocks are defined, and only leave the "policy" there, for example the
> clock protection code (even if that should probably be removed too,
> together with clkdev), the various rates / parenting enforcements,
> etc.

Interesting! Where are you planning to store the clock data?

Regards,
Mike

> 
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Mike Turquette]

Quoting Maxime Ripard (2014-09-13 03:26:03)
> On Fri, Sep 12, 2014 at 11:16:26AM +0800, Chen-Yu Tsai wrote:
> > Hi,
> > 
> > On Fri, Sep 12, 2014 at 5:02 AM, Maxime Ripard
> > <maxime.ripard@free-electrons.com> wrote:
> > > Hi,
> > >
> > > On Sat, Sep 06, 2014 at 06:47:24PM +0800, Chen-Yu Tsai wrote:
> > >> This patch unifies the sun6i AHB1 clock, originally supported
> > >> with separate mux and divider clks. It also adds support for
> > >> the pre-divider on the PLL6 input, thus allowing the clock to
> > >> be muxed to PLL6 with proper clock rate calculation.
> > >>
> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > >
> > > It looks fine, but I'd rather see this in a separate file, especially
> > > since we don't seem to have any order dependency.
> > 
> > Sorry, just to be clear, separate file under clk/sunxi?
> 
> Yes
> 
> > This cannot be in a separate file, as it shares a spinlock with apb1
> > divider. They share the same register.
> > 
> > We could move apb1 out though. But i would prefer to do that when
> > we split out all the clocks into individual OF_CLK_DECLAREs.
> 
> Ah right, my bad :)
> 
> My plan on the long term is to kill clk-sunxi as a place where all the
> clocks are defined, and only leave the "policy" there, for example the
> clock protection code (even if that should probably be removed too,
> together with clkdev), the various rates / parenting enforcements,
> etc.

Interesting! Where are you planning to store the clock data?

Regards,
Mike

> 
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Re: [PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]

On Thu, Sep 25, 2014 at 04:03:40PM -0700, Mike Turquette wrote:
> Quoting Maxime Ripard (2014-09-13 03:26:03)
> > On Fri, Sep 12, 2014 at 11:16:26AM +0800, Chen-Yu Tsai wrote:
> > > Hi,
> > > 
> > > On Fri, Sep 12, 2014 at 5:02 AM, Maxime Ripard
> > > <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > > > Hi,
> > > >
> > > > On Sat, Sep 06, 2014 at 06:47:24PM +0800, Chen-Yu Tsai wrote:
> > > >> This patch unifies the sun6i AHB1 clock, originally supported
> > > >> with separate mux and divider clks. It also adds support for
> > > >> the pre-divider on the PLL6 input, thus allowing the clock to
> > > >> be muxed to PLL6 with proper clock rate calculation.
> > > >>
> > > >> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> > > >
> > > > It looks fine, but I'd rather see this in a separate file, especially
> > > > since we don't seem to have any order dependency.
> > > 
> > > Sorry, just to be clear, separate file under clk/sunxi?
> > 
> > Yes
> > 
> > > This cannot be in a separate file, as it shares a spinlock with apb1
> > > divider. They share the same register.
> > > 
> > > We could move apb1 out though. But i would prefer to do that when
> > > we split out all the clocks into individual OF_CLK_DECLAREs.
> > 
> > Ah right, my bad :)
> > 
> > My plan on the long term is to kill clk-sunxi as a place where all the
> > clocks are defined, and only leave the "policy" there, for example the
> > clock protection code (even if that should probably be removed too,
> > together with clkdev), the various rates / parenting enforcements,
> > etc.
> 
> Interesting! Where are you planning to store the clock data?

Which data? 

I guess, for the rate boundaries, the DT would be the right place,
wether a clock should be protected can be derived from its
compatible. And for the rate to enforce, maybe a clock-frequency
property in the DT too, or directly in the driver, I haven't really
thought about that part at the moment to be honest :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/7] clk: sunxi: unify sun6i AHB1 clock with proper PLL6 pre-divider

[Maxime Ripard]

On Thu, Sep 25, 2014 at 04:03:40PM -0700, Mike Turquette wrote:
> Quoting Maxime Ripard (2014-09-13 03:26:03)
> > On Fri, Sep 12, 2014 at 11:16:26AM +0800, Chen-Yu Tsai wrote:
> > > Hi,
> > > 
> > > On Fri, Sep 12, 2014 at 5:02 AM, Maxime Ripard
> > > <maxime.ripard@free-electrons.com> wrote:
> > > > Hi,
> > > >
> > > > On Sat, Sep 06, 2014 at 06:47:24PM +0800, Chen-Yu Tsai wrote:
> > > >> This patch unifies the sun6i AHB1 clock, originally supported
> > > >> with separate mux and divider clks. It also adds support for
> > > >> the pre-divider on the PLL6 input, thus allowing the clock to
> > > >> be muxed to PLL6 with proper clock rate calculation.
> > > >>
> > > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> > > >
> > > > It looks fine, but I'd rather see this in a separate file, especially
> > > > since we don't seem to have any order dependency.
> > > 
> > > Sorry, just to be clear, separate file under clk/sunxi?
> > 
> > Yes
> > 
> > > This cannot be in a separate file, as it shares a spinlock with apb1
> > > divider. They share the same register.
> > > 
> > > We could move apb1 out though. But i would prefer to do that when
> > > we split out all the clocks into individual OF_CLK_DECLAREs.
> > 
> > Ah right, my bad :)
> > 
> > My plan on the long term is to kill clk-sunxi as a place where all the
> > clocks are defined, and only leave the "policy" there, for example the
> > clock protection code (even if that should probably be removed too,
> > together with clkdev), the various rates / parenting enforcements,
> > etc.
> 
> Interesting! Where are you planning to store the clock data?

Which data? 

I guess, for the rate boundaries, the DT would be the right place,
wether a clock should be protected can be derived from its
compatible. And for the rate to enforce, maybe a clock-frequency
property in the DT too, or directly in the driver, I haven't really
thought about that part at the moment to be honest :)

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/20140926/11e68a61/attachment.sig>

[PATCH 4/7] ARM: dts: sun8i: Unify ahb1 clock nodes

[Chen-Yu Tsai]

The clock driver has unified support for the ahb1 clock.
Unify the clock nodes so it works.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun8i-a23.dtsi | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/sun8i-a23.dtsi b/arch/arm/boot/dts/sun8i-a23.dtsi
index 2ec86d36..cc8c650 100644
--- a/arch/arm/boot/dts/sun8i-a23.dtsi
+++ b/arch/arm/boot/dts/sun8i-a23.dtsi
@@ -104,19 +104,11 @@
 			clock-output-names = "axi";
 		};
 
-		ahb1_mux: ahb1_mux_clk@01c20054 {
-			#clock-cells = <0>;
-			compatible = "allwinner,sun6i-a31-ahb1-mux-clk";
-			reg = <0x01c20054 0x4>;
-			clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
-			clock-output-names = "ahb1_mux";
-		};
-
 		ahb1: ahb1_clk@01c20054 {
 			#clock-cells = <0>;
-			compatible = "allwinner,sun4i-a10-ahb-clk";
+			compatible = "allwinner,sun6i-a31-ahb1-clk";
 			reg = <0x01c20054 0x4>;
-			clocks = <&ahb1_mux>;
+			clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
 			clock-output-names = "ahb1";
 		};
 
-- 
2.1.0

[PATCH 4/7] ARM: dts: sun8i: Unify ahb1 clock nodes

[Chen-Yu Tsai]

The clock driver has unified support for the ahb1 clock.
Unify the clock nodes so it works.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun8i-a23.dtsi | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/sun8i-a23.dtsi b/arch/arm/boot/dts/sun8i-a23.dtsi
index 2ec86d36..cc8c650 100644
--- a/arch/arm/boot/dts/sun8i-a23.dtsi
+++ b/arch/arm/boot/dts/sun8i-a23.dtsi
@@ -104,19 +104,11 @@
 			clock-output-names = "axi";
 		};
 
-		ahb1_mux: ahb1_mux_clk at 01c20054 {
-			#clock-cells = <0>;
-			compatible = "allwinner,sun6i-a31-ahb1-mux-clk";
-			reg = <0x01c20054 0x4>;
-			clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
-			clock-output-names = "ahb1_mux";
-		};
-
 		ahb1: ahb1_clk at 01c20054 {
 			#clock-cells = <0>;
-			compatible = "allwinner,sun4i-a10-ahb-clk";
+			compatible = "allwinner,sun6i-a31-ahb1-clk";
 			reg = <0x01c20054 0x4>;
-			clocks = <&ahb1_mux>;
+			clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
 			clock-output-names = "ahb1";
 		};
 
-- 
2.1.0

[PATCH 5/7] ARM: dts: sun6i: Unify ahb1 clock nodes

[Chen-Yu Tsai]

The clock driver has unified support for the ahb1 clock.
Unify the clock nodes so it works.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index c1295bf..8eb2c6d 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -126,19 +126,11 @@
 			clock-output-names = "axi";
 		};
 
-		ahb1_mux: ahb1_mux@01c20054 {
-			#clock-cells = <0>;
-			compatible = "allwinner,sun6i-a31-ahb1-mux-clk";
-			reg = <0x01c20054 0x4>;
-			clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
-			clock-output-names = "ahb1_mux";
-		};
-
 		ahb1: ahb1@01c20054 {
 			#clock-cells = <0>;
-			compatible = "allwinner,sun4i-a10-ahb-clk";
+			compatible = "allwinner,sun6i-a31-ahb1-clk";
 			reg = <0x01c20054 0x4>;
-			clocks = <&ahb1_mux>;
+			clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
 			clock-output-names = "ahb1";
 		};
 
-- 
2.1.0

[PATCH 5/7] ARM: dts: sun6i: Unify ahb1 clock nodes

[Chen-Yu Tsai]

The clock driver has unified support for the ahb1 clock.
Unify the clock nodes so it works.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index c1295bf..8eb2c6d 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -126,19 +126,11 @@
 			clock-output-names = "axi";
 		};
 
-		ahb1_mux: ahb1_mux at 01c20054 {
-			#clock-cells = <0>;
-			compatible = "allwinner,sun6i-a31-ahb1-mux-clk";
-			reg = <0x01c20054 0x4>;
-			clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
-			clock-output-names = "ahb1_mux";
-		};
-
 		ahb1: ahb1 at 01c20054 {
 			#clock-cells = <0>;
-			compatible = "allwinner,sun4i-a10-ahb-clk";
+			compatible = "allwinner,sun6i-a31-ahb1-clk";
 			reg = <0x01c20054 0x4>;
-			clocks = <&ahb1_mux>;
+			clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
 			clock-output-names = "ahb1";
 		};
 
-- 
2.1.0

[PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Chen-Yu Tsai]

The DMA controller requires AHB1 bus clock to be clocked from PLL6.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 8eb2c6d..1117989 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -317,6 +317,11 @@
 			clocks = <&ahb1_gates 6>;
 			resets = <&ahb1_rst 6>;
 			#dma-cells = <1>;
+
+			/* DMA controller requires AHB1 clocked from PLL6 */
+			assigned-clocks = <&ahb1>;
+			assigned-clock-parents = <&pll6>;
+			assigned-clock-rates = <200000000>;
 		};
 
 		mmc0: mmc@01c0f000 {
-- 
2.1.0

[PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Chen-Yu Tsai]

The DMA controller requires AHB1 bus clock to be clocked from PLL6.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 8eb2c6d..1117989 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -317,6 +317,11 @@
 			clocks = <&ahb1_gates 6>;
 			resets = <&ahb1_rst 6>;
 			#dma-cells = <1>;
+
+			/* DMA controller requires AHB1 clocked from PLL6 */
+			assigned-clocks = <&ahb1>;
+			assigned-clock-parents = <&pll6>;
+			assigned-clock-rates = <200000000>;
 		};
 
 		mmc0: mmc at 01c0f000 {
-- 
2.1.0

Re: [PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
> 
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> ---
>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> index 8eb2c6d..1117989 100644
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -317,6 +317,11 @@
>  			clocks = <&ahb1_gates 6>;
>  			resets = <&ahb1_rst 6>;
>  			#dma-cells = <1>;
> +
> +			/* DMA controller requires AHB1 clocked from PLL6 */
> +			assigned-clocks = <&ahb1>;
> +			assigned-clock-parents = <&pll6>;
> +			assigned-clock-rates = <200000000>;

Where did you get that from?

The user manual says that it should be clocked at 600MHz, and I'm not
sure it should be enforced there either.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Maxime Ripard]

On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> index 8eb2c6d..1117989 100644
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -317,6 +317,11 @@
>  			clocks = <&ahb1_gates 6>;
>  			resets = <&ahb1_rst 6>;
>  			#dma-cells = <1>;
> +
> +			/* DMA controller requires AHB1 clocked from PLL6 */
> +			assigned-clocks = <&ahb1>;
> +			assigned-clock-parents = <&pll6>;
> +			assigned-clock-rates = <200000000>;

Where did you get that from?

The user manual says that it should be clocked at 600MHz, and I'm not
sure it should be enforced there either.

Thanks!
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/20140911/fb780bb6/attachment.sig>

Re: [PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Chen-Yu Tsai]

On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
>> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
>>
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
>> index 8eb2c6d..1117989 100644
>> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
>> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
>> @@ -317,6 +317,11 @@
>>                       clocks = <&ahb1_gates 6>;
>>                       resets = <&ahb1_rst 6>;
>>                       #dma-cells = <1>;
>> +
>> +                     /* DMA controller requires AHB1 clocked from PLL6 */
>> +                     assigned-clocks = <&ahb1>;
>> +                     assigned-clock-parents = <&pll6>;
>> +                     assigned-clock-rates = <200000000>;
>
> Where did you get that from?
>
> The user manual says that it should be clocked at 600MHz, and I'm not
> sure it should be enforced there either.

The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
not "pll6 should be 200 MHz". I assume you were misled by them.

Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
vendor BSP default:

On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
pll6, which sets pre-divider to /3 and divider to /1.

Hope this clears it up. :)


Cheers
ChenYu

[PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Chen-Yu Tsai]

On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
>> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
>> index 8eb2c6d..1117989 100644
>> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
>> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
>> @@ -317,6 +317,11 @@
>>                       clocks = <&ahb1_gates 6>;
>>                       resets = <&ahb1_rst 6>;
>>                       #dma-cells = <1>;
>> +
>> +                     /* DMA controller requires AHB1 clocked from PLL6 */
>> +                     assigned-clocks = <&ahb1>;
>> +                     assigned-clock-parents = <&pll6>;
>> +                     assigned-clock-rates = <200000000>;
>
> Where did you get that from?
>
> The user manual says that it should be clocked at 600MHz, and I'm not
> sure it should be enforced there either.

The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
not "pll6 should be 200 MHz". I assume you were misled by them.

Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
vendor BSP default:

On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
pll6, which sets pre-divider to /3 and divider to /1.

Hope this clears it up. :)


Cheers
ChenYu

Re: [PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]

On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
> On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
> >> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
> >>
> >> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> >> ---
> >>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> index 8eb2c6d..1117989 100644
> >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> @@ -317,6 +317,11 @@
> >>                       clocks = <&ahb1_gates 6>;
> >>                       resets = <&ahb1_rst 6>;
> >>                       #dma-cells = <1>;
> >> +
> >> +                     /* DMA controller requires AHB1 clocked from PLL6 */
> >> +                     assigned-clocks = <&ahb1>;
> >> +                     assigned-clock-parents = <&pll6>;
> >> +                     assigned-clock-rates = <200000000>;
> >
> > Where did you get that from?
> >
> > The user manual says that it should be clocked at 600MHz, and I'm not
> > sure it should be enforced there either.
> 
> The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
> not "pll6 should be 200 MHz". I assume you were misled by them.
> 
> Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
> vendor BSP default:
> 
> On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
> ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
> pll6, which sets pre-divider to /3 and divider to /1.
> 
> Hope this clears it up. :)

It does, thanks :)

But still, I find it the wrong place to enforce such a limit. This
should go into the clock driver itself. The DMA controller requires
such a parenting, but it doesn't require any specific rate, this is
more something of the global system policy.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Maxime Ripard]

On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
> On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
> >> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
> >>
> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >> ---
> >>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> index 8eb2c6d..1117989 100644
> >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> @@ -317,6 +317,11 @@
> >>                       clocks = <&ahb1_gates 6>;
> >>                       resets = <&ahb1_rst 6>;
> >>                       #dma-cells = <1>;
> >> +
> >> +                     /* DMA controller requires AHB1 clocked from PLL6 */
> >> +                     assigned-clocks = <&ahb1>;
> >> +                     assigned-clock-parents = <&pll6>;
> >> +                     assigned-clock-rates = <200000000>;
> >
> > Where did you get that from?
> >
> > The user manual says that it should be clocked at 600MHz, and I'm not
> > sure it should be enforced there either.
> 
> The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
> not "pll6 should be 200 MHz". I assume you were misled by them.
> 
> Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
> vendor BSP default:
> 
> On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
> ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
> pll6, which sets pre-divider to /3 and divider to /1.
> 
> Hope this clears it up. :)

It does, thanks :)

But still, I find it the wrong place to enforce such a limit. This
should go into the clock driver itself. The DMA controller requires
such a parenting, but it doesn't require any specific rate, this is
more something of the global system policy.

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/25a15db7/attachment-0001.sig>

Re: [PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Chen-Yu Tsai]

On Tue, Sep 16, 2014 at 11:48 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
>> On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
>> >> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
>> >>
>> >> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> >> ---
>> >>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> index 8eb2c6d..1117989 100644
>> >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> @@ -317,6 +317,11 @@
>> >>                       clocks = <&ahb1_gates 6>;
>> >>                       resets = <&ahb1_rst 6>;
>> >>                       #dma-cells = <1>;
>> >> +
>> >> +                     /* DMA controller requires AHB1 clocked from PLL6 */
>> >> +                     assigned-clocks = <&ahb1>;
>> >> +                     assigned-clock-parents = <&pll6>;
>> >> +                     assigned-clock-rates = <200000000>;
>> >
>> > Where did you get that from?
>> >
>> > The user manual says that it should be clocked at 600MHz, and I'm not
>> > sure it should be enforced there either.
>>
>> The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
>> not "pll6 should be 200 MHz". I assume you were misled by them.
>>
>> Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
>> vendor BSP default:
>>
>> On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
>> ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
>> pll6, which sets pre-divider to /3 and divider to /1.
>>
>> Hope this clears it up. :)
>
> It does, thanks :)
>
> But still, I find it the wrong place to enforce such a limit. This
> should go into the clock driver itself. The DMA controller requires
> such a parenting, but it doesn't require any specific rate, this is
> more something of the global system policy.

But I am guessing the AHB bus clock cannot be clock at any arbitrary
rate. Just muxing without setting a specific clock rate might result
in a hang. We could address this with per clock limitations, like the
patch you posted earlier. Not sure if there's anything in the kernel
like that at the moment, or how the clock framework handles clock rates
after re-parenting.

Or we could just specify in the clock driver, or in the DT via clock
supplier defaults, that the AHB bus should be clocked from PLL6 @ 200 MHz.

The latter seems easier to do.

Also, I tested the DMA controller using dmatest on my A31 Hummingbird
without this re-parenting patch. It worked fine. How did your tests fail?


Cheers
ChenYu

[PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Chen-Yu Tsai]

On Tue, Sep 16, 2014 at 11:48 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
>> On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
>> >> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
>> >>
>> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> >> ---
>> >>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> index 8eb2c6d..1117989 100644
>> >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> @@ -317,6 +317,11 @@
>> >>                       clocks = <&ahb1_gates 6>;
>> >>                       resets = <&ahb1_rst 6>;
>> >>                       #dma-cells = <1>;
>> >> +
>> >> +                     /* DMA controller requires AHB1 clocked from PLL6 */
>> >> +                     assigned-clocks = <&ahb1>;
>> >> +                     assigned-clock-parents = <&pll6>;
>> >> +                     assigned-clock-rates = <200000000>;
>> >
>> > Where did you get that from?
>> >
>> > The user manual says that it should be clocked at 600MHz, and I'm not
>> > sure it should be enforced there either.
>>
>> The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
>> not "pll6 should be 200 MHz". I assume you were misled by them.
>>
>> Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
>> vendor BSP default:
>>
>> On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
>> ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
>> pll6, which sets pre-divider to /3 and divider to /1.
>>
>> Hope this clears it up. :)
>
> It does, thanks :)
>
> But still, I find it the wrong place to enforce such a limit. This
> should go into the clock driver itself. The DMA controller requires
> such a parenting, but it doesn't require any specific rate, this is
> more something of the global system policy.

But I am guessing the AHB bus clock cannot be clock at any arbitrary
rate. Just muxing without setting a specific clock rate might result
in a hang. We could address this with per clock limitations, like the
patch you posted earlier. Not sure if there's anything in the kernel
like that at the moment, or how the clock framework handles clock rates
after re-parenting.

Or we could just specify in the clock driver, or in the DT via clock
supplier defaults, that the AHB bus should be clocked from PLL6 @ 200 MHz.

The latter seems easier to do.

Also, I tested the DMA controller using dmatest on my A31 Hummingbird
without this re-parenting patch. It worked fine. How did your tests fail?


Cheers
ChenYu

Re: [PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3806 bytes --]

On Wed, Sep 17, 2014 at 12:01:46AM +0800, Chen-Yu Tsai wrote:
> On Tue, Sep 16, 2014 at 11:48 PM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
> >> On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
> >> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >> > On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
> >> >> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
> >> >>
> >> >> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> >> >> ---
> >> >>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
> >> >>  1 file changed, 5 insertions(+)
> >> >>
> >> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> index 8eb2c6d..1117989 100644
> >> >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> @@ -317,6 +317,11 @@
> >> >>                       clocks = <&ahb1_gates 6>;
> >> >>                       resets = <&ahb1_rst 6>;
> >> >>                       #dma-cells = <1>;
> >> >> +
> >> >> +                     /* DMA controller requires AHB1 clocked from PLL6 */
> >> >> +                     assigned-clocks = <&ahb1>;
> >> >> +                     assigned-clock-parents = <&pll6>;
> >> >> +                     assigned-clock-rates = <200000000>;
> >> >
> >> > Where did you get that from?
> >> >
> >> > The user manual says that it should be clocked at 600MHz, and I'm not
> >> > sure it should be enforced there either.
> >>
> >> The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
> >> not "pll6 should be 200 MHz". I assume you were misled by them.
> >>
> >> Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
> >> vendor BSP default:
> >>
> >> On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
> >> ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
> >> pll6, which sets pre-divider to /3 and divider to /1.
> >>
> >> Hope this clears it up. :)
> >
> > It does, thanks :)
> >
> > But still, I find it the wrong place to enforce such a limit. This
> > should go into the clock driver itself. The DMA controller requires
> > such a parenting, but it doesn't require any specific rate, this is
> > more something of the global system policy.
> 
> But I am guessing the AHB bus clock cannot be clock at any arbitrary
> rate. Just muxing without setting a specific clock rate might result
> in a hang. We could address this with per clock limitations, like the
> patch you posted earlier. Not sure if there's anything in the kernel
> like that at the moment, or how the clock framework handles clock rates
> after re-parenting.

There's the Tomeu patch series that allows to set clock rate
requirements from the user side, I guess that could be easily extended
to integrate some clock side boundaries. That's why I stopped working
on the sun5i MMC clock issue until that patch is merged.

> Or we could just specify in the clock driver, or in the DT via clock
> supplier defaults, that the AHB bus should be clocked from PLL6 @ 200 MHz.
> 
> The latter seems easier to do.

IIRC, there was some opposition to having the muxing assignment in the
clock driver.

> Also, I tested the DMA controller using dmatest on my A31 Hummingbird
> without this re-parenting patch. It worked fine. How did your tests fail?

It was just timeouting, and actually, it was only working to the SRAM,
and not the DRAM.

Maybe the bootloader sets up the clock muxing already?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Maxime Ripard]

On Wed, Sep 17, 2014 at 12:01:46AM +0800, Chen-Yu Tsai wrote:
> On Tue, Sep 16, 2014 at 11:48 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
> >> On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
> >> >> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
> >> >>
> >> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >> >> ---
> >> >>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
> >> >>  1 file changed, 5 insertions(+)
> >> >>
> >> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> index 8eb2c6d..1117989 100644
> >> >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> @@ -317,6 +317,11 @@
> >> >>                       clocks = <&ahb1_gates 6>;
> >> >>                       resets = <&ahb1_rst 6>;
> >> >>                       #dma-cells = <1>;
> >> >> +
> >> >> +                     /* DMA controller requires AHB1 clocked from PLL6 */
> >> >> +                     assigned-clocks = <&ahb1>;
> >> >> +                     assigned-clock-parents = <&pll6>;
> >> >> +                     assigned-clock-rates = <200000000>;
> >> >
> >> > Where did you get that from?
> >> >
> >> > The user manual says that it should be clocked at 600MHz, and I'm not
> >> > sure it should be enforced there either.
> >>
> >> The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
> >> not "pll6 should be 200 MHz". I assume you were misled by them.
> >>
> >> Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
> >> vendor BSP default:
> >>
> >> On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
> >> ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
> >> pll6, which sets pre-divider to /3 and divider to /1.
> >>
> >> Hope this clears it up. :)
> >
> > It does, thanks :)
> >
> > But still, I find it the wrong place to enforce such a limit. This
> > should go into the clock driver itself. The DMA controller requires
> > such a parenting, but it doesn't require any specific rate, this is
> > more something of the global system policy.
> 
> But I am guessing the AHB bus clock cannot be clock at any arbitrary
> rate. Just muxing without setting a specific clock rate might result
> in a hang. We could address this with per clock limitations, like the
> patch you posted earlier. Not sure if there's anything in the kernel
> like that at the moment, or how the clock framework handles clock rates
> after re-parenting.

There's the Tomeu patch series that allows to set clock rate
requirements from the user side, I guess that could be easily extended
to integrate some clock side boundaries. That's why I stopped working
on the sun5i MMC clock issue until that patch is merged.

> Or we could just specify in the clock driver, or in the DT via clock
> supplier defaults, that the AHB bus should be clocked from PLL6 @ 200 MHz.
> 
> The latter seems easier to do.

IIRC, there was some opposition to having the muxing assignment in the
clock driver.

> Also, I tested the DMA controller using dmatest on my A31 Hummingbird
> without this re-parenting patch. It worked fine. How did your tests fail?

It was just timeouting, and actually, it was only working to the SRAM,
and not the DRAM.

Maybe the bootloader sets up the clock muxing already?

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/20140920/000ec49f/attachment.sig>

Re: [PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Chen-Yu Tsai]

On Sat, Sep 20, 2014 at 5:59 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Wed, Sep 17, 2014 at 12:01:46AM +0800, Chen-Yu Tsai wrote:
>> On Tue, Sep 16, 2014 at 11:48 PM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
>> >> On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
>> >> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> >> > On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
>> >> >> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
>> >> >>
>> >> >> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> >> >> ---
>> >> >>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
>> >> >>  1 file changed, 5 insertions(+)
>> >> >>
>> >> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> >> index 8eb2c6d..1117989 100644
>> >> >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> >> @@ -317,6 +317,11 @@
>> >> >>                       clocks = <&ahb1_gates 6>;
>> >> >>                       resets = <&ahb1_rst 6>;
>> >> >>                       #dma-cells = <1>;
>> >> >> +
>> >> >> +                     /* DMA controller requires AHB1 clocked from PLL6 */
>> >> >> +                     assigned-clocks = <&ahb1>;
>> >> >> +                     assigned-clock-parents = <&pll6>;
>> >> >> +                     assigned-clock-rates = <200000000>;
>> >> >
>> >> > Where did you get that from?
>> >> >
>> >> > The user manual says that it should be clocked at 600MHz, and I'm not
>> >> > sure it should be enforced there either.
>> >>
>> >> The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
>> >> not "pll6 should be 200 MHz". I assume you were misled by them.
>> >>
>> >> Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
>> >> vendor BSP default:
>> >>
>> >> On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
>> >> ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
>> >> pll6, which sets pre-divider to /3 and divider to /1.
>> >>
>> >> Hope this clears it up. :)
>> >
>> > It does, thanks :)
>> >
>> > But still, I find it the wrong place to enforce such a limit. This
>> > should go into the clock driver itself. The DMA controller requires
>> > such a parenting, but it doesn't require any specific rate, this is
>> > more something of the global system policy.
>>
>> But I am guessing the AHB bus clock cannot be clock at any arbitrary
>> rate. Just muxing without setting a specific clock rate might result
>> in a hang. We could address this with per clock limitations, like the
>> patch you posted earlier. Not sure if there's anything in the kernel
>> like that at the moment, or how the clock framework handles clock rates
>> after re-parenting.
>
> There's the Tomeu patch series that allows to set clock rate
> requirements from the user side, I guess that could be easily extended
> to integrate some clock side boundaries. That's why I stopped working
> on the sun5i MMC clock issue until that patch is merged.

I could take a look, but AFAIK none of our drivers need to propagate
clock rate changes.

>> Or we could just specify in the clock driver, or in the DT via clock
>> supplier defaults, that the AHB bus should be clocked from PLL6 @ 200 MHz.
>>
>> The latter seems easier to do.
>
> IIRC, there was some opposition to having the muxing assignment in the
> clock driver.
>
>> Also, I tested the DMA controller using dmatest on my A31 Hummingbird
>> without this re-parenting patch. It worked fine. How did your tests fail?
>
> It was just timeouting, and actually, it was only working to the SRAM,
> and not the DRAM.
>
> Maybe the bootloader sets up the clock muxing already?

I just reran my tests on the A31, and dmatest indeed passes.
I'm using your A31 sdcard.img, replaced u-boot with one built from
u-boot-sunxi, so it supports DT.

I also checked both the clock framework and the raw registers.
AHB1 is indeed clocked from AXI on my system.

Full log: https://gist.github.com/wens/ae17aed6728b83c0a9c3

If you (or someone else with A31) can verify the results,
maybe we can just drop this patch.


Cheers
ChenYu

[PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Chen-Yu Tsai]

On Sat, Sep 20, 2014 at 5:59 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Sep 17, 2014 at 12:01:46AM +0800, Chen-Yu Tsai wrote:
>> On Tue, Sep 16, 2014 at 11:48 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
>> >> On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
>> >> <maxime.ripard@free-electrons.com> wrote:
>> >> > On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
>> >> >> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
>> >> >>
>> >> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> >> >> ---
>> >> >>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
>> >> >>  1 file changed, 5 insertions(+)
>> >> >>
>> >> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> >> index 8eb2c6d..1117989 100644
>> >> >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
>> >> >> @@ -317,6 +317,11 @@
>> >> >>                       clocks = <&ahb1_gates 6>;
>> >> >>                       resets = <&ahb1_rst 6>;
>> >> >>                       #dma-cells = <1>;
>> >> >> +
>> >> >> +                     /* DMA controller requires AHB1 clocked from PLL6 */
>> >> >> +                     assigned-clocks = <&ahb1>;
>> >> >> +                     assigned-clock-parents = <&pll6>;
>> >> >> +                     assigned-clock-rates = <200000000>;
>> >> >
>> >> > Where did you get that from?
>> >> >
>> >> > The user manual says that it should be clocked at 600MHz, and I'm not
>> >> > sure it should be enforced there either.
>> >>
>> >> The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
>> >> not "pll6 should be 200 MHz". I assume you were misled by them.
>> >>
>> >> Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
>> >> vendor BSP default:
>> >>
>> >> On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
>> >> ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
>> >> pll6, which sets pre-divider to /3 and divider to /1.
>> >>
>> >> Hope this clears it up. :)
>> >
>> > It does, thanks :)
>> >
>> > But still, I find it the wrong place to enforce such a limit. This
>> > should go into the clock driver itself. The DMA controller requires
>> > such a parenting, but it doesn't require any specific rate, this is
>> > more something of the global system policy.
>>
>> But I am guessing the AHB bus clock cannot be clock at any arbitrary
>> rate. Just muxing without setting a specific clock rate might result
>> in a hang. We could address this with per clock limitations, like the
>> patch you posted earlier. Not sure if there's anything in the kernel
>> like that at the moment, or how the clock framework handles clock rates
>> after re-parenting.
>
> There's the Tomeu patch series that allows to set clock rate
> requirements from the user side, I guess that could be easily extended
> to integrate some clock side boundaries. That's why I stopped working
> on the sun5i MMC clock issue until that patch is merged.

I could take a look, but AFAIK none of our drivers need to propagate
clock rate changes.

>> Or we could just specify in the clock driver, or in the DT via clock
>> supplier defaults, that the AHB bus should be clocked from PLL6 @ 200 MHz.
>>
>> The latter seems easier to do.
>
> IIRC, there was some opposition to having the muxing assignment in the
> clock driver.
>
>> Also, I tested the DMA controller using dmatest on my A31 Hummingbird
>> without this re-parenting patch. It worked fine. How did your tests fail?
>
> It was just timeouting, and actually, it was only working to the SRAM,
> and not the DRAM.
>
> Maybe the bootloader sets up the clock muxing already?

I just reran my tests on the A31, and dmatest indeed passes.
I'm using your A31 sdcard.img, replaced u-boot with one built from
u-boot-sunxi, so it supports DT.

I also checked both the clock framework and the raw registers.
AHB1 is indeed clocked from AXI on my system.

Full log: https://gist.github.com/wens/ae17aed6728b83c0a9c3

If you (or someone else with A31) can verify the results,
maybe we can just drop this patch.


Cheers
ChenYu

Re: [PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4987 bytes --]

On Sun, Sep 21, 2014 at 04:31:06PM +0800, Chen-Yu Tsai wrote:
> On Sat, Sep 20, 2014 at 5:59 PM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On Wed, Sep 17, 2014 at 12:01:46AM +0800, Chen-Yu Tsai wrote:
> >> On Tue, Sep 16, 2014 at 11:48 PM, Maxime Ripard
> >> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >> > On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
> >> >> On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
> >> >> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >> >> > On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
> >> >> >> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
> >> >> >>
> >> >> >> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> >> >> >> ---
> >> >> >>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
> >> >> >>  1 file changed, 5 insertions(+)
> >> >> >>
> >> >> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> >> index 8eb2c6d..1117989 100644
> >> >> >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> >> @@ -317,6 +317,11 @@
> >> >> >>                       clocks = <&ahb1_gates 6>;
> >> >> >>                       resets = <&ahb1_rst 6>;
> >> >> >>                       #dma-cells = <1>;
> >> >> >> +
> >> >> >> +                     /* DMA controller requires AHB1 clocked from PLL6 */
> >> >> >> +                     assigned-clocks = <&ahb1>;
> >> >> >> +                     assigned-clock-parents = <&pll6>;
> >> >> >> +                     assigned-clock-rates = <200000000>;
> >> >> >
> >> >> > Where did you get that from?
> >> >> >
> >> >> > The user manual says that it should be clocked at 600MHz, and I'm not
> >> >> > sure it should be enforced there either.
> >> >>
> >> >> The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
> >> >> not "pll6 should be 200 MHz". I assume you were misled by them.
> >> >>
> >> >> Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
> >> >> vendor BSP default:
> >> >>
> >> >> On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
> >> >> ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
> >> >> pll6, which sets pre-divider to /3 and divider to /1.
> >> >>
> >> >> Hope this clears it up. :)
> >> >
> >> > It does, thanks :)
> >> >
> >> > But still, I find it the wrong place to enforce such a limit. This
> >> > should go into the clock driver itself. The DMA controller requires
> >> > such a parenting, but it doesn't require any specific rate, this is
> >> > more something of the global system policy.
> >>
> >> But I am guessing the AHB bus clock cannot be clock at any arbitrary
> >> rate. Just muxing without setting a specific clock rate might result
> >> in a hang. We could address this with per clock limitations, like the
> >> patch you posted earlier. Not sure if there's anything in the kernel
> >> like that at the moment, or how the clock framework handles clock rates
> >> after re-parenting.
> >
> > There's the Tomeu patch series that allows to set clock rate
> > requirements from the user side, I guess that could be easily extended
> > to integrate some clock side boundaries. That's why I stopped working
> > on the sun5i MMC clock issue until that patch is merged.
> 
> I could take a look, but AFAIK none of our drivers need to propagate
> clock rate changes.

Don't worry about this.

> >> Or we could just specify in the clock driver, or in the DT via clock
> >> supplier defaults, that the AHB bus should be clocked from PLL6 @ 200 MHz.
> >>
> >> The latter seems easier to do.
> >
> > IIRC, there was some opposition to having the muxing assignment in the
> > clock driver.
> >
> >> Also, I tested the DMA controller using dmatest on my A31 Hummingbird
> >> without this re-parenting patch. It worked fine. How did your tests fail?
> >
> > It was just timeouting, and actually, it was only working to the SRAM,
> > and not the DRAM.
> >
> > Maybe the bootloader sets up the clock muxing already?
> 
> I just reran my tests on the A31, and dmatest indeed passes.
> I'm using your A31 sdcard.img, replaced u-boot with one built from
> u-boot-sunxi, so it supports DT.
> 
> I also checked both the clock framework and the raw registers.
> AHB1 is indeed clocked from AXI on my system.
> 
> Full log: https://gist.github.com/wens/ae17aed6728b83c0a9c3
> 
> If you (or someone else with A31) can verify the results,
> maybe we can just drop this patch.

I'm away from the colombus board this week, but if it works on the
hummingbird, we can just move this patch to the colombus DT, and not
put it in the DTSI.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 6/7] ARM: dts: sun6i: Add required ahb1 clock parent and rates for dma controller

[Maxime Ripard]

On Sun, Sep 21, 2014 at 04:31:06PM +0800, Chen-Yu Tsai wrote:
> On Sat, Sep 20, 2014 at 5:59 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Wed, Sep 17, 2014 at 12:01:46AM +0800, Chen-Yu Tsai wrote:
> >> On Tue, Sep 16, 2014 at 11:48 PM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > On Fri, Sep 12, 2014 at 10:10:25AM +0800, Chen-Yu Tsai wrote:
> >> >> On Fri, Sep 12, 2014 at 5:15 AM, Maxime Ripard
> >> >> <maxime.ripard@free-electrons.com> wrote:
> >> >> > On Sat, Sep 06, 2014 at 06:47:27PM +0800, Chen-Yu Tsai wrote:
> >> >> >> The DMA controller requires AHB1 bus clock to be clocked from PLL6.
> >> >> >>
> >> >> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >> >> >> ---
> >> >> >>  arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
> >> >> >>  1 file changed, 5 insertions(+)
> >> >> >>
> >> >> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> >> index 8eb2c6d..1117989 100644
> >> >> >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> >> >> @@ -317,6 +317,11 @@
> >> >> >>                       clocks = <&ahb1_gates 6>;
> >> >> >>                       resets = <&ahb1_rst 6>;
> >> >> >>                       #dma-cells = <1>;
> >> >> >> +
> >> >> >> +                     /* DMA controller requires AHB1 clocked from PLL6 */
> >> >> >> +                     assigned-clocks = <&ahb1>;
> >> >> >> +                     assigned-clock-parents = <&pll6>;
> >> >> >> +                     assigned-clock-rates = <200000000>;
> >> >> >
> >> >> > Where did you get that from?
> >> >> >
> >> >> > The user manual says that it should be clocked at 600MHz, and I'm not
> >> >> > sure it should be enforced there either.
> >> >>
> >> >> The bindings mean that ahb1 should be clocked from pll6 and at 200 MHz,
> >> >> not "pll6 should be 200 MHz". I assume you were misled by them.
> >> >>
> >> >> Clocking ahb1 from pll6 and at 200 MHz with the /3 pre-divider is the
> >> >> vendor BSP default:
> >> >>
> >> >> On sun6i, the clock init code calls aw_ccu_switch_ahb_2_pll6(), which muxes
> >> >> ahb1 from pll6 with the highest dividers, then sets the rate for ahb1 to
> >> >> pll6, which sets pre-divider to /3 and divider to /1.
> >> >>
> >> >> Hope this clears it up. :)
> >> >
> >> > It does, thanks :)
> >> >
> >> > But still, I find it the wrong place to enforce such a limit. This
> >> > should go into the clock driver itself. The DMA controller requires
> >> > such a parenting, but it doesn't require any specific rate, this is
> >> > more something of the global system policy.
> >>
> >> But I am guessing the AHB bus clock cannot be clock at any arbitrary
> >> rate. Just muxing without setting a specific clock rate might result
> >> in a hang. We could address this with per clock limitations, like the
> >> patch you posted earlier. Not sure if there's anything in the kernel
> >> like that at the moment, or how the clock framework handles clock rates
> >> after re-parenting.
> >
> > There's the Tomeu patch series that allows to set clock rate
> > requirements from the user side, I guess that could be easily extended
> > to integrate some clock side boundaries. That's why I stopped working
> > on the sun5i MMC clock issue until that patch is merged.
> 
> I could take a look, but AFAIK none of our drivers need to propagate
> clock rate changes.

Don't worry about this.

> >> Or we could just specify in the clock driver, or in the DT via clock
> >> supplier defaults, that the AHB bus should be clocked from PLL6 @ 200 MHz.
> >>
> >> The latter seems easier to do.
> >
> > IIRC, there was some opposition to having the muxing assignment in the
> > clock driver.
> >
> >> Also, I tested the DMA controller using dmatest on my A31 Hummingbird
> >> without this re-parenting patch. It worked fine. How did your tests fail?
> >
> > It was just timeouting, and actually, it was only working to the SRAM,
> > and not the DRAM.
> >
> > Maybe the bootloader sets up the clock muxing already?
> 
> I just reran my tests on the A31, and dmatest indeed passes.
> I'm using your A31 sdcard.img, replaced u-boot with one built from
> u-boot-sunxi, so it supports DT.
> 
> I also checked both the clock framework and the raw registers.
> AHB1 is indeed clocked from AXI on my system.
> 
> Full log: https://gist.github.com/wens/ae17aed6728b83c0a9c3
> 
> If you (or someone else with A31) can verify the results,
> maybe we can just drop this patch.

I'm away from the colombus board this week, but if it works on the
hummingbird, we can just move this patch to the colombus DT, and not
put it in the DTSI.

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/20140925/dacb99ce/attachment.sig>

[PATCH 7/7] dmaengine: sun6i: Remove obsolete clk muxing code

[Chen-Yu Tsai]

The sun6i DMA controller requires the AHB1 bus clock to be
clocked from PLL6. This was originally done by the dmaengine
driver during probe time. The AHB1 clock driver has since been
unified, so the original code does not work.

Remove the clk muxing code, and replace it with DT clk default
properties.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 drivers/dma/sun6i-dma.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 1f92a56..3aa10b3 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -862,7 +862,6 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 {
 	struct sun6i_dma_dev *sdc;
 	struct resource *res;
-	struct clk *mux, *pll6;
 	int ret, i;
 
 	sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
@@ -886,28 +885,6 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 		return PTR_ERR(sdc->clk);
 	}
 
-	mux = clk_get(NULL, "ahb1_mux");
-	if (IS_ERR(mux)) {
-		dev_err(&pdev->dev, "Couldn't get AHB1 Mux\n");
-		return PTR_ERR(mux);
-	}
-
-	pll6 = clk_get(NULL, "pll6");
-	if (IS_ERR(pll6)) {
-		dev_err(&pdev->dev, "Couldn't get PLL6\n");
-		clk_put(mux);
-		return PTR_ERR(pll6);
-	}
-
-	ret = clk_set_parent(mux, pll6);
-	clk_put(pll6);
-	clk_put(mux);
-
-	if (ret) {
-		dev_err(&pdev->dev, "Couldn't reparent AHB1 on PLL6\n");
-		return ret;
-	}
-
 	sdc->rstc = devm_reset_control_get(&pdev->dev, NULL);
 	if (IS_ERR(sdc->rstc)) {
 		dev_err(&pdev->dev, "No reset controller specified\n");
-- 
2.1.0

[PATCH 7/7] dmaengine: sun6i: Remove obsolete clk muxing code

[Chen-Yu Tsai]

The sun6i DMA controller requires the AHB1 bus clock to be
clocked from PLL6. This was originally done by the dmaengine
driver during probe time. The AHB1 clock driver has since been
unified, so the original code does not work.

Remove the clk muxing code, and replace it with DT clk default
properties.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/dma/sun6i-dma.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 1f92a56..3aa10b3 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -862,7 +862,6 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 {
 	struct sun6i_dma_dev *sdc;
 	struct resource *res;
-	struct clk *mux, *pll6;
 	int ret, i;
 
 	sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
@@ -886,28 +885,6 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 		return PTR_ERR(sdc->clk);
 	}
 
-	mux = clk_get(NULL, "ahb1_mux");
-	if (IS_ERR(mux)) {
-		dev_err(&pdev->dev, "Couldn't get AHB1 Mux\n");
-		return PTR_ERR(mux);
-	}
-
-	pll6 = clk_get(NULL, "pll6");
-	if (IS_ERR(pll6)) {
-		dev_err(&pdev->dev, "Couldn't get PLL6\n");
-		clk_put(mux);
-		return PTR_ERR(pll6);
-	}
-
-	ret = clk_set_parent(mux, pll6);
-	clk_put(pll6);
-	clk_put(mux);
-
-	if (ret) {
-		dev_err(&pdev->dev, "Couldn't reparent AHB1 on PLL6\n");
-		return ret;
-	}
-
 	sdc->rstc = devm_reset_control_get(&pdev->dev, NULL);
 	if (IS_ERR(sdc->rstc)) {
 		dev_err(&pdev->dev, "No reset controller specified\n");
-- 
2.1.0

Re: [PATCH 7/7] dmaengine: sun6i: Remove obsolete clk muxing code

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

On Sat, Sep 06, 2014 at 06:47:28PM +0800, Chen-Yu Tsai wrote:
> The sun6i DMA controller requires the AHB1 bus clock to be
> clocked from PLL6. This was originally done by the dmaengine
> driver during probe time. The AHB1 clock driver has since been
> unified, so the original code does not work.
> 
> Remove the clk muxing code, and replace it with DT clk default
> properties.
> 
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 7/7] dmaengine: sun6i: Remove obsolete clk muxing code

[Maxime Ripard]

On Sat, Sep 06, 2014 at 06:47:28PM +0800, Chen-Yu Tsai wrote:
> The sun6i DMA controller requires the AHB1 bus clock to be
> clocked from PLL6. This was originally done by the dmaengine
> driver during probe time. The AHB1 clock driver has since been
> unified, so the original code does not work.
> 
> Remove the clk muxing code, and replace it with DT clk default
> properties.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
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/20140911/070a48b6/attachment.sig>

Re: [PATCH 7/7] dmaengine: sun6i: Remove obsolete clk muxing code

[Vinod Koul]

On Sat, Sep 06, 2014 at 06:47:28PM +0800, Chen-Yu Tsai wrote:
> The sun6i DMA controller requires the AHB1 bus clock to be
> clocked from PLL6. This was originally done by the dmaengine
> driver during probe time. The AHB1 clock driver has since been
> unified, so the original code does not work.
> 
> Remove the clk muxing code, and replace it with DT clk default
> properties.

Applied, thanks

-- 
~Vinod

[PATCH 7/7] dmaengine: sun6i: Remove obsolete clk muxing code

[Vinod Koul]

On Sat, Sep 06, 2014 at 06:47:28PM +0800, Chen-Yu Tsai wrote:
> The sun6i DMA controller requires the AHB1 bus clock to be
> clocked from PLL6. This was originally done by the dmaengine
> driver during probe time. The AHB1 clock driver has since been
> unified, so the original code does not work.
> 
> Remove the clk muxing code, and replace it with DT clk default
> properties.

Applied, thanks

-- 
~Vinod

Re: [PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

Hi Chen-Yu,

On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
> Hi everyone,
> 
> This series unifies the mux and divider parts of the AHB1 clock found
> on sun6i and sun8i, while also adding support for the pre-divider on
> the PLL6 input.
> 
> The rate calculation logic must factor in which parent it is using to
> calculate the rate, to decide whether to use the pre-divider or not.
> This is beyond the original factors clk design in sunxi. To avoid
> feature bloat, this is implemented as a seperate composite clk.
> 
> The new clock driver is registered with a separate OF_CLK_DECLARE.
> This is done so that assigned-clocks* properties on the clk provider
> node can actually work. The clock framework arranges the clock setup
> order by checking whether all clock parents are available, by checking
> the node matching OF_CLK_DECLARE.
> 
> However, the sunxi clk driver is based on the root node compatible,
> has no defined dependencies (parents), and is setup before the fixed-rate
> clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
> There is no way to calculate the required clock factors to set a default
> clock rate under these circumstances. This happens when we set the
> defaults in the clock node (provider), rather than a clock consumer.
>
> I can think of 2 ways to solve the dependency issue, but neither is
> pretty. One would be to move the root fixed-rate clocks into the sunxi
> clk driver. The other would be separating all the clocks into individual
> OF_CLK_DECLARE statements, which adds a lot of boilerplate code.

I don't know what Mike thinks of this, but I'd prefer the second.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Maxime Ripard]

Hi Chen-Yu,

On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
> Hi everyone,
> 
> This series unifies the mux and divider parts of the AHB1 clock found
> on sun6i and sun8i, while also adding support for the pre-divider on
> the PLL6 input.
> 
> The rate calculation logic must factor in which parent it is using to
> calculate the rate, to decide whether to use the pre-divider or not.
> This is beyond the original factors clk design in sunxi. To avoid
> feature bloat, this is implemented as a seperate composite clk.
> 
> The new clock driver is registered with a separate OF_CLK_DECLARE.
> This is done so that assigned-clocks* properties on the clk provider
> node can actually work. The clock framework arranges the clock setup
> order by checking whether all clock parents are available, by checking
> the node matching OF_CLK_DECLARE.
> 
> However, the sunxi clk driver is based on the root node compatible,
> has no defined dependencies (parents), and is setup before the fixed-rate
> clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
> There is no way to calculate the required clock factors to set a default
> clock rate under these circumstances. This happens when we set the
> defaults in the clock node (provider), rather than a clock consumer.
>
> I can think of 2 ways to solve the dependency issue, but neither is
> pretty. One would be to move the root fixed-rate clocks into the sunxi
> clk driver. The other would be separating all the clocks into individual
> OF_CLK_DECLARE statements, which adds a lot of boilerplate code.

I don't know what Mike thinks of this, but I'd prefer the second.

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/20140911/c5b977f3/attachment.sig>

Re: [PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Mike Turquette]

Quoting Maxime Ripard (2014-09-11 13:36:23)
> Hi Chen-Yu,
> 
> On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
> > Hi everyone,
> > 
> > This series unifies the mux and divider parts of the AHB1 clock found
> > on sun6i and sun8i, while also adding support for the pre-divider on
> > the PLL6 input.
> > 
> > The rate calculation logic must factor in which parent it is using to
> > calculate the rate, to decide whether to use the pre-divider or not.
> > This is beyond the original factors clk design in sunxi. To avoid
> > feature bloat, this is implemented as a seperate composite clk.
> > 
> > The new clock driver is registered with a separate OF_CLK_DECLARE.
> > This is done so that assigned-clocks* properties on the clk provider
> > node can actually work. The clock framework arranges the clock setup
> > order by checking whether all clock parents are available, by checking
> > the node matching OF_CLK_DECLARE.
> > 
> > However, the sunxi clk driver is based on the root node compatible,
> > has no defined dependencies (parents), and is setup before the fixed-rate
> > clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
> > There is no way to calculate the required clock factors to set a default
> > clock rate under these circumstances. This happens when we set the
> > defaults in the clock node (provider), rather than a clock consumer.
> >
> > I can think of 2 ways to solve the dependency issue, but neither is
> > pretty. One would be to move the root fixed-rate clocks into the sunxi
> > clk driver. The other would be separating all the clocks into individual
> > OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
> 
> I don't know what Mike thinks of this, but I'd prefer the second.

I do not fully understand the problem. Ideally the clock driver should
have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
registered. Those fixed-rate parents are enumerated in your dts, right?
Why isn't this enough?

Thanks,
Mike

> 
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Mike Turquette]

Quoting Maxime Ripard (2014-09-11 13:36:23)
> Hi Chen-Yu,
> 
> On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
> > Hi everyone,
> > 
> > This series unifies the mux and divider parts of the AHB1 clock found
> > on sun6i and sun8i, while also adding support for the pre-divider on
> > the PLL6 input.
> > 
> > The rate calculation logic must factor in which parent it is using to
> > calculate the rate, to decide whether to use the pre-divider or not.
> > This is beyond the original factors clk design in sunxi. To avoid
> > feature bloat, this is implemented as a seperate composite clk.
> > 
> > The new clock driver is registered with a separate OF_CLK_DECLARE.
> > This is done so that assigned-clocks* properties on the clk provider
> > node can actually work. The clock framework arranges the clock setup
> > order by checking whether all clock parents are available, by checking
> > the node matching OF_CLK_DECLARE.
> > 
> > However, the sunxi clk driver is based on the root node compatible,
> > has no defined dependencies (parents), and is setup before the fixed-rate
> > clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
> > There is no way to calculate the required clock factors to set a default
> > clock rate under these circumstances. This happens when we set the
> > defaults in the clock node (provider), rather than a clock consumer.
> >
> > I can think of 2 ways to solve the dependency issue, but neither is
> > pretty. One would be to move the root fixed-rate clocks into the sunxi
> > clk driver. The other would be separating all the clocks into individual
> > OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
> 
> I don't know what Mike thinks of this, but I'd prefer the second.

I do not fully understand the problem. Ideally the clock driver should
have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
registered. Those fixed-rate parents are enumerated in your dts, right?
Why isn't this enough?

Thanks,
Mike

> 
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Re: [PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Chen-Yu Tsai]

On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Quoting Maxime Ripard (2014-09-11 13:36:23)
>> Hi Chen-Yu,
>>
>> On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
>> > Hi everyone,
>> >
>> > This series unifies the mux and divider parts of the AHB1 clock found
>> > on sun6i and sun8i, while also adding support for the pre-divider on
>> > the PLL6 input.
>> >
>> > The rate calculation logic must factor in which parent it is using to
>> > calculate the rate, to decide whether to use the pre-divider or not.
>> > This is beyond the original factors clk design in sunxi. To avoid
>> > feature bloat, this is implemented as a seperate composite clk.
>> >
>> > The new clock driver is registered with a separate OF_CLK_DECLARE.
>> > This is done so that assigned-clocks* properties on the clk provider
>> > node can actually work. The clock framework arranges the clock setup
>> > order by checking whether all clock parents are available, by checking
>> > the node matching OF_CLK_DECLARE.
>> >
>> > However, the sunxi clk driver is based on the root node compatible,
>> > has no defined dependencies (parents), and is setup before the fixed-rate
>> > clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
>> > There is no way to calculate the required clock factors to set a default
>> > clock rate under these circumstances. This happens when we set the
>> > defaults in the clock node (provider), rather than a clock consumer.
>> >
>> > I can think of 2 ways to solve the dependency issue, but neither is
>> > pretty. One would be to move the root fixed-rate clocks into the sunxi
>> > clk driver. The other would be separating all the clocks into individual
>> > OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
>>
>> I don't know what Mike thinks of this, but I'd prefer the second.
>
> I do not fully understand the problem. Ideally the clock driver should
> have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
> registered. Those fixed-rate parents are enumerated in your dts, right?
> Why isn't this enough?

This is due to the way the sunxi clock driver is setup. The clock driver's
OF_CLK_DECLARE matches against the "soc" node, not the individual clock
nodes. When the setup function is called, it just registers all the
supported clocks. There are no dependencies listed.

Unfortunately, the fixed-factor clock is in the middle of the whole clock
tree. The sunxi clock driver provides its parents _and_ its children.
Naturally the clock framework then probes the fixed-factor clock after
the sunxi ones. Any attempts to change the state of clocks under the
unavailable fixed-factor clock, such as done by of_clk_set_defaults(),
would get an incomplete clock, likely with no parents and parent_rate = 0.
That is until of_clk_init() finishes and all clocks are properly hooked
up.

Anyway, this problem only occurred when I added clk-assigned-* defaults
to the clock provider node, which is not the case anymore.

The second method i proposed is to have OF_CLK_DECLAREs for each individual
clock. An example can be found here:

  https://github.com/wens/linux/commit/1276898da02a93da4af163ed5bdc88cdead565dc

This does add a lot of boilerplate code. Not really happy about it. But
it seems the proper way to split up the driver.


Cheers
ChenYu

[PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Chen-Yu Tsai]

On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Maxime Ripard (2014-09-11 13:36:23)
>> Hi Chen-Yu,
>>
>> On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
>> > Hi everyone,
>> >
>> > This series unifies the mux and divider parts of the AHB1 clock found
>> > on sun6i and sun8i, while also adding support for the pre-divider on
>> > the PLL6 input.
>> >
>> > The rate calculation logic must factor in which parent it is using to
>> > calculate the rate, to decide whether to use the pre-divider or not.
>> > This is beyond the original factors clk design in sunxi. To avoid
>> > feature bloat, this is implemented as a seperate composite clk.
>> >
>> > The new clock driver is registered with a separate OF_CLK_DECLARE.
>> > This is done so that assigned-clocks* properties on the clk provider
>> > node can actually work. The clock framework arranges the clock setup
>> > order by checking whether all clock parents are available, by checking
>> > the node matching OF_CLK_DECLARE.
>> >
>> > However, the sunxi clk driver is based on the root node compatible,
>> > has no defined dependencies (parents), and is setup before the fixed-rate
>> > clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
>> > There is no way to calculate the required clock factors to set a default
>> > clock rate under these circumstances. This happens when we set the
>> > defaults in the clock node (provider), rather than a clock consumer.
>> >
>> > I can think of 2 ways to solve the dependency issue, but neither is
>> > pretty. One would be to move the root fixed-rate clocks into the sunxi
>> > clk driver. The other would be separating all the clocks into individual
>> > OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
>>
>> I don't know what Mike thinks of this, but I'd prefer the second.
>
> I do not fully understand the problem. Ideally the clock driver should
> have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
> registered. Those fixed-rate parents are enumerated in your dts, right?
> Why isn't this enough?

This is due to the way the sunxi clock driver is setup. The clock driver's
OF_CLK_DECLARE matches against the "soc" node, not the individual clock
nodes. When the setup function is called, it just registers all the
supported clocks. There are no dependencies listed.

Unfortunately, the fixed-factor clock is in the middle of the whole clock
tree. The sunxi clock driver provides its parents _and_ its children.
Naturally the clock framework then probes the fixed-factor clock after
the sunxi ones. Any attempts to change the state of clocks under the
unavailable fixed-factor clock, such as done by of_clk_set_defaults(),
would get an incomplete clock, likely with no parents and parent_rate = 0.
That is until of_clk_init() finishes and all clocks are properly hooked
up.

Anyway, this problem only occurred when I added clk-assigned-* defaults
to the clock provider node, which is not the case anymore.

The second method i proposed is to have OF_CLK_DECLAREs for each individual
clock. An example can be found here:

  https://github.com/wens/linux/commit/1276898da02a93da4af163ed5bdc88cdead565dc

This does add a lot of boilerplate code. Not really happy about it. But
it seems the proper way to split up the driver.


Cheers
ChenYu

Re: [PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Mike Turquette]

Quoting Chen-Yu Tsai (2014-09-25 17:55:27)
> On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > Quoting Maxime Ripard (2014-09-11 13:36:23)
> >> Hi Chen-Yu,
> >>
> >> On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
> >> > Hi everyone,
> >> >
> >> > This series unifies the mux and divider parts of the AHB1 clock found
> >> > on sun6i and sun8i, while also adding support for the pre-divider on
> >> > the PLL6 input.
> >> >
> >> > The rate calculation logic must factor in which parent it is using to
> >> > calculate the rate, to decide whether to use the pre-divider or not.
> >> > This is beyond the original factors clk design in sunxi. To avoid
> >> > feature bloat, this is implemented as a seperate composite clk.
> >> >
> >> > The new clock driver is registered with a separate OF_CLK_DECLARE.
> >> > This is done so that assigned-clocks* properties on the clk provider
> >> > node can actually work. The clock framework arranges the clock setup
> >> > order by checking whether all clock parents are available, by checking
> >> > the node matching OF_CLK_DECLARE.
> >> >
> >> > However, the sunxi clk driver is based on the root node compatible,
> >> > has no defined dependencies (parents), and is setup before the fixed-rate
> >> > clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
> >> > There is no way to calculate the required clock factors to set a default
> >> > clock rate under these circumstances. This happens when we set the
> >> > defaults in the clock node (provider), rather than a clock consumer.
> >> >
> >> > I can think of 2 ways to solve the dependency issue, but neither is
> >> > pretty. One would be to move the root fixed-rate clocks into the sunxi
> >> > clk driver. The other would be separating all the clocks into individual
> >> > OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
> >>
> >> I don't know what Mike thinks of this, but I'd prefer the second.
> >
> > I do not fully understand the problem. Ideally the clock driver should
> > have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
> > registered. Those fixed-rate parents are enumerated in your dts, right?
> > Why isn't this enough?
> 
> This is due to the way the sunxi clock driver is setup. The clock driver's
> OF_CLK_DECLARE matches against the "soc" node, not the individual clock
> nodes. When the setup function is called, it just registers all the
> supported clocks. There are no dependencies listed.
> 
> Unfortunately, the fixed-factor clock is in the middle of the whole clock
> tree. The sunxi clock driver provides its parents _and_ its children.
> Naturally the clock framework then probes the fixed-factor clock after
> the sunxi ones. Any attempts to change the state of clocks under the
> unavailable fixed-factor clock, such as done by of_clk_set_defaults(),
> would get an incomplete clock, likely with no parents and parent_rate = 0.
> That is until of_clk_init() finishes and all clocks are properly hooked
> up.
> 
> Anyway, this problem only occurred when I added clk-assigned-* defaults
> to the clock provider node, which is not the case anymore.

Makes sense. I guess you could ignore the problem until you need to use
the assigned defaults.

> 
> The second method i proposed is to have OF_CLK_DECLAREs for each individual
> clock. An example can be found here:
> 
>   https://github.com/wens/linux/commit/1276898da02a93da4af163ed5bdc88cdead565dc
> 
> This does add a lot of boilerplate code. Not really happy about it. But
> it seems the proper way to split up the driver.

Yeah, this is OK. Ugly, but OK.

Regards,
Mike

> 
> 
> Cheers
> ChenYu

[PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Mike Turquette]

Quoting Chen-Yu Tsai (2014-09-25 17:55:27)
> On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette <mturquette@linaro.org> wrote:
> > Quoting Maxime Ripard (2014-09-11 13:36:23)
> >> Hi Chen-Yu,
> >>
> >> On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
> >> > Hi everyone,
> >> >
> >> > This series unifies the mux and divider parts of the AHB1 clock found
> >> > on sun6i and sun8i, while also adding support for the pre-divider on
> >> > the PLL6 input.
> >> >
> >> > The rate calculation logic must factor in which parent it is using to
> >> > calculate the rate, to decide whether to use the pre-divider or not.
> >> > This is beyond the original factors clk design in sunxi. To avoid
> >> > feature bloat, this is implemented as a seperate composite clk.
> >> >
> >> > The new clock driver is registered with a separate OF_CLK_DECLARE.
> >> > This is done so that assigned-clocks* properties on the clk provider
> >> > node can actually work. The clock framework arranges the clock setup
> >> > order by checking whether all clock parents are available, by checking
> >> > the node matching OF_CLK_DECLARE.
> >> >
> >> > However, the sunxi clk driver is based on the root node compatible,
> >> > has no defined dependencies (parents), and is setup before the fixed-rate
> >> > clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
> >> > There is no way to calculate the required clock factors to set a default
> >> > clock rate under these circumstances. This happens when we set the
> >> > defaults in the clock node (provider), rather than a clock consumer.
> >> >
> >> > I can think of 2 ways to solve the dependency issue, but neither is
> >> > pretty. One would be to move the root fixed-rate clocks into the sunxi
> >> > clk driver. The other would be separating all the clocks into individual
> >> > OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
> >>
> >> I don't know what Mike thinks of this, but I'd prefer the second.
> >
> > I do not fully understand the problem. Ideally the clock driver should
> > have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
> > registered. Those fixed-rate parents are enumerated in your dts, right?
> > Why isn't this enough?
> 
> This is due to the way the sunxi clock driver is setup. The clock driver's
> OF_CLK_DECLARE matches against the "soc" node, not the individual clock
> nodes. When the setup function is called, it just registers all the
> supported clocks. There are no dependencies listed.
> 
> Unfortunately, the fixed-factor clock is in the middle of the whole clock
> tree. The sunxi clock driver provides its parents _and_ its children.
> Naturally the clock framework then probes the fixed-factor clock after
> the sunxi ones. Any attempts to change the state of clocks under the
> unavailable fixed-factor clock, such as done by of_clk_set_defaults(),
> would get an incomplete clock, likely with no parents and parent_rate = 0.
> That is until of_clk_init() finishes and all clocks are properly hooked
> up.
> 
> Anyway, this problem only occurred when I added clk-assigned-* defaults
> to the clock provider node, which is not the case anymore.

Makes sense. I guess you could ignore the problem until you need to use
the assigned defaults.

> 
> The second method i proposed is to have OF_CLK_DECLAREs for each individual
> clock. An example can be found here:
> 
>   https://github.com/wens/linux/commit/1276898da02a93da4af163ed5bdc88cdead565dc
> 
> This does add a lot of boilerplate code. Not really happy about it. But
> it seems the proper way to split up the driver.

Yeah, this is OK. Ugly, but OK.

Regards,
Mike

> 
> 
> Cheers
> ChenYu

Re: [PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Chen-Yu Tsai]

On Sat, Sep 27, 2014 at 2:53 AM, Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Quoting Chen-Yu Tsai (2014-09-25 17:55:27)
>> On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> > Quoting Maxime Ripard (2014-09-11 13:36:23)
>> >> Hi Chen-Yu,
>> >>
>> >> On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
>> >> > Hi everyone,
>> >> >
>> >> > This series unifies the mux and divider parts of the AHB1 clock found
>> >> > on sun6i and sun8i, while also adding support for the pre-divider on
>> >> > the PLL6 input.
>> >> >
>> >> > The rate calculation logic must factor in which parent it is using to
>> >> > calculate the rate, to decide whether to use the pre-divider or not.
>> >> > This is beyond the original factors clk design in sunxi. To avoid
>> >> > feature bloat, this is implemented as a seperate composite clk.
>> >> >
>> >> > The new clock driver is registered with a separate OF_CLK_DECLARE.
>> >> > This is done so that assigned-clocks* properties on the clk provider
>> >> > node can actually work. The clock framework arranges the clock setup
>> >> > order by checking whether all clock parents are available, by checking
>> >> > the node matching OF_CLK_DECLARE.
>> >> >
>> >> > However, the sunxi clk driver is based on the root node compatible,
>> >> > has no defined dependencies (parents), and is setup before the fixed-rate
>> >> > clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
>> >> > There is no way to calculate the required clock factors to set a default
>> >> > clock rate under these circumstances. This happens when we set the
>> >> > defaults in the clock node (provider), rather than a clock consumer.
>> >> >
>> >> > I can think of 2 ways to solve the dependency issue, but neither is
>> >> > pretty. One would be to move the root fixed-rate clocks into the sunxi
>> >> > clk driver. The other would be separating all the clocks into individual
>> >> > OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
>> >>
>> >> I don't know what Mike thinks of this, but I'd prefer the second.
>> >
>> > I do not fully understand the problem. Ideally the clock driver should
>> > have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
>> > registered. Those fixed-rate parents are enumerated in your dts, right?
>> > Why isn't this enough?
>>
>> This is due to the way the sunxi clock driver is setup. The clock driver's
>> OF_CLK_DECLARE matches against the "soc" node, not the individual clock
>> nodes. When the setup function is called, it just registers all the
>> supported clocks. There are no dependencies listed.
>>
>> Unfortunately, the fixed-factor clock is in the middle of the whole clock
>> tree. The sunxi clock driver provides its parents _and_ its children.
>> Naturally the clock framework then probes the fixed-factor clock after
>> the sunxi ones. Any attempts to change the state of clocks under the
>> unavailable fixed-factor clock, such as done by of_clk_set_defaults(),
>> would get an incomplete clock, likely with no parents and parent_rate = 0.
>> That is until of_clk_init() finishes and all clocks are properly hooked
>> up.
>>
>> Anyway, this problem only occurred when I added clk-assigned-* defaults
>> to the clock provider node, which is not the case anymore.
>
> Makes sense. I guess you could ignore the problem until you need to use
> the assigned defaults.

An update on this. Improper ordering of clock probing also affects
sunxi's clock protection code.

Currently we have 2 mechanisms for protecting clocks.

  a) A list of clock names in sunxi/clk-sunxi.c, fetched and enabled
     using clkdev.
  b) Enabling clocks right after they are registered. Used for separated
     clock drivers like sun5i-a13-mbus and sun8i-a23-mbus.

One issue I ran across was when most of the clock tree is registered using
independent CLK_OF_DECLAREs, as I'm doing for the A80, if the protected
clocks list is handled before the clock tree is complete, the prepare
and enable calls are not correctly propagated to the parents that arrive
later on.

This happens to the ahb*_sdram gates.

>> The second method i proposed is to have OF_CLK_DECLAREs for each individual
>> clock. An example can be found here:
>>
>>   https://github.com/wens/linux/commit/1276898da02a93da4af163ed5bdc88cdead565dc
>>
>> This does add a lot of boilerplate code. Not really happy about it. But
>> it seems the proper way to split up the driver.
>
> Yeah, this is OK. Ugly, but OK.

Thanks.

ChenYu

[PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Chen-Yu Tsai]

On Sat, Sep 27, 2014 at 2:53 AM, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Chen-Yu Tsai (2014-09-25 17:55:27)
>> On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette <mturquette@linaro.org> wrote:
>> > Quoting Maxime Ripard (2014-09-11 13:36:23)
>> >> Hi Chen-Yu,
>> >>
>> >> On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
>> >> > Hi everyone,
>> >> >
>> >> > This series unifies the mux and divider parts of the AHB1 clock found
>> >> > on sun6i and sun8i, while also adding support for the pre-divider on
>> >> > the PLL6 input.
>> >> >
>> >> > The rate calculation logic must factor in which parent it is using to
>> >> > calculate the rate, to decide whether to use the pre-divider or not.
>> >> > This is beyond the original factors clk design in sunxi. To avoid
>> >> > feature bloat, this is implemented as a seperate composite clk.
>> >> >
>> >> > The new clock driver is registered with a separate OF_CLK_DECLARE.
>> >> > This is done so that assigned-clocks* properties on the clk provider
>> >> > node can actually work. The clock framework arranges the clock setup
>> >> > order by checking whether all clock parents are available, by checking
>> >> > the node matching OF_CLK_DECLARE.
>> >> >
>> >> > However, the sunxi clk driver is based on the root node compatible,
>> >> > has no defined dependencies (parents), and is setup before the fixed-rate
>> >> > clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
>> >> > There is no way to calculate the required clock factors to set a default
>> >> > clock rate under these circumstances. This happens when we set the
>> >> > defaults in the clock node (provider), rather than a clock consumer.
>> >> >
>> >> > I can think of 2 ways to solve the dependency issue, but neither is
>> >> > pretty. One would be to move the root fixed-rate clocks into the sunxi
>> >> > clk driver. The other would be separating all the clocks into individual
>> >> > OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
>> >>
>> >> I don't know what Mike thinks of this, but I'd prefer the second.
>> >
>> > I do not fully understand the problem. Ideally the clock driver should
>> > have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
>> > registered. Those fixed-rate parents are enumerated in your dts, right?
>> > Why isn't this enough?
>>
>> This is due to the way the sunxi clock driver is setup. The clock driver's
>> OF_CLK_DECLARE matches against the "soc" node, not the individual clock
>> nodes. When the setup function is called, it just registers all the
>> supported clocks. There are no dependencies listed.
>>
>> Unfortunately, the fixed-factor clock is in the middle of the whole clock
>> tree. The sunxi clock driver provides its parents _and_ its children.
>> Naturally the clock framework then probes the fixed-factor clock after
>> the sunxi ones. Any attempts to change the state of clocks under the
>> unavailable fixed-factor clock, such as done by of_clk_set_defaults(),
>> would get an incomplete clock, likely with no parents and parent_rate = 0.
>> That is until of_clk_init() finishes and all clocks are properly hooked
>> up.
>>
>> Anyway, this problem only occurred when I added clk-assigned-* defaults
>> to the clock provider node, which is not the case anymore.
>
> Makes sense. I guess you could ignore the problem until you need to use
> the assigned defaults.

An update on this. Improper ordering of clock probing also affects
sunxi's clock protection code.

Currently we have 2 mechanisms for protecting clocks.

  a) A list of clock names in sunxi/clk-sunxi.c, fetched and enabled
     using clkdev.
  b) Enabling clocks right after they are registered. Used for separated
     clock drivers like sun5i-a13-mbus and sun8i-a23-mbus.

One issue I ran across was when most of the clock tree is registered using
independent CLK_OF_DECLAREs, as I'm doing for the A80, if the protected
clocks list is handled before the clock tree is complete, the prepare
and enable calls are not correctly propagated to the parents that arrive
later on.

This happens to the ahb*_sdram gates.

>> The second method i proposed is to have OF_CLK_DECLAREs for each individual
>> clock. An example can be found here:
>>
>>   https://github.com/wens/linux/commit/1276898da02a93da4af163ed5bdc88cdead565dc
>>
>> This does add a lot of boilerplate code. Not really happy about it. But
>> it seems the proper way to split up the driver.
>
> Yeah, this is OK. Ugly, but OK.

Thanks.

ChenYu

Re: [PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4761 bytes --]

On Thu, Oct 09, 2014 at 11:16:50AM +0800, Chen-Yu Tsai wrote:
> On Sat, Sep 27, 2014 at 2:53 AM, Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > Quoting Chen-Yu Tsai (2014-09-25 17:55:27)
> >> On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >> > Quoting Maxime Ripard (2014-09-11 13:36:23)
> >> >> Hi Chen-Yu,
> >> >>
> >> >> On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
> >> >> > Hi everyone,
> >> >> >
> >> >> > This series unifies the mux and divider parts of the AHB1 clock found
> >> >> > on sun6i and sun8i, while also adding support for the pre-divider on
> >> >> > the PLL6 input.
> >> >> >
> >> >> > The rate calculation logic must factor in which parent it is using to
> >> >> > calculate the rate, to decide whether to use the pre-divider or not.
> >> >> > This is beyond the original factors clk design in sunxi. To avoid
> >> >> > feature bloat, this is implemented as a seperate composite clk.
> >> >> >
> >> >> > The new clock driver is registered with a separate OF_CLK_DECLARE.
> >> >> > This is done so that assigned-clocks* properties on the clk provider
> >> >> > node can actually work. The clock framework arranges the clock setup
> >> >> > order by checking whether all clock parents are available, by checking
> >> >> > the node matching OF_CLK_DECLARE.
> >> >> >
> >> >> > However, the sunxi clk driver is based on the root node compatible,
> >> >> > has no defined dependencies (parents), and is setup before the fixed-rate
> >> >> > clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
> >> >> > There is no way to calculate the required clock factors to set a default
> >> >> > clock rate under these circumstances. This happens when we set the
> >> >> > defaults in the clock node (provider), rather than a clock consumer.
> >> >> >
> >> >> > I can think of 2 ways to solve the dependency issue, but neither is
> >> >> > pretty. One would be to move the root fixed-rate clocks into the sunxi
> >> >> > clk driver. The other would be separating all the clocks into individual
> >> >> > OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
> >> >>
> >> >> I don't know what Mike thinks of this, but I'd prefer the second.
> >> >
> >> > I do not fully understand the problem. Ideally the clock driver should
> >> > have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
> >> > registered. Those fixed-rate parents are enumerated in your dts, right?
> >> > Why isn't this enough?
> >>
> >> This is due to the way the sunxi clock driver is setup. The clock driver's
> >> OF_CLK_DECLARE matches against the "soc" node, not the individual clock
> >> nodes. When the setup function is called, it just registers all the
> >> supported clocks. There are no dependencies listed.
> >>
> >> Unfortunately, the fixed-factor clock is in the middle of the whole clock
> >> tree. The sunxi clock driver provides its parents _and_ its children.
> >> Naturally the clock framework then probes the fixed-factor clock after
> >> the sunxi ones. Any attempts to change the state of clocks under the
> >> unavailable fixed-factor clock, such as done by of_clk_set_defaults(),
> >> would get an incomplete clock, likely with no parents and parent_rate = 0.
> >> That is until of_clk_init() finishes and all clocks are properly hooked
> >> up.
> >>
> >> Anyway, this problem only occurred when I added clk-assigned-* defaults
> >> to the clock provider node, which is not the case anymore.
> >
> > Makes sense. I guess you could ignore the problem until you need to use
> > the assigned defaults.
> 
> An update on this. Improper ordering of clock probing also affects
> sunxi's clock protection code.
> 
> Currently we have 2 mechanisms for protecting clocks.
> 
>   a) A list of clock names in sunxi/clk-sunxi.c, fetched and enabled
>      using clkdev.
>   b) Enabling clocks right after they are registered. Used for separated
>      clock drivers like sun5i-a13-mbus and sun8i-a23-mbus.
> 
> One issue I ran across was when most of the clock tree is registered using
> independent CLK_OF_DECLAREs, as I'm doing for the A80, if the protected
> clocks list is handled before the clock tree is complete, the prepare
> and enable calls are not correctly propagated to the parents that arrive
> later on.
> 
> This happens to the ahb*_sdram gates.

I guess that eventually, we should be able to remove a). For the time
being, maybe we can just move the clock protection code itself to a
later initcall?

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Maxime Ripard]

On Thu, Oct 09, 2014 at 11:16:50AM +0800, Chen-Yu Tsai wrote:
> On Sat, Sep 27, 2014 at 2:53 AM, Mike Turquette <mturquette@linaro.org> wrote:
> > Quoting Chen-Yu Tsai (2014-09-25 17:55:27)
> >> On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette <mturquette@linaro.org> wrote:
> >> > Quoting Maxime Ripard (2014-09-11 13:36:23)
> >> >> Hi Chen-Yu,
> >> >>
> >> >> On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
> >> >> > Hi everyone,
> >> >> >
> >> >> > This series unifies the mux and divider parts of the AHB1 clock found
> >> >> > on sun6i and sun8i, while also adding support for the pre-divider on
> >> >> > the PLL6 input.
> >> >> >
> >> >> > The rate calculation logic must factor in which parent it is using to
> >> >> > calculate the rate, to decide whether to use the pre-divider or not.
> >> >> > This is beyond the original factors clk design in sunxi. To avoid
> >> >> > feature bloat, this is implemented as a seperate composite clk.
> >> >> >
> >> >> > The new clock driver is registered with a separate OF_CLK_DECLARE.
> >> >> > This is done so that assigned-clocks* properties on the clk provider
> >> >> > node can actually work. The clock framework arranges the clock setup
> >> >> > order by checking whether all clock parents are available, by checking
> >> >> > the node matching OF_CLK_DECLARE.
> >> >> >
> >> >> > However, the sunxi clk driver is based on the root node compatible,
> >> >> > has no defined dependencies (parents), and is setup before the fixed-rate
> >> >> > clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
> >> >> > There is no way to calculate the required clock factors to set a default
> >> >> > clock rate under these circumstances. This happens when we set the
> >> >> > defaults in the clock node (provider), rather than a clock consumer.
> >> >> >
> >> >> > I can think of 2 ways to solve the dependency issue, but neither is
> >> >> > pretty. One would be to move the root fixed-rate clocks into the sunxi
> >> >> > clk driver. The other would be separating all the clocks into individual
> >> >> > OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
> >> >>
> >> >> I don't know what Mike thinks of this, but I'd prefer the second.
> >> >
> >> > I do not fully understand the problem. Ideally the clock driver should
> >> > have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
> >> > registered. Those fixed-rate parents are enumerated in your dts, right?
> >> > Why isn't this enough?
> >>
> >> This is due to the way the sunxi clock driver is setup. The clock driver's
> >> OF_CLK_DECLARE matches against the "soc" node, not the individual clock
> >> nodes. When the setup function is called, it just registers all the
> >> supported clocks. There are no dependencies listed.
> >>
> >> Unfortunately, the fixed-factor clock is in the middle of the whole clock
> >> tree. The sunxi clock driver provides its parents _and_ its children.
> >> Naturally the clock framework then probes the fixed-factor clock after
> >> the sunxi ones. Any attempts to change the state of clocks under the
> >> unavailable fixed-factor clock, such as done by of_clk_set_defaults(),
> >> would get an incomplete clock, likely with no parents and parent_rate = 0.
> >> That is until of_clk_init() finishes and all clocks are properly hooked
> >> up.
> >>
> >> Anyway, this problem only occurred when I added clk-assigned-* defaults
> >> to the clock provider node, which is not the case anymore.
> >
> > Makes sense. I guess you could ignore the problem until you need to use
> > the assigned defaults.
> 
> An update on this. Improper ordering of clock probing also affects
> sunxi's clock protection code.
> 
> Currently we have 2 mechanisms for protecting clocks.
> 
>   a) A list of clock names in sunxi/clk-sunxi.c, fetched and enabled
>      using clkdev.
>   b) Enabling clocks right after they are registered. Used for separated
>      clock drivers like sun5i-a13-mbus and sun8i-a23-mbus.
> 
> One issue I ran across was when most of the clock tree is registered using
> independent CLK_OF_DECLAREs, as I'm doing for the A80, if the protected
> clocks list is handled before the clock tree is complete, the prepare
> and enable calls are not correctly propagated to the parents that arrive
> later on.
> 
> This happens to the ahb*_sdram gates.

I guess that eventually, we should be able to remove a). For the time
being, maybe we can just move the clock protection code itself to a
later initcall?

-- 
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/20141013/1d84f837/attachment.sig>