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

[Alexey Brodkin]

Hi Jose,

On Tue, 2016-03-29 at 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