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
