Re: [PATCH v4 3/8] mailbox: mtk-cmdq: Add driver data to support for MT8196

[Jason-JH Lin (林睿祥)]

Hi Angelo,

Thanks for the reviews.

On Tue, 2025-03-04 at 10:32 +0100, AngeloGioacchino Del Regno wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 18/02/25 06:41, Jason-JH Lin ha scritto:
> > MT8196 has 3 new hardware configuration compared with the previous
> > SoC,
> > which correspond to the 3 new driver data:
> > 
> > 1. mminfra_offset: For GCE data plane control
> >     Since GCE has been moved into mminfra, GCE needs to append the
> >     mminfra offset to the DRAM address when accessing the DRAM.
> > 
> > 2. gce_vm: For GCE hardware virtualization
> >     Currently, the first version of the mt8196 mailbox controller
> > only
> >     requires setting the VM-related registers to enable the
> > permissions
> >     of a host VM.
> 
> I think that the GCE VM changes should go to a different commit, as
> that
> looks like being something not critical for basic functionality of
> the
> MMINFRA GCE.
> 
> I really like seeing support for that, but please split the basic
> stuff
> from the extra functionality :-)
> 

The VM configuration is the basic configuration for MT8196, so I put it
together with other configurations in one patch.
But I can understand you want the new function as a independent patch.
So I will split the VM related part, mminfra_offset part and dma_mask
part to 3 single pathes. Then add them as a driver data for MT8196 in
this patch.

> > 
> > 3. dma_mask_bit: For dma address bit control
> >     In order to avoid the hardware limitations of MT8196 accessing
> > DRAM,
> >     GCE needs to configure the DMA address to be less than 35 bits.
> > 
> > Signed-off-by: Jason-JH Lin <jason-jh.lin@xxxxxxxxxxxx>
> > ---
> >   drivers/mailbox/mtk-cmdq-mailbox.c       | 90
> > +++++++++++++++++++++---
> >   include/linux/mailbox/mtk-cmdq-mailbox.h |  2 +
> >   2 files changed, 84 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index d186865b8dce..0abe10a7fef9 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -43,6 +43,17 @@
> >   #define GCE_CTRL_BY_SW                              GENMASK(2, 0)
> >   #define GCE_DDR_EN                          GENMASK(18, 16)
> > 
> > +#define GCE_VM_ID_MAP0                       0x5018
> > +#define GCE_VM_MAP0_ALL_HOST                 GENMASK(29, 0)
> > +#define GCE_VM_ID_MAP1                       0x501c
> > +#define GCE_VM_MAP1_ALL_HOST                 GENMASK(29, 0)
> > +#define GCE_VM_ID_MAP2                       0x5020
> > +#define GCE_VM_MAP2_ALL_HOST                 GENMASK(29, 0)
> > +#define GCE_VM_ID_MAP3                       0x5024
> > +#define GCE_VM_MAP3_ALL_HOST                 GENMASK(5, 0)
> > +#define GCE_VM_CPR_GSIZE             0x50c4
> > +#define GCE_VM_CPR_GSIZE_HSOT                        GENMASK(3, 0)
> 
> typo: GSIZE_HOST....
> 

Thanks, I'll fix it.

> ...but also, if you could add some brief description of what the
> VMIDs are used for
> and what the GSIZE is... that'd be very much appreciated from whoever
> is reading
> this.
> 
VMID_MAP configuration is in the previous reply mail for CK.
CPR_GSIZE is the setting for allocating the CPR SRAM size to each VM.

> The GCE stuff isn't even properly described in datasheets - I do
> (probably!)
> understand what those are for, but asking people to get years of
> experience on
> MediaTek to understand what's going on would be a bit rude, wouldn't
> it? :-D
> 

I agree with you :-)
I'll put them in the VM patch and add some brief description for them.

> > +
> >   #define CMDQ_THR_ACTIVE_SLOT_CYCLES 0x3200
> >   #define CMDQ_THR_ENABLED            0x1
> >   #define CMDQ_THR_DISABLED           0x0
> > @@ -87,11 +98,24 @@ struct cmdq {
> >   struct gce_plat {
> >       u32 thread_nr;
> >       u8 shift;
> > +     dma_addr_t mminfra_offset;
> 
> It looks like this is exactly the DRAM's iostart... at least, I can
> see that in the
> downstream devicetree that's where it starts.
> 
>         memory: memory@80000000 {
>                 device_type = "memory";
>                 reg = <0 0x80000000 0 0x40000000>;
>         };
> 
> It doesn't really look like being a coincidence, but, for the sake of
> asking:
> is this just a coincidence? :-)
> 

As the confirmation with the hardware designer in previous reply mail
for CK:
https://patchwork.kernel.org/project/linux-mediatek/patch/20250218054405.2017918-4-jason-jh.lin@xxxxxxxxxxxx/#26258463

Since the MMINFRA remap subtracting 2G is done in the hardware circuit
and cannot be configured by software, the address +2G adjustment is
necessary to implement in the CMDQ driver.

So that might not be a coincidence.
But even if DRAM start address changes, this mminfra_offset is still
subtracting 2G, so I think it is a better choice to define it as the
driver data for MT8196.

> >       bool control_by_sw;
> >       bool sw_ddr_en;
> > +     bool gce_vm;
> > +     u32 dma_mask_bit;
> >       u32 gce_num;
> >   };
> > 
> > +static inline u32 cmdq_reg_shift_addr(dma_addr_t addr, const
> > struct gce_plat *pdata)
> > +{
> > +     return ((addr + pdata->mminfra_offset) >> pdata->shift);
> > +}
> > +
> > +static inline u32 cmdq_reg_revert_addr(dma_addr_t addr, const
> > struct gce_plat *pdata)
> > +{
> > +     return ((addr << pdata->shift) - pdata->mminfra_offset);
> > +}
> 
> I'm not sure that you really need those two functions... probably
> it's simply
> cleaner and easier to just write that single line every time... and
> I'm
> saying that especially for how you're using those functions, with
> some readl()
> passed directly as param, decreasing human readability by "a whole
> lot" :-)
> 

The reason why I use API wrapper instead of writing it directly in
readl() is to avoid missing the shift or mminfra_offset conversion in
some places.
This problem is not easy to debug, and I have encountered it at least
twice...

I think the advantage of using function is that it can be uniformly
modified to all places that need to handle DRAM address conversion.
What do you think? :-)

> > +
> >   static void cmdq_sw_ddr_enable(struct cmdq *cmdq, bool enable)
> >   {
> >       WARN_ON(clk_bulk_enable(cmdq->pdata->gce_num, cmdq->clocks));
> > @@ -112,6 +136,30 @@ u8 cmdq_get_shift_pa(struct mbox_chan *chan)
> >   }
> >   EXPORT_SYMBOL(cmdq_get_shift_pa);
> > 
> > +dma_addr_t cmdq_get_offset_pa(struct mbox_chan *chan)
> > +{
> > +     struct cmdq *cmdq = container_of(chan->mbox, struct cmdq,
> > mbox);
> > +
> > +     return cmdq->pdata->mminfra_offset;
> > +}
> > +EXPORT_SYMBOL(cmdq_get_offset_pa);
> 
> I think I remember this get_offset_pa from the old times, then CK
> removed it (and I
> was really happy about that disappearing), or am I confusing this
> with something
> else?
> 
> (of course, this wasn't used for mminfra, but for something else!)
> 

I can't find any remove history in mtk-cmdq-mailbox.c.

Maybe you mean the patch in this series?
https://lore.kernel.org/all/171213938049.123698.15573779837703602591.b4-ty@xxxxxxxxxxxxx/

> > +
> > +bool cmdq_addr_need_offset(struct mbox_chan *chan, dma_addr_t
> > addr)
> > +{
> > +     struct cmdq *cmdq = container_of(chan->mbox, struct cmdq,
> > mbox);
> > +
> > +     if (cmdq->pdata->mminfra_offset == 0)
> > +             return false;
> > +
> > +     /*
> > +      * mminfra will recognize the addr that greater than the
> > mminfra_offset
> > +      * as a transaction to DRAM.
> > +      * So the caller needs to append mminfra_offset for the true
> > case.
> > +      */
> > +     return (addr >= cmdq->pdata->mminfra_offset);
> 
> 
> /**
>   * cmdq_is_mminfra_gce() - Brief description
>   * @args.....
>   *
>   * The MMINFRA GCE will recognize an address greater than DRAM
> iostart as a
>   * DRAM transaction instead of ....xyz
>   *
>   * In order for callers to perform (xyz) transactions through the
> CMDQ, those
>   * need to know if they are using a GCE located in MMINFRA.
>   */
> bool cmdq_is_mminfra_gce(...)
> {
>         return cmdq->pdata->mminfra_offset &&
>                (addr >= cmdq->pdata->mminfra_offset)
> 
> > +}
> > +EXPORT_SYMBOL(cmdq_addr_need_offset);
> > +
> 

OK, I'll modify the API like this.

> ...but then, is there really no way of just handling the GCE being in
> MMINFRA
> transparently from the callers? Do the callers really *need* to know
> that they're
> using a new GCE?!
> 

Since the address subtracting is done in MMINFRA hardware, I think GCE
users really need to handle it in driver.

> Another way of saying: can't we just handle the address translation
> in here instead
> of instructing each and every driver about how to communicate with
> the new GCE?!
> 

The DRAM address may not only be the command buffer to GCE, but also
the working buffer provided by CMDQ users and being a part of GCE
instruction, so we need to handle the address translation in CMDQ
helper driver for the instruction generation.
E.g. ISP drivers may use GCE to write a hardware settings to a DRAM as
backup buffer. The GCE write instruction will be:
WRITE the value of ISP register to DRAM address + mminfra_offset.

But most of the CMDQ users only need to use GCE to write hardware
register, so I only keep the translation in cmdq_pkt_mem_move(),
cmdq_pkt_poll_addr() and cmdq_pkt_jump_abs() at the latest series.

Regards,
Jason-JH lin

> 
> Cheers,
> Angelo