| 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
| ||
|
Message-ID: <1j4jvwavoe.fsf@starbuckisacylon.baylibre.com>
Date: Sat, 22 Oct 2022 14:22:15 +0200
From: Jerome Brunet <jbrunet@...libre.com>
To: Yu Tu <yu.tu@...ogic.com>, linux-clk@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
Neil Armstrong <narmstrong@...libre.com>,
Kevin Hilman <khilman@...libre.com>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: Re: [PATCH V3 3/6] clk: meson: S4: add support for Amlogic S4 SoC
PLL clock driver
On Thu 29 Sep 2022 at 15:07, Yu Tu <yu.tu@...ogic.com> wrote:
> Hi Jerome,
> Thank you for your reply.
>
> On 2022/9/28 23:27, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> On Wed 21 Sep 2022 at 16:40, Yu Tu <yu.tu@...ogic.com> wrote:
>>
>>> Hi Jerome，
>>>
>>> On 2022/8/30 15:37, Yu Tu wrote:
>>>> On 2022/8/30 14:44, Jerome Brunet wrote:
>>>>> [ EXTERNAL EMAIL ]
>>>>>
>>>>>
>>>>> On Tue 30 Aug 2022 at 14:13, Yu Tu <yu.tu@...ogic.com> wrote:
>>>>>
>>>>>> On 2022/8/29 17:48, Jerome Brunet wrote:
>>>>>>> [ EXTERNAL EMAIL ]
>>>>>>> On Mon 15 Aug 2022 at 21:20, Yu Tu <yu.tu@...ogic.com> wrote:
>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> +static struct clk_regmap s4_hdmi_pll_dco = {
>>>>>>>>>>> + .data = &(struct meson_clk_pll_data){
>>>>>>>>>>> + .en = {
>>>>>>>>>>> + .reg_off = ANACTRL_HDMIPLL_CTRL0,
>>>>>>>>>>> + .shift = 28,
>>>>>>>>>>> + .width = 1,
>>>>>>>>>>> + },
>>>>>>>>>>> + .m = {
>>>>>>>>>>> + .reg_off = ANACTRL_HDMIPLL_CTRL0,
>>>>>>>>>>> + .shift = 0,
>>>>>>>>>>> + .width = 8,
>>>>>>>>>>> + },
>>>>>>>>>>> + .n = {
>>>>>>>>>>> + .reg_off = ANACTRL_HDMIPLL_CTRL0,
>>>>>>>>>>> + .shift = 10,
>>>>>>>>>>> + .width = 5,
>>>>>>>>>>> + },
>>>>>>>>>>> + .frac = {
>>>>>>>>>>> + .reg_off = ANACTRL_HDMIPLL_CTRL1,
>>>>>>>>>>> + .shift = 0,
>>>>>>>>>>> + .width = 17,
>>>>>>>>>>> + },
>>>>>>>>>>> + .l = {
>>>>>>>>>>> + .reg_off = ANACTRL_HDMIPLL_CTRL0,
>>>>>>>>>>> + .shift = 31,
>>>>>>>>>>> + .width = 1,
>>>>>>>>>>> + },
>>>>>>>>>>> + .rst = {
>>>>>>>>>>> + .reg_off = ANACTRL_HDMIPLL_CTRL0,
>>>>>>>>>>> + .shift = 29,
>>>>>>>>>>> + .width = 1,
>>>>>>>>>>> + },
>>>>>>>>>>> + },
>>>>>>>>>>> + .hw.init = &(struct clk_init_data){
>>>>>>>>>>> + .name = "hdmi_pll_dco",
>>>>>>>>>>> + .ops = &meson_clk_pll_ro_ops,
>>>>>>>>>>> + .parent_data = (const struct clk_parent_data []) {
>>>>>>>>>>> + { .fw_name = "xtal", }
>>>>>>>>>>> + },
>>>>>>>>>>> + .num_parents = 1,
>>>>>>>>>>> + /*
>>>>>>>>>>> + * Display directly handle hdmi pll registers ATM, we need
>>>>>>>>>>> + * NOCACHE to keep our view of the clock as accurate as
>>>>>>>>>>> + * possible
>>>>>>>>>>> + */
>>>>>>>>>>
>>>>>>>>>> Is it really ?
>>>>>>>>>>
>>>>>>>>>> Given that HDMI support for the s4 is there yet, the
>>>>>>>>>> addresses have changes and the region is no longer a syscon, it is
>>>>>>>>>> time
>>>>>>>>>> for the HDMI driver to get fixed.
>>>>>>>> The HDMI PLL is configured in the Uboot phase and does not change the
>>>>>>>> frequency in the kernel phase. So we use the NOCACHE flag and
>>>>>>>> "ro_ops".
>>>>>>> That's no reason to put NOCACHE or ro-ops
>>>>>>> If you want the frequencies to be statically assinged, the correct way
>>>>>>> would be through assigned-rate in DT I guess.
>>>>>>
>>>>>> Okay. You're right. However, when registering with OPS, HDMI PLL will be
>>>>>> reset. It takes time for PLL to stabilize the output frequency, which
>>>>>> will
>>>>>> lead to the startup screen flashing.
>>>>>>
>>>>>> I would like to know how to solve this problem if not using ro_ops.
>>>>>>
>>>>>>>
>>>>>
>>>>> You can add new ops or tweak the current init function.
>>>> HDMI PLL is not different from other PLLS, so I think adding OPS is
>>>> weird.
>>>>
>>>>>
>>>>> Safest would be to do the following :
>>>>> * Check if the PLLs is already on.
>>>>> * Check if the 'pll->init_regs' matches what is already set
>>>>> - if so, you can skip the reset
>>>>> - if not, you need to reset as usual
>>>> static int meson_clk_pll_init(struct clk_hw *hw)
>>>> {
>>>> struct clk_regmap *clk = to_clk_regmap(hw);
>>>> struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>>>> if (pll->init_count) {
>>>> meson_parm_write(clk->map, &pll->rst, 1);
>>>> regmap_multi_reg_write(clk->map, pll->init_regs,
>>>> | pll->init_count);
>>>> meson_parm_write(clk->map, &pll->rst, 0);
>>>> }
>>>> return 0;
>>>> }
>>>> Because the init function looks like this. Therefore, HDMI PLL init_count
>>>> is not given.
>> I don't get the remark. You've got pll->init_count right there.
>>
>>>> Can I change it like this?
>> What change ? The function above looks a lot like meson_clk_pll_init()
>> in the actual source
>>
>>>
>>> I don't know if this change meets your requirements? Please give us your
>>> valuable advice.
>> What change ?
>
> static struct clk_regmap s4_hdmi_pll_dco = { .data = &(struct
> meson_clk_pll_data){ .en = {
> .reg_off = ANACTRL_HDMIPLL_CTRL0, .shift = 28,
> .width = 1, }, .m = {
> .reg_off = ANACTRL_HDMIPLL_CTRL0, .shift = 0,
> .width = 8, }, .n = {
> .reg_off = ANACTRL_HDMIPLL_CTRL0, .shift = 10,
> .width = 5, }, .frac = {
> .reg_off = ANACTRL_HDMIPLL_CTRL1, .shift = 0,
> .width = 17, }, .l = {
> .reg_off = ANACTRL_HDMIPLL_CTRL0, .shift = 31,
> .width = 1, }, .rst = {
> .reg_off = ANACTRL_HDMIPLL_CTRL0, .shift = 29,
> .width = 1, }, .range =
> &s4_gp0_pll_mult_range, }, .hw.init = &(struct
> clk_init_data){ .name = "hdmi_pll_dco",
> .ops = &meson_clk_pll_ops, .parent_data = (const struct
> clk_parent_data []) { { .fw_name = "xtal", }
> }, .num_parents = 1, }, };
I'm sorry but I can't read this
>
> This is my code right now. Because init_count and init_regs are not
> defined, HDMI PLL is not reset. In this way, the kernel will not blink when
> the Uboot starts. Then in the kernel stage, if we want to change the HDMI
> PLL frequency value, we can directly change M, N and OD. In fact, we will
> not change the HDMI PLL frequency value later.
>
> I wonder if you accept this change?
I think it would be better to separate this topic from your new SoC support
I think you can make another version of the clock controller for the new
SoC, keeping the pll_ops as it is now.
With another series, you may submit something to avoid reseting the PLL if
is already correctly initialized. This can be useful to all the SoC, not
just the last one. It will be easier to discuss on properly formatted
patches
Powered by blists - more mailing lists