From: Evan Green <evgreen@chromium.org> To: georgi.djakov@linaro.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: Fri, 11 May 2018 14:29:42 -0700 [thread overview] Message-ID: <CAE=gft4LT_JSGytuiSx5uwzfeGc0DK140+D4c_J8FVUHioHdcg@mail.gmail.com> (raw) In-Reply-To: <20180309210958.16672-6-georgi.djakov@linaro.org> 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. > + } > + > + 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. > + > + 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? -Evan
WARNING: multiple messages have this Message-ID (diff)
From: evgreen@chromium.org (Evan Green) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v4 5/7] interconnect: qcom: Add msm8916 interconnect provider driver Date: Fri, 11 May 2018 14:29:42 -0700 [thread overview] Message-ID: <CAE=gft4LT_JSGytuiSx5uwzfeGc0DK140+D4c_J8FVUHioHdcg@mail.gmail.com> (raw) In-Reply-To: <20180309210958.16672-6-georgi.djakov@linaro.org> 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. > + } > + > + 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. > + > + 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? -Evan
next prev parent reply other threads:[~2018-05-11 21:29 UTC|newest] Thread overview: 68+ 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 ` Georgi Djakov 2018-03-09 21:09 ` [PATCH v4 1/7] interconnect: Add generic " Georgi Djakov 2018-03-09 21:09 ` Georgi Djakov 2018-03-09 21:09 ` Georgi Djakov 2018-04-06 17:38 ` Matthias Kaehlcke 2018-04-06 17:38 ` Matthias Kaehlcke 2018-04-12 13:06 ` Georgi Djakov 2018-04-12 13:06 ` Georgi Djakov 2018-05-11 21:30 ` Evan Green 2018-05-11 21:30 ` Evan Green 2018-06-06 14:59 ` Georgi Djakov 2018-06-06 14:59 ` Georgi Djakov 2018-06-06 18:09 ` Georgi Djakov 2018-06-06 18:09 ` Georgi Djakov 2018-06-07 1:06 ` Evan Green 2018-06-07 1:06 ` Evan Green 2018-06-07 1:06 ` Evan Green 2018-05-25 8:26 ` Amit Kucheria 2018-05-25 8:26 ` Amit Kucheria 2018-06-06 15:08 ` Georgi Djakov 2018-06-06 15:08 ` Georgi Djakov 2018-06-08 15:57 ` Alexandre Bailon 2018-06-08 15:57 ` Alexandre Bailon 2018-06-09 19:15 ` Georgi Djakov 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-09 21:09 ` Georgi Djakov 2018-03-18 22:50 ` Bjorn Andersson 2018-03-18 22:50 ` Bjorn Andersson 2018-03-19 9:34 ` Georgi Djakov 2018-03-19 9:34 ` Georgi Djakov 2018-04-12 13:15 ` Neil Armstrong 2018-04-12 13:15 ` Neil Armstrong 2018-06-06 15:23 ` Georgi Djakov 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 ` Georgi Djakov 2018-03-09 21:09 ` [PATCH v4 4/7] interconnect: qcom: Add RPM communication Georgi Djakov 2018-03-09 21:09 ` Georgi Djakov 2018-05-11 21:30 ` Evan Green 2018-05-11 21:30 ` Evan Green 2018-06-06 15:00 ` Georgi Djakov 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-03-09 21:09 ` Georgi Djakov 2018-04-05 22:58 ` Matthias Kaehlcke 2018-04-05 22:58 ` Matthias Kaehlcke 2018-04-12 13:09 ` Georgi Djakov 2018-04-12 13:09 ` Georgi Djakov 2018-05-11 21:29 ` Evan Green [this message] 2018-05-11 21:29 ` Evan Green 2018-06-06 15:03 ` Georgi Djakov 2018-06-06 15:03 ` Georgi Djakov 2018-05-25 8:27 ` Amit Kucheria 2018-05-25 8:27 ` Amit Kucheria 2018-06-06 15:14 ` Georgi Djakov 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-09 21:09 ` Georgi Djakov 2018-03-18 22:49 ` Bjorn Andersson 2018-03-18 22:49 ` Bjorn Andersson 2018-03-19 9:41 ` Georgi Djakov 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-03-09 21:09 ` Georgi Djakov 2018-05-11 21:29 ` Evan Green 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='CAE=gft4LT_JSGytuiSx5uwzfeGc0DK140+D4c_J8FVUHioHdcg@mail.gmail.com' \ --to=evgreen@chromium.org \ --cc=amit.kucheria@linaro.org \ --cc=bjorn.andersson@linaro.org \ --cc=davidai@quicinc.com \ --cc=georgi.djakov@linaro.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.