Re: [PATCH v3 3/3] interconnect: Add Qualcomm msm8916 interconnect provider driver

From: Amit Kucheria
Date: Thu Nov 02 2017 - 03:28:58 EST

Next message: Stephen Rothwell: "linux-next: manual merge of the akpm-current tree with the drm tree"
Previous message: Gilad Ben-Yossef: "Re: [PATCH 2/3] staging: ccree: handle limiting of DMA masks"
Next in thread: Georgi Djakov: "Re: [PATCH v3 3/3] interconnect: Add Qualcomm msm8916 interconnect provider driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Sep 8, 2017 at 10:48 PM, Georgi Djakov <georgi.djakov@xxxxxxxxxx> wrote:
> Add driver for the Qualcomm interconnect buses found in msm8916 based
> platforms. This patch contains only a partial topology to make reviewing
> easier.
>
> Signed-off-by: Georgi Djakov <georgi.djakov@xxxxxxxxxx>
> ---
> drivers/interconnect/Kconfig | 5 +
> drivers/interconnect/Makefile | 1 +
> drivers/interconnect/qcom/Kconfig | 11 +
> drivers/interconnect/qcom/Makefile | 1 +
> drivers/interconnect/qcom/interconnect_msm8916.c | 375 +++++++++++++++++++++++
> include/linux/interconnect/qcom-msm8916.h | 92 ++++++
> 6 files changed, 485 insertions(+)
> create mode 100644 drivers/interconnect/qcom/Kconfig
> create mode 100644 drivers/interconnect/qcom/Makefile
> create mode 100644 drivers/interconnect/qcom/interconnect_msm8916.c
> create mode 100644 include/linux/interconnect/qcom-msm8916.h
>
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 1e50e951cdc1..b123a76e2f9d 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -8,3 +8,8 @@ menuconfig INTERCONNECT
>
> If unsure, say no.
>
> +if INTERCONNECT
> +
> +source "drivers/interconnect/qcom/Kconfig"
> +
> +endif
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index d9da6a6c3560..62a01de24aeb 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -1 +1,2 @@
> obj-$(CONFIG_INTERCONNECT) += interconnect.o
> +obj-$(CONFIG_INTERCONNECT_QCOM) += qcom/
> diff --git a/drivers/interconnect/qcom/Kconfig b/drivers/interconnect/qcom/Kconfig
> new file mode 100644
> index 000000000000..86465dc37bd4
> --- /dev/null
> +++ b/drivers/interconnect/qcom/Kconfig
> @@ -0,0 +1,11 @@
> +config INTERCONNECT_QCOM
> + bool "Qualcomm Network-on-Chip interconnect drivers"
> + depends on INTERCONNECT
> + depends on ARCH_QCOM || COMPILE_TEST
> + default y
> +
> +config INTERCONNECT_QCOM_MSM8916
> + tristate "Qualcomm MSM8916 interconnect driver"
> + depends on INTERCONNECT_QCOM
> + help
> + This is a driver for the Qualcomm Network-on-Chip on msm8916-based platforms.
> diff --git a/drivers/interconnect/qcom/Makefile b/drivers/interconnect/qcom/Makefile
> new file mode 100644
> index 000000000000..0831080e1100
> --- /dev/null
> +++ b/drivers/interconnect/qcom/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_INTERCONNECT_QCOM_MSM8916) += interconnect_msm8916.o
> diff --git a/drivers/interconnect/qcom/interconnect_msm8916.c b/drivers/interconnect/qcom/interconnect_msm8916.c
> new file mode 100644
> index 000000000000..9b001e100974
> --- /dev/null
> +++ b/drivers/interconnect/qcom/interconnect_msm8916.c
> @@ -0,0 +1,375 @@
> +/*
> + * Copyright (C) 2017 Linaro Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/interconnect/qcom-msm8916.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define to_qcom_icp(_icp) \
> + container_of(_icp, struct qcom_interconnect_provider, icp)
> +#define to_qcom_node(_node) \
> + container_of(_node, struct qcom_interconnect_node, node)
> +
> +enum qcom_bus_type {
> + QCOM_BUS_TYPE_NOC = 0,
> + QCOM_BUS_TYPE_MEM,
> +};
> +
> +struct qcom_interconnect_provider {
> + struct icp icp;
> + void __iomem *base;
> + struct clk *bus_clk;
> + struct clk *bus_a_clk;
> + u32 base_offset;
> + u32 qos_offset;
> + enum qcom_bus_type type;
> +};
> +
> +struct qcom_interconnect_node {
> + struct interconnect_node node;
> + unsigned char *name;
> + struct interconnect_node *links[8];

Magic number 8. Replace with 8916_MAX_LINKS or something.

> + u16 id;
> + u16 num_links;
> + u16 port;
> + u16 buswidth;

Comment on what a buswidth is just to be clear

> + u64 rate;

Comment on units

> +};
> +
> +static struct qcom_interconnect_node snoc_int_0;
> +static struct qcom_interconnect_node snoc_int_1;
> +static struct qcom_interconnect_node snoc_int_bimc;
> +static struct qcom_interconnect_node snoc_bimc_0_mas;
> +static struct qcom_interconnect_node pnoc_snoc_slv;
> +
> +static struct qcom_interconnect_node snoc_bimc_0_slv;

IMO, saving an 'a' and 'e' is not worth it for the sake of
readability. Just use 'slave' and 'master' in the names.

> +static struct qcom_interconnect_node slv_ebi_ch0;
> +
> +static struct qcom_interconnect_node pnoc_int_1;
> +static struct qcom_interconnect_node mas_pnoc_sdcc_1;
> +static struct qcom_interconnect_node mas_pnoc_sdcc_2;
> +static struct qcom_interconnect_node pnoc_snoc_mas;
> +
> +struct qcom_interconnect_desc {
> + struct qcom_interconnect_node **nodes;
> + size_t num_nodes;
> +};
> +
> +static struct qcom_interconnect_node snoc_int_0 = {
> + .id = 10004,
> + .name = "snoc-int-0",
> +#if 0

Please get rid if these.

> + .links = { &snoc_pnoc_mas.node },
> + .num_links = 1,
> +#endif
> + .buswidth = 8,
> +};
> +
> +static struct qcom_interconnect_node snoc_int_1 = {
> + .id = 10005,
> + .name = "snoc-int-1",
> +#if 0
> + .links = { &slv_apss.node, &slv_cats_0.node, &slv_cats_1.node },
> + .num_links = 3,
> +#endif
> + .buswidth = 8,
> +};
> +
> +static struct qcom_interconnect_node snoc_int_bimc = {
> + .id = 10006,
> + .name = "snoc-bimc",
> + .links = { &snoc_bimc_0_mas.node },
> + .num_links = 1,
> + .buswidth = 8,
> +};
> +
> +static struct qcom_interconnect_node snoc_bimc_0_mas = {
> + .id = 10007,
> + .name = "snoc-bimc-0-mas",
> + .links = { &snoc_bimc_0_slv.node },
> + .num_links = 1,
> + .buswidth = 8,
> +};
> +
> +static struct qcom_interconnect_node pnoc_snoc_slv = {
> + .id = 10011,
> + .name = "snoc-pnoc",
> + .links = { &snoc_int_0.node, &snoc_int_bimc.node, &snoc_int_1.node },
> + .num_links = 3,
> + .buswidth = 8,
> +};

Suggest replacing this list of definitions with a macro such as:

#define DEFINE_XCON_NODE(_name, _id, numlinks, buswidth, ...) \
static struct qcom_interconnect_node _name; \
static struct qcom_interconnect_node _name = { \
.id = _id,
\
.name = _name,
\
.buswidth = buswidth,
\
.num_links = numlinks,
\
.links = { __VA_ARGS__ },
\
};

This will reduce these definitions to a series of definitions as
follows that improves readability.

DEFINE_XCON_NODE(snoc_int_0, 10004, 1, 8, &snoc_pnoc_mas.node)
DEFINE_XCON_NODE(snoc_int_1, 10005, 3, 8, &snoc_apss.node,
&slv_cats_0.node, &slv_cats_1.node)
DEFINE_XCON_NODE(snoc_int_bimc, 10006, 1, 8, &snoc_bimc_0_mas.node)

If fact you could take it one step further and move the definitions
under the provider definition below directly.

> +static struct qcom_interconnect_node *msm8916_snoc_nodes[] = {
> + [SNOC_INT_0] = &snoc_int_0,
> + [SNOC_INT_1] = &snoc_int_1,
> + [SNOC_INT_BIMC] = &snoc_int_bimc,
> + [SNOC_BIMC_0_MAS] = &snoc_bimc_0_mas,
> + [PNOC_SNOC_SLV] = &pnoc_snoc_slv,
> +};
> +
> +static struct qcom_interconnect_desc msm8916_snoc = {
> + .nodes = msm8916_snoc_nodes,
> + .num_nodes = ARRAY_SIZE(msm8916_snoc_nodes),
> +};
> +
> +static struct qcom_interconnect_node snoc_bimc_0_slv = {
> + .id = 10025,
> + .name = "snoc_bimc_0_slv",
> + .links = { &slv_ebi_ch0.node },
> + .num_links = 1,
> + .buswidth = 8,
> +};
> +
> +static struct qcom_interconnect_node slv_ebi_ch0 = {
> + .id = 512,
> + .name = "slv-ebi-ch0",
> + .buswidth = 8,
> +};
> +
> +static struct qcom_interconnect_node *msm8916_bimc_nodes[] = {
> + [SNOC_BIMC_0_SLV] = &snoc_bimc_0_slv,
> + [SLV_EBI_CH0] = &slv_ebi_ch0,
> +};
> +
> +static struct qcom_interconnect_desc msm8916_bimc = {
> + .nodes = msm8916_bimc_nodes,
> + .num_nodes = ARRAY_SIZE(msm8916_bimc_nodes),
> +};
> +
> +static struct qcom_interconnect_node pnoc_int_1 = {
> + .id = 10013,
> + .name = "pnoc-int-1",
> + .links = { &pnoc_snoc_mas.node },
> + .num_links = 1,
> + .buswidth = 8,
> +};
> +
> +static struct qcom_interconnect_node mas_pnoc_sdcc_1 = {
> + .id = 78,
> + .name = "mas-pnoc-sdcc-1",
> + .links = { &pnoc_int_1.node },
> + .num_links = 1,
> + .port = 7,
> + .buswidth = 8,
> +};
> +
> +static struct qcom_interconnect_node mas_pnoc_sdcc_2 = {
> + .id = 81,
> + .name = "mas-pnoc-sdcc-2",
> + .links = { &pnoc_int_1.node },
> + .num_links = 1,
> + .port = 8,
> + .buswidth = 8,
> +};
> +
> +static struct qcom_interconnect_node pnoc_snoc_mas = {
> + .id = 10010,
> + .name = "pnoc-snoc-mas",
> + .links = { &pnoc_snoc_slv.node },
> + .num_links = 1,
> + .buswidth = 8,
> +};
> +
> +static struct qcom_interconnect_node *msm8916_pnoc_nodes[] = {
> + [PNOC_INT_1] = &pnoc_int_1,
> + [MAS_PNOC_SDCC_1] = &mas_pnoc_sdcc_1,
> + [MAS_PNOC_SDCC_2] = &mas_pnoc_sdcc_2,
> + [PNOC_SNOC_MAS] = &pnoc_snoc_mas,
> +};

There are big holes in this node array definition because the index
values are not contiguous. Are you planning fill these in later for
each of the providers?

> +static struct qcom_interconnect_desc msm8916_pnoc = {
> + .nodes = msm8916_pnoc_nodes,
> + .num_nodes = ARRAY_SIZE(msm8916_pnoc_nodes),
> +};
> +
> +static int qcom_interconnect_init(struct interconnect_node *node)
> +{
> + struct qcom_interconnect_node *qn = to_qcom_node(node);
> +
> + /* populate default values */
> + if (!qn->buswidth)
> + qn->buswidth = 8;
> +
> + /* TODO: init qos and priority */
> +
> + return 0;

Next message: Stephen Rothwell: "linux-next: manual merge of the akpm-current tree with the drm tree"
Previous message: Gilad Ben-Yossef: "Re: [PATCH 2/3] staging: ccree: handle limiting of DMA masks"
Next in thread: Georgi Djakov: "Re: [PATCH v3 3/3] interconnect: Add Qualcomm msm8916 interconnect provider driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]