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

[Joe Sandom <jsandom@axon.com>]

On Wed, Apr 08, 2026 at 11:57:18AM +0200, Konrad Dybcio wrote:
> On 4/7/26 5:46 PM, Joe Sandom via B4 Relay wrote:
> > From: Joe Sandom <jsandom@axon.com>
> > 
> > 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..
> 
> 
> [...]
> 
> > +		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
>
Agree. Good catch - will amend this in v3.
> [...]
> 
> > +&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
This was originally carried over from the downstream workaround for 
clock driver ceiling behaviour that I saw;

/*
 * Due to level shifter insertion, HS mode frequency is reduced to 37.5MHz
 * but clk's driver supply 37MHz only and uses ceil ops. So vote for
 * 37MHz to avoid picking next ceil value.
 */
#define LEVEL_SHIFTER_HIGH_SPEED_FREQ	37000000

But I now realise this isn't an issue in the upstream driver, so I will
drop this in v3. Thanks!
> 
> > +
> > +	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://urldefense.com/v3/__https://lore.kernel.org/linux-arm-msm/20260321135031.3107408-1-viken.dadhaniya@oss.qualcomm.com/__;!!K76kBA!3JvIWVouSl6ZkbxojAFYUus-8UPRvjrHx0qENKOyIIBpZ9knecOfH0NCBAr8ESxNXg2H6982UXqs_8QyR_k9NuKnmA$ 
> 
Good to know. Confirmed that it will be needed.
Happy to provide a follow-up patch once the above series lands if that
works for you?
> [...]
> 
> > +&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)?
Yes, 32-35 for NFC and 36-39 for fingerprint sensor. I'll add a comment
in v3 so it's clear.
> 
> 
> > +
> > +	bt_default: bt-default-state {
> > +		pins = "gpio81";
> 
> It would be best to keep these entries ordered by pin idx
Fair point. Will amend this for v3.
> 
> > +		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
Fair point. Will amend this for v3.
> 
>  
> > +	};
> > +
> > +	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)
> 
Good point. Will amend this for v3.
> Konrad