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@xxxxxxxxxxxx>
>> Signed-off-by: Leonard Crestez <leonard.crestez@xxxxxxx>
>> Tested-by: Martin Kepplinger <martin.kepplinger@xxxxxxx>
>> ---
>> 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@xxxxxxxxxxxx>
>> + * Author: Leonard Crestez <leonard.crestez@xxxxxxx>
>> + */
>> +
>> +#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@xxxxxxxxxxxx>
>> + * Author: Leonard Crestez <leonard.crestez@xxxxxxx>
>> + */
>> +#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 */
