diff mbox series

[v2] firmware: arm_scmi: Avoid to call mbox_client_txdone on txdone_irq mode

Message ID 20240201095754.25374-1-flash.liu@mediatek.com
State New
Headers
Series [v2] firmware: arm_scmi: Avoid to call mbox_client_txdone on txdone_irq mode |

Commit Message

Flash Liu (劉炳傳) Feb. 1, 2024, 9:57 a.m. UTC 
  On txdone_irq mode, tx_tick is done from mbox_chan_txdone.
Calling to mbox_client_txdone could get error message
and return directly, add a check to avoid this.

Signed-off-by: Pin-Chuan Liu <flash.liu@mediatek.com>
---
V1 -> V2:
 - Remove Change-Id
 - Use single line comment
 
 drivers/firmware/arm_scmi/mailbox.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Cristian Marussi Feb. 2, 2024, 10:56 a.m. UTC | #1 
On Thu, Feb 01, 2024 at 05:57:52PM +0800, Pin-Chuan Liu wrote:
> On txdone_irq mode, tx_tick is done from mbox_chan_txdone.
> Calling to mbox_client_txdone could get error message
> and return directly, add a check to avoid this.
> 
> Signed-off-by: Pin-Chuan Liu <flash.liu@mediatek.com>

Hi Pin-Chuan,

thanks for this, it was indeed sort of on my todo-list too, to allow MHUs
equipped with Tx-Ack IRQ to work with SCMI.

Having said that, this looks good to me in general, my only pain points
are: the fact that we have to peek into the controller structures to know
how it is configured, BUT I wouldn't know how to do it in any other
way in fact as pof now...; and the fact that there is a constant runtime
conditional check for each message sent even though the tx_ack irq presence
can be detected once for all at setup time, BUT even this is not easily
solvable as of now in the SCMI stack.

So, after all of this babbling of mine, I would say that  your patch is fine
as it is, also because it is easy to backport; next week when I am back I'll
give it a go on a couple of platforms and get back to you with a proper
review/test.

Thanks again !
Cristian
Flash Liu (劉炳傳) Feb. 2, 2024, 12:36 p.m. UTC | #2 
Hi Cristian, 

Thanks for kindly review and explains. Yeah, I have ever tried another
way to skip the call (i.e. let mark_txdone be null). However, it looks
not easy and also to backport.

Awaiting your test results and further suggestions, thanks.

Regards,
Pin-Chuan

On Fri, 2024-02-02 at 10:56 +0000, Cristian Marussi wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Thu, Feb 01, 2024 at 05:57:52PM +0800, Pin-Chuan Liu wrote:
> > On txdone_irq mode, tx_tick is done from mbox_chan_txdone.
> > Calling to mbox_client_txdone could get error message
> > and return directly, add a check to avoid this.
> > 
> > Signed-off-by: Pin-Chuan Liu <flash.liu@mediatek.com>
> 
> Hi Pin-Chuan,
> 
> thanks for this, it was indeed sort of on my todo-list too, to allow
> MHUs
> equipped with Tx-Ack IRQ to work with SCMI.
> 
> Having said that, this looks good to me in general, my only pain
> points
> are: the fact that we have to peek into the controller structures to
> know
> how it is configured, BUT I wouldn't know how to do it in any other
> way in fact as pof now...; and the fact that there is a constant
> runtime
> conditional check for each message sent even though the tx_ack irq
> presence
> can be detected once for all at setup time, BUT even this is not
> easily
> solvable as of now in the SCMI stack.
> 
> So, after all of this babbling of mine, I would say that  your patch
> is fine
> as it is, also because it is easy to backport; next week when I am
> back I'll
> give it a go on a couple of platforms and get back to you with a
> proper
> review/test.
> 
> Thanks again !
> Cristian
diff mbox series

Patch

diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
index b8d470417e8f..d391463e3208 100644
--- a/drivers/firmware/arm_scmi/mailbox.c
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -8,6 +8,7 @@ 
 
 #include <linux/err.h>
 #include <linux/device.h>
+#include <linux/mailbox_controller.h>
 #include <linux/mailbox_client.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -275,7 +276,10 @@  static void mailbox_mark_txdone(struct scmi_chan_info *cinfo, int ret,
 	 * Unfortunately, we have to kick the mailbox framework after we have
 	 * received our message.
 	 */
-	mbox_client_txdone(smbox->chan, ret);
+
+	/* With txdone_irq mode, kick can be done by mbox_chan_txdone */
+	if (!(smbox->chan->mbox->txdone_irq))
+		mbox_client_txdone(smbox->chan, ret);
 }
 
 static void mailbox_fetch_response(struct scmi_chan_info *cinfo,