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
>
--
மணிவண்ணன் சதாசிவம்
