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 <narmstr...@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 <narmstr...@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
