[PATCH] net: fsl_mdio: Fix busy flag polling register

[PATCH] net: fsl_mdio: Fix busy flag polling register

[Markus Koch]

NXP's mEMAC reference manual, Chapter 6.5.5 "MDIO Ethernet Management
Interface usage", specifies to poll the BSY (0) bit in the CFG (we call
it CTL) register to wait until a transaction has finished, not bit 31 in
the data register.

In the Linux kernel, this has already been fixed in commit 26eee0210ad7
("net/fsl: fix a bug in xgmac_mdio").

Signed-off-by: Markus Koch <markus@notsyncing.net>
---

I only stumbled over this section of code while looking at something else, but
I'm surprised this even works the way it is now. Maybe it's luck.

Sadly I have not yet had the chance to test this change on actual hardware, and
I'm not sure I will anytime soon, so I'm asking whether there's anyone who
could compile and run my code to see whether MDIO transactions work as expected.

Thanks!
Markus

 drivers/net/fm/memac_phy.c | 2 +-
 include/fsl_memac.h        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/fm/memac_phy.c b/drivers/net/fm/memac_phy.c
index 72b500a6d1..0af6e83a8f 100644
--- a/drivers/net/fm/memac_phy.c
+++ b/drivers/net/fm/memac_phy.c
@@ -64,7 +64,7 @@ static int memac_wait_until_done(struct memac_mdio_controller *regs)
 {
 	unsigned int timeout = MAX_NUM_RETRIES;
 
-	while ((memac_in_32(&regs->mdio_data) & MDIO_DATA_BSY) && timeout--)
+	while ((memac_in_32(&regs->mdio_ctl) & MDIO_CTL_BSY) && timeout--)
 		;
 
 	if (!timeout) {
diff --git a/include/fsl_memac.h b/include/fsl_memac.h
index d067f1511c..d973fc0a5e 100644
--- a/include/fsl_memac.h
+++ b/include/fsl_memac.h
@@ -246,6 +246,7 @@ struct memac_mdio_controller {
 #define MDIO_STAT_HOLD_15_CLK	(7 << 2)
 #define MDIO_STAT_NEG		(1 << 23)
 
+#define MDIO_CTL_BSY		(1 << 0)
 #define MDIO_CTL_DEV_ADDR(x)	(x & 0x1f)
 #define MDIO_CTL_PORT_ADDR(x)	((x & 0x1f) << 5)
 #define MDIO_CTL_PRE_DIS	(1 << 10)
@@ -254,7 +255,6 @@ struct memac_mdio_controller {
 #define MDIO_CTL_READ		(1 << 15)
 
 #define MDIO_DATA(x)		(x & 0xffff)
-#define MDIO_DATA_BSY		(1 << 31)
 
 struct fsl_enet_mac;
 
-- 
2.34.1

Re: [PATCH] net: fsl_mdio: Fix busy flag polling register

[Ramon Fried]

On Sun, Jan 2, 2022 at 7:36 PM Markus Koch <markus@notsyncing.net> wrote:
>
> NXP's mEMAC reference manual, Chapter 6.5.5 "MDIO Ethernet Management
> Interface usage", specifies to poll the BSY (0) bit in the CFG (we call
> it CTL) register to wait until a transaction has finished, not bit 31 in
> the data register.
>
> In the Linux kernel, this has already been fixed in commit 26eee0210ad7
> ("net/fsl: fix a bug in xgmac_mdio").
>
> Signed-off-by: Markus Koch <markus@notsyncing.net>
> ---
>
> I only stumbled over this section of code while looking at something else, but
> I'm surprised this even works the way it is now. Maybe it's luck.
>
> Sadly I have not yet had the chance to test this change on actual hardware, and
> I'm not sure I will anytime soon, so I'm asking whether there's anyone who
> could compile and run my code to see whether MDIO transactions work as expected.
>
> Thanks!
> Markus
>
>  drivers/net/fm/memac_phy.c | 2 +-
>  include/fsl_memac.h        | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/fm/memac_phy.c b/drivers/net/fm/memac_phy.c
> index 72b500a6d1..0af6e83a8f 100644
> --- a/drivers/net/fm/memac_phy.c
> +++ b/drivers/net/fm/memac_phy.c
> @@ -64,7 +64,7 @@ static int memac_wait_until_done(struct memac_mdio_controller *regs)
>  {
>         unsigned int timeout = MAX_NUM_RETRIES;
>
> -       while ((memac_in_32(&regs->mdio_data) & MDIO_DATA_BSY) && timeout--)
> +       while ((memac_in_32(&regs->mdio_ctl) & MDIO_CTL_BSY) && timeout--)
>                 ;
>
>         if (!timeout) {
> diff --git a/include/fsl_memac.h b/include/fsl_memac.h
> index d067f1511c..d973fc0a5e 100644
> --- a/include/fsl_memac.h
> +++ b/include/fsl_memac.h
> @@ -246,6 +246,7 @@ struct memac_mdio_controller {
>  #define MDIO_STAT_HOLD_15_CLK  (7 << 2)
>  #define MDIO_STAT_NEG          (1 << 23)
>
> +#define MDIO_CTL_BSY           (1 << 0)
>  #define MDIO_CTL_DEV_ADDR(x)   (x & 0x1f)
>  #define MDIO_CTL_PORT_ADDR(x)  ((x & 0x1f) << 5)
>  #define MDIO_CTL_PRE_DIS       (1 << 10)
> @@ -254,7 +255,6 @@ struct memac_mdio_controller {
>  #define MDIO_CTL_READ          (1 << 15)
>
>  #define MDIO_DATA(x)           (x & 0xffff)
> -#define MDIO_DATA_BSY          (1 << 31)
>
>  struct fsl_enet_mac;
>
> --
> 2.34.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH] net: fsl_mdio: Fix busy flag polling register

[Ioana Ciornei]

On Sun, Jan 02, 2022 at 06:34:18PM +0100, Markus Koch wrote:
> NXP's mEMAC reference manual, Chapter 6.5.5 "MDIO Ethernet Management
> Interface usage", specifies to poll the BSY (0) bit in the CFG (we call
> it CTL) register to wait until a transaction has finished, not bit 31 in
> the data register.

First of all, the CFG (Configuration) and CTL (Control) are two
different registers so the '(we call it CTL)' part of your statement is
false.

Indeed, the BSY bit is located in the MDIO_CFG register but that is not
accessed through the mdio_ctl field as you used it in your changes.
It's instead accessed through the mdio_stat field (the MDIO_CFG is
called in some RMs as the Configuration and Status register).

> 
> In the Linux kernel, this has already been fixed in commit 26eee0210ad7
> ("net/fsl: fix a bug in xgmac_mdio").
> 

Even the commit that you referenced is using the mdio_stat field.

--- a/drivers/net/ethernet/freescale/xgmac_mdio.c
+++ b/drivers/net/ethernet/freescale/xgmac_mdio.c
@@ -79,7 +79,7 @@ static int xgmac_wait_until_done(struct device *dev,
 
        /* Wait till the MDIO write is complete */
        timeout = TIMEOUT;
-       while ((ioread32be(&regs->mdio_data) & MDIO_DATA_BSY) && timeout) {
+       while ((ioread32be(&regs->mdio_stat) & MDIO_STAT_BSY) && timeout) {
                cpu_relax();
                timeout--;
        }

Please fix this up in a v2.

Thanks,
Ioana