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