On 12/16/19 11:57 PM, Leonard Crestez wrote:

On 16.12.2019 03:00, Chanwoo Choi wrote:

quoted

Hi,

Also, I think that 'devfreq' word is not proper for device driver name.
imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.

I'll rename to "imx-bus". Calling it "imx-noc" is not appropriate 
because I also want to use it for PL301 NICs.

OK.

quoted

And, I have a question.
This driver adds the devfreq device with either passive governor
or userspace governor.

As I understood, the devfreq device with passive governor
will be operated with imx8m-ddrc.c driver.
But, when is operating with userspace governor?

There are multiple scalable buses inside the SOC, for example there's a 
NIC for display controllers and one for (pci+usb). They can use 
userspace governor for explicit frequency control.

quoted

I think that you better to add the explanation to description
for two scenarios how to operate with interconnect provider
on either passive governor or userspace governor usage case.

I'll elaborate the example in bindings.

OK.

quoted

On 12/13/19 10:51 AM, Chanwoo Choi wrote:

quoted

On 12/13/19 10:30 AM, Chanwoo Choi wrote:

quoted

Hi,

On 11/15/19 5:09 AM, Leonard Crestez wrote:

quoted

Add initial support for dynamic frequency switching on pieces of the imx
interconnect fabric.

All this driver does is set a clk rate based on an opp table, it does
not map register areas.

Signed-off-by: Leonard Crestez <redacted>
---
  drivers/devfreq/Kconfig       |   9 ++
  drivers/devfreq/Makefile      |   1 +
  drivers/devfreq/imx-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
  3 files changed, 160 insertions(+)
  create mode 100644 drivers/devfreq/imx-devfreq.c

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 923a6132e741..fef5ce831e90 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig

@@ -98,10 +98,19 @@ config ARM_IMX8M_DDRC_DEVFREQ
  	select DEVFREQ_GOV_USERSPACE
  	help
  	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
  	  adjusting DRAM frequency.
  
+config ARM_IMX_DEVFREQ
+	tristate "i.MX Generic DEVFREQ Driver"
+	depends on ARCH_MXC || COMPILE_TEST
+	select DEVFREQ_GOV_PASSIVE
+	select DEVFREQ_GOV_USERSPACE
+	help
+	  This adds the generic DEVFREQ driver for i.MX interconnects. It
+	  allows adjusting NIC/NOC frequency.
+
  config ARM_TEGRA_DEVFREQ
  	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
  	depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
  		ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
  		ARCH_TEGRA_210_SOC || \

diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 3eb4d5e6635c..61d0edee16f7 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile

@@ -8,10 +8,11 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
  obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
  
  # DEVFREQ Drivers
  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
  obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.o
  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
  obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
  
  # DEVFREQ Event Drivers

diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
new file mode 100644
index 000000000000..620b344e87aa
--- /dev/null
+++ b/drivers/devfreq/imx-devfreq.c

@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct imx_devfreq {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;
+	struct clk *clk;
+	struct devfreq_passive_data passive_data;
+};
+
+static int imx_devfreq_target(struct device *dev,
+			      unsigned long *freq, u32 flags)

Don't use space for the indentation. Please use only tab.

OK

quoted

quoted

quoted

quoted

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+	struct dev_pm_opp *new_opp;
+	unsigned long new_freq;
+	int ret;
+
+	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;
+	}
+	new_freq = dev_pm_opp_get_freq(new_opp);
+	dev_pm_opp_put(new_opp);
+
+	return clk_set_rate(priv->clk, new_freq);
+}
+
+static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static int imx_devfreq_get_dev_status(struct device *dev,
+				      struct devfreq_dev_status *stat)

ditto. Please use tab for the indentation.

quoted

+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	stat->busy_time = 0;
+	stat->total_time = 0;
+	stat->current_frequency = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static void imx_devfreq_exit(struct device *dev)
+{
+	dev_pm_opp_of_remove_table(dev);
+}
+
+static int imx_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_devfreq *priv;

How about changing the variable name 'priv' to 'imx' or 'imx_data'?
because it is not easy to catch the role of 'priv' from variable name.

The name "priv" refers to private data of current device: it is short 
and not ambiguous in this context. I don't think that mentioning "imx" 
adds any additional useful information.

It doesn't seem like there's much of a convention for "local variable 
containing private data", for example exynos-bus.c uses "struct 
exynos_bus* bus" internally.

OK. it is nitpick. Keep your style.

quoted

quoted

quoted

quoted

+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	void *govdata = NULL;

How about changing the variable name 'govdata' to 'gov_data'?
- govdata -> gov_data

OK

quoted

quoted

quoted

quoted

+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->clk = devm_clk_get(dev, NULL);

nitpick: because the clock-name is not mandatory.
Don't need to specify the clock name to inform the role of clock
of other developer/user?

For example, "ddr", "bus" and so on.

I'll call this bus, but I'm not sure it's useful when a single clock is 
involved.

quoted

quoted

And, this driver doesn't include the 'clk_prepare_enable'.
how to enable the clock?

Clocks are either always on or perhaps controlled by some other 
peripheral. This driver only provides scaling.

It is not proper use-case of clock. If device driver
want to control the clock, it have to be enabled on device driver.

Even it clock is always, the user don't know the state of clock.
Also, user can't know what kind of device driver control the clock.

It have to be controlled on this device driver
before changing the clock frequency.

quoted

quoted

quoted

quoted

+	if (IS_ERR(priv->clk)) {
+		ret = PTR_ERR(priv->clk);
+		dev_err(dev, "failed to fetch clk: %d\n", ret);
+		return ret;
+	}
+	platform_set_drvdata(pdev, priv);
+
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get OPP table\n");
+		return ret;
+	}
+
+	priv->profile.polling_ms = 1000;
+	priv->profile.target = imx_devfreq_target;
+	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
+	priv->profile.exit = imx_devfreq_exit;
+	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	/* Handle passive devfreq parent link */
+	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (!IS_ERR(priv->passive_data.parent)) {
+		dev_info(dev, "setup passive link to %s\n",
+			 dev_name(priv->passive_data.parent->dev.parent));
+		gov = DEVFREQ_GOV_PASSIVE;
+		govdata = &priv->passive_data;
+	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
+		// -ENODEV means no parent: not an error.
+		ret = PTR_ERR(priv->passive_data.parent);
+		if (ret != -EPROBE_DEFER)
+			dev_warn(dev, "failed to get initialize passive parent: %d\n",
+				 ret);
+		goto err;
+	}

You better to change the exception handling as following: It is more simple.

	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
		goto err;
	} else {
		ret = PTR_ERR(priv->passive_data.parent);
		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
		goto err;
	}

But -ENODEV is not an error, it means no passive parent was found.

OK. just I want to make 'if statement' more simple. This style
is complicated.

quoted

quoted

quoted

quoted

+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, govdata);
+	if (IS_ERR(priv->devfreq)) {
+		ret = PTR_ERR(priv->devfreq);
+		dev_err(dev, "failed to add devfreq device: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	dev_pm_opp_of_remove_table(dev);
+	return ret;
+}
+
+static const struct of_device_id imx_devfreq_of_match[] = {
+	{ .compatible = "fsl,imx8m-noc", },
+	{ .compatible = "fsl,imx8m-nic", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
+
+static struct platform_driver imx_devfreq_platdrv = {
+	.probe		= imx_devfreq_probe,
+	.driver = {
+		.name	= "imx-devfreq",
+		.of_match_table = of_match_ptr(imx_devfreq_of_match),
+	},
+};
+module_platform_driver(imx_devfreq_platdrv);
+
+MODULE_DESCRIPTION("Generic i.MX bus frequency driver");

If this driver is for bus frequency, you better to use 'bus' for the clock-name
for the readability.

OK

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel