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

On Mon, Dec 19, 2011 at 1:17 PM, Tanmay Upadhyay
[off-list ref] wrote:

On Monday 19 December 2011 10:32 AM, Haojian Zhuang wrote:

quoted

On Mon, Dec 19, 2011 at 12:55 PM, Tanmay Upadhyay
[off-list ref] ?wrote:

quoted

On Monday 19 December 2011 10:15 AM, Haojian Zhuang wrote:

quoted

On Fri, Dec 16, 2011 at 7:15 AM, Chris Ball[off-list ref] ? ?wrote:

quoted

Hi Eric and Jason,

On Thu, Dec 01 2011, Chris Ball wrote:

quoted

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.

NACK.

quoted

quoted

quoted

+/* 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

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

+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.