Hi Jason,
On Mon, Oct 23, 2023 at 12:39 PM Jason-JH.Lin <[email protected]> wrote:
>
> Add cmdq_pkt_logic_command to support match operation.
s/match/math/
>
> cmdq_pkt_logic_command can append logic command to the CMDQ packet,
> ask GCE to execute a arithematic calculate instruction,
s/arithematic/arithmetic/
> such as add, subtract, multiply, AND, OR and NOT, etc.
>
> Note that all instructions just accept unsigned calculate.
s/calculate/calculation/
Or I'd rephrase it as:
Note that all arithmetic instructions are unsigned calculations.
> If there are any overflows, GCE will sent the invalid IRQ to notify
> CMDQ driver.
>
> Signed-off-by: Jason-JH.Lin <[email protected]>
> ---
> drivers/soc/mediatek/mtk-cmdq-helper.c | 36 ++++++++++++++++++++++
> include/linux/soc/mediatek/mtk-cmdq.h | 41 ++++++++++++++++++++++++++
> 2 files changed, 77 insertions(+)
>
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index b0cd071c4719..5194d66dfc0a 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -13,9 +13,18 @@
> #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> #define CMDQ_POLL_ENABLE_MASK BIT(0)
> #define CMDQ_EOC_IRQ_EN BIT(0)
> +#define CMDQ_IMMEDIATE_VALUE 0
> #define CMDQ_REG_TYPE 1
> #define CMDQ_JUMP_RELATIVE 1
>
> +#define CMDQ_OPERAND_GET_IDX_VALUE(operand) \
> + ({ \
> + struct cmdq_operand *op = operand; \
> + op->reg ? op->idx : op->value; \
> + })
`((operand)->reg ? (operand)->idx : (operand)->value)` fits in one line.
> +#define CMDQ_OPERAND_TYPE(operand) \
> + ((operand)->reg ? CMDQ_REG_TYPE : CMDQ_IMMEDIATE_VALUE)
> +
> struct cmdq_instruction {
> union {
> u32 value;
> @@ -380,6 +389,33 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> }
> EXPORT_SYMBOL(cmdq_pkt_poll_mask);
>
> +int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, enum CMDQ_LOGIC_ENUM s_op,
> + u16 result_reg_idx,
> + struct cmdq_operand *left_operand,
> + struct cmdq_operand *right_operand)
> +{
> + struct cmdq_instruction inst = { {0} };
> + u32 left_idx_value;
> + u32 right_idx_value;
> +
> + if (!left_operand || !right_operand)
> + return -EINVAL;
> +
> + left_idx_value = CMDQ_OPERAND_GET_IDX_VALUE(left_operand);
> + right_idx_value = CMDQ_OPERAND_GET_IDX_VALUE(right_operand);
> + inst.op = CMDQ_CODE_LOGIC;
> + inst.dst_t = CMDQ_REG_TYPE;
> + inst.src_t = CMDQ_OPERAND_TYPE(left_operand);
> + inst.arg_c_t = CMDQ_OPERAND_TYPE(right_operand);
> + inst.sop = s_op;
> + inst.arg_c = right_idx_value;
> + inst.src_reg = left_idx_value;
> + inst.reg_dst = result_reg_idx;
> +
> + return cmdq_pkt_append_command(pkt, inst);
> +}
> +EXPORT_SYMBOL(cmdq_pkt_logic_command);
> +
> int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> {
> struct cmdq_instruction inst = {};
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index a253c001c861..ea4fadfb5443 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -26,6 +26,30 @@
>
> struct cmdq_pkt;
>
> +enum CMDQ_LOGIC_ENUM {
Use lower case, and perhaps use `cmdq_logic_type` or `cmdq_operator_type`.
Regards,
Fei
On Mon, Oct 23, 2023 at 4:26 PM Fei Shao <[email protected]> wrote:
>
> Hi Jason,
>
> On Mon, Oct 23, 2023 at 12:39 PM Jason-JH.Lin <[email protected]> wrote:
<snip>
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index a253c001c861..ea4fadfb5443 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -26,6 +26,30 @@
> >
> > struct cmdq_pkt;
> >
> > +enum CMDQ_LOGIC_ENUM {
> Use lower case, and perhaps use `cmdq_logic_type` or `cmdq_operator_type`.
I overlooked the CMDQ_CODE_LOGIC line, so cmdq_logic_type makes more sense.
Regards,
Fei
Hi Fei,
Thanks for the reviews.
On Mon, 2023-10-23 at 16:26 +0800, Fei Shao wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Hi Jason,
>
> On Mon, Oct 23, 2023 at 12:39 PM Jason-JH.Lin <
> [email protected]> wrote:
> >
> > Add cmdq_pkt_logic_command to support match operation.
> s/match/math/
>
I'll fix this typo. Thanks.
> >
> > cmdq_pkt_logic_command can append logic command to the CMDQ packet,
> > ask GCE to execute a arithematic calculate instruction,
> s/arithematic/arithmetic/
>
I'll fix this typo. Thanks.
> > such as add, subtract, multiply, AND, OR and NOT, etc.
> >
> > Note that all instructions just accept unsigned calculate.
> s/calculate/calculation/
>
>
> Or I'd rephrase it as:
> Note that all arithmetic instructions are unsigned calculations.
>
OK, I'll rephrase as yours. Thanks!
> > If there are any overflows, GCE will sent the invalid IRQ to notify
> > CMDQ driver.
> >
> > Signed-off-by: Jason-JH.Lin <[email protected]>
> > ---
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 36 ++++++++++++++++++++++
> > include/linux/soc/mediatek/mtk-cmdq.h | 41
> ++++++++++++++++++++++++++
> > 2 files changed, 77 insertions(+)
> >
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index b0cd071c4719..5194d66dfc0a 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -13,9 +13,18 @@
> > #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> > #define CMDQ_POLL_ENABLE_MASK BIT(0)
> > #define CMDQ_EOC_IRQ_EN BIT(0)
> > +#define CMDQ_IMMEDIATE_VALUE 0
> > #define CMDQ_REG_TYPE 1
> > #define CMDQ_JUMP_RELATIVE 1
> >
> > +#define CMDQ_OPERAND_GET_IDX_VALUE(operand) \
> > + ({ \
> > + struct cmdq_operand *op = operand; \
> > + op->reg ? op->idx : op->value; \
> > + })
> `((operand)->reg ? (operand)->idx : (operand)->value)` fits in one
> line.
>
I had use that way, but it will get this CHECK warning:
CHECK: Macro argument reuse '_stat' - possible side-effects?
So I change to current way.
> > +#define CMDQ_OPERAND_TYPE(operand) \
> > + ((operand)->reg ? CMDQ_REG_TYPE : CMDQ_IMMEDIATE_VALUE)
> > +
> > struct cmdq_instruction {
> > union {
> > u32 value;
> > @@ -380,6 +389,33 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt,
> u8 subsys,
> > }
> > EXPORT_SYMBOL(cmdq_pkt_poll_mask);
> >
> > +int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, enum
> CMDQ_LOGIC_ENUM s_op,
> > + u16 result_reg_idx,
> > + struct cmdq_operand *left_operand,
> > + struct cmdq_operand *right_operand)
> > +{
> > + struct cmdq_instruction inst = { {0} };
> > + u32 left_idx_value;
> > + u32 right_idx_value;
> > +
> > + if (!left_operand || !right_operand)
> > + return -EINVAL;
> > +
> > + left_idx_value = CMDQ_OPERAND_GET_IDX_VALUE(left_operand);
> > + right_idx_value =
> CMDQ_OPERAND_GET_IDX_VALUE(right_operand);
> > + inst.op = CMDQ_CODE_LOGIC;
> > + inst.dst_t = CMDQ_REG_TYPE;
> > + inst.src_t = CMDQ_OPERAND_TYPE(left_operand);
> > + inst.arg_c_t = CMDQ_OPERAND_TYPE(right_operand);
> > + inst.sop = s_op;
> > + inst.arg_c = right_idx_value;
> > + inst.src_reg = left_idx_value;
> > + inst.reg_dst = result_reg_idx;
> > +
> > + return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_logic_command);
> > +
> > int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> > {
> > struct cmdq_instruction inst = {};
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> b/include/linux/soc/mediatek/mtk-cmdq.h
> > index a253c001c861..ea4fadfb5443 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -26,6 +26,30 @@
> >
> > struct cmdq_pkt;
> >
> > +enum CMDQ_LOGIC_ENUM {
> Use lower case, and perhaps use `cmdq_logic_type` or
> `cmdq_operator_type`.
>
Thanks! I think I would change to `cmdq_logic_op`.
Regards,
Jason-JH.Lin
> Regards,
> Fei