Re: [PATCH V1 2/9] clk: sprd: Add common infrastructure

[Chunyan Zhang]

On 20 June 2017 at 09:29, Stephen Boyd  wrote:
> On 06/18, Chunyan Zhang wrote:
>> Added Spreadtrum's clock driver common structure and registration code.
>>
>> Signed-off-by: Chunyan Zhang 
>> ---
>>  drivers/clk/Makefile  |  1 +
>>  drivers/clk/sprd/Makefile |  3 ++
>>  drivers/clk/sprd/ccu_common.c | 78 +
>>  drivers/clk/sprd/ccu_common.h | 90 
>> +++
>>  4 files changed, 172 insertions(+)
>>  create mode 100644 drivers/clk/sprd/Makefile
>>  create mode 100644 drivers/clk/sprd/ccu_common.c
>>  create mode 100644 drivers/clk/sprd/ccu_common.h
>>
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index c19983a..1d62721 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -81,6 +81,7 @@ obj-$(CONFIG_COMMON_CLK_SAMSUNG)+= samsung/
>>  obj-$(CONFIG_ARCH_SIRF)  += sirf/
>>  obj-$(CONFIG_ARCH_SOCFPGA)   += socfpga/
>>  obj-$(CONFIG_PLAT_SPEAR) += spear/
>> +obj-$(CONFIG_ARCH_SPRD)  += sprd/
>>  obj-$(CONFIG_ARCH_STI)   += st/
>>  obj-$(CONFIG_ARCH_SUNXI) += sunxi/
>>  obj-$(CONFIG_ARCH_SUNXI) += sunxi-ng/
>> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
>> new file mode 100644
>> index 000..8f802b2
>> --- /dev/null
>> +++ b/drivers/clk/sprd/Makefile
>> @@ -0,0 +1,3 @@
>> +ifneq ($(CONFIG_OF),)
>> +obj-y+= ccu_common.o
>> +endif
>
> I'd prefer a Kconfig for SPRD clk drivers instead of this
> CONFIG_OF check. Then we can compile test the sprd code in
> configurations that don't have CONFIG_ARCH_SPRD set too.

Ok, make sense, will address this in the next version.

>
>> diff --git a/drivers/clk/sprd/ccu_common.c b/drivers/clk/sprd/ccu_common.c
>> new file mode 100644
>> index 000..911f4ba
>> --- /dev/null
>> +++ b/drivers/clk/sprd/ccu_common.c
>> @@ -0,0 +1,78 @@
>> +/*
>> + * Spreadtrum clock infrastructure
>> + *
>> + * Copyright (C) 2017 Spreadtrum, Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> +#include "ccu_common.h"
>> +
>> +static inline void __iomem *ccu_find_base(struct ccu_addr_map *maps,
>> +   unsigned int num, unsigned int reg)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < num; i++)
>> + if ((reg & 0x) == maps[i].phy)
>
> What is this?

You can look at ccu-sc9860.c, different from sunxi-ng, we specify the
whole register address rather than register offset when initializing
Spreadtrum clocks, the high 16 bits is the base address which is
configured in DT, and is mapped in the board clock files like
ccu-sc9860.c, I will write more in commit message to explain this when
cooking next version of the patchset.

>
>> + return maps[i].virt;
>> +
>> + return 0;
>> +}
>> +
>> +int sprd_ccu_probe(struct device_node *node, struct ccu_addr_map *maps,
>> +unsigned int count, const struct sprd_ccu_desc *desc)
>> +{
>> + int i, ret = 0;
>> + struct ccu_common *cclk;
>> + struct clk_hw *hw;
>> +
>> + for (i = 0; i < desc->num_ccu_clks; i++) {
>> + cclk = desc->ccu_clks[i];
>> + if (!cclk)
>> + continue;
>> +
>> + cclk->base = ccu_find_base(maps, count, cclk->reg);
>> + if (!cclk->base) {
>> + pr_err("%s: No mapped address found for clock(0x%x)\n",
>> + __func__, cclk->reg);
>> + return -EINVAL;
>> + }
>> + cclk->reg = cclk->reg & 0x;
>> + }
>> +
>> + for (i = 0; i < desc->hw_clks->num; i++) {
>> +
>> + hw = desc->hw_clks->hws[i];
>> +
>> + if (!hw)
>> + continue;
>> +
>> + ret = clk_hw_register(NULL, hw);
>> + if (ret) {
>> + pr_err("Couldn't register clock %d - %s\n",
>> +i, hw->init->name);
>> + goto err_clk_unreg;
>> + }
>> + }
>> +
>> + ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
>> +  desc->hw_clks);
>> + if (ret) {
>> + pr_err("Failed to add clock provider.\n");
>> + goto err_clk_unreg;
>> + }
>> +
>> + return 0;
>> +
>> +err_clk_unreg:
>> + while (--i >= 0) {
>> + hw = desc->hw_clks->hws[i];
>> + if (!hw)
>> + continue;
>> +
>> + clk_hw_unregister(hw);
>> + }
>> +
>> + return ret;
>> +}
>> diff --git a/drivers/clk/sprd/ccu_common.h b/drivers/clk/sprd/ccu_common.h
>> new file mode 100644
>> index 000..ff07772
>> --- /dev/null
>> +++ b/drivers/clk/sprd/ccu_common.h
>> @@ -0,0 +1,90 @@
>> +/*
>> + * Spreadtrum clock infrastructure
>> + *
>> + * Copyright (C) 2017 Spreadtrum, Inc.
>> + *
>> + 

Re: [PATCH V1 2/9] clk: sprd: Add common infrastructure

[Chunyan Zhang]

On 20 June 2017 at 09:29, Stephen Boyd  wrote:
> On 06/18, Chunyan Zhang wrote:
>> Added Spreadtrum's clock driver common structure and registration code.
>>
>> Signed-off-by: Chunyan Zhang 
>> ---
>>  drivers/clk/Makefile  |  1 +
>>  drivers/clk/sprd/Makefile |  3 ++
>>  drivers/clk/sprd/ccu_common.c | 78 +
>>  drivers/clk/sprd/ccu_common.h | 90 
>> +++
>>  4 files changed, 172 insertions(+)
>>  create mode 100644 drivers/clk/sprd/Makefile
>>  create mode 100644 drivers/clk/sprd/ccu_common.c
>>  create mode 100644 drivers/clk/sprd/ccu_common.h
>>
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index c19983a..1d62721 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -81,6 +81,7 @@ obj-$(CONFIG_COMMON_CLK_SAMSUNG)+= samsung/
>>  obj-$(CONFIG_ARCH_SIRF)  += sirf/
>>  obj-$(CONFIG_ARCH_SOCFPGA)   += socfpga/
>>  obj-$(CONFIG_PLAT_SPEAR) += spear/
>> +obj-$(CONFIG_ARCH_SPRD)  += sprd/
>>  obj-$(CONFIG_ARCH_STI)   += st/
>>  obj-$(CONFIG_ARCH_SUNXI) += sunxi/
>>  obj-$(CONFIG_ARCH_SUNXI) += sunxi-ng/
>> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
>> new file mode 100644
>> index 000..8f802b2
>> --- /dev/null
>> +++ b/drivers/clk/sprd/Makefile
>> @@ -0,0 +1,3 @@
>> +ifneq ($(CONFIG_OF),)
>> +obj-y+= ccu_common.o
>> +endif
>
> I'd prefer a Kconfig for SPRD clk drivers instead of this
> CONFIG_OF check. Then we can compile test the sprd code in
> configurations that don't have CONFIG_ARCH_SPRD set too.

Ok, make sense, will address this in the next version.

>
>> diff --git a/drivers/clk/sprd/ccu_common.c b/drivers/clk/sprd/ccu_common.c
>> new file mode 100644
>> index 000..911f4ba
>> --- /dev/null
>> +++ b/drivers/clk/sprd/ccu_common.c
>> @@ -0,0 +1,78 @@
>> +/*
>> + * Spreadtrum clock infrastructure
>> + *
>> + * Copyright (C) 2017 Spreadtrum, Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> +#include "ccu_common.h"
>> +
>> +static inline void __iomem *ccu_find_base(struct ccu_addr_map *maps,
>> +   unsigned int num, unsigned int reg)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < num; i++)
>> + if ((reg & 0x) == maps[i].phy)
>
> What is this?

You can look at ccu-sc9860.c, different from sunxi-ng, we specify the
whole register address rather than register offset when initializing
Spreadtrum clocks, the high 16 bits is the base address which is
configured in DT, and is mapped in the board clock files like
ccu-sc9860.c, I will write more in commit message to explain this when
cooking next version of the patchset.

>
>> + return maps[i].virt;
>> +
>> + return 0;
>> +}
>> +
>> +int sprd_ccu_probe(struct device_node *node, struct ccu_addr_map *maps,
>> +unsigned int count, const struct sprd_ccu_desc *desc)
>> +{
>> + int i, ret = 0;
>> + struct ccu_common *cclk;
>> + struct clk_hw *hw;
>> +
>> + for (i = 0; i < desc->num_ccu_clks; i++) {
>> + cclk = desc->ccu_clks[i];
>> + if (!cclk)
>> + continue;
>> +
>> + cclk->base = ccu_find_base(maps, count, cclk->reg);
>> + if (!cclk->base) {
>> + pr_err("%s: No mapped address found for clock(0x%x)\n",
>> + __func__, cclk->reg);
>> + return -EINVAL;
>> + }
>> + cclk->reg = cclk->reg & 0x;
>> + }
>> +
>> + for (i = 0; i < desc->hw_clks->num; i++) {
>> +
>> + hw = desc->hw_clks->hws[i];
>> +
>> + if (!hw)
>> + continue;
>> +
>> + ret = clk_hw_register(NULL, hw);
>> + if (ret) {
>> + pr_err("Couldn't register clock %d - %s\n",
>> +i, hw->init->name);
>> + goto err_clk_unreg;
>> + }
>> + }
>> +
>> + ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
>> +  desc->hw_clks);
>> + if (ret) {
>> + pr_err("Failed to add clock provider.\n");
>> + goto err_clk_unreg;
>> + }
>> +
>> + return 0;
>> +
>> +err_clk_unreg:
>> + while (--i >= 0) {
>> + hw = desc->hw_clks->hws[i];
>> + if (!hw)
>> + continue;
>> +
>> + clk_hw_unregister(hw);
>> + }
>> +
>> + return ret;
>> +}
>> diff --git a/drivers/clk/sprd/ccu_common.h b/drivers/clk/sprd/ccu_common.h
>> new file mode 100644
>> index 000..ff07772
>> --- /dev/null
>> +++ b/drivers/clk/sprd/ccu_common.h
>> @@ -0,0 +1,90 @@
>> +/*
>> + * Spreadtrum clock infrastructure
>> + *
>> + * Copyright (C) 2017 Spreadtrum, Inc.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> 

Re: [PATCH V1 2/9] clk: sprd: Add common infrastructure

[Stephen Boyd]

On 06/18, Chunyan Zhang wrote:
> Added Spreadtrum's clock driver common structure and registration code.
> 
> Signed-off-by: Chunyan Zhang 
> ---
>  drivers/clk/Makefile  |  1 +
>  drivers/clk/sprd/Makefile |  3 ++
>  drivers/clk/sprd/ccu_common.c | 78 +
>  drivers/clk/sprd/ccu_common.h | 90 
> +++
>  4 files changed, 172 insertions(+)
>  create mode 100644 drivers/clk/sprd/Makefile
>  create mode 100644 drivers/clk/sprd/ccu_common.c
>  create mode 100644 drivers/clk/sprd/ccu_common.h
> 
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index c19983a..1d62721 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_COMMON_CLK_SAMSUNG)+= samsung/
>  obj-$(CONFIG_ARCH_SIRF)  += sirf/
>  obj-$(CONFIG_ARCH_SOCFPGA)   += socfpga/
>  obj-$(CONFIG_PLAT_SPEAR) += spear/
> +obj-$(CONFIG_ARCH_SPRD)  += sprd/
>  obj-$(CONFIG_ARCH_STI)   += st/
>  obj-$(CONFIG_ARCH_SUNXI) += sunxi/
>  obj-$(CONFIG_ARCH_SUNXI) += sunxi-ng/
> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
> new file mode 100644
> index 000..8f802b2
> --- /dev/null
> +++ b/drivers/clk/sprd/Makefile
> @@ -0,0 +1,3 @@
> +ifneq ($(CONFIG_OF),)
> +obj-y+= ccu_common.o
> +endif

I'd prefer a Kconfig for SPRD clk drivers instead of this
CONFIG_OF check. Then we can compile test the sprd code in
configurations that don't have CONFIG_ARCH_SPRD set too.

> diff --git a/drivers/clk/sprd/ccu_common.c b/drivers/clk/sprd/ccu_common.c
> new file mode 100644
> index 000..911f4ba
> --- /dev/null
> +++ b/drivers/clk/sprd/ccu_common.c
> @@ -0,0 +1,78 @@
> +/*
> + * Spreadtrum clock infrastructure
> + *
> + * Copyright (C) 2017 Spreadtrum, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#include "ccu_common.h"
> +
> +static inline void __iomem *ccu_find_base(struct ccu_addr_map *maps,
> +   unsigned int num, unsigned int reg)
> +{
> + int i;
> +
> + for (i = 0; i < num; i++)
> + if ((reg & 0x) == maps[i].phy)

What is this?

> + return maps[i].virt;
> +
> + return 0;
> +}
> +
> +int sprd_ccu_probe(struct device_node *node, struct ccu_addr_map *maps,
> +unsigned int count, const struct sprd_ccu_desc *desc)
> +{
> + int i, ret = 0;
> + struct ccu_common *cclk;
> + struct clk_hw *hw;
> +
> + for (i = 0; i < desc->num_ccu_clks; i++) {
> + cclk = desc->ccu_clks[i];
> + if (!cclk)
> + continue;
> +
> + cclk->base = ccu_find_base(maps, count, cclk->reg);
> + if (!cclk->base) {
> + pr_err("%s: No mapped address found for clock(0x%x)\n",
> + __func__, cclk->reg);
> + return -EINVAL;
> + }
> + cclk->reg = cclk->reg & 0x;
> + }
> +
> + for (i = 0; i < desc->hw_clks->num; i++) {
> +
> + hw = desc->hw_clks->hws[i];
> +
> + if (!hw)
> + continue;
> +
> + ret = clk_hw_register(NULL, hw);
> + if (ret) {
> + pr_err("Couldn't register clock %d - %s\n",
> +i, hw->init->name);
> + goto err_clk_unreg;
> + }
> + }
> +
> + ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
> +  desc->hw_clks);
> + if (ret) {
> + pr_err("Failed to add clock provider.\n");
> + goto err_clk_unreg;
> + }
> +
> + return 0;
> +
> +err_clk_unreg:
> + while (--i >= 0) {
> + hw = desc->hw_clks->hws[i];
> + if (!hw)
> + continue;
> +
> + clk_hw_unregister(hw);
> + }
> +
> + return ret;
> +}
> diff --git a/drivers/clk/sprd/ccu_common.h b/drivers/clk/sprd/ccu_common.h
> new file mode 100644
> index 000..ff07772
> --- /dev/null
> +++ b/drivers/clk/sprd/ccu_common.h
> @@ -0,0 +1,90 @@
> +/*
> + * Spreadtrum clock infrastructure
> + *
> + * Copyright (C) 2017 Spreadtrum, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#ifndef _CCU_COMMON_H_
> +#define _CCU_COMMON_H_
> +
> +#include 
> +
> +struct device_node;
> +
> +#define CLK_HW_INIT_NO_PARENT(_name, _ops, _flags)   \
> + (&(struct clk_init_data) {  \
> + .flags  = _flags,   \
> + .name   = _name,\
> + .parent_names   = NULL, \
> + .num_parents= 0,\
> + .ops= _ops, \
> + })
> +
> +#define CLK_HW_INIT(_name, _parent, _ops, _flags)\
> + 

Re: [PATCH V1 2/9] clk: sprd: Add common infrastructure

[Stephen Boyd]

On 06/18, Chunyan Zhang wrote:
> Added Spreadtrum's clock driver common structure and registration code.
> 
> Signed-off-by: Chunyan Zhang 
> ---
>  drivers/clk/Makefile  |  1 +
>  drivers/clk/sprd/Makefile |  3 ++
>  drivers/clk/sprd/ccu_common.c | 78 +
>  drivers/clk/sprd/ccu_common.h | 90 
> +++
>  4 files changed, 172 insertions(+)
>  create mode 100644 drivers/clk/sprd/Makefile
>  create mode 100644 drivers/clk/sprd/ccu_common.c
>  create mode 100644 drivers/clk/sprd/ccu_common.h
> 
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index c19983a..1d62721 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -81,6 +81,7 @@ obj-$(CONFIG_COMMON_CLK_SAMSUNG)+= samsung/
>  obj-$(CONFIG_ARCH_SIRF)  += sirf/
>  obj-$(CONFIG_ARCH_SOCFPGA)   += socfpga/
>  obj-$(CONFIG_PLAT_SPEAR) += spear/
> +obj-$(CONFIG_ARCH_SPRD)  += sprd/
>  obj-$(CONFIG_ARCH_STI)   += st/
>  obj-$(CONFIG_ARCH_SUNXI) += sunxi/
>  obj-$(CONFIG_ARCH_SUNXI) += sunxi-ng/
> diff --git a/drivers/clk/sprd/Makefile b/drivers/clk/sprd/Makefile
> new file mode 100644
> index 000..8f802b2
> --- /dev/null
> +++ b/drivers/clk/sprd/Makefile
> @@ -0,0 +1,3 @@
> +ifneq ($(CONFIG_OF),)
> +obj-y+= ccu_common.o
> +endif

I'd prefer a Kconfig for SPRD clk drivers instead of this
CONFIG_OF check. Then we can compile test the sprd code in
configurations that don't have CONFIG_ARCH_SPRD set too.

> diff --git a/drivers/clk/sprd/ccu_common.c b/drivers/clk/sprd/ccu_common.c
> new file mode 100644
> index 000..911f4ba
> --- /dev/null
> +++ b/drivers/clk/sprd/ccu_common.c
> @@ -0,0 +1,78 @@
> +/*
> + * Spreadtrum clock infrastructure
> + *
> + * Copyright (C) 2017 Spreadtrum, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#include "ccu_common.h"
> +
> +static inline void __iomem *ccu_find_base(struct ccu_addr_map *maps,
> +   unsigned int num, unsigned int reg)
> +{
> + int i;
> +
> + for (i = 0; i < num; i++)
> + if ((reg & 0x) == maps[i].phy)

What is this?

> + return maps[i].virt;
> +
> + return 0;
> +}
> +
> +int sprd_ccu_probe(struct device_node *node, struct ccu_addr_map *maps,
> +unsigned int count, const struct sprd_ccu_desc *desc)
> +{
> + int i, ret = 0;
> + struct ccu_common *cclk;
> + struct clk_hw *hw;
> +
> + for (i = 0; i < desc->num_ccu_clks; i++) {
> + cclk = desc->ccu_clks[i];
> + if (!cclk)
> + continue;
> +
> + cclk->base = ccu_find_base(maps, count, cclk->reg);
> + if (!cclk->base) {
> + pr_err("%s: No mapped address found for clock(0x%x)\n",
> + __func__, cclk->reg);
> + return -EINVAL;
> + }
> + cclk->reg = cclk->reg & 0x;
> + }
> +
> + for (i = 0; i < desc->hw_clks->num; i++) {
> +
> + hw = desc->hw_clks->hws[i];
> +
> + if (!hw)
> + continue;
> +
> + ret = clk_hw_register(NULL, hw);
> + if (ret) {
> + pr_err("Couldn't register clock %d - %s\n",
> +i, hw->init->name);
> + goto err_clk_unreg;
> + }
> + }
> +
> + ret = of_clk_add_hw_provider(node, of_clk_hw_onecell_get,
> +  desc->hw_clks);
> + if (ret) {
> + pr_err("Failed to add clock provider.\n");
> + goto err_clk_unreg;
> + }
> +
> + return 0;
> +
> +err_clk_unreg:
> + while (--i >= 0) {
> + hw = desc->hw_clks->hws[i];
> + if (!hw)
> + continue;
> +
> + clk_hw_unregister(hw);
> + }
> +
> + return ret;
> +}
> diff --git a/drivers/clk/sprd/ccu_common.h b/drivers/clk/sprd/ccu_common.h
> new file mode 100644
> index 000..ff07772
> --- /dev/null
> +++ b/drivers/clk/sprd/ccu_common.h
> @@ -0,0 +1,90 @@
> +/*
> + * Spreadtrum clock infrastructure
> + *
> + * Copyright (C) 2017 Spreadtrum, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#ifndef _CCU_COMMON_H_
> +#define _CCU_COMMON_H_
> +
> +#include 
> +
> +struct device_node;
> +
> +#define CLK_HW_INIT_NO_PARENT(_name, _ops, _flags)   \
> + (&(struct clk_init_data) {  \
> + .flags  = _flags,   \
> + .name   = _name,\
> + .parent_names   = NULL, \
> + .num_parents= 0,\
> + .ops= _ops, \
> + })
> +
> +#define CLK_HW_INIT(_name, _parent, _ops, _flags)\
> + (&(struct clk_init_data) {