[PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Jacky Chou]

Hi Andrew,

I am ASPEED team.

>> system1 has 2 transceiver connected through the RGMII interfaces. Added
>> device tree entry to enable RGMII support.
>> 
>> ASPEED AST2600 documentation recommends using 'rgmii-rxid' as a
>> 'phy-mode' for mac0 and mac1 to enable the RX interface delay from the
>> PHY chip.
>
>Why?
>
>Does the mac0 TX clock have an extra long clock line on the PCB?
>
>Does the mac1 TX and RX clocks have extra long clock lines on the PCB?
>
>Anything but rgmii-id is in most cases wrong, so you need a really
>good explanation why you need to use something else. Something that
>shows you understand what is going on, and why what you have is
>correct.

Here I'll add some explanation.

In our design, we hope the TX and RX RGMII delay are configured by our MAC side.
We can control the TX/RX RGMII delay on MAC step by step, it is not a setting to delay to 2 ns.
We are not sure the all target PHYs are support for RX internal delay.

But ast2600 MAC1/2 delay cell cannot cover range to 2 ns, MAC 3/4 can do that.
Therefore, when using ast2600 MAC1/2, please enable the RX internal delay on the PHY side 
to make up for the part we cannot cover.

Summarize our design and we recommend.
AST2600 MAC1/2: rgmii-rxid
(RGMII with internal RX delay provided by the PHY, the MAC should not add an RX delay in this 
case)
AST2600 MAC3/4: rgmii
(RX and TX delays are added by the MAC when required)

rgmii and rgmii-rxid are referred from ethernet-controller.yaml file.

Thanks,
Jacky

Re: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

> >Does the mac0 TX clock have an extra long clock line on the PCB?
> >
> >Does the mac1 TX and RX clocks have extra long clock lines on the PCB?
> >
> >Anything but rgmii-id is in most cases wrong, so you need a really
> >good explanation why you need to use something else. Something that
> >shows you understand what is going on, and why what you have is
> >correct.
> 
> Here I'll add some explanation.
> 
> In our design, we hope the TX and RX RGMII delay are configured by our MAC side.
> We can control the TX/RX RGMII delay on MAC step by step, it is not a setting to delay to 2 ns.
> We are not sure the all target PHYs are support for RX internal delay.
> 
> But ast2600 MAC1/2 delay cell cannot cover range to 2 ns, MAC 3/4 can do that.
> Therefore, when using ast2600 MAC1/2, please enable the RX internal delay on the PHY side 
> to make up for the part we cannot cover.
> 
> Summarize our design and we recommend.
> AST2600 MAC1/2: rgmii-rxid
> (RGMII with internal RX delay provided by the PHY, the MAC should not add an RX delay in this 
> case)
> AST2600 MAC3/4: rgmii
> (RX and TX delays are added by the MAC when required)
> 
> rgmii and rgmii-rxid are referred from ethernet-controller.yaml file.

O.K, so you have the meaning of phy-mode wrong. phy-mode effectively
described the PCB. Does the PCB implement the 2ns delay via extra long
clock lines or not. If the PCB has long clock lines, phy-mode is
'rgmii'. If the PCB does not have long clock lines, 'rgmii-id' is
used, meaning either the MAC or the PHY needs to add the delays.

The MAC driver is the one that reads the phy-mode from the DT, and
then it decides what to do. 95% of linux MAC drivers simply pass it
direct to the PHY. If the phy-mode is 'rgmii', the PHY does nothing,
because the PCB has added the delays. If it is rgmii-id, it adds
delays in both directions. This works, because nearly very RGMII PHY
on the market does support RGMII delays.

There is however a very small number of MAC drivers which do things
differently. Renesas produced an RDK with a PHY which could not do
delays in the PHY, and so were forced to do the delays in the
MAC. Please look at how the ravb driver works. If the PCB does not add
the delays, rmgii-id, the MAC driver adds the delays. It then masks
the phy-mode it passes to of_phy_connect() back to 'rgmii', so that
the PHY does not add any delays. If the PCB did add the delays,
'rgmii', the MAC driver does not add delays, and it passed rgmii to
the PHY driver, and it also does not add delays.

So, your MAC driver is broken. It is not correctly handling the
phy-mode. First question is, how many boards are there in mainline
which have broken phy-modes. If this is the first board, we can fix
the MAC driver. If there are already boards in mainline we have to be
much more careful when fixing this, so as not to regress boards which
are already merged.

Humm, interesting. Looking at ftgmac100.c, i don't see where you
configure the RGMII delays in the MAC?

	  Andrew

Re: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Ninad Palsule]

Hi Andrew,

Thanks for the reply.

On 1/8/25 11:52, Andrew Lunn wrote:
>>> Does the mac0 TX clock have an extra long clock line on the PCB?
>>>
>>> Does the mac1 TX and RX clocks have extra long clock lines on the PCB?
>>>
>>> Anything but rgmii-id is in most cases wrong, so you need a really
>>> good explanation why you need to use something else. Something that
>>> shows you understand what is going on, and why what you have is
>>> correct.
>> Here I'll add some explanation.
>>
>> In our design, we hope the TX and RX RGMII delay are configured by our MAC side.
>> We can control the TX/RX RGMII delay on MAC step by step, it is not a setting to delay to 2 ns.
>> We are not sure the all target PHYs are support for RX internal delay.
>>
>> But ast2600 MAC1/2 delay cell cannot cover range to 2 ns, MAC 3/4 can do that.
>> Therefore, when using ast2600 MAC1/2, please enable the RX internal delay on the PHY side
>> to make up for the part we cannot cover.
>>
>> Summarize our design and we recommend.
>> AST2600 MAC1/2: rgmii-rxid
>> (RGMII with internal RX delay provided by the PHY, the MAC should not add an RX delay in this
>> case)
>> AST2600 MAC3/4: rgmii
>> (RX and TX delays are added by the MAC when required)
>>
>> rgmii and rgmii-rxid are referred from ethernet-controller.yaml file.
> O.K, so you have the meaning of phy-mode wrong. phy-mode effectively
> described the PCB. Does the PCB implement the 2ns delay via extra long
> clock lines or not. If the PCB has long clock lines, phy-mode is
> 'rgmii'. If the PCB does not have long clock lines, 'rgmii-id' is
> used, meaning either the MAC or the PHY needs to add the delays.

I checked with out hardware team and they did not add any extra delay on 
the board.

We have normal point to point clock without any delay added by line.

Regards,

Ninad

>
> The MAC driver is the one that reads the phy-mode from the DT, and
> then it decides what to do. 95% of linux MAC drivers simply pass it
> direct to the PHY. If the phy-mode is 'rgmii', the PHY does nothing,
> because the PCB has added the delays. If it is rgmii-id, it adds
> delays in both directions. This works, because nearly very RGMII PHY
> on the market does support RGMII delays.
>
> There is however a very small number of MAC drivers which do things
> differently. Renesas produced an RDK with a PHY which could not do
> delays in the PHY, and so were forced to do the delays in the
> MAC. Please look at how the ravb driver works. If the PCB does not add
> the delays, rmgii-id, the MAC driver adds the delays. It then masks
> the phy-mode it passes to of_phy_connect() back to 'rgmii', so that
> the PHY does not add any delays. If the PCB did add the delays,
> 'rgmii', the MAC driver does not add delays, and it passed rgmii to
> the PHY driver, and it also does not add delays.
>
> So, your MAC driver is broken. It is not correctly handling the
> phy-mode. First question is, how many boards are there in mainline
> which have broken phy-modes. If this is the first board, we can fix
> the MAC driver. If there are already boards in mainline we have to be
> much more careful when fixing this, so as not to regress boards which
> are already merged.
>
> Humm, interesting. Looking at ftgmac100.c, i don't see where you
> configure the RGMII delays in the MAC?
>
> 	  Andrew
>

Re: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

> I checked with out hardware team and they did not add any extra delay on the
> board.
> 
> We have normal point to point clock without any delay added by line.

Thanks for checking. Thus phy-mode must be "rgmii-id". We now need to
fix the MAC driver so it does the correct thing when passed that.

    Andrew

Re: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Ninad Palsule]

Hello Andrew & Jacky,

On 1/8/25 11:52, Andrew Lunn wrote:
>>> Does the mac0 TX clock have an extra long clock line on the PCB?
>>>
>>> Does the mac1 TX and RX clocks have extra long clock lines on the PCB?
>>>
>>> Anything but rgmii-id is in most cases wrong, so you need a really
>>> good explanation why you need to use something else. Something that
>>> shows you understand what is going on, and why what you have is
>>> correct.
>> Here I'll add some explanation.
>>
>> In our design, we hope the TX and RX RGMII delay are configured by our MAC side.
>> We can control the TX/RX RGMII delay on MAC step by step, it is not a setting to delay to 2 ns.
>> We are not sure the all target PHYs are support for RX internal delay.
>>
>> But ast2600 MAC1/2 delay cell cannot cover range to 2 ns, MAC 3/4 can do that.
>> Therefore, when using ast2600 MAC1/2, please enable the RX internal delay on the PHY side
>> to make up for the part we cannot cover.
>>
>> Summarize our design and we recommend.
>> AST2600 MAC1/2: rgmii-rxid
>> (RGMII with internal RX delay provided by the PHY, the MAC should not add an RX delay in this
>> case)
>> AST2600 MAC3/4: rgmii
>> (RX and TX delays are added by the MAC when required)
>>
>> rgmii and rgmii-rxid are referred from ethernet-controller.yaml file.
> O.K, so you have the meaning of phy-mode wrong. phy-mode effectively
> described the PCB. Does the PCB implement the 2ns delay via extra long
> clock lines or not. If the PCB has long clock lines, phy-mode is
> 'rgmii'. If the PCB does not have long clock lines, 'rgmii-id' is
> used, meaning either the MAC or the PHY needs to add the delays.
>
> The MAC driver is the one that reads the phy-mode from the DT, and
> then it decides what to do. 95% of linux MAC drivers simply pass it
> direct to the PHY. If the phy-mode is 'rgmii', the PHY does nothing,
> because the PCB has added the delays. If it is rgmii-id, it adds
> delays in both directions. This works, because nearly very RGMII PHY
> on the market does support RGMII delays.
>
> There is however a very small number of MAC drivers which do things
> differently. Renesas produced an RDK with a PHY which could not do
> delays in the PHY, and so were forced to do the delays in the
> MAC. Please look at how the ravb driver works. If the PCB does not add
> the delays, rmgii-id, the MAC driver adds the delays. It then masks
> the phy-mode it passes to of_phy_connect() back to 'rgmii', so that
> the PHY does not add any delays. If the PCB did add the delays,
> 'rgmii', the MAC driver does not add delays, and it passed rgmii to
> the PHY driver, and it also does not add delays.
>
> So, your MAC driver is broken. It is not correctly handling the
> phy-mode. First question is, how many boards are there in mainline
> which have broken phy-modes. If this is the first board, we can fix
> the MAC driver. If there are already boards in mainline we have to be
> much more careful when fixing this, so as not to regress boards which
> are already merged.

There are around 11 boards in Aspeed SOC with phy-mode set to "rgmii" 
(some of them are mac0&1 and others are mac2&3). "rgmii-rxid" is only mine.

No one in aspeed SOC using "rgmii-id".

>
> Humm, interesting. Looking at ftgmac100.c, i don't see where you
> configure the RGMII delays in the MAC?
>
> 	  Andrew
>

Re: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

> There are around 11 boards in Aspeed SOC with phy-mode set to "rgmii" (some
> of them are mac0&1 and others are mac2&3). "rgmii-rxid" is only mine.
> 
> No one in aspeed SOC using "rgmii-id".

O.K, so we have to be careful how we fix this. But the fact they are
all equally broken might help here.

> > Humm, interesting. Looking at ftgmac100.c, i don't see where you
> > configure the RGMII delays in the MAC?

This is going to be important. How are delays configured if they are
not in the MAC driver?

    Andrew

回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Jacky Chou]

Hi Andrew,

> > There are around 11 boards in Aspeed SOC with phy-mode set to "rgmii"
> > (some of them are mac0&1 and others are mac2&3). "rgmii-rxid" is only
> mine.
> >
> > No one in aspeed SOC using "rgmii-id".
> 
> O.K, so we have to be careful how we fix this. But the fact they are all equally
> broken might help here.
> 
> > > Humm, interesting. Looking at ftgmac100.c, i don't see where you
> > > configure the RGMII delays in the MAC?
> 
> This is going to be important. How are delays configured if they are not in the
> MAC driver?

The RGMII delay is adjusted on clk-ast2600 driver. Please refer to the following link.
https://github.com/AspeedTech-BMC/linux/blob/f52a0cf7c475dc576482db46759e2d854c1f36e4/drivers/clk/clk-ast2600.c#L1008

We recently plan to upstream to mainline this part about the RGMII delay configuration.
All MAC RGMII delay of ast2600 are configured on the SCU register.
As mentioned before, we would like to configure the RGMII TX/RX delay on BMC side.
Therefore, we use the "rgmii" and "rgmii-rxid" as the recommendation for our design.

Thanks,
Jacky

Re: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

On Thu, Jan 09, 2025 at 10:33:20AM +0000, Jacky Chou wrote:
> Hi Andrew,
> 
> > > There are around 11 boards in Aspeed SOC with phy-mode set to "rgmii"
> > > (some of them are mac0&1 and others are mac2&3). "rgmii-rxid" is only
> > mine.
> > >
> > > No one in aspeed SOC using "rgmii-id".
> > 
> > O.K, so we have to be careful how we fix this. But the fact they are all equally
> > broken might help here.
> > 
> > > > Humm, interesting. Looking at ftgmac100.c, i don't see where you
> > > > configure the RGMII delays in the MAC?
> > 
> > This is going to be important. How are delays configured if they are not in the
> > MAC driver?
> 
> The RGMII delay is adjusted on clk-ast2600 driver. Please refer to the following link.
> https://github.com/AspeedTech-BMC/linux/blob/f52a0cf7c475dc576482db46759e2d854c1f36e4/drivers/clk/clk-ast2600.c#L1008

O.K. So in your vendor tree, you have additional DT properties
mac1-clk-delay, mac2-clk-delay, mac3-clk-delay. Which is fine, you can
do whatever you want in your vendor tree, it is all open source.

But for mainline, this will not be accepted. We have standard
properties defined for configuring MAC delays in picoseconds:

        rx-internal-delay-ps:
          description:
            RGMII Receive Clock Delay defined in pico seconds. This is used for
            controllers that have configurable RX internal delays. If this
            property is present then the MAC applies the RX delay.
        tx-internal-delay-ps:
          description:
            RGMII Transmit Clock Delay defined in pico seconds. This is used for
            controllers that have configurable TX internal delays. If this
            property is present then the MAC applies the TX delay.


You need to use these, and in the MAC driver, not a clock driver. That
is also part of the issue. Your MAC driver looks correct, it just
silently passes phy-mode to the PHY just like every other MAC
driver. But you have some code hidden away in the clock controller
which adds the delays. If this was in the MAC driver, where it should
be, this broken behaviour would of been found earlier.

So, looking at mainline, i see where you create a gated clock. But
what i do not see is where you set the delays.

How does this work in mainline? Is there more hidden code somewhere
setting the ASPEED_MAC12_CLK_DLY register?

	Andrew

Re: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Ninad Palsule]

Hello Andrew,

On 1/9/25 07:21, Andrew Lunn wrote:
> On Thu, Jan 09, 2025 at 10:33:20AM +0000, Jacky Chou wrote:
>> Hi Andrew,
>>
>>>> There are around 11 boards in Aspeed SOC with phy-mode set to "rgmii"
>>>> (some of them are mac0&1 and others are mac2&3). "rgmii-rxid" is only
>>> mine.
>>>> No one in aspeed SOC using "rgmii-id".
>>> O.K, so we have to be careful how we fix this. But the fact they are all equally
>>> broken might help here.
>>>
>>>>> Humm, interesting. Looking at ftgmac100.c, i don't see where you
>>>>> configure the RGMII delays in the MAC?
>>> This is going to be important. How are delays configured if they are not in the
>>> MAC driver?
>> The RGMII delay is adjusted on clk-ast2600 driver. Please refer to the following link.
>> https://github.com/AspeedTech-BMC/linux/blob/f52a0cf7c475dc576482db46759e2d854c1f36e4/drivers/clk/clk-ast2600.c#L1008
> O.K. So in your vendor tree, you have additional DT properties
> mac1-clk-delay, mac2-clk-delay, mac3-clk-delay. Which is fine, you can
> do whatever you want in your vendor tree, it is all open source.
>
> But for mainline, this will not be accepted. We have standard
> properties defined for configuring MAC delays in picoseconds:
>
>          rx-internal-delay-ps:
>            description:
>              RGMII Receive Clock Delay defined in pico seconds. This is used for
>              controllers that have configurable RX internal delays. If this
>              property is present then the MAC applies the RX delay.
>          tx-internal-delay-ps:
>            description:
>              RGMII Transmit Clock Delay defined in pico seconds. This is used for
>              controllers that have configurable TX internal delays. If this
>              property is present then the MAC applies the TX delay.
>
>
> You need to use these, and in the MAC driver, not a clock driver. That
> is also part of the issue. Your MAC driver looks correct, it just
> silently passes phy-mode to the PHY just like every other MAC
> driver. But you have some code hidden away in the clock controller
> which adds the delays. If this was in the MAC driver, where it should
> be, this broken behaviour would of been found earlier.
>
> So, looking at mainline, i see where you create a gated clock. But
> what i do not see is where you set the delays.
>
> How does this work in mainline? Is there more hidden code somewhere
> setting the ASPEED_MAC12_CLK_DLY register?

I think the code already exist in the mainline: 
https://github.com/torvalds/linux/blob/master/drivers/clk/clk-ast2600.c#L595

It is configuring SCU register in the ast2600 SOC to introduce delays. 
The mac is part of the SOC.

Regards,

Ninad

>
> 	Andrew

Re: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Ninad Palsule]

Hi Andrew,

Thanks for the explanation.

On 1/8/25 11:52, Andrew Lunn wrote:
>>> Does the mac0 TX clock have an extra long clock line on the PCB?
>>>
>>> Does the mac1 TX and RX clocks have extra long clock lines on the PCB?
>>>
>>> Anything but rgmii-id is in most cases wrong, so you need a really
>>> good explanation why you need to use something else. Something that
>>> shows you understand what is going on, and why what you have is
>>> correct.
>> Here I'll add some explanation.
>>
>> In our design, we hope the TX and RX RGMII delay are configured by our MAC side.
>> We can control the TX/RX RGMII delay on MAC step by step, it is not a setting to delay to 2 ns.
>> We are not sure the all target PHYs are support for RX internal delay.
>>
>> But ast2600 MAC1/2 delay cell cannot cover range to 2 ns, MAC 3/4 can do that.
>> Therefore, when using ast2600 MAC1/2, please enable the RX internal delay on the PHY side
>> to make up for the part we cannot cover.
>>
>> Summarize our design and we recommend.
>> AST2600 MAC1/2: rgmii-rxid
>> (RGMII with internal RX delay provided by the PHY, the MAC should not add an RX delay in this
>> case)
>> AST2600 MAC3/4: rgmii
>> (RX and TX delays are added by the MAC when required)
>>
>> rgmii and rgmii-rxid are referred from ethernet-controller.yaml file.
> O.K, so you have the meaning of phy-mode wrong. phy-mode effectively
> described the PCB. Does the PCB implement the 2ns delay via extra long
> clock lines or not. If the PCB has long clock lines, phy-mode is
> 'rgmii'. If the PCB does not have long clock lines, 'rgmii-id' is
> used, meaning either the MAC or the PHY needs to add the delays.
>
> The MAC driver is the one that reads the phy-mode from the DT, and
> then it decides what to do. 95% of linux MAC drivers simply pass it
> direct to the PHY. If the phy-mode is 'rgmii', the PHY does nothing,
> because the PCB has added the delays. If it is rgmii-id, it adds
> delays in both directions. This works, because nearly very RGMII PHY
> on the market does support RGMII delays.
>
> There is however a very small number of MAC drivers which do things
> differently. Renesas produced an RDK with a PHY which could not do
> delays in the PHY, and so were forced to do the delays in the
> MAC. Please look at how the ravb driver works. If the PCB does not add
> the delays, rmgii-id, the MAC driver adds the delays. It then masks
> the phy-mode it passes to of_phy_connect() back to 'rgmii', so that
> the PHY does not add any delays. If the PCB did add the delays,
> 'rgmii', the MAC driver does not add delays, and it passed rgmii to
> the PHY driver, and it also does not add delays.

When does someone use rgmii-txid and rgmii-rxid?

Regards,

Ninad

>
> So, your MAC driver is broken. It is not correctly handling the
> phy-mode. First question is, how many boards are there in mainline
> which have broken phy-modes. If this is the first board, we can fix
> the MAC driver. If there are already boards in mainline we have to be
> much more careful when fixing this, so as not to regress boards which
> are already merged.
>
> Humm, interesting. Looking at ftgmac100.c, i don't see where you
> configure the RGMII delays in the MAC?
>
> 	  Andrew
>

Re: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

> When does someone use rgmii-txid and rgmii-rxid?

When there is an extra long RX clock line on the PCB, but not the TX
clock line, you would use rgmii-txid. If there is an extra long TX
clock line, but not RX clock, you would use rgmii-rxid. You do not see
this very often, but it does exist:

arch/arm/boot/dts/nxp/ls/ls1021a-tsn.dts

/* RGMII delays added via PCB traces */
&enet2 {
        phy-mode = "rgmii";
        status = "okay";

	Andrew

Re: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

On Thu, Jan 09, 2025 at 08:25:28AM -0600, Ninad Palsule wrote:
> Hello Andrew,
> 
> On 1/9/25 07:21, Andrew Lunn wrote:
> > On Thu, Jan 09, 2025 at 10:33:20AM +0000, Jacky Chou wrote:
> > > Hi Andrew,
> > > 
> > > > > There are around 11 boards in Aspeed SOC with phy-mode set to "rgmii"
> > > > > (some of them are mac0&1 and others are mac2&3). "rgmii-rxid" is only
> > > > mine.
> > > > > No one in aspeed SOC using "rgmii-id".
> > > > O.K, so we have to be careful how we fix this. But the fact they are all equally
> > > > broken might help here.
> > > > 
> > > > > > Humm, interesting. Looking at ftgmac100.c, i don't see where you
> > > > > > configure the RGMII delays in the MAC?
> > > > This is going to be important. How are delays configured if they are not in the
> > > > MAC driver?
> > > The RGMII delay is adjusted on clk-ast2600 driver. Please refer to the following link.
> > > https://github.com/AspeedTech-BMC/linux/blob/f52a0cf7c475dc576482db46759e2d854c1f36e4/drivers/clk/clk-ast2600.c#L1008
> > O.K. So in your vendor tree, you have additional DT properties
> > mac1-clk-delay, mac2-clk-delay, mac3-clk-delay. Which is fine, you can
> > do whatever you want in your vendor tree, it is all open source.
> > 
> > But for mainline, this will not be accepted. We have standard
> > properties defined for configuring MAC delays in picoseconds:
> > 
> >          rx-internal-delay-ps:
> >            description:
> >              RGMII Receive Clock Delay defined in pico seconds. This is used for
> >              controllers that have configurable RX internal delays. If this
> >              property is present then the MAC applies the RX delay.
> >          tx-internal-delay-ps:
> >            description:
> >              RGMII Transmit Clock Delay defined in pico seconds. This is used for
> >              controllers that have configurable TX internal delays. If this
> >              property is present then the MAC applies the TX delay.
> > 
> > 
> > You need to use these, and in the MAC driver, not a clock driver. That
> > is also part of the issue. Your MAC driver looks correct, it just
> > silently passes phy-mode to the PHY just like every other MAC
> > driver. But you have some code hidden away in the clock controller
> > which adds the delays. If this was in the MAC driver, where it should
> > be, this broken behaviour would of been found earlier.
> > 
> > So, looking at mainline, i see where you create a gated clock. But
> > what i do not see is where you set the delays.
> > 
> > How does this work in mainline? Is there more hidden code somewhere
> > setting the ASPEED_MAC12_CLK_DLY register?
> 
> I think the code already exist in the mainline:
> https://github.com/torvalds/linux/blob/master/drivers/clk/clk-ast2600.c#L595
> 
> It is configuring SCU register in the ast2600 SOC to introduce delays. The
> mac is part of the SOC.

I could be reading this wrong, but that appears to create a gated
clock.

hw = clk_hw_register_gate(dev, "mac1rclk", "mac12rclk", 0,
	       		scu_g6_base + ASPEED_MAC12_CLK_DLY, 29, 0,
			&aspeed_g6_clk_lock);

/**
 * clk_hw_register_gate - register a gate clock with the clock framework
 * @dev: device that is registering this clock
 * @name: name of this clock
 * @parent_name: name of this clock's parent
 * @flags: framework-specific flags for this clock
 * @reg: register address to control gating of this clock
 * @bit_idx: which bit in the register controls gating of this clock
 * @clk_gate_flags: gate-specific flags for this clock
 * @lock: shared register lock for this clock
 */

There is nothing here about writing a value into @reg at creation time
to give it a default value. If you look at the vendor code, it has
extra writes, but i don't see anything like that in mainline.

	Andrew

回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Jacky Chou]

Hi Andrew,

Thank you for your reply.

> >
> > I think the code already exist in the mainline:
> > https://github.com/torvalds/linux/blob/master/drivers/clk/clk-ast2600.
> > c#L595
> >
> > It is configuring SCU register in the ast2600 SOC to introduce delays.
> > The mac is part of the SOC.
> 
> I could be reading this wrong, but that appears to create a gated clock.
> 
> hw = clk_hw_register_gate(dev, "mac1rclk", "mac12rclk", 0,
> 	       		scu_g6_base + ASPEED_MAC12_CLK_DLY, 29, 0,
> 			&aspeed_g6_clk_lock);
> 
> /**
>  * clk_hw_register_gate - register a gate clock with the clock framework
>  * @dev: device that is registering this clock
>  * @name: name of this clock
>  * @parent_name: name of this clock's parent
>  * @flags: framework-specific flags for this clock
>  * @reg: register address to control gating of this clock
>  * @bit_idx: which bit in the register controls gating of this clock
>  * @clk_gate_flags: gate-specific flags for this clock
>  * @lock: shared register lock for this clock  */
> 
> There is nothing here about writing a value into @reg at creation time to give
> it a default value. If you look at the vendor code, it has extra writes, but i don't
> see anything like that in mainline.

Agree. You are right. This part is used to create a gated clock.
We will configure these RGMII delay in bootloader like U-boot.
Therefore, here does not configure delay again.

Currently, the delay of RGMII is configured in SCU region not in ftgma100 region.
And I studied ethernet-controller.yaml file, as you said, it has defined about rgmii 
delay property for MAC side to set.
My plan is that I will move this delay setting to ftgmac100 driver from SCU.
Add a SCU syscon property for ftgmac100 driver configures the RGMII delay.

// aspeed-g6.dtsi
mac0: ethernet@1e660000 {
			compatible = "aspeed,ast2600-mac", "faraday,ftgmac100";
			reg = <0x1e660000 0x180>;
			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
			clocks = <&syscon ASPEED_CLK_GATE_MAC1CLK>;
			aspeed,scu = <&syscon>;    ------> add
			status = "disabled";
		};

Because AST2600 MAC1/2 RGMII delay setting in scu region is combined to one 32-bit register, 
MAC3/4 is also. I will also use 'aliase' to get MAC index to set delay in scu.

// aspeed-g6.dtsi
aliases {
		..........
		mac0 = &mac0;
		mac1 = &mac1;
		mac2 = &mac2;
		mac4 = &mac3;
	};

Then, we can use rx-internal-delay-ps and tx-internal-delay-ps property to configure delay
In ftgmac100 driver.

If you have any questions, please let me know. Thank you.

Thanks,
Jacky

Re: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

> Agree. You are right. This part is used to create a gated clock.
> We will configure these RGMII delay in bootloader like U-boot.
> Therefore, here does not configure delay again.

> Because AST2600 MAC1/2 RGMII delay setting in scu region is combined to one 32-bit register, 
> MAC3/4 is also. I will also use 'aliase' to get MAC index to set delay in scu.
> 
> // aspeed-g6.dtsi
> aliases {
> 		..........
> 		mac0 = &mac0;
> 		mac1 = &mac1;
> 		mac2 = &mac2;
> 		mac4 = &mac3;
> 	};

I would avoid that, because they are under control of the DT
developer. You sometimes seen the order changed in the hope of
changing the interface names, rather than use a udev script, or
systemd naming scheme.

The physical address of each interface is well known and fixed? Are
they the same for all ASTxxxx devices? I would hard code them into the
driver to identify the instance.

But first we need to fix what is broken with the existing DT phy-modes
etc.

What is the reset default of these SCU registers? 0? So we can tell if
the bootloader has modified it and inserted a delay?

What i think you need to do is during probe of the MAC driver, compare
phy-mode and how the delays are configured in hardware. If the delays
in hardware are 0, assume phy-mode is correct and use it. If the
delays are not 0, compare them with phy-mode. If the delays and
phy-mode agree, use them. If they disagree, assume phy-mode is wrong,
issue a dev_warn() that the DT blob is out of date, and modify
phy-mode to match the delays in the hardware, including a good
explanation of what is going on in the commit message to help those
with out of tree DT files. And then patch all the in tree DT files to
use the correct phy-mode.

Please double check my logic, just to make sure it is correct. If i
have it correct, it should be backwards compatible. The one feature
you loose out on is when the bootloader sets the wrong delays and you
want phy-mode to actually override it.

When AST2700 comes along, you can skip all this, get it right from the
start and not need this workaround.

	Andrew

Re: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Ninad Palsule]

Hi Jacky,

On 1/10/25 03:15, Jacky Chou wrote:
> Hi Andrew,
>
> Thank you for your reply.
>
>>> I think the code already exist in the mainline:
>>> https://github.com/torvalds/linux/blob/master/drivers/clk/clk-ast2600.
>>> c#L595
>>>
>>> It is configuring SCU register in the ast2600 SOC to introduce delays.
>>> The mac is part of the SOC.
>> I could be reading this wrong, but that appears to create a gated clock.
>>
>> hw = clk_hw_register_gate(dev, "mac1rclk", "mac12rclk", 0,
>> 	       		scu_g6_base + ASPEED_MAC12_CLK_DLY, 29, 0,
>> 			&aspeed_g6_clk_lock);
>>
>> /**
>>   * clk_hw_register_gate - register a gate clock with the clock framework
>>   * @dev: device that is registering this clock
>>   * @name: name of this clock
>>   * @parent_name: name of this clock's parent
>>   * @flags: framework-specific flags for this clock
>>   * @reg: register address to control gating of this clock
>>   * @bit_idx: which bit in the register controls gating of this clock
>>   * @clk_gate_flags: gate-specific flags for this clock
>>   * @lock: shared register lock for this clock  */
>>
>> There is nothing here about writing a value into @reg at creation time to give
>> it a default value. If you look at the vendor code, it has extra writes, but i don't
>> see anything like that in mainline.
> Agree. You are right. This part is used to create a gated clock.
> We will configure these RGMII delay in bootloader like U-boot.
> Therefore, here does not configure delay again.
>
> Currently, the delay of RGMII is configured in SCU region not in ftgma100 region.
> And I studied ethernet-controller.yaml file, as you said, it has defined about rgmii
> delay property for MAC side to set.
> My plan is that I will move this delay setting to ftgmac100 driver from SCU.
> Add a SCU syscon property for ftgmac100 driver configures the RGMII delay.
>
> // aspeed-g6.dtsi
> mac0: ethernet@1e660000 {
> 			compatible = "aspeed,ast2600-mac", "faraday,ftgmac100";
> 			reg = <0x1e660000 0x180>;
> 			interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> 			clocks = <&syscon ASPEED_CLK_GATE_MAC1CLK>;
> 			aspeed,scu = <&syscon>;    ------> add
> 			status = "disabled";
> 		};
>
> Because AST2600 MAC1/2 RGMII delay setting in scu region is combined to one 32-bit register,
> MAC3/4 is also. I will also use 'aliase' to get MAC index to set delay in scu.
>
> // aspeed-g6.dtsi
> aliases {
> 		..........
> 		mac0 = &mac0;
> 		mac1 = &mac1;
> 		mac2 = &mac2;
> 		mac4 = &mac3;
> 	};
>
> Then, we can use rx-internal-delay-ps and tx-internal-delay-ps property to configure delay
> In ftgmac100 driver.

Thanks. When are you planning to push this change? I might need to hold 
on to mac changes until then.

Regards,

Ninad

>
> If you have any questions, please let me know. Thank you.
>
> Thanks,
> Jacky

Re: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Ninad Palsule]

Hello Andrew & Jacky,

On 1/10/25 08:04, Andrew Lunn wrote:
>> Agree. You are right. This part is used to create a gated clock.
>> We will configure these RGMII delay in bootloader like U-boot.
>> Therefore, here does not configure delay again.
>> Because AST2600 MAC1/2 RGMII delay setting in scu region is combined to one 32-bit register,
>> MAC3/4 is also. I will also use 'aliase' to get MAC index to set delay in scu.
>>
>> // aspeed-g6.dtsi
>> aliases {
>> 		..........
>> 		mac0 = &mac0;
>> 		mac1 = &mac1;
>> 		mac2 = &mac2;
>> 		mac4 = &mac3;
>> 	};
> I would avoid that, because they are under control of the DT
> developer. You sometimes seen the order changed in the hope of
> changing the interface names, rather than use a udev script, or
> systemd naming scheme.
>
> The physical address of each interface is well known and fixed? Are
> they the same for all ASTxxxx devices? I would hard code them into the
> driver to identify the instance.
>
> But first we need to fix what is broken with the existing DT phy-modes
> etc.
>
> What is the reset default of these SCU registers? 0? So we can tell if
> the bootloader has modified it and inserted a delay?

Jacky,

Here are the values on my system and those are expected that means 
u-boot is setting correct value?

# devmem 0x1E6E2340 32
0x9028A410
# devmem 0x1E6E2348 32
0x00410410
# devmem 0x1E6E234c 32
0x00410410

# devmem 0x1E6E2350 32
0x40104145
# devmem 0x1E6E2358 32
0x00104145
# devmem 0x1E6E235c 32
0x00104145

>
> What i think you need to do is during probe of the MAC driver, compare
> phy-mode and how the delays are configured in hardware. If the delays
> in hardware are 0, assume phy-mode is correct and use it. If the
> delays are not 0, compare them with phy-mode. If the delays and
> phy-mode agree, use them. If they disagree, assume phy-mode is wrong,
> issue a dev_warn() that the DT blob is out of date, and modify
> phy-mode to match the delays in the hardware, including a good
> explanation of what is going on in the commit message to help those
> with out of tree DT files. And then patch all the in tree DT files to
> use the correct phy-mode.

Andrew,

Do we need updates on this description. It doesn't talk about external 
PCB level delays?

https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L77-L90

This is what you explained:

MAC driver reads following phy-mode from device tree. 95% of mac driver 
directly
  pass it to PHY through phy_connect.

rgmii - PCB has long clock lines so delay is added by PCB
         On this mode PHY does nothing.
rgmii-id - PCB doesn't add delay. Either MAC or PHY needs to add the delay
            Add delays in both directions. Some PHY may not add delay in 
that
            case MAC needs to add the delay mask rgmii-id to rgmii.
rgmii-rxid - If there is an extra long TX clock line, but not RX clock,
              you would use rgmii-rxid
rgmii-txid - When there is an extra long RX clock line on the PCB, but not
              the TX clock line, you would use rgmii-txid

Regards,

Ninad

>
> Please double check my logic, just to make sure it is correct. If i
> have it correct, it should be backwards compatible. The one feature
> you loose out on is when the bootloader sets the wrong delays and you
> want phy-mode to actually override it.
>
> When AST2700 comes along, you can skip all this, get it right from the
> start and not need this workaround.
>
> 	Andrew
>

Re: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

> Do we need updates on this description. It doesn't talk about external PCB
> level delays?
> 
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L77-L90
> 
> This is what you explained:
> 
> MAC driver reads following phy-mode from device tree. 95% of mac driver
> directly
>  pass it to PHY through phy_connect.
> 
> rgmii - PCB has long clock lines so delay is added by PCB
>         On this mode PHY does nothing.
> rgmii-id - PCB doesn't add delay. Either MAC or PHY needs to add the delay
>            Add delays in both directions. Some PHY may not add delay in that
>            case MAC needs to add the delay mask rgmii-id to rgmii.
> rgmii-rxid - If there is an extra long TX clock line, but not RX clock,
>              you would use rgmii-rxid
> rgmii-txid - When there is an extra long RX clock line on the PCB, but not
>              the TX clock line, you would use rgmii-txid

The documentation is not great, that has been said a few times.  What
is described here is the view from the PHY, which is not ideal.

      # RX and TX delays are added by the MAC when required
      - rgmii

From the perspective of the PHY, this means it does not need to add
delays, because the MAC has added the delays, if required, e.g. the
PCB has not added the delays.

We have the problem that DT is supposed to describe the
hardware. Saying the PHY should add the delays, but if the MAC adds
the delays it needs to mask the value passed to the PHY does not
describe the hardware, it is Linix implementation details. The DT
Maintainers don't want that in the DT binding because other OSes might
decide to implement the details differently.

So your description becomes:

rgmii      - PCB has long clock lines so delays are added by the PCB
rgmii-id   - PCB doesn't add delay. Either MAC or PHY needs to add the delays
             in both directions.
rgmii-rxid - There is an extra long TX clock line on the PCB, but not the RX clock.
rgmii-txid - There is an extra long RX clock line on the PCB, but not the TX clock.

It is correct, but leaves so much unsaid developers will still get it
wrong. What we really want is that developers:

1) Read the mailing list. Patches for RGMII delays are posted at least
   once an month and i point out how they are wrong. If developers
   actually read the mails, they would not make the same mistake again
   and again.

2) Developers for some reason like to invent their own code, rather
   than taking the easy route of copy from a driver already in Linux.
   The majority of drivers in Linux get this right, so if you copy
   another driver, you should get it right for free as well.

	Andrew

回覆: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Jacky Chou]

Hi Andrew

Thank you for your reply.

> > Agree. You are right. This part is used to create a gated clock.
> > We will configure these RGMII delay in bootloader like U-boot.
> > Therefore, here does not configure delay again.
> 
> > Because AST2600 MAC1/2 RGMII delay setting in scu region is combined
> > to one 32-bit register,
> > MAC3/4 is also. I will also use 'aliase' to get MAC index to set delay in scu.
> >
> > // aspeed-g6.dtsi
> > aliases {
> > 		..........
> > 		mac0 = &mac0;
> > 		mac1 = &mac1;
> > 		mac2 = &mac2;
> > 		mac4 = &mac3;
> > 	};
> 
> I would avoid that, because they are under control of the DT developer. You
> sometimes seen the order changed in the hope of changing the interface
> names, rather than use a udev script, or systemd naming scheme.
> 
> The physical address of each interface is well known and fixed? Are they the
> same for all ASTxxxx devices? I would hard code them into the driver to
> identify the instance.

The physical address of each interface is all different in all aspeed device.
And they are fixed and known. I can use the address to distinguish the interface.

> 
> But first we need to fix what is broken with the existing DT phy-modes etc.
> 
> What is the reset default of these SCU registers? 0? So we can tell if the
> bootloader has modified it and inserted a delay?
> 
> What i think you need to do is during probe of the MAC driver, compare
> phy-mode and how the delays are configured in hardware. If the delays in
> hardware are 0, assume phy-mode is correct and use it. If the delays are not 0,
> compare them with phy-mode. If the delays and phy-mode agree, use them. If
> they disagree, assume phy-mode is wrong, issue a dev_warn() that the DT blob
> is out of date, and modify phy-mode to match the delays in the hardware,
> including a good explanation of what is going on in the commit message to
> help those with out of tree DT files. And then patch all the in tree DT files to
> use the correct phy-mode.

Agree. I think this method is good. The RGMII delay reset value in SCU is zero.
I can use the reset value to know if the RGMII delay is configured.
If the values are not match the phy-mode, print warning message and apply the 
RGMII delay property if there is in MAC node of dts.

> 
> Please double check my logic, just to make sure it is correct. If i have it correct,
> it should be backwards compatible. The one feature you loose out on is when
> the bootloader sets the wrong delays and you want phy-mode to actually
> override it.
> 
> When AST2700 comes along, you can skip all this, get it right from the start
> and not need this workaround.

Thanks for your friendly reminder.

Thanks,
Jacky

回覆: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Jacky Chou]

Hi Ninad,

> 
> Thanks. When are you planning to push this change? I might need to hold on to
> mac changes until then.

Yes. We have a plan to push the patch about RGMII delay configuration.
Currently, I try to align our SDK to kernel.

Thanks,
Jacky

Re: 回覆: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Ninad Palsule]

Hi Jacky,

On 1/13/25 00:22, Jacky Chou wrote:
> Hi Ninad,
>
>> Thanks. When are you planning to push this change? I might need to hold on to
>> mac changes until then.
> Yes. We have a plan to push the patch about RGMII delay configuration.
> Currently, I try to align our SDK to kernel.

Thanks. What will be the "phy-mode" value after you make these changes?

Will it be "rgmii-id" for MAC1..4?

If that is the case then I can test it with current configuration which 
may add extra delays in the RX from PHY side.

Regards,

Ninad

>
> Thanks,
> Jacky

Re: 回覆: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

On Mon, Jan 13, 2025 at 08:26:08AM -0600, Ninad Palsule wrote:
> Hi Jacky,
> 
> On 1/13/25 00:22, Jacky Chou wrote:
> > Hi Ninad,
> > 
> > > Thanks. When are you planning to push this change? I might need to hold on to
> > > mac changes until then.
> > Yes. We have a plan to push the patch about RGMII delay configuration.
> > Currently, I try to align our SDK to kernel.
> 
> Thanks. What will be the "phy-mode" value after you make these changes?
> 
> Will it be "rgmii-id" for MAC1..4?

It should be.

> If that is the case then I can test it with current configuration which may
> add extra delays in the RX from PHY side.

I would then expect traffic will not flow correctly, and your see CRC
errors, because of double delays. It is however a useful test, because
if it does somehow work, it probably means the PHY driver is also
broken with its handling of delays. I don't know what PHY driver you
are using, but those in mainline should be correct, it is something i
try to review carefully.

	Andrew

回覆: 回覆: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Jacky Chou]

Hi Andrew and Ninad,

> >
> > Thanks. What will be the "phy-mode" value after you make these changes?
> >
> > Will it be "rgmii-id" for MAC1..4?
> 
> It should be.

Perhaps we will keep using "rgmii", still add RGMII delay configure on MAC side.
The reason is we cannot be sure all PHYs have support for phy-mode property.
We will refer to the other MACs and PHYs driver about phy-mode and 
rx/tx-internal-delay-ps properties how they implement.

Currently, we will plan to implement RGMII delay in ftgmac100 driver based on
ethernet-controller.yaml.

At same time, we will think how to configure these phy-mode "rgmii-rxid", "rgmii-txid" 
and "rgmii-id in MAC driver.

> 
> > If that is the case then I can test it with current configuration
> > which may add extra delays in the RX from PHY side.
> 
> I would then expect traffic will not flow correctly, and your see CRC errors,
> because of double delays. It is however a useful test, because if it does
> somehow work, it probably means the PHY driver is also broken with its
> handling of delays. I don't know what PHY driver you are using, but those in
> mainline should be correct, it is something i try to review carefully.

We will submit a series patch of RGMII delay in ftgmac100 driver to mainline.

Thanks,
Jacky

Re: 回覆: 回覆: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

On Wed, Jan 15, 2025 at 02:57:04AM +0000, Jacky Chou wrote:
> Hi Andrew and Ninad,
> 
> > >
> > > Thanks. What will be the "phy-mode" value after you make these changes?
> > >
> > > Will it be "rgmii-id" for MAC1..4?
> > 
> > It should be.
> 
> Perhaps we will keep using "rgmii"

No. It is wrong.

> The reason is we cannot be sure all PHYs have support for phy-mode property.
> We will refer to the other MACs and PHYs driver about phy-mode and 
> rx/tx-internal-delay-ps properties how they implement.
> 
> Currently, we will plan to implement RGMII delay in ftgmac100 driver based on
> ethernet-controller.yaml.
> 
> At same time, we will think how to configure these phy-mode "rgmii-rxid", "rgmii-txid" 
> and "rgmii-id in MAC driver.

I already explain how this works once. Please read this thread
again.... The MAC can apply the delays, but it must mask the phy-mode
it passes to the PHY.

	Andrew

回覆: 回覆: 回覆: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Jacky Chou]

Hi Andrew

> >
> > Perhaps we will keep using "rgmii"
> 
> No. It is wrong.
> 
> > The reason is we cannot be sure all PHYs have support for phy-mode
> property.
> > We will refer to the other MACs and PHYs driver about phy-mode and
> > rx/tx-internal-delay-ps properties how they implement.
> >
> > Currently, we will plan to implement RGMII delay in ftgmac100 driver
> > based on ethernet-controller.yaml.
> >
> > At same time, we will think how to configure these phy-mode "rgmii-rxid",
> "rgmii-txid"
> > and "rgmii-id in MAC driver.
> 
> I already explain how this works once. Please read this thread again.... The
> MAC can apply the delays, but it must mask the phy-mode it passes to the PHY.

Yes. I have read these mails.

I understand what you mean.
"rgmii": delay on PCB, not MAC or PHY.
"rgmii-id": delay on MAC or PHY, not PCB.

ftgmac100 driver gets phy driver handle from of_phy_get_and_connect(), it will pass the phy-mode to
phy driver from the node of mac dts.
Therefore, I use "rgmii-id" and the phy will enable tx/rx internal delay.
If I use "rgmii-id" and configure the RGMII delay in ftgmac100 driver, I cannot pass the phy-mode to 
phy driver.
May I be correct?

Based on ethernel-controller.yaml, maybe need to adjust the description about phy-mode in this file first?

Thanks,
Jacky

Re: 回覆: 回覆: 回覆: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

> > I already explain how this works once. Please read this thread again.... The
> > MAC can apply the delays, but it must mask the phy-mode it passes to the PHY.
> 
> Yes. I have read these mails.
> 
> I understand what you mean.
> "rgmii": delay on PCB, not MAC or PHY.
> "rgmii-id": delay on MAC or PHY, not PCB.
> 
> ftgmac100 driver gets phy driver handle from of_phy_get_and_connect(), it will pass the phy-mode to
> phy driver from the node of mac dts.
> Therefore, I use "rgmii-id" and the phy will enable tx/rx internal delay.
> If I use "rgmii-id" and configure the RGMII delay in ftgmac100 driver, I cannot pass the phy-mode to 
> phy driver.

Quoting myself, yet again:

> > MAC can apply the delays, but it must mask the phy-mode it passes to the PHY.

If you decide the MAC does the RX clock delay, it needs to mask that
from the phy-mode, otherwise the PHY will also do it. If you decide
the MAC does the TX clock delay, its needs to mask that from the
phy-mode.

	Andrew

回覆: 回覆: 回覆: 回覆: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Jacky Chou]

Hi Andrew

Thank you for your reply.

> >
> > Yes. I have read these mails.
> >
> > I understand what you mean.
> > "rgmii": delay on PCB, not MAC or PHY.
> > "rgmii-id": delay on MAC or PHY, not PCB.
> >
> > ftgmac100 driver gets phy driver handle from of_phy_get_and_connect(),
> > it will pass the phy-mode to phy driver from the node of mac dts.
> > Therefore, I use "rgmii-id" and the phy will enable tx/rx internal delay.
> > If I use "rgmii-id" and configure the RGMII delay in ftgmac100 driver,
> > I cannot pass the phy-mode to phy driver.
> 
> Quoting myself, yet again:
> 
> > > MAC can apply the delays, but it must mask the phy-mode it passes to the
> PHY.
> 
> If you decide the MAC does the RX clock delay, it needs to mask that from the
> phy-mode, otherwise the PHY will also do it. If you decide the MAC does the TX
> clock delay, its needs to mask that from the phy-mode.

We need to confirm the MAC and RGMII design in AST2600,
And we will replan the phy-mode configuration in ftgmac100 driver and device tree.

Thanks,
Jacky

Re: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Maxime Chevallier]

Hello Andrew,

On Fri, 10 Jan 2025 16:38:18 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> > Do we need updates on this description. It doesn't talk about external PCB
> > level delays?
> > 
> > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L77-L90
> > 
> > This is what you explained:
> > 
> > MAC driver reads following phy-mode from device tree. 95% of mac driver
> > directly
> >  pass it to PHY through phy_connect.
> > 
> > rgmii - PCB has long clock lines so delay is added by PCB
> >         On this mode PHY does nothing.
> > rgmii-id - PCB doesn't add delay. Either MAC or PHY needs to add the delay
> >            Add delays in both directions. Some PHY may not add delay in that
> >            case MAC needs to add the delay mask rgmii-id to rgmii.
> > rgmii-rxid - If there is an extra long TX clock line, but not RX clock,
> >              you would use rgmii-rxid
> > rgmii-txid - When there is an extra long RX clock line on the PCB, but not
> >              the TX clock line, you would use rgmii-txid  
> 
> The documentation is not great, that has been said a few times.  What
> is described here is the view from the PHY, which is not ideal.
> 
>       # RX and TX delays are added by the MAC when required
>       - rgmii
> 
> From the perspective of the PHY, this means it does not need to add
> delays, because the MAC has added the delays, if required, e.g. the
> PCB has not added the delays.
> 
> We have the problem that DT is supposed to describe the
> hardware. Saying the PHY should add the delays, but if the MAC adds
> the delays it needs to mask the value passed to the PHY does not
> describe the hardware, it is Linix implementation details. The DT
> Maintainers don't want that in the DT binding because other OSes might
> decide to implement the details differently.
> 
> So your description becomes:
> 
> rgmii      - PCB has long clock lines so delays are added by the PCB
> rgmii-id   - PCB doesn't add delay. Either MAC or PHY needs to add the delays
>              in both directions.
> rgmii-rxid - There is an extra long TX clock line on the PCB, but not the RX clock.
> rgmii-txid - There is an extra long RX clock line on the PCB, but not the TX clock.
> 
> It is correct, but leaves so much unsaid developers will still get it
> wrong.

I myself got it wrong, as the kernel doc explicitely says that the "rgmii"
phy-mode is the one to use to get MAC-side delay insertion, whereas the way I
understand it, mac-side delay insertion doesn't really depend on the phy-mode
passed from DT. Ideally we would even consider that these mac-side delay
insertion would have to be ignored in basic 'RGMII' mode, but I think that would
break quite some existing setups ?

Can we consider an update in the kernel doc along these lines :

---
 Documentation/networking/phy.rst | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/Documentation/networking/phy.rst b/Documentation/networking/phy.rst
index f64641417c54..7ab77f9867a0 100644
--- a/Documentation/networking/phy.rst
+++ b/Documentation/networking/phy.rst
@@ -106,14 +106,17 @@ Whenever possible, use the PHY side RGMII delay for these reasons:
   configure correctly a specified delay enables more designs with similar delay
   requirements to be operated correctly
 
-For cases where the PHY is not capable of providing this delay, but the
-Ethernet MAC driver is capable of doing so, the correct phy_interface_t value
-should be PHY_INTERFACE_MODE_RGMII, and the Ethernet MAC driver should be
-configured correctly in order to provide the required transmit and/or receive
-side delay from the perspective of the PHY device. Conversely, if the Ethernet
-MAC driver looks at the phy_interface_t value, for any other mode but
-PHY_INTERFACE_MODE_RGMII, it should make sure that the MAC-level delays are
-disabled.
+The MAC driver may add delays if the PCB doesn't include any. This can be
+detected based on firmware "rx-internal-delay-ps" and "tx-internal-delay-ps"
+properties.
+
+When the MAC driver can insert the delays, it should always do so when these
+properties are present and non-zero, regardless of the RGMII mode specified.
+
+However, the MAC driver must adjust the PHY_INTERFACE_MODE_RGMII_* mode it passes
+to the connected PHY device (through phy_attach or phylink_create() for example)
+to account for MAC-side delay insertion, so that the the PHY device knows
+if any delays still needs insertion on either TX or RX paths.
 
 In case neither the Ethernet MAC, nor the PHY are capable of providing the
 required delays, as defined per the RGMII standard, several options may be
-- 

Thanks,

Maxime

Re: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Andrew Lunn]

> I myself got it wrong, as the kernel doc explicitely says that the "rgmii"
> phy-mode is the one to use to get MAC-side delay insertion, whereas the way I
> understand it, mac-side delay insertion doesn't really depend on the phy-mode
> passed from DT. Ideally we would even consider that these mac-side delay
> insertion would have to be ignored in basic 'RGMII' mode, but I think that would
> break quite some existing setups ?
> 
> Can we consider an update in the kernel doc along these lines :
> 
> ---
>  Documentation/networking/phy.rst | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/networking/phy.rst b/Documentation/networking/phy.rst
> index f64641417c54..7ab77f9867a0 100644
> --- a/Documentation/networking/phy.rst
> +++ b/Documentation/networking/phy.rst
> @@ -106,14 +106,17 @@ Whenever possible, use the PHY side RGMII delay for these reasons:
>    configure correctly a specified delay enables more designs with similar delay
>    requirements to be operated correctly
>  
> -For cases where the PHY is not capable of providing this delay, but the
> -Ethernet MAC driver is capable of doing so, the correct phy_interface_t value
> -should be PHY_INTERFACE_MODE_RGMII, and the Ethernet MAC driver should be
> -configured correctly in order to provide the required transmit and/or receive
> -side delay from the perspective of the PHY device. Conversely, if the Ethernet
> -MAC driver looks at the phy_interface_t value, for any other mode but
> -PHY_INTERFACE_MODE_RGMII, it should make sure that the MAC-level delays are
> -disabled.
> +The MAC driver may add delays if the PCB doesn't include any. This can be
> +detected based on firmware "rx-internal-delay-ps" and "tx-internal-delay-ps"
> +properties.
> +
> +When the MAC driver can insert the delays, it should always do so when these
> +properties are present and non-zero, regardless of the RGMII mode specified.
> +
> +However, the MAC driver must adjust the PHY_INTERFACE_MODE_RGMII_* mode it passes
> +to the connected PHY device (through phy_attach or phylink_create() for example)
> +to account for MAC-side delay insertion, so that the the PHY device knows
> +if any delays still needs insertion on either TX or RX paths.

You dropped:

   For cases where the PHY is not capable of providing this delay...

This is something i would like to keep, to strengthen that we really
do want the PHY to add the delays. Many MACs are capable of adding
delays, but we don't want them to, the PHY should do it, so we have
consistency.

The language i've tried to use is that "rx-internal-delay-ps" and
"tx-internal-delay-ps" can be used to fine tune the delays, so i'm
expecting their values to be small, because the PHY is adding the 2ns,
and the MAC is just adding/removing 0-200ps etc. I've also used the
same terminology for PHY drivers, the PHY DT properties for delays are
used for fine tuning, but the basic 2ns on/off comes from the phy-mode
passed to phylib.

If it is just fine tuning, and not adding the full 2ns, it should just
pass phy-mode straight through.

So your text becomes something like:

  The MAC driver may fine tune the delays. This can be configured
  based on firmware "rx-internal-delay-ps" and "tx-internal-delay-ps"
  properties. These values are expected to be small, not the full 2ns
  delay.

  A MAC driver inserting these fine tuning delays should always do so
  when these properties are present and non-zero, regardless of the
  RGMII mode specified.

Then we can address when the MAC adds the full 2ns.

  For cases where the PHY is not capable of providing the 2ns delay,
  the MAC must provide it, if the phy-mode indicates the PCB is not
  providing the delays. The MAC driver must adjust the
  PHY_INTERFACE_MODE_RGMII_* mode it passes to the connected PHY
  device (through phy_attach or phylink_create() for example) to
  account for MAC-side delay insertion, so that the the PHY device
  does not add additional delays.

I also think we need something near the beginning like:

  The device tree property phy-mode describes the hardware. When used
  with RGMII, its value indicates if the hardware, i.e. the PCB,
  provides the 2ns delay required for RGMII. A phy-mode of 'rgmii'
  indicates the PCB is adding the 2ns delay. For other values, the
  MAC/PHY pair must insert the needed 2ns delay, with the strong
  preference the PHY adds the delay.

	Andrew

Re: 回覆: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

[Maxime Chevallier]

> > Can we consider an update in the kernel doc along these lines :
> > 
> > ---
> >  Documentation/networking/phy.rst | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/networking/phy.rst b/Documentation/networking/phy.rst
> > index f64641417c54..7ab77f9867a0 100644
> > --- a/Documentation/networking/phy.rst
> > +++ b/Documentation/networking/phy.rst
> > @@ -106,14 +106,17 @@ Whenever possible, use the PHY side RGMII delay for these reasons:
> >    configure correctly a specified delay enables more designs with similar delay
> >    requirements to be operated correctly
> >  
> > -For cases where the PHY is not capable of providing this delay, but the
> > -Ethernet MAC driver is capable of doing so, the correct phy_interface_t value
> > -should be PHY_INTERFACE_MODE_RGMII, and the Ethernet MAC driver should be
> > -configured correctly in order to provide the required transmit and/or receive
> > -side delay from the perspective of the PHY device. Conversely, if the Ethernet
> > -MAC driver looks at the phy_interface_t value, for any other mode but
> > -PHY_INTERFACE_MODE_RGMII, it should make sure that the MAC-level delays are
> > -disabled.
> > +The MAC driver may add delays if the PCB doesn't include any. This can be
> > +detected based on firmware "rx-internal-delay-ps" and "tx-internal-delay-ps"
> > +properties.
> > +
> > +When the MAC driver can insert the delays, it should always do so when these
> > +properties are present and non-zero, regardless of the RGMII mode specified.
> > +
> > +However, the MAC driver must adjust the PHY_INTERFACE_MODE_RGMII_* mode it passes
> > +to the connected PHY device (through phy_attach or phylink_create() for example)
> > +to account for MAC-side delay insertion, so that the the PHY device knows
> > +if any delays still needs insertion on either TX or RX paths.  
> 
> You dropped:
> 
>    For cases where the PHY is not capable of providing this delay...
> 
> This is something i would like to keep, to strengthen that we really
> do want the PHY to add the delays. Many MACs are capable of adding
> delays, but we don't want them to, the PHY should do it, so we have
> consistency.
> 
> The language i've tried to use is that "rx-internal-delay-ps" and
> "tx-internal-delay-ps" can be used to fine tune the delays, so i'm
> expecting their values to be small, because the PHY is adding the 2ns,
> and the MAC is just adding/removing 0-200ps etc. I've also used the
> same terminology for PHY drivers, the PHY DT properties for delays are
> used for fine tuning, but the basic 2ns on/off comes from the phy-mode
> passed to phylib.
> 
> If it is just fine tuning, and not adding the full 2ns, it should just
> pass phy-mode straight through.
> 
> So your text becomes something like:
> 
>   The MAC driver may fine tune the delays. This can be configured
>   based on firmware "rx-internal-delay-ps" and "tx-internal-delay-ps"
>   properties. These values are expected to be small, not the full 2ns
>   delay.
> 
>   A MAC driver inserting these fine tuning delays should always do so
>   when these properties are present and non-zero, regardless of the
>   RGMII mode specified.
> 
> Then we can address when the MAC adds the full 2ns.
> 
>   For cases where the PHY is not capable of providing the 2ns delay,
>   the MAC must provide it, if the phy-mode indicates the PCB is not
>   providing the delays. The MAC driver must adjust the
>   PHY_INTERFACE_MODE_RGMII_* mode it passes to the connected PHY
>   device (through phy_attach or phylink_create() for example) to
>   account for MAC-side delay insertion, so that the the PHY device
>   does not add additional delays.
> 
> I also think we need something near the beginning like:
> 
>   The device tree property phy-mode describes the hardware. When used
>   with RGMII, its value indicates if the hardware, i.e. the PCB,
>   provides the 2ns delay required for RGMII. A phy-mode of 'rgmii'
>   indicates the PCB is adding the 2ns delay. For other values, the
>   MAC/PHY pair must insert the needed 2ns delay, with the strong
>   preference the PHY adds the delay.

Thanks Andrew for the suggestions, your wording is definitely better
than mine :) I'll queue that for when net-next re-opens.

Maxime