[2/3] net: macb: add support for C45 MDIO read/write
diff mbox series

Message ID 1574759389-103118-1-git-send-email-mparab@cadence.com
State Deferred
Delegated to: David Miller
Headers show
Series
  • net: macb: cover letter
Related show

Commit Message

Milind Parab Nov. 26, 2019, 9:09 a.m. UTC
This patch modify MDIO read/write functions to support
communication with C45 PHY.

Signed-off-by: Milind Parab <mparab@cadence.com>
---
 drivers/net/ethernet/cadence/macb.h      | 15 ++++--
 drivers/net/ethernet/cadence/macb_main.c | 61 +++++++++++++++++++-----
 2 files changed, 61 insertions(+), 15 deletions(-)

Comments

Andrew Lunn Nov. 26, 2019, 2:37 p.m. UTC | #1
On Tue, Nov 26, 2019 at 09:09:49AM +0000, Milind Parab wrote:
> This patch modify MDIO read/write functions to support
> communication with C45 PHY.

I think i've asked this before, at least once, but you have not added
it to the commit messages. Do all generations of the macb support C45?


FYI: Net next is closed at the moment, so your patches will be
rejected. Please post again when it re-opens.

	  Andrew
Nicolas Ferre Nov. 27, 2019, 6:31 p.m. UTC | #2
On 26/11/2019 at 15:37, Andrew Lunn wrote:
> On Tue, Nov 26, 2019 at 09:09:49AM +0000, Milind Parab wrote:
>> This patch modify MDIO read/write functions to support
>> communication with C45 PHY.
> 
> I think i've asked this before, at least once, but you have not added
> it to the commit messages. Do all generations of the macb support C45?

For what I can tell from the different IP revisions that we implemented 
throughout the years in Atmel then Microchip products (back to 
at91rm9200 and at91sam9263), it seems yes.

The "PHY Maintenance Register" "MACB_MAN_*" was always present with the 
same bits 32-28 layout (with somehow different names).

But definitively we would need to hear that from Cadence itself which 
would be far better.

[..]

Best regards,
Andrew Lunn Nov. 27, 2019, 6:51 p.m. UTC | #3
On Wed, Nov 27, 2019 at 06:31:54PM +0000, Nicolas.Ferre@microchip.com wrote:
> On 26/11/2019 at 15:37, Andrew Lunn wrote:
> > On Tue, Nov 26, 2019 at 09:09:49AM +0000, Milind Parab wrote:
> >> This patch modify MDIO read/write functions to support
> >> communication with C45 PHY.
> > 
> > I think i've asked this before, at least once, but you have not added
> > it to the commit messages. Do all generations of the macb support C45?
> 
> For what I can tell from the different IP revisions that we implemented 
> throughout the years in Atmel then Microchip products (back to 
> at91rm9200 and at91sam9263), it seems yes.
> 
> The "PHY Maintenance Register" "MACB_MAN_*" was always present with the 
> same bits 32-28 layout (with somehow different names).
> 
> But definitively we would need to hear that from Cadence itself which 
> would be far better.

Hi Nicolas

Thanks, that is useful.

I'm just trying to avoid backward compatibility issues, somebody
issues a C45 request on old silicon and it all goes horribly wrong.

       Andrew
Milind Parab Nov. 28, 2019, 8:29 a.m. UTC | #4
>-----Original Message-----
>From: Andrew Lunn <andrew@lunn.ch>
>Sent: Thursday, November 28, 2019 12:21 AM
>To: Nicolas.Ferre@microchip.com
>Cc: Milind Parab <mparab@cadence.com>; antoine.tenart@bootlin.com;
>davem@davemloft.net; netdev@vger.kernel.org; f.fainelli@gmail.com;
>hkallweit1@gmail.com; linux-kernel@vger.kernel.org; Dhananjay Vilasrao
>Kangude <dkangude@cadence.com>; Parshuram Raju Thombare
><pthombar@cadence.com>; rmk+kernel@arm.linux.org.uk
>Subject: Re: [PATCH 2/3] net: macb: add support for C45 MDIO read/write
>
>EXTERNAL MAIL
>
>
>On Wed, Nov 27, 2019 at 06:31:54PM +0000, Nicolas.Ferre@microchip.com
>wrote:
>> On 26/11/2019 at 15:37, Andrew Lunn wrote:
>> > On Tue, Nov 26, 2019 at 09:09:49AM +0000, Milind Parab wrote:
>> >> This patch modify MDIO read/write functions to support
>> >> communication with C45 PHY.
>> >
>> > I think i've asked this before, at least once, but you have not
>> > added it to the commit messages. Do all generations of the macb support
>C45?
>>
>> For what I can tell from the different IP revisions that we
>> implemented throughout the years in Atmel then Microchip products
>> (back to
>> at91rm9200 and at91sam9263), it seems yes.
>>
>> The "PHY Maintenance Register" "MACB_MAN_*" was always present with
>> the same bits 32-28 layout (with somehow different names).
>>
>> But definitively we would need to hear that from Cadence itself which
>> would be far better.
>
>Hi Nicolas
>
>Thanks, that is useful.
>
>I'm just trying to avoid backward compatibility issues, somebody issues a C45
>request on old silicon and it all goes horribly wrong.

This patch doesn't affect current C22 operation of the driver. 
However if a user selects C45 on incompatible MAC (there are old MAC, prior to Release1p10, released 10th April 2003) MDIO operations may fails.
Adding check will cover this corner case.

We will add this check in v2 of patch set.
>
>       Andrew
Andrew Lunn Nov. 28, 2019, 2:52 p.m. UTC | #5
> This patch doesn't affect current C22 operation of the driver. 
> However if a user selects C45 on incompatible MAC (there are old MAC, prior to Release1p10, released 10th April 2003) MDIO operations may fails.

How do they fail? Lockup the chip and require a cold boot? Timeout
after 10ms and return -ETIMEOUT?

Currently, there is nothing stopping a C45 access to happen. There has
been talk of making the probe for C45 PHYs the same as C22, scan the
bus. I guess that would be implemented by adding a flag to each MDIO
bus driver indicating it supports C45. All 32 addresses would then be
probed using C45 as well as C22. So we need to be sure it is safe to
use C45 on these older devices.

      Andrew
Milind Parab Nov. 29, 2019, 10:02 a.m. UTC | #6
>
>> This patch doesn't affect current C22 operation of the driver.
>> However if a user selects C45 on incompatible MAC (there are old MAC,
>prior to Release1p10, released 10th April 2003) MDIO operations may fails.
>
>How do they fail? Lockup the chip and require a cold boot? Timeout after
>10ms and return -ETIMEOUT?
>
No such catastrophic failure will happen. Failure will only on the possible response from the PHY.
Hence, it is safe to assume all versions of the MAC (old and new) using the GPL driver support both Clause 22 and Clause 45 operation.
Whether the access is in Clause 22 or Clause 45 format depends on the data pattern written to the PHY management register.

On response from PHY which would depend on the Start of Frame SOF
The relevant parts of the IEEE 802.3 standard are for identifying Clause 22 and Clause 45 operation is here:
22.2.4.5.3 ST (start of frame) 
The start of frame is indicated by a <01> pattern. This pattern assures transitions from the default logic one line state to zero and back to one.

45.3.3 ST (start of frame)
The start of frame for indirect access cycles is indicated by the <00> pattern. This pattern assures a transition from the default one and identifies the frame as an indirect access. Frames that contain the ST=<01> pattern defined in Clause 22 shall be ignored by the devices specified in Clause 45.

>Currently, there is nothing stopping a C45 access to happen. There has been
>talk of making the probe for C45 PHYs the same as C22, scan the bus. I guess
>that would be implemented by adding a flag to each MDIO bus driver
>indicating it supports C45. All 32 addresses would then be probed using C45 as
>well as C22. So we need to be sure it is safe to use C45 on these older devices.
>
>      Andrew

Patch
diff mbox series

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 19fe4f4867c7..dbf7070fcdba 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -630,10 +630,17 @@ 
 #define GEM_CLK_DIV96				5
 
 /* Constants for MAN register */
-#define MACB_MAN_SOF				1
-#define MACB_MAN_WRITE				1
-#define MACB_MAN_READ				2
-#define MACB_MAN_CODE				2
+#define MACB_MAN_C22_SOF			1
+#define MACB_MAN_C22_WRITE			1
+#define MACB_MAN_C22_READ			2
+#define MACB_MAN_C22_CODE			2
+
+#define MACB_MAN_C45_SOF			0
+#define MACB_MAN_C45_ADDR			0
+#define MACB_MAN_C45_WRITE			1
+#define MACB_MAN_C45_POST_READ_INCR		2
+#define MACB_MAN_C45_READ			3
+#define MACB_MAN_C45_CODE			2
 
 /* Capability mask bits */
 #define MACB_CAPS_ISR_CLEAR_ON_WRITE		0x00000001
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 5e6d27d33d43..7cdadc200c28 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -337,11 +337,30 @@  static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	if (status < 0)
 		goto mdio_read_exit;
 
-	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
-			      | MACB_BF(RW, MACB_MAN_READ)
-			      | MACB_BF(PHYA, mii_id)
-			      | MACB_BF(REGA, regnum)
-			      | MACB_BF(CODE, MACB_MAN_CODE)));
+	if (regnum & MII_ADDR_C45) {
+		macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF)
+			    | MACB_BF(RW, MACB_MAN_C45_ADDR)
+			    | MACB_BF(PHYA, mii_id)
+			    | MACB_BF(REGA, (regnum >> 16) & 0x1F)
+			    | MACB_BF(DATA, regnum & 0xFFFF)
+			    | MACB_BF(CODE, MACB_MAN_C45_CODE)));
+
+		status = macb_mdio_wait_for_idle(bp);
+		if (status < 0)
+			goto mdio_read_exit;
+
+		macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF)
+			    | MACB_BF(RW, MACB_MAN_C45_READ)
+			    | MACB_BF(PHYA, mii_id)
+			    | MACB_BF(REGA, (regnum >> 16) & 0x1F)
+			    | MACB_BF(CODE, MACB_MAN_C45_CODE)));
+	} else {
+		macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C22_SOF)
+				| MACB_BF(RW, MACB_MAN_C22_READ)
+				| MACB_BF(PHYA, mii_id)
+				| MACB_BF(REGA, regnum)
+				| MACB_BF(CODE, MACB_MAN_C22_CODE)));
+	}
 
 	status = macb_mdio_wait_for_idle(bp);
 	if (status < 0)
@@ -370,12 +389,32 @@  static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	if (status < 0)
 		goto mdio_write_exit;
 
-	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
-			      | MACB_BF(RW, MACB_MAN_WRITE)
-			      | MACB_BF(PHYA, mii_id)
-			      | MACB_BF(REGA, regnum)
-			      | MACB_BF(CODE, MACB_MAN_CODE)
-			      | MACB_BF(DATA, value)));
+	if (regnum & MII_ADDR_C45) {
+		macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF)
+			    | MACB_BF(RW, MACB_MAN_C45_ADDR)
+			    | MACB_BF(PHYA, mii_id)
+			    | MACB_BF(REGA, (regnum >> 16) & 0x1F)
+			    | MACB_BF(DATA, regnum & 0xFFFF)
+			    | MACB_BF(CODE, MACB_MAN_C45_CODE)));
+
+		status = macb_mdio_wait_for_idle(bp);
+		if (status < 0)
+			goto mdio_write_exit;
+
+		macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF)
+			    | MACB_BF(RW, MACB_MAN_C45_WRITE)
+			    | MACB_BF(PHYA, mii_id)
+			    | MACB_BF(REGA, (regnum >> 16) & 0x1F)
+			    | MACB_BF(CODE, MACB_MAN_C45_CODE)
+			    | MACB_BF(DATA, value)));
+	} else {
+		macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C22_SOF)
+				| MACB_BF(RW, MACB_MAN_C22_WRITE)
+				| MACB_BF(PHYA, mii_id)
+				| MACB_BF(REGA, regnum)
+				| MACB_BF(CODE, MACB_MAN_C22_CODE)
+				| MACB_BF(DATA, value)));
+	}
 
 	status = macb_mdio_wait_for_idle(bp);
 	if (status < 0)