Re: [PATCH] arm64/boot/dts and arm_scpi: add to support Phytium FT2004 CPU

[Sudeep Holla]

On Fri, Dec 02, 2022 at 12:24:48PM +0800, Wang Honghui wrote:
> arm64/boot/dts and arm_scpi: add to support Phytium FT2004 CPU.
>

Krzysztof had commented pointing out some of the issues already. I will
skip those. I am surprised as you seem to still post patches when there
was ongoing discussions on SCPI compatibles on the other thread[1].

> Signed-off-by: Wang Honghui <honghui.wang@xxxxxxxxxxx>
> ---
>  arch/arm64/Kconfig.platforms                  |   5 +
>  arch/arm64/boot/dts/Makefile                  |   1 +
>  arch/arm64/boot/dts/phytium/Makefile          |   5 +
>  .../dts/phytium/ft2004-devboard-d4-dsk.dts    |  73 +++
>  .../dts/phytium/ft2004-generic-psci-soc.dtsi  | 469 ++++++++++++++++++
>  drivers/firmware/arm_scpi.c                   |   1 +

You didn't respond with the reason for the need to use extra compatible
when you can manage with generic compatible. I still think you must not
need this change in arm_scpi as you simply can use existing compatible.
Also if you need, it needs to be separate patch, I have already pointed
out that.

> diff --git a/arch/arm64/boot/dts/phytium/ft2004-generic-psci-soc.dtsi b/arch/arm64/boot/dts/phytium/ft2004-generic-psci-soc.dtsi
> new file mode 100644
> index 000000000000..80d64e17899b
> --- /dev/null
> +++ b/arch/arm64/boot/dts/phytium/ft2004-generic-psci-soc.dtsi
> @@ -0,0 +1,469 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * dts file for FT-2000/4 SoC
> + *
> + * Copyright (C) 2018-2019, Phytium Technology Co., Ltd.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> +	compatible = "phytium,ft2004";
> +	interrupt-parent = <&gic>;
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	aliases {
> +		ethernet0 = &gmac0;
> +		ethernet1 = &gmac1;
> +	};
> +
> +	psci {
> +		compatible   = "arm,psci-1.0";
> +		method       = "smc";

Drop all the below properties they are needed only for v0.1. For v0.2 and
above it is fixed and don't need to be described in the DTS.

> +		cpu_suspend  = <0xc4000001>;
> +		cpu_off      = <0x84000002>;
> +		cpu_on       = <0xc4000003>;
> +		sys_poweroff = <0x84000008>;
> +		sys_reset    = <0x84000009>;
> +	};
> +
> +	cpus {
> +		#address-cells = <0x2>;
> +		#size-cells = <0x0>;
> +
> +		cpu0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,armv8";

I assume this is not model and it is real platform in which case it must
have real processor compatible like "arm,cortex-a57" or whatever it is.

> +
> +		sram: sram@2a006000 {
> +			compatible = "phytium,ft2004-sram-ns","mmio-sram";
> +			reg = <0x0 0x2a006000 0x0 0x2000>;
> +
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges = <0x0 0x0 0x2a006000 0x2000>;
> +
> +			scpi_lpri: scpi-shmem@0 {
> +				compatible = "phytium,ft2004-scpi-shmem";

As mentioned multiple times, use the generic compatible "arm,scp-shmem"
unless you need it for some specific reason in which case I would expect
associated changes in the driver.

> +				reg = <0x1000 0x800>;
> +			};
> +		};
> +
> +};
> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c
> index 435d0e2658a4..876eb2f9ff81 100644
> --- a/drivers/firmware/arm_scpi.c
> +++ b/drivers/firmware/arm_scpi.c
> @@ -904,6 +904,7 @@ static const struct of_device_id shmem_of_match[] __maybe_unused = {
>  	{ .compatible = "amlogic,meson-axg-scp-shmem", },
>  	{ .compatible = "arm,juno-scp-shmem", },
>  	{ .compatible = "arm,scp-shmem", },
> +	{ .compatible = "phytium,ft2004-scpi-shmem", },

Drop the above if there is no other change need in the driver which means
you are compatible with std. SCPI spec and need nothing different meaning
no need for this extra compatible.

-- 
Regards,
Sudeep

[1] https://lore.kernel.org/all/20221201114107.2ig6pdncekzlpdq2@bogus