Re: [PATCH v5 5/5] clk: meson: a1: add support for Amlogic A1 Peripheral clock driver

[Martin Blumenstingl <martin.blumenstingl@googlemail.com>]

 Hi Jian,

my comments and questions below
please keep in mind that I don't have access to the A1 datasheets, so
I may ask stupid questions :)

On Fri, Dec 27, 2019 at 10:47 AM Jian Hu <jian.hu@amlogic.com> wrote:
[...]
> +/* PLLs clock in gates, its parent is xtal */
yes. doesn't the code below describe exactly this (what is so special
about it that we need an extra comment)?

[...]
> +static const struct clk_parent_data sys_clk_parents[] = {
> +       { .fw_name = "xtal" },
> +       { .fw_name = "fclk_div2"},
> +       { .fw_name = "fclk_div3"},
> +       { .fw_name = "fclk_div5"},
the last three values are missing a space before "}"

[...]
> +       .hw.init = &(struct clk_init_data){
> +               .name = "sys_clk",
> +               .ops = &clk_regmap_mux_ro_ops,
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &a1_sys_a.hw, &a1_sys_b.hw,
> +               },
> +               .num_parents = 2,
> +               /*
> +                * This clock is used by APB bus which setted in Romcode
like in the PLL clkc patch:
- setted -> "is set"
- Romcode == boot ROM ?

[...]
> +static struct clk_regmap a1_rtc_32k_sel = {
> +       .data = &(struct clk_regmap_mux_data) {
> +               .offset = RTC_CTRL,
> +               .mask = 0x3,
> +               .shift = 0,
> +               .flags = CLK_MUX_ROUND_CLOSEST,
CLK_MUX_ROUND_CLOSEST means the common clock framework will also
accept rates greater than 32kHz.
is that fine for this case?

[...]
> +/*
> + * the second parent is sys_pll_div16, it will complete in the CPU clock,
I was confused by this but I assume you mean the parent with index 2?

> + * the forth parent is the clock measurement source, it relies on
> + * the clock measurement register configuration.
...and parent with index 4 here

[...]
> +static struct clk_regmap a1_pwm_a = {
> +       .data = &(struct clk_regmap_gate_data){
> +               .offset = PWM_CLK_AB_CTRL,
> +               .bit_idx = 8,
> +       },
> +       .hw.init = &(struct clk_init_data) {
> +               .name = "pwm_a",
> +               .ops = &clk_regmap_gate_ops,
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &a1_pwm_a_div.hw
> +               },
> +               .num_parents = 1,
> +               /*
> +                * The CPU working voltage is controlled by pwm_a
> +                * in BL2 firmware. add the CLK_IS_CRITICAL flag
> +                * to avoid changing at runtime.
on G12A and G12B Linux has to manage the CPU voltage regulator
can you confirm that for the A1 SoC this is really done by BL2? (I'm
wondering since A1 is newer than G12)

> +/*
> + * spicc clk
> + *    div2   |\         |\       _____
> + *  ---------| |---DIV--| |     |     |    spicc out
> + *  ---------| |        | |-----|GATE |---------
> + *     ..... |/         | /     |_____|
> + *  --------------------|/
> + *                 24M
does that "div2" stand for fclk_div2?

[...]
> +static const struct meson_eeclkc_data a1_periphs_data = {
> +               .regmap_clks = a1_periphs_regmaps,
> +               .regmap_clk_num = ARRAY_SIZE(a1_periphs_regmaps),
> +               .hw_onecell_data = &a1_periphs_hw_onecell_data,
> +};
same comment as for the PLL clkc: please drop this and use the
variables directly inside _probe to get rid of the struct
meson_eeclkc_data (so I won't be confused about "EE clocks" on A1,
while according to your description there's no "EE" domain)


Martin