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
