From: Georgi Djakov <georgi.djakov@linaro.org>
To: Evan Green <evgreen@chromium.org>
Cc: linux-pm@vger.kernel.org, gregkh@linuxfoundation.org,
rjw@rjwysocki.net, robh+dt@kernel.org,
Michael Turquette <mturquette@baylibre.com>,
khilman@baylibre.com, vincent.guittot@linaro.org,
skannan@codeaurora.org,
Bjorn Andersson <bjorn.andersson@linaro.org>,
amit.kucheria@linaro.org, seansw@qti.qualcomm.com,
davidai@quicinc.com, mark.rutland@arm.com,
lorenzo.pieralisi@arm.com, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v4 5/7] interconnect: qcom: Add msm8916 interconnect provider driver
Date: Wed, 6 Jun 2018 18:03:21 +0300 [thread overview]
Message-ID: <21c5adcc-dc18-73a1-e07a-d84f2e6ae534@linaro.org> (raw)
In-Reply-To: <CAE=gft4LT_JSGytuiSx5uwzfeGc0DK140+D4c_J8FVUHioHdcg@mail.gmail.com>
Hi Evan,
On 12.05.18 г. 0:29, Evan Green wrote:
> Hi Georgi,
>
> On Fri, Mar 9, 2018 at 1:11 PM Georgi Djakov <georgi.djakov@linaro.org>
> wrote:
>
>> Add driver for the Qualcomm interconnect buses found in msm8916 based
>> platforms.
>
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>> drivers/interconnect/Kconfig | 5 +
>> drivers/interconnect/Makefile | 1 +
>> drivers/interconnect/qcom/Kconfig | 11 +
>> drivers/interconnect/qcom/Makefile | 2 +
>> drivers/interconnect/qcom/msm8916.c | 482
> ++++++++++++++++++++++++++++++++++++
>> include/linux/interconnect/qcom.h | 350 ++++++++++++++++++++++++++
>> 6 files changed, 851 insertions(+)
>> create mode 100644 drivers/interconnect/qcom/Kconfig
>> create mode 100644 drivers/interconnect/qcom/msm8916.c
>> create mode 100644 include/linux/interconnect/qcom.h
> ...
>> diff --git a/drivers/interconnect/qcom/msm8916.c
> b/drivers/interconnect/qcom/msm8916.c
>> new file mode 100644
>> index 000000000000..d5b54f8261c8
>> --- /dev/null
>> +++ b/drivers/interconnect/qcom/msm8916.c
> ...
>> +static int qnoc_probe(struct platform_device *pdev)
>> +{
>> + const struct qcom_icc_desc *desc;
>> + struct qcom_icc_node **qnodes;
>> + struct qcom_icc_provider *qp;
>> + struct resource *res;
>> + struct icc_provider *provider;
>> + size_t num_nodes, i;
>> + int ret;
>> +
>> + /* wait for RPM */
>> + if (!qcom_icc_rpm_smd_available())
>> + return -EPROBE_DEFER;
>> +
>> + desc = of_device_get_match_data(&pdev->dev);
>> + if (!desc)
>> + return -EINVAL;
>> +
>> + qnodes = desc->nodes;
>> + num_nodes = desc->num_nodes;
>> +
>> + qp = devm_kzalloc(&pdev->dev, sizeof(*qp), GFP_KERNEL);
>> + if (!qp)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + qp->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(qp->base))
>> + return PTR_ERR(qp->base);
>> +
>> + qp->bus_clk = devm_clk_get(&pdev->dev, "bus_clk");
>> + if (IS_ERR(qp->bus_clk))
>> + return PTR_ERR(qp->bus_clk);
>> +
>> + qp->bus_a_clk = devm_clk_get(&pdev->dev, "bus_a_clk");
>> + if (IS_ERR(qp->bus_a_clk))
>> + return PTR_ERR(qp->bus_a_clk);
>> +
>> + provider = &qp->provider;
>> + provider->dev = &pdev->dev;
>> + provider->set = &qcom_icc_set;
>> + INIT_LIST_HEAD(&provider->nodes);
>> + provider->data = qp;
>> +
>> + ret = icc_add_provider(provider);
>> + if (ret) {
>> + dev_err(&pdev->dev, "error adding interconnect
> provider\n");
>> + return ret;
>> + }
>> +
>> + for (i = 0; i < num_nodes; i++) {
>> + struct icc_node *node;
>> + int ret;
>> + 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);
>> +
>> + dev_dbg(&pdev->dev, "registered node %p %s %d\n", node,
>> + qnodes[i]->name, node->id);
>> +
>> + /* populate links */
>> + for (j = 0; j < qnodes[i]->num_links; j++)
>> + if (qnodes[i]->links[j])
>> + icc_link_create(node,
> qnodes[i]->links[j]);
>> +
>> + ret = qcom_icc_init(node);
>> + if (ret)
>> + dev_err(&pdev->dev, "%s init error (%d)\n",
> node->name,
>> + ret);
>
> Don't you want to call qcom_icc_init before icc_link_create? As soon as
> icc_link_create is called, the node is connected to the graph, and
> qcom_icc_set can be called.
>
I agree.
>> + }
>> +
>> + platform_set_drvdata(pdev, provider);
>> +
>> + return ret;
>> +err:
>> + icc_del_provider(provider);
>> + return ret;
>> +}
>> +
>> +static int qnoc_remove(struct platform_device *pdev)
>> +{
>> + struct icc_provider *provider = platform_get_drvdata(pdev);
>> +
>> + icc_del_provider(provider);
>
> Presumably in the framework nodes and links ought to get cleaned up
> somewhere too, right? As it is now, the devm code frees provider when this
> device is removed, even though provider is still very connected in the
> graph via the nodes and links.
Sorry, this part is incomplete. Will implement the rest of the cleanup code.
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id qnoc_of_match[] = {
>> + { .compatible = "qcom,msm8916-pnoc", .data = &msm8916_pnoc },
>> + { .compatible = "qcom,msm8916-snoc", .data = &msm8916_snoc },
>> + { .compatible = "qcom,msm8916-bimc", .data = &msm8916_bimc },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, qnoc_of_match);
>> +
>> +static struct platform_driver qnoc_driver = {
>> + .probe = qnoc_probe,
>> + .remove = qnoc_remove,
>> + .driver = {
>> + .name = "qnoc-msm8916",
>> + .of_match_table = qnoc_of_match,
>> + },
>> +};
>> +module_platform_driver(qnoc_driver);
>> +MODULE_AUTHOR("Georgi Djakov <georgi.djakov@linaro.org>");
>> +MODULE_DESCRIPTION("Qualcomm msm8916 NoC driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/interconnect/qcom.h
> b/include/linux/interconnect/qcom.h
>> new file mode 100644
>> index 000000000000..2cd378d2f575
>> --- /dev/null
>> +++ b/include/linux/interconnect/qcom.h
>> @@ -0,0 +1,350 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Qualcomm interconnect IDs
>> + *
>> + * Copyright (c) 2018, Linaro Ltd.
>> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
>> + */
>> +
>> +#ifndef __QCOM_INTERCONNECT_IDS_H
>> +#define __QCOM_INTERCONNECT_IDS_H
>> +
>> +#define FAB_BIMC 0
>> +#define FAB_SYS_NOC 1024
>> +#define FAB_MMSS_NOC 2048
>> +#define FAB_OCMEM_NOC 3072
>> +#define FAB_PERIPH_NOC 4096
>> +#define FAB_CONFIG_NOC 5120
>> +#define FAB_OCMEM_VNOC 6144
>
> Looks like you're still following that downstream convention of NoCs being
> divisible by 1024, masters in 1-512, and slaves in 512-1023, then I don't
> know about the 10000s. This was documented somewhere downstream, can you
> add that documentation somewhere in here?
Yes, i am currently following the existing IDs. The 10000s are the nodes
used for the interconnects between the NoCs. I don't think it's worth
documenting this, as the plan is to switch to arbitrary numbers.
Thanks,
Georgi
next prev parent reply other threads:[~2018-06-06 15:03 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-09 21:09 [PATCH v4 0/7] Introduce on-chip interconnect API Georgi Djakov
2018-03-09 21:09 ` [PATCH v4 1/7] interconnect: Add generic " Georgi Djakov
2018-04-06 17:38 ` Matthias Kaehlcke
2018-04-12 13:06 ` Georgi Djakov
2018-05-11 21:30 ` Evan Green
2018-06-06 14:59 ` Georgi Djakov
2018-06-06 18:09 ` Georgi Djakov
2018-06-07 1:06 ` Evan Green
2018-05-25 8:26 ` Amit Kucheria
2018-06-06 15:08 ` Georgi Djakov
2018-06-08 15:57 ` Alexandre Bailon
2018-06-09 19:15 ` Georgi Djakov
2018-03-09 21:09 ` [PATCH v4 2/7] dt-bindings: Introduce interconnect provider bindings Georgi Djakov
2018-03-18 22:50 ` Bjorn Andersson
2018-03-19 9:34 ` Georgi Djakov
2018-04-12 13:15 ` Neil Armstrong
2018-06-06 15:23 ` Georgi Djakov
2018-03-09 21:09 ` [PATCH v4 3/7] interconnect: Add debugfs support Georgi Djakov
2018-03-09 21:09 ` [PATCH v4 4/7] interconnect: qcom: Add RPM communication Georgi Djakov
2018-05-11 21:30 ` Evan Green
2018-06-06 15:00 ` Georgi Djakov
2018-03-09 21:09 ` [PATCH v4 5/7] interconnect: qcom: Add msm8916 interconnect provider driver Georgi Djakov
2018-04-05 22:58 ` Matthias Kaehlcke
2018-04-12 13:09 ` Georgi Djakov
2018-05-11 21:29 ` Evan Green
2018-06-06 15:03 ` Georgi Djakov [this message]
2018-05-25 8:27 ` Amit Kucheria
2018-06-06 15:14 ` Georgi Djakov
2018-03-09 21:09 ` [PATCH v4 6/7] dt-bindings: Introduce interconnect consumers bindings Georgi Djakov
2018-03-18 22:49 ` Bjorn Andersson
2018-03-19 9:41 ` Georgi Djakov
2018-03-09 21:09 ` [PATCH v4 7/7] interconnect: Allow endpoints translation via DT Georgi Djakov
2018-05-11 21:29 ` Evan Green
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=21c5adcc-dc18-73a1-e07a-d84f2e6ae534@linaro.org \
--to=georgi.djakov@linaro.org \
--cc=amit.kucheria@linaro.org \
--cc=bjorn.andersson@linaro.org \
--cc=davidai@quicinc.com \
--cc=evgreen@chromium.org \
--cc=gregkh@linuxfoundation.org \
--cc=khilman@baylibre.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=mark.rutland@arm.com \
--cc=mturquette@baylibre.com \
--cc=rjw@rjwysocki.net \
--cc=robh+dt@kernel.org \
--cc=seansw@qti.qualcomm.com \
--cc=skannan@codeaurora.org \
--cc=vincent.guittot@linaro.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).