From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Thu, 12 Jan 2012 16:18:14 -0800 Subject: [U-Boot] [PATCH 13/14] tegra: Add EMC settings for Seaboard, Harmony In-Reply-To: <74CDBE0F657A3D45AFBB94109FB122FF17801D1E94@HQMAIL01.nvidia.com> References: <1324927987-13100-1-git-send-email-sjg@chromium.org> <1324927987-13100-14-git-send-email-sjg@chromium.org> <4F0C879E.1040504@nvidia.com> <74CDBE0F657A3D45AFBB94109FB122FF17801D1E74@HQMAIL01.nvidia.com> <74CDBE0F657A3D45AFBB94109FB122FF17801D1E8E@HQMAIL01.nvidia.com> <74CDBE0F657A3D45AFBB94109FB122FF17801D1E94@HQMAIL01.nvidia.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Stephen, On Thu, Jan 12, 2012 at 4:10 PM, Stephen Warren wrote: > Simon Glass wrote at Thursday, January 12, 2012 5:06 PM: >> On Thu, Jan 12, 2012 at 4:01 PM, Stephen Warren wrote: >> > Simon Glass wrote at Thursday, January 12, 2012 4:55 PM: >> >> On Thu, Jan 12, 2012 at 3:42 PM, Stephen Warren wrote: >> >> > Simon Glass wrote at Thursday, January 12, 2012 4:05 PM: >> >> >> On Tue, Jan 10, 2012 at 10:46 AM, Stephen Warren wrote: >> >> >> > On 12/26/2011 12:33 PM, Simon Glass wrote: >> >> >> >> From: Jimmy Zhang >> >> >> >> >> >> >> >> Set Seaboard and Harmony to optimal memory settings based on the SOC >> >> >> >> in use (T20 or T25). >> >> > ... >> >> >> >> +int board_emc_init(void) >> >> >> >> +{ >> >> >> >> + ? ? ? int ? ? i; >> >> >> >> + ? ? ? DECLARE_GLOBAL_DATA_PTR; >> >> >> >> + >> >> >> >> +#ifdef CONFIG_TEGRA_PMU >> >> >> >> + ? ? ? /* if voltage has not been set properly, return */ >> >> >> >> + ? ? ? if (!pmu_is_voltage_nominal()) >> >> >> >> + ? ? ? ? ? ? ? return -1; >> >> >> >> +#endif >> >> >> > >> >> >> > Why/when would the PMU voltage not be nominal? >> >> >> >> >> >> On boot, it starts up lower and we raise it to nominal so we can run >> >> >> at full speed. >> >> >> >> >> >> > Can't we error out the compile if the options that cause the PMU voltage >> >> >> > to be initialized to nominal are not set, instead of detecting this at >> >> >> > runtime? >> >> >> >> >> >> I don't think so, since we can't know in U-Boot what the start-up voltages are. >> >> > >> >> > So how does the system get to the nominal state? And if board_emc_init() >> >> > is called when the system isn't in the nominal state, does it somehow get >> >> > called again later once it is, so that the EMC initialization doesn't fail >> >> > the error-check quoted above? >> >> >> >> We call board_emc_init() after pmu_set_nominal(). >> >> >> >> > >> >> > In other words, presumably U-Boot explicitly programs the PMU into the >> >> > nominal stage at some point. Shouldn't we defer calling board_emc_init() >> >> > until after that time, thus making that error-check redundant? >> >> >> >> Yes, but if you look at the patch above, that's what we do: >> >> >> >> ?#ifdef CONFIG_TEGRA_PMU >> >> ? ? ? ?pmu_set_nominal(); >> >> + >> >> + ? ? ? board_emc_init(); >> >> ?#endif >> >> ?#endif >> > >> > OK, so in practice, >> > >> > /* if voltage has not been set properly, return */ >> > if (!pmu_is_voltage_nominal()) >> > >> > ... will never fire. My original point was that if so, why is that check >> > needed? I suppose it's a reasonable safety net though - that's the >> > reason? >> >> OK I see. It certainly shouldn't - it is a check that everything is >> well since this code is in a different file and it is possible that >> someone may get this wrong. If they do then the system may continue >> but die later in interesting ways. Still, the user has other equally >> complex things to worry about. >> >> I'm happy to remove this particularly as this might become example >> code for other boards - what do you think? > > The check itself is probably fine for the reasons you state. However, > I'd suggest adjusting the comment to something more like: > > This code should only be called once the PMU is operating at nominal > voltage. Hence, this test should never fail. However, this prevents > unpredictable failures from occurring later if this pre-condition is > not met. OK, will do. Regards, Simon > > -- > nvpublic >