Re: [PATCHv3 1/8] devfreq: exynos: Add generic exynos memory bus frequency driver

[MyungJoo Ham <myungjoo.ham@samsung.com>]

>   
>  This patch adds the generic exynos bus frequency driver for memory bus
> with DEVFREQ framework. The Samsung Exynos SoCs have the common architecture
> for memory bus between DRAM memory and MMC/sub IP in SoC. This driver can
> support the memory bus frequency driver for Exynos SoCs.
> 
> Each memory bus block has a clock for memory bus speed and frequency
> table which is changed according to the utilization of memory bus on runtime.
> And then each memory bus group has the one more memory bus blocks and
> OPP table (including frequency and voltage), regulator, devfreq-event
> devices.
> 
> There are a little difference about the number of memory bus because each Exynos
> SoC have the different sub-IP and different memory bus speed. In spite of this
> difference among Exynos SoCs, we can support almost Exynos SoC by adding
> unique data of memory bus to devicetree file.
> 
> Cc: Myungjoo Ham <myungjoo.ham@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Kukjin Kim <kgene@kernel.org>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/Kconfig          |  15 +
>  drivers/devfreq/Makefile         |   1 +
>  drivers/devfreq/exynos-busfreq.c | 589 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 605 insertions(+)
>  create mode 100644 drivers/devfreq/exynos-busfreq.c

Exynos drivers are located at drivers/devfreq/exynos/
Please relocate/rename exynos-busfreq.c

[]

> diff --git a/drivers/devfreq/exynos-busfreq.c b/drivers/devfreq/exynos-busfreq.c
> new file mode 100644
> index 0000000..b180f43
> --- /dev/null
> +++ b/drivers/devfreq/exynos-busfreq.c

[]

> +
> +#define BUS_SATURATION_RATIO	40

In order to be a common driver, this should be tunable.

Because .dts is supposed to have hardware configuration only,
you may keep a table of { chip-name, saturation ratio} in this
driver and look up the saturation ratio based on the chip-name.

> +#define SAFEVOLT		50000
> +
> +struct exynos_memory_bus_opp_info {
> +	unsigned long rate;
> +	unsigned long volt;
> +};
> +
> +struct exynos_memory_bus_block {
> +	struct clk *clk;
> +	struct exynos_memory_bus_opp_info *freq_table;
> +};
> +

[]

> +#ifdef CONFIG_PM_SLEEP
> +static int exynos_busfreq_resume(struct device *dev)
> +{
> +	struct exynos_memory_bus_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = exynos_busfreq_enable_edev(data);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable the devfreq-event devices\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int exynos_busfreq_suspend(struct device *dev)
> +{
> +	struct exynos_memory_bus_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = exynos_busfreq_disable_edev(data);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to disable the devfreq-event devices\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +#endif

Please disable regulator at suspend and resume it at resume.

You may interfere with low-power mode of corresponding voltage line,
which is often implemented to be activated if the regulator is
explicitely disabled for "alive" regulators like this one.

> +
> +static const struct dev_pm_ops exynos_busfreq_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(exynos_busfreq_suspend, exynos_busfreq_resume)
> +};
> +
> +static const struct of_device_id exynos_busfreq_of_match[] = {
> +	{ .compatible = "samsung,exynos-memory-bus", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_busfreq_of_match);
> +
> +static struct platform_driver exynos_busfreq_platdrv = {
> +	.probe		= exynos_busfreq_probe,
> +	.remove		= exynos_busfreq_remove,
> +	.driver = {
> +		.name	= "exynos-memory-bus",
> +		.owner	= THIS_MODULE,
> +		.pm	= &exynos_busfreq_pm,
> +		.of_match_table = of_match_ptr(exynos_busfreq_of_match),
> +	},
> +};
> +module_platform_driver(exynos_busfreq_platdrv);
> +
> +MODULE_DESCRIPTION("Generic Exynos Memory Bus Frequency driver");
> +MODULE_AUTHOR("Chanwoo Choi <cw00.choi@samsung.com>");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.8.5.5
> 
>