mbox series

[v9,0/8] Introduce on-chip interconnect API

Message ID 20180831140151.13972-1-georgi.djakov@linaro.org
Headers show
Series Introduce on-chip interconnect API | expand

Message

Georgi Djakov Aug. 31, 2018, 2:01 p.m. UTC
Modern SoCs have multiple processors and various dedicated cores (video, gpu,
graphics, modem). These cores are talking to each other and can generate a
lot of data flowing through the on-chip interconnects. These interconnect
buses could form different topologies such as crossbar, point to point buses,
hierarchical buses or use the network-on-chip concept.

These buses have been sized usually to handle use cases with high data
throughput but it is not necessary all the time and consume a lot of power.
Furthermore, the priority between masters can vary depending on the running
use case like video playback or CPU intensive tasks.

Having an API to control the requirement of the system in terms of bandwidth
and QoS, so we can adapt the interconnect configuration to match those by
scaling the frequencies, setting link priority and tuning QoS parameters.
This configuration can be a static, one-time operation done at boot for some
platforms or a dynamic set of operations that happen at run-time.

This patchset introduce a new API to get the requirement and configure the
interconnect buses across the entire chipset to fit with the current demand.
The API is NOT for changing the performance of the endpoint devices, but only
the interconnect path in between them.

The API is using a consumer/provider-based model, where the providers are
the interconnect buses and the consumers could be various drivers.
The consumers request interconnect resources (path) to an endpoint and set
the desired constraints on this data flow path. The provider(s) receive
requests from consumers and aggregate these requests for all master-slave
pairs on that path. Then the providers configure each participating in the
topology node according to the requested data flow path, physical links and
constraints. The topology could be complicated and multi-tiered and is SoC
specific.

Below is a simplified diagram of a real-world SoC topology. The interconnect
providers are the NoCs.

+----------------+    +----------------+
| HW Accelerator |--->|      M NoC     |<---------------+
+----------------+    +----------------+                |
                        |      |                    +------------+
 +-----+  +-------------+      V       +------+     |            |
 | DDR |  |                +--------+  | PCIe |     |            |
 +-----+  |                | Slaves |  +------+     |            |
   ^ ^    |                +--------+     |         |   C NoC    |
   | |    V                               V         |            |
+------------------+   +------------------------+   |            |   +-----+
|                  |-->|                        |-->|            |-->| CPU |
|                  |-->|                        |<--|            |   +-----+
|     Mem NoC      |   |         S NoC          |   +------------+
|                  |<--|                        |---------+    |
|                  |<--|                        |<------+ |    |   +--------+
+------------------+   +------------------------+       | |    +-->| Slaves |
  ^  ^    ^    ^          ^                             | |        +--------+
  |  |    |    |          |                             | V
+------+  |  +-----+   +-----+  +---------+   +----------------+   +--------+
| CPUs |  |  | GPU |   | DSP |  | Masters |-->|       P NoC    |-->| Slaves |
+------+  |  +-----+   +-----+  +---------+   +----------------+   +--------+
          |
      +-------+
      | Modem |
      +-------+

TODO:
* Create icc_set_extended() to handle parameters such as latency and other
  QoS values.
* Convert from using global node identifiers to local per provider ids.
* Cache the path between the nodes instead of walking the graph on each get().
* Sync interconnect requests with the idle state of the device.

Changes since patchset v8 (https://lkml.org/lkml/2018/8/10/387)
* Fixed the names of the files when built as modules.
* Corrected some typos in comments.

Changes since patchset v7 (https://lkml.org/lkml/2018/7/31/647)
* Addressed comments on kernel-doc and grammar. (Randy)
* Picked Reviewed-by: Evan
* Squashed consumer and provider DT bindings into single patch. (Rob)
* Cleaned-up msm8916 DT bindings docs by removing unused port ids.
* Updated documentation for the cases when NULL is returned. (Saravana)
* New patch to add myself as maintainer.

Changes since patchset v6 (https://lkml.org/lkml/2018/7/9/698)
* [patches 1,6]: Move the aggregation within the provider from the framework to
  the platform driver's set() callback, as the aggregation point could be SoC
  specific.
* [patch 1]: Include missing header, reset state only of the traversed nodes,
  move more code into path_init(), add more asserts, move misplaced mutex,
  simplify icc_link_destroy() (Evan)
* [patch 1]: Fix the order of requests to go from source to destination. (Alex)
* [patch 7]: Use better wording in the documentation. (Evan)
* [patch 6]: Reorder struct members, sort nodes alphabetically, improve naming
  of variables , add missing clk_disable_unprepare() in error paths. (Matthias)
* [patch 6]: Remove redundant NULL pointer check in msm8916 driver. (Alex)
* [patch 6]: Add missing depend on QCOM_SMD_RPM in Kconfig. (Evan)
* [patch 3]: Don't check for errors on debugfs calls, remove debugfs directory
  when module is unloaded (Greg)

Changes since patchset v5 (https://lkml.org/lkml/2018/6/20/453)
* Fix the modular build, make rpm-smd driver a module.
* Optimize locking and move to higher level. (Evan)
* Code cleanups. Fix typos. (Evan, Matthias)
* Add the source node to the path. (Evan)
* Rename path_allocate() to path_init() with minor refactoring. (Evan)
* Rename *_remove() functions to *_destroy().
* Return fixed errors in icc_link_destroy(). (Evan)
* Fix krealloc() usage in icc_link_destroy(). (Evan)
* Add missing kfree() in icc_node_create(). (Matthias)
* Make icc_node_add() return void. (Matthias)
* Change mutex_init to mutex_lock in icc_provider_add(). (Matthias)
* Add new icc_node_del() function to delete nodes from provider.
* Fix the header guard to reflect the path in smd-rpm.h. (Evan)
* Check for errors returned by qcom_icc_rpm_smd_send(). (Evan)
* Propagate the error of icc_provider_del(). (Evan)

Changes since patchset v4 (https://lkml.org/lkml/2018/3/9/856)
* Simplified locking by using a single global mutex. (Evan)
* Changed the aggregation function interface.
* Implemented functions for node, link, provider removal. (Evan)
* Naming changes on variables and functions, removed redundant code. (Evan)
* Fixes and clarifications in the docs. (Matthias, Evan, Amit, Alexandre)
* Removed mandatory reg DT property, made interconnect-names optional. (Bjorn)
* Made interconnect-cells property required to align with other bindings. (Neil)
* Moved msm8916 specific bindings into a separate file and patch. (Bjorn)
* Use the names, instead of the hardcoded ids for topology. (Matthias)
* Init the node before creating the links. (Evan)
* Added icc_units_to_bps macro. (Amit)

Changes since patchset v3 (https://lkml.org/lkml/2017/9/8/544)
* Refactored the constraints aggregation.
* Use the IDR API.
* Split the provider and consumer bindings into separate patches and propose
  new bindings for consumers, which allows to specify the local source port.
* Adopted the icc_ prefix for API functions.
* Introduced separate API functions for creating interconnect nodes and links.
* Added DT lookup support in addition to platform data.
* Dropped the event tracing patch for now.
* Added a patch to provide summary via debugfs.
* Use macro for the list of topology definitions in the platform driver.
* Various minor changes.

Changes since patchset v2 (https://lkml.org/lkml/2017/7/20/825)
* Split the aggregation into per node and per provider. Cache the
  aggregated values.
* Various small refactorings and cleanups in the framework.
* Added a patch introducing basic tracepoint support for monitoring
  the time required to update the interconnect nodes.

Changes since patchset v1 (https://lkml.org/lkml/2017/6/27/890)
* Updates in the documentation.
* Changes in request aggregation, locking.
* Dropped the aggregate() callback and use the default as it currently
  sufficient for the single vendor driver. Will add it later when needed.
* Dropped the dt-bindings draft patch for now.

Changes since RFC v2 (https://lkml.org/lkml/2017/6/12/316)
* Converted documentation to rst format.
* Fixed an incorrect call to mutex_lock. Renamed max_bw to peak_bw.

Changes since RFC v1 (https://lkml.org/lkml/2017/5/15/605)
* Refactored code into shorter functions.
* Added a new aggregate() API function.
* Rearranged some structs to reduce padding bytes.

Changes since RFC v0 (https://lkml.org/lkml/2017/3/1/599)
* Removed DT support and added optional Patch 3 with new bindings proposal.
* Converted the topology into internal driver data.
* Made the framework modular.
* interconnect_get() now takes (src and dst ports as arguments).
* Removed public declarations of some structs.
* Now passing prev/next nodes to the vendor driver.
* Properly remove requests on _put().
* Added refcounting.
* Updated documentation.
* Changed struct interconnect_path to use array instead of linked list.

Georgi Djakov (8):
  interconnect: Add generic on-chip interconnect API
  dt-bindings: Introduce interconnect binding
  interconnect: Allow endpoints translation via DT
  interconnect: Add debugfs support
  interconnect: qcom: Add RPM communication
  dt-bindings: interconnect: Document qcom,msm8916 NoC bindings
  interconnect: qcom: Add msm8916 interconnect provider driver
  MAINTAINERS: add a maintainer for the interconnect API

 .../bindings/interconnect/interconnect.txt    |  60 ++
 .../bindings/interconnect/qcom-msm8916.txt    |  41 +
 .../bindings/interconnect/qcom-smd.txt        |  32 +
 Documentation/interconnect/interconnect.rst   |  94 +++
 MAINTAINERS                                   |  10 +
 drivers/Kconfig                               |   2 +
 drivers/Makefile                              |   1 +
 drivers/interconnect/Kconfig                  |  15 +
 drivers/interconnect/Makefile                 |   6 +
 drivers/interconnect/core.c                   | 729 ++++++++++++++++++
 drivers/interconnect/qcom/Kconfig             |  22 +
 drivers/interconnect/qcom/Makefile            |   7 +
 drivers/interconnect/qcom/msm8916.c           | 510 ++++++++++++
 drivers/interconnect/qcom/smd-rpm.c           |  91 +++
 drivers/interconnect/qcom/smd-rpm.h           |  15 +
 include/dt-bindings/interconnect/qcom.h       |  98 +++
 include/linux/interconnect-provider.h         | 125 +++
 include/linux/interconnect.h                  |  49 ++
 18 files changed, 1907 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interconnect/interconnect.txt
 create mode 100644 Documentation/devicetree/bindings/interconnect/qcom-msm8916.txt
 create mode 100644 Documentation/devicetree/bindings/interconnect/qcom-smd.txt
 create mode 100644 Documentation/interconnect/interconnect.rst
 create mode 100644 drivers/interconnect/Kconfig
 create mode 100644 drivers/interconnect/Makefile
 create mode 100644 drivers/interconnect/core.c
 create mode 100644 drivers/interconnect/qcom/Kconfig
 create mode 100644 drivers/interconnect/qcom/Makefile
 create mode 100644 drivers/interconnect/qcom/msm8916.c
 create mode 100644 drivers/interconnect/qcom/smd-rpm.c
 create mode 100644 drivers/interconnect/qcom/smd-rpm.h
 create mode 100644 include/dt-bindings/interconnect/qcom.h
 create mode 100644 include/linux/interconnect-provider.h
 create mode 100644 include/linux/interconnect.h

Comments

Stephen Rothwell Sept. 4, 2018, 11:36 p.m. UTC | #1
Hi all,

On Tue, 4 Sep 2018 15:54:27 +0530 Amit Kucheria <amit.kucheria@linaro.org> wrote:
>

> I'm currently reviewing this patchset (long overdue), but considering

> that we haven't added any major new features to the framework for the

> last couple of revisions, can you get this patchset merged into

> linux-next to see how things shake out there? We've had this merged

> branch merged into a CI build on kernelci for a while now w/o any

> major incident so we should increase its exposure.

> 

> You could ask Stephen Rothwell if he'll accept the a branch directly

> from you or he needs an upstream maintainer (GregKH?) to carry it.


Since it appears to have been well reviewed and tested, I can take it
if you send me a git URL and the names of contacts in case of issues
with the branch.  Once Greg has merged it, I will try to drop it again
(unless you think there will be an ongoing reason to keep it in
linux-next separately), but a prompting email would also be nice in
case I forget.
-- 
Cheers,
Stephen Rothwell
Rob Herring (Arm) Sept. 25, 2018, 6:22 p.m. UTC | #2
On Fri, Aug 31, 2018 at 05:01:49PM +0300, Georgi Djakov wrote:
> Document the device-tree bindings of the Network-On-Chip interconnect

> hardware found on Qualcomm msm8916 platforms.

> 

> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>

> ---

>  .../bindings/interconnect/qcom-msm8916.txt    | 41 ++++++++

>  include/dt-bindings/interconnect/qcom.h       | 98 +++++++++++++++++++

>  2 files changed, 139 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/interconnect/qcom-msm8916.txt

>  create mode 100644 include/dt-bindings/interconnect/qcom.h

> 

> diff --git a/Documentation/devicetree/bindings/interconnect/qcom-msm8916.txt b/Documentation/devicetree/bindings/interconnect/qcom-msm8916.txt

> new file mode 100644

> index 000000000000..744df51df4ed

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/interconnect/qcom-msm8916.txt

> @@ -0,0 +1,41 @@

> +Qualcomm MSM8916 Network-On-Chip interconnect driver binding

> +----------------------------------------------------

> +

> +Required properties :

> +- compatible : shall contain only one of the following:

> +			"qcom,msm8916-bimc"

> +			"qcom,msm8916-pnoc"

> +			"qcom,msm8916-snoc"

> +- #interconnect-cells : should contain 1

> +- reg : shall contain base register location and length

> +

> +Optional properties :

> +clocks : list of phandles and specifiers to all interconnect bus clocks

> +clock-names : clock names should include both "bus_clk" and "bus_a_clk"

> +

> +Examples:

> +

> +		snoc: snoc@580000 {


interconnect@...

> +			compatible = "qcom,msm8916-snoc";

> +			#interconnect-cells = <1>;

> +			reg = <0x580000 0x14000>;

> +			clock-names = "bus_clk", "bus_a_clk";

> +			clocks = <&rpmcc RPM_SMD_SNOC_CLK>,

> +				 <&rpmcc RPM_SMD_SNOC_A_CLK>;

> +		};

> +		bimc: bimc@400000 {


interconnect@...

> +			compatible = "qcom,msm8916-bimc";

> +			#interconnect-cells = <1>;

> +			reg = <0x400000 0x62000>;

> +			clock-names = "bus_clk", "bus_a_clk";

> +			clocks = <&rpmcc RPM_SMD_BIMC_CLK>,

> +				 <&rpmcc RPM_SMD_BIMC_A_CLK>;

> +		};

> +		pnoc: pnoc@500000 {


and here.

> +			compatible = "qcom,msm8916-pnoc";

> +			#interconnect-cells = <1>;

> +			reg = <0x500000 0x11000>;

> +			clock-names = "bus_clk", "bus_a_clk";

> +			clocks = <&rpmcc RPM_SMD_PCNOC_CLK>,

> +				 <&rpmcc RPM_SMD_PCNOC_A_CLK>;

> +		};

> diff --git a/include/dt-bindings/interconnect/qcom.h b/include/dt-bindings/interconnect/qcom.h

> new file mode 100644

> index 000000000000..48f944b30e5d

> --- /dev/null

> +++ b/include/dt-bindings/interconnect/qcom.h

> @@ -0,0 +1,98 @@

> +/* SPDX-License-Identifier: GPL-2.0 */

> +/*

> + * Qualcomm interconnect IDs

> + *

> + * Copyright (c) 2018, Linaro Ltd.

> + * Author: Georgi Djakov <georgi.djakov@linaro.org>

> + */

> +

> +#ifndef __DT_BINDINGS_INTERCONNECT_QCOM_H

> +#define __DT_BINDINGS_INTERCONNECT_QCOM_H

> +

> +#define BIMC_SNOC_MAS			1

> +#define BIMC_SNOC_SLV			2

> +#define MASTER_AMPSS_M0			3

> +#define MASTER_BLSP_1			4

> +#define MASTER_CRYPTO_CORE0		5

> +#define MASTER_DEHR			6

> +#define MASTER_GRAPHICS_3D		7

> +#define MASTER_JPEG			8

> +#define MASTER_LPASS			9

> +#define MASTER_MDP_PORT0		10

> +#define MASTER_QDSS_BAM			11

> +#define MASTER_QDSS_ETR			12

> +#define MASTER_SDCC_1			13

> +#define MASTER_SDCC_2			14

> +#define MASTER_SNOC_CFG			15

> +#define MASTER_SPDM			16

> +#define MASTER_TCU_0			17

> +#define MASTER_TCU_1			18

> +#define MASTER_USB_HS			19

> +#define MASTER_VFE			20

> +#define MASTER_VIDEO_P0			21

> +#define PNOC_INT_0			22

> +#define PNOC_INT_1			23

> +#define PNOC_M_0			24

> +#define PNOC_M_1			25

> +#define PNOC_SLV_0			26

> +#define PNOC_SLV_1			27

> +#define PNOC_SLV_2			28

> +#define PNOC_SLV_3			29

> +#define PNOC_SLV_4			30

> +#define PNOC_SLV_8			31

> +#define PNOC_SLV_9			32

> +#define PNOC_SNOC_MAS			33

> +#define PNOC_SNOC_SLV			34

> +#define SLAVE_AMPSS_L2			35

> +#define SLAVE_BIMC_CFG			36

> +#define SLAVE_BLSP_1			37

> +#define SLAVE_BOOT_ROM			38

> +#define SLAVE_CAMERA_CFG		39

> +#define SLAVE_CATS_128			40

> +#define SLAVE_CLK_CTL			41

> +#define SLAVE_CRYPTO_0_CFG		42

> +#define SLAVE_DEHR_CFG			43

> +#define SLAVE_DISPLAY_CFG		44

> +#define SLAVE_EBI_CH0			45

> +#define SLAVE_GRAPHICS_3D_CFG		46

> +#define SLAVE_IMEM_CFG			47

> +#define SLAVE_LPASS			48

> +#define SLAVE_MPM			49

> +#define SLAVE_MSM_PDM			50

> +#define SLAVE_MSM_TCSR			51

> +#define SLAVE_MSS			52

> +#define SLAVE_OCMEM_64			53

> +#define SLAVE_PMIC_ARB			54

> +#define SLAVE_PNOC_CFG			55

> +#define SLAVE_PRNG			56

> +#define SLAVE_QDSS_CFG			57

> +#define SLAVE_QDSS_STM			58

> +#define SLAVE_RBCPR_CFG			59

> +#define SLAVE_RPM_MSG_RAM		60

> +#define SLAVE_SDCC_1			61

> +#define SLAVE_SDCC_4			62

> +#define SLAVE_SECURITY			63

> +#define SLAVE_SERVICE_SNOC		64

> +#define SLAVE_SNOC_CFG			65

> +#define SLAVE_SPDM			66

> +#define SLAVE_SYSTEM_IMEM		67

> +#define SLAVE_TLMM			68

> +#define SLAVE_USB_HS			69

> +#define SLAVE_VENUS_CFG			70

> +#define SNOC_BIMC_0_MAS			71

> +#define SNOC_BIMC_0_SLV			72

> +#define SNOC_BIMC_1_MAS			73

> +#define SNOC_BIMC_1_SLV			74

> +#define SNOC_INT_0			75

> +#define SNOC_INT_1			76

> +#define SNOC_INT_BIMC			77

> +#define SNOC_MM_INT_0			78

> +#define SNOC_MM_INT_1			79

> +#define SNOC_MM_INT_2			80

> +#define SNOC_MM_INT_BIMC		81

> +#define SNOC_PNOC_MAS			82

> +#define SNOC_PNOC_SLV			83

> +#define SNOC_QDSS_INT			84

> +#define SYSTEM_SLAVE_FAB_APPS		85


Each of the 3 providers in the example should have their own number 
space. This looks like you have combined them all. Though that is hard 
to tell because I can't really decipher the naming convention 
completely.

Rob
Sudeep Holla Sept. 26, 2018, 2:48 p.m. UTC | #3
On Wed, Sep 26, 2018 at 05:42:15PM +0300, Georgi Djakov wrote:
> Hi Rob,

> 

> Thanks for the comments!

> 

> On 09/25/2018 09:02 PM, Rob Herring wrote:

> > On Fri, Aug 31, 2018 at 05:01:45PM +0300, Georgi Djakov wrote:

> >> This binding is intended to represent the relations between the interconnect

> >> controllers (providers) and consumer device nodes. It will allow creating links

> >> between consumers and interconnect paths (exposed by interconnect providers).

> > 

> > As I mentioned in person, I want to see other SoC families using this 

> > before accepting. They don't have to be ready for upstream, but WIP 

> > patches or even just a "yes, this works for us and we're going to use 

> > this binding on X".

> 

> Other than the 3 Qualcomm SoCs (msm8916, msm8996, sdm845) that are

> currently using this binding, there is ongoing work from at least two

> other vendors that would be using this same binding. I will check on

> what is their progress so far.

> 

> > Also, I think the QCom GPU use of this should be fully sorted out. Or 

> > more generically how this fits into OPP binding which seems to be never 

> > ending extended...

> 

> I see this as a further step. It could be OPP binding which include

> bandwidth values or some separate DT property. Jordan has already

> proposed something, do you have any initial comments on that?


I am curious as how this fits into new systems which have firmware driven
CPUFreq and other DVFS. I would like to avoid using this in such systems
and leave it upto the firmware to scale the bus/interconnect based on the
other components that are connected to it and active.

--
Regards,
Sudeep
Saravana Kannan Oct. 1, 2018, 9:51 p.m. UTC | #4
On 10/01/2018 02:26 PM, Jordan Crouse wrote:
> On Mon, Oct 01, 2018 at 01:56:32PM -0700, Saravana Kannan wrote:

>>

>> On 09/26/2018 07:34 AM, Jordan Crouse wrote:

>>> On Tue, Sep 25, 2018 at 01:02:15PM -0500, Rob Herring wrote:

>>>> On Fri, Aug 31, 2018 at 05:01:45PM +0300, Georgi Djakov wrote:

>>>>> This binding is intended to represent the relations between the interconnect

>>>>> controllers (providers) and consumer device nodes. It will allow creating links

>>>>> between consumers and interconnect paths (exposed by interconnect providers).

>>>> As I mentioned in person, I want to see other SoC families using this

>>>> before accepting. They don't have to be ready for upstream, but WIP

>>>> patches or even just a "yes, this works for us and we're going to use

>>>> this binding on X".

>>>>

>>>> Also, I think the QCom GPU use of this should be fully sorted out. Or

>>>> more generically how this fits into OPP binding which seems to be never

>>>> ending extended...

>>> This is a discussion I wouldn't mind having now.  To jog memories, this is what

>>> I posted a few weeks ago:

>>>

>>> https://patchwork.freedesktop.org/patch/246117/

>>>

>>> This seems like the easiest way to me to tie the frequency and the bandwidth

>>> quota together for GPU devfreq scaling but I'm not married to the format and

>>> I'll happily go a few rounds on the bikeshed if we can get something we can

>>> be happy with.

>>>

>>> Jordan

>> Been meaning to send this out for a while, but caught up with other stuff.

>>

>> That GPU BW patch is very specific to device to device mapping and

>> doesn't work well for different use cases (Eg: those that  can

>> calculate based on use case, etc).

>>

>> Interconnect paths have different BW (bandwidth) operating points

>> that they can support. For example: 1 GB/s, 1.7 GB/s, 5GB/s, etc.

>> Having a mapping from GPU or CPU to those are fine/necessary, but we

>> still need a separate BW OPP table for interconnect paths to list

>> what they can actually support.

>>

>> Two different ways we could represent BW OPP tables for interconnect paths:

>> 1.  Represent interconnect paths (CPU to DDR, GPU to DDR, etc) as

>> devices and have OPPs for those devices.

>>

>> 2. We can have a "interconnect-opp-tables" DT binding similar to

>> "interconnects" and "interconnect-names". So if a device GPU or

>> Video decoder or I2C device needs to vote on an interconnect path,

>> they can also list the OPP tables that those paths can support.

>>

>> I know Rob doesn't like (1). But I'm hoping at least (2) is

>> acceptable. I'm open to other suggestions too.

>>

>> Both (1) and (2) need BW OPP tables similar to frequency OPP tables.

>> That should be easy to add and Viresh is open to that. I'm open to

>> other options too, but the fundamental missing part is how to tie a

>> list of BW OPPs to interconnect paths in DT.

>>

>> Once we have one of the above two options, we can use the

>> required-opps field (already present in kernel) for the mapping

>> between GPU to a particular BW need (suggested by Viresh during an

>> in person conversation).

> Assuming we are willing to maintain the bandwidth OPP tables and the

> names / phandles needed to describe a 1:1 GPU -> bandwidth mapping

> I'm okay with required-opps but for the sake of argument how would

> required-opps work for a device that needs to vote multiple paths

> for a given OPP?


You can list multiple required-opps per device OPP. It's an array of 
phandles to OPP entries in other tables.

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Saravana Kannan Oct. 1, 2018, 11:49 p.m. UTC | #5
On 09/26/2018 07:48 AM, Sudeep Holla wrote:
> On Wed, Sep 26, 2018 at 05:42:15PM +0300, Georgi Djakov wrote:

>> Hi Rob,

>>

>> Thanks for the comments!

>>

>> On 09/25/2018 09:02 PM, Rob Herring wrote:

>>> On Fri, Aug 31, 2018 at 05:01:45PM +0300, Georgi Djakov wrote:

>>>> This binding is intended to represent the relations between the interconnect

>>>> controllers (providers) and consumer device nodes. It will allow creating links

>>>> between consumers and interconnect paths (exposed by interconnect providers).

>>> As I mentioned in person, I want to see other SoC families using this

>>> before accepting. They don't have to be ready for upstream, but WIP

>>> patches or even just a "yes, this works for us and we're going to use

>>> this binding on X".

>> Other than the 3 Qualcomm SoCs (msm8916, msm8996, sdm845) that are

>> currently using this binding, there is ongoing work from at least two

>> other vendors that would be using this same binding. I will check on

>> what is their progress so far.

>>

>>> Also, I think the QCom GPU use of this should be fully sorted out. Or

>>> more generically how this fits into OPP binding which seems to be never

>>> ending extended...

>> I see this as a further step. It could be OPP binding which include

>> bandwidth values or some separate DT property. Jordan has already

>> proposed something, do you have any initial comments on that?

> I am curious as how this fits into new systems which have firmware driven

> CPUFreq and other DVFS. I would like to avoid using this in such systems

> and leave it upto the firmware to scale the bus/interconnect based on the

> other components that are connected to it and active.

>


You've made the same point multiple times across different patch sets. 
Not all FW can do arbitrary functions. A lot of them are very limited in 
their capabilities. So, as much as you and I would like to let the FW do 
the work, it's not always possible. So, in those cases, we do need to 
have support for the kernel scaling the interconnects correctly. 
Hopefully this clears up your questions about FW capabilities.

Thanks,
Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Georgi Djakov Oct. 2, 2018, 11:02 a.m. UTC | #6
Hi Rob,

On 09/25/2018 09:22 PM, Rob Herring wrote:
> On Fri, Aug 31, 2018 at 05:01:49PM +0300, Georgi Djakov wrote:

>> Document the device-tree bindings of the Network-On-Chip interconnect

>> hardware found on Qualcomm msm8916 platforms.

>>

>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>

>> ---

>>  .../bindings/interconnect/qcom-msm8916.txt    | 41 ++++++++

>>  include/dt-bindings/interconnect/qcom.h       | 98 +++++++++++++++++++

>>  2 files changed, 139 insertions(+)

>>  create mode 100644 Documentation/devicetree/bindings/interconnect/qcom-msm8916.txt

>>  create mode 100644 include/dt-bindings/interconnect/qcom.h

>>

>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom-msm8916.txt b/Documentation/devicetree/bindings/interconnect/qcom-msm8916.txt

>> new file mode 100644

>> index 000000000000..744df51df4ed

>> --- /dev/null

>> +++ b/Documentation/devicetree/bindings/interconnect/qcom-msm8916.txt

>> @@ -0,0 +1,41 @@

>> +Qualcomm MSM8916 Network-On-Chip interconnect driver binding

>> +----------------------------------------------------

>> +

>> +Required properties :

>> +- compatible : shall contain only one of the following:

>> +			"qcom,msm8916-bimc"

>> +			"qcom,msm8916-pnoc"

>> +			"qcom,msm8916-snoc"

>> +- #interconnect-cells : should contain 1

>> +- reg : shall contain base register location and length

>> +

>> +Optional properties :

>> +clocks : list of phandles and specifiers to all interconnect bus clocks

>> +clock-names : clock names should include both "bus_clk" and "bus_a_clk"

>> +

>> +Examples:

>> +

>> +		snoc: snoc@580000 {

> 

> interconnect@...


Ok.

> 

>> +			compatible = "qcom,msm8916-snoc";

>> +			#interconnect-cells = <1>;

>> +			reg = <0x580000 0x14000>;

>> +			clock-names = "bus_clk", "bus_a_clk";

>> +			clocks = <&rpmcc RPM_SMD_SNOC_CLK>,

>> +				 <&rpmcc RPM_SMD_SNOC_A_CLK>;

>> +		};

>> +		bimc: bimc@400000 {

> 

> interconnect@...


Ok.

> 

>> +			compatible = "qcom,msm8916-bimc";

>> +			#interconnect-cells = <1>;

>> +			reg = <0x400000 0x62000>;

>> +			clock-names = "bus_clk", "bus_a_clk";

>> +			clocks = <&rpmcc RPM_SMD_BIMC_CLK>,

>> +				 <&rpmcc RPM_SMD_BIMC_A_CLK>;

>> +		};

>> +		pnoc: pnoc@500000 {

> 

> and here.


Ok.

> 

>> +			compatible = "qcom,msm8916-pnoc";

>> +			#interconnect-cells = <1>;

>> +			reg = <0x500000 0x11000>;

>> +			clock-names = "bus_clk", "bus_a_clk";

>> +			clocks = <&rpmcc RPM_SMD_PCNOC_CLK>,

>> +				 <&rpmcc RPM_SMD_PCNOC_A_CLK>;

>> +		};

>> diff --git a/include/dt-bindings/interconnect/qcom.h b/include/dt-bindings/interconnect/qcom.h

>> new file mode 100644

>> index 000000000000..48f944b30e5d

>> --- /dev/null

>> +++ b/include/dt-bindings/interconnect/qcom.h

>> @@ -0,0 +1,98 @@

>> +/* SPDX-License-Identifier: GPL-2.0 */

>> +/*

>> + * Qualcomm interconnect IDs

>> + *

>> + * Copyright (c) 2018, Linaro Ltd.

>> + * Author: Georgi Djakov <georgi.djakov@linaro.org>

>> + */

>> +

>> +#ifndef __DT_BINDINGS_INTERCONNECT_QCOM_H

>> +#define __DT_BINDINGS_INTERCONNECT_QCOM_H

>> +

>> +#define BIMC_SNOC_MAS			1

>> +#define BIMC_SNOC_SLV			2

>> +#define MASTER_AMPSS_M0			3

>> +#define MASTER_BLSP_1			4

>> +#define MASTER_CRYPTO_CORE0		5

>> +#define MASTER_DEHR			6

>> +#define MASTER_GRAPHICS_3D		7

>> +#define MASTER_JPEG			8

>> +#define MASTER_LPASS			9

>> +#define MASTER_MDP_PORT0		10

>> +#define MASTER_QDSS_BAM			11

>> +#define MASTER_QDSS_ETR			12

>> +#define MASTER_SDCC_1			13

>> +#define MASTER_SDCC_2			14

>> +#define MASTER_SNOC_CFG			15

>> +#define MASTER_SPDM			16

>> +#define MASTER_TCU_0			17

>> +#define MASTER_TCU_1			18

>> +#define MASTER_USB_HS			19

>> +#define MASTER_VFE			20

>> +#define MASTER_VIDEO_P0			21

>> +#define PNOC_INT_0			22

>> +#define PNOC_INT_1			23

>> +#define PNOC_M_0			24

>> +#define PNOC_M_1			25

>> +#define PNOC_SLV_0			26

>> +#define PNOC_SLV_1			27

>> +#define PNOC_SLV_2			28

>> +#define PNOC_SLV_3			29

>> +#define PNOC_SLV_4			30

>> +#define PNOC_SLV_8			31

>> +#define PNOC_SLV_9			32

>> +#define PNOC_SNOC_MAS			33

>> +#define PNOC_SNOC_SLV			34

>> +#define SLAVE_AMPSS_L2			35

>> +#define SLAVE_BIMC_CFG			36

>> +#define SLAVE_BLSP_1			37

>> +#define SLAVE_BOOT_ROM			38

>> +#define SLAVE_CAMERA_CFG		39

>> +#define SLAVE_CATS_128			40

>> +#define SLAVE_CLK_CTL			41

>> +#define SLAVE_CRYPTO_0_CFG		42

>> +#define SLAVE_DEHR_CFG			43

>> +#define SLAVE_DISPLAY_CFG		44

>> +#define SLAVE_EBI_CH0			45

>> +#define SLAVE_GRAPHICS_3D_CFG		46

>> +#define SLAVE_IMEM_CFG			47

>> +#define SLAVE_LPASS			48

>> +#define SLAVE_MPM			49

>> +#define SLAVE_MSM_PDM			50

>> +#define SLAVE_MSM_TCSR			51

>> +#define SLAVE_MSS			52

>> +#define SLAVE_OCMEM_64			53

>> +#define SLAVE_PMIC_ARB			54

>> +#define SLAVE_PNOC_CFG			55

>> +#define SLAVE_PRNG			56

>> +#define SLAVE_QDSS_CFG			57

>> +#define SLAVE_QDSS_STM			58

>> +#define SLAVE_RBCPR_CFG			59

>> +#define SLAVE_RPM_MSG_RAM		60

>> +#define SLAVE_SDCC_1			61

>> +#define SLAVE_SDCC_4			62

>> +#define SLAVE_SECURITY			63

>> +#define SLAVE_SERVICE_SNOC		64

>> +#define SLAVE_SNOC_CFG			65

>> +#define SLAVE_SPDM			66

>> +#define SLAVE_SYSTEM_IMEM		67

>> +#define SLAVE_TLMM			68

>> +#define SLAVE_USB_HS			69

>> +#define SLAVE_VENUS_CFG			70

>> +#define SNOC_BIMC_0_MAS			71

>> +#define SNOC_BIMC_0_SLV			72

>> +#define SNOC_BIMC_1_MAS			73

>> +#define SNOC_BIMC_1_SLV			74

>> +#define SNOC_INT_0			75

>> +#define SNOC_INT_1			76

>> +#define SNOC_INT_BIMC			77

>> +#define SNOC_MM_INT_0			78

>> +#define SNOC_MM_INT_1			79

>> +#define SNOC_MM_INT_2			80

>> +#define SNOC_MM_INT_BIMC		81

>> +#define SNOC_PNOC_MAS			82

>> +#define SNOC_PNOC_SLV			83

>> +#define SNOC_QDSS_INT			84

>> +#define SYSTEM_SLAVE_FAB_APPS		85

> 

> Each of the 3 providers in the example should have their own number 

> space. This looks like you have combined them all. Though that is hard 

> to tell because I can't really decipher the naming convention 

> completely.


That's the plan. Thanks for reviewing!

BR,
Georgi
Saravana Kannan Oct. 2, 2018, 6:56 p.m. UTC | #7
On 10/02/2018 04:17 AM, Sudeep Holla wrote:
> On Mon, Oct 01, 2018 at 04:49:32PM -0700, Saravana Kannan wrote:

>> On 09/26/2018 07:48 AM, Sudeep Holla wrote:

>>> On Wed, Sep 26, 2018 at 05:42:15PM +0300, Georgi Djakov wrote:

>>>> Hi Rob,

>>>>

>>>> Thanks for the comments!

>>>>

>>>> On 09/25/2018 09:02 PM, Rob Herring wrote:

>>>>> On Fri, Aug 31, 2018 at 05:01:45PM +0300, Georgi Djakov wrote:

>>>>>> This binding is intended to represent the relations between the interconnect

>>>>>> controllers (providers) and consumer device nodes. It will allow creating links

>>>>>> between consumers and interconnect paths (exposed by interconnect providers).

>>>>> As I mentioned in person, I want to see other SoC families using this

>>>>> before accepting. They don't have to be ready for upstream, but WIP

>>>>> patches or even just a "yes, this works for us and we're going to use

>>>>> this binding on X".

>>>> Other than the 3 Qualcomm SoCs (msm8916, msm8996, sdm845) that are

>>>> currently using this binding, there is ongoing work from at least two

>>>> other vendors that would be using this same binding. I will check on

>>>> what is their progress so far.

>>>>

>>>>> Also, I think the QCom GPU use of this should be fully sorted out. Or

>>>>> more generically how this fits into OPP binding which seems to be never

>>>>> ending extended...

>>>> I see this as a further step. It could be OPP binding which include

>>>> bandwidth values or some separate DT property. Jordan has already

>>>> proposed something, do you have any initial comments on that?

>>> I am curious as how this fits into new systems which have firmware driven

>>> CPUFreq and other DVFS. I would like to avoid using this in such systems

>>> and leave it upto the firmware to scale the bus/interconnect based on the

>>> other components that are connected to it and active.

>>>

>> You've made the same point multiple times across different patch sets. Not

>> all FW can do arbitrary functions. A lot of them are very limited in their

>> capabilities. So, as much as you and I would like to let the FW do the work,

>> it's not always possible. So, in those cases, we do need to have support for

>> the kernel scaling the interconnects correctly. Hopefully this clears up

>> your questions about FW capabilities.

> Yes, I do understand I have made the same point multiple time and it's

> intentional. We need to get the fragmented f/w support story fixed.

> Different ARM vendors are doing different things in f/w and ARM sees the

> same fragmentation story as before. We have come up with new specification

> and my annoying multiple emails are just to constantly remind the same.

>

> I do understand we have existing implementations to consider, but fixing

> the functionality in arbitrary way is not a good design and it better

> to get them fixed for future.


I believe the fragmentation you are referring to is  in the 
interface/communication protocol. I see the benefit of standardizing 
that as long as the standard actually turns out to be good. But that's 
completely separate from what the FW can/can't do. Asking to standardize 
what the FW can/can't do doesn't seem realistic as each chip vendor will 
have different priorities - power, performance, cost, chip area, etc. 
It's the conflation of these separate topics that doesn't help IMHO.

-Saravana


-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Saravana Kannan Oct. 3, 2018, 6:06 p.m. UTC | #8
On 10/03/2018 02:33 AM, Sudeep Holla wrote:
> On Tue, Oct 02, 2018 at 11:56:56AM -0700, Saravana Kannan wrote:

>> On 10/02/2018 04:17 AM, Sudeep Holla wrote:

> [...]

>

>>> Yes, I do understand I have made the same point multiple time and it's

>>> intentional. We need to get the fragmented f/w support story fixed.

>>> Different ARM vendors are doing different things in f/w and ARM sees the

>>> same fragmentation story as before. We have come up with new specification

>>> and my annoying multiple emails are just to constantly remind the same.

>>>

>>> I do understand we have existing implementations to consider, but fixing

>>> the functionality in arbitrary way is not a good design and it better

>>> to get them fixed for future.

>> I believe the fragmentation you are referring to is  in the

>> interface/communication protocol. I see the benefit of standardizing that as

>> long as the standard actually turns out to be good. But that's completely

>> separate from what the FW can/can't do. Asking to standardize what the FW

>> can/can't do doesn't seem realistic as each chip vendor will have different

>> priorities - power, performance, cost, chip area, etc. It's the conflation

>> of these separate topics that doesn't help IMHO.

> I agree on interface/communication protocol fragmentation and firmware

> can implement whatever the vendor wish. What I was also referring was

> the mix-n-match approach which should be avoided.

>

> e.g. Device A and B's PM is managed completely by firmware using OSPM hints

> Suppose Device X's PM is dependent on Device A and B, in which case it's

> simpler and cleaner to leave Device X PM to firmware. Reading the state

> of A and B and using that as hint for X is just overhead which firmware

> can manage better. That was my main concern here: A=CPU and B=some other

> device and X is inter-connect to which A and B are connected.

>

> If CPU OPPs are obtained from f/w and this inter-connect from DT, mapping

> then is a mess and that's what I was concerned. I am sorry if that's not

> the scenario here, I may have mistaken then.

>

What you are asking would be an ideal case, but this is not an ideal 
world. There are tons of constraints for each chip vendor. Saying you 
can't mix and match makes perfect the enemy of the good. Adding FW 
support for A and B might make them optimal. But adding support for X 
might not be possible for multiple real world constraints (chip area, 
cost, time to market, etc). Saying "either do it all or do nothing" is 
going to hold back a lot progress that can come in increments. Heck, we 
do the same thing in the kernel. We'll add basic simple features first 
and then improve on them. Why is it suddenly frowned up if a FW/HW 
follows the same approach? I'll just have to agree to disagree with you 
on this view point.

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project