[PATCH] cpsw_mdio.c: Use correct reg in cpsw_mdio_get_alive
cpsw_mdio_get_alive reads the wrong register. See page 2316 in SPRUH73Q AM335x TRM
Signed-off-by: Ulf Samuelsson ulf@emagii.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Ramon Fried rfried.dev@gmail.com --- drivers/net/ti/cpsw_mdio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ti/cpsw_mdio.c b/drivers/net/ti/cpsw_mdio.c index a5ba73b739..ac791faa81 100644 --- a/drivers/net/ti/cpsw_mdio.c +++ b/drivers/net/ti/cpsw_mdio.c @@ -51,7 +51,7 @@ struct cpsw_mdio_regs { #define USERACCESS_PHY_REG_SHIFT (21) #define USERACCESS_PHY_ADDR_SHIFT (16) #define USERACCESS_DATA GENMASK(15, 0) - } user[0]; + } user[2]; };
#define CPSW_MDIO_DIV_DEF 0xff @@ -366,8 +366,8 @@ u32 cpsw_mdio_get_alive(struct mii_dev *bus) struct cpsw_mdio *mdio = bus->priv; u32 val;
- val = readl(&mdio->regs->control); - return val & GENMASK(15, 0); + val = readl(&mdio->regs->alive); + return val & GENMASK(7, 0); }
struct mii_dev *cpsw_mdio_init(const char *name, phys_addr_t mdio_base,
Hello Ulf,
On 07/02/23 13:55, Ulf Samuelsson wrote:
cpsw_mdio_get_alive reads the wrong register. See page 2316 in SPRUH73Q AM335x TRM
Signed-off-by: Ulf Samuelsson ulf@emagii.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Ramon Fried rfried.dev@gmail.com
drivers/net/ti/cpsw_mdio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ti/cpsw_mdio.c b/drivers/net/ti/cpsw_mdio.c index a5ba73b739..ac791faa81 100644 --- a/drivers/net/ti/cpsw_mdio.c +++ b/drivers/net/ti/cpsw_mdio.c @@ -51,7 +51,7 @@ struct cpsw_mdio_regs { #define USERACCESS_PHY_REG_SHIFT (21) #define USERACCESS_PHY_ADDR_SHIFT (16) #define USERACCESS_DATA GENMASK(15, 0)
- } user[0];
- } user[2];
};
#define CPSW_MDIO_DIV_DEF 0xff @@ -366,8 +366,8 @@ u32 cpsw_mdio_get_alive(struct mii_dev *bus) struct cpsw_mdio *mdio = bus->priv; u32 val;
- val = readl(&mdio->regs->control);
- return val & GENMASK(15, 0);
- val = readl(&mdio->regs->alive);
- return val & GENMASK(7, 0);
Referring to the TRM for AM335x at [0], on page 2401, the MDIOALIVE register reports the presence of Ethernet PHYs up to ADDR 31. Also, the cpsw driver: drivers/net/ti/cpsw.c which uses the cpsw_mdio_get_alive() function, expects the return value to be a 16 bit value. Therefore, I believe that the GENMASK should retain the previous value of "GENMASK(15, 0)", while only the register being read needs to be changed from "control" to "alive". Please let me know if I misunderstood something regarding your implementation.
[0] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf
Regards, Siddharth.
}
struct mii_dev *cpsw_mdio_init(const char *name, phys_addr_t mdio_base,
Den 2023-02-09 kl. 08:29, skrev Siddharth Vadapalli:
Hello Ulf,
On 07/02/23 13:55, Ulf Samuelsson wrote:
cpsw_mdio_get_alive reads the wrong register. See page 2316 in SPRUH73Q AM335x TRM
Signed-off-by: Ulf Samuelsson ulf@emagii.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Ramon Fried rfried.dev@gmail.com
drivers/net/ti/cpsw_mdio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ti/cpsw_mdio.c b/drivers/net/ti/cpsw_mdio.c index a5ba73b739..ac791faa81 100644 --- a/drivers/net/ti/cpsw_mdio.c +++ b/drivers/net/ti/cpsw_mdio.c @@ -51,7 +51,7 @@ struct cpsw_mdio_regs { #define USERACCESS_PHY_REG_SHIFT (21) #define USERACCESS_PHY_ADDR_SHIFT (16) #define USERACCESS_DATA GENMASK(15, 0)
- } user[0];
} user[2]; };
#define CPSW_MDIO_DIV_DEF 0xff
@@ -366,8 +366,8 @@ u32 cpsw_mdio_get_alive(struct mii_dev *bus) struct cpsw_mdio *mdio = bus->priv; u32 val;
- val = readl(&mdio->regs->control);
- return val & GENMASK(15, 0);
- val = readl(&mdio->regs->alive);
- return val & GENMASK(7, 0);
Referring to the TRM for AM335x at [0], on page 2401, the MDIOALIVE register reports the presence of Ethernet PHYs up to ADDR 31. Also, the cpsw driver: drivers/net/ti/cpsw.c which uses the cpsw_mdio_get_alive() function, expects the return value to be a 16 bit value. Therefore, I believe that the GENMASK should retain the previous value of "GENMASK(15, 0)", while only the register being read needs to be changed from "control" to "alive". Please let me know if I misunderstood something regarding your implementation.
We had an unusual H/W where the PHY (actually a switch) was connected to the second Ethernet port. All boards I have seen have the PHY connected to the first port or have both ports connected.
IIRC correctly (this patch is > 2 years old), we had bad data in the bits (15:8) and this is why only the 8 bits. Only bits [1:0] are used anyway.
[0] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf
Regards, Siddharth.
}
struct mii_dev *cpsw_mdio_init(const char *name, phys_addr_t mdio_base,
Best Regards Ulf Samuelsson
On 09/02/23 14:56, Ulf Samuelsson wrote:
Den 2023-02-09 kl. 08:29, skrev Siddharth Vadapalli:
Hello Ulf,
On 07/02/23 13:55, Ulf Samuelsson wrote:
cpsw_mdio_get_alive reads the wrong register. See page 2316 in SPRUH73Q AM335x TRM
Signed-off-by: Ulf Samuelsson ulf@emagii.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Ramon Fried rfried.dev@gmail.com
drivers/net/ti/cpsw_mdio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ti/cpsw_mdio.c b/drivers/net/ti/cpsw_mdio.c index a5ba73b739..ac791faa81 100644 --- a/drivers/net/ti/cpsw_mdio.c +++ b/drivers/net/ti/cpsw_mdio.c @@ -51,7 +51,7 @@ struct cpsw_mdio_regs { #define USERACCESS_PHY_REG_SHIFT (21) #define USERACCESS_PHY_ADDR_SHIFT (16) #define USERACCESS_DATA GENMASK(15, 0) - } user[0]; + } user[2]; }; #define CPSW_MDIO_DIV_DEF 0xff @@ -366,8 +366,8 @@ u32 cpsw_mdio_get_alive(struct mii_dev *bus) struct cpsw_mdio *mdio = bus->priv; u32 val; - val = readl(&mdio->regs->control); - return val & GENMASK(15, 0); + val = readl(&mdio->regs->alive); + return val & GENMASK(7, 0);
Referring to the TRM for AM335x at [0], on page 2401, the MDIOALIVE register reports the presence of Ethernet PHYs up to ADDR 31. Also, the cpsw driver: drivers/net/ti/cpsw.c which uses the cpsw_mdio_get_alive() function, expects the return value to be a 16 bit value. Therefore, I believe that the GENMASK should retain the previous value of "GENMASK(15, 0)", while only the register being read needs to be changed from "control" to "alive". Please let me know if I misunderstood something regarding your implementation.
We had an unusual H/W where the PHY (actually a switch) was connected to the second Ethernet port. All boards I have seen have the PHY connected to the first port or have both ports connected.
IIRC correctly (this patch is > 2 years old), we had bad data in the bits (15:8) and this is why only the 8 bits. Only bits [1:0] are used anyway.
Thank you for clarifying.
Reviewed-by: Siddharth Vadapalli s-vadapalli@ti.com
Regards, Siddharth.
On Tue, Feb 07, 2023 at 09:25:27AM +0100, Ulf Samuelsson wrote:
cpsw_mdio_get_alive reads the wrong register. See page 2316 in SPRUH73Q AM335x TRM
Signed-off-by: Ulf Samuelsson ulf@emagii.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Ramon Fried rfried.dev@gmail.com Reviewed-by: Siddharth Vadapalli s-vadapalli@ti.com
Applied to u-boot/master, thanks!
participants (4)
-
Siddharth Vadapalli -
Tom Rini -
Ulf Samuelsson -
Ulf Samuelsson