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);
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