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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 29 Feb 2020 21:36:31 +0800
From:   Dennis-YC Hsieh <dennis-yc.hsieh@...iatek.com>
To:     CK Hu <ck.hu@...iatek.com>
CC:     Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Jassi Brar <jassisinghbrar@...il.com>,
        <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>, <devicetree@...r.kernel.org>,
        <wsd_upstream@...iatek.com>,
        Bibby Hsieh <bibby.hsieh@...iatek.com>,
        Houlong Wei <houlong.wei@...iatek.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <srv_heupstream@...iatek.com>,
        "Ming-Fan Chen" <ming-fan.chen@...iatek.com>
Subject: Re: [PATCH v3 02/13] mailbox: cmdq: variablize address shift in
 platform

Hi CK,

Thanks for your comment.

On Fri, 2020-02-28 at 23:22 +0800, CK Hu wrote:
> Hi, Dennis:
> 
> On Fri, 2020-02-28 at 21:44 +0800, Dennis YC Hsieh wrote:
> > Some gce hardware shift pc and end address in register to support
> > large dram addressing.
> > Implement gce address shift when write or read pc and end register.
> > And add shift bit in platform definition.
> > 
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@...iatek.com>
> > ---
> >  drivers/mailbox/mtk-cmdq-mailbox.c       | 57 ++++++++++++++++++------
> >  drivers/soc/mediatek/mtk-cmdq-helper.c   |  3 +-
> >  include/linux/mailbox/mtk-cmdq-mailbox.h |  2 +
> >  3 files changed, 48 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 9a6ce9f5a7db..a98f0357dd7d 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -76,8 +76,22 @@ struct cmdq {
> >  	struct cmdq_thread	*thread;
> >  	struct clk		*clock;
> >  	bool			suspended;
> > +	u8			shift_pa;
> >  };
> >  
> > +struct gce_plat {
> > +	u32 thread_nr;
> > +	u8 shift;
> > +};
> > +
> > +u8 cmdq_mbox_shift(struct mbox_chan *chan)
> > +{
> > +	struct cmdq *cmdq = container_of(chan->mbox, struct cmdq, mbox);
> > +
> > +	return cmdq->shift_pa;
> > +}
> > +EXPORT_SYMBOL(cmdq_mbox_shift);
> > +
> >  static int cmdq_thread_suspend(struct cmdq *cmdq, struct cmdq_thread *thread)
> >  {
> >  	u32 status;
> > @@ -183,7 +197,7 @@ static void cmdq_task_remove_wfe(struct cmdq_task *task)
> >  	for (i = 0; i < CMDQ_NUM_CMD(task->pkt); i++)
> >  		if (cmdq_command_is_wfe(base[i]))
> >  			base[i] = (u64)CMDQ_JUMP_BY_OFFSET << 32 |
> > -				  CMDQ_JUMP_PASS;
> > +				  CMDQ_JUMP_PASS >> task->cmdq->shift_pa;
> >  	dma_sync_single_for_device(dev, task->pa_base, task->pkt->cmd_buf_size,
> >  				   DMA_TO_DEVICE);
> >  }
> > @@ -221,13 +235,15 @@ static void cmdq_task_handle_error(struct cmdq_task *task)
> >  {
> >  	struct cmdq_thread *thread = task->thread;
> >  	struct cmdq_task *next_task;
> > +	struct cmdq *cmdq = task->cmdq;
> >  
> >  	dev_err(task->cmdq->mbox.dev, "task 0x%p error\n", task);
> >  	WARN_ON(cmdq_thread_suspend(task->cmdq, thread) < 0);
> 
> If you invent local variable 'cmdq', I think you could replace all
> task->cmdq with cmdq in this function.
> 

Ok, will do.


> >  	next_task = list_first_entry_or_null(&thread->task_busy_list,
> >  			struct cmdq_task, list_entry);
> >  	if (next_task)
> > -		writel(next_task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
> > +		writel(next_task->pa_base >> cmdq->shift_pa,
> > +		       thread->base + CMDQ_THR_CURR_ADDR);
> >  	cmdq_thread_resume(thread);
> >  }
> >  
> > @@ -257,7 +273,7 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq,
> >  	else
> >  		return;
> >  
> > -	curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR);
> > +	curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR) << cmdq->shift_pa;
> >  
> >  	list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
> >  				 list_entry) {
> > @@ -373,16 +389,20 @@ static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data)
> >  		WARN_ON(clk_enable(cmdq->clock) < 0);
> >  		WARN_ON(cmdq_thread_reset(cmdq, thread) < 0);
> >  
> > -		writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
> > -		writel(task->pa_base + pkt->cmd_buf_size,
> > +		writel(task->pa_base >> cmdq->shift_pa,
> > +		       thread->base + CMDQ_THR_CURR_ADDR);
> > +		writel((task->pa_base + pkt->cmd_buf_size) >> cmdq->shift_pa,
> >  		       thread->base + CMDQ_THR_END_ADDR);
> > +
> >  		writel(thread->priority, thread->base + CMDQ_THR_PRIORITY);
> >  		writel(CMDQ_THR_IRQ_EN, thread->base + CMDQ_THR_IRQ_ENABLE);
> >  		writel(CMDQ_THR_ENABLED, thread->base + CMDQ_THR_ENABLE_TASK);
> >  	} else {
> >  		WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> > -		curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR);
> > -		end_pa = readl(thread->base + CMDQ_THR_END_ADDR);
> > +		curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR) <<
> > +			cmdq->shift_pa;
> > +		end_pa = readl(thread->base + CMDQ_THR_END_ADDR) <<
> > +			cmdq->shift_pa;
> >  
> >  		/*
> >  		 * Atomic execution should remove the following wfe, i.e. only
> > @@ -395,7 +415,7 @@ static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data)
> >  				cmdq_thread_wait_end(thread, end_pa);
> >  				WARN_ON(cmdq_thread_suspend(cmdq, thread) < 0);
> >  				/* set to this task directly */
> > -				writel(task->pa_base,
> > +				writel(task->pa_base >> cmdq->shift_pa,
> >  				       thread->base + CMDQ_THR_CURR_ADDR);
> >  			} else {
> >  				cmdq_task_insert_into_thread(task);
> > @@ -407,14 +427,14 @@ static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data)
> >  			if (curr_pa == end_pa - CMDQ_INST_SIZE ||
> >  			    curr_pa == end_pa) {
> >  				/* set to this task directly */
> > -				writel(task->pa_base,
> > +				writel(task->pa_base >> cmdq->shift_pa,
> >  				       thread->base + CMDQ_THR_CURR_ADDR);
> >  			} else {
> >  				cmdq_task_insert_into_thread(task);
> >  				smp_mb(); /* modify jump before enable thread */
> >  			}
> >  		}
> > -		writel(task->pa_base + pkt->cmd_buf_size,
> > +		writel((task->pa_base + pkt->cmd_buf_size) >> cmdq->shift_pa,
> >  		       thread->base + CMDQ_THR_END_ADDR);
> >  		cmdq_thread_resume(thread);
> >  	}
> > @@ -461,6 +481,7 @@ static int cmdq_probe(struct platform_device *pdev)
> >  	struct resource *res;
> >  	struct cmdq *cmdq;
> >  	int err, i;
> > +	struct gce_plat *plat_data;
> >  
> >  	cmdq = devm_kzalloc(dev, sizeof(*cmdq), GFP_KERNEL);
> >  	if (!cmdq)
> > @@ -479,7 +500,14 @@ static int cmdq_probe(struct platform_device *pdev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	cmdq->thread_nr = (u32)(unsigned long)of_device_get_match_data(dev);
> > +	plat_data = (struct gce_plat *)of_device_get_match_data(dev);
> > +	if (!plat_data) {
> > +		dev_err(dev, "failed to get match data\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	cmdq->thread_nr = plat_data->thread_nr;
> > +	cmdq->shift_pa = plat_data->shift;
> >  	cmdq->irq_mask = GENMASK(cmdq->thread_nr - 1, 0);
> >  	err = devm_request_irq(dev, cmdq->irq, cmdq_irq_handler, IRQF_SHARED,
> >  			       "mtk_cmdq", cmdq);
> > @@ -542,9 +570,12 @@ static const struct dev_pm_ops cmdq_pm_ops = {
> >  	.resume = cmdq_resume,
> >  };
> >  
> > +static const struct gce_plat gce_plat_v2 = {.thread_nr = 16};
> > +static const struct gce_plat gce_plat_v3 = {.thread_nr = 24};
> > +
> >  static const struct of_device_id cmdq_of_ids[] = {
> > -	{.compatible = "mediatek,mt8173-gce", .data = (void *)16},
> > -	{.compatible = "mediatek,mt8183-gce", .data = (void *)24},
> > +	{.compatible = "mediatek,mt8173-gce", .data = (void *)&gce_plat_v2},
> > +	{.compatible = "mediatek,mt8183-gce", .data = (void *)&gce_plat_v3},
> >  	{}
> >  };
> >  
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index de20e6cba83b..2e1bc513569b 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -291,7 +291,8 @@ static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >  
> >  	/* JUMP to end */
> >  	inst.op = CMDQ_CODE_JUMP;
> > -	inst.value = CMDQ_JUMP_PASS;
> > +	inst.value = CMDQ_JUMP_PASS >>
> > +		cmdq_mbox_shift(((struct cmdq_client *)pkt->cl)->chan);
> 
> Why not just cmdq_mbox_shift(pkt->cl->chan) ?

Ok, will do.


Regards,
Dennis


> 
> Regards,
> CK
> 
> >  	err = cmdq_pkt_append_command(pkt, inst);
> >  
> >  	return err;
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index a4dc45fbec0a..dfe5b2eb85cc 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -88,4 +88,6 @@ struct cmdq_pkt {
> >  	void			*cl;
> >  };
> >  
> > +u8 cmdq_mbox_shift(struct mbox_chan *chan);
> > +
> >  #endif /* __MTK_CMDQ_MAILBOX_H__ */
> 
> 

Powered by blists - more mailing lists