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

[Alexandre Mergnat]

On 13/10/2023 04:52, Jianjun Wang (王建军) wrote:
> On Thu, 2023-10-12 at 15:30 +0200, Alexandre Mergnat wrote:
> >   
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >  
> >
> > 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.
>
> Take the power of 2 above size is a solution, but another concern will
> be the flexibility. With this patch, we can split the MMIO space to
> multiple ranges like:
> ranges = <0x82000000 0 0x20000000 0x0 0x20000000 0 0x100000>,
>           <0x81000000 0 0x20100000 0x0 0x20100000 0 0x300000>,
>           <0x82000000 0 0x20300000 0x0 0x20300000 0 0x3c00000>;
> Not sure if that can really happen, but it will have overlap ranges
> when take the power of 2 above.

Yes, you can avoid overlap by changing the next start address to fit the 
previous allocated range. If that isn't possible or introduce too much 
complexity compared to your solution, then your implementation could be 
the best from my PoV. :)


--
Regards,
Alexandre