On 1/9/2023 2:39 PM, Bjorn Andersson wrote:
Not all GLINK transports uses an interrupt and a mailbox instance. The
interrupt for RPM needs to be IRQF_NOSUSPEND, while it seems reasonable
for the SMEM interrupt to use irq_set_wake. The glink struct device is
constructed in the SMEM and RPM drivers but torn down in the core
driver.
Move the interrupt and kick handling into the SMEM and RPM driver, to
improve this and facilitate further improvements.
Signed-off-by: Bjorn Andersson <quic_bjorande@xxxxxxxxxxx>
---
drivers/rpmsg/qcom_glink_native.c | 48 ++------------------------
drivers/rpmsg/qcom_glink_native.h | 3 +-
drivers/rpmsg/qcom_glink_rpm.c | 50 ++++++++++++++++++++++++++-
drivers/rpmsg/qcom_glink_smem.c | 56 +++++++++++++++++++++++++++++--
4 files changed, 108 insertions(+), 49 deletions(-)
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 5fd8b70271b7..db5d946d5901 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -11,7 +11,6 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_address.h>
-#include <linux/of_irq.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/rpmsg.h>
@@ -78,11 +77,8 @@ struct glink_core_rx_intent {
/**
* struct qcom_glink - driver context, relates to one remote subsystem
* @dev: reference to the associated struct device
- * @mbox_client: mailbox client
- * @mbox_chan: mailbox channel
* @rx_pipe: pipe object for receive FIFO
* @tx_pipe: pipe object for transmit FIFO
- * @irq: IRQ for signaling incoming events
* @rx_work: worker for handling received control messages
* @rx_lock: protects the @rx_queue
* @rx_queue: queue of received control messages to be processed in @rx_work
@@ -98,14 +94,9 @@ struct glink_core_rx_intent {
struct qcom_glink {
struct device *dev;
- struct mbox_client mbox_client;
- struct mbox_chan *mbox_chan;
-
struct qcom_glink_pipe *rx_pipe;
struct qcom_glink_pipe *tx_pipe;
- int irq;
-
struct work_struct rx_work;
spinlock_t rx_lock;
struct list_head rx_queue;
@@ -305,8 +296,7 @@ static void qcom_glink_tx_write(struct qcom_glink *glink,
static void qcom_glink_tx_kick(struct qcom_glink *glink)
{
- mbox_send_message(glink->mbox_chan, NULL);
- mbox_client_txdone(glink->mbox_chan, 0);
+ glink->tx_pipe->kick(glink->tx_pipe);
I think that we need to check that tx_pipe is not null or validate that
tx_pipe is not null in the native register probe.
}
static void qcom_glink_send_read_notify(struct qcom_glink *glink)
@@ -1004,9 +994,8 @@ static int qcom_glink_rx_open_ack(struct qcom_glink *glink, unsigned int lcid)
return 0;
}
-static irqreturn_t qcom_glink_native_intr(int irq, void *data)
+void qcom_glink_native_intr(struct qcom_glink *glink)
{
- struct qcom_glink *glink = data;
struct glink_msg msg;
unsigned int param1;
unsigned int param2;
@@ -1075,9 +1064,8 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
if (ret)
break;
}
-
- return IRQ_HANDLED;
}
+EXPORT_SYMBOL(qcom_glink_native_intr);
/* Locally initiated rpmsg_create_ept */
static struct glink_channel *qcom_glink_create_local(struct qcom_glink *glink,
@@ -1723,7 +1711,6 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
struct qcom_glink_pipe *tx,
bool intentless)
{
- int irq;
int ret;
struct qcom_glink *glink;
@@ -1754,27 +1741,6 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
if (ret)
dev_err(dev, "failed to add groups\n");
- glink->mbox_client.dev = dev;
- glink->mbox_client.knows_txdone = true;
- glink->mbox_chan = mbox_request_channel(&glink->mbox_client, 0);
- if (IS_ERR(glink->mbox_chan)) {
- if (PTR_ERR(glink->mbox_chan) != -EPROBE_DEFER)
- dev_err(dev, "failed to acquire IPC channel\n");
- return ERR_CAST(glink->mbox_chan);
- }
-
- irq = of_irq_get(dev->of_node, 0);
- ret = devm_request_irq(dev, irq,
- qcom_glink_native_intr,
- IRQF_NO_SUSPEND | IRQF_SHARED,
- "glink-native", glink);
- if (ret) {
- dev_err(dev, "failed to request IRQ\n");
- return ERR_PTR(ret);
- }
-
- glink->irq = irq;
-
ret = qcom_glink_send_version(glink);
if (ret)
return ERR_PTR(ret);
@@ -1800,7 +1766,6 @@ void qcom_glink_native_remove(struct qcom_glink *glink)
int cid;
int ret;
- disable_irq(glink->irq);
qcom_glink_cancel_rx_work(glink);
ret = device_for_each_child(glink->dev, NULL, qcom_glink_remove_device);
@@ -1817,15 +1782,8 @@ void qcom_glink_native_remove(struct qcom_glink *glink)
idr_destroy(&glink->lcids);
idr_destroy(&glink->rcids);
- mbox_free_channel(glink->mbox_chan);
}
EXPORT_SYMBOL_GPL(qcom_glink_native_remove);
-void qcom_glink_native_unregister(struct qcom_glink *glink)
-{
- device_unregister(glink->dev);
-}
-EXPORT_SYMBOL_GPL(qcom_glink_native_unregister);
-
MODULE_DESCRIPTION("Qualcomm GLINK driver");
MODULE_LICENSE("GPL v2");
diff --git a/drivers/rpmsg/qcom_glink_native.h b/drivers/rpmsg/qcom_glink_native.h
index e9a8671616c7..0129fe1b2b6c 100644
--- a/drivers/rpmsg/qcom_glink_native.h
+++ b/drivers/rpmsg/qcom_glink_native.h
@@ -24,6 +24,7 @@ struct qcom_glink_pipe {
void (*write)(struct qcom_glink_pipe *glink_pipe,
const void *hdr, size_t hlen,
const void *data, size_t dlen);
+ void (*kick)(struct qcom_glink_pipe *glink_pipe);
};
struct device;
@@ -35,6 +36,6 @@ struct qcom_glink *qcom_glink_native_probe(struct device *dev,
struct qcom_glink_pipe *tx,
bool intentless);
void qcom_glink_native_remove(struct qcom_glink *glink);
+void qcom_glink_native_intr(struct qcom_glink *glink);
We could rename this away from qcom_glink_native_intr to something like
qcom_glink_native_rx. Seeing this in the header, the purpose sounds a
bit obscure.
-void qcom_glink_native_unregister(struct qcom_glink *glink);
#endif
diff --git a/drivers/rpmsg/qcom_glink_rpm.c b/drivers/rpmsg/qcom_glink_rpm.c
index 6443843df6ca..9136645d6251 100644
--- a/drivers/rpmsg/qcom_glink_rpm.c
+++ b/drivers/rpmsg/qcom_glink_rpm.c
@@ -11,6 +11,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_address.h>
+#include <linux/of_irq.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/rpmsg.h>
@@ -56,6 +57,11 @@ struct glink_rpm_pipe {
struct glink_rpm {
struct qcom_glink *glink;
+ int irq;
+
+ struct mbox_client mbox_client;
+ struct mbox_chan *mbox_chan;
+
struct glink_rpm_pipe rx_pipe;
struct glink_rpm_pipe tx_pipe;
};
@@ -186,6 +192,24 @@ static void glink_rpm_tx_write(struct qcom_glink_pipe *glink_pipe,
writel(head, pipe->head);
}
+static void glink_rpm_tx_kick(struct qcom_glink_pipe *glink_pipe)
+{
+ struct glink_rpm_pipe *pipe = to_rpm_pipe(glink_pipe);
+ struct glink_rpm *rpm = container_of(pipe, struct glink_rpm, tx_pipe);
+
+ mbox_send_message(rpm->mbox_chan, NULL);
+ mbox_client_txdone(rpm->mbox_chan, 0);
+}
+
+static irqreturn_t qcom_glink_rpm_intr(int irq, void *data)
+{
+ struct glink_rpm *rpm = data;
+
+ qcom_glink_native_intr(rpm->glink);
+
+ return IRQ_HANDLED;
+}
+
static int glink_rpm_parse_toc(struct device *dev,
void __iomem *msg_ram,
size_t msg_ram_size,
@@ -292,12 +316,28 @@ static int glink_rpm_probe(struct platform_device *pdev)
if (ret)
return ret;
+ rpm->irq = of_irq_get(dev->of_node, 0);
+ ret = devm_request_irq(dev, rpm->irq, qcom_glink_rpm_intr,
+ IRQF_NO_SUSPEND | IRQF_NO_AUTOEN,
+ "glink-rpm", rpm);
+ if (ret) {
+ dev_err(dev, "failed to request IRQ\n");
+ return ret;
+ }
+
+ rpm->mbox_client.dev = dev;
+ rpm->mbox_client.knows_txdone = true;
+ rpm->mbox_chan = mbox_request_channel(&rpm->mbox_client, 0);
+ if (IS_ERR(rpm->mbox_chan))
+ return dev_err_probe(dev, PTR_ERR(rpm->mbox_chan), "failed to acquire IPC channel\n");
+
/* Pipe specific accessors */
rpm->rx_pipe.native.avail = glink_rpm_rx_avail;
rpm->rx_pipe.native.peak = glink_rpm_rx_peak;
rpm->rx_pipe.native.advance = glink_rpm_rx_advance;
rpm->tx_pipe.native.avail = glink_rpm_tx_avail;
rpm->tx_pipe.native.write = glink_rpm_tx_write;
+ rpm->tx_pipe.native.kick = glink_rpm_tx_kick;
writel(0, rpm->tx_pipe.head);
writel(0, rpm->rx_pipe.tail);
@@ -307,13 +347,17 @@ static int glink_rpm_probe(struct platform_device *pdev)
&rpm->rx_pipe.native,
&rpm->tx_pipe.native,
true);
- if (IS_ERR(glink))
+ if (IS_ERR(glink)) {
+ mbox_free_channel(rpm->mbox_chan);
return PTR_ERR(glink);
+ }
rpm->glink = glink;
platform_set_drvdata(pdev, rpm);
+ enable_irq(rpm->irq);
+
return 0;
}
@@ -322,8 +366,12 @@ static int glink_rpm_remove(struct platform_device *pdev)
struct glink_rpm *rpm = platform_get_drvdata(pdev);
struct qcom_glink *glink = rpm->glink;
+ disable_irq(rpm->irq);
+
qcom_glink_native_remove(glink);
+ mbox_free_channel(rpm->mbox_chan);
+
return 0;
}
diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
index 703e63fa5a86..eec47ae98d67 100644
--- a/drivers/rpmsg/qcom_glink_smem.c
+++ b/drivers/rpmsg/qcom_glink_smem.c
@@ -7,8 +7,10 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_address.h>
+#include <linux/of_irq.h>
#include <linux/interrupt.h>
#include <linux/platform_device.h>
+#include <linux/mailbox_client.h>
#include <linux/mfd/syscon.h>
#include <linux/slab.h>
#include <linux/rpmsg.h>
@@ -36,8 +38,12 @@
struct qcom_glink_smem {
struct device dev;
+ int irq;
struct qcom_glink *glink;
+ struct mbox_client mbox_client;
+ struct mbox_chan *mbox_chan;
+
u32 remote_pid;
};
@@ -186,6 +192,24 @@ static void glink_smem_tx_write(struct qcom_glink_pipe *glink_pipe,
*pipe->head = cpu_to_le32(head);
}
+static void glink_smem_tx_kick(struct qcom_glink_pipe *glink_pipe)
+{
+ struct glink_smem_pipe *pipe = to_smem_pipe(glink_pipe);
+ struct qcom_glink_smem *smem = pipe->smem;
+
+ mbox_send_message(smem->mbox_chan, NULL);
+ mbox_client_txdone(smem->mbox_chan, 0);
+}
+
+static irqreturn_t qcom_glink_smem_intr(int irq, void *data)
+{
+ struct qcom_glink_smem *smem = data;
+
+ qcom_glink_native_intr(smem->glink);
+
+ return IRQ_HANDLED;
+}
+
static void qcom_glink_smem_release(struct device *dev)
{
struct qcom_glink_smem *smem = container_of(dev, struct qcom_glink_smem, dev);
@@ -277,6 +301,24 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
goto err_put_dev;
}
+ smem->irq = of_irq_get(smem->dev.of_node, 0);
+ ret = devm_request_irq(&smem->dev, smem->irq, qcom_glink_smem_intr,
+ IRQF_NO_SUSPEND | IRQF_NO_AUTOEN,
+ "glink-smem", smem);
Are we adding dropping IRQF_NO_SUSPEND and adding enable irq wake for
smem in follow up change?
+ if (ret) {
+ dev_err(&smem->dev, "failed to request IRQ\n");
+ goto err_put_dev;
+ }
+
+ smem->mbox_client.dev = &smem->dev;
+ smem->mbox_client.knows_txdone = true;
+ smem->mbox_chan = mbox_request_channel(&smem->mbox_client, 0);
+ if (IS_ERR(smem->mbox_chan)) {
+ ret = dev_err_probe(&smem->dev, PTR_ERR(smem->mbox_chan),
+ "failed to acquire IPC channel\n");
+ goto err_put_dev;
+ }
+
rx_pipe->smem = smem;
rx_pipe->native.avail = glink_smem_rx_avail;
rx_pipe->native.peak = glink_smem_rx_peak;
@@ -285,6 +327,7 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
tx_pipe->smem = smem;
tx_pipe->native.avail = glink_smem_tx_avail;
tx_pipe->native.write = glink_smem_tx_write;
+ tx_pipe->native.kick = glink_smem_tx_kick;
*rx_pipe->tail = 0;
*tx_pipe->head = 0;
@@ -295,13 +338,17 @@ struct qcom_glink_smem *qcom_glink_smem_register(struct device *parent,
false);
if (IS_ERR(glink)) {
ret = PTR_ERR(glink);
- goto err_put_dev;
+ goto err_free_mbox;
}
smem->glink = glink;
+ enable_irq(smem->irq);
+
return smem;
+err_free_mbox:
+ mbox_free_channel(smem->mbox_chan);
err_put_dev:
device_unregister(&smem->dev);
@@ -314,8 +361,13 @@ void qcom_glink_smem_unregister(struct qcom_glink_smem *smem)
{
struct qcom_glink *glink = smem->glink;
+ disable_irq(smem->irq);
+
qcom_glink_native_remove(glink);
- qcom_glink_native_unregister(glink);
+
+ device_unregister(&smem->dev);
+
+ mbox_free_channel(smem->mbox_chan);
This might need to be moved above device_unregister. I think the release
function frees the smem structure.
}
EXPORT_SYMBOL_GPL(qcom_glink_smem_unregister);
