[net-next 0/3] Add Aspeed G7 MDIO support

[net-next 0/3] Add Aspeed G7 MDIO support

[Jacky Chou]

The Aspeed 7th generation SoC features three MDIO controllers.
The design of AST2700 MDIO controller is the same as AST2600.

Jacky Chou (3):
  dt-bindings: net: add support for AST2700
  net: mdio: aspeed: Add support for AST2700
  net: mdio: aspeed: Add dummy read for fire control

 .../devicetree/bindings/net/aspeed,ast2600-mdio.yaml      | 4 +++-
 drivers/net/mdio/mdio-aspeed.c                            | 8 ++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

-- 
2.25.1

[net-next 1/3] dt-bindings: net: add support for AST2700

[Jacky Chou]

The AST2700 is the 7th generation SoC from Aspeed.
Add compatible support for AST2700 in yaml.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 .../devicetree/bindings/net/aspeed,ast2600-mdio.yaml          | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml b/Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml
index d6ef468495c5..6dadca099875 100644
--- a/Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml
+++ b/Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml
@@ -19,7 +19,9 @@ allOf:
 
 properties:
   compatible:
-    const: aspeed,ast2600-mdio
+    enum:
+      - aspeed,ast2600-mdio
+      - aspeed,ast2700-mdio
 
   reg:
     maxItems: 1
-- 
2.25.1

[net-next 2/3] net: mdio: aspeed: Add support for AST2700

[Jacky Chou]

The Aspeed 7th generation SoC features three Aspeed MDIO.
The design of AST2700 MDIO controller is the same as AST2600.
Therefore, just add AST2700 compatible here.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/mdio/mdio-aspeed.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/mdio/mdio-aspeed.c b/drivers/net/mdio/mdio-aspeed.c
index e55be6dc9ae7..4d5a115baf85 100644
--- a/drivers/net/mdio/mdio-aspeed.c
+++ b/drivers/net/mdio/mdio-aspeed.c
@@ -188,6 +188,7 @@ static void aspeed_mdio_remove(struct platform_device *pdev)
 
 static const struct of_device_id aspeed_mdio_of_match[] = {
 	{ .compatible = "aspeed,ast2600-mdio", },
+	{ .compatible = "aspeed,ast2700-mdio", },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, aspeed_mdio_of_match);
-- 
2.25.1

[net-next 3/3] net: mdio: aspeed: Add dummy read for fire control

[Jacky Chou]

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);
 
 	return readl_poll_timeout(ctx->base + ASPEED_MDIO_CTRL, ctrl,
 				!(ctrl & ASPEED_MDIO_CTRL_FIRE),
-- 
2.25.1

Re: [net-next 3/3] net: mdio: aspeed: Add dummy read for fire control

[Andrew Jeffery]

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

Re: [net-next 0/3] Add Aspeed G7 MDIO support

[Andrew Lunn]

On Mon, Nov 18, 2024 at 06:47:32PM +0800, Jacky Chou wrote:
> The Aspeed 7th generation SoC features three MDIO controllers.
> The design of AST2700 MDIO controller is the same as AST2600.

If they are identical, why do you need a new compatible?

	Andrew

Re: [net-next 3/3] net: mdio: aspeed: Add dummy read for fire control

[Andrew Lunn]

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

回覆: [net-next 0/3] Add Aspeed G7 MDIO support

[Jacky Chou]

Hi Andrew Lunn

Thank you for your reply.

> > The Aspeed 7th generation SoC features three MDIO controllers.
> > The design of AST2700 MDIO controller is the same as AST2600.
> 
> If they are identical, why do you need a new compatible?

We want consistent naming in the DTS of the new SoC, even if Its 
design is the same as the older SoC.

Thanks,
Jacky

回覆: [net-next 3/3] net: mdio: aspeed: Add dummy read for fire control

[Jacky Chou]

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

回覆: [net-next 3/3] net: mdio: aspeed: Add dummy read for fire control

[Jacky Chou]

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

Re: 回覆: [net-next 0/3] Add Aspeed G7 MDIO support

[Andrew Lunn]

On Tue, Nov 19, 2024 at 05:35:40AM +0000, Jacky Chou wrote:
> Hi Andrew Lunn
> 
> Thank you for your reply.
> 
> > > The Aspeed 7th generation SoC features three MDIO controllers.
> > > The design of AST2700 MDIO controller is the same as AST2600.
> > 
> > If they are identical, why do you need a new compatible?
> 
> We want consistent naming in the DTS of the new SoC, even if Its 
> design is the same as the older SoC.

You might find the DT Maintainers push back against this. What you
want is effectively a Marketing game, it has little to do with
technology, describing the hardware.

	Andrew

Re: [net-next 1/3] dt-bindings: net: add support for AST2700

[Rob Herring]

On Mon, Nov 18, 2024 at 06:47:33PM +0800, Jacky Chou wrote:
> The AST2700 is the 7th generation SoC from Aspeed.
> Add compatible support for AST2700 in yaml.

"Add compatible..." is obvious from the diff. Add something about why 
the diff is how it is. For example, it is not compatible with AST2600 
because...

> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  .../devicetree/bindings/net/aspeed,ast2600-mdio.yaml          | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml b/Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml
> index d6ef468495c5..6dadca099875 100644
> --- a/Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/aspeed,ast2600-mdio.yaml
> @@ -19,7 +19,9 @@ allOf:
>  
>  properties:
>    compatible:
> -    const: aspeed,ast2600-mdio
> +    enum:
> +      - aspeed,ast2600-mdio
> +      - aspeed,ast2700-mdio
>  
>    reg:
>      maxItems: 1
> -- 
> 2.25.1
> 

Re: [net-next 2/3] net: mdio: aspeed: Add support for AST2700

[Rob Herring]

On Mon, Nov 18, 2024 at 06:47:34PM +0800, Jacky Chou wrote:
> The Aspeed 7th generation SoC features three Aspeed MDIO.
> The design of AST2700 MDIO controller is the same as AST2600.
> Therefore, just add AST2700 compatible here.

If they are "the same", you don't need to add the compatible here. You 
still need to add it to the binding, but make ast2600 a fallback 
compatible.

> 
> Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
> ---
>  drivers/net/mdio/mdio-aspeed.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/mdio/mdio-aspeed.c b/drivers/net/mdio/mdio-aspeed.c
> index e55be6dc9ae7..4d5a115baf85 100644
> --- a/drivers/net/mdio/mdio-aspeed.c
> +++ b/drivers/net/mdio/mdio-aspeed.c
> @@ -188,6 +188,7 @@ static void aspeed_mdio_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id aspeed_mdio_of_match[] = {
>  	{ .compatible = "aspeed,ast2600-mdio", },
> +	{ .compatible = "aspeed,ast2700-mdio", },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, aspeed_mdio_of_match);
> -- 
> 2.25.1
> 

回覆: [net-next 1/3] dt-bindings: net: add support for AST2700

[Jacky Chou]

Hi Rob,

Thank you for your replay.

> > The AST2700 is the 7th generation SoC from Aspeed.
> > Add compatible support for AST2700 in yaml.
> 
> "Add compatible..." is obvious from the diff. Add something about why the diff
> is how it is. For example, it is not compatible with AST2600 because...
> 

I understand what you mean.
We will continue to use compatible ast2600 in 7th generation.

Thanks,
Jacky