Re: [PATCH] arm64: dts: rockchip: Enable pcie2 and audio jack on rk3566-roc-pc

[Heiko Stuebner <heiko@sntech.de>]

Hi Furkan,

Am Sonntag, 9. Oktober 2022, 22:07:07 CEST schrieb Furkan Kardame:
> Add dts nodes to enable combphy2, pcie2 and audio jack.
> Disable i2c5 as it not used.

A commit message like this very much indicates that its contents want
to get split over multiple patches.

A commit should do one thing that can be described with a full commit
message but not multiple different things.


The big patches would be:
- enable sound on rk3566-roc-pc
- enable pcie2 on rk3566-roc-pc

plus the other stuff I mention below.


> Signed-off-by: Furkan Kardame <f.kardame@manjaro.org>
> ---
>  .../arm64/boot/dts/rockchip/rk3566-roc-pc.dts | 97 ++++++++++++++++++-
>  1 file changed, 93 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3566-roc-pc.dts b/arch/arm64/boot/dts/rockchip/rk3566-roc-pc.dts
> index dba648c2f..1e5c83012 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3566-roc-pc.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3566-roc-pc.dts
> @@ -39,6 +39,28 @@ hdmi_con_in: endpoint {
>  		};
>  	};
>  
> +	i2s1_8ch: i2s@fe410000 {
> +		compatible = "rockchip,rk3568-i2s-tdm";
> +		reg = <0x0 0xfe410000 0x0 0x1000>;
> +		interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>;
> +		assigned-clocks = <&cru CLK_I2S1_8CH_TX_SRC>, <&cru CLK_I2S1_8CH_RX_SRC>;
> +		assigned-clock-rates = <1188000000>, <1188000000>;
> +		clocks = <&cru MCLK_I2S1_8CH_TX>, <&cru MCLK_I2S1_8CH_RX>,
> +			 <&cru HCLK_I2S1_8CH>;
> +		clock-names = "mclk_tx", "mclk_rx", "hclk";
> +		dmas = <&dmac1 3>, <&dmac1 2>;
> +		dma-names = "rx", "tx";
> +		resets = <&cru SRST_M_I2S1_8CH_TX>, <&cru SRST_M_I2S1_8CH_RX>;
> +		reset-names = "tx-m", "rx-m";
> +		rockchip,grf = <&grf>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&i2s1m0_sclktx &i2s1m0_sclkrx
> +			     &i2s1m0_lrcktx &i2s1m0_lrckrx
> +			     &i2s1m0_sdi0   &i2s1m0_sdo0>;
> +		#sound-dai-cells = <0>;
> +		status = "disabled";
> +	};

this looks like a core i2s controller and should
therefore be in rk356x.dtsi (as it most likely exists
in both rk3566 and rk3568) - and be a separate patch.


>  	leds {
>  		compatible = "gpio-leds";
>  
> @@ -53,6 +75,22 @@ led-user {
>  		};
>  	};
>  
> +	rk809-sound {
> +		compatible = "simple-audio-card";
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,name = "STATION-M2-FRONT";
> +		simple-audio-card,mclk-fs = <256>;
> +		status = "okay";
> +
> +		simple-audio-card,cpu {
> +			sound-dai = <&i2s1_8ch>;
> +		};
> +
> +		simple-audio-card,codec {
> +			sound-dai = <&rk809>;
> +		};
> +	};
> +
>  	sdio_pwrseq: sdio-pwrseq {
>  		status = "okay";
>  		compatible = "mmc-pwrseq-simple";
> @@ -82,6 +120,18 @@ vcc5v0_sys: vcc5v0-sys-regulator {
>  		vin-supply = <&usb_5v>;
>  	};
>  
> +	vcc3v3_pcie: vcc3v3-pcie-regulator {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio0 RK_PC4 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pcie_enable_h>;
> +		regulator-name = "vcc3v3_pcie";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&vcc5v0_sys>;
> +	};
> +
>  	vcc3v3_sys: vcc3v3-sys-regulator {
>  		compatible = "regulator-fixed";
>  		regulator-name = "vcc3v3_sys";
> @@ -122,6 +172,10 @@ &combphy1 {
>  	status = "okay";
>  };
>  
> +&combphy2 {
> +	status = "okay";
> +};
> +
>  &cpu0 {
>  	cpu-supply = <&vdd_cpu>;
>  };
> @@ -142,7 +196,7 @@ &gmac1 {
>  	assigned-clocks = <&cru SCLK_GMAC1_RX_TX>, <&cru SCLK_GMAC1_RGMII_SPEED>, <&cru SCLK_GMAC1>;
>  	assigned-clock-parents = <&cru SCLK_GMAC1_RGMII_SPEED>, <&cru SCLK_GMAC1>, <&gmac1_clkin>;
>  	clock_in_out = "input";
> -	phy-mode = "rgmii-id";
> +	phy-mode = "rgmii";

what is this doing here?
Definitly separate and with a lot of explanation
why it is necessary.


>  	phy-supply = <&vcc_3v3>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&gmac1m0_miim
> @@ -210,9 +264,13 @@ rk809: pmic@20 {
>  		interrupt-parent = <&gpio0>;
>  		interrupts = <RK_PA7 IRQ_TYPE_LEVEL_LOW>;
>  		clock-output-names = "rk808-clkout1", "rk808-clkout2";
> -
> +		assigned-clocks = <&cru I2S1_MCLKOUT_TX>;
> +		assigned-clock-parents = <&cru CLK_I2S1_8CH_TX>;
> +		clock-names = "mclk";
> +		clocks = <&cru I2S1_MCLKOUT_TX>;
> +		pinctrl-0 = <&pmic_int>, <&i2s1m0_mclk>;
> +		#sound-dai-cells = <0>;
>  		pinctrl-names = "default";
> -		pinctrl-0 = <&pmic_int>;
>  		rockchip,system-power-controller;
>  		wakeup-source;
>  		#clock-cells = <1>;
> @@ -419,6 +477,10 @@ vcc3v3_sd: SWITCH_REG2 {
>  				regulator-boot-on;
>  			};
>  		};
> +
> +		codec {
> +			mic-in-differential;
> +		};
>  	};
>  };
>  
> @@ -432,11 +494,20 @@ &i2c2 {
>  
>  &i2c3 {
>  	pinctrl-names = "default";
> -	pinctrl-0 = <&i2c3m1_xfer>;
> +	pinctrl-0 = <&i2c3m0_xfer>;

Also separate with a paragraph on why.


>  	status = "okay";
>  };
>  
>  &i2c5 {
> +	status = "disabled";

definitly also separate


> +};
> +
> +&i2s0_8ch {
> +	status = "okay";
> +};

where is this i2s0 connected to?


> +
> +&i2s1_8ch {
> +	rockchip,trcm-sync-tx-only;
>  	status = "okay";
>  };
>  
> @@ -447,6 +518,14 @@ rgmii_phy1: ethernet-phy@0 {
>  	};
>  };
>  
> +&pcie2x1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pcie_reset_h>;
> +	reset-gpios = <&gpio1 RK_PB2 GPIO_ACTIVE_HIGH>;
> +	vpcie3v3-supply = <&vcc3v3_pcie>;
> +	status = "okay";
> +};
> +
>  &pinctrl {
>  	bt {
>  		bt_enable_h: bt-enable-h {
> @@ -468,6 +547,16 @@ user_led_enable_h: user-led-enable-h {
>  		};
>  	};
>  
> +	pcie {
> +		pcie_enable_h: pcie-enable-h {
> +			rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +
> +		pcie_reset_h: pcie-reset-h {
> +			rockchip,pins = <1 RK_PB2 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
>  	pmic {
>  		pmic_int: pmic_int {
>  			rockchip,pins =
> 


Heiko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel