Re: [PATCH v2 02/18] clk: intel: Add clock driver for Intel MIPS SoCs

[Stephen Boyd]

Quoting yixin zhu (2018-08-08 01:52:20)
> On 8/8/2018 1:50 PM, Stephen Boyd wrote:
> > Quoting Songjun Wu (2018-08-02 20:02:21)
> >> +       struct clk *clk;
> >> +       int idx;
> >> +
> >> +       for (idx = 0; idx < nr_clks; idx++, osc++) {
> >> +               if (!osc->dt_freq ||
> >> +                   of_property_read_u32(ctx->np, osc->dt_freq, &freq))
> >> +                       freq = osc->def_rate;
> >> +
> >> +               clk = clk_register_fixed_rate(NULL, osc->name, NULL, 0, freq);
> > Should come from DT itself.
> Yes. It can be defined as fixed-clock node in device tree.
> Do you mean it should be defined in device tree and driver reference it 
> via device tree?

Yes the oscillator should be in DT and then the DT node here can call
clk_get() or just hardcode the parent name to be what it knows it is.
Eventually we'd like to be able to move away from string names for
hierarchy descriptions but that's far off. To get there, we would need
DT nodes for clock controllers to indicate their clk parents with the
clocks and clock-names properties. So for the oscillator, DT would
define it and then the driver would eventually have a way to specify
that some parent is index 5 or clock name "foo" and then the clk core
could figure out the linkage. I haven't written that code yet, but I'll
probably do it soon if nobody beats me to it.

> >> +/**
> >> + * struct intel_clk_provider
> >> + * @map: regmap type base address for register.
> >> + * @np: device node
> >> + * @clk_data: array of hw clocks and clk number.
> >> + */
> >> +struct intel_clk_provider {
> >> +       struct regmap           *map;
> >> +       struct device_node      *np;
> >> +       struct clk_onecell_data clk_data;
> > Please register clk_hw pointers instead of clk pointers with the of
> > provider APIs.
> Sorry. I'm not sure I understand you correctly.
> If only registering clk_hw pointer, not registering of_provider API, then
> how to reference it in the user drivers ?
> Could you please give me more hints ?

Clk provider drivers shouldn't be using clk pointers directly. Usually
when that happens something is wrong. So new clk drivers should register
clk_hw pointers and pretty much only deal with clk_hw pointers instead
of struct clk pointers. You still register an of_provider, but that
provider hands out clk_hw pointers so that clk provider drivers aren't
tempted to use struct clk pointers.

> 
> 
> >
> >> + */
> >> +struct intel_pll_clk {
> >> +       unsigned int            id;
> >> +       const char              *name;
> >> +       const char              *const *parent_names;
> >> +       u8                      num_parents;
> > Can the PLL have multiple parents?
> Yes. But not in this platform.
> The define here make it easy to expand to support new platform.
> 

Ok, so it has a mux inside.

> 
> >> +       unsigned int                    id;
> >> +       enum intel_clk_type             type;
> >> +       const char                      *name;
> >> +       const char                      *const *parent_names;
> >> +       u8                              num_parents;
> >> +       unsigned long                   flags;
> >> +       unsigned int                    mux_off;
> >> +       u8                              mux_shift;
> >> +       u8                              mux_width;
> >> +       unsigned long                   mux_flags;
> >> +       unsigned int                    mux_val;
> >> +       unsigned int                    div_off;
> >> +       u8                              div_shift;
> >> +       u8                              div_width;
> >> +       unsigned long                   div_flags;
> >> +       unsigned int                    div_val;
> >> +       const struct clk_div_table      *div_table;
> >> +       unsigned int                    gate_off;
> >> +       u8                              gate_shift;
> >> +       unsigned long                   gate_flags;
> >> +       unsigned int                    gate_val;
> >> +       unsigned int                    mult;
> >> +       unsigned int                    div;
> >> +};
> >> +
> >> +/* clock flags definition */
> >> +#define CLOCK_FLAG_VAL_INIT    BIT(16)
> >> +#define GATE_CLK_HW            BIT(17)
> >> +#define GATE_CLK_SW            BIT(18)
> >> +#define GATE_CLK_VT            BIT(19)
> > What does VT mean? Virtual?
> Yes. VT means virtual here.
> Will change to GATE_CLK_VIRT.
> 

Is it a hardware concept? Or virtualization with hypervisor?

> >
> >> +}
> >> +
> >> +CLK_OF_DECLARE(intel_grx500_cgu, "intel,grx500-cgu", grx500_clk_init);
> > Any reason a platform driver can't be used instead of CLK_OF_DECLARE()?
> It provides CPU clock which is used in early boot stage.
> 

Ok. What is the CPU clock doing in early boot stage? Some sort of timer
frequency? If the driver can be split into two pieces, one to handle the
really early stuff that must be in place to get timers up and running
and the other to register the rest of the clks that aren't critical from
a regular platform driver it would be good. That's preferred model if
something is super critical.