This is a series of patches that fixes the PCIe endpoint controller driver
for the Rockchip RK3399 SoC. It is based on Linux kernel 6.0.19

The original driver in mainline had issues and would not allow for the
RK3399 to operate in PCIe endpoint mode. This patch series fixes that so
that the PCIe core controller of the RK3399 SoC can now act as a PCIe
endpoint.

This patch series has been tested on kernel 6.0.19 (and 5.19)
on real hardware, a FriendlyElec NanoPC-T4 RK3399 based single computer
board connected to a host computer through PCIe x1 and x4. The PCIe
endpoint test function driver was loaded on the SoC and the PCIe endpoint
test driver was loaded on the host computer. The following tests were
executed through this setup :

* enumeration of the PCIe endpoint device (lspci)
  lspci -vvv
* validation of PCI header and capabilities
  setpci and lspci -xxxx
* device was recognized by host computer dans PCIe endpoint test driver
  was loaded
  lspci -v states "Kernel modules: pci_endpoint_test"
* tested the BARs 0-5
  sudo /sur/bin/pcitest -b 0
  ...
  sudo /usr/bin/pcitest -b 5
* tested legacy interrupt through the test driver
  sudo /usr/bin/pcitest -i 0
  sudo /usr/bin/pcitest -l
* tested MSI interrupt through the test driver
  sudo /usr/bin/pcitest -i 1
  sudo /usr/bin/pcitest -m 1
* tested read/write to and from host through the test driver with checksum
  sudo /usr/bin/pcitest -r -s 1024
  sudo /usr/bin/pcitest -w -s 1024
* tested read/write with DMA enabled (all read/write tests also did IRQ)
  sudo /usr/bin/pcitest -r -d -s 8192
  sudo /usr/bin/pcitest -w -d -s 8192

Summary of changes :

This patch series is composed of 8 patches that do the following :
* Removed writes to unused registers in the PCIe core register space.
  The registers that were written to is marked "unused" and read
  only in the technical reference manual of the RK3399 SoC.
* Fixed setup to the PCI Device ID (DID), this was written to a read
  only register and therefore would not update the DID.
* Fixed setup of the PCIe endpoint controller so that it would stop
  sending Configuration Request Retry Status (CRS) messages to the
  host once configured, without this the host would retry until
  timeout and cancel the PCI configuration.
* Added a poll with timeout to check the PHY PLL lock status, this
  is the only patch that also applies to the root complex function
  of the PCIe core controller, without this the kernel would
  sometimes access registers in the PHY PLL clock domain when the PLLs
  were not yet locked and the system would hang. This was hackily solved
  in other non mainline patches (e.g., in armbian) with a "msleep()"
  that was added after PHY PLL configuration but without realizing
  why it was needed. A poll with timeout seems like a sane approach.
* Added a dtsi entry for the PCIe endpoint controller. The new entry is
  in "disabled" status by default, so unless it is explicitly enabled
  it will not conflict with the PCIe root complex controller entry.
  Developers that will enable it would know that the root complex function
  then must be disabled, this can be done in the board level DTS.
* Fixed the window translation between CPU space and PCI space.
  Allows up to 32 memory windows, with (1MB) page allocation and mapping.
* Fixed the legacy IRQ (INTx) generation of the PCIe core in
  endpoint mode.
* Fixed the generation of message signalled interrupts (MSI) of the
  PCIe core in endpoint mode.

Thank you in advance for reviewing these changes and hopefully
getting this merged. Having a functional PCIe endpoint controller
driver for the RK3399 would allow to develop further PCIe endpoint
functions through the Linux PCIe endpoint framework using this SoC.

I have tested and confirmed all basic functionality required for the
endpoint with the test driver and tools. With the previous state of
the driver the device would not even be enumerated by the host
computer (mainly because of CRS messages being sent back to the root
complex) and tests would not pass (driver would not even be loaded
because DID was not set correctly) and then only the BAR test would
pass. Now all tests pass as stated above.

Best regards
Rick

Commands used on the SoC to launch the endpoint function (configfs) :

modprobe -i pci-epf-test
mkdir -p /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0
echo 0xb500 > /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0/deviceid
echo 0x104c > /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0/vendorid
echo 16 > /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0/msi_interrupts 
ln -s /sys/kernel/config/pci_ep/functions/pci_epf_test/pci_epf_test.0 \
/sys/kernel/config/pci_ep/controllers/fd000000.pcie-ep/
echo 1 > /sys/kernel/config/pci_ep/controllers/fd000000.pcie-ep/start

Note: to enable the endpoint controller on the board the file :
arch/arm64/boot/dts/rockchip/rk3399-nanopc-t4.dts
Was edited to set the status of &pcie0 to "disabled" and &pcie0_ep
to "okay". This is not submitted as a patch because most users
will use the PCIe core controller in host (root complex) mode
rather than endpoint mode.

Rick Wertenbroek (8):
  PCI: rockchip: Removed writes to unused registers
  PCI: rockchip: Fixed setup of Device ID
  PCI: rockchip: Fixed endpoint controller Configuration Request Retry
    Status
  PCI: rockchip: Added poll and timeout to wait for PHY PLLs to be
    locked
  PCI: rockchip: Added dtsi entry for PCIe endpoint controller
  PCI: rockchip: Fixed window mapping and address translation for
    endpoint
  PCI: rockchip: Fixed legacy IRQ generation for endpoint
  PCI: rockchip: Fixed MSI generation from PCIe endpoint core

 arch/arm64/boot/dts/rockchip/rk3399.dtsi  |  25 ++++
 drivers/pci/controller/pcie-rockchip-ep.c | 149 +++++++++++-----------
 drivers/pci/controller/pcie-rockchip.c    |  16 +++
 drivers/pci/controller/pcie-rockchip.h    |  36 ++++--
 4 files changed, 137 insertions(+), 89 deletions(-)

-- 
2.25.1

Hi Rick,

Thanks very much for your work.

On Thu, Jan 26, 2023 at 02:50:40PM +0100, Rick Wertenbroek wrote:
> This is a series of patches that fixes the PCIe endpoint controller driver
> for the Rockchip RK3399 SoC. It is based on Linux kernel 6.0.19
> 
> The original driver in mainline had issues and would not allow for the
> RK3399 to operate in PCIe endpoint mode. This patch series fixes that so
> that the PCIe core controller of the RK3399 SoC can now act as a PCIe
> endpoint.

So we merged cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip
PCIe controller") when it actually didn't work?  Ouch.  Thanks for
fixing it and testing it.

> Rick Wertenbroek (8):
>   PCI: rockchip: Removed writes to unused registers
>   PCI: rockchip: Fixed setup of Device ID
>   PCI: rockchip: Fixed endpoint controller Configuration Request Retry
>     Status
>   PCI: rockchip: Added poll and timeout to wait for PHY PLLs to be
>     locked
>   PCI: rockchip: Added dtsi entry for PCIe endpoint controller
>   PCI: rockchip: Fixed window mapping and address translation for
>     endpoint
>   PCI: rockchip: Fixed legacy IRQ generation for endpoint
>   PCI: rockchip: Fixed MSI generation from PCIe endpoint core

For the next iteration, can you please update these subject lines and
commit logs to:

  - Use imperative mood, i.e., read like a command, instead of a past
    tense description of what was done.  For example, say "Remove
    writes to unused registers" instead of "Removed writes ..."

  - Be more specific when possible.  "Fix" conveys no information
    about the actual code change.  For example, "Fixed endpoint
    controller Configuration Request Retry Status" gives a general
    idea, but it would be more useful if it said something about
    clearing config mode after probe.

  - Say what the patch does in the commit log.  The current ones often
    describe a *problem*, but do not explicitly say what the patch
    does.  The commit log should be complete in itself even without
    the subject line, so it usually contains a slightly expanded
    version of the subject line.

  - Split patches that do more than one logical thing.  The commit log
    for "Fixed MSI generation ..." talks about a u16/u32 shift issue,
    but the patch also adds an unrelated check for multi-function
    devices.

  - If a patch is a fix for an existing issue and may need to be
    backported, identify the commit that introduced the issue and add
    "Fixes: " lines.  This helps distros figure out whether and how
    far to backport patches.

  - Refer to the device consistently.  I see:
      RK3399 PCI EP core
      RK3399 SoC PCIe EP core
      RK3399 PCIe endpoint core
    I vote for "RK3399 PCIe Endpoint core".

Notes about imperative mood:
  https://chris.beams.io/posts/git-commit/
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.0#n94

>  arch/arm64/boot/dts/rockchip/rk3399.dtsi  |  25 ++++
>  drivers/pci/controller/pcie-rockchip-ep.c | 149 +++++++++++-----------
>  drivers/pci/controller/pcie-rockchip.c    |  16 +++
>  drivers/pci/controller/pcie-rockchip.h    |  36 ++++--
>  4 files changed, 137 insertions(+), 89 deletions(-)
> 
> -- 
> 2.25.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hello, Bjorn
Thank you for your prompt reply and helpful comments.

Le jeu. 26 janv. 2023 à 15:52, Bjorn Helgaas <helgaas@kernel.org> a écrit :
>
> Hi Rick,
>
> Thanks very much for your work.
>
> On Thu, Jan 26, 2023 at 02:50:40PM +0100, Rick Wertenbroek wrote:
> > This is a series of patches that fixes the PCIe endpoint controller driver
> > for the Rockchip RK3399 SoC. It is based on Linux kernel 6.0.19
> >
> > The original driver in mainline had issues and would not allow for the
> > RK3399 to operate in PCIe endpoint mode. This patch series fixes that so
> > that the PCIe core controller of the RK3399 SoC can now act as a PCIe
> > endpoint.
>
> So we merged cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip
> PCIe controller") when it actually didn't work?  Ouch.  Thanks for
> fixing it and testing it.

It seems it wasn't fully tested, the code compiles and kernel module loads,
but further functionality didn't seem to have been tested
(e.g., lspci, and with the pcitest tool and pci_endpoit_test_driver).

>
> For the next iteration, can you please update these subject lines and
> commit logs to:

Thank you, I will prepare the changes and add them to the next iteration
with changes from other comments that may arise.

>
>   - Use imperative mood, i.e., read like a command, instead of a past
>     tense description of what was done.  For example, say "Remove
>     writes to unused registers" instead of "Removed writes ..."
>
>   - Be more specific when possible.  "Fix" conveys no information
>     about the actual code change.  For example, "Fixed endpoint
>     controller Configuration Request Retry Status" gives a general
>     idea, but it would be more useful if it said something about
>     clearing config mode after probe.
>
>   - Say what the patch does in the commit log.  The current ones often
>     describe a *problem*, but do not explicitly say what the patch
>     does.  The commit log should be complete in itself even without
>     the subject line, so it usually contains a slightly expanded
>     version of the subject line.
>
>   - Split patches that do more than one logical thing.  The commit log
>     for "Fixed MSI generation ..." talks about a u16/u32 shift issue,
>     but the patch also adds an unrelated check for multi-function
>     devices.

I will. I tried to split everything into the function it was related to, but I
now understand I should split even more so that the commit message
and changes are more tightly linked.

>
>   - If a patch is a fix for an existing issue and may need to be
>     backported, identify the commit that introduced the issue and add
>     "Fixes: " lines.  This helps distros figure out whether and how
>     far to backport patches.

Does this mean I should refer to the commit cf590b078391
("PCI: rockchip: Add EP driver for Rockchip PCIe controller") ?
Because it wasn't working in the first place ?

>
>   - Refer to the device consistently.  I see:
>       RK3399 PCI EP core
>       RK3399 SoC PCIe EP core
>       RK3399 PCIe endpoint core
>     I vote for "RK3399 PCIe Endpoint core".

I agree.

>
> Notes about imperative mood:
>   https://chris.beams.io/posts/git-commit/
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.0#n94

Thank you for all the pointers, I'll take them into account for the
next iteration. This is the first time I actually submitted a series of
patches to the LKML so it's all relatively new to me.

On Thu, Jan 26, 2023 at 04:23:57PM +0100, Rick Wertenbroek wrote:
> Le jeu. 26 janv. 2023 à 15:52, Bjorn Helgaas <helgaas@kernel.org> a écrit :
> > Thanks very much for your work.
> >
> > On Thu, Jan 26, 2023 at 02:50:40PM +0100, Rick Wertenbroek wrote:
> > > This is a series of patches that fixes the PCIe endpoint controller driver
> > > for the Rockchip RK3399 SoC. It is based on Linux kernel 6.0.19
> > >
> > > The original driver in mainline had issues and would not allow for the
> > > RK3399 to operate in PCIe endpoint mode. This patch series fixes that so
> > > that the PCIe core controller of the RK3399 SoC can now act as a PCIe
> > > endpoint.
> >
> > So we merged cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip
> > PCIe controller") when it actually didn't work?  Ouch.  Thanks for
> > fixing it and testing it.
> 
> It seems it wasn't fully tested, the code compiles and kernel module loads,
> but further functionality didn't seem to have been tested
> (e.g., lspci, and with the pcitest tool and pci_endpoit_test_driver).

OK, I guess that happens sometimes.  Glad you're getting it into
shape!

> Does this mean I should refer to the commit cf590b078391
> ("PCI: rockchip: Add EP driver for Rockchip PCIe controller") ?
> Because it wasn't working in the first place ?

Yes, I think so.

> Thank you for all the pointers, I'll take them into account for the
> next iteration. This is the first time I actually submitted a series of
> patches to the LKML so it's all relatively new to me.

Welcome to Linux, and great start!

Bjorn