On Mon, Jul 29, 2013 at 11:49:34AM +0100, Jonas Jensen wrote:

This binding looks sensible, but I'd appreciate someone who understands
MMCs checking that this captures the relevant information, as I don't
know much about MMCs.

Most mmc bindings I see have to describe some gpio pins, pinctrl to mux
the relevant pins on from the SoC, etc. Is this a full description of
the hardware supporting all features?

[...]

+static int moxart_get_ro(struct mmc_host *mmc)
+{
+       int ret;
+       struct moxart_host *host = mmc_priv(mmc);
+
+       (readl(&host->reg->status) & MSD_WRITE_PROT) ? (ret = 1) : (ret = 0);
+       return ret;

This looks a little odd, how about something like:

return !!(readl(&host->reg->status) & SD_WRITE_PROT);

+}
+
+static struct mmc_host_ops moxart_ops = {
+       .request = moxart_request,
+       .set_ios = moxart_set_ios,
+       .get_ro = moxart_get_ro,
+};
+
+static int moxart_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct device_node *node = dev->of_node;
+       struct resource res_mmc;
+       struct mmc_host *mmc;
+       struct moxart_host *host = NULL;
+       void __iomem *reg_mmc;
+       dma_cap_mask_t mask;
+       int ret;
+       struct dma_slave_config cfg;
+       unsigned int irq;
+       struct clk *clk;
+       unsigned int dma_chan_rx_req = 1;
+       unsigned int dma_chan_tx_req = 0;
+
+       mmc = mmc_alloc_host(sizeof(struct moxart_host), dev);
+       if (!mmc) {
+               dev_err(dev, "%s: mmc_alloc_host failed\n", __func__);
+               ret = -ENOMEM;
+               goto out;
+       }
+
+       ret = of_address_to_resource(node, 0, &res_mmc);
+       if (ret) {
+               dev_err(dev, "%s: could not get MMC base resource\n", __func__);
+               goto out;
+       }
+
+       irq = irq_of_parse_and_map(node, 0);
+
+       reg_mmc = devm_ioremap_resource(dev, &res_mmc);
+       if (IS_ERR(reg_mmc)) {
+               dev_err(dev, "%s: devm_ioremap_resource res_mmc failed\n",
+                       __func__);
+               return PTR_ERR(reg_mmc);
+       }
+
+       mmc->ops = &moxart_ops;
+
+       /*
+        * hardware does not support MMC_CAP_SD_HIGHSPEED
+        * CMD6 will timeout and make things not work
+        */
+       mmc->caps = MMC_CAP_4_BIT_DATA;
+
+       mmc->f_min = 400000;
+       mmc->f_max = 25000000;
+       mmc->ocr_avail = 0xffff00;      /* support 2.0v - 3.6v power */
+       mmc->max_segs = 32;
+       mmc->max_blk_size = 512;
+       mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size;
+       mmc->max_seg_size = mmc->max_req_size;
+
+       host = mmc_priv(mmc);
+       host->mmc = mmc;
+       host->reg = (struct moxart_reg *)reg_mmc;
+       host->reg_phys = res_mmc.start;
+       host->timeout = msecs_to_jiffies(1000);
+
+       dma_cap_zero(mask);
+       dma_cap_set(DMA_SLAVE, mask);
+
+       clk = of_clk_get(node, 0);
+       if (IS_ERR(clk)) {
+               dev_err(dev, "%s: of_clk_get failed\n", __func__);
+               return PTR_ERR(clk);
+       }
+       host->sysclk = clk_get_rate(clk);
+
+       spin_lock_init(&host->lock);
+
+       /* disable all interrupt */
+       writel(0, &host->reg->interrupt_mask);
+
+       /* reset chip */
+       writel(MSD_SDC_RST, &host->reg->command);
+
+       /* wait for reset finished */
+       while (readl(&host->reg->command) & MSD_SDC_RST)
+               udelay(10);
+
+       host->dma_chan_tx = dma_request_channel(mask, moxart_filter_fn,
+                                               (void *)&dma_chan_tx_req);
+       host->dma_chan_rx = dma_request_channel(mask, moxart_filter_fn,
+                                               (void *)&dma_chan_rx_req);
+       dev_dbg(dev, "%s: using 2 DMA channels rx=%p tx=%p\n",
+               __func__, host->dma_chan_rx, host->dma_chan_tx);
+
+       if (!host->dma_chan_rx || !host->dma_chan_tx) {
+               host->have_dma = false;
+               mmc->max_blk_count = 1;
+       } else {
+               cfg.slave_id = APB_DMA_SD_REQ_NO;
+               cfg.direction = DMA_MEM_TO_DEV;
+               cfg.src_addr = 0;
+               cfg.dst_addr = (unsigned int)host->reg_phys + MSD_DATA_WIN_REG;
+               cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+               dmaengine_slave_config(host->dma_chan_tx, &cfg);
+
+               cfg.slave_id = APB_DMA_SD_REQ_NO;
+               cfg.direction = DMA_DEV_TO_MEM;
+               cfg.src_addr = (unsigned int)host->reg_phys + MSD_DATA_WIN_REG;
+               cfg.dst_addr = 0;
+               cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+               dmaengine_slave_config(host->dma_chan_rx, &cfg);
+
+               host->have_dma = true;
+
+               /*
+                * there seems to be a max size on transfers so
+                * set max_blk_count low for both DMA and PIO
+                *
+                * sending large chunks result either in timeout
+                * or render the MMC controller unresponsive
+                * (status register 0 on consecutive read retries,
+                * also see comments in moxart_send_command)
+                *
+                * obviously, DMA is quicker and can handle
+                * larger chunks but setting it higher than 16
+                * can still bug the controller
+                */
+               mmc->max_blk_count = 16;
+       }
+
+       devm_request_irq(dev, irq, moxart_irq, 0, "moxart-mmc", host);
+
+       if (ret)
+               goto out;

Presumably you meant to record the return code of devm_request_irq?

+
+       dev_set_drvdata(dev, mmc);
+       mmc_add_host(mmc);
+
+       dev_dbg(dev, "%s: IRQ=%d\n", __func__, irq);
+
+       return 0;
+
+out:
+       if (mmc)
+               mmc_free_host(mmc);
+       return ret;
+}

[...]

+struct moxart_reg {
+
+#define MSD_SDC_RST            BIT(10)
+#define MSD_CMD_EN             BIT(9)
+#define MSD_APP_CMD            BIT(8)
+#define MSD_LONG_RSP           BIT(7)
+#define MSD_NEED_RSP           BIT(6)
+#define MSD_CMD_IDX_MASK       0x3f
+       unsigned int    command;
+
+       unsigned int    argument;
+       unsigned int    response0;
+       unsigned int    response1;
+       unsigned int    response2;
+       unsigned int    response3;
+
+#define MSD_RSP_CMD_APP                BIT(6)
+#define MSD_RSP_CMD_IDX_MASK   0x3f
+       unsigned int    response_command;
+
+#define MSD_DATA_EN            BIT(6)
+#define MSD_DMA_EN             BIT(5)
+#define MSD_DATA_WRITE         BIT(4)
+#define MSD_BLK_SIZE_MASK      0x0f
+       unsigned int    data_control;
+
+       unsigned int    data_timer;
+
+#define MSD_DATA_LEN_MASK      0xffffff
+       unsigned int    data_length;
+
+#define MSD_WRITE_PROT         BIT(12)
+#define MSD_CARD_DETECT                BIT(11)
+/* 1-10 below can be sent to interrupt or clear register */
+#define MSD_CARD_CHANGE                BIT(10)
+#define MSD_FIFO_ORUN          BIT(9)
+#define MSD_FIFO_URUN          BIT(8)
+#define MSD_DATA_END           BIT(7)
+#define MSD_CMD_SENT           BIT(6)
+#define MSD_DATA_CRC_OK                BIT(5)
+#define MSD_RSP_CRC_OK         BIT(4)
+#define MSD_DATA_TIMEOUT       BIT(3)
+#define MSD_RSP_TIMEOUT                BIT(2)
+#define MSD_DATA_CRC_FAIL      BIT(1)
+#define MSD_RSP_CRC_FAIL       BIT(0)
+       unsigned int    status;
+
+       unsigned int    clear;
+       unsigned int    interrupt_mask;
+
+#define MSD_SD_POWER_ON                BIT(4)
+#define MSD_SD_POWER_MASK      0x0f
+       unsigned int    power_control;
+
+#define MSD_CLK_DIS            BIT(8)
+#define MSD_CLK_SD             BIT(7)
+#define MSD_CLK_DIV_MASK       0x7f
+       unsigned int    clock_control;
+
+#define MSD_WIDE_BUS_SUPPORT   BIT(3)
+#define MSD_WIDE_BUS           BIT(2)  /* bus width is 4 bytes */
+#define MSD_SINGLE_BUS         BIT(0)  /* bus width is 1 byte */
+       unsigned int    bus_width;
+
+       unsigned int    data_window;
+
+#define MSD_CPRM_FUNCTION      BIT(8)
+       unsigned int    feature;
+
+       unsigned int    revision;
+};

I see you're using this to handle your register offsets. This isn't
necessarily portable, as you're relying on the size of unsigned int
being 4 bytes, and that elements aren't padded to a larger size. At the
very least unsigned int should be replaced with u32.

Thanks,
Mark.