Re: [PATCH v5 3/3] ARM: dts: sun8i: r40: add second ethernet support

[Maxime Ripard <maxime@cerno.tech>]

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

Hi,

On Fri, Jan 14, 2022 at 03:10:34PM +0300, Evgeny Boger wrote:
> 14.01.2022 13:37, Andre Przywara пишет:
> > On Fri, 14 Jan 2022 10:42:55 +0100
> > Maxime Ripard <maxime@cerno.tech> wrote:
> > 
> > Hi,
> > 
> > > On Thu, Jan 13, 2022 at 08:37:34AM +0300, Evgeny Boger wrote:
> > > > R40 (aka V40, A40i, T3) has two different Ethernet IPs
> > > > called EMAC and GMAC. EMAC only support 10/100 Mbit in MII mode,
> > > > while GMAC support both 10/100 (MII) and 10/100/1000 (RGMII).
> > > > 
> > > > In contrast to A10/A20 where GMAC and EMAC share the same pins
> > > > making EMAC somewhat pointless, on R40 EMAC can be routed to port H.
> > > > Both EMAC (on port H) and GMAC (on port A) can be then enabled at
> > > > the same time, allowing for two ethernet ports.
> > > > 
> > > > Signed-off-by: Evgeny Boger <boger@wirenboard.com>
> > > > ---
> > > >   arch/arm/boot/dts/sun8i-r40.dtsi | 49 ++++++++++++++++++++++++++++++++
> > > >   1 file changed, 49 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi
> > > > index 03d3e5f45a09..8770b105f86e 100644
> > > > --- a/arch/arm/boot/dts/sun8i-r40.dtsi
> > > > +++ b/arch/arm/boot/dts/sun8i-r40.dtsi
> > > > @@ -217,6 +217,19 @@ syscon: system-control@1c00000 {
> > > >   			#size-cells = <1>;
> > > >   			ranges;
> > > > +			sram_a: sram@0 {
> > > > +				compatible = "mmio-sram";
> > > > +				reg = <0x00000000 0xc000>;
> > > > +				#address-cells = <1>;
> > > > +				#size-cells = <1>;
> > > > +				ranges = <0 0x00000000 0xc000>;
> > > > +
> > > > +				emac_sram: sram-section@8000 {
> > > > +					compatible = "allwinner,sun4i-a10-sram-a3-a4";
> > > > +					reg = <0x8000 0x4000>;
> > > > +				};
> > > > +			};
> > > > +
> > > >   			sram_c: sram@1d00000 {
> > > >   				compatible = "mmio-sram";
> > > >   				reg = <0x01d00000 0xd0000>;
> > > > @@ -553,6 +566,24 @@ gmac_rgmii_pins: gmac-rgmii-pins {
> > > >   				drive-strength = <40>;
> > > >   			};
> > > > +			emac_pa_pins: emac-pa-pins {
> > > > +				pins = "PA0", "PA1", "PA2",
> > > > +				       "PA3", "PA4", "PA5", "PA6",
> > > > +				       "PA7", "PA8", "PA9", "PA10",
> > > > +				       "PA11", "PA12", "PA13", "PA14",
> > > > +				       "PA15", "PA16";
> > > > +				function = "emac";
> > > > +			};
> > > > +
> > > > +			emac_ph_pins: emac-ph-pins {
> > > > +				pins = "PH8", "PH9", "PH10", "PH11",
> > > > +				       "PH14", "PH15", "PH16", "PH17",
> > > > +				       "PH18","PH19", "PH20", "PH21",
> > > > +				       "PH22", "PH23", "PH24", "PH25",
> > > > +				       "PH26", "PH27";
> > > > +				function = "emac";
> > > > +			};
> > > There's 17 pins on the first group, but 18 on the second, is it intentional?
> > Yeah, looks like PA17 is missing above. This pin is used for MII only, so
> > it is omitted from the existing gmac_rgmii_pins group.
> > 
> > Evgeny: Did you try a 100MBit PHY on PortA? That should work with both the
> > GMAC and EMAC, right? I wonder if we should add a group that connects all
> > pins needed for MII to the GMAC as well, so basically the above (with PA17
> > added), but using 'function = "gmac";'? Put an "/omit-if-no-ref/" before
> > that (also to those above?) to avoid blowing up the DTB needlessly.
>
> No, it's not intentional, thank you for noticing that!
> I haven't tried EMAC on port A with the latest patch, but it should be
> trivial to do.
> The problem with TXERR signal is that it's kind of optional, so it's hard to
> notice if it doesn't work properly.

TXERR seems to be rarely used for our boards (there's only one in tree,
plus the A20-marsboard currently discussed) so having it as a separate
pinctrl node would make more sense.

> As for adding gmac_*mii*_pins node, I think it could be useful. Do you
> suggest to add it to the same series? Strictly speaking, it has nothing to
> do with R40 and second ethernet support. GMAC is ubiquitous among Allwinner
> SoCs, so I think it would make sense to add to all SoCs at once.
> 
> As for /omit-if-no-ref/ on pinctrl nodes, is there a policy on it? I mean
> there are people (ourselves included) who use device tree overlays a lot,
> both in bootloader and kernel, so it's not that harmless.

/omit-if-no-ref/ doesn't do anything if the DT is compiled with overlay
support (dtc -@)

Maxime

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