Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Nov 28, 2017 at 04:55:39PM -0800, David Daney wrote:
> +static int bgx_probe(struct platform_device *pdev)
> +{
> +	struct mac_platform_data platform_data;
> +	const __be32 *reg;
> +	u32 port;
> +	u64 addr;
> +	struct device_node *child;
> +	struct platform_device *new_dev;
> +	struct platform_device *pki_dev;
> +	int numa_node, interface;
> +	int i;
> +	int r = 0;
> +	char id[64];
> +	u64 data;
> +
> +	reg = of_get_property(pdev->dev.of_node, "reg", NULL);
> +	addr = of_translate_address(pdev->dev.of_node, reg);
> +	interface = (addr >> 24) & 0xf;
> +	numa_node = (addr >> 36) & 0x7;

Hi David

You have these two a few times in the code. Maybe add a helper to do
it? The NUMA one i assume could go somewhere in the SoC code?

> +static int bgx_mix_init_from_fdt(void)
> +{
> +	struct device_node	*node;
> +	struct device_node	*parent = NULL;
> +	int			mix = 0;

> +		/* Get the lmac index */
> +		reg = of_get_property(lmac_fdt_node, "reg", NULL);
> +		if (!reg)
> +			goto err;
> +
> +		mix_port_lmacs[mix].lmac = *reg;

I don't think of_get_property() deals with endianness. Is there any
danger of this driver being used on hardware with the other endianness
to what you have tested?

> +/**
> + * bgx_pki_init_from_param - Initialize the list of lmacs that connect to the
> + *			     pki from information in the "pki_port" parameter.
> + *
> + *			     The pki_port parameter format is as follows:
> + *			     pki_port=nbl
> + *			     where:
> + *				n = node
> + *				b = bgx
> + *				l = lmac
> + *
> + *			     Commas must be used to separate multiple lmacs:
> + *			     pki_port=000,100,110
> + *
> + *			     Asterisks (*) specify all possible characters in
> + *			     the subset:
> + *			     pki_port=00* (all lmacs of node0 bgx0).
> + *
> + *			     Missing lmacs identifiers default to all
> + *			     possible characters in the subset:
> + *			     pki_port=00 (all lmacs on node0 bgx0)
> + *
> + *			     Brackets ('[' and ']') specify the valid
> + *			     characters in the subset:
> + *			     pki_port=00[01] (lmac0 and lmac1 of node0 bgx0).
> + *
> + * Returns 0 if successful.
> + * Returns <0 for error codes.

I've not used kerneldoc much, but i suspect this is wrongly formated:

https://www.kernel.org/doc/html/v4.9/kernel-documentation.html#function-documentation

> +int bgx_port_ethtool_set_settings(struct net_device	*netdev,
> +				  struct ethtool_cmd	*cmd)
> +{
> +	struct bgx_port_priv *p = bgx_port_netdev2priv(netdev);
> +
> +	if (!capable(CAP_NET_ADMIN))
> +		return -EPERM;

Not required. The enforces this. See dev_ethtool()

> +
> +	if (p->phydev)
> +		return phy_ethtool_sset(p->phydev, cmd);
> +
> +	return -EOPNOTSUPP;
> +}
> +EXPORT_SYMBOL(bgx_port_ethtool_set_settings);
> +
> +int bgx_port_ethtool_nway_reset(struct net_device *netdev)
> +{
> +	struct bgx_port_priv *p = bgx_port_netdev2priv(netdev);
> +
> +	if (!capable(CAP_NET_ADMIN))
> +		return -EPERM;

Also not needed.

> +static void bgx_port_adjust_link(struct net_device *netdev)
> +{
> +	struct bgx_port_priv	*priv = bgx_port_netdev2priv(netdev);
> +	bool			link_changed = false;
> +	unsigned int		link;
> +	unsigned int		speed;
> +	unsigned int		duplex;
> +
> +	mutex_lock(&priv->lock);
> +
> +	if (!priv->phydev->link && priv->last_status.link)
> +		link_changed = true;
> +
> +	if (priv->phydev->link &&
> +	    (priv->last_status.link != priv->phydev->link ||
> +	     priv->last_status.duplex != priv->phydev->duplex ||
> +	     priv->last_status.speed != priv->phydev->speed))
> +		link_changed = true;
> +
> +	link = priv->phydev->link;
> +	priv->last_status.link = priv->phydev->link;
> +
> +	speed = priv->phydev->speed;
> +	priv->last_status.speed = priv->phydev->speed;
> +
> +	duplex = priv->phydev->duplex;
> +	priv->last_status.duplex = priv->phydev->duplex;
> +
> +	mutex_unlock(&priv->lock);
> +
> +	if (link_changed) {
> +		struct port_status status;
> +
> +		phy_print_status(priv->phydev);
> +
> +		status.link = link ? 1 : 0;
> +		status.duplex = duplex;
> +		status.speed = speed;
> +		if (!link) {
> +			netif_carrier_off(netdev);
> +			 /* Let TX drain. FIXME check that it is drained. */
> +			mdelay(50);
> +		}
> +		priv->set_link(priv, status);
> +		if (link)
> +			netif_carrier_on(netdev);

The code should do netif_carrier_on/off for you. See phy_link_change()

> +static void bgx_port_check_state(struct work_struct *work)
> +{
> +	struct bgx_port_priv	*priv;
> +	struct port_status	status;
> +
> +	priv = container_of(work, struct bgx_port_priv, dwork.work);
> +
> +	status = priv->get_link(priv);
> +
> +	if (!status.link &&
> +	    priv->mode != PORT_MODE_SGMII && priv->mode != PORT_MODE_RGMII)
> +		bgx_port_init_xaui_link(priv);
> +
> +	if (priv->last_status.link != status.link) {
> +		priv->last_status.link = status.link;
> +		if (status.link)
> +			netdev_info(priv->netdev, "Link is up - %d/%s\n",
> +				    status.speed,
> +				    status.duplex == DUPLEX_FULL ? "Full" : "Half");

You already have phy_print_status() in bgx_port_adjust_link(). Do you need this here?

> +		else
> +			netdev_info(priv->netdev, "Link is down\n");
> +	}
> +
> +	mutex_lock(&priv->lock);
> +	if (priv->work_queued)
> +		queue_delayed_work(check_state_wq, &priv->dwork, HZ);
> +	mutex_unlock(&priv->lock);
> +}
> +
> +int bgx_port_enable(struct net_device *netdev)
> +{


> +	} else {
> +		priv->phydev = of_phy_connect(netdev, priv->phy_np,
> +					      bgx_port_adjust_link, 0, priv->phy_mode);
> +		if (!priv->phydev)
> +			return -ENODEV;
> +
> +		netif_carrier_off(netdev);
> +
> +		if (priv->phydev)

You already checked this above.

> +			phy_start_aneg(priv->phydev);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(bgx_port_enable);
> +
> +int bgx_port_change_mtu(struct net_device *netdev, int new_mtu)
> +{
> +	struct bgx_port_priv *priv = bgx_port_netdev2priv(netdev);
> +	int max_frame;
> +
> +	if (new_mtu < 60 || new_mtu > 65392) {

See dev_set_mtu(). If you have done your initialisation correctly, this
won't happen.

> +static int bgx_port_probe(struct platform_device *pdev)
> +{
> +	switch (priv->mode) {
> +	case PORT_MODE_SGMII:
> +		if (priv->phy_np &&
> +		    priv->phy_mode != PHY_INTERFACE_MODE_SGMII)
> +			dev_warn(&pdev->dev, "SGMII phy mode mismatch.\n");
> +		goto set_link_functions;
> +	case PORT_MODE_RGMII:
> +		if (priv->phy_np &&
> +		    priv->phy_mode != PHY_INTERFACE_MODE_RGMII &&
> +		    priv->phy_mode != PHY_INTERFACE_MODE_RGMII_ID &&
> +		    priv->phy_mode != PHY_INTERFACE_MODE_RGMII_RXID &&
> +		    priv->phy_mode != PHY_INTERFACE_MODE_RGMII_TXID)

phy_interface_mode_is_rgmii()

More later, maybe.

     Andrew