Hi Ulf,
Am 28.08.2018 um 13:25 schrieb Ulf Hansson:
> Oleksij,
>
> First, again, apologize for the delay in giving feedback.
no problem.
> On 18 June 2018 at 07:19, Oleksij Rempel <linux@xxxxxxxxxxxxxxxx> wrote:
>> this driver provides support for Alcor Micro AU6601 and AU6621
>
> On what platforms are these being used? Would you mind adding some of
> that information to the changelog?
ok.
So far I know, this controller are used in different notebooks and
available as mini PCIe cards. Currently I have two ASUS notebooks with
AU6601 and AU6621. I also plan to test AU6601 on iMX6Quad board. With
other word, every platform with PCI support can theoretically use this
driver.
>> Signed-off-by: Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>
>> ---
>> drivers/mmc/host/Kconfig | 9 +
>> drivers/mmc/host/Makefile | 1 +
>> drivers/mmc/host/au6601.c | 1744 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 1754 insertions(+)
>> create mode 100644 drivers/mmc/host/au6601.c
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 9589f9c9046f..7112d1fbba6d 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -421,6 +421,15 @@ config MMC_WBSD
>>
>> If unsure, say N.
>>
>> +config MMC_AU6601
>> + tristate "Alcor Micro AU6601"
>> + depends on PCI
>> + help
>> + This selects the Alcor Micro Multimedia card interface.
>> +
>> + If unsure, say N.
>> +
>> +
>> config MMC_AU1X
>> tristate "Alchemy AU1XX0 MMC Card Interface support"
>> depends on MIPS_ALCHEMY
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index 6aead24879b4..b8d4271e7b29 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -22,6 +22,7 @@ obj-$(CONFIG_MMC_SDHCI_F_SDH30) += sdhci_f_sdh30.o
>> obj-$(CONFIG_MMC_SDHCI_SPEAR) += sdhci-spear.o
>> obj-$(CONFIG_MMC_WBSD) += wbsd.o
>> obj-$(CONFIG_MMC_AU1X) += au1xmmc.o
>> +obj-$(CONFIG_MMC_AU6601) += au6601.o
>> obj-$(CONFIG_MMC_MTK) += mtk-sd.o
>> obj-$(CONFIG_MMC_OMAP) += omap.o
>> obj-$(CONFIG_MMC_OMAP_HS) += omap_hsmmc.o
>> diff --git a/drivers/mmc/host/au6601.c b/drivers/mmc/host/au6601.c
>> new file mode 100644
>> index 000000000000..d9e2c0fc4ef8
>> --- /dev/null
>> +++ b/drivers/mmc/host/au6601.c
>> @@ -0,0 +1,1744 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2018 Oleksij Rempel <linux@xxxxxxxxxxxxxxxx>
>> + *
>> + * Direver for Alcor Micro AU6601 and AU6621 controllers
>
> /s/Direver/Driver
>
>> + */
>> +
......
>> +#define AU6621_DMA_CTRL 0x0c
>> +#define AU6621_DMA_ENABLE BIT(0)
>> +/* ADMA phy address. AU6621 only. */
>> +#define REG_10 0x10
>
> I understand you don't have the full documentation to the controller.
> However, instead of adding a useless defines here, let's just add a
> comment somewhere, describe the unknown register bits as "unknown".
>
> This comment applies to several more places in this file.
ok, thx.
>
>> +/* CMD index */
>> +#define AU6601_REG_CMD_OPCODE 0x23
>> +/* CMD parametr */
>> +#define AU6601_REG_CMD_ARG 0x24
>> +/* CMD response 4x4 Bytes */
>> +#define AU6601_REG_CMD_RSP0 0x30
>> +#define AU6601_REG_CMD_RSP1 0x34
>> +#define AU6601_REG_CMD_RSP2 0x38
>> +#define AU6601_REG_CMD_RSP3 0x3C
>> +/* LED ctrl? */
>> +#define REG_51 0x51
>
> Again, remove the define, but feel free to keep some useful comments.
>
>> +/* ??? */
>
> Please avoid all these questions marks. Better to just state what it's
> unknown - or if you have reasons to believe it has a purpose, then
> tell it and why.
>
> Again, this applies to more places in the file.
>
>> +#define REG_52 0x52
>> +/* LED related? Always toggled BIT0 */
>> +#define REG_61 0x61
...
>> +#define REG_77 0x77
>> +/* looks like soft reset? */
>> +#define AU6601_REG_SW_RESET 0x79
>
> If I have read the code correctly, you have actually implemented the
> reset. So, does it work or not? More comments about this later.
ok, i'll remove it
>> +#define AU6601_BUF_CTRL_RESET BIT(7)
>> +#define AU6601_RESET_DATA BIT(3)
>> +#define AU6601_RESET_CMD BIT(0)
....
>> +#define AU6601_INT_ALL_MASK ((u32)-1)
>> +
>> +/* MS_CARD mode registers */
>
> Hmm, is this both a SD card and memstick capable PCI controller?
yes.
> Is there plans to implement the memstick parts as well?
yes.
> More comments about this later.
>
>> +
>> +#define AU6601_MS_STATUS 0xa0
...
>> +
>> +static unsigned use_dma = 1;
>> +module_param(use_dma, uint, 0);
>> +MODULE_PARM_DESC(use_dma, "Whether to use DMA or not. Default = 1");
>
> If possible, I would avoid module parameters. Why, exactly, is this needed?
ok
> Could it be useful to instead deploy a policy of always trying DMA and
> if it fails, fallback to use PIO?
Yes, this was my first attempt, i'll think about it.
>> +
>> +enum au6601_cookie {
>> + COOKIE_UNMAPPED,
>> + COOKIE_PRE_MAPPED, /* mapped by pre_req() of dwmmc */
>> + COOKIE_MAPPED, /* mapped by prepare_data() of dwmmc */
>> +};
....
>
> Looks unused.
ok. thx.
>> +
>> + struct mmc_host *mmc;
>> + struct mmc_request *mrq;
>> + struct mmc_command *cmd;
>> + struct mmc_data *data;
>> + unsigned int dma_on:1;
>> + unsigned int early_data:1;
>> + bool use_dma;
>> +
>> + struct mutex cmd_mutex;
>> + spinlock_t lock;
>
> Can you please elaborate on how the spinlock are being used, I think
> it looks a bit suspicious.
>
>> +
>> + struct delayed_work timeout_work;
>> +
>> + struct sg_mapping_iter sg_miter; /* SG state for PIO */
>> + struct scatterlist *sg;
>> + unsigned int blocks; /* remaining PIO blocks */
>> + int sg_count;
>> +
>> + u32 irq_status_sd;
>> + struct au6601_dev_cfg *cfg;
>> + unsigned char cur_power_mode;
>> + unsigned char cur_bus_mode;
>
> cur_bus_mode seems to be unused.
>
>> +
>> + /* aspm section */
>> + int pdev_cap_off;
>> + u8 pdev_aspm_cap;
>> + int parent_cap_off;
>> + u8 parent_aspm_cap;
>> + u8 ext_config_dev_aspm;
>
> Can you please elaborate a bit on what these are being used for and
> then also add some comment next to the definitions.
It is local PCI ASPM implementation. So far I didn't found anything
generic. May be some hints from PCI experts would be welcome here.
>> +};
>> +
>> +static const struct au6601_pll_conf au6601_pll_cfg[] = {
>> + /* MHZ, CLK src, max div, min div */
>> + { 31250000, AU6601_CLK_31_25_MHZ, 1, 511},
>> + { 48000000, AU6601_CLK_48_MHZ, 1, 511},
>> + {125000000, AU6601_CLK_125_MHZ, 1, 511},
>> + {384000000, AU6601_CLK_384_MHZ, 1, 511},
>> +};
>> +
>> +static void au6601_send_cmd(struct au6601_host *host,
>> + struct mmc_command *cmd);
>> +
>> +static void au6601_prepare_data(struct au6601_host *host,
>> + struct mmc_command *cmd);
>> +static void au6601_finish_data(struct au6601_host *host);
>> +static void au6601_request_complete(struct au6601_host *host,
>> + bool cancel_timeout);
>> +static int au6601_get_cd(struct mmc_host *mmc);
>
> Please put the implementation of the functions in the correct order,
> thus avoid these declarations altogether.
ok.
>> +
>> +static const struct au6601_dev_cfg au6601_cfg = {
>> + .dma = 0,
>> +};
>> +
>> + 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?
>> +}
>> +
>> +static void au6601_write8(struct au6601_host *host, u8 val,
>> + unsigned int addr)
>> +{
>> + au6601_reg_decode(1, 1, val, addr);
>> +}
>> +
>> +static int pci_find_cap_offset(struct au6601_host *host, struct pci_dev *pci)
>> +{
>> + int where;
>> + u8 val8;
>> + u32 val32;
>> +
>> +#define CAP_START_OFFSET 0x34
>
> I prefer putting defines in the beginning of the file, rather than
> putting them inside functions like this.
>
> This applies to several more places in this file, please fix them as well.
ok.
>> +
>> + where = CAP_START_OFFSET;
>> + pci_read_config_byte(pci, where, &val8);
>> + if (!val8) {
>> + return 0;
>> + }
>> +
>> + where = (int)val8;
>> + while (1) {
>> + pci_read_config_dword(pci, where, &val32);
>> + if (val32 == 0xffffffff) {
>> + dev_dbg(host->dev, "pci_find_cap_offset invailid value %x.\n", val32);
>> + return 0;
>> + }
>> +
>> + if ((val32 & 0xff) == 0x10) {
>> + dev_dbg(host->dev, "pcie cap offset: %x\n", where);
>> + return where;
>> + }
>> +
>> + if ((val32 & 0xff00) == 0x00) {
>> + dev_dbg(host->dev, "pci_find_cap_offset invailid value %x.\n", val32);
>> + break;
>> + }
>> + where = (int)((val32 >> 8) & 0xff);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/* FIXME: return results are currently ignored */
>
> This looks lazy to me, sorry.
>
> Please, wither deal with the return code or convert the function to a
> void. Whatever you prefer - and then remove the FIXME comment.
ok.
>> +static int pci_init_check_aspm(struct au6601_host *host)
>> +{
>> +#define PCIE_LINK_CAP_OFFSET 0x0c
>> +
>> + struct pci_dev *pci;
>> + int where;
>> + u32 val32;
>> +
>> + dev_dbg(host->dev, "pci_init_check_aspm\n");
>> +static void au6601_reset(struct au6601_host *host, u8 val)
>> +{
>> + int i;
>> +
>> + au6601_write8(host, val | AU6601_BUF_CTRL_RESET,
>> + AU6601_REG_SW_RESET);
>> + for (i = 0; i < 100; i++) {
>> + if (!(au6601_read8(host, AU6601_REG_SW_RESET) & val))
>> + return;
>> + udelay(50);
>> + }
>> + dev_err(host->dev, "%s: timeout\n", __func__);
>> +}
>> +
>
> Here is the reset function I mentioned above. Is it used and does it work?
yes, it should reset internal state machine. Not sure how big is impact
of this reset. I have no documentation.
> [...]
>
>> +
>> +/*****************************************************************************\
>> + * *
>> + * Core functions *
>
> To me, these kind of section markers are a bit silly, please just drop
> them for the file.
ok.
>> + * *
>> +\*****************************************************************************/
>
> [...]
>
>> +
>> +static void au6601_send_cmd(struct au6601_host *host,
>> + struct mmc_command *cmd)
>> +{
>> + unsigned long timeout;
>> + u8 ctrl = 0;
>> +
>> + cancel_delayed_work_sync(&host->timeout_work);
>
> This can deadlock, because you are holding the mutex already and the
> delayed work may try to lock the same mutex. See
> au6601_timeout_timer().
>
> Fix this, simply by calling cancel_delayed_work_sync() before you have
> locked the mutex.
ok.
>> +
>> + if (!cmd->data && cmd->busy_timeout)
>> + timeout = cmd->busy_timeout;
>> + else
>> + timeout = 10000;
>
> Please make this a define instead. Simply because it's and important
> value and using a define makes it easier to see and possibly change
> it.
ok.
>> +
>> + host->cmd = cmd;
>> + au6601_prepare_data(host, cmd);
>> +
>> +
>> + schedule_delayed_work(&host->timeout_work, msecs_to_jiffies(timeout));
>> +}
>> +
>
> [...]
>
>> +
>> +static void au6601_cmd_irq_thread(struct au6601_host *host, u32 intmask)
>> +{
>> + intmask &= AU6601_INT_CMD_END;
>> +
>> + if (!intmask)
>> + return;
>> +
>> + if (!host->cmd && intmask & AU6601_INT_CMD_END) {
>> + dev_err(host->dev,
>> + "Got command interrupt 0x%08x even though no command operation was in progress.\n",
>> + intmask);
>
> It's not unusual that devices generates spurious IRQs, because of buggy HW.
ok, i'll check it.
> How critical is this error? Should it be converted to dev_warn or
> possibly even dev_dbg?
>
> Moreover, overall, I would suggest you to go over all prints. Make
> sure those are printed at the correct level and likely drop lots of
> dev_dbg, as there are quite many and we also have ftrace to use
> instead.
ok.
>> + }
>> +
>> + dev_dbg(host->dev, "%s %x\n", __func__, intmask);
>> +
>> + /* Processed actual command. */
>> + if (!host->data)
>> + au6601_request_complete(host, 1);
>> + else
>> + au6601_trigger_data_transfer(host, false);
>> + host->cmd = NULL;
>> +}
>
> [...]
>
>> +static int au6601_card_busy(struct mmc_host *mmc)
>> +{
>> + struct au6601_host *host = mmc_priv(mmc);
>> + u8 status;
>> +
>> + dev_dbg(host->dev, "%s:%i\n", __func__, __LINE__);
>> + /* Check whether dat[0:3] low */
>> + status = au6601_read8(host, AU6601_DATA_PIN_STATE);
>> +
>> + return !(status & AU6601_BUS_STAT_DAT_MASK);
>> +}
>
> [...]
>
>> +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
>
> 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.
>> + break;
>> + default:
>> + /* No signal voltage switch required */
>> + break;
...
>> + struct mmc_request *mrq;
>> +
>> + /*
>> + * If this tasklet gets rescheduled while running, it will
>> + * be run again afterwards but without any active request.
>> + */
>
> There is no tasklet. Please re-word or remove this comment.
ok.
>> + if (!host->mrq) {
>> + dev_dbg(host->dev, "nothing to complete\n");
>> + return;
>> + }
>> +
>> + if (cancel_timeout)
>> + cancel_delayed_work_sync(&host->timeout_work);
>> +
...
>> +
>> +
>> +
>
> I couple of unnecessary line-breaks.
>
>> +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.
> 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.
>
>> + 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.
> At least a few comments about that would be helpful.
>
>> + mmc->caps2 = MMC_CAP2_NO_SDIO;
>> + mmc->ops = &au6601_sdc_ops;
>> +
>> + /* Hardware cannot do scatter lists */
>> + mmc->max_segs = host->use_dma ? AU6601_MAX_DMA_SEGMENTS
>> + : AU6601_MAX_PIO_SEGMENTS;
>> + mmc->max_seg_size = host->use_dma ? AU6601_MAX_DMA_BLOCK_SIZE
>> + : AU6601_MAX_PIO_BLOCK_SIZE;
>> +
>> + mmc->max_blk_size = mmc->max_seg_size;
>> + mmc->max_blk_count = mmc->max_segs;
>> +
>> + mmc->max_req_size = mmc->max_seg_size * mmc->max_segs;
>> +}
>> +
>> +static void au6601_hw_init(struct au6601_host *host)
>> +{
>> + struct au6601_dev_cfg *cfg = host->cfg;
>> +
>> + au6601_reset(host, AU6601_RESET_CMD);
>> +
>> + au6601_write8(host, 0, AU6601_DMA_BOUNDARY);
>> + au6601_write8(host, AU6601_SD_CARD, AU6601_ACTIVE_CTRL);
>> +
>> + au6601_write8(host, 0, AU6601_REG_BUS_CTRL);
>> +
>> + au6601_reset(host, AU6601_RESET_DATA);
>> + au6601_write8(host, 0, AU6601_DMA_BOUNDARY);
>> +
>> + au6601_write8(host, 0, AU6601_INTERFACE_MODE_CTRL);
>> + au6601_write8(host, 0x44, AU6601_PAD_DRIVE0);
>> + au6601_write8(host, 0x44, AU6601_PAD_DRIVE1);
>> + au6601_write8(host, 0x00, AU6601_PAD_DRIVE2);
>> +
>> + /* kind of read eeprom */
>
> Please rephrase this to something that makes more sense.
>
>> + au6601_write8(host, 0x01, AU6601_FUNCTION);
>> + au6601_read8(host, AU6601_FUNCTION);
>> +
>> + /* for 6601 - dma_boundary; for 6621 - dma_page_cnt */
>> + au6601_write8(host, cfg->dma, AU6601_DMA_BOUNDARY);
>> +
>> + au6601_write8(host, 0, AU6601_OUTPUT_ENABLE);
>> + au6601_write8(host, 0, AU6601_POWER_CONTROL);
>> + pci_aspm_ctrl(host, 1);
>> +
>> + host->dma_on = 0;
>> +
>> + au6601_write8(host, AU6601_DETECT_EN, AU6601_DETECT_STATUS);
>> + /* now we should be safe to enable IRQs */
>> + au6601_unmask_sd_irqs(host);
>> + /* currently i don't know how to properly handle MS IRQ
>> + * and HW to test it. */
>
> Perhaps re-phrase to something along the lines of saying that the
> memstick support is currently not implemented, due to lack of
> documentation.
ok.
> 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.
- 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..
Please stop me if go too far :)
--
Regards,
Oleksij
Attachment:
signature.asc
Description: OpenPGP digital signature
