mbox series

[v1,00/10] Introduce ASPEED AST27XX BMC SoC

Message ID 20240726110355.2181563-1-kevin_chen@aspeedtech.com
Headers show
Series Introduce ASPEED AST27XX BMC SoC | expand

Message

Kevin Chen July 26, 2024, 11:03 a.m. UTC
This patchset adds initial support for the ASPEED.
AST27XX Board Management controller (BMC) SoC family.

AST2700 is ASPEED's 8th-generation server management processor.
Featuring a quad-core ARM Cortex A35 64-bit processor and two
independent ARM Cortex M4 processors

This patchset adds minimal architecture and drivers such as:
Clocksource, Clock and Reset

This patchset was tested on the ASPEED AST2700 evaluation board.

Kevin Chen (10):
  dt-binding: mfd: aspeed,ast2x00-scu: Add binding for ASPEED AST2700
    SCU
  dt-binding: clk: ast2700: Add binding for Aspeed AST27xx Clock
  clk: ast2700: add clock controller
  dt-bindings: reset: ast2700: Add binding for ASPEED AST2700 Reset
  dt-bindings: arm: aspeed: Add maintainer
  dt-bindings: arm: aspeed: Add aspeed,ast2700-evb compatible string
  arm64: aspeed: Add support for ASPEED AST2700 BMC SoC
  arm64: dts: aspeed: Add initial AST27XX device tree
  arm64: dts: aspeed: Add initial AST2700 EVB device tree
  arm64: defconfig: Add ASPEED AST2700 family support

 .../bindings/arm/aspeed/aspeed.yaml           |    6 +
 .../bindings/mfd/aspeed,ast2x00-scu.yaml      |    3 +
 MAINTAINERS                                   |    3 +
 arch/arm64/Kconfig.platforms                  |   14 +
 arch/arm64/boot/dts/Makefile                  |    1 +
 arch/arm64/boot/dts/aspeed/Makefile           |    4 +
 arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi     |  217 +++
 arch/arm64/boot/dts/aspeed/ast2700-evb.dts    |   50 +
 arch/arm64/configs/defconfig                  |    1 +
 drivers/clk/Makefile                          |    1 +
 drivers/clk/clk-ast2700.c                     | 1166 +++++++++++++++++
 .../dt-bindings/clock/aspeed,ast2700-clk.h    |  180 +++
 .../dt-bindings/reset/aspeed,ast2700-reset.h  |  126 ++
 13 files changed, 1772 insertions(+)
 create mode 100644 arch/arm64/boot/dts/aspeed/Makefile
 create mode 100644 arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
 create mode 100644 arch/arm64/boot/dts/aspeed/ast2700-evb.dts
 create mode 100644 drivers/clk/clk-ast2700.c
 create mode 100644 include/dt-bindings/clock/aspeed,ast2700-clk.h
 create mode 100644 include/dt-bindings/reset/aspeed,ast2700-reset.h

Comments

Krzysztof Kozlowski July 26, 2024, 11:13 a.m. UTC | #1
On 26/07/2024 13:03, Kevin Chen wrote:
> Signed-off-by: Kevin Chen <kevin_chen@aspeedtech.com>

So you did not write commit msgs to none of the commits?

> ---
>  drivers/clk/Makefile      |    1 +
>  drivers/clk/clk-ast2700.c | 1166 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 1167 insertions(+)
>  create mode 100644 drivers/clk/clk-ast2700.c
> 
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index f793a16cad40..0d5992ea0fa4 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_COMMON_CLK_FSL_SAI)	+= clk-fsl-sai.o
>  obj-$(CONFIG_COMMON_CLK_GEMINI)		+= clk-gemini.o
>  obj-$(CONFIG_COMMON_CLK_ASPEED)		+= clk-aspeed.o
>  obj-$(CONFIG_MACH_ASPEED_G6)		+= clk-ast2600.o
> +obj-$(CONFIG_MACH_ASPEED_G7)		+= clk-ast2700.o

...

> +
> +static const char *const pspclk_sel[] = {
> +	"soc0-mpll",
> +	"soc0-hpll",
> +};
> +
> +static const char *const soc0_uartclk_sel[] = {
> +	"soc0-clk24Mhz",
> +	"soc0-clk192Mhz",
> +};
> +
> +static const char *const emmcclk_sel[] = {
> +	"soc0-mpll_div4",
> +	"soc0-hpll_div4",
> +};
> +
> +static int ast2700_soc0_clk_init(struct device_node *soc0_node)
> +{
> +	struct clk_hw_onecell_data *clk_data;
> +	void __iomem *clk_base;
> +	struct ast2700_reset *reset;
> +	struct clk_hw **clks;
> +	int div;
> +	u32 val;
> +	int ret;
> +
> +	clk_data = kzalloc(struct_size(clk_data, hws, SOC0_NUM_CLKS), GFP_KERNEL);
> +	if (!clk_data)
> +		return -ENOMEM;
> +
> +	clk_data->num = SOC0_NUM_CLKS;
> +	clks = clk_data->hws;
> +
> +	clk_base = of_iomap(soc0_node, 0);
> +	if (WARN_ON(IS_ERR(clk_base)))

Drop WARN_ON

> +		return PTR_ERR(clk_base);
> +
> +	reset = kzalloc(sizeof(*reset), GFP_KERNEL);
> +	if (!reset)
> +		return -ENOMEM;
> +
> +	reset->base = clk_base;
> +
> +	reset->rcdev.owner = THIS_MODULE;
> +	reset->rcdev.nr_resets = SOC0_RESET_NUMS;
> +	reset->rcdev.ops = &ast2700_reset_ops;
> +	reset->rcdev.of_node = soc0_node;
> +
> +	ret = reset_controller_register(&reset->rcdev);
> +	if (ret) {
> +		pr_err("soc0 failed to register reset controller\n");
> +		return ret;
> +	}
> +
> +	//refclk

Weird comment. Please read Coding Style.


> +	clks[SCU0_CLKIN] =
> +		clk_hw_register_fixed_rate(NULL, "soc0-clkin", NULL, 0, SCU_CLK_25MHZ);
> +
> +	clks[SCU0_CLK_24M] =
> +		clk_hw_register_fixed_rate(NULL, "soc0-clk24Mhz", NULL, 0, SCU_CLK_24MHZ);
> +
> +	clks[SCU0_CLK_192M] =
> +		clk_hw_register_fixed_rate(NULL, "soc0-clk192Mhz", NULL, 0, SCU_CLK_192MHZ);
> +
> +	//hpll
> +	val = readl(clk_base + SCU0_HWSTRAP1);
> +	if ((val & GENMASK(3, 2)) != 0) {
> +		switch ((val & GENMASK(3, 2)) >> 2) {
> +		case 1:
> +			clks[SCU0_CLK_HPLL] =
> +				clk_hw_register_fixed_rate(NULL, "soc0-hpll", NULL, 0, 1900000000);
> +			break;
> +		case 2:
> +			clks[SCU0_CLK_HPLL] =
> +				clk_hw_register_fixed_rate(NULL, "soc0-hpll", NULL, 0, 1800000000);
> +			break;
> +		case 3:
> +			clks[SCU0_CLK_HPLL] =
> +				clk_hw_register_fixed_rate(NULL, "soc0-hpll", NULL, 0, 1700000000);
> +			break;
> +		}
> +	} else {
> +		val = readl(clk_base + SCU0_HPLL_PARAM);
> +		clks[SCU0_CLK_HPLL] = ast2700_soc0_hw_pll("soc0-hpll", "soc0-clkin", val);
> +	}
> +	clks[SCU0_CLK_HPLL_DIV2] = clk_hw_register_fixed_factor(NULL, "soc0-hpll_div2", "soc0-hpll", 0, 1, 2);
> +	clks[SCU0_CLK_HPLL_DIV4] = clk_hw_register_fixed_factor(NULL, "soc0-hpll_div4", "soc0-hpll", 0, 1, 4);
> +
> +	//dpll



Best regards,
Krzysztof
Krzysztof Kozlowski July 26, 2024, 11:16 a.m. UTC | #2
On 26/07/2024 13:03, Kevin Chen wrote:
> ---
>  MAINTAINERS                  |  3 +++
>  arch/arm64/Kconfig.platforms | 14 ++++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c0a3d9e93689..08609430cfe0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2121,7 +2121,10 @@ Q:	https://patchwork.ozlabs.org/project/linux-aspeed/list/
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/joel/bmc.git
>  F:	Documentation/devicetree/bindings/arm/aspeed/
>  F:	arch/arm/boot/dts/aspeed/
> +F:	arch/arm64/boot/dts/aspeed/
>  F:	arch/arm/mach-aspeed/
> +F:	include/dt-bindings/clock/aspeed,ast2700-clk.h
> +F:	include/dt-bindings/reset/aspeed,ast2700-reset.h
>  N:	aspeed
>  
>  ARM/AXM LSI SOC
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 6c6d11536b42..1db7b6f1ee0a 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -40,6 +40,20 @@ config ARCH_APPLE
>  	  This enables support for Apple's in-house ARM SoC family, starting
>  	  with the Apple M1.
>  
> +config ARCH_ASPEED
> +	bool "Aspeed SoC family"
> +	select MACH_ASPEED_G7
> +	help
> +	  Say yes if you intend to run on an Aspeed ast2700 or similar
> +	  seventh generation Aspeed BMCs.
> +
> +config MACH_ASPEED_G7
> +	bool "Aspeed SoC AST2700"

There are no MACHines for arm64. Look at this code. Do you see MACH
anywhere else? No. Then why Aspeed must be different?

No. Drop.

Best regards,
Krzysztof
Krzysztof Kozlowski July 26, 2024, 11:16 a.m. UTC | #3
On 26/07/2024 13:03, Kevin Chen wrote:
> ---
>  arch/arm64/boot/dts/aspeed/Makefile        |  4 ++
>  arch/arm64/boot/dts/aspeed/ast2700-evb.dts | 50 ++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/aspeed/Makefile
>  create mode 100644 arch/arm64/boot/dts/aspeed/ast2700-evb.dts
> 
> diff --git a/arch/arm64/boot/dts/aspeed/Makefile b/arch/arm64/boot/dts/aspeed/Makefile
> new file mode 100644
> index 000000000000..ffe7e15017cc
> --- /dev/null
> +++ b/arch/arm64/boot/dts/aspeed/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +dtb-$(CONFIG_ARCH_ASPEED) += \
> +	ast2700-evb.dtb
> diff --git a/arch/arm64/boot/dts/aspeed/ast2700-evb.dts b/arch/arm64/boot/dts/aspeed/ast2700-evb.dts
> new file mode 100644
> index 000000000000..187c458e566b
> --- /dev/null
> +++ b/arch/arm64/boot/dts/aspeed/ast2700-evb.dts
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/dts-v1/;
> +
> +#include "aspeed-g7.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +
> +/ {
> +	model = "AST2700A1-EVB";
> +	compatible = "aspeed,ast2700a1-evb", "aspeed,ast2700";

You have never tested this.

Sorry, test your DTS first.

> +
> +	chosen {
> +		bootargs = "console=ttyS12,115200n8";

Drop.

> +		stdout-path = &uart12;
> +	};
> +
> +	firmware {
> +		optee: optee {
> +			compatible = "linaro,optee-tz";
> +			method = "smc";
> +		};
> +	};
> +
> +	memory@400000000 {
> +		device_type = "memory";
> +		reg = <0x4 0x00000000 0x0 0x40000000>;
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		mcu_fw: mcu-firmware@42fe00000 {
> +			reg = <0x4 0x2fe00000 0x0 0x200000>;
> +			no-map;
> +		};
> +
> +		atf: trusted-firmware-a@430000000 {
> +			reg = <0x4 0x30000000 0x0 0x80000>;
> +			no-map;
> +		};
> +
> +		optee_core: optee_core@430080000 {

Read DTS coding style.

> +			reg = <0x4 0x30080000 0x0 0x1000000>;
> +			no-map;
> +		};
> +	};
> +};
> +

Remove stray blank line.

Best regards,
Krzysztof
Krzysztof Kozlowski July 26, 2024, 11:19 a.m. UTC | #4
On 26/07/2024 13:03, Kevin Chen wrote:
> ---
>  arch/arm64/boot/dts/Makefile              |   1 +
>  arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi | 217 ++++++++++++++++++++++
>  2 files changed, 218 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
> 
> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> index 21cd3a87f385..c909c19dc5dd 100644
> --- a/arch/arm64/boot/dts/Makefile
> +++ b/arch/arm64/boot/dts/Makefile
> @@ -34,3 +34,4 @@ subdir-y += tesla
>  subdir-y += ti
>  subdir-y += toshiba
>  subdir-y += xilinx
> +subdir-y += aspeed
> diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
> new file mode 100644
> index 000000000000..858ab95251e4
> --- /dev/null
> +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <dt-bindings/clock/aspeed,ast2700-clk.h>
> +#include <dt-bindings/reset/aspeed,ast2700-reset.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/interrupt-controller/aspeed-scu-ic.h>
> +
> +/ {
> +	model = "Aspeed BMC";

Model of what? No, drop.

> +	compatible = "aspeed,ast2700";

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.


> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +	interrupt-parent = <&gic>;
> +
> +	aliases {
> +		serial12 = &uart12;

Nope. Such aliases are board specific.

> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu@0 {
> +			compatible = "arm,cortex-a35";
> +			enable-method = "psci";
> +			device_type = "cpu";
> +			reg = <0>;
> +			d-cache-size = <0x8000>;
> +			d-cache-line-size = <64>;
> +			d-cache-sets = <128>;
> +			i-cache-size = <0x8000>;
> +			i-cache-line-size = <64>;
> +			i-cache-sets = <256>;
> +			next-level-cache = <&l2>;
> +		};
> +
> +		cpu@1 {
> +			compatible = "arm,cortex-a35";
> +			enable-method = "psci";
> +			device_type = "cpu";
> +			reg = <1>;
> +			d-cache-size = <0x8000>;
> +			d-cache-line-size = <64>;
> +			d-cache-sets = <128>;
> +			i-cache-size = <0x8000>;
> +			i-cache-line-size = <64>;
> +			i-cache-sets = <256>;
> +			next-level-cache = <&l2>;
> +		};
> +
> +		cpu@2 {
> +			compatible = "arm,cortex-a35";
> +			enable-method = "psci";
> +			device_type = "cpu";
> +			reg = <2>;
> +			d-cache-size = <0x8000>;
> +			d-cache-line-size = <64>;
> +			d-cache-sets = <128>;
> +			i-cache-size = <0x8000>;
> +			i-cache-line-size = <64>;
> +			i-cache-sets = <256>;
> +			next-level-cache = <&l2>;
> +		};
> +
> +		cpu@3 {
> +			compatible = "arm,cortex-a35";
> +			enable-method = "psci";
> +			device_type = "cpu";
> +			reg = <3>;
> +			d-cache-size = <0x8000>;
> +			d-cache-line-size = <64>;
> +			d-cache-sets = <128>;
> +			i-cache-size = <0x8000>;
> +			i-cache-line-size = <64>;
> +			i-cache-sets = <256>;
> +			next-level-cache = <&l2>;
> +		};
> +
> +		l2: l2-cache0 {
> +			compatible = "cache";
> +			cache-size = <0x80000>;
> +			cache-line-size = <64>;
> +			cache-sets = <1024>;
> +			cache-level = <2>;
> +		};
> +	};
> +
> +	pmu {
> +		compatible = "arm,cortex-a35-pmu";
> +		interrupt-parent = <&gic>;
> +		interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>;
> +	};
> +
> +	psci {

Order the nodes according to DTS coding style.

Fix all your patches:
1. To pass flawlessly checkpatch (you did not run it)
2. To pass dt_binding_check and dtbs_check (you did not run these)
3. To adhere to kernel coding style
4. To adhere to DTS coding style

> +		compatible = "arm,psci-1.0";
> +		method = "smc";
> +	};
> +
> +	gic: interrupt-controller@12200000 {

Nope, this cannot be outside of SoC.

> +		compatible = "arm,gic-v3";
> +		interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_HIGH)>;
> +		#interrupt-cells = <3>;
> +		interrupt-controller;
> +		interrupt-parent = <&gic>;
> +		#redistributor-regions = <1>;
> +		reg =	<0 0x12200000 0 0x10000>,		//GICD
> +			<0 0x12280000 0 0x80000>,		//GICR
> +			<0 0x40440000 0 0x1000>;		//GICC

Read DTS coding style and order this correctly.

> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupt-parent = <&gic>;
> +		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)>;
> +		arm,cpu-registers-not-fw-configured;
> +		always-on;
> +	};
> +
> +	soc0: soc@10000000 {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		soc0_sram: sram@10000000 {
> +			compatible = "mmio-sram";
> +			reg = <0x0 0x10000000 0x0 0x20000>;	/* 128KiB SRAM on soc0 */
> +			ranges = <0x0 0x0 0x0 0x10000000 0x0 0x20000>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			no-memory-wc;
> +
> +			exported@0 {
> +				reg = <0 0x0 0 0x20000>;
> +				export;
> +			};
> +		};
> +
> +		syscon0: syscon@12c02000 {
> +			compatible = "aspeed,ast2700-scu0", "syscon", "simple-mfd";
> +			reg = <0x0 0x12c02000 0x0 0x1000>;
> +			ranges = <0x0 0x0 0 0x12c02000 0 0x1000>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +
> +			silicon-id@0 {
> +				compatible = "aspeed,ast2700-silicon-id", "aspeed,silicon-id";
> +				reg = <0 0x0 0 0x4>;
> +			};
> +
> +			scu_ic0: interrupt-controller@1D0 {

DTS coding style...

> +				#interrupt-cells = <1>;
> +				compatible = "aspeed,ast2700-scu-ic0";
> +				reg = <0 0x1d0 0 0xc>;
> +				interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
> +				interrupt-controller;
> +			};
> +
> +			scu_ic1: interrupt-controller@1E0 {
> +				#interrupt-cells = <1>;
> +				compatible = "aspeed,ast2700-scu-ic1";
> +				reg = <0 0x1e0 0 0xc>;
> +				interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> +				interrupt-controller;
> +			};
> +
> +			soc0_rst: reset-controller@200 {
> +				reg = <0 0x200 0 0x40>;
> +			};
> +
> +			soc0_clk: clock-controller@240 {
> +				reg = <0 0x240 0 0x1c0>;
> +			};
> +		};
> +
> +	};
> +
> +	soc1: soc@14000000 {

Wait, what, to socs?

> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		syscon1: syscon@14c02000 {
> +			compatible = "aspeed,ast2700-scu1", "syscon", "simple-mfd";
> +			reg = <0x0 0x14c02000 0x0 0x1000>;
> +			ranges = <0x0 0x0 0x0 0x14c02000 0x0 0x1000>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +
> +			soc1_rst: reset-controller@200 {
> +				#reset-cells = <1>;
> +			};
> +
> +			soc1_clk: clock-controller@240 {
> +				reg = <0 0x240 0 0x1c0>;
> +			};
> +		};
> +
> +		uart12: serial@14c33b00 {
> +			compatible = "ns16550a";
> +			reg = <0x0 0x14c33b00 0x0 0x100>;
> +			reg-shift = <2>;
> +			reg-io-width = <4>;
> +			clocks = <&syscon1 SCU1_CLK_GATE_UART12CLK>;
> +			no-loopback-test;
> +			pinctrl-names = "default";

What is this?

> +		};
> +	};
> +};
> +

Best regards,
Krzysztof
Rob Herring (Arm) July 26, 2024, 1:09 p.m. UTC | #5
On Fri, 26 Jul 2024 19:03:45 +0800, Kevin Chen wrote:
> This patchset adds initial support for the ASPEED.
> AST27XX Board Management controller (BMC) SoC family.
> 
> AST2700 is ASPEED's 8th-generation server management processor.
> Featuring a quad-core ARM Cortex A35 64-bit processor and two
> independent ARM Cortex M4 processors
> 
> This patchset adds minimal architecture and drivers such as:
> Clocksource, Clock and Reset
> 
> This patchset was tested on the ASPEED AST2700 evaluation board.
> 
> Kevin Chen (10):
>   dt-binding: mfd: aspeed,ast2x00-scu: Add binding for ASPEED AST2700
>     SCU
>   dt-binding: clk: ast2700: Add binding for Aspeed AST27xx Clock
>   clk: ast2700: add clock controller
>   dt-bindings: reset: ast2700: Add binding for ASPEED AST2700 Reset
>   dt-bindings: arm: aspeed: Add maintainer
>   dt-bindings: arm: aspeed: Add aspeed,ast2700-evb compatible string
>   arm64: aspeed: Add support for ASPEED AST2700 BMC SoC
>   arm64: dts: aspeed: Add initial AST27XX device tree
>   arm64: dts: aspeed: Add initial AST2700 EVB device tree
>   arm64: defconfig: Add ASPEED AST2700 family support
> 
>  .../bindings/arm/aspeed/aspeed.yaml           |    6 +
>  .../bindings/mfd/aspeed,ast2x00-scu.yaml      |    3 +
>  MAINTAINERS                                   |    3 +
>  arch/arm64/Kconfig.platforms                  |   14 +
>  arch/arm64/boot/dts/Makefile                  |    1 +
>  arch/arm64/boot/dts/aspeed/Makefile           |    4 +
>  arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi     |  217 +++
>  arch/arm64/boot/dts/aspeed/ast2700-evb.dts    |   50 +
>  arch/arm64/configs/defconfig                  |    1 +
>  drivers/clk/Makefile                          |    1 +
>  drivers/clk/clk-ast2700.c                     | 1166 +++++++++++++++++
>  .../dt-bindings/clock/aspeed,ast2700-clk.h    |  180 +++
>  .../dt-bindings/reset/aspeed,ast2700-reset.h  |  126 ++
>  13 files changed, 1772 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/aspeed/Makefile
>  create mode 100644 arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
>  create mode 100644 arch/arm64/boot/dts/aspeed/ast2700-evb.dts
>  create mode 100644 drivers/clk/clk-ast2700.c
>  create mode 100644 include/dt-bindings/clock/aspeed,ast2700-clk.h
>  create mode 100644 include/dt-bindings/reset/aspeed,ast2700-reset.h
> 
> --
> 2.34.1
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y aspeed/ast2700-evb.dtb' for 20240726110355.2181563-1-kevin_chen@aspeedtech.com:

arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: /: failed to match any schema with compatible: ['aspeed,ast2700a1-evb', 'aspeed,ast2700']
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: /: failed to match any schema with compatible: ['aspeed,ast2700a1-evb', 'aspeed,ast2700']
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: l2-cache0: 'cache-unified' is a dependency of 'cache-size'
	from schema $id: http://devicetree.org/schemas/cache.yaml#
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: l2-cache0: 'cache-unified' is a dependency of 'cache-sets'
	from schema $id: http://devicetree.org/schemas/cache.yaml#
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: l2-cache0: 'cache-unified' is a dependency of 'cache-line-size'
	from schema $id: http://devicetree.org/schemas/cache.yaml#
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: l2-cache0: 'cache-unified' is a required property
	from schema $id: http://devicetree.org/schemas/cache.yaml#
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: l2-cache0: Unevaluated properties are not allowed ('cache-level', 'cache-line-size', 'cache-sets', 'cache-size' were unexpected)
	from schema $id: http://devicetree.org/schemas/cache.yaml#
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: sram@10000000: #address-cells: 1 was expected
	from schema $id: http://devicetree.org/schemas/sram/sram.yaml#
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: sram@10000000: #size-cells: 1 was expected
	from schema $id: http://devicetree.org/schemas/sram/sram.yaml#
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: sram@10000000: 'exported@0' does not match any of the regexes: '^([a-z0-9]*-)?sram(-section)?@[a-f0-9]+$', 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/sram/sram.yaml#
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: /soc@10000000/syscon@12c02000: failed to match any schema with compatible: ['aspeed,ast2700-scu0', 'syscon', 'simple-mfd']
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: /soc@10000000/syscon@12c02000/interrupt-controller@1D0: failed to match any schema with compatible: ['aspeed,ast2700-scu-ic0']
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: /soc@10000000/syscon@12c02000/interrupt-controller@1E0: failed to match any schema with compatible: ['aspeed,ast2700-scu-ic1']
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: /soc@14000000/syscon@14c02000: failed to match any schema with compatible: ['aspeed,ast2700-scu1', 'syscon', 'simple-mfd']
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: serial@14c33b00: 'oneOf' conditional failed, one must be fixed:
	'interrupts' is a required property
	'interrupts-extended' is a required property
	from schema $id: http://devicetree.org/schemas/serial/8250.yaml#
arch/arm64/boot/dts/aspeed/ast2700-evb.dtb: serial@14c33b00: 'pinctrl-0' is a dependency of 'pinctrl-names'
	from schema $id: http://devicetree.org/schemas/pinctrl/pinctrl-consumer.yaml#
Krzysztof Kozlowski July 26, 2024, 1:33 p.m. UTC | #6
On 26/07/2024 15:09, Rob Herring (Arm) wrote:
> 
> On Fri, 26 Jul 2024 19:03:45 +0800, Kevin Chen wrote:
>> This patchset adds initial support for the ASPEED.
>> AST27XX Board Management controller (BMC) SoC family.
>>
>> AST2700 is ASPEED's 8th-generation server management processor.
>> Featuring a quad-core ARM Cortex A35 64-bit processor and two
>> independent ARM Cortex M4 processors
>>
>> This patchset adds minimal architecture and drivers such as:
>> Clocksource, Clock and Reset
>>
>> This patchset was tested on the ASPEED AST2700 evaluation board.
>>
>> Kevin Chen (10):
>>   dt-binding: mfd: aspeed,ast2x00-scu: Add binding for ASPEED AST2700
>>     SCU
>>   dt-binding: clk: ast2700: Add binding for Aspeed AST27xx Clock
>>   clk: ast2700: add clock controller
>>   dt-bindings: reset: ast2700: Add binding for ASPEED AST2700 Reset
>>   dt-bindings: arm: aspeed: Add maintainer
>>   dt-bindings: arm: aspeed: Add aspeed,ast2700-evb compatible string
>>   arm64: aspeed: Add support for ASPEED AST2700 BMC SoC
>>   arm64: dts: aspeed: Add initial AST27XX device tree
>>   arm64: dts: aspeed: Add initial AST2700 EVB device tree
>>   arm64: defconfig: Add ASPEED AST2700 family support
>>
>>  .../bindings/arm/aspeed/aspeed.yaml           |    6 +
>>  .../bindings/mfd/aspeed,ast2x00-scu.yaml      |    3 +
>>  MAINTAINERS                                   |    3 +
>>  arch/arm64/Kconfig.platforms                  |   14 +
>>  arch/arm64/boot/dts/Makefile                  |    1 +
>>  arch/arm64/boot/dts/aspeed/Makefile           |    4 +
>>  arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi     |  217 +++
>>  arch/arm64/boot/dts/aspeed/ast2700-evb.dts    |   50 +
>>  arch/arm64/configs/defconfig                  |    1 +
>>  drivers/clk/Makefile                          |    1 +
>>  drivers/clk/clk-ast2700.c                     | 1166 +++++++++++++++++
>>  .../dt-bindings/clock/aspeed,ast2700-clk.h    |  180 +++
>>  .../dt-bindings/reset/aspeed,ast2700-reset.h  |  126 ++
>>  13 files changed, 1772 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/aspeed/Makefile
>>  create mode 100644 arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
>>  create mode 100644 arch/arm64/boot/dts/aspeed/ast2700-evb.dts
>>  create mode 100644 drivers/clk/clk-ast2700.c
>>  create mode 100644 include/dt-bindings/clock/aspeed,ast2700-clk.h
>>  create mode 100644 include/dt-bindings/reset/aspeed,ast2700-reset.h
>>
>> --
>> 2.34.1
>>
>>
>>
> 
> 
> My bot found new DTB warnings on the .dts files added or changed in this
> series.
> 
> Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
> are fixed by another series. Ultimately, it is up to the platform
> maintainer whether these warnings are acceptable or not. No need to reply
> unless the platform maintainer has comments.
> 
> If you already ran DT checks and didn't see these error(s), then
> make sure dt-schema is up to date:
> 
>   pip3 install dtschema --upgrade
> 
> 
> New warnings running 'make CHECK_DTBS=y aspeed/ast2700-evb.dtb' for 20240726110355.2181563-1-kevin_chen@aspeedtech.com:

Kevin,
Just to clarify. Looking at the patches it was quite obvious you did not
test it with dtbs_check. For a new arm64 platform without any legacy,
having 0 warnings is a must.

Consider Documentation/process/maintainer-soc-clean-dts.rst being
implied for this platform.

Best regards,
Krzysztof
Kevin Chen Aug. 16, 2024, 4:06 a.m. UTC | #7
Hi Krzk,

I will separate clock part in the v3 patch into Ryan's clock series.

>
> On 26/07/2024 15:09, Rob Herring (Arm) wrote:
> >
> > On Fri, 26 Jul 2024 19:03:45 +0800, Kevin Chen wrote:
> >> This patchset adds initial support for the ASPEED.
> >> AST27XX Board Management controller (BMC) SoC family.
> >>
> >> AST2700 is ASPEED's 8th-generation server management processor.
> >> Featuring a quad-core ARM Cortex A35 64-bit processor and two
> >> independent ARM Cortex M4 processors
> >>
> >> This patchset adds minimal architecture and drivers such as:
> >> Clocksource, Clock and Reset
> >>
> >> This patchset was tested on the ASPEED AST2700 evaluation board.
> >>
> >> Kevin Chen (10):
> >>   dt-binding: mfd: aspeed,ast2x00-scu: Add binding for ASPEED AST2700
> >>     SCU
> >>   dt-binding: clk: ast2700: Add binding for Aspeed AST27xx Clock
> >>   clk: ast2700: add clock controller
> >>   dt-bindings: reset: ast2700: Add binding for ASPEED AST2700 Reset
> >>   dt-bindings: arm: aspeed: Add maintainer
> >>   dt-bindings: arm: aspeed: Add aspeed,ast2700-evb compatible string
> >>   arm64: aspeed: Add support for ASPEED AST2700 BMC SoC
> >>   arm64: dts: aspeed: Add initial AST27XX device tree
> >>   arm64: dts: aspeed: Add initial AST2700 EVB device tree
> >>   arm64: defconfig: Add ASPEED AST2700 family support
> >>
> >>  .../bindings/arm/aspeed/aspeed.yaml           |    6 +
> >>  .../bindings/mfd/aspeed,ast2x00-scu.yaml      |    3 +
> >>  MAINTAINERS                                   |    3 +
> >>  arch/arm64/Kconfig.platforms                  |   14 +
> >>  arch/arm64/boot/dts/Makefile                  |    1 +
> >>  arch/arm64/boot/dts/aspeed/Makefile           |    4 +
> >>  arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi     |  217 +++
> >>  arch/arm64/boot/dts/aspeed/ast2700-evb.dts    |   50 +
> >>  arch/arm64/configs/defconfig                  |    1 +
> >>  drivers/clk/Makefile                          |    1 +
> >>  drivers/clk/clk-ast2700.c                     | 1166
> +++++++++++++++++
> >>  .../dt-bindings/clock/aspeed,ast2700-clk.h    |  180 +++
> >>  .../dt-bindings/reset/aspeed,ast2700-reset.h  |  126 ++
> >>  13 files changed, 1772 insertions(+)  create mode 100644
> >> arch/arm64/boot/dts/aspeed/Makefile
> >>  create mode 100644 arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
> >>  create mode 100644 arch/arm64/boot/dts/aspeed/ast2700-evb.dts
> >>  create mode 100644 drivers/clk/clk-ast2700.c  create mode 100644
> >> include/dt-bindings/clock/aspeed,ast2700-clk.h
> >>  create mode 100644 include/dt-bindings/reset/aspeed,ast2700-reset.h
> >>
> >> --
> >> 2.34.1
> >>
> >>
> >>
> >
> >
> > My bot found new DTB warnings on the .dts files added or changed in
> > this series.
> >
> > Some warnings may be from an existing SoC .dtsi. Or perhaps the
> > warnings are fixed by another series. Ultimately, it is up to the
> > platform maintainer whether these warnings are acceptable or not. No
> > need to reply unless the platform maintainer has comments.
> >
> > If you already ran DT checks and didn't see these error(s), then make
> > sure dt-schema is up to date:
> >
> >   pip3 install dtschema --upgrade
> >
> >
> > New warnings running 'make CHECK_DTBS=y aspeed/ast2700-evb.dtb' for
> 20240726110355.2181563-1-kevin_chen@aspeedtech.com:
>
> Kevin,
> Just to clarify. Looking at the patches it was quite obvious you did not test it
> with dtbs_check. For a new arm64 platform without any legacy, having 0
> warnings is a must.
Agree.

>
> Consider Documentation/process/maintainer-soc-clean-dts.rst being implied
> for this platform.
>
> Best regards,
> Krzysztof

--
Best Regards,
Kevin.Chen
************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊,並受法律保護。如 台端非指定之收件者,請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
Kevin Chen Aug. 16, 2024, 4:06 a.m. UTC | #8
Hi Krzk,

I will separate clock part in the v3 patch into Ryan's clock series.
>
> So you did not write commit msgs to none of the commits?
>
> > ---
> >  drivers/clk/Makefile      |    1 +
> >  drivers/clk/clk-ast2700.c | 1166
> > +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 1167 insertions(+)
> >  create mode 100644 drivers/clk/clk-ast2700.c
> >
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index
> > f793a16cad40..0d5992ea0fa4 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -38,6 +38,7 @@ obj-$(CONFIG_COMMON_CLK_FSL_SAI)  += clk-fsl-sai.o
> >  obj-$(CONFIG_COMMON_CLK_GEMINI)            += clk-gemini.o
> >  obj-$(CONFIG_COMMON_CLK_ASPEED)            += clk-aspeed.o
> >  obj-$(CONFIG_MACH_ASPEED_G6)               += clk-ast2600.o
> > +obj-$(CONFIG_MACH_ASPEED_G7)               += clk-ast2700.o
>
> ...
>
> > +
> > +static const char *const pspclk_sel[] = {
> > +   "soc0-mpll",
> > +   "soc0-hpll",
> > +};
> > +
> > +static const char *const soc0_uartclk_sel[] = {
> > +   "soc0-clk24Mhz",
> > +   "soc0-clk192Mhz",
> > +};
> > +
> > +static const char *const emmcclk_sel[] = {
> > +   "soc0-mpll_div4",
> > +   "soc0-hpll_div4",
> > +};
> > +
> > +static int ast2700_soc0_clk_init(struct device_node *soc0_node) {
> > +   struct clk_hw_onecell_data *clk_data;
> > +   void __iomem *clk_base;
> > +   struct ast2700_reset *reset;
> > +   struct clk_hw **clks;
> > +   int div;
> > +   u32 val;
> > +   int ret;
> > +
> > +   clk_data = kzalloc(struct_size(clk_data, hws, SOC0_NUM_CLKS),
> GFP_KERNEL);
> > +   if (!clk_data)
> > +           return -ENOMEM;
> > +
> > +   clk_data->num = SOC0_NUM_CLKS;
> > +   clks = clk_data->hws;
> > +
> > +   clk_base = of_iomap(soc0_node, 0);
> > +   if (WARN_ON(IS_ERR(clk_base)))
>
> Drop WARN_ON
I remind Ryan for this fix. He will reply in these series.
https://patchwork.kernel.org/project/linux-clk/patch/20240808075937.2756733-5-ryan_chen@aspeedtech.com/

>
> > +           return PTR_ERR(clk_base);
> > +
> > +   reset = kzalloc(sizeof(*reset), GFP_KERNEL);
> > +   if (!reset)
> > +           return -ENOMEM;
> > +
> > +   reset->base = clk_base;
> > +
> > +   reset->rcdev.owner = THIS_MODULE;
> > +   reset->rcdev.nr_resets = SOC0_RESET_NUMS;
> > +   reset->rcdev.ops = &ast2700_reset_ops;
> > +   reset->rcdev.of_node = soc0_node;
> > +
> > +   ret = reset_controller_register(&reset->rcdev);
> > +   if (ret) {
> > +           pr_err("soc0 failed to register reset controller\n");
> > +           return ret;
> > +   }
> > +
> > +   //refclk
>
> Weird comment. Please read Coding Style.
I remind Ryan for this fix. He will reply in these series.
https://patchwork.kernel.org/project/linux-clk/patch/20240808075937.2756733-5-ryan_chen@aspeedtech.com/

>
>
> > +   clks[SCU0_CLKIN] =
> > +           clk_hw_register_fixed_rate(NULL, "soc0-clkin", NULL, 0,
> > +SCU_CLK_25MHZ);
> > +
> > +   clks[SCU0_CLK_24M] =
> > +           clk_hw_register_fixed_rate(NULL, "soc0-clk24Mhz", NULL, 0,
> > +SCU_CLK_24MHZ);
> > +
> > +   clks[SCU0_CLK_192M] =
> > +           clk_hw_register_fixed_rate(NULL, "soc0-clk192Mhz", NULL, 0,
> > +SCU_CLK_192MHZ);
> > +
> > +   //hpll
> > +   val = readl(clk_base + SCU0_HWSTRAP1);
> > +   if ((val & GENMASK(3, 2)) != 0) {
> > +           switch ((val & GENMASK(3, 2)) >> 2) {
> > +           case 1:
> > +                   clks[SCU0_CLK_HPLL] =
> > +                           clk_hw_register_fixed_rate(NULL, "soc0-hpll", NULL, 0,
> 1900000000);
> > +                   break;
> > +           case 2:
> > +                   clks[SCU0_CLK_HPLL] =
> > +                           clk_hw_register_fixed_rate(NULL, "soc0-hpll", NULL, 0,
> 1800000000);
> > +                   break;
> > +           case 3:
> > +                   clks[SCU0_CLK_HPLL] =
> > +                           clk_hw_register_fixed_rate(NULL, "soc0-hpll", NULL, 0,
> 1700000000);
> > +                   break;
> > +           }
> > +   } else {
> > +           val = readl(clk_base + SCU0_HPLL_PARAM);
> > +           clks[SCU0_CLK_HPLL] = ast2700_soc0_hw_pll("soc0-hpll",
> "soc0-clkin", val);
> > +   }
> > +   clks[SCU0_CLK_HPLL_DIV2] = clk_hw_register_fixed_factor(NULL,
> "soc0-hpll_div2", "soc0-hpll", 0, 1, 2);
> > +   clks[SCU0_CLK_HPLL_DIV4] = clk_hw_register_fixed_factor(NULL,
> > +"soc0-hpll_div4", "soc0-hpll", 0, 1, 4);
> > +
> > +   //dpll
>
>
>
> Best regards,
> Krzysztof

--
Best Regards,
Kevin.Chen
************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊,並受法律保護。如 台端非指定之收件者,請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
Kevin Chen Aug. 16, 2024, 4:07 a.m. UTC | #9
Hi Krzk,

> > ---
> >  arch/arm64/boot/dts/aspeed/Makefile        |  4 ++
> >  arch/arm64/boot/dts/aspeed/ast2700-evb.dts | 50
> ++++++++++++++++++++++
> >  2 files changed, 54 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/aspeed/Makefile
> >  create mode 100644 arch/arm64/boot/dts/aspeed/ast2700-evb.dts
> >
> > diff --git a/arch/arm64/boot/dts/aspeed/Makefile
> b/arch/arm64/boot/dts/aspeed/Makefile
> > new file mode 100644
> > index 000000000000..ffe7e15017cc
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/aspeed/Makefile
> > @@ -0,0 +1,4 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +dtb-$(CONFIG_ARCH_ASPEED) += \
> > +   ast2700-evb.dtb
> > diff --git a/arch/arm64/boot/dts/aspeed/ast2700-evb.dts
> b/arch/arm64/boot/dts/aspeed/ast2700-evb.dts
> > new file mode 100644
> > index 000000000000..187c458e566b
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/aspeed/ast2700-evb.dts
> > @@ -0,0 +1,50 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +/dts-v1/;
> > +
> > +#include "aspeed-g7.dtsi"
> > +#include <dt-bindings/gpio/aspeed-gpio.h>
> > +
> > +/ {
> > +   model = "AST2700A1-EVB";
> > +   compatible = "aspeed,ast2700a1-evb", "aspeed,ast2700";
>
> You have never tested this.
>
> Sorry, test your DTS first.
Agree. I will test my dts by make dtbs_check W=1

>
> > +
> > +   chosen {
> > +           bootargs = "console=ttyS12,115200n8";
>
> Drop.
Agree.

>
> > +           stdout-path = &uart12;
> > +   };
> > +
> > +   firmware {
> > +           optee: optee {
> > +                   compatible = "linaro,optee-tz";
> > +                   method = "smc";
> > +           };
> > +   };
> > +
> > +   memory@400000000 {
> > +           device_type = "memory";
> > +           reg = <0x4 0x00000000 0x0 0x40000000>;
> > +   };
> > +
> > +   reserved-memory {
> > +           #address-cells = <2>;
> > +           #size-cells = <2>;
> > +           ranges;
> > +
> > +           mcu_fw: mcu-firmware@42fe00000 {
> > +                   reg = <0x4 0x2fe00000 0x0 0x200000>;
> > +                   no-map;
> > +           };
> > +
> > +           atf: trusted-firmware-a@430000000 {
> > +                   reg = <0x4 0x30000000 0x0 0x80000>;
> > +                   no-map;
> > +           };
> > +
> > +           optee_core: optee_core@430080000 {
>
> Read DTS coding style.
Agree. Should I change to optee_core: optee-core@430080000 {

>
> > +                   reg = <0x4 0x30080000 0x0 0x1000000>;
> > +                   no-map;
> > +           };
> > +   };
> > +};
> > +
>
> Remove stray blank line.
Agree.

>
> Best regards,
> Krzysztof

--
Best Regards,
Kevin.Chen
************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊,並受法律保護。如 台端非指定之收件者,請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
Kevin Chen Aug. 16, 2024, 4:07 a.m. UTC | #10
Hi Krzk,

> > ---
> >  arch/arm64/boot/dts/Makefile              |   1 +
> >  arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi | 217
> > ++++++++++++++++++++++
> >  2 files changed, 218 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/Makefile
> > b/arch/arm64/boot/dts/Makefile index 21cd3a87f385..c909c19dc5dd
> 100644
> > --- a/arch/arm64/boot/dts/Makefile
> > +++ b/arch/arm64/boot/dts/Makefile
> > @@ -34,3 +34,4 @@ subdir-y += tesla
> >  subdir-y += ti
> >  subdir-y += toshiba
> >  subdir-y += xilinx
> > +subdir-y += aspeed
> > diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
> > b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
> > new file mode 100644
> > index 000000000000..858ab95251e4
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later #include
> > +<dt-bindings/clock/aspeed,ast2700-clk.h>
> > +#include <dt-bindings/reset/aspeed,ast2700-reset.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/interrupt-controller/aspeed-scu-ic.h>
> > +
> > +/ {
> > +   model = "Aspeed BMC";
>
> Model of what? No, drop.
Can I change to "model = "AST2700 EVB""

>
> > +   compatible = "aspeed,ast2700";
>
> Please run scripts/checkpatch.pl and fix reported warnings. Then please run
> `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
> Some warnings can be ignored, especially from --strict run, but the code here
> looks like it needs a fix. Feel free to get in touch if the warning is not clear.
>
>
> > +   #address-cells = <2>;
> > +   #size-cells = <2>;
> > +   interrupt-parent = <&gic>;
> > +
> > +   aliases {
> > +           serial12 = &uart12;
>
> Nope. Such aliases are board specific.
Agree, I will move to ast2700-evb.dts.

>
> > +   };
> > +
> > +   cpus {
> > +           #address-cells = <1>;
> > +           #size-cells = <0>;
> > +
> > +           cpu@0 {
> > +                   compatible = "arm,cortex-a35";
> > +                   enable-method = "psci";
> > +                   device_type = "cpu";
> > +                   reg = <0>;
> > +                   d-cache-size = <0x8000>;
> > +                   d-cache-line-size = <64>;
> > +                   d-cache-sets = <128>;
> > +                   i-cache-size = <0x8000>;
> > +                   i-cache-line-size = <64>;
> > +                   i-cache-sets = <256>;
> > +                   next-level-cache = <&l2>;
> > +           };
> > +
> > +           cpu@1 {
> > +                   compatible = "arm,cortex-a35";
> > +                   enable-method = "psci";
> > +                   device_type = "cpu";
> > +                   reg = <1>;
> > +                   d-cache-size = <0x8000>;
> > +                   d-cache-line-size = <64>;
> > +                   d-cache-sets = <128>;
> > +                   i-cache-size = <0x8000>;
> > +                   i-cache-line-size = <64>;
> > +                   i-cache-sets = <256>;
> > +                   next-level-cache = <&l2>;
> > +           };
> > +
> > +           cpu@2 {
> > +                   compatible = "arm,cortex-a35";
> > +                   enable-method = "psci";
> > +                   device_type = "cpu";
> > +                   reg = <2>;
> > +                   d-cache-size = <0x8000>;
> > +                   d-cache-line-size = <64>;
> > +                   d-cache-sets = <128>;
> > +                   i-cache-size = <0x8000>;
> > +                   i-cache-line-size = <64>;
> > +                   i-cache-sets = <256>;
> > +                   next-level-cache = <&l2>;
> > +           };
> > +
> > +           cpu@3 {
> > +                   compatible = "arm,cortex-a35";
> > +                   enable-method = "psci";
> > +                   device_type = "cpu";
> > +                   reg = <3>;
> > +                   d-cache-size = <0x8000>;
> > +                   d-cache-line-size = <64>;
> > +                   d-cache-sets = <128>;
> > +                   i-cache-size = <0x8000>;
> > +                   i-cache-line-size = <64>;
> > +                   i-cache-sets = <256>;
> > +                   next-level-cache = <&l2>;
> > +           };
> > +
> > +           l2: l2-cache0 {
> > +                   compatible = "cache";
> > +                   cache-size = <0x80000>;
> > +                   cache-line-size = <64>;
> > +                   cache-sets = <1024>;
> > +                   cache-level = <2>;
> > +           };
> > +   };
> > +
> > +   pmu {
> > +           compatible = "arm,cortex-a35-pmu";
> > +           interrupt-parent = <&gic>;
> > +           interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(2) |
> IRQ_TYPE_LEVEL_HIGH)>;
> > +   };
> > +
> > +   psci {
>
> Order the nodes according to DTS coding style.
>
> Fix all your patches:
> 1. To pass flawlessly checkpatch (you did not run it) 2. To pass
> dt_binding_check and dtbs_check (you did not run these) 3. To adhere to
> kernel coding style 4. To adhere to DTS coding style
Agree.

>
> > +           compatible = "arm,psci-1.0";
> > +           method = "smc";
> > +   };
> > +
> > +   gic: interrupt-controller@12200000 {
>
> Nope, this cannot be outside of SoC.
Agree.

>
> > +           compatible = "arm,gic-v3";
> > +           interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(2) |
> IRQ_TYPE_LEVEL_HIGH)>;
> > +           #interrupt-cells = <3>;
> > +           interrupt-controller;
> > +           interrupt-parent = <&gic>;
> > +           #redistributor-regions = <1>;
> > +           reg =   <0 0x12200000 0 0x10000>,               //GICD
> > +                   <0 0x12280000 0 0x80000>,               //GICR
> > +                   <0 0x40440000 0 0x1000>;                //GICC
>
> Read DTS coding style and order this correctly.
Agree.

>
> > +   };
> > +
> > +   timer {
> > +           compatible = "arm,armv8-timer";
> > +           interrupt-parent = <&gic>;
> > +           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)>;
> > +           arm,cpu-registers-not-fw-configured;
> > +           always-on;
> > +   };
> > +
> > +   soc0: soc@10000000 {
> > +           compatible = "simple-bus";
> > +           #address-cells = <2>;
> > +           #size-cells = <2>;
> > +           ranges;
> > +
> > +           soc0_sram: sram@10000000 {
> > +                   compatible = "mmio-sram";
> > +                   reg = <0x0 0x10000000 0x0 0x20000>;     /* 128KiB SRAM on soc0
> */
> > +                   ranges = <0x0 0x0 0x0 0x10000000 0x0 0x20000>;
> > +                   #address-cells = <2>;
> > +                   #size-cells = <2>;
> > +                   no-memory-wc;
> > +
> > +                   exported@0 {
> > +                           reg = <0 0x0 0 0x20000>;
> > +                           export;
> > +                   };
> > +           };
> > +
> > +           syscon0: syscon@12c02000 {
> > +                   compatible = "aspeed,ast2700-scu0", "syscon", "simple-mfd";
> > +                   reg = <0x0 0x12c02000 0x0 0x1000>;
> > +                   ranges = <0x0 0x0 0 0x12c02000 0 0x1000>;
> > +                   #address-cells = <2>;
> > +                   #size-cells = <2>;
> > +                   #clock-cells = <1>;
> > +                   #reset-cells = <1>;
> > +
> > +                   silicon-id@0 {
> > +                           compatible = "aspeed,ast2700-silicon-id",
> "aspeed,silicon-id";
> > +                           reg = <0 0x0 0 0x4>;
> > +                   };
> > +
> > +                   scu_ic0: interrupt-controller@1D0 {
>
> DTS coding style...
Agree. I will fix to "scu_ic0: interrupt-controller@1d0 {"

>
> > +                           #interrupt-cells = <1>;
> > +                           compatible = "aspeed,ast2700-scu-ic0";
> > +                           reg = <0 0x1d0 0 0xc>;
> > +                           interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
> > +                           interrupt-controller;
> > +                   };
> > +
> > +                   scu_ic1: interrupt-controller@1E0 {
> > +                           #interrupt-cells = <1>;
> > +                           compatible = "aspeed,ast2700-scu-ic1";
> > +                           reg = <0 0x1e0 0 0xc>;
> > +                           interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> > +                           interrupt-controller;
> > +                   };
> > +
> > +                   soc0_rst: reset-controller@200 {
> > +                           reg = <0 0x200 0 0x40>;
> > +                   };
> > +
> > +                   soc0_clk: clock-controller@240 {
> > +                           reg = <0 0x240 0 0x1c0>;
> > +                   };
> > +           };
> > +
> > +   };
> > +
> > +   soc1: soc@14000000 {
>
> Wait, what, to socs?
Yes.
In AST2700, there are two socs with different base address for use.

>
> > +           compatible = "simple-bus";
> > +           #address-cells = <2>;
> > +           #size-cells = <2>;
> > +           ranges;
> > +
> > +           syscon1: syscon@14c02000 {
> > +                   compatible = "aspeed,ast2700-scu1", "syscon", "simple-mfd";
> > +                   reg = <0x0 0x14c02000 0x0 0x1000>;
> > +                   ranges = <0x0 0x0 0x0 0x14c02000 0x0 0x1000>;
> > +                   #address-cells = <2>;
> > +                   #size-cells = <2>;
> > +                   #clock-cells = <1>;
> > +                   #reset-cells = <1>;
> > +
> > +                   soc1_rst: reset-controller@200 {
> > +                           #reset-cells = <1>;
> > +                   };
> > +
> > +                   soc1_clk: clock-controller@240 {
> > +                           reg = <0 0x240 0 0x1c0>;
> > +                   };
> > +           };
> > +
> > +           uart12: serial@14c33b00 {
> > +                   compatible = "ns16550a";
> > +                   reg = <0x0 0x14c33b00 0x0 0x100>;
> > +                   reg-shift = <2>;
> > +                   reg-io-width = <4>;
> > +                   clocks = <&syscon1 SCU1_CLK_GATE_UART12CLK>;
> > +                   no-loopback-test;
> > +                   pinctrl-names = "default";
>
> What is this?
BMC UART is used in uart12 in soc1.

>
> > +           };
> > +   };
> > +};
> > +
>
> Best regards,
> Krzysztof

--
Best Regards,
Kevin.Chen
************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊,並受法律保護。如 台端非指定之收件者,請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
Kevin Chen Aug. 16, 2024, 4:07 a.m. UTC | #11
Hi Krzk,

> > ---
> >  MAINTAINERS                  |  3 +++
> >  arch/arm64/Kconfig.platforms | 14 ++++++++++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > c0a3d9e93689..08609430cfe0 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2121,7 +2121,10 @@ Q:
>       https://patchwork.ozlabs.org/project/linux-aspeed/list/
> >  T: git git://git.kernel.org/pub/scm/linux/kernel/git/joel/bmc.git
> >  F: Documentation/devicetree/bindings/arm/aspeed/
> >  F: arch/arm/boot/dts/aspeed/
> > +F: arch/arm64/boot/dts/aspeed/
> >  F: arch/arm/mach-aspeed/
> > +F: include/dt-bindings/clock/aspeed,ast2700-clk.h
> > +F: include/dt-bindings/reset/aspeed,ast2700-reset.h
> >  N: aspeed
> >
> >  ARM/AXM LSI SOC
> > diff --git a/arch/arm64/Kconfig.platforms
> > b/arch/arm64/Kconfig.platforms index 6c6d11536b42..1db7b6f1ee0a 100644
> > --- a/arch/arm64/Kconfig.platforms
> > +++ b/arch/arm64/Kconfig.platforms
> > @@ -40,6 +40,20 @@ config ARCH_APPLE
> >       This enables support for Apple's in-house ARM SoC family, starting
> >       with the Apple M1.
> >
> > +config ARCH_ASPEED
> > +   bool "Aspeed SoC family"
> > +   select MACH_ASPEED_G7
> > +   help
> > +     Say yes if you intend to run on an Aspeed ast2700 or similar
> > +     seventh generation Aspeed BMCs.
> > +
> > +config MACH_ASPEED_G7
> > +   bool "Aspeed SoC AST2700"
>
> There are no MACHines for arm64. Look at this code. Do you see MACH
> anywhere else? No. Then why Aspeed must be different?
>
> No. Drop.
Agree.

>
> Best regards,
> Krzysztof

--
Best Regards,
Kevin.Chen
************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊,並受法律保護。如 台端非指定之收件者,請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
Krzysztof Kozlowski Aug. 16, 2024, 5:08 a.m. UTC | #12
On 15/08/2024 07:50, Kevin Chen wrote:
>>> diff --git a/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
>>> new file mode 100644
>>> index 000000000000..858ab95251e4
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi
>>> @@ -0,0 +1,217 @@
>>> +// SPDX-License-Identifier: GPL-2.0-or-later
>>> +#include <dt-bindings/clock/aspeed,ast2700-clk.h>
>>> +#include <dt-bindings/reset/aspeed,ast2700-reset.h>
>>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +#include <dt-bindings/interrupt-controller/aspeed-scu-ic.h>
>>> +
>>> +/ {
>>> +     model = "Aspeed BMC";
>>
>> Model of what? No, drop.
> Can I change to "model = "AST2700 EVB""

Model of what? No, it does not make sense here.


..


>>> +
>>> +             uart12: serial@14c33b00 {
>>> +                     compatible = "ns16550a";
>>> +                     reg = <0x0 0x14c33b00 0x0 0x100>;
>>> +                     reg-shift = <2>;
>>> +                     reg-io-width = <4>;
>>> +                     clocks = <&syscon1 SCU1_CLK_GATE_UART12CLK>;
>>> +                     no-loopback-test;
>>> +                     pinctrl-names = "default";
>>
>> What is this?
> BMC uart using in uart12 in soc1.

No, that line. pinctrl-names do not make sense here without values.

> 
>>
>>> +             };
>>> +     };
>>> +};
>>> +
> 
> --
> Best Regards,
> Kevin. Chen
> ________________________________
> 寄件者: Krzysztof Kozlowski <krzk@kernel.org>
> 寄件日期: 2024年7月26日 下午 07:19
> 收件者: Kevin Chen <kevin_chen@aspeedtech.com>; robh@kernel.org <robh@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; conor+dt@kernel.org <conor+dt@kernel.org>; joel@jms.id.au <joel@jms.id.au>; andrew@codeconstruct.com.au <andrew@codeconstruct.com.au>; lee@kernel.org <lee@kernel.org>; catalin.marinas@arm.com <catalin.marinas@arm.com>; will@kernel.org <will@kernel.org>; arnd@arndb.de <arnd@arndb.de>; olof@lixom.net <olof@lixom.net>; soc@kernel.org <soc@kernel.org>; mturquette@baylibre.com <mturquette@baylibre.com>; sboyd@kernel.org <sboyd@kernel.org>; p.zabel@pengutronix.de <p.zabel@pengutronix.de>; quic_bjorande@quicinc.com <quic_bjorande@quicinc.com>; geert+renesas@glider.be <geert+renesas@glider.be>; dmitry.baryshkov@linaro.org <dmitry.baryshkov@linaro.org>; shawnguo@kernel.org <shawnguo@kernel.org>; neil.armstrong@linaro.org <neil.armstrong@linaro.org>; m.szyprowski@samsung.com <m.szyprowski@samsung.com>; nfraprado@collabora.com <nfraprado@collabora.com>; u-kumar1@ti.com <u-kumar1@ti.com>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linux-aspeed@lists.ozlabs.org <linux-aspeed@lists.ozlabs.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-clk@vger.kernel.org <linux-clk@vger.kernel.org>
> 主旨: Re: [PATCH v1 08/10] arm64: dts: aspeed: Add initial AST27XX device tree
> 

Why do you quote my email twice?


...

>> +             uart12: serial@14c33b00 {
>> +                     compatible = "ns16550a";
>> +                     reg = <0x0 0x14c33b00 0x0 0x100>;
>> +                     reg-shift = <2>;
>> +                     reg-io-width = <4>;
>> +                     clocks = <&syscon1 SCU1_CLK_GATE_UART12CLK>;
>> +                     no-loopback-test;
>> +                     pinctrl-names = "default";
> 
> What is this?
> 
>> +             };
>> +     };
>> +};
>> +
> 
> Best regards,
> Krzysztof
> 
> ************* Email Confidentiality Notice ********************
> 免責聲明:
> 本信件(或其附件)可能包含機密資訊,並受法律保護。如 台端非指定之收件者,請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!
> 
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.

Maybe I am the intended recipient of your message, maybe not. I don't
want to have any legal questions regarding upstream, public
collaboration, thus probably I should just remove your messages.

Please talk with your IT that such disclaimers in open-source are not
desired (and maybe even harmful).
If you do not understand why, please also see:
https://www.youtube.com/live/fMeH7wqOwXA?si=GY7igfbda6vnjXlJ&t=835

If you need to go around company SMTP server, then consider using b4
web-relay: https://b4.docs.kernel.org/en/latest/contributor/send.html

Please be informed that by responding to this email you agree that all
communications from you and/or your company is made public. In other
words, all messages originating from you and/or your company will be
made public.

Best regards,
Krzysztof