Hi Wolfram,
Thanks for your patch!
On Sun, Dec 27, 2020 at 2:04 PM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> From: Tho Vu <tho.vu.wh@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> [wsa: rebased]
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> --- 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@xxxxxxxxxxxxxx
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
