Re: [PATCH 6/6] arm: imx8mp: add karo electronics qsxp imx8mp som support

[Stefan Kerkmann <s.kerkmann@pengutronix.de>]

Hello Ahmad,

On 11.03.24 11:13, Ahmad Fatoum wrote:
> Hello Stefan,
> 
> On 11.03.24 10:10, Stefan Kerkmann wrote:
>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>>
>> The Ka-Ro QSXP is a i.MX8M Plus solder down system on module. This
>> commit adds support for the SOM on the QSBASE4 RDK.
>>
>> [1]: https://www.karo-electronics.de/qsxp
>>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
>> ---
>>  arch/arm/boards/Makefile                           |    1 +
>>  arch/arm/boards/karo-qsxp-ml81/Makefile            |    4 +
>>  arch/arm/boards/karo-qsxp-ml81/board.c             |   35 +
>>  .../flash-header-karo-qsxp-ml81.imxcfg             |    9 +
>>  arch/arm/boards/karo-qsxp-ml81/lowlevel.c          |  102 ++
>>  arch/arm/boards/karo-qsxp-ml81/lowlevel.h          |    8 +
>>  arch/arm/boards/karo-qsxp-ml81/lpddr4-timing.c     | 1597 ++++++++++++++++++++
> 
> If QSXP is the SoM name, rename the folder name to drop ml81.
> 

See reply to patch 5/6, the full manufacturer part number is `QSXP-ML81`
therefore I vote to keep the `ml81` part.

>> +
>> +	/*
>> +	 * If we are in EL3 we are running for the first time out of OCRAM,
>> +	 * we'll need to initialize the DRAM and run TF-A (BL31). The TF-A
>> +	 * will then jump to DRAM in EL2
>> +	 */
>> +	if (current_el() == 3) {
>> +		imx8mp_early_clock_init();
>> +
>> +		power_init_board();
>> +
>> +		imx8mp_ddr_init(&karo_qsxp_ml81_dram_timing, DRAM_TYPE_LPDDR4);
>> +
>> +		imx8mp_load_and_start_image_via_tfa();
>> +	}
>> +
>> +	/* Standard entry we hit once we initialized both DDR and ATF */
>> +	imx8mm_barebox_entry(__dtb_z_imx8mp_karo_qsxp_ml81_qsbase4_start);
> 
> s/imx8mm/imx8mp/
> 

Ack.

>> +/* ddr phy trained csr */
>> +static struct dram_cfg_param ddr_ddrphy_trained_csr[] = {
> 
> This is unused. See commit
> 975acf1bafba ("ARM: i.MX8M: delete unused per-board ddr_ddrphy_trained_csr array")
> 
> Unless your value differ from the default, just drop this from here.
> 

Ack.

>> +static struct dram_fsp_msg ddr_dram_fsp_msg[] = {
>> +	{
>> +		/* P0 3200mts 1D */
>> +		.drate = 3200,
>> +		.fw_type = FW_1D_IMAGE,
>> +		.fsp_cfg = ddr_fsp0_cfg,
>> +		.fsp_cfg_num = ARRAY_SIZE(ddr_fsp0_cfg),
>> +	},
>> +	{
>> +		/* P0 3200mts 2D */
>> +		.drate = 3200,
>> +		.fw_type = FW_2D_IMAGE,
>> +		.fsp_cfg = ddr_fsp0_2d_cfg,
>> +		.fsp_cfg_num = ARRAY_SIZE(ddr_fsp0_2d_cfg),
>> +	},
> 
> JFYI, if you want to dynamic frequency scaling for DDR, you'll need some
> more entries here.
> 

I'm unfamiliar with dynamic frequency scaling, so I'm not sure if we
need/want more for a first support of this SOM?

>> +config MACH_KARO_QSXP_ML81
> 
> Please enable this in imx_v8_defconfig and multi_v8_defconfig for CI coverage.
> 

Ack.

> 
> Cheers,
> Ahmad
> 

Cheers,
Stefan

-- 
Pengutronix e.K.                       | Stefan Kerkmann             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |