* [PATCH 0/3] net: macb: cover letter
@ 2019-11-26 9:09 Milind Parab
2019-11-26 9:09 ` [PATCH 1/3] net: macb: fix for fixed-link mode Milind Parab
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: Milind Parab @ 2019-11-26 9:09 UTC (permalink / raw)
To: andrew, antoine.tenart
Cc: nicolas.ferre, davem, netdev, f.fainelli, hkallweit1,
linux-kernel, dkangude, pthombar, Milind Parab
Hi,
This patch set contains following patches
for Cadence 10G ethernet controller driver.
1. 0001-net-macb-fix-for-fixed-link-mode
This patch fix the issue with fixed link mode in macb.
2. 0002-net-macb-add-support-for-C45-MDIO-read-write
This patch is to support C45 PHY.
3. 0003-net-macb-add-support-for-high-speed-interface
This patch add support for 10G fixed mode.
Thanks,
Milind Parab
Milind Parab (3):
net: macb: fix for fixed-link mode
net: macb: add support for C45 MDIO read/write
net: macb: add support for high speed interface
drivers/net/ethernet/cadence/macb.h | 65 ++++++-
drivers/net/ethernet/cadence/macb_main.c | 215 +++++++++++++++++++----
2 files changed, 239 insertions(+), 41 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 1/3] net: macb: fix for fixed-link mode 2019-11-26 9:09 [PATCH 0/3] net: macb: cover letter Milind Parab @ 2019-11-26 9:09 ` Milind Parab 2019-11-26 14:30 ` Andrew Lunn 2019-11-27 18:02 ` Nicolas.Ferre 2019-11-26 9:09 ` [PATCH 2/3] net: macb: add support for C45 MDIO read/write Milind Parab ` (2 subsequent siblings) 3 siblings, 2 replies; 19+ messages in thread From: Milind Parab @ 2019-11-26 9:09 UTC (permalink / raw) To: andrew, antoine.tenart Cc: nicolas.ferre, davem, netdev, f.fainelli, hkallweit1, linux-kernel, dkangude, pthombar, Milind Parab This patch fix the issue with fixed link mode in macb. Signed-off-by: Milind Parab <mparab@cadence.com> --- drivers/net/ethernet/cadence/macb_main.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index d5ae2e1e0b0e..5e6d27d33d43 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -617,15 +617,11 @@ static int macb_phylink_connect(struct macb *bp) struct phy_device *phydev; int ret; - if (bp->pdev->dev.of_node && - of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) { + if (bp->pdev->dev.of_node) ret = phylink_of_phy_connect(bp->phylink, bp->pdev->dev.of_node, 0); - if (ret) { - netdev_err(dev, "Could not attach PHY (%d)\n", ret); - return ret; - } - } else { + + if ((!bp->pdev->dev.of_node || ret == -ENODEV) && bp->mii_bus) { phydev = phy_find_first(bp->mii_bus); if (!phydev) { netdev_err(dev, "no PHY found\n"); @@ -635,7 +631,7 @@ static int macb_phylink_connect(struct macb *bp) /* attach the mac to the phy */ ret = phylink_connect_phy(bp->phylink, phydev); if (ret) { - netdev_err(dev, "Could not attach to PHY (%d)\n", ret); + netdev_err(dev, "Could not attach to PHY\n"); return ret; } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] net: macb: fix for fixed-link mode 2019-11-26 9:09 ` [PATCH 1/3] net: macb: fix for fixed-link mode Milind Parab @ 2019-11-26 14:30 ` Andrew Lunn 2019-11-27 18:02 ` Nicolas.Ferre 1 sibling, 0 replies; 19+ messages in thread From: Andrew Lunn @ 2019-11-26 14:30 UTC (permalink / raw) To: Milind Parab Cc: antoine.tenart, nicolas.ferre, davem, netdev, f.fainelli, hkallweit1, linux-kernel, dkangude, pthombar On Tue, Nov 26, 2019 at 09:09:40AM +0000, Milind Parab wrote: > This patch fix the issue with fixed link mode in macb. Hi Milind. What issue is it fixing? Please give a good description here, so we can understand what/why the patch exists. Andrew ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] net: macb: fix for fixed-link mode 2019-11-26 9:09 ` [PATCH 1/3] net: macb: fix for fixed-link mode Milind Parab 2019-11-26 14:30 ` Andrew Lunn @ 2019-11-27 18:02 ` Nicolas.Ferre 2019-11-28 6:50 ` Milind Parab 1 sibling, 1 reply; 19+ messages in thread From: Nicolas.Ferre @ 2019-11-27 18:02 UTC (permalink / raw) To: mparab, andrew, antoine.tenart, f.fainelli Cc: davem, netdev, hkallweit1, linux-kernel, dkangude, pthombar, a.fatoum, brad.mouring, rmk+kernel On 26/11/2019 at 10:09, Milind Parab wrote: > This patch fix the issue with fixed link mode in macb. I would need more context here. What needs to be fixed? I think we had several attempts, at the phylib days, to have this part of the driver behave correctly, so providing us more insight will help understand what is going wrong now. For instance, is it related to the patch that converts the driver to the phylink interface done by this patch in net-next "net: macb: convert to phylink"? > Signed-off-by: Milind Parab <mparab@cadence.com> > --- > drivers/net/ethernet/cadence/macb_main.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index d5ae2e1e0b0e..5e6d27d33d43 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -617,15 +617,11 @@ static int macb_phylink_connect(struct macb *bp) > struct phy_device *phydev; > int ret; > > - if (bp->pdev->dev.of_node && > - of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) { You mean we don't need to parse this phandle anymore because it's better handled by phylink_of_phy_connect() below that takes care of the fixed-link case? If yes, then telling it in commit message is worth it... > + if (bp->pdev->dev.of_node) > ret = phylink_of_phy_connect(bp->phylink, bp->pdev->dev.of_node, > 0); > - if (ret) { > - netdev_err(dev, "Could not attach PHY (%d)\n", ret); > - return ret; > - } > - } else { > + > + if ((!bp->pdev->dev.of_node || ret == -ENODEV) && bp->mii_bus) { > phydev = phy_find_first(bp->mii_bus); > if (!phydev) { > netdev_err(dev, "no PHY found\n"); > @@ -635,7 +631,7 @@ static int macb_phylink_connect(struct macb *bp) > /* attach the mac to the phy */ > ret = phylink_connect_phy(bp->phylink, phydev); > if (ret) { > - netdev_err(dev, "Could not attach to PHY (%d)\n", ret); > + netdev_err(dev, "Could not attach to PHY\n"); Why modifying this? > return ret; > } > } > -- > 2.17.1 > Best regards, Nicolas -- Nicolas Ferre ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH 1/3] net: macb: fix for fixed-link mode 2019-11-27 18:02 ` Nicolas.Ferre @ 2019-11-28 6:50 ` Milind Parab 0 siblings, 0 replies; 19+ messages in thread From: Milind Parab @ 2019-11-28 6:50 UTC (permalink / raw) To: Nicolas.Ferre, andrew, antoine.tenart, f.fainelli Cc: davem, netdev, hkallweit1, linux-kernel, Dhananjay Vilasrao Kangude, Parshuram Raju Thombare, a.fatoum, brad.mouring, rmk+kernel >-----Original Message----- >From: Nicolas.Ferre@microchip.com <Nicolas.Ferre@microchip.com> >Sent: Wednesday, November 27, 2019 11:32 PM >To: Milind Parab <mparab@cadence.com>; andrew@lunn.ch; >antoine.tenart@bootlin.com; f.fainelli@gmail.com >Cc: davem@davemloft.net; netdev@vger.kernel.org; hkallweit1@gmail.com; >linux-kernel@vger.kernel.org; Dhananjay Vilasrao Kangude ><dkangude@cadence.com>; Parshuram Raju Thombare ><pthombar@cadence.com>; a.fatoum@pengutronix.de; >brad.mouring@ni.com; rmk+kernel@armlinux.org.uk >Subject: Re: [PATCH 1/3] net: macb: fix for fixed-link mode > >EXTERNAL MAIL > > >On 26/11/2019 at 10:09, Milind Parab wrote: >> This patch fix the issue with fixed link mode in macb. > >I would need more context here. What needs to be fixed? > >I think we had several attempts, at the phylib days, to have this part of the >driver behave correctly, so providing us more insight will help understand >what is going wrong now. >For instance, is it related to the patch that converts the driver to the phylink >interface done by this patch in net-next "net: macb: convert to phylink"? > Yes, this is related to the patch that converts the driver to phylink With phylink patch, in fixed-link the device open is failing. The reason for failure is because here an attempt is made to search and connect to PHY even for the fixed-link. phylink_of_phy_connect() handles this case well, and for the fixed-link it just returns 0. So, further steps to search and connect to PHY are not needed. This patch solves this problem by allowing phylink_of_phy_connect() to take this decision > >> Signed-off-by: Milind Parab <mparab@cadence.com> >> --- >> drivers/net/ethernet/cadence/macb_main.c | 12 ++++-------- >> 1 file changed, 4 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb_main.c >> b/drivers/net/ethernet/cadence/macb_main.c >> index d5ae2e1e0b0e..5e6d27d33d43 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -617,15 +617,11 @@ static int macb_phylink_connect(struct macb *bp) >> struct phy_device *phydev; >> int ret; >> >> - if (bp->pdev->dev.of_node && >> - of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) { > >You mean we don't need to parse this phandle anymore because it's better >handled by phylink_of_phy_connect() below that takes care of the fixed-link >case? >If yes, then telling it in commit message is worth it... Yes, this we will explain in the commit message > >> + if (bp->pdev->dev.of_node) >> ret = phylink_of_phy_connect(bp->phylink, bp->pdev- >>dev.of_node, >> 0); >> - if (ret) { >> - netdev_err(dev, "Could not attach PHY (%d)\n", ret); >> - return ret; >> - } >> - } else { >> + >> + if ((!bp->pdev->dev.of_node || ret == -ENODEV) && bp->mii_bus) >> + { >> phydev = phy_find_first(bp->mii_bus); >> if (!phydev) { >> netdev_err(dev, "no PHY found\n"); @@ -635,7 >> +631,7 @@ static int macb_phylink_connect(struct macb *bp) >> /* attach the mac to the phy */ >> ret = phylink_connect_phy(bp->phylink, phydev); >> if (ret) { >> - netdev_err(dev, "Could not attach to PHY (%d)\n", ret); >> + netdev_err(dev, "Could not attach to PHY\n"); > >Why modifying this? This is by mistake. This will be corrected in the revision patch > >> return ret; >> } >> } >> -- >> 2.17.1 >> > >Best regards, > Nicolas > >-- >Nicolas Ferre ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/3] net: macb: add support for C45 MDIO read/write 2019-11-26 9:09 [PATCH 0/3] net: macb: cover letter Milind Parab 2019-11-26 9:09 ` [PATCH 1/3] net: macb: fix for fixed-link mode Milind Parab @ 2019-11-26 9:09 ` Milind Parab 2019-11-26 14:37 ` Andrew Lunn 2019-11-26 9:09 ` [PATCH 3/3] net: macb: add support for high speed interface Milind Parab 2019-11-26 18:09 ` [PATCH 0/3] net: macb: cover letter David Miller 3 siblings, 1 reply; 19+ messages in thread From: Milind Parab @ 2019-11-26 9:09 UTC (permalink / raw) To: andrew, antoine.tenart Cc: nicolas.ferre, davem, netdev, f.fainelli, hkallweit1, linux-kernel, dkangude, pthombar, Milind Parab 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(-) 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) -- 2.17.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] net: macb: add support for C45 MDIO read/write 2019-11-26 9:09 ` [PATCH 2/3] net: macb: add support for C45 MDIO read/write Milind Parab @ 2019-11-26 14:37 ` Andrew Lunn 2019-11-27 18:31 ` Nicolas.Ferre 0 siblings, 1 reply; 19+ messages in thread From: Andrew Lunn @ 2019-11-26 14:37 UTC (permalink / raw) To: Milind Parab Cc: antoine.tenart, nicolas.ferre, davem, netdev, f.fainelli, hkallweit1, linux-kernel, dkangude, pthombar 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] net: macb: add support for C45 MDIO read/write 2019-11-26 14:37 ` Andrew Lunn @ 2019-11-27 18:31 ` Nicolas.Ferre 2019-11-27 18:51 ` Andrew Lunn 0 siblings, 1 reply; 19+ messages in thread From: Nicolas.Ferre @ 2019-11-27 18:31 UTC (permalink / raw) To: andrew, mparab Cc: antoine.tenart, davem, netdev, f.fainelli, hkallweit1, linux-kernel, dkangude, pthombar, rmk+kernel 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, -- Nicolas Ferre ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] net: macb: add support for C45 MDIO read/write 2019-11-27 18:31 ` Nicolas.Ferre @ 2019-11-27 18:51 ` Andrew Lunn 2019-11-28 8:29 ` Milind Parab 0 siblings, 1 reply; 19+ messages in thread From: Andrew Lunn @ 2019-11-27 18:51 UTC (permalink / raw) To: Nicolas.Ferre Cc: mparab, antoine.tenart, davem, netdev, f.fainelli, hkallweit1, linux-kernel, dkangude, pthombar, rmk+kernel 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH 2/3] net: macb: add support for C45 MDIO read/write 2019-11-27 18:51 ` Andrew Lunn @ 2019-11-28 8:29 ` Milind Parab 2019-11-28 14:52 ` Andrew Lunn 0 siblings, 1 reply; 19+ messages in thread From: Milind Parab @ 2019-11-28 8:29 UTC (permalink / raw) To: Andrew Lunn, Nicolas.Ferre Cc: antoine.tenart, davem, netdev, f.fainelli, hkallweit1, linux-kernel, Dhananjay Vilasrao Kangude, Parshuram Raju Thombare, rmk+kernel >-----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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] net: macb: add support for C45 MDIO read/write 2019-11-28 8:29 ` Milind Parab @ 2019-11-28 14:52 ` Andrew Lunn 2019-11-29 10:02 ` Milind Parab 0 siblings, 1 reply; 19+ messages in thread From: Andrew Lunn @ 2019-11-28 14:52 UTC (permalink / raw) To: Milind Parab Cc: Nicolas.Ferre, antoine.tenart, davem, netdev, f.fainelli, hkallweit1, linux-kernel, Dhananjay Vilasrao Kangude, Parshuram Raju Thombare, rmk+kernel > 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH 2/3] net: macb: add support for C45 MDIO read/write 2019-11-28 14:52 ` Andrew Lunn @ 2019-11-29 10:02 ` Milind Parab 0 siblings, 0 replies; 19+ messages in thread From: Milind Parab @ 2019-11-29 10:02 UTC (permalink / raw) To: Andrew Lunn Cc: Nicolas.Ferre, antoine.tenart, davem, netdev, f.fainelli, hkallweit1, linux-kernel, Dhananjay Vilasrao Kangude, Parshuram Raju Thombare, rmk+kernel > >> 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] net: macb: add support for high speed interface 2019-11-26 9:09 [PATCH 0/3] net: macb: cover letter Milind Parab 2019-11-26 9:09 ` [PATCH 1/3] net: macb: fix for fixed-link mode Milind Parab 2019-11-26 9:09 ` [PATCH 2/3] net: macb: add support for C45 MDIO read/write Milind Parab @ 2019-11-26 9:09 ` Milind Parab 2019-11-26 18:09 ` [PATCH 0/3] net: macb: cover letter David Miller 3 siblings, 0 replies; 19+ messages in thread From: Milind Parab @ 2019-11-26 9:09 UTC (permalink / raw) To: andrew, antoine.tenart Cc: nicolas.ferre, davem, netdev, f.fainelli, hkallweit1, linux-kernel, dkangude, pthombar, Milind Parab This patch add support for high speed USXGMII PCS and 10G speed in Cadence ethernet controller driver. Signed-off-by: Milind Parab <mparab@cadence.com> --- drivers/net/ethernet/cadence/macb.h | 50 ++++++++ drivers/net/ethernet/cadence/macb_main.c | 142 ++++++++++++++++++++--- 2 files changed, 174 insertions(+), 18 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index dbf7070fcdba..b731807d1c49 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -76,10 +76,12 @@ #define MACB_RBQPH 0x04D4 /* GEM register offsets. */ +#define GEM_NCR 0x0000 /* Network Control */ #define GEM_NCFGR 0x0004 /* Network Config */ #define GEM_USRIO 0x000c /* User IO */ #define GEM_DMACFG 0x0010 /* DMA Configuration */ #define GEM_JML 0x0048 /* Jumbo Max Length */ +#define GEM_HS_MAC_CONFIG 0x0050 /* GEM high speed config */ #define GEM_HRB 0x0080 /* Hash Bottom */ #define GEM_HRT 0x0084 /* Hash Top */ #define GEM_SA1B 0x0088 /* Specific1 Bottom */ @@ -164,6 +166,9 @@ #define GEM_DCFG7 0x0298 /* Design Config 7 */ #define GEM_DCFG8 0x029C /* Design Config 8 */ #define GEM_DCFG10 0x02A4 /* Design Config 10 */ +#define GEM_DCFG12 0x02AC /* Design Config 12 */ +#define GEM_USX_CONTROL 0x0A80 /* USXGMII control register */ +#define GEM_USX_STATUS 0x0A88 /* USXGMII status register */ #define GEM_TXBDCTRL 0x04cc /* TX Buffer Descriptor control register */ #define GEM_RXBDCTRL 0x04d0 /* RX Buffer Descriptor control register */ @@ -270,11 +275,19 @@ #define MACB_IRXFCS_OFFSET 19 #define MACB_IRXFCS_SIZE 1 +/* GEM specific NCR bitfields. */ +#define GEM_ENABLE_HS_MAC_OFFSET 31 +#define GEM_ENABLE_HS_MAC_SIZE 1 + /* GEM specific NCFGR bitfields. */ +#define GEM_FD_OFFSET 1 /* Full duplex */ +#define GEM_FD_SIZE 1 #define GEM_GBE_OFFSET 10 /* Gigabit mode enable */ #define GEM_GBE_SIZE 1 #define GEM_PCSSEL_OFFSET 11 #define GEM_PCSSEL_SIZE 1 +#define GEM_PAE_OFFSET 13 /* Pause enable */ +#define GEM_PAE_SIZE 1 #define GEM_CLK_OFFSET 18 /* MDC clock division */ #define GEM_CLK_SIZE 3 #define GEM_DBW_OFFSET 21 /* Data bus width */ @@ -455,11 +468,17 @@ #define MACB_REV_OFFSET 0 #define MACB_REV_SIZE 16 +/* Bitfield in HS_MAC_CONFIG */ +#define GEM_HS_MAC_SPEED_OFFSET 0 +#define GEM_HS_MAC_SPEED_SIZE 3 + /* Bitfields in DCFG1. */ #define GEM_IRQCOR_OFFSET 23 #define GEM_IRQCOR_SIZE 1 #define GEM_DBWDEF_OFFSET 25 #define GEM_DBWDEF_SIZE 3 +#define GEM_NO_PCS_OFFSET 0 +#define GEM_NO_PCS_SIZE 1 /* Bitfields in DCFG2. */ #define GEM_RX_PKT_BUFF_OFFSET 20 @@ -494,6 +513,34 @@ #define GEM_RXBD_RDBUFF_OFFSET 8 #define GEM_RXBD_RDBUFF_SIZE 4 +/* Bitfields in DCFG12. */ +#define GEM_HIGH_SPEED_OFFSET 26 +#define GEM_HIGH_SPEED_SIZE 1 + +/* Bitfields in USX_CONTROL. */ +#define GEM_USX_CTRL_SPEED_OFFSET 14 +#define GEM_USX_CTRL_SPEED_SIZE 3 +#define GEM_SERDES_RATE_OFFSET 12 +#define GEM_SERDES_RATE_SIZE 2 +#define GEM_RX_SCR_BYPASS_OFFSET 9 +#define GEM_RX_SCR_BYPASS_SIZE 1 +#define GEM_TX_SCR_BYPASS_OFFSET 8 +#define GEM_TX_SCR_BYPASS_SIZE 1 +#define GEM_RX_SYNC_RESET_OFFSET 2 +#define GEM_RX_SYNC_RESET_SIZE 1 +#define GEM_TX_EN_OFFSET 1 +#define GEM_TX_EN_SIZE 1 +#define GEM_SIGNAL_OK_OFFSET 0 +#define GEM_SIGNAL_OK_SIZE 1 + +/* Bitfields in USX_STATUS. */ +#define GEM_USX_TX_FAULT_OFFSET 28 +#define GEM_USX_TX_FAULT_SIZE 1 +#define GEM_USX_RX_FAULT_OFFSET 27 +#define GEM_USX_RX_FAULT_SIZE 1 +#define GEM_USX_BLOCK_LOCK_OFFSET 0 +#define GEM_USX_BLOCK_LOCK_SIZE 1 + /* Bitfields in TISUBN */ #define GEM_SUBNSINCR_OFFSET 0 #define GEM_SUBNSINCRL_OFFSET 24 @@ -656,6 +703,8 @@ #define MACB_CAPS_GIGABIT_MODE_AVAILABLE 0x20000000 #define MACB_CAPS_SG_DISABLED 0x40000000 #define MACB_CAPS_MACB_IS_GEM 0x80000000 +#define MACB_CAPS_PCS 0x01000000 +#define MACB_CAPS_HIGH_SPEED 0x02000000 /* LSO settings */ #define MACB_LSO_UFO_ENABLE 0x01 @@ -724,6 +773,7 @@ }) #define MACB_READ_NSR(bp) macb_readl(bp, NSR) +#define GEM_READ_USX_STATUS(bp) gem_readl(bp, USX_STATUS) /* struct macb_dma_desc - Hardware DMA descriptor * @addr: DMA address of data buffer diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 7cdadc200c28..832d4481c337 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -81,6 +81,18 @@ struct sifive_fu540_macb_mgmt { #define MACB_WOL_HAS_MAGIC_PACKET (0x1 << 0) #define MACB_WOL_ENABLED (0x1 << 1) +enum { + HS_MAC_SPEED_100M, + HS_MAC_SPEED_1000M, + HS_MAC_SPEED_2500M, + HS_MAC_SPEED_5000M, + HS_MAC_SPEED_10000M, +}; + +enum { + MACB_SERDES_RATE_10G = 1, +}; + /* Graceful stop timeouts in us. We should allow up to * 1 frame time (10 Mbits/s, full-duplex, ignoring collisions) */ @@ -90,6 +102,8 @@ struct sifive_fu540_macb_mgmt { #define MACB_MDIO_TIMEOUT 1000000 /* in usecs */ +#define MACB_USX_BLOCK_LOCK_TIMEOUT 1000000 /* in usecs */ + /* DMA buffer descriptor might be different size * depends on hardware configuration: * @@ -506,6 +520,7 @@ static void macb_validate(struct phylink_config *config, state->interface != PHY_INTERFACE_MODE_RMII && state->interface != PHY_INTERFACE_MODE_GMII && state->interface != PHY_INTERFACE_MODE_SGMII && + state->interface != PHY_INTERFACE_MODE_USXGMII && !phy_interface_mode_is_rgmii(state->interface)) { bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); return; @@ -518,6 +533,13 @@ static void macb_validate(struct phylink_config *config, return; } + if (state->interface == PHY_INTERFACE_MODE_USXGMII && + !(bp->caps & MACB_CAPS_HIGH_SPEED && + bp->caps & MACB_CAPS_PCS)) { + bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS); + return; + } + phylink_set_port_modes(mask); phylink_set(mask, Autoneg); phylink_set(mask, Asym_Pause); @@ -527,6 +549,22 @@ static void macb_validate(struct phylink_config *config, phylink_set(mask, 100baseT_Half); phylink_set(mask, 100baseT_Full); + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE && + (state->interface == PHY_INTERFACE_MODE_NA || + state->interface == PHY_INTERFACE_MODE_USXGMII)) { + phylink_set(mask, 10000baseCR_Full); + phylink_set(mask, 10000baseER_Full); + phylink_set(mask, 10000baseKR_Full); + phylink_set(mask, 10000baseLR_Full); + phylink_set(mask, 10000baseLRM_Full); + phylink_set(mask, 10000baseSR_Full); + phylink_set(mask, 10000baseT_Full); + phylink_set(mask, 5000baseT_Full); + phylink_set(mask, 2500baseX_Full); + phylink_set(mask, 1000baseX_Full); + phylink_set(mask, 1000baseT_Full); + } + if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE && (state->interface == PHY_INTERFACE_MODE_NA || state->interface == PHY_INTERFACE_MODE_GMII || @@ -544,6 +582,60 @@ static void macb_validate(struct phylink_config *config, __ETHTOOL_LINK_MODE_MASK_NBITS); } +static int gem_mac_usx_configure(struct macb *bp, + const struct phylink_link_state *state) +{ + u32 speed, config, val; + int ret; + + val = gem_readl(bp, NCFGR); + val = GEM_BIT(PCSSEL) | (~GEM_BIT(SGMIIEN) & val); + if (state->pause & MLO_PAUSE_TX) + val |= GEM_BIT(PAE); + gem_writel(bp, NCFGR, val); + gem_writel(bp, NCR, gem_readl(bp, NCR) | GEM_BIT(ENABLE_HS_MAC)); + gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) | GEM_BIT(FD)); + config = gem_readl(bp, USX_CONTROL); + config = GEM_BFINS(SERDES_RATE, MACB_SERDES_RATE_10G, config); + config &= ~GEM_BIT(TX_SCR_BYPASS); + config &= ~GEM_BIT(RX_SCR_BYPASS); + gem_writel(bp, USX_CONTROL, config | GEM_BIT(TX_EN)); + config = gem_readl(bp, USX_CONTROL); + gem_writel(bp, USX_CONTROL, config | GEM_BIT(SIGNAL_OK)); + ret = readx_poll_timeout(GEM_READ_USX_STATUS, bp, val, + val & GEM_BIT(USX_BLOCK_LOCK), + 1, MACB_USX_BLOCK_LOCK_TIMEOUT); + if (ret < 0) { + netdev_warn(bp->dev, "USXGMII block lock failed"); + return -ETIMEDOUT; + } + + switch (state->speed) { + case SPEED_10000: + speed = HS_MAC_SPEED_10000M; + break; + case SPEED_5000: + speed = HS_MAC_SPEED_5000M; + break; + case SPEED_2500: + speed = HS_MAC_SPEED_2500M; + break; + case SPEED_1000: + speed = HS_MAC_SPEED_1000M; + break; + default: + case SPEED_100: + speed = HS_MAC_SPEED_100M; + break; + } + + gem_writel(bp, HS_MAC_CONFIG, GEM_BFINS(HS_MAC_SPEED, speed, + gem_readl(bp, HS_MAC_CONFIG))); + gem_writel(bp, USX_CONTROL, GEM_BFINS(USX_CTRL_SPEED, speed, + gem_readl(bp, USX_CONTROL))); + return 0; +} + static void macb_mac_pcs_get_state(struct phylink_config *config, struct phylink_link_state *state) { @@ -565,30 +657,39 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode, spin_lock_irqsave(&bp->lock, flags); - old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR); + if (bp->phy_interface == PHY_INTERFACE_MODE_USXGMII) { + if (gem_mac_usx_configure(bp, state) < 0) { + spin_unlock_irqrestore(&bp->lock, flags); + phylink_mac_change(bp->phylink, false); + return; + } + } else { + old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR); - /* Clear all the bits we might set later */ - ctrl &= ~(GEM_BIT(GBE) | MACB_BIT(SPD) | MACB_BIT(FD) | MACB_BIT(PAE) | - GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL)); + /* Clear all the bits we might set later */ + ctrl &= ~(GEM_BIT(GBE) | MACB_BIT(SPD) | + MACB_BIT(FD) | MACB_BIT(PAE) | + GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL)); - if (state->speed == SPEED_1000) - ctrl |= GEM_BIT(GBE); - else if (state->speed == SPEED_100) - ctrl |= MACB_BIT(SPD); + if (state->speed == SPEED_1000) + ctrl |= GEM_BIT(GBE); + else if (state->speed == SPEED_100) + ctrl |= MACB_BIT(SPD); - if (state->duplex) - ctrl |= MACB_BIT(FD); + if (state->duplex) + ctrl |= MACB_BIT(FD); - /* We do not support MLO_PAUSE_RX yet */ - if (state->pause & MLO_PAUSE_TX) - ctrl |= MACB_BIT(PAE); + /* We do not support MLO_PAUSE_RX yet */ + if (state->pause & MLO_PAUSE_TX) + ctrl |= MACB_BIT(PAE); - if (state->interface == PHY_INTERFACE_MODE_SGMII) - ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL); + if (state->interface == PHY_INTERFACE_MODE_SGMII) + ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL); - /* Apply the new configuration, if any */ - if (old_ctrl ^ ctrl) - macb_or_gem_writel(bp, NCFGR, ctrl); + /* Apply the new configuration, if any */ + if (old_ctrl ^ ctrl) + macb_or_gem_writel(bp, NCFGR, ctrl); + } bp->speed = state->speed; @@ -3396,6 +3497,11 @@ static void macb_configure_caps(struct macb *bp, dcfg = gem_readl(bp, DCFG1); if (GEM_BFEXT(IRQCOR, dcfg) == 0) bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE; + if (GEM_BFEXT(NO_PCS, dcfg) == 0) + bp->caps |= MACB_CAPS_PCS; + dcfg = gem_readl(bp, DCFG12); + if (GEM_BFEXT(HIGH_SPEED, dcfg) == 1) + bp->caps |= MACB_CAPS_HIGH_SPEED; dcfg = gem_readl(bp, DCFG2); if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0) bp->caps |= MACB_CAPS_FIFO_MODE; -- 2.17.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] net: macb: cover letter 2019-11-26 9:09 [PATCH 0/3] net: macb: cover letter Milind Parab ` (2 preceding siblings ...) 2019-11-26 9:09 ` [PATCH 3/3] net: macb: add support for high speed interface Milind Parab @ 2019-11-26 18:09 ` David Miller 3 siblings, 0 replies; 19+ messages in thread From: David Miller @ 2019-11-26 18:09 UTC (permalink / raw) To: mparab Cc: andrew, antoine.tenart, nicolas.ferre, netdev, f.fainelli, hkallweit1, linux-kernel, dkangude, pthombar You need to say something better than "cover letter", state what the overall series is doing at a high level, in the most concise way. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G @ 2019-12-09 11:13 Milind Parab 2019-12-09 11:14 ` [PATCH 1/3] net: macb: fix for fixed-link mode Milind Parab 0 siblings, 1 reply; 19+ messages in thread From: Milind Parab @ 2019-12-09 11:13 UTC (permalink / raw) To: nicolas.nerre, andrew, antoine.tenart, f.fainelli Cc: davem, netdev, hkallweit1, linux-kernel, dkangude, a.fatoum, brad.mouring, rmk+kernel, pthombar, Milind Parab This patch series applies to Cadence Ethernet MAC Controller. The first patch in this series is related to the patch that converts the driver to phylink interface in net-next "net: macb: convert to phylink". Next patch is for adding support for C45 interface and the final patch adds 10G MAC support. The patch sequences are detailed as below 1. 0001-net-macb-fix-for-fixed-link-mode This patch fix the issue with fixed-link mode in macb phylink interface. In fixed-link we don't need to parse phandle because it's better handled by phylink_of_phy_connect() 2. 0002-net-macb-add-support-for-C45-MDIO-read-write This patch is to support C45 interface to PHY. This upgrade is downward compatible. 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. 3. 0003-net-macb-add-support-for-high-speed-interface This patch add support for 10G fixed mode. Thanks, Milind Parab Milind Parab (3): net: macb: fix for fixed-link mode net: macb: add support for C45 MDIO read/write net: macb: add support for high speed interface drivers/net/ethernet/cadence/macb.h | 65 ++++++- drivers/net/ethernet/cadence/macb_main.c | 220 +++++++++++++++++++---- 2 files changed, 243 insertions(+), 42 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/3] net: macb: fix for fixed-link mode 2019-12-09 11:13 [PATCH 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G Milind Parab @ 2019-12-09 11:14 ` Milind Parab 2019-12-09 11:26 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 19+ messages in thread From: Milind Parab @ 2019-12-09 11:14 UTC (permalink / raw) To: nicolas.nerre, andrew, antoine.tenart, f.fainelli Cc: davem, netdev, hkallweit1, linux-kernel, dkangude, a.fatoum, brad.mouring, rmk+kernel, pthombar, Milind Parab This patch fix the issue with fixed link. With fixed-link device opening fails due to macb_phylink_connect not handling fixed-link mode, in which case no MAC-PHY connection is needed and phylink_connect return success (0), however in current driver attempt is made to search and connect to PHY even for fixed-link. Signed-off-by: Milind Parab <mparab@cadence.com> --- drivers/net/ethernet/cadence/macb_main.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 9c767ee252ac..6b68ef34ab19 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -615,17 +615,13 @@ static int macb_phylink_connect(struct macb *bp) { struct net_device *dev = bp->dev; struct phy_device *phydev; + struct device_node *dn = bp->pdev->dev.of_node; int ret; - if (bp->pdev->dev.of_node && - of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) { - ret = phylink_of_phy_connect(bp->phylink, bp->pdev->dev.of_node, - 0); - if (ret) { - netdev_err(dev, "Could not attach PHY (%d)\n", ret); - return ret; - } - } else { + if (dn) + ret = phylink_of_phy_connect(bp->phylink, dn, 0); + + if (!dn || (ret && !of_parse_phandle(dn, "phy-handle", 0))) { phydev = phy_find_first(bp->mii_bus); if (!phydev) { netdev_err(dev, "no PHY found\n"); @@ -638,6 +634,9 @@ static int macb_phylink_connect(struct macb *bp) netdev_err(dev, "Could not attach to PHY (%d)\n", ret); return ret; } + } else if (ret) { + netdev_err(dev, "Could not attach PHY (%d)\n", ret); + return ret; } phylink_start(bp->phylink); -- 2.17.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] net: macb: fix for fixed-link mode 2019-12-09 11:14 ` [PATCH 1/3] net: macb: fix for fixed-link mode Milind Parab @ 2019-12-09 11:26 ` Russell King - ARM Linux admin 2019-12-10 9:14 ` Milind Parab 0 siblings, 1 reply; 19+ messages in thread From: Russell King - ARM Linux admin @ 2019-12-09 11:26 UTC (permalink / raw) To: Milind Parab Cc: nicolas.nerre, andrew, antoine.tenart, f.fainelli, davem, netdev, hkallweit1, linux-kernel, dkangude, a.fatoum, brad.mouring, pthombar On Mon, Dec 09, 2019 at 11:14:21AM +0000, Milind Parab wrote: > This patch fix the issue with fixed link. With fixed-link > device opening fails due to macb_phylink_connect not > handling fixed-link mode, in which case no MAC-PHY connection > is needed and phylink_connect return success (0), however > in current driver attempt is made to search and connect to > PHY even for fixed-link. > > Signed-off-by: Milind Parab <mparab@cadence.com> > --- > drivers/net/ethernet/cadence/macb_main.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 9c767ee252ac..6b68ef34ab19 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -615,17 +615,13 @@ static int macb_phylink_connect(struct macb *bp) > { > struct net_device *dev = bp->dev; > struct phy_device *phydev; > + struct device_node *dn = bp->pdev->dev.of_node; > int ret; > > - if (bp->pdev->dev.of_node && > - of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) { > - ret = phylink_of_phy_connect(bp->phylink, bp->pdev->dev.of_node, > - 0); > - if (ret) { > - netdev_err(dev, "Could not attach PHY (%d)\n", ret); > - return ret; > - } > - } else { > + if (dn) > + ret = phylink_of_phy_connect(bp->phylink, dn, 0); > + > + if (!dn || (ret && !of_parse_phandle(dn, "phy-handle", 0))) { Hi, If of_parse_phandle() returns non-null, the device_node it returns will have its reference count increased by one. That reference needs to be put. I assume you're trying to determine whether phylink_of_phy_connect() failed because of a missing phy-handle rather than of_phy_attach() failing? Maybe those two failures ought to be distinguished by errno return value? of_phy_attach() may fail due to of_phy_find_device() failing to find the PHY, or phy_attach_direct() failing. We could switch from using of_phy_attach(), to using of_phy_find_device() directly so we can then propagate phy_attach_direct()'s error code back, rather than losing it. That would then leave the case of of_phy_find_device() failure to be considered in terms of errno return value. > phydev = phy_find_first(bp->mii_bus); > if (!phydev) { > netdev_err(dev, "no PHY found\n"); > @@ -638,6 +634,9 @@ static int macb_phylink_connect(struct macb *bp) > netdev_err(dev, "Could not attach to PHY (%d)\n", ret); > return ret; > } > + } else if (ret) { > + netdev_err(dev, "Could not attach PHY (%d)\n", ret); > + return ret; > } > > phylink_start(bp->phylink); > -- > 2.17.1 > > -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH 1/3] net: macb: fix for fixed-link mode 2019-12-09 11:26 ` Russell King - ARM Linux admin @ 2019-12-10 9:14 ` Milind Parab 2019-12-10 11:38 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 19+ messages in thread From: Milind Parab @ 2019-12-10 9:14 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: nicolas.nerre, andrew, antoine.tenart, f.fainelli, davem, netdev, hkallweit1, linux-kernel, Dhananjay Vilasrao Kangude, a.fatoum, brad.mouring, Parshuram Raju Thombare >> This patch fix the issue with fixed link. With fixed-link >> device opening fails due to macb_phylink_connect not >> handling fixed-link mode, in which case no MAC-PHY connection >> is needed and phylink_connect return success (0), however >> in current driver attempt is made to search and connect to >> PHY even for fixed-link. >> >> Signed-off-by: Milind Parab <mparab@cadence.com> >> --- >> drivers/net/ethernet/cadence/macb_main.c | 17 ++++++++--------- >> 1 file changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >> index 9c767ee252ac..6b68ef34ab19 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -615,17 +615,13 @@ static int macb_phylink_connect(struct macb *bp) >> { >> struct net_device *dev = bp->dev; >> struct phy_device *phydev; >> + struct device_node *dn = bp->pdev->dev.of_node; >> int ret; >> >> - if (bp->pdev->dev.of_node && >> - of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) { >> - ret = phylink_of_phy_connect(bp->phylink, bp->pdev- >>dev.of_node, >> - 0); >> - if (ret) { >> - netdev_err(dev, "Could not attach PHY (%d)\n", ret); >> - return ret; >> - } >> - } else { >> + if (dn) >> + ret = phylink_of_phy_connect(bp->phylink, dn, 0); >> + >> + if (!dn || (ret && !of_parse_phandle(dn, "phy-handle", 0))) { > >Hi, >If of_parse_phandle() returns non-null, the device_node it returns will >have its reference count increased by one. That reference needs to be >put. > Okay, as per your suggestion below addition will be okay to store the "phy_node" and then of_node_put(phy_node) on error phy_node = of_parse_phandle(dn, "phy-handle", 0); if (!dn || (ret && !phy_node)) { phydev = phy_find_first(bp->mii_bus); if (!phydev) { netdev_err(dev, "no PHY found\n"); ret = -ENXIO; goto phylink_connect_err; } /* attach the mac to the phy */ ret = phylink_connect_phy(bp->phylink, phydev); if (ret) { netdev_err(dev, "Could not attach to PHY (%d)\n", ret); goto phylink_connect_err; } } else if (ret) { netdev_err(dev, "Could not attach PHY (%d)\n", ret); goto phylink_connect_err; } phylink_start(bp->phylink); phylink_connect_err: if (phy_node) of_node_put(phy_node); return ret; >I assume you're trying to determine whether phylink_of_phy_connect() >failed because of a missing phy-handle rather than of_phy_attach() >failing? Maybe those two failures ought to be distinguished by errno >return value? Yes, PHY will be scanned only if phylink_of_phy_connect() returns error due to missing "phy-handle". Currently, phylink_of_phy_connect() returns same error for missing "phy-handle" and of_phy_attach() failure. >of_phy_attach() may fail due to of_phy_find_device() failing to find >the PHY, or phy_attach_direct() failing. We could switch from using >of_phy_attach(), to using of_phy_find_device() directly so we can then >propagate phy_attach_direct()'s error code back, rather than losing it. >That would then leave the case of of_phy_find_device() failure to be >considered in terms of errno return value. >> phydev = phy_find_first(bp->mii_bus); >> if (!phydev) { >> netdev_err(dev, "no PHY found\n"); >> @@ -638,6 +634,9 @@ static int macb_phylink_connect(struct macb *bp) >> netdev_err(dev, "Could not attach to PHY (%d)\n",ret); >> return ret; >> } >> + } else if (ret) { >> + netdev_err(dev, "Could not attach PHY (%d)\n", ret); >> + return ret; >> } >> >> phylink_start(bp->phylink); >> -- >> 2.17.1 >> >> >--RMK's Patch system: https://urldefense.proofpoint.com/v2/url?u=https- >3A__www.armlinux.org.uk_developer_patches_&d=DwIBAg&c=aUq983L2pue >2FqKFoP6PGHMJQyoJ7kl3s3GZ- >_haXqY&r=BDdk1JtITE_JJ0519WwqU7IKF80Cw1i55lZOGqv2su8&m=blnuaRbic >V2uF6XaoVuWN0U5yR5cFOzUSAs3ZPlxioU&s=rhp71ilc6R4_pmDsY07- >kLPGbhyoyixXoHF0hMGu4Go&e= > >FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps >up > >According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] net: macb: fix for fixed-link mode 2019-12-10 9:14 ` Milind Parab @ 2019-12-10 11:38 ` Russell King - ARM Linux admin 2019-12-11 8:21 ` Milind Parab 0 siblings, 1 reply; 19+ messages in thread From: Russell King - ARM Linux admin @ 2019-12-10 11:38 UTC (permalink / raw) To: Milind Parab Cc: nicolas.nerre, andrew, antoine.tenart, f.fainelli, davem, netdev, hkallweit1, linux-kernel, Dhananjay Vilasrao Kangude, a.fatoum, brad.mouring, Parshuram Raju Thombare On Tue, Dec 10, 2019 at 09:14:13AM +0000, Milind Parab wrote: > >> This patch fix the issue with fixed link. With fixed-link > >> device opening fails due to macb_phylink_connect not > >> handling fixed-link mode, in which case no MAC-PHY connection > >> is needed and phylink_connect return success (0), however > >> in current driver attempt is made to search and connect to > >> PHY even for fixed-link. > >> > >> Signed-off-by: Milind Parab <mparab@cadence.com> > >> --- > >> drivers/net/ethernet/cadence/macb_main.c | 17 ++++++++--------- > >> 1 file changed, 8 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > >> index 9c767ee252ac..6b68ef34ab19 100644 > >> --- a/drivers/net/ethernet/cadence/macb_main.c > >> +++ b/drivers/net/ethernet/cadence/macb_main.c > >> @@ -615,17 +615,13 @@ static int macb_phylink_connect(struct macb *bp) > >> { > >> struct net_device *dev = bp->dev; > >> struct phy_device *phydev; > >> + struct device_node *dn = bp->pdev->dev.of_node; > >> int ret; > >> > >> - if (bp->pdev->dev.of_node && > >> - of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) { > >> - ret = phylink_of_phy_connect(bp->phylink, bp->pdev- > >>dev.of_node, > >> - 0); > >> - if (ret) { > >> - netdev_err(dev, "Could not attach PHY (%d)\n", ret); > >> - return ret; > >> - } > >> - } else { > >> + if (dn) > >> + ret = phylink_of_phy_connect(bp->phylink, dn, 0); > >> + > >> + if (!dn || (ret && !of_parse_phandle(dn, "phy-handle", 0))) { > > > >Hi, > >If of_parse_phandle() returns non-null, the device_node it returns will > >have its reference count increased by one. That reference needs to be > >put. > > > > Okay, as per your suggestion below addition will be okay to store the "phy_node" and then of_node_put(phy_node) on error > > phy_node = of_parse_phandle(dn, "phy-handle", 0); > if (!dn || (ret && !phy_node)) { > phydev = phy_find_first(bp->mii_bus); ... > if (phy_node) > of_node_put(phy_node); As you're only interested in whether phy-handle exists or not, you could do this instead: phy_node = of_parse_phandle(dn, "phy-handle", 0); of_node_put(phy_node); if (!dn || (ret && !phy_node)) { ... Yes, it looks a bit weird, but the only thing you're interested in here is whether of_parse_phandle() returned NULL or non-NULL. You're not interested in dereferencing the pointer. Some may raise some eye-brows at that, so it may be better to have this as a helper: static bool macb_phy_handle_exists(struct device_node *dn) { dn = of_parse_phandle(dn, "phy-handle", 0); of_node_put(dn); return dn != NULL; } and use it as: if (!dn || (ret && !macb_phy_handle_exists(dn))) { which is more obvious what is going on. > > return ret; > > >I assume you're trying to determine whether phylink_of_phy_connect() > >failed because of a missing phy-handle rather than of_phy_attach() > >failing? Maybe those two failures ought to be distinguished by errno > >return value? > > Yes, PHY will be scanned only if phylink_of_phy_connect() returns error due to missing "phy-handle". > Currently, phylink_of_phy_connect() returns same error for missing "phy-handle" and of_phy_attach() failure. > > >of_phy_attach() may fail due to of_phy_find_device() failing to find > >the PHY, or phy_attach_direct() failing. We could switch from using > >of_phy_attach(), to using of_phy_find_device() directly so we can then > >propagate phy_attach_direct()'s error code back, rather than losing it. > >That would then leave the case of of_phy_find_device() failure to be > >considered in terms of errno return value. Here's a patch I quickly knocked up that does this - may not apply to the kernel you're using as there's a whole bunch of work I have outstanding, but gives the outline idea. Does this help? 8<=== From: Russell King <rmk+kernel@armlinux.org.uk> Subject: [PATCH] net: phylink: avoid of_phy_attach() of_phy_attach() hides the return value of phy_attach_direct(), forcing us to return a "generic" ENODEV error code that is indistinguishable from the lack-of-phy-property case. Switch to using of_phy_find_device() to find the PHY device, and then propagating any phy_attach_direct() error back to the caller. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phylink.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index e9036b72114c..5a5109428d9e 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -887,14 +887,17 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn, return 0; } - phy_dev = of_phy_attach(pl->netdev, phy_node, flags, - pl->link_interface); + phy_dev = of_phy_find_device(phy_node); /* We're done with the phy_node handle */ of_node_put(phy_node); - if (!phy_dev) return -ENODEV; + ret = phy_attach_direct(pl->netdev, phy_dev, flags, + pl->link_interface); + if (ret) + return ret; + ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface); if (ret) phy_detach(phy_dev); -- 2.20.1 -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up ^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [PATCH 1/3] net: macb: fix for fixed-link mode 2019-12-10 11:38 ` Russell King - ARM Linux admin @ 2019-12-11 8:21 ` Milind Parab 0 siblings, 0 replies; 19+ messages in thread From: Milind Parab @ 2019-12-11 8:21 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: nicolas.nerre, andrew, antoine.tenart, f.fainelli, davem, netdev, hkallweit1, linux-kernel, Dhananjay Vilasrao Kangude, a.fatoum, brad.mouring, Parshuram Raju Thombare >> >> + ret = phylink_of_phy_connect(bp->phylink, dn, 0); >> >> + >> >> + if (!dn || (ret && !of_parse_phandle(dn, "phy-handle", 0))) { >> > >> >Hi, >> >If of_parse_phandle() returns non-null, the device_node it returns will >> >have its reference count increased by one. That reference needs to be >> >put. >> > >> >> Okay, as per your suggestion below addition will be okay to store the >"phy_node" and then of_node_put(phy_node) on error >> >> phy_node = of_parse_phandle(dn, "phy-handle", 0); >> if (!dn || (ret && !phy_node)) { >> phydev = phy_find_first(bp->mii_bus); >... >> if (phy_node) >> of_node_put(phy_node); > >As you're only interested in whether phy-handle exists or not, you >could do this instead: > > phy_node = of_parse_phandle(dn, "phy-handle", 0); > of_node_put(phy_node); > if (!dn || (ret && !phy_node)) { > ... > >Yes, it looks a bit weird, but the only thing you're interested in >here is whether of_parse_phandle() returned NULL or non-NULL. You're >not interested in dereferencing the pointer. > >Some may raise some eye-brows at that, so it may be better to have >this as a helper: > >static bool macb_phy_handle_exists(struct device_node *dn) >{ > dn = of_parse_phandle(dn, "phy-handle", 0); > of_node_put(dn); > return dn != NULL; >} > >and use it as: > > if (!dn || (ret && !macb_phy_handle_exists(dn))) { > >which is more obvious what is going on. > This is good. I will put this in the revised patch. > >> >> return ret; >> >> >I assume you're trying to determine whether phylink_of_phy_connect() >> >failed because of a missing phy-handle rather than of_phy_attach() >> >failing? Maybe those two failures ought to be distinguished by errno >> >return value? >> >> Yes, PHY will be scanned only if phylink_of_phy_connect() returns error due to missing "phy-handle". >> Currently, phylink_of_phy_connect() returns same error for missing "phy-handle" and of_phy_attach() failure. >> >> >of_phy_attach() may fail due to of_phy_find_device() failing to find >> >the PHY, or phy_attach_direct() failing. We could switch from using >> >of_phy_attach(), to using of_phy_find_device() directly so we can then >> >propagate phy_attach_direct()'s error code back, rather than losing it. >> >That would then leave the case of of_phy_find_device() failure to be >> >considered in terms of errno return value. > >Here's a patch I quickly knocked up that does this - may not apply to >the kernel you're using as there's a whole bunch of work I have >outstanding, but gives the outline idea. Does this help? > > Yes, this will help. Once available, we will adopt this change. >8<=== >From: Russell King <rmk+kernel@armlinux.org.uk> >Subject: [PATCH] net: phylink: avoid of_phy_attach() > >of_phy_attach() hides the return value of phy_attach_direct(), forcing >us to return a "generic" ENODEV error code that is indistinguishable >from the lack-of-phy-property case. >Switch to using of_phy_find_device() to find the PHY device, and then >propagating any phy_attach_direct() error back to the caller. > > >Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >--- > ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-12-11 8:22 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-26 9:09 [PATCH 0/3] net: macb: cover letter Milind Parab 2019-11-26 9:09 ` [PATCH 1/3] net: macb: fix for fixed-link mode Milind Parab 2019-11-26 14:30 ` Andrew Lunn 2019-11-27 18:02 ` Nicolas.Ferre 2019-11-28 6:50 ` Milind Parab 2019-11-26 9:09 ` [PATCH 2/3] net: macb: add support for C45 MDIO read/write Milind Parab 2019-11-26 14:37 ` Andrew Lunn 2019-11-27 18:31 ` Nicolas.Ferre 2019-11-27 18:51 ` Andrew Lunn 2019-11-28 8:29 ` Milind Parab 2019-11-28 14:52 ` Andrew Lunn 2019-11-29 10:02 ` Milind Parab 2019-11-26 9:09 ` [PATCH 3/3] net: macb: add support for high speed interface Milind Parab 2019-11-26 18:09 ` [PATCH 0/3] net: macb: cover letter David Miller 2019-12-09 11:13 [PATCH 0/3] net: macb: fix for fixed link, support for c45 mdio and 10G Milind Parab 2019-12-09 11:14 ` [PATCH 1/3] net: macb: fix for fixed-link mode Milind Parab 2019-12-09 11:26 ` Russell King - ARM Linux admin 2019-12-10 9:14 ` Milind Parab 2019-12-10 11:38 ` Russell King - ARM Linux admin 2019-12-11 8:21 ` Milind Parab
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).