* Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller
2024-09-24 5:09 ` [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller Shivnandan Kumar
@ 2024-09-24 6:44 ` Dmitry Baryshkov
2024-10-03 5:42 ` Shivnandan Kumar
2024-09-24 15:27 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 1 reply; 23+ messages in thread
From: Dmitry Baryshkov @ 2024-09-24 6:44 UTC (permalink / raw)
To: Shivnandan Kumar
Cc: Sibi Sankar, Jassi Brar, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, cros-qcom-dts-watchers, Bjorn Andersson,
Konrad Dybcio, linux-arm-msm, linux-kernel, devicetree,
Ramakrishna Gottimukkula
On Tue, Sep 24, 2024 at 10:39:40AM GMT, Shivnandan Kumar wrote:
> The SC7280 CPUCP mailbox controller is compatible with legacy mailbox
> hardware.
> Implement support for this functionality which enable HLOS to
s/HLOS/Linux/
> CPUCP communication.
This enables Linux to do this and that.
>
> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> ---
> drivers/mailbox/qcom-cpucp-mbox.c | 156 +++++++++++++++++++++++-------
> 1 file changed, 122 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> index e5437c294803..faae6e069ea1 100644
> --- a/drivers/mailbox/qcom-cpucp-mbox.c
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -13,18 +13,24 @@
> #include <linux/platform_device.h>
>
> #define APSS_CPUCP_IPC_CHAN_SUPPORTED 3
> -#define APSS_CPUCP_MBOX_CMD_OFF 0x4
> -
> -/* Tx Registers */
> -#define APSS_CPUCP_TX_MBOX_CMD(i) (0x100 + ((i) * 8))
>
> /* Rx Registers */
> -#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
> -#define APSS_CPUCP_RX_MBOX_CMD_MASK GENMASK_ULL(63, 0)
> +#define APSS_CPUCP_V2_RX_MBOX_CMD_MASK GENMASK_ULL(63, 0)
> +#define APSS_CPUCP_V1_SEND_IRQ_VAL BIT(28)
> +#define APSS_CPUCP_V1_CLEAR_IRQ_VAL BIT(3)
> +#define APSS_CPUCP_V1_STATUS_IRQ_VAL BIT(3)
> +
> +struct qcom_cpucp_mbox_desc {
> + u32 enable_reg;
> + u32 map_reg;
> + u32 rx_reg;
> + u32 tx_reg;
> + u32 status_reg;
> + u32 clear_reg;
> + u32 chan_stride;
> + bool v2_mbox;
> + u32 num_chans;
> +};
>
> /**
> * struct qcom_cpucp_mbox - Holder for the mailbox driver
> @@ -35,6 +41,7 @@
> */
> struct qcom_cpucp_mbox {
> struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
> + const struct qcom_cpucp_mbox_desc *desc;
> struct mbox_controller mbox;
> void __iomem *tx_base;
> void __iomem *rx_base;
> @@ -48,13 +55,40 @@ static inline int channel_number(struct mbox_chan *chan)
> static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
qcom_cpucp_v1_mbox_irq_fn()
> {
> struct qcom_cpucp_mbox *cpucp = data;
> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> + int i;
> +
> + for (i = 0; i < desc->num_chans; i++) {
> + u32 val = readl(cpucp->rx_base + desc->status_reg + (i * desc->chan_stride));
> + struct mbox_chan *chan = &cpucp->chans[i];
> + unsigned long flags;
> +
> + if (val & APSS_CPUCP_V1_STATUS_IRQ_VAL) {
> + writel(APSS_CPUCP_V1_CLEAR_IRQ_VAL,
> + cpucp->rx_base + desc->clear_reg + (i * desc->chan_stride));
> + /* Make sure reg write is complete before proceeding */
> + mb();
> + spin_lock_irqsave(&chan->lock, flags);
> + if (chan->cl)
> + mbox_chan_received_data(chan, NULL);
v1 clears IRQ before processing, v2 does it after pinging mbox subsys.
Is that expected? What warrants such a difference?
> + spin_unlock_irqrestore(&chan->lock, flags);
> + }
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t qcom_cpucp_v2_mbox_irq_fn(int irq, void *data)
> +{
> + struct qcom_cpucp_mbox *cpucp = data;
> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> u64 status;
> int i;
>
> - status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> + status = readq(cpucp->rx_base + desc->status_reg);
>
> - for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
> - u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> + for_each_set_bit(i, (unsigned long *)&status, desc->num_chans) {
> + u32 val = readl(cpucp->rx_base + desc->rx_reg + (i * desc->chan_stride));
> struct mbox_chan *chan = &cpucp->chans[i];
> unsigned long flags;
>
> @@ -62,7 +96,7 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> spin_lock_irqsave(&chan->lock, flags);
> if (chan->cl)
> mbox_chan_received_data(chan, &val);
> - writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
> + writeq(BIT(i), cpucp->rx_base + desc->clear_reg);
> spin_unlock_irqrestore(&chan->lock, flags);
> }
>
> @@ -72,12 +106,15 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
> {
> struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> 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);
> + if (desc->v2_mbox) {
> + val = readq(cpucp->rx_base + desc->enable_reg);
> + val |= BIT(chan_id);
> + writeq(val, cpucp->rx_base + desc->enable_reg);
> + }
So, startup and shutdown are effectively empty for v1 CPUCP chips? Could
you please add two separate families of functions and two separate
mbox_chan_ops instances? Which probably will mean one register less in
the qcom_cpucp_mbox_desc structure.
>
> return 0;
> }
> @@ -85,22 +122,26 @@ static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
> static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
> {
> struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> 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);
> + if (desc->v2_mbox) {
> + val = readq(cpucp->rx_base + desc->enable_reg);
> + val &= ~BIT(chan_id);
> + writeq(val, cpucp->rx_base + desc->enable_reg);
> + }
> }
>
> 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);
> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> + u32 val = desc->v2_mbox ? *(u32 *)data : APSS_CPUCP_V1_SEND_IRQ_VAL;
> 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);
> + u32 offset = desc->v2_mbox ? (chan_id * desc->chan_stride) : 0;
>
> + writel(val, cpucp->tx_base + desc->tx_reg + offset);
> return 0;
> }
>
> @@ -112,41 +153,66 @@ static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
>
> static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
> {
> + const struct qcom_cpucp_mbox_desc *desc;
> struct device *dev = &pdev->dev;
> struct qcom_cpucp_mbox *cpucp;
> struct mbox_controller *mbox;
> + struct resource *res;
> int irq, ret;
>
> + desc = device_get_match_data(&pdev->dev);
> + if (!desc)
> + return -EINVAL;
> +
> cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
> if (!cpucp)
> return -ENOMEM;
>
> - cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> - if (IS_ERR(cpucp->rx_base))
> - return PTR_ERR(cpucp->rx_base);
> + cpucp->desc = desc;
> +
> + if (desc->v2_mbox) {
> + cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> + if (IS_ERR(cpucp->rx_base))
> + return PTR_ERR(cpucp->rx_base);
> + /* Legacy mailbox quirks due to shared region with EPSS register space */
Does that mean that on these platforms cpucp should be a child node of the EPSS? Also it might be a high time when the drivers should be switched to use regmaps.
> + } else {
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get the device base address\n");
> + return -ENODEV;
> + }
> + cpucp->rx_base = devm_ioremap(dev, res->start, resource_size(res));
> + if (!cpucp->rx_base) {
> + dev_err(dev, "Failed to ioremap the cpucp rx irq addr\n");
> + return -ENOMEM;
> + }
> + }
>
> cpucp->tx_base = devm_of_iomap(dev, 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);
> + if (desc->v2_mbox) {
> + writeq(0, cpucp->rx_base + desc->enable_reg);
> + writeq(0, cpucp->rx_base + desc->clear_reg);
> + writeq(0, cpucp->rx_base + desc->map_reg);
> + }
If these registers are only present on V2, there is no need to have them
in the qcom_cpucp_mbox_desc.
>
> irq = platform_get_irq(pdev, 0);
> if (irq < 0)
> return irq;
>
> - ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
> - IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
> + ret = devm_request_irq(dev, irq, desc->v2_mbox ? qcom_cpucp_v2_mbox_irq_fn :
> + qcom_cpucp_mbox_irq_fn, IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
> if (ret < 0)
> return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);
>
> - writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
> + if (desc->v2_mbox)
> + writeq(APSS_CPUCP_V2_RX_MBOX_CMD_MASK, cpucp->rx_base + desc->map_reg);
>
> mbox = &cpucp->mbox;
> mbox->dev = dev;
> - mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
> + mbox->num_chans = desc->num_chans;
> mbox->chans = cpucp->chans;
> mbox->ops = &qcom_cpucp_mbox_chan_ops;
>
> @@ -157,8 +223,30 @@ static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct qcom_cpucp_mbox_desc sc7280_cpucp_mbox = {
> + .tx_reg = 0xC,
> + .chan_stride = 0x1000,
> + .status_reg = 0x30C,
> + .clear_reg = 0x308,
> + .v2_mbox = false,
> + .num_chans = 2,
> +};
> +
> +static const struct qcom_cpucp_mbox_desc x1e80100_cpucp_mbox = {
> + .rx_reg = 0x104,
> + .tx_reg = 0x104,
> + .chan_stride = 0x8,
> + .map_reg = 0x4000,
> + .status_reg = 0x4400,
> + .clear_reg = 0x4800,
> + .enable_reg = 0x4C00,
> + .v2_mbox = true,
> + .num_chans = 3,
> +};
> +
> static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
> - { .compatible = "qcom,x1e80100-cpucp-mbox" },
> + { .compatible = "qcom,x1e80100-cpucp-mbox", .data = &x1e80100_cpucp_mbox},
> + { .compatible = "qcom,sc7280-cpucp-mbox", .data = &sc7280_cpucp_mbox},
Please keep the table sorted.
> {}
> };
> MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
> --
> 2.25.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller
2024-09-24 6:44 ` Dmitry Baryshkov
@ 2024-10-03 5:42 ` Shivnandan Kumar
0 siblings, 0 replies; 23+ messages in thread
From: Shivnandan Kumar @ 2024-10-03 5:42 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Sibi Sankar, Jassi Brar, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, cros-qcom-dts-watchers, Bjorn Andersson,
Konrad Dybcio, linux-arm-msm, linux-kernel, devicetree,
Ramakrishna Gottimukkula
Thanks Dmitry for reviewing this patch.
On 9/24/2024 12:14 PM, Dmitry Baryshkov wrote:
> On Tue, Sep 24, 2024 at 10:39:40AM GMT, Shivnandan Kumar wrote:
>> The SC7280 CPUCP mailbox controller is compatible with legacy mailbox
>> hardware.
>> Implement support for this functionality which enable HLOS to
>
> s/HLOS/Linux/
>
ACK
>> CPUCP communication.
>
> This enables Linux to do this and that.
>
>>
>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>> ---
>> drivers/mailbox/qcom-cpucp-mbox.c | 156 +++++++++++++++++++++++-------
>> 1 file changed, 122 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
>> index e5437c294803..faae6e069ea1 100644
>> --- a/drivers/mailbox/qcom-cpucp-mbox.c
>> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
>> @@ -13,18 +13,24 @@
>> #include <linux/platform_device.h>
>>
>> #define APSS_CPUCP_IPC_CHAN_SUPPORTED 3
>> -#define APSS_CPUCP_MBOX_CMD_OFF 0x4
>> -
>> -/* Tx Registers */
>> -#define APSS_CPUCP_TX_MBOX_CMD(i) (0x100 + ((i) * 8))
>>
>> /* Rx Registers */
>> -#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
>> -#define APSS_CPUCP_RX_MBOX_CMD_MASK GENMASK_ULL(63, 0)
>> +#define APSS_CPUCP_V2_RX_MBOX_CMD_MASK GENMASK_ULL(63, 0)
>> +#define APSS_CPUCP_V1_SEND_IRQ_VAL BIT(28)
>> +#define APSS_CPUCP_V1_CLEAR_IRQ_VAL BIT(3)
>> +#define APSS_CPUCP_V1_STATUS_IRQ_VAL BIT(3)
>> +
>> +struct qcom_cpucp_mbox_desc {
>> + u32 enable_reg;
>> + u32 map_reg;
>> + u32 rx_reg;
>> + u32 tx_reg;
>> + u32 status_reg;
>> + u32 clear_reg;
>> + u32 chan_stride;
>> + bool v2_mbox;
>> + u32 num_chans;
>> +};
>>
>> /**
>> * struct qcom_cpucp_mbox - Holder for the mailbox driver
>> @@ -35,6 +41,7 @@
>> */
>> struct qcom_cpucp_mbox {
>> struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
>> + const struct qcom_cpucp_mbox_desc *desc;
>> struct mbox_controller mbox;
>> void __iomem *tx_base;
>> void __iomem *rx_base;
>> @@ -48,13 +55,40 @@ static inline int channel_number(struct mbox_chan *chan)
>> static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>
> qcom_cpucp_v1_mbox_irq_fn()
>
>> {
>> struct qcom_cpucp_mbox *cpucp = data;
>> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> + int i;
>> +
>> + for (i = 0; i < desc->num_chans; i++) {
>> + u32 val = readl(cpucp->rx_base + desc->status_reg + (i * desc->chan_stride));
>> + struct mbox_chan *chan = &cpucp->chans[i];
>> + unsigned long flags;
>> +
>> + if (val & APSS_CPUCP_V1_STATUS_IRQ_VAL) {
>> + writel(APSS_CPUCP_V1_CLEAR_IRQ_VAL,
>> + cpucp->rx_base + desc->clear_reg + (i * desc->chan_stride));
>> + /* Make sure reg write is complete before proceeding */
>> + mb();
>> + spin_lock_irqsave(&chan->lock, flags);
>> + if (chan->cl)
>> + mbox_chan_received_data(chan, NULL);
>
> v1 clears IRQ before processing, v2 does it after pinging mbox subsys.
> Is that expected? What warrants such a difference?
>
ACK, I will change it to make it same as v2.
>> + spin_unlock_irqrestore(&chan->lock, flags);
>> + }
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t qcom_cpucp_v2_mbox_irq_fn(int irq, void *data)
>> +{
>> + struct qcom_cpucp_mbox *cpucp = data;
>> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> u64 status;
>> int i;
>>
>> - status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
>> + status = readq(cpucp->rx_base + desc->status_reg);
>>
>> - for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
>> - u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
>> + for_each_set_bit(i, (unsigned long *)&status, desc->num_chans) {
>> + u32 val = readl(cpucp->rx_base + desc->rx_reg + (i * desc->chan_stride));
>> struct mbox_chan *chan = &cpucp->chans[i];
>> unsigned long flags;
>>
>> @@ -62,7 +96,7 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>> spin_lock_irqsave(&chan->lock, flags);
>> if (chan->cl)
>> mbox_chan_received_data(chan, &val);
>> - writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
>> + writeq(BIT(i), cpucp->rx_base + desc->clear_reg);
>> spin_unlock_irqrestore(&chan->lock, flags);
>> }
>>
>> @@ -72,12 +106,15 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>> static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>> {
>> struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> 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);
>> + if (desc->v2_mbox) {
>> + val = readq(cpucp->rx_base + desc->enable_reg);
>> + val |= BIT(chan_id);
>> + writeq(val, cpucp->rx_base + desc->enable_reg);
>> + }
>
> So, startup and shutdown are effectively empty for v1 CPUCP chips? Could
> you please add two separate families of functions and two separate
> mbox_chan_ops instances? Which probably will mean one register less in
> the qcom_cpucp_mbox_desc structure.
>
ACK,
>>
>> return 0;
>> }
>> @@ -85,22 +122,26 @@ static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>> static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
>> {
>> struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> 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);
>> + if (desc->v2_mbox) {
>> + val = readq(cpucp->rx_base + desc->enable_reg);
>> + val &= ~BIT(chan_id);
>> + writeq(val, cpucp->rx_base + desc->enable_reg);
>> + }
>> }
>>
>> 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);
>> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> + u32 val = desc->v2_mbox ? *(u32 *)data : APSS_CPUCP_V1_SEND_IRQ_VAL;
>> 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);
>> + u32 offset = desc->v2_mbox ? (chan_id * desc->chan_stride) : 0;
>>
>> + writel(val, cpucp->tx_base + desc->tx_reg + offset);
>> return 0;
>> }
>>
>> @@ -112,41 +153,66 @@ static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
>>
>> static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>> {
>> + const struct qcom_cpucp_mbox_desc *desc;
>> struct device *dev = &pdev->dev;
>> struct qcom_cpucp_mbox *cpucp;
>> struct mbox_controller *mbox;
>> + struct resource *res;
>> int irq, ret;
>>
>> + desc = device_get_match_data(&pdev->dev);
>> + if (!desc)
>> + return -EINVAL;
>> +
>> cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
>> if (!cpucp)
>> return -ENOMEM;
>>
>> - cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
>> - if (IS_ERR(cpucp->rx_base))
>> - return PTR_ERR(cpucp->rx_base);
>> + cpucp->desc = desc;
>> +
>> + if (desc->v2_mbox) {
>> + cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
>> + if (IS_ERR(cpucp->rx_base))
>> + return PTR_ERR(cpucp->rx_base);
>> + /* Legacy mailbox quirks due to shared region with EPSS register space */
>
> Does that mean that on these platforms cpucp should be a child node of the EPSS? Also it might be a high time when the drivers should be switched to use regmaps.
>
This will not work as mailbox tx path is not shared with EPSS.
>> + } else {
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + dev_err(&pdev->dev, "Failed to get the device base address\n");
>> + return -ENODEV;
>> + }
>> + cpucp->rx_base = devm_ioremap(dev, res->start, resource_size(res));
>> + if (!cpucp->rx_base) {
>> + dev_err(dev, "Failed to ioremap the cpucp rx irq addr\n");
>> + return -ENOMEM;
>> + }
>> + }
>>
>> cpucp->tx_base = devm_of_iomap(dev, 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);
>> + if (desc->v2_mbox) {
>> + writeq(0, cpucp->rx_base + desc->enable_reg);
>> + writeq(0, cpucp->rx_base + desc->clear_reg);
>> + writeq(0, cpucp->rx_base + desc->map_reg);
>> + }
>
> If these registers are only present on V2, there is no need to have them
> in the qcom_cpucp_mbox_desc.
>
ACK
>>
>> irq = platform_get_irq(pdev, 0);
>> if (irq < 0)
>> return irq;
>>
>> - ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
>> - IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>> + ret = devm_request_irq(dev, irq, desc->v2_mbox ? qcom_cpucp_v2_mbox_irq_fn :
>> + qcom_cpucp_mbox_irq_fn, IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>> if (ret < 0)
>> return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);
>>
>> - writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
>> + if (desc->v2_mbox)
>> + writeq(APSS_CPUCP_V2_RX_MBOX_CMD_MASK, cpucp->rx_base + desc->map_reg);
>>
>> mbox = &cpucp->mbox;
>> mbox->dev = dev;
>> - mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
>> + mbox->num_chans = desc->num_chans;
>> mbox->chans = cpucp->chans;
>> mbox->ops = &qcom_cpucp_mbox_chan_ops;
>>
>> @@ -157,8 +223,30 @@ static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static const struct qcom_cpucp_mbox_desc sc7280_cpucp_mbox = {
>> + .tx_reg = 0xC,
>> + .chan_stride = 0x1000,
>> + .status_reg = 0x30C,
>> + .clear_reg = 0x308,
>> + .v2_mbox = false,
>> + .num_chans = 2,
>> +};
>> +
>> +static const struct qcom_cpucp_mbox_desc x1e80100_cpucp_mbox = {
>> + .rx_reg = 0x104,
>> + .tx_reg = 0x104,
>> + .chan_stride = 0x8,
>> + .map_reg = 0x4000,
>> + .status_reg = 0x4400,
>> + .clear_reg = 0x4800,
>> + .enable_reg = 0x4C00,
>> + .v2_mbox = true,
>> + .num_chans = 3,
>> +};
>> +
>> static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
>> - { .compatible = "qcom,x1e80100-cpucp-mbox" },
>> + { .compatible = "qcom,x1e80100-cpucp-mbox", .data = &x1e80100_cpucp_mbox},
>> + { .compatible = "qcom,sc7280-cpucp-mbox", .data = &sc7280_cpucp_mbox},
>
> Please keep the table sorted.
>
ACK
Thanks,
Shivnandan
>> {}
>> };
>> MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
>> --
>> 2.25.1
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller
2024-09-24 5:09 ` [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller Shivnandan Kumar
2024-09-24 6:44 ` Dmitry Baryshkov
@ 2024-09-24 15:27 ` kernel test robot
2024-10-06 2:33 ` Bjorn Andersson
2024-10-17 21:15 ` Konrad Dybcio
3 siblings, 0 replies; 23+ messages in thread
From: kernel test robot @ 2024-09-24 15:27 UTC (permalink / raw)
To: Shivnandan Kumar, Sibi Sankar, Jassi Brar, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, cros-qcom-dts-watchers,
Bjorn Andersson, Konrad Dybcio
Cc: oe-kbuild-all, linux-arm-msm, linux-kernel, devicetree,
Ramakrishna Gottimukkula, Shivnandan Kumar
Hi Shivnandan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.11 next-20240924]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Shivnandan-Kumar/dt-bindings-mailbox-qcom-cpucp-mbox-Add-sc7280-cpucp-mailbox-instance/20240924-133657
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240924050941.1251485-3-quic_kshivnan%40quicinc.com
patch subject: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240924/202409242349.yXq4CDaH-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409242349.yXq4CDaH-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409242349.yXq4CDaH-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/mailbox/qcom-cpucp-mbox.c:48: warning: Function parameter or struct member 'desc' not described in 'qcom_cpucp_mbox'
vim +48 drivers/mailbox/qcom-cpucp-mbox.c
0e2a9a03106cd5 Sibi Sankar 2024-06-12 34
0e2a9a03106cd5 Sibi Sankar 2024-06-12 35 /**
0e2a9a03106cd5 Sibi Sankar 2024-06-12 36 * struct qcom_cpucp_mbox - Holder for the mailbox driver
0e2a9a03106cd5 Sibi Sankar 2024-06-12 37 * @chans: The mailbox channel
0e2a9a03106cd5 Sibi Sankar 2024-06-12 38 * @mbox: The mailbox controller
0e2a9a03106cd5 Sibi Sankar 2024-06-12 39 * @tx_base: Base address of the CPUCP tx registers
0e2a9a03106cd5 Sibi Sankar 2024-06-12 40 * @rx_base: Base address of the CPUCP rx registers
0e2a9a03106cd5 Sibi Sankar 2024-06-12 41 */
0e2a9a03106cd5 Sibi Sankar 2024-06-12 42 struct qcom_cpucp_mbox {
0e2a9a03106cd5 Sibi Sankar 2024-06-12 43 struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
654ed3904f995a Shivnandan Kumar 2024-09-24 44 const struct qcom_cpucp_mbox_desc *desc;
0e2a9a03106cd5 Sibi Sankar 2024-06-12 45 struct mbox_controller mbox;
0e2a9a03106cd5 Sibi Sankar 2024-06-12 46 void __iomem *tx_base;
0e2a9a03106cd5 Sibi Sankar 2024-06-12 47 void __iomem *rx_base;
0e2a9a03106cd5 Sibi Sankar 2024-06-12 @48 };
0e2a9a03106cd5 Sibi Sankar 2024-06-12 49
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller
2024-09-24 5:09 ` [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller Shivnandan Kumar
2024-09-24 6:44 ` Dmitry Baryshkov
2024-09-24 15:27 ` kernel test robot
@ 2024-10-06 2:33 ` Bjorn Andersson
2024-10-17 11:52 ` Shivnandan Kumar
2024-10-17 21:15 ` Konrad Dybcio
3 siblings, 1 reply; 23+ messages in thread
From: Bjorn Andersson @ 2024-10-06 2:33 UTC (permalink / raw)
To: Shivnandan Kumar
Cc: Sibi Sankar, Jassi Brar, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, cros-qcom-dts-watchers, Konrad Dybcio,
linux-arm-msm, linux-kernel, devicetree, Ramakrishna Gottimukkula
On Tue, Sep 24, 2024 at 10:39:40AM GMT, Shivnandan Kumar wrote:
> The SC7280 CPUCP mailbox controller is compatible with legacy mailbox
> hardware.
"mailbox hardware" is a very vague description of something.
> Implement support for this functionality which enable HLOS to
> CPUCP communication.
>
Please describe the problem that this solves. What "legacy mailbox
hardware"? Why do you want to talk to the CPUCP? What is a HLOS? What
is the CPUCP?
It seems from the patch that the current implementation supports
something we call "version 2" of the cpucp mailbox interface and you're
adding support for v1. Please make sure that the commit message describe
such things.
> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> ---
> drivers/mailbox/qcom-cpucp-mbox.c | 156 +++++++++++++++++++++++-------
> 1 file changed, 122 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> index e5437c294803..faae6e069ea1 100644
> --- a/drivers/mailbox/qcom-cpucp-mbox.c
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -13,18 +13,24 @@
> #include <linux/platform_device.h>
>
> #define APSS_CPUCP_IPC_CHAN_SUPPORTED 3
> -#define APSS_CPUCP_MBOX_CMD_OFF 0x4
> -
> -/* Tx Registers */
> -#define APSS_CPUCP_TX_MBOX_CMD(i) (0x100 + ((i) * 8))
>
> /* Rx Registers */
> -#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
> -#define APSS_CPUCP_RX_MBOX_CMD_MASK GENMASK_ULL(63, 0)
> +#define APSS_CPUCP_V2_RX_MBOX_CMD_MASK GENMASK_ULL(63, 0)
> +#define APSS_CPUCP_V1_SEND_IRQ_VAL BIT(28)
> +#define APSS_CPUCP_V1_CLEAR_IRQ_VAL BIT(3)
> +#define APSS_CPUCP_V1_STATUS_IRQ_VAL BIT(3)
> +
> +struct qcom_cpucp_mbox_desc {
> + u32 enable_reg;
Do you really need these parameters to be dynamic? E.g. you only touch
enable_reg from the v2 code paths...
> + u32 map_reg;
> + u32 rx_reg;
> + u32 tx_reg;
> + u32 status_reg;
> + u32 clear_reg;
> + u32 chan_stride;
"u32" tells me that this has to be 32 bits, e.g. because the value is
going into a register... But these are just offsets...
Please use "unsigned int" to denote "a natural number".
> + bool v2_mbox;
How about "version" and give it a value 1 or 2?
> + u32 num_chans;
> +};
>
> /**
> * struct qcom_cpucp_mbox - Holder for the mailbox driver
> @@ -35,6 +41,7 @@
> */
> struct qcom_cpucp_mbox {
> struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
> + const struct qcom_cpucp_mbox_desc *desc;
> struct mbox_controller mbox;
> void __iomem *tx_base;
> void __iomem *rx_base;
> @@ -48,13 +55,40 @@ static inline int channel_number(struct mbox_chan *chan)
> static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
Why is the existing function renamed "v2" and this newly introduced
function not given a version?
> {
> struct qcom_cpucp_mbox *cpucp = data;
> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> + int i;
> +
> + for (i = 0; i < desc->num_chans; i++) {
> + u32 val = readl(cpucp->rx_base + desc->status_reg + (i * desc->chan_stride));
> + struct mbox_chan *chan = &cpucp->chans[i];
> + unsigned long flags;
> +
> + if (val & APSS_CPUCP_V1_STATUS_IRQ_VAL) {
> + writel(APSS_CPUCP_V1_CLEAR_IRQ_VAL,
> + cpucp->rx_base + desc->clear_reg + (i * desc->chan_stride));
> + /* Make sure reg write is complete before proceeding */
> + mb();
> + spin_lock_irqsave(&chan->lock, flags);
> + if (chan->cl)
> + mbox_chan_received_data(chan, NULL);
> + spin_unlock_irqrestore(&chan->lock, flags);
> + }
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t qcom_cpucp_v2_mbox_irq_fn(int irq, void *data)
> +{
> + struct qcom_cpucp_mbox *cpucp = data;
> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> u64 status;
> int i;
>
> - status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> + status = readq(cpucp->rx_base + desc->status_reg);
>
> - for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
> - u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> + for_each_set_bit(i, (unsigned long *)&status, desc->num_chans) {
> + u32 val = readl(cpucp->rx_base + desc->rx_reg + (i * desc->chan_stride));
> struct mbox_chan *chan = &cpucp->chans[i];
> unsigned long flags;
>
> @@ -62,7 +96,7 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> spin_lock_irqsave(&chan->lock, flags);
> if (chan->cl)
> mbox_chan_received_data(chan, &val);
> - writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
> + writeq(BIT(i), cpucp->rx_base + desc->clear_reg);
> spin_unlock_irqrestore(&chan->lock, flags);
> }
>
> @@ -72,12 +106,15 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
> {
> struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> 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);
> + if (desc->v2_mbox) {
> + val = readq(cpucp->rx_base + desc->enable_reg);
> + val |= BIT(chan_id);
> + writeq(val, cpucp->rx_base + desc->enable_reg);
> + }
No equivalent in "legacy"?
>
> return 0;
> }
> @@ -85,22 +122,26 @@ static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
> static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
> {
> struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> 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);
> + if (desc->v2_mbox) {
> + val = readq(cpucp->rx_base + desc->enable_reg);
> + val &= ~BIT(chan_id);
> + writeq(val, cpucp->rx_base + desc->enable_reg);
> + }
Ditto
> }
>
> 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);
> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> + u32 val = desc->v2_mbox ? *(u32 *)data : APSS_CPUCP_V1_SEND_IRQ_VAL;
Please rewrite this without ternary operators.
> 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);
> + u32 offset = desc->v2_mbox ? (chan_id * desc->chan_stride) : 0;
>
> + writel(val, cpucp->tx_base + desc->tx_reg + offset);
> return 0;
> }
>
> @@ -112,41 +153,66 @@ static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
>
> static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
> {
> + const struct qcom_cpucp_mbox_desc *desc;
> struct device *dev = &pdev->dev;
> struct qcom_cpucp_mbox *cpucp;
> struct mbox_controller *mbox;
> + struct resource *res;
> int irq, ret;
>
> + desc = device_get_match_data(&pdev->dev);
> + if (!desc)
> + return -EINVAL;
> +
> cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
> if (!cpucp)
> return -ENOMEM;
>
> - cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> - if (IS_ERR(cpucp->rx_base))
> - return PTR_ERR(cpucp->rx_base);
> + cpucp->desc = desc;
> +
> + if (desc->v2_mbox) {
> + cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> + if (IS_ERR(cpucp->rx_base))
> + return PTR_ERR(cpucp->rx_base);
> + /* Legacy mailbox quirks due to shared region with EPSS register space */
Why can't we have the same code in both cases?
> + } else {
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get the device base address\n");
It's not only base address.
> + return -ENODEV;
> + }
> + cpucp->rx_base = devm_ioremap(dev, res->start, resource_size(res));
> + if (!cpucp->rx_base) {
> + dev_err(dev, "Failed to ioremap the cpucp rx irq addr\n");
> + return -ENOMEM;
> + }
> + }
>
> cpucp->tx_base = devm_of_iomap(dev, 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);
> + if (desc->v2_mbox) {
> + writeq(0, cpucp->rx_base + desc->enable_reg);
> + writeq(0, cpucp->rx_base + desc->clear_reg);
> + writeq(0, cpucp->rx_base + desc->map_reg);
Is there a reason why the legacy system does not need or want to clear
these?
> + }
>
> irq = platform_get_irq(pdev, 0);
> if (irq < 0)
> return irq;
>
> - ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
> - IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
> + ret = devm_request_irq(dev, irq, desc->v2_mbox ? qcom_cpucp_v2_mbox_irq_fn :
> + qcom_cpucp_mbox_irq_fn, IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
The use of a ternary operator, in combination with odd line wrapping
makes this completely unreadable. Please fix.
> if (ret < 0)
> return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);
>
> - writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
> + if (desc->v2_mbox)
> + writeq(APSS_CPUCP_V2_RX_MBOX_CMD_MASK, cpucp->rx_base + desc->map_reg);
>
> mbox = &cpucp->mbox;
> mbox->dev = dev;
> - mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
> + mbox->num_chans = desc->num_chans;
> mbox->chans = cpucp->chans;
> mbox->ops = &qcom_cpucp_mbox_chan_ops;
>
> @@ -157,8 +223,30 @@ static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct qcom_cpucp_mbox_desc sc7280_cpucp_mbox = {
> + .tx_reg = 0xC,
> + .chan_stride = 0x1000,
> + .status_reg = 0x30C,
Lowercase hex digits please (although the question above whether these
needs to be defined remains).
> + .clear_reg = 0x308,
> + .v2_mbox = false,
> + .num_chans = 2,
> +};
> +
> +static const struct qcom_cpucp_mbox_desc x1e80100_cpucp_mbox = {
> + .rx_reg = 0x104,
> + .tx_reg = 0x104,
> + .chan_stride = 0x8,
> + .map_reg = 0x4000,
> + .status_reg = 0x4400,
> + .clear_reg = 0x4800,
> + .enable_reg = 0x4C00,
> + .v2_mbox = true,
> + .num_chans = 3,
> +};
> +
> static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
> - { .compatible = "qcom,x1e80100-cpucp-mbox" },
> + { .compatible = "qcom,x1e80100-cpucp-mbox", .data = &x1e80100_cpucp_mbox},
> + { .compatible = "qcom,sc7280-cpucp-mbox", .data = &sc7280_cpucp_mbox},
Perhaps I'm missing something, but seems like the only information you
actually need to pass here is 1 or 2, to denote which version/code paths
you should take through the driver.
Regards,
Bjorn
> {}
> };
> MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller
2024-10-06 2:33 ` Bjorn Andersson
@ 2024-10-17 11:52 ` Shivnandan Kumar
2024-10-18 14:44 ` Bjorn Andersson
0 siblings, 1 reply; 23+ messages in thread
From: Shivnandan Kumar @ 2024-10-17 11:52 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Sibi Sankar, Jassi Brar, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, cros-qcom-dts-watchers, Konrad Dybcio,
linux-arm-msm, linux-kernel, devicetree, Ramakrishna Gottimukkula
Thank you, Bjorn, for taking the time to review this patch series.
On 10/6/2024 8:03 AM, Bjorn Andersson wrote:
> On Tue, Sep 24, 2024 at 10:39:40AM GMT, Shivnandan Kumar wrote:
>> The SC7280 CPUCP mailbox controller is compatible with legacy mailbox
>> hardware.
>
> "mailbox hardware" is a very vague description of something.
>
>> Implement support for this functionality which enable HLOS to
>> CPUCP communication.
>>
>
> Please describe the problem that this solves. What "legacy mailbox
> hardware"? Why do you want to talk to the CPUCP? What is a HLOS? What
> is the CPUCP?
>
ACK, I will add description in next patch series.
> It seems from the patch that the current implementation supports
> something we call "version 2" of the cpucp mailbox interface and you're
> adding support for v1. Please make sure that the commit message describe
> such things.
>
ACK
>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>> ---
>> drivers/mailbox/qcom-cpucp-mbox.c | 156 +++++++++++++++++++++++-------
>> 1 file changed, 122 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
>> index e5437c294803..faae6e069ea1 100644
>> --- a/drivers/mailbox/qcom-cpucp-mbox.c
>> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
>> @@ -13,18 +13,24 @@
>> #include <linux/platform_device.h>
>>
>> #define APSS_CPUCP_IPC_CHAN_SUPPORTED 3
>> -#define APSS_CPUCP_MBOX_CMD_OFF 0x4
>> -
>> -/* Tx Registers */
>> -#define APSS_CPUCP_TX_MBOX_CMD(i) (0x100 + ((i) * 8))
>>
>> /* Rx Registers */
>> -#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
>> -#define APSS_CPUCP_RX_MBOX_CMD_MASK GENMASK_ULL(63, 0)
>> +#define APSS_CPUCP_V2_RX_MBOX_CMD_MASK GENMASK_ULL(63, 0)
>> +#define APSS_CPUCP_V1_SEND_IRQ_VAL BIT(28)
>> +#define APSS_CPUCP_V1_CLEAR_IRQ_VAL BIT(3)
>> +#define APSS_CPUCP_V1_STATUS_IRQ_VAL BIT(3)
>> +
>> +struct qcom_cpucp_mbox_desc {
>> + u32 enable_reg;
>
> Do you really need these parameters to be dynamic? E.g. you only touch
> enable_reg from the v2 code paths...
>
Will remove this in next patch series.
>> + u32 map_reg;
>> + u32 rx_reg;
>> + u32 tx_reg;
>> + u32 status_reg;
>> + u32 clear_reg;
>> + u32 chan_stride;
>
> "u32" tells me that this has to be 32 bits, e.g. because the value is
> going into a register... But these are just offsets...
>
> Please use "unsigned int" to denote "a natural number".
>
ACK
>> + bool v2_mbox;
>
> How about "version" and give it a value 1 or 2?
>
Ok, will do like that.
>> + u32 num_chans;
>> +};
>>
>> /**
>> * struct qcom_cpucp_mbox - Holder for the mailbox driver
>> @@ -35,6 +41,7 @@
>> */
>> struct qcom_cpucp_mbox {
>> struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
>> + const struct qcom_cpucp_mbox_desc *desc;
>> struct mbox_controller mbox;
>> void __iomem *tx_base;
>> void __iomem *rx_base;
>> @@ -48,13 +55,40 @@ static inline int channel_number(struct mbox_chan *chan)
>> static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>
> Why is the existing function renamed "v2" and this newly introduced
> function not given a version?
>
ACK
>> {
>> struct qcom_cpucp_mbox *cpucp = data;
>> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> + int i;
>> +
>> + for (i = 0; i < desc->num_chans; i++) {
>> + u32 val = readl(cpucp->rx_base + desc->status_reg + (i * desc->chan_stride));
>> + struct mbox_chan *chan = &cpucp->chans[i];
>> + unsigned long flags;
>> +
>> + if (val & APSS_CPUCP_V1_STATUS_IRQ_VAL) {
>> + writel(APSS_CPUCP_V1_CLEAR_IRQ_VAL,
>> + cpucp->rx_base + desc->clear_reg + (i * desc->chan_stride));
>> + /* Make sure reg write is complete before proceeding */
>> + mb();
>> + spin_lock_irqsave(&chan->lock, flags);
>> + if (chan->cl)
>> + mbox_chan_received_data(chan, NULL);
>> + spin_unlock_irqrestore(&chan->lock, flags);
>> + }
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t qcom_cpucp_v2_mbox_irq_fn(int irq, void *data)
>> +{
>> + struct qcom_cpucp_mbox *cpucp = data;
>> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> u64 status;
>> int i;
>>
>> - status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
>> + status = readq(cpucp->rx_base + desc->status_reg);
>>
>> - for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
>> - u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
>> + for_each_set_bit(i, (unsigned long *)&status, desc->num_chans) {
>> + u32 val = readl(cpucp->rx_base + desc->rx_reg + (i * desc->chan_stride));
>> struct mbox_chan *chan = &cpucp->chans[i];
>> unsigned long flags;
>>
>> @@ -62,7 +96,7 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>> spin_lock_irqsave(&chan->lock, flags);
>> if (chan->cl)
>> mbox_chan_received_data(chan, &val);
>> - writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
>> + writeq(BIT(i), cpucp->rx_base + desc->clear_reg);
>> spin_unlock_irqrestore(&chan->lock, flags);
>> }
>>
>> @@ -72,12 +106,15 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>> static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>> {
>> struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> 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);
>> + if (desc->v2_mbox) {
>> + val = readq(cpucp->rx_base + desc->enable_reg);
>> + val |= BIT(chan_id);
>> + writeq(val, cpucp->rx_base + desc->enable_reg);
>> + }
>
> No equivalent in "legacy"?
Yes, right
>>
>> return 0;
>> }
>> @@ -85,22 +122,26 @@ static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>> static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
>> {
>> struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> 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);
>> + if (desc->v2_mbox) {
>> + val = readq(cpucp->rx_base + desc->enable_reg);
>> + val &= ~BIT(chan_id);
>> + writeq(val, cpucp->rx_base + desc->enable_reg);
>> + }
>
> Ditto
>
We do not have equivalent in "legacy".
>> }
>>
>> 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);
>> + const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> + u32 val = desc->v2_mbox ? *(u32 *)data : APSS_CPUCP_V1_SEND_IRQ_VAL;
>
> Please rewrite this without ternary operators.
>
ACK
>> 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);
>> + u32 offset = desc->v2_mbox ? (chan_id * desc->chan_stride) : 0;
>>
>> + writel(val, cpucp->tx_base + desc->tx_reg + offset);
>> return 0;
>> }
>>
>> @@ -112,41 +153,66 @@ static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
>>
>> static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>> {
>> + const struct qcom_cpucp_mbox_desc *desc;
>> struct device *dev = &pdev->dev;
>> struct qcom_cpucp_mbox *cpucp;
>> struct mbox_controller *mbox;
>> + struct resource *res;
>> int irq, ret;
>>
>> + desc = device_get_match_data(&pdev->dev);
>> + if (!desc)
>> + return -EINVAL;
>> +
>> cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
>> if (!cpucp)
>> return -ENOMEM;
>>
>> - cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
>> - if (IS_ERR(cpucp->rx_base))
>> - return PTR_ERR(cpucp->rx_base);
>> + cpucp->desc = desc;
>> +
>> + if (desc->v2_mbox) {
>> + cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
>> + if (IS_ERR(cpucp->rx_base))
>> + return PTR_ERR(cpucp->rx_base);
>> + /* Legacy mailbox quirks due to shared region with EPSS register space */
>
> Why can't we have the same code in both cases?
>
RX address space share region with EPSS. Due to which devm_of_iomap
returns -EBUSY.
>> + } else {
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + dev_err(&pdev->dev, "Failed to get the device base address\n");
>
> It's not only base address.
>
Will add appropriate print statement.
>> + return -ENODEV;
>> + }
>> + cpucp->rx_base = devm_ioremap(dev, res->start, resource_size(res));
>> + if (!cpucp->rx_base) {
>> + dev_err(dev, "Failed to ioremap the cpucp rx irq addr\n");
>> + return -ENOMEM;
>> + }
>> + }
>>
>> cpucp->tx_base = devm_of_iomap(dev, 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);
>> + if (desc->v2_mbox) {
>> + writeq(0, cpucp->rx_base + desc->enable_reg);
>> + writeq(0, cpucp->rx_base + desc->clear_reg);
>> + writeq(0, cpucp->rx_base + desc->map_reg);
>
>
> Is there a reason why the legacy system does not need or want to clear
> these?
>
Legacy system does not have equivalent registers.
>> + }
>>
>> irq = platform_get_irq(pdev, 0);
>> if (irq < 0)
>> return irq;
>>
>> - ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
>> - IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>> + ret = devm_request_irq(dev, irq, desc->v2_mbox ? qcom_cpucp_v2_mbox_irq_fn :
>> + qcom_cpucp_mbox_irq_fn, IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>
> The use of a ternary operator, in combination with odd line wrapping
> makes this completely unreadable. Please fix.
>
ACK
>> if (ret < 0)
>> return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);
>>
>> - writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
>> + if (desc->v2_mbox)
>> + writeq(APSS_CPUCP_V2_RX_MBOX_CMD_MASK, cpucp->rx_base + desc->map_reg);
>>
>> mbox = &cpucp->mbox;
>> mbox->dev = dev;
>> - mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
>> + mbox->num_chans = desc->num_chans;
>> mbox->chans = cpucp->chans;
>> mbox->ops = &qcom_cpucp_mbox_chan_ops;
>>
>> @@ -157,8 +223,30 @@ static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static const struct qcom_cpucp_mbox_desc sc7280_cpucp_mbox = {
>> + .tx_reg = 0xC,
>> + .chan_stride = 0x1000,
>> + .status_reg = 0x30C,
>
> Lowercase hex digits please (although the question above whether these
> needs to be defined remains).
ACK
>
>> + .clear_reg = 0x308,
>> + .v2_mbox = false,
>> + .num_chans = 2,
>> +};
>> +
>> +static const struct qcom_cpucp_mbox_desc x1e80100_cpucp_mbox = {
>> + .rx_reg = 0x104,
>> + .tx_reg = 0x104,
>> + .chan_stride = 0x8,
>> + .map_reg = 0x4000,
>> + .status_reg = 0x4400,
>> + .clear_reg = 0x4800,
>> + .enable_reg = 0x4C00,
>> + .v2_mbox = true,
>> + .num_chans = 3,
>> +};
>> +
>> static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
>> - { .compatible = "qcom,x1e80100-cpucp-mbox" },
>> + { .compatible = "qcom,x1e80100-cpucp-mbox", .data = &x1e80100_cpucp_mbox},
>> + { .compatible = "qcom,sc7280-cpucp-mbox", .data = &sc7280_cpucp_mbox},
>
> Perhaps I'm missing something, but seems like the only information you
> actually need to pass here is 1 or 2, to denote which version/code paths
> you should take through the driver.
>
okay, I will rename sc7280_cpucp_mbox and x1e80100_cpucp_mbox structures
as v1 and v2 respectively.
> Regards,
> Bjorn
>
>> {}
>> };
>> MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller
2024-10-17 11:52 ` Shivnandan Kumar
@ 2024-10-18 14:44 ` Bjorn Andersson
0 siblings, 0 replies; 23+ messages in thread
From: Bjorn Andersson @ 2024-10-18 14:44 UTC (permalink / raw)
To: Shivnandan Kumar
Cc: Bjorn Andersson, Sibi Sankar, Jassi Brar, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, cros-qcom-dts-watchers,
Konrad Dybcio, linux-arm-msm, linux-kernel, devicetree,
Ramakrishna Gottimukkula
On Thu, Oct 17, 2024 at 05:22:36PM +0530, Shivnandan Kumar wrote:
> On 10/6/2024 8:03 AM, Bjorn Andersson wrote:
> > On Tue, Sep 24, 2024 at 10:39:40AM GMT, Shivnandan Kumar wrote:
[..]
> > > static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
> > > {
> > > + const struct qcom_cpucp_mbox_desc *desc;
> > > struct device *dev = &pdev->dev;
> > > struct qcom_cpucp_mbox *cpucp;
> > > struct mbox_controller *mbox;
> > > + struct resource *res;
> > > int irq, ret;
> > >
> > > + desc = device_get_match_data(&pdev->dev);
> > > + if (!desc)
> > > + return -EINVAL;
> > > +
> > > cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
> > > if (!cpucp)
> > > return -ENOMEM;
> > >
> > > - cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> > > - if (IS_ERR(cpucp->rx_base))
> > > - return PTR_ERR(cpucp->rx_base);
> > > + cpucp->desc = desc;
> > > +
> > > + if (desc->v2_mbox) {
> > > + cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> > > + if (IS_ERR(cpucp->rx_base))
> > > + return PTR_ERR(cpucp->rx_base);
> > > + /* Legacy mailbox quirks due to shared region with EPSS register space */
> >
> > Why can't we have the same code in both cases?
> >
>
>
> RX address space share region with EPSS. Due to which devm_of_iomap returns
> -EBUSY.
>
I assumed that was the case, and that explains why the legacy system
needs a different code path.
But, couldn't you use the same for the v2 solution, so we avoid having
two different code paths?
Regards,
Bjorn
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller
2024-09-24 5:09 ` [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller Shivnandan Kumar
` (2 preceding siblings ...)
2024-10-06 2:33 ` Bjorn Andersson
@ 2024-10-17 21:15 ` Konrad Dybcio
3 siblings, 0 replies; 23+ messages in thread
From: Konrad Dybcio @ 2024-10-17 21:15 UTC (permalink / raw)
To: Shivnandan Kumar, Sibi Sankar, Jassi Brar, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, cros-qcom-dts-watchers,
Bjorn Andersson, Konrad Dybcio
Cc: linux-arm-msm, linux-kernel, devicetree, Ramakrishna Gottimukkula
On 24.09.2024 7:09 AM, Shivnandan Kumar wrote:
> The SC7280 CPUCP mailbox controller is compatible with legacy mailbox
> hardware.
> Implement support for this functionality which enable HLOS to
> CPUCP communication.
>
> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> ---
If you need to if-out everything that isn't Linux boilerplate, I don't
think this driver is the correct plate to describe the 7280 mailbox
Konrad
^ permalink raw reply [flat|nested] 23+ messages in thread