[PATCH v2 06/12] rockchip: board: qnap-ts433: Support multiple models of the TSx33 family

[Quentin Schulz]

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 <heiko at sntech.de>
> ---
>   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 <heiko at sntech.de>
> +#
> +# 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 <heiko at sntech.de>
> + */
> +
> +#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