LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Yassine Oudjana <y.oudjana@protonmail.com>
To: Georgi Djakov <djakov@kernel.org>
Cc: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	AngeloGioacchino Del Regno <kholk11@gmail.com>,
	linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	okukatla@codeaurora.org, mdtipton@codeaurora.org
Subject: Re: [PATCH v3 5/6] interconnect: qcom: Add MSM8996 interconnect provider driver
Date: Wed, 11 Aug 2021 12:56:57 +0000	[thread overview]
Message-ID: <CMEOXQ.F594ZQFPV8PT2@protonmail.com> (raw)
In-Reply-To: <a520d353-f291-4596-e21b-6bdd27fdc965@kernel.org>



On Wed, Aug 11 2021 at 14:56:01 +0400, Georgi Djakov 
<djakov@kernel.org> wrote:
> 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?

I'm not sure why, but I get a SError interrupt when writing to its 
registers.
I tried enabling AGGRE0_NOC_GDSC but it didn't change anything.

> 
>> 
>>  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.

Noted.

> 
>>  +/*
>>  + * 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?

I was going to better format that TODO or remove it but it seems that I 
forgot.
Downstream has both ports specified.

> 
> [..]
> 
>>  +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.

Noted.

> 
>>  +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

Will do.

Thanks,
Yassine




  reply	other threads:[~2021-08-11 12:57 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-11  4:35 [PATCH v3 0/6] interconnect: qcom: Add MSM8996 interconnect driver Yassine Oudjana
2021-08-11  4:35 ` [PATCH v3 1/6] interconnect: qcom: icc-rpmh: Rename qcom_icc_set Yassine Oudjana
2021-08-11  4:36 ` [PATCH v3 2/6] interconnect: qcom: sdm660: Commonize RPM-QoS Yassine Oudjana
2021-08-11 17:31   ` kernel test robot
2021-08-11  4:36 ` [PATCH v3 3/6] dt-bindings: interconnect: Move SDM660 to a new RPM-QoS file Yassine Oudjana
2021-08-11  4:37 ` [PATCH v3 4/6] dt-bindings: interconnect: Add Qualcomm MSM8996 DT bindings Yassine Oudjana
2021-08-17 21:08   ` Rob Herring
2021-08-11  4:37 ` [PATCH v3 5/6] interconnect: qcom: Add MSM8996 interconnect provider driver Yassine Oudjana
2021-08-11 10:56   ` Georgi Djakov
2021-08-11 12:56     ` Yassine Oudjana [this message]
2021-08-18  3:31   ` Bjorn Andersson
2021-08-11  4:37 ` [PATCH v3 6/6] arm64: dts: qcom: msm8996: Add interconnect support Yassine Oudjana

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CMEOXQ.F594ZQFPV8PT2@protonmail.com \
    --to=y.oudjana@protonmail.com \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=djakov@kernel.org \
    --cc=kholk11@gmail.com \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mdtipton@codeaurora.org \
    --cc=okukatla@codeaurora.org \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH v3 5/6] interconnect: qcom: Add MSM8996 interconnect provider driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).