Skip to contentSkip to conversation

MMC:Add support MMCIF for SuperH

PATCH In review Yusuke Goda 3 participants 3 messages Apr 28, 2010 – Apr 30, 2010

Subsystems (2)
MMC, SD and SDIO SuperH
lore
Share & tools
lore

Yusuke Goda

author
#0 lore raw

PATCH v2 1/2 MMC:Add support MMCIF for SuperH

MMCIF is MMC Host Interface in SuperH.

Signed-off-by Yusuke Goda <[email protected]>
---
 drivers/mmc/host/Kconfig     |    8 +
 drivers/mmc/host/Makefile    |    1 +
 drivers/mmc/host/sh_mmcif.c  |  965 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/sh_mmcif.h |   39 ++
 4 files changed, 1013 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mmc/host/sh_mmcif.c
 create mode 100644 include/linux/mmc/sh_mmcif.h
drivers/mmc/host/Kconfig +8 -0
index 2e13b94..aeecbc9 100644
@@ -412,3 +412,11 @@ config SDH_BFIN_MISSING_CMD_PULLUP_WORKAROUND
 	depends on SDH_BFIN
 	help
 	  If you say yes here SD-Cards may work on the EZkit.
+
+config MMC_SH_MMCIF
+	tristate "SuperH Internal MMCIF support"
+	depends on MMC_BLOCK && (SUPERH || ARCH_SHMOBILE)
+	help
+	  This selects the MMC Host Interface controler (MMCIF).
+
+	  This driver supports MMCIF in sh7724/sh7757/sh7372.
drivers/mmc/host/Makefile +1 -0
index f480397..2447b66 100644
@@ -34,6 +34,7 @@ obj-$(CONFIG_MMC_TMIO)		+= tmio_mmc.o
 obj-$(CONFIG_MMC_CB710)	+= cb710-mmc.o
 obj-$(CONFIG_MMC_VIA_SDMMC)	+= via-sdmmc.o
 obj-$(CONFIG_SDH_BFIN)		+= bfin_sdh.o
+obj-$(CONFIG_MMC_SH_MMCIF)	+= sh_mmcif.o
 
 obj-$(CONFIG_MMC_SDHCI_OF)	+= sdhci-of.o
 sdhci-of-y				:= sdhci-of-core.o
drivers/mmc/host/sh_mmcif.c new file +964 -0

964 lines not on this page · raw message

include/linux/mmc/sh_mmcif.h new file +39 -0
new file mode 100644
index 0000000..aafe832
@@ -0,0 +1,39 @@
+/*
+ * include/linux/mmc/sh_mmcif.h
+ *
+ * platform data for eMMC driver
+ *
+ * Copyright (C) 2010 Renesas Solutions Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ *
+ */
+
+#ifndef __SH_MMCIF_H__
+#define __SH_MMCIF_H__
+
+/*
+ * MMCIF : CE_CLK_CTRL [19:16]
+ * 1000 : Peripheral clock / 512
+ * 0111 : Peripheral clock / 256
+ * 0110 : Peripheral clock / 128
+ * 0101 : Peripheral clock / 64
+ * 0100 : Peripheral clock / 32
+ * 0011 : Peripheral clock / 16
+ * 0010 : Peripheral clock / 8
+ * 0001 : Peripheral clock / 4
+ * 0000 : Peripheral clock / 2
+ * 1111 : Peripheral clock (sup_pclk set '1')
+ */
+
+struct sh_mmcif_plat_data {
+	void (*set_pwr)(struct platform_device *pdev, int state);
+	void (*down_pwr)(struct platform_device *pdev);
+	u8	sup_pclk;	/* 1 :SH7757, 0: SH7724/SH7372 */
+	unsigned long caps;
+	u32	ocr;
+};
+
+#endif /* __SH_MMCIF_H__ */
Signature · 1 line
1.7.0
#1 lore raw

Review

Quoted 5 lines · On Wed, 28 Apr 2010 14:06:08 +0900 Yusuke Goda <[email protected]>…
(quoted diff) +5 -0
+	time = wait_event_interruptible_timeout(host->intr_wait,
+			host->wait_int == 1 ||
+			host->sd_error == 1, host->timeout);
+	if (host->wait_int != 1 && (time == 0 || host->sd_error != 0))
+		return sh_mmcif_error_manage(host);

wait_event_interruptible_timeout() will return early with -ERESTARTSYS if the calling process gets signalled (eg, ^C was hit).

The driver uses wait_event_interruptible_timeout() rather a lot and the two sites I looked at seem to handle the signal_pending() case correctly.

But incorrectly handling signals with interruptible waits is a frequently-occurring error in drivers. Did you deliberately cater for this case, and have you runtime tested it?

#2 lore raw

Hi Andrew,

Thanks for your review!

Quoted 11 lines · Andrew Morton
Quoted 5 lines · On Wed, 28 Apr 2010 14:06:08 +0900 Yusuke Goda <[email protected]>…

+     time = wait_event_interruptible_timeout(host->intr_wait,
+                     host->wait_int == 1 ||
+                     host->sd_error == 1, host->timeout);
+     if (host->wait_int != 1 && (time == 0 || host->sd_error != 0))
+             return sh_mmcif_error_manage(host);

wait_event_interruptible_timeout() will return early with -ERESTARTSYS if the calling process gets signalled (eg, ^C was hit).

Quoted 7 lines

The driver uses wait_event_interruptible_timeout() rather a lot and the two sites I looked at seem to handle the signal_pending() case correctly.

But incorrectly handling signals with interruptible waits is a frequently-occurring error in drivers.  Did you deliberately cater for this case, and have you runtime tested it?

My plan is to use this driver on SH-Mobile ARM (sh7372), and I'd be happy to fix up the driver to become a bit less non-standard wrt blocking compared to the other MMC host drivers. Is it possible that Goda-san fixes up whatever minor bits that need rework and that you pick up at that point?

I'd like to rework the MMCIF blocking code and perhaps also add dmaengine support as feature patches on top of Goda-sans work if that's ok with you. Better PM is also on the TODO. Not sure if any of these features will make it in time for the 2.6.35 merge window though.

It would be very useful to have the MMCIF driver as-is in 2.6.35 mainline to begin with if possible. Or do you think the driver needs to be reworked more before that can happen?

Thanks,

/ magnus