diff mbox

[3/3] clk: meson: add sub EMMC clock controller driver

Message ID 20180703145716.31860-4-yixun.lan@amlogic.com (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

Yixun Lan July 3, 2018, 2:57 p.m. UTC 
This patch will add a EMMC clock controller driver support,
It provide a mux and divider clock.

This clock driver can be protentially used by either EMMC and
NAND driver.

Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/clk/meson/Kconfig     |   9 +++
 drivers/clk/meson/Makefile    |   1 +
 drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++++++++++++++++++
 3 files changed, 146 insertions(+)
 create mode 100644 drivers/clk/meson/emmc-clkc.c

Comments

Jerome Brunet July 3, 2018, 8:51 a.m. UTC | #1 
On Tue, 2018-07-03 at 14:57 +0000, Yixun Lan wrote:
> This patch will add a EMMC clock controller driver support,
> It provide a mux and divider clock.
> 
> This clock driver can be protentially used by either EMMC and
> NAND driver.
> 
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  drivers/clk/meson/Kconfig     |   9 +++
>  drivers/clk/meson/Makefile    |   1 +
>  drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++++++++++++++++++
>  3 files changed, 146 insertions(+)
>  create mode 100644 drivers/clk/meson/emmc-clkc.c
> 
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index efaa70f682b4..2f27ff08e4eb 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -15,6 +15,15 @@ config COMMON_CLK_MESON_AO
>  	select COMMON_CLK_REGMAP_MESON
>  	select RESET_CONTROLLER
>  
> +config COMMON_CLK_EMMC_MESON
> +	tristate "Meson EMMC Sub Clock Controller Driver"
> +	depends on COMMON_CLK_AMLOGIC
> +	select MFD_SYSCON
> +	select REGMAP
> +	help
> +	  Support for the EMMC sub clock controller on AmLogic Meson Platform,
I thought you were not writing amlogic this way anymore -^ ??  ^ 
 ... or be consistent about it.                                |
                                                               |
Drop the camel case please                        -------------|
> +	  Say Y if you want this clock enabled.
> +
>  config COMMON_CLK_REGMAP_MESON
>  	bool
>  	select REGMAP
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 72ec8c40d848..2f04f77ba4de 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -9,4 +9,5 @@ obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>  obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
>  obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
> +obj-$(CONFIG_COMMON_CLK_EMMC_MESON) 	+= emmc-clkc.o
>  obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
> diff --git a/drivers/clk/meson/emmc-clkc.c b/drivers/clk/meson/emmc-clkc.c
> new file mode 100644
> index 000000000000..cf5bb9f34327
> --- /dev/null
> +++ b/drivers/clk/meson/emmc-clkc.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson EMMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/init.h>

replace with linux/module.h

> +#include <linux/of_address.h>

Why do you need this ?

> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/emmc-clkc.h>
> +
> +#include "clkc.h"
> +
> +#define SD_EMMC_CLOCK		0
> +#define MUX_CLK_NUM_PARENTS	2
> +#define EMMC_MAX_CLKS		2
> +#define CLK_NAME_LEN		48
> +
> +static struct clk_regmap_mux_data emmc_clkc_mux_data = {
> +	.offset = SD_EMMC_CLOCK,
> +	.mask = 0x3,
> +	.shift = 6,

If you round the divider "closest", shouldn't you do the same for the mux ?

> +};
> +
> +static struct clk_regmap_div_data emmc_clkc_div_data = {
> +	.offset = SD_EMMC_CLOCK,
> +	.shift = 0,
> +	.width = 6,
> +	.flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
> +};
> +
> +static const struct of_device_id clkc_match_table[] = {
> +	{ .compatible = "amlogic,emmc-clkc" },
> +	{}
> +};
> +
> +static int emmc_clkc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct clk_regmap *mux, *div;
> +	struct regmap *map;
> +	struct clk *clk;
> +	int i, ret;
> +	const char *parent_names[MUX_CLK_NUM_PARENTS];
> +	struct clk_init_data init;
> +	char mux_name[CLK_NAME_LEN], div_name[CLK_NAME_LEN];

I'm not big fan, especially if you append the dev_name() to the clock name.

> +	struct clk_hw_onecell_data *onecell_data;
> +
> +	map = syscon_node_to_regmap(dev->of_node);
> +
> +	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> +	div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
> +
> +	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
> +				    sizeof(*onecell_data->hws) * EMMC_MAX_CLKS,
> +				    GFP_KERNEL);
> +
> +	if (!mux || !div || !onecell_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> +		char name[8];
> +
> +		snprintf(name, sizeof(name), "clkin%d", i);
> +		clk = devm_clk_get(dev, name);
> +		if (IS_ERR(clk)) {
> +			if (clk != ERR_PTR(-EPROBE_DEFER))
> +				dev_err(dev, "Missing clock %s\n", name);
> +			return PTR_ERR(clk);
> +		}
> +
> +		parent_names[i] = __clk_get_name(clk);
> +	}
> +
> +	mux->map = map;
> +	mux->data = &emmc_clkc_mux_data;
> +
> +	snprintf(mux_name, CLK_NAME_LEN, "%s#emmc_mux", dev_name(dev));

I'd prefer if you used kasprintf() and free the name after the name after
registration.

> +
> +	init.name = mux_name;
> +	init.ops = &clk_regmap_mux_ops;
> +	init.flags = CLK_SET_RATE_PARENT;

If you really intend to have manual control over the mux, as commented in patch
2, you need CLK_SET_RATE_NOREPARENT here, otherwise whatever you set may be
overwritten by the next clk_set_rate() call. Please choose.

> +	init.parent_names = parent_names;
> +	init.num_parents = MUX_CLK_NUM_PARENTS;
> +
> +	mux->hw.init = &init;
> +	ret = devm_clk_hw_register(dev, &mux->hw);
> +	if (ret) {
> +		dev_err(dev, "Mux clock registration failed\n");
> +		return ret;
> +	}
> +
> +	parent_names[0] = mux_name;
> +	div->map = map;
> +	div->data = &emmc_clkc_div_data;
> +
> +	snprintf(div_name, CLK_NAME_LEN, "%s#emmc_div", dev_name(dev));
> +
> +	init.name = div_name;
> +	init.ops = &clk_regmap_divider_ops;
> +	init.flags = CLK_SET_RATE_PARENT;
> +	init.parent_names = parent_names;
> +	init.num_parents = 1;
> +
> +	div->hw.init = &init;
> +	ret = devm_clk_hw_register(dev, &div->hw);
> +	if (ret) {
> +		dev_err(dev, "Div clock registration failed\n");

s/Div/Divider

> +		return ret;
> +	}
> +
> +	onecell_data->hws[CLKID_EMMC_C_MUX] = &mux->hw,
> +	onecell_data->hws[CLKID_EMMC_C_DIV] = &div->hw,
> +	onecell_data->num = EMMC_MAX_CLKS;
> +
> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +					   onecell_data);

This is a long function with a lot of locals. please break that up.
(for example, one function to register each clock, helper to allocate the clk_hw
and names ...)

> +}
> +
> +static struct platform_driver emmc_clkc_driver = {
> +	.probe		= emmc_clkc_probe,
> +	.driver		= {
> +		.name	= "emmc-clkc",
> +		.of_match_table = clkc_match_table,
> +	},
> +};
> +

It should be a module, not a builtin - especially when the configuration is a
tristate
 
> +builtin_platform_driver(emmc_clkc_driver);

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yixun Lan July 3, 2018, 9:56 a.m. UTC | #2 
Hi Jerome:

see my comments

On 07/03/18 16:51, Jerome Brunet wrote:
> On Tue, 2018-07-03 at 14:57 +0000, Yixun Lan wrote:
>> This patch will add a EMMC clock controller driver support,
>> It provide a mux and divider clock.
>>
>> This clock driver can be protentially used by either EMMC and
>> NAND driver.
>>
>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> ---
>>  drivers/clk/meson/Kconfig     |   9 +++
>>  drivers/clk/meson/Makefile    |   1 +
>>  drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 146 insertions(+)
>>  create mode 100644 drivers/clk/meson/emmc-clkc.c
>>
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index efaa70f682b4..2f27ff08e4eb 100644
>> --- a/drivers/clk/meson/Kconfig
>> +++ b/drivers/clk/meson/Kconfig
>> @@ -15,6 +15,15 @@ config COMMON_CLK_MESON_AO
>>  	select COMMON_CLK_REGMAP_MESON
>>  	select RESET_CONTROLLER
>>  
>> +config COMMON_CLK_EMMC_MESON
>> +	tristate "Meson EMMC Sub Clock Controller Driver"
>> +	depends on COMMON_CLK_AMLOGIC
>> +	select MFD_SYSCON
>> +	select REGMAP
>> +	help
>> +	  Support for the EMMC sub clock controller on AmLogic Meson Platform,
> I thought you were not writing amlogic this way anymore -^ ??  ^ 
>  ... or be consistent about it.                                |
>                                                                |
> Drop the camel case please                        -------------|

oops, I'm not trying to find an excuse myself..
but this is copy & paste from COMMON_CLK_AXG_AUDIO section,
so it's also need an update.

I will fix in next version


>> +	  Say Y if you want this clock enabled.
>> +
>>  config COMMON_CLK_REGMAP_MESON
>>  	bool
>>  	select REGMAP
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index 72ec8c40d848..2f04f77ba4de 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -9,4 +9,5 @@ obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>>  obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
>>  obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
>>  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
>> +obj-$(CONFIG_COMMON_CLK_EMMC_MESON) 	+= emmc-clkc.o
>>  obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
>> diff --git a/drivers/clk/meson/emmc-clkc.c b/drivers/clk/meson/emmc-clkc.c
>> new file mode 100644
>> index 000000000000..cf5bb9f34327
>> --- /dev/null
>> +++ b/drivers/clk/meson/emmc-clkc.c
>> @@ -0,0 +1,136 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson EMMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/init.h>
> 
> replace with linux/module.h
> 
ok

>> +#include <linux/of_address.h>
> 
> Why do you need this ?
>
this can be removed, I will update

>> +#include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <dt-bindings/clock/emmc-clkc.h>
>> +
>> +#include "clkc.h"
>> +
>> +#define SD_EMMC_CLOCK		0
>> +#define MUX_CLK_NUM_PARENTS	2
>> +#define EMMC_MAX_CLKS		2
>> +#define CLK_NAME_LEN		48
>> +
>> +static struct clk_regmap_mux_data emmc_clkc_mux_data = {
>> +	.offset = SD_EMMC_CLOCK,
>> +	.mask = 0x3,
>> +	.shift = 6,
> 
> If you round the divider "closest", shouldn't you do the same for the mux ?
> 
>> +};
>> +
>> +static struct clk_regmap_div_data emmc_clkc_div_data = {
>> +	.offset = SD_EMMC_CLOCK,
>> +	.shift = 0,
>> +	.width = 6,
>> +	.flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
>> +};
>> +
>> +static const struct of_device_id clkc_match_table[] = {
>> +	{ .compatible = "amlogic,emmc-clkc" },
>> +	{}
>> +};
>> +
>> +static int emmc_clkc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct clk_regmap *mux, *div;
>> +	struct regmap *map;
>> +	struct clk *clk;
>> +	int i, ret;
>> +	const char *parent_names[MUX_CLK_NUM_PARENTS];
>> +	struct clk_init_data init;
>> +	char mux_name[CLK_NAME_LEN], div_name[CLK_NAME_LEN];
> 
> I'm not big fan, especially if you append the dev_name() to the clock name.
> 

the main idea of appending dev_name() is :
 I want to make this driver work with SD_EMMC_A, SD_EMMC_B clock
controller, not just SD_EMMC_C

so basically we can have one driver, then populate three EMMC clock
instance, and can distinguish from the dev_name()

or if you have better suggestion, do speak up

for this, I'm not sure if we really have plan to convert the EMMC driver
to use this sub emmc clock driver. if the answer is NO, then I could
even use static clock data to make this driver much simple (avoid using
various devm_kzalloc..)

>> +	struct clk_hw_onecell_data *onecell_data;
>> +
>> +	map = syscon_node_to_regmap(dev->of_node);
>> +
>> +	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>> +	div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
>> +
>> +	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
>> +				    sizeof(*onecell_data->hws) * EMMC_MAX_CLKS,
>> +				    GFP_KERNEL);
>> +
>> +	if (!mux || !div || !onecell_data)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
>> +		char name[8];
>> +
>> +		snprintf(name, sizeof(name), "clkin%d", i);
>> +		clk = devm_clk_get(dev, name);
>> +		if (IS_ERR(clk)) {
>> +			if (clk != ERR_PTR(-EPROBE_DEFER))
>> +				dev_err(dev, "Missing clock %s\n", name);
>> +			return PTR_ERR(clk);
>> +		}
>> +
>> +		parent_names[i] = __clk_get_name(clk);
>> +	}
>> +
>> +	mux->map = map;
>> +	mux->data = &emmc_clkc_mux_data;
>> +
>> +	snprintf(mux_name, CLK_NAME_LEN, "%s#emmc_mux", dev_name(dev));
> 
> I'd prefer if you used kasprintf() and free the name after the name after
> registration.
> 
Ok, I can do this

>> +
>> +	init.name = mux_name;
>> +	init.ops = &clk_regmap_mux_ops;
>> +	init.flags = CLK_SET_RATE_PARENT;
> 
> If you really intend to have manual control over the mux, as commented in patch
> 2, you need CLK_SET_RATE_NOREPARENT here, otherwise whatever you set may be
> overwritten by the next clk_set_rate() call. Please choose.
> 

ok, thanks for pointing this
>> +	init.parent_names = parent_names;
>> +	init.num_parents = MUX_CLK_NUM_PARENTS;
>> +
>> +	mux->hw.init = &init;
>> +	ret = devm_clk_hw_register(dev, &mux->hw);
>> +	if (ret) {
>> +		dev_err(dev, "Mux clock registration failed\n");
>> +		return ret;
>> +	}
>> +
>> +	parent_names[0] = mux_name;
>> +	div->map = map;
>> +	div->data = &emmc_clkc_div_data;
>> +
>> +	snprintf(div_name, CLK_NAME_LEN, "%s#emmc_div", dev_name(dev));
>> +
>> +	init.name = div_name;
>> +	init.ops = &clk_regmap_divider_ops;
>> +	init.flags = CLK_SET_RATE_PARENT;
>> +	init.parent_names = parent_names;
>> +	init.num_parents = 1;
>> +
>> +	div->hw.init = &init;
>> +	ret = devm_clk_hw_register(dev, &div->hw);
>> +	if (ret) {
>> +		dev_err(dev, "Div clock registration failed\n");
> 
> s/Div/Divider
> 
ok

>> +		return ret;
>> +	}
>> +
>> +	onecell_data->hws[CLKID_EMMC_C_MUX] = &mux->hw,
>> +	onecell_data->hws[CLKID_EMMC_C_DIV] = &div->hw,
>> +	onecell_data->num = EMMC_MAX_CLKS;
>> +
>> +	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>> +					   onecell_data);
> 
> This is a long function with a lot of locals. please break that up.
> (for example, one function to register each clock, helper to allocate the clk_hw
> and names ...)
> 
ok
>> +}
>> +
>> +static struct platform_driver emmc_clkc_driver = {
>> +	.probe		= emmc_clkc_probe,
>> +	.driver		= {
>> +		.name	= "emmc-clkc",
>> +		.of_match_table = clkc_match_table,
>> +	},
>> +};
>> +
> 
> It should be a module, not a builtin - especially when the configuration is a
> tristate
>  
>> +builtin_platform_driver(emmc_clkc_driver);
Ok, I will fix this
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin Blumenstingl July 3, 2018, 6:58 p.m. UTC | #3 
Hi Yixun,

apart from what Jerome found this looks good to me.
one small "issue" and a question are inline below

On Tue, Jul 3, 2018 at 9:00 AM Yixun Lan <yixun.lan@amlogic.com> wrote:
>
> This patch will add a EMMC clock controller driver support,
> It provide a mux and divider clock.
>
> This clock driver can be protentially used by either EMMC and
> NAND driver.
>
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  drivers/clk/meson/Kconfig     |   9 +++
>  drivers/clk/meson/Makefile    |   1 +
>  drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++++++++++++++++++
>  3 files changed, 146 insertions(+)
>  create mode 100644 drivers/clk/meson/emmc-clkc.c
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index efaa70f682b4..2f27ff08e4eb 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -15,6 +15,15 @@ config COMMON_CLK_MESON_AO
>         select COMMON_CLK_REGMAP_MESON
>         select RESET_CONTROLLER
>
> +config COMMON_CLK_EMMC_MESON
> +       tristate "Meson EMMC Sub Clock Controller Driver"
> +       depends on COMMON_CLK_AMLOGIC
> +       select MFD_SYSCON
> +       select REGMAP
> +       help
> +         Support for the EMMC sub clock controller on AmLogic Meson Platform,
> +         Say Y if you want this clock enabled.
> +
>  config COMMON_CLK_REGMAP_MESON
>         bool
>         select REGMAP
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 72ec8c40d848..2f04f77ba4de 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -9,4 +9,5 @@ obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>  obj-$(CONFIG_COMMON_CLK_GXBB)   += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
>  obj-$(CONFIG_COMMON_CLK_AXG)    += axg.o axg-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)     += axg-audio.o
> +obj-$(CONFIG_COMMON_CLK_EMMC_MESON)    += emmc-clkc.o
>  obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)  += clk-regmap.o
> diff --git a/drivers/clk/meson/emmc-clkc.c b/drivers/clk/meson/emmc-clkc.c
> new file mode 100644
> index 000000000000..cf5bb9f34327
> --- /dev/null
> +++ b/drivers/clk/meson/emmc-clkc.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Amlogic Meson EMMC Sub Clock Controller Driver
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Yixun Lan <yixun.lan@amlogic.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/init.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/emmc-clkc.h>
> +
> +#include "clkc.h"
> +
> +#define SD_EMMC_CLOCK          0
> +#define MUX_CLK_NUM_PARENTS    2
> +#define EMMC_MAX_CLKS          2
> +#define CLK_NAME_LEN           48
> +
> +static struct clk_regmap_mux_data emmc_clkc_mux_data = {
> +       .offset = SD_EMMC_CLOCK,
> +       .mask = 0x3,
> +       .shift = 6,
> +};
> +
> +static struct clk_regmap_div_data emmc_clkc_div_data = {
> +       .offset = SD_EMMC_CLOCK,
> +       .shift = 0,
> +       .width = 6,
> +       .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
> +};
> +
> +static const struct of_device_id clkc_match_table[] = {
> +       { .compatible = "amlogic,emmc-clkc" },
shouldn't this be "amlogic,meson-axg-emmc-clkc" (and optionally also
"amlogic,meson-gx-emmc-clkc")?

> +       {}
> +};
> +
> +static int emmc_clkc_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct clk_regmap *mux, *div;
> +       struct regmap *map;
> +       struct clk *clk;
> +       int i, ret;
> +       const char *parent_names[MUX_CLK_NUM_PARENTS];
> +       struct clk_init_data init;
> +       char mux_name[CLK_NAME_LEN], div_name[CLK_NAME_LEN];
> +       struct clk_hw_onecell_data *onecell_data;
> +
> +       map = syscon_node_to_regmap(dev->of_node);
> +
> +       mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> +       div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
> +
> +       onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
> +                                   sizeof(*onecell_data->hws) * EMMC_MAX_CLKS,
> +                                   GFP_KERNEL);
> +
> +       if (!mux || !div || !onecell_data)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
> +               char name[8];
> +
> +               snprintf(name, sizeof(name), "clkin%d", i);
> +               clk = devm_clk_get(dev, name);
> +               if (IS_ERR(clk)) {
> +                       if (clk != ERR_PTR(-EPROBE_DEFER))
> +                               dev_err(dev, "Missing clock %s\n", name);
> +                       return PTR_ERR(clk);
> +               }
> +
> +               parent_names[i] = __clk_get_name(clk);
> +       }
> +
> +       mux->map = map;
> +       mux->data = &emmc_clkc_mux_data;
> +
> +       snprintf(mux_name, CLK_NAME_LEN, "%s#emmc_mux", dev_name(dev));
> +
> +       init.name = mux_name;
> +       init.ops = &clk_regmap_mux_ops;
> +       init.flags = CLK_SET_RATE_PARENT;
> +       init.parent_names = parent_names;
> +       init.num_parents = MUX_CLK_NUM_PARENTS;
> +
> +       mux->hw.init = &init;
> +       ret = devm_clk_hw_register(dev, &mux->hw);
> +       if (ret) {
> +               dev_err(dev, "Mux clock registration failed\n");
> +               return ret;
> +       }
> +
> +       parent_names[0] = mux_name;
> +       div->map = map;
> +       div->data = &emmc_clkc_div_data;
> +
> +       snprintf(div_name, CLK_NAME_LEN, "%s#emmc_div", dev_name(dev));
> +
> +       init.name = div_name;
> +       init.ops = &clk_regmap_divider_ops;
> +       init.flags = CLK_SET_RATE_PARENT;
> +       init.parent_names = parent_names;
> +       init.num_parents = 1;
> +
> +       div->hw.init = &init;
> +       ret = devm_clk_hw_register(dev, &div->hw);
> +       if (ret) {
> +               dev_err(dev, "Div clock registration failed\n");
> +               return ret;
> +       }
> +
> +       onecell_data->hws[CLKID_EMMC_C_MUX] = &mux->hw,
> +       onecell_data->hws[CLKID_EMMC_C_DIV] = &div->hw,
> +       onecell_data->num = EMMC_MAX_CLKS;
you are describing the mux and the divider here
however, meson-gx-mmc.c has a few more clock related bits:
- CLK_CORE_PHASE_MASK
- CLK_TX_PHASE_MASK
- CLK_RX_PHASE_MASK
- CLK_V2_TX_DELAY_MASK / CLK_V3_TX_DELAY_MASK
- CLK_V2_RX_DELAY_MASK / CLK_V3_RX_DELAY_MASK
- CLK_V2_ALWAYS_ON / CLK_V3_ALWAYS_ON

are these used for the MMC clock or are some of these routed to the
NAND pins as well?


Regards
Martin
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yixun Lan July 4, 2018, 7:17 a.m. UTC | #4 
Hi Martin


On 07/04/18 02:58, Martin Blumenstingl wrote:
> Hi Yixun,
> 
> apart from what Jerome found this looks good to me.
> one small "issue" and a question are inline below
> 
> On Tue, Jul 3, 2018 at 9:00 AM Yixun Lan <yixun.lan@amlogic.com> wrote:
>>
>> This patch will add a EMMC clock controller driver support,
>> It provide a mux and divider clock.
>>
>> This clock driver can be protentially used by either EMMC and
>> NAND driver.
>>
>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> ---
>>  drivers/clk/meson/Kconfig     |   9 +++
>>  drivers/clk/meson/Makefile    |   1 +
>>  drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 146 insertions(+)
>>  create mode 100644 drivers/clk/meson/emmc-clkc.c
>>
>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> index efaa70f682b4..2f27ff08e4eb 100644
>> --- a/drivers/clk/meson/Kconfig
>> +++ b/drivers/clk/meson/Kconfig
>> @@ -15,6 +15,15 @@ config COMMON_CLK_MESON_AO
>>         select COMMON_CLK_REGMAP_MESON
>>         select RESET_CONTROLLER
>>
>> +config COMMON_CLK_EMMC_MESON
>> +       tristate "Meson EMMC Sub Clock Controller Driver"
>> +       depends on COMMON_CLK_AMLOGIC
>> +       select MFD_SYSCON
>> +       select REGMAP
>> +       help
>> +         Support for the EMMC sub clock controller on AmLogic Meson Platform,
>> +         Say Y if you want this clock enabled.
>> +
>>  config COMMON_CLK_REGMAP_MESON
>>         bool
>>         select REGMAP
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index 72ec8c40d848..2f04f77ba4de 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -9,4 +9,5 @@ obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>>  obj-$(CONFIG_COMMON_CLK_GXBB)   += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
>>  obj-$(CONFIG_COMMON_CLK_AXG)    += axg.o axg-aoclk.o
>>  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)     += axg-audio.o
>> +obj-$(CONFIG_COMMON_CLK_EMMC_MESON)    += emmc-clkc.o
>>  obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)  += clk-regmap.o
>> diff --git a/drivers/clk/meson/emmc-clkc.c b/drivers/clk/meson/emmc-clkc.c
>> new file mode 100644
>> index 000000000000..cf5bb9f34327
>> --- /dev/null
>> +++ b/drivers/clk/meson/emmc-clkc.c
>> @@ -0,0 +1,136 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Amlogic Meson EMMC Sub Clock Controller Driver
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Yixun Lan <yixun.lan@amlogic.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/init.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <dt-bindings/clock/emmc-clkc.h>
>> +
>> +#include "clkc.h"
>> +
>> +#define SD_EMMC_CLOCK          0
>> +#define MUX_CLK_NUM_PARENTS    2
>> +#define EMMC_MAX_CLKS          2
>> +#define CLK_NAME_LEN           48
>> +
>> +static struct clk_regmap_mux_data emmc_clkc_mux_data = {
>> +       .offset = SD_EMMC_CLOCK,
>> +       .mask = 0x3,
>> +       .shift = 6,
>> +};
>> +
>> +static struct clk_regmap_div_data emmc_clkc_div_data = {
>> +       .offset = SD_EMMC_CLOCK,
>> +       .shift = 0,
>> +       .width = 6,
>> +       .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
>> +};
>> +
>> +static const struct of_device_id clkc_match_table[] = {
>> +       { .compatible = "amlogic,emmc-clkc" },
> shouldn't this be "amlogic,meson-axg-emmc-clkc" (and optionally also
> "amlogic,meson-gx-emmc-clkc")?
> 
sounds good to me..
is it a convention to always make the compatible specific ?..

>> +       {}
>> +};
>> +
>> +static int emmc_clkc_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct clk_regmap *mux, *div;
>> +       struct regmap *map;
>> +       struct clk *clk;
>> +       int i, ret;
>> +       const char *parent_names[MUX_CLK_NUM_PARENTS];
>> +       struct clk_init_data init;
>> +       char mux_name[CLK_NAME_LEN], div_name[CLK_NAME_LEN];
>> +       struct clk_hw_onecell_data *onecell_data;
>> +
>> +       map = syscon_node_to_regmap(dev->of_node);
>> +
>> +       mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
>> +       div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
>> +
>> +       onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
>> +                                   sizeof(*onecell_data->hws) * EMMC_MAX_CLKS,
>> +                                   GFP_KERNEL);
>> +
>> +       if (!mux || !div || !onecell_data)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
>> +               char name[8];
>> +
>> +               snprintf(name, sizeof(name), "clkin%d", i);
>> +               clk = devm_clk_get(dev, name);
>> +               if (IS_ERR(clk)) {
>> +                       if (clk != ERR_PTR(-EPROBE_DEFER))
>> +                               dev_err(dev, "Missing clock %s\n", name);
>> +                       return PTR_ERR(clk);
>> +               }
>> +
>> +               parent_names[i] = __clk_get_name(clk);
>> +       }
>> +
>> +       mux->map = map;
>> +       mux->data = &emmc_clkc_mux_data;
>> +
>> +       snprintf(mux_name, CLK_NAME_LEN, "%s#emmc_mux", dev_name(dev));
>> +
>> +       init.name = mux_name;
>> +       init.ops = &clk_regmap_mux_ops;
>> +       init.flags = CLK_SET_RATE_PARENT;
>> +       init.parent_names = parent_names;
>> +       init.num_parents = MUX_CLK_NUM_PARENTS;
>> +
>> +       mux->hw.init = &init;
>> +       ret = devm_clk_hw_register(dev, &mux->hw);
>> +       if (ret) {
>> +               dev_err(dev, "Mux clock registration failed\n");
>> +               return ret;
>> +       }
>> +
>> +       parent_names[0] = mux_name;
>> +       div->map = map;
>> +       div->data = &emmc_clkc_div_data;
>> +
>> +       snprintf(div_name, CLK_NAME_LEN, "%s#emmc_div", dev_name(dev));
>> +
>> +       init.name = div_name;
>> +       init.ops = &clk_regmap_divider_ops;
>> +       init.flags = CLK_SET_RATE_PARENT;
>> +       init.parent_names = parent_names;
>> +       init.num_parents = 1;
>> +
>> +       div->hw.init = &init;
>> +       ret = devm_clk_hw_register(dev, &div->hw);
>> +       if (ret) {
>> +               dev_err(dev, "Div clock registration failed\n");
>> +               return ret;
>> +       }
>> +
>> +       onecell_data->hws[CLKID_EMMC_C_MUX] = &mux->hw,
>> +       onecell_data->hws[CLKID_EMMC_C_DIV] = &div->hw,
>> +       onecell_data->num = EMMC_MAX_CLKS;
> you are describing the mux and the divider here
> however, meson-gx-mmc.c has a few more clock related bits:
> - CLK_CORE_PHASE_MASK
> - CLK_TX_PHASE_MASK
> - CLK_RX_PHASE_MASK
> - CLK_V2_TX_DELAY_MASK / CLK_V3_TX_DELAY_MASK
> - CLK_V2_RX_DELAY_MASK / CLK_V3_RX_DELAY_MASK
> - CLK_V2_ALWAYS_ON / CLK_V3_ALWAYS_ON
> 
> are these used for the MMC clock or are some of these routed to the
> NAND pins as well?

There clocks are not used in NAND driver..

I understand your concern here, if there clocks are also routed to NAND
pins, then we also need to implement them here
actually, to answer your question, I need to query the ASIC team..

> 
> 
> Regards
> Martin
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jerome Brunet July 4, 2018, 8:07 a.m. UTC | #5 
On Wed, 2018-07-04 at 15:17 +0800, Yixun Lan wrote:
> > you are describing the mux and the divider here
> > however, meson-gx-mmc.c has a few more clock related bits:
> > - CLK_CORE_PHASE_MASK
> > - CLK_TX_PHASE_MASK
> > - CLK_RX_PHASE_MASK
> > - CLK_V2_TX_DELAY_MASK / CLK_V3_TX_DELAY_MASK
> > - CLK_V2_RX_DELAY_MASK / CLK_V3_RX_DELAY_MASK
> > - CLK_V2_ALWAYS_ON / CLK_V3_ALWAYS_ON
> > 
> > are these used for the MMC clock or are some of these routed to the
> > NAND pins as well?
> 
> There clocks are not used in NAND driver..
> 
> I understand your concern here, if there clocks are also routed to NAND
> pins, then we also need to implement them here
> actually, to answer your question, I need to query the ASIC team..

Even if the NAND driver does not need to change the phases, it might need to
make sure these phases are reset on init. It would not hurt to handle these
phases in your clock controller.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index efaa70f682b4..2f27ff08e4eb 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -15,6 +15,15 @@  config COMMON_CLK_MESON_AO
 	select COMMON_CLK_REGMAP_MESON
 	select RESET_CONTROLLER
 
+config COMMON_CLK_EMMC_MESON
+	tristate "Meson EMMC Sub Clock Controller Driver"
+	depends on COMMON_CLK_AMLOGIC
+	select MFD_SYSCON
+	select REGMAP
+	help
+	  Support for the EMMC sub clock controller on AmLogic Meson Platform,
+	  Say Y if you want this clock enabled.
+
 config COMMON_CLK_REGMAP_MESON
 	bool
 	select REGMAP
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 72ec8c40d848..2f04f77ba4de 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -9,4 +9,5 @@  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
 obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o
 obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
 obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
+obj-$(CONFIG_COMMON_CLK_EMMC_MESON) 	+= emmc-clkc.o
 obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
diff --git a/drivers/clk/meson/emmc-clkc.c b/drivers/clk/meson/emmc-clkc.c
new file mode 100644
index 000000000000..cf5bb9f34327
--- /dev/null
+++ b/drivers/clk/meson/emmc-clkc.c
@@ -0,0 +1,136 @@ 
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Amlogic Meson EMMC Sub Clock Controller Driver
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yixun Lan <yixun.lan@amlogic.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/init.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/emmc-clkc.h>
+
+#include "clkc.h"
+
+#define SD_EMMC_CLOCK		0
+#define MUX_CLK_NUM_PARENTS	2
+#define EMMC_MAX_CLKS		2
+#define CLK_NAME_LEN		48
+
+static struct clk_regmap_mux_data emmc_clkc_mux_data = {
+	.offset = SD_EMMC_CLOCK,
+	.mask = 0x3,
+	.shift = 6,
+};
+
+static struct clk_regmap_div_data emmc_clkc_div_data = {
+	.offset = SD_EMMC_CLOCK,
+	.shift = 0,
+	.width = 6,
+	.flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED,
+};
+
+static const struct of_device_id clkc_match_table[] = {
+	{ .compatible = "amlogic,emmc-clkc" },
+	{}
+};
+
+static int emmc_clkc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct clk_regmap *mux, *div;
+	struct regmap *map;
+	struct clk *clk;
+	int i, ret;
+	const char *parent_names[MUX_CLK_NUM_PARENTS];
+	struct clk_init_data init;
+	char mux_name[CLK_NAME_LEN], div_name[CLK_NAME_LEN];
+	struct clk_hw_onecell_data *onecell_data;
+
+	map = syscon_node_to_regmap(dev->of_node);
+
+	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
+	div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
+
+	onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) +
+				    sizeof(*onecell_data->hws) * EMMC_MAX_CLKS,
+				    GFP_KERNEL);
+
+	if (!mux || !div || !onecell_data)
+		return -ENOMEM;
+
+	for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) {
+		char name[8];
+
+		snprintf(name, sizeof(name), "clkin%d", i);
+		clk = devm_clk_get(dev, name);
+		if (IS_ERR(clk)) {
+			if (clk != ERR_PTR(-EPROBE_DEFER))
+				dev_err(dev, "Missing clock %s\n", name);
+			return PTR_ERR(clk);
+		}
+
+		parent_names[i] = __clk_get_name(clk);
+	}
+
+	mux->map = map;
+	mux->data = &emmc_clkc_mux_data;
+
+	snprintf(mux_name, CLK_NAME_LEN, "%s#emmc_mux", dev_name(dev));
+
+	init.name = mux_name;
+	init.ops = &clk_regmap_mux_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	init.parent_names = parent_names;
+	init.num_parents = MUX_CLK_NUM_PARENTS;
+
+	mux->hw.init = &init;
+	ret = devm_clk_hw_register(dev, &mux->hw);
+	if (ret) {
+		dev_err(dev, "Mux clock registration failed\n");
+		return ret;
+	}
+
+	parent_names[0] = mux_name;
+	div->map = map;
+	div->data = &emmc_clkc_div_data;
+
+	snprintf(div_name, CLK_NAME_LEN, "%s#emmc_div", dev_name(dev));
+
+	init.name = div_name;
+	init.ops = &clk_regmap_divider_ops;
+	init.flags = CLK_SET_RATE_PARENT;
+	init.parent_names = parent_names;
+	init.num_parents = 1;
+
+	div->hw.init = &init;
+	ret = devm_clk_hw_register(dev, &div->hw);
+	if (ret) {
+		dev_err(dev, "Div clock registration failed\n");
+		return ret;
+	}
+
+	onecell_data->hws[CLKID_EMMC_C_MUX] = &mux->hw,
+	onecell_data->hws[CLKID_EMMC_C_DIV] = &div->hw,
+	onecell_data->num = EMMC_MAX_CLKS;
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					   onecell_data);
+}
+
+static struct platform_driver emmc_clkc_driver = {
+	.probe		= emmc_clkc_probe,
+	.driver		= {
+		.name	= "emmc-clkc",
+		.of_match_table = clkc_match_table,
+	},
+};
+
+builtin_platform_driver(emmc_clkc_driver);