Re: [PATCH v5 05/10] soc: qcom: Add AOSS QMP communication driver
On Fri 01 Feb 15:36 PST 2019, Doug Anderson wrote: > Hi, > > On Wed, Jan 30, 2019 at 4:40 PM Bjorn Andersson > wrote: > > +++ b/drivers/soc/qcom/aoss-qmp.c > > @@ -0,0 +1,317 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018, Linaro Ltd > > + */ > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#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_STATE0x14 > > +#define QMP_DESC_UCORE_CH_STATE_ACK0x18 > > +#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_STATE0x2c > > +#define QMP_DESC_MCORE_CH_STATE_ACK0x30 > > +#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. > I had to look at the code again to figure out which side was which, so I'll add a comment here to indicate which is which. > > > +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()? > This must be me misinterpreting the downstream driver, the magic is already in place when we enter here. > > > + 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? > Didn't know I could do that, but that said this is conditional on !qmp->size, so I'm dropping the 0x0 from the error... > > > + 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? > I guess it does, but I think there is a kick inbetween here in the downstream driver, I'll add one for good measure. > > > + 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. > Right > > > +static int qmp_probe(struct platform_device *pdev) > > +{ >
Re: [PATCH v5 05/10] soc: qcom: Add AOSS QMP communication driver
Hi, On Wed, Jan 30, 2019 at 4:40 PM Bjorn Andersson wrote: > +++ b/drivers/soc/qcom/aoss-qmp.c > @@ -0,0 +1,317 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018, Linaro Ltd > + */ > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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_STATE0x14 > +#define QMP_DESC_UCORE_CH_STATE_ACK0x18 > +#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_STATE0x2c > +#define QMP_DESC_MCORE_CH_STATE_ACK0x30 > +#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(>dev, sizeof(*qmp), GFP_KERNEL); > + if (!qmp) > + return -ENOMEM; > + > + qmp->dev = >dev; > + init_waitqueue_head(>event); > + mutex_init(>tx_lock); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + qmp->msgram = devm_ioremap_resource(>dev, res); > + if (IS_ERR(qmp->msgram)) > + return PTR_ERR(qmp->msgram); > + > + qmp->mbox_client.dev = >dev; > + qmp->mbox_client.knows_txdone = true; > + qmp->mbox_chan = mbox_request_channel(>mbox_client, 0); nit: your code would be simplified a bit if you created devm_mbox_request_channel() in a prior patch.