From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chin Liang See Date: Fri, 20 Sep 2013 00:08:08 -0500 Subject: [U-Boot] [PATCH v2] socfpga: Adding Freeze Controller driver In-Reply-To: <20130919225528.GB20481@amd.pavel.ucw.cz> References: <1379603653-5524-1-git-send-email-clsee@altera.com> <20130919225528.GB20481@amd.pavel.ucw.cz> Message-ID: <1379653688.5736.11.camel@clsee-VirtualBox> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > Cc: Wolfgang Denk > > CC: Pavel Machek > > Cc: Dinh Nguyen > > --- > > 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