RE: [PATCH 2/2] mailbox: imx: support channel type tx doorbell v2

[Peng Fan]

Hi Daniel,

> Subject: Re: [PATCH 2/2] mailbox: imx: support channel type tx doorbell v2
> 
> On Sun, Sep 17, 2023 at 5:45 PM Peng Fan (OSS) <peng.fan@xxxxxxxxxxx>
> wrote:
> >
> > From: Peng Fan <peng.fan@xxxxxxx>
> >
> > The Message Unit(MU) General Purpose Control registers are used for TX
> > doorbell, but there is no hardware ACK support.
> >
> > The current TX doorbell channel is using tasklet to emulate hardware
> > ACK support to kick the TX tick from controller driver side.
> >
> > The new added TX doorbell channel V2 not using tasklet to emulate the
> > hardware ACK support. The behavior for the channel is just writing the
> > GCR register, and no else. This will be used for SCMI mailbox.
> >
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > ---
> >  drivers/mailbox/imx-mailbox.c | 32 +++++++++++++++++++++++++++++---
> >  1 file changed, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mailbox/imx-mailbox.c
> > b/drivers/mailbox/imx-mailbox.c index 3ef4dd8adf5d..0af739ab571c
> > 100644
> > --- a/drivers/mailbox/imx-mailbox.c
> > +++ b/drivers/mailbox/imx-mailbox.c
> > @@ -20,7 +20,9 @@
> >  #include <linux/suspend.h>
> >  #include <linux/slab.h>
> >
> > -#define IMX_MU_CHANS           17
> > +#include "mailbox.h"
> > +
> > +#define IMX_MU_CHANS           24
> >  /* TX0/RX0/RXDB[0-3] */
> >  #define IMX_MU_SCU_CHANS       6
> >  /* TX0/RX0 */
> > @@ -39,6 +41,7 @@ enum imx_mu_chan_type {
> >         IMX_MU_TYPE_TXDB        = 2, /* Tx doorbell */
> >         IMX_MU_TYPE_RXDB        = 3, /* Rx doorbell */
> >         IMX_MU_TYPE_RST         = 4, /* Reset */
> > +       IMX_MU_TYPE_TXDB_V2     = 5, /* Tx doorbell with S/W ACK */
> >  };
> >
> >  enum imx_mu_xcr {
> > @@ -226,6 +229,9 @@ static int imx_mu_generic_tx(struct imx_mu_priv
> *priv,
> >                 imx_mu_xcr_rmw(priv, IMX_MU_GCR, IMX_MU_xCR_GIRn(priv-
> >dcfg->type, cp->idx), 0);
> >                 tasklet_schedule(&cp->txdb_tasklet);
> >                 break;
> > +       case IMX_MU_TYPE_TXDB_V2:
> > +               imx_mu_xcr_rmw(priv, IMX_MU_GCR, IMX_MU_xCR_GIRn(priv-
> >dcfg->type, cp->idx), 0);
> > +               break;
> >         default:
> >                 dev_warn_ratelimited(priv->dev, "Send data on wrong channel
> type: %d\n", cp->type);
> >                 return -EINVAL;
> > @@ -554,6 +560,9 @@ static int imx_mu_startup(struct mbox_chan *chan)
> >         int ret;
> >
> >         pm_runtime_get_sync(priv->dev);
> > +       if (cp->type == IMX_MU_TYPE_TXDB_V2)
> > +               return 0;
> > +
> >         if (cp->type == IMX_MU_TYPE_TXDB) {
> >                 /* Tx doorbell don't have ACK support */
> >                 tasklet_init(&cp->txdb_tasklet, imx_mu_txdb_tasklet,
> > @@ -595,6 +604,11 @@ static void imx_mu_shutdown(struct mbox_chan
> *chan)
> >         int ret;
> >         u32 sr;
> >
> > +       if (cp->type == IMX_MU_TYPE_TXDB_V2) {
> > +               pm_runtime_put_sync(priv->dev);
> > +               return;
> > +       }
> > +
> >         if (cp->type == IMX_MU_TYPE_TXDB) {
> >                 tasklet_kill(&cp->txdb_tasklet);
> >                 pm_runtime_put_sync(priv->dev); @@ -671,6 +685,7 @@
> > static struct mbox_chan *imx_mu_specific_xlate(struct mbox_controller
> > *mbox,  static struct mbox_chan * imx_mu_xlate(struct mbox_controller
> *mbox,
> >                                        const struct of_phandle_args
> > *sp)  {
> > +       struct mbox_chan *p_chan;
> >         u32 type, idx, chan;
> >
> >         if (sp->args_count != 2) {
> > @@ -680,14 +695,25 @@ static struct mbox_chan * imx_mu_xlate(struct
> > mbox_controller *mbox,
> >
> >         type = sp->args[0]; /* channel type */
> >         idx = sp->args[1]; /* index */
> > -       chan = type * 4 + idx;
> >
> > +       /* RST only supports 1 channel */
> > +       if ((type == IMX_MU_TYPE_RST) && idx) {
> > +               dev_err(mbox->dev, "Invalid RST channel %d\n", idx);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> 
> I don't understand how is this related to introduction of a new doorbell
> channel. Can you please add it in a separate patch with proper explanation?

RST only supports 1 channel, but its value is 4, so here I extend to 4 channel
for RST in code, to make the calculation a bit easier to get TXDB_V2 channel
pointer. But from user side, nothing changed

IMX_MU_TYPE_RST         = 4, /* Reset */
IMX_MU_TYPE_TXDB_V2     = 5, /* Tx doorbell with S/W ACK */

It does not make much sense to separate this change, because this change
Would only make sense together with TXDB_V2 changes.

Thanks,
Peng.

> 
> 
> > +
> > +       chan = type * 4 + idx;
> >         if (chan >= mbox->num_chans) {
> >                 dev_err(mbox->dev, "Not supported channel number: %d.
> (type: %d, idx: %d)\n", chan, type, idx);
> >                 return ERR_PTR(-EINVAL);
> >         }
> >
> > -       return &mbox->chans[chan];
> > +       p_chan = &mbox->chans[chan];
> > +
> > +       if (type == IMX_MU_TYPE_TXDB_V2)
> > +               p_chan->txdone_method = TXDONE_BY_ACK;
> > +
> > +       return p_chan;
> >  }
> >
> >  static struct mbox_chan *imx_mu_seco_xlate(struct mbox_controller
> > *mbox,
> >
> > --
> > 2.37.1
> >