Re: [PATCH 02/10] riscv: dts: sophgo: cv18xx: Split into CPU core and peripheral parts

[Krzysztof Kozlowski <krzk@kernel.org>]

On 10/02/2025 15:26, Alexander Sverdlin wrote:
> Hi Krzysztof!
> 
> On Mon, 2025-02-10 at 09:43 +0100, Krzysztof Kozlowski wrote:
>>> diff --git a/arch/riscv/boot/dts/sophgo/cv18xx-periph.dtsi b/arch/riscv/boot/dts/sophgo/cv18xx-periph.dtsi
>>> new file mode 100644
>>> index 000000000000..53834b0658b2
>>> --- /dev/null
>>> +++ b/arch/riscv/boot/dts/sophgo/cv18xx-periph.dtsi
>>> @@ -0,0 +1,313 @@
>>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>>> +/*
>>> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
>>> + * Copyright (C) 2023 Inochi Amaoto <inochiama@outlook.com>
>>> + */
>>> +
>>> +#include <dt-bindings/clock/sophgo,cv1800.h>
>>> +#include <dt-bindings/gpio/gpio.h>
>>> +#include <dt-bindings/interrupt-controller/irq.h>
>>> +
>>> +/ {
>>> +	osc: oscillator {
>>> +		compatible = "fixed-clock";
>>
>> I really doubt that external oscillator is a peripheral. This is either
>> part of board or the SoC.
>>
>>
>>> +		clock-output-names = "osc_25m";
>>> +		#clock-cells = <0>;
>>> +	};
>>> +
>>> +	soc {
>>> +		compatible = "simple-bus";
>>> +		#address-cells = <1>;
>>> +		#size-cells = <1>;
>>
>> No, override by phandle/label instead of duplicating SoC.
> 
> Is this one critical? Otherwise I struggle in v2 to both keep

Yes, because duplicated definition is both pain and confusing. It is IMO
semantically not correct - there is only one soc, not two SoCs. If you
have two, then you miss proper unit address.

> SOC_PERIPHERAL_IRQ() in [a new] cv18xx-cpu.dtsi and reference &soc

SOC_PERIPHERAL_IRQ() does not belong here, but to the base DTSI for your
arch. I would rather recommend not to create fake DTSI structure
reflecting some arbitrary choice. cv18xx-cpu.dtsi is not better - for
example type of interrupts are rather arch or GIC specific, not the CPU.
Unless you meant something else by CPU, but then it is getting more
confusing.

Look how others, e.g. Renesas, defines it - no problem overriding soc,
no problem with SOC_PERIPHERAL_IRQ().

> from cv18xx-cpu.dtsi. It's kind of circular-dependency.
> 


Best regards,
Krzysztof

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv