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

From: Adam Ford <aford173@gmail.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	Adam Ford-BE <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: Wed, 8 Jul 2020 17:00:09 -0500	[thread overview]
Message-ID: <CAHCN7xLygG5YRG0wt0b9JWW3PHDwMV_kiLRpJqPdSAx7gOoc9w@mail.gmail.com> (raw)
In-Reply-To: <CAHCN7xJWbP16SA-Ok-5syNnqOZAt8OFJo2_rtg5VrNVsN2-eiQ@mail.gmail.com>

On Wed, Jul 8, 2020 at 4:53 PM Adam Ford <aford173@gmail.com> wrote:
>
> On Mon, Jun 22, 2020 at 8:20 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Hi Adam,
> >
> > Thanks for your patch!
>
> Thanks for the review.  :-)
>
> >
> > 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
>
> (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.

Geert,

I forgot to ask.  What is the protocol for something when new bindings
have been accepted in one branch, but another branch where I want to
reference them hasn't merged with the other branch?  I'd really like
to get this board into the next kernel.  I could remove these
references and the calling functions, but that may cause instability
due to undefined behaviour of some of the versaclock functions because
they are not programmed.

However, I would rather have the board mostly work if it means getting
it accepted into the kernel.  Beacon hasn't shipped any outside of the
company yet, so I am not really worried about people seeing problems.
If the board gets accepted without these, I could apply some 'fixes'
at a late date to correct the undefined behavior.  Let me know what
the best way to proceed should be, and I'll send a V2 patch.

thank you,

adam
> >
> > > +
> > > +/ {
> > > +       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@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