Message-ID: <20240919115923.6869adfb@donnerap.manchester.arm.com>
Date: Thu, 19 Sep 2024 11:59:23 +0100
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 v4 3/3] arm64: dts: allwinner: h616: add support for T95
 tv boxes

On Wed, 20 Mar 2024 00:33:50 +0100
Kamil Kasperski <ressetkk@...il.com> wrote:

Hi Kamil,

> W dniu 20.03.2024 o 00:22, Andre Przywara pisze:
> > On Tue, 19 Mar 2024 18:50:24 +0100
> > Kamil Kasperski <ressetkk@...il.com> wrote:
> >
> > Hi Kamil,
> >  
> >> 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.
> >>
> >> Signed-off-by: Kamil Kasperski <ressetkk@...il.com>  
> > thanks for the changes, looks good now, although a bit minimal ;-)
> >
> > Reviewed-by: Andre Przywara <andre.przywara@....com>
> >
> > Please can you wait till the -rc1 release on Sunday, and send a rebased
> > version next week? There is a small merge conflict in the dts Makefile
> > as of now.
> >
> > Cheers,
> > Andre  
> 
> Sure, no problem. Thank you very much for a review.

It's been a while, but can you maybe send a rebased version of this
patch set again? The merge window has just opened, so exactly now is
probably not a good time, but if you rebase on v6.12-rc1, due to be
released on the 29th September, we can get this queued.

Some minor things I just spotted below:

> Once it gets merged I'll get back to u-boot patch.
> 
> Cheers,
> Kamil
> 
> >  
> >> ---
> >>  arch/arm64/boot/dts/allwinner/Makefile             |   1 +
> >>  arch/arm64/boot/dts/allwinner/sun50i-h616-t95.dtsi | 109 +++++++++++++++++++++
> >>  .../dts/allwinner/sun50i-h616-t95max-axp313.dts    |  84 ++++++++++++++++
> >>  3 files changed, 194 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..4c02408733bc
> >> --- /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;

Can you please add a line:
		vin-supply = <&reg_vcc5v>;
here, to not leave this regulator dangling? That should also suppress a
warning about a dummy regulator.

> >> +	};
> >> +
> >> +	wifi_pwrseq: pwrseq {
> >> +		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";

Please add a:
	disable-wp;
here, as microSD slots don't have a write-protect switch. We are in the
process of fixing all sunxi arm64 boards in this respect, but this DT would
probably miss that effort.

> >> +};
> >> +
> >> +&mmc1 {
> >> +	mmc-pwrseq = <&wifi_pwrseq>;
> >> +	bus-width = <4>;
> >> +	non-removable;
> >> +	status = "okay";
> >> +
> >> +	sdio_wifi: wifi@1 {
> >> +		reg = <1>;
> >> +	};
> >> +};
> >> +
> >> +&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";

As Jernej mentioned: this is probably for connecting to the Bluetooth part
of the WiFi chip, so please add a least a comment here. And I wonder if we
know what Bluetooth IP this is, and if there is a compatible string for
that? Does the vendor firmware give any clue here?

Cheers,
Andre

> >> +};
> >> +
> >> +&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..08a6b4fcc235
> >> --- /dev/null
> >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616-t95max-axp313.dts
> >> @@ -0,0 +1,84 @@
> >> +// 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;
> >> +
> >> +		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_aldo1>;
> >> +	vcc-ph-supply = <&reg_dldo1>;
> >> +	vcc-pi-supply = <&reg_dldo1>;
> >> +};
> >>  
> 

Powered by blists - more mailing lists