Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <49daf501-3d01-b772-2173-df3f3ac3e6db@kernel.org>
Date:   Sat, 12 Mar 2022 13:04:27 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH] arm64: dts: intel: add device tree for n6010
Content-Language: en-US
To:     niravkumar.l.rabara@intel.com, dinguyen@kernel.org,
        robh+dt@kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220312080717.1528401-1-niravkumar.l.rabara@intel.com>
From:   Krzysztof Kozlowski <krzk@kernel.org>
In-Reply-To: <20220312080717.1528401-1-niravkumar.l.rabara@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/03/2022 09:07, niravkumar.l.rabara@intel.com wrote:
> From: Niravkumar L Rabara <niravkumar.l.rabara@intel.com>
> 
> Add a dts file for the Silicom FPGA SmartNIC N6010/N6011,
> which is based on the Intel Agliex platform.
> 
> Acked-by: Dinh Nguyen <dinguyen@kernel.org>

It's a v1, so did Dinh ack it off-line?

> Signed-off-by: Niravkumar L Rabara <niravkumar.l.rabara@intel.com>
> ---
>  arch/arm64/boot/dts/intel/Makefile            |  1 +
>  .../boot/dts/intel/socfpga_agilex_n6010.dts   | 83 +++++++++++++++++++
>  2 files changed, 84 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_n6010.dts
> 
> diff --git a/arch/arm64/boot/dts/intel/Makefile b/arch/arm64/boot/dts/intel/Makefile
> index 0b5477442263..a05a610a4006 100644
> --- a/arch/arm64/boot/dts/intel/Makefile
> +++ b/arch/arm64/boot/dts/intel/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += socfpga_agilex_socdk.dtb \
>  				socfpga_agilex_socdk_nand.dtb \
> +				socfpga_agilex_n6010.dtb \

Put the names in alphabetical order.

>  				socfpga_n5x_socdk.dtb
>  dtb-$(CONFIG_ARCH_KEEMBAY) += keembay-evm.dtb
> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_n6010.dts b/arch/arm64/boot/dts/intel/socfpga_agilex_n6010.dts
> new file mode 100644
> index 000000000000..c105f0621eb4
> --- /dev/null
> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex_n6010.dts
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier:     GPL-2.0
> +/*
> + * Copyright (C) 2022, Intel Corporation
> + */
> +#include "socfpga_agilex.dtsi"
> +
> +/ {
> +	model = "SoCFPGA Agilex n6010";

You missed here compatible and respective bindings change.

> +
> +	aliases {
> +		serial0 = &uart0;
> +		ethernet0 = &gmac0;
> +		ethernet1 = &gmac1;
> +		ethernet2 = &gmac2;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	memory {
> +		device_type = "memory";
> +		/* We expect the bootloader to fill in the reg */
> +		reg = <0 0 0 0>;
> +	};
> +
> +	soc {
> +		clocks {
> +			osc1 {

Override with labels, not by full path. See other code as examples. Try
coding in similar way how it is in Linux kernel - it is a great way to
learn.

> +				clock-frequency = <25000000>;


> +			};
> +		};
> +                agilex_hps_bridges: bridge@80000000 {

Weird indentation.

> +                        compatible = "simple-bus";
> +                        reg = <0x80000000 0x60000000>,
> +                                <0xf9000000 0x00100000>;
> +                        reg-names = "axi_h2f", "axi_h2f_lw";
> +                        #address-cells = <0x2>;
> +                        #size-cells = <0x1>;

No need for hex numbers

> +                        ranges = <0x00000000 0x00000000 0xf9000000 0x00001000>,
> +                                <0x00000001 0x02001000 0x82001000 0x00000800>,
> +                                <0x00000001 0x02080000 0x82080000 0x00004000>,
> +                                <0x00000001 0x02100000 0x82100000 0x00080000>,
> +                                <0x00000001 0x02000040 0x82000040 0x00000020>,
> +                                <0x00000001 0x02000800 0x82000800 0x00000020>,
> +                                <0x00000001 0x02000820 0x82000820 0x00000020>,
> +                                <0x00000001 0x02000900 0x82000900 0x00000020>,
> +                                <0x00000001 0x02000920 0x82000920 0x00000020>,
> +                                <0x00000001 0x02000940 0x82000940 0x00000020>,
> +                                <0x00000001 0x00000300 0xf9000300 0x00000010>,
> +                                <0x00000001 0x02000000 0x82000000 0x00000010>;
> +
> +                        uio_cp_eng@0xf9000000 {
> +                                compatible = "generic-uio";
> +                                reg = <0x00000000 0x00000000 0x00001000>;
> +                                status = "okay";
> +                        };
> +		};
> +	};
> +};
> +
> +&uart0 {
> +	status = "okay";
> +};
> +
> +&spi0 {
> +	status = "okay";
> +
> +	spidev: spidev@0 {
> +		status = "okay";
> +		compatible = "linux,spidev";

No, for multiple reasons:
1. Did you see anywhere such usage?
2. Did you see compatible documented?
3. Did you see the warning when booting the kernel?

> +		spi-max-frequency = <25000000>;
> +		reg = <0>;
> +	};
> +};
> +
> +&watchdog0 {
> +	status = "okay";
> +};
> +
> +&fpga_mgr {
> +	status = "disabled";
> +};


Best regards,
Krzysztof