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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 30 Apr 2020 15:49:17 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Cc:     Magnus Damm <magnus.damm@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Vinod Koul <vkoul@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Marc Zyngier <maz@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Russell King <linux@...linux.org.uk>,
        Lad Prabhakar <prabhakar.csengg@...il.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dmaengine <dmaengine@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 07/18] ARM: dts: r8a7742: Initial SoC device tree

Hi Prabhakar,

Thanks for your patch!

On Wed, Apr 29, 2020 at 11:58 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@...renesas.com> 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.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@...renesas.com>

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

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

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

> +       soc {

> +               scifa2: serial@...60000 {
> +                       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.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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