Re: [PATCH v2 0/9] PCI: rockchip: Fix RK3399 PCIe endpoint controller driver

[Rick Wertenbroek]

On Tue, Mar 14, 2023 at 9:10 AM Damien Le Moal
<damien.lemoal@xxxxxxxxxxxxxxxxxx> wrote:
>
> On 3/14/23 16:57, Rick Wertenbroek wrote:
> > On Tue, Mar 14, 2023 at 1:02 AM Damien Le Moal
> > <damien.lemoal@xxxxxxxxxxxxxxxxxx> wrote:
> >>
> >> On 2/14/23 23:08, Rick Wertenbroek wrote:
> >>> This is a series of patches that fixes the PCIe endpoint controller driver
> >>> for the Rockchip RK3399 SoC. The driver was introduced in
> >>> cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe controller")
> >>> The original driver had issues and would not allow for the RK3399 to
> >>> operate in PCIe endpoint mode correctly. This patch series fixes that so
> >>> that the PCIe core controller of the RK3399 SoC can now act as a PCIe
> >>> endpoint. This is v2 of the patch series and addresses the concerns that
> >>> were raised during the review of the first version.
> >>
> >> Rick,
> >>
> >> Are you going to send a rebased V3 soon ? I have a couple of additional
> >> patches to add on top of your series...
> >>
> >
> > I'll try to send a V3 this week. The changes to V2 will be the issues
> > raised and discussed on the V2 here in the mailing list with the additional
> > code for removing the unsupported MSI-X capability list (was discussed
> > in the mailing list as well).
>
> Thanks.
>
> Additional patch needed to avoid problems with this controller is that
> we need to set ".align = 256" in the features. Otherwise, things do not
> work well. This is because the ATU drops the low 8-bits of the PCI
> addresses. It is a one liner patch, so feel free to add it to your series.
>
> I also noticed random issues wich seem to be due to link-up timing... We
> probably will need to implement a poll thread to detect and notify with
> the linkup callback when we actually have the link established with the
> host (see the dw-ep controller which does something similar).
>

Hello Damien,
I also noticed random issues I suspect to be related to link status or power
state, in my case it sometimes happens that the BARs (0-6) in the config
space get reset to 0. This is not due to the driver because the driver never
ever accesses these registers (@0xfd80'0010 to 0xfd80'0024 TRM
17.6.4.1.5-17.6.4.1.10).
I don't think the host rewrites them because lspci shows the BARs as
"[virtual]" which means they have been assigned by host but have 0
value in the endpoint device (when lspci rereads the PCI config header).
See https://github.com/pciutils/pciutils/blob/master/lspci.c#L422

So I suspect the controller detects something related to link status or
power state and internally (in hardware) resets those registers. It's not
the kernel code, it never accesses these regs. The problem occurs
very randomly, sometimes in a few seconds, sometimes I cannot see
it for a whole day.

Is this similar to what you are experiencing ?
Do you have any idea as to what could make these registers to be reset
(I could not find anything in the TRM, also nothing in the driver seems to
cause it).

>
> From: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
> Date: Thu, 9 Mar 2023 16:37:24 +0900
> Subject: [PATCH] PCI: rockchip: Set address alignment for endpoint mode
>
> The address translation unit of the rockchip EP controller does not use
> the lower 8 bits of a PCIe-space address to map local memory. Thus we
> must set the align feature field to 256 to let the user know about this
> constraint.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/pci/controller/pcie-rockchip-ep.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c
> b/drivers/pci/controller/pcie-rockchip-ep.c
> index 12db9a9d92af..c6a23db84967 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -471,6 +471,7 @@ static const struct pci_epc_features
> rockchip_pcie_epc_features = {
>         .linkup_notifier = false,
>         .msi_capable = true,
>         .msix_capable = false,
> +       .align = 256,
>  };
>
>  static const struct pci_epc_features*
> --
> 2.39.2
>
>
> --
> Damien Le Moal
> Western Digital Research
>

Do you want me to include this patch in the V3 series or will you
submit another patch series for the changes you applied on the RK3399 PCIe
endpoint controller ? I don't know if you prefer to build the V3
together or if you
prefer to submit another patch series on top of mine. Let me know.

Best regards.
Rick