Re: [PATCH v9 5/9] mailbox: add CIX mailbox driver

[Guomin chen <guomin.chen@cixtech.com>]

On Sun, Jul 13, 2025 at 12:00:06PM -0500, Jassi Brar wrote:
> [Some people who received this message don't often get email from jassisinghbrar@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL
> 
> On Tue, Jul 8, 2025 at 8:54 AM Guomin chen <guomin.chen@cixtech.com> wrote:
> ....
> > > > +/* [0~7] Fast channel
> > > > + * [8] doorbell base channel
> > > > + * [9]fifo base channel
> > > > + * [10] register base channel
> > > > + */
> > > > +#define MBOX_FAST_IDX          7
> > > > +#define MBOX_DB_IDX            8
> > > > +#define MBOX_FIFO_IDX          9
> > > > +#define MBOX_REG_IDX           10
> > > > +#define CIX_MBOX_CHANS         11
> > > > +
> > > if it is not really a single controller owning different channels,
> > > maybe implement only what you currently use.
> > >
> > As mentioned in the previous email, a single controller can support
> > multiple different channels.
> >
> OK. I am not too worried about having all variants in one driver esp
> when it is manageable and share the code.
> Unless I am overlooking something. Arnd?
> 
> 
> > > > +static u32 cix_mbox_read(struct cix_mbox_priv *priv, u32 offset)
> > > > +{
> > > > +       if (priv->use_shmem)
> > > > +               return ioread32(priv->base + offset - SHMEM_OFFSET);
> > > > +       else
> > > > +               return ioread32(priv->base + offset);
> > > > +}
> > > > +
> > > use_shmem is set for only CIX_MBOX_TYPE_DB, but it affects every read/write.
> > > Maybe instead adjust the base for TYPE_DB?
> > >
> > The reason we introduced use_shmem here is that we had to adjust the base
> > address of TYPE_DB to resolve the reg conflict in the DTS.
> > This change has virtually no impact on performance.
> >
> Yes, I can see it should have no impact on performance and I think
> adjusting the base once
> during init is cleaner than checking the flag every read/write.
> But wait... use_shmem is a controller wide flag, and isn't
> priv->use_shmem always set to true  in cix_mbox_init() ?
> Is the driver even tested?
> ....
Yes, we did perform testing before sending out the patch. 
The test here shows no issues because there aren’t more clients. 
There indeed exists a problem with priv->use_shmem always being set to 
true in cix_mbox_init(). 
So I will add a restriction in the probe function in the next version.

> > > > +static int cix_mbox_startup(struct mbox_chan *chan)
> > > > +{
> > > > +       struct cix_mbox_priv *priv = to_cix_mbox_priv(chan->mbox);
> > > > +       struct cix_mbox_con_priv *cp = chan->con_priv;
> > > > +       int index = cp->index, ret;
> > > > +       u32 val_32;
> > > > +
> > > > +       ret = request_irq(priv->irq, cix_mbox_isr, 0,
> > > > +                         dev_name(priv->dev), chan);
> > > The same irq is requested for each channel. How do you expect it to
> > > work? Maybe request it just once in probe and pass the 'priv' instead
> > > of 'chan' , and in the cix_mbox_isr handle according to INT_STATUS
> > >
> > For the same mailbox controller, there won't be multiple channels
> > simultaneously requesting the same IRQ, so there won't be an issue
> > here. As you mentioned, if we need to handle multiple channels working
> > concurrently in the future, we would need to modify cix_mbox_isr.
> > However, that is not required at the moment.
> >
> Is it too hard to do it right already?
>
We haven't set the IRQF_SHARED flag here because there is no scenario 
where a single mailbox controller supports multiple channel clients simultaneously.
And, the ISR still requires the channel as an argument. 
While this approach does not support multiple clients in parallel, it does allow 
for better sequential support of multiple clients.

So we have placed it in the client's startup/shutdown lifecycle rather than in the
probe function.

Thanks
Guomin Chen