Re: [PATCH v2 02/14] mailbox: cmdq: variablize address shift in platform

From: Dennis-YC Hsieh
Date: Wed Dec 11 2019 - 20:03:24 EST

Next message: Marek Vasut: "Re: [PATCH v2 2/2] mtd: rawnand: denali_dt: add reset controlling"
Previous message: Yang Weijiang: "Re: [PATCH v8 4/7] KVM: VMX: Load CET states on vmentry/vmexit"
In reply to: CK Hu: "Re: [PATCH v2 02/14] mailbox: cmdq: variablize address shift in platform"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi CK,

Thanks for your comment.

On Tue, 2019-12-10 at 09:55 +0800, CK Hu wrote:
> Hi, Dennis:
>
> On Wed, 2019-11-27 at 09:58 +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@xxxxxxxxxxxx>
> > ---
> > 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..d5536563fce1 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -76,8 +76,21 @@ 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);
>

will do

> > +
> > static int cmdq_thread_suspend(struct cmdq *cmdq, struct cmdq_thread *thread)
> > {
> > u32 status;
> > @@ -176,6 +189,7 @@ static void cmdq_task_remove_wfe(struct cmdq_task *task)
> > {
> > struct device *dev = task->cmdq->mbox.dev;
> > u64 *base = task->pkt->va_base;
> > + struct cmdq *cmdq = task->cmdq;
> > int i;
> >
> > dma_sync_single_for_cpu(dev, task->pa_base, task->pkt->cmd_buf_size,
> > @@ -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 >> cmdq->shift_pa;
>
> cmdq is only used here, so I would like
>
> CMDQ_JUMP_PASS >> task->cmdq->shift_pa;
>

ok, will fix

> > 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);
> > 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, .shift = 0};
> > +static const struct gce_plat gce_plat_v3 = {.thread_nr = 24, .shift = 0};
>
> For global variable, you need not to initialize it to zero.
>

will fix

> > +
> > 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 9add0fd5fa6c..274f6f311d05 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -281,6 +281,7 @@ EXPORT_SYMBOL(cmdq_pkt_poll_mask);
> >
> > static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > {
> > + struct cmdq_client *cl = pkt->cl;
> > struct cmdq_instruction inst = { {0} };
> > int err;
> >
> > @@ -293,7 +294,7 @@ 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(cl->chan);
>
> cl is used only here, so I would like
>
> cmdq_mbox_shift(pkt->cl->chan);
>
> Regards,
> CK
>

will fix

Regards,
Dennis

> > 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__ */
>
>

Next message: Marek Vasut: "Re: [PATCH v2 2/2] mtd: rawnand: denali_dt: add reset controlling"
Previous message: Yang Weijiang: "Re: [PATCH v8 4/7] KVM: VMX: Load CET states on vmentry/vmexit"
In reply to: CK Hu: "Re: [PATCH v2 02/14] mailbox: cmdq: variablize address shift in platform"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]