[PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Ezequiel Garcia]

This patchset adds support for a set of clocks available in Armada 370/XP
known as "Core Divider" clocks.

These are a set of gatable clocks, derived from a fixed PLL at
a configurable ratio. Although this patchset adds support for the more
general case, we only support one of the clocks, namely the NAND ECC clock.

With this infrastructure in place, the addition of the rest of the clocks
would be fairly easy.

In addition, the NAND clock is added as a fixed-ratio from the NAND ECC clock,
to model accurately the SoC clock tree.

A pictorical representation of the clock tree would be:

    PLL
    |
    |
    ---------------------
    |			|
    |			|
    NAND ECC clock	...
    |
    |
    NAND clock

This patchset is based on v3.12-rc2. Any opinions are highly appreciated.

Thanks!

Ezequiel Garcia (5):
  clk: mvebu: Add core-divider clock
  ARM: mvebu: Add Core Divider clock device-tree binding
  ARM: mvebu: Add a 2 GHz fixed-clock Armada 370/XP
  ARM: mvebu: Add the core-divider clock to Armada 370/XP
  ARM: mvebu: Add NAND clock to Armada 370/XP

 .../bindings/clock/mvebu-corediv-clock.txt         |  19 ++
 arch/arm/boot/dts/armada-370-xp.dtsi               |  25 +++
 drivers/clk/mvebu/Kconfig                          |   5 +
 drivers/clk/mvebu/Makefile                         |   1 +
 drivers/clk/mvebu/clk-corediv.c                    | 209 +++++++++++++++++++++
 5 files changed, 259 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
 create mode 100644 drivers/clk/mvebu/clk-corediv.c

-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] ARM: mvebu: Add Core Divider clock device-tree binding

[Ezequiel Garcia]

The Armada 370/XP SoCs have a Core Divider clock providing
several clocks. For now, only the NAND clock is supported.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Ezequiel Garcia <ezequiel.garcia-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../devicetree/bindings/clock/mvebu-corediv-clock.txt | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt

diff --git a/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt b/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
new file mode 100644
index 0000000..ea60f4d
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
@@ -0,0 +1,19 @@
+* Core Divider Clock bindings for Marvell MVEBU SoCs
+
+The following is a list of provided IDs and clock names on Armada 370/XP:
+ 0 = nand_ecc   (NAND ECC clock)
+
+Required properties:
+- compatible : must be "marvell,armada-370-corediv-clock"
+- reg : must be the register address of Core Divider control register
+- #clock-cells : from common clock binding; shall be set to 1
+- #clocks : must be set to the parent's phandle
+
+Example:
+
+corediv_clk: corediv-clocks@18740 {
+	compatible = "marvell,armada-370-corediv-clock";
+	reg = <0x18740 0x8>;
+	#clock-cells = <1>;
+	clocks = <&pll>;
+};
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Ezequiel Garcia]

(Ccing forgotten mvebu maintainers)

On Wed, Sep 25, 2013 at 06:28:17PM -0300, Ezequiel Garcia wrote:
> This patchset adds support for a set of clocks available in Armada 370/XP
> known as "Core Divider" clocks.
> 
> These are a set of gatable clocks, derived from a fixed PLL at
> a configurable ratio. Although this patchset adds support for the more
> general case, we only support one of the clocks, namely the NAND ECC clock.
> 
> With this infrastructure in place, the addition of the rest of the clocks
> would be fairly easy.
> 
> In addition, the NAND clock is added as a fixed-ratio from the NAND ECC clock,
> to model accurately the SoC clock tree.
> 
> A pictorical representation of the clock tree would be:
> 
>     PLL
>     |
>     |
>     ---------------------
>     |			|
>     |			|
>     NAND ECC clock	...
>     |
>     |
>     NAND clock
> 
> This patchset is based on v3.12-rc2. Any opinions are highly appreciated.
> 
> Thanks!
> 
> Ezequiel Garcia (5):
>   clk: mvebu: Add core-divider clock
>   ARM: mvebu: Add Core Divider clock device-tree binding
>   ARM: mvebu: Add a 2 GHz fixed-clock Armada 370/XP
>   ARM: mvebu: Add the core-divider clock to Armada 370/XP
>   ARM: mvebu: Add NAND clock to Armada 370/XP
> 
>  .../bindings/clock/mvebu-corediv-clock.txt         |  19 ++
>  arch/arm/boot/dts/armada-370-xp.dtsi               |  25 +++
>  drivers/clk/mvebu/Kconfig                          |   5 +
>  drivers/clk/mvebu/Makefile                         |   1 +
>  drivers/clk/mvebu/clk-corediv.c                    | 209 +++++++++++++++++++++
>  5 files changed, 259 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
>  create mode 100644 drivers/clk/mvebu/clk-corediv.c
> 
> -- 
> 1.8.1.5
> 

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Gregory CLEMENT]

Hi Ezequiel,

On 25/09/2013 23:37, Ezequiel Garcia wrote:
> (Ccing forgotten mvebu maintainers)
> 
> On Wed, Sep 25, 2013 at 06:28:17PM -0300, Ezequiel Garcia wrote:
>> This patchset adds support for a set of clocks available in Armada 370/XP
>> known as "Core Divider" clocks.
>>
>> These are a set of gatable clocks, derived from a fixed PLL at
>> a configurable ratio. Although this patchset adds support for the more
>> general case, we only support one of the clocks, namely the NAND ECC clock.
>>
>> With this infrastructure in place, the addition of the rest of the clocks
>> would be fairly easy.
>>
>> In addition, the NAND clock is added as a fixed-ratio from the NAND ECC clock,
>> to model accurately the SoC clock tree.
>>
>> A pictorical representation of the clock tree would be:
>>
>>     PLL
>>     |
>>     |
>>     ---------------------
>>     |			|
>>     |			|
>>     NAND ECC clock	...
>>     |
>>     |
>>     NAND clock
>>
>> This patchset is based on v3.12-rc2. Any opinions are highly appreciated.
>>

For the whole series you can add my
Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

I just wonder if you were enable to test the enable and disable for the clock,
because the datasheet is a little fuzzy about it.

Thanks,

Gregory
>> Thanks!
>>
>> Ezequiel Garcia (5):
>>   clk: mvebu: Add core-divider clock
>>   ARM: mvebu: Add Core Divider clock device-tree binding
>>   ARM: mvebu: Add a 2 GHz fixed-clock Armada 370/XP
>>   ARM: mvebu: Add the core-divider clock to Armada 370/XP
>>   ARM: mvebu: Add NAND clock to Armada 370/XP
>>
>>  .../bindings/clock/mvebu-corediv-clock.txt         |  19 ++
>>  arch/arm/boot/dts/armada-370-xp.dtsi               |  25 +++
>>  drivers/clk/mvebu/Kconfig                          |   5 +
>>  drivers/clk/mvebu/Makefile                         |   1 +
>>  drivers/clk/mvebu/clk-corediv.c                    | 209 +++++++++++++++++++++
>>  5 files changed, 259 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/mvebu-corediv-clock.txt
>>  create mode 100644 drivers/clk/mvebu/clk-corediv.c
>>
>> -- 
>> 1.8.1.5
>>
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Ezequiel Garcia]

On Thu, Sep 26, 2013 at 09:38:19AM +0200, Gregory CLEMENT wrote:
> Hi Ezequiel,
> 
> On 25/09/2013 23:37, Ezequiel Garcia wrote:
> > (Ccing forgotten mvebu maintainers)
> > 
> > On Wed, Sep 25, 2013 at 06:28:17PM -0300, Ezequiel Garcia wrote:
> >> This patchset adds support for a set of clocks available in Armada 370/XP
> >> known as "Core Divider" clocks.
> >>
> >> These are a set of gatable clocks, derived from a fixed PLL at
> >> a configurable ratio. Although this patchset adds support for the more
> >> general case, we only support one of the clocks, namely the NAND ECC clock.
> >>
> >> With this infrastructure in place, the addition of the rest of the clocks
> >> would be fairly easy.
> >>
> >> In addition, the NAND clock is added as a fixed-ratio from the NAND ECC clock,
> >> to model accurately the SoC clock tree.
> >>
> >> A pictorical representation of the clock tree would be:
> >>
> >>     PLL
> >>     |
> >>     |
> >>     ---------------------
> >>     |			|
> >>     |			|
> >>     NAND ECC clock	...
> >>     |
> >>     |
> >>     NAND clock
> >>
> >> This patchset is based on v3.12-rc2. Any opinions are highly appreciated.
> >>
> 
> For the whole series you can add my
> Reviewed-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> 

Great! However, I found some issues on this patchset :-(, see below.

> I just wonder if you were enable to test the enable and disable for the clock,
> because the datasheet is a little fuzzy about it.
> 

Enabling and disabling the clock seems to work OK. I must admit I did
just a simple test:

* If NAND DT node does not consume the correct clock, the controller stops
working just as soon as the core clock framework disables the unused clocks.
* If you make the NAND DT node consumer of the "ndclk", the controller
  works fine.

On the other side, I found this approach is incorrect! (silly me!).

Declaring the NAND clock as a fixed-ratio from the NAND ECC clock,
and making the NAND DT consume the NAND clock removes the possibility
of setting the clock rate from the NAND driver.

This happens because the controller's clock is declared as fixed,
whereas the clock that can be adjusted is the NAND ECC (the parent of NAND
clock).

I've been thinking on how to work this out in a clean way, but I'm not
sure which way is cleaner/better. These are my options so far, feel free
to propose any other:

1. The NAND DT node consumes the two clocks: NAND ECC and NAND clock.
It uses the former to set the rate, and the latter to read the rate to
calculate the timings.

This means we have to modify the NAND driver (but not too much).

2. We register the NAND clock in the clk-corediv driver, as a child of
NAND ECC, and implement some set_rate operation to act on NAND ECC.
The NAND DT node can then consume this clock.

This has the advantage that it's transparent to the NAND driver
(which currently consumes one clock).

3. We hack the NAND driver to consume the NAND ECC, but use the rate
as the half of it, and forget about the halved-rate NAND clock.
This seems certainly hacky.

Thoughts?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Thu, 26 Sep 2013 12:26:52 -0300, Ezequiel Garcia wrote:

> 3. We hack the NAND driver to consume the NAND ECC, but use the rate
> as the half of it, and forget about the halved-rate NAND clock.
> This seems certainly hacky.

Is this really hacky? Since we can't change the rate of one without
changing the other, or gating the one without the other, we can also
see those two clocks as being an internal business of the NAND hardware
block. So instead of seeing things as:

                        -------------
 NAND ECC clk    ---->  |           | 
     ||                 |  NAND HW  |
     ||                 |   block   |
     \/                 |           |
 NAND clk        ---->  |           |
                        -------------

You can see things as follows:

                        -------------
                        |           | 
                        |  NAND HW  |
 NAND clk        ---->  |   block   |
                        |           |
                        |           |
                        -------------

and the ECC clock is actually some internal business of the NAND hw
block, and therefore handled internally by the NAND driver, as your
option (3) suggests.

Since the amount of details that we have about the exact hardware
architecture are pretty scarce, I believe this is probably the easiest
solution.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Ezequiel Garcia]

On Thu, Sep 26, 2013 at 05:47:55PM +0200, Thomas Petazzoni wrote:
> Dear Ezequiel Garcia,
> 
> On Thu, 26 Sep 2013 12:26:52 -0300, Ezequiel Garcia wrote:
> 
> > 3. We hack the NAND driver to consume the NAND ECC, but use the rate
> > as the half of it, and forget about the halved-rate NAND clock.
> > This seems certainly hacky.
> 
> Is this really hacky? Since we can't change the rate of one without
> changing the other, or gating the one without the other, we can also
> see those two clocks as being an internal business of the NAND hardware
> block. So instead of seeing things as:
> 
>                         -------------
>  NAND ECC clk    ---->  |           | 
>      ||                 |  NAND HW  |
>      ||                 |   block   |
>      \/                 |           |
>  NAND clk        ---->  |           |
>                         -------------
> 
> You can see things as follows:
> 
>                         -------------
>                         |           | 
>                         |  NAND HW  |
>  NAND clk        ---->  |   block   |
>                         |           |
>                         |           |
>                         -------------
> 
> and the ECC clock is actually some internal business of the NAND hw
> block, and therefore handled internally by the NAND driver, as your
> option (3) suggests.
> 
> Since the amount of details that we have about the exact hardware
> architecture are pretty scarce, I believe this is probably the easiest
> solution.
> 

Hm... could be. Considering the lack of hardware details (as you
point out) maybe this is indeed the best option.

And it has the cool advantage of simplfying the clock tree, which
is unnecessarily complex with the NAND ECC clock -> NAND clock layout.

On the other side, Emilio has just pointed out (in private) that there's
a flag "CLK_SET_RATE_PARENT" that is meant for these cases.

Anyway, I feel inclined to your suggestion and just forget about the NAND
ECC clock for good, which is only confusing.

Thanks,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Add Core Divider clock support for Armada 370/XP

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Thu, 26 Sep 2013 13:00:11 -0300, Ezequiel Garcia wrote:

> Hm... could be. Considering the lack of hardware details (as you
> point out) maybe this is indeed the best option.
> 
> And it has the cool advantage of simplfying the clock tree, which
> is unnecessarily complex with the NAND ECC clock -> NAND clock layout.
> 
> On the other side, Emilio has just pointed out (in private) that
> there's a flag "CLK_SET_RATE_PARENT" that is meant for these cases.
> 
> Anyway, I feel inclined to your suggestion and just forget about the
> NAND ECC clock for good, which is only confusing.

Yes, I also believe it's the easiest solution.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] clk: mvebu: Add core-divider clock

[Andrew Lunn]

Hi Ezequiel

> +static int clk_corediv_enable(struct clk_hw *hwclk)
> +{
> +	struct clk_corediv *corediv = to_corediv_clk(hwclk);
> +	struct clk_corediv_desc *desc = &corediv->desc;
> +	u32 reg;
> +
> +	reg = readl(corediv->reg);
> +	reg |= (BIT(desc->fieldbit) << CORE_CLOCK_DIVIDER_ENABLE_OFFSET);
> +	writel(reg, corediv->reg);
> +	return 0;
> +}

Shouldn't there be spinlocks around these register accesses? At least
the core gate clk driver has a spinlock.

> +static long clk_corediv_round_rate(struct clk_hw *hwclk, unsigned long rate,
> +	     				          unsigned long *parent_rate)
> +{
> +	/* Valid ratio are 1:4, 1:5, 1:6 and 1:8 */
> +	u32 div;
> +
> +	div = *parent_rate / rate;
> +	if (div <= 4)
> +	   div = 4;
> +	   else if (div <= 5)
> +	   	div = 5;
> +		else if (div <= 6)
> +		     div = 6;
> +		     else
> +			div = 8;
> +
> +	return *parent_rate / div;
> +}

This looks odd. Is not the following clearer?

	div = *parent_rate / rate;
	if (div < 5)
	   div = 4;
	   else if (div > 6)
		div = 8;

The CodingStyle might require some {} here?

+   /*
+    * Wait for clocks to settle down, and then clear all the
+     * ratios request and the reload request.
+      */
+      udelay(1000);
+      reg &= ~(CORE_CLOCK_DIVIDER_RATIO_MASK | CORE_CLOCK_DIVIDER_RATIO_RELOAD);
+      writel(reg, corediv->reg);
+      udelay(1000);


Documentation/timers/timers-howto.txt says: 

       SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
       		* Use usleep_range

	Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] clk: mvebu: Add core-divider clock

[Ezequiel Garcia]

On Thu, Sep 26, 2013 at 10:24:04AM +0200, Andrew Lunn wrote:
> Hi Ezequiel
> 
> > +static int clk_corediv_enable(struct clk_hw *hwclk)
> > +{
> > +	struct clk_corediv *corediv = to_corediv_clk(hwclk);
> > +	struct clk_corediv_desc *desc = &corediv->desc;
> > +	u32 reg;
> > +
> > +	reg = readl(corediv->reg);
> > +	reg |= (BIT(desc->fieldbit) << CORE_CLOCK_DIVIDER_ENABLE_OFFSET);
> > +	writel(reg, corediv->reg);
> > +	return 0;
> > +}
> 
> Shouldn't there be spinlocks around these register accesses? At least
> the core gate clk driver has a spinlock.
> 

Indeed.

> > +static long clk_corediv_round_rate(struct clk_hw *hwclk, unsigned long rate,
> > +	     				          unsigned long *parent_rate)
> > +{
> > +	/* Valid ratio are 1:4, 1:5, 1:6 and 1:8 */
> > +	u32 div;
> > +
> > +	div = *parent_rate / rate;
> > +	if (div <= 4)
> > +	   div = 4;
> > +	   else if (div <= 5)
> > +	   	div = 5;
> > +		else if (div <= 6)
> > +		     div = 6;
> > +		     else
> > +			div = 8;
> > +
> > +	return *parent_rate / div;
> > +}
> 
> This looks odd. Is not the following clearer?
> 
> 	div = *parent_rate / rate;
> 	if (div < 5)
> 	   div = 4;
> 	   else if (div > 6)
> 		div = 8;
> 
> The CodingStyle might require some {} here?
> 

Mmmm... no, it's not at all clearer to me.
IMHO, the original construction explicitly show the possible ratios:

	/* If it's smaller than or equal to 4, set to 4 */
	if (div <= 4)
		div = 4;

	/* Otherwise, if it's between 4 and 5, set to 5 */
	else if (div <= 5)
	 	div = 5;

	/* Otherwise, if it's between 5 and 6, set to 6 */
	else if (div <= 6)
		div = 6;

	/* Otherwise, if it's bigger than 6, set to 8 */
	else
		div = 8;

(And I don't think we need any braces).

Is this not clear?

> +   /*
> +    * Wait for clocks to settle down, and then clear all the
> +     * ratios request and the reload request.
> +      */
> +      udelay(1000);
> +      reg &= ~(CORE_CLOCK_DIVIDER_RATIO_MASK | CORE_CLOCK_DIVIDER_RATIO_RELOAD);
> +      writel(reg, corediv->reg);
> +      udelay(1000);
> 
> 
> Documentation/timers/timers-howto.txt says: 
> 
>        SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
>        		* Use usleep_range
> 

Right, forgot about that as well...

Thanks for the feedback!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] clk: mvebu: Add core-divider clock

[Andrew Lunn]

> > > +	/* Valid ratio are 1:4, 1:5, 1:6 and 1:8 */
> > > +	u32 div;
> > > +
> > > +	div = *parent_rate / rate;
> > > +	if (div <= 4)
> > > +	   div = 4;
> > > +	   else if (div <= 5)
> > > +	   	div = 5;
> > > +		else if (div <= 6)
> > > +		     div = 6;
> > > +		     else
> > > +			div = 8;
> > > +
> > > +	return *parent_rate / div;
> > > +}
> > 
> > This looks odd. Is not the following clearer?
> > 
> > 	div = *parent_rate / rate;
> > 	if (div < 5)
> > 	   div = 4;
> > 	   else if (div > 6)
> > 		div = 8;
> > 
> 
> Mmmm... no, it's not at all clearer to me.
> IMHO, the original construction explicitly show the possible ratios:
> 
> 	/* If it's smaller than or equal to 4, set to 4 */
> 	if (div <= 4)
> 		div = 4;
> 
> 	/* Otherwise, if it's between 4 and 5, set to 5 */
> 	else if (div <= 5)
> 	 	div = 5;

If div was a float or double, i would probably agree. But its a u32.
It cannot be between 4 and 5. It must be 5. So it becomes

 	if (div <= 4)
 		div = 4;
        else if (div == 5)
	        div = 5;
        else if (div == 6)
                div = 6
        else 
	    div = 8

Those two middle statements look odd to me...

      Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] clk: mvebu: Add core-divider clock

[Ezequiel Garcia]

Hey Andrew,

On Thu, Sep 26, 2013 at 10:24:04AM +0200, Andrew Lunn wrote:
> 
> +   /*
> +    * Wait for clocks to settle down, and then clear all the
> +     * ratios request and the reload request.
> +      */
> +      udelay(1000);
> +      reg &= ~(CORE_CLOCK_DIVIDER_RATIO_MASK | CORE_CLOCK_DIVIDER_RATIO_RELOAD);
> +      writel(reg, corediv->reg);
> +      udelay(1000);
> 
> 
> Documentation/timers/timers-howto.txt says: 
> 
>        SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
>        		* Use usleep_range
> 

Given the set_rate should be protected by spinlocks (as you pointed out),
I guess we can't afford using usleep_range (the context is now non-atomic).

Now, according to arch/arm/include/asm/delay.h this is the suggestion
for udelay and friends:

 * Use only for very small delays ( < 2 msec).  Should probably use a
 * lookup table, really, as the multiplications take much too long with
 * short delays.  This is a "reasonable" implementation, though (and
 * the first constant multiplications gets optimized away if the delay is
 * a constant)  

Which means we could just use udelay(1000) without much trouble.

On the other side, since we know the rate the clock is running, we could
use __delay() just as drivers/clk/sunxi/clk-factors.c is doing.
__delay() has a much simpler implementation, which feels correct in
this case of "wait until the clock settles".

Therefore, I'm currently going to try that option.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] ARM: mvebu: Add Core Divider clock device-tree binding

[Andrew Lunn]

Hi Ezequiel

+#define CORE_CLOCK_DIVIDER_RATIO_OFFSET		0x8
...

reg = readl(corediv->reg + CORE_CLOCK_DIVIDER_RATIO_OFFSET);

...

+Example:
+
+corediv_clk: corediv-clocks@18740 {
+	      compatible = "marvell,armada-370-corediv-clock";
+	      reg = <0x18740 0x8>;
+	      #clock-cells = <1>;
+	      clocks = <&pll>;
+};

The reg property seems to be wrong.

    Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] ARM: mvebu: Add Core Divider clock device-tree binding

[Ezequiel Garcia]

On Thu, Sep 26, 2013 at 10:36:55AM +0200, Andrew Lunn wrote:
> Hi Ezequiel
> 
> +#define CORE_CLOCK_DIVIDER_RATIO_OFFSET		0x8
> ...
> 
> reg = readl(corediv->reg + CORE_CLOCK_DIVIDER_RATIO_OFFSET);
> 
> ...
> 
> +Example:
> +
> +corediv_clk: corediv-clocks@18740 {
> +	      compatible = "marvell,armada-370-corediv-clock";
> +	      reg = <0x18740 0x8>;
> +	      #clock-cells = <1>;
> +	      clocks = <&pll>;
> +};
> 
> The reg property seems to be wrong.
> 

Mm.. good catch!

Thanks for the review,
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html