| Message ID | 20230213211825.3507034-2-tanmay.shah@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers: remoteproc: xilinx: add mailbox support | expand |
[AMD Official Use Only - General] Hi, > -----Original Message----- > From: Tanmay Shah <tanmay.shah@amd.com> > Sent: Tuesday, February 14, 2023 2:48 AM > To: Simek, Michal <michal.simek@amd.com>; andersson@kernel.org; > mathieu.poirier@linaro.org; jaswinder.singh@linaro.org; Levinsky, Ben > <ben.levinsky@amd.com>; Datta, Shubhrajyoti > <shubhrajyoti.datta@amd.com> > Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-remoteproc@vger.kernel.org; Shah, Tanmay > <tanmay.shah@amd.com> > Subject: [PATCH v3 1/3] drivers: mailbox: zynqmp: handle multiple child > nodes > > 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. It does multiple things it will be good if one patch does one stuff.
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@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. Split or not: Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org> > */ > struct zynqmp_ipi_message { > -- > 2.25.1 >
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@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. 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. Thanks, Michal
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@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 ? > > Thanks, > Michal >
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@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
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@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
On 2/27/23 16:25, Tanmay Shah wrote: > 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. thx. > > 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. yes that's fine. M
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. */ struct zynqmp_ipi_message {
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@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(-)