Re: [PATCH 2/2] clk: meson-axg: Add AO Clock and Reset controller driver
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>> Signed-off-by: Yixun Lan >> --- >> 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 ..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 >> + * >> + * Copyright (c) 2018 Amlogic, inc. >> + * Author: Qiufang Dai >> + */ >> +#include >> +#include > I did not see what is requiring this header. Did I miss something ? > will drop this >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > Same question for this one. > ditto >> +#include >> +#include > > 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 = _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,
Re: [PATCH 2/2] clk: meson-axg: Add AO Clock and Reset controller driver
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 >> Signed-off-by: Yixun Lan >> --- >> 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 ..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 >> + * >> + * Copyright (c) 2018 Amlogic, inc. >> + * Author: Qiufang Dai >> + */ >> +#include >> +#include > I did not see what is requiring this header. Did I miss something ? > will drop this >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include > Same question for this one. > ditto >> +#include >> +#include > > 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 = _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); >>
Re: [PATCH 2/2] clk: meson-axg: Add AO Clock and Reset controller driver
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> > Signed-off-by: Yixun Lan > > --- > > 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
Re: [PATCH 2/2] clk: meson-axg: Add AO Clock and Reset controller driver
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 > > Signed-off-by: Yixun Lan > > --- > > 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
Re: [PATCH 2/2] clk: meson-axg: Add AO Clock and Reset controller driver
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> Signed-off-by: Yixun Lan > --- > 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 ..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 > + * > + * Copyright (c) 2018 Amlogic, inc. > + * Author: Qiufang Dai > + */ > +#include > +#include I did not see what is requiring this header. Did I miss something ? > +#include > +#include > +#include > +#include > +#include > +#include Same question for this one. > +#include > +#include 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 = _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 = _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
Re: [PATCH 2/2] clk: meson-axg: Add AO Clock and Reset controller driver
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 > Signed-off-by: Yixun Lan > --- > 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 ..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 > + * > + * Copyright (c) 2018 Amlogic, inc. > + * Author: Qiufang Dai > + */ > +#include > +#include I did not see what is requiring this header. Did I miss something ? > +#include > +#include > +#include > +#include > +#include > +#include Same question for this one. > +#include > +#include 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 = _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 = _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 =