| Message ID | 20241119095141.1236414-1-jacky_chou@aspeedtech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: mdio: aspeed: Add dummy read for fire control | expand |
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
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
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
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
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. */
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
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
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),
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(+)