Messages in this thread | | | Date | Thu, 18 Apr 2024 13:01:50 +0530 | Subject | Re: [PATCH V3 2/5] mailbox: Add support for QTI CPUCP mailbox controller | From | Sibi Sankar <> |
| |
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 >>
| |