Date:   Thu, 11 Jun 2020 15:39:28 +0200
From:   Andreas Färber <afaerber@...e.de>
To:     Daniel Palmer <daniel@...f.com>
Cc:     k@...ko.eu, tim.bird@...y.com, devicetree@...r.kernel.org,
        Daniel Palmer <daniel@...ngy.jp>,
        Rob Herring <robh+dt@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Sam Ravnborg <sam@...nborg.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Heiko Stuebner <heiko.stuebner@...obroma-systems.com>,
        Maxime Ripard <mripard@...nel.org>,
        Lubomir Rintel <lkundrak@...sk>,
        Stephan Gerhold <stephan@...hold.net>,
        Mark Brown <broonie@...nel.org>, allen <allen.chen@....com.tw>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Corbet <corbet@....net>,
        Arnd Bergmann <arnd@...db.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Doug Anderson <armlinux@...isordat.com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Gregory Fong <gregory.0xf0@...il.com>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Will Deacon <will@...nel.org>,
        Nathan Huckleberry <nhuck15@...il.com>,
        Nathan Chancellor <natechancellor@...il.com>,
        Marc Zyngier <maz@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/5] ARM: mstar: Add infinity/mercury series dtsi

Hi,

Am 10.06.20 um 11:04 schrieb Daniel Palmer:
> Adds initial dtsi for the base MStar ARMv7 SoCs, family dtsis for infinity
> and mercury families, and then some chip level dtsis for chips in those
> families.
> 
> Signed-off-by: Daniel Palmer <daniel@...f.com>
> ---
>   MAINTAINERS                              |  3 +
>   arch/arm/boot/dts/infinity-msc313.dtsi   | 14 +++++
>   arch/arm/boot/dts/infinity.dtsi          | 10 ++++
>   arch/arm/boot/dts/infinity3-msc313e.dtsi | 14 +++++
>   arch/arm/boot/dts/infinity3.dtsi         | 10 ++++
>   arch/arm/boot/dts/mercury5-ssc8336n.dtsi | 14 +++++
>   arch/arm/boot/dts/mercury5.dtsi          | 10 ++++
>   arch/arm/boot/dts/mstar-v7.dtsi          | 71 ++++++++++++++++++++++++
>   8 files changed, 146 insertions(+)

Can you split this up into three parts for easier review?

>   create mode 100644 arch/arm/boot/dts/infinity-msc313.dtsi
>   create mode 100644 arch/arm/boot/dts/infinity.dtsi
>   create mode 100644 arch/arm/boot/dts/infinity3-msc313e.dtsi
>   create mode 100644 arch/arm/boot/dts/infinity3.dtsi
>   create mode 100644 arch/arm/boot/dts/mercury5-ssc8336n.dtsi
>   create mode 100644 arch/arm/boot/dts/mercury5.dtsi
>   create mode 100644 arch/arm/boot/dts/mstar-v7.dtsi
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 754521938303..839ae0250d3d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2114,6 +2114,9 @@ ARM/MStar/Sigmastar ARMv7 SoC support
>   M:	Daniel Palmer <daniel@...ngy.jp>
>   L:	linux-arm-kernel@...ts.infradead.org (moderated for non-subscribers)
>   S:	Maintained
> +F:	arch/arm/boot/dts/infinity*.dtsi
> +F:	arch/arm/boot/dts/mercury*.dtsi
> +F:	arch/arm/boot/dts/mstar-v7.dtsi

Sort order wrt D.

>   F:	arch/arm/mach-mstar/
>   F:	Documentation/devicetree/bindings/arm/mstar.yaml
>   
> diff --git a/arch/arm/boot/dts/infinity-msc313.dtsi b/arch/arm/boot/dts/infinity-msc313.dtsi
> new file mode 100644
> index 000000000000..4eb522e6a75d
> --- /dev/null
> +++ b/arch/arm/boot/dts/infinity-msc313.dtsi
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0

DTs as hardware description should be dual-licensed as either MIT or 
BSD-2-Clause, similar to the schema.

Also, elsewhere, for any code that might get reused for OpenOCD (e.g., 
clk drivers and low-level init like machine - 2/5) or other non-kernel 
projects potentially incompatible with GPL-2.0-only, it would be useful 
to use the -or-later version of the GPL for code sharing - if the 
sources you used permit that.

> +/*
> + * Copyright (c) 2019 thingy.jp.

2019-2020? Check elsewhere.

> + * Author: Daniel Palmer <daniel@...ngy.jp>
> + */
> +
> +#include "infinity.dtsi"
> +
> +/ {
> +	memory {
> +		device_type = "memory";
> +		reg = <0x20000000 0x4000000>;

The memory node needs to become memory@...00000 then.

> +	};

I take it, this RAM is integrated? Maybe add some explanation of what 
this file is

> +};
> diff --git a/arch/arm/boot/dts/infinity.dtsi b/arch/arm/boot/dts/infinity.dtsi
> new file mode 100644
> index 000000000000..25d379028689
> --- /dev/null
> +++ b/arch/arm/boot/dts/infinity.dtsi
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 thingy.jp.
> + * Author: Daniel Palmer <daniel@...ngy.jp>
> + */
> +
> +#include "mstar-v7.dtsi"
> +
> +/ {
> +};

What do you intend to add here? Is it really needed? (same below)

Pre-DT-Schema I used to add a compatible property in the .dtsi, to make 
sure we have at least the SoC's, in case someone neglects to add one in 
their board's .dts. With DT schema that's no longer valid (if enum/const 
is required), but Linux would still work better with than without.

> diff --git a/arch/arm/boot/dts/infinity3-msc313e.dtsi b/arch/arm/boot/dts/infinity3-msc313e.dtsi
> new file mode 100644
> index 000000000000..d0c53153faad
> --- /dev/null
> +++ b/arch/arm/boot/dts/infinity3-msc313e.dtsi
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 thingy.jp.
> + * Author: Daniel Palmer <daniel@...ngy.jp>
> + */
> +
> +#include "infinity3.dtsi"
> +
> +/ {
> +	memory {

Ditto, unit address missing.

> +		device_type = "memory";
> +		reg = <0x20000000 0x4000000>;
> +	};
> +};
> diff --git a/arch/arm/boot/dts/infinity3.dtsi b/arch/arm/boot/dts/infinity3.dtsi
> new file mode 100644
> index 000000000000..cf5f18a07835
> --- /dev/null
> +++ b/arch/arm/boot/dts/infinity3.dtsi
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 thingy.jp.
> + * Author: Daniel Palmer <daniel@...ngy.jp>
> + */
> +
> +#include "infinity.dtsi"
> +
> +/ {
> +};

Don't you anticipate incompatibilities between infinity and infinity3, 
i.e., things you don't want to inherit? Seems a bit optimistic. You can 
of course overwrite properties, but deleting is more difficult.

> diff --git a/arch/arm/boot/dts/mercury5-ssc8336n.dtsi b/arch/arm/boot/dts/mercury5-ssc8336n.dtsi
> new file mode 100644
> index 000000000000..7513f903c838
> --- /dev/null
> +++ b/arch/arm/boot/dts/mercury5-ssc8336n.dtsi
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 thingy.jp.
> + * Author: Daniel Palmer <daniel@...ngy.jp>
> + */
> +
> +#include "mercury5.dtsi"
> +
> +/ {
> +	memory {

Unit address.

> +		device_type = "memory";
> +		reg = <0x20000000 0x4000000>;
> +	};
> +};
> diff --git a/arch/arm/boot/dts/mercury5.dtsi b/arch/arm/boot/dts/mercury5.dtsi
> new file mode 100644
> index 000000000000..25d379028689
> --- /dev/null
> +++ b/arch/arm/boot/dts/mercury5.dtsi
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 thingy.jp.
> + * Author: Daniel Palmer <daniel@...ngy.jp>
> + */
> +
> +#include "mstar-v7.dtsi"
> +
> +/ {
> +};
> diff --git a/arch/arm/boot/dts/mstar-v7.dtsi b/arch/arm/boot/dts/mstar-v7.dtsi
> new file mode 100644
> index 000000000000..0fccc4ca52a4
> --- /dev/null
> +++ b/arch/arm/boot/dts/mstar-v7.dtsi

So this is the only file starting with mstar. Have you thought about 
prefixing infinity/mercury, so that they're grouped together?

> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 thingy.jp.
> + * Author: Daniel Palmer <daniel@...ngy.jp>
> + */
> +
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;> 
> +	interrupt-parent = <&gic>;
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0x0>;
> +		};
> +	};
> +
> +	arch_timer {
> +		compatible = "arm,armv7-timer";
> +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2)
> +				| IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2)
> +				| IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2)
> +				| IRQ_TYPE_LEVEL_LOW)>,
> +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(2)
> +				| IRQ_TYPE_LEVEL_LOW)>;
> +		clock-frequency = <6000000>;
> +	};
> +
> +	pmu {
> +		compatible = "arm,cortex-a7-pmu";
> +		interrupts = <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;

Lacking interrupt-affinity.

"This property should be present when there is more than a single SPI."

To deal with single- vs. dual-core models, you should give the pmu node 
a label, e.g., arm_pmu used elsewhere, I think. Depending on your 
preferences you could set it here in common code (less work) or in the 
SoC-specific .dtsi where you know the number of CPUs for sure (safer to 
not forget later).

> +	};
> +
> +	soc: soc {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;

I had been instructed not to use full identity ranges but to exclude RAM 
ranges for safety reasons.

> +
> +		gic: interrupt-controller@...01000 {
> +			compatible = "arm,cortex-a7-gic";
> +			#interrupt-cells = <3>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			interrupt-controller;
> +			reg = <0x16001000 0x1000>,
> +			      <0x16002000 0x1000>;

In addition to Marc's comments, please reorder reg to below compatible 
for consistency. Suggest to also reorder interrupt-controller before the 
-cells properties, after reg.

> +		};
> +
> +		pm_uart: uart@...21000 {
> +			compatible = "ns16550a";
> +			reg = <0x1f221000 0x100>;
> +			reg-shift = <3>;
> +			clock-frequency = <172000000>;
> +			status = "disabled";
> +		};

If you have any decent manuals for these SoCs, I suggest to check 
whether there are any internal buses that you may want to model as 
simple-bus for grouping. In-tree examples include meson and recently 
merged rtd1195 - it affects the reg addresses and unit addresses via 
suitable ranges mappings.

> +	};
> +};

Regards,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

Powered by blists - more mailing lists