Re: [RESEND, PATCH v13 09/12] soc: mediatek: cmdq: define the instruction struct
On 27/08/2019 06:12, Bibby Hsieh wrote: >>> >>> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) >>> { >>> - u32 arg_b; >>> + struct cmdq_instruction *inst; >>> >>> if (event >= CMDQ_MAX_EVENT) >>> return -EINVAL; >>> >>> - /* >>> -* WFE arg_b >>> -* bit 0-11: wait value >>> -* bit 15: 1 - wait, 0 - no wait >>> -* bit 16-27: update value >>> -* bit 31: 1 - update, 0 - no update >>> -*/ >> >> I have no strong opinion of CMDQ_WFE_OPTION but if you want to introduce it, >> then please copy the comment over to include/linux/mailbox/mtk-cmdq-mailbox.h > > Ok. let's move the descriptions to header. >> >> Just one question, why did you call it _OPTION? It's not really expressive >> for me. > > Actually, _OPTION is come from our hardware design name... > Ok, then I'll stop bike-shedding. I leave it up to you to rename it or not. Regards, Matthias
Re: [RESEND, PATCH v13 09/12] soc: mediatek: cmdq: define the instruction struct
On Fri, 2019-08-23 at 15:50 +0200, Matthias Brugger wrote: > > On 20/08/2019 10:49, Bibby Hsieh wrote: > > Define an instruction structure for gce driver to append command. > > This structure can make the client's code more readability. > > > > Signed-off-by: Bibby Hsieh > > Reviewed-by: CK Hu > > --- > > drivers/soc/mediatek/mtk-cmdq-helper.c | 106 +++ > > include/linux/mailbox/mtk-cmdq-mailbox.h | 2 + > > 2 files changed, 74 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > > b/drivers/soc/mediatek/mtk-cmdq-helper.c > > index 7aa0517ff2f3..e3d5b0be8e79 100644 > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > > @@ -9,12 +9,24 @@ > > #include > > #include > > > > -#define CMDQ_ARG_A_WRITE_MASK 0x > > #define CMDQ_WRITE_ENABLE_MASK BIT(0) > > #define CMDQ_EOC_IRQ_ENBIT(0) > > #define CMDQ_EOC_CMD ((u64)((CMDQ_CODE_EOC << > > CMDQ_OP_CODE_SHIFT)) \ > > << 32 | CMDQ_EOC_IRQ_EN) > > > > +struct cmdq_instruction { > > + union { > > + u32 value; > > + u32 mask; > > + }; > > + union { > > + u16 offset; > > + u16 event; > > + }; > > + u8 subsys; > > + u8 op; > > +}; > > + > > static void cmdq_client_timeout(struct timer_list *t) > > { > > struct cmdq_client *client = from_timer(client, t, timer); > > @@ -110,10 +122,8 @@ void cmdq_pkt_destroy(struct cmdq_pkt *pkt) > > } > > EXPORT_SYMBOL(cmdq_pkt_destroy); > > > > -static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code > > code, > > - u32 arg_a, u32 arg_b) > > +static struct cmdq_instruction *cmdq_pkt_append_command(struct cmdq_pkt > > *pkt) > > { > > - u64 *cmd_ptr; > > > > if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) { > > /* > > @@ -127,81 +137,109 @@ static int cmdq_pkt_append_command(struct cmdq_pkt > > *pkt, enum cmdq_code code, > > pkt->cmd_buf_size += CMDQ_INST_SIZE; > > WARN_ONCE(1, "%s: buffer size %u is too small !\n", > > __func__, (u32)pkt->buf_size); > > - return -ENOMEM; > > + return NULL; > > } > > - cmd_ptr = pkt->va_base + pkt->cmd_buf_size; > > - (*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b; > > + > > + *(u64 *)(pkt->va_base + pkt->cmd_buf_size) = 0;> > > pkt->cmd_buf_size += CMDQ_INST_SIZE; > > > > - return 0; > > + return pkt->va_base + pkt->cmd_buf_size - CMDQ_INST_SIZE; > > } > > > > int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value) > > { > > - u32 arg_a = (offset & CMDQ_ARG_A_WRITE_MASK) | > > - (subsys << CMDQ_SUBSYS_SHIFT); > > + struct cmdq_instruction *inst; > > + > > + inst = cmdq_pkt_append_command(pkt); > > + if (!inst) > > + return -ENOMEM; > > + > > + inst->op = CMDQ_CODE_WRITE; > > + inst->value = value; > > + inst->offset = offset; > > + inst->subsys = subsys; > > > > I can see that using cmdq_instruction will make the code more readable, but I > dislike the approach that cmdq_pkt_append_command returns a pointer where we > write the instruction to. Better we pass inst to cmdq_pkt_append_command() and > write it there to cmd_ptr. > > I think this way we can get rid of explicitly setting the memory to zero: > *(u64 *)(pkt->va_base + pkt->cmd_buf_size) = 0; > > And if we pass the inst to the append_command we don't have to change the > return > value handling of cmdq_pkt_append_command(), which makes the patch easier to > understand. Ok, I will change and resend it. > > > - return cmdq_pkt_append_command(pkt, CMDQ_CODE_WRITE, arg_a, value); > > + return 0; > > } > > EXPORT_SYMBOL(cmdq_pkt_write); > > > > int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys, > > u16 offset, u32 value, u32 mask) > > { > > - u32 offset_mask = offset; > > - int err = 0; > > + struct cmdq_instruction *inst; > > + u16 offset_mask = offset; > > > > if (mask != 0x) { > > - err = cmdq_pkt_append_command(pkt, CMDQ_CODE_MASK, 0, ~mask); > > + inst = cmdq_pkt_append_command(pkt); > > + if (!inst) > > + return -ENOMEM; > > + > > + inst->op = CMDQ_CODE_MASK; > > + inst->mask = ~mask; > > offset_mask |= CMDQ_WRITE_ENABLE_MASK; > > } > > - err |= cmdq_pkt_write(pkt, value, subsys, offset_mask); > > > > - return err; > > + return cmdq_pkt_write(pkt, subsys, offset_mask, value); > > } > > EXPORT_SYMBOL(cmdq_pkt_write_mask); > > > > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > > { > > - u32 arg_b; > > + struct cmdq_instruction *inst; > > > > if (event >= CMDQ_MAX_EVENT) > > return -EINVAL; > > > > - /* > > -* WFE arg_b > > -* bit 0-11:
Re: [RESEND, PATCH v13 09/12] soc: mediatek: cmdq: define the instruction struct
On 20/08/2019 10:49, Bibby Hsieh wrote: > Define an instruction structure for gce driver to append command. > This structure can make the client's code more readability. > > Signed-off-by: Bibby Hsieh > Reviewed-by: CK Hu > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 106 +++ > include/linux/mailbox/mtk-cmdq-mailbox.h | 2 + > 2 files changed, 74 insertions(+), 34 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > b/drivers/soc/mediatek/mtk-cmdq-helper.c > index 7aa0517ff2f3..e3d5b0be8e79 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -9,12 +9,24 @@ > #include > #include > > -#define CMDQ_ARG_A_WRITE_MASK0x > #define CMDQ_WRITE_ENABLE_MASK BIT(0) > #define CMDQ_EOC_IRQ_EN BIT(0) > #define CMDQ_EOC_CMD ((u64)((CMDQ_CODE_EOC << CMDQ_OP_CODE_SHIFT)) \ > << 32 | CMDQ_EOC_IRQ_EN) > > +struct cmdq_instruction { > + union { > + u32 value; > + u32 mask; > + }; > + union { > + u16 offset; > + u16 event; > + }; > + u8 subsys; > + u8 op; > +}; > + > static void cmdq_client_timeout(struct timer_list *t) > { > struct cmdq_client *client = from_timer(client, t, timer); > @@ -110,10 +122,8 @@ void cmdq_pkt_destroy(struct cmdq_pkt *pkt) > } > EXPORT_SYMBOL(cmdq_pkt_destroy); > > -static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code, > -u32 arg_a, u32 arg_b) > +static struct cmdq_instruction *cmdq_pkt_append_command(struct cmdq_pkt *pkt) > { > - u64 *cmd_ptr; > > if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) { > /* > @@ -127,81 +137,109 @@ static int cmdq_pkt_append_command(struct cmdq_pkt > *pkt, enum cmdq_code code, > pkt->cmd_buf_size += CMDQ_INST_SIZE; > WARN_ONCE(1, "%s: buffer size %u is too small !\n", > __func__, (u32)pkt->buf_size); > - return -ENOMEM; > + return NULL; > } > - cmd_ptr = pkt->va_base + pkt->cmd_buf_size; > - (*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b; > + > + *(u64 *)(pkt->va_base + pkt->cmd_buf_size) = 0;> > pkt->cmd_buf_size += CMDQ_INST_SIZE; > > - return 0; > + return pkt->va_base + pkt->cmd_buf_size - CMDQ_INST_SIZE; > } > > int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value) > { > - u32 arg_a = (offset & CMDQ_ARG_A_WRITE_MASK) | > - (subsys << CMDQ_SUBSYS_SHIFT); > + struct cmdq_instruction *inst; > + > + inst = cmdq_pkt_append_command(pkt); > + if (!inst) > + return -ENOMEM; > + > + inst->op = CMDQ_CODE_WRITE; > + inst->value = value; > + inst->offset = offset; > + inst->subsys = subsys; > I can see that using cmdq_instruction will make the code more readable, but I dislike the approach that cmdq_pkt_append_command returns a pointer where we write the instruction to. Better we pass inst to cmdq_pkt_append_command() and write it there to cmd_ptr. I think this way we can get rid of explicitly setting the memory to zero: *(u64 *)(pkt->va_base + pkt->cmd_buf_size) = 0; And if we pass the inst to the append_command we don't have to change the return value handling of cmdq_pkt_append_command(), which makes the patch easier to understand. > - return cmdq_pkt_append_command(pkt, CMDQ_CODE_WRITE, arg_a, value); > + return 0; > } > EXPORT_SYMBOL(cmdq_pkt_write); > > int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys, > u16 offset, u32 value, u32 mask) > { > - u32 offset_mask = offset; > - int err = 0; > + struct cmdq_instruction *inst; > + u16 offset_mask = offset; > > if (mask != 0x) { > - err = cmdq_pkt_append_command(pkt, CMDQ_CODE_MASK, 0, ~mask); > + inst = cmdq_pkt_append_command(pkt); > + if (!inst) > + return -ENOMEM; > + > + inst->op = CMDQ_CODE_MASK; > + inst->mask = ~mask; > offset_mask |= CMDQ_WRITE_ENABLE_MASK; > } > - err |= cmdq_pkt_write(pkt, value, subsys, offset_mask); > > - return err; > + return cmdq_pkt_write(pkt, subsys, offset_mask, value); > } > EXPORT_SYMBOL(cmdq_pkt_write_mask); > > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > { > - u32 arg_b; > + struct cmdq_instruction *inst; > > if (event >= CMDQ_MAX_EVENT) > return -EINVAL; > > - /* > - * WFE arg_b > - * bit 0-11: wait value > - * bit 15: 1 - wait, 0 - no wait > - * bit 16-27: update value > - * bit 31: 1 - update, 0 - no update > - */ I have no strong opinion of CMDQ_WFE_OPTION but if you want to introduce it, then please copy the comment over to
Re: [RESEND, PATCH v13 09/12] soc: mediatek: cmdq: define the instruction struct
Reviewed-by: Houlong Wei On Tue, 2019-08-20 at 16:49 +0800, Bibby Hsieh wrote: > Define an instruction structure for gce driver to append command. > This structure can make the client's code more readability. > > Signed-off-by: Bibby Hsieh > Reviewed-by: CK Hu > --- > drivers/soc/mediatek/mtk-cmdq-helper.c | 106 +++ > include/linux/mailbox/mtk-cmdq-mailbox.h | 2 + > 2 files changed, 74 insertions(+), 34 deletions(-) > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c > b/drivers/soc/mediatek/mtk-cmdq-helper.c > index 7aa0517ff2f3..e3d5b0be8e79 100644 > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c > @@ -9,12 +9,24 @@ > #include > #include > > -#define CMDQ_ARG_A_WRITE_MASK0x > #define CMDQ_WRITE_ENABLE_MASK BIT(0) > #define CMDQ_EOC_IRQ_EN BIT(0) > #define CMDQ_EOC_CMD ((u64)((CMDQ_CODE_EOC << CMDQ_OP_CODE_SHIFT)) \ > << 32 | CMDQ_EOC_IRQ_EN) > > +struct cmdq_instruction { > + union { > + u32 value; > + u32 mask; > + }; > + union { > + u16 offset; > + u16 event; > + }; > + u8 subsys; > + u8 op; > +}; > + > static void cmdq_client_timeout(struct timer_list *t) > { > struct cmdq_client *client = from_timer(client, t, timer); > @@ -110,10 +122,8 @@ void cmdq_pkt_destroy(struct cmdq_pkt *pkt) > } > EXPORT_SYMBOL(cmdq_pkt_destroy); > > -static int cmdq_pkt_append_command(struct cmdq_pkt *pkt, enum cmdq_code code, > -u32 arg_a, u32 arg_b) > +static struct cmdq_instruction *cmdq_pkt_append_command(struct cmdq_pkt *pkt) > { > - u64 *cmd_ptr; > > if (unlikely(pkt->cmd_buf_size + CMDQ_INST_SIZE > pkt->buf_size)) { > /* > @@ -127,81 +137,109 @@ static int cmdq_pkt_append_command(struct cmdq_pkt > *pkt, enum cmdq_code code, > pkt->cmd_buf_size += CMDQ_INST_SIZE; > WARN_ONCE(1, "%s: buffer size %u is too small !\n", > __func__, (u32)pkt->buf_size); > - return -ENOMEM; > + return NULL; > } > - cmd_ptr = pkt->va_base + pkt->cmd_buf_size; > - (*cmd_ptr) = (u64)((code << CMDQ_OP_CODE_SHIFT) | arg_a) << 32 | arg_b; > + > + *(u64 *)(pkt->va_base + pkt->cmd_buf_size) = 0; > pkt->cmd_buf_size += CMDQ_INST_SIZE; > > - return 0; > + return pkt->va_base + pkt->cmd_buf_size - CMDQ_INST_SIZE; > } > > int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value) > { > - u32 arg_a = (offset & CMDQ_ARG_A_WRITE_MASK) | > - (subsys << CMDQ_SUBSYS_SHIFT); > + struct cmdq_instruction *inst; > + > + inst = cmdq_pkt_append_command(pkt); > + if (!inst) > + return -ENOMEM; > + > + inst->op = CMDQ_CODE_WRITE; > + inst->value = value; > + inst->offset = offset; > + inst->subsys = subsys; > > - return cmdq_pkt_append_command(pkt, CMDQ_CODE_WRITE, arg_a, value); > + return 0; > } > EXPORT_SYMBOL(cmdq_pkt_write); > > int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys, > u16 offset, u32 value, u32 mask) > { > - u32 offset_mask = offset; > - int err = 0; > + struct cmdq_instruction *inst; > + u16 offset_mask = offset; > > if (mask != 0x) { > - err = cmdq_pkt_append_command(pkt, CMDQ_CODE_MASK, 0, ~mask); > + inst = cmdq_pkt_append_command(pkt); > + if (!inst) > + return -ENOMEM; > + > + inst->op = CMDQ_CODE_MASK; > + inst->mask = ~mask; > offset_mask |= CMDQ_WRITE_ENABLE_MASK; > } > - err |= cmdq_pkt_write(pkt, value, subsys, offset_mask); > > - return err; > + return cmdq_pkt_write(pkt, subsys, offset_mask, value); > } > EXPORT_SYMBOL(cmdq_pkt_write_mask); > > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event) > { > - u32 arg_b; > + struct cmdq_instruction *inst; > > if (event >= CMDQ_MAX_EVENT) > return -EINVAL; > > - /* > - * WFE arg_b > - * bit 0-11: wait value > - * bit 15: 1 - wait, 0 - no wait > - * bit 16-27: update value > - * bit 31: 1 - update, 0 - no update > - */ > - arg_b = CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE; > + inst = cmdq_pkt_append_command(pkt); > + if (!inst) > + return -ENOMEM; > + > + inst->op = CMDQ_CODE_WFE; > + inst->value = CMDQ_WFE_OPTION; > + inst->event = event; > > - return cmdq_pkt_append_command(pkt, CMDQ_CODE_WFE, event, arg_b); > + return 0; > } > EXPORT_SYMBOL(cmdq_pkt_wfe); > > int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event) > { > + struct cmdq_instruction *inst; > + > if (event >= CMDQ_MAX_EVENT) > return -EINVAL; > > - return cmdq_pkt_append_command(pkt,