Re: [PATCH v2 2/2] arm64: dts: rockchip: Add FriendlyElec CM3588 NAS board

[Heiko Stuebner <heiko@sntech.de>]

Am Donnerstag, 6. Juni 2024, 15:13:20 CEST schrieb Space Meyer:
> + Sebastian Reichel regarding pcie3x4 BAR 1 overlap
> 

> On 02.06.2024 22:20, Sebastian Kropatsch wrote:
> > Some RK3588 boards are still using this property, the following quote
> > is from rk3588-tiger-haikou.dts for example:
> >      &sdmmc {
> >          /* while the same pin, sdmmc_det does not detect card changes */
> >          cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
> > 
> > I am unsure as to whether this comment from the quote might apply for
> > the CM3588 as well. Please let me know if you are able to tell :-)
> 
> I don't quite understand this. However GPIO0_A4 *is* routed to the micro 
> sd CD according to the NAS schematic, page 16 around A5.

for the actual sdmmc_det functionality ... possibly some pinconfig thing?
I.e. pull-whatever settings?


> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-friendlyelec-cm3588-nas.dts
> 
> > +	adc_keys: adc-keys {
> 
> AFAICT this board uses only 1 button per ADC input. Hence I think we 
> need seperate ADC defs per button. The usual plural "adc-keys" does not 
> apply.

Also if you need to define multiple ones, you'll need distinctive names.


> > +	analog-sound {
> > +		compatible = "simple-audio-card";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&headphone_detect>;
> > +
> > +		simple-audio-card,name = "realtek,rt5616-codec";
> > +		simple-audio-card,format = "i2s";
> > +		simple-audio-card,mclk-fs = <256>;
> > +
> > +		simple-audio-card,hp-det-gpio = <&gpio1 RK_PC4 GPIO_ACTIVE_LOW>;
> > +
> > +		simple-audio-card,routing =
> > +			"Headphones", "HPOL",
> > +			"Headphones", "HPOR",
> > +			"MIC1", "Microphone Jack",
> > +			"Microphone Jack", "micbias1";
> > +		simple-audio-card,widgets =
> > +			"Headphone", "Headphones",
> > +			"Microphone", "Microphone Jack";
> > +
> > +		simple-audio-card,cpu {
> > +			sound-dai = <&i2s0_8ch>;
> > +		};
> > +
> > +		simple-audio-card,codec {
> > +			sound-dai = <&rt5616>;
> > +		};
> > +	};
> 
> The rt5616 is on the SoM according to the schematic. Maybe move it all 
> there and then only define the hp-det-gpio here?

When the codec itself is on the som _it_ should be there.
For the card itself I don't necessarily think so.

I.e. another baseboard (if it ever exists) might not route sound at all,
so I guess it might make more sense to leave the actual "card" on the
baseboard that also provides the actual outputs and inputs.


> > +	vcc_3v3_host_32: regulator-vcc-3v3-host-32 {
> > +		compatible = "regulator-fixed";
> > +		enable-active-high;
> > +		gpios = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&vcc_3v3_host32_en>;
> > +		regulator-name = "vcc_3v3_host_32";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		vin-supply = <&vcc_5v0_sys>;
> > +	};
> 
> I think this is a 5v0 regulator?

At least the vcc_3v3_host_32 naming suggests 3.3V though?


> > +	vcc_3v3_pcie30: regulator-vcc-3v3-pcie30 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc_3v3_pcie30";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		vin-supply = <&vcc_5v0_sys>;
> > +	};
> 
> These are 4 seperate regulators according to the schematic. However, as 
> they are all fixed, idk if they should be split or kept like this.

personally, I really like the power-diagram to match schematics.
I.e. $debugfs/regulator/regulator_summary will produce a really nice
graph of all the system's regulators, so it's definitly nice if the
hirarchy matches. Also prevents head-scratching later on ;-)


> > +	fusb302: typec-portc@22 {
> > +		compatible = "fcs,fusb302";
> > +		reg = <0x22>;
> > +		interrupt-parent = <&gpio0>;
> > +		interrupts = <RK_PD3 IRQ_TYPE_LEVEL_LOW>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&usbc0_int>;
> > +		vbus-supply = <&vbus_5v0_typec>;
> 
> Isn't this missing a `status = "okay";`?

status okay is the default, so when you add a completely new node it is
"okay" by default and you only add a status if you need something else.


> > +&pinctrl {
> > +	audio {
> > +		headphone_detect: headphone-detect {
> > +			rockchip,pins = <1 RK_PC4 RK_FUNC_GPIO &pcfg_pull_none>;
> 
> You could use &gpio1 instead of 1. Same for every entry in &pinctrl.

No, that is a number - that of the pin-bank and not a reference to the
gpio controller. Please any other board/soc with its pin-groups.


> > +&pinctrl {
> > +	gpio-leds {
> > +		led_sys_pin: led-sys-pin {
> > +			rockchip,pins = <2 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>;
> 
> You could use &gpio2 instead of 2. Same for every entry in &pinctrl.

same as above


Heiko