mmc: sdhci-pxa: Add SDHCI driver for PXA16x

PATCH Superseded Tanmay Upadhyay 5 participants 26 messages Oct 25, 2011 – Jan 23, 2012 1 message not in the archive

Subsystems (5)

Arm (32-bit) Arm sub-architectures MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver +1 more subsystems

lore

Share & tools

lore

Tanmay Upadhyay

author

2011-10-25 12:37 UTC #0 lore raw

PATCH 2/3 mmc: sdhci-pxa: Add SDHCI driver for PXA16x

Signed-off-by Philip Rakity <[email protected]>

Signed-off-by Tanmay Upadhyay <[email protected]>

---
 drivers/mmc/host/Kconfig                |   13 ++
 drivers/mmc/host/Makefile               |    1 +
 drivers/mmc/host/sdhci-pxav1.c          |  256 +++++++++++++++++++++++++++++++
 drivers/mmc/host/sdhci.c                |    3 +
 drivers/mmc/host/sdhci.h                |    1 +
 include/linux/platform_data/pxa_sdhci.h |    1 +
 6 files changed, 275 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mmc/host/sdhci-pxav1.c

drivers/mmc/host/Kconfig +13 -0

index 8c87096..9c9b73a 100644
@@ -207,6 +207,19 @@ config MMC_SDHCI_PXAV2
 
 	  If unsure, say N.
 
+config MMC_SDHCI_PXAV1
+	tristate "Marvell PXA16X SD Host Controller support (PXAV1)"
+	depends on CLKDEV_LOOKUP
+	select MMC_SDHCI
+	select MMC_SDHCI_PLTFM
+	default CPU_PXA168
+	help
+	  This selects the Marvell(R) PXAV1 SD Host Controller.
+	  If you have a PXA16X platform with SD Host Controller
+	  and a card slot, say Y or M here.
+
+	  If unsure, say N.
+
 config MMC_SDHCI_SPEAR
 	tristate "SDHCI support on ST SPEAr platform"
 	depends on MMC_SDHCI && PLAT_SPEAR

drivers/mmc/host/Makefile +1 -0

index b4b83f3..5094af8 100644
@@ -11,6 +11,7 @@ obj-$(CONFIG_MMC_SDHCI)		+= sdhci.o
 obj-$(CONFIG_MMC_SDHCI_PCI)	+= sdhci-pci.o
 obj-$(CONFIG_MMC_SDHCI_PXAV3)	+= sdhci-pxav3.o
 obj-$(CONFIG_MMC_SDHCI_PXAV2)	+= sdhci-pxav2.o
+obj-$(CONFIG_MMC_SDHCI_PXAV1)	+= sdhci-pxav1.o
 obj-$(CONFIG_MMC_SDHCI_S3C)	+= sdhci-s3c.o
 obj-$(CONFIG_MMC_SDHCI_SPEAR)	+= sdhci-spear.o
 obj-$(CONFIG_MMC_WBSD)		+= wbsd.o

drivers/mmc/host/sdhci-pxav1.c new file +256 -0

256 lines not on this page · raw message

drivers/mmc/host/sdhci.c +3 -0

index 0e02cc1..26043aa 100644
@@ -960,6 +960,9 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mdelay(1);
 	}
 
+	if (host->ops->platform_specific_completion)
+		host->ops->platform_specific_completion(host);
+
 	mod_timer(&host->timer, jiffies + 10 * HZ);
 
 	host->cmd = cmd;

drivers/mmc/host/sdhci.h +1 -0

index 745c42f..363eebc 100644
@@ -274,6 +274,7 @@ struct sdhci_ops {
 	void	(*platform_reset_exit)(struct sdhci_host *host, u8 mask);
 	int	(*set_uhs_signaling)(struct sdhci_host *host, unsigned int uhs);
 
+	void	(*platform_specific_completion)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS

include/linux/platform_data/pxa_sdhci.h +1 -0

index 51ad099..b77f017 100644
@@ -51,6 +51,7 @@ struct sdhci_pxa_platdata {
 	unsigned int	host_caps;
 	unsigned int	quirks;
 	unsigned int	pm_caps;
+	unsigned int	delay_in_ms;
 };
 
 struct sdhci_pxa {

Signature · 1 line

1.6.3.3

Chris Ball

2011-11-10 20:09 UTC #1 lore raw

PATCH 2/3 mmc: sdhci-pxa: Add SDHCI driver for PXA16x

Hi Tanmay and Philip,

Quoted 6 lines · Tanmay Upadhyay

Signed-off-by Philip Rakity <[email protected]>
Signed-off-by Tanmay Upadhyay <[email protected]>
---
 drivers/mmc/host/Kconfig                |   13 ++
 drivers/mmc/host/Makefile               |    1 +
 drivers/mmc/host/sdhci-pxav1.c          |  256 +++++++++++++++++++++++++++++++

By my script's count, 215 of the 256 lines in this file (84%) are simply duplicated from sdhci-pxav2.c, which makes me think we've failed at abstraction here. Can you have a think about how to do better, please?

My intuition is that since there are only 41 lines of difference (actually, less, since the only difference on many of these is s/pxav2/pxav1/g), I'd rather see this become special-cases in the sdhci-pxav2.c code.

Thanks,

- Chris.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Tanmay Upadhyay

author

2011-11-21 04:23 UTC #2 lore raw

PATCH 1/4 mmc: sdhci-pxa: Trivial fix in Kconfig

Select MMC_SDHCI_PXAV3 by default if CPU is MMP2 Select MMC_SDHCI_PXAV2 by default if CPU is PXA910

Signed-off-by Tanmay Upadhyay <[email protected]>

Reviewed-by Philip Rakity <[email protected]>

---
 drivers/mmc/host/Kconfig |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/mmc/host/Kconfig +2 -2

index cf444b0..21b4149 100644
@@ -186,7 +186,7 @@ config MMC_SDHCI_PXAV3
 	depends on CLKDEV_LOOKUP
 	select MMC_SDHCI
 	select MMC_SDHCI_PLTFM
-	default CPU_MMP2
+	default y if CPU_MMP2
 	help
 	  This selects the Marvell(R) PXAV3 SD Host Controller.
 	  If you have a MMP2 platform with SD Host Controller

@@ -199,7 +199,7 @@ config MMC_SDHCI_PXAV2
 	depends on CLKDEV_LOOKUP
 	select MMC_SDHCI
 	select MMC_SDHCI_PLTFM
-	default CPU_PXA910
+	default y if CPU_PXA910
 	help
 	  This selects the Marvell(R) PXAV2 SD Host Controller.
 	  If you have a PXA9XX platform with SD Host Controller

Signature · 1 line

1.7.0.4

Tanmay Upadhyay

author

2011-11-21 04:26 UTC #3 lore raw

PATCH v3 2/4 ARM: pxa168: Add SDHCI support

In reply to Chris Ball

v2 - clock register for SDHCI are not common across all MMP SoCs.
So, move PXA168 implementation to pxa168.c

v3 - sdhci-pxav1 driver code is merged with sdhci-pxav2. So, change
the device name accordingly
- start sdhci device numbering from 1 as other PXA168 devices does that

Signed-off-by Tanmay Upadhyay <[email protected]>

Reviewed-by Philip Rakity <[email protected]>

---
 arch/arm/mach-mmp/include/mach/pxa168.h |   20 +++++++++++++
 arch/arm/mach-mmp/pxa168.c              |   46 +++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 0 deletions(-)

arch/arm/mach-mmp/include/mach/pxa168.h +20 -0

index 7fb568d..a181608 100644
@@ -15,6 +15,7 @@ extern void pxa168_clear_keypad_wakeup(void);
 #include <plat/pxa27x_keypad.h>
 #include <mach/cputype.h>
 #include <linux/pxa168_eth.h>
+#include <linux/platform_data/pxa_sdhci.h>
 
 extern struct pxa_device_desc pxa168_device_uart1;
 extern struct pxa_device_desc pxa168_device_uart2;

@@ -34,6 +35,10 @@ extern struct pxa_device_desc pxa168_device_nand;
 extern struct pxa_device_desc pxa168_device_fb;
 extern struct pxa_device_desc pxa168_device_keypad;
 extern struct pxa_device_desc pxa168_device_eth;
+extern struct pxa_device_desc pxa168_device_sdh1;
+extern struct pxa_device_desc pxa168_device_sdh2;
+extern struct pxa_device_desc pxa168_device_sdh3;
+extern struct pxa_device_desc pxa168_device_sdh4;
 
 struct pxa168_usb_pdata {
 	/* If NULL, default phy init routine for PXA168 would be called */

@@ -132,4 +137,19 @@ static inline int pxa168_add_eth(struct pxa168_eth_platform_data *data)
 {
 	return pxa_register_device(&pxa168_device_eth, data, sizeof(*data));
 }
+
+static inline int pxa168_add_sdh(int id, struct sdhci_pxa_platdata *data)
+{
+	struct pxa_device_desc *d = NULL;
+
+	switch (id) {
+	case 1: d = &pxa168_device_sdh1; break;
+	case 2: d = &pxa168_device_sdh2; break;
+	case 3: d = &pxa168_device_sdh3; break;
+	case 4: d = &pxa168_device_sdh4; break;
+	default:
+		return -EINVAL;
+	}
+	return pxa_register_device(d, data, sizeof(*data));
+}
 #endif /* __ASM_MACH_PXA168_H */

arch/arm/mach-mmp/pxa168.c +46 -0

76 lines not on this page · raw message

Signature · 1 line

1.7.0.4

Chris Ball

2011-12-01 18:05 UTC #4 lore raw

PATCH v3 2/4 ARM: pxa168: Add SDHCI support

Hi Eric, Jason,

Please could you ACK this patch if you agree with it, and I'll take it and the rest of the series via the MMC tree? Thanks.

Quoted 133 lines · Tanmay Upadhyay

v2 - clock register for SDHCI are not common across all MMP SoCs.
So, move PXA168 implementation to pxa168.c

v3 - sdhci-pxav1 driver code is merged with sdhci-pxav2. So, change
the device name accordingly
- start sdhci device numbering from 1 as other PXA168 devices does that

Signed-off-by Tanmay Upadhyay <[email protected]>

Reviewed-by Philip Rakity <[email protected]>

---
 arch/arm/mach-mmp/include/mach/pxa168.h |   20 +++++++++++++
 arch/arm/mach-mmp/pxa168.c              |   46 +++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 0 deletions(-)

arch/arm/mach-mmp/include/mach/pxa168.h +20 -0

index 7fb568d..a181608 100644
@@ -15,6 +15,7 @@ extern void pxa168_clear_keypad_wakeup(void);
 #include <plat/pxa27x_keypad.h>
 #include <mach/cputype.h>
 #include <linux/pxa168_eth.h>
+#include <linux/platform_data/pxa_sdhci.h>
 
 extern struct pxa_device_desc pxa168_device_uart1;
 extern struct pxa_device_desc pxa168_device_uart2;

@@ -34,6 +35,10 @@ extern struct pxa_device_desc pxa168_device_nand;
 extern struct pxa_device_desc pxa168_device_fb;
 extern struct pxa_device_desc pxa168_device_keypad;
 extern struct pxa_device_desc pxa168_device_eth;
+extern struct pxa_device_desc pxa168_device_sdh1;
+extern struct pxa_device_desc pxa168_device_sdh2;
+extern struct pxa_device_desc pxa168_device_sdh3;
+extern struct pxa_device_desc pxa168_device_sdh4;
 
 struct pxa168_usb_pdata {
 	/* If NULL, default phy init routine for PXA168 would be called */

@@ -132,4 +137,19 @@ static inline int pxa168_add_eth(struct pxa168_eth_platform_data *data)
 {
 	return pxa_register_device(&pxa168_device_eth, data, sizeof(*data));
 }
+
+static inline int pxa168_add_sdh(int id, struct sdhci_pxa_platdata *data)
+{
+	struct pxa_device_desc *d = NULL;
+
+	switch (id) {
+	case 1: d = &pxa168_device_sdh1; break;
+	case 2: d = &pxa168_device_sdh2; break;
+	case 3: d = &pxa168_device_sdh3; break;
+	case 4: d = &pxa168_device_sdh4; break;
+	default:
+		return -EINVAL;
+	}
+	return pxa_register_device(d, data, sizeof(*data));
+}
 #endif /* __ASM_MACH_PXA168_H */

arch/arm/mach-mmp/pxa168.c +42 -0

index 76ca15c..102f65f 100644
@@ -45,6 +45,14 @@ static struct mfp_addr_map pxa168_mfp_addr_map[] __initdata =
 
 #define APMASK(i)	(GPIO_REGS_VIRT + BANK_OFF(i) + 0x09c)
 
+/* Offset defined in arch/arm/mach-mmp/include/mach/regs-apmu.h are for MMP2
+ * PXA168 has different offset */
+#undef APMU_SDH2
+#undef APMU_SDH3
+
+#define APMU_SDH2	APMU_REG(0xe0)
+#define APMU_SDH3	APMU_REG(0xe4)
+
 static void __init pxa168_init_gpio(void)
 {
 	int i;

@@ -65,6 +73,31 @@ void __init pxa168_init_irq(void)
 	pxa168_init_gpio();
 }
 
+static void sdh_clk_enable(struct clk *clk)
+{
+	void __iomem *clk_reg_offset = clk->clk_rst;
+
+	/* Can't see any clean way to do this: Bits 3 & 0 in registers
+	 * for host 0 & 2 should be set for host 1 & 3 also */
+	if (clk_reg_offset == APMU_SDH0 || clk_reg_offset == APMU_SDH1)
+		__raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
+	if (clk_reg_offset == APMU_SDH2 || clk_reg_offset == APMU_SDH3)
+		__raw_writel(__raw_readl(APMU_SDH2) | 0x9, APMU_SDH2);
+
+	__raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
+}
+
+static void sdh_clk_disable(struct clk *clk)
+{
+	__raw_writel(__raw_readl(clk->clk_rst) & ~(clk->enable_val),
+			clk->clk_rst);
+}
+
+struct clkops sdh_clk_ops = {
+	.enable		= sdh_clk_enable,
+	.disable	= sdh_clk_disable,
+};
+
 /* APB peripheral clocks */
 static APBC_CLK(uart1, PXA168_UART1, 1, 14745600);
 static APBC_CLK(uart2, PXA168_UART2, 1, 14745600);

@@ -87,6 +120,11 @@ static APMU_CLK(lcd, LCD, 0x7f, 312000000);
 static APMU_CLK(eth, ETH, 0x09, 0);
 static APMU_CLK(usb, USB, 0x12, 0);
 
+static APMU_CLK_OPS(sdh1, SDH0, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh2, SDH1, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh3, SDH2, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh4, SDH3, 0x12, 48000000, &sdh_clk_ops);
+
 /* device and clock bindings */
 static struct clk_lookup pxa168_clkregs[] = {
 	INIT_CLKREG(&clk_uart1, "pxa2xx-uart.0", NULL),

@@ -108,6 +146,10 @@ static struct clk_lookup pxa168_clkregs[] = {
 	INIT_CLKREG(&clk_keypad, "pxa27x-keypad", NULL),
 	INIT_CLKREG(&clk_eth, "pxa168-eth", "MFUCLK"),
 	INIT_CLKREG(&clk_usb, "pxa168-ehci", "PXA168-USBCLK"),
+	INIT_CLKREG(&clk_sdh1, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh2, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh3, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh4, NULL, "PXA-SDHCLK"),
 };
 
 static int __init pxa168_init(void)

arch/arm/mach-mmp/pxa168.c PXA168_DEVICE(ssp5, "pxa168-ssp", 4, SSP5, 0xd4021000, 0x40, 60, 61); · line 224

Quoted 10 lines

arch/arm/mach-mmp/pxa168.c +4 -0

@@ -173,6 +215,10 @@ PXA168_DEVICE(ssp5, "pxa168-ssp", 4, SSP5, 0xd4021000, 0x40, 60, 61);
 PXA168_DEVICE(fb, "pxa168-fb", -1, LCD, 0xd420b000, 0x1c8);
 PXA168_DEVICE(keypad, "pxa27x-keypad", -1, KEYPAD, 0xd4012000, 0x4c);
 PXA168_DEVICE(eth, "pxa168-eth", -1, MFU, 0xc0800000, 0x0fff);
+PXA168_DEVICE(sdh1, "sdhci-pxav2", 0, SDH1, 0xd4280000, 0x100);
+PXA168_DEVICE(sdh2, "sdhci-pxav2", 1, SDH1, 0xd4281000, 0x100);
+PXA168_DEVICE(sdh3, "sdhci-pxav2", 2, SDH2, 0xd427e000, 0x100);
+PXA168_DEVICE(sdh4, "sdhci-pxav2", 3, SDH2, 0xd427f000, 0x100);
 
 struct resource pxa168_usb_host_resources[] = {
 	/* USB Host conroller register base */

- Chris.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Chris Ball

2011-12-15 23:15 UTC #5 lore raw

PATCH v3 2/4 ARM: pxa168: Add SDHCI support

Hi Eric and Jason,

Quoted 4 lines · Chris Ball

Hi Eric, Jason,
Please could you ACK this patch if you agree with it, and I'll take it and the rest of the series via the MMC tree? Thanks.

Ping?

Thanks,

- Chris.

Quoted 144 lines · Tanmay Upadhyay

Quoted 143 lines

v2 - clock register for SDHCI are not common across all MMP SoCs.
So, move PXA168 implementation to pxa168.c

v3 - sdhci-pxav1 driver code is merged with sdhci-pxav2. So, change
the device name accordingly
- start sdhci device numbering from 1 as other PXA168 devices does that

Signed-off-by Tanmay Upadhyay <[email protected]>

Reviewed-by Philip Rakity <[email protected]>

---
 arch/arm/mach-mmp/include/mach/pxa168.h |   20 +++++++++++++
 arch/arm/mach-mmp/pxa168.c              |   46 +++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 0 deletions(-)

arch/arm/mach-mmp/include/mach/pxa168.h +20 -0

index 7fb568d..a181608 100644
@@ -15,6 +15,7 @@ extern void pxa168_clear_keypad_wakeup(void);
 #include <plat/pxa27x_keypad.h>
 #include <mach/cputype.h>
 #include <linux/pxa168_eth.h>
+#include <linux/platform_data/pxa_sdhci.h>
 
 extern struct pxa_device_desc pxa168_device_uart1;
 extern struct pxa_device_desc pxa168_device_uart2;

@@ -34,6 +35,10 @@ extern struct pxa_device_desc pxa168_device_nand;
 extern struct pxa_device_desc pxa168_device_fb;
 extern struct pxa_device_desc pxa168_device_keypad;
 extern struct pxa_device_desc pxa168_device_eth;
+extern struct pxa_device_desc pxa168_device_sdh1;
+extern struct pxa_device_desc pxa168_device_sdh2;
+extern struct pxa_device_desc pxa168_device_sdh3;
+extern struct pxa_device_desc pxa168_device_sdh4;
 
 struct pxa168_usb_pdata {
 	/* If NULL, default phy init routine for PXA168 would be called */

@@ -132,4 +137,19 @@ static inline int pxa168_add_eth(struct pxa168_eth_platform_data *data)
 {
 	return pxa_register_device(&pxa168_device_eth, data, sizeof(*data));
 }
+
+static inline int pxa168_add_sdh(int id, struct sdhci_pxa_platdata *data)
+{
+	struct pxa_device_desc *d = NULL;
+
+	switch (id) {
+	case 1: d = &pxa168_device_sdh1; break;
+	case 2: d = &pxa168_device_sdh2; break;
+	case 3: d = &pxa168_device_sdh3; break;
+	case 4: d = &pxa168_device_sdh4; break;
+	default:
+		return -EINVAL;
+	}
+	return pxa_register_device(d, data, sizeof(*data));
+}
 #endif /* __ASM_MACH_PXA168_H */

arch/arm/mach-mmp/pxa168.c +46 -0

index 76ca15c..102f65f 100644
@@ -45,6 +45,14 @@ static struct mfp_addr_map pxa168_mfp_addr_map[] __initdata =
 
 #define APMASK(i)	(GPIO_REGS_VIRT + BANK_OFF(i) + 0x09c)
 
+/* Offset defined in arch/arm/mach-mmp/include/mach/regs-apmu.h are for MMP2
+ * PXA168 has different offset */
+#undef APMU_SDH2
+#undef APMU_SDH3
+
+#define APMU_SDH2	APMU_REG(0xe0)
+#define APMU_SDH3	APMU_REG(0xe4)
+
 static void __init pxa168_init_gpio(void)
 {
 	int i;

@@ -65,6 +73,31 @@ void __init pxa168_init_irq(void)
 	pxa168_init_gpio();
 }
 
+static void sdh_clk_enable(struct clk *clk)
+{
+	void __iomem *clk_reg_offset = clk->clk_rst;
+
+	/* Can't see any clean way to do this: Bits 3 & 0 in registers
+	 * for host 0 & 2 should be set for host 1 & 3 also */
+	if (clk_reg_offset == APMU_SDH0 || clk_reg_offset == APMU_SDH1)
+		__raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
+	if (clk_reg_offset == APMU_SDH2 || clk_reg_offset == APMU_SDH3)
+		__raw_writel(__raw_readl(APMU_SDH2) | 0x9, APMU_SDH2);
+
+	__raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
+}
+
+static void sdh_clk_disable(struct clk *clk)
+{
+	__raw_writel(__raw_readl(clk->clk_rst) & ~(clk->enable_val),
+			clk->clk_rst);
+}
+
+struct clkops sdh_clk_ops = {
+	.enable		= sdh_clk_enable,
+	.disable	= sdh_clk_disable,
+};
+
 /* APB peripheral clocks */
 static APBC_CLK(uart1, PXA168_UART1, 1, 14745600);
 static APBC_CLK(uart2, PXA168_UART2, 1, 14745600);

@@ -87,6 +120,11 @@ static APMU_CLK(lcd, LCD, 0x7f, 312000000);
 static APMU_CLK(eth, ETH, 0x09, 0);
 static APMU_CLK(usb, USB, 0x12, 0);
 
+static APMU_CLK_OPS(sdh1, SDH0, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh2, SDH1, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh3, SDH2, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh4, SDH3, 0x12, 48000000, &sdh_clk_ops);
+
 /* device and clock bindings */
 static struct clk_lookup pxa168_clkregs[] = {
 	INIT_CLKREG(&clk_uart1, "pxa2xx-uart.0", NULL),

@@ -108,6 +146,10 @@ static struct clk_lookup pxa168_clkregs[] = {
 	INIT_CLKREG(&clk_keypad, "pxa27x-keypad", NULL),
 	INIT_CLKREG(&clk_eth, "pxa168-eth", "MFUCLK"),
 	INIT_CLKREG(&clk_usb, "pxa168-ehci", "PXA168-USBCLK"),
+	INIT_CLKREG(&clk_sdh1, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh2, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh3, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh4, NULL, "PXA-SDHCLK"),
 };
 
 static int __init pxa168_init(void)

@@ -173,6 +215,10 @@ PXA168_DEVICE(ssp5, "pxa168-ssp", 4, SSP5, 0xd4021000, 0x40, 60, 61);
 PXA168_DEVICE(fb, "pxa168-fb", -1, LCD, 0xd420b000, 0x1c8);
 PXA168_DEVICE(keypad, "pxa27x-keypad", -1, KEYPAD, 0xd4012000, 0x4c);
 PXA168_DEVICE(eth, "pxa168-eth", -1, MFU, 0xc0800000, 0x0fff);
+PXA168_DEVICE(sdh1, "sdhci-pxav2", 0, SDH1, 0xd4280000, 0x100);
+PXA168_DEVICE(sdh2, "sdhci-pxav2", 1, SDH1, 0xd4281000, 0x100);
+PXA168_DEVICE(sdh3, "sdhci-pxav2", 2, SDH2, 0xd427e000, 0x100);
+PXA168_DEVICE(sdh4, "sdhci-pxav2", 3, SDH2, 0xd427f000, 0x100);
 
 struct resource pxa168_usb_host_resources[] = {
 	/* USB Host conroller register base */

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Haojian Zhuang

NAK

2011-12-19 04:45 UTC #6 lore raw

PATCH v3 2/4 ARM: pxa168: Add SDHCI support

Quoted 5 lines · Chris Ball

Hi Eric and Jason,
Quoted 2 lines · Chris Ball
Hi Eric, Jason,

Quoted 9 lines

Quoted 2 lines
Please could you ACK this patch if you agree with it, and I'll take it and the rest of the series via the MMC tree? ?Thanks.
Ping?
Thanks,
- Chris.

NACK.

Quoted 8 lines

Quoted 8 lines
arch/arm/mach-mmp/pxa168.c static struct mfp_addr_map pxa168_mfp_addr_map[] __initdata = · line 55
Quoted 8 lines
arch/arm/mach-mmp/pxa168.c +8 -0
+/* Offset defined in arch/arm/mach-mmp/include/mach/regs-apmu.h are for MMP2
+ * PXA168 has different offset */
+#undef APMU_SDH2
+#undef APMU_SDH3
+
+#define APMU_SDH2   APMU_REG(0xe0)
+#define APMU_SDH3   APMU_REG(0xe4)
+

Please don't use #undef at here. If the register setting is different, I prefer to use two different clk operations.

Tanmay Upadhyay

author

2011-12-19 04:55 UTC #7 lore raw

PATCH v3 2/4 ARM: pxa168: Add SDHCI support

Quoted 16 lines · Haojian Zhuang

Quoted 5 lines · Chris Ball
Hi Eric and Jason,
Quoted 2 lines · Chris Ball
Hi Eric, Jason,
Quoted 8 lines
Quoted 2 lines
Please could you ACK this patch if you agree with it, and I'll take it and the rest of the series via the MMC tree? Thanks.
Ping?
Thanks,
- Chris.
NACK.

Quoted 11 lines

Quoted 8 lines
Quoted 8 lines
arch/arm/mach-mmp/pxa168.c static struct mfp_addr_map pxa168_mfp_addr_map[] __initdata = · line 55
Quoted 8 lines
arch/arm/mach-mmp/pxa168.c +8 -0
+/* Offset defined in arch/arm/mach-mmp/include/mach/regs-apmu.h are for MMP2
+ * PXA168 has different offset */
+#undef APMU_SDH2
+#undef APMU_SDH3
+
+#define APMU_SDH2   APMU_REG(0xe0)
+#define APMU_SDH3   APMU_REG(0xe4)
+
Please don't use #undef at here. If the register setting is different, I prefer to use two different clk operations.

Sorry I couldn't get you. Could you please elaborate? Here regs-apmu.h defines clock register offsets. They are correct for MMP2, but not for PXA168. So I thought to correct them in a PXA168 specific file. Could you please point me a better way?

Thanks,

Tanmay

Haojian Zhuang

2011-12-19 05:02 UTC #8 lore raw

PATCH v3 2/4 ARM: pxa168: Add SDHCI support

Quoted 37 lines · Tanmay Upadhyay

Quoted 20 lines · Haojian Zhuang
Quoted 7 lines · On Fri, Dec 16, 2011 at 7:15 AM, Chris Ball<[email protected]> ?wrote:
Hi Eric and Jason,
Quoted 3 lines · Chris Ball
Hi Eric, Jason,
Quoted 9 lines
Quoted 2 lines
Please could you ACK this patch if you agree with it, and I'll take it and the rest of the series via the MMC tree? ?Thanks.
Ping?
Thanks,
- Chris.
NACK.
Quoted 13 lines
Quoted 9 lines
Quoted 9 lines
arch/arm/mach-mmp/pxa168.c static struct mfp_addr_map pxa168_mfp_addr_map[] __initdata = · line 55
Quoted 9 lines
+/* Offset defined in arch/arm/mach-mmp/include/mach/regs-apmu.h are for MMP2
+ * PXA168 has different offset */
arch/arm/mach-mmp/pxa168.c +6 -0
+#undef APMU_SDH2
+#undef APMU_SDH3
+
+#define APMU_SDH2 ? APMU_REG(0xe0)
+#define APMU_SDH3 ? APMU_REG(0xe4)
+
Please don't use #undef at here. If the register setting is different, I prefer to use two different clk operations.

Quoted 8 lines

Sorry I couldn't get you. Could you please elaborate? Here regs-apmu.h defines clock register offsets. They are correct for MMP2, but not for PXA168. So I thought to correct them in a PXA168 specific file. Could you please point me a better way?
Thanks,
Tanmay

APMU_PXA168_SDH2
APMU_MMP2_SDH2

I think this is better.

Tanmay Upadhyay

author

2011-12-19 05:17 UTC #9 lore raw

PATCH v3 2/4 ARM: pxa168: Add SDHCI support

Quoted 37 lines · Haojian Zhuang

Quoted 35 lines · Tanmay Upadhyay
Quoted 16 lines · Haojian Zhuang
Quoted 5 lines · Chris Ball
Hi Eric and Jason,
Quoted 2 lines · Chris Ball
Hi Eric, Jason,
Quoted 8 lines
Quoted 2 lines
Please could you ACK this patch if you agree with it, and I'll take it and the rest of the series via the MMC tree? Thanks.
Ping?
Thanks,
- Chris.
NACK.
Quoted 12 lines
Quoted 9 lines
Quoted 9 lines
arch/arm/mach-mmp/pxa168.c static struct mfp_addr_map pxa168_mfp_addr_map[] __initdata = · line 55
Quoted 9 lines
+/* Offset defined in arch/arm/mach-mmp/include/mach/regs-apmu.h are for MMP2
+ * PXA168 has different offset */
arch/arm/mach-mmp/pxa168.c +6 -0
+#undef APMU_SDH2
+#undef APMU_SDH3
+
+#define APMU_SDH2   APMU_REG(0xe0)
+#define APMU_SDH3   APMU_REG(0xe4)
+
Please don't use #undef at here. If the register setting is different, I prefer to use two different clk operations.
Sorry I couldn't get you. Could you please elaborate? Here regs-apmu.h defines clock register offsets. They are correct for MMP2, but not for PXA168. So I thought to correct them in a PXA168 specific file. Could you please point me a better way?

Quoted 8 lines

Quoted 3 lines
Thanks,
Tanmay
APMU_PXA168_SDH2
APMU_MMP2_SDH2
I think this is better.

Thanks for the suggestion. This looks a better option. However, I would like add here that not only the register offset, but also the register bits are different for MMP2 & PXA168. So, the code that would control SD/MMC clock will be architecture specific & hence in different architecture specific files. Hope this solution looks good to all of you in that case as well.

Thanks,

Tanmay

Haojian Zhuang

2011-12-19 05:24 UTC #10 lore raw

PATCH v3 2/4 ARM: pxa168: Add SDHCI support

Quoted 63 lines · Tanmay Upadhyay

Quoted 44 lines · Haojian Zhuang
Quoted 41 lines · On Mon, Dec 19, 2011 at 12:55 PM, Tanmay Upadhyay…
Quoted 20 lines · Haojian Zhuang
Quoted 7 lines · On Fri, Dec 16, 2011 at 7:15 AM, Chris Ball<[email protected]> ? ?wrote:
Hi Eric and Jason,
Quoted 3 lines · Chris Ball
Hi Eric, Jason,
Quoted 9 lines
Quoted 2 lines
Please could you ACK this patch if you agree with it, and I'll take it and the rest of the series via the MMC tree? ?Thanks.
Ping?
Thanks,
- Chris.
NACK.
Quoted 13 lines
Quoted 9 lines
Quoted 9 lines
Review
Quoted 9 lines
+/* Offset defined in arch/arm/mach-mmp/include/mach/regs-apmu.h are for MMP2
+ * PXA168 has different offset */
(quoted diff) +6 -0
+#undef APMU_SDH2
+#undef APMU_SDH3
+
+#define APMU_SDH2 ? APMU_REG(0xe0)
+#define APMU_SDH3 ? APMU_REG(0xe4)
+
Please don't use #undef at here. If the register setting is different, I prefer to use two different clk operations.
Sorry I couldn't get you. Could you please elaborate? Here regs-apmu.h defines clock register offsets. They are correct for MMP2, but not for PXA168. So I thought to correct them in a PXA168 specific file. Could you please point me a better way?
Quoted 9 lines
Quoted 3 lines
Thanks,
Tanmay
APMU_PXA168_SDH2
APMU_MMP2_SDH2
I think this is better.
Thanks for the suggestion. This looks a better option. However, I would like add here that not only the register offset, but also the register bits are different for MMP2 & PXA168. So, the code that would control SD/MMC clock will be architecture specific & hence in different architecture specific files. Hope this solution looks good to all of you in that case as well.

Quoted 3 lines

Thanks,
Tanmay

+static void sdh_clk_enable(struct clk *clk)
+{
+ void __iomem *clk_reg_offset = clk->clk_rst;
+
+ /* Can't see any clean way to do this: Bits 3 & 0 in registers
+ * for host 0 & 2 should be set for host 1 & 3 also */
+ if (clk_reg_offset == APMU_SDH0 || clk_reg_offset == APMU_SDH1)
+ __raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
+ if (clk_reg_offset == APMU_SDH2 || clk_reg_offset == APMU_SDH3)
+ __raw_writel(__raw_readl(APMU_SDH2) | 0x9, APMU_SDH2);
+
+ __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
+}
+
+struct clkops sdh_clk_ops = {

+       .enable         = sdh_clk_enable,
+       .disable        = sdh_clk_disable,

+};
+

+static APMU_CLK_OPS(sdh1, SDH0, 0x12, 48000000, &sdh_clk_ops); +static APMU_CLK_OPS(sdh2, SDH1, 0x12, 48000000, &sdh_clk_ops); +static APMU_CLK_OPS(sdh3, SDH2, 0x12, 48000000, &sdh_clk_ops); +static APMU_CLK_OPS(sdh4, SDH3, 0x12, 48000000, &sdh_clk_ops); +

You defined both sdh_clk_ops & APMU_CLK_OPS. So you can define pxa168_sdh_clk_ops in pxa168.c, and mmp2_sdh_clk_ops in mmp2.c.

Tanmay Upadhyay

author

2011-12-20 13:13 UTC #11 lore raw

PATCH v4 2/4 ARM: pxa168: Add SDHCI support

v2 - clock register for SDHCI are not common across all MMP SoCs.
So, move PXA168 implementation to pxa168.c

v3 - sdhci-pxav1 driver code is merged with sdhci-pxav2. So, change
the device name accordingly
- start sdhci device numbering from 1 as other PXA168 devices does that

v4 - Use different names for SD clock registers for PXA168 instead
of redefining them in pxa168.c. Suggested by Haojian Zhuang

Signed-off-by Tanmay Upadhyay <[email protected]>

Reviewed-by Philip Rakity <[email protected]>

---
 arch/arm/mach-mmp/include/mach/pxa168.h    |   20 ++++++++++++++
 arch/arm/mach-mmp/include/mach/regs-apmu.h |   40 ++++++++++++++-------------
 arch/arm/mach-mmp/pxa168.c                 |   39 +++++++++++++++++++++++++++
 3 files changed, 80 insertions(+), 19 deletions(-)

arch/arm/mach-mmp/include/mach/pxa168.h +20 -0

index 7fb568d..a181608 100644
@@ -15,6 +15,7 @@ extern void pxa168_clear_keypad_wakeup(void);
 #include <plat/pxa27x_keypad.h>
 #include <mach/cputype.h>
 #include <linux/pxa168_eth.h>
+#include <linux/platform_data/pxa_sdhci.h>
 
 extern struct pxa_device_desc pxa168_device_uart1;
 extern struct pxa_device_desc pxa168_device_uart2;

@@ -34,6 +35,10 @@ extern struct pxa_device_desc pxa168_device_nand;
 extern struct pxa_device_desc pxa168_device_fb;
 extern struct pxa_device_desc pxa168_device_keypad;
 extern struct pxa_device_desc pxa168_device_eth;
+extern struct pxa_device_desc pxa168_device_sdh1;
+extern struct pxa_device_desc pxa168_device_sdh2;
+extern struct pxa_device_desc pxa168_device_sdh3;
+extern struct pxa_device_desc pxa168_device_sdh4;
 
 struct pxa168_usb_pdata {
 	/* If NULL, default phy init routine for PXA168 would be called */

@@ -132,4 +137,19 @@ static inline int pxa168_add_eth(struct pxa168_eth_platform_data *data)
 {
 	return pxa_register_device(&pxa168_device_eth, data, sizeof(*data));
 }
+
+static inline int pxa168_add_sdh(int id, struct sdhci_pxa_platdata *data)
+{
+	struct pxa_device_desc *d = NULL;
+
+	switch (id) {
+	case 1: d = &pxa168_device_sdh1; break;
+	case 2: d = &pxa168_device_sdh2; break;
+	case 3: d = &pxa168_device_sdh3; break;
+	case 4: d = &pxa168_device_sdh4; break;
+	default:
+		return -EINVAL;
+	}
+	return pxa_register_device(d, data, sizeof(*data));
+}
 #endif /* __ASM_MACH_PXA168_H */

arch/arm/mach-mmp/include/mach/regs-apmu.h +21 ~~-19~~

index 8447ac6..a84de77 100644
@@ -17,27 +17,29 @@
 #define APMU_REG(x)	(APMU_VIRT_BASE + (x))
 
 /* Clock Reset Control */
-#define APMU_IRE	APMU_REG(0x048)
-#define APMU_LCD	APMU_REG(0x04c)
-#define APMU_CCIC	APMU_REG(0x050)
-#define APMU_SDH0	APMU_REG(0x054)
-#define APMU_SDH1	APMU_REG(0x058)
-#define APMU_USB	APMU_REG(0x05c)
-#define APMU_NAND	APMU_REG(0x060)
-#define APMU_DMA	APMU_REG(0x064)
-#define APMU_GEU	APMU_REG(0x068)
-#define APMU_BUS	APMU_REG(0x06c)
-#define APMU_SDH2	APMU_REG(0x0e8)
-#define APMU_SDH3	APMU_REG(0x0ec)
-#define APMU_ETH	APMU_REG(0x0fc)
-
-#define APMU_FNCLK_EN	(1 << 4)
-#define APMU_AXICLK_EN	(1 << 3)
-#define APMU_FNRST_DIS	(1 << 1)
-#define APMU_AXIRST_DIS	(1 << 0)
+#define APMU_IRE		APMU_REG(0x048)
+#define APMU_LCD		APMU_REG(0x04c)
+#define APMU_CCIC		APMU_REG(0x050)
+#define APMU_SDH0		APMU_REG(0x054)
+#define APMU_SDH1		APMU_REG(0x058)
+#define APMU_USB		APMU_REG(0x05c)
+#define APMU_NAND		APMU_REG(0x060)
+#define APMU_DMA		APMU_REG(0x064)
+#define APMU_GEU		APMU_REG(0x068)
+#define APMU_BUS		APMU_REG(0x06c)
+#define APMU_PXA168_SDH2	APMU_REG(0x0e0)
+#define APMU_PXA168_SDH3	APMU_REG(0x0e4)
+#define APMU_SDH2		APMU_REG(0x0e8)
+#define APMU_SDH3		APMU_REG(0x0ec)
+#define APMU_ETH		APMU_REG(0x0fc)
+
+#define APMU_FNCLK_EN		(1 << 4)
+#define APMU_AXICLK_EN		(1 << 3)
+#define APMU_FNRST_DIS		(1 << 1)
+#define APMU_AXIRST_DIS		(1 << 0)
 
 /* Wake Clear Register */
-#define APMU_WAKE_CLR	APMU_REG(0x07c)
+#define APMU_WAKE_CLR		APMU_REG(0x07c)
 
 #define APMU_PXA168_KP_WAKE_CLR		(1 << 7)
 #define APMU_PXA168_CFI_WAKE_CLR	(1 << 6)

arch/arm/mach-mmp/pxa168.c +39 -0

index 76ca15c..023659b 100644
@@ -65,6 +65,32 @@ void __init pxa168_init_irq(void)
 	pxa168_init_gpio();
 }
 
+static void sdh_clk_enable(struct clk *clk)
+{
+	void __iomem *reg_offset = clk->clk_rst;
+
+	/* Can't see any clean way to do this: Bits 3 & 0 in registers
+	 * for host 0 & 2 should be set for host 1 & 3 also */
+	if (reg_offset == APMU_SDH0 || reg_offset == APMU_SDH1)
+		__raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
+	if (reg_offset == APMU_PXA168_SDH2 || reg_offset == APMU_PXA168_SDH3)
+		__raw_writel(__raw_readl(APMU_PXA168_SDH2) | 0x9,
+				APMU_PXA168_SDH2);
+
+	__raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
+}
+
+static void sdh_clk_disable(struct clk *clk)
+{
+	__raw_writel(__raw_readl(clk->clk_rst) & ~(clk->enable_val),
+			clk->clk_rst);
+}
+
+struct clkops sdh_clk_ops = {
+	.enable		= sdh_clk_enable,
+	.disable	= sdh_clk_disable,
+};
+
 /* APB peripheral clocks */
 static APBC_CLK(uart1, PXA168_UART1, 1, 14745600);
 static APBC_CLK(uart2, PXA168_UART2, 1, 14745600);

@@ -87,6 +113,11 @@ static APMU_CLK(lcd, LCD, 0x7f, 312000000);
 static APMU_CLK(eth, ETH, 0x09, 0);
 static APMU_CLK(usb, USB, 0x12, 0);
 
+static APMU_CLK_OPS(sdh1, SDH0, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh2, SDH1, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh3, PXA168_SDH2, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh4, PXA168_SDH3, 0x12, 48000000, &sdh_clk_ops);
+
 /* device and clock bindings */
 static struct clk_lookup pxa168_clkregs[] = {
 	INIT_CLKREG(&clk_uart1, "pxa2xx-uart.0", NULL),

@@ -108,6 +139,10 @@ static struct clk_lookup pxa168_clkregs[] = {
 	INIT_CLKREG(&clk_keypad, "pxa27x-keypad", NULL),
 	INIT_CLKREG(&clk_eth, "pxa168-eth", "MFUCLK"),
 	INIT_CLKREG(&clk_usb, "pxa168-ehci", "PXA168-USBCLK"),
+	INIT_CLKREG(&clk_sdh1, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh2, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh3, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh4, NULL, "PXA-SDHCLK"),
 };
 
 static int __init pxa168_init(void)

@@ -173,6 +208,10 @@ PXA168_DEVICE(ssp5, "pxa168-ssp", 4, SSP5, 0xd4021000, 0x40, 60, 61);
 PXA168_DEVICE(fb, "pxa168-fb", -1, LCD, 0xd420b000, 0x1c8);
 PXA168_DEVICE(keypad, "pxa27x-keypad", -1, KEYPAD, 0xd4012000, 0x4c);
 PXA168_DEVICE(eth, "pxa168-eth", -1, MFU, 0xc0800000, 0x0fff);
+PXA168_DEVICE(sdh1, "sdhci-pxav2", 0, SDH1, 0xd4280000, 0x100);
+PXA168_DEVICE(sdh2, "sdhci-pxav2", 1, SDH1, 0xd4281000, 0x100);
+PXA168_DEVICE(sdh3, "sdhci-pxav2", 2, SDH2, 0xd427e000, 0x100);
+PXA168_DEVICE(sdh4, "sdhci-pxav2", 3, SDH2, 0xd427f000, 0x100);
 
 struct resource pxa168_usb_host_resources[] = {
 	/* USB Host conroller register base */

Signature · 1 line

1.7.0.4

Haojian Zhuang

2011-12-20 13:25 UTC #12 lore raw

PATCH v4 2/4 ARM: pxa168: Add SDHCI support

Quoted 128 lines · Tanmay Upadhyay

v2 - clock register for SDHCI are not common across all MMP SoCs. ? ? So, move PXA168 implementation to pxa168.c

v3 - sdhci-pxav1 driver code is merged with sdhci-pxav2. So, change ? ? the device name accordingly
? - start sdhci device numbering from 1 as other PXA168 devices ? ? does that

v4 - Use different names for SD clock registers for PXA168 instead ? ? of redefining them in pxa168.c. Suggested by Haojian Zhuang

Signed-off-by Tanmay Upadhyay <[email protected]>

Reviewed-by Philip Rakity <[email protected]>

---

?arch/arm/mach-mmp/include/mach/pxa168.h ? ?| ? 20 ++++++++++++++
?arch/arm/mach-mmp/include/mach/regs-apmu.h | ? 40 ++++++++++++++-------------
?arch/arm/mach-mmp/pxa168.c ? ? ? ? ? ? ? ? | ? 39 +++++++++++++++++++++++++++

?3 files changed, 80 insertions(+), 19 deletions(-)

arch/arm/mach-mmp/include/mach/pxa168.h +0 -0

index 7fb568d..a181608 100644
@@ -15,6 +15,7 @@ extern void pxa168_clear_keypad_wakeup(void);

?#include <plat/pxa27x_keypad.h>
?#include <mach/cputype.h>
?#include <linux/pxa168_eth.h>
+#include <linux/platform_data/pxa_sdhci.h>

?extern struct pxa_device_desc pxa168_device_uart1; ?extern struct pxa_device_desc pxa168_device_uart2;

arch/arm/mach-mmp/include/mach/pxa168.h +4 -0

@@ -34,6 +35,10 @@ extern struct pxa_device_desc pxa168_device_nand;
 ?extern struct pxa_device_desc pxa168_device_fb;
 ?extern struct pxa_device_desc pxa168_device_keypad;
 ?extern struct pxa_device_desc pxa168_device_eth;
+extern struct pxa_device_desc pxa168_device_sdh1;
+extern struct pxa_device_desc pxa168_device_sdh2;
+extern struct pxa_device_desc pxa168_device_sdh3;
+extern struct pxa_device_desc pxa168_device_sdh4;
 
 ?struct pxa168_usb_pdata {

? ? ? ?/* If NULL, default phy init routine for PXA168 would be called */

arch/arm/mach-mmp/include/mach/pxa168.h +15 -0

@@ -132,4 +137,19 @@ static inline int pxa168_add_eth(struct pxa168_eth_platform_data *data)
 ?{
 ? ? ? ?return pxa_register_device(&pxa168_device_eth, data, sizeof(*data));
 ?}
+
+static inline int pxa168_add_sdh(int id, struct sdhci_pxa_platdata *data)
+{
+ ? ? ? struct pxa_device_desc *d = NULL;
+
+ ? ? ? switch (id) {
+ ? ? ? case 1: d = &pxa168_device_sdh1; break;
+ ? ? ? case 2: d = &pxa168_device_sdh2; break;
+ ? ? ? case 3: d = &pxa168_device_sdh3; break;
+ ? ? ? case 4: d = &pxa168_device_sdh4; break;
+ ? ? ? default:
+ ? ? ? ? ? ? ? return -EINVAL;
+ ? ? ? }
+ ? ? ? return pxa_register_device(d, data, sizeof(*data));
+}

?#endif /* __ASM_MACH_PXA168_H */

arch/arm/mach-mmp/include/mach/regs-apmu.h +0 -0

index 8447ac6..a84de77 100644
@@ -17,27 +17,29 @@
 ?#define APMU_REG(x) ? ?(APMU_VIRT_BASE + (x))

?/* Clock Reset Control */

arch/arm/mach-mmp/include/mach/regs-apmu.h +20 ~~-18~~

-#define APMU_IRE ? ? ? APMU_REG(0x048)
-#define APMU_LCD ? ? ? APMU_REG(0x04c)
-#define APMU_CCIC ? ? ?APMU_REG(0x050)
-#define APMU_SDH0 ? ? ?APMU_REG(0x054)
-#define APMU_SDH1 ? ? ?APMU_REG(0x058)
-#define APMU_USB ? ? ? APMU_REG(0x05c)
-#define APMU_NAND ? ? ?APMU_REG(0x060)
-#define APMU_DMA ? ? ? APMU_REG(0x064)
-#define APMU_GEU ? ? ? APMU_REG(0x068)
-#define APMU_BUS ? ? ? APMU_REG(0x06c)
-#define APMU_SDH2 ? ? ?APMU_REG(0x0e8)
-#define APMU_SDH3 ? ? ?APMU_REG(0x0ec)
-#define APMU_ETH ? ? ? APMU_REG(0x0fc)
-
-#define APMU_FNCLK_EN ?(1 << 4)
-#define APMU_AXICLK_EN (1 << 3)
-#define APMU_FNRST_DIS (1 << 1)
-#define APMU_AXIRST_DIS ? ? ? ?(1 << 0)
+#define APMU_IRE ? ? ? ? ? ? ? APMU_REG(0x048)
+#define APMU_LCD ? ? ? ? ? ? ? APMU_REG(0x04c)
+#define APMU_CCIC ? ? ? ? ? ? ?APMU_REG(0x050)
+#define APMU_SDH0 ? ? ? ? ? ? ?APMU_REG(0x054)
+#define APMU_SDH1 ? ? ? ? ? ? ?APMU_REG(0x058)
+#define APMU_USB ? ? ? ? ? ? ? APMU_REG(0x05c)
+#define APMU_NAND ? ? ? ? ? ? ?APMU_REG(0x060)
+#define APMU_DMA ? ? ? ? ? ? ? APMU_REG(0x064)
+#define APMU_GEU ? ? ? ? ? ? ? APMU_REG(0x068)
+#define APMU_BUS ? ? ? ? ? ? ? APMU_REG(0x06c)
+#define APMU_PXA168_SDH2 ? ? ? APMU_REG(0x0e0)
+#define APMU_PXA168_SDH3 ? ? ? APMU_REG(0x0e4)
+#define APMU_SDH2 ? ? ? ? ? ? ?APMU_REG(0x0e8)
+#define APMU_SDH3 ? ? ? ? ? ? ?APMU_REG(0x0ec)
+#define APMU_ETH ? ? ? ? ? ? ? APMU_REG(0x0fc)
+
+#define APMU_FNCLK_EN ? ? ? ? ?(1 << 4)
+#define APMU_AXICLK_EN ? ? ? ? (1 << 3)
+#define APMU_FNRST_DIS ? ? ? ? (1 << 1)
+#define APMU_AXIRST_DIS ? ? ? ? ? ? ? ?(1 << 0)

?/* Wake Clear Register */

arch/arm/mach-mmp/include/mach/regs-apmu.h +1 -1

-#define APMU_WAKE_CLR ?APMU_REG(0x07c)
+#define APMU_WAKE_CLR ? ? ? ? ?APMU_REG(0x07c)

?#define APMU_PXA168_KP_WAKE_CLR ? ? ? ? ? ? ? ?(1 << 7) ?#define APMU_PXA168_CFI_WAKE_CLR ? ? ? (1 << 6)

arch/arm/mach-mmp/pxa168.c +5 -0

index 76ca15c..023659b 100644
@@ -65,6 +65,32 @@ void __init pxa168_init_irq(void)
 ? ? ? ?pxa168_init_gpio();
 ?}
 
+static void sdh_clk_enable(struct clk *clk)
+{
+ ? ? ? void __iomem *reg_offset = clk->clk_rst;
+
+ ? ? ? /* Can't see any clean way to do this: Bits 3 & 0 in registers

arch/arm/mach-mmp/pxa168.c void __init pxa168_init_irq(void) · line 82

Quoted 10 lines

arch/arm/mach-mmp/pxa168.c +10 -0

+ ? ? ? ?* for host 0 & 2 should be set for host 1 & 3 also */
+ ? ? ? if (reg_offset == APMU_SDH0 || reg_offset == APMU_SDH1)
+ ? ? ? ? ? ? ? __raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
+ ? ? ? if (reg_offset == APMU_PXA168_SDH2 || reg_offset == APMU_PXA168_SDH3)
+ ? ? ? ? ? ? ? __raw_writel(__raw_readl(APMU_PXA168_SDH2) | 0x9,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? APMU_PXA168_SDH2);
+
+ ? ? ? __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
+}
+

How about use the below code instead? I think we can avoid to check register offset.

static void sdh0_clk_enable(struct clk *clk)

{
       void __iomem *reg_offset = clk->clk_rst;

       __raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
       __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
}

static void sdh2_clk_enable(struct clk *clk)

{
       void __iomem *reg_offset = clk->clk_rst;
               __raw_writel(__raw_readl(APMU_PXA168_SDH2) | 0x9,
                               APMU_PXA168_SDH2);
       __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);

}

struct clkops sdh0_clk_ops = {
.enable = sdh0_clk_enable,
.disable = sdh_clk_disable,
};

struct clkops sdh2_clk_ops = {
.enable = sdh2_clk_enable,
.disable = sdh_clk_disable,
};

static APMU_CLK_OPS(sdh1, SDH0, 0x12, 48000000, &sdh0_clk_ops); static APMU_CLK_OPS(sdh2, SDH1, 0x12, 48000000, &sdh0_clk_ops); static APMU_CLK_OPS(sdh3, PXA168_SDH2, 0x12, 48000000, &sdh2_clk_ops); static APMU_CLK_OPS(sdh4, PXA168_SDH3, 0x12, 48000000, &sdh2_clk_ops);

Quoted 51 lines

arch/arm/mach-mmp/pxa168.c +11 -0

+static void sdh_clk_disable(struct clk *clk)
+{
+ ? ? ? __raw_writel(__raw_readl(clk->clk_rst) & ~(clk->enable_val),
+ ? ? ? ? ? ? ? ? ? ? ? clk->clk_rst);
+}
+
+struct clkops sdh_clk_ops = {
+ ? ? ? .enable ? ? ? ? = sdh_clk_enable,
+ ? ? ? .disable ? ? ? ?= sdh_clk_disable,
+};
+

?/* APB peripheral clocks */
?static APBC_CLK(uart1, PXA168_UART1, 1, 14745600); ?static APBC_CLK(uart2, PXA168_UART2, 1, 14745600);

arch/arm/mach-mmp/pxa168.c +5 -0

@@ -87,6 +113,11 @@ static APMU_CLK(lcd, LCD, 0x7f, 312000000);
 ?static APMU_CLK(eth, ETH, 0x09, 0);
 ?static APMU_CLK(usb, USB, 0x12, 0);
 
+static APMU_CLK_OPS(sdh1, SDH0, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh2, SDH1, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh3, PXA168_SDH2, 0x12, 48000000, &sdh_clk_ops);
+static APMU_CLK_OPS(sdh4, PXA168_SDH3, 0x12, 48000000, &sdh_clk_ops);
+

?/* device and clock bindings */
?static struct clk_lookup pxa168_clkregs[] = { ? ? ? ?INIT_CLKREG(&clk_uart1, "pxa2xx-uart.0", NULL),

arch/arm/mach-mmp/pxa168.c +8 -0

@@ -108,6 +139,10 @@ static struct clk_lookup pxa168_clkregs[] = {
 ? ? ? ?INIT_CLKREG(&clk_keypad, "pxa27x-keypad", NULL),
 ? ? ? ?INIT_CLKREG(&clk_eth, "pxa168-eth", "MFUCLK"),
 ? ? ? ?INIT_CLKREG(&clk_usb, "pxa168-ehci", "PXA168-USBCLK"),
+ ? ? ? INIT_CLKREG(&clk_sdh1, NULL, "PXA-SDHCLK"),
+ ? ? ? INIT_CLKREG(&clk_sdh2, NULL, "PXA-SDHCLK"),
+ ? ? ? INIT_CLKREG(&clk_sdh3, NULL, "PXA-SDHCLK"),
+ ? ? ? INIT_CLKREG(&clk_sdh4, NULL, "PXA-SDHCLK"),
 ?};
 
 ?static int __init pxa168_init(void)

@@ -173,6 +208,10 @@ PXA168_DEVICE(ssp5, "pxa168-ssp", 4, SSP5, 0xd4021000, 0x40, 60, 61);
 ?PXA168_DEVICE(fb, "pxa168-fb", -1, LCD, 0xd420b000, 0x1c8);
 ?PXA168_DEVICE(keypad, "pxa27x-keypad", -1, KEYPAD, 0xd4012000, 0x4c);
 ?PXA168_DEVICE(eth, "pxa168-eth", -1, MFU, 0xc0800000, 0x0fff);
+PXA168_DEVICE(sdh1, "sdhci-pxav2", 0, SDH1, 0xd4280000, 0x100);
+PXA168_DEVICE(sdh2, "sdhci-pxav2", 1, SDH1, 0xd4281000, 0x100);
+PXA168_DEVICE(sdh3, "sdhci-pxav2", 2, SDH2, 0xd427e000, 0x100);
+PXA168_DEVICE(sdh4, "sdhci-pxav2", 3, SDH2, 0xd427f000, 0x100);
 
 ?struct resource pxa168_usb_host_resources[] = {

? ? ? ?/* USB Host conroller register base */
--
1.7.0.4

Haojian Zhuang

2011-12-20 13:38 UTC #13 lore raw

PATCH v4 2/4 ARM: pxa168: Add SDHCI support

Quoted 97 lines · Haojian Zhuang

Quoted 95 lines · Tanmay Upadhyay

v2 - clock register for SDHCI are not common across all MMP SoCs. ? ? So, move PXA168 implementation to pxa168.c

v3 - sdhci-pxav1 driver code is merged with sdhci-pxav2. So, change ? ? the device name accordingly
? - start sdhci device numbering from 1 as other PXA168 devices ? ? does that

v4 - Use different names for SD clock registers for PXA168 instead ? ? of redefining them in pxa168.c. Suggested by Haojian Zhuang

Signed-off-by Tanmay Upadhyay <[email protected]>

Reviewed-by Philip Rakity <[email protected]>

---

?arch/arm/mach-mmp/include/mach/pxa168.h ? ?| ? 20 ++++++++++++++
?arch/arm/mach-mmp/include/mach/regs-apmu.h | ? 40 ++++++++++++++-------------
?arch/arm/mach-mmp/pxa168.c ? ? ? ? ? ? ? ? | ? 39 +++++++++++++++++++++++++++

?3 files changed, 80 insertions(+), 19 deletions(-)

arch/arm/mach-mmp/include/mach/pxa168.h +0 -0

index 7fb568d..a181608 100644
@@ -15,6 +15,7 @@ extern void pxa168_clear_keypad_wakeup(void);

?#include <plat/pxa27x_keypad.h>
?#include <mach/cputype.h>
?#include <linux/pxa168_eth.h>
+#include <linux/platform_data/pxa_sdhci.h>

?extern struct pxa_device_desc pxa168_device_uart1; ?extern struct pxa_device_desc pxa168_device_uart2;

arch/arm/mach-mmp/include/mach/pxa168.h +4 -0

@@ -34,6 +35,10 @@ extern struct pxa_device_desc pxa168_device_nand;
 ?extern struct pxa_device_desc pxa168_device_fb;
 ?extern struct pxa_device_desc pxa168_device_keypad;
 ?extern struct pxa_device_desc pxa168_device_eth;
+extern struct pxa_device_desc pxa168_device_sdh1;
+extern struct pxa_device_desc pxa168_device_sdh2;
+extern struct pxa_device_desc pxa168_device_sdh3;
+extern struct pxa_device_desc pxa168_device_sdh4;
 
 ?struct pxa168_usb_pdata {

? ? ? ?/* If NULL, default phy init routine for PXA168 would be called */

arch/arm/mach-mmp/include/mach/pxa168.h +15 -0

@@ -132,4 +137,19 @@ static inline int pxa168_add_eth(struct pxa168_eth_platform_data *data)
 ?{
 ? ? ? ?return pxa_register_device(&pxa168_device_eth, data, sizeof(*data));
 ?}
+
+static inline int pxa168_add_sdh(int id, struct sdhci_pxa_platdata *data)
+{
+ ? ? ? struct pxa_device_desc *d = NULL;
+
+ ? ? ? switch (id) {
+ ? ? ? case 1: d = &pxa168_device_sdh1; break;
+ ? ? ? case 2: d = &pxa168_device_sdh2; break;
+ ? ? ? case 3: d = &pxa168_device_sdh3; break;
+ ? ? ? case 4: d = &pxa168_device_sdh4; break;
+ ? ? ? default:
+ ? ? ? ? ? ? ? return -EINVAL;
+ ? ? ? }
+ ? ? ? return pxa_register_device(d, data, sizeof(*data));
+}

?#endif /* __ASM_MACH_PXA168_H */

arch/arm/mach-mmp/include/mach/regs-apmu.h +0 -0

index 8447ac6..a84de77 100644
@@ -17,27 +17,29 @@
 ?#define APMU_REG(x) ? ?(APMU_VIRT_BASE + (x))

?/* Clock Reset Control */

arch/arm/mach-mmp/include/mach/regs-apmu.h +7 ~~-18~~

-#define APMU_IRE ? ? ? APMU_REG(0x048)
-#define APMU_LCD ? ? ? APMU_REG(0x04c)
-#define APMU_CCIC ? ? ?APMU_REG(0x050)
-#define APMU_SDH0 ? ? ?APMU_REG(0x054)
-#define APMU_SDH1 ? ? ?APMU_REG(0x058)
-#define APMU_USB ? ? ? APMU_REG(0x05c)
-#define APMU_NAND ? ? ?APMU_REG(0x060)
-#define APMU_DMA ? ? ? APMU_REG(0x064)
-#define APMU_GEU ? ? ? APMU_REG(0x068)
-#define APMU_BUS ? ? ? APMU_REG(0x06c)
-#define APMU_SDH2 ? ? ?APMU_REG(0x0e8)
-#define APMU_SDH3 ? ? ?APMU_REG(0x0ec)
-#define APMU_ETH ? ? ? APMU_REG(0x0fc)
-
-#define APMU_FNCLK_EN ?(1 << 4)
-#define APMU_AXICLK_EN (1 << 3)
-#define APMU_FNRST_DIS (1 << 1)
-#define APMU_AXIRST_DIS ? ? ? ?(1 << 0)
+#define APMU_IRE ? ? ? ? ? ? ? APMU_REG(0x048)
+#define APMU_LCD ? ? ? ? ? ? ? APMU_REG(0x04c)
+#define APMU_CCIC ? ? ? ? ? ? ?APMU_REG(0x050)
+#define APMU_SDH0 ? ? ? ? ? ? ?APMU_REG(0x054)
+#define APMU_SDH1 ? ? ? ? ? ? ?APMU_REG(0x058)
+#define APMU_USB ? ? ? ? ? ? ? APMU_REG(0x05c)
+#define APMU_NAND ? ? ? ? ? ? ?APMU_REG(0x060)

Quoted 8 lines

arch/arm/mach-mmp/include/mach/regs-apmu.h

Quoted 8 lines

arch/arm/mach-mmp/include/mach/regs-apmu.h +8 -0

+#define APMU_DMA ? ? ? ? ? ? ? APMU_REG(0x064)
+#define APMU_GEU ? ? ? ? ? ? ? APMU_REG(0x068)
+#define APMU_BUS ? ? ? ? ? ? ? APMU_REG(0x06c)
+#define APMU_PXA168_SDH2 ? ? ? APMU_REG(0x0e0)
+#define APMU_PXA168_SDH3 ? ? ? APMU_REG(0x0e4)
+#define APMU_SDH2 ? ? ? ? ? ? ?APMU_REG(0x0e8)
+#define APMU_SDH3 ? ? ? ? ? ? ?APMU_REG(0x0ec)
+#define APMU_ETH ? ? ? ? ? ? ? APMU_REG(0x0fc)

Since you only append APMU_PXA168_SDH2 & APMU_PXA168_SDH3, you'd better not touch other registers.
If you want to clean the code, you can submit another patch to handle it.

Quoted 70 lines

Quoted 25 lines
arch/arm/mach-mmp/include/mach/regs-apmu.h +5 -0
+
+#define APMU_FNCLK_EN ? ? ? ? ?(1 << 4)
+#define APMU_AXICLK_EN ? ? ? ? (1 << 3)
+#define APMU_FNRST_DIS ? ? ? ? (1 << 1)
+#define APMU_AXIRST_DIS ? ? ? ? ? ? ? ?(1 << 0)
?/* Wake Clear Register */
arch/arm/mach-mmp/include/mach/regs-apmu.h +1 -1
-#define APMU_WAKE_CLR ?APMU_REG(0x07c)
+#define APMU_WAKE_CLR ? ? ? ? ?APMU_REG(0x07c)
?#define APMU_PXA168_KP_WAKE_CLR ? ? ? ? ? ? ? ?(1 << 7) ?#define APMU_PXA168_CFI_WAKE_CLR ? ? ? (1 << 6)
arch/arm/mach-mmp/pxa168.c +5 -0
index 76ca15c..023659b 100644
@@ -65,6 +65,32 @@ void __init pxa168_init_irq(void)
 ? ? ? ?pxa168_init_gpio();
 ?}
 
+static void sdh_clk_enable(struct clk *clk)
+{
+ ? ? ? void __iomem *reg_offset = clk->clk_rst;
+
+ ? ? ? /* Can't see any clean way to do this: Bits 3 & 0 in registers
arch/arm/mach-mmp/pxa168.c void __init pxa168_init_irq(void) · line 82
Quoted 10 lines
arch/arm/mach-mmp/pxa168.c +10 -0
+ ? ? ? ?* for host 0 & 2 should be set for host 1 & 3 also */
+ ? ? ? if (reg_offset == APMU_SDH0 || reg_offset == APMU_SDH1)
+ ? ? ? ? ? ? ? __raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
+ ? ? ? if (reg_offset == APMU_PXA168_SDH2 || reg_offset == APMU_PXA168_SDH3)
+ ? ? ? ? ? ? ? __raw_writel(__raw_readl(APMU_PXA168_SDH2) | 0x9,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? APMU_PXA168_SDH2);
+
+ ? ? ? __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
+}
+
How about use the below code instead? I think we can avoid to check register offset.
static void sdh0_clk_enable(struct clk *clk)
{
? ? ? void __iomem *reg_offset = clk->clk_rst;
? ? ? __raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
? ? ? __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst); }
static void sdh2_clk_enable(struct clk *clk)
{
? ? ? void __iomem *reg_offset = clk->clk_rst;
? ? ? ? ? ? ? __raw_writel(__raw_readl(APMU_PXA168_SDH2) | 0x9,
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? APMU_PXA168_SDH2);
? ? ? __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
}
struct clkops sdh0_clk_ops = {
? ? ?.enable ? ? ? ? = sdh0_clk_enable,
? ? ?.disable ? ? ? ?= sdh_clk_disable,
};
struct clkops sdh2_clk_ops = {
? ? ?.enable ? ? ? ? = sdh2_clk_enable,
? ? ?.disable ? ? ? ?= sdh_clk_disable,
};
static APMU_CLK_OPS(sdh1, SDH0, 0x12, 48000000, &sdh0_clk_ops); static APMU_CLK_OPS(sdh2, SDH1, 0x12, 48000000, &sdh0_clk_ops); static APMU_CLK_OPS(sdh3, PXA168_SDH2, 0x12, 48000000, &sdh2_clk_ops); static APMU_CLK_OPS(sdh4, PXA168_SDH3, 0x12, 48000000, &sdh2_clk_ops);

Tanmay Upadhyay

author

2011-12-20 13:44 UTC #14 lore raw

PATCH v4 2/4 ARM: pxa168: Add SDHCI support

Quoted 102 lines · Haojian Zhuang

Quoted 100 lines · Haojian Zhuang

Quoted 98 lines · Tanmay Upadhyay

v2 - clock register for SDHCI are not common across all MMP SoCs.
So, move PXA168 implementation to pxa168.c

v3 - sdhci-pxav1 driver code is merged with sdhci-pxav2. So, change
the device name accordingly
- start sdhci device numbering from 1 as other PXA168 devices does that

v4 - Use different names for SD clock registers for PXA168 instead
of redefining them in pxa168.c. Suggested by Haojian Zhuang

Signed-off-by Tanmay Upadhyay<[email protected]>

Reviewed-by Philip Rakity<[email protected]>

---
  arch/arm/mach-mmp/include/mach/pxa168.h    |   20 ++++++++++++++
  arch/arm/mach-mmp/include/mach/regs-apmu.h |   40 ++++++++++++++-------------
  arch/arm/mach-mmp/pxa168.c                 |   39 +++++++++++++++++++++++++++
  3 files changed, 80 insertions(+), 19 deletions(-)

arch/arm/mach-mmp/include/mach/pxa168.h +20 -0

index 7fb568d..a181608 100644
@@ -15,6 +15,7 @@ extern void pxa168_clear_keypad_wakeup(void);
  #include<plat/pxa27x_keypad.h>
  #include<mach/cputype.h>
  #include<linux/pxa168_eth.h>
+#include<linux/platform_data/pxa_sdhci.h>
 
  extern struct pxa_device_desc pxa168_device_uart1;
  extern struct pxa_device_desc pxa168_device_uart2;

@@ -34,6 +35,10 @@ extern struct pxa_device_desc pxa168_device_nand;
  extern struct pxa_device_desc pxa168_device_fb;
  extern struct pxa_device_desc pxa168_device_keypad;
  extern struct pxa_device_desc pxa168_device_eth;
+extern struct pxa_device_desc pxa168_device_sdh1;
+extern struct pxa_device_desc pxa168_device_sdh2;
+extern struct pxa_device_desc pxa168_device_sdh3;
+extern struct pxa_device_desc pxa168_device_sdh4;
 
  struct pxa168_usb_pdata {
        /* If NULL, default phy init routine for PXA168 would be called */

@@ -132,4 +137,19 @@ static inline int pxa168_add_eth(struct pxa168_eth_platform_data *data)
  {
        return pxa_register_device(&pxa168_device_eth, data, sizeof(*data));
  }
+
+static inline int pxa168_add_sdh(int id, struct sdhci_pxa_platdata *data)
+{
+       struct pxa_device_desc *d = NULL;
+
+       switch (id) {
+       case 1: d =&pxa168_device_sdh1; break;
+       case 2: d =&pxa168_device_sdh2; break;
+       case 3: d =&pxa168_device_sdh3; break;
+       case 4: d =&pxa168_device_sdh4; break;
+       default:
+               return -EINVAL;
+       }
+       return pxa_register_device(d, data, sizeof(*data));
+}
  #endif /* __ASM_MACH_PXA168_H */

arch/arm/mach-mmp/include/mach/regs-apmu.h +10 ~~-18~~

index 8447ac6..a84de77 100644
@@ -17,27 +17,29 @@
  #define APMU_REG(x)    (APMU_VIRT_BASE + (x))
 
  /* Clock Reset Control */
-#define APMU_IRE       APMU_REG(0x048)
-#define APMU_LCD       APMU_REG(0x04c)
-#define APMU_CCIC      APMU_REG(0x050)
-#define APMU_SDH0      APMU_REG(0x054)
-#define APMU_SDH1      APMU_REG(0x058)
-#define APMU_USB       APMU_REG(0x05c)
-#define APMU_NAND      APMU_REG(0x060)
-#define APMU_DMA       APMU_REG(0x064)
-#define APMU_GEU       APMU_REG(0x068)
-#define APMU_BUS       APMU_REG(0x06c)
-#define APMU_SDH2      APMU_REG(0x0e8)
-#define APMU_SDH3      APMU_REG(0x0ec)
-#define APMU_ETH       APMU_REG(0x0fc)
-
-#define APMU_FNCLK_EN  (1<<  4)
-#define APMU_AXICLK_EN (1<<  3)
-#define APMU_FNRST_DIS (1<<  1)
-#define APMU_AXIRST_DIS        (1<<  0)
+#define APMU_IRE               APMU_REG(0x048)
+#define APMU_LCD               APMU_REG(0x04c)
+#define APMU_CCIC              APMU_REG(0x050)
+#define APMU_SDH0              APMU_REG(0x054)
+#define APMU_SDH1              APMU_REG(0x058)
+#define APMU_USB               APMU_REG(0x05c)
+#define APMU_NAND              APMU_REG(0x060)
+#define APMU_DMA               APMU_REG(0x064)
+#define APMU_GEU               APMU_REG(0x068)
+#define APMU_BUS               APMU_REG(0x06c)

Quoted 9 lines

Quoted 5 lines
arch/arm/mach-mmp/include/mach/regs-apmu.h line 34
Quoted 5 lines
arch/arm/mach-mmp/include/mach/regs-apmu.h +5 -0
+#define APMU_PXA168_SDH2       APMU_REG(0x0e0)
+#define APMU_PXA168_SDH3       APMU_REG(0x0e4)
+#define APMU_SDH2              APMU_REG(0x0e8)
+#define APMU_SDH3              APMU_REG(0x0ec)
+#define APMU_ETH               APMU_REG(0x0fc)
Since you only append APMU_PXA168_SDH2& APMU_PXA168_SDH3, you'd better not touch other registers.
If you want to clean the code, you can submit another patch to handle it.

Ok.

-- Tanmay

Quoted 70 lines

Quoted 25 lines

arch/arm/mach-mmp/include/mach/regs-apmu.h +6 -1

+
+#define APMU_FNCLK_EN          (1<<  4)
+#define APMU_AXICLK_EN         (1<<  3)
+#define APMU_FNRST_DIS         (1<<  1)
+#define APMU_AXIRST_DIS                (1<<  0)
 
  /* Wake Clear Register */
-#define APMU_WAKE_CLR  APMU_REG(0x07c)
+#define APMU_WAKE_CLR          APMU_REG(0x07c)
 
  #define APMU_PXA168_KP_WAKE_CLR                (1<<  7)
  #define APMU_PXA168_CFI_WAKE_CLR       (1<<  6)

arch/arm/mach-mmp/pxa168.c +5 -0

index 76ca15c..023659b 100644
@@ -65,6 +65,32 @@ void __init pxa168_init_irq(void)
        pxa168_init_gpio();
  }
 
+static void sdh_clk_enable(struct clk *clk)
+{
+       void __iomem *reg_offset = clk->clk_rst;
+
+       /* Can't see any clean way to do this: Bits 3&  0 in registers

arch/arm/mach-mmp/pxa168.c void __init pxa168_init_irq(void) · line 82

Quoted 10 lines

arch/arm/mach-mmp/pxa168.c +10 -0

+        * for host 0&  2 should be set for host 1&  3 also */
+       if (reg_offset == APMU_SDH0 || reg_offset == APMU_SDH1)
+               __raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
+       if (reg_offset == APMU_PXA168_SDH2 || reg_offset == APMU_PXA168_SDH3)
+               __raw_writel(__raw_readl(APMU_PXA168_SDH2) | 0x9,
+                               APMU_PXA168_SDH2);
+
+       __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
+}
+

How about use the below code instead? I think we can avoid to check register offset.

static void sdh0_clk_enable(struct clk *clk)

{
       void __iomem *reg_offset = clk->clk_rst;

       __raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
       __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
}

static void sdh2_clk_enable(struct clk *clk)

{
       void __iomem *reg_offset = clk->clk_rst;
               __raw_writel(__raw_readl(APMU_PXA168_SDH2) | 0x9,
                               APMU_PXA168_SDH2);
       __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);

}

struct clkops sdh0_clk_ops = {
.enable = sdh0_clk_enable,
.disable = sdh_clk_disable,
};

struct clkops sdh2_clk_ops = {
.enable = sdh2_clk_enable,
.disable = sdh_clk_disable,
};

static APMU_CLK_OPS(sdh1, SDH0, 0x12, 48000000,&sdh0_clk_ops); static APMU_CLK_OPS(sdh2, SDH1, 0x12, 48000000,&sdh0_clk_ops); static APMU_CLK_OPS(sdh3, PXA168_SDH2, 0x12, 48000000,&sdh2_clk_ops); static APMU_CLK_OPS(sdh4, PXA168_SDH3, 0x12, 48000000,&sdh2_clk_ops);

Tanmay Upadhyay

author

2011-12-20 13:44 UTC #15 lore raw

PATCH v4 2/4 ARM: pxa168: Add SDHCI support

In reply to Haojian Zhuang

Quoted 170 lines · Haojian Zhuang

Quoted 128 lines · Tanmay Upadhyay

v2 - clock register for SDHCI are not common across all MMP SoCs.
So, move PXA168 implementation to pxa168.c

v3 - sdhci-pxav1 driver code is merged with sdhci-pxav2. So, change
the device name accordingly
- start sdhci device numbering from 1 as other PXA168 devices does that

v4 - Use different names for SD clock registers for PXA168 instead
of redefining them in pxa168.c. Suggested by Haojian Zhuang

Signed-off-by Tanmay Upadhyay<[email protected]>

Reviewed-by Philip Rakity<[email protected]>

---
  arch/arm/mach-mmp/include/mach/pxa168.h    |   20 ++++++++++++++
  arch/arm/mach-mmp/include/mach/regs-apmu.h |   40 ++++++++++++++-------------
  arch/arm/mach-mmp/pxa168.c                 |   39 +++++++++++++++++++++++++++
  3 files changed, 80 insertions(+), 19 deletions(-)

arch/arm/mach-mmp/include/mach/pxa168.h +20 -0

index 7fb568d..a181608 100644
@@ -15,6 +15,7 @@ extern void pxa168_clear_keypad_wakeup(void);
  #include<plat/pxa27x_keypad.h>
  #include<mach/cputype.h>
  #include<linux/pxa168_eth.h>
+#include<linux/platform_data/pxa_sdhci.h>
 
  extern struct pxa_device_desc pxa168_device_uart1;
  extern struct pxa_device_desc pxa168_device_uart2;

@@ -34,6 +35,10 @@ extern struct pxa_device_desc pxa168_device_nand;
  extern struct pxa_device_desc pxa168_device_fb;
  extern struct pxa_device_desc pxa168_device_keypad;
  extern struct pxa_device_desc pxa168_device_eth;
+extern struct pxa_device_desc pxa168_device_sdh1;
+extern struct pxa_device_desc pxa168_device_sdh2;
+extern struct pxa_device_desc pxa168_device_sdh3;
+extern struct pxa_device_desc pxa168_device_sdh4;
 
  struct pxa168_usb_pdata {
        /* If NULL, default phy init routine for PXA168 would be called */

@@ -132,4 +137,19 @@ static inline int pxa168_add_eth(struct pxa168_eth_platform_data *data)
  {
        return pxa_register_device(&pxa168_device_eth, data, sizeof(*data));
  }
+
+static inline int pxa168_add_sdh(int id, struct sdhci_pxa_platdata *data)
+{
+       struct pxa_device_desc *d = NULL;
+
+       switch (id) {
+       case 1: d =&pxa168_device_sdh1; break;
+       case 2: d =&pxa168_device_sdh2; break;
+       case 3: d =&pxa168_device_sdh3; break;
+       case 4: d =&pxa168_device_sdh4; break;
+       default:
+               return -EINVAL;
+       }
+       return pxa_register_device(d, data, sizeof(*data));
+}
  #endif /* __ASM_MACH_PXA168_H */

arch/arm/mach-mmp/include/mach/regs-apmu.h +21 ~~-19~~

index 8447ac6..a84de77 100644
@@ -17,27 +17,29 @@
  #define APMU_REG(x)    (APMU_VIRT_BASE + (x))
 
  /* Clock Reset Control */
-#define APMU_IRE       APMU_REG(0x048)
-#define APMU_LCD       APMU_REG(0x04c)
-#define APMU_CCIC      APMU_REG(0x050)
-#define APMU_SDH0      APMU_REG(0x054)
-#define APMU_SDH1      APMU_REG(0x058)
-#define APMU_USB       APMU_REG(0x05c)
-#define APMU_NAND      APMU_REG(0x060)
-#define APMU_DMA       APMU_REG(0x064)
-#define APMU_GEU       APMU_REG(0x068)
-#define APMU_BUS       APMU_REG(0x06c)
-#define APMU_SDH2      APMU_REG(0x0e8)
-#define APMU_SDH3      APMU_REG(0x0ec)
-#define APMU_ETH       APMU_REG(0x0fc)
-
-#define APMU_FNCLK_EN  (1<<  4)
-#define APMU_AXICLK_EN (1<<  3)
-#define APMU_FNRST_DIS (1<<  1)
-#define APMU_AXIRST_DIS        (1<<  0)
+#define APMU_IRE               APMU_REG(0x048)
+#define APMU_LCD               APMU_REG(0x04c)
+#define APMU_CCIC              APMU_REG(0x050)
+#define APMU_SDH0              APMU_REG(0x054)
+#define APMU_SDH1              APMU_REG(0x058)
+#define APMU_USB               APMU_REG(0x05c)
+#define APMU_NAND              APMU_REG(0x060)
+#define APMU_DMA               APMU_REG(0x064)
+#define APMU_GEU               APMU_REG(0x068)
+#define APMU_BUS               APMU_REG(0x06c)
+#define APMU_PXA168_SDH2       APMU_REG(0x0e0)
+#define APMU_PXA168_SDH3       APMU_REG(0x0e4)
+#define APMU_SDH2              APMU_REG(0x0e8)
+#define APMU_SDH3              APMU_REG(0x0ec)
+#define APMU_ETH               APMU_REG(0x0fc)
+
+#define APMU_FNCLK_EN          (1<<  4)
+#define APMU_AXICLK_EN         (1<<  3)
+#define APMU_FNRST_DIS         (1<<  1)
+#define APMU_AXIRST_DIS                (1<<  0)
 
  /* Wake Clear Register */
-#define APMU_WAKE_CLR  APMU_REG(0x07c)
+#define APMU_WAKE_CLR          APMU_REG(0x07c)
 
  #define APMU_PXA168_KP_WAKE_CLR                (1<<  7)
  #define APMU_PXA168_CFI_WAKE_CLR       (1<<  6)

arch/arm/mach-mmp/pxa168.c +5 -0

index 76ca15c..023659b 100644
@@ -65,6 +65,32 @@ void __init pxa168_init_irq(void)
        pxa168_init_gpio();
  }
 
+static void sdh_clk_enable(struct clk *clk)
+{
+       void __iomem *reg_offset = clk->clk_rst;
+
+       /* Can't see any clean way to do this: Bits 3&  0 in registers

arch/arm/mach-mmp/pxa168.c void __init pxa168_init_irq(void) · line 82

Quoted 10 lines

arch/arm/mach-mmp/pxa168.c +10 -0

+        * for host 0&  2 should be set for host 1&  3 also */
+       if (reg_offset == APMU_SDH0 || reg_offset == APMU_SDH1)
+               __raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
+       if (reg_offset == APMU_PXA168_SDH2 || reg_offset == APMU_PXA168_SDH3)
+               __raw_writel(__raw_readl(APMU_PXA168_SDH2) | 0x9,
+                               APMU_PXA168_SDH2);
+
+       __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
+}
+

How about use the below code instead? I think we can avoid to check register offset.

static void sdh0_clk_enable(struct clk *clk)

{
        void __iomem *reg_offset = clk->clk_rst;

        __raw_writel(__raw_readl(APMU_SDH0) | 0x9, APMU_SDH0);
        __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);
}

static void sdh2_clk_enable(struct clk *clk)

{
        void __iomem *reg_offset = clk->clk_rst;
                __raw_writel(__raw_readl(APMU_PXA168_SDH2) | 0x9,
                                APMU_PXA168_SDH2);
        __raw_writel(__raw_readl(clk->clk_rst) | clk->enable_val, clk->clk_rst);

}

struct clkops sdh0_clk_ops = {
.enable = sdh0_clk_enable,
.disable = sdh_clk_disable,
};

struct clkops sdh2_clk_ops = {
.enable = sdh2_clk_enable,
.disable = sdh_clk_disable,
};

Quoted 5 lines

static APMU_CLK_OPS(sdh1, SDH0, 0x12, 48000000,&sdh0_clk_ops); static APMU_CLK_OPS(sdh2, SDH1, 0x12, 48000000,&sdh0_clk_ops); static APMU_CLK_OPS(sdh3, PXA168_SDH2, 0x12, 48000000,&sdh2_clk_ops); static APMU_CLK_OPS(sdh4, PXA168_SDH3, 0x12, 48000000,&sdh2_clk_ops);

Sound good! :) Will change accordingly & submit patch.

Thanks,

Tanmay

Quoted 51 lines

arch/arm/mach-mmp/pxa168.c +24 -1

+static void sdh_clk_disable(struct clk *clk)
+{
+       __raw_writel(__raw_readl(clk->clk_rst)&  ~(clk->enable_val),
+                       clk->clk_rst);
+}
+
+struct clkops sdh_clk_ops = {
+       .enable         = sdh_clk_enable,
+       .disable        = sdh_clk_disable,
+};
+
  /* APB peripheral clocks */
  static APBC_CLK(uart1, PXA168_UART1, 1, 14745600);
  static APBC_CLK(uart2, PXA168_UART2, 1, 14745600);

@@ -87,6 +113,11 @@ static APMU_CLK(lcd, LCD, 0x7f, 312000000);
  static APMU_CLK(eth, ETH, 0x09, 0);
  static APMU_CLK(usb, USB, 0x12, 0);
 
+static APMU_CLK_OPS(sdh1, SDH0, 0x12, 48000000,&sdh_clk_ops);
+static APMU_CLK_OPS(sdh2, SDH1, 0x12, 48000000,&sdh_clk_ops);
+static APMU_CLK_OPS(sdh3, PXA168_SDH2, 0x12, 48000000,&sdh_clk_ops);
+static APMU_CLK_OPS(sdh4, PXA168_SDH3, 0x12, 48000000,&sdh_clk_ops);
+
  /* device and clock bindings */
  static struct clk_lookup pxa168_clkregs[] = {
        INIT_CLKREG(&clk_uart1, "pxa2xx-uart.0", NULL),

@@ -108,6 +139,10 @@ static struct clk_lookup pxa168_clkregs[] = {
        INIT_CLKREG(&clk_keypad, "pxa27x-keypad", NULL),
        INIT_CLKREG(&clk_eth, "pxa168-eth", "MFUCLK"),
        INIT_CLKREG(&clk_usb, "pxa168-ehci", "PXA168-USBCLK"),
+       INIT_CLKREG(&clk_sdh1, NULL, "PXA-SDHCLK"),
+       INIT_CLKREG(&clk_sdh2, NULL, "PXA-SDHCLK"),
+       INIT_CLKREG(&clk_sdh3, NULL, "PXA-SDHCLK"),
+       INIT_CLKREG(&clk_sdh4, NULL, "PXA-SDHCLK"),
  };
 
  static int __init pxa168_init(void)

@@ -173,6 +208,10 @@ PXA168_DEVICE(ssp5, "pxa168-ssp", 4, SSP5, 0xd4021000, 0x40, 60, 61);
  PXA168_DEVICE(fb, "pxa168-fb", -1, LCD, 0xd420b000, 0x1c8);
  PXA168_DEVICE(keypad, "pxa27x-keypad", -1, KEYPAD, 0xd4012000, 0x4c);
  PXA168_DEVICE(eth, "pxa168-eth", -1, MFU, 0xc0800000, 0x0fff);
+PXA168_DEVICE(sdh1, "sdhci-pxav2", 0, SDH1, 0xd4280000, 0x100);
+PXA168_DEVICE(sdh2, "sdhci-pxav2", 1, SDH1, 0xd4281000, 0x100);
+PXA168_DEVICE(sdh3, "sdhci-pxav2", 2, SDH2, 0xd427e000, 0x100);
+PXA168_DEVICE(sdh4, "sdhci-pxav2", 3, SDH2, 0xd427f000, 0x100);
 
  struct resource pxa168_usb_host_resources[] = {
        /* USB Host conroller register base */
--

1.7.0.4

Russell King - ARM Linux

2011-12-20 21:03 UTC #16 lore raw

PATCH v4 2/4 ARM: pxa168: Add SDHCI support

In reply to Tanmay Upadhyay

Just two minor style issues in this patch.

arch/arm/mach-mmp/include/mach/pxa168.h static inline int pxa168_add_eth(struct pxa168_eth_platform_data *data) · line 149

Quoted 9 lines · Tanmay Upadhyay

arch/arm/mach-mmp/include/mach/pxa168.h +9 -0

+static inline int pxa168_add_sdh(int id, struct sdhci_pxa_platdata *data)
+{
+	struct pxa_device_desc *d = NULL;
+
+	switch (id) {
+	case 1: d = &pxa168_device_sdh1; break;
+	case 2: d = &pxa168_device_sdh2; break;
+	case 3: d = &pxa168_device_sdh3; break;
+	case 4: d = &pxa168_device_sdh4; break;

Style.

	case X:
		d = &y;
		break;

arch/arm/mach-mmp/pxa168.c void __init pxa168_init_irq(void) · line 85

Quoted 3 lines

arch/arm/mach-mmp/pxa168.c +3 -0

+static void sdh_clk_disable(struct clk *clk)
+{
+	__raw_writel(__raw_readl(clk->clk_rst) & ~(clk->enable_val),

Parens around clk->enable_val not required.

arch/arm/mach-mmp/pxa168.c static struct clk_lookup pxa168_clkregs[] = { · line 145

Quoted 4 lines

arch/arm/mach-mmp/pxa168.c +4 -0

+	INIT_CLKREG(&clk_sdh1, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh2, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh3, NULL, "PXA-SDHCLK"),
+	INIT_CLKREG(&clk_sdh4, NULL, "PXA-SDHCLK"),

So these clocks are registered with the same name? How are they distinguished by clk_get()?

Haojian Zhuang

2011-12-21 01:48 UTC #17 lore raw

PATCH v4 2/4 ARM: pxa168: Add SDHCI support

Quoted 24 lines · Russell King - ARM Linux

Just two minor style issues in this patch.
Review
Quoted 9 lines · Tanmay Upadhyay
(quoted diff) +9 -0
+static inline int pxa168_add_sdh(int id, struct sdhci_pxa_platdata *data)
+{
+ ? ? struct pxa_device_desc *d = NULL;
+
+ ? ? switch (id) {
+ ? ? case 1: d = &pxa168_device_sdh1; break;
+ ? ? case 2: d = &pxa168_device_sdh2; break;
+ ? ? case 3: d = &pxa168_device_sdh3; break;
+ ? ? case 4: d = &pxa168_device_sdh4; break;
Style.
? ? ? ?case X:
? ? ? ? ? ? ? ?d = &y;
? ? ? ? ? ? ? ?break;
arch/arm/mach-mmp/pxa168.c void __init pxa168_init_irq(void)
Quoted 3 lines
arch/arm/mach-mmp/pxa168.c +3 -0
+static void sdh_clk_disable(struct clk *clk)
+{
+ ? ? __raw_writel(__raw_readl(clk->clk_rst) & ~(clk->enable_val),
Parens around clk->enable_val not required.

Quoted 8 lines

Quoted 4 lines
+ ? ? INIT_CLKREG(&clk_sdh1, NULL, "PXA-SDHCLK"),
+ ? ? INIT_CLKREG(&clk_sdh2, NULL, "PXA-SDHCLK"),
+ ? ? INIT_CLKREG(&clk_sdh3, NULL, "PXA-SDHCLK"),
+ ? ? INIT_CLKREG(&clk_sdh4, NULL, "PXA-SDHCLK"),
So these clocks are registered with the same name? ?How are they distinguished by clk_get()?

Yes, it's better to use devname.

Tanmay Upadhyay

author

2011-12-29 13:43 UTC #18 lore raw

PATCH 2/4 ARM: pxa168: Add SDHCI support

v2 - clock register for SDHCI are not common across all MMP SoCs.
So, move PXA168 implementation to pxa168.c

v3 - sdhci-pxav1 driver code is merged with sdhci-pxav2. So, change
the device name accordingly
- start sdhci device numbering from 1 as other PXA168 devices does that

v4 - Use different names for SD clock registers for PXA168 instead
of redefining them in pxa168.c. Suggested by Haojian Zhuang

v5 - Have two different clock enable functions for clock block 1 & 2
& don't change indentation in regs-apmu.h as suggested by Haojian Zhuang
- Use device name while adding clock as suggested by Russell King

Signed-off-by Tanmay Upadhyay <[email protected]>

Reviewed-by Philip Rakity <[email protected]>

---
 arch/arm/mach-mmp/include/mach/pxa168.h    |   20 ++++++++++++
 arch/arm/mach-mmp/include/mach/regs-apmu.h |    2 +
 arch/arm/mach-mmp/pxa168.c                 |   47 ++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 0 deletions(-)

arch/arm/mach-mmp/include/mach/pxa168.h +20 -0

index 7fb568d..a181608 100644
@@ -15,6 +15,7 @@ extern void pxa168_clear_keypad_wakeup(void);
 #include <plat/pxa27x_keypad.h>
 #include <mach/cputype.h>
 #include <linux/pxa168_eth.h>
+#include <linux/platform_data/pxa_sdhci.h>
 
 extern struct pxa_device_desc pxa168_device_uart1;
 extern struct pxa_device_desc pxa168_device_uart2;

@@ -34,6 +35,10 @@ extern struct pxa_device_desc pxa168_device_nand;
 extern struct pxa_device_desc pxa168_device_fb;
 extern struct pxa_device_desc pxa168_device_keypad;
 extern struct pxa_device_desc pxa168_device_eth;
+extern struct pxa_device_desc pxa168_device_sdh1;
+extern struct pxa_device_desc pxa168_device_sdh2;
+extern struct pxa_device_desc pxa168_device_sdh3;
+extern struct pxa_device_desc pxa168_device_sdh4;
 
 struct pxa168_usb_pdata {
 	/* If NULL, default phy init routine for PXA168 would be called */

@@ -132,4 +137,19 @@ static inline int pxa168_add_eth(struct pxa168_eth_platform_data *data)
 {
 	return pxa_register_device(&pxa168_device_eth, data, sizeof(*data));
 }
+
+static inline int pxa168_add_sdh(int id, struct sdhci_pxa_platdata *data)
+{
+	struct pxa_device_desc *d = NULL;
+
+	switch (id) {
+	case 1: d = &pxa168_device_sdh1; break;
+	case 2: d = &pxa168_device_sdh2; break;
+	case 3: d = &pxa168_device_sdh3; break;
+	case 4: d = &pxa168_device_sdh4; break;
+	default:
+		return -EINVAL;
+	}
+	return pxa_register_device(d, data, sizeof(*data));
+}
 #endif /* __ASM_MACH_PXA168_H */

arch/arm/mach-mmp/include/mach/regs-apmu.h +2 -0

index 8447ac6..db55618 100644
@@ -27,6 +27,8 @@
 #define APMU_DMA	APMU_REG(0x064)
 #define APMU_GEU	APMU_REG(0x068)
 #define APMU_BUS	APMU_REG(0x06c)
+#define APMU_PXA168_SDH2	APMU_REG(0x0e0)
+#define APMU_PXA168_SDH3	APMU_REG(0x0e4)
 #define APMU_SDH2	APMU_REG(0x0e8)
 #define APMU_SDH3	APMU_REG(0x0ec)
 #define APMU_ETH	APMU_REG(0x0fc)

arch/arm/mach-mmp/pxa168.c +47 -0

71 lines not on this page · raw message

Signature · 1 line

1.7.0.4

Haojian Zhuang

2011-12-30 00:36 UTC #19 lore raw

PATCH 2/4 ARM: pxa168: Add SDHCI support

Quoted 11 lines · Tanmay Upadhyay

v2 - clock register for SDHCI are not common across all MMP SoCs. ? ? So, move PXA168 implementation to pxa168.c
v3 - sdhci-pxav1 driver code is merged with sdhci-pxav2. So, change ? ? the device name accordingly
? - start sdhci device numbering from 1 as other PXA168 devices ? ? does that
v4 - Use different names for SD clock registers for PXA168 instead ? ? of redefining them in pxa168.c. Suggested by Haojian Zhuang

Quoted 8 lines

v5 - Have two different clock enable functions for clock block 1 & 2 ? ? & don't change indentation in regs-apmu.h as suggested by Haojian ? ? Zhuang
? - Use device name while adding clock as suggested by Russell King
Signed-off-by Tanmay Upadhyay <[email protected]>
Reviewed-by Philip Rakity <[email protected]>
---

Acked-by Haojian Zhuang <[email protected]>

Tanmay Upadhyay

author

2012-01-23 22:45 UTC #20 lore raw

PATCH 2/4 ARM: pxa168: Add SDHCI support

Quoted 18 lines · Haojian Zhuang

Quoted 16 lines · Tanmay Upadhyay
v2 - clock register for SDHCI are not common across all MMP SoCs.
So, move PXA168 implementation to pxa168.c
v3 - sdhci-pxav1 driver code is merged with sdhci-pxav2. So, change
the device name accordingly
- start sdhci device numbering from 1 as other PXA168 devices does that
v4 - Use different names for SD clock registers for PXA168 instead
of redefining them in pxa168.c. Suggested by Haojian Zhuang
v5 - Have two different clock enable functions for clock block 1& 2
& don't change indentation in regs-apmu.h as suggested by Haojian Zhuang
- Use device name while adding clock as suggested by Russell King

Quoted 4 lines

Quoted 3 lines
Signed-off-by Tanmay Upadhyay<[email protected]>
Reviewed-by Philip Rakity<[email protected]>
---
Acked-by Haojian Zhuang<[email protected]>

Hi Eric, Jason,

Does this look good to you?

Thanks,

Tanmay

Philip Rakity

2011-12-19 05:24 UTC #21 lore raw

PATCH v3 2/4 ARM: pxa168: Add SDHCI support

In reply to Haojian Zhuang

Haojian,

Should the APMU definitions be moved to a separate include for pxa168 and say given the name pxa168_apmu.h ?

Same comment applies to all the register set since they are not really common.

Philip

Quoted 52 lines · Haojian Zhuang

Quoted 37 lines · Tanmay Upadhyay
Quoted 20 lines · Haojian Zhuang
Quoted 7 lines · Chris Ball
Hi Eric and Jason,
Quoted 3 lines · Chris Ball
Hi Eric, Jason,
Quoted 9 lines
Quoted 2 lines
Please could you ACK this patch if you agree with it, and I'll take it and the rest of the series via the MMC tree? Thanks.
Ping?
Thanks,
- Chris.
NACK.
Quoted 13 lines
Quoted 9 lines
Quoted 9 lines
arch/arm/mach-mmp/pxa168.c static struct mfp_addr_map pxa168_mfp_addr_map[] __initdata = · line 55
Quoted 9 lines
+/* Offset defined in arch/arm/mach-mmp/include/mach/regs-apmu.h are for MMP2
+ * PXA168 has different offset */
arch/arm/mach-mmp/pxa168.c +6 -0
+#undef APMU_SDH2
+#undef APMU_SDH3
+
+#define APMU_SDH2   APMU_REG(0xe0)
+#define APMU_SDH3   APMU_REG(0xe4)
+
Please don't use #undef at here. If the register setting is different, I prefer to use two different clk operations.
Quoted 8 lines
Sorry I couldn't get you. Could you please elaborate? Here regs-apmu.h defines clock register offsets. They are correct for MMP2, but not for PXA168. So I thought to correct them in a PXA168 specific file. Could you please point me a better way?
Thanks,
Tanmay
APMU_PXA168_SDH2
APMU_MMP2_SDH2
I think this is better.

Haojian Zhuang

2011-12-19 05:30 UTC #22 lore raw

PATCH v3 2/4 ARM: pxa168: Add SDHCI support

Quoted 8 lines · Philip Rakity

Haojian,
Should the APMU definitions be moved to a separate include for pxa168 and say given the name pxa168_apmu.h ?
Same comment applies to all the register set since they are not really common.
Philip

I think it's unnecessary to create a new apmu.h. We can distinguish them by name in macro, as PXA168/MMP2.

Thanks
Haojian

Tanmay Upadhyay

author

2011-11-21 04:26 UTC #23 lore raw

PATCH v2 3/4 mmc: sdhci-pxa: Add SDHCI driver for PXA16x

In reply to Chris Ball

PXA16x devices uses SDHCI controller v1. As it's not much different than v2 controller, v1 driver is merged with sdhci-pxav2 driver

v2 - instead of having separate file sdhci-pxav1, merge code with
sdhci-pxav2 driver code as suggested by Chris Ball

Signed-off-by Philip Rakity <[email protected]>

Signed-off-by Tanmay Upadhyay <[email protected]>

---
 drivers/mmc/host/Kconfig                |    7 ++++---
 drivers/mmc/host/sdhci-pxav2.c          |   30 +++++++++++++++++++++++++++++-
 drivers/mmc/host/sdhci.c                |    3 +++
 drivers/mmc/host/sdhci.h                |    1 +
 include/linux/platform_data/pxa_sdhci.h |    2 ++
 5 files changed, 39 insertions(+), 4 deletions(-)

drivers/mmc/host/Kconfig +4 -3

index 21b4149..77541f5 100644
@@ -195,14 +195,15 @@ config MMC_SDHCI_PXAV3
 	  If unsure, say N.
 
 config MMC_SDHCI_PXAV2
-	tristate "Marvell PXA9XX SD Host Controller support (PXAV2)"
+	tristate "Marvell PXA16X/PXA9XX SD Host Controller support (PXAV1/V2)"
 	depends on CLKDEV_LOOKUP
 	select MMC_SDHCI
 	select MMC_SDHCI_PLTFM
 	default y if CPU_PXA910
+	default y if CPU_PXA168
 	help
-	  This selects the Marvell(R) PXAV2 SD Host Controller.
-	  If you have a PXA9XX platform with SD Host Controller
+	  This selects the Marvell(R) PXAV1/V2 SD Host Controller.
+	  If you have a PXA16X or PXA9XX platform with SD Host Controller
 	  and a card slot, say Y or M here.
 
 	  If unsure, say N.

drivers/mmc/host/sdhci-pxav2.c +29 -1

index d4bf6d3..633cc77 100644
@@ -28,6 +28,7 @@
 #include <linux/mmc/host.h>
 #include <linux/platform_data/pxa_sdhci.h>
 #include <linux/slab.h>
+#include <linux/delay.h>
 #include "sdhci.h"
 #include "sdhci-pltfm.h"

@@ -72,7 +73,13 @@ static void pxav2_set_private_registers(struct sdhci_host *host, u8 mask)
 			writew(tmp, host->ioaddr + SD_CLOCK_BURST_SIZE_SETUP);
 		}
 
-		if (pdata && (pdata->flags & PXA_FLAG_ENABLE_CLOCK_GATING)) {
+		if (pdata && pdata->pxav1_controller) {
+			/* no clock gating */
+			tmp = readw(host->ioaddr + SD_FIFO_PARAM);
+			tmp |= DIS_PAD_SD_CLK_GATE;
+			writew(tmp, host->ioaddr + SD_FIFO_PARAM);
+		} else if (pdata && (pdata->flags
+				& PXA_FLAG_ENABLE_CLOCK_GATING)) {
 			tmp = readw(host->ioaddr + SD_FIFO_PARAM);
 			tmp &= ~CLK_GATE_SETTING_BITS;
 			writew(tmp, host->ioaddr + SD_FIFO_PARAM);

@@ -115,6 +122,20 @@ static u32 pxav2_get_max_clock(struct sdhci_host *host)
 	return clk_get_rate(pltfm_host->clk);
 }
 
+/*
+ * we cannot talk to controller for 8 bus cycles according to sdio spec
+ * at lowest speed this is 100,000 HZ per cycle or 800,000 cycles
+ * which is quite a LONG TIME on a fast cpu -- so delay if needed
+ */
+static void platform_specific_completion(struct sdhci_host *host)
+{
+	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
+	struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
+
+	if (host->clock < 3200000 && pdata && pdata->delay_in_ms)
+		mdelay(pdata->delay_in_ms);
+}
+
 static struct sdhci_ops pxav2_sdhci_ops = {
 	.get_max_clock = pxav2_get_max_clock,
 	.platform_reset_exit = pxav2_set_private_registers,

@@ -167,6 +188,13 @@ static int __devinit sdhci_pxav2_probe(struct platform_device *pdev)
 		if (pdata->flags & PXA_FLAG_SD_8_BIT_CAPABLE_SLOT)
 			host->mmc->caps |= MMC_CAP_8_BIT_DATA;
 
+		if (pdata->pxav1_controller) {
+			host->quirks |=	SDHCI_QUIRK_NO_BUSY_IRQ
+					| SDHCI_QUIRK_32BIT_DMA_SIZE;
+			pxav2_sdhci_ops.platform_specific_completion
+				= platform_specific_completion;
+		}
+
 		if (pdata->quirks)
 			host->quirks |= pdata->quirks;
 		if (pdata->host_caps)

drivers/mmc/host/sdhci.c +3 -0

index 6d8eea3..c6b52b8 100644
@@ -983,6 +983,9 @@ static void sdhci_send_command(struct sdhci_host *host, struct mmc_command *cmd)
 		mdelay(1);
 	}
 
+	if (host->ops->platform_specific_completion)
+		host->ops->platform_specific_completion(host);
+
 	mod_timer(&host->timer, jiffies + 10 * HZ);
 
 	host->cmd = cmd;

drivers/mmc/host/sdhci.h +1 -0

index 0a5b654..4ba3244 100644
@@ -274,6 +274,7 @@ struct sdhci_ops {
 	void	(*platform_reset_exit)(struct sdhci_host *host, u8 mask);
 	int	(*set_uhs_signaling)(struct sdhci_host *host, unsigned int uhs);
 	void	(*hw_reset)(struct sdhci_host *host);
+	void	(*platform_specific_completion)(struct sdhci_host *host);
 };
 
 #ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS

include/linux/platform_data/pxa_sdhci.h +2 -0

index 51ad099..99bf54d 100644
@@ -51,6 +51,8 @@ struct sdhci_pxa_platdata {
 	unsigned int	host_caps;
 	unsigned int	quirks;
 	unsigned int	pm_caps;
+	bool		pxav1_controller;  /* set if pxa168 */
+	unsigned int	delay_in_ms;
 };
 
 struct sdhci_pxa {

Signature · 1 line

1.7.0.4

A message that is not in the archive (sent off-list or not yet received): CAMj5Bki3Ev8K63538-dNbyijUw1oNTLChADLd4=Ef-LeCkoacw@mail.gmail.com

Philip Rakity

2011-11-25 18:53 UTC #25 lore raw

PATCH v2 3/4 mmc: sdhci-pxa: Add SDHCI driver for PXA16x

Quoted 18 lines · zhangfei gao

On Mon, Nov 21, 2011 at 12:26 PM, Tanmay Upadhyay <[email protected]> wrote:
PXA16x devices uses SDHCI controller v1. As it's not much different
than v2 controller, v1 driver is merged with sdhci-pxav2 driver
v2 - instead of having separate file sdhci-pxav1, merge code with
sdhci-pxav2 driver code as suggested by Chris Ball
Signed-off-by Philip Rakity <[email protected]>
Signed-off-by Tanmay Upadhyay <[email protected]>
---
+/*
+ * we cannot talk to controller for 8 bus cycles according to sdio spec
+ * at lowest speed this is 100,000 HZ per cycle or 800,000 cycles
+ * which is quite a LONG TIME on a fast cpu -- so delay if needed
+ */

Quoted 3 lines

Would you mind provide more info, still not understand.
What sdio spec 12.1 says cards shall switch speed mode within 8 clocks after the end bit of the corresponding response.

The pxa168 h/w controller is broken. It does NOT wait for the 8 clocks.

Quoted 10 lines

drivers/mmc/host/sdhci-pxav2.c +9 -0

+static void platform_specific_completion(struct sdhci_host *host)
+{
+       struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
+       struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
+
+       if (host->clock < 3200000 && pdata && pdata->delay_in_ms)
+               mdelay(pdata->delay_in_ms);
+}
+

Tanmay Upadhyay

author

2011-11-21 04:27 UTC #26 lore raw

PATCH v2 4/4 ARM: pxa168/gplugd: Add support for SD port 1

In reply to Chris Ball

v2 - after sdhci-pxav1 driver is merged with sdhci-pxav2, pass
pxav1_controller = 1
- as sdhci device numbering now starts from 1, call pxa168_add_sdh accordingly

Signed-off-by Tanmay Upadhyay <[email protected]>

Reviewed-by Philip Rakity <[email protected]>

---
 arch/arm/mach-mmp/gplugd.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

arch/arm/mach-mmp/gplugd.c +6 -0

index 6915656..d7947e7 100644
@@ -154,6 +154,11 @@ struct pxa168_eth_platform_data gplugd_eth_platform_data = {
 	.init        = gplugd_eth_init,
 };
 
+struct sdhci_pxa_platdata gplugd_sdh_platdata = {
+	.delay_in_ms = 5,
+	.pxav1_controller = 1,
+};
+
 static void __init select_disp_freq(void)
 {
 	/* set GPIO 35 & clear GPIO 85 to set LCD External Clock to 74.25 MHz */

@@ -186,6 +191,7 @@ static void __init gplugd_init(void)
 	pxa168_add_twsi(0, NULL, ARRAY_AND_SIZE(gplugd_i2c_board_info));
 
 	pxa168_add_eth(&gplugd_eth_platform_data);
+	pxa168_add_sdh(1, &gplugd_sdh_platdata);
 }
 
 MACHINE_START(GPLUGD, "PXA168-based GuruPlug Display (gplugD) Platform")

Signature · 1 line

1.7.0.4