| Message ID | 20200529163200.18031-3-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Exynos: Simple QoS for exynos-bus using interconnect | expand |
Hi Sylwester, On Sat, May 30, 2020 at 1:34 AM Sylwester Nawrocki <s.nawrocki@samsung.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@samsung.com> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.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@samsung.com> > + * Sylwester Nawrocki <s.nawrocki@samsung.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@samsung.com>"); > +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:exynos-generic-icc"); > -- > 2.7.4 > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.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.
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@samsung.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@samsung.com> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.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@samsung.com> >> + * Sylwester Nawrocki <s.nawrocki@samsung.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@samsung.com>"); >> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); >> +MODULE_LICENSE("GPL v2"); >> +MODULE_ALIAS("platform:exynos-generic-icc"); >> -- >> 2.7.4 >> > > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.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.
On Fri, May 29, 2020 at 06:31:56PM +0200, Sylwester Nawrocki 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@samsung.com> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.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@samsung.com> > + * Sylwester Nawrocki <s.nawrocki@samsung.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); I think of_node_put() should happen after of_icc_get_from_provider(). Best regards, Krzysztof
On 02.06.2020 10:21, Krzysztof Kozlowski wrote: >> +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); > I think of_node_put() should happen after of_icc_get_from_provider(). Indeed, thanks, I will amend that. It seems the node reference count decrementing is often not done properly after existing calls to of_parse_phandle_with_args().
Hi Sylwester, Thanks for the patch and apologies for the delayed reply. On 5/29/20 19:31, Sylwester Nawrocki 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@samsung.com> > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.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/i.nterconnect. > --- > 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 [..] > +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); > +} > + > + Nit: multiple blank lines [..] > +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)) Nit: !IS_ERR_OR_NULL? > + 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); Nit: Maybe it would be better to move this after the node is created. The idea is to create the nodes first and add the provider when the topology is populated. It's fine either way here, but i am planning to change this in some of the existing provider drivers. > + 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@samsung.com>"); > +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:exynos-generic-icc"); All looks good to me, but it seems that the patch-set is not on Rob's radar currently, so please re-send and CC the DT mailing list. Thanks, Georgi
Hi Georgi, On 01.07.2020 14:50, Georgi Djakov wrote: > Thanks for the patch and apologies for the delayed reply. Thanks, no problem. It's actually just in time as I put that patchset aside for a while and was just about to post an update. > On 5/29/20 19:31, Sylwester Nawrocki 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@samsung.com> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> >> +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); >> +} >> + >> + > > Nit: multiple blank lines Fixed. > [..] >> +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)) > > Nit: !IS_ERR_OR_NULL? It was left on purpose that way but I changed it now to IS_ERR_OR_NULL. >> + 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); > > Nit: Maybe it would be better to move this after the node is created. The > idea is to create the nodes first and add the provider when the topology is > populated. It's fine either way here, but i am planning to change this in > some of the existing provider drivers. OK, it makes the clean up path a bit less straightforward. And still we need to register the provider before calling icc_node_add(). I made a change as below. --------------8<------------------ @@ -124,14 +123,14 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) provider->inter_set = true; provider->data = priv; + icc_node = icc_node_create(pdev->id); + if (IS_ERR(icc_node)) + return PTR_ERR(icc_node); + ret = icc_provider_add(provider); - if (ret < 0) + if (ret < 0) { + icc_node_destroy(icc_node->id); 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; @@ -171,9 +170,7 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) icc_link_destroy(icc_node, icc_parent_node); err_node_del: icc_nodes_remove(provider); -err_prov_del: icc_provider_del(provider); - return ret; } --------------8<------------------ >> + 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; >> +} > All looks good to me, but it seems that the patch-set is not on > Rob's radar currently, so please re-send and CC the DT mailing list. Thanks, indeed I missed some mailing list when posting. I will make sure Rob and DT ML list is on Cc, especially that I have added new "bus-width" property in v6.
Hi Sylwester, On 7/2/20 15:01, Sylwester Nawrocki wrote: > Hi Georgi, > > On 01.07.2020 14:50, Georgi Djakov wrote: >> Thanks for the patch and apologies for the delayed reply. > > Thanks, no problem. It's actually just in time as I put that patchset > aside for a while and was just about to post an update. > >> On 5/29/20 19:31, Sylwester Nawrocki 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@samsung.com> >>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > >>> +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); >>> +} >>> + >>> + >> >> Nit: multiple blank lines > > Fixed. > >> [..] >>> +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)) >> >> Nit: !IS_ERR_OR_NULL? > > It was left on purpose that way but I changed it now to IS_ERR_OR_NULL. Well, i have no strong opinion on that, it's up to you. >>> + 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); >> >> Nit: Maybe it would be better to move this after the node is created. The >> idea is to create the nodes first and add the provider when the topology is >> populated. It's fine either way here, but i am planning to change this in >> some of the existing provider drivers. > > OK, it makes the clean up path a bit less straightforward. And still we need > to register the provider before calling icc_node_add(). > I made a change as below. > > --------------8<------------------ > @@ -124,14 +123,14 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) > provider->inter_set = true; > provider->data = priv; > > + icc_node = icc_node_create(pdev->id); > + if (IS_ERR(icc_node)) > + return PTR_ERR(icc_node); > + > ret = icc_provider_add(provider); > - if (ret < 0) > + if (ret < 0) { > + icc_node_destroy(icc_node->id); > 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; > @@ -171,9 +170,7 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) > icc_link_destroy(icc_node, icc_parent_node); > err_node_del: > icc_nodes_remove(provider); > -err_prov_del: > icc_provider_del(provider); > - > return ret; > } > --------------8<------------------ Actually i need to post some patches first, so maybe keep it as is for now and we will update it afterwards. Sorry for the confusion. > > >>> + 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; >>> +} > >> All looks good to me, but it seems that the patch-set is not on >> Rob's radar currently, so please re-send and CC the DT mailing list. > > Thanks, indeed I missed some mailing list when posting. I will make sure > Rob and DT ML list is on Cc, especially that I have added new "bus-width" > property in v6. Ok, good. I have been thinking about bus-width and we might want to make it even a generic DT property if there are multiple platforms which want to use it - maybe if the bus-width is the same across the whole interconnect provider. But as most of the existing drivers have different bus-widths, i haven't done it yet, but let's see and start a discussion. Thanks, Georgi
On 02.07.2020 14:33, Georgi Djakov wrote: > On 7/2/20 15:01, Sylwester Nawrocki wrote: >> On 01.07.2020 14:50, Georgi Djakov wrote: >>> On 5/29/20 19:31, Sylwester Nawrocki wrote: >>>> +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)) >>> >>> Nit: !IS_ERR_OR_NULL? >> >> It was left on purpose that way but I changed it now to IS_ERR_OR_NULL. > > Well, i have no strong opinion on that, it's up to you. I will leave it as you suggested, otherwise we are likely to see "clean up" patches sooner or later. >>>> + 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); >>> >>> Nit: Maybe it would be better to move this after the node is created. The >>> idea is to create the nodes first and add the provider when the topology is >>> populated. It's fine either way here, but i am planning to change this in >>> some of the existing provider drivers. >> >> OK, it makes the clean up path a bit less straightforward. And still we need >> to register the provider before calling icc_node_add(). >> I made a change as below. >> >> --------------8<------------------ >> @@ -124,14 +123,14 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) >> provider->inter_set = true; >> provider->data = priv; >> >> + icc_node = icc_node_create(pdev->id); >> + if (IS_ERR(icc_node)) >> + return PTR_ERR(icc_node); >> + >> ret = icc_provider_add(provider); >> - if (ret < 0) >> + if (ret < 0) { >> + icc_node_destroy(icc_node->id); >> 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; >> @@ -171,9 +170,7 @@ static int exynos_generic_icc_probe(struct platform_device *pdev) >> icc_link_destroy(icc_node, icc_parent_node); >> err_node_del: >> icc_nodes_remove(provider); >> -err_prov_del: >> icc_provider_del(provider); >> - >> return ret; >> } >> --------------8<------------------ > > Actually i need to post some patches first, so maybe keep it as is for now and > we will update it afterwards. Sorry for the confusion. OK, anyway this helped me to remove a memory leak, which was there since icc_nodes_remove() was used before a call to icc_node_add() in order to free the node allocated with icc_node_create(), instead of icc_node_destroy(). >>> All looks good to me, but it seems that the patch-set is not on >>> Rob's radar currently, so please re-send and CC the DT mailing list. >> >> Thanks, indeed I missed some mailing list when posting. I will make sure >> Rob and DT ML list is on Cc, especially that I have added new "bus-width" >> property in v6. > > Ok, good. I have been thinking about bus-width and we might want to make it > even a generic DT property if there are multiple platforms which want to > use it - maybe if the bus-width is the same across the whole interconnect > provider. But as most of the existing drivers have different bus-widths, i > haven't done it yet, but let's see and start a discussion. I see, it sounds good to me. Having an array property to allow specifying bus width for each node is probably not so good idea.
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@samsung.com> + * Sylwester Nawrocki <s.nawrocki@samsung.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@samsung.com>"); +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:exynos-generic-icc");