[v4] clk/axs10x: Add I2S PLL clock driver
diff mbox

Message ID 50c75be8ecab225a1dd49628a173d211a02755b2.1459791946.git.joabreu@synopsys.com
State New
Headers show

Commit Message

Jose Abreu April 4, 2016, 6 p.m. UTC
The ARC SDP I2S clock can be programmed using a
specific PLL.

This patch has the goal of adding a clock driver
that programs this PLL.

At this moment the rate values are hardcoded in
a table but in the future it would be ideal to
use a function which determines the PLL values
given the desired rate.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---

Changes v3 -> v4:
* Added binding document (as suggested by Stephen Boyd)
* Minor code style fixes (as suggested by Stephen Boyd)
* Use ioremap (as suggested by Stephen Boyd)
* Implement round_rate (as suggested by Stephen Boyd)
* Change to platform driver (as suggested by Stephen Boyd)
* Use {readl/writel}_relaxed (as suggested by Vineet Gupta)

Changes v2 -> v3:
* Implemented recalc_rate

Changes v1 -> v2:
* Renamed folder to axs10x (as suggested by Alexey Brodkin)
* Added more supported rates

 .../devicetree/bindings/clock/i2s-pll-clock.txt    |  17 ++
 drivers/clk/Makefile                               |   1 +
 drivers/clk/axs10x/Makefile                        |   1 +
 drivers/clk/axs10x/i2s_pll_clock.c                 | 217 +++++++++++++++++++++
 4 files changed, 236 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/i2s-pll-clock.txt
 create mode 100644 drivers/clk/axs10x/Makefile
 create mode 100644 drivers/clk/axs10x/i2s_pll_clock.c

Comments

Alexey Brodkin April 11, 2016, 4:47 p.m. UTC | #1
Hi Jose,

On Mon, 2016-04-11 at 11:41 +0100, Jose Abreu wrote:
> The ARC SDP I2S clock can be programmed using a
> specific PLL.
> 
> This patch has the goal of adding a clock driver
> that programs this PLL.
> 
> At this moment the rate values are hardcoded in
> a table but in the future it would be ideal to
> use a function which determines the PLL values
> given the desired rate.
> 
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> ---
> 
> Changes v3 -> v4:
> * Added binding document (as suggested by Stephen Boyd)
> * Minor code style fixes (as suggested by Stephen Boyd)
> * Use ioremap (as suggested by Stephen Boyd)
> * Implement round_rate (as suggested by Stephen Boyd)
> * Change to platform driver (as suggested by Stephen Boyd)
> * Use {readl/writel}_relaxed (as suggested by Vineet Gupta)
> 
> Changes v2 -> v3:
> * Implemented recalc_rate
> 
> Changes v1 -> v2:
> * Renamed folder to axs10x (as suggested by Alexey Brodkin)
> * Added more supported rates

[snip]

> diff --git a/Documentation/devicetree/bindings/clock/i2s-pll-clock.txt b/Documentation/devicetree/bindings/clock/i2s-
> pll-clock.txt
> new file mode 100644
> index 0000000..ff86a41
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/i2s-pll-clock.txt
> @@ -0,0 +1,17 @@
> +Binding for the AXS10X I2S PLL clock
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible: shall be "snps,i2s-pll-clock"
> +- #clock-cells: from common clock binding; Should always be set to 0.
> +- reg : Address and length of the I2S PLL register set.
> +
> +Example:
> +	clock@0x100a0 {

Please remove "0x" from node name.

> +		compatible = "snps,i2s-pll-clock";
> +		reg = <0x100a0 0x10>;
> +		#clock-cells = <0>;
> +	};
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 46869d6..2ca62dc6 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -84,3 +84,4 @@ obj-$(CONFIG_X86)			+= x86/
>  obj-$(CONFIG_ARCH_ZX)			+= zte/
>  obj-$(CONFIG_ARCH_ZYNQ)			+= zynq/
>  obj-$(CONFIG_H8300)		+= h8300/
> +obj-$(CONFIG_ARC_PLAT_AXS10X)		+= axs10x/
> diff --git a/drivers/clk/axs10x/Makefile b/drivers/clk/axs10x/Makefile
> new file mode 100644
> index 0000000..01996b8
> --- /dev/null
> +++ b/drivers/clk/axs10x/Makefile
> @@ -0,0 +1 @@
> +obj-y += i2s_pll_clock.o
> diff --git a/drivers/clk/axs10x/i2s_pll_clock.c b/drivers/clk/axs10x/i2s_pll_clock.c
> new file mode 100644
> index 0000000..3ba4e2f
> --- /dev/null
> +++ b/drivers/clk/axs10x/i2s_pll_clock.c
> @@ -0,0 +1,217 @@
> +/*
> + * Synopsys AXS10X SDP I2S PLL clock driver
> + *
> + * Copyright (C) 2016 Synopsys
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/device.h>

"linux/platform_device.h" includes "linux/device.h" so you may make this list of headers
a little bit shorter.

> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>

"linux/of_address.h" already includes "linux/of.h".

[snip]

> +
> +static const struct of_device_id i2s_pll_clk_id[] = {
> +	{ .compatible = "snps,i2s-pll-clock", },

I would think that it makes sense to add the board name in
this compatible string. So something like "snps,axs10x-i2s-pll-clock"
IMHO looks much more informative.

Also adding Rob Herring and DT mailing list in Cc.
Please make sure Rod acks your bindings and corresponding docs.

-Alexey
Stephen Boyd April 11, 2016, 10:03 p.m. UTC | #2
On 04/11, Alexey Brodkin wrote:
> On Mon, 2016-04-11 at 11:41 +0100, Jose Abreu wrote:
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/err.h>
> > +#include <linux/device.h>
> 
> "linux/platform_device.h" includes "linux/device.h" so you may make this list of headers
> a little bit shorter.
> 
> > +#include <linux/of_address.h>
> > +#include <linux/slab.h>
> > +#include <linux/of.h>
> 
> "linux/of_address.h" already includes "linux/of.h".

It's ok to include things twice. In fact, its better to avoid any
implicit includes so that if we ever want to remove includes from
other headers we can do so without disturbing this driver.
Alexey Brodkin April 15, 2016, 12:08 p.m. UTC | #3
Hi Stephen,

On Mon, 2016-04-11 at 15:03 -0700, sboyd@codeaurora.org wrote:
> On 04/11, Alexey Brodkin wrote:
> > 
> > On Mon, 2016-04-11 at 11:41 +0100, Jose Abreu wrote:
> > > 
> > > + * warranty of any kind, whether express or implied.
> > > + */
> > > +
> > > +#include <linux/platform_device.h>
> > > +#include <linux/module.h>
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/err.h>
> > > +#include <linux/device.h>
> > "linux/platform_device.h" includes "linux/device.h" so you may make this list of headers
> > a little bit shorter.
> > 
> > > 
> > > +#include <linux/of_address.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/of.h>
> > "linux/of_address.h" already includes "linux/of.h".
> It's ok to include things twice. In fact, its better to avoid any
> implicit includes so that if we ever want to remove includes from
> other headers we can do so without disturbing this driver.

IMHO approach with minimal amount of headers is nice just because
it's easier to check if everything is in place. I mean attempt to compile
will immediately reveal a missing header.

So that's what I do - I remove as many inclusions as I may until stuff compiles.

But with approach of explicit inclusion it's much easier to include
much more headers than really needed. The only way to figure out if header is really
required is to manually check all used functions in the current source
which is way too unreliable and probably nobody will do it ever anyways.
And that's how we'll get more stale and pointless inclusions.

-Alexey
Stephen Boyd April 15, 2016, 11:38 p.m. UTC | #4
On 04/15, Alexey Brodkin wrote:
> Hi Stephen,
> 
> On Mon, 2016-04-11 at 15:03 -0700, sboyd@codeaurora.org wrote:
> > On 04/11, Alexey Brodkin wrote:
> > > 
> > > On Mon, 2016-04-11 at 11:41 +0100, Jose Abreu wrote:
> > > > 
> > > > + * warranty of any kind, whether express or implied.
> > > > + */
> > > > +
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/clk-provider.h>
> > > > +#include <linux/err.h>
> > > > +#include <linux/device.h>
> > > "linux/platform_device.h" includes "linux/device.h" so you may make this list of headers
> > > a little bit shorter.
> > > 
> > > > 
> > > > +#include <linux/of_address.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/of.h>
> > > "linux/of_address.h" already includes "linux/of.h".
> > It's ok to include things twice. In fact, its better to avoid any
> > implicit includes so that if we ever want to remove includes from
> > other headers we can do so without disturbing this driver.
> 
> IMHO approach with minimal amount of headers is nice just because
> it's easier to check if everything is in place. I mean attempt to compile
> will immediately reveal a missing header.
> 
> So that's what I do - I remove as many inclusions as I may until stuff compiles.

Please don't. People try to break header includes cycles by
moving things in one header to another header, and then those
changes may need to fix random drivers because they are now
missing an implicit include they were relying on, etc.

> 
> But with approach of explicit inclusion it's much easier to include
> much more headers than really needed. The only way to figure out if header is really
> required is to manually check all used functions in the current source
> which is way too unreliable and probably nobody will do it ever anyways.
> And that's how we'll get more stale and pointless inclusions.

Sounds like a job for a script!
Stephen Boyd April 15, 2016, 11:46 p.m. UTC | #5
On 04/11, Jose Abreu wrote:
> new file mode 100644
> index 0000000..3ba4e2f
> --- /dev/null
> +++ b/drivers/clk/axs10x/i2s_pll_clock.c
> @@ -0,0 +1,217 @@
> +
> +static int i2s_pll_clk_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	const char *clk_name;
> +	struct clk *clk;
> +	struct i2s_pll_clk *pll_clk;
> +	struct clk_init_data init;
> +	struct resource *mem;
> +
> +	if (!node)
> +		return -ENODEV;

Does this ever happen? Looks like dead code.

> +
> +	pll_clk = devm_kzalloc(dev, sizeof(*pll_clk), GFP_KERNEL);
> +	if (!pll_clk)
> +		return -ENOMEM;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	pll_clk->base = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(pll_clk->base))
> +		return PTR_ERR(pll_clk->base);
> +
> +	clk_name = node->name;
> +	init.name = clk_name;
> +	init.ops = &i2s_pll_ops;
> +	init.num_parents = 0;
> +	pll_clk->hw.init = &init;
> +
> +	clk = clk_register(NULL, &pll_clk->hw);

Pass dev as first argument. Also use devm_clk_register() instead.

> +	if (IS_ERR(clk)) {
> +		dev_err(dev, "failed to register %s div clock (%ld)\n",
> +				clk_name, PTR_ERR(clk));
> +		return PTR_ERR(clk);
> +	}
> +
> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M) {

Please don't readl directly from addresses. I think I mentioned
that before and didn't get back to you when you replied asking
for other solutions. I still think a proper DT is in order
instead of doing this check for ref_clk.

> +		pll_clk->ref_clk = 27000000;
> +		pll_clk->pll_cfg = i2s_pll_cfg_27m;
> +	} else {
> +		pll_clk->ref_clk = 28224000;
> +		pll_clk->pll_cfg = i2s_pll_cfg_28m;
> +	}

We should do this before registering the clk with the framework.

> +
> +	return of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +}
> +
> +static int i2s_pll_clk_remove(struct platform_device *pdev)
> +{
> +	of_clk_del_provider(pdev->dev.of_node);
> +	return 0;
> +}
> +
> +static const struct of_device_id i2s_pll_clk_id[] = {
> +	{ .compatible = "snps,i2s-pll-clock", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, i2s_pll_clk_id);
> +
> +static struct platform_driver i2s_pll_clk_driver = {
> +	.driver = {
> +		.name = "i2s-pll-clock",
> +		.of_match_table = of_match_ptr(i2s_pll_clk_id),

You can drop of_match_ptr(), it doesn't have much use besides
introducing compilation warnings.

> +	},
> +	.probe = i2s_pll_clk_probe,
> +	.remove = i2s_pll_clk_remove,
> +};
> +module_platform_driver(i2s_pll_clk_driver);
>
Jose Abreu April 18, 2016, 10:30 a.m. UTC | #6
Hi Stephen,


On 16-04-2016 00:46, Stephen Boyd wrote:
> On 04/11, Jose Abreu wrote:
>> new file mode 100644
>> index 0000000..3ba4e2f
>> --- /dev/null
>> +++ b/drivers/clk/axs10x/i2s_pll_clock.c
>> @@ -0,0 +1,217 @@
>> +
>> +static int i2s_pll_clk_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *node = dev->of_node;
>> +	const char *clk_name;
>> +	struct clk *clk;
>> +	struct i2s_pll_clk *pll_clk;
>> +	struct clk_init_data init;
>> +	struct resource *mem;
>> +
>> +	if (!node)
>> +		return -ENODEV;
> Does this ever happen? Looks like dead code.

Will remove.

>> +
>> +	pll_clk = devm_kzalloc(dev, sizeof(*pll_clk), GFP_KERNEL);
>> +	if (!pll_clk)
>> +		return -ENOMEM;
>> +
>> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	pll_clk->base = devm_ioremap_resource(dev, mem);
>> +	if (IS_ERR(pll_clk->base))
>> +		return PTR_ERR(pll_clk->base);
>> +
>> +	clk_name = node->name;
>> +	init.name = clk_name;
>> +	init.ops = &i2s_pll_ops;
>> +	init.num_parents = 0;
>> +	pll_clk->hw.init = &init;
>> +
>> +	clk = clk_register(NULL, &pll_clk->hw);
> Pass dev as first argument. Also use devm_clk_register() instead.

Ok.

>> +	if (IS_ERR(clk)) {
>> +		dev_err(dev, "failed to register %s div clock (%ld)\n",
>> +				clk_name, PTR_ERR(clk));
>> +		return PTR_ERR(clk);
>> +	}
>> +
>> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M) {
> Please don't readl directly from addresses. I think I mentioned
> that before and didn't get back to you when you replied asking
> for other solutions. I still think a proper DT is in order
> instead of doing this check for ref_clk.

I think that the DT approach would be better but I also think that using two DT
files with only one change between them is not viable. I can see some alternatives:
    1) Pass the region of FPGA version in reg field of DT so that writel is not
directly used;
    2) Create a dummy parent clock driver that reads from FPGA version register
and returns the rate;
    3) Last resort: Use two DT files for each FPGA version.

@Vineet, @Alexey: Can you give some suggestions?

Some background:
We are expecting a new firmware release for the AXS board that will change the
reference clock value of the I2S PLL from 27MHz to 28.224MHz. Due to this change
the dividers of this PLL will change. Right now I am directly reading from the
FPGA version register but Stephen suggested to use a DT approach so that this
rate is declared as parent clock. This would be a good solution but would
require the usage of two different DT files (one for the current firmware and
another for the new firmware), which I think is not ideal. What is your opinion?
Some other solutions are listed above.

>> +		pll_clk->ref_clk = 27000000;
>> +		pll_clk->pll_cfg = i2s_pll_cfg_27m;
>> +	} else {
>> +		pll_clk->ref_clk = 28224000;
>> +		pll_clk->pll_cfg = i2s_pll_cfg_28m;
>> +	}
> We should do this before registering the clk with the framework.

Ok.

>> +
>> +	return of_clk_add_provider(node, of_clk_src_simple_get, clk);
>> +}
>> +
>> +static int i2s_pll_clk_remove(struct platform_device *pdev)
>> +{
>> +	of_clk_del_provider(pdev->dev.of_node);
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id i2s_pll_clk_id[] = {
>> +	{ .compatible = "snps,i2s-pll-clock", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, i2s_pll_clk_id);
>> +
>> +static struct platform_driver i2s_pll_clk_driver = {
>> +	.driver = {
>> +		.name = "i2s-pll-clock",
>> +		.of_match_table = of_match_ptr(i2s_pll_clk_id),
> You can drop of_match_ptr(), it doesn't have much use besides
> introducing compilation warnings.

Ok.

>> +	},
>> +	.probe = i2s_pll_clk_probe,
>> +	.remove = i2s_pll_clk_remove,
>> +};
>> +module_platform_driver(i2s_pll_clk_driver);
>>

Best regards,
Jose Miguel Abreu
Vineet Gupta April 18, 2016, 11:49 a.m. UTC | #7
On Monday 18 April 2016 04:00 PM, Jose Abreu wrote:
>>> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M) {
>> > Please don't readl directly from addresses. I think I mentioned
>> > that before and didn't get back to you when you replied asking
>> > for other solutions. I still think a proper DT is in order
>> > instead of doing this check for ref_clk.
> I think that the DT approach would be better but I also think that using two DT
> files with only one change between them is not viable. I can see some alternatives:
>     1) Pass the region of FPGA version in reg field of DT so that writel is not
> directly used;
>     2) Create a dummy parent clock driver that reads from FPGA version register
> and returns the rate;
>     3) Last resort: Use two DT files for each FPGA version.
> 
> @Vineet, @Alexey: Can you give some suggestions?
> 
> Some background:
> We are expecting a new firmware release for the AXS board that will change the
> reference clock value of the I2S PLL from 27MHz to 28.224MHz. Due to this change
> the dividers of this PLL will change. Right now I am directly reading from the
> FPGA version register but Stephen suggested to use a DT approach so that this
> rate is declared as parent clock. This would be a good solution but would
> require the usage of two different DT files (one for the current firmware and
> another for the new firmware), which I think is not ideal. What is your opinion?
> Some other solutions are listed above.

Consider this my ignorance of clk drivers, what exactly is the problem with that
readl() for FPGA ver. Having 2 versions of DT is annoyance for sure, but the
bigger headache is that it still won't help cases of users mixing and matching
boards and DT. IMO this runtime check is pretty nice and will support both types
of boards with exact same code/DT !

FWIW, both solutions #1 and #3 seem to imply a different DT - no ?

And I really don't see how #2 makes things more elegant/abstracted w.r.t clk
framework ?

So I prefer what you had before.
-Vineet
Jose Abreu April 19, 2016, 9:13 a.m. UTC | #8
Hi Vineet,


On 18-04-2016 12:49, Vineet Gupta wrote:
> On Monday 18 April 2016 04:00 PM, Jose Abreu wrote:
>>>> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M) {
>>>> Please don't readl directly from addresses. I think I mentioned
>>>> that before and didn't get back to you when you replied asking
>>>> for other solutions. I still think a proper DT is in order
>>>> instead of doing this check for ref_clk.
>> I think that the DT approach would be better but I also think that using two DT
>> files with only one change between them is not viable. I can see some alternatives:
>>     1) Pass the region of FPGA version in reg field of DT so that writel is not
>> directly used;
>>     2) Create a dummy parent clock driver that reads from FPGA version register
>> and returns the rate;
>>     3) Last resort: Use two DT files for each FPGA version.
>>
>> @Vineet, @Alexey: Can you give some suggestions?
>>
>> Some background:
>> We are expecting a new firmware release for the AXS board that will change the
>> reference clock value of the I2S PLL from 27MHz to 28.224MHz. Due to this change
>> the dividers of this PLL will change. Right now I am directly reading from the
>> FPGA version register but Stephen suggested to use a DT approach so that this
>> rate is declared as parent clock. This would be a good solution but would
>> require the usage of two different DT files (one for the current firmware and
>> another for the new firmware), which I think is not ideal. What is your opinion?
>> Some other solutions are listed above.
> Consider this my ignorance of clk drivers, what exactly is the problem with that
> readl() for FPGA ver. Having 2 versions of DT is annoyance for sure, but the
> bigger headache is that it still won't help cases of users mixing and matching
> boards and DT. IMO this runtime check is pretty nice and will support both types
> of boards with exact same code/DT !
>
> FWIW, both solutions #1 and #3 seem to imply a different DT - no ?

Solution 1 only requires that the FPGA version register is declared in the DT,
something like this:
    i2s_clock@100a0 {
        compatible = "snps,axs10x-i2s-pll-clock";
        reg = <0x100a0 0x10 0x11230 0x04>;
        #clock-cells = <0>;
    };

And then the region is io-remapped. This solution would discard the direct readl
from the address and would still be compatible with the different firmwares
using the same DT.

Solution 3 is the alternative that Stephen suggested which requires two
different DT's.

>
> And I really don't see how #2 makes things more elegant/abstracted w.r.t clk
> framework ?

Yes, solution 2 is more of a workaround and is not the best by far.

>
> So I prefer what you had before.
> -Vineet

@Stephen: can you give some input so that I can submit a v6?

Best regards,
Jose Miguel Abreu
Stephen Boyd April 20, 2016, 1:54 a.m. UTC | #9
On 04/19, Jose Abreu wrote:
> 
> @Stephen: can you give some input so that I can submit a v6?
> 

I don't prefer putting the second register in the same DT node,
but that's really up to the DT reviewers to approve such a
design. The current binding has been acked by Rob right?

Assuming the new binding is acked/reviewed then that solution is
fine.

Otherwise, I still prefer two DTS files for the two different FPGA
versions. At the least, please use ioremap for any pointers that
you readl/writel here.

Beyond that, we should have a fixed rate source clk somewhere in
the software view of the clk tree, because that reflects reality.
Hardcoding the parent rate in the structure works, but doesn't
properly express the clk tree.
Jose Abreu April 20, 2016, 9:47 a.m. UTC | #10
Hi Stephen,


On 20-04-2016 02:54, Stephen Boyd wrote:
> On 04/19, Jose Abreu wrote:
>> @Stephen: can you give some input so that I can submit a v6?
>>
> I don't prefer putting the second register in the same DT node,
> but that's really up to the DT reviewers to approve such a
> design. The current binding has been acked by Rob right?

Yes.

> Assuming the new binding is acked/reviewed then that solution is
> fine.

Ok, will then use the DT to pass the FPGA version register.

> Otherwise, I still prefer two DTS files for the two different FPGA
> versions. At the least, please use ioremap for any pointers that
> you readl/writel here.
>
> Beyond that, we should have a fixed rate source clk somewhere in
> the software view of the clk tree, because that reflects reality.
> Hardcoding the parent rate in the structure works, but doesn't
> properly express the clk tree.
>

Can I use a property in the DT to pass this reference clock? something like this:
    snps,parent-freq = <0xFBED9 27000000>, <0x0 28224000>; /* Tuple
<fpga-version reference-clock-freq>, fpga-version = 0 is default */

Or use a parent clock? like:
    clk {
        compatible = "fixed-clock";
        clock-frequency = <27000000>;
        #clock-cells = <0>;
        snps,fpga-version = <0xFBED9>;
    }

It is important to distinguish between the different versions automatically, is
any of these solutions ok?

Best regards,
Jose Miguel Abreu
Alexey Brodkin April 20, 2016, 4:12 p.m. UTC | #11
Hi Jose, Stephen,

On Wed, 2016-04-20 at 10:47 +0100, Jose Abreu wrote:
> Hi Stephen,
> 
> 
> On 20-04-2016 02:54, Stephen Boyd wrote:
> > 
> > On 04/19, Jose Abreu wrote:
> > > 
> > > @Stephen: can you give some input so that I can submit a v6?
> > > 
> > I don't prefer putting the second register in the same DT node,
> > but that's really up to the DT reviewers to approve such a
> > design. The current binding has been acked by Rob right?
> Yes.
> 
> > 
> > Assuming the new binding is acked/reviewed then that solution is
> > fine.
> Ok, will then use the DT to pass the FPGA version register.

We won't need to know FPGA version at all I think.
Read my comment below.

> > 
> > Otherwise, I still prefer two DTS files for the two different FPGA
> > versions. At the least, please use ioremap for any pointers that
> > you readl/writel here.
> > 
> > Beyond that, we should have a fixed rate source clk somewhere in
> > the software view of the clk tree, because that reflects reality.
> > Hardcoding the parent rate in the structure works, but doesn't
> > properly express the clk tree.
> > 
> Can I use a property in the DT to pass this reference clock? something like this:
>     snps,parent-freq = <0xFBED9 27000000>, <0x0 28224000>; /* Tuple
> <fpga-version reference-clock-freq>, fpga-version = 0 is default */
> 
> Or use a parent clock? like:
>     clk {
>         compatible = "fixed-clock";
>         clock-frequency = <27000000>;
>         #clock-cells = <0>;
>         snps,fpga-version = <0xFBED9>;
>     }
> 
> It is important to distinguish between the different versions automatically, is
> any of these solutions ok?

I do like that solution with a master clock but with some fine-tuning
for simplification.

We'll add master clock node for I2S as a fixed clock like that:
------------------->8------------------
	i2s_master_clock: clk {
		#clock-cells = <0>;
		compatible = "fixed-clock";
		clock-frequency = <27000000>;
	};
------------------->8------------------

Note there's no mention of MB version, just a value of the frequency.
And in the driver itself value of that master clock will be used for
population of "pll_clk->ref_clk" directly.

These are benefits we'll get with that approach:
 [1] We escape any IOs not related to our clock device (I mean
     "snps,i2s-pll-clock") itself.
 [2] We'll use whatever reference clock value is given.
     I.e. we'll be able to do a fix-up of that reference clock
     value early in platform code depending on HW we're running on.
     That's what people do here and there.
 [3] Remember another clock driver for AXS10x board is right around
     the corner. I mean the one for ARC PGU which uses exactly the same
     master clock. So one fixup as mentioned above will work
     at once for 2 clock drivers.

Let me know if above makes sense.

-Alexey
Jose Abreu April 21, 2016, 9:51 a.m. UTC | #12
Hi Alexey,


On 20-04-2016 17:12, Alexey Brodkin wrote:
> Hi Jose, Stephen,
>
> On Wed, 2016-04-20 at 10:47 +0100, Jose Abreu wrote:
>> Hi Stephen,
>>
>>
>> On 20-04-2016 02:54, Stephen Boyd wrote:
>>> On 04/19, Jose Abreu wrote:
>>>> @Stephen: can you give some input so that I can submit a v6?
>>>>
>>> I don't prefer putting the second register in the same DT node,
>>> but that's really up to the DT reviewers to approve such a
>>> design. The current binding has been acked by Rob right?
>> Yes.
>>
>>> Assuming the new binding is acked/reviewed then that solution is
>>> fine.
>> Ok, will then use the DT to pass the FPGA version register.
> We won't need to know FPGA version at all I think.
> Read my comment below.
>
>>> Otherwise, I still prefer two DTS files for the two different FPGA
>>> versions. At the least, please use ioremap for any pointers that
>>> you readl/writel here.
>>>
>>> Beyond that, we should have a fixed rate source clk somewhere in
>>> the software view of the clk tree, because that reflects reality.
>>> Hardcoding the parent rate in the structure works, but doesn't
>>> properly express the clk tree.
>>>
>> Can I use a property in the DT to pass this reference clock? something like this:
>>     snps,parent-freq = <0xFBED9 27000000>, <0x0 28224000>; /* Tuple
>> <fpga-version reference-clock-freq>, fpga-version = 0 is default */
>>
>> Or use a parent clock? like:
>>     clk {
>>         compatible = "fixed-clock";
>>         clock-frequency = <27000000>;
>>         #clock-cells = <0>;
>>         snps,fpga-version = <0xFBED9>;
>>     }
>>
>> It is important to distinguish between the different versions automatically, is
>> any of these solutions ok?
> I do like that solution with a master clock but with some fine-tuning
> for simplification.
>
> We'll add master clock node for I2S as a fixed clock like that:
> ------------------->8------------------
> 	i2s_master_clock: clk {
> 		#clock-cells = <0>;
> 		compatible = "fixed-clock";
> 		clock-frequency = <27000000>;
> 	};
> ------------------->8------------------
>
> Note there's no mention of MB version, just a value of the frequency.
> And in the driver itself value of that master clock will be used for
> population of "pll_clk->ref_clk" directly.
>
> These are benefits we'll get with that approach:
>  [1] We escape any IOs not related to our clock device (I mean
>      "snps,i2s-pll-clock") itself.
>  [2] We'll use whatever reference clock value is given.
>      I.e. we'll be able to do a fix-up of that reference clock
>      value early in platform code depending on HW we're running on.
>      That's what people do here and there.
>  [3] Remember another clock driver for AXS10x board is right around
>      the corner. I mean the one for ARC PGU which uses exactly the same
>      master clock. So one fixup as mentioned above will work
>      at once for 2 clock drivers.
>
> Let me know if above makes sense.

That approach can't be used because the reference clock value will change in the
next firmware release.  The new release will have a reference clock of 28224000
Hz instead of the usual 27000000 Hz, so we need to have a way to distinguish
between them. Because of that we can't have only one master clock unless you
state to users that they have to change the reference clock value when using the
new firmware release. Stephen suggested to use two DT files (one for each
firmware release), but as Vineet said this would be annoying to the user so I am
trying to use another solution so that only one DT file is required.

>
> -Alexey

Best regards,
Jose Miguel Abreu
Alexey Brodkin April 21, 2016, 12:18 p.m. UTC | #13
Hi Jose,

On Thu, 2016-04-21 at 10:51 +0100, Jose Abreu wrote:
> Hi Alexey,


> > > > Otherwise, I still prefer two DTS files for the two different FPGA
> > > > versions. At the least, please use ioremap for any pointers that
> > > > you readl/writel here.
> > > > 
> > > > Beyond that, we should have a fixed rate source clk somewhere in
> > > > the software view of the clk tree, because that reflects reality.
> > > > Hardcoding the parent rate in the structure works, but doesn't
> > > > properly express the clk tree.
> > > > 
> > > Can I use a property in the DT to pass this reference clock? something like this:
> > >     snps,parent-freq = <0xFBED9 27000000>, <0x0 28224000>; /* Tuple
> > > <fpga-version reference-clock-freq>, fpga-version = 0 is default */
> > > 
> > > Or use a parent clock? like:
> > >     clk {
> > >         compatible = "fixed-clock";
> > >         clock-frequency = <27000000>;
> > >         #clock-cells = <0>;
> > >         snps,fpga-version = <0xFBED9>;
> > >     }
> > > 
> > > It is important to distinguish between the different versions automatically, is
> > > any of these solutions ok?
> > I do like that solution with a master clock but with some fine-tuning
> > for simplification.
> > 
> > We'll add master clock node for I2S as a fixed clock like that:
> > ------------------->8------------------
> > 	i2s_master_clock: clk {
> > 		#clock-cells = <0>;
> > 		compatible = "fixed-clock";
> > 		clock-frequency = <27000000>;
> > 	};
> > ------------------->8------------------
> > 
> > Note there's no mention of MB version, just a value of the frequency.
> > And in the driver itself value of that master clock will be used for
> > population of "pll_clk->ref_clk" directly.
> > 
> > These are benefits we'll get with that approach:
> >  [1] We escape any IOs not related to our clock device (I mean
> >      "snps,i2s-pll-clock") itself.
> >  [2] We'll use whatever reference clock value is given.
> >      I.e. we'll be able to do a fix-up of that reference clock
> >      value early in platform code depending on HW we're running on.
> >      That's what people do here and there.
> >  [3] Remember another clock driver for AXS10x board is right around
> >      the corner. I mean the one for ARC PGU which uses exactly the same
> >      master clock. So one fixup as mentioned above will work
> >      at once for 2 clock drivers.
> > 
> > Let me know if above makes sense.
> That approach can't be used because the reference clock value will change in the
> next firmware release.  The new release will have a reference clock of 28224000
> Hz instead of the usual 27000000 Hz, so we need to have a way to distinguish
> between them. Because of that we can't have only one master clock unless you
> state to users that they have to change the reference clock value when using the
> new firmware release. Stephen suggested to use two DT files (one for each
> firmware release), but as Vineet said this would be annoying to the user so I am
> trying to use another solution so that only one DT file is required.

Ok reference clock will change.
But I may guess we'll still be able to determine at least that new
firmware version in run-time, right? If so we'll update a fix-up in
early axs10x platform code so that reference clock will be set as 28224000 Hz.

And indeed 2 DT files is a no go - we want to run the same one binary
(with built-in .dtb) on all flavors of AXS boards. And fix-up I'm talking about
will actually do transformation of .dtb early on kernel boot process so that will
be a complete equivalent of different DT files.

-Alexey
Jose Abreu April 21, 2016, 1:10 p.m. UTC | #14
Hi Alexey,


On 21-04-2016 13:18, Alexey Brodkin wrote:
> Hi Jose,
>
> On Thu, 2016-04-21 at 10:51 +0100, Jose Abreu wrote:
>> Hi Alexey,
>
>>>>> Otherwise, I still prefer two DTS files for the two different FPGA
>>>>> versions. At the least, please use ioremap for any pointers that
>>>>> you readl/writel here.
>>>>>
>>>>> Beyond that, we should have a fixed rate source clk somewhere in
>>>>> the software view of the clk tree, because that reflects reality.
>>>>> Hardcoding the parent rate in the structure works, but doesn't
>>>>> properly express the clk tree.
>>>>>
>>>> Can I use a property in the DT to pass this reference clock? something like this:
>>>>     snps,parent-freq = <0xFBED9 27000000>, <0x0 28224000>; /* Tuple
>>>> <fpga-version reference-clock-freq>, fpga-version = 0 is default */
>>>>
>>>> Or use a parent clock? like:
>>>>     clk {
>>>>         compatible = "fixed-clock";
>>>>         clock-frequency = <27000000>;
>>>>         #clock-cells = <0>;
>>>>         snps,fpga-version = <0xFBED9>;
>>>>     }
>>>>
>>>> It is important to distinguish between the different versions automatically, is
>>>> any of these solutions ok?
>>> I do like that solution with a master clock but with some fine-tuning
>>> for simplification.
>>>
>>> We'll add master clock node for I2S as a fixed clock like that:
>>> ------------------->8------------------
>>> 	i2s_master_clock: clk {
>>> 		#clock-cells = <0>;
>>> 		compatible = "fixed-clock";
>>> 		clock-frequency = <27000000>;
>>> 	};
>>> ------------------->8------------------
>>>
>>> Note there's no mention of MB version, just a value of the frequency.
>>> And in the driver itself value of that master clock will be used for
>>> population of "pll_clk->ref_clk" directly.
>>>
>>> These are benefits we'll get with that approach:
>>>  [1] We escape any IOs not related to our clock device (I mean
>>>      "snps,i2s-pll-clock") itself.
>>>  [2] We'll use whatever reference clock value is given.
>>>      I.e. we'll be able to do a fix-up of that reference clock
>>>      value early in platform code depending on HW we're running on.
>>>      That's what people do here and there.
>>>  [3] Remember another clock driver for AXS10x board is right around
>>>      the corner. I mean the one for ARC PGU which uses exactly the same
>>>      master clock. So one fixup as mentioned above will work
>>>      at once for 2 clock drivers.
>>>
>>> Let me know if above makes sense.
>> That approach can't be used because the reference clock value will change in the
>> next firmware release.  The new release will have a reference clock of 28224000
>> Hz instead of the usual 27000000 Hz, so we need to have a way to distinguish
>> between them. Because of that we can't have only one master clock unless you
>> state to users that they have to change the reference clock value when using the
>> new firmware release. Stephen suggested to use two DT files (one for each
>> firmware release), but as Vineet said this would be annoying to the user so I am
>> trying to use another solution so that only one DT file is required.
> Ok reference clock will change.
> But I may guess we'll still be able to determine at least that new
> firmware version in run-time, right? If so we'll update a fix-up in
> early axs10x platform code so that reference clock will be set as 28224000 Hz.

Yes, there is a register where the FPGA version date is encoded, we can use that
to check which firmware is used (if date <= old_firmware_date then
clock=27000000; else clock=28224000). If that fix is acceptable it could be a
good solution without having to use custom parameters in the DT (no need to
encode the different clocks and we would only use one master clock) but I am not
sure where and how this can be encoded and I don't know how to change the DT on
runtime. Can you give me some guidelines?

>
> And indeed 2 DT files is a no go - we want to run the same one binary
> (with built-in .dtb) on all flavors of AXS boards. And fix-up I'm talking about
> will actually do transformation of .dtb early on kernel boot process so that will
> be a complete equivalent of different DT files.

And doing modifications on the DT can cause some misdirections to users.
Besides, we would have clock specific functions in init procedures which is
precisely what we are trying to avoid by submitting this driver.

>
> -Alexey

Best regards,
Jose Miguel Abreu
Alexey Brodkin April 21, 2016, 2:15 p.m. UTC | #15
Hi Jose,

On Thu, 2016-04-21 at 14:10 +0100, Jose Abreu wrote:
> Hi Alexey,
> 
> 
> On 21-04-2016 13:18, Alexey Brodkin wrote:
> > 
> > Hi Jose,
> > 
> > On Thu, 2016-04-21 at 10:51 +0100, Jose Abreu wrote:
> > > 
> > > Hi Alexey,
> > > 
> > Ok reference clock will change.
> > But I may guess we'll still be able to determine at least that new
> > firmware version in run-time, right? If so we'll update a fix-up in
> > early axs10x platform code so that reference clock will be set as 28224000 Hz.
> Yes, there is a register where the FPGA version date is encoded, we can use that
> to check which firmware is used (if date <= old_firmware_date then
> clock=27000000; else clock=28224000). If that fix is acceptable it could be a
> good solution without having to use custom parameters in the DT (no need to
> encode the different clocks and we would only use one master clock) but I am not
> sure where and how this can be encoded and I don't know how to change the DT on
> runtime. Can you give me some guidelines?

Take a look here - http://git.kernel.org/cgit/linux/kernel/git/vgupta/arc.git/commit/arch/arc/plat-axs10x/axs10x.c?h=for
-next&id=5cd0f5102753a7405548d0c66c11a2a0a05bbf2e

We do something very similar here - we're patching in run-time
core frequency that was specified in .dts.

And in the very same way one will be able to do fix-ups for other
clocks.

Moreover I would propose to think about that fix-up as of completely
separate topic. I.e. in your driver for AXS' I2S clock just use a new
reference "fixed-clock" (that you'll add in "axs10x_mb.dtsi" as a part of
your driver submission). And once your driver gets accepted we'll work on
fix-up in axs10x platform.

This way we'll move with smaller steps and hopefully will get things done
sooner.

> > And indeed 2 DT files is a no go - we want to run the same one binary
> > (with built-in .dtb) on all flavors of AXS boards. And fix-up I'm talking about
> > will actually do transformation of .dtb early on kernel boot process so that will
> > be a complete equivalent of different DT files.
> And doing modifications on the DT can cause some misdirections to users.

What do you mean here? What kind of problems do you expect to face?

> Besides, we would have clock specific functions in init procedures which is
> precisely what we are trying to avoid by submitting this driver.

You're talking about fixups above here?

-Alexey
Vineet Gupta April 22, 2016, 5:50 a.m. UTC | #16
On Thursday 21 April 2016 05:48 PM, Alexey Brodkin wrote:
> Hi Jose,
> 
> On Thu, 2016-04-21 at 10:51 +0100, Jose Abreu wrote:
>> Hi Alexey,
> 
> 
>>>>> Otherwise, I still prefer two DTS files for the two different FPGA
>>>>> versions. At the least, please use ioremap for any pointers that
>>>>> you readl/writel here.
>>>>>
>>>>> Beyond that, we should have a fixed rate source clk somewhere in
>>>>> the software view of the clk tree, because that reflects reality.
>>>>> Hardcoding the parent rate in the structure works, but doesn't
>>>>> properly express the clk tree.
>>>>>
>>>> Can I use a property in the DT to pass this reference clock? something like this:
>>>>     snps,parent-freq = <0xFBED9 27000000>, <0x0 28224000>; /* Tuple
>>>> <fpga-version reference-clock-freq>, fpga-version = 0 is default */
>>>>
>>>> Or use a parent clock? like:
>>>>     clk {
>>>>         compatible = "fixed-clock";
>>>>         clock-frequency = <27000000>;
>>>>         #clock-cells = <0>;
>>>>         snps,fpga-version = <0xFBED9>;
>>>>     }
>>>>
>>>> It is important to distinguish between the different versions automatically, is
>>>> any of these solutions ok?
>>> I do like that solution with a master clock but with some fine-tuning
>>> for simplification.
>>>
>>> We'll add master clock node for I2S as a fixed clock like that:
>>> ------------------->8------------------
>>> 	i2s_master_clock: clk {
>>> 		#clock-cells = <0>;
>>> 		compatible = "fixed-clock";
>>> 		clock-frequency = <27000000>;
>>> 	};
>>> ------------------->8------------------
>>>
>>> Note there's no mention of MB version, just a value of the frequency.
>>> And in the driver itself value of that master clock will be used for
>>> population of "pll_clk->ref_clk" directly.
>>>
>>> These are benefits we'll get with that approach:
>>>  [1] We escape any IOs not related to our clock device (I mean
>>>      "snps,i2s-pll-clock") itself.
>>>  [2] We'll use whatever reference clock value is given.
>>>      I.e. we'll be able to do a fix-up of that reference clock
>>>      value early in platform code depending on HW we're running on.
>>>      That's what people do here and there.
>>>  [3] Remember another clock driver for AXS10x board is right around
>>>      the corner. I mean the one for ARC PGU which uses exactly the same
>>>      master clock. So one fixup as mentioned above will work
>>>      at once for 2 clock drivers.
>>>
>>> Let me know if above makes sense.
>> That approach can't be used because the reference clock value will change in the
>> next firmware release.  The new release will have a reference clock of 28224000
>> Hz instead of the usual 27000000 Hz, so we need to have a way to distinguish
>> between them. Because of that we can't have only one master clock unless you
>> state to users that they have to change the reference clock value when using the
>> new firmware release. Stephen suggested to use two DT files (one for each
>> firmware release), but as Vineet said this would be annoying to the user so I am
>> trying to use another solution so that only one DT file is required.
> 
> Ok reference clock will change.
> But I may guess we'll still be able to determine at least that new
> firmware version in run-time, right? If so we'll update a fix-up in
> early axs10x platform code so that reference clock will be set as 28224000 Hz.


Please no - lets not bolt-in more hacks and instead try do this cleanly if
possible. And from other discusions it seems there might be a way. The readl
approach seems fine to me (with ioremap) if that is what it takes.


> And indeed 2 DT files is a no go - we want to run the same one binary
> (with built-in .dtb) on all flavors of AXS boards.

Right - 2 DT is not acceptable unless we are feeling bored and want more emails
for AXS board support :-)

 And fix-up I'm talking about
> will actually do transformation of .dtb early on kernel boot process so that will
> be a complete equivalent of different DT files.
> 
> -Alexey--
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/clock/i2s-pll-clock.txt b/Documentation/devicetree/bindings/clock/i2s-pll-clock.txt
new file mode 100644
index 0000000..ff86a41
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/i2s-pll-clock.txt
@@ -0,0 +1,17 @@ 
+Binding for the AXS10X I2S PLL clock
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible: shall be "snps,i2s-pll-clock"
+- #clock-cells: from common clock binding; Should always be set to 0.
+- reg : Address and length of the I2S PLL register set.
+
+Example:
+	clock@0x100a0 {
+		compatible = "snps,i2s-pll-clock";
+		reg = <0x100a0 0x10>;
+		#clock-cells = <0>;
+	};
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 46869d6..2ca62dc6 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -84,3 +84,4 @@  obj-$(CONFIG_X86)			+= x86/
 obj-$(CONFIG_ARCH_ZX)			+= zte/
 obj-$(CONFIG_ARCH_ZYNQ)			+= zynq/
 obj-$(CONFIG_H8300)		+= h8300/
+obj-$(CONFIG_ARC_PLAT_AXS10X)		+= axs10x/
diff --git a/drivers/clk/axs10x/Makefile b/drivers/clk/axs10x/Makefile
new file mode 100644
index 0000000..01996b8
--- /dev/null
+++ b/drivers/clk/axs10x/Makefile
@@ -0,0 +1 @@ 
+obj-y += i2s_pll_clock.o
diff --git a/drivers/clk/axs10x/i2s_pll_clock.c b/drivers/clk/axs10x/i2s_pll_clock.c
new file mode 100644
index 0000000..3ba4e2f
--- /dev/null
+++ b/drivers/clk/axs10x/i2s_pll_clock.c
@@ -0,0 +1,217 @@ 
+/*
+ * Synopsys AXS10X SDP I2S PLL clock driver
+ *
+ * Copyright (C) 2016 Synopsys
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+
+/* FPGA Version Info */
+#define FPGA_VER_INFO	0xE0011230
+#define FPGA_VER_27M	0x000FBED9
+
+/* PLL registers addresses */
+#define PLL_IDIV_REG	0x0
+#define PLL_FBDIV_REG	0x4
+#define PLL_ODIV0_REG	0x8
+#define PLL_ODIV1_REG	0xC
+
+struct i2s_pll_cfg {
+	unsigned int rate;
+	unsigned int idiv;
+	unsigned int fbdiv;
+	unsigned int odiv0;
+	unsigned int odiv1;
+};
+
+static const struct i2s_pll_cfg i2s_pll_cfg_27m[] = {
+	/* 27 Mhz */
+	{ 1024000, 0x104, 0x451, 0x10E38, 0x2000 },
+	{ 1411200, 0x104, 0x596, 0x10D35, 0x2000 },
+	{ 1536000, 0x208, 0xA28, 0x10B2C, 0x2000 },
+	{ 2048000, 0x82, 0x451, 0x10E38, 0x2000 },
+	{ 2822400, 0x82, 0x596, 0x10D35, 0x2000 },
+	{ 3072000, 0x104, 0xA28, 0x10B2C, 0x2000 },
+	{ 2116800, 0x82, 0x3CF, 0x10C30, 0x2000 },
+	{ 2304000, 0x104, 0x79E, 0x10B2C, 0x2000 },
+	{ 0, 0, 0, 0, 0 },
+};
+
+static const struct i2s_pll_cfg i2s_pll_cfg_28m[] = {
+	/* 28.224 Mhz */
+	{ 1024000, 0x82, 0x105, 0x107DF, 0x2000 },
+	{ 1411200, 0x28A, 0x1, 0x10001, 0x2000 },
+	{ 1536000, 0xA28, 0x187, 0x10042, 0x2000 },
+	{ 2048000, 0x41, 0x105, 0x107DF, 0x2000 },
+	{ 2822400, 0x145, 0x1, 0x10001, 0x2000 },
+	{ 3072000, 0x514, 0x187, 0x10042, 0x2000 },
+	{ 2116800, 0x514, 0x42, 0x10001, 0x2000 },
+	{ 2304000, 0x619, 0x82, 0x10001, 0x2000 },
+	{ 0, 0, 0, 0, 0 },
+};
+
+struct i2s_pll_clk {
+	void __iomem *base;
+	struct clk_hw hw;
+	unsigned int ref_clk;
+	const struct i2s_pll_cfg *pll_cfg;
+};
+
+static inline void i2s_pll_write(struct i2s_pll_clk *clk, unsigned int reg,
+		unsigned int val)
+{
+	writel_relaxed(val, clk->base + reg);
+}
+
+static inline unsigned int i2s_pll_read(struct i2s_pll_clk *clk,
+		unsigned int reg)
+{
+	return readl_relaxed(clk->base + reg);
+}
+
+static inline struct i2s_pll_clk *to_i2s_pll_clk(struct clk_hw *hw)
+{
+	return container_of(hw, struct i2s_pll_clk, hw);
+}
+
+static unsigned int i2s_pll_get_value(unsigned int val)
+{
+	return (val & 0x3F) + ((val >> 6) & 0x3F);
+}
+
+static unsigned long i2s_pll_recalc_rate(struct clk_hw *hw,
+			unsigned long parent_rate)
+{
+	struct i2s_pll_clk *clk = to_i2s_pll_clk(hw);
+	unsigned int idiv, fbdiv, odiv;
+
+	idiv = i2s_pll_get_value(i2s_pll_read(clk, PLL_IDIV_REG));
+	fbdiv = i2s_pll_get_value(i2s_pll_read(clk, PLL_FBDIV_REG));
+	odiv = i2s_pll_get_value(i2s_pll_read(clk, PLL_ODIV0_REG));
+
+	return ((clk->ref_clk / idiv) * fbdiv) / odiv;
+}
+
+static long i2s_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+			unsigned long *prate)
+{
+	const struct i2s_pll_cfg *pll_cfg = to_i2s_pll_clk(hw)->pll_cfg;
+	int i;
+
+	for (i = 0; pll_cfg[i].rate != 0; i++)
+		if (pll_cfg[i].rate == rate)
+			return rate;
+
+	return -EINVAL;
+}
+
+static int i2s_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+			unsigned long parent_rate)
+{
+	struct i2s_pll_clk *clk = to_i2s_pll_clk(hw);
+	const struct i2s_pll_cfg *pll_cfg = clk->pll_cfg;
+	int i;
+
+	for (i = 0; pll_cfg[i].rate != 0; i++) {
+		if (pll_cfg[i].rate == rate) {
+			i2s_pll_write(clk, PLL_IDIV_REG, pll_cfg[i].idiv);
+			i2s_pll_write(clk, PLL_FBDIV_REG, pll_cfg[i].fbdiv);
+			i2s_pll_write(clk, PLL_ODIV0_REG, pll_cfg[i].odiv0);
+			i2s_pll_write(clk, PLL_ODIV1_REG, pll_cfg[i].odiv1);
+			return 0;
+		}
+	}
+
+	pr_err("%s: invalid rate=%ld, parent_rate=%ld\n", __func__,
+			rate, parent_rate);
+	return -EINVAL;
+}
+
+static const struct clk_ops i2s_pll_ops = {
+	.recalc_rate = i2s_pll_recalc_rate,
+	.round_rate = i2s_pll_round_rate,
+	.set_rate = i2s_pll_set_rate,
+};
+
+static int i2s_pll_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	const char *clk_name;
+	struct clk *clk;
+	struct i2s_pll_clk *pll_clk;
+	struct clk_init_data init;
+	struct resource *mem;
+
+	if (!node)
+		return -ENODEV;
+
+	pll_clk = devm_kzalloc(dev, sizeof(*pll_clk), GFP_KERNEL);
+	if (!pll_clk)
+		return -ENOMEM;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pll_clk->base = devm_ioremap_resource(dev, mem);
+	if (IS_ERR(pll_clk->base))
+		return PTR_ERR(pll_clk->base);
+
+	clk_name = node->name;
+	init.name = clk_name;
+	init.ops = &i2s_pll_ops;
+	init.num_parents = 0;
+	pll_clk->hw.init = &init;
+
+	clk = clk_register(NULL, &pll_clk->hw);
+	if (IS_ERR(clk)) {
+		dev_err(dev, "failed to register %s div clock (%ld)\n",
+				clk_name, PTR_ERR(clk));
+		return PTR_ERR(clk);
+	}
+
+	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M) {
+		pll_clk->ref_clk = 27000000;
+		pll_clk->pll_cfg = i2s_pll_cfg_27m;
+	} else {
+		pll_clk->ref_clk = 28224000;
+		pll_clk->pll_cfg = i2s_pll_cfg_28m;
+	}
+
+	return of_clk_add_provider(node, of_clk_src_simple_get, clk);
+}
+
+static int i2s_pll_clk_remove(struct platform_device *pdev)
+{
+	of_clk_del_provider(pdev->dev.of_node);
+	return 0;
+}
+
+static const struct of_device_id i2s_pll_clk_id[] = {
+	{ .compatible = "snps,i2s-pll-clock", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, i2s_pll_clk_id);
+
+static struct platform_driver i2s_pll_clk_driver = {
+	.driver = {
+		.name = "i2s-pll-clock",
+		.of_match_table = of_match_ptr(i2s_pll_clk_id),
+	},
+	.probe = i2s_pll_clk_probe,
+	.remove = i2s_pll_clk_remove,
+};
+module_platform_driver(i2s_pll_clk_driver);
+
+MODULE_AUTHOR("Jose Abreu <joabreu@synopsys.com>");
+MODULE_DESCRIPTION("Synopsys AXS10X SDP I2S PLL Clock Driver");
+MODULE_LICENSE("GPL v2");