Re: [PATCH v8, 3/4] mailbox: mtk-cmdq: add gce ddr enable support flow

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 04/10/22 11:39, yongqiang.niu ha scritto:
> On Mon, 2022-10-03 at 16:54 +0200, AngeloGioacchino Del Regno wrote:
>> Il 30/09/22 18:06, Yongqiang Niu ha scritto:
>>> add gce ddr enable control flow when gce suspend/resume
>>>
>>> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
>>> ---
>>>    drivers/mailbox/mtk-cmdq-mailbox.c | 22 ++++++++++++++++++++++
>>>    1 file changed, 22 insertions(+)
>>>
>>> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
>>> b/drivers/mailbox/mtk-cmdq-mailbox.c
>>> index 04eb44d89119..2db82ff838ed 100644
>>> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
>>> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
>>> @@ -94,6 +94,18 @@ struct gce_plat {
>>>    	u32 gce_num;
>>>    };
>>>    
>>> +static void cmdq_sw_ddr_enable(struct cmdq *cmdq, bool enable)
>>> +{
>>> +	WARN_ON(clk_bulk_enable(cmdq->gce_num, cmdq->clocks));
>>> +
>>> +	if (enable)
>>> +		writel(GCE_DDR_EN | GCE_CTRL_BY_SW, cmdq->base +
>>> GCE_GCTL_VALUE);
>>
>> My only concern here is about the previous value stored in the
>> GCE_GCTL_VALUE
>> register, as you're overwriting it in its entirety with
>> GCE_DDR_EN | GCE_CTRL_BY_SW.
>>
>> Can you guarantee that this register is not pre-initialized with some
>> value,
>> and that these are the only bits to be `1` in this register?
>>
>> Otherwise, you will have to readl and modify the bits instead... by
>> the way,
>> if this register doesn't get any changes during runtime, you may
>> cache it
>> at probe time to avoid reading it for every suspend/resume operation.
>>
>> Regards,
>> Angelo
>>
>>
> 
> 0x48[2:0] means control by software
> 0x48[18:16] means ddr enable
> 0x48[2:0] is pre-condition of 0x48[18:16].
> if we want set 0x48[18:16] ddr enable, 0x48[2:0] must be set at same
> time.
> and only these bits is useful, other bits is useless bits
> 
> we need set 0x48[18:16] to 0 disable gce access ddr when suspend.
> and  set 0x48[18:16] to 0x7 enable gce access ddr when resume, there
> will be cmdq client send task to process.
> this control flow should controlled in suspend/resume flow.
> 
> 

That's perfect. Thanks for the explanation.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>