>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?
>
New ethernet controller have MAC which support 2.5G speed.
Also there is addition of PCS and SGMII interface.
>>
>> Signed-off-by: Parshuram Thombare <pthom...@cadence.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.
Do you mean a tab each for rate and break lines ?
All switch statements are aligned at a tab. I am not sure how does case and
rate got on same line.
>
>> 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.
>
I ran checkpatch.pl and all indentation issues were cleared. But I think having
function
is better option, I will make that change.
>> + }
>> + } 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.
PHY_INTERFACE_MODE_SGMII is selected only on the basis of presence of PCS.
This additional check is to make sure PHY also support 1G/2.5G.
>> + 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.
>
Ok, I will change it.
>> +
>> + 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?
My understanding is all GEM (ID >= 0x2) support GIGABIT mode.
Was there any other reason for this check ?
>> + 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
My intention was to continue (instead of failing) with whatever functionality
is available.
Can we have some error message and continue with what is available ?
Regards,
Parshuram Thombare
