Messages in this thread | | | Date | Fri, 7 Oct 2022 10:57:24 +0200 | Subject | Re: [PATCH v9, 2/4] mailbox: mtk-cmdq: add gce software ddr enable private data | From | AngeloGioacchino Del Regno <> |
| |
Il 07/10/22 03:51, yongqiang.niu ha scritto: > On Thu, 2022-10-06 at 11:29 +0200, AngeloGioacchino Del Regno wrote: >> Il 06/10/22 06:34, Yongqiang Niu ha scritto: >>> if gce work control by software, we need set software enable >>> for MT8186 Soc >>> >>> there is a handshake flow between gce and ddr hardware, >>> if not set ddr enable flag of gce, ddr will fall into idle >>> mode, then gce instructions will not process done. >>> we need set this flag of gce to tell ddr when gce is idle or busy >>> controlled by software flow. >>> >>> 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 >>> >>> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com> >>> --- >>> drivers/mailbox/mtk-cmdq-mailbox.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c >>> b/drivers/mailbox/mtk-cmdq-mailbox.c >>> index c3cb24f51699..04eb44d89119 100644 >>> --- a/drivers/mailbox/mtk-cmdq-mailbox.c >>> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c >>> @@ -39,6 +39,7 @@ >>> >>> #define GCE_GCTL_VALUE 0x48 >>> #define GCE_CTRL_BY_SW GENMASK(2, 0) >>> +#define GCE_DDR_EN GENMASK(18, 16) >>> >>> #define CMDQ_THR_ACTIVE_SLOT_CYCLES 0x3200 >>> #define CMDQ_THR_ENABLED 0x1 >>> @@ -81,6 +82,7 @@ struct cmdq { >>> bool suspended; >>> u8 shift_pa; >>> bool control_by_sw; >>> + bool sw_ddr_en; >>> u32 gce_num; >>> }; >>> >>> @@ -88,6 +90,7 @@ struct gce_plat { >>> u32 thread_nr; >>> u8 shift; >>> bool control_by_sw; >>> + bool sw_ddr_en; >>> u32 gce_num; >>> }; >>> >>> @@ -132,6 +135,9 @@ static void cmdq_init(struct cmdq *cmdq) >>> if (cmdq->control_by_sw) >>> writel(GCE_CTRL_BY_SW, cmdq->base + GCE_GCTL_VALUE); >>> >>> + if (cmdq->sw_ddr_en) >>> + writel(GCE_DDR_EN | GCE_CTRL_BY_SW, cmdq->base + >>> GCE_GCTL_VALUE); >>> + >> >> No. That's redundant. >> Here's a better way: >> >> u32 gctl_regval = 0; >> >> if (cmdq->control_by_sw) >> gctl_regval = GCE_CTRL_BY_SW; >> if (cmdq->sw_ddr_en) >> gctl_regval |= GCE_DDR_EN; >> >> if (val) >> writel(gctl_regval, cmdq->base + GCE_GCTL_VALUE); >> >> Regards, >> Angelo > > thanks very much for your advise. > shall i separate this into two patches? > 1st one is > u32 gctl_regval = 0; > if (cmdq->control_by_sw) >> gctl_regval = GCE_CTRL_BY_SW; >> if (val) >> writel(gctl_regval, cmdq->base + GCE_GCTL_VALUE); > > > > 2nd just add this > if (cmdq->sw_ddr_en) >> gctl_regval |= GCE_DDR_EN; > > or one patch is ok? >
One patch is ok.
Thanks, Angelo
>> >>> writel(CMDQ_THR_ACTIVE_SLOT_CYCLES, cmdq->base + >>> CMDQ_THR_SLOT_CYCLES); >>> for (i = 0; i <= CMDQ_MAX_EVENT; i++) >>> writel(i, cmdq->base + CMDQ_SYNC_TOKEN_UPDATE); >>> @@ -545,6 +551,7 @@ static int cmdq_probe(struct platform_device >>> *pdev) >>> cmdq->thread_nr = plat_data->thread_nr; >>> cmdq->shift_pa = plat_data->shift; >>> cmdq->control_by_sw = plat_data->control_by_sw; >>> + cmdq->sw_ddr_en = plat_data->sw_ddr_en; >>> cmdq->gce_num = plat_data->gce_num; >>> cmdq->irq_mask = GENMASK(cmdq->thread_nr - 1, 0); >>> err = devm_request_irq(dev, cmdq->irq, cmdq_irq_handler, >>> IRQF_SHARED, >> >> >> >
| |