Re: [PATCH] arm64: dts: Introduce r8a774a1-beacon-rzg2m-kit

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Adam Ford <aford173@gmail.com>
Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	aford@beaconembedded.com, Magnus Damm <magnus.damm@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] arm64: dts: Introduce r8a774a1-beacon-rzg2m-kit
Date: Mon, 22 Jun 2020 15:20:39 +0200	[thread overview]
Message-ID: <CAMuHMdWjpyi6QiGTHkwXcepMzP8hN7MkXSJ=Xnxn40VkRU9OXg@mail.gmail.com> (raw)
In-Reply-To: <20200617120510.25071-1-aford173@gmail.com>

Hi Adam,

Thanks for your patch!

On Wed, Jun 17, 2020 at 2:05 PM Adam Ford <aford173@gmail.com> wrote:
> Beacon EmebddedWorks, formerly Logic PD is introducing a new

EmbeddedWorks

> SOM and development kit based on the RZ/G2M SoC from Renesas.
>
> The SOM supports eMMC, WiFi and Bluetooth, along with a Cat-M1
> cellular radio.
>
> The Baseboard has Ethernet, USB, HDMI, stereo audio in and out,
> along with a vareity of push buttons and LED's.

variety

Are schematics for this kit available? That would make it easier to review
the DTS.

Please add the DTB to arch/arm64/boot/dts/renesas/Makefile, else it
won't be built :-)

> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi
> @@ -0,0 +1,733 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020, Compass Electronics Group, LLC
> + */
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/clk/versaclock.h>

This depends on "[PATCH V3 2/3] dt: Add additional option bindings for
IDT VersaClock", which hasn't been accepted yet, AFAIK.

> +
> +/ {
> +       aliases {
> +               serial0 = &scif2;
> +               serial1 = &hscif0;
> +               serial2 = &hscif1;
> +               serial3 = &scif0;
> +               serial4 = &hscif2;
> +               serial5 = &scif5;
> +               spi0 = &msiof0;
> +               spi1 = &msiof1;
> +               spi2 = &msiof2;
> +               spi3 = &msiof3;

Please drop the spiX aliases, as they are not needed.

> +               ethernet0 = &avb;
> +       };

> +       leds {
> +               compatible = "gpio-leds";
> +               pinctrl-0 = <&led_pins>;
> +               pinctrl-names = "default";
> +
> +               led0 {
> +                       gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
> +                       label = "LED0";
> +                       linux,default-trigger = "heartbeat";
> +               };
> +               led1 {
> +                       gpios = <&gpio7 0 GPIO_ACTIVE_HIGH>;
> +                       label = "LED1";
> +                       linux,default-trigger = "heartbeat";
> +               };
> +               led2 {
> +                       gpios = <&gpio7 1 GPIO_ACTIVE_HIGH>;
> +                       label = "LED2";
> +                       linux,default-trigger = "heartbeat";
> +               };
> +               led3 {
> +                       gpios = <&gpio7 3 GPIO_ACTIVE_HIGH>;
> +                       label = "LED3";
> +                       linux,default-trigger = "heartbeat";

Lots of heartbeats ;-)

> +               };
> +       };

> +&ehci0 {
> +       dr_mode = "otg";
> +       status = "okay";
> +       clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>, <&versaclock5 3>, <&versaclock6_bb 4>;

What's the purpose of the 3rd and 4th clock?

> +};
> +
> +&ehci1 {
> +       status = "okay";
> +       clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>, <&versaclock5 4>;

What's the purpose of the 3rd clock?

> +};

> +       msiof1_pins: msiof1 {
> +               groups = "msiof1_clk_g", "msiof1_rxd_g", "msiof1_txd_g";
> +               function = "msiof1";
> +       };

What is msiof1 connected to?

> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi

> +       eeprom@50 {
> +               compatible = "microchip, at24c64", "atmel,24c64";

Bogus space after the first comma.

> +               pagesize = <32>;
> +               read-only;      /* Manufacturing EEPROM programmed at factory */
> +               reg = <0x50>;
> +       };

> new file mode 100644
> index 000000000000..e7ed5d480618
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r8a774a1-beacon-rzg2m-kit.dts
> @@ -0,0 +1,15 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020, Compass Electronics Group, LLC
> + */
> +
> +/dts-v1/;
> +
> +#include "r8a774a1.dtsi"
> +#include "beacon-renesom-som.dtsi"
> +#include "beacon-renesom-baseboard.dtsi"
> +
> +/ {
> +       model = "Beacon Embedded Works RZ/G2M Development Kit";
> +       compatible =    "beacon,beacon-rzg2m", "renesas,r8a774a1";
> +};

Please include a patch to add "beacon,beacon-rzg2m" to
Documentation/devicetree/bindings/arm/renesas.yaml.

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