Re: [PATCH] PCI: mediatek-gen3: Fix translation window

[Alexandre Mergnat]

On 12/10/2023 14:52, AngeloGioacchino Del Regno wrote:
> Il 12/10/23 12:27, Alexandre Mergnat ha scritto:
> > On 12/10/2023 08:17, Jianjun Wang (王建军) wrote:
> > > On Wed, 2023-10-11 at 17:38 +0200, Alexandre Mergnat wrote:
> > > > External email : Please do not click links or open attachments until
> > > > you have verified the sender or the content.
> > > >
> > > >
> > > > On 11/10/2023 14:26, Jianjun Wang wrote:
> > > > > The size of translation table should be a power of 2, using fls()
> > > > cannot > get the proper value when the size is not a power of 2. For
> > > > example, > fls(0x3e00000) - 1 = 25, hence the PCIe translation 
> > > > window size
> > > > will be > set to 0x2000000 instead of the expected size 0x3e00000. Fix
> > > > translation > window by splitting the MMIO space to multiple tables 
> > > > if its size
> > > > is not > a power of 2.
> > > >
> > > > Hi Jianjun,
> > > >
> > > > I've no knowledge in PCIE, so maybe what my suggestion is stupid:
> > > >
> > > > Is it mandatory to fit the translation table size with 0x3e00000 (in 
> > > > this example) ?
> > > > I'm asking because you can have an issue by reaching the maximum 
> > > > translation table number.
> > > >
> > > > Is it possible to just use only one table with the power of 2 size
> > > > above 0x3e00000 => 0x4000000 ( fls(0x3e00000) = 26 = 0x4000000). The
> > > > downside of this method is wasting allocation space. AFAIK I already 
> > > > see this kind of method for memory protection/allocation in embedded 
> > > > systems,
> > > > so I'm wondering if this method is safer than using multiple table for
> > > > only one size which isn't a power of 2.
> > >
> > > Hi Alexandre,
> > >
> > > It's not mandatory to fit the translation table size with 0x3e00000,
> > > and yes we can use only one table with the power of 2 size to prevent
> > > this.
> > >
> > > For MediaTek's SoCs, the MMIO space range for each PCIe port is fixed,
> > > and it will always be a power of 2, most of them will be 64MB. The
> > > reason we have the size which isn't a power of 2 is that we reserve an
> > > IO space for compatible purpose, some older devices may still use IO
> > > space.
> > >
> > > Take MT8195 as an example, its MMIO size is 64MB, and the declaration
> > > in the DT is like:
> > > ranges = <0x81000000 0 0x20000000 0x0 0x20000000 0 0x200000>,
> > >           <0x82000000 0 0x20200000 0x0 0x20200000 0 0x3e00000>;
> > >
> > > The MMIO space is splited to 2MB IO space and 62MB MEM space, that's
> > > cause the current risk of the MEM space range, its actual available MEM
> > > space is 32MB. But it still works for now because most of the devices
> > > only require a very small amount of MEM space and will not reach ranges
> > > higher than 32MB.
> > >
> > > So for the concern of reaching the maximum translation table number, I
> > > think maybe we can just print the warning message instead of return
> > > error code, since it still works but have some limitations(MEM space
> > > not set as DT expected).
> >
> > Ok understood, thanks for your explanation.
> > Then, IMHO, you should use only one table with the power of 2 size 
> > above to make the code simpler, efficient, robust, more readable and 
> > avoid confusion about the warning.
> >
> > This is what is done for pci-mvebu.c AFAII.
> >
> > If you prefer waiting another reviewer with a better PCIE expertise 
> > than me, it's ok for me. With the information I have currently, I 
> > prefer to not approve the current implementation because, from my PoV, 
> > it introduce unnecessary complexity.
>
>  From what I understand, using only one table with a size that is a 
> power of two
> won't let us use the entire MMIO space, hence the only solution to allow 
> using
> the entire range is to split to more than one table.

You can take the power of 2 above, which is directly returned by fls().
That let us use the entire MMIO space.
In this example, if your size is 0x3e00000, the you will allow 0x4000000.


--
Regards,
Alexandre