From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Wed, 27 Feb 2013 11:08:20 -0700 Subject: [U-Boot] [PATCH 3/5] Tegra30: MMC: Add SD bus power-rail and SDMMC pad init routines In-Reply-To: References: <1361911596-16518-1-git-send-email-twarren@nvidia.com> <1361911596-16518-4-git-send-email-twarren@nvidia.com> <512D4493.6060502@wwwdotorg.org> Message-ID: <512E4B94.8060302@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 02/27/2013 09:59 AM, Tom Warren wrote: > Stephen, > > On Tue, Feb 26, 2013 at 4:26 PM, Stephen Warren wrote: >> On 02/26/2013 01:46 PM, Tom Warren wrote: >>> T30 requires specific SDMMC pad programming, and bus power-rail bringup. >> >>> diff --git a/board/nvidia/cardhu/cardhu.c b/board/nvidia/cardhu/cardhu.c >> >>> +/* >>> + * Do I2C/PMU writes to bring up SD card bus power >>> + * >>> + */ >>> +void board_sdmmc_voltage_init(void) >> >> We really shouldn't be adding to board files if we're remotely serious >> about device tree; the whole point of DT is to remove code from the >> board files, and describe the desired configuration as data in DT instead. >> >> This function should be replaced by regulator nodes/properties in the >> device tree, and the MMC (core?) driver calling into the board-specific >> regulator driver to request the desired voltages. >> >> But so long as we file a bug to replace this code with an explicit >> regulator driver in the future, I guess it's fine for now. > > I'll file a bug for doing all PMU/power rail work from DT. I think it > makes much more sense as a separate (non-MMC) patch. Yes, certainly a separate patch. Ideally it'd be implemented before other code that relies on it. That's why I think we need to take a higher-level look at DT support in U-Boot, rather than simply finding these issue accidentally while implementing the features we already know we need. >> BTW, I just noticed that commit f01b631 "Tegra30: Add/enable Cardhu >> build (T30 reference board)" adds a file called >> board/nvidia/cardhu/cardhu.c.mmc. That's a mistake, right? > > Yep, that's the Cardhu file I was hacking on for MMC support way back > when. I can remove it in V2 of these patches, or would you prefer a > single, separate patch to do that? Is the commit that adds that file already pulled into higher-level repos? If not, I would simply rebase it to remove that file. If it has, then a separate patch to delete it before this series would make sense. >>> diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c >> >> Hmm. This seems like SoC code, not board code... >> >>> +void pad_init_mmc(struct tegra_mmc *reg) >>> +{ >>> +#if defined(CONFIG_TEGRA30) >> >>> + /* Set the pad drive strength for SDMMC1 or 3 only */ >>> + if (offset != TEGRA_SDMMC1_BASE && offset != TEGRA_SDMMC3_BASE) { >>> + debug("%s: settings are only valid for SDMMC1/SDMMC3!\n", >>> + __func__); >>> + return; >>> + } >> >> Perhaps pass in the MMC instance ID instead of the base address. That'd >> avoid having to know the base addresses in this code. > > I still need to know if I've got a SDIO or eMMC ID, though, and I > don't think the flags for that in the mmc/host structs (mmc->version, > etc.) get populated until the mmc driver has done some I/O with the > device (eMMC or SD-card), and I need to set up the pads before that. > >> In fact, just putting this code into the pinmux driver (which owns these >> registers) seems like a better idea; there's no need to only do this >> when the SD controller is enabled, is there? > > Half of these regs are in the SD controller register space > (sdmemcmpctl and autocalcfg), and get reset when the controller gets > reset (mmc_reset). So they need to be set each time a reset occurs. It > makes sense to keep the GP SDIOCFG writes here, too. > > As to where the pad_init_mmc function belongs, it is SoC-specific, > yes. T20 doesn't need it (no sdmemcmpctl or autocalcfg regs on T20 > SDMMC), and T30 and T114 use slightly different bits/values for GP > SDIOCFG and sdmemcmpctl/autocalcfg. But the differences are small > enough that I thought this routine should be placed in a common area, > and not duplicated for each SoC, so I put it here. > > Do you have a suggestion of where it would be better placed? For the pinmux registers, I think they should be programmed by the pinmux driver at the same time as the rest of the pinmux is programmed. For the SD registers, I guess we need to program them from the SD driver as you say, but need to add some more properties to the DT in order to parameterize this.