Messages in this thread | | | Subject | Re: [RFC v0 1/2] interconnect: Add generic interconnect controller API | From | Georgi Djakov <> | Date | Thu, 23 Mar 2017 17:21:03 +0200 |
| |
On 03/23/2017 03:21 AM, Michael Turquette wrote: > Hi Georgi, > > Quoting Georgi Djakov (2017-03-01 10:22:34) >> diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt >> new file mode 100644 >> index 000000000000..c62d86e4c52d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt >> @@ -0,0 +1,91 @@ >> +Interconnect Provider Device Tree Bindings >> +========================================= > > I agree with Rob to skip the DT bindings for now. However I did review > this privately in February and I'll re-post my review comments here for > when the bindings do get discussed at a later date: >
Thanks!
>> +Optional properties: >> +interconnect-port : A phandle and interconnect provider specifier as defined by >> + bindings of the interconnect provider specified by phandle. >> + This denotes the port to which the interconnect consumer is >> + wired. It is used when there are multiple interconnect providers >> + that have one or multiple links between them. > > Go ahead and remove the interconnect-port property description from the > provider part of the binding. It should be sufficient to say, > "interconnect providers can also be interconnect consumers, such as in > the case where two network-on-chip fabrics interface directly". >
Sounds good. Done.
>> + >> +Example: >> + >> + snoc: snoc@0580000 { >> + compatible = "qcom,msm-bus-snoc"; >> + reg = <0x580000 0x14000>; >> + #interconnect-cells = <1>; >> + clock-names = "bus_clk", "bus_a_clk"; >> + clocks = <&rpmcc RPM_SMD_SNOC_CLK>, <&rpmcc RPM_SMD_SNOC_A_CLK>; >> + status = "okay"; >> + interconnect-port = <&bimc MAS_SNOC_CFG>, >> + <&bimc SNOC_BIMC_0_MAS>, >> + <&bimc SNOC_BIMC_1_MAS>, >> + <&pnoc SNOC_PNOC_SLV>; >> + }; >> + bimc: bimc@0400000 { >> + compatible = "qcom,msm-bus-bimc"; >> + reg = <0x400000 0x62000>; >> + #interconnect-cells = <1>; >> + clock-names = "bus_clk", "bus_a_clk"; >> + clocks = <&rpmcc RPM_SMD_BIMC_CLK>, <&rpmcc RPM_SMD_BIMC_A_CLK>; >> + status = "okay"; >> + interconnect-port = <&snoc BIMC_SNOC_MAS>; >> + }; >> + pnoc: pnoc@500000 { >> + compatible = "qcom,msm-bus-pnoc"; >> + reg = <0x500000 0x11000>; >> + #interconnect-cells = <1>; >> + clock-names = "bus_clk", "bus_a_clk"; >> + clocks = <&rpmcc RPM_SMD_PCNOC_CLK>, <&rpmcc RPM_SMD_PCNOC_A_CLK>; >> + status = "okay"; >> + interconnect-port = <&snoc PNOC_SNOC_SLV>; >> + }; >> + >> += interconnect consumers = >> + >> +The interconnect consumers are device nodes which consume the interconnect >> +path(s) provided by the interconnect provider. There can be multiple >> +interconnect providers on a SoC and the consumer may consume multiple paths >> +from different providers depending on usecase and the components it has to >> +interact with. >> + >> +Required-properties: >> +interconnect-port : A phandle and interconnect provider specifier as defined by >> + bindings of the interconnect provider specified by phandle. >> + This denotes the port to which the interconnect consumer is >> + wired. >> +interconnect-path : List of phandles to the data path endpoints. > > More copy/paste from February review: > > "Path" means everything between the endpoints (e.g. the details of the > graph traversal), whereas this DT property is really only meant to > defint the target endpoint. Let's rename it to interconnect-target. > > When referring to endpoints I think we should some pairing terminology > like: src,dst or local,remote or initiator,target. > > So how about: > s/interconnect-port/interconnect-sources/ > s/interconnect-path/interconnect-targets/ > > This keeps things symmetrical and the (source,target) pair just makes > sense. I used plural as well since there can be multiple ports. > > It might even make sense to shorten it with: source-ports, target-ports >
Agree that the port/path pairs might be confusing. Currently my favorites are interconnect-src and interconnect-dst.
>> +interconnect-path-names : List of interconnect path name strings sorted in the >> + same order as the interconnect-path property. Consumers drivers >> + will use interconnect-path-names to match the link names with >> + interconnect specifiers. > > Let's not use string names. No reason to copy the clkdev-style of > resource lookups when an integer id will do just fine. >
Yes, this is on my TODO list. Anyway for the platform data i will be using integers only.
>> + >> +Example: >> + >> + sdhci@07864000 { >> + ... >> + interconnect-port = <&pnoc MAS_PNOC_SDCC_2>; >> + interconnect-path = <&blsp1_uart2>; > > interconnect-path (err, port-target?) should not point to a consumer > device node, it should point to the local port of the consumer device. > > How about: > > sdhci@07864000 { > ... > interconnect-sources = <&pnoc 123>; > interconnect-targets = <&pnoc 456>, <&snoc 789>; > }; > > And the magic numbers above (123, 456, 789) can be replaced by > human-readable macros via the DT include chroot. E.g: > > interconnect-source = <&pnoc USB_OTG>; > interconnect-target = <&pnoc OMG_WTF>, <&pnoc BBQ>; > >> + interconnect-path-names = "spi"; >> + }; >> diff --git a/Documentation/interconnect/interconnect.txt b/Documentation/interconnect/interconnect.txt >> new file mode 100644 >> index 000000000000..051d3955f28b >> --- /dev/null >> +++ b/Documentation/interconnect/interconnect.txt >> @@ -0,0 +1,68 @@ > ... >> +The interconnect framework provide the following APIs to consumers: >> + >> +struct interconnect_path *interconnect_get(struct device *dev, const char *id); > > Replace strings with an integer offset for the port. >
Yep.
>> diff --git a/drivers/interconnect/interconnect.c b/drivers/interconnect/interconnect.c >> new file mode 100644 >> index 000000000000..dadd1ffdbc50 >> --- /dev/null >> +++ b/drivers/interconnect/interconnect.c >> @@ -0,0 +1,285 @@ > ... >> +struct interconnect_path *interconnect_get(struct device *dev, const char *id) > > If the consumer device has more than one local port (e.g. source), it > must be specified along the target port, right? >
Yes, we may add it to the arguments and use 0 for the default case when we have only one source port. The platform i am currently playing with only supports only one source port, but we can easy add support for multiple sources.
>> +{ >> + struct device_node *np; >> + struct platform_device *dst_pdev; >> + struct interconnect_node *src, *dst, *node; >> + struct interconnect_path *path; >> + int ret, index; >> + >> + if (WARN_ON(!dev || !id)) >> + return ERR_PTR(-EINVAL); >> + >> + src = find_node(dev->of_node); >> + if (IS_ERR(src)) >> + return ERR_CAST(src); > > Does this assume that dev only has a single local port?
Currently yes.
> >> + >> + index = of_property_match_string(dev->of_node, >> + "interconnect-path-names", id); >> + if (index < 0) { >> + dev_err(dev, "missing interconnect-path-names DT property on %s\n", >> + dev->of_node->full_name); >> + return ERR_PTR(index); >> + } >> + >> + /* get the destination endpoint device_node */ >> + np = of_parse_phandle(dev->of_node, "interconnect-path", index); >> + >> + dst_pdev = of_find_device_by_node(np); >> + if (!dst_pdev) { >> + dev_err(dev, "error finding device by node %s\n", np->name); >> + return ERR_PTR(-ENODEV); >> + } >> + >> + dst = find_node(np); >> + if (IS_ERR(dst)) >> + return ERR_CAST(dst); > > Some of the above can be simplified by using a symbolic constant integer > instead of a string name for the index. > >> diff --git a/include/linux/interconnect-consumer.h b/include/linux/interconnect-consumer.h >> new file mode 100644 >> index 000000000000..8bd5bb3e4196 >> --- /dev/null >> +++ b/include/linux/interconnect-consumer.h >> @@ -0,0 +1,70 @@ >> +/* >> + * Copyright (c) 2017, Linaro Ltd. >> + * >> + * This software is licensed under the terms of the GNU General Public >> + * License version 2, as published by the Free Software Foundation, and >> + * may be copied, distributed, and modified under those terms. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#ifndef _LINUX_INTERCONNECT_CONSUMER_H >> +#define _LINUX_INTERCONNECT_CONSUMER_H >> + >> +struct interconnect_node; >> + >> +/** >> + * struct interconnect_path - interconnect path structure >> + * >> + * @node_list: list of the interconnect nodes >> + * @src_dev: source endpoint >> + * @dst_dev: destination endpoint >> + */ >> +struct interconnect_path { >> + struct list_head node_list; >> + struct device *src_dev; >> + struct device *dst_dev; >> +}; > > Don't expose the definition of interconnect_path to users. They should > only have an opaque handle to the interconnect_path object.
Agree. Done.
> >> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h >> new file mode 100644 >> index 000000000000..af1ca739ce52 >> --- /dev/null >> +++ b/include/linux/interconnect-provider.h >> @@ -0,0 +1,92 @@ >> +/* >> + * Copyright (c) 2017, Linaro Ltd. >> + * >> + * This software is licensed under the terms of the GNU General Public >> + * License version 2, as published by the Free Software Foundation, and >> + * may be copied, distributed, and modified under those terms. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#ifndef _LINUX_INTERCONNECT_PROVIDER_H >> +#define _LINUX_INTERCONNECT_PROVIDER_H >> + >> +#include <linux/interconnect-consumer.h> >> + >> +/** >> + * struct icp_ops - platform specific callback operations for interconnect >> + * providers that will be called from drivers >> + * >> + * @set: set constraints on interconnect >> + * @xlate: provider-specific callback for mapping nodes from phandle arguments >> + */ >> +struct icp_ops { >> + int (*set)(struct interconnect_node *node, u32 bandwidth); >> + struct interconnect_node *(*xlate)(struct of_phandle_args *spec, void *data); >> +}; >> + >> +/** >> + * struct icp - interconnect provider (controller) entity that might >> + * provide multiple interconnect controls >> + * >> + * @icp_list: list of the registered interconnect providers >> + * @nodes: internal list of the interconnect provider nodes >> + * @ops: pointer to device specific struct icp_ops >> + * @dev: the device this interconnect provider belongs to >> + * @of_node: the corresponding device tree node as phandle target >> + * @data: pointer to private data >> + */ >> +struct icp { >> + struct list_head icp_list; >> + struct list_head nodes; >> + const struct icp_ops *ops; >> + struct device *dev; >> + const char *name; >> + struct device_node *of_node; >> + void *data; >> +}; > > Does this need to be defined for provider drivers? >
At the moment, this data is directly populated by the provider drivers, but we could re-factor this - define ops for each node, introduce a register_node(provider, node) function and populate the rest of the data within the framework.
>> + >> +/** >> + * struct interconnect_node - entity that is part of the interconnect topology >> + * >> + * @links: links to other interconnect nodes >> + * @num_links: number of interconnect nodes >> + * @icp: points to the interconnect provider struct this node belongs to >> + * @icn_list: list of interconnect nodes >> + * @search_list: list used when walking the nodes graph >> + * @reverse: pointer to previous node when walking the nodes graph >> + * @is_traversed: flag that is used when walking the nodes graph >> + * @qos_list: a list of QoS constraints >> + */ >> +struct interconnect_node { >> + struct interconnect_node **links; >> + size_t num_links; >> + >> + struct icp *icp; >> + struct list_head icn_list; >> + struct list_head search_list; >> + struct interconnect_node *reverse; >> + bool is_traversed; >> + struct hlist_head qos_list; >> +}; > > Don't define this publicly. Should just be declared as an opaque handle. >
Some data may be used by provider drivers, the qos_list for example, which contains a list of constraints. The other way around would be to make the constraints provider specific or create functions for accessing this data?
>> + >> +/** >> + * struct icn_qos - constraints that are attached to each node >> + * >> + * @node: linked list node >> + * @path: the interconnect path which is using this constraint >> + * @bandwidth: an integer describing the bandwidth in kbps >> + */ >> +struct icn_qos { >> + struct hlist_node node; >> + struct interconnect_path *path; >> + u32 bandwidth; >> +}; > > > Don't define this publicly. Should just be declared as an opaque handle.
Create a function for setting QoS/constrains on a node?
Thanks, Georgi
| |