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

[Heiko Stuebner]

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?

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
> > >
>