Re: [PATCH 07/18] ARM: dts: r8a7742: Initial SoC device tree

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Thu, Apr 30, 2020 at 2:49 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> On Wed, Apr 29, 2020 at 11:58 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote:
> > Basic support for the RZ/G1H (R8A7742) SoC. Added placeholders
> > for the peripherals supported by the SoC which will be filled up
> > by incremental patches.
>
> Please remove the placeholders, as there is nothing that depends on their
> presence.
>
Sure will drop that.

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@xxxxxxxxxxxxxx>
>
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/r8a7742.dtsi
> > @@ -0,0 +1,715 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Device Tree Source for the r8a7742 SoC
> > + *
> > + * Copyright (C) 2020 Renesas Electronics Corp.
> > + */
> > +
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/clock/r8a7742-cpg-mssr.h>
> > +#include <dt-bindings/power/r8a7742-sysc.h>
> > +
> > +/ {
> > +       compatible = "renesas,r8a7742";
> > +       #address-cells = <2>;
> > +       #size-cells = <2>;
> > +
> > +       /*
> > +        * The external audio clocks are configured as 0 Hz fixed frequency
> > +        * clocks by default.
> > +        * Boards that provide audio clocks should override them.
> > +        */
> > +       audio_clk_a: audio_clk_a {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency = <0>;
> > +       };
> > +
> > +       audio_clk_b: audio_clk_b {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency = <0>;
> > +       };
> > +
> > +       audio_clk_c: audio_clk_c {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency = <0>;
> > +       };
> > +
> > +       /* External CAN clock */
> > +       can_clk: can {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               /* This value must be overridden by the board. */
> > +               clock-frequency = <0>;
> > +       };
>
> Please drop the audio and CAN clocks for now, as they are not used.
>
OK.

> > +       /* External root clock */
> > +       extal_clk: extal {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               /* This value must be overridden by the board. */
> > +               clock-frequency = <0>;
> > +       };
> > +
> > +       /* External PCIe clock - can be overridden by the board */
> > +       pcie_bus_clk: pcie_bus {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency = <0>;
> > +       };
>
> Please drop the PCI clock for now, as it is not used.
>
OK

> > +
> > +       /* External SCIF clock */
> > +       scif_clk: scif {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               /* This value must be overridden by the board. */
> > +               clock-frequency = <0>;
> > +       };
>
> This should be used (see below).
>
> > +
> > +       /* External USB clock - can be overridden by the board */
> > +       usb_extal_clk: usb_extal {
> > +               compatible = "fixed-clock";
> > +               #clock-cells = <0>;
> > +               clock-frequency = <48000000>;
> > +       };
> > +
> > +       cpus {
>
> Please sort nodes by unit-address (if present) per type, or alphabetically.
>
Sure will take care of it.

> > +       soc {
>
> > +               scifa2: serial@e6c60000 {
> > +                       reg = <0 0xe6c60000 0 0x40>;
> > +                       /* placeholder */
> > +               };
>
> I prefer to see a real node for the serial console, so the system can at
> least be boot tested to a console prompt.
> Note that this requires adding a minimal board DTS, too.
>
OK so in that case Ill enable scifa2 and SDHI interface so it can be bootable.

Cheers,
--Prabhakar

> 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