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

[Matthias Brugger]

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@xxxxxxxxxxxx>
> ---
>   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