linux-kernel - Re: [PATCH v3 3/5] ARM: dts: sunxi: add support for NetCube Systems Nagami SoM

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: <3700020.R56niFO833@lukas-hpz440workstation>
Date: Tue, 08 Jul 2025 17:19:31 +0200
From: Lukas Schmid <lukas.schmid@...cube.li>
To: Andre Przywara <andre.przywara@....com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
 Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>,
 Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Alexandre Ghiti <alex@...ti.fr>, 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,
 linux-riscv@...ts.infradead.org
Subject:
 Re: [PATCH v3 3/5] ARM: dts: sunxi: add support for NetCube Systems Nagami
 SoM

On Dienstag, 8. Juli 2025 01:22:05 CEST Andre Przywara wrote:
> On Mon,  7 Jul 2025 20:44:15 +0200
> Lukas Schmid <lukas.schmid@...cube.li> wrote:
> 
Hi Andre,

> Hi Lukas,
> 
> please try to refrain from sending subsequent patches too quickly, that
> might just put off and confuse reviewers. To acknowledge a change
> request, it is probably sufficient to just reply to the mail with a
> confirmation.

Sorry about that. I'm still trying to get the hang of this whole "submitting 
patches" thing. What is a good time/reason to send the next revision/version 
of a patchset?

> 
> > NetCube Systems Nagami SoM is a module based around the Allwinner T113s
> > SoC. It includes the following features and interfaces:
> > 
> > - 128MB DDR3 included in SoC
> > - 10/100 Mbps Ethernet using LAN8720A phy
> > - One USB-OTG interface
> > - One USB-Host interface
> > - One I2S interface with in and output support
> > - Two CAN interfaces
> > - ESP32 over SDIO
> > - One SPI interface
> > - I2C EEPROM for MAC address
> > - One QWIIC I2C Interface with dedicated interrupt pin shared with EEPROM
> > - One external I2C interface
> > - SD interface for external SD-Card
> > 
> > Signed-off-by: Lukas Schmid <lukas.schmid@...cube.li>
> > ---
> > 
> >  .../allwinner/sun8i-t113s-netcube-nagami.dtsi | 233 ++++++++++++++++++
> >  1 file changed, 233 insertions(+)
> >  create mode 100644
> >  arch/arm/boot/dts/allwinner/sun8i-t113s-netcube-nagami.dtsi> 
> > diff --git a/arch/arm/boot/dts/allwinner/sun8i-t113s-netcube-nagami.dtsi
> > b/arch/arm/boot/dts/allwinner/sun8i-t113s-netcube-nagami.dtsi new file
> > mode 100644
> > index 000000000..0db867b47
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/allwinner/sun8i-t113s-netcube-nagami.dtsi
> > @@ -0,0 +1,233 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright (C) 2025 Lukas Schmid <lukas.schmid@...cube.li>
> > + */
> > +
> > +/dts-v1/;
> > +#include "sun8i-t113s.dtsi"
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +/ {
> > +	model = "NetCube Systems Nagami SoM";
> > +	compatible = "netcube,nagami", "allwinner,sun8i-t113s";
> > +
> > +	aliases {
> > +		serial1 = &uart1; // ESP32 Bootloader UART
> > +		serial3 = &uart3; // Console UART on Card Edge
> > +		ethernet0 = &emac;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial3:115200n8";
> > +	};
> > +
> > +	/* module wide 3.3V supply directly from the card edge */
> > +	reg_vcc3v3: regulator-3v3 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc-3v3";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		regulator-always-on;
> > +	};
> > +
> > +	/* SY8008 DC/DC regulator on the board, also supplying VDD-SYS */
> > +	reg_vcc_core: regulator-core {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc-core";
> > +		regulator-min-microvolt = <880000>;
> > +		regulator-max-microvolt = <880000>;
> > +		vin-supply = <&reg_vcc3v3>;
> > +	};
> > +};
> > +
> > +&cpu0 {
> > +	cpu-supply = <&reg_vcc_core>;
> > +};
> > +
> > +&cpu1 {
> > +	cpu-supply = <&reg_vcc_core>;
> > +};
> > +
> > +&dcxo {
> > +	clock-frequency = <24000000>;
> > +};
> > +
> > +&emac {
> > +	nvmem-cells = <&eth0_macaddress>;
> > +	nvmem-cell-names = "mac-address";
> > +	phy-handle = <&lan8720a>;
> > +	phy-mode = "rmii";
> > +	pinctrl-0 = <&rmii_pe_pins>;
> > +	pinctrl-names = "default";
> > +	status = "okay";
> > +};
> > +
> > +/* Exposed as I2C on the card edge */
> > +&i2c2 {
> > +	pinctrl-0 = <&i2c2_pins>;
> > +	pinctrl-names = "default";
> 
> I wonder if this belongs here. In general we don't describe
> pins/devices that are on generic connectors (like pin headers), because
> the connection is determined by the user, not by the board.
> In this case PD20 and PD21 could be used as generic GPIOs or as PWMs.
> IIUC, even for the basic carrier those pins end up on headers, so even
> there I wouldn't describe it. On the keypad carrier it's of course a
> different story, since the MCP23008 is apparently soldered there.

I get that part. However the module's description (as of yet unreleased) 
specifies those pins for this exact function as they are meant to be available 
on our modules using the same connector but other CPU/SoC. The Module's state 
that this is the default option but other function's can be reconfigured or 
they can be used as IO. 

If it's an issue I can remove those here, but I would still add them into the 
basic carrier as, even though they are only pin header, they are marked with 
the dedicated function of said pin. For example the headers there are 
specifically for "SPI", "I2S", "I2C" and the SD-Card slot.

> 
> > +};
> > +
> > +/* Exposed as the QWIIC connector and used by the internal EEPROM */
> > +&i2c3 {
> > +	pinctrl-0 = <&i2c3_pins>;
> > +	pinctrl-names = "default";
> > +	status = "okay";
> > +
> > +	eeprom0: eeprom@50 {
> > +		compatible = "atmel,24c02";		/* 
actually it's a 24AA02E48 */
> > +		reg = <0x50>;
> > +		pagesize = <16>;
> > +		read-only;
> > +		vcc-supply = <&reg_vcc3v3>;
> > +
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +
> > +		eth0_macaddress: macaddress@fa {
> > +			reg = <0xfa 0x06>;
> > +		};
> > +	};
> > +};
> > +
> > +/* Exposed as I2S on the card edge */
> > +&i2s1 {
> > +	pinctrl-0 = <&i2s1_pins>, <&i2s1_din_pins>, <&i2s1_dout_pins>;
> > +	pinctrl-names = "default";
> 
> Same story here, what prevents a user from using those edge pins as
> GPIO or PWM?
> 
> > +};
> > +
> > +/* Phy is on SoM. MDI signals pre-magentics are on the card edge */
> > +&mdio {
> > +	lan8720a: ethernet-phy@0 {
> > +		compatible = "ethernet-phy-ieee802.3-c22";
> > +		reg = <0>;
> > +	};
> > +};
> > +
> > +/* Exposed as SD on the card edge */
> > +&mmc0 {
> > +	pinctrl-0 = <&mmc0_pins>;
> > +	pinctrl-names = "default";
> > +	vmmc-supply = <&reg_vcc3v3>;
> > +	broken-cd;
> > +	disable-wp;
> > +	bus-width = <4>;
> > +};
> 
> I think this node doesn't belong into the SoM .dtsi, as many details
> are not set here, but on the carrier board: card detect, write protect,
> Vmmc supply, and even bus width (could be 1 as well). So please move
> this node to where the SD card connector sits. You might want to keep
> the pinctrl nodes in here.

Totally agree here, I will move this definition to the basic carrier, as it's 
currently the only one with the SD-Interface connected. However my question 
from above about the default function still applies here.
> 
> > +
> > +/* Connected to the on-board ESP32 */
> > +&mmc1 {
> > +	pinctrl-0 = <&mmc1_pins>;
> > +	pinctrl-names = "default";
> > +	vmmc-supply = <&reg_vcc3v3>;
> > +	bus-width = <4>;
> > +	non-removable;
> > +	status = "okay";
> > +};
> > +
> > +/* Connected to the on-board eMMC */
> > +&mmc2 {
> > +	pinctrl-0 = <&mmc2_pins>;
> > +	pinctrl-names = "default";
> > +	vmmc-supply = <&reg_vcc3v3>;
> > +	vqmmc-supply = <&reg_vcc3v3>;
> > +	bus-width = <4>;
> > +	non-removable;
> > +	status = "okay";
> > +};
> > +
> > +&pio {
> > +	vcc-pb-supply = <&reg_vcc3v3>;
> > +	vcc-pc-supply = <&reg_vcc3v3>;
> > +	vcc-pd-supply = <&reg_vcc3v3>;
> > +	vcc-pe-supply = <&reg_vcc3v3>;
> > +	vcc-pf-supply = <&reg_vcc3v3>;
> > +	vcc-pg-supply = <&reg_vcc3v3>;
> > +
> > +	gpio-line-names = "", "", "", "", // PA
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "CAN0_TX", 
"CAN0_RX", // PB
> > +					  "CAN1_TX", 
"CAN1_RX", "UART3_TX", "UART3_RX",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "eMMC_CLK", 
"eMMC_CMD", // PC
> > +					  "eMMC_D2", 
"eMMC_D1", "eMMC_D0", "eMMC_D3",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "", // 
PD
> > +					  "", "", "", "",
> > +					  "", "", "SPI1_CS", 
"SPI1_CLK",
> > +					  "SPI1_MOSI", 
"SPI1_MISO", "SPI1_HOLD", "SPI1_WP",
> > +					  "PD16", "", "", "",
> > +					  "I2C2_SCL", 
"I2C2_SDA", "PD22", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "ETH_CRSDV", 
"ETH_RXD0", "ETH_RXD1", "ETH_TXCK", // PE
> > +					  "ETH_TXD0", 
"ETH_TXD1", "ETH_TXEN", "",
> > +					  "ETH_MDC", 
"ETH_MDIO", "QWIIC_nINT", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "SD_D1", "SD_D0", 
"SD_CLK", "SD_CLK", // PF
> > +					  "SD_D3", "SD_D2", 
"PF6", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "ESP_CLK", 
"ESP_CMD", "ESP_D0", "ESP_D1", // PG
> > +					  "ESP_D2", "ESP_D3", 
"UART1_TXD", "UART1_RXD",
> > +					  "ESP_nBOOT", 
"ESP_nRST", "I2C3_SCL", "I2C3_SDA",
> > +					  "I2S1_WS", 
"I2S1_CLK", "I2S1_DIN0", "I2S1_DOUT0",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "",
> > +					  "", "", "", "";
> > +};
> > +
> > +/* Remove the unused CK pin from the pinctl as it is unconnected */
> > +&rmii_pe_pins {
> > +	pins = "PE0", "PE1", "PE2", "PE3", "PE4",
> > +		   "PE5", "PE6", "PE8", "PE9";
> > +};
> > +
> > +/* Exposed as SPI on the card-edge */
> > +&spi1 {
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +	pinctrl-0 = <&spi1_pins>;
> > +	pinctrl-names = "default";
> > +	cs-gpios = <0>;
> > +};
> 
> I wonder if this belongs here as well, since it's again just generic
> edge pins.

Same here too
> 
> Cheers,
> Andre
> 
> > +
> > +/* Connected to the Bootloader/Console of the ESP32 */
> > +&uart1 {
> > +	pinctrl-0 = <&uart1_pg6_pins>;
> > +	pinctrl-names = "default";
> > +	status = "okay";
> > +};
> > +
> > +/* Exposed as the Console/Debug UART on the card-edge */
> > +&uart3 {
> > +	pinctrl-0 = <&uart3_pb_pins>;
> > +	pinctrl-names = "default";
> > +	status = "okay";
> > +};


Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)