| Message ID | 1574759389-103118-1-git-send-email-mparab@cadence.com |
|---|---|
| State | Deferred |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: macb: cover letter | expand |
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
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,
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
>-----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
> 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
> >> 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
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)
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(-)