Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4f12f2b254a5ea795720052cb1c58e59abe9b9ce.camel@mediatek.com>
Subject: Re: [PATCH 3/4] clk: mediatek: Add new clock driver to handle FHCTL
 hardware
From:   Edward-JW Yang <edward-jw.yang@mediatek.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Johnson Wang =?UTF-8?Q?=28=E7=8E=8B=E8=81=96=E9=91=AB=29?= 
        <Johnson.Wang@mediatek.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "sboyd@kernel.org" <sboyd@kernel.org>
CC:     "linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-mediatek@lists.infradead.org" 
        <linux-mediatek@lists.infradead.org>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@mediatek.com>
Date:   Tue, 6 Sep 2022 14:15:15 +0800
In-Reply-To: <d2abe2d3-f17c-6c22-ccae-514fe196441e@collabora.com>
References: <20220831124850.7748-1-johnson.wang@mediatek.com>
         <20220831124850.7748-4-johnson.wang@mediatek.com>
         <d2abe2d3-f17c-6c22-ccae-514fe196441e@collabora.com>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Angelo,

Thanks for suggestions.

On Thu, 2022-09-01 at 16:05 +0800, AngeloGioacchino Del Regno wrote:
> Il 31/08/22 14:48, Johnson Wang ha scritto:
> > To implement frequency hopping and spread spectrum clocking
> > function, we introduce new clock type and APIs to handle
> > FHCTL hardware.
> > 
> > Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> > ---
> >   drivers/clk/mediatek/Makefile    |   2 +-
> >   drivers/clk/mediatek/clk-fhctl.c | 258 +++++++++++++++++++++++++++++
> >   drivers/clk/mediatek/clk-fhctl.h |  27 +++
> >   drivers/clk/mediatek/clk-pllfh.c | 271 +++++++++++++++++++++++++++++++
> >   drivers/clk/mediatek/clk-pllfh.h |  81 +++++++++
> >   5 files changed, 638 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/clk/mediatek/clk-fhctl.c
> >   create mode 100644 drivers/clk/mediatek/clk-fhctl.h
> >   create mode 100644 drivers/clk/mediatek/clk-pllfh.c
> >   create mode 100644 drivers/clk/mediatek/clk-pllfh.h
> > 
> > diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> > index caf2ce93d666..0e674a55e51e 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -1,5 +1,5 @@
> >   # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o
> > +obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o clk-fhctl.o clk-pllfh.o
> >   
> >   obj-$(CONFIG_COMMON_CLK_MT6765) += clk-mt6765.o
> >   obj-$(CONFIG_COMMON_CLK_MT6765_AUDIOSYS) += clk-mt6765-audio.o
> > diff --git a/drivers/clk/mediatek/clk-fhctl.c b/drivers/clk/mediatek/clk-fhctl.c
> > new file mode 100644
> > index 000000000000..69bd4ce2bc72
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-fhctl.c
> > @@ -0,0 +1,258 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> 
> #include <linux/io.h>
> 
> Please don't rely on implicit header inclusion.

OK, I'll fix it. Thanks.

> 
> > +#include <linux/iopoll.h>
> > +
> > +#include "clk-mtk.h"
> > +#include "clk-pllfh.h"
> > +#include "clk-fhctl.h"
> > +
> > +#define PERCENT_TO_DDSLMT(dds, percent_m10) \
> > +	((((dds) * (percent_m10)) >> 5) / 100)
> > +
> > +static const struct fhctl_offset fhctl_offset = {
> > +	.offset_hp_en = 0x0,
> > +	.offset_clk_con = 0x8,
> > +	.offset_rst_con = 0xc,
> > +	.offset_slope0 = 0x10,
> > +	.offset_slope1 = 0x14,
> > +	.offset_cfg = 0x0,
> > +	.offset_updnlmt = 0x4,
> > +	.offset_dds = 0x8,
> > +	.offset_dvfs = 0xc,
> > +	.offset_mon = 0x10,
> > +};
> > +
> > +const struct fhctl_offset *fhctl_get_offset_table(void)
> > +{
> > +	return &fhctl_offset;
> > +}
> > +
> > +static void dump_hw(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +		    const struct fh_pll_data *data)
> > +{
> > +	pr_info("hp_en<%x>,clk_con<%x>,slope0<%x>,slope1<%x>\n",
> > +		readl(regs->reg_hp_en), readl(regs->reg_clk_con),
> > +		readl(regs->reg_slope0), readl(regs->reg_slope1));
> > +	pr_info("cfg<%x>,lmt<%x>,dds<%x>,dvfs<%x>,mon<%x>\n",
> > +		readl(regs->reg_cfg), readl(regs->reg_updnlmt),
> > +		readl(regs->reg_dds), readl(regs->reg_dvfs),
> > +		readl(regs->reg_mon));
> > +	pr_info("pcw<%x>\n", readl(pll->pcw_addr));
> > +}
> > +
> > +static int fhctl_set_ssc_regs(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +			      const struct fh_pll_data *data, u32 rate)
> > +{
> > +	u32 updnlmt_val, r;
> > +
> > +	writel((readl(regs->reg_cfg) & ~(data->frddsx_en)), regs->reg_cfg);
> > +	writel((readl(regs->reg_cfg) & ~(data->sfstrx_en)), regs->reg_cfg);
> > +	writel((readl(regs->reg_cfg) & ~(data->fhctlx_en)), regs->reg_cfg);
> > +
> > +	if (rate > 0) {
> > +		/* Set the relative parameter registers (dt/df/upbnd/downbnd) */
> > +		r = readl(regs->reg_cfg);
> > +		r &= ~(data->msk_frddsx_dys);
> > +		r |= (data->df_val << (ffs(data->msk_frddsx_dys) - 1));
> > +		writel(r, regs->reg_cfg);
> > +
> > +		r = readl(regs->reg_cfg);
> > +		r &= ~(data->msk_frddsx_dts);
> > +		r |= (data->dt_val << (ffs(data->msk_frddsx_dts) - 1));
> > +		writel(r, regs->reg_cfg);
> > +
> > +		writel((readl(pll->pcw_addr) & data->dds_mask) | data->tgl_org,
> > +			regs->reg_dds);
> > +
> > +		/* Calculate UPDNLMT */
> > +		updnlmt_val = PERCENT_TO_DDSLMT((readl(regs->reg_dds) &
> > +						 data->dds_mask), rate) <<
> > +						 data->updnlmt_shft;
> > +
> > +		writel(updnlmt_val, regs->reg_updnlmt);
> > +		writel(readl(regs->reg_hp_en) | BIT(data->fh_id),
> > +		       regs->reg_hp_en);
> > +		/* Enable SSC */
> > +		writel(readl(regs->reg_cfg) | data->frddsx_en, regs->reg_cfg);
> > +		/* Enable Hopping control */
> > +		writel(readl(regs->reg_cfg) | data->fhctlx_en, regs->reg_cfg);
> > +
> > +	} else {
> > +		/* Switch to APMIXEDSYS control */
> > +		writel(readl(regs->reg_hp_en) & ~BIT(data->fh_id),
> > +		       regs->reg_hp_en);
> > +		/* Wait for DDS to be stable */
> > +		udelay(30);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int hopping_hw_flow(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +			   const struct fh_pll_data *data,
> > +			   struct fh_pll_state *state, unsigned int new_dds)
> > +{
> > +	u32 dds_mask = data->dds_mask;
> > +	u32 mon_dds = 0;
> > +	u32 con_pcw_tmp;
> > +	int ret;
> > +
> > +	if (state->ssc_rate)
> > +		fhctl_set_ssc_regs(pll, regs, data, 0);
> > +
> > +	writel((readl(pll->pcw_addr) & dds_mask) | data->tgl_org,
> > +		regs->reg_dds);
> > +
> > +	writel(readl(regs->reg_cfg) | data->sfstrx_en, regs->reg_cfg);
> > +	writel(readl(regs->reg_cfg) | data->fhctlx_en, regs->reg_cfg);
> > +	writel(data->slope0_value, regs->reg_slope0);
> > +	writel(data->slope1_value, regs->reg_slope1);
> > +
> > +	writel(readl(regs->reg_hp_en) | BIT(data->fh_id), regs->reg_hp_en);
> > +	writel((new_dds) | (data->dvfs_tri), regs->reg_dvfs);
> > +
> > +	/* Wait 1000 us until DDS stable */
> > +	ret = readl_poll_timeout_atomic(regs->reg_mon, mon_dds,
> > +				       (mon_dds & dds_mask) == new_dds,
> > +					10, 1000);
> > +	if (ret) {
> > +		pr_warn("%s: FHCTL hopping timeout\n", pll->data->name);
> > +		dump_hw(pll, regs, data);
> > +	}
> > +
> > +	con_pcw_tmp = readl(pll->pcw_addr) & (~dds_mask);
> > +	con_pcw_tmp = (con_pcw_tmp | (readl(regs->reg_mon) & dds_mask) |
> > +		       data->pcwchg);
> > +
> > +	writel(con_pcw_tmp, pll->pcw_addr);
> > +	writel(readl(regs->reg_hp_en) & ~BIT(data->fh_id), regs->reg_hp_en);
> > +
> > +	if (state->ssc_rate)
> > +		fhctl_set_ssc_regs(pll, regs, data, state->ssc_rate);
> 
> Does it really make sense to set the SSC registers if the FH operation times out?

FH opration times out means DDS doesn't reach the target dds value but the de-sense issue
may still exist. We can still base on current dds_mon value to do SSC.

> 
> > +
> > +	return ret;
> > +}
> > +
> > +static unsigned int __get_postdiv(struct mtk_clk_pll *pll,
> > +				  struct fh_pll_regs *regs,
> > +				  const struct fh_pll_data *fh_data)
> 
> You don't need regs, nor fh_data, so this can be as simple as
> 
> static unsigned int __get_postdiv(struct mtk_clk_pll *pll)

OK, I'll fix it. Thanks.

> 
> > +{
> > +	unsigned int regval;
> > +
> > +	regval = readl(pll->pd_addr) >> pll->data->pd_shift;
> > +	regval &= POSTDIV_MASK;
> > +
> 
> 	return BIT(regval);

OK, I'll fix it. Thanks.

> 
> > +	return (1 << regval);
> > +}
> > +
> > +static void __set_postdiv(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +			  const struct fh_pll_data *data, int postdiv)
> 
> static void __set_postdiv(struct mtk_clk_pll *pll, unsigned int val)

OK, I'll fix it. Thanks.

> 
> > +{
> > +	unsigned int regval;
> > +
> > +	regval = readl(pll->pd_addr);
> > +	regval &= ~(POSTDIV_MASK << pll->data->pd_shift);
> > +	regval |= (ffs(postdiv) - 1) << pll->data->pd_shift;
> > +	writel(regval, pll->pd_addr);
> > +}
> > +
> > +static int fhctl_hopping(struct mtk_fh *fh, unsigned int new_dds, int postdiv)
> 
> The postdiv cannot ever be negative, so you can change it to an unsigned type...
> 
> static int fhctl_hopping(struct mtk_fh *fh, unsigned int new_dds,
> 			 unsigned int postdiv)

OK, I'll fix it. Thanks.

> 
> > +{
> > +	const struct fh_pll_data *data = &fh->pllfh_data->data;
> > +	struct fh_pll_state *state = &fh->pllfh_data->state;
> > +	struct fh_pll_regs *regs = &fh->regs;
> > +	struct mtk_clk_pll *pll = &fh->clk_pll;
> > +	spinlock_t *lock = fh->lock;
> > +	unsigned int pll_postdiv;
> > +	unsigned long flags = 0;
> > +	int ret;
> > +
> 
> ...so since postdiv is unsigned, we can write this conditional as
> 
> 	if (postdiv)

OK, I'll fix it. Thanks.

> 
> > +	if (postdiv > 0) {
> > +		pll_postdiv = __get_postdiv(pll, regs, data);
> > +
> > +		if (postdiv > pll_postdiv)
> > +			__set_postdiv(pll, regs, data, postdiv);
> > +	}
> > +
> > +	spin_lock_irqsave(lock, flags);
> > +
> > +	ret = hopping_hw_flow(pll, regs, data, state, new_dds);
> > +
> > +	spin_unlock_irqrestore(lock, flags);
> > +
> > +	if (postdiv > 0) {
> > +		if (postdiv < pll_postdiv)
> 
> ...and this one as
> 
> if (postdiv && postdiv < pll_postdiv)

OK, I'll fix it. Thanks.

> 
> > +			__set_postdiv(pll, regs, data, postdiv);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int __fhctl_ssc_enable(struct mtk_fh *fh, u32 rate)
> > +{
> > +	const struct fh_pll_data *data = &fh->pllfh_data->data;
> > +	struct fh_pll_state *state = &fh->pllfh_data->state;
> > +	struct fh_pll_regs *regs = &fh->regs;
> > +	struct mtk_clk_pll *pll = &fh->clk_pll;
> > +	spinlock_t *lock = fh->lock;
> > +	unsigned long flags = 0;
> > +
> > +	spin_lock_irqsave(lock, flags);
> > +
> > +	fhctl_set_ssc_regs(pll, regs, data, rate);
> > +	state->ssc_rate = rate;
> > +
> > +	spin_unlock_irqrestore(lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int fhctl_ssc_enable(struct mtk_fh *fh, u32 rate)
> > +{
> > +	return __fhctl_ssc_enable(fh, rate);
> > +}
> > +
> > +static int fhctl_ssc_disable(struct mtk_fh *fh)
> > +{
> > +	return __fhctl_ssc_enable(fh, 0);
> > +}
> > +
> > +static const struct fh_operation fhctl_ops = {
> > +	.hopping = fhctl_hopping,
> > +	.ssc_enable = fhctl_ssc_enable,
> 
> Just one callback for ssc_enable is enough, it's fine to keep the logic as
> ssc_enable(fh, >0) to enable and ssc_enable(fh <=0) to disable.

OK, I'll drop ssc_disable callback.

> 
> > +	.ssc_disable = fhctl_ssc_disable,
> > +};
> > +
> > +const struct fh_operation *fhctl_get_ops(void)
> > +{
> > +	return &fhctl_ops;
> > +}
> > +
> > +void fhctl_hw_init(struct mtk_fh *fh)
> > +{
> > +	const struct fh_pll_data data = fh->pllfh_data->data;
> > +	struct fh_pll_state state = fh->pllfh_data->state;
> > +	struct fh_pll_regs regs = fh->regs;
> > +	u32 val;
> > +
> > +	/* initial hw register */
> > +	val = readl(regs.reg_clk_con) | BIT(data.fh_id);
> > +	writel(val, regs.reg_clk_con);
> > +
> > +	val = readl(regs.reg_rst_con) & ~BIT(data.fh_id);
> > +	writel(val, regs.reg_rst_con);
> > +	val = readl(regs.reg_rst_con) | BIT(data.fh_id);
> > +	writel(val, regs.reg_rst_con);
> > +
> > +	writel(0x0, regs.reg_cfg);
> > +	writel(0x0, regs.reg_updnlmt);
> > +	writel(0x0, regs.reg_dds);
> > +
> > +	/* enable ssc if needed */
> > +	if (state.ssc_rate)
> > +		fh->ops->ssc_enable(fh, state.ssc_rate);
> > +}
> > diff --git a/drivers/clk/mediatek/clk-fhctl.h b/drivers/clk/mediatek/clk-fhctl.h
> > new file mode 100644
> > index 000000000000..3cd4921c39e9
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-fhctl.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> > +#ifndef __CLK_FHCTL_H
> > +#define __CLK_FHCTL_H
> > +
> 
> One blank line is enough.

OK, I'll fix it. Thanks

> 
> > +
> > +struct fhctl_offset {
> > +	u32 offset_hp_en;
> > +	u32 offset_clk_con;
> > +	u32 offset_rst_con;
> > +	u32 offset_slope0;
> > +	u32 offset_slope1;
> > +	u32 offset_cfg;
> > +	u32 offset_updnlmt;
> > +	u32 offset_dds;
> > +	u32 offset_dvfs;
> > +	u32 offset_mon;
> > +};
> > +const struct fhctl_offset *fhctl_get_offset_table(void);
> > +const struct fh_operation *fhctl_get_ops(void);
> > +void fhctl_hw_init(struct mtk_fh *fh);
> > +
> > +#endif
> > diff --git a/drivers/clk/mediatek/clk-pllfh.c b/drivers/clk/mediatek/clk-pllfh.c
> > new file mode 100644
> > index 000000000000..71b35323b526
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-pllfh.c
> > @@ -0,0 +1,271 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/io.h>
> > +#include <linux/slab.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/delay.h>
> > +
> > +#include "clk-mtk.h"
> > +#include "clk-pllfh.h"
> > +#include "clk-fhctl.h"
> > +
> > +static DEFINE_SPINLOCK(pllfh_lock);
> > +
> > +inline struct mtk_fh *to_mtk_fh(struct clk_hw *hw)
> > +{
> > +	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> > +
> > +	return container_of(pll, struct mtk_fh, clk_pll);
> > +}
> > +
> > +static int mtk_fhctl_set_rate(struct clk_hw *hw, unsigned long rate,
> > +			      unsigned long parent_rate)
> > +{
> > +	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> > +	struct mtk_fh *fh = to_mtk_fh(hw);
> > +	u32 pcw = 0;
> > +	u32 postdiv;
> > +
> > +	mtk_pll_calc_values(pll, &pcw, &postdiv, rate, parent_rate);
> > +	fh->ops->hopping(fh, pcw, postdiv);
> 
> return fh->ops->hopping(....);

OK, I'll fix it. Thanks.

> 
> > +
> > +	return 0;
> > +}
> > +
> 
> ..snip..
> 
> > diff --git a/drivers/clk/mediatek/clk-pllfh.h b/drivers/clk/mediatek/clk-pllfh.h
> > new file mode 100644
> > index 000000000000..d9f7c8527548
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-pllfh.h
> > @@ -0,0 +1,81 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> > +#ifndef __DRV_CLKFH_H
> > +#define __DRV_CLKFH_H
> > +
> > +#include "clk-pll.h"
> > +
> > +struct fh_pll_state {
> 
> Please, base as first member.

OK, I'll modify it.

> 
> > +	u32 fh_enable;
> > +	u32 ssc_rate;
> > +	void __iomem *base;
> > +};
> > +
> 
> Regards,
> Angelo
> 
>