Date:   Sat, 25 Apr 2020 11:47:07 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Florinel Iordache <florinel.iordache@....com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, andrew@...n.ch,
        f.fainelli@...il.com, hkallweit1@...il.com,
        devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
        robh+dt@...nel.org, mark.rutland@....com, kuba@...nel.org,
        corbet@....net, shawnguo@...nel.org, leoyang.li@....com,
        madalin.bucur@....nxp.com, ioana.ciornei@....com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 6/9] net: phy: add backplane kr driver support

On Fri, Apr 24, 2020 at 03:46:28PM +0300, Florinel Iordache wrote:
> Add support for backplane kr generic driver including link training
> (ieee802.3ap/ba) and fixed equalization algorithm

This looks like it's still very much modelled on being a phylib driver,
which I thought we discussed shouldn't be the case.

Some further comments inline, but given the size of this patch I haven't
spent too long reviewing it.

> diff --git a/drivers/net/phy/backplane/Kconfig b/drivers/net/phy/backplane/Kconfig
> new file mode 100644
> index 0000000..9ec54b5
> --- /dev/null
> +++ b/drivers/net/phy/backplane/Kconfig
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
> +config ETH_BACKPLANE
> +	tristate "Ethernet Backplane support"
> +	depends on OF_MDIO
> +	help
> +	  This module provides driver support for Ethernet Operation over
> +	  Electrical Backplanes. It includes Backplane generic
> +	  driver including support for Link Training (IEEE802.3ap/ba).
> +	  Based on the link quality, a signal equalization is required.
> +	  The standard specifies that a start-up algorithm should be in place
> +	  in order to get the link up.
> +
> +config ETH_BACKPLANE_FIXED
> +	tristate "Fixed: No Equalization algorithm"
> +	depends on ETH_BACKPLANE
> +	help
> +	  This module provides a driver to setup fixed user configurable
> +	  coefficient values for backplanes equalization. This means
> +	  No Equalization algorithm is used to adapt the initial coefficients
> +	  initially set by the user.
> \ No newline at end of file

Please fix.

> +static u32 le_ioread32(void __iomem *reg)
> +{
> +	return ioread32(reg);
> +}
> +
> +static void le_iowrite32(u32 value, void __iomem *reg)
> +{
> +	iowrite32(value, reg);
> +}
> +
> +static u32 be_ioread32(void __iomem *reg)
> +{
> +	return ioread32be(reg);
> +}
> +
> +static void be_iowrite32(u32 value, void __iomem *reg)
> +{
> +	iowrite32be(value, reg);
> +}

Do these accessors really add any value - they just rename our existing
accessors, and therefore make the learning curve to understand this
code unnecessarily harder than it needs to be.

> +/* Read AN Link Status */
> +static int is_an_link_up(struct phy_device *phydev)
> +{
> +	struct backplane_device *bpdev = phydev->priv;
> +	int ret, val = 0;
> +
> +	mutex_lock(&bpdev->bpphy_lock);
> +
> +	/* Read twice because Link_Status is LL (Latched Low) bit */
> +	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> +	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);

What if an error occurs? while reading?

Do you care if the link went down momentarily?

> +/* backplane_write_mmd - Wrapper function for phy_write_mmd
> + * for writing a register on an MMD on a given PHY.
> + *
> + * Same rules as for phy_write_mmd();
> + */

Exported functions ought to have proper kerneldoc documentation.

> +int backplane_write_mmd(struct lane_device *lane, int devad, u32 regnum,
> +			u16 val)
> +{
> +	struct backplane_device *bpdev = lane->bpdev;
> +	struct phy_device *phydev = lane->phydev;
> +	int mdio_addr = phydev->mdio.addr;
> +	int err;
> +
> +	mutex_lock(&bpdev->bpphy_lock);
> +
> +	if (devad == MDIO_MMD_AN && backplane_is_multi_lane(bpdev)) {
> +		/* Multilane AN: prepare mdio address
> +		 * for writing phydev AN registers on respective lane
> +		 * AN MDIO address offset for multilane is equal
> +		 * to number of lanes
> +		 */
> +		phydev->mdio.addr = bpdev->num_lanes + lane->idx;
> +	}
> +
> +	err = phy_write_mmd(phydev, devad, regnum, val);
> +	if (err)
> +		bpdev_err(phydev,
> +			  "Writing PHY (%p) MMD = 0x%02x register = 0x%02x failed with error code: 0x%08x\n",
> +			  phydev, devad, regnum, err);
> +
> +	if (devad == MDIO_MMD_AN && backplane_is_multi_lane(bpdev)) {
> +		/* Multilane AN: restore mdio address */
> +		phydev->mdio.addr = mdio_addr;
> +	}
> +
> +	mutex_unlock(&bpdev->bpphy_lock);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(backplane_write_mmd);

Consider EXPORT_SYMBOL_GPL().

> +
> +/* backplane_read_mmd - Wrapper function for phy_read_mmd
> + * for reading a register from an MMD on a given PHY.
> + *
> + * Same rules as for phy_read_mmd();
> + */
> +int backplane_read_mmd(struct lane_device *lane, int devad, u32 regnum)
> +{
> +	struct backplane_device *bpdev = lane->bpdev;
> +	struct phy_device *phydev = lane->phydev;
> +	int mdio_addr = phydev->mdio.addr;
> +	int ret;
> +
> +	mutex_lock(&bpdev->bpphy_lock);
> +
> +	if (devad == MDIO_MMD_AN && backplane_is_multi_lane(bpdev)) {
> +		/* Multilane AN: prepare mdio address
> +		 * for reading phydev AN registers on respective lane
> +		 * AN MDIO address offset for multilane is equal to
> +		 * number of lanes
> +		 */
> +		phydev->mdio.addr = bpdev->num_lanes + lane->idx;
> +	}
> +
> +	ret = phy_read_mmd(phydev, devad, regnum);
> +
> +	if (devad == MDIO_MMD_AN && backplane_is_multi_lane(bpdev)) {
> +		/* Multilane AN: restore mdio address */
> +		phydev->mdio.addr = mdio_addr;
> +	}
> +
> +	mutex_unlock(&bpdev->bpphy_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(backplane_read_mmd);

I think the above two functions are coding implementation specifics into
what is trying to be a generic layer here.  What guarantee do we have
that all KR PHYs will live at consecutive addresses for each lane?

This brings me on to another issue - I thought we discussed the point of
reusing struct phy_device, and we strongly recommended against it ?
I think it would make more sense for struct lane_device to contain a
'struct mdio_device' or maybe a new 'struct pcs_mdio_device' which
points to the specific MDIO device for this lane.  That would have the
nice effect of avoiding the implementation specifics here.

> +bool backplane_is_mode_kr(phy_interface_t interface)
> +{
> +	return (interface >= PHY_INTERFACE_MODE_10GKR &&
> +		interface <= PHY_INTERFACE_MODE_40GKR4);

This really should not be here - you're reliant on no one
inappropriately adding between these enumeration values - but this code
is divorsed from its definition.  It should be in linux/phy.h to
complement things like phy_interface_mode_is_rgmii() et.al.

> +}
> +EXPORT_SYMBOL(backplane_is_mode_kr);
> +
> +bool backplane_is_valid_mode(phy_interface_t interface)
> +{
> +	return (interface >= PHY_INTERFACE_MODE_10GKR &&
> +		interface <= PHY_INTERFACE_MODE_40GKR4);

Same problem - it looks the same as backplane_is_mode_kr().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Powered by blists - more mailing lists