[U-Boot] [PATCH v1] mmc: sdhci: SDHCI controllers also need power

Sun Apr 23 01:18:42 UTC 2017

Hi Andy,

On 19 April 2017 at 05:50, Andy Shevchenko
<andriy.shevchenko at linux.intel.com> wrote:
> On Tue, 2017-04-18 at 18:12 -0600, Simon Glass wrote:
>> Hi Andy,
>>
>> On 18 April 2017 at 08:45, Andy Shevchenko
>> <andriy.shevchenko at linux.intel.com> wrote:
>> > On Tue, 2017-04-18 at 08:33 -0600, Simon Glass wrote:
>> > > Hi Andy,
>> > >
>> > > On 18 April 2017 at 08:29, Andy Shevchenko
>> > > <andriy.shevchenko at linux.intel.com> 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 <jh80.chung at sa
>> > > > > > 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 <sjg at chr
>> > > > > > > > > > omiu
>> > > > > > > > > > m.or
>> > > > > > > > > > g>
>> > > > > > > > > > wrote:
>> > > > > > > > > > > On 1 April 2017 at 07:11, Andy Shevchenko
>> > > > > > > > > > > <andriy.shevchenko at linux.intel.com> 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