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

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

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 0/3] Add Aspeed G7 MDIO support

[Jacky Chou]

[-- Attachment #1: Type: text/plain, Size: 2669 bytes --]

Hi Andrew,

This is Jacky from ASPEED.
Last year, I submitted a series of patches to add a new compatible string "aspeed,ast2700-mdio". At that time, the feedback I received was that if there were no functional changes, a new compatible string would not be necessary.
Recently, we are submitting the AST2700 platform support to the Linux kernel. In the following discussion thread, it appears that the MDIO driver might need a new compatible string for the AST2700 platform:
https://lore.kernel.org/all/b048afc1-a143-4fd0-94c9-3677339d7f56@lunn.ch/
I would like to confirm whether this case should be submitted separately to net-next, and in general, if there are no hardware or design changes, is it still required to introduce a new compatible string?
Thank you for your guidance.

Thanks,
Jacky

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

________________________________
寄件者: Andrew Lunn <andrew@lunn.ch>
寄件日期: 2024年11月19日 上午 09:50
收件者: Jacky Chou <jacky_chou@aspeedtech.com>
副本: andrew+netdev@lunn.ch <andrew+netdev@lunn.ch>; davem@davemloft.net <davem@davemloft.net>; edumazet@google.com <edumazet@google.com>; kuba@kernel.org <kuba@kernel.org>; pabeni@redhat.com <pabeni@redhat.com>; robh@kernel.org <robh@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; conor+dt@kernel.org <conor+dt@kernel.org>; joel@jms.id.au <joel@jms.id.au>; andrew@codeconstruct.com.au <andrew@codeconstruct.com.au>; hkallweit1@gmail.com <hkallweit1@gmail.com>; linux@armlinux.org.uk <linux@armlinux.org.uk>; netdev@vger.kernel.org <netdev@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linux-aspeed@lists.ozlabs.org <linux-aspeed@lists.ozlabs.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
主旨: Re: [net-next 0/3] Add Aspeed G7 MDIO support

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

[-- Attachment #2: Type: text/html, Size: 5868 bytes --]

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

[Andrew Lunn]

On Mon, Oct 27, 2025 at 02:44:23AM +0000, Jacky Chou wrote:
> Hi Andrew,
> 
> This is Jacky from ASPEED.
> Last year, I submitted a series of patches to add a new compatible string
> "aspeed,ast2700-mdio". At that time, the feedback I received was that if there
> were no functional changes, a new compatible string would not be necessary.
> Recently, we are submitting the AST2700 platform support to the Linux kernel.
> In the following discussion thread, it appears that the MDIO driver might need
> a new compatible string for the AST2700 platform:
> https://lore.kernel.org/all/b048afc1-a143-4fd0-94c9-3677339d7f56@lunn.ch/
> I would like to confirm whether this case should be submitted separately to
> net-next, and in general, if there are no hardware or design changes, is it
> still required to introduce a new compatible string?

Are you sure it is identical? And are you sure there are no bugs in
the driver which would require breaking backwards compatibility, like
you are going to be doing for the MAC driver?

Take the reset handling for example. It looks like it was added after
basic support for the 2600 has added, so it had to be optional, to not
break backwards compatibility with older DT blobs. But since there is
no support for the 2700 yet, you could make the reset mandatory,
without breaking anything.

How is the clock handled on this hardware? The MDC is currently
ticking at 2.5MHz. However many PHYs and MDIO based Ethernet switches
will happy run at a faster speed. So you could implemented
'clock-frequency'. But for that, do you need a clock listed? Should
that clock be listed now, as a mandatory property for the 2700?

Having a specific compatible and a fallback costs nothing, so i would
do it. 

	Andrew