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

["Jianjun Wang (王建军)" <Jianjun.Wang@mediatek.com>]

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.

Thanks.
> 
> 
> -- 
> Regards,
> Alexandre