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

[Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

Hi,

On 05/05/2014 10:33 PM, Ulf Hansson wrote:
> [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".

It is because the sdcard slot power tends to be hooked directly to the 3.3V
of the board. So in a sense this is a fixed regulator, but I really, REALLY
don't want to add fixed regulator boilerplate to all sunxi dts files for this.

In other subsystems where there are similar cases (ie ahci-platform, supply for
various ethernet phys), the regulator is always optional and does not need
to be specified in the dts when the device is just hardwired to the power.

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

Ok, so that patch seems to replace the somewhat alarming message
reported by devm_regulator_get by an acceptable:

dev_info(dev, "No vmmc regulator found\n");

I can live with that, so I'm going to assume that something like that
patch will get merged in the near future and I'll switch to mmc_regulator_get_supply
in the next version and just live with the error messages this causes for now.

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

Ok, I've done some reading up on threaded irq handlers and I'll
I'll convert this to a threaded irq handler (only using the thread for the error
handling case).

<snip>

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

Ok.

<snip>

>>>> +       /* 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!?

Yeah I'll add a comment that there used to be a usleep there :)

Regards,

Hans