[...]
>>> + pr_debug("%s.%i: 0x%02x 0x%08x (%s)\n", write ? "> w" : "< r",
>>> + size, addr_short, val, reg);
>>
>> Maybe you want this debug function behind a specific debug define?
>> Thus use a stub function when not defined.
>>
>> Otherwise you will walk this code always for every register access,
>> seems like unnecessary overhead. No?
>
> Most of HW information is based on reverse engineering of windows driver
> with QEMU, then extended with source provided some years later by the
> vendor. I still need to some how remote debug it by users. Is tracing
> interface suitable for this scenario?
Yes, I think so. Of course it depends a little bit on what
circumstances you want to collect the debug data.
But more importantly, it has with very little impact on performance.
At this point I would invent a debug define, then you can convert to
tracing in following steps.
[...]
>>
>>> +static int au6601_signal_voltage_switch(struct mmc_host *mmc,
>>> + struct mmc_ios *ios)
>>> +{
>>> + struct au6601_host *host = mmc_priv(mmc);
>>> +
>>> + mutex_lock(&host->cmd_mutex);
>>> +
>>> + dev_dbg(host->dev, "%s:%i\n", __func__, __LINE__);
>>> + switch (ios->signal_voltage) {
>>> + case MMC_SIGNAL_VOLTAGE_330:
>>> + au6601_rmw8(host, AU6601_OPT, AU6601_OPT_SD_18V, 0);
>>> + break;
>>> + case MMC_SIGNAL_VOLTAGE_180:
>>> + au6601_rmw8(host, AU6601_OPT, 0, AU6601_OPT_SD_18V);
>>
>> So the 1.8V signal voltage is typically used for SD cards running in
>> UHS mode, such as SDR12, SDR25, etc.
>>
>> The reason why I bring it up, is because I couldn't find anywhere
>> during ->probe() when you set the corresponding mmc caps for these
>> speed modes.
>
> I didn't set it, because i was not able to properly test it. According
> to the product brief, AU6621 is supporting SDR104/SDR50
Great.
I assume that means also SDR25, which is also an UHS-I mode which uses
1.8V I/O signaling voltage.
>
>>
>> Does, that means some of the code above is untested? If so, at least
>> that should be documented by some comments in the code.
>
> I tested this code with different SD and MMC cards, i can list exact
> names if needed.
> In general, i'll be thankful for any testing, validation suggestions.
Most cards should supports SDR25, if not too old, at least. So my
recommendation is to start with enabling that mmc cap and see if gets
properly detected and initialized.
That mode doesn't require any turnings to happen, so it's the first
step of validating the 1.8V I/O support.
[...]
>>> +static void au6601_init_mmc(struct au6601_host *host)
>>> +{
>>> + struct mmc_host *mmc = host->mmc;
>>> +
>>> + mmc->f_min = AU6601_MIN_CLOCK;
>>> + mmc->f_max = AU6601_MAX_CLOCK;
>>> + /* mesured Vdd: 3.4 and 1.8 */
>>> + mmc->ocr_avail = MMC_VDD_165_195 | MMC_VDD_33_34;
>>
>> People tends to confuse VDD power with the I/O (signal) voltage level.
>> VDD is the power to the card. Supporting a VDD of MMC_VDD_165_195, is
>> typically only to support eMMC devices.
>
> I have 1GB Kingston MMC mobile, dual-voltage cards.
Honestly, I don't re-call when I used a removable MMC card the last
time. Not to speak about dual-voltage MMC cards.
However, in principle this should work as validation, I think, but
don't take my word for it.
>
>> Just making sure the above is correct?
>
> ok, i'll recheck it.
>
>>
>> BTW, how do you control the different voltage levels? I guess that is
>> internally by the PCI controller HW, no?
>
> Setting AU6601_OPT_SD_18V bit.
Hold on, this doesn't seem right. You are using that same bit to
change to I/O signaling voltage!?
As I stated above, I/O signalling voltage is not the same power as the
VDD power, it's two different voltages.
The I/O signalling voltage is the term for the voltage level on the
data/clk lines towards the card. The bus interface voltage.
The VDD is the term for the actual power to the internals of card. The
internals of the card could be things as a NAND-flash array, the flash
controller, etc.
Please go back and double check, so we get this right.
>
>>
>>> + mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED;
>>
>> As stated earlier, it seems like the controller supports also SD
>> card's UHS-modes. Is it something you tried?
>
> Currently i use Intenso SDHC UHS-I 32GB card, with this code i can get
> about 40MB/s. If I see it correctly, I also need to implement tuning.
40MB/s, that's more than what the MMC_CAP_SD_HIGHSPEED should give you. :-)
MMC_CAP_SD_HIGHSPEED uses 50MHz and 4 data lines - > 25MB/s. Are you
sure you didn't measure access to file system cache?
In regards to tuning, it depends on what speed mode you want to run it
in. Tuning isn't needed for MMC_CAP_UHS_SDR25|12.
As stated earlier, I would try to validate the 1.8V I/O voltage first,
simply set MMC_CAP_UHS_SDR25|12 and see if the cards get detected as
an UHS card. You should see something about "ultra high speed SDR25"
in the log.
>From a throughput point of view, you should get the similar numbers as
MMC_CAP_UHS_SDR25 also uses 50MHz and 4 data lines.
[...]
>> Anyway, clearly there is memstick part. That makes me think that we
>> should split this driver. One part would then have to be moved into
>> drivers/misc/cardreader/* and the needed helper functions to
>> write/read etc data needs to be exported so the mmc driver can use it.
>>
>> The misc device should be represented by the PCI device. During its
>> probe, it should add an mfd child device for the corresponding sd-card
>> controller part (the memstick device can be left to the future).
>> Finally, the mmc driver should be converted to a platform driver.
>>
>> If you want some references, have a look at:
>> drivers/misc/cardreader/rtsx_pcr.c
>> drivers/mmc/host/rtsx_pci_sdmmc.c
>>
>> Doing this split, would prepare for implementing the memstick part,
>> without having to re-factor all of the code.
>>
>> Does it make sense?
>
> yes, it is.
>
> There are some more points which will make sense for MFD splitting to
> more components. For example:
> - clock seems to support external source, in this case binding it with
> clock framework may make sense.
True. Actually I was thinking about that as well.
> - a bit for toggling what ever connected to the extarnal pin can be
> combined with LED framework.
> at least for external clock and what ever LED (if installed) some kind
> of configuration should be provided - quirks, devicetree..
Seems reasonable as well.
>
> Please stop me if go too far :)
My only point to raise, is to try to keep it as simple as possible
initially, that makes it easier to get it upstream. Then we can build
and extend the drivers along the road, on top.
Kind regards
Uffe
