Re: [PATCH v9, 2/4] mailbox: mtk-cmdq: add gce software ddr enable private data

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

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,
>>
>>
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel