linux-kernel - Re: [PATCH v3 7/9] clk: mediatek: Fix asymmetrical PLL enable and disable control

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]

Date:   Thu, 1 Oct 2020 16:15:43 +0200
From:   Matthias Brugger <matthias.bgg@...il.com>
To:     Weiyi Lu <weiyi.lu@...iatek.com>, Rob Herring <robh@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Nicolas Boichat <drinkcat@...omium.org>
Cc:     James Liao <jamesjj.liao@...iatek.com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-mediatek@...ts.infradead.org, linux-clk@...r.kernel.org,
        srv_heupstream@...iatek.com, Wendell Lin <wendell.lin@...iatek.com>
Subject: Re: [PATCH v3 7/9] clk: mediatek: Fix asymmetrical PLL enable and
 disable control



On 03/09/2020 05:22, Weiyi Lu wrote:
> The en_mask actually is a combination of divider enable mask
> and pll enable bit(bit0).
> Before this patch, we enabled both divider mask and bit0 in prepare(),
> but only cleared the bit0 in unprepare().
> Now, setting the enable register(CON0) in 2 steps: first divider mask,
> then bit0 during prepare(), vice versa.
> Hence, en_mask will only be used as divider enable mask.
> Meanwhile, all the SoC PLL data are updated.
> 
> Signed-off-by: Weiyi Lu <weiyi.lu@...iatek.com>
> ---
>   drivers/clk/mediatek/clk-mt2701.c | 26 +++++++++++++-------------
>   drivers/clk/mediatek/clk-mt2712.c | 30 +++++++++++++++---------------
>   drivers/clk/mediatek/clk-mt6765.c | 20 ++++++++++----------
>   drivers/clk/mediatek/clk-mt6779.c | 24 ++++++++++++------------
>   drivers/clk/mediatek/clk-mt6797.c | 20 ++++++++++----------
>   drivers/clk/mediatek/clk-mt7622.c | 18 +++++++++---------
>   drivers/clk/mediatek/clk-mt7629.c | 12 ++++++------
>   drivers/clk/mediatek/clk-mt8173.c | 28 ++++++++++++++--------------
>   drivers/clk/mediatek/clk-mt8183.c | 22 +++++++++++-----------
>   drivers/clk/mediatek/clk-pll.c    | 16 ++++++++++++----
>   10 files changed, 112 insertions(+), 104 deletions(-)
> 
[...]
>   
> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> index f440f2cd..e0b00bc 100644
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -247,10 +247,14 @@ static int mtk_pll_prepare(struct clk_hw *hw)
>   	writel(r, pll->pwr_addr);
>   	udelay(1);
>   
> -	r = readl(pll->base_addr + REG_CON0);
> -	r |= pll->data->en_mask;
> +	r = readl(pll->base_addr + REG_CON0) | CON0_BASE_EN;
>   	writel(r, pll->base_addr + REG_CON0);
>   
> +	if (pll->data->en_mask) {
> +		r = readl(pll->base_addr + REG_CON0) | pll->data->en_mask;
> +		writel(r, pll->base_addr + REG_CON0);
> +	}
> +

I think a better approach here would be to add a flag to mtk_pll_data instead of 
changing all drivers in one big patch. This will allow you to add the driver 
that needs to write the en_mask after writing CON0_BASE_EN more easily. And it 
will later allow you to change the remaining driver one by one until all are 
using the new flag.

Regards,
Matthias