On Fri, Apr 05, 2024 at 09:43:42PM +0900, Damien Le Moal wrote: > On 4/5/24 21:20, Niklas Cassel wrote: > > > > Now I understand that rockchip is the first one that does not have a fixed > > alignment. > > So for that platform, map_align() will be different from ep->page_size. > > (For all DWC based drivers the outbound iATU alignment requirement is > > the same as the page size.) > > Yes. So we can have a generic map_align() implementation that all these drivers > can use as there .map_align method. No need to expose page size to the epc/epf > core code. I don't follow. ep->page_size is used by pci_epc_multi_mem_init(), pci_epc_mem_alloc_addr() and pci_epc_mem_free_addr(), so it is used by EPC core. pci_epc_mem_alloc_addr() currently uses it to align up an allocation to the page size, so that an allocation from the PCI window/memory space is a multiple of page_size. How can we avoid exposing the page size to EPC core? For a DWC-based driver, the mapping part requires that the start address of the mapping should be aligned to the page size. But e.g. drivers/pci/controller/pcie-rockchip-ep.c, sets page size (smallest allocation): to 1 MB: windows[i].page_size = SZ_1M; But the mapping part for rockchip-ep appears to have different requirements. > > > However, it would be nice if: > > 1) We could have a default implementation of map_align() that by default uses > > ep->page_size. Platforms that have non-fixed alignment requirements could > > define their own map_align(). > > See above. The default implementation can be a helper function defined in epc > core that the drivers can use for their .map_align() method. Sounds good. > > 2) We fix dw_pcie_ep_raise_msi_irq() and dw_pcie_ep_raise_msix_irq() to use > > the new pci_epc_map_align(). > > Why ? That is completely internal to the controller driver. Well, dw_pcie_ep_raise_msi_irq() and dw_pcie_ep_raise_msix_irq() currently does some clearing of lower bits of the address before using .map_addr() to map the an aligned outbound address. It will then write to the mapped address + some offset. Since it is already using .map_addr(), it seems like the perfect use case for pci_epc_map_align(), as the function then would not need to do any clearing of lower bits before mapping, nor writing to an offset within the mapping. It would just use pci_epc_map_align() and then write to map->pci_addr. > > 3) It is getting too complicated with all these... > > epc_features->align, ep->page_size, map_align(), and .alignment in host driver. > > I think that we need to document each of these in Documentation/PCI/endpoint/ > > test host driver .alignment needs to be nuked. That one is nonsense. > ep->page_size needs to stay internal to the driver. .map_align method is enough > to handle any PCI address mapping constraint and will indicate memory size to > allocate, offset into it etc. And for the BARs alignment, .align feature is not > exactly great as it is not clear, but it is enough I think. So we could just > rename it to be clear. And even maybe remove it from features. I do not see why > an EPF needs to care about it given that epc core funstions are used to setup > the bars. +1 on renaming it to bar_alignment or inbound_alignment or similar. I don't think that we can remove it from epc_features. It is used by pci_epf_alloc_space() which uses epc_features->align to ensure that when an EPF driver allocates the backing memory for a BAR, the backing memory is aligned to bar_alignment. (An allocation of size X is guaranteed to be aligned to X.) > > 4) It would be nice if we could set page_size correctly for all the PCI device > > and vendor IDs that have defined an .alignment in drivers/misc/pci_endpoint_test.c > > in the correct EPC driver. That way, we should be able to completely remove all > > .alignment specified in drivers/misc/pci_endpoint_test.c. > > The host side should be allowed to use any PCI address alignment it wants. So no > alignment communicated at all. It is the EP side that needs to deal with alignment. I think we are saying the same thing. But in order to remove all .alignment uses in drivers/misc/pci_endpoint_test.c, we will need to add modify the corresponding EPC driver to either: - Define the ep->page_size, so that the generic map_align() implementation will work. (If you grep for page_size in drivers/pci/controller, you will see that very few EPC drivers currently set ep->page_size, even though the PCI device and vendor IDs for those same controllers have specified an alignment in drivers/misc/pci_endpoint_test.c) - Define a custom map_align() implementation. > > 5) Unfortunately drivers/misc/pci_endpoint_test.c defines a default alignment > > of 4K: > > https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/misc/pci_endpoint_test.c#L968 > > https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/misc/pci_endpoint_test.c#L820 > > > > It would be nice if we could get rid of this as well. Or perhaps add an option > > to pci_test so that it does not use this 4k alignment, such that we can verify > > that pci_epc_map_align() is actually working. > > Exactly. Get rid of any default alignment, add a test parameter to define one so > that we can test different alignment+size combinations. > > > In my opinion 4) is the biggest win with this series, because it means that > > we define the alignment in the EPC driver, instead of needing to define it in > > each and every host side driver. But right now, this great improvement is not > > really visible for someone looking quickly at the current series. > > Yes. Once in place, we can rework the test driver alignment stuff to make it > optional instead of mandatory because of bad handling on the EP side :) Perhaps it would be nice to have 5) implemented for this initial series, so that it is possible to test that this new API is behaving as intended? Kind regards, Niklas
