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
