Re: [PATCH v6 3/8] soc: mediatek: cmdq: Add cmdq_pkt_logic_command to support math operation

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

On Mon, 2024-06-24 at 13:39 +0200, AngeloGioacchino Del Regno wrote:
> Il 28/05/24 17:52, Jason-JH Lin (林睿祥) ha scritto:
> > Hi Angelo,
> > 
> > On Tue, 2024-05-28 at 12:24 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 26/05/24 01:08, Jason-JH.Lin ha scritto:
> > > > Add cmdq_pkt_logic_command to support math operation.
> > > > 
> > > > cmdq_pkt_logic_command can append logic command to the CMDQ
> > > > packet,
> > > > ask GCE to execute a arithmetic calculate instruction,
> > > > such as add, subtract, multiply, AND, OR and NOT, etc.
> > > > 
> > > > 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 <jason-jh.lin@mediatek.com>
> > > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> > > > ---
> > > >    drivers/soc/mediatek/mtk-cmdq-helper.c | 36
> > > > ++++++++++++++++++++++
> > > >    include/linux/soc/mediatek/mtk-cmdq.h  | 42
> > > > ++++++++++++++++++++++++++
> > > >    2 files changed, 78 insertions(+)
> > > > 
> > > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > index 046522664dc1..42fae05f61a8 100644
> > > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > @@ -15,10 +15,19 @@
> > > >    /* dedicate the last GPR_R15 to assign the register address
> > > > to be
> > > > poll */
> > > >    #define CMDQ_POLL_ADDR_GPR	(15)
> > > >    #define CMDQ_EOC_IRQ_EN		BIT(0)
> > > > +#define CMDQ_IMMEDIATE_VALUE	0
> > > >    #define CMDQ_REG_TYPE		1
> > > >    #define CMDQ_JUMP_RELATIVE	0
> > > >    #define CMDQ_JUMP_ABSOLUTE	1
> > > >    
> > > > +#define CMDQ_OPERAND_GET_IDX_VALUE(operand) \
> > > > +	({ \
> > > > +		struct cmdq_operand *op = operand; \
> > > > +		op->reg ? op->idx : op->value; \
> > > > +	})
> > > 
> > > I think this CMDQ_OPERAND_GET_IDX_VALUE would be better expressed
> > > as
> > > a (inline?)
> > > function instead...
> > > 
> > 
> > Yes, I can change them to static inline function to pass their type
> > directly.
> > 
> > > static inline u16 cmdq_operand_get_idx_value(struct cmdq_operand
> > > *op)
> > > {
> > > 	return op->reg ? op->idx : op->value;
> > > }
> > > 
> > > and maybe the same for the other definition too..
> > > 
> > > static inline u16 cmdq_operand_type(struct cmdq_operand *op)
> > > {
> > > 	return op->reg ? CMDQ_REG_TYPE : CMDQ_IMMEDIATE_VALUE;
> > > }
> > > 
> > > but definitely the first one is what I don't like much, the
> > > second
> > > one can pass.
> > > 
> > 
> > You mean '#define CMDQ_OPERAND_GET_IDX_VALUE()' is the first one or
> > 'static inline u16 cmdq_operand_get_idx_value()' is the first one?
> > 
> 
> I'm sorry, your reply slipped through for some reason and I've read
> it just now.
> 
> I mean I don't like the macros (#define
> CMDQ_OPERAND_GET_IDX_VALUE(..) and
> CMDQ_OPERAND_TYPE(..)) :-)
> 
> Please use static inline functions and resend; if you can do that
> this week, I
> can pick the commit before I send the PR.
> 
> P.S.: And you're right, re-reading my own reply, it was a bit
> ambiguous, so
> I'm again sorry for that, will be clearer next time.
> 

No problem!
Since other patches in this series are still cooking, I'll resend this
patch separately. Thanks!

Regards,
Jason-JH.Lin

> Thanks,
> Angelo
> 
> > Regards,
> > Jason-JH.Lin
> > 
> > > Cheers,
> > > Angelo
> > > 
> > > > +#define CMDQ_OPERAND_TYPE(operand) \
> > > > +	((operand)->reg ? CMDQ_REG_TYPE : CMDQ_IMMEDIATE_VALUE)
> > > > +
> > > >    struct cmdq_instruction {
> > > >    	union {
> > > >    		u32 value;
> > > > @@ -461,6 +470,33 @@ int cmdq_pkt_poll_addr(struct cmdq_pkt
> > > > *pkt,
> > > > dma_addr_t addr, u32 value, u32 mas
> > > >    }
> > > >    EXPORT_SYMBOL(cmdq_pkt_poll_addr);
> > > >    
> > > > +int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16
> > > > result_reg_idx,
> > > > +			   struct cmdq_operand *left_operand,
> > > > +			   enum cmdq_logic_op s_op,
> > > > +			   struct cmdq_operand *right_operand)
> > > > +{
> > > > +	struct cmdq_instruction inst = { {0} };
> > > > +	u32 left_idx_value;
> > > > +	u32 right_idx_value;
> > > > +
> > > > +	if (!left_operand || !right_operand || s_op >=
> > > > CMDQ_LOGIC_MAX)
> > > > +		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.reg_dst = result_reg_idx;
> > > > +	inst.src_reg = left_idx_value;
> > > > +	inst.arg_c = right_idx_value;
> > > > +
> > > > +	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 d4a8e34505e6..5bee6f7fc400 100644
> > > > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > > > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > > > @@ -25,6 +25,31 @@
> > > >    
> > > >    struct cmdq_pkt;
> > > >    
> > > > +enum cmdq_logic_op {
> > > > +	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,
> > > > +	CMDQ_LOGIC_MAX,
> > > > +};
> > > > +
> > > > +struct cmdq_operand {
> > > > +	/* register type */
> > > > +	bool reg;
> > > > +	union {
> > > > +		/* index */
> > > > +		u16 idx;
> > > > +		/* value */
> > > > +		u16 value;
> > > > +	};
> > > > +};
> > > > +
> > > >    struct cmdq_client_reg {
> > > >    	u8 subsys;
> > > >    	u16 offset;
> > > > @@ -272,6 +297,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
> > > > + * @result_reg_idx:	SPR index that store operation result
> > > > of left_operand and right_operand
> > > > + * @left_operand:	left operand
> > > > + * @s_op:		the logic operator enum
> > > > + * @right_operand:	right operand
> > > > + *
> > > > + * Return: 0 for success; else the error code is returned
> > > > + */
> > > > +int cmdq_pkt_logic_command(struct cmdq_pkt *pkt, u16
> > > > result_reg_idx,
> > > > +			   struct cmdq_operand *left_operand,
> > > > +			   enum cmdq_logic_op s_op,
> > > > +			   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
> > > 
> > > 
> > > 
> > > 
> 
>