On Tue 24 Sep 22:41 PDT 2019, Stephen Boyd wrote:
> I don't see any users of icc_get() in the kernel today, and adding them
> doesn't make sense. That's because adding calls to that function in a
> driver will make the driver SoC specific given that the arguments are
> some sort of source and destination numbers that would typically be
> listed in DT or come from platform data so they can match a global
> numberspace of interconnect numbers. It would be better to follow the
> approach of other kernel frameworks where the API is the same no matter
> how the platform is described (i.e. platform data, DT, ACPI, etc.) and
> swizzle the result in the framework to match whatever the device is by
> checking for a DT node pointer or a fwnode pointer, etc. Therefore,
> install icc_get() as the defacto API and make drivers use that instead
> of of_icc_get() which implies the driver is DT specific when it doesn't
> need to be.
>
+1 on this part!
> The DT binding could also be simplified somewhat. Currently a path needs
> to be specified in DT for each and every use case that is possible for a
> device to want. Typically the path is to memory, which looks to be
> reserved for in the binding with the "dma-mem" named path, but sometimes
> the path is from a device to the CPU or more generically from a device
> to another device which could be a CPU, cache, DMA master, or another
> device if some sort of DMA to DMA scenario is happening. Let's remove
> the pair part of the binding so that we just list out a device's
> possible endpoints on the bus or busses that it's connected to.
>
> If the kernel wants to figure out what the path is to memory or the CPU
> or a cache or something else it should be able to do that by finding the
> node for the "destination" endpoint, extracting that node's
> "interconnects" property, and deriving the path in software. For
> example, we shouldn't need to write out each use case path by path in DT
> for each endpoint node that wants to set a bandwidth to memory. We
> should just be able to indicate what endpoint(s) a device sits on based
> on the interconnect provider in the system and then walk the various
> interconnects to find the path from that source endpoint to the
> destination endpoint.
>
But doesn't this implies that the other end of the path is always some
specific node, e.g. DDR? With a single node how would you describe
CPU->LLCC or GPU->OCIMEM?
> Obviously this patch doesn't compile but I'm sending it out to start
> this discussion so we don't get stuck on the binding or the kernel APIs
> for a long time. It looks like we should be OK in terms of backwards
> compatibility because we can just ignore the second element in an old
> binding, but maybe we'll want to describe paths in different directions
> (e.g. the path from the CPU to the SD controller may be different than
> the path the SD controller takes to the CPU) and that may require
> extending interconnect-names to indicate what direction/sort of path it
> is. I'm basically thinking about master vs. slave ports in AXI land.
>
> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> Cc: <linux-pm@xxxxxxxxxxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: <devicetree@xxxxxxxxxxxxxxx>
> Cc: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> Cc: Evan Green <evgreen@xxxxxxxxxxxx>
> Cc: David Dai <daidavid1@xxxxxxxxxxxxxx>
> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> ---
> .../bindings/interconnect/interconnect.txt | 19 ++++---------------
> include/linux/interconnect.h | 13 ++-----------
> 2 files changed, 6 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt
> index 6f5d23a605b7..f8979186b8a7 100644
> --- a/Documentation/devicetree/bindings/interconnect/interconnect.txt
> +++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt
> @@ -11,7 +11,7 @@ The interconnect provider binding is intended to represent the interconnect
> controllers in the system. Each provider registers a set of interconnect
> nodes, which expose the interconnect related capabilities of the interconnect
> to consumer drivers. These capabilities can be throughput, latency, priority
> -etc. The consumer drivers set constraints on interconnect path (or endpoints)
> +etc. The consumer drivers set constraints on interconnect paths (or endpoints)
> depending on the use case. Interconnect providers can also be interconnect
> consumers, such as in the case where two network-on-chip fabrics interface
> directly.
> @@ -42,23 +42,12 @@ multiple paths from different providers depending on use case and the
> components it has to interact with.
>
> Required properties:
> -interconnects : Pairs of phandles and interconnect provider specifier to denote
> - the edge source and destination ports of the interconnect path.
> -
> -Optional properties:
> -interconnect-names : List of interconnect path name strings sorted in the same
> - order as the interconnects property. Consumers drivers will use
> - interconnect-names to match interconnect paths with interconnect
> - specifier pairs.
> -
> - Reserved interconnect names:
> - * dma-mem: Path from the device to the main memory of
> - the system
> +interconnects : phandle and interconnect provider specifier to denote
> + the edge source for this node.
>
> Example:
>
> sdhci@7864000 {
> ...
> - interconnects = <&pnoc MASTER_SDCC_1 &bimc SLAVE_EBI_CH0>;
> - interconnect-names = "sdhc-mem";
> + interconnects = <&pnoc MASTER_SDCC_1>;
This example seems incomplete, as it doesn't describe the path between
CPU and the config space, with this in place I think you need the
interconnect-names.
But with a single interconnect, the interconnect-names should be
omitted, as done in other frameworks.
> };
> diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> index d70a914cba11..e1ae704f5ab1 100644
> --- a/include/linux/interconnect.h
> +++ b/include/linux/interconnect.h
> @@ -25,23 +25,14 @@ struct device;
>
> #if IS_ENABLED(CONFIG_INTERCONNECT)
>
> -struct icc_path *icc_get(struct device *dev, const int src_id,
> - const int dst_id);
> -struct icc_path *of_icc_get(struct device *dev, const char *name);
> +struct icc_path *icc_get(struct device *dev, const char *name);
> void icc_put(struct icc_path *path);
> int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
> void icc_set_tag(struct icc_path *path, u32 tag);
>
> #else
>
> -static inline struct icc_path *icc_get(struct device *dev, const int src_id,
> - const int dst_id)
> -{
> - return NULL;
> -}
> -
> -static inline struct icc_path *of_icc_get(struct device *dev,
> - const char *name)
> +static inline struct icc_path *icc_get(struct device *dev, const char *name)
I like this part, if mimics what's done in other frameworks and removes
the ties to OF from the API.
Regards,
Bjorn
> {
> return NULL;
> }
>
> base-commit: b5b3bd898ba99fb0fb6aed3b23ec6353a1724d6f
> --
> Sent by a computer through tubes
>
