linux-kernel - Re: [PATCH 2/2] arm64: dts: allwinner: h313: Add Amediatech X96Q

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aMhcxcBYF2vMjd5g@probook>
Date: Mon, 15 Sep 2025 18:36:56 +0000
From: J. Neuschäfer <j.ne@...teo.net>
To: Chen-Yu Tsai <wens@...e.org>
Cc: Andre Przywara <andre.przywara@....com>,
	J. Neuschäfer via B4 Relay <devnull+j.ne.posteo.net@...nel.org>,
	j.ne@...teo.net, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Samuel Holland <samuel@...lland.org>,
	Maxime Ripard <mripard@...nel.org>, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] arm64: dts: allwinner: h313: Add Amediatech X96Q

On Sat, Sep 13, 2025 at 05:36:12PM +0800, Chen-Yu Tsai wrote:
> On Fri, Sep 12, 2025 at 5:54 PM Andre Przywara <andre.przywara@....com> wrote:
> >
> > On Fri, 12 Sep 2025 01:52:10 +0200
> > J. Neuschäfer via B4 Relay <devnull+j.ne.posteo.net@...nel.org> wrote:
> >
> > Hi,
> >
> > many thanks for posting the DT, I really wish more people would do that!
> >
> > > From: "J. Neuschäfer" <j.ne@...teo.net>
> > >
> > > The X96Q is a set-top box with an H313 SoC, AXP305 PMIC, 1 or 2 GiB RAM,
> > > 8 or 16 GiB eMMC flash, 2x USB A, Micro-SD, HDMI, Ethernet, audio/video
> > > output, and infrared input.
> > >
> > >   https://x96mini.com/products/x96q-tv-box-android-10-set-top-box
> > >
> > > Tested, works:
> > > - debug UART
> > > - status LED
> > > - USB ports in host mode
> > > - MicroSD
> > > - eMMC
> > > - recovery button hidden behind audio/video port
> > > - analog audio (line out)
> > >
> > > Does not work:
> > > - Ethernet (requires AC200 MFD/EPHY driver)
> > > - analog video output (requires AC200 driver)
> > > - HDMI audio/video output
> > >
> > > Untested:
> > > - "OTG" USB port in device mode
> > > - built-in IR receiver
> > > - external IR receiver
> > > - WLAN (requires out-of-tree XRadio driver)
> > >
> > > Table of regulators on the downstream kernel, for reference:
> > >
> > >  vcc-5v      1   15      0 unknown  5000mV     0mA  5000mV  5000mV
> > >     dcdca    0    0      0 unknown   900mV     0mA     0mV     0mV
> > >     dcdcb    0    0      0 unknown  1350mV     0mA     0mV     0mV
> > >     dcdcc    0    0      0 unknown   900mV     0mA     0mV     0mV
> > >     dcdcd    0    0      0 unknown  1500mV     0mA     0mV     0mV
> > >     dcdce    0    0      0 unknown  3300mV     0mA     0mV     0mV
> > >     aldo1    0    0      0 unknown  3300mV     0mA     0mV     0mV
> > >     aldo2    0    0      0 unknown   700mV     0mA     0mV     0mV
> > >     aldo3    0    0      0 unknown   700mV     0mA     0mV     0mV
> > >     bldo1    0    0      0 unknown  1800mV     0mA     0mV     0mV
> > >     bldo2    0    0      0 unknown  1800mV     0mA     0mV     0mV
> > >     bldo3    0    0      0 unknown   700mV     0mA     0mV     0mV
> > >     bldo4    0    0      0 unknown   700mV     0mA     0mV     0mV
> > >     cldo1    0    0      0 unknown  2500mV     0mA     0mV     0mV
> > >     cldo2    0    0      0 unknown   700mV     0mA     0mV     0mV
> > >     cldo3    0    0      0 unknown   700mV     0mA     0mV     0mV
> > >
> > > Signed-off-by: J. Neuschäfer <j.ne@...teo.net>
> > > ---
[...]
> > > +&mmc0 {
> > > +     vmmc-supply = <&reg_aldo1>;
> > > +     cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;  /* PF6 */
> > > +     disable-wp;
> > > +     bus-width = <4>;
> > > +     max-frequency = <150000000>;
> >
> > That line is already in the .dtsi file, so redundant.
> >
> > > +     status = "okay";
> > > +     /* µSD */
> >
> > If we really need this comment, it should be above, right after the
> > "&mmc0 {". And I wonder if it should be "microSD" instead.
> 
> Yes. Please use ASCII in the code if possible, since some of us have
> setups that don't quite work well with extended character sets.

ACK

> 
> > > +};
> > > +
> > > +&mmc1 {
> > > +     /* TODO: XRadio XR819 WLAN */
> >
> > Either you just keep the comment, an mention mmc1, but don't reference the
> > node, or you add the properties that you know of already, like
> > vmmc-supply, vqmmc-supply, mmc-pwrseq, bus-width, non-removable.
> > But this "empty reference with a comment" is somewhat odd.
> 
> I'd say just fill it in completely so that the mmc host is enabled and
> the SDIO card is detected. Missing driver support for the chip is a
> different issue, but since this is an enumerable bus it shouldn't prevent
> you from describing everything already.

I gave it a try just now, but I wasn't successful with enumeration:

	&mmc1 {
		/* XRadio XR819 WLAN */
		vmmc-supply = <&reg_aldo1>;
		vqmmc-supply = <&reg_bldo1>;
		mmc-pwrseq = <&wifi_pwrseq>;
		bus-width = <4>;
		non-removable;
		mmc-ddr-1_8v;
		status = "okay";
	};

The result is unsuccessful (with or without the questionable mmc-ddr-1_8v):

	[    1.607511] mmc1: Failed to initialize a non-removable card

The downstream DT mentions a few relevant properties:

	/wlan {
		wlan_regon = <&pio 6 18 GPIO_ACTIVE_LOW>;
		pinctrl-names = "default";
		pinctrl-0 = <&losc>;
	};
	...
	losc: clk_losc@0 {
		linux,phandle = <0xd3>;
		phandle = <0xd3>;
		allwinner,drive = <0x02>;
		allwinner,function = "x32kfout";
		allwinner,muxsel = <0x03>;
		allwinner,pins = "PG10";
		allwinner,pull = <0x01>;
	};

Translating this (roughly) into mainline bindings:

	mmc-pwrseq = <&wifi_pwrseq>;
	...

	wifi_pwrseq: pwrseq {
		compatible = "mmc-pwrseq-emmc";
		reset-gpios = <&pio 6 18 GPIO_ACTIVE_LOW>;
		clocks = <&rtc CLK_OSC32K_FANOUT>;
		clock-names = "ext_clock";
		pinctrl-names = "default";
		pinctrl-0 = <&x32clk_fanout_pin>;
	};

... it fails in drivers/reset/core.c, because __reset_add_reset_gpio_device
doesn't handle #gpio-cells = <3>, which is the case for &pio:

	/*
	 * Currently only #gpio-cells=2 is supported with the meaning of:
	 * args[0]: GPIO number
	 * args[1]: GPIO flags
	 * TODO: Handle other cases.
	 */
	if (args->args_count != 2)
		return -ENOENT;

Relatedly, I'd expect this limitation to break WiFi on sun50i-h313-tanix-tx1.dts
(upstreamed by Andre) as well.

> > > +};
> > > +
> > > +&mmc2 {
> > > +     vmmc-supply = <&reg_aldo1>;
> > > +     vqmmc-supply = <&reg_bldo1>;
> > > +     non-removable;
> > > +     cap-mmc-hw-reset;
> > > +     mmc-ddr-1_8v;
> > > +     mmc-hs200-1_8v;
> > > +     bus-width = <8>;
> > > +     max-frequency = <100000000>;
> >
> > Are you sure you need that?

After some more testing, it turns out that I do need to limit the
frequency to 100 MHz. At the default of 150 MHz, stable operation of
the eMMC isn't possible (all other properties being as they are).

> >
> > > +     status = "okay";
> > > +     /* eMMC */
> >
> > Please move that comment up.
> 
> I don't think it's necessary though. hs200 and 8-bit width would make
> it obvious that it's an eMMC.

Not necessary, but perhaps still useful for someone having a quick
glance at the devicetree, so I'd prefer to keep it in.


Best regards,
J. Neuschäfer