Re: [PATCH net-next v9 3/3] net: dsa: yt921x: Add support for Motorcomm YT921x

[Andrew Lunn <andrew@lunn.ch>]

> +static int
> +yt921x_port_up(struct yt921x_priv *priv, int port, unsigned int mode,
> +	       phy_interface_t interface, int speed, int duplex,
> +	       bool tx_pause, bool rx_pause)
> +{
> +	struct device *dev = to_device(priv);
> +	u32 mask;
> +	u32 ctrl;
> +	int res;
> +
> +	switch (speed) {
> +	case SPEED_10:
> +		ctrl = YT921X_PORT_SPEED_10;
> +		break;
> +	case SPEED_100:
> +		ctrl = YT921X_PORT_SPEED_100;
> +		break;
> +	case SPEED_1000:
> +		ctrl = YT921X_PORT_SPEED_1000;
> +		break;
> +	case SPEED_10000:
> +		ctrl = YT921X_PORT_SPEED_10000;
> +		break;
> +	case SPEED_2500:
> +		ctrl = YT921X_PORT_SPEED_2500;
> +		break;
> +	default:
> +		dev_err(dev, "Unsupported speed %d\n", speed);
> +		/* compile complains about uninitialized variable */
> +		ctrl = 0;
> +		break;

If this should not happen, it is better to return -EINVAL.

I would also suggest sorting these numerically. 10G after 2.5G.

> +	}
> +	if (duplex == DUPLEX_FULL)
> +		ctrl |= YT921X_PORT_DUPLEX_FULL;
> +	if (tx_pause)
> +		ctrl |= YT921X_PORT_TX_PAUSE;
> +	if (rx_pause)
> +		ctrl |= YT921X_PORT_RX_PAUSE;
> +	ctrl |= YT921X_PORT_RX_MAC_EN | YT921X_PORT_TX_MAC_EN;
> +	res = yt921x_reg_write(priv, YT921X_PORTn_CTRL(port), ctrl);
> +	if (res)
> +		return res;
> +
> +	if (yt921x_port_is_external(port)) {
> +		mask = YT921X_SGMII_SPEED_M;
> +		switch (speed) {
> +		case SPEED_10:
> +			ctrl = YT921X_SGMII_SPEED_10;
> +			break;
> +		case SPEED_100:
> +			ctrl = YT921X_SGMII_SPEED_100;
> +			break;
> +		case SPEED_1000:
> +			ctrl = YT921X_SGMII_SPEED_1000;
> +			break;
> +		case SPEED_10000:
> +			ctrl = YT921X_SGMII_SPEED_10000;
> +			break;
> +		case SPEED_2500:
> +			ctrl = YT921X_SGMII_SPEED_2500;
> +			break;
> +		default:
> +			ctrl = 0;
> +			break;

Same here.

> +		}
> +		mask |= YT921X_SGMII_DUPLEX_FULL;
> +		if (duplex == DUPLEX_FULL)
> +			ctrl |= YT921X_SGMII_DUPLEX_FULL;
> +		mask |= YT921X_SGMII_TX_PAUSE;
> +		if (tx_pause)
> +			ctrl |= YT921X_SGMII_TX_PAUSE;
> +		mask |= YT921X_SGMII_RX_PAUSE;
> +		if (rx_pause)
> +			ctrl |= YT921X_SGMII_RX_PAUSE;
> +		mask |= YT921X_SGMII_LINK;
> +		ctrl |= YT921X_SGMII_LINK;
> +		res = yt921x_reg_update_bits(priv, YT921X_SGMIIn(port),
> +					     mask, ctrl);
> +		if (res)
> +			return res;
> +
> +		mask = YT921X_XMII_LINK;
> +		res = yt921x_reg_set_bits(priv, YT921X_XMIIn(port), mask);
> +		if (res)
> +			return res;
> +
> +		switch (speed) {
> +		case SPEED_10:
> +			ctrl = YT921X_MDIO_POLLING_SPEED_10;
> +			break;
> +		case SPEED_100:
> +			ctrl = YT921X_MDIO_POLLING_SPEED_100;
> +			break;
> +		case SPEED_1000:
> +			ctrl = YT921X_MDIO_POLLING_SPEED_1000;
> +			break;
> +		case SPEED_10000:
> +			ctrl = YT921X_MDIO_POLLING_SPEED_10000;
> +			break;
> +		case SPEED_2500:
> +			ctrl = YT921X_MDIO_POLLING_SPEED_2500;
> +			break;
> +		default:
> +			ctrl = 0;
> +			break;

and again.


> +static int
> +yt921x_port_config(struct yt921x_priv *priv, int port, unsigned int mode,
> +		   phy_interface_t interface)
> +{
> +	struct device *dev = to_device(priv);
> +	u32 mask;
> +	u32 ctrl;
> +	int res;
> +
> +	if (!yt921x_port_is_external(port)) {
> +		if (interface != PHY_INTERFACE_MODE_INTERNAL) {
> +			dev_err(dev, "Wrong mode %d on port %d\n",
> +				interface, port);
> +			return -EINVAL;
> +		}
> +		return 0;
> +	}
> +
> +	switch (interface) {
> +	/* SGMII */
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_100BASEX:
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	case PHY_INTERFACE_MODE_2500BASEX:

This comment is wrong. Only the first is SGMII.

> +static void
> +yt921x_phylink_mac_link_down(struct phylink_config *config, unsigned int mode,
> +			     phy_interface_t interface)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct dsa_switch *ds = dp->ds;
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct device *dev = to_device(priv);
> +	int port = dp->index;
> +	int res;
> +
> +	cancel_delayed_work(&priv->ports[port].mib_read);

Should that be cancel_delayed_work_sync() ? Does it matter if the work
is running on a different CPU?

> +static void
> +yt921x_dsa_phylink_get_caps(struct dsa_switch *ds, int port,
> +			    struct phylink_config *config)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	const struct yt921x_info *info = priv->info;
> +
> +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +				   MAC_10 | MAC_100 | MAC_1000;
> +
> +	if ((info->internal_mask & BIT(port)) != 0) {
> +		/* Port 10 for MCU should probably go here too. But since that
> +		 * is untested yet, turn it down for the moment by letting it
> +		 * fall to the default branch.
> +		 */
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +	} else if ((info->external_mask & BIT(port)) != 0) {
> +		/* TODO: external ports may support SGMII only, XMII only, or
> +		 * SGMII + XMII depending on the chip. However, we can't get
> +		 * the accurate config table due to lack of document, thus
> +		 * we simply declare SGMII + XMII and rely on the correctness
> +		 * of devicetree for now.
> +		 */
> +
> +		/* SGMII */
> +		__set_bit(PHY_INTERFACE_MODE_SGMII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_100BASEX,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +			  config->supported_interfaces);
> +		config->mac_capabilities |= MAC_2500FD;

And again

> +static int yt921x_chip_detect(struct yt921x_priv *priv)
> +{
> +	struct device *dev = to_device(priv);
> +	const struct yt921x_info *info;
> +	u8 extmode;
> +	u32 chipid;
> +	u32 major;
> +	u32 mode;
> +	int res;
> +
> +	res = yt921x_reg_read(priv, YT921X_CHIP_ID, &chipid);
> +	if (res)
> +		return res;
> +
> +	major = FIELD_GET(YT921X_CHIP_ID_MAJOR, chipid);
> +
> +	for (info = yt921x_infos; info->name; info++)
> +		if (info->major == major)
> +			goto found_major;
> +
> +	dev_err(dev, "Unexpected chipid 0x%x\n", chipid);
> +	return -ENODEV;
> +
> +found_major:
> +	res = yt921x_reg_read(priv, YT921X_CHIP_MODE, &mode);
> +	if (res)
> +		return res;
> +	res = yt921x_edata_read(priv, YT921X_EDATA_EXTMODE, &extmode);
> +	if (res)
> +		return res;
> +
> +	for (; info->name; info++)
> +		if (info->major == major && info->mode == mode &&
> +		    info->extmode == extmode)
> +			goto found_chip;
> +
> +	dev_err(dev, "Unsupported chipid 0x%x with chipmode 0x%x 0x%x\n",
> +		chipid, mode, extmode);
> +	return -ENODEV;
> +
> +found_chip:
> +	/* Print chipid here since we are interested in lower 16 bits */
> +	dev_info(dev,
> +		 "Motorcomm %s ethernet switch, chipid: 0x%x, "
> +		 "chipmode: 0x%x 0x%x\n",
> +		 info->name, chipid, mode, extmode);
> +
> +	priv->info = info;
> +	return 0;

The use of gotos here is backwards to normal. They are pretty much
only used in Linux to jump to the end to do cleanup on error. I don't
know of any other driver which uses goto on success. Please change
this.

> +	/* Register the internal mdio bus. Nodes for internal ports should have
> +	 * proper phy-handle pointing to their PHYs. Not enabling the internal
> +	 * bus is possible, though pretty wired, if internal ports are not used.

weird, not wired. 

    Andrew

---
pw-bot: cr