| Message ID | cover.1572562150.git.leonard.crestez@nxp.com |
|---|---|
| Headers | show |
| Series | interconnect: Add imx support via devfreq | expand |
Hi Leonard, Thanks for the patch! On 1.11.19 г. 0:52 ч., 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 adjustable nodes are found based on an "interconnect-node-id" > property by scanning the entire device tree. Are the adjustable nodes SoC specific? Can we have them here in the driver instead of scanning the entire device tree? > The interconnect provider doesn't need an virtual OF node, instead those > same adjustable nodes are registered as proxies which xlate to the > platform-level provider. > > The platform device for the interconnect needs to be registered from a > SOC driver (similar to cpufreq). > > 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 | 273 ++++++++++++++++++++++++++++++ > drivers/interconnect/imx/imx.h | 60 +++++++ > 6 files changed, 341 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 b6ea8f0a6122..f57e77b8731c 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..7d248e01dcf0 > --- /dev/null > +++ b/drivers/interconnect/imx/imx.c > @@ -0,0 +1,273 @@ > +// 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/devfreq.h> > +#include <linux/device.h> > +#include <linux/interconnect-provider.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pm_qos.h> > + > +#include "imx.h" > + > +/* private icc_provider data */ > +struct imx_icc_provider { > + struct device *dev; What device is this? There is already a *dev in struct icc_provider. Please add kernel-doc. > +}; > + > +/* private icc_node data */ > +struct imx_icc_node { > + const struct imx_icc_node_desc *desc; > + struct devfreq *devfreq; > + 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 struct icc_node *imx_icc_xlate(struct of_phandle_args *spec, void *data) > +{ > + struct imx_icc_provider *desc = data; > + struct icc_provider *provider = dev_get_drvdata(desc->dev); > + unsigned int id = spec->args[0]; > + struct icc_node *node; > + > + list_for_each_entry(node, &provider->nodes, node_list) > + if (node->id == id) > + return node; > + > + return ERR_PTR(-EINVAL); > +} > + > +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->devfreq) > + return 0; > + > + freq = (node->avg_bw + node->peak_bw) * node_data->desc->adj->bw_mul; Why the sum of average and peak bandwidth? > + 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->devfreq->dev.parent), > + node->avg_bw, node->peak_bw, freq); > + > + 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); > +} > + > +static int imx_icc_node_init_devfreq(struct device *dev, > + struct icc_node *node) > +{ > + struct imx_icc_node *node_data = node->data; > + struct device_node *dn; > + u32 node_id; > + int ret; > + > + /* Find nodes based on interconnect-node-id property */ > + for_each_node_with_property(dn, "interconnect-node-id") { > + ret = of_property_read_u32(dn, "interconnect-node-id", > + &node_id); > + if (ret != 0) > + continue; > + > + if (node_id == node->id) { > + of_node_get(dn); > + break; > + } > + } > + > + if (!dn) > + return 0; > + > + dev_info(dev, "node %s[%d] has device node %pOF\n", > + node->name, node->id, dn); > + node_data->devfreq = devfreq_get_devfreq_by_node(dn); Ah, so you need to get the devfreq nodes? So looking at the next patches it seems that noc and ddrc are the only adjustable nodes? Maybe we should model them both as interconnect providers, as they seem to be dealing with the bandwidth/frequency requirements and changing the clock rates. Thanks, Georgi > + if (IS_ERR(node_data->devfreq)) { > + of_node_put(dn); > + ret = PTR_ERR(node_data->devfreq); > + dev_err(dev, "failed to fetch devfreq for %s: %d\n", > + node->name, ret); > + return ret; > + } > + > + of_node_put(dn); > + > + return dev_pm_qos_add_request(node_data->devfreq->dev.parent, > + &node_data->qos_req, > + DEV_PM_QOS_MIN_FREQUENCY, 0); > +} > +
On 12.11.2019 17:08, Georgi Djakov wrote: > Hi Leonard, > > Thanks for the patch! > > On 1.11.19 г. 0:52 ч., 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 adjustable nodes are found based on an "interconnect-node-id" >> property by scanning the entire device tree. > > Are the adjustable nodes SoC specific? Can we have them here in the driver > instead of scanning the entire device tree? > >> The interconnect provider doesn't need an virtual OF node, instead those >> same adjustable nodes are registered as proxies which xlate to the >> platform-level provider. >> >> The platform device for the interconnect needs to be registered from a >> SOC driver (similar to cpufreq). >> >> 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 | 273 ++++++++++++++++++++++++++++++ >> drivers/interconnect/imx/imx.h | 60 +++++++ >> 6 files changed, 341 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 b6ea8f0a6122..f57e77b8731c 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..7d248e01dcf0 >> --- /dev/null >> +++ b/drivers/interconnect/imx/imx.c >> @@ -0,0 +1,273 @@ >> +// 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/devfreq.h> >> +#include <linux/device.h> >> +#include <linux/interconnect-provider.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_qos.h> >> + >> +#include "imx.h" >> + >> +/* private icc_provider data */ >> +struct imx_icc_provider { >> + struct device *dev; > > What device is this? There is already a *dev in struct icc_provider. > Please add kernel-doc. This is set as icc_provider->data and used for xlate but it can be removed entirely if driver switches to icc_xlate_onecell. >> +/* private icc_node data */ >> +struct imx_icc_node { >> + const struct imx_icc_node_desc *desc; >> + struct devfreq *devfreq; >> + 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 struct icc_node *imx_icc_xlate(struct of_phandle_args *spec, void *data) >> +{ >> + struct imx_icc_provider *desc = data; >> + struct icc_provider *provider = dev_get_drvdata(desc->dev); >> + unsigned int id = spec->args[0]; >> + struct icc_node *node; >> + >> + list_for_each_entry(node, &provider->nodes, node_list) >> + if (node->id == id) >> + return node; >> + >> + return ERR_PTR(-EINVAL); >> +} >> + >> +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->devfreq) >> + return 0; >> + >> + freq = (node->avg_bw + node->peak_bw) * node_data->desc->adj->bw_mul; > > Why the sum of average and peak bandwidth? What else? Averages from all path requests are added and the max() of all per-path peak requests is added on top. The average BW is guaranteed to be consumed (for example a display controller) and the BW for occasional bursts (such as USB) should be reserved on top of that. Using max(avg, peak) here would make requests for peak BW largely ineffective when there's also a large "avg" request. >> + 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->devfreq->dev.parent), >> + node->avg_bw, node->peak_bw, freq); >> + >> + 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); >> +} >> + >> +static int imx_icc_node_init_devfreq(struct device *dev, >> + struct icc_node *node) >> +{ >> + struct imx_icc_node *node_data = node->data; >> + struct device_node *dn; >> + u32 node_id; >> + int ret; >> + >> + /* Find nodes based on interconnect-node-id property */ >> + for_each_node_with_property(dn, "interconnect-node-id") { >> + ret = of_property_read_u32(dn, "interconnect-node-id", >> + &node_id); >> + if (ret != 0) >> + continue; >> + >> + if (node_id == node->id) { >> + of_node_get(dn); >> + break; >> + } >> + } >> + >> + if (!dn) >> + return 0; >> + >> + dev_info(dev, "node %s[%d] has device node %pOF\n", >> + node->name, node->id, dn); >> + node_data->devfreq = devfreq_get_devfreq_by_node(dn); > > Ah, so you need to get the devfreq nodes? So looking at the next > patches it seems that noc and ddrc are the only adjustable nodes? I want to add more scalable nodes in the future. imx8mq/8mm/8mn have additional NICs (AXI switches with a different implementation) and upcoming imx8mp will have additional NOCs and scalable NTTP links. > Maybe we should model them both as interconnect providers, as they > seem to be dealing with the bandwidth/frequency requirements and > changing the clock rates. The ddrc is not really an interconnect, it only has an AXI slave port. In theory it could register itself as a single-node icc provider solely so that it can handle the "set" call but that seems hacky. Current icc driver implementations send aggregated BW requests to a separate controller and internally it changes some frequencies, correct? What I'm doing is implementing the frequency adjustment part inside the kernel as devfreq drivers. These devices have their own OF nodes with OPP tables and supply regulators and governors (based on bandwith measurement). It makes a lot of sense to layer the "interconnect" on top of "scalable nodes". -- Regards, Leonard
This series adds upstream DRAM and 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 (NIC/NOC and DDRC) is implemented through devfreq. The imx interconnect provider doesn't communicate with devfreq instance directly but rather computes "minimum frequencies" for nodes along the path and creates dev_pm_qos requests. This needs core changes for dev_pm_qos frequency support for devfreq: https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=196443 Since imx interconnect only needs to interact with imx devfreq it would also be possible to tie them together directly, but it would be a little sad. 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 is a bit odd. On i.MX there are multiple instances of a few interconnect IPs, this patch identifies them by enumerating the entire devicetree scanning for the "interconnect-node-id" property. As far as I see current providers do this based entirely on compatible string of the interconnect instance and this seems very odd to me. Do you think we could move this "interconnect-node-id" approach to core interconnect bindings? The same interconnect-node-id property is used to identify the DDRC and this is arguably a hack. Also as a github branch (with few other changes): https://github.com/cdleonard/linux/tree/next_imx_busfreq This is layered upon imx-ddrc scaling support: * https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=196459 The DDRC only really interacts by receiving dev_pm_qos requests. 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/patch/11056789/ Leonard Crestez (10): dt-bindings: devfreq: Add bindings for generic imx buses PM / devfreq: Add generic imx bus driver PM / devfreq: imx: Register interconnect device PM / devfreq: Add devfreq_get_devfreq_by_node 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 .../devicetree/bindings/devfreq/imx.yaml | 83 ++++++ arch/arm64/boot/dts/freescale/imx8mm.dtsi | 26 ++ arch/arm64/boot/dts/freescale/imx8mn.dtsi | 26 ++ arch/arm64/boot/dts/freescale/imx8mq.dtsi | 26 ++ drivers/devfreq/Kconfig | 12 + drivers/devfreq/Makefile | 1 + drivers/devfreq/devfreq.c | 42 ++- drivers/devfreq/imx-devfreq.c | 185 ++++++++++++ drivers/interconnect/Kconfig | 1 + drivers/interconnect/Makefile | 1 + drivers/interconnect/imx/Kconfig | 17 ++ drivers/interconnect/imx/Makefile | 4 + drivers/interconnect/imx/imx.c | 273 ++++++++++++++++++ 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 +++ include/linux/devfreq.h | 1 + 21 files changed, 1187 insertions(+), 11 deletions(-) create mode 100644 Documentation/devicetree/bindings/devfreq/imx.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