Re: [PATCH v2 5/5] arm64: dts: qcom: qcs8550: add QCS8550 RB5Gen2 board support

[Neil Armstrong]

On 4/8/26 11:57, Konrad Dybcio wrote:
> On 4/7/26 5:46 PM, Joe Sandom via B4 Relay wrote:
> > From: Joe Sandom <jsandom@xxxxxxxx>
> >
> > The RB5gen2 is an embedded development platform for the
> > QCS8550, based on the Snapdragon 8 Gen 2 SoC (SM8550).
>
> [...]
>
>
> > +	/* Lontium LT9611UXC fails FW upgrade and has timeouts with geni-i2c */
> > +	/* Workaround is to use bit-banged I2C */
>
> Interesting.. I was under the impression that it was only an issue on
> RB1 and RB2 boards.. perhaps we're missing some magic register write..

On the SM8650-HDK, the HDMI i2c is shared with the battmgr in ADSP, causing a lot
of issues... the only solution is [1] to declare the bus shared and use GPI DMA.
I assume it's the same here.

Sharing the same bus between APPS and ADSP is a weird feat... It seems that having
26 i2c busses and 212 gpios available on the system is not enough.

[1] https://lore.kernel.org/all/20241129144357.2008465-1-quic_msavaliy@xxxxxxxxxxx/

> [...]
>
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&wlan_en>, <&bt_default>, <&sw_ctrl_default>,
> > +			    <&pmk8550_sleep_clk>;
>
> nit: let's keep the order of
>
> property-n
> property-names
>
> file-wide
>
> [...]
>
> > +&sdhc_2 {
> > +	cd-gpios = <&pm8550_gpios 12 GPIO_ACTIVE_LOW>;
> > +
> > +	pinctrl-0 = <&sdc2_default>, <&sdc2_card_det_n>;
> > +	pinctrl-1 = <&sdc2_sleep>, <&sdc2_card_det_n>;
> > +	pinctrl-names = "default", "sleep";
> > +
> > +	vmmc-supply = <&vreg_l9b_2p9>;
> > +	vqmmc-supply = <&vreg_l8b_1p8>;
> > +
> > +	max-sd-hs-hz = <37000000>;
>
> Are you sure you want to overwrite that? The value in the SoC DTSI is
> set to half a MHz higher
>
>
> > +
> > +	no-sdio;
> > +	no-mmc;
> > +
> > +	status = "okay";
> > +};
> > +
> > +&sleep_clk {
> > +	clock-frequency = <32764>;
> > +};
> > +
> > +&spi11 {
> > +	status = "okay";
> > +
> > +	can@0 {
> > +		compatible = "microchip,mcp2518fd";
> > +		reg = <0>;
> > +		interrupts-extended = <&tlmm 55 IRQ_TYPE_LEVEL_LOW>;
> > +		clocks = <&clk40m>;
> > +		spi-max-frequency = <10000000>;
> > +		vdd-supply = <&vreg_l14b_3p2>;
> > +		xceiver-supply = <&vreg_l14b_3p2>;
>
> It may be that for this chip to actually be able to communiate with devices
> on the bus, you need to set the new 'microchip,xstbyen' property
>
> see:
>
> https://lore.kernel.org/linux-arm-msm/20260321135031.3107408-1-viken.dadhaniya@xxxxxxxxxxxxxxxx/
>
> [...]
>
> > +&tlmm {
> > +	gpio-reserved-ranges = <32 8>;
>
> Would you happen to know what these pins are connected to, and if
> so, add a comment (like in arch/arm64/boot/dts/qcom/x1-crd.dtsi)?
>
>
>
> > +
> > +	bt_default: bt-default-state {
> > +		pins = "gpio81";
>
> It would be best to keep these entries ordered by pin idx
>
> > +		function = "gpio";
> > +		drive-strength = <16>;
> > +		bias-disable;
> > +	};
> > +
> > +	sw_ctrl_default: sw-ctrl-default-state {
> > +		pins = "gpio82";
> > +		function = "gpio";
> > +		bias-pull-down;
> > +	};
> > +
> > +	lt9611_irq_pin: lt9611-irq-state {
> > +		pins = "gpio40";
> > +		function = "gpio";
> > +		bias-disable;
> > +	};
> > +
> > +	lt9611_rst_pin: lt9611-rst-state {
> > +		pins = "gpio7";
> > +		function = "gpio";
> > +		output-high;
>
> You shouldn't need to assert the GPIO state in the pin entry node
> - the driver should take care of that
>
>   
> > +	};
> > +
> > +	ntn0_en: ntn0-en-state {
> > +		pins = "gpio67";
> > +		function = "gpio";
> > +		drive-strength = <2>;
> > +		bias-disable;
> > +	};
> > +
> > +	ntn1_en: ntn1-en-state {
> > +		pins = "gpio42";
> > +		function = "gpio";
> > +		drive-strength = <2>;
> > +		bias-disable;
> > +	};
> > +
> > +	upd_1p05_en: upd-1p05-en-state {
> > +		pins = "gpio179";
> > +		function = "gpio";
> > +		drive-strength = <2>;
> > +		bias-pull-up;
> > +	};
>
> I don't know if pulling up an active-high pin is what you want
> (there's some more occurences)
>
> Konrad