Received-SPF: pass (google.com: domain of linux-kernel+bounces-149696-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Heiko =?ISO-8859-1?Q?St=FCbner?= <heiko@sntech.de>
To: Quentin Schulz <quentin.schulz@theobroma-systems.com>
Cc: linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org,
 linux-kernel@vger.kernel.org, Heiko Stuebner <heiko.stuebner@cherry.de>
Subject: Re: [PATCH] arm64: dts: rockchip: add PCIe3 support on rk3588-jaguar
Date: Thu, 18 Apr 2024 10:12:54 +0200
Message-ID: <5932394.MhkbZ0Pkbq@diego>
In-Reply-To: <cb2004e3-563e-4d36-ab69-89b1d9f6221b@theobroma-systems.com>
References:
 <20240417141642.2243193-1-heiko@sntech.de>
 <cb2004e3-563e-4d36-ab69-89b1d9f6221b@theobroma-systems.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"

Am Donnerstag, 18. April 2024, 10:07:37 CEST schrieb Quentin Schulz:
> On 4/17/24 16:16, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@cherry.de>
> > 
> > The Jaguar SBC provides a M.2 slot connected to the pcie3 controller.
> > In contrast to a number of other boards the pcie-refclk is gpio-controlled,
> > so the necessary clock is added to the list of pcie3 clocks.
> > 
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> > ---
> >   .../arm64/boot/dts/rockchip/rk3588-jaguar.dts | 53 +++++++++++++++++++
> >   1 file changed, 53 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
> > index 5002105dc78e..908fbabd8b00 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-jaguar.dts
> > @@ -72,6 +72,25 @@ led-1 {
> >   		};
> >   	};
> >   
> > +	/*
> > +	 * 100MHz reference clock for PCIe peripherals from PI6C557-05BLE
> > +	 * clock generator.
> > +	 * The clock output is gated via the OE pin on the clock generator.
> > +	 * This is modeled as a fixed-clock plus a gpio-gate-clock.
> > +	 */
> > +	pcie_refclk_gen: pcie-refclk-gen-clock {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <1000000000>;
> > +	};
> > +
> > +	pcie_refclk: pcie-refclk-clock {
> > +		compatible = "gpio-gate-clock";
> > +		clocks = <&pcie_refclk_gen>;
> > +		#clock-cells = <0>;
> > +		enable-gpios = <&gpio0 RK_PC6 GPIO_ACTIVE_LOW>; /* PCIE30X4_CLKREQN_M0 */
> 
> I think we usually want to have the pinctrl for GPIOs as well to not 
> assume the pins are muxed in that function by default or by the bootloader?
> 
> > +	};
> > +
> >   	pps {
> >   		compatible = "pps-gpio";
> >   		gpios = <&gpio0 RK_PD5 GPIO_ACTIVE_HIGH>;
> > @@ -466,6 +485,40 @@ &pcie2x1l0 {
> >   	status = "okay";
> >   };
> >   
> > +&pcie30phy {
> > +	status = "okay";
> > +};
> > +
> > +&pcie30x4m0_pins {
> 
> I'm wondering if it really makes sense to reuse this node if we're 
> planning to change the only property it has to mean something different?

ok, will create a specific node for the new pinctrl
As an added pro, this also moves it to the pinctrl area in the dts

> > +	/*
> > +	 * pcie30x4_clkreqn_m0 is used by the refclk generator
> > +	 * pcie30x4_perstn_m0 is used as via the reset-gpio
> > +	 */
> > +	rockchip,pins =
> > +		/* pcie30x4_waken_m0 */
> > +		<0 RK_PC7 12 &pcfg_pull_none>;
> > +};
> > +
> > +&pcie3x4 {
> > +	/*
> > +	 * The board has a gpio-controlled "pcie_refclk" generator,
> > +	 * so add it to the list of clocks.
> > +	 */
> > +	clocks = <&cru ACLK_PCIE_4L_MSTR>, <&cru ACLK_PCIE_4L_SLV>,
> > +		 <&cru ACLK_PCIE_4L_DBI>, <&cru PCLK_PCIE_4L>,
> > +		 <&cru CLK_PCIE_AUX0>, <&cru CLK_PCIE4L_PIPE>,
> > +		 <&pcie_refclk>;
> > +	clock-names = "aclk_mst", "aclk_slv",
> > +		      "aclk_dbi", "pclk",
> > +		      "aux", "pipe",
> > +		      "ref";
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pcie30x4m0_pins>;
> > +	reset-gpios = <&gpio0 RK_PD0 GPIO_ACTIVE_HIGH>; /* PCIE30X4_PERSTN_M0 */
> 
> Ditto, I assume we want to have a pinmux for that GPIO as well?

Correct. While the Rockchip pinctrl driver _does_ the muxing when a gpio
is requested, it wouldn't touch pinconf settings u-boot or whoever might
have set differently before.

Heiko