Message-ID: <e2ef52ba-7633-3958-4b40-e047a7bba820@amd.com>
Date:   Fri, 24 Feb 2023 09:37:16 +0100
From:   Michal Simek <michal.simek@....com>
To:     Tanmay Shah <tanmays@....com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Tanmay Shah <tanmay.shah@....com>
CC:     <andersson@...nel.org>, <jaswinder.singh@...aro.org>,
        <ben.levinsky@....com>, <shubhrajyoti.datta@....com>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-remoteproc@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] drivers: mailbox: zynqmp: handle multiple child
 nodes



On 2/23/23 15:47, Tanmay Shah wrote:
> 
> On 2/23/23 1:40 AM, Michal Simek wrote:
>>
>>
>> On 2/22/23 18:34, Mathieu Poirier wrote:
>>> On Mon, Feb 13, 2023 at 01:18:24PM -0800, Tanmay Shah wrote:
>>>> As of now only one child node is handled by zynqmp-ipi
>>>> mailbox driver. Upon introducing remoteproc r5 core mailbox
>>>> nodes, found few enhancements in Xilinx zynqmp mailbox driver
>>>> as following:
>>>>
>>>> - fix mailbox child node counts
>>>>    If child mailbox node status is disabled it causes
>>>>    crash in interrupt handler. Fix this by assigning
>>>>    only available child node during driver probe.
>>>>
>>>> - fix typo in IPI documentation %s/12/32/
>>>>    Xilinx IPI message buffers allows 32-byte data transfer.
>>>>    Fix documentation that says 12 bytes
>>>>
>>>> - fix bug in zynqmp-ipi isr handling
>>>>    Multiple IPI channels are mapped to same interrupt handler.
>>>>    Current isr implementation handles only one channel per isr.
>>>>    Fix this behavior by checking isr status bit of all child
>>>>    mailbox nodes.
>>>>
>>>> Fixes: 4981b82ba2ff ("mailbox: ZynqMP IPI mailbox controller")
>>>> Signed-off-by: Tanmay Shah <tanmay.shah@....com>
>>>> ---
>>>>
>>>> Changelog:
>>>>    - This is first version of this change, however posting as part of the 
>>>> series
>>>>      that has version v3.
>>>>
>>>> v2: https://lore.kernel.org/all/20230126213154.1707300-1-tanmay.shah@amd.com/
>>>>
>>>>   drivers/mailbox/zynqmp-ipi-mailbox.c       | 8 ++++----
>>>>   include/linux/mailbox/zynqmp-ipi-message.h | 2 +-
>>>>   2 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/mailbox/zynqmp-ipi-mailbox.c 
>>>> b/drivers/mailbox/zynqmp-ipi-mailbox.c
>>>> index 12e004ff1a14..b1498f6f06e1 100644
>>>> --- a/drivers/mailbox/zynqmp-ipi-mailbox.c
>>>> +++ b/drivers/mailbox/zynqmp-ipi-mailbox.c
>>>> @@ -152,7 +152,7 @@ static irqreturn_t zynqmp_ipi_interrupt(int irq, void 
>>>> *data)
>>>>       struct zynqmp_ipi_message *msg;
>>>>       u64 arg0, arg3;
>>>>       struct arm_smccc_res res;
>>>> -    int ret, i;
>>>> +    int ret, i, status = IRQ_NONE;
>>>>         (void)irq;
>>>>       arg0 = SMC_IPI_MAILBOX_STATUS_ENQUIRY;
>>>> @@ -170,11 +170,11 @@ static irqreturn_t zynqmp_ipi_interrupt(int irq, void 
>>>> *data)
>>>>                   memcpy_fromio(msg->data, mchan->req_buf,
>>>>                             msg->len);
>>>>                   mbox_chan_received_data(chan, (void *)msg);
>>>> -                return IRQ_HANDLED;
>>>> +                status = IRQ_HANDLED;
>>>>               }
>>>>           }
>>>>       }
>>>> -    return IRQ_NONE;
>>>> +    return status;
>>>>   }
>>>>     /**
>>>> @@ -634,7 +634,7 @@ static int zynqmp_ipi_probe(struct platform_device *pdev)
>>>>       struct zynqmp_ipi_mbox *mbox;
>>>>       int num_mboxes, ret = -EINVAL;
>>>>   -    num_mboxes = of_get_child_count(np);
>>>> +    num_mboxes = of_get_available_child_count(np);
>>>>       pdata = devm_kzalloc(dev, sizeof(*pdata) + (num_mboxes * sizeof(*mbox)),
>>>>                    GFP_KERNEL);
>>>>       if (!pdata)
>>>> diff --git a/include/linux/mailbox/zynqmp-ipi-message.h 
>>>> b/include/linux/mailbox/zynqmp-ipi-message.h
>>>> index 35ce84c8ca02..31d8046d945e 100644
>>>> --- a/include/linux/mailbox/zynqmp-ipi-message.h
>>>> +++ b/include/linux/mailbox/zynqmp-ipi-message.h
>>>> @@ -9,7 +9,7 @@
>>>>    * @data: message payload
>>>>    *
>>>>    * This is the structure for data used in mbox_send_message
>>>> - * the maximum length of data buffer is fixed to 12 bytes.
>>>> + * the maximum length of data buffer is fixed to 32 bytes.
>>>>    * Client is supposed to be aware of this.
>>>
>>> I agree that this should be split in 3 patches but the fixes are so small that
>>> it is hardly required.  I'll leave it up to Michal to decide.
>>
>> Generic guidance is saying that you should split that patches. I personally 
>> prefer to have one patch per change. It is useful for bisecting and faster for 
>> reviewing.
>> I would expect that this patch should go via mailbox tree and the rest via 
>> remoteproc tree. That's why maintainer should say what it is preferred way.
>>
> 
> Thanks Michal for reviews. I will split the patch in three different patches.
> 
> 
>> In connection mailbox. I recently had some time to look at this driver and I 
>> didn't really get why there are registers listed. Because all that addresses 
>> can be calculated based on soc compatible string and by xlnx,ipi-id for both 
>> sides.
> 
> 
> Yes the IPI configuration register addresses are retrieved from TF-A in 
> zynqmp-ipi-driver using xlnx,ipi-id property.
> 
> Other than that there are message buffers provided in hardware for IPI 
> communication. We list those message buffer addresses
> 
> using reg addresses and they are expected in dts. As per bindings we do not map 
> message buffers to IPI ID.
> 
> I am not sure which register listing you are referring to ?

Based on
https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/Message-Buffer

xlnx,ipi-id = 2 (versal case) APU
pmu1 has xlnx,ipi-id = 1; PMC

Base on versal is 0xFF3F0000

Local buffers for sending from 2 -> 1
Buffer 2 starts at offset 0x400

Order in destination is PSM, PMC, IPI0... where you have request 32B and 
response 32B too.

It means 2->1 - target is PMC
that means 0x40 for request 0x60 for response.

When this is put together

0xff3f0000 + 0x400 + 0x40 = ff3f0440 - local request
0xff3f0000 + 0x400 + 0x60 = ff3f0460 - local response

For the way back from 1->2
Buffer one starts at 0x200
I want to send it to APU which we use channel 2 for.
Channel 2 start at ID * 0x40 = 0x80 is for request
0x80 + 32 = 0xa0 for response

It means 2->1 - target is APU at ID 2
0xff3f0000 + 0x200 + 0x80 = ff3f0280 - remote request
0xff3f0000 + 0x200 + 0xa0 = ff3f02a0 - remote response

Based on this you see that reg/reg names property are pretty much useless and 
should be removed from dt binding document because simply base and source ipi-id 
and destination ipi-id will tell you which addresses should be used.

As far as I know ZynqMP is using the same logic. The only difference is really 
just different base address for buffers.

That's why I think the DT node should be just like this and all addresses

Versal
         versal_ipi {
                 compatible = "xlnx,versal-ipi-mailbox";
                 interrupt-parent = <&gic>;
                 interrupts = <0 30 4>;
                 xlnx,ipi-id = <2>;

                 ipi_mailbox_pmu1: mailbox {
                         #mbox-cells = <1>;
                         xlnx,ipi-id = <1>;
                 };
         };

Where different compatible string will ensure that base address is assigned 
based on SOC.

Thanks,
Michal

Powered by blists - more mailing lists