| Message ID | cover.1585751281.git.leonard.crestez@nxp.com |
|---|---|
| Headers | show |
| Series | interconnect: Add imx support via devfreq | expand |
Hi Leonard, As I replied on v1 mail thread, it needs a dt-binding document under Documentation/devicetree/bindings/devfreq. And I add one comment below. Except for them, looks good to me. On 4/1/20 11:33 PM, 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> > Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm> > --- > drivers/devfreq/Kconfig | 8 +++ > drivers/devfreq/Makefile | 1 + > drivers/devfreq/imx-bus.c | 139 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 148 insertions(+) > create mode 100644 drivers/devfreq/imx-bus.c > > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig > index 0b1df12e0f21..37dc40d1fcfb 100644 > --- a/drivers/devfreq/Kconfig > +++ b/drivers/devfreq/Kconfig > @@ -89,10 +89,18 @@ config ARM_EXYNOS_BUS_DEVFREQ > Each memory bus group could contain many memoby bus block. It reads > PPMU counters of memory controllers by using DEVFREQ-event device > and adjusts the operating frequencies and voltages with OPP support. > This does not yet operate with optimal voltages. > > +config ARM_IMX_BUS_DEVFREQ > + tristate "i.MX Generic Bus DEVFREQ Driver" > + depends on ARCH_MXC || COMPILE_TEST > + select DEVFREQ_GOV_USERSPACE > + help > + This adds the generic DEVFREQ driver for i.MX interconnects. It > + allows adjusting NIC/NOC frequency. > + > config ARM_IMX8M_DDRC_DEVFREQ > tristate "i.MX8M DDRC DEVFREQ Driver" > depends on (ARCH_MXC && HAVE_ARM_SMCCC) || \ > (COMPILE_TEST && HAVE_ARM_SMCCC) > select DEVFREQ_GOV_SIMPLE_ONDEMAND > 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 > 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..7915d7277349 > --- /dev/null > +++ b/drivers/devfreq/imx-bus.c > @@ -0,0 +1,139 @@ > +// 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; > +}; > + > +static int imx_bus_target(struct device *dev, > + unsigned long *freq, u32 flags) > +{ > + struct imx_bus *priv = dev_get_drvdata(dev); It is not used. Need to remove it. > + struct dev_pm_opp *new_opp; > + int ret; > + > + new_opp = devfreq_recommended_opp(dev, freq, flags); > + if (IS_ERR(new_opp)) { > + ret = PTR_ERR(new_opp); > + dev_err(dev, "failed to get recommended opp: %d\n", ret); > + return ret; > + } > + dev_pm_opp_put(new_opp); > + > + return dev_pm_opp_set_rate(dev, *freq); > +} > + > +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 explicitly enable. > + * > + * 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 4/1/20 11:33 PM, 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> > Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm> > --- > drivers/devfreq/imx-bus.c | 41 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c > index 7915d7277349..240eeea66f13 100644 > --- a/drivers/devfreq/imx-bus.c > +++ b/drivers/devfreq/imx-bus.c > @@ -14,10 +14,11 @@ > > struct imx_bus { > struct devfreq_dev_profile profile; > struct devfreq *devfreq; > struct clk *clk; > + struct platform_device *icc_pdev; > }; > > static int imx_bus_target(struct device *dev, > unsigned long *freq, u32 flags) > { > @@ -57,11 +58,44 @@ 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 */ > +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) { > + dev_err(dev, "unknown interconnect driver\n"); > + return 0; > + } > + > + 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; > @@ -109,18 +143,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) > + 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); > Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
Hi Leonard, Thank you for updating the patches! On 4/1/20 17:33, Leonard Crestez wrote: > This adds support for i.MX SoC family to interconnect framework. > > Platform drivers can describe the interconnect graph and several > adjustment knobs where icc node bandwidth is converted to a > DEV_PM_QOS_MIN_FREQUENCY request. > > The interconnect provider is probed through the main NOC device and > other adjustable nodes on the same graph are found from a > fsl,scalable-nodes phandle array property. > > Signed-off-by: Alexandre Bailon <abailon@baylibre.com> > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm> > --- > drivers/interconnect/Kconfig | 1 + > drivers/interconnect/Makefile | 1 + > drivers/interconnect/imx/Kconfig | 5 + > drivers/interconnect/imx/Makefile | 3 + > drivers/interconnect/imx/imx.c | 298 ++++++++++++++++++++++++++++++ > drivers/interconnect/imx/imx.h | 62 +++++++ > 6 files changed, 370 insertions(+) > create mode 100644 drivers/interconnect/imx/Kconfig > create mode 100644 drivers/interconnect/imx/Makefile > create mode 100644 drivers/interconnect/imx/imx.c > create mode 100644 drivers/interconnect/imx/imx.h > > diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig > index bfa4ca3ab7a9..e61802230f90 100644 > --- a/drivers/interconnect/Kconfig > +++ b/drivers/interconnect/Kconfig > @@ -10,7 +10,8 @@ menuconfig INTERCONNECT > If unsure, say no. > > if INTERCONNECT > > source "drivers/interconnect/qcom/Kconfig" > +source "drivers/interconnect/imx/Kconfig" Nit: Please move it up to make it sorted. > > endif > diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile > index 725029ae7a2c..6998288a7d98 100644 > --- a/drivers/interconnect/Makefile > +++ b/drivers/interconnect/Makefile > @@ -3,5 +3,6 @@ > CFLAGS_core.o := -I$(src) > icc-core-objs := core.o > > obj-$(CONFIG_INTERCONNECT) += icc-core.o > obj-$(CONFIG_INTERCONNECT_QCOM) += qcom/ > +obj-$(CONFIG_INTERCONNECT_IMX) += imx/ Ditto. > diff --git a/drivers/interconnect/imx/Kconfig b/drivers/interconnect/imx/Kconfig > new file mode 100644 > index 000000000000..f39336f8d603 > --- /dev/null > +++ b/drivers/interconnect/imx/Kconfig > @@ -0,0 +1,5 @@ > +config INTERCONNECT_IMX > + tristate "i.MX interconnect drivers" > + depends on ARCH_MXC || COMPILE_TEST > + help > + Generic interconnect drivers for i.MX SOCs > diff --git a/drivers/interconnect/imx/Makefile b/drivers/interconnect/imx/Makefile > new file mode 100644 > index 000000000000..86ae0bd28d8c > --- /dev/null > +++ b/drivers/interconnect/imx/Makefile > @@ -0,0 +1,3 @@ > +imx-interconnect-objs := imx.o > + > +obj-$(CONFIG_INTERCONNECT_IMX) += imx-interconnect.o > diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c > new file mode 100644 > index 000000000000..527b1de1c41a > --- /dev/null > +++ b/drivers/interconnect/imx/imx.c > @@ -0,0 +1,298 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Interconnect framework driver for i.MX SoC > + * > + * Copyright (c) 2019, BayLibre > + * Copyright (c) 2019, NXP Maybe update it to 2020. > + * Author: Alexandre Bailon <abailon@baylibre.com> > + * Author: Leonard Crestez <leonard.crestez@nxp.com> > + */ > + > +#include <linux/device.h> > +#include <linux/interconnect-provider.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/pm_qos.h> > + > +#include "imx.h" > + > +/* private icc_node data */ > +struct imx_icc_node { > + const struct imx_icc_node_desc *desc; > + struct device *qos_dev; > + struct dev_pm_qos_request qos_req; > +}; > + > +static int imx_icc_aggregate(struct icc_node *node, u32 tag, > + u32 avg_bw, u32 peak_bw, > + u32 *agg_avg, u32 *agg_peak) > +{ > + *agg_avg += avg_bw; > + *agg_peak = max(*agg_peak, peak_bw); > + > + return 0; > +} Please remove and use the common library function: icc_std_aggregate(). > + > +static int imx_icc_node_set(struct icc_node *node) > +{ > + struct device *dev = node->provider->dev; > + struct imx_icc_node *node_data = node->data; > + u64 freq; > + > + if (!node_data->qos_dev) > + return 0; > + > + freq = (node->avg_bw + node->peak_bw) * node_data->desc->adj->bw_mul; > + do_div(freq, node_data->desc->adj->bw_div); > + dev_dbg(dev, "node %s device %s avg_bw %ukBps peak_bw %ukBps min_freq %llukHz\n", > + node->name, dev_name(node_data->qos_dev), > + node->avg_bw, node->peak_bw, freq); > + > + if (freq > S32_MAX) { > + dev_err(dev, "%s can't request more than S32_MAX freq\n", > + node->name); Please align with the open parenthesis. > + return -ERANGE; > + } > + > + dev_pm_qos_update_request(&node_data->qos_req, freq); > + > + return 0; > +} > + > +static int imx_icc_set(struct icc_node *src, struct icc_node *dst) > +{ > + return imx_icc_node_set(dst); > +} > + > +/* imx_icc_node_destroy() - Destroy an imx icc_node, including private data */ > +static void imx_icc_node_destroy(struct icc_node *node) > +{ > + struct imx_icc_node *node_data = node->data; > + int ret; > + > + if (dev_pm_qos_request_active(&node_data->qos_req)) { > + ret = dev_pm_qos_remove_request(&node_data->qos_req); > + if (ret) > + dev_warn(node->provider->dev, "failed to remove qos request for %s\n", > + dev_name(node_data->qos_dev)); > + } > + > + put_device(node_data->qos_dev); > + icc_node_del(node); > + icc_node_destroy(node->id); > +} > + > +static int imx_icc_node_init_qos(struct icc_provider *provider, > + struct icc_node *node) > +{ > + struct imx_icc_node *node_data = node->data; > + const struct imx_icc_node_adj_desc *adj = node_data->desc->adj; > + struct device *dev = provider->dev; > + struct device_node *dn = NULL; > + struct platform_device *pdev; > + > + if (adj->main_noc) { > + node_data->qos_dev = dev; > + dev_info(dev, "icc node %s[%d] is main noc itself\n", > + node->name, node->id); Should this be dev_dbg()? > + } else { > + dn = of_parse_phandle(dev->of_node, adj->phandle_name, 0); > + if (IS_ERR(dn)) { > + dev_warn(dev, "Failed to parse %s: %ld\n", > + adj->phandle_name, PTR_ERR(dn)); Please align with the open parenthesis. > + return PTR_ERR(dn); > + } > + /* Allow scaling to be disabled on a per-node basis */ > + if (!dn || !of_device_is_available(dn)) { > + dev_warn(dev, "Missing property %s, skip scaling %s\n", > + adj->phandle_name, node->name); Please align with the open parenthesis. > + return 0; > + } > + > + pdev = of_find_device_by_node(dn); > + of_node_put(dn); > + if (!pdev) { > + dev_warn(dev, "node %s[%d] missing device for %pOF\n", > + node->name, node->id, dn); Please align with the open parenthesis. > + return -EPROBE_DEFER; > + } > + node_data->qos_dev = &pdev->dev; > + dev_info(dev, "node %s[%d] has device node %pOF\n", > + node->name, node->id, dn); dev_dbg() again maybe? Unless you think that it make sense to print this information to the user? > + } > + > + return dev_pm_qos_add_request(node_data->qos_dev, > + &node_data->qos_req, > + DEV_PM_QOS_MIN_FREQUENCY, 0); > +} > + > +static struct icc_node *imx_icc_node_add(struct icc_provider *provider, > + const struct imx_icc_node_desc *node_desc) > +{ > + struct device *dev = provider->dev; > + struct imx_icc_node *node_data; > + struct icc_node *node; > + int ret; > + > + node = icc_node_create(node_desc->id); > + if (IS_ERR(node)) { > + dev_err(dev, "failed to create node %d\n", node_desc->id); > + return node; > + } > + > + if (node->data) { > + dev_err(dev, "already created node %s id=%d\n", > + node_desc->name, node_desc->id); Please align with the open parenthesis. > + return ERR_PTR(-EEXIST); > + } > + > + node_data = devm_kzalloc(dev, sizeof(*node_data), GFP_KERNEL); > + if (!node_data) { > + icc_node_destroy(node->id); > + return ERR_PTR(-ENOMEM); > + } > + > + node->name = node_desc->name; > + node->data = node_data; > + node_data->desc = node_desc; > + icc_node_add(node, provider); > + > + if (node_desc->adj) { > + ret = imx_icc_node_init_qos(provider, node); > + if (ret < 0) { > + imx_icc_node_destroy(node); > + return ERR_PTR(ret); > + } > + } > + > + return node; > +} > + > +static void imx_icc_unregister_nodes(struct icc_provider *provider) > +{ > + struct icc_node *node, *tmp; > + > + list_for_each_entry_safe(node, tmp, &provider->nodes, node_list) > + imx_icc_node_destroy(node); > +} > + > +static int imx_icc_register_nodes(struct icc_provider *provider, > + const struct imx_icc_node_desc *descs, > + int count) > +{ > + struct icc_onecell_data *provider_data = provider->data; > + int ret; > + int i; > + > + for (i = 0; i < count; i++) { > + struct icc_node *node; > + const struct imx_icc_node_desc *node_desc = &descs[i]; > + size_t j; > + > + node = imx_icc_node_add(provider, node_desc); > + if (IS_ERR(node)) { > + ret = PTR_ERR(node); > + if (ret != -EPROBE_DEFER) > + dev_err(provider->dev, "failed to add %s: %d\n", > + node_desc->name, ret); > + goto err; > + } > + provider_data->nodes[node->id] = node; > + > + for (j = 0; j < node_desc->num_links; j++) { > + ret = icc_link_create(node, node_desc->links[j]); > + if (ret) { > + dev_err(provider->dev, "failed to link node %d to %d: %d\n", > + node->id, node_desc->links[j], ret); > + goto err; > + } > + } > + } > + > + return 0; > + > +err: > + imx_icc_unregister_nodes(provider); > + > + return ret; > +} > + > +static int get_max_node_id(struct imx_icc_node_desc *nodes, int nodes_count) > +{ > + int i, ret = 0; > + > + for (i = 0; i < nodes_count; ++i) > + if (nodes[i].id > ret) > + ret = nodes[i].id; > + > + return ret; > +} > + > +int imx_icc_register(struct platform_device *pdev, > + struct imx_icc_node_desc *nodes, int nodes_count) > +{ > + struct device *dev = &pdev->dev; > + struct icc_onecell_data *data; > + struct icc_provider *provider; > + int max_node_id; > + int ret; > + > + /* icc_onecell_data is indexed by node_id, unlike nodes param */ > + max_node_id = get_max_node_id(nodes, nodes_count); > + data = devm_kzalloc(dev, struct_size(data, nodes, max_node_id), > + GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + data->num_nodes = max_node_id; > + > + provider = devm_kzalloc(dev, sizeof(*provider), GFP_KERNEL); > + if (!provider) > + return -ENOMEM; > + provider->set = imx_icc_set; > + provider->aggregate = imx_icc_aggregate; provider->aggregate = icc_std_aggregate; > + provider->xlate = of_icc_xlate_onecell; > + provider->data = data; > + provider->dev = dev->parent; > + platform_set_drvdata(pdev, provider); > + > + ret = icc_provider_add(provider); > + if (ret) { > + dev_err(dev, "error adding interconnect provider: %d\n", ret); > + return ret; > + } > + > + ret = imx_icc_register_nodes(provider, nodes, nodes_count); > + if (ret) > + goto provider_del; > + > + return 0; > + > +provider_del: > + icc_provider_del(provider); > + return ret; > +} > +EXPORT_SYMBOL_GPL(imx_icc_register); > + > +int imx_icc_unregister(struct platform_device *pdev) > +{ > + struct icc_provider *provider = platform_get_drvdata(pdev); > + int ret; > + > + if (provider->users) { > + dev_warn(&pdev->dev, "interconnect provider still has %d users\n", > + provider->users); > + return -EBUSY; > + } The above check already exists in icc_provider_del(). But i assume you don't want to delete any nodes from the provider if it still has users. Maybe it will be sensible to add this check into icc_nodes_remove() instead, so that such cases are handled on a framework level. > + imx_icc_unregister_nodes(provider); > + > + ret = icc_provider_del(provider); > + if (ret) > + return ret; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(imx_icc_unregister); > + > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/interconnect/imx/imx.h b/drivers/interconnect/imx/imx.h > new file mode 100644 > index 000000000000..aa811e4ebb7e > --- /dev/null > +++ b/drivers/interconnect/imx/imx.h > @@ -0,0 +1,62 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Interconnect framework driver for i.MX SoC > + * > + * Copyright (c) 2019, BayLibre > + * Copyright (c) 2019, NXP > + * Author: Alexandre Bailon <abailon@baylibre.com> > + * Author: Leonard Crestez <leonard.crestez@nxp.com> > + */ > +#ifndef __DRIVERS_INTERCONNECT_IMX_H > +#define __DRIVERS_INTERCONNECT_IMX_H > + > +#include <linux/kernel.h> > + > +#define IMX_ICC_MAX_LINKS 4 > + > +/* > + * struct imx_icc_node_adj - Describe a dynamic adjustable node > + */ > +struct imx_icc_node_adj_desc { > + unsigned int bw_mul, bw_div; > + const char *phandle_name; > + bool main_noc; > +}; > + > +/* > + * struct imx_icc_node - Describe an interconnect node > + * @name: name of the node > + * @id: an unique id to identify the node > + * @links: an array of slaves' node id > + * @num_links: number of id defined in links > + */ > +struct imx_icc_node_desc { > + const char *name; > + u16 id; > + u16 links[IMX_ICC_MAX_LINKS]; > + u16 num_links; > + Why the blank line? > + const struct imx_icc_node_adj_desc *adj; > +}; > + > +#define DEFINE_BUS_INTERCONNECT(_name, _id, _adj, ...) \ > + { \ > + .id = _id, \ > + .name = _name, \ > + .adj = _adj, \ > + .num_links = ARRAY_SIZE(((int[]){ __VA_ARGS__ })), \ > + .links = { __VA_ARGS__ }, \ > + } > + > +#define DEFINE_BUS_MASTER(_name, _id, _dest_id) \ > + DEFINE_BUS_INTERCONNECT(_name, _id, NULL, _dest_id) > + > +#define DEFINE_BUS_SLAVE(_name, _id, _adj) \ > + DEFINE_BUS_INTERCONNECT(_name, _id, _adj) > + > +int imx_icc_register(struct platform_device *pdev, > + struct imx_icc_node_desc *nodes, > + int nodes_count); > +int imx_icc_unregister(struct platform_device *pdev); > + > +#endif /* __DRIVERS_INTERCONNECT_IMX_H */ > Thanks, Georgi
On 2020-04-02 2:06 PM, Georgi Djakov wrote: > Hi Leonard, > > Thank you for updating the patches! > > On 4/1/20 17:33, Leonard Crestez wrote: >> This adds support for i.MX SoC family to interconnect framework. >> >> Platform drivers can describe the interconnect graph and several >> adjustment knobs where icc node bandwidth is converted to a >> DEV_PM_QOS_MIN_FREQUENCY request. >> >> The interconnect provider is probed through the main NOC device and >> other adjustable nodes on the same graph are found from a >> fsl,scalable-nodes phandle array property. >> >> Signed-off-by: Alexandre Bailon <abailon@baylibre.com> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm> >> --- >> drivers/interconnect/Kconfig | 1 + >> drivers/interconnect/Makefile | 1 + >> drivers/interconnect/imx/Kconfig | 5 + >> drivers/interconnect/imx/Makefile | 3 + >> drivers/interconnect/imx/imx.c | 298 ++++++++++++++++++++++++++++++ >> drivers/interconnect/imx/imx.h | 62 +++++++ >> 6 files changed, 370 insertions(+) >> create mode 100644 drivers/interconnect/imx/Kconfig >> create mode 100644 drivers/interconnect/imx/Makefile >> create mode 100644 drivers/interconnect/imx/imx.c >> create mode 100644 drivers/interconnect/imx/imx.h >> >> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig >> index bfa4ca3ab7a9..e61802230f90 100644 >> --- a/drivers/interconnect/Kconfig >> +++ b/drivers/interconnect/Kconfig >> @@ -10,7 +10,8 @@ menuconfig INTERCONNECT >> If unsure, say no. >> >> if INTERCONNECT >> >> source "drivers/interconnect/qcom/Kconfig" >> +source "drivers/interconnect/imx/Kconfig" > > Nit: Please move it up to make it sorted. > >> >> endif >> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile >> index 725029ae7a2c..6998288a7d98 100644 >> --- a/drivers/interconnect/Makefile >> +++ b/drivers/interconnect/Makefile >> @@ -3,5 +3,6 @@ >> CFLAGS_core.o := -I$(src) >> icc-core-objs := core.o >> >> obj-$(CONFIG_INTERCONNECT) += icc-core.o >> obj-$(CONFIG_INTERCONNECT_QCOM) += qcom/ >> +obj-$(CONFIG_INTERCONNECT_IMX) += imx/ > > Ditto. > >> diff --git a/drivers/interconnect/imx/Kconfig b/drivers/interconnect/imx/Kconfig >> new file mode 100644 >> index 000000000000..f39336f8d603 >> --- /dev/null >> +++ b/drivers/interconnect/imx/Kconfig >> @@ -0,0 +1,5 @@ >> +config INTERCONNECT_IMX >> + tristate "i.MX interconnect drivers" >> + depends on ARCH_MXC || COMPILE_TEST >> + help >> + Generic interconnect drivers for i.MX SOCs >> diff --git a/drivers/interconnect/imx/Makefile b/drivers/interconnect/imx/Makefile >> new file mode 100644 >> index 000000000000..86ae0bd28d8c >> --- /dev/null >> +++ b/drivers/interconnect/imx/Makefile >> @@ -0,0 +1,3 @@ >> +imx-interconnect-objs := imx.o >> + >> +obj-$(CONFIG_INTERCONNECT_IMX) += imx-interconnect.o >> diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c >> new file mode 100644 >> index 000000000000..527b1de1c41a >> --- /dev/null >> +++ b/drivers/interconnect/imx/imx.c >> @@ -0,0 +1,298 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Interconnect framework driver for i.MX SoC >> + * >> + * Copyright (c) 2019, BayLibre >> + * Copyright (c) 2019, NXP > > Maybe update it to 2020. > >> + * Author: Alexandre Bailon <abailon@baylibre.com> >> + * Author: Leonard Crestez <leonard.crestez@nxp.com> >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/interconnect-provider.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_qos.h> >> + >> +#include "imx.h" >> + >> +/* private icc_node data */ >> +struct imx_icc_node { >> + const struct imx_icc_node_desc *desc; >> + struct device *qos_dev; >> + struct dev_pm_qos_request qos_req; >> +}; >> + >> +static int imx_icc_aggregate(struct icc_node *node, u32 tag, >> + u32 avg_bw, u32 peak_bw, >> + u32 *agg_avg, u32 *agg_peak) >> +{ >> + *agg_avg += avg_bw; >> + *agg_peak = max(*agg_peak, peak_bw); >> + >> + return 0; >> +} > > Please remove and use the common library function: > icc_std_aggregate(). OK >> + >> +static int imx_icc_node_set(struct icc_node *node) >> +{ >> + struct device *dev = node->provider->dev; >> + struct imx_icc_node *node_data = node->data; >> + u64 freq; >> + >> + if (!node_data->qos_dev) >> + return 0; >> + >> + freq = (node->avg_bw + node->peak_bw) * node_data->desc->adj->bw_mul; >> + do_div(freq, node_data->desc->adj->bw_div); >> + dev_dbg(dev, "node %s device %s avg_bw %ukBps peak_bw %ukBps min_freq %llukHz\n", >> + node->name, dev_name(node_data->qos_dev), >> + node->avg_bw, node->peak_bw, freq); >> + >> + if (freq > S32_MAX) { >> + dev_err(dev, "%s can't request more than S32_MAX freq\n", >> + node->name); > > Please align with the open parenthesis. Fixed all instances. >> + return -ERANGE; >> + } >> + >> + dev_pm_qos_update_request(&node_data->qos_req, freq); >> + >> + return 0; >> +} >> + >> +static int imx_icc_set(struct icc_node *src, struct icc_node *dst) >> +{ >> + return imx_icc_node_set(dst); >> +} >> + >> +/* imx_icc_node_destroy() - Destroy an imx icc_node, including private data */ >> +static void imx_icc_node_destroy(struct icc_node *node) >> +{ >> + struct imx_icc_node *node_data = node->data; >> + int ret; >> + >> + if (dev_pm_qos_request_active(&node_data->qos_req)) { >> + ret = dev_pm_qos_remove_request(&node_data->qos_req); >> + if (ret) >> + dev_warn(node->provider->dev, "failed to remove qos request for %s\n", >> + dev_name(node_data->qos_dev)); >> + } >> + >> + put_device(node_data->qos_dev); >> + icc_node_del(node); >> + icc_node_destroy(node->id); >> +} >> + >> +static int imx_icc_node_init_qos(struct icc_provider *provider, >> + struct icc_node *node) >> +{ >> + struct imx_icc_node *node_data = node->data; >> + const struct imx_icc_node_adj_desc *adj = node_data->desc->adj; >> + struct device *dev = provider->dev; >> + struct device_node *dn = NULL; >> + struct platform_device *pdev; >> + >> + if (adj->main_noc) { >> + node_data->qos_dev = dev; >> + dev_info(dev, "icc node %s[%d] is main noc itself\n", >> + node->name, node->id); > > Should this be dev_dbg()? > >> + } else { >> + dn = of_parse_phandle(dev->of_node, adj->phandle_name, 0); >> + if (IS_ERR(dn)) { >> + dev_warn(dev, "Failed to parse %s: %ld\n", >> + adj->phandle_name, PTR_ERR(dn)); > > Please align with the open parenthesis. > >> + return PTR_ERR(dn); >> + } >> + /* Allow scaling to be disabled on a per-node basis */ >> + if (!dn || !of_device_is_available(dn)) { >> + dev_warn(dev, "Missing property %s, skip scaling %s\n", >> + adj->phandle_name, node->name); > > Please align with the open parenthesis. > >> + return 0; >> + } >> + >> + pdev = of_find_device_by_node(dn); >> + of_node_put(dn); >> + if (!pdev) { >> + dev_warn(dev, "node %s[%d] missing device for %pOF\n", >> + node->name, node->id, dn); > > Please align with the open parenthesis. > >> + return -EPROBE_DEFER; >> + } >> + node_data->qos_dev = &pdev->dev; >> + dev_info(dev, "node %s[%d] has device node %pOF\n", >> + node->name, node->id, dn); > > dev_dbg() again maybe? Unless you think that it make sense to print this > information to the user? > >> + } >> + >> + return dev_pm_qos_add_request(node_data->qos_dev, >> + &node_data->qos_req, >> + DEV_PM_QOS_MIN_FREQUENCY, 0); >> +} >> + >> +static struct icc_node *imx_icc_node_add(struct icc_provider *provider, >> + const struct imx_icc_node_desc *node_desc) >> +{ >> + struct device *dev = provider->dev; >> + struct imx_icc_node *node_data; >> + struct icc_node *node; >> + int ret; >> + >> + node = icc_node_create(node_desc->id); >> + if (IS_ERR(node)) { >> + dev_err(dev, "failed to create node %d\n", node_desc->id); >> + return node; >> + } >> + >> + if (node->data) { >> + dev_err(dev, "already created node %s id=%d\n", >> + node_desc->name, node_desc->id); > > Please align with the open parenthesis. > >> + return ERR_PTR(-EEXIST); >> + } >> + >> + node_data = devm_kzalloc(dev, sizeof(*node_data), GFP_KERNEL); >> + if (!node_data) { >> + icc_node_destroy(node->id); >> + return ERR_PTR(-ENOMEM); >> + } >> + >> + node->name = node_desc->name; >> + node->data = node_data; >> + node_data->desc = node_desc; >> + icc_node_add(node, provider); >> + >> + if (node_desc->adj) { >> + ret = imx_icc_node_init_qos(provider, node); >> + if (ret < 0) { >> + imx_icc_node_destroy(node); >> + return ERR_PTR(ret); >> + } >> + } >> + >> + return node; >> +} >> + >> +static void imx_icc_unregister_nodes(struct icc_provider *provider) >> +{ >> + struct icc_node *node, *tmp; >> + >> + list_for_each_entry_safe(node, tmp, &provider->nodes, node_list) >> + imx_icc_node_destroy(node); >> +} >> + >> +static int imx_icc_register_nodes(struct icc_provider *provider, >> + const struct imx_icc_node_desc *descs, >> + int count) >> +{ >> + struct icc_onecell_data *provider_data = provider->data; >> + int ret; >> + int i; >> + >> + for (i = 0; i < count; i++) { >> + struct icc_node *node; >> + const struct imx_icc_node_desc *node_desc = &descs[i]; >> + size_t j; >> + >> + node = imx_icc_node_add(provider, node_desc); >> + if (IS_ERR(node)) { >> + ret = PTR_ERR(node); >> + if (ret != -EPROBE_DEFER) >> + dev_err(provider->dev, "failed to add %s: %d\n", >> + node_desc->name, ret); >> + goto err; >> + } >> + provider_data->nodes[node->id] = node; >> + >> + for (j = 0; j < node_desc->num_links; j++) { >> + ret = icc_link_create(node, node_desc->links[j]); >> + if (ret) { >> + dev_err(provider->dev, "failed to link node %d to %d: %d\n", >> + node->id, node_desc->links[j], ret); >> + goto err; >> + } >> + } >> + } >> + >> + return 0; >> + >> +err: >> + imx_icc_unregister_nodes(provider); >> + >> + return ret; >> +} >> + >> +static int get_max_node_id(struct imx_icc_node_desc *nodes, int nodes_count) >> +{ >> + int i, ret = 0; >> + >> + for (i = 0; i < nodes_count; ++i) >> + if (nodes[i].id > ret) >> + ret = nodes[i].id; >> + >> + return ret; >> +} >> + >> +int imx_icc_register(struct platform_device *pdev, >> + struct imx_icc_node_desc *nodes, int nodes_count) >> +{ >> + struct device *dev = &pdev->dev; >> + struct icc_onecell_data *data; >> + struct icc_provider *provider; >> + int max_node_id; >> + int ret; >> + >> + /* icc_onecell_data is indexed by node_id, unlike nodes param */ >> + max_node_id = get_max_node_id(nodes, nodes_count); >> + data = devm_kzalloc(dev, struct_size(data, nodes, max_node_id), >> + GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + data->num_nodes = max_node_id; >> + >> + provider = devm_kzalloc(dev, sizeof(*provider), GFP_KERNEL); >> + if (!provider) >> + return -ENOMEM; >> + provider->set = imx_icc_set; >> + provider->aggregate = imx_icc_aggregate; > > provider->aggregate = icc_std_aggregate; > >> + provider->xlate = of_icc_xlate_onecell; >> + provider->data = data; >> + provider->dev = dev->parent; >> + platform_set_drvdata(pdev, provider); >> + >> + ret = icc_provider_add(provider); >> + if (ret) { >> + dev_err(dev, "error adding interconnect provider: %d\n", ret); >> + return ret; >> + } >> + >> + ret = imx_icc_register_nodes(provider, nodes, nodes_count); >> + if (ret) >> + goto provider_del; >> + >> + return 0; >> + >> +provider_del: >> + icc_provider_del(provider); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(imx_icc_register); >> + >> +int imx_icc_unregister(struct platform_device *pdev) >> +{ >> + struct icc_provider *provider = platform_get_drvdata(pdev); >> + int ret; >> + >> + if (provider->users) { >> + dev_warn(&pdev->dev, "interconnect provider still has %d users\n", >> + provider->users); >> + return -EBUSY; >> + } > > The above check already exists in icc_provider_del(). But i assume you don't > want to delete any nodes from the provider if it still has users. Maybe it will > be sensible to add this check into icc_nodes_remove() instead, so that such > cases are handled on a framework level. Actually device removal is not allowed to fail, for example doing an explicit unbind will ignore errors coming from the unregister function: echo imx8mm-interconnect > /sys/bus/platform/drivers/imx8mm-interconnect/unbind results in the following stack trace where device_driver_detach actually continues even if remove returns an error: [ 24.017901] imx_icc_unregister+0x24/0x80 [ 24.021925] imx8mm_icc_remove+0x18/0x28 [ 24.025863] platform_drv_remove+0x34/0x58 [ 24.029974] device_release_driver_internal+0x104/0x1d8 [ 24.035213] device_driver_detach+0x20/0x30 [ 24.039409] unbind_store+0xe8/0x110 [ 24.042999] drv_attr_store+0x2c/0x40 [ 24.046677] sysfs_kf_write+0x54/0x80 I'm not sure how the framework should handle provider removal. Right now icc_path holds icc_node pointers so when nodes are freed any consumer calling into icc will corrupt memory. One option would be to reference count the allocation of struct icc_node, this way icc_path functions could safely check for node->provider == NULL and return an error. I'll remove this check from the imx driver because it does not solve anything. > >> + imx_icc_unregister_nodes(provider); >> + >> + ret = icc_provider_del(provider); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(imx_icc_unregister); >> + >> +MODULE_LICENSE("GPL v2"); >> diff --git a/drivers/interconnect/imx/imx.h b/drivers/interconnect/imx/imx.h >> new file mode 100644 >> index 000000000000..aa811e4ebb7e >> --- /dev/null >> +++ b/drivers/interconnect/imx/imx.h >> @@ -0,0 +1,62 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Interconnect framework driver for i.MX SoC >> + * >> + * Copyright (c) 2019, BayLibre >> + * Copyright (c) 2019, NXP >> + * Author: Alexandre Bailon <abailon@baylibre.com> >> + * Author: Leonard Crestez <leonard.crestez@nxp.com> >> + */ >> +#ifndef __DRIVERS_INTERCONNECT_IMX_H >> +#define __DRIVERS_INTERCONNECT_IMX_H >> + >> +#include <linux/kernel.h> >> + >> +#define IMX_ICC_MAX_LINKS 4 >> + >> +/* >> + * struct imx_icc_node_adj - Describe a dynamic adjustable node >> + */ >> +struct imx_icc_node_adj_desc { >> + unsigned int bw_mul, bw_div; >> + const char *phandle_name; >> + bool main_noc; >> +}; >> + >> +/* >> + * struct imx_icc_node - Describe an interconnect node >> + * @name: name of the node >> + * @id: an unique id to identify the node >> + * @links: an array of slaves' node id >> + * @num_links: number of id defined in links >> + */ >> +struct imx_icc_node_desc { >> + const char *name; >> + u16 id; >> + u16 links[IMX_ICC_MAX_LINKS]; >> + u16 num_links; >> + > > Why the blank line? Removed >> + const struct imx_icc_node_adj_desc *adj; >> +}; >> + >> +#define DEFINE_BUS_INTERCONNECT(_name, _id, _adj, ...) \ >> + { \ >> + .id = _id, \ >> + .name = _name, \ >> + .adj = _adj, \ >> + .num_links = ARRAY_SIZE(((int[]){ __VA_ARGS__ })), \ >> + .links = { __VA_ARGS__ }, \ >> + } >> + >> +#define DEFINE_BUS_MASTER(_name, _id, _dest_id) \ >> + DEFINE_BUS_INTERCONNECT(_name, _id, NULL, _dest_id) >> + >> +#define DEFINE_BUS_SLAVE(_name, _id, _adj) \ >> + DEFINE_BUS_INTERCONNECT(_name, _id, _adj) >> + >> +int imx_icc_register(struct platform_device *pdev, >> + struct imx_icc_node_desc *nodes, >> + int nodes_count); >> +int imx_icc_unregister(struct platform_device *pdev); >> + >> +#endif /* __DRIVERS_INTERCONNECT_IMX_H */
On Wed, Apr 1, 2020 at 9:35 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: > > Add nodes for the main interconnect of the imx8m series chips. > > These nodes are bound to by devfreq and interconnect drivers. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 24 +++++++++++++++++++++++ > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 24 +++++++++++++++++++++++ > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 24 +++++++++++++++++++++++ > 3 files changed, 72 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > index 175c28ae10cf..41047b6709b6 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > @@ -6,10 +6,11 @@ > #include <dt-bindings/clock/imx8mm-clock.h> > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/input/input.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/thermal/thermal.h> > +#include <dt-bindings/interconnect/imx8mm.h> > > #include "imx8mm-pinfunc.h" > > / { > interrupt-parent = <&gic>; > @@ -860,10 +861,33 @@ > status = "disabled"; > }; > > }; > > + noc: interconnect@32700000 { > + compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc"; > + reg = <0x32700000 0x100000>; > + clocks = <&clk IMX8MM_CLK_NOC>; > + fsl,ddrc = <&ddrc>; > + #interconnect-cells = <1>; > + operating-points-v2 = <&noc_opp_table>; > + > + noc_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-150M { > + opp-hz = /bits/ 64 <150000000>; > + }; > + opp-375M { > + opp-hz = /bits/ 64 <375000000>; > + }; > + opp-750M { > + opp-hz = /bits/ 64 <750000000>; Out of curiosity, the 8M Mini runs up to 750M, and the 8M Nano and 8MQ run up to 800. The 8MQ had a patch to increase the assigned clock speed for the NOC to 800MHz See: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/patch/arch/arm64/boot/dts/freescale?id=912b9dacf3f0ffad55e1a1b3c5af0e433ebdb5dd) The 8M Mini and 8M Nano appear to be setting the default speed to 0. Should the 8M Mini or 8M Nano do something similar to what the 8MQ did, or does this series negate the need for such a patch? thanks adam > + }; > + }; > + }; > + > aips4: bus@32c00000 { > compatible = "fsl,aips-bus", "simple-bus"; > reg = <0x32df0000 0x10000>; > #address-cells = <1>; > #size-cells = <1>; > diff --git a/arch/arm64/boot/dts/freescale/imx8mn.dtsi b/arch/arm64/boot/dts/freescale/imx8mn.dtsi > index 88e7d74e077f..e8a55956813f 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mn.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mn.dtsi > @@ -6,10 +6,11 @@ > #include <dt-bindings/clock/imx8mn-clock.h> > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/input/input.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/thermal/thermal.h> > +#include <dt-bindings/interconnect/imx8mn.h> > > #include "imx8mn-pinfunc.h" > > / { > interrupt-parent = <&gic>; > @@ -751,10 +752,33 @@ > status = "disabled"; > }; > > }; > > + noc: interconnect@32700000 { > + compatible = "fsl,imx8mn-noc", "fsl,imx8m-noc"; > + reg = <0x32700000 0x100000>; > + clocks = <&clk IMX8MN_CLK_NOC>; > + fsl,ddrc = <&ddrc>; > + #interconnect-cells = <1>; > + operating-points-v2 = <&noc_opp_table>; > + > + noc_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-100M { > + opp-hz = /bits/ 64 <100000000>; > + }; > + opp-600M { > + opp-hz = /bits/ 64 <600000000>; > + }; > + opp-800M { > + opp-hz = /bits/ 64 <800000000>; > + }; > + }; > + }; > + > aips4: bus@32c00000 { > compatible = "fsl,aips-bus", "simple-bus"; > reg = <0x32df0000 0x10000>; > #address-cells = <1>; > #size-cells = <1>; > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > index ea93bc4b7d7e..3a208feec74c 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi > @@ -9,10 +9,11 @@ > #include <dt-bindings/reset/imx8mq-reset.h> > #include <dt-bindings/gpio/gpio.h> > #include "dt-bindings/input/input.h" > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/thermal/thermal.h> > +#include <dt-bindings/interconnect/imx8mq.h> > #include "imx8mq-pinfunc.h" > > / { > interrupt-parent = <&gpc>; > > @@ -1026,10 +1027,33 @@ > fsl,num-rx-queues = <3>; > status = "disabled"; > }; > }; > > + noc: interconnect@32700000 { > + compatible = "fsl,imx8mq-noc", "fsl,imx8m-noc"; > + reg = <0x32700000 0x100000>; > + clocks = <&clk IMX8MQ_CLK_NOC>; > + fsl,ddrc = <&ddrc>; > + #interconnect-cells = <1>; > + operating-points-v2 = <&noc_opp_table>; > + > + noc_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-133M { > + opp-hz = /bits/ 64 <133333333>; > + }; > + opp-400M { > + opp-hz = /bits/ 64 <400000000>; > + }; > + opp-800M { > + opp-hz = /bits/ 64 <800000000>; > + }; > + }; > + }; > + > bus@32c00000 { /* AIPS4 */ > compatible = "fsl,aips-bus", "simple-bus"; > reg = <0x32df0000 0x10000>; > #address-cells = <1>; > #size-cells = <1>; > -- > 2.17.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 2020-04-04 4:25 PM, Adam Ford wrote: > On Wed, Apr 1, 2020 at 9:35 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: >> >> Add nodes for the main interconnect of the imx8m series chips. >> >> These nodes are bound to by devfreq and interconnect drivers. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> --- >> arch/arm64/boot/dts/freescale/imx8mm.dtsi | 24 +++++++++++++++++++++++ >> arch/arm64/boot/dts/freescale/imx8mn.dtsi | 24 +++++++++++++++++++++++ >> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 24 +++++++++++++++++++++++ >> 3 files changed, 72 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi >> index 175c28ae10cf..41047b6709b6 100644 >> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi >> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi >> @@ -6,10 +6,11 @@ >> #include <dt-bindings/clock/imx8mm-clock.h> >> #include <dt-bindings/gpio/gpio.h> >> #include <dt-bindings/input/input.h> >> #include <dt-bindings/interrupt-controller/arm-gic.h> >> #include <dt-bindings/thermal/thermal.h> >> +#include <dt-bindings/interconnect/imx8mm.h> >> >> #include "imx8mm-pinfunc.h" >> >> / { >> interrupt-parent = <&gic>; >> @@ -860,10 +861,33 @@ >> status = "disabled"; >> }; >> >> }; >> >> + noc: interconnect@32700000 { >> + compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc"; >> + reg = <0x32700000 0x100000>; >> + clocks = <&clk IMX8MM_CLK_NOC>; >> + fsl,ddrc = <&ddrc>; >> + #interconnect-cells = <1>; >> + operating-points-v2 = <&noc_opp_table>; >> + >> + noc_opp_table: opp-table { >> + compatible = "operating-points-v2"; >> + >> + opp-150M { >> + opp-hz = /bits/ 64 <150000000>; >> + }; >> + opp-375M { >> + opp-hz = /bits/ 64 <375000000>; >> + }; >> + opp-750M { >> + opp-hz = /bits/ 64 <750000000>; > > Out of curiosity, the 8M Mini runs up to 750M, and the 8M Nano and > 8MQ run up to 800. The 8MQ had a patch to increase the assigned clock > speed for the NOC to 800MHz > > See: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fnext%2Flinux-next.git%2Fpatch%2Farch%2Farm64%2Fboot%2Fdts%2Ffreescale%3Fid%3D912b9dacf3f0ffad55e1a1b3c5af0e433ebdb5dd&data=02%7C01%7Cleonard.crestez%40nxp.com%7C32c3655718e4459028e008d7d89baa31%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637216035403876452&sdata=z%2B5afsPGbCk4HkRp4nR6QepOrm70Fi5B5dohyvaquxo%3D&reserved=0) > > The 8M Mini and 8M Nano appear to be setting the default speed to 0. I'm not sure what you mean about this, the noc clock is required for mostly everything. > Should the 8M Mini or 8M Nano do something similar to what the 8MQ > did, or does this series negate the need for such a patch? Instead of doing assigned-clocks noc frequency needs to be tweaked by adjusting OPPs in this list. The devfreq device for noc will overwrite other frequencies set for the noc. -- Regards, Leonard
On Tue, Apr 7, 2020 at 4:10 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: > > On 2020-04-04 4:25 PM, Adam Ford wrote: > > On Wed, Apr 1, 2020 at 9:35 AM Leonard Crestez <leonard.crestez@nxp.com> wrote: > >> > >> Add nodes for the main interconnect of the imx8m series chips. > >> > >> These nodes are bound to by devfreq and interconnect drivers. > >> > >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > >> --- > >> arch/arm64/boot/dts/freescale/imx8mm.dtsi | 24 +++++++++++++++++++++++ > >> arch/arm64/boot/dts/freescale/imx8mn.dtsi | 24 +++++++++++++++++++++++ > >> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 24 +++++++++++++++++++++++ > >> 3 files changed, 72 insertions(+) > >> > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > >> index 175c28ae10cf..41047b6709b6 100644 > >> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > >> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > >> @@ -6,10 +6,11 @@ > >> #include <dt-bindings/clock/imx8mm-clock.h> > >> #include <dt-bindings/gpio/gpio.h> > >> #include <dt-bindings/input/input.h> > >> #include <dt-bindings/interrupt-controller/arm-gic.h> > >> #include <dt-bindings/thermal/thermal.h> > >> +#include <dt-bindings/interconnect/imx8mm.h> > >> > >> #include "imx8mm-pinfunc.h" > >> > >> / { > >> interrupt-parent = <&gic>; > >> @@ -860,10 +861,33 @@ > >> status = "disabled"; > >> }; > >> > >> }; > >> > >> + noc: interconnect@32700000 { > >> + compatible = "fsl,imx8mm-noc", "fsl,imx8m-noc"; > >> + reg = <0x32700000 0x100000>; > >> + clocks = <&clk IMX8MM_CLK_NOC>; > >> + fsl,ddrc = <&ddrc>; > >> + #interconnect-cells = <1>; > >> + operating-points-v2 = <&noc_opp_table>; > >> + > >> + noc_opp_table: opp-table { > >> + compatible = "operating-points-v2"; > >> + > >> + opp-150M { > >> + opp-hz = /bits/ 64 <150000000>; > >> + }; > >> + opp-375M { > >> + opp-hz = /bits/ 64 <375000000>; > >> + }; > >> + opp-750M { > >> + opp-hz = /bits/ 64 <750000000>; > > > > Out of curiosity, the 8M Mini runs up to 750M, and the 8M Nano and > > 8MQ run up to 800. The 8MQ had a patch to increase the assigned clock > > speed for the NOC to 800MHz > > > > See: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fnext%2Flinux-next.git%2Fpatch%2Farch%2Farm64%2Fboot%2Fdts%2Ffreescale%3Fid%3D912b9dacf3f0ffad55e1a1b3c5af0e433ebdb5dd&data=02%7C01%7Cleonard.crestez%40nxp.com%7C32c3655718e4459028e008d7d89baa31%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637216035403876452&sdata=z%2B5afsPGbCk4HkRp4nR6QepOrm70Fi5B5dohyvaquxo%3D&reserved=0) > > > > The 8M Mini and 8M Nano appear to be setting the default speed to 0. > > I'm not sure what you mean about this, the noc clock is required for > mostly everything. As an example, the i.MX8MM looks like this: assigned-clocks = <&clk IMX8MM_CLK_NOC>, <&clk IMX8MM_CLK_AUDIO_AHB>, <&clk IMX8MM_CLK_IPG_AUDIO_ROOT>, <&clk IMX8MM_SYS_PLL3>, <&clk IMX8MM_VIDEO_PLL1>, <&clk IMX8MM_AUDIO_PLL1>, <&clk IMX8MM_AUDIO_PLL2>; assigned-clock-parents = <&clk IMX8MM_SYS_PLL3_OUT>, <&clk IMX8MM_SYS_PLL1_800M>; assigned-clock-rates = <0>, <400000000>, <400000000>, <750000000>, <594000000>, <393216000>, <361267200>; If I am reading this correctly, it appears to me that IMX8MM_CLK_NOC is set to 0. The i.MX8MN is similar, but the patch above shows IMX8MQ_CLK_NOC used to be 0, but was updated for better performance. > > > Should the 8M Mini or 8M Nano do something similar to what the 8MQ > > did, or does this series negate the need for such a patch? > > Instead of doing assigned-clocks noc frequency needs to be tweaked by > adjusting OPPs in this list. The devfreq device for noc will overwrite > other frequencies set for the noc. My question was whether or not we should consider a patch to made the default assigned-clock-rate for IMX8MM_CLK_NOC to be 750000000. Based on your response, it sounds like the answer might be that it's not necessary. thanks adam > > -- > Regards, > Leonard