Message-ID: <b42c753b-a475-4875-8248-5823dd007e22@quicinc.com>
Date:   Mon, 13 Feb 2023 18:40:21 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH v2 4/6] rpmsg: glink: Move irq and mbox handling to
 transports
Content-Language: en-US
To:     Bjorn Andersson <quic_bjorande@quicinc.com>,
        Bjorn Andersson <andersson@kernel.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>
CC:     <linux-arm-msm@vger.kernel.org>,
        <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230213155215.1237059-1-quic_bjorande@quicinc.com>
 <20230213155215.1237059-5-quic_bjorande@quicinc.com>
From:   Chris Lew <quic_clew@quicinc.com>
In-Reply-To: <20230213155215.1237059-5-quic_bjorande@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/13/2023 7:52 AM, 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@quicinc.com>
> ---
> 
> Changes since v1:
> - Renamed qcom_glink_native_{intr,rx}
> - Moved mbox_free_channel() before device_unregister()
> 
>   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   | 55 +++++++++++++++++++++++++++++--
>   4 files changed, 107 insertions(+), 49 deletions(-)
> 

Reviewed-by: Chris Lew <quic_clew@quicinc.com>

> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 5fd8b70271b7..8dd8cf033b2d 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);
>   }
>   
>   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_rx(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_rx);
>   
>   /* 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..9462b37eccee 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_rx(struct qcom_glink *glink);
>   
> -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..5179f834a10f 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_rx(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 a9c477df4d68..05b4fe0a7387 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_rx(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);
> @@ -280,6 +304,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);
> +	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;
> @@ -288,6 +330,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;
> @@ -298,13 +341,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(dev);
> @@ -317,8 +364,12 @@ 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);
> +
> +	mbox_free_channel(smem->mbox_chan);
> +	device_unregister(&smem->dev);
>   }
>   EXPORT_SYMBOL_GPL(qcom_glink_smem_unregister);
>