Re: [PATCH v5 09/13] soc: mediatek: cmdq: add write_s value function

[Matthias Brugger]

On 25/05/2020 04:27, Dennis-YC Hsieh wrote:
> 
> On Sun, 2020-05-24 at 20:13 +0200, Matthias Brugger wrote:
>>
>> On 24/05/2020 19:31, Dennis-YC Hsieh wrote:
>>> Hi Matthias,
>>>
>>> Thanks for your comment.
>>>
>>> On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote:
>>>>
>>>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>>>> add write_s function in cmdq helper functions which
>>>>> writes a constant value to address with large dma
>>>>> access support.
>>>>>
>>>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@xxxxxxxxxxxx>
>>>>> Reviewed-by: CK Hu <ck.hu@xxxxxxxxxxxx>
>>>>> ---
>>>>>  drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++
>>>>>  include/linux/soc/mediatek/mtk-cmdq.h  | 14 ++++++++++++++
>>>>>  2 files changed, 40 insertions(+)
>>>>>
>>>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>>> index 03c129230cd7..a9ebbabb7439 100644
>>>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>>>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>>>  }
>>>>>  EXPORT_SYMBOL(cmdq_pkt_write_s);
>>>>>  
>>>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>>> +			   u16 addr_low, u32 value, u32 mask)
>>>>> +{
>>>>> +	struct cmdq_instruction inst = { {0} };
>>>>> +	int 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 = high_addr_reg_idx;
>>>>
>>>> Writing u16 value in a 5 bit wide variable?
>>>
>>> We need only 5 bits in this case. I'll change high_addr_reg_idx
>>> parameter to u8.
>>>
>>
>> Ok, please make sure to mask the value, so that it's explicit in the code that
>> we only use the lowest 5 bits of high_addr_reg_idx.
> 
> Is it necessary to mask the value?
> Since sop already defined as "u8 sop:5;", I thought it is explicit that
> only use 5 bits and compiler should do the rest jobs.

Yes but it makes the code more explicit if we have a
inst.sop = high_addr_reg_idx & 0x1f;

What do you think?

Regards,
Matthias

> 
> 
> Regards,
> Dennis
> 
>>
>> Regards,
>> Matthias
>>
>>>>
>>>>> +	inst.offset = addr_low;
>>>>> +	inst.value = value;
>>>>> +
>>>>> +	return cmdq_pkt_append_command(pkt, inst);
>>>>> +}
>>>>> +EXPORT_SYMBOL(cmdq_pkt_write_s_value);
>>>>> +
>>>>>  int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
>>>>>  {
>>>>>  	struct cmdq_instruction inst = { {0} };
>>>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>>>> index 01b4184af310..fec292aac83c 100644
>>>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>>>> @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
>>>>>  int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>>>  		     u16 addr_low, u16 src_reg_idx, u32 mask);
>>>>>  
>>>>> +/**
>>>>> + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ
>>>>> + *			      packet which write value to a physical address
>>>>> + * @pkt:	the CMDQ packet
>>>>> + * @high_addr_reg_idx:	internal regisger ID which contains high address of pa
>>>>
>>>> register
>>>
>>> will fix
>>>
>>>
>>> Regards,
>>> Dennis
>>>
>>>>
>>>>> + * @addr_low:	low address of pa
>>>>> + * @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_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>>>> +			   u16 addr_low, u32 value, u32 mask);
>>>>> +
>>>>>  /**
>>>>>   * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
>>>>>   * @pkt:	the CMDQ packet
>>>>>
>>>
>