| Message ID | 20240301144403.2977-6-jason-jh.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add CMDQ API for upcoming ISP feature | expand |
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@mediatek.com> > --- > 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 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? > +{ > + struct cmdq *cmdq = container_of(((struct mbox_chan *)chan)- > >mbox, > + typeof(*cmdq), mbox); > + unsigned long flags; > + > + spin_lock_irqsave(&cmdq->event_lock, flags); > + > + writel(CMDQ_TOKEN_UPDATE_VALUE | event_id, cmdq->base + > CMDQ_SYNC_TOKEN_UPDATE); > + > + spin_unlock_irqrestore(&cmdq->event_lock, flags); > +} > +EXPORT_SYMBOL(cmdq_set_event); > + > +u32 cmdq_get_event(void *chan, u16 event_id) Does this get the event status? I think event has only two status, set or cleared. So I would like this to return true for set and false for cleared. Regards, CK > +{ > + struct cmdq *cmdq = container_of(((struct mbox_chan *)chan)- > >mbox, > + typeof(*cmdq), mbox); > + unsigned long flags; > + u32 value = 0; > + > + spin_lock_irqsave(&cmdq->event_lock, flags); > + > + writel(CMDQ_TOKEN_ID_MASK & event_id, cmdq->base + > CMDQ_SYNC_TOKEN_ID); > + value = readl(cmdq->base + CMDQ_SYNC_TOKEN_VALUE); > + > + spin_unlock_irqrestore(&cmdq->event_lock, flags); > + > + return value; > +} > +EXPORT_SYMBOL(cmdq_get_event); > + > static int cmdq_thread_suspend(struct cmdq *cmdq, struct cmdq_thread > *thread) > { > u32 status; > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h > b/include/linux/mailbox/mtk-cmdq-mailbox.h > index a8f0070c7aa9..f05cabd230da 100644 > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h > @@ -79,5 +79,7 @@ struct cmdq_pkt { > }; > > u8 cmdq_get_shift_pa(struct mbox_chan *chan); > +void cmdq_set_event(void *chan, u16 event_id); > +u32 cmdq_get_event(void *chan, u16 event_id); > > #endif /* __MTK_CMDQ_MAILBOX_H__ */
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@mediatek.com> > > --- > > 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. > > +{ > > + struct cmdq *cmdq = container_of(((struct mbox_chan *)chan)- > > > mbox, > > > > + typeof(*cmdq), mbox); > > + unsigned long flags; > > + > > + spin_lock_irqsave(&cmdq->event_lock, flags); > > + > > + writel(CMDQ_TOKEN_UPDATE_VALUE | event_id, cmdq->base + > > CMDQ_SYNC_TOKEN_UPDATE); > > + > > + spin_unlock_irqrestore(&cmdq->event_lock, flags); > > +} > > +EXPORT_SYMBOL(cmdq_set_event); > > + > > +u32 cmdq_get_event(void *chan, u16 event_id) > > Does this get the event status? I think event has only two status, > set > or cleared. So I would like this to return true for set and false for > cleared. Yes, the event status is 1 or 0. I'll change it to boolean. Regards, Jason-JH.Lin > > Regards, > CK >
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@mediatek.com> > > > --- > > > 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? Regards, CK > > > > +{ > > > + struct cmdq *cmdq = container_of(((struct mbox_chan *)chan)- > > > > mbox, > > > > > > + typeof(*cmdq), mbox); > > > + unsigned long flags; > > > + > > > + spin_lock_irqsave(&cmdq->event_lock, flags); > > > + > > > + writel(CMDQ_TOKEN_UPDATE_VALUE | event_id, cmdq->base + > > > CMDQ_SYNC_TOKEN_UPDATE); > > > + > > > + spin_unlock_irqrestore(&cmdq->event_lock, flags); > > > +} > > > +EXPORT_SYMBOL(cmdq_set_event); > > > + > > > +u32 cmdq_get_event(void *chan, u16 event_id) > > > > Does this get the event status? I think event has only two status, > > set > > or cleared. So I would like this to return true for set and false > > for > > cleared. > > Yes, the event status is 1 or 0. I'll change it to boolean. > > Regards, > Jason-JH.Lin > > > > > Regards, > > CK > >
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@mediatek.com> > > > > --- > > > > 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 >
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 cmdq *cmdq = container_of(((struct mbox_chan *)chan)->mbox, + typeof(*cmdq), mbox); + unsigned long flags; + + spin_lock_irqsave(&cmdq->event_lock, flags); + + writel(CMDQ_TOKEN_UPDATE_VALUE | event_id, cmdq->base + CMDQ_SYNC_TOKEN_UPDATE); + + spin_unlock_irqrestore(&cmdq->event_lock, flags); +} +EXPORT_SYMBOL(cmdq_set_event); + +u32 cmdq_get_event(void *chan, u16 event_id) +{ + struct cmdq *cmdq = container_of(((struct mbox_chan *)chan)->mbox, + typeof(*cmdq), mbox); + unsigned long flags; + u32 value = 0; + + spin_lock_irqsave(&cmdq->event_lock, flags); + + writel(CMDQ_TOKEN_ID_MASK & event_id, cmdq->base + CMDQ_SYNC_TOKEN_ID); + value = readl(cmdq->base + CMDQ_SYNC_TOKEN_VALUE); + + spin_unlock_irqrestore(&cmdq->event_lock, flags); + + return value; +} +EXPORT_SYMBOL(cmdq_get_event); + static int cmdq_thread_suspend(struct cmdq *cmdq, struct cmdq_thread *thread) { u32 status; diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h index a8f0070c7aa9..f05cabd230da 100644 --- a/include/linux/mailbox/mtk-cmdq-mailbox.h +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h @@ -79,5 +79,7 @@ struct cmdq_pkt { }; u8 cmdq_get_shift_pa(struct mbox_chan *chan); +void cmdq_set_event(void *chan, u16 event_id); +u32 cmdq_get_event(void *chan, u16 event_id); #endif /* __MTK_CMDQ_MAILBOX_H__ */
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@mediatek.com> --- drivers/mailbox/mtk-cmdq-mailbox.c | 37 ++++++++++++++++++++++++ include/linux/mailbox/mtk-cmdq-mailbox.h | 2 ++ 2 files changed, 39 insertions(+)