Hi Heiko,
On 1/14/26 9:28 AM, Heiko Stuebner wrote:
The RK3568-based NAS systems share a common basic structure and mainly
only differ in the number of available hard-drive slots.
They also contain EEPROMs on their mainboard and backplane, that contain
identifying information and therefore make it possible to determine the
model at runtime.
There are 3 different formats for identifiers found on TSx33 models in
the wild, which can only be distinguised by the position of dashes ("-")
s/distinguised/distinguished/
in them, so the code determines which format it is, and uses a dedicated
decoder function to get the relevant information.
Add that EEPROM reading and model association, to set the correct
devicetree filename for the OS that gets booted. This allows to just
set fdtdir in extlinux and have u-boot then load the correct devicetree.
For u-boot itself, all models are similar enough up to the emmc that
in theory we wouldn't necessarily need to diversify here, especially as
reading the eeproms gets tricky at the stage where embedded_dtb_select
would run. But especially the still to introduce TS133 does have a
quite different USB setup, so definitly will need its own DTB.
s/definitly/definitely/
So for now, we'll still need separate configs per model, but can share
the board-code. I do hope to consolidate this further in the future.
Signed-off-by: Heiko Stuebner <[email protected]>
---
board/qnap/ts433_rk3568/Makefile | 7 +
board/qnap/ts433_rk3568/board.c | 386 ++++++++++++++++++++++++++++
configs/qnap-ts433-rk3568_defconfig | 2 +
3 files changed, 395 insertions(+)
create mode 100644 board/qnap/ts433_rk3568/Makefile
create mode 100644 board/qnap/ts433_rk3568/board.c
diff --git a/board/qnap/ts433_rk3568/Makefile b/board/qnap/ts433_rk3568/Makefile
new file mode 100644
index 00000000000..d87f916d00b
--- /dev/null
+++ b/board/qnap/ts433_rk3568/Makefile
@@ -0,0 +1,7 @@
+#
+# (C) Copyright 2026 Heiko Stuebner <[email protected]>
+#
+# SPDX-License-Identifier: GPL-2.0+
+#
+
+obj-y += board.o
Only build for non-SPL...
"""
ifneq ($(CONFIG_XPL_BUILD),y)
obj-y += board.o
endif
"""
diff --git a/board/qnap/ts433_rk3568/board.c b/board/qnap/ts433_rk3568/board.c
new file mode 100644
index 00000000000..006add6fb30
--- /dev/null
+++ b/board/qnap/ts433_rk3568/board.c
@@ -0,0 +1,386 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2026 Heiko Stuebner <[email protected]>
+ */
+
+#include <dm.h>
+#include <env.h>
+#include <i2c_eeprom.h>
+#include <init.h>
+#include <net.h>
+#include <netdev.h>
+#include <vsprintf.h>
+
+#ifndef CONFIG_XPL_BUILD
+
... to avoid this big ifdef.
+DECLARE_GLOBAL_DATA_PTR;
+
+#define DTB_DIR "rockchip/"
+
+struct tsx33_model {
Please document members (bp meaning wasn't so obvious, had to read a bit
more to get that this was meant as backplane).
+ const char *mb;
+ const u8 mb_pcb;
+ const char *bp;
+ const u8 bp_pcb;
+ const char *name;
+ const char *fdtfile;
+};
+
+/*
+ * gd->board_type is unsigned long, so start at 1 for actual found types
Don't understand why that justifies starting at 1?
+ * The numeric PCB ids should be matched against the highest available
+ * one for best feature matching. For example Q0AI0_13 gained per-disk
+ * gpios for presence detection and power-control. Similar changes
+ * for the other boards.
+ * So the list should be sorted higest to lowest pcb-id.
s/higest/highest/
+ */
+enum tsx33_device_id {
+ UNKNOWN_TSX33 = 0,
+ Q0AI0_13,
+ Q0AI0_11,
+ Q0AJ0_Q0AM0_12_11,
+ Q0AJ0_Q0AM0_11_10,
+ Q0B20_Q0AW0_12_10,
+ Q0B20_Q0B30_12_10,
+ Q0B20_Q0B30_10_10,
+ QA0110_10,
+ TSX33_MODELS,
Because you start at 1, this means this represents n+1 supported models.
+};
+
+/*
+ * All TSx33 devices consist of a mainboard and possible backplane.
+ * Each board has a model identifier and a pcb code written to an eeprom
+ * on it. Later board revisions got per-harddrive presence detection
+ * and power-supply switches, so might need an overlay applied later on
+ * to support those.
Maybe make the last sentence a TODO: ?
+ */
+static const struct tsx33_model tsx33_models[TSX33_MODELS] = {
+ [UNKNOWN_TSX33] = {
+ "UNKNOWN",
+ 0,
+ NULL,
+ 0,
+ "Unknown TSx33",
+ NULL,
+ },
+ [Q0AI0_13] = {
+ "Q0AI0",
+ 13,
+ NULL,
+ 0,
+ "TS133",
+ NULL, /* not yet supported */
+ },
+ [Q0AI0_11] = {
+ "Q0AI0",
+ 11,
+ NULL,
+ 0,
+ "TS133",
+ NULL, /* not yet supported */
+ },
+ [Q0AJ0_Q0AM0_12_11] = {
+ "Q0AJ0",
+ 12,
+ "Q0AM0",
+ 11,
+ "TS233",
+ NULL, /* not yet supported */
+ },
+ [Q0AJ0_Q0AM0_11_10] = {
+ "Q0AJ0",
+ 11,
+ "Q0AM0",
+ 10,
+ "TS233",
+ NULL, /* not yet supported */
+ },
+ [Q0B20_Q0AW0_12_10] = {
+ "Q0B20",
+ 12,
+ "Q0AW0",
+ 10,
+ "TS216G",
+ NULL, /* not yet supported */
+ },
+ [Q0B20_Q0B30_12_10] = {
+ "Q0B20",
+ 12,
+ "Q0B30",
+ 10,
+ "TS433",
+ DTB_DIR "rk3568-qnap-ts433.dtb",
+ },
+ [Q0B20_Q0B30_10_10] = {
+ "Q0B20",
+ 10,
+ "Q0B30",
+ 10,
+ "TS433",
+ DTB_DIR "rk3568-qnap-ts433.dtb",
+ },
+ [QA0110_10] = {
+ /*
+ * The board ident in the original firmware is longer here.
+ * Likely the field order is different in the ident string.
+ * So this needs an EEPROM dump to figure out.
+ */
+ "QA0110",
+ 11,
+ NULL,
+ 0,
+ "TS433eU",
+ NULL, /* not yet supported */
+ },
+};
+
+enum tsx33_product_id_format {
+ LEN_22 = 0,
+ LEN_21,
+ LEN_12, /* has the least amound of dashes in the string, so last */
s/amound/amount/
+ TSX33_PRODUCT_FORMATS,
+};
+
+/*
+ * We (only) identify the serial format via the different position of dashes.
+ * Because we read the product string from an eeprom there is no
there is no?
+ */
+struct tsx33_product_format {
+ int *dashes;
+ int num_dashes;
+ int (*decoder)(char *product, char *board, int *pcb);
+};
+
+/*
+ * The longest board-name length known.
+ * Buffers using this need to be MAX-LEN + 1 for the final 0-termination.
s/MAX-LEN/MAX_BOARD_LEN/
But also, why? It seems the other _LEN variables do take this into
account and you have - 1 wherever applicable, some consistency would be
nice?
+ */
+#define PRODUCT_MAX_BOARD_LEN 6
+
+#define LEN22_PRODUCT_BOARD_OFF 6
+#define LEN22_PRODUCT_BOARD_OFF2 12
+#define LEN22_PRODUCT_BOARD_LEN 6
+#define LEN22_PRODUCT_PCB_OFF 16
+#define LEN22_PRODUCT_PCB_LEN 2
+#define LEN22_PRODUCT_BOM_OFF 14
+#define LEN22_PRODUCT_BOM_LEN 1
+
+static int tsx33_decode_len22(char *product, char *board, int *pcb)
+{
Please document this.
"We get this as input, we return this as output" such that we know
what's supposed to happen without deciphering the implementation?
Ditto for other decode functions.
+ strncpy(board, product + LEN22_PRODUCT_BOARD_OFF,
+ LEN22_PRODUCT_BOARD_LEN - 1);
+ board[LEN22_PRODUCT_BOARD_LEN - 1] = product[LEN22_PRODUCT_BOARD_OFF2];
+
+ /* add an artificial end to the PCB part for strtol */
+ product[LEN22_PRODUCT_PCB_OFF + LEN22_PRODUCT_PCB_LEN] = '\0';
+ *pcb = (int)simple_strtol(product + LEN22_PRODUCT_PCB_OFF, NULL, 0);
+
Are you sure you want to auto-detect the base? If the string starts with
0 it'll be understood as octal. I think you want 10 as base?
+ return 0;
+}
+
+#define LEN21_PRODUCT_BOARD_OFF 6
+#define LEN21_PRODUCT_BOARD_OFF2 11
+#define LEN21_PRODUCT_BOARD_LEN 5
+#define LEN21_PRODUCT_PCB_OFF 15
+#define LEN21_PRODUCT_PCB_LEN 2
+#define LEN21_PRODUCT_BOM_OFF 13
+#define LEN21_PRODUCT_BOM_LEN 1
+
+static int tsx33_decode_len21(char *product, char *board, int *pcb)
+{
+ strncpy(board, product + LEN21_PRODUCT_BOARD_OFF,
+ LEN21_PRODUCT_BOARD_LEN - 1);
+ board[LEN21_PRODUCT_BOARD_LEN - 1] = product[LEN21_PRODUCT_BOARD_OFF2];
+
+ /* add an artificial end to the PCB part for strtol */
+ product[LEN21_PRODUCT_PCB_OFF + LEN21_PRODUCT_PCB_LEN] = '\0';
+ *pcb = (int)simple_strtol(product + LEN21_PRODUCT_PCB_OFF, NULL, 0);
+
+ return 0;
+}
+
+#define LEN12_PRODUCT_BOARD_OFF 4
+#define LEN12_PRODUCT_BOARD_LEN 5
+#define LEN12_PRODUCT_PCB_OFF 9
+#define LEN12_PRODUCT_PCB_LEN 2
+#define LEN12_PRODUCT_BOM_OFF 11
+#define LEN12_PRODUCT_BOM_LEN 1
+
+static int tsx33_decode_len12(char *product, char *board, int *pcb)
+{
+ strncpy(board, product + LEN12_PRODUCT_BOARD_OFF,
+ LEN12_PRODUCT_BOARD_LEN);
+
+ /* add an artificial end to the PCB part for strtol */
+ product[LEN12_PRODUCT_PCB_OFF + LEN12_PRODUCT_PCB_LEN] = '\0';
+ *pcb = (int)simple_strtol(product + LEN12_PRODUCT_PCB_OFF, NULL, 0);
+
+ return 0;
+}
The logic seem to be awfully similar, would it make sense to have a
single function with offsets passed as parameters?
+
+const struct tsx33_product_format product_formats[TSX33_PRODUCT_FORMATS] = {
Can be static I assume?
+ [LEN_22] = {
+ (int[]){ 2, 11, 15 },
+ 3,
If you'd declared an array, you could simply use ARRAY_SIZE(array) and
never have to worry about them being unsynced.
+ tsx33_decode_len22,
+ },
+ [LEN_21] = {
+ (int[]){ 2, 10, 14 },
+ 3,
+ tsx33_decode_len21,
+ },
+ [LEN_12] = {
+ (int[]){ 2 },
+ 1,
+ tsx33_decode_len12,
+ },
+};
+
+static int tsx33_decode_product(char *product, char *board, int *pcb)
+{
+ const struct tsx33_product_format *format;
+ int i, j;
+
+ /* Find the correct serial variant */
+ for (i = 0; i < TSX33_PRODUCT_FORMATS; i++) {
ARRAY_SIZE(product_formats)?
+ format = &product_formats[i];
+
+ for (j = 0; j < format->num_dashes; j++) {
+ int dash_char = format->dashes[j];
+
+ if (product[dash_char] != '-')
+ goto next_format;
+ }
You can use strstr() instead of looping on all characters manually.
Cheers,
Quentin
