| Message ID | 20230211031821.976408-8-cristian.ciocaltea@collabora.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Enable networking support for StarFive JH7100 SoC | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
> + 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
diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml index e88a86623fce..71522a2cd7a4 100644 --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml @@ -89,6 +89,7 @@ properties: - snps,dwmac-5.10a - snps,dwxgmac - snps,dwxgmac-2.10 + - starfive,jh7100-dwmac reg: minItems: 1 @@ -131,12 +132,17 @@ properties: - ptp_ref resets: - maxItems: 1 - description: - MAC Reset signal. + minItems: 1 + items: + - description: MAC Reset signal + - description: AHB Reset signal reset-names: - const: stmmaceth + minItems: 1 + contains: + enum: + - stmmaceth + - ahb power-domains: maxItems: 1 @@ -578,6 +584,7 @@ allOf: - snps,dwxgmac - snps,dwxgmac-2.10 - st,spear600-gmac + - starfive,jh7100-dwmac then: properties: diff --git a/Documentation/devicetree/bindings/net/starfive,jh7100-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7100-dwmac.yaml new file mode 100644 index 000000000000..6afe30690172 --- /dev/null +++ b/Documentation/devicetree/bindings/net/starfive,jh7100-dwmac.yaml @@ -0,0 +1,106 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (C) 2022 StarFive Technology Co., Ltd. +# Copyright (C) 2022 Emil Renner Berthing <kernel@esmil.dk> +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/starfive,jh7100-dwmac.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: StarFive JH7100 DWMAC Ethernet Controller + +maintainers: + - Emil Renner Berthing <kernel@esmil.dk> + +# We need a select here so we don't match all nodes with 'snps,dwmac' +select: + properties: + compatible: + contains: + const: starfive,jh7100-dwmac + required: + - compatible + +allOf: + - $ref: snps,dwmac.yaml# + +properties: + compatible: + items: + - const: starfive,jh7100-dwmac + - const: snps,dwmac + + clocks: + items: + - description: GMAC main clock + - description: GMAC AHB clock + - description: PTP clock + - description: GTX clock + - description: TX clock + + clock-names: + items: + - const: stmmaceth + - const: pclk + - const: ptp_ref + - const: gtxc + - const: tx + + resets: + description: AHB Reset signal + + reset-names: + const: ahb + + starfive,syscon: + $ref: /schemas/types.yaml#/definitions/phandle + description: phandle to the syscon node + + starfive,gtxclk-dlychain: + $ref: /schemas/types.yaml#/definitions/uint32 + description: GTX clock delay chain setting + +required: + - compatible + - clocks + - clock-names + - resets + - reset-names + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/starfive-jh7100.h> + #include <dt-bindings/reset/starfive-jh7100.h> + + 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"; + snps,multicast-filter-bins = <32>; + snps,perfect-filter-entries = <128>; + starfive,syscon = <&sysmain>; + rx-fifo-depth = <32768>; + tx-fifo-depth = <16384>; + snps,axi-config = <&stmmac_axi_setup>; + snps,fixed-burst; + snps,force_thresh_dma_mode; + snps,no-pbl-x8; + + stmmac_axi_setup: stmmac-axi-config { + snps,wr_osr_lmt = <0xf>; + snps,rd_osr_lmt = <0xf>; + snps,blen = <256 128 64 32 0 0 0>; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index abed40db41f0..d48468b81b94 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19816,6 +19816,11 @@ M: Emil Renner Berthing <kernel@esmil.dk> S: Maintained F: arch/riscv/boot/dts/starfive/ +STARFIVE DWMAC GLUE LAYER +M: Emil Renner Berthing <kernel@esmil.dk> +S: Maintained +F: Documentation/devicetree/bindings/net/starfive,jh7100-dwmac.yaml + STARFIVE JH7100 CLOCK DRIVERS M: Emil Renner Berthing <kernel@esmil.dk> S: Maintained
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