| Message ID | cover.1585188174.git.leonard.crestez@nxp.com |
|---|---|
| Headers | show |
| Series | interconnect: Add imx support via devfreq | expand |
On 26.03.20 03:16, 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. > > Only the ddrc and main noc are scalable right now but more can be added. > > Also available on a github branch (with various unrelated changes): > https://github.com/cdleonard/linux/tree/next > Testing currently requires NXP branch of atf+uboot > > Martin: I believe you should be able to use this to control DRAM > frequency from video by just adding interconnect consumer code to > nwl-dsi. Sample code: > https://github.com/cdleonard/linux/commit/43772762aa5045f1ce5623740f9a4baef988d083 > https://github.com/cdleonard/linux/commit/7b601e981b1f517b5d98b43bde292972ded13086 > Thanks for updating this series Leonard! A few questions for my understanding before trying to test: Isn't the ddrc_opp_table missing in these additions to the DT? That's what I want to scale after all. If I want to keep calling the "request", now icc_set_bw(), in nwl-dsi: I'd add an "interconnects" property to the node, but what would be my interconnect master? i.e.: interconnects = <&noc master? &noc IMX8MQ_ICS_DRAM>; At least it's not obvious to me from interconnect/imx/imx8mq.c the interconnect framework seems to be powerful indeed, but I still need to fully wrap my head around it. thanks for the help so far, martin > Changes since RFCv6: > * Replace scalable-nodes stuff with just a fsl,ddrc property. Future > scalable nodes can be added as additional phandles on the NOC > * Allow building interconnect drivers as modules > * Handle icc_provider_del errors in imx_icc_unregister (like EBUSY). > * Rename imx-devfreq to imx-bus, similar to exynos-bus > * Explain why imx bus clock enabling is not required > * All dependencies accepted (some time ago). > Link: https://patchwork.kernel.org/cover/11244421/ > > 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://patchwork.kernel.org/cover/11222015/ > > 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://patchwork.kernel.org/cover/11111865/ > > 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://patchwork.kernel.org/cover/11078671/ > > 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://patchwork.kernel.org/cover/11021563/ > > Leonard Crestez (8): > 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 > > .../bindings/interconnect/fsl,imx8m-noc.yaml | 138 ++++++++ > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 24 ++ > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 24 ++ > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 24 ++ > drivers/devfreq/Kconfig | 9 + > drivers/devfreq/Makefile | 1 + > drivers/devfreq/imx-bus.c | 181 +++++++++++ > drivers/interconnect/Kconfig | 1 + > drivers/interconnect/Makefile | 1 + > drivers/interconnect/imx/Kconfig | 17 + > drivers/interconnect/imx/Makefile | 9 + > drivers/interconnect/imx/imx.c | 298 ++++++++++++++++++ > drivers/interconnect/imx/imx.h | 62 ++++ > drivers/interconnect/imx/imx8mm.c | 108 +++++++ > drivers/interconnect/imx/imx8mn.c | 97 ++++++ > drivers/interconnect/imx/imx8mq.c | 106 +++++++ > include/dt-bindings/interconnect/imx8mm.h | 49 +++ > include/dt-bindings/interconnect/imx8mn.h | 41 +++ > include/dt-bindings/interconnect/imx8mq.h | 48 +++ > 19 files changed, 1238 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml > create mode 100644 drivers/devfreq/imx-bus.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 Martin, On Thu, Mar 26, 2020 at 02:55:27PM +0100, Martin Kepplinger wrote: > On 26.03.20 03:16, 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. > > > > Only the ddrc and main noc are scalable right now but more can be added. > > > > Also available on a github branch (with various unrelated changes): > > https://github.com/cdleonard/linux/tree/next > > Testing currently requires NXP branch of atf+uboot > > > > Martin: I believe you should be able to use this to control DRAM > > frequency from video by just adding interconnect consumer code to > > nwl-dsi. Sample code: > > https://github.com/cdleonard/linux/commit/43772762aa5045f1ce5623740f9a4baef988d083 > > https://github.com/cdleonard/linux/commit/7b601e981b1f517b5d98b43bde292972ded13086 > > > > Thanks for updating this series Leonard! A few questions for my > understanding before trying to test: > > Isn't the ddrc_opp_table missing in these additions to the DT? That's > what I want to scale after all. > > If I want to keep calling the "request", now icc_set_bw(), in nwl-dsi: > I'd add an "interconnects" property to the node, but what would be my > interconnect master? i.e.: interconnects = <&noc master? &noc > IMX8MQ_ICS_DRAM>; At least it's not obvious to me from > interconnect/imx/imx8mq.c The NWL DSI host controller is fed by DCSS or mxsfb so any bandwidth requirements should (as far as I understand things) go into the display controller driver since that's what fetches from RAM. Cheers, -- Guido > > the interconnect framework seems to be powerful indeed, but I still need > to fully wrap my head around it. > > thanks for the help so far, > > martin > > > > Changes since RFCv6: > > * Replace scalable-nodes stuff with just a fsl,ddrc property. Future > > scalable nodes can be added as additional phandles on the NOC > > * Allow building interconnect drivers as modules > > * Handle icc_provider_del errors in imx_icc_unregister (like EBUSY). > > * Rename imx-devfreq to imx-bus, similar to exynos-bus > > * Explain why imx bus clock enabling is not required > > * All dependencies accepted (some time ago). > > Link: https://patchwork.kernel.org/cover/11244421/ > > > > 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://patchwork.kernel.org/cover/11222015/ > > > > 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://patchwork.kernel.org/cover/11111865/ > > > > 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://patchwork.kernel.org/cover/11078671/ > > > > 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://patchwork.kernel.org/cover/11021563/ > > > > Leonard Crestez (8): > > 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 > > > > .../bindings/interconnect/fsl,imx8m-noc.yaml | 138 ++++++++ > > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 24 ++ > > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 24 ++ > > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 24 ++ > > drivers/devfreq/Kconfig | 9 + > > drivers/devfreq/Makefile | 1 + > > drivers/devfreq/imx-bus.c | 181 +++++++++++ > > drivers/interconnect/Kconfig | 1 + > > drivers/interconnect/Makefile | 1 + > > drivers/interconnect/imx/Kconfig | 17 + > > drivers/interconnect/imx/Makefile | 9 + > > drivers/interconnect/imx/imx.c | 298 ++++++++++++++++++ > > drivers/interconnect/imx/imx.h | 62 ++++ > > drivers/interconnect/imx/imx8mm.c | 108 +++++++ > > drivers/interconnect/imx/imx8mn.c | 97 ++++++ > > drivers/interconnect/imx/imx8mq.c | 106 +++++++ > > include/dt-bindings/interconnect/imx8mm.h | 49 +++ > > include/dt-bindings/interconnect/imx8mn.h | 41 +++ > > include/dt-bindings/interconnect/imx8mq.h | 48 +++ > > 19 files changed, 1238 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml > > create mode 100644 drivers/devfreq/imx-bus.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 > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 27.03.20 12:36, Guido Günther wrote: > Hi Martin, > On Thu, Mar 26, 2020 at 02:55:27PM +0100, Martin Kepplinger wrote: >> On 26.03.20 03:16, 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. >>> >>> Only the ddrc and main noc are scalable right now but more can be added. >>> >>> Also available on a github branch (with various unrelated changes): >>> https://github.com/cdleonard/linux/tree/next >>> Testing currently requires NXP branch of atf+uboot >>> >>> Martin: I believe you should be able to use this to control DRAM >>> frequency from video by just adding interconnect consumer code to >>> nwl-dsi. Sample code: >>> https://github.com/cdleonard/linux/commit/43772762aa5045f1ce5623740f9a4baef988d083 >>> https://github.com/cdleonard/linux/commit/7b601e981b1f517b5d98b43bde292972ded13086 >>> >> >> Thanks for updating this series Leonard! A few questions for my >> understanding before trying to test: >> >> Isn't the ddrc_opp_table missing in these additions to the DT? That's >> what I want to scale after all. >> >> If I want to keep calling the "request", now icc_set_bw(), in nwl-dsi: >> I'd add an "interconnects" property to the node, but what would be my >> interconnect master? i.e.: interconnects = <&noc master? &noc >> IMX8MQ_ICS_DRAM>; At least it's not obvious to me from >> interconnect/imx/imx8mq.c > > The NWL DSI host controller is fed by DCSS or mxsfb so any bandwidth > requirements should (as far as I understand things) go into the display > controller driver since that's what fetches from RAM. > Cheers, > -- Guido > Hi, Thanks a lot Leonard and Guido! Here's the tree I'm running, which is your patches based on Linus' tree, with icc request in mxsfb: https://source.puri.sm/martin.kepplinger/linux-next/commits/5.6-rc7/librem5__integration_devfreq1 The path from icc via pm_qos to devfreq does work (which is great) - however only after setting the minimum frequencies via a governor - I set the "powersave" governor. After that, frequencies are both set to high / low correctly. My impression was that I should be able to use the "passive" governor (a passive devfreq device?). What am I missing with using devfreq correctly? Or do I already? other than the above uncertainty: Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm> thanks so far! martin
On 2020-03-30 11:52 AM, Martin Kepplinger wrote: > On 27.03.20 12:36, Guido Günther wrote: >> Hi Martin, >> On Thu, Mar 26, 2020 at 02:55:27PM +0100, Martin Kepplinger wrote: >>> On 26.03.20 03:16, 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. >>>> >>>> Only the ddrc and main noc are scalable right now but more can be added. >>>> >>>> Also available on a github branch (with various unrelated changes): >>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcdleonard%2Flinux%2Ftree%2Fnext&data=02%7C01%7Cleonard.crestez%40nxp.com%7C52eb55181dce469eb85608d7d487bb93%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637211551737394247&sdata=A0l5FBF%2BT3k7H5HMtRMfX8WfVSqQm9jijgr8aexCoUA%3D&reserved=0 >>>> Testing currently requires NXP branch of atf+uboot >>>> >>>> Martin: I believe you should be able to use this to control DRAM >>>> frequency from video by just adding interconnect consumer code to >>>> nwl-dsi. Sample code: >>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcdleonard%2Flinux%2Fcommit%2F43772762aa5045f1ce5623740f9a4baef988d083&data=02%7C01%7Cleonard.crestez%40nxp.com%7C52eb55181dce469eb85608d7d487bb93%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637211551737394247&sdata=%2B%2BGWQTaQLLk98yFRHJ0o3Sks9DHGuKv7twBvn89f1Tg%3D&reserved=0 >>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcdleonard%2Flinux%2Fcommit%2F7b601e981b1f517b5d98b43bde292972ded13086&data=02%7C01%7Cleonard.crestez%40nxp.com%7C52eb55181dce469eb85608d7d487bb93%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637211551737394247&sdata=Jy2%2FI3CE1H3ilmLvZAVhjlPHO3KRNK6%2F9dX%2BS124ROA%3D&reserved=0 >>>> >>> >>> Thanks for updating this series Leonard! A few questions for my >>> understanding before trying to test: >>> >>> Isn't the ddrc_opp_table missing in these additions to the DT? That's >>> what I want to scale after all. DDRC OPP table belongs in board file because it can vary with RAM type on boards. >>> If I want to keep calling the "request", now icc_set_bw(), in nwl-dsi: >>> I'd add an "interconnects" property to the node, but what would be my >>> interconnect master? i.e.: interconnects = <&noc master? &noc >>> IMX8MQ_ICS_DRAM>; At least it's not obvious to me from >>> interconnect/imx/imx8mq.c >> >> The NWL DSI host controller is fed by DCSS or mxsfb so any bandwidth >> requirements should (as far as I understand things) go into the display >> controller driver since that's what fetches from RAM. >> Cheers, >> -- Guido This is correct. > Thanks a lot Leonard and Guido! Here's the tree I'm running, which is > your patches based on Linus' tree, with icc request in mxsfb: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2Fmartin.kepplinger%2Flinux-next%2Fcommits%2F5.6-rc7%2Flibrem5__integration_devfreq1&data=02%7C01%7Cleonard.crestez%40nxp.com%7C52eb55181dce469eb85608d7d487bb93%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637211551737394247&sdata=FM8JoOuNa2gg09XVJ%2FLTLqK9rlL4hAwig2iM9cMYFhg%3D&reserved=0 > > The path from icc via pm_qos to devfreq does work (which is great) - > however only after setting the minimum frequencies via a governor - I > set the "powersave" governor. > > After that, frequencies are both set to high / low correctly. > > My impression was that I should be able to use the "passive" governor (a > passive devfreq device?). What am I missing with using devfreq > correctly? Or do I already? The devfreq governor is something else: it's used to make one devfreq device match frequencies with another devfreq device. Setting the default governor to "powersave" is correct and roughly matches behavior in NXP kernel. > Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm> Thanks!
Hi, Looks good to me. I added the comments. But, it need to add the dt binding documentation for this device. The old email of Artur Świgoń is not used. On next time, use following the new email address Because when I reply the mail, always show the fail message from thunderbird due to the Artur's old email. <a.swigon@partnet.samsung.com> -> <a.swigon@samsung.com> On 3/26/20 11:16 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-bus.c | 142 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 152 insertions(+) > create mode 100644 drivers/devfreq/imx-bus.c > > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig > index 0b1df12e0f21..44d26192ddc4 100644 > --- a/drivers/devfreq/Kconfig > +++ b/drivers/devfreq/Kconfig > @@ -99,10 +99,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_BUS_DEVFREQ > + tristate "i.MX Generic Bus DEVFREQ Driver" > + depends on ARCH_MXC || COMPILE_TEST > + select DEVFREQ_GOV_PASSIVE > + select DEVFREQ_GOV_USERSPACE Maybe, you would update it by using passive governor? But, in this version, it doesn't handle the any passive governor. > + 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..3ca1ad0ecb97 100644 > --- a/drivers/devfreq/Makefile > +++ b/drivers/devfreq/Makefile > @@ -7,10 +7,11 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o > obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o > obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o > > # DEVFREQ Drivers > obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o > +obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ) += imx-bus.o The ARM_IMX_BUS_DEVFREQ config is under ARM_IMX8M_DDRC_DEVFREQ and imx-bus.o is over imx8m-ddrc.o. Need to edit the sequence. > obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o > obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o > obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o > obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o > > diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c > new file mode 100644 > index 000000000000..285e0f1ae6b1 > --- /dev/null > +++ b/drivers/devfreq/imx-bus.c > @@ -0,0 +1,142 @@ > +// 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_bus { > + struct devfreq_dev_profile profile; > + struct devfreq *devfreq; > + struct clk *clk; > + struct devfreq_passive_data passive_data; This patch doesn't touch the passive_data. > +}; > + > +static int imx_bus_target(struct device *dev, > + unsigned long *freq, u32 flags) > +{ > + struct imx_bus *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); It doesn't need. Because the new frequency is stored to 'freq' by calling devfreq_recommended_opp(). > + dev_pm_opp_put(new_opp); > + > + return clk_set_rate(priv->clk, new_freq); nitpick. you can use dev_pm_opp_set_rate(). But, I'm not forcing to use it. > +} > + > +static int imx_bus_get_cur_freq(struct device *dev, unsigned long *freq) > +{ > + struct imx_bus *priv = dev_get_drvdata(dev); > + > + *freq = clk_get_rate(priv->clk); > + > + return 0; > +} > + > +static int imx_bus_get_dev_status(struct device *dev, > + struct devfreq_dev_status *stat) > +{ > + struct imx_bus *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_bus_exit(struct device *dev) > +{ > + dev_pm_opp_of_remove_table(dev); > +} > + > +static int imx_bus_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct imx_bus *priv; > + const char *gov = DEVFREQ_GOV_USERSPACE; > + int ret; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + /* > + * Fetch the clock to adjust but don't explictly enable. Need to fix typo. s/explictly/explicitly > + * > + * For imx bus clock clk_set_rate is safe no matter if the clock is on > + * or off and some peripheral side-buses might be off unless enabled by > + * drivers for devices on those specific buses. > + * > + * Rate adjustment on a disabled bus clock just takes effect later. > + */ > + 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_bus_target; > + priv->profile.get_dev_status = imx_bus_get_dev_status; > + priv->profile.exit = imx_bus_exit; > + priv->profile.get_cur_freq = imx_bus_get_cur_freq; > + priv->profile.initial_freq = clk_get_rate(priv->clk); > + > + priv->devfreq = devm_devfreq_add_device(dev, &priv->profile, > + gov, NULL); > + 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_bus_of_match[] = { > + { .compatible = "fsl,imx8m-noc", }, > + { .compatible = "fsl,imx8m-nic", }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, imx_bus_of_match); > + > +static struct platform_driver imx_bus_platdrv = { > + .probe = imx_bus_probe, > + .driver = { > + .name = "imx-bus-devfreq", > + .of_match_table = of_match_ptr(imx_bus_of_match), > + }, > +}; > +module_platform_driver(imx_bus_platdrv); > + > +MODULE_DESCRIPTION("Generic i.MX bus frequency scaling driver"); > +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>"); > +MODULE_LICENSE("GPL v2"); >
Hi Leonard, On 3/26/20 11:16 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-bus.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c > index 285e0f1ae6b1..e9b13e43bf0a 100644 > --- a/drivers/devfreq/imx-bus.c > +++ b/drivers/devfreq/imx-bus.c > @@ -15,10 +15,11 @@ > struct imx_bus { > 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_bus_target(struct device *dev, > unsigned long *freq, u32 flags) > { > @@ -60,11 +61,42 @@ static int imx_bus_get_dev_status(struct device *dev, > return 0; > } > > static void imx_bus_exit(struct device *dev) > { > + struct imx_bus *priv = dev_get_drvdata(dev); > + > dev_pm_opp_of_remove_table(dev); > + platform_device_unregister(priv->icc_pdev); > +} > + > +/* imx_bus_init_icc() - register matching icc provider if required */ Better to add following comments without 'imx_bus_init_icc() -' comment. /* Register matching icc provider if required */ > +static int imx_bus_init_icc(struct device *dev) > +{ > + struct imx_bus *priv = dev_get_drvdata(dev); > + const char *icc_driver_name; > + > + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) > + return 0; > + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) { > + dev_warn(dev, "imx interconnect drivers disabled\n"); > + return 0; > + } > + > + icc_driver_name = of_device_get_match_data(dev); > + if (!icc_driver_name) > + return 0; Recommend to add the error or warning message. > + > + priv->icc_pdev = platform_device_register_data( > + dev, icc_driver_name, -1, 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_bus_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -112,18 +144,25 @@ static int imx_bus_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_bus_init_icc(dev); > + if (ret) Recommend to add the error message. > + goto err; > + > return 0; > > err: > dev_pm_opp_of_remove_table(dev); > return ret; > } > > static const struct of_device_id imx_bus_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_bus_of_match); >
On 2020-04-01 2:00 AM, Chanwoo Choi wrote: > Hi Leonard, > > On 3/26/20 11:16 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-bus.c | 39 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c >> index 285e0f1ae6b1..e9b13e43bf0a 100644 >> --- a/drivers/devfreq/imx-bus.c >> +++ b/drivers/devfreq/imx-bus.c >> @@ -15,10 +15,11 @@ >> struct imx_bus { >> 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_bus_target(struct device *dev, >> unsigned long *freq, u32 flags) >> { >> @@ -60,11 +61,42 @@ static int imx_bus_get_dev_status(struct device *dev, >> return 0; >> } >> >> static void imx_bus_exit(struct device *dev) >> { >> + struct imx_bus *priv = dev_get_drvdata(dev); >> + >> dev_pm_opp_of_remove_table(dev); >> + platform_device_unregister(priv->icc_pdev); >> +} >> + >> +/* imx_bus_init_icc() - register matching icc provider if required */ > > Better to add following comments without 'imx_bus_init_icc() -' comment. > /* Register matching icc provider if required */ This form looks like standard kernel-doc >> +static int imx_bus_init_icc(struct device *dev) >> +{ >> + struct imx_bus *priv = dev_get_drvdata(dev); >> + const char *icc_driver_name; >> + >> + if (!of_get_property(dev->of_node, "#interconnect-cells", 0)) >> + return 0; >> + if (!IS_ENABLED(CONFIG_INTERCONNECT_IMX)) { >> + dev_warn(dev, "imx interconnect drivers disabled\n"); >> + return 0; >> + } >> + >> + icc_driver_name = of_device_get_match_data(dev); >> + if (!icc_driver_name) >> + return 0; > > Recommend to add the error or warning message. Makes sense, imx8m-nic shouldn't have #interconnect-cells so if this condition is reach an error should be printed. >> + >> + priv->icc_pdev = platform_device_register_data( >> + dev, icc_driver_name, -1, 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_bus_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -112,18 +144,25 @@ static int imx_bus_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_bus_init_icc(dev); >> + if (ret) > > Recommend to add the error message. imx_bus_init_icc already prints errors. Printing again here would always result in a second message. >> + goto err; >> + >> return 0; >> >> err: >> dev_pm_opp_of_remove_table(dev); >> return ret; >> } >> >> static const struct of_device_id imx_bus_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_bus_of_match); >>
On 2020-04-01 1:55 AM, Chanwoo Choi wrote: > Hi, > > Looks good to me. I added the comments. > But, it need to add the dt binding documentation for this device. DT bindings were included: https://patchwork.kernel.org/patch/11458981/ > The old email of Artur Świgoń is not used. On next time, > use following the new email address Because when I reply the mail, > always show the fail message from thunderbird due to the Artur's old email. > <a.swigon@partnet.samsung.com> -> <a.swigon@samsung.com> Yeah, I received multiple bounces because of this. > On 3/26/20 11:16 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-bus.c | 142 ++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 152 insertions(+) >> create mode 100644 drivers/devfreq/imx-bus.c >> >> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig >> index 0b1df12e0f21..44d26192ddc4 100644 >> --- a/drivers/devfreq/Kconfig >> +++ b/drivers/devfreq/Kconfig >> @@ -99,10 +99,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_BUS_DEVFREQ >> + tristate "i.MX Generic Bus DEVFREQ Driver" >> + depends on ARCH_MXC || COMPILE_TEST >> + select DEVFREQ_GOV_PASSIVE >> + select DEVFREQ_GOV_USERSPACE > > Maybe, you would update it by using passive governor? > But, in this version, it doesn't handle the any passive governor. dropped >> + 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..3ca1ad0ecb97 100644 >> --- a/drivers/devfreq/Makefile >> +++ b/drivers/devfreq/Makefile >> @@ -7,10 +7,11 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o >> obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o >> obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o >> >> # DEVFREQ Drivers >> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o >> +obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ) += imx-bus.o > > The ARM_IMX_BUS_DEVFREQ config is under ARM_IMX8M_DDRC_DEVFREQ > and imx-bus.o is over imx8m-ddrc.o. Need to edit the sequence. Reordered kconfig to match. 8M_DDRC sorts before _BUS alphabetically but it's pettier this way, and matches tegra. >> obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o >> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o >> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o >> obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o >> >> diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c >> new file mode 100644 >> index 000000000000..285e0f1ae6b1 >> --- /dev/null >> +++ b/drivers/devfreq/imx-bus.c >> @@ -0,0 +1,142 @@ >> +// 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_bus { >> + struct devfreq_dev_profile profile; >> + struct devfreq *devfreq; >> + struct clk *clk; >> + struct devfreq_passive_data passive_data; > > This patch doesn't touch the passive_data. dropped >> +}; >> + >> +static int imx_bus_target(struct device *dev, >> + unsigned long *freq, u32 flags) >> +{ >> + struct imx_bus *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); > > It doesn't need. Because the new frequency is stored to 'freq' > by calling devfreq_recommended_opp(). fixed >> + dev_pm_opp_put(new_opp); >> + >> + return clk_set_rate(priv->clk, new_freq); > > nitpick. you can use dev_pm_opp_set_rate(). But, I'm not forcing to use it. Switched to dev_pm_opp_set_rate. It might be interesting to add regulators control later, on some chips the main NOC can run at different voltages. > >> +} >> + >> +static int imx_bus_get_cur_freq(struct device *dev, unsigned long *freq) >> +{ >> + struct imx_bus *priv = dev_get_drvdata(dev); >> + >> + *freq = clk_get_rate(priv->clk); >> + >> + return 0; >> +} >> + >> +static int imx_bus_get_dev_status(struct device *dev, >> + struct devfreq_dev_status *stat) >> +{ >> + struct imx_bus *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_bus_exit(struct device *dev) >> +{ >> + dev_pm_opp_of_remove_table(dev); >> +} >> + >> +static int imx_bus_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct imx_bus *priv; >> + const char *gov = DEVFREQ_GOV_USERSPACE; >> + int ret; >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + /* >> + * Fetch the clock to adjust but don't explictly enable. > > Need to fix typo. > s/explictly/explicitly fixed >> + * >> + * For imx bus clock clk_set_rate is safe no matter if the clock is on >> + * or off and some peripheral side-buses might be off unless enabled by >> + * drivers for devices on those specific buses. >> + * >> + * Rate adjustment on a disabled bus clock just takes effect later. >> + */ >> + 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_bus_target; >> + priv->profile.get_dev_status = imx_bus_get_dev_status; >> + priv->profile.exit = imx_bus_exit; >> + priv->profile.get_cur_freq = imx_bus_get_cur_freq; >> + priv->profile.initial_freq = clk_get_rate(priv->clk); >> + >> + priv->devfreq = devm_devfreq_add_device(dev, &priv->profile, >> + gov, NULL); >> + 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_bus_of_match[] = { >> + { .compatible = "fsl,imx8m-noc", }, >> + { .compatible = "fsl,imx8m-nic", }, >> + { /* sentinel */ }, >> +}; >> +MODULE_DEVICE_TABLE(of, imx_bus_of_match); >> + >> +static struct platform_driver imx_bus_platdrv = { >> + .probe = imx_bus_probe, >> + .driver = { >> + .name = "imx-bus-devfreq", >> + .of_match_table = of_match_ptr(imx_bus_of_match), >> + }, >> +}; >> +module_platform_driver(imx_bus_platdrv); >> + >> +MODULE_DESCRIPTION("Generic i.MX bus frequency scaling driver"); >> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>"); >> +MODULE_LICENSE("GPL v2"); >> > >
On 4/1/20 11:20 PM, Leonard Crestez wrote: > On 2020-04-01 1:55 AM, Chanwoo Choi wrote: >> Hi, >> >> Looks good to me. I added the comments. >> But, it need to add the dt binding documentation for this device. > > DT bindings were included: > > https://patchwork.kernel.org/patch/11458981/ The dt-binding document for this driver is required under Documentation/devicetree/binding/devfreq. It is difficult to catch where is the dt-binding document for this driver for who don't know the detailed history of this driver. I don't said that add the duplicate documentation But, at least the some document have to point out the reference. > >> The old email of Artur Świgoń is not used. On next time, >> use following the new email address Because when I reply the mail, >> always show the fail message from thunderbird due to the Artur's old email. >> <a.swigon@partnet.samsung.com> -> <a.swigon@samsung.com> > > Yeah, I received multiple bounces because of this. > >> On 3/26/20 11:16 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-bus.c | 142 ++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 152 insertions(+) >>> create mode 100644 drivers/devfreq/imx-bus.c >>> >>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig >>> index 0b1df12e0f21..44d26192ddc4 100644 >>> --- a/drivers/devfreq/Kconfig >>> +++ b/drivers/devfreq/Kconfig >>> @@ -99,10 +99,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_BUS_DEVFREQ >>> + tristate "i.MX Generic Bus DEVFREQ Driver" >>> + depends on ARCH_MXC || COMPILE_TEST >>> + select DEVFREQ_GOV_PASSIVE >>> + select DEVFREQ_GOV_USERSPACE >> >> Maybe, you would update it by using passive governor? >> But, in this version, it doesn't handle the any passive governor. > > dropped > >>> + 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..3ca1ad0ecb97 100644 >>> --- a/drivers/devfreq/Makefile >>> +++ b/drivers/devfreq/Makefile >>> @@ -7,10 +7,11 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o >>> obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o >>> obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o >>> >>> # DEVFREQ Drivers >>> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o >>> +obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ) += imx-bus.o >> >> The ARM_IMX_BUS_DEVFREQ config is under ARM_IMX8M_DDRC_DEVFREQ >> and imx-bus.o is over imx8m-ddrc.o. Need to edit the sequence. > > Reordered kconfig to match. 8M_DDRC sorts before _BUS alphabetically but > it's pettier this way, and matches tegra. > >>> obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o >>> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o >>> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o >>> obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o >>> >>> diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c >>> new file mode 100644 >>> index 000000000000..285e0f1ae6b1 >>> --- /dev/null >>> +++ b/drivers/devfreq/imx-bus.c >>> @@ -0,0 +1,142 @@ >>> +// 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_bus { >>> + struct devfreq_dev_profile profile; >>> + struct devfreq *devfreq; >>> + struct clk *clk; >>> + struct devfreq_passive_data passive_data; >> >> This patch doesn't touch the passive_data. > > dropped > >>> +}; >>> + >>> +static int imx_bus_target(struct device *dev, >>> + unsigned long *freq, u32 flags) >>> +{ >>> + struct imx_bus *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); >> >> It doesn't need. Because the new frequency is stored to 'freq' >> by calling devfreq_recommended_opp(). > > fixed > >>> + dev_pm_opp_put(new_opp); >>> + >>> + return clk_set_rate(priv->clk, new_freq); >> >> nitpick. you can use dev_pm_opp_set_rate(). But, I'm not forcing to use it. > > Switched to dev_pm_opp_set_rate. > > It might be interesting to add regulators control later, on some chips > the main NOC can run at different voltages. > >> >>> +} >>> + >>> +static int imx_bus_get_cur_freq(struct device *dev, unsigned long *freq) >>> +{ >>> + struct imx_bus *priv = dev_get_drvdata(dev); >>> + >>> + *freq = clk_get_rate(priv->clk); >>> + >>> + return 0; >>> +} >>> + >>> +static int imx_bus_get_dev_status(struct device *dev, >>> + struct devfreq_dev_status *stat) >>> +{ >>> + struct imx_bus *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_bus_exit(struct device *dev) >>> +{ >>> + dev_pm_opp_of_remove_table(dev); >>> +} >>> + >>> +static int imx_bus_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct imx_bus *priv; >>> + const char *gov = DEVFREQ_GOV_USERSPACE; >>> + int ret; >>> + >>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>> + if (!priv) >>> + return -ENOMEM; >>> + >>> + /* >>> + * Fetch the clock to adjust but don't explictly enable. >> >> Need to fix typo. >> s/explictly/explicitly > > fixed > >>> + * >>> + * For imx bus clock clk_set_rate is safe no matter if the clock is on >>> + * or off and some peripheral side-buses might be off unless enabled by >>> + * drivers for devices on those specific buses. >>> + * >>> + * Rate adjustment on a disabled bus clock just takes effect later. >>> + */ >>> + 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_bus_target; >>> + priv->profile.get_dev_status = imx_bus_get_dev_status; >>> + priv->profile.exit = imx_bus_exit; >>> + priv->profile.get_cur_freq = imx_bus_get_cur_freq; >>> + priv->profile.initial_freq = clk_get_rate(priv->clk); >>> + >>> + priv->devfreq = devm_devfreq_add_device(dev, &priv->profile, >>> + gov, NULL); >>> + 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_bus_of_match[] = { >>> + { .compatible = "fsl,imx8m-noc", }, >>> + { .compatible = "fsl,imx8m-nic", }, >>> + { /* sentinel */ }, >>> +}; >>> +MODULE_DEVICE_TABLE(of, imx_bus_of_match); >>> + >>> +static struct platform_driver imx_bus_platdrv = { >>> + .probe = imx_bus_probe, >>> + .driver = { >>> + .name = "imx-bus-devfreq", >>> + .of_match_table = of_match_ptr(imx_bus_of_match), >>> + }, >>> +}; >>> +module_platform_driver(imx_bus_platdrv); >>> + >>> +MODULE_DESCRIPTION("Generic i.MX bus frequency scaling driver"); >>> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>"); >>> +MODULE_LICENSE("GPL v2"); >>> >> >> > > > >
On 2020-04-02 1:48 AM, Chanwoo Choi wrote: > On 4/1/20 11:20 PM, Leonard Crestez wrote: >> On 2020-04-01 1:55 AM, Chanwoo Choi wrote: >>> Hi, >>> >>> Looks good to me. I added the comments. >>> But, it need to add the dt binding documentation for this device. >> >> DT bindings were included: >> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11458981%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C7381d117a4d1468cd2c608d7d68ecfac%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637213781167514099&sdata=Qu8x14cXnuxlwOT2SlUOf%2FLgCVWbnJRKA4TBjMIWQeA%3D&reserved=0 > > The dt-binding document for this driver is required under > Documentation/devicetree/binding/devfreq. Bindings for imx8m-ddrc were at one point posted for devicetree/bindings/devfreq but Rob Herring suggested to move them under "memory-controller" instead and I expect same logic makes sense here. Link to previous discussion: https://patchwork.kernel.org/patch/11221919/ DT bindings should try to describe "hardware" rather than "drivers" and an "interconnect" is a class of hardware while "devfreq" isn't. Not only that but the main noc has properties parsed by interconnect driver. > It is difficult to catch where is the dt-binding document > for this driver for who don't know the detailed history > of this driver. I don't said that add the duplicate documentation > But, at least the some document have to point out the reference. What I usually do to find information about a device is grep for the compat string in the entire tree. >>> The old email of Artur Świgoń is not used. On next time, >>> use following the new email address Because when I reply the mail, >>> always show the fail message from thunderbird due to the Artur's old email. >>> <a.swigon@partnet.samsung.com> -> <a.swigon@samsung.com> >> >> Yeah, I received multiple bounces because of this. >> >>> On 3/26/20 11:16 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-bus.c | 142 ++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 152 insertions(+) >>>> create mode 100644 drivers/devfreq/imx-bus.c >>>> >>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig >>>> index 0b1df12e0f21..44d26192ddc4 100644 >>>> --- a/drivers/devfreq/Kconfig >>>> +++ b/drivers/devfreq/Kconfig >>>> @@ -99,10 +99,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_BUS_DEVFREQ >>>> + tristate "i.MX Generic Bus DEVFREQ Driver" >>>> + depends on ARCH_MXC || COMPILE_TEST >>>> + select DEVFREQ_GOV_PASSIVE >>>> + select DEVFREQ_GOV_USERSPACE >>> >>> Maybe, you would update it by using passive governor? >>> But, in this version, it doesn't handle the any passive governor. >> >> dropped >> >>>> + 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..3ca1ad0ecb97 100644 >>>> --- a/drivers/devfreq/Makefile >>>> +++ b/drivers/devfreq/Makefile >>>> @@ -7,10 +7,11 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o >>>> obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o >>>> obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o >>>> >>>> # DEVFREQ Drivers >>>> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o >>>> +obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ) += imx-bus.o >>> >>> The ARM_IMX_BUS_DEVFREQ config is under ARM_IMX8M_DDRC_DEVFREQ >>> and imx-bus.o is over imx8m-ddrc.o. Need to edit the sequence. >> >> Reordered kconfig to match. 8M_DDRC sorts before _BUS alphabetically but >> it's pettier this way, and matches tegra. >> >>>> obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o >>>> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o >>>> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o >>>> obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o >>>> >>>> diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c >>>> new file mode 100644 >>>> index 000000000000..285e0f1ae6b1 >>>> --- /dev/null >>>> +++ b/drivers/devfreq/imx-bus.c >>>> @@ -0,0 +1,142 @@ >>>> +// 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_bus { >>>> + struct devfreq_dev_profile profile; >>>> + struct devfreq *devfreq; >>>> + struct clk *clk; >>>> + struct devfreq_passive_data passive_data; >>> >>> This patch doesn't touch the passive_data. >> >> dropped >> >>>> +}; >>>> + >>>> +static int imx_bus_target(struct device *dev, >>>> + unsigned long *freq, u32 flags) >>>> +{ >>>> + struct imx_bus *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); >>> >>> It doesn't need. Because the new frequency is stored to 'freq' >>> by calling devfreq_recommended_opp(). >> >> fixed >> >>>> + dev_pm_opp_put(new_opp); >>>> + >>>> + return clk_set_rate(priv->clk, new_freq); >>> >>> nitpick. you can use dev_pm_opp_set_rate(). But, I'm not forcing to use it. >> >> Switched to dev_pm_opp_set_rate. >> >> It might be interesting to add regulators control later, on some chips >> the main NOC can run at different voltages. >> >>> >>>> +} >>>> + >>>> +static int imx_bus_get_cur_freq(struct device *dev, unsigned long *freq) >>>> +{ >>>> + struct imx_bus *priv = dev_get_drvdata(dev); >>>> + >>>> + *freq = clk_get_rate(priv->clk); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int imx_bus_get_dev_status(struct device *dev, >>>> + struct devfreq_dev_status *stat) >>>> +{ >>>> + struct imx_bus *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_bus_exit(struct device *dev) >>>> +{ >>>> + dev_pm_opp_of_remove_table(dev); >>>> +} >>>> + >>>> +static int imx_bus_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device *dev = &pdev->dev; >>>> + struct imx_bus *priv; >>>> + const char *gov = DEVFREQ_GOV_USERSPACE; >>>> + int ret; >>>> + >>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>>> + if (!priv) >>>> + return -ENOMEM; >>>> + >>>> + /* >>>> + * Fetch the clock to adjust but don't explictly enable. >>> >>> Need to fix typo. >>> s/explictly/explicitly >> >> fixed >> >>>> + * >>>> + * For imx bus clock clk_set_rate is safe no matter if the clock is on >>>> + * or off and some peripheral side-buses might be off unless enabled by >>>> + * drivers for devices on those specific buses. >>>> + * >>>> + * Rate adjustment on a disabled bus clock just takes effect later. >>>> + */ >>>> + 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_bus_target; >>>> + priv->profile.get_dev_status = imx_bus_get_dev_status; >>>> + priv->profile.exit = imx_bus_exit; >>>> + priv->profile.get_cur_freq = imx_bus_get_cur_freq; >>>> + priv->profile.initial_freq = clk_get_rate(priv->clk); >>>> + >>>> + priv->devfreq = devm_devfreq_add_device(dev, &priv->profile, >>>> + gov, NULL); >>>> + 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_bus_of_match[] = { >>>> + { .compatible = "fsl,imx8m-noc", }, >>>> + { .compatible = "fsl,imx8m-nic", }, >>>> + { /* sentinel */ }, >>>> +}; >>>> +MODULE_DEVICE_TABLE(of, imx_bus_of_match); >>>> + >>>> +static struct platform_driver imx_bus_platdrv = { >>>> + .probe = imx_bus_probe, >>>> + .driver = { >>>> + .name = "imx-bus-devfreq", >>>> + .of_match_table = of_match_ptr(imx_bus_of_match), >>>> + }, >>>> +}; >>>> +module_platform_driver(imx_bus_platdrv); >>>> + >>>> +MODULE_DESCRIPTION("Generic i.MX bus frequency scaling driver"); >>>> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>"); >>>> +MODULE_LICENSE("GPL v2"); >>>> >>> >>> >> >> >> >> > >
On 4/2/20 6:53 PM, Leonard Crestez wrote: > On 2020-04-02 1:48 AM, Chanwoo Choi wrote: >> On 4/1/20 11:20 PM, Leonard Crestez wrote: >>> On 2020-04-01 1:55 AM, Chanwoo Choi wrote: >>>> Hi, >>>> >>>> Looks good to me. I added the comments. >>>> But, it need to add the dt binding documentation for this device. >>> >>> DT bindings were included: >>> >>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11458981%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C7381d117a4d1468cd2c608d7d68ecfac%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637213781167514099&sdata=Qu8x14cXnuxlwOT2SlUOf%2FLgCVWbnJRKA4TBjMIWQeA%3D&reserved=0 >> >> The dt-binding document for this driver is required under >> Documentation/devicetree/binding/devfreq. > > Bindings for imx8m-ddrc were at one point posted for > devicetree/bindings/devfreq but Rob Herring suggested to move them under > "memory-controller" instead and I expect same logic makes sense here. > Link to previous discussion: > > https://patchwork.kernel.org/patch/11221919/ > > DT bindings should try to describe "hardware" rather than "drivers" and > an "interconnect" is a class of hardware while "devfreq" isn't. > > Not only that but the main noc has properties parsed by interconnect driver. OK. Thanks for reply. > >> It is difficult to catch where is the dt-binding document >> for this driver for who don't know the detailed history >> of this driver. I don't said that add the duplicate documentation >> But, at least the some document have to point out the reference. > > What I usually do to find information about a device is grep for the > compat string in the entire tree. > >>>> The old email of Artur Świgoń is not used. On next time, >>>> use following the new email address Because when I reply the mail, >>>> always show the fail message from thunderbird due to the Artur's old email. >>>> <a.swigon@partnet.samsung.com> -> <a.swigon@samsung.com> >>> >>> Yeah, I received multiple bounces because of this. >>> >>>> On 3/26/20 11:16 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-bus.c | 142 ++++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 152 insertions(+) >>>>> create mode 100644 drivers/devfreq/imx-bus.c >>>>> >>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig >>>>> index 0b1df12e0f21..44d26192ddc4 100644 >>>>> --- a/drivers/devfreq/Kconfig >>>>> +++ b/drivers/devfreq/Kconfig >>>>> @@ -99,10 +99,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_BUS_DEVFREQ >>>>> + tristate "i.MX Generic Bus DEVFREQ Driver" >>>>> + depends on ARCH_MXC || COMPILE_TEST >>>>> + select DEVFREQ_GOV_PASSIVE >>>>> + select DEVFREQ_GOV_USERSPACE >>>> >>>> Maybe, you would update it by using passive governor? >>>> But, in this version, it doesn't handle the any passive governor. >>> >>> dropped >>> >>>>> + 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..3ca1ad0ecb97 100644 >>>>> --- a/drivers/devfreq/Makefile >>>>> +++ b/drivers/devfreq/Makefile >>>>> @@ -7,10 +7,11 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o >>>>> obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o >>>>> obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o >>>>> >>>>> # DEVFREQ Drivers >>>>> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o >>>>> +obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ) += imx-bus.o >>>> >>>> The ARM_IMX_BUS_DEVFREQ config is under ARM_IMX8M_DDRC_DEVFREQ >>>> and imx-bus.o is over imx8m-ddrc.o. Need to edit the sequence. >>> >>> Reordered kconfig to match. 8M_DDRC sorts before _BUS alphabetically but >>> it's pettier this way, and matches tegra. >>> >>>>> obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o >>>>> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o >>>>> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o >>>>> obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o >>>>> >>>>> diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c >>>>> new file mode 100644 >>>>> index 000000000000..285e0f1ae6b1 >>>>> --- /dev/null >>>>> +++ b/drivers/devfreq/imx-bus.c >>>>> @@ -0,0 +1,142 @@ >>>>> +// 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_bus { >>>>> + struct devfreq_dev_profile profile; >>>>> + struct devfreq *devfreq; >>>>> + struct clk *clk; >>>>> + struct devfreq_passive_data passive_data; >>>> >>>> This patch doesn't touch the passive_data. >>> >>> dropped >>> >>>>> +}; >>>>> + >>>>> +static int imx_bus_target(struct device *dev, >>>>> + unsigned long *freq, u32 flags) >>>>> +{ >>>>> + struct imx_bus *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); >>>> >>>> It doesn't need. Because the new frequency is stored to 'freq' >>>> by calling devfreq_recommended_opp(). >>> >>> fixed >>> >>>>> + dev_pm_opp_put(new_opp); >>>>> + >>>>> + return clk_set_rate(priv->clk, new_freq); >>>> >>>> nitpick. you can use dev_pm_opp_set_rate(). But, I'm not forcing to use it. >>> >>> Switched to dev_pm_opp_set_rate. >>> >>> It might be interesting to add regulators control later, on some chips >>> the main NOC can run at different voltages. >>> >>>> >>>>> +} >>>>> + >>>>> +static int imx_bus_get_cur_freq(struct device *dev, unsigned long *freq) >>>>> +{ >>>>> + struct imx_bus *priv = dev_get_drvdata(dev); >>>>> + >>>>> + *freq = clk_get_rate(priv->clk); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int imx_bus_get_dev_status(struct device *dev, >>>>> + struct devfreq_dev_status *stat) >>>>> +{ >>>>> + struct imx_bus *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_bus_exit(struct device *dev) >>>>> +{ >>>>> + dev_pm_opp_of_remove_table(dev); >>>>> +} >>>>> + >>>>> +static int imx_bus_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct device *dev = &pdev->dev; >>>>> + struct imx_bus *priv; >>>>> + const char *gov = DEVFREQ_GOV_USERSPACE; >>>>> + int ret; >>>>> + >>>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>>>> + if (!priv) >>>>> + return -ENOMEM; >>>>> + >>>>> + /* >>>>> + * Fetch the clock to adjust but don't explictly enable. >>>> >>>> Need to fix typo. >>>> s/explictly/explicitly >>> >>> fixed >>> >>>>> + * >>>>> + * For imx bus clock clk_set_rate is safe no matter if the clock is on >>>>> + * or off and some peripheral side-buses might be off unless enabled by >>>>> + * drivers for devices on those specific buses. >>>>> + * >>>>> + * Rate adjustment on a disabled bus clock just takes effect later. >>>>> + */ >>>>> + 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_bus_target; >>>>> + priv->profile.get_dev_status = imx_bus_get_dev_status; >>>>> + priv->profile.exit = imx_bus_exit; >>>>> + priv->profile.get_cur_freq = imx_bus_get_cur_freq; >>>>> + priv->profile.initial_freq = clk_get_rate(priv->clk); >>>>> + >>>>> + priv->devfreq = devm_devfreq_add_device(dev, &priv->profile, >>>>> + gov, NULL); >>>>> + 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_bus_of_match[] = { >>>>> + { .compatible = "fsl,imx8m-noc", }, >>>>> + { .compatible = "fsl,imx8m-nic", }, >>>>> + { /* sentinel */ }, >>>>> +}; >>>>> +MODULE_DEVICE_TABLE(of, imx_bus_of_match); >>>>> + >>>>> +static struct platform_driver imx_bus_platdrv = { >>>>> + .probe = imx_bus_probe, >>>>> + .driver = { >>>>> + .name = "imx-bus-devfreq", >>>>> + .of_match_table = of_match_ptr(imx_bus_of_match), >>>>> + }, >>>>> +}; >>>>> +module_platform_driver(imx_bus_platdrv); >>>>> + >>>>> +MODULE_DESCRIPTION("Generic i.MX bus frequency scaling driver"); >>>>> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>"); >>>>> +MODULE_LICENSE("GPL v2"); >>>>> >>>> >>>> >>> >>> >>> >>> >> >> > > >