linux-kernel - Re: [PATCH v2 13/17] riscv: dts: microchip: use hart and clk defines for icicle kit

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]
Message-ID: <CAMuHMdUYUEpU4bFobK=fpVDGzwAHQWJ=dvPVmp20DUTLSe+DRw@mail.gmail.com>
Date:   Fri, 17 Dec 2021 14:40:28 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Conor Dooley <conor.dooley@...rochip.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Rob Herring <robh+dt@...nel.org>,
        Jassi Brar <jassisinghbrar@...il.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Mark Brown <broonie@...nel.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, Lee Jones <lee.jones@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux I2C <linux-i2c@...r.kernel.org>,
        Linux PWM List <linux-pwm@...r.kernel.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        linux-rtc@...r.kernel.org, linux-spi <linux-spi@...r.kernel.org>,
        USB list <linux-usb@...r.kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Bin Meng <bin.meng@...driver.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Lewis Hanly <lewis.hanly@...rochip.com>,
        daire.mcnamara@...rochip.com, ivan.griffin@...rochip.com,
        Atish Patra <atish.patra@....com>
Subject: Re: [PATCH v2 13/17] riscv: dts: microchip: use hart and clk defines
 for icicle kit

Hi Conor,

Thanks for your patch!

On Fri, Dec 17, 2021 at 10:33 AM <conor.dooley@...rochip.com> wrote:
> From: Conor Dooley <conor.dooley@...rochip.com>
>
> Update the Microchip Icicle kit device tree by replacing interrupt and
> clock related magic numbers with their defined counterparts.

Usually we make a distinction between (a) numbers that can be looked
up easily in a datasheet, and (b) numbers that were made up because
we needed some mapping. Of course both types of numbers are fixed,
and cannot be changed.

For (a), we tend to use the hardcoded numbers in the DTS files, to
avoid reviewers having to go through another layer of indirection
(i.e. does the number for the define match the number in the
datasheet?).
For (b), we use the defines, as there is no other official place to
look up the numbers.

> --- a/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
> +++ b/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
> @@ -2,6 +2,8 @@
>  /* Copyright (c) 2020 Microchip Technology Inc */
>
>  /dts-v1/;
> +#include "dt-bindings/clock/microchip,mpfs-clock.h"

The clock numbers we're made-up, so they fall under (b).

> +#include "dt-bindings/interrupt-controller/riscv-hart.h"

I believe these are just the official CLIC interrupt IDs, so they
fall under (a)?

> @@ -165,11 +167,16 @@ cache-controller@...0000 {
>                 clint@...0000 {
>                         compatible = "sifive,fu540-c000-clint", "sifive,clint0";
>                         reg = <0x0 0x2000000 0x0 0xC000>;
> -                       interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>,
> -                                             <&cpu1_intc 3>, <&cpu1_intc 7>,
> -                                             <&cpu2_intc 3>, <&cpu2_intc 7>,
> -                                             <&cpu3_intc 3>, <&cpu3_intc 7>,
> -                                             <&cpu4_intc 3>, <&cpu4_intc 7>;
> +                       interrupts-extended = <&cpu0_intc HART_INT_M_SOFT>,
> +                                             <&cpu0_intc HART_INT_M_TIMER>,
> +                                             <&cpu1_intc HART_INT_M_SOFT>,
> +                                             <&cpu1_intc HART_INT_M_TIMER>,
> +                                             <&cpu2_intc HART_INT_M_SOFT>,
> +                                             <&cpu2_intc HART_INT_M_TIMER>,
> +                                             <&cpu3_intc HART_INT_M_SOFT>,
> +                                             <&cpu3_intc HART_INT_M_TIMER>,
> +                                             <&cpu4_intc HART_INT_M_SOFT>,
> +                                             <&cpu4_intc HART_INT_M_TIMER>;

Hence I'm not sure we want changes like this?

>                 };
>
>                 plic: interrupt-controller@...0000 {
         };
>
> @@ -210,7 +221,7 @@ serial0: serial@...00000 {
>                         interrupt-parent = <&plic>;
>                         interrupts = <90>;
>                         current-speed = <115200>;
> -                       clocks = <&clkcfg 8>;
> +                       clocks = <&clkcfg CLK_MMUART0>;

But this change is fine.

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