diff mbox

[2/2] clk: meson-axg: Add AO Clock and Reset controller driver

Message ID 20180209070026.193879-3-yixun.lan@amlogic.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Yixun Lan Feb. 9, 2018, 7 a.m. UTC
Adds a Clock and Reset controller driver for the Always-On part
of the Amlogic Meson-AXG SoC.

Signed-off-by: Qiufang Dai <qiufang.dai@amlogic.com>
Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
---
 drivers/clk/meson/Makefile    |   2 +-
 drivers/clk/meson/axg-aoclk.c | 236 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/axg-aoclk.h |  25 +++++
 3 files changed, 262 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/meson/axg-aoclk.c
 create mode 100644 drivers/clk/meson/axg-aoclk.h

Comments

Jerome Brunet Feb. 12, 2018, 3:32 p.m. UTC | #1
On Fri, 2018-02-09 at 15:00 +0800, Yixun Lan wrote:
> Adds a Clock and Reset controller driver for the Always-On part
> of the Amlogic Meson-AXG SoC.
> 
> Signed-off-by: Qiufang Dai <qiufang.dai@amlogic.com>
> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> ---
>  drivers/clk/meson/Makefile    |   2 +-
>  drivers/clk/meson/axg-aoclk.c | 236 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/axg-aoclk.h |  25 +++++
>  3 files changed, 262 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/meson/axg-aoclk.c
>  create mode 100644 drivers/clk/meson/axg-aoclk.h
> 
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 11f99139b844..c7510744406a 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -6,6 +6,6 @@ obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-audio-divider.o
>  obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-phase.o
>  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
> +obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
>  obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
> diff --git a/drivers/clk/meson/axg-aoclk.c b/drivers/clk/meson/axg-aoclk.c
> new file mode 100644
> index 000000000000..832aa19dd76c
> --- /dev/null
> +++ b/drivers/clk/meson/axg-aoclk.c
> @@ -0,0 +1,236 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * AmLogic Meson-AXG Clock Controller Driver

Unless there a rule behind it, I think we should stop 
with the funny camel case and stick to "Amlogic" like written below.

> + *
> + * Copyright (c) 2016 Baylibre SAS.
> + * Author: Michael Turquette <mturquette@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
> + */
> +#include <linux/clk-provider.h>
> +#include <linux/of_address.h>
I did not see what is requiring this header. Did I miss something ?

> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/init.h>
> +#include <linux/delay.h>
Same question for this one.

> +#include <dt-bindings/clock/axg-aoclkc.h>
> +#include <dt-bindings/reset/axg-aoclkc.h>

I think those should be included in the same fashion as axg.h, gxbb.h and
meson8.h ... which is at the end of the private header.

> +#include "clkc.h"
> +#include "axg-aoclk.h"
> +
> +struct axg_aoclk_reset_controller {
> +	struct reset_controller_dev reset;
> +	unsigned int *data;
> +	struct regmap *regmap;
> +};

This duplicate what is gxbb-aoclk.
Please make something cleaner with a shared definition.

> +
> +static int axg_aoclk_do_reset(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct axg_aoclk_reset_controller *reset =
> +		container_of(rcdev, struct axg_aoclk_reset_controller, reset);
> +
> +	return regmap_write(reset->regmap, AO_RTI_GEN_CNTL_REG0,
> +			    BIT(reset->data[id]));
> +}
> +
> +static const struct reset_control_ops axg_aoclk_reset_ops = {
> +	.reset = axg_aoclk_do_reset,
> +};
> +
> +#define AXG_AO_GATE(_name, _bit)					\
> +static struct clk_regmap _name##_ao = {					\
> +	.data = &(struct clk_regmap_gate_data) {			\
> +		.offset = (AO_RTI_GEN_CNTL_REG0),			\
> +		.bit_idx = (_bit),					\
> +	},								\
> +	.hw.init = &(struct clk_init_data) {				\
> +		.name = #_name "_ao",					\
> +		.ops = &clk_regmap_gate_ops,				\
> +		.parent_names = (const char *[]){ "clk81" },		\
> +		.num_parents = 1,					\
> +		.flags = (CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED),	\

These clock are peripheral clocks (meant to enable/disable IPs):
* CLK_SET_RATE_PARENT: why do you need rate propagation here ? do you really
want a consumer of these gate to be able to change clk81 ? 

* CLK_IGNORE_UNUSED: Why do need this flag ? All the IPs behind this
definition should be able to claim the pclk properly. If not the driver should
be fixed.

> +	},								\
> +}
> +
> +AXG_AO_GATE(remote, 0);
> +AXG_AO_GATE(i2c_master, 1);
> +AXG_AO_GATE(i2c_slave, 2);
> +AXG_AO_GATE(uart1, 3);
> +AXG_AO_GATE(uart2, 5);
> +AXG_AO_GATE(ir_blaster, 6);
> +AXG_AO_GATE(saradc, 7);
> +
> +static struct clk_fixed_rate ao_alt_xtal = {
> +	.fixed_rate = 32000,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "ao_alt_xtal",
> +		.num_parents = 0,
> +		.ops = &clk_fixed_rate_ops,
> +	},
> +};

Unless this xtal is actually embedded in SoC, this should be defined it the DT,
same as the other xtal.

> +
> +static struct clk_regmap ao_clk81 = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = AO_RTI_PWR_CNTL_REG0,
> +		.mask = 0x1,
> +		.shift = 8,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "ao_clk81",
> +		.ops = &clk_regmap_mux_ro_ops,
> +		.parent_names = (const char *[]){ "clk81", "ao_alt_xtal"},
> +		.num_parents = 2,
> +	},
> +};
> +
> +static struct clk_regmap axg_saradc_mux = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = AO_SAR_CLK,
> +		.mask = 0x3,
> +		.shift = 9,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "axg_saradc_mux",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_names = (const char *[]){ "xtal", "ao_clk81" },
> +		.num_parents = 2,
> +	},
> +};
> +
> +static struct clk_regmap axg_saradc_div = {
> +	.data = &(struct clk_regmap_div_data) {
> +		.offset = AO_SAR_CLK,
> +		.shift = 0,
> +		.width = 8,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "axg_saradc_div",
> +		.ops = &clk_regmap_divider_ops,
> +		.parent_names = (const char *[]){ "axg_saradc_mux" },
> +		.num_parents = 1,

Should the saradc mux change to accomodate the requested rate, if needed ?
If so, you might want to add a CLK_SET_RATE_PARENT here.

> +	},
> +};
> +
> +static struct clk_regmap axg_saradc_gate = {
> +	.data = &(struct clk_regmap_gate_data) {
> +		.offset = AO_SAR_CLK,
> +		.bit_idx = 8,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "axg_saradc_gate",
> +		.ops = &clk_regmap_gate_ops,
> +		.parent_names = (const char *[]){ "axg_saradc_div" },
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static unsigned int axg_aoclk_reset[] = {
> +	[RESET_AO_REMOTE] = 16,
> +	[RESET_AO_I2C_MASTER] = 18,
> +	[RESET_AO_I2C_SLAVE] = 19,
> +	[RESET_AO_UART1] = 17,
> +	[RESET_AO_UART2] = 22,
> +	[RESET_AO_IR_BLASTER] = 23,
> +};
> +
> +static struct clk_regmap *axg_aoclk_regmap[] = {
> +	[CLKID_AO_REMOTE]	= &remote_ao,
> +	[CLKID_AO_I2C_MASTER]	= &i2c_master_ao,
> +	[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao,
> +	[CLKID_AO_UART1]	= &uart1_ao,
> +	[CLKID_AO_UART2]	= &uart2_ao,
> +	[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao,
> +	[CLKID_AO_SAR_ADC]	= &saradc_ao,
> +	[CLKID_AO_CLK81]	= &ao_clk81,
> +	[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux,
> +	[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div,
> +	[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate,
> +};
> +
> +static struct clk_hw_onecell_data axg_aoclk_onecell_data = {
> +	.hws = {
> +		[CLKID_AO_REMOTE]	= &remote_ao.hw,
> +		[CLKID_AO_I2C_MASTER]	= &i2c_master_ao.hw,
> +		[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao.hw,
> +		[CLKID_AO_UART1]	= &uart1_ao.hw,
> +		[CLKID_AO_UART2]	= &uart2_ao.hw,
> +		[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao.hw,
> +		[CLKID_AO_SAR_ADC]	= &saradc_ao.hw,
> +		[CLKID_AO_CLK81]	= &ao_clk81.hw,
> +		[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux.hw,
> +		[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div.hw,
> +		[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate.hw,
> +		[CLKID_AO_ALT_XTAL]	= &ao_alt_xtal.hw,
> +	},
> +	.num = 12,

I would prefer having a NR_CLKS defined in drivers/clk/meson/axg-aoclk.h

> +};
> +
> +static int axg_aoclkc_probe(struct platform_device *pdev)
> +{
> +	struct axg_aoclk_reset_controller *rstc;
> +	struct device *dev = &pdev->dev;
> +	struct regmap *regmap;
> +	int ret, clkid;
> +
> +	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
> +	if (!rstc)
> +		return -ENOMEM;
> +
> +	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "failed to get regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Reset Controller */
> +	rstc->regmap = regmap;
> +	rstc->data = axg_aoclk_reset;
> +	rstc->reset.ops = &axg_aoclk_reset_ops;
> +	rstc->reset.nr_resets = ARRAY_SIZE(axg_aoclk_reset);
> +	rstc->reset.of_node = dev->of_node;
> +	ret = devm_reset_controller_register(dev, &rstc->reset);
> +
> +	/*
> +	 * Populate regmap and register all clks
> +	 */
> +	for (clkid = 0; clkid < ARRAY_SIZE(axg_aoclk_regmap); clkid++) {
> +		axg_aoclk_regmap[clkid]->map = regmap;
> +
> +		ret = devm_clk_hw_register(dev,
> +					axg_aoclk_onecell_data.hws[clkid]);
> +		if (ret) {
> +			dev_err(dev, "clk register failed.\n");
> +			return ret;
> +		}
> +	}
> +
> +	/* Specific clocks */
> +	ret = devm_clk_hw_register(dev, &ao_alt_xtal.hw);
> +	if (ret) {
> +		dev_err(dev, "clk alt_xtal register failed.\n");
> +		return ret;
> +	}
> +
> +	return of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
> +			&axg_aoclk_onecell_data);
> +}
> +
> +static const struct of_device_id axg_aoclkc_match_table[] = {
> +	{ .compatible = "amlogic,meson-axg-aoclkc" },
> +	{ }
> +};
> +
> +static struct platform_driver axg_aoclkc_driver = {
> +	.probe		= axg_aoclkc_probe,
> +	.driver		= {
> +		.name	= "axg-aoclkc",
> +		.of_match_table = axg_aoclkc_match_table,
> +	},
> +};
> +
> +builtin_platform_driver(axg_aoclkc_driver);
> diff --git a/drivers/clk/meson/axg-aoclk.h b/drivers/clk/meson/axg-aoclk.h
> new file mode 100644
> index 000000000000..70f82004d3db
> --- /dev/null
> +++ b/drivers/clk/meson/axg-aoclk.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Copyright (c) 2017 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * Copyright (c) 2018 Amlogic, inc.
> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
> + */
> +
> +#ifndef __AXG_AOCLKC_H
> +#define __AXG_AOCLKC_H
> +
> +/* AO Configuration Clock registers offsets
> + * Register offsets from the data sheet must be multiplied by 4.
> + */
> +#define AO_RTI_PWR_CNTL_REG1	(0x03 << 2)
> +#define AO_RTI_PWR_CNTL_REG0	(0x04 << 2)
> +#define AO_RTI_GEN_CNTL_REG0	(0x10 << 2)
> +#define AO_OSCIN_CNTL		(0x16 << 2)
> +#define AO_CRT_CLK_CNTL1	(0x1a << 2)
> +#define AO_SAR_CLK		(0x24 << 2)
> +#define AO_RTC_ALT_CLK_CNTL0	(0x25 << 2)
> +#define AO_RTC_ALT_CLK_CNTL1	(0x26 << 2)

As already pointed out for the axg clk series, please remove those
calculation from the header file.

> +
> +#endif /* __AXG_AOCLKC_H */
Jerome Brunet Feb. 13, 2018, 2:51 p.m. UTC | #2
On Mon, 2018-02-12 at 16:32 +0100, Jerome Brunet wrote:
> On Fri, 2018-02-09 at 15:00 +0800, Yixun Lan wrote:
> > Adds a Clock and Reset controller driver for the Always-On part
> > of the Amlogic Meson-AXG SoC.
> > 
> > Signed-off-by: Qiufang Dai <qiufang.dai@amlogic.com>
> > Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
> > ---
> >   drivers/clk/meson/Makefile    |   2 +-
> >   drivers/clk/meson/axg-aoclk.c | 236 ++++++++++++++++++++++++++++++++++++++++++
> >   drivers/clk/meson/axg-aoclk.h |  25 +++++
> >   3 files changed, 262 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/clk/meson/axg-aoclk.c
> >   create mode 100644 drivers/clk/meson/axg-aoclk.h

Yixun,

Actually, looking at this driver further, except for some of the data, the
driver is exactly the same as gxbb-aoclk. There is no reason to duplicate all
this code.

I'd prefer if you'd factor all this in a dedicated driver.
This philosophy is more or less the same as meson's pinctrl drivers, which all
provides their own data and bindings but share the driver code.

Regards
Jerome
Yixun Lan March 23, 2018, 2:13 p.m. UTC | #3
Hi Jerome:


On 02/12/2018 11:32 PM, Jerome Brunet wrote:
> On Fri, 2018-02-09 at 15:00 +0800, Yixun Lan wrote:
>> Adds a Clock and Reset controller driver for the Always-On part
>> of the Amlogic Meson-AXG SoC.
>>
>> Signed-off-by: Qiufang Dai <qiufang.dai@amlogic.com>
>> Signed-off-by: Yixun Lan <yixun.lan@amlogic.com>
>> ---
>>  drivers/clk/meson/Makefile    |   2 +-
>>  drivers/clk/meson/axg-aoclk.c | 236 ++++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/meson/axg-aoclk.h |  25 +++++
>>  3 files changed, 262 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/clk/meson/axg-aoclk.c
>>  create mode 100644 drivers/clk/meson/axg-aoclk.h
>>
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index 11f99139b844..c7510744406a 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -6,6 +6,6 @@ obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-audio-divider.o
>>  obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-phase.o
>>  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
>> +obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
>>  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
>>  obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
>> diff --git a/drivers/clk/meson/axg-aoclk.c b/drivers/clk/meson/axg-aoclk.c
>> new file mode 100644
>> index 000000000000..832aa19dd76c
>> --- /dev/null
>> +++ b/drivers/clk/meson/axg-aoclk.c
>> @@ -0,0 +1,236 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * AmLogic Meson-AXG Clock Controller Driver
> 
> Unless there a rule behind it, I think we should stop 
> with the funny camel case and stick to "Amlogic" like written below.
> 
will fix it

>> + *
>> + * Copyright (c) 2016 Baylibre SAS.
>> + * Author: Michael Turquette <mturquette@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
>> + */
>> +#include <linux/clk-provider.h>
>> +#include <linux/of_address.h>
> I did not see what is requiring this header. Did I miss something ?
> 
will drop this
>> +#include <linux/platform_device.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/regmap.h>
>> +#include <linux/init.h>
>> +#include <linux/delay.h>
> Same question for this one.
> 
ditto

>> +#include <dt-bindings/clock/axg-aoclkc.h>
>> +#include <dt-bindings/reset/axg-aoclkc.h>
> 
> I think those should be included in the same fashion as axg.h, gxbb.h and
> meson8.h ... which is at the end of the private header.
> 
will do

>> +#include "clkc.h"
>> +#include "axg-aoclk.h"
>> +
>> +struct axg_aoclk_reset_controller {
>> +	struct reset_controller_dev reset;
>> +	unsigned int *data;
>> +	struct regmap *regmap;
>> +};
> 
> This duplicate what is gxbb-aoclk.
> Please make something cleaner with a shared definition.
> 
as you already raised this in another email.
and I will factor the common code, and send out an updated version
>> +
>> +static int axg_aoclk_do_reset(struct reset_controller_dev *rcdev,
>> +			       unsigned long id)
>> +{
>> +	struct axg_aoclk_reset_controller *reset =
>> +		container_of(rcdev, struct axg_aoclk_reset_controller, reset);
>> +
>> +	return regmap_write(reset->regmap, AO_RTI_GEN_CNTL_REG0,
>> +			    BIT(reset->data[id]));
>> +}
>> +
>> +static const struct reset_control_ops axg_aoclk_reset_ops = {
>> +	.reset = axg_aoclk_do_reset,
>> +};
>> +
>> +#define AXG_AO_GATE(_name, _bit)					\
>> +static struct clk_regmap _name##_ao = {					\
>> +	.data = &(struct clk_regmap_gate_data) {			\
>> +		.offset = (AO_RTI_GEN_CNTL_REG0),			\
>> +		.bit_idx = (_bit),					\
>> +	},								\
>> +	.hw.init = &(struct clk_init_data) {				\
>> +		.name = #_name "_ao",					\
>> +		.ops = &clk_regmap_gate_ops,				\
>> +		.parent_names = (const char *[]){ "clk81" },		\
>> +		.num_parents = 1,					\
>> +		.flags = (CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED),	\
> 
> These clock are peripheral clocks (meant to enable/disable IPs):
> * CLK_SET_RATE_PARENT: why do you need rate propagation here ? do you really
> want a consumer of these gate to be able to change clk81 ? 
> 

No, we shouldn't change clk81, will drop this flag

> * CLK_IGNORE_UNUSED: Why do need this flag ? All the IPs behind this
> definition should be able to claim the pclk properly. If not the driver should
> be fixed.
> 
not really, we could drop this flag

but I'd like to address this in yet another thread (rather than in this
"add ao clock and reset driver")

>> +	},								\
>> +}
>> +
>> +AXG_AO_GATE(remote, 0);
>> +AXG_AO_GATE(i2c_master, 1);
>> +AXG_AO_GATE(i2c_slave, 2);
>> +AXG_AO_GATE(uart1, 3);
>> +AXG_AO_GATE(uart2, 5);
>> +AXG_AO_GATE(ir_blaster, 6);
>> +AXG_AO_GATE(saradc, 7);
>> +
>> +static struct clk_fixed_rate ao_alt_xtal = {
>> +	.fixed_rate = 32000,
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "ao_alt_xtal",
>> +		.num_parents = 0,
>> +		.ops = &clk_fixed_rate_ops,
>> +	},
>> +};
> 
> Unless this xtal is actually embedded in SoC, this should be defined it the DT,
> same as the other xtal.
> 
it's an external crystal, I will put it into DT file, thanks for hint

>> +
>> +static struct clk_regmap ao_clk81 = {
>> +	.data = &(struct clk_regmap_mux_data) {
>> +		.offset = AO_RTI_PWR_CNTL_REG0,
>> +		.mask = 0x1,
>> +		.shift = 8,
>> +	},
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "ao_clk81",
>> +		.ops = &clk_regmap_mux_ro_ops,
>> +		.parent_names = (const char *[]){ "clk81", "ao_alt_xtal"},
>> +		.num_parents = 2,
>> +	},
>> +};
>> +
>> +static struct clk_regmap axg_saradc_mux = {
>> +	.data = &(struct clk_regmap_mux_data) {
>> +		.offset = AO_SAR_CLK,
>> +		.mask = 0x3,
>> +		.shift = 9,
>> +	},
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "axg_saradc_mux",
>> +		.ops = &clk_regmap_mux_ops,
>> +		.parent_names = (const char *[]){ "xtal", "ao_clk81" },
>> +		.num_parents = 2,
>> +	},
>> +};
>> +
>> +static struct clk_regmap axg_saradc_div = {
>> +	.data = &(struct clk_regmap_div_data) {
>> +		.offset = AO_SAR_CLK,
>> +		.shift = 0,
>> +		.width = 8,
>> +	},
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "axg_saradc_div",
>> +		.ops = &clk_regmap_divider_ops,
>> +		.parent_names = (const char *[]){ "axg_saradc_mux" },
>> +		.num_parents = 1,
> 
> Should the saradc mux change to accomodate the requested rate, if needed ?
> If so, you might want to add a CLK_SET_RATE_PARENT here.
> 
yes, from hardware perspective, the mux can select clk parent, I will
update this

>> +	},
>> +};
>> +
>> +static struct clk_regmap axg_saradc_gate = {
>> +	.data = &(struct clk_regmap_gate_data) {
>> +		.offset = AO_SAR_CLK,
>> +		.bit_idx = 8,
>> +	},
>> +	.hw.init = &(struct clk_init_data){
>> +		.name = "axg_saradc_gate",
>> +		.ops = &clk_regmap_gate_ops,
>> +		.parent_names = (const char *[]){ "axg_saradc_div" },
>> +		.num_parents = 1,
>> +		.flags = CLK_SET_RATE_PARENT,
>> +	},
>> +};
>> +
>> +static unsigned int axg_aoclk_reset[] = {
>> +	[RESET_AO_REMOTE] = 16,
>> +	[RESET_AO_I2C_MASTER] = 18,
>> +	[RESET_AO_I2C_SLAVE] = 19,
>> +	[RESET_AO_UART1] = 17,
>> +	[RESET_AO_UART2] = 22,
>> +	[RESET_AO_IR_BLASTER] = 23,
>> +};
>> +
>> +static struct clk_regmap *axg_aoclk_regmap[] = {
>> +	[CLKID_AO_REMOTE]	= &remote_ao,
>> +	[CLKID_AO_I2C_MASTER]	= &i2c_master_ao,
>> +	[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao,
>> +	[CLKID_AO_UART1]	= &uart1_ao,
>> +	[CLKID_AO_UART2]	= &uart2_ao,
>> +	[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao,
>> +	[CLKID_AO_SAR_ADC]	= &saradc_ao,
>> +	[CLKID_AO_CLK81]	= &ao_clk81,
>> +	[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux,
>> +	[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div,
>> +	[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate,
>> +};
>> +
>> +static struct clk_hw_onecell_data axg_aoclk_onecell_data = {
>> +	.hws = {
>> +		[CLKID_AO_REMOTE]	= &remote_ao.hw,
>> +		[CLKID_AO_I2C_MASTER]	= &i2c_master_ao.hw,
>> +		[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao.hw,
>> +		[CLKID_AO_UART1]	= &uart1_ao.hw,
>> +		[CLKID_AO_UART2]	= &uart2_ao.hw,
>> +		[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao.hw,
>> +		[CLKID_AO_SAR_ADC]	= &saradc_ao.hw,
>> +		[CLKID_AO_CLK81]	= &ao_clk81.hw,
>> +		[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux.hw,
>> +		[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div.hw,
>> +		[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate.hw,
>> +		[CLKID_AO_ALT_XTAL]	= &ao_alt_xtal.hw,
>> +	},
>> +	.num = 12,
> 
> I would prefer having a NR_CLKS defined in drivers/clk/meson/axg-aoclk.h
> 
can do

>> +};
>> +
>> +static int axg_aoclkc_probe(struct platform_device *pdev)
>> +{
>> +	struct axg_aoclk_reset_controller *rstc;
>> +	struct device *dev = &pdev->dev;
>> +	struct regmap *regmap;
>> +	int ret, clkid;
>> +
>> +	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
>> +	if (!rstc)
>> +		return -ENOMEM;
>> +
>> +	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
>> +	if (IS_ERR(regmap)) {
>> +		dev_err(dev, "failed to get regmap\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* Reset Controller */
>> +	rstc->regmap = regmap;
>> +	rstc->data = axg_aoclk_reset;
>> +	rstc->reset.ops = &axg_aoclk_reset_ops;
>> +	rstc->reset.nr_resets = ARRAY_SIZE(axg_aoclk_reset);
>> +	rstc->reset.of_node = dev->of_node;
>> +	ret = devm_reset_controller_register(dev, &rstc->reset);
>> +
>> +	/*
>> +	 * Populate regmap and register all clks
>> +	 */
>> +	for (clkid = 0; clkid < ARRAY_SIZE(axg_aoclk_regmap); clkid++) {
>> +		axg_aoclk_regmap[clkid]->map = regmap;
>> +
>> +		ret = devm_clk_hw_register(dev,
>> +					axg_aoclk_onecell_data.hws[clkid]);
>> +		if (ret) {
>> +			dev_err(dev, "clk register failed.\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	/* Specific clocks */
>> +	ret = devm_clk_hw_register(dev, &ao_alt_xtal.hw);
>> +	if (ret) {
>> +		dev_err(dev, "clk alt_xtal register failed.\n");
>> +		return ret;
>> +	}
>> +
>> +	return of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
>> +			&axg_aoclk_onecell_data);
>> +}
>> +
>> +static const struct of_device_id axg_aoclkc_match_table[] = {
>> +	{ .compatible = "amlogic,meson-axg-aoclkc" },
>> +	{ }
>> +};
>> +
>> +static struct platform_driver axg_aoclkc_driver = {
>> +	.probe		= axg_aoclkc_probe,
>> +	.driver		= {
>> +		.name	= "axg-aoclkc",
>> +		.of_match_table = axg_aoclkc_match_table,
>> +	},
>> +};
>> +
>> +builtin_platform_driver(axg_aoclkc_driver);
>> diff --git a/drivers/clk/meson/axg-aoclk.h b/drivers/clk/meson/axg-aoclk.h
>> new file mode 100644
>> index 000000000000..70f82004d3db
>> --- /dev/null
>> +++ b/drivers/clk/meson/axg-aoclk.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +/*
>> + * Copyright (c) 2017 BayLibre, SAS
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + *
>> + * Copyright (c) 2018 Amlogic, inc.
>> + * Author: Qiufang Dai <qiufang.dai@amlogic.com>
>> + */
>> +
>> +#ifndef __AXG_AOCLKC_H
>> +#define __AXG_AOCLKC_H
>> +
>> +/* AO Configuration Clock registers offsets
>> + * Register offsets from the data sheet must be multiplied by 4.
>> + */
>> +#define AO_RTI_PWR_CNTL_REG1	(0x03 << 2)
>> +#define AO_RTI_PWR_CNTL_REG0	(0x04 << 2)
>> +#define AO_RTI_GEN_CNTL_REG0	(0x10 << 2)
>> +#define AO_OSCIN_CNTL		(0x16 << 2)
>> +#define AO_CRT_CLK_CNTL1	(0x1a << 2)
>> +#define AO_SAR_CLK		(0x24 << 2)
>> +#define AO_RTC_ALT_CLK_CNTL0	(0x25 << 2)
>> +#define AO_RTC_ALT_CLK_CNTL1	(0x26 << 2)
> 
> As already pointed out for the axg clk series, please remove those
> calculation from the header file.
> 
Ok, will update
>> +
>> +#endif /* __AXG_AOCLKC_H */
> 
> .
>
diff mbox

Patch

diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 11f99139b844..c7510744406a 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -6,6 +6,6 @@  obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-mpll.o clk-audio-divider.o
 obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-phase.o
 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
+obj-$(CONFIG_COMMON_CLK_AXG)	 += axg.o axg-aoclk.o
 obj-$(CONFIG_COMMON_CLK_AXG_AUDIO)	+= axg-audio.o
 obj-$(CONFIG_COMMON_CLK_REGMAP_MESON)	+= clk-regmap.o
diff --git a/drivers/clk/meson/axg-aoclk.c b/drivers/clk/meson/axg-aoclk.c
new file mode 100644
index 000000000000..832aa19dd76c
--- /dev/null
+++ b/drivers/clk/meson/axg-aoclk.c
@@ -0,0 +1,236 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AmLogic Meson-AXG Clock Controller Driver
+ *
+ * Copyright (c) 2016 Baylibre SAS.
+ * Author: Michael Turquette <mturquette@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Qiufang Dai <qiufang.dai@amlogic.com>
+ */
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <dt-bindings/clock/axg-aoclkc.h>
+#include <dt-bindings/reset/axg-aoclkc.h>
+#include "clkc.h"
+#include "axg-aoclk.h"
+
+struct axg_aoclk_reset_controller {
+	struct reset_controller_dev reset;
+	unsigned int *data;
+	struct regmap *regmap;
+};
+
+static int axg_aoclk_do_reset(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct axg_aoclk_reset_controller *reset =
+		container_of(rcdev, struct axg_aoclk_reset_controller, reset);
+
+	return regmap_write(reset->regmap, AO_RTI_GEN_CNTL_REG0,
+			    BIT(reset->data[id]));
+}
+
+static const struct reset_control_ops axg_aoclk_reset_ops = {
+	.reset = axg_aoclk_do_reset,
+};
+
+#define AXG_AO_GATE(_name, _bit)					\
+static struct clk_regmap _name##_ao = {					\
+	.data = &(struct clk_regmap_gate_data) {			\
+		.offset = (AO_RTI_GEN_CNTL_REG0),			\
+		.bit_idx = (_bit),					\
+	},								\
+	.hw.init = &(struct clk_init_data) {				\
+		.name = #_name "_ao",					\
+		.ops = &clk_regmap_gate_ops,				\
+		.parent_names = (const char *[]){ "clk81" },		\
+		.num_parents = 1,					\
+		.flags = (CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED),	\
+	},								\
+}
+
+AXG_AO_GATE(remote, 0);
+AXG_AO_GATE(i2c_master, 1);
+AXG_AO_GATE(i2c_slave, 2);
+AXG_AO_GATE(uart1, 3);
+AXG_AO_GATE(uart2, 5);
+AXG_AO_GATE(ir_blaster, 6);
+AXG_AO_GATE(saradc, 7);
+
+static struct clk_fixed_rate ao_alt_xtal = {
+	.fixed_rate = 32000,
+	.hw.init = &(struct clk_init_data){
+		.name = "ao_alt_xtal",
+		.num_parents = 0,
+		.ops = &clk_fixed_rate_ops,
+	},
+};
+
+static struct clk_regmap ao_clk81 = {
+	.data = &(struct clk_regmap_mux_data) {
+		.offset = AO_RTI_PWR_CNTL_REG0,
+		.mask = 0x1,
+		.shift = 8,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "ao_clk81",
+		.ops = &clk_regmap_mux_ro_ops,
+		.parent_names = (const char *[]){ "clk81", "ao_alt_xtal"},
+		.num_parents = 2,
+	},
+};
+
+static struct clk_regmap axg_saradc_mux = {
+	.data = &(struct clk_regmap_mux_data) {
+		.offset = AO_SAR_CLK,
+		.mask = 0x3,
+		.shift = 9,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "axg_saradc_mux",
+		.ops = &clk_regmap_mux_ops,
+		.parent_names = (const char *[]){ "xtal", "ao_clk81" },
+		.num_parents = 2,
+	},
+};
+
+static struct clk_regmap axg_saradc_div = {
+	.data = &(struct clk_regmap_div_data) {
+		.offset = AO_SAR_CLK,
+		.shift = 0,
+		.width = 8,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "axg_saradc_div",
+		.ops = &clk_regmap_divider_ops,
+		.parent_names = (const char *[]){ "axg_saradc_mux" },
+		.num_parents = 1,
+	},
+};
+
+static struct clk_regmap axg_saradc_gate = {
+	.data = &(struct clk_regmap_gate_data) {
+		.offset = AO_SAR_CLK,
+		.bit_idx = 8,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "axg_saradc_gate",
+		.ops = &clk_regmap_gate_ops,
+		.parent_names = (const char *[]){ "axg_saradc_div" },
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static unsigned int axg_aoclk_reset[] = {
+	[RESET_AO_REMOTE] = 16,
+	[RESET_AO_I2C_MASTER] = 18,
+	[RESET_AO_I2C_SLAVE] = 19,
+	[RESET_AO_UART1] = 17,
+	[RESET_AO_UART2] = 22,
+	[RESET_AO_IR_BLASTER] = 23,
+};
+
+static struct clk_regmap *axg_aoclk_regmap[] = {
+	[CLKID_AO_REMOTE]	= &remote_ao,
+	[CLKID_AO_I2C_MASTER]	= &i2c_master_ao,
+	[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao,
+	[CLKID_AO_UART1]	= &uart1_ao,
+	[CLKID_AO_UART2]	= &uart2_ao,
+	[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao,
+	[CLKID_AO_SAR_ADC]	= &saradc_ao,
+	[CLKID_AO_CLK81]	= &ao_clk81,
+	[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux,
+	[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div,
+	[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate,
+};
+
+static struct clk_hw_onecell_data axg_aoclk_onecell_data = {
+	.hws = {
+		[CLKID_AO_REMOTE]	= &remote_ao.hw,
+		[CLKID_AO_I2C_MASTER]	= &i2c_master_ao.hw,
+		[CLKID_AO_I2C_SLAVE]	= &i2c_slave_ao.hw,
+		[CLKID_AO_UART1]	= &uart1_ao.hw,
+		[CLKID_AO_UART2]	= &uart2_ao.hw,
+		[CLKID_AO_IR_BLASTER]	= &ir_blaster_ao.hw,
+		[CLKID_AO_SAR_ADC]	= &saradc_ao.hw,
+		[CLKID_AO_CLK81]	= &ao_clk81.hw,
+		[CLKID_AO_SAR_ADC_SEL]	= &axg_saradc_mux.hw,
+		[CLKID_AO_SAR_ADC_DIV]	= &axg_saradc_div.hw,
+		[CLKID_AO_SAR_ADC_CLK]	= &axg_saradc_gate.hw,
+		[CLKID_AO_ALT_XTAL]	= &ao_alt_xtal.hw,
+	},
+	.num = 12,
+};
+
+static int axg_aoclkc_probe(struct platform_device *pdev)
+{
+	struct axg_aoclk_reset_controller *rstc;
+	struct device *dev = &pdev->dev;
+	struct regmap *regmap;
+	int ret, clkid;
+
+	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
+	if (!rstc)
+		return -ENOMEM;
+
+	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "failed to get regmap\n");
+		return -ENODEV;
+	}
+
+	/* Reset Controller */
+	rstc->regmap = regmap;
+	rstc->data = axg_aoclk_reset;
+	rstc->reset.ops = &axg_aoclk_reset_ops;
+	rstc->reset.nr_resets = ARRAY_SIZE(axg_aoclk_reset);
+	rstc->reset.of_node = dev->of_node;
+	ret = devm_reset_controller_register(dev, &rstc->reset);
+
+	/*
+	 * Populate regmap and register all clks
+	 */
+	for (clkid = 0; clkid < ARRAY_SIZE(axg_aoclk_regmap); clkid++) {
+		axg_aoclk_regmap[clkid]->map = regmap;
+
+		ret = devm_clk_hw_register(dev,
+					axg_aoclk_onecell_data.hws[clkid]);
+		if (ret) {
+			dev_err(dev, "clk register failed.\n");
+			return ret;
+		}
+	}
+
+	/* Specific clocks */
+	ret = devm_clk_hw_register(dev, &ao_alt_xtal.hw);
+	if (ret) {
+		dev_err(dev, "clk alt_xtal register failed.\n");
+		return ret;
+	}
+
+	return of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
+			&axg_aoclk_onecell_data);
+}
+
+static const struct of_device_id axg_aoclkc_match_table[] = {
+	{ .compatible = "amlogic,meson-axg-aoclkc" },
+	{ }
+};
+
+static struct platform_driver axg_aoclkc_driver = {
+	.probe		= axg_aoclkc_probe,
+	.driver		= {
+		.name	= "axg-aoclkc",
+		.of_match_table = axg_aoclkc_match_table,
+	},
+};
+
+builtin_platform_driver(axg_aoclkc_driver);
diff --git a/drivers/clk/meson/axg-aoclk.h b/drivers/clk/meson/axg-aoclk.h
new file mode 100644
index 000000000000..70f82004d3db
--- /dev/null
+++ b/drivers/clk/meson/axg-aoclk.h
@@ -0,0 +1,25 @@ 
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * Copyright (c) 2017 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Qiufang Dai <qiufang.dai@amlogic.com>
+ */
+
+#ifndef __AXG_AOCLKC_H
+#define __AXG_AOCLKC_H
+
+/* AO Configuration Clock registers offsets
+ * Register offsets from the data sheet must be multiplied by 4.
+ */
+#define AO_RTI_PWR_CNTL_REG1	(0x03 << 2)
+#define AO_RTI_PWR_CNTL_REG0	(0x04 << 2)
+#define AO_RTI_GEN_CNTL_REG0	(0x10 << 2)
+#define AO_OSCIN_CNTL		(0x16 << 2)
+#define AO_CRT_CLK_CNTL1	(0x1a << 2)
+#define AO_SAR_CLK		(0x24 << 2)
+#define AO_RTC_ALT_CLK_CNTL0	(0x25 << 2)
+#define AO_RTC_ALT_CLK_CNTL1	(0x26 << 2)
+
+#endif /* __AXG_AOCLKC_H */