LKML Archive on lore.kernel.org
 help / Atom feed
From: Doug Anderson <dianders@chromium.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Andy Gross <andy.gross@linaro.org>,
	David Brown <david.brown@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Ohad Ben-Cohen <ohad@wizery.com>,
	Arun Kumar Neelakantam <aneela@codeaurora.org>,
	Sibi Sankar <sibis@codeaurora.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	devicetree@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
	linux-remoteproc@vger.kernel.org
Subject: Re: [PATCH v5 05/10] soc: qcom: Add AOSS QMP communication driver
Date: Fri, 1 Feb 2019 15:36:38 -0800
Message-ID: <CAD=FV=U617Q6RFPnJaRoNEf=hr1ag9qqon2M-msbQD483tmicg@mail.gmail.com> (raw)
In-Reply-To: <20190131003933.11436-6-bjorn.andersson@linaro.org>

Hi,

On Wed, Jan 30, 2019 at 4:40 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> +++ b/drivers/soc/qcom/aoss-qmp.c
> @@ -0,0 +1,317 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, Linaro Ltd
> + */
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/soc/qcom/aoss-qmp.h>
> +
> +#define QMP_DESC_MAGIC                 0x0
> +#define QMP_DESC_VERSION               0x4
> +#define QMP_DESC_FEATURES              0x8
> +
> +#define QMP_DESC_UCORE_LINK_STATE      0xc
> +#define QMP_DESC_UCORE_LINK_STATE_ACK  0x10
> +#define QMP_DESC_UCORE_CH_STATE                0x14
> +#define QMP_DESC_UCORE_CH_STATE_ACK    0x18
> +#define QMP_DESC_UCORE_MBOX_SIZE       0x1c
> +#define QMP_DESC_UCORE_MBOX_OFFSET     0x20
> +
> +#define QMP_DESC_MCORE_LINK_STATE      0x24
> +#define QMP_DESC_MCORE_LINK_STATE_ACK  0x28
> +#define QMP_DESC_MCORE_CH_STATE                0x2c
> +#define QMP_DESC_MCORE_CH_STATE_ACK    0x30
> +#define QMP_DESC_MCORE_MBOX_SIZE       0x34
> +#define QMP_DESC_MCORE_MBOX_OFFSET     0x38

I sure wish something in this file told me what a mcore and a ucore
were.  The only thing I can think of is that an "m" core is two "u"
cores flipped upside down and placed really close to each other.
...if we had 6 upside down "u" cores we'd have an "mmm" core.  Mmm,
core.


> +static int qmp_open(struct qmp *qmp)
> +{
> +       int ret;
> +       u32 val;
> +
> +       ret = wait_event_timeout(qmp->event, qmp_magic_valid(qmp), HZ);

I'm a totally noob here, but I'm curious: what kicks this event?  Do
we just assume that an IRQ is pending already when the probe()
function is called?  Maybe you could add a comment?

...or maybe you never actually get an IRQ here and just rely on the
magic value being right at boot in which case we should just check
qmp_magic_valid()

...or maybe you never actually get an IRQ here and this is equivalent
to msleep(1000) followed by a check of qmp_magic_valid()?


> +       if (!ret) {
> +               dev_err(qmp->dev, "QMP magic doesn't match\n");
> +               return -ETIMEDOUT;
> +       }
> +
> +       val = readl(qmp->msgram + QMP_DESC_VERSION);
> +       if (val != QMP_VERSION) {
> +               dev_err(qmp->dev, "unsupported QMP version %d\n", val);
> +               return -EINVAL;
> +       }
> +
> +       qmp->offset = readl(qmp->msgram + QMP_DESC_MCORE_MBOX_OFFSET);
> +       qmp->size = readl(qmp->msgram + QMP_DESC_MCORE_MBOX_SIZE);
> +       if (!qmp->size) {
> +               dev_err(qmp->dev, "invalid mailbox size 0x%zx\n", qmp->size);

nitty nit: Can you do "%#zx" to avoid the need for the 0x?


> +               return -EINVAL;
> +       }
> +
> +       /* Ack remote core's link state */
> +       val = readl(qmp->msgram + QMP_DESC_UCORE_LINK_STATE);
> +       writel(val, qmp->msgram + QMP_DESC_UCORE_LINK_STATE_ACK);
> +
> +       /* Set local core's link state to up */
> +       writel(QMP_STATE_UP, qmp->msgram + QMP_DESC_MCORE_LINK_STATE);
> +
> +       qmp_kick(qmp);
> +
> +       ret = wait_event_timeout(qmp->event, qmp_link_acked(qmp), HZ);
> +       if (!ret) {
> +               dev_err(qmp->dev, "ucore didn't ack link\n");
> +               goto timeout_close_link;
> +       }
> +
> +       writel(QMP_STATE_UP, qmp->msgram + QMP_DESC_MCORE_CH_STATE);
> +
> +       ret = wait_event_timeout(qmp->event, qmp_ucore_channel_up(qmp), HZ);

Again maybe a noob question, but what kicks the interrupt here?  Is
the other side looping waiting to see us write "QMP_STATE_UP" into
"QMP_DESC_MCORE_CH_STATE" and then it sends us another interrupt?
...or are we just getting lucky that the condition is right to begin
with?


> +       if (!ret) {
> +               dev_err(qmp->dev, "ucore didn't open channel\n");
> +               goto timeout_close_channel;
> +       }
> +
> +       /* Ack remote core's channel state */
> +       val = readl(qmp->msgram + QMP_DESC_UCORE_CH_STATE);
> +       writel(val, qmp->msgram + QMP_DESC_UCORE_CH_STATE_ACK);

nit: the readl() is silly here.  Just before this you called
qmp_ucore_channel_up() and that confirmed that the value you're
getting here is exactly equal to "QMP_STATE_UP".  Just write that.


> +static int qmp_probe(struct platform_device *pdev)
> +{
> +       struct resource *res;
> +       struct qmp *qmp;
> +       int irq;
> +       int ret;
> +
> +       qmp = devm_kzalloc(&pdev->dev, sizeof(*qmp), GFP_KERNEL);
> +       if (!qmp)
> +               return -ENOMEM;
> +
> +       qmp->dev = &pdev->dev;
> +       init_waitqueue_head(&qmp->event);
> +       mutex_init(&qmp->tx_lock);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       qmp->msgram = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(qmp->msgram))
> +               return PTR_ERR(qmp->msgram);
> +
> +       qmp->mbox_client.dev = &pdev->dev;
> +       qmp->mbox_client.knows_txdone = true;
> +       qmp->mbox_chan = mbox_request_channel(&qmp->mbox_client, 0);

nit: your code would be simplified a bit if you created
devm_mbox_request_channel() in a prior patch.


> +       if (IS_ERR(qmp->mbox_chan)) {
> +               dev_err(&pdev->dev, "failed to acquire ipc mailbox\n");
> +               return PTR_ERR(qmp->mbox_chan);
> +       }
> +
> +       irq = platform_get_irq(pdev, 0);
> +       ret = devm_request_irq(&pdev->dev, irq, qmp_intr, IRQF_ONESHOT,
> +                              "aoss-qmp", qmp);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "failed to request interrupt\n");
> +               mbox_free_channel(qmp->mbox_chan);
> +               return ret;
> +       }
> +
> +       ret = qmp_open(qmp);
> +       if (ret < 0) {
> +               mbox_free_channel(qmp->mbox_chan);
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, qmp);
> +
> +       if (of_property_read_bool(pdev->dev.of_node, "#power-domain-cells")) {
> +               qmp->pd_pdev = platform_device_register_data(&pdev->dev,
> +                                                            "aoss_qmp_pd",
> +                                                            PLATFORM_DEVID_NONE,
> +                                                            NULL, 0);
> +               if (IS_ERR(qmp->pd_pdev))
> +                       dev_err(&pdev->dev, "failed to register AOSS PD\n");

nit: I'd prefer dev_warn() for serious but non-fatal errors.  This
appears to be non-fatal since it doesn't cause you to return an error.

...ideally the error message should indicate that the error is being ignored.


I'm also not 100% sure why the "aoss_qmp_pd" needs to be broken up as
a separate driver.  I guess there is expectation that there will be
more sub-drivers that use qmp_send() and that stuffing them all in the
same driver would be too much?  It sure seems like your life would be
simplified if they were just one driver though unless you think
someone would want to enable "AOSS_QMP" without enabling
"AOSS_QMP_PD".


> +static int qmp_remove(struct platform_device *pdev)
> +{
> +       struct qmp *qmp = platform_get_drvdata(pdev);
> +
> +       platform_device_unregister(qmp->pd_pdev);

Presumably the above should be prefixed with:

if (!IS_ERR(qmp->pd_pdev))

...since it appears that the probe will return with no error if you
fail to register the pd_pdev and thus you need to handle it being an
error in remove.


> +       mbox_free_channel(qmp->mbox_chan);
> +       qmp_close(qmp);

nit: I always expect that remove should be in the opposite order of
probe.  That means qmp_close() should be before mbox_free_channel().

  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-31  0:39 [PATCH v5 00/10] Qualcomm AOSS QMP driver and modem dts Bjorn Andersson
2019-01-31  0:39 ` [PATCH v5 01/10] arm64: dts: qcom: sdm845: Update reserved memory map Bjorn Andersson
2019-01-31 16:58   ` Sibi Sankar
2019-01-31  0:39 ` [PATCH v5 02/10] arm64: dts: qcom: sdm845: Define rmtfs memory Bjorn Andersson
2019-01-31 17:09   ` Sibi Sankar
2019-01-31  0:39 ` [PATCH v5 03/10] arm64: dts: sdm845: Introduce ADSP and CDSP PAS nodes Bjorn Andersson
2019-02-01  5:49   ` Sibi Sankar
2019-02-01 23:54   ` Doug Anderson
2019-01-31  0:39 ` [PATCH v5 04/10] dt-bindings: soc: qcom: Add AOSS QMP binding Bjorn Andersson
2019-01-31  0:39 ` [PATCH v5 05/10] soc: qcom: Add AOSS QMP communication driver Bjorn Andersson
2019-02-01 23:36   ` Doug Anderson [this message]
2019-02-06  1:33     ` Bjorn Andersson
2019-01-31  0:39 ` [PATCH v5 06/10] soc: qcom: Add AOSS QMP genpd provider Bjorn Andersson
2019-02-01  7:15   ` Sibi Sankar
2019-02-01 23:39   ` Doug Anderson
2019-01-31  0:39 ` [PATCH v5 07/10] remoteproc: q6v5-mss: Vote for rpmh power domains Bjorn Andersson
2019-01-31  4:51   ` Bjorn Andersson
2019-01-31  0:39 ` [PATCH v5 08/10] remoteproc: q6v5-mss: Active powerdomain for SDM845 Bjorn Andersson
2019-01-31  4:51   ` Bjorn Andersson
2019-01-31  0:39 ` [PATCH v5 09/10] arm64: dts: qcom: Add AOSS QMP node Bjorn Andersson
2019-02-01  7:17   ` Sibi Sankar
2019-01-31  0:39 ` [PATCH v5 10/10] arm64: dts: qcom: sdm845: Add Q6V5 MSS node Bjorn Andersson

Reply instructions:

You may reply publically 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='CAD=FV=U617Q6RFPnJaRoNEf=hr1ag9qqon2M-msbQD483tmicg@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=andy.gross@linaro.org \
    --cc=aneela@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=david.brown@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=ohad@wizery.com \
    --cc=robh+dt@kernel.org \
    --cc=sibis@codeaurora.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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox