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

[Neil Armstrong <neil.armstrong@linaro.org>]

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@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..

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@quicinc.com/

> 
> 
> [...]
> 
>> +		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@oss.qualcomm.com/
> 
> [...]
> 
>> +&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