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
>>>>>
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
