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

[Chin Liang See]

Hi Pavel,

On Fri, 2013-09-20 at 00:55 +0200, ZY - pavel wrote:
> 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.

Actually with current code, we still not yet to boot on real board yet.
That why I am working hard to get the code upstreamed asap. Of course,
appreciate your help too as you are very helpful.

> 
> > 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:

Yup, actually I miss out the header file after update the source file.

> 
> > +#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?

Yup

> 
> > +#define FREEZE_CHANNEL_NUM		(4)
> 
> Check this one, it may not be needed.

Actually we still need this to maintain the state of IO. It would be
useful for the next new driver "Scan Manager".

> 
> > +typedef enum {
> > +	FREEZE_CTRL_FROZEN = 0,
> > +	FREEZE_CTRL_THAWED = 1
> > +} FREEZE_CTRL_CHAN_STATE;
> 
> This definitely should not be needed.

Same as above

> 
> > +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?

Good suggestion. We put that initially due to potential hardware change
in initial design. We can remove this now.

> 
> > +typedef enum {
> > +	FREEZE_CONTROLLER_FSM_SW = 0,
> > +	FREEZE_CONTROLLER_FSM_HW,
> > +	FREEZE_CONTROLLER_FSM_UNDEFINED
> > +} FreezeControllerFSMSelect;
> 
> No longer needed.

Yup :)

> 
> > +#define SYSMGR_FRZCTRL_HWCTRL_VIO1STATE_GET(x) (((x) & 0x00000006)
>   >> 1)
> 
> Is this still used?

Nope, no more.

> 
> > +#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?

Yup removed the second parameter. About the name freeze and thaw, its
how its labeled in hardware documentation. :)

> 
> Thanks, and sorry for two-phase review.

No problem as you are very supportive.

Thanks

Chin Liang

> 									Pavel