[PATCH v3 0/2] Add support for the rockchip mmc clock phases using the framework

[PATCH v3 0/2] Add support for the rockchip mmc clock phases using the framework

[Alexandru M Stan]

For now all I have is the getter and setter for the phase, nothing that uses it
(that is ready). You can test the getter like this:
localhost ~ # cat /sys/kernel/debug/clk/clk_summary|grep sample -C 1
    sclk_sdio1                            0            0    24000000          0 0
       sdio1_sample                       0            0    12000000          0 0
       sdio1_drv                          0            0    12000000          0 90
--
          sclk_sdmmc                      1            1   297000000          0 0
             sdmmc_sample                 0            0   148500000          0 134
             sdmmc_drv                    0            0   148500000          0 90
--
          sclk_sdio0                      1            1   100000000          0 0
             sdio0_sample                 0            0    50000000          0 0
             sdio0_drv                    0            0    50000000          0 90
          sclk_emmc                       1            1   100000000          0 0
             emmc_sample                  0            0    50000000          0 0
             emmc_drv                     0            0    50000000          0 180

Next thing that will come is some dts changes that will make use of these new
clocks, and eventually mmc code will be changed to tune with these clocks.

Changes in v3:
- renamed everything internal from phase to just mmc_clock or mmc
- added RK3288_MMC_CLKGEN_DIV instead of the magic number
- added new paragraph to commit message

Changes in v2:
- fixed my cc/to list
- removed dangling #DEFINE DEBUG

Alexandru M Stan (2):
  clk: rockchip: add bindings for the mmc clocks
  clk: rockchip: Add support for the mmc clock phases using the
    framework

 drivers/clk/rockchip/Makefile          |   1 +
 drivers/clk/rockchip/clk-mmc-phase.c   | 151 +++++++++++++++++++++++++++++++++
 drivers/clk/rockchip/clk-rk3288.c      |  12 +++
 drivers/clk/rockchip/clk.c             |   8 ++
 drivers/clk/rockchip/clk.h             |  23 +++++
 include/dt-bindings/clock/rk3288-cru.h |  10 +++
 6 files changed, 205 insertions(+)
 create mode 100644 drivers/clk/rockchip/clk-mmc-phase.c

-- 
2.1.0.rc2.206.gedb03e5

[PATCH v3 1/2] clk: rockchip: add bindings for the mmc clocks

[Alexandru M Stan]

These clocks represent the physical clocks (including phases) and they will
later be used for clock phase tuning.

Suggested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Alexandru M Stan <amstan@chromium.org>
---
Changes in v3: None
Changes in v2: None

 include/dt-bindings/clock/rk3288-cru.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/dt-bindings/clock/rk3288-cru.h b/include/dt-bindings/clock/rk3288-cru.h
index 100a08c..b4e9142 100644
--- a/include/dt-bindings/clock/rk3288-cru.h
+++ b/include/dt-bindings/clock/rk3288-cru.h
@@ -72,6 +72,16 @@
 #define SCLK_HEVC_CABAC		111
 #define SCLK_HEVC_CORE		112
 
+#define SCLK_SDMMC_DRV		113
+#define SCLK_SDIO0_DRV		114
+#define SCLK_SDIO1_DRV		115
+#define SCLK_EMMC_DRV		116
+
+#define SCLK_SDMMC_SAMPLE	117
+#define SCLK_SDIO0_SAMPLE	118
+#define SCLK_SDIO1_SAMPLE	119
+#define SCLK_EMMC_SAMPLE	120
+
 #define DCLK_VOP0		190
 #define DCLK_VOP1		191
 
-- 
2.1.0.rc2.206.gedb03e5

[PATCH v3 2/2] clk: rockchip: Add support for the mmc clock phases using the framework

[Alexandru M Stan]

This patch adds the 2 physical clocks for the mmc (drive and sample). They're
mostly there for the phase properties, but they also show the true clock
(by dividing by RK3288_MMC_CLKGEN_DIV).

The drive and sample phases are generated by dividing an upstream parent clock
by 2, this allows us to adjust the phase by 90 deg.

There's also an option to have up to 255 delay elements (40-80 picoseconds long).
This driver uses those elements (under the assumption that they're 60ps long) to
generate a rough 45deg (which might be from 33deg to 66deg) setting.

Suggested-by: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Alexandru M Stan <amstan@chromium.org>
---
Changes in v3:
- renamed everything internal from phase to just mmc_clock or mmc
- added RK3288_MMC_CLKGEN_DIV instead of the magic number
- added new paragraph to commit message

Changes in v2:
- fixed my cc/to list
- removed dangling #DEFINE DEBUG

 drivers/clk/rockchip/Makefile        |   1 +
 drivers/clk/rockchip/clk-mmc-phase.c | 151 +++++++++++++++++++++++++++++++++++
 drivers/clk/rockchip/clk-rk3288.c    |  12 +++
 drivers/clk/rockchip/clk.c           |   8 ++
 drivers/clk/rockchip/clk.h           |  23 ++++++
 5 files changed, 195 insertions(+)
 create mode 100644 drivers/clk/rockchip/clk-mmc-phase.c

diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
index bd8514d..2714097 100644
--- a/drivers/clk/rockchip/Makefile
+++ b/drivers/clk/rockchip/Makefile
@@ -6,6 +6,7 @@ obj-y	+= clk-rockchip.o
 obj-y	+= clk.o
 obj-y	+= clk-pll.o
 obj-y	+= clk-cpu.o
+obj-y	+= clk-mmc-phase.o
 obj-$(CONFIG_RESET_CONTROLLER)	+= softrst.o
 
 obj-y	+= clk-rk3188.o
diff --git a/drivers/clk/rockchip/clk-mmc-phase.c b/drivers/clk/rockchip/clk-mmc-phase.c
new file mode 100644
index 0000000..9da4fd6
--- /dev/null
+++ b/drivers/clk/rockchip/clk-mmc-phase.c
@@ -0,0 +1,151 @@
+/*
+ * Copyright 2014 Google, Inc
+ * Author: Alexandru M Stan <amstan@chromium.org>
+ *
+ * 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, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/slab.h>
+#include <linux/clk-provider.h>
+#include "clk.h"
+
+struct rockchip_mmc_clock {
+	struct clk_hw	hw;
+	void __iomem	*reg;
+	int		id;
+	int		shift;
+};
+
+#define to_mmc_clock(_hw) container_of(_hw, struct rockchip_mmc_clock, hw)
+
+#define RK3288_MMC_CLKGEN_DIV 2
+
+static unsigned long rockchip_mmc_recalc(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	return parent_rate / RK3288_MMC_CLKGEN_DIV;
+}
+
+#define ROCKCHIP_MMC_DELAY_SEL BIT(10)
+#define ROCKCHIP_MMC_DEGREE_MASK 0x3
+#define ROCKCHIP_MMC_DELAYNUM_OFFSET 2
+#define ROCKCHIP_MMC_DELAYNUM_MASK (0xff << ROCKCHIP_MMC_DELAYNUM_OFFSET)
+
+#define PSECS_PER_SEC 1000000000000LL
+
+/*
+ * Each fine delay is between 40ps-80ps. Assume each fine delay is 60ps to
+ * simplify calculations. So 45degs could be anywhere between 33deg and 66deg.
+ */
+#define DELAY_ELEMENT_PSEC 60
+
+static int rockchip_mmc_get_phase(struct clk_hw *hw)
+{
+	struct rockchip_mmc_clock *mmc_clock = to_mmc_clock(hw);
+	unsigned long rate = clk_get_rate(hw->clk);
+	u32 raw_value;
+	u16 degrees;
+	u8 delay_num = 0;
+
+	raw_value = readl(mmc_clock->reg) >> (mmc_clock->shift);
+
+	degrees = (raw_value & ROCKCHIP_MMC_DEGREE_MASK) * 90;
+
+	if (raw_value & ROCKCHIP_MMC_DELAY_SEL) {
+		/* degrees/delaynum * 10000 */
+		unsigned long factor = (DELAY_ELEMENT_PSEC / 10) * 36 *
+					(rate / 1000000);
+
+		delay_num = (raw_value & ROCKCHIP_MMC_DELAYNUM_MASK);
+		delay_num >>= ROCKCHIP_MMC_DELAYNUM_OFFSET;
+		degrees += delay_num * factor / 10000;
+	}
+
+	return degrees % 360;
+}
+
+static int rockchip_mmc_set_phase(struct clk_hw *hw, int degrees)
+{
+	struct rockchip_mmc_clock *mmc_clock = to_mmc_clock(hw);
+	unsigned long rate = clk_get_rate(hw->clk);
+	u8 nineties, is_fortyfive;
+	u8 delay_num = 0;
+	u32 raw_value;
+
+	degrees -= degrees % 45;
+
+	nineties = degrees / 90;
+	is_fortyfive = (degrees % 90) == 45;
+
+	if (is_fortyfive) {
+		u64 delay;
+
+		delay = PSECS_PER_SEC;
+		do_div(delay, rate);
+		do_div(delay, 360 / 45);
+		do_div(delay, DELAY_ELEMENT_PSEC);
+
+		delay_num = (u8) min(delay, 255ULL);
+	}
+
+	raw_value = ROCKCHIP_MMC_DELAY_SEL;
+	raw_value |= delay_num << ROCKCHIP_MMC_DELAYNUM_OFFSET;
+	raw_value |= nineties;
+	writel(HIWORD_UPDATE(raw_value, 0xffff, mmc_clock->shift), mmc_clock->reg);
+
+	pr_debug("%s->set_phase(%d) delay_nums=%u reg[0x%p]=0x%x actual_degrees=%d\n",
+		__clk_get_name(hw->clk), degrees, delay_num,
+		mmc_clock->reg, raw_value>>(mmc_clock->shift),
+		rockchip_mmc_get_phase(hw)
+	);
+
+	return 0;
+}
+
+static const struct clk_ops rockchip_mmc_clk_ops = {
+	.recalc_rate	= rockchip_mmc_recalc,
+	.get_phase	= rockchip_mmc_get_phase,
+	.set_phase	= rockchip_mmc_set_phase,
+};
+
+struct clk *rockchip_clk_register_mmc(const char *name,
+				const char **parent_names, u8 num_parents,
+				void __iomem *reg, int shift)
+{
+	struct clk_init_data init;
+	struct rockchip_mmc_clock *mmc_clock;
+	struct clk *clk;
+
+	mmc_clock = kmalloc(sizeof(*mmc_clock), GFP_KERNEL);
+	if (!mmc_clock)
+		return NULL;
+
+	init.num_parents = num_parents;
+	init.parent_names = parent_names;
+	init.ops = &rockchip_mmc_clk_ops;
+
+	mmc_clock->hw.init = &init;
+	mmc_clock->reg = reg;
+	mmc_clock->shift = shift;
+
+	if (name)
+		init.name = name;
+
+	clk = clk_register(NULL, &mmc_clock->hw);
+	if (IS_ERR(clk))
+		goto err_free;
+
+	return clk;
+
+err_free:
+	kfree(mmc_clock);
+	return NULL;
+}
diff --git a/drivers/clk/rockchip/clk-rk3288.c b/drivers/clk/rockchip/clk-rk3288.c
index 2327829..f415f84 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -483,6 +483,18 @@ static struct rockchip_clk_branch rk3288_clk_branches[] __initdata = {
 			RK3288_CLKSEL_CON(12), 14, 2, MFLAGS, 8, 6, DFLAGS,
 			RK3288_CLKGATE_CON(13), 3, GFLAGS),
 
+	MMC(SCLK_SDMMC_DRV,    "sdmmc_drv",    "sclk_sdmmc", RK3288_SDMMC_CON0, 1),
+	MMC(SCLK_SDMMC_SAMPLE, "sdmmc_sample", "sclk_sdmmc", RK3288_SDMMC_CON1, 0),
+
+	MMC(SCLK_SDIO0_DRV,    "sdio0_drv",    "sclk_sdio0", RK3288_SDIO0_CON0, 1),
+	MMC(SCLK_SDIO0_SAMPLE, "sdio0_sample", "sclk_sdio0", RK3288_SDIO0_CON1, 0),
+
+	MMC(SCLK_SDIO1_DRV,    "sdio1_drv",    "sclk_sdio1", RK3288_SDIO1_CON0, 1),
+	MMC(SCLK_SDIO1_SAMPLE, "sdio1_sample", "sclk_sdio1", RK3288_SDIO1_CON1, 0),
+
+	MMC(SCLK_EMMC_DRV,     "emmc_drv",     "sclk_emmc",  RK3288_EMMC_CON0,  1),
+	MMC(SCLK_EMMC_SAMPLE,  "emmc_sample",  "sclk_emmc",  RK3288_EMMC_CON1,  0),
+
 	COMPOSITE(0, "sclk_tspout", mux_tspout_p, 0,
 			RK3288_CLKSEL_CON(35), 14, 2, MFLAGS, 8, 5, DFLAGS,
 			RK3288_CLKGATE_CON(4), 11, GFLAGS),
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 1e68bff..ae143f8 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -279,6 +279,14 @@ void __init rockchip_clk_register_branches(
 				list->gate_offset, list->gate_shift,
 				list->gate_flags, flags, &clk_lock);
 			break;
+		case branch_mmc:
+			clk = rockchip_clk_register_mmc(
+				list->name,
+				list->parent_names, list->num_parents,
+				reg_base + list->muxdiv_offset,
+				list->div_shift
+			);
+			break;
 		}
 
 		/* none of the cases above matched */
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index ca009ab..136e8a9 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -48,6 +48,14 @@
 #define RK3288_GLB_SRST_SND		0x1b4
 #define RK3288_SOFTRST_CON(x)		(x * 0x4 + 0x1b8)
 #define RK3288_MISC_CON			0x1e8
+#define RK3288_SDMMC_CON0		0x200
+#define RK3288_SDMMC_CON1		0x204
+#define RK3288_SDIO0_CON0		0x208
+#define RK3288_SDIO0_CON1		0x20c
+#define RK3288_SDIO1_CON0		0x210
+#define RK3288_SDIO1_CON1		0x214
+#define RK3288_EMMC_CON0		0x218
+#define RK3288_EMMC_CON1		0x21c
 
 enum rockchip_pll_type {
 	pll_rk3066,
@@ -152,6 +160,10 @@ struct clk *rockchip_clk_register_cpuclk(const char *name,
 			const struct rockchip_cpuclk_rate_table *rates,
 			int nrates, void __iomem *reg_base, spinlock_t *lock);
 
+struct clk *rockchip_clk_register_mmc(const char *name,
+				const char **parent_names, u8 num_parents,
+				void __iomem *reg, int shift);
+
 #define PNAME(x) static const char *x[] __initconst
 
 enum rockchip_clk_branch_type {
@@ -160,6 +172,7 @@ enum rockchip_clk_branch_type {
 	branch_divider,
 	branch_fraction_divider,
 	branch_gate,
+	branch_mmc,
 };
 
 struct rockchip_clk_branch {
@@ -352,6 +365,16 @@ struct rockchip_clk_branch {
 		.gate_flags	= gf,				\
 	}
 
+#define MMC(_id, cname, pname, offset, shift)			\
+	{							\
+		.id		= _id,				\
+		.branch_type	= branch_mmc,			\
+		.name		= cname,			\
+		.parent_names	= (const char *[]){ pname },	\
+		.num_parents	= 1,				\
+		.muxdiv_offset	= offset,			\
+		.div_shift	= shift,			\
+	}
 
 void rockchip_clk_init(struct device_node *np, void __iomem *base,
 		       unsigned long nr_clks);
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH v3 0/2] Add support for the rockchip mmc clock phases using the framework

[Heiko Stübner]

Am Dienstag, 18. November 2014, 13:08:26 schrieb Alexandru M Stan:
> For now all I have is the getter and setter for the phase, nothing that uses
> it (that is ready). You can test the getter like this:
> localhost ~ # cat /sys/kernel/debug/clk/clk_summary|grep sample -C 1
>     sclk_sdio1                            0            0    24000000        
>  0 0 sdio1_sample                       0            0    12000000         
> 0 0 sdio1_drv                          0            0    12000000         
> 0 90 --
>           sclk_sdmmc                      1            1   297000000        
>  0 0 sdmmc_sample                 0            0   148500000          0 134
> sdmmc_drv                    0            0   148500000          0 90 --
>           sclk_sdio0                      1            1   100000000        
>  0 0 sdio0_sample                 0            0    50000000          0 0
> sdio0_drv                    0            0    50000000          0 90
> sclk_emmc                       1            1   100000000          0 0
> emmc_sample                  0            0    50000000          0 0
> emmc_drv                     0            0    50000000          0 180
> 
> Next thing that will come is some dts changes that will make use of these
> new clocks, and eventually mmc code will be changed to tune with these
> clocks.

As already said in v1, this looks nice to me, but I'll give Mike another day 
or two to voice possible concerns.


Heiko

Re: [PATCH v3 0/2] Add support for the rockchip mmc clock phases using the framework

[Doug Anderson]

Heiko,

On Tue, Nov 25, 2014 at 1:35 AM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Dienstag, 18. November 2014, 13:08:26 schrieb Alexandru M Stan:
>> For now all I have is the getter and setter for the phase, nothing that uses
>> it (that is ready). You can test the getter like this:
>> localhost ~ # cat /sys/kernel/debug/clk/clk_summary|grep sample -C 1
>>     sclk_sdio1                            0            0    24000000
>>  0 0 sdio1_sample                       0            0    12000000
>> 0 0 sdio1_drv                          0            0    12000000
>> 0 90 --
>>           sclk_sdmmc                      1            1   297000000
>>  0 0 sdmmc_sample                 0            0   148500000          0 134
>> sdmmc_drv                    0            0   148500000          0 90 --
>>           sclk_sdio0                      1            1   100000000
>>  0 0 sdio0_sample                 0            0    50000000          0 0
>> sdio0_drv                    0            0    50000000          0 90
>> sclk_emmc                       1            1   100000000          0 0
>> emmc_sample                  0            0    50000000          0 0
>> emmc_drv                     0            0    50000000          0 180
>>
>> Next thing that will come is some dts changes that will make use of these
>> new clocks, and eventually mmc code will be changed to tune with these
>> clocks.
>
> As already said in v1, this looks nice to me, but I'll give Mike another day
> or two to voice possible concerns.

Please hold off on applying.  I spent some time with Alex over the
last few days and we found a few issues.  There were a few tiny bugs,
but also we found that some cards were not tuning properly.

Specifically it appears that on our board most UHS cards will reports
two valid ranges of valid phases, like:
  0 - 135: good
  225 - 270: good

That means that 180 was bad and 315 were bad.  It's a little unclear
why there are two bad ranges (I know very little of the signaling of
MMC), but my uneducated guess was that one of the bad ranges was
because of timing and the other because of signal integrity
(reflections, etc?)

In any case, on some cards we were actually finding only one range,
like maybe we'd find that 0 - 270 were good and only 315 was bad.
Then we'd pick right in the middle of the range, like 135.  The
problem was that on these cards 135 was a little iffy.  It somehow
managed to pass the tuning most of the time but then it would fail in
real usage.  Even repeating tuning 1000 times wasn't enough to get it
to fail reliably.  Could it be that the "fine delay" actually varies
with a very long period, so maybe it acts like "40ps" for a while then
"80ps" for a while?  That means that when we thought tuned with 135
degrees maybe we actually tuned with 120 degrees or 150 degrees.  When
it changed we started getting failures?


Our current proposal is to add "22.5" degree increments to the driver,
which seemed to fix the problem on the cards we tested.  It
successfully noticed some extra failing phases.  Going by 22.5 is the
smallest increment we can go without exposing the iffy-ness of the
fine delay to the driver.  Remember that the fine delay is between
40-80ps per count and we assume 60ps.  So our delay can be 1.33x as
long or .67 times as long.  Going by 22.5, we get

0 = exactly 0 degrees (coarse delay)
22.5 = 15 degrees - 30 degrees (coarse delay + fine delay)
45.0 = 30 degrees - 60 degrees (coarse delay + fine delay)
67.5 = 45 degrees - 90 degrees (coarse delay + fine delay)
90.0 = exactly 90 degrees (coarse delay)

That means we can be guaranteed that the real delay when the user
requests 90.0 degrees is >= the relay delay when they request 67.5.
We also get a guarantee that we _definitely_ tested a phase that is 0,
a phase that is < 45 degrees and one that was >= 45 degrees.


If the above proposal is not OK or we find cards that still fail with
that, we might have to go back to the drawing board and somehow expose
the fact that our fine delay is not very precise.  Other proposals
that were talked about:

1. On higher speed cards, we could completely use the fine delay.  The
fine delay can easily make all 360 degrees, but it doesn't make them
reliably so you can't assume that 0 and 360 are the same  dw_mmc would
need to be aware of that.  Note that if the fine delay really does
change over time (like if it changed from 40ps per to 80ps per), this
could be a non-starter.

2. The really important thing is to find failure points and make sure
that we don't pick phases that are close to them.  As long as dw_mmc
was aware that requesting 80 degrees might give you anywhere between
53 and 107 degrees it could still gather good information.  If it saw
a failure when that failed then it probably shouldn't pick 45 or 90
degrees.  We could add some logic to the clock phase API for this.