diff mbox

[U-Boot,RFC,1/1] m68k: mfc5227x: do not rely on uninitialized stack values

Message ID 24db24a5-24eb-5bbb-c62c-35f56ed736d0@sysam.it
State RFC
Delegated to: Jason Jin
Headers show

Commit Message

Angelo Dureghello July 31, 2017, 3:16 p.m. UTC 
Hi Heinrich,

thanks for the patch

On 30/07/2017 20:59, Heinrich Schuchardt wrote:
> The behavior get_clocks depends on the unitialized
> value of bootmode.
> 
> By setting it to zero we get a defined behavior and
> can get rid of superfluos coding.
> 
> The problem was indicated by cppcheck.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> I have no hardware to test this.
> Please, check thorougly before merging.
> ---
> ---
>   arch/m68k/cpu/mcf5227x/speed.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/m68k/cpu/mcf5227x/speed.c b/arch/m68k/cpu/mcf5227x/speed.c
> index 44de4a6701..978cc7adc6 100644
> --- a/arch/m68k/cpu/mcf5227x/speed.c
> +++ b/arch/m68k/cpu/mcf5227x/speed.c
> @@ -73,14 +73,8 @@ int get_clocks(void)
>   
>   	ccm_t *ccm = (ccm_t *)MMAP_CCM;
>   	pll_t *pll = (pll_t *)MMAP_PLL;
> -	int vco, temp, pcrvalue, pfdr;
> -	u8 bootmode;
> -
> -	pcrvalue = in_be32(&pll->pcr) & 0xFF0F0FFF;
> -	pfdr = pcrvalue >> 24;
> -
> -	if (pfdr == 0x1E)
> -		bootmode = 0;	/* Normal Mode */
> +	int vco, temp, pcrvalue;
> +	u8 bootmode = 0;	/* Normal Mode */;
>   
>   #ifdef CONFIG_CF_SBF
>   	bootmode = 3;		/* Serial Mode */
> 

sure uninitialized vars are an issue.This change works and make things simpler.

But looking the code i have a doubt now:
this code seems attempting to detect "runtime" the boot mode, getting clocks from
pll registers already set i.e. form SBF header.
So there can be a case where jumpers (or pull up/down resistors) are set to SBF (11)
and "bootmode" variable will be set to 3 only if CONFIG_CF_SBF is set, otherwise
it will be 0 and not 3.

Of course, if we try to boot by SBF and CONFIG_CF_SBF is not set, u-boot will not
boot in anyway, since spi init will be totally missing.

But in case a different "fully runtime" proposal could be:


What do you think ?

Regards,
Angelo Dureghello
diff mbox

Patch

diff --git a/arch/m68k/cpu/mcf5227x/speed.c b/arch/m68k/cpu/mcf5227x/speed.c
index 44de4a6701..e21c1d574b 100644
--- a/arch/m68k/cpu/mcf5227x/speed.c
+++ b/arch/m68k/cpu/mcf5227x/speed.c
@@ -74,7 +74,7 @@  int get_clocks(void)
         ccm_t *ccm = (ccm_t *)MMAP_CCM;
         pll_t *pll = (pll_t *)MMAP_PLL;
         int vco, temp, pcrvalue, pfdr;
-       u8 bootmode;
+       u8 bootmode = 3;

         pcrvalue = in_be32(&pll->pcr) & 0xFF0F0FFF;
         pfdr = pcrvalue >> 24;
@@ -82,10 +82,6 @@  int get_clocks(void)
         if (pfdr == 0x1E)
                 bootmode = 0;   /* Normal Mode */

-#ifdef CONFIG_CF_SBF
-       bootmode = 3;           /* Serial Mode */
-#endif
-
         if (bootmode == 0) {
                 /* Normal mode */
                 vco = ((in_be32(&pll->pcr) & 0xFF000000) >> 24) * CONFIG_SYS_INPUT_CLKSRC;