Messages in this thread | | | Date | Tue, 4 Oct 2022 11:55:25 +0200 | Subject | Re: [PATCH v8, 3/4] mailbox: mtk-cmdq: add gce ddr enable support flow | From | AngeloGioacchino Del Regno <> |
| |
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>
| |