linux-kernel - Re: [PATCH] clk: mediatek: Fix unused 'ops' field in mtk_pll

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ead37cb0-c841-df1a-ca10-a396b5e9951c@collabora.com>
Date:   Wed, 18 May 2022 14:15:13 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Boris Lysov <arz65xx@...il.com>, arzamas-16@...l.ee,
        mturquette@...libre.com, sboyd@...nel.org, matthias.bgg@...il.com,
        wenst@...omium.org, miles.chen@...iatek.com
Cc:     linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] clk: mediatek: Fix unused 'ops' field in mtk_pll_data

Il 15/05/22 14:24, Boris Lysov ha scritto:
> From: Boris Lysov <arzamas-16@...l.ee>
> 
> Allow to specify optional clk_ops in mtk_pll_data which will be picked up in
> mtk_clk_register_pll. So far no already supported Mediatek SoC needs
> non-default clk_ops for PLLs but instead of removing this field it will be
> actually used in the future for supporting older SoCs (see [1] for details)
> with quirky PLLs.
> 

Hello Boris,

I disagree about this change and would rather see the ops pointer removed
with fire.

I got that you're trying to do something about "quirky PLLs", but is it
really about the PLLs that you're mentioning being "quirky", or are they
simply a different IP?

Also, if it's just about a bit inversion and a bigger delay:
1. Bigger delay: Depending on how bigger, we may simply delay more by default
    for all PLLs, even the ones that aren't requiring us to wait for longer...
    ...after all, if it's about waiting for 10/20 *microseconds* more, that's
    really not going to affect anyone's UX, nor make things slower for real,
    as the .prepare() ops for MediaTek PLLs are seldom called.. and even if
    that wasn't true, I don't think that a total of 30uS would be that much
    detrimental to the system's overall operation latency.
    Besides, if you see a case of a PLL not just switching on and off, but
    preparing and unpreparing continuously, there must be some big issue in
    some driver, or in the clock framework somewhere (and that ain't the case);

2. Bit inversion: that can be solved simply with a flag in the prepare/unprepare
    ops for this driver... and if you want something that performs even better,
    sparing you a nanosecond or two, you can always assign an "inverted" callback
    for managing that single bit;

3. Different IP: mtk_clk_register_(name-of-the-new-ip)_pll() - I don't think that
    there's anything to explain to that one.

Regards,
Angelo

> This patch depends on series "clk: mediatek: Move to struct clk_hw provider
> APIs" [2] by Chen-Yu Tsai.
> 
> [1] https://lists.infradead.org/pipermail/linux-mediatek/2022-February/035093.html
> [2] https://lists.infradead.org/pipermail/linux-mediatek/2022-May/040921.html
> 
> Signed-off-by: Boris Lysov <arzamas-16@...l.ee>
> ---
>   drivers/clk/mediatek/clk-pll.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> index cabdf25a27f3..509959a325f0 100644
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -347,7 +347,10 @@ static struct clk_hw *mtk_clk_register_pll(const struct mtk_pll_data *data,
>   
>   	init.name = data->name;
>   	init.flags = (data->flags & PLL_AO) ? CLK_IS_CRITICAL : 0;
> -	init.ops = &mtk_pll_ops;
> +	if (data->ops)
> +		init.ops = data->ops;
> +	else
> +		init.ops = &mtk_pll_ops;
>   	if (data->parent_name)
>   		init.parent_names = &data->parent_name;
>   	else
>