diff mbox

[v6,09/11] clk: mmp: parse clock from dts

Message ID 1374833133-21119-10-git-send-email-haojian.zhuang@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Haojian Zhuang July 26, 2013, 10:05 a.m. UTC
Parse clock information from DTS file for mach-mmp.

Changelog:
v6:
1. Remove marvell string from properties in clock driver.
2. Append document for clock binding device tree.
3. Use apbc to replace apbcp. Since the main difference is register
base.

v5:
1. Replace clk_register_mux() by clk_register_mux_table().

Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
---
 .../devicetree/bindings/clock/mmp-clock.txt        |  51 ++++
 arch/arm/mach-mmp/mmp-dt.c                         |   2 +
 drivers/clk/mmp/Makefile                           |   2 +-
 drivers/clk/mmp/clk-mmp.c                          | 300 +++++++++++++++++++++
 4 files changed, 354 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/mmp-clock.txt
 create mode 100644 drivers/clk/mmp/clk-mmp.c

Comments

Mark Rutland Aug. 9, 2013, 4:04 p.m. UTC | #1
On Fri, Jul 26, 2013 at 11:05:31AM +0100, Haojian Zhuang wrote:
> Parse clock information from DTS file for mach-mmp.
>
> Changelog:
> v6:
> 1. Remove marvell string from properties in clock driver.
> 2. Append document for clock binding device tree.
> 3. Use apbc to replace apbcp. Since the main difference is register
> base.
>
> v5:
> 1. Replace clk_register_mux() by clk_register_mux_table().
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> ---
>  .../devicetree/bindings/clock/mmp-clock.txt        |  51 ++++
>  arch/arm/mach-mmp/mmp-dt.c                         |   2 +
>  drivers/clk/mmp/Makefile                           |   2 +-
>  drivers/clk/mmp/clk-mmp.c                          | 300 +++++++++++++++++++++
>  4 files changed, 354 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/clock/mmp-clock.txt
>  create mode 100644 drivers/clk/mmp/clk-mmp.c
>
> diff --git a/Documentation/devicetree/bindings/clock/mmp-clock.txt b/Documentation/devicetree/bindings/clock/mmp-clock.txt
> new file mode 100644
> index 0000000..5fe52a2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/mmp-clock.txt
> @@ -0,0 +1,51 @@
> +Device Tree Clock bindings for arch-mmp
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +

Are these used somewhere already?

> +Required properties for fixed rate clocks:
> +  - compatible : should be "marvell,mmp-fixed-clkrate".
> +  - clocks : should be the input parent clock phandle for the clock. This
> +       should be the reference clock.
> +  - clock-output-names : should be reference name.
> +  - #clock-cells : from common clock binding; should be set to 0.
> +  - clock-frequency : frequency of the fixed rate clock

Why is "fixed-clock" not good enough?

This doesn't seem to describe anything more, and there's nothing
additional in the driver...

> +
> +Required properties for fixed factor clocks:
> +  - compatible : should be "marvell,mmp-fixed-clkfactor".
> +  - clocks : should be the input parent clock phandle for the clock. This
> +       should be the reference clock.
> +  - clock-output-names : should be reference name.
> +  - #clock-cells : from common clock binding; should be set to 0.

The code seems to use an additional property not described here
(mmp-fixed-factor). What does that represent?

Why not "fixed-factor-clock"?

> +
> +Required properties for mux clocks:
> +  - compatible : should be "marvell,mmp-clkmux".
> +  - clocks : should be the input parent clock phandle for the clock. This
> +       should be the reference clock.
> +  - clock-output-names : should be reference name.
> +  - #clock-cells : from common clock binding; should be set to 0.
> +  - mmp-clk-reg : mux register offset & mask bits.

Offset from where? There's no reg, no container's described and there's
no example.

> +  - mmp-clk-sel : array of mux select bits
> +
> +Required properties for divider clocks:
> +  - compatible : should be "marvell,mmp-apmu-clkdiv".
> +  - clocks : should be the input parent clock phandle for the clock. This
> +       should be the reference clock.
> +  - clock-output-names : should be reference name.
> +  - #clock-cells : from common clock binding; should be set to 0.
> +  - mmp-clk-reg : divider register offset & mask bits.

Similar comments to those above. This is also a fixed-factor-clock, no?

> +
> +Required properties for gate clocks:
> +  - compatible : should be "marvell,mmp-apbc-clk".
> +  - clocks : should be the input parent clock phandle for the clock. This
> +       should be the reference clock.
> +  - clock-output-names : should be reference name.
> +  - #clock-cells : from common clock binding; should be set to 0.
> +  - mmp-clk-reg : gate register offset & mask bits.

Offset from where?

> +
> +Optional properties for gate clocks:
> +  - mmp-clk-delay : delay between enabling function clock and apb clock

Units?

> +  - mmp-apbc-power-ctrl : enable apbc power control bit

Is this a boolean or the offset of the bit offset?

> +
> +For example:

Missing example?

[...]

> +static const struct of_device_id mmp_of_match[] = {
> +       { .compatible = "marvell,mmp-mpmu", .data = (void *)MMP_MPMU, },
> +       { .compatible = "marvell,mmp-apmu", .data = (void *)MMP_APMU, },
> +       { .compatible = "marvell,mmp-apbc", .data = (void *)MMP_APBC, },
> +       { .compatible = "marvell,mmp-apbcp", .data = (void *)MMP_APBCP, },
> +       { .compatible = "mrvl,apb-bus", .data = (void *)MMP_APB, },
> +};

Not documented?

Thanks,
Mark.
Tomasz Figa Aug. 10, 2013, 11:06 a.m. UTC | #2
Hi Mark, Haojian,

On Friday 09 of August 2013 17:04:01 Mark Rutland wrote:
> On Fri, Jul 26, 2013 at 11:05:31AM +0100, Haojian Zhuang wrote:
> > Parse clock information from DTS file for mach-mmp.
> > 
> > Changelog:
> > v6:
> > 1. Remove marvell string from properties in clock driver.
> > 2. Append document for clock binding device tree.
> > 3. Use apbc to replace apbcp. Since the main difference is register
> > base.
> > 
> > v5:
> > 1. Replace clk_register_mux() by clk_register_mux_table().
> > 
> > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> > ---
> > 
> >  .../devicetree/bindings/clock/mmp-clock.txt        |  51 ++++
> >  arch/arm/mach-mmp/mmp-dt.c                         |   2 +
> >  drivers/clk/mmp/Makefile                           |   2 +-
> >  drivers/clk/mmp/clk-mmp.c                          | 300
> >  +++++++++++++++++++++ 4 files changed, 354 insertions(+), 1
> >  deletion(-)
> >  create mode 100644
> >  Documentation/devicetree/bindings/clock/mmp-clock.txt create mode
> >  100644 drivers/clk/mmp/clk-mmp.c
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/mmp-clock.txt
> > b/Documentation/devicetree/bindings/clock/mmp-clock.txt new file mode
> > 100644
> > index 0000000..5fe52a2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/mmp-clock.txt
> > @@ -0,0 +1,51 @@
> > +Device Tree Clock bindings for arch-mmp
> > +
> > +This binding uses the common clock binding[1].
> > +
> > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +
> 
> Are these used somewhere already?

Hmm? Are you asking about the generic clock bindings or I didn't get your 
question right? They are supposed to be used (and they are used) whenever 
clocks are involved.

> > +Required properties for fixed rate clocks:
> > +  - compatible : should be "marvell,mmp-fixed-clkrate".
> > +  - clocks : should be the input parent clock phandle for the clock.
> > This +       should be the reference clock.
> > +  - clock-output-names : should be reference name.
> > +  - #clock-cells : from common clock binding; should be set to 0.
> > +  - clock-frequency : frequency of the fixed rate clock
> 
> Why is "fixed-clock" not good enough?
> 
> This doesn't seem to describe anything more, and there's nothing
> additional in the driver...

+1

> > +
> > +Required properties for fixed factor clocks:
> > +  - compatible : should be "marvell,mmp-fixed-clkfactor".
> > +  - clocks : should be the input parent clock phandle for the clock.
> > This +       should be the reference clock.
> > +  - clock-output-names : should be reference name.
> > +  - #clock-cells : from common clock binding; should be set to 0.
> 
> The code seems to use an additional property not described here
> (mmp-fixed-factor). What does that represent?
> 
> Why not "fixed-factor-clock"?

+1

> > +
> > +Required properties for mux clocks:
> > +  - compatible : should be "marvell,mmp-clkmux".
> > +  - clocks : should be the input parent clock phandle for the clock.
> > This +       should be the reference clock.
> > +  - clock-output-names : should be reference name.
> > +  - #clock-cells : from common clock binding; should be set to 0.
> > +  - mmp-clk-reg : mux register offset & mask bits.
> 
> Offset from where? There's no reg, no container's described and there's
> no example.

+1

> > +  - mmp-clk-sel : array of mux select bits
> > +
> > +Required properties for divider clocks:
> > +  - compatible : should be "marvell,mmp-apmu-clkdiv".
> > +  - clocks : should be the input parent clock phandle for the clock.
> > This +       should be the reference clock.
> > +  - clock-output-names : should be reference name.
> > +  - #clock-cells : from common clock binding; should be set to 0.
> > +  - mmp-clk-reg : divider register offset & mask bits.
> 
> Similar comments to those above. This is also a fixed-factor-clock, no?

This looks like a clock with configurable divisor for me, but again, why 
not use the standard divider clock here?

Best regards,
Tomasz

> > +
> > +Required properties for gate clocks:
> > +  - compatible : should be "marvell,mmp-apbc-clk".
> > +  - clocks : should be the input parent clock phandle for the clock.
> > This +       should be the reference clock.
> > +  - clock-output-names : should be reference name.
> > +  - #clock-cells : from common clock binding; should be set to 0.
> > +  - mmp-clk-reg : gate register offset & mask bits.
> 
> Offset from where?
> 
> > +
> > +Optional properties for gate clocks:
> > +  - mmp-clk-delay : delay between enabling function clock and apb
> > clock
> Units?
> 
> > +  - mmp-apbc-power-ctrl : enable apbc power control bit
> 
> Is this a boolean or the offset of the bit offset?
> 
> > +
> 
> > +For example:
> Missing example?
> 
> [...]
> 
> > +static const struct of_device_id mmp_of_match[] = {
> > +       { .compatible = "marvell,mmp-mpmu", .data = (void *)MMP_MPMU,
> > }, +       { .compatible = "marvell,mmp-apmu", .data = (void
> > *)MMP_APMU, }, +       { .compatible = "marvell,mmp-apbc", .data =
> > (void *)MMP_APBC, }, +       { .compatible = "marvell,mmp-apbcp",
> > .data = (void *)MMP_APBCP, }, +       { .compatible = "mrvl,apb-bus",
> > .data = (void *)MMP_APB, }, +};
> 
> Not documented?
> 
> Thanks,
> Mark.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Mark Rutland Aug. 10, 2013, 12:31 p.m. UTC | #3
On Sat, Aug 10, 2013 at 12:06:01PM +0100, Tomasz Figa wrote:
> Hi Mark, Haojian,
> 
> On Friday 09 of August 2013 17:04:01 Mark Rutland wrote:
> > On Fri, Jul 26, 2013 at 11:05:31AM +0100, Haojian Zhuang wrote:
> > > Parse clock information from DTS file for mach-mmp.
> > > 
> > > Changelog:
> > > v6:
> > > 1. Remove marvell string from properties in clock driver.
> > > 2. Append document for clock binding device tree.
> > > 3. Use apbc to replace apbcp. Since the main difference is register
> > > base.
> > > 
> > > v5:
> > > 1. Replace clk_register_mux() by clk_register_mux_table().
> > > 
> > > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> > > ---
> > > 
> > >  .../devicetree/bindings/clock/mmp-clock.txt        |  51 ++++
> > >  arch/arm/mach-mmp/mmp-dt.c                         |   2 +
> > >  drivers/clk/mmp/Makefile                           |   2 +-
> > >  drivers/clk/mmp/clk-mmp.c                          | 300
> > >  +++++++++++++++++++++ 4 files changed, 354 insertions(+), 1
> > >  deletion(-)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/clock/mmp-clock.txt create mode
> > >  100644 drivers/clk/mmp/clk-mmp.c
> > > 
> > > diff --git a/Documentation/devicetree/bindings/clock/mmp-clock.txt
> > > b/Documentation/devicetree/bindings/clock/mmp-clock.txt new file mode
> > > 100644
> > > index 0000000..5fe52a2
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/mmp-clock.txt
> > > @@ -0,0 +1,51 @@
> > > +Device Tree Clock bindings for arch-mmp
> > > +
> > > +This binding uses the common clock binding[1].
> > > +
> > > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > +
> > 
> > Are these used somewhere already?
> 
> Hmm? Are you asking about the generic clock bindings or I didn't get your 
> question right? They are supposed to be used (and they are used) whenever 
> clocks are involved.

Sorry, I meant are there any dts out there using these specific
bindings? I wasn't sure how far upstream the series was, and wanted to
make sure this hadn't gone upstream before I managed to reply.

Thanks,
Mark
Tomasz Figa Aug. 10, 2013, 12:34 p.m. UTC | #4
On Saturday 10 of August 2013 13:31:23 Mark Rutland wrote:
> On Sat, Aug 10, 2013 at 12:06:01PM +0100, Tomasz Figa wrote:
> > Hi Mark, Haojian,
> > 
> > On Friday 09 of August 2013 17:04:01 Mark Rutland wrote:
> > > On Fri, Jul 26, 2013 at 11:05:31AM +0100, Haojian Zhuang wrote:
> > > > Parse clock information from DTS file for mach-mmp.
> > > > 
> > > > Changelog:
> > > > v6:
> > > > 1. Remove marvell string from properties in clock driver.
> > > > 2. Append document for clock binding device tree.
> > > > 3. Use apbc to replace apbcp. Since the main difference is
> > > > register
> > > > base.
> > > > 
> > > > v5:
> > > > 1. Replace clk_register_mux() by clk_register_mux_table().
> > > > 
> > > > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
> > > > ---
> > > > 
> > > >  .../devicetree/bindings/clock/mmp-clock.txt        |  51 ++++
> > > >  arch/arm/mach-mmp/mmp-dt.c                         |   2 +
> > > >  drivers/clk/mmp/Makefile                           |   2 +-
> > > >  drivers/clk/mmp/clk-mmp.c                          | 300
> > > >  +++++++++++++++++++++ 4 files changed, 354 insertions(+), 1
> > > >  deletion(-)
> > > >  create mode 100644
> > > >  Documentation/devicetree/bindings/clock/mmp-clock.txt create mode
> > > >  100644 drivers/clk/mmp/clk-mmp.c
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/clock/mmp-clock.txt
> > > > b/Documentation/devicetree/bindings/clock/mmp-clock.txt new file
> > > > mode
> > > > 100644
> > > > index 0000000..5fe52a2
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/mmp-clock.txt
> > > > @@ -0,0 +1,51 @@
> > > > +Device Tree Clock bindings for arch-mmp
> > > > +
> > > > +This binding uses the common clock binding[1].
> > > > +
> > > > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > > > +
> > > 
> > > Are these used somewhere already?
> > 
> > Hmm? Are you asking about the generic clock bindings or I didn't get
> > your question right? They are supposed to be used (and they are used)
> > whenever clocks are involved.
> 
> Sorry, I meant are there any dts out there using these specific
> bindings? I wasn't sure how far upstream the series was, and wanted to
> make sure this hadn't gone upstream before I managed to reply.

OK, sorry for the noise then.

Best regards,
Tomasz
Daniel Drake Aug. 10, 2013, 2:57 p.m. UTC | #5
Hi Haojian,

I just stumbed upon this mail... I see that I have done some similar
work in my recent patches. Maybe you could CC me in future when we are
duplicating efforts.

On Fri, Jul 26, 2013 at 4:05 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> Parse clock information from DTS file for mach-mmp.
>
> Changelog:
> v6:
> 1. Remove marvell string from properties in clock driver.
> 2. Append document for clock binding device tree.
> 3. Use apbc to replace apbcp. Since the main difference is register
> base.

If one replaces the other then why are both still present in the patch?
I think all these clock type acronyms deserve a quick comment
explaining what they are.

> +Required properties for divider clocks:
> +  - compatible : should be "marvell,mmp-apmu-clkdiv".
> +  - clocks : should be the input parent clock phandle for the clock. This
> +       should be the reference clock.
> +  - clock-output-names : should be reference name.
> +  - #clock-cells : from common clock binding; should be set to 0.
> +  - mmp-clk-reg : divider register offset & mask bits.

I think you should use the standard 'reg' property for providing
register offets, and then other properties (with better names) for
other info that needs to be supplied e.g. mask.

I see that your implementation here moves away from using the existing
code in clk-apmu.c and instead just uses clk-divider directly. That
might make sense, but your new DTS does not reimplement many of the
static APMU clocks that were previously implemented (e.g. ccic, sdh in
clk-mmp2.c). Why is this?

> +Required properties for gate clocks:
> +  - compatible : should be "marvell,mmp-apbc-clk".
> +  - clocks : should be the input parent clock phandle for the clock. This
> +       should be the reference clock.
> +  - clock-output-names : should be reference name.
> +  - #clock-cells : from common clock binding; should be set to 0.
> +  - mmp-clk-reg : gate register offset & mask bits.

mmp-clk-reg contains two elements, I guess. But the second element is
not used in your driver for mmp-apbc-clk.

Thanks
Daniel
Haojian Zhuang Aug. 11, 2013, 5:22 a.m. UTC | #6
On Sat, Aug 10, 2013 at 10:57 PM, Daniel Drake <dsd@laptop.org> wrote:
> Hi Haojian,
>
> I just stumbed upon this mail... I see that I have done some similar
> work in my recent patches. Maybe you could CC me in future when we are
> duplicating efforts.

Yes, I'll cc you for the next patches.

>
> On Fri, Jul 26, 2013 at 4:05 AM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
>> Parse clock information from DTS file for mach-mmp.
>>
>> Changelog:
>> v6:
>> 1. Remove marvell string from properties in clock driver.
>> 2. Append document for clock binding device tree.
>> 3. Use apbc to replace apbcp. Since the main difference is register
>> base.
>
> If one replaces the other then why are both still present in the patch?
> I think all these clock type acronyms deserve a quick comment
> explaining what they are.

Maybe I didn't explain it well. I still use the apbcp node, but I replace all
apbcp properties by apbc properties.

The apbc registers are similar to apbcp registers. But the base registers
are different. So I use different apbc & apbcp node.

>
>> +Required properties for divider clocks:
>> +  - compatible : should be "marvell,mmp-apmu-clkdiv".
>> +  - clocks : should be the input parent clock phandle for the clock. This
>> +       should be the reference clock.
>> +  - clock-output-names : should be reference name.
>> +  - #clock-cells : from common clock binding; should be set to 0.
>> +  - mmp-clk-reg : divider register offset & mask bits.
>
> I think you should use the standard 'reg' property for providing
> register offets, and then other properties (with better names) for
> other info that needs to be supplied e.g. mask.

It seems that reg property is better. I'll change it.

>
> I see that your implementation here moves away from using the existing
> code in clk-apmu.c and instead just uses clk-divider directly. That
> might make sense, but your new DTS does not reimplement many of the
> static APMU clocks that were previously implemented (e.g. ccic, sdh in
> clk-mmp2.c). Why is this?
>
I'll append them later.

>> +Required properties for gate clocks:
>> +  - compatible : should be "marvell,mmp-apbc-clk".
>> +  - clocks : should be the input parent clock phandle for the clock. This
>> +       should be the reference clock.
>> +  - clock-output-names : should be reference name.
>> +  - #clock-cells : from common clock binding; should be set to 0.
>> +  - mmp-clk-reg : gate register offset & mask bits.
>
> mmp-clk-reg contains two elements, I guess. But the second element is
> not used in your driver for mmp-apbc-clk.

Thanks. I'll fix it.

>
> Thanks
> Daniel
Daniel Drake Aug. 14, 2013, 9:25 p.m. UTC | #7
On Sat, Aug 10, 2013 at 11:22 PM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
>> I see that your implementation here moves away from using the existing
>> code in clk-apmu.c and instead just uses clk-divider directly. That
>> might make sense, but your new DTS does not reimplement many of the
>> static APMU clocks that were previously implemented (e.g. ccic, sdh in
>> clk-mmp2.c). Why is this?
>>
> I'll append them later.

Looking closer I see that you only implemented this for pxa910 - and
(as noted above) you only reimplemented a subset of the clocks.

The original static clock code is still in place and being called
(pxa910_init calls pxa910_clk_init). This will result in some of the
clocks being duplicated (defined once in DT and again in static code).
I think you should either add a mechanism to avoid doing both (e.g.
see the end of http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/177153.html)
or simply remove support for non-DT clocks (depending on user base,
not sure if this is possible).

I think adding support for all the clocks in DT now is quite important
as I suspect it will change your DT design. Your patch treats APMU
clocks always as simple dividers or muxes, always enabled, however I
don't think it is possible to represent the SDH/display APMU clocks in
this way, at least I have not found a way to do it. These clocks are
in reset by default and certain bits must be written to enable them.

Finally there is something confusing in mmp_apbc_setup()

+       if (of_property_read_string(np, "clock-names", &clk_name))
+               clkdev = 1;
+       else {
+               of_property_read_string(np, "clock-output-names", &clk_name);
+               clkdev = 0;
+       }
+       if (clkdev)
+               clk_register_clkdev(clk, clk_name, NULL);

Can you explain what this logic is trying to do?
I think it is wrong. If I provide the name for the clock I want to
output from my provider, I do not get registered? And in order for me
to get my clock provider registered, I have to provide a clock-names
property which is then used to name the device?? But clock-names is
usually used for the input clock name. And I don't see why providing a
name is necessary, it could just use node->name.

Thanks
Daniel
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/clock/mmp-clock.txt b/Documentation/devicetree/bindings/clock/mmp-clock.txt
new file mode 100644
index 0000000..5fe52a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/mmp-clock.txt
@@ -0,0 +1,51 @@ 
+Device Tree Clock bindings for arch-mmp
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties for fixed rate clocks:
+  - compatible : should be "marvell,mmp-fixed-clkrate".
+  - clocks : should be the input parent clock phandle for the clock. This
+	should be the reference clock.
+  - clock-output-names : should be reference name.
+  - #clock-cells : from common clock binding; should be set to 0.
+  - clock-frequency : frequency of the fixed rate clock
+
+Required properties for fixed factor clocks:
+  - compatible : should be "marvell,mmp-fixed-clkfactor".
+  - clocks : should be the input parent clock phandle for the clock. This
+	should be the reference clock.
+  - clock-output-names : should be reference name.
+  - #clock-cells : from common clock binding; should be set to 0.
+
+Required properties for mux clocks:
+  - compatible : should be "marvell,mmp-clkmux".
+  - clocks : should be the input parent clock phandle for the clock. This
+	should be the reference clock.
+  - clock-output-names : should be reference name.
+  - #clock-cells : from common clock binding; should be set to 0.
+  - mmp-clk-reg : mux register offset & mask bits.
+  - mmp-clk-sel : array of mux select bits
+
+Required properties for divider clocks:
+  - compatible : should be "marvell,mmp-apmu-clkdiv".
+  - clocks : should be the input parent clock phandle for the clock. This
+	should be the reference clock.
+  - clock-output-names : should be reference name.
+  - #clock-cells : from common clock binding; should be set to 0.
+  - mmp-clk-reg : divider register offset & mask bits.
+
+Required properties for gate clocks:
+  - compatible : should be "marvell,mmp-apbc-clk".
+  - clocks : should be the input parent clock phandle for the clock. This
+	should be the reference clock.
+  - clock-output-names : should be reference name.
+  - #clock-cells : from common clock binding; should be set to 0.
+  - mmp-clk-reg : gate register offset & mask bits.
+
+Optional properties for gate clocks:
+  - mmp-clk-delay : delay between enabling function clock and apb clock
+  - mmp-apbc-power-ctrl : enable apbc power control bit
+
+For example:
diff --git a/arch/arm/mach-mmp/mmp-dt.c b/arch/arm/mach-mmp/mmp-dt.c
index cca529c..f109bf6 100644
--- a/arch/arm/mach-mmp/mmp-dt.c
+++ b/arch/arm/mach-mmp/mmp-dt.c
@@ -9,6 +9,7 @@ 
  *  publishhed by the Free Software Foundation.
  */
 
+#include <linux/clk-provider.h>
 #include <linux/irqchip.h>
 #include <linux/of_platform.h>
 #include <asm/mach/arch.h>
@@ -48,6 +49,7 @@  static void __init pxa168_dt_init(void)
 
 static void __init pxa910_dt_init(void)
 {
+	of_clk_init(NULL);
 	of_platform_populate(NULL, of_default_bus_match_table,
 			     pxa910_auxdata_lookup, NULL);
 }
diff --git a/drivers/clk/mmp/Makefile b/drivers/clk/mmp/Makefile
index 392d780..83182e5 100644
--- a/drivers/clk/mmp/Makefile
+++ b/drivers/clk/mmp/Makefile
@@ -2,7 +2,7 @@ 
 # Makefile for mmp specific clk
 #
 
-obj-y += clk-apbc.o clk-apmu.o clk-frac.o
+obj-y += clk-apbc.o clk-apmu.o clk-frac.o clk-mmp.o
 
 obj-$(CONFIG_CPU_PXA168) += clk-pxa168.o
 obj-$(CONFIG_CPU_PXA910) += clk-pxa910.o
diff --git a/drivers/clk/mmp/clk-mmp.c b/drivers/clk/mmp/clk-mmp.c
new file mode 100644
index 0000000..50c16a0
--- /dev/null
+++ b/drivers/clk/mmp/clk-mmp.c
@@ -0,0 +1,300 @@ 
+/*
+ * Marvell MMP clock driver
+ *
+ * Copyright (c) 2012-2013 Linaro Limited.
+ *
+ * Author: Haojian Zhuang <haojian.zhuang@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+#include "clk.h"
+
+enum {
+	MMP_MPMU = 0,
+	MMP_APMU,
+	MMP_APBC,
+	MMP_APBCP,
+	MMP_APB,
+	MMP_MAX,
+};
+
+static void __iomem *mmp_clk_base[MMP_MAX];
+
+static DEFINE_SPINLOCK(mmp_clk_lock);
+
+static const struct of_device_id mmp_of_match[] = {
+	{ .compatible = "marvell,mmp-mpmu", .data = (void *)MMP_MPMU, },
+	{ .compatible = "marvell,mmp-apmu", .data = (void *)MMP_APMU, },
+	{ .compatible = "marvell,mmp-apbc", .data = (void *)MMP_APBC, },
+	{ .compatible = "marvell,mmp-apbcp", .data = (void *)MMP_APBCP, },
+	{ .compatible = "mrvl,apb-bus", .data = (void *)MMP_APB, },
+};
+
+void __iomem __init *mmp_init_clocks(struct device_node *np)
+{
+	struct device_node *parent;
+	const struct of_device_id *match;
+	void __iomem *ret = NULL;
+	int i;
+
+	parent = of_get_parent(np);
+	if (!parent)
+		goto out;
+	match = of_match_node(mmp_of_match, parent);
+	if (!match)
+		goto out;
+
+	i = (unsigned int)match->data;
+	switch (i) {
+	case MMP_MPMU:
+	case MMP_APMU:
+	case MMP_APBC:
+	case MMP_APBCP:
+	case MMP_APB:
+		if (!mmp_clk_base[i]) {
+			ret = of_iomap(parent, 0);
+			WARN_ON(!ret);
+			mmp_clk_base[i] = ret;
+		} else {
+			ret = mmp_clk_base[i];
+		}
+		break;
+	default:
+		goto out;
+	}
+out:
+	return ret;
+}
+
+static void __init mmp_fixed_rate_setup(struct device_node *np)
+{
+	struct clk *clk;
+	const char *clk_name, *parent_name;
+	int rate;
+
+	if (of_property_read_u32(np, "clock-frequency", &rate))
+		return;
+
+	if (of_property_read_string(np, "clock-output-names", &clk_name))
+		return;
+
+	/* this node has only one parent */
+	parent_name = of_clk_get_parent_name(np, 0);
+	if (!parent_name)
+		return;
+
+	clk = clk_register_fixed_rate(NULL, clk_name, parent_name, 0, rate);
+	if (IS_ERR(clk))
+		return;
+	of_clk_add_provider(np, of_clk_src_simple_get, clk);
+}
+CLK_OF_DECLARE(mmp_fixed_rate, "marvell,mmp-fixed-clkrate",
+	       mmp_fixed_rate_setup);
+
+static void __init mmp_fixed_factor_setup(struct device_node *np)
+{
+	struct clk *clk;
+	const char *clk_name, *parent_name;
+	u32 data[2];
+
+	if (of_property_read_u32_array(np, "mmp-fixed-factor",
+				&data[0], 2))
+		return;
+	if (of_property_read_string(np, "clock-output-names", &clk_name))
+		return;
+
+	/* this node has only one parent */
+	parent_name = of_clk_get_parent_name(np, 0);
+	if (!parent_name)
+		return;
+
+	clk = clk_register_fixed_factor(NULL, clk_name, parent_name, 0,
+			data[0], data[1]);
+	if (IS_ERR(clk))
+		return;
+	clk_register_clkdev(clk, clk_name, NULL);
+	of_clk_add_provider(np, of_clk_src_simple_get, clk);
+}
+CLK_OF_DECLARE(mmp_fixed_factor, "marvell,mmp-fixed-clkfactor",
+	       mmp_fixed_factor_setup);
+
+static void __init mmp_apmu_div_setup(struct device_node *np)
+{
+	u32 data[2];
+	u8 shift, width;
+	void __iomem *reg, *base;
+	const char *parent_name, *clk_name;
+	struct clk *clk;
+
+	base = mmp_init_clocks(np);
+	if (!base)
+		return;
+
+	if (of_property_read_u32_array(np, "mmp-clk-reg", &data[0], 2))
+		return;
+	reg = base + data[0];
+	shift = ffs(data[1]) - 1;
+	width = fls(data[1]) - ffs(data[1]) + 1;
+
+	parent_name = of_clk_get_parent_name(np, 0);
+	if (!parent_name)
+		return;
+	if (of_property_read_string(np, "clock-output-names", &clk_name))
+		return;
+
+	clk = clk_register_divider(NULL, clk_name, parent_name,
+			CLK_SET_RATE_PARENT, reg, shift, width, 0,
+			&mmp_clk_lock);
+	if (IS_ERR(clk))
+		return;
+	of_clk_add_provider(np, of_clk_src_simple_get, clk);
+	return;
+}
+CLK_OF_DECLARE(mmp_div, "marvell,mmp-apmu-clkdiv", mmp_apmu_div_setup);
+
+static int __init mmp_parse_mux(struct device_node *np,
+				u8 *num_parents,
+				u32 *clk_sel)
+{
+	int i, cnt, ret;
+
+	/* get the count of items in mux */
+	for (i = 0, cnt = 0; ; i++, cnt++) {
+		/* parent's #clock-cells property is always 0 */
+		if (!of_parse_phandle(np, "clocks", i))
+			break;
+	}
+
+	for (i = 0; i < cnt; i++) {
+		if (!of_clk_get_parent_name(np, i))
+			break;
+	}
+	*num_parents = cnt;
+	clk_sel = kzalloc(sizeof(u32 *) * cnt, GFP_KERNEL);
+	if (!clk_sel)
+		return -ENOMEM;
+	ret = of_property_read_u32_array(np, "mmp-clk-sel", clk_sel, cnt);
+	if (ret)
+		goto err;
+	return 0;
+err:
+	kfree(clk_sel);
+	return ret;
+}
+
+static void __init mmp_mux_setup(struct device_node *np)
+{
+	u32 data[2], mask, *clk_sel = NULL, mux_flags = 0;
+	u8 shift, num_parents;
+	void __iomem *reg, *base;
+	const char **parent_names;
+	const char *clk_name;
+	struct clk *clk;
+	int i, ret;
+
+	base = mmp_init_clocks(np);
+	if (!base)
+		return;
+	if (of_property_read_string(np, "clock-output-names", &clk_name))
+		return;
+	if (of_property_read_u32_array(np, "mmp-clk-reg", &data[0], 2))
+		return;
+	ret = mmp_parse_mux(np, &num_parents, clk_sel);
+	if (ret)
+		return;
+
+	reg = base + data[0];
+	shift = ffs(data[1]) - 1;
+	mask = data[1] >> shift;
+
+	parent_names = kzalloc(sizeof(char *) * num_parents, GFP_KERNEL);
+	if (!parent_names)
+		goto err;
+	for (i = 0; i < num_parents; i++)
+		parent_names[i] = of_clk_get_parent_name(np, i);
+	clk = clk_register_mux_table(NULL, clk_name, parent_names, num_parents,
+				     CLK_SET_RATE_PARENT, reg, shift, mask,
+				     mux_flags, clk_sel, &mmp_clk_lock);
+	if (IS_ERR(clk))
+		goto err_clk;
+	of_clk_add_provider(np, of_clk_src_simple_get, clk);
+	return;
+err_clk:
+	kfree(parent_names);
+err:
+	kfree(clk_sel);
+}
+CLK_OF_DECLARE(mmp_mux, "marvell,mmp-clkmux", mmp_mux_setup);
+
+#define APBC_NO_BUS_CTRL	BIT(0)
+#define APBC_POWER_CTRL		BIT(1)
+
+static void __init mmp_apbc_setup(struct device_node *np)
+{
+	u32 data[2], delay, apbc_flags, clkdev;
+	void __iomem *reg, *base;
+	const char **parent_names;
+	const char *clk_name = NULL;
+	struct clk *clk;
+
+	base = mmp_init_clocks(np);
+	if (!base)
+		return;
+
+	if (of_property_read_string(np, "clock-names", &clk_name))
+		clkdev = 1;
+	else {
+		of_property_read_string(np, "clock-output-names", &clk_name);
+		clkdev = 0;
+	}
+
+	if (of_property_read_u32_array(np, "mmp-clk-reg", &data[0], 2))
+		return;
+	/* If mmp-clk-delay property isn't defined, set delay as 10us */
+	if (of_property_read_u32(np, "mmp-clk-delay", &delay))
+		delay = 10;
+	if (of_get_property(np, "mmp-apbc-power-ctrl", NULL))
+		apbc_flags |= APBC_POWER_CTRL;
+
+	reg = base + data[0];
+
+	/* only has the fixed parent */
+	parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
+	if (!parent_names)
+		return;
+	parent_names[0] = of_clk_get_parent_name(np, 0);
+
+	clk = mmp_clk_register_apbc(clk_name, parent_names[0],
+				    reg, delay, 0, &mmp_clk_lock);
+	if (IS_ERR(clk)) {
+		kfree(parent_names);
+		return;
+	}
+	if (clkdev)
+		clk_register_clkdev(clk, clk_name, NULL);
+	of_clk_add_provider(np, of_clk_src_simple_get, clk);
+}
+CLK_OF_DECLARE(mmp_apbc, "marvell,mmp-apbc-clk", mmp_apbc_setup);