On Wed, Jul 8, 2020 at 4:53 PM Adam Ford <aford173@xxxxxxxxx> wrote:
>
> 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.
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@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
