RE: [PATCH net-next v4] net: dsa: microchip: enable the SGMII port of the KSZ9897S

[Tristram.Ha]

> The KSZ9897 has an SGMII/1000BASE-X capable port 7, but its chip_info
> entry never set .sgmii_port. ksz_has_sgmii_port() is therefore false,
> ksz9477_pcs_create() is never called and ksz9477_phylink_mac_select_pcs()
> returns NULL for port 7, so a phy-mode = "sgmii" or "1000base-x" link on
> that port cannot come up.
> 
> Only the KSZ9897S has that port; the KSZ9897R has a second RGMII port
> instead. Both report chip ID 0x00989700, so the port cannot be enabled
> from the chip ID alone, and enabling it unconditionally breaks the
> KSZ9897R badly: ksz9477_setup() propagates the failure of
> xpcs_create_pcs_mdiodev() on a part without an XPCS, so the switch as a
> whole stops probing.
> 
> The variants do differ in the read-only bit 7 of the port 7 XMII control
> 0 register, which reads one on the KSZ9897S and zero on the KSZ9897R.
> Compare the KSZ9897S data sheet DS00002394C section 5.2.4.1 with the
> KSZ9897R data sheet DS00002330D section 5.2.3.1.
> 
> Read that bit in ksz_switch_detect(), next to the existing KSZ8765 and
> KSZ9563 variant detection, and select a KSZ9897S chip_info entry which
> is the KSZ9897 one plus .sgmii_port = 7, with port 7 no longer marked as
> supporting MII, RMII and RGMII. On the KSZ9897S that port is permanently
> an SGMII port and only its differential pairs are brought out, so
> advertising the xMII modes on it would let phylink accept a mode the
> driver never programs: ksz_phylink_need_config() skips the XMII control
> registers for the SGMII port. The KSZ9477, which also has SGMII on port
> 7, is described the same way. See DS00002394C section 4.11.
> 
> KSZ9897S_CHIP_ID only selects that entry and is never read from a
> device: the low byte holds the silicon revision, which detection masks
> off, so no device can report it.
> 
> Device trees describe the KSZ9897 without naming the variant, so accept
> a detected KSZ9897S for a device tree naming the KSZ9897. No binding
> change is needed and existing device trees keep working, whichever
> variant is fitted.
> 
> Tested on a KSZ9897S with a 1000BASE-LX fibre SFP on port 7: bit 7 of
> the port 7 XMII control 0 register reads one, the switch is identified
> as a KSZ9897S and the PCS is created. The port advertises
> 1000baseX/Full, clause 37 in-band autonegotiation completes against the
> link partner, and the link comes up at 1Gbps/full duplex and passes
> traffic.
> 
> Suggested-by: Nicolai Buchwitz <nb@xxxxxxxxxxx>
> Suggested-by: Jerry Ray <Jerry.Ray@xxxxxxxxxxxxx>
> Signed-off-by: Tapio Reijonen <tapio.reijonen@xxxxxxxxxxx>
> ---
> Changes in v4:
> - KSZ9897S: stop advertising MII, RMII and RGMII on port 7. On the S
>   that port is permanently SGMII with only its differential pairs
>   brought out (DS00002394C section 4.11), and
>   ksz_phylink_need_config() skips the XMII control registers for the
>   SGMII port, so phylink could accept an xMII mode that the driver
>   never programs. The KSZ9477, which also has SGMII on port 7, is
>   already described this way. v3 inherited the KSZ9897 values, which
>   are correct only for the R, whose port 7 is a second RGMII port.
>   Flagged by the netdev Sashiko review bot on v3 and then confirmed
>   against the data sheet.
> - The in-tree LAN9646 entry pairs .sgmii_port = 7 with the same xMII
>   advertisement and looks wrong for the same reason; not touched here.
> - Link to v3: https://lore.kernel.org/r/20260831-ksz9897-sgmii-port-v3-1-
> 2376aa4edf11@xxxxxxxxxxx
> 
> Changes in v3:
> - Only enable the SGMII port on the KSZ9897S. v2 enabled it for every
>   KSZ9897, which makes a KSZ9897R fail to probe at all, because
>   ksz9477_setup() propagates the xpcs_create_pcs_mdiodev() failure on a
>   part that has no XPCS. Reported by Nicolai Buchwitz.
> - Tell the two variants apart at run time in ksz_switch_detect(), from
>   the read-only bit 7 of the port 7 XMII control 0 register, and select
>   a KSZ9897S chip_info entry, next to the existing KSZ8765 and KSZ9563
>   variant detection. Register suggested by Nicolai Buchwitz and
>   confirmed by Jerry Ray against both data sheets.
> - Accept a detected KSZ9897S for a device tree naming the KSZ9897, so
>   no binding change is needed and existing device trees keep working.
> - Tested on a KSZ9897S with a 1000BASE-LX fibre SFP on port 7.
> - Link to v2: https://lore.kernel.org/r/20260811-ksz9897-sgmii-port-v2-1-
> 04ae287a0b27@xxxxxxxxxxx
> 
> Changes in v2:
> - Resend only, no functional change: v1 was dropped when the netdev
>   patch queue overflowed.
> - Rebased onto current net-next.
> - Link to v1: https://lore.kernel.org/r/20260703-ksz9897-sgmii-port-v1-1-
> 387e7632dd0b@xxxxxxxxxxx
> ---
>  drivers/net/dsa/microchip/ksz_common.c      | 60
> ++++++++++++++++++++++++++++-
>  drivers/net/dsa/microchip/ksz_common.h      |  5 +++
>  include/linux/platform_data/microchip-ksz.h |  1 +
>  3 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index
> 1a9d6f83a0232830ac4996ae5f53b8bde0b3747d..0bc809f582bed3abe50ce5055
> 3c7bfaaf4c75ead 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1486,6 +1486,41 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>                 .gbit_capable   = {true, true, true, true, true, true, true},
>         },
> 
> +       [KSZ9897S] = {
> +               .chip_id = KSZ9897S_CHIP_ID,
> +               .dev_name = "KSZ9897S",
> +               .num_vlans = 4096,
> +               .num_alus = 4096,
> +               .num_statics = 16,
> +               .cpu_ports = 0x7F,      /* can be configured as cpu port */
> +               .port_cnt = 7,          /* total physical port count */
> +               .port_nirqs = 2,
> +               .num_tx_queues = 4,
> +               .num_ipms = 8,
> +               .ops = &ksz9477_dev_ops,
> +               .switch_ops = &ksz9477_switch_ops,
> +               .phylink_mac_ops = &ksz9477_phylink_mac_ops,
> +               .phy_errata_9477 = true,
> +               .mib_names = ksz9477_mib_names,
> +               .mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
> +               .reg_mib_cnt = MIB_COUNTER_NUM,
> +               .regs = ksz9477_regs,
> +               .masks = ksz9477_masks,
> +               .shifts = ksz9477_shifts,
> +               .xmii_ctrl0 = ksz9477_xmii_ctrl0,
> +               .xmii_ctrl1 = ksz9477_xmii_ctrl1,
> +               .supports_mii   = {false, false, false, false,
> +                                  false, true, false},
> +               .supports_rmii  = {false, false, false, false,
> +                                  false, true, false},
> +               .supports_rgmii = {false, false, false, false,
> +                                  false, true, false},
> +               .internal_phy   = {true, true, true, true,
> +                                  true, false, false},
> +               .gbit_capable   = {true, true, true, true, true, true, true},
> +               .sgmii_port = 7,
> +       },
> +
>         [KSZ9893] = {
>                 .chip_id = KSZ9893_CHIP_ID,
>                 .dev_name = "KSZ9893",
> @@ -1852,6 +1887,13 @@ static int ksz_check_device_id(struct ksz_device
> *dev)
>                 expected_chip_id = expected_chip_data->chip_id;
>         }
> 
> +       /* The KSZ9897S is only told apart from the KSZ9897R at run time, so a
> +        * device tree naming the KSZ9897 matches it as well.
> +        */
> +       if (expected_chip_id == KSZ9897_CHIP_ID &&
> +           dev->chip_id == KSZ9897S_CHIP_ID)
> +               return 0;
> +
>         if (expected_chip_id != dev->chip_id) {
>                 dev_err(dev->dev,
>                         "Device tree specifies chip %s but found %s, please fix it!\n",
> @@ -2966,7 +3008,6 @@ static int ksz_switch_detect(struct ksz_device *dev)
>                 switch (id32) {
>                 case KSZ9477_CHIP_ID:
>                 case KSZ9896_CHIP_ID:
> -               case KSZ9897_CHIP_ID:
>                 case KSZ9567_CHIP_ID:
>                 case KSZ8567_CHIP_ID:
>                 case LAN9370_CHIP_ID:
> @@ -2979,6 +3020,23 @@ static int ksz_switch_detect(struct ksz_device *dev)
>                         if (dev->chip_id != LAN9646_CHIP_ID)
>                                 dev->chip_id = id32;
>                         break;
> +               case KSZ9897_CHIP_ID:
> +                       /* Only the KSZ9897S has an SGMII port 7; the KSZ9897R
> +                        * has a second RGMII port instead. See the KSZ9897S
> +                        * data sheet DS00002394C section 5.2.4.1 and the
> +                        * KSZ9897R data sheet DS00002330D section 5.2.3.1.
> +                        */
> +                       ret = ksz_read8(dev, KSZ9897_REG_PORT7_XMII_CTRL_0,
> +                                       &id4);
> +                       if (ret)
> +                               return ret;
> +
> +                       if (id4 & KSZ9897_PORT7_SGMII_SEL)
> +                               dev->chip_id = KSZ9897S_CHIP_ID;
> +                       else
> +                               dev->chip_id = KSZ9897_CHIP_ID;
> +
> +                       break;
>                 case KSZ9893_CHIP_ID:
>                         ret = ksz_read8(dev, REG_CHIP_ID4,
>                                         &id4);
> diff --git a/drivers/net/dsa/microchip/ksz_common.h
> b/drivers/net/dsa/microchip/ksz_common.h
> index
> cbe98494578c371ee22b80f9508c3b35b8d6291a..0cff33df06a4b8ca585d94f9fe4
> 37ad62bcd82dc 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -284,6 +284,7 @@ enum ksz_model {
>         KSZ9477,
>         KSZ9896,
>         KSZ9897,
> +       KSZ9897S,
>         KSZ9893,
>         KSZ9563,
>         KSZ9567,
> @@ -874,6 +875,10 @@ static inline bool ksz_is_sgmii_port(struct ksz_device
> *dev, int port)
>  #define SKU_ID_KSZ8563                 0x3c
>  #define SKU_ID_KSZ9563                 0x1c
> 
> +/* KSZ9897 specific register: port 7 XMII control 0 */
> +#define KSZ9897_REG_PORT7_XMII_CTRL_0  0x7300
> +#define KSZ9897_PORT7_SGMII_SEL                BIT(7)
> +
>  /* Driver set switch broadcast storm protection at 10% rate. */
>  #define BROADCAST_STORM_PROT_RATE      10
> 
> diff --git a/include/linux/platform_data/microchip-ksz.h
> b/include/linux/platform_data/microchip-ksz.h
> index
> 028781ad40593248bae1b501e40ce31c5e0c69ca..fe8df1774289c401445ef308c8
> d3656e1e7c25b6 100644
> --- a/include/linux/platform_data/microchip-ksz.h
> +++ b/include/linux/platform_data/microchip-ksz.h
> @@ -34,6 +34,7 @@ enum ksz_chip_id {
>         KSZ9477_CHIP_ID = 0x00947700,
>         KSZ9896_CHIP_ID = 0x00989600,
>         KSZ9897_CHIP_ID = 0x00989700,
> +       KSZ9897S_CHIP_ID = 0x00989701,
>         KSZ9893_CHIP_ID = 0x00989300,
>         KSZ9563_CHIP_ID = 0x00956300,
>         KSZ8567_CHIP_ID = 0x00856700,

The only needed change to add SGMII port support to KSZ9897 is the
chip_data information specifying the sgmii port.  This information is
assigned using the ksz_lookup_info function.  It is simpler to check the
chip to see if there is a SGMII port, then use KSZ9897S_CHIP_ID to get the
chip_data information and keep using KSZ9897_CHIP_ID in dev->chip_id so
there is no need to add KSZ9897S_CHIP_ID in other chip comparisons.

The device tree will need to update to use SFP for the 7 port but no need
to actually specifythe ksz9897S chip.