Stephen, On Wed, Feb 27, 2013 at 11:08 AM, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 02/27/2013 09:59 AM, Tom Warren wrote: >> Stephen, >> >> On Tue, Feb 26, 2013 at 4:26 PM, Stephen Warren <swar...@wwwdotorg.org> >> 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. Technically, they're not pinmux registers (PINMUX_AUX_ space), but GP regs (APB_MISC_GP_ space). Since the pinmux _code_ (no pinmux driver is used in Tegra U-Boot) for T30 is just a large table slam, I don't think it makes sense to add GP pad reg writes there. These pads need to be tuned when you've got a board w/an SD-card device hanging off of them. So it makes sense to have these 2 register writes here in pad_init_mmc(). I can take out the SDMMC1/3 test and just write both SDIO1CFG and SDIO3CFG, since the values are the same. > > 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. Why don't we get this in so we can move forward, and when there's a kernel version of these params in the DT files, I can port it over.
I've got the compatible string changed to just 'nvidia,tegra30-sdhci' in the dtsi file, and I've changed the pad_init_mmc() code to use the mmc_id (PERIPH_ID_SDMMCx) instead of the base addresses to check for an SDIO port before setting the pad config regs. I'll send that up as V2 of the patchset, probably tomorrow. Tom _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot