| 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
| ||
|
Message-ID: <673edcd1-9ea5-9efe-1f66-3f8ea8d3f092@gmail.com>
Date: Sun, 24 Feb 2019 18:31:01 -0800
From: Florian Fainelli <f.fainelli@...il.com>
To: Parshuram Thombare <pthombar@...ence.com>,
nicolas.ferre@...rochip.com, davem@...emloft.net,
netdev@...r.kernel.org, andrew@...n.ch, hkallweit1@...il.com,
linux-kernel@...r.kernel.org, rafalc@...ence.com,
piotrs@...ence.com, jank@...ence.com
Subject: Re: [PATCH 1/3] net: ethernet: add support for PCS and 2.5G speed
Le 2/22/19 à 12:12 PM, Parshuram Thombare a écrit :
> This patch add support for PCS (for SGMII interface) and 2.5Gbps MAC
> in Cadence ethernet controller driver.
At a high level you don't seem to be making use of PHYLINK so which
2.5Gbps interfaces do you actually support?
>
> Signed-off-by: Parshuram Thombare <pthombar@...ence.com>
> ---
[snip]
> @@ -361,26 +361,50 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> * macb_set_tx_clk() - Set a clock to a new frequency
> * @clk Pointer to the clock to change
> * @rate New frequency in Hz
> + * @interafce Phy interface
Typo: @interface and this is an unrelated change.
> * @dev Pointer to the struct net_device
> */
> -static void macb_set_tx_clk(struct clk *clk, int speed, struct net_device *dev)
> +static void macb_set_tx_clk(struct clk *clk, int speed,
> + phy_interface_t interface, struct net_device *dev)
> {
> long ferr, rate, rate_rounded;
>
> if (!clk)
> return;
>
> - switch (speed) {
> - case SPEED_10:
> + if (interface == PHY_INTERFACE_MODE_GMII ||
> + interface == PHY_INTERFACE_MODE_MII) {
> + switch (speed) {
> + case SPEED_10:> rate = 2500000;
You need to add one tab to align rate and break.
> break;
> - case SPEED_100:
> + case SPEED_100:
> rate = 25000000;
> break;
> - case SPEED_1000:
> + case SPEED_1000:
> rate = 125000000;
> break;
> - default:
> + default:
> + return;
> + }
> + } else if (interface == PHY_INTERFACE_MODE_SGMII) {
> + switch (speed) {
> + case SPEED_10:
> + rate = 1250000;
> + break;
> + case SPEED_100:
> + rate = 12500000;
> + break;
> + case SPEED_1000:
> + rate = 125000000;
> + break;
> + case SPEED_2500:
> + rate = 312500000;
> + break;
> + default:
> + return;
The indentation is broken here and you can greatly simplify this with a
simple function that returns speed * 1250 and does an initial check for
unsupported speeds.
> + }
> + } else {
> return;
> }
>
> @@ -410,30 +434,49 @@ static void macb_handle_link_change(struct net_device *dev)
>
> spin_lock_irqsave(&bp->lock, flags);
>
> - if (phydev->link) {
> - if ((bp->speed != phydev->speed) ||
> - (bp->duplex != phydev->duplex)) {
> - u32 reg;
> -
> - reg = macb_readl(bp, NCFGR);
> - reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
> - if (macb_is_gem(bp))
> - reg &= ~GEM_BIT(GBE);
> + if (phydev->link && (bp->speed != phydev->speed ||
> + bp->duplex != phydev->duplex)) {
> + u32 reg;
>
> - if (phydev->duplex)
> - reg |= MACB_BIT(FD);
> + reg = macb_readl(bp, NCFGR);
> + reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
> + if (macb_is_gem(bp))
> + reg &= ~GEM_BIT(GBE);
> + if (phydev->duplex)
> + reg |= MACB_BIT(FD);
> + macb_or_gem_writel(bp, NCFGR, reg);
> +
> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII &&
> + (phydev->speed == SPEED_1000 ||
> + phydev->speed == SPEED_2500)) {
> + if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED) {
> + reg = gem_readl(bp, NCR) &
> + ~GEM_BIT(TWO_PT_FIVE_GIG);
> + gem_writel(bp, NCR, reg);
> + }
If you are making correct use of the capabilities then there is no point
in re-checking them here. If you allowed the MAC to advertise 2.5Gbps
then it is de-facto SGMII capable.
> + gem_writel(bp, NCFGR, GEM_BIT(GBE) |
> + gem_readl(bp, NCFGR));
> + if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED &&
> + phydev->speed == SPEED_2500)
> + gem_writel(bp, NCR, gem_readl(bp, NCR) |
> + GEM_BIT(TWO_PT_FIVE_GIG));
> + } else if (phydev->speed == SPEED_1000) {
> + gem_writel(bp, NCFGR, GEM_BIT(GBE) |
> + gem_readl(bp, NCFGR));
> + } else {
> + if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> + reg = gem_readl(bp, NCFGR);
> + reg &= ~(GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
> + gem_writel(bp, NCFGR, reg);
> + }
> if (phydev->speed == SPEED_100)
> - reg |= MACB_BIT(SPD);
> - if (phydev->speed == SPEED_1000 &&
> - bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
> - reg |= GEM_BIT(GBE);
> -
> - macb_or_gem_writel(bp, NCFGR, reg);
> -
> - bp->speed = phydev->speed;
> - bp->duplex = phydev->duplex;
> - status_change = 1;
> + macb_writel(bp, NCFGR, MACB_BIT(SPD) |
> + macb_readl(bp, NCFGR));
> }
There is a lot of repetition while setting the GBE bit which always set
based on speed == 1000 irrespective of the mode, so take that part out
of the if () else if () else () clauses.
> +
> + bp->speed = phydev->speed;
> + bp->duplex = phydev->duplex;
> + status_change = 1;
> }
>
> if (phydev->link != bp->link) {
> @@ -453,7 +496,8 @@ static void macb_handle_link_change(struct net_device *dev)
> /* Update the TX clock rate if and only if the link is
> * up and there has been a link change.
> */
> - macb_set_tx_clk(bp->tx_clk, phydev->speed, dev);
> + macb_set_tx_clk(bp->tx_clk, phydev->speed,
> + bp->phy_interface, dev);
>
> netif_carrier_on(dev);
> netdev_info(dev, "link up (%d/%s)\n",
> @@ -543,10 +587,16 @@ static int macb_mii_probe(struct net_device *dev)
> }
>
> /* mask with MAC supported features */
> - if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
> - phy_set_max_speed(phydev, SPEED_1000);
> - else
> - phy_set_max_speed(phydev, SPEED_100);
> + if (macb_is_gem(bp)) {
You have changed the previous logic that also checked for
MACB_CAPS_GIGABIT_MODE_AVAILABLE, why?
> + linkmode_copy(phydev->supported, PHY_GBIT_FEATURES);
> + if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED)
> + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> + phydev->supported);
> + } else {
> + linkmode_copy(phydev->supported, PHY_BASIC_FEATURES);
> + }
> +
> + linkmode_copy(phydev->advertising, phydev->supported);
>
> if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
> phy_remove_link_mode(phydev,
> @@ -2217,8 +2267,6 @@ static void macb_init_hw(struct macb *bp)
> macb_set_hwaddr(bp);
>
> config = macb_mdc_clk_div(bp);
> - if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
> - config |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
> config |= MACB_BF(RBOF, NET_IP_ALIGN); /* Make eth data aligned */
> config |= MACB_BIT(PAE); /* PAuse Enable */
> config |= MACB_BIT(DRFCS); /* Discard Rx FCS */
> @@ -3255,6 +3303,23 @@ static void macb_configure_caps(struct macb *bp,
> dcfg = gem_readl(bp, DCFG1);
> if (GEM_BFEXT(IRQCOR, dcfg) == 0)
> bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE;
> + if (GEM_BFEXT(NO_PCS, dcfg) == 0)
> + bp->caps |= MACB_CAPS_PCS;
> + switch (MACB_BFEXT(IDNUM, macb_readl(bp, MID))) {
> + case MACB_GEM7016_IDNUM:
> + case MACB_GEM7017_IDNUM:
> + case MACB_GEM7017A_IDNUM:
> + case MACB_GEM7020_IDNUM:
> + case MACB_GEM7021_IDNUM:
> + case MACB_GEM7021A_IDNUM:
> + case MACB_GEM7022_IDNUM:
> + if (bp->caps & MACB_CAPS_PCS)
> + bp->caps |= MACB_CAPS_TWO_PT_FIVE_GIG_SPEED;
> + break;
> +
> + default:
> + break;
> + }
> dcfg = gem_readl(bp, DCFG2);
> if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
> bp->caps |= MACB_CAPS_FIFO_MODE;
> @@ -4110,7 +4175,28 @@ static int macb_probe(struct platform_device *pdev)
> else
> bp->phy_interface = PHY_INTERFACE_MODE_MII;
> } else {
> + switch (err) {
> + case PHY_INTERFACE_MODE_SGMII:
> + if (bp->caps & MACB_CAPS_PCS) {
> + bp->phy_interface = PHY_INTERFACE_MODE_SGMII;
> + break;
> + }
If SGMII was selected on a version of the IP that does not support it,
then falling back to GMII or MII does not sound correct, this is a hard
error that must be handled as such.
--
Florian
Powered by blists - more mailing lists