| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 29 Aug 2018 14:56:22 +0800
From: "Zhu, Yi Xin" <yixin.zhu@...ux.intel.com>
To: Stephen Boyd <sboyd@...nel.org>,
Songjun Wu <songjun.wu@...ux.intel.com>,
chuanhua.lei@...ux.intel.com, hua.ma@...ux.intel.com,
qi-ming.wu@...el.com
Cc: linux-mips@...ux-mips.org, linux-clk@...r.kernel.org,
linux-serial@...r.kernel.org, devicetree@...r.kernel.org,
Michael Turquette <mturquette@...libre.com>,
linux-kernel@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v2 02/18] clk: intel: Add clock driver for Intel MIPS SoCs
On 8/28/2018 3:09 AM, Stephen Boyd wrote:
> 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.
Thanks. Will update.
>
>>>> +/**
>>>> + * 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.
Understood. Will update to use clk_hw_onecell_data and change the
registration accordingly.
>>
>>>> + */
>>>> +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?
Some peripheral drivers want to use same code cross platforms.
But not all platforms provide HW gate clock. So in this case, clock
driver creates
a virtual gate clock to make it work if no HW gate clock in the SoC.
>
>>>> +}
>>>> +
>>>> +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.
Yes, CPU clock is providing CPU frequency in the early boot stage.
Will put the non-critical clocks in the platform driver.
>
Powered by blists - more mailing lists