Message ID | 58231E6C.2040603@laposte.net |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On 11/09/2016 05:02 AM, Sebastian Frias wrote: > On 11/04/2016 05:49 PM, Måns Rullgård wrote: >>>> But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers >>>> will apply the delay. >>>> >>>> I think a better way of dealing with this is that both, PHY and MAC >>>> drivers exchange information so that the delay is applied only once. >>> >>> Exchange what information? The PHY device interface (phydev->interface) >>> conveys the needed information for both entities. >> >> There doesn't seem to be any consensus among the drivers regarding where >> the delay should be applied. Since only a few drivers, MAC or PHY, act >> on this property, most combinations still work by chance. It is common >> for boards to set the delay at the PHY using external config pins so no >> software setup is required (although I have one Sigma based board that >> gets this wrong). I suspect if drivers/net/ethernet/broadcom/genet were >> used with one of the four PHY drivers that also set the delay based on >> this DT property, things would go wrong. >> > > Exactly, what about a patch like (I can make a formal submission, even > merge it with the patch discussed in this thread, consider this a RFC): I really don't see a point in doing this when we can just clarify what phydev->interface does and already have the knowledge that we need without introducing additional flags in the phy driver. > > diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c > index fba2699..4217ff4 100644 > --- a/drivers/net/ethernet/aurora/nb8800.c > +++ b/drivers/net/ethernet/aurora/nb8800.c > @@ -1283,6 +1283,10 @@ static int nb8800_tangox_init(struct net_device *dev) > case PHY_INTERFACE_MODE_RGMII_RXID: > case PHY_INTERFACE_MODE_RGMII_TXID: > pad_mode = PAD_MODE_RGMII; > + > + if ((dev->phydev->flags & PHY_SUPPORTS_TXID) == 0) > + pad_mode |= PAD_MODE_GTX_CLK_DELAY; > + > break; > > default: > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index 2e0c759..5eddb04 100644 > --- a/drivers/net/phy/at803x.c > +++ b/drivers/net/phy/at803x.c > @@ -426,7 +426,9 @@ static int at803x_aneg_done(struct phy_device *phydev) > .suspend = at803x_suspend, > .resume = at803x_resume, > .features = PHY_GBIT_FEATURES, > - .flags = PHY_HAS_INTERRUPT, > + .flags = PHY_HAS_INTERRUPT | > + PHY_SUPPORTS_RXID | > + PHY_SUPPORTS_TXID, > .config_aneg = genphy_config_aneg, > .read_status = genphy_read_status, > .ack_interrupt = at803x_ack_interrupt, > diff --git a/include/linux/phy.h b/include/linux/phy.h > index e7e1fd3..0f0b17e 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -61,6 +61,8 @@ > #define PHY_HAS_INTERRUPT 0x00000001 > #define PHY_HAS_MAGICANEG 0x00000002 > #define PHY_IS_INTERNAL 0x00000004 > +#define PHY_SUPPORTS_RXID 0x00000008 > +#define PHY_SUPPORTS_TXID 0x00000010 > #define MDIO_DEVICE_IS_PHY 0x80000000 > > /* Interface Mode definitions */ >
On 11/09/2016 06:03 PM, Florian Fainelli wrote: > On 11/09/2016 05:02 AM, Sebastian Frias wrote: >> On 11/04/2016 05:49 PM, Måns Rullgård wrote: >>>>> But when doing so, both the Atheros 8035 and the Aurora NB8800 drivers >>>>> will apply the delay. >>>>> >>>>> I think a better way of dealing with this is that both, PHY and MAC >>>>> drivers exchange information so that the delay is applied only once. >>>> >>>> Exchange what information? The PHY device interface (phydev->interface) >>>> conveys the needed information for both entities. >>> >>> There doesn't seem to be any consensus among the drivers regarding where >>> the delay should be applied. Since only a few drivers, MAC or PHY, act >>> on this property, most combinations still work by chance. It is common >>> for boards to set the delay at the PHY using external config pins so no >>> software setup is required (although I have one Sigma based board that >>> gets this wrong). I suspect if drivers/net/ethernet/broadcom/genet were >>> used with one of the four PHY drivers that also set the delay based on >>> this DT property, things would go wrong. >>> >> >> Exactly, what about a patch like (I can make a formal submission, even >> merge it with the patch discussed in this thread, consider this a RFC): > > I really don't see a point in doing this when we can just clarify what > phydev->interface does and already have the knowledge that we need > without introducing additional flags in the phy driver. > Ok, so who can clarify what "phydev->interface" does, especially in the context of this discussion? What happens when a TX delay must be applied and: - both the PHY and the MAC support the delay - only the PHY supports the delay - only the MAC supports the delay Best regards, Sebastian
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c index fba2699..4217ff4 100644 --- a/drivers/net/ethernet/aurora/nb8800.c +++ b/drivers/net/ethernet/aurora/nb8800.c @@ -1283,6 +1283,10 @@ static int nb8800_tangox_init(struct net_device *dev) case PHY_INTERFACE_MODE_RGMII_RXID: case PHY_INTERFACE_MODE_RGMII_TXID: pad_mode = PAD_MODE_RGMII; + + if ((dev->phydev->flags & PHY_SUPPORTS_TXID) == 0) + pad_mode |= PAD_MODE_GTX_CLK_DELAY; + break; default: diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index 2e0c759..5eddb04 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -426,7 +426,9 @@ static int at803x_aneg_done(struct phy_device *phydev) .suspend = at803x_suspend, .resume = at803x_resume, .features = PHY_GBIT_FEATURES, - .flags = PHY_HAS_INTERRUPT, + .flags = PHY_HAS_INTERRUPT | + PHY_SUPPORTS_RXID | + PHY_SUPPORTS_TXID, .config_aneg = genphy_config_aneg, .read_status = genphy_read_status, .ack_interrupt = at803x_ack_interrupt, diff --git a/include/linux/phy.h b/include/linux/phy.h index e7e1fd3..0f0b17e 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -61,6 +61,8 @@ #define PHY_HAS_INTERRUPT 0x00000001 #define PHY_HAS_MAGICANEG 0x00000002 #define PHY_IS_INTERNAL 0x00000004 +#define PHY_SUPPORTS_RXID 0x00000008 +#define PHY_SUPPORTS_TXID 0x00000010 #define MDIO_DEVICE_IS_PHY 0x80000000 /* Interface Mode definitions */