[PATCH v3 1/3] drivers: mailbox: zynqmp: handle multiple child nodes

[Tanmay Shah]

Thanks Michal for this calculation.

I will send separate patch after some more analysis to accommodate this 
implementation of accessing message buffers from ipi-id.

However, for this series this isn't required. For this series, I will 
just split this patch into three different patches. I hope it's okay.

Thanks,

Tanmay

On 2/24/23 2:37 AM, Michal Simek wrote:
>
>
> 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 at amd.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