[PATCH v2 00/12] Enable networking support for StarFive JH7100 SoC

[PATCH v2 00/12] Enable networking support for StarFive JH7100 SoC

[Cristian Ciocaltea]

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(-)

-- 
2.42.0

[PATCH v2 01/12] dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset

[Cristian Ciocaltea]

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]
       - const: ahb
 
   power-domains:
-- 
2.42.0

Re: [PATCH v2 01/12] dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset

[Krzysztof Kozlowski]

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.

Best regards,
Krzysztof

Re: [PATCH v2 01/12] dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset

[Cristian Ciocaltea]

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";
- 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).

Thanks,
Cristian

Re: [PATCH v2 01/12] dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset

[Cristian Ciocaltea]

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).

Re: [PATCH v2 01/12] dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset

[Krzysztof Kozlowski]

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.

Best regards,
Krzysztof

Re: [PATCH v2 01/12] dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset

[Cristian Ciocaltea]

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.

Thanks,
Cristian

Re: [PATCH v2 01/12] dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset

[Krzysztof Kozlowski]

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

Re: [PATCH v2 01/12] dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset

[Cristian Ciocaltea]

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

Re: [PATCH v2 01/12] dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset

[Krzysztof Kozlowski]

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

Re: [PATCH v2 01/12] dt-bindings: net: snps,dwmac: Allow exclusive usage of ahb reset

[Cristian Ciocaltea]

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

[PATCH v2 02/12] dt-bindings: net: starfive,jh7110-dwmac: Drop superfluous select

[Cristian Ciocaltea]

The usage of 'select' doesn't seem to have any influence on how this
binding schema is applied to the nodes, hence remove it.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 .../devicetree/bindings/net/starfive,jh7110-dwmac.yaml   | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
index 5e7cfbbebce6..cc3e1c6fc135 100644
--- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
@@ -11,15 +11,6 @@ maintainers:
   - Emil Renner Berthing <kernel@esmil.dk>
   - Samin Guo <samin.guo@starfivetech.com>
 
-select:
-  properties:
-    compatible:
-      contains:
-        enum:
-          - starfive,jh7110-dwmac
-  required:
-    - compatible
-
 properties:
   compatible:
     items:
-- 
2.42.0

Re: [PATCH v2 02/12] dt-bindings: net: starfive,jh7110-dwmac: Drop superfluous select

[Krzysztof Kozlowski]

On 29/10/2023 05:27, Cristian Ciocaltea wrote:
> The usage of 'select' doesn't seem to have any influence on how this
> binding schema is applied to the nodes, hence remove it.
> 

It has. Why do you think it doesn't? You should see new errors from
dwmac schema.

Best regards,
Krzysztof

Re: [PATCH v2 02/12] dt-bindings: net: starfive,jh7110-dwmac: Drop superfluous select

[Cristian Ciocaltea]

On 10/29/23 13:18, Krzysztof Kozlowski wrote:
> On 29/10/2023 05:27, Cristian Ciocaltea wrote:
>> The usage of 'select' doesn't seem to have any influence on how this
>> binding schema is applied to the nodes, hence remove it.
>>
> 
> It has. Why do you think it doesn't? You should see new errors from
> dwmac schema.

This patch came as a result of testing both variants (w/ and w/o
'select') with several different compatible strings and seeing
consistent output:

- "starfive,jh7110-dwmac", "invalid";
- "starfive,jh7110-dwmac";
- "invalid", "snps,dwmac-5.20";
- "invalid"

Did I miss something?

Thanks for the review,
Cristian

Re: [PATCH v2 02/12] dt-bindings: net: starfive,jh7110-dwmac: Drop superfluous select

[Krzysztof Kozlowski]

On 29/10/2023 22:08, Cristian Ciocaltea wrote:
> On 10/29/23 13:18, Krzysztof Kozlowski wrote:
>> On 29/10/2023 05:27, Cristian Ciocaltea wrote:
>>> The usage of 'select' doesn't seem to have any influence on how this
>>> binding schema is applied to the nodes, hence remove it.
>>>
>>
>> It has. Why do you think it doesn't? You should see new errors from
>> dwmac schema.
> 
> This patch came as a result of testing both variants (w/ and w/o
> 'select') with several different compatible strings and seeing
> consistent output:
> 
> - "starfive,jh7110-dwmac", "invalid";
> - "starfive,jh7110-dwmac";
> - "invalid", "snps,dwmac-5.20";
> - "invalid"
> 
> Did I miss something?

Testing all bindings? The select is there to prevent matching unrelated
bindings.

Best regards,
Krzysztof

Re: [PATCH v2 02/12] dt-bindings: net: starfive,jh7110-dwmac: Drop superfluous select

[Cristian Ciocaltea]

On 10/30/23 09:27, Krzysztof Kozlowski wrote:
> On 29/10/2023 22:08, Cristian Ciocaltea wrote:
>> On 10/29/23 13:18, Krzysztof Kozlowski wrote:
>>> On 29/10/2023 05:27, Cristian Ciocaltea wrote:
>>>> The usage of 'select' doesn't seem to have any influence on how this
>>>> binding schema is applied to the nodes, hence remove it.
>>>>
>>>
>>> It has. Why do you think it doesn't? You should see new errors from
>>> dwmac schema.
>>
>> This patch came as a result of testing both variants (w/ and w/o
>> 'select') with several different compatible strings and seeing
>> consistent output:
>>
>> - "starfive,jh7110-dwmac", "invalid";
>> - "starfive,jh7110-dwmac";
>> - "invalid", "snps,dwmac-5.20";
>> - "invalid"
>>
>> Did I miss something?
> 
> Testing all bindings? The select is there to prevent matching unrelated
> bindings.

Indeed, my bad, as I've been using DT_SCHEMA_FILES to restrict the scope
during initial testing and missed to include later other schemas for an
extended validation (note that since [1] it's possible to specify a list
of file paths separated by ':').

Will drop this in v3.

[1] 25eba1598c8e ("dt-bindings: Fix multi pattern support in
DT_SCHEMA_FILES")

Thanks,
Cristian

[PATCH v2 03/12] dt-bindings: net: starfive,jh7110-dwmac: Drop redundant reset description

[Cristian Ciocaltea]

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
 
   reset-names:
     items:
-- 
2.42.0

Re: [PATCH v2 03/12] dt-bindings: net: starfive,jh7110-dwmac: Drop redundant reset description

[Krzysztof Kozlowski]

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.

Best regards,
Krzysztof

Re: [PATCH v2 03/12] dt-bindings: net: starfive,jh7110-dwmac: Drop redundant reset description

[Cristian Ciocaltea]

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

Thanks,
Cristian

Re: [PATCH v2 03/12] dt-bindings: net: starfive,jh7110-dwmac: Drop redundant reset description

[Krzysztof Kozlowski]

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

Re: [PATCH v2 03/12] dt-bindings: net: starfive,jh7110-dwmac: Drop redundant reset description

[Cristian Ciocaltea]

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

[PATCH v2 04/12] dt-bindings: net: starfive,jh7110-dwmac: Add JH7100 SoC compatible

[Cristian Ciocaltea]

The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
similar to the newer JH7110, but it requires only two interrupts and a
single reset line.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 .../devicetree/bindings/net/snps,dwmac.yaml   |  1 +
 .../bindings/net/starfive,jh7110-dwmac.yaml   | 74 +++++++++++++------
 2 files changed, 54 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
index a4d7172ea701..c1380ff1c054 100644
--- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
@@ -95,6 +95,7 @@ properties:
         - snps,dwmac-5.20
         - snps,dwxgmac
         - snps,dwxgmac-2.10
+        - starfive,jh7100-dwmac
         - starfive,jh7110-dwmac
 
   reg:
diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
index 44e58755a5a2..70e35a3401f4 100644
--- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
+++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
@@ -13,10 +13,14 @@ maintainers:
 
 properties:
   compatible:
-    items:
-      - enum:
-          - starfive,jh7110-dwmac
-      - const: snps,dwmac-5.20
+    oneOf:
+      - items:
+          - const: starfive,jh7100-dwmac
+          - const: snps,dwmac
+      - items:
+          - enum:
+              - starfive,jh7110-dwmac
+          - const: snps,dwmac-5.20
 
   reg:
     maxItems: 1
@@ -37,23 +41,6 @@ properties:
       - const: tx
       - const: gtx
 
-  interrupts:
-    minItems: 3
-    maxItems: 3
-
-  interrupt-names:
-    minItems: 3
-    maxItems: 3
-
-  resets:
-    minItems: 2
-    maxItems: 2
-
-  reset-names:
-    items:
-      - const: stmmaceth
-      - const: ahb
-
   starfive,tx-use-rgmii-clk:
     description:
       Tx clock is provided by external rgmii clock.
@@ -84,6 +71,51 @@ required:
 allOf:
   - $ref: snps,dwmac.yaml#
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: starfive,jh7100-dwmac
+    then:
+      properties:
+        interrupts:
+          minItems: 2
+          maxItems: 2
+
+        interrupt-names:
+          minItems: 2
+          maxItems: 2
+
+        resets:
+          maxItems: 1
+
+        reset-names:
+          const: ahb
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: starfive,jh7110-dwmac
+    then:
+      properties:
+        interrupts:
+          minItems: 3
+          maxItems: 3
+
+        interrupt-names:
+          minItems: 3
+          maxItems: 3
+
+        resets:
+          minItems: 2
+          maxItems: 2
+
+        reset-names:
+          items:
+            - const: stmmaceth
+            - const: ahb
+
 unevaluatedProperties: false
 
 examples:
-- 
2.42.0

Re: [PATCH v2 04/12] dt-bindings: net: starfive,jh7110-dwmac: Add JH7100 SoC compatible

[Krzysztof Kozlowski]

On 29/10/2023 05:27, Cristian Ciocaltea wrote:
> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
> similar to the newer JH7110, but it requires only two interrupts and a
> single reset line.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  .../devicetree/bindings/net/snps,dwmac.yaml   |  1 +
>  .../bindings/net/starfive,jh7110-dwmac.yaml   | 74 +++++++++++++------
>  2 files changed, 54 insertions(+), 21 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> index a4d7172ea701..c1380ff1c054 100644
> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
> @@ -95,6 +95,7 @@ properties:
>          - snps,dwmac-5.20
>          - snps,dwxgmac
>          - snps,dwxgmac-2.10
> +        - starfive,jh7100-dwmac
>          - starfive,jh7110-dwmac
>  
>    reg:
> diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
> index 44e58755a5a2..70e35a3401f4 100644
> --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
> +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
> @@ -13,10 +13,14 @@ maintainers:
>  
>  properties:
>    compatible:
> -    items:
> -      - enum:
> -          - starfive,jh7110-dwmac
> -      - const: snps,dwmac-5.20
> +    oneOf:
> +      - items:
> +          - const: starfive,jh7100-dwmac
> +          - const: snps,dwmac
> +      - items:
> +          - enum:
> +              - starfive,jh7110-dwmac
> +          - const: snps,dwmac-5.20

Why do you use different fallback?

>  
>    reg:
>      maxItems: 1
> @@ -37,23 +41,6 @@ properties:
>        - const: tx
>        - const: gtx
>  
> -  interrupts:
> -    minItems: 3
> -    maxItems: 3
> -
> -  interrupt-names:
> -    minItems: 3
> -    maxItems: 3
> -
> -  resets:
> -    minItems: 2
> -    maxItems: 2

You just changed it in previous patches... So the previous code allowing
one item was correct?


Best regards,
Krzysztof

Re: [PATCH v2 04/12] dt-bindings: net: starfive,jh7110-dwmac: Add JH7100 SoC compatible

[Cristian Ciocaltea]

On 10/29/23 13:24, Krzysztof Kozlowski wrote:
> On 29/10/2023 05:27, Cristian Ciocaltea wrote:
>> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
>> similar to the newer JH7110, but it requires only two interrupts and a
>> single reset line.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  .../devicetree/bindings/net/snps,dwmac.yaml   |  1 +
>>  .../bindings/net/starfive,jh7110-dwmac.yaml   | 74 +++++++++++++------
>>  2 files changed, 54 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>> index a4d7172ea701..c1380ff1c054 100644
>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>> @@ -95,6 +95,7 @@ properties:
>>          - snps,dwmac-5.20
>>          - snps,dwxgmac
>>          - snps,dwxgmac-2.10
>> +        - starfive,jh7100-dwmac
>>          - starfive,jh7110-dwmac
>>  
>>    reg:
>> diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>> index 44e58755a5a2..70e35a3401f4 100644
>> --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>> +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>> @@ -13,10 +13,14 @@ maintainers:
>>  
>>  properties:
>>    compatible:
>> -    items:
>> -      - enum:
>> -          - starfive,jh7110-dwmac
>> -      - const: snps,dwmac-5.20
>> +    oneOf:
>> +      - items:
>> +          - const: starfive,jh7100-dwmac
>> +          - const: snps,dwmac
>> +      - items:
>> +          - enum:
>> +              - starfive,jh7110-dwmac
>> +          - const: snps,dwmac-5.20
> 
> Why do you use different fallback?

AFAIK, dwmac-5.20 is currently only used by JH7110.

>>  
>>    reg:
>>      maxItems: 1
>> @@ -37,23 +41,6 @@ properties:
>>        - const: tx
>>        - const: gtx
>>  
>> -  interrupts:
>> -    minItems: 3
>> -    maxItems: 3
>> -
>> -  interrupt-names:
>> -    minItems: 3
>> -    maxItems: 3
>> -
>> -  resets:
>> -    minItems: 2
>> -    maxItems: 2
> 
> You just changed it in previous patches... So the previous code allowing
> one item was correct?

I mentioned the possible use-cases in the previous email. So yes, JH7110
requires 2 resets, while JH7100 just one.

Regards,
Cristian

Re: [PATCH v2 04/12] dt-bindings: net: starfive,jh7110-dwmac: Add JH7100 SoC compatible

[Krzysztof Kozlowski]

On 29/10/2023 23:15, Cristian Ciocaltea wrote:
> On 10/29/23 13:24, Krzysztof Kozlowski wrote:
>> On 29/10/2023 05:27, Cristian Ciocaltea wrote:
>>> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
>>> similar to the newer JH7110, but it requires only two interrupts and a
>>> single reset line.
>>>
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>> ---
>>>  .../devicetree/bindings/net/snps,dwmac.yaml   |  1 +
>>>  .../bindings/net/starfive,jh7110-dwmac.yaml   | 74 +++++++++++++------
>>>  2 files changed, 54 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> index a4d7172ea701..c1380ff1c054 100644
>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>> @@ -95,6 +95,7 @@ properties:
>>>          - snps,dwmac-5.20
>>>          - snps,dwxgmac
>>>          - snps,dwxgmac-2.10
>>> +        - starfive,jh7100-dwmac
>>>          - starfive,jh7110-dwmac
>>>  
>>>    reg:
>>> diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>> index 44e58755a5a2..70e35a3401f4 100644
>>> --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>> @@ -13,10 +13,14 @@ maintainers:
>>>  
>>>  properties:
>>>    compatible:
>>> -    items:
>>> -      - enum:
>>> -          - starfive,jh7110-dwmac
>>> -      - const: snps,dwmac-5.20
>>> +    oneOf:
>>> +      - items:
>>> +          - const: starfive,jh7100-dwmac
>>> +          - const: snps,dwmac
>>> +      - items:
>>> +          - enum:
>>> +              - starfive,jh7110-dwmac
>>> +          - const: snps,dwmac-5.20
>>
>> Why do you use different fallback?
> 
> AFAIK, dwmac-5.20 is currently only used by JH7110.

What is used by JH7000?

> 
>>>  
>>>    reg:
>>>      maxItems: 1
>>> @@ -37,23 +41,6 @@ properties:
>>>        - const: tx
>>>        - const: gtx
>>>  
>>> -  interrupts:
>>> -    minItems: 3
>>> -    maxItems: 3
>>> -
>>> -  interrupt-names:
>>> -    minItems: 3
>>> -    maxItems: 3
>>> -
>>> -  resets:
>>> -    minItems: 2
>>> -    maxItems: 2
>>
>> You just changed it in previous patches... So the previous code allowing
>> one item was correct?
> 
> I mentioned the possible use-cases in the previous email. So yes, JH7110
> requires 2 resets, while JH7100 just one.


Your previous patch does not make sense now...

Best regards,
Krzysztof

Re: [PATCH v2 04/12] dt-bindings: net: starfive,jh7110-dwmac: Add JH7100 SoC compatible

[Cristian Ciocaltea]

On 10/30/23 09:30, Krzysztof Kozlowski wrote:
> On 29/10/2023 23:15, Cristian Ciocaltea wrote:
>> On 10/29/23 13:24, Krzysztof Kozlowski wrote:
>>> On 29/10/2023 05:27, Cristian Ciocaltea wrote:
>>>> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
>>>> similar to the newer JH7110, but it requires only two interrupts and a
>>>> single reset line.
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>>  .../devicetree/bindings/net/snps,dwmac.yaml   |  1 +
>>>>  .../bindings/net/starfive,jh7110-dwmac.yaml   | 74 +++++++++++++------
>>>>  2 files changed, 54 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>>> index a4d7172ea701..c1380ff1c054 100644
>>>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml
>>>> @@ -95,6 +95,7 @@ properties:
>>>>          - snps,dwmac-5.20
>>>>          - snps,dwxgmac
>>>>          - snps,dwxgmac-2.10
>>>> +        - starfive,jh7100-dwmac
>>>>          - starfive,jh7110-dwmac
>>>>  
>>>>    reg:
>>>> diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>>> index 44e58755a5a2..70e35a3401f4 100644
>>>> --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>>> @@ -13,10 +13,14 @@ maintainers:
>>>>  
>>>>  properties:
>>>>    compatible:
>>>> -    items:
>>>> -      - enum:
>>>> -          - starfive,jh7110-dwmac
>>>> -      - const: snps,dwmac-5.20
>>>> +    oneOf:
>>>> +      - items:
>>>> +          - const: starfive,jh7100-dwmac
>>>> +          - const: snps,dwmac
>>>> +      - items:
>>>> +          - enum:
>>>> +              - starfive,jh7110-dwmac
>>>> +          - const: snps,dwmac-5.20
>>>
>>> Why do you use different fallback?
>>
>> AFAIK, dwmac-5.20 is currently only used by JH7110.
> 
> What is used by JH7000?

Driver reports "Synopsys ID: 0x37", so it could be 3.70a or 3.710, as
those are the only compatibles available for 3.7x.

It's worth noting the driver does not rely on the compatibles for
implementing version specific logic, as it gets the IDs directly from
chip registers.

The usage of generic snps,dwmac fallback was borrowed from downstream code.

Regards,
Cristian

Re: [PATCH v2 04/12] dt-bindings: net: starfive,jh7110-dwmac: Add JH7100 SoC compatible

[Rob Herring]

On Sun, 29 Oct 2023 06:27:04 +0200, Cristian Ciocaltea wrote:
> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
> similar to the newer JH7110, but it requires only two interrupts and a
> single reset line.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  .../devicetree/bindings/net/snps,dwmac.yaml   |  1 +
>  .../bindings/net/starfive,jh7110-dwmac.yaml   | 74 +++++++++++++------
>  2 files changed, 54 insertions(+), 21 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: compatible: 'oneOf' conditional failed, one must be fixed:
	'starfive,jh7100-dwmac' was expected
	'amlogic,meson-gxbb-dwmac' is not one of ['starfive,jh7110-dwmac']
	'snps,dwmac-5.20' was expected
	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: reg: [[3376480256, 65536], [3364046144, 8]] is too long
	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: clocks: [[4294967295], [4294967295], [4294967295], [4294967295]] is too short
	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: clock-names:1: 'pclk' was expected
	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: clock-names:2: 'ptp_ref' was expected
	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: clock-names:3: 'tx' was expected
	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: clock-names: ['stmmaceth', 'clkin0', 'clkin1', 'timing-adjustment'] is too short
	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: 'resets' is a required property
	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: 'reset-names' is a required property
	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231029042712.520010-5-cristian.ciocaltea@collabora.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 04/12] dt-bindings: net: starfive,jh7110-dwmac: Add JH7100 SoC compatible

[Krzysztof Kozlowski]

On 30/10/2023 02:37, Rob Herring wrote:
> 
> On Sun, 29 Oct 2023 06:27:04 +0200, Cristian Ciocaltea wrote:
>> The Synopsys DesignWare MAC found on StarFive JH7100 SoC is quite
>> similar to the newer JH7110, but it requires only two interrupts and a
>> single reset line.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  .../devicetree/bindings/net/snps,dwmac.yaml   |  1 +
>>  .../bindings/net/starfive,jh7110-dwmac.yaml   | 74 +++++++++++++------
>>  2 files changed, 54 insertions(+), 21 deletions(-)
>>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/amlogic,meson-dwmac.example.dtb: ethernet@c9410000: compatible: 'oneOf' conditional failed, one must be fixed:
> 	'starfive,jh7100-dwmac' was expected
> 	'amlogic,meson-gxbb-dwmac' is not one of ['starfive,jh7110-dwmac']
> 	'snps,dwmac-5.20' was expected
> 	from schema $id: http://devicetree.org/schemas/net/starfive,jh7110-dwmac.yaml#

And here you have proof of missing testing of patch #2.

Best regards,
Krzysztof

[PATCH v2 05/12] net: stmmac: dwmac-starfive: Add support for JH7100 SoC

[Cristian Ciocaltea]

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>
---
 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) {
+		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
+};
+
 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

Re: [PATCH v2 05/12] net: stmmac: dwmac-starfive: Add support for JH7100 SoC

[Emil Renner Berthing]

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..

> ---
>  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)

> +		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.

> +};
> +
>  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
>

Re: [PATCH v2 05/12] net: stmmac: dwmac-starfive: Add support for JH7100 SoC

[Cristian Ciocaltea]

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
>>

[PATCH v2 06/12] riscv: dts: starfive: jh7100: Add dma-noncoherent property

[Cristian Ciocaltea]

The RISC-V architecture is by default coherent, since it selects
ARCH_DMA_DEFAULT_COHERENT, but the StarFive JH7100 is not, hence provide
the dma-noncoherent property to the soc DT node.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 arch/riscv/boot/dts/starfive/jh7100.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7100.dtsi b/arch/riscv/boot/dts/starfive/jh7100.dtsi
index e68cafe7545f..06bb157ce111 100644
--- a/arch/riscv/boot/dts/starfive/jh7100.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7100.dtsi
@@ -144,6 +144,7 @@ soc {
 		interrupt-parent = <&plic>;
 		#address-cells = <2>;
 		#size-cells = <2>;
+		dma-noncoherent;
 		ranges;
 
 		clint: clint@2000000 {
-- 
2.42.0

[PATCH v2 07/12] riscv: dts: starfive: jh7100: Add ccache DT node

[Cristian Ciocaltea]

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.

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

Re: [PATCH v2 07/12] riscv: dts: starfive: jh7100: Add ccache DT node

[Emil Renner Berthing]

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
>

Re: [PATCH v2 07/12] riscv: dts: starfive: jh7100: Add ccache DT node

[Cristian Ciocaltea]

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
>>

[PATCH v2 08/12] riscv: dts: starfive: Add pool for coherent DMA memory on JH7100 boards

[Cristian Ciocaltea]

From: Emil Renner Berthing <emil.renner.berthing@canonical.com>

The StarFive JH7100 SoC has non-coherent device DMAs, but most drivers
expect to be able to allocate coherent memory for DMA descriptors and
such. However on the JH7100 DDR memory appears twice in the physical
memory map, once cached and once uncached:

  0x00_8000_0000 - 0x08_7fff_ffff : Off chip DDR memory, cached
  0x10_0000_0000 - 0x17_ffff_ffff : Off chip DDR memory, uncached

To use this uncached region we create a global DMA memory pool there and
reserve the corresponding area in the cached region.

However the uncached region is fully above the 32bit address limit, so add
a dma-ranges map so the DMA address used for peripherals is still in the
regular cached region below the limit.

Link: https://github.com/starfive-tech/JH7100_Docs/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 .../boot/dts/starfive/jh7100-common.dtsi      | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
index b93ce351a90f..504c73f01f14 100644
--- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
@@ -39,6 +39,30 @@ led-ack {
 			label = "ack";
 		};
 	};
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		dma-reserved {
+			reg = <0x0 0xfa000000 0x0 0x1000000>;
+			no-map;
+		};
+
+		linux,dma {
+			compatible = "shared-dma-pool";
+			reg = <0x10 0x7a000000 0x0 0x1000000>;
+			no-map;
+			linux,dma-default;
+		};
+	};
+
+	soc {
+		dma-ranges = <0x00 0x80000000 0x00 0x80000000 0x00 0x7a000000>,
+			     <0x00 0xfa000000 0x10 0x7a000000 0x00 0x01000000>,
+			     <0x00 0xfb000000 0x00 0xfb000000 0x07 0x85000000>;
+	};
 };
 
 &gpio {
-- 
2.42.0

Re: [PATCH v2 08/12] riscv: dts: starfive: Add pool for coherent DMA memory on JH7100 boards

[Andrew Lunn]

On Sun, Oct 29, 2023 at 06:27:08AM +0200, Cristian Ciocaltea wrote:
> From: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> 
> The StarFive JH7100 SoC has non-coherent device DMAs, but most drivers
> expect to be able to allocate coherent memory for DMA descriptors and
> such. However on the JH7100 DDR memory appears twice in the physical
> memory map, once cached and once uncached:
> 
>   0x00_8000_0000 - 0x08_7fff_ffff : Off chip DDR memory, cached
>   0x10_0000_0000 - 0x17_ffff_ffff : Off chip DDR memory, uncached
> 
> To use this uncached region we create a global DMA memory pool there and
> reserve the corresponding area in the cached region.
> 
> However the uncached region is fully above the 32bit address limit, so add
> a dma-ranges map so the DMA address used for peripherals is still in the
> regular cached region below the limit.
> 
> Link: https://github.com/starfive-tech/JH7100_Docs/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
> Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  .../boot/dts/starfive/jh7100-common.dtsi      | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> index b93ce351a90f..504c73f01f14 100644
> --- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> @@ -39,6 +39,30 @@ led-ack {
>  			label = "ack";
>  		};
>  	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		dma-reserved {
> +			reg = <0x0 0xfa000000 0x0 0x1000000>;

If i'm reading this correctly, this is at the top of the first 4G of
RAM. But this is jh7100-common.dtsi. Is it guaranteed that all boards
derived from this have at least 4G? What happens is a board only has
2G?

It might also be worth putting a comment here about the memory being
mapped twice. In the ARM world that would be illegal, so its maybe not
seen that often. Yes, the commit message explains that, but when i
look at the code on its own, it is less obvious.

> +			no-map;
> +		};
> +
> +		linux,dma {
> +			compatible = "shared-dma-pool";
> +			reg = <0x10 0x7a000000 0x0 0x1000000>;
> +			no-map;
> +			linux,dma-default;
> +		};
> +	};

Re: [PATCH v2 08/12] riscv: dts: starfive: Add pool for coherent DMA memory on JH7100 boards

[Emil Renner Berthing]

Andrew Lunn wrote:
> On Sun, Oct 29, 2023 at 06:27:08AM +0200, Cristian Ciocaltea wrote:
> > From: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> >
> > The StarFive JH7100 SoC has non-coherent device DMAs, but most drivers
> > expect to be able to allocate coherent memory for DMA descriptors and
> > such. However on the JH7100 DDR memory appears twice in the physical
> > memory map, once cached and once uncached:
> >
> >   0x00_8000_0000 - 0x08_7fff_ffff : Off chip DDR memory, cached
> >   0x10_0000_0000 - 0x17_ffff_ffff : Off chip DDR memory, uncached
> >
> > To use this uncached region we create a global DMA memory pool there and
> > reserve the corresponding area in the cached region.
> >
> > However the uncached region is fully above the 32bit address limit, so add
> > a dma-ranges map so the DMA address used for peripherals is still in the
> > regular cached region below the limit.
> >
> > Link: https://github.com/starfive-tech/JH7100_Docs/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
> > Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> > ---
> >  .../boot/dts/starfive/jh7100-common.dtsi      | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> > index b93ce351a90f..504c73f01f14 100644
> > --- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> > +++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> > @@ -39,6 +39,30 @@ led-ack {
> >  			label = "ack";
> >  		};
> >  	};
> > +
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		dma-reserved {
> > +			reg = <0x0 0xfa000000 0x0 0x1000000>;
>
> If i'm reading this correctly, this is at the top of the first 4G of
> RAM. But this is jh7100-common.dtsi. Is it guaranteed that all boards
> derived from this have at least 4G? What happens is a board only has
> 2G?

Yes, both the BeagleV Starlight and StarFive VisionFive V1 boards have at least
4G of ram and there won't be any more boards with this SoC. It was a test chip
for the JH7110 after all.

There aren't really any limitations on where this pool could be placed, so I
just chose to wedge it between ranges reserved for graphics by the bootloader.
If anyone has a better idea please go ahead and change it.

>
> It might also be worth putting a comment here about the memory being
> mapped twice. In the ARM world that would be illegal, so its maybe not
> seen that often. Yes, the commit message explains that, but when i
> look at the code on its own, it is less obvious.
>
> > +			no-map;
> > +		};
> > +
> > +		linux,dma {
> > +			compatible = "shared-dma-pool";
> > +			reg = <0x10 0x7a000000 0x0 0x1000000>;
> > +			no-map;
> > +			linux,dma-default;
> > +		};
> > +	};

Re: [PATCH v2 08/12] riscv: dts: starfive: Add pool for coherent DMA memory on JH7100 boards

[Emil Renner Berthing]

Cristian Ciocaltea wrote:
> From: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>
> The StarFive JH7100 SoC has non-coherent device DMAs, but most drivers
> expect to be able to allocate coherent memory for DMA descriptors and
> such. However on the JH7100 DDR memory appears twice in the physical
> memory map, once cached and once uncached:
>
>   0x00_8000_0000 - 0x08_7fff_ffff : Off chip DDR memory, cached
>   0x10_0000_0000 - 0x17_ffff_ffff : Off chip DDR memory, uncached
>
> To use this uncached region we create a global DMA memory pool there and
> reserve the corresponding area in the cached region.
>
> However the uncached region is fully above the 32bit address limit, so add
> a dma-ranges map so the DMA address used for peripherals is still in the
> regular cached region below the limit.

Adding these nodes to the device tree won't actually do anything without
enabling CONFIG_DMA_GLOBAL_POOL as is done here:

https://github.com/esmil/linux/commit/e14ad9ff67fd51dcc76415d4cc7f3a30ffcba379

>
> Link: https://github.com/starfive-tech/JH7100_Docs/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
> Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  .../boot/dts/starfive/jh7100-common.dtsi      | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> index b93ce351a90f..504c73f01f14 100644
> --- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> @@ -39,6 +39,30 @@ led-ack {
>  			label = "ack";
>  		};
>  	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		dma-reserved {
> +			reg = <0x0 0xfa000000 0x0 0x1000000>;
> +			no-map;
> +		};
> +
> +		linux,dma {
> +			compatible = "shared-dma-pool";
> +			reg = <0x10 0x7a000000 0x0 0x1000000>;
> +			no-map;
> +			linux,dma-default;
> +		};
> +	};
> +
> +	soc {
> +		dma-ranges = <0x00 0x80000000 0x00 0x80000000 0x00 0x7a000000>,
> +			     <0x00 0xfa000000 0x10 0x7a000000 0x00 0x01000000>,
> +			     <0x00 0xfb000000 0x00 0xfb000000 0x07 0x85000000>;
> +	};
>  };
>
>  &gpio {
> --
> 2.42.0
>

Re: [PATCH v2 08/12] riscv: dts: starfive: Add pool for coherent DMA memory on JH7100 boards

[Cristian Ciocaltea]

On 10/31/23 16:40, Emil Renner Berthing wrote:
> Cristian Ciocaltea wrote:
>> From: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>>
>> The StarFive JH7100 SoC has non-coherent device DMAs, but most drivers
>> expect to be able to allocate coherent memory for DMA descriptors and
>> such. However on the JH7100 DDR memory appears twice in the physical
>> memory map, once cached and once uncached:
>>
>>   0x00_8000_0000 - 0x08_7fff_ffff : Off chip DDR memory, cached
>>   0x10_0000_0000 - 0x17_ffff_ffff : Off chip DDR memory, uncached
>>
>> To use this uncached region we create a global DMA memory pool there and
>> reserve the corresponding area in the cached region.
>>
>> However the uncached region is fully above the 32bit address limit, so add
>> a dma-ranges map so the DMA address used for peripherals is still in the
>> regular cached region below the limit.
> 
> Adding these nodes to the device tree won't actually do anything without
> enabling CONFIG_DMA_GLOBAL_POOL as is done here:
> 
> https://github.com/esmil/linux/commit/e14ad9ff67fd51dcc76415d4cc7f3a30ffcba379

Should I pick this up for v3 or maybe it would be better to be handled
as part of your ccache series?

Thanks,
Cristian

>>
>> Link: https://github.com/starfive-tech/JH7100_Docs/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
>> Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  .../boot/dts/starfive/jh7100-common.dtsi      | 24 +++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
>> index b93ce351a90f..504c73f01f14 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
>> @@ -39,6 +39,30 @@ led-ack {
>>  			label = "ack";
>>  		};
>>  	};
>> +
>> +	reserved-memory {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +
>> +		dma-reserved {
>> +			reg = <0x0 0xfa000000 0x0 0x1000000>;
>> +			no-map;
>> +		};
>> +
>> +		linux,dma {
>> +			compatible = "shared-dma-pool";
>> +			reg = <0x10 0x7a000000 0x0 0x1000000>;
>> +			no-map;
>> +			linux,dma-default;
>> +		};
>> +	};
>> +
>> +	soc {
>> +		dma-ranges = <0x00 0x80000000 0x00 0x80000000 0x00 0x7a000000>,
>> +			     <0x00 0xfa000000 0x10 0x7a000000 0x00 0x01000000>,
>> +			     <0x00 0xfb000000 0x00 0xfb000000 0x07 0x85000000>;
>> +	};
>>  };
>>
>>  &gpio {
>> --
>> 2.42.0
>>

[PATCH v2 09/12] riscv: dts: starfive: jh7100: Add sysmain and gmac DT nodes

[Cristian Ciocaltea]

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>;
+				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

Re: [PATCH v2 09/12] riscv: dts: starfive: jh7100: Add sysmain and gmac DT nodes

[Emil Renner Berthing]

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
>

Re: [PATCH v2 09/12] riscv: dts: starfive: jh7100: Add sysmain and gmac DT nodes

[Cristian Ciocaltea]

On 11/26/23 23:15, Emil Renner Berthing wrote:
> 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?

That's a left over from v1. Will fix, thanks!

>> +				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
>>

[PATCH v2 10/12] riscv: dts: starfive: jh7100-common: Setup gmac pinmux

[Cristian Ciocaltea]

Add pinmux configuration for the DWMAC found on the JH7100 based boards.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 .../boot/dts/starfive/jh7100-common.dtsi      | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
index 504c73f01f14..b556e28069c4 100644
--- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
@@ -65,7 +65,83 @@ soc {
 	};
 };
 
+&gmac {
+	pinctrl-names = "default";
+	pinctrl-0 = <&gmac_pins>;
+};
+
 &gpio {
+	gmac_pins: gmac-0 {
+		gtxclk-pins {
+			pins = <PAD_FUNC_SHARE(115)>;
+			bias-pull-up;
+			drive-strength = <35>;
+			input-enable;
+			input-schmitt-enable;
+			slew-rate = <0>;
+		};
+		miitxclk-pins {
+			pins = <PAD_FUNC_SHARE(116)>;
+			bias-pull-up;
+			drive-strength = <14>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+		tx-pins {
+			pins = <PAD_FUNC_SHARE(117)>,
+			       <PAD_FUNC_SHARE(119)>,
+			       <PAD_FUNC_SHARE(120)>,
+			       <PAD_FUNC_SHARE(121)>,
+			       <PAD_FUNC_SHARE(122)>,
+			       <PAD_FUNC_SHARE(123)>,
+			       <PAD_FUNC_SHARE(124)>,
+			       <PAD_FUNC_SHARE(125)>,
+			       <PAD_FUNC_SHARE(126)>;
+			bias-pull-up;
+			drive-strength = <35>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+		rxclk-pins {
+			pins = <PAD_FUNC_SHARE(127)>;
+			bias-pull-up;
+			drive-strength = <14>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <6>;
+		};
+		rxer-pins {
+			pins = <PAD_FUNC_SHARE(129)>;
+			bias-pull-up;
+			drive-strength = <14>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+		rx-pins {
+			pins = <PAD_FUNC_SHARE(128)>,
+			       <PAD_FUNC_SHARE(130)>,
+			       <PAD_FUNC_SHARE(131)>,
+			       <PAD_FUNC_SHARE(132)>,
+			       <PAD_FUNC_SHARE(133)>,
+			       <PAD_FUNC_SHARE(134)>,
+			       <PAD_FUNC_SHARE(135)>,
+			       <PAD_FUNC_SHARE(136)>,
+			       <PAD_FUNC_SHARE(137)>,
+			       <PAD_FUNC_SHARE(138)>,
+			       <PAD_FUNC_SHARE(139)>,
+			       <PAD_FUNC_SHARE(140)>,
+			       <PAD_FUNC_SHARE(141)>;
+			bias-pull-up;
+			drive-strength = <14>;
+			input-enable;
+			input-schmitt-enable;
+			slew-rate = <0>;
+		};
+	};
+
 	i2c0_pins: i2c0-0 {
 		i2c-pins {
 			pinmux = <GPIOMUX(62, GPO_LOW,
-- 
2.42.0

[PATCH v2 11/12] riscv: dts: starfive: visionfive-v1: Enable gmac and setup phy

[Cristian Ciocaltea]

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.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 .../starfive/jh7100-starfive-visionfive-v1.dts  | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7100-starfive-visionfive-v1.dts b/arch/riscv/boot/dts/starfive/jh7100-starfive-visionfive-v1.dts
index e82af72f1aaf..e043a27f7612 100644
--- a/arch/riscv/boot/dts/starfive/jh7100-starfive-visionfive-v1.dts
+++ b/arch/riscv/boot/dts/starfive/jh7100-starfive-visionfive-v1.dts
@@ -18,3 +18,20 @@ gpio-restart {
 		priority = <224>;
 	};
 };
+
+&gmac {
+	phy-handle = <&phy>;
+	phy-mode = "rgmii-id";
+	status = "okay";
+
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "snps,dwmac-mdio";
+
+		phy: ethernet-phy@0 {
+			reg = <0>;
+			rx-internal-delay-ps = <900>;
+		};
+	};
+};
-- 
2.42.0

Re: [PATCH v2 11/12] riscv: dts: starfive: visionfive-v1: Enable gmac and setup phy

[Andrew Lunn]

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.

    Andrew

Re: [PATCH v2 11/12] riscv: dts: starfive: visionfive-v1: Enable gmac and setup phy

[Cristian Ciocaltea]

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

Re: [PATCH v2 11/12] riscv: dts: starfive: visionfive-v1: Enable gmac and setup phy

[Andrew Lunn]

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.

	Andrew

Re: [PATCH v2 11/12] riscv: dts: starfive: visionfive-v1: Enable gmac and setup phy

[Cristian Ciocaltea]

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

[PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
RGMII-ID.

TODO: Verify if manual adjustment of the RX internal delay is needed. If
yes, add the mdio & phy sub-nodes.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
index 7cda3a89020a..d3f4c99d98da 100644
--- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
+++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
@@ -11,3 +11,8 @@ / {
 	model = "BeagleV Starlight Beta";
 	compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100";
 };
+
+&gmac {
+	phy-mode = "rgmii-id";
+	status = "okay";
+};
-- 
2.42.0

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Andrew Lunn]

On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote:
> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
> RGMII-ID.
> 
> TODO: Verify if manual adjustment of the RX internal delay is needed. If
> yes, add the mdio & phy sub-nodes.

Please could you try to get this tested. It might shed some light on
what is going on here, since it is a different PHY.

	Andrew

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 10/29/23 20:46, Andrew Lunn wrote:
> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote:
>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>> RGMII-ID.
>>
>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
>> yes, add the mdio & phy sub-nodes.
> 
> Please could you try to get this tested. It might shed some light on
> what is going on here, since it is a different PHY.

Actually, this is the main reason I added the patch. I don't have access
to this board, so it would be great if we could get some help with testing.

Thanks,
Cristian

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 10/30/23 00:53, Cristian Ciocaltea wrote:
> On 10/29/23 20:46, Andrew Lunn wrote:
>> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote:
>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>>> RGMII-ID.
>>>
>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
>>> yes, add the mdio & phy sub-nodes.
>>
>> Please could you try to get this tested. It might shed some light on
>> what is going on here, since it is a different PHY.
> 
> Actually, this is the main reason I added the patch. I don't have access
> to this board, so it would be great if we could get some help with testing.

@Emil, @Conor: Any idea who might help us with a quick test on the
BeagleV Starlight board?

Thanks,
Cristian

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

On Thu, Nov 16, 2023 at 03:15:46PM +0200, Cristian Ciocaltea wrote:
> On 10/30/23 00:53, Cristian Ciocaltea wrote:
> > On 10/29/23 20:46, Andrew Lunn wrote:
> >> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote:
> >>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
> >>> RGMII-ID.
> >>>
> >>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
> >>> yes, add the mdio & phy sub-nodes.
> >>
> >> Please could you try to get this tested. It might shed some light on
> >> what is going on here, since it is a different PHY.
> > 
> > Actually, this is the main reason I added the patch. I don't have access
> > to this board, so it would be great if we could get some help with testing.
> 
> @Emil, @Conor: Any idea who might help us with a quick test on the
> BeagleV Starlight board?

I don't have one & I am not sure if Emil does. Geert (CCed) should have
one though. Is there a specific test you need to have done?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 11/16/23 19:55, Conor Dooley wrote:
> On Thu, Nov 16, 2023 at 03:15:46PM +0200, Cristian Ciocaltea wrote:
>> On 10/30/23 00:53, Cristian Ciocaltea wrote:
>>> On 10/29/23 20:46, Andrew Lunn wrote:
>>>> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote:
>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>>>>> RGMII-ID.
>>>>>
>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
>>>>> yes, add the mdio & phy sub-nodes.
>>>>
>>>> Please could you try to get this tested. It might shed some light on
>>>> what is going on here, since it is a different PHY.
>>>
>>> Actually, this is the main reason I added the patch. I don't have access
>>> to this board, so it would be great if we could get some help with testing.
>>
>> @Emil, @Conor: Any idea who might help us with a quick test on the
>> BeagleV Starlight board?
> 
> I don't have one & I am not sure if Emil does. Geert (CCed) should have
> one though. Is there a specific test you need to have done?

As Andrew already pointed out, we'd like to know if networking for this
board works without any further adjustment of the RX internal delay.

This was necessary for VisionFive (see previous PATCH v2 11/12), but the
PHY is different (Motorcomm YT8521), hence this test might help us
understand if there's a potential issue with the SoC or the PHY.

Thanks,
Cristian

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Geert Uytterhoeven]

On Thu, Nov 16, 2023 at 6:55 PM Conor Dooley <conor@kernel.org> wrote:
> On Thu, Nov 16, 2023 at 03:15:46PM +0200, Cristian Ciocaltea wrote:
> > On 10/30/23 00:53, Cristian Ciocaltea wrote:
> > > On 10/29/23 20:46, Andrew Lunn wrote:
> > >> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote:
> > >>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
> > >>> RGMII-ID.
> > >>>
> > >>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
> > >>> yes, add the mdio & phy sub-nodes.
> > >>
> > >> Please could you try to get this tested. It might shed some light on
> > >> what is going on here, since it is a different PHY.
> > >
> > > Actually, this is the main reason I added the patch. I don't have access
> > > to this board, so it would be great if we could get some help with testing.
> >
> > @Emil, @Conor: Any idea who might help us with a quick test on the
> > BeagleV Starlight board?
>
> I don't have one & I am not sure if Emil does. Geert (CCed) should have

I believe Esmil has.

> one though. Is there a specific test you need to have done?

I gave it a try, on top of latest renesas-drivers[1].

------------[ cut here ]------------
simple-pm-bus soc: device non-coherent but no non-coherent operations supported
WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
arch_setup_dma_ops+0x7e/0xa2
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted
6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
Hardware name: BeagleV Starlight Beta (DT)
epc : arch_setup_dma_ops+0x7e/0xa2
 ra : arch_setup_dma_ops+0x7e/0xa2
epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b90
 gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81217238
 t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013bc0
 s1 : 0000000000000000 a0 : 000000000000004f a1 : 0000000200000020
 a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
 a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
 s2 : ffffffd880111410 s3 : ffffffff812d712c s4 : 00000000ffffffff
 s5 : ffffffffffffffed s6 : ffffffd9fffeb530 s7 : ffffffff80a00d70
 s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
 s11: 0000000000000000 t3 : ffffffff812ebb5c t4 : ffffffff812ebb5c
 t5 : ffffffff812ebb38 t6 : ffffffff812ebbb7
status: 0000000200000120 badaddr: ffffffff812d712c cause: 0000000000000003
[<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
[<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
[<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
[<ffffffff803ec382>] really_probe+0x54/0x20c
[<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
[<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
[<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
[<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
[<ffffffff803ebdc8>] driver_attach+0x1a/0x22
[<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
[<ffffffff803ed476>] driver_register+0x3e/0xd8
[<ffffffff803ee590>] __platform_driver_register+0x1c/0x24
[<ffffffff8081db32>] simple_pm_bus_driver_init+0x1a/0x22
[<ffffffff800020f0>] do_one_initcall+0x38/0x17c
[<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
[<ffffffff806b1328>] kernel_init+0x1e/0x112
[<ffffffff806b891e>] ret_from_fork+0xe/0x1c
---[ end trace 0000000000000000 ]---
------------[ cut here ]------------
clk-starfive-jh7100 11800000.clock-controller: device non-coherent but
no non-coherent operations supported
WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
arch_setup_dma_ops+0x7e/0xa2
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S
6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
Hardware name: BeagleV Starlight Beta (DT)
epc : arch_setup_dma_ops+0x7e/0xa2
 ra : arch_setup_dma_ops+0x7e/0xa2
epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b70
 gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81217f48
 t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013ba0
 s1 : 0000000000000000 a0 : 000000000000006b a1 : 0000000200000020
 a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
 a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
 s2 : ffffffd880112410 s3 : ffffffff812d712c s4 : 0000000fffffffff
 s5 : 0000000000000000 s6 : ffffffd9fffed3c8 s7 : ffffffff80a00d70
 s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
 s11: 0000000000000000 t3 : ffffffff812ec564 t4 : ffffffff812ec564
 t5 : ffffffff812ec540 t6 : ffffffff812ec5db
status: 0000000200000120 badaddr: ffffffd880130008 cause: 0000000000000003
[<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
[<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
[<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
[<ffffffff803ec382>] really_probe+0x54/0x20c
[<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
[<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
[<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
[<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
[<ffffffff803ebdc8>] driver_attach+0x1a/0x22
[<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
[<ffffffff803ed476>] driver_register+0x3e/0xd8
[<ffffffff803ee6ce>] __platform_driver_probe+0x40/0x9c
[<ffffffff8081eb80>] clk_starfive_jh7100_driver_init+0x22/0x2a
[<ffffffff800020f0>] do_one_initcall+0x38/0x17c
[<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
[<ffffffff806b1328>] kernel_init+0x1e/0x112
[<ffffffff806b891e>] ret_from_fork+0xe/0x1c
---[ end trace 0000000000000000 ]---
CCACHE: DataFail @ 0x00000000.7FEB8930
CCACHE: 2 banks, 16 ways, sets/bank=1024, bytes/block=64
CCACHE: Index of the largest way enabled: 0
------------[ cut here ]------------
jh7100-reset 11840000.reset-controller: device non-coherent but no
non-coherent operations supported
WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
arch_setup_dma_ops+0x7e/0xa2
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S
6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
Hardware name: BeagleV Starlight Beta (DT)
epc : arch_setup_dma_ops+0x7e/0xa2
 ra : arch_setup_dma_ops+0x7e/0xa2
epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b70
 gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81218d60
 t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013ba0
 s1 : 0000000000000000 a0 : 0000000000000064 a1 : 0000000200000020
 a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
 a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
 s2 : ffffffd880112c10 s3 : ffffffff812d712c s4 : 0000000fffffffff
 s5 : 0000000000000000 s6 : ffffffd9fffed7a8 s7 : ffffffff80a00d70
 s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
 s11: 0000000000000000 t3 : ffffffff812ed054 t4 : ffffffff812ed054
 t5 : ffffffff812ed030 t6 : ffffffff812ed0c4
status: 0000000200000120 badaddr: ffffffd880130008 cause: 0000000000000003
[<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
[<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
[<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
[<ffffffff803ec382>] really_probe+0x54/0x20c
[<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
[<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
[<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
[<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
[<ffffffff803ebdc8>] driver_attach+0x1a/0x22
[<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
[<ffffffff803ed476>] driver_register+0x3e/0xd8
[<ffffffff803ee6ce>] __platform_driver_probe+0x40/0x9c
[<ffffffff8081f284>] jh7100_reset_driver_init+0x22/0x2a
[<ffffffff800020f0>] do_one_initcall+0x38/0x17c
[<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
[<ffffffff806b1328>] kernel_init+0x1e/0x112
[<ffffffff806b891e>] ret_from_fork+0xe/0x1c
---[ end trace 0000000000000000 ]---
CCACHE: DataFail @ 0x00000000.7FEB0830
CCACHE: DataFail @ 0x00000000.7FEB07F0
CCACHE: DataFail @ 0x00000000.7FEB07F0
CCACHE: DataFail @ 0x00000000.7FEB07F0
CCACHE: DataFail @ 0x00000000.7FEB0830
CCACHE: DataFail @ 0x00000000.7FEB07F0

[...]

Looks like it needs more non-coherent support before we can test
Ethernet.

Note that it boots fine into Debian nfsroot when merging Emil's[2]
visionfive branch instead.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/tag/?h=renesas-drivers-2023-11-14-v6.7-rc1
[2] https://github.com/esmil/linux

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 11/17/23 10:37, Geert Uytterhoeven wrote:
> On Thu, Nov 16, 2023 at 6:55 PM Conor Dooley <conor@kernel.org> wrote:
>> On Thu, Nov 16, 2023 at 03:15:46PM +0200, Cristian Ciocaltea wrote:
>>> On 10/30/23 00:53, Cristian Ciocaltea wrote:
>>>> On 10/29/23 20:46, Andrew Lunn wrote:
>>>>> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote:
>>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>>>>>> RGMII-ID.
>>>>>>
>>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
>>>>>> yes, add the mdio & phy sub-nodes.
>>>>>
>>>>> Please could you try to get this tested. It might shed some light on
>>>>> what is going on here, since it is a different PHY.
>>>>
>>>> Actually, this is the main reason I added the patch. I don't have access
>>>> to this board, so it would be great if we could get some help with testing.
>>>
>>> @Emil, @Conor: Any idea who might help us with a quick test on the
>>> BeagleV Starlight board?
>>
>> I don't have one & I am not sure if Emil does. Geert (CCed) should have
> 
> I believe Esmil has.
> 
>> one though. Is there a specific test you need to have done?
> 
> I gave it a try, on top of latest renesas-drivers[1].
> 
> ------------[ cut here ]------------
> simple-pm-bus soc: device non-coherent but no non-coherent operations supported
> WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
> arch_setup_dma_ops+0x7e/0xa2
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> 6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
> Hardware name: BeagleV Starlight Beta (DT)
> epc : arch_setup_dma_ops+0x7e/0xa2
>  ra : arch_setup_dma_ops+0x7e/0xa2
> epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b90
>  gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81217238
>  t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013bc0
>  s1 : 0000000000000000 a0 : 000000000000004f a1 : 0000000200000020
>  a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
>  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
>  s2 : ffffffd880111410 s3 : ffffffff812d712c s4 : 00000000ffffffff
>  s5 : ffffffffffffffed s6 : ffffffd9fffeb530 s7 : ffffffff80a00d70
>  s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
>  s11: 0000000000000000 t3 : ffffffff812ebb5c t4 : ffffffff812ebb5c
>  t5 : ffffffff812ebb38 t6 : ffffffff812ebbb7
> status: 0000000200000120 badaddr: ffffffff812d712c cause: 0000000000000003
> [<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
> [<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
> [<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
> [<ffffffff803ec382>] really_probe+0x54/0x20c
> [<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
> [<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
> [<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
> [<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
> [<ffffffff803ebdc8>] driver_attach+0x1a/0x22
> [<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
> [<ffffffff803ed476>] driver_register+0x3e/0xd8
> [<ffffffff803ee590>] __platform_driver_register+0x1c/0x24
> [<ffffffff8081db32>] simple_pm_bus_driver_init+0x1a/0x22
> [<ffffffff800020f0>] do_one_initcall+0x38/0x17c
> [<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
> [<ffffffff806b1328>] kernel_init+0x1e/0x112
> [<ffffffff806b891e>] ret_from_fork+0xe/0x1c
> ---[ end trace 0000000000000000 ]---
> ------------[ cut here ]------------
> clk-starfive-jh7100 11800000.clock-controller: device non-coherent but
> no non-coherent operations supported
> WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
> arch_setup_dma_ops+0x7e/0xa2
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S
> 6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
> Hardware name: BeagleV Starlight Beta (DT)
> epc : arch_setup_dma_ops+0x7e/0xa2
>  ra : arch_setup_dma_ops+0x7e/0xa2
> epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b70
>  gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81217f48
>  t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013ba0
>  s1 : 0000000000000000 a0 : 000000000000006b a1 : 0000000200000020
>  a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
>  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
>  s2 : ffffffd880112410 s3 : ffffffff812d712c s4 : 0000000fffffffff
>  s5 : 0000000000000000 s6 : ffffffd9fffed3c8 s7 : ffffffff80a00d70
>  s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
>  s11: 0000000000000000 t3 : ffffffff812ec564 t4 : ffffffff812ec564
>  t5 : ffffffff812ec540 t6 : ffffffff812ec5db
> status: 0000000200000120 badaddr: ffffffd880130008 cause: 0000000000000003
> [<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
> [<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
> [<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
> [<ffffffff803ec382>] really_probe+0x54/0x20c
> [<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
> [<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
> [<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
> [<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
> [<ffffffff803ebdc8>] driver_attach+0x1a/0x22
> [<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
> [<ffffffff803ed476>] driver_register+0x3e/0xd8
> [<ffffffff803ee6ce>] __platform_driver_probe+0x40/0x9c
> [<ffffffff8081eb80>] clk_starfive_jh7100_driver_init+0x22/0x2a
> [<ffffffff800020f0>] do_one_initcall+0x38/0x17c
> [<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
> [<ffffffff806b1328>] kernel_init+0x1e/0x112
> [<ffffffff806b891e>] ret_from_fork+0xe/0x1c
> ---[ end trace 0000000000000000 ]---
> CCACHE: DataFail @ 0x00000000.7FEB8930
> CCACHE: 2 banks, 16 ways, sets/bank=1024, bytes/block=64
> CCACHE: Index of the largest way enabled: 0
> ------------[ cut here ]------------
> jh7100-reset 11840000.reset-controller: device non-coherent but no
> non-coherent operations supported
> WARNING: CPU: 0 PID: 1 at arch/riscv/mm/dma-noncoherent.c:140
> arch_setup_dma_ops+0x7e/0xa2
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S
> 6.7.0-rc1-starlight-01489-g995c2f0b8b7e #259
> Hardware name: BeagleV Starlight Beta (DT)
> epc : arch_setup_dma_ops+0x7e/0xa2
>  ra : arch_setup_dma_ops+0x7e/0xa2
> epc : ffffffff8000d462 ra : ffffffff8000d462 sp : ffffffc800013b70
>  gp : ffffffff812d6460 tp : ffffffd880130000 t0 : ffffffff81218d60
>  t1 : 000000000000002d t2 : 2d2d2d2d2d2d2d2d s0 : ffffffc800013ba0
>  s1 : 0000000000000000 a0 : 0000000000000064 a1 : 0000000200000020
>  a2 : 0000000000000000 a3 : 0000000000000001 a4 : 0000000000000000
>  a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000000000030
>  s2 : ffffffd880112c10 s3 : ffffffff812d712c s4 : 0000000fffffffff
>  s5 : 0000000000000000 s6 : ffffffd9fffed7a8 s7 : ffffffff80a00d70
>  s8 : ffffffff80e3f2c0 s9 : 0000000000000008 s10: ffffffff808000ba
>  s11: 0000000000000000 t3 : ffffffff812ed054 t4 : ffffffff812ed054
>  t5 : ffffffff812ed030 t6 : ffffffff812ed0c4
> status: 0000000200000120 badaddr: ffffffd880130008 cause: 0000000000000003
> [<ffffffff8000d462>] arch_setup_dma_ops+0x7e/0xa2
> [<ffffffff80518cb2>] of_dma_configure_id+0xc4/0x222
> [<ffffffff803ee76e>] platform_dma_configure+0x44/0x4e
> [<ffffffff803ec382>] really_probe+0x54/0x20c
> [<ffffffff803ec596>] __driver_probe_device+0x5c/0xd0
> [<ffffffff803ec636>] driver_probe_device+0x2c/0xb0
> [<ffffffff803ec7b6>] __driver_attach+0x6e/0x104
> [<ffffffff803ea652>] bus_for_each_dev+0x58/0xa4
> [<ffffffff803ebdc8>] driver_attach+0x1a/0x22
> [<ffffffff803eb7ba>] bus_add_driver+0xd4/0x19e
> [<ffffffff803ed476>] driver_register+0x3e/0xd8
> [<ffffffff803ee6ce>] __platform_driver_probe+0x40/0x9c
> [<ffffffff8081f284>] jh7100_reset_driver_init+0x22/0x2a
> [<ffffffff800020f0>] do_one_initcall+0x38/0x17c
> [<ffffffff80801354>] kernel_init_freeable+0x1a8/0x20c
> [<ffffffff806b1328>] kernel_init+0x1e/0x112
> [<ffffffff806b891e>] ret_from_fork+0xe/0x1c
> ---[ end trace 0000000000000000 ]---
> CCACHE: DataFail @ 0x00000000.7FEB0830
> CCACHE: DataFail @ 0x00000000.7FEB07F0
> CCACHE: DataFail @ 0x00000000.7FEB07F0
> CCACHE: DataFail @ 0x00000000.7FEB07F0
> CCACHE: DataFail @ 0x00000000.7FEB0830
> CCACHE: DataFail @ 0x00000000.7FEB07F0
> 
> [...]
> 
> Looks like it needs more non-coherent support before we can test
> Ethernet.

Hi Geert,

Thanks for taking the time to test this!

Could you please check if the following are enabled in your kernel config:

  CONFIG_DMA_GLOBAL_POOL
  CONFIG_RISCV_DMA_NONCOHERENT
  CONFIG_RISCV_NONSTANDARD_CACHE_OPS
  CONFIG_SIFIVE_CCACHE

Regards,
Cristian


> Note that it boots fine into Debian nfsroot when merging Emil's[2]
> visionfive branch instead.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/tag/?h=renesas-drivers-2023-11-14-v6.7-rc1
> [2] https://github.com/esmil/linux
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 11/17/23 10:49, Cristian Ciocaltea wrote:
> On 11/17/23 10:37, Geert Uytterhoeven wrote:
>> On Thu, Nov 16, 2023 at 6:55 PM Conor Dooley <conor@kernel.org> wrote:
>>> On Thu, Nov 16, 2023 at 03:15:46PM +0200, Cristian Ciocaltea wrote:
>>>> On 10/30/23 00:53, Cristian Ciocaltea wrote:
>>>>> On 10/29/23 20:46, Andrew Lunn wrote:
>>>>>> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote:
>>>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>>>>>>> RGMII-ID.
>>>>>>>
>>>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
>>>>>>> yes, add the mdio & phy sub-nodes.
>>>>>>
>>>>>> Please could you try to get this tested. It might shed some light on
>>>>>> what is going on here, since it is a different PHY.
>>>>>
>>>>> Actually, this is the main reason I added the patch. I don't have access
>>>>> to this board, so it would be great if we could get some help with testing.
>>>>
>>>> @Emil, @Conor: Any idea who might help us with a quick test on the
>>>> BeagleV Starlight board?
>>>
>>> I don't have one & I am not sure if Emil does. Geert (CCed) should have
>>
>> I believe Esmil has.
>>
>>> one though. Is there a specific test you need to have done?
>>
>> I gave it a try, on top of latest renesas-drivers[1].

[...]

>>
>> Looks like it needs more non-coherent support before we can test
>> Ethernet.
> 
> Hi Geert,
> 
> Thanks for taking the time to test this!
> 
> Could you please check if the following are enabled in your kernel config:
> 
>   CONFIG_DMA_GLOBAL_POOL
>   CONFIG_RISCV_DMA_NONCOHERENT
>   CONFIG_RISCV_NONSTANDARD_CACHE_OPS
>   CONFIG_SIFIVE_CCACHE

Also please note the series requires the SiFive Composable Cache 
controller patches provided by Emil [1].

[1]: https://lore.kernel.org/all/20231031141444.53426-1-emil.renner.berthing@canonical.com/

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Geert Uytterhoeven]

Hi Cristian,

On Fri, Nov 17, 2023 at 9:59 AM Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> wrote:
> On 11/17/23 10:49, Cristian Ciocaltea wrote:
> > On 11/17/23 10:37, Geert Uytterhoeven wrote:
> >> On Thu, Nov 16, 2023 at 6:55 PM Conor Dooley <conor@kernel.org> wrote:
> >>> On Thu, Nov 16, 2023 at 03:15:46PM +0200, Cristian Ciocaltea wrote:
> >>>> On 10/30/23 00:53, Cristian Ciocaltea wrote:
> >>>>> On 10/29/23 20:46, Andrew Lunn wrote:
> >>>>>> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote:
> >>>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
> >>>>>>> RGMII-ID.
> >>>>>>>
> >>>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
> >>>>>>> yes, add the mdio & phy sub-nodes.
> >>>>>>
> >>>>>> Please could you try to get this tested. It might shed some light on
> >>>>>> what is going on here, since it is a different PHY.
> >>>>>
> >>>>> Actually, this is the main reason I added the patch. I don't have access
> >>>>> to this board, so it would be great if we could get some help with testing.
> >>>>
> >>>> @Emil, @Conor: Any idea who might help us with a quick test on the
> >>>> BeagleV Starlight board?
> >>>
> >>> I don't have one & I am not sure if Emil does. Geert (CCed) should have
> >>
> >> I believe Esmil has.
> >>
> >>> one though. Is there a specific test you need to have done?
> >>
> >> I gave it a try, on top of latest renesas-drivers[1].
>
> [...]
>
> >>
> >> Looks like it needs more non-coherent support before we can test
> >> Ethernet.
> >
> > Hi Geert,
> >
> > Thanks for taking the time to test this!
> >
> > Could you please check if the following are enabled in your kernel config:
> >
> >   CONFIG_DMA_GLOBAL_POOL
> >   CONFIG_RISCV_DMA_NONCOHERENT
> >   CONFIG_RISCV_NONSTANDARD_CACHE_OPS
> >   CONFIG_SIFIVE_CCACHE

CONFIG_DMA_GLOBAL_POOL and CONFIG_RISCV_NONSTANDARD_CACHE_OPS were
indeed no longer enabled, as they cannot be enabled manually.

After cherry-picking commit e14ad9ff67fd51dc ("riscv: errata: Add
StarFive JH7100 errata") in esmil/visionfive these options become
enabled. Now it gets a bit further, but still lots of CCACHE DataFail
errors.

> Also please note the series requires the SiFive Composable Cache
> controller patches provided by Emil [1].
>
> [1]: https://lore.kernel.org/all/20231031141444.53426-1-emil.renner.berthing@canonical.com/

That series does not contain any Kconfig changes, so there must be
other missing dependencies?

Perhaps I should just defer to Emil ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 11/17/23 11:12, Geert Uytterhoeven wrote:
> Hi Cristian,
> 
> On Fri, Nov 17, 2023 at 9:59 AM Cristian Ciocaltea
> <cristian.ciocaltea@collabora.com> wrote:
>> On 11/17/23 10:49, Cristian Ciocaltea wrote:
>>> On 11/17/23 10:37, Geert Uytterhoeven wrote:
>>>> On Thu, Nov 16, 2023 at 6:55 PM Conor Dooley <conor@kernel.org> wrote:
>>>>> On Thu, Nov 16, 2023 at 03:15:46PM +0200, Cristian Ciocaltea wrote:
>>>>>> On 10/30/23 00:53, Cristian Ciocaltea wrote:
>>>>>>> On 10/29/23 20:46, Andrew Lunn wrote:
>>>>>>>> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote:
>>>>>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>>>>>>>>> RGMII-ID.
>>>>>>>>>
>>>>>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
>>>>>>>>> yes, add the mdio & phy sub-nodes.
>>>>>>>>
>>>>>>>> Please could you try to get this tested. It might shed some light on
>>>>>>>> what is going on here, since it is a different PHY.
>>>>>>>
>>>>>>> Actually, this is the main reason I added the patch. I don't have access
>>>>>>> to this board, so it would be great if we could get some help with testing.
>>>>>>
>>>>>> @Emil, @Conor: Any idea who might help us with a quick test on the
>>>>>> BeagleV Starlight board?
>>>>>
>>>>> I don't have one & I am not sure if Emil does. Geert (CCed) should have
>>>>
>>>> I believe Esmil has.
>>>>
>>>>> one though. Is there a specific test you need to have done?
>>>>
>>>> I gave it a try, on top of latest renesas-drivers[1].
>>
>> [...]
>>
>>>>
>>>> Looks like it needs more non-coherent support before we can test
>>>> Ethernet.
>>>
>>> Hi Geert,
>>>
>>> Thanks for taking the time to test this!
>>>
>>> Could you please check if the following are enabled in your kernel config:
>>>
>>>   CONFIG_DMA_GLOBAL_POOL
>>>   CONFIG_RISCV_DMA_NONCOHERENT
>>>   CONFIG_RISCV_NONSTANDARD_CACHE_OPS
>>>   CONFIG_SIFIVE_CCACHE
> 
> CONFIG_DMA_GLOBAL_POOL and CONFIG_RISCV_NONSTANDARD_CACHE_OPS were
> indeed no longer enabled, as they cannot be enabled manually.
> 
> After cherry-picking commit e14ad9ff67fd51dc ("riscv: errata: Add
> StarFive JH7100 errata") in esmil/visionfive these options become
> enabled. Now it gets a bit further, but still lots of CCACHE DataFail
> errors.

Right, there is an open question [2] in PATCH v2 08/12 if this patch
should have been part of Emil's ccache series or I will send it in v3
of my series.

[2]: https://lore.kernel.org/lkml/4f661818-1585-41d8-a305-96fd359bc8b8@collabora.com/

>> Also please note the series requires the SiFive Composable Cache
>> controller patches provided by Emil [1].
>>
>> [1]: https://lore.kernel.org/all/20231031141444.53426-1-emil.renner.berthing@canonical.com/
> 
> That series does not contain any Kconfig changes, so there must be
> other missing dependencies?

There shouldn't be any additional Kconfig changes or dependencies as 
those patches just extend an already existing driver. There were some 
changes in v2, but they are still compatible with this series (I've 
retested that to make sure).

My tree is based on next-20231024, so I'm going to rebase it onto
next-20231117, to exclude the possibility of a regression somewhere.

I will also test with renesas-drivers.

Thanks,
Cristian
 
> Perhaps I should just defer to Emil ;-)
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 11/17/23 13:19, Cristian Ciocaltea wrote:
> On 11/17/23 11:12, Geert Uytterhoeven wrote:
>> Hi Cristian,
>>
>> On Fri, Nov 17, 2023 at 9:59 AM Cristian Ciocaltea
>> <cristian.ciocaltea@collabora.com> wrote:
>>> On 11/17/23 10:49, Cristian Ciocaltea wrote:
>>>> On 11/17/23 10:37, Geert Uytterhoeven wrote:
>>>>> On Thu, Nov 16, 2023 at 6:55 PM Conor Dooley <conor@kernel.org> wrote:
>>>>>> On Thu, Nov 16, 2023 at 03:15:46PM +0200, Cristian Ciocaltea wrote:
>>>>>>> On 10/30/23 00:53, Cristian Ciocaltea wrote:
>>>>>>>> On 10/29/23 20:46, Andrew Lunn wrote:
>>>>>>>>> On Sun, Oct 29, 2023 at 06:27:12AM +0200, Cristian Ciocaltea wrote:
>>>>>>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>>>>>>>>>> RGMII-ID.
>>>>>>>>>>
>>>>>>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
>>>>>>>>>> yes, add the mdio & phy sub-nodes.
>>>>>>>>>
>>>>>>>>> Please could you try to get this tested. It might shed some light on
>>>>>>>>> what is going on here, since it is a different PHY.
>>>>>>>>
>>>>>>>> Actually, this is the main reason I added the patch. I don't have access
>>>>>>>> to this board, so it would be great if we could get some help with testing.
>>>>>>>
>>>>>>> @Emil, @Conor: Any idea who might help us with a quick test on the
>>>>>>> BeagleV Starlight board?
>>>>>>
>>>>>> I don't have one & I am not sure if Emil does. Geert (CCed) should have
>>>>>
>>>>> I believe Esmil has.
>>>>>
>>>>>> one though. Is there a specific test you need to have done?
>>>>>
>>>>> I gave it a try, on top of latest renesas-drivers[1].
>>>
>>> [...]
>>>
>>>>>
>>>>> Looks like it needs more non-coherent support before we can test
>>>>> Ethernet.
>>>>
>>>> Hi Geert,
>>>>
>>>> Thanks for taking the time to test this!
>>>>
>>>> Could you please check if the following are enabled in your kernel config:
>>>>
>>>>   CONFIG_DMA_GLOBAL_POOL
>>>>   CONFIG_RISCV_DMA_NONCOHERENT
>>>>   CONFIG_RISCV_NONSTANDARD_CACHE_OPS
>>>>   CONFIG_SIFIVE_CCACHE
>>
>> CONFIG_DMA_GLOBAL_POOL and CONFIG_RISCV_NONSTANDARD_CACHE_OPS were
>> indeed no longer enabled, as they cannot be enabled manually.
>>
>> After cherry-picking commit e14ad9ff67fd51dc ("riscv: errata: Add
>> StarFive JH7100 errata") in esmil/visionfive these options become
>> enabled. Now it gets a bit further, but still lots of CCACHE DataFail
>> errors.
> 
> Right, there is an open question [2] in PATCH v2 08/12 if this patch
> should have been part of Emil's ccache series or I will send it in v3
> of my series.
> 
> [2]: https://lore.kernel.org/lkml/4f661818-1585-41d8-a305-96fd359bc8b8@collabora.com/
> 
>>> Also please note the series requires the SiFive Composable Cache
>>> controller patches provided by Emil [1].
>>>
>>> [1]: https://lore.kernel.org/all/20231031141444.53426-1-emil.renner.berthing@canonical.com/
>>
>> That series does not contain any Kconfig changes, so there must be
>> other missing dependencies?
> 
> There shouldn't be any additional Kconfig changes or dependencies as 
> those patches just extend an already existing driver. There were some 
> changes in v2, but they are still compatible with this series (I've 
> retested that to make sure).
> 
> My tree is based on next-20231024, so I'm going to rebase it onto
> next-20231117, to exclude the possibility of a regression somewhere.
> 
> I will also test with renesas-drivers.

I verified with both trees and didn't notice any issues with my 
VisionFive board, so I don't really understand why BeagleV Starlight 
shows a different behavior.

For reference, please see [3] which contains all required patches 
applied on top of next-20231117. The top-most one 9d36dec7e6da ("riscv: 
dts: starfive: Add JH7100 MMC nodes") is optional, I added it to extend 
a bit the test suite (SD-card card access also works fine).

[3]: https://gitlab.collabora.com/cristicc/linux-next/-/tree/visionfive-eth

For configuring the kernel, I used:

  $ make [...] defconfig
  $ scripts/config --enable CONFIG_NONPORTABLE --enable ERRATA_STARFIVE_JH7100

I also noticed a warning message right before building starts, but it 
doesn't seem to be harmful:

WARNING: unmet direct dependencies detected for DMA_GLOBAL_POOL
  Depends on [n]: !ARCH_HAS_DMA_SET_UNCACHED [=n] && !DMA_DIRECT_REMAP [=y]
  Selected by [y]:
  - ERRATA_STARFIVE_JH7100 [=y] && ARCH_STARFIVE [=y] && NONPORTABLE [=y]

Thanks,
Cristian

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Emil Renner Berthing]

Cristian Ciocaltea wrote:
> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
> RGMII-ID.
>
> TODO: Verify if manual adjustment of the RX internal delay is needed. If
> yes, add the mdio & phy sub-nodes.

Sorry for being late here. I've tested that removing the mdio and phy nodes on
the the Starlight board works fine, but the rx-internal-delay-ps = <900>
property not needed on any of my VisionFive V1 boards either. So I wonder why
you need that on your board

Also in the driver patch you add support for phy-mode = "rgmii-txid", but here
you still set it to "rgmii-id", so which is it?

You've alse removed the phy reset gpio on the Starlight board:

  snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>

Why?

>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> index 7cda3a89020a..d3f4c99d98da 100644
> --- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> +++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> @@ -11,3 +11,8 @@ / {
>  	model = "BeagleV Starlight Beta";
>  	compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100";
>  };
> +
> +&gmac {
> +	phy-mode = "rgmii-id";
> +	status = "okay";
> +};

Lastly the phy-mode and status are the same for the VF1 and Starlight boards,
so why can't these be set in the jh7100-common.dtsi?

/Emil

> --
> 2.42.0
>

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 11/26/23 23:10, Emil Renner Berthing wrote:
> Cristian Ciocaltea wrote:
>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>> RGMII-ID.
>>
>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
>> yes, add the mdio & phy sub-nodes.
> 
> Sorry for being late here. I've tested that removing the mdio and phy nodes on
> the the Starlight board works fine, but the rx-internal-delay-ps = <900>
> property not needed on any of my VisionFive V1 boards either. 

No problem, thanks a lot for taking the time to help with the testing!

> So I wonder why you need that on your board

I noticed you have a patch 70ca054e82b5 ("net: phy: motorcomm: Disable
rgmii rx delay") in your tree, hence I you please confirm the tests were
done with that commit reverted?

> Also in the driver patch you add support for phy-mode = "rgmii-txid", but here
> you still set it to "rgmii-id", so which is it?

Please try with "rgmii-id" first. I added "rgmii-txid" to have a
fallback solution in case the former cannot be used.

> You've alse removed the phy reset gpio on the Starlight board:
> 
>   snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>
> 
> Why?

I missed this in v1 as the gmac handling was done exclusively in
jh7100-common. Thanks for noticing!

>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>> index 7cda3a89020a..d3f4c99d98da 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>> +++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>> @@ -11,3 +11,8 @@ / {
>>  	model = "BeagleV Starlight Beta";
>>  	compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100";
>>  };
>> +
>> +&gmac {
>> +	phy-mode = "rgmii-id";
>> +	status = "okay";
>> +};
> 
> Lastly the phy-mode and status are the same for the VF1 and Starlight boards,
> so why can't these be set in the jh7100-common.dtsi?

I wasn't sure "rgmii-id" can be used for both boards and I didn't want
to unconditionally enable gmac on Starlight before getting a
confirmation that this actually works.

If there is no way to make it working with "rgmii-id" (w/ or w/o
adjusting rx-internal-delay-ps), than we should switch to "rgmii-txid".

Thanks,
Cristian

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Emil Renner Berthing]

Cristian Ciocaltea wrote:
> On 11/26/23 23:10, Emil Renner Berthing wrote:
> > Cristian Ciocaltea wrote:
> >> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
> >> RGMII-ID.
> >>
> >> TODO: Verify if manual adjustment of the RX internal delay is needed. If
> >> yes, add the mdio & phy sub-nodes.
> >
> > Sorry for being late here. I've tested that removing the mdio and phy nodes on
> > the the Starlight board works fine, but the rx-internal-delay-ps = <900>
> > property not needed on any of my VisionFive V1 boards either.
>
> No problem, thanks a lot for taking the time to help with the testing!
>
> > So I wonder why you need that on your board
>
> I noticed you have a patch 70ca054e82b5 ("net: phy: motorcomm: Disable
> rgmii rx delay") in your tree, hence I you please confirm the tests were
> done with that commit reverted?
>
> > Also in the driver patch you add support for phy-mode = "rgmii-txid", but here
> > you still set it to "rgmii-id", so which is it?
>
> Please try with "rgmii-id" first. I added "rgmii-txid" to have a
> fallback solution in case the former cannot be used.

Ah, I see. Sorry I should have read up on the whole thread. Yes, the Starlight
board with the Microchip phy works with "rgmii-id" as is. And you're right,
with "rgmii-id" my VF1 needs the rx-internal-delay-ps = <900> property too.

>
> > You've alse removed the phy reset gpio on the Starlight board:
> >
> >   snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>
> >
> > Why?
>
> I missed this in v1 as the gmac handling was done exclusively in
> jh7100-common. Thanks for noticing!
>
> >>
> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >> ---
> >>  arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> >> index 7cda3a89020a..d3f4c99d98da 100644
> >> --- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> >> +++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> >> @@ -11,3 +11,8 @@ / {
> >>  	model = "BeagleV Starlight Beta";
> >>  	compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100";
> >>  };
> >> +
> >> +&gmac {
> >> +	phy-mode = "rgmii-id";
> >> +	status = "okay";
> >> +};
> >
> > Lastly the phy-mode and status are the same for the VF1 and Starlight boards,
> > so why can't these be set in the jh7100-common.dtsi?
>
> I wasn't sure "rgmii-id" can be used for both boards and I didn't want
> to unconditionally enable gmac on Starlight before getting a
> confirmation that this actually works.
>
> If there is no way to make it working with "rgmii-id" (w/ or w/o
> adjusting rx-internal-delay-ps), than we should switch to "rgmii-txid".

Yeah, I don't exactly know the difference, but both boards seem to work fine
with "rgmii-id", so if that is somehow better and/or more correct let's just go
with that.

Thanks!
/Emil

>
> Thanks,
> Cristian

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 11/28/23 14:08, Emil Renner Berthing wrote:
> Cristian Ciocaltea wrote:
>> On 11/26/23 23:10, Emil Renner Berthing wrote:
>>> Cristian Ciocaltea wrote:
>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>>>> RGMII-ID.
>>>>
>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
>>>> yes, add the mdio & phy sub-nodes.
>>>
>>> Sorry for being late here. I've tested that removing the mdio and phy nodes on
>>> the the Starlight board works fine, but the rx-internal-delay-ps = <900>
>>> property not needed on any of my VisionFive V1 boards either.
>>
>> No problem, thanks a lot for taking the time to help with the testing!
>>
>>> So I wonder why you need that on your board
>>
>> I noticed you have a patch 70ca054e82b5 ("net: phy: motorcomm: Disable
>> rgmii rx delay") in your tree, hence I you please confirm the tests were
>> done with that commit reverted?
>>
>>> Also in the driver patch you add support for phy-mode = "rgmii-txid", but here
>>> you still set it to "rgmii-id", so which is it?
>>
>> Please try with "rgmii-id" first. I added "rgmii-txid" to have a
>> fallback solution in case the former cannot be used.
> 
> Ah, I see. Sorry I should have read up on the whole thread. Yes, the Starlight
> board with the Microchip phy works with "rgmii-id" as is. And you're right,
> with "rgmii-id" my VF1 needs the rx-internal-delay-ps = <900> property too.

That's great, we have now a pretty clear indication that this uncommon behavior
stems from the Motorcomm PHY, and *not* from GMAC.
 
>>
>>> You've alse removed the phy reset gpio on the Starlight board:
>>>
>>>   snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>
>>>
>>> Why?
>>
>> I missed this in v1 as the gmac handling was done exclusively in
>> jh7100-common. Thanks for noticing!
>>
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>>  arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>>>> index 7cda3a89020a..d3f4c99d98da 100644
>>>> --- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>>>> +++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>>>> @@ -11,3 +11,8 @@ / {
>>>>  	model = "BeagleV Starlight Beta";
>>>>  	compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100";
>>>>  };
>>>> +
>>>> +&gmac {
>>>> +	phy-mode = "rgmii-id";
>>>> +	status = "okay";
>>>> +};
>>>
>>> Lastly the phy-mode and status are the same for the VF1 and Starlight boards,
>>> so why can't these be set in the jh7100-common.dtsi?
>>
>> I wasn't sure "rgmii-id" can be used for both boards and I didn't want
>> to unconditionally enable gmac on Starlight before getting a
>> confirmation that this actually works.
>>
>> If there is no way to make it working with "rgmii-id" (w/ or w/o
>> adjusting rx-internal-delay-ps), than we should switch to "rgmii-txid".
> 
> Yeah, I don't exactly know the difference, but both boards seem to work fine
> with "rgmii-id", so if that is somehow better and/or more correct let's just go
> with that.

As Andrew already pointed out, going with "rgmii-id" would be the recommended
approach, as this passes the responsibility of adding both TX and RX delays to
the PHY.  "rgmii-txid" requires the MAC to handle the RX delay, which might
break the boards having a conformant (aka well-behaving) PHY.  For some reason
the Microchip PHY seems to work fine in both cases, but that's most likely an
exception, as other PHYs might expose a totally different and undesired
behavior.

I will prepare a v3 soon, and will drop the patches you have already submitted
as part of [1].

Thanks again for your support, 
Cristian

[1]: https://lore.kernel.org/all/20231126232746.264302-1-emil.renner.berthing@canonical.com/

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Emil Renner Berthing]

Cristian Ciocaltea wrote:
> On 11/28/23 14:08, Emil Renner Berthing wrote:
> > Cristian Ciocaltea wrote:
> >> On 11/26/23 23:10, Emil Renner Berthing wrote:
> >>> Cristian Ciocaltea wrote:
> >>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
> >>>> RGMII-ID.
> >>>>
> >>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
> >>>> yes, add the mdio & phy sub-nodes.
> >>>
> >>> Sorry for being late here. I've tested that removing the mdio and phy nodes on
> >>> the the Starlight board works fine, but the rx-internal-delay-ps = <900>
> >>> property not needed on any of my VisionFive V1 boards either.
> >>
> >> No problem, thanks a lot for taking the time to help with the testing!
> >>
> >>> So I wonder why you need that on your board
> >>
> >> I noticed you have a patch 70ca054e82b5 ("net: phy: motorcomm: Disable
> >> rgmii rx delay") in your tree, hence I you please confirm the tests were
> >> done with that commit reverted?
> >>
> >>> Also in the driver patch you add support for phy-mode = "rgmii-txid", but here
> >>> you still set it to "rgmii-id", so which is it?
> >>
> >> Please try with "rgmii-id" first. I added "rgmii-txid" to have a
> >> fallback solution in case the former cannot be used.
> >
> > Ah, I see. Sorry I should have read up on the whole thread. Yes, the Starlight
> > board with the Microchip phy works with "rgmii-id" as is. And you're right,
> > with "rgmii-id" my VF1 needs the rx-internal-delay-ps = <900> property too.
>
> That's great, we have now a pretty clear indication that this uncommon behavior
> stems from the Motorcomm PHY, and *not* from GMAC.
>
> >>
> >>> You've alse removed the phy reset gpio on the Starlight board:
> >>>
> >>>   snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>
> >>>
> >>> Why?
> >>
> >> I missed this in v1 as the gmac handling was done exclusively in
> >> jh7100-common. Thanks for noticing!
> >>
> >>>>
> >>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >>>> ---
> >>>>  arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> >>>> index 7cda3a89020a..d3f4c99d98da 100644
> >>>> --- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> >>>> +++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> >>>> @@ -11,3 +11,8 @@ / {
> >>>>  	model = "BeagleV Starlight Beta";
> >>>>  	compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100";
> >>>>  };
> >>>> +
> >>>> +&gmac {
> >>>> +	phy-mode = "rgmii-id";
> >>>> +	status = "okay";
> >>>> +};
> >>>
> >>> Lastly the phy-mode and status are the same for the VF1 and Starlight boards,
> >>> so why can't these be set in the jh7100-common.dtsi?
> >>
> >> I wasn't sure "rgmii-id" can be used for both boards and I didn't want
> >> to unconditionally enable gmac on Starlight before getting a
> >> confirmation that this actually works.
> >>
> >> If there is no way to make it working with "rgmii-id" (w/ or w/o
> >> adjusting rx-internal-delay-ps), than we should switch to "rgmii-txid".
> >
> > Yeah, I don't exactly know the difference, but both boards seem to work fine
> > with "rgmii-id", so if that is somehow better and/or more correct let's just go
> > with that.
>
> As Andrew already pointed out, going with "rgmii-id" would be the recommended
> approach, as this passes the responsibility of adding both TX and RX delays to
> the PHY.  "rgmii-txid" requires the MAC to handle the RX delay, which might
> break the boards having a conformant (aka well-behaving) PHY.  For some reason
> the Microchip PHY seems to work fine in both cases, but that's most likely an
> exception, as other PHYs might expose a totally different and undesired
> behavior.
>
> I will prepare a v3 soon, and will drop the patches you have already submitted
> as part of [1].

Sounds good. Then what's missing for ethernet to work is just the clock patches:
https://github.com/esmil/linux/commit/b5abe1cb3815765739aff7949deed6f65b952c4a
https://github.com/esmil/linux/commit/3a7a423b15a9f796586cbbdc37010d2b83ff2367

You can either include those as part of your patch series enabling ethernet, or
they can be submitted separately with the audio clocks. Either way is
fine by me.

/Emil

>
> Thanks again for your support,
> Cristian
>
> [1]: https://lore.kernel.org/all/20231126232746.264302-1-emil.renner.berthing@canonical.com/

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 11/28/23 18:09, Emil Renner Berthing wrote:
> Cristian Ciocaltea wrote:
>> On 11/28/23 14:08, Emil Renner Berthing wrote:
>>> Cristian Ciocaltea wrote:
>>>> On 11/26/23 23:10, Emil Renner Berthing wrote:
>>>>> Cristian Ciocaltea wrote:
>>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>>>>>> RGMII-ID.
>>>>>>
>>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
>>>>>> yes, add the mdio & phy sub-nodes.
>>>>>
>>>>> Sorry for being late here. I've tested that removing the mdio and phy nodes on
>>>>> the the Starlight board works fine, but the rx-internal-delay-ps = <900>
>>>>> property not needed on any of my VisionFive V1 boards either.
>>>>
>>>> No problem, thanks a lot for taking the time to help with the testing!
>>>>
>>>>> So I wonder why you need that on your board
>>>>
>>>> I noticed you have a patch 70ca054e82b5 ("net: phy: motorcomm: Disable
>>>> rgmii rx delay") in your tree, hence I you please confirm the tests were
>>>> done with that commit reverted?
>>>>
>>>>> Also in the driver patch you add support for phy-mode = "rgmii-txid", but here
>>>>> you still set it to "rgmii-id", so which is it?
>>>>
>>>> Please try with "rgmii-id" first. I added "rgmii-txid" to have a
>>>> fallback solution in case the former cannot be used.
>>>
>>> Ah, I see. Sorry I should have read up on the whole thread. Yes, the Starlight
>>> board with the Microchip phy works with "rgmii-id" as is. And you're right,
>>> with "rgmii-id" my VF1 needs the rx-internal-delay-ps = <900> property too.
>>
>> That's great, we have now a pretty clear indication that this uncommon behavior
>> stems from the Motorcomm PHY, and *not* from GMAC.
>>
>>>>
>>>>> You've alse removed the phy reset gpio on the Starlight board:
>>>>>
>>>>>   snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>
>>>>>
>>>>> Why?
>>>>
>>>> I missed this in v1 as the gmac handling was done exclusively in
>>>> jh7100-common. Thanks for noticing!
>>>>
>>>>>>
>>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>>>> ---
>>>>>>  arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++
>>>>>>  1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>>>>>> index 7cda3a89020a..d3f4c99d98da 100644
>>>>>> --- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>>>>>> +++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>>>>>> @@ -11,3 +11,8 @@ / {
>>>>>>  	model = "BeagleV Starlight Beta";
>>>>>>  	compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100";
>>>>>>  };
>>>>>> +
>>>>>> +&gmac {
>>>>>> +	phy-mode = "rgmii-id";
>>>>>> +	status = "okay";
>>>>>> +};
>>>>>
>>>>> Lastly the phy-mode and status are the same for the VF1 and Starlight boards,
>>>>> so why can't these be set in the jh7100-common.dtsi?
>>>>
>>>> I wasn't sure "rgmii-id" can be used for both boards and I didn't want
>>>> to unconditionally enable gmac on Starlight before getting a
>>>> confirmation that this actually works.
>>>>
>>>> If there is no way to make it working with "rgmii-id" (w/ or w/o
>>>> adjusting rx-internal-delay-ps), than we should switch to "rgmii-txid".
>>>
>>> Yeah, I don't exactly know the difference, but both boards seem to work fine
>>> with "rgmii-id", so if that is somehow better and/or more correct let's just go
>>> with that.
>>
>> As Andrew already pointed out, going with "rgmii-id" would be the recommended
>> approach, as this passes the responsibility of adding both TX and RX delays to
>> the PHY.  "rgmii-txid" requires the MAC to handle the RX delay, which might
>> break the boards having a conformant (aka well-behaving) PHY.  For some reason
>> the Microchip PHY seems to work fine in both cases, but that's most likely an
>> exception, as other PHYs might expose a totally different and undesired
>> behavior.
>>
>> I will prepare a v3 soon, and will drop the patches you have already submitted
>> as part of [1].
> 
> Sounds good. Then what's missing for ethernet to work is just the clock patches:
> https://github.com/esmil/linux/commit/b5abe1cb3815765739aff7949deed6f65b952c4a
> https://github.com/esmil/linux/commit/3a7a423b15a9f796586cbbdc37010d2b83ff2367
> 
> You can either include those as part of your patch series enabling ethernet, or
> they can be submitted separately with the audio clocks. Either way is
> fine by me.

I can cherry-pick them, but so far I couldn't identify any networking
related issues if those patches are not applied. Could it be something
specific to Starlight board only?

> /Emil
> 
>>
>> Thanks again for your support,
>> Cristian
>>
>> [1]: https://lore.kernel.org/all/20231126232746.264302-1-emil.renner.berthing@canonical.com/

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Emil Renner Berthing]

Cristian Ciocaltea wrote:
> On 11/28/23 18:09, Emil Renner Berthing wrote:
> > Cristian Ciocaltea wrote:
> >> On 11/28/23 14:08, Emil Renner Berthing wrote:
> >>> Cristian Ciocaltea wrote:
> >>>> On 11/26/23 23:10, Emil Renner Berthing wrote:
> >>>>> Cristian Ciocaltea wrote:
> >>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
> >>>>>> RGMII-ID.
> >>>>>>
> >>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
> >>>>>> yes, add the mdio & phy sub-nodes.
> >>>>>
> >>>>> Sorry for being late here. I've tested that removing the mdio and phy nodes on
> >>>>> the the Starlight board works fine, but the rx-internal-delay-ps = <900>
> >>>>> property not needed on any of my VisionFive V1 boards either.
> >>>>
> >>>> No problem, thanks a lot for taking the time to help with the testing!
> >>>>
> >>>>> So I wonder why you need that on your board
> >>>>
> >>>> I noticed you have a patch 70ca054e82b5 ("net: phy: motorcomm: Disable
> >>>> rgmii rx delay") in your tree, hence I you please confirm the tests were
> >>>> done with that commit reverted?
> >>>>
> >>>>> Also in the driver patch you add support for phy-mode = "rgmii-txid", but here
> >>>>> you still set it to "rgmii-id", so which is it?
> >>>>
> >>>> Please try with "rgmii-id" first. I added "rgmii-txid" to have a
> >>>> fallback solution in case the former cannot be used.
> >>>
> >>> Ah, I see. Sorry I should have read up on the whole thread. Yes, the Starlight
> >>> board with the Microchip phy works with "rgmii-id" as is. And you're right,
> >>> with "rgmii-id" my VF1 needs the rx-internal-delay-ps = <900> property too.
> >>
> >> That's great, we have now a pretty clear indication that this uncommon behavior
> >> stems from the Motorcomm PHY, and *not* from GMAC.
> >>
> >>>>
> >>>>> You've alse removed the phy reset gpio on the Starlight board:
> >>>>>
> >>>>>   snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>
> >>>>>
> >>>>> Why?
> >>>>
> >>>> I missed this in v1 as the gmac handling was done exclusively in
> >>>> jh7100-common. Thanks for noticing!
> >>>>
> >>>>>>
> >>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >>>>>> ---
> >>>>>>  arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++
> >>>>>>  1 file changed, 5 insertions(+)
> >>>>>>
> >>>>>> diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> >>>>>> index 7cda3a89020a..d3f4c99d98da 100644
> >>>>>> --- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> >>>>>> +++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
> >>>>>> @@ -11,3 +11,8 @@ / {
> >>>>>>  	model = "BeagleV Starlight Beta";
> >>>>>>  	compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100";
> >>>>>>  };
> >>>>>> +
> >>>>>> +&gmac {
> >>>>>> +	phy-mode = "rgmii-id";
> >>>>>> +	status = "okay";
> >>>>>> +};
> >>>>>
> >>>>> Lastly the phy-mode and status are the same for the VF1 and Starlight boards,
> >>>>> so why can't these be set in the jh7100-common.dtsi?
> >>>>
> >>>> I wasn't sure "rgmii-id" can be used for both boards and I didn't want
> >>>> to unconditionally enable gmac on Starlight before getting a
> >>>> confirmation that this actually works.
> >>>>
> >>>> If there is no way to make it working with "rgmii-id" (w/ or w/o
> >>>> adjusting rx-internal-delay-ps), than we should switch to "rgmii-txid".
> >>>
> >>> Yeah, I don't exactly know the difference, but both boards seem to work fine
> >>> with "rgmii-id", so if that is somehow better and/or more correct let's just go
> >>> with that.
> >>
> >> As Andrew already pointed out, going with "rgmii-id" would be the recommended
> >> approach, as this passes the responsibility of adding both TX and RX delays to
> >> the PHY.  "rgmii-txid" requires the MAC to handle the RX delay, which might
> >> break the boards having a conformant (aka well-behaving) PHY.  For some reason
> >> the Microchip PHY seems to work fine in both cases, but that's most likely an
> >> exception, as other PHYs might expose a totally different and undesired
> >> behavior.
> >>
> >> I will prepare a v3 soon, and will drop the patches you have already submitted
> >> as part of [1].
> >
> > Sounds good. Then what's missing for ethernet to work is just the clock patches:
> > https://github.com/esmil/linux/commit/b5abe1cb3815765739aff7949deed6f65b952c4a
> > https://github.com/esmil/linux/commit/3a7a423b15a9f796586cbbdc37010d2b83ff2367
> >
> > You can either include those as part of your patch series enabling ethernet, or
> > they can be submitted separately with the audio clocks. Either way is
> > fine by me.
>
> I can cherry-pick them, but so far I couldn't identify any networking
> related issues if those patches are not applied. Could it be something
> specific to Starlight board only?

No, it's the same for both boards. The dwmac-starfive driver adjusts
the tx clock:

1000Mbit -> 125MHz
 100Mbit ->  25MHz
  10Mbit -> 2.5MHz

The tx clock is given in the device tree as the gmac_tx_inv clock which derives
from either the gmac_root_div or gmac_rmii_ref external clock like this:

gmac_rmii_ref (external) -> gmac_rmii_txclk     \
gmac_root_div  (500MHz)  -> gmac_gtxclk (div N) -> gmac_tx (mux) -> gmac_tx_inv

..where N defaults to 4 and the gmac_tx mux defaults to the gmac_gtxclk, so
the gmac_tx_inv clock defaults to 125MHz suitable for 1000Mbit connections.
See /sys/kernel/debug/clk/clk_summary for another overview.

When the dwmac_starfive driver request to change gmac_tx_inv to 25MHz the clock
framework will that it has the CLK_SET_RATE_PARENT flag set, so it will try
the gmac_tx clock next. This is a mux that can choose either the
125MHz gmac_gtxclk
or the external gmac_rmii_txclk which defaults to 0MHz in the current
device trees,
so the request cannot be met.

That's why we need to set the CLK_SET_RATE_PARENT (and CLK_SET_RATE_NO_REPARENT)
flags on the gmac_tx clock so the clock framework again goes to try setting the
gmac_gtxclk to 25MHz, which it can because it's a divider and setting N=20
does the trick.

On your board you can manually force a 100Mbit connection with
ethtool -s eth0 speed 100

That fails on my boards without those two patches.
/Emil

> >>
> >> Thanks again for your support,
> >> Cristian
> >>
> >> [1]: https://lore.kernel.org/all/20231126232746.264302-1-emil.renner.berthing@canonical.com/

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 11/29/23 16:28, Emil Renner Berthing wrote:
> Cristian Ciocaltea wrote:
>> On 11/28/23 18:09, Emil Renner Berthing wrote:
>>> Cristian Ciocaltea wrote:
>>>> On 11/28/23 14:08, Emil Renner Berthing wrote:
>>>>> Cristian Ciocaltea wrote:
>>>>>> On 11/26/23 23:10, Emil Renner Berthing wrote:
>>>>>>> Cristian Ciocaltea wrote:
>>>>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>>>>>>>> RGMII-ID.
>>>>>>>>
>>>>>>>> TODO: Verify if manual adjustment of the RX internal delay is needed. If
>>>>>>>> yes, add the mdio & phy sub-nodes.
>>>>>>>
>>>>>>> Sorry for being late here. I've tested that removing the mdio and phy nodes on
>>>>>>> the the Starlight board works fine, but the rx-internal-delay-ps = <900>
>>>>>>> property not needed on any of my VisionFive V1 boards either.
>>>>>>
>>>>>> No problem, thanks a lot for taking the time to help with the testing!
>>>>>>
>>>>>>> So I wonder why you need that on your board
>>>>>>
>>>>>> I noticed you have a patch 70ca054e82b5 ("net: phy: motorcomm: Disable
>>>>>> rgmii rx delay") in your tree, hence I you please confirm the tests were
>>>>>> done with that commit reverted?
>>>>>>
>>>>>>> Also in the driver patch you add support for phy-mode = "rgmii-txid", but here
>>>>>>> you still set it to "rgmii-id", so which is it?
>>>>>>
>>>>>> Please try with "rgmii-id" first. I added "rgmii-txid" to have a
>>>>>> fallback solution in case the former cannot be used.
>>>>>
>>>>> Ah, I see. Sorry I should have read up on the whole thread. Yes, the Starlight
>>>>> board with the Microchip phy works with "rgmii-id" as is. And you're right,
>>>>> with "rgmii-id" my VF1 needs the rx-internal-delay-ps = <900> property too.
>>>>
>>>> That's great, we have now a pretty clear indication that this uncommon behavior
>>>> stems from the Motorcomm PHY, and *not* from GMAC.
>>>>
>>>>>>
>>>>>>> You've alse removed the phy reset gpio on the Starlight board:
>>>>>>>
>>>>>>>   snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>
>>>>>>>
>>>>>>> Why?
>>>>>>
>>>>>> I missed this in v1 as the gmac handling was done exclusively in
>>>>>> jh7100-common. Thanks for noticing!
>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>>>>>> ---
>>>>>>>>  arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts | 5 +++++
>>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>>>>>>>> index 7cda3a89020a..d3f4c99d98da 100644
>>>>>>>> --- a/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>>>>>>>> +++ b/arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
>>>>>>>> @@ -11,3 +11,8 @@ / {
>>>>>>>>  	model = "BeagleV Starlight Beta";
>>>>>>>>  	compatible = "beagle,beaglev-starlight-jh7100-r0", "starfive,jh7100";
>>>>>>>>  };
>>>>>>>> +
>>>>>>>> +&gmac {
>>>>>>>> +	phy-mode = "rgmii-id";
>>>>>>>> +	status = "okay";
>>>>>>>> +};
>>>>>>>
>>>>>>> Lastly the phy-mode and status are the same for the VF1 and Starlight boards,
>>>>>>> so why can't these be set in the jh7100-common.dtsi?
>>>>>>
>>>>>> I wasn't sure "rgmii-id" can be used for both boards and I didn't want
>>>>>> to unconditionally enable gmac on Starlight before getting a
>>>>>> confirmation that this actually works.
>>>>>>
>>>>>> If there is no way to make it working with "rgmii-id" (w/ or w/o
>>>>>> adjusting rx-internal-delay-ps), than we should switch to "rgmii-txid".
>>>>>
>>>>> Yeah, I don't exactly know the difference, but both boards seem to work fine
>>>>> with "rgmii-id", so if that is somehow better and/or more correct let's just go
>>>>> with that.
>>>>
>>>> As Andrew already pointed out, going with "rgmii-id" would be the recommended
>>>> approach, as this passes the responsibility of adding both TX and RX delays to
>>>> the PHY.  "rgmii-txid" requires the MAC to handle the RX delay, which might
>>>> break the boards having a conformant (aka well-behaving) PHY.  For some reason
>>>> the Microchip PHY seems to work fine in both cases, but that's most likely an
>>>> exception, as other PHYs might expose a totally different and undesired
>>>> behavior.
>>>>
>>>> I will prepare a v3 soon, and will drop the patches you have already submitted
>>>> as part of [1].
>>>
>>> Sounds good. Then what's missing for ethernet to work is just the clock patches:
>>> https://github.com/esmil/linux/commit/b5abe1cb3815765739aff7949deed6f65b952c4a
>>> https://github.com/esmil/linux/commit/3a7a423b15a9f796586cbbdc37010d2b83ff2367
>>>
>>> You can either include those as part of your patch series enabling ethernet, or
>>> they can be submitted separately with the audio clocks. Either way is
>>> fine by me.
>>
>> I can cherry-pick them, but so far I couldn't identify any networking
>> related issues if those patches are not applied. Could it be something
>> specific to Starlight board only?
> 
> No, it's the same for both boards. The dwmac-starfive driver adjusts
> the tx clock:
> 
> 1000Mbit -> 125MHz
>  100Mbit ->  25MHz
>   10Mbit -> 2.5MHz
> 
> The tx clock is given in the device tree as the gmac_tx_inv clock which derives
> from either the gmac_root_div or gmac_rmii_ref external clock like this:
> 
> gmac_rmii_ref (external) -> gmac_rmii_txclk     \
> gmac_root_div  (500MHz)  -> gmac_gtxclk (div N) -> gmac_tx (mux) -> gmac_tx_inv
> 
> ..where N defaults to 4 and the gmac_tx mux defaults to the gmac_gtxclk, so
> the gmac_tx_inv clock defaults to 125MHz suitable for 1000Mbit connections.
> See /sys/kernel/debug/clk/clk_summary for another overview.
> 
> When the dwmac_starfive driver request to change gmac_tx_inv to 25MHz the clock
> framework will that it has the CLK_SET_RATE_PARENT flag set, so it will try
> the gmac_tx clock next. This is a mux that can choose either the
> 125MHz gmac_gtxclk
> or the external gmac_rmii_txclk which defaults to 0MHz in the current
> device trees,
> so the request cannot be met.
> 
> That's why we need to set the CLK_SET_RATE_PARENT (and CLK_SET_RATE_NO_REPARENT)
> flags on the gmac_tx clock so the clock framework again goes to try setting the
> gmac_gtxclk to 25MHz, which it can because it's a divider and setting N=20
> does the trick.
> 
> On your board you can manually force a 100Mbit connection with
> ethtool -s eth0 speed 100
> 
> That fails on my boards without those two patches.
> /Emil

Thanks for the detailed explanation! I've been only verified with
gigabit connectivity, that would explain why I didn't notice the issue.
I will make sure to properly test this before sending v3.

Regards,
Cristian

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 11/28/23 02:40, Cristian Ciocaltea wrote:
> On 11/26/23 23:10, Emil Renner Berthing wrote:
>> Cristian Ciocaltea wrote:
>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>>> RGMII-ID.
>>>

[...]
 
>> You've alse removed the phy reset gpio on the Starlight board:
>>
>>   snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>
>>
>> Why?
> 
> I missed this in v1 as the gmac handling was done exclusively in
> jh7100-common. Thanks for noticing!

Hi Emil,

I think the reset doesn't actually trigger because "snps,reset-gpios" is
not a valid property, it should have been "snps,reset-gpio" (without the
trailing "s").

However, this seems to be deprecated now, and the recommended approach
would be to define the reset gpio in the phy node, which I did in [1].

Hopefully this won't cause any unexpected behaviour. Otherwise we should
probably simply drop it.

[1]: https://lore.kernel.org/lkml/20231215204050.2296404-8-cristian.ciocaltea@collabora.com/ 

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Emil Renner Berthing]

Cristian Ciocaltea wrote:
> On 11/28/23 02:40, Cristian Ciocaltea wrote:
> > On 11/26/23 23:10, Emil Renner Berthing wrote:
> >> Cristian Ciocaltea wrote:
> >>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
> >>> RGMII-ID.
> >>>
>
> [...]
>
> >> You've alse removed the phy reset gpio on the Starlight board:
> >>
> >>   snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>
> >>
> >> Why?
> >
> > I missed this in v1 as the gmac handling was done exclusively in
> > jh7100-common. Thanks for noticing!
>
> Hi Emil,
>
> I think the reset doesn't actually trigger because "snps,reset-gpios" is
> not a valid property, it should have been "snps,reset-gpio" (without the
> trailing "s").
>
> However, this seems to be deprecated now, and the recommended approach
> would be to define the reset gpio in the phy node, which I did in [1].
>
> Hopefully this won't cause any unexpected behaviour. Otherwise we should
> probably simply drop it.
>
> [1]: https://lore.kernel.org/lkml/20231215204050.2296404-8-cristian.ciocaltea@collabora.com/

Oh, nice catch! With your v3 patches the Starlight board still works fine and
GPIO63 is correctly grabbed and used for "PHY reset".

/Emil

Re: [PATCH v2 12/12] [UNTESTED] riscv: dts: starfive: beaglev-starlight: Enable gmac

[Cristian Ciocaltea]

On 12/16/23 21:24, Emil Renner Berthing wrote:
> Cristian Ciocaltea wrote:
>> On 11/28/23 02:40, Cristian Ciocaltea wrote:
>>> On 11/26/23 23:10, Emil Renner Berthing wrote:
>>>> Cristian Ciocaltea wrote:
>>>>> The BeagleV Starlight SBC uses a Microchip KSZ9031RNXCA PHY supporting
>>>>> RGMII-ID.
>>>>>
>>
>> [...]
>>
>>>> You've alse removed the phy reset gpio on the Starlight board:
>>>>
>>>>   snps,reset-gpios = <&gpio 63 GPIO_ACTIVE_LOW>
>>>>
>>>> Why?
>>>
>>> I missed this in v1 as the gmac handling was done exclusively in
>>> jh7100-common. Thanks for noticing!
>>
>> Hi Emil,
>>
>> I think the reset doesn't actually trigger because "snps,reset-gpios" is
>> not a valid property, it should have been "snps,reset-gpio" (without the
>> trailing "s").
>>
>> However, this seems to be deprecated now, and the recommended approach
>> would be to define the reset gpio in the phy node, which I did in [1].
>>
>> Hopefully this won't cause any unexpected behaviour. Otherwise we should
>> probably simply drop it.
>>
>> [1]: https://lore.kernel.org/lkml/20231215204050.2296404-8-cristian.ciocaltea@collabora.com/
> 
> Oh, nice catch! With your v3 patches the Starlight board still works fine and
> GPIO63 is correctly grabbed and used for "PHY reset".

Great, thanks a lot for retesting this!