Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

[Andrew Lunn]

> I know this looks boring, I do not particularly enjoy it myself, but I
> think this is also important. I don't mind fixing the poking function as
> well in the near future.

It would be great if you do. It could be as simple as using
phy_ethtool_get_eee() and phy_ethtool_set_eee().

  Andrew

Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

[Vivien Didelot]

Hi David,

David Miller  writes:

> However, in this particular case, this issue was brought to Vivien's
> attention multiple times in the past.
>
> And I think the direct PHY poking issue is much more important than
> these seemingly endless reorganizations of the driver that Vivien is
> doing.
>
> So I personally share Andrew's serious frustration that we are doing
> constant reorgs but not addressing directly the specific issues that
> one has been made clearly aware of.

We support 26 Marvell Ethernet switch chips, so I am often comparing the
documentation of many of them to make sure the driver stops writing
arbitrary registers, ending up with many inconsistencies (like Remote
Management being currently enabled on all chips with an RMU, fix coming)
mainly due to poor documentation and device setup.

I know this looks boring, I do not particularly enjoy it myself, but I
think this is also important. I don't mind fixing the poking function as
well in the near future.


Thanks,

Vivien

Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

[David Miller]

From: Florian Fainelli 
Date: Tue, 18 Jul 2017 09:01:01 -0700

> On 07/17/2017 02:10 PM, David Miller wrote:
>> From: Andrew Lunn 
>> Date: Mon, 17 Jul 2017 23:04:05 +0200
>> 
>>> On Mon, Jul 17, 2017 at 01:45:49PM -0700, David Miller wrote:
 From: Vivien Didelot 
 Date: Mon, 17 Jul 2017 15:32:52 -0400

> Hi Andrew,
>
> Andrew Lunn  writes:
>
>> I never liked this. I think it is architecturally wrong for the switch
>> to be poking around in the PHY. It should ask the PHY driver. This is
>> especially true for external PHYs which might not be a Marvell PHY.
>
> I share the same concern. However this patch is just isolating the
> existing code so that we get rid of the last caps and flags and stop
> writing (without reading them first) arbitrary registers.
>
> Once this portion is moved to the PHY driver, one can remove it from
> mv88e6xxx.

 Seems a reasonable plan of action.

 Andrew, do you agree?
>>>
>>> Hi David
>>>
>>> I just fear it will not get fixed, just put into a corner to
>>> fester. Having to fix it properly before these patches are merged
>>> provides some incentive.
>> 
>> If Vivien doesn't make good on his promises to do so, tell me and
>> I will revert all of these changes.
>> 
>> Ok?
> 
> This seems to be completely unfair to Vivien, there is nothing wrong
> with his patch series per-se other than he was unfortunate enough he
> highlighted something that needs fixing. This was not a serious enough
> problem before and it cannot possibly be one now either with just a code
> move.
> 
> On a general note, we cannot have whoever was the last one to touch a
> piece of code that makes us see that this or that said piece of code is
> less than ideal be selected as the random victim for doing that cleanup,
> this just does not work. I know this is standard practice in Linux and
> other open source software (been there before with the USB maintainers),
> but this creates only one thing: making you want to runaway and scream
> lalalalala.
> 
> So let's be pragmatic and maintain a public TODO list for this driver
> that people can pick items to fix/cleanup/change that have been
> identified as candidates for patches.

However, in this particular case, this issue was brought to Vivien's
attention multiple times in the past.

And I think the direct PHY poking issue is much more important than
these seemingly endless reorganizations of the driver that Vivien is
doing.

So I personally share Andrew's serious frustration that we are doing
constant reorgs but not addressing directly the specific issues that
one has been made clearly aware of.

Thanks.

Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

[Florian Fainelli]

On 07/17/2017 02:10 PM, David Miller wrote:
> From: Andrew Lunn 
> Date: Mon, 17 Jul 2017 23:04:05 +0200
> 
>> On Mon, Jul 17, 2017 at 01:45:49PM -0700, David Miller wrote:
>>> From: Vivien Didelot 
>>> Date: Mon, 17 Jul 2017 15:32:52 -0400
>>>
 Hi Andrew,

 Andrew Lunn  writes:

> I never liked this. I think it is architecturally wrong for the switch
> to be poking around in the PHY. It should ask the PHY driver. This is
> especially true for external PHYs which might not be a Marvell PHY.

 I share the same concern. However this patch is just isolating the
 existing code so that we get rid of the last caps and flags and stop
 writing (without reading them first) arbitrary registers.

 Once this portion is moved to the PHY driver, one can remove it from
 mv88e6xxx.
>>>
>>> Seems a reasonable plan of action.
>>>
>>> Andrew, do you agree?
>>
>> Hi David
>>
>> I just fear it will not get fixed, just put into a corner to
>> fester. Having to fix it properly before these patches are merged
>> provides some incentive.
> 
> If Vivien doesn't make good on his promises to do so, tell me and
> I will revert all of these changes.
> 
> Ok?

This seems to be completely unfair to Vivien, there is nothing wrong
with his patch series per-se other than he was unfortunate enough he
highlighted something that needs fixing. This was not a serious enough
problem before and it cannot possibly be one now either with just a code
move.

On a general note, we cannot have whoever was the last one to touch a
piece of code that makes us see that this or that said piece of code is
less than ideal be selected as the random victim for doing that cleanup,
this just does not work. I know this is standard practice in Linux and
other open source software (been there before with the USB maintainers),
but this creates only one thing: making you want to runaway and scream
lalalalala.

So let's be pragmatic and maintain a public TODO list for this driver
that people can pick items to fix/cleanup/change that have been
identified as candidates for patches.
-- 
Florian

Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

[David Miller]

From: Andrew Lunn 
Date: Mon, 17 Jul 2017 23:04:05 +0200

> On Mon, Jul 17, 2017 at 01:45:49PM -0700, David Miller wrote:
>> From: Vivien Didelot 
>> Date: Mon, 17 Jul 2017 15:32:52 -0400
>> 
>> > Hi Andrew,
>> > 
>> > Andrew Lunn  writes:
>> > 
>> >> I never liked this. I think it is architecturally wrong for the switch
>> >> to be poking around in the PHY. It should ask the PHY driver. This is
>> >> especially true for external PHYs which might not be a Marvell PHY.
>> > 
>> > I share the same concern. However this patch is just isolating the
>> > existing code so that we get rid of the last caps and flags and stop
>> > writing (without reading them first) arbitrary registers.
>> > 
>> > Once this portion is moved to the PHY driver, one can remove it from
>> > mv88e6xxx.
>> 
>> Seems a reasonable plan of action.
>> 
>> Andrew, do you agree?
> 
> Hi David
> 
> I just fear it will not get fixed, just put into a corner to
> fester. Having to fix it properly before these patches are merged
> provides some incentive.

If Vivien doesn't make good on his promises to do so, tell me and
I will revert all of these changes.

Ok?

Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

[Andrew Lunn]

On Mon, Jul 17, 2017 at 01:45:49PM -0700, David Miller wrote:
> From: Vivien Didelot 
> Date: Mon, 17 Jul 2017 15:32:52 -0400
> 
> > Hi Andrew,
> > 
> > Andrew Lunn  writes:
> > 
> >> I never liked this. I think it is architecturally wrong for the switch
> >> to be poking around in the PHY. It should ask the PHY driver. This is
> >> especially true for external PHYs which might not be a Marvell PHY.
> > 
> > I share the same concern. However this patch is just isolating the
> > existing code so that we get rid of the last caps and flags and stop
> > writing (without reading them first) arbitrary registers.
> > 
> > Once this portion is moved to the PHY driver, one can remove it from
> > mv88e6xxx.
> 
> Seems a reasonable plan of action.
> 
> Andrew, do you agree?

Hi David

I just fear it will not get fixed, just put into a corner to
fester. Having to fix it properly before these patches are merged
provides some incentive.

 Andrew

Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

[David Miller]

From: Vivien Didelot 
Date: Mon, 17 Jul 2017 15:32:52 -0400

> Hi Andrew,
> 
> Andrew Lunn  writes:
> 
>> I never liked this. I think it is architecturally wrong for the switch
>> to be poking around in the PHY. It should ask the PHY driver. This is
>> especially true for external PHYs which might not be a Marvell PHY.
> 
> I share the same concern. However this patch is just isolating the
> existing code so that we get rid of the last caps and flags and stop
> writing (without reading them first) arbitrary registers.
> 
> Once this portion is moved to the PHY driver, one can remove it from
> mv88e6xxx.

Seems a reasonable plan of action.

Andrew, do you agree?

Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  writes:

> I never liked this. I think it is architecturally wrong for the switch
> to be poking around in the PHY. It should ask the PHY driver. This is
> especially true for external PHYs which might not be a Marvell PHY.

I share the same concern. However this patch is just isolating the
existing code so that we get rid of the last caps and flags and stop
writing (without reading them first) arbitrary registers.

Once this portion is moved to the PHY driver, one can remove it from
mv88e6xxx.

Thanks,

Vivien

Re: [PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

[Andrew Lunn]

> diff --git a/drivers/net/dsa/mv88e6xxx/phy.c b/drivers/net/dsa/mv88e6xxx/phy.c
> index 436668bd50dc..317ae89cfa68 100644
> --- a/drivers/net/dsa/mv88e6xxx/phy.c
> +++ b/drivers/net/dsa/mv88e6xxx/phy.c
> @@ -246,3 +246,99 @@ int mv88e6xxx_phy_setup(struct mv88e6xxx_chip *chip)
>  {
>   return mv88e6xxx_phy_ppu_enable(chip);
>  }
> +
> +/* Page 0, Register 16: Copper Specific Control Register 1 */
> +
> +int mv88e6352_phy_energy_detect_read(struct mv88e6xxx_chip *chip, int phy,
> +  struct ethtool_eee *eee)
> +{
> + u16 val;
> + int err;
> +
> + err = mv88e6xxx_phy_read(chip, phy, MV88E6XXX_PHY_CSCTL1, );
> + if (err)
> + return err;
> +
> + val &= MV88E6352_PHY_CSCTL1_ENERGY_DETECT_MASK;
> +
> + eee->eee_enabled = false;
> + eee->tx_lpi_enabled = false;
> +
> + switch (val) {
> + case MV88E6352_PHY_CSCTL1_ENERGY_DETECT_SENSE_NLP:
> + eee->tx_lpi_enabled = true;
> + /* fall through... */
> + case MV88E6352_PHY_CSCTL1_ENERGY_DETECT_SENSE_RCV:
> + eee->eee_enabled = true;
> + }
> +
> + return 0;
> +}

Hi Vivien

I never liked this. I think it is architecturally wrong for the switch
to be poking around in the PHY. It should ask the PHY driver. This is
especially true for external PHYs which might not be a Marvell PHY.

   Andrew

[PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

[Vivien Didelot]

The 88E6352 family supports Energy Detect and has one bit for Sense and
one bit for periodically transmit NLP (Energy Detect+TM). The 88E6390
family adds another bit to distinguish Auto or SW wake-up. Chips
supporting EEE all have an EEE Enabled bit in the Port Status Register.

This patch adds new ops for the PHY Energy Detect accesses.

This also allows us to get rid of the MV88E6XXX_FLAG_EEE flag.

Signed-off-by: Vivien Didelot 
---
 drivers/net/dsa/mv88e6xxx/chip.c | 91 ++---
 drivers/net/dsa/mv88e6xxx/chip.h | 24 +-
 drivers/net/dsa/mv88e6xxx/phy.c  | 96 
 drivers/net/dsa/mv88e6xxx/phy.h  | 22 +
 drivers/net/dsa/mv88e6xxx/port.c | 17 +++
 drivers/net/dsa/mv88e6xxx/port.h |  3 ++
 6 files changed, 204 insertions(+), 49 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index eb4871a66076..be61983dfed4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -810,31 +810,40 @@ static void mv88e6xxx_get_regs(struct dsa_switch *ds, int 
port,
mutex_unlock(>reg_lock);
 }
 
+static int mv88e6xxx_energy_detect_read(struct mv88e6xxx_chip *chip, int port,
+   struct ethtool_eee *eee)
+{
+   int err;
+
+   if (!chip->info->ops->phy_energy_detect_read)
+   return -EOPNOTSUPP;
+
+   /* assign eee->eee_enabled and eee->tx_lpi_enabled */
+   err = chip->info->ops->phy_energy_detect_read(chip, port, eee);
+   if (err)
+   return err;
+
+   /* assign eee->eee_active */
+   return mv88e6xxx_port_status_eee(chip, port, eee);
+}
+
+static int mv88e6xxx_energy_detect_write(struct mv88e6xxx_chip *chip, int port,
+struct ethtool_eee *eee)
+{
+   if (!chip->info->ops->phy_energy_detect_write)
+   return -EOPNOTSUPP;
+
+   return chip->info->ops->phy_energy_detect_write(chip, port, eee);
+}
+
 static int mv88e6xxx_get_eee(struct dsa_switch *ds, int port,
 struct ethtool_eee *e)
 {
struct mv88e6xxx_chip *chip = ds->priv;
-   u16 reg;
int err;
 
-   if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_EEE))
-   return -EOPNOTSUPP;
-
mutex_lock(>reg_lock);
-
-   err = mv88e6xxx_phy_read(chip, port, 16, );
-   if (err)
-   goto out;
-
-   e->eee_enabled = !!(reg & 0x0200);
-   e->tx_lpi_enabled = !!(reg & 0x0100);
-
-   err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, );
-   if (err)
-   goto out;
-
-   e->eee_active = !!(reg & MV88E6352_PORT_STS_EEE);
-out:
+   err = mv88e6xxx_energy_detect_read(chip, port, e);
mutex_unlock(>reg_lock);
 
return err;
@@ -844,26 +853,10 @@ static int mv88e6xxx_set_eee(struct dsa_switch *ds, int 
port,
 struct phy_device *phydev, struct ethtool_eee *e)
 {
struct mv88e6xxx_chip *chip = ds->priv;
-   u16 reg;
int err;
 
-   if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_EEE))
-   return -EOPNOTSUPP;
-
mutex_lock(>reg_lock);
-
-   err = mv88e6xxx_phy_read(chip, port, 16, );
-   if (err)
-   goto out;
-
-   reg &= ~0x0300;
-   if (e->eee_enabled)
-   reg |= 0x0200;
-   if (e->tx_lpi_enabled)
-   reg |= 0x0100;
-
-   err = mv88e6xxx_phy_write(chip, port, 16, reg);
-out:
+   err = mv88e6xxx_energy_detect_write(chip, port, e);
mutex_unlock(>reg_lock);
 
return err;
@@ -2528,6 +2521,8 @@ static const struct mv88e6xxx_ops mv88e6141_ops = {
.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
.phy_read = mv88e6xxx_g2_smi_phy_read,
.phy_write = mv88e6xxx_g2_smi_phy_write,
+   .phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
+   .phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
.port_set_link = mv88e6xxx_port_set_link,
.port_set_duplex = mv88e6xxx_port_set_duplex,
.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
@@ -2653,6 +2648,8 @@ static const struct mv88e6xxx_ops mv88e6172_ops = {
.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
.phy_read = mv88e6xxx_g2_smi_phy_read,
.phy_write = mv88e6xxx_g2_smi_phy_write,
+   .phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
+   .phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
.port_set_link = mv88e6xxx_port_set_link,
.port_set_duplex = mv88e6xxx_port_set_duplex,
.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
@@ -2722,6 +2719,8 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
.phy_read = mv88e6xxx_g2_smi_phy_read,
.phy_write = mv88e6xxx_g2_smi_phy_write,
+   .phy_energy_detect_read = 

[PATCH net-next 11/12] net: dsa: mv88e6xxx: add Energy Detect ops

[Vivien Didelot]

The 88E6352 family supports Energy Detect and has one bit for Sense and
one bit for periodically transmit NLP (Energy Detect+TM). The 88E6390
family adds another bit to distinguish Auto or SW wake-up. Chips
supporting EEE all have an EEE Enabled bit in the Port Status Register.

This patch adds new ops for the PHY Energy Detect accesses.

This also allows us to get rid of the MV88E6XXX_FLAG_EEE flag.

Signed-off-by: Vivien Didelot 
---
 drivers/net/dsa/mv88e6xxx/chip.c | 91 ++---
 drivers/net/dsa/mv88e6xxx/chip.h | 24 +-
 drivers/net/dsa/mv88e6xxx/phy.c  | 96 
 drivers/net/dsa/mv88e6xxx/phy.h  | 22 +
 drivers/net/dsa/mv88e6xxx/port.c | 17 +++
 drivers/net/dsa/mv88e6xxx/port.h |  3 ++
 6 files changed, 204 insertions(+), 49 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index eb4871a66076..be61983dfed4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -810,31 +810,40 @@ static void mv88e6xxx_get_regs(struct dsa_switch *ds, int 
port,
mutex_unlock(>reg_lock);
 }
 
+static int mv88e6xxx_energy_detect_read(struct mv88e6xxx_chip *chip, int port,
+   struct ethtool_eee *eee)
+{
+   int err;
+
+   if (!chip->info->ops->phy_energy_detect_read)
+   return -EOPNOTSUPP;
+
+   /* assign eee->eee_enabled and eee->tx_lpi_enabled */
+   err = chip->info->ops->phy_energy_detect_read(chip, port, eee);
+   if (err)
+   return err;
+
+   /* assign eee->eee_active */
+   return mv88e6xxx_port_status_eee(chip, port, eee);
+}
+
+static int mv88e6xxx_energy_detect_write(struct mv88e6xxx_chip *chip, int port,
+struct ethtool_eee *eee)
+{
+   if (!chip->info->ops->phy_energy_detect_write)
+   return -EOPNOTSUPP;
+
+   return chip->info->ops->phy_energy_detect_write(chip, port, eee);
+}
+
 static int mv88e6xxx_get_eee(struct dsa_switch *ds, int port,
 struct ethtool_eee *e)
 {
struct mv88e6xxx_chip *chip = ds->priv;
-   u16 reg;
int err;
 
-   if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_EEE))
-   return -EOPNOTSUPP;
-
mutex_lock(>reg_lock);
-
-   err = mv88e6xxx_phy_read(chip, port, 16, );
-   if (err)
-   goto out;
-
-   e->eee_enabled = !!(reg & 0x0200);
-   e->tx_lpi_enabled = !!(reg & 0x0100);
-
-   err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_STS, );
-   if (err)
-   goto out;
-
-   e->eee_active = !!(reg & MV88E6352_PORT_STS_EEE);
-out:
+   err = mv88e6xxx_energy_detect_read(chip, port, e);
mutex_unlock(>reg_lock);
 
return err;
@@ -844,26 +853,10 @@ static int mv88e6xxx_set_eee(struct dsa_switch *ds, int 
port,
 struct phy_device *phydev, struct ethtool_eee *e)
 {
struct mv88e6xxx_chip *chip = ds->priv;
-   u16 reg;
int err;
 
-   if (!mv88e6xxx_has(chip, MV88E6XXX_FLAG_EEE))
-   return -EOPNOTSUPP;
-
mutex_lock(>reg_lock);
-
-   err = mv88e6xxx_phy_read(chip, port, 16, );
-   if (err)
-   goto out;
-
-   reg &= ~0x0300;
-   if (e->eee_enabled)
-   reg |= 0x0200;
-   if (e->tx_lpi_enabled)
-   reg |= 0x0100;
-
-   err = mv88e6xxx_phy_write(chip, port, 16, reg);
-out:
+   err = mv88e6xxx_energy_detect_write(chip, port, e);
mutex_unlock(>reg_lock);
 
return err;
@@ -2528,6 +2521,8 @@ static const struct mv88e6xxx_ops mv88e6141_ops = {
.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
.phy_read = mv88e6xxx_g2_smi_phy_read,
.phy_write = mv88e6xxx_g2_smi_phy_write,
+   .phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
+   .phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
.port_set_link = mv88e6xxx_port_set_link,
.port_set_duplex = mv88e6xxx_port_set_duplex,
.port_set_rgmii_delay = mv88e6390_port_set_rgmii_delay,
@@ -2653,6 +2648,8 @@ static const struct mv88e6xxx_ops mv88e6172_ops = {
.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
.phy_read = mv88e6xxx_g2_smi_phy_read,
.phy_write = mv88e6xxx_g2_smi_phy_write,
+   .phy_energy_detect_read = mv88e6352_phy_energy_detect_read,
+   .phy_energy_detect_write = mv88e6352_phy_energy_detect_write,
.port_set_link = mv88e6xxx_port_set_link,
.port_set_duplex = mv88e6xxx_port_set_duplex,
.port_set_rgmii_delay = mv88e6352_port_set_rgmii_delay,
@@ -2722,6 +2719,8 @@ static const struct mv88e6xxx_ops mv88e6176_ops = {
.set_switch_mac = mv88e6xxx_g2_set_switch_mac,
.phy_read = mv88e6xxx_g2_smi_phy_read,
.phy_write = mv88e6xxx_g2_smi_phy_write,
+   .phy_energy_detect_read =