Message-ID: <4d9606ef-8b5c-4116-f2fe-bdd0d846ac3b@quicinc.com>
Date:   Tue, 24 Jan 2023 22:55:47 -0800
From:   Chris Lew <quic_clew@...cinc.com>
To:     Bjorn Andersson <quic_bjorande@...cinc.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>
CC:     <linux-arm-msm@...r.kernel.org>,
        <linux-remoteproc@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/6] rpmsg: glink: Move irq and mbox handling to
 transports



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@...cinc.com>
> ---
>   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);
>   

Powered by blists - more mailing lists