linux-kernel - Re: [PATCH 2/3] arm64: dts: rockchip: Add PCIe Gen2x1 controller for RK3528

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aMKAvCglcaC6-00k@pie>
Date: Thu, 11 Sep 2025 07:56:44 +0000
From: Yao Zi <ziyao@...root.org>
To: Jonas Karlman <jonas@...boo.se>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kwilczynski@...nel.org>,
	Manivannan Sadhasivam <mani@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Shawn Lin <shawn.lin@...k-chips.com>,
	Simon Xue <xxm@...k-chips.com>, linux-pci@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Chukun Pan <amadeus@....edu.cn>
Subject: Re: [PATCH 2/3] arm64: dts: rockchip: Add PCIe Gen2x1 controller for
 RK3528

On Wed, Sep 10, 2025 at 11:29:00PM +0200, Jonas Karlman wrote:
> Hi Yao Zi,
> 
> On 9/6/2025 3:52 PM, Yao Zi wrote:
> > Describes the PCIe Gen2x1 controller integrated in RK3528 SoC. The SoC
> > doesn't provide a separate MSI controller, thus the one integrated in
> > designware PCIe IP must be used.
> > 
> > Signed-off-by: Yao Zi <ziyao@...root.org>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3528.dtsi | 56 +++++++++++++++++++++++-
> >  1 file changed, 55 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3528.dtsi b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > index db5dbcac7756..2d2af467e5ab 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3528.dtsi
> > @@ -7,6 +7,7 @@
> >  #include <dt-bindings/gpio/gpio.h>
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  #include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/phy/phy.h>
> >  #include <dt-bindings/pinctrl/rockchip.h>
> >  #include <dt-bindings/clock/rockchip,rk3528-cru.h>
> >  #include <dt-bindings/power/rockchip,rk3528-power.h>
> > @@ -239,7 +240,7 @@ gmac0_clk: clock-gmac50m {
> >  
> >  	soc {
> >  		compatible = "simple-bus";
> > -		ranges = <0x0 0xfe000000 0x0 0xfe000000 0x0 0x2000000>;
> > +		ranges = <0x0 0xfc000000 0x0 0xfc000000 0x0 0x44400000>;
> 
> We should use the dbi reg area in the 32-bit address space, please use:
> 
>   ranges = <0x0 0xfc000000 0x0 0xfc000000 0x0 0x4000000>;

This seems strange to me. I read through TRMs for RK3562 and RK3576, and
found it's common for Rockchip SoCs to map DBI regions of PCIe
controllers to two separate MMIO regions, but am still not sure why it's
necessary to use the mapping in the 32-bit address space.

However, I'm willing to follow the vendor's decision here in order to
avoid unexpected problems. Will adapt this in v2.

> >  		#address-cells = <2>;
> >  		#size-cells = <2>;
> >  
> > @@ -1133,6 +1134,59 @@ combphy: phy@...c0000 {
> >  			rockchip,pipe-phy-grf = <&pipe_phy_grf>;
> >  			status = "disabled";
> >  		};
> > +
> > +		pcie: pcie@...f0000 {
> 
> With the dbi reg area changed below, please update the node name and
> move this node to top of the soc node.
> 
>   pcie@...00000
> 
> > +			compatible = "rockchip,rk3528-pcie",
> > +				     "rockchip,rk3568-pcie";
> > +			reg = <0x1 0x40000000 0x0 0x400000>,
> 
> We should use the dbi reg area in the 32-bit address space, please use:
> 
>   reg = <0x0 0xfe000000 0x0 0x400000>,
> 
> > +			      <0x0 0xfe4f0000 0x0 0x10000>,
> > +			      <0x0 0xfc000000 0x0 0x100000>;
> > +			reg-names = "dbi", "apb", "config";
> > +			bus-range = <0x0 0xff>;
> > +			clocks = <&cru ACLK_PCIE>, <&cru HCLK_PCIE_SLV>,
> > +				 <&cru HCLK_PCIE_DBI>, <&cru PCLK_PCIE>,
> > +				 <&cru CLK_PCIE_AUX>, <&cru PCLK_PCIE_PHY>;
> > +			clock-names = "aclk_mst", "aclk_slv",
> > +				      "aclk_dbi", "pclk",
> > +				      "aux", "pipe";
> 
> In my U-Boot test I did not have the pipe/phy clock here, do we need it?

Just as mentioned by Chukun, the clock should indeed be managed by phy
instead of the PCIe controller. Will fix it as well.

> With above fixed this more or less matches my U-Boot testing, and is:
> 
> Reviewed-by: Jonas Karlman <jonas@...boo.se>

Much thanks.

> Regards,
> Jonas

Best regards,
Yao Zi