Re: [PATCH v1 07/12] soc: mediatek: cmdq: add write_s function

[CK Hu <ck.hu@mediatek.com>]

Hi, Dennis:

On Fri, 2019-11-22 at 18:11 +0800, Dennis-YC Hsieh wrote:
> Hi CK,
> 
> On Fri, 2019-11-22 at 16:56 +0800, CK Hu wrote:
> > Hi, Dennis:
> > 
> > On Thu, 2019-11-21 at 17:12 +0800, Dennis YC Hsieh wrote:
> > > add write_s function in cmdq helper functions which
> > > support large dma access.
> > > 
> > > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > > ---
> > >  drivers/soc/mediatek/mtk-cmdq-helper.c   |   34 ++++++++++++++++++++++++++++++
> > >  include/linux/mailbox/mtk-cmdq-mailbox.h |    2 ++
> > >  include/linux/soc/mediatek/mtk-cmdq.h    |   13 ++++++++++++
> > >  3 files changed, 49 insertions(+)
> > > 
> > > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > index d419e99..1b074a9 100644
> > > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > > @@ -15,6 +15,9 @@
> > >  #define CMDQ_EOC_CMD		((u64)((CMDQ_CODE_EOC << CMDQ_OP_CODE_SHIFT)) \
> > >  				<< 32 | CMDQ_EOC_IRQ_EN)
> > >  #define CMDQ_REG_TYPE		1
> > > +#define CMDQ_ADDR_HIGH(addr)	((u32)(((addr) >> 16) & GENMASK(31, 0)))
> > > +#define CMDQ_ADDR_LOW_BIT	BIT(1)
> > > +#define CMDQ_ADDR_LOW(addr)	((u16)(addr) | CMDQ_ADDR_LOW_BIT)
> > >  
> > >  struct cmdq_instruction {
> > >  	union {
> > > @@ -224,6 +227,37 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
> > >  }
> > >  EXPORT_SYMBOL(cmdq_pkt_write_mask);
> > >  
> > > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, dma_addr_t addr,
> > > +		     u32 value, u32 mask)
> > > +{
> > > +	struct cmdq_instruction inst = { {0} };
> > > +	int err;
> > > +	const u16 dst_reg_idx = CMDQ_SPR_TEMP;
> > > +
> > > +	err = cmdq_pkt_assign(pkt, dst_reg_idx, CMDQ_ADDR_HIGH(addr));
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	if (mask != U32_MAX) {
> > > +		inst.op = CMDQ_CODE_MASK;
> > > +		inst.mask = ~mask;
> > > +		err = cmdq_pkt_append_command(pkt, inst);
> > > +		if (err < 0)
> > > +			return err;
> > > +
> > > +		inst.op = CMDQ_CODE_WRITE_S_MASK;
> > > +	} else {
> > > +		inst.op = CMDQ_CODE_WRITE_S;
> > > +	}
> > > +
> > > +	inst.sop = dst_reg_idx;
> > > +	inst.offset = CMDQ_ADDR_LOW(addr);
> > > +	inst.value = value;
> > > +
> > > +	return cmdq_pkt_append_command(pkt, inst);
> > > +}
> > > +EXPORT_SYMBOL(cmdq_pkt_write_s);
> > > +
> > >  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> > >  {
> > >  	struct cmdq_instruction inst = { {0} };
> > > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > > index 121c3bb..8ef87e1 100644
> > > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > > @@ -59,6 +59,8 @@ enum cmdq_code {
> > >  	CMDQ_CODE_JUMP = 0x10,
> > >  	CMDQ_CODE_WFE = 0x20,
> > >  	CMDQ_CODE_EOC = 0x40,
> > > +	CMDQ_CODE_WRITE_S = 0x90,
> > > +	CMDQ_CODE_WRITE_S_MASK = 0x91,
> > >  	CMDQ_CODE_LOGIC = 0xa0,
> > >  };
> > >  
> > > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > > index 8334021..8dbd046 100644
> > > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > > @@ -12,6 +12,7 @@
> > >  #include <linux/timer.h>
> > >  
> > >  #define CMDQ_NO_TIMEOUT		0xffffffffu
> > > +#define CMDQ_SPR_TEMP		0
> > >  
> > >  struct cmdq_pkt;
> > >  
> > > @@ -103,6 +104,18 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
> > >  			u16 offset, u32 value, u32 mask);
> > >  
> > >  /**
> > > + * cmdq_pkt_write_s() - append write_s command with mask to the CMDQ packet
> > > + * @pkt:	the CMDQ packet
> > > + * @addr:	the physical address of register or dma
> > > + * @value:	the specified target value
> > > + * @mask:	the specified target mask
> > > + *
> > > + * Return: 0 for success; else the error code is returned
> > > + */
> > > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, dma_addr_t addr,
> > > +		     u32 value, u32 mask);
> > 
> > You have an API cmdq_pkt_read_s() which read data into gce internal
> > register, so I expect that cmdq_pkt_write_s() is an API which write data
> > from gce internal register, the expected prototype is
> > 
> > int cmdq_pkt_write_s(struct cmdq_pkt *pkt, phys_addr_t addr, u16
> > reg_idx);
> > 
> > Your version would confuse the user because you hide the internal
> > register parameter. If you want to provide this service, I would like
> > you to change the function name so that user would not be confused and
> > easily to understand what you want to do in this function.
> > 
> > Another choice is: cmdq_pkt_write_s() is implemented in my definition,
> > and user could call cmdq_pkt_assign() and cmdq_pkt_write_s() to achieve
> > this function.
> > 
> > Regards,
> > CK
> > 
> 
> Thanks for your comment.
> 
> Ok, we have to provide write constant value service to client, so I will
> change the function name to cmdq_pkt_write_s_value() in this patch.
> 
> And since it is better to provide consistent API so I will design
> another function with interface as your suggestion:
> int cmdq_pkt_write_s(struct cmdq_pkt *pkt, phys_addr_t addr, u16
> reg_idx);
> 
> In another patch I provide cmdq_pkt_mem_move(). I will move part of
> implementation to cmdq_pkt_write_s(), so that cmdq_pkt_mem_move() can be
> combination of cmdq_pkt_read_s() and cmdq_pkt_write_s().
> 
> How do you think?

So cmdq_pkt_read_s()/cmdq_pkt_write_s() are the basic function and
cmdq_pkt_write_s_value()/cmdq_pkt_mem_move() are combination function. I
would like to keep the basic function and drop the combination function
at first. I think what we place in helper is used by two or more
clients. It's strong believed that basic function could be used by two
or more client, but it's doubt that combination would be. If only one
client use this combination, just place the combination in that client.
If later second client use this combination, we then move the common
code in helper and both client call the helper function. If you could
prove that this combination is used by two or more clients now, just
show me.

Regards,
CK

> 
> 
> Regards,
> Dennis
> 
> > > +
> > > +/**
> > >   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> > >   * @pkt:	the CMDQ packet
> > >   * @event:	the desired event type to "wait and CLEAR"
> > 
> > 
> 
>