Re: [PATCH v3 5/6] interconnect: qcom: Add MSM8996 interconnect provider driver

[Georgi Djakov <djakov@kernel.org>]

Hi Yassine,

Thank you for working on this!

On 11.08.21 7:37, Yassine Oudjana wrote:
> Add a driver for the MSM8996 NoCs. This chip is similar to SDM660 where
> some busses are controlled by RPM, while others directly by the AP with
> writes to QoS registers.
> 
> This driver currently supports all NoCs except a0noc.

Just curious what's the issue with a0noc. Do we need to enable some GDSC
or clock in order to write to the QoS registers?

> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>   drivers/interconnect/qcom/Kconfig   |   9 +
>   drivers/interconnect/qcom/Makefile  |   2 +
>   drivers/interconnect/qcom/msm8996.c | 574 ++++++++++++++++++++++++++++
>   drivers/interconnect/qcom/msm8996.h | 149 ++++++++
>   4 files changed, 734 insertions(+)
>   create mode 100644 drivers/interconnect/qcom/msm8996.c
>   create mode 100644 drivers/interconnect/qcom/msm8996.h
> 
> diff --git a/drivers/interconnect/qcom/Kconfig b/drivers/interconnect/qcom/Kconfig
> index ad16224f1720..e30ad95e5584 100644
> --- a/drivers/interconnect/qcom/Kconfig
> +++ b/drivers/interconnect/qcom/Kconfig
> @@ -35,6 +35,15 @@ config INTERCONNECT_QCOM_MSM8974
>   	 This is a driver for the Qualcomm Network-on-Chip on msm8974-based
>   	 platforms.
>   
> +config INTERCONNECT_QCOM_MSM8996
> +	tristate "Qualcomm MSM8996 interconnect driver"
> +	depends on INTERCONNECT_QCOM
> +	depends on QCOM_SMD_RPM
> +	select INTERCONNECT_QCOM_SMD_RPM_QOS
> +	help
> +	  This is a driver for the Qualcomm Network-on-Chip on msm8996-based
> +	  platforms.
> +
>   config INTERCONNECT_QCOM_OSM_L3
>   	tristate "Qualcomm OSM L3 interconnect driver"
>   	depends on INTERCONNECT_QCOM || COMPILE_TEST
> diff --git a/drivers/interconnect/qcom/Makefile b/drivers/interconnect/qcom/Makefile
> index 2d04d024f46e..8a198b8b7a45 100644
> --- a/drivers/interconnect/qcom/Makefile
> +++ b/drivers/interconnect/qcom/Makefile
> @@ -4,6 +4,7 @@ icc-bcm-voter-objs			:= bcm-voter.o
>   qnoc-msm8916-objs			:= msm8916.o
>   qnoc-msm8939-objs			:= msm8939.o
>   qnoc-msm8974-objs			:= msm8974.o
> +qnoc-msm8996-objs			:= msm8996.o
>   icc-osm-l3-objs				:= osm-l3.o
>   qnoc-qcs404-objs			:= qcs404.o
>   icc-rpmh-obj				:= icc-rpmh.o
> @@ -22,6 +23,7 @@ obj-$(CONFIG_INTERCONNECT_QCOM_BCM_VOTER) += icc-bcm-voter.o
>   obj-$(CONFIG_INTERCONNECT_QCOM_MSM8916) += qnoc-msm8916.o
>   obj-$(CONFIG_INTERCONNECT_QCOM_MSM8939) += qnoc-msm8939.o
>   obj-$(CONFIG_INTERCONNECT_QCOM_MSM8974) += qnoc-msm8974.o
> +obj-$(CONFIG_INTERCONNECT_QCOM_MSM8996) += qnoc-msm8996.o
>   obj-$(CONFIG_INTERCONNECT_QCOM_OSM_L3) += icc-osm-l3.o
>   obj-$(CONFIG_INTERCONNECT_QCOM_QCS404) += qnoc-qcs404.o
>   obj-$(CONFIG_INTERCONNECT_QCOM_RPMH) += icc-rpmh.o
> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
> new file mode 100644
> index 000000000000..0cb93d743f35
> --- /dev/null
> +++ b/drivers/interconnect/qcom/msm8996.c
> @@ -0,0 +1,574 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */

The // style is preferred for .c files.

> +/*
> + * Qualcomm MSM8996 Network-on-Chip (NoC) QoS driver
> + *
> + * Copyright (c) 2021 Yassine Oudjana <y.oudjana@protonmail.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/interconnect/qcom,msm8996.h>
> +
> +#include "icc-rpm-qos.h"
> +#include "smd-rpm.h"
> +#include "msm8996.h"
> +
[..]
> +DEFINE_QNODE(mas_mdp_p0, MSM8996_MASTER_MDP_PORT0, 32, 8, -1, true, NOC_QOS_MODE_BYPASS, 0, 1, MSM8996_SLAVE_MNOC_BIMC);
> +DEFINE_QNODE(mas_mdp_p1, MSM8996_MASTER_MDP_PORT1, 32, 61, -1, true, NOC_QOS_MODE_BYPASS, 0, 2, MSM8996_SLAVE_MNOC_BIMC);
> +DEFINE_QNODE(mas_rotator, MSM8996_MASTER_ROTATOR, 32, 120, -1, true, NOC_QOS_MODE_BYPASS, 0, 0, MSM8996_SLAVE_MNOC_BIMC);
> +DEFINE_QNODE(mas_venus, MSM8996_MASTER_VIDEO_P0, 32, 9, -1, true, NOC_QOS_MODE_BYPASS, 0, 3 /* TODO: 3 4 ?? */, MSM8996_SLAVE_MNOC_BIMC);

Is the TODO for multiple QoS ports?

[..]

> +static const struct regmap_config msm8996_mnoc_regmap_config = {
> +	.reg_bits	= 32,
> +	.reg_stride	= 4,
> +	.val_bits	= 32,
> +	.max_register	= 0x20000,
> +	.fast_io	= true,
> +};
> +
> +static const struct qcom_icc_desc msm8996_mnoc = {
> +	.nodes = mnoc_nodes,
> +	.num_nodes = ARRAY_SIZE(mnoc_nodes),
> +	.regmap_cfg = &msm8996_mnoc_regmap_config,
> +};
> +
> +

Nit: No multiple blank lines, please.

> +static struct qcom_icc_node *pnoc_nodes[] = {
> +	[MASTER_SNOC_PNOC] = &mas_snoc_pnoc,
> +	[MASTER_SDCC_1] = &mas_sdcc_1,
> +	[MASTER_SDCC_2] = &mas_sdcc_2,
> +	[MASTER_SDCC_4] = &mas_sdcc_4,
> +	[MASTER_USB_HS] = &mas_usb_hs,
> +	[MASTER_BLSP_1] = &mas_blsp_1,
> +	[MASTER_BLSP_2] = &mas_blsp_2,
> +	[MASTER_TSIF] = &mas_tsif,
> +	[SLAVE_PNOC_A1NOC] = &slv_pnoc_a1noc,
> +	[SLAVE_USB_HS] = &slv_usb_hs,
> +	[SLAVE_SDCC_2] = &slv_sdcc_2,
> +	[SLAVE_SDCC_4] = &slv_sdcc_4,
> +	[SLAVE_TSIF] = &slv_tsif,
> +	[SLAVE_BLSP_2] = &slv_blsp_2,
> +	[SLAVE_SDCC_1] = &slv_sdcc_1,
> +	[SLAVE_BLSP_1] = &slv_blsp_1,
> +	[SLAVE_PDM] = &slv_pdm,
> +	[SLAVE_AHB2PHY] = &slv_ahb2phy,
> +};
> +
> +static const struct regmap_config msm8996_pnoc_regmap_config = {
> +	.reg_bits	= 32,
> +	.reg_stride	= 4,
> +	.val_bits	= 32,
> +	.max_register	= 0x3000,
> +	.fast_io	= true,
> +};
> +
[..]
> +	for (i = 0; i < num_nodes; i++) {
> +		size_t j;
> +
> +		node = icc_node_create(qnodes[i]->id);
> +		if (IS_ERR(node)) {
> +			ret = PTR_ERR(node);
> +			goto err;
> +		}
> +
> +		node->name = qnodes[i]->name;
> +		node->data = qnodes[i];
> +		icc_node_add(node, provider);
> +
> +		for (j = 0; j < qnodes[i]->num_links; j++) {
> +			icc_link_create(node, qnodes[i]->links[j]);
> +		}

Nit: No need for braces.

Also please rebase all patches onto linux-next.

Thanks,
Georgi