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 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.
> 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
