IronPort-SDR: cLK9yLdE/J+fRZUFNmEqO+ZpQinsy3ahe/LneDXdg6TbWMHi8VnphjjpGjyvQOyca7HgcctDIf
 rmy1w7MLQrE08PPEHdt3qVZdtPPf+S1oHKDQbxrPIqIvIrmOLIrpvheZ21hsEIscqP+lOB7HBF
 eGXeoJymRgpusp6x52riw4YNmwWU78T6i/GapQROsiUq9R52MLDs0xadCSfYEqQvuFru+43ZyJ
 fyvUH8azHMxLMzfM24H6htkz81dJ6hxSWzYRNV8/5hSE13iYmwq89UuVIuTCtQRLq0ZEizWzT1
 FNo=
IronPort-SDR: grbqd4EuZDlRrBBIENbIy3TQ90ZbRcrOsPTwE6dYNMQdMe2gm4N1v6pQ1vuFsS4EtdeCDrdwar
 DySoN7kNrGO9hSM6WiX+/wEAnTYbHbj6pFcNbcS+VKLmE6xXyaWqNnGeS/11yAdXaM9mq6u6ea
 uuMrEYRT3jm08LkwUOLtiOwezhTLGratHwy0pVeD+Tr5aSEFGo4bVKm6G+Snp2CtHSY2zsE04i
 I7UoDbsTMC/MrH0AhDslwpBq+3jM0BwCLNrmFbeFw65k6n/feAKbVfbNhIQwsBaRjLn6Iba9Ku
 9b8=
WDCIronportException: Internal
Message-ID: <5c15e1d1-c7e9-0b7c-9b14-f95543c70383@opensource.wdc.com>
Date:   Thu, 16 Feb 2023 16:28:38 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v2 1/9] PCI: rockchip: Remove writes to unused registers
Content-Language: en-US
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
To:     Rick Wertenbroek <rick.wertenbroek@gmail.com>
Cc:     alberto.dassatti@heig-vd.ch, xxm@rock-chips.com,
        rick.wertenbroek@heig-vd.ch, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Heiko Stuebner <heiko@sntech.de>,
        Shawn Lin <shawn.lin@rock-chips.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Jani Nikula <jani.nikula@intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Mikko Kovanen <mikko.kovanen@aavamobile.com>,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-pci@vger.kernel.org
References: <20230214140858.1133292-1-rick.wertenbroek@gmail.com>
 <20230214140858.1133292-2-rick.wertenbroek@gmail.com>
 <2ebd33e2-46ef-356d-ff4c-81b74950d02f@opensource.wdc.com>
 <CAAEEuhr273bKFBWiTVyTjhHhxjuTK=TVd+5K2B07WfWMD+N7mA@mail.gmail.com>
 <559bdd8c-9cc8-d7ae-a937-ffee9cfbb8a6@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <559bdd8c-9cc8-d7ae-a937-ffee9cfbb8a6@opensource.wdc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/15/23 18:58, Damien Le Moal wrote:
[...]
> WRITE ( 131072 bytes):		OKAY
> WRITE (1024000 bytes):		OKAY
> 
> Then stops here doing the 1024001 B case. The host waits for a completion that
> does not come. On the EP side, I see:
> 
> [   94.307215] pci_epf_test pci_epf_test.0: READ src addr 0xffd00000, 1024001 B
> [   94.307960] pci_epc fd000000.pcie-ep: Map region 1 phys addr 0xfa100000 to
> pci addr 0xffd00000, 1024001 B
> [   94.308924] rockchip-pcie-ep fd000000.pcie-ep: Set atu: region 1, cpu addr
> 0xfa100000, pci addr 0xffd00000, 1024001 B
> [  132.309645] dma-pl330 ff6e0000.dma-controller: Reset Channel-2
> CS-20000e FTC-40000
> 
>                                                   ^^^^^^^^^^^^^^^
> The DMA engine does not like something at all. Back to where I was when I tried
> your series initially, which is why I tried removing patch 1 to see what happens...
> 
> [  132.370479] pci_epf_test pci_epf_test.0: READ => Size: 1024001 B, DMA: YES,
> Time: 38.059623935 s, Rate: 26 KB/s
> [  132.372152] pci_epc fd000000.pcie-ep: Unmap region 1
> [  132.372780] pci_epf_test pci_epf_test.0: RAISE MSI IRQ 1
> [  132.373312] rockchip-pcie-ep fd000000.pcie-ep: Send MSI IRQ 1
> [  132.373844] rockchip-pcie-ep fd000000.pcie-ep: MSI disabled
> [  132.374388] pci_epf_test pci_epf_test.0: Raise IRQ failed -22
> 
> And it looks like the PCI core crashed or something because MSI does not work
> anymore as well (note that this is wheat I see with my nvme epf driver too, but
> I do not have that DMA channel reset message...)
> 
> If I run the tests without DMA (mmio only), everything seems fine:
> 
> ## Read Tests (No DMA)
> READ (      1 bytes):		OKAY
> READ (   1024 bytes):		OKAY
> READ (   1025 bytes):		OKAY
> READ (1024000 bytes):		OKAY
> READ (1024001 bytes):		OKAY
> 
> ## Write Tests (No DMA)
> WRITE (      1 bytes):		OKAY
> WRITE (   1024 bytes):		OKAY
> WRITE (   1025 bytes):		OKAY
> WRITE (1024000 bytes):		OKAY
> WRITE (1024001 bytes):		OKAY
> 
> ## Copy Tests (No DMA)
> COPY (      1 bytes):		OKAY
> COPY (   1024 bytes):		OKAY
> COPY (   1025 bytes):		OKAY
> COPY (1024000 bytes):		OKAY
> COPY (1024001 bytes):		OKAY
> 
> So it looks like translation is working with your patch, but that the driver is
> still missing something for DMA to work correctly...

I kept testing this and realized that I was not getting a consistent behavior.
Sometimes all tests passed, but would not repeat (running again would fail
everything), sometimes NMIs from bad accesses, and other times "hang" (test not
completing but no real machine hang/crash). So it started to hint at something
randomly initialized...

Re-reading the TRM, particularly section 17.5.5.1.1, I realized that the lower
16 bits of the desc2 register are used for the translation, but we never set
them with the current code. Only desc0 and desc1... So I added a write(0) to
desc2 and now it is finally working well. Running the tests in a loop, they all
pass and no bad behavior is observed.

My cleaned-up rockchip_pcie_prog_ep_ob_atu() function now looks like this:

static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
                                         u32 r, u32 type, u64 phys_addr,
                                         u64 pci_addr, size_t size)
{
        u64 sz = 1ULL << fls64(size - 1);
        int num_pass_bits = ilog2(sz);
        u32 addr0, addr1, desc0;

        /* Sanity checks */
        if (WARN_ON_ONCE(type == AXI_WRAPPER_NOR_MSG))
                return;
        if (WARN_ON_ONCE(ALIGN_DOWN(phys_addr, SZ_1M) != phys_addr))
                return;
        if (WARN_ON_ONCE(rockchip_ob_region(phys_addr + size - 1) != r))
                return;

        /* We must pass at least 8 bits of PCI bus address */
        if (num_pass_bits < 8)
                num_pass_bits = 8;

        /* PCI bus address region */
        addr0 = ((num_pass_bits - 1) & PCIE_CORE_OB_REGION_ADDR0_NUM_BITS) |
                (lower_32_bits(pci_addr) & PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
        addr1 = upper_32_bits(pci_addr);
        desc0 = ROCKCHIP_PCIE_AT_OB_REGION_DESC0_DEVFN(fn) | type;

        rockchip_pcie_write(rockchip, addr0,
                            ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(r));
        rockchip_pcie_write(rockchip, addr1,
                            ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(r));
        rockchip_pcie_write(rockchip, desc0,
                            ROCKCHIP_PCIE_AT_OB_REGION_DESC0(r));
        rockchip_pcie_write(rockchip, 0,
                            ROCKCHIP_PCIE_AT_OB_REGION_DESC1(r));
        rockchip_pcie_write(rockchip, 0,
                            ROCKCHIP_PCIE_AT_OB_REGION_DESC2(r));
}

And the function rockchip_pcie_clear_ep_ob_atu() also clears desc2:

static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
                                          u32 region)
{
        rockchip_pcie_write(rockchip, 0,
                            ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR0(region));
        rockchip_pcie_write(rockchip, 0,
                            ROCKCHIP_PCIE_AT_OB_REGION_PCI_ADDR1(region));
        rockchip_pcie_write(rockchip, 0,
                            ROCKCHIP_PCIE_AT_OB_REGION_DESC0(region));
        rockchip_pcie_write(rockchip, 0,
                            ROCKCHIP_PCIE_AT_OB_REGION_DESC1(region));
        rockchip_pcie_write(rockchip, 0,
                            ROCKCHIP_PCIE_AT_OB_REGION_DESC2(region));
}

Thoughts ?

-- 
Damien Le Moal
Western Digital Research