On 2019-12-02 7:39 AM, Shawn Guo wrote: > On Thu, Oct 31, 2019 at 11:50:25PM +0200, 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> A more recent version of this patch is already in next: https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-next&id=518e99e2a22e318944d531a92aab5082fabb4d38 >> drivers/devfreq/Makefile | 1 + >> drivers/devfreq/imx-ddrc.c | 430 +++++++++++++++++++++++++++++++++++++ >> 2 files changed, 431 insertions(+) >> create mode 100644 drivers/devfreq/imx-ddrc.c >> +++ b/drivers/devfreq/imx-ddrc.c >> @@ -0,0 +1,430 @@ >> +// 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> > > This is a header that should ideally be used by clock drivers only. > >> +#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 > > These two defines are not used. Will be? No, can post a separate patch to remove them. > >> + >> +/* 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 imx_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 IMX_DDRC_MAX_FREQ_COUNT 4 >> + >> +/* >> + * imx DRAM controller >> + * >> + * imx 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 imx_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 imx_ddrc_freq freq_table[IMX_DDRC_MAX_FREQ_COUNT]; >> +}; >> + ... snip ... >> +static void imx_ddrc_smc_set_freq(int target_freq) >> +{ >> + struct arm_smccc_res res; >> + u32 online_cpus = 0; >> + int cpu; >> + >> + local_irq_disable(); >> + >> + for_each_online_cpu(cpu) >> + online_cpus |= (1 << (cpu * 8)); > > Nit: one level of unnecessary parentheses. Yes >> + >> + /* change the ddr freqency */ >> + arm_smccc_smc(IMX_SIP_DDR_DVFS, target_freq, online_cpus, >> + 0, 0, 0, 0, 0, &res); >> + >> + 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); > > Okay, this is why you need clk-provider.h. But this > clk_get_parent_by_index() function looks completely generic, and should > be proposed to clock core? There are very few driver users of clk_hw_get_parent_by_index: $ git grep -wl clk_hw_get_parent_by_index |grep -v drivers/clk arch/mips/alchemy/common/clock.c drivers/cpufreq/qoriq-cpufreq.c drivers/devfreq/imx8m-ddrc.c drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c drivers/media/platform/atmel/atmel-isc-base.c drivers/rtc/rtc-ac100.c include/linux/clk-provider.h Even clk_get_parent has few users and it contains this strange comment: /* TODO: Create a per-user clk and change callers to call clk_put */ That proposed change effectively creates a new API? I didn't want to add a new clk core API with unclear semantics. -- Regards, Leonard
