Re: [PATCH 2/2] PM / devfreq: qcom: Introduce CCI devfreq driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux