[v2,00/18] Improve PCI memory mapping API

This series introduces the new functions pci_epc_map_align(),
pci_epc_mem_map() and pci_epc_mem_unmap() to improve handling of the
PCI address mapping alignment constraints of endpoint controllers in a
controller independent manner.

The issue fixed is that the fixed alignment defined by the "align" field
of struct pci_epc_features assumes that the alignment of the endpoint
memory used to map a RC PCI address range is independent of the PCI
address being mapped. But that is not the case for the rk3399 SoC
controller: in endpoint mode, this controller uses the lower bits of the
local endpoint memory address as the lower bits for the PCI addresses
for data transfers. That is, when mapping local memory, one must take
into account the number of bits of the RC PCI address that change from
the start address of the mapping.

To fix this, the new endpoint controller method .map_align is introduced
and called from pci_epc_map_align(). This method is optional and for
controllers that do not define it, the mapping information returned
is based of the fixed alignment constraint as defined by the align
feature.

The functions pci_epc_mem_map() is a helper function which obtains
mapping information, allocates endpoint controller memory according to
the mapping size obtained and maps the memory. pci_epc_mem_map() unmaps
and frees the endpoint memory.

This series is organized as follows:
 - Patch 1 tidy up the epc core code
 - Patch 2 and 3 introduce the new map_align endpoint controller method
   and related epc functions.
 - Patch 4 to 6 modify the test endpoint driver to use these new
   functions and improve the code of this driver.
 - Finally, Patch 7 to 18 fix the rk3399 endpoint driver, defining a
   .map_align method for it and improving its overall code readability
   and features.

Changes from v1:
 - Changed pci_epc_check_func() to pci_epc_function_is_valid() in patch
   1.
 - Removed patch "PCI: endpoint: Improve pci_epc_mem_alloc_addr()"
   (former patch 2 of v1)
 - Various typos cleanups all over. Also fixed some blank space
   indentation.
 - Added review tags

Damien Le Moal (17):
  PCI: endpoint: Introduce pci_epc_function_is_valid()
  PCI: endpoint: Introduce pci_epc_map_align()
  PCI: endpoint: Introduce pci_epc_mem_map()/unmap()
  PCI: endpoint: test: Use pci_epc_mem_map/unmap()
  PCI: endpoint: test: Synchronously cancel command handler work
  PCI: endpoint: test: Implement link_down event operation
  PCI: rockchip-ep: Fix address translation unit programming
  PCI: rockchip-ep: Use a macro to define EP controller .align feature
  PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()
  PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()
  PCI: rockchip-ep: Implement the map_align endpoint controller operation
  PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations
  PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding
  PCI: rockchip-ep: Refactor endpoint link training enable
  PCI: rockship-ep: Introduce rockchip_pcie_ep_stop()
  PCI: rockchip-ep: Improve link training
  PCI: rockchip-ep: Handle PERST# signal in endpoint mode

Wilfred Mallawa (1):
  dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property

 .../bindings/pci/rockchip,rk3399-pcie-ep.yaml |   3 +
 drivers/pci/controller/pcie-rockchip-ep.c     | 393 ++++++++++++++----
 drivers/pci/controller/pcie-rockchip.c        |  17 +-
 drivers/pci/controller/pcie-rockchip.h        |  22 +
 drivers/pci/endpoint/functions/pci-epf-test.c | 390 +++++++++--------
 drivers/pci/endpoint/pci-epc-core.c           | 213 +++++++---
 include/linux/pci-epc.h                       |  39 ++
 7 files changed, 768 insertions(+), 309 deletions(-)

On Sat, Mar 30, 2024 at 5:19 AM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> This series introduces the new functions pci_epc_map_align(),
> pci_epc_mem_map() and pci_epc_mem_unmap() to improve handling of the
> PCI address mapping alignment constraints of endpoint controllers in a
> controller independent manner.
>
> The issue fixed is that the fixed alignment defined by the "align" field
> of struct pci_epc_features assumes that the alignment of the endpoint
> memory used to map a RC PCI address range is independent of the PCI
> address being mapped. But that is not the case for the rk3399 SoC
> controller: in endpoint mode, this controller uses the lower bits of the
> local endpoint memory address as the lower bits for the PCI addresses
> for data transfers. That is, when mapping local memory, one must take
> into account the number of bits of the RC PCI address that change from
> the start address of the mapping.
>
> To fix this, the new endpoint controller method .map_align is introduced
> and called from pci_epc_map_align(). This method is optional and for
> controllers that do not define it, the mapping information returned
> is based of the fixed alignment constraint as defined by the align
> feature.
>
> The functions pci_epc_mem_map() is a helper function which obtains
> mapping information, allocates endpoint controller memory according to
> the mapping size obtained and maps the memory. pci_epc_mem_map() unmaps
> and frees the endpoint memory.

This way of mapping is not only useful for the RK3399 but would also
help for the addition of other future PCI endpoint controller drivers.

For example, on several FPGA PCI endpoint IPs the window mapping is
also done by passing N bits from the mapped address and M bits from
the window mapping address (where N+M=bus width, e.g., 32 or 64).
Using AND/OR masks/operations to combine the bits for the hardware
address from the mapped address and map base uses less resources than
using add/subtract to get the hardware address from an unaligned map
base and offset. So I guess that more than a few IPs, being hard or
soft IPs, use this kind of mapping (to reduce size, logic, improve max
operating frequency, improve efficiency etc.)

Two major examples come to mind :
1) The AMD/Xilinx PCIe endpoint IP. The mapping is documentented in
"AXI Bridge for PCI Express Gen3 Subsystem Product Guide (PG194)" [1]
section BAR and Address Translation (Figure AXI to PCIe Address
Translation).
2) The Intel/Altera PCIe endpoint IP. The mapping is documented in
"Multi Channel DMA Intel® FPGA IP for PCI Express* User Guide" [2]
section 3.6. Root Port Address Translation Table Enablement.

Both those IPs don't have mainline support yet as PCIe endpoint
controllers but also use a similar kind of mapping as suggested here
for the RK3399. So these changes would also make the addition of these
controller drivers easier.

The new mapping scheme also makes it much clearer in the PCI endpoint
framework. Because without it some mapping operation would fail
because of alignment requirements in the controller, this requires
extra code and checks in the drivers that implement the endpoint
functions. With the current state of the PCI endpoint controller
framework there is no good way to express that the controller does an
AND/OR mask combination to create the hardware address and therefore
requires the map to be aligned to the window size, rather than doing a
window base addition with an offset (subtraction) in the mapping. This
could benefit from further clarification in the endpoint framework.

Best regards,
Rick

[1] https://docs.amd.com/r/en-US/pg194-axi-bridge-pcie-gen3/Address-Translation
[2] https://www.intel.com/content/www/us/en/docs/programmable/683821/23-4/

>
> This series is organized as follows:
>  - Patch 1 tidy up the epc core code
>  - Patch 2 and 3 introduce the new map_align endpoint controller method
>    and related epc functions.
>  - Patch 4 to 6 modify the test endpoint driver to use these new
>    functions and improve the code of this driver.
>  - Finally, Patch 7 to 18 fix the rk3399 endpoint driver, defining a
>    .map_align method for it and improving its overall code readability
>    and features.
>
> Changes from v1:
>  - Changed pci_epc_check_func() to pci_epc_function_is_valid() in patch
>    1.
>  - Removed patch "PCI: endpoint: Improve pci_epc_mem_alloc_addr()"
>    (former patch 2 of v1)
>  - Various typos cleanups all over. Also fixed some blank space
>    indentation.
>  - Added review tags
>
> Damien Le Moal (17):
>   PCI: endpoint: Introduce pci_epc_function_is_valid()
>   PCI: endpoint: Introduce pci_epc_map_align()
>   PCI: endpoint: Introduce pci_epc_mem_map()/unmap()
>   PCI: endpoint: test: Use pci_epc_mem_map/unmap()
>   PCI: endpoint: test: Synchronously cancel command handler work
>   PCI: endpoint: test: Implement link_down event operation
>   PCI: rockchip-ep: Fix address translation unit programming
>   PCI: rockchip-ep: Use a macro to define EP controller .align feature
>   PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()
>   PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()
>   PCI: rockchip-ep: Implement the map_align endpoint controller operation
>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations
>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding
>   PCI: rockchip-ep: Refactor endpoint link training enable
>   PCI: rockship-ep: Introduce rockchip_pcie_ep_stop()
>   PCI: rockchip-ep: Improve link training
>   PCI: rockchip-ep: Handle PERST# signal in endpoint mode
>
> Wilfred Mallawa (1):
>   dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property
>
>  .../bindings/pci/rockchip,rk3399-pcie-ep.yaml |   3 +
>  drivers/pci/controller/pcie-rockchip-ep.c     | 393 ++++++++++++++----
>  drivers/pci/controller/pcie-rockchip.c        |  17 +-
>  drivers/pci/controller/pcie-rockchip.h        |  22 +
>  drivers/pci/endpoint/functions/pci-epf-test.c | 390 +++++++++--------
>  drivers/pci/endpoint/pci-epc-core.c           | 213 +++++++---
>  include/linux/pci-epc.h                       |  39 ++
>  7 files changed, 768 insertions(+), 309 deletions(-)
>
> --
> 2.44.0
>

On Sat, Mar 30, 2024 at 01:19:10PM +0900, Damien Le Moal wrote:
> This series introduces the new functions pci_epc_map_align(),
> pci_epc_mem_map() and pci_epc_mem_unmap() to improve handling of the
> PCI address mapping alignment constraints of endpoint controllers in a
> controller independent manner.
> 
> The issue fixed is that the fixed alignment defined by the "align" field
> of struct pci_epc_features assumes that the alignment of the endpoint
> memory used to map a RC PCI address range is independent of the PCI
> address being mapped. But that is not the case for the rk3399 SoC
> controller: in endpoint mode, this controller uses the lower bits of the
> local endpoint memory address as the lower bits for the PCI addresses
> for data transfers. That is, when mapping local memory, one must take
> into account the number of bits of the RC PCI address that change from
> the start address of the mapping.
> 
> To fix this, the new endpoint controller method .map_align is introduced
> and called from pci_epc_map_align(). This method is optional and for
> controllers that do not define it, the mapping information returned
> is based of the fixed alignment constraint as defined by the align
> feature.
> 
> The functions pci_epc_mem_map() is a helper function which obtains
> mapping information, allocates endpoint controller memory according to
> the mapping size obtained and maps the memory. pci_epc_mem_map() unmaps
> and frees the endpoint memory.
> 
> This series is organized as follows:
>  - Patch 1 tidy up the epc core code
>  - Patch 2 and 3 introduce the new map_align endpoint controller method
>    and related epc functions.
>  - Patch 4 to 6 modify the test endpoint driver to use these new
>    functions and improve the code of this driver.

While posting the next version, please split the endpoint patches into a
separate series. It helps in code review and can be applied separately.

- Mani

>  - Finally, Patch 7 to 18 fix the rk3399 endpoint driver, defining a
>    .map_align method for it and improving its overall code readability
>    and features.
> 
> Changes from v1:
>  - Changed pci_epc_check_func() to pci_epc_function_is_valid() in patch
>    1.
>  - Removed patch "PCI: endpoint: Improve pci_epc_mem_alloc_addr()"
>    (former patch 2 of v1)
>  - Various typos cleanups all over. Also fixed some blank space
>    indentation.
>  - Added review tags
> 
> Damien Le Moal (17):
>   PCI: endpoint: Introduce pci_epc_function_is_valid()
>   PCI: endpoint: Introduce pci_epc_map_align()
>   PCI: endpoint: Introduce pci_epc_mem_map()/unmap()
>   PCI: endpoint: test: Use pci_epc_mem_map/unmap()
>   PCI: endpoint: test: Synchronously cancel command handler work
>   PCI: endpoint: test: Implement link_down event operation
>   PCI: rockchip-ep: Fix address translation unit programming
>   PCI: rockchip-ep: Use a macro to define EP controller .align feature
>   PCI: rockchip-ep: Improve rockchip_pcie_ep_unmap_addr()
>   PCI: rockchip-ep: Improve rockchip_pcie_ep_map_addr()
>   PCI: rockchip-ep: Implement the map_align endpoint controller operation
>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() memory allocations
>   PCI: rockchip-ep: Refactor rockchip_pcie_ep_probe() MSI-X hiding
>   PCI: rockchip-ep: Refactor endpoint link training enable
>   PCI: rockship-ep: Introduce rockchip_pcie_ep_stop()
>   PCI: rockchip-ep: Improve link training
>   PCI: rockchip-ep: Handle PERST# signal in endpoint mode
> 
> Wilfred Mallawa (1):
>   dt-bindings: pci: rockchip,rk3399-pcie-ep: Add ep-gpios property
> 
>  .../bindings/pci/rockchip,rk3399-pcie-ep.yaml |   3 +
>  drivers/pci/controller/pcie-rockchip-ep.c     | 393 ++++++++++++++----
>  drivers/pci/controller/pcie-rockchip.c        |  17 +-
>  drivers/pci/controller/pcie-rockchip.h        |  22 +
>  drivers/pci/endpoint/functions/pci-epf-test.c | 390 +++++++++--------
>  drivers/pci/endpoint/pci-epc-core.c           | 213 +++++++---
>  include/linux/pci-epc.h                       |  39 ++
>  7 files changed, 768 insertions(+), 309 deletions(-)
> 
> -- 
> 2.44.0
>

On 4/3/24 16:50, Manivannan Sadhasivam wrote:
> On Sat, Mar 30, 2024 at 01:19:10PM +0900, Damien Le Moal wrote:
>> This series introduces the new functions pci_epc_map_align(),
>> pci_epc_mem_map() and pci_epc_mem_unmap() to improve handling of the
>> PCI address mapping alignment constraints of endpoint controllers in a
>> controller independent manner.
>>
>> The issue fixed is that the fixed alignment defined by the "align" field
>> of struct pci_epc_features assumes that the alignment of the endpoint
>> memory used to map a RC PCI address range is independent of the PCI
>> address being mapped. But that is not the case for the rk3399 SoC
>> controller: in endpoint mode, this controller uses the lower bits of the
>> local endpoint memory address as the lower bits for the PCI addresses
>> for data transfers. That is, when mapping local memory, one must take
>> into account the number of bits of the RC PCI address that change from
>> the start address of the mapping.
>>
>> To fix this, the new endpoint controller method .map_align is introduced
>> and called from pci_epc_map_align(). This method is optional and for
>> controllers that do not define it, the mapping information returned
>> is based of the fixed alignment constraint as defined by the align
>> feature.
>>
>> The functions pci_epc_mem_map() is a helper function which obtains
>> mapping information, allocates endpoint controller memory according to
>> the mapping size obtained and maps the memory. pci_epc_mem_map() unmaps
>> and frees the endpoint memory.
>>
>> This series is organized as follows:
>>  - Patch 1 tidy up the epc core code
>>  - Patch 2 and 3 introduce the new map_align endpoint controller method
>>    and related epc functions.
>>  - Patch 4 to 6 modify the test endpoint driver to use these new
>>    functions and improve the code of this driver.
> 
> While posting the next version, please split the endpoint patches into a
> separate series. It helps in code review and can be applied separately.

Which patches ? They are all endpoint related:
 (1) Core code
 (2) test function driver
 (3) rockchip rk3399 controller

(2) and (3) depend on the patches in (1), so splitting the series is a big
possible only if (1) is applied first, so that is a source of delays and breaks
the context of the patches...

On Wed, Apr 03, 2024 at 04:58:42PM +0900, Damien Le Moal wrote:
> On 4/3/24 16:50, Manivannan Sadhasivam wrote:
> > On Sat, Mar 30, 2024 at 01:19:10PM +0900, Damien Le Moal wrote:
> >> This series introduces the new functions pci_epc_map_align(),
> >> pci_epc_mem_map() and pci_epc_mem_unmap() to improve handling of the
> >> PCI address mapping alignment constraints of endpoint controllers in a
> >> controller independent manner.
> >>
> >> The issue fixed is that the fixed alignment defined by the "align" field
> >> of struct pci_epc_features assumes that the alignment of the endpoint
> >> memory used to map a RC PCI address range is independent of the PCI
> >> address being mapped. But that is not the case for the rk3399 SoC
> >> controller: in endpoint mode, this controller uses the lower bits of the
> >> local endpoint memory address as the lower bits for the PCI addresses
> >> for data transfers. That is, when mapping local memory, one must take
> >> into account the number of bits of the RC PCI address that change from
> >> the start address of the mapping.
> >>
> >> To fix this, the new endpoint controller method .map_align is introduced
> >> and called from pci_epc_map_align(). This method is optional and for
> >> controllers that do not define it, the mapping information returned
> >> is based of the fixed alignment constraint as defined by the align
> >> feature.
> >>
> >> The functions pci_epc_mem_map() is a helper function which obtains
> >> mapping information, allocates endpoint controller memory according to
> >> the mapping size obtained and maps the memory. pci_epc_mem_map() unmaps
> >> and frees the endpoint memory.
> >>
> >> This series is organized as follows:
> >>  - Patch 1 tidy up the epc core code
> >>  - Patch 2 and 3 introduce the new map_align endpoint controller method
> >>    and related epc functions.
> >>  - Patch 4 to 6 modify the test endpoint driver to use these new
> >>    functions and improve the code of this driver.
> > 
> > While posting the next version, please split the endpoint patches into a
> > separate series. It helps in code review and can be applied separately.
> 
> Which patches ? They are all endpoint related:
>  (1) Core code
>  (2) test function driver

Till patch 6, that's why I inlined my reply at the 3rd point.

>  (3) rockchip rk3399 controller
> 
> (2) and (3) depend on the patches in (1), so splitting the series is a big
> possible only if (1) is applied first, so that is a source of delays and breaks
> the context of the patches...
> 

If you split patches 1-6 and post the rest of the Rockchip patches as a follow
up, it perfectly makes sense.

- Mani