[RFC PATCH net-next 0/8] net: dsa: New registration API

[RFC PATCH net-next 0/8] net: dsa: New registration API

[Florian Fainelli]

Hi all,

This patch series is the long promised API rework to allow registering DSA
switch as PCI, platform, SPI, or PHY drivers, and this is pretty much the
grand plan described here:
http://permalink.gmane.org/gmane.linux.network/329848

Note that unless appropriate Device Tree or platform_data configuration is
changed, this is not conflicting with the current approach of registering DSA
switches using the special "dsa" platform device/driver. Rather this introduces
a parallel API which allows drivers to be converted and tested one by one.

This has been tested on the following two systems:

- Linksys/Cisco EA4500 which has a MV88E6171 at MDIO address 16
- BCM7445 reference board with the on-chip Starfighter 2 switch (MMIO)

Note that there are currenlty no incompatibles changes made to existing Device
Tree sources, rather, depending on the bus we are probed for, e.g: MDIO
the dsa,mii-bus and dsa,ethernet phandles and first cell of the  "reg" property
will become obsolete, everything else remains entirely compatible. This is all
appearing in patches.

Known issues:

- the new API probably does not work with multiple switches in a tree
  since I don't have such a system, I would welcome testing on these
  particular setups and would help take care of the problems, this is
  currently the biggest limitation I see

- there is a bit of duplication and special casing now, but that is
  mostly due to having to maintain two different ways of registering DSA
  drivers, this will eventually go away

- on my EA4500, doing PHY-driver registration slows things down dramatically
  iperf switches from ~700Mbits/sec to ~200Mbits/sec, not sure what's wrong
  here, could be some sort of RGMII setting going wrong

These changes can be pulled from my tree here:

https://github.com/ffainelli/linux/tree/dsa-model-proposal

Happy testing!

Florian Fainelli (8):
  net: dsa: Move dsa_switch_tree final setup in separate function
  net: phy: Check fixup lists in get_phy_device()
  net: phy: Allow PHY devices to identify themselves as Ethernet
    switches
  net: mv643xx_eth: Handle Ethernet switches as PHY devices
  net: dsa: add new API to register switch devices
  net: dsa: bcm_sf2: make it a real platform driver
  net: dsa: mv88e6060: make it a proper PHY driver
  net: dsa: mv88e6xxx: Allow them to be proper PHY drivers

 drivers/net/dsa/bcm_sf2.c                  | 239 +++++++++++++++--------------
 drivers/net/dsa/mv88e6060.c                | 114 +++++++++++++-
 drivers/net/dsa/mv88e6123_61_65.c          | 116 ++++++++++++++
 drivers/net/dsa/mv88e6131.c                | 107 +++++++++++++
 drivers/net/dsa/mv88e6171.c                |  95 ++++++++++++
 drivers/net/dsa/mv88e6352.c                | 102 ++++++++++++
 drivers/net/dsa/mv88e6xxx.c                |  40 ++++-
 drivers/net/dsa/mv88e6xxx.h                |  12 ++
 drivers/net/ethernet/marvell/mv643xx_eth.c |   4 +-
 drivers/net/phy/phy_device.c               |   9 +-
 include/linux/phy.h                        |  12 ++
 include/net/dsa.h                          |  15 ++
 net/dsa/dsa.c                              | 219 ++++++++++++++++++++------
 13 files changed, 917 insertions(+), 167 deletions(-)

-- 
2.1.0

[RFC PATCH net-next 1/8] net: dsa: Move dsa_switch_tree final setup in separate function

[Florian Fainelli]

In preparation for allowing a different API to register DSA switches,
move the end portion of dsa_setup_dst(), which binds a net_device with
its dsa_ptr, and setups the optional link polling routine into
dsa_setup_dst_end().

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/dsa.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 079a224471e7..58b99ff4591b 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -702,6 +702,26 @@ static inline void dsa_of_remove(struct device *dev)
 }
 #endif
 
+static void dsa_setup_dst_end(struct net_device *dev, struct dsa_switch_tree *dst)
+{
+	/*
+	 * If we use a tagging format that doesn't have an ethertype
+	 * field, make sure that all packets from this point on get
+	 * sent to the tag format's receive function.
+	 */
+	wmb();
+	dev->dsa_ptr = (void *)dst;
+
+	if (dst->link_poll_needed) {
+		INIT_WORK(&dst->link_poll_work, dsa_link_poll_work);
+		init_timer(&dst->link_poll_timer);
+		dst->link_poll_timer.data = (unsigned long)dst;
+		dst->link_poll_timer.function = dsa_link_poll_timer;
+		dst->link_poll_timer.expires = round_jiffies(jiffies + HZ);
+		add_timer(&dst->link_poll_timer);
+	}
+}
+
 static void dsa_setup_dst(struct dsa_switch_tree *dst, struct net_device *dev,
 			  struct device *parent, struct dsa_platform_data *pd)
 {
@@ -727,22 +747,7 @@ static void dsa_setup_dst(struct dsa_switch_tree *dst, struct net_device *dev,
 			dst->link_poll_needed = 1;
 	}
 
-	/*
-	 * If we use a tagging format that doesn't have an ethertype
-	 * field, make sure that all packets from this point on get
-	 * sent to the tag format's receive function.
-	 */
-	wmb();
-	dev->dsa_ptr = (void *)dst;
-
-	if (dst->link_poll_needed) {
-		INIT_WORK(&dst->link_poll_work, dsa_link_poll_work);
-		init_timer(&dst->link_poll_timer);
-		dst->link_poll_timer.data = (unsigned long)dst;
-		dst->link_poll_timer.function = dsa_link_poll_timer;
-		dst->link_poll_timer.expires = round_jiffies(jiffies + HZ);
-		add_timer(&dst->link_poll_timer);
-	}
+	dsa_setup_dst_end(dev, dst);
 }
 
 static int dsa_probe(struct platform_device *pdev)
-- 
2.1.0

Re: [RFC PATCH net-next 1/8] net: dsa: Move dsa_switch_tree final setup in separate function

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 29 Apr 2015 18:57:37 -0700

> +static void dsa_setup_dst_end(struct net_device *dev, struct dsa_switch_tree *dst)
> +{
> +	/*
> +	 * If we use a tagging format that doesn't have an ethertype
> +	 * field, make sure that all packets from this point on get
> +	 * sent to the tag format's receive function.
> +	 */
> +	wmb();
> +	dev->dsa_ptr = (void *)dst;

I know you're just moving code, but this cast seems pointless to me.

dev->dsa_ptr is of type "struct dsa_switch_tree".

[RFC PATCH net-next 2/8] net: phy: Check fixup lists in get_phy_device()

[Florian Fainelli]

MDIO-connected Ethernet switches will typicall appear as regular
Ethernet PHYs on the MDIO bus, except that they will be accessed using
their unique pseudo-PHY address (16 for Marvell, 30 for Broadcom etc...)
and reading standard MII registers such as MII_PHYSID1 and MII_PHYSID2
from this pseudo-PHY address will not yield expected results.

This is expected because the pseudo-PHY is a special port of the switch
to access internal registers.

PHY drivers will typically intercept these reads by registering a fixup,
checking that the fixup should only apply to the PHY address at the
pseudo-PHY location, and return the expected 32-bits unique OUI of the
switch when reads to that address are performed.

This currently does not work when probed from Device Tree since we
end-up calling get_phy_device() from drivers/of/of_mdio.c, read, e.g: all
F's and decide there is no such PHY there.

A simple fix to that problem is to verify that the fixup list for that
PHY is not empty, if not then we just allow the PHY device creation
since that means a corresponding driver is intending to fixup reads of
the PHY id.

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

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index bdfe51fc3a65..8efc4b8ffc59 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -333,8 +333,13 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 	if (r)
 		return ERR_PTR(r);
 
-	/* If the phy_id is mostly Fs, there is no device there */
-	if ((phy_id & 0x1fffffff) == 0x1fffffff)
+	/* If the phy_id is mostly Fs and we have no fixups registered which
+	 * could alter how phydev->phy_id is going to be read,
+	 * there is no device there */
+	mutex_lock(&phy_fixup_lock);
+	r = ((phy_id & 0x1fffffff) == 0x1fffffff) && list_empty(&phy_fixup_list);
+	mutex_unlock(&phy_fixup_lock);
+	if (r)
 		return NULL;
 
 	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
-- 
2.1.0

Re: [RFC PATCH net-next 2/8] net: phy: Check fixup lists in get_phy_device()

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 29 Apr 2015 18:57:38 -0700

> MDIO-connected Ethernet switches will typicall appear as regular

Typo "typically".

[RFC PATCH net-next 3/8] net: phy: Allow PHY devices to identify themselves as Ethernet switches

[Florian Fainelli]

Some Ethernet MAC drivers using the PHY library require the hardcoding
of link parameters when interfaced to a switch device. This has
typically lead to various ad-hoc implementations looking like this:

- using a "fixed PHY" emulated device, which will provide link
  indication towards the Ethernet MAC driver and hardware

- pretend there is no PHY and hardcode link parameters, ala mv643x_eth

Based on that, it is desireable to have the PHY drivers advertise the
correct link parameters, just like regular Ethernet PHYs towards their
CPU Ethernet MAC drivers, however, Ethernet MAC drivers should be able
to tell whether this link should be monitored or not. In the context of
an Ethernet switch, we do not need to monitor this link since it should
be always up.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/phy.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 685809835b5c..52fc64874fdb 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -327,6 +327,7 @@ struct phy_c45_device_ids {
  * 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.
+ * is_switch: Set to true if this phy is an Ethernet switch.
  * has_fixups: Set to true if this phy has fixups/quirks.
  * suspended: Set to true if this phy has been suspended successfully.
  * state: state of the PHY for management purposes
@@ -365,6 +366,7 @@ struct phy_device {
 	struct phy_c45_device_ids c45_ids;
 	bool is_c45;
 	bool is_internal;
+	bool is_switch;
 	bool has_fixups;
 	bool suspended;
 
@@ -675,6 +677,16 @@ static inline bool phy_is_internal(struct phy_device *phydev)
 }
 
 /**
+ * phy_is_ethswitch - Convenience function for testing if this PHY is the
+ * CPU port facing side of an Ethernet switch
+ * @phydev: the phy_device struct
+ */
+static inline bool phy_is_ethswitch(struct phy_device *phydev)
+{
+	return phydev->is_switch;
+}
+
+/**
  * phy_write_mmd - Convenience function for writing a register
  * on an MMD on a given PHY.
  * @phydev: The phy_device struct
-- 
2.1.0

Re: [RFC PATCH net-next 3/8] net: phy: Allow PHY devices to identify themselves as Ethernet switches

[Andrew Lunn]

On Wed, Apr 29, 2015 at 06:57:39PM -0700, Florian Fainelli wrote:
> Some Ethernet MAC drivers using the PHY library require the hardcoding
> of link parameters when interfaced to a switch device. This has
> typically lead to various ad-hoc implementations looking like this:
> 
> - using a "fixed PHY" emulated device, which will provide link
>   indication towards the Ethernet MAC driver and hardware
> 
> - pretend there is no PHY and hardcode link parameters, ala mv643x_eth
> 
> Based on that, it is desireable to have the PHY drivers advertise the
> correct link parameters, just like regular Ethernet PHYs towards their
> CPU Ethernet MAC drivers, however, Ethernet MAC drivers should be able
> to tell whether this link should be monitored or not. In the context of
> an Ethernet switch, we do not need to monitor this link since it should
> be always up.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  include/linux/phy.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 685809835b5c..52fc64874fdb 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -327,6 +327,7 @@ struct phy_c45_device_ids {
>   * 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.
> + * is_switch: Set to true if this phy is an Ethernet switch.
>   * has_fixups: Set to true if this phy has fixups/quirks.
>   * suspended: Set to true if this phy has been suspended successfully.
>   * state: state of the PHY for management purposes
> @@ -365,6 +366,7 @@ struct phy_device {
>  	struct phy_c45_device_ids c45_ids;
>  	bool is_c45;
>  	bool is_internal;
> +	bool is_switch;

Hi Florian

I have another two use cases for fixed_phy which i'm thinking about
implementing soon. Both require putting a fixed_phy into DSA port
properties in DT.

The first is when the CPU ethernet and the switch don't have the same
speed capabilities. At the moment, the switch driver configures the
CPU port to its maximum speed. But i know of a board coming soon with
gigabit switch ports, but the CPU Ethernet is only fast Ethernet.

When switch ports are connected to an SFP module, not a copper phy. A
fixed phy allows the switch to be configured to the correct
speed/duplex to match the SFP module.

So in this respect, i'm wondering if is_switch is the best of names?

   Andrew

Re: [RFC PATCH net-next 3/8] net: phy: Allow PHY devices to identify themselves as Ethernet switches

[Florian Fainelli]

On 30/04/15 05:56, Andrew Lunn wrote:
> On Wed, Apr 29, 2015 at 06:57:39PM -0700, Florian Fainelli wrote:
>> Some Ethernet MAC drivers using the PHY library require the hardcoding
>> of link parameters when interfaced to a switch device. This has
>> typically lead to various ad-hoc implementations looking like this:
>>
>> - using a "fixed PHY" emulated device, which will provide link
>>   indication towards the Ethernet MAC driver and hardware
>>
>> - pretend there is no PHY and hardcode link parameters, ala mv643x_eth
>>
>> Based on that, it is desireable to have the PHY drivers advertise the
>> correct link parameters, just like regular Ethernet PHYs towards their
>> CPU Ethernet MAC drivers, however, Ethernet MAC drivers should be able
>> to tell whether this link should be monitored or not. In the context of
>> an Ethernet switch, we do not need to monitor this link since it should
>> be always up.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  include/linux/phy.h | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 685809835b5c..52fc64874fdb 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -327,6 +327,7 @@ struct phy_c45_device_ids {
>>   * 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.
>> + * is_switch: Set to true if this phy is an Ethernet switch.
>>   * has_fixups: Set to true if this phy has fixups/quirks.
>>   * suspended: Set to true if this phy has been suspended successfully.
>>   * state: state of the PHY for management purposes
>> @@ -365,6 +366,7 @@ struct phy_device {
>>  	struct phy_c45_device_ids c45_ids;
>>  	bool is_c45;
>>  	bool is_internal;
>> +	bool is_switch;
> 
> Hi Florian
> 
> I have another two use cases for fixed_phy which i'm thinking about
> implementing soon. Both require putting a fixed_phy into DSA port
> properties in DT.
> 
> The first is when the CPU ethernet and the switch don't have the same
> speed capabilities. At the moment, the switch driver configures the
> CPU port to its maximum speed. But i know of a board coming soon with
> gigabit switch ports, but the CPU Ethernet is only fast Ethernet.

With the patch after, if your switch is MDIO connected to your host, you
could make that happen easily, since the read_status() callback would
only be invoked for the CPU port from the CPU Ethernet MAC driver
(that's the theory).

> 
> When switch ports are connected to an SFP module, not a copper phy. A
> fixed phy allows the switch to be configured to the correct
> speed/duplex to match the SFP module.

Ok, but not these cases though.

> 
> So in this respect, i'm wondering if is_switch is the best of names?

Probably not, I agree, pseudo_fixed_link ;)?
--
Florian

Re: [RFC PATCH net-next 3/8] net: phy: Allow PHY devices to identify themselves as Ethernet switches

[Andrew Lunn]

> > Hi Florian
> > 
> > I have another two use cases for fixed_phy which i'm thinking about
> > implementing soon. Both require putting a fixed_phy into DSA port
> > properties in DT.
> > 
> > The first is when the CPU ethernet and the switch don't have the same
> > speed capabilities. At the moment, the switch driver configures the
> > CPU port to its maximum speed. But i know of a board coming soon with
> > gigabit switch ports, but the CPU Ethernet is only fast Ethernet.
> 
> With the patch after, if your switch is MDIO connected to your host, you
> could make that happen easily, since the read_status() callback would
> only be invoked for the CPU port from the CPU Ethernet MAC driver
> (that's the theory).

I'm not sure i get what you are saying.

What we currently have is:

CPU ETH->fixed_phy                      Switch port CPU
|                                       |
+---------------------------------------+

and what i'm thinking we want is:

CPU ETH->fixed_phy          fixed_phy <-Switch port CPU
|                                       |
+---------------------------------------+

So that when setting up the switch side of the link, i can read from
the switch ports fixed_phy how the port should be configured.

I'm also wondering if they could even share one fixed_phy? 
But i suspect that will not work.

> > So in this respect, i'm wondering if is_switch is the best of names?
> 
> Probably not, I agree, pseudo_fixed_link ;)?

Yes, something like that.

     Andrew

Re: [RFC PATCH net-next 3/8] net: phy: Allow PHY devices to identify themselves as Ethernet switches

[Florian Fainelli]

On 30/04/15 10:16, Andrew Lunn wrote:
>>> Hi Florian
>>>
>>> I have another two use cases for fixed_phy which i'm thinking about
>>> implementing soon. Both require putting a fixed_phy into DSA port
>>> properties in DT.
>>>
>>> The first is when the CPU ethernet and the switch don't have the same
>>> speed capabilities. At the moment, the switch driver configures the
>>> CPU port to its maximum speed. But i know of a board coming soon with
>>> gigabit switch ports, but the CPU Ethernet is only fast Ethernet.
>>
>> With the patch after, if your switch is MDIO connected to your host, you
>> could make that happen easily, since the read_status() callback would
>> only be invoked for the CPU port from the CPU Ethernet MAC driver
>> (that's the theory).
> 
> I'm not sure i get what you are saying.
> 
> What we currently have is:
> 
> CPU ETH->fixed_phy                      Switch port CPU
> |                                       |
> +---------------------------------------+
> 
> and what i'm thinking we want is:
> 
> CPU ETH->fixed_phy          fixed_phy <-Switch port CPU
> |                                       |
> +---------------------------------------+
> 
> So that when setting up the switch side of the link, i can read from
> the switch ports fixed_phy how the port should be configured.

I see, but it still seems to me like these fixed PHY devices could be
eliminated completely only, and only if your switch is MDIO connected,
because then this becomes this:

CPU ETH -> PHY driver <- CPU port of the switch

and the PHY driver provided by your switch gives you the correct link
parameters for both ends, right?

If you do not have a MDIO switch, or something we could discover the
link parameters from, then a fixed PHY has to be used, and it seems to
me like we should have some sort of generic code doing that in DSA
today: try to find a 'fixed-link' subnode (or old 5-tuple property) in
the device tree node pointed at by dsa,ethernet, read these and feed
them back to the DSA switch driver such that link parameters can be
properly configured?

Feels like whatever we decide there is definitively something missing
today though, either the Ethernet MAC does not have the hooks to get the
parameters, or the switch has hardcoded them...
-- 
Florian

Re: [RFC PATCH net-next 3/8] net: phy: Allow PHY devices to identify themselves as Ethernet switches

[Andrew Lunn]

On Thu, Apr 30, 2015 at 10:37:44AM -0700, Florian Fainelli wrote:
> On 30/04/15 10:16, Andrew Lunn wrote:
> >>> Hi Florian
> >>>
> >>> I have another two use cases for fixed_phy which i'm thinking about
> >>> implementing soon. Both require putting a fixed_phy into DSA port
> >>> properties in DT.
> >>>
> >>> The first is when the CPU ethernet and the switch don't have the same
> >>> speed capabilities. At the moment, the switch driver configures the
> >>> CPU port to its maximum speed. But i know of a board coming soon with
> >>> gigabit switch ports, but the CPU Ethernet is only fast Ethernet.
> >>
> >> With the patch after, if your switch is MDIO connected to your host, you
> >> could make that happen easily, since the read_status() callback would
> >> only be invoked for the CPU port from the CPU Ethernet MAC driver
> >> (that's the theory).
> > 
> > I'm not sure i get what you are saying.
> > 
> > What we currently have is:
> > 
> > CPU ETH->fixed_phy                      Switch port CPU
> > |                                       |
> > +---------------------------------------+
> > 
> > and what i'm thinking we want is:
> > 
> > CPU ETH->fixed_phy          fixed_phy <-Switch port CPU
> > |                                       |
> > +---------------------------------------+
> > 
> > So that when setting up the switch side of the link, i can read from
> > the switch ports fixed_phy how the port should be configured.
> 
> I see, but it still seems to me like these fixed PHY devices could be
> eliminated completely only, and only if your switch is MDIO connected,
> because then this becomes this:
> 
> CPU ETH -> PHY driver <- CPU port of the switch
> 
> and the PHY driver provided by your switch gives you the correct link
> parameters for both ends, right?

Are you suggesting this phy driver sat in the middle effectively
performs auto negotiation in both directions? It sees what both sides
offer and then gives back the highest common setting?

> If you do not have a MDIO switch, or something we could discover the
> link parameters from, then a fixed PHY has to be used, and it seems to
> me like we should have some sort of generic code doing that in DSA
> today: try to find a 'fixed-link' subnode (or old 5-tuple property) in
> the device tree node pointed at by dsa,ethernet, read these and feed
> them back to the DSA switch driver such that link parameters can be
> properly configured?

I don't think it should come from dsa,ethernet. Think of the case of
two CPU ports? It is a property of the cpu port(s).
 
> Feels like whatever we decide there is definitively something missing
> today though, either the Ethernet MAC does not have the hooks to get the
> parameters, or the switch has hardcoded them...

Agreed. And we should also try to include into the picture how we code
with fixed phy configurations on normal ports, e.g. SFP modules. Maybe
we don't need anything special for CPU and DSA ports, we solve it for
all types of ports.

    Andrew

Re: [RFC PATCH net-next 3/8] net: phy: Allow PHY devices to identify themselves as Ethernet switches

[Florian Fainelli]

On 30/04/15 10:48, Andrew Lunn wrote:
> On Thu, Apr 30, 2015 at 10:37:44AM -0700, Florian Fainelli wrote:
>> On 30/04/15 10:16, Andrew Lunn wrote:
>>>>> Hi Florian
>>>>>
>>>>> I have another two use cases for fixed_phy which i'm thinking about
>>>>> implementing soon. Both require putting a fixed_phy into DSA port
>>>>> properties in DT.
>>>>>
>>>>> The first is when the CPU ethernet and the switch don't have the same
>>>>> speed capabilities. At the moment, the switch driver configures the
>>>>> CPU port to its maximum speed. But i know of a board coming soon with
>>>>> gigabit switch ports, but the CPU Ethernet is only fast Ethernet.
>>>>
>>>> With the patch after, if your switch is MDIO connected to your host, you
>>>> could make that happen easily, since the read_status() callback would
>>>> only be invoked for the CPU port from the CPU Ethernet MAC driver
>>>> (that's the theory).
>>>
>>> I'm not sure i get what you are saying.
>>>
>>> What we currently have is:
>>>
>>> CPU ETH->fixed_phy                      Switch port CPU
>>> |                                       |
>>> +---------------------------------------+
>>>
>>> and what i'm thinking we want is:
>>>
>>> CPU ETH->fixed_phy          fixed_phy <-Switch port CPU
>>> |                                       |
>>> +---------------------------------------+
>>>
>>> So that when setting up the switch side of the link, i can read from
>>> the switch ports fixed_phy how the port should be configured.
>>
>> I see, but it still seems to me like these fixed PHY devices could be
>> eliminated completely only, and only if your switch is MDIO connected,
>> because then this becomes this:
>>
>> CPU ETH -> PHY driver <- CPU port of the switch
>>
>> and the PHY driver provided by your switch gives you the correct link
>> parameters for both ends, right?
> 
> Are you suggesting this phy driver sat in the middle effectively
> performs auto negotiation in both directions? It sees what both sides
> offer and then gives back the highest common setting?

Yes, that's what I would be tempted to do.

> 
>> If you do not have a MDIO switch, or something we could discover the
>> link parameters from, then a fixed PHY has to be used, and it seems to
>> me like we should have some sort of generic code doing that in DSA
>> today: try to find a 'fixed-link' subnode (or old 5-tuple property) in
>> the device tree node pointed at by dsa,ethernet, read these and feed
>> them back to the DSA switch driver such that link parameters can be
>> properly configured?
> 
> I don't think it should come from dsa,ethernet. Think of the case of
> two CPU ports? It is a property of the cpu port(s).

True. That part actually should work correctly, except for the CPU port
for which we do not parse any of these properties, see below.

>  
>> Feels like whatever we decide there is definitively something missing
>> today though, either the Ethernet MAC does not have the hooks to get the
>> parameters, or the switch has hardcoded them...
> 
> Agreed. And we should also try to include into the picture how we code
> with fixed phy configurations on normal ports, e.g. SFP modules. Maybe
> we don't need anything special for CPU and DSA ports, we solve it for
> all types of ports.

So right now, for any port you can utilize standard PHY-related or
fixed-PHY related properties, for instance this is what I use on a
BCM7445 which has the following setup:

- Port 0 has an internal gigabit PHY on dsa,mii-bus
- Port 1 is connected to an external BCM53125 switch
- Port 7 is connected to an internal MoCA PHY, whose link comes from a
special interrupt
- Port 8 is CPU

So today, the only thing we are missing is giving the CPU port some link
information, but we could probably utilize a 'fixed-link' property for
that maybe?

                        ethernet_switch@0 {
                                #address-cells = <0x2>;
                                #size-cells = <0x0>;
                                brcm,num-acb-queues = <0x40>;
                                brcm,num-gphy = <0x1>;
                                brcm,num-rgmii-ports = <0x2>;
                                compatible = "brcm,bcm7445-switch-v4.0",
"brcm,bcm53012";
                                dsa,ethernet = <0x1d>;
                                dsa,mii-bus = <0x1e>;
                                reg = <0x0 0x40000 0x40000 0x110 0x40340
0x30 0x40380 0x30 0x40400 0x34 0x40600 0x208>;
                                reg-names = "core", "reg", "intrl2_0",
"intrl2_1", "fcb", "acb";
                                interrupts = <0x0 0x18 0x0 0x0 0x19 0x0>;
                                interrupt-names = "switch_0", "switch_1";
                                brcm,fcb-pause-override;
                                brcm,acb-packets-inflight;
                                clocks = <0x1f 0x20>;
                                clock-names = "sw_switch", "sw_switch_mdiv";

                                switch@0 {
                                        #address-cells = <0x1>;
                                        #size-cells = <0x0>;
                                        reg = <0x0 0x0>;

                                        port@0 {
                                                phy-mode = "internal";
                                                phy-handle = <0x8f>;
                                                linux,phandle = <0x8d>;
                                                phandle = <0x8d>;
                                                reg = <0x0>;
                                                label = "gphy";
                                        };

                                        port@1 {
                                                phy-mode = "rgmii-txid";
                                                phy-handle = <0x91>;
                                                linux,phandle = <0x90>;
                                                phandle = <0x90>;
                                                reg = <0x1>;
                                                label = "rgmii_1";
                                        };

                                        port@2 {
                                                phy-mode = "rgmii-txid";
                                                fixed-link = <0x2 0x1
0x3e8 0x0 0x0>;
                                                linux,phandle = <0x92>;
                                                phandle = <0x92>;
                                                reg = <0x2>;
                                                label = "rgmii_2";
                                        };

                                        port@7 {
                                                phy-mode = "moca";
                                                fixed-link = <0x7 0x1
0x3e8 0x0 0x0>;
                                                linux,phandle = <0x93>;
                                                phandle = <0x93>;
                                                reg = <0x7>;
                                                label = "moca";
                                        };

                                        port@8 {
                                                linux,phandle = <0x94>;
                                                phandle = <0x94>;
                                                reg = <0x8>;
                                                label = "cpu";
                                        };
                                };
                        };

/                       mdio@403c0 {
                                reg = <0x403c0 0x8 0x40300 0x18>;
                                #address-cells = <0x0>;
                                #size-cells = <0x1>;
                                compatible = "brcm,bcm7445-mdio-v4.0",
"brcm,unimac-mdio";
                                reg-names = "mdio", "mdio_indir_rw";
                                linux,phandle = <0x1e>;
                                phandle = <0x1e>;

                                phy0: ethernet-phy@0 {
                                        linux,phandle = <0x91>;
                                        phandle = <0x91>;
                                        device_type = "ethernet-phy";
                                        max-speed = <0x3e8>;
                                        reg = <0x0>;
                                        compatible = "brcm,bcm53125",
"ethernet-phy-ieee802.3-c22";
                                };

                                phy5: ethernet-phy@5 {
                                        linux,phandle = <0x8f>;
                                        phandle = <0x8f>;
                                        clock-names = "sw_gphy";
                                        clocks = <0x8e>;
                                        device_type = "ethernet-phy";
                                        max-speed = <0x3e8>;
                                        reg = <0x5>;
                                        compatible = "brcm,28nm-gphy",
"ethernet-phy-ieee802.3-c22";
                                };
                        };
-- 
Florian

Re: [RFC PATCH net-next 3/8] net: phy: Allow PHY devices to identify themselves as Ethernet switches

[Andrew Lunn]

> > Are you suggesting this phy driver sat in the middle effectively
> > performs auto negotiation in both directions? It sees what both sides
> > offer and then gives back the highest common setting?
> 
> Yes, that's what I would be tempted to do.

O.K, i need to think about that for a while.
 
> So right now, for any port you can utilize standard PHY-related or
> fixed-PHY related properties, for instance this is what I use on a
> BCM7445 which has the following setup:
> 
> - Port 0 has an internal gigabit PHY on dsa,mii-bus
> - Port 1 is connected to an external BCM53125 switch
> - Port 7 is connected to an internal MoCA PHY, whose link comes from a
> special interrupt
> - Port 8 is CPU
> 
> So today, the only thing we are missing is giving the CPU port some link
> information, but we could probably utilize a 'fixed-link' property for
> that maybe?

The Marvell drivers don't look at this phy information. But i should
have good examples from the SF2 driver i can use :-)

I also think the core DSA code does not allow for a phy on CPU and DSA
ports. But that should not be too big a problem.

       Andrew

[RFC PATCH net-next 4/8] net: mv643xx_eth: Handle Ethernet switches as PHY devices

[Florian Fainelli]

In preparation for allowing Ethernet switches to be real PHY devices,
adjust the mv643x_eth driver to do two things in case such a PHY device
is detected:

- do not program the PHY polling unit to watch for this PHY since it
  would yield inconsistent results, reading from the pseudo-PHY address,
  not regular MII_BMSR registers

- force the link indication since this is the CPU-facing port of the
  switch which is always up and running

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

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index 1c75829eb166..72ce27562d6f 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2321,7 +2321,7 @@ static void port_start(struct mv643xx_eth_private *mp)
 	wrlp(mp, PORT_SERIAL_CONTROL, pscr);
 
 	pscr |= DO_NOT_FORCE_LINK_FAIL;
-	if (mp->phy == NULL)
+	if (mp->phy == NULL || (mp->phy && phy_is_ethswitch(mp->phy)))
 		pscr |= FORCE_LINK_PASS;
 	wrlp(mp, PORT_SERIAL_CONTROL, pscr);
 
@@ -3101,7 +3101,7 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
 					 PHY_INTERFACE_MODE_GMII);
 		if (!mp->phy)
 			err = -ENODEV;
-		else
+		else if (!phy_is_ethswitch(mp->phy))
 			phy_addr_set(mp, mp->phy->addr);
 	} else if (pd->phy_addr != MV643XX_ETH_PHY_NONE) {
 		mp->phy = phy_scan(mp, pd->phy_addr);
-- 
2.1.0

Re: [RFC PATCH net-next 4/8] net: mv643xx_eth: Handle Ethernet switches as PHY devices

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 29 Apr 2015 18:57:40 -0700

> @@ -2321,7 +2321,7 @@ static void port_start(struct mv643xx_eth_private *mp)
>  	wrlp(mp, PORT_SERIAL_CONTROL, pscr);
>  
>  	pscr |= DO_NOT_FORCE_LINK_FAIL;
> -	if (mp->phy == NULL)
> +	if (mp->phy == NULL || (mp->phy && phy_is_ethswitch(mp->phy)))
>  		pscr |= FORCE_LINK_PASS;

Maybe it looks clearer to you in this form, but the C language
guarentees that:

	if (!ptr || ptr->foo)

will not perform a NULL pointer deref.

So I'm pretty sure you could simply this statement to:

	if (mp->phy == NULL || phy_is_ethswitch(mp->phy))

[RFC PATCH net-next 5/8] net: dsa: add new API to register switch devices

[Florian Fainelli]

Add a new API to register DSA switch devices:

- dsa_switch_register
- dsa_switch_unregister

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/net/dsa.h |  15 +++++
 net/dsa/dsa.c     | 182 +++++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 168 insertions(+), 29 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index fbca63ba8f73..b057536f6a77 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -139,6 +139,12 @@ struct dsa_switch {
 	struct dsa_chip_data	*pd;
 
 	/*
+	 * Configuration came from "classic" platform_data
+	 * registration
+	 */
+	bool			from_pd;
+
+	/*
 	 * The used switch driver.
 	 */
 	struct dsa_switch_driver	*drv;
@@ -317,4 +323,13 @@ static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
 {
 	return dst->rcv != NULL;
 }
+
+static inline struct dsa_switch *dsa_alloc_switch(size_t priv_size)
+{
+	return kzalloc(sizeof(struct dsa_switch) + priv_size, GFP_KERNEL);
+}
+
+int dsa_switch_register(struct dsa_switch *ds);
+int dsa_switch_register_phydev(struct dsa_switch *ds, struct phy_device *phy);
+void dsa_switch_unregister(struct dsa_switch *ds);
 #endif
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 58b99ff4591b..2d9762974880 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -364,6 +364,7 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
 	ds->dst = dst;
 	ds->index = index;
 	ds->pd = pd;
+	ds->from_pd = true;
 	ds->drv = drv;
 	ds->tag_protocol = drv->tag_protocol;
 	ds->master_dev = host_dev;
@@ -570,12 +571,12 @@ static void dsa_of_free_platform_data(struct dsa_platform_data *pd)
 	kfree(pd->chip);
 }
 
-static int dsa_of_probe(struct device *dev)
+static int dsa_of_probe(struct device *dev, struct phy_device *phydev)
 {
 	struct device_node *np = dev->of_node;
 	struct device_node *child, *mdio, *ethernet, *port, *link;
-	struct mii_bus *mdio_bus;
-	struct net_device *ethernet_dev;
+	struct mii_bus *mdio_bus = NULL;
+	struct net_device *ethernet_dev = NULL;
 	struct dsa_platform_data *pd;
 	struct dsa_chip_data *cd;
 	const char *port_name;
@@ -584,28 +585,38 @@ static int dsa_of_probe(struct device *dev)
 	u32 eeprom_len;
 	int ret;
 
-	mdio = of_parse_phandle(np, "dsa,mii-bus", 0);
-	if (!mdio)
-		return -EINVAL;
-
-	mdio_bus = of_mdio_find_bus(mdio);
-	if (!mdio_bus)
-		return -EPROBE_DEFER;
-
-	ethernet = of_parse_phandle(np, "dsa,ethernet", 0);
-	if (!ethernet)
-		return -EINVAL;
-
-	ethernet_dev = of_find_net_device_by_node(ethernet);
-	if (!ethernet_dev)
-		return -EPROBE_DEFER;
+	/* Parsing of these properties only makes sense in the context of
+	 * non-MDIO probed devices (phydev == NULL) since all of these are
+	 * already implied by the PHY device:
+	 *
+	 * phydev->attached_dev is our netdev
+	 * phydev->bus is our MDIO bus
+	 */
+	if (!phydev) {
+		mdio = of_parse_phandle(np, "dsa,mii-bus", 0);
+		if (!mdio)
+			return -EINVAL;
+
+		mdio_bus = of_mdio_find_bus(mdio);
+		if (!mdio_bus)
+			return -EPROBE_DEFER;
+
+		ethernet = of_parse_phandle(np, "dsa,ethernet", 0);
+		if (!ethernet)
+			return -EINVAL;
+
+		ethernet_dev = of_find_net_device_by_node(ethernet);
+		if (!ethernet_dev)
+			return -EPROBE_DEFER;
+	}
 
 	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
 	if (!pd)
 		return -ENOMEM;
 
 	dev->platform_data = pd;
-	pd->of_netdev = ethernet_dev;
+	if (!phydev)
+		pd->of_netdev = ethernet_dev;
 	pd->nr_chips = of_get_available_child_count(np);
 	if (pd->nr_chips > DSA_MAX_SWITCHES)
 		pd->nr_chips = DSA_MAX_SWITCHES;
@@ -623,15 +634,21 @@ static int dsa_of_probe(struct device *dev)
 		cd = &pd->chip[chip_index];
 
 		cd->of_node = child;
-		cd->host_dev = &mdio_bus->dev;
-
-		sw_addr = of_get_property(child, "reg", NULL);
-		if (!sw_addr)
-			continue;
+		if (!phydev)
+			cd->host_dev = &mdio_bus->dev;
+		else
+			cd->host_dev = &phydev->bus->dev;
+
+		if (!phydev) {
+			sw_addr = of_get_property(child, "reg", NULL);
+			if (!sw_addr)
+				continue;
 
-		cd->sw_addr = be32_to_cpup(sw_addr);
-		if (cd->sw_addr > PHY_MAX_ADDR)
-			continue;
+			cd->sw_addr = be32_to_cpup(sw_addr);
+			if (cd->sw_addr > PHY_MAX_ADDR)
+				continue;
+		} else
+			cd->sw_addr = phydev->addr;
 
 		if (!of_property_read_u32(np, "eeprom-length", &eeprom_len))
 			cd->eeprom_len = eeprom_len;
@@ -761,7 +778,7 @@ static int dsa_probe(struct platform_device *pdev)
 		       dsa_driver_version);
 
 	if (pdev->dev.of_node) {
-		ret = dsa_of_probe(&pdev->dev);
+		ret = dsa_of_probe(&pdev->dev, NULL);
 		if (ret)
 			return ret;
 
@@ -807,6 +824,107 @@ out:
 	return ret;
 }
 
+static int __dsa_switch_register(struct dsa_switch *ds,
+				 struct phy_device *phydev)
+{
+	struct dsa_switch_tree *dst;
+	struct dsa_platform_data *pd = NULL;
+	struct net_device *dev;
+	int ret;
+
+	/* If using Device Tree, just try to parse standard properties */
+	if (ds->master_dev->of_node) {
+		ret = dsa_of_probe(ds->master_dev, phydev);
+		if (ret)
+			return ret;
+
+		pd = ds->master_dev->platform_data;
+	}
+
+	if (pd == NULL)
+		return -ENODEV;
+
+	/* Cannot find the network device */
+	if (!phydev && (pd->netdev == NULL || pd->of_netdev == NULL))
+		return -ENODEV;
+
+	if (pd->of_netdev) {
+		dev = pd->of_netdev;
+		dev_hold(dev);
+	} else if (phydev == NULL) {
+		dev = dev_to_net_device(pd->netdev);
+	} else {
+		dev = phydev->attached_dev;
+		dev_hold(dev);
+	}
+
+	/* Regardless of our position within a switch tree, we should be
+	 * attached to a network device
+	 */
+	if (dev == NULL) {
+		dev_err(ds->master_dev, "invalid network device\n");
+		if (pd->of_netdev)
+			dev_put(pd->of_netdev);
+		if (phydev)
+			dev_put(phydev->attached_dev);
+		ret = -EPROBE_DEFER;
+		goto out;
+	}
+
+	/* For now we just bail out, but we would want to deal with stacked
+	 * setups with different tagging protocols sitting on top of each other
+	 * in the future
+	 */
+	if (dev->dsa_ptr != NULL) {
+		dev_put(dev);
+		ret = -EEXIST;
+		goto out;
+	}
+
+	dst = kzalloc(sizeof(*dst), GFP_KERNEL);
+	if (!dst) {
+		dev_put(dev);
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	dst->pd = pd;
+	ds->dst = dst;
+	ds->index = 0;
+	dst->master_netdev = dev;
+	dst->cpu_switch = -1;
+	dst->cpu_port = -1;
+	ds->pd = (struct dsa_chip_data *)dst->pd->chip + ds->index;
+
+	ret = dsa_switch_setup_one(ds, ds->master_dev);
+	if (ret)
+		return ret;
+
+	dst->ds[ds->index] = ds;
+
+	dsa_setup_dst_end(dev, dst);
+
+	netdev_info(dev, "Attached switch@%d\n", ds->index);
+
+	return 0;
+
+out:
+	dsa_of_remove(ds->master_dev);
+	return ret;
+}
+
+int dsa_switch_register(struct dsa_switch *ds)
+{
+	return __dsa_switch_register(ds, NULL);
+}
+EXPORT_SYMBOL_GPL(dsa_switch_register);
+
+int dsa_switch_register_phydev(struct dsa_switch *ds, struct phy_device *phydev)
+{
+	return __dsa_switch_register(ds, phydev);
+}
+EXPORT_SYMBOL_GPL(dsa_switch_register_phydev);
+
 static void dsa_remove_dst(struct dsa_switch_tree *dst)
 {
 	int i;
@@ -824,6 +942,13 @@ static void dsa_remove_dst(struct dsa_switch_tree *dst)
 	}
 }
 
+void dsa_switch_unregister(struct dsa_switch *ds)
+{
+	dsa_remove_dst(ds->dst);
+	dsa_of_remove(ds->master_dev);
+}
+EXPORT_SYMBOL_GPL(dsa_switch_unregister);
+
 static int dsa_remove(struct platform_device *pdev)
 {
 	struct dsa_switch_tree *dst = platform_get_drvdata(pdev);
@@ -897,7 +1022,6 @@ static int dsa_resume(struct device *d)
 static SIMPLE_DEV_PM_OPS(dsa_pm_ops, dsa_suspend, dsa_resume);
 
 static const struct of_device_id dsa_of_match_table[] = {
-	{ .compatible = "brcm,bcm7445-switch-v4.0" },
 	{ .compatible = "marvell,dsa", },
 	{}
 };
-- 
2.1.0

Re: [RFC PATCH net-next 5/8] net: dsa: add new API to register switch devices

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 29 Apr 2015 18:57:41 -0700

> @@ -897,7 +1022,6 @@ static int dsa_resume(struct device *d)
>  static SIMPLE_DEV_PM_OPS(dsa_pm_ops, dsa_suspend, dsa_resume);
>  
>  static const struct of_device_id dsa_of_match_table[] = {
> -	{ .compatible = "brcm,bcm7445-switch-v4.0" },
>  	{ .compatible = "marvell,dsa", },
>  	{}
>  };

Hmmm, is this really meant to be a part of this patch?

If it is, you'll need to explain this clearly in the commit message
so others can understand too.

[RFC PATCH net-next 6/8] net: dsa: bcm_sf2: make it a real platform driver

[Florian Fainelli]

The Broadcom Starfighter 2 switch driver should be a proper platform
driver, now that the DSA code has been updated to allow that, register a
switch device, feed it with the proper configuration data coming from
Device Tree and register our switch device.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 239 +++++++++++++++++++++++++---------------------
 1 file changed, 128 insertions(+), 111 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index cedb572bf25a..1e75f30eaf5c 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -131,11 +131,6 @@ static int bcm_sf2_sw_get_sset_count(struct dsa_switch *ds)
 	return BCM_SF2_STATS_SIZE;
 }
 
-static char *bcm_sf2_sw_probe(struct device *host_dev, int sw_addr)
-{
-	return "Broadcom Starfighter 2";
-}
-
 static void bcm_sf2_imp_vlan_setup(struct dsa_switch *ds, int cpu_port)
 {
 	struct bcm_sf2_priv *priv = ds_to_priv(ds);
@@ -612,76 +607,8 @@ static void bcm_sf2_intr_disable(struct bcm_sf2_priv *priv)
 
 static int bcm_sf2_sw_setup(struct dsa_switch *ds)
 {
-	const char *reg_names[BCM_SF2_REGS_NUM] = BCM_SF2_REGS_NAME;
 	struct bcm_sf2_priv *priv = ds_to_priv(ds);
-	struct device_node *dn;
-	void __iomem **base;
 	unsigned int port;
-	unsigned int i;
-	u32 reg, rev;
-	int ret;
-
-	spin_lock_init(&priv->indir_lock);
-	mutex_init(&priv->stats_mutex);
-
-	/* All the interesting properties are at the parent device_node
-	 * level
-	 */
-	dn = ds->pd->of_node->parent;
-
-	priv->irq0 = irq_of_parse_and_map(dn, 0);
-	priv->irq1 = irq_of_parse_and_map(dn, 1);
-
-	base = &priv->core;
-	for (i = 0; i < BCM_SF2_REGS_NUM; i++) {
-		*base = of_iomap(dn, i);
-		if (*base == NULL) {
-			pr_err("unable to find register: %s\n", reg_names[i]);
-			ret = -ENOMEM;
-			goto out_unmap;
-		}
-		base++;
-	}
-
-	ret = bcm_sf2_sw_rst(priv);
-	if (ret) {
-		pr_err("unable to software reset switch: %d\n", ret);
-		goto out_unmap;
-	}
-
-	/* Disable all interrupts and request them */
-	bcm_sf2_intr_disable(priv);
-
-	ret = request_irq(priv->irq0, bcm_sf2_switch_0_isr, 0,
-			  "switch_0", priv);
-	if (ret < 0) {
-		pr_err("failed to request switch_0 IRQ\n");
-		goto out_unmap;
-	}
-
-	ret = request_irq(priv->irq1, bcm_sf2_switch_1_isr, 0,
-			  "switch_1", priv);
-	if (ret < 0) {
-		pr_err("failed to request switch_1 IRQ\n");
-		goto out_free_irq0;
-	}
-
-	/* Reset the MIB counters */
-	reg = core_readl(priv, CORE_GMNCFGCFG);
-	reg |= RST_MIB_CNT;
-	core_writel(priv, reg, CORE_GMNCFGCFG);
-	reg &= ~RST_MIB_CNT;
-	core_writel(priv, reg, CORE_GMNCFGCFG);
-
-	/* Get the maximum number of ports for this switch */
-	priv->hw_params.num_ports = core_readl(priv, CORE_IMP0_PRT_ID) + 1;
-	if (priv->hw_params.num_ports > DSA_MAX_PORTS)
-		priv->hw_params.num_ports = DSA_MAX_PORTS;
-
-	/* Assume a single GPHY setup if we can't read that property */
-	if (of_property_read_u32(dn, "brcm,num-gphy",
-				 &priv->hw_params.num_gphy))
-		priv->hw_params.num_gphy = 1;
 
 	/* Enable all valid ports and disable those unused */
 	for (port = 0; port < priv->hw_params.num_ports; port++) {
@@ -699,31 +626,7 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds)
 	 */
 	ds->phys_mii_mask |= ((1 << 30) | (1 << 0));
 
-	rev = reg_readl(priv, REG_SWITCH_REVISION);
-	priv->hw_params.top_rev = (rev >> SWITCH_TOP_REV_SHIFT) &
-					SWITCH_TOP_REV_MASK;
-	priv->hw_params.core_rev = (rev & SF2_REV_MASK);
-
-	rev = reg_readl(priv, REG_PHY_REVISION);
-	priv->hw_params.gphy_rev = rev & PHY_REVISION_MASK;
-
-	pr_info("Starfighter 2 top: %x.%02x, core: %x.%02x base: 0x%p, IRQs: %d, %d\n",
-		priv->hw_params.top_rev >> 8, priv->hw_params.top_rev & 0xff,
-		priv->hw_params.core_rev >> 8, priv->hw_params.core_rev & 0xff,
-		priv->core, priv->irq0, priv->irq1);
-
 	return 0;
-
-out_free_irq0:
-	free_irq(priv->irq0, priv);
-out_unmap:
-	base = &priv->core;
-	for (i = 0; i < BCM_SF2_REGS_NUM; i++) {
-		if (*base)
-			iounmap(*base);
-		base++;
-	}
-	return ret;
 }
 
 static int bcm_sf2_sw_set_addr(struct dsa_switch *ds, u8 *addr)
@@ -947,8 +850,10 @@ static void bcm_sf2_sw_fixed_link_update(struct dsa_switch *ds, int port,
 		status->pause = 1;
 }
 
-static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
+static int bcm_sf2_sw_suspend(struct device *d)
 {
+	struct platform_device *pdev = to_platform_device(d);
+	struct dsa_switch *ds = platform_get_drvdata(pdev);
 	struct bcm_sf2_priv *priv = ds_to_priv(ds);
 	unsigned int port;
 
@@ -967,8 +872,10 @@ static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
 	return 0;
 }
 
-static int bcm_sf2_sw_resume(struct dsa_switch *ds)
+static int bcm_sf2_sw_resume(struct device *d)
 {
+	struct platform_device *pdev = to_platform_device(d);
+	struct dsa_switch *ds = platform_get_drvdata(pdev);
 	struct bcm_sf2_priv *priv = ds_to_priv(ds);
 	unsigned int port;
 	int ret;
@@ -1044,10 +951,8 @@ static int bcm_sf2_sw_set_wol(struct dsa_switch *ds, int port,
 	return p->ethtool_ops->set_wol(p, wol);
 }
 
-static struct dsa_switch_driver bcm_sf2_switch_driver = {
+static struct dsa_switch_driver bcm_sf2_switch_ops = {
 	.tag_protocol		= DSA_TAG_PROTO_BRCM,
-	.priv_size		= sizeof(struct bcm_sf2_priv),
-	.probe			= bcm_sf2_sw_probe,
 	.setup			= bcm_sf2_sw_setup,
 	.set_addr		= bcm_sf2_sw_set_addr,
 	.get_phy_flags		= bcm_sf2_sw_get_phy_flags,
@@ -1058,8 +963,6 @@ static struct dsa_switch_driver bcm_sf2_switch_driver = {
 	.get_sset_count		= bcm_sf2_sw_get_sset_count,
 	.adjust_link		= bcm_sf2_sw_adjust_link,
 	.fixed_link_update	= bcm_sf2_sw_fixed_link_update,
-	.suspend		= bcm_sf2_sw_suspend,
-	.resume			= bcm_sf2_sw_resume,
 	.get_wol		= bcm_sf2_sw_get_wol,
 	.set_wol		= bcm_sf2_sw_set_wol,
 	.port_enable		= bcm_sf2_port_setup,
@@ -1071,19 +974,133 @@ static struct dsa_switch_driver bcm_sf2_switch_driver = {
 	.port_stp_update	= bcm_sf2_sw_br_set_stp_state,
 };
 
-static int __init bcm_sf2_init(void)
+static int bcm_sf2_sw_probe(struct platform_device *pdev)
 {
-	register_switch_driver(&bcm_sf2_switch_driver);
+	const char *reg_names[BCM_SF2_REGS_NUM] = BCM_SF2_REGS_NAME;
+	struct resource *res;
+	struct dsa_switch *ds;
+	struct bcm_sf2_priv *priv;
+	struct device_node *dn = pdev->dev.of_node;
+	void __iomem **base;
+	unsigned int i;
+	u32 reg, rev;
+	int ret;
 
-	return 0;
+	ds = devm_kzalloc(&pdev->dev, sizeof(*ds) + sizeof(*priv), GFP_KERNEL);
+	if (!ds)
+		return -ENOMEM;
+
+	priv = ds_to_priv(ds);
+
+	ds->master_dev = &pdev->dev;
+	ds->tag_protocol = DSA_TAG_PROTO_BRCM;
+	ds->drv = &bcm_sf2_switch_ops;
+
+	spin_lock_init(&priv->indir_lock);
+	mutex_init(&priv->stats_mutex);
+
+	priv->irq0 = platform_get_irq(pdev, 0);
+	priv->irq1 = platform_get_irq(pdev, 1);
+
+	base = &priv->core;
+	for (i = 0; i < BCM_SF2_REGS_NUM; i++) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		*base = devm_ioremap_resource(&pdev->dev, res);
+		if (*base == NULL) {
+			dev_err(&pdev->dev,
+				"unable to find register: %s\n", reg_names[i]);
+			return -ENODEV;
+		}
+		base++;
+	}
+
+	ret = bcm_sf2_sw_rst(priv);
+	if (ret) {
+		pr_err("unable to software reset switch: %d\n", ret);
+		return ret;
+	}
+
+	/* Disable all interrupts and request them */
+	bcm_sf2_intr_disable(priv);
+
+	ret = devm_request_irq(&pdev->dev, priv->irq0, bcm_sf2_switch_0_isr, 0,
+			       "switch_0", priv);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to request switch_0 IRQ\n");
+		return ret;
+	}
+
+	ret = devm_request_irq(&pdev->dev, priv->irq1, bcm_sf2_switch_1_isr, 0,
+			       "switch_1", priv);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to request switch_1 IRQ\n");
+		return ret;
+	}
+
+	/* Reset the MIB counters */
+	reg = core_readl(priv, CORE_GMNCFGCFG);
+	reg |= RST_MIB_CNT;
+	core_writel(priv, reg, CORE_GMNCFGCFG);
+	reg &= ~RST_MIB_CNT;
+	core_writel(priv, reg, CORE_GMNCFGCFG);
+
+	/* Get the maximum number of ports for this switch */
+	priv->hw_params.num_ports = core_readl(priv, CORE_IMP0_PRT_ID) + 1;
+	if (priv->hw_params.num_ports > DSA_MAX_PORTS)
+		priv->hw_params.num_ports = DSA_MAX_PORTS;
+
+	/* Assume a single GPHY setup if we can't read that property */
+	if (of_property_read_u32(dn, "brcm,num-gphy",
+				 &priv->hw_params.num_gphy))
+		priv->hw_params.num_gphy = 1;
+
+	rev = reg_readl(priv, REG_SWITCH_REVISION);
+	priv->hw_params.top_rev = (rev >> SWITCH_TOP_REV_SHIFT) &
+					SWITCH_TOP_REV_MASK;
+	priv->hw_params.core_rev = (rev & SF2_REV_MASK);
+
+	dev_info(&pdev->dev,
+		"Starfighter 2 top: %x.%02x, core: %x.%02x base: 0x%p, IRQs: %d, %d\n",
+		priv->hw_params.top_rev >> 8, priv->hw_params.top_rev & 0xff,
+		priv->hw_params.core_rev >> 8, priv->hw_params.core_rev & 0xff,
+		priv->core, priv->irq0, priv->irq1);
+
+	platform_set_drvdata(pdev, ds);
+
+	return dsa_switch_register(ds);
 }
-module_init(bcm_sf2_init);
 
-static void __exit bcm_sf2_exit(void)
+static int bcm_sf2_sw_remove(struct platform_device *pdev)
 {
-	unregister_switch_driver(&bcm_sf2_switch_driver);
+	struct dsa_switch *ds = platform_get_drvdata(pdev);
+	struct bcm_sf2_priv *priv = ds_to_priv(ds);
+
+	/* Disable all ports and interrupts */
+	priv->wol_ports_mask = 0;
+	bcm_sf2_sw_suspend(&pdev->dev);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
 }
-module_exit(bcm_sf2_exit);
+
+static SIMPLE_DEV_PM_OPS(bcm_sf2_pm_ops,
+			 bcm_sf2_sw_suspend, bcm_sf2_sw_resume);
+
+static const struct of_device_id bcm_sf2_of_match[] = {
+	{ .compatible = "brcm,bcm7445-switch-v4.0" },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver bcm_sf2_driver = {
+	.probe	= bcm_sf2_sw_probe,
+	.remove	= bcm_sf2_sw_remove,
+	.driver = {
+		.name = "brcm-sf2",
+		.of_match_table = bcm_sf2_of_match,
+		.pm = &bcm_sf2_pm_ops,
+	},
+};
+module_platform_driver(bcm_sf2_driver);
 
 MODULE_AUTHOR("Broadcom Corporation");
 MODULE_DESCRIPTION("Driver for Broadcom Starfighter 2 ethernet switch chip");
-- 
2.1.0

[RFC PATCH net-next 7/8] net: dsa: mv88e6060: make it a proper PHY driver

[Florian Fainelli]

Convert the Marvell 88E6060 switch driver into a proper PHY library
driver that can be registered. To make sure we do not introduce
functional changes, the PHY driver provides autoneg and status callbacks
to make sure the attached Ethernet MAC driver still sees a link UP at
the CPU port full speed.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/mv88e6060.c | 114 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 109 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
index c29aebe1e62b..7bd15bbb0787 100644
--- a/drivers/net/dsa/mv88e6060.c
+++ b/drivers/net/dsa/mv88e6060.c
@@ -19,9 +19,23 @@
 #define REG_PORT(p)		(8 + (p))
 #define REG_GLOBAL		0x0f
 
+#define MV88E6060_MAGIC		0x600
+#define MV88E6060_MAGIC_MASK	0xfff0
+
+static inline struct mii_bus *mv88e6060_mii_bus(struct dsa_switch *ds)
+{
+	struct phy_device *phydev;
+
+	if (ds->from_pd)
+		return dsa_host_dev_to_mii_bus(ds->master_dev);
+
+	phydev = to_phy_device(ds->master_dev);
+	return phydev->bus;
+}
+
 static int reg_read(struct dsa_switch *ds, int addr, int reg)
 {
-	struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev);
+	struct mii_bus *bus = mv88e6060_mii_bus(ds);
 
 	if (bus == NULL)
 		return -EINVAL;
@@ -42,7 +56,7 @@ static int reg_read(struct dsa_switch *ds, int addr, int reg)
 
 static int reg_write(struct dsa_switch *ds, int addr, int reg, u16 val)
 {
-	struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev);
+	struct mii_bus *bus = mv88e6060_mii_bus(ds);
 
 	if (bus == NULL)
 		return -EINVAL;
@@ -59,7 +73,7 @@ static int reg_write(struct dsa_switch *ds, int addr, int reg, u16 val)
 			return __ret;				\
 	})
 
-static char *mv88e6060_probe(struct device *host_dev, int sw_addr)
+static char *mv88e6060_switch_probe(struct device *host_dev, int sw_addr)
 {
 	struct mii_bus *bus = dsa_host_dev_to_mii_bus(host_dev);
 	int ret;
@@ -275,7 +289,7 @@ static void mv88e6060_poll_link(struct dsa_switch *ds)
 
 static struct dsa_switch_driver mv88e6060_switch_driver = {
 	.tag_protocol	= DSA_TAG_PROTO_TRAILER,
-	.probe		= mv88e6060_probe,
+	.probe		= mv88e6060_switch_probe,
 	.setup		= mv88e6060_setup,
 	.set_addr	= mv88e6060_set_addr,
 	.phy_read	= mv88e6060_phy_read,
@@ -283,16 +297,106 @@ static struct dsa_switch_driver mv88e6060_switch_driver = {
 	.poll_link	= mv88e6060_poll_link,
 };
 
+static int mv88e6060_probe(struct phy_device *phydev)
+{
+	struct dsa_switch *ds;
+
+	ds = dsa_alloc_switch(0);
+	if (!ds)
+		return -ENOMEM;
+
+	phydev->priv = ds;
+	phydev->is_switch = true;
+	ds->master_dev = &phydev->dev;
+	ds->tag_protocol = DSA_TAG_PROTO_TRAILER;
+	ds->drv = &mv88e6060_switch_driver;
+
+	return 0;
+}
+
+static void mv88e6060_remove(struct phy_device *phydev)
+{
+	struct dsa_switch *ds = phydev->priv;
+
+	dsa_switch_unregister(ds);
+	phydev->priv = NULL;
+}
+
+static int mv88e6060_config_init(struct phy_device *phydev)
+{
+	struct dsa_switch *ds = phydev->priv;
+
+	return dsa_switch_register_phydev(ds, phydev);
+}
+
+static int mv88e6060_config_aneg(struct phy_device *phydev)
+{
+	return 0;
+}
+
+/* Called by the network device driver attached to this PHY
+ * we need to reflect the CPU port status here since the Ethernet MAC will
+ * use those PHY device link parameters for its configuration
+ */
+static int mv88e6060_read_status(struct phy_device *phydev)
+{
+	phydev->speed = 100;
+	phydev->duplex = DUPLEX_FULL;
+	phydev->link = 1;
+	phydev->state = PHY_RUNNING;
+
+	netif_carrier_on(phydev->attached_dev);
+	phydev->adjust_link(phydev->attached_dev);
+
+	return 0;
+}
+
+/* Read the switch identifier register using the special Port register, and if
+ * successful, override the PHY ID for this device
+ */
+static int mv88e6060_phy_fixup(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Marvell switches should be accessed using MDIO address 16 */
+	if (phydev->addr != 16)
+		return 0;
+
+	ret = mdiobus_read(phydev->bus, REG_PORT(0), 0x03) &
+			   MV88E6060_MAGIC_MASK;
+	if (ret != MV88E6060_MAGIC)
+		return 0;
+
+	phydev->phy_id = MV88E6060_MAGIC;
+
+	return 0;
+}
+
+static struct phy_driver mv88e6060_phy_driver = {
+	.phy_id		= MV88E6060_MAGIC,
+	.name		= "Marvell 88E6060",
+	.phy_id_mask	= 0xffffffff,
+	.features	= PHY_BASIC_FEATURES,
+	.probe		= mv88e6060_probe,
+	.remove		= mv88e6060_remove,
+	.config_aneg	= mv88e6060_config_aneg,
+	.config_init	= mv88e6060_config_init,
+	.read_status	= mv88e6060_read_status,
+	.driver		= { .owner = THIS_MODULE, },
+};
+
 static int __init mv88e6060_init(void)
 {
 	register_switch_driver(&mv88e6060_switch_driver);
-	return 0;
+	phy_register_fixup_for_id(PHY_ANY_ID, mv88e6060_phy_fixup);
+	return phy_driver_register(&mv88e6060_phy_driver);
 }
 module_init(mv88e6060_init);
 
 static void __exit mv88e6060_cleanup(void)
 {
 	unregister_switch_driver(&mv88e6060_switch_driver);
+	phy_driver_unregister(&mv88e6060_phy_driver);
 }
 module_exit(mv88e6060_cleanup);
 
-- 
2.1.0

Re: [RFC PATCH net-next 7/8] net: dsa: mv88e6060: make it a proper PHY driver

[Andrew Lunn]

> +/* Read the switch identifier register using the special Port register, and if
> + * successful, override the PHY ID for this device
> + */
> +static int mv88e6060_phy_fixup(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* Marvell switches should be accessed using MDIO address 16 */
> +	if (phydev->addr != 16)
> +		return 0;
> +
> +	ret = mdiobus_read(phydev->bus, REG_PORT(0), 0x03) &
> +			   MV88E6060_MAGIC_MASK;
> +	if (ret != MV88E6060_MAGIC)
> +		return 0;
> +
> +	phydev->phy_id = MV88E6060_MAGIC;
> +
> +	return 0;
> +}

Hi Florian

The 6060 datasheet is the only public one. It talks a little bit about
this. The switch can either use addresses 0-15, or 16-31, depending on
EE_CLK/ADDR4 pin. So your first check needs expanding.

The other chips have two different addressing modes, depending on pins
at reset time. They can be similar to the 6060, using a large number
of registers over a number of MDIO addresses. Or they can use two
registers at a configurable MDIO address, with these two registers
being Operation and Data. You access all the real switch registers
indirectly. Probing such a setup could be destructive, you need to
perform writes to register address 0 and 1, which if it is a real phy,
not a switch, means writing to its control register and status
register. So i guess before doing this, we need to read phyid1 and
phyid2, and only do such a probe if we get 0xffff for both.

I guess we need to see how problematic this is, before we can modify
all the drivers to probe like this. I do have hardware using this
indirect mode, so i can do some testing.

    Andrew

Re: [RFC PATCH net-next 7/8] net: dsa: mv88e6060: make it a proper PHY driver

[Guenter Roeck]

On 04/29/2015 06:57 PM, Florian Fainelli wrote:
> Convert the Marvell 88E6060 switch driver into a proper PHY library
> driver that can be registered. To make sure we do not introduce
> functional changes, the PHY driver provides autoneg and status callbacks
> to make sure the attached Ethernet MAC driver still sees a link UP at
> the CPU port full speed.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   drivers/net/dsa/mv88e6060.c | 114 ++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 109 insertions(+), 5 deletions(-)
>

The whole complexity added here makes me wonder if we are really on the right track.
After all, switches are _not_ phy devices. Forcing them to register as phy devices
just because they use mdio and just because the Linux mdio implementation assumes
that anything connected to it is a phy seems odd.

A much better solution might be be to disconnect mdio from phy, ie to create a new
mdio bus framework, as then use this framework for anything connected to an mdio bus.

Does this make any sense, or am I completely off track ?

Thanks,
Guenter

Re: [RFC PATCH net-next 7/8] net: dsa: mv88e6060: make it a proper PHY driver

[Florian Fainelli]

On 30/04/15 06:49, Guenter Roeck wrote:
> On 04/29/2015 06:57 PM, Florian Fainelli wrote:
>> Convert the Marvell 88E6060 switch driver into a proper PHY library
>> driver that can be registered. To make sure we do not introduce
>> functional changes, the PHY driver provides autoneg and status callbacks
>> to make sure the attached Ethernet MAC driver still sees a link UP at
>> the CPU port full speed.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>   drivers/net/dsa/mv88e6060.c | 114
>> ++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 109 insertions(+), 5 deletions(-)
>>
> 
> The whole complexity added here makes me wonder if we are really on the
> right track.
> After all, switches are _not_ phy devices. Forcing them to register as
> phy devices
> just because they use mdio and just because the Linux mdio
> implementation assumes
> that anything connected to it is a phy seems odd.

The net advantage I see with this approach is that currently, with DSA,
you get to do the following:

- register a "dsa" platform device
- force your CPU Ethernet MAC to hardcode link parameters, either via a
"fixed PHY" or via custom settings (ala mv643xx_eth)
- probing needs to occur in a *very* specific order: MDIO first,
Ethernet device second, DSA last

Now, with this patchset, you don't need to have DSA awareness in
anything but the "provider" driver which in this case is a PHY driver
because it sits on the MDIO bus (see below on that topic) and things
tend to "flow" a bit more naturally one you do that.

> 
> A much better solution might be be to disconnect mdio from phy, ie to
> create a new
> mdio bus framework, as then use this framework for anything connected to
> an mdio bus.

So essentially end-up creating a separate class of MDIO addressable
devices which are switches? I guess we could try to do that. We would
still have to update existing Ethernet MAC drivers which speak phylib to
know what to do with their CPU port and set correct link parameters,
would we want to create a special PHY drivers for these, just for the
CPU port?

> 
> Does this make any sense, or am I completely off track ?

I think this is very valid, my main point behind this entire patch
series is to divorce from the special "dsa" platform_device for common
cases: MDIO or MMIO connected switches to a SoC, because it is
convoluted and prevents a lot of things from being done: module
unloading, proper device reference (and parenting), need for "out of
band" link information to be provided to unmodified Ethernet drivers etc...
-- 
Florian

Re: [RFC PATCH net-next 7/8] net: dsa: mv88e6060: make it a proper PHY driver

[Andrew Lunn]

> The net advantage I see with this approach is that currently, with DSA,
> you get to do the following:
> 
> - register a "dsa" platform device
> - force your CPU Ethernet MAC to hardcode link parameters, either via a
> "fixed PHY" or via custom settings (ala mv643xx_eth)
> - probing needs to occur in a *very* specific order: MDIO first,
> Ethernet device second, DSA last

I'm not sure this is true. I'm pretty sure i've seen DSA return with
-EPROBE_DEFER and try again later.

      Andrew

Re: [RFC PATCH net-next 7/8] net: dsa: mv88e6060: make it a proper PHY driver

[Florian Fainelli]

On 30/04/15 10:02, Andrew Lunn wrote:
>> The net advantage I see with this approach is that currently, with DSA,
>> you get to do the following:
>>
>> - register a "dsa" platform device
>> - force your CPU Ethernet MAC to hardcode link parameters, either via a
>> "fixed PHY" or via custom settings (ala mv643xx_eth)
>> - probing needs to occur in a *very* specific order: MDIO first,
>> Ethernet device second, DSA last
> 
> I'm not sure this is true. I'm pretty sure i've seen DSA return with
> -EPROBE_DEFER and try again later.

Right, I fixed that a little while ago, but that still prevents all the
scenarios described in my reply, in particular module unloading is
completely hairy, see this attempt:

http://permalink.gmane.org/gmane.linux.network/345803
-- 
Florian

Re: [RFC PATCH net-next 7/8] net: dsa: mv88e6060: make it a proper PHY driver

[Guenter Roeck]

On 04/30/2015 09:46 AM, Florian Fainelli wrote:
> On 30/04/15 06:49, Guenter Roeck wrote:
>> On 04/29/2015 06:57 PM, Florian Fainelli wrote:
>>> Convert the Marvell 88E6060 switch driver into a proper PHY library
>>> driver that can be registered. To make sure we do not introduce
>>> functional changes, the PHY driver provides autoneg and status callbacks
>>> to make sure the attached Ethernet MAC driver still sees a link UP at
>>> the CPU port full speed.
>>>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>>    drivers/net/dsa/mv88e6060.c | 114
>>> ++++++++++++++++++++++++++++++++++++++++++--
>>>    1 file changed, 109 insertions(+), 5 deletions(-)
>>>
>>
>> The whole complexity added here makes me wonder if we are really on the
>> right track.
>> After all, switches are _not_ phy devices. Forcing them to register as
>> phy devices
>> just because they use mdio and just because the Linux mdio
>> implementation assumes
>> that anything connected to it is a phy seems odd.
>
> The net advantage I see with this approach is that currently, with DSA,
> you get to do the following:
>
> - register a "dsa" platform device
> - force your CPU Ethernet MAC to hardcode link parameters, either via a
> "fixed PHY" or via custom settings (ala mv643xx_eth)
> - probing needs to occur in a *very* specific order: MDIO first,
> Ethernet device second, DSA last
>
> Now, with this patchset, you don't need to have DSA awareness in
> anything but the "provider" driver which in this case is a PHY driver
> because it sits on the MDIO bus (see below on that topic) and things
> tend to "flow" a bit more naturally one you do that.
>

See, that is where I have the problem. The switch connects to the MDIO bus,
and the Linux infrastructure therefore declares that it has to be a PHY
chip, and that one must implement a phy driver to connect to it.

But it isn't a PHY we are dealing with. It is a switch chip.

>>
>> A much better solution might be be to disconnect mdio from phy, ie to
>> create a new
>> mdio bus framework, as then use this framework for anything connected to
>> an mdio bus.
>
> So essentially end-up creating a separate class of MDIO addressable
> devices which are switches? I guess we could try to do that. We would

Yes, that would be the idea. Instead of declaring that everything
connected to MDIO must by definition be a PHY chip (and thus implement
a phy driver), we could have a MDIO infrastructure and different types
of devices connect to it (such as phy and switch chips, and whatever
else may be out there that uses MDIO).

In a way this is similar to other busses, such as I2C or SPI or PCIe.
To me, modeling the switch driver as phy driver is kind of similar
to modeling all I2C drivers as, say, EEPROMs.

> still have to update existing Ethernet MAC drivers which speak phylib to
> know what to do with their CPU port and set correct link parameters,
> would we want to create a special PHY drivers for these, just for the
> CPU port?
>
Not sure I follow you here.

>>
>> Does this make any sense, or am I completely off track ?
>
> I think this is very valid, my main point behind this entire patch
> series is to divorce from the special "dsa" platform_device for common
> cases: MDIO or MMIO connected switches to a SoC, because it is
> convoluted and prevents a lot of things from being done: module
> unloading, proper device reference (and parenting), need for "out of
> band" link information to be provided to unmodified Ethernet drivers etc...
>

Oh, I absolutely agree that the current model has problems. I am just not
happy about modeling the switch drivers as phy driver.

Thanks,
Guenter

Re: [RFC PATCH net-next 7/8] net: dsa: mv88e6060: make it a proper PHY driver

[Florian Fainelli]

Le 04/30/15 19:28, Guenter Roeck a écrit :
> On 04/30/2015 09:46 AM, Florian Fainelli wrote:
>> On 30/04/15 06:49, Guenter Roeck wrote:
>>> On 04/29/2015 06:57 PM, Florian Fainelli wrote:
>>>> Convert the Marvell 88E6060 switch driver into a proper PHY library
>>>> driver that can be registered. To make sure we do not introduce
>>>> functional changes, the PHY driver provides autoneg and status
>>>> callbacks
>>>> to make sure the attached Ethernet MAC driver still sees a link UP at
>>>> the CPU port full speed.
>>>>
>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>> ---
>>>>    drivers/net/dsa/mv88e6060.c | 114
>>>> ++++++++++++++++++++++++++++++++++++++++++--
>>>>    1 file changed, 109 insertions(+), 5 deletions(-)
>>>>
>>>
>>> The whole complexity added here makes me wonder if we are really on the
>>> right track.
>>> After all, switches are _not_ phy devices. Forcing them to register as
>>> phy devices
>>> just because they use mdio and just because the Linux mdio
>>> implementation assumes
>>> that anything connected to it is a phy seems odd.
>>
>> The net advantage I see with this approach is that currently, with DSA,
>> you get to do the following:
>>
>> - register a "dsa" platform device
>> - force your CPU Ethernet MAC to hardcode link parameters, either via a
>> "fixed PHY" or via custom settings (ala mv643xx_eth)
>> - probing needs to occur in a *very* specific order: MDIO first,
>> Ethernet device second, DSA last
>>
>> Now, with this patchset, you don't need to have DSA awareness in
>> anything but the "provider" driver which in this case is a PHY driver
>> because it sits on the MDIO bus (see below on that topic) and things
>> tend to "flow" a bit more naturally one you do that.
>>
> 
> See, that is where I have the problem. The switch connects to the MDIO bus,
> and the Linux infrastructure therefore declares that it has to be a PHY
> chip, and that one must implement a phy driver to connect to it.
> 
> But it isn't a PHY we are dealing with. It is a switch chip.
> 
>>>
>>> A much better solution might be be to disconnect mdio from phy, ie to
>>> create a new
>>> mdio bus framework, as then use this framework for anything connected to
>>> an mdio bus.
>>
>> So essentially end-up creating a separate class of MDIO addressable
>> devices which are switches? I guess we could try to do that. We would
> 
> Yes, that would be the idea. Instead of declaring that everything
> connected to MDIO must by definition be a PHY chip (and thus implement
> a phy driver), we could have a MDIO infrastructure and different types
> of devices connect to it (such as phy and switch chips, and whatever
> else may be out there that uses MDIO).

Ok.

> 
> In a way this is similar to other busses, such as I2C or SPI or PCIe.
> To me, modeling the switch driver as phy driver is kind of similar
> to modeling all I2C drivers as, say, EEPROMs.

Well that is true to some degree, switches do typically expose real PHYs
for their individual non-CPU ports, so they are kind of enhanced PHY
devices in some sense, but yet not quite full-featured Ethernet MACs as
well.

> 
>> still have to update existing Ethernet MAC drivers which speak phylib to
>> know what to do with their CPU port and set correct link parameters,
>> would we want to create a special PHY drivers for these, just for the
>> CPU port?
>>
> Not sure I follow you here.

If you have an Ethernet MAC driver, e.g: mv643xx_eth, bcmgenet, mvneta
or others which are already using the PHY library via of_phy_connect()
for instance; if your switch driver is implemented as a PHY driver, you
get to discover this switch pretty much for free, just like if it was a
regular PHY. That is why I used this approach, and what others have done
in OpenWrt as well, such that you can have mostly unmodified Ethernet
drivers transparently use MDIO-connected switches just like they would
do with an Ethernet PHY.

If we do not do that, which is certainly fine, I am more worried about
having to patch every single driver that knows phylib and teach them how
to speak to switches, a different class of devices. Maybe the PHY
library is pushing the abstraction a little to far, and what we need is
something that looks like the "old" style MII bus code that sb1250-mac.c
has for instance?

> 
>>>
>>> Does this make any sense, or am I completely off track ?
>>
>> I think this is very valid, my main point behind this entire patch
>> series is to divorce from the special "dsa" platform_device for common
>> cases: MDIO or MMIO connected switches to a SoC, because it is
>> convoluted and prevents a lot of things from being done: module
>> unloading, proper device reference (and parenting), need for "out of
>> band" link information to be provided to unmodified Ethernet drivers
>> etc...
>>
> 
> Oh, I absolutely agree that the current model has problems. I am just not
> happy about modeling the switch drivers as phy driver.
> 
> Thanks,
> Guenter
> 


-- 
Florian

[RFC PATCH net-next 8/8] net: dsa: mv88e6xxx: Allow them to be proper PHY drivers

[Florian Fainelli]

Register a PHY fixup for the different MV88E6xxx switches supported by
the 88e6xxxx driver, basically replacing the legacy probe function.

Since these are all now a PHY driver, the Ethernet MAC controller attached
to it needs to be signaled proper link parameters (speed, duplex, link)

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/mv88e6123_61_65.c | 116 ++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6131.c       | 107 +++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6171.c       |  95 +++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6352.c       | 102 +++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx.c       |  40 ++++++++++++-
 drivers/net/dsa/mv88e6xxx.h       |  12 ++++
 6 files changed, 470 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mv88e6123_61_65.c b/drivers/net/dsa/mv88e6123_61_65.c
index b4af6d5aff7c..acd0e08d6b21 100644
--- a/drivers/net/dsa/mv88e6123_61_65.c
+++ b/drivers/net/dsa/mv88e6123_61_65.c
@@ -296,6 +296,122 @@ struct dsa_switch_driver mv88e6123_61_65_switch_driver = {
 	.get_regs		= mv88e6xxx_get_regs,
 };
 
+/* PHY library read status callback, must reflect the CPU port link parameters
+ * towards the attached Ethernet MAC driver
+ */
+static int mv88e6123_61_65_read_status(struct phy_device *phydev)
+{
+	phydev->link = 1;
+	phydev->speed = SPEED_1000;
+	phydev->duplex = DUPLEX_FULL;
+	phydev->state = PHY_RUNNING;
+
+	netif_carrier_on(phydev->attached_dev);
+	phydev->adjust_link(phydev->attached_dev);
+
+	return 0;
+}
+
+static int mv88e6123_61_65_phy_probe(struct phy_device *phydev)
+{
+	struct dsa_switch *ds;
+
+	ds = dsa_alloc_switch(sizeof(struct mv88e6xxx_priv_state));
+	if (!ds)
+		return -ENOMEM;
+
+	phydev->priv = ds;
+	phydev->is_switch = true;
+	ds->drv = &mv88e6123_61_65_switch_driver;
+	ds->tag_protocol = DSA_TAG_PROTO_EDSA;
+	ds->master_dev = &phydev->dev;
+
+	return 0;
+}
+
+static void mv88e6123_61_65_remove(struct phy_device *phydev)
+{
+	struct dsa_switch *ds = phydev->priv;
+
+	dsa_switch_unregister(ds);
+	kfree(ds);
+	phydev->priv = NULL;
+}
+
+static int mv88e6123_61_65_phy_fixup(struct phy_device *phydev)
+{
+	int ret;
+
+	if (phydev->addr != 16)
+		return 0;
+
+	ret = __mv88e6xxx_reg_read(phydev->bus, phydev->addr, REG_PORT(0), 0x03);
+	if (ret < 0)
+		return 0;
+
+	switch (ret) {
+	case PORT_SWITCH_ID_6123_A1:
+	case PORT_SWITCH_ID_6123_A2:
+	case PORT_SWITCH_ID_6161_A1:
+	case PORT_SWITCH_ID_6161_A2:
+	case PORT_SWITCH_ID_6165_A1:
+	case PORT_SWITCH_ID_6165_A2:
+		phydev->phy_id = ret;
+		break;
+	default:
+		ret &= 0xfff0;
+		switch (ret) {
+		case PORT_SWITCH_ID_6123:
+		case PORT_SWITCH_ID_6161:
+		case PORT_SWITCH_ID_6165:
+			phydev->phy_id = ret;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+#define MV88E61XX_DRV(_magic, _name)			\
+{							\
+	.phy_id		= (_magic),			\
+	.phy_id_mask	= 0xffffffff,			\
+	.name		= (_name),			\
+	.features	= PHY_GBIT_FEATURES,		\
+	.config_init	= mv88e6xxx_config_init,	\
+	.config_aneg	= mv88e6xxx_config_aneg,	\
+	.read_status	= mv88e6123_61_65_read_status,	\
+	.probe		= mv88e6123_61_65_phy_probe,	\
+	.remove		= mv88e6123_61_65_remove,	\
+	.driver		= { .owner = THIS_MODULE },	\
+}
+
+static struct phy_driver mv88e6123_61_65_phy_drivers[] = {
+	MV88E61XX_DRV(PORT_SWITCH_ID_6123_A1, "Marvell 88E6123 (A1)"),
+	MV88E61XX_DRV(PORT_SWITCH_ID_6123_A2, "Marvell 88E6123 (A2)"),
+	MV88E61XX_DRV(PORT_SWITCH_ID_6123, "Marvell 88E6123"),
+	MV88E61XX_DRV(PORT_SWITCH_ID_6161_A1, "Marvell 88E6161 (A1)"),
+	MV88E61XX_DRV(PORT_SWITCH_ID_6161_A2, "Marvell 88E6161 (A2)"),
+	MV88E61XX_DRV(PORT_SWITCH_ID_6161, "Marvell 88E6161"),
+	MV88E61XX_DRV(PORT_SWITCH_ID_6165_A1, "Marvell 88E6165 (A1)"),
+	MV88E61XX_DRV(PORT_SWITCH_ID_6165_A2, "Marvell 88E6165 (A2)"),
+	MV88E61XX_DRV(PORT_SWITCH_ID_6165, "Marvell 88E6165"),
+};
+
+int __init mv88e6123_61_65_phy_drivers_register(void)
+{
+	phy_register_fixup_for_id(PHY_ANY_ID, mv88e6123_61_65_phy_fixup);
+
+	return phy_drivers_register(mv88e6123_61_65_phy_drivers,
+				    ARRAY_SIZE(mv88e6123_61_65_phy_drivers));
+}
+
+void __exit mv88e6123_61_65_phy_drivers_unregister(void)
+{
+	phy_drivers_unregister(mv88e6123_61_65_phy_drivers,
+			       ARRAY_SIZE(mv88e6123_61_65_phy_drivers));
+}
+
 MODULE_ALIAS("platform:mv88e6123");
 MODULE_ALIAS("platform:mv88e6161");
 MODULE_ALIAS("platform:mv88e6165");
diff --git a/drivers/net/dsa/mv88e6131.c b/drivers/net/dsa/mv88e6131.c
index e54824fa0d95..e60e3e3a2e65 100644
--- a/drivers/net/dsa/mv88e6131.c
+++ b/drivers/net/dsa/mv88e6131.c
@@ -313,6 +313,113 @@ struct dsa_switch_driver mv88e6131_switch_driver = {
 	.get_sset_count		= mv88e6xxx_get_sset_count,
 };
 
+static int mv88e6131_read_status(struct phy_device *phydev)
+{
+	struct dsa_switch *ds = phydev->priv;
+	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+
+	phydev->link = 1;
+	phydev->duplex = DUPLEX_FULL;
+	if (ps->id == PORT_SWITCH_ID_6085)
+		phydev->speed = SPEED_100;
+	else
+		phydev->speed = SPEED_1000;
+	phydev->state = PHY_RUNNING;
+
+	netif_carrier_on(phydev->attached_dev);
+	phydev->adjust_link(phydev->attached_dev);
+
+	return 0;
+}
+
+static int mv88e6131_phy_probe(struct phy_device *phydev)
+{
+	struct dsa_switch *ds;
+
+	ds = dsa_alloc_switch(sizeof(struct mv88e6xxx_priv_state));
+	if (!ds)
+		return -ENOMEM;
+
+	phydev->priv = ds;
+	phydev->is_switch = true;
+	ds->master_dev = &phydev->dev;
+	ds->drv = &mv88e6131_switch_driver;
+	ds->tag_protocol = DSA_TAG_PROTO_DSA;
+
+	return 0;
+}
+
+static void mv88e6131_remove(struct phy_device *phydev)
+{
+	struct dsa_switch *ds = phydev->priv;
+
+	dsa_switch_unregister(ds);
+	kfree(ds);
+	phydev->priv = NULL;
+}
+
+static int mv88e6131_phy_fixup(struct phy_device *phydev)
+{
+	int ret;
+
+	if (phydev->addr != 16)
+		return 0;
+
+	ret = __mv88e6xxx_reg_read(phydev->bus, phydev->addr, REG_PORT(0), 0x03);
+	if (ret < 0)
+		return 0;
+
+	if (ret == PORT_SWITCH_ID_6131_B2) {
+		phydev->phy_id = ret;
+		return 0;
+	}
+
+	ret &= 0xfff0;
+	switch (ret) {
+	case PORT_SWITCH_ID_6085:
+	case PORT_SWITCH_ID_6095:
+	case PORT_SWITCH_ID_6131:
+		phydev->phy_id = ret;
+		break;
+	}
+
+	return 0;
+}
+
+#define MV88E61XX_DRV(_magic, _feat, _name)		\
+{							\
+	.phy_id		= (_magic),			\
+	.phy_id_mask	= 0xffffffff,			\
+	.name		= _name,			\
+	.features	= (_feat),			\
+	.config_init	= mv88e6xxx_config_init,	\
+	.config_aneg	= mv88e6xxx_config_aneg,	\
+	.read_status	= mv88e6131_read_status,	\
+	.probe		= mv88e6131_phy_probe,		\
+	.remove		= mv88e6131_remove,		\
+	.driver		= { .owner = THIS_MODULE },	\
+}
+static struct phy_driver mv88e6131_phy_drivers[] = {
+	MV88E61XX_DRV(PORT_SWITCH_ID_6085, PHY_BASIC_FEATURES, "Marvell 88E6085"),
+	MV88E61XX_DRV(PORT_SWITCH_ID_6095, PHY_GBIT_FEATURES, "Marvell 88E6095/88E6095F"),
+	MV88E61XX_DRV(PORT_SWITCH_ID_6131_B2, PHY_GBIT_FEATURES, "Marvell 88E6131 (B2)"),
+	MV88E61XX_DRV(PORT_SWITCH_ID_6131, PHY_GBIT_FEATURES, "Marvell 88E6131"),
+};
+
+int __init mv88e6131_phy_drivers_register(void)
+{
+	phy_register_fixup_for_id(PHY_ANY_ID, mv88e6131_phy_fixup);
+
+	return phy_drivers_register(mv88e6131_phy_drivers,
+				    ARRAY_SIZE(mv88e6131_phy_drivers));
+}
+
+void __exit mv88e6131_phy_drivers_unregister(void)
+{
+	phy_drivers_unregister(mv88e6131_phy_drivers,
+			       ARRAY_SIZE(mv88e6131_phy_drivers));
+}
+
 MODULE_ALIAS("platform:mv88e6085");
 MODULE_ALIAS("platform:mv88e6095");
 MODULE_ALIAS("platform:mv88e6095f");
diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c
index 9104efea0e3e..120a0f46e321 100644
--- a/drivers/net/dsa/mv88e6171.c
+++ b/drivers/net/dsa/mv88e6171.c
@@ -307,5 +307,100 @@ struct dsa_switch_driver mv88e6171_switch_driver = {
 	.fdb_getnext		= mv88e6xxx_port_fdb_getnext,
 };
 
+static int mv88e6171_read_status(struct phy_device *phydev)
+{
+	phydev->link = 1;
+	phydev->duplex = DUPLEX_FULL;
+	phydev->state = PHY_RUNNING;
+
+	netif_carrier_on(phydev->attached_dev);
+	phydev->adjust_link(phydev->attached_dev);
+
+	return 0;
+}
+
+static int mv88e6171_phy_probe(struct phy_device *phydev)
+{
+	struct dsa_switch *ds;
+
+	ds = dsa_alloc_switch(sizeof(struct mv88e6xxx_priv_state));
+	if (!ds)
+		return -ENOMEM;
+
+	/* We cannot register the switch yet, since we do not have an attached
+	 * network device
+	 */
+	phydev->is_switch = true;
+	phydev->priv = ds;
+	ds->master_dev = &phydev->dev;
+	ds->drv = &mv88e6171_switch_driver;
+	ds->tag_protocol = DSA_TAG_PROTO_EDSA;
+
+	return 0;
+}
+
+static void mv88e6171_remove(struct phy_device *phydev)
+{
+	struct dsa_switch *ds = phydev->priv;
+
+	dsa_switch_unregister(ds);
+	kfree(ds);
+	phydev->priv = NULL;
+}
+
+static int mv88e6171_phy_fixup(struct phy_device *phydev)
+{
+	int ret;
+
+	if (phydev->addr != 16)
+		return 0;
+
+	ret = __mv88e6xxx_reg_read(phydev->bus, phydev->addr, REG_PORT(0), 0x03);
+	if (ret < 0)
+		return 0;
+
+	ret &= 0xfff0;
+	switch (ret) {
+	case PORT_SWITCH_ID_6171:
+	case PORT_SWITCH_ID_6172:
+		phydev->phy_id = ret;
+		break;
+	}
+
+	return 0;
+}
+
+#define MV88E6171_DRV(_magic, _name)			\
+{							\
+	.phy_id		= (_magic),			\
+	.phy_id_mask	= 0xfffffff0,			\
+	.name		= _name,			\
+	.features	= PHY_GBIT_FEATURES,		\
+	.config_init	= mv88e6xxx_config_init,	\
+	.config_aneg	= mv88e6xxx_config_aneg,	\
+	.read_status	= mv88e6171_read_status,	\
+	.probe		= mv88e6171_phy_probe,		\
+	.remove		= mv88e6171_remove,		\
+	.driver		= { .owner = THIS_MODULE },	\
+}
+static struct phy_driver mv88e6171_phy_drivers[] = {
+	MV88E6171_DRV(PORT_SWITCH_ID_6171, "Marvell 88E6171"),
+	MV88E6171_DRV(PORT_SWITCH_ID_6172, "Marvell 88E6172"),
+};
+
+int __init mv88e6171_phy_drivers_register(void)
+{
+	phy_register_fixup_for_id(PHY_ANY_ID, mv88e6171_phy_fixup);
+
+	return phy_drivers_register(mv88e6171_phy_drivers,
+				    ARRAY_SIZE(mv88e6171_phy_drivers));
+}
+
+void __exit mv88e6171_phy_drivers_unregister(void)
+{
+	phy_drivers_unregister(mv88e6171_phy_drivers,
+			       ARRAY_SIZE(mv88e6171_phy_drivers));
+}
+
 MODULE_ALIAS("platform:mv88e6171");
 MODULE_ALIAS("platform:mv88e6172");
diff --git a/drivers/net/dsa/mv88e6352.c b/drivers/net/dsa/mv88e6352.c
index 126c11b81e75..e7198edddd13 100644
--- a/drivers/net/dsa/mv88e6352.c
+++ b/drivers/net/dsa/mv88e6352.c
@@ -551,4 +551,106 @@ struct dsa_switch_driver mv88e6352_switch_driver = {
 	.fdb_getnext		= mv88e6xxx_port_fdb_getnext,
 };
 
+static int mv88e6352_read_status(struct phy_device *phydev)
+{
+	phydev->link = 1;
+	phydev->duplex = DUPLEX_FULL;
+	phydev->speed = SPEED_1000;
+	phydev->state = PHY_RUNNING;
+
+	netif_carrier_on(phydev->attached_dev);
+	phydev->adjust_link(phydev->attached_dev);
+
+	return 0;
+}
+
+static int mv88e6352_phy_probe(struct phy_device *phydev)
+{
+	struct dsa_switch *ds;
+
+	ds = dsa_alloc_switch(sizeof(struct mv88e6xxx_priv_state));
+	if (!ds)
+		return -ENOMEM;
+
+	phydev->priv = ds;
+	phydev->is_switch = true;
+	ds->master_dev = &phydev->dev;
+	ds->drv = &mv88e6352_switch_driver;
+	ds->tag_protocol = DSA_TAG_PROTO_EDSA;
+
+	return 0;
+}
+
+static void mv88e6352_remove(struct phy_device *phydev)
+{
+	struct dsa_switch *ds = phydev->priv;
+
+	dsa_switch_unregister(ds);
+	kfree(ds);
+	phydev->priv = NULL;
+}
+
+static int mv88e6352_phy_fixup(struct phy_device *phydev)
+{
+	int ret;
+
+	if (phydev->addr != 16)
+		return 0;
+
+	ret = __mv88e6xxx_reg_read(phydev->bus, phydev->addr, REG_PORT(0), 0x03);
+	if (ret < 0)
+		return 0;
+
+	switch (ret) {
+	case PORT_SWITCH_ID_6352_A0:
+	case PORT_SWITCH_ID_6352_A1:
+		phydev->phy_id = ret;
+		return 0;
+	}
+
+	ret &= 0xfff0;
+	switch (ret) {
+	case PORT_SWITCH_ID_6176:
+	case PORT_SWITCH_ID_6352:
+		phydev->phy_id = ret;
+		break;
+	}
+
+	return 0;
+}
+#define MV88E6352_DRV(_magic, _name)			\
+{							\
+	.phy_id		= (_magic),			\
+	.phy_id_mask	= 0xffffffff,			\
+	.name		= _name,			\
+	.features	= PHY_GBIT_FEATURES,		\
+	.config_init	= mv88e6xxx_config_init,	\
+	.config_aneg	= mv88e6xxx_config_aneg,	\
+	.read_status	= mv88e6352_read_status,	\
+	.probe		= mv88e6352_phy_probe,		\
+	.remove		= mv88e6352_remove,		\
+	.driver		= { .owner = THIS_MODULE },	\
+}
+
+static struct phy_driver mv88e6352_phy_drivers[] = {
+	MV88E6352_DRV(PORT_SWITCH_ID_6176, "Marvell 88E6176"),
+	MV88E6352_DRV(PORT_SWITCH_ID_6352_A0, "Marvell 88E6352 (A0)"),
+	MV88E6352_DRV(PORT_SWITCH_ID_6352_A1, "Marvell 88E6352 (A1)"),
+	MV88E6352_DRV(PORT_SWITCH_ID_6352, "Marvell 88E6352"),
+};
+
+int __init mv88e6352_phy_drivers_register(void)
+{
+	phy_register_fixup_for_id(PHY_ANY_ID, mv88e6352_phy_fixup);
+
+	return phy_drivers_register(mv88e6352_phy_drivers,
+				    ARRAY_SIZE(mv88e6352_phy_drivers));
+}
+
+void __exit mv88e6352_phy_drivers_unregister(void)
+{
+	phy_drivers_unregister(mv88e6352_phy_drivers,
+			       ARRAY_SIZE(mv88e6352_phy_drivers));
+}
+
 MODULE_ALIAS("platform:mv88e6352");
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index af639ab4c55b..c51c6f179682 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -75,10 +75,22 @@ int __mv88e6xxx_reg_read(struct mii_bus *bus, int sw_addr, int addr, int reg)
 	return ret & 0xffff;
 }
 
+static inline struct mii_bus *mv88e6xxx_mii_bus(struct dsa_switch *ds)
+{
+	struct phy_device *phydev;
+	if (ds->from_pd)
+		return dsa_host_dev_to_mii_bus(ds->master_dev);
+
+	phydev = to_phy_device(ds->master_dev);
+
+	return phydev->bus;
+}
+
+
 /* Must be called with SMI mutex held */
 static int _mv88e6xxx_reg_read(struct dsa_switch *ds, int addr, int reg)
 {
-	struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev);
+	struct mii_bus *bus = mv88e6xxx_mii_bus(ds);
 	int ret;
 
 	if (bus == NULL)
@@ -142,7 +154,7 @@ int __mv88e6xxx_reg_write(struct mii_bus *bus, int sw_addr, int addr,
 static int _mv88e6xxx_reg_write(struct dsa_switch *ds, int addr, int reg,
 				u16 val)
 {
-	struct mii_bus *bus = dsa_host_dev_to_mii_bus(ds->master_dev);
+	struct mii_bus *bus = mv88e6xxx_mii_bus(ds);
 
 	if (bus == NULL)
 		return -EINVAL;
@@ -1446,19 +1458,36 @@ mv88e6xxx_phy_write_indirect(struct dsa_switch *ds, int port, int regnum,
 	return ret;
 }
 
+int mv88e6xxx_config_init(struct phy_device *phydev)
+{
+	struct dsa_switch *ds = phydev->priv;
+
+	/* We now have an attached network device, register this device */
+	return dsa_switch_register_phydev(ds, phydev);
+}
+
+int mv88e6xxx_config_aneg(struct phy_device *phydev)
+{
+	return 0;
+}
+
 static int __init mv88e6xxx_init(void)
 {
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)
 	register_switch_driver(&mv88e6131_switch_driver);
+	mv88e6131_phy_drivers_register();
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6123_61_65)
 	register_switch_driver(&mv88e6123_61_65_switch_driver);
+	mv88e6123_61_65_phy_drivers_register();
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6352)
 	register_switch_driver(&mv88e6352_switch_driver);
+	mv88e6352_phy_drivers_register();
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6171)
 	register_switch_driver(&mv88e6171_switch_driver);
+	mv88e6171_phy_drivers_register();
 #endif
 	return 0;
 }
@@ -1468,12 +1497,19 @@ static void __exit mv88e6xxx_cleanup(void)
 {
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6171)
 	unregister_switch_driver(&mv88e6171_switch_driver);
+	mv88e6171_phy_drivers_unregister();
+#endif
+#if IS_ENABLED(CONFIG_NET_DSA_MV88E6352)
+	unregister_switch_driver(&mv88e6352_switch_driver);
+	mv88e6352_phy_drivers_unregister();
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6123_61_65)
 	unregister_switch_driver(&mv88e6123_61_65_switch_driver);
+	mv88e6123_61_65_phy_drivers_unregister();
 #endif
 #if IS_ENABLED(CONFIG_NET_DSA_MV88E6131)
 	unregister_switch_driver(&mv88e6131_switch_driver);
+	mv88e6131_phy_drivers_unregister();
 #endif
 }
 module_exit(mv88e6xxx_cleanup);
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index e045154f3364..449f948ca27f 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -315,6 +315,18 @@ extern struct dsa_switch_driver mv88e6123_61_65_switch_driver;
 extern struct dsa_switch_driver mv88e6352_switch_driver;
 extern struct dsa_switch_driver mv88e6171_switch_driver;
 
+int mv88e6xxx_config_init(struct phy_device *phydev);
+int mv88e6xxx_config_aneg(struct phy_device *phydev);
+
+int mv88e6123_61_65_phy_drivers_register(void);
+void mv88e6123_61_65_phy_drivers_unregister(void);
+int mv88e6131_phy_drivers_register(void);
+void mv88e6131_phy_drivers_unregister(void);
+int mv88e6352_phy_drivers_register(void);
+void mv88e6352_phy_drivers_unregister(void);
+int mv88e6171_phy_drivers_register(void);
+void mv88e6171_phy_drivers_unregister(void);
+
 #define REG_READ(addr, reg)						\
 	({								\
 		int __ret;						\
-- 
2.1.0

Re: [RFC PATCH net-next 0/8] net: dsa: New registration API

[Andrew Lunn]

> Note that there are currenlty no incompatibles changes made to existing Device
> Tree sources, rather, depending on the bus we are probed for, e.g: MDIO
> the dsa,mii-bus and dsa,ethernet phandles and first cell of the  "reg" property
> will become obsolete, everything else remains entirely compatible. 

Hi Florian

I'm not sure dsa,mii-bus and dsa,ethernet will become obsolete. At
least they are probably needed for multi switch setups, and the
possible but probably unlikely multi DSA setups.

You cannot assume that dsa,mii-bus and dsa,ethernet have the same
parent. In a multi switch setup, it could be there is an mdio-mux in
the picture. So all your probe really tells you, is that there is a
switch on this mii bus, but you don't know what ethernet it is hanging
off.

The switch could be hanging off multiple ethernets. I'm working on
supporting this for the WRT1900AC, where i use the bond driver on the
host side. So dsa,ethernet is a phandle to a bond interface.

The probe is likely to find all switches in a multi switch setup. But
i guess we only want the probe of the root devices in a switch tree to
cause a DSA setup.

So i think there needs to be some matching performed when looking in
the device tree. The dsa,mii-bus and address discovered by probing
need to match what is in the DSA properties.

     Andrew

Re: [RFC PATCH net-next 0/8] net: dsa: New registration API

[Florian Fainelli]

On 30/04/15 06:12, Andrew Lunn wrote:
>> Note that there are currenlty no incompatibles changes made to existing Device
>> Tree sources, rather, depending on the bus we are probed for, e.g: MDIO
>> the dsa,mii-bus and dsa,ethernet phandles and first cell of the  "reg" property
>> will become obsolete, everything else remains entirely compatible. 
> 
> Hi Florian
> 
> I'm not sure dsa,mii-bus and dsa,ethernet will become obsolete. At
> least they are probably needed for multi switch setups, and the
> possible but probably unlikely multi DSA setups.
> 
> You cannot assume that dsa,mii-bus and dsa,ethernet have the same
> parent. In a multi switch setup, it could be there is an mdio-mux in
> the picture. So all your probe really tells you, is that there is a
> switch on this mii bus, but you don't know what ethernet it is hanging
> off.

Good point.

> 
> The switch could be hanging off multiple ethernets. I'm working on
> supporting this for the WRT1900AC, where i use the bond driver on the
> host side. So dsa,ethernet is a phandle to a bond interface.

Humm, bond is a software constructs, don't you rather want two phandles
to the relevant Ethernet interfaces instead and then learn through
netlink/netdevice notifiers that these are eventually part of the same bond?

> 
> The probe is likely to find all switches in a multi switch setup. But
> i guess we only want the probe of the root devices in a switch tree to
> cause a DSA setup.

Right.

> 
> So i think there needs to be some matching performed when looking in
> the device tree. The dsa,mii-bus and address discovered by probing
> need to match what is in the DSA properties.

Sure, that's a good point. I tried to start with simple case first, if
you can recommend consumer/off the shelf hardware which has a cascaded
setup, that could help too.

The configuration I have access to with Broadcom switches looks like
this: internal SF2 switch MMIO-mapped, with an external RGMII interface
to a MDIO connected BCM53125 switch.

so this not a true switch tree here, they can (and should) be treated as
different switch trees, attached to different interfaces: eth0 for CPU
for the internal one and the DSA-created "rgmii" interface for the
second switch.
-- 
Florian

Re: [RFC PATCH net-next 0/8] net: dsa: New registration API

[Andrew Lunn]

On Thu, Apr 30, 2015 at 09:50:36AM -0700, Florian Fainelli wrote:
> On 30/04/15 06:12, Andrew Lunn wrote:
> >> Note that there are currenlty no incompatibles changes made to existing Device
> >> Tree sources, rather, depending on the bus we are probed for, e.g: MDIO
> >> the dsa,mii-bus and dsa,ethernet phandles and first cell of the  "reg" property
> >> will become obsolete, everything else remains entirely compatible. 
> > 
> > Hi Florian
> > 
> > I'm not sure dsa,mii-bus and dsa,ethernet will become obsolete. At
> > least they are probably needed for multi switch setups, and the
> > possible but probably unlikely multi DSA setups.
> > 
> > You cannot assume that dsa,mii-bus and dsa,ethernet have the same
> > parent. In a multi switch setup, it could be there is an mdio-mux in
> > the picture. So all your probe really tells you, is that there is a
> > switch on this mii bus, but you don't know what ethernet it is hanging
> > off.
> 
> Good point.
> 
> > 
> > The switch could be hanging off multiple ethernets. I'm working on
> > supporting this for the WRT1900AC, where i use the bond driver on the
> > host side. So dsa,ethernet is a phandle to a bond interface.
> 
> Humm, bond is a software constructs,

I thought about this for a while, and came to the conclusion that it
is often a software construct, but it can also be a hardware
construct, when you have two ethernet interfaces connected to a
switch, all on one PCB. So i added a DT binding for bonding. In the
case of WRT1900AC, it looks like:

       bond: bond {
               compatible = "linux,bond";
               slaves = < &eth0 >, < &eth1 >;
       };

and then in DSA i have a phandle to this bond interface. This part
works great, but i've not posted these patches yet, because it does
not work yet because of some other issue. Maybe when i do post this,
it will get shot down?

     Andrew

Re: [RFC PATCH net-next 0/8] net: dsa: New registration API

[Florian Fainelli]

On 30/04/15 10:27, Andrew Lunn wrote:
> On Thu, Apr 30, 2015 at 09:50:36AM -0700, Florian Fainelli wrote:
>> On 30/04/15 06:12, Andrew Lunn wrote:
>>>> Note that there are currenlty no incompatibles changes made to existing Device
>>>> Tree sources, rather, depending on the bus we are probed for, e.g: MDIO
>>>> the dsa,mii-bus and dsa,ethernet phandles and first cell of the  "reg" property
>>>> will become obsolete, everything else remains entirely compatible. 
>>>
>>> Hi Florian
>>>
>>> I'm not sure dsa,mii-bus and dsa,ethernet will become obsolete. At
>>> least they are probably needed for multi switch setups, and the
>>> possible but probably unlikely multi DSA setups.
>>>
>>> You cannot assume that dsa,mii-bus and dsa,ethernet have the same
>>> parent. In a multi switch setup, it could be there is an mdio-mux in
>>> the picture. So all your probe really tells you, is that there is a
>>> switch on this mii bus, but you don't know what ethernet it is hanging
>>> off.
>>
>> Good point.
>>
>>>
>>> The switch could be hanging off multiple ethernets. I'm working on
>>> supporting this for the WRT1900AC, where i use the bond driver on the
>>> host side. So dsa,ethernet is a phandle to a bond interface.
>>
>> Humm, bond is a software constructs,
> 
> I thought about this for a while, and came to the conclusion that it
> is often a software construct, but it can also be a hardware
> construct, when you have two ethernet interfaces connected to a
> switch, all on one PCB. So i added a DT binding for bonding. In the
> case of WRT1900AC, it looks like:
> 
>        bond: bond {
>                compatible = "linux,bond";
>                slaves = < &eth0 >, < &eth1 >;
>        };

It seems to me that what we would want to represent here, is strictly
the two "CPU" MACs mapping to their respective ports of the switch
(assuming they are on different ports, right?).

Then whether they are part of a bond, such that you can utilize the full
Wi-Fi throughput, is left to the user to configure that.

For instance, on the Linksys EA4500 I use for prototyping, there are two
mv643xx_eth instances connected to a LAN-facing 4 ports group, and the
other to the WAN, port 5 of the switch, this is similar to Mathieu's use
case I believe, and whether we want to have a WAN/LAN separation, or
have the two MACs participate in a LAN or WAN bond should be
user-configurable.

By the same token, we could create bridges for ports 0-3 aka LAN
directly from Device Tree ;)

> 
> and then in DSA i have a phandle to this bond interface. This part
> works great, but i've not posted these patches yet, because it does
> not work yet because of some other issue. Maybe when i do post this,
> it will get shot down?

I suppose you could post the patches and we look at this from there?
-- 
Florian

Re: [RFC PATCH net-next 0/8] net: dsa: New registration API

[Andrew Lunn]

> > I thought about this for a while, and came to the conclusion that it
> > is often a software construct, but it can also be a hardware
> > construct, when you have two ethernet interfaces connected to a
> > switch, all on one PCB. So i added a DT binding for bonding. In the
> > case of WRT1900AC, it looks like:
> > 
> >        bond: bond {
> >                compatible = "linux,bond";
> >                slaves = < &eth0 >, < &eth1 >;
> >        };
> 
> It seems to me that what we would want to represent here, is strictly
> the two "CPU" MACs mapping to their respective ports of the switch
> (assuming they are on different ports, right?).

Yes, different ports.
 
> Then whether they are part of a bond, such that you can utilize the full
> Wi-Fi throughput, is left to the user to configure that.
> 
> For instance, on the Linksys EA4500 I use for prototyping, there are two
> mv643xx_eth instances connected to a LAN-facing 4 ports group, and the
> other to the WAN, port 5 of the switch, this is similar to Mathieu's use
> case I believe, and whether we want to have a WAN/LAN separation, or
> have the two MACs participate in a LAN or WAN bond should be
> user-configurable.

Think about how you configure that today using DSA? You somehow need
to tell the switch that it should not use port 5, leave it enabled but
dumb, and the same for the second CPU port.

Bonding actually makes this simpler. With DSA you have your lan0-lan3
interfaces, and your wan0 interface. These are logically separate. You
then create a bridge, enslave lan0-lan3, and let the new hardware
offloading of bridging do the work.

> 
> By the same token, we could create bridges for ports 0-3 aka LAN
> directly from Device Tree ;)
> 
> > 
> > and then in DSA i have a phandle to this bond interface. This part
> > works great, but i've not posted these patches yet, because it does
> > not work yet because of some other issue. Maybe when i do post this,
> > it will get shot down?
> 
> I suppose you could post the patches and we look at this from there?

Well, i want to wait until it actually works. Having two CPU ports is
not mentioned in anywhere in the Marvell SDK.

    Andrew