| Message ID | 20241118104735.3741749-4-jacky_chou@aspeedtech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add Aspeed G7 MDIO support | expand |
Hi Jacky, On Mon, 2024-11-18 at 18:47 +0800, Jacky Chou wrote: > Add a dummy read to ensure triggering mdio controller before starting > polling the status of mdio controller. > > 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 4d5a115baf85..feae30bc3e78 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); Why do this when the same register is immediately read by readl_poll_timeout() below? If there is a reason, I'd like some more explanation in the comment you've added, discussing the details of the problem it's solving when taking into account the readl_poll_timeout() call. > > return readl_poll_timeout(ctx->base + ASPEED_MDIO_CTRL, ctrl, > !(ctrl & ASPEED_MDIO_CTRL_FIRE), Cheers, Andrew
On Tue, Nov 19, 2024 at 09:35:39AM +1030, Andrew Jeffery wrote: > Hi Jacky, > > On Mon, 2024-11-18 at 18:47 +0800, Jacky Chou wrote: > > Add a dummy read to ensure triggering mdio controller before starting > > polling the status of mdio controller. > > > > 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 4d5a115baf85..feae30bc3e78 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); > > Why do this when the same register is immediately read by > readl_poll_timeout() below? > > If there is a reason, I'd like some more explanation in the comment > you've added, discussing the details of the problem it's solving when > taking into account the readl_poll_timeout() call. Also, is this a fix? Should it have a Fixes: tag? If so, it should not be part of this series, assuming the older devices have the same issue. Andrew
Hi Andrew Jeffery, Thank you for your reply. > > iowrite32(ctrl, ctx->base + ASPEED_MDIO_CTRL); > > + /* Add dummy read to ensure triggering mdio controller */ > > + (void)ioread32(ctx->base + ASPEED_MDIO_CTRL); > > Why do this when the same register is immediately read by > readl_poll_timeout() below? > > If there is a reason, I'd like some more explanation in the comment you've > added, discussing the details of the problem it's solving when taking into > account the readl_poll_timeout() call. > Agree. When the bus is sometimes busy, it may cause the driver is unable to write the register to the MDIO controller immediately. Therefore, add a dummy read to ensure the previous write command has arrived to the MDIO controller before polling MDIO controller status. I will add more details in next version of the commit. Thanks, Jacky
Hi Andrew Lunn, Thank you for your reply. > > > iowrite32(ctrl, ctx->base + ASPEED_MDIO_CTRL); > > > + /* Add dummy read to ensure triggering mdio controller */ > > > + (void)ioread32(ctx->base + ASPEED_MDIO_CTRL); > > > > Why do this when the same register is immediately read by > > readl_poll_timeout() below? > > > > If there is a reason, I'd like some more explanation in the comment > > you've added, discussing the details of the problem it's solving when > > taking into account the readl_poll_timeout() call. > > Also, is this a fix? Should it have a Fixes: tag? If so, it should not be part of this > series, assuming the older devices have the same issue. Agree. It may be a fix. The patch is also applied in older device. I will separate from this series and send it to net tree. Thanks, Jacky
diff --git a/drivers/net/mdio/mdio-aspeed.c b/drivers/net/mdio/mdio-aspeed.c index 4d5a115baf85..feae30bc3e78 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),
Add a dummy read to ensure triggering mdio controller before starting polling the status of mdio controller. Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com> --- drivers/net/mdio/mdio-aspeed.c | 2 ++ 1 file changed, 2 insertions(+)