[PATCH v2 3/9] soc: mailbox: Add cmdq_pkt_logic_command to support math operation
Jason-JH Lin (林睿祥)
Jason-JH.Lin at mediatek.com
Tue Oct 24 10:11:47 PDT 2023
Hi Angelo,
Thanks for the reviews.
On Mon, 2023-10-23 at 11:50 +0200, AngeloGioacchino Del Regno wrote:
> Il 23/10/23 06:37, Jason-JH.Lin ha scritto:
> > Add cmdq_pkt_logic_command to support match operation.
> >
> > cmdq_pkt_logic_command can append logic command to the CMDQ packet,
> > ask GCE to execute a arithematic calculate instruction,
> > such as add, subtract, multiply, AND, OR and NOT, etc.
> >
> > Note that all instructions just accept unsigned calculate.
> > If there are any overflows, GCE will sent the invalid IRQ to notify
> > CMDQ driver.
> >
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin at mediatek.com>
>
> Please always run `git log` on the file that you're changing to
> properly format the
> commit title.
>
> In this case, this should be:
>
> soc: mediatek: cmdq: .....
I think it happended because I deleted the `\n` in the first line of
the title when I review these patches in `git send-email` cmd.
I won't do that in the next version. Thanks.
>
> > ---
> > 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; \
> > + })
> > +#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)
>
> I would also check that the requested logic operation is actually
> implemented:
>
> if (!left_operand || !right_operand || s_op >= CMDQ_LOGIC_MAX)
> return -EINVAL;
>
OK, I'll add this check.
> > + 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 {
>
> Please no capital letters in enumeration names.
> Besides, it's clearer if you named this
>
> enum cmdq_logic_operation {
>
> or
>
> enum cmdq_logic_op {
OK, I'll change to this one. Thanks.
>
> > + CMDQ_LOGIC_ASSIGN = 0,
> > + CMDQ_LOGIC_ADD = 1,
> > + CMDQ_LOGIC_SUBTRACT = 2,
> > + CMDQ_LOGIC_MULTIPLY = 3,
> > + CMDQ_LOGIC_XOR = 8,
> > + CMDQ_LOGIC_NOT = 9,
> > + CMDQ_LOGIC_OR = 10,
> > + CMDQ_LOGIC_AND = 11,
> > + CMDQ_LOGIC_LEFT_SHIFT = 12,
> > + CMDQ_LOGIC_RIGHT_SHIFT = 13,
>
> You should also add a `CMDQ_LOGIC_MAX,` here to use it in the code
> for validation.
OK, I'll add this. Thanks.
>
> > +};
> > +
> > +struct cmdq_operand {
> > + /* register type */
> > + bool reg;
> > + union {
> > + /* index */
> > + u16 idx;
> > + /* value */
> > + u16 value;
> > + };
> > +};
> > +
> > struct cmdq_client_reg {
> > u8 subsys;
> > u16 offset;
> > @@ -244,6 +268,23 @@ int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8
> > subsys,
> > int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> > u16 offset, u32 value, u32 mask);
> >
> > +/**
> > + * cmdq_pkt_logic_command() - Append logic command to the CMDQ
> > packet, ask GCE to
> > + * execute an instruction that store the result
> > of logic operation
> > + * with left and right operand into
> > result_reg_idx.
> > + * @pkt: the CMDQ packet
> > + * @s_op: the logic operator enum
> > + * @result_reg_idx: SPR index that store operation result
> > of left_operand and right_operand
> > + * @left_operand: left operand
> > + * @right_operand: right operand
> > + *
>
> I think that there's a way to dramatically increase human readability
> of calls to
> this function: being this a request to perform calculations, it would
> be way easier
> to read it like an actual calculation :-)
>
> cmdq_pkt_logic_command(pkt, result, op_x, CMDQ_LOGIC_ADD, op_y);
>
> At least in my head, this easily resembles "result = op_x + op_y".
>
> I therefore propose to change this to:
>
> int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16 result_reg_idx,
> struct cmdq_operand *left_operand,
> enum cmdq_logic_operation s_op,
> struct cmdq_operand *right_operand);
>
OK, I think it's a good idea.
I'll change that. Thanks!
Regards,
Jason-JH.Lin
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +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);
> > +
> > /**
> > * cmdq_pkt_assign() - Append logic assign command to the CMDQ
> > packet, ask GCE
> > * to execute an instruction that set a
> > constant value into
>
>
> Regards,
> Angelo
>
More information about the linux-arm-kernel
mailing list