Message-ID: <ed7f291d-200f-0ba3-953d-cc4abf53b8c1@gmail.com>
Date:	Thu, 28 Jul 2016 10:35:05 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Raju Lakkaraju <Raju.Lakkaraju@...rosemi.com>,
	Andrew Lunn <andrew@...n.ch>
Cc:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Allan Nielsen <Allan.Nielsen@...rosemi.com>
Subject: Re: Microsemi VSC 8531/41 PHY Driver

On 07/27/2016 11:44 PM, Raju Lakkaraju wrote:
> Hello Andrew,
> 
> Thank you for given valuable comments.
> Please see the my responses inline.
> 
> Thanks,
> Raju
> 
> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@...n.ch] 
> Sent: Tuesday, July 26, 2016 6:14 PM
> To: Raju Lakkaraju
> Cc: netdev@...r.kernel.org; f.fainelli@...il.com; Allan Nielsen
> Subject: Re: Microsemi VSC 8531/41 PHY Driver
> 
> EXTERNAL EMAIL
> 
> 
>> +/* RGMII Rx Clock delay value change with board lay-out */ static u8 
>> +rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;
> 
> Doesn't this stop you from having a board with two PHYs with different layouts? You should be getting this value from the device tree.
> 
> Raju: As of now, RGMII Rx clock delay value should be 1.1 nsec as optimized/recommended value. 
> We tested on Beaglebone Black with VSC 8531 PHY.

That is true, until the next design with a PHY that does not need this
value and then, it will have to be adjusted.

> We would like to provide new function to configure correct/require value based on PHY layouts 
> alone with other RGMII configuration parameters as part of our next implementation.

You can either introduce a Device Tree property to allow boards to
specify what the correct delay(s) should be, or if the platform does not
use Device Tree, using phy_register_fixup_for_id would be acceptable for
that.

> 
>> +     phydev->supported = (SUPPORTED_1000baseT_Full |
>> +                          SUPPORTED_1000baseT_Half |
>> +                          SUPPORTED_100baseT_Full  |
>> +                          SUPPORTED_100baseT_Half  |
>> +                          SUPPORTED_10baseT_Full   |
>> +                          SUPPORTED_10baseT_Half   |
>> +                          SUPPORTED_Autoneg        |
>> +                          SUPPORTED_Pause          |
>> +                          SUPPORTED_Asym_Pause     |
>> +                          SUPPORTED_TP);

This is not necessary, your driver should advertise what the PHY is
capable of in phy_driver::features. The Ethernet MAC driver later should
be adjusting phydev->supported with what it actually support, there are
cases where you connect a 10/100Mbits MAC to a 1Gbits PHY, and you want
to properly restrict unsupported speeds.

>> +
>> +     phydev->speed = SPEED_1000;
>> +     phydev->duplex = DUPLEX_FULL;
>> +     phydev->pause = 0;
>> +     phydev->asym_pause = 0;
>> +     phydev->interface = PHY_INTERFACE_MODE_RGMII;
>> +     phydev->mdix = ETH_TP_MDI_AUTO;
> 
> Why are you setting all these? This is not normal, if you look at other drivers.
> 
> Raju: I would like to update the default values in software data structure (phydev). 
> Our PHY is 1G speed support device and RGMII supported device.

Whether RGMII is used as an interface/connection type between the MAC
and PHY is something that is within the consumer of the PHYLIB API
(typically Ethernet MAC/Switch driver), your PHY cannot enforce
anything, but the driver can check that the connection interface is sensble.

All of these default values that you are setting here will need to be
potentially changed by the state machine (link, duplex, pause) upon
reaction to link state changes, this change needs to be dropped.

> 
>> +
>> +     mutex_lock(&phydev->lock);
> 
> What are you locking against?
> 
> Raju: VSC 8531 has different PAGEs. Whenever MDC/MDIO access the PHY control registers, 
> first set the page number then read/write the register address. Default page should be Page 0.
> When I want to access not default page register, I have to lock phy device access and change 
> the page number and register access as atomic operation. 

Based on the execution context of this function, acquiring the mutex is
not necessary, the state machine has not started yet, so there cannot be
a conflicting PHY read which would end up changing the page selection.

[snip]

>> +
>> +static int vsc85xx_ack_interrupt(struct phy_device *phydev) {
>> +     int rc = 0;
>> +
>> +     if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
>> +             rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
>> +
>> +     return (rc < 0) ? rc : 0;
>> +}
>> +
>> +static int vsc85xx_config_intr(struct phy_device *phydev) {
>> +     int rc;
>> +
>> +     if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
>> +             rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
>> +                            MII_VSC85XX_INT_MASK_MASK);
>> +     } else {
>> +             rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
>> +             if (rc < 0)
>> +                     return rc;
> 
> And the purpose of this read is? I assume it clears an outstanding interrupt? If so, shouldn't you do it after disabling interrupts, not before? Otherwise you have a race condition.
> 
> Raju: The Interrupt status register is read on clean. When, PHY_INTERRUPT_DISABLE case, 
> I should make sure that status should be clear. If I read the Interrupt status registers, it clears all preexisting interrupts.

Should not you clear the interrupt status irrespective of whether PHY
interrupts will be enabled or not as a first operation? Then later on,
if the PHY needs to enable interrupt or not, this can be based on
PHY_INTERRUPT_ENABLED.
-- 
Florian

Powered by blists - more mailing lists