Message-ID: <20240319000805.42ae84ac@minigeek.lan>
Date: Tue, 19 Mar 2024 00:08:05 +0000
From: Andre Przywara <andre.przywara@....com>
To: Kamil Kasperski <ressetkk@...il.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>, Maxime Ripard <mripard@...nel.org>,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH v3 3/3] arm64: dts: allwinner: h616: add support for T95
 tv boxes

On Tue, 19 Mar 2024 00:28:43 +0100
Kamil Kasperski <ressetkk@...il.com> wrote:

Hi Kamil,

> W dniu 18.03.2024 o 12:42, Andre Przywara pisze:
> > On Sun, 17 Mar 2024 20:44:51 +0100
> > Kamil Kasperski <ressetkk@...il.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.

That's alright, I was just curious. I think Jernej has such an early
box, and IIRC he used mostly the X96 Mate DT with it, so I was assuming
it has eMMC.
 
> >  
> >> Signed-off-by: Kamil Kasperski <ressetkk@...il.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@...il.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.

No, that's fine, I was just curious. But another thing to consider are
any GPIOs connected to the module, for reset and wakeup. Do you have a
dump of the vendor firmware DT somewhere? At least this information in
there seems reliable.
You should be able to access the loaded DTB via /sys/firmware/fdt, then
can decompile that with dtc. Look for properties ending in -gpios,
related to WiFi/WLAN.

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

Sure, using the SDIO ID it should autodetect it, and mainline Linux not
(yet) supporting it is strictly speaking not a reason to not describe
it in the DT - only that we would like to test it somehow, of course.
So if in doubt leave it as it is, that should not hurt in any case.

> >> +};
> >> +
> >> +&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.

No, again that's fine from a pure DT perspective. The information above
seems accurate. But again there might be GPIOs at play here, something
to look out for in the vendor DT.
In my experience the BT seem to be easier to support. Even for combined
modules you might have luck and see BT working, even when WiFi does not.
But again, just leave it as it is now, we can always add that later,
that should not block the submission.

> >> +};
> >> +
> >> +&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@...il.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.

Thanks!

Cheers,
Andre
  
> >> +
> >> +		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>;
> >> +};
> >>  
> 
> 

Powered by blists - more mailing lists