| Message ID | 1361911596-16518-4-git-send-email-twarren@nvidia.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Warren |
| Headers | show |
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. > +{ > + uchar reg, data_buffer[1]; > + int i; > + > + i2c_set_bus_num(0); /* PMU is on bus 0 */ > + > + data_buffer[0] = 0x65; > + reg = 0x32; We should at least comment what those register numbers and values mean. 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? > 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. 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?
Stephen, On Tue, Feb 26, 2013 at 4:26 PM, Stephen Warren <swarren@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. > >> +{ >> + uchar reg, data_buffer[1]; >> + int i; >> + >> + i2c_set_bus_num(0); /* PMU is on bus 0 */ >> + >> + data_buffer[0] = 0x65; >> + reg = 0x32; > > We should at least comment what those register numbers and values mean. Unfortunately, that's not documented in any downstream/NV-generated code that I've seen. I'll have to find the PMU spec and decode what these writes are doing. > > 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? > >> 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? Tom
On 02/27/2013 09:59 AM, Tom Warren wrote: > Stephen, > > On Tue, Feb 26, 2013 at 4:26 PM, Stephen Warren <swarren@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. 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.
Stephen, On Wed, Feb 27, 2013 at 11:08 AM, Stephen Warren <swarren@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 <swarren@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
On 03/04/2013 04:11 PM, Tom Warren wrote: > Stephen, > > On Wed, Feb 27, 2013 at 11:08 AM, Stephen Warren <swarren@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 <swarren@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) The distinction isn't relevant for this discussion. Anyway, there really is a driver... > 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. Are these value board-specific or SoC-specific? I thought that the values came from the TRM and simply had to be set as described. If the values are SoC-specific, I think we can just key off the compatible value to determine whether to apply them. If the values are board-specific, I really think we must put them into device tree. Otherwise, we cannot claim that we have actually supported DT in the driver - we're only doing half the job, and can't support new boards simply by providing a new DT. The one out might be that we could claim the current values are the default if the DT provides no other values, even once we do define a way for DT to provide those values. That would at least be backwards-compatible. However, that's again going down the path of initially defining the minimal DT binding required to get some system running, but planning to incrementally enhance the DT binding later. While we aren't getting strong pushback on this right now from upstream, I can guarantee we will do very soon, so we really ought to just get into the habit of completely defining DT bindings from the start. Still, I suppose I won't nak this patch because of that yet.
On Mon, Mar 4, 2013 at 5:28 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 03/04/2013 04:11 PM, Tom Warren wrote: >> Stephen, >> >> On Wed, Feb 27, 2013 at 11:08 AM, Stephen Warren <swarren@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 <swarren@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) > > The distinction isn't relevant for this discussion. Anyway, there really > is a driver... I must be dense. Please point it out to me. It is relevant, because you've asked to put the GP register writes in the pinmux driver. > >> 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. > > Are these value board-specific or SoC-specific? I thought that the > values came from the TRM and simply had to be set as described. > > If the values are SoC-specific, I think we can just key off the > compatible value to determine whether to apply them. Correct. They are SoC-specific as per the TRM. I am using the mmc_id (periph_id value) to determine whether to write SDIO1CFG or SDIO3CFG in V2, going up this morning. > > If the values are board-specific, I really think we must put them into > device tree. Otherwise, we cannot claim that we have actually supported > DT in the driver - we're only doing half the job, and can't support new > boards simply by providing a new DT. > > The one out might be that we could claim the current values are the > default if the DT provides no other values, even once we do define a way > for DT to provide those values. That would at least be > backwards-compatible. However, that's again going down the path of > initially defining the minimal DT binding required to get some system > running, but planning to incrementally enhance the DT binding later. > While we aren't getting strong pushback on this right now from upstream, > I can guarantee we will do very soon, so we really ought to just get > into the habit of completely defining DT bindings from the start. Still, > I suppose I won't nak this patch because of that yet.
On 03/05/2013 08:28 AM, Tom Warren wrote: > On Mon, Mar 4, 2013 at 5:28 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 03/04/2013 04:11 PM, Tom Warren wrote: >>> On Wed, Feb 27, 2013 at 11:08 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> On 02/27/2013 09:59 AM, Tom Warren wrote: >>>>> On Tue, Feb 26, 2013 at 4:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: ... >>>> 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) >> >> The distinction isn't relevant for this discussion. Anyway, there really >> is a driver... > > I must be dense. Please point it out to me. It is relevant, because > you've asked to put the GP register writes in the pinmux driver. [swarren@swarren-lx1 u-boot]$ find|grep pinmux|grep tegra|grep \\.c$ ./arch/arm/cpu/tegra30-common/pinmux.c ./arch/arm/cpu/tegra114-common/pinmux.c ./arch/arm/cpu/tegra20-common/pinmux.c
On Tue, Mar 5, 2013 at 10:03 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 03/05/2013 08:28 AM, Tom Warren wrote: >> On Mon, Mar 4, 2013 at 5:28 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 03/04/2013 04:11 PM, Tom Warren wrote: >>>> On Wed, Feb 27, 2013 at 11:08 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> On 02/27/2013 09:59 AM, Tom Warren wrote: >>>>>> On Tue, Feb 26, 2013 at 4:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > ... >>>>> 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) >>> >>> The distinction isn't relevant for this discussion. Anyway, there really >>> is a driver... >> >> I must be dense. Please point it out to me. It is relevant, because >> you've asked to put the GP register writes in the pinmux driver. > > [swarren@swarren-lx1 u-boot]$ find|grep pinmux|grep tegra|grep \\.c$ > ./arch/arm/cpu/tegra30-common/pinmux.c > ./arch/arm/cpu/tegra114-common/pinmux.c > ./arch/arm/cpu/tegra20-common/pinmux.c OK, there's a semantic chasm here. This is not a 'driver' in my mind - it's just the pinmux code to set the bits in the PINMUX_AUX regs, and over half of it's tables & structs. Again, T30 reads in a few tables of pinmux/pingroup settings, and slams 'em out to the regs using the functions in pinmux.c (set_tristate, set_pullupdown, etc.). So would you be satisfied if I removed the GP sdio pad cfg writes from the common board-level code (common/board.c) and put them in a function that's called during pinmux_init(), early on in board_early_init_f()? That'll preclude being able to parse the mmc_id to see if I need to write SDIO1CFG or SDIO3CFG, so I'll have to slam 'em both, but that should be OK.
On 03/05/2013 10:21 AM, Tom Warren wrote: > On Tue, Mar 5, 2013 at 10:03 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 03/05/2013 08:28 AM, Tom Warren wrote: >>> On Mon, Mar 4, 2013 at 5:28 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> On 03/04/2013 04:11 PM, Tom Warren wrote: >>>>> On Wed, Feb 27, 2013 at 11:08 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>>> On 02/27/2013 09:59 AM, Tom Warren wrote: >>>>>>> On Tue, Feb 26, 2013 at 4:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> ... >>>>>> 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) We're discussing struct apb_misc_gp_ctrl fields sdio1cfg and sdio3cfg, right? Those /are/ pinmux registers. The pinmux HW has two sets of registers that feed into it; the pin mux selects (see 17.1.6 in the Tegra30 TRM) and the pad control registers (see 17.1.4 in the Tegra30 TRM). Both sets of registers should fully controlled by the pinmux driver, the values/tables being provided by a board-specific file. Perhaps a common table could be provided if all/many boards use the same value for some settings, i.e. in pinmux_init(): pinmux_config_padctrl(tegra_padctrl_sdio1_common, ARRAY_SIZE(...)); pinmux_config_padctrl(tegra_padctrl_sdio3_common, ...); pinmux_config_padctrl(tegra_padctrl_cardhu_specific, ...); or: pinmux_config_padctrl(cardhu_padctrl, ...); ... where cardhu_padctrl[] is probably defined in pinmux-config-cardhu.h, and could use some centralized macros to create the appropriate SDIO1CFG/... entries.
diff --git a/board/nvidia/cardhu/cardhu.c b/board/nvidia/cardhu/cardhu.c index df4cb6b..363bddc 100644 --- a/board/nvidia/cardhu/cardhu.c +++ b/board/nvidia/cardhu/cardhu.c @@ -24,6 +24,10 @@ #include <common.h> #include <asm/arch/pinmux.h> #include "pinmux-config-cardhu.h" +#include <i2c.h> + +#define PMU_I2C_ADDRESS 0x2D +#define MAX_I2C_RETRY 3 /* * Routine: pinmux_init @@ -37,3 +41,48 @@ void pinmux_init(void) pinmux_config_table(unused_pins_lowpower, ARRAY_SIZE(unused_pins_lowpower)); } + +#if defined(CONFIG_TEGRA_MMC) +/* + * Do I2C/PMU writes to bring up SD card bus power + * + */ +void board_sdmmc_voltage_init(void) +{ + uchar reg, data_buffer[1]; + int i; + + i2c_set_bus_num(0); /* PMU is on bus 0 */ + + data_buffer[0] = 0x65; + reg = 0x32; + + for (i = 0; i < MAX_I2C_RETRY; ++i) { + if (i2c_write(PMU_I2C_ADDRESS, reg, 1, data_buffer, 1)) + udelay(100); + } + + data_buffer[0] = 0x09; + reg = 0x67; + + for (i = 0; i < MAX_I2C_RETRY; ++i) { + if (i2c_write(PMU_I2C_ADDRESS, reg, 1, data_buffer, 1)) + udelay(100); + } +} + +/* + * Routine: pin_mux_mmc + * Description: setup the MMC muxes, power rails, etc. + */ +void pin_mux_mmc(void) +{ + /* + * NOTE: We don't do mmc-specific pin muxes here. + * They were done globally in pinmux_init(). + */ + + /* Bring up the SDIO1 power rail */ + board_sdmmc_voltage_init(); +} +#endif /* MMC */ diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c index babbe08..4ca6b29 100644 --- a/board/nvidia/common/board.c +++ b/board/nvidia/common/board.c @@ -49,6 +49,8 @@ #include <asm/arch-tegra/usb.h> #endif #ifdef CONFIG_TEGRA_MMC +#include <asm/arch/gp_padctrl.h> +#include <asm/arch-tegra/tegra_mmc.h> #include <asm/arch-tegra/mmc.h> #endif #include <i2c.h> @@ -245,4 +247,50 @@ int board_mmc_init(bd_t *bd) return 0; } -#endif + +void pad_init_mmc(struct tegra_mmc *reg) +{ +#if defined(CONFIG_TEGRA30) + struct apb_misc_gp_ctlr *const gpc = + (struct apb_misc_gp_ctlr *)NV_PA_APB_MISC_GP_BASE; + struct tegra_mmc *const sdmmc = (struct tegra_mmc *)reg; + u32 val, padcfg, padmask, offset = (unsigned int)reg; + + debug("%s: sdmmc address = %08x\n", __func__, (unsigned int)sdmmc); + + /* 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; + } + + /* Set pads as per T30 TRM, section 24.6.1.2 */ + padcfg = (GP_SDIOCFG_DRVUP_SLWF | GP_SDIOCFG_DRVDN_SLWR | \ + GP_SDIOCFG_DRVUP | GP_SDIOCFG_DRVDN); + padmask = 0x00000FFF; + + if (offset == TEGRA_SDMMC1_BASE) { + val = readl(&gpc->sdio1cfg); + val &= padmask; + val |= padcfg; + writel(val, &gpc->sdio1cfg); + } else {/* SDMMC3 */ + val = readl(&gpc->sdio3cfg); + val &= padmask; + val |= padcfg; + writel(val, &gpc->sdio3cfg); + } + + val = readl(&sdmmc->sdmemcmppadctl); + val &= 0xFFFFFFF0; + val |= MEMCOMP_PADCTRL_VREF; + writel(val, &sdmmc->sdmemcmppadctl); + + val = readl(&sdmmc->autocalcfg); + val &= 0xFFFF0000; + val |= AUTO_CAL_PU_OFFSET | AUTO_CAL_PD_OFFSET | AUTO_CAL_ENABLED; + writel(val, &sdmmc->autocalcfg); +#endif /* T30 */ +} +#endif /* MMC */
T30 requires specific SDMMC pad programming, and bus power-rail bringup. Signed-off-by: Tom Warren <twarren@nvidia.com> --- board/nvidia/cardhu/cardhu.c | 49 +++++++++++++++++++++++++++++++++++++++++ board/nvidia/common/board.c | 50 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 1 deletions(-)