| Message ID | cover.1573761527.git.leonard.crestez@nxp.com |
|---|---|
| Headers | show |
| Series | interconnect: Add imx support via devfreq | expand |
Hi Leonard, On 2019-11-14 12:09, Leonard Crestez wrote: > 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. > Is this working with mainline ATF or does it still need to be used with your modified ATF code ? Thanks Angus > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > 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) > +{ > + 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) > +{ > + 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; > + const char *gov = DEVFREQ_GOV_USERSPACE; > + void *govdata = NULL; > + int ret; > + > + 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; > + } > + 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; > + } > + > + 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"); > +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>"); > +MODULE_LICENSE("GPL v2");
On 20.11.2019 16:08, Angus Ainslie wrote: > Hi Leonard, > > On 2019-11-14 12:09, Leonard Crestez wrote: >> 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. >> > > Is this working with mainline ATF or does it still need to be used with > your modified ATF code ? This series doesn't perform SMC calls, that's done by the imx8m-ddrc driver: https://patchwork.kernel.org/cover/11244283/ This particular patch allows switching NOC frequency but that's just clk_set_rate. DDRC frequency switching requires the imx branch of ATF (v2.0 + ~200 patches) otherwise you will get probe failures. Source for imx atf is published here: https://source.codeaurora.org/external/imx/imx-atf/ For your particular 8mq B0 case slightly different setpoints are used and the fix is not in any public release yet so you need this: https://github.com/cdleonard/arm-trusted-firmware/commits/imx_2.0.y_busfreq Is "mainline ATF" an important criteria for Purism? -- Regards, Leonard
Hi Leonard, On 19-11-20 15:04, Leonard Crestez wrote: > On 20.11.2019 16:08, Angus Ainslie wrote: > > Hi Leonard, > > > > On 2019-11-14 12:09, Leonard Crestez wrote: > >> 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. > >> > > > > Is this working with mainline ATF or does it still need to be used with > > your modified ATF code ? > > This series doesn't perform SMC calls, that's done by the imx8m-ddrc > driver: https://patchwork.kernel.org/cover/11244283/ > > This particular patch allows switching NOC frequency but that's just > clk_set_rate. > > DDRC frequency switching requires the imx branch of ATF (v2.0 + ~200 > patches) otherwise you will get probe failures. Source for imx atf is > published here: https://source.codeaurora.org/external/imx/imx-atf/ > > For your particular 8mq B0 case slightly different setpoints are used > and the fix is not in any public release yet so you need this: > > https://github.com/cdleonard/arm-trusted-firmware/commits/imx_2.0.y_busfreq > > Is "mainline ATF" an important criteria for Purism? Sorry for jumping in here. Just asking myself if the nxp-atf is required for a mainline kernel for the imx8mq devices? Thanks, Marco > -- > Regards, > Leonard > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi Leonard, On 2019-11-20 07:04, Leonard Crestez wrote: > On 20.11.2019 16:08, Angus Ainslie wrote: >> Hi Leonard, >> >> On 2019-11-14 12:09, Leonard Crestez wrote: >>> 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. >>> >> >> Is this working with mainline ATF or does it still need to be used >> with >> your modified ATF code ? > > This series doesn't perform SMC calls, that's done by the imx8m-ddrc > driver: https://patchwork.kernel.org/cover/11244283/ > > This particular patch allows switching NOC frequency but that's just > clk_set_rate. > > DDRC frequency switching requires the imx branch of ATF (v2.0 + ~200 > patches) otherwise you will get probe failures. Source for imx atf is > published here: https://source.codeaurora.org/external/imx/imx-atf/ Ok I was under the impression that the imx_2.0.y_busfreq branch below was based on this. Shouldn't those patches be added to the imx ATF ? > > For your particular 8mq B0 case slightly different setpoints are used > and the fix is not in any public release yet so you need this: > > https://github.com/cdleonard/arm-trusted-firmware/commits/imx_2.0.y_busfreq > We also have 2n14w ( is that B1 ? ) imx8mq's that we are working with. > Is "mainline ATF" an important criteria for Purism? > Yes we intend to bring all of our patches to mainline and were hoping that NXP would be doing the same. Shouldn't a mainline kernel run on a mainline ATF ? Thanks Angus > -- > Regards, > Leonard
On 20.11.2019 17:41, Angus Ainslie wrote: > Hi Leonard, > > On 2019-11-20 07:04, Leonard Crestez wrote: >> On 20.11.2019 16:08, Angus Ainslie wrote: >>> Hi Leonard, >>> >>> On 2019-11-14 12:09, Leonard Crestez wrote: >>>> 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. >>>> >>> >>> Is this working with mainline ATF or does it still need to be used >>> with your modified ATF code ? >> >> This series doesn't perform SMC calls, that's done by the imx8m-ddrc >> driver: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11244283%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C186d3c14d8bc41216e3b08d76dd0106d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637098612732915017&sdata=ER10Ts4hk2Ft7%2FWBZ7r8lyFkB6un1VRwk0rSvRMm3ew%3D&reserved=0 >> >> This particular patch allows switching NOC frequency but that's just >> clk_set_rate. >> >> DDRC frequency switching requires the imx branch of ATF (v2.0 + ~200 >> patches) otherwise you will get probe failures. Source for imx atf is >> published here: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Fimx-atf%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C186d3c14d8bc41216e3b08d76dd0106d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637098612732915017&sdata=KcdzTQaW4xxualeRUU%2B9LjBeq99wUtzDBxrHWLVbkDo%3D&reserved=0 > > Ok I was under the impression that the imx_2.0.y_busfreq branch below > was based on this. Shouldn't those patches be added to the imx ATF ? Already done, it's just that CAF public releases are only made after internal testing. TF-A is open-source so I push patches to my personal github to help more adventurous developers. >> For your particular 8mq B0 case slightly different setpoints are used >> and the fix is not in any public release yet so you need this: >> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcdleonard%2Farm-trusted-firmware%2Fcommits%2Fimx_2.0.y_busfreq&data=02%7C01%7Cleonard.crestez%40nxp.com%7C186d3c14d8bc41216e3b08d76dd0106d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637098612732925012&sdata=Ape5T7xfiR0yfSuO1Lv9OQhmK5p3f0ROWpzJiAMw1VA%3D&reserved=0 >> > > We also have 2n14w ( is that B1 ? ) imx8mq's that we are working with. Errata is e11327 and does not appear on 2N14W: https://www.nxp.com/docs/en/errata/IMX8MDQLQ_1N14W.pdf https://www.nxp.com/docs/en/errata/IMX8MDQLQ_2N14W.pdf You should be able to test with a published release: https://source.codeaurora.org/external/imx/imx-atf/log/?h=imx_4.19.35_1.1.0 >> Is "mainline ATF" an important criteria for Purism? > > Yes we intend to bring all of our patches to mainline and were hoping > that NXP would be doing the same. Shouldn't a mainline kernel run on a > mainline ATF ? You can still use mainline ATF (tested right now) but the imx8m-ddrc driver won't probe. The ability to mix and match different branches of firmware and kernel is very useful for testing. There might be slight incompatibilities but in theory if a feature depends on both firmware and kernel support then it should gracefully degrade rather than crash or hang. ATF support for this feature will be mainlined eventually, I picked the linux side first because review is more challenging and changes are much larger relative to what we have in our internal tree. -- Regards, Leonard
On 2019-11-20 08:30, Leonard Crestez wrote: > On 20.11.2019 17:41, Angus Ainslie wrote: >> Hi Leonard, >> >> On 2019-11-20 07:04, Leonard Crestez wrote: >>> On 20.11.2019 16:08, Angus Ainslie wrote: >>> Is "mainline ATF" an important criteria for Purism? >> >> Yes we intend to bring all of our patches to mainline and were hoping >> that NXP would be doing the same. Shouldn't a mainline kernel run on a >> mainline ATF ? > > You can still use mainline ATF (tested right now) but the imx8m-ddrc > driver won't probe. > Sorry I was talking about the DDR frequency scaling specifically. > The ability to mix and match different branches of firmware and kernel > is very useful for testing. There might be slight incompatibilities but > in theory if a feature depends on both firmware and kernel support then > it should gracefully degrade rather than crash or hang. I saw the check you put in for the correct ATF version and that's very helpful thanks. > > ATF support for this feature will be mainlined eventually, I picked the > linux side first because review is more challenging and changes are > much > larger relative to what we have in our internal tree. > Do you have a patch against mainline ATF that we can test this feature with ? Thanks Angus > -- > Regards, > Leonard
On 20.11.2019 18:38, Angus Ainslie wrote: > On 2019-11-20 08:30, Leonard Crestez wrote: >> On 20.11.2019 17:41, Angus Ainslie wrote: >>> Hi Leonard, >>> >>> On 2019-11-20 07:04, Leonard Crestez wrote: >>>> On 20.11.2019 16:08, Angus Ainslie wrote: >>>> Is "mainline ATF" an important criteria for Purism? >>> >>> Yes we intend to bring all of our patches to mainline and were hoping >>> that NXP would be doing the same. Shouldn't a mainline kernel run on a >>> mainline ATF ? >> >> You can still use mainline ATF (tested right now) but the imx8m-ddrc >> driver won't probe. > > Sorry I was talking about the DDR frequency scaling specifically. > >> The ability to mix and match different branches of firmware and kernel >> is very useful for testing. There might be slight incompatibilities but >> in theory if a feature depends on both firmware and kernel support then >> it should gracefully degrade rather than crash or hang. > > I saw the check you put in for the correct ATF version and that's very > helpful thanks. > >> ATF support for this feature will be mainlined eventually, I picked the >> linux side first because review is more challenging and changes are >> much larger relative to what we have in our internal tree. > > Do you have a patch against mainline ATF that we can test this feature > with ? Not right now, and imx atf is based on a slightly older version so some porting effort might be required. -- Regards, Leonard
On 14.11.2019 22:10, Leonard Crestez wrote: > This series adds interconnect scaling support for imx8m series chips. It uses a > per-SOC interconnect provider layered on top of multiple instances of devfreq > for scalable nodes along the interconnect. > > Existing qcom interconnect providers mostly translate bandwidth requests into > firmware calls but equivalent firmware on imx8m is much thinner. Scaling > support for individual nodes is implemented as distinct devfreq drivers > instead. > > The imx interconnect provider doesn't communicate with devfreq directly > but rather computes "minimum frequencies" for nodes along the path and > creates dev_pm_qos requests. > > Since there is no single devicetree node that can represent the > "interconnect" the main NOC is picked as the "interconnect provider" and > will probe the interconnect platform device if #interconnect-cells is > present. This avoids introducing "virtual" devices but it means that DT > bindings of main NOC includes properties for both devfreq and > interconnect. > > This depends on other series for devfreq pm/qos and ddrc: It took a while but all runtime dependencies are included in next-20191210 and the compile-time dependency on DEV_PM_QOS_MIN_FREQUENCY is also included in v5.5-rc1. This series is a bit old but still applies usefully so I thought I'd ask for additional comments instead of a blank resend. Georgi: can you please take a look at this series and suggest what would make it suitable for inclusion? Some dubious aspects: * Existing drivers have multiple providers per SOC (for each internal NOC?) but this is a single driver for the entire SOC topology. Not clear what would be gain from this. * NOC DT node implements devfreq (because it can be scaled) and interconnect (to avoid virtual DT nodes). Maybe the devfreq NOC scaling implementation could be moved into drivers/interconnect/imx? It is quite different from qcom providers but hardware/firmware is also very different. > Changes since RFCv5: > * Replace scanning for interconnect-node-id with explicit > scalable-nodes/scalable-node-ids property on NoC. > * Now passes make `dtbs_check` > * Remove struct imx_icc_provider > * Switch to of_icc_xlate_onecell > * Use of_find_device_by_node to fetch QoS target, this causes fewer probe > deferrals, removes dependency on devfreq API and even allows reloading ddrc > module at runtime > * Add imx_icc_node_destroy helper > * Remove 0/1 on DEFINE_BUS_SLAVE/MASTER which created spurious links > Link: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11222015%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Cff0127cbde3e4e6f6c8508d7693eb8d5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637093590418229667&sdata=0EN1w8RtXHgyvXpRnOovEkWILoVlQ%2FEXJ0zdOOHI%2FgM%3D&reserved=0 > > Changes since RFCv4: > * Drop icc proxy nonsense > * Make devfreq driver for NOC probe the ICC driver if > #interconnect-cells is present > * Move NOC support to interconnect series and rename the node in DT > * Add support for all chips at once, differences are not intereseting > and there is more community interest for 8mq than 8mm. > Link: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11111865%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Cff0127cbde3e4e6f6c8508d7693eb8d5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637093590418229667&sdata=bsJqXPVJSszKJf%2BXdqFT2E8wriuKQU00sWwkRqZYmn0%3D&reserved=0 > > Changes since RFCv3: > * Remove the virtual "icc" node and add devfreq nodes as proxy providers > * Fix build on 32-bit arm (reported by kbuilt test robot) > * Remove ARCH_MXC_ARM64 (never existed in upstream) > * Remove _numlinks, calculate instead > * Replace __BUSFREQ_H header guard > * Improve commit message and comment spelling > * Fix checkpatch issues > Link to RFCv3: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11078671%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Cff0127cbde3e4e6f6c8508d7693eb8d5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637093590418229667&sdata=c9xKEw9Fjlv%2FqDiWKr7zOUY5zZMWXjNyApBd94Nidnc%3D&reserved=0 > > Changes since RFCv2 and initial work by Alexandre Bailon: > * Relying on devfreq and dev_pm_qos instead of CLK > * No more "platform opp" stuff > * No more special suspend handling: use suspend-opp on devfreq instead > * Replace all mentions of "busfreq" with "interconnect" > Link to v2: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11021563%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7Cff0127cbde3e4e6f6c8508d7693eb8d5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637093590418229667&sdata=1tEQS9BaHRTjdUYvZJ9%2FJG2BQQGl6hkbPWxiIZ811FY%3D&reserved=0 > > Leonard Crestez (9): > dt-bindings: interconnect: Add bindings for imx8m noc > PM / devfreq: Add generic imx bus scaling driver > PM / devfreq: imx: Register interconnect device > interconnect: Add imx core driver > interconnect: imx: Add platform driver for imx8mm > interconnect: imx: Add platform driver for imx8mq > interconnect: imx: Add platform driver for imx8mn > arm64: dts: imx8m: Add NOC nodes > arm64: dts: imx8m: Add interconnect provider properties > > .../bindings/interconnect/fsl,imx8m-noc.yaml | 104 ++++++ > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 28 ++ > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 28 ++ > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 28 ++ > drivers/devfreq/Kconfig | 9 + > drivers/devfreq/Makefile | 1 + > drivers/devfreq/imx-devfreq.c | 187 +++++++++++ > drivers/interconnect/Kconfig | 1 + > drivers/interconnect/Makefile | 1 + > drivers/interconnect/imx/Kconfig | 17 + > drivers/interconnect/imx/Makefile | 4 + > drivers/interconnect/imx/imx.c | 301 ++++++++++++++++++ > drivers/interconnect/imx/imx.h | 60 ++++ > drivers/interconnect/imx/imx8mm.c | 105 ++++++ > drivers/interconnect/imx/imx8mn.c | 94 ++++++ > drivers/interconnect/imx/imx8mq.c | 103 ++++++ > include/dt-bindings/interconnect/imx8mm.h | 49 +++ > include/dt-bindings/interconnect/imx8mn.h | 41 +++ > include/dt-bindings/interconnect/imx8mq.h | 48 +++ > 19 files changed, 1209 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml > create mode 100644 drivers/devfreq/imx-devfreq.c > create mode 100644 drivers/interconnect/imx/Kconfig > create mode 100644 drivers/interconnect/imx/Makefile > create mode 100644 drivers/interconnect/imx/imx.c > create mode 100644 drivers/interconnect/imx/imx.h > create mode 100644 drivers/interconnect/imx/imx8mm.c > create mode 100644 drivers/interconnect/imx/imx8mn.c > create mode 100644 drivers/interconnect/imx/imx8mq.c > create mode 100644 include/dt-bindings/interconnect/imx8mm.h > create mode 100644 include/dt-bindings/interconnect/imx8mn.h > create mode 100644 include/dt-bindings/interconnect/imx8mq.h >
Hi Leonard, Thank you for your continuous work on the patches and sorry for not reviewing this earlier. On 11/14/19 22:09, Leonard Crestez wrote: > This adds support for i.MX SoC family to interconnect framework. > > Platform drivers can describe the interconnect graph and several > adjustment knobs where icc node bandwidth is converted to a > DEV_PM_QOS_MIN_FREQUENCY request. > > The interconnect provider is probed through the main NOC device and > other adjustable nodes on the same graph are found from a > fsl,scalable-nodes phandle array property. > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/interconnect/Kconfig | 1 + > drivers/interconnect/Makefile | 1 + > drivers/interconnect/imx/Kconfig | 5 + > drivers/interconnect/imx/Makefile | 1 + > drivers/interconnect/imx/imx.c | 301 ++++++++++++++++++++++++++++++ > drivers/interconnect/imx/imx.h | 60 ++++++ > 6 files changed, 369 insertions(+) > create mode 100644 drivers/interconnect/imx/Kconfig > create mode 100644 drivers/interconnect/imx/Makefile > create mode 100644 drivers/interconnect/imx/imx.c > create mode 100644 drivers/interconnect/imx/imx.h > > diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig > index bfa4ca3ab7a9..e61802230f90 100644 > --- a/drivers/interconnect/Kconfig > +++ b/drivers/interconnect/Kconfig > @@ -10,7 +10,8 @@ menuconfig INTERCONNECT > If unsure, say no. > > if INTERCONNECT > > source "drivers/interconnect/qcom/Kconfig" > +source "drivers/interconnect/imx/Kconfig" > > endif > diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile > index 28f2ab0824d5..20a13b7eb37f 100644 > --- a/drivers/interconnect/Makefile > +++ b/drivers/interconnect/Makefile > @@ -2,5 +2,6 @@ > > icc-core-objs := core.o > > obj-$(CONFIG_INTERCONNECT) += icc-core.o > obj-$(CONFIG_INTERCONNECT_QCOM) += qcom/ > +obj-$(CONFIG_INTERCONNECT_IMX) += imx/ > diff --git a/drivers/interconnect/imx/Kconfig b/drivers/interconnect/imx/Kconfig > new file mode 100644 > index 000000000000..7d81d3c83a61 > --- /dev/null > +++ b/drivers/interconnect/imx/Kconfig > @@ -0,0 +1,5 @@ > +config INTERCONNECT_IMX > + bool "i.MX interconnect drivers" > + depends on ARCH_MXC || COMPILE_TEST > + help > + Generic interconnect driver for i.MX SOCs > diff --git a/drivers/interconnect/imx/Makefile b/drivers/interconnect/imx/Makefile > new file mode 100644 > index 000000000000..bb92fd9fe4a5 > --- /dev/null > +++ b/drivers/interconnect/imx/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_INTERCONNECT_IMX) += imx.o > diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c > new file mode 100644 > index 000000000000..d08e64c239cd > --- /dev/null > +++ b/drivers/interconnect/imx/imx.c > @@ -0,0 +1,301 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Interconnect framework driver for i.MX SoC > + * > + * Copyright (c) 2019, BayLibre > + * Copyright (c) 2019, NXP > + * Author: Alexandre Bailon <abailon@baylibre.com> > + * Author: Leonard Crestez <leonard.crestez@nxp.com> > + */ > + > +#include <linux/device.h> > +#include <linux/interconnect-provider.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/pm_qos.h> > + > +#include "imx.h" > + > +/* private icc_node data */ > +struct imx_icc_node { > + const struct imx_icc_node_desc *desc; > + struct device *qos_dev; > + struct dev_pm_qos_request qos_req; > +}; > + > +static int imx_icc_aggregate(struct icc_node *node, u32 tag, > + u32 avg_bw, u32 peak_bw, > + u32 *agg_avg, u32 *agg_peak) > +{ > + *agg_avg += avg_bw; > + *agg_peak = max(*agg_peak, peak_bw); > + > + return 0; > +} > + > +static int imx_icc_node_set(struct icc_node *node) > +{ > + struct device *dev = node->provider->dev; > + struct imx_icc_node *node_data = node->data; > + u64 freq; > + > + if (!node_data->qos_dev) > + return 0; > + > + freq = (node->avg_bw + node->peak_bw) * node_data->desc->adj->bw_mul; > + do_div(freq, node_data->desc->adj->bw_div); > + dev_dbg(dev, "node %s device %s avg_bw %ukBps peak_bw %ukBps min_freq %llukHz\n", > + node->name, dev_name(node_data->qos_dev), > + node->avg_bw, node->peak_bw, freq); Nit: Please align all dev_dbg/dev_err to the open parenthesis. > + > + if (freq > S32_MAX) { > + dev_err(dev, "%s can't request more than S32_MAX freq\n", > + node->name); > + return -ERANGE; > + } > + > + dev_pm_qos_update_request(&node_data->qos_req, freq); > + > + return 0; > +} > + > +static int imx_icc_set(struct icc_node *src, struct icc_node *dst) > +{ > + return imx_icc_node_set(dst); > +} > + > +/* imx_icc_node_destroy() - Destroy an imx icc_node, including private data */ > +static void imx_icc_node_destroy(struct icc_node *node) > +{ > + struct imx_icc_node *node_data = node->data; > + > + if (dev_pm_qos_request_active(&node_data->qos_req)) > + dev_pm_qos_remove_request(&node_data->qos_req); > + put_device(node_data->qos_dev); > + icc_node_del(node); > + icc_node_destroy(node->id); > +} > + > +static int imx_icc_node_init_qos(struct icc_provider *provider, > + struct icc_node *node) > +{ > + struct imx_icc_node *node_data = node->data; > + struct device *dev = provider->dev; > + struct device_node *dn = NULL; > + struct platform_device *pdev; > + int i, count; > + u32 node_id; > + int ret; > + > + count = of_property_count_u32_elems(dev->of_node, > + "fsl,scalable-node-ids"); > + if (count < 0) { > + dev_err(dev, "Failed to parse fsl,scalable-node-ids: %d\n", > + count); > + return count; > + } > + > + for (i = 0; i < count; i++) { > + ret = of_property_read_u32_index(dev->of_node, > + "fsl,scalable-node-ids", > + i, &node_id); > + > + if (ret < 0) { > + dev_err(dev, "Failed to parse fsl,scalable-node-ids[%d]: %d\n", > + i, ret); > + return ret; > + } > + if (node_id != node->id) > + continue; > + > + dn = of_parse_phandle(dev->of_node, "fsl,scalable-nodes", i); Why is this needed? I would expect that the interconnect provider driver already knows which nodes are scalable based on the platform compatible string. Then maybe this driver should create devfreq devices for each node that is scalable? > + if (IS_ERR(dn)) { > + dev_err(dev, "Failed to parse fsl,scalable-nodes[%d]: %ld\n", > + i, PTR_ERR(dn)); > + return PTR_ERR(dn); > + } > + break; > + } > + > + /* Allow scaling to be disabled on a per-node basis */ > + if (!dn || !of_device_is_available(dn)) > + return 0; > + > + pdev = of_find_device_by_node(dn); > + of_node_put(dn); > + if (!pdev) { > + dev_warn(dev, "node %s[%d] missing device for %pOF\n", > + node->name, node->id, dn); > + return -EPROBE_DEFER; > + } > + > + node_data->qos_dev = &pdev->dev; > + dev_info(dev, "node %s[%d] has device node %pOF\n", > + node->name, node->id, dn); > + return dev_pm_qos_add_request(node_data->qos_dev, > + &node_data->qos_req, > + DEV_PM_QOS_MIN_FREQUENCY, 0); > +} > + > +static struct icc_node *imx_icc_node_add( > + struct icc_provider *provider, Nit: Move this after the open parenthesis on the line above and then align the line below to the parenthesis and ignore the 80 cols rule. > + const struct imx_icc_node_desc *node_desc) > +{ > + struct device *dev = provider->dev; > + struct imx_icc_node *node_data; > + struct icc_node *node; > + int ret; > + > + node = icc_node_create(node_desc->id); > + if (IS_ERR(node)) { > + dev_err(dev, "failed to create node %d\n", node_desc->id); > + return node; > + } > + > + if (node->data) { > + dev_err(dev, "already created node %s id=%d\n", > + node_desc->name, node_desc->id); > + return ERR_PTR(-EEXIST); > + } > + > + node_data = devm_kzalloc(dev, sizeof(*node_data), GFP_KERNEL); > + if (!node_data) { > + icc_node_destroy(node->id); > + return ERR_PTR(-ENOMEM); > + } > + > + node->name = node_desc->name; > + node->data = node_data; > + node_data->desc = node_desc; > + icc_node_add(node, provider); > + > + if (node_desc->adj) { > + ret = imx_icc_node_init_qos(provider, node); > + if (ret < 0) { > + imx_icc_node_destroy(node); > + return ERR_PTR(ret); > + } > + } > + > + return node; > +} > + > +static void imx_icc_unregister_nodes(struct icc_provider *provider) > +{ > + struct icc_node *node, *tmp; > + > + list_for_each_entry_safe(node, tmp, &provider->nodes, node_list) > + imx_icc_node_destroy(node); > +} > + > +static int imx_icc_register_nodes(struct icc_provider *provider, > + const struct imx_icc_node_desc *descs, > + int count) > +{ > + struct icc_onecell_data *provider_data = provider->data; > + int ret; > + int i; > + > + for (i = 0; i < count; i++) { > + struct icc_node *node; > + const struct imx_icc_node_desc *node_desc = &descs[i]; > + size_t j; > + > + node = imx_icc_node_add(provider, node_desc); > + if (IS_ERR(node)) { > + ret = PTR_ERR(node); > + if (ret != -EPROBE_DEFER) > + dev_err(provider->dev, "failed to add %s: %d\n", > + node_desc->name, ret); > + goto err; > + } > + provider_data->nodes[node->id] = node; > + > + for (j = 0; j < node_desc->num_links; j++) { > + ret = icc_link_create(node, node_desc->links[j]); > + if (ret) { > + dev_err(provider->dev, "failed to link node %d to %d: %d\n", > + node->id, node_desc->links[j], ret); > + goto err; > + } > + } > + } > + > + return 0; > + > +err: > + imx_icc_unregister_nodes(provider); > + > + return ret; > +} > + > +static int get_max_node_id(struct imx_icc_node_desc *nodes, int nodes_count) > +{ > + int i, ret = 0; > + > + for (i = 0; i < nodes_count; ++i) > + if (nodes[i].id > ret) > + ret = nodes[i].id; > + > + return ret; > +} > + > +int imx_icc_register(struct platform_device *pdev, > + struct imx_icc_node_desc *nodes, int nodes_count) > +{ > + struct device *dev = &pdev->dev; > + struct icc_onecell_data *data; > + struct icc_provider *provider; > + int max_node_id; > + int ret; > + > + /* icc_onecell_data is indexed by node_id, unlike nodes param */ > + max_node_id = get_max_node_id(nodes, nodes_count); > + data = devm_kzalloc(dev, struct_size(data, nodes, max_node_id), > + GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + data->num_nodes = max_node_id; > + > + provider = devm_kzalloc(dev, sizeof(*provider), GFP_KERNEL); > + if (!provider) > + return -ENOMEM; > + provider->set = imx_icc_set; > + provider->aggregate = imx_icc_aggregate; > + provider->xlate = of_icc_xlate_onecell; > + provider->data = data; > + provider->dev = dev->parent; > + platform_set_drvdata(pdev, provider); > + > + ret = icc_provider_add(provider); > + if (ret) { > + dev_err(dev, "error adding interconnect provider: %d\n", ret); > + return ret; > + } > + > + ret = imx_icc_register_nodes(provider, nodes, nodes_count); > + if (ret) > + goto provider_del; > + > + pr_info("registered %s\n", pdev->name); > + > + return 0; > + > +provider_del: > + icc_provider_del(provider); > + return ret; > +} > +EXPORT_SYMBOL_GPL(imx_icc_register); > + > +int imx_icc_unregister(struct platform_device *pdev) > +{ > + struct icc_provider *provider = platform_get_drvdata(pdev); > + > + icc_provider_del(provider); > + imx_icc_unregister_nodes(provider); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(imx_icc_unregister); > diff --git a/drivers/interconnect/imx/imx.h b/drivers/interconnect/imx/imx.h > new file mode 100644 > index 000000000000..9f06c54ac2c8 > --- /dev/null > +++ b/drivers/interconnect/imx/imx.h > @@ -0,0 +1,60 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Interconnect framework driver for i.MX SoC > + * > + * Copyright (c) 2019, BayLibre > + * Copyright (c) 2019, NXP > + * Author: Alexandre Bailon <abailon@baylibre.com> > + * Author: Leonard Crestez <leonard.crestez@nxp.com> > + */ > +#ifndef __DRIVERS_INTERCONNECT_IMX_H > +#define __DRIVERS_INTERCONNECT_IMX_H > + > +#include <linux/kernel.h> > + > +#define IMX_ICC_MAX_LINKS 4 > + > +/* > + * struct imx_icc_node_adj - Describe a dynamic adjustment knob > + */ > +struct imx_icc_node_adj_desc { > + unsigned int bw_mul, bw_div; > +}; > + > +/* > + * struct imx_icc_node - Describe an interconnect node > + * @name: name of the node > + * @id: an unique id to identify the node > + * @links: an array of slaves' node id > + * @num_links: number of id defined in links > + */ > +struct imx_icc_node_desc { > + const char *name; > + u16 id; > + u16 links[IMX_ICC_MAX_LINKS]; > + u16 num_links; > + > + const struct imx_icc_node_adj_desc *adj; > +}; > + > +#define DEFINE_BUS_INTERCONNECT(_name, _id, _adj, ...) \ > + { \ > + .id = _id, \ > + .name = _name, \ > + .adj = _adj, \ > + .num_links = ARRAY_SIZE(((int[]){ __VA_ARGS__ })), \ > + .links = { __VA_ARGS__ }, \ > + } > + > +#define DEFINE_BUS_MASTER(_name, _id, _dest_id) \ > + DEFINE_BUS_INTERCONNECT(_name, _id, NULL, _dest_id) > + > +#define DEFINE_BUS_SLAVE(_name, _id, _adj) \ > + DEFINE_BUS_INTERCONNECT(_name, _id, _adj) > + > +int imx_icc_register(struct platform_device *pdev, > + struct imx_icc_node_desc *nodes, > + int nodes_count); > +int imx_icc_unregister(struct platform_device *pdev); > + > +#endif /* __DRIVERS_INTERCONNECT_IMX_H */ > The rest look ok to me. We just need to figure out the scalable nodes thing. Thanks, Georgi
Hi, On 11/15/19 5:09 AM, Leonard Crestez wrote: > 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 <leonard.crestez@nxp.com> > --- > 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. > +{ > + 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. > +{ > + 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. > + const char *gov = DEVFREQ_GOV_USERSPACE; > + void *govdata = NULL; How about changing the variable name 'govdata' to 'gov_data'? - govdata -> gov_data > + 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. > + 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; } > + > + 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. > +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>"); > +MODULE_LICENSE("GPL v2"); >
On 12/13/19 10:30 AM, Chanwoo Choi wrote: > Hi, > > On 11/15/19 5:09 AM, Leonard Crestez wrote: >> 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 <leonard.crestez@nxp.com> >> --- >> 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. > >> +{ >> + 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. > >> +{ >> + 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. > >> + const char *gov = DEVFREQ_GOV_USERSPACE; >> + void *govdata = NULL; > > How about changing the variable name 'govdata' to 'gov_data'? > - govdata -> gov_data > >> + 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. And, this driver doesn't include the 'clk_prepare_enable'. how to enable the clock? > >> + 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; > } > >> + >> + 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. > >> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>"); >> +MODULE_LICENSE("GPL v2"); >> > >
Hi, On 11/15/19 5:09 AM, Leonard Crestez wrote: > There is no single device which can represent the imx interconnect. > Instead of adding a virtual one just make the main &noc act as the > global interconnect provider. > > The imx interconnect provider driver will scale the NOC and DDRC based > on bandwidth request. More scalable nodes can be added in the future, > for example for audio/display/vpu/gpu NICs. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/devfreq/imx-devfreq.c | 37 +++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c > index 620b344e87aa..585d340c0f6e 100644 > --- a/drivers/devfreq/imx-devfreq.c > +++ b/drivers/devfreq/imx-devfreq.c > @@ -15,10 +15,11 @@ > struct imx_devfreq { > struct devfreq_dev_profile profile; > struct devfreq *devfreq; > struct clk *clk; > struct devfreq_passive_data passive_data; > + struct platform_device *icc_pdev; > }; > > static int imx_devfreq_target(struct device *dev, > unsigned long *freq, u32 flags) > { > @@ -60,11 +61,40 @@ static int imx_devfreq_get_dev_status(struct device *dev, > return 0; > } > > static void imx_devfreq_exit(struct device *dev) > { > + struct imx_devfreq *priv = dev_get_drvdata(dev); > + > dev_pm_opp_of_remove_table(dev); > + platform_device_unregister(priv->icc_pdev); > +} > + > +/* imx_devfreq_init_icc() - register matching icc provider if required */ > +static int imx_devfreq_init_icc(struct device *dev) > +{ > + struct imx_devfreq *priv = dev_get_drvdata(dev); > + const char *icc_driver_name; > + > + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) > + return 0; It is not proper to check the enable state of CONFIG_INTERCONNECT_IMX configuration on device driver. Why don't you add the 'select CONFIG_INTERCONNECT_IMX' on Kconfig? > + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) > + return 0; > + > + icc_driver_name = of_device_get_match_data(dev); > + if (!icc_driver_name) > + return 0; > + > + priv->icc_pdev = platform_device_register_data( > + dev, icc_driver_name, 0, NULL, 0); > + if (IS_ERR(priv->icc_pdev)) { > + dev_err(dev, "failed to register icc provider %s: %ld\n", > + icc_driver_name, PTR_ERR(priv->devfreq)); > + return PTR_ERR(priv->devfreq); > + } > + > + return 0; > } > > static int imx_devfreq_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -120,18 +150,25 @@ static int imx_devfreq_probe(struct platform_device *pdev) > ret = PTR_ERR(priv->devfreq); > dev_err(dev, "failed to add devfreq device: %d\n", ret); > goto err; > } > > + ret = imx_devfreq_init_icc(dev); > + if (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,imx8mq-noc", .data = "imx8mq-interconnect", }, > + { .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", }, > + { .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", }, > { .compatible = "fsl,imx8m-noc", }, > { .compatible = "fsl,imx8m-nic", }, > { /* sentinel */ }, > }; > MODULE_DEVICE_TABLE(of, imx_devfreq_of_match); >
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. 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? 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. On 12/13/19 10:51 AM, Chanwoo Choi wrote: > On 12/13/19 10:30 AM, Chanwoo Choi wrote: >> Hi, >> >> On 11/15/19 5:09 AM, Leonard Crestez wrote: >>> 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 <leonard.crestez@nxp.com> >>> --- >>> 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. >> >>> +{ >>> + 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. >> >>> +{ >>> + 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. >> >>> + const char *gov = DEVFREQ_GOV_USERSPACE; >>> + void *govdata = NULL; >> >> How about changing the variable name 'govdata' to 'gov_data'? >> - govdata -> gov_data >> >>> + 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. > > And, this driver doesn't include the 'clk_prepare_enable'. > how to enable the clock? > >> >>> + 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; >> } >> >>> + >>> + 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. >> >>> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>"); >>> +MODULE_LICENSE("GPL v2"); >>> >> >> > >
On 16.12.2019 03:00, Chanwoo Choi wrote: > 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. > 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. > 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. > On 12/13/19 10:51 AM, Chanwoo Choi wrote: >> On 12/13/19 10:30 AM, Chanwoo Choi wrote: >>> Hi, >>> >>> On 11/15/19 5:09 AM, Leonard Crestez wrote: >>>> 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 <leonard.crestez@nxp.com> >>>> --- >>>> 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 >>>> +{ >>>> + 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. >>> >>>> +{ >>>> + 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. >>> >>>> + const char *gov = DEVFREQ_GOV_USERSPACE; >>>> + void *govdata = NULL; >>> >>> How about changing the variable name 'govdata' to 'gov_data'? >>> - govdata -> gov_data OK >>>> + 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. >> 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. >>>> + 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. >>>> + 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
On 13.12.2019 06:22, Chanwoo Choi wrote: > Hi, > > On 11/15/19 5:09 AM, Leonard Crestez wrote: >> There is no single device which can represent the imx interconnect. >> Instead of adding a virtual one just make the main &noc act as the >> global interconnect provider. >> >> The imx interconnect provider driver will scale the NOC and DDRC based >> on bandwidth request. More scalable nodes can be added in the future, >> for example for audio/display/vpu/gpu NICs. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> --- >> drivers/devfreq/imx-devfreq.c | 37 +++++++++++++++++++++++++++++++++++ >> 1 file changed, 37 insertions(+) >> >> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c >> index 620b344e87aa..585d340c0f6e 100644 >> --- a/drivers/devfreq/imx-devfreq.c >> +++ b/drivers/devfreq/imx-devfreq.c >> @@ -15,10 +15,11 @@ >> struct imx_devfreq { >> struct devfreq_dev_profile profile; >> struct devfreq *devfreq; >> struct clk *clk; >> struct devfreq_passive_data passive_data; >> + struct platform_device *icc_pdev; >> }; >> >> static int imx_devfreq_target(struct device *dev, >> unsigned long *freq, u32 flags) >> { >> @@ -60,11 +61,40 @@ static int imx_devfreq_get_dev_status(struct device *dev, >> return 0; >> } >> >> static void imx_devfreq_exit(struct device *dev) >> { >> + struct imx_devfreq *priv = dev_get_drvdata(dev); >> + >> dev_pm_opp_of_remove_table(dev); >> + platform_device_unregister(priv->icc_pdev); >> +} >> + >> +/* imx_devfreq_init_icc() - register matching icc provider if required */ >> +static int imx_devfreq_init_icc(struct device *dev) >> +{ >> + struct imx_devfreq *priv = dev_get_drvdata(dev); >> + const char *icc_driver_name; >> + >> + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) >> + return 0; > > It is not proper to check the enable state of CONFIG_INTERCONNECT_IMX configuration > on device driver. Why don't you add the 'select CONFIG_INTERCONNECT_IMX' on Kconfig? Because it's optional. You can disable interconnect support and just tweak frequencies using the devfreq sysfs API. But indeed would only really be useful for debugging. >> + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) >> + return 0; >> + >> + icc_driver_name = of_device_get_match_data(dev); >> + if (!icc_driver_name) >> + return 0; >> + >> + priv->icc_pdev = platform_device_register_data( >> + dev, icc_driver_name, 0, NULL, 0); >> + if (IS_ERR(priv->icc_pdev)) { >> + dev_err(dev, "failed to register icc provider %s: %ld\n", >> + icc_driver_name, PTR_ERR(priv->devfreq)); >> + return PTR_ERR(priv->devfreq); >> + } >> + >> + return 0; >> } >> >> static int imx_devfreq_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -120,18 +150,25 @@ static int imx_devfreq_probe(struct platform_device *pdev) >> ret = PTR_ERR(priv->devfreq); >> dev_err(dev, "failed to add devfreq device: %d\n", ret); >> goto err; >> } >> >> + ret = imx_devfreq_init_icc(dev); >> + if (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,imx8mq-noc", .data = "imx8mq-interconnect", }, >> + { .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", }, >> + { .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", }, >> { .compatible = "fsl,imx8m-noc", }, >> { .compatible = "fsl,imx8m-nic", }, >> { /* sentinel */ }, >> }; >> MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
On 12/16/19 11:57 PM, Leonard Crestez wrote: > On 16.12.2019 03:00, Chanwoo Choi wrote: >> 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. > >> 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. > >> 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. > >> On 12/13/19 10:51 AM, Chanwoo Choi wrote: >>> On 12/13/19 10:30 AM, Chanwoo Choi wrote: >>>> Hi, >>>> >>>> On 11/15/19 5:09 AM, Leonard Crestez wrote: >>>>> 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 <leonard.crestez@nxp.com> >>>>> --- >>>>> 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 > >>>>> +{ >>>>> + 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. >>>> >>>>> +{ >>>>> + 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. > >>>> >>>>> + const char *gov = DEVFREQ_GOV_USERSPACE; >>>>> + void *govdata = NULL; >>>> >>>> How about changing the variable name 'govdata' to 'gov_data'? >>>> - govdata -> gov_data > > OK > >>>>> + 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. > >>> 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. > >>>>> + 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. > >>>>> + 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 > >
On 12/17/19 12:00 AM, Leonard Crestez wrote: > On 13.12.2019 06:22, Chanwoo Choi wrote: >> Hi, >> >> On 11/15/19 5:09 AM, Leonard Crestez wrote: >>> There is no single device which can represent the imx interconnect. >>> Instead of adding a virtual one just make the main &noc act as the >>> global interconnect provider. >>> >>> The imx interconnect provider driver will scale the NOC and DDRC based >>> on bandwidth request. More scalable nodes can be added in the future, >>> for example for audio/display/vpu/gpu NICs. >>> >>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>> --- >>> drivers/devfreq/imx-devfreq.c | 37 +++++++++++++++++++++++++++++++++++ >>> 1 file changed, 37 insertions(+) >>> >>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c >>> index 620b344e87aa..585d340c0f6e 100644 >>> --- a/drivers/devfreq/imx-devfreq.c >>> +++ b/drivers/devfreq/imx-devfreq.c >>> @@ -15,10 +15,11 @@ >>> struct imx_devfreq { >>> struct devfreq_dev_profile profile; >>> struct devfreq *devfreq; >>> struct clk *clk; >>> struct devfreq_passive_data passive_data; >>> + struct platform_device *icc_pdev; >>> }; >>> >>> static int imx_devfreq_target(struct device *dev, >>> unsigned long *freq, u32 flags) >>> { >>> @@ -60,11 +61,40 @@ static int imx_devfreq_get_dev_status(struct device *dev, >>> return 0; >>> } >>> >>> static void imx_devfreq_exit(struct device *dev) >>> { >>> + struct imx_devfreq *priv = dev_get_drvdata(dev); >>> + >>> dev_pm_opp_of_remove_table(dev); >>> + platform_device_unregister(priv->icc_pdev); >>> +} >>> + >>> +/* imx_devfreq_init_icc() - register matching icc provider if required */ >>> +static int imx_devfreq_init_icc(struct device *dev) >>> +{ >>> + struct imx_devfreq *priv = dev_get_drvdata(dev); >>> + const char *icc_driver_name; >>> + >>> + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) >>> + return 0; >> >> It is not proper to check the enable state of CONFIG_INTERCONNECT_IMX configuration >> on device driver. Why don't you add the 'select CONFIG_INTERCONNECT_IMX' on Kconfig? > > Because it's optional. > > You can disable interconnect support and just tweak frequencies using > the devfreq sysfs API. But indeed would only really be useful for debugging. Even if it's optional, I don't prefer to use 'IS_ENABLED' macro. Generally, add or delete the property or value at DT file to either enable or disable the some feature provided by device driver instead of checking the configuration. If user adds the property/value related to interconnect and imx-bus.c configuration is enabled, the behavior related to interconnect on imx-bus.c doesn't work. It make some confusion. The imx-bus.c have to add the 'select CONFIG_INTERCONNECT_IMX' and hand over the right which use the interconnect feature or not, to user. If there are any requirement to add the additional property to check whether interconnect feature will be used or not, you can add the extra property. But, I think that it is enough to check the '#interconnect-cells'. In result, I think that it is right to decide the usage of feature of device driver by user on Devicetree. > >>> + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) >>> + return 0; >>> + >>> + icc_driver_name = of_device_get_match_data(dev); >>> + if (!icc_driver_name) >>> + return 0; >>> + >>> + priv->icc_pdev = platform_device_register_data( >>> + dev, icc_driver_name, 0, NULL, 0); >>> + if (IS_ERR(priv->icc_pdev)) { >>> + dev_err(dev, "failed to register icc provider %s: %ld\n", >>> + icc_driver_name, PTR_ERR(priv->devfreq)); >>> + return PTR_ERR(priv->devfreq); >>> + } >>> + >>> + return 0; >>> } >>> >>> static int imx_devfreq_probe(struct platform_device *pdev) >>> { >>> struct device *dev = &pdev->dev; >>> @@ -120,18 +150,25 @@ static int imx_devfreq_probe(struct platform_device *pdev) >>> ret = PTR_ERR(priv->devfreq); >>> dev_err(dev, "failed to add devfreq device: %d\n", ret); >>> goto err; >>> } >>> >>> + ret = imx_devfreq_init_icc(dev); >>> + if (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,imx8mq-noc", .data = "imx8mq-interconnect", }, >>> + { .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", }, >>> + { .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", }, >>> { .compatible = "fsl,imx8m-noc", }, >>> { .compatible = "fsl,imx8m-nic", }, >>> { /* sentinel */ }, >>> }; >>> MODULE_DEVICE_TABLE(of, imx_devfreq_of_match); > >
On 17.12.2019 02:35, Chanwoo Choi wrote: > On 12/16/19 11:57 PM, Leonard Crestez wrote: >> On 16.12.2019 03:00, Chanwoo Choi wrote: >>> 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. > >> >>> 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. >> >>> 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. > >> >>> On 12/13/19 10:51 AM, Chanwoo Choi wrote: >>>> On 12/13/19 10:30 AM, Chanwoo Choi wrote: >>>>> Hi, >>>>> >>>>> On 11/15/19 5:09 AM, Leonard Crestez wrote: >>>>>> 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 <leonard.crestez@nxp.com> >>>>>> --- >>>>>> 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 The spaces are required in order to align arguments to open paranthesis. Should I drop that? It seems that check_patch.pl and process/coding-style.rst doesn't have a strong opinion on this; my personal preference is for long argument lists to just use double indentation. >>>>>> +{ >>>>>> + 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. >>>>> >>>>>> +{ >>>>>> + 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. > >> >>>>> >>>>>> + const char *gov = DEVFREQ_GOV_USERSPACE; >>>>>> + void *govdata = NULL; >>>>> >>>>> How about changing the variable name 'govdata' to 'gov_data'? >>>>> - govdata -> gov_data >> >> OK >> >>>>>> + 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. >> >>>> 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. From clock framework perspective prepare/enable and rate bits can be controlled separately. Many peripherals are grouped with their own bus (for example a PL301 NIC) which is normally off and only gets enabled when explicitly requested by drivers. If this devfreq driver always enabled bus clocks then it would waste power for no reason. For example a display controller will first enable clocks to allow access to device registers, then configure a resolution and make a bandwith request which gets translated a min_freq request. Then when the display is blanked the entire display bus should be powered off, even if this makes control registers inaccessible. This series only enables scaling for the main NOC which can't be turned off anyway. >>>>>> + 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. I can avoid handling EPROBE_DEFER in a nested if. >>>>>> + 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.
On 12/18/19 6:05 AM, Leonard Crestez wrote: > On 17.12.2019 02:35, Chanwoo Choi wrote: >> On 12/16/19 11:57 PM, Leonard Crestez wrote: >>> On 16.12.2019 03:00, Chanwoo Choi wrote: >>>> 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. >> >>> >>>> 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. >>> >>>> 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. >> >>> >>>> On 12/13/19 10:51 AM, Chanwoo Choi wrote: >>>>> On 12/13/19 10:30 AM, Chanwoo Choi wrote: >>>>>> Hi, >>>>>> >>>>>> On 11/15/19 5:09 AM, Leonard Crestez wrote: >>>>>>> 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 <leonard.crestez@nxp.com> >>>>>>> --- >>>>>>> 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 > > The spaces are required in order to align arguments to open paranthesis. > Should I drop that? > > It seems that check_patch.pl and process/coding-style.rst doesn't have a > strong opinion on this; my personal preference is for long argument > lists to just use double indentation. Generally, almost patches use the tab for the indentation. I don't use space for the indentation. If use the tab for the indentation, it is not harmful for the readability. If use the space for the pretty to make the alignment between parameter, I think it it not good. > >>>>>>> +{ >>>>>>> + 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. >>>>>> >>>>>>> +{ >>>>>>> + 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. >> >>> >>>>>> >>>>>>> + const char *gov = DEVFREQ_GOV_USERSPACE; >>>>>>> + void *govdata = NULL; >>>>>> >>>>>> How about changing the variable name 'govdata' to 'gov_data'? >>>>>> - govdata -> gov_data >>> >>> OK >>> >>>>>>> + 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. >>> >>>>> 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. > > From clock framework perspective prepare/enable and rate bits can be > controlled separately. > > Many peripherals are grouped with their own bus (for example a PL301 > NIC) which is normally off and only gets enabled when explicitly > requested by drivers. If this devfreq driver always enabled bus clocks > then it would waste power for no reason. You can save the power with following sequence. You are keeping the following sequence without any problem. clk_prepare_enable() clk_set_rate() clk_disable_unprepare() clk API support the reference count for the clock user. It doesn't affect the any behavior of other device sharing the bus clock and waste any power-consumption because it will always restore the reference count after changing the clock rate. > > For example a display controller will first enable clocks to allow > access to device registers, then configure a resolution and make a > bandwith request which gets translated a min_freq request. Then when the > display is blanked the entire display bus should be powered off, even if > this makes control registers inaccessible. > > This series only enables scaling for the main NOC which can't be turned > off anyway. > >>>>>>> + 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. > > I can avoid handling EPROBE_DEFER in a nested if. Anyway, if you make the exception more simple, I'm ok. > >>>>>>> + 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. > > > >
On 18.12.2019 05:08, Chanwoo Choi wrote: > On 12/18/19 6:05 AM, Leonard Crestez wrote: >> On 17.12.2019 02:35, Chanwoo Choi wrote: >>> On 12/16/19 11:57 PM, Leonard Crestez wrote: >>>> On 16.12.2019 03:00, Chanwoo Choi wrote: >>>>> 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. >>> >>>> >>>>> 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. >>>> >>>>> 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. >>> >>>> >>>>> On 12/13/19 10:51 AM, Chanwoo Choi wrote: >>>>>> On 12/13/19 10:30 AM, Chanwoo Choi wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 11/15/19 5:09 AM, Leonard Crestez wrote: >>>>>>>> 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 <leonard.crestez@nxp.com> >>>>>>>> --- >>>>>>>> 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 >> >> The spaces are required in order to align arguments to open paranthesis. >> Should I drop that? >> >> It seems that check_patch.pl and process/coding-style.rst doesn't have a >> strong opinion on this; my personal preference is for long argument >> lists to just use double indentation. > > Generally, almost patches use the tab for the indentation. > I don't use space for the indentation. If use the tab > for the indentation, it is not harmful for the readability. > > If use the space for the pretty to make the alignment between parameter, > I think it it not good. OK, I'll just use two tabs. This also matches my personal preference. >>>>>>>> +{ >>>>>>>> + 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. >>>>>>> >>>>>>>> +{ >>>>>>>> + 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. >>> >>>> >>>>>>> >>>>>>>> + const char *gov = DEVFREQ_GOV_USERSPACE; >>>>>>>> + void *govdata = NULL; >>>>>>> >>>>>>> How about changing the variable name 'govdata' to 'gov_data'? >>>>>>> - govdata -> gov_data >>>> >>>> OK >>>> >>>>>>>> + 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. >>>> >>>>>> 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. >> >> From clock framework perspective prepare/enable and rate bits can be >> controlled separately. >> >> Many peripherals are grouped with their own bus (for example a PL301 >> NIC) which is normally off and only gets enabled when explicitly >> requested by drivers. If this devfreq driver always enabled bus clocks >> then it would waste power for no reason. > > You can save the power with following sequence. > You are keeping the following sequence without any problem. > clk_prepare_enable() > clk_set_rate() > clk_disable_unprepare() > > clk API support the reference count for the clock user. > It doesn't affect the any behavior of other device sharing the bus clock > and waste any power-consumption because it will always restore > the reference count after changing the clock rate. But this doesn't serve any purpose? In some cases (depending on clock flags like CLK_SET_RATE_GATE or CLK_SET_RATE_UNGATE flags) the clk_set_rate function can require than clocks are either on or off and otherwise return an error. For imx bus clocks there is no such requirement. >> For example a display controller will first enable clocks to allow >> access to device registers, then configure a resolution and make a >> bandwith request which gets translated a min_freq request. Then when the >> display is blanked the entire display bus should be powered off, even if >> this makes control registers inaccessible. >> >> This series only enables scaling for the main NOC which can't be turned >> off anyway. >> >>>>>>>> + 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. >> >> I can avoid handling EPROBE_DEFER in a nested if. > > Anyway, if you make the exception more simple, I'm ok. > >> >>>>>>>> + 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. >> >> >> >> > >
On 17.12.2019 02:55, Chanwoo Choi wrote: > On 12/17/19 12:00 AM, Leonard Crestez wrote: >> On 13.12.2019 06:22, Chanwoo Choi wrote: >>> Hi, >>> >>> On 11/15/19 5:09 AM, Leonard Crestez wrote: >>>> There is no single device which can represent the imx interconnect. >>>> Instead of adding a virtual one just make the main &noc act as the >>>> global interconnect provider. >>>> >>>> The imx interconnect provider driver will scale the NOC and DDRC based >>>> on bandwidth request. More scalable nodes can be added in the future, >>>> for example for audio/display/vpu/gpu NICs. >>>> >>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>> --- >>>> drivers/devfreq/imx-devfreq.c | 37 +++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 37 insertions(+) >>>> >>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c >>>> index 620b344e87aa..585d340c0f6e 100644 >>>> --- a/drivers/devfreq/imx-devfreq.c >>>> +++ b/drivers/devfreq/imx-devfreq.c >>>> @@ -15,10 +15,11 @@ >>>> struct imx_devfreq { >>>> struct devfreq_dev_profile profile; >>>> struct devfreq *devfreq; >>>> struct clk *clk; >>>> struct devfreq_passive_data passive_data; >>>> + struct platform_device *icc_pdev; >>>> }; >>>> >>>> static int imx_devfreq_target(struct device *dev, >>>> unsigned long *freq, u32 flags) >>>> { >>>> @@ -60,11 +61,40 @@ static int imx_devfreq_get_dev_status(struct device *dev, >>>> return 0; >>>> } >>>> >>>> static void imx_devfreq_exit(struct device *dev) >>>> { >>>> + struct imx_devfreq *priv = dev_get_drvdata(dev); >>>> + >>>> dev_pm_opp_of_remove_table(dev); >>>> + platform_device_unregister(priv->icc_pdev); >>>> +} >>>> + >>>> +/* imx_devfreq_init_icc() - register matching icc provider if required */ >>>> +static int imx_devfreq_init_icc(struct device *dev) >>>> +{ >>>> + struct imx_devfreq *priv = dev_get_drvdata(dev); >>>> + const char *icc_driver_name; >>>> + >>>> + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) >>>> + return 0; >>> >>> It is not proper to check the enable state of CONFIG_INTERCONNECT_IMX configuration >>> on device driver. Why don't you add the 'select CONFIG_INTERCONNECT_IMX' on Kconfig? >> >> Because it's optional. >> >> You can disable interconnect support and just tweak frequencies using >> the devfreq sysfs API. But indeed would only really be useful for debugging. > > Even if it's optional, I don't prefer to use 'IS_ENABLED' macro. > > Generally, add or delete the property or value at DT file > to either enable or disable the some feature provided by device driver > instead of checking the configuration. > > If user adds the property/value related to interconnect > and imx-bus.c configuration is enabled, the behavior > related to interconnect on imx-bus.c doesn't work. It make some confusion. Maybe I could print a warning if #interconnect-cells is present but CONFIG_INTERCONNECT_IMX is off? An explicit select in Kconfig seems like a pointless limitation but in practice it would almost never be useful to build one without the other. > The imx-bus.c have to add the 'select CONFIG_INTERCONNECT_IMX' > and hand over the right which use the interconnect feature or not, to user. > > If there are any requirement to add the additional property > to check whether interconnect feature will be used or not, > you can add the extra property. But, I think that it is enough > to check the '#interconnect-cells'. > > In result, I think that it is right to decide the usage of feature > of device driver by user on Devicetree. > >> >>>> + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) >>>> + return 0; >>>> + >>>> + icc_driver_name = of_device_get_match_data(dev); >>>> + if (!icc_driver_name) >>>> + return 0; >>>> + >>>> + priv->icc_pdev = platform_device_register_data( >>>> + dev, icc_driver_name, 0, NULL, 0); >>>> + if (IS_ERR(priv->icc_pdev)) { >>>> + dev_err(dev, "failed to register icc provider %s: %ld\n", >>>> + icc_driver_name, PTR_ERR(priv->devfreq)); >>>> + return PTR_ERR(priv->devfreq); >>>> + } >>>> + >>>> + return 0; >>>> } >>>> >>>> static int imx_devfreq_probe(struct platform_device *pdev) >>>> { >>>> struct device *dev = &pdev->dev; >>>> @@ -120,18 +150,25 @@ static int imx_devfreq_probe(struct platform_device *pdev) >>>> ret = PTR_ERR(priv->devfreq); >>>> dev_err(dev, "failed to add devfreq device: %d\n", ret); >>>> goto err; >>>> } >>>> >>>> + ret = imx_devfreq_init_icc(dev); >>>> + if (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,imx8mq-noc", .data = "imx8mq-interconnect", }, >>>> + { .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", }, >>>> + { .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", }, >>>> { .compatible = "fsl,imx8m-noc", }, >>>> { .compatible = "fsl,imx8m-nic", }, >>>> { /* sentinel */ }, >>>> }; >>>> MODULE_DEVICE_TABLE(of, imx_devfreq_of_match); >> >> > >
Hi, 2019년 12월 18일 (수) 오후 7:11, Leonard Crestez <leonard.crestez@nxp.com>님이 작성: > > On 18.12.2019 05:08, Chanwoo Choi wrote: > > On 12/18/19 6:05 AM, Leonard Crestez wrote: > >> On 17.12.2019 02:35, Chanwoo Choi wrote: > >>> On 12/16/19 11:57 PM, Leonard Crestez wrote: > >>>> On 16.12.2019 03:00, Chanwoo Choi wrote: > >>>>> 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. > >>> > >>>> > >>>>> 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. > >>>> > >>>>> 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. > >>> > >>>> > >>>>> On 12/13/19 10:51 AM, Chanwoo Choi wrote: > >>>>>> On 12/13/19 10:30 AM, Chanwoo Choi wrote: > >>>>>>> Hi, > >>>>>>> > >>>>>>> On 11/15/19 5:09 AM, Leonard Crestez wrote: > >>>>>>>> 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 <leonard.crestez@nxp.com> > >>>>>>>> --- > >>>>>>>> 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 > >> > >> The spaces are required in order to align arguments to open paranthesis. > >> Should I drop that? > >> > >> It seems that check_patch.pl and process/coding-style.rst doesn't have a > >> strong opinion on this; my personal preference is for long argument > >> lists to just use double indentation. > > > > Generally, almost patches use the tab for the indentation. > > I don't use space for the indentation. If use the tab > > for the indentation, it is not harmful for the readability. > > > > If use the space for the pretty to make the alignment between parameter, > > I think it it not good. > > OK, I'll just use two tabs. This also matches my personal preference. > > >>>>>>>> +{ > >>>>>>>> + 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. > >>>>>>> > >>>>>>>> +{ > >>>>>>>> + 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. > >>> > >>>> > >>>>>>> > >>>>>>>> + const char *gov = DEVFREQ_GOV_USERSPACE; > >>>>>>>> + void *govdata = NULL; > >>>>>>> > >>>>>>> How about changing the variable name 'govdata' to 'gov_data'? > >>>>>>> - govdata -> gov_data > >>>> > >>>> OK > >>>> > >>>>>>>> + 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. > >>>> > >>>>>> 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. > >> > >> From clock framework perspective prepare/enable and rate bits can be > >> controlled separately. > >> > >> Many peripherals are grouped with their own bus (for example a PL301 > >> NIC) which is normally off and only gets enabled when explicitly > >> requested by drivers. If this devfreq driver always enabled bus clocks > >> then it would waste power for no reason. > > > > You can save the power with following sequence. > > You are keeping the following sequence without any problem. > > clk_prepare_enable() > > clk_set_rate() > > clk_disable_unprepare() > > > > clk API support the reference count for the clock user. > > It doesn't affect the any behavior of other device sharing the bus clock > > and waste any power-consumption because it will always restore > > the reference count after changing the clock rate. > > But this doesn't serve any purpose? > > In some cases (depending on clock flags like CLK_SET_RATE_GATE or > CLK_SET_RATE_UNGATE flags) the clk_set_rate function can require than > clocks are either on or off and otherwise return an error. > > For imx bus clocks there is no such requirement. If you guarantee that it is not required on all board related to imx, please add this comment when calling clk_set_rate(). Because the user who don't know the history and character, don't understand why don't enable or disable for clock. I'll agree if you add the comment why don't need to enable/disable control on this driver. > > >> For example a display controller will first enable clocks to allow > >> access to device registers, then configure a resolution and make a > >> bandwith request which gets translated a min_freq request. Then when the > >> display is blanked the entire display bus should be powered off, even if > >> this makes control registers inaccessible. > >> > >> This series only enables scaling for the main NOC which can't be turned > >> off anyway. > >> > >>>>>>>> + 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. > >> > >> I can avoid handling EPROBE_DEFER in a nested if. > > > > Anyway, if you make the exception more simple, I'm ok. > > > >> > >>>>>>>> + 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. > >> > >> > >> > >> > > > > >
2019년 12월 18일 (수) 오후 7:14, Leonard Crestez <leonard.crestez@nxp.com>님이 작성: > > On 17.12.2019 02:55, Chanwoo Choi wrote: > > On 12/17/19 12:00 AM, Leonard Crestez wrote: > >> On 13.12.2019 06:22, Chanwoo Choi wrote: > >>> Hi, > >>> > >>> On 11/15/19 5:09 AM, Leonard Crestez wrote: > >>>> There is no single device which can represent the imx interconnect. > >>>> Instead of adding a virtual one just make the main &noc act as the > >>>> global interconnect provider. > >>>> > >>>> The imx interconnect provider driver will scale the NOC and DDRC based > >>>> on bandwidth request. More scalable nodes can be added in the future, > >>>> for example for audio/display/vpu/gpu NICs. > >>>> > >>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > >>>> --- > >>>> drivers/devfreq/imx-devfreq.c | 37 +++++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 37 insertions(+) > >>>> > >>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c > >>>> index 620b344e87aa..585d340c0f6e 100644 > >>>> --- a/drivers/devfreq/imx-devfreq.c > >>>> +++ b/drivers/devfreq/imx-devfreq.c > >>>> @@ -15,10 +15,11 @@ > >>>> struct imx_devfreq { > >>>> struct devfreq_dev_profile profile; > >>>> struct devfreq *devfreq; > >>>> struct clk *clk; > >>>> struct devfreq_passive_data passive_data; > >>>> + struct platform_device *icc_pdev; > >>>> }; > >>>> > >>>> static int imx_devfreq_target(struct device *dev, > >>>> unsigned long *freq, u32 flags) > >>>> { > >>>> @@ -60,11 +61,40 @@ static int imx_devfreq_get_dev_status(struct device *dev, > >>>> return 0; > >>>> } > >>>> > >>>> static void imx_devfreq_exit(struct device *dev) > >>>> { > >>>> + struct imx_devfreq *priv = dev_get_drvdata(dev); > >>>> + > >>>> dev_pm_opp_of_remove_table(dev); > >>>> + platform_device_unregister(priv->icc_pdev); > >>>> +} > >>>> + > >>>> +/* imx_devfreq_init_icc() - register matching icc provider if required */ > >>>> +static int imx_devfreq_init_icc(struct device *dev) > >>>> +{ > >>>> + struct imx_devfreq *priv = dev_get_drvdata(dev); > >>>> + const char *icc_driver_name; > >>>> + > >>>> + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) > >>>> + return 0; > >>> > >>> It is not proper to check the enable state of CONFIG_INTERCONNECT_IMX configuration > >>> on device driver. Why don't you add the 'select CONFIG_INTERCONNECT_IMX' on Kconfig? > >> > >> Because it's optional. > >> > >> You can disable interconnect support and just tweak frequencies using > >> the devfreq sysfs API. But indeed would only really be useful for debugging. > > > > Even if it's optional, I don't prefer to use 'IS_ENABLED' macro. > > > > Generally, add or delete the property or value at DT file > > to either enable or disable the some feature provided by device driver > > instead of checking the configuration. > > > > If user adds the property/value related to interconnect > > and imx-bus.c configuration is enabled, the behavior > > related to interconnect on imx-bus.c doesn't work. It make some confusion. > > Maybe I could print a warning if #interconnect-cells is present but > CONFIG_INTERCONNECT_IMX is off? Actually, user might think that if imx-bus.c is enabled , CONFIG_INTERCONNECT_MIX is enabled. Because, the dt binding document of imx-bus.c will contain the property for interconnect. If device driver support the various feature, the device driver have to enable all configuration in order to support the features for user. > > An explicit select in Kconfig seems like a pointless limitation but in > practice it would almost never be useful to build one without the other. This patch is for the some CONFIG_INTERCONNECT_IMX driver. I don't understand why is not meaningful to select CONFIG_INTERCONNECT_IMX in Kconfig? > > > The imx-bus.c have to add the 'select CONFIG_INTERCONNECT_IMX' > > and hand over the right which use the interconnect feature or not, to user. > > > > If there are any requirement to add the additional property > > to check whether interconnect feature will be used or not, > > you can add the extra property. But, I think that it is enough > > to check the '#interconnect-cells'. > > > > In result, I think that it is right to decide the usage of feature > > of device driver by user on Devicetree. > > > >> > >>>> + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) > >>>> + return 0; > >>>> + > >>>> + icc_driver_name = of_device_get_match_data(dev); > >>>> + if (!icc_driver_name) > >>>> + return 0; > >>>> + > >>>> + priv->icc_pdev = platform_device_register_data( > >>>> + dev, icc_driver_name, 0, NULL, 0); > >>>> + if (IS_ERR(priv->icc_pdev)) { > >>>> + dev_err(dev, "failed to register icc provider %s: %ld\n", > >>>> + icc_driver_name, PTR_ERR(priv->devfreq)); > >>>> + return PTR_ERR(priv->devfreq); > >>>> + } > >>>> + > >>>> + return 0; > >>>> } > >>>> > >>>> static int imx_devfreq_probe(struct platform_device *pdev) > >>>> { > >>>> struct device *dev = &pdev->dev; > >>>> @@ -120,18 +150,25 @@ static int imx_devfreq_probe(struct platform_device *pdev) > >>>> ret = PTR_ERR(priv->devfreq); > >>>> dev_err(dev, "failed to add devfreq device: %d\n", ret); > >>>> goto err; > >>>> } > >>>> > >>>> + ret = imx_devfreq_init_icc(dev); > >>>> + if (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,imx8mq-noc", .data = "imx8mq-interconnect", }, > >>>> + { .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", }, > >>>> + { .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", }, > >>>> { .compatible = "fsl,imx8m-noc", }, > >>>> { .compatible = "fsl,imx8m-nic", }, > >>>> { /* sentinel */ }, > >>>> }; > >>>> MODULE_DEVICE_TABLE(of, imx_devfreq_of_match); > >> > >> > > > > >
2019년 12월 18일 (수) 오후 7:46, Chanwoo Choi <chanwoo@kernel.org>님이 작성: > > Hi, > > 2019년 12월 18일 (수) 오후 7:11, Leonard Crestez <leonard.crestez@nxp.com>님이 작성: > > > > On 18.12.2019 05:08, Chanwoo Choi wrote: > > > On 12/18/19 6:05 AM, Leonard Crestez wrote: > > >> On 17.12.2019 02:35, Chanwoo Choi wrote: > > >>> On 12/16/19 11:57 PM, Leonard Crestez wrote: > > >>>> On 16.12.2019 03:00, Chanwoo Choi wrote: > > >>>>> 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. > > >>> > > >>>> > > >>>>> 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. > > >>>> > > >>>>> 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. > > >>> > > >>>> > > >>>>> On 12/13/19 10:51 AM, Chanwoo Choi wrote: > > >>>>>> On 12/13/19 10:30 AM, Chanwoo Choi wrote: > > >>>>>>> Hi, > > >>>>>>> > > >>>>>>> On 11/15/19 5:09 AM, Leonard Crestez wrote: > > >>>>>>>> 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 <leonard.crestez@nxp.com> > > >>>>>>>> --- > > >>>>>>>> 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 > > >> > > >> The spaces are required in order to align arguments to open paranthesis. > > >> Should I drop that? > > >> > > >> It seems that check_patch.pl and process/coding-style.rst doesn't have a > > >> strong opinion on this; my personal preference is for long argument > > >> lists to just use double indentation. > > > > > > Generally, almost patches use the tab for the indentation. > > > I don't use space for the indentation. If use the tab > > > for the indentation, it is not harmful for the readability. > > > > > > If use the space for the pretty to make the alignment between parameter, > > > I think it it not good. > > > > OK, I'll just use two tabs. This also matches my personal preference. > > > > >>>>>>>> +{ > > >>>>>>>> + 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. > > >>>>>>> > > >>>>>>>> +{ > > >>>>>>>> + 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. > > >>> > > >>>> > > >>>>>>> > > >>>>>>>> + const char *gov = DEVFREQ_GOV_USERSPACE; > > >>>>>>>> + void *govdata = NULL; > > >>>>>>> > > >>>>>>> How about changing the variable name 'govdata' to 'gov_data'? > > >>>>>>> - govdata -> gov_data > > >>>> > > >>>> OK > > >>>> > > >>>>>>>> + 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. > > >>>> > > >>>>>> 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. > > >> > > >> From clock framework perspective prepare/enable and rate bits can be > > >> controlled separately. > > >> > > >> Many peripherals are grouped with their own bus (for example a PL301 > > >> NIC) which is normally off and only gets enabled when explicitly > > >> requested by drivers. If this devfreq driver always enabled bus clocks > > >> then it would waste power for no reason. > > > > > > You can save the power with following sequence. > > > You are keeping the following sequence without any problem. > > > clk_prepare_enable() > > > clk_set_rate() > > > clk_disable_unprepare() > > > > > > clk API support the reference count for the clock user. > > > It doesn't affect the any behavior of other device sharing the bus clock > > > and waste any power-consumption because it will always restore > > > the reference count after changing the clock rate. > > > > But this doesn't serve any purpose? Again thinking, it show the at least sequence to control the clock. To do this sequence, it doesn't affect the behavior of any device driver which share the clock. And it show the clock is well-controlled by this device driver. > > > > In some cases (depending on clock flags like CLK_SET_RATE_GATE or > > CLK_SET_RATE_UNGATE flags) the clk_set_rate function can require than > > clocks are either on or off and otherwise return an error. > > > > For imx bus clocks there is no such requirement. Usually, the clock is enabled before using it. It doesn't depend on the any arch or SoC. CCF define the this rule for the clock control. > > If you guarantee that it is not required on all board related to imx, > please add this comment when calling clk_set_rate(). > Because the user who don't know the history and character, > don't understand why don't enable or disable for clock. > > I'll agree if you add the comment why don't need to > enable/disable control on this driver. > > > > > > >> For example a display controller will first enable clocks to allow > > >> access to device registers, then configure a resolution and make a > > >> bandwith request which gets translated a min_freq request. Then when the > > >> display is blanked the entire display bus should be powered off, even if > > >> this makes control registers inaccessible. > > >> > > >> This series only enables scaling for the main NOC which can't be turned > > >> off anyway. > > >> > > >>>>>>>> + 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. > > >> > > >> I can avoid handling EPROBE_DEFER in a nested if. > > > > > > Anyway, if you make the exception more simple, I'm ok. > > > > > >> > > >>>>>>>> + 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. > > >> > > >> > > >> > > >> > > > > > > > > > > > -- > Best Regards, > Chanwoo Choi
On 18.12.2019 13:06, Chanwoo Choi wrote: > 2019년 12월 18일 (수) 오후 7:14, Leonard Crestez <leonard.crestez@nxp.com>님이 작성: >> On 17.12.2019 02:55, Chanwoo Choi wrote: >>> On 12/17/19 12:00 AM, Leonard Crestez wrote: >>>> On 13.12.2019 06:22, Chanwoo Choi wrote: >>>>> On 11/15/19 5:09 AM, Leonard Crestez wrote: >>>>>> There is no single device which can represent the imx interconnect. >>>>>> Instead of adding a virtual one just make the main &noc act as the >>>>>> global interconnect provider. >>>>>> >>>>>> The imx interconnect provider driver will scale the NOC and DDRC based >>>>>> on bandwidth request. More scalable nodes can be added in the future, >>>>>> for example for audio/display/vpu/gpu NICs. >>>>>> >>>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>>>> --- >>>>>> drivers/devfreq/imx-devfreq.c | 37 +++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 37 insertions(+) >>>>>> >>>>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c >>>>>> index 620b344e87aa..585d340c0f6e 100644 >>>>>> --- a/drivers/devfreq/imx-devfreq.c >>>>>> +++ b/drivers/devfreq/imx-devfreq.c >>>>>> @@ -15,10 +15,11 @@ >>>>>> struct imx_devfreq { >>>>>> struct devfreq_dev_profile profile; >>>>>> struct devfreq *devfreq; >>>>>> struct clk *clk; >>>>>> struct devfreq_passive_data passive_data; >>>>>> + struct platform_device *icc_pdev; >>>>>> }; >>>>>> >>>>>> static int imx_devfreq_target(struct device *dev, >>>>>> unsigned long *freq, u32 flags) >>>>>> { >>>>>> @@ -60,11 +61,40 @@ static int imx_devfreq_get_dev_status(struct device *dev, >>>>>> return 0; >>>>>> } >>>>>> >>>>>> static void imx_devfreq_exit(struct device *dev) >>>>>> { >>>>>> + struct imx_devfreq *priv = dev_get_drvdata(dev); >>>>>> + >>>>>> dev_pm_opp_of_remove_table(dev); >>>>>> + platform_device_unregister(priv->icc_pdev); >>>>>> +} >>>>>> + >>>>>> +/* imx_devfreq_init_icc() - register matching icc provider if required */ >>>>>> +static int imx_devfreq_init_icc(struct device *dev) >>>>>> +{ >>>>>> + struct imx_devfreq *priv = dev_get_drvdata(dev); >>>>>> + const char *icc_driver_name; >>>>>> + >>>>>> + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) >>>>>> + return 0; >>>>> >>>>> It is not proper to check the enable state of CONFIG_INTERCONNECT_IMX configuration >>>>> on device driver. Why don't you add the 'select CONFIG_INTERCONNECT_IMX' on Kconfig? >>>> >>>> Because it's optional. >>>> >>>> You can disable interconnect support and just tweak frequencies using >>>> the devfreq sysfs API. But indeed would only really be useful for debugging. >>> >>> Even if it's optional, I don't prefer to use 'IS_ENABLED' macro. >>> >>> Generally, add or delete the property or value at DT file >>> to either enable or disable the some feature provided by device driver >>> instead of checking the configuration. >>> >>> If user adds the property/value related to interconnect >>> and imx-bus.c configuration is enabled, the behavior >>> related to interconnect on imx-bus.c doesn't work. It make some confusion. >> >> Maybe I could print a warning if #interconnect-cells is present but >> CONFIG_INTERCONNECT_IMX is off? > > Actually, user might think that if imx-bus.c is enabled > , CONFIG_INTERCONNECT_MIX is enabled. > Because, the dt binding document of imx-bus.c will > contain the property for interconnect. > > If device driver support the various feature, > the device driver have to enable all configuration > in order to support the features for user. >> An explicit select in Kconfig seems like a pointless limitation but in >> practice it would almost never be useful to build one without the other. > > This patch is for the some CONFIG_INTERCONNECT_IMX driver. > I don't understand why is not meaningful to select CONFIG_INTERCONNECT_IMX > in Kconfig? One issue is that the interconnect graph is described per-soc and there are per-soc config options while imx-bus applies to all. So the "if" condition is not sufficient either; if the per-soc interconnect driver is omitted then the platform device will be added but no driver will be ever be found. There are ways around this: for example all of imx interconnect could be built as a single module. But I think it's reasonable for devices to be partially functional if some config options are missing and heavy config customization sometimes requires a bit of debugging. There are various issues when building the current series as "m" but I can solve them and post a final patch which sets all the relevant options on "m" in arm64 defconfig. The it will all "just work" out of the box. >>> The imx-bus.c have to add the 'select CONFIG_INTERCONNECT_IMX' >>> and hand over the right which use the interconnect feature or not, to user. >>> >>> If there are any requirement to add the additional property >>> to check whether interconnect feature will be used or not, >>> you can add the extra property. But, I think that it is enough >>> to check the '#interconnect-cells'. >>> >>> In result, I think that it is right to decide the usage of feature >>> of device driver by user on Devicetree. >>> >>>> >>>>>> + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) >>>>>> + return 0; >>>>>> + >>>>>> + icc_driver_name = of_device_get_match_data(dev); >>>>>> + if (!icc_driver_name) >>>>>> + return 0; >>>>>> + >>>>>> + priv->icc_pdev = platform_device_register_data( >>>>>> + dev, icc_driver_name, 0, NULL, 0); >>>>>> + if (IS_ERR(priv->icc_pdev)) { >>>>>> + dev_err(dev, "failed to register icc provider %s: %ld\n", >>>>>> + icc_driver_name, PTR_ERR(priv->devfreq)); >>>>>> + return PTR_ERR(priv->devfreq); >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> } >>>>>> >>>>>> static int imx_devfreq_probe(struct platform_device *pdev) >>>>>> { >>>>>> struct device *dev = &pdev->dev; >>>>>> @@ -120,18 +150,25 @@ static int imx_devfreq_probe(struct platform_device *pdev) >>>>>> ret = PTR_ERR(priv->devfreq); >>>>>> dev_err(dev, "failed to add devfreq device: %d\n", ret); >>>>>> goto err; >>>>>> } >>>>>> >>>>>> + ret = imx_devfreq_init_icc(dev); >>>>>> + if (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,imx8mq-noc", .data = "imx8mq-interconnect", }, >>>>>> + { .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", }, >>>>>> + { .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", }, >>>>>> { .compatible = "fsl,imx8m-noc", }, >>>>>> { .compatible = "fsl,imx8m-nic", }, >>>>>> { /* sentinel */ }, >>>>>> }; >>>>>> MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
On 12.12.2019 09:29, Georgi Djakov wrote: > Hi Leonard, > > Thank you for your continuous work on the patches and sorry for not reviewing > this earlier. > On 11/14/19 22:09, Leonard Crestez wrote: >> This adds support for i.MX SoC family to interconnect framework. >> >> Platform drivers can describe the interconnect graph and several >> adjustment knobs where icc node bandwidth is converted to a >> DEV_PM_QOS_MIN_FREQUENCY request. >> >> The interconnect provider is probed through the main NOC device and >> other adjustable nodes on the same graph are found from a >> fsl,scalable-nodes phandle array property. >> >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> +static int imx_icc_node_init_qos(struct icc_provider *provider, >> + struct icc_node *node) >> +{ >> + struct imx_icc_node *node_data = node->data; >> + struct device *dev = provider->dev; >> + struct device_node *dn = NULL; >> + struct platform_device *pdev; >> + int i, count; >> + u32 node_id; >> + int ret; >> + >> + count = of_property_count_u32_elems(dev->of_node, >> + "fsl,scalable-node-ids"); >> + if (count < 0) { >> + dev_err(dev, "Failed to parse fsl,scalable-node-ids: %d\n", >> + count); >> + return count; >> + } >> + >> + for (i = 0; i < count; i++) { >> + ret = of_property_read_u32_index(dev->of_node, >> + "fsl,scalable-node-ids", >> + i, &node_id); >> + >> + if (ret < 0) { >> + dev_err(dev, "Failed to parse fsl,scalable-node-ids[%d]: %d\n", >> + i, ret); >> + return ret; >> + } >> + if (node_id != node->id) >> + continue; >> + >> + dn = of_parse_phandle(dev->of_node, "fsl,scalable-nodes", i); > > Why is this needed? I would expect that the interconnect provider driver already > knows which nodes are scalable based on the platform compatible string. > Then maybe this driver should create devfreq devices for each node that is scalable? The scalable nodes are independent devfreq instances which are probed through their own DT compat strings. It's even possible to reload imx8m-ddrc (the driver scaling the dram controller) at runtime. The most common solution to fetch other devices on DT systems is via phandles and fsl,scalable-nodes is a phandle array. Since the provider is platform-specific and knows the topology of the soc it could even use of_find_node_by_path but that seems very messy. It's also quite brittle, I've seen several bugs caused by DT node renaming. This support for arbitrary "scalable nodes" might be excessively generic and strict DT compatibility might be difficult to maintain if too much is exposed. Changing per-soc driver data is otherwise easy. In vendor tree we only ever scale the main NOC and DDRC anyway so equivalent functionality could be achieved with a single "fsl,ddrc" phandle property on the noc. Support for scaling peripheral buses could be implemented by adding additional properties like "fsl,display-nic". Such a feature would need careful measurement on real hardware anyway. -- Regards, Leonard
On 12/19/19 2:13 AM, Leonard Crestez wrote: > On 18.12.2019 13:06, Chanwoo Choi wrote: >> 2019년 12월 18일 (수) 오후 7:14, Leonard Crestez <leonard.crestez@nxp.com>님이 작성: >>> On 17.12.2019 02:55, Chanwoo Choi wrote: >>>> On 12/17/19 12:00 AM, Leonard Crestez wrote: >>>>> On 13.12.2019 06:22, Chanwoo Choi wrote: >>>>>> On 11/15/19 5:09 AM, Leonard Crestez wrote: >>>>>>> There is no single device which can represent the imx interconnect. >>>>>>> Instead of adding a virtual one just make the main &noc act as the >>>>>>> global interconnect provider. >>>>>>> >>>>>>> The imx interconnect provider driver will scale the NOC and DDRC based >>>>>>> on bandwidth request. More scalable nodes can be added in the future, >>>>>>> for example for audio/display/vpu/gpu NICs. >>>>>>> >>>>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>>>>> --- >>>>>>> drivers/devfreq/imx-devfreq.c | 37 +++++++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 37 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c >>>>>>> index 620b344e87aa..585d340c0f6e 100644 >>>>>>> --- a/drivers/devfreq/imx-devfreq.c >>>>>>> +++ b/drivers/devfreq/imx-devfreq.c >>>>>>> @@ -15,10 +15,11 @@ >>>>>>> struct imx_devfreq { >>>>>>> struct devfreq_dev_profile profile; >>>>>>> struct devfreq *devfreq; >>>>>>> struct clk *clk; >>>>>>> struct devfreq_passive_data passive_data; >>>>>>> + struct platform_device *icc_pdev; >>>>>>> }; >>>>>>> >>>>>>> static int imx_devfreq_target(struct device *dev, >>>>>>> unsigned long *freq, u32 flags) >>>>>>> { >>>>>>> @@ -60,11 +61,40 @@ static int imx_devfreq_get_dev_status(struct device *dev, >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> static void imx_devfreq_exit(struct device *dev) >>>>>>> { >>>>>>> + struct imx_devfreq *priv = dev_get_drvdata(dev); >>>>>>> + >>>>>>> dev_pm_opp_of_remove_table(dev); >>>>>>> + platform_device_unregister(priv->icc_pdev); >>>>>>> +} >>>>>>> + >>>>>>> +/* imx_devfreq_init_icc() - register matching icc provider if required */ >>>>>>> +static int imx_devfreq_init_icc(struct device *dev) >>>>>>> +{ >>>>>>> + struct imx_devfreq *priv = dev_get_drvdata(dev); >>>>>>> + const char *icc_driver_name; >>>>>>> + >>>>>>> + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) >>>>>>> + return 0; >>>>>> >>>>>> It is not proper to check the enable state of CONFIG_INTERCONNECT_IMX configuration >>>>>> on device driver. Why don't you add the 'select CONFIG_INTERCONNECT_IMX' on Kconfig? >>>>> >>>>> Because it's optional. >>>>> >>>>> You can disable interconnect support and just tweak frequencies using >>>>> the devfreq sysfs API. But indeed would only really be useful for debugging. >>>> >>>> Even if it's optional, I don't prefer to use 'IS_ENABLED' macro. >>>> >>>> Generally, add or delete the property or value at DT file >>>> to either enable or disable the some feature provided by device driver >>>> instead of checking the configuration. >>>> >>>> If user adds the property/value related to interconnect >>>> and imx-bus.c configuration is enabled, the behavior >>>> related to interconnect on imx-bus.c doesn't work. It make some confusion. >>> >>> Maybe I could print a warning if #interconnect-cells is present but >>> CONFIG_INTERCONNECT_IMX is off? >> >> Actually, user might think that if imx-bus.c is enabled >> , CONFIG_INTERCONNECT_MIX is enabled. >> Because, the dt binding document of imx-bus.c will >> contain the property for interconnect. >> >> If device driver support the various feature, >> the device driver have to enable all configuration >> in order to support the features for user. > > >>> An explicit select in Kconfig seems like a pointless limitation but in >>> practice it would almost never be useful to build one without the other. >> >> This patch is for the some CONFIG_INTERCONNECT_IMX driver. >> I don't understand why is not meaningful to select CONFIG_INTERCONNECT_IMX >> in Kconfig? > > One issue is that the interconnect graph is described per-soc and there > are per-soc config options while imx-bus applies to all. So the "if" > condition is not sufficient either; if the per-soc interconnect driver > is omitted then the platform device will be added but no driver will be > ever be found. > > There are ways around this: for example all of imx interconnect could be > built as a single module. But I think it's reasonable for devices to be > partially functional if some config options are missing and heavy config > customization sometimes requires a bit of debugging. > > There are various issues when building the current series as "m" but I > can solve them and post a final patch which sets all the relevant > options on "m" in arm64 defconfig. The it will all "just work" out of > the box. Sorry. Actually, I don't understand the relationship perfectly between imx-bus and imx-interconnect. As you mentioned, I'll expect the your solution on next version. > >>>> The imx-bus.c have to add the 'select CONFIG_INTERCONNECT_IMX' >>>> and hand over the right which use the interconnect feature or not, to user. >>>> >>>> If there are any requirement to add the additional property >>>> to check whether interconnect feature will be used or not, >>>> you can add the extra property. But, I think that it is enough >>>> to check the '#interconnect-cells'. >>>> >>>> In result, I think that it is right to decide the usage of feature >>>> of device driver by user on Devicetree. >>>> >>>>> >>>>>>> + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) >>>>>>> + return 0; >>>>>>> + >>>>>>> + icc_driver_name = of_device_get_match_data(dev); >>>>>>> + if (!icc_driver_name) >>>>>>> + return 0; >>>>>>> + >>>>>>> + priv->icc_pdev = platform_device_register_data( >>>>>>> + dev, icc_driver_name, 0, NULL, 0); >>>>>>> + if (IS_ERR(priv->icc_pdev)) { >>>>>>> + dev_err(dev, "failed to register icc provider %s: %ld\n", >>>>>>> + icc_driver_name, PTR_ERR(priv->devfreq)); >>>>>>> + return PTR_ERR(priv->devfreq); >>>>>>> + } >>>>>>> + >>>>>>> + return 0; >>>>>>> } >>>>>>> >>>>>>> static int imx_devfreq_probe(struct platform_device *pdev) >>>>>>> { >>>>>>> struct device *dev = &pdev->dev; >>>>>>> @@ -120,18 +150,25 @@ static int imx_devfreq_probe(struct platform_device *pdev) >>>>>>> ret = PTR_ERR(priv->devfreq); >>>>>>> dev_err(dev, "failed to add devfreq device: %d\n", ret); >>>>>>> goto err; >>>>>>> } >>>>>>> >>>>>>> + ret = imx_devfreq_init_icc(dev); >>>>>>> + if (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,imx8mq-noc", .data = "imx8mq-interconnect", }, >>>>>>> + { .compatible = "fsl,imx8mm-noc", .data = "imx8mm-interconnect", }, >>>>>>> + { .compatible = "fsl,imx8mn-noc", .data = "imx8mn-interconnect", }, >>>>>>> { .compatible = "fsl,imx8m-noc", }, >>>>>>> { .compatible = "fsl,imx8m-nic", }, >>>>>>> { /* sentinel */ }, >>>>>>> }; >>>>>>> MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
On 20.11.19 19:02, Leonard Crestez wrote: > On 20.11.2019 18:38, Angus Ainslie wrote: >> On 2019-11-20 08:30, Leonard Crestez wrote: >>> On 20.11.2019 17:41, Angus Ainslie wrote: >>>> Hi Leonard, >>>> >>>> On 2019-11-20 07:04, Leonard Crestez wrote: >>>>> On 20.11.2019 16:08, Angus Ainslie wrote: >>>>> Is "mainline ATF" an important criteria for Purism? >>>> >>>> Yes we intend to bring all of our patches to mainline and were hoping >>>> that NXP would be doing the same. Shouldn't a mainline kernel run on a >>>> mainline ATF ? >>> >>> You can still use mainline ATF (tested right now) but the imx8m-ddrc >>> driver won't probe. >> >> Sorry I was talking about the DDR frequency scaling specifically. > >>> The ability to mix and match different branches of firmware and kernel >>> is very useful for testing. There might be slight incompatibilities but >>> in theory if a feature depends on both firmware and kernel support then >>> it should gracefully degrade rather than crash or hang. >> >> I saw the check you put in for the correct ATF version and that's very >> helpful thanks. >> >>> ATF support for this feature will be mainlined eventually, I picked the >>> linux side first because review is more challenging and changes are >>> much larger relative to what we have in our internal tree. >> >> Do you have a patch against mainline ATF that we can test this feature >> with ? > > Not right now, and imx atf is based on a slightly older version so some > porting effort might be required. > Hi Leonard, Have you, by chance, looked at the mainline ATF side for your devfreq work in the meantime? I'd happily test. (btw. the linux driver fails "nicely" there.) thanks, martin
On 04.02.20 10:45, Martin Kepplinger wrote: > On 20.11.19 19:02, Leonard Crestez wrote: >> On 20.11.2019 18:38, Angus Ainslie wrote: >>> On 2019-11-20 08:30, Leonard Crestez wrote: >>>> On 20.11.2019 17:41, Angus Ainslie wrote: >>>>> Hi Leonard, >>>>> >>>>> On 2019-11-20 07:04, Leonard Crestez wrote: >>>>>> On 20.11.2019 16:08, Angus Ainslie wrote: >>>>>> Is "mainline ATF" an important criteria for Purism? >>>>> >>>>> Yes we intend to bring all of our patches to mainline and were hoping >>>>> that NXP would be doing the same. Shouldn't a mainline kernel run on a >>>>> mainline ATF ? >>>> >>>> You can still use mainline ATF (tested right now) but the imx8m-ddrc >>>> driver won't probe. >>> >>> Sorry I was talking about the DDR frequency scaling specifically. > >>>> The ability to mix and match different branches of firmware and kernel >>>> is very useful for testing. There might be slight incompatibilities but >>>> in theory if a feature depends on both firmware and kernel support then >>>> it should gracefully degrade rather than crash or hang. >>> >>> I saw the check you put in for the correct ATF version and that's very >>> helpful thanks. >>> >>>> ATF support for this feature will be mainlined eventually, I picked the >>>> linux side first because review is more challenging and changes are >>>> much larger relative to what we have in our internal tree. >>> >>> Do you have a patch against mainline ATF that we can test this feature >>> with ? >> >> Not right now, and imx atf is based on a slightly older version so some >> porting effort might be required. >> > > Hi Leonard, > > Have you, by chance, looked at the mainline ATF side for your devfreq > work in the meantime? I'd happily test. (btw. the linux driver fails > "nicely" there.) > hi again. Do you have any plans regarding the ATF implementation of IMX_SIP_DDR_DVFS in mainline yet? The kernel is only one half of the devfreq solution after all. I'm asking just to schedule my work a bit :) thanks, martin