Re: [RFC PATCH] interconnect: Replace of_icc_get() with icc_get() and reduce DT binding

[Bjorn Andersson]

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
>