Re: [PATCH v5 2/3] clk: meson: add a driver for the Meson8/8b/8m2 SDHC clock controller

[Martin Blumenstingl]

Hi Jerome,

thank you for looking into this!

On Mon, Apr 27, 2020 at 10:41 AM Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:
[...]
> > +#include "clk-regmap.h"
> > +#include "clk-pll.h"
>
> If you need the pll clocks, it should probably appear in the Kconfig
> deps as well
this driver does not need "clk-pll.h"
good catch - thank you

> > +
> > +#define MESON_SDHC_CLKC                      0x10
> > +
> > +static const struct clk_regmap meson_mx_sdhc_src_sel = {
> > +     .data = &(struct clk_regmap_mux_data){
> > +             .offset = MESON_SDHC_CLKC,
> > +             .mask = 0x3,
> > +             .shift = 16,
> > +     },
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "sdhc_src_sel",
> > +             .ops = &clk_regmap_mux_ops,
> > +             .parent_data = (const struct clk_parent_data[]) {
> > +                     { .fw_name = "clkin0", .index = -1, },
> > +                     { .fw_name = "clkin1", .index = -1, },
> > +                     { .fw_name = "clkin2", .index = -1, },
> > +                     { .fw_name = "clkin3", .index = -1, },
>
> When fw_name is specified, setting the index is not necessary
noted, will fix this

[...]
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "sdhc_div",
> > +             .ops = &clk_regmap_divider_ops,
> > +             .parent_data = (const struct clk_parent_data[]) {
> > +                     { .name = "sdhc_src_sel", .index = -1, },
>
> Any reason for using the lagacy names and not the clk_hw pointers ?
> Same comment for the rest of the clocks
indeed, there is a reason and it took me a while to figure out
__clk_register will set hw->init = NULL;
This means: if we unregister the driver and register it again all
hw->init will be lost (as it's now NULL)
This is why I am effectively cloning (devm_kzalloc + memcpy) these
clocks which only serve as a template
Due to this I can't easily use a reference to another clk_hw

We don't have this problem in any of our other clock controller
drivers because these cannot be unloaded

[...]
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "sdhc_mod_clk_on",
> > +             .ops = &clk_regmap_gate_ops,
> > +             .parent_data = (const struct clk_parent_data[]) {
> > +                     { .name = "sdhc_div", .index = -1, },
> > +             },
> > +             .num_parents = 1,
> > +             .flags = CLK_SET_RATE_GATE,
>
> Why can't the clock change rate unless gated ? Maybe you prefer to
> change the rate in the mmc while clock is gated, but this is the
> handling of the clock by the mmc driver, not a constraint of the actual
> clock HW, isn't it ?
>
> Also, this is a gate so I suppose the rate propagates through it ?
> Can you explain why CLK_SET_RATE_PARENT is not set  ?
[...]
> Ok so apparently you only want to set the rate through the RX clock.
> You are free to call set_rate() only on this clock in the mmc driver.
> However, I don't think this should reflect as clock constraints.
I think these two belong together
looking back at this I believe that you are right:
- CLK_SET_RATE_GATE should be dropped because that's not a constraint
of the clock but of the clock consumer (MMC driver)
- CLK_SET_RATE_PARENT should be added to all clocks because rate
propagation will work for all clocks

> > +     },
> > +};
> > +
> > +static const struct clk_regmap meson_mx_sdhc_sd_clk_en = {
> > +     .data = &(struct clk_regmap_gate_data){
> > +             .offset = MESON_SDHC_CLKC,
> > +             .bit_idx = 12,
> > +     },
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "sdhc_sd_clk_on",
> > +             .ops = &clk_regmap_gate_ops,
> > +             .parent_data = (const struct clk_parent_data[]) {
> > +                     { .name = "sdhc_div", .index = -1, },
> > +             },
> > +             .num_parents = 1,
> > +             .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>
> ... now I lost with these flags. I'm sure there is an idea related to
> the mmc driver. Clockwise, I don't get it.
indeed, just like above I'll fix these


Martin