linux-kernel - Re: [PATCH 4/7] net: stmmac: introducing support for DWC xPCS logics

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190424134136.GO28405@lunn.ch>
Date:   Wed, 24 Apr 2019 15:41:36 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Weifeng Voon <weifeng.voon@...el.com>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Ong Boon Leong <boon.leong.ong@...el.com>,
        Kweh Hock Leong <hock.leong.kweh@...el.com>
Subject: Re: [PATCH 4/7] net: stmmac: introducing support for DWC xPCS logics

On Thu, Apr 25, 2019 at 01:17:18AM +0800, Weifeng Voon wrote:
> From: Ong Boon Leong <boon.leong.ong@...el.com>
> 
> xPCS is DWC Ethernet Physical Coding Sublayer that may be integrated
> into a GbE controller that uses DWC EQoS MAC controller. An example of
> HW configuration is shown below:-
> 
>   <-----------------GBE Controller---------->|<--External PHY chip-->
> 
>   +----------+         +----+    +---+               +--------------+
>   |   EQoS   | <-GMII->|xPCS|<-->|L1 | <-- SGMII --> | External GbE |
>   |   MAC    |         |    |    |PHY|               | PHY Chip     |
>   +----------+         +----+    +---+               +--------------+
>          ^               ^                                  ^
>          |               |                                  |
>          +---------------------MDIO-------------------------+
> 
> xPCS is a Clause-45 MDIO Manageable Device (MMD) and we need a way to
> differentiate it from external PHY chip that is discovered over MDIO.
> Therefore, xpcs_phy_addr is introduced in stmmac platform data
> (plat_stmmacenet_data) for differentiating xPCS from 'phy_addr' that
> belongs to external PHY.
> 
> Basic functionalities for initializing xPCS and configuring auto
> negotiation (AN), loopback, link status, AN advertisement and Link
> Partner ability are implemented.
> 
> xPCS interrupt handling for C37 AN complete is also implemented.
> 
> Tested-by: Kweh Hock Leong <hock.leong.kweh@...el.com>
> Reviewed-by: Chuah Kim Tatt <kim.tatt.chuah@...el.com>
> Reviewed-by: Voon Weifeng <weifeng.voon@...el.com>
> Reviewed-by: Kweh Hock Leong <hock.leong.kweh@...el.com>
> Reviewed-by: Baoli Zhang <baoli.zhang@...el.com>
> Signed-off-by: Ong Boon Leong <boon.leong.ong@...el.com>

You need your own signed-off-by here, since you are submitting it.

> ---
>  drivers/net/ethernet/stmicro/stmmac/dw_xpcs.h | 288 ++++++++++++++++++++++++++
>  drivers/net/ethernet/stmicro/stmmac/hwif.h    |  17 ++
>  include/linux/stmmac.h                        |   1 +
>  3 files changed, 306 insertions(+)
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/dw_xpcs.h
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dw_xpcs.h b/drivers/net/ethernet/stmicro/stmmac/dw_xpcs.h
> new file mode 100644
> index 0000000..446b714
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dw_xpcs.h
> @@ -0,0 +1,288 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* dw_xpcs.h: DWC Ethernet Physical Coding Sublayer Header
> + *
> + * Copyright (c) 2019, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */

Since you have a SPDX line, you don't need the license boilerplate.

> +#ifndef __DW_XPCS_H__
> +#define __DW_XPCS_H__
> +
> +#include <linux/mdio.h>
> +#include <linux/bitops.h>
> +#include "stmmac.h"
> +
> +/* XPCS Control & MII MMD Device Addresses */
> +#define XPCS_MDIO_CONTROL_MMD	MDIO_MMD_VEND1
> +#define XPCS_MDIO_MII_MMD	MDIO_MMD_VEND2
> +
> +/* Control MMD register offsets */
> +#define MDIO_CONTROL_MMD_CTRL		0x9	/* Control */
> +
> +/* Control MMD Control defines */
> +#define MDIO_CONTROL_MMD_CTRL_MII_MMD_EN	1 /* MII MMD Enable */
> +
> +/* MII MMD registers offsets */
> +#define MDIO_MII_MMD_CTRL		0x0	/* Control */
> +#define MDIO_MII_MMD_ADV		0x4	/* AN Advertisement */
> +#define MDIO_MII_MMD_LPA		0x5	/* Link Partner Ability */

MII_BMCR, MII_ADVERTISE, MII_LPA.

> +#define MDIO_MII_MMD_DIGITAL_CTRL_1	0x8000	/* Digital Control 1 */
> +#define MDIO_MII_MMD_AN_CTRL		0x8001	/* AN Control */
> +#define MDIO_MII_MMD_AN_STAT		0x8002	/* AN Status */
> +
> +/* MII MMD Control defines */
> +#define MDIO_MII_MMD_CTRL_ANE		BIT(12)	/* AN Enable */
> +#define MDIO_MII_MMD_CTRL_LBE		BIT(14)	/* Loopback Enable */
> +#define MDIO_MII_MMD_CTRL_RANE		BIT(9)	/* Restart AN */

BMCR_ANENABLE, BMCR_LOOPBACK, BMCR_ANENABLE

If you use the standard names, developers are going to find it easier
to understand the code.


> +/* MII MMD AN Advertisement & Link Partner Ability */
> +#define MDIO_MII_MMD_HD			BIT(6)	/* Half duplex */
> +#define MDIO_MII_MMD_FD			BIT(5)	/* Full duplex */
> +#define MDIO_MII_MMD_PSE_SHIFT		7	/* Pause Ability shift */
> +#define MDIO_MII_MMD_PSE	GENMASK(8, 7)	/* Pause Ability */
> +
> +/* MII MMD Digital Control 1 defines */
> +#define MDIO_MII_MMD_DIGI_CTRL_1_SGMII_PHY_MD	BIT(0) /* SGMII PHY mode */
> +
> +/* MII MMD AN Control defines */
> +#define MDIO_MII_MMD_AN_CTRL_TX_CONFIG_SHIFT	3 /* TX Config shift */
> +#define AN_CTRL_TX_CONF_PHY_SIDE_SGMII		0x1 /* PHY side SGMII mode */
> +#define AN_CTRL_TX_CONF_MAC_SIDE_SGMII		0x0 /* MAC side SGMII mode */
> +#define MDIO_MII_MMD_AN_CTRL_PCS_MD_SHIFT	1  /* PCS Mode shift */
> +#define MDIO_MII_MMD_AN_CTRL_PCS_MD	GENMASK(2, 1) /* PCS Mode */
> +#define AN_CTRL_PCS_MD_C37_1000BASEX	0x0	/* C37 AN for 1000BASE-X */
> +#define AN_CTRL_PCS_MD_C37_SGMII	0x2	/* C37 AN for SGMII */
> +#define MDIO_MII_MMD_AN_CTRL_AN_INTR_EN	BIT(0)	/* AN Complete Intr Enable */

> +
> +/* MII MMD AN Status defines for C37 AN SGMII Status */
> +#define AN_STAT_C37_AN_CMPLT		BIT(0)	/* AN Complete Intr */
> +#define AN_STAT_C37_AN_FD		BIT(1)	/* Full Duplex */
> +#define AN_STAT_C37_AN_SPEED_SHIFT	2	/* AN Speed shift */
> +#define AN_STAT_C37_AN_SPEED		GENMASK(3, 2)	/* AN Speed */
> +#define AN_STAT_C37_AN_10MBPS		0x0	/* 10 Mbps */
> +#define AN_STAT_C37_AN_100MBPS		0x1	/* 100 Mbps */
> +#define AN_STAT_C37_AN_1000MBPS		0x2	/* 1000 Mbps */
> +#define AN_STAT_C37_AN_LNKSTS		BIT(4)	/* Link Status */

Is these are standardized, not proprietary, consider adding them to
include/uapi/linux/mii.h so similar.

> +
> +/**
> + * dw_xpcs_init - To initialize xPCS
> + * @ndev: network device pointer
> + * @mode: PCS mode
> + * Description: this is to initialize xPCS
> + */
> +static inline void dw_xpcs_init(struct net_device *ndev, int pcs_mode)
> +{
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	int xpcs_phy_addr = priv->plat->xpcs_phy_addr;
> +
> +	/* Set SGMII PHY mode control */
> +	u16 phydata = (u16)mdiobus_read(priv->mii, xpcs_phy_addr,
> +				    (MII_ADDR_C45 |
> +				     (XPCS_MDIO_MII_MMD <<
> +				      MII_DEVADDR_C45_SHIFT) |
> +				     MDIO_MII_MMD_DIGITAL_CTRL_1));
> +

Why cast to u16? It return int for a reason.

> +	phydata |= MDIO_MII_MMD_DIGI_CTRL_1_SGMII_PHY_MD;
> +
> +	mdiobus_write(priv->mii, xpcs_phy_addr,
> +		      (MII_ADDR_C45 | (XPCS_MDIO_MII_MMD <<
> +		       MII_DEVADDR_C45_SHIFT) | MDIO_MII_MMD_DIGITAL_CTRL_1),
> +		      (int)phydata);

Maybe add helpers xpcs_read() and xpcs_write()?

      Andrew