[PATCH 0/2 net-next] phy: allow flagging a device as internal

[PATCH 0/2 net-next] phy: allow flagging a device as internal

[Florian Fainelli]

David, Andy,

This small patch set updates libphy to allow PHY drivers to flag a
specific PHY device as internal, which is then used to accurately
report the transceiver type (internal vs external) in ethtool.

As far as I can tell we only have one in-tree driver for the moment
which is known to be for internal PHYs.

Florian Fainelli (2):
  phy: allow drivers to flag a PHY device as internal
  phy: bcm63xx: report Broadcom BCM63xx PHYs as internal

 drivers/net/phy/bcm63xx.c    | 4 ++--
 drivers/net/phy/phy.c        | 2 +-
 drivers/net/phy/phy_device.c | 3 +++
 include/linux/phy.h          | 3 +++
 4 files changed, 9 insertions(+), 3 deletions(-)

-- 
1.8.1.2

[PATCH 1/2 net-next] phy: allow drivers to flag a PHY device as internal

[Florian Fainelli]

libphy currently always reports a PHY as an external transceiver from
the ethtool output. This is accurate, because some drivers should be
able to tell that a PHY device is an internal transceiver of an Ethernet
MAC. Add a new flag (PHY_IS_INTERNAL) which can be set by PHY drivers
just like other flags, and which is used by the PHY ethtool code to
report it to userspace.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy.c        | 2 +-
 drivers/net/phy/phy_device.c | 3 +++
 include/linux/phy.h          | 3 +++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2d28a0e..bcfcca1 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -294,7 +294,7 @@ int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd)
 	cmd->duplex = phydev->duplex;
 	cmd->port = PORT_MII;
 	cmd->phy_address = phydev->addr;
-	cmd->transceiver = XCVR_EXTERNAL;
+	cmd->transceiver = phydev->is_internal ? XCVR_INTERNAL : XCVR_EXTERNAL;
 	cmd->autoneg = phydev->autoneg;
 
 	return 0;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index b55aa33..74630e9 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1017,6 +1017,9 @@ static int phy_probe(struct device *dev)
 			phy_interrupt_is_valid(phydev))
 		phydev->irq = PHY_POLL;
 
+	if (phydrv->flags & PHY_IS_INTERNAL)
+		phydev->is_internal = true;
+
 	mutex_lock(&phydev->lock);
 
 	/* Start out supporting everything. Eventually,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fdfa115..271d623 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -49,6 +49,7 @@
 
 #define PHY_HAS_INTERRUPT	0x00000001
 #define PHY_HAS_MAGICANEG	0x00000002
+#define PHY_IS_INTERNAL		0x00000004
 
 /* Interface Mode definitions */
 typedef enum {
@@ -261,6 +262,7 @@ struct phy_c45_device_ids {
  * phy_id: UID for this device found during discovery
  * c45_ids: 802.3-c45 Device Identifers if is_c45.
  * is_c45:  Set to true if this phy uses clause 45 addressing.
+ * is_internal: Set to true if this phy is internal to a MAC.
  * state: state of the PHY for management purposes
  * dev_flags: Device-specific flags used by the PHY driver.
  * addr: Bus address of PHY
@@ -298,6 +300,7 @@ struct phy_device {
 
 	struct phy_c45_device_ids c45_ids;
 	bool is_c45;
+	bool is_internal;
 
 	enum phy_state state;
 
-- 
1.8.1.2

[PATCH 2/2 net-next] phy: bcm63xx: report Broadcom BCM63xx PHYs as internal

[Florian Fainelli]

The Broadcom BCM63xx PHY driver is for the SoC internal PHYs, flag these
as internal PHY devices.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/bcm63xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/bcm63xx.c b/drivers/net/phy/bcm63xx.c
index 84c7a39..ac55b08 100644
--- a/drivers/net/phy/bcm63xx.c
+++ b/drivers/net/phy/bcm63xx.c
@@ -78,7 +78,7 @@ static struct phy_driver bcm63xx_driver[] = {
 	.name		= "Broadcom BCM63XX (1)",
 	/* ASYM_PAUSE bit is marked RO in datasheet, so don't cheat */
 	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
-	.flags		= PHY_HAS_INTERRUPT,
+	.flags		= PHY_HAS_INTERRUPT | PHY_IS_INTERNAL,
 	.config_init	= bcm63xx_config_init,
 	.config_aneg	= genphy_config_aneg,
 	.read_status	= genphy_read_status,
@@ -91,7 +91,7 @@ static struct phy_driver bcm63xx_driver[] = {
 	.phy_id_mask	= 0xfffffc00,
 	.name		= "Broadcom BCM63XX (2)",
 	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
-	.flags		= PHY_HAS_INTERRUPT,
+	.flags		= PHY_HAS_INTERRUPT | PHY_IS_INTERNAL,
 	.config_init	= bcm63xx_config_init,
 	.config_aneg	= genphy_config_aneg,
 	.read_status	= genphy_read_status,
-- 
1.8.1.2

Re: [PATCH 0/2 net-next] phy: allow flagging a device as internal

[Ben Hutchings]

On Tue, 2013-05-21 at 11:37 +0100, Florian Fainelli wrote:
> David, Andy,
> 
> This small patch set updates libphy to allow PHY drivers to flag a
> specific PHY device as internal, which is then used to accurately
> report the transceiver type (internal vs external) in ethtool.
> 
> As far as I can tell we only have one in-tree driver for the moment
> which is known to be for internal PHYs.

I don't think you should make any change like this until there is proper
documentation of what the 'transceiver' field actually means.

It appears that XCVR_EXTERNAL was originally used for a transceiver
module external to the system, usually connected to an AUI port.  The
modern equivalent of that might be SFP+ and similar module slots, but
they're a bit different because the Physical Coding Sublayer is usually
part of the controller.

Many newer drivers are using XCVR_EXTERNAL to indicate that the PHY is a
separate package from the controller, which is what you seem to be doing
here.  But nowhere is that specified!

The transceiver field doesn't even really matter much unless the user
has a choice of which to use.  Does anyone make boards like that any
more?  And it's probably entirely redundant with the port and
phy_address fields in that case, anyway.

Ben.

> Florian Fainelli (2):
>   phy: allow drivers to flag a PHY device as internal
>   phy: bcm63xx: report Broadcom BCM63xx PHYs as internal
> 
>  drivers/net/phy/bcm63xx.c    | 4 ++--
>  drivers/net/phy/phy.c        | 2 +-
>  drivers/net/phy/phy_device.c | 3 +++
>  include/linux/phy.h          | 3 +++
>  4 files changed, 9 insertions(+), 3 deletions(-)
> 

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 0/2 net-next] phy: allow flagging a device as internal

[Fleming Andy-AFLEMING]

On May 21, 2013, at 11:45, "Ben Hutchings" <bhutchings@solarflare.com> wrote:

> On Tue, 2013-05-21 at 11:37 +0100, Florian Fainelli wrote:
>> David, Andy,
>> 
>> This small patch set updates libphy to allow PHY drivers to flag a
>> specific PHY device as internal, which is then used to accurately
>> report the transceiver type (internal vs external) in ethtool.
>> 
>> As far as I can tell we only have one in-tree driver for the moment
>> which is known to be for internal PHYs.
> 
> I don't think you should make any change like this until there is proper
> documentation of what the 'transceiver' field actually means.
> 
> It appears that XCVR_EXTERNAL was originally used for a transceiver
> module external to the system, usually connected to an AUI port.  The
> modern equivalent of that might be SFP+ and similar module slots, but
> they're a bit different because the Physical Coding Sublayer is usually
> part of the controller.
> 
> Many newer drivers are using XCVR_EXTERNAL to indicate that the PHY is a
> separate package from the controller, which is what you seem to be doing
> here.  But nowhere is that specified!
> 
> The transceiver field doesn't even really matter much unless the user
> has a choice of which to use.  Does anyone make boards like that any
> more?  And it's probably entirely redundant with the port and
> phy_address fields in that case, anyway.


Alas, yes. We have a 10G card that can switch between copper and fiber. But I agree that the field is poorly-defined.

Andy

Re: [PATCH 0/2 net-next] phy: allow flagging a device as internal

[Ben Hutchings]

On Tue, 2013-05-21 at 18:25 +0100, Florian Fainelli wrote:
> 2013/5/21 Ben Hutchings <bhutchings@solarflare.com>
>         On Tue, 2013-05-21 at 11:37 +0100, Florian Fainelli wrote:
>         > David, Andy,
>         >
>         > This small patch set updates libphy to allow PHY drivers to
>         flag a
>         > specific PHY device as internal, which is then used to
>         accurately
>         > report the transceiver type (internal vs external) in
>         ethtool.
>         >
>         > As far as I can tell we only have one in-tree driver for the
>         moment
>         > which is known to be for internal PHYs.
>         
>         
>         I don't think you should make any change like this until there
>         is proper
>         documentation of what the 'transceiver' field actually means.
>         
>         It appears that XCVR_EXTERNAL was originally used for a
>         transceiver
>         module external to the system, usually connected to an AUI
>         port.  The
>         modern equivalent of that might be SFP+ and similar module
>         slots, but
>         they're a bit different because the Physical Coding Sublayer
>         is usually
>         part of the controller.
>         
>         Many newer drivers are using XCVR_EXTERNAL to indicate that
>         the PHY is a
>         separate package from the controller, which is what you seem
>         to be doing
>         here.  But nowhere is that specified!
> 
> 
> Well, it depends how you define "specified", I can see quite a lot of
> drivers actually defining it to match the definition you gave above,
> so the de facto situation is like you describe it.

I would like to see someone (you?) provide a longer comment in ethtool.h
that says what XCVR_INTERNAL and XCVR_EXTERNAL are supposed to mean (the
other values are obviously driver-specific so don't matter as much).

>         The transceiver field doesn't even really matter much unless
>         the user
>         has a choice of which to use. 
> 
> 
> This is about reporting accurate information, the field is not
> user-changeable using libphy's ethtool knobs.

The information cannot be accurate without also being clear.

>         Does anyone make boards like that any
>         more?  And it's probably entirely redundant with the port and
>         phy_address fields in that case, anyway.
> 
> 
> At least I need to know from my driver if the PHY is an internal one
> (package perspective) or not because there is a different
> configuration path depending on this. I could just drop the hunk which
> sets cmd->transceiver then.

I think that makes more sense.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.