Re: [PATCH 2/5] arm64: dts: qcom: sc8180x: Add Microsoft Surface Pro X

[Val Packett]

On 11/4/26 5:36 AM, Konrad Dybcio wrote:
> On 11/4/26 8:04 AM, Val Packett wrote:
> > From: Maximilian Luz <luzmaximilian@xxxxxxxxx>
> >
> > Introduce a device tree for the Surface Pro X tablet in all 3 variants
> > (SQ1 with cellular, SQ1 Wi-Fi only, SQ2 with cellular).
> >
> > Currently working:
> >
> > * Display, backlight (eDP panel)
> > * USB 2.0
> > * Wi-Fi and Bluetooth (WCN3998)
> > * NVMe on PCIe2
> > * Embedded controller (Surface SAM over UART)
> > * All remoteprocs (ADSP, CDSP, MPSS, SLPI)
> > * Volume and power buttons
> [...]
>
> This needs some sorting etc., but let's tackle the bigger prob first
>
>
> > +&mdss_edp {
> > +	pinctrl-0 = <&edp_hpd_active>;
> > +	pinctrl-names = "default";
> > +
> > +	status = "okay";
> > +
> > +	aux-bus {
> > +		panel {
> > +			compatible = "edp-panel";
> > +			enable-gpios = <&tlmm 130 GPIO_ACTIVE_HIGH>;
> > +			no-hpd;
> Is there really no HPD? The pinctrl above suggests otherwise

Looks like that's been pretty much copy-pasted from lenovo-flex-5g where 
it's also set with no explanation, with the same pinctrl also existing.

I'm suspecting that might've been an attempt at working around the 
inconsistent display initialization, pre clk_rcg2_shared_ops etc.?

I'll try removing it. (Also will convert gpio130 to a power supply like 
flex-5g has it)

> [...]
>
> > +&pcie2 {
> > +	// XXX: pcie1 and pcie2 are multi'd together, for now use just one
> > +	num-lanes = <2>;
> We should be using the new phy-qcom-qmp-pcie-multiphy.c driver I think,
> I'll try chasing down the right magic values
Thanks!!
> > +
> > +	pinctrl-0 = <&pcie1_default_state>;
> > +	pinctrl-names = "default";
> > +
> > +	// HACK: Remove "global" interrupt
> Why?

Related to not having the proper multiphy driver support,

"it seems like that interrupt is expected to signal when the device is 
ready for use after initial setup. Unfortunately that doesn't seem to 
work for the NVME, potentially due to the composite PHYs. Likely it's 
just waiting forever and the IRQ doesn't get triggered. As it's 
optional, we can just drop it for now."


~val