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

[snip]

On 05/05/2014 02:41 PM, Ulf Hansson wrote:

quoted

quoted

+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"

quoted

quoted

+       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]

quoted

quoted

+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. :-)

quoted

quoted

+               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.

quoted

quoted

+
+               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().

quoted

+               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]

quoted

quoted

+       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