| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <396329f7-a8e9-79da-9187-d5fd61c2bf4e@samsung.com>
Date: Mon, 1 Jun 2020 11:57:23 +0200
From: Sylwester Nawrocki <s.nawrocki@...sung.com>
To: Chanwoo Choi <chanwoo@...nel.org>
Cc: Georgi Djakov <georgi.djakov@...aro.org>,
Chanwoo Choi <cw00.choi@...sung.com>,
Krzysztof Kozlowski <krzk@...nel.org>,
Artur Świgoń <a.swigon@...sung.com>,
MyungJoo Ham <myungjoo.ham@...sung.com>, inki.dae@...sung.com,
Seung-Woo Kim <sw0312.kim@...sung.com>,
Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
Marek Szyprowski <m.szyprowski@...sung.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
dri-devel <dri-devel@...ts.freedesktop.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
Rob Herring <robh+dt@...nel.org>,
devicetree <devicetree@...r.kernel.org>
Subject: Re: [RFC PATCH v5 2/6] interconnect: Add generic interconnect
driver for Exynos SoCs
Cc: Rob, devicetree ML
On 31.05.2020 02:13, Chanwoo Choi wrote:
> On Sat, May 30, 2020 at 1:34 AM Sylwester Nawrocki
> <s.nawrocki@...sung.com> wrote:
>>
>> This patch adds a generic interconnect driver for Exynos SoCs in order
>> to provide interconnect functionality for each "samsung,exynos-bus"
>> compatible device.
>>
>> The SoC topology is a graph (or more specifically, a tree) and its
>> edges are specified using the 'samsung,interconnect-parent' in the
>> DT. Due to unspecified relative probing order, -EPROBE_DEFER may be
>> propagated to ensure that the parent is probed before its children.
>>
>> Each bus is now an interconnect provider and an interconnect node as
>> well (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
>> registers itself as a node. Node IDs are not hardcoded but rather
>> assigned dynamically at runtime. This approach allows for using this
>> driver with various Exynos SoCs.
>>
>> Frequencies requested via the interconnect API for a given node are
>> propagated to devfreq using dev_pm_qos_update_request(). Please note
>> that it is not an error when CONFIG_INTERCONNECT is 'n', in which
>> case all interconnect API functions are no-op.
>>
>> Signed-off-by: Artur Świgoń <a.swigon@...sung.com>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@...sung.com>
>>
>> Changes for v5:
>> - adjust to renamed exynos,interconnect-parent-node property,
>> - use automatically generated platform device id as the interconect
>> node id instead of a now unavailable devfreq->id field,
>> - add icc_ prefix to some variables to make the code more self-commenting,
>> - use icc_nodes_remove() instead of icc_node_del() + icc_node_destroy(),
>> - adjust to exynos,interconnect-parent-node property rename to
>> samsung,interconnect-parent,
>> - converted to a separate platform driver in drivers/interconnect.
>> ---
>> drivers/interconnect/Kconfig | 1 +
>> drivers/interconnect/Makefile | 1 +
>> drivers/interconnect/exynos/Kconfig | 6 ++
>> drivers/interconnect/exynos/Makefile | 4 +
>> drivers/interconnect/exynos/exynos.c | 185 +++++++++++++++++++++++++++++++++++
>> 5 files changed, 197 insertions(+)
>> create mode 100644 drivers/interconnect/exynos/Kconfig
>> create mode 100644 drivers/interconnect/exynos/Makefile
>> create mode 100644 drivers/interconnect/exynos/exynos.c
>>
>> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
>> index 5b7204e..eca6eda 100644
>> --- a/drivers/interconnect/Kconfig
>> +++ b/drivers/interconnect/Kconfig
>> @@ -11,6 +11,7 @@ menuconfig INTERCONNECT
>>
>> if INTERCONNECT
>>
>> +source "drivers/interconnect/exynos/Kconfig"
>> source "drivers/interconnect/imx/Kconfig"
>> source "drivers/interconnect/qcom/Kconfig"
>>
>> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
>> index 4825c28..2ba1de6 100644
>> --- a/drivers/interconnect/Makefile
>> +++ b/drivers/interconnect/Makefile
>> @@ -4,5 +4,6 @@ CFLAGS_core.o := -I$(src)
>> icc-core-objs := core.o
>>
>> obj-$(CONFIG_INTERCONNECT) += icc-core.o
>> +obj-$(CONFIG_INTERCONNECT_EXYNOS) += exynos/
>> obj-$(CONFIG_INTERCONNECT_IMX) += imx/
>> obj-$(CONFIG_INTERCONNECT_QCOM) += qcom/
>> diff --git a/drivers/interconnect/exynos/Kconfig b/drivers/interconnect/exynos/Kconfig
>> new file mode 100644
>> index 0000000..e51e52e
>> --- /dev/null
>> +++ b/drivers/interconnect/exynos/Kconfig
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +config INTERCONNECT_EXYNOS
>> + tristate "Exynos generic interconnect driver"
>> + depends on ARCH_EXYNOS || COMPILE_TEST
>> + help
>> + Generic interconnect driver for Exynos SoCs.
>> diff --git a/drivers/interconnect/exynos/Makefile b/drivers/interconnect/exynos/Makefile
>> new file mode 100644
>> index 0000000..e19d1df
>> --- /dev/null
>> +++ b/drivers/interconnect/exynos/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +exynos-interconnect-objs := exynos.o
>> +
>> +obj-$(CONFIG_INTERCONNECT_EXYNOS) += exynos-interconnect.o
>> diff --git a/drivers/interconnect/exynos/exynos.c b/drivers/interconnect/exynos/exynos.c
>> new file mode 100644
>> index 0000000..8278194
>> --- /dev/null
>> +++ b/drivers/interconnect/exynos/exynos.c
>> @@ -0,0 +1,185 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Exynos generic interconnect provider driver
>> + *
>> + * Copyright (c) 2020 Samsung Electronics Co., Ltd.
>> + *
>> + * Authors: Artur Świgoń <a.swigon@...sung.com>
>> + * Sylwester Nawrocki <s.nawrocki@...sung.com>
>> + */
>> +#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>
>> +
>> +#define kbps_to_khz(x) ((x) / 8)
>> +
>> +struct exynos_icc_priv {
>> + struct device *dev;
>> +
>> + /* One interconnect node per provider */
>> + struct icc_provider provider;
>> + struct icc_node *node;
>> +
>> + struct dev_pm_qos_request qos_req;
>> +};
>> +
>> +static struct icc_node *exynos_icc_get_parent(struct device_node *np)
>> +{
>> + struct of_phandle_args args;
>> + int num, ret;
>> +
>> + num = of_count_phandle_with_args(np, "samsung,interconnect-parent",
>> + "#interconnect-cells");
>> + if (num != 1)
>> + return NULL; /* parent nodes are optional */
>> +
>> + ret = of_parse_phandle_with_args(np, "samsung,interconnect-parent",
>> + "#interconnect-cells", 0, &args);
>> + if (ret < 0)
>> + return ERR_PTR(ret);
>> +
>> + of_node_put(args.np);
>> +
>> + return of_icc_get_from_provider(&args);
>> +}
>> +
>> +
>> +static int exynos_generic_icc_set(struct icc_node *src, struct icc_node *dst)
>> +{
>> + struct exynos_icc_priv *src_priv = src->data, *dst_priv = dst->data;
>> + s32 src_freq = kbps_to_khz(max(src->avg_bw, src->peak_bw));
>> + s32 dst_freq = kbps_to_khz(max(dst->avg_bw, dst->peak_bw));
>> + int ret;
>> +
>> + ret = dev_pm_qos_update_request(&src_priv->qos_req, src_freq);
>> + if (ret < 0) {
>> + dev_err(src_priv->dev, "failed to update PM QoS of %s\n",
>> + src->name);
>> + return ret;
>> + }
>> +
>> + ret = dev_pm_qos_update_request(&dst_priv->qos_req, dst_freq);
>> + if (ret < 0) {
>> + dev_err(dst_priv->dev, "failed to update PM QoS of %s\n",
>> + dst->name);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static struct icc_node *exynos_generic_icc_xlate(struct of_phandle_args *spec,
>> + void *data)
>> +{
>> + struct exynos_icc_priv *priv = data;
>> +
>> + if (spec->np != priv->dev->parent->of_node)
>> + return ERR_PTR(-EINVAL);
>> +
>> + return priv->node;
>> +}
>> +
>> +static int exynos_generic_icc_remove(struct platform_device *pdev)
>> +{
>> + struct exynos_icc_priv *priv = platform_get_drvdata(pdev);
>> + struct icc_node *parent_node, *node = priv->node;
>> +
>> + parent_node = exynos_icc_get_parent(priv->dev->parent->of_node);
>> + if (parent_node && !IS_ERR(parent_node))
>> + icc_link_destroy(node, parent_node);
>> +
>> + icc_nodes_remove(&priv->provider);
>> + icc_provider_del(&priv->provider);
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_generic_icc_probe(struct platform_device *pdev)
>> +{
>> + struct device *bus_dev = pdev->dev.parent;
>> + struct exynos_icc_priv *priv;
>> + struct icc_provider *provider;
>> + struct icc_node *icc_node, *icc_parent_node;
>> + int ret;
>> +
>> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->dev = &pdev->dev;
>> + platform_set_drvdata(pdev, priv);
>> +
>> + provider = &priv->provider;
>> +
>> + provider->set = exynos_generic_icc_set;
>> + provider->aggregate = icc_std_aggregate;
>> + provider->xlate = exynos_generic_icc_xlate;
>> + provider->dev = bus_dev;
>> + provider->inter_set = true;
>> + provider->data = priv;
>> +
>> + ret = icc_provider_add(provider);
>> + if (ret < 0)
>> + return ret;
>> +
>> + icc_node = icc_node_create(pdev->id);
>> + if (IS_ERR(icc_node)) {
>> + ret = PTR_ERR(icc_node);
>> + goto err_prov_del;
>> + }
>> +
>> + priv->node = icc_node;
>> + icc_node->name = bus_dev->of_node->name;
>> + icc_node->data = priv;
>> + icc_node_add(icc_node, provider);
>> +
>> + icc_parent_node = exynos_icc_get_parent(bus_dev->of_node);
>> + if (IS_ERR(icc_parent_node)) {
>> + ret = PTR_ERR(icc_parent_node);
>> + goto err_node_del;
>> + }
>> + if (icc_parent_node) {
>> + ret = icc_link_create(icc_node, icc_parent_node->id);
>> + if (ret < 0)
>> + goto err_node_del;
>> + }
>> +
>> + /*
>> + * Register a PM QoS request for the bus device for which also devfreq
>> + * functionality is registered.
>> + */
>> + ret = dev_pm_qos_add_request(bus_dev, &priv->qos_req,
>> + DEV_PM_QOS_MIN_FREQUENCY, 0);
>> + if (ret < 0)
>> + goto err_link_destroy;
>> +
>> + return 0;
>> +
>> +err_link_destroy:
>> + if (icc_parent_node)
>> + icc_link_destroy(icc_node, icc_parent_node);
>> +err_node_del:
>> + icc_nodes_remove(provider);
>> +err_prov_del:
>> + icc_provider_del(provider);
>> +
>> + return ret;
>> +}
>> +
>> +static struct platform_driver exynos_generic_icc_driver = {
>> + .driver = {
>> + .name = "exynos-generic-icc",
>> + },
>> + .probe = exynos_generic_icc_probe,
>> + .remove = exynos_generic_icc_remove,
>> +};
>> +module_platform_driver(exynos_generic_icc_driver);
>> +
>> +MODULE_DESCRIPTION("Exynos generic interconnect driver");
>> +MODULE_AUTHOR("Artur Świgoń <a.swigon@...sung.com>");
>> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@...sung.com>");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:exynos-generic-icc");
>> --
>> 2.7.4
>>
>
> Reviewed-by: Chanwoo Choi <cw00.choi@...sung.com>
>
> As I commented, How about changing the compatible name 'exynos-icc'
> without 'generic'?
> The 'exynos-icc' doesn't have the any specific version of Exynos SoC,
> it think that it already contain the 'generic' meaning for Exynos SoC.
Sure, I can change it to "exynos-icc". However please note it is just
a name for the driver and its related virtual platform (sub)device that
is created in the devfreq driver, which matches on the "samsung,exynos-bus"
compatible. Of course we could add any specific DT compatible to this driver
in future if needed.
--
Thanks,
Sylwester
Powered by blists - more mailing lists