Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230517133309.9874-1-ddrokosov@sberdevices.ru> <20230517133309.9874-7-ddrokosov@sberdevices.ru>
In-Reply-To: <20230517133309.9874-7-ddrokosov@sberdevices.ru>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Fri, 19 May 2023 23:03:54 +0200
Message-ID: <CAFBinCBs7-9CvfQLxLoG5=FjmSK+S5eGsLXOAyQN9kNOg2q-2g@mail.gmail.com>
Subject: Re: [PATCH v15 6/6] clk: meson: a1: add Amlogic A1 Peripherals clock
 controller driver
To:     Dmitry Rokosov <ddrokosov@sberdevices.ru>
Cc:     neil.armstrong@linaro.org, jbrunet@baylibre.com,
        mturquette@baylibre.com, sboyd@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, khilman@baylibre.com,
        jian.hu@amlogic.com, kernel@sberdevices.ru, rockosov@gmail.com,
        linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Dmitry,

On Wed, May 17, 2023 at 3:33=E2=80=AFPM Dmitry Rokosov <ddrokosov@sberdevic=
es.ru> wrote:
[...]
> +static struct clk_regmap sys_b_sel =3D {
> +       .data =3D &(struct clk_regmap_mux_data){
> +               .offset =3D SYS_CLK_CTRL0,
> +               .mask =3D 0x7,
> +               .shift =3D 26,
> +               .table =3D mux_table_sys,
> +       },
> +       .hw.init =3D &(struct clk_init_data){
> +               .name =3D "sys_b_sel",
> +               .ops =3D &clk_regmap_mux_ro_ops,
the sys_*_sel muxes and sys_*_gate are _ro...

> +               .parent_data =3D sys_parents,
> +               .num_parents =3D ARRAY_SIZE(sys_parents),
> +       },
> +};
> +
> +static struct clk_regmap sys_b_div =3D {
> +       .data =3D &(struct clk_regmap_div_data){
> +               .offset =3D SYS_CLK_CTRL0,
> +               .shift =3D 16,
> +               .width =3D 10,
> +       },
> +       .hw.init =3D &(struct clk_init_data){
> +               .name =3D "sys_b_div",
> +               .ops =3D &clk_regmap_divider_ops,
...but the sys_*_div aren't
Is this on purpose? If it is: why can the divider be changed at
runtime but the mux can't?

[...]
> +/*
> + * the index 2 is sys_pll_div16, it will be implemented in the CPU clock=
 driver,
We need to add the "sys_pll_div16" input to the dt-bindings since they
should always describe the hardware (regardless of what the driver
implements currently).
I'm not sure how to manage this while we don't have the CPU clock
driver ready yet but I'm sure Rob or Krzysztof will be able to help us
here.

> + * the index 4 is the clock measurement source, it's not supported yet
I suspect that this comes from the clock measurer IP block and if so
the dt-bindings should probably describe this input. But again, we'd
need to keep it optional for now since our clock measurer driver
doesn't even implement a clock controller.

[...]
> +static struct clk_regmap pwm_a_sel =3D {
> +       .data =3D &(struct clk_regmap_mux_data){
> +               .offset =3D PWM_CLK_AB_CTRL,
> +               .mask =3D 0x1,
> +               .shift =3D 9,
> +       },
> +       .hw.init =3D &(struct clk_init_data){
> +               .name =3D "pwm_a_sel",
> +               .ops =3D &clk_regmap_mux_ops,
> +               .parent_data =3D pwm_abcd_parents,
> +               .num_parents =3D ARRAY_SIZE(pwm_abcd_parents),
> +               /* For more information, please refer to rtc clock */
> +               .flags =3D CLK_SET_RATE_NO_REPARENT,
As mentioned in [0] we'll work with Heiner to see if we can improve
the decision making process of the PWM controller driver so that we
can just have .flags =3D 0 here.
This applies to all other occurrences of the same comment about the rtc clo=
ck.


Best regards,
Martin