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

[Manivannan Sadhasivam]

On Sat, Oct 12, 2024 at 09:02:43PM +0900, Damien Le Moal wrote:
> 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 at kernel.org>
> >>
> >> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam at 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.
> 

Ah okay, thanks a lot for the explanation.

> So we could remove the cpu_addr argument of this function.
> 

yeah, and may be a comment would also help.

> 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.
> 

Ok!

- Mani

-- 
மணிவண்ணன் சதாசிவம்