Re: [PATCH v2 7/8] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Marc Zyngier <maz@kernel.org>]

On Mon, 14 Mar 2022 21:31:42 +0000,
Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
> 
> The 98DX2530 SoC is the Control and Management CPU integrated into
> the Marvell 98DX25xx and 98DX35xx series of switch chip (internally
> referred to as AlleyCat5 and AlleyCat5X).
> 
> These files have been taken from the Marvell SDK and lightly cleaned
> up with the License and copyright retained.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     The Marvell SDK has a number of new compatible strings. I've brought
>     through some of the drivers or where possible used an in-tree
>     alternative (e.g. there is SDK code for a ac5-gpio but the existing
>     marvell,armada-8k-gpio seems to cover what is needed if you use an
>     appropriate binding). I expect that there will a new series of patches
>     when I get some different hardware (or additions to this series
>     depending on if/when it lands).
>     
>     Changes in v2:
>     - Make pinctrl a child node of a syscon node
>     - Use marvell,armada-8k-gpio instead of orion-gpio
>     - Remove nand peripheral. The Marvell SDK does have some changes for the
>       ac5-nand-controller but I currently lack hardware with NAND fitted so
>       I can't test it right now. I've therefore chosen to omit the node and
>       not attempted to bring in the driver or binding.
>     - Remove pcie peripheral. Again there are changes in the SDK and I have
>       no way of testing them.
>     - Remove prestera node.
>     - Remove "marvell,ac5-ehci" compatible from USB node as
>       "marvell,orion-ehci" is sufficient
>     - Remove watchdog node. There is a buggy driver for the ac5 watchdog in
>       the SDK but it needs some work so I've dropped the node for now.
> 
>  arch/arm64/boot/dts/marvell/Makefile          |   1 +
>  .../boot/dts/marvell/armada-98dx2530.dtsi     | 343 ++++++++++++++++++
>  arch/arm64/boot/dts/marvell/rd-ac5x.dts       |  62 ++++
>  3 files changed, 406 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
>  create mode 100644 arch/arm64/boot/dts/marvell/rd-ac5x.dts
> 
> diff --git a/arch/arm64/boot/dts/marvell/Makefile b/arch/arm64/boot/dts/marvell/Makefile
> index 1c794cdcb8e6..3905dee558b4 100644
> --- a/arch/arm64/boot/dts/marvell/Makefile
> +++ b/arch/arm64/boot/dts/marvell/Makefile
> @@ -24,3 +24,4 @@ dtb-$(CONFIG_ARCH_MVEBU) += cn9132-db.dtb
>  dtb-$(CONFIG_ARCH_MVEBU) += cn9132-db-B.dtb
>  dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-A.dtb
>  dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-B.dtb
> +dtb-$(CONFIG_ARCH_MVEBU) += rd-ac5x.dtb
> diff --git a/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
> new file mode 100644
> index 000000000000..ebe464b9ebd2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/marvell/armada-98dx2530.dtsi
> @@ -0,0 +1,343 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Device Tree For AC5.
> + *
> + * Copyright (C) 2021 Marvell
> + *
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> +	model = "Marvell AC5 SoC";
> +	compatible = "marvell,ac5", "marvell,armada3700";

Are you sure about this compatibility with 3700? If so, why not reuse
the existing file? But the rest of the file tends to show that the SoC
is somehow different (the PPIs are all over the place -- someone got
pointlessly creative here...). I'd really drop this string.

> +	interrupt-parent = <&gic>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	aliases {
> +		serial0 = &uart0;
> +		spiflash0 = &spiflash0;
> +		gpio0 = &gpio0;
> +		gpio1 = &gpio1;
> +		ethernet0 = &eth0;
> +		ethernet1 = &eth1;
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-0.2";
> +		method = "smc";
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>,
> +				 <GIC_PPI 8 IRQ_TYPE_LEVEL_HIGH>,
> +				 <GIC_PPI 10 IRQ_TYPE_LEVEL_HIGH>,
> +				 <GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;

If you have an A55, you're missing an interrupt for the EL2 virtual
timer.

> +		clock-frequency = <25000000>;

Please drop this. The firmware should do the right thing, and if not,
that's an opportunity to fix it.

> +	};
> +

[...]

> +	gic: interrupt-controller@80600000 {
> +		compatible = "arm,gic-v3";
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +		/*#redistributor-regions = <1>;*/
> +		redistributor-stride = <0x0 0x20000>;	// 128kB stride

Please drop these two lines. They are totally pointless as they are
only expressing the default values.

> +		reg = <0x0 0x80600000 0x0 0x10000>, /* GICD */
> +			  <0x0 0x80660000 0x0 0x40000>; /* GICR */
> +		interrupts = <GIC_PPI 6 IRQ_TYPE_LEVEL_HIGH>;
> +	};

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.