Re: [PATCH v10 03/15] ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs

[Ulf Hansson <ulf.hansson@linaro.org>]

[snip]

> On 05/05/2014 02:41 PM, Ulf Hansson wrote:
>>> +struct sunxi_mmc_host {
>>> +       struct mmc_host *mmc;
>>> +       struct regulator *vmmc;
>>
>> Instead of having a specific regulator for this driver, please use the
>> mmc_regulator_get_supply API.
>
> We cannot use mmc_regulator_get_supply because for the sunxi mmc controller
> not only vqmmc but also vmmc itself is optional, and mmc_regulator_get_supply
> calls devm_regulator_get rather then devm_regulator_get_optional for vmmc.

Is that because the mmc controller handle the power to the card or
because you have a fixed supply?

Having a fixed regulator supply could easily be set up in DT, which
then also dynamically gives you the ocr mask instead of having a them
"hard coded".

>
> Using mmc_regulator_get_supply would lead to false postive errors being logged
> on 99/100 boards.

I was kind of expecting a response like this. :-) Actually I would
prefer if we could make the API suit drivers like this one as well.

For reference, there are currently a patch being discussed which
relates to this topic.
"mmc: core: Improve support for deferred regulators"

>
>>
>>> +       struct reset_control *reset;
>>> +
>>> +       /* IO mapping base */
>>> +       void __iomem    *reg_base;
>>> +
>>> +       spinlock_t      lock;
>>> +       struct tasklet_struct manual_stop_tasklet;
>>
>> Any reason why you can't use a threaded IRQ handler instead of a tasklet?
>
> AFAIK IRQ threaded handlers always have the highest priority. When
> the manual_stop_tasklet runs we disable irqs and start polling to
> recover from an error condition, which is nothing something I want
> todo with the highest priority on the system.

To me, that seems like a good match for a threaded irq handler.

I suppose you could change priority of the kthread that executes the
threaded irq handler, if you need that.

[snip]

>>> +static void sunxi_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>> +{
>>> +       struct sunxi_mmc_host *host = mmc_priv(mmc);
>>> +       u32 rval;
>>> +       s32 err;
>>> +
>>> +       /* Set the power state */
>>> +       switch (ios->power_mode) {
>>> +       case MMC_POWER_ON:
>>> +               break;
>>> +
>>> +       case MMC_POWER_UP:
>>> +               if (!IS_ERR(host->vmmc)) {
>>> +                       mmc_regulator_set_ocr(host->mmc, host->vmmc, ios->vdd);
>>> +                       udelay(200);
>>> +               }
>>> +
>>> +               err = sunxi_mmc_init_host(mmc);
>>
>> So, sunxi_mmc_init_host() will ungate the clocks - but you shouldn't
>> use ->set_ios() callback to implement power save.
>>
>> Clocks should be ungated at ->probe() and gated at ->remove().
>>
>> If fine grained power save is wanted, I advise you to rework the clock
>> handling - and to build it upon runtime PM instead. Typically you
>> would do these adaptations:
>>
>> 1. Besides enabling the clocks at ->probe(), also enable runtime PM
>> and use the runtime PM auto-suspend feature.
>> 2. Add pm_runtime_get|put at the proper places in the driver.
>> 3. Implement the runtime PM callbacks (suspend|resume) and do clock
>> gating|ungating from there.
>>
>> You may refer to drivers/mmc/host/mmci.c to get an example.
>
> I'll just move the enable / disable to probe / remove for now.

Seems reasonable!

While you do that, it would be nice to have some understanding whether
the reset sequence, also performed by sunxi_mmc_init_host(), is
connected to clk gating|ungating, or a power cycle. I guess you will
find out during testing. :-)

>
>>> +               if (err) {
>>> +                       host->ferror = 1;
>>> +                       return;
>>> +               }
>>> +
>>> +               enable_irq(host->irq);

Just realize that I also think you should move the enable|disable_irq
to ->probe|remove().

That will mean you will be better prepared to implement runtime PM
support and thus make it possible to disable irqs during request
inactivity.

>>> +
>>> +               dev_dbg(mmc_dev(host->mmc), "power on!\n");
>>> +               host->ferror = 0;
>>> +               break;
>>> +
>>> +       case MMC_POWER_OFF:
>>> +               dev_dbg(mmc_dev(host->mmc), "power off!\n");
>>> +               disable_irq(host->irq);
>>> +               sunxi_mmc_exit_host(host);
>>
>> See comment above for sunxi_mmc_init_host().
>>
>>> +               if (!IS_ERR(host->vmmc))
>>> +                       mmc_regulator_set_ocr(host->mmc, host->vmmc, 0);
>>> +
>>> +               host->ferror = 0;
>>> +               break;
>>> +       }
>>> +
>>> +       /* set bus width */
>>> +       switch (ios->bus_width) {
>>> +       case MMC_BUS_WIDTH_1:
>>> +               mci_writel(host, REG_WIDTH, SDXC_WIDTH1);
>>> +               host->bus_width = 1;
>>> +               break;
>>> +       case MMC_BUS_WIDTH_4:
>>> +               mci_writel(host, REG_WIDTH, SDXC_WIDTH4);
>>> +               host->bus_width = 4;
>>> +               break;
>>> +       case MMC_BUS_WIDTH_8:
>>> +               mci_writel(host, REG_WIDTH, SDXC_WIDTH8);
>>> +               host->bus_width = 8;
>>> +               break;
>>> +       }
>>> +
>>> +       /* set ddr mode */
>>> +       rval = mci_readl(host, REG_GCTRL);
>>> +       if (ios->timing == MMC_TIMING_UHS_DDR50) {
>>> +               rval |= SDXC_DDR_MODE;
>>> +               host->ddr = 1;
>>> +       } else {
>>> +               rval &= ~SDXC_DDR_MODE;
>>> +               host->ddr = 0;
>>> +       }
>>> +       mci_writel(host, REG_GCTRL, rval);
>>> +
>>> +       /* set up clock */
>>> +       if (ios->clock && ios->power_mode) {
>>> +               dev_dbg(mmc_dev(host->mmc), "ios->clock: %d\n", ios->clock);
>>> +               sunxi_mmc_clk_set_rate(host, ios->clock);
>>> +               usleep_range(50000, 55000);
>>
>> Is those values for usleep really correct? I am not sure how many
>> times we execute this path while detecting/powering the card, but
>> quite a few.
>> Detecting/powering the card is also done during each system
>> suspend/resume cycle - thus this will heavily affect these cycles.
>
> The problem is we've no docs, so this is all based on android code, the
> android code has 2 drivers, lets call them the old and the new one.
>
> This works is based on the new driver as that one was significantly
> cleaner then the old driver. This bit comes directly from the new driver,
> but it seems that the old driver has no delay at all. And clk_set_rate
> already does a busy-wait waiting for the hardware to acknowledge the
> clock rate change, so I think this is not really necessary. I'll run
> some tests with it removed and if everything still works I'll drop it.

Okay, great!

Maybe we could add some comments, no matter what!?

[snip]

>>> +       if (cmd->opcode == MMC_GO_IDLE_STATE) {
>>> +               cmd_val |= SDXC_SEND_INIT_SEQUENCE;
>>> +               imask |= SDXC_COMMAND_DONE;
>>
>> This seems really strange! Please elaborate on why need specific
>> handling of this CMD.
>
> I've no clue, again no docs.

I see the problem. :-)

[snip]

Kind regards
Ulf Hansson