| 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
| ||
|
Message-ID: <IA1PR20MB4953434E04427C0940C8DCF5BBD2A@IA1PR20MB4953.namprd20.prod.outlook.com>
Date: Fri, 13 Oct 2023 17:52:03 +0800
From: Inochi Amaoto <inochiama@...look.com>
To: Conor Dooley <conor+dt@...nel.org>
Cc: Inochi Amaoto <inochiama@...look.com>,
Chao Wei <chao.wei@...hgo.com>,
Chen Wang <unicorn_wang@...look.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>,
Jisheng Zhang <jszhang@...nel.org>,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v2 6/7] riscv: dts: sophgo: Separate common devices from cv1800b soc
Sorry for the wrong title.
>Yo,
>
>On Mon, Oct 09, 2023 at 07:26:35PM +0800, Inochi Amaoto wrote:
>> Move the cpu and the common peripherals of CV181x and CV180x to new file.
>>
>> Signed-off-by: Inochi Amaoto <inochiama@...look.com>
>> ---
>> arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 95 +------------------
>> .../dts/sophgo/{cv1800b.dtsi => cv180x.dtsi} | 19 +---
>> 2 files changed, 2 insertions(+), 112 deletions(-)
>> copy arch/riscv/boot/dts/sophgo/{cv1800b.dtsi => cv180x.dtsi} (80%)
>>
>> diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
>> index df40e87ee063..0904154f9829 100644
>> --- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
>> +++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
>> @@ -3,106 +3,13 @@
>> * Copyright (C) 2023 Jisheng Zhang <jszhang@...nel.org>
>> */
>>
>> -#include <dt-bindings/interrupt-controller/irq.h>
>> +#include "cv180x.dtsi"
>>
>> / {
>> compatible = "sophgo,cv1800b";
>> - #address-cells = <1>;
>> - #size-cells = <1>;
>> -
>> - cpus: cpus {
>> - #address-cells = <1>;
>> - #size-cells = <0>;
>> - timebase-frequency = <25000000>;
>> -
>> - cpu0: cpu@0 {
>> - compatible = "thead,c906", "riscv";
>> - device_type = "cpu";
>> - reg = <0>;
>> - d-cache-block-size = <64>;
>> - d-cache-sets = <512>;
>> - d-cache-size = <65536>;
>> - i-cache-block-size = <64>;
>> - i-cache-sets = <128>;
>> - i-cache-size = <32768>;
>> - mmu-type = "riscv,sv39";
>> - riscv,isa = "rv64imafdc";
>> - riscv,isa-base = "rv64i";
>> - riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zicntr", "zicsr",
>> - "zifencei", "zihpm";
>> -
>> - cpu0_intc: interrupt-controller {
>> - compatible = "riscv,cpu-intc";
>> - interrupt-controller;
>> - #address-cells = <0>;
>> - #interrupt-cells = <1>;
>> - };
>> - };
>> - };
>> -
>> - osc: oscillator {
>> - compatible = "fixed-clock";
>> - clock-output-names = "osc_25m";
>> - #clock-cells = <0>;
>> - };
>>
>> soc {
>> - compatible = "simple-bus";
>> interrupt-parent = <&plic>;
>> - #address-cells = <1>;
>> - #size-cells = <1>;
>> - dma-noncoherent;
>> - ranges;
>> -
>> - uart0: serial@...0000 {
>> - compatible = "snps,dw-apb-uart";
>> - reg = <0x04140000 0x100>;
>> - interrupts = <44 IRQ_TYPE_LEVEL_HIGH>;
>> - clocks = <&osc>;
>> - reg-shift = <2>;
>> - reg-io-width = <4>;
>> - status = "disabled";
>> - };
>> -
>> - uart1: serial@...0000 {
>> - compatible = "snps,dw-apb-uart";
>> - reg = <0x04150000 0x100>;
>> - interrupts = <45 IRQ_TYPE_LEVEL_HIGH>;
>> - clocks = <&osc>;
>> - reg-shift = <2>;
>> - reg-io-width = <4>;
>> - status = "disabled";
>> - };
>> -
>> - uart2: serial@...0000 {
>> - compatible = "snps,dw-apb-uart";
>> - reg = <0x04160000 0x100>;
>> - interrupts = <46 IRQ_TYPE_LEVEL_HIGH>;
>> - clocks = <&osc>;
>> - reg-shift = <2>;
>> - reg-io-width = <4>;
>> - status = "disabled";
>> - };
>> -
>> - uart3: serial@...0000 {
>> - compatible = "snps,dw-apb-uart";
>> - reg = <0x04170000 0x100>;
>> - interrupts = <47 IRQ_TYPE_LEVEL_HIGH>;
>> - clocks = <&osc>;
>> - reg-shift = <2>;
>> - reg-io-width = <4>;
>> - status = "disabled";
>> - };
>> -
>> - uart4: serial@...0000 {
>> - compatible = "snps,dw-apb-uart";
>> - reg = <0x041c0000 0x100>;
>> - interrupts = <48 IRQ_TYPE_LEVEL_HIGH>;
>> - clocks = <&osc>;
>> - reg-shift = <2>;
>> - reg-io-width = <4>;
>> - status = "disabled";
>> - };
>>
>> plic: interrupt-controller@...00000 {
>> compatible = "sophgo,cv1800b-plic", "thead,c900-plic";
>> diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv180x.dtsi
>> similarity index 80%
>> copy from arch/riscv/boot/dts/sophgo/cv1800b.dtsi
>> copy to arch/riscv/boot/dts/sophgo/cv180x.dtsi
>> index df40e87ee063..ffaf51724c98 100644
>
>Firstly, this form of diff really threw me, I was quite confused for a
>few minutes. A copy plus a pair of diffs doesn't really make much sense,
>when the operation being carried is an extraction of some nodes to a
>different file.
>
I was told to use -C/-M/-B to generate patch, and the git format-patch
give me this wired output if I use -C, using -M seems no change from v1.
The -B output is also disappointing. Maybe I need to generate agaion?
The v1 version:
https://lore.kernel.org/linux-riscv/IA1PR20MB495360B632D106BBB833D82ABBCFA@IA1PR20MB4953.namprd20.prod.outlook.com/
>> --- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
>> +++ b/arch/riscv/boot/dts/sophgo/cv180x.dtsi
>> @@ -1,12 +1,12 @@
>> // SPDX-License-Identifier: (GPL-2.0 OR MIT)
>> /*
>> * Copyright (C) 2023 Jisheng Zhang <jszhang@...nel.org>
>> + * Copyright (C) 2023 Inochi Amaoto <inochiama@...look.com>
>
>Also, is moving around some bits of hw description really a
>copyrightable change?
>
It seems to be a mistake when I splitting the patch from v1.
This copyright should in the next patch.
>> */
>>
>> #include <dt-bindings/interrupt-controller/irq.h>
>>
>> / {
>> - compatible = "sophgo,cv1800b";
>> #address-cells = <1>;
>> #size-cells = <1>;
>>
>> @@ -48,7 +48,6 @@ osc: oscillator {
>>
>> soc {
>> compatible = "simple-bus";
>> - interrupt-parent = <&plic>;
>> #address-cells = <1>;
>> #size-cells = <1>;
>> dma-noncoherent;
>> @@ -103,21 +102,5 @@ uart4: serial@...0000 {
>> reg-io-width = <4>;
>> status = "disabled";
>> };
>> -
>> - plic: interrupt-controller@...00000 {
>> - compatible = "sophgo,cv1800b-plic", "thead,c900-plic";
>> - reg = <0x70000000 0x4000000>;
>> - interrupts-extended = <&cpu0_intc 11>, <&cpu0_intc 9>;
>> - interrupt-controller;
>> - #address-cells = <0>;
>> - #interrupt-cells = <2>;
>> - riscv,ndev = <101>;
>> - };
>> -
>> - clint: timer@...00000 {
>> - compatible = "sophgo,cv1800b-clint", "thead,c900-clint";
>> - reg = <0x74000000 0x10000>;
>> - interrupts-extended = <&cpu0_intc 3>, <&cpu0_intc 7>;
>> - };
>> };
>> };
>
>What I wanted to comment on was this though - it seems that both the
>cv1800b and the cv1812h have identical plic and clint nodes, other than
>their compatibles? If that is the case, why create a cv1800b and a
>cv1812h specific file containing entirely new nodes, when overriding the
>compatible would be sufficient? Doubly so if the other SoCs in the
>cv18xx series are going to have identical layouts.
>
>I gave it a quick test locally with the below diff applied on top of
>this series - although I didn't make sure that I didn't re-order the
>plic & clint nodes, I just wanted to demonstrate what I had done.
>
Thanks for demonstration. AFAIK, what you said is true. the most devices
of CV180x and CV181x are the same, including plic and clint. The reason I
used a new one is to identify these two devices without making the
compatible string confusing.
Should I change the binding name of plic and clint to "sophgo,cv1800-xxx"
to mark there are the same series? I think this can avoid this confusing
dt nodes.
>Cheers,
>Conor.
>
Powered by blists - more mailing lists