Re: [PATCH 4/5] arm64: dts: renesas: falcon: Add Ethernet-AVB support

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Wolfram,

Thanks for your patch!

On Sun, Dec 27, 2020 at 2:04 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> From: Tho Vu <tho.vu.wh@renesas.com>
>
> Define the Falcon board dependent part of the Ethernet-AVB device nodes.
> Only AVB0 was tested because it was the only port with a PHY on current
> hardware.

I'm a bit confused: according to the schematics, AVB0 is wired by
default to a KSZ9031 PHY connected to an RJ45 connector on the
breakout-board, while AVB1-5 are wired to 88Q2110 PHYs connected to a
5port MATEnet connector on the Ethernet sub board.  So all PHYs are
present?

(The alternative wiring for AVB0 is to a 88Q2110 PHY connected to a
1000Base-T1/TE MATEnet connector on the Ethernet sub board)

>
> Signed-off-by: Tho Vu <tho.vu.wh@renesas.com>
> [wsa: rebased]
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

> --- a/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
> @@ -7,6 +7,7 @@
>
>  /dts-v1/;
>  #include "r8a779a0-falcon-cpu.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
>
>  / {
>         model = "Renesas Falcon CPU and Breakout boards based on r8a779a0";

Missing ethernet0 alias, preventing U-Boot from finding the device-node
and adding an appropriate "local-mac-address" property.

> @@ -21,6 +22,97 @@ chosen {
>         };
>  };
>
> +&avb0 {
> +       pinctrl-0 = <&avb0_pins>;
> +       pinctrl-names = "default";
> +       phy-handle = <&phy0>;
> +       phy-mode = "rgmii-txid";

As the default wiring of AVB0 is similar to Salvator-XS, I think the
default phy-mode of "rgmii" in the base .dtsi should be fine, but
"tx-internal-delay-ps" should be overridden to <2000>.

> +       status = "okay";
> +
> +       phy0: ethernet-phy@0 {
> +               rxc-skew-ps = <1500>;
> +               reg = <0>;
> +               interrupt-parent = <&gpio4>;
> +               interrupts = <16 IRQ_TYPE_LEVEL_LOW>;
> +               reset-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
> +       };
> +};
> +
> +&avb1 {
> +       pinctrl-0 = <&avb1_pins>;
> +       pinctrl-names = "default";
> +       phy-handle = <&phy1>;
> +       phy-mode = "rgmii-txid";
> +
> +       phy1: ethernet-phy@1 {

Why not @0?
As the PHYs are present, why not set "status" to "okay"?

> +               rxc-skew-ps = <1500>;

This property is only supported by the Micrel PHY driver, not by
the Marvell PHY driver.

> +               reg = <0>;
> +               interrupt-parent = <&gpio5>;
> +               interrupts = <16 IRQ_TYPE_LEVEL_LOW>;
> +               reset-gpios = <&gpio5 15 GPIO_ACTIVE_LOW>;
> +       };
> +};

Same questions and comments for all instances below.
Perhaps we should postpone adding avb1-5 until they can be tested?

> @@ -78,6 +170,109 @@ &i2c6 {
>  };
>
>  &pfc {
> +       avb0_pins: avb0 {
> +               mux {
> +                       groups = "avb0_link", "avb0_mdio", "avb0_rgmii", "avb0_txcrefclk";
> +                       function = "avb0";
> +               };
> +
> +               pins_mdio {
> +                       groups = "avb0_mdio";
> +                       drive-strength = <21>;
> +               };
> +
> +               pins_mii_tx {

Strange node name, as the "avb0_rgmii" group includes rx pins.

> +                       groups = "avb0_rgmii";
> +                       drive-strength = <21>;

I can't comment on the drive-strength values.

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