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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <6047c983-a367-6a45-6849-9a605f70e5c1@collabora.com>
Date:   Tue, 4 Oct 2022 11:55:25 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     "yongqiang.niu" <yongqiang.niu@...iatek.com>,
        CK Hu <ck.hu@...iatek.com>,
        Chun-Kuang Hu <chunkuang.hu@...nel.org>
Cc:     Jassi Brar <jassisinghbrar@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        Hsin-Yi Wang <hsinyi@...omium.org>
Subject: Re: [PATCH v8, 3/4] mailbox: mtk-cmdq: add gce ddr enable support
 flow

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@...iatek.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@...labora.com>