| Message ID | 1387213476-22122-2-git-send-email-dinguyen@altera.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 12/17/2013 01:04 AM, dinguyen@altera.com wrote: > From: Dinh Nguyen <dinguyen@altera.com> > +static int socfpga_clk_prepare(struct clk_hw *hwclk) > +{ > + struct socfpga_clk *socfpgaclk = to_socfpga_clk(hwclk); > + struct regmap *sys_mgr_base_addr; > + u32 hs_timing; > + > + if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) { > + 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(socfpgaclk->clk_phase[0], > + socfpgaclk->clk_phase[1]); > + regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET, > + hs_timing); > + } > + return 0; > +} So reusing gate-clk here and check the node of "altr,sys-mgr". I think it is good and simple. Also can define new clock combined with node "altr,sys-mgr" with parent of sdmmc_clk. Thanks for the update, it is fine to me. > + > static struct clk_ops gateclk_ops = { > + .prepare = socfpga_clk_prepare, > .recalc_rate = socfpga_clk_recalc_rate, > .get_parent = socfpga_clk_get_parent, > .set_parent = socfpga_clk_set_parent, > @@ -254,6 +283,7 @@ static void __init socfpga_gate_clk_init(struct device_node *node, > { > u32 clk_gate[2]; > u32 div_reg[3]; > + u32 clk_phase[2]; > u32 fixed_div; > struct clk *clk; > struct socfpga_clk *socfpga_clk; > @@ -294,6 +324,12 @@ static void __init socfpga_gate_clk_init(struct device_node *node, > socfpga_clk->div_reg = 0; > } > > + rc = of_property_read_u32_array(node, "clk-phase", clk_phase, 2); > + if (!rc) { > + socfpga_clk->clk_phase[0] = clk_phase[0]; > + socfpga_clk->clk_phase[1] = clk_phase[1]; > + } > + > of_property_read_string(node, "clock-output-names", &clk_name); > > init.name = clk_name; > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Zhangfei, On 12/17/13 1:46 AM, zhangfei wrote: > > > On 12/17/2013 01:04 AM, dinguyen@altera.com wrote: >> From: Dinh Nguyen <dinguyen@altera.com> > >> +static int socfpga_clk_prepare(struct clk_hw *hwclk) >> +{ >> + struct socfpga_clk *socfpgaclk = to_socfpga_clk(hwclk); >> + struct regmap *sys_mgr_base_addr; >> + u32 hs_timing; >> + >> + if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) { >> + 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(socfpgaclk->clk_phase[0], >> + socfpgaclk->clk_phase[1]); >> + regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET, >> + hs_timing); >> + } >> + return 0; >> +} > > So reusing gate-clk here and check the node of "altr,sys-mgr". > I think it is good and simple. > Also can define new clock combined with node "altr,sys-mgr" with > parent of sdmmc_clk. > > Thanks for the update, it is fine to me. Thanks, can I get an Ack from you for this version? Thanks, Dinh > >> + >> static struct clk_ops gateclk_ops = { >> + .prepare = socfpga_clk_prepare, >> .recalc_rate = socfpga_clk_recalc_rate, >> .get_parent = socfpga_clk_get_parent, >> .set_parent = socfpga_clk_set_parent, >> @@ -254,6 +283,7 @@ static void __init socfpga_gate_clk_init(struct >> device_node *node, >> { >> u32 clk_gate[2]; >> u32 div_reg[3]; >> + u32 clk_phase[2]; >> u32 fixed_div; >> struct clk *clk; >> struct socfpga_clk *socfpga_clk; >> @@ -294,6 +324,12 @@ static void __init socfpga_gate_clk_init(struct >> device_node *node, >> socfpga_clk->div_reg = 0; >> } >> >> + rc = of_property_read_u32_array(node, "clk-phase", clk_phase, 2); >> + if (!rc) { >> + socfpga_clk->clk_phase[0] = clk_phase[0]; >> + socfpga_clk->clk_phase[1] = clk_phase[1]; >> + } >> + >> of_property_read_string(node, "clock-output-names", &clk_name); >> >> init.name = clk_name; >> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/17/2013 09:44 PM, Dinh Nguyen wrote: > Hi Zhangfei, > > On 12/17/13 1:46 AM, zhangfei wrote: >> >> >> On 12/17/2013 01:04 AM, dinguyen@altera.com wrote: >>> From: Dinh Nguyen <dinguyen@altera.com> >> >>> +static int socfpga_clk_prepare(struct clk_hw *hwclk) >>> +{ >>> + struct socfpga_clk *socfpgaclk = to_socfpga_clk(hwclk); >>> + struct regmap *sys_mgr_base_addr; >>> + u32 hs_timing; >>> + >>> + if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) { >>> + 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(socfpgaclk->clk_phase[0], >>> + socfpgaclk->clk_phase[1]); >>> + regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET, >>> + hs_timing); >>> + } >>> + return 0; >>> +} >> >> So reusing gate-clk here and check the node of "altr,sys-mgr". >> I think it is good and simple. >> Also can define new clock combined with node "altr,sys-mgr" with >> parent of sdmmc_clk. >> >> Thanks for the update, it is fine to me. > Thanks, can I get an Ack from you for this version? > Sure, if it is helpful. Thanks -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Dinh Nguyen (2013-12-17 05:44:47) > Hi Zhangfei, > > On 12/17/13 1:46 AM, zhangfei wrote: > > > > > > On 12/17/2013 01:04 AM, dinguyen@altera.com wrote: > >> From: Dinh Nguyen <dinguyen@altera.com> > > > >> +static int socfpga_clk_prepare(struct clk_hw *hwclk) > >> +{ > >> + struct socfpga_clk *socfpgaclk = to_socfpga_clk(hwclk); > >> + struct regmap *sys_mgr_base_addr; > >> + u32 hs_timing; > >> + > >> + if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) { > >> + 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(socfpgaclk->clk_phase[0], > >> + socfpgaclk->clk_phase[1]); > >> + regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET, > >> + hs_timing); > >> + } > >> + return 0; > >> +} > > > > So reusing gate-clk here and check the node of "altr,sys-mgr". > > I think it is good and simple. > > Also can define new clock combined with node "altr,sys-mgr" with > > parent of sdmmc_clk. > > > > Thanks for the update, it is fine to me. > Thanks, can I get an Ack from you for this version? Dinh, Will you be spinning another version of this patch? If not I can go ahead and take this into clk-next. Regards, Mike > > Thanks, > Dinh > > > >> + > >> static struct clk_ops gateclk_ops = { > >> + .prepare = socfpga_clk_prepare, > >> .recalc_rate = socfpga_clk_recalc_rate, > >> .get_parent = socfpga_clk_get_parent, > >> .set_parent = socfpga_clk_set_parent, > >> @@ -254,6 +283,7 @@ static void __init socfpga_gate_clk_init(struct > >> device_node *node, > >> { > >> u32 clk_gate[2]; > >> u32 div_reg[3]; > >> + u32 clk_phase[2]; > >> u32 fixed_div; > >> struct clk *clk; > >> struct socfpga_clk *socfpga_clk; > >> @@ -294,6 +324,12 @@ static void __init socfpga_gate_clk_init(struct > >> device_node *node, > >> socfpga_clk->div_reg = 0; > >> } > >> > >> + rc = of_property_read_u32_array(node, "clk-phase", clk_phase, 2); > >> + if (!rc) { > >> + socfpga_clk->clk_phase[0] = clk_phase[0]; > >> + socfpga_clk->clk_phase[1] = clk_phase[1]; > >> + } > >> + > >> of_property_read_string(node, "clock-output-names", &clk_name); > >> > >> init.name = clk_name; > >> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mike, On 12/17/13 5:55 PM, Mike Turquette wrote: > Quoting Dinh Nguyen (2013-12-17 05:44:47) >> Hi Zhangfei, >> >> On 12/17/13 1:46 AM, zhangfei wrote: >>> >>> On 12/17/2013 01:04 AM, dinguyen@altera.com wrote: >>>> From: Dinh Nguyen <dinguyen@altera.com> >>>> +static int socfpga_clk_prepare(struct clk_hw *hwclk) >>>> +{ >>>> + struct socfpga_clk *socfpgaclk = to_socfpga_clk(hwclk); >>>> + struct regmap *sys_mgr_base_addr; >>>> + u32 hs_timing; >>>> + >>>> + if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) { >>>> + 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(socfpgaclk->clk_phase[0], >>>> + socfpgaclk->clk_phase[1]); >>>> + regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET, >>>> + hs_timing); >>>> + } >>>> + return 0; >>>> +} >>> So reusing gate-clk here and check the node of "altr,sys-mgr". >>> I think it is good and simple. >>> Also can define new clock combined with node "altr,sys-mgr" with >>> parent of sdmmc_clk. >>> >>> Thanks for the update, it is fine to me. >> Thanks, can I get an Ack from you for this version? > Dinh, > > Will you be spinning another version of this patch? If not I can go > ahead and take this into clk-next. If you are OK with the patch, then yes, can you please apply it to clk-next? Thanks, Dinh > > Regards, > Mike > >> Thanks, >> Dinh >>>> + >>>> static struct clk_ops gateclk_ops = { >>>> + .prepare = socfpga_clk_prepare, >>>> .recalc_rate = socfpga_clk_recalc_rate, >>>> .get_parent = socfpga_clk_get_parent, >>>> .set_parent = socfpga_clk_set_parent, >>>> @@ -254,6 +283,7 @@ static void __init socfpga_gate_clk_init(struct >>>> device_node *node, >>>> { >>>> u32 clk_gate[2]; >>>> u32 div_reg[3]; >>>> + u32 clk_phase[2]; >>>> u32 fixed_div; >>>> struct clk *clk; >>>> struct socfpga_clk *socfpga_clk; >>>> @@ -294,6 +324,12 @@ static void __init socfpga_gate_clk_init(struct >>>> device_node *node, >>>> socfpga_clk->div_reg = 0; >>>> } >>>> >>>> + rc = of_property_read_u32_array(node, "clk-phase", clk_phase, 2); >>>> + if (!rc) { >>>> + socfpga_clk->clk_phase[0] = clk_phase[0]; >>>> + socfpga_clk->clk_phase[1] = clk_phase[1]; >>>> + } >>>> + >>>> of_property_read_string(node, "clock-output-names", &clk_name); >>>> >>>> init.name = clk_name; >>>> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting dinguyen@altera.com (2013-12-16 09:04:33) > From: Dinh Nguyen <dinguyen@altera.com> > > The clk-phase property is used to represent the 2 clock phase values that is > needed for the SD/MMC driver. Add a prepare function to the clk_ops, that will > use the syscon driver to set sdmmc_clk's phase shift that is located in the > system manager. > > Signed-off-by: Dinh Nguyen <dinguyen@altera.com> > --- > v7: Add dts property to represent the clk phase of the sdmmc_clk. Add a > prepare function to the gate clk that will toggle clock phase setting. > Remove the "altr,socfpga-sdmmc-sdr-clk" clock type. > 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 > --- > .../devicetree/bindings/clock/altr_socfpga.txt | 14 ++++++++ > arch/arm/boot/dts/socfpga.dtsi | 1 + > drivers/clk/socfpga/clk.c | 36 ++++++++++++++++++++ > 3 files changed, 51 insertions(+) > > diff --git a/Documentation/devicetree/bindings/clock/altr_socfpga.txt b/Documentation/devicetree/bindings/clock/altr_socfpga.txt > index 0045433..4b66754 100644 > --- a/Documentation/devicetree/bindings/clock/altr_socfpga.txt > +++ b/Documentation/devicetree/bindings/clock/altr_socfpga.txt > @@ -23,3 +23,17 @@ Optional properties: > and the bit index. > - div-reg : For "socfpga-gate-clk", div-reg contains the divider register, bit shift, > and width. > +- clk-phase : For the sdmmc_clk, contains the value of the clock phase that controls > + the SDMMC CIU clock. The first value is the clk_sample(smpsel), and the second > + value is the cclk_in_drv(drvsel). The clk-phase is used to enable the correct > + hold/delay times that is needed for the SD/MMC CIU clock. The values of both > + value can be one of the following: > + > + <0x0> : 0 degrees > + <0x1> : 45 degrees > + <0x2> : 90 degrees > + <0x3> : 135 degrees > + <0x4> : 180 degrees > + <0x5> : 225 degrees > + <0x6> : 270 degrees > + <0x7> : 315 degrees > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi > index f936476..616d9ee 100644 > --- a/arch/arm/boot/dts/socfpga.dtsi > +++ b/arch/arm/boot/dts/socfpga.dtsi > @@ -413,6 +413,7 @@ > compatible = "altr,socfpga-gate-clk"; > clocks = <&f2s_periph_ref_clk>, <&main_nand_sdmmc_clk>, <&per_nand_mmc_clk>; > clk-gate = <0xa0 8>; > + clk-phase = <0 3>; Looking at this again, I have a hard time understanding the values in the clk-phase property. You reference some functions in the property definition above, but they are not obvious to me. Additionally I wonder if the binding would better if the clock-phase property was simply the value in degrees. E.g: clk-phase = <315>; // 315 degrees Regards, Mike > }; > > nand_x_clk: nand_x_clk { > diff --git a/drivers/clk/socfpga/clk.c b/drivers/clk/socfpga/clk.c > index 280c983..7cfebd6 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 > @@ -56,6 +58,11 @@ > #define div_mask(width) ((1 << (width)) - 1) > #define streq(a, b) (strcmp((a), (b)) == 0) > > +/* SDMMC Group for System Manager defines */ > +#define SYSMGR_SDMMCGRP_CTRL_OFFSET 0x108 > +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \ > + ((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0)) > + > static void __iomem *clk_mgr_base_addr; > > struct socfpga_clk { > @@ -66,6 +73,7 @@ struct socfpga_clk { > void __iomem *div_reg; > u32 width; /* only valid if div_reg != 0 */ > u32 shift; /* only valid if div_reg != 0 */ > + u32 clk_phase[2]; > }; > #define to_socfpga_clk(p) container_of(p, struct socfpga_clk, hw.hw) > > @@ -243,7 +251,28 @@ static unsigned long socfpga_clk_recalc_rate(struct clk_hw *hwclk, > return parent_rate / div; > } > > +static int socfpga_clk_prepare(struct clk_hw *hwclk) > +{ > + struct socfpga_clk *socfpgaclk = to_socfpga_clk(hwclk); > + struct regmap *sys_mgr_base_addr; > + u32 hs_timing; > + > + if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) { > + 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(socfpgaclk->clk_phase[0], > + socfpgaclk->clk_phase[1]); > + regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET, > + hs_timing); > + } > + return 0; > +} > + > static struct clk_ops gateclk_ops = { > + .prepare = socfpga_clk_prepare, > .recalc_rate = socfpga_clk_recalc_rate, > .get_parent = socfpga_clk_get_parent, > .set_parent = socfpga_clk_set_parent, > @@ -254,6 +283,7 @@ static void __init socfpga_gate_clk_init(struct device_node *node, > { > u32 clk_gate[2]; > u32 div_reg[3]; > + u32 clk_phase[2]; > u32 fixed_div; > struct clk *clk; > struct socfpga_clk *socfpga_clk; > @@ -294,6 +324,12 @@ static void __init socfpga_gate_clk_init(struct device_node *node, > socfpga_clk->div_reg = 0; > } > > + rc = of_property_read_u32_array(node, "clk-phase", clk_phase, 2); > + if (!rc) { > + socfpga_clk->clk_phase[0] = clk_phase[0]; > + socfpga_clk->clk_phase[1] = clk_phase[1]; > + } > + > of_property_read_string(node, "clock-output-names", &clk_name); > > init.name = clk_name; > -- > 1.7.9.5 > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 18 December 2013, Mike Turquette wrote: > > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi > > index f936476..616d9ee 100644 > > --- a/arch/arm/boot/dts/socfpga.dtsi > > +++ b/arch/arm/boot/dts/socfpga.dtsi > > @@ -413,6 +413,7 @@ > > compatible = "altr,socfpga-gate-clk"; > > clocks = <&f2s_periph_ref_clk>, <&main_nand_sdmmc_clk>, <&per_nand_mmc_clk>; > > clk-gate = <0xa0 8>; > > + clk-phase = <0 3>; > > Looking at this again, I have a hard time understanding the values in > the clk-phase property. You reference some functions in the property > definition above, but they are not obvious to me. > > Additionally I wonder if the binding would better if the clock-phase > property was simply the value in degrees. E.g: > > clk-phase = <315>; // 315 degrees I would definitely prefer using degrees over an arbitrary enumeration that might work on some platforms but not on others. I'm also a bit skeptical about the idea of putting the phase into the clock provider rather than the consumer, given the comments about the clk_set_phase() interface earlier. Generally we try to avoid having consumer-specific settings in a provider node (for any DT binding, not just clocks). Can't you have the same numbers in the dw-mshc node instead and let the mmc driver call clk_set_phase instead? If every clock has a fixed phase for a given piece of hardware, it could even be set automatically by making the common clk code read the clk-phase attribute at the time a driver calls clk_get. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/18/13 3:21 PM, Arnd Bergmann wrote: > On Wednesday 18 December 2013, Mike Turquette wrote: >>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi >>> index f936476..616d9ee 100644 >>> --- a/arch/arm/boot/dts/socfpga.dtsi >>> +++ b/arch/arm/boot/dts/socfpga.dtsi >>> @@ -413,6 +413,7 @@ >>> compatible = "altr,socfpga-gate-clk"; >>> clocks = <&f2s_periph_ref_clk>, <&main_nand_sdmmc_clk>, <&per_nand_mmc_clk>; >>> clk-gate = <0xa0 8>; >>> + clk-phase = <0 3>; >> Looking at this again, I have a hard time understanding the values in >> the clk-phase property. You reference some functions in the property >> definition above, but they are not obvious to me. >> >> Additionally I wonder if the binding would better if the clock-phase >> property was simply the value in degrees. E.g: >> >> clk-phase = <315>; // 315 degrees > I would definitely prefer using degrees over an arbitrary enumeration that > might work on some platforms but not on others. > > I'm also a bit skeptical about the idea of putting the phase into the clock > provider rather than the consumer, given the comments about the > clk_set_phase() interface earlier. Generally we try to avoid having > consumer-specific settings in a provider node (for any DT binding, > not just clocks). Can't you have the same numbers in the dw-mshc > node instead and let the mmc driver call clk_set_phase instead? > If every clock has a fixed phase for a given piece of hardware, it > could even be set automatically by making the common clk code read > the clk-phase attribute at the time a driver calls clk_get. So I think this is what you're suggesting: clocks = <&sdmmc_clk 0 135>, this would specify 0 and 135 degrees phase. The clock-bindings document is stating that the integer in the clocks property is specifying the output number of the clock. Would this approach cause a conflict and would need an update to that document/approach? Dinh > > Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 19 December 2013, Dinh Nguyen wrote: > On 12/18/13 3:21 PM, Arnd Bergmann wrote: > > On Wednesday 18 December 2013, Mike Turquette wrote: > > I would definitely prefer using degrees over an arbitrary enumeration that > > might work on some platforms but not on others. > > > > I'm also a bit skeptical about the idea of putting the phase into the clock > > provider rather than the consumer, given the comments about the > > clk_set_phase() interface earlier. Generally we try to avoid having > > consumer-specific settings in a provider node (for any DT binding, > > not just clocks). Can't you have the same numbers in the dw-mshc > > node instead and let the mmc driver call clk_set_phase instead? > > If every clock has a fixed phase for a given piece of hardware, it > > could even be set automatically by making the common clk code read > > the clk-phase attribute at the time a driver calls clk_get. > > So I think this is what you're suggesting: > clocks = <&sdmmc_clk 0 135>, this would specify 0 and 135 degrees phase. It's not what I meant in the paragraph above, but I have suggested this method in the past, and I think that would solve the problem very nicely as well, if Mike agrees. > The clock-bindings document is stating that the integer in the clocks > property is > specifying the output number of the clock. Would this approach cause a > conflict and would > need an update to that document/approach? I don't think we have to change the common binding. While it lists the use of the integer for multiple clock outputs, that's not necessarily the only possible use, and specific bindings can always override the generic ones in a compatible way. You would certainly have to update the binding of your clock controller to do this, in particular because #clock-cells is now <1>. You will probably need to add a new "compatible" string for a clock that has a phase setting rather than just a gate, and document what the argument is for. If possible, try to model the hardware the way it actually is implemented. If the clock controller supports both the gate and the phase in the same block, then make it one clock device node, but if you have one block generating the clock and another one to shift the phase, it may be best to model that second clock in a separate node so you can split the driver code according to the registers, like mmcclock-shifted { compatible = "altr,socfpga-clk-shift"; reg = <0xabcd>; clocks = <&mmcclock>; #clock-cells = <1> }; mshc { ... clocks = <&/clocks/mmcclock-shifted 135>; clock-names = "ciu"; }; Coming back to what I actually tried to suggest above was mshc { ... clocks = <&/clocks/mmcclock>; clock-names = "ciu"; clock-phases = <135>; }; This would keep the phase setting out of the specific binding and move it to the generic clock binding. In Linux we could implement this either by making the mshc driver read the phase value and call clock_set_phase() on it, or the common clock code could look up the clock-phases property at the same time it looks up clock-names and clocks, and set this behind the back of the driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/clock/altr_socfpga.txt b/Documentation/devicetree/bindings/clock/altr_socfpga.txt index 0045433..4b66754 100644 --- a/Documentation/devicetree/bindings/clock/altr_socfpga.txt +++ b/Documentation/devicetree/bindings/clock/altr_socfpga.txt @@ -23,3 +23,17 @@ Optional properties: and the bit index. - div-reg : For "socfpga-gate-clk", div-reg contains the divider register, bit shift, and width. +- clk-phase : For the sdmmc_clk, contains the value of the clock phase that controls + the SDMMC CIU clock. The first value is the clk_sample(smpsel), and the second + value is the cclk_in_drv(drvsel). The clk-phase is used to enable the correct + hold/delay times that is needed for the SD/MMC CIU clock. The values of both + value can be one of the following: + + <0x0> : 0 degrees + <0x1> : 45 degrees + <0x2> : 90 degrees + <0x3> : 135 degrees + <0x4> : 180 degrees + <0x5> : 225 degrees + <0x6> : 270 degrees + <0x7> : 315 degrees diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi index f936476..616d9ee 100644 --- a/arch/arm/boot/dts/socfpga.dtsi +++ b/arch/arm/boot/dts/socfpga.dtsi @@ -413,6 +413,7 @@ compatible = "altr,socfpga-gate-clk"; clocks = <&f2s_periph_ref_clk>, <&main_nand_sdmmc_clk>, <&per_nand_mmc_clk>; clk-gate = <0xa0 8>; + clk-phase = <0 3>; }; nand_x_clk: nand_x_clk { diff --git a/drivers/clk/socfpga/clk.c b/drivers/clk/socfpga/clk.c index 280c983..7cfebd6 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 @@ -56,6 +58,11 @@ #define div_mask(width) ((1 << (width)) - 1) #define streq(a, b) (strcmp((a), (b)) == 0) +/* SDMMC Group for System Manager defines */ +#define SYSMGR_SDMMCGRP_CTRL_OFFSET 0x108 +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \ + ((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0)) + static void __iomem *clk_mgr_base_addr; struct socfpga_clk { @@ -66,6 +73,7 @@ struct socfpga_clk { void __iomem *div_reg; u32 width; /* only valid if div_reg != 0 */ u32 shift; /* only valid if div_reg != 0 */ + u32 clk_phase[2]; }; #define to_socfpga_clk(p) container_of(p, struct socfpga_clk, hw.hw) @@ -243,7 +251,28 @@ static unsigned long socfpga_clk_recalc_rate(struct clk_hw *hwclk, return parent_rate / div; } +static int socfpga_clk_prepare(struct clk_hw *hwclk) +{ + struct socfpga_clk *socfpgaclk = to_socfpga_clk(hwclk); + struct regmap *sys_mgr_base_addr; + u32 hs_timing; + + if (socfpgaclk->clk_phase[0] || socfpgaclk->clk_phase[1]) { + 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(socfpgaclk->clk_phase[0], + socfpgaclk->clk_phase[1]); + regmap_write(sys_mgr_base_addr, SYSMGR_SDMMCGRP_CTRL_OFFSET, + hs_timing); + } + return 0; +} + static struct clk_ops gateclk_ops = { + .prepare = socfpga_clk_prepare, .recalc_rate = socfpga_clk_recalc_rate, .get_parent = socfpga_clk_get_parent, .set_parent = socfpga_clk_set_parent, @@ -254,6 +283,7 @@ static void __init socfpga_gate_clk_init(struct device_node *node, { u32 clk_gate[2]; u32 div_reg[3]; + u32 clk_phase[2]; u32 fixed_div; struct clk *clk; struct socfpga_clk *socfpga_clk; @@ -294,6 +324,12 @@ static void __init socfpga_gate_clk_init(struct device_node *node, socfpga_clk->div_reg = 0; } + rc = of_property_read_u32_array(node, "clk-phase", clk_phase, 2); + if (!rc) { + socfpga_clk->clk_phase[0] = clk_phase[0]; + socfpga_clk->clk_phase[1] = clk_phase[1]; + } + of_property_read_string(node, "clock-output-names", &clk_name); init.name = clk_name;