On 12.11.2019 02:54, Chanwoo Choi wrote:
> On 11/12/19 2:23 AM, Leonard Crestez wrote:
>> On 11.11.2019 05:18, Chanwoo Choi wrote:
>>> Hi Leonard,
>>>
>>> On 11/9/19 7:39 AM, Leonard Crestez wrote:
>>>> Add driver for dynamic scaling the DDR Controller on imx8m chips. Actual
>>>> frequency switching is implemented inside TF-A, this driver wraps the
>>>> SMC calls and synchronizes the clk tree.
>>>>
>>>> The DRAM clocks on imx8m have the following structure (abridged):
>>>>
>>>> +----------+ |\ +------+
>>>> | dram_pll |-------|M| dram_core | |
>>>> +----------+ |U|---------->| D |
>>>> /--|X| | D |
>>>> dram_alt_root | |/ | R |
>>>> | | C |
>>>> +---------+ | |
>>>> |FIX DIV/4| | |
>>>> +---------+ | |
>>>> composite: | | |
>>>> +----------+ | | |
>>>> | dram_alt |----/ | |
>>>> +----------+ | |
>>>> | dram_apb |-------------------->| |
>>>> +----------+ +------+
>>>>
>>>> The dram_pll is used for higher rates and dram_alt is used for lower
>>>> rates. The dram_alt and dram_apb clocks are "imx composite" and their
>>>> parent can also be modified.
>>>>
>>>> This driver will prepare/enable the new parents ahead of switching (so
>>>> that the expected roots are enabled) and afterwards it will call
>>>> clk_set_parent to ensure the parents in clock framework are up-to-date.
>>>>
>>>> The driver relies on dram_pll dram_alt and dram_apb being marked with
>>>> CLK_GET_RATE_NOCACHE for rate updates.
>>>>
>>>> Signed-off-by: Leonard Crestez <leonard.crestez@xxxxxxx>
>>>> ---
>>>> drivers/devfreq/Kconfig | 10 +
>>>> drivers/devfreq/Makefile | 1 +
>>>> drivers/devfreq/imx8m-ddrc.c | 416 +++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 427 insertions(+)
>>>> create mode 100644 drivers/devfreq/imx8m-ddrc.c
>>>>
>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>> index defe1d438710..c519fd27808f 100644
>>>> --- a/drivers/devfreq/Kconfig
>>>> +++ b/drivers/devfreq/Kconfig
>>>> @@ -90,10 +90,20 @@ config ARM_EXYNOS_BUS_DEVFREQ
>>>> Each memory bus group could contain many memoby bus block. It reads
>>>> PPMU counters of memory controllers by using DEVFREQ-event device
>>>> and adjusts the operating frequencies and voltages with OPP support.
>>>> This does not yet operate with optimal voltages.
>>>>
>>>> +config ARM_IMX8M_DDRC_DEVFREQ
>>>> + tristate "i.MX8M DDRC DEVFREQ Driver"
>>>> + depends on ARCH_MXC || COMPILE_TEST
>>>> + select DEVFREQ_GOV_SIMPLE_ONDEMAND
>>>> + select DEVFREQ_GOV_USERSPACE
>>>> + select PM_OPP
>>>
>>> It doesn't need to add. CONFIG_DEVFREQ add 'select PM_OPP'
>>> because 'PM_OPP' is mandatory for devfreq.
>>
>> OK.
>>
>> All other drivers select PM_OPP, this was just copied
>>
>>>> + help
>>>> + This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
>>>> + adjusting DRAM 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 338ae8440db6..3eb4d5e6635c 100644
>>>> --- a/drivers/devfreq/Makefile
>>>> +++ b/drivers/devfreq/Makefile
>>>> @@ -7,10 +7,11 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
>>>> 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_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/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
>>>> new file mode 100644
>>>> index 000000000000..51903fee21a7
>>>> --- /dev/null
>>>> +++ b/drivers/devfreq/imx8m-ddrc.c
>>>> @@ -0,0 +1,416 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright 2019 NXP
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/devfreq.h>
>>>> +#include <linux/pm_opp.h>
>>>> +#include <linux/clk.h>
>>>> +#include <linux/clk-provider.h>
>>>> +#include <linux/arm-smccc.h>
>>>> +
>>>> +#define IMX_SIP_DDR_DVFS 0xc2000004
>>>> +
>>>> +/* Values starting from 0 switch to specific frequency */
>>>> +#define IMX_SIP_DDR_FREQ_SET_HIGH 0x00
>>>> +
>>>> +/* Deprecated after moving IRQ handling to ATF */
>>>> +#define IMX_SIP_DDR_DVFS_WAIT_CHANGE 0x0F
>>>> +
>>>> +/* Query available frequencies. */
>>>> +#define IMX_SIP_DDR_DVFS_GET_FREQ_COUNT 0x10
>>>> +#define IMX_SIP_DDR_DVFS_GET_FREQ_INFO 0x11
>>>> +
>>>> +/*
>>>> + * This should be in a 1:1 mapping with devicetree OPPs but
>>>> + * firmware provides additional info.
>>>> + */
>>>> +struct imx8m_ddrc_freq {
>>>> + unsigned long rate;
>>>> + unsigned long smcarg;
>>>> + int dram_core_parent_index;
>>>> + int dram_alt_parent_index;
>>>> + int dram_apb_parent_index;
>>>> +};
>>>> +
>>>> +/* Hardware limitation */
>>>> +#define IMX8M_DDRC_MAX_FREQ_COUNT 4
>>>> +
>>>> +/*
>>>> + * i.MX8M DRAM Controller clocks have the following structure (abridged):
>>>> + *
>>>> + * +----------+ |\ +------+
>>>> + * | dram_pll |-------|M| dram_core | |
>>>> + * +----------+ |U|---------->| D |
>>>> + * /--|X| | D |
>>>> + * dram_alt_root | |/ | R |
>>>> + * | | C |
>>>> + * +---------+ | |
>>>> + * |FIX DIV/4| | |
>>>> + * +---------+ | |
>>>> + * composite: | | |
>>>> + * +----------+ | | |
>>>> + * | dram_alt |----/ | |
>>>> + * +----------+ | |
>>>> + * | dram_apb |-------------------->| |
>>>> + * +----------+ +------+
>>>> + *
>>>> + * The dram_pll is used for higher rates and dram_alt is used for lower rates.
>>>> + *
>>>> + * Frequency switching is implemented in TF-A (via SMC call) and can change the
>>>> + * configuration of the clocks, including mux parents. The dram_alt and
>>>> + * dram_apb clocks are "imx composite" and their parent can change too.
>>>> + *
>>>> + * We need to prepare/enable the new mux parents head of switching and update
>>>> + * their information afterwards.
>>>> + */
>>>> +struct imx8m_ddrc {
>>>> + struct devfreq_dev_profile profile;
>>>> + struct devfreq *devfreq;
>>>> +
>>>> + /* For frequency switching: */
>>>> + struct clk *dram_core;
>>>> + struct clk *dram_pll;
>>>> + struct clk *dram_alt;
>>>> + struct clk *dram_apb;
>>>> +
>>>> + int freq_count;
>>>> + struct imx8m_ddrc_freq freq_table[IMX8M_DDRC_MAX_FREQ_COUNT];
>>>> +};
>>>> +
>>>> +static struct imx8m_ddrc_freq *imx8m_ddrc_find_freq(struct imx8m_ddrc *priv,
>>>> + unsigned long rate)
>>>> +{
>>>> + struct imx8m_ddrc_freq *freq;
>>>> + int i;
>>>> +
>>>> + /*
>>>> + * Firmware reports values in MT/s, so we round-down from Hz
>>>> + * Rounding is extra generous to ensure a match.
>>>> + */
>>>> + rate = DIV_ROUND_CLOSEST(rate, 250000);
>>>> + for (i = 0; i < priv->freq_count; ++i) {
>>>> + freq = &priv->freq_table[i];
>>>> + if (freq->rate == rate ||
>>>> + freq->rate + 1 == rate ||
>>>> + freq->rate - 1 == rate)
>>>> + return freq;
>>>> + }
>>>> +
>>>> + return NULL;
>>>> +}
>>>> +
>>>> +static void imx8m_ddrc_smc_set_freq(int target_freq)
>>>> +{
>>>> + struct arm_smccc_res res;
>>>> + u32 online_cpus = 0;
>>>> + int cpu;
>>>> +
>>>> + local_irq_disable();
>>>
>>> local_irq_disable is more proper than local_irq_save()?
>>
>> There's no need to use save/restore because we know that irqs are
>> enabled when entering the function. This is only called from devfreq
>> set_target which runs in process context.
>
> local_irq might affect the whole architecture and other running device driver
> which requires the some CPU irq at the same time. Don't need to save the irqflag?
As far as I know IRQ disabling implicitly disables all forms of
preemption, no other code can run until irqs are restored.
The difference between disable/enable and save/restore is that the
latter also works when function is called with irqs already disabled but
that is not required here. We know that devfreq set_target is called in
process context with irqs enabled, otherwise it would be impossible to
use the OPP framework (it uses mutexes internally) or touch regulators
(they frequently sleep waiting on i2c transactions).
>>>> + for_each_online_cpu(cpu)
>>>> + online_cpus |= (1 << (cpu * 8));
>>>> +
>>>> + /* change the ddr freqency */
>>>> + arm_smccc_smc(IMX_SIP_DDR_DVFS, target_freq, online_cpus,
>>>> + 0, 0, 0, 0, 0, &res);
>>>> +
>>>> + local_irq_enable();
>>>
>>> ditto. local_irq_restore() instead of local_irq_enable()
>>>
>>>> +}
>>>> +
>>>> +struct clk *clk_get_parent_by_index(struct clk *clk, int index)
>>>> +{
>>>> + struct clk_hw *hw;
>>>> +
>>>> + hw = clk_hw_get_parent_by_index(__clk_get_hw(clk), index);
>>>> +
>>>> + return hw ? hw->clk : NULL;
>>>> +}
>>>> +
>>>> +static int imx8m_ddrc_set_freq(struct device *dev, struct imx8m_ddrc_freq *freq)
>>>> +{
>>>> + struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>>> + struct clk *new_dram_core_parent;
>>>> + struct clk *new_dram_alt_parent;
>>>> + struct clk *new_dram_apb_parent;
>>>> + int ret;
>>>> +
>>>> + new_dram_core_parent = clk_get_parent_by_index(
>>> > You can use 'clk_hw_get_parent_by_index' directly.
>>
>> The helper above avoids duplicating ?:
>
> I think that it is not duplicate. You use 'clk_hw_get_parent_by_index' function
> and check the return value. it is essential behavior.
The clk_hw_get_parent_by_index function returns a clk_hw and I'm using
the inner hw->clk. Without clk_get_parent_by_index I'd have to write:
hw = clk_hw_get_parent_by_index
if (!hw || !hw->clk) {
dev_err(...);
return
}
clk = hw->clk;
This seems pointlessly verbose.
>>> And, you need to check whether the return value is NULL or not.
>> Yes for dram_core_parent but others are deliberately allowed to be NULL.
>> For some setpoints some parents might be indifferent or require no
>> changes. For example when dram_core has pll as parent the expectation is
>> that dram_all will be OFF so parent is irrelevant.
>
> If all case is not same as you commented, you just add the proper dev_warn
> or dev_info message according to the h/w characteristic. Because user
> cannot know the detailed reason why don't check the return value.
Really it's only freq->dram_alt/apb_parent_index which can be zero, if
that has a valid value then clk fetching shouldn't fail for any other
reason. Current code could indeed hide some errors.
>> Driver relies on the fact that clock API ignores prepare/enable for NULL
>> clocks. I can add a comment explaining that.
>>
>>>> + priv->dram_core, freq->dram_core_parent_index - 1);
>>>> + new_dram_alt_parent = clk_get_parent_by_index(
>>>
>>> ditto.
>>>
>>>> + priv->dram_alt, freq->dram_alt_parent_index - 1);
>>>> + new_dram_apb_parent = clk_get_parent_by_index(
>>>
>>> ditto.
>>
>>>> + priv->dram_apb, freq->dram_apb_parent_index - 1);
>>>> +
>>>> + /* increase reference counts and ensure clks are ON before switch */
>>>> + ret = clk_prepare_enable(new_dram_core_parent);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed enable new dram_core parent: %d\n", ret);
>>>> + goto out;
>>>> + }
>>>> + ret = clk_prepare_enable(new_dram_alt_parent);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed enable new dram_alt parent: %d\n", ret);
>>>> + goto out_dis_core;
>>>
>>> I think that 'dis' is not general expression for 'disable'.
>>> Just, I think that 'out_disable_core' is better than 'out_dis_core'.
>>
>> OK
>>
>>>> + }
>>>> + ret = clk_prepare_enable(new_dram_apb_parent);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed enable new dram_apb parent: %d\n", ret);
>>>> + goto out_dis_alt;
>>>
>>> ditto.
>>>
>>>> + }
>>>> +
>>>> + imx8m_ddrc_smc_set_freq(freq->smcarg);
>>>> +
>>>> + /* update parents in clk tree after switch. */
>>>> + ret = clk_set_parent(priv->dram_core, new_dram_core_parent);
>>>> + if (ret)
>>>> + dev_err(dev, "failed set dram_core parent: %d\n", ret);
>>>
>>> if you don't return directly, you better to use 'dev_warn' instead of 'dev_err'.
>>
>> OK.
>>
>>>> + if (new_dram_alt_parent) {
>>>> + ret = clk_set_parent(priv->dram_alt, new_dram_alt_parent);
>>>> + if (ret)
>>>> + dev_err(dev, "failed set dram_alt parent: %d\n", ret);
>>>
>>> ditto. Use dev_warn.
>>>
>>>> + }
>>>> + if (new_dram_apb_parent) {
>>>> + ret = clk_set_parent(priv->dram_apb, new_dram_apb_parent);
>>>> + if (ret)
>>>> + dev_err(dev, "failed set dram_apb parent: %d\n", ret);
>>>
>>> ditto. Use dev_warn.
>>>
>>>> + }
>>>> +
>>>> + /*
>>>> + * Explicitly refresh dram PLL rate.
>>>> + *
>>>> + * Even if it's marked with CLK_GET_RATE_NOCACHE the rate will not be
>>>
>>> nitpick:
>>> What is more proper description either or 'Even if' or 'if' ?
>>
>> Rate updates work for dram_alt/apb but not for dram_pll because
>> additional clocks might be present between the PLL and dram_core mux.
>> This happens *even if* pll is marked with CLK_GET_RATE_NOCACHE.
>
> ok.
>
>>
>>>> + * automatically refreshed when clk_get_rate is called on children.
>>>> + */
>>>> + clk_get_rate(priv->dram_pll);
>>>> +
>>>> + /*
>>>> + * clk_set_parent transfer the reference count from old parent.
>>>> + * now we drop extra reference counts used during the switch
>>>> + */
>>>> + clk_disable_unprepare(new_dram_apb_parent);
>>>> +out_dis_alt:
>>>> + clk_disable_unprepare(new_dram_alt_parent);
>>>> +out_dis_core:
>>>> + clk_disable_unprepare(new_dram_core_parent);
>>>> +out:
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int imx8m_ddrc_target(struct device *dev, unsigned long *freq, u32 flags)
>>>> +{
>>>> + struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>>> + struct imx8m_ddrc_freq *freq_info;
>>>> + struct dev_pm_opp *new_opp;
>>>> + unsigned long old_freq, 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;
>>>> + }
>>>> + dev_pm_opp_put(new_opp);
>>>> +
>>>> + old_freq = clk_get_rate(priv->dram_core);
>>>> + if (*freq == old_freq)
>>>> + return 0;
>>>> +
>>>> + freq_info = imx8m_ddrc_find_freq(priv, *freq);
>>>> + if (!freq_info)
>>>> + return -EINVAL;
>>>> + ret = imx8m_ddrc_set_freq(dev, freq_info);
>>>
>>> Need to check whether 'ret' is valid or not.
>>>
>>>> +
>>>> + /* Also read back the clk rate to verify switch was correct */
>>>> + new_freq = clk_get_rate(priv->dram_core);
>>>> + if (ret || *freq != new_freq)
>>>
>>> You should check 'ret' right after imx8m_ddrc_set_freq()
>>> instead of this position.
>>
>> OK, I can add two error paths.
>>
>>>> + dev_err(dev, "ddrc failed freq set to %lu from %lu, now at %lu\n",
>>>> + old_freq, *freq, new_freq);
>>>> + else
>>>> + dev_dbg(dev, "ddrc freq set to %lu (was %lu)\n",
>>>> + *freq, old_freq);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int imx8m_ddrc_get_cur_freq(struct device *dev, unsigned long *freq)
>>>> +{
>>>> + struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>>> +
>>>> + *freq = clk_get_rate(priv->dram_core);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int imx8m_ddrc_init_freq_info(struct device *dev)
>>>> +{
>>>> + struct imx8m_ddrc *priv = dev_get_drvdata(dev);
>>>> + struct arm_smccc_res res;
>>>> + int index;
>>>> +
>>>> + /*
>>>> + * An error here means DDR DVFS API not supported by firmware
>>>> + */
>>>
>>> Don't need to add multiple line comments. It is possible to be change as following:
>>>
>>> /* An error here means DDR DVFS API not supported by firmware */
>>
>> OK.
>>>> + arm_smccc_smc(IMX_SIP_DDR_DVFS, IMX_SIP_DDR_DVFS_GET_FREQ_COUNT,
>>>> + 0, 0, 0, 0, 0, 0, &res);
>>>> + priv->freq_count = res.a0;
>>>> + if (priv->freq_count <= 0 ||
>>>> + priv->freq_count > IMX8M_DDRC_MAX_FREQ_COUNT)
>>>> + return -ENODEV;
>>>> +
>>>> + for (index = 0; index < priv->freq_count; ++index) {
>>>> + struct imx8m_ddrc_freq *freq = &priv->freq_table[index];
>>>> +
>>>> + arm_smccc_smc(IMX_SIP_DDR_DVFS, IMX_SIP_DDR_DVFS_GET_FREQ_INFO,
>>>> + index, 0, 0, 0, 0, 0, &res);
>>>> + /* Result should be strictly positive */
>>>> + if ((long)res.a0 <= 0)
>>>> + return -ENODEV;
>>>> +
>>>> + freq->rate = res.a0;
>>>> + freq->smcarg = index;
>>>> + freq->dram_core_parent_index = res.a1;
>>>> + freq->dram_alt_parent_index = res.a2;
>>>> + freq->dram_apb_parent_index = res.a3;
>>>> +
>>>> + /* dram_core has 2 options: dram_pll or dram_alt_root */
>>>> + if (freq->dram_core_parent_index != 1 &&
>>>> + freq->dram_core_parent_index != 2)
>>>> + return -ENODEV;
>>>> + /* dram_apb and dram_alt have exactly 8 possible parents */
>>>> + if (freq->dram_alt_parent_index > 8 ||
>>>> + freq->dram_apb_parent_index > 8)
>>>> + return -ENODEV;
>>>> + /* dram_core from alt requires explicit dram_alt parent */
>>>> + if (freq->dram_core_parent_index == 2 &&
>>>> + freq->dram_alt_parent_index == 0)
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/* imx8m_ddrc_check_opps() - disable OPPs not supported by firmware */
>>>
>>> nitpick:
>>> On this driver, there are no some description for internal function.
>>> In order to keep the coding style, you better to move this comment
>>> into function before 'freq = ULONG_MAX'.
>>
>> Is it a problem if only some internal functions have documentation? I
>> can add docs to more functions.
>
> It is not problem. It is just preferable way to add the comment.
> Usually, if the function is public and exported, I added the detailed
> function comment as following example of devfreq_add_device().
>
> /**
> 656 * devfreq_add_device() - Add devfreq feature to the device
> 657 * @dev: the device to add devfreq feature.
> 658 * @profile: device-specific profile to run devfreq.
> 659 * @governor_name: name of the policy to choose frequency.
> 660 * @data: private data for the governor. The devfreq framework does not
> 661 * touch this value.
> 662 */
> 663 struct devfreq *devfreq_add_device(struct device *dev,
> 664 struct devfreq_dev_profile *profile,
> 665 const char *governor_name,
> 666 void *data)
>
>
> Otherwise, if function is used in the only local with static keyword,
> usually I don't add the above detailed description. Instead,
> add the detailed comment into the function.
>
> It is not the standard way. Just preferable way for the function description
> if possible.
>
>
>
>>
>> BTW: how can I generate devfreq documentation from these comments?
>>
