Remove write accesses to registers that are marked "unused" (and
therefore read-only) in the technical reference manual (TRM)
(see RK3399 TRM 17.6.8.1)
Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
---
drivers/pci/controller/pcie-rockchip-ep.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index d1a200b93..d5c477020 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -61,10 +61,6 @@ static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip,
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_CPU_ADDR0(region));
- rockchip_pcie_write(rockchip, 0,
- ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR1(region));
}
static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
@@ -114,12 +110,6 @@ static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn,
PCIE_CORE_OB_REGION_ADDR0_LO_ADDR);
addr1 = upper_32_bits(cpu_addr);
}
-
- /* CPU bus address region */
- rockchip_pcie_write(rockchip, addr0,
- ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR0(r));
- rockchip_pcie_write(rockchip, addr1,
- ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR1(r));
}
static int rockchip_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
--
2.25.1On 2/14/23 23:08, Rick Wertenbroek wrote: > Remove write accesses to registers that are marked "unused" (and > therefore read-only) in the technical reference manual (TRM) > (see RK3399 TRM 17.6.8.1) > > Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com> I checked the TRM and indeed these registers are listed as unused. However, with this patch, nothing work for me using a Pine rockpro64 board. Keeping this patch, your series (modulo some other fixes, more emails coming) is making things work ! So I think the bug is with the TRM, not the code. THinking logically about htis, it makes sense: this is programming the address translation unit to translate mmio & dma between host PCI address and local CPU space address. If we never set the PU address, how can that unit possibly ever translate anything ? > --- > drivers/pci/controller/pcie-rockchip-ep.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c > index d1a200b93..d5c477020 100644 > --- a/drivers/pci/controller/pcie-rockchip-ep.c > +++ b/drivers/pci/controller/pcie-rockchip-ep.c > @@ -61,10 +61,6 @@ static void rockchip_pcie_clear_ep_ob_atu(struct rockchip_pcie *rockchip, > 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_CPU_ADDR0(region)); > - rockchip_pcie_write(rockchip, 0, > - ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR1(region)); > } > > static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn, > @@ -114,12 +110,6 @@ static void rockchip_pcie_prog_ep_ob_atu(struct rockchip_pcie *rockchip, u8 fn, > PCIE_CORE_OB_REGION_ADDR0_LO_ADDR); > addr1 = upper_32_bits(cpu_addr); > } > - > - /* CPU bus address region */ > - rockchip_pcie_write(rockchip, addr0, > - ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR0(r)); > - rockchip_pcie_write(rockchip, addr1, > - ROCKCHIP_PCIE_AT_OB_REGION_CPU_ADDR1(r)); > } > > static int rockchip_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn, -- Damien Le Moal Western Digital Research
On Wed, Feb 15, 2023 at 12:56 AM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> I checked the TRM and indeed these registers are listed as unused.
> However, with this patch, nothing work for me using a Pine rockpro64
> board. Keeping this patch, your series (modulo some other fixes, more
> emails coming) is making things work !
Hello, Thank you for testing the driver and commenting, I'll incorporate your
suggestions in the next version of this series.
This patch alone does not make the driver work. Without the fixes to the
address windows and translation found in [PATCH v2 6/9] ("PCI: rockchip:
Fix window mapping and address translation for endpoint") transfers will not
work. However, as you said, with the patch series, the driver works.
Good to see that you have the driver working on the rockpro64 which is a
very similar but different board than the one I used (FriendlyElec NanoPC-T4).
> So I think the bug is with the TRM, not the code. THinking logically about
> htis, it makes sense: this is programming the address translation unit to
> translate mmio & dma between host PCI address and local CPU space address.
> If we never set the PU address, how can that unit possibly ever translate
> anything ?
No, the bug is not in the TRM:
The RK3399 PCIe endpoint core has the physical address space of 64MB
@ 0xF800'0000 to access the PCIe address space (TRM 17.5.4).
This space is split into 33 windows, one of 32MBytes and 32 of 1MByte.
Read-write accesses by the CPU to that region will be translated. Each
window has a mapping that is configured through the ATR Configuration
Register Address Map (TRM 17.6.8) and the registers addr0 and addr1
will dictate the translation between the window (a physical CPU addr)
into a PCI space address (with this the unit can translate). The other
registers are for the PCIe header descriptor.
The translation process is documented in TRM 17.5.5.1.1
The core will translate all read-write accesses to the windows that fall
in the 64MB space @ 0xF800'0000 and generate the PCIe addresses
and headers according to the values in the registers in the ATR
Configuration Register Address Map (@ 0xFDC0'0000).
Translation does indeed take place and works
but requires the changes in [PATCH v2 6/9] ("PCI: rockchip:
Fix window mapping and address translation for endpoint")
because it was broken from the start...
The two writes that were removed are to unused (read-only) registers.
The writes don't do anything, manually writing and reading back these
addresses will always lead to 0 (they are read-only). So they are removed.
On 2/15/23 18:04, Rick Wertenbroek wrote:
> On Wed, Feb 15, 2023 at 12:56 AM Damien Le Moal
> <damien.lemoal@opensource.wdc.com> wrote:
>>
>> I checked the TRM and indeed these registers are listed as unused.
>> However, with this patch, nothing work for me using a Pine rockpro64
>> board. Keeping this patch, your series (modulo some other fixes, more
>> emails coming) is making things work !
>
> Hello, Thank you for testing the driver and commenting, I'll incorporate your
> suggestions in the next version of this series.
>
> This patch alone does not make the driver work. Without the fixes to the
> address windows and translation found in [PATCH v2 6/9] ("PCI: rockchip:
> Fix window mapping and address translation for endpoint") transfers will not
> work. However, as you said, with the patch series, the driver works.
> Good to see that you have the driver working on the rockpro64 which is a
> very similar but different board than the one I used (FriendlyElec NanoPC-T4).
>
>> So I think the bug is with the TRM, not the code. THinking logically about
>> htis, it makes sense: this is programming the address translation unit to
>> translate mmio & dma between host PCI address and local CPU space address.
>> If we never set the PU address, how can that unit possibly ever translate
>> anything ?
>
> No, the bug is not in the TRM:
> The RK3399 PCIe endpoint core has the physical address space of 64MB
> @ 0xF800'0000 to access the PCIe address space (TRM 17.5.4).
> This space is split into 33 windows, one of 32MBytes and 32 of 1MByte.
> Read-write accesses by the CPU to that region will be translated. Each
> window has a mapping that is configured through the ATR Configuration
> Register Address Map (TRM 17.6.8) and the registers addr0 and addr1
> will dictate the translation between the window (a physical CPU addr)
> into a PCI space address (with this the unit can translate). The other
> registers are for the PCIe header descriptor.
> The translation process is documented in TRM 17.5.5.1.1
> The core will translate all read-write accesses to the windows that fall
> in the 64MB space @ 0xF800'0000 and generate the PCIe addresses
> and headers according to the values in the registers in the ATR
> Configuration Register Address Map (@ 0xFDC0'0000).
>
> Translation does indeed take place and works
> but requires the changes in [PATCH v2 6/9] ("PCI: rockchip:
> Fix window mapping and address translation for endpoint")
> because it was broken from the start...
>
> The two writes that were removed are to unused (read-only) registers.
> The writes don't do anything, manually writing and reading back these
> addresses will always lead to 0 (they are read-only). So they are removed.
OK. Tested again and it is working-ish...
./pcitest.sh
## Loading pci_endpoint_test
## BAR tests
BAR0: OKAY
BAR1: OKAY
BAR2: OKAY
BAR3: OKAY
BAR4: OKAY
BAR5: OKAY
## Legacy interrupt tests
SET IRQ TYPE TO LEGACY: OKAY
LEGACY IRQ: OKAY
## MSI interrupt tests
SET IRQ TYPE TO MSI: OKAY
MSI1: OKAY
MSI2: OKAY
MSI3: OKAY
MSI4: OKAY
MSI5: OKAY
MSI6: OKAY
MSI7: OKAY
MSI8: OKAY
MSI9: OKAY
MSI10: OKAY
MSI11: OKAY
MSI12: OKAY
MSI13: OKAY
MSI14: OKAY
MSI15: OKAY
MSI16: OKAY
## Read Tests (DMA)
READ ( 1 bytes): OKAY
READ ( 1024 bytes): OKAY
READ ( 1025 bytes): OKAY
READ ( 4096 bytes): OKAY
READ ( 131072 bytes): OKAY
READ (1024000 bytes): OKAY
READ (1024001 bytes): OKAY
READ (1048576 bytes): OKAY
## Write Tests (DMA)
WRITE ( 1 bytes): OKAY
WRITE ( 1024 bytes): OKAY
WRITE ( 1025 bytes): OKAY
WRITE ( 4096 bytes): OKAY
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...
Will keep digging.
Note that this is all tested with the patch series fixing pci-epf-test and
pci_endpoint_test drivers that I posted earlier today. As mentioned, my host is
an AMD Ryzen PC.
--
Damien Le Moal
Western Digital Research
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
On Thu, Feb 16, 2023 at 8:28 AM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> 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
>
desc2 dictates the bits [79-64] of the PCIe header descriptor.
These bits are for the PF TLP Processing hints.
I did not set them because I thought the default value (0) would be fine.
The TRM section 17.6.8.2.5 says that this register values are reset
to 0, therefore, the write here (0) to desc2 should not change anything unless
that register is written somewhere (I don't think it is).
Anyways, it's not a bad idea to set desc2 to 0 in those two functions.
Rick
On 2/16/23 17:43, Rick Wertenbroek wrote:
> On Thu, Feb 16, 2023 at 8:28 AM Damien Le Moal
> <damien.lemoal@opensource.wdc.com> wrote:
>>
>> 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
>>
>
> desc2 dictates the bits [79-64] of the PCIe header descriptor.
> These bits are for the PF TLP Processing hints.
> I did not set them because I thought the default value (0) would be fine.
> The TRM section 17.6.8.2.5 says that this register values are reset
> to 0, therefore, the write here (0) to desc2 should not change anything unless
> that register is written somewhere (I don't think it is).
> Anyways, it's not a bad idea to set desc2 to 0 in those two functions.
I wonder if that register changes when TLPs are processed... So when the region
is remapped, previous values still there cause the problems I am seeing ?
As mentioned, with this "fix", the pcitest.sh is now solid. But I am still
seeing the same issues with my nvme endpoint driver when Linux takes over from
BIOS. Bios works, but not Linux, still no IRQs at all. So there is likely still
another issue that I cannot see at the moment. No hints whatsoever.
--
Damien Le Moal
Western Digital Research
On 2/15/23 18:04, Rick Wertenbroek wrote:
> On Wed, Feb 15, 2023 at 12:56 AM Damien Le Moal
> <damien.lemoal@opensource.wdc.com> wrote:
>>
>> I checked the TRM and indeed these registers are listed as unused.
>> However, with this patch, nothing work for me using a Pine rockpro64
>> board. Keeping this patch, your series (modulo some other fixes, more
>> emails coming) is making things work !
>
> Hello, Thank you for testing the driver and commenting, I'll incorporate your
> suggestions in the next version of this series.
>
> This patch alone does not make the driver work. Without the fixes to the
> address windows and translation found in [PATCH v2 6/9] ("PCI: rockchip:
> Fix window mapping and address translation for endpoint") transfers will not
> work. However, as you said, with the patch series, the driver works.
> Good to see that you have the driver working on the rockpro64 which is a
> very similar but different board than the one I used (FriendlyElec NanoPC-T4).
>
>> So I think the bug is with the TRM, not the code. THinking logically about
>> htis, it makes sense: this is programming the address translation unit to
>> translate mmio & dma between host PCI address and local CPU space address.
>> If we never set the PU address, how can that unit possibly ever translate
>> anything ?
>
> No, the bug is not in the TRM:
> The RK3399 PCIe endpoint core has the physical address space of 64MB
> @ 0xF800'0000 to access the PCIe address space (TRM 17.5.4).
> This space is split into 33 windows, one of 32MBytes and 32 of 1MByte.
> Read-write accesses by the CPU to that region will be translated. Each
> window has a mapping that is configured through the ATR Configuration
> Register Address Map (TRM 17.6.8) and the registers addr0 and addr1
> will dictate the translation between the window (a physical CPU addr)
> into a PCI space address (with this the unit can translate). The other
> registers are for the PCIe header descriptor.
> The translation process is documented in TRM 17.5.5.1.1
> The core will translate all read-write accesses to the windows that fall
> in the 64MB space @ 0xF800'0000 and generate the PCIe addresses
> and headers according to the values in the registers in the ATR
> Configuration Register Address Map (@ 0xFDC0'0000).
>
> Translation does indeed take place and works
> but requires the changes in [PATCH v2 6/9] ("PCI: rockchip:
> Fix window mapping and address translation for endpoint")
> because it was broken from the start...
>
> The two writes that were removed are to unused (read-only) registers.
> The writes don't do anything, manually writing and reading back these
> addresses will always lead to 0 (they are read-only). So they are removed.
OK. I tried so many things to get something working that I probably got
confused with this one :) Let me retry with this patch 1 to see if I get
the same results, which is: pci-epf-test solidly working (with the patches
I sent earlier today), and my on-going nvme epf driver working-ish (BIOS
OK, but IRQs to Linux miserably failing to be sent from EP).
--
Damien Le Moal
Western Digital Research
© 2016 - 2026 Red Hat, Inc.