| Message ID | 20230211031821.976408-1-cristian.ciocaltea@collabora.com |
|---|---|
| Headers | show |
| Series | Enable networking support for StarFive JH7100 SoC | expand |
On 11/02/2023 03:18, Cristian Ciocaltea wrote: > From: Emil Renner Berthing <kernel@esmil.dk> > > This variant is used on the StarFive JH7100 SoC. > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > --- > arch/riscv/Kconfig | 6 ++++-- > arch/riscv/mm/dma-noncoherent.c | 37 +++++++++++++++++++++++++++++++-- > 2 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index 9c687da7756d..05f6c77faf6f 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -232,12 +232,14 @@ config LOCKDEP_SUPPORT > def_bool y > > config RISCV_DMA_NONCOHERENT > - bool > + bool "Support non-coherent DMA" > + default SOC_STARFIVE > select ARCH_HAS_DMA_PREP_COHERENT > + select ARCH_HAS_DMA_SET_UNCACHED > + select ARCH_HAS_DMA_CLEAR_UNCACHED > select ARCH_HAS_SYNC_DMA_FOR_DEVICE > select ARCH_HAS_SYNC_DMA_FOR_CPU > select ARCH_HAS_SETUP_DMA_OPS > - select DMA_DIRECT_REMAP > > config AS_HAS_INSN > def_bool $(as-instr,.insn r 51$(comma) 0$(comma) 0$(comma) t0$(comma) t0$(comma) zero) > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index d919efab6eba..e07e53aea537 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -9,14 +9,21 @@ > #include <linux/dma-map-ops.h> > #include <linux/mm.h> > #include <asm/cacheflush.h> > +#include <soc/sifive/sifive_ccache.h> > > static bool noncoherent_supported; > > void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > enum dma_data_direction dir) > { > - void *vaddr = phys_to_virt(paddr); > + void *vaddr; > > + if (sifive_ccache_handle_noncoherent()) { > + sifive_ccache_flush_range(paddr, size); > + return; > + } > + > + vaddr = phys_to_virt(paddr); > switch (dir) { > case DMA_TO_DEVICE: > ALT_CMO_OP(clean, vaddr, size, riscv_cbom_block_size); > @@ -35,8 +42,14 @@ void arch_sync_dma_for_device(phys_addr_t paddr, size_t size, > void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size, > enum dma_data_direction dir) > { > - void *vaddr = phys_to_virt(paddr); > + void *vaddr; > + > + if (sifive_ccache_handle_noncoherent()) { > + sifive_ccache_flush_range(paddr, size); > + return; > + } ok, what happens if we have an system where the ccache and another level of cache also requires maintenance operations? > > + vaddr = phys_to_virt(paddr); > switch (dir) { > case DMA_TO_DEVICE: > break; > @@ -49,10 +62,30 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size, > } > } > > +void *arch_dma_set_uncached(void *addr, size_t size) > +{ > + if (sifive_ccache_handle_noncoherent()) > + return sifive_ccache_set_uncached(addr, size); > + > + return addr; > +} > + > +void arch_dma_clear_uncached(void *addr, size_t size) > +{ > + if (sifive_ccache_handle_noncoherent()) > + sifive_ccache_clear_uncached(addr, size); > +} > + > void arch_dma_prep_coherent(struct page *page, size_t size) > { > void *flush_addr = page_address(page); > > + if (sifive_ccache_handle_noncoherent()) { > + memset(flush_addr, 0, size); > + sifive_ccache_flush_range(__pa(flush_addr), size); > + return; > + } > + > ALT_CMO_OP(flush, flush_addr, size, riscv_cbom_block_size); > } >
On 11/02/2023 04:18, Cristian Ciocaltea wrote: > Document the compatible for the SiFive Composable Cache Controller found > on the StarFive JH7100 SoC. > > This also requires extending the 'reg' property to handle distinct > ranges, as specified via 'reg-names'. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
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 11/02/2023 04:18, Cristian Ciocaltea wrote: > Provide the sysmain and gmac DT nodes supporting the DWMAC found on the > StarFive JH7100 SoC. > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > --- > arch/riscv/boot/dts/starfive/jh7100.dtsi | 38 ++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/arch/riscv/boot/dts/starfive/jh7100.dtsi b/arch/riscv/boot/dts/starfive/jh7100.dtsi > index 88f91bc5753b..0918af7b6eb0 100644 > --- a/arch/riscv/boot/dts/starfive/jh7100.dtsi > +++ b/arch/riscv/boot/dts/starfive/jh7100.dtsi > @@ -164,6 +164,44 @@ rstgen: reset-controller@11840000 { > #reset-cells = <1>; > }; > > + sysmain: syscon@11850000 { > + compatible = "starfive,jh7100-sysmain", "syscon"; > + reg = <0x0 0x11850000 0x0 0x10000>; > + }; > + > + gmac: ethernet@10020000 { Aren't the nodes ordered by address? Best regards, Krzysztof
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 >
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 Sat, Feb 11, 2023 at 05:18:12AM +0200, Cristian Ciocaltea wrote: > From: Emil Renner Berthing <kernel@esmil.dk> > > This adds support for the StarFive JH7100 SoC which also feature this > SiFive cache controller. > > Unfortunately the interrupt for uncorrected data is broken on the JH7100 > and fires continuously, so add a quirk to not register a handler for it. > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > [drop JH7110, rework Kconfig] > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> This driver doesn't really do very much of anything as things stand, so I don't see really see all that much value in picking it up right now, since the non-coherent bits aren't usable yet. > --- > drivers/soc/sifive/Kconfig | 1 + > drivers/soc/sifive/sifive_ccache.c | 11 ++++++++++- > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/soc/sifive/Kconfig b/drivers/soc/sifive/Kconfig > index e86870be34c9..867cf16273a4 100644 > --- a/drivers/soc/sifive/Kconfig > +++ b/drivers/soc/sifive/Kconfig > @@ -4,6 +4,7 @@ if SOC_SIFIVE || SOC_STARFIVE > > config SIFIVE_CCACHE > bool "Sifive Composable Cache controller" > + default SOC_STARFIVE I don't think this should have a default set w/ the support that this patch brings in. Perhaps later we should be doing defaulting, but not at this point in the series. Other than that, this is fine by me: Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Thanks, Conor.
On 3/7/23 01:32, Conor Dooley wrote: > On Sat, Feb 11, 2023 at 05:18:12AM +0200, Cristian Ciocaltea wrote: >> From: Emil Renner Berthing <kernel@esmil.dk> >> >> This adds support for the StarFive JH7100 SoC which also feature this >> SiFive cache controller. >> >> Unfortunately the interrupt for uncorrected data is broken on the JH7100 >> and fires continuously, so add a quirk to not register a handler for it. >> >> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >> [drop JH7110, rework Kconfig] >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > > This driver doesn't really do very much of anything as things stand, so > I don't see really see all that much value in picking it up right now, > since the non-coherent bits aren't usable yet. > >> --- >> drivers/soc/sifive/Kconfig | 1 + >> drivers/soc/sifive/sifive_ccache.c | 11 ++++++++++- >> 2 files changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/soc/sifive/Kconfig b/drivers/soc/sifive/Kconfig >> index e86870be34c9..867cf16273a4 100644 >> --- a/drivers/soc/sifive/Kconfig >> +++ b/drivers/soc/sifive/Kconfig >> @@ -4,6 +4,7 @@ if SOC_SIFIVE || SOC_STARFIVE >> >> config SIFIVE_CCACHE >> bool "Sifive Composable Cache controller" >> + default SOC_STARFIVE > > I don't think this should have a default set w/ the support that this > patch brings in. Perhaps later we should be doing defaulting, but not at > this point in the series. I will handle this is v2 as soon as the non-coherency stuff is ready. > Other than that, this is fine by me: > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Thanks for reviewing, Cristian > Thanks, > Conor.
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
This patch series adds ethernet support for the StarFive JH7100 SoC and makes it available for the StarFive VisionFive V1 and BeagleV Starlight boards, although I could only validate on the former SBC. The work is heavily based on the reference implementation [1] and requires the non-coherent DMA support provided by Emil via the Sifive Composable Cache controller. Also note there is an overlap in "[PATCH 08/12] net: stmmac: Add glue layer for StarFive JH7100 SoC" with the Yanhong Wang's upstreaming attempt [2]: "[PATCH v4 5/7] net: stmmac: Add glue layer for StarFive JH7110 SoCs". Since I cannot test the JH7110 SoC, I dropped the support for it from Emil's variant of the stmmac glue layer. Hence, we might need a bit of coordination in order to get this properly merged. [1] https://github.com/starfive-tech/linux/commits/visionfive [2] https://lore.kernel.org/linux-riscv/20230118061701.30047-6-yanhong.wang@starfivetech.com/ Cristian Ciocaltea (7): dt-bindings: riscv: sifive-ccache: Add compatible for StarFive JH7100 SoC dt-bindings: riscv: sifive-ccache: Add 'uncached-offset' property dt-bindings: net: Add StarFive JH7100 SoC riscv: dts: starfive: Add dma-noncoherent for JH7100 SoC riscv: dts: starfive: jh7100: Add ccache DT node riscv: dts: starfive: jh7100: Add sysmain and gmac DT nodes riscv: dts: starfive: jh7100-common: Setup pinmux and enable gmac Emil Renner Berthing (5): soc: sifive: ccache: Add StarFive JH7100 support soc: sifive: ccache: Add non-coherent DMA handling riscv: Implement non-coherent DMA support via SiFive cache flushing dt-bindings: mfd: syscon: Add StarFive JH7100 sysmain compatible net: stmmac: Add glue layer for StarFive JH7100 SoC .../devicetree/bindings/mfd/syscon.yaml | 1 + .../devicetree/bindings/net/snps,dwmac.yaml | 15 +- .../bindings/net/starfive,jh7100-dwmac.yaml | 106 ++++++++++++ .../bindings/riscv/sifive,ccache0.yaml | 33 +++- MAINTAINERS | 6 + arch/riscv/Kconfig | 6 +- .../boot/dts/starfive/jh7100-common.dtsi | 78 +++++++++ arch/riscv/boot/dts/starfive/jh7100.dtsi | 55 +++++++ arch/riscv/mm/dma-noncoherent.c | 37 ++++- drivers/net/ethernet/stmicro/stmmac/Kconfig | 12 ++ drivers/net/ethernet/stmicro/stmmac/Makefile | 1 + .../ethernet/stmicro/stmmac/dwmac-starfive.c | 155 ++++++++++++++++++ drivers/soc/sifive/Kconfig | 1 + drivers/soc/sifive/sifive_ccache.c | 71 +++++++- include/soc/sifive/sifive_ccache.h | 21 +++ 15 files changed, 587 insertions(+), 11 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/starfive,jh7100-dwmac.yaml create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c