Re: [PATCH] imx: imx8mm: Add suppoer for Mettler-Toledo snowflake board.

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2023-03-16 11:33:47 [+0100], Frieder Schrempf wrote:
> > diff --git a/arch/arm/dts/imx8mm-mt-snowflake-v2.dts b/arch/arm/dts/imx8mm-mt-snowflake-v2.dts
> > new file mode 100644
> > index 0000000000000..49761b22afcf0
> > --- /dev/null
> > +++ b/arch/arm/dts/imx8mm-mt-snowflake-v2.dts
> > @@ -0,0 +1,186 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 Mettler Toledo GmbH
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "imx8mm-kontron-sl.dtsi"
> > +#include "imx8mm-u-boot.dtsi"
> > +
> > +/ {
> > +	model = "Mettler Toledo i.MX8MM Snowflake V2";
> > +	compatible = "mt,imx8mm-snowflake-v2", "fsl,imx8mm";
> 
> I think the compatible would normally include the SoM:
> 
> compatible = "mt,imx8mm-snowflake-v2", "kontron,imx8mm-sl", "fsl,imx8mm";

This might work. Let me add it.

…
> > +
> > +&i2c1 {
> > +	clock-frequency = <400000>;
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_i2c1>;
> > +	status = "okay";
> > +
> > +	pca9450: pmic@25 {
…
> > +};
> 
> The included SoM DTSI already contains the PMIC node. You can remove
> that here. Or if you need to do adjustments for the board level you
> should only overwrite what is needed instead of duplicating everything.

They appear to identical except for the regulator-name but this does not
matter. Dropping that then.

…
> > +	pinctrl_i2c1: i2c1grp {
> > +		fsl,pins = <
> > +			MX8MM_IOMUXC_I2C1_SCL_I2C1_SCL			0x400001c3
> > +			MX8MM_IOMUXC_I2C1_SDA_I2C1_SDA			0x400001c3
> > +		>;
> > +	};
> 
> Same here, this node is already available from the SoM DTSI.

gone.

…
> > diff --git a/board/mt/snowflake_v2/spl.c b/board/mt/snowflake_v2/spl.c
> > new file mode 100644
> > index 0000000000000..3e27256914b32
> > --- /dev/null
> > +++ b/board/mt/snowflake_v2/spl.c
…
> > +	dram_timing.fsp_msg[0].fsp_cfg[9].val = 0x110;
> > +	dram_timing.fsp_msg[0].fsp_cfg[21].val = 0x1;
> > +	dram_timing.fsp_msg[1].fsp_cfg[10].val = 0x110;
> > +	dram_timing.fsp_msg[1].fsp_cfg[22].val = 0x1;
> > +	dram_timing.fsp_msg[2].fsp_cfg[10].val = 0x110;
> > +	dram_timing.fsp_msg[2].fsp_cfg[22].val = 0x1;
> > +	dram_timing.fsp_msg[3].fsp_cfg[10].val = 0x110;
> > +	dram_timing.fsp_msg[3].fsp_cfg[22].val = 0x1;
> 
> As you only support the 1GB DDR on this board, you could integrate these
> values directly in the tables in lpddr4_timing.c and remove them here.

I'm not sure how much of the header file is auto-generated including
these bits here.
Let me merge this now and worry later.

> > +
> > +	if (!ddr_init(&dram_timing) && check_ram_available(SZ_1G)) {
> > +		size = 1;
> > +		puts("1GB DDR RAM\n");
> > +	} else {
> > +		puts("Init DDR RAM failed\n");
> > +		size = 1;
> > +	}
> > +
> > +	writel(size, M4_BOOTROM_BASE_ADDR);
> 
> You could also simplify this and improve the error handling for your case:
> 
> if (ddr_init(&dram_timing) || !check_ram_available(SZ_1G)) {
> 	puts("Init DDR RAM failed\n");
> 	halt();
> }
> 
> puts("1GB DDR RAM\n");
> writel(1, M4_BOOTROM_BASE_ADDR);

Why not. There is not "halt()" in tree so I'm going to substite that
block with panic().

…
> > diff --git a/include/configs/snowflake_v2.h b/include/configs/snowflake_v2.h
> > new file mode 100644
> > index 0000000000000..f038dc9e6d7d8
> > --- /dev/null
> > +++ b/include/configs/snowflake_v2.h
> > @@ -0,0 +1,54 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2019 Kontron Electronics GmbH
> > + * Copyright (C) 2020 Mettler Toledo GmbH
> > + *
> > + * Configuration settings for the MT Snowflake v2 Terminal, based on Kontron N8000 i.MX8MM SMARC module.
> 
> It's not a SMARC module. And nowadays the proper product name is
> "Kontron SL i.MX8M Mini".

okay.

That should have been all of your comments.

Sebastian