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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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.

I'm not sure, though, whether PCIe devices would be able to use a MEM space that
is not power of two, or if those do even exist.

If there are devices that can use 32MB < mem <= 62MB, then I completely agree
with Jianjun on this commit.... so please, can any PCI(/e) maintainer comment
on this situation?

Regards,
Angelo