| 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: <d2abe2d3-f17c-6c22-ccae-514fe196441e@collabora.com>
Date: Thu, 1 Sep 2022 10:05:00 +0200
From: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>
To: Johnson Wang <johnson.wang@...iatek.com>, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, sboyd@...nel.org
Cc: linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org,
Project_Global_Chrome_Upstream_Group@...iatek.com,
Edward-JW Yang <edward-jw.yang@...iatek.com>
Subject: Re: [PATCH 3/4] clk: mediatek: Add new clock driver to handle FHCTL
hardware
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@...iatek.com>
> Signed-off-by: Edward-JW Yang <edward-jw.yang@...iatek.com>
> Signed-off-by: Johnson Wang <johnson.wang@...iatek.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@...iatek.com>
> + */
> +
#include <linux/io.h>
Please don't rely on implicit header inclusion.
> +#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?
> +
> + 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)
> +{
> + unsigned int regval;
> +
> + regval = readl(pll->pd_addr) >> pll->data->pd_shift;
> + regval &= POSTDIV_MASK;
> +
return BIT(regval);
> + 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)
> +{
> + 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)
> +{
> + 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)
> + 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)
> + __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.
> + .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@...iatek.com>
> + */
> +
> +#ifndef __CLK_FHCTL_H
> +#define __CLK_FHCTL_H
> +
One blank line is enough.
> +
> +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@...iatek.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(....);
> +
> + 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@...iatek.com>
> + */
> +
> +#ifndef __DRV_CLKFH_H
> +#define __DRV_CLKFH_H
> +
> +#include "clk-pll.h"
> +
> +struct fh_pll_state {
Please, base as first member.
> + u32 fh_enable;
> + u32 ssc_rate;
> + void __iomem *base;
> +};
> +
Regards,
Angelo
Powered by blists - more mailing lists