diff mbox

[PATCHv6,2/5] clk: socfpga: Add a clock type for the SD/MMC driver

Message ID 1386880245-10192-3-git-send-email-dinguyen@altera.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dinh Nguyen Dec. 12, 2013, 8:30 p.m. UTC
From: Dinh Nguyen <dinguyen@altera.com>

Add a "altr,socfpga-sdmmc-sdr-clk" clock type in the SOCFPGA clock driver. This
clock type is not really a "clock" for say, but a mechanism to set the phase
shift of the clock that is used to feed the SD/MMC CIU's clock. This clock does
not have parent so it is designated as a CLK_IS_ROOT.

This clock implements the set_clk_rate method that is meant to receive the SDR
settings that is designated by the "samsung,dw-mshc-sdr-timing" binding. The
SD/MMC driver passes this clock phase information into the clock driver to use.

This enables the SD/MMC driver to touch registers that are located outside of
the SD/MMC IP, which helps make the core SD/MMC driver generic.

Signed-off-by: Dinh Nguyen <dinguyen@alter.com>
---
v6: Add a new clock type "altr,socfpga-sdmmc-sdr-clk" that will be used to
set the phase shift settings.
v5: Use the "snps,dw-mshc" binding
v4: Use the sdmmc_clk prepare function to set the phase shift settings
v3: Not use the syscon driver because as of 3.13-rc1, the syscon driver is
loaded after the clock driver.
v2: Use the syscon driver
---
 drivers/clk/socfpga/clk.c |   86 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

Comments

Arnd Bergmann Dec. 14, 2013, 9:33 p.m. UTC | #1
On Thursday 12 December 2013, dinguyen@altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> Add a "altr,socfpga-sdmmc-sdr-clk" clock type in the SOCFPGA clock driver. This
> clock type is not really a "clock" for say, but a mechanism to set the phase
> shift of the clock that is used to feed the SD/MMC CIU's clock. This clock does
> not have parent so it is designated as a CLK_IS_ROOT.
> 
> This clock implements the set_clk_rate method that is meant to receive the SDR
> settings that is designated by the "samsung,dw-mshc-sdr-timing" binding. The
> SD/MMC driver passes this clock phase information into the clock driver to use.
> 
> This enables the SD/MMC driver to touch registers that are located outside of
> the SD/MMC IP, which helps make the core SD/MMC driver generic.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@alter.com>

Ok, this looks like it will work, but we haven't concluded if this is the
best way to do it. I'm looking for guidance from Mike here.

The alternatives I can see for this particular problem are:

1. fake a clock and use the 'clk_set_rate' callback to set the phase
   rather than the clock frequency (as you do in this patch).
2. extend the common clock API to provide a 'clk_set_phase' interface
   that you can call on the CIU clock to set this, so you don't have
   to fake anything. This seems to be the nicest interface if we have
   the same problem in more drivers.
3. make the phase setting private to whichever clock is used for CIU
   (as one of your previous patches did, which I did not like).
4. make the phase an additional argument to the clock specifier,
   so you would have <&mmcclock 12345 5678> rather than just <&mmcclock>
   in the mmc host device node to specify the two possible phases.

I would also like to get buy-in from Zhangfei Gao, since he is working
on the same thing. Please coordinate with him to make sure whatever
one of you comes up with works for the other one too. At the moment,
patches are flying so fast without much discussion inbetween that I'd
prefer Chris to hold off from merging either one until you have both
revieved and Acked each other's patches.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhangfei Gao Dec. 15, 2013, 2:18 a.m. UTC | #2
Dear Arnd

On 12/15/2013 05:33 AM, Arnd Bergmann wrote:
> On Thursday 12 December 2013, dinguyen@altera.com wrote:
>> From: Dinh Nguyen <dinguyen@altera.com>
>>
>> Add a "altr,socfpga-sdmmc-sdr-clk" clock type in the SOCFPGA clock driver. This
>> clock type is not really a "clock" for say, but a mechanism to set the phase
>> shift of the clock that is used to feed the SD/MMC CIU's clock. This clock does
>> not have parent so it is designated as a CLK_IS_ROOT.
>>
>> This clock implements the set_clk_rate method that is meant to receive the SDR
>> settings that is designated by the "samsung,dw-mshc-sdr-timing" binding. The
>> SD/MMC driver passes this clock phase information into the clock driver to use.
>>
>> This enables the SD/MMC driver to touch registers that are located outside of
>> the SD/MMC IP, which helps make the core SD/MMC driver generic.
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@alter.com>
>
> Ok, this looks like it will work, but we haven't concluded if this is the
> best way to do it. I'm looking for guidance from Mike here.
>
> The alternatives I can see for this particular problem are:
>
> 1. fake a clock and use the 'clk_set_rate' callback to set the phase
>     rather than the clock frequency (as you do in this patch).
> 2. extend the common clock API to provide a 'clk_set_phase' interface
>     that you can call on the CIU clock to set this, so you don't have
>     to fake anything. This seems to be the nicest interface if we have
>     the same problem in more drivers.
> 3. make the phase setting private to whichever clock is used for CIU
>     (as one of your previous patches did, which I did not like).
> 4. make the phase an additional argument to the clock specifier,
>     so you would have <&mmcclock 12345 5678> rather than just <&mmcclock>
>     in the mmc host device node to specify the two possible phases.
>
> I would also like to get buy-in from Zhangfei Gao, since he is working
> on the same thing. Please coordinate with him to make sure whatever
> one of you comes up with works for the other one too. At the moment,
> patches are flying so fast without much discussion inbetween that I'd
> prefer Chris to hold off from merging either one until you have both
> revieved and Acked each other's patches.
>

This is our case
In this version ip, supported now.
Only several mode (LEGACY, HS) are supported in the code, where fixed 
div and sample is need to be set, so we can hide them in clock.
And each controller have different value & register, so different 
controller use different clock.
These clock can be ciu_clock, whose parent are original clock input.

In next version ip, which still not in hand.
HS200 and other mode have to be supported, tuning process have to be 
included, good example is drivers/mmc/host/dw_mmc-exynos.c
.execute_tuning         = dw_mci_exynos_execute_tuning,
It have to choose the best point during the tuning process.
Some other mode may still need tuning, which is special in our case.

Zhangfei





--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Turquette Dec. 15, 2013, 4:51 a.m. UTC | #3
Quoting zhangfei (2013-12-14 18:18:45)
> Dear Arnd
> 
> On 12/15/2013 05:33 AM, Arnd Bergmann wrote:
> > On Thursday 12 December 2013, dinguyen@altera.com wrote:
> >> From: Dinh Nguyen <dinguyen@altera.com>
> >>
> >> Add a "altr,socfpga-sdmmc-sdr-clk" clock type in the SOCFPGA clock driver. This
> >> clock type is not really a "clock" for say, but a mechanism to set the phase
> >> shift of the clock that is used to feed the SD/MMC CIU's clock. This clock does
> >> not have parent so it is designated as a CLK_IS_ROOT.
> >>
> >> This clock implements the set_clk_rate method that is meant to receive the SDR
> >> settings that is designated by the "samsung,dw-mshc-sdr-timing" binding. The
> >> SD/MMC driver passes this clock phase information into the clock driver to use.
> >>
> >> This enables the SD/MMC driver to touch registers that are located outside of
> >> the SD/MMC IP, which helps make the core SD/MMC driver generic.
> >>
> >> Signed-off-by: Dinh Nguyen <dinguyen@alter.com>
> >
> > Ok, this looks like it will work, but we haven't concluded if this is the
> > best way to do it. I'm looking for guidance from Mike here.
> >
> > The alternatives I can see for this particular problem are:
> >
> > 1. fake a clock and use the 'clk_set_rate' callback to set the phase
> >     rather than the clock frequency (as you do in this patch).
> > 2. extend the common clock API to provide a 'clk_set_phase' interface
> >     that you can call on the CIU clock to set this, so you don't have
> >     to fake anything. This seems to be the nicest interface if we have
> >     the same problem in more drivers.

clk_set_phase has been proposed before and now may be the time to add
it. There are two things that need to be addressed:

1) what are the values for the phase? This needs to work for others that
must set clk phase, so we need to consider all those requirements before
making a new function declaration in clk.h

2) is setting a clock's phase something done dynamically? Put another
way, does the same clock has it's phase set multiple times while the
system is running? For static configuration that only happens during
initialization we do not need a new API. The clock driver can handle it
privately. For dynamic operations though we likely need a new API.

I've Cc'd Peter since I think Tegra has clock phase requirements as well
that we discussed some time back.

Regards,
Mike

> > 3. make the phase setting private to whichever clock is used for CIU
> >     (as one of your previous patches did, which I did not like).
> > 4. make the phase an additional argument to the clock specifier,
> >     so you would have <&mmcclock 12345 5678> rather than just <&mmcclock>
> >     in the mmc host device node to specify the two possible phases.
> >
> > I would also like to get buy-in from Zhangfei Gao, since he is working
> > on the same thing. Please coordinate with him to make sure whatever
> > one of you comes up with works for the other one too. At the moment,
> > patches are flying so fast without much discussion inbetween that I'd
> > prefer Chris to hold off from merging either one until you have both
> > revieved and Acked each other's patches.
> >
> 
> This is our case
> In this version ip, supported now.
> Only several mode (LEGACY, HS) are supported in the code, where fixed 
> div and sample is need to be set, so we can hide them in clock.
> And each controller have different value & register, so different 
> controller use different clock.
> These clock can be ciu_clock, whose parent are original clock input.
> 
> In next version ip, which still not in hand.
> HS200 and other mode have to be supported, tuning process have to be 
> included, good example is drivers/mmc/host/dw_mmc-exynos.c
> .execute_tuning         = dw_mci_exynos_execute_tuning,
> It have to choose the best point during the tuning process.
> Some other mode may still need tuning, which is special in our case.
> 
> Zhangfei
> 
> 
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Emilio López Dec. 16, 2013, 8:55 p.m. UTC | #4
Hi Mike et al,

El 15/12/13 01:51, Mike Turquette escribió:
> clk_set_phase has been proposed before and now may be the time to add
> it. There are two things that need to be addressed:
>
> 1) what are the values for the phase? This needs to work for others that
> must set clk phase, so we need to consider all those requirements before
> making a new function declaration in clk.h
>
> 2) is setting a clock's phase something done dynamically? Put another
> way, does the same clock has it's phase set multiple times while the
> system is running? For static configuration that only happens during
> initialization we do not need a new API. The clock driver can handle it
> privately. For dynamic operations though we likely need a new API.

We on sunxi also need this for our (not yet merged) MMC driver; we 
currently have it implemented as an exported

void clk_sunxi_mmc_phase_control(struct clk_hw *hw, u8 sample, u8 output)

that takes the MMC clock (and only the MMC clock) and does the setup 
(it's basically configuring two values, "sample" and "output", into the 
clock register). I really don't know what does this do/why is it 
required/when is it used; I'm cc'ing Hans and David who can hopefully 
explain that part.

I also saw a similar requirement from the gmac people (on cc too), who 
needed to set the phy type (or something like that) on one of the clock 
registers; so I'm thinking maybe a generic "tune something" approach 
that lets users configure some clock-dependent, arbitrary aspect of the 
clock is the way forward. Otherwise, we're going to end up bloating the 
clock framework with a lot of callback pointers that are going to be 
used in one or two clocks out of potentially hundreds on the whole system.

Cheers,

Emilio
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede Dec. 16, 2013, 9:06 p.m. UTC | #5
Hi,

On 12/16/2013 09:55 PM, Emilio López wrote:
> Hi Mike et al,
>
> El 15/12/13 01:51, Mike Turquette escribió:
>> clk_set_phase has been proposed before and now may be the time to add
>> it. There are two things that need to be addressed:
>>
>> 1) what are the values for the phase? This needs to work for others that
>> must set clk phase, so we need to consider all those requirements before
>> making a new function declaration in clk.h
>>
>> 2) is setting a clock's phase something done dynamically? Put another
>> way, does the same clock has it's phase set multiple times while the
>> system is running? For static configuration that only happens during
>> initialization we do not need a new API. The clock driver can handle it
>> privately. For dynamic operations though we likely need a new API.
>
> We on sunxi also need this for our (not yet merged) MMC driver; we currently have it implemented as an exported
>
> void clk_sunxi_mmc_phase_control(struct clk_hw *hw, u8 sample, u8 output)
>
> that takes the MMC clock (and only the MMC clock) and does the setup (it's basically configuring two values, "sample" and "output", into the clock register). I really don't know what does this do/why is it required/when is it used; I'm cc'ing Hans and David who can hopefully explain that part.

I'm afraid I cannot explain that part, the MMC controller in the sunxi SoCs
is undocumented, so what we're doing there comes straight from the android
kernel code. I do understand most of the bits of the sunxi-mci driver, but
this bit is black magic to me.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Lanzendörfer Dec. 16, 2013, 9:54 p.m. UTC | #6
Hi
> that takes the MMC clock (and only the MMC clock) and does the setup
> (it's basically configuring two values, "sample" and "output", into the
> clock register). I really don't know what does this do/why is it
> required/when is it used; I'm cc'ing Hans and David who can hopefully
> explain that part.
Since the read/write operations have to happen asynchronously and in a manner
so that the data provided by the SD-Card can be fetched on time, a specific
clock phase shift is required as you can see in the linked picture[1].
That's what the function is doing: It accesses the special registers of the
MMC clock device and configures this phase offset.

> I also saw a similar requirement from the gmac people (on cc too), who
> needed to set the phy type (or something like that) on one of the clock
> registers; so I'm thinking maybe a generic "tune something" approach
> that lets users configure some clock-dependent, arbitrary aspect of the
> clock is the way forward. Otherwise, we're going to end up bloating the
> clock framework with a lot of callback pointers that are going to be
> used in one or two clocks out of potentially hundreds on the whole system.
It totally makes sense, since GMAC will be doing asynchrous
operations as well.

-David

[1] http://www.chipestimate.com/techtalk/images/11022010_fig2_3.JPG
Chen-Yu Tsai Dec. 17, 2013, 2:17 a.m. UTC | #7
Hi,

On Tue, Dec 17, 2013 at 4:55 AM, Emilio López <emilio@elopez.com.ar> wrote:
> I also saw a similar requirement from the gmac people (on cc too), who
> needed to set the phy type (or something like that) on one of the clock

The GMAC clock register controls the tx clock source (a mux) and the
interface type, which is most likely a gate to control the direction
of the tx clock.

So I modeled it as a composite clock source for now.
I'm open to other options.

> registers; so I'm thinking maybe a generic "tune something" approach that
> lets users configure some clock-dependent, arbitrary aspect of the clock is
> the way forward. Otherwise, we're going to end up bloating the clock
> framework with a lot of callback pointers that are going to be used in one
> or two clocks out of potentially hundreds on the whole system.

ChenYu
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Turquette Dec. 18, 2013, 8:10 p.m. UTC | #8
Quoting David Lanzendörfer (2013-12-16 13:54:21)
> Hi
> > that takes the MMC clock (and only the MMC clock) and does the setup
> > (it's basically configuring two values, "sample" and "output", into the
> > clock register). I really don't know what does this do/why is it
> > required/when is it used; I'm cc'ing Hans and David who can hopefully
> > explain that part.
> Since the read/write operations have to happen asynchronously and in a manner
> so that the data provided by the SD-Card can be fetched on time, a specific
> clock phase shift is required as you can see in the linked picture[1].
> That's what the function is doing: It accesses the special registers of the
> MMC clock device and configures this phase offset.

Yeah, clock phase is a fairly common thing to tune but there are two
things to consider when crafting a new api like clk_set_phase(...):

1) are we adjusting the phase of the clock signal or the phase of
something else related to the clock rate? This is just something to
think about. Sometimes it would be better to have
video_signal_set_phase() instead of clk_set_phase().

2) what is the input parameter to clk_set_phase? I guess the two best
options are degrees (zero to 359), or a fraction of the clock period
(1/4, 1/2, etc).

Any thoughts?

Regards,
Mike

> 
> > I also saw a similar requirement from the gmac people (on cc too), who
> > needed to set the phy type (or something like that) on one of the clock
> > registers; so I'm thinking maybe a generic "tune something" approach
> > that lets users configure some clock-dependent, arbitrary aspect of the
> > clock is the way forward. Otherwise, we're going to end up bloating the
> > clock framework with a lot of callback pointers that are going to be
> > used in one or two clocks out of potentially hundreds on the whole system.
> It totally makes sense, since GMAC will be doing asynchrous
> operations as well.
> 
> -David
> 
> [1] http://www.chipestimate.com/techtalk/images/11022010_fig2_3.JPG
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/clk/socfpga/clk.c b/drivers/clk/socfpga/clk.c
index 280c983..f4c983e 100644
--- a/drivers/clk/socfpga/clk.c
+++ b/drivers/clk/socfpga/clk.c
@@ -21,8 +21,10 @@ 
 #include <linux/clkdev.h>
 #include <linux/clk-provider.h>
 #include <linux/io.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/regmap.h>
 
 /* Clock Manager offsets */
 #define CLKMGR_CTRL	0x0
@@ -69,6 +71,84 @@  struct socfpga_clk {
 };
 #define to_socfpga_clk(p) container_of(p, struct socfpga_clk, hw.hw)
 
+/* SDMMC Group for System Manager defines */
+#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
+	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
+struct sdmmc_sdr_clk {
+	struct clk_hw	hw;
+	u32	reg;
+};
+#define to_sdmmc_sdr_clk(p) container_of(p, struct sdmmc_sdr_clk, hw)
+
+static int sdr_clk_set_rate(struct clk_hw *hwclk, unsigned long rate,
+					unsigned long parent_rate)
+{
+	struct sdmmc_sdr_clk *sdmmc_sdr_clk = to_sdmmc_sdr_clk(hwclk);
+	struct regmap *sys_mgr_base_addr;
+	u32 hs_timing;
+
+	sys_mgr_base_addr = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
+	if (IS_ERR(sys_mgr_base_addr)) {
+		pr_err("%s: failed to find altr,sys-mgr regmap!\n", __func__);
+		return -EINVAL;
+	}
+	hs_timing = SYSMGR_SDMMC_CTRL_SET(((rate > 4) & 0xf), (rate & 0xf));
+	regmap_write(sys_mgr_base_addr, sdmmc_sdr_clk->reg, hs_timing);
+	return 0;
+}
+
+static unsigned long sdr_clk_recalc_rate(struct clk_hw *hwclk,
+						unsigned long parent_rate)
+{
+	return parent_rate;
+}
+
+static long sdr_clk_round_rate(struct clk_hw *hwclk, unsigned long rate,
+						unsigned long *parent_rate)
+{
+	return rate;
+}
+
+static const struct clk_ops sdmmc_sdr_clk_ops = {
+	.recalc_rate = sdr_clk_recalc_rate,
+	.round_rate = sdr_clk_round_rate,
+	.set_rate = sdr_clk_set_rate,
+};
+
+static __init struct clk *socfpga_sdmmc_sdr_clk_init(struct device_node *node,
+						const struct clk_ops *ops)
+{
+	u32 reg;
+	struct clk *clk;
+	struct sdmmc_sdr_clk *sdmmc_sdr_clk;
+	const char *clk_name = node->name;
+	struct clk_init_data init;
+	int rc;
+
+	rc = of_property_read_u32(node, "reg", &reg);
+
+	sdmmc_sdr_clk = kzalloc(sizeof(*sdmmc_sdr_clk), GFP_KERNEL);
+	if (WARN_ON(!sdmmc_sdr_clk))
+		return NULL;
+
+	sdmmc_sdr_clk->reg = reg;
+	of_property_read_string(node, "clock-output-names", &clk_name);
+
+	init.name = clk_name;
+	init.ops = ops;
+	init.flags = CLK_IS_ROOT;
+	init.num_parents = 0;
+	sdmmc_sdr_clk->hw.init = &init;
+
+	clk = clk_register(NULL, &sdmmc_sdr_clk->hw);
+	if (WARN_ON(IS_ERR(clk))) {
+		kfree(sdmmc_sdr_clk);
+		return NULL;
+	}
+	rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	return clk;
+}
+
 static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
 					 unsigned long parent_rate)
 {
@@ -332,10 +412,16 @@  static void __init socfpga_gate_init(struct device_node *node)
 	socfpga_gate_clk_init(node, &gateclk_ops);
 }
 
+static void __init socfpga_sdmmc_init(struct device_node *node)
+{
+	socfpga_sdmmc_sdr_clk_init(node, &sdmmc_sdr_clk_ops);
+}
+
 static struct of_device_id socfpga_child_clocks[] = {
 	{ .compatible = "altr,socfpga-pll-clock", socfpga_pll_init, },
 	{ .compatible = "altr,socfpga-perip-clk", socfpga_periph_init, },
 	{ .compatible = "altr,socfpga-gate-clk", socfpga_gate_init, },
+	{ .compatible = "altr,socfpga-sdmmc-sdr-clk", socfpga_sdmmc_init, },
 	{},
 };