Message-ID: <34f9c682-8a72-491e-9dbd-fc9002496d31@arm.com>
Date: Tue, 7 Jan 2025 14:17:08 +0000
From: Vincenzo Frascino <vincenzo.frascino@....com>
To: Rob Herring <robh@...nel.org>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org,
 Linus Walleij <linus.walleij@...aro.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Liviu Dudau <liviu.dudau@....com>,
 Sudeep Holla <sudeep.holla@....com>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH v4 2/4] arm64: dts: morello: Add support for common
 functionalities

Hi Rob,

Happy new year!

On 03/01/2025 22:14, Rob Herring wrote:
> On Fri, Jan 3, 2025 at 12:16 PM Vincenzo Frascino
> <vincenzo.frascino@....com> wrote:
>>
>> The Morello architecture is an experimental extension to Armv8.2-A,
>> which extends the AArch64 state with the principles proposed in
>> version 7 of the Capability Hardware Enhanced RISC Instructions
>> (CHERI) ISA.
>>
>> The Morello Platform (soc) and the Fixed Virtual Platfom (fvp) share
>> some functionalities that have conveniently been included in
>> morello.dtsi to avoid duplication.
>>
>> Introduce morello.dtsi.
>>
>> Note: Morello fvp will be introduced with a future patch series.
>>
>> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@....com>
>> ---
>>  arch/arm64/boot/dts/arm/morello.dtsi | 467 +++++++++++++++++++++++++++
>>  1 file changed, 467 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/arm/morello.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/arm/morello.dtsi b/arch/arm64/boot/dts/arm/morello.dtsi
>> new file mode 100644
>> index 000000000000..0089b4f2eca7
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/arm/morello.dtsi
>> @@ -0,0 +1,467 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
>> +/*
>> + * Copyright (c) 2020-2024, Arm Limited. All rights reserved.
>> + */
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +/ {
>> +       interrupt-parent = <&gic>;
>> +
>> +       #address-cells = <2>;
>> +       #size-cells = <2>;
>> +
>> +       clocks {
> 
> Drop this container node.
>

Will do in v5.

>> +               soc_refclk50mhz: clock-50000000 {
>> +                       compatible = "fixed-clock";
>> +                       #clock-cells = <0>;
>> +                       clock-frequency = <50000000>;
>> +                       clock-output-names = "apb_pclk";
>> +               };
>> +
>> +               soc_uartclk: clock-50000000-uart {
> 
> There's little point to having 2 fixed clocks with no s/w controls at
> the same frequency. Drop.
> 

Ok, will use the same reference for both.

>> +                       compatible = "fixed-clock";
>> +                       #clock-cells = <0>;
>> +                       clock-frequency = <50000000>;
>> +                       clock-output-names = "uartclk";
>> +               };
>> +
>> +               soc_refclk85mhz: clock-85000000 {
>> +                       compatible = "fixed-clock";
>> +                       #clock-cells = <0>;
>> +                       clock-frequency = <85000000>;
>> +                       clock-output-names = "iofpga:aclk";
>> +               };
>> +
>> +               dpu_aclk: clock-350000000 {
>> +                       /* 77.1 MHz derived from 24 MHz reference clock */
>> +                       compatible = "fixed-clock";
>> +                       #clock-cells = <0>;
>> +                       clock-frequency = <350000000>;
>> +                       clock-output-names = "aclk";
>> +               };
>> +
>> +               dpu_pixel_clk: clock-148500000 {
>> +                       compatible = "fixed-clock";
>> +                       #clock-cells = <0>;
>> +                       clock-frequency = <148500000>;
>> +                       clock-output-names = "pxclk";
>> +               };
>> +       };
>> +
>> +       cpus {
>> +               #address-cells = <2>;
>> +               #size-cells = <0>;
>> +
>> +               cpu0: cpu@0 {
>> +                       compatible = "arm,neoverse-n1";
> 
> I'm pretty sure the N1 doesn't support CHERI/morello. Perhaps
> "arm,neoverse-n1-morello" if we want to capture what it is derived
> from and since "arm,morello" is taken already.
> 

Thank you for this, is the type of feedback I was looking for. My interpretation
of "compatible" was that it is the common denominator in between in between two
or more elements (hence I used neoverse-n1), but clearly it was a bit off.

Since the CPU core is called Rainier, we could either use "arm,ranier" or
"arm,morello-r0p1" to be in line with the SMBIOS.

What do you think?

[...]

>> +
>> +       pmu {
>> +               compatible = "arm,armv8-pmuv3";
> 
> Missing the CPU specific compatible.
>

I will add it in v5.

[...]

>> +
>> +       soc: soc {
>> +               compatible = "simple-bus";
>> +               #address-cells = <2>;
>> +               #size-cells = <2>;
>> +               interrupt-parent = <&gic>;
>> +               ranges;
>> +
>> +               dp0: display@...00000 {
> 
> Sort nodes by address.
> 

Ok, I will do in v5.

[...]

>> +               pcie_ctlr: pcie@...0000000 {
>> +                       device_type = "pci";
>> +                       compatible = "pci-host-ecam-generic";
>> +                       reg = <0x28 0xC0000000 0 0x10000000>;
>> +                       ranges = <0x01000000 0x00 0x00000000 0x00 0x6f000000 0x00 0x00800000>,
>> +                                <0x02000000 0x00 0x60000000 0x00 0x60000000 0x00 0x0f000000>,
>> +                                <0x42000000 0x09 0x00000000 0x09 0x00000000 0x1f 0xc0000000>;
>> +                       bus-range = <0 255>;
> 
> Not needed unless less than this range.
> 

Ok, I will remove it in v5.

[...]

-- 
Regards,
Vincenzo

Powered by blists - more mailing lists