Messages in this thread | | | Subject | Re: [PATCH 2/3] clk: meson: g12a: Add support for G12B CPUB clocks | From | Neil Armstrong <> | Date | Wed, 22 May 2019 09:57:58 +0200 |
| |
On 21/05/2019 19:49, Martin Blumenstingl wrote: > Hi Neil, > > On Tue, May 21, 2019 at 5:02 PM Neil Armstrong <narmstrong@baylibre.com> wrote: >> >> Update the Meson G12A Clock driver to support the Amlogic G12B SoC. >> >> G12B clock driver is very close, the main differences are : >> - the clock tree is duplicated for the both clusters, and the >> SYS_PLL are swapped between the clusters >> - G12A has additional clocks like for CSI an other components > should this also be G12B?
Indeed, another leftover from previous patchset...
> > [...] >> +static struct clk_regmap g12b_cpub_clk_apb_div = { > if you also think that it's worth it then please add a comment stating > that this is called "PCLK_mux" in the datasheet > same goes for the ATB and AXI clocks below as the naming in the driver > and datasheet differs > >> + .data = &(struct clk_regmap_div_data){ >> + .offset = HHI_SYS_CPUB_CLK_CNTL1, >> + .shift = 3, >> + .width = 3, >> + .flags = CLK_DIVIDER_POWER_OF_TWO, >> + }, >> + .hw.init = &(struct clk_init_data){ >> + .name = "cpub_clk_apb_div", >> + .ops = &clk_regmap_divider_ro_ops, >> + .parent_names = (const char *[]){ "cpub_clk" }, >> + .num_parents = 1, >> + }, >> +}; > I'm assuming you checked that this is really a power of two divider, > on the Meson8/8b/8m2 SoCs this is a mux between div[2..8] > (the same goes for the ATB, AXI and trace div clocks below)
Indeed it's not a power of 2 here, it's mux between div[2..8]
I'll also need to update the first cluster aswell...
> >> + >> +static struct clk_regmap g12b_cpub_clk_apb = { >> + .data = &(struct clk_regmap_gate_data){ >> + .offset = HHI_SYS_CPUB_CLK_CNTL1, >> + .bit_idx = 16, > the public S922X datasheet calls this "PCLK_dis", does this mean you > need a flag here? > .flags = CLK_GATE_SET_TO_DISABLE,
The first cluster register has some description, but with the same fields naming : APB_CLK_DIS: set to 1 to manually disable the APB clock...
So you are right, I'll also fix the first cluster clocks.
> > [...] >> +static struct clk_regmap g12b_cpub_clk_atb = { >> + .data = &(struct clk_regmap_gate_data){ >> + .offset = HHI_SYS_CPUB_CLK_CNTL1, >> + .bit_idx = 17, > the public S922X datasheet calls this "ATCLK_clk_dis", does this mean > you need a flag here? > .flags = CLK_GATE_SET_TO_DISABLE,
Exact
> > [...] >> +static struct clk_regmap g12b_cpub_clk_axi = { >> + .data = &(struct clk_regmap_gate_data){ >> + .offset = HHI_SYS_CPUB_CLK_CNTL1, >> + .bit_idx = 18, > the public S922X datasheet calls this "ACLKM_clk_dis", does this mean > you need a flag here? > .flags = CLK_GATE_SET_TO_DISABLE,
Exact
> > [...] >> +static struct clk_regmap g12b_cpub_clk_trace = { >> + .data = &(struct clk_regmap_gate_data){ >> + .offset = HHI_SYS_CPUB_CLK_CNTL1, >> + .bit_idx = 23, > the public S922X datasheet calls this "Trace_clk_dis", does this mean > you need a flag here? > .flags = CLK_GATE_SET_TO_DISABLE,
Exact
Thanks for the review !
Neil
> > > Regards > Martin >
| |