[PATCHv3,2/4] clk: socfpga: add a clock driver for the Arria 10 platform

Commit Message

[e-mail] May 7, 2015, 3:12 p.m. UTC

From: Dinh Nguyen <[e-mail]>

The clocks on the Arria 10 platform is a bit different than the Cyclone/Arria 5
platform that it should just have it's own driver.

Signed-off-by: Dinh Nguyen <[e-mail]>
---
v3: Assign pointer to NULL instead of integer 0 per sparse check
---
 drivers/clk/socfpga/Makefile         |   1 +
 drivers/clk/socfpga/clk-gate-a10.c   | 187 +++++++++++++++++++++++++++++++++++
 drivers/clk/socfpga/clk-periph-a10.c | 131 ++++++++++++++++++++++++
 drivers/clk/socfpga/clk-pll-a10.c    | 132 +++++++++++++++++++++++++
 drivers/clk/socfpga/clk.c            |   7 +-
 drivers/clk/socfpga/clk.h            |   4 +
 6 files changed, 461 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/socfpga/clk-gate-a10.c
 create mode 100644 drivers/clk/socfpga/clk-periph-a10.c
 create mode 100644 drivers/clk/socfpga/clk-pll-a10.c

Comments

Stephen Boyd May 16, 2015, 12:52 a.m. UTC | #1

On 05/07, [e-mail] wrote:
> diff --git a/drivers/clk/socfpga/clk-gate-a10.c b/drivers/clk/socfpga/clk-gate-a10.c
> new file mode 100644
> index 0000000..fadf6f7
> --- /dev/null
> +++ b/drivers/clk/socfpga/clk-gate-a10.c
> @@ -0,0 +1,187 @@
[...]
> +
> +static int socfpga_clk_prepare(struct clk_hw *hwclk)
> +{
> +	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
> +	struct regmap *sys_mgr_base_addr;
> +	int i;
> +	u32 hs_timing;
> +	u32 clk_phase[2];
> +
> +	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
> +		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
> +		if (IS_ERR(sys_mgr_base_addr)) {

Is there a reason the syscon is grabbed lazily in prepare? Why
not get it before registering this clock?

> +			pr_err("%s: failed to find altr,sys-mgr regmap!\n", __func__);
> +			return -EINVAL;
> +		}
> +
> +		for (i = 0; i < 2; i++) {

i < ARRAY_SIZE(clk_phase) ?

> +			switch (socfpgaclk->clk_phase[i]) {
> +			case 0:
> +				clk_phase[i] = 0;
> +				break;
> +			case 45:
> +				clk_phase[i] = 1;
> +				break;
> +			case 90:
> +				clk_phase[i] = 2;
> +				break;
> +			case 135:
> +				clk_phase[i] = 3;
> +				break;
> +			case 180:
> +				clk_phase[i] = 4;
> +				break;
> +			case 225:
> +				clk_phase[i] = 5;
> +				break;
> +			case 270:
> +				clk_phase[i] = 6;
> +				break;
> +			case 315:
> +				clk_phase[i] = 7;
> +				break;
> +			default:
> +				clk_phase[i] = 0;
> +				break;
> +			}
> +		}
> +
> +		hs_timing = SYSMGR_SDMMC_CTRL_SET(clk_phase[0], clk_phase[1]);
> +		regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET,
> +			     hs_timing);
> +	}
> +	return 0;
> +}
> +
> +static struct clk_ops gateclk_ops = {

const?

> +	.prepare = socfpga_clk_prepare,
> +	.recalc_rate = socfpga_gate_clk_recalc_rate,
> +};
> +
> diff --git a/drivers/clk/socfpga/clk-periph-a10.c b/drivers/clk/socfpga/clk-periph-a10.c
> new file mode 100644
> index 0000000..81b9274
> --- /dev/null
> +++ b/drivers/clk/socfpga/clk-periph-a10.c
> @@ -0,0 +1,131 @@
[...]
> + */
> +#include <linux/clk.h>

Are you using this include?

> +#include <linux/clkdev.h>

Are you using this include?

Applies to every file added in this patch.

> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +
> +#include "clk.h"
> +
> +#define CLK_MGR_FREE_SHIFT		16
> +#define CLK_MGR_FREE_MASK		0x7
> +
> +#define SOCFPGA_MPU_FREE_CLK		"mpu_free_clk"
> +#define SOCFPGA_NOC_FREE_CLK		"noc_free_clk"
> +#define SOCFPGA_SDMMC_FREE_CLK		"sdmmc_free_clk"
[..]
> +
> +static __init void __socfpga_periph_init(struct device_node *node,
> +	const struct clk_ops *ops)
> +{
[..]
> +	init.name = clk_name;
> +	init.ops = ops;
> +	init.flags = 0;
> +
> +	parent_name = of_clk_get_parent_name(node, 0);
> +	init.num_parents = 1;
> +	init.parent_names = &parent_name;
> +
> +	periph_clk->hw.hw.init = &init;
> +
> +	clk = clk_register(NULL, &periph_clk->hw.hw);
> +	if (WARN_ON(IS_ERR(clk))) {
> +		kfree(periph_clk);
> +		return;
> +	}
> +	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);

Why not check the return value?

> +
> diff --git a/drivers/clk/socfpga/clk-pll-a10.c b/drivers/clk/socfpga/clk-pll-a10.c
> new file mode 100644
> index 0000000..2adc2f5
> --- /dev/null
> +++ b/drivers/clk/socfpga/clk-pll-a10.c
[..]
> +
> +static u8 clk_pll_get_parent(struct clk_hw *hwclk)
> +{
> +	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
> +	u32 pll_src;
> +
> +	pll_src = readl(socfpgaclk->hw.reg);
> +
> +	return (pll_src >> CLK_MGR_PLL_CLK_SRC_SHIFT) &
> +		CLK_MGR_PLL_CLK_SRC_MASK;
> +}
> +
> +

Nitpick: Single newline please.

> +static struct clk_ops clk_pll_ops = {

const?

> +	.recalc_rate = clk_pll_recalc_rate,
> +	.get_parent = clk_pll_get_parent,
> +};
> +
> +static __init struct clk *__socfpga_pll_init(struct device_node *node,

__init goes after the return type, doesn't it?

> +	const struct clk_ops *ops)
> +{
> +	u32 reg;
> +	struct clk *clk;
> +	struct socfpga_pll *pll_clk;
> +	const char *clk_name = node->name;
> +	const char *parent_name[SOCFGPA_MAX_PARENTS];
> +	struct clk_init_data init;
> +	struct device_node *clkmgr_np;
> +	int rc;
> +	int i = 0;
> +
> +	of_property_read_u32(node, "reg", &reg);
> +
> +	pll_clk = kzalloc(sizeof(*pll_clk), GFP_KERNEL);
> +	if (WARN_ON(!pll_clk))
> +		return NULL;
> +
> +	clkmgr_np = of_find_compatible_node(NULL, NULL, "altr,clk-mgr");

I haven't looked at the binding, but I would expect there to be
some phandle to this node somewhere so that we don't have to
search the whole tree?

> +	clk_mgr_a10_base_addr = of_iomap(clkmgr_np, 0);
> +	BUG_ON(!clk_mgr_a10_base_addr);
> +	pll_clk->hw.reg = clk_mgr_a10_base_addr + reg;
> +
> +	of_property_read_string(node, "clock-output-names", &clk_name);
> +
> +	init.name = clk_name;
> +	init.ops = ops;
[..]
> diff --git a/drivers/clk/socfpga/clk.h b/drivers/clk/socfpga/clk.h
> index b09a5d5..6989847 100644
> --- a/drivers/clk/socfpga/clk.h
> +++ b/drivers/clk/socfpga/clk.h
> @@ -34,10 +34,14 @@
>  	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
>  
>  extern void __iomem *clk_mgr_base_addr;
> +extern void __iomem *clk_mgr_a10_base_addr;
>  
>  void __init socfpga_pll_init(struct device_node *node);
>  void __init socfpga_periph_init(struct device_node *node);
>  void __init socfpga_gate_init(struct device_node *node);
> +void __init socfpga_a10_pll_init(struct device_node *node);
> +void __init socfpga_a10_periph_init(struct device_node *node);
> +void __init socfpga_a10_gate_init(struct device_node *node);

__init is useless on prototypes. It's not your fault for copying
previous code, but it would be good to avoid adding more and to
clean this up in some other patch.

[e-mail] May 19, 2015, 4:29 p.m. UTC | #2

On 5/15/15 7:52 PM, Stephen Boyd wrote:
> On 05/07, [e-mail] wrote:
>> diff --git a/drivers/clk/socfpga/clk-gate-a10.c b/drivers/clk/socfpga/clk-gate-a10.c
>> new file mode 100644
>> index 0000000..fadf6f7
>> --- /dev/null
>> +++ b/drivers/clk/socfpga/clk-gate-a10.c
>> @@ -0,0 +1,187 @@
> [...]
>> +
>> +static int socfpga_clk_prepare(struct clk_hw *hwclk)
>> +{
>> +	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
>> +	struct regmap *sys_mgr_base_addr;
>> +	int i;
>> +	u32 hs_timing;
>> +	u32 clk_phase[2];
>> +
>> +	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
>> +		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
>> +		if (IS_ERR(sys_mgr_base_addr)) {
> 
> Is there a reason the syscon is grabbed lazily in prepare? Why
> not get it before registering this clock?

This syscon node is only associated with clocks that have a clk-phase
property, which on the SoCFPGA platform, is the SD/MMC clocks. The way
to implement this went through quite a few rounds of discussion for the
Cyclone5/Arria5 platform before settling to this method.

The reason why syscon is grabbed here is that the setting of the clock
phase must be done before enabling of the clock, so it seem that prepare
was a good place. Should this be move moved to the socfpga_gate_init()
instead?

> 
>> +			pr_err("%s: failed to find altr,sys-mgr regmap!\n", __func__);
>> +			return -EINVAL;
>> +		}
>> +
>> +		for (i = 0; i < 2; i++) {
> 
> i < ARRAY_SIZE(clk_phase) ?
> 

Will fix.

>> +			switch (socfpgaclk->clk_phase[i]) {
>> +			case 0:
>> +				clk_phase[i] = 0;
>> +				break;
>> +			case 45:
>> +				clk_phase[i] = 1;
>> +				break;
>> +			case 90:
>> +				clk_phase[i] = 2;
>> +				break;
>> +			case 135:
>> +				clk_phase[i] = 3;
>> +				break;
>> +			case 180:
>> +				clk_phase[i] = 4;
>> +				break;
>> +			case 225:
>> +				clk_phase[i] = 5;
>> +				break;
>> +			case 270:
>> +				clk_phase[i] = 6;
>> +				break;
>> +			case 315:
>> +				clk_phase[i] = 7;
>> +				break;
>> +			default:
>> +				clk_phase[i] = 0;
>> +				break;
>> +			}
>> +		}
>> +
>> +		hs_timing = SYSMGR_SDMMC_CTRL_SET(clk_phase[0], clk_phase[1]);
>> +		regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET,
>> +			     hs_timing);
>> +	}
>> +	return 0;
>> +}
>> +
>> +static struct clk_ops gateclk_ops = {
> 
> const?
> 

I cannot make this a const as I am assigning the .enable/.disable to use
the common clk_gate_ops.

>> +	.prepare = socfpga_clk_prepare,
>> +	.recalc_rate = socfpga_gate_clk_recalc_rate,
>> +};
>> +
>> diff --git a/drivers/clk/socfpga/clk-periph-a10.c b/drivers/clk/socfpga/clk-periph-a10.c
>> new file mode 100644
>> index 0000000..81b9274
>> --- /dev/null
>> +++ b/drivers/clk/socfpga/clk-periph-a10.c
>> @@ -0,0 +1,131 @@
> [...]
>> + */
>> +#include <linux/clk.h>
> 
> Are you using this include?
> 
>> +#include <linux/clkdev.h>
> 
> Are you using this include?
> 
> Applies to every file added in this patch.

Removed...

> 
>> +#include <linux/clk-provider.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +
>> +#include "clk.h"
>> +
>> +#define CLK_MGR_FREE_SHIFT		16
>> +#define CLK_MGR_FREE_MASK		0x7
>> +
>> +#define SOCFPGA_MPU_FREE_CLK		"mpu_free_clk"
>> +#define SOCFPGA_NOC_FREE_CLK		"noc_free_clk"
>> +#define SOCFPGA_SDMMC_FREE_CLK		"sdmmc_free_clk"
> [..]
>> +
>> +static __init void __socfpga_periph_init(struct device_node *node,
>> +	const struct clk_ops *ops)
>> +{
> [..]
>> +	init.name = clk_name;
>> +	init.ops = ops;
>> +	init.flags = 0;
>> +
>> +	parent_name = of_clk_get_parent_name(node, 0);
>> +	init.num_parents = 1;
>> +	init.parent_names = &parent_name;
>> +
>> +	periph_clk->hw.hw.init = &init;
>> +
>> +	clk = clk_register(NULL, &periph_clk->hw.hw);
>> +	if (WARN_ON(IS_ERR(clk))) {
>> +		kfree(periph_clk);
>> +		return;
>> +	}
>> +	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
> 
> Why not check the return value?

Added check...

> 
>> +
>> diff --git a/drivers/clk/socfpga/clk-pll-a10.c b/drivers/clk/socfpga/clk-pll-a10.c
>> new file mode 100644
>> index 0000000..2adc2f5
>> --- /dev/null
>> +++ b/drivers/clk/socfpga/clk-pll-a10.c
> [..]
>> +
>> +static u8 clk_pll_get_parent(struct clk_hw *hwclk)
>> +{
>> +	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
>> +	u32 pll_src;
>> +
>> +	pll_src = readl(socfpgaclk->hw.reg);
>> +
>> +	return (pll_src >> CLK_MGR_PLL_CLK_SRC_SHIFT) &
>> +		CLK_MGR_PLL_CLK_SRC_MASK;
>> +}
>> +
>> +
> 
> Nitpick: Single newline please.
> 

Cleaned up...
>> +static struct clk_ops clk_pll_ops = {
> 
> const?
> 

Same comment applies here as I'm using clk_get_ops for .enable/.disable.

>> +	.recalc_rate = clk_pll_recalc_rate,
>> +	.get_parent = clk_pll_get_parent,
>> +};
>> +
>> +static __init struct clk *__socfpga_pll_init(struct device_node *node,
> 
> __init goes after the return type, doesn't it?
> 
>> +	const struct clk_ops *ops)
>> +{
>> +	u32 reg;
>> +	struct clk *clk;
>> +	struct socfpga_pll *pll_clk;
>> +	const char *clk_name = node->name;
>> +	const char *parent_name[SOCFGPA_MAX_PARENTS];
>> +	struct clk_init_data init;
>> +	struct device_node *clkmgr_np;
>> +	int rc;
>> +	int i = 0;
>> +
>> +	of_property_read_u32(node, "reg", &reg);
>> +
>> +	pll_clk = kzalloc(sizeof(*pll_clk), GFP_KERNEL);
>> +	if (WARN_ON(!pll_clk))
>> +		return NULL;
>> +
>> +	clkmgr_np = of_find_compatible_node(NULL, NULL, "altr,clk-mgr");
> 
> I haven't looked at the binding, but I would expect there to be
> some phandle to this node somewhere so that we don't have to
> search the whole tree?
> 

I'm putting all of the clocks under the clk-mgr node as much of this
clock driver was derived from clk-highbank.

>> +	clk_mgr_a10_base_addr = of_iomap(clkmgr_np, 0);
>> +	BUG_ON(!clk_mgr_a10_base_addr);
>> +	pll_clk->hw.reg = clk_mgr_a10_base_addr + reg;
>> +
>> +	of_property_read_string(node, "clock-output-names", &clk_name);
>> +
>> +	init.name = clk_name;
>> +	init.ops = ops;
> [..]
>> diff --git a/drivers/clk/socfpga/clk.h b/drivers/clk/socfpga/clk.h
>> index b09a5d5..6989847 100644
>> --- a/drivers/clk/socfpga/clk.h
>> +++ b/drivers/clk/socfpga/clk.h
>> @@ -34,10 +34,14 @@
>>  	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
>>  
>>  extern void __iomem *clk_mgr_base_addr;
>> +extern void __iomem *clk_mgr_a10_base_addr;
>>  
>>  void __init socfpga_pll_init(struct device_node *node);
>>  void __init socfpga_periph_init(struct device_node *node);
>>  void __init socfpga_gate_init(struct device_node *node);
>> +void __init socfpga_a10_pll_init(struct device_node *node);
>> +void __init socfpga_a10_periph_init(struct device_node *node);
>> +void __init socfpga_a10_gate_init(struct device_node *node);
> 
> __init is useless on prototypes. It's not your fault for copying
> previous code, but it would be good to avoid adding more and to
> clean this up in some other patch.
> 

Will clean up.

Thanks for reviewing this patch.

Dinh

Stephen Boyd May 19, 2015, 9:50 p.m. UTC | #3

On 05/19/15 09:29, Dinh Nguyen wrote:
>
> On 5/15/15 7:52 PM, Stephen Boyd wrote:
>> On 05/07, [e-mail] wrote:
>>> +
>>> +static int socfpga_clk_prepare(struct clk_hw *hwclk)
>>> +{
>>> +	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
>>> +	struct regmap *sys_mgr_base_addr;
>>> +	int i;
>>> +	u32 hs_timing;
>>> +	u32 clk_phase[2];
>>> +
>>> +	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
>>> +		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
>>> +		if (IS_ERR(sys_mgr_base_addr)) {
>> Is there a reason the syscon is grabbed lazily in prepare? Why
>> not get it before registering this clock?
> This syscon node is only associated with clocks that have a clk-phase
> property, which on the SoCFPGA platform, is the SD/MMC clocks. The way
> to implement this went through quite a few rounds of discussion for the
> Cyclone5/Arria5 platform before settling to this method.
>
> The reason why syscon is grabbed here is that the setting of the clock
> phase must be done before enabling of the clock, so it seem that prepare
> was a good place. Should this be move moved to the socfpga_gate_init()
> instead?

I was expecting the regmap to be found before the clock is registered
and stored away into the  socfpga_gate_clk structure. Getting the regmap
during prepare is akin to ioremapping a register region during prepare,
which doesn't sound right at all. Maybe there's some good reason in the
earlier discussions? Any hints?

>>> +			switch (socfpgaclk->clk_phase[i]) {
>>> +			case 0:
>>> +				clk_phase[i] = 0;
>>> +				break;
>>> +			case 45:
>>> +				clk_phase[i] = 1;
>>> +				break;
>>> +			case 90:
>>> +				clk_phase[i] = 2;
>>> +				break;
>>> +			case 135:
>>> +				clk_phase[i] = 3;
>>> +				break;
>>> +			case 180:
>>> +				clk_phase[i] = 4;
>>> +				break;
>>> +			case 225:
>>> +				clk_phase[i] = 5;
>>> +				break;
>>> +			case 270:
>>> +				clk_phase[i] = 6;
>>> +				break;
>>> +			case 315:
>>> +				clk_phase[i] = 7;
>>> +				break;
>>> +			default:
>>> +				clk_phase[i] = 0;
>>> +				break;
>>> +			}
>>> +		}
>>> +
>>> +		hs_timing = SYSMGR_SDMMC_CTRL_SET(clk_phase[0], clk_phase[1]);
>>> +		regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET,
>>> +			     hs_timing);
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>> +static struct clk_ops gateclk_ops = {
>> const?
>>
> I cannot make this a const as I am assigning the .enable/.disable to use
> the common clk_gate_ops.
>
>

Hm.. ok. Maybe we should export those functions to modules.

[e-mail] May 19, 2015, 11:12 p.m. UTC | #4

On 5/19/15 4:50 PM, Stephen Boyd wrote:
> On 05/19/15 09:29, Dinh Nguyen wrote:
>>
>> On 5/15/15 7:52 PM, Stephen Boyd wrote:
>>> On 05/07, [e-mail] wrote:
>>>> +
>>>> +static int socfpga_clk_prepare(struct clk_hw *hwclk)
>>>> +{
>>>> +	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
>>>> +	struct regmap *sys_mgr_base_addr;
>>>> +	int i;
>>>> +	u32 hs_timing;
>>>> +	u32 clk_phase[2];
>>>> +
>>>> +	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
>>>> +		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
>>>> +		if (IS_ERR(sys_mgr_base_addr)) {
>>> Is there a reason the syscon is grabbed lazily in prepare? Why
>>> not get it before registering this clock?
>> This syscon node is only associated with clocks that have a clk-phase
>> property, which on the SoCFPGA platform, is the SD/MMC clocks. The way
>> to implement this went through quite a few rounds of discussion for the
>> Cyclone5/Arria5 platform before settling to this method.
>>
>> The reason why syscon is grabbed here is that the setting of the clock
>> phase must be done before enabling of the clock, so it seem that prepare
>> was a good place. Should this be move moved to the socfpga_gate_init()
>> instead?
> 
> I was expecting the regmap to be found before the clock is registered
> and stored away into the  socfpga_gate_clk structure. Getting the regmap
> during prepare is akin to ioremapping a register region during prepare,
> which doesn't sound right at all. Maybe there's some good reason in the
> earlier discussions? Any hints?
> 

Ah okay, the earlier discussions revolve mainly around moving the regmap
from the SD/MMC driver into the clock driver. But there weren't any
issue raised for putting the regmap in the prepare function.

If you're curious, here are the links to the discussion for adding the
clk-phase to the driver:

http://archive.arm.linux.org.uk/lurker/message/20131212.203042.d37c8ee9.en.html

http://archive.arm.linux.org.uk/lurker/message/20140109.213116.1f13b27a.en.html

But perhaps putting the regmap lookup in the init function is the
correct way to do this?

Thanks,
Dinh

Stephen Boyd May 20, 2015, 12:22 a.m. UTC | #5

On 05/19/15 16:12, Dinh Nguyen wrote:
>
> On 5/19/15 4:50 PM, Stephen Boyd wrote:
>> On 05/19/15 09:29, Dinh Nguyen wrote:
>>> On 5/15/15 7:52 PM, Stephen Boyd wrote:
>>>> On 05/07, [e-mail] wrote:
>>>>> +
>>>>> +static int socfpga_clk_prepare(struct clk_hw *hwclk)
>>>>> +{
>>>>> +	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
>>>>> +	struct regmap *sys_mgr_base_addr;
>>>>> +	int i;
>>>>> +	u32 hs_timing;
>>>>> +	u32 clk_phase[2];
>>>>> +
>>>>> +	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
>>>>> +		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
>>>>> +		if (IS_ERR(sys_mgr_base_addr)) {
>>>> Is there a reason the syscon is grabbed lazily in prepare? Why
>>>> not get it before registering this clock?
>>> This syscon node is only associated with clocks that have a clk-phase
>>> property, which on the SoCFPGA platform, is the SD/MMC clocks. The way
>>> to implement this went through quite a few rounds of discussion for the
>>> Cyclone5/Arria5 platform before settling to this method.
>>>
>>> The reason why syscon is grabbed here is that the setting of the clock
>>> phase must be done before enabling of the clock, so it seem that prepare
>>> was a good place. Should this be move moved to the socfpga_gate_init()
>>> instead?
>> I was expecting the regmap to be found before the clock is registered
>> and stored away into the  socfpga_gate_clk structure. Getting the regmap
>> during prepare is akin to ioremapping a register region during prepare,
>> which doesn't sound right at all. Maybe there's some good reason in the
>> earlier discussions? Any hints?
>>
> Ah okay, the earlier discussions revolve mainly around moving the regmap
> from the SD/MMC driver into the clock driver. But there weren't any
> issue raised for putting the regmap in the prepare function.
>
> If you're curious, here are the links to the discussion for adding the
> clk-phase to the driver:
>
> http://archive.arm.linux.org.uk/lurker/message/20131212.203042.d37c8ee9.en.html
>
> http://archive.arm.linux.org.uk/lurker/message/20140109.213116.1f13b27a.en.html
>
> But perhaps putting the regmap lookup in the init function is the
> correct way to do this?

Yes that would seem more appropriate. I suspect this lazy approach is
done because syscon isn't ready when of_clk_init() runs though. If this
was written to be a proper device driver with probe defer support this
wouldn't be a problem.

Patch

diff --git a/drivers/clk/socfpga/Makefile b/drivers/clk/socfpga/Makefile
index 7e2d15a..d8bb239 100644
--- a/drivers/clk/socfpga/Makefile
+++ b/drivers/clk/socfpga/Makefile
@@ -2,3 +2,4 @@  obj-y += clk.o
 obj-y += clk-gate.o
 obj-y += clk-pll.o
 obj-y += clk-periph.o
+obj-y += clk-pll-a10.o clk-periph-a10.o clk-gate-a10.o
diff --git a/drivers/clk/socfpga/clk-gate-a10.c b/drivers/clk/socfpga/clk-gate-a10.c
new file mode 100644
index 0000000..fadf6f7
--- /dev/null
+++ b/drivers/clk/socfpga/clk-gate-a10.c
@@ -0,0 +1,187 @@ 
+/*
+ * Copyright (C) 2015 Altera Corporation. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+
+#include "clk.h"
+
+#define streq(a, b) (strcmp((a), (b)) == 0)
+
+#define to_socfpga_gate_clk(p) container_of(p, struct socfpga_gate_clk, hw.hw)
+
+/* SDMMC Group for System Manager defines */
+#define SYSMGR_SDMMCGRP_CTRL_OFFSET	0x28
+
+static unsigned long socfpga_gate_clk_recalc_rate(struct clk_hw *hwclk,
+	unsigned long parent_rate)
+{
+	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
+	u32 div = 1, val;
+
+	if (socfpgaclk->fixed_div)
+		div = socfpgaclk->fixed_div;
+	else if (socfpgaclk->div_reg) {
+		val = readl(socfpgaclk->div_reg) >> socfpgaclk->shift;
+		val &= div_mask(socfpgaclk->width);
+			div = (1 << val);
+	}
+
+	return parent_rate / div;
+}
+
+static int socfpga_clk_prepare(struct clk_hw *hwclk)
+{
+	struct socfpga_gate_clk *socfpgaclk = to_socfpga_gate_clk(hwclk);
+	struct regmap *sys_mgr_base_addr;
+	int i;
+	u32 hs_timing;
+	u32 clk_phase[2];
+
+	if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) {
+		sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
+		if (IS_ERR(sys_mgr_base_addr)) {
+			pr_err("%s: failed to find altr,sys-mgr regmap!\n", __func__);
+			return -EINVAL;
+		}
+
+		for (i = 0; i < 2; i++) {
+			switch (socfpgaclk->clk_phase[i]) {
+			case 0:
+				clk_phase[i] = 0;
+				break;
+			case 45:
+				clk_phase[i] = 1;
+				break;
+			case 90:
+				clk_phase[i] = 2;
+				break;
+			case 135:
+				clk_phase[i] = 3;
+				break;
+			case 180:
+				clk_phase[i] = 4;
+				break;
+			case 225:
+				clk_phase[i] = 5;
+				break;
+			case 270:
+				clk_phase[i] = 6;
+				break;
+			case 315:
+				clk_phase[i] = 7;
+				break;
+			default:
+				clk_phase[i] = 0;
+				break;
+			}
+		}
+
+		hs_timing = SYSMGR_SDMMC_CTRL_SET(clk_phase[0], clk_phase[1]);
+		regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET,
+			     hs_timing);
+	}
+	return 0;
+}
+
+static struct clk_ops gateclk_ops = {
+	.prepare = socfpga_clk_prepare,
+	.recalc_rate = socfpga_gate_clk_recalc_rate,
+};
+
+static void __init __socfpga_gate_init(struct device_node *node,
+	const struct clk_ops *ops)
+{
+	u32 clk_gate[2];
+	u32 div_reg[3];
+	u32 clk_phase[2];
+	u32 fixed_div;
+	struct clk *clk;
+	struct socfpga_gate_clk *socfpga_clk;
+	const char *clk_name = node->name;
+	const char *parent_name[SOCFPGA_MAX_PARENTS];
+	struct clk_init_data init;
+	int rc;
+	int i = 0;
+
+	socfpga_clk = kzalloc(sizeof(*socfpga_clk), GFP_KERNEL);
+	if (WARN_ON(!socfpga_clk))
+		return;
+
+	rc = of_property_read_u32_array(node, "clk-gate", clk_gate, 2);
+	if (rc)
+		clk_gate[0] = 0;
+
+	if (clk_gate[0]) {
+		socfpga_clk->hw.reg = clk_mgr_a10_base_addr + clk_gate[0];
+		socfpga_clk->hw.bit_idx = clk_gate[1];
+
+		gateclk_ops.enable = clk_gate_ops.enable;
+		gateclk_ops.disable = clk_gate_ops.disable;
+	}
+
+	rc = of_property_read_u32(node, "fixed-divider", &fixed_div);
+	if (rc)
+		socfpga_clk->fixed_div = 0;
+	else
+		socfpga_clk->fixed_div = fixed_div;
+
+	rc = of_property_read_u32_array(node, "div-reg", div_reg, 3);
+	if (!rc) {
+		socfpga_clk->div_reg = clk_mgr_a10_base_addr + div_reg[0];
+		socfpga_clk->shift = div_reg[1];
+		socfpga_clk->width = div_reg[2];
+	} else {
+		socfpga_clk->div_reg = NULL;
+	}
+
+	rc = of_property_read_u32_array(node, "clk-phase", clk_phase, 2);
+	if (!rc) {
+		socfpga_clk->clk_phase[0] = clk_phase[0];
+		socfpga_clk->clk_phase[1] = clk_phase[1];
+	}
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = 0;
+	while (i < SOCFPGA_MAX_PARENTS && (parent_name[i] =
+			of_clk_get_parent_name(node, i)) != NULL)
+		i++;
+
+	init.parent_names = parent_name;
+	init.num_parents = i;
+	socfpga_clk->hw.hw.init = &init;
+
+	clk = clk_register(NULL, &socfpga_clk->hw.hw);
+	if (WARN_ON(IS_ERR(clk))) {
+		kfree(socfpga_clk);
+		return;
+	}
+	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	if (WARN_ON(rc))
+		return;
+}
+
+void __init socfpga_a10_gate_init(struct device_node *node)
+{
+	__socfpga_gate_init(node, &gateclk_ops);
+}
diff --git a/drivers/clk/socfpga/clk-periph-a10.c b/drivers/clk/socfpga/clk-periph-a10.c
new file mode 100644
index 0000000..81b9274
--- /dev/null
+++ b/drivers/clk/socfpga/clk-periph-a10.c
@@ -0,0 +1,131 @@ 
+/*
+ * Copyright (C) 2015 Altera Corporation. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/of.h>
+
+#include "clk.h"
+
+#define CLK_MGR_FREE_SHIFT		16
+#define CLK_MGR_FREE_MASK		0x7
+
+#define SOCFPGA_MPU_FREE_CLK		"mpu_free_clk"
+#define SOCFPGA_NOC_FREE_CLK		"noc_free_clk"
+#define SOCFPGA_SDMMC_FREE_CLK		"sdmmc_free_clk"
+#define to_socfpga_periph_clk(p) container_of(p, struct socfpga_periph_clk, hw.hw)
+
+static unsigned long clk_periclk_recalc_rate(struct clk_hw *hwclk,
+					     unsigned long parent_rate)
+{
+	struct socfpga_periph_clk *socfpgaclk = to_socfpga_periph_clk(hwclk);
+	u32 div;
+
+	if (socfpgaclk->fixed_div) {
+		div = socfpgaclk->fixed_div;
+	} else if (socfpgaclk->div_reg) {
+		div = readl(socfpgaclk->div_reg) >> socfpgaclk->shift;
+		div &= div_mask(socfpgaclk->width);
+		div += 1;
+	} else {
+		div = ((readl(socfpgaclk->hw.reg) & 0x7ff) + 1);
+	}
+
+	return parent_rate / div;
+}
+
+static u8 clk_periclk_get_parent(struct clk_hw *hwclk)
+{
+	struct socfpga_periph_clk *socfpgaclk = to_socfpga_periph_clk(hwclk);
+	u32 clk_src;
+
+	clk_src = readl(socfpgaclk->hw.reg);
+	if (streq(hwclk->init->name, SOCFPGA_MPU_FREE_CLK) ||
+	    streq(hwclk->init->name, SOCFPGA_NOC_FREE_CLK) ||
+	    streq(hwclk->init->name, SOCFPGA_SDMMC_FREE_CLK))
+		return (clk_src >> CLK_MGR_FREE_SHIFT) &
+			CLK_MGR_FREE_MASK;
+	else
+		return 0;
+}
+
+static const struct clk_ops periclk_ops = {
+	.recalc_rate = clk_periclk_recalc_rate,
+	.get_parent = clk_periclk_get_parent,
+};
+
+static __init void __socfpga_periph_init(struct device_node *node,
+	const struct clk_ops *ops)
+{
+	u32 reg;
+	struct clk *clk;
+	struct socfpga_periph_clk *periph_clk;
+	const char *clk_name = node->name;
+	const char *parent_name;
+	struct clk_init_data init;
+	int rc;
+	u32 fixed_div;
+	u32 div_reg[3];
+
+	of_property_read_u32(node, "reg", &reg);
+
+	periph_clk = kzalloc(sizeof(*periph_clk), GFP_KERNEL);
+	if (WARN_ON(!periph_clk))
+		return;
+
+	periph_clk->hw.reg = clk_mgr_a10_base_addr + reg;
+
+	rc = of_property_read_u32_array(node, "div-reg", div_reg, 3);
+	if (!rc) {
+		periph_clk->div_reg = clk_mgr_a10_base_addr + div_reg[0];
+		periph_clk->shift = div_reg[1];
+		periph_clk->width = div_reg[2];
+	} else {
+		periph_clk->div_reg = NULL;
+	}
+
+	rc = of_property_read_u32(node, "fixed-divider", &fixed_div);
+	if (rc)
+		periph_clk->fixed_div = 0;
+	else
+		periph_clk->fixed_div = fixed_div;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = 0;
+
+	parent_name = of_clk_get_parent_name(node, 0);
+	init.num_parents = 1;
+	init.parent_names = &parent_name;
+
+	periph_clk->hw.hw.init = &init;
+
+	clk = clk_register(NULL, &periph_clk->hw.hw);
+	if (WARN_ON(IS_ERR(clk))) {
+		kfree(periph_clk);
+		return;
+	}
+	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+
+}
+
+void __init socfpga_a10_periph_init(struct device_node *node)
+{
+	__socfpga_periph_init(node, &periclk_ops);
+}
diff --git a/drivers/clk/socfpga/clk-pll-a10.c b/drivers/clk/socfpga/clk-pll-a10.c
new file mode 100644
index 0000000..2adc2f5
--- /dev/null
+++ b/drivers/clk/socfpga/clk-pll-a10.c
@@ -0,0 +1,132 @@ 
+/*
+ * Copyright (C) 2015 Altera Corporation. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/clk.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#include "clk.h"
+
+/* Clock Manager offsets */
+#define CLK_MGR_PLL_CLK_SRC_SHIFT	8
+#define CLK_MGR_PLL_CLK_SRC_MASK	0x3
+
+/* Clock bypass bits */
+#define SOCFPGA_PLL_BG_PWRDWN		0
+#define SOCFPGA_PLL_PWR_DOWN		1
+#define SOCFPGA_PLL_EXT_ENA		2
+#define SOCFPGA_PLL_DIVF_MASK		0x00001FFF
+#define SOCFPGA_PLL_DIVF_SHIFT	0
+#define SOCFPGA_PLL_DIVQ_MASK		0x003F0000
+#define SOCFPGA_PLL_DIVQ_SHIFT	16
+#define SOCFGPA_MAX_PARENTS	5
+
+#define SOCFPGA_MAIN_PLL_CLK		"main_pll"
+#define SOCFPGA_PERIP_PLL_CLK		"periph_pll"
+
+#define to_socfpga_clk(p) container_of(p, struct socfpga_pll, hw.hw)
+
+void __iomem *clk_mgr_a10_base_addr;
+
+static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
+					 unsigned long parent_rate)
+{
+	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
+	unsigned long divf, divq, reg;
+	unsigned long long vco_freq;
+
+	/* read VCO1 reg for numerator and denominator */
+	reg = readl(socfpgaclk->hw.reg + 0x4);
+	divf = (reg & SOCFPGA_PLL_DIVF_MASK) >> SOCFPGA_PLL_DIVF_SHIFT;
+	divq = (reg & SOCFPGA_PLL_DIVQ_MASK) >> SOCFPGA_PLL_DIVQ_SHIFT;
+	vco_freq = (unsigned long long)parent_rate * (divf + 1);
+	do_div(vco_freq, (1 + divq));
+	return (unsigned long)vco_freq;
+}
+
+static u8 clk_pll_get_parent(struct clk_hw *hwclk)
+{
+	struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
+	u32 pll_src;
+
+	pll_src = readl(socfpgaclk->hw.reg);
+
+	return (pll_src >> CLK_MGR_PLL_CLK_SRC_SHIFT) &
+		CLK_MGR_PLL_CLK_SRC_MASK;
+}
+
+
+static struct clk_ops clk_pll_ops = {
+	.recalc_rate = clk_pll_recalc_rate,
+	.get_parent = clk_pll_get_parent,
+};
+
+static __init struct clk *__socfpga_pll_init(struct device_node *node,
+	const struct clk_ops *ops)
+{
+	u32 reg;
+	struct clk *clk;
+	struct socfpga_pll *pll_clk;
+	const char *clk_name = node->name;
+	const char *parent_name[SOCFGPA_MAX_PARENTS];
+	struct clk_init_data init;
+	struct device_node *clkmgr_np;
+	int rc;
+	int i = 0;
+
+	of_property_read_u32(node, "reg", &reg);
+
+	pll_clk = kzalloc(sizeof(*pll_clk), GFP_KERNEL);
+	if (WARN_ON(!pll_clk))
+		return NULL;
+
+	clkmgr_np = of_find_compatible_node(NULL, NULL, "altr,clk-mgr");
+	clk_mgr_a10_base_addr = of_iomap(clkmgr_np, 0);
+	BUG_ON(!clk_mgr_a10_base_addr);
+	pll_clk->hw.reg = clk_mgr_a10_base_addr + reg;
+
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = 0;
+
+	while (i < SOCFGPA_MAX_PARENTS && (parent_name[i] =
+			of_clk_get_parent_name(node, i)) != NULL)
+		i++;
+	init.num_parents = i;
+	init.parent_names = parent_name;
+	pll_clk->hw.hw.init = &init;
+
+	pll_clk->hw.bit_idx = SOCFPGA_PLL_EXT_ENA;
+	clk_pll_ops.enable = clk_gate_ops.enable;
+	clk_pll_ops.disable = clk_gate_ops.disable;
+
+	clk = clk_register(NULL, &pll_clk->hw.hw);
+	if (WARN_ON(IS_ERR(clk))) {
+		kfree(pll_clk);
+		return NULL;
+	}
+	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	return clk;
+}
+
+void __init socfpga_a10_pll_init(struct device_node *node)
+{
+	__socfpga_pll_init(node, &clk_pll_ops);
+}
diff --git a/drivers/clk/socfpga/clk.c b/drivers/clk/socfpga/clk.c
index 43db947..7564d2e 100644
--- a/drivers/clk/socfpga/clk.c
+++ b/drivers/clk/socfpga/clk.c
@@ -24,4 +24,9 @@ 
 CLK_OF_DECLARE(socfpga_pll_clk, "altr,socfpga-pll-clock", socfpga_pll_init);
 CLK_OF_DECLARE(socfpga_perip_clk, "altr,socfpga-perip-clk", socfpga_periph_init);
 CLK_OF_DECLARE(socfpga_gate_clk, "altr,socfpga-gate-clk", socfpga_gate_init);
-
+CLK_OF_DECLARE(socfpga_a10_pll_clk, "altr,socfpga-a10-pll-clock",
+	       socfpga_a10_pll_init);
+CLK_OF_DECLARE(socfpga_a10_perip_clk, "altr,socfpga-a10-perip-clk",
+	       socfpga_a10_periph_init);
+CLK_OF_DECLARE(socfpga_a10_gate_clk, "altr,socfpga-a10-gate-clk",
+	       socfpga_a10_gate_init);
diff --git a/drivers/clk/socfpga/clk.h b/drivers/clk/socfpga/clk.h
index b09a5d5..6989847 100644
--- a/drivers/clk/socfpga/clk.h
+++ b/drivers/clk/socfpga/clk.h
@@ -34,10 +34,14 @@ 
 	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
 
 extern void __iomem *clk_mgr_base_addr;
+extern void __iomem *clk_mgr_a10_base_addr;
 
 void __init socfpga_pll_init(struct device_node *node);
 void __init socfpga_periph_init(struct device_node *node);
 void __init socfpga_gate_init(struct device_node *node);
+void __init socfpga_a10_pll_init(struct device_node *node);
+void __init socfpga_a10_periph_init(struct device_node *node);
+void __init socfpga_a10_gate_init(struct device_node *node);
 
 struct socfpga_pll {
 	struct clk_gate	hw;