| Message ID | 1386880245-10192-3-git-send-email-dinguyen@altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 > > > > >
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
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
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
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
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
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", ®); + + 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, }, {}, };