Re: [PATCH v4 3/7] PCI: endpoint: Introduce pci_epc_map_align()

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

On Fri, Oct 11, 2024 at 10:07:30AM +0900, Damien Le Moal wrote:
> On 10/10/24 23:36, Manivannan Sadhasivam wrote:
> > On Mon, Oct 07, 2024 at 01:03:15PM +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 an endpoint function driver is not possible with the current EPC
> >> API as only the ->align field in struct pci_epc_features is provided
> >> and used for BAR (inbound ATU mappings) mapping. A new API is needed
> >> for function drivers to discover mapping constraints and handle
> >> non-static requirements based on the RC PCI address range to access.
> >>
> >> Introduce 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
> >> allocated and mapped to access an RC PCI address region. The size
> >> of the mapping 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 provided can be used to
> >> correctly handle data transfers.
> >>
> >> For endpoint controllers that have PCI address alignment constraints,
> >> pci_epc_map_align() may indicate upon return an effective PCI address
> >> region 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 accesses over the desired PCI address range in fragments,
> >> by repeatedly using pci_epc_map_align() over the PCI address range.
> >>
> >> The controller operation ->map_align is optional: controllers that do
> >> not have any alignment constraints for mapping a RC PCI address region
> >> do not need to implement this operation. For such controllers,
> >> pci_epc_map_align() always returns the mapping size as equal to the
> >> requested size of the PCI region and an offset equal to 0.
> >>
> >> The new structure struct pci_epc_map is introduced to represent a
> >> mapping start PCI address, mapping effective size, the size and offset
> >> into the controller memory needed for mapping the PCI address region as
> >> well as the physical and virtual CPU addresses of the mapping (phys_base
> >> and virt_base fields). For convenience, the physical and virtual CPU
> >> addresses within that mapping to access the target RC PCI address region
> >> are also provided (phys_addr and virt_addr fields).
> >>
> > 
> > I'm fine with the concept of this patch, but I don't get why you need an API for
> > this and not just a callback to be used in the pci_epc_mem_{map/unmap} APIs.
> > Furthermore, I don't see an user of this API (in 3 series you've sent out so
> > far). Let me know if I failed to spot it.
> > 
> > Also, the API name pci_epc_map_align() sounds like it does the mapping, but it
> > doesn't. So I'd not have it exposed as an API at all.
> 
> OK. Fine with me. I will move this inside pci_epc_mem_map(). But note that
> without this function, pci_epc_mem_alloc_addr() and pci_epc_map_addr() are
> totally useless for EP controllers that have a mapping alignment requirement,
> which without the pci_epc_map_align() function, an endpoint function driver
> cannot discover *at all* currently. That does not fix the overall API of EPC...
> 

Not at all. EPF drivers still can use "epf_mhi->epc_features->align" to discover
the alignment requirement and calculate the offset on their own (please see
pci-epf-mhi). But I'm not in favor of that approach since the APIs need to do
that job and that's why I like your pci_epc_mem_map() API.

> By not having pci_epc_map_align(), pci_epc_mem_alloc_addr() and
> pci_epc_map_addr() remain broken, but the introduction of pci_epc_mem_map() does
> provide a working solution for the general case.
> 
> So I think we will still need to do something about this bad state of the API later.
> 

We can always rework the APIs to incorporate the alignment requirement.

- Mani

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