Oleksij,
First, again, apologize for the delay in giving feedback.
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?
>
> 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
> + */
> +
> +
> +#include <linux/delay.h>
> +#include <linux/pci.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/pm.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 PCI_ID_AU6621 0x6621
> +
> +#define MHZ_TO_HZ(freq) ((freq) * 1000 * 1000)
> +
> +#define AU6601_BASE_CLOCK MHZ_TO_HZ(31)
> +#define AU6601_MIN_CLOCK (150 * 1000)
> +#define AU6601_MAX_CLOCK MHZ_TO_HZ(208)
> +#define AU6601_MAX_DMA_SEGMENTS (8 * 120)
> +#define AU6601_MAX_PIO_SEGMENTS 1
> +#define AU6601_MAX_DMA_BLOCK_SIZE 0x1000
> +#define AU6601_MAX_PIO_BLOCK_SIZE 0x200
> +#define AU6601_MAX_DMA_BLOCKS 1
> +#define AU6601_DMA_LOCAL_SEGMENTS 1
> +
> +/* SDMA phy address. Higer then 0x0800.0000?
> + * The au6601 and au6621 have different DMA engines with different issues. One
> + * For example au6621 engine is triggered by addr change. No other interaction
> + * is needed. This means, if we get two buffers with same address, then engine
> + * will stall.
> + */
> +#define AU6601_REG_SDMA_ADDR 0x00
> +#define AU6601_SDMA_MASK 0xffffffff
> +
> +#define AU6601_DMA_BOUNDARY 0x05
> +#define AU6621_DMA_PAGE_CNT 0x05
> +/* PIO */
> +#define AU6601_REG_BUFFER 0x08
> +/* ADMA ctrl? AU6621 only. */
> +#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.
> +/* 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
> +/* Same as REG_61? */
> +#define REG_63 0x63
> +/* default timeout set to 125: 125 * 40ms = 5 sec
> + * how exactly it is calculated? */
> +#define AU6601_TIME_OUT_CTRL 0x69
> +/* Block size for SDMA or PIO */
> +#define AU6601_REG_BLOCK_SIZE 0x6c
> +/* Some power related reg, used together with AU6601_OUTPUT_ENABLE */
> +#define AU6601_POWER_CONTROL 0x70
> +
> +
> +/* PLL ctrl */
> +#define AU6601_CLK_SELECT 0x72
> +#define AU6601_CLK_OVER_CLK 0x80
> +#define AU6601_CLK_384_MHZ 0x30
> +#define AU6601_CLK_125_MHZ 0x20
> +#define AU6601_CLK_48_MHZ 0x10
> +#define AU6601_CLK_EXT_PLL 0x04
> +#define AU6601_CLK_X2_MODE 0x02
> +#define AU6601_CLK_ENABLE 0x01
> +#define AU6601_CLK_31_25_MHZ 0x00
> +
> +#define AU6601_CLK_DIVIDER 0x73
> +
> +#define AU6601_INTERFACE_MODE_CTRL 0x74
> +#define AU6601_DLINK_MODE 0x80
> +#define AU6601_INTERRUPT_DELAY_TIME 0x40
> +#define AU6601_SIGNAL_REQ_CTRL 0x30
> +#define AU6601_MS_CARD_WP BIT(3)
> +#define AU6601_SD_CARD_WP BIT(0)
> +
> +/* ???
> + * same register values are used for:
> + * - AU6601_OUTPUT_ENABLE
> + * - AU6601_POWER_CONTROL
> + */
> +#define AU6601_ACTIVE_CTRL 0x75
> +#define AU6601_XD_CARD BIT(4)
> +/* AU6601_MS_CARD_ACTIVE - will cativate MS card section? */
> +#define AU6601_MS_CARD BIT(3)
> +#define AU6601_SD_CARD BIT(0)
> +
> +/* card slot state. It should automatically detect type of
> + * the card
> + */
> +#define AU6601_DETECT_STATUS 0x76
> +#define AU6601_DETECT_EN BIT(7)
> +#define AU6601_MS_DETECTED BIT(3)
> +#define AU6601_SD_DETECTED BIT(0)
> +#define AU6601_DETECT_STATUS_M 0xf
> +/* ??? */
> +#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.
> +#define AU6601_BUF_CTRL_RESET BIT(7)
> +#define AU6601_RESET_DATA BIT(3)
> +#define AU6601_RESET_CMD BIT(0)
> +
> +#define AU6601_OUTPUT_ENABLE 0x7a
> +
> +#define AU6601_PAD_DRIVE0 0x7b
> +#define AU6601_PAD_DRIVE1 0x7c
> +#define AU6601_PAD_DRIVE2 0x7d
> +/* read EEPROM? */
> +#define AU6601_FUNCTION 0x7f
> +
> +#define AU6601_CMD_XFER_CTRL 0x81
> +#define AU6601_CMD_17_BYTE_CRC 0xc0
> +#define AU6601_CMD_6_BYTE_WO_CRC 0x80
> +#define AU6601_CMD_6_BYTE_CRC 0x40
> +#define AU6601_CMD_START_XFER 0x20
> +#define AU6601_CMD_STOP_WAIT_RDY 0x10
> +#define AU6601_CMD_NO_RESP 0x00
> +
> +#define AU6601_REG_BUS_CTRL 0x82
> +#define AU6601_BUS_WIDTH_4BIT 0x20
> +#define AU6601_BUS_WIDTH_8BIT 0x10
> +#define AU6601_BUS_WIDTH_1BIT 0x00
> +
> +#define AU6601_DATA_XFER_CTRL 0x83
> +#define AU6601_DATA_WRITE BIT(7)
> +#define AU6601_DATA_DMA_MODE BIT(6)
> +#define AU6601_DATA_START_XFER BIT(0)
> +
> +#define AU6601_DATA_PIN_STATE 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 AU6601_OPT 0x85
> +#define AU6601_OPT_CMD_LINE_LEVEL 0x80
> +#define AU6601_OPT_NCRC_16_CLK BIT(4)
> +#define AU6601_OPT_CMD_NWT BIT(3)
> +#define AU6601_OPT_STOP_CLK BIT(2)
> +#define AU6601_OPT_DDR_MODE BIT(1)
> +#define AU6601_OPT_SD_18V BIT(0)
> +
> +#define AU6601_CLK_DELAY 0x86
> +#define AU6601_CLK_DATA_POSITIVE_EDGE 0x80
> +#define AU6601_CLK_CMD_POSITIVE_EDGE 0x40
> +#define AU6601_CLK_POSITIVE_EDGE_ALL \
> + AU6601_CLK_CMD_POSITIVE_EDGE | AU6601_CLK_DATA_POSITIVE_EDGE
> +
> +
> +#define AU6601_REG_INT_STATUS 0x90
> +#define AU6601_REG_INT_ENABLE 0x94
> +#define AU6601_INT_DATA_END_BIT_ERR BIT(22)
> +#define AU6601_INT_DATA_CRC_ERR BIT(21)
> +#define AU6601_INT_DATA_TIMEOUT_ERR BIT(20)
> +#define AU6601_INT_CMD_INDEX_ERR BIT(19)
> +#define AU6601_INT_CMD_END_BIT_ERR BIT(18)
> +#define AU6601_INT_CMD_CRC_ERR BIT(17)
> +#define AU6601_INT_CMD_TIMEOUT_ERR BIT(16)
> +#define AU6601_INT_ERROR BIT(15)
> +#define AU6601_INT_OVER_CURRENT_ERR BIT(8)
> +#define AU6601_INT_CARD_INSERT BIT(7)
> +#define AU6601_INT_CARD_REMOVE BIT(6)
> +#define AU6601_INT_READ_BUF_RDY BIT(5)
> +#define AU6601_INT_WRITE_BUF_RDY BIT(4)
> +#define AU6601_INT_DMA_END BIT(3)
> +#define AU6601_INT_DATA_END BIT(1)
> +#define AU6601_INT_CMD_END BIT(0)
> +
> +#define AU6601_INT_NORMAL_MASK 0x00007FFF
> +#define AU6601_INT_ERROR_MASK 0xFFFF8000
> +
> +#define AU6601_INT_CMD_MASK (AU6601_INT_CMD_END | \
> + AU6601_INT_CMD_TIMEOUT_ERR | AU6601_INT_CMD_CRC_ERR | \
> + AU6601_INT_CMD_END_BIT_ERR | AU6601_INT_CMD_INDEX_ERR)
> +#define AU6601_INT_DATA_MASK (AU6601_INT_DATA_END | AU6601_INT_DMA_END | \
> + AU6601_INT_READ_BUF_RDY | AU6601_INT_WRITE_BUF_RDY | \
> + AU6601_INT_DATA_TIMEOUT_ERR | AU6601_INT_DATA_CRC_ERR | \
> + AU6601_INT_DATA_END_BIT_ERR)
> +#define AU6601_INT_ALL_MASK ((u32)-1)
> +
> +/* MS_CARD mode registers */
Hmm, is this both a SD card and memstick capable PCI controller?
Is there plans to implement the memstick parts as well?
More comments about this later.
> +
> +#define AU6601_MS_STATUS 0xa0
> +
> +#define AU6601_MS_BUS_MODE_CTRL 0xa1
> +#define AU6601_MS_BUS_8BIT_MODE 0x03
> +#define AU6601_MS_BUS_4BIT_MODE 0x01
> +#define AU6601_MS_BUS_1BIT_MODE 0x00
> +
> +#define AU6601_MS_TPC_CMD 0xa2
> +#define AU6601_MS_TPC_READ_PAGE_DATA 0x02
> +#define AU6601_MS_TPC_READ_REG 0x04
> +#define AU6601_MS_TPC_GET_INT 0x07
> +#define AU6601_MS_TPC_WRITE_PAGE_DATA 0x0D
> +#define AU6601_MS_TPC_WRITE_REG 0x0B
> +#define AU6601_MS_TPC_SET_RW_REG_ADRS 0x08
> +#define AU6601_MS_TPC_SET_CMD 0x0E
> +#define AU6601_MS_TPC_EX_SET_CMD 0x09
> +#define AU6601_MS_TPC_READ_SHORT_DATA 0x03
> +#define AU6601_MS_TPC_WRITE_SHORT_DATA 0x0C
> +
> +#define AU6601_MS_TRANSFER_MODE 0xa3
> +#define AU6601_MS_XFER_INT_TIMEOUT_CHK BIT(2)
> +#define AU6601_MS_XFER_DMA_ENABLE BIT(1)
> +#define AU6601_MS_XFER_START BIT(0)
> +
> +#define AU6601_MS_DATA_PIN_STATE 0xa4
> +
> +#define AU6601_MS_INT_STATUS 0xb0
> +#define AU6601_MS_INT_ENABLE 0xb4
> +#define AU6601_MS_INT_OVER_CURRENT_ERROR BIT(23)
> +#define AU6601_MS_INT_DATA_CRC_ERROR BIT(21)
> +#define AU6601_MS_INT_INT_TIMEOUT BIT(20)
> +#define AU6601_MS_INT_INT_RESP_ERROR BIT(19)
> +#define AU6601_MS_INT_CED_ERROR BIT(18)
> +#define AU6601_MS_INT_TPC_TIMEOUT BIT(16)
> +#define AU6601_MS_INT_ERROR BIT(15)
> +#define AU6601_MS_INT_CARD_INSERT BIT(7)
> +#define AU6601_MS_INT_CARD_REMOVE BIT(6)
> +#define AU6601_MS_INT_BUF_READ_RDY BIT(5)
> +#define AU6601_MS_INT_BUF_WRITE_RDY BIT(4)
> +#define AU6601_MS_INT_DMA_END BIT(3)
> +#define AU6601_MS_INT_TPC_END BIT(1)
> +
> +#define AU6601_MS_INT_DATA_MASK 0x00000038
> +#define AU6601_MS_INT_TPC_MASK 0x003d8002
> +#define AU6601_MS_INT_TPC_ERROR 0x003d0000
> +
> +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?
Could it be useful to instead deploy a policy of always trying DMA and
if it fails, fallback to use PIO?
> +
> +enum au6601_cookie {
> + COOKIE_UNMAPPED,
> + COOKIE_PRE_MAPPED, /* mapped by pre_req() of dwmmc */
> + COOKIE_MAPPED, /* mapped by prepare_data() of dwmmc */
> +};
> +
> +struct au6601_dev_cfg {
> + u32 flags;
> + u8 dma;
> +};
> +
> +struct au6601_pll_conf {
> + unsigned int clk_src_freq;
> + unsigned int clk_src_reg;
> + unsigned int min_div;
> + unsigned int max_div;
> +};
> +
> +struct au6601_host {
> + struct pci_dev *pdev;
> + struct pci_dev *parent_pdev;
> + struct device *dev;
> + void __iomem *iobase;
> + void __iomem *dma_trap_virt;
Looks unused.
> + dma_addr_t dma_trap_phys;
Looks unused.
> +
> + 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.
> +};
> +
> +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.
> +
> +static const struct au6601_dev_cfg au6601_cfg = {
> + .dma = 0,
> +};
> +
> +static const struct au6601_dev_cfg au6621_cfg = {
> + .dma = 1,
> +};
> +
> +static const struct pci_device_id pci_ids[] = {
> + { PCI_DEVICE(PCI_ID_ALCOR_MICRO, PCI_ID_AU6601),
> + .driver_data = (kernel_ulong_t)&au6601_cfg },
> + { PCI_DEVICE(PCI_ID_ALCOR_MICRO, PCI_ID_AU6621),
> + .driver_data = (kernel_ulong_t)&au6621_cfg },
> + { },
> +};
> +MODULE_DEVICE_TABLE(pci, pci_ids);
> +
> +static void au6601_reg_decode(int write, int size, u32 val,
> + unsigned int addr_short)
> +{
> + const char *reg;
> +
> + switch (addr_short)
> + {
> + case 0x00: reg = "SDMA_ADDR"; break;
> + case 0x05: reg = "DMA_BOUNDARY"; break;
> + case 0x08: reg = "PIO_BUFFER"; break;
> + case 0x0c: reg = "DMA_CTRL"; break;
> + case 0x23: reg = "CMD_OPCODE"; break;
> + case 0x24: reg = "CMD_ARG"; break;
> + case 0x30: reg = "CMD_RSP0"; break;
> + case 0x34: reg = "CMD_RSP1"; break;
> + case 0x38: reg = "CMD_RSP2"; break;
> + case 0x3C: reg = "CMD_RSP3"; break;
> + case 0x69: reg = "TIME_OUT_CTRL"; break;
> + case 0x6c: reg = "BLOCK_SIZE"; break;
> + case 0x70: reg = "POWER_CONTROL"; break;
> + case 0x72: reg = "CLK_SELECT"; break;
> + case 0x73: reg = "CLK_DIVIDER"; break;
> + case 0x74: reg = "INTERFACE_MODE_CTRL"; break;
> + case 0x75: reg = "ACTIVE_CTRL"; break;
> + case 0x76: reg = "DETECT_STATUS"; break;
> + case 0x79: reg = "SW_RESE"; break;
> + case 0x7a: reg = "OUTPUT_ENABLE"; break;
> + case 0x7b: reg = "PAD_DRIVE0"; break;
> + case 0x7c: reg = "PAD_DRIVE1"; break;
> + case 0x7d: reg = "PAD_DRIVE2"; break;
> + case 0x7f: reg = "EEPROM"; break;
> + case 0x81: reg = "CMD_XFER_CTRL"; break;
> + case 0x82: reg = "BUS_CTRL"; break;
> + case 0x83: reg = "DATA_XFER_CTRL"; break;
> + case 0x84: reg = "DATA_PIN_STATE"; break;
> + case 0x85: reg = "OPT"; break;
> + case 0x86: reg = "CLK_DELAY"; break;
> + case 0x90: reg = "INT_STATUS"; break;
> + case 0x94: reg = "INT_ENABLE"; break;
> + case 0xa0: reg = "MS_STATUS"; break;
> + default: reg = "unkn"; break;
> + }
> +
> + 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?
> +}
> +
> +static void au6601_write8(struct au6601_host *host, u8 val,
> + unsigned int addr)
> +{
> + au6601_reg_decode(1, 1, val, addr);
> + writeb(val, host->iobase + addr);
> +}
> +
> +static void au6601_write16(struct au6601_host *host, u16 val,
> + unsigned int addr)
> +{
> + au6601_reg_decode(1, 2, val, addr);
> + writew(val, host->iobase + addr);
> +}
> +
> +static void au6601_write32(struct au6601_host *host, u32 val,
> + unsigned int addr)
> +{
> + au6601_reg_decode(1, 4, val, addr);
> + writel(val, host->iobase + addr);
> +}
> +
> +static u8 au6601_read8(struct au6601_host *host,
> + unsigned int addr)
> +{
> + u8 val;
> + val = readb(host->iobase + addr);
> + au6601_reg_decode(0, 1, val, addr);
> + return val;
> +}
> +
> +static u32 au6601_read32(struct au6601_host *host,
> + unsigned int addr)
> +{
> + u32 val;
> + val = readl(host->iobase + addr);
> + au6601_reg_decode(0, 4, val, addr);
> + return val;
> +}
> +
> +static u32 au6601_read32be(struct au6601_host *host,
> + unsigned int addr)
> +{
> + u32 val;
> + val = ioread32be(host->iobase + addr);
> + au6601_reg_decode(0, 4, val, addr);
> + return val;
> +}
> +
> +static void au6601_write32be(struct au6601_host *host,
> + u32 val, unsigned int addr)
> +{
> + au6601_reg_decode(1, 4, val, addr);
> + iowrite32be(val, host->iobase + addr);
> +}
> +
> +static inline void au6601_rmw8(struct au6601_host *host, unsigned int addr,
> + u8 clear, u8 set)
> +{
> + u32 var;
> +
> + var = au6601_read8(host, addr);
> + var &= ~clear;
> + var |= set;
> + au6601_write8(host, var, 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.
> +
> + 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.
> +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");
> +
> + host->pdev_cap_off = pci_find_cap_offset(host, host->pdev);
> + host->parent_cap_off = pci_find_cap_offset(host, host->parent_pdev);
> +
> + if ((host->pdev_cap_off == 0) || (host->parent_cap_off == 0)) {
> + dev_dbg(host->dev, "pci_cap_off: %x, parent_cap_off: %x\n",
> + host->pdev_cap_off, host->parent_cap_off);
> + return 0;
> + }
> +
> + /* link capability */
> + pci = host->pdev;
> + where = host->pdev_cap_off + PCIE_LINK_CAP_OFFSET;
> + pci_read_config_dword(pci, where, &val32);
> + host->pdev_aspm_cap = (u8)(val32 >> 10) & 0x03;
> +
> + pci = host->parent_pdev;
> + where = host->parent_cap_off + PCIE_LINK_CAP_OFFSET;
> + pci_read_config_dword(pci, where, &val32);
> + host->parent_aspm_cap = (u8)(val32 >> 10) & 0x03;
> +
> + if (host->pdev_aspm_cap != host->parent_aspm_cap) {
> + u8 aspm_cap;
> + dev_dbg(host->dev, "host->pdev_aspm_cap: %x\n",
> + host->pdev_aspm_cap);
> + dev_dbg(host->dev, "host->parent_aspm_cap: %x\n",
> + host->parent_aspm_cap);
> + aspm_cap = host->pdev_aspm_cap & host->parent_aspm_cap;
> + host->pdev_aspm_cap = aspm_cap;
> + host->parent_aspm_cap = aspm_cap;
> + }
> +
> + dev_dbg(host->dev, "ext_config_dev_aspm: %x, host->pdev_aspm_cap: %x\n",
> + host->ext_config_dev_aspm, host->pdev_aspm_cap);
> + host->ext_config_dev_aspm &= host->pdev_aspm_cap;
> + return 1;
> +}
> +
[...]
> +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?
[...]
> +
> +/*****************************************************************************\
> + * *
> + * Core functions *
To me, these kind of section markers are a bit silly, please just drop
them for the file.
> + * *
> +\*****************************************************************************/
[...]
> +
> +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.
> +
> + 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.
> +
> + host->cmd = cmd;
> + au6601_prepare_data(host, cmd);
> +
> + dev_dbg(host->dev, "send CMD. opcode: 0x%02x, arg; 0x%08x\n", cmd->opcode,
> + cmd->arg);
> + au6601_write8(host, cmd->opcode | 0x40, AU6601_REG_CMD_OPCODE);
> + au6601_write32be(host, cmd->arg, AU6601_REG_CMD_ARG);
> +
> + switch (mmc_resp_type(cmd)) {
> + case MMC_RSP_NONE:
> + ctrl = AU6601_CMD_NO_RESP;
> + break;
> + case MMC_RSP_R1:
> + ctrl = AU6601_CMD_6_BYTE_CRC;
> + break;
> + case MMC_RSP_R1B:
> + ctrl = AU6601_CMD_6_BYTE_CRC | AU6601_CMD_STOP_WAIT_RDY;
> + break;
> + case MMC_RSP_R2:
> + ctrl = AU6601_CMD_17_BYTE_CRC;
> + break;
> + case MMC_RSP_R3:
> + ctrl = AU6601_CMD_6_BYTE_WO_CRC;
> + break;
> + default:
> + dev_err(host->dev, "%s: cmd->flag (0x%02x) is not valid\n",
> + mmc_hostname(host->mmc), mmc_resp_type(cmd));
> + break;
> + }
> +
> + dev_dbg(host->dev, "xfer ctrl: 0x%02x; timeout: %lu\n", ctrl, timeout);
> + au6601_write8(host, ctrl | AU6601_CMD_START_XFER,
> + AU6601_CMD_XFER_CTRL);
> +
> + 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.
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.
> + }
> +
> + 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.
Does, that means some of the code above is untested? If so, at least
that should be documented by some comments in the code.
> + break;
> + default:
> + /* No signal voltage switch required */
> + break;
> + }
> +
> + mutex_unlock(&host->cmd_mutex);
> + return 0;
> +}
> +
> +static const struct mmc_host_ops au6601_sdc_ops = {
> + .card_busy = au6601_card_busy,
> + .get_cd = au6601_get_cd,
> + .get_ro = au6601_get_ro,
> + .post_req = au6601_post_req,
> + .pre_req = au6601_pre_req,
> + .request = au6601_request,
> + .set_ios = au6601_set_ios,
> + .start_signal_voltage_switch = au6601_signal_voltage_switch,
> +};
> +
> +static void au6601_request_complete(struct au6601_host *host,
> + bool cancel_timeout)
> +{
> + 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.
> + if (!host->mrq) {
> + dev_dbg(host->dev, "nothing to complete\n");
> + return;
> + }
> +
> + if (cancel_timeout)
> + cancel_delayed_work_sync(&host->timeout_work);
> +
> + mrq = host->mrq;
> +
> + host->mrq = NULL;
> + host->cmd = NULL;
> + host->data = NULL;
> + host->dma_on = 0;
> +
> + dev_dbg(host->dev, "request complete\n");
> + mmc_request_done(host->mmc, mrq);
> +}
> +
> +static void au6601_timeout_timer(struct work_struct *work)
> +{
> + struct delayed_work *d = to_delayed_work(work);
> + struct au6601_host *host = container_of(d, struct au6601_host,
> + timeout_work);
> + mutex_lock(&host->cmd_mutex);
> +
> + dev_dbg(host->dev, "triggered timeout\n");
> + if (host->mrq) {
> + dev_err(host->dev,
> + "Timeout waiting for hardware interrupt.\n");
> +
> + if (host->data) {
> + host->data->error = -ETIMEDOUT;
> + } else {
> + if (host->cmd)
> + host->cmd->error = -ETIMEDOUT;
> + else
> + host->mrq->cmd->error = -ETIMEDOUT;
> + }
> +
> + au6601_reset(host, AU6601_RESET_CMD | AU6601_RESET_DATA);
> + au6601_request_complete(host, 0);
> + }
> +
> + mmiowb();
> + mutex_unlock(&host->cmd_mutex);
> +}
> +
> +
> +
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.
Just making sure the above is correct?
BTW, how do you control the different voltage levels? I guess that is
internally by the PCI controller HW, no?
> + 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?
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.
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?
> + au6601_mask_ms_irqs(host);
> +}
> +
[...]
Kind regards
Uffe
