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

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

> > 

[snip]

> > > > > > +
> > > > > >     #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@mediatek.com/*26258463
> > > > 
> > > 
> > > That explanation was simply wonderful.
> > > 
> > > > 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.
> > > > 
> > > 
> > > ....so, this makes me think the following:
> > > 
> > > 1. The DRAM start address cannot *ever* be less than 2G, because
> > > otherwise the
> > >      MMINFRA HW would have a hole in the usable address range;
> > >      1a. If the start address changes to less than 2G, then also
> > > the
> > > IOMMU would
> > >          get limitations, not only the mminfra..!
> > >      2b. This makes it very very very unlikely for the start
> > > address
> > > to be changed
> > >          to less than 0x80000000
> > > 
> > > 2. If the DRAM start address changes to be ABOVE 2G (so more than
> > > 0x80000000),
> > >      there would be no point for MMINFRA to start a "config path"
> > > write (or read)
> > >      in the SMMU DRAM block, would it? ;-)
> > > 
> > 
> > GCE is using IOMMU in MT8196, so all the address put into the GCE
> > instruction or GCE register for GCE access should be IOVA.
> > 
> > The DRAM start address is 2G(PA=0x80000000, IOVA=0x0) currently, so
> > when GCE want to access the IOVA=0x0, it will need to +2G into the
> > instruction, then the MMINFRA will see it as data path(IOVA > 2G)
> > and
> > subtract 2G for that IOVA, so GCE can finally access the IOVA=0x0.
> > 
> > I'm not sure if I've misunderstood what you mean by ABOVE 2G. :-)
> > If DRAM start address is changed to 3G(PA=0xc0000000) the IOVA is
> > still
> > 0x0, so GCE still need to + 2G to make MMINFRA go to the data path.
> > 
> > But if you mean PA=0x80000000 and IOVA start address is
> > 3G(0xc0000000),
> > then MMINFRA will go to the data path without GCE +2G.
> > However, MMINFRA will -2G when going to the data path and that will
> > cause GCE access the wrong IOVA.
> > So GCE still need to +2G no matter IOVA start address is already
> > can
> > make MMINFRA go to the data path(IOVA > 2G).
> > 
> > We have already complained to our hardware designer that MMINFRA -
> > 2G
> > con not be changed, which will make software operation very
> > troublesome.
> > So in the next few generations of SoC will change this MMINFRA -2G
> > to
> > software configurable. Then we can just make IOVA start address to
> > 2G
> > without adding the mminfra_offset to the IOVA for GCE.
> > 
> 
> Okay now I got it, the reality is way worse than I was thinking...
> eww... :-(
> 
> > > I get it - if the DRAM moves up, MMINFRA is still at 2G because
> > > that's hard baked
> > > into the hardware, but I foresee that it'll be unlikely to see a
> > > platform changing
> > > the DRAM start address arbitrarily, getting out-of-sync with
> > > MMINFRA.
> > > 
> > > I propose to just get the address from the memory node for now,
> > > and
> > > to add a nice
> > > comment in the code that explains that "In at least MT8196, the
> > > MMINFRA hardware
> > > subtracts xyz etc etc" (and that explanation from the previous
> > > email
> > > is again
> > > wonderful and shall not be lost: either use that in the comment,
> > > or
> > > add it to
> > > the commit description, because it's really that good).
> > > 
> > > Should a new SoC appear in the future requiring an offset from
> > > the
> > > DRAM start
> > > address, we will think about how to make that work in the best
> > > possible way: in
> > > that case we could either reference something else to get the
> > > right
> > > address or
> > > we can just change this driver to just use the 2G offset
> > > statically
> > > for all.
> > > 
> > > What I'm trying to do here is to reduce the amount of changes
> > > that
> > > we'd need for
> > > adding new SoCs: since that 2G MMINFRA offset -> 2G DRAM start is
> > > not
> > > a coincidence
> > > I think that, should the DRAM start vary on new SoCs, the MMINFRA
> > > offset will
> > > follow the trend and vary with it.
> > > 
> > > So what I think is:
> > > 1. If I'm right, adding a new SoC (with different MMINFRA + DRAM
> > > offset) will be
> > >      as easy as adding a compatible string in the bindings, no
> > > effort
> > > in changing
> > >      this driver with new pdata offsets;
> > > 2. If I'm wrong, adding a new SoC means adding compat string and
> > > adding pdata and
> > >      one variable in the cmdq struct.
> > > 
> > > Where N.2 is what we would do anyway if we don't go with my
> > > proposed
> > > solution...
> > > 
> > > All this is just to give you my considerations about this topic -
> > > you're left
> > > completely free to disagree with me.
> > > If you disagree, I will trust your judgement, no problem here.
> > > 
> > 
> > Yes, I think your are right. No matter the IOVA start address
> > changing,
> > MMINFRA will still -2G(the start address of DRAM PA).
> > Do you mean we can get the mminfra_offset from the start address of
> > memory in DTS, rather than defining it in pdata?
> > 
> 
> After the last explanation... no, it would be wrong to get the start
> from
> memory in DTS, because then this will still need hacks.
> I was somehow convinced that the DRAM start address and the MMINFRA
> offset
> were directly related and that it would've been hard to change the
> DRAM
> start address with the MMINFRA offset being -2G, but it's not, so
> doing it
> my way will eventually backfire on us.
> 
> So my way is not good, as it's not showing the reality of things.
> 
> Just go with your current way, as it's really tied to the hardware
> and it's
> not restricted to that dram start coincidence in the end. That's a
> pity.
> 
> Just instead of writing 0x80000000, use " SZ_2G " instead... and
> please
> add a comment in the code that explains in brief that there's this
> strange
> behavior for which we.. need that, and that's a static subtraction,
> and is
> tied to the MMINFRA hardware, and cannot be changed :-(
> 
> btw, being clear..
> 
> #include <linux/sizes.h>
> 
> .mminfra_offset = SZ_2G,
> 
> ..that way you don't even need a comment saying /* 2GB */ ....
> 
> Cheers!

Got it. I'll modify it.
Thanks for the confirmation.

Regards,
Jason-JH Lin