Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

[Andrew Lunn <andrew@lunn.ch>]

> Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
> Co-developed-by: Michael Rasmussen <mir@bang-olufsen.dk>
> Signed-off-by: Michael Rasmussen <mir@bang-olufsen.dk>

Hi Alvin

Since you are submitting the patch, your SOB goes last.

> +/* Port mapping macros
> + *
> + * PORT_NUM_x2y: map a port number from domain x to domain y
> + * PORT_MASK_x2y: map a port mask from domain x to domain y
> + *
> + * L = logical port domain, i.e. dsa_port.index
> + * P = physical port domain, used by the Realtek ASIC for port indexing;
> + *     for ports with internal PHYs, this is also the PHY index
> + * E = extension port domain, used by the Realtek ASIC for managing EXT ports
> + *
> + * The terminology is borrowed from the vendor driver. The extension port domain
> + * is mostly used to navigate the labyrinthine layout of EXT port configuration
> + * registers and is not considered intuitive by the author.
> + *
> + * Unless a function is accessing chip registers, it should be using the logical
> + * port domain. Moreover, function arguments for port numbers and port masks
> + * must always be in the logical domain. The conversion must be done as close as
> + * possible to the register access to avoid chaos.
> + *
> + * The mappings vary between chips in the family supported by this driver. Here
> + * is an example of the mapping for the RTL8365MB-VC:
> + *
> + *    L | P | E | remark
> + *   ---+---+---+--------
> + *    0 | 0 |   | user port
> + *    1 | 1 |   | user port
> + *    2 | 2 |   | user port
> + *    3 | 3 |   | user port
> + *    4 | 6 | 1 | extension (CPU) port

Did you consider not bothering with this. Just always use the Physical
port number? The DSA framework does not care if there are ports
missing. If it makes the code simpler, ignore the logical number, and
just enforce that the missing ports are not used, by returning -EINVAL
in the port_enable() callback.

> +/* Interrupt control register - enable or disable specific interrupt types */
> +#define RTL8365MB_INTR_CTRL				0x1101
> +#define   RTL8365MB_INTR_CTRL_SLIENT_START_2_MASK	0x1000
> +#define   RTL8365MB_INTR_CTRL_SLIENT_START_MASK		0x800
> +#define   RTL8365MB_INTR_CTRL_ACL_ACTION_MASK		0x200
> +#define   RTL8365MB_INTR_CTRL_CABLE_DIAG_FIN_MASK	0x100
> +#define   RTL8365MB_INTR_CTRL_INTERRUPT_8051_MASK	0x80
> +#define   RTL8365MB_INTR_CTRL_LOOP_DETECTION_MASK	0x40
> +#define   RTL8365MB_INTR_CTRL_GREEN_TIMER_MASK		0x20
> +#define   RTL8365MB_INTR_CTRL_SPECIAL_CONGEST_MASK	0x10
> +#define   RTL8365MB_INTR_CTRL_SPEED_CHANGE_MASK		0x8
> +#define   RTL8365MB_INTR_CTRL_LEARN_OVER_MASK		0x4
> +#define   RTL8365MB_INTR_CTRL_METER_EXCEEDED_MASK	0x2
> +#define   RTL8365MB_INTR_CTRL_LINK_CHANGE_MASK		0x1
> +
> +
> +/* Interrupt status register */
> +#define RTL8365MB_INTR_STATUS_REG			0x1102
> +#define   RTL8365MB_INTR_STATUS_SLIENT_START_2_MASK	0x1000
> +#define   RTL8365MB_INTR_STATUS_SLIENT_START_MASK	0x800
> +#define   RTL8365MB_INTR_STATUS_ACL_ACTION_MASK		0x200
> +#define   RTL8365MB_INTR_STATUS_CABLE_DIAG_FIN_MASK	0x100
> +#define   RTL8365MB_INTR_STATUS_INTERRUPT_8051_MASK	0x80
> +#define   RTL8365MB_INTR_STATUS_LOOP_DETECTION_MASK	0x40
> +#define   RTL8365MB_INTR_STATUS_GREEN_TIMER_MASK	0x20
> +#define   RTL8365MB_INTR_STATUS_SPECIAL_CONGEST_MASK	0x10
> +#define   RTL8365MB_INTR_STATUS_SPEED_CHANGE_MASK	0x8
> +#define   RTL8365MB_INTR_STATUS_LEARN_OVER_MASK		0x4
> +#define   RTL8365MB_INTR_STATUS_METER_EXCEEDED_MASK	0x2
> +#define   RTL8365MB_INTR_STATUS_LINK_CHANGE_MASK	0x1
> +#define   RTL8365MB_INTR_STATUS_ALL_MASK                      \

Interrupt control and status registers are generally identical. So you
don't need to define the values twice.

 +static void rtl8365mb_phylink_validate(struct dsa_switch *ds, int port,
> +				       unsigned long *supported,
> +				       struct phylink_link_state *state)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0 };
> +
> +	/* include/linux/phylink.h says:
> +	 *     When @state->interface is %PHY_INTERFACE_MODE_NA, phylink
> +	 *     expects the MAC driver to return all supported link modes.
> +	 */
> +	if (state->interface != PHY_INTERFACE_MODE_NA &&
> +	    !rtl8365mb_phy_mode_supported(ds, port, state->interface)) {
> +		dev_err(smi->dev, "phy mode %s is unsupported on port %d\n",
> +			phy_modes(state->interface), port);
> +		linkmode_zero(supported);
> +		return;
> +	}
> +
> +	phylink_set_port_modes(mask);
> +
> +	phylink_set(mask, Autoneg);
> +	phylink_set(mask, Pause);
> +	phylink_set(mask, Asym_Pause);
> +
> +	phylink_set(mask, 10baseT_Half);
> +	phylink_set(mask, 10baseT_Full);
> +	phylink_set(mask, 100baseT_Half);
> +	phylink_set(mask, 100baseT_Full);
> +	phylink_set(mask, 1000baseT_Full);
> +	phylink_set(mask, 1000baseT_Half);

Does the documentation actually mention 1000baseT_Half? Often it is
not implemented.

> +static int rtl8365mb_port_enable(struct dsa_switch *ds, int port,
> +				 struct phy_device *phy)
> +{
> +	struct realtek_smi *smi = ds->priv;
> +	int val;
> +
> +	if (dsa_is_user_port(ds, port)) {
> +		/* Power up the internal PHY and restart autonegotiation */
> +		val = rtl8365mb_phy_read(smi, port, MII_BMCR);
> +		if (val < 0)
> +			return val;
> +
> +		val &= ~BMCR_PDOWN;
> +		val |= BMCR_ANRESTART;
> +
> +		return rtl8365mb_phy_write(smi, port, MII_BMCR, val);
> +	}

There should not be any need to do this. phylib should do it. In
generally, you should not need to touch any PHY registers, you just
need to allow access to the PHY registers.

I want to take another look at the interrupt code. But this looks
pretty nice in general.

       Andrew