| Message ID | 20231029042712.520010-1-cristian.ciocaltea@collabora.com |
|---|---|
| Headers | show |
| Series | Enable networking support for StarFive JH7100 SoC | expand |
On 10/29/23 23:55, Cristian Ciocaltea wrote: > On 10/29/23 13:21, Krzysztof Kozlowski wrote: >> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >>> just the 'ahb' reset name, but the binding allows selecting it only in >>> conjunction with 'stmmaceth'. >>> >>> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >>> >>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>> --- >>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> index 5c2769dc689a..a4d7172ea701 100644 >>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> @@ -146,7 +146,7 @@ properties: >>> reset-names: >>> minItems: 1 >>> items: >>> - - const: stmmaceth >>> + - enum: [stmmaceth, ahb] >> >> Your patch #3 says you have minimum two items. Here you claim you have >> only one reset. It's confusing. > > At least the following use-cases need to be supported: > > - JH7110: reset-names = "stmmaceth", "ahb"; > - JH7110: reset-names = "ahb"; I've just realized my mistake here - this is for JH7100, sorry for the confusion: - JH7100: reset-names = "ahb"; > - other: reset-names = "stmmaceth"; > > Since this is the schema which gets included later in other bindings, > the property needs to be generic enough to cope with all the above. > [added actual content here for more clarity] > > reset-names: > minItems: 1 > items: > - enum: [stmmaceth, ahb] > - const: ahb > > Therefore, only the lower limit (1) is enforced here, while > starfive,jh7110-dwmac.yaml (which PATCH 3 relates to) adds further > constraints (limiting to precisely two items): > > reset-names: > items: > - const: stmmaceth > - const: ahb > > I understand the generic binding also allows now specifying 'ahb' twice, > but I'm not sure if there's a convenient way to avoid that (e.g. without > complicating excessively the schema).
On 10/29/23 20:45, Andrew Lunn wrote: > On Sun, Oct 29, 2023 at 06:27:11AM +0200, Cristian Ciocaltea wrote: >> The StarFive VisionFive V1 SBC has a Motorcomm YT8521 PHY supporting >> RGMII-ID, but requires manual adjustment of the RX internal delay to >> work properly. >> >> The default RX delay provided by the driver is 1.95 ns, which proves to >> be too high. Applying a 50% reduction seems to mitigate the issue. > > I'm not so happy this cannot be explained. You are potentially heading > into horrible backwards compatibility problems with old DT blobs and > new kernels once this is explained and fixed. It seems the visionfive-v2 board also required setting some delays, but unfortunately no details were provided: 0104340a67b1 ("riscv: dts: starfive: visionfive 2: Add configuration of mac and phy") Thanks, Cristian
On 10/30/23 00:50, Andrew Lunn wrote: > On Mon, Oct 30, 2023 at 12:41:23AM +0200, Cristian Ciocaltea wrote: >> On 10/29/23 20:45, Andrew Lunn wrote: >>> On Sun, Oct 29, 2023 at 06:27:11AM +0200, Cristian Ciocaltea wrote: >>>> The StarFive VisionFive V1 SBC has a Motorcomm YT8521 PHY supporting >>>> RGMII-ID, but requires manual adjustment of the RX internal delay to >>>> work properly. >>>> >>>> The default RX delay provided by the driver is 1.95 ns, which proves to >>>> be too high. Applying a 50% reduction seems to mitigate the issue. >>> >>> I'm not so happy this cannot be explained. You are potentially heading >>> into horrible backwards compatibility problems with old DT blobs and >>> new kernels once this is explained and fixed. >> >> It seems the visionfive-v2 board also required setting some delays, but >> unfortunately no details were provided: >> >> 0104340a67b1 ("riscv: dts: starfive: visionfive 2: Add configuration of >> mac and phy") > > That board also uses a YT8531 PHY. Its possible this is somehow to do > with the PHY. Which is why testing with the Microchip PHY is > important. That should answer the question is it a SoC or a PHY > problem. There is also YT8531S, which looks compatible with YT8521, but YT8531 seems to be a bit different. Regardless of what VisionFive v2 is using, it would be indeed interesting to find out how the Microchip PHY behaves in this context. Regards, Cristian
On 29/10/2023 23:24, Cristian Ciocaltea wrote: > On 10/29/23 13:25, Krzysztof Kozlowski wrote: >> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >>> just the 'ahb' reset name, but the binding allows selecting it only in >>> conjunction with 'stmmaceth'. >>> >>> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >>> >>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>> --- >>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> index 5c2769dc689a..a4d7172ea701 100644 >>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> @@ -146,7 +146,7 @@ properties: >>> reset-names: >>> minItems: 1 >>> items: >>> - - const: stmmaceth >>> + - enum: [stmmaceth, ahb] >> >> Also, this makes sense only with patch #4, so this should be squashed there. > > I added this as a separate patch since it changes the generic schema > which is included by many other bindings. JH7100 just happens to be the > first use-case requiring this update. But I can squash the patch if > that's not a good enough reason to keep it separately. If there is no single user of this, why changing this? I would even argue that it is not correct from existing bindings point of view - nothing allows and uses ahb as the only reset. Even the commit msg mentions your hardware from patch 4. Best regards, Krzysztof
On 29/10/2023 22:23, Cristian Ciocaltea wrote: > On 10/29/23 13:19, Krzysztof Kozlowski wrote: >> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>> The reset description items are already provided by the referenced >>> snps,dwmac.yaml schema, hence replace them with the necessary >>> {min,max}Items. >>> >>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>> --- >>> .../devicetree/bindings/net/starfive,jh7110-dwmac.yaml | 5 ++--- >>> 1 file changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml >>> index cc3e1c6fc135..44e58755a5a2 100644 >>> --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml >>> +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml >>> @@ -46,9 +46,8 @@ properties: >>> maxItems: 3 >>> >>> resets: >>> - items: >>> - - description: MAC Reset signal. >>> - - description: AHB Reset signal. >>> + minItems: 2 >>> + maxItems: 2 >> >> You must also update reset-names. They must have same constraints. > > reset-names explicitly lists the items and we need to keep them as such > because the order is not fixed, i.e. PATCH 1 allows using 'ahb' instead > of 'stmmaceth' as the first (and only) item. > > reset-names: > items: > - const: stmmaceth > - const: ahb OK. Anyway this patch is no-op because next one removes this code. Adding cleanup which is immediately removed does not make much sense. Drop it. > > Thanks, > Cristian Best regards, Krzysztof
On 10/30/23 09:26, Krzysztof Kozlowski wrote: > On 29/10/2023 23:24, Cristian Ciocaltea wrote: >> On 10/29/23 13:25, Krzysztof Kozlowski wrote: >>> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>>> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >>>> just the 'ahb' reset name, but the binding allows selecting it only in >>>> conjunction with 'stmmaceth'. >>>> >>>> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >>>> >>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>>> --- >>>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>> index 5c2769dc689a..a4d7172ea701 100644 >>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>> @@ -146,7 +146,7 @@ properties: >>>> reset-names: >>>> minItems: 1 >>>> items: >>>> - - const: stmmaceth >>>> + - enum: [stmmaceth, ahb] >>> >>> Also, this makes sense only with patch #4, so this should be squashed there. >> >> I added this as a separate patch since it changes the generic schema >> which is included by many other bindings. JH7100 just happens to be the >> first use-case requiring this update. But I can squash the patch if >> that's not a good enough reason to keep it separately. > > If there is no single user of this, why changing this? I would even > argue that it is not correct from existing bindings point of view - > nothing allows and uses ahb as the only reset. Even the commit msg > mentions your hardware from patch 4. Sorry, I'm not sure I follow. JH7100 is (or will be) the user of it and, as a matter of fact, something similar has been done recently while adding support for JH7110. In particular, commit [1] changed this binding before the JH7110 compatible was introduced in a subsequent patch. On a closer look that commit made a statement which is not entirely correct: "dwmac controller may require one (stmmaceth) or two (stmmaceth+ahb) reset signals" That's because stmmaceth is also optional in dwmac's driver, hence the correct message would have been: "[...] may require one (stmmaceth OR ahb) [...]" Hence, I think it makes sense to keep this patch, after adding the above details in the commit message. [1] 843f603762a5 ("dt-bindings: net: snps,dwmac: Add 'ahb' reset/reset-name") Thanks, Cristian
On 10/30/23 09:29, Krzysztof Kozlowski wrote: > On 29/10/2023 22:23, Cristian Ciocaltea wrote: >> On 10/29/23 13:19, Krzysztof Kozlowski wrote: >>> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>>> The reset description items are already provided by the referenced >>>> snps,dwmac.yaml schema, hence replace them with the necessary >>>> {min,max}Items. >>>> >>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>>> --- >>>> .../devicetree/bindings/net/starfive,jh7110-dwmac.yaml | 5 ++--- >>>> 1 file changed, 2 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml >>>> index cc3e1c6fc135..44e58755a5a2 100644 >>>> --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml >>>> +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml >>>> @@ -46,9 +46,8 @@ properties: >>>> maxItems: 3 >>>> >>>> resets: >>>> - items: >>>> - - description: MAC Reset signal. >>>> - - description: AHB Reset signal. >>>> + minItems: 2 >>>> + maxItems: 2 >>> >>> You must also update reset-names. They must have same constraints. >> >> reset-names explicitly lists the items and we need to keep them as such >> because the order is not fixed, i.e. PATCH 1 allows using 'ahb' instead >> of 'stmmaceth' as the first (and only) item. >> >> reset-names: >> items: >> - const: stmmaceth >> - const: ahb > > OK. Anyway this patch is no-op because next one removes this code. > Adding cleanup which is immediately removed does not make much sense. > Drop it. The next patch just moves that under an if clause. Regards, Cristian
On 30/10/2023 20:07, Cristian Ciocaltea wrote: > On 10/30/23 09:26, Krzysztof Kozlowski wrote: >> On 29/10/2023 23:24, Cristian Ciocaltea wrote: >>> On 10/29/23 13:25, Krzysztof Kozlowski wrote: >>>> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>>>> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >>>>> just the 'ahb' reset name, but the binding allows selecting it only in >>>>> conjunction with 'stmmaceth'. >>>>> >>>>> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >>>>> >>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>>>> --- >>>>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>> index 5c2769dc689a..a4d7172ea701 100644 >>>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>> @@ -146,7 +146,7 @@ properties: >>>>> reset-names: >>>>> minItems: 1 >>>>> items: >>>>> - - const: stmmaceth >>>>> + - enum: [stmmaceth, ahb] >>>> >>>> Also, this makes sense only with patch #4, so this should be squashed there. >>> >>> I added this as a separate patch since it changes the generic schema >>> which is included by many other bindings. JH7100 just happens to be the >>> first use-case requiring this update. But I can squash the patch if >>> that's not a good enough reason to keep it separately. >> >> If there is no single user of this, why changing this? I would even >> argue that it is not correct from existing bindings point of view - >> nothing allows and uses ahb as the only reset. Even the commit msg >> mentions your hardware from patch 4. > > Sorry, I'm not sure I follow. JH7100 is (or will be) the user of it and, > as a matter of fact, something similar has been done recently while > adding support for JH7110. Every patch should stand on its own and at this point nothing uses it. We apply this patch #1 and we add dead code, unused case. > > In particular, commit [1] changed this binding before the JH7110 > compatible was introduced in a subsequent patch. On a closer look that > commit made a statement which is not entirely correct: > > "dwmac controller may require one (stmmaceth) or two (stmmaceth+ahb) > reset signals" > > That's because stmmaceth is also optional in dwmac's driver, hence the > correct message would have been: > > "[...] may require one (stmmaceth OR ahb) [...]" Driver does not describe the hardware. The bindings do, so according to that description all supported hardware required MAC reset (stmmaceth). Otherwise please point me to any hardware which skips MAC reset and requires AHB reset instead (not future hardware, but current). > > Hence, I think it makes sense to keep this patch, after adding the above > details in the commit message. > > [1] 843f603762a5 ("dt-bindings: net: snps,dwmac: Add 'ahb' > reset/reset-name") > > Thanks, > Cristian Best regards, Krzysztof
On 10/31/23 07:48, Krzysztof Kozlowski wrote: > On 30/10/2023 20:07, Cristian Ciocaltea wrote: >> On 10/30/23 09:26, Krzysztof Kozlowski wrote: >>> On 29/10/2023 23:24, Cristian Ciocaltea wrote: >>>> On 10/29/23 13:25, Krzysztof Kozlowski wrote: >>>>> On 29/10/2023 05:27, Cristian Ciocaltea wrote: >>>>>> The Synopsys DesignWare MAC found on the StarFive JH7100 SoC requires >>>>>> just the 'ahb' reset name, but the binding allows selecting it only in >>>>>> conjunction with 'stmmaceth'. >>>>>> >>>>>> Fix the issue by permitting exclusive usage of the 'ahb' reset name. >>>>>> >>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >>>>>> --- >>>>>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>>> index 5c2769dc689a..a4d7172ea701 100644 >>>>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>>>>> @@ -146,7 +146,7 @@ properties: >>>>>> reset-names: >>>>>> minItems: 1 >>>>>> items: >>>>>> - - const: stmmaceth >>>>>> + - enum: [stmmaceth, ahb] >>>>> >>>>> Also, this makes sense only with patch #4, so this should be squashed there. >>>> >>>> I added this as a separate patch since it changes the generic schema >>>> which is included by many other bindings. JH7100 just happens to be the >>>> first use-case requiring this update. But I can squash the patch if >>>> that's not a good enough reason to keep it separately. >>> >>> If there is no single user of this, why changing this? I would even >>> argue that it is not correct from existing bindings point of view - >>> nothing allows and uses ahb as the only reset. Even the commit msg >>> mentions your hardware from patch 4. >> >> Sorry, I'm not sure I follow. JH7100 is (or will be) the user of it and, >> as a matter of fact, something similar has been done recently while >> adding support for JH7110. > > Every patch should stand on its own and at this point nothing uses it. > We apply this patch #1 and we add dead code, unused case. > >> >> In particular, commit [1] changed this binding before the JH7110 >> compatible was introduced in a subsequent patch. On a closer look that >> commit made a statement which is not entirely correct: >> >> "dwmac controller may require one (stmmaceth) or two (stmmaceth+ahb) >> reset signals" >> >> That's because stmmaceth is also optional in dwmac's driver, hence the >> correct message would have been: >> >> "[...] may require one (stmmaceth OR ahb) [...]" > > Driver does not describe the hardware. The bindings do, so according to > that description all supported hardware required MAC reset (stmmaceth). > Otherwise please point me to any hardware which skips MAC reset and > requires AHB reset instead (not future hardware, but current). I might have found one (arch/arm/boot/dts/allwinner/sun6i-a31.dtsi): gmac: ethernet@1c30000 { compatible = "allwinner,sun7i-a20-gmac"; [...] resets = <&ccu RST_AHB1_EMAC>; reset-names = "stmmaceth"; [...] }; It's possible the 'stmmaceth' naming has been used instead of 'ahb' just to avoid updating the binding, but that's just an assumption looking at RST_AHB1_EMAC. I will proceed with the squash for v3. Thanks for clarifying, Cristian
Cristian Ciocaltea wrote: > Provide a DT node for the SiFive Composable Cache controller found on > the StarFive JH7100 SoC. > > Note this is also used to support non-coherent DMA, via the > sifive,cache-ops cache flushing operations. This property is no longer needed: https://lore.kernel.org/linux-riscv/20231031141444.53426-1-emil.renner.berthing@canonical.com/ Also it would be nice to mention that these nodes are copied from my visionfive patches ;) > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > --- > arch/riscv/boot/dts/starfive/jh7100.dtsi | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/arch/riscv/boot/dts/starfive/jh7100.dtsi b/arch/riscv/boot/dts/starfive/jh7100.dtsi > index 06bb157ce111..a8a5bb00b0d8 100644 > --- a/arch/riscv/boot/dts/starfive/jh7100.dtsi > +++ b/arch/riscv/boot/dts/starfive/jh7100.dtsi > @@ -32,6 +32,7 @@ U74_0: cpu@0 { > i-tlb-sets = <1>; > i-tlb-size = <32>; > mmu-type = "riscv,sv39"; > + next-level-cache = <&ccache>; > riscv,isa = "rv64imafdc"; > riscv,isa-base = "rv64i"; > riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr", > @@ -60,6 +61,7 @@ U74_1: cpu@1 { > i-tlb-sets = <1>; > i-tlb-size = <32>; > mmu-type = "riscv,sv39"; > + next-level-cache = <&ccache>; > riscv,isa = "rv64imafdc"; > riscv,isa-base = "rv64i"; > riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr", > @@ -147,6 +149,18 @@ soc { > dma-noncoherent; > ranges; > > + ccache: cache-controller@2010000 { > + compatible = "starfive,jh7100-ccache", "sifive,ccache0", "cache"; > + reg = <0x0 0x2010000 0x0 0x1000>; > + interrupts = <128>, <130>, <131>, <129>; > + cache-block-size = <64>; > + cache-level = <2>; > + cache-sets = <2048>; > + cache-size = <2097152>; > + cache-unified; > + sifive,cache-ops; > + }; > + > clint: clint@2000000 { > compatible = "starfive,jh7100-clint", "sifive,clint0"; > reg = <0x0 0x2000000 0x0 0x10000>; > -- > 2.42.0 >
On 10/31/23 16:33, Emil Renner Berthing wrote: > Cristian Ciocaltea wrote: >> Add a missing quirk to enable support for the StarFive JH7100 SoC. >> >> Additionally, for greater flexibility in operation, allow using the >> rgmii-rxid and rgmii-txid phy modes. >> >> Co-developed-by: Emil Renner Berthing <kernel@esmil.dk> >> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > > Hi Cristian, > > Thanks for working on this! This driver has code to update the phy clock for > different line speeds. I don't think that will work without the > CLK_SET_RATE_PARENT flag added to the clock in [1] which in turn depends on > [2]. > > [1]: https://github.com/esmil/linux/commit/b200c3054b58a49ba25af67aff82d9045e3c3666 > [2]: https://github.com/esmil/linux/commit/dce189542c16bf0eb8533d96c0305cb59d149dae > > Two more comments below.. Hi Emil, Thanks for the review! I've been only testing this at 1000 Mbps and so far it seems to work fine. I will try with different line speeds and report back. >> --- >> drivers/net/ethernet/stmicro/stmmac/Kconfig | 6 ++-- >> .../ethernet/stmicro/stmmac/dwmac-starfive.c | 32 ++++++++++++++++--- >> 2 files changed, 31 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig >> index a2b9e289aa36..c3c2c8360047 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig >> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig >> @@ -165,9 +165,9 @@ config DWMAC_STARFIVE >> help >> Support for ethernet controllers on StarFive RISC-V SoCs >> >> - This selects the StarFive platform specific glue layer support for >> - the stmmac device driver. This driver is used for StarFive JH7110 >> - ethernet controller. >> + This selects the StarFive platform specific glue layer support >> + for the stmmac device driver. This driver is used for the >> + StarFive JH7100 and JH7110 ethernet controllers. >> >> config DWMAC_STI >> tristate "STi GMAC support" >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c >> index 5d630affb4d1..88c431edcea0 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c >> @@ -15,13 +15,20 @@ >> >> #include "stmmac_platform.h" >> >> -#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1 >> -#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4 >> -#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U >> +#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1 >> +#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4 >> +#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U >> + >> +#define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8 >> + >> +struct starfive_dwmac_data { >> + unsigned int gtxclk_dlychain; >> +}; >> >> struct starfive_dwmac { >> struct device *dev; >> struct clk *clk_tx; >> + const struct starfive_dwmac_data *data; >> }; >> >> static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode) >> @@ -67,6 +74,8 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat) >> >> case PHY_INTERFACE_MODE_RGMII: >> case PHY_INTERFACE_MODE_RGMII_ID: >> + case PHY_INTERFACE_MODE_RGMII_RXID: >> + case PHY_INTERFACE_MODE_RGMII_TXID: >> mode = STARFIVE_DWMAC_PHY_INFT_RGMII; >> break; >> >> @@ -89,6 +98,14 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat) >> if (err) >> return dev_err_probe(dwmac->dev, err, "error setting phy mode\n"); >> >> + if (dwmac->data) { > > I think you want something like this so future quirks don't need to touch this > code: > > if (dwmac->data && dwmac->data->gtxclk_dlychain) Yes, but that would prevent having a quirk that explicitly wants to write 0. I was initially thinking to something more generic, like providing a list of register-value pairs, but I'm not sure this is going to be ever needed. I'm still open to apply the suggested change, though. >> + err = regmap_write(regmap, JH7100_SYSMAIN_REGISTER49_DLYCHAIN, >> + dwmac->data->gtxclk_dlychain); >> + if (err) >> + return dev_err_probe(dwmac->dev, err, >> + "error selecting gtxclk delay chain\n"); >> + } >> + >> return 0; >> } >> >> @@ -114,6 +131,8 @@ static int starfive_dwmac_probe(struct platform_device *pdev) >> if (!dwmac) >> return -ENOMEM; >> >> + dwmac->data = device_get_match_data(&pdev->dev); >> + >> dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx"); >> if (IS_ERR(dwmac->clk_tx)) >> return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx), >> @@ -144,8 +163,13 @@ static int starfive_dwmac_probe(struct platform_device *pdev) >> return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res); >> } >> >> +static const struct starfive_dwmac_data jh7100_data = { >> + .gtxclk_dlychain = 4 > > Please add a , at the end of this line. I know it's unlikely that we need to > add more properties, but it's still good practice to do. This way such patches > won't need to touch this line. Sure, will do. >> +}; >> + >> static const struct of_device_id starfive_dwmac_match[] = { >> - { .compatible = "starfive,jh7110-dwmac" }, >> + { .compatible = "starfive,jh7100-dwmac", .data = &jh7100_data }, >> + { .compatible = "starfive,jh7110-dwmac" }, >> { /* sentinel */ } >> }; >> MODULE_DEVICE_TABLE(of, starfive_dwmac_match); >> -- >> 2.42.0 >>
On 10/31/23 16:38, Emil Renner Berthing wrote: > Cristian Ciocaltea wrote: >> Provide a DT node for the SiFive Composable Cache controller found on >> the StarFive JH7100 SoC. >> >> Note this is also used to support non-coherent DMA, via the >> sifive,cache-ops cache flushing operations. > > This property is no longer needed: > https://lore.kernel.org/linux-riscv/20231031141444.53426-1-emil.renner.berthing@canonical.com/ Thanks for the heads up! I actually noticed that from v1 reviews and was just waiting for v2. :) > Also it would be nice to mention that these nodes are copied from my > visionfive patches ;) Ups, sorry about that! Those were initially taken from a patch adding a full DT (the repo is mentioned in the cover letter) with many contributors mentioned, without being clear who did what. That's why I didn't provide a Co-developed-by tag and, unfortunately, I also missed to add it in v2 (will handle this in v3 and also provide the link to the new repo), but I'm still not sure about the gmac stuff. Thanks, Cristian >> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >> --- >> arch/riscv/boot/dts/starfive/jh7100.dtsi | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/arch/riscv/boot/dts/starfive/jh7100.dtsi b/arch/riscv/boot/dts/starfive/jh7100.dtsi >> index 06bb157ce111..a8a5bb00b0d8 100644 >> --- a/arch/riscv/boot/dts/starfive/jh7100.dtsi >> +++ b/arch/riscv/boot/dts/starfive/jh7100.dtsi >> @@ -32,6 +32,7 @@ U74_0: cpu@0 { >> i-tlb-sets = <1>; >> i-tlb-size = <32>; >> mmu-type = "riscv,sv39"; >> + next-level-cache = <&ccache>; >> riscv,isa = "rv64imafdc"; >> riscv,isa-base = "rv64i"; >> riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr", >> @@ -60,6 +61,7 @@ U74_1: cpu@1 { >> i-tlb-sets = <1>; >> i-tlb-size = <32>; >> mmu-type = "riscv,sv39"; >> + next-level-cache = <&ccache>; >> riscv,isa = "rv64imafdc"; >> riscv,isa-base = "rv64i"; >> riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr", >> @@ -147,6 +149,18 @@ soc { >> dma-noncoherent; >> ranges; >> >> + ccache: cache-controller@2010000 { >> + compatible = "starfive,jh7100-ccache", "sifive,ccache0", "cache"; >> + reg = <0x0 0x2010000 0x0 0x1000>; >> + interrupts = <128>, <130>, <131>, <129>; >> + cache-block-size = <64>; >> + cache-level = <2>; >> + cache-sets = <2048>; >> + cache-size = <2097152>; >> + cache-unified; >> + sifive,cache-ops; >> + }; >> + >> clint: clint@2000000 { >> compatible = "starfive,jh7100-clint", "sifive,clint0"; >> reg = <0x0 0x2000000 0x0 0x10000>; >> -- >> 2.42.0 >>
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 | 36 ++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/arch/riscv/boot/dts/starfive/jh7100.dtsi b/arch/riscv/boot/dts/starfive/jh7100.dtsi > index a8a5bb00b0d8..e8228e96d350 100644 > --- a/arch/riscv/boot/dts/starfive/jh7100.dtsi > +++ b/arch/riscv/boot/dts/starfive/jh7100.dtsi > @@ -179,6 +179,37 @@ plic: interrupt-controller@c000000 { > riscv,ndev = <133>; > }; > > + 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_TX_INV>, > + <&clkgen JH7100_CLK_GMAC_GTX>; > + clock-names = "stmmaceth", "pclk", "ptp_ref", "tx", "gtx"; > + resets = <&rstgen JH7100_RSTN_GMAC_AHB>; > + reset-names = "ahb"; > + interrupts = <6>, <7>; > + interrupt-names = "macirq", "eth_wake_irq"; > + max-frame-size = <9000>; > + snps,multicast-filter-bins = <32>; > + snps,perfect-filter-entries = <128>; > + starfive,syscon = <&sysmain 0x70 0>; > + rx-fifo-depth = <32768>; > + tx-fifo-depth = <16384>; > + snps,axi-config = <&stmmac_axi_setup>; > + snps,fixed-burst; > + snps,force_thresh_dma_mode; > + status = "disabled"; > + > + stmmac_axi_setup: stmmac-axi-config { > + snps,wr_osr_lmt = <0xf>; > + snps,rd_osr_lmt = <0xf>; As I also noted on the JH7110 patches these are not addresses or offsets but limits eg. counting things, which makes a lot more sense in decimal for most humans. But here you've changed them back to 0xf, why? > + snps,blen = <256 128 64 32 0 0 0>; > + }; > + }; > + > clkgen: clock-controller@11800000 { > compatible = "starfive,jh7100-clkgen"; > reg = <0x0 0x11800000 0x0 0x10000>; > @@ -193,6 +224,11 @@ rstgen: reset-controller@11840000 { > #reset-cells = <1>; > }; > > + sysmain: syscon@11850000 { > + compatible = "starfive,jh7100-sysmain", "syscon"; > + reg = <0x0 0x11850000 0x0 0x10000>; > + }; > + > i2c0: i2c@118b0000 { > compatible = "snps,designware-i2c"; > reg = <0x0 0x118b0000 0x0 0x10000>; > -- > 2.42.0 >
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 depends the non-coherent DMA support provided by Emil via the SiFive Composable Cache controller [2]. [1] https://github.com/starfive-tech/linux/commits/visionfive [2] https://lore.kernel.org/all/CAJM55Z_pdoGxRXbmBgJ5GbVWyeM1N6+LHihbNdT26Oo_qA5VYA@mail.gmail.com/ Changes in v2: - Dropped ccache PATCH 01-05 reworked by Emil via [2] - Dropped already applied PATCH 06/12 - Added PATCH v2 01 to prepare snps-dwmac binding for JH7100 support - Added PATCH v2 02-03 to provide some jh7110-dwmac binding optimizations - Handled JH7110 conflicting work in PATCH 07 via PATCH v2 04 - Reworked PATCH 8 via PATCH v2 05, adding JH7100 quirk and dropped - starfive,gtxclk-dlychain DT property, also fixed register naming - Added PATCH v2 08 providing DMA coherency related DT changes - Updated PATCH 9 commit msg: s/OF_DMA_DEFAULT_COHERENT/ARCH_DMA_DEFAULT_COHERENT/ - Replaced 'uncached-offset' property with 'sifive,cache-ops' in PATCH 10/12 and dropped 'sideband' reg - Add new patch providing coherent DMA memory pool (PATCH v2 10) - Updated PATCH 11/12 according to the stmmac glue layer changes in upstream - Split PATCH 12/12 into PATCH v2 10-12 to handle individual gmac setup of VisionFive v1 and BeagleV boards as they use different PHYs; also switched phy-mode from "rgmii-tx" to "rgmii-id" (requires a reduction of rx-internal-delay-ps by ~50%) - Rebased series onto next-20231024 - v1: https://lore.kernel.org/lkml/20230211031821.976408-1-cristian.ciocaltea@collabora.com/ Cristian Ciocaltea (11): dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset dt-bindings: net: starfive,jh7110-dwmac: Drop superfluous select dt-bindings: net: starfive,jh7110-dwmac: Drop redundant reset description dt-bindings: net: starfive,jh7110-dwmac: Add JH7100 SoC compatible net: stmmac: dwmac-starfive: Add support for JH7100 SoC riscv: dts: starfive: jh7100: Add dma-noncoherent property riscv: dts: starfive: jh7100: Add ccache DT node riscv: dts: starfive: jh7100: Add sysmain and gmac DT nodes riscv: dts: starfive: jh7100-common: Setup gmac pinmux riscv: dts: starfive: visionfive-v1: Enable gmac and setup phy [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac Emil Renner Berthing (1): riscv: dts: starfive: Add pool for coherent DMA memory on JH7100 boards .../devicetree/bindings/net/snps,dwmac.yaml | 3 +- .../bindings/net/starfive,jh7110-dwmac.yaml | 84 +++++++++------ .../dts/starfive/jh7100-beaglev-starlight.dts | 5 + .../boot/dts/starfive/jh7100-common.dtsi | 100 ++++++++++++++++++ .../jh7100-starfive-visionfive-v1.dts | 17 +++ arch/riscv/boot/dts/starfive/jh7100.dtsi | 51 +++++++++ drivers/net/ethernet/stmicro/stmmac/Kconfig | 6 +- .../ethernet/stmicro/stmmac/dwmac-starfive.c | 32 +++++- 8 files changed, 259 insertions(+), 39 deletions(-)