Re: [PATCH net-next v1] net:dsa:yt922x: Add support for Motorcomm YT922x

[Kyle Switch]

On 8/17/26 05:17, David Yang wrote:
> On Fri, Aug 14, 2026 at 9:50 AM Kyle Switch <kyle.switch@xxxxxxxxxxxxxx> wrote:
>
> > @@ -148,10 +148,15 @@ static const struct yt921x_info yt921x_infos[] = {
> >                  YT921X_PORT_MASK_INT0_n(8),
> >                  YT921X_PORT_MASK_EXT0 | YT921X_PORT_MASK_EXT1,
> >          },
> > +       {
> > +               "YT9224", YT9224_MAJOR, 0, 0,
> > +               0x1f1,
> > +               0x0,
> > +       },
> Why plain magic numbers here, despite what the above entries shows?
Ans: will fixed done in patch v2.
> >          {}
> >   };
> >
> > -#define YT921X_NAME    "yt921x"
> > +#define YT92XX_NAME    "yt92xx"
> This changed the module name which may affect end users. While I have
> no objection to this, subsystem maintainers may have different
> opinions.
Ans: This is used to indicate support for both YT921X and YT922X.
> > +static int
> > +yt922x_intif_ext_read(struct yt921x_priv *priv, int port, int reg, u16 *valp)
> [...]
>
> > +static int
> > +yt921x_intif_ext_write(struct yt921x_priv *priv, int port, int reg, u16 val)
> Place them with yt921x_*if_*().
Ans: fix done in patch v2.
> > +static int yt922x_internal_phyaddr_get(int port,
> > +                                      enum yt922x_phy_reg_type reg_type,
> > +                                      enum yt922x_phy_reg_space reg_space)
> [...]
>
> This makes me headache and is prone to errors (an example below).
> Reorder it with early returns and/or lookup tables so that it is
> obviously correct.
Ans: The interface will be optimized in v1 using early returns.
> > +               default:
> > +                       if (reg_space != YT922X_PHY_REG_SPACE_PHY)
> > +                               res = -EINVAL;
> > +                       break;
> What would you expect here, when reg_space == YT922X_PHY_REG_SPACE_PHY?
Ans: The expected return result is an error.
> > +static int yt922x_port_sds_init(struct yt921x_priv *priv, int port,
> > +                               phy_interface_t interface)
> [...]
>
> > +       addr = yt922x_internal_phyaddr_get(port, YT922X_PHY_REG_TYPE_MII,
> > +                                          YT922X_PHY_REG_SPACE_SGMII);
> > +       if (addr < 0)
> > +               return res;
> > +       res = yt921x_intif_read(priv, addr, 0x2000, &data);
> > +       if (res)
> > +               return res;
> > +       data &= ~(1 << 15);
> > +       res = yt921x_intif_write(priv, addr, 0x2000, data);
> > +       if (res)
> > +               return res;
> > +       addr = yt922x_internal_phyaddr_get(port, YT922X_PHY_REG_TYPE_MII,
> > +                                          YT922X_PHY_REG_SPACE_USXGMII);
> > +       if (addr < 0)
> > +               return res;
> > +       res = yt921x_intif_read(priv, addr, 0x0, &data);
> > +       if (res)
> > +               return res;
> > +       data |= 1 << 15;
> > +       res = yt921x_intif_write(priv, addr, 0x0, data);
> > +       if (res)
> > +               return res;
> Numerous magic numbers. On YT9215, same finetunes are not required for
> a usable port. If you cannot name it, drop it if it is not mandatory.

Ans: Some patches are mandatory, while others are for performance 
optimization.

       These will be further refined/optimized in the future.

> > +static int yt922x_cpu_tag_mode_set(struct yt921x_priv *priv)
> > +{
> > +       struct device *dev = to_device(priv);
> > +       u16 eth_p_tag;
> > +       u32 val;
> > +       u32 val1;
> > +       int res;
> > +
> > +       /* cpu tag mode set */
> > +       res = yt921x_reg_read(priv, YT922X_CPU_TAG_RX_CTRL, &val);
> > +       if (res)
> > +               return res;
> > +       res = yt921x_reg_read(priv, YT922X_CPU_TAG_TX_CTRL, &val1);
> > +       if (res)
> > +               return res;
> > +       val &= ~YT922X_CPU_TAG_RX_MODE;
> > +       val1 &= ~YT922X_CPU_TAG_TX_MODE;
> > +       val1 &= ~YT922X_CPU_TAG_TX_TYPE;
> > +       res = yt921x_reg_write(priv, YT922X_CPU_TAG_RX_CTRL, val);
> > +       if (res)
> > +               return res;
> > +       res = yt921x_reg_write(priv, YT922X_CPU_TAG_TX_CTRL, val1);
> > +       if (res)
> > +               return res;
> > +
> > +       /* tpid check */
> > +       res = yt921x_reg_read(priv, YT921X_CPU_TAG_TPID, &val);
> > +       if (res)
> > +               return res;
> > +       eth_p_tag = FIELD_GET(YT921X_CPU_TAG_TPID_TPID_M, val);
> > +       if (eth_p_tag != ETH_P_YT921X) {
> > +               dev_err(dev, "Tag type 0x%x != 0x%x\n", eth_p_tag,
> > +                       ETH_P_YT921X);
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> This looks similar to yt921x_chip_reset(), so you'd better merge it
> into yt922x_chip_reset() above.

Ans:Given that this interface is responsible for mode selection related 
to CPU tags, the cpu tag

       TPID check has been consolidated here as well.

> > +static int yt922x_cpu_port_set(struct yt921x_priv *priv)
> > +{
> > +       struct dsa_switch *ds = &priv->ds;
> > +       u32 ctrl;
> > +       int res;
> > +
> > +       /* cpu tag mode */
> > +       res = yt922x_cpu_tag_mode_set(priv);
> > +       if (res)
> > +               return res;
> > +
> > +       /* Enable DSA */
> > +       priv->cpu_ports_mask = dsa_cpu_ports(ds);
> > +       ctrl = YT921X_EXT_CPU_PORT_TAG_EN | YT921X_EXT_CPU_PORT_PORT_EN |
> > +              YT921X_EXT_CPU_PORT_PORT(__ffs(priv->cpu_ports_mask));
> > +       res = yt921x_reg_write(priv, YT921X_EXT_CPU_PORT, ctrl);
> > +       if (res)
> > +               return res;
> > +
> > +       /* Setup software switch */
> > +       ctrl = YT922X_CPU_COPY_TO_EXT_CPU;
> > +       res = yt921x_reg_write(priv, YT922X_CPU_COPY, ctrl);
> > +       if (res)
> > +               return res;
> > +
> > +       return res;
> > +}
> Also, snippets from yt921x_chip_setup_dsa() and unaligned function names.
>
> > -static void yt921x_mdio_remove(struct mdio_device *mdiodev)
> > +static void yt92xx_mdio_remove(struct mdio_device *mdiodev)
> Unnecessary renaming.
>
> > +#define YT922X_PORT_SDS_MODE           0x400
> > +#define YT922X_PORT_SDS_MODE_M         GENMASK(6, 4)
> > +#define YT92XX_SERDES_MODE_SGMII                       0
> > +#define YT92XX_SERDES_MODE_REVSGMII                    1
> > +#define YT92XX_SERDES_MODE_1000BASEX                   2
> > +#define YT92XX_SERDES_MODE_100BASEX                    3
> > +#define YT92XX_SERDES_MODE_2500BASEX                   4
> > +#define YT92XX_SERDES_MODE_USXGMII                     6
> Not an objection, but better to keep the formats with YT921X_SERDESn:
Ans: fix done in patch v2.
> #define YT922X_PORT_SERDES           0x400
> #define  YT922X_PORT_SERDES_MODE_M           GENMASK(6, 4)
> #define   YT922X_PORT_SERDES_MODE(x)
> FIELD_PREP(YT922X_PORT_SERDES_MODE_M, (x))
> #define   YT922X_PORT_SERDES_MODE_SGMII              YT922X_PORT_SERDES_MODE(0)
>
> > +#define YT922X_ACT_UNK_ACTn_M(port)            GENMASK(2 * (port) + 1, 2 * (port))
> > +#define YT922X_ACT_UNK_ACTn(port, x)           ((x) << (2 * (port)))
> > +#define YT922X_ACT_UNK_ACTn_FORWARD(port)      YT922X_ACT_UNK_ACTn(port, 0)  /* flood */
> > +#define YT922X_ACT_UNK_ACTn_TRAP(port)         YT922X_ACT_UNK_ACTn(port, 3)  /* steer to CPU */
> > +#define YT922X_ACT_UNK_ACTn_DROP(port)         YT922X_ACT_UNK_ACTn(port, 1)  /* discard */
> > +/* NEVER use this action; see comments in the tag driver */
> > +#define YT922X_ACT_UNK_ACTn_COPY(port)         YT922X_ACT_UNK_ACTn(port, 2)  /* flood and copy */
> Sort them in numerical order.
Ans: Fix done.
> > +
> > +/* CPU PORT */
> > +#define YT922X_CPU_COPY                        0x181100
> > +#define YT922X_CPU_COPY_TO_INT_CPU     BIT(1)
> > +#define YT922X_CPU_COPY_TO_EXT_CPU     BIT(0)
> > +#define YT922X_CPU_TAG_RX_CTRL         0x80504
> > +#define YT922X_CPU_TAG_RX_MODE         BIT(0)
> > +#define YT922X_CPU_TAG_TX_CTRL         0x100710
> > +#define YT922X_CPU_TAG_TX_TYPE         BIT(0)
> > +#define YT922X_CPU_TAG_TX_MODE         BIT(1)
> > +#define YT922X_CPU_TAG_TX_CTAG_OP      BIT(2)
> > +#define YT922X_CPU_TAG_TX_STAG_OP      BIT(3)
> Not an objection, but better to keep the indentation formats of yt921x
> register definitions above.
>
> > +struct yt92xx_chip_info {
> > +       enum yt92xx_mode mode;
> > +       const char *name;
> > +       unsigned int ports;
> > +       unsigned int lag_id;
> lag_ids, or num_lag_ids (and num_ports) for consistency.
>
> >   struct yt921x_priv {
> >          struct dsa_switch ds;
> >
> > +       const struct yt92xx_chip_info *chip_info;
> >          const struct yt921x_info *info;
> yt921x_priv :: info is chip info already. You may want `series_info`,
> or more straightforward, `series` and `struct yt92xx_series`.
Ans: Fix done.
> > diff --git a/net/dsa/tag_yt921x.c b/net/dsa/tag_yt921x.c
> > index 294784ab6694..a3012ec39868 100644
> > --- a/net/dsa/tag_yt921x.c
> > +++ b/net/dsa/tag_yt921x.c
> [...]
>
> > +/* To define the from cpu tag format 8 bytes:
> > + *
> > + * 0 1 2 3 4 5 6 7 |0 1 2 3 4 5 6 7
> > + *|<------------TPID 0x9988------->|
> > + *|<--RESERVE-->|<-----DST POR---->|
> > + *|-|<---------RESERVE------------>|
> > + *|<------------------------------>|
> > + */
> > +#define YT922X_TAG_NAME                        "yt922x"
> > +#define YT922X_TAG_PORTMASK_0          BIT(15)
> Sort them according to their corresponding word.
>
> > +#define YT922X_TAG_PORTMASK_M          GENMASK(8, 0)
> > +#define YT922X_TAG_PORTS(x)            FIELD_PREP(YT922X_TAG_PORTMASK_M, (x))
> > +#define YT922X_TAG_FORCE_DST           BIT(9)
> > +#define YT922X_TAG_PRIO_M              GENMASK(12, 10)
> > +#define YT922X_TAG_PRIO_EN             BIT(13)
> > +#define YT922X_TAG_PRIO(x)             (FIELD_PREP(YT922X_TAG_PRIO_M, (x)) | YT922X_TAG_PRIO_EN)
> > +#define YT922X_TAG_RX_PORT_M           GENMASK(5, 2)
> > +#define YT922X_TAG_RX_PRIO_M           GENMASK(15, 13)
> This did not answer the question in the previous review: do tag_yt922x
> and tag_yt921x share any common routines? If not, better to make
> tag_yt922x a separate file.
Ans: Fix done.
> > +static struct sk_buff *
> > +yt922x_tag_xmit(struct sk_buff *skb, struct net_device *netdev)
> > +{
> > +       struct dsa_port *dp = dsa_user_to_port(netdev);
> > +       __be16 *tag;
> > +       u16 ctrl;
> > +
> > +       skb_push(skb, YT921X_TAG_LEN);
> > +       dsa_alloc_etype_header(skb, YT921X_TAG_LEN);
> > +       tag = dsa_etype_header_pos_tx(skb);
> > +
> > +       tag[0] = htons(ETH_P_YT921X);
> > +       if (dp->index != 0) {
> > +               /* Port index is not equal 0 in tag[1] */
> > +               ctrl = YT922X_TAG_PRIO(skb->priority) | YT922X_TAG_FORCE_DST |
> > +                       YT922X_TAG_PORTS(dsa_xmit_port_mask(skb, netdev) - 1);
> dsa_xmit_port_mask() returns a bit mask - you won't do arithmetic
> operations to a bit mask normally.
>
> The field name does suggest it is a port mask. Have you noticed
> excessive packets on other ports?
Ans: yes, There are some issues here that will be fixed in patch v2.