Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable

[Andrew Lunn]

> Historical reasons mostly. Considering the complexity of
> dsa_slave_phy_setup(), I would certainly be extremely careful in
> changing any of this, the potential for breakage is pretty big.

Yes, i took a look at this, wondering how to convert to phylink. I
went away and got a stiff drink :-)

 Andrew

Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable

[Florian Fainelli]

On 09/22/2017 11:12 AM, Vivien Didelot wrote:
> Hi Florian,
> 
> Florian Fainelli  writes:
> 
>> On 09/22/2017 09:17 AM, Vivien Didelot wrote:
>>> The .port_enable and .port_disable functions are meant to deal with the
>>> switch ports only, and no driver is using the phy argument anyway.
>>> Remove it.
>>
>> I don't think this makes sense, there are perfectly legit reasons why a
>> switch driver may have something to do with the PHY device attached to
>> its per-port network interface, we should definitively keep that around,
>> unless you think we should be accessing the PHY within the switch
>> drivers by doing:
>>
>> struct phy_device *phydev = ds->ports[port].netdev->phydev?
> 
> bcm_sf2 is the only user for this phy argument right now. The reason I'm
> doing this is because I prefer to discourage switch drivers to dig into
> the phy device themselves while as you said there must be a cleaner
> solution. This must be handled somehow elsewhere in the stack.

The current approach of passing the phy_device reference as an argument
is certainly a cleaner way then. The port_enable caller can provide the
correct phy_device and that lifts the switch driver from having to dig
it itself from its per-port netdev.

> 
> In the meantime, moving the PHY device up to the dsa_port structure is a
> good solution, in order not to expose it in switch ops, but still make
> it available to more complex drivers.
> 
> Do you know if netdev->phydev is usable? Why do DSA has its own copy in
> dsa_slave_priv then?

Historical reasons mostly. Considering the complexity of
dsa_slave_phy_setup(), I would certainly be extremely careful in
changing any of this, the potential for breakage is pretty big. At first
glance, I would say that this is a safe conversion to do, and I can test
this on the HW I have here anyway.
-- 
Florian

Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable

[Vivien Didelot]

Hi Florian,

Florian Fainelli  writes:

> On 09/22/2017 09:17 AM, Vivien Didelot wrote:
>> The .port_enable and .port_disable functions are meant to deal with the
>> switch ports only, and no driver is using the phy argument anyway.
>> Remove it.
>
> I don't think this makes sense, there are perfectly legit reasons why a
> switch driver may have something to do with the PHY device attached to
> its per-port network interface, we should definitively keep that around,
> unless you think we should be accessing the PHY within the switch
> drivers by doing:
>
> struct phy_device *phydev = ds->ports[port].netdev->phydev?

bcm_sf2 is the only user for this phy argument right now. The reason I'm
doing this is because I prefer to discourage switch drivers to dig into
the phy device themselves while as you said there must be a cleaner
solution. This must be handled somehow elsewhere in the stack.

In the meantime, moving the PHY device up to the dsa_port structure is a
good solution, in order not to expose it in switch ops, but still make
it available to more complex drivers.

Do you know if netdev->phydev is usable? Why do DSA has its own copy in
dsa_slave_priv then?


I'll respin, thanks.

Vivien

Re: [PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable

[Florian Fainelli]

On 09/22/2017 09:17 AM, Vivien Didelot wrote:
> The .port_enable and .port_disable functions are meant to deal with the
> switch ports only, and no driver is using the phy argument anyway.
> Remove it.

I don't think this makes sense, there are perfectly legit reasons why a
switch driver may have something to do with the PHY device attached to
its per-port network interface, we should definitively keep that around,
unless you think we should be accessing the PHY within the switch
drivers by doing:

struct phy_device *phydev = ds->ports[port].netdev->phydev?

> 
> Signed-off-by: Vivien Didelot 
> ---
>  drivers/net/dsa/b53/b53_common.c   |  6 +++---
>  drivers/net/dsa/b53/b53_priv.h |  4 ++--
>  drivers/net/dsa/bcm_sf2.c  | 16 +++-
>  drivers/net/dsa/lan9303-core.c |  6 ++
>  drivers/net/dsa/microchip/ksz_common.c |  6 ++
>  drivers/net/dsa/mt7530.c   |  8 +++-
>  drivers/net/dsa/mv88e6xxx/chip.c   |  6 ++
>  drivers/net/dsa/qca8k.c|  6 ++
>  include/net/dsa.h  |  6 ++
>  net/dsa/slave.c|  4 ++--
>  10 files changed, 27 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/dsa/b53/b53_common.c 
> b/drivers/net/dsa/b53/b53_common.c
> index d4ce092def83..e46eb29d29f0 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -502,7 +502,7 @@ void b53_imp_vlan_setup(struct dsa_switch *ds, int 
> cpu_port)
>  }
>  EXPORT_SYMBOL(b53_imp_vlan_setup);
>  
> -int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
> +int b53_enable_port(struct dsa_switch *ds, int port)
>  {
>   struct b53_device *dev = ds->priv;
>   unsigned int cpu_port = dev->cpu_port;
> @@ -531,7 +531,7 @@ int b53_enable_port(struct dsa_switch *ds, int port, 
> struct phy_device *phy)
>  }
>  EXPORT_SYMBOL(b53_enable_port);
>  
> -void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device 
> *phy)
> +void b53_disable_port(struct dsa_switch *ds, int port)
>  {
>   struct b53_device *dev = ds->priv;
>   u8 reg;
> @@ -874,7 +874,7 @@ static int b53_setup(struct dsa_switch *ds)
>   if (dsa_is_cpu_port(ds, port))
>   b53_enable_cpu_port(dev, port);
>   else if (!(BIT(port) & ds->enabled_port_mask))
> - b53_disable_port(ds, port, NULL);
> + b53_disable_port(ds, port);
>   }
>  
>   return ret;
> diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
> index 603c66d240d8..688d02ee6155 100644
> --- a/drivers/net/dsa/b53/b53_priv.h
> +++ b/drivers/net/dsa/b53/b53_priv.h
> @@ -311,8 +311,8 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
>  struct dsa_mall_mirror_tc_entry *mirror, bool ingress);
>  void b53_mirror_del(struct dsa_switch *ds, int port,
>   struct dsa_mall_mirror_tc_entry *mirror);
> -int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
> -void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device 
> *phy);
> +int b53_enable_port(struct dsa_switch *ds, int port);
> +void b53_disable_port(struct dsa_switch *ds, int port);
>  void b53_brcm_hdr_setup(struct dsa_switch *ds, int port);
>  void b53_eee_enable_set(struct dsa_switch *ds, int port, bool enable);
>  int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy);
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index ad96b9725a2c..77e0c43f973b 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -159,8 +159,7 @@ static inline void bcm_sf2_port_intr_disable(struct 
> bcm_sf2_priv *priv,
>   intrl2_1_writel(priv, P_IRQ_MASK(off), INTRL2_CPU_CLEAR);
>  }
>  
> -static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
> -   struct phy_device *phy)
> +static int bcm_sf2_port_setup(struct dsa_switch *ds, int port)
>  {
>   struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
>   unsigned int i;
> @@ -191,11 +190,10 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, 
> int port,
>   if (port == priv->moca_port)
>   bcm_sf2_port_intr_enable(priv, port);
>  
> - return b53_enable_port(ds, port, phy);
> + return b53_enable_port(ds, port);
>  }
>  
> -static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
> -  struct phy_device *phy)
> +static void bcm_sf2_port_disable(struct dsa_switch *ds, int port)
>  {
>   struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
>   u32 off, reg;
> @@ -214,7 +212,7 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, 
> int port,
>   else
>   off = CORE_G_PCTL_PORT(port);
>  
> - b53_disable_port(ds, port, phy);
> + b53_disable_port(ds, port);
>  
>   /* Power down the port memory */
>   reg = 

[PATCH net-next 2/4] net: dsa: remove phy arg from port enable/disable

[Vivien Didelot]

The .port_enable and .port_disable functions are meant to deal with the
switch ports only, and no driver is using the phy argument anyway.
Remove it.

Signed-off-by: Vivien Didelot 
---
 drivers/net/dsa/b53/b53_common.c   |  6 +++---
 drivers/net/dsa/b53/b53_priv.h |  4 ++--
 drivers/net/dsa/bcm_sf2.c  | 16 +++-
 drivers/net/dsa/lan9303-core.c |  6 ++
 drivers/net/dsa/microchip/ksz_common.c |  6 ++
 drivers/net/dsa/mt7530.c   |  8 +++-
 drivers/net/dsa/mv88e6xxx/chip.c   |  6 ++
 drivers/net/dsa/qca8k.c|  6 ++
 include/net/dsa.h  |  6 ++
 net/dsa/slave.c|  4 ++--
 10 files changed, 27 insertions(+), 41 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index d4ce092def83..e46eb29d29f0 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -502,7 +502,7 @@ void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port)
 }
 EXPORT_SYMBOL(b53_imp_vlan_setup);
 
-int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
+int b53_enable_port(struct dsa_switch *ds, int port)
 {
struct b53_device *dev = ds->priv;
unsigned int cpu_port = dev->cpu_port;
@@ -531,7 +531,7 @@ int b53_enable_port(struct dsa_switch *ds, int port, struct 
phy_device *phy)
 }
 EXPORT_SYMBOL(b53_enable_port);
 
-void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
+void b53_disable_port(struct dsa_switch *ds, int port)
 {
struct b53_device *dev = ds->priv;
u8 reg;
@@ -874,7 +874,7 @@ static int b53_setup(struct dsa_switch *ds)
if (dsa_is_cpu_port(ds, port))
b53_enable_cpu_port(dev, port);
else if (!(BIT(port) & ds->enabled_port_mask))
-   b53_disable_port(ds, port, NULL);
+   b53_disable_port(ds, port);
}
 
return ret;
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 603c66d240d8..688d02ee6155 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -311,8 +311,8 @@ int b53_mirror_add(struct dsa_switch *ds, int port,
   struct dsa_mall_mirror_tc_entry *mirror, bool ingress);
 void b53_mirror_del(struct dsa_switch *ds, int port,
struct dsa_mall_mirror_tc_entry *mirror);
-int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
-void b53_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
+int b53_enable_port(struct dsa_switch *ds, int port);
+void b53_disable_port(struct dsa_switch *ds, int port);
 void b53_brcm_hdr_setup(struct dsa_switch *ds, int port);
 void b53_eee_enable_set(struct dsa_switch *ds, int port, bool enable);
 int b53_eee_init(struct dsa_switch *ds, int port, struct phy_device *phy);
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index ad96b9725a2c..77e0c43f973b 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -159,8 +159,7 @@ static inline void bcm_sf2_port_intr_disable(struct 
bcm_sf2_priv *priv,
intrl2_1_writel(priv, P_IRQ_MASK(off), INTRL2_CPU_CLEAR);
 }
 
-static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
- struct phy_device *phy)
+static int bcm_sf2_port_setup(struct dsa_switch *ds, int port)
 {
struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
unsigned int i;
@@ -191,11 +190,10 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int 
port,
if (port == priv->moca_port)
bcm_sf2_port_intr_enable(priv, port);
 
-   return b53_enable_port(ds, port, phy);
+   return b53_enable_port(ds, port);
 }
 
-static void bcm_sf2_port_disable(struct dsa_switch *ds, int port,
-struct phy_device *phy)
+static void bcm_sf2_port_disable(struct dsa_switch *ds, int port)
 {
struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
u32 off, reg;
@@ -214,7 +212,7 @@ static void bcm_sf2_port_disable(struct dsa_switch *ds, int 
port,
else
off = CORE_G_PCTL_PORT(port);
 
-   b53_disable_port(ds, port, phy);
+   b53_disable_port(ds, port);
 
/* Power down the port memory */
reg = core_readl(priv, CORE_MEM_PSM_VDD_CTRL);
@@ -613,7 +611,7 @@ static int bcm_sf2_sw_suspend(struct dsa_switch *ds)
for (port = 0; port < DSA_MAX_PORTS; port++) {
if ((1 << port) & ds->enabled_port_mask ||
dsa_is_cpu_port(ds, port))
-   bcm_sf2_port_disable(ds, port, NULL);
+   bcm_sf2_port_disable(ds, port);
}
 
return 0;
@@ -636,7 +634,7 @@ static int bcm_sf2_sw_resume(struct dsa_switch *ds)
 
for (port = 0; port < DSA_MAX_PORTS; port++) {
if ((1 <<