Re: [PATCH] arm64: dts: intel: add device tree for n6000

[matthew . gerlach]

On Tue, 3 May 2022, Krzysztof Kozlowski wrote:

> On 02/05/2022 18:58, matthew.gerlach@xxxxxxxxxxxxxxx wrote:
> > From: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> >
> > Add a device tree for the n6000 instantiation of Agilex
> > Hard Processor System (HPS).
> >
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> > ---
> >  arch/arm64/boot/dts/intel/Makefile            |  1 +
> >  .../boot/dts/intel/socfpga_agilex_n6000.dts   | 77 +++++++++++++++++++
> >  2 files changed, 78 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts
> >
> > diff --git a/arch/arm64/boot/dts/intel/Makefile b/arch/arm64/boot/dts/intel/Makefile
> > index 0b5477442263..1425853877cc 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_n6000.dtb \
> >  				socfpga_n5x_socdk.dtb
> >  dtb-$(CONFIG_ARCH_KEEMBAY) += keembay-evm.dtb
> > diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts b/arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts
> > new file mode 100644
> > index 000000000000..07f5a5983e5c
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/intel/socfpga_agilex_n6000.dts
> > @@ -0,0 +1,77 @@
> > +// SPDX-License-Identifier:     GPL-2.0
>
> Except what Rob said: remove the indentation before license.

Indentation will be removed.  I noticed my checkpatch.pl execution was 
failing due to missing python module, git.  I installed the git module, 
but the script didn't report an error.

> > +/*
> > + * Copyright (C) 2021-2022, Intel Corporation
> > + */
> > +#include "socfpga_agilex.dtsi"
> > +
> > +/ {
> > +	model = "SoCFPGA Agilex n6000";
> > +
> > +	aliases {
> > +		serial0 = &uart1;
> > +		serial1 = &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 {
> > +				clock-frequency = <25000000>;
>
> This does not look like SoC property... If it is part of Soc, why it is
> not provided by clock controller? Where compatible?
>
> If you intended to override nodes, override by label, not by path.

I will make this change.

> Best regards,
> Krzysztof

Thank you for the review.

Matthew