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

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

 



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'?

> +
> +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. 

> +	kfree(speedbin);
> +
> +	/* Convert speedbin and PVS into version bit map */
> +	switch (SPEED_PVS(speed, pvs)) {
> +	case SPEED_PVS(0, 0): return 0x1;
> +	case SPEED_PVS(2, 0): return 0x2;
> +	case SPEED_PVS(2, 2): return 0x4;
> +	case SPEED_PVS(4, 0): return 0x8;
> +	case SPEED_PVS(5, 0): return 0x10;
> +	case SPEED_PVS(5, 6): return 0x20;
> +	default:
> +		return 0x1;> +	}
> +}
> +
> +static void qcom_cci_exit(struct device *dev)
> +{
> +	dev_pm_opp_of_remove_table(dev);
> +}
> +
> +static int qcom_cci_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct qcom_cci *priv;
> +	const char *gov = DEVFREQ_GOV_USERSPACE;

Even if you select 'DEVFREQ_GOV_PASSIVE' on Kconfig,
Is it right that this driver use the userspace governor?


> +	struct device_node *np = dev->of_node;
> +	struct nvmem_cell *speedbin_nvmem;
> +	int ret;
> +	u32 version;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		ret = PTR_ERR(priv->clk);
> +		dev_err(dev, "failed to fetch clk: %d\n", ret);
> +		return ret;

You can use dev_err_probe as following:

	return dev_err_probe(dev, ret, "failed to fetch clk: %d\n", ret);

> +	}
> +	platform_set_drvdata(pdev, priv);
> +
> +	/* Check whether we have profiled speed version per chip */
> +	speedbin_nvmem = of_nvmem_cell_get(np, NULL);
> +	if (IS_ERR(speedbin_nvmem))
> +		return PTR_ERR(speedbin_nvmem);

I recommend that you need to add the fail log with dev_err.

> +
> +	version = qcom_get_dev_version(speedbin_nvmem);
> +	dev_info(dev, "%s: set opp table version 0x%x\n", __func__, version);

Don't need to use '__func__' because dev_info will print the module name.
It is enough.

> +
> +	nvmem_cell_put(speedbin_nvmem);
> +	ret = dev_pm_opp_set_supported_hw(dev, &version, 1);
> +	if (ret) {
> +		dev_err(dev, "Failed to set supported hardware\n");
> +		return ret;
> +	}
> +
> +	ret = dev_pm_opp_of_add_table(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get OPP table\n");
> +		return ret;
> +	}
> +
> +	priv->profile.target = qcom_cci_target;
> +	priv->profile.exit = qcom_cci_exit;
> +	priv->profile.get_cur_freq = qcom_cci_get_cur_freq;
> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
> +
> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
> +						gov, NULL);
> +	if (IS_ERR(priv->devfreq)) {
> +		ret = PTR_ERR(priv->devfreq);
> +		dev_err(dev, "failed to add devfreq device: %d\n", ret);
> +		goto err;

Need to goto 'err_remove_opp_table' instead of 'err'.

> +	}
> +
> +	return 0;
> +
> +err:
> +	dev_pm_opp_of_remove_table(dev);
> +	return ret;

For more correct exception handling, need to change it as following:

err_remove_opp_table:
	dev_pm_opp_of_remove_table(dev);
err:
	return ret;

> +}
> +
> +static const struct of_device_id qcom_cci_of_match[] = {
> +	{ .compatible = "qcom,msm8939-cci"},
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_cci_of_match);
> +
> +static struct platform_driver qcom_cci_platdrv = {
> +	.probe		= qcom_cci_probe,
> +	.driver = {
> +		.name	= "qcom-cci-devfreq",
> +		.of_match_table = qcom_cci_of_match,
> +	},
> +};
> +module_platform_driver(qcom_cci_platdrv);
> +
> +MODULE_DESCRIPTION("QCOM cci frequency scaling driver");

cci is the abbreviation. You need to use the captical letter as following:

MODULE_DESCRIPTION("QCOM CCI frequency scaling driver");

> +MODULE_AUTHOR("Jun Nie <jun.nie@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL");

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi




[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