$subject should begin with "net: macb: "
Parshuram,
Sorry but NACK on the series.
David,
This patch series seem pretty intrusive, so I would like that you wait
for my explicit ACK before applying even next versions of it.
More comments below...
On 25/02/2019 at 18:21, Florian Fainelli wrote:
> On 2/25/19 1:11 AM, Parshuram Raju Thombare wrote:
>>> 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.
>
> I should have asked this more clearly: have you tested with SFP modules
> for instance? If you want to be able to reliably support 2500baseT
> and/or 2500baseX with hot plugging of such modules, you need to
> implement PHYLINK for that network driver, there is no other way around.
>
>>
>>>>
>>>> Signed-off-by: Parshuram Thombare <pthom...@cadence.com>
>>>> ---
>>>
[..]
>>>>
>>>> - 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.
>
> It should look like this:
>
> switch (cond) {
> case cond1:
> do_something();
> break;
>
> etc.
Read the coding style documentation, everything is well explained there.
>>>> 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;
>>>> }
[..]
>>>> 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 ?
We use (ID >= 0x2) and only 10/100 mode on sama5d4/sama5d2 for more than
5 years, as seen in their respective struct macb_config, weren't you aware?
> Well, if anyone would know, it would be you, I don't work for Cadence
> nor have access to the internal IP documentation.
I agree with Florian, and what you modify makes me feel that the rest of
the patch might break my use of the Cadence IP in my products. That's
not a good sign, to say the least.
I'm expecting that Cadence don't break software used by their customers
on products already deployed on the field.
For next series, I would like that you report that you actually tested
your changes on older IP revisions and that non-regression tests are
passed successfully for some of the long time users of this IP (namely
Microchip/Atmel and Xilinx products).
>>>> + 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 ?
>
> This is probably not going to help anyone, imagine you incorrectly
> specified a 'phy-mode' property in the Device Tree and you have to dig
> into the driver in the function that sets the clock rate to find out
> what you just defaulted to 1Gbits/GMII for instance. This is not user
> friendly at all and will increase the support burden on your end as
> well, if SGMII is specified and the IP does not support it, detect it
> and return an error, how that propagates, either as a probe failure or
> the inability to open the network device, your call.
>
Best regards,
--
Nicolas Ferre
