Messages in this thread | | | From | Jason-JH Lin (林睿祥) <> | Subject | Re: [PATCH v2 3/9] soc: mailbox: Add cmdq_pkt_logic_command to support math operation | Date | Tue, 24 Oct 2023 17:11:47 +0000 |
| |
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@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 >
| |