Re: [PATCH v3 04/12] PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()

[Damien Le Moal <dlemoal@kernel.org>]

On 10/12/24 18:31, Manivannan Sadhasivam wrote:
> On Thu, Oct 10, 2024 at 12:43:57PM +0530, Manivannan Sadhasivam wrote:
>> On Mon, Oct 07, 2024 at 01:12:10PM +0900, Damien Le Moal wrote:
>>> Add a check to verify that the outbound region to be used for mapping an
>>> address is not already in use.
>>>
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
> 
> I'm trying to understand the ob window mapping here. So if rockchip_ob_region()
> returns same index for different *CPU* addresses, then you cannot map both of
> them? Is this a hardware ATU limitation?

The CPU addresses mapped are (under normal use) addresses from one of the 32 1MB
memory windows that pci_epc_alloc_addr() will give us. To map a memory window
address, we use the ATU entry at the index given by:

static inline u32 rockchip_ob_region(phys_addr_t addr)
{
        return (addr >> ilog2(SZ_1M)) & 0x1f;
}

So each memory window always use the same ATU entry and addresses from different
windows cannot use the same entry, ever.

But if there is a bug in the endpoint driver and map() or unmap() are called
with an invalid address (e.g. one that is not from a memory window), we will
still get a valid ATU entry number for that address. Hence the check that the
address passed to unmap is the one that is actually mapped, and also why we
check that the entry for an address to map is not being used.

> Also rockchip_pcie_prog_ep_ob_atu() is not taking into account of the cpu_addr.
> So I'm not sure how the mapping happens either.

Each ATU entry is for the corresponding memory window at the same index in the
controller memory. So the cpu address is not needed when programming the ATU as
it is implied from the entry we are programming.

So we could remove the cpu_addr argument of this function.

I also suspect that we potentially could play games with the .align_addr() to
assume a fixed 1MB alignment constraint for a PCI address mapping and always
pass 20 to the num_bits field of the ADDR0 register. However, I have not tried that.

-- 
Damien Le Moal
Western Digital Research