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@altera.com Cc: Wolfgang Denk wd@denx.de CC: Pavel Machek pavel@denx.de Cc: Dinh Nguyen dinguyen@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)
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