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.
>
> 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.
>> + /*
>> + * 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
