Re: [PATCH v2 02/18] PCI: endpoint: Introduce pci_epc_map_align()

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Wed, Apr 03, 2024 at 04:54:32PM +0900, Damien Le Moal wrote:
> On 4/3/24 16:45, Manivannan Sadhasivam wrote:
> > On Sat, Mar 30, 2024 at 01:19:12PM +0900, Damien Le Moal wrote:
> >> Some endpoint controllers have requirements on the alignment of the
> >> controller physical memory address that must be used to map a RC PCI
> >> address region. For instance, the rockchip endpoint controller uses
> >> at most the lower 20 bits of a physical memory address region as the
> >> lower bits of an RC PCI address. For mapping a PCI address region of
> >> size bytes starting from pci_addr, the exact number of address bits
> >> used is the number of address bits changing in the address range
> >> [pci_addr..pci_addr + size - 1].
> >>
> >> For this example, this creates the following constraints:
> >> 1) The offset into the controller physical memory allocated for a
> >>    mapping depends on the mapping size *and* the starting PCI address
> >>    for the mapping.
> >> 2) A mapping size cannot exceed the controller windows size (1MB) minus
> >>    the offset needed into the allocated physical memory, which can end
> >>    up being a smaller size than the desired mapping size.
> >>
> >> Handling these constraints independently of the controller being used in
> >> a PCI EP function driver is not possible with the current EPC API as
> >> it only provides the ->align field in struct pci_epc_features.
> >> Furthermore, this alignment is static and does not depend on a mapping
> >> pci address and size.
> >>
> >> Solve this by introducing the function pci_epc_map_align() and the
> >> endpoint controller operation ->map_align to allow endpoint function
> >> drivers to obtain the size and the offset into a controller address
> >> region that must be used to map an RC PCI address region. The size
> >> of the physical address region provided by pci_epc_map_align() can then
> >> be used as the size argument for the function pci_epc_mem_alloc_addr().
> >> The offset into the allocated controller memory can be used to
> >> correctly handle data transfers. Of note is that pci_epc_map_align() may
> >> indicate upon return a mapping size that is smaller (but not 0) than the
> >> requested PCI address region size. For such case, an endpoint function
> >> driver must handle data transfers in fragments.
> >>
> > 
> > Is there any incentive in exposing pci_epc_map_align()? I mean, why can't it be
> > hidden inside the new alloc() API itself?
> 
> I could drop pci_epc_map_align(), but the idea here was to have an API that is
> not restrictive. E.g., a function driver could allocate memory, keep it and
> repetedly use map_align and map() function to remap it to different PCI
> addresses. With your suggestion, that would not be possible.
> 

Is there any requirement currently? If not, let's try to introduce it when the
actual requirement comes.

> > 
> > Furthermore, is it possible to avoid the map_align() callback and handle the
> > alignment within the EPC driver?
> 
> I am not so sure that this is possible because handling the alignment can
> potentially result in changing the amount of memory to allocate, based on the
> PCI address also. So the allocation API would need to change, a lot.
> 

Hmm, looking at patch 11/18, I think it might become complicated.

- Mani

> >> +	/*
> >> +	 * Assume a fixed alignment constraint as specified by the controller
> >> +	 * features.
> >> +	 */
> >> +	features = pci_epc_get_features(epc, func_no, vfunc_no);
> >> +	if (!features || !features->align) {
> >> +		map->map_pci_addr = pci_addr;
> >> +		map->map_size = size;
> >> +		map->map_ofst = 0;
> > 
> > These values are overwritten anyway below.
> 
> Looks like "return" got dropped. Bug. Will re-add it.
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 

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