The Allwinner H616 contains an "EMAC" Ethernet MAC compatible to the A64
version.
Add it to the list of compatible strings.
Signed-off-by: Andre Przywara <[email protected]>
---
.../devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
index 6a4831fd3616c..87f1306831cc9 100644
--- a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
+++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
@@ -22,6 +22,7 @@ properties:
- enum:
- allwinner,sun20i-d1-emac
- allwinner,sun50i-h6-emac
+ - allwinner,sun50i-h616-emac
- const: allwinner,sun50i-a64-emac
reg:
--
2.25.1
On Fri, 01 Jul 2022 12:24:47 +0100, Andre Przywara wrote:
> The Allwinner H616 contains an "EMAC" Ethernet MAC compatible to the A64
> version.
>
> Add it to the list of compatible strings.
>
> Signed-off-by: Andre Przywara <[email protected]>
> ---
> .../devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
Acked-by: Rob Herring <[email protected]>
On 7/1/22 6:24 AM, Andre Przywara wrote:
> The Allwinner H616 contains an "EMAC" Ethernet MAC compatible to the A64
> version.
>
> Add it to the list of compatible strings.
>
> Signed-off-by: Andre Przywara <[email protected]>
> ---
> .../devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> index 6a4831fd3616c..87f1306831cc9 100644
> --- a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> @@ -22,6 +22,7 @@ properties:
> - enum:
> - allwinner,sun20i-d1-emac
> - allwinner,sun50i-h6-emac
> + - allwinner,sun50i-h616-emac
The H616 manual has register fields for an internal PHY, like H3. Are these not
hooked up for either EMAC?
Regards,
Samuel
> - const: allwinner,sun50i-a64-emac
>
> reg:
>
On Mon, 4 Jul 2022 18:53:14 -0500
Samuel Holland <[email protected]> wrote:
Hi Samuel,
> On 7/1/22 6:24 AM, Andre Przywara wrote:
> > The Allwinner H616 contains an "EMAC" Ethernet MAC compatible to the A64
> > version.
> >
> > Add it to the list of compatible strings.
> >
> > Signed-off-by: Andre Przywara <[email protected]>
> > ---
> > .../devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> > index 6a4831fd3616c..87f1306831cc9 100644
> > --- a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> > +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> > @@ -22,6 +22,7 @@ properties:
> > - enum:
> > - allwinner,sun20i-d1-emac
> > - allwinner,sun50i-h6-emac
> > + - allwinner,sun50i-h616-emac
>
> The H616 manual has register fields for an internal PHY, like H3. Are these not
> hooked up for either EMAC?
Which register fields do you mean, exactly? The H616 uses the same
internal PHY solution as the H6: an AC200 die co-packaged on the carrier
(or whatever integration solution they actually chose). The difference to
the H6 is that EMAC0 is hardwired to the external RGMII pins, whereas EMAC1
is hardwired to the internal AC200 RMII pins.
From all I could see that does not impact the actual MAC IP: both are the
same as in the H6, or A64, for that matter.
There is one twist, though: the second EMAC uses a separate EMAC clock
register in the syscon. I came up with this patch to support that:
https://github.com/apritzel/linux/commit/078f591017794a0ec689345b0eeb7150908cf85a
That extends the syscon to take an optional(!) index. So EMAC0 works
exactly like before (both as "<&syscon>;", or "<&syscon 0>;", but for EMAC1
we need the index: "<&syscon 4>;".
But in my opinion this should not affect the MAC binding, at least not for
MAC0. And I think we should get away without a different compatible string
for EMAC1, since the MAC IP is technically the same, it's just the
connection that is different.
In any case I think this does not affect the level of support we promise
today: EMAC0 with an external PHY only.
Cheers,
Andre
>
> > - const: allwinner,sun50i-a64-emac
> >
> > reg:
> >
>
Hi Andre,
On 7/5/22 5:19 AM, Andre Przywara wrote:
> On Mon, 4 Jul 2022 18:53:14 -0500
> Samuel Holland <[email protected]> wrote:
>> On 7/1/22 6:24 AM, Andre Przywara wrote:
>>> The Allwinner H616 contains an "EMAC" Ethernet MAC compatible to the A64
>>> version.
>>>
>>> Add it to the list of compatible strings.
>>>
>>> Signed-off-by: Andre Przywara <[email protected]>
>>> ---
>>> .../devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
>>> index 6a4831fd3616c..87f1306831cc9 100644
>>> --- a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
>>> +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
>>> @@ -22,6 +22,7 @@ properties:
>>> - enum:
>>> - allwinner,sun20i-d1-emac
>>> - allwinner,sun50i-h6-emac
>>> + - allwinner,sun50i-h616-emac
>>
>> The H616 manual has register fields for an internal PHY, like H3. Are these not
>> hooked up for either EMAC?
>
> Which register fields do you mean, exactly?
I mean bits 15-31 of EMAC_EPHY_CLK_REG0.
> The H616 uses the same internal PHY solution as the H6: an AC200 die
> co-packaged on the carrier (or whatever integration solution they actually
> chose). The difference to the H6 is that EMAC0 is hardwired to the external
> RGMII pins, whereas EMAC1 is hardwired to the internal AC200 RMII pins.
> From all I could see that does not impact the actual MAC IP: both are the
> same as in the H6, or A64, for that matter.
If those bits in EMAC_EPHY_CLK_REG0 have no effect, then I agree. But if
switching bit 15 to internal PHY causes Ethernet to stop working, then the mux
really does exist (even if one side is not connected to anything). In that case,
we need to make sure the mux is set to the external PHY, using the code from H3.
> There is one twist, though: the second EMAC uses a separate EMAC clock
> register in the syscon. I came up with this patch to support that:
> https://github.com/apritzel/linux/commit/078f591017794a0ec689345b0eeb7150908cf85a
> That extends the syscon to take an optional(!) index. So EMAC0 works
> exactly like before (both as "<&syscon>;", or "<&syscon 0>;", but for EMAC1
> we need the index: "<&syscon 4>;".
> But in my opinion this should not affect the MAC binding, at least not for
> MAC0.
It definitely affects the MAC binding, because we have to change the definition
of the syscon property. We should still get that reviewed before doing anything
that depends on it. (And I think EMAC0 support depends on it.)
> And I think we should get away without a different compatible string
> for EMAC1, since the MAC IP is technically the same, it's just the
> connection that is different.
If you claim that both EMACs are compatible with allwinner,sun50i-a64-emac, then
you are saying that any existing driver for allwinner,sun50i-a64-emac will also
work with both of the H616 EMACs. But this is not true. If I hook up both EMACs
in the DT per the binding, and use the driver in master, at best only EMAC0 will
work, and likely neither will work.
So at minimum you need a new compatible for the second EMAC, so it only binds to
drivers that know about the syscon offset specifier.
> In any case I think this does not affect the level of support we promise
> today: EMAC0 with an external PHY only.
This can work if you introduce a second compatible for EMAC1. But at that point
you don't need the syscon offset specifier; it can be part of the driver data,
like for R40. (And any future EMAC1 could likely fall back to this compatible.)
Regards,
Samuel
On Tue, 5 Jul 2022 22:55:54 -0500
Samuel Holland <[email protected]> wrote:
Hi Samuel,
thanks for the answer! I think we settled this particular issue, just
for clarification some comments/questions below.
> On 7/5/22 5:19 AM, Andre Przywara wrote:
> > On Mon, 4 Jul 2022 18:53:14 -0500
> > Samuel Holland <[email protected]> wrote:
> >> On 7/1/22 6:24 AM, Andre Przywara wrote:
> >>> The Allwinner H616 contains an "EMAC" Ethernet MAC compatible to the A64
> >>> version.
> >>>
> >>> Add it to the list of compatible strings.
> >>>
> >>> Signed-off-by: Andre Przywara <[email protected]>
> >>> ---
> >>> .../devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml | 1 +
> >>> 1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> >>> index 6a4831fd3616c..87f1306831cc9 100644
> >>> --- a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> >>> +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> >>> @@ -22,6 +22,7 @@ properties:
> >>> - enum:
> >>> - allwinner,sun20i-d1-emac
> >>> - allwinner,sun50i-h6-emac
> >>> + - allwinner,sun50i-h616-emac
> >>
> >> The H616 manual has register fields for an internal PHY, like H3. Are these not
> >> hooked up for either EMAC?
> >
> > Which register fields do you mean, exactly?
>
> I mean bits 15-31 of EMAC_EPHY_CLK_REG0.
Right, so those bits look the same as in the H6, and probably do the
same "nothing" here as well, except for PHY_SELECT[15]?
> > The H616 uses the same internal PHY solution as the H6: an AC200 die
> > co-packaged on the carrier (or whatever integration solution they actually
> > chose). The difference to the H6 is that EMAC0 is hardwired to the external
> > RGMII pins, whereas EMAC1 is hardwired to the internal AC200 RMII pins.
> > From all I could see that does not impact the actual MAC IP: both are the
> > same as in the H6, or A64, for that matter.
>
> If those bits in EMAC_EPHY_CLK_REG0 have no effect, then I agree. But if
> switching bit 15 to internal PHY causes Ethernet to stop working, then the mux
> really does exist (even if one side is not connected to anything). In that case,
> we need to make sure the mux is set to the external PHY, using the code from H3.
I guess so, but this is what we do for the H6/A64 already, if I read the
code correctly?
if (gmac->variant->soc_has_internal_phy) {
...
} else {
/* For SoCs without internal PHY the PHY selection bit should be
* set to 0 (external PHY).
*/
reg &= ~H3_EPHY_SELECT;
}
And since we set soc_has_internal_phy to false for the A64 (and H6)
compatible, we should be good?
> > There is one twist, though: the second EMAC uses a separate EMAC clock
> > register in the syscon. I came up with this patch to support that:
> > https://github.com/apritzel/linux/commit/078f591017794a0ec689345b0eeb7150908cf85a
> > That extends the syscon to take an optional(!) index. So EMAC0 works
> > exactly like before (both as "<&syscon>;", or "<&syscon 0>;", but for EMAC1
> > we need the index: "<&syscon 4>;".
> > But in my opinion this should not affect the MAC binding, at least not for
> > MAC0.
>
> It definitely affects the MAC binding, because we have to change the definition
> of the syscon property. We should still get that reviewed before doing anything
> that depends on it. (And I think EMAC0 support depends on it.)
Technically I agree that it affects the current binding, at least for
EMAC1. Though this looks like some shortcoming of our binding then, as
the actual EMAC IP looks the same for both instances. It just seems
to be the *connection* to the PHYs that differ, so it's a more PHY
*setup* property than an EMAC configuration issue.
But I guess this ship has sailed, and we have to stick with what we
have right now, so would need a different compatible and some code
additions for EMAC1. But since this relies on the AC200 support
anyway (buggy RFC/WIP code here [1], btw), this doesn't look like a big
problem for now.
[1] https://github.com/apritzel/linux/commits/ac200-WIP)
> > And I think we should get away without a different compatible string
> > for EMAC1, since the MAC IP is technically the same, it's just the
> > connection that is different.
>
> If you claim that both EMACs are compatible with allwinner,sun50i-a64-emac, then
> you are saying that any existing driver for allwinner,sun50i-a64-emac will also
> work with both of the H616 EMACs. But this is not true. If I hook up both EMACs
> in the DT per the binding, and use the driver in master, at best only EMAC0 will
> work, and likely neither will work.
>
> So at minimum you need a new compatible for the second EMAC, so it only binds to
> drivers that know about the syscon offset specifier.
>
> > In any case I think this does not affect the level of support we promise
> > today: EMAC0 with an external PHY only.
>
> This can work if you introduce a second compatible for EMAC1. But at that point
> you don't need the syscon offset specifier; it can be part of the driver data,
> like for R40. (And any future EMAC1 could likely fall back to this compatible.)
Yeah, I agree. For robustness I would prefer having a specifier, so any
future twisted EMAC could be covered without yet another compatible
string. But this is a detail.
Or we use the opportunity to design this differently, so that the PHY
driver enables its PHY using this bit, maybe as part of this AC200 EPHY
"control" driver?
Cheers,
Andre