Received-SPF: pass (google.com: domain of linux-kernel+bounces-149653-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <b8ba5179-cb36-6b13-4244-6da1ff96c2d3@quicinc.com>
Date: Thu, 18 Apr 2024 13:01:50 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH V3 2/5] mailbox: Add support for QTI CPUCP mailbox
 controller
Content-Language: en-US
To: Bjorn Andersson <quic_bjorande@quicinc.com>
CC: <sudeep.holla@arm.com>, <cristian.marussi@arm.com>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <jassisinghbrar@gmail.com>,
        <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <dmitry.baryshkov@linaro.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <quic_rgottimu@quicinc.com>, <quic_kshivnan@quicinc.com>,
        <conor+dt@kernel.org>, <quic_gkohli@quicinc.com>,
        <quic_nkela@quicinc.com>, <quic_psodagud@quicinc.com>
References: <20240417132856.1106250-1-quic_sibis@quicinc.com>
 <20240417132856.1106250-3-quic_sibis@quicinc.com>
 <ZiA+mE3QduLxc+a5@hu-bjorande-lv.qualcomm.com>
From: Sibi Sankar <quic_sibis@quicinc.com>
In-Reply-To: <ZiA+mE3QduLxc+a5@hu-bjorande-lv.qualcomm.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 4/18/24 02:56, Bjorn Andersson wrote:
> On Wed, Apr 17, 2024 at 06:58:53PM +0530, Sibi Sankar wrote:
>> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
>> new file mode 100644
>> index 000000000000..059eb25f217c
>> --- /dev/null
>> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
>> @@ -0,0 +1,188 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*

Hey Bjorn,

Thanks for taking time to review the series :)

>> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> 
> Nope.

ack, artefact from the v1 of legacy driver :(

> 
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/module.h>
>> +
>> +#define APSS_CPUCP_IPC_CHAN_SUPPORTED		3
>> +#define APSS_CPUCP_MBOX_CMD_OFF			0x4
>> +
>> +/* Tx Registers */
>> +#define APSS_CPUCP_TX_MBOX_IDR			0
>> +#define APSS_CPUCP_TX_MBOX_CMD(i)		(0x100 + ((i) * 8))
>> +
>> +/* Rx Registers */
>> +#define APSS_CPUCP_RX_MBOX_IDR			0
>> +#define APSS_CPUCP_RX_MBOX_CMD(i)		(0x100 + ((i) * 8))
>> +#define APSS_CPUCP_RX_MBOX_MAP			0x4000
>> +#define APSS_CPUCP_RX_MBOX_STAT			0x4400
>> +#define APSS_CPUCP_RX_MBOX_CLEAR		0x4800
>> +#define APSS_CPUCP_RX_MBOX_EN			0x4C00
> 
> Can we have lower case hex digits, plz?

Sure

> 
>> +#define APSS_CPUCP_RX_MBOX_CMD_MASK		0xFFFFFFFFFFFFFFFF
>> +
>> +/**
>> + * struct qcom_cpucp_mbox - Holder for the mailbox driver
>> + * @chans:			The mailbox channel
>> + * @mbox:			The mailbox controller
>> + * @tx_base:			Base address of the CPUCP tx registers
>> + * @rx_base:			Base address of the CPUCP rx registers
>> + * @dev:			Device associated with this instance
>> + * @irq:			CPUCP to AP irq
> 
> @dev and @irq can be a local variables in qcom_cpucp_mbox_probe().

Ack

> 
>> + */
>> +struct qcom_cpucp_mbox {
>> +	struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
>> +	struct mbox_controller mbox;
>> +	void __iomem *tx_base;
>> +	void __iomem *rx_base;
>> +	struct device *dev;
>> +	int irq;
>> +};
>> +
>> +static inline int channel_number(struct mbox_chan *chan)
>> +{
>> +	return chan - chan->mbox->chans;
>> +}
>> +
>> +static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>> +{
>> +	struct qcom_cpucp_mbox *cpucp = data;
>> +	struct mbox_chan *chan;
>> +	unsigned long flags;
>> +	u64 status;
>> +	u32 val;
>> +	int i;
>> +
>> +	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
>> +
>> +	for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
>> +		val = 0;
> 
> This value is immediately overwritten (or unused).

Ack

> 
>> +		if (status & BIT(i)) {
> 
> Can't you combine the for loop and this conditional into a
> for_each_bit_set()?

The only drawback I see here is if the number of channels increase to
it's full capacity of 64 since for_each_set_bit expects unsigned long.

> 
>> +			val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
>> +			chan = &cpucp->chans[i];
>> +			spin_lock_irqsave(&chan->lock, flags);
> 
> Can you please add a comment here to document that the lock is taken
> here to deal with races against client registration? (It wasn't obvious
> to me...)

This is was put in to handle irqs after channel closure. Meaning we
don't want to send data on a closed/empty channel.

> 
>> +			if (chan->cl)
>> +				mbox_chan_received_data(chan, &val);
>> +			writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
>> +			spin_unlock_irqrestore(&chan->lock, flags);
>> +		}
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>> +{
>> +	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>> +	unsigned long chan_id = channel_number(chan);
>> +	u64 val;
>> +
>> +	val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
>> +	val |= BIT(chan_id);
>> +	writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
>> +
>> +	return 0;
>> +}
>> +
>> +static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
>> +{
>> +	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>> +	unsigned long chan_id = channel_number(chan);
>> +	u64 val;
>> +
>> +	val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
>> +	val &= ~BIT(chan_id);
>> +	writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
>> +}
>> +
>> +static int qcom_cpucp_mbox_send_data(struct mbox_chan *chan, void *data)
>> +{
>> +	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>> +	unsigned long chan_id = channel_number(chan);
>> +	u32 *val = data;
>> +
>> +	writel(*val, cpucp->tx_base + APSS_CPUCP_TX_MBOX_CMD(chan_id) + APSS_CPUCP_MBOX_CMD_OFF);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
>> +	.startup = qcom_cpucp_mbox_startup,
>> +	.send_data = qcom_cpucp_mbox_send_data,
>> +	.shutdown = qcom_cpucp_mbox_shutdown
>> +};
>> +
>> +static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>> +{
>> +	struct qcom_cpucp_mbox *cpucp;
>> +	struct mbox_controller *mbox;
>> +	int ret;
>> +
>> +	cpucp = devm_kzalloc(&pdev->dev, sizeof(*cpucp), GFP_KERNEL);
>> +	if (!cpucp)
>> +		return -ENOMEM;
>> +
>> +	cpucp->dev = &pdev->dev;
>> +
>> +	cpucp->rx_base = devm_of_iomap(cpucp->dev, cpucp->dev->of_node, 0, NULL);
>> +	if (IS_ERR(cpucp->rx_base))
>> +		return PTR_ERR(cpucp->rx_base);
>> +
>> +	cpucp->tx_base = devm_of_iomap(cpucp->dev, cpucp->dev->of_node, 1, NULL);
>> +	if (IS_ERR(cpucp->tx_base))
>> +		return PTR_ERR(cpucp->tx_base);
>> +
>> +	writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
>> +	writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
>> +	writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
>> +
>> +	cpucp->irq = platform_get_irq(pdev, 0);
>> +	if (cpucp->irq < 0)
>> +		return cpucp->irq;
>> +
>> +	ret = devm_request_irq(cpucp->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
>> +			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>> +	if (ret < 0)
>> +		return dev_err_probe(cpucp->dev, ret, "Failed to register irq: %d\n", cpucp->irq);
>> +
>> +	writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
>> +
>> +	mbox = &cpucp->mbox;
>> +	mbox->dev = cpucp->dev;
>> +	mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
>> +	mbox->chans = cpucp->chans;
>> +	mbox->ops = &qcom_cpucp_mbox_chan_ops;
>> +	mbox->txdone_irq = false;
>> +	mbox->txdone_poll = false;
>> +
>> +	ret = devm_mbox_controller_register(cpucp->dev, mbox);
>> +	if (ret)
>> +		return dev_err_probe(cpucp->dev, ret, "Failed to create mailbox\n");
>> +
>> +	platform_set_drvdata(pdev, cpucp);
> 
> I don't see you using the drvdata anywhere, can we drop this?

Yeash I'll drop this in the next re-spin.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
>> +	{ .compatible = "qcom,x1e80100-cpucp-mbox"},
> 
> A space after the final '"' would be good for aesthetics.

Not sure how I missed it :(

-Sibi

> 
> Regards,
> Bjorn
> 
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
>> +
>> +static struct platform_driver qcom_cpucp_mbox_driver = {
>> +	.probe = qcom_cpucp_mbox_probe,
>> +	.driver = {
>> +		.name = "qcom_cpucp_mbox",
>> +		.of_match_table = qcom_cpucp_mbox_of_match,
>> +	},
>> +};
>> +module_platform_driver(qcom_cpucp_mbox_driver);
>> +
>> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.34.1
>>