Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20230917-imx95-mbox-v1-0-440245287356@nxp.com>
 <20230917-imx95-mbox-v1-2-440245287356@nxp.com> <CAEnQRZBbdrwX3mq=1RVHwzPAUtWjrKmz3y4ezOann6yZUwZAYA@mail.gmail.com>
 <DU0PR04MB94170D1357C13A2EC66A05F488FBA@DU0PR04MB9417.eurprd04.prod.outlook.com>
In-Reply-To: <DU0PR04MB94170D1357C13A2EC66A05F488FBA@DU0PR04MB9417.eurprd04.prod.outlook.com>
From:   Daniel Baluta <daniel.baluta@gmail.com>
Date:   Mon, 18 Sep 2023 17:40:08 +0300
Message-ID: <CAEnQRZBdFupPMHD6fy65uEB2LhHxqoPz=1v4HaZhNCjGV2yvBA@mail.gmail.com>
Subject: Re: [PATCH 2/2] mailbox: imx: support channel type tx doorbell v2
To:     Peng Fan <peng.fan@nxp.com>
Cc:     "Peng Fan (OSS)" <peng.fan@oss.nxp.com>,
        Jassi Brar <jassisinghbrar@gmail.com>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        dl-linux-imx <linux-imx@nxp.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Aisheng Dong <aisheng.dong@nxp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Sep 18, 2023 at 3:01=E2=80=AFPM Peng Fan <peng.fan@nxp.com> wrote:
>
> Hi Daniel,
>
> > Subject: Re: [PATCH 2/2] mailbox: imx: support channel type tx doorbell=
 v2
> >
> > On Sun, Sep 17, 2023 at 5:45=E2=80=AFPM Peng Fan (OSS) <peng.fan@oss.nx=
p.com>
> > wrote:
> > >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > The Message Unit(MU) General Purpose Control registers are used for T=
X
> > > 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 th=
e
> > > GCR register, and no else. This will be used for SCMI mailbox.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  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        =3D 2, /* Tx doorbell */
> > >         IMX_MU_TYPE_RXDB        =3D 3, /* Rx doorbell */
> > >         IMX_MU_TYPE_RST         =3D 4, /* Reset */
> > > +       IMX_MU_TYPE_TXDB_V2     =3D 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 c=
hannel
> > 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 =3D=3D IMX_MU_TYPE_TXDB_V2)
> > > +               return 0;
> > > +
> > >         if (cp->type =3D=3D 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 =3D=3D IMX_MU_TYPE_TXDB_V2) {
> > > +               pm_runtime_put_sync(priv->dev);
> > > +               return;
> > > +       }
> > > +
> > >         if (cp->type =3D=3D 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 !=3D 2) {
> > > @@ -680,14 +695,25 @@ static struct mbox_chan * imx_mu_xlate(struct
> > > mbox_controller *mbox,
> > >
> > >         type =3D sp->args[0]; /* channel type */
> > >         idx =3D sp->args[1]; /* index */
> > > -       chan =3D type * 4 + idx;
> > >
> > > +       /* RST only supports 1 channel */
> > > +       if ((type =3D=3D 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 doorbel=
l
> > channel. Can you please add it in a separate patch with proper explanat=
ion?
>
> RST only supports 1 channel, but its value is 4, so here I extend to 4 ch=
annel
> for RST in code, to make the calculation a bit easier to get TXDB_V2 chan=
nel
> pointer. But from user side, nothing changed
>
> IMX_MU_TYPE_RST         =3D 4, /* Reset */
> IMX_MU_TYPE_TXDB_V2     =3D 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.


Ok, I see. Good for me, then.