Re: [PATCH net-next v2] net: dsa: mv88e6060: add phylink_get_caps implementation

[Vladimir Oltean <olteanv@gmail.com>]

Hi Russell,

On Wed, Aug 09, 2023 at 03:46:03PM +0100, Russell King (Oracle) wrote:
> Add a phylink_get_caps implementation for Marvell 88e6060 DSA switch.
> This is a fast ethernet switch, with internal PHYs for ports 0 through
> 4. Port 4 also supports MII, REVMII, REVRMII and SNI. Port 5 supports
> MII, REVMII, REVRMII and SNI without an internal PHY.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
> Sergei,
> 
> Would it be possible for you to check that this patch works with your
> setup please?
> 
> Thanks!
> 
>  drivers/net/dsa/mv88e6060.c | 46 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
> index fdda62d6eb16..0e776be5e941 100644
> --- a/drivers/net/dsa/mv88e6060.c
> +++ b/drivers/net/dsa/mv88e6060.c
> @@ -247,11 +247,57 @@ mv88e6060_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val)
>  	return reg_write(priv, addr, regnum, val);
>  }
>  
> +static void mv88e6060_phylink_get_caps(struct dsa_switch *ds, int port,
> +				       struct phylink_config *config)
> +{
> +	unsigned long *interfaces = config->supported_interfaces;
> +	struct mv88e6060_priv *priv = ds->priv;
> +	int addr = REG_PORT(port);
> +	int ret;
> +
> +	ret = reg_read(priv, addr, PORT_STATUS);
> +	if (ret < 0) {
> +		dev_err(ds->dev,
> +			"port %d: unable to read status register: %pe\n",
> +			port, ERR_PTR(ret));
> +		return;
> +	}
> +
> +	if (!(ret & PORT_STATUS_PORTMODE)) {
> +		/* Port configured in SNI mode (acts as a 10Mbps PHY) */
> +		config->mac_capabilities = MAC_10 | MAC_SYM_PAUSE;
> +		/* I don't think SNI is SMII - SMII has a sync signal, and
> +		 * SNI doesn't.
> +		 */
> +		__set_bit(PHY_INTERFACE_MODE_SMII, interfaces);

I don't think that SNI == SMII either.

From what I could gather (datasheets of implementations in the wild,
rather than any official spec):
KSZ8895: https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8895MQX-RQX-FQX-MLX-Integrated-5-Port-10-100-Managed-Ethernet-Switch-with-MII-RMII-Interface-DS00002246C.pdf
DP83910A: https://www.ti.com/lit/ds/symlink/dp83910a.pdf
RTL8201BL: https://file.elecfans.com/web1/M00/9A/9F/pIYBAF0fDAuANJTlAAqoYDastvw919.pdf

SNI (7-wire Serial Network Interface) has the same structural pin layout
as the parallel MII/Rev-MII, except for the fact that RXD[3:0] and
TXD[3:0] became RXD0 and TXD0, resulting in an effectively "serial"
interface and reducing the baud rate of the 100 Mbps MII to a quarter,
and the TX_CLK and RX_CLK signals also operate at a reduced 10 MHz
rather than MII's 25 MHz, to provide a further 2.5x baud rate reduction
down to 10 Mbps. It was a once popular (in the 1990s) interface mode
between a MAC and a 10Mbps-only PHY.

If we compare that to SMII (Serial MII), I could only find this document here:
https://opencores.org/ocsvn/smii/smii/trunk/doc/SMII.pdf

but it appears to be quite different. SMII seems to be a gasket/encapsulation
module which serializes both the data and control signals of up to 4 10/100
Mbps MACs, which can be connected to a quad SMII PHY. The resulting
(cumulated, or individual) bandwidth is much larger than that of SNI, too.

The pinout of SMII is:
- one RX and one TX signal for each MAC. Data transfer consists of
  segments (10 serial bits on these lines). The bits in each segment are:
  RX direction: CRS, RX_DV, RXD0, RXD1, RXD2, RXD3, RXD4, RXD5, RXD6, RXD7
  TX direction: TX_ER, TX_EN, TXD0, TXD1, TXD2, TXD3, TXD4, TXD5, TXD6, TXD7
- SYNC: denotes the beginning of a new segment
- CLK: denotes the beginning of a new bit

So, I guess we have to introduce PHY_INTERFACE_MODE_SNI rather than
pretend it is the same as SMII.

The rest looks ok (but, as SNI is a 10Mbps only interface, you could, in
v2, populate config->mac_capabilities in a common code path to MAC_100 |
MAC_10, and let phylink_get_capabilities() reduce it).

pw-bot: cr

> +		return;
> +	}
> +
> +	config->mac_capabilities = MAC_100 | MAC_10 | MAC_SYM_PAUSE;
> +
> +	if (port >= 4) {
> +		/* Ports 4 and 5 can support MII, REVMII and REVRMII modes */
> +		__set_bit(PHY_INTERFACE_MODE_MII, interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_REVMII, interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_REVRMII, interfaces);
> +	}
> +	if (port <= 4) {
> +		/* Ports 0 to 3 have internal PHYs, and port 4 can optionally
> +		 * use an internal PHY.
> +		 */
> +		/* Internal PHY */
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL, interfaces);
> +		/* Default phylib interface mode */
> +		__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
> +	}
> +}
> +
>  static const struct dsa_switch_ops mv88e6060_switch_ops = {
>  	.get_tag_protocol = mv88e6060_get_tag_protocol,
>  	.setup		= mv88e6060_setup,
>  	.phy_read	= mv88e6060_phy_read,
>  	.phy_write	= mv88e6060_phy_write,
> +	.phylink_get_caps = mv88e6060_phylink_get_caps,
>  };
>  
>  static int mv88e6060_probe(struct mdio_device *mdiodev)
> -- 
> 2.30.2
>