[PATCH net v2] net: mdio: aspeed: Add dummy read for fire control

[PATCH net v2] net: mdio: aspeed: Add dummy read for fire control

[Jacky Chou]

When the command bus is sometimes busy, it may cause the command is not
arrived to MDIO controller immediately. On software, the driver issues a
write command to the command bus does not wait for command complete and
it returned back to code immediately. But a read command will wait for
the data back, once a read command was back indicates the previous write
command had arrived to controller.
Add a dummy read to ensure triggering mdio controller before starting
polling the status of mdio controller to avoid polling unexpected timeout.

Fixes: a9770eac511a ("net: mdio: Move MDIO drivers into a new subdirectory")
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/mdio/mdio-aspeed.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/mdio/mdio-aspeed.c b/drivers/net/mdio/mdio-aspeed.c
index c2170650415c..373902d33b96 100644
--- a/drivers/net/mdio/mdio-aspeed.c
+++ b/drivers/net/mdio/mdio-aspeed.c
@@ -62,6 +62,8 @@ static int aspeed_mdio_op(struct mii_bus *bus, u8 st, u8 op, u8 phyad, u8 regad,
 		| FIELD_PREP(ASPEED_MDIO_DATA_MIIRDATA, data);
 
 	iowrite32(ctrl, ctx->base + ASPEED_MDIO_CTRL);
+	/* Add dummy read to ensure triggering mdio controller */
+	(void)ioread32(ctx->base + ASPEED_MDIO_CTRL);
 
 	return readl_poll_timeout(ctx->base + ASPEED_MDIO_CTRL, ctrl,
 				!(ctrl & ASPEED_MDIO_CTRL_FIRE),
-- 
2.25.1

Re: [PATCH net v2] net: mdio: aspeed: Add dummy read for fire control

[Andrew Lunn]

On Tue, Nov 19, 2024 at 05:51:41PM +0800, Jacky Chou wrote:
> When the command bus is sometimes busy, it may cause the command is not
> arrived to MDIO controller immediately. On software, the driver issues a
> write command to the command bus does not wait for command complete and
> it returned back to code immediately. But a read command will wait for
> the data back, once a read command was back indicates the previous write
> command had arrived to controller.
> Add a dummy read to ensure triggering mdio controller before starting
> polling the status of mdio controller to avoid polling unexpected timeout.

Please have another attempt at writing the commit message.

> Fixes: a9770eac511a ("net: mdio: Move MDIO drivers into a new subdirectory")
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  drivers/net/mdio/mdio-aspeed.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/mdio/mdio-aspeed.c b/drivers/net/mdio/mdio-aspeed.c
> index c2170650415c..373902d33b96 100644
> --- a/drivers/net/mdio/mdio-aspeed.c
> +++ b/drivers/net/mdio/mdio-aspeed.c
> @@ -62,6 +62,8 @@ static int aspeed_mdio_op(struct mii_bus *bus, u8 st, u8 op, u8 phyad, u8 regad,
>  		| FIELD_PREP(ASPEED_MDIO_DATA_MIIRDATA, data);
>  
>  	iowrite32(ctrl, ctx->base + ASPEED_MDIO_CTRL);
> +	/* Add dummy read to ensure triggering mdio controller */
> +	(void)ioread32(ctx->base + ASPEED_MDIO_CTRL);

Maybe: /* Dummy read to flush previous write to controller */


    Andrew

---
pw-bot: cr

回覆: [PATCH net v2] net: mdio: aspeed: Add dummy read for fire control

[Jacky Chou]

Hi Andrew Lunn,

Thank you for your reply.

> > When the command bus is sometimes busy, it may cause the command is
> > not arrived to MDIO controller immediately. On software, the driver
> > issues a write command to the command bus does not wait for command
> > complete and it returned back to code immediately. But a read command
> > will wait for the data back, once a read command was back indicates
> > the previous write command had arrived to controller.
> > Add a dummy read to ensure triggering mdio controller before starting
> > polling the status of mdio controller to avoid polling unexpected timeout.
> 
> Please have another attempt at writing the commit message.
> 
> > Fixes: a9770eac511a ("net: mdio: Move MDIO drivers into a new
> > subdirectory")
> > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> > ---
> >  drivers/net/mdio/mdio-aspeed.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/mdio/mdio-aspeed.c
> > b/drivers/net/mdio/mdio-aspeed.c index c2170650415c..373902d33b96
> > 100644
> > --- a/drivers/net/mdio/mdio-aspeed.c
> > +++ b/drivers/net/mdio/mdio-aspeed.c
> > @@ -62,6 +62,8 @@ static int aspeed_mdio_op(struct mii_bus *bus, u8 st,
> u8 op, u8 phyad, u8 regad,
> >  		| FIELD_PREP(ASPEED_MDIO_DATA_MIIRDATA, data);
> >
> >  	iowrite32(ctrl, ctx->base + ASPEED_MDIO_CTRL);
> > +	/* Add dummy read to ensure triggering mdio controller */
> > +	(void)ioread32(ctx->base + ASPEED_MDIO_CTRL);
> 
> Maybe: /* Dummy read to flush previous write to controller */

Agree. I will change the title in next version.

Thanks,
Jacky

Re: [PATCH net v2] net: mdio: aspeed: Add dummy read for fire control

[Andrew Jeffery]

On Tue, 2024-11-19 at 17:51 +0800, Jacky Chou wrote:
> When the command bus is sometimes busy, it may cause the command is
> not
> arrived to MDIO controller immediately. On software, the driver
> issues a
> write command to the command bus does not wait for command complete
> and
> it returned back to code immediately. But a read command will wait
> for
> the data back, once a read command was back indicates the previous
> write
> command had arrived to controller.
> Add a dummy read to ensure triggering mdio controller before starting
> polling the status of mdio controller to avoid polling unexpected
> timeout.

Why use the explicit dummy read rather than adjust the poll interval or
duration? I still don't think that's been adequately explained given
the hardware-clear of the fire bit on completion, which is what we're
polling for.

Andrew

回覆: [PATCH net v2] net: mdio: aspeed: Add dummy read for fire control

[Jacky Chou]

Hi Andrew Jeffery,

Thank you for your reply.

> > When the command bus is sometimes busy, it may cause the command is
> > not arrived to MDIO controller immediately. On software, the driver
> > issues a write command to the command bus does not wait for command
> > complete and it returned back to code immediately. But a read command
> > will wait for the data back, once a read command was back indicates
> > the previous write command had arrived to controller.
> > Add a dummy read to ensure triggering mdio controller before starting
> > polling the status of mdio controller to avoid polling unexpected
> > timeout.
> 
> Why use the explicit dummy read rather than adjust the poll interval or
> duration? I still don't think that's been adequately explained given the
> hardware-clear of the fire bit on completion, which is what we're polling for.

We cannot know exactly what the timeout value of polling should be set to.
Because no one could know when the write command has arrived MDIO controller.

A dummy read can ensure the previous write command had arrived MDIO controller
Therefore, we chose to use a dummy read instead of increasing the timeout value.

Thanks,
Jacky

Re: [PATCH net v2] net: mdio: aspeed: Add dummy read for fire control

[Russell King (Oracle)]

On Tue, Nov 19, 2024 at 05:51:41PM +0800, Jacky Chou wrote:
> When the command bus is sometimes busy, it may cause the command is not
> arrived to MDIO controller immediately. On software, the driver issues a
> write command to the command bus does not wait for command complete and
> it returned back to code immediately. But a read command will wait for
> the data back, once a read command was back indicates the previous write
> command had arrived to controller.
> Add a dummy read to ensure triggering mdio controller before starting
> polling the status of mdio controller to avoid polling unexpected timeout.
> 
> Fixes: a9770eac511a ("net: mdio: Move MDIO drivers into a new subdirectory")
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  drivers/net/mdio/mdio-aspeed.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/mdio/mdio-aspeed.c b/drivers/net/mdio/mdio-aspeed.c
> index c2170650415c..373902d33b96 100644
> --- a/drivers/net/mdio/mdio-aspeed.c
> +++ b/drivers/net/mdio/mdio-aspeed.c
> @@ -62,6 +62,8 @@ static int aspeed_mdio_op(struct mii_bus *bus, u8 st, u8 op, u8 phyad, u8 regad,
>  		| FIELD_PREP(ASPEED_MDIO_DATA_MIIRDATA, data);
>  
>  	iowrite32(ctrl, ctx->base + ASPEED_MDIO_CTRL);
> +	/* Add dummy read to ensure triggering mdio controller */
> +	(void)ioread32(ctx->base + ASPEED_MDIO_CTRL);

I'd change that comment to:

	/* The above write could be posted, causing the timeout below to
	 * be inaccurate. Ensure the controller starts before we start the
	 * timeout.
	 */

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v2] net: mdio: aspeed: Add dummy read for fire control

[Andrew Lunn]

On Wed, Nov 20, 2024 at 03:13:11PM +1030, Andrew Jeffery wrote:
> On Tue, 2024-11-19 at 17:51 +0800, Jacky Chou wrote:
> > When the command bus is sometimes busy, it may cause the command is
> > not
> > arrived to MDIO controller immediately. On software, the driver
> > issues a
> > write command to the command bus does not wait for command complete
> > and
> > it returned back to code immediately. But a read command will wait
> > for
> > the data back, once a read command was back indicates the previous
> > write
> > command had arrived to controller.
> > Add a dummy read to ensure triggering mdio controller before starting
> > polling the status of mdio controller to avoid polling unexpected
> > timeout.
> 
> Why use the explicit dummy read rather than adjust the poll interval or
> duration? I still don't think that's been adequately explained given
> the hardware-clear of the fire bit on completion, which is what we're
> polling for.

I'm guessing here, but if the hardware has not received the write, the
read could return an indication that the hardware is idle, and so the
poll exits immediately. The returned value of the first read need to
be ignored. It is simpler and more reliable to do that with an
explicit read, rather than try to play with the poll timing.

AS i said, a guess. We need a good commit message explaining the
reality of what is happening here.

	Andrew

Re: [PATCH net v2] net: mdio: aspeed: Add dummy read for fire control

[Andrew Jeffery]

On Wed, 2024-11-20 at 14:44 +0100, Andrew Lunn wrote:
> On Wed, Nov 20, 2024 at 03:13:11PM +1030, Andrew Jeffery wrote:
> > On Tue, 2024-11-19 at 17:51 +0800, Jacky Chou wrote:
> > > When the command bus is sometimes busy, it may cause the command
> > > is
> > > not
> > > arrived to MDIO controller immediately. On software, the driver
> > > issues a
> > > write command to the command bus does not wait for command
> > > complete
> > > and
> > > it returned back to code immediately. But a read command will
> > > wait
> > > for
> > > the data back, once a read command was back indicates the
> > > previous
> > > write
> > > command had arrived to controller.
> > > Add a dummy read to ensure triggering mdio controller before
> > > starting
> > > polling the status of mdio controller to avoid polling unexpected
> > > timeout.
> > 
> > Why use the explicit dummy read rather than adjust the poll
> > interval or
> > duration? I still don't think that's been adequately explained
> > given
> > the hardware-clear of the fire bit on completion, which is what
> > we're
> > polling for.
> 
> I'm guessing here, but if the hardware has not received the write,
> the
> read could return an indication that the hardware is idle, and so the
> poll exits immediately. The returned value of the first read need to
> be ignored. It is simpler and more reliable to do that with an
> explicit read, rather than try to play with the poll timing.
> 
> AS i said, a guess. We need a good commit message explaining the
> reality of what is happening here.

I agree, the commit message needs to be more precise about the
interactions and effects.

Andrew