[PATCH] clk/arc: Add I2S PLL clock driver

[PATCH] clk/arc: Add I2S PLL clock driver

[Alexey Brodkin]

Hi Jose,

On Tue, 2016-03-29@11:56 +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 at synopsys.com>
> ---

I believe these kind of patches worth sending to
linux-snps mailing list (at least add it in Cc) so
they won't be lost in time and could be used later as
useful references.

> ?arch/arc/boot/dts/axs10x_mb.dtsi |???5 ++
> ?drivers/clk/Makefile?????????????|???1 +
> ?drivers/clk/arc/Makefile?????????|???1 +
> ?drivers/clk/arc/i2s_pll_clock.c??| 143 +++++++++++++++++++++++++++++++++++++++
> ?4 files changed, 150 insertions(+)
> ?create mode 100644 drivers/clk/arc/Makefile
> ?create mode 100644 drivers/clk/arc/i2s_pll_clock.c
> 
> diff --git a/arch/arc/boot/dts/axs10x_mb.dtsi b/arch/arc/boot/dts/axs10x_mb.dtsi
> index ab5d570..9c68226 100644
> --- a/arch/arc/boot/dts/axs10x_mb.dtsi
> +++ b/arch/arc/boot/dts/axs10x_mb.dtsi
> @@ -23,6 +23,11 @@
> ?				#clock-cells = <0>;
> ?			};
> ?
> +			i2sclk: i2sclk {
> +				compatible = "snps,i2s-pll-clock";
> +				#clock-cells = <0>;
> +			};
> +
> ?			apbclk: apbclk {
> ?				compatible = "fixed-clock";
> ?				clock-frequency = <50000000>;
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 46869d6..620e47f 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)		+= arc/

Two things here:
?[1] This clock is relevant to only one board but not SoC, CPU core
? ? ?or whole architecture, so it makes sense to make it dependent on
? ? ?the AXS platform, i.e. CONFIG_ARC_PLAT_AXS10X.

?[2] Something similar is applicable to folder name, I would suggest to
? ? ?use "drivers/clk/axs10x"

> diff --git a/drivers/clk/arc/Makefile b/drivers/clk/arc/Makefile
> new file mode 100644
> index 0000000..01996b8
> --- /dev/null
> +++ b/drivers/clk/arc/Makefile
> @@ -0,0 +1 @@
> +obj-y += i2s_pll_clock.o
> diff --git a/drivers/clk/arc/i2s_pll_clock.c b/drivers/clk/arc/i2s_pll_clock.c
> new file mode 100644
> index 0000000..8c401f1
> --- /dev/null
> +++ b/drivers/clk/arc/i2s_pll_clock.c
> @@ -0,0 +1,143 @@
> +/*
> + * Synopsys I2S PLL clock driver

Again that's not generic SNPS I2S clock but clock driver for a particular board.

> + *
> + * drivers/clk/arc/i2s_pll_clock.c

BTW not sure why this path could be needed here.
IMHO that might be safely dropped.

> + *
> + * 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/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +/* FPGA Version Info */
> +#define FPGA_VER_INFO	0xE0011230
> +#define FPGA_VER_27M	0x000FBED9

This is a little bit tricky.
We'll need to do that check in each and every clock driver for AXS10x
and so it worth putting this code in some common location (I mean common
for all axs10x clock drivers).

> +/* PLL registers addresses */
> +#define PLL_IDIV_ADDR	0xE00100A0
> +#define PLL_FBDIV_ADDR	0xE00100A4
> +#define PLL_ODIV0_ADDR	0xE00100A8
> +#define PLL_ODIV1_ADDR	0xE00100AC
>

> +struct i2s_pll_clk {
> +	struct clk_hw hw;
> +};
> +
> +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 },
> +	{ 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 },
> +	{ 0, 0, 0, 0, 0 },
> +};
> +
> +static unsigned long i2s_pll_recalc_rate(struct clk_hw *hw,
> +			unsigned long parent_rate)
> +{
> +	/* TODO */
> +	pr_info("%s: parent_rate=%ld\n", __func__, parent_rate);
> +	return parent_rate;
> +}
> +
> +static long i2s_pll_round_rate(struct clk_hw *hw, unsigned long rate,
> +			unsigned long *prate)
> +{
> +	/* TODO: Round rate to nearest valid rate */
> +	*prate = rate;
> +	pr_info("%s: rate=%ld, prate=%ld\n", __func__, rate, *prate);
> +	return *prate;
> +}

These are now just dummy functions so probably they could be dropped.

> +static int i2s_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> +			unsigned long parent_rate)
> +{
> +	const struct i2s_pll_cfg *pll_cfg;
> +	int i;
> +
> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M)
> +		pll_cfg = i2s_pll_cfg_27m;
> +	else
> +		pll_cfg = i2s_pll_cfg_28m;

As I mentioned above we need to check board version in common place and then
just use some variable or structure member for comparison.

Otherwise it looks pretty nice!

-Alexey

[PATCH] clk/arc: Add I2S PLL clock driver

[Vineet Gupta]

On Tuesday 29 March 2016 04:55 PM, Alexey Brodkin wrote:
> Hi Jose,
>
> On Tue, 2016-03-29@11:56 +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 at synopsys.com>
>> ---
> I believe these kind of patches worth sending to
> linux-snps mailing list (at least add it in Cc) so
> they won't be lost in time and could be used later as
> useful references.

Please do also CC the common clk maintainers

COMMON CLK FRAMEWORK
M:    Michael Turquette <mturquette at baylibre.com>
M:    Stephen Boyd <sboyd at codeaurora.org>
L:    linux-clk at vger.kernel.org

>
>>  arch/arc/boot/dts/axs10x_mb.dtsi |   5 ++
>>  drivers/clk/Makefile             |   1 +
>>  drivers/clk/arc/Makefile         |   1 +
>>  drivers/clk/arc/i2s_pll_clock.c  | 143 +++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 150 insertions(+)
>>  create mode 100644 drivers/clk/arc/Makefile
>>  create mode 100644 drivers/clk/arc/i2s_pll_clock.c
>>
>> diff --git a/arch/arc/boot/dts/axs10x_mb.dtsi b/arch/arc/boot/dts/axs10x_mb.dtsi
>> index ab5d570..9c68226 100644
>> --- a/arch/arc/boot/dts/axs10x_mb.dtsi
>> +++ b/arch/arc/boot/dts/axs10x_mb.dtsi
>> @@ -23,6 +23,11 @@
>>  				#clock-cells = <0>;
>>  			};
>>  
>> +			i2sclk: i2sclk {
>> +				compatible = "snps,i2s-pll-clock";
>> +				#clock-cells = <0>;
>> +			};
>> +
>>  			apbclk: apbclk {
>>  				compatible = "fixed-clock";
>>  				clock-frequency = <50000000>;
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index 46869d6..620e47f 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)		+= arc/
> Two things here:
>  [1] This clock is relevant to only one board but not SoC, CPU core
>      or whole architecture, so it makes sense to make it dependent on
>      the AXS platform, i.e. CONFIG_ARC_PLAT_AXS10X.
>
>  [2] Something similar is applicable to folder name, I would suggest to
>      use "drivers/clk/axs10x"
>
>> diff --git a/drivers/clk/arc/Makefile b/drivers/clk/arc/Makefile
>> new file mode 100644
>> index 0000000..01996b8
>> --- /dev/null
>> +++ b/drivers/clk/arc/Makefile
>> @@ -0,0 +1 @@
>> +obj-y += i2s_pll_clock.o
>> diff --git a/drivers/clk/arc/i2s_pll_clock.c b/drivers/clk/arc/i2s_pll_clock.c
>> new file mode 100644
>> index 0000000..8c401f1
>> --- /dev/null
>> +++ b/drivers/clk/arc/i2s_pll_clock.c
>> @@ -0,0 +1,143 @@
>> +/*
>> + * Synopsys I2S PLL clock driver
> Again that's not generic SNPS I2S clock but clock driver for a particular board.
>
>> + *
>> + * drivers/clk/arc/i2s_pll_clock.c
> BTW not sure why this path could be needed here.
> IMHO that might be safely dropped.
>
>> + *
>> + * 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/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/slab.h>
>> +
>> +/* FPGA Version Info */
>> +#define FPGA_VER_INFO	0xE0011230
>> +#define FPGA_VER_27M	0x000FBED9
> This is a little bit tricky.
> We'll need to do that check in each and every clock driver for AXS10x
> and so it worth putting this code in some common location (I mean common
> for all axs10x clock drivers).
>
>> +/* PLL registers addresses */
>> +#define PLL_IDIV_ADDR	0xE00100A0
>> +#define PLL_FBDIV_ADDR	0xE00100A4
>> +#define PLL_ODIV0_ADDR	0xE00100A8
>> +#define PLL_ODIV1_ADDR	0xE00100AC
>>
>> +struct i2s_pll_clk {
>> +	struct clk_hw hw;
>> +};
>> +
>> +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 },
>> +	{ 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 },
>> +	{ 0, 0, 0, 0, 0 },
>> +};
>> +
>> +static unsigned long i2s_pll_recalc_rate(struct clk_hw *hw,
>> +			unsigned long parent_rate)
>> +{
>> +	/* TODO */
>> +	pr_info("%s: parent_rate=%ld\n", __func__, parent_rate);
>> +	return parent_rate;
>> +}
>> +
>> +static long i2s_pll_round_rate(struct clk_hw *hw, unsigned long rate,
>> +			unsigned long *prate)
>> +{
>> +	/* TODO: Round rate to nearest valid rate */
>> +	*prate = rate;
>> +	pr_info("%s: rate=%ld, prate=%ld\n", __func__, rate, *prate);
>> +	return *prate;
>> +}
> These are now just dummy functions so probably they could be dropped.
>
>> +static int i2s_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>> +			unsigned long parent_rate)
>> +{
>> +	const struct i2s_pll_cfg *pll_cfg;
>> +	int i;
>> +
>> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M)
>> +		pll_cfg = i2s_pll_cfg_27m;
>> +	else
>> +		pll_cfg = i2s_pll_cfg_28m;
> As I mentioned above we need to check board version in common place and then
> just use some variable or structure member for comparison.
>
> Otherwise it looks pretty nice!
>
> -Alexey

[PATCH] clk/arc: Add I2S PLL clock driver

[Jose Abreu]

Hi Vineet,

On 29-03-2016 13:36, Vineet Gupta wrote:
> On Tuesday 29 March 2016 04:55 PM, Alexey Brodkin wrote:
>> Hi Jose,
>>
>> On Tue, 2016-03-29@11:56 +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 at synopsys.com>
>>> ---
>> I believe these kind of patches worth sending to
>> linux-snps mailing list (at least add it in Cc) so
>> they won't be lost in time and could be used later as
>> useful references.
> Please do also CC the common clk maintainers
>
> COMMON CLK FRAMEWORK
> M:    Michael Turquette <mturquette at baylibre.com>
> M:    Stephen Boyd <sboyd at codeaurora.org>
> L:    linux-clk at vger.kernel.org

Well, this was just an experiment and I think this patch is still far from being
ready to go mainline, that's why I did not cc'ed the clk maintainers. I will
send a v2 soon so you can review and if you think its worth sending to mainline
I will do that.

>>>  arch/arc/boot/dts/axs10x_mb.dtsi |   5 ++
>>>  drivers/clk/Makefile             |   1 +
>>>  drivers/clk/arc/Makefile         |   1 +
>>>  drivers/clk/arc/i2s_pll_clock.c  | 143 +++++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 150 insertions(+)
>>>  create mode 100644 drivers/clk/arc/Makefile
>>>  create mode 100644 drivers/clk/arc/i2s_pll_clock.c
>>>
>>> diff --git a/arch/arc/boot/dts/axs10x_mb.dtsi b/arch/arc/boot/dts/axs10x_mb.dtsi
>>> index ab5d570..9c68226 100644
>>> --- a/arch/arc/boot/dts/axs10x_mb.dtsi
>>> +++ b/arch/arc/boot/dts/axs10x_mb.dtsi
>>> @@ -23,6 +23,11 @@
>>>  				#clock-cells = <0>;
>>>  			};
>>>  
>>> +			i2sclk: i2sclk {
>>> +				compatible = "snps,i2s-pll-clock";
>>> +				#clock-cells = <0>;
>>> +			};
>>> +
>>>  			apbclk: apbclk {
>>>  				compatible = "fixed-clock";
>>>  				clock-frequency = <50000000>;
>>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>>> index 46869d6..620e47f 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)		+= arc/
>> Two things here:
>>  [1] This clock is relevant to only one board but not SoC, CPU core
>>      or whole architecture, so it makes sense to make it dependent on
>>      the AXS platform, i.e. CONFIG_ARC_PLAT_AXS10X.
>>
>>  [2] Something similar is applicable to folder name, I would suggest to
>>      use "drivers/clk/axs10x"
>>
>>> diff --git a/drivers/clk/arc/Makefile b/drivers/clk/arc/Makefile
>>> new file mode 100644
>>> index 0000000..01996b8
>>> --- /dev/null
>>> +++ b/drivers/clk/arc/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-y += i2s_pll_clock.o
>>> diff --git a/drivers/clk/arc/i2s_pll_clock.c b/drivers/clk/arc/i2s_pll_clock.c
>>> new file mode 100644
>>> index 0000000..8c401f1
>>> --- /dev/null
>>> +++ b/drivers/clk/arc/i2s_pll_clock.c
>>> @@ -0,0 +1,143 @@
>>> +/*
>>> + * Synopsys I2S PLL clock driver
>> Again that's not generic SNPS I2S clock but clock driver for a particular board.
>>
>>> + *
>>> + * drivers/clk/arc/i2s_pll_clock.c
>> BTW not sure why this path could be needed here.
>> IMHO that might be safely dropped.
>>
>>> + *
>>> + * 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/clk-provider.h>
>>> +#include <linux/err.h>
>>> +#include <linux/device.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/slab.h>
>>> +
>>> +/* FPGA Version Info */
>>> +#define FPGA_VER_INFO	0xE0011230
>>> +#define FPGA_VER_27M	0x000FBED9
>> This is a little bit tricky.
>> We'll need to do that check in each and every clock driver for AXS10x
>> and so it worth putting this code in some common location (I mean common
>> for all axs10x clock drivers).
>>
>>> +/* PLL registers addresses */
>>> +#define PLL_IDIV_ADDR	0xE00100A0
>>> +#define PLL_FBDIV_ADDR	0xE00100A4
>>> +#define PLL_ODIV0_ADDR	0xE00100A8
>>> +#define PLL_ODIV1_ADDR	0xE00100AC
>>>
>>> +struct i2s_pll_clk {
>>> +	struct clk_hw hw;
>>> +};
>>> +
>>> +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 },
>>> +	{ 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 },
>>> +	{ 0, 0, 0, 0, 0 },
>>> +};
>>> +
>>> +static unsigned long i2s_pll_recalc_rate(struct clk_hw *hw,
>>> +			unsigned long parent_rate)
>>> +{
>>> +	/* TODO */
>>> +	pr_info("%s: parent_rate=%ld\n", __func__, parent_rate);
>>> +	return parent_rate;
>>> +}
>>> +
>>> +static long i2s_pll_round_rate(struct clk_hw *hw, unsigned long rate,
>>> +			unsigned long *prate)
>>> +{
>>> +	/* TODO: Round rate to nearest valid rate */
>>> +	*prate = rate;
>>> +	pr_info("%s: rate=%ld, prate=%ld\n", __func__, rate, *prate);
>>> +	return *prate;
>>> +}
>> These are now just dummy functions so probably they could be dropped.
>>
>>> +static int i2s_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>>> +			unsigned long parent_rate)
>>> +{
>>> +	const struct i2s_pll_cfg *pll_cfg;
>>> +	int i;
>>> +
>>> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M)
>>> +		pll_cfg = i2s_pll_cfg_27m;
>>> +	else
>>> +		pll_cfg = i2s_pll_cfg_28m;
>> As I mentioned above we need to check board version in common place and then
>> just use some variable or structure member for comparison.
>>
>> Otherwise it looks pretty nice!
>>
>> -Alexey
>

Best regards,
Jose Miguel Abreu

[PATCH] clk/arc: Add I2S PLL clock driver

[Vineet Gupta]

On Tuesday 29 March 2016 11:21 PM, Jose Abreu wrote:

Well, this was just an experiment and I think this patch is still far from being
ready to go mainline, that's why I did not cc'ed the clk maintainers. I will
send a v2 soon so you can review and if you think its worth sending to mainline
I will do that.


It never hurts to CC actual maintainer as they will be able to provide you the best feedback.
Each of us have our domain expertise so I may review your code superficially but only the person who knows the subsystem inside-out can tell you exactly what to do.

They are grumpy at times and might bitch a bit but as they they "it is your code that gets criticized - not you"
I've learnt this myself over the years :-)

-Vineet

[PATCH] clk/arc: Add I2S PLL clock driver

[Jose Abreu]

Hi Alexey,

On 29-03-2016 12:25, Alexey Brodkin wrote:
> Hi Jose,
>
> On Tue, 2016-03-29@11:56 +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 at synopsys.com>
>> ---
> I believe these kind of patches worth sending to
> linux-snps mailing list (at least add it in Cc) so
> they won't be lost in time and could be used later as
> useful references.

Agree.

>
>>  arch/arc/boot/dts/axs10x_mb.dtsi |   5 ++
>>  drivers/clk/Makefile             |   1 +
>>  drivers/clk/arc/Makefile         |   1 +
>>  drivers/clk/arc/i2s_pll_clock.c  | 143 +++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 150 insertions(+)
>>  create mode 100644 drivers/clk/arc/Makefile
>>  create mode 100644 drivers/clk/arc/i2s_pll_clock.c
>>
>> diff --git a/arch/arc/boot/dts/axs10x_mb.dtsi b/arch/arc/boot/dts/axs10x_mb.dtsi
>> index ab5d570..9c68226 100644
>> --- a/arch/arc/boot/dts/axs10x_mb.dtsi
>> +++ b/arch/arc/boot/dts/axs10x_mb.dtsi
>> @@ -23,6 +23,11 @@
>>  				#clock-cells = <0>;
>>  			};
>>  
>> +			i2sclk: i2sclk {
>> +				compatible = "snps,i2s-pll-clock";
>> +				#clock-cells = <0>;
>> +			};
>> +
>>  			apbclk: apbclk {
>>  				compatible = "fixed-clock";
>>  				clock-frequency = <50000000>;
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index 46869d6..620e47f 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)		+= arc/
> Two things here:
>  [1] This clock is relevant to only one board but not SoC, CPU core
>      or whole architecture, so it makes sense to make it dependent on
>      the AXS platform, i.e. CONFIG_ARC_PLAT_AXS10X.
>
>  [2] Something similar is applicable to folder name, I would suggest to
>      use "drivers/clk/axs10x"

I will rename to axs10x and use CONFIG_ARC_PLAT_AXS10X.

>> diff --git a/drivers/clk/arc/Makefile b/drivers/clk/arc/Makefile
>> new file mode 100644
>> index 0000000..01996b8
>> --- /dev/null
>> +++ b/drivers/clk/arc/Makefile
>> @@ -0,0 +1 @@
>> +obj-y += i2s_pll_clock.o
>> diff --git a/drivers/clk/arc/i2s_pll_clock.c b/drivers/clk/arc/i2s_pll_clock.c
>> new file mode 100644
>> index 0000000..8c401f1
>> --- /dev/null
>> +++ b/drivers/clk/arc/i2s_pll_clock.c
>> @@ -0,0 +1,143 @@
>> +/*
>> + * Synopsys I2S PLL clock driver
> Again that's not generic SNPS I2S clock but clock driver for a particular board.
>
>> + *
>> + * drivers/clk/arc/i2s_pll_clock.c
> BTW not sure why this path could be needed here.
> IMHO that might be safely dropped.
>
>> + *
>> + * 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/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/slab.h>
>> +
>> +/* FPGA Version Info */
>> +#define FPGA_VER_INFO	0xE0011230
>> +#define FPGA_VER_27M	0x000FBED9
> This is a little bit tricky.
> We'll need to do that check in each and every clock driver for AXS10x
> and so it worth putting this code in some common location (I mean common
> for all axs10x clock drivers).
>

This is not common to all clocks. We are expecting a new firmware release which
will change the reference clock input for the I2S PLL from 27Mhz to 28.224 Mhz.
To support both the old and the new firmware we check this version info register
so that we can determine which firmware version is being used. Due to this
change the PLL dividers are different. As the remaining clocks will be
unaffected I think this can remain only in this driver.

>> +/* PLL registers addresses */
>> +#define PLL_IDIV_ADDR	0xE00100A0
>> +#define PLL_FBDIV_ADDR	0xE00100A4
>> +#define PLL_ODIV0_ADDR	0xE00100A8
>> +#define PLL_ODIV1_ADDR	0xE00100AC
>>
>> +struct i2s_pll_clk {
>> +	struct clk_hw hw;
>> +};
>> +
>> +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 },
>> +	{ 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 },
>> +	{ 0, 0, 0, 0, 0 },
>> +};
>> +
>> +static unsigned long i2s_pll_recalc_rate(struct clk_hw *hw,
>> +			unsigned long parent_rate)
>> +{
>> +	/* TODO */
>> +	pr_info("%s: parent_rate=%ld\n", __func__, parent_rate);
>> +	return parent_rate;
>> +}
>> +
>> +static long i2s_pll_round_rate(struct clk_hw *hw, unsigned long rate,
>> +			unsigned long *prate)
>> +{
>> +	/* TODO: Round rate to nearest valid rate */
>> +	*prate = rate;
>> +	pr_info("%s: rate=%ld, prate=%ld\n", __func__, rate, *prate);
>> +	return *prate;
>> +}
> These are now just dummy functions so probably they could be dropped.

Agree but I will have to check if the clock core needs this functions to be defined.

>> +static int i2s_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>> +			unsigned long parent_rate)
>> +{
>> +	const struct i2s_pll_cfg *pll_cfg;
>> +	int i;
>> +
>> +	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M)
>> +		pll_cfg = i2s_pll_cfg_27m;
>> +	else
>> +		pll_cfg = i2s_pll_cfg_28m;
> As I mentioned above we need to check board version in common place and then
> just use some variable or structure member for comparison.
>
> Otherwise it looks pretty nice!
>
> -Alexey

Thanks for your comments!

Best regards,
Jose Miguel Abreu