Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <11245d23-76a2-1008-eac6-f396954857c7@linaro.org>
Date:   Fri, 10 Jun 2022 11:59:00 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH v2 18/20] arm64: dts: nuvoton: Add initial NPCM8XX device
 tree
Content-Language: en-US
To:     Tomer Maimon <tmaimon77@gmail.com>
Cc:     Avi Fishman <avifishman70@gmail.com>,
        Tali Perry <tali.perry1@gmail.com>,
        Joel Stanley <joel@jms.id.au>,
        Patrick Venture <venture@google.com>,
        Nancy Yuen <yuenn@google.com>,
        Benjamin Fair <benjaminfair@google.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Greg KH <gregkh@linuxfoundation.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Wim Van Sebroeck <wim@linux-watchdog.org>,
        Guenter Roeck <linux@roeck-us.net>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Olof Johansson <olof@lixom.net>,
        Jiri Slaby <jirislaby@kernel.org>,
        Shawn Guo <shawnguo@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Marcel Ziswiler <marcel.ziswiler@toradex.com>,
        Vinod Koul <vkoul@kernel.org>,
        Biju Das <biju.das.jz@bp.renesas.com>,
        Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>,
        Robert Hancock <robert.hancock@calian.com>,
        =?UTF-8?Q?Jonathan_Neusch=c3=a4fer?= <j.neuschaefer@gmx.net>,
        Lubomir Rintel <lkundrak@v3.sk>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-clk <linux-clk@vger.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
        LINUXWATCHDOG <linux-watchdog@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>
References: <20220608095623.22327-1-tmaimon77@gmail.com>
 <20220608095623.22327-19-tmaimon77@gmail.com>
 <24ad8ba0-4244-1159-328d-12d0e67951e1@linaro.org>
 <CAP6Zq1iXaN8D-g2O=cD-XERGj3BROQO=NJ66mquVsOw8nSM=0A@mail.gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <CAP6Zq1iXaN8D-g2O=cD-XERGj3BROQO=NJ66mquVsOw8nSM=0A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/06/2022 00:29, Tomer Maimon wrote:
>>> +     clk_refclk: clk-refclk {
>>> +             compatible = "fixed-clock";
>>> +             #clock-cells = <0>;
>>> +             clock-frequency = <25000000>;
>>
>> Ignored comment.
> Could we use it as a default clock-frequency?

In DTS? If my assumption, that this clock is not on SoC itself, is
correct, then the answer is no, you cannot. The clock physically sits on
the board, so it is defined by board DTS. Feel free to embed in SoC DTSI
most of the clock properties, but the core property - frequency - must
be outside.

>>
>>> +             clock-output-names = "refclk";
>>> +     };
>>> +
>>> +     /* external reference clock for cpu. float in normal operation */
>>> +     clk_sysbypck: clk-sysbypck {
>>> +             compatible = "fixed-clock";
>>> +             #clock-cells = <0>;
>>> +             clock-frequency = <1000000000>;
>>
>> Ignored comment.
> same as above
>>
>>> +             clock-output-names = "sysbypck";
>>> +     };
>>> +
>>> +     /* external reference clock for MC. float in normal operation */
>>> +     clk_mcbypck: clk-mcbypck {
>>> +             compatible = "fixed-clock";
>>> +             #clock-cells = <0>;
>>> +             clock-frequency = <1050000000>;
> same as above
>>> +             clock-output-names = "mcbypck";
>>> +     };
>>> +
>>> +     soc {
>>> +             #address-cells = <2>;
>>> +             #size-cells = <2>;
>>> +             compatible = "simple-bus";
>>> +             interrupt-parent = <&gic>;
>>> +             ranges;
>>> +
>>> +             gcr: gcr@f0800000 {
> I understand it sounds generic but I try to be as much compatible with NPCM7XX
> https://elixir.bootlin.com/linux/v5.19-rc1/source/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi#L91

Then fix NPCM7XX. Please do not multiple bad choices because it looks
similar. Fix the other wrong one.

>>
>> Ignored comment.
>>
>>> +                     compatible = "nuvoton,npcm845-gcr", "syscon",
>>> +                             "simple-mfd";
>>
>> This is not a simple-mfd... I see original bindings defined it that way,
>> but why? I think they should be corrected - remove simple-mfd from the
>> bindings and DTS.
> will remove in both places in V3
>>
>>
>>> +                     reg = <0x0 0xf0800000 0x0 0x1000>;
>>> +             };
>>> +
>>> +             gic: interrupt-controller@dfff9000 {
>>> +                     compatible = "arm,gic-400";
>>> +                     reg = <0x0 0xdfff9000 0x0 0x1000>,
>>> +                           <0x0 0xdfffa000 0x0 0x2000>,
>>> +                           <0x0 0xdfffc000 0x0 0x2000>,
>>> +                           <0x0 0xdfffe000 0x0 0x2000>;
>>> +                     interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>>> +                     #interrupt-cells = <3>;
>>> +                     interrupt-controller;
>>> +                     #address-cells = <0>;
>>> +                     ppi-partitions {
>>> +                             ppi_cluster0: interrupt-partition-0 {
>>> +                                     affinity = <&cpu0 &cpu1 &cpu2 &cpu3>;
>>> +                             };
>>> +                     };
>>> +             };
>>> +     };
>>> +
>>> +     ahb {
>>> +             #address-cells = <2>;
>>> +             #size-cells = <2>;
>>> +             compatible = "simple-bus";
>>> +             interrupt-parent = <&gic>;
> 
>>> +             ranges;
>>> +
>>> +             rstc: rstc@f0801000 {
>>
>> Ignored comment.
>>
> I understand it sounds generic but I try to be as much compatible with NPCM7XX
> https://elixir.bootlin.com/linux/v5.19-rc1/source/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi#L109

Fix 7xx.


Best regards,
Krzysztof