Re: [PATCH v3 3/3] arm64: dts: allwinner: h616: add support for T95 tv boxes

[Kamil Kasperski <ressetkk@gmail.com>]

W dniu 18.03.2024 o 12:42, Andre Przywara pisze:
> On Sun, 17 Mar 2024 20:44:51 +0100
> Kamil Kasperski <ressetkk@gmail.com> wrote:
>
> Hi Kamil,
>
> thanks a lot for putting together those patches and sending them
> for upstream inclusion!
>
>> Add dtsi file for T95 tv boxes and add initial support for T95 5G AXP313A
>> variant with a board name H616-T95MAX-AXP313A-v3.0 Internal storage is not
>> accessible due to lack of support for H616 NAND controller.
> Them using raw NAND is really unfortunate. I think the original T95 box
> used eMMC?

I'm not sure. I don't have access to the other revision of this box.

>
>> Signed-off-by: Kamil Kasperski <ressetkk@gmail.com>
>> ---
>>  arch/arm64/boot/dts/allwinner/Makefile             |   1 +
>>  arch/arm64/boot/dts/allwinner/sun50i-h616-t95.dtsi | 109 +++++++++++++++++++++
>>  .../dts/allwinner/sun50i-h616-t95max-axp313.dts    |  85 ++++++++++++++++
>>  3 files changed, 195 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
>> index 21149b346a60..294921f12b73 100644
>> --- a/arch/arm64/boot/dts/allwinner/Makefile
>> +++ b/arch/arm64/boot/dts/allwinner/Makefile
>> @@ -42,6 +42,7 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-tanix-tx6-mini.dtb
>>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-bigtreetech-cb1-manta.dtb
>>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-bigtreetech-pi.dtb
>>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-orangepi-zero2.dtb
>> +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-t95max-axp313.dtb
>>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-x96-mate.dtb
>>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-longanpi-3h.dtb
>>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero2w.dtb
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616-t95.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616-t95.dtsi
>> new file mode 100644
>> index 000000000000..815cf2dac24b
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616-t95.dtsi
>> @@ -0,0 +1,109 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (C) 2024 Kamil Kasperski <ressetkk@gmail.com>
>> + *
>> + * Common DT nodes for H616-based T95 TV boxes
>> + * There are two versions reported with different PMIC variants.
>> + */
>> +
>> +#include "sun50i-h616.dtsi"
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +/ {
>> +	aliases {
>> +		ethernet1 = &sdio_wifi;
>> +		serial0 = &uart0;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial0:115200n8";
>> +	};
>> +
>> +	reg_vcc5v: vcc5v {
>> +		/* board wide 5V supply directly from the DC input */
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc-5v";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +		regulator-always-on;
>> +	};
>> +
>> +	reg_vcc3v3: vcc3v3 {
>> +		/* discrete 3.3V regulator */
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc-3v3";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		regulator-always-on;
>> +	};
>> +
>> +	wifi_pwrseq: wifi-pwrseq {
> Krzysztof recently sent a patch to just use "pwrseq" as the node name,
> so can you do the same here?
>
> https://lore.kernel.org/linux-sunxi/20240317184130.157695-2-krzysztof.kozlowski@linaro.org/T/#u

Sure. I'll send v4.

>
>> +		compatible = "mmc-pwrseq-simple";
>> +		clocks = <&rtc CLK_OSC32K_FANOUT>;
>> +		clock-names = "ext_clock";
>> +		pinctrl-0 = <&x32clk_fanout_pin>;
>> +		pinctrl-names = "default";
>> +		reset-gpios = <&pio 6 18 GPIO_ACTIVE_LOW>; /* PG18 */
>> +	};
>> +};
>> +
>> +&ehci0 {
>> +	status = "okay";
>> +};
>> +
>> +&ehci2 {
>> +	status = "okay";
>> +};
>> +
>> +&ir {
>> +	status = "okay";
>> +};
>> +
>> +&mmc0 {
>> +	cd-gpios = <&pio 8 16 GPIO_ACTIVE_LOW>;	/* PI16 */
>> +	bus-width = <4>;
>> +	status = "okay";
>> +};
>> +
>> +&mmc1 {
>> +	mmc-pwrseq = <&wifi_pwrseq>;
>> +	bus-width = <4>;
>> +	non-removable;
>> +	status = "okay";
>> +
>> +	sdio_wifi: wifi@1 {
>> +		reg = <1>;
>> +	};
> So does the WiFi work with mainline drivers? IIUC the BCM43342 is not
> supported by the existing Broadcom drivers?

It's actually BCM43342/1 It System doesn't detect this chip by default.
The most relevant message from dmesg is:
[   14.042035] kernel: brcmfmac: brcmf_fw_alloc_request: Unknown chip BCM43342/1
I believe that it's only a matter of missing module. I don't think it is
supported in mainline ATM. I left it to have a wi-fi node accessible
and detectable by kernel. If you think that it's better to remove the
node if it's not supported I can do it.

Somebody actually extracted modified precompiled module from custom
5.15.16 rockchip kernel, which implements support for this card. There's no
patch for it that could be submitted to mainline unfortunately ATM.

I've found a patch that adds chip id strings to brcmfmac, but I would like to
test it beforehand.

>
>> +};
>> +
>> +&ohci0 {
>> +	status = "okay";
>> +};
>> +
>> +&ohci2 {
>> +	status = "okay";
>> +};
>> +
>> +&uart0 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&uart0_ph_pins>;
>> +	status = "okay";
>> +};
>> +
>> +&uart1 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
>> +	uart-has-rtscts;
>> +	status = "okay";
> Similar question here, what is the Bluetooth situation in mainline? I
> guess it's not supported, since you didn't put a BT node in here?

I haven't tested that yet. It's partially due to lack of experience with
DTS. I would like to figure out mainline support eventually, but for now
I would like to just have a working foundation for these TVB.

If that's also not really a wanted option to leave those nodes, I can
remove them for now.

>
>> +};
>> +
>> +&usbotg {
>> +	dr_mode = "host";	/* USB A type receptable */
>> +	status = "okay";
>> +};
>> +
>> +&usbphy {
>> +	status = "okay";
>> +};
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616-t95max-axp313.dts b/arch/arm64/boot/dts/allwinner/sun50i-h616-t95max-axp313.dts
>> new file mode 100644
>> index 000000000000..c8650aca2407
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616-t95max-axp313.dts
>> @@ -0,0 +1,85 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (C) 2024 Kamil Kasperski <ressetkk@gmail.com>
>> + *
>> + * Configuration for T95 TV box with board label H616-T95MAX-AXP313A-v3.0
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include "sun50i-h616-t95.dtsi"
>> +
>> +/ {
>> +	model = "T95 5G (AXP313)";
>> +	compatible = "t95,t95max-axp313", "allwinner,sun50i-h616";
>> +};
>> +
>> +&mmc0 {
>> +	vmmc-supply = <&reg_dldo1>;
>> +};
>> +
>> +&mmc1 {
>> +	vmmc-supply = <&reg_dldo1>;
>> +	vqmmc-supply = <&reg_aldo1>;
>> +};
>> +
>> +&r_i2c {
>> +	status = "okay";
>> +
>> +	axp313: pmic@36 {
>> +		compatible = "x-powers,axp313a";
>> +		reg = <0x36>;
>> +		#interrupt-cells = <1>;
>> +		interrupt-controller;
>> +		interrupt-parent = <&pio>;
> I don't think you need interrupt-parent unless you also actually specify
> an interrupt line.
> (But please keep #interrupt-cells and interrupt-controller.)

I think you may be right. I based this part off of OPiZ3 dts, rather than
transpeed. Will update in the v4.

>
>> +
>> +		vin1-supply = <&reg_vcc5v>;
>> +		vin2-supply = <&reg_vcc5v>;
>> +		vin3-supply = <&reg_vcc5v>;
>> +
>> +		regulators {
>> +			reg_aldo1: aldo1 {
>> +				regulator-always-on;
>> +				regulator-min-microvolt = <1800000>;
>> +				regulator-max-microvolt = <1800000>;
>> +				regulator-name = "vcc1v8";
>> +			};
>> +
>> +			reg_dldo1: dldo1 {
>> +				regulator-always-on;
>> +				regulator-min-microvolt = <3300000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-name = "vcc3v3";
>> +			};
>> +
>> +			reg_dcdc1: dcdc1 {
>> +				regulator-always-on;
>> +				regulator-min-microvolt = <810000>;
>> +				regulator-max-microvolt = <990000>;
>> +				regulator-name = "vdd-gpu-sys";
>> +			};
>> +
>> +			reg_dcdc2: dcdc2 {
>> +				regulator-always-on;
>> +				regulator-min-microvolt = <810000>;
>> +				regulator-max-microvolt = <1100000>;
>> +				regulator-name = "vdd-cpu";
>> +			};
>> +
>> +			reg_dcdc3: dcdc3 {
>> +				regulator-always-on;
>> +				regulator-min-microvolt = <1500000>;
>> +				regulator-max-microvolt = <1500000>;
>> +				regulator-name = "vdd-dram";
>> +			};
>> +		};
>> +	};
>> +};
>> +
>> +&pio {
>> +	vcc-pc-supply = <&reg_aldo1>;
>> +	vcc-pf-supply = <&reg_dldo1>;
>> +	vcc-pg-supply = <&reg_dldo1>;
> So if vqmmc-supply for MMC1 is at the 1.8V from ALDO1, that must mean that
> the whole of PortG is at 1.8V, right? So I think this would need to be
> reg_aldo1 here.
>
> Apart from those minor things it looks good to me.
>
> Cheers,
> Andre

Good catch. Will update in v4.

Cheers,
Kamil

>> +	vcc-ph-supply = <&reg_dldo1>;
>> +	vcc-pi-supply = <&reg_dldo1>;
>> +};
>>