On Mon, Sep 28, 2026 at 12:13:22PM +0200, Vincent Jardin wrote:
> The SD1 lane configuration can support many cases, not just the
> 1x100GE + 4x25G split.
>
> Let's describe each one as an entry of a table.
> Then, let's check the serdes1:<mode> of hwconfig.
>
> The master-lane lookup, the halt and the reset of the split become
> helpers.
>
> Suggested-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Vincent Jardin <[email protected]>
>
> ---
Still hard to review due to unrelated changes mixed here.
>
> Changes in v4:
> - New patch
>
> board/nxp/lx2160a/serdes_split.c | 272 +++++++++++++++++++++----------
> board/nxp/lx2160a/serdes_split.h | 16 +-
> 2 files changed, 192 insertions(+), 96 deletions(-)
>
> diff --git a/board/nxp/lx2160a/serdes_split.c
> b/board/nxp/lx2160a/serdes_split.c
> index 1a5c44e7740..11bec35fd22 100644
> --- a/board/nxp/lx2160a/serdes_split.c
> +++ b/board/nxp/lx2160a/serdes_split.c
> @@ -12,6 +12,7 @@
> #include <asm/arch/fsl_serdes.h>
> #include <asm/arch/immap_lsch3.h>
> #include <asm/arch/soc.h> /* gur_in32() picks the GUR endianness
> */
> +#include <linux/bitmap.h>
> #include <linux/kernel.h>
> #include <linux/string.h>
> #include <linux/delay.h>
> @@ -245,16 +246,22 @@ static void print_rcw_view(int sd)
>
> #if defined(CONFIG_ARCH_LX2160A)
>
> -/*
> - * The split itself.
> - *
> - * This is NXP's e100g1_split.rcw sequence, moved from the PBI into
> - * U-Boot.
> - */
> -#define SPLIT_FIRST_LANE 4 /* LNE */
> -#define SPLIT_LAST_LANE 7 /* LNH */
> +struct sd1_mode {
> + const char *name; /* hwconfig serdes1:<name> */
> + const char *desc;
> + const char *from;
> + bool (*is_active)(unsigned long base);
> + int (*enter)(unsigned long base);
> +};
> +
> +#define SD1_LNE_LNH GENMASK(7, 4)
> +
> +#define SRDS_POLL_US 10000 /* NXP waits ~100 cycles */
>
> /*
> + * 1x 100GE + 4x 25G. This is NXP's e100g1_split.rcw sequence, adapted
> + * from the PBI sequence to U-Boot.
> + *
> * PROTO_SEL=11010b (25G/50G/100G, shared) + IF_WIDTH=100b (40-bit),
> * PORT_RST_LEFT=0 and PORT_LN0_B=0 so every lane becomes its own
> * single-lane port. Only those last two bits actually change: the
> @@ -264,31 +271,24 @@ static void print_rcw_view(int sd)
> #define SPLIT_PCCE 0x00000100 /* E100Ga off, E100Gb on */
> #define SPLIT_PCCD 0x11110000 /* E25Ga..E25Gd on */
>
> -#define SPLIT_POLL_US 10000 /* NXP waits ~100
> cycles */
> -
> -static bool srds1_split_state(unsigned long base)
> +static bool sd1_is_2x100g(unsigned long base)
> {
> u32 pccd = in_le32((void __iomem *)SRDS_PCCD(base));
> u32 pcce = in_le32((void __iomem *)SRDS_PCCE(base));
>
> - return PCCE_E100GA_CFG(pcce) == 0 &&
> - PCCD_E25G_CFG(pccd, 0) && PCCD_E25G_CFG(pccd, 1) &&
> - PCCD_E25G_CFG(pccd, 2) && PCCD_E25G_CFG(pccd, 3);
> -}
> -
> -bool lx2160a_serdes1_is_split(void)
> -{
> - return srds1_split_state(srds_base(FSL_SRDS_1));
> + return PCCE_E100GA_CFG(pcce) != 0 && PCCE_E100GB_CFG(pcce) != 0 &&
> + PCCD_E25G_CFG(pccd, 0) == 0 && PCCD_E25G_CFG(pccd, 1) == 0 &&
> + PCCD_E25G_CFG(pccd, 2) == 0 && PCCD_E25G_CFG(pccd, 3) == 0;
> }
>
> -static bool srds1_is_unsplit(unsigned long base)
> +static bool sd1_is_100g4x25g(unsigned long base)
> {
> u32 pccd = in_le32((void __iomem *)SRDS_PCCD(base));
> u32 pcce = in_le32((void __iomem *)SRDS_PCCE(base));
>
> - return PCCE_E100GA_CFG(pcce) != 0 && PCCE_E100GB_CFG(pcce) != 0 &&
> - PCCD_E25G_CFG(pccd, 0) == 0 && PCCD_E25G_CFG(pccd, 1) == 0 &&
> - PCCD_E25G_CFG(pccd, 2) == 0 && PCCD_E25G_CFG(pccd, 3) == 0;
> + return PCCE_E100GA_CFG(pcce) == 0 &&
> + PCCD_E25G_CFG(pccd, 0) && PCCD_E25G_CFG(pccd, 1) &&
> + PCCD_E25G_CFG(pccd, 2) && PCCD_E25G_CFG(pccd, 3);
It is not immediately obvious that the old 'split' configuration
corresponds to '100g4x25g' and 'unsplit' to '2x100g'. Better to
explicitly say this, so that reviewers can follow along.
And then, the diff looks kind of bad here, because you reverse the order
of the is_active() expressions. In the old code, 'split' was first then
'unsplit'. In the new code, '2x100g' comes first (aka unsplit), followed
by '100g4x25g' (aka split). So it looks as if the logic is changing,
plus there are gratuitous diff lines in the commit.
Not easy to review at all, and not obviously correct.
> }
>
> /*
> @@ -298,7 +298,7 @@ static bool srds1_is_unsplit(unsigned long base)
> */
> static int poll_clear(unsigned long reg, u32 bit)
> {
> - int us = SPLIT_POLL_US;
> + int us = SRDS_POLL_US;
Renaming SPLIT_POLL_US to SRDS_POLL_US is noise which can be isolated to
its own change.
>
> while (us--) {
> if (!(in_le32((void __iomem *)reg) & bit))
> @@ -309,79 +309,82 @@ static int poll_clear(unsigned long reg, u32 bit)
> return -ETIMEDOUT;
> }
>
> -int lx2160a_serdes1_split(void)
> +static int sd1_port_master(unsigned long base, unsigned long lanes)
> {
> - unsigned long base = srds_base(FSL_SRDS_1);
> - int m, master = -1, ret = 0;
> -
> - if (srds1_split_state(base)) {
> - debug("lx2160a serdes: SD1 already split, nothing to do\n");
> - return 0;
> - }
> + int m, master = -1;
>
> - if (!srds1_is_unsplit(base)) {
> - printf("lx2160a serdes: SD1 is in neither the split nor the
> unsplit state (PCCD=%08x PCCE=%08x), refusing\n",
> - in_le32((void __iomem *)SRDS_PCCD(base)),
> - in_le32((void __iomem *)SRDS_PCCE(base)));
> - return -EINVAL;
> - }
> -
> - /*
> - * Halt. "Halt only has meaning on the master source
> - * clock lane of a port (LNmGCR0[PORT_LN0_B]=0)". Lanes E-H are one
> - * 100G port at this point, so exactly one of them is the master and
> - * the other three requests are ignored. Issue all four anyway
> - * (that is what NXP's sequence does) but only wait on the lane
> - * where the bit can actually clear.
> - */
> - for (m = SPLIT_FIRST_LANE; m <= SPLIT_LAST_LANE; m++) {
> + for_each_set_bit(m, &lanes, SRDS_LANES)
> if (!(in_le32((void __iomem *)SRDS_LNmGCR0(base, m)) &
> GCR0_PORT_LN0_B))
> master = m;
> +
> + if (master < 0)
> + printf("lx2160a serdes: no master lane among LN%c-LN%c,
> refusing\n",
> + 'A' + (int)__ffs(lanes), 'A' + (int)__fls(lanes));
> +
> + return master;
> +}
> +
> +/*
> + * Halt the port made of @lanes.
> + *
> + * "Halt only has meaning on the master source clock lane of a port
> + * (LNmGCR0[PORT_LN0_B]=0)", so the request goes to every lane, as
> + * NXP's sequence does, and only the master is waited on:
> + * it is the one lane where the bit can clear.
> + *
> + * -EINVAL means nothing was touched
> + * -ETIMEDOUT is only a warning
> + */
> +static int sd1_halt_port(unsigned long base, unsigned long lanes)
Can you create sd1_halt_port() as a helper in a preparatory change?
> +{
> + int m, master = sd1_port_master(base, lanes);
> +
> + if (master < 0)
> + return -EINVAL;
> +
> + for_each_set_bit(m, &lanes, SRDS_LANES) {
> out_le32((void __iomem *)SRDS_LNmTRSTCTL(base, m),
> RSTCTL_HLT_REQ);
> out_le32((void __iomem *)SRDS_LNmRRSTCTL(base, m),
> RSTCTL_HLT_REQ);
> }
>
> - if (master < 0) {
> - printf("lx2160a serdes: no master lane among LNE-LNH,
> refusing\n");
> - return -EINVAL;
> - }
> -
> if (poll_clear(SRDS_LNmTRSTCTL(base, master), RSTCTL_HLT_REQ) ||
> poll_clear(SRDS_LNmRRSTCTL(base, master), RSTCTL_HLT_REQ)) {
> printf("lx2160a serdes: LN%c did not halt within %d us,
> continuing\n",
> - 'A' + master, SPLIT_POLL_US);
> - ret = -ETIMEDOUT;
> + 'A' + master, SRDS_POLL_US);
> + return -ETIMEDOUT;
> }
>
> - /*
> - * Reconfigure while halted: four one-lane ports instead of one
> - * four-lane port. Everything else about the lane is unchanged.
> - */
> - for (m = SPLIT_FIRST_LANE; m <= SPLIT_LAST_LANE; m++)
> - out_le32((void __iomem *)SRDS_LNmGCR0(base, m), SPLIT_GCR0);
> + return 0;
> +}
>
> - /* Re-point the protocol converters. */
> - out_le32((void __iomem *)SRDS_PCCE(base), SPLIT_PCCE);
> - out_le32((void __iomem *)SRDS_PCCD(base), SPLIT_PCCD);
> +static void sd1_reset_req(unsigned long base, unsigned long lanes, int skip)
Nitpick: the skip logic is not needed as of this patch.
> +{
> + int m;
>
> - /* Reset. Every lane is its own master now, so all four count. */
> - for (m = SPLIT_FIRST_LANE; m <= SPLIT_LAST_LANE; m++) {
> + for_each_set_bit(m, &lanes, SRDS_LANES) {
> + if (m == skip)
> + continue;
> out_le32((void __iomem *)SRDS_LNmTRSTCTL(base, m),
> RSTCTL_RST_REQ);
> out_le32((void __iomem *)SRDS_LNmRRSTCTL(base, m),
> RSTCTL_RST_REQ);
> }
> +}
> +
> +static int sd1_reset_wait(unsigned long base, unsigned long lanes)
> +{
> + int m, ret = 0;
>
> - for (m = SPLIT_FIRST_LANE; m <= SPLIT_LAST_LANE; m++) {
> + for_each_set_bit(m, &lanes, SRDS_LANES) {
> u32 t, r;
>
> if (poll_clear(SRDS_LNmTRSTCTL(base, m), RSTCTL_RST_REQ) ||
> poll_clear(SRDS_LNmRRSTCTL(base, m), RSTCTL_RST_REQ)) {
> printf("lx2160a serdes: LN%c reset did not complete
> within %d us\n",
> - 'A' + m, SPLIT_POLL_US);
> + 'A' + m, SRDS_POLL_US);
> ret = -ETIMEDOUT;
> continue;
> }
> @@ -395,33 +398,138 @@ int lx2160a_serdes1_split(void)
> }
> }
>
> - if (!srds1_split_state(base)) {
> - printf("lx2160a serdes: split did not take (PCCD=%08x
> PCCE=%08x)\n",
> - in_le32((void __iomem *)SRDS_PCCD(base)),
> - in_le32((void __iomem *)SRDS_PCCE(base)));
> - return -EIO;
> + return ret;
> +}
> +
> +static int sd1_enter_100g4x25g(unsigned long base)
> +{
> + unsigned long lanes = SD1_LNE_LNH;
> + int m, ret, err;
> +
> + ret = sd1_halt_port(base, lanes);
> + if (ret == -EINVAL)
> + return ret;
> +
> + /*
> + * Reconfigure while halted: four 1-lane ports instead of one
> + * 4-lane port. Everything else about the lane is unchanged.
> + */
> + for_each_set_bit(m, &lanes, SRDS_LANES)
> + out_le32((void __iomem *)SRDS_LNmGCR0(base, m), SPLIT_GCR0);
> +
> + /* Re-point the protocol converters. */
> + out_le32((void __iomem *)SRDS_PCCE(base), SPLIT_PCCE);
> + out_le32((void __iomem *)SRDS_PCCD(base), SPLIT_PCCD);
> +
> + /* Reset. Every lane is its own master now, so all four count. */
> + sd1_reset_req(base, lanes, -1);
> + err = sd1_reset_wait(base, lanes);
> +
> + return err ? err : ret;
> +}
> +
> +static const struct sd1_mode sd1_modes[] = {
> + {
> + .name = "2x100g",
> + .desc = "2x 100GE",
> + .is_active = sd1_is_2x100g,
> + },
> + {
> + .name = "100g4x25g",
> + .desc = "1x 100GE + 4x 25G",
> + .from = "2x100g",
> + .is_active = sd1_is_100g4x25g,
> + .enter = sd1_enter_100g4x25g,
> + },
> +};
> +
> +static const struct sd1_mode *sd1_mode_find(const char *name)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(sd1_modes); i++)
> + if (!strcmp(sd1_modes[i].name, name))
> + return &sd1_modes[i];
> +
> + return NULL;
> +}
> +
> +static void sd1_print_pcc(unsigned long base)
> +{
> + printf("PCCC=%08x PCCD=%08x PCCE=%08x",
> + in_le32((void __iomem *)SRDS_PCCC(base)),
> + in_le32((void __iomem *)SRDS_PCCD(base)),
> + in_le32((void __iomem *)SRDS_PCCE(base)));
> +}
> +
> +/* Put SD1 in @mode, for serdes1:@target */
> +static int sd1_enter(const struct sd1_mode *mode,
> + const struct sd1_mode *target, int depth)
This recursive sd1_enter() also deserves its own change. It's a bit
strange to pack it together with a change titled "set SD1 with a table",
when it permits the code to do something which it couldn't do before:
change SerDes protocols between A and B hop by hop, as long as there
exists a chain of intermediary states which is at most ARRAY_SIZE(sd1_modes)
nodes deep. Interesting idea for sure, but it isn't going to have the
proper review if it's buried so deep as it is now.
> +{
> + unsigned long base = srds_base(FSL_SRDS_1);
> + const struct sd1_mode *from;
> + int ret;
> +
> + if (mode->is_active(base))
> + return 0;
> +
> + if (!mode->enter) {
> + printf("lx2160a serdes: serdes1:%s needs SD1 to be %s, it is
> not (",
> + target->name, mode->desc);
> + sd1_print_pcc(base);
> + printf("), refusing\n");
> + return -EINVAL;
> + }
> +
> + from = sd1_mode_find(mode->from);
> + if (!from || depth >= ARRAY_SIZE(sd1_modes))
> + return -EINVAL;
> +
> + ret = sd1_enter(from, target, depth + 1);
> + if (ret)
> + return ret;
> +
> + ret = mode->enter(base);
> + if (!mode->is_active(base)) {
> + printf("lx2160a serdes: SD1 is not %s (", mode->desc);
> + sd1_print_pcc(base);
> + printf("), stopping\n");
> + return ret ? ret : -EIO;
> }
>
> - printf("lx2160a serdes: SD1 100GE.1 split into 4x 25G%s\n",
> + printf("lx2160a serdes: SD1 now %s%s\n", mode->desc,
> ret ? " (with warnings above)" : "");
>
> - return ret;
> + return 0;
> }
