lkml.org 
[lkml]   [2019]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 4/7] net: stmmac: introducing support for DWC xPCS logics
Date
>-----Original Message-----
>From: Andrew Lunn [mailto:andrew@lunn.ch]
>Sent: Wednesday, April 24, 2019 9:42 PM
>To: Voon, Weifeng <weifeng.voon@intel.com>
>Cc: David S. Miller <davem@davemloft.net>; netdev@vger.kernel.org; linux-
>kernel@vger.kernel.org; Ong, Boon Leong <boon.leong.ong@intel.com>;
>Kweh, Hock Leong <hock.leong.kweh@intel.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@intel.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@intel.com>
>> Reviewed-by: Chuah Kim Tatt <kim.tatt.chuah@intel.com>
>> Reviewed-by: Voon Weifeng <weifeng.voon@intel.com>
>> Reviewed-by: Kweh Hock Leong <hock.leong.kweh@intel.com>
>> Reviewed-by: Baoli Zhang <baoli.zhang@intel.com>
>> Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.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.
Thanks. Will edit.

>
>> +#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.
Ok. Will change.

>
>> +#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.
Ok. Will change.

>
>
>> +/* 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.

Yeah, it does look very standardized. However, per DW spec, they are
vendor-specific register set which uses MDIO_MMD_VEND2 to access.

>
>> +
>> +/**
>> + * 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.
Will change to u32 here.

>
>> + 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()?
Good suggestion. Thanks for your review.

>
> Andrew

\
 
 \ /
  Last update: 2019-04-25 03:46    [W:0.091 / U:27.716 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site