2023-09-17 13:47:41

by Peng Fan (OSS)

[permalink] [raw]
Subject: [PATCH 2/2] mailbox: imx: support channel type tx doorbell v2

From: Peng Fan <[email protected]>

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 <[email protected]>
---
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);
+ }
+
+ 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


2023-09-18 14:07:54

by Peng Fan

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

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) <[email protected]>
> wrote:
> >
> > From: Peng Fan <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
> >

2023-09-18 19:05:25

by Daniel Baluta

[permalink] [raw]
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) <[email protected]> wrote:
>
> From: Peng Fan <[email protected]>
>
> 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 <[email protected]>
> ---
> 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?


> +
> + 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
>

2023-09-19 03:00:40

by Daniel Baluta

[permalink] [raw]
Subject: Re: [PATCH 2/2] mailbox: imx: support channel type tx doorbell v2

On Mon, Sep 18, 2023 at 3:01 PM Peng Fan <[email protected]> wrote:
>
> 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) <[email protected]>
> > wrote:
> > >
> > > From: Peng Fan <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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.


Ok, I see. Good for me, then.