mbox series

[v5,0/5] Add QCS404 interconnect provider driver

Message ID 20190723142339.27772-1-georgi.djakov@linaro.org
Headers show
Series Add QCS404 interconnect provider driver | expand

Message

Georgi Djakov July 23, 2019, 2:23 p.m. UTC 
Add drivers to support scaling of the on-chip interconnects on QCS404-based
platforms. Also add the necessary device-tree nodes, so that the driver for
each NoC can probe and register as interconnect-provider.

The plan is to get patches 2,5 through the Qcom/ARM tree and the rest
through the interconnect tree.

v5:
- Make reg and clocks DT properties required. (Bjorn)
- Remove _clk suffix from clock names. (Bjorn)
- Use the more succinct return !!icc_smd_rpm; (Bjorn)

v4: https://lore.kernel.org/lkml/20190613151323.10850-1-georgi.djakov@linaro.org/
- Move DT headers into the dt-bindings patch (Bjorn)
- Pick Bjorn's r-b on some patches.
- Return error if platform_device_register_data() fails (Bjorn)
- Use platform_set_drvdata() only in the child device. (Bjorn)
- Hide the smd-rpm proxy driver from config menu. (Bjorn)
- Add remove() function to zero out the rpm handle. (Bjorn)
- Move move the qcs404 driver patch later in the serie. (Bjorn)
- Insert the DT nodes after rng to keep the list sorted by address. (Bjorn)

v3: https://lore.kernel.org/lkml/20190611164157.24656-1-georgi.djakov@linaro.org/
- Drop the patch introducing the qcom,qos DT property.
- Add two new patches to create an interconnect proxy device. This device is
  part of the RPM hardware and handles the communication of the bus bandwidth
  requests.
- Add a DT reg property and move the interconnect nodes under the "soc" node.

v2: https://lore.kernel.org/lkml/20190415104357.5305-1-georgi.djakov@linaro.org/
- Use the clk_bulk API. (Bjorn)
- Move the port IDs into the provider file. (Bjorn)
- Use ARRAY_SIZE in the macro to automagically count the num_links. (Bjorn)
- Improve code readability. (Bjorn)
- Add patch [4/4] introducing a qcom,qos DT property to represent the link to
  the MMIO QoS registers HW block.

v1: https://lore.kernel.org/lkml/20190405035446.31886-1-georgi.djakov@linaro.org/


Bjorn Andersson (1):
  interconnect: qcom: Add QCS404 interconnect provider driver

Georgi Djakov (4):
  dt-bindings: interconnect: Add Qualcomm QCS404 DT bindings
  soc: qcom: smd-rpm: Create RPM interconnect proxy child device
  interconnect: qcom: Add interconnect SMD over SMD driver
  arm64: dts: qcs404: Add interconnect provider DT nodes

 .../bindings/interconnect/qcom,qcs404.txt     |  45 ++
 arch/arm64/boot/dts/qcom/qcs404.dtsi          |  28 +
 drivers/interconnect/qcom/Kconfig             |  12 +
 drivers/interconnect/qcom/Makefile            |   4 +
 drivers/interconnect/qcom/qcs404.c            | 539 ++++++++++++++++++
 drivers/interconnect/qcom/smd-rpm.c           |  77 +++
 drivers/interconnect/qcom/smd-rpm.h           |  15 +
 drivers/soc/qcom/smd-rpm.c                    |  17 +-
 .../dt-bindings/interconnect/qcom,qcs404.h    |  88 +++
 9 files changed, 824 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
 create mode 100644 drivers/interconnect/qcom/qcs404.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,qcs404.h

Comments

Brian Masney Aug. 25, 2019, 8:41 p.m. UTC | #1 
Hi Georgi and Bjorn,

I'm finishing up a msm8974 interconnect driver and I used qcs404 as a
starting point. I have a question below.

On Tue, Jul 23, 2019 at 05:23:38PM +0300, Georgi Djakov wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Add driver for the interconnect buses found in Qualcomm QCS404-based
> platforms. The topology consists of three NoCs that are controlled by
> a remote processor. This remote processor collects the aggregated
> bandwidth for each master-slave pairs.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---

[snip]

> +enum {
> +       QCS404_MASTER_AMPSS_M0 = 1,

[snip]

> +#define DEFINE_QNODE(_name, _id, _buswidth, _mas_rpm_id, _slv_rpm_id,	\
> +		     ...)						\
> +		static struct qcom_icc_node _name = {			\
> +		.name = #_name,						\
> +		.id = _id,						\
> +		.buswidth = _buswidth,					\
> +		.mas_rpm_id = _mas_rpm_id,				\
> +		.slv_rpm_id = _slv_rpm_id,				\
> +		.num_links = ARRAY_SIZE(((int[]){ __VA_ARGS__ })),	\
> +		.links = { __VA_ARGS__ },				\
> +	}
> +
> +DEFINE_QNODE(mas_apps_proc, QCS404_MASTER_AMPSS_M0, 8, 0, -1, QCS404_SLAVE_EBI_CH0, QCS404_BIMC_SNOC_SLV);

[snip]

> +static struct qcom_icc_node *qcs404_bimc_nodes[] = {
> +	[MASTER_AMPSS_M0] = &mas_apps_proc,

Should the id in DEFINE_QNODE() above be MASTER_AMPSS_M0 instead of
QCS404_MASTER_AMPSS_M0?

of_icc_xlate_onecell() looks up the id by the array index
MASTER_AMPSS_M0 (1), however qnoc_probe() passes the id that's
in struct qcom_icc_node to icc_node_create(), which is
QCS404_MASTER_AMPSS_M0 (0). They have different values and I'm unsure
why we can't just use the ids that are in qcom,qcs404.h and drop the
enum above.

Thanks,

Brian
Brian Masney Sept. 2, 2019, 9:29 p.m. UTC | #2 
On Sun, Aug 25, 2019 at 04:41:55PM -0400, Brian Masney wrote:
> Hi Georgi and Bjorn,
> 
> I'm finishing up a msm8974 interconnect driver and I used qcs404 as a
> starting point. I have a question below.
> 
> On Tue, Jul 23, 2019 at 05:23:38PM +0300, Georgi Djakov wrote:
> > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > Add driver for the interconnect buses found in Qualcomm QCS404-based
> > platforms. The topology consists of three NoCs that are controlled by
> > a remote processor. This remote processor collects the aggregated
> > bandwidth for each master-slave pairs.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> > ---
> 
> [snip]
> 
> > +enum {
> > +       QCS404_MASTER_AMPSS_M0 = 1,
> 
> [snip]
> 
> > +#define DEFINE_QNODE(_name, _id, _buswidth, _mas_rpm_id, _slv_rpm_id,	\
> > +		     ...)						\
> > +		static struct qcom_icc_node _name = {			\
> > +		.name = #_name,						\
> > +		.id = _id,						\
> > +		.buswidth = _buswidth,					\
> > +		.mas_rpm_id = _mas_rpm_id,				\
> > +		.slv_rpm_id = _slv_rpm_id,				\
> > +		.num_links = ARRAY_SIZE(((int[]){ __VA_ARGS__ })),	\
> > +		.links = { __VA_ARGS__ },				\
> > +	}
> > +
> > +DEFINE_QNODE(mas_apps_proc, QCS404_MASTER_AMPSS_M0, 8, 0, -1, QCS404_SLAVE_EBI_CH0, QCS404_BIMC_SNOC_SLV);
> 
> [snip]
> 
> > +static struct qcom_icc_node *qcs404_bimc_nodes[] = {
> > +	[MASTER_AMPSS_M0] = &mas_apps_proc,
> 
> Should the id in DEFINE_QNODE() above be MASTER_AMPSS_M0 instead of
> QCS404_MASTER_AMPSS_M0?
> 
> of_icc_xlate_onecell() looks up the id by the array index
> MASTER_AMPSS_M0 (1), however qnoc_probe() passes the id that's
> in struct qcom_icc_node to icc_node_create(), which is
> QCS404_MASTER_AMPSS_M0 (0). They have different values and I'm unsure
> why we can't just use the ids that are in qcom,qcs404.h and drop the
> enum above.

I dug through the interconnect code some more and I now see why these
separate IDs are needed.

I posted a work in progress msm8974 interconnect driver:
https://lore.kernel.org/lkml/20190902211925.27169-1-masneyb@onstation.org/T/#t

Brian