[PATCH v3 0/4] Add support for NetCube Systems Kumquat

[PATCH v3 0/4] Add support for NetCube Systems Kumquat

[Lukas Schmid]

This series adds dt-bindings and dts for the NetCube Systems Kumquat board.

Changes in v2:
  Fix the devicetrees according to the "dt-schema" and "make dtbs_check W=1"
  Fix the License of the devicetree as requested
  Create a cover letter for the patch series

Changes in v3:
  Disable rtc inside the SoC again, as the rtc does not work
  Add the gpio-reserved-ranges property to the pinctrl bindings
  Reorder the nodes in the devicetree to match the order of the nodes in the
  sun8i-v3s.dtsi file

Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
---
Lukas Schmid (4):
  dt-bindings: vendor-prefixes: Add NetCube Systems Austria name
  dt-bindings: arm: sunxi: Add NetCube Systems Kumquat board
  dt-bindings: pinctrl: sunxi: Add gpio-reserved-ranges property
  ARM: dts: sunxi: add support for NetCube Systems Kumquat

 .../devicetree/bindings/arm/sunxi.yaml        |   5 +
 .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml  |   5 +
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 arch/arm/boot/dts/allwinner/Makefile          |   2 +
 .../allwinner/sun8i-v3s-netcube-kumquat.dts   | 290 ++++++++++++++++++
 arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi    |   6 +
 6 files changed, 310 insertions(+)
 create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts

-- 
2.47.1

[PATCH v3 1/4] dt-bindings: vendor-prefixes: Add NetCube Systems Austria name

[Lukas Schmid]

Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index da01616802c7..a30ed9547098 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1027,6 +1027,8 @@ patternProperties:
     description: Neofidelity Inc.
   "^neonode,.*":
     description: Neonode Inc.
+  "^netcube,.*":
+    description: NetCube Systems Austria
   "^netgear,.*":
     description: NETGEAR
   "^netlogic,.*":
-- 
2.47.1

Re: [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add NetCube Systems Austria name

[Andre Przywara]

On Fri,  3 Jan 2025 20:45:17 +0000
Lukas Schmid <lukas.schmid@netcube.li> wrote:

> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>

double Signed-off-by:

Also can please add a single line briefly saying what the company does
or produce, maybe add an URL?
In general some text in the body of the commit message is always
appreciated, even if it repeats the subject line (check the other
commits to that file).

Cheers,
Andre

> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index da01616802c7..a30ed9547098 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1027,6 +1027,8 @@ patternProperties:
>      description: Neofidelity Inc.
>    "^neonode,.*":
>      description: Neonode Inc.
> +  "^netcube,.*":
> +    description: NetCube Systems Austria
>    "^netgear,.*":
>      description: NETGEAR
>    "^netlogic,.*":

Re: [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add NetCube Systems Austria name

[Krzysztof Kozlowski]

On Fri, Jan 03, 2025 at 08:45:17PM +0000, Lukas Schmid wrote:
> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>


Repeating again one of my comments:

Please run scripts/checkpatch.pl and fix reported warnings. After that,
run also 'scripts/checkpatch.pl --strict' and (probably) fix more
warnings. Some warnings can be ignored, especially from --strict run,
but the code here looks like it needs a fix. Feel free to get in touch
if the warning is not clear.

Please implement all comments.

Best regards,
Krzysztof

Re: [PATCH v3 1/4] dt-bindings: vendor-prefixes: Add NetCube Systems Austria name

[Krzysztof Kozlowski]

On 04/01/2025 10:30, Krzysztof Kozlowski wrote:
> On Fri, Jan 03, 2025 at 08:45:17PM +0000, Lukas Schmid wrote:
>> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
>>
>> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
> 
> 
> Repeating again one of my comments:
> 
> Please run scripts/checkpatch.pl and fix reported warnings. After that,
> run also 'scripts/checkpatch.pl --strict' and (probably) fix more
> warnings. Some warnings can be ignored, especially from --strict run,
> but the code here looks like it needs a fix. Feel free to get in touch
> if the warning is not clear.
> 
> Please implement all comments.

My bad, I did not write that hint earlier.

Anyway, your patchset - all four patches - has multiple issues so you
need to fix all of them before posting next version.

Best regards,
Krzysztof

[PATCH v3 2/4] dt-bindings: arm: sunxi: Add NetCube Systems Kumquat board

[Lukas Schmid]

Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
---
 Documentation/devicetree/bindings/arm/sunxi.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml b/Documentation/devicetree/bindings/arm/sunxi.yaml
index 046536d02706..f536cdd2c1a6 100644
--- a/Documentation/devicetree/bindings/arm/sunxi.yaml
+++ b/Documentation/devicetree/bindings/arm/sunxi.yaml
@@ -589,6 +589,11 @@ properties:
           - const: emlid,neutis-n5h3
           - const: allwinner,sun8i-h3
 
+      - description: NetCube Systems Kumquat
+        items:
+          - const: netcube,kumquat
+          - const: allwinner,sun8i-v3s
+
       - description: NextThing Co. CHIP
         items:
           - const: nextthing,chip
-- 
2.47.1

Re: [PATCH v3 2/4] dt-bindings: arm: sunxi: Add NetCube Systems Kumquat board

[Andre Przywara]

On Fri,  3 Jan 2025 20:45:18 +0000
Lukas Schmid <lukas.schmid@netcube.li> wrote:

> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>

Same double Signed-off-by: here as is patch 1/4.

Also would be nice to have some prose here, maybe mention that it's for
industrial(?) embedded applications (as opposed to some hobbyist SBC).

Cheers,
Andre


> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
> ---
>  Documentation/devicetree/bindings/arm/sunxi.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml b/Documentation/devicetree/bindings/arm/sunxi.yaml
> index 046536d02706..f536cdd2c1a6 100644
> --- a/Documentation/devicetree/bindings/arm/sunxi.yaml
> +++ b/Documentation/devicetree/bindings/arm/sunxi.yaml
> @@ -589,6 +589,11 @@ properties:
>            - const: emlid,neutis-n5h3
>            - const: allwinner,sun8i-h3
>  
> +      - description: NetCube Systems Kumquat
> +        items:
> +          - const: netcube,kumquat
> +          - const: allwinner,sun8i-v3s
> +
>        - description: NextThing Co. CHIP
>          items:
>            - const: nextthing,chip

Re: [PATCH v3 2/4] dt-bindings: arm: sunxi: Add NetCube Systems Kumquat board

[Krzysztof Kozlowski]

On Fri, Jan 03, 2025 at 08:45:18PM +0000, Lukas Schmid wrote:
> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

No, patch changed. You removed now commit msg entirely, so don't keep my
ack... My previous guideline described this case: if you change patch
significantly, then drop the tag. Removing entire commit msg, for
whatever reason, is a significant change.... unless that's a mistake,
but checkpatch, which I asked you to run, would prevent it. :/

Best regards,
Krzysztof

[PATCH v3 3/4] dt-bindings: pinctrl: sunxi: Add gpio-reserved-ranges property

[Lukas Schmid]

Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
---
 .../bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml        | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
index 450240570314..24b90a5538d6 100644
--- a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
@@ -83,6 +83,11 @@ properties:
 
   gpio-controller: true
   interrupt-controller: true
+
+  gpio-reserved-ranges:
+    minItems: 1
+    maxItems: 90
+
   gpio-line-names: true
 
   input-debounce:
-- 
2.47.1

Re: [PATCH v3 3/4] dt-bindings: pinctrl: sunxi: Add gpio-reserved-ranges property

[Andre Przywara]

On Fri,  3 Jan 2025 20:45:19 +0000
Lukas Schmid <lukas.schmid@netcube.li> wrote:

Hi,

You would definitely need some text here, at the very least this "90"
deserves some comment.
But what is this property meant to say, actually? Looking at the DT I
guess you want to mark GPIOs as not connected/exposed or such? I don't
think this is the intended purpose of this property, it's more for
describing ranges of GPIOs not implemented in the GPIO *controller* -
say there are 18 pins on PortA, but pins 9-12 are not implemented (no
registers) - check Documentation/devicetree/bindings/gpio/gpio.txt.
But keep in mind that the Allwinner pinctrl device does not use this
kind of per-port binding in the first place, and I wouldn't know of any
SoC having non-consecutive pins per port anyway.

So I think this is not needed, not in this binding, and not in the DT.

Cheers,
Andre

P.S. Feel free to discuss things in replies to emails - that's why we
have those mails. In the past you seemed to always answer very quickly
with a new series, even though you seemed to disagree on some things
(RTC?)

> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
> ---
>  .../bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml        | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> index 450240570314..24b90a5538d6 100644
> --- a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
> @@ -83,6 +83,11 @@ properties:
>  
>    gpio-controller: true
>    interrupt-controller: true
> +
> +  gpio-reserved-ranges:
> +    minItems: 1
> +    maxItems: 90
> +
>    gpio-line-names: true
>  
>    input-debounce:

[PATCH v3 4/4] ARM: dts: sunxi: add support for NetCube Systems Kumquat

[Lukas Schmid]

NetCube Systems Kumquat is a board based on the Allwinner V3s SoC,
including:

- 64MB DDR2 included in SoC
- 10/100 Mbps Ethernet
- USB-C DRD
- Audio Codec
- Isolated CAN-FD
- ESP32 over SDIO
- 8MB SPI-NOR Flash for bootloader
- I2C EEPROM for MAC addresses
- SDIO Connector for eMMC or SD-Card
- 8x 12/24V IOs, 4x normally open relays
- DS3232 RTC
- QWIIC connectors for external I2C devices

Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
---
 arch/arm/boot/dts/allwinner/Makefile          |   2 +
 .../allwinner/sun8i-v3s-netcube-kumquat.dts   | 290 ++++++++++++++++++
 arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi    |   6 +
 3 files changed, 298 insertions(+)
 create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts

diff --git a/arch/arm/boot/dts/allwinner/Makefile b/arch/arm/boot/dts/allwinner/Makefile
index 48666f73e638..d799ad153b37 100644
--- a/arch/arm/boot/dts/allwinner/Makefile
+++ b/arch/arm/boot/dts/allwinner/Makefile
@@ -199,6 +199,7 @@ DTC_FLAGS_sun8i-h3-nanopi-r1 := -@
 DTC_FLAGS_sun8i-h3-orangepi-pc := -@
 DTC_FLAGS_sun8i-h3-bananapi-m2-plus-v1.2 := -@
 DTC_FLAGS_sun8i-h3-orangepi-pc-plus := -@
+DTC_FLAGS_sun8i-v3s-netcube-kumquat := -@
 dtb-$(CONFIG_MACH_SUN8I) += \
 	sun8i-a23-evb.dtb \
 	sun8i-a23-gt90h-v4.dtb \
@@ -261,6 +262,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
 	sun8i-v3s-anbernic-rg-nano.dtb \
 	sun8i-v3s-licheepi-zero.dtb \
 	sun8i-v3s-licheepi-zero-dock.dtb \
+	sun8i-v3s-netcube-kumquat.dtb \
 	sun8i-v40-bananapi-m2-berry.dtb
 dtb-$(CONFIG_MACH_SUN9I) += \
 	sun9i-a80-optimus.dtb \
diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
new file mode 100644
index 000000000000..e5d2a716eb69
--- /dev/null
+++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2025 Lukas Schmid <lukas.schmid@netcube.li>
+ */
+
+/dts-v1/;
+#include "sun8i-v3s.dtsi"
+
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/common.h>
+#include <dt-bindings/gpio/gpio.h>
+
+/{
+	model = "NetCube Systems Kumquat";
+	compatible = "netcube,kumquat", "allwinner,sun8i-v3s";
+
+	aliases {
+		serial0 = &uart0;
+		ethernet0 = &emac;
+		rtc0 = &ds3232;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	/* 40 MHz Crystal Oscillator on PCB */
+	clk_can0: clock-can0 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency  = <40000000>;
+	};
+
+	gpio-keys {
+		compatible = "gpio-keys";
+		autorepeat;
+
+		key-user {
+			label = "GPIO Key User";
+			linux,code = <KEY_PROG1>;
+			gpios = <&pio 1 2 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; /* PB2 */
+		};
+	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		led-heartbeat {
+			gpios = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4 */
+			linux,default-trigger = "heartbeat";
+			color = <LED_COLOR_ID_GREEN>;
+			function = LED_FUNCTION_HEARTBEAT;
+		};
+
+		led-mmc0-act {
+			gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
+			linux,default-trigger = "mmc0";
+			color = <LED_COLOR_ID_GREEN>;
+			function = LED_FUNCTION_DISK;
+		};
+	};
+
+	/* XC6206-3.0 Linear Regualtor */
+	reg_vcc3v0: regulator-3v0 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc3v0";
+		regulator-min-microvolt = <3000000>;
+		regulator-max-microvolt = <3000000>;
+		vin-supply = <&reg_vcc3v3>;
+	};
+
+	/* EA3036C Switching 3 Channel Regulator - Channel 2 */
+	reg_vcc3v3: regulator-3v3 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc3v3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		vin-supply = <&reg_vcc5v0>;
+	};
+
+	/* K7805-1000R3 Switching Regulator supplied from main 12/24V terminal block */
+	reg_vcc5v0: regulator-5v0 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc5v0";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+	};
+};
+
+&mmc0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc0_pins>;
+	vmmc-supply = <&reg_vcc3v3>;
+	bus-width = <4>;
+	broken-cd;
+	status = "okay";
+};
+
+&mmc1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc1_pins>;
+	vmmc-supply = <&reg_vcc3v3>;
+	bus-width = <4>;
+	broken-cd;
+	status = "okay";
+};
+
+&usb_otg {
+	extcon = <&tusb320 0>;
+	dr_mode = "otg";
+	status = "okay";
+};
+
+&usbphy {
+	usb0_id_det-gpios = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4 */
+	status = "okay";
+};
+
+&ehci {
+	status = "okay";
+};
+
+&ohci {
+	status = "okay";
+};
+
+&rtc {
+	status = "disabled";
+};
+
+&pio {
+	vcc-pb-supply = <&reg_vcc3v3>;
+	vcc-pc-supply = <&reg_vcc3v3>;
+	vcc-pe-supply = <&reg_vcc3v3>;
+	vcc-pf-supply = <&reg_vcc3v3>;
+	vcc-pg-supply = <&reg_vcc3v3>;
+
+	gpio-reserved-ranges = <0 32>, <42 22>, <68 28>, <96 32>, <153 7>, <167 25>, <198 26>;
+	gpio-line-names = "", "", "", "", // PA
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "", 
+			  "CAN_nCS", "CAN_nINT", "USER_SW", "PB3", // PB
+			  "USB_ID", "USBC_nINT", "I2C0_SCL", "I2C0_SDA",
+			  "UART0_TX", "UART0_RX", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "SPI_MISO", "SPI_SCK", "FLASH_nCS", "SPI_MOSI", // PC
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "", 
+			  "", "", "", "", // PD
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "Q12", "Q11", "Q10", "Q9", // PE
+			  "LED_SYS0", "I1", "Q1", "Q2",
+			  "I2", "I3", "Q3", "Q4",
+			  "I4", "I5", "Q5", "Q6",
+			  "I6", "I7", "Q7", "Q8",
+			  "I8", "UART1_TXD", "UART1_RXD", "ESP_nRST",
+			  "ESP_nBOOT", "", "", "",
+			  "", "", "", "",
+			  "SD_D1", "SD_D0", "SD_CLK", "SD_CMD", // PF
+			  "SD_D3", "SD_D2", "LED_SYS1", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "ESP_CLK", "ESP_CMD", "ESP_D0", "ESP_D1", // PG
+			  "ESP_D2", "ESP_D3", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "",
+			  "", "", "", "";
+};
+
+&lradc {
+	vref-supply = <&reg_vcc3v0>;
+	status = "okay";
+};
+
+&codec {
+	allwinner,audio-routing =
+		"Headphone", "HP",
+		"Headphone", "HPCOM",
+		"MIC1", "Mic",
+		"Mic", "HBIAS";
+	status = "okay";
+};
+
+&uart0 {
+	pinctrl-0 = <&uart0_pb_pins>;
+	pinctrl-names = "default";
+	status = "okay";
+};
+
+&uart1 {
+	pinctrl-0 = <&uart1_pe_pins>;
+	pinctrl-names = "default";
+	status = "okay";
+};
+
+&i2c0 {
+	pinctrl-0 = <&i2c0_pins>;
+	pinctrl-names = "default";
+	status = "okay";
+
+	ds3232: rtc@68 {
+		compatible = "dallas,ds3232";
+		reg = <0x68>;
+	};
+
+	eeprom0: eeprom@50 {
+		compatible = "atmel,24c02";		/* actually it's a 24AA02E48 */
+		pagesize = <16>;
+		read-only;
+		reg = <0x50>;
+		vcc-supply = <&reg_vcc3v3>;
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		eth0_macaddress: macaddress@fa {
+			reg = <0xfa 0x06>;
+		};
+	};
+
+	tusb320: typec@60 {
+		compatible = "ti,tusb320";
+		reg = <0x60>;
+		interrupt-parent = <&pio>;
+		interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
+	};
+};
+
+&emac {
+	allwinner,leds-active-low;
+	nvmem-cells = <&eth0_macaddress>;		/* custom nvmem reference */
+	nvmem-cell-names = "mac-address";		/* see ethernet-controller.yaml */
+	status = "okay";
+};
+
+&spi0 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi0_pins>;
+	cs-gpios = <0>, <&pio 1 0 GPIO_ACTIVE_LOW>; /* PB0 */
+	status = "okay";
+
+	flash@0 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "jedec,spi-nor";
+		reg = <0>;
+		label = "firmware";
+		spi-max-frequency = <40000000>;
+	};
+
+	can@1 {
+		compatible = "microchip,mcp2518fd";
+		reg = <1>;
+		clocks = <&clk_can0>;
+		interrupt-parent = <&pio>;
+		interrupts = <1 1 IRQ_TYPE_LEVEL_LOW>;  /* PB1 */
+		spi-max-frequency = <20000000>;
+		vdd-supply = <&reg_vcc3v3>;
+		xceiver-supply = <&reg_vcc3v3>;
+	};
+};
\ No newline at end of file
diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
index 9e13c2aa8911..f909b1d4dbca 100644
--- a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
+++ b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
@@ -416,6 +416,12 @@ uart0_pb_pins: uart0-pb-pins {
 				function = "uart0";
 			};
 
+			/omit-if-no-ref/
+			uart1_pe_pins: uart1-pe-pins {
+				pins = "PE21", "PE22";
+				function = "uart1";
+			};
+
 			uart2_pins: uart2-pins {
 				pins = "PB0", "PB1";
 				function = "uart2";
-- 
2.47.1

Re: [PATCH v3 4/4] ARM: dts: sunxi: add support for NetCube Systems Kumquat

[Andre Przywara]

On Fri,  3 Jan 2025 20:45:20 +0000
Lukas Schmid <lukas.schmid@netcube.li> wrote:

(CC:ing Icenowy for a question about the RTC below ...)

> NetCube Systems Kumquat is a board based on the Allwinner V3s SoC,
> including:
> 
> - 64MB DDR2 included in SoC
> - 10/100 Mbps Ethernet
> - USB-C DRD
> - Audio Codec
> - Isolated CAN-FD
> - ESP32 over SDIO
> - 8MB SPI-NOR Flash for bootloader
> - I2C EEPROM for MAC addresses
> - SDIO Connector for eMMC or SD-Card
> - 8x 12/24V IOs, 4x normally open relays
> - DS3232 RTC
> - QWIIC connectors for external I2C devices
> 
> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
> ---
>  arch/arm/boot/dts/allwinner/Makefile          |   2 +
>  .../allwinner/sun8i-v3s-netcube-kumquat.dts   | 290 ++++++++++++++++++
>  arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi    |   6 +
>  3 files changed, 298 insertions(+)
>  create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
> 
> diff --git a/arch/arm/boot/dts/allwinner/Makefile b/arch/arm/boot/dts/allwinner/Makefile
> index 48666f73e638..d799ad153b37 100644
> --- a/arch/arm/boot/dts/allwinner/Makefile
> +++ b/arch/arm/boot/dts/allwinner/Makefile
> @@ -199,6 +199,7 @@ DTC_FLAGS_sun8i-h3-nanopi-r1 := -@
>  DTC_FLAGS_sun8i-h3-orangepi-pc := -@
>  DTC_FLAGS_sun8i-h3-bananapi-m2-plus-v1.2 := -@
>  DTC_FLAGS_sun8i-h3-orangepi-pc-plus := -@
> +DTC_FLAGS_sun8i-v3s-netcube-kumquat := -@
>  dtb-$(CONFIG_MACH_SUN8I) += \
>  	sun8i-a23-evb.dtb \
>  	sun8i-a23-gt90h-v4.dtb \
> @@ -261,6 +262,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>  	sun8i-v3s-anbernic-rg-nano.dtb \
>  	sun8i-v3s-licheepi-zero.dtb \
>  	sun8i-v3s-licheepi-zero-dock.dtb \
> +	sun8i-v3s-netcube-kumquat.dtb \
>  	sun8i-v40-bananapi-m2-berry.dtb
>  dtb-$(CONFIG_MACH_SUN9I) += \
>  	sun9i-a80-optimus.dtb \
> diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
> new file mode 100644
> index 000000000000..e5d2a716eb69
> --- /dev/null
> +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
> @@ -0,0 +1,290 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (C) 2025 Lukas Schmid <lukas.schmid@netcube.li>
> + */
> +
> +/dts-v1/;
> +#include "sun8i-v3s.dtsi"
> +
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/{
> +	model = "NetCube Systems Kumquat";
> +	compatible = "netcube,kumquat", "allwinner,sun8i-v3s";
> +
> +	aliases {
> +		serial0 = &uart0;
> +		ethernet0 = &emac;
> +		rtc0 = &ds3232;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	/* 40 MHz Crystal Oscillator on PCB */
> +	clk_can0: clock-can0 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency  = <40000000>;
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		autorepeat;
> +
> +		key-user {
> +			label = "GPIO Key User";
> +			linux,code = <KEY_PROG1>;
> +			gpios = <&pio 1 2 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; /* PB2 */
> +		};
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		led-heartbeat {
> +			gpios = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4 */
> +			linux,default-trigger = "heartbeat";
> +			color = <LED_COLOR_ID_GREEN>;
> +			function = LED_FUNCTION_HEARTBEAT;
> +		};
> +
> +		led-mmc0-act {
> +			gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
> +			linux,default-trigger = "mmc0";
> +			color = <LED_COLOR_ID_GREEN>;
> +			function = LED_FUNCTION_DISK;
> +		};
> +	};
> +
> +	/* XC6206-3.0 Linear Regualtor */
> +	reg_vcc3v0: regulator-3v0 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc3v0";
> +		regulator-min-microvolt = <3000000>;
> +		regulator-max-microvolt = <3000000>;
> +		vin-supply = <&reg_vcc3v3>;
> +	};
> +
> +	/* EA3036C Switching 3 Channel Regulator - Channel 2 */
> +	reg_vcc3v3: regulator-3v3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc3v3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		vin-supply = <&reg_vcc5v0>;
> +	};
> +
> +	/* K7805-1000R3 Switching Regulator supplied from main 12/24V terminal block */
> +	reg_vcc5v0: regulator-5v0 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc5v0";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +	};
> +};
> +
> +&mmc0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc0_pins>;
> +	vmmc-supply = <&reg_vcc3v3>;
> +	bus-width = <4>;
> +	broken-cd;
> +	status = "okay";
> +};
> +
> +&mmc1 {

what's connected here? Are both MMC ports on headers/connectors, and
it's up to the user to connect some SDIO device or an SD/eMMC
card/slot? Or is this port connected to the ESP32?

> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc1_pins>;
> +	vmmc-supply = <&reg_vcc3v3>;
> +	bus-width = <4>;
> +	broken-cd;
> +	status = "okay";
> +};
> +
> +&usb_otg {

I think traditionally referenced nodes in the board .dts files are
ordered by label name, so usb_otg is but-last. Yes, this is in contrast
to nodes in the SoC .dtsi file, which are ordered by MMIO addresses.

> +	extcon = <&tusb320 0>;
> +	dr_mode = "otg";
> +	status = "okay";
> +};
> +
> +&usbphy {
> +	usb0_id_det-gpios = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4 */
> +	status = "okay";
> +};
> +
> +&ehci {
> +	status = "okay";
> +};
> +
> +&ohci {
> +	status = "okay";
> +};
> +
> +&rtc {
> +	status = "disabled";

Please can you explain a bit more what's going on here? I saw you
mentioning in the cover letter that you brought the "disabled" back,
but I still don't see how this is working when the CCU and the pinctrl
nodes reference the RTC clocks? So what is broken, exactly? Is this
some bug in the SoC? Or don't you supply the SoC's VCC_RTC, so the
calendar is not working when the board is not powered - in contrast to
the external RTC?

> +};
> +
> +&pio {
> +	vcc-pb-supply = <&reg_vcc3v3>;
> +	vcc-pc-supply = <&reg_vcc3v3>;
> +	vcc-pe-supply = <&reg_vcc3v3>;
> +	vcc-pf-supply = <&reg_vcc3v3>;
> +	vcc-pg-supply = <&reg_vcc3v3>;
> +
> +	gpio-reserved-ranges = <0 32>, <42 22>, <68 28>, <96 32>, <153 7>, <167 25>, <198 26>;

As mentioned in the reply to the previous patch, this doesn't look
right here. Why do you need this, exactly?

> +	gpio-line-names = "", "", "", "", // PA
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "", 
> +			  "CAN_nCS", "CAN_nINT", "USER_SW", "PB3", // PB
> +			  "USB_ID", "USBC_nINT", "I2C0_SCL", "I2C0_SDA",
> +			  "UART0_TX", "UART0_RX", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "SPI_MISO", "SPI_SCK", "FLASH_nCS", "SPI_MOSI", // PC
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "", 
> +			  "", "", "", "", // PD
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "Q12", "Q11", "Q10", "Q9", // PE
> +			  "LED_SYS0", "I1", "Q1", "Q2",
> +			  "I2", "I3", "Q3", "Q4",
> +			  "I4", "I5", "Q5", "Q6",
> +			  "I6", "I7", "Q7", "Q8",
> +			  "I8", "UART1_TXD", "UART1_RXD", "ESP_nRST",
> +			  "ESP_nBOOT", "", "", "",
> +			  "", "", "", "",
> +			  "SD_D1", "SD_D0", "SD_CLK", "SD_CMD", // PF
> +			  "SD_D3", "SD_D2", "LED_SYS1", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "ESP_CLK", "ESP_CMD", "ESP_D0", "ESP_D1", // PG
> +			  "ESP_D2", "ESP_D3", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "",
> +			  "", "", "", "";
> +};
> +
> +&lradc {
> +	vref-supply = <&reg_vcc3v0>;
> +	status = "okay";
> +};
> +
> +&codec {
> +	allwinner,audio-routing =
> +		"Headphone", "HP",
> +		"Headphone", "HPCOM",
> +		"MIC1", "Mic",
> +		"Mic", "HBIAS";
> +	status = "okay";
> +};
> +
> +&uart0 {
> +	pinctrl-0 = <&uart0_pb_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};
> +
> +&uart1 {
> +	pinctrl-0 = <&uart1_pe_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};
> +
> +&i2c0 {
> +	pinctrl-0 = <&i2c0_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +
> +	ds3232: rtc@68 {
> +		compatible = "dallas,ds3232";
> +		reg = <0x68>;
> +	};
> +
> +	eeprom0: eeprom@50 {
> +		compatible = "atmel,24c02";		/* actually it's a 24AA02E48 */
> +		pagesize = <16>;
> +		read-only;
> +		reg = <0x50>;
> +		vcc-supply = <&reg_vcc3v3>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		eth0_macaddress: macaddress@fa {
> +			reg = <0xfa 0x06>;
> +		};
> +	};
> +
> +	tusb320: typec@60 {
> +		compatible = "ti,tusb320";
> +		reg = <0x60>;
> +		interrupt-parent = <&pio>;
> +		interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
> +	};
> +};
> +
> +&emac {
> +	allwinner,leds-active-low;
> +	nvmem-cells = <&eth0_macaddress>;		/* custom nvmem reference */
> +	nvmem-cell-names = "mac-address";		/* see ethernet-controller.yaml */
> +	status = "okay";
> +};
> +
> +&spi0 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&spi0_pins>;

Those two lines look redundant, as they are already specified in the
.dtsi file.

> +	cs-gpios = <0>, <&pio 1 0 GPIO_ACTIVE_LOW>; /* PB0 */
> +	status = "okay";
> +
> +	flash@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "jedec,spi-nor";

I think traditionally we have the compatible first, and #a-c and #s-c
last in the node.
And do you have anything partitioned in there? If not, you wouldn't
need the #a-c and #s-c properties, I think.

> +		reg = <0>;
> +		label = "firmware";
> +		spi-max-frequency = <40000000>;
> +	};
> +
> +	can@1 {
> +		compatible = "microchip,mcp2518fd";
> +		reg = <1>;
> +		clocks = <&clk_can0>;
> +		interrupt-parent = <&pio>;
> +		interrupts = <1 1 IRQ_TYPE_LEVEL_LOW>;  /* PB1 */
> +		spi-max-frequency = <20000000>;
> +		vdd-supply = <&reg_vcc3v3>;
> +		xceiver-supply = <&reg_vcc3v3>;
> +	};
> +};
> \ No newline at end of file

Please add a newline at the end.

> diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi

I don't know for sure if people want SoC .dtsi patches separately?

Cheers,
Andre

> index 9e13c2aa8911..f909b1d4dbca 100644
> --- a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> @@ -416,6 +416,12 @@ uart0_pb_pins: uart0-pb-pins {
>  				function = "uart0";
>  			};
>  
> +			/omit-if-no-ref/
> +			uart1_pe_pins: uart1-pe-pins {
> +				pins = "PE21", "PE22";
> +				function = "uart1";
> +			};
> +
>  			uart2_pins: uart2-pins {
>  				pins = "PB0", "PB1";
>  				function = "uart2";

Re: [PATCH v3 4/4] ARM: dts: sunxi: add support for NetCube Systems Kumquat

[Icenowy Zheng]

在 2025-01-03星期五的 23:57 +0000，Andre Przywara写道：
> On Fri,  3 Jan 2025 20:45:20 +0000
> Lukas Schmid <lukas.schmid@netcube.li> wrote:
> 
> (CC:ing Icenowy for a question about the RTC below ...)
> 
> > NetCube Systems Kumquat is a board based on the Allwinner V3s SoC,
> > including:
> > 
> > - 64MB DDR2 included in SoC
> > - 10/100 Mbps Ethernet
> > - USB-C DRD
> > - Audio Codec
> > - Isolated CAN-FD
> > - ESP32 over SDIO
> > - 8MB SPI-NOR Flash for bootloader
> > - I2C EEPROM for MAC addresses
> > - SDIO Connector for eMMC or SD-Card
> > - 8x 12/24V IOs, 4x normally open relays
> > - DS3232 RTC
> > - QWIIC connectors for external I2C devices
> > 
> > Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
> > ---
> >  arch/arm/boot/dts/allwinner/Makefile          |   2 +
> >  .../allwinner/sun8i-v3s-netcube-kumquat.dts   | 290
> > ++++++++++++++++++
> >  arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi    |   6 +
> >  3 files changed, 298 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
> > kumquat.dts
> > 
> > diff --git a/arch/arm/boot/dts/allwinner/Makefile
> > b/arch/arm/boot/dts/allwinner/Makefile
> > index 48666f73e638..d799ad153b37 100644
> > --- a/arch/arm/boot/dts/allwinner/Makefile
> > +++ b/arch/arm/boot/dts/allwinner/Makefile
> > @@ -199,6 +199,7 @@ DTC_FLAGS_sun8i-h3-nanopi-r1 := -@
> >  DTC_FLAGS_sun8i-h3-orangepi-pc := -@
> >  DTC_FLAGS_sun8i-h3-bananapi-m2-plus-v1.2 := -@
> >  DTC_FLAGS_sun8i-h3-orangepi-pc-plus := -@
> > +DTC_FLAGS_sun8i-v3s-netcube-kumquat := -@
> >  dtb-$(CONFIG_MACH_SUN8I) += \
> >         sun8i-a23-evb.dtb \
> >         sun8i-a23-gt90h-v4.dtb \
> > @@ -261,6 +262,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
> >         sun8i-v3s-anbernic-rg-nano.dtb \
> >         sun8i-v3s-licheepi-zero.dtb \
> >         sun8i-v3s-licheepi-zero-dock.dtb \
> > +       sun8i-v3s-netcube-kumquat.dtb \
> >         sun8i-v40-bananapi-m2-berry.dtb
> >  dtb-$(CONFIG_MACH_SUN9I) += \
> >         sun9i-a80-optimus.dtb \
> > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
> > kumquat.dts b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
> > kumquat.dts
> > new file mode 100644
> > index 000000000000..e5d2a716eb69
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
> > @@ -0,0 +1,290 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright (C) 2025 Lukas Schmid <lukas.schmid@netcube.li>
> > + */
> > +
> > +/dts-v1/;
> > +#include "sun8i-v3s.dtsi"
> > +
> > +#include <dt-bindings/input/input.h>
> > +#include <dt-bindings/leds/common.h>
> > +#include <dt-bindings/gpio/gpio.h>
> > +
> > +/{
> > +       model = "NetCube Systems Kumquat";
> > +       compatible = "netcube,kumquat", "allwinner,sun8i-v3s";
> > +
> > +       aliases {
> > +               serial0 = &uart0;
> > +               ethernet0 = &emac;
> > +               rtc0 = &ds3232;
> > +       };
> > +
> > +       chosen {
> > +               stdout-path = "serial0:115200n8";
> > +       };
> > +
> > +       /* 40 MHz Crystal Oscillator on PCB */
> > +       clk_can0: clock-can0 {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency  = <40000000>;
> > +       };
> > +
> > +       gpio-keys {
> > +               compatible = "gpio-keys";
> > +               autorepeat;
> > +
> > +               key-user {
> > +                       label = "GPIO Key User";
> > +                       linux,code = <KEY_PROG1>;
> > +                       gpios = <&pio 1 2 (GPIO_ACTIVE_LOW |
> > GPIO_PULL_UP)>; /* PB2 */
> > +               };
> > +       };
> > +
> > +       leds {
> > +               compatible = "gpio-leds";
> > +
> > +               led-heartbeat {
> > +                       gpios = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4
> > */
> > +                       linux,default-trigger = "heartbeat";
> > +                       color = <LED_COLOR_ID_GREEN>;
> > +                       function = LED_FUNCTION_HEARTBEAT;
> > +               };
> > +
> > +               led-mmc0-act {
> > +                       gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6
> > */
> > +                       linux,default-trigger = "mmc0";
> > +                       color = <LED_COLOR_ID_GREEN>;
> > +                       function = LED_FUNCTION_DISK;
> > +               };
> > +       };
> > +
> > +       /* XC6206-3.0 Linear Regualtor */
> > +       reg_vcc3v0: regulator-3v0 {
> > +               compatible = "regulator-fixed";
> > +               regulator-name = "vcc3v0";
> > +               regulator-min-microvolt = <3000000>;
> > +               regulator-max-microvolt = <3000000>;
> > +               vin-supply = <&reg_vcc3v3>;
> > +       };
> > +
> > +       /* EA3036C Switching 3 Channel Regulator - Channel 2 */
> > +       reg_vcc3v3: regulator-3v3 {
> > +               compatible = "regulator-fixed";
> > +               regulator-name = "vcc3v3";
> > +               regulator-min-microvolt = <3300000>;
> > +               regulator-max-microvolt = <3300000>;
> > +               vin-supply = <&reg_vcc5v0>;
> > +       };
> > +
> > +       /* K7805-1000R3 Switching Regulator supplied from main
> > 12/24V terminal block */
> > +       reg_vcc5v0: regulator-5v0 {
> > +               compatible = "regulator-fixed";
> > +               regulator-name = "vcc5v0";
> > +               regulator-min-microvolt = <5000000>;
> > +               regulator-max-microvolt = <5000000>;
> > +       };
> > +};
> > +
> > +&mmc0 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&mmc0_pins>;
> > +       vmmc-supply = <&reg_vcc3v3>;
> > +       bus-width = <4>;
> > +       broken-cd;
> > +       status = "okay";
> > +};
> > +
> > +&mmc1 {
> 
> what's connected here? Are both MMC ports on headers/connectors, and
> it's up to the user to connect some SDIO device or an SD/eMMC
> card/slot? Or is this port connected to the ESP32?
> 
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&mmc1_pins>;
> > +       vmmc-supply = <&reg_vcc3v3>;
> > +       bus-width = <4>;
> > +       broken-cd;
> > +       status = "okay";
> > +};
> > +
> > +&usb_otg {
> 
> I think traditionally referenced nodes in the board .dts files are
> ordered by label name, so usb_otg is but-last. Yes, this is in
> contrast
> to nodes in the SoC .dtsi file, which are ordered by MMIO addresses.
> 
> > +       extcon = <&tusb320 0>;
> > +       dr_mode = "otg";
> > +       status = "okay";
> > +};
> > +
> > +&usbphy {
> > +       usb0_id_det-gpios = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4 */
> > +       status = "okay";
> > +};
> > +
> > +&ehci {
> > +       status = "okay";
> > +};
> > +
> > +&ohci {
> > +       status = "okay";
> > +};
> > +
> > +&rtc {
> > +       status = "disabled";
> 
> Please can you explain a bit more what's going on here? I saw you
> mentioning in the cover letter that you brought the "disabled" back,
> but I still don't see how this is working when the CCU and the
> pinctrl
> nodes reference the RTC clocks? So what is broken, exactly? Is this
> some bug in the SoC? Or don't you supply the SoC's VCC_RTC, so the
> calendar is not working when the board is not powered - in contrast
> to
> the external RTC?

Maybe a lack of crystal? But I can understand nothing here, and a
detailed explaination is needed.

> 
> > +};
> > +
> > +&pio {
> > +       vcc-pb-supply = <&reg_vcc3v3>;
> > +       vcc-pc-supply = <&reg_vcc3v3>;
> > +       vcc-pe-supply = <&reg_vcc3v3>;
> > +       vcc-pf-supply = <&reg_vcc3v3>;
> > +       vcc-pg-supply = <&reg_vcc3v3>;
> > +
> > +       gpio-reserved-ranges = <0 32>, <42 22>, <68 28>, <96 32>,
> > <153 7>, <167 25>, <198 26>;
> 
> As mentioned in the reply to the previous patch, this doesn't look
> right here. Why do you need this, exactly?

Ah? I don't think there's any tradition on Allwinner platforms to
reserve any GPIOs, except if you have another firmware running on
another processor (e.g. AR100) that needs some GPIO.

My previous sight of such property is on Qualcomm smartphones, where a
few GPIOs are reserved for "Trusted" thingy.

> 
> > +       gpio-line-names = "", "", "", "", // PA
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "", 
> > +                         "CAN_nCS", "CAN_nINT", "USER_SW", "PB3",
> > // PB
> > +                         "USB_ID", "USBC_nINT", "I2C0_SCL",
> > "I2C0_SDA",
> > +                         "UART0_TX", "UART0_RX", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "SPI_MISO", "SPI_SCK", "FLASH_nCS",
> > "SPI_MOSI", // PC
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "", 
> > +                         "", "", "", "", // PD
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "Q12", "Q11", "Q10", "Q9", // PE
> > +                         "LED_SYS0", "I1", "Q1", "Q2",
> > +                         "I2", "I3", "Q3", "Q4",
> > +                         "I4", "I5", "Q5", "Q6",
> > +                         "I6", "I7", "Q7", "Q8",
> > +                         "I8", "UART1_TXD", "UART1_RXD",
> > "ESP_nRST",
> > +                         "ESP_nBOOT", "", "", "",
> > +                         "", "", "", "",
> > +                         "SD_D1", "SD_D0", "SD_CLK", "SD_CMD", //
> > PF
> > +                         "SD_D3", "SD_D2", "LED_SYS1", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "ESP_CLK", "ESP_CMD", "ESP_D0", "ESP_D1",
> > // PG
> > +                         "ESP_D2", "ESP_D3", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "",
> > +                         "", "", "", "";
> > +};
> > +
> > +&lradc {
> > +       vref-supply = <&reg_vcc3v0>;
> > +       status = "okay";
> > +};
> > +
> > +&codec {
> > +       allwinner,audio-routing =
> > +               "Headphone", "HP",
> > +               "Headphone", "HPCOM",
> > +               "MIC1", "Mic",
> > +               "Mic", "HBIAS";
> > +       status = "okay";
> > +};
> > +
> > +&uart0 {
> > +       pinctrl-0 = <&uart0_pb_pins>;
> > +       pinctrl-names = "default";
> > +       status = "okay";
> > +};
> > +
> > +&uart1 {
> > +       pinctrl-0 = <&uart1_pe_pins>;
> > +       pinctrl-names = "default";
> > +       status = "okay";
> > +};
> > +
> > +&i2c0 {
> > +       pinctrl-0 = <&i2c0_pins>;
> > +       pinctrl-names = "default";
> > +       status = "okay";
> > +
> > +       ds3232: rtc@68 {
> > +               compatible = "dallas,ds3232";
> > +               reg = <0x68>;
> > +       };

If you're afraid of the non-running internal RTC superseding this
external RTC, you can use an alias rtc0 = &ds3232 to force the ext. one
to be the first.

> > +
> > +       eeprom0: eeprom@50 {
> > +               compatible = "atmel,24c02";             /* actually
> > it's a 24AA02E48 */
> > +               pagesize = <16>;
> > +               read-only;
> > +               reg = <0x50>;
> > +               vcc-supply = <&reg_vcc3v3>;
> > +
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +
> > +               eth0_macaddress: macaddress@fa {
> > +                       reg = <0xfa 0x06>;
> > +               };
> > +       };
> > +
> > +       tusb320: typec@60 {
> > +               compatible = "ti,tusb320";
> > +               reg = <0x60>;
> > +               interrupt-parent = <&pio>;
> > +               interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
> > +       };
> > +};
> > +
> > +&emac {
> > +       allwinner,leds-active-low;
> > +       nvmem-cells = <&eth0_macaddress>;               /* custom
> > nvmem reference */
> > +       nvmem-cell-names = "mac-address";               /* see
> > ethernet-controller.yaml */
> > +       status = "okay";
> > +};
> > +
> > +&spi0 {
> > +       #address-cells = <1>;
> > +       #size-cells = <0>;
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&spi0_pins>;
> 
> Those two lines look redundant, as they are already specified in the
> .dtsi file.
> 
> > +       cs-gpios = <0>, <&pio 1 0 GPIO_ACTIVE_LOW>; /* PB0 */
> > +       status = "okay";
> > +
> > +       flash@0 {
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +               compatible = "jedec,spi-nor";
> 
> I think traditionally we have the compatible first, and #a-c and #s-c
> last in the node.
> And do you have anything partitioned in there? If not, you wouldn't
> need the #a-c and #s-c properties, I think.
> 
> > +               reg = <0>;
> > +               label = "firmware";
> > +               spi-max-frequency = <40000000>;
> > +       };
> > +
> > +       can@1 {
> > +               compatible = "microchip,mcp2518fd";
> > +               reg = <1>;
> > +               clocks = <&clk_can0>;
> > +               interrupt-parent = <&pio>;
> > +               interrupts = <1 1 IRQ_TYPE_LEVEL_LOW>;  /* PB1 */
> > +               spi-max-frequency = <20000000>;
> > +               vdd-supply = <&reg_vcc3v3>;
> > +               xceiver-supply = <&reg_vcc3v3>;
> > +       };
> > +};
> > \ No newline at end of file
> 
> Please add a newline at the end.

Well maybe this file is written with some non-Unix-traditional editor,
well Linux is something Unix-traditional, and for these editors manual
insertion of an empty line will be needed (on Unix-traditional things
e.g. Vim, no empty lines should be presented at all.)

> 
> > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> > b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> 
> I don't know for sure if people want SoC .dtsi patches separately?
> 
> Cheers,
> Andre
> 
> > index 9e13c2aa8911..f909b1d4dbca 100644
> > --- a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> > @@ -416,6 +416,12 @@ uart0_pb_pins: uart0-pb-pins {
> >                                 function = "uart0";
> >                         };
> >  
> > +                       /omit-if-no-ref/
> > +                       uart1_pe_pins: uart1-pe-pins {
> > +                               pins = "PE21", "PE22";
> > +                               function = "uart1";
> > +                       };
> > +
> >                         uart2_pins: uart2-pins {
> >                                 pins = "PB0", "PB1";
> >                                 function = "uart2";
> 
> 

Re: [PATCH v3 4/4] ARM: dts: sunxi: add support for NetCube Systems Kumquat

[Lukas Schmid]

Am 2025-01-04 06:12, schrieb Icenowy Zheng:
> 在 2025-01-03星期五的 23:57 +0000，Andre Przywara写道：
>> On Fri,  3 Jan 2025 20:45:20 +0000
>> Lukas Schmid <lukas.schmid@netcube.li> wrote:
>> 
>> (CC:ing Icenowy for a question about the RTC below ...)
>> 
>> > NetCube Systems Kumquat is a board based on the Allwinner V3s SoC,
>> > including:
>> >
>> > - 64MB DDR2 included in SoC
>> > - 10/100 Mbps Ethernet
>> > - USB-C DRD
>> > - Audio Codec
>> > - Isolated CAN-FD
>> > - ESP32 over SDIO
>> > - 8MB SPI-NOR Flash for bootloader
>> > - I2C EEPROM for MAC addresses
>> > - SDIO Connector for eMMC or SD-Card
>> > - 8x 12/24V IOs, 4x normally open relays
>> > - DS3232 RTC
>> > - QWIIC connectors for external I2C devices
>> >
>> > Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
>> > ---
>> >  arch/arm/boot/dts/allwinner/Makefile          |   2 +
>> >  .../allwinner/sun8i-v3s-netcube-kumquat.dts   | 290
>> > ++++++++++++++++++
>> >  arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi    |   6 +
>> >  3 files changed, 298 insertions(+)
>> >  create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
>> > kumquat.dts
>> >
>> > diff --git a/arch/arm/boot/dts/allwinner/Makefile
>> > b/arch/arm/boot/dts/allwinner/Makefile
>> > index 48666f73e638..d799ad153b37 100644
>> > --- a/arch/arm/boot/dts/allwinner/Makefile
>> > +++ b/arch/arm/boot/dts/allwinner/Makefile
>> > @@ -199,6 +199,7 @@ DTC_FLAGS_sun8i-h3-nanopi-r1 := -@
>> >  DTC_FLAGS_sun8i-h3-orangepi-pc := -@
>> >  DTC_FLAGS_sun8i-h3-bananapi-m2-plus-v1.2 := -@
>> >  DTC_FLAGS_sun8i-h3-orangepi-pc-plus := -@
>> > +DTC_FLAGS_sun8i-v3s-netcube-kumquat := -@
>> >  dtb-$(CONFIG_MACH_SUN8I) += \
>> >         sun8i-a23-evb.dtb \
>> >         sun8i-a23-gt90h-v4.dtb \
>> > @@ -261,6 +262,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>> >         sun8i-v3s-anbernic-rg-nano.dtb \
>> >         sun8i-v3s-licheepi-zero.dtb \
>> >         sun8i-v3s-licheepi-zero-dock.dtb \
>> > +       sun8i-v3s-netcube-kumquat.dtb \
>> >         sun8i-v40-bananapi-m2-berry.dtb
>> >  dtb-$(CONFIG_MACH_SUN9I) += \
>> >         sun9i-a80-optimus.dtb \
>> > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
>> > kumquat.dts b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
>> > kumquat.dts
>> > new file mode 100644
>> > index 000000000000..e5d2a716eb69
>> > --- /dev/null
>> > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
>> > @@ -0,0 +1,290 @@
>> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> > +/*
>> > + * Copyright (C) 2025 Lukas Schmid <lukas.schmid@netcube.li>
>> > + */
>> > +
>> > +/dts-v1/;
>> > +#include "sun8i-v3s.dtsi"
>> > +
>> > +#include <dt-bindings/input/input.h>
>> > +#include <dt-bindings/leds/common.h>
>> > +#include <dt-bindings/gpio/gpio.h>
>> > +
>> > +/{
>> > +       model = "NetCube Systems Kumquat";
>> > +       compatible = "netcube,kumquat", "allwinner,sun8i-v3s";
>> > +
>> > +       aliases {
>> > +               serial0 = &uart0;
>> > +               ethernet0 = &emac;
>> > +               rtc0 = &ds3232;
>> > +       };
>> > +
>> > +       chosen {
>> > +               stdout-path = "serial0:115200n8";
>> > +       };
>> > +
>> > +       /* 40 MHz Crystal Oscillator on PCB */
>> > +       clk_can0: clock-can0 {
>> > +               compatible = "fixed-clock";
>> > +               #clock-cells = <0>;
>> > +               clock-frequency  = <40000000>;
>> > +       };
>> > +
>> > +       gpio-keys {
>> > +               compatible = "gpio-keys";
>> > +               autorepeat;
>> > +
>> > +               key-user {
>> > +                       label = "GPIO Key User";
>> > +                       linux,code = <KEY_PROG1>;
>> > +                       gpios = <&pio 1 2 (GPIO_ACTIVE_LOW |
>> > GPIO_PULL_UP)>; /* PB2 */
>> > +               };
>> > +       };
>> > +
>> > +       leds {
>> > +               compatible = "gpio-leds";
>> > +
>> > +               led-heartbeat {
>> > +                       gpios = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4
>> > */
>> > +                       linux,default-trigger = "heartbeat";
>> > +                       color = <LED_COLOR_ID_GREEN>;
>> > +                       function = LED_FUNCTION_HEARTBEAT;
>> > +               };
>> > +
>> > +               led-mmc0-act {
>> > +                       gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6
>> > */
>> > +                       linux,default-trigger = "mmc0";
>> > +                       color = <LED_COLOR_ID_GREEN>;
>> > +                       function = LED_FUNCTION_DISK;
>> > +               };
>> > +       };
>> > +
>> > +       /* XC6206-3.0 Linear Regualtor */
>> > +       reg_vcc3v0: regulator-3v0 {
>> > +               compatible = "regulator-fixed";
>> > +               regulator-name = "vcc3v0";
>> > +               regulator-min-microvolt = <3000000>;
>> > +               regulator-max-microvolt = <3000000>;
>> > +               vin-supply = <&reg_vcc3v3>;
>> > +       };
>> > +
>> > +       /* EA3036C Switching 3 Channel Regulator - Channel 2 */
>> > +       reg_vcc3v3: regulator-3v3 {
>> > +               compatible = "regulator-fixed";
>> > +               regulator-name = "vcc3v3";
>> > +               regulator-min-microvolt = <3300000>;
>> > +               regulator-max-microvolt = <3300000>;
>> > +               vin-supply = <&reg_vcc5v0>;
>> > +       };
>> > +
>> > +       /* K7805-1000R3 Switching Regulator supplied from main
>> > 12/24V terminal block */
>> > +       reg_vcc5v0: regulator-5v0 {
>> > +               compatible = "regulator-fixed";
>> > +               regulator-name = "vcc5v0";
>> > +               regulator-min-microvolt = <5000000>;
>> > +               regulator-max-microvolt = <5000000>;
>> > +       };
>> > +};
>> > +
>> > +&mmc0 {
>> > +       pinctrl-names = "default";
>> > +       pinctrl-0 = <&mmc0_pins>;
>> > +       vmmc-supply = <&reg_vcc3v3>;
>> > +       bus-width = <4>;
>> > +       broken-cd;
>> > +       status = "okay";
>> > +};
>> > +
>> > +&mmc1 {
>> 
>> what's connected here? Are both MMC ports on headers/connectors, and
>> it's up to the user to connect some SDIO device or an SD/eMMC
>> card/slot? Or is this port connected to the ESP32?
>> 
>> > +       pinctrl-names = "default";
>> > +       pinctrl-0 = <&mmc1_pins>;
>> > +       vmmc-supply = <&reg_vcc3v3>;
>> > +       bus-width = <4>;
>> > +       broken-cd;
>> > +       status = "okay";
>> > +};
>> > +
>> > +&usb_otg {
>> 
>> I think traditionally referenced nodes in the board .dts files are
>> ordered by label name, so usb_otg is but-last. Yes, this is in
>> contrast
>> to nodes in the SoC .dtsi file, which are ordered by MMIO addresses.
>> 
>> > +       extcon = <&tusb320 0>;
>> > +       dr_mode = "otg";
>> > +       status = "okay";
>> > +};
>> > +
>> > +&usbphy {
>> > +       usb0_id_det-gpios = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4 */
>> > +       status = "okay";
>> > +};
>> > +
>> > +&ehci {
>> > +       status = "okay";
>> > +};
>> > +
>> > +&ohci {
>> > +       status = "okay";
>> > +};
>> > +
>> > +&rtc {
>> > +       status = "disabled";
>> 
>> Please can you explain a bit more what's going on here? I saw you
>> mentioning in the cover letter that you brought the "disabled" back,
>> but I still don't see how this is working when the CCU and the
>> pinctrl
>> nodes reference the RTC clocks? So what is broken, exactly? Is this
>> some bug in the SoC? Or don't you supply the SoC's VCC_RTC, so the
>> calendar is not working when the board is not powered - in contrast
>> to
>> the external RTC?
> 
> Maybe a lack of crystal? But I can understand nothing here, and a
> detailed explaination is needed.
> 

I have tried to enable the RTC, but I get a "RTC still busy" from the
sunxi-rtc module. The RTC Power is connected, and the 32kHz Crystal is 
also
on Board. If the RTC's driver wouldn't throw the error I have no issue 
with
leaving the rtc enabled. It would however loose it's memory after power
cycle as the Battery is only connected to the DS3232+, hence the alias 
to
rtc0

>> 
>> > +};
>> > +
>> > +&pio {
>> > +       vcc-pb-supply = <&reg_vcc3v3>;
>> > +       vcc-pc-supply = <&reg_vcc3v3>;
>> > +       vcc-pe-supply = <&reg_vcc3v3>;
>> > +       vcc-pf-supply = <&reg_vcc3v3>;
>> > +       vcc-pg-supply = <&reg_vcc3v3>;
>> > +
>> > +       gpio-reserved-ranges = <0 32>, <42 22>, <68 28>, <96 32>,
>> > <153 7>, <167 25>, <198 26>;
>> 
>> As mentioned in the reply to the previous patch, this doesn't look
>> right here. Why do you need this, exactly?
> 
> Ah? I don't think there's any tradition on Allwinner platforms to
> reserve any GPIOs, except if you have another firmware running on
> another processor (e.g. AR100) that needs some GPIO.
> 
> My previous sight of such property is on Qualcomm smartphones, where a
> few GPIOs are reserved for "Trusted" thingy.
> 

My intention here was to have the GPIOs which are not accessible on the 
SoC's
package disabled so that stuff like libgpiod cannot try to access them. 
The
gpiodetect tool did show me them as 'used' when I added the 
reserved-ranges,
so I thought the driver does understand this tag.

>> 
>> > +       gpio-line-names = "", "", "", "", // PA
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "CAN_nCS", "CAN_nINT", "USER_SW", "PB3",
>> > // PB
>> > +                         "USB_ID", "USBC_nINT", "I2C0_SCL",
>> > "I2C0_SDA",
>> > +                         "UART0_TX", "UART0_RX", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "SPI_MISO", "SPI_SCK", "FLASH_nCS",
>> > "SPI_MOSI", // PC
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "", // PD
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "Q12", "Q11", "Q10", "Q9", // PE
>> > +                         "LED_SYS0", "I1", "Q1", "Q2",
>> > +                         "I2", "I3", "Q3", "Q4",
>> > +                         "I4", "I5", "Q5", "Q6",
>> > +                         "I6", "I7", "Q7", "Q8",
>> > +                         "I8", "UART1_TXD", "UART1_RXD",
>> > "ESP_nRST",
>> > +                         "ESP_nBOOT", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "SD_D1", "SD_D0", "SD_CLK", "SD_CMD", //
>> > PF
>> > +                         "SD_D3", "SD_D2", "LED_SYS1", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "ESP_CLK", "ESP_CMD", "ESP_D0", "ESP_D1",
>> > // PG
>> > +                         "ESP_D2", "ESP_D3", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "",
>> > +                         "", "", "", "";
>> > +};
>> > +
>> > +&lradc {
>> > +       vref-supply = <&reg_vcc3v0>;
>> > +       status = "okay";
>> > +};
>> > +
>> > +&codec {
>> > +       allwinner,audio-routing =
>> > +               "Headphone", "HP",
>> > +               "Headphone", "HPCOM",
>> > +               "MIC1", "Mic",
>> > +               "Mic", "HBIAS";
>> > +       status = "okay";
>> > +};
>> > +
>> > +&uart0 {
>> > +       pinctrl-0 = <&uart0_pb_pins>;
>> > +       pinctrl-names = "default";
>> > +       status = "okay";
>> > +};
>> > +
>> > +&uart1 {
>> > +       pinctrl-0 = <&uart1_pe_pins>;
>> > +       pinctrl-names = "default";
>> > +       status = "okay";
>> > +};
>> > +
>> > +&i2c0 {
>> > +       pinctrl-0 = <&i2c0_pins>;
>> > +       pinctrl-names = "default";
>> > +       status = "okay";
>> > +
>> > +       ds3232: rtc@68 {
>> > +               compatible = "dallas,ds3232";
>> > +               reg = <0x68>;
>> > +       };
> 
> If you're afraid of the non-running internal RTC superseding this
> external RTC, you can use an alias rtc0 = &ds3232 to force the ext. one
> to be the first.
> 

Yes exactly, I already have an alias to this rtc as rtc0

>> > +
>> > +       eeprom0: eeprom@50 {
>> > +               compatible = "atmel,24c02";             /* actually
>> > it's a 24AA02E48 */
>> > +               pagesize = <16>;
>> > +               read-only;
>> > +               reg = <0x50>;
>> > +               vcc-supply = <&reg_vcc3v3>;
>> > +
>> > +               #address-cells = <1>;
>> > +               #size-cells = <1>;
>> > +
>> > +               eth0_macaddress: macaddress@fa {
>> > +                       reg = <0xfa 0x06>;
>> > +               };
>> > +       };
>> > +
>> > +       tusb320: typec@60 {
>> > +               compatible = "ti,tusb320";
>> > +               reg = <0x60>;
>> > +               interrupt-parent = <&pio>;
>> > +               interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
>> > +       };
>> > +};
>> > +
>> > +&emac {
>> > +       allwinner,leds-active-low;
>> > +       nvmem-cells = <&eth0_macaddress>;               /* custom
>> > nvmem reference */
>> > +       nvmem-cell-names = "mac-address";               /* see
>> > ethernet-controller.yaml */
>> > +       status = "okay";
>> > +};
>> > +
>> > +&spi0 {
>> > +       #address-cells = <1>;
>> > +       #size-cells = <0>;
>> > +       pinctrl-names = "default";
>> > +       pinctrl-0 = <&spi0_pins>;
>> 
>> Those two lines look redundant, as they are already specified in the
>> .dtsi file.
>> 
>> > +       cs-gpios = <0>, <&pio 1 0 GPIO_ACTIVE_LOW>; /* PB0 */
>> > +       status = "okay";
>> > +
>> > +       flash@0 {
>> > +               #address-cells = <1>;
>> > +               #size-cells = <1>;
>> > +               compatible = "jedec,spi-nor";
>> 
>> I think traditionally we have the compatible first, and #a-c and #s-c
>> last in the node.
>> And do you have anything partitioned in there? If not, you wouldn't
>> need the #a-c and #s-c properties, I think.
>> 
>> > +               reg = <0>;
>> > +               label = "firmware";
>> > +               spi-max-frequency = <40000000>;
>> > +       };
>> > +
>> > +       can@1 {
>> > +               compatible = "microchip,mcp2518fd";
>> > +               reg = <1>;
>> > +               clocks = <&clk_can0>;
>> > +               interrupt-parent = <&pio>;
>> > +               interrupts = <1 1 IRQ_TYPE_LEVEL_LOW>;  /* PB1 */
>> > +               spi-max-frequency = <20000000>;
>> > +               vdd-supply = <&reg_vcc3v3>;
>> > +               xceiver-supply = <&reg_vcc3v3>;
>> > +       };
>> > +};
>> > \ No newline at end of file
>> 
>> Please add a newline at the end.
> 
> Well maybe this file is written with some non-Unix-traditional editor,
> well Linux is something Unix-traditional, and for these editors manual
> insertion of an empty line will be needed (on Unix-traditional things
> e.g. Vim, no empty lines should be presented at all.)
> 
>> 
>> > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> > b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> 
>> I don't know for sure if people want SoC .dtsi patches separately?
>> 
>> Cheers,
>> Andre
>> 
>> > index 9e13c2aa8911..f909b1d4dbca 100644
>> > --- a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> > @@ -416,6 +416,12 @@ uart0_pb_pins: uart0-pb-pins {
>> >                                 function = "uart0";
>> >                         };
>> >  
>> > +                       /omit-if-no-ref/
>> > +                       uart1_pe_pins: uart1-pe-pins {
>> > +                               pins = "PE21", "PE22";
>> > +                               function = "uart1";
>> > +                       };
>> > +
>> >                         uart2_pins: uart2-pins {
>> >                                 pins = "PB0", "PB1";
>> >                                 function = "uart2";
>> 
>> 

Re: [PATCH v3 4/4] ARM: dts: sunxi: add support for NetCube Systems Kumquat

[Icenowy Zheng]

在 2025-01-04星期六的 14:03 +0100，Lukas Schmid写道：
> Am 2025-01-04 06:12, schrieb Icenowy Zheng:
> > 在 2025-01-03星期五的 23:57 +0000，Andre Przywara写道：
> > > On Fri,  3 Jan 2025 20:45:20 +0000
> > > Lukas Schmid <lukas.schmid@netcube.li> wrote:
> > > 
> > > (CC:ing Icenowy for a question about the RTC below ...)
> > > 
> > > > NetCube Systems Kumquat is a board based on the Allwinner V3s
> > > > SoC,
> > > > including:
> > > > 
> > > > - 64MB DDR2 included in SoC
> > > > - 10/100 Mbps Ethernet
> > > > - USB-C DRD
> > > > - Audio Codec
> > > > - Isolated CAN-FD
> > > > - ESP32 over SDIO
> > > > - 8MB SPI-NOR Flash for bootloader
> > > > - I2C EEPROM for MAC addresses
> > > > - SDIO Connector for eMMC or SD-Card
> > > > - 8x 12/24V IOs, 4x normally open relays
> > > > - DS3232 RTC
> > > > - QWIIC connectors for external I2C devices
> > > > 
> > > > Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
> > > > ---
> > > >  arch/arm/boot/dts/allwinner/Makefile          |   2 +
> > > >  .../allwinner/sun8i-v3s-netcube-kumquat.dts   | 290
> > > > ++++++++++++++++++
> > > >  arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi    |   6 +
> > > >  3 files changed, 298 insertions(+)
> > > >  create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v3s-
> > > > netcube-
> > > > kumquat.dts
> > > > 
> > > > diff --git a/arch/arm/boot/dts/allwinner/Makefile
> > > > b/arch/arm/boot/dts/allwinner/Makefile
> > > > index 48666f73e638..d799ad153b37 100644
> > > > --- a/arch/arm/boot/dts/allwinner/Makefile
> > > > +++ b/arch/arm/boot/dts/allwinner/Makefile
> > > > @@ -199,6 +199,7 @@ DTC_FLAGS_sun8i-h3-nanopi-r1 := -@
> > > >  DTC_FLAGS_sun8i-h3-orangepi-pc := -@
> > > >  DTC_FLAGS_sun8i-h3-bananapi-m2-plus-v1.2 := -@
> > > >  DTC_FLAGS_sun8i-h3-orangepi-pc-plus := -@
> > > > +DTC_FLAGS_sun8i-v3s-netcube-kumquat := -@
> > > >  dtb-$(CONFIG_MACH_SUN8I) += \
> > > >         sun8i-a23-evb.dtb \
> > > >         sun8i-a23-gt90h-v4.dtb \
> > > > @@ -261,6 +262,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
> > > >         sun8i-v3s-anbernic-rg-nano.dtb \
> > > >         sun8i-v3s-licheepi-zero.dtb \
> > > >         sun8i-v3s-licheepi-zero-dock.dtb \
> > > > +       sun8i-v3s-netcube-kumquat.dtb \
> > > >         sun8i-v40-bananapi-m2-berry.dtb
> > > >  dtb-$(CONFIG_MACH_SUN9I) += \
> > > >         sun9i-a80-optimus.dtb \
> > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
> > > > kumquat.dts b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
> > > > kumquat.dts
> > > > new file mode 100644
> > > > index 000000000000..e5d2a716eb69
> > > > --- /dev/null
> > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
> > > > @@ -0,0 +1,290 @@
> > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > +/*
> > > > + * Copyright (C) 2025 Lukas Schmid <lukas.schmid@netcube.li>
> > > > + */
> > > > +
> > > > +/dts-v1/;
> > > > +#include "sun8i-v3s.dtsi"
> > > > +
> > > > +#include <dt-bindings/input/input.h>
> > > > +#include <dt-bindings/leds/common.h>
> > > > +#include <dt-bindings/gpio/gpio.h>
> > > > +
> > > > +/{
> > > > +       model = "NetCube Systems Kumquat";
> > > > +       compatible = "netcube,kumquat", "allwinner,sun8i-v3s";
> > > > +
> > > > +       aliases {
> > > > +               serial0 = &uart0;
> > > > +               ethernet0 = &emac;
> > > > +               rtc0 = &ds3232;
> > > > +       };
> > > > +
> > > > +       chosen {
> > > > +               stdout-path = "serial0:115200n8";
> > > > +       };
> > > > +
> > > > +       /* 40 MHz Crystal Oscillator on PCB */
> > > > +       clk_can0: clock-can0 {
> > > > +               compatible = "fixed-clock";
> > > > +               #clock-cells = <0>;
> > > > +               clock-frequency  = <40000000>;
> > > > +       };
> > > > +
> > > > +       gpio-keys {
> > > > +               compatible = "gpio-keys";
> > > > +               autorepeat;
> > > > +
> > > > +               key-user {
> > > > +                       label = "GPIO Key User";
> > > > +                       linux,code = <KEY_PROG1>;
> > > > +                       gpios = <&pio 1 2 (GPIO_ACTIVE_LOW |
> > > > GPIO_PULL_UP)>; /* PB2 */
> > > > +               };
> > > > +       };
> > > > +
> > > > +       leds {
> > > > +               compatible = "gpio-leds";
> > > > +
> > > > +               led-heartbeat {
> > > > +                       gpios = <&pio 4 4 GPIO_ACTIVE_HIGH>; /*
> > > > PE4
> > > > */
> > > > +                       linux,default-trigger = "heartbeat";
> > > > +                       color = <LED_COLOR_ID_GREEN>;
> > > > +                       function = LED_FUNCTION_HEARTBEAT;
> > > > +               };
> > > > +
> > > > +               led-mmc0-act {
> > > > +                       gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /*
> > > > PF6
> > > > */
> > > > +                       linux,default-trigger = "mmc0";
> > > > +                       color = <LED_COLOR_ID_GREEN>;
> > > > +                       function = LED_FUNCTION_DISK;
> > > > +               };
> > > > +       };
> > > > +
> > > > +       /* XC6206-3.0 Linear Regualtor */
> > > > +       reg_vcc3v0: regulator-3v0 {
> > > > +               compatible = "regulator-fixed";
> > > > +               regulator-name = "vcc3v0";
> > > > +               regulator-min-microvolt = <3000000>;
> > > > +               regulator-max-microvolt = <3000000>;
> > > > +               vin-supply = <&reg_vcc3v3>;
> > > > +       };
> > > > +
> > > > +       /* EA3036C Switching 3 Channel Regulator - Channel 2 */
> > > > +       reg_vcc3v3: regulator-3v3 {
> > > > +               compatible = "regulator-fixed";
> > > > +               regulator-name = "vcc3v3";
> > > > +               regulator-min-microvolt = <3300000>;
> > > > +               regulator-max-microvolt = <3300000>;
> > > > +               vin-supply = <&reg_vcc5v0>;
> > > > +       };
> > > > +
> > > > +       /* K7805-1000R3 Switching Regulator supplied from main
> > > > 12/24V terminal block */
> > > > +       reg_vcc5v0: regulator-5v0 {
> > > > +               compatible = "regulator-fixed";
> > > > +               regulator-name = "vcc5v0";
> > > > +               regulator-min-microvolt = <5000000>;
> > > > +               regulator-max-microvolt = <5000000>;
> > > > +       };
> > > > +};
> > > > +
> > > > +&mmc0 {
> > > > +       pinctrl-names = "default";
> > > > +       pinctrl-0 = <&mmc0_pins>;
> > > > +       vmmc-supply = <&reg_vcc3v3>;
> > > > +       bus-width = <4>;
> > > > +       broken-cd;
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&mmc1 {
> > > 
> > > what's connected here? Are both MMC ports on headers/connectors,
> > > and
> > > it's up to the user to connect some SDIO device or an SD/eMMC
> > > card/slot? Or is this port connected to the ESP32?
> > > 
> > > > +       pinctrl-names = "default";
> > > > +       pinctrl-0 = <&mmc1_pins>;
> > > > +       vmmc-supply = <&reg_vcc3v3>;
> > > > +       bus-width = <4>;
> > > > +       broken-cd;
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&usb_otg {
> > > 
> > > I think traditionally referenced nodes in the board .dts files
> > > are
> > > ordered by label name, so usb_otg is but-last. Yes, this is in
> > > contrast
> > > to nodes in the SoC .dtsi file, which are ordered by MMIO
> > > addresses.
> > > 
> > > > +       extcon = <&tusb320 0>;
> > > > +       dr_mode = "otg";
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&usbphy {
> > > > +       usb0_id_det-gpios = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4
> > > > */
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&ehci {
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&ohci {
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&rtc {
> > > > +       status = "disabled";
> > > 
> > > Please can you explain a bit more what's going on here? I saw you
> > > mentioning in the cover letter that you brought the "disabled"
> > > back,
> > > but I still don't see how this is working when the CCU and the
> > > pinctrl
> > > nodes reference the RTC clocks? So what is broken, exactly? Is
> > > this
> > > some bug in the SoC? Or don't you supply the SoC's VCC_RTC, so
> > > the
> > > calendar is not working when the board is not powered - in
> > > contrast
> > > to
> > > the external RTC?
> > 
> > Maybe a lack of crystal? But I can understand nothing here, and a
> > detailed explaination is needed.
> > 
> 
> I have tried to enable the RTC, but I get a "RTC still busy" from the
> sunxi-rtc module. The RTC Power is connected, and the 32kHz Crystal
> is 

Weird... Never heard such kind of things...

How do modules in the main SoC that depend on LOSC (32768 Hz osc)
perform?

> also
> on Board. If the RTC's driver wouldn't throw the error I have no
> issue 
> with
> leaving the rtc enabled. It would however loose it's memory after
> power
> cycle as the Battery is only connected to the DS3232+, hence the
> alias 
> to
> rtc0
> 
> > > 
> > > > +};
> > > > +
> > > > +&pio {
> > > > +       vcc-pb-supply = <&reg_vcc3v3>;
> > > > +       vcc-pc-supply = <&reg_vcc3v3>;
> > > > +       vcc-pe-supply = <&reg_vcc3v3>;
> > > > +       vcc-pf-supply = <&reg_vcc3v3>;
> > > > +       vcc-pg-supply = <&reg_vcc3v3>;
> > > > +
> > > > +       gpio-reserved-ranges = <0 32>, <42 22>, <68 28>, <96
> > > > 32>,
> > > > <153 7>, <167 25>, <198 26>;
> > > 
> > > As mentioned in the reply to the previous patch, this doesn't
> > > look
> > > right here. Why do you need this, exactly?
> > 
> > Ah? I don't think there's any tradition on Allwinner platforms to
> > reserve any GPIOs, except if you have another firmware running on
> > another processor (e.g. AR100) that needs some GPIO.
> > 
> > My previous sight of such property is on Qualcomm smartphones,
> > where a
> > few GPIOs are reserved for "Trusted" thingy.
> > 
> 
> My intention here was to have the GPIOs which are not accessible on
> the 
> SoC's
> package disabled so that stuff like libgpiod cannot try to access
> them. 
> The
> gpiodetect tool did show me them as 'used' when I added the 
> reserved-ranges,
> so I thought the driver does understand this tag.

Interesting point... Maybe this kind of things should enter the per-
SoC(package) DTSI?

Although technically they are not "used" but "unavailable".

> 
> > > 
> > > > +       gpio-line-names = "", "", "", "", // PA
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "CAN_nCS", "CAN_nINT", "USER_SW",
> > > > "PB3",
> > > > // PB
> > > > +                         "USB_ID", "USBC_nINT", "I2C0_SCL",
> > > > "I2C0_SDA",
> > > > +                         "UART0_TX", "UART0_RX", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "SPI_MISO", "SPI_SCK", "FLASH_nCS",
> > > > "SPI_MOSI", // PC
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "", // PD
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "Q12", "Q11", "Q10", "Q9", // PE
> > > > +                         "LED_SYS0", "I1", "Q1", "Q2",
> > > > +                         "I2", "I3", "Q3", "Q4",
> > > > +                         "I4", "I5", "Q5", "Q6",
> > > > +                         "I6", "I7", "Q7", "Q8",
> > > > +                         "I8", "UART1_TXD", "UART1_RXD",
> > > > "ESP_nRST",
> > > > +                         "ESP_nBOOT", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "SD_D1", "SD_D0", "SD_CLK", "SD_CMD",
> > > > //
> > > > PF
> > > > +                         "SD_D3", "SD_D2", "LED_SYS1", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "ESP_CLK", "ESP_CMD", "ESP_D0",
> > > > "ESP_D1",
> > > > // PG
> > > > +                         "ESP_D2", "ESP_D3", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "",
> > > > +                         "", "", "", "";
> > > > +};
> > > > +
> > > > +&lradc {
> > > > +       vref-supply = <&reg_vcc3v0>;
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&codec {
> > > > +       allwinner,audio-routing =
> > > > +               "Headphone", "HP",
> > > > +               "Headphone", "HPCOM",
> > > > +               "MIC1", "Mic",
> > > > +               "Mic", "HBIAS";
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&uart0 {
> > > > +       pinctrl-0 = <&uart0_pb_pins>;
> > > > +       pinctrl-names = "default";
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&uart1 {
> > > > +       pinctrl-0 = <&uart1_pe_pins>;
> > > > +       pinctrl-names = "default";
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&i2c0 {
> > > > +       pinctrl-0 = <&i2c0_pins>;
> > > > +       pinctrl-names = "default";
> > > > +       status = "okay";
> > > > +
> > > > +       ds3232: rtc@68 {
> > > > +               compatible = "dallas,ds3232";
> > > > +               reg = <0x68>;
> > > > +       };
> > 
> > If you're afraid of the non-running internal RTC superseding this
> > external RTC, you can use an alias rtc0 = &ds3232 to force the ext.
> > one
> > to be the first.
> > 
> 
> Yes exactly, I already have an alias to this rtc as rtc0
> 
> > > > +
> > > > +       eeprom0: eeprom@50 {
> > > > +               compatible = "atmel,24c02";             /*
> > > > actually
> > > > it's a 24AA02E48 */
> > > > +               pagesize = <16>;
> > > > +               read-only;
> > > > +               reg = <0x50>;
> > > > +               vcc-supply = <&reg_vcc3v3>;
> > > > +
> > > > +               #address-cells = <1>;
> > > > +               #size-cells = <1>;
> > > > +
> > > > +               eth0_macaddress: macaddress@fa {
> > > > +                       reg = <0xfa 0x06>;
> > > > +               };
> > > > +       };
> > > > +
> > > > +       tusb320: typec@60 {
> > > > +               compatible = "ti,tusb320";
> > > > +               reg = <0x60>;
> > > > +               interrupt-parent = <&pio>;
> > > > +               interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
> > > > +       };
> > > > +};
> > > > +
> > > > +&emac {
> > > > +       allwinner,leds-active-low;
> > > > +       nvmem-cells = <&eth0_macaddress>;               /*
> > > > custom
> > > > nvmem reference */
> > > > +       nvmem-cell-names = "mac-address";               /* see
> > > > ethernet-controller.yaml */
> > > > +       status = "okay";
> > > > +};
> > > > +
> > > > +&spi0 {
> > > > +       #address-cells = <1>;
> > > > +       #size-cells = <0>;
> > > > +       pinctrl-names = "default";
> > > > +       pinctrl-0 = <&spi0_pins>;
> > > 
> > > Those two lines look redundant, as they are already specified in
> > > the
> > > .dtsi file.
> > > 
> > > > +       cs-gpios = <0>, <&pio 1 0 GPIO_ACTIVE_LOW>; /* PB0 */
> > > > +       status = "okay";
> > > > +
> > > > +       flash@0 {
> > > > +               #address-cells = <1>;
> > > > +               #size-cells = <1>;
> > > > +               compatible = "jedec,spi-nor";
> > > 
> > > I think traditionally we have the compatible first, and #a-c and
> > > #s-c
> > > last in the node.
> > > And do you have anything partitioned in there? If not, you
> > > wouldn't
> > > need the #a-c and #s-c properties, I think.
> > > 
> > > > +               reg = <0>;
> > > > +               label = "firmware";
> > > > +               spi-max-frequency = <40000000>;
> > > > +       };
> > > > +
> > > > +       can@1 {
> > > > +               compatible = "microchip,mcp2518fd";
> > > > +               reg = <1>;
> > > > +               clocks = <&clk_can0>;
> > > > +               interrupt-parent = <&pio>;
> > > > +               interrupts = <1 1 IRQ_TYPE_LEVEL_LOW>;  /* PB1
> > > > */
> > > > +               spi-max-frequency = <20000000>;
> > > > +               vdd-supply = <&reg_vcc3v3>;
> > > > +               xceiver-supply = <&reg_vcc3v3>;
> > > > +       };
> > > > +};
> > > > \ No newline at end of file
> > > 
> > > Please add a newline at the end.
> > 
> > Well maybe this file is written with some non-Unix-traditional
> > editor,
> > well Linux is something Unix-traditional, and for these editors
> > manual
> > insertion of an empty line will be needed (on Unix-traditional
> > things
> > e.g. Vim, no empty lines should be presented at all.)
> > 
> > > 
> > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> > > > b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> > > 
> > > I don't know for sure if people want SoC .dtsi patches
> > > separately?
> > > 
> > > Cheers,
> > > Andre
> > > 
> > > > index 9e13c2aa8911..f909b1d4dbca 100644
> > > > --- a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> > > > @@ -416,6 +416,12 @@ uart0_pb_pins: uart0-pb-pins {
> > > >                                 function = "uart0";
> > > >                         };
> > > >  
> > > > +                       /omit-if-no-ref/
> > > > +                       uart1_pe_pins: uart1-pe-pins {
> > > > +                               pins = "PE21", "PE22";
> > > > +                               function = "uart1";
> > > > +                       };
> > > > +
> > > >                         uart2_pins: uart2-pins {
> > > >                                 pins = "PB0", "PB1";
> > > >                                 function = "uart2";
> > > 
> > > 
> 
> 

Re: [PATCH v3 4/4] ARM: dts: sunxi: add support for NetCube Systems Kumquat

[Lukas Schmid]

Am 2025-01-05 05:17, schrieb Icenowy Zheng:
> 在 2025-01-04星期六的 14:03 +0100，Lukas Schmid写道：
>> Am 2025-01-04 06:12, schrieb Icenowy Zheng:
>> > 在 2025-01-03星期五的 23:57 +0000，Andre Przywara写道：
>> > > On Fri,  3 Jan 2025 20:45:20 +0000
>> > > Lukas Schmid <lukas.schmid@netcube.li> wrote:
>> > >
>> > > (CC:ing Icenowy for a question about the RTC below ...)
>> > >
>> > > > NetCube Systems Kumquat is a board based on the Allwinner V3s
>> > > > SoC,
>> > > > including:
>> > > >
>> > > > - 64MB DDR2 included in SoC
>> > > > - 10/100 Mbps Ethernet
>> > > > - USB-C DRD
>> > > > - Audio Codec
>> > > > - Isolated CAN-FD
>> > > > - ESP32 over SDIO
>> > > > - 8MB SPI-NOR Flash for bootloader
>> > > > - I2C EEPROM for MAC addresses
>> > > > - SDIO Connector for eMMC or SD-Card
>> > > > - 8x 12/24V IOs, 4x normally open relays
>> > > > - DS3232 RTC
>> > > > - QWIIC connectors for external I2C devices
>> > > >
>> > > > Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
>> > > > ---
>> > > >  arch/arm/boot/dts/allwinner/Makefile          |   2 +
>> > > >  .../allwinner/sun8i-v3s-netcube-kumquat.dts   | 290
>> > > > ++++++++++++++++++
>> > > >  arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi    |   6 +
>> > > >  3 files changed, 298 insertions(+)
>> > > >  create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v3s-
>> > > > netcube-
>> > > > kumquat.dts
>> > > >
>> > > > diff --git a/arch/arm/boot/dts/allwinner/Makefile
>> > > > b/arch/arm/boot/dts/allwinner/Makefile
>> > > > index 48666f73e638..d799ad153b37 100644
>> > > > --- a/arch/arm/boot/dts/allwinner/Makefile
>> > > > +++ b/arch/arm/boot/dts/allwinner/Makefile
>> > > > @@ -199,6 +199,7 @@ DTC_FLAGS_sun8i-h3-nanopi-r1 := -@
>> > > >  DTC_FLAGS_sun8i-h3-orangepi-pc := -@
>> > > >  DTC_FLAGS_sun8i-h3-bananapi-m2-plus-v1.2 := -@
>> > > >  DTC_FLAGS_sun8i-h3-orangepi-pc-plus := -@
>> > > > +DTC_FLAGS_sun8i-v3s-netcube-kumquat := -@
>> > > >  dtb-$(CONFIG_MACH_SUN8I) += \
>> > > >         sun8i-a23-evb.dtb \
>> > > >         sun8i-a23-gt90h-v4.dtb \
>> > > > @@ -261,6 +262,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>> > > >         sun8i-v3s-anbernic-rg-nano.dtb \
>> > > >         sun8i-v3s-licheepi-zero.dtb \
>> > > >         sun8i-v3s-licheepi-zero-dock.dtb \
>> > > > +       sun8i-v3s-netcube-kumquat.dtb \
>> > > >         sun8i-v40-bananapi-m2-berry.dtb
>> > > >  dtb-$(CONFIG_MACH_SUN9I) += \
>> > > >         sun9i-a80-optimus.dtb \
>> > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
>> > > > kumquat.dts b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
>> > > > kumquat.dts
>> > > > new file mode 100644
>> > > > index 000000000000..e5d2a716eb69
>> > > > --- /dev/null
>> > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
>> > > > @@ -0,0 +1,290 @@
>> > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> > > > +/*
>> > > > + * Copyright (C) 2025 Lukas Schmid <lukas.schmid@netcube.li>
>> > > > + */
>> > > > +
>> > > > +/dts-v1/;
>> > > > +#include "sun8i-v3s.dtsi"
>> > > > +
>> > > > +#include <dt-bindings/input/input.h>
>> > > > +#include <dt-bindings/leds/common.h>
>> > > > +#include <dt-bindings/gpio/gpio.h>
>> > > > +
>> > > > +/{
>> > > > +       model = "NetCube Systems Kumquat";
>> > > > +       compatible = "netcube,kumquat", "allwinner,sun8i-v3s";
>> > > > +
>> > > > +       aliases {
>> > > > +               serial0 = &uart0;
>> > > > +               ethernet0 = &emac;
>> > > > +               rtc0 = &ds3232;
>> > > > +       };
>> > > > +
>> > > > +       chosen {
>> > > > +               stdout-path = "serial0:115200n8";
>> > > > +       };
>> > > > +
>> > > > +       /* 40 MHz Crystal Oscillator on PCB */
>> > > > +       clk_can0: clock-can0 {
>> > > > +               compatible = "fixed-clock";
>> > > > +               #clock-cells = <0>;
>> > > > +               clock-frequency  = <40000000>;
>> > > > +       };
>> > > > +
>> > > > +       gpio-keys {
>> > > > +               compatible = "gpio-keys";
>> > > > +               autorepeat;
>> > > > +
>> > > > +               key-user {
>> > > > +                       label = "GPIO Key User";
>> > > > +                       linux,code = <KEY_PROG1>;
>> > > > +                       gpios = <&pio 1 2 (GPIO_ACTIVE_LOW |
>> > > > GPIO_PULL_UP)>; /* PB2 */
>> > > > +               };
>> > > > +       };
>> > > > +
>> > > > +       leds {
>> > > > +               compatible = "gpio-leds";
>> > > > +
>> > > > +               led-heartbeat {
>> > > > +                       gpios = <&pio 4 4 GPIO_ACTIVE_HIGH>; /*
>> > > > PE4
>> > > > */
>> > > > +                       linux,default-trigger = "heartbeat";
>> > > > +                       color = <LED_COLOR_ID_GREEN>;
>> > > > +                       function = LED_FUNCTION_HEARTBEAT;
>> > > > +               };
>> > > > +
>> > > > +               led-mmc0-act {
>> > > > +                       gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /*
>> > > > PF6
>> > > > */
>> > > > +                       linux,default-trigger = "mmc0";
>> > > > +                       color = <LED_COLOR_ID_GREEN>;
>> > > > +                       function = LED_FUNCTION_DISK;
>> > > > +               };
>> > > > +       };
>> > > > +
>> > > > +       /* XC6206-3.0 Linear Regualtor */
>> > > > +       reg_vcc3v0: regulator-3v0 {
>> > > > +               compatible = "regulator-fixed";
>> > > > +               regulator-name = "vcc3v0";
>> > > > +               regulator-min-microvolt = <3000000>;
>> > > > +               regulator-max-microvolt = <3000000>;
>> > > > +               vin-supply = <&reg_vcc3v3>;
>> > > > +       };
>> > > > +
>> > > > +       /* EA3036C Switching 3 Channel Regulator - Channel 2 */
>> > > > +       reg_vcc3v3: regulator-3v3 {
>> > > > +               compatible = "regulator-fixed";
>> > > > +               regulator-name = "vcc3v3";
>> > > > +               regulator-min-microvolt = <3300000>;
>> > > > +               regulator-max-microvolt = <3300000>;
>> > > > +               vin-supply = <&reg_vcc5v0>;
>> > > > +       };
>> > > > +
>> > > > +       /* K7805-1000R3 Switching Regulator supplied from main
>> > > > 12/24V terminal block */
>> > > > +       reg_vcc5v0: regulator-5v0 {
>> > > > +               compatible = "regulator-fixed";
>> > > > +               regulator-name = "vcc5v0";
>> > > > +               regulator-min-microvolt = <5000000>;
>> > > > +               regulator-max-microvolt = <5000000>;
>> > > > +       };
>> > > > +};
>> > > > +
>> > > > +&mmc0 {
>> > > > +       pinctrl-names = "default";
>> > > > +       pinctrl-0 = <&mmc0_pins>;
>> > > > +       vmmc-supply = <&reg_vcc3v3>;
>> > > > +       bus-width = <4>;
>> > > > +       broken-cd;
>> > > > +       status = "okay";
>> > > > +};
>> > > > +
>> > > > +&mmc1 {
>> > >
>> > > what's connected here? Are both MMC ports on headers/connectors,
>> > > and
>> > > it's up to the user to connect some SDIO device or an SD/eMMC
>> > > card/slot? Or is this port connected to the ESP32?
>> > >
>> > > > +       pinctrl-names = "default";
>> > > > +       pinctrl-0 = <&mmc1_pins>;
>> > > > +       vmmc-supply = <&reg_vcc3v3>;
>> > > > +       bus-width = <4>;
>> > > > +       broken-cd;
>> > > > +       status = "okay";
>> > > > +};
>> > > > +
>> > > > +&usb_otg {
>> > >
>> > > I think traditionally referenced nodes in the board .dts files
>> > > are
>> > > ordered by label name, so usb_otg is but-last. Yes, this is in
>> > > contrast
>> > > to nodes in the SoC .dtsi file, which are ordered by MMIO
>> > > addresses.
>> > >
>> > > > +       extcon = <&tusb320 0>;
>> > > > +       dr_mode = "otg";
>> > > > +       status = "okay";
>> > > > +};
>> > > > +
>> > > > +&usbphy {
>> > > > +       usb0_id_det-gpios = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4
>> > > > */
>> > > > +       status = "okay";
>> > > > +};
>> > > > +
>> > > > +&ehci {
>> > > > +       status = "okay";
>> > > > +};
>> > > > +
>> > > > +&ohci {
>> > > > +       status = "okay";
>> > > > +};
>> > > > +
>> > > > +&rtc {
>> > > > +       status = "disabled";
>> > >
>> > > Please can you explain a bit more what's going on here? I saw you
>> > > mentioning in the cover letter that you brought the "disabled"
>> > > back,
>> > > but I still don't see how this is working when the CCU and the
>> > > pinctrl
>> > > nodes reference the RTC clocks? So what is broken, exactly? Is
>> > > this
>> > > some bug in the SoC? Or don't you supply the SoC's VCC_RTC, so
>> > > the
>> > > calendar is not working when the board is not powered - in
>> > > contrast
>> > > to
>> > > the external RTC?
>> >
>> > Maybe a lack of crystal? But I can understand nothing here, and a
>> > detailed explaination is needed.
>> >
>> 
>> I have tried to enable the RTC, but I get a "RTC still busy" from the
>> sunxi-rtc module. The RTC Power is connected, and the 32kHz Crystal
>> is
> 
> Weird... Never heard such kind of things...
> 
> How do modules in the main SoC that depend on LOSC (32768 Hz osc)
> perform?
> 

I didn't notice anything running the board. I had one running for
about a month on Kernel 6.8.7 without issues, with the rtc disabled

Do you have anything I should check in particular? From the
Datasheet it seems to me that all peripherals in the SoC should
be able to run from the hosc too.

>> also
>> on Board. If the RTC's driver wouldn't throw the error I have no
>> issue
>> with
>> leaving the rtc enabled. It would however loose it's memory after
>> power
>> cycle as the Battery is only connected to the DS3232+, hence the
>> alias
>> to
>> rtc0
>> 
>> > >
>> > > > +};
>> > > > +
>> > > > +&pio {
>> > > > +       vcc-pb-supply = <&reg_vcc3v3>;
>> > > > +       vcc-pc-supply = <&reg_vcc3v3>;
>> > > > +       vcc-pe-supply = <&reg_vcc3v3>;
>> > > > +       vcc-pf-supply = <&reg_vcc3v3>;
>> > > > +       vcc-pg-supply = <&reg_vcc3v3>;
>> > > > +
>> > > > +       gpio-reserved-ranges = <0 32>, <42 22>, <68 28>, <96
>> > > > 32>,
>> > > > <153 7>, <167 25>, <198 26>;
>> > >
>> > > As mentioned in the reply to the previous patch, this doesn't
>> > > look
>> > > right here. Why do you need this, exactly?
>> >
>> > Ah? I don't think there's any tradition on Allwinner platforms to
>> > reserve any GPIOs, except if you have another firmware running on
>> > another processor (e.g. AR100) that needs some GPIO.
>> >
>> > My previous sight of such property is on Qualcomm smartphones,
>> > where a
>> > few GPIOs are reserved for "Trusted" thingy.
>> >
>> 
>> My intention here was to have the GPIOs which are not accessible on
>> the
>> SoC's
>> package disabled so that stuff like libgpiod cannot try to access
>> them.
>> The
>> gpiodetect tool did show me them as 'used' when I added the
>> reserved-ranges,
>> so I thought the driver does understand this tag.
> 
> Interesting point... Maybe this kind of things should enter the per-
> SoC(package) DTSI?
> 
> Although technically they are not "used" but "unavailable".
> 

Sure, I think I'll remove it from my Patchset for now, but maybe
will create another set in the future.

>> 
>> > >
>> > > > +       gpio-line-names = "", "", "", "", // PA
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "CAN_nCS", "CAN_nINT", "USER_SW",
>> > > > "PB3",
>> > > > // PB
>> > > > +                         "USB_ID", "USBC_nINT", "I2C0_SCL",
>> > > > "I2C0_SDA",
>> > > > +                         "UART0_TX", "UART0_RX", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "SPI_MISO", "SPI_SCK", "FLASH_nCS",
>> > > > "SPI_MOSI", // PC
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "", // PD
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "Q12", "Q11", "Q10", "Q9", // PE
>> > > > +                         "LED_SYS0", "I1", "Q1", "Q2",
>> > > > +                         "I2", "I3", "Q3", "Q4",
>> > > > +                         "I4", "I5", "Q5", "Q6",
>> > > > +                         "I6", "I7", "Q7", "Q8",
>> > > > +                         "I8", "UART1_TXD", "UART1_RXD",
>> > > > "ESP_nRST",
>> > > > +                         "ESP_nBOOT", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "SD_D1", "SD_D0", "SD_CLK", "SD_CMD",
>> > > > //
>> > > > PF
>> > > > +                         "SD_D3", "SD_D2", "LED_SYS1", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "ESP_CLK", "ESP_CMD", "ESP_D0",
>> > > > "ESP_D1",
>> > > > // PG
>> > > > +                         "ESP_D2", "ESP_D3", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "",
>> > > > +                         "", "", "", "";
>> > > > +};
>> > > > +
>> > > > +&lradc {
>> > > > +       vref-supply = <&reg_vcc3v0>;
>> > > > +       status = "okay";
>> > > > +};
>> > > > +
>> > > > +&codec {
>> > > > +       allwinner,audio-routing =
>> > > > +               "Headphone", "HP",
>> > > > +               "Headphone", "HPCOM",
>> > > > +               "MIC1", "Mic",
>> > > > +               "Mic", "HBIAS";
>> > > > +       status = "okay";
>> > > > +};
>> > > > +
>> > > > +&uart0 {
>> > > > +       pinctrl-0 = <&uart0_pb_pins>;
>> > > > +       pinctrl-names = "default";
>> > > > +       status = "okay";
>> > > > +};
>> > > > +
>> > > > +&uart1 {
>> > > > +       pinctrl-0 = <&uart1_pe_pins>;
>> > > > +       pinctrl-names = "default";
>> > > > +       status = "okay";
>> > > > +};
>> > > > +
>> > > > +&i2c0 {
>> > > > +       pinctrl-0 = <&i2c0_pins>;
>> > > > +       pinctrl-names = "default";
>> > > > +       status = "okay";
>> > > > +
>> > > > +       ds3232: rtc@68 {
>> > > > +               compatible = "dallas,ds3232";
>> > > > +               reg = <0x68>;
>> > > > +       };
>> >
>> > If you're afraid of the non-running internal RTC superseding this
>> > external RTC, you can use an alias rtc0 = &ds3232 to force the ext.
>> > one
>> > to be the first.
>> >
>> 
>> Yes exactly, I already have an alias to this rtc as rtc0
>> 
>> > > > +
>> > > > +       eeprom0: eeprom@50 {
>> > > > +               compatible = "atmel,24c02";             /*
>> > > > actually
>> > > > it's a 24AA02E48 */
>> > > > +               pagesize = <16>;
>> > > > +               read-only;
>> > > > +               reg = <0x50>;
>> > > > +               vcc-supply = <&reg_vcc3v3>;
>> > > > +
>> > > > +               #address-cells = <1>;
>> > > > +               #size-cells = <1>;
>> > > > +
>> > > > +               eth0_macaddress: macaddress@fa {
>> > > > +                       reg = <0xfa 0x06>;
>> > > > +               };
>> > > > +       };
>> > > > +
>> > > > +       tusb320: typec@60 {
>> > > > +               compatible = "ti,tusb320";
>> > > > +               reg = <0x60>;
>> > > > +               interrupt-parent = <&pio>;
>> > > > +               interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
>> > > > +       };
>> > > > +};
>> > > > +
>> > > > +&emac {
>> > > > +       allwinner,leds-active-low;
>> > > > +       nvmem-cells = <&eth0_macaddress>;               /*
>> > > > custom
>> > > > nvmem reference */
>> > > > +       nvmem-cell-names = "mac-address";               /* see
>> > > > ethernet-controller.yaml */
>> > > > +       status = "okay";
>> > > > +};
>> > > > +
>> > > > +&spi0 {
>> > > > +       #address-cells = <1>;
>> > > > +       #size-cells = <0>;
>> > > > +       pinctrl-names = "default";
>> > > > +       pinctrl-0 = <&spi0_pins>;
>> > >
>> > > Those two lines look redundant, as they are already specified in
>> > > the
>> > > .dtsi file.
>> > >
>> > > > +       cs-gpios = <0>, <&pio 1 0 GPIO_ACTIVE_LOW>; /* PB0 */
>> > > > +       status = "okay";
>> > > > +
>> > > > +       flash@0 {
>> > > > +               #address-cells = <1>;
>> > > > +               #size-cells = <1>;
>> > > > +               compatible = "jedec,spi-nor";
>> > >
>> > > I think traditionally we have the compatible first, and #a-c and
>> > > #s-c
>> > > last in the node.
>> > > And do you have anything partitioned in there? If not, you
>> > > wouldn't
>> > > need the #a-c and #s-c properties, I think.
>> > >
>> > > > +               reg = <0>;
>> > > > +               label = "firmware";
>> > > > +               spi-max-frequency = <40000000>;
>> > > > +       };
>> > > > +
>> > > > +       can@1 {
>> > > > +               compatible = "microchip,mcp2518fd";
>> > > > +               reg = <1>;
>> > > > +               clocks = <&clk_can0>;
>> > > > +               interrupt-parent = <&pio>;
>> > > > +               interrupts = <1 1 IRQ_TYPE_LEVEL_LOW>;  /* PB1
>> > > > */
>> > > > +               spi-max-frequency = <20000000>;
>> > > > +               vdd-supply = <&reg_vcc3v3>;
>> > > > +               xceiver-supply = <&reg_vcc3v3>;
>> > > > +       };
>> > > > +};
>> > > > \ No newline at end of file
>> > >
>> > > Please add a newline at the end.
>> >
>> > Well maybe this file is written with some non-Unix-traditional
>> > editor,
>> > well Linux is something Unix-traditional, and for these editors
>> > manual
>> > insertion of an empty line will be needed (on Unix-traditional
>> > things
>> > e.g. Vim, no empty lines should be presented at all.)
>> >
>> > >
>> > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> > > > b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> > >
>> > > I don't know for sure if people want SoC .dtsi patches
>> > > separately?
>> > >
>> > > Cheers,
>> > > Andre
>> > >
>> > > > index 9e13c2aa8911..f909b1d4dbca 100644
>> > > > --- a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> > > > @@ -416,6 +416,12 @@ uart0_pb_pins: uart0-pb-pins {
>> > > >                                 function = "uart0";
>> > > >                         };
>> > > >  
>> > > > +                       /omit-if-no-ref/
>> > > > +                       uart1_pe_pins: uart1-pe-pins {
>> > > > +                               pins = "PE21", "PE22";
>> > > > +                               function = "uart1";
>> > > > +                       };
>> > > > +
>> > > >                         uart2_pins: uart2-pins {
>> > > >                                 pins = "PB0", "PB1";
>> > > >                                 function = "uart2";
>> > >
>> > >
>> 
>> 

Re: [PATCH v3 4/4] ARM: dts: sunxi: add support for NetCube Systems Kumquat

[Andre Przywara]

On Sun, 05 Jan 2025 12:25:26 +0100
Lukas Schmid <lukas.schmid@netcube.li> wrote:

> Am 2025-01-05 05:17, schrieb Icenowy Zheng:
> > 在 2025-01-04星期六的 14:03 +0100，Lukas Schmid写道：  
> >> Am 2025-01-04 06:12, schrieb Icenowy Zheng:  
> >> > 在 2025-01-03星期五的 23:57 +0000，Andre Przywara写道：  
> >> > > On Fri,  3 Jan 2025 20:45:20 +0000
> >> > > Lukas Schmid <lukas.schmid@netcube.li> wrote:
> >> > >
> >> > > (CC:ing Icenowy for a question about the RTC below ...)
> >> > >  
> >> > > > NetCube Systems Kumquat is a board based on the Allwinner V3s
> >> > > > SoC,
> >> > > > including:
> >> > > >
> >> > > > - 64MB DDR2 included in SoC
> >> > > > - 10/100 Mbps Ethernet
> >> > > > - USB-C DRD
> >> > > > - Audio Codec
> >> > > > - Isolated CAN-FD
> >> > > > - ESP32 over SDIO
> >> > > > - 8MB SPI-NOR Flash for bootloader
> >> > > > - I2C EEPROM for MAC addresses
> >> > > > - SDIO Connector for eMMC or SD-Card
> >> > > > - 8x 12/24V IOs, 4x normally open relays
> >> > > > - DS3232 RTC
> >> > > > - QWIIC connectors for external I2C devices
> >> > > >
> >> > > > Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
> >> > > > ---
> >> > > >  arch/arm/boot/dts/allwinner/Makefile          |   2 +
> >> > > >  .../allwinner/sun8i-v3s-netcube-kumquat.dts   | 290
> >> > > > ++++++++++++++++++
> >> > > >  arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi    |   6 +
> >> > > >  3 files changed, 298 insertions(+)
> >> > > >  create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v3s-
> >> > > > netcube-
> >> > > > kumquat.dts
> >> > > >
> >> > > > diff --git a/arch/arm/boot/dts/allwinner/Makefile
> >> > > > b/arch/arm/boot/dts/allwinner/Makefile
> >> > > > index 48666f73e638..d799ad153b37 100644
> >> > > > --- a/arch/arm/boot/dts/allwinner/Makefile
> >> > > > +++ b/arch/arm/boot/dts/allwinner/Makefile
> >> > > > @@ -199,6 +199,7 @@ DTC_FLAGS_sun8i-h3-nanopi-r1 := -@
> >> > > >  DTC_FLAGS_sun8i-h3-orangepi-pc := -@
> >> > > >  DTC_FLAGS_sun8i-h3-bananapi-m2-plus-v1.2 := -@
> >> > > >  DTC_FLAGS_sun8i-h3-orangepi-pc-plus := -@
> >> > > > +DTC_FLAGS_sun8i-v3s-netcube-kumquat := -@
> >> > > >  dtb-$(CONFIG_MACH_SUN8I) += \
> >> > > >         sun8i-a23-evb.dtb \
> >> > > >         sun8i-a23-gt90h-v4.dtb \
> >> > > > @@ -261,6 +262,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
> >> > > >         sun8i-v3s-anbernic-rg-nano.dtb \
> >> > > >         sun8i-v3s-licheepi-zero.dtb \
> >> > > >         sun8i-v3s-licheepi-zero-dock.dtb \
> >> > > > +       sun8i-v3s-netcube-kumquat.dtb \
> >> > > >         sun8i-v40-bananapi-m2-berry.dtb
> >> > > >  dtb-$(CONFIG_MACH_SUN9I) += \
> >> > > >         sun9i-a80-optimus.dtb \
> >> > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
> >> > > > kumquat.dts b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
> >> > > > kumquat.dts
> >> > > > new file mode 100644
> >> > > > index 000000000000..e5d2a716eb69
> >> > > > --- /dev/null
> >> > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
> >> > > > @@ -0,0 +1,290 @@
> >> > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> > > > +/*
> >> > > > + * Copyright (C) 2025 Lukas Schmid <lukas.schmid@netcube.li>
> >> > > > + */
> >> > > > +
> >> > > > +/dts-v1/;
> >> > > > +#include "sun8i-v3s.dtsi"
> >> > > > +
> >> > > > +#include <dt-bindings/input/input.h>
> >> > > > +#include <dt-bindings/leds/common.h>
> >> > > > +#include <dt-bindings/gpio/gpio.h>
> >> > > > +
> >> > > > +/{
> >> > > > +       model = "NetCube Systems Kumquat";
> >> > > > +       compatible = "netcube,kumquat", "allwinner,sun8i-v3s";
> >> > > > +
> >> > > > +       aliases {
> >> > > > +               serial0 = &uart0;
> >> > > > +               ethernet0 = &emac;
> >> > > > +               rtc0 = &ds3232;
> >> > > > +       };
> >> > > > +
> >> > > > +       chosen {
> >> > > > +               stdout-path = "serial0:115200n8";
> >> > > > +       };
> >> > > > +
> >> > > > +       /* 40 MHz Crystal Oscillator on PCB */
> >> > > > +       clk_can0: clock-can0 {
> >> > > > +               compatible = "fixed-clock";
> >> > > > +               #clock-cells = <0>;
> >> > > > +               clock-frequency  = <40000000>;
> >> > > > +       };
> >> > > > +
> >> > > > +       gpio-keys {
> >> > > > +               compatible = "gpio-keys";
> >> > > > +               autorepeat;
> >> > > > +
> >> > > > +               key-user {
> >> > > > +                       label = "GPIO Key User";
> >> > > > +                       linux,code = <KEY_PROG1>;
> >> > > > +                       gpios = <&pio 1 2 (GPIO_ACTIVE_LOW |  
> >> > > > GPIO_PULL_UP)>; /* PB2 */  
> >> > > > +               };
> >> > > > +       };
> >> > > > +
> >> > > > +       leds {
> >> > > > +               compatible = "gpio-leds";
> >> > > > +
> >> > > > +               led-heartbeat {
> >> > > > +                       gpios = <&pio 4 4 GPIO_ACTIVE_HIGH>; /*
> >> > > > PE4
> >> > > > */
> >> > > > +                       linux,default-trigger = "heartbeat";
> >> > > > +                       color = <LED_COLOR_ID_GREEN>;
> >> > > > +                       function = LED_FUNCTION_HEARTBEAT;
> >> > > > +               };
> >> > > > +
> >> > > > +               led-mmc0-act {
> >> > > > +                       gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /*
> >> > > > PF6
> >> > > > */
> >> > > > +                       linux,default-trigger = "mmc0";
> >> > > > +                       color = <LED_COLOR_ID_GREEN>;
> >> > > > +                       function = LED_FUNCTION_DISK;
> >> > > > +               };
> >> > > > +       };
> >> > > > +
> >> > > > +       /* XC6206-3.0 Linear Regualtor */
> >> > > > +       reg_vcc3v0: regulator-3v0 {
> >> > > > +               compatible = "regulator-fixed";
> >> > > > +               regulator-name = "vcc3v0";
> >> > > > +               regulator-min-microvolt = <3000000>;
> >> > > > +               regulator-max-microvolt = <3000000>;
> >> > > > +               vin-supply = <&reg_vcc3v3>;
> >> > > > +       };
> >> > > > +
> >> > > > +       /* EA3036C Switching 3 Channel Regulator - Channel 2 */
> >> > > > +       reg_vcc3v3: regulator-3v3 {
> >> > > > +               compatible = "regulator-fixed";
> >> > > > +               regulator-name = "vcc3v3";
> >> > > > +               regulator-min-microvolt = <3300000>;
> >> > > > +               regulator-max-microvolt = <3300000>;
> >> > > > +               vin-supply = <&reg_vcc5v0>;
> >> > > > +       };
> >> > > > +
> >> > > > +       /* K7805-1000R3 Switching Regulator supplied from main
> >> > > > 12/24V terminal block */
> >> > > > +       reg_vcc5v0: regulator-5v0 {
> >> > > > +               compatible = "regulator-fixed";
> >> > > > +               regulator-name = "vcc5v0";
> >> > > > +               regulator-min-microvolt = <5000000>;
> >> > > > +               regulator-max-microvolt = <5000000>;
> >> > > > +       };
> >> > > > +};
> >> > > > +
> >> > > > +&mmc0 {
> >> > > > +       pinctrl-names = "default";
> >> > > > +       pinctrl-0 = <&mmc0_pins>;
> >> > > > +       vmmc-supply = <&reg_vcc3v3>;
> >> > > > +       bus-width = <4>;
> >> > > > +       broken-cd;
> >> > > > +       status = "okay";
> >> > > > +};
> >> > > > +
> >> > > > +&mmc1 {  
> >> > >
> >> > > what's connected here? Are both MMC ports on headers/connectors,
> >> > > and
> >> > > it's up to the user to connect some SDIO device or an SD/eMMC
> >> > > card/slot? Or is this port connected to the ESP32?
> >> > >  
> >> > > > +       pinctrl-names = "default";
> >> > > > +       pinctrl-0 = <&mmc1_pins>;
> >> > > > +       vmmc-supply = <&reg_vcc3v3>;
> >> > > > +       bus-width = <4>;
> >> > > > +       broken-cd;
> >> > > > +       status = "okay";
> >> > > > +};
> >> > > > +
> >> > > > +&usb_otg {  
> >> > >
> >> > > I think traditionally referenced nodes in the board .dts files
> >> > > are
> >> > > ordered by label name, so usb_otg is but-last. Yes, this is in
> >> > > contrast
> >> > > to nodes in the SoC .dtsi file, which are ordered by MMIO
> >> > > addresses.
> >> > >  
> >> > > > +       extcon = <&tusb320 0>;
> >> > > > +       dr_mode = "otg";
> >> > > > +       status = "okay";
> >> > > > +};
> >> > > > +
> >> > > > +&usbphy {
> >> > > > +       usb0_id_det-gpios = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4
> >> > > > */
> >> > > > +       status = "okay";
> >> > > > +};
> >> > > > +
> >> > > > +&ehci {
> >> > > > +       status = "okay";
> >> > > > +};
> >> > > > +
> >> > > > +&ohci {
> >> > > > +       status = "okay";
> >> > > > +};
> >> > > > +
> >> > > > +&rtc {
> >> > > > +       status = "disabled";  
> >> > >
> >> > > Please can you explain a bit more what's going on here? I saw you
> >> > > mentioning in the cover letter that you brought the "disabled"
> >> > > back,
> >> > > but I still don't see how this is working when the CCU and the
> >> > > pinctrl
> >> > > nodes reference the RTC clocks? So what is broken, exactly? Is
> >> > > this
> >> > > some bug in the SoC? Or don't you supply the SoC's VCC_RTC, so
> >> > > the
> >> > > calendar is not working when the board is not powered - in
> >> > > contrast
> >> > > to
> >> > > the external RTC?  
> >> >
> >> > Maybe a lack of crystal? But I can understand nothing here, and a
> >> > detailed explaination is needed.
> >> >  
> >> 
> >> I have tried to enable the RTC, but I get a "RTC still busy" from the
> >> sunxi-rtc module. The RTC Power is connected, and the 32kHz Crystal
> >> is  

That sounds actually like a bug somewhere, which should be investigated
and fixed. Disabling the RTC is a quick and dirty workaround, but
nothing for upstream.
There is a 50ms timeout, which sounds plenty, although you could
experiment whether it's too short here:
drivers/rtc/rtc-sun6i.c:sun6i_rtc_settime()
My hunch is that even longer timeouts won't change that, so we should
have a look what's going wrong here, why the RTC does not respond.
Don't you supply VCC_RTC to the SoC at all, maybe? Maybe we need
the 3.3V for the RTC to work, regardless of whether that is battery
backed or not?

> > Weird... Never heard such kind of things...
> > 
> > How do modules in the main SoC that depend on LOSC (32768 Hz osc)
> > perform?
> >   
> 
> I didn't notice anything running the board. I had one running for
> about a month on Kernel 6.8.7 without issues, with the rtc disabled
> 
> Do you have anything I should check in particular? From the
> Datasheet it seems to me that all peripherals in the SoC should
> be able to run from the hosc too.

That's a DT issue, you don't *need* the LOSC crystal for operation:
The clock controller and the GPIO device reference the RTC LOSC clock,
so the respective drivers go ahead and request them, which triggers
probing the RTC. If that now fails, because the RTC is disabled, that
clock cannot be used, and this has consequences:
- The (combinded) GPIO/pinctrl driver only needs the LOSC clock for the
  debouncing part. If that fails, the debouncing init routine will bail
  out (I guess it will not work), but the driver ignores that error and
  proceeds probing, so you are saved, but more by accident. I wonder if
  we should at least print a message here.
- The CCU clock driver only uses the LOSC as a potential parent for
  some muxes (CPU, AHB, APB2), and the kernel will probably never change
  those muxes, and even if, not to the LOSC. So again saved here, by
  chance.

So this means your system continues working, but it's very sketchy, and
technically wrong. So disabling the RTC is not really an option for
mainline, I'd say.

> >> also
> >> on Board. If the RTC's driver wouldn't throw the error I have no
> >> issue
> >> with
> >> leaving the rtc enabled. It would however loose it's memory after
> >> power
> >> cycle as the Battery is only connected to the DS3232+, hence the
> >> alias
> >> to
> >> rtc0
> >>   
> >> > >  
> >> > > > +};
> >> > > > +
> >> > > > +&pio {
> >> > > > +       vcc-pb-supply = <&reg_vcc3v3>;
> >> > > > +       vcc-pc-supply = <&reg_vcc3v3>;
> >> > > > +       vcc-pe-supply = <&reg_vcc3v3>;
> >> > > > +       vcc-pf-supply = <&reg_vcc3v3>;
> >> > > > +       vcc-pg-supply = <&reg_vcc3v3>;
> >> > > > +
> >> > > > +       gpio-reserved-ranges = <0 32>, <42 22>, <68 28>, <96  
> >> > > > 32>,  
> >> > > > <153 7>, <167 25>, <198 26>;  
> >> > >
> >> > > As mentioned in the reply to the previous patch, this doesn't
> >> > > look
> >> > > right here. Why do you need this, exactly?  
> >> >
> >> > Ah? I don't think there's any tradition on Allwinner platforms to
> >> > reserve any GPIOs, except if you have another firmware running on
> >> > another processor (e.g. AR100) that needs some GPIO.
> >> >
> >> > My previous sight of such property is on Qualcomm smartphones,
> >> > where a
> >> > few GPIOs are reserved for "Trusted" thingy.
> >> >  
> >> 
> >> My intention here was to have the GPIOs which are not accessible on
> >> the
> >> SoC's
> >> package disabled so that stuff like libgpiod cannot try to access
> >> them.
> >> The
> >> gpiodetect tool did show me them as 'used' when I added the
> >> reserved-ranges,
> >> so I thought the driver does understand this tag.  
> > 
> > Interesting point... Maybe this kind of things should enter the per-
> > SoC(package) DTSI?

Our sunxi pinctrl driver does not use this information, we have the
number of pins per port and their availability hardcoded in that large
table in pinctrl-sun8i-v3s.c.

If there is more code in the kernel that uses this DT property to
filter the (userspace?) view of the GPIOs, that's a side effect. If we
want this filtering, this code should query the pinctrl driver for this
map, but that's a separate issue. The binding pretty clearly speaks of
some per-port GPIO drivers to use this map, ours it not one of them.

> > Although technically they are not "used" but "unavailable".
> >   
> 
> Sure, I think I'll remove it from my Patchset for now

Yes, please, do that, I think it's wrong here. 

> , but maybe will create another set in the future.

If that solves a problem for you, it should be investigated why, to me
it looks like it's a side effect, and that should be done properly
instead - for instance by using all those empty strings below as an
indicator for availability.
But I don't think insisting on that property is the right thing.

Cheers,
Andre

> >> > > > +       gpio-line-names = "", "", "", "", // PA
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "CAN_nCS", "CAN_nINT", "USER_SW",
> >> > > > "PB3",
> >> > > > // PB
> >> > > > +                         "USB_ID", "USBC_nINT", "I2C0_SCL",
> >> > > > "I2C0_SDA",
> >> > > > +                         "UART0_TX", "UART0_RX", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "SPI_MISO", "SPI_SCK", "FLASH_nCS",
> >> > > > "SPI_MOSI", // PC
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "", // PD
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "Q12", "Q11", "Q10", "Q9", // PE
> >> > > > +                         "LED_SYS0", "I1", "Q1", "Q2",
> >> > > > +                         "I2", "I3", "Q3", "Q4",
> >> > > > +                         "I4", "I5", "Q5", "Q6",
> >> > > > +                         "I6", "I7", "Q7", "Q8",
> >> > > > +                         "I8", "UART1_TXD", "UART1_RXD",
> >> > > > "ESP_nRST",
> >> > > > +                         "ESP_nBOOT", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "SD_D1", "SD_D0", "SD_CLK", "SD_CMD",
> >> > > > //
> >> > > > PF
> >> > > > +                         "SD_D3", "SD_D2", "LED_SYS1", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "ESP_CLK", "ESP_CMD", "ESP_D0",
> >> > > > "ESP_D1",
> >> > > > // PG
> >> > > > +                         "ESP_D2", "ESP_D3", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "",
> >> > > > +                         "", "", "", "";
> >> > > > +};
> >> > > > +
> >> > > > +&lradc {
> >> > > > +       vref-supply = <&reg_vcc3v0>;
> >> > > > +       status = "okay";
> >> > > > +};
> >> > > > +
> >> > > > +&codec {
> >> > > > +       allwinner,audio-routing =
> >> > > > +               "Headphone", "HP",
> >> > > > +               "Headphone", "HPCOM",
> >> > > > +               "MIC1", "Mic",
> >> > > > +               "Mic", "HBIAS";
> >> > > > +       status = "okay";
> >> > > > +};
> >> > > > +
> >> > > > +&uart0 {
> >> > > > +       pinctrl-0 = <&uart0_pb_pins>;
> >> > > > +       pinctrl-names = "default";
> >> > > > +       status = "okay";
> >> > > > +};
> >> > > > +
> >> > > > +&uart1 {
> >> > > > +       pinctrl-0 = <&uart1_pe_pins>;
> >> > > > +       pinctrl-names = "default";
> >> > > > +       status = "okay";
> >> > > > +};
> >> > > > +
> >> > > > +&i2c0 {
> >> > > > +       pinctrl-0 = <&i2c0_pins>;
> >> > > > +       pinctrl-names = "default";
> >> > > > +       status = "okay";
> >> > > > +
> >> > > > +       ds3232: rtc@68 {
> >> > > > +               compatible = "dallas,ds3232";
> >> > > > +               reg = <0x68>;
> >> > > > +       };  
> >> >
> >> > If you're afraid of the non-running internal RTC superseding this
> >> > external RTC, you can use an alias rtc0 = &ds3232 to force the ext.
> >> > one
> >> > to be the first.
> >> >  
> >> 
> >> Yes exactly, I already have an alias to this rtc as rtc0
> >>   
> >> > > > +
> >> > > > +       eeprom0: eeprom@50 {
> >> > > > +               compatible = "atmel,24c02";             /*
> >> > > > actually
> >> > > > it's a 24AA02E48 */
> >> > > > +               pagesize = <16>;
> >> > > > +               read-only;
> >> > > > +               reg = <0x50>;
> >> > > > +               vcc-supply = <&reg_vcc3v3>;
> >> > > > +
> >> > > > +               #address-cells = <1>;
> >> > > > +               #size-cells = <1>;
> >> > > > +
> >> > > > +               eth0_macaddress: macaddress@fa {
> >> > > > +                       reg = <0xfa 0x06>;
> >> > > > +               };
> >> > > > +       };
> >> > > > +
> >> > > > +       tusb320: typec@60 {
> >> > > > +               compatible = "ti,tusb320";
> >> > > > +               reg = <0x60>;
> >> > > > +               interrupt-parent = <&pio>;
> >> > > > +               interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
> >> > > > +       };
> >> > > > +};
> >> > > > +
> >> > > > +&emac {
> >> > > > +       allwinner,leds-active-low;
> >> > > > +       nvmem-cells = <&eth0_macaddress>;               /*
> >> > > > custom
> >> > > > nvmem reference */
> >> > > > +       nvmem-cell-names = "mac-address";               /* see
> >> > > > ethernet-controller.yaml */
> >> > > > +       status = "okay";
> >> > > > +};
> >> > > > +
> >> > > > +&spi0 {
> >> > > > +       #address-cells = <1>;
> >> > > > +       #size-cells = <0>;
> >> > > > +       pinctrl-names = "default";
> >> > > > +       pinctrl-0 = <&spi0_pins>;  
> >> > >
> >> > > Those two lines look redundant, as they are already specified in
> >> > > the
> >> > > .dtsi file.
> >> > >  
> >> > > > +       cs-gpios = <0>, <&pio 1 0 GPIO_ACTIVE_LOW>; /* PB0 */
> >> > > > +       status = "okay";
> >> > > > +
> >> > > > +       flash@0 {
> >> > > > +               #address-cells = <1>;
> >> > > > +               #size-cells = <1>;
> >> > > > +               compatible = "jedec,spi-nor";  
> >> > >
> >> > > I think traditionally we have the compatible first, and #a-c and
> >> > > #s-c
> >> > > last in the node.
> >> > > And do you have anything partitioned in there? If not, you
> >> > > wouldn't
> >> > > need the #a-c and #s-c properties, I think.
> >> > >  
> >> > > > +               reg = <0>;
> >> > > > +               label = "firmware";
> >> > > > +               spi-max-frequency = <40000000>;
> >> > > > +       };
> >> > > > +
> >> > > > +       can@1 {
> >> > > > +               compatible = "microchip,mcp2518fd";
> >> > > > +               reg = <1>;
> >> > > > +               clocks = <&clk_can0>;
> >> > > > +               interrupt-parent = <&pio>;
> >> > > > +               interrupts = <1 1 IRQ_TYPE_LEVEL_LOW>;  /* PB1
> >> > > > */
> >> > > > +               spi-max-frequency = <20000000>;
> >> > > > +               vdd-supply = <&reg_vcc3v3>;
> >> > > > +               xceiver-supply = <&reg_vcc3v3>;
> >> > > > +       };
> >> > > > +};
> >> > > > \ No newline at end of file  
> >> > >
> >> > > Please add a newline at the end.  
> >> >
> >> > Well maybe this file is written with some non-Unix-traditional
> >> > editor,
> >> > well Linux is something Unix-traditional, and for these editors
> >> > manual
> >> > insertion of an empty line will be needed (on Unix-traditional
> >> > things
> >> > e.g. Vim, no empty lines should be presented at all.)
> >> >  
> >> > >  
> >> > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> >> > > > b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi  
> >> > >
> >> > > I don't know for sure if people want SoC .dtsi patches
> >> > > separately?
> >> > >
> >> > > Cheers,
> >> > > Andre
> >> > >  
> >> > > > index 9e13c2aa8911..f909b1d4dbca 100644
> >> > > > --- a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> >> > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> >> > > > @@ -416,6 +416,12 @@ uart0_pb_pins: uart0-pb-pins {
> >> > > >                                 function = "uart0";
> >> > > >                         };
> >> > > >  
> >> > > > +                       /omit-if-no-ref/
> >> > > > +                       uart1_pe_pins: uart1-pe-pins {
> >> > > > +                               pins = "PE21", "PE22";
> >> > > > +                               function = "uart1";
> >> > > > +                       };
> >> > > > +
> >> > > >                         uart2_pins: uart2-pins {
> >> > > >                                 pins = "PB0", "PB1";
> >> > > >                                 function = "uart2";  
> >> > >
> >> > >  
> >> 
> >>   
> 
> 

Re: [PATCH v3 4/4] ARM: dts: sunxi: add support for NetCube Systems Kumquat

[Lukas Schmid]

Am 2025-01-06 01:47, schrieb Andre Przywara:
> On Sun, 05 Jan 2025 12:25:26 +0100
> Lukas Schmid <lukas.schmid@netcube.li> wrote:
> 
>> Am 2025-01-05 05:17, schrieb Icenowy Zheng:
>> > 在 2025-01-04星期六的 14:03 +0100，Lukas Schmid写道：
>> >> Am 2025-01-04 06:12, schrieb Icenowy Zheng:
>> >> > 在 2025-01-03星期五的 23:57 +0000，Andre Przywara写道：
>> >> > > On Fri,  3 Jan 2025 20:45:20 +0000
>> >> > > Lukas Schmid <lukas.schmid@netcube.li> wrote:
>> >> > >
>> >> > > (CC:ing Icenowy for a question about the RTC below ...)
>> >> > >
>> >> > > > NetCube Systems Kumquat is a board based on the Allwinner V3s
>> >> > > > SoC,
>> >> > > > including:
>> >> > > >
>> >> > > > - 64MB DDR2 included in SoC
>> >> > > > - 10/100 Mbps Ethernet
>> >> > > > - USB-C DRD
>> >> > > > - Audio Codec
>> >> > > > - Isolated CAN-FD
>> >> > > > - ESP32 over SDIO
>> >> > > > - 8MB SPI-NOR Flash for bootloader
>> >> > > > - I2C EEPROM for MAC addresses
>> >> > > > - SDIO Connector for eMMC or SD-Card
>> >> > > > - 8x 12/24V IOs, 4x normally open relays
>> >> > > > - DS3232 RTC
>> >> > > > - QWIIC connectors for external I2C devices
>> >> > > >
>> >> > > > Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
>> >> > > > ---
>> >> > > >  arch/arm/boot/dts/allwinner/Makefile          |   2 +
>> >> > > >  .../allwinner/sun8i-v3s-netcube-kumquat.dts   | 290
>> >> > > > ++++++++++++++++++
>> >> > > >  arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi    |   6 +
>> >> > > >  3 files changed, 298 insertions(+)
>> >> > > >  create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v3s-
>> >> > > > netcube-
>> >> > > > kumquat.dts
>> >> > > >
>> >> > > > diff --git a/arch/arm/boot/dts/allwinner/Makefile
>> >> > > > b/arch/arm/boot/dts/allwinner/Makefile
>> >> > > > index 48666f73e638..d799ad153b37 100644
>> >> > > > --- a/arch/arm/boot/dts/allwinner/Makefile
>> >> > > > +++ b/arch/arm/boot/dts/allwinner/Makefile
>> >> > > > @@ -199,6 +199,7 @@ DTC_FLAGS_sun8i-h3-nanopi-r1 := -@
>> >> > > >  DTC_FLAGS_sun8i-h3-orangepi-pc := -@
>> >> > > >  DTC_FLAGS_sun8i-h3-bananapi-m2-plus-v1.2 := -@
>> >> > > >  DTC_FLAGS_sun8i-h3-orangepi-pc-plus := -@
>> >> > > > +DTC_FLAGS_sun8i-v3s-netcube-kumquat := -@
>> >> > > >  dtb-$(CONFIG_MACH_SUN8I) += \
>> >> > > >         sun8i-a23-evb.dtb \
>> >> > > >         sun8i-a23-gt90h-v4.dtb \
>> >> > > > @@ -261,6 +262,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>> >> > > >         sun8i-v3s-anbernic-rg-nano.dtb \
>> >> > > >         sun8i-v3s-licheepi-zero.dtb \
>> >> > > >         sun8i-v3s-licheepi-zero-dock.dtb \
>> >> > > > +       sun8i-v3s-netcube-kumquat.dtb \
>> >> > > >         sun8i-v40-bananapi-m2-berry.dtb
>> >> > > >  dtb-$(CONFIG_MACH_SUN9I) += \
>> >> > > >         sun9i-a80-optimus.dtb \
>> >> > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
>> >> > > > kumquat.dts b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
>> >> > > > kumquat.dts
>> >> > > > new file mode 100644
>> >> > > > index 000000000000..e5d2a716eb69
>> >> > > > --- /dev/null
>> >> > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
>> >> > > > @@ -0,0 +1,290 @@
>> >> > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> >> > > > +/*
>> >> > > > + * Copyright (C) 2025 Lukas Schmid <lukas.schmid@netcube.li>
>> >> > > > + */
>> >> > > > +
>> >> > > > +/dts-v1/;
>> >> > > > +#include "sun8i-v3s.dtsi"
>> >> > > > +
>> >> > > > +#include <dt-bindings/input/input.h>
>> >> > > > +#include <dt-bindings/leds/common.h>
>> >> > > > +#include <dt-bindings/gpio/gpio.h>
>> >> > > > +
>> >> > > > +/{
>> >> > > > +       model = "NetCube Systems Kumquat";
>> >> > > > +       compatible = "netcube,kumquat", "allwinner,sun8i-v3s";
>> >> > > > +
>> >> > > > +       aliases {
>> >> > > > +               serial0 = &uart0;
>> >> > > > +               ethernet0 = &emac;
>> >> > > > +               rtc0 = &ds3232;
>> >> > > > +       };
>> >> > > > +
>> >> > > > +       chosen {
>> >> > > > +               stdout-path = "serial0:115200n8";
>> >> > > > +       };
>> >> > > > +
>> >> > > > +       /* 40 MHz Crystal Oscillator on PCB */
>> >> > > > +       clk_can0: clock-can0 {
>> >> > > > +               compatible = "fixed-clock";
>> >> > > > +               #clock-cells = <0>;
>> >> > > > +               clock-frequency  = <40000000>;
>> >> > > > +       };
>> >> > > > +
>> >> > > > +       gpio-keys {
>> >> > > > +               compatible = "gpio-keys";
>> >> > > > +               autorepeat;
>> >> > > > +
>> >> > > > +               key-user {
>> >> > > > +                       label = "GPIO Key User";
>> >> > > > +                       linux,code = <KEY_PROG1>;
>> >> > > > +                       gpios = <&pio 1 2 (GPIO_ACTIVE_LOW |
>> >> > > > GPIO_PULL_UP)>; /* PB2 */
>> >> > > > +               };
>> >> > > > +       };
>> >> > > > +
>> >> > > > +       leds {
>> >> > > > +               compatible = "gpio-leds";
>> >> > > > +
>> >> > > > +               led-heartbeat {
>> >> > > > +                       gpios = <&pio 4 4 GPIO_ACTIVE_HIGH>; /*
>> >> > > > PE4
>> >> > > > */
>> >> > > > +                       linux,default-trigger = "heartbeat";
>> >> > > > +                       color = <LED_COLOR_ID_GREEN>;
>> >> > > > +                       function = LED_FUNCTION_HEARTBEAT;
>> >> > > > +               };
>> >> > > > +
>> >> > > > +               led-mmc0-act {
>> >> > > > +                       gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /*
>> >> > > > PF6
>> >> > > > */
>> >> > > > +                       linux,default-trigger = "mmc0";
>> >> > > > +                       color = <LED_COLOR_ID_GREEN>;
>> >> > > > +                       function = LED_FUNCTION_DISK;
>> >> > > > +               };
>> >> > > > +       };
>> >> > > > +
>> >> > > > +       /* XC6206-3.0 Linear Regualtor */
>> >> > > > +       reg_vcc3v0: regulator-3v0 {
>> >> > > > +               compatible = "regulator-fixed";
>> >> > > > +               regulator-name = "vcc3v0";
>> >> > > > +               regulator-min-microvolt = <3000000>;
>> >> > > > +               regulator-max-microvolt = <3000000>;
>> >> > > > +               vin-supply = <&reg_vcc3v3>;
>> >> > > > +       };
>> >> > > > +
>> >> > > > +       /* EA3036C Switching 3 Channel Regulator - Channel 2 */
>> >> > > > +       reg_vcc3v3: regulator-3v3 {
>> >> > > > +               compatible = "regulator-fixed";
>> >> > > > +               regulator-name = "vcc3v3";
>> >> > > > +               regulator-min-microvolt = <3300000>;
>> >> > > > +               regulator-max-microvolt = <3300000>;
>> >> > > > +               vin-supply = <&reg_vcc5v0>;
>> >> > > > +       };
>> >> > > > +
>> >> > > > +       /* K7805-1000R3 Switching Regulator supplied from main
>> >> > > > 12/24V terminal block */
>> >> > > > +       reg_vcc5v0: regulator-5v0 {
>> >> > > > +               compatible = "regulator-fixed";
>> >> > > > +               regulator-name = "vcc5v0";
>> >> > > > +               regulator-min-microvolt = <5000000>;
>> >> > > > +               regulator-max-microvolt = <5000000>;
>> >> > > > +       };
>> >> > > > +};
>> >> > > > +
>> >> > > > +&mmc0 {
>> >> > > > +       pinctrl-names = "default";
>> >> > > > +       pinctrl-0 = <&mmc0_pins>;
>> >> > > > +       vmmc-supply = <&reg_vcc3v3>;
>> >> > > > +       bus-width = <4>;
>> >> > > > +       broken-cd;
>> >> > > > +       status = "okay";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&mmc1 {
>> >> > >
>> >> > > what's connected here? Are both MMC ports on headers/connectors,
>> >> > > and
>> >> > > it's up to the user to connect some SDIO device or an SD/eMMC
>> >> > > card/slot? Or is this port connected to the ESP32?
>> >> > >
>> >> > > > +       pinctrl-names = "default";
>> >> > > > +       pinctrl-0 = <&mmc1_pins>;
>> >> > > > +       vmmc-supply = <&reg_vcc3v3>;
>> >> > > > +       bus-width = <4>;
>> >> > > > +       broken-cd;
>> >> > > > +       status = "okay";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&usb_otg {
>> >> > >
>> >> > > I think traditionally referenced nodes in the board .dts files
>> >> > > are
>> >> > > ordered by label name, so usb_otg is but-last. Yes, this is in
>> >> > > contrast
>> >> > > to nodes in the SoC .dtsi file, which are ordered by MMIO
>> >> > > addresses.
>> >> > >
>> >> > > > +       extcon = <&tusb320 0>;
>> >> > > > +       dr_mode = "otg";
>> >> > > > +       status = "okay";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&usbphy {
>> >> > > > +       usb0_id_det-gpios = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4
>> >> > > > */
>> >> > > > +       status = "okay";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&ehci {
>> >> > > > +       status = "okay";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&ohci {
>> >> > > > +       status = "okay";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&rtc {
>> >> > > > +       status = "disabled";
>> >> > >
>> >> > > Please can you explain a bit more what's going on here? I saw you
>> >> > > mentioning in the cover letter that you brought the "disabled"
>> >> > > back,
>> >> > > but I still don't see how this is working when the CCU and the
>> >> > > pinctrl
>> >> > > nodes reference the RTC clocks? So what is broken, exactly? Is
>> >> > > this
>> >> > > some bug in the SoC? Or don't you supply the SoC's VCC_RTC, so
>> >> > > the
>> >> > > calendar is not working when the board is not powered - in
>> >> > > contrast
>> >> > > to
>> >> > > the external RTC?
>> >> >
>> >> > Maybe a lack of crystal? But I can understand nothing here, and a
>> >> > detailed explaination is needed.
>> >> >
>> >>
>> >> I have tried to enable the RTC, but I get a "RTC still busy" from the
>> >> sunxi-rtc module. The RTC Power is connected, and the 32kHz Crystal
>> >> is
> 
> That sounds actually like a bug somewhere, which should be investigated
> and fixed. Disabling the RTC is a quick and dirty workaround, but
> nothing for upstream.
> There is a 50ms timeout, which sounds plenty, although you could
> experiment whether it's too short here:
> drivers/rtc/rtc-sun6i.c:sun6i_rtc_settime()
> My hunch is that even longer timeouts won't change that, so we should
> have a look what's going wrong here, why the RTC does not respond.
> Don't you supply VCC_RTC to the SoC at all, maybe? Maybe we need
> the 3.3V for the RTC to work, regardless of whether that is battery
> backed or not?
> 
>> > Weird... Never heard such kind of things...
>> >
>> > How do modules in the main SoC that depend on LOSC (32768 Hz osc)
>> > perform?
>> >
>> 
>> I didn't notice anything running the board. I had one running for
>> about a month on Kernel 6.8.7 without issues, with the rtc disabled
>> 
>> Do you have anything I should check in particular? From the
>> Datasheet it seems to me that all peripherals in the SoC should
>> be able to run from the hosc too.
> 
> That's a DT issue, you don't *need* the LOSC crystal for operation:
> The clock controller and the GPIO device reference the RTC LOSC clock,
> so the respective drivers go ahead and request them, which triggers
> probing the RTC. If that now fails, because the RTC is disabled, that
> clock cannot be used, and this has consequences:
> - The (combinded) GPIO/pinctrl driver only needs the LOSC clock for the
>   debouncing part. If that fails, the debouncing init routine will bail
>   out (I guess it will not work), but the driver ignores that error and
>   proceeds probing, so you are saved, but more by accident. I wonder if
>   we should at least print a message here.
> - The CCU clock driver only uses the LOSC as a potential parent for
>   some muxes (CPU, AHB, APB2), and the kernel will probably never 
> change
>   those muxes, and even if, not to the LOSC. So again saved here, by
>   chance.
> 
> So this means your system continues working, but it's very sketchy, and
> technically wrong. So disabling the RTC is not really an option for
> mainline, I'd say.

Okay, I'll submit v4 with all changes now, and keep the rtc enabled.
I will try to find out what causes the issue and hopefull create
another patch.

I just checked, and the rtc does not throw the error in 6.8.7, so
that's something to go by...

> 
>> >> also
>> >> on Board. If the RTC's driver wouldn't throw the error I have no
>> >> issue
>> >> with
>> >> leaving the rtc enabled. It would however loose it's memory after
>> >> power
>> >> cycle as the Battery is only connected to the DS3232+, hence the
>> >> alias
>> >> to
>> >> rtc0
>> >>
>> >> > >
>> >> > > > +};
>> >> > > > +
>> >> > > > +&pio {
>> >> > > > +       vcc-pb-supply = <&reg_vcc3v3>;
>> >> > > > +       vcc-pc-supply = <&reg_vcc3v3>;
>> >> > > > +       vcc-pe-supply = <&reg_vcc3v3>;
>> >> > > > +       vcc-pf-supply = <&reg_vcc3v3>;
>> >> > > > +       vcc-pg-supply = <&reg_vcc3v3>;
>> >> > > > +
>> >> > > > +       gpio-reserved-ranges = <0 32>, <42 22>, <68 28>, <96
>> >> > > > 32>,
>> >> > > > <153 7>, <167 25>, <198 26>;
>> >> > >
>> >> > > As mentioned in the reply to the previous patch, this doesn't
>> >> > > look
>> >> > > right here. Why do you need this, exactly?
>> >> >
>> >> > Ah? I don't think there's any tradition on Allwinner platforms to
>> >> > reserve any GPIOs, except if you have another firmware running on
>> >> > another processor (e.g. AR100) that needs some GPIO.
>> >> >
>> >> > My previous sight of such property is on Qualcomm smartphones,
>> >> > where a
>> >> > few GPIOs are reserved for "Trusted" thingy.
>> >> >
>> >>
>> >> My intention here was to have the GPIOs which are not accessible on
>> >> the
>> >> SoC's
>> >> package disabled so that stuff like libgpiod cannot try to access
>> >> them.
>> >> The
>> >> gpiodetect tool did show me them as 'used' when I added the
>> >> reserved-ranges,
>> >> so I thought the driver does understand this tag.
>> >
>> > Interesting point... Maybe this kind of things should enter the per-
>> > SoC(package) DTSI?
> 
> Our sunxi pinctrl driver does not use this information, we have the
> number of pins per port and their availability hardcoded in that large
> table in pinctrl-sun8i-v3s.c.
> 
> If there is more code in the kernel that uses this DT property to
> filter the (userspace?) view of the GPIOs, that's a side effect. If we
> want this filtering, this code should query the pinctrl driver for this
> map, but that's a separate issue. The binding pretty clearly speaks of
> some per-port GPIO drivers to use this map, ours it not one of them.
> 
>> > Although technically they are not "used" but "unavailable".
>> >
>> 
>> Sure, I think I'll remove it from my Patchset for now
> 
> Yes, please, do that, I think it's wrong here.
> 
>> , but maybe will create another set in the future.
> 
> If that solves a problem for you, it should be investigated why, to me
> it looks like it's a side effect, and that should be done properly
> instead - for instance by using all those empty strings below as an
> indicator for availability.
> But I don't think insisting on that property is the right thing.
> 
> Cheers,
> Andre
> 
>> >> > > > +       gpio-line-names = "", "", "", "", // PA
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "CAN_nCS", "CAN_nINT", "USER_SW",
>> >> > > > "PB3",
>> >> > > > // PB
>> >> > > > +                         "USB_ID", "USBC_nINT", "I2C0_SCL",
>> >> > > > "I2C0_SDA",
>> >> > > > +                         "UART0_TX", "UART0_RX", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "SPI_MISO", "SPI_SCK", "FLASH_nCS",
>> >> > > > "SPI_MOSI", // PC
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "", // PD
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "Q12", "Q11", "Q10", "Q9", // PE
>> >> > > > +                         "LED_SYS0", "I1", "Q1", "Q2",
>> >> > > > +                         "I2", "I3", "Q3", "Q4",
>> >> > > > +                         "I4", "I5", "Q5", "Q6",
>> >> > > > +                         "I6", "I7", "Q7", "Q8",
>> >> > > > +                         "I8", "UART1_TXD", "UART1_RXD",
>> >> > > > "ESP_nRST",
>> >> > > > +                         "ESP_nBOOT", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "SD_D1", "SD_D0", "SD_CLK", "SD_CMD",
>> >> > > > //
>> >> > > > PF
>> >> > > > +                         "SD_D3", "SD_D2", "LED_SYS1", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "ESP_CLK", "ESP_CMD", "ESP_D0",
>> >> > > > "ESP_D1",
>> >> > > > // PG
>> >> > > > +                         "ESP_D2", "ESP_D3", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "",
>> >> > > > +                         "", "", "", "";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&lradc {
>> >> > > > +       vref-supply = <&reg_vcc3v0>;
>> >> > > > +       status = "okay";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&codec {
>> >> > > > +       allwinner,audio-routing =
>> >> > > > +               "Headphone", "HP",
>> >> > > > +               "Headphone", "HPCOM",
>> >> > > > +               "MIC1", "Mic",
>> >> > > > +               "Mic", "HBIAS";
>> >> > > > +       status = "okay";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&uart0 {
>> >> > > > +       pinctrl-0 = <&uart0_pb_pins>;
>> >> > > > +       pinctrl-names = "default";
>> >> > > > +       status = "okay";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&uart1 {
>> >> > > > +       pinctrl-0 = <&uart1_pe_pins>;
>> >> > > > +       pinctrl-names = "default";
>> >> > > > +       status = "okay";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&i2c0 {
>> >> > > > +       pinctrl-0 = <&i2c0_pins>;
>> >> > > > +       pinctrl-names = "default";
>> >> > > > +       status = "okay";
>> >> > > > +
>> >> > > > +       ds3232: rtc@68 {
>> >> > > > +               compatible = "dallas,ds3232";
>> >> > > > +               reg = <0x68>;
>> >> > > > +       };
>> >> >
>> >> > If you're afraid of the non-running internal RTC superseding this
>> >> > external RTC, you can use an alias rtc0 = &ds3232 to force the ext.
>> >> > one
>> >> > to be the first.
>> >> >
>> >>
>> >> Yes exactly, I already have an alias to this rtc as rtc0
>> >>
>> >> > > > +
>> >> > > > +       eeprom0: eeprom@50 {
>> >> > > > +               compatible = "atmel,24c02";             /*
>> >> > > > actually
>> >> > > > it's a 24AA02E48 */
>> >> > > > +               pagesize = <16>;
>> >> > > > +               read-only;
>> >> > > > +               reg = <0x50>;
>> >> > > > +               vcc-supply = <&reg_vcc3v3>;
>> >> > > > +
>> >> > > > +               #address-cells = <1>;
>> >> > > > +               #size-cells = <1>;
>> >> > > > +
>> >> > > > +               eth0_macaddress: macaddress@fa {
>> >> > > > +                       reg = <0xfa 0x06>;
>> >> > > > +               };
>> >> > > > +       };
>> >> > > > +
>> >> > > > +       tusb320: typec@60 {
>> >> > > > +               compatible = "ti,tusb320";
>> >> > > > +               reg = <0x60>;
>> >> > > > +               interrupt-parent = <&pio>;
>> >> > > > +               interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
>> >> > > > +       };
>> >> > > > +};
>> >> > > > +
>> >> > > > +&emac {
>> >> > > > +       allwinner,leds-active-low;
>> >> > > > +       nvmem-cells = <&eth0_macaddress>;               /*
>> >> > > > custom
>> >> > > > nvmem reference */
>> >> > > > +       nvmem-cell-names = "mac-address";               /* see
>> >> > > > ethernet-controller.yaml */
>> >> > > > +       status = "okay";
>> >> > > > +};
>> >> > > > +
>> >> > > > +&spi0 {
>> >> > > > +       #address-cells = <1>;
>> >> > > > +       #size-cells = <0>;
>> >> > > > +       pinctrl-names = "default";
>> >> > > > +       pinctrl-0 = <&spi0_pins>;
>> >> > >
>> >> > > Those two lines look redundant, as they are already specified in
>> >> > > the
>> >> > > .dtsi file.
>> >> > >
>> >> > > > +       cs-gpios = <0>, <&pio 1 0 GPIO_ACTIVE_LOW>; /* PB0 */
>> >> > > > +       status = "okay";
>> >> > > > +
>> >> > > > +       flash@0 {
>> >> > > > +               #address-cells = <1>;
>> >> > > > +               #size-cells = <1>;
>> >> > > > +               compatible = "jedec,spi-nor";
>> >> > >
>> >> > > I think traditionally we have the compatible first, and #a-c and
>> >> > > #s-c
>> >> > > last in the node.
>> >> > > And do you have anything partitioned in there? If not, you
>> >> > > wouldn't
>> >> > > need the #a-c and #s-c properties, I think.
>> >> > >
>> >> > > > +               reg = <0>;
>> >> > > > +               label = "firmware";
>> >> > > > +               spi-max-frequency = <40000000>;
>> >> > > > +       };
>> >> > > > +
>> >> > > > +       can@1 {
>> >> > > > +               compatible = "microchip,mcp2518fd";
>> >> > > > +               reg = <1>;
>> >> > > > +               clocks = <&clk_can0>;
>> >> > > > +               interrupt-parent = <&pio>;
>> >> > > > +               interrupts = <1 1 IRQ_TYPE_LEVEL_LOW>;  /* PB1
>> >> > > > */
>> >> > > > +               spi-max-frequency = <20000000>;
>> >> > > > +               vdd-supply = <&reg_vcc3v3>;
>> >> > > > +               xceiver-supply = <&reg_vcc3v3>;
>> >> > > > +       };
>> >> > > > +};
>> >> > > > \ No newline at end of file
>> >> > >
>> >> > > Please add a newline at the end.
>> >> >
>> >> > Well maybe this file is written with some non-Unix-traditional
>> >> > editor,
>> >> > well Linux is something Unix-traditional, and for these editors
>> >> > manual
>> >> > insertion of an empty line will be needed (on Unix-traditional
>> >> > things
>> >> > e.g. Vim, no empty lines should be presented at all.)
>> >> >
>> >> > >
>> >> > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> >> > > > b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> >> > >
>> >> > > I don't know for sure if people want SoC .dtsi patches
>> >> > > separately?
>> >> > >
>> >> > > Cheers,
>> >> > > Andre
>> >> > >
>> >> > > > index 9e13c2aa8911..f909b1d4dbca 100644
>> >> > > > --- a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> >> > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> >> > > > @@ -416,6 +416,12 @@ uart0_pb_pins: uart0-pb-pins {
>> >> > > >                                 function = "uart0";
>> >> > > >                         };
>> >> > > >  
>> >> > > > +                       /omit-if-no-ref/
>> >> > > > +                       uart1_pe_pins: uart1-pe-pins {
>> >> > > > +                               pins = "PE21", "PE22";
>> >> > > > +                               function = "uart1";
>> >> > > > +                       };
>> >> > > > +
>> >> > > >                         uart2_pins: uart2-pins {
>> >> > > >                                 pins = "PB0", "PB1";
>> >> > > >                                 function = "uart2";
>> >> > >
>> >> > >
>> >>
>> >>
>> 
>> 

Re: [PATCH v3 4/4] ARM: dts: sunxi: add support for NetCube Systems Kumquat

[Lukas Schmid]

Am 2025-01-06 18:25, schrieb Lukas Schmid:
> Am 2025-01-06 01:47, schrieb Andre Przywara:
>> On Sun, 05 Jan 2025 12:25:26 +0100
>> Lukas Schmid <lukas.schmid@netcube.li> wrote:
>> 
>>> Am 2025-01-05 05:17, schrieb Icenowy Zheng:
>>> > 在 2025-01-04星期六的 14:03 +0100，Lukas Schmid写道：
>>> >> Am 2025-01-04 06:12, schrieb Icenowy Zheng:
>>> >> > 在 2025-01-03星期五的 23:57 +0000，Andre Przywara写道：
>>> >> > > On Fri,  3 Jan 2025 20:45:20 +0000
>>> >> > > Lukas Schmid <lukas.schmid@netcube.li> wrote:
>>> >> > >
>>> >> > > (CC:ing Icenowy for a question about the RTC below ...)
>>> >> > >
>>> >> > > > NetCube Systems Kumquat is a board based on the Allwinner V3s
>>> >> > > > SoC,
>>> >> > > > including:
>>> >> > > >
>>> >> > > > - 64MB DDR2 included in SoC
>>> >> > > > - 10/100 Mbps Ethernet
>>> >> > > > - USB-C DRD
>>> >> > > > - Audio Codec
>>> >> > > > - Isolated CAN-FD
>>> >> > > > - ESP32 over SDIO
>>> >> > > > - 8MB SPI-NOR Flash for bootloader
>>> >> > > > - I2C EEPROM for MAC addresses
>>> >> > > > - SDIO Connector for eMMC or SD-Card
>>> >> > > > - 8x 12/24V IOs, 4x normally open relays
>>> >> > > > - DS3232 RTC
>>> >> > > > - QWIIC connectors for external I2C devices
>>> >> > > >
>>> >> > > > Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
>>> >> > > > ---
>>> >> > > >  arch/arm/boot/dts/allwinner/Makefile          |   2 +
>>> >> > > >  .../allwinner/sun8i-v3s-netcube-kumquat.dts   | 290
>>> >> > > > ++++++++++++++++++
>>> >> > > >  arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi    |   6 +
>>> >> > > >  3 files changed, 298 insertions(+)
>>> >> > > >  create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v3s-
>>> >> > > > netcube-
>>> >> > > > kumquat.dts
>>> >> > > >
>>> >> > > > diff --git a/arch/arm/boot/dts/allwinner/Makefile
>>> >> > > > b/arch/arm/boot/dts/allwinner/Makefile
>>> >> > > > index 48666f73e638..d799ad153b37 100644
>>> >> > > > --- a/arch/arm/boot/dts/allwinner/Makefile
>>> >> > > > +++ b/arch/arm/boot/dts/allwinner/Makefile
>>> >> > > > @@ -199,6 +199,7 @@ DTC_FLAGS_sun8i-h3-nanopi-r1 := -@
>>> >> > > >  DTC_FLAGS_sun8i-h3-orangepi-pc := -@
>>> >> > > >  DTC_FLAGS_sun8i-h3-bananapi-m2-plus-v1.2 := -@
>>> >> > > >  DTC_FLAGS_sun8i-h3-orangepi-pc-plus := -@
>>> >> > > > +DTC_FLAGS_sun8i-v3s-netcube-kumquat := -@
>>> >> > > >  dtb-$(CONFIG_MACH_SUN8I) += \
>>> >> > > >         sun8i-a23-evb.dtb \
>>> >> > > >         sun8i-a23-gt90h-v4.dtb \
>>> >> > > > @@ -261,6 +262,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>>> >> > > >         sun8i-v3s-anbernic-rg-nano.dtb \
>>> >> > > >         sun8i-v3s-licheepi-zero.dtb \
>>> >> > > >         sun8i-v3s-licheepi-zero-dock.dtb \
>>> >> > > > +       sun8i-v3s-netcube-kumquat.dtb \
>>> >> > > >         sun8i-v40-bananapi-m2-berry.dtb
>>> >> > > >  dtb-$(CONFIG_MACH_SUN9I) += \
>>> >> > > >         sun9i-a80-optimus.dtb \
>>> >> > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
>>> >> > > > kumquat.dts b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-
>>> >> > > > kumquat.dts
>>> >> > > > new file mode 100644
>>> >> > > > index 000000000000..e5d2a716eb69
>>> >> > > > --- /dev/null
>>> >> > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
>>> >> > > > @@ -0,0 +1,290 @@
>>> >> > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>> >> > > > +/*
>>> >> > > > + * Copyright (C) 2025 Lukas Schmid <lukas.schmid@netcube.li>
>>> >> > > > + */
>>> >> > > > +
>>> >> > > > +/dts-v1/;
>>> >> > > > +#include "sun8i-v3s.dtsi"
>>> >> > > > +
>>> >> > > > +#include <dt-bindings/input/input.h>
>>> >> > > > +#include <dt-bindings/leds/common.h>
>>> >> > > > +#include <dt-bindings/gpio/gpio.h>
>>> >> > > > +
>>> >> > > > +/{
>>> >> > > > +       model = "NetCube Systems Kumquat";
>>> >> > > > +       compatible = "netcube,kumquat", "allwinner,sun8i-v3s";
>>> >> > > > +
>>> >> > > > +       aliases {
>>> >> > > > +               serial0 = &uart0;
>>> >> > > > +               ethernet0 = &emac;
>>> >> > > > +               rtc0 = &ds3232;
>>> >> > > > +       };
>>> >> > > > +
>>> >> > > > +       chosen {
>>> >> > > > +               stdout-path = "serial0:115200n8";
>>> >> > > > +       };
>>> >> > > > +
>>> >> > > > +       /* 40 MHz Crystal Oscillator on PCB */
>>> >> > > > +       clk_can0: clock-can0 {
>>> >> > > > +               compatible = "fixed-clock";
>>> >> > > > +               #clock-cells = <0>;
>>> >> > > > +               clock-frequency  = <40000000>;
>>> >> > > > +       };
>>> >> > > > +
>>> >> > > > +       gpio-keys {
>>> >> > > > +               compatible = "gpio-keys";
>>> >> > > > +               autorepeat;
>>> >> > > > +
>>> >> > > > +               key-user {
>>> >> > > > +                       label = "GPIO Key User";
>>> >> > > > +                       linux,code = <KEY_PROG1>;
>>> >> > > > +                       gpios = <&pio 1 2 (GPIO_ACTIVE_LOW |
>>> >> > > > GPIO_PULL_UP)>; /* PB2 */
>>> >> > > > +               };
>>> >> > > > +       };
>>> >> > > > +
>>> >> > > > +       leds {
>>> >> > > > +               compatible = "gpio-leds";
>>> >> > > > +
>>> >> > > > +               led-heartbeat {
>>> >> > > > +                       gpios = <&pio 4 4 GPIO_ACTIVE_HIGH>; /*
>>> >> > > > PE4
>>> >> > > > */
>>> >> > > > +                       linux,default-trigger = "heartbeat";
>>> >> > > > +                       color = <LED_COLOR_ID_GREEN>;
>>> >> > > > +                       function = LED_FUNCTION_HEARTBEAT;
>>> >> > > > +               };
>>> >> > > > +
>>> >> > > > +               led-mmc0-act {
>>> >> > > > +                       gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /*
>>> >> > > > PF6
>>> >> > > > */
>>> >> > > > +                       linux,default-trigger = "mmc0";
>>> >> > > > +                       color = <LED_COLOR_ID_GREEN>;
>>> >> > > > +                       function = LED_FUNCTION_DISK;
>>> >> > > > +               };
>>> >> > > > +       };
>>> >> > > > +
>>> >> > > > +       /* XC6206-3.0 Linear Regualtor */
>>> >> > > > +       reg_vcc3v0: regulator-3v0 {
>>> >> > > > +               compatible = "regulator-fixed";
>>> >> > > > +               regulator-name = "vcc3v0";
>>> >> > > > +               regulator-min-microvolt = <3000000>;
>>> >> > > > +               regulator-max-microvolt = <3000000>;
>>> >> > > > +               vin-supply = <&reg_vcc3v3>;
>>> >> > > > +       };
>>> >> > > > +
>>> >> > > > +       /* EA3036C Switching 3 Channel Regulator - Channel 2 */
>>> >> > > > +       reg_vcc3v3: regulator-3v3 {
>>> >> > > > +               compatible = "regulator-fixed";
>>> >> > > > +               regulator-name = "vcc3v3";
>>> >> > > > +               regulator-min-microvolt = <3300000>;
>>> >> > > > +               regulator-max-microvolt = <3300000>;
>>> >> > > > +               vin-supply = <&reg_vcc5v0>;
>>> >> > > > +       };
>>> >> > > > +
>>> >> > > > +       /* K7805-1000R3 Switching Regulator supplied from main
>>> >> > > > 12/24V terminal block */
>>> >> > > > +       reg_vcc5v0: regulator-5v0 {
>>> >> > > > +               compatible = "regulator-fixed";
>>> >> > > > +               regulator-name = "vcc5v0";
>>> >> > > > +               regulator-min-microvolt = <5000000>;
>>> >> > > > +               regulator-max-microvolt = <5000000>;
>>> >> > > > +       };
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&mmc0 {
>>> >> > > > +       pinctrl-names = "default";
>>> >> > > > +       pinctrl-0 = <&mmc0_pins>;
>>> >> > > > +       vmmc-supply = <&reg_vcc3v3>;
>>> >> > > > +       bus-width = <4>;
>>> >> > > > +       broken-cd;
>>> >> > > > +       status = "okay";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&mmc1 {
>>> >> > >
>>> >> > > what's connected here? Are both MMC ports on headers/connectors,
>>> >> > > and
>>> >> > > it's up to the user to connect some SDIO device or an SD/eMMC
>>> >> > > card/slot? Or is this port connected to the ESP32?
>>> >> > >
>>> >> > > > +       pinctrl-names = "default";
>>> >> > > > +       pinctrl-0 = <&mmc1_pins>;
>>> >> > > > +       vmmc-supply = <&reg_vcc3v3>;
>>> >> > > > +       bus-width = <4>;
>>> >> > > > +       broken-cd;
>>> >> > > > +       status = "okay";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&usb_otg {
>>> >> > >
>>> >> > > I think traditionally referenced nodes in the board .dts files
>>> >> > > are
>>> >> > > ordered by label name, so usb_otg is but-last. Yes, this is in
>>> >> > > contrast
>>> >> > > to nodes in the SoC .dtsi file, which are ordered by MMIO
>>> >> > > addresses.
>>> >> > >
>>> >> > > > +       extcon = <&tusb320 0>;
>>> >> > > > +       dr_mode = "otg";
>>> >> > > > +       status = "okay";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&usbphy {
>>> >> > > > +       usb0_id_det-gpios = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4
>>> >> > > > */
>>> >> > > > +       status = "okay";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&ehci {
>>> >> > > > +       status = "okay";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&ohci {
>>> >> > > > +       status = "okay";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&rtc {
>>> >> > > > +       status = "disabled";
>>> >> > >
>>> >> > > Please can you explain a bit more what's going on here? I saw you
>>> >> > > mentioning in the cover letter that you brought the "disabled"
>>> >> > > back,
>>> >> > > but I still don't see how this is working when the CCU and the
>>> >> > > pinctrl
>>> >> > > nodes reference the RTC clocks? So what is broken, exactly? Is
>>> >> > > this
>>> >> > > some bug in the SoC? Or don't you supply the SoC's VCC_RTC, so
>>> >> > > the
>>> >> > > calendar is not working when the board is not powered - in
>>> >> > > contrast
>>> >> > > to
>>> >> > > the external RTC?
>>> >> >
>>> >> > Maybe a lack of crystal? But I can understand nothing here, and a
>>> >> > detailed explaination is needed.
>>> >> >
>>> >>
>>> >> I have tried to enable the RTC, but I get a "RTC still busy" from the
>>> >> sunxi-rtc module. The RTC Power is connected, and the 32kHz Crystal
>>> >> is
>> 
>> That sounds actually like a bug somewhere, which should be 
>> investigated
>> and fixed. Disabling the RTC is a quick and dirty workaround, but
>> nothing for upstream.
>> There is a 50ms timeout, which sounds plenty, although you could
>> experiment whether it's too short here:
>> drivers/rtc/rtc-sun6i.c:sun6i_rtc_settime()
>> My hunch is that even longer timeouts won't change that, so we should
>> have a look what's going wrong here, why the RTC does not respond.
>> Don't you supply VCC_RTC to the SoC at all, maybe? Maybe we need
>> the 3.3V for the RTC to work, regardless of whether that is battery
>> backed or not?
>> 
>>> > Weird... Never heard such kind of things...
>>> >
>>> > How do modules in the main SoC that depend on LOSC (32768 Hz osc)
>>> > perform?
>>> >
>>> 
>>> I didn't notice anything running the board. I had one running for
>>> about a month on Kernel 6.8.7 without issues, with the rtc disabled
>>> 
>>> Do you have anything I should check in particular? From the
>>> Datasheet it seems to me that all peripherals in the SoC should
>>> be able to run from the hosc too.
>> 
>> That's a DT issue, you don't *need* the LOSC crystal for operation:
>> The clock controller and the GPIO device reference the RTC LOSC clock,
>> so the respective drivers go ahead and request them, which triggers
>> probing the RTC. If that now fails, because the RTC is disabled, that
>> clock cannot be used, and this has consequences:
>> - The (combinded) GPIO/pinctrl driver only needs the LOSC clock for 
>> the
>>   debouncing part. If that fails, the debouncing init routine will 
>> bail
>>   out (I guess it will not work), but the driver ignores that error 
>> and
>>   proceeds probing, so you are saved, but more by accident. I wonder 
>> if
>>   we should at least print a message here.
>> - The CCU clock driver only uses the LOSC as a potential parent for
>>   some muxes (CPU, AHB, APB2), and the kernel will probably never
>> change
>>   those muxes, and even if, not to the LOSC. So again saved here, by
>>   chance.
>> 
>> So this means your system continues working, but it's very sketchy, 
>> and
>> technically wrong. So disabling the RTC is not really an option for
>> mainline, I'd say.
> 
> Okay, I'll submit v4 with all changes now, and keep the rtc enabled.
> I will try to find out what causes the issue and hopefull create
> another patch.
> 
> I just checked, and the rtc does not throw the error in 6.8.7, so
> that's something to go by...
> 

So I do not know what the issue with the rtc was. I assume it's a
fault on my development-board. I tried it again with a production
pcb, and on it the rtc in the SoC works as intended. I have sent the
new patches with the rtc node removed, and another alias to force the
internal rtc to be rtc1, as the sun6i-rtc seems to get loaded before
the ds3232+.

>> 
>>> >> also
>>> >> on Board. If the RTC's driver wouldn't throw the error I have no
>>> >> issue
>>> >> with
>>> >> leaving the rtc enabled. It would however loose it's memory after
>>> >> power
>>> >> cycle as the Battery is only connected to the DS3232+, hence the
>>> >> alias
>>> >> to
>>> >> rtc0
>>> >>
>>> >> > >
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&pio {
>>> >> > > > +       vcc-pb-supply = <&reg_vcc3v3>;
>>> >> > > > +       vcc-pc-supply = <&reg_vcc3v3>;
>>> >> > > > +       vcc-pe-supply = <&reg_vcc3v3>;
>>> >> > > > +       vcc-pf-supply = <&reg_vcc3v3>;
>>> >> > > > +       vcc-pg-supply = <&reg_vcc3v3>;
>>> >> > > > +
>>> >> > > > +       gpio-reserved-ranges = <0 32>, <42 22>, <68 28>, <96
>>> >> > > > 32>,
>>> >> > > > <153 7>, <167 25>, <198 26>;
>>> >> > >
>>> >> > > As mentioned in the reply to the previous patch, this doesn't
>>> >> > > look
>>> >> > > right here. Why do you need this, exactly?
>>> >> >
>>> >> > Ah? I don't think there's any tradition on Allwinner platforms to
>>> >> > reserve any GPIOs, except if you have another firmware running on
>>> >> > another processor (e.g. AR100) that needs some GPIO.
>>> >> >
>>> >> > My previous sight of such property is on Qualcomm smartphones,
>>> >> > where a
>>> >> > few GPIOs are reserved for "Trusted" thingy.
>>> >> >
>>> >>
>>> >> My intention here was to have the GPIOs which are not accessible on
>>> >> the
>>> >> SoC's
>>> >> package disabled so that stuff like libgpiod cannot try to access
>>> >> them.
>>> >> The
>>> >> gpiodetect tool did show me them as 'used' when I added the
>>> >> reserved-ranges,
>>> >> so I thought the driver does understand this tag.
>>> >
>>> > Interesting point... Maybe this kind of things should enter the per-
>>> > SoC(package) DTSI?
>> 
>> Our sunxi pinctrl driver does not use this information, we have the
>> number of pins per port and their availability hardcoded in that large
>> table in pinctrl-sun8i-v3s.c.
>> 
>> If there is more code in the kernel that uses this DT property to
>> filter the (userspace?) view of the GPIOs, that's a side effect. If we
>> want this filtering, this code should query the pinctrl driver for 
>> this
>> map, but that's a separate issue. The binding pretty clearly speaks of
>> some per-port GPIO drivers to use this map, ours it not one of them.
>> 
>>> > Although technically they are not "used" but "unavailable".
>>> >
>>> 
>>> Sure, I think I'll remove it from my Patchset for now
>> 
>> Yes, please, do that, I think it's wrong here.
>> 
>>> , but maybe will create another set in the future.
>> 
>> If that solves a problem for you, it should be investigated why, to me
>> it looks like it's a side effect, and that should be done properly
>> instead - for instance by using all those empty strings below as an
>> indicator for availability.
>> But I don't think insisting on that property is the right thing.
>> 
>> Cheers,
>> Andre
>> 
>>> >> > > > +       gpio-line-names = "", "", "", "", // PA
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "CAN_nCS", "CAN_nINT", "USER_SW",
>>> >> > > > "PB3",
>>> >> > > > // PB
>>> >> > > > +                         "USB_ID", "USBC_nINT", "I2C0_SCL",
>>> >> > > > "I2C0_SDA",
>>> >> > > > +                         "UART0_TX", "UART0_RX", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "SPI_MISO", "SPI_SCK", "FLASH_nCS",
>>> >> > > > "SPI_MOSI", // PC
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "", // PD
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "Q12", "Q11", "Q10", "Q9", // PE
>>> >> > > > +                         "LED_SYS0", "I1", "Q1", "Q2",
>>> >> > > > +                         "I2", "I3", "Q3", "Q4",
>>> >> > > > +                         "I4", "I5", "Q5", "Q6",
>>> >> > > > +                         "I6", "I7", "Q7", "Q8",
>>> >> > > > +                         "I8", "UART1_TXD", "UART1_RXD",
>>> >> > > > "ESP_nRST",
>>> >> > > > +                         "ESP_nBOOT", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "SD_D1", "SD_D0", "SD_CLK", "SD_CMD",
>>> >> > > > //
>>> >> > > > PF
>>> >> > > > +                         "SD_D3", "SD_D2", "LED_SYS1", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "ESP_CLK", "ESP_CMD", "ESP_D0",
>>> >> > > > "ESP_D1",
>>> >> > > > // PG
>>> >> > > > +                         "ESP_D2", "ESP_D3", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "",
>>> >> > > > +                         "", "", "", "";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&lradc {
>>> >> > > > +       vref-supply = <&reg_vcc3v0>;
>>> >> > > > +       status = "okay";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&codec {
>>> >> > > > +       allwinner,audio-routing =
>>> >> > > > +               "Headphone", "HP",
>>> >> > > > +               "Headphone", "HPCOM",
>>> >> > > > +               "MIC1", "Mic",
>>> >> > > > +               "Mic", "HBIAS";
>>> >> > > > +       status = "okay";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&uart0 {
>>> >> > > > +       pinctrl-0 = <&uart0_pb_pins>;
>>> >> > > > +       pinctrl-names = "default";
>>> >> > > > +       status = "okay";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&uart1 {
>>> >> > > > +       pinctrl-0 = <&uart1_pe_pins>;
>>> >> > > > +       pinctrl-names = "default";
>>> >> > > > +       status = "okay";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&i2c0 {
>>> >> > > > +       pinctrl-0 = <&i2c0_pins>;
>>> >> > > > +       pinctrl-names = "default";
>>> >> > > > +       status = "okay";
>>> >> > > > +
>>> >> > > > +       ds3232: rtc@68 {
>>> >> > > > +               compatible = "dallas,ds3232";
>>> >> > > > +               reg = <0x68>;
>>> >> > > > +       };
>>> >> >
>>> >> > If you're afraid of the non-running internal RTC superseding this
>>> >> > external RTC, you can use an alias rtc0 = &ds3232 to force the ext.
>>> >> > one
>>> >> > to be the first.
>>> >> >
>>> >>
>>> >> Yes exactly, I already have an alias to this rtc as rtc0
>>> >>
>>> >> > > > +
>>> >> > > > +       eeprom0: eeprom@50 {
>>> >> > > > +               compatible = "atmel,24c02";             /*
>>> >> > > > actually
>>> >> > > > it's a 24AA02E48 */
>>> >> > > > +               pagesize = <16>;
>>> >> > > > +               read-only;
>>> >> > > > +               reg = <0x50>;
>>> >> > > > +               vcc-supply = <&reg_vcc3v3>;
>>> >> > > > +
>>> >> > > > +               #address-cells = <1>;
>>> >> > > > +               #size-cells = <1>;
>>> >> > > > +
>>> >> > > > +               eth0_macaddress: macaddress@fa {
>>> >> > > > +                       reg = <0xfa 0x06>;
>>> >> > > > +               };
>>> >> > > > +       };
>>> >> > > > +
>>> >> > > > +       tusb320: typec@60 {
>>> >> > > > +               compatible = "ti,tusb320";
>>> >> > > > +               reg = <0x60>;
>>> >> > > > +               interrupt-parent = <&pio>;
>>> >> > > > +               interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
>>> >> > > > +       };
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&emac {
>>> >> > > > +       allwinner,leds-active-low;
>>> >> > > > +       nvmem-cells = <&eth0_macaddress>;               /*
>>> >> > > > custom
>>> >> > > > nvmem reference */
>>> >> > > > +       nvmem-cell-names = "mac-address";               /* see
>>> >> > > > ethernet-controller.yaml */
>>> >> > > > +       status = "okay";
>>> >> > > > +};
>>> >> > > > +
>>> >> > > > +&spi0 {
>>> >> > > > +       #address-cells = <1>;
>>> >> > > > +       #size-cells = <0>;
>>> >> > > > +       pinctrl-names = "default";
>>> >> > > > +       pinctrl-0 = <&spi0_pins>;
>>> >> > >
>>> >> > > Those two lines look redundant, as they are already specified in
>>> >> > > the
>>> >> > > .dtsi file.
>>> >> > >
>>> >> > > > +       cs-gpios = <0>, <&pio 1 0 GPIO_ACTIVE_LOW>; /* PB0 */
>>> >> > > > +       status = "okay";
>>> >> > > > +
>>> >> > > > +       flash@0 {
>>> >> > > > +               #address-cells = <1>;
>>> >> > > > +               #size-cells = <1>;
>>> >> > > > +               compatible = "jedec,spi-nor";
>>> >> > >
>>> >> > > I think traditionally we have the compatible first, and #a-c and
>>> >> > > #s-c
>>> >> > > last in the node.
>>> >> > > And do you have anything partitioned in there? If not, you
>>> >> > > wouldn't
>>> >> > > need the #a-c and #s-c properties, I think.
>>> >> > >
>>> >> > > > +               reg = <0>;
>>> >> > > > +               label = "firmware";
>>> >> > > > +               spi-max-frequency = <40000000>;
>>> >> > > > +       };
>>> >> > > > +
>>> >> > > > +       can@1 {
>>> >> > > > +               compatible = "microchip,mcp2518fd";
>>> >> > > > +               reg = <1>;
>>> >> > > > +               clocks = <&clk_can0>;
>>> >> > > > +               interrupt-parent = <&pio>;
>>> >> > > > +               interrupts = <1 1 IRQ_TYPE_LEVEL_LOW>;  /* PB1
>>> >> > > > */
>>> >> > > > +               spi-max-frequency = <20000000>;
>>> >> > > > +               vdd-supply = <&reg_vcc3v3>;
>>> >> > > > +               xceiver-supply = <&reg_vcc3v3>;
>>> >> > > > +       };
>>> >> > > > +};
>>> >> > > > \ No newline at end of file
>>> >> > >
>>> >> > > Please add a newline at the end.
>>> >> >
>>> >> > Well maybe this file is written with some non-Unix-traditional
>>> >> > editor,
>>> >> > well Linux is something Unix-traditional, and for these editors
>>> >> > manual
>>> >> > insertion of an empty line will be needed (on Unix-traditional
>>> >> > things
>>> >> > e.g. Vim, no empty lines should be presented at all.)
>>> >> >
>>> >> > >
>>> >> > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>>> >> > > > b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>>> >> > >
>>> >> > > I don't know for sure if people want SoC .dtsi patches
>>> >> > > separately?
>>> >> > >
>>> >> > > Cheers,
>>> >> > > Andre
>>> >> > >
>>> >> > > > index 9e13c2aa8911..f909b1d4dbca 100644
>>> >> > > > --- a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>>> >> > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>>> >> > > > @@ -416,6 +416,12 @@ uart0_pb_pins: uart0-pb-pins {
>>> >> > > >                                 function = "uart0";
>>> >> > > >                         };
>>> >> > > >  
>>> >> > > > +                       /omit-if-no-ref/
>>> >> > > > +                       uart1_pe_pins: uart1-pe-pins {
>>> >> > > > +                               pins = "PE21", "PE22";
>>> >> > > > +                               function = "uart1";
>>> >> > > > +                       };
>>> >> > > > +
>>> >> > > >                         uart2_pins: uart2-pins {
>>> >> > > >                                 pins = "PB0", "PB1";
>>> >> > > >                                 function = "uart2";
>>> >> > >
>>> >> > >
>>> >>
>>> >>
>>> 
>>> 

Re: [PATCH v3 4/4] ARM: dts: sunxi: add support for NetCube Systems Kumquat

[Lukas Schmid]

Am 2025-01-04 00:57, schrieb Andre Przywara:
> On Fri,  3 Jan 2025 20:45:20 +0000
> Lukas Schmid <lukas.schmid@netcube.li> wrote:
> 
> (CC:ing Icenowy for a question about the RTC below ...)
> 
>> NetCube Systems Kumquat is a board based on the Allwinner V3s SoC,
>> including:
>> 
>> - 64MB DDR2 included in SoC
>> - 10/100 Mbps Ethernet
>> - USB-C DRD
>> - Audio Codec
>> - Isolated CAN-FD
>> - ESP32 over SDIO
>> - 8MB SPI-NOR Flash for bootloader
>> - I2C EEPROM for MAC addresses
>> - SDIO Connector for eMMC or SD-Card
>> - 8x 12/24V IOs, 4x normally open relays
>> - DS3232 RTC
>> - QWIIC connectors for external I2C devices
>> 
>> Signed-off-by: Lukas Schmid <lukas.schmid@netcube.li>
>> ---
>>  arch/arm/boot/dts/allwinner/Makefile          |   2 +
>>  .../allwinner/sun8i-v3s-netcube-kumquat.dts   | 290 
>> ++++++++++++++++++
>>  arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi    |   6 +
>>  3 files changed, 298 insertions(+)
>>  create mode 100644 
>> arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
>> 
>> diff --git a/arch/arm/boot/dts/allwinner/Makefile 
>> b/arch/arm/boot/dts/allwinner/Makefile
>> index 48666f73e638..d799ad153b37 100644
>> --- a/arch/arm/boot/dts/allwinner/Makefile
>> +++ b/arch/arm/boot/dts/allwinner/Makefile
>> @@ -199,6 +199,7 @@ DTC_FLAGS_sun8i-h3-nanopi-r1 := -@
>>  DTC_FLAGS_sun8i-h3-orangepi-pc := -@
>>  DTC_FLAGS_sun8i-h3-bananapi-m2-plus-v1.2 := -@
>>  DTC_FLAGS_sun8i-h3-orangepi-pc-plus := -@
>> +DTC_FLAGS_sun8i-v3s-netcube-kumquat := -@
>>  dtb-$(CONFIG_MACH_SUN8I) += \
>>  	sun8i-a23-evb.dtb \
>>  	sun8i-a23-gt90h-v4.dtb \
>> @@ -261,6 +262,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>>  	sun8i-v3s-anbernic-rg-nano.dtb \
>>  	sun8i-v3s-licheepi-zero.dtb \
>>  	sun8i-v3s-licheepi-zero-dock.dtb \
>> +	sun8i-v3s-netcube-kumquat.dtb \
>>  	sun8i-v40-bananapi-m2-berry.dtb
>>  dtb-$(CONFIG_MACH_SUN9I) += \
>>  	sun9i-a80-optimus.dtb \
>> diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts 
>> b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
>> new file mode 100644
>> index 000000000000..e5d2a716eb69
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
>> @@ -0,0 +1,290 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (C) 2025 Lukas Schmid <lukas.schmid@netcube.li>
>> + */
>> +
>> +/dts-v1/;
>> +#include "sun8i-v3s.dtsi"
>> +
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/leds/common.h>
>> +#include <dt-bindings/gpio/gpio.h>
>> +
>> +/{
>> +	model = "NetCube Systems Kumquat";
>> +	compatible = "netcube,kumquat", "allwinner,sun8i-v3s";
>> +
>> +	aliases {
>> +		serial0 = &uart0;
>> +		ethernet0 = &emac;
>> +		rtc0 = &ds3232;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial0:115200n8";
>> +	};
>> +
>> +	/* 40 MHz Crystal Oscillator on PCB */
>> +	clk_can0: clock-can0 {
>> +		compatible = "fixed-clock";
>> +		#clock-cells = <0>;
>> +		clock-frequency  = <40000000>;
>> +	};
>> +
>> +	gpio-keys {
>> +		compatible = "gpio-keys";
>> +		autorepeat;
>> +
>> +		key-user {
>> +			label = "GPIO Key User";
>> +			linux,code = <KEY_PROG1>;
>> +			gpios = <&pio 1 2 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; /* PB2 */
>> +		};
>> +	};
>> +
>> +	leds {
>> +		compatible = "gpio-leds";
>> +
>> +		led-heartbeat {
>> +			gpios = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4 */
>> +			linux,default-trigger = "heartbeat";
>> +			color = <LED_COLOR_ID_GREEN>;
>> +			function = LED_FUNCTION_HEARTBEAT;
>> +		};
>> +
>> +		led-mmc0-act {
>> +			gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
>> +			linux,default-trigger = "mmc0";
>> +			color = <LED_COLOR_ID_GREEN>;
>> +			function = LED_FUNCTION_DISK;
>> +		};
>> +	};
>> +
>> +	/* XC6206-3.0 Linear Regualtor */
>> +	reg_vcc3v0: regulator-3v0 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc3v0";
>> +		regulator-min-microvolt = <3000000>;
>> +		regulator-max-microvolt = <3000000>;
>> +		vin-supply = <&reg_vcc3v3>;
>> +	};
>> +
>> +	/* EA3036C Switching 3 Channel Regulator - Channel 2 */
>> +	reg_vcc3v3: regulator-3v3 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc3v3";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		vin-supply = <&reg_vcc5v0>;
>> +	};
>> +
>> +	/* K7805-1000R3 Switching Regulator supplied from main 12/24V 
>> terminal block */
>> +	reg_vcc5v0: regulator-5v0 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc5v0";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +	};
>> +};
>> +
>> +&mmc0 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&mmc0_pins>;
>> +	vmmc-supply = <&reg_vcc3v3>;
>> +	bus-width = <4>;
>> +	broken-cd;
>> +	status = "okay";
>> +};
>> +
>> +&mmc1 {
> 
> what's connected here? Are both MMC ports on headers/connectors, and
> it's up to the user to connect some SDIO device or an SD/eMMC
> card/slot? Or is this port connected to the ESP32?
> 

The MMC0 is on a pin-header taking either an eMMC or SD-Card adapter. I
have added a description in a comment for both mmc nodes.

MMC1 is directly connected to an ESP32

>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&mmc1_pins>;
>> +	vmmc-supply = <&reg_vcc3v3>;
>> +	bus-width = <4>;
>> +	broken-cd;
>> +	status = "okay";
>> +};
>> +
>> +&usb_otg {
> 
> I think traditionally referenced nodes in the board .dts files are
> ordered by label name, so usb_otg is but-last. Yes, this is in contrast
> to nodes in the SoC .dtsi file, which are ordered by MMIO addresses.
> 

I have read in the Kernel Documentation that I can either have the 
alphabetically
or can order them the same as they are specified in the dtsi. I choose 
the dtsi
as this keeps the usb nodes together.

>> +	extcon = <&tusb320 0>;
>> +	dr_mode = "otg";
>> +	status = "okay";
>> +};
>> +
>> +&usbphy {
>> +	usb0_id_det-gpios = <&pio 1 4 GPIO_ACTIVE_HIGH>; /* PB4 */
>> +	status = "okay";
>> +};
>> +
>> +&ehci {
>> +	status = "okay";
>> +};
>> +
>> +&ohci {
>> +	status = "okay";
>> +};
>> +
>> +&rtc {
>> +	status = "disabled";
> 
> Please can you explain a bit more what's going on here? I saw you
> mentioning in the cover letter that you brought the "disabled" back,
> but I still don't see how this is working when the CCU and the pinctrl
> nodes reference the RTC clocks? So what is broken, exactly? Is this
> some bug in the SoC? Or don't you supply the SoC's VCC_RTC, so the
> calendar is not working when the board is not powered - in contrast to
> the external RTC?
> 

As I already mentioned in the reply to Icenowy. The RTC is connected 
fully
except the battery is only connected to the external DS3232+. I would 
have
no issue keeping the internal one enabled, but the kernel driver/module
keeps throwing errors that the RTC is still busy.

>> +};
>> +
>> +&pio {
>> +	vcc-pb-supply = <&reg_vcc3v3>;
>> +	vcc-pc-supply = <&reg_vcc3v3>;
>> +	vcc-pe-supply = <&reg_vcc3v3>;
>> +	vcc-pf-supply = <&reg_vcc3v3>;
>> +	vcc-pg-supply = <&reg_vcc3v3>;
>> +
>> +	gpio-reserved-ranges = <0 32>, <42 22>, <68 28>, <96 32>, <153 7>, 
>> <167 25>, <198 26>;
> 
> As mentioned in the reply to the previous patch, this doesn't look
> right here. Why do you need this, exactly?
> 
>> +	gpio-line-names = "", "", "", "", // PA
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "CAN_nCS", "CAN_nINT", "USER_SW", "PB3", // PB
>> +			  "USB_ID", "USBC_nINT", "I2C0_SCL", "I2C0_SDA",
>> +			  "UART0_TX", "UART0_RX", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "SPI_MISO", "SPI_SCK", "FLASH_nCS", "SPI_MOSI", // PC
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "", // PD
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "Q12", "Q11", "Q10", "Q9", // PE
>> +			  "LED_SYS0", "I1", "Q1", "Q2",
>> +			  "I2", "I3", "Q3", "Q4",
>> +			  "I4", "I5", "Q5", "Q6",
>> +			  "I6", "I7", "Q7", "Q8",
>> +			  "I8", "UART1_TXD", "UART1_RXD", "ESP_nRST",
>> +			  "ESP_nBOOT", "", "", "",
>> +			  "", "", "", "",
>> +			  "SD_D1", "SD_D0", "SD_CLK", "SD_CMD", // PF
>> +			  "SD_D3", "SD_D2", "LED_SYS1", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "ESP_CLK", "ESP_CMD", "ESP_D0", "ESP_D1", // PG
>> +			  "ESP_D2", "ESP_D3", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "",
>> +			  "", "", "", "";
>> +};
>> +
>> +&lradc {
>> +	vref-supply = <&reg_vcc3v0>;
>> +	status = "okay";
>> +};
>> +
>> +&codec {
>> +	allwinner,audio-routing =
>> +		"Headphone", "HP",
>> +		"Headphone", "HPCOM",
>> +		"MIC1", "Mic",
>> +		"Mic", "HBIAS";
>> +	status = "okay";
>> +};
>> +
>> +&uart0 {
>> +	pinctrl-0 = <&uart0_pb_pins>;
>> +	pinctrl-names = "default";
>> +	status = "okay";
>> +};
>> +
>> +&uart1 {
>> +	pinctrl-0 = <&uart1_pe_pins>;
>> +	pinctrl-names = "default";
>> +	status = "okay";
>> +};
>> +
>> +&i2c0 {
>> +	pinctrl-0 = <&i2c0_pins>;
>> +	pinctrl-names = "default";
>> +	status = "okay";
>> +
>> +	ds3232: rtc@68 {
>> +		compatible = "dallas,ds3232";
>> +		reg = <0x68>;
>> +	};
>> +
>> +	eeprom0: eeprom@50 {
>> +		compatible = "atmel,24c02";		/* actually it's a 24AA02E48 */
>> +		pagesize = <16>;
>> +		read-only;
>> +		reg = <0x50>;
>> +		vcc-supply = <&reg_vcc3v3>;
>> +
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +
>> +		eth0_macaddress: macaddress@fa {
>> +			reg = <0xfa 0x06>;
>> +		};
>> +	};
>> +
>> +	tusb320: typec@60 {
>> +		compatible = "ti,tusb320";
>> +		reg = <0x60>;
>> +		interrupt-parent = <&pio>;
>> +		interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
>> +	};
>> +};
>> +
>> +&emac {
>> +	allwinner,leds-active-low;
>> +	nvmem-cells = <&eth0_macaddress>;		/* custom nvmem reference */
>> +	nvmem-cell-names = "mac-address";		/* see ethernet-controller.yaml 
>> */
>> +	status = "okay";
>> +};
>> +
>> +&spi0 {
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&spi0_pins>;
> 
> Those two lines look redundant, as they are already specified in the
> .dtsi file.
> 

Ah yes, seems I have missed this. I will remove them.

>> +	cs-gpios = <0>, <&pio 1 0 GPIO_ACTIVE_LOW>; /* PB0 */
>> +	status = "okay";
>> +
>> +	flash@0 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		compatible = "jedec,spi-nor";
> 
> I think traditionally we have the compatible first, and #a-c and #s-c
> last in the node.
> And do you have anything partitioned in there? If not, you wouldn't
> need the #a-c and #s-c properties, I think.
> 

I provide the partitions via mtdparts from U-Boot so that they can
be modified without updating the device tree. Do I still require them
then? As for the ordering, I had a look in other devices trees and just
copied the section.

>> +		reg = <0>;
>> +		label = "firmware";
>> +		spi-max-frequency = <40000000>;
>> +	};
>> +
>> +	can@1 {
>> +		compatible = "microchip,mcp2518fd";
>> +		reg = <1>;
>> +		clocks = <&clk_can0>;
>> +		interrupt-parent = <&pio>;
>> +		interrupts = <1 1 IRQ_TYPE_LEVEL_LOW>;  /* PB1 */
>> +		spi-max-frequency = <20000000>;
>> +		vdd-supply = <&reg_vcc3v3>;
>> +		xceiver-supply = <&reg_vcc3v3>;
>> +	};
>> +};
>> \ No newline at end of file
> 
> Please add a newline at the end.
> 
>> diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi 
>> b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
> 
> I don't know for sure if people want SoC .dtsi patches separately?
> 

I will move it into another patch just to be safe.

> Cheers,
> Andre
> 
>> index 9e13c2aa8911..f909b1d4dbca 100644
>> --- a/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s.dtsi
>> @@ -416,6 +416,12 @@ uart0_pb_pins: uart0-pb-pins {
>>  				function = "uart0";
>>  			};
>> 
>> +			/omit-if-no-ref/
>> +			uart1_pe_pins: uart1-pe-pins {
>> +				pins = "PE21", "PE22";
>> +				function = "uart1";
>> +			};
>> +
>>  			uart2_pins: uart2-pins {
>>  				pins = "PB0", "PB1";
>>  				function = "uart2";