Re: [PATCH v2] arm64: dts: rockchip: rk356x: Fix PCIe register map and ranges

[Peter Geis]

On Mon, Oct 17, 2022 at 7:56 AM Heiko Stuebner <heiko@xxxxxxxxx> wrote:
>
> Hi,
>
> Am Donnerstag, 6. Oktober 2022, 00:08:12 CEST schrieb Ondřej Jirman:
> > On Wed, Oct 05, 2022 at 07:42:54AM -0400, Peter Geis wrote:
> > > On Wed, Oct 5, 2022 at 4:54 AM Ondrej Jirman <megi@xxxxxx> wrote:
> > > >
> > >
> > > Good Morning,
> > >
> > > > I have two Realtek PCIe wifi cards connected over the 4 port PCIe swtich
> > > > to Quartz64-A. The cards fail to work, when nvme SSD is connected at the
> > > > same time to the bridge. Without nvme connected, cards work fine. The
> > > > issue seems to be related to mixed use of devices which make use of I/O
> > > > ranges and memory ranges.
> > > >
> > > > This patch changes I/O, MEM and config mappings so that config and I/O
> > > > mappings use the 0xf4000000 outbound address space, and MEM range uses
> > > > the whole 0x300000000 outbound space.
> > > >
> > > > This is simialar to how BSP does the mappings.
> > >
> > > This change was very recent in the BSP stuff (Jan 2022):
> > > https://github.com/rockchip-linux/kernel/commit/cfab7abefc4093daa379fbd90a1e7ac1a484332b
> > > A few other interesting changes there as well. They added a 32 bit
> > > window in the lower range and made the entire upper range a 64 bit
> > > relocatable (why?) and prefetchable window. They also set the viewport
> > > number to 8. The dt-binding says this is autodetected, but I wonder if
> > > the value is being detected correctly.
> > >
> > > It looks like it is dependent in BSP on a backported change from mainline:
> > > https://github.com/rockchip-linux/kernel/commit/50a01d3c10a6212f66364575a3c8f66c07f41591
> > >
> > > Can someone weigh in why the dw core has config in the reg node
> > > instead of ranges?
> > >
> > > >
> > > > I changed num-ob-windows to value detected by the kernel so if for whatever
> > > > reason the kernel ever starts respecting this DT property, it would not
> > > > switch to sharing I/O and CFG spaces via a single iATU mapping for
> > > > no reason.
> > >
> > > This worries me that this value may be being detected incorrectly,
> > > they set it to this for a reason. It's not unheard of for Rockchip to
> > > need to override what they encode in the silicon.
> >
> > I just noticed that you may be thinking that BSP does some detection. It does
> > not. It just uses either value from DT or hardcoded value 2 in the code.
> >
> > https://github.com/rockchip-linux/kernel/blob/develop-4.19/drivers/pci/controller/dwc/pcie-designware-host.c#L450
>
> @Peter or other people in the recipient list with more PCIe
> experience than me, can someone provide some more judgement
> on this topic?

Good Morning Heiko,

Apologies for just getting to this, I'm still in the middle of moving
and just got my lab set back up.

I've tested this patch series and it leads to the same regression with
NVMe drives. A loop of md5sum on two identical 4GB random files
produces the following results:
d11cf0caa541b72551ca22dc5bef2de0  test-rand.img
fad97e91da8d4fd554c895cafa89809b  test-rand2.img
2d56a7baa05c38535f4c19a2b371f90a  test-rand.img
74e8e6f93d7c3dc3ad250e91176f5901  test-rand2.img
25cfcfecf4dd529e4e9fbbe2be482053  test-rand.img
74e8e6f93d7c3dc3ad250e91176f5901  test-rand2.img
b9637505bf88ed725f6d03deb7065dab  test-rand.img
f7437e88d524ea92e097db51dce1c60d  test-rand2.img

Before this patch series:
d11cf0caa541b72551ca22dc5bef2de0  test-rand.img
d11cf0caa541b72551ca22dc5bef2de0  test-rand2.img
d11cf0caa541b72551ca22dc5bef2de0  test-rand.img
d11cf0caa541b72551ca22dc5bef2de0  test-rand2.img
d11cf0caa541b72551ca22dc5bef2de0  test-rand.img
d11cf0caa541b72551ca22dc5bef2de0  test-rand2.img
d11cf0caa541b72551ca22dc5bef2de0  test-rand.img
d11cf0caa541b72551ca22dc5bef2de0  test-rand2.img

Though I do love where this patch is going and would like to see if it
can be made to work, in its current form it does not.

Very Respectfully,
Peter Geis

>
> Thanks
> Heiko
>
>
> > > Very Respectfully,
> > > Peter Geis
> > >
> > > >
> > > > This change to the regs/ranges makes the issue go away and both nvme and
> > > > wifi cards work when connected at the same time to the bridge. I tested
> > > > the nvme with large amount of reads/writes, both behind the PCIe bridge
> > > > and when directly connected to Quartz64-A board.
> > > >
> > > > Signed-off-by: Ondrej Jirman <megi@xxxxxx>
> > > > ---
> > > > BSP for reference: https://github.com/rockchip-linux/kernel/blob/develop-4.19/arch/arm64/boot/dts/rockchip/rk3568.dtsi#L2370
> > > >
> > > > v2:
> > > > - change ranges to use 0x300000000 fully for MEM and make use of
> > > >   the 0xf4000000 outbound range for IO and config
> > > > - full retest with/without the switch
> > > > - if lscpi/dmesg is useful in the future for comparison, see:
> > > >   https://xff.cz/kernels/random/quartz64a-pcie/
> > > >
> > > > I used this script for the tests:
> > > >
> > > > #!/bin/bash
> > > >
> > > > OUT=/mnt/data
> > > > n=8
> > > >
> > > > test -f /tmp/test.dat || \
> > > >     dd if=/dev/urandom of=/tmp/test.dat bs=1M count=1024
> > > > md5sum /tmp/test.dat
> > > >
> > > > i=0
> > > > while test $i -lt $n
> > > > do
> > > >     dd if=/tmp/test.dat of=$OUT/test$i.dat bs=4M oflag=direct
> > > >
> > > >     i=$(($i+1))
> > > > done
> > > >
> > > > i=0
> > > > while test $i -lt $n
> > > > do
> > > >     dd if=$OUT/test$i.dat bs=4M iflag=direct | md5sum
> > > >
> > > >     i=$(($i+1))
> > > > done
> > > >
> > > >
> > > >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > index 319981c3e9f7..99fd9543fc6f 100644
> > > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > > > @@ -855,7 +855,8 @@ pcie2x1: pcie@fe260000 {
> > > >                 compatible = "rockchip,rk3568-pcie";
> > > >                 reg = <0x3 0xc0000000 0x0 0x00400000>,
> > > >                       <0x0 0xfe260000 0x0 0x00010000>,
> > > > -                     <0x3 0x3f000000 0x0 0x01000000>;
> > > > +                     <0x0 0xf4000000 0x0 0x01f00000>;
> > > > +
> > > >                 reg-names = "dbi", "apb", "config";
> > > >                 interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>,
> > > >                              <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>,
> > > > @@ -877,15 +878,15 @@ pcie2x1: pcie@fe260000 {
> > > >                                 <0 0 0 4 &pcie_intc 3>;
> > > >                 linux,pci-domain = <0>;
> > > >                 num-ib-windows = <6>;
> > > > -               num-ob-windows = <2>;
> > > > +               num-ob-windows = <8>;
> > > >                 max-link-speed = <2>;
> > > >                 msi-map = <0x0 &gic 0x0 0x1000>;
> > > >                 num-lanes = <1>;
> > > >                 phys = <&combphy2 PHY_TYPE_PCIE>;
> > > >                 phy-names = "pcie-phy";
> > > >                 power-domains = <&power RK3568_PD_PIPE>;
> > > > -               ranges = <0x01000000 0x0 0x3ef00000 0x3 0x3ef00000 0x0 0x00100000
> > > > -                         0x02000000 0x0 0x00000000 0x3 0x00000000 0x0 0x3ef00000>;
> > > > +               ranges = <0x01000000 0x0 0x00000000 0x0 0xf5f00000 0x0 0x00100000
> > > > +                         0x02000000 0x0 0x40000000 0x3 0x00000000 0x0 0x40000000>;
> > > >                 resets = <&cru SRST_PCIE20_POWERUP>;
> > > >                 reset-names = "pipe";
> > > >                 #address-cells = <3>;
> > > > --
> > > > 2.37.3
> > > >
> >
>
>
>
>