| Message ID | 1497298832-1896-1-git-send-email-joe.hershberger@ni.com |
|---|---|
| State | RFC |
| Delegated to: | Joe Hershberger |
| Headers | show |
On 06/12/2017 10:20 PM, Joe Hershberger wrote: > Don't wait forever, Pass errors back, etc. > > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> > > --- > This is a pass at improving the code quality. > This has not been tested in any way. > > drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 50 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c > index cf60d11..c8352d1 100644 > --- a/drivers/net/ag7xxx.c > +++ b/drivers/net/ag7xxx.c > @@ -26,6 +26,7 @@ enum ag7xxx_model { > AG7XXX_MODEL_AG934X, > }; > > +/* MAC Configuration 1 */ > #define AG7XXX_ETH_CFG1 0x00 > #define AG7XXX_ETH_CFG1_SOFT_RST BIT(31) > #define AG7XXX_ETH_CFG1_RX_RST BIT(19) > @@ -34,6 +35,7 @@ enum ag7xxx_model { > #define AG7XXX_ETH_CFG1_RX_EN BIT(2) > #define AG7XXX_ETH_CFG1_TX_EN BIT(0) > > +/* MAC Configuration 2 */ > #define AG7XXX_ETH_CFG2 0x04 > #define AG7XXX_ETH_CFG2_IF_1000 BIT(9) > #define AG7XXX_ETH_CFG2_IF_10_100 BIT(8) > @@ -43,26 +45,34 @@ enum ag7xxx_model { > #define AG7XXX_ETH_CFG2_PAD_CRC_EN BIT(2) > #define AG7XXX_ETH_CFG2_FDX BIT(0) > > +/* MII Configuration */ > #define AG7XXX_ETH_MII_MGMT_CFG 0x20 > #define AG7XXX_ETH_MII_MGMT_CFG_RESET BIT(31) > > +/* MII Command */ > #define AG7XXX_ETH_MII_MGMT_CMD 0x24 > #define AG7XXX_ETH_MII_MGMT_CMD_READ 0x1 > > +/* MII Address */ > #define AG7XXX_ETH_MII_MGMT_ADDRESS 0x28 > #define AG7XXX_ETH_MII_MGMT_ADDRESS_SHIFT 8 > > +/* MII Control */ > #define AG7XXX_ETH_MII_MGMT_CTRL 0x2c > > +/* MII Status */ > #define AG7XXX_ETH_MII_MGMT_STATUS 0x30 > > +/* MII Indicators */ > #define AG7XXX_ETH_MII_MGMT_IND 0x34 > #define AG7XXX_ETH_MII_MGMT_IND_INVALID BIT(2) > #define AG7XXX_ETH_MII_MGMT_IND_BUSY BIT(0) > > +/* STA Address 1 & 2 */ > #define AG7XXX_ETH_ADDR1 0x40 > #define AG7XXX_ETH_ADDR2 0x44 > > +/* ETH Configuration 0 - 5 */ > #define AG7XXX_ETH_FIFO_CFG_0 0x48 > #define AG7XXX_ETH_FIFO_CFG_1 0x4c > #define AG7XXX_ETH_FIFO_CFG_2 0x50 > @@ -70,20 +80,32 @@ enum ag7xxx_model { > #define AG7XXX_ETH_FIFO_CFG_4 0x58 > #define AG7XXX_ETH_FIFO_CFG_5 0x5c > > +/* DMA Transfer Control for Queue 0 */ > #define AG7XXX_ETH_DMA_TX_CTRL 0x180 > #define AG7XXX_ETH_DMA_TX_CTRL_TXE BIT(0) > > +/* Descriptor Address for Queue 0 Tx */ > #define AG7XXX_ETH_DMA_TX_DESC 0x184 > > +/* DMA Tx Status */ > #define AG7XXX_ETH_DMA_TX_STATUS 0x188 > > +/* Rx Control */ > #define AG7XXX_ETH_DMA_RX_CTRL 0x18c > #define AG7XXX_ETH_DMA_RX_CTRL_RXE BIT(0) > > +/* Pointer to Rx Descriptor */ > #define AG7XXX_ETH_DMA_RX_DESC 0x190 > > +/* Rx Status */ > #define AG7XXX_ETH_DMA_RX_STATUS 0x194 > > +/* PHY Control Registers */ > + > +/* PHY Specific Status Register */ > +#define AG7XXX_PHY_PSSR 0x11 > +#define AG7XXX_PHY_PSSR_LINK_UP BIT(10) > + > /* Custom register at 0x18070000 */ > #define AG7XXX_GMAC_ETH_CFG 0x00 > #define AG7XXX_ETH_CFG_SW_PHY_ADDR_SWAP BIT(8) > @@ -269,18 +291,33 @@ static int ag7xxx_switch_reg_write(struct mii_dev *bus, int reg, u32 val) > return 0; > } > > -static u16 ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) > +static int ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) > { > u32 data; > + unsigned long start; > + int ret; > + /* No idea if this is long enough or too long */ > + int timeout_ms = 1000; > > /* Dummy read followed by PHY read/write command. */ > - ag7xxx_switch_reg_read(bus, 0x98, &data); > + ret = ag7xxx_switch_reg_read(bus, 0x98, &data); > + if (ret < 0) > + return ret; > data = val | (reg << 16) | (addr << 21) | BIT(30) | BIT(31); > - ag7xxx_switch_reg_write(bus, 0x98, data); > + ret = ag7xxx_switch_reg_write(bus, 0x98, data); > + if (ret < 0) > + return ret; > + > + start = get_timer(0); > > /* Wait for operation to finish */ > do { > - ag7xxx_switch_reg_read(bus, 0x98, &data); > + ret = ag7xxx_switch_reg_read(bus, 0x98, &data); > + if (ret < 0) > + return ret; > + > + if (get_timer(start) > timeout_ms) > + return -ETIMEDOUT; > } while (data & BIT(31)); > > return data & 0xffff; > @@ -294,7 +331,11 @@ static int ag7xxx_mdio_read(struct mii_dev *bus, int addr, int devad, int reg) > static int ag7xxx_mdio_write(struct mii_dev *bus, int addr, int devad, int reg, > u16 val) > { > - ag7xxx_mdio_rw(bus, addr, reg, val); > + int ret; > + > + ret = ag7xxx_mdio_rw(bus, addr, reg, val); > + if (ret < 0) > + return ret; > return 0; > } > > @@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) > return ret; > > /* Read out link status */ > - ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR); > + ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); > if (ret < 0) > return ret; > > + if (!(ret & AG7XXX_PHY_PSSR_LINK_UP)) > + return -ENOLINK; Are you sure about this ? > return 0; > } > > @@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) > return ret; > } > > - for (i = 0; i < phymax; i++) { > - /* Read out link status */ > - ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR); > - if (ret < 0) > - return ret; > - } And this ? > return 0; > } > >
On Tue, Jun 13, 2017 at 4:24 AM, Marek Vasut <marex@denx.de> wrote: > On 06/12/2017 10:20 PM, Joe Hershberger wrote: >> Don't wait forever, Pass errors back, etc. >> >> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> >> >> --- >> This is a pass at improving the code quality. >> This has not been tested in any way. >> >> drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 50 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c >> index cf60d11..c8352d1 100644 >> --- a/drivers/net/ag7xxx.c >> +++ b/drivers/net/ag7xxx.c [...] SNIP >> @@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) >> return ret; >> >> /* Read out link status */ >> - ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR); >> + ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); >> if (ret < 0) >> return ret; >> >> + if (!(ret & AG7XXX_PHY_PSSR_LINK_UP)) >> + return -ENOLINK; > > Are you sure about this ? It seems reasonable to me, but I don't have the HW to test against as noted above. >> return 0; >> } >> >> @@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) >> return ret; >> } >> >> - for (i = 0; i < phymax; i++) { >> - /* Read out link status */ >> - ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR); >> - if (ret < 0) >> - return ret; >> - } > > And this ? This was based on your comment: "Actually, I think this is only for the switch ports, so we don't care about the link status." >> return 0; >> } >> >> > > > -- > Best regards, > Marek Vasut > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
On 06/13/2017 06:28 PM, Joe Hershberger wrote: > On Tue, Jun 13, 2017 at 4:24 AM, Marek Vasut <marex@denx.de> wrote: >> On 06/12/2017 10:20 PM, Joe Hershberger wrote: >>> Don't wait forever, Pass errors back, etc. >>> >>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> >>> >>> --- >>> This is a pass at improving the code quality. >>> This has not been tested in any way. >>> >>> drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- >>> 1 file changed, 50 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c >>> index cf60d11..c8352d1 100644 >>> --- a/drivers/net/ag7xxx.c >>> +++ b/drivers/net/ag7xxx.c > > [...] SNIP > >>> @@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) >>> return ret; >>> >>> /* Read out link status */ >>> - ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR); >>> + ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); >>> if (ret < 0) >>> return ret; >>> >>> + if (!(ret & AG7XXX_PHY_PSSR_LINK_UP)) >>> + return -ENOLINK; >> >> Are you sure about this ? > > It seems reasonable to me, but I don't have the HW to test against as > noted above. CCing Wills . I wouldn't be surprised if the hardware was somehow magically screwed up, so I'd prefer to stick with equivalent changes and maybe changes of the logic in a separate patch. >>> return 0; >>> } >>> >>> @@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) >>> return ret; >>> } >>> >>> - for (i = 0; i < phymax; i++) { >>> - /* Read out link status */ >>> - ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR); >>> - if (ret < 0) >>> - return ret; >>> - } >> >> And this ? > > This was based on your comment: "Actually, I think this is only for > the switch ports, so we don't care about the link status." Just so I understand it correctly, the code reads link status and does nothing with it ?
On Mon, Jun 19, 2017 at 4:37 AM, Marek Vasut <marex@denx.de> wrote: > On 06/13/2017 06:28 PM, Joe Hershberger wrote: >> On Tue, Jun 13, 2017 at 4:24 AM, Marek Vasut <marex@denx.de> wrote: >>> On 06/12/2017 10:20 PM, Joe Hershberger wrote: >>>> Don't wait forever, Pass errors back, etc. >>>> >>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> >>>> >>>> --- >>>> This is a pass at improving the code quality. >>>> This has not been tested in any way. >>>> >>>> drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- >>>> 1 file changed, 50 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c >>>> index cf60d11..c8352d1 100644 >>>> --- a/drivers/net/ag7xxx.c >>>> +++ b/drivers/net/ag7xxx.c >> >> [...] SNIP >> >>>> @@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) >>>> return ret; >>>> >>>> /* Read out link status */ >>>> - ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR); >>>> + ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); >>>> if (ret < 0) >>>> return ret; >>>> >>>> + if (!(ret & AG7XXX_PHY_PSSR_LINK_UP)) >>>> + return -ENOLINK; >>> >>> Are you sure about this ? >> >> It seems reasonable to me, but I don't have the HW to test against as >> noted above. > > CCing Wills . I wouldn't be surprised if the hardware was somehow > magically screwed up, so I'd prefer to stick with equivalent changes and > maybe changes of the logic in a separate patch. OK. >>>> return 0; >>>> } >>>> >>>> @@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) >>>> return ret; >>>> } >>>> >>>> - for (i = 0; i < phymax; i++) { >>>> - /* Read out link status */ >>>> - ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR); >>>> - if (ret < 0) >>>> - return ret; >>>> - } >>> >>> And this ? >> >> This was based on your comment: "Actually, I think this is only for >> the switch ports, so we don't care about the link status." > > Just so I understand it correctly, the code reads link status and does > nothing with it ? That's how I read it. > > -- > Best regards, > Marek Vasut
On 06/19/2017 05:55 PM, Joe Hershberger wrote: > On Mon, Jun 19, 2017 at 4:37 AM, Marek Vasut <marex@denx.de> wrote: >> On 06/13/2017 06:28 PM, Joe Hershberger wrote: >>> On Tue, Jun 13, 2017 at 4:24 AM, Marek Vasut <marex@denx.de> wrote: >>>> On 06/12/2017 10:20 PM, Joe Hershberger wrote: >>>>> Don't wait forever, Pass errors back, etc. >>>>> >>>>> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> >>>>> >>>>> --- >>>>> This is a pass at improving the code quality. >>>>> This has not been tested in any way. >>>>> >>>>> drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- >>>>> 1 file changed, 50 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c >>>>> index cf60d11..c8352d1 100644 >>>>> --- a/drivers/net/ag7xxx.c >>>>> +++ b/drivers/net/ag7xxx.c >>> >>> [...] SNIP >>> >>>>> @@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) >>>>> return ret; >>>>> >>>>> /* Read out link status */ >>>>> - ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR); >>>>> + ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); >>>>> if (ret < 0) >>>>> return ret; >>>>> >>>>> + if (!(ret & AG7XXX_PHY_PSSR_LINK_UP)) >>>>> + return -ENOLINK; >>>> >>>> Are you sure about this ? >>> >>> It seems reasonable to me, but I don't have the HW to test against as >>> noted above. >> >> CCing Wills . I wouldn't be surprised if the hardware was somehow >> magically screwed up, so I'd prefer to stick with equivalent changes and >> maybe changes of the logic in a separate patch. > > OK. > >>>>> return 0; >>>>> } >>>>> >>>>> @@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) >>>>> return ret; >>>>> } >>>>> >>>>> - for (i = 0; i < phymax; i++) { >>>>> - /* Read out link status */ >>>>> - ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR); >>>>> - if (ret < 0) >>>>> - return ret; >>>>> - } >>>> >>>> And this ? >>> >>> This was based on your comment: "Actually, I think this is only for >>> the switch ports, so we don't care about the link status." >> >> Just so I understand it correctly, the code reads link status and does >> nothing with it ? > > That's how I read it. Sigh, well ... if you could split the patch in two, that'd be nice. I will try to test it as soon as I have a chance. If it's broken, I'll try to bisect it then.
diff --git a/drivers/net/ag7xxx.c b/drivers/net/ag7xxx.c index cf60d11..c8352d1 100644 --- a/drivers/net/ag7xxx.c +++ b/drivers/net/ag7xxx.c @@ -26,6 +26,7 @@ enum ag7xxx_model { AG7XXX_MODEL_AG934X, }; +/* MAC Configuration 1 */ #define AG7XXX_ETH_CFG1 0x00 #define AG7XXX_ETH_CFG1_SOFT_RST BIT(31) #define AG7XXX_ETH_CFG1_RX_RST BIT(19) @@ -34,6 +35,7 @@ enum ag7xxx_model { #define AG7XXX_ETH_CFG1_RX_EN BIT(2) #define AG7XXX_ETH_CFG1_TX_EN BIT(0) +/* MAC Configuration 2 */ #define AG7XXX_ETH_CFG2 0x04 #define AG7XXX_ETH_CFG2_IF_1000 BIT(9) #define AG7XXX_ETH_CFG2_IF_10_100 BIT(8) @@ -43,26 +45,34 @@ enum ag7xxx_model { #define AG7XXX_ETH_CFG2_PAD_CRC_EN BIT(2) #define AG7XXX_ETH_CFG2_FDX BIT(0) +/* MII Configuration */ #define AG7XXX_ETH_MII_MGMT_CFG 0x20 #define AG7XXX_ETH_MII_MGMT_CFG_RESET BIT(31) +/* MII Command */ #define AG7XXX_ETH_MII_MGMT_CMD 0x24 #define AG7XXX_ETH_MII_MGMT_CMD_READ 0x1 +/* MII Address */ #define AG7XXX_ETH_MII_MGMT_ADDRESS 0x28 #define AG7XXX_ETH_MII_MGMT_ADDRESS_SHIFT 8 +/* MII Control */ #define AG7XXX_ETH_MII_MGMT_CTRL 0x2c +/* MII Status */ #define AG7XXX_ETH_MII_MGMT_STATUS 0x30 +/* MII Indicators */ #define AG7XXX_ETH_MII_MGMT_IND 0x34 #define AG7XXX_ETH_MII_MGMT_IND_INVALID BIT(2) #define AG7XXX_ETH_MII_MGMT_IND_BUSY BIT(0) +/* STA Address 1 & 2 */ #define AG7XXX_ETH_ADDR1 0x40 #define AG7XXX_ETH_ADDR2 0x44 +/* ETH Configuration 0 - 5 */ #define AG7XXX_ETH_FIFO_CFG_0 0x48 #define AG7XXX_ETH_FIFO_CFG_1 0x4c #define AG7XXX_ETH_FIFO_CFG_2 0x50 @@ -70,20 +80,32 @@ enum ag7xxx_model { #define AG7XXX_ETH_FIFO_CFG_4 0x58 #define AG7XXX_ETH_FIFO_CFG_5 0x5c +/* DMA Transfer Control for Queue 0 */ #define AG7XXX_ETH_DMA_TX_CTRL 0x180 #define AG7XXX_ETH_DMA_TX_CTRL_TXE BIT(0) +/* Descriptor Address for Queue 0 Tx */ #define AG7XXX_ETH_DMA_TX_DESC 0x184 +/* DMA Tx Status */ #define AG7XXX_ETH_DMA_TX_STATUS 0x188 +/* Rx Control */ #define AG7XXX_ETH_DMA_RX_CTRL 0x18c #define AG7XXX_ETH_DMA_RX_CTRL_RXE BIT(0) +/* Pointer to Rx Descriptor */ #define AG7XXX_ETH_DMA_RX_DESC 0x190 +/* Rx Status */ #define AG7XXX_ETH_DMA_RX_STATUS 0x194 +/* PHY Control Registers */ + +/* PHY Specific Status Register */ +#define AG7XXX_PHY_PSSR 0x11 +#define AG7XXX_PHY_PSSR_LINK_UP BIT(10) + /* Custom register at 0x18070000 */ #define AG7XXX_GMAC_ETH_CFG 0x00 #define AG7XXX_ETH_CFG_SW_PHY_ADDR_SWAP BIT(8) @@ -269,18 +291,33 @@ static int ag7xxx_switch_reg_write(struct mii_dev *bus, int reg, u32 val) return 0; } -static u16 ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) +static int ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val) { u32 data; + unsigned long start; + int ret; + /* No idea if this is long enough or too long */ + int timeout_ms = 1000; /* Dummy read followed by PHY read/write command. */ - ag7xxx_switch_reg_read(bus, 0x98, &data); + ret = ag7xxx_switch_reg_read(bus, 0x98, &data); + if (ret < 0) + return ret; data = val | (reg << 16) | (addr << 21) | BIT(30) | BIT(31); - ag7xxx_switch_reg_write(bus, 0x98, data); + ret = ag7xxx_switch_reg_write(bus, 0x98, data); + if (ret < 0) + return ret; + + start = get_timer(0); /* Wait for operation to finish */ do { - ag7xxx_switch_reg_read(bus, 0x98, &data); + ret = ag7xxx_switch_reg_read(bus, 0x98, &data); + if (ret < 0) + return ret; + + if (get_timer(start) > timeout_ms) + return -ETIMEDOUT; } while (data & BIT(31)); return data & 0xffff; @@ -294,7 +331,11 @@ static int ag7xxx_mdio_read(struct mii_dev *bus, int addr, int devad, int reg) static int ag7xxx_mdio_write(struct mii_dev *bus, int addr, int devad, int reg, u16 val) { - ag7xxx_mdio_rw(bus, addr, reg, val); + int ret; + + ret = ag7xxx_mdio_rw(bus, addr, reg, val); + if (ret < 0) + return ret; return 0; } @@ -723,10 +764,13 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret; /* Read out link status */ - ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR); + ret = ag7xxx_mdio_read(priv->bus, phymax, 0, AG7XXX_PHY_PSSR); if (ret < 0) return ret; + if (!(ret & AG7XXX_PHY_PSSR_LINK_UP)) + return -ENOLINK; + return 0; } @@ -743,13 +787,6 @@ static int ag933x_phy_setup_common(struct udevice *dev) return ret; } - for (i = 0; i < phymax; i++) { - /* Read out link status */ - ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR); - if (ret < 0) - return ret; - } - return 0; }
Don't wait forever, Pass errors back, etc. Signed-off-by: Joe Hershberger <joe.hershberger@ni.com> --- This is a pass at improving the code quality. This has not been tested in any way. drivers/net/ag7xxx.c | 63 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 13 deletions(-)