Message-ID: <CAMcHhXq8kjsbDGHBn=63JutD1TcD6=KVxCQtPHRoLOwE+FY-sA@mail.gmail.com>
Date: Tue, 1 Apr 2025 20:05:05 +0200
From: Aleksandrs Vinarskis <alex.vinarskis@...il.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: Bjorn Andersson <andersson@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, Dmitry Baryshkov <lumag@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Abel Vesa <abel.vesa@...aro.org>, 
	Johan Hovold <johan+linaro@...nel.org>, linux-arm-msm@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-usb@...r.kernel.org, Konrad Dybcio <konradybcio@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Subject: Re: [PATCH v1 6/6] arm64: dts: qcom: Add support for X1-based Asus
 Zenbook A14

On Tue, 1 Apr 2025 at 17:59, Konrad Dybcio
<konrad.dybcio@....qualcomm.com> wrote:
>
> On 3/31/25 11:53 PM, Aleksandrs Vinarskis wrote:
> > Initial support for Asus Zenbook A14. Particular moddel exists
> > in X1-26-100, X1P-42-100 (UX3407QA) and X1E-78-100 (UX3407RA).
> >
> > Mostly similar to other X1-based laptops. Notable differences are:
> > * Wifi/Bluetooth combo being Qualcomm FastConnect 6900 on UX3407QA
> >   and Qualcomm FastConnect 7800 on UX3407RA
> > * USB Type-C retimers are Parade PS8833, appear to behave identical
> >   to Parade PS8830
> > * gpio90 is TZ protected
>
> [...]
>
> > +     leds {
> > +             compatible = "gpio-leds";
> > +
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&cam_indicator_en>;
>
> property-n
> property-names
>
> please, we're trying to unify such small things even though we know
> it's "wrong" in a lot of places
>

will do.

> > +
> > +&i2c0 {
> > +     clock-frequency = <400000>;
> > +     status = "okay";
> > +
> > +     /* ELAN, 04F3:3315 */
> > +     touchpad@15 {
> > +             compatible = "hid-over-i2c";
> > +             reg = <0x15>;
> > +
> > +             hid-descr-addr = <0x1>;
> > +             interrupts-extended = <&tlmm 3 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +             pinctrl-0 = <&tpad_default>;
> > +             pinctrl-names = "default";
> > +
> > +             wakeup-source;
> > +     };
> > +};
> > +
> > +&i2c3 {
> > +     clock-frequency = <400000>;
> > +     status = "okay";
>
> It's also customary to leave a newline before 'status'

will do.

>
> > +&pm8550_gpios {
> > +     rtmr0_default: rtmr0-reset-n-active-state {
> > +             pins = "gpio10";
> > +             function = "normal";
> > +             power-source = <1>; /* 1.8V */
>
> Drop the 1.8v comments please

will do.

>
> [...]
>
> > +&spi10 {
> > +     status = "disabled";
> > +
> > +     /* Unknown device */
> > +};
>
> Does the device crash if you enable this bus? Keeping it 'okay' would
> make it easier for folks to poke at it

It does boot just fine, but does not initialize:
```
geni_spi a88000.spi: Invalid proto 9
...
qnoc-x1e80100 interconnect-1: sync_state() pending due to a88000.spi
...
```

I only quickly checked that 9 is indeed invalid state, iirc should've
been 2. But haven't looked deeper into it, so left it disabled. So I
thought best to leave it off for now. Unless you prefer to drop it
altogether?

>
> > +
> > +&tlmm {
> > +     gpio-reserved-ranges = <44 4>,  /* SPI11, TZ Protected */
> > +                            <90 1>;  /* Unknown, TZ Protected */
> > +
> > +     bt_en_default: bt-en-sleep {
> > +             pins = "gpio116";
> > +             function = "gpio";
> > +             output-low;
> > +             bias-disable;
> > +             qcom,drive-strength = <16>;
>
> drop "qcom," and please keep the order of:
>
> pins
> function
> drive-strength
> bias
> output/input
>
> as you did below

Will do.

Should I also drop 'qcom,' from the 'misc_3p3_reg_en' and adjust order
the same way, or that one is somehow special?

>
> > +
> > +/ {
> > +     model = "ASUS Zenbook A14 UX3407RA";
>
> There's no strict policy, but variants usually go in braces

Parenthesis I guess, "ASUS Zenbook A14 (UX3407RA)" ?

>
> > +     compatible = "asus,x1e80100-zenbook-a14", "qcom,x1e80100";
> > +};
> > +
> > +&gpu_zap_shader {
> > +     firmware-name = "qcom/x1e80100/ASUSTeK/zenbook-a14/qcdxkmsuc8380.mbn";
> > +};
> > +
> > +&remoteproc_adsp {
> > +     firmware-name = "qcom/x1e80100/ASUSTeK/zenbook-a14/qcadsp8380.mbn",
> > +                     "qcom/x1e80100/ASUSTeK/zenbook-a14/adsp_dtbs.elf";
> > +
> > +     status = "okay";
> > +};
> > +
> > +&remoteproc_cdsp {
> > +     firmware-name = "qcom/x1e80100/ASUSTeK/zenbook-a14/qccdsp8380.mbn",
> > +                     "qcom/x1e80100/ASUSTeK/zenbook-a14/cdsp_dtbs.elf";
> > +
> > +     status = "okay";
> > +};
> > +
> > +&uart14 {
> > +     status = "okay";
> > +
> > +     bluetooth {
> > +             compatible = "qcom,wcn7850-bt";
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&bt_en_default>;
> > +             enable-gpios = <&tlmm 116 GPIO_ACTIVE_HIGH>;
> > +             max-speed = <3000000>;
> > +     };
> > +};
> > diff --git a/arch/arm64/boot/dts/qcom/x1p42100-asus-zenbook-a14.dts b/arch/arm64/boot/dts/qcom/x1p42100-asus-zenbook-a14.dts
> > new file mode 100644
> > index 000000000000..b6c9a707090f
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/x1p42100-asus-zenbook-a14.dts
> > @@ -0,0 +1,48 @@
> > +// SPDX-License-Identifier: BSD-3-Clause
> > +/*
> > + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
> > + * Copyright (c) 2025 Aleksandrs Vinarskis <alex.vinarskis@...il.com>
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "x1p42100.dtsi"
> > +#include "x1-zenbook-a14.dtsi"
> > +
> > +/delete-node/ &pmc8380_6;
> > +/delete-node/ &pmc8380_6_thermal;
> > +
> > +/ {
> > +     model = "ASUS Zenbook A14 UX3407QA";
> > +     compatible = "asus,x1p42100-zenbook-a14", "qcom,x1p42100";
> > +};
> > +
> > +&gpu_zap_shader {
> > +     firmware-name = "qcom/x1p42100/ASUSTeK/zenbook-a14/qcdxkmsuc8380.mbn";
> > +};
>
> This file is not going to work on this SoC, you can drop it

I guess it would need a different firmware name? If yes, can we
already add the new name, such that once x1p42100 gains GPU support it
will get enabled 'automatically'?

Otherwise, I will just drop it.

>
> > +
> > +&remoteproc_adsp {
> > +     firmware-name = "qcom/x1p42100/ASUSTeK/zenbook-a14/qcadsp8380.mbn",
> > +                     "qcom/x1p42100/ASUSTeK/zenbook-a14/adsp_dtbs.elf";
> > +
> > +     status = "okay";
> > +};
> > +
> > +&remoteproc_cdsp {
> > +     firmware-name = "qcom/x1p42100/ASUSTeK/zenbook-a14/qccdsp8380.mbn",
> > +                     "qcom/x1p42100/ASUSTeK/zenbook-a14/cdsp_dtbs.elf";
> > +
> > +     status = "okay";
>
> Are the DSP firmware files actually different between the two?

CDSP is the same. ADSP blobs to my surprise are different, both '.elf'
and '.mbn'. But like I wrote in the cover letter, perhaps Asus just
forgot to update adsp firmware? Though according to changelong on
device pages [2],[4] both have "ADSP Driver : 2.0.4135.0200"

Compared by:
* Downloading UX3407QA's drivers [1], from the device page [2] and
UX3407RA'a drivers [3] from the device page [4]
* Extract and flatten with `7z e filename.exe`
* Compare via `md5sum *dsp*elf *dsp*mbn *dsp*jsn`

Though, even if the blobs would be/will be the same, I think it is
still beneficial to define firmware path per model, as this makes
firmware extraction from driver/Windows partition and placement much
easier. Unfortunately, as it seems so far, most of the devices besides
Lenovos are not having firmware upstreamed, so this is pretty
relevant. Eg. Ubuntu already has 'firmware extracting tool' [5] (draft
MR to include Zenbook as well), I'm guessing other distros have
something similar, though I haven't followed up.

On the other hand, these tools could of course get path from device
tree directly, eg. via `cat
/sys/firmware/devicetree/base/soc@...emoteproc@...00000/firmware-name`,
then having all the blobs for the device in one location is less
relevant...

Thanks for reviewing,
Alex

[1] https://dlcdnets.asus.com/pub/ASUS/nb/Image/Driver/DriverPackage/42706/SOCPackage_forWebSite_Qualcomm_Z_V1.305.7550.2_42706.exe?model=UX3407QA
[2] https://www.asus.com/ch-en/laptops/for-home/zenbook/asus-zenbook-a14-ux3407/helpdesk_download?model2Name=UX3407QA
[3] https://dlcdnets.asus.com/pub/ASUS/nb/Image/Driver/DriverPackage/42705/SOCPackage_forWebSite_Qualcomm_Z_V1.305.7550.2_42705.exe?model=UX3407RA
[4] https://www.asus.com/ch-en/laptops/for-home/zenbook/asus-zenbook-a14-ux3407/helpdesk_download?model2Name=UX3407RA
[5] https://git.launchpad.net/~alexvinarskis/ubuntu/+source/qcom-firmware-extract/tree/qcom-firmware-extract?h=asus-zenbook-a14

>
> Konrad

Powered by blists - more mailing lists