* [PATCH 0/2] clk: meson: g12a: Add CPU Clock support @ 2019-03-01 10:21 Neil Armstrong 2019-03-01 10:21 ` [PATCH 1/2] clk: meson-g12a: add cpu clock bindings Neil Armstrong 2019-03-01 10:21 ` [PATCH 2/2] clk: meson: g12a: add cpu clocks Neil Armstrong 0 siblings, 2 replies; 9+ messages in thread From: Neil Armstrong @ 2019-03-01 10:21 UTC (permalink / raw) To: jbrunet Cc: Neil Armstrong, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel This patchset adds support for the clock tree feeding the 4xCortex A53 cpu cluster. This patchet does not handle clock changing, this will be added in a secondary patchset. The CPU clock can either use the SYS_PLL for > 1GHz frequencies or use a couple of div+mux from 1GHz/667MHz/24MHz source with 2 non-glitch muxes. The CPU clock must be switched to a safe clock while changing the clocks before the non-glitch muxes. Proper support will be added later. In this patchset, clocks are set read-only. Neil Armstrong (2): clk: meson-g12a: add cpu clock bindings clk: meson: g12a: add cpu clocks drivers/clk/meson/g12a.c | 348 ++++++++++++++++++++++++++ drivers/clk/meson/g12a.h | 25 +- include/dt-bindings/clock/g12a-clkc.h | 1 + 3 files changed, 373 insertions(+), 1 deletion(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] clk: meson-g12a: add cpu clock bindings 2019-03-01 10:21 [PATCH 0/2] clk: meson: g12a: Add CPU Clock support Neil Armstrong @ 2019-03-01 10:21 ` Neil Armstrong 2019-03-01 15:26 ` Martin Blumenstingl 2019-03-01 10:21 ` [PATCH 2/2] clk: meson: g12a: add cpu clocks Neil Armstrong 1 sibling, 1 reply; 9+ messages in thread From: Neil Armstrong @ 2019-03-01 10:21 UTC (permalink / raw) To: jbrunet, devicetree Cc: Neil Armstrong, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel Add Amlogic G12A Family CPU clocks bindings, only export CPU_CLK since it should be the only ID used. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/clk/meson/g12a.h | 24 +++++++++++++++++++++++- include/dt-bindings/clock/g12a-clkc.h | 1 + 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/drivers/clk/meson/g12a.h b/drivers/clk/meson/g12a.h index f399dfe1401c..4854750df902 100644 --- a/drivers/clk/meson/g12a.h +++ b/drivers/clk/meson/g12a.h @@ -166,8 +166,30 @@ #define CLKID_MALI_0_DIV 170 #define CLKID_MALI_1_DIV 173 #define CLKID_MPLL_5OM_DIV 176 +#define CLKID_PCIE_PLL_DCO 178 +#define CLKID_PCIE_PLL_DCO_DIV2 179 +#define CLKID_PCIE_PLL_OD 180 +#define CLKID_SYS_PLL_DIV16_EN 181 +#define CLKID_SYS_PLL_DIV16 182 +#define CLKID_CPU_CLK_DYN0_SEL 183 +#define CLKID_CPU_CLK_DYN0_DIV 184 +#define CLKID_CPU_CLK_DYN0 185 +#define CLKID_CPU_CLK_DYN1_SEL 186 +#define CLKID_CPU_CLK_DYN1_DIV 187 +#define CLKID_CPU_CLK_DYN1 188 +#define CLKID_CPU_CLK_DYN 189 +#define CLKID_CPU_CLK_DIV16_EN 191 +#define CLKID_CPU_CLK_DIV16 192 +#define CLKID_CPU_CLK_APB_DIV 193 +#define CLKID_CPU_CLK_APB 194 +#define CLKID_CPU_CLK_ATB_DIV 195 +#define CLKID_CPU_CLK_ATB 196 +#define CLKID_CPU_CLK_AXI_DIV 197 +#define CLKID_CPU_CLK_AXI 198 +#define CLKID_CPU_CLK_TRACE_DIV 299 +#define CLKID_CPU_CLK_TRACE 200 -#define NR_CLKS 178 +#define NR_CLKS 201 /* include the CLKIDs that have been made part of the DT binding */ #include <dt-bindings/clock/g12a-clkc.h> diff --git a/include/dt-bindings/clock/g12a-clkc.h b/include/dt-bindings/clock/g12a-clkc.h index 83b657038d1e..33aba232282c 100644 --- a/include/dt-bindings/clock/g12a-clkc.h +++ b/include/dt-bindings/clock/g12a-clkc.h @@ -131,5 +131,6 @@ #define CLKID_MALI_1 174 #define CLKID_MALI 175 #define CLKID_MPLL_5OM 177 +#define CLKID_CPU_CLK 190 #endif /* __G12A_CLKC_H */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: meson-g12a: add cpu clock bindings 2019-03-01 10:21 ` [PATCH 1/2] clk: meson-g12a: add cpu clock bindings Neil Armstrong @ 2019-03-01 15:26 ` Martin Blumenstingl 2019-03-01 16:43 ` Neil Armstrong 0 siblings, 1 reply; 9+ messages in thread From: Martin Blumenstingl @ 2019-03-01 15:26 UTC (permalink / raw) To: Neil Armstrong Cc: jbrunet, devicetree, linux-amlogic, linux-kernel, linux-clk, linux-arm-kernel Hi Neil, On Fri, Mar 1, 2019 at 11:22 AM Neil Armstrong <narmstrong@baylibre.com> wrote: > > Add Amlogic G12A Family CPU clocks bindings, only export CPU_CLK since > it should be the only ID used. is this also true for the CPU post-dividers (APB, ATB, AXI, CPU CLK TRACE)? > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/clk/meson/g12a.h | 24 +++++++++++++++++++++++- > include/dt-bindings/clock/g12a-clkc.h | 1 + > 2 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/meson/g12a.h b/drivers/clk/meson/g12a.h > index f399dfe1401c..4854750df902 100644 > --- a/drivers/clk/meson/g12a.h > +++ b/drivers/clk/meson/g12a.h > @@ -166,8 +166,30 @@ > #define CLKID_MALI_0_DIV 170 > #define CLKID_MALI_1_DIV 173 > #define CLKID_MPLL_5OM_DIV 176 > +#define CLKID_PCIE_PLL_DCO 178 > +#define CLKID_PCIE_PLL_DCO_DIV2 179 > +#define CLKID_PCIE_PLL_OD 180 how are these PCIe clock related to the CPU clocks? > +#define CLKID_SYS_PLL_DIV16_EN 181 > +#define CLKID_SYS_PLL_DIV16 182 > +#define CLKID_CPU_CLK_DYN0_SEL 183 > +#define CLKID_CPU_CLK_DYN0_DIV 184 > +#define CLKID_CPU_CLK_DYN0 185 > +#define CLKID_CPU_CLK_DYN1_SEL 186 > +#define CLKID_CPU_CLK_DYN1_DIV 187 > +#define CLKID_CPU_CLK_DYN1 188 > +#define CLKID_CPU_CLK_DYN 189 > +#define CLKID_CPU_CLK_DIV16_EN 191 > +#define CLKID_CPU_CLK_DIV16 192 > +#define CLKID_CPU_CLK_APB_DIV 193 > +#define CLKID_CPU_CLK_APB 194 > +#define CLKID_CPU_CLK_ATB_DIV 195 > +#define CLKID_CPU_CLK_ATB 196 > +#define CLKID_CPU_CLK_AXI_DIV 197 > +#define CLKID_CPU_CLK_AXI 198 > +#define CLKID_CPU_CLK_TRACE_DIV 299 > +#define CLKID_CPU_CLK_TRACE 200 > > -#define NR_CLKS 178 > +#define NR_CLKS 201 shouldn't all changes to this file (drivers/clk/meson/g12a.h) be part of the patch which adds the actual clocks so the dt-bindings patch is independent of the clock driver patches? in this case the subject should also be updated to "dt-bindings: clock: g12a: ..." Regards Martin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: meson-g12a: add cpu clock bindings 2019-03-01 15:26 ` Martin Blumenstingl @ 2019-03-01 16:43 ` Neil Armstrong 2019-03-01 17:05 ` Martin Blumenstingl 0 siblings, 1 reply; 9+ messages in thread From: Neil Armstrong @ 2019-03-01 16:43 UTC (permalink / raw) To: Martin Blumenstingl Cc: jbrunet, devicetree, linux-amlogic, linux-kernel, linux-clk, linux-arm-kernel Hi Martin, On 01/03/2019 16:26, Martin Blumenstingl wrote: > Hi Neil, > > On Fri, Mar 1, 2019 at 11:22 AM Neil Armstrong <narmstrong@baylibre.com> wrote: >> >> Add Amlogic G12A Family CPU clocks bindings, only export CPU_CLK since >> it should be the only ID used. > is this also true for the CPU post-dividers (APB, ATB, AXI, CPU CLK TRACE)? Do we need these to be exported ? > >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/clk/meson/g12a.h | 24 +++++++++++++++++++++++- >> include/dt-bindings/clock/g12a-clkc.h | 1 + >> 2 files changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/meson/g12a.h b/drivers/clk/meson/g12a.h >> index f399dfe1401c..4854750df902 100644 >> --- a/drivers/clk/meson/g12a.h >> +++ b/drivers/clk/meson/g12a.h >> @@ -166,8 +166,30 @@ >> #define CLKID_MALI_0_DIV 170 >> #define CLKID_MALI_1_DIV 173 >> #define CLKID_MPLL_5OM_DIV 176 >> +#define CLKID_PCIE_PLL_DCO 178 >> +#define CLKID_PCIE_PLL_DCO_DIV2 179 >> +#define CLKID_PCIE_PLL_OD 180 > how are these PCIe clock related to the CPU clocks? Oops, bad merge... > >> +#define CLKID_SYS_PLL_DIV16_EN 181 >> +#define CLKID_SYS_PLL_DIV16 182 >> +#define CLKID_CPU_CLK_DYN0_SEL 183 >> +#define CLKID_CPU_CLK_DYN0_DIV 184 >> +#define CLKID_CPU_CLK_DYN0 185 >> +#define CLKID_CPU_CLK_DYN1_SEL 186 >> +#define CLKID_CPU_CLK_DYN1_DIV 187 >> +#define CLKID_CPU_CLK_DYN1 188 >> +#define CLKID_CPU_CLK_DYN 189 >> +#define CLKID_CPU_CLK_DIV16_EN 191 >> +#define CLKID_CPU_CLK_DIV16 192 >> +#define CLKID_CPU_CLK_APB_DIV 193 >> +#define CLKID_CPU_CLK_APB 194 >> +#define CLKID_CPU_CLK_ATB_DIV 195 >> +#define CLKID_CPU_CLK_ATB 196 >> +#define CLKID_CPU_CLK_AXI_DIV 197 >> +#define CLKID_CPU_CLK_AXI 198 >> +#define CLKID_CPU_CLK_TRACE_DIV 299 >> +#define CLKID_CPU_CLK_TRACE 200 >> >> -#define NR_CLKS 178 >> +#define NR_CLKS 201 > shouldn't all changes to this file (drivers/clk/meson/g12a.h) be part > of the patch which adds the actual clocks so the dt-bindings patch is > independent of the clock driver patches? > in this case the subject should also be updated to "dt-bindings: > clock: g12a: ..." I don't have any opinion, Jerome ? > > > Regards > Martin > Thanks ! Neil ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: meson-g12a: add cpu clock bindings 2019-03-01 16:43 ` Neil Armstrong @ 2019-03-01 17:05 ` Martin Blumenstingl 0 siblings, 0 replies; 9+ messages in thread From: Martin Blumenstingl @ 2019-03-01 17:05 UTC (permalink / raw) To: Neil Armstrong Cc: jbrunet, devicetree, linux-amlogic, linux-kernel, linux-clk, linux-arm-kernel Hi Neil, On Fri, Mar 1, 2019 at 5:43 PM Neil Armstrong <narmstrong@baylibre.com> wrote: > > Hi Martin, > > On 01/03/2019 16:26, Martin Blumenstingl wrote: > > Hi Neil, > > > > On Fri, Mar 1, 2019 at 11:22 AM Neil Armstrong <narmstrong@baylibre.com> wrote: > >> > >> Add Amlogic G12A Family CPU clocks bindings, only export CPU_CLK since > >> it should be the only ID used. > > is this also true for the CPU post-dividers (APB, ATB, AXI, CPU CLK TRACE)? > > Do we need these to be exported ? I'm not sure as I couldn't find more details about APB, ATB and AXI on G12A: - APB and ATB may be needed by the CoreSight bindings (Documentation/devicetree/bindings/arm/coresight.txt) - AXI may be needed by the VPU driver (the S912 datasheet mentions in OSD1_AFBCD_ENABLE: "id_fifo_thrd : unsigned , default = 64, axi id fifo threshold") if you don't know either then I'm fine with skipping them for now, we can still export them later. Martin ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] clk: meson: g12a: add cpu clocks 2019-03-01 10:21 [PATCH 0/2] clk: meson: g12a: Add CPU Clock support Neil Armstrong 2019-03-01 10:21 ` [PATCH 1/2] clk: meson-g12a: add cpu clock bindings Neil Armstrong @ 2019-03-01 10:21 ` Neil Armstrong 2019-03-01 15:21 ` Martin Blumenstingl 1 sibling, 1 reply; 9+ messages in thread From: Neil Armstrong @ 2019-03-01 10:21 UTC (permalink / raw) To: jbrunet Cc: Neil Armstrong, linux-clk, linux-amlogic, linux-arm-kernel, linux-kernel Add the Amlogic G12A Family CPU Clock tree in read/only for now. The CPU clock can either use the SYS_PLL for > 1GHz frequencies or use a couple of div+mux from 1GHz/667MHz/24MHz source with 2 non-glitch muxes. Proper DVFS support will come in a second time. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/clk/meson/g12a.c | 348 +++++++++++++++++++++++++++++++++++++++ drivers/clk/meson/g12a.h | 1 + 2 files changed, 349 insertions(+) diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c index 0e1ce8c03259..4c938f1b8421 100644 --- a/drivers/clk/meson/g12a.c +++ b/drivers/clk/meson/g12a.c @@ -150,6 +150,316 @@ static struct clk_regmap g12a_sys_pll = { }, }; +static struct clk_regmap g12a_sys_pll_div16_en = { + .data = &(struct clk_regmap_gate_data){ + .offset = HHI_SYS_CPU_CLK_CNTL1, + .bit_idx = 24, + }, + .hw.init = &(struct clk_init_data) { + .name = "sys_pll_div16_en", + .ops = &clk_regmap_gate_ops, + .parent_names = (const char *[]){ "sys_pll" }, + .num_parents = 1, + /* + * This clock is used to debug the sys_pll range + * Linux should not change it at runtime + */ + .flags = CLK_IGNORE_UNUSED, + }, +}; + +static struct clk_fixed_factor g12a_sys_pll_div16 = { + .mult = 1, + .div = 16, + .hw.init = &(struct clk_init_data){ + .name = "sys_pll_div16", + .ops = &clk_fixed_factor_ops, + .parent_names = (const char *[]){ "sys_pll_div16_en" }, + .num_parents = 1, + }, +}; + +static struct clk_regmap g12a_cpu_clk_dyn0_sel = { + .data = &(struct clk_regmap_mux_data){ + .offset = HHI_SYS_CPU_CLK_CNTL0, + .mask = 0x3, + .shift = 0, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_dyn0_sel", + .ops = &clk_regmap_mux_ro_ops, + .parent_names = (const char *[]){ IN_PREFIX "xtal", + "fclk_div2", + "fclk_div3" }, + .num_parents = 3, + }, +}; + +static struct clk_regmap g12a_cpu_clk_dyn0_div = { + .data = &(struct clk_regmap_div_data){ + .offset = HHI_SYS_CPU_CLK_CNTL0, + .shift = 4, + .width = 6, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_dyn0_div", + .ops = &clk_regmap_divider_ro_ops, + .parent_names = (const char *[]){ "cpu_clk_dyn0_sel" }, + .num_parents = 1, + }, +}; + +static struct clk_regmap g12a_cpu_clk_dyn0 = { + .data = &(struct clk_regmap_mux_data){ + .offset = HHI_SYS_CPU_CLK_CNTL0, + .mask = 0x1, + .shift = 2, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_dyn0", + .ops = &clk_regmap_mux_ro_ops, + .parent_names = (const char *[]){ "cpu_clk_dyn0_sel", + "cpu_clk_dyn0_div" }, + .num_parents = 2, + }, +}; + +static struct clk_regmap g12a_cpu_clk_dyn1_sel = { + .data = &(struct clk_regmap_mux_data){ + .offset = HHI_SYS_CPU_CLK_CNTL0, + .mask = 0x3, + .shift = 16, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_dyn1_sel", + .ops = &clk_regmap_mux_ro_ops, + .parent_names = (const char *[]){ IN_PREFIX "xtal", + "fclk_div2", + "fclk_div3" }, + .num_parents = 3, + }, +}; + +static struct clk_regmap g12a_cpu_clk_dyn1_div = { + .data = &(struct clk_regmap_div_data){ + .offset = HHI_SYS_CPU_CLK_CNTL0, + .shift = 20, + .width = 6, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_dyn1_div", + .ops = &clk_regmap_divider_ro_ops, + .parent_names = (const char *[]){ "cpu_clk_dyn1_sel" }, + .num_parents = 1, + }, +}; + +static struct clk_regmap g12a_cpu_clk_dyn1 = { + .data = &(struct clk_regmap_mux_data){ + .offset = HHI_SYS_CPU_CLK_CNTL0, + .mask = 0x1, + .shift = 18, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_dyn1", + .ops = &clk_regmap_mux_ro_ops, + .parent_names = (const char *[]){ "cpu_clk_dyn1_sel", + "cpu_clk_dyn1_div" }, + .num_parents = 2, + }, +}; + +static struct clk_regmap g12a_cpu_clk_dyn = { + .data = &(struct clk_regmap_mux_data){ + .offset = HHI_SYS_CPU_CLK_CNTL0, + .mask = 0x1, + .shift = 10, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_dyn", + .ops = &clk_regmap_mux_ro_ops, + .parent_names = (const char *[]){ "cpu_clk_dyn0", + "cpu_clk_dyn1" }, + .num_parents = 2, + }, +}; + +static struct clk_regmap g12a_cpu_clk = { + .data = &(struct clk_regmap_mux_data){ + .offset = HHI_SYS_CPU_CLK_CNTL0, + .mask = 0x1, + .shift = 11, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk", + .ops = &clk_regmap_mux_ro_ops, + .parent_names = (const char *[]){ "cpu_clk_dyn", + "sys_pll" }, + .num_parents = 2, + }, +}; + +static struct clk_regmap g12a_cpu_clk_div16_en = { + .data = &(struct clk_regmap_gate_data){ + .offset = HHI_SYS_CPU_CLK_CNTL1, + .bit_idx = 1, + }, + .hw.init = &(struct clk_init_data) { + .name = "cpu_clk_div16_en", + .ops = &clk_regmap_gate_ops, + .parent_names = (const char *[]){ "cpu_clk" }, + .num_parents = 1, + /* + * This clock is used to debug the cpu_clk range + * Linux should not change it at runtime + */ + .flags = CLK_IGNORE_UNUSED, + }, +}; + +static struct clk_fixed_factor g12a_cpu_clk_div16 = { + .mult = 1, + .div = 16, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_div16", + .ops = &clk_fixed_factor_ops, + .parent_names = (const char *[]){ "cpu_clk_div16_en" }, + .num_parents = 1, + }, +}; + +static struct clk_regmap g12a_cpu_clk_apb_div = { + .data = &(struct clk_regmap_div_data){ + .offset = HHI_SYS_CPU_CLK_CNTL1, + .shift = 3, + .width = 3, + .flags = CLK_DIVIDER_POWER_OF_TWO, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_apb_div", + .ops = &clk_regmap_divider_ro_ops, + .parent_names = (const char *[]){ "cpu_clk" }, + .num_parents = 1, + }, +}; + +static struct clk_regmap g12a_cpu_clk_apb = { + .data = &(struct clk_regmap_gate_data){ + .offset = HHI_SYS_CPU_CLK_CNTL1, + .bit_idx = 1, + }, + .hw.init = &(struct clk_init_data) { + .name = "cpu_clk_apb", + .ops = &clk_regmap_gate_ops, + .parent_names = (const char *[]){ "cpu_clk_apb_div" }, + .num_parents = 1, + /* + * This clock is set by the ROM monitor code, + * Linux should not change it at runtime + */ + .flags = CLK_IGNORE_UNUSED, + }, +}; + +static struct clk_regmap g12a_cpu_clk_atb_div = { + .data = &(struct clk_regmap_div_data){ + .offset = HHI_SYS_CPU_CLK_CNTL1, + .shift = 6, + .width = 3, + .flags = CLK_DIVIDER_POWER_OF_TWO, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_atb_div", + .ops = &clk_regmap_divider_ro_ops, + .parent_names = (const char *[]){ "cpu_clk" }, + .num_parents = 1, + }, +}; + +static struct clk_regmap g12a_cpu_clk_atb = { + .data = &(struct clk_regmap_gate_data){ + .offset = HHI_SYS_CPU_CLK_CNTL1, + .bit_idx = 17, + }, + .hw.init = &(struct clk_init_data) { + .name = "cpu_clk_atb", + .ops = &clk_regmap_gate_ops, + .parent_names = (const char *[]){ "cpu_clk_atb_div" }, + .num_parents = 1, + /* + * This clock is set by the ROM monitor code, + * Linux should not change it at runtime + */ + .flags = CLK_IGNORE_UNUSED, + }, +}; + +static struct clk_regmap g12a_cpu_clk_axi_div = { + .data = &(struct clk_regmap_div_data){ + .offset = HHI_SYS_CPU_CLK_CNTL1, + .shift = 9, + .width = 3, + .flags = CLK_DIVIDER_POWER_OF_TWO, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_axi_div", + .ops = &clk_regmap_divider_ro_ops, + .parent_names = (const char *[]){ "cpu_clk" }, + .num_parents = 1, + }, +}; + +static struct clk_regmap g12a_cpu_clk_axi = { + .data = &(struct clk_regmap_gate_data){ + .offset = HHI_SYS_CPU_CLK_CNTL1, + .bit_idx = 18, + }, + .hw.init = &(struct clk_init_data) { + .name = "cpu_clk_axi", + .ops = &clk_regmap_gate_ops, + .parent_names = (const char *[]){ "cpu_clk_axi_div" }, + .num_parents = 1, + /* + * This clock is set by the ROM monitor code, + * Linux should not change it at runtime + */ + .flags = CLK_IGNORE_UNUSED, + }, +}; + +static struct clk_regmap g12a_cpu_clk_trace_div = { + .data = &(struct clk_regmap_div_data){ + .offset = HHI_SYS_CPU_CLK_CNTL1, + .shift = 20, + .width = 3, + .flags = CLK_DIVIDER_POWER_OF_TWO, + }, + .hw.init = &(struct clk_init_data){ + .name = "cpu_clk_trace_div", + .ops = &clk_regmap_divider_ro_ops, + .parent_names = (const char *[]){ "cpu_clk" }, + .num_parents = 1, + }, +}; + +static struct clk_regmap g12a_cpu_clk_trace = { + .data = &(struct clk_regmap_gate_data){ + .offset = HHI_SYS_CPU_CLK_CNTL1, + .bit_idx = 23, + }, + .hw.init = &(struct clk_init_data) { + .name = "cpu_clk_trace", + .ops = &clk_regmap_gate_ops, + .parent_names = (const char *[]){ "cpu_clk_trace_div" }, + .num_parents = 1, + /* + * This clock is set by the ROM monitor code, + * Linux should not change it at runtime + */ + .flags = CLK_IGNORE_UNUSED, + }, +}; + static const struct pll_mult_range g12a_gp0_pll_mult_range = { .min = 55, .max = 255, @@ -2167,6 +2477,26 @@ static struct clk_hw_onecell_data g12a_hw_onecell_data = { [CLKID_MALI] = &g12a_mali.hw, [CLKID_MPLL_5OM_DIV] = &g12a_mpll_50m_div.hw, [CLKID_MPLL_5OM] = &g12a_mpll_50m.hw, + [CLKID_SYS_PLL_DIV16_EN] = &g12a_sys_pll_div16_en.hw, + [CLKID_SYS_PLL_DIV16] = &g12a_sys_pll_div16.hw, + [CLKID_CPU_CLK_DYN0_SEL] = &g12a_cpu_clk_dyn0_sel.hw, + [CLKID_CPU_CLK_DYN0_DIV] = &g12a_cpu_clk_dyn0_div.hw, + [CLKID_CPU_CLK_DYN0] = &g12a_cpu_clk_dyn0.hw, + [CLKID_CPU_CLK_DYN1_SEL] = &g12a_cpu_clk_dyn1_sel.hw, + [CLKID_CPU_CLK_DYN1_DIV] = &g12a_cpu_clk_dyn1_div.hw, + [CLKID_CPU_CLK_DYN1] = &g12a_cpu_clk_dyn1.hw, + [CLKID_CPU_CLK_DYN] = &g12a_cpu_clk_dyn.hw, + [CLKID_CPU_CLK] = &g12a_cpu_clk.hw, + [CLKID_CPU_CLK_DIV16_EN] = &g12a_cpu_clk_div16_en.hw, + [CLKID_CPU_CLK_DIV16] = &g12a_cpu_clk_div16.hw, + [CLKID_CPU_CLK_APB_DIV] = &g12a_cpu_clk_apb_div.hw, + [CLKID_CPU_CLK_APB] = &g12a_cpu_clk_apb.hw, + [CLKID_CPU_CLK_ATB_DIV] = &g12a_cpu_clk_atb_div.hw, + [CLKID_CPU_CLK_ATB] = &g12a_cpu_clk_atb.hw, + [CLKID_CPU_CLK_AXI_DIV] = &g12a_cpu_clk_axi_div.hw, + [CLKID_CPU_CLK_AXI] = &g12a_cpu_clk_axi.hw, + [CLKID_CPU_CLK_TRACE_DIV] = &g12a_cpu_clk_trace_div.hw, + [CLKID_CPU_CLK_TRACE] = &g12a_cpu_clk_trace.hw, [NR_CLKS] = NULL, }, .num = NR_CLKS, @@ -2335,6 +2665,24 @@ static struct clk_regmap *const g12a_clk_regmaps[] = { &g12a_mali_1, &g12a_mali, &g12a_mpll_50m, + &g12a_sys_pll_div16_en, + &g12a_cpu_clk_dyn0_sel, + &g12a_cpu_clk_dyn0_div, + &g12a_cpu_clk_dyn0, + &g12a_cpu_clk_dyn1_sel, + &g12a_cpu_clk_dyn1_div, + &g12a_cpu_clk_dyn1, + &g12a_cpu_clk_dyn, + &g12a_cpu_clk, + &g12a_cpu_clk_div16_en, + &g12a_cpu_clk_apb_div, + &g12a_cpu_clk_apb, + &g12a_cpu_clk_atb_div, + &g12a_cpu_clk_atb, + &g12a_cpu_clk_axi_div, + &g12a_cpu_clk_axi, + &g12a_cpu_clk_trace_div, + &g12a_cpu_clk_trace, }; static const struct meson_eeclkc_data g12a_clkc_data = { diff --git a/drivers/clk/meson/g12a.h b/drivers/clk/meson/g12a.h index 4854750df902..0ba3bfe4d46d 100644 --- a/drivers/clk/meson/g12a.h +++ b/drivers/clk/meson/g12a.h @@ -50,6 +50,7 @@ #define HHI_GCLK_MPEG2 0x148 #define HHI_GCLK_OTHER 0x150 #define HHI_GCLK_OTHER2 0x154 +#define HHI_SYS_CPU_CLK_CNTL1 0x15c #define HHI_VID_CLK_DIV 0x164 #define HHI_MPEG_CLK_CNTL 0x174 #define HHI_AUD_CLK_CNTL 0x178 -- 2.20.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: meson: g12a: add cpu clocks 2019-03-01 10:21 ` [PATCH 2/2] clk: meson: g12a: add cpu clocks Neil Armstrong @ 2019-03-01 15:21 ` Martin Blumenstingl 2019-03-01 16:41 ` Neil Armstrong 0 siblings, 1 reply; 9+ messages in thread From: Martin Blumenstingl @ 2019-03-01 15:21 UTC (permalink / raw) To: Neil Armstrong Cc: jbrunet, linux-amlogic, linux-kernel, linux-clk, linux-arm-kernel Hi Neil, it's great to see the progress on G12A! On Fri, Mar 1, 2019 at 11:22 AM Neil Armstrong <narmstrong@baylibre.com> wrote: > > Add the Amlogic G12A Family CPU Clock tree in read/only for now. > > The CPU clock can either use the SYS_PLL for > 1GHz frequencies or > use a couple of div+mux from 1GHz/667MHz/24MHz source with 2 non-glitch > muxes. > > Proper DVFS support will come in a second time. can you please also mention that this adds various CPU clock post-dividers (APB, ATB, AXI and CPU trace)? I don't mind them being int his patchset disclaimer for my code-review: - I don't have access to the datasheet so I can't verify if the clock tree from this patch is correct - the latest buildroot code with G12A support (buildroot_openlinux_kernel_4.9_fbdev_20180706) doesn't have proper names for all clocks - based on my experience with Meson8* this looks good overall, some questions and comments below > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > > --- > drivers/clk/meson/g12a.c | 348 +++++++++++++++++++++++++++++++++++++++ > drivers/clk/meson/g12a.h | 1 + > 2 files changed, 349 insertions(+) > > diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c > index 0e1ce8c03259..4c938f1b8421 100644 > --- a/drivers/clk/meson/g12a.c > +++ b/drivers/clk/meson/g12a.c > @@ -150,6 +150,316 @@ static struct clk_regmap g12a_sys_pll = { > }, > }; > > +static struct clk_regmap g12a_sys_pll_div16_en = { > + .data = &(struct clk_regmap_gate_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL1, > + .bit_idx = 24, > + }, > + .hw.init = &(struct clk_init_data) { > + .name = "sys_pll_div16_en", > + .ops = &clk_regmap_gate_ops, > + .parent_names = (const char *[]){ "sys_pll" }, > + .num_parents = 1, > + /* > + * This clock is used to debug the sys_pll range > + * Linux should not change it at runtime > + */ if we're not supposed to touch this the enable bit, can you switch to clk_regmap_gate_ro_ops ? > + .flags = CLK_IGNORE_UNUSED, > + }, > +}; > + > +static struct clk_fixed_factor g12a_sys_pll_div16 = { > + .mult = 1, > + .div = 16, > + .hw.init = &(struct clk_init_data){ > + .name = "sys_pll_div16", > + .ops = &clk_fixed_factor_ops, > + .parent_names = (const char *[]){ "sys_pll_div16_en" }, > + .num_parents = 1, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_dyn0_sel = { > + .data = &(struct clk_regmap_mux_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL0, > + .mask = 0x3, > + .shift = 0, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_dyn0_sel", the buildroot code has a variable with the name "p_premux" I'm not sure what the datasheet states, but maybe this should be cpu_clk_dyn0_pre_sel same applies to the corresponding dyn1 clock below > + .ops = &clk_regmap_mux_ro_ops, > + .parent_names = (const char *[]){ IN_PREFIX "xtal", > + "fclk_div2", > + "fclk_div3" }, > + .num_parents = 3, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_dyn0_div = { > + .data = &(struct clk_regmap_div_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL0, > + .shift = 4, > + .width = 6, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_dyn0_div", > + .ops = &clk_regmap_divider_ro_ops, > + .parent_names = (const char *[]){ "cpu_clk_dyn0_sel" }, > + .num_parents = 1, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_dyn0 = { > + .data = &(struct clk_regmap_mux_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL0, > + .mask = 0x1, > + .shift = 2, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_dyn0", the buildroot code has a variable with the name "p_postmux". in this case I would leave the name "cpu_clk_dyn0" because it's the "output" of this specific "clock tree/branch". same applies to the corresponding dyn1 clock below > + .ops = &clk_regmap_mux_ro_ops, > + .parent_names = (const char *[]){ "cpu_clk_dyn0_sel", > + "cpu_clk_dyn0_div" }, > + .num_parents = 2, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_dyn1_sel = { > + .data = &(struct clk_regmap_mux_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL0, > + .mask = 0x3, > + .shift = 16, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_dyn1_sel", > + .ops = &clk_regmap_mux_ro_ops, > + .parent_names = (const char *[]){ IN_PREFIX "xtal", > + "fclk_div2", > + "fclk_div3" }, > + .num_parents = 3, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_dyn1_div = { > + .data = &(struct clk_regmap_div_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL0, > + .shift = 20, > + .width = 6, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_dyn1_div", > + .ops = &clk_regmap_divider_ro_ops, > + .parent_names = (const char *[]){ "cpu_clk_dyn1_sel" }, > + .num_parents = 1, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_dyn1 = { > + .data = &(struct clk_regmap_mux_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL0, > + .mask = 0x1, > + .shift = 18, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_dyn1", > + .ops = &clk_regmap_mux_ro_ops, > + .parent_names = (const char *[]){ "cpu_clk_dyn1_sel", > + "cpu_clk_dyn1_div" }, > + .num_parents = 2, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_dyn = { > + .data = &(struct clk_regmap_mux_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL0, > + .mask = 0x1, > + .shift = 10, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_dyn", > + .ops = &clk_regmap_mux_ro_ops, > + .parent_names = (const char *[]){ "cpu_clk_dyn0", > + "cpu_clk_dyn1" }, > + .num_parents = 2, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk = { > + .data = &(struct clk_regmap_mux_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL0, > + .mask = 0x1, > + .shift = 11, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk", > + .ops = &clk_regmap_mux_ro_ops, > + .parent_names = (const char *[]){ "cpu_clk_dyn", > + "sys_pll" }, > + .num_parents = 2, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_div16_en = { > + .data = &(struct clk_regmap_gate_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL1, > + .bit_idx = 1, > + }, > + .hw.init = &(struct clk_init_data) { > + .name = "cpu_clk_div16_en", > + .ops = &clk_regmap_gate_ops, > + .parent_names = (const char *[]){ "cpu_clk" }, > + .num_parents = 1, > + /* > + * This clock is used to debug the cpu_clk range > + * Linux should not change it at runtime > + */ same as above: if we're not supposed to touch this the enable bit, can you switch to clk_regmap_gate_ro_ops ? > + .flags = CLK_IGNORE_UNUSED, > + }, > +}; > + > +static struct clk_fixed_factor g12a_cpu_clk_div16 = { > + .mult = 1, > + .div = 16, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_div16", > + .ops = &clk_fixed_factor_ops, > + .parent_names = (const char *[]){ "cpu_clk_div16_en" }, > + .num_parents = 1, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_apb_div = { > + .data = &(struct clk_regmap_div_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL1, > + .shift = 3, > + .width = 3, > + .flags = CLK_DIVIDER_POWER_OF_TWO, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_apb_div", > + .ops = &clk_regmap_divider_ro_ops, > + .parent_names = (const char *[]){ "cpu_clk" }, > + .num_parents = 1, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_apb = { > + .data = &(struct clk_regmap_gate_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL1, > + .bit_idx = 1, > + }, > + .hw.init = &(struct clk_init_data) { > + .name = "cpu_clk_apb", > + .ops = &clk_regmap_gate_ops, > + .parent_names = (const char *[]){ "cpu_clk_apb_div" }, > + .num_parents = 1, > + /* > + * This clock is set by the ROM monitor code, > + * Linux should not change it at runtime > + */ same as above: if we're not supposed to touch this the enable bit, can you switch to clk_regmap_gate_ro_ops ? > + .flags = CLK_IGNORE_UNUSED, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_atb_div = { > + .data = &(struct clk_regmap_div_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL1, > + .shift = 6, > + .width = 3, > + .flags = CLK_DIVIDER_POWER_OF_TWO, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_atb_div", > + .ops = &clk_regmap_divider_ro_ops, > + .parent_names = (const char *[]){ "cpu_clk" }, > + .num_parents = 1, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_atb = { > + .data = &(struct clk_regmap_gate_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL1, > + .bit_idx = 17, > + }, > + .hw.init = &(struct clk_init_data) { > + .name = "cpu_clk_atb", > + .ops = &clk_regmap_gate_ops, > + .parent_names = (const char *[]){ "cpu_clk_atb_div" }, > + .num_parents = 1, > + /* > + * This clock is set by the ROM monitor code, > + * Linux should not change it at runtime > + */ same as above: if we're not supposed to touch this the enable bit, can you switch to clk_regmap_gate_ro_ops ? > + .flags = CLK_IGNORE_UNUSED, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_axi_div = { > + .data = &(struct clk_regmap_div_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL1, > + .shift = 9, > + .width = 3, > + .flags = CLK_DIVIDER_POWER_OF_TWO, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_axi_div", > + .ops = &clk_regmap_divider_ro_ops, out of curiosity (this applies to all CPU clock post-dividers): did you check whether CLK_DIVIDER_POWER_OF_TWO is correct on G12A? I'm asking because on Meson8b the post-dividers select between one of: "cpu_clk divided by 2, 3, 4, 5, 6, 7 or 8". also some of the post-dividers use register value 0 for cpu_clk_div2 while others use register value 1 for cpu_clk_div2. > + .parent_names = (const char *[]){ "cpu_clk" }, > + .num_parents = 1, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_axi = { > + .data = &(struct clk_regmap_gate_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL1, > + .bit_idx = 18, > + }, > + .hw.init = &(struct clk_init_data) { > + .name = "cpu_clk_axi", > + .ops = &clk_regmap_gate_ops, > + .parent_names = (const char *[]){ "cpu_clk_axi_div" }, > + .num_parents = 1, > + /* > + * This clock is set by the ROM monitor code, > + * Linux should not change it at runtime > + */ same as above: if we're not supposed to touch this the enable bit, can you switch to clk_regmap_gate_ro_ops ? > + .flags = CLK_IGNORE_UNUSED, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_trace_div = { > + .data = &(struct clk_regmap_div_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL1, > + .shift = 20, > + .width = 3, > + .flags = CLK_DIVIDER_POWER_OF_TWO, > + }, > + .hw.init = &(struct clk_init_data){ > + .name = "cpu_clk_trace_div", > + .ops = &clk_regmap_divider_ro_ops, > + .parent_names = (const char *[]){ "cpu_clk" }, > + .num_parents = 1, > + }, > +}; > + > +static struct clk_regmap g12a_cpu_clk_trace = { > + .data = &(struct clk_regmap_gate_data){ > + .offset = HHI_SYS_CPU_CLK_CNTL1, > + .bit_idx = 23, > + }, > + .hw.init = &(struct clk_init_data) { > + .name = "cpu_clk_trace", > + .ops = &clk_regmap_gate_ops, > + .parent_names = (const char *[]){ "cpu_clk_trace_div" }, > + .num_parents = 1, > + /* > + * This clock is set by the ROM monitor code, > + * Linux should not change it at runtime > + */ same as above: if we're not supposed to touch this the enable bit, can you switch to clk_regmap_gate_ro_ops ? > + .flags = CLK_IGNORE_UNUSED, > + }, > +}; > + > static const struct pll_mult_range g12a_gp0_pll_mult_range = { > .min = 55, > .max = 255, > @@ -2167,6 +2477,26 @@ static struct clk_hw_onecell_data g12a_hw_onecell_data = { > [CLKID_MALI] = &g12a_mali.hw, > [CLKID_MPLL_5OM_DIV] = &g12a_mpll_50m_div.hw, > [CLKID_MPLL_5OM] = &g12a_mpll_50m.hw, > + [CLKID_SYS_PLL_DIV16_EN] = &g12a_sys_pll_div16_en.hw, > + [CLKID_SYS_PLL_DIV16] = &g12a_sys_pll_div16.hw, > + [CLKID_CPU_CLK_DYN0_SEL] = &g12a_cpu_clk_dyn0_sel.hw, > + [CLKID_CPU_CLK_DYN0_DIV] = &g12a_cpu_clk_dyn0_div.hw, > + [CLKID_CPU_CLK_DYN0] = &g12a_cpu_clk_dyn0.hw, > + [CLKID_CPU_CLK_DYN1_SEL] = &g12a_cpu_clk_dyn1_sel.hw, > + [CLKID_CPU_CLK_DYN1_DIV] = &g12a_cpu_clk_dyn1_div.hw, > + [CLKID_CPU_CLK_DYN1] = &g12a_cpu_clk_dyn1.hw, > + [CLKID_CPU_CLK_DYN] = &g12a_cpu_clk_dyn.hw, > + [CLKID_CPU_CLK] = &g12a_cpu_clk.hw, > + [CLKID_CPU_CLK_DIV16_EN] = &g12a_cpu_clk_div16_en.hw, > + [CLKID_CPU_CLK_DIV16] = &g12a_cpu_clk_div16.hw, > + [CLKID_CPU_CLK_APB_DIV] = &g12a_cpu_clk_apb_div.hw, > + [CLKID_CPU_CLK_APB] = &g12a_cpu_clk_apb.hw, > + [CLKID_CPU_CLK_ATB_DIV] = &g12a_cpu_clk_atb_div.hw, > + [CLKID_CPU_CLK_ATB] = &g12a_cpu_clk_atb.hw, > + [CLKID_CPU_CLK_AXI_DIV] = &g12a_cpu_clk_axi_div.hw, > + [CLKID_CPU_CLK_AXI] = &g12a_cpu_clk_axi.hw, > + [CLKID_CPU_CLK_TRACE_DIV] = &g12a_cpu_clk_trace_div.hw, > + [CLKID_CPU_CLK_TRACE] = &g12a_cpu_clk_trace.hw, > [NR_CLKS] = NULL, > }, > .num = NR_CLKS, > @@ -2335,6 +2665,24 @@ static struct clk_regmap *const g12a_clk_regmaps[] = { > &g12a_mali_1, > &g12a_mali, > &g12a_mpll_50m, > + &g12a_sys_pll_div16_en, > + &g12a_cpu_clk_dyn0_sel, > + &g12a_cpu_clk_dyn0_div, > + &g12a_cpu_clk_dyn0, > + &g12a_cpu_clk_dyn1_sel, > + &g12a_cpu_clk_dyn1_div, > + &g12a_cpu_clk_dyn1, > + &g12a_cpu_clk_dyn, > + &g12a_cpu_clk, > + &g12a_cpu_clk_div16_en, > + &g12a_cpu_clk_apb_div, > + &g12a_cpu_clk_apb, > + &g12a_cpu_clk_atb_div, > + &g12a_cpu_clk_atb, > + &g12a_cpu_clk_axi_div, > + &g12a_cpu_clk_axi, > + &g12a_cpu_clk_trace_div, > + &g12a_cpu_clk_trace, > }; > > static const struct meson_eeclkc_data g12a_clkc_data = { > diff --git a/drivers/clk/meson/g12a.h b/drivers/clk/meson/g12a.h > index 4854750df902..0ba3bfe4d46d 100644 > --- a/drivers/clk/meson/g12a.h > +++ b/drivers/clk/meson/g12a.h > @@ -50,6 +50,7 @@ > #define HHI_GCLK_MPEG2 0x148 > #define HHI_GCLK_OTHER 0x150 > #define HHI_GCLK_OTHER2 0x154 > +#define HHI_SYS_CPU_CLK_CNTL1 0x15c > #define HHI_VID_CLK_DIV 0x164 > #define HHI_MPEG_CLK_CNTL 0x174 > #define HHI_AUD_CLK_CNTL 0x178 > -- > 2.20.1 > > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: meson: g12a: add cpu clocks 2019-03-01 15:21 ` Martin Blumenstingl @ 2019-03-01 16:41 ` Neil Armstrong 2019-03-01 16:52 ` Martin Blumenstingl 0 siblings, 1 reply; 9+ messages in thread From: Neil Armstrong @ 2019-03-01 16:41 UTC (permalink / raw) To: Martin Blumenstingl Cc: jbrunet, linux-amlogic, linux-kernel, linux-clk, linux-arm-kernel Hi Martin, On 01/03/2019 16:21, Martin Blumenstingl wrote: > Hi Neil, > > it's great to see the progress on G12A! > > On Fri, Mar 1, 2019 at 11:22 AM Neil Armstrong <narmstrong@baylibre.com> wrote: >> >> Add the Amlogic G12A Family CPU Clock tree in read/only for now. >> >> The CPU clock can either use the SYS_PLL for > 1GHz frequencies or >> use a couple of div+mux from 1GHz/667MHz/24MHz source with 2 non-glitch >> muxes. >> >> Proper DVFS support will come in a second time. > can you please also mention that this adds various CPU clock > post-dividers (APB, ATB, AXI and CPU trace)? > I don't mind them being int his patchset indeed, I forgot this ! > > disclaimer for my code-review: > - I don't have access to the datasheet so I can't verify if the clock > tree from this patch is correct > - the latest buildroot code with G12A support > (buildroot_openlinux_kernel_4.9_fbdev_20180706) doesn't have proper > names for all clocks > - based on my experience with Meson8* this looks good overall, some > questions and comments below > >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> >> --- >> drivers/clk/meson/g12a.c | 348 +++++++++++++++++++++++++++++++++++++++ >> drivers/clk/meson/g12a.h | 1 + >> 2 files changed, 349 insertions(+) >> >> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c >> index 0e1ce8c03259..4c938f1b8421 100644 >> --- a/drivers/clk/meson/g12a.c >> +++ b/drivers/clk/meson/g12a.c >> @@ -150,6 +150,316 @@ static struct clk_regmap g12a_sys_pll = { >> }, >> }; >> >> +static struct clk_regmap g12a_sys_pll_div16_en = { >> + .data = &(struct clk_regmap_gate_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL1, >> + .bit_idx = 24, >> + }, >> + .hw.init = &(struct clk_init_data) { >> + .name = "sys_pll_div16_en", >> + .ops = &clk_regmap_gate_ops, >> + .parent_names = (const char *[]){ "sys_pll" }, >> + .num_parents = 1, >> + /* >> + * This clock is used to debug the sys_pll range >> + * Linux should not change it at runtime >> + */ > if we're not supposed to touch this the enable bit, can you switch to > clk_regmap_gate_ro_ops ? exact > >> + .flags = CLK_IGNORE_UNUSED, >> + }, >> +}; >> + >> +static struct clk_fixed_factor g12a_sys_pll_div16 = { >> + .mult = 1, >> + .div = 16, >> + .hw.init = &(struct clk_init_data){ >> + .name = "sys_pll_div16", >> + .ops = &clk_fixed_factor_ops, >> + .parent_names = (const char *[]){ "sys_pll_div16_en" }, >> + .num_parents = 1, >> + }, >> +}; >> + >> +static struct clk_regmap g12a_cpu_clk_dyn0_sel = { >> + .data = &(struct clk_regmap_mux_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL0, >> + .mask = 0x3, >> + .shift = 0, >> + }, >> + .hw.init = &(struct clk_init_data){ >> + .name = "cpu_clk_dyn0_sel", > the buildroot code has a variable with the name "p_premux" > I'm not sure what the datasheet states, but maybe this should be > cpu_clk_dyn0_pre_sel > same applies to the corresponding dyn1 clock below these bit are named "premux1", and cpu_clk_dyn0 names "postmux1", which has no sense because there is no mux in between. clk_dyn0_sel is the actual source selector of the dyn0 tree, clkdyn0 is the top of the dyn0 tree, this is why i did not add "sel" in it. > >> + .ops = &clk_regmap_mux_ro_ops, >> + .parent_names = (const char *[]){ IN_PREFIX "xtal", >> + "fclk_div2", >> + "fclk_div3" }, >> + .num_parents = 3, >> + }, >> +}; >> + >> +static struct clk_regmap g12a_cpu_clk_dyn0_div = { >> + .data = &(struct clk_regmap_div_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL0, >> + .shift = 4, >> + .width = 6, >> + }, >> + .hw.init = &(struct clk_init_data){ >> + .name = "cpu_clk_dyn0_div", >> + .ops = &clk_regmap_divider_ro_ops, >> + .parent_names = (const char *[]){ "cpu_clk_dyn0_sel" }, >> + .num_parents = 1, >> + }, >> +}; >> + >> +static struct clk_regmap g12a_cpu_clk_dyn0 = { >> + .data = &(struct clk_regmap_mux_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL0, >> + .mask = 0x1, >> + .shift = 2, >> + }, >> + .hw.init = &(struct clk_init_data){ >> + .name = "cpu_clk_dyn0", > the buildroot code has a variable with the name "p_postmux". in this > case I would leave the name "cpu_clk_dyn0" because it's the "output" > of this specific "clock tree/branch". > same applies to the corresponding dyn1 clock below > >> + .ops = &clk_regmap_mux_ro_ops, >> + .parent_names = (const char *[]){ "cpu_clk_dyn0_sel", >> + "cpu_clk_dyn0_div" }, >> + .num_parents = 2, >> + }, >> +}; >> + [...] >> + >> +static struct clk_regmap g12a_cpu_clk_div16_en = { >> + .data = &(struct clk_regmap_gate_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL1, >> + .bit_idx = 1, >> + }, >> + .hw.init = &(struct clk_init_data) { >> + .name = "cpu_clk_div16_en", >> + .ops = &clk_regmap_gate_ops, >> + .parent_names = (const char *[]){ "cpu_clk" }, >> + .num_parents = 1, >> + /* >> + * This clock is used to debug the cpu_clk range >> + * Linux should not change it at runtime >> + */ > same as above: if we're not supposed to touch this the enable bit, can > you switch to clk_regmap_gate_ro_ops ? Yep > >> + .flags = CLK_IGNORE_UNUSED, >> + }, >> +}; >> + >> +static struct clk_fixed_factor g12a_cpu_clk_div16 = { >> + .mult = 1, >> + .div = 16, >> + .hw.init = &(struct clk_init_data){ >> + .name = "cpu_clk_div16", >> + .ops = &clk_fixed_factor_ops, >> + .parent_names = (const char *[]){ "cpu_clk_div16_en" }, >> + .num_parents = 1, >> + }, >> +}; >> + >> +static struct clk_regmap g12a_cpu_clk_apb_div = { >> + .data = &(struct clk_regmap_div_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL1, >> + .shift = 3, >> + .width = 3, >> + .flags = CLK_DIVIDER_POWER_OF_TWO, >> + }, >> + .hw.init = &(struct clk_init_data){ >> + .name = "cpu_clk_apb_div", >> + .ops = &clk_regmap_divider_ro_ops, >> + .parent_names = (const char *[]){ "cpu_clk" }, >> + .num_parents = 1, >> + }, >> +}; >> + >> +static struct clk_regmap g12a_cpu_clk_apb = { >> + .data = &(struct clk_regmap_gate_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL1, >> + .bit_idx = 1, >> + }, >> + .hw.init = &(struct clk_init_data) { >> + .name = "cpu_clk_apb", >> + .ops = &clk_regmap_gate_ops, >> + .parent_names = (const char *[]){ "cpu_clk_apb_div" }, >> + .num_parents = 1, >> + /* >> + * This clock is set by the ROM monitor code, >> + * Linux should not change it at runtime >> + */ > same as above: if we're not supposed to touch this the enable bit, can > you switch to clk_regmap_gate_ro_ops ? Yep > >> + .flags = CLK_IGNORE_UNUSED, >> + }, >> +}; >> + >> +static struct clk_regmap g12a_cpu_clk_atb_div = { >> + .data = &(struct clk_regmap_div_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL1, >> + .shift = 6, >> + .width = 3, >> + .flags = CLK_DIVIDER_POWER_OF_TWO, >> + }, >> + .hw.init = &(struct clk_init_data){ >> + .name = "cpu_clk_atb_div", >> + .ops = &clk_regmap_divider_ro_ops, >> + .parent_names = (const char *[]){ "cpu_clk" }, >> + .num_parents = 1, >> + }, >> +}; >> + >> +static struct clk_regmap g12a_cpu_clk_atb = { >> + .data = &(struct clk_regmap_gate_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL1, >> + .bit_idx = 17, >> + }, >> + .hw.init = &(struct clk_init_data) { >> + .name = "cpu_clk_atb", >> + .ops = &clk_regmap_gate_ops, >> + .parent_names = (const char *[]){ "cpu_clk_atb_div" }, >> + .num_parents = 1, >> + /* >> + * This clock is set by the ROM monitor code, >> + * Linux should not change it at runtime >> + */ > same as above: if we're not supposed to touch this the enable bit, can > you switch to clk_regmap_gate_ro_ops ? Yep > >> + .flags = CLK_IGNORE_UNUSED, >> + }, >> +}; >> + >> +static struct clk_regmap g12a_cpu_clk_axi_div = { >> + .data = &(struct clk_regmap_div_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL1, >> + .shift = 9, >> + .width = 3, >> + .flags = CLK_DIVIDER_POWER_OF_TWO, >> + }, >> + .hw.init = &(struct clk_init_data){ >> + .name = "cpu_clk_axi_div", >> + .ops = &clk_regmap_divider_ro_ops, > out of curiosity (this applies to all CPU clock post-dividers): > did you check whether CLK_DIVIDER_POWER_OF_TWO is correct on G12A? > I'm asking because on Meson8b the post-dividers select between one of: > "cpu_clk divided by 2, 3, 4, 5, 6, 7 or 8". also some of the > post-dividers use register value 0 for cpu_clk_div2 while others use > register value 1 for cpu_clk_div2. It's correct ! > >> + .parent_names = (const char *[]){ "cpu_clk" }, >> + .num_parents = 1, >> + }, >> +}; >> + >> +static struct clk_regmap g12a_cpu_clk_axi = { >> + .data = &(struct clk_regmap_gate_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL1, >> + .bit_idx = 18, >> + }, >> + .hw.init = &(struct clk_init_data) { >> + .name = "cpu_clk_axi", >> + .ops = &clk_regmap_gate_ops, >> + .parent_names = (const char *[]){ "cpu_clk_axi_div" }, >> + .num_parents = 1, >> + /* >> + * This clock is set by the ROM monitor code, >> + * Linux should not change it at runtime >> + */ > same as above: if we're not supposed to touch this the enable bit, can > you switch to clk_regmap_gate_ro_ops ? Yep > >> + .flags = CLK_IGNORE_UNUSED, >> + }, >> +}; >> + >> +static struct clk_regmap g12a_cpu_clk_trace_div = { >> + .data = &(struct clk_regmap_div_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL1, >> + .shift = 20, >> + .width = 3, >> + .flags = CLK_DIVIDER_POWER_OF_TWO, >> + }, >> + .hw.init = &(struct clk_init_data){ >> + .name = "cpu_clk_trace_div", >> + .ops = &clk_regmap_divider_ro_ops, >> + .parent_names = (const char *[]){ "cpu_clk" }, >> + .num_parents = 1, >> + }, >> +}; >> + >> +static struct clk_regmap g12a_cpu_clk_trace = { >> + .data = &(struct clk_regmap_gate_data){ >> + .offset = HHI_SYS_CPU_CLK_CNTL1, >> + .bit_idx = 23, >> + }, >> + .hw.init = &(struct clk_init_data) { >> + .name = "cpu_clk_trace", >> + .ops = &clk_regmap_gate_ops, >> + .parent_names = (const char *[]){ "cpu_clk_trace_div" }, >> + .num_parents = 1, >> + /* >> + * This clock is set by the ROM monitor code, >> + * Linux should not change it at runtime >> + */ > same as above: if we're not supposed to touch this the enable bit, can > you switch to clk_regmap_gate_ro_ops ? Yep > >> + .flags = CLK_IGNORE_UNUSED, >> + }, >> +}; >> + >> static const struct pll_mult_range g12a_gp0_pll_mult_range = { >> .min = 55, >> .max = 255, Thanks ! Neil ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: meson: g12a: add cpu clocks 2019-03-01 16:41 ` Neil Armstrong @ 2019-03-01 16:52 ` Martin Blumenstingl 0 siblings, 0 replies; 9+ messages in thread From: Martin Blumenstingl @ 2019-03-01 16:52 UTC (permalink / raw) To: Neil Armstrong Cc: jbrunet, linux-amlogic, linux-kernel, linux-clk, linux-arm-kernel Hi Neil, On Fri, Mar 1, 2019 at 5:42 PM Neil Armstrong <narmstrong@baylibre.com> wrote: [...] > >> +static struct clk_regmap g12a_cpu_clk_dyn0_sel = { > >> + .data = &(struct clk_regmap_mux_data){ > >> + .offset = HHI_SYS_CPU_CLK_CNTL0, > >> + .mask = 0x3, > >> + .shift = 0, > >> + }, > >> + .hw.init = &(struct clk_init_data){ > >> + .name = "cpu_clk_dyn0_sel", > > the buildroot code has a variable with the name "p_premux" > > I'm not sure what the datasheet states, but maybe this should be > > cpu_clk_dyn0_pre_sel > > same applies to the corresponding dyn1 clock below > > these bit are named "premux1", and cpu_clk_dyn0 names "postmux1", > which has no sense because there is no mux in between. > > clk_dyn0_sel is the actual source selector of the dyn0 tree, > clkdyn0 is the top of the dyn0 tree, this is why i did not add "sel" > in it. OK, thank you for the explanation. can you please add a comment with the name from the datasheet in that case? that'll make it easier (at least for me) to compare the datasheet (and also buildroot kernel, since similar names are used there) with the mainline drivers [...] > > > >> + .flags = CLK_IGNORE_UNUSED, > >> + }, > >> +}; > >> + > >> +static struct clk_regmap g12a_cpu_clk_axi_div = { > >> + .data = &(struct clk_regmap_div_data){ > >> + .offset = HHI_SYS_CPU_CLK_CNTL1, > >> + .shift = 9, > >> + .width = 3, > >> + .flags = CLK_DIVIDER_POWER_OF_TWO, > >> + }, > >> + .hw.init = &(struct clk_init_data){ > >> + .name = "cpu_clk_axi_div", > >> + .ops = &clk_regmap_divider_ro_ops, > > out of curiosity (this applies to all CPU clock post-dividers): > > did you check whether CLK_DIVIDER_POWER_OF_TWO is correct on G12A? > > I'm asking because on Meson8b the post-dividers select between one of: > > "cpu_clk divided by 2, 3, 4, 5, 6, 7 or 8". also some of the > > post-dividers use register value 0 for cpu_clk_div2 while others use > > register value 1 for cpu_clk_div2. > > It's correct ! thanks for looking this up again and double-checking! Martin ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-03-01 17:05 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-01 10:21 [PATCH 0/2] clk: meson: g12a: Add CPU Clock support Neil Armstrong 2019-03-01 10:21 ` [PATCH 1/2] clk: meson-g12a: add cpu clock bindings Neil Armstrong 2019-03-01 15:26 ` Martin Blumenstingl 2019-03-01 16:43 ` Neil Armstrong 2019-03-01 17:05 ` Martin Blumenstingl 2019-03-01 10:21 ` [PATCH 2/2] clk: meson: g12a: add cpu clocks Neil Armstrong 2019-03-01 15:21 ` Martin Blumenstingl 2019-03-01 16:41 ` Neil Armstrong 2019-03-01 16:52 ` Martin Blumenstingl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).