[U-Boot] [PATCH v2] socfpga: Adding Freeze Controller driver

[Pavel Machek]

Hi!

> Adding Freeze Controller driver. All HPS IOs need to be
> in freeze state during pin mux or IO buffer configuration.
> It is to avoid any glitch which might happen
> during the configuration from propagating to external devices.

So... code that is currently in u-boot appears to work, but may
produce some unwanted electrical spikes. They don't matter on
development boards, but we'd like to get rid of them for production. ?

It would be nice to put some explanation as a comment into code.

> Signed-off-by: Chin Liang See <clsee at altera.com>
> Cc: Wolfgang Denk <wd at denx.de>
> CC: Pavel Machek <pavel at denx.de>
> Cc: Dinh Nguyen <dinguyen at altera.com>
> ---
> Changes for v2
> - Removed FREEZE_CONTROLLER_FSM_HW
> - Removed the get_timer_count_masked and convert to use delay in us
> - Used shorter local variables

Thanks! Still there's a bit to go:

> +#define SYSMGR_FRZCTRL_LOOP_PARAM       (1000)
> +#define SYSMGR_FRZCTRL_DELAY_LOOP_PARAM (10)
> +#define SYSMGR_FRZCTRL_INTOSC_33	(33)
> +#define SYSMGR_FRZCTRL_INTOSC_1000	(1000)

We no longer need these, right?

> +#define FREEZE_CHANNEL_NUM		(4)

Check this one, it may not be needed.

> +typedef enum {
> +	FREEZE_CTRL_FROZEN = 0,
> +	FREEZE_CTRL_THAWED = 1
> +} FREEZE_CTRL_CHAN_STATE;

This definitely should not be needed.

> +typedef enum  {
> +	FREEZE_CHANNEL_0 = 0,   /* EMAC_IO & MIXED2_IO */
> +	FREEZE_CHANNEL_1,	/* MIXED1_IO and FLASH_IO */
> +	FREEZE_CHANNEL_2,	/* General IO */
> +	FREEZE_CHANNEL_3,	/* DDR IO */
> +	FREEZE_CHANNEL_UNDEFINED
> +} FreezeChannelSelect;

CamelCaseIsUnwelcome. And actually haveing enum for numbers 0..3 looks
like overkill. Could we just use plain numbers?

> +typedef enum {
> +	FREEZE_CONTROLLER_FSM_SW = 0,
> +	FREEZE_CONTROLLER_FSM_HW,
> +	FREEZE_CONTROLLER_FSM_UNDEFINED
> +} FreezeControllerFSMSelect;

No longer needed.

> +#define SYSMGR_FRZCTRL_HWCTRL_VIO1STATE_GET(x) (((x) & 0x00000006)
  >> 1)

Is this still used?

> +#define SYSMGR_FRZCTRL_VIOCTRL_SHIFT 0x2
> +
> +u32 sys_mgr_frzctrl_freeze_req(FreezeChannelSelect channel_id,
> +	FreezeControllerFSMSelect fsm_select);
> +u32 sys_mgr_frzctrl_thaw_req(FreezeChannelSelect channel_id,
> +	FreezeControllerFSMSelect fsm_select);

We can remove second parameter here. And maybe use some more
reasonable names.

sysmgr_freeze_req / sysmgr_thaw_req?

Thanks, and sorry for two-phase review.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html