Re: [PATCH RESEND v2] MMC: SD/MMC Host Controller for Wondermedia WM8505/WM8650

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Tony,

On Sat, Oct 27 2012, Tony Prisk wrote:
> This patch adds support for the SD/MMC host controller found
> on Wondermedia 8xxx series SoCs, currently supported under
> arm/arch-vt8500.
>
> A binding document is also included, based on mmc.txt with
> additional properties.
>
> Signed-off-by: Tony Prisk <linux@xxxxxxxxxxxxxxx>
> ---
> This is a resend of the arch-vt8500 sd/mmc host controller patch for review.
>
> v2: Changes made as per review by Girish K S

Sorry for the delay.  This looks fine, just a few comments:

>  .../devicetree/bindings/mmc/vt8500-sdmmc.txt       |   24 +
>  drivers/mmc/host/Kconfig                           |   12 +
>  drivers/mmc/host/Makefile                          |    1 +
>  drivers/mmc/host/wmt-sdmmc.c                       | 1022 ++++++++++++++++++++
>  4 files changed, 1059 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/vt8500-sdmmc.txt
>  create mode 100644 drivers/mmc/host/wmt-sdmmc.c

Would you like to add a MAINTAINERS entry, too?

> diff --git a/Documentation/devicetree/bindings/mmc/vt8500-sdmmc.txt b/Documentation/devicetree/bindings/mmc/vt8500-sdmmc.txt
> new file mode 100644
> index 0000000..69a817e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/vt8500-sdmmc.txt
> @@ -0,0 +1,24 @@
> +* Wondermedia WM8505/WM8650 SD/MMC Host Controller
> +
> +Required properties:
> +- compatible: Should be "wm,wm8505-sdhc".
> +- reg: Memory for sdhc controller.
> +- interrupts: Two interrupts are required - regular irq and dma irq.
> +- clocks: pHandle to clock for controller.
> +- bus-width: <1>,<4> or <8> data lines connected
> +
> +Optional properties:
> +- sdon-inverted: SD_ON bit is inverted on the controller
> +- cd-inverted: CD bit is inverted on the controller
> +
> +Examples:
> +
> +sdhc@d800a000 {
> +	compatible = "wm,wm8505-sdhc";
> +	reg = <0xd800a000 0x1000>;
> +	interrupts = <20 21>;
> +	clocks = <&sdhc>;
> +	bus-width = <4>;
> +	sdon-inverted;
> +};

It's not necessary to mention properties that are already covered by
mmc.txt outside of the examples section.  I think you can just say:

* Wondermedia WM8505/WM8650 SD/MMC Host Controller

This file documents differences between the core properties described
by mmc.txt and the properties used by the wmt-sdmmc driver.

Required properties:
- compatible: Should be "wm,wm8505-sdhc".
- interrupts: Two interrupts are required - regular irq and dma irq.

Optional properties:
- sdon-inverted: SD_ON bit is inverted on the controller

[..]
> +	if (status != DMA_CCR_EVT_SUCCESS) {
> +		pr_err("%s: DMA Error: Status = %d\n", __func__, status);

If it's possible to use more than one controller on the same system,
these pr_err()s (and the other uses of pr_*) would be better as
dev_err()s so that multiple controllers can be distinguished.

Finally, here's a trivial style patch to align better with the rest
of MMC, please apply it before you resend if you agree.  Thanks!

- Chris.


diff --git a/drivers/mmc/host/wmt-sdmmc.c b/drivers/mmc/host/wmt-sdmmc.c
index b6d7148..5404dc1 100644
--- a/drivers/mmc/host/wmt-sdmmc.c
+++ b/drivers/mmc/host/wmt-sdmmc.c
@@ -216,21 +216,21 @@ static void wmt_set_sd_power(struct wmt_mci_priv *priv, int enable)
 		if (priv->power_inverted) {
 			reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
 			writeb(reg_tmp | BM_SD_OFF,
-				priv->sdmmc_base + SDMMC_BUSMODE);
+			       priv->sdmmc_base + SDMMC_BUSMODE);
 		} else {
 			reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
 			writeb(reg_tmp & (~BM_SD_OFF),
-				priv->sdmmc_base + SDMMC_BUSMODE);
+			       priv->sdmmc_base + SDMMC_BUSMODE);
 		}
 	} else {
 		if (priv->power_inverted) {
 			reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
 			writeb(reg_tmp & (~BM_SD_OFF),
-				priv->sdmmc_base + SDMMC_BUSMODE);
+			       priv->sdmmc_base + SDMMC_BUSMODE);
 		} else {
 			reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
 			writeb(reg_tmp | BM_SD_OFF,
-				priv->sdmmc_base + SDMMC_BUSMODE);
+			       priv->sdmmc_base + SDMMC_BUSMODE);
 		}
 	}
 }
@@ -251,7 +251,7 @@ static void wmt_mci_read_response(struct mmc_host *mmc)
 				tmp_resp = readb(priv->sdmmc_base + SDMMC_RSP);
 			else
 				tmp_resp = readb(priv->sdmmc_base + SDMMC_RSP +
-					(idx1*4) + idx2 + 1);
+						 (idx1*4) + idx2 + 1);
 			response |= (tmp_resp << (idx2 * 8));
 		}
 		priv->cmd->resp[idx1] = cpu_to_be32(response);
@@ -267,7 +267,7 @@ static void wmt_mci_start_command(struct wmt_mci_priv *priv)
 }
 
 static int wmt_mci_send_command(struct mmc_host *mmc, u8 command, u8 cmdtype,
-		u32 arg, u8 rsptype)
+				u32 arg,  u8 rsptype)
 {
 	struct wmt_mci_priv *priv;
 	u32 reg_tmp;
@@ -294,8 +294,8 @@ static int wmt_mci_send_command(struct mmc_host *mmc, u8 command, u8 cmdtype,
 
 	/* set command type */
 	reg_tmp = readb(priv->sdmmc_base + SDMMC_CTLR);
-	writeb((reg_tmp & 0x0F) | (cmdtype << 4), priv->sdmmc_base +
-		SDMMC_CTLR);
+	writeb((reg_tmp & 0x0F) | (cmdtype << 4),
+	       priv->sdmmc_base + SDMMC_CTLR);
 
 	return 0;
 }
@@ -316,10 +316,10 @@ static void wmt_complete_data_request(struct wmt_mci_priv *priv)
 	/* unmap the DMA pages used for write data */
 	if (req->data->flags & MMC_DATA_WRITE)
 		dma_unmap_sg(mmc_dev(priv->mmc), req->data->sg,
-			req->data->sg_len, DMA_TO_DEVICE);
+			     req->data->sg_len, DMA_TO_DEVICE);
 	else
 		dma_unmap_sg(mmc_dev(priv->mmc), req->data->sg,
-			req->data->sg_len, DMA_FROM_DEVICE);
+			     req->data->sg_len, DMA_FROM_DEVICE);
 
 	/* Check if the DMA ISR returned a data error */
 	if ((req->cmd->error) || (req->data->error))
@@ -339,7 +339,7 @@ static void wmt_complete_data_request(struct wmt_mci_priv *priv)
 			init_completion(priv->comp_cmd);
 			priv->cmd = req->data->stop;
 			wmt_mci_send_command(priv->mmc, req->data->stop->opcode,
-						7, req->data->stop->arg, 9);
+					     7, req->data->stop->arg, 9);
 			wmt_mci_start_command(priv);
 		}
 	}
@@ -413,17 +413,17 @@ static irqreturn_t wmt_mci_regular_isr(int irq_num, void *data)
 		return IRQ_HANDLED;
 	}
 
-	if ((!priv->req->data) || ((priv->req->data->stop) &&
-					(priv->cmd == priv->req->data->stop))) {
+	if (!priv->req->data ||
+	    (priv->req->data->stop && (priv->cmd == priv->req->data->stop))) {
 		/* handle non-data & stop_transmission requests */
 		if (status1 & STS1_CMDRSP_DONE) {
 			priv->cmd->error = 0;
 			cmd_done = 1;
-		} else
-		if ((status1 & STS1_RSP_TIMEOUT) ||
-					(status1 & STS1_DATA_TIMEOUT)) {
-			priv->cmd->error = -ETIMEDOUT;
-			cmd_done = 1;
+		}
+		else if ((status1 & STS1_RSP_TIMEOUT) ||
+			 (status1 & STS1_DATA_TIMEOUT)) {
+				priv->cmd->error = -ETIMEDOUT;
+				cmd_done = 1;
 		}
 		if (cmd_done) {
 			priv->comp_cmd = NULL;
@@ -445,7 +445,7 @@ static irqreturn_t wmt_mci_regular_isr(int irq_num, void *data)
 		}
 
 		if ((status1 & STS1_RSP_TIMEOUT) ||
-				(status1 & STS1_DATA_TIMEOUT)) {
+		    (status1 & STS1_DATA_TIMEOUT)) {
 			if (priv->cmd)
 				priv->cmd->error = -ETIMEDOUT;
 			if (priv->comp_cmd)
@@ -496,9 +496,9 @@ static void wmt_reset_hardware(struct mmc_host *mmc)
 
 	/* setup interrupts */
 	writeb(INT0_CD_INT_EN | INT0_DI_INT_EN, priv->sdmmc_base +
-		SDMMC_INTMASK0);
+	       SDMMC_INTMASK0);
 	writeb(INT1_DATA_TOUT_INT_EN | INT1_CMD_RES_TRAN_DONE_INT_EN |
-		INT1_CMD_RES_TOUT_INT_EN, priv->sdmmc_base + SDMMC_INTMASK1);
+	       INT1_CMD_RES_TOUT_INT_EN, priv->sdmmc_base + SDMMC_INTMASK1);
 
 	/* set the DMA timeout */
 	writew(8191, priv->sdmmc_base + SDMMC_DMATIMEOUT);
@@ -553,11 +553,11 @@ static void wmt_dma_config(struct mmc_host *mmc, u32 descaddr, u8 dir)
 	if (dir == PDMA_WRITE) {
 		reg_tmp = readl(priv->sdmmc_base + SDDMA_CCR);
 		writel(reg_tmp & DMA_CCR_IF_TO_PERIPHERAL, priv->sdmmc_base +
-				SDDMA_CCR);
+		       SDDMA_CCR);
 	} else {
 		reg_tmp = readl(priv->sdmmc_base + SDDMA_CCR);
 		writel(reg_tmp | DMA_CCR_PERIPHERAL_TO_IF, priv->sdmmc_base +
-				SDDMA_CCR);
+		       SDDMA_CCR);
 	}
 }
 
@@ -622,7 +622,7 @@ static void wmt_mci_request(struct mmc_host *mmc, struct mmc_request *req)
 		/* set controller data length */
 		reg_tmp = readw(priv->sdmmc_base + SDMMC_BLKLEN);
 		writew((reg_tmp & 0xF800) | (req->data->blksz - 1),
-			priv->sdmmc_base + SDMMC_BLKLEN);
+		       priv->sdmmc_base + SDMMC_BLKLEN);
 
 		/* set controller block count */
 		writew(req->data->blocks, priv->sdmmc_base + SDMMC_BLKCNT);
@@ -631,13 +631,13 @@ static void wmt_mci_request(struct mmc_host *mmc, struct mmc_request *req)
 
 		if (req->data->flags & MMC_DATA_WRITE) {
 			sg_cnt = dma_map_sg(mmc_dev(mmc), req->data->sg,
-					req->data->sg_len, DMA_TO_DEVICE);
+					    req->data->sg_len, DMA_TO_DEVICE);
 			cmdtype = 1;
 			if (req->data->blocks > 1)
 				cmdtype = 3;
 		} else {
 			sg_cnt = dma_map_sg(mmc_dev(mmc), req->data->sg,
-					req->data->sg_len, DMA_FROM_DEVICE);
+					    req->data->sg_len, DMA_FROM_DEVICE);
 			cmdtype = 2;
 			if (req->data->blocks > 1)
 				cmdtype = 4;
@@ -650,8 +650,8 @@ static void wmt_mci_request(struct mmc_host *mmc, struct mmc_request *req)
 			offset = 0;
 			while (offset < sg_dma_len(sg)) {
 				wmt_dma_init_descriptor(desc, req->data->blksz,
-					sg_dma_address(sg)+offset, dma_address,
-					0);
+						sg_dma_address(sg)+offset,
+						dma_address, 0);
 				desc++;
 				desc_cnt++;
 				offset += req->data->blksz;
@@ -665,10 +665,10 @@ static void wmt_mci_request(struct mmc_host *mmc, struct mmc_request *req)
 
 		if (req->data->flags & MMC_DATA_WRITE)
 			wmt_dma_config(mmc, priv->dma_desc_device_addr,
-					PDMA_WRITE);
+				       PDMA_WRITE);
 		else
 			wmt_dma_config(mmc, priv->dma_desc_device_addr,
-					PDMA_READ);
+				       PDMA_READ);
 
 		wmt_mci_send_command(mmc, command, cmdtype, arg, rsptype);
 
@@ -706,7 +706,7 @@ static void wmt_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	case MMC_BUS_WIDTH_4:
 		reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
 		writeb(reg_tmp | BM_FOURBIT_MODE, priv->sdmmc_base +
-			SDMMC_BUSMODE);
+		       SDMMC_BUSMODE);
 
 		reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
 		writeb(reg_tmp & 0xFB, priv->sdmmc_base + SDMMC_EXTCTRL);
@@ -714,7 +714,7 @@ static void wmt_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	case MMC_BUS_WIDTH_1:
 		reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
 		writeb(reg_tmp & BM_ONEBIT_MASK, priv->sdmmc_base +
-			SDMMC_BUSMODE);
+		       SDMMC_BUSMODE);
 
 		reg_tmp = readb(priv->sdmmc_base + SDMMC_EXTCTRL);
 		writeb(reg_tmp & 0xFB, priv->sdmmc_base + SDMMC_EXTCTRL);
@@ -750,7 +750,7 @@ static struct wmt_mci_caps wm8505_caps = {
 	.f_max = 50000000,
 	.ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34,
 	.caps = MMC_CAP_4_BIT_DATA | MMC_CAP_MMC_HIGHSPEED |
-						MMC_CAP_SD_HIGHSPEED,
+		MMC_CAP_SD_HIGHSPEED,
 	.max_seg_size = 65024,
 	.max_segs = 128,
 	.max_blk_size = 2048,
@@ -767,7 +767,7 @@ static int __devinit wmt_mci_probe(struct platform_device *pdev)
 	struct wmt_mci_priv *priv;
 	struct device_node *np = pdev->dev.of_node;
 	const struct of_device_id *of_id =
-				of_match_device(wmt_mci_dt_ids, &pdev->dev);
+		of_match_device(wmt_mci_dt_ids, &pdev->dev);
 	const struct wmt_mci_caps *wmt_caps = of_id->data;
 	int ret;
 	int regular_irq, dma_irq;
@@ -779,7 +779,7 @@ static int __devinit wmt_mci_probe(struct platform_device *pdev)
 
 	if (!np) {
 		pr_err("%s: Missing SDMMC description in devicetree\n",
-								__func__);
+		       __func__);
 		return -EFAULT;
 	}
 
@@ -847,7 +847,9 @@ static int __devinit wmt_mci_probe(struct platform_device *pdev)
 
 	/* alloc some DMA buffers for descriptors/transfers */
 	priv->dma_desc_buffer = dma_alloc_coherent(&pdev->dev,
-		mmc->max_blk_count * 16, &priv->dma_desc_device_addr, 208);
+						   mmc->max_blk_count * 16,
+						   &priv->dma_desc_device_addr,
+						   208);
 	if (!priv->dma_desc_buffer) {
 		pr_err("%s: DMA alloc fail\n", __func__);
 		ret = -EPERM;
@@ -905,7 +907,7 @@ static int __devexit wmt_mci_remove(struct platform_device *pdev)
 
 	/* release the dma buffers */
 	dma_free_coherent(&pdev->dev, priv->mmc->max_blk_count * 16,
-			priv->dma_desc_buffer, priv->dma_desc_device_addr);
+			  priv->dma_desc_buffer, priv->dma_desc_device_addr);
 
 	mmc_remove_host(mmc);
 
@@ -947,7 +949,7 @@ static int wmt_mci_suspend(struct device *dev)
 	if (!ret) {
 		reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
 		writeb(reg_tmp | BM_SOFT_RESET, priv->sdmmc_base +
-			SDMMC_BUSMODE);
+		       SDMMC_BUSMODE);
 
 		reg_tmp = readw(priv->sdmmc_base + SDMMC_BLKLEN);
 		writew(reg_tmp & 0x5FFF, priv->sdmmc_base + SDMMC_BLKLEN);
@@ -974,15 +976,15 @@ static int wmt_mci_resume(struct device *dev)
 
 		reg_tmp = readb(priv->sdmmc_base + SDMMC_BUSMODE);
 		writeb(reg_tmp | BM_SOFT_RESET, priv->sdmmc_base +
-			SDMMC_BUSMODE);
+		       SDMMC_BUSMODE);
 
 		reg_tmp = readw(priv->sdmmc_base + SDMMC_BLKLEN);
 		writew(reg_tmp | (BLKL_GPI_CD | BLKL_INT_ENABLE),
-			priv->sdmmc_base + SDMMC_BLKLEN);
+		       priv->sdmmc_base + SDMMC_BLKLEN);
 
 		reg_tmp = readb(priv->sdmmc_base + SDMMC_INTMASK0);
 		writeb(reg_tmp | INT0_DI_INT_EN, priv->sdmmc_base +
-			SDMMC_INTMASK0);
+		       SDMMC_INTMASK0);
 
 		ret = mmc_resume_host(mmc);
 	}


-- 
Chris Ball   <cjb@xxxxxxxxxx>   <http://printf.net/>
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux