Re: [RESEND, PATCH 5/5] mailbox: mtk-cmdq: Add support runtime get and set GCE event

[Jason-JH Lin (林睿祥)]

On Tue, 2024-03-05 at 03:31 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Mon, 2024-03-04 at 15:50 +0000, Jason-JH Lin (林睿祥) wrote:
> > Hi CK,
> > 
> > Thanks for the reviews.
> > 
> > On Mon, 2024-03-04 at 02:30 +0000, CK Hu (胡俊光) wrote:
> > > Hi, Jason:
> > > 
> > > On Fri, 2024-03-01 at 22:44 +0800, Jason-JH.Lin wrote:
> > > > ISP drivers need to get and set GCE event in their runtime
> > > > contorl
> > > > flow.
> > > > So add these functions to support get and set GCE by CPU.
> > > > 
> > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>
> > > > ---
> > > >  drivers/mailbox/mtk-cmdq-mailbox.c       | 37
> > > > ++++++++++++++++++++++++
> > > >  include/linux/mailbox/mtk-cmdq-mailbox.h |  2 ++
> > > >  2 files changed, 39 insertions(+)
> > > > 
> > > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > index ead2200f39ba..d7c08249c898 100644
> > > > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > > > @@ -25,7 +25,11 @@
> > > >  #define CMDQ_GCE_NUM_MAX		(2)
> > > >  
> > > >  #define CMDQ_CURR_IRQ_STATUS		0x10
> > > > +#define CMDQ_SYNC_TOKEN_ID		0x60
> > > > +#define CMDQ_SYNC_TOKEN_VALUE		0x64
> > > > +#define CMDQ_TOKEN_ID_MASK			GENMASK(9, 0)
> > > >  #define CMDQ_SYNC_TOKEN_UPDATE		0x68
> > > > +#define CMDQ_TOKEN_UPDATE_VALUE			BIT(16)
> > > >  #define CMDQ_THR_SLOT_CYCLES		0x30
> > > >  #define CMDQ_THR_BASE			0x100
> > > >  #define CMDQ_THR_SIZE			0x80
> > > > @@ -83,6 +87,7 @@ struct cmdq {
> > > >  	struct cmdq_thread	*thread;
> > > >  	struct clk_bulk_data	clocks[CMDQ_GCE_NUM_MAX];
> > > >  	bool			suspended;
> > > > +	spinlock_t		event_lock; /* lock for gce
> > > > event */
> > > >  };
> > > >  
> > > >  struct gce_plat {
> > > > @@ -113,6 +118,38 @@ u8 cmdq_get_shift_pa(struct mbox_chan
> > > > *chan)
> > > >  }
> > > >  EXPORT_SYMBOL(cmdq_get_shift_pa);
> > > >  
> > > > +void cmdq_set_event(void *chan, u16 event_id)
> > > 
> > > struct mbox_chan *chan
> > > 
> > 
> > OK, I'll change it.
> > 
> > > Is the event_id the hardware event id listed in include/dt-
> > > bindings/gce 
> > > ? I mean CPU could trigger the event which should be trigger by
> > > hardware?
> > > 
> > 
> > Yes, this can also trigger the hardware event, but CMDQ user should
> > not
> > do that. Otherwise, it will cause error in other GCE threads that
> > use
> > this hardware event.
> 
> So, what event id could client driver use? And how to prevent
> different
> client driver use the same event id?
> 
Yes, this might be a problem.

Client user should use the SW token events defined in dt-binding and
parsing them from dts.
Maybe different user should see if the SW token events has used by
other user.

Anyway, after confirming with ISP owners, they dropped the usage of
these 2 APIs. So I'll drop this patch in the next version.

Regards,
Jason-JH.Lin

> Regards,
> CK
>