Chanwoo Choi <cwchoi00@xxxxxxxxx> 于2023年2月1日周三 19:02写道：
>
> Hi,
>
> On 23. 2. 1. 17:02, Jun Nie wrote:
> > Cache Coherent Interconnect (CCI) is used by some Qualcomm SoCs. This
> > driver is introduced so that its freqency can be adjusted. And regulator
> > associated with opp table can be also adjusted accordingly which is
> > shared with cpu cluster.
> >
> > Signed-off-by: Jun Nie <jun.nie@xxxxxxxxxx>
> > ---
> > drivers/devfreq/Kconfig | 9 +++
> > drivers/devfreq/Makefile | 1 +
> > drivers/devfreq/qcom-cci.c | 162 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 172 insertions(+)
> > create mode 100644 drivers/devfreq/qcom-cci.c
> >
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 9754d8b31621..6f3f1872f3fb 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -130,6 +130,15 @@ config ARM_MEDIATEK_CCI_DEVFREQ
> > buck voltages and update a proper CCI frequency. Use the notification
> > to get the regulator status.
> >
> > +config ARM_QCOM_CCI_DEVFREQ
> > + tristate "QUALCOMM CCI DEVFREQ Driver"
> > + depends on ARCH_QCOM || COMPILE_TEST
> > + select DEVFREQ_GOV_PASSIVE
> > + help
> > + This adds a devfreq driver for Qualcomm Cache Coherent Interconnect which
> > + shares the same regulator with the cpu cluster. This driver can track a
> > + proper regulator state while CCI frequency is updated.
>
> Maybe, this driver use the passive governor because as this description,
> the regulator is shared with cpu cluster. But, as I commented below,
> you use the 'userspace' governor in probe. is it right?
>
> > +
> > config ARM_RK3399_DMC_DEVFREQ
> > tristate "ARM RK3399 DMC DEVFREQ Driver"
> > depends on (ARCH_ROCKCHIP && HAVE_ARM_SMCCC) || \
> > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> > index bf40d04928d0..f2526d8c168d 100644
> > --- a/drivers/devfreq/Makefile
> > +++ b/drivers/devfreq/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
> > obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ) += imx-bus.o
> > obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o
> > obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ) += mtk-cci-devfreq.o
> > +obj-$(CONFIG_ARM_QCOM_CCI_DEVFREQ) += qcom-cci.o
> > obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
> > obj-$(CONFIG_ARM_SUN8I_A33_MBUS_DEVFREQ) += sun8i-a33-mbus.o
> > obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o
> > diff --git a/drivers/devfreq/qcom-cci.c b/drivers/devfreq/qcom-cci.c
> > new file mode 100644
> > index 000000000000..21b5f133cddc
> > --- /dev/null
> > +++ b/drivers/devfreq/qcom-cci.c
> > @@ -0,0 +1,162 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2023 Linaro Ltd.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/devfreq.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#define SPEED_PVS(s, p) ((s << 16) | p)
>
> What it meaning of PVS? Could you add the comment for 'PVS' and 's', 'p'?
>
This is what I have from Qualcomm code. Maybe others have more info about it.
Process Voltage Scaling(PVS) Tables defines the voltage and frequency
value based on the msm-id in SMEM and speedbin blown in the efuse
combination.
> > +
> > +struct qcom_cci {
> > + struct devfreq_dev_profile profile;
> > + struct devfreq *devfreq;
> > + struct clk *clk;
> > +};
> > +
> > +static int qcom_cci_target(struct device *dev,
> > + unsigned long *freq, u32 flags)
>
> Actually, this line is not long. You can type it on one line as following:
>
> static int qcom_cci_target(struct device *dev, unsigned long *freq, u32 flags)
>
> > +{
> > + struct dev_pm_opp *new_opp;
> > + int ret;
>
> As I mentioned belwo, this local variable is not needed
> if just return PTR_ERR(new_opp).
>
> > +
> > + new_opp = devfreq_recommended_opp(dev, freq, flags);
> > + if (IS_ERR(new_opp)) {
> > + ret = PTR_ERR(new_opp);
> > + dev_err(dev, "failed to get recommended opp: %d\n", ret);
> > + return ret;
>
> Better to add 'return PTR_ERR(new_opp)' without 'ret' local variable.
>
> > + }
> > + dev_pm_opp_put(new_opp);
> > +
> > + return dev_pm_opp_set_rate(dev, *freq);
> > +}
> > +
> > +static int qcom_cci_get_cur_freq(struct device *dev, unsigned long *freq)
> > +{
> > + struct qcom_cci *priv = dev_get_drvdata(dev);
> > +
> > + *freq = clk_get_rate(priv->clk);
> > +
> > + return 0;
> > +}
> > +
> > +static int qcom_get_dev_version(struct nvmem_cell *speedbin_nvmem)
> > +{
> > + int speed = 0, pvs = 0;
> > + u8 *speedbin;
> > + size_t len;
> > +
> > + speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> > + if (IS_ERR(speedbin))
> > + return PTR_ERR(speedbin);
> > +
> > + speed = (speedbin[0xc] >> 2) & 0x7;
> > + pvs = (speedbin[0x3] >> 5 & 0x1) | ((speedbin[0x6] >> 2 & 0x3) << 1);
>
> Actually, 0xc, 0x3, 0x7, 0x1 and so on. It is impossible to understand
> the meaning of this hex value. Plesae add the constant defintion
> for the readability.
>
Thanks for pointing it. The bit wise manipulation will be converted
into nvmem offset and
bit position in next version. So driver will read out value directly
without bit shift/mask.
Other comments will be adopted in next version. Thanks!
- Jun
