[PATCH net-next 1/4] net: phy: bcm54140: use genphy_soft_reset()

[PATCH net-next 1/4] net: phy: bcm54140: use genphy_soft_reset()

[Michael Walle]

Set the .soft_reset() op to be sure there will be a reset even if there
is no hardware reset line registered.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/bcm54140.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index 7341f0126cc4..eb5dbacc1253 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -862,6 +862,7 @@ static struct phy_driver bcm54140_drivers[] = {
 		.probe		= bcm54140_probe,
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
+		.soft_reset	= genphy_soft_reset,
 		.get_tunable	= bcm54140_get_tunable,
 		.set_tunable	= bcm54140_set_tunable,
 	},
-- 
2.20.1

[PATCH net-next 2/4] net: phy: bcm54140: fix phy_id_mask

[Michael Walle]

Broadcom defines the bits for this PHY as follows:
  { oui[24:3], model[6:0], revision[2:0] }

Thus we have to mask the lower three bits only.

Fixes: 6937602ed3f9 ("net: phy: add Broadcom BCM54140 support")
Signed-off-by: Michael Walle <michael@walle.cc>
---

Please note that although this patch contains a Fixes tag its subject
contains the net-next tag, because the commit in question is only in
net-next.

 drivers/net/phy/bcm54140.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index eb5dbacc1253..edafc9dc2f63 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -852,7 +852,7 @@ static int bcm54140_set_tunable(struct phy_device *phydev,
 static struct phy_driver bcm54140_drivers[] = {
 	{
 		.phy_id         = PHY_ID_BCM54140,
-		.phy_id_mask    = 0xfffffff0,
+		.phy_id_mask    = 0xfffffff8,
 		.name           = "Broadcom BCM54140",
 		.features       = PHY_GBIT_FEATURES,
 		.config_init    = bcm54140_config_init,
@@ -870,7 +870,7 @@ static struct phy_driver bcm54140_drivers[] = {
 module_phy_driver(bcm54140_drivers);
 
 static struct mdio_device_id __maybe_unused bcm54140_tbl[] = {
-	{ PHY_ID_BCM54140, 0xfffffff0 },
+	{ PHY_ID_BCM54140, 0xfffffff8 },
 	{ }
 };
 
-- 
2.20.1

Re: [PATCH net-next 2/4] net: phy: bcm54140: fix phy_id_mask

[Andrew Lunn]

On Tue, Apr 28, 2020 at 11:08:52PM +0200, Michael Walle wrote:
> Broadcom defines the bits for this PHY as follows:
>   { oui[24:3], model[6:0], revision[2:0] }
> 
> Thus we have to mask the lower three bits only.
> 
> Fixes: 6937602ed3f9 ("net: phy: add Broadcom BCM54140 support")
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 2/4] net: phy: bcm54140: fix phy_id_mask

[Florian Fainelli]

On 4/28/20 2:08 PM, Michael Walle wrote:
> Broadcom defines the bits for this PHY as follows:
>   { oui[24:3], model[6:0], revision[2:0] }
> 
> Thus we have to mask the lower three bits only.
> 
> Fixes: 6937602ed3f9 ("net: phy: add Broadcom BCM54140 support")
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
> 
> Please note that although this patch contains a Fixes tag its subject
> contains the net-next tag, because the commit in question is only in
> net-next.

Yes, and this is fine, thanks for doing this!
-- 
Florian

[PATCH net-next 3/4] net: phy: bcm54140: apply the workaround on b0 chips

[Michael Walle]

The lower three bits of the phy_id specifies the chip stepping. The
workaround is specifically for the B0 stepping. Apply it only on these
chips.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/bcm54140.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index edafc9dc2f63..d73cbddbc69b 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -115,6 +115,9 @@
 #define BCM54140_HWMON_IN_ALARM_BIT(ch) ((ch) ? BCM54140_RDB_MON_ISR_3V3 \
 					      : BCM54140_RDB_MON_ISR_1V0)
 
+#define BCM54140_PHY_ID_REV(phy_id)	((phy_id) & 0x7)
+#define BCM54140_REV_B0			1
+
 #define BCM54140_DEFAULT_DOWNSHIFT 5
 #define BCM54140_MAX_DOWNSHIFT 9
 
@@ -632,9 +635,11 @@ static int bcm54140_config_init(struct phy_device *phydev)
 	int ret;
 
 	/* Apply hardware errata */
-	ret = bcm54140_b0_workaround(phydev);
-	if (ret)
-		return ret;
+	if (BCM54140_PHY_ID_REV(phydev->phy_id) == BCM54140_REV_B0) {
+		ret = bcm54140_b0_workaround(phydev);
+		if (ret)
+			return ret;
+	}
 
 	/* Unmask events we are interested in. */
 	reg &= ~(BCM54140_RDB_INT_DUPLEX |
-- 
2.20.1

Re: [PATCH net-next 3/4] net: phy: bcm54140: apply the workaround on b0 chips

[Andrew Lunn]

On Tue, Apr 28, 2020 at 11:08:53PM +0200, Michael Walle wrote:
> The lower three bits of the phy_id specifies the chip stepping. The
> workaround is specifically for the B0 stepping. Apply it only on these
> chips.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 3/4] net: phy: bcm54140: apply the workaround on b0 chips

[Florian Fainelli]

On 4/28/20 2:08 PM, Michael Walle wrote:
> The lower three bits of the phy_id specifies the chip stepping. The
> workaround is specifically for the B0 stepping. Apply it only on these
> chips.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 4/4] net: phy: bcm54140: add second PHY ID

[Michael Walle]

This PHY have to PHY IDs depending on its mode. Adjust the mask so that
it includes both IDs.

Also move the PHY_ID_BCM54140 out of the brcmphy.h into the driver to
have the mask and the ID in one place.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/net/phy/bcm54140.c | 12 ++++++++++--
 include/linux/brcmphy.h    |  1 -
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index d73cbddbc69b..6c54f067c7fb 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -115,6 +115,14 @@
 #define BCM54140_HWMON_IN_ALARM_BIT(ch) ((ch) ? BCM54140_RDB_MON_ISR_3V3 \
 					      : BCM54140_RDB_MON_ISR_1V0)
 
+/* This PHY has two different PHY IDs depening on its MODE_SEL pin. This
+ * pin choses between 4x SGMII and QSGMII mode:
+ *   AE02_5009 4x SGMII
+ *   AE02_5019 QSGMII
+ */
+#define PHY_ID_BCM54140		0xae025009
+#define BCM54140_PHY_ID_MASK	0xffffffe8
+
 #define BCM54140_PHY_ID_REV(phy_id)	((phy_id) & 0x7)
 #define BCM54140_REV_B0			1
 
@@ -857,7 +865,7 @@ static int bcm54140_set_tunable(struct phy_device *phydev,
 static struct phy_driver bcm54140_drivers[] = {
 	{
 		.phy_id         = PHY_ID_BCM54140,
-		.phy_id_mask    = 0xfffffff8,
+		.phy_id_mask    = BCM54140_PHY_ID_MASK,
 		.name           = "Broadcom BCM54140",
 		.features       = PHY_GBIT_FEATURES,
 		.config_init    = bcm54140_config_init,
@@ -875,7 +883,7 @@ static struct phy_driver bcm54140_drivers[] = {
 module_phy_driver(bcm54140_drivers);
 
 static struct mdio_device_id __maybe_unused bcm54140_tbl[] = {
-	{ PHY_ID_BCM54140, 0xfffffff8 },
+	{ PHY_ID_BCM54140, BCM54140_PHY_ID_MASK },
 	{ }
 };
 
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 8be150e69c7c..897b69309964 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -25,7 +25,6 @@
 #define PHY_ID_BCM5461			0x002060c0
 #define PHY_ID_BCM54612E		0x03625e60
 #define PHY_ID_BCM54616S		0x03625d10
-#define PHY_ID_BCM54140			0xae025019
 #define PHY_ID_BCM57780			0x03625d90
 #define PHY_ID_BCM89610			0x03625cd0
 
-- 
2.20.1

Re: [PATCH net-next 4/4] net: phy: bcm54140: add second PHY ID

[Andrew Lunn]

On Tue, Apr 28, 2020 at 11:08:54PM +0200, Michael Walle wrote:
> This PHY have to PHY IDs depending on its mode. Adjust the mask so that
> it includes both IDs.

Hi Michael

I don't have a strong opinion, but maybe list it as two different
PHYs? I do sometimes grep for PHY IDs, and that would not work due to
the odd mask.

    Andrew

Re: [PATCH net-next 4/4] net: phy: bcm54140: add second PHY ID

[Michael Walle]

Hi Andrew,

Am 2020-04-28 23:29, schrieb Andrew Lunn:
> On Tue, Apr 28, 2020 at 11:08:54PM +0200, Michael Walle wrote:
>> This PHY have to PHY IDs depending on its mode. Adjust the mask so 
>> that
>> it includes both IDs.
> 
> Hi Michael
> 
> I don't have a strong opinion, but maybe list it as two different
> PHYs? I do sometimes grep for PHY IDs, and that would not work due to
> the odd mask.

Me neither. I just looked odd to have actually the same PHY listed twice
with just another id. That makes me wonder if it is possible to have
the same PHY driver name twice. IIRC it is at leased used somewhere
in the sysfs. If that is true, I'd prefer to just have one PHY
"BCM54140" instead of a "BCM54140 (QSGMII)" and "BCM54140 (4x SGMII)".
Because it is actually the same PHY but only another interface towards
the MAC is used.

-michael

Re: [PATCH net-next 4/4] net: phy: bcm54140: add second PHY ID

[Florian Fainelli]

On 4/28/20 2:08 PM, Michael Walle wrote:
> This PHY have to PHY IDs depending on its mode. Adjust the mask so that
> it includes both IDs.
> 
> Also move the PHY_ID_BCM54140 out of the brcmphy.h into the driver to
> have the mask and the ID in one place.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
[snip]

Just one request below.

> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
> index 8be150e69c7c..897b69309964 100644
> --- a/include/linux/brcmphy.h
> +++ b/include/linux/brcmphy.h
> @@ -25,7 +25,6 @@
>  #define PHY_ID_BCM5461			0x002060c0
>  #define PHY_ID_BCM54612E		0x03625e60
>  #define PHY_ID_BCM54616S		0x03625d10
> -#define PHY_ID_BCM54140			0xae025019

I would prefer if you could leave the entry here since this the file
that centralizes all Broadcom PHY OUIs.

Thanks!
-- 
Florian

Re: [PATCH net-next 1/4] net: phy: bcm54140: use genphy_soft_reset()

[Florian Fainelli]

On 4/28/20 2:08 PM, Michael Walle wrote:
> Set the .soft_reset() op to be sure there will be a reset even if there
> is no hardware reset line registered.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian