[07/12] dt-bindings: net: Add StarFive JH7100 SoC

  Add DT bindings documentation for the Synopsys DesignWare MAC found on
the StarFive JH7100 SoC.

Adjust 'reset' and 'reset-names' properties to allow using 'ahb' instead
of the 'stmmaceth' reset signal, as required by JH7100.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 .../devicetree/bindings/net/snps,dwmac.yaml   |  15 ++-
 .../bindings/net/starfive,jh7100-dwmac.yaml   | 106 ++++++++++++++++++
 MAINTAINERS                                   |   5 +
 3 files changed, 122 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/starfive,jh7100-dwmac.yaml
  

> +  starfive,gtxclk-dlychain:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: GTX clock delay chain setting

Please could you add more details to this. Is this controlling the
RGMII delays? 0ns or 2ns?

> +    gmac: ethernet@10020000 {
> +      compatible = "starfive,jh7100-dwmac", "snps,dwmac";
> +      reg = <0x0 0x10020000 0x0 0x10000>;
> +      clocks = <&clkgen JH7100_CLK_GMAC_ROOT_DIV>,
> +               <&clkgen JH7100_CLK_GMAC_AHB>,
> +               <&clkgen JH7100_CLK_GMAC_PTP_REF>,
> +               <&clkgen JH7100_CLK_GMAC_GTX>,
> +               <&clkgen JH7100_CLK_GMAC_TX_INV>;
> +      clock-names = "stmmaceth", "pclk", "ptp_ref", "gtxc", "tx";
> +      resets = <&rstgen JH7100_RSTN_GMAC_AHB>;
> +      reset-names = "ahb";
> +      interrupts = <6>, <7>;
> +      interrupt-names = "macirq", "eth_wake_irq";
> +      max-frame-size = <9000>;
> +      phy-mode = "rgmii-txid";

This is unusual. Does your board have a really long RX clock line to
insert the 2ns delay needed on the RX side?

       Andrew
  

On 11/02/2023 04:18, Cristian Ciocaltea wrote:
> Add DT bindings documentation for the Synopsys DesignWare MAC found on
> the StarFive JH7100 SoC.
> 
> Adjust 'reset' and 'reset-names' properties to allow using 'ahb' instead
> of the 'stmmaceth' reset signal, as required by JH7100.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  .../devicetree/bindings/net/snps,dwmac.yaml   |  15 ++-
>  .../bindings/net/starfive,jh7100-dwmac.yaml   | 106 ++++++++++++++++++


FYI, there is conflicting work:

https://lore.kernel.org/all/20230118061701.30047-5-yanhong.wang@starfivetech.com/

It's almost the same, thus this should be dropped.

Best regards,
Krzysztof
  

On 2/11/23 18:01, Andrew Lunn wrote:
>> +  starfive,gtxclk-dlychain:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: GTX clock delay chain setting
> 
> Please could you add more details to this. Is this controlling the
> RGMII delays? 0ns or 2ns?

This is what gets written to JH7100_SYSMAIN_REGISTER49 and it's 
currently set to 4 in patch 12/12. As already mentioned, I don't have 
the register information in the datasheet, but I'll update this as soon 
as we get some details.

>> +    gmac: ethernet@10020000 {
>> +      compatible = "starfive,jh7100-dwmac", "snps,dwmac";
>> +      reg = <0x0 0x10020000 0x0 0x10000>;
>> +      clocks = <&clkgen JH7100_CLK_GMAC_ROOT_DIV>,
>> +               <&clkgen JH7100_CLK_GMAC_AHB>,
>> +               <&clkgen JH7100_CLK_GMAC_PTP_REF>,
>> +               <&clkgen JH7100_CLK_GMAC_GTX>,
>> +               <&clkgen JH7100_CLK_GMAC_TX_INV>;
>> +      clock-names = "stmmaceth", "pclk", "ptp_ref", "gtxc", "tx";
>> +      resets = <&rstgen JH7100_RSTN_GMAC_AHB>;
>> +      reset-names = "ahb";
>> +      interrupts = <6>, <7>;
>> +      interrupt-names = "macirq", "eth_wake_irq";
>> +      max-frame-size = <9000>;
>> +      phy-mode = "rgmii-txid";
> 
> This is unusual. Does your board have a really long RX clock line to
> insert the 2ns delay needed on the RX side?

Just tested with "rgmii" and didn't notice any issues. If I'm not 
missing anything, I'll do the change in the next revision.

>         Andrew

Thanks,
Cristian
  

On Wed, Feb 15, 2023 at 02:34:23AM +0200, Cristian Ciocaltea wrote:
> On 2/11/23 18:01, Andrew Lunn wrote:
> > > +  starfive,gtxclk-dlychain:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    description: GTX clock delay chain setting
> > 
> > Please could you add more details to this. Is this controlling the
> > RGMII delays? 0ns or 2ns?
> 
> This is what gets written to JH7100_SYSMAIN_REGISTER49 and it's currently
> set to 4 in patch 12/12. As already mentioned, I don't have the register
> information in the datasheet, but I'll update this as soon as we get some
> details.

I have seen what happens to this value, but i have no idea what it
actually means. And without knowing what it means, i cannot say if it
is being used correctly or not. And it could be related to the next
part of my comment...

> 
> > > +    gmac: ethernet@10020000 {
> > > +      compatible = "starfive,jh7100-dwmac", "snps,dwmac";
> > > +      reg = <0x0 0x10020000 0x0 0x10000>;
> > > +      clocks = <&clkgen JH7100_CLK_GMAC_ROOT_DIV>,
> > > +               <&clkgen JH7100_CLK_GMAC_AHB>,
> > > +               <&clkgen JH7100_CLK_GMAC_PTP_REF>,
> > > +               <&clkgen JH7100_CLK_GMAC_GTX>,
> > > +               <&clkgen JH7100_CLK_GMAC_TX_INV>;
> > > +      clock-names = "stmmaceth", "pclk", "ptp_ref", "gtxc", "tx";
> > > +      resets = <&rstgen JH7100_RSTN_GMAC_AHB>;
> > > +      reset-names = "ahb";
> > > +      interrupts = <6>, <7>;
> > > +      interrupt-names = "macirq", "eth_wake_irq";
> > > +      max-frame-size = <9000>;
> > > +      phy-mode = "rgmii-txid";
> > 
> > This is unusual. Does your board have a really long RX clock line to
> > insert the 2ns delay needed on the RX side?
> 
> Just tested with "rgmii" and didn't notice any issues. If I'm not missing
> anything, I'll do the change in the next revision.

rgmii-id is generally the value to be used. That indicates the board
needs 2ns delays adding by something, either the MAC or the PHY. And
then i always recommend the MAC driver does nothing, pass the value to
the PHY and let the PHY add the delays.

So try both rgmii and rgmii-id and do a lot of bi directional
transfers. Then look at the reported ethernet frame check sum error
counts, both local and the link peer. I would expect one setting gives
you lots of errors, and the other works much better.

    Andrew
  

On 2/15/23 15:01, Andrew Lunn wrote:
> On Wed, Feb 15, 2023 at 02:34:23AM +0200, Cristian Ciocaltea wrote:
>> On 2/11/23 18:01, Andrew Lunn wrote:
>>>> +  starfive,gtxclk-dlychain:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description: GTX clock delay chain setting
>>>
>>> Please could you add more details to this. Is this controlling the
>>> RGMII delays? 0ns or 2ns?
>>
>> This is what gets written to JH7100_SYSMAIN_REGISTER49 and it's currently
>> set to 4 in patch 12/12. As already mentioned, I don't have the register
>> information in the datasheet, but I'll update this as soon as we get some
>> details.
> 
> I have seen what happens to this value, but i have no idea what it
> actually means. And without knowing what it means, i cannot say if it
> is being used correctly or not. And it could be related to the next
> part of my comment...
> 
>>
>>>> +    gmac: ethernet@10020000 {
>>>> +      compatible = "starfive,jh7100-dwmac", "snps,dwmac";
>>>> +      reg = <0x0 0x10020000 0x0 0x10000>;
>>>> +      clocks = <&clkgen JH7100_CLK_GMAC_ROOT_DIV>,
>>>> +               <&clkgen JH7100_CLK_GMAC_AHB>,
>>>> +               <&clkgen JH7100_CLK_GMAC_PTP_REF>,
>>>> +               <&clkgen JH7100_CLK_GMAC_GTX>,
>>>> +               <&clkgen JH7100_CLK_GMAC_TX_INV>;
>>>> +      clock-names = "stmmaceth", "pclk", "ptp_ref", "gtxc", "tx";
>>>> +      resets = <&rstgen JH7100_RSTN_GMAC_AHB>;
>>>> +      reset-names = "ahb";
>>>> +      interrupts = <6>, <7>;
>>>> +      interrupt-names = "macirq", "eth_wake_irq";
>>>> +      max-frame-size = <9000>;
>>>> +      phy-mode = "rgmii-txid";
>>>
>>> This is unusual. Does your board have a really long RX clock line to
>>> insert the 2ns delay needed on the RX side?
>>
>> Just tested with "rgmii" and didn't notice any issues. If I'm not missing
>> anything, I'll do the change in the next revision.
> 
> rgmii-id is generally the value to be used. That indicates the board
> needs 2ns delays adding by something, either the MAC or the PHY. And
> then i always recommend the MAC driver does nothing, pass the value to
> the PHY and let the PHY add the delays.
> 
> So try both rgmii and rgmii-id and do a lot of bi directional
> transfers. Then look at the reported ethernet frame check sum error
> counts, both local and the link peer. I would expect one setting gives
> you lots of errors, and the other works much better.

I gave "rgmii-id" a try and it's not usable, I get too many errors. So 
"rgmii" should be the right choice here.

Thanks,
Cristian

>      Andrew
>
  

> I gave "rgmii-id" a try and it's not usable, I get too many errors. So
> "rgmii" should be the right choice here.

I would actually say it shows we don't understand what is going on
with delays. "rgmii" is not every often the correct value. The fact it
works suggests the MAC is adding delays.

What value are you using for starfive,gtxclk-dlychain ? Try 0 and then
"rgmii-id"

	Andrew
  

On 2/16/23 19:54, Andrew Lunn wrote:
>> I gave "rgmii-id" a try and it's not usable, I get too many errors. So
>> "rgmii" should be the right choice here.
> 
> I would actually say it shows we don't understand what is going on
> with delays. "rgmii" is not every often the correct value. The fact it
> works suggests the MAC is adding delays.
> 
> What value are you using for starfive,gtxclk-dlychain ? 

This is set to '4' in patch 12/12.

> Try 0 and then "rgmii-id"

I made some more tests and it seems the only stable configuration is 
"rgmii" with "starfive,gtxclk-dlychain" set to 4:

phy-mode | dlychain | status
---------+----------+--------------------------------------------
rgmii    |        4 | OK (no issues observed)
rgmii-id |        4 | BROKEN (errors reported [1])
rgmii    |        0 | UNRELIABLE (no errors, but frequent stalls)
rgmii-id |        0 | BROKEN (errors reported)

[1] Reported errors in case of BROKEN status:
$ grep '' /sys/class/net/eth0/statistics/* | grep -v ':0$'

/sys/class/net/eth0/statistics/rx_crc_errors:6
/sys/class/net/eth0/statistics/rx_errors:6
/sys/class/net/eth0/statistics/tx_bytes:10836
/sys/class/net/eth0/statistics/tx_packets:46

> 	Andrew			
>
  

> > I would actually say it shows we don't understand what is going on
> > with delays. "rgmii" is not every often the correct value. The fact it
> > works suggests the MAC is adding delays.
> > 
> > What value are you using for starfive,gtxclk-dlychain ?
> 
> This is set to '4' in patch 12/12.
> 
> > Try 0 and then "rgmii-id"
> 
> I made some more tests and it seems the only stable configuration is "rgmii"
> with "starfive,gtxclk-dlychain" set to 4:
> 
> phy-mode | dlychain | status
> ---------+----------+--------------------------------------------
> rgmii    |        4 | OK (no issues observed)
> rgmii-id |        4 | BROKEN (errors reported [1])
> rgmii    |        0 | UNRELIABLE (no errors, but frequent stalls)
> rgmii-id |        0 | BROKEN (errors reported)
> 
> [1] Reported errors in case of BROKEN status:
> $ grep '' /sys/class/net/eth0/statistics/* | grep -v ':0$'

Thanks for the testing.

So it seems like something is adding delays when it probably should
not. Ideally we want to know what.

There is a danger here, something which has happened in the past. A
PHY which ignored "rgmii" and actually did power on defaults which was
"rgmii-id". As a result, lots of boards put "rmgii" in there DT blob,
which 'worked'. Until a board came along which really did need
"rgmii". The developer bringing that board up debugged the PHY, found
the problem and made it respect "rgmii" so their board worked. And the
fix broke a number of 'working' boards which had the wrong "rgmii"
instead of "rgmii-id".

So you have a choice. Go with 4 and "rgmii", but put in a big fat
warning, "Works somehow but is technically wrong and will probably
break sometime in the future". Or try to understand what is really
going on here, were are the delays coming from, and fix the issue.

      Andrew
  

On 2/17/23 15:30, Andrew Lunn wrote:
>>> I would actually say it shows we don't understand what is going on
>>> with delays. "rgmii" is not every often the correct value. The fact it
>>> works suggests the MAC is adding delays.
>>>
>>> What value are you using for starfive,gtxclk-dlychain ?
>>
>> This is set to '4' in patch 12/12.
>>
>>> Try 0 and then "rgmii-id"
>>
>> I made some more tests and it seems the only stable configuration is "rgmii"
>> with "starfive,gtxclk-dlychain" set to 4:
>>
>> phy-mode | dlychain | status
>> ---------+----------+--------------------------------------------
>> rgmii    |        4 | OK (no issues observed)
>> rgmii-id |        4 | BROKEN (errors reported [1])
>> rgmii    |        0 | UNRELIABLE (no errors, but frequent stalls)
>> rgmii-id |        0 | BROKEN (errors reported)
>>
>> [1] Reported errors in case of BROKEN status:
>> $ grep '' /sys/class/net/eth0/statistics/* | grep -v ':0$'
> 
> Thanks for the testing.
> 
> So it seems like something is adding delays when it probably should
> not. Ideally we want to know what.
> 
> There is a danger here, something which has happened in the past. A
> PHY which ignored "rgmii" and actually did power on defaults which was
> "rgmii-id". As a result, lots of boards put "rmgii" in there DT blob,
> which 'worked'. Until a board came along which really did need
> "rgmii". The developer bringing that board up debugged the PHY, found
> the problem and made it respect "rgmii" so their board worked. And the
> fix broke a number of 'working' boards which had the wrong "rgmii"
> instead of "rgmii-id".

Thanks for the heads-up.

> So you have a choice. Go with 4 and "rgmii", but put in a big fat
> warning, "Works somehow but is technically wrong and will probably
> break sometime in the future". Or try to understand what is really
> going on here, were are the delays coming from, and fix the issue.
> 
>        Andrew

I will try to analyze this further.

Regards,
Cristian
  

On 2/17/23 17:25, Cristian Ciocaltea wrote:
> On 2/17/23 15:30, Andrew Lunn wrote:
>>>> I would actually say it shows we don't understand what is going on
>>>> with delays. "rgmii" is not every often the correct value. The fact it
>>>> works suggests the MAC is adding delays.
>>>>
>>>> What value are you using for starfive,gtxclk-dlychain ?
>>>
>>> This is set to '4' in patch 12/12.
>>>
>>>> Try 0 and then "rgmii-id"
>>>
>>> I made some more tests and it seems the only stable configuration is
>>> "rgmii"
>>> with "starfive,gtxclk-dlychain" set to 4:
>>>
>>> phy-mode | dlychain | status
>>> ---------+----------+--------------------------------------------
>>> rgmii    |        4 | OK (no issues observed)
>>> rgmii-id |        4 | BROKEN (errors reported [1])
>>> rgmii    |        0 | UNRELIABLE (no errors, but frequent stalls)
>>> rgmii-id |        0 | BROKEN (errors reported)
>>>
>>> [1] Reported errors in case of BROKEN status:
>>> $ grep '' /sys/class/net/eth0/statistics/* | grep -v ':0$'
>>
>> Thanks for the testing.
>>
>> So it seems like something is adding delays when it probably should
>> not. Ideally we want to know what.
>>
>> There is a danger here, something which has happened in the past. A
>> PHY which ignored "rgmii" and actually did power on defaults which was
>> "rgmii-id". As a result, lots of boards put "rmgii" in there DT blob,
>> which 'worked'. Until a board came along which really did need
>> "rgmii". The developer bringing that board up debugged the PHY, found
>> the problem and made it respect "rgmii" so their board worked. And the
>> fix broke a number of 'working' boards which had the wrong "rgmii"
>> instead of "rgmii-id".
> 
> Thanks for the heads-up.
> 
>> So you have a choice. Go with 4 and "rgmii", but put in a big fat
>> warning, "Works somehow but is technically wrong and will probably
>> break sometime in the future". Or try to understand what is really
>> going on here, were are the delays coming from, and fix the issue.
>>
>>        Andrew
> 
> I will try to analyze this further.

As the non-coherent DMA work this series depended on has been completed,
I started to investigate further the "rgmii-id" issue.
I couldn't spot anything wrong in the Motorcomm PHY driver, but
eventually got this working by adjusting rx-internal-delay-ps.

Will do some more testing before submitting v2.

Thanks,
Cristian