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

[Lukas Schmid <lukas.schmid@netcube.li>]

[-- Attachment #1: Type: text/plain, Size: 15257 bytes --]

On Thursday, January 2, 2025 3:31:16 PM CET Andre Przywara wrote:
> On Thu,  2 Jan 2025 12:18:59 +0000
> Lukas Schmid <lukas.schmid@netcube.li> wrote:
> 
> Hi Lukas,
> 
> thanks for taking care and sending a patch!
> 
> I guess you want the DT merged into the official repositories? Then you
> would need to send the patch to the Linux kernel mailing lists, having the
> sunxi maintainers in To:, and the lists in Cc:.
> $ scripts/get_maintainer.pl 0001-name.patch
> would give you the exact list.
> 
> Also you would need to add the vendor name and the board name to the
> bindings, see patches 12 and 13 for an example here:
> https://lore.kernel.org/linux-sunxi/20241111013033.22793-13-andre.przywara@a
> rm.com/T/#u
> 
> So this would make this patch a 3-patch series (vendor prefix, board name,
> dts file).
> 
> > 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   | 269 ++++++++++++++++++
> >  2 files changed, 271 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..1793a9b7f796
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-netcube-kumquat.dts
> > @@ -0,0 +1,269 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> DT files should have a dual license header:
>         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";
> > +     };
> > +
> > +     cpus {
> > +             cpu0: cpu@0 {
> > +                     clock-frequency = <1200000000>;
> > +             };
> > +     };
> 
> Please remove this node. I do understand that the kernel complains about
> the missing property, but this warning is severely outdated (in the age of
> DVFS) and should be removed instead.
> 
> > +
> > +     leds {
> > +             compatible = "gpio-leds";
> > +
> > +             heartbeat_led {
> > +                     gpios = <&pio 4 4 GPIO_ACTIVE_HIGH>; /* PE4 */
> > +                     linux,default-trigger = "heartbeat";
> > +                     function = LED_FUNCTION_HEARTBEAT;
> > +                     color = <LED_COLOR_ID_GREEN>;
> > +             };
> > +
> > +             mmc0_act_led {
> > +                     gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
> > +                     linux,default-trigger = "mmc0";
> > +                     function = LED_FUNCTION_DISK;
> > +                     color = <LED_COLOR_ID_GREEN>;
> > +             };
> > +     };
> > +
> > +     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 */ +             };
> > +     };
> > +
> > +     reg_vcc5v0: vcc5v0 {
> 
> Can you add a comment here where this voltage comes from? I guess it's the
> primary board power supply, via some USB or barrel plug? Just add a
> one-liner comment here to make it clear this is the root of the power tree.
> 
> > +             compatible = "regulator-fixed";
> > +             regulator-name = "vcc5v0";
> > +             regulator-min-microvolt = <5000000>;
> > +             regulator-max-microvolt = <5000000>;
> > +     };
> > +
> > +     reg_vcc3v3: vcc3v3 {
> 
> What kind of regulator is this? Some discrete linear or buck regulator?
> Can you add a one-line comment stating the type, and maybe the model?
> 
> Those always-on fixed regulators are sometimes sketchy, so we would like
> to know why they cannot be controlled.
> 
> > +             compatible = "regulator-fixed";
> > +             regulator-name = "vcc3v3";
> > +             regulator-min-microvolt = <3300000>;
> > +             regulator-max-microvolt = <3300000>;
> > +             vin-supply = <&reg_vcc5v0>;
> > +     };
> > +
> > +     reg_vcc3v0: vcc3v0 {
> 
> Same here, please give people an idea what kind of regulator this is.
> 
> > +             compatible = "regulator-fixed";
> > +             regulator-name = "vcc3v0";
> > +             regulator-min-microvolt = <3000000>;
> > +             regulator-max-microvolt = <3000000>;
> > +             vin-supply = <&reg_vcc3v3>;
> > +     };
> > +
> > +     can0_osc: can0_osc {
> 
> Is that a crystal oscillator on the board? Can you state that in a brief
> comment, please?
> 
> > +             compatible = "fixed-clock";
> > +             #clock-cells = <0>;
> > +             clock-frequency  = <40000000>;
> > +     };
> > +};
> > +
> > +&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 {
> > +     /delete-property/ phys;
> > +     /delete-property/ phy-names;
> 
> Why is this?
> 
> > +     status = "okay";
> > +};
> > +
> > +&ohci {
> > +     /delete-property/ phys;
> > +     /delete-property/ phy-names;
> > +     status = "okay";
> > +};
> > +
> > +&lradc {
> > +     vref-supply = <&reg_vcc3v0>;
> > +     status = "disabled";
> 
> Why would you specify the supply voltage, but then keep it disabled?
> Either it's not working or not usable, then you wouldn't need to do
> anything (since the status is already "disabled", so that's definitely
> redundant), or it's good, then you should say '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_pins>;
> > +     pinctrl-names = "default";
> > +     status = "okay";
> > +};
> > +
> > +&i2c0 {
> > +     pinctrl-0 = <&i2c0_pins>;
> > +     pinctrl-names = "default";
> > +     status = "okay";
> > +
> > +     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: eth0_macaddress@FA {
> > +                     reg = <0xFA 0x06>;
> > +             };
> > +     };
> > +
> > +     tusb320: tusb320@60 {
> > +             compatible = "ti,tusb320";
> > +             reg = <0x60>;
> > +             interrupt-parent = <&pio>;
> > +             interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
> > +     };
> > +
> > +     ds3232: rtc@68 {
> > +             compatible = "dallas,ds3232";
> > +             reg = <0x68>;
> > +     };
> > +};
> > +
> > +&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>;
> > +             reg = <0>;
> > +             compatible = "jedec,spi-nor";
> > +             label = "firmware";
> > +             spi-max-frequency = <40000000>;
> > +     };
> > +
> > +     can@1 {
> > +        compatible = "microchip,mcp2518fd";
> > +        reg = <1>;
> 
> The indentation is not right for the two lines above.
> 
> > +             clocks = <&can0_osc>;
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&can0_pins>;
> 
> What is this pin for? If I get this correctly, this is a SPI to CAN
> bridge, right? Why would it require a GPIO on the host SoC?
> (see below)
> 
> > +             spi-max-frequency = <20000000>;
> > +             interrupt-parent = <&pio>;
> > +             interrupts = <1 1 IRQ_TYPE_LEVEL_LOW>;
> 
> Ah, I guess it's for the interrupt? In this case I think you do not
> declare this via pinctrl, as the PIO interrupt controller should reserve
> this pin, configuring it with the EINT pinmux.
> 
> And please add a comment with the pin name in it (PB1).
> 
> > +             vdd-supply = <&reg_vcc3v3>;
> > +             xceiver-supply = <&reg_vcc3v3>;
> > +     };
> > +};
> > +
> > +&rtc {
> > +     status = "disabled";
> 
> I don't think you can do that, since we rely on some clocks provided by
> the RTC device. I guess you want to say that the kernel should use the I2C
> RTC instead of the built-in one, but this should already be covered by the
> rtc0 alias above?
> 
> > +};
> > +
> > +&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", "USB_ID", "USBC_nINT",
> > "I2C0_SCL", "I2C0_SDA", "UART0_TX", "UART0_RX", "", "", "", "", "", "",
> > "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", //PB +   
> >                                          "SPI_MISO", "SPI_SCK",
> > "FLASH_nCS", "SPI_MOSI", "", "", "", "", "", "", "", "", "", "", "", "",
> > "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", //PC +   
> >                                          "", "", "", "", "", "", "", "",
> > "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
> > "", "", "", "", "", "", //PD +                                           
> >  "Q12", "Q11", "Q10", "Q9", "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", "", "", "", "", "",
> > "", "", //PE +                                             "SD_D1",
> > "SD_D0", "SD_CLK", "SD_CMD", "SD_D3", "SD_D2", "LED_SYS1", "", "", "",
> > "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
> > "", "", "", "", //PF +                                            
> > "ESP_CLK", "ESP_CMD", "ESP_D0", "ESP_D1", "ESP_D2", "ESP_D3", "", "", "",
> > "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "",
> > "", "", "", "", ""; //PG +
> > +     uart0_pins: uart0-pins {
> > +             pins = "PB8", "PB9";
> > +             function = "uart0";
> > +     };
> 
> Why would you need that node? There is already uart0_pb_pins in the .dtsi.
> 
> > +
> > +     uart1_pins: uart1-pins {
> > +             pins = "PE21", "PE22";
> > +             function = "uart1";
> > +     };
> 
> Please specify those pinmux in the .dtsi file, as they can be shared
> across boards. It's typically the first user adding them to the .dtsi. For
> rare pins, we add a /omit-if-no-ref/ tag.
> 
> > +
> > +     spi0_pins: spi0-pins {
> > +             pins = "PC0", "PC1", "PC2", "PC3";
> > +             function = "spi0";
> > +     };
> 
> Same redundancy again, it's already in the .dtsi.
> 
> > +
> > +     can0_pins: can0-pins {
> > +             pins = "PB1";
> > +             function = "gpio_in";
> > +     };
> 
> As mentioned above, this wouldn't be needed, and in particular gpio_in
> looks wrong. It would need to be the EINT pinmux (0x6), but the PIO IRQ
> controller code takes care of that already.

Hi,

I had just tried the can-bus on my board again. It is not getting any messages 
with this removed. I do not get any interrupts according to /proc/interrupts 
now.

Is this an issue with the pio or do I just add the pinctrl back?

> 
> Cheers,
> Andre
> 
> > +};
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]