From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Sat, 22 Apr 2017 19:18:42 -0600 Subject: [U-Boot] [PATCH v1] mmc: sdhci: SDHCI controllers also need power In-Reply-To: <1492602635.24567.79.camel@linux.intel.com> References: <20170315182521.4359-1-andriy.shevchenko@linux.intel.com> <1490014266.19767.101.camel@linux.intel.com> <1491052273.708.95.camel@linux.intel.com> <1354bacc-4ad5-91c4-c3d1-0b28cdf09617@samsung.com> <1491471989.24567.19.camel@linux.intel.com> <1492525775.24567.69.camel@linux.intel.com> <1492526753.24567.71.camel@linux.intel.com> <1492602635.24567.79.camel@linux.intel.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 Andy, On 19 April 2017 at 05:50, Andy Shevchenko wrote: > On Tue, 2017-04-18 at 18:12 -0600, Simon Glass wrote: >> Hi Andy, >> >> On 18 April 2017 at 08:45, Andy Shevchenko >> wrote: >> > On Tue, 2017-04-18 at 08:33 -0600, Simon Glass wrote: >> > > Hi Andy, >> > > >> > > On 18 April 2017 at 08:29, Andy Shevchenko >> > > wrote: >> > > > On Fri, 2017-04-07 at 19:05 +0900, Jaehoon Chung wrote: >> > > > > Hi Andy, >> > > > > >> > > > > On 04/06/2017 07:58 PM, Andy Shevchenko wrote: >> > > > > > On Thu, Apr 6, 2017 at 1:50 PM, Jaehoon Chung > > > > > > msun >> > > > > > g.co >> > > > > > m> wrote: >> > > > > > > On 04/06/2017 06:46 PM, Andy Shevchenko wrote: >> > > > > > > > On Thu, 2017-04-06 at 18:24 +0900, Jaehoon Chung wrote: >> > > > > > > > > On 04/06/2017 05:51 PM, Andy Shevchenko wrote: >> > > > > > > > > > On Thu, Apr 6, 2017 at 6:44 AM, Simon Glass > > > > > > > > > > omiu >> > > > > > > > > > m.or >> > > > > > > > > > g> >> > > > > > > > > > wrote: >> > > > > > > > > > > On 1 April 2017 at 07:11, Andy Shevchenko >> > > > > > > > > > > wrote: >> > > > > > > > > >> > > > > > > > > how about mmc_power_init() is called in mmc_probe()? >> > > > > > > > Yes, that's what I'm referring to. But the driver is >> > > > > > > > pure >> > > > > > > > SDHCI, >> > > > > > > > it >> > > > > > > > doesn't call mmc_probe() IIRC. >> > > > > > > >> > > > > > > After converting to DM, it might have the dependent to >> > > > > > > probing >> > > > > > > sequence. >> > > > > > > I'm not sure that u-boot has the priority for probing. >> > > > > > > maybe >> > > > > > > not... >> > > > > > > >> > > > > > > hmm..need to consider this patch..but i will think about >> > > > > > > more >> > > > > > > generic solution.. >> > > > > > >> > > > > > It would be nice to have a generic solution indeed. >> > > > > >> > > > > Just thinking about below..? >> > > > > >> > > > > vcc_sd: sdmmc-regulator { >> > > > > ... >> > > > > regulator-boot-on; >> > > > > or >> > > > > regulator-always-on; >> > > > > ... >> > > > > >> > > > > }; >> > > > > >> > > > > It should be always enabled.. >> > > > >> > > > Sorry, but no. It's not a regulator. >> > > > >> > > > If you would like to know details, the 2 bits in PMU registers >> > > > basically >> > > > represent clock gate and reset signal per IP which PMU controls. >> > > > >> > > > P.S. Hardware might have a common regulator per power island >> > > > which >> > > > is >> > > > automatically latches the power down if all devices on the >> > > > island >> > > > are on >> > > > D3hot. But it's not controlled by software. >> > > >> > > You have a few options: >> > > >> > > - Add a regulator/pmic driver for the PMU >> > >> > I dunno how many times should I repeat that it is *not* a PMIC at >> > all! >> > >> > PMIC is a separate *external* IC which is connected to Atom SoC. And >> > it >> > has nothing to do with PMU (on software level). >> >> That doesn't really matter though. The point is how it is modeled in >> U-Boot. > > Hardware matters. Software (drivers) represents whatever hardware design > is underneath. This is how Linux kernel at least being designed. Does U- > Boot follow the same paradigm? I'm not sure what you are getting at. Is there a call to board_mmc_power_init() in the middle of the Linux MMC stack? It breaks the driver model. While U-Boot's driver model is perhaps a bit stricter than Linux, on this point they surely agree. > >> > >> > > - Add a reset driver to handle the reset and perhaps a clock >> > > driver to >> > > handle the clock gate, then handle this in your driver >> > >> > No, I disclosed details just for your understanding that it's not a >> > regulator. On the other hand it's 1:1 mapping to D0/D3hot in PCI, >> > and >> > bits can't be switched separately by specification. >> > >> > TBH I even don't know which one is which. >> > >> > > You can subclass sdhci.c and adjust it as you need it. >> > > >> > > > >> > > > So, please consider my initial approach. >> > > >> > > We should use DM rather than custom hooks. >> > >> > Can anyone answer to a simple question why MMC code *has* been >> > calling >> > such hook and you strongly object to do the same / similar for >> > SDHCI? >> >> Can you point me to the mmc function you are referring to? > > drivers/mmc/mmc.c: > > -> (mmc-uclass.c) mmc_blk_probe() > -> mmc_init() > -> mmc_start_init() > -> mmc_power_init() > -> board_mmc_power_init() If you look at mmc_power_init() you can see it already has the code to enable a regulator. This is what you should be using. I've just sent a patch to clarify that: http://patchwork.ozlabs.org/patch/753851/ An alternative if you like is to enable the power in your board code before you even get to your MMC driver. > >> > > If this doesn't make sense >> > >> > It does not. >> > >> > > please let me know how I can help expound on it. >> > >> > Please, elaborate how pure SDHCI drivers are so different to MMC in >> > init >> > stage and why, but please don't offer regulators. >> >> It's just that we cannot call a board hook function from DM. > > World is not ideal, and for me is clear that DM is not ideal either. Ideal in what sense? > >> That's >> the way things used to work, but with DM we need to have things in the >> driver. >> >> I'm sorry if you're finding this frustrating, but I do want to >> understand this. While it seems like a minor point it actually is a >> key design feature of DM. > > I understand your point. And I am all ears to implement the best of > possible solutions (with current U-Boot design), OTOH I don't like any > idea of faking in software something that is not present on real > platform (like doing weird PMIC or regulators for PMU which is not > either of them). What exactly is it, then? My understanding is that it is a bit in the register that controls power and reset for the MMC controller. Is that right? In that case, with driver model, we would use a regulator and a reset. If you really don't want to do that, then you can put the code in your board init e.g. in board_early_init_r(). > > Please, understand my point as well. > > The other option (not a good one from user experience) is to disable SD > card slot in U-Boot completely. You could start with that, and then enable it later once you have the rest of the support in if you like. I'm sorry that I being so inflexible here. I can tell that you are frustrated. But so far I cannot see a compelling reason to break the driver model approach here. While some boards have challenges I have not seen any big issues with dealing with power and reset with REGULATOR and RESET drivers. Regards, Simon