Re: [PATCH 4/5] ARM: i.MX8MP: don't reparent GIC from BootROM default

[Lucas Stach <l.stach@pengutronix.de>]

Am Freitag, dem 19.04.2024 um 08:13 +0200 schrieb Ahmad Fatoum:
> On i.MX8MP, GIC can run at up to 400 MHz in nominal drive mode and up
> to 500 MHz in overdrive mode. We currently configure unconditionally
> to 100 MHz on i.MX8MP.
> 
> The BootROM default is running it on 400 MHz, which works well for us on
> the i.MX8MP, so skip the GIC configuration on the i.MX8MP.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  arch/arm/mach-imx/imx8m.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/imx8m.c b/arch/arm/mach-imx/imx8m.c
> index 56330cef500c..0966f7fdf076 100644
> --- a/arch/arm/mach-imx/imx8m.c
> +++ b/arch/arm/mach-imx/imx8m.c
> @@ -100,13 +100,6 @@ static void __imx8m_early_clock_init(int cpu_type)
>  		INTPLL_DIV20_CLKE_MASK;
>  	writel(val, ana + IMX8MM_CCM_ANALOG_SYS_PLL2_GEN_CTRL);
>  
> -	/* config GIC to sys_pll2_100m */
> -	imx8m_ccgr_clock_disable(IMX8M_CCM_CCGR_GIC);
> -	imx8m_clock_set_target_val(IMX8M_GIC_CLK_ROOT,
> -				   IMX8M_CCM_TARGET_ROOTn_ENABLE |
> -				   IMX8M_CCM_TARGET_ROOTn_MUX(3));
> -	imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_GIC);
> -
>  	if (cpu_type == IMX_CPU_IMX8MN || cpu_type == IMX_CPU_IMX8MP)
>  		pll3_freq = 600000000UL;
>  	else
> @@ -118,11 +111,20 @@ static void __imx8m_early_clock_init(int cpu_type)
>  
>  	if (cpu_type == IMX_CPU_IMX8MP) {
>  		/* 8MP ROM already set NOC to 800Mhz, only need to configure NOC_IO clk to 600Mhz */
> +		/* 8MP ROM already set GIC to 400Mhz, system_pll1_800m with div = 2 */

This comment change looks odd. Now it doesn't explain anymore what's
done here and why, but rather explains why the else path isn't executed
on the 8MP.
>  		imx8m_clock_set_target_val(IMX8M_NOC_IO_CLK_ROOT,
>  					   IMX8M_CCM_TARGET_ROOTn_ENABLE |
>  					   IMX8M_CCM_TARGET_ROOTn_MUX(2));
> +	} else {

Maybe move this into a separate condition != IMX_CPU_IMX8MP and move
the comment above here? Leaving the comment about NOC_IO clocks
untouched?

> +		/* config GIC to sys_pll2_100m */
> +		imx8m_ccgr_clock_disable(IMX8M_CCM_CCGR_GIC);
> +		imx8m_clock_set_target_val(IMX8M_GIC_CLK_ROOT,
> +					   IMX8M_CCM_TARGET_ROOTn_ENABLE |
> +					   IMX8M_CCM_TARGET_ROOTn_MUX(3));
> +		imx8m_ccgr_clock_enable(IMX8M_CCM_CCGR_GIC);
>  	}
>  
> +
>  	clrsetbits_le32(ccm + IMX8M_CCM_TARGET_ROOTn(IMX8M_ARM_A53_CLK_ROOT),
>  			IMX8M_CCM_TARGET_ROOTn_MUX(7),
>  			IMX8M_CCM_TARGET_ROOTn_MUX(2));