| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c89715cc-2c45-d921-9837-b0a59469d233@amd.com>
Date: Mon, 27 Feb 2023 09:25:10 -0600
From: Tanmay Shah <tanmays@....com>
To: Michal Simek <michal.simek@....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
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@....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