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

[Adam Ford]

On Mon, Jun 22, 2020 at 8:20 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Adam,
>
> Thanks for your patch!

Thanks for the review.  :-)

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

(facepalm)  I feel stupid.

>
> > 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.

As of right now, we don't have them available publicly.  If you're
willing to sign an NDA, I might be able get you access to them.

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

oops.

>
> > --- /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.

OK.
>
> > +               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 ;-)

Yeah, I cut than down to one.  We don't have a dedicated plan for what
they need to do yet.  :-)

>
> > +               };
> > +       };
>
> > +&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?

We added a versaclock to drive something else, but that other thing
didn't request the clocks, so they didn't ever turn on.  This was bad
hack to get something to turn them on.  We've revised the hardware and
device tree to eliminate this.

>
> > +};
> > +
> > +&ehci1 {
> > +       status = "okay";
> > +       clocks = <&cpg CPG_MOD 703>, <&cpg CPG_MOD 704>, <&versaclock5 4>;
>
> What's the purpose of the 3rd clock?

Same as above, and it'll be gone in V2 as well.
>
> > +};
>
> > +       msiof1_pins: msiof1 {
> > +               groups = "msiof1_clk_g", "msiof1_rxd_g", "msiof1_txd_g";
> > +               function = "msiof1";
> > +       };
>
> What is msiof1 connected to?

There is a GPIO header on the main board with a SPI connector, but
nothing actually on the SPI bus.  We can remove this.

>
> > --- /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.

Noted.

>
> > +               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.

OK.

adam
>
> 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