Re: [PATCH] mmc: add new au6601 driver

[Oleksij Rempel <linux@rempel-privat.de>]

[-- Attachment #1: Type: text/plain, Size: 17233 bytes --]

Hi Ulf,

Am 16.06.2014 12:02, schrieb Ulf Hansson:
> Hi Oleksij,
> 
> Sorry for a late reply.

no problem. Right now i have problems to response too :(

> 
> On 8 May 2014 11:58, Oleksij Rempel <linux@rempel-privat.de> wrote:
>> This driver is based on documentation which was based on my RE-work and
>> comparision with other MMC drivers.
>> It works in legacy mode and can provide 20MB/s R/W spead for most modern
>> SD cards, even if at least 40MB/s should be possible on this hardware.
>> It was not possible provide description for all register. But some of them
>> are important to make this hardware work in some unknown way.
>>
>> Biggest part of RE-work was done by emulating AU6601 in QEMU.
>>
>> Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
>> ---
>>  drivers/mmc/host/Kconfig  |    8 +
>>  drivers/mmc/host/Makefile |    1 +
>>  drivers/mmc/host/au6601.c | 1276 +++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 1285 insertions(+)
>>  create mode 100644 drivers/mmc/host/au6601.c
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 8aaf8c1..99b309f 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -315,6 +315,14 @@ config MMC_WBSD
>>
>>           If unsure, say N.
>>
>> +config MMC_AU6601
>> +       tristate "Alcor Micro AU6601"
>> +       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 0c8aa5e..8f3c64c 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -18,6 +18,7 @@ obj-$(CONFIG_MMC_SDHCI_SIRF)          += sdhci-sirf.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_OMAP)         += omap.o
>>  obj-$(CONFIG_MMC_OMAP_HS)      += omap_hsmmc.o
>>  obj-$(CONFIG_MMC_ATMELMCI)     += atmel-mci.o
>> diff --git a/drivers/mmc/host/au6601.c b/drivers/mmc/host/au6601.c
>> new file mode 100644
>> index 0000000..92d639f
>> --- /dev/null
>> +++ b/drivers/mmc/host/au6601.c
>> @@ -0,0 +1,1276 @@
>> +/*
>> + * Copyright (C) 2014 Oleksij Rempel.
>> + *
>> + * Authors: Oleksij Rempel <linux@rempel-privat.de>
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +
>> +#include <linux/delay.h>
>> +#include <linux/pci.h>
>> +#include <linux/module.h>
>> +#include <linux/io.h>
>> +#include <linux/irq.h>
>> +#include <linux/interrupt.h>
>> +
>> +#include <linux/mmc/host.h>
>> +#include <linux/mmc/mmc.h>
>> +
>> +#define DRVNAME                        "au6601-pci"
>> +#define PCI_ID_ALCOR_MICRO     0x1aea
>> +#define PCI_ID_AU6601          0x6601
>> +
>> +#define MHZ_TO_HZ(freq)        ((freq) * 1000 * 1000)
>> +
>> +#define AU6601_MIN_CLOCK               (150 * 1000)
>> +#define AU6601_MAX_CLOCK               MHZ_TO_HZ(208)
>> +#define AU6601_MAX_SEGMENTS            512
>> +#define AU6601_MAX_BLOCK_LENGTH                512
>> +#define AU6601_MAX_DMA_BLOCKS          8
>> +#define AU6601_MAX_BLOCK_COUNT         65536
>> +
>> +/* SDMA phy address. Higer then 0x0800.0000? */
>> +#define AU6601_REG_SDMA_ADDR   0x00
>> +/* ADMA block count? AU6621 only. */
>> +#define REG_05 0x05
>> +/* PIO */
>> +#define AU6601_REG_BUFFER      0x08
>> +/* ADMA ctrl? AU6621 only. */
>> +#define REG_0C 0x0c
>> +/* ADMA phy address. AU6621 only. */
>> +#define REG_10 0x10
>> +/* 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
>> +/* ??? */
>> +#define REG_52 0x52
>> +/* LED related? Always toggled BIT0 */
>> +#define REG_61 0x61
>> +/* Same as REG_61? */
>> +#define REG_63 0x63
>> +/* ??? */
>> +#define REG_69 0x69
>> +/* Block size for SDMA or PIO */
>> +#define AU6601_REG_BLOCK_SIZE  0x6c
>> +/* Some power related reg, used together with REG_7A */
>> +#define REG_70 0x70
>> +/* PLL ctrl */
>> +#define AU6601_REG_PLL_CTRL    0x72
>> +/* ??? */
>> +#define REG_74 0x74
>> +/* ??? */
>> +#define REG_75 0x75
>> +/* card slot state? */
>> +#define REG_76 0x76
>> +/* ??? */
>> +#define REG_77 0x77
>> +/* looks like soft reset? */
>> +#define AU6601_REG_SW_RESET    0x79
>> + #define AU6601_RESET_UNK      BIT(7)  /* unknown bit */
>> + #define AU6601_RESET_DATA     BIT(3)
>> + #define AU6601_RESET_CMD      BIT(0)
>> +/* see REG_70 */
>> +#define REG_7A 0x7a
>> +/* ??? Padding? Timeing? */
>> +#define REG_7B 0x7b
>> +/* ??? Padding? Timeing? */
>> +#define REG_7C 0x7c
>> +/* ??? Padding? Timeing? */
>> +#define REG_7D 0x7d
>> +/* read EEPROM? */
>> +#define REG_7F 0x7f
>> +
>> +#define AU6601_REG_CMD_CTRL    0x81
>> +#define AU6601_REG_BUS_CTRL    0x82
>> + #define AU6601_BUS_WIDTH_4BIT BIT(5)
>> +#define REG_83 0x83
>> +
>> +#define AU6601_REG_BUS_STATUS  0x84
>> + #define AU6601_BUS_STAT_CMD   BIT(15)
>> +/* BIT(4) - BIT(7) are permanently 1.
>> + * May be reseved or not attached DAT4-DAT7 */
>> + #define AU6601_BUS_STAT_DAT3          BIT(3)
>> + #define AU6601_BUS_STAT_DAT2          BIT(2)
>> + #define AU6601_BUS_STAT_DAT1          BIT(1)
>> + #define AU6601_BUS_STAT_DAT0          BIT(0)
>> + #define AU6601_BUS_STAT_DAT_MASK      0xf
>> +#define REG_85 0x85
>> +/* ??? */
>> +#define REG_86 0x86
>> +#define AU6601_REG_INT_STATUS  0x90 /* IRQ intmask */
>> +#define AU6601_REG_INT_ENABLE  0x94
>> +/* ??? */
>> +#define REG_A1 0xa1
>> +/* ??? */
>> +#define REG_A2 0xa2
>> +/* ??? */
>> +#define REG_A3 0xa3
>> +/* ??? */
>> +#define REG_B0 0xb0
>> +/* ??? */
>> +#define REG_B4 0xb4
>> +
>> + /* AU6601_REG_INT_STATUS is identical or almost identical with sdhci.h */
>> + /* OK - are tested and confirmed bits */
>> + #define  AU6601_INT_RESPONSE          0x00000001      /* ok */
>> + #define  AU6601_INT_DATA_END          0x00000002      /* fifo, ok */
>> + #define  AU6601_INT_BLK_GAP           0x00000004
>> + #define  AU6601_INT_DMA_END           0x00000008
>> + #define  AU6601_INT_SPACE_AVAIL       0x00000010      /* fifo, ok */
>> + #define  AU6601_INT_DATA_AVAIL                0x00000020      /* fifo, ok */
>> + #define  AU6601_INT_CARD_REMOVE       0x00000040
>> + #define  AU6601_INT_CARD_INSERT       0x00000080      /* 0x40 and 0x80 flip */
>> + #define  AU6601_INT_CARD_INT          0x00000100
>> + #define  AU6601_INT_ERROR             0x00008000      /* ok */
>> + #define  AU6601_INT_TIMEOUT           0x00010000      /* seems to be ok */
>> + #define  AU6601_INT_CRC               0x00020000      /* seems to be ok */
>> + #define  AU6601_INT_END_BIT           0x00040000
>> + #define  AU6601_INT_INDEX             0x00080000
>> + #define  AU6601_INT_DATA_TIMEOUT      0x00100000
>> + #define  AU6601_INT_DATA_CRC          0x00200000
>> + #define  AU6601_INT_DATA_END_BIT      0x00400000
>> + #define  AU6601_INT_BUS_POWER         0x00800000
>> + #define  AU6601_INT_ACMD12ERR         0x01000000
>> + #define  AU6601_INT_ADMA_ERROR                0x02000000
>> +
>> + #define  AU6601_INT_NORMAL_MASK       0x00007FFF
>> + #define  AU6601_INT_ERROR_MASK                0xFFFF8000
>> +
>> +/* magic 0xF0001 */
>> + #define  AU6601_INT_CMD_MASK  (AU6601_INT_RESPONSE | AU6601_INT_TIMEOUT | \
>> +               AU6601_INT_CRC | AU6601_INT_END_BIT | AU6601_INT_INDEX)
>> +/* magic 0x70003A */
>> + #define  AU6601_INT_DATA_MASK (AU6601_INT_DATA_END | AU6601_INT_DMA_END | \
>> +               AU6601_INT_DATA_AVAIL | AU6601_INT_SPACE_AVAIL | \
>> +               AU6601_INT_DATA_TIMEOUT | AU6601_INT_DATA_CRC | \
>> +               AU6601_INT_DATA_END_BIT)
>> + #define AU6601_INT_ALL_MASK   ((uint32_t)-1)
>> +
>> +bool disable_dma = 0;
> 
> Could this not be apart of the struct au6601_host? Or you might even
> be able to use "dma_on", which is already there?

I'll check it.

>> +
>> +struct au6601_host {
>> +       struct pci_dev *pdev;
>> +       struct  device *dev;
>> +       void __iomem *iobase;
>> +       void __iomem *virt_base;
>> +       dma_addr_t phys_base;
>> +
>> +       struct mmc_host *mmc;
>> +       struct mmc_request *mrq;
>> +       struct mmc_command *cmd;
>> +       struct mmc_data *data;
>> +       unsigned int data_early:1;      /* Data finished before cmd */
>> +       unsigned int dma_on:1;
>> +       unsigned int trigger_dma_dac:1; /* Trigger Data after Command.
>> +                                        * In some cases data ragister
>> +                                        * should be triggered after
>> +                                        * command was done */
>> +
>> +       spinlock_t lock;
>> +
>> +       struct tasklet_struct card_tasklet;
>> +       struct tasklet_struct finish_tasklet;
> 
> Please try using threaded irqs in favor of tasklets.

Right now i have some problems with this part.
I will need to rewrite DMA too.

>> +
>> +       struct timer_list timer;
>> +
>> +       struct sg_mapping_iter sg_miter;        /* SG state for PIO */
>> +       unsigned int blocks;            /* remaining PIO blocks */
>> +       unsigned int requested_blocks;          /* count of requested */
>> +       int sg_count;      /* Mapped sg entries */
>> +};
>> +
>> +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 const struct pci_device_id pci_ids[] = {

======== snip ==========================

>> +/* val = 0x1 abort command; 0x8 abort data? */
>> +static void au6601_reset(struct au6601_host *host, u8 val)
>> +{
>> +       int i;
>> +       iowrite8(val | AU6601_RESET_UNK, host->iobase + AU6601_REG_SW_RESET);
>> +       for (i = 0; i < 100; i++) {
>> +               if (!(ioread8(host->iobase + AU6601_REG_SW_RESET) & val))
>> +                       return;
>> +               udelay(50);
>> +       }
>> +       dev_err(host->dev, "%s: timeout\n", __func__);
>> +}
>> +
>> +/*
>> + * - 0x8       only Vcc is on
>> + * - 0x1       Vcc and other pins are on
>> + * - 0x1 | 0x8 like 0x1, but DAT2 is off
>> + */
>> +static void au6601_set_power(struct au6601_host *host,
>> +                            unsigned int value, unsigned int set)
>> +{
>> +       u8 tmp1, tmp2;
>> +
>> +       tmp1 = ioread8(host->iobase + REG_70);
>> +       tmp2 = ioread8(host->iobase + REG_7A);
>> +       if (set) {
>> +               iowrite8(tmp1 | value, host->iobase + REG_70);
>> +               msleep(20);
>> +               iowrite8(tmp2 | value, host->iobase + REG_7A);
>> +       } else {
>> +               iowrite8(tmp2 & ~value, host->iobase + REG_7A);
>> +               iowrite8(tmp1 & ~value, host->iobase + REG_70);
>> +       }
>> +}
>> +
>> +static void au6601_trigger_data_transfer(struct au6601_host *host,
>> +               unsigned int dma)
>> +{
>> +       struct mmc_data *data = host->data;
>> +       u8 ctrl = 0;
>> +
>> +       BUG_ON(data == NULL);
> 
> Do you really need BUG_ON?
> 
> Also, please go through the complete patch to look for these
> BUG/BUG_ON, I think you shouldn't use them in most of the cases.
> 
>> +       WARN_ON_ONCE(host->dma_on == 1);
> 
> Why?

Good question, mostly because i was not knowing what i am doing and made
some buggy assumptions. Will fix it.

> 
>> +
>> +       if (data->flags & MMC_DATA_WRITE)
>> +               ctrl |= 0x80;
>> +
>> +       if (dma) {
>> +               iowrite32(host->phys_base, host->iobase + AU6601_REG_SDMA_ADDR);
>> +               ctrl |= 0x40;
>> +               host->dma_on = 1;
>> +
>> +               if (data->flags & MMC_DATA_WRITE)
>> +                       goto done;
>> +               /* prepare first DMA buffer for write operation */
>> +               if (host->blocks > AU6601_MAX_DMA_BLOCKS)
>> +                       host->requested_blocks = AU6601_MAX_DMA_BLOCKS;
>> +               else
>> +                       host->requested_blocks = host->blocks;
>> +
>> +       }
>> +
>> +done:
>> +       iowrite32(data->blksz * host->requested_blocks,
>> +               host->iobase + AU6601_REG_BLOCK_SIZE);
>> +       iowrite8(ctrl | 0x1, host->iobase + REG_83);
>> +}
>> +
>> +/*****************************************************************************\
>> + *                                                                          *
>> + * Core functions                                                           *
>> + *                                                                          *
>> +\*****************************************************************************/

===================

>> +static void au6601_tasklet_finish(unsigned long param)
>> +{
>> +       struct au6601_host *host;
>> +       unsigned long flags;
>> +       struct mmc_request *mrq;
>> +
>> +       host = (struct au6601_host *)param;
>> +
>> +       spin_lock_irqsave(&host->lock, flags);
>> +
>> +       /*
>> +        * If this tasklet gets rescheduled while running, it will
>> +        * be run again afterwards but without any active request.
>> +        */
>> +       if (!host->mrq) {
>> +               spin_unlock_irqrestore(&host->lock, flags);
>> +               return;
>> +       }
>> +
>> +       del_timer(&host->timer);
>> +
>> +       mrq = host->mrq;
>> +
>> +       /*
>> +        * The controller needs a reset of internal state machines
>> +        * upon error conditions.
>> +        */
>> +       if ((mrq->cmd && mrq->cmd->error) ||
>> +                (mrq->data && (mrq->data->error ||
>> +                 (mrq->data->stop && mrq->data->stop->error)))) {
>> +
>> +               au6601_reset(host, AU6601_RESET_CMD);
>> +               au6601_reset(host, AU6601_RESET_DATA);
>> +       }
>> +
>> +       host->mrq = NULL;
>> +       host->cmd = NULL;
>> +       host->data = NULL;
>> +       host->dma_on = 0;
>> +       host->trigger_dma_dac = 0;
>> +
>> +       spin_unlock_irqrestore(&host->lock, flags);
>> +
>> +       mmc_request_done(host->mmc, mrq);
>> +}
>> +
>> +static void au6601_timeout_timer(unsigned long data)
> 
> I am just a bit curious, does this controller support hardware busy
> detection on DAT1 line while waiting for command completion?

Do you mean AU6601_REG_BUS_STATUS?
See at the beginning of patch.

>> +{
>> +       struct au6601_host *host;
>> +       unsigned long flags;
>> +
>> +       host = (struct au6601_host *)data;
>> +
>> +       spin_lock_irqsave(&host->lock, flags);
>> +
>> +       if (host->mrq) {
>> +               dev_err(host->dev,
>> +                       "Timeout waiting for hardware interrupt.\n");
>> +
>> +               if (host->data) {
>> +                       host->data->error = -ETIMEDOUT;
>> +                       au6601_finish_data(host);
>> +               } else {

>> +
>> +static int au6601_resume(struct pci_dev *pdev)
>> +{
>> +       struct au6601_host *host;
>> +       int ret;
>> +
>> +       host = pci_get_drvdata(pdev);
>> +
>> +       pci_set_power_state(pdev, PCI_D0);
>> +       pci_restore_state(pdev);
>> +       ret = pci_enable_device(pdev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       au6601_hw_init(host);
>> +       return 0;
>> +}
>> +
>> +
>> +#else /* CONFIG_PM */
>> +
>> +#define au6601_suspend NULL
>> +#define au6601_resume NULL
>> +
>> +#endif /* CONFIG_PM */
> 
> Instead of the above tricks, can't you use the dev_pm_ops instead of
> the PM callbacks in the driver? And the use the "SIMPLE_DEV_PM_OPS"
> macro?

ok, on it.

>> +
>> +static struct pci_driver au6601_driver = {
>> +       .name =  DRVNAME,
>> +       .id_table =     pci_ids,
>> +       .probe =        au6601_pci_probe,
>> +       .remove =       au6601_pci_remove,
>> +       .suspend = au6601_suspend,
>> +       .resume = au6601_resume,
>> +};
>> +
>> +module_pci_driver(au6601_driver);

-- 
Regards,
Oleksij


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 278 bytes --]