| Message ID | 1409238671-30452-1-git-send-email-ulrich.hecht+renesas@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Ulrich, On Thu, Aug 28, 2014 at 04:11:11PM +0100, Ulrich Hecht wrote: > From: Ulrich Hecht <ulrich.hecht@gmail.com> > > Support for setting the parent at initialization time based on the current > hardware configuration in DIV6 clocks with selectable parents as found in > the r8a73a4, r8a7740, sh73a0, and other SoCs. > > Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com> > Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > > This is v3 plus some style adjustments suggested by Laurent. > > CU > Uli > > Changes since v3: > - note that renesas,src-shift and renesas,src-width depend on each other > - clarified description > - minor coding style fixes > > Changes since v2: > - add r8a73a4 to bindings > - use u32 where appropriate > - don't split error message > > Changes since v1: > - make sure unrelated register bits are preserved > - use the plural for the clocks property in bindings > > > > .../bindings/clock/renesas,cpg-div6-clocks.txt | 12 +++++++- > drivers/clk/shmobile/clk-div6.c | 32 ++++++++++++++++++---- > 2 files changed, 38 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt > index 952e373..2633ea1 100644 > --- a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt > +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt > @@ -7,14 +7,24 @@ to 64. > Required Properties: > > - compatible: Must be one of the following > + - "renesas,r8a73a4-div6-clock" for R8A73A4 (R-Mobile APE6) DIV6 clocks > + - "renesas,r8a7740-div6-clock" for R8A7740 (R-Mobile A1) DIV6 clocks > - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks > - "renesas,r8a7791-div6-clock" for R8A7791 (R-Car M2) DIV6 clocks > + - "renesas,sh73a0-div6-clock" for SH73A0 (SH-MobileAG5) DIV6 clocks > - "renesas,cpg-div6-clock" for generic DIV6 clocks > - reg: Base address and length of the memory resource used by the DIV6 clock > - - clocks: Reference to the parent clock > + - clocks: Reference to the parent clock(s) > - #clock-cells: Must be 0 > - clock-output-names: The name of the clock as a free-form string > > +Optional Properties: > + > + - renesas,src-shift: Bit position of the input clock selector (default: > + fixed input clock; requires renesas,src-width) > + - renesas,src-width: Bit width of the input clock selector (default: fixed > + input clock; requires renesas,src-shift) I'm slightly confused by these properties, but I'm not at all familiar with the HW. How variable is the format of the configuration register? Is it fixed for a given compatible string? Are there other fields we care about? Thanks, Mark. > + > > Example > ------- > diff --git a/drivers/clk/shmobile/clk-div6.c b/drivers/clk/shmobile/clk-div6.c > index f065f69..f8b57bf 100644 > --- a/drivers/clk/shmobile/clk-div6.c > +++ b/drivers/clk/shmobile/clk-div6.c > @@ -39,8 +39,11 @@ struct div6_clock { > static int cpg_div6_clock_enable(struct clk_hw *hw) > { > struct div6_clock *clock = to_div6_clock(hw); > + u32 val; > > - clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg); > + val = (clk_readl(clock->reg) & ~(CPG_DIV6_DIV_MASK | CPG_DIV6_CKSTP)) > + | CPG_DIV6_DIV(clock->div - 1); > + clk_writel(val, clock->reg); > > return 0; > } > @@ -52,7 +55,7 @@ static void cpg_div6_clock_disable(struct clk_hw *hw) > /* DIV6 clocks require the divisor field to be non-zero when stopping > * the clock. > */ > - clk_writel(CPG_DIV6_CKSTP | CPG_DIV6_DIV(CPG_DIV6_DIV_MASK), > + clk_writel(clk_readl(clock->reg) | CPG_DIV6_CKSTP | CPG_DIV6_DIV_MASK, > clock->reg); > } > > @@ -94,12 +97,14 @@ static int cpg_div6_clock_set_rate(struct clk_hw *hw, unsigned long rate, > { > struct div6_clock *clock = to_div6_clock(hw); > unsigned int div = cpg_div6_clock_calc_div(rate, parent_rate); > + u32 val; > > clock->div = div; > > + val = clk_readl(clock->reg) & ~CPG_DIV6_DIV_MASK; > /* Only program the new divisor if the clock isn't stopped. */ > - if (!(clk_readl(clock->reg) & CPG_DIV6_CKSTP)) > - clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg); > + if (!(val & CPG_DIV6_CKSTP)) > + clk_writel(val | CPG_DIV6_DIV(clock->div - 1), clock->reg); > > return 0; > } > @@ -120,6 +125,8 @@ static void __init cpg_div6_clock_init(struct device_node *np) > const char *parent_name; > const char *name; > struct clk *clk; > + u32 src_shift; > + u32 src_width; > int ret; > > clock = kzalloc(sizeof(*clock), GFP_KERNEL); > @@ -150,7 +157,22 @@ static void __init cpg_div6_clock_init(struct device_node *np) > goto error; > } > > - parent_name = of_clk_get_parent_name(np, 0); > + if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) { > + if (!of_property_read_u32(np, "renesas,src-width", > + &src_width)) { > + unsigned int parent_idx = > + (clk_readl(clock->reg) >> src_shift) & > + (BIT(src_width) - 1); > + parent_name = of_clk_get_parent_name(np, parent_idx); > + } else { > + pr_err("%s: renesas,src-shift without renesas,src-width in %s\n", > + __func__, np->name); > + goto error; > + } > + } else { > + parent_name = of_clk_get_parent_name(np, 0); > + } > + > if (parent_name == NULL) { > pr_err("%s: failed to get %s DIV6 clock parent name\n", > __func__, np->name); > -- > 1.8.4.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 > -- 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 Mark and Ulrich, On Thursday 28 August 2014 17:05:28 Mark Rutland wrote: > On Thu, Aug 28, 2014 at 04:11:11PM +0100, Ulrich Hecht wrote: > > From: Ulrich Hecht <ulrich.hecht@gmail.com> > > > > Support for setting the parent at initialization time based on the current > > hardware configuration in DIV6 clocks with selectable parents as found in > > the r8a73a4, r8a7740, sh73a0, and other SoCs. > > > > Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com> > > Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > > > This is v3 plus some style adjustments suggested by Laurent. > > > > CU > > Uli > > > > Changes since v3: > > - note that renesas,src-shift and renesas,src-width depend on each other > > - clarified description > > - minor coding style fixes > > > > Changes since v2: > > - add r8a73a4 to bindings > > - use u32 where appropriate > > - don't split error message > > > > Changes since v1: > > - make sure unrelated register bits are preserved > > - use the plural for the clocks property in bindings > > > > .../bindings/clock/renesas,cpg-div6-clocks.txt | 12 +++++++- > > drivers/clk/shmobile/clk-div6.c | 32 +++++++++++++---- > > 2 files changed, 38 insertions(+), 6 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt > > b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt > > index 952e373..2633ea1 100644 > > --- a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt > > +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt > > @@ -7,14 +7,24 @@ to 64. > > > > Required Properties: > > - compatible: Must be one of the following > > + - "renesas,r8a73a4-div6-clock" for R8A73A4 (R-Mobile APE6) DIV6 > > clocks > > + - "renesas,r8a7740-div6-clock" for R8A7740 (R-Mobile A1) DIV6 clocks > > - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks > > - "renesas,r8a7791-div6-clock" for R8A7791 (R-Car M2) DIV6 clocks > > + - "renesas,sh73a0-div6-clock" for SH73A0 (SH-MobileAG5) DIV6 clocks > > - "renesas,cpg-div6-clock" for generic DIV6 clocks > > > > - reg: Base address and length of the memory resource used by the DIV6 > > clock > > > > - - clocks: Reference to the parent clock > > + - clocks: Reference to the parent clock(s) > > - #clock-cells: Must be 0 > > - clock-output-names: The name of the clock as a free-form string > > > > +Optional Properties: > > + > > + - renesas,src-shift: Bit position of the input clock selector (default: > > + fixed input clock; requires renesas,src-width) > > + - renesas,src-width: Bit width of the input clock selector (default: > > fixed > > + input clock; requires renesas,src-shift) > > I'm slightly confused by these properties, but I'm not at all familiar > with the HW. > > How variable is the format of the configuration register? > > Is it fixed for a given compatible string? > > Are there other fields we care about? On SH73A0 the DIV6-compatible clocks have different register layouts. Bits Name Function ----------------------------------------------------------------------------- 14-12 EXSRC Source Selection (VCLK[123] clocks) 8 CKSTP Clock Stop 7 EXSRC Source Selection (BSC, FL, MSU, MFG[12], DSITX clocks) 7-6 EXSRC Source Selection (SD[012], FSI[AB], SPUA, SUPV, HSI clocks) 7-6 PDIV PLL Division Ratio (VCLK3 clock) 5-0 DIV Division Ratio If we consider the EXSRC on bit 7 as a special case of EXSRC on bits 7-6, and the PDIV field as being present for all VCLK clocks with the only valid value being 0 for VCLK1 and VCLK2, this splits the DIV6 clocks in two categories, the VCLK clocks and the other clocks. On R8A73A4 the situation is similar, with three categories: - EXSRC on bits 7-6 (BSC, DSI[01]TX, SD[012], MMC[01], FSI[AB], SPUV, SLIMB, HSI, SSP, SSPRS, MPHY, C2C) - EXSRC on bits 14-12 and PDIV on bits 7-6 (VCLK[12345]) - no EXSRC (HSIC) Similarly R8A7740 has the following categories: - EXSRC on bits 7-6 (FMSI, FMSO, FSI[AB], SPU, VOU) - EXSRC on bits 14-12 and no PDIV bits (VCLK[12]) - no EXSRC (STPR) On R8A7790 and R8A7791 all the DIV6-compatible clocks (SD[123], MMC[01], SSP, SSPRS) share the same register layout with just the CKSTP and DIV bits. We could thus extend the DIV6 clocks with clock source selection using the following layout Bits Name Function ----------------------------------------------------------------------------- 8 CKSTP Clock Stop 7-6 EXSRC Source Selection 5-0 DIV Division Ratio The clocks property would be a list of one, two, three or four clock references, mapping to the 00, 01, 10 and 11 values of the EXSRC field. Empty references (just a 0 value) would be used to denote invalid values of the EXSRC field. A new compatible string would then be added for the VCLK clocks with the following layout Bits Name Function ----------------------------------------------------------------------------- 14-12 EXSRC Source Selection (VCLK[123] clocks) 8 CKSTP Clock Stop 7-6 PDIV PLL Division Ratio (VCLK3 clock) 5-0 DIV Division Ratio The clocks property would be handled as for the common DIV6 clocks, and we would need to define a way to describe the valid values of the PDIV field. As far as I can see, the PDIV values are identical for all VCLK clocks when implemented (00 -> 1/1, 01 -> 1/2, 10 -> Reserved, 11 -> 1/4), so a single renesas,has-pdiv property should be enough.
Quoting Ulrich Hecht (2014-08-28 08:11:11) > From: Ulrich Hecht <ulrich.hecht@gmail.com> > > Support for setting the parent at initialization time based on the current > hardware configuration in DIV6 clocks with selectable parents as found in > the r8a73a4, r8a7740, sh73a0, and other SoCs. <snip> > - parent_name = of_clk_get_parent_name(np, 0); > + if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) { > + if (!of_property_read_u32(np, "renesas,src-width", > + &src_width)) { > + unsigned int parent_idx = > + (clk_readl(clock->reg) >> src_shift) & > + (BIT(src_width) - 1); > + parent_name = of_clk_get_parent_name(np, parent_idx); Can the clock source be selected at run-time? Is there a use case for this? If so it is probably better to actually model these clocks as multiplexers with the corresponding .get_parent and .set_parent callbacks. Regards, Mike -- 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 Fri, Aug 29, 2014 at 1:58 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: [...] > We could thus extend the DIV6 clocks with clock source selection using the > following layout > > Bits Name Function > ----------------------------------------------------------------------------- > 8 CKSTP Clock Stop > 7-6 EXSRC Source Selection > 5-0 DIV Division Ratio > > The clocks property would be a list of one, two, three or four clock > references, mapping to the 00, 01, 10 and 11 values of the EXSRC field. Empty > references (just a 0 value) would be used to denote invalid values of the > EXSRC field. > > A new compatible string would then be added for the VCLK clocks with the > following layout > > Bits Name Function > ----------------------------------------------------------------------------- > 14-12 EXSRC Source Selection (VCLK[123] clocks) > 8 CKSTP Clock Stop > 7-6 PDIV PLL Division Ratio (VCLK3 clock) > 5-0 DIV Division Ratio I think even a new compatible string would not be necessary, given that all such clocks have more than four parents, which cannot be the case for clocks with two EXSRC bits. > The clocks property would be handled as for the common DIV6 clocks, and we > would need to define a way to describe the valid values of the PDIV field. As > far as I can see, the PDIV values are identical for all VCLK clocks when > implemented (00 -> 1/1, 01 -> 1/2, 10 -> Reserved, 11 -> 1/4), so a single > renesas,has-pdiv property should be enough. Sounds reasonable to me. I'll send a patch. CU Uli -- 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/renesas,cpg-div6-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt index 952e373..2633ea1 100644 --- a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt @@ -7,14 +7,24 @@ to 64. Required Properties: - compatible: Must be one of the following + - "renesas,r8a73a4-div6-clock" for R8A73A4 (R-Mobile APE6) DIV6 clocks + - "renesas,r8a7740-div6-clock" for R8A7740 (R-Mobile A1) DIV6 clocks - "renesas,r8a7790-div6-clock" for R8A7790 (R-Car H2) DIV6 clocks - "renesas,r8a7791-div6-clock" for R8A7791 (R-Car M2) DIV6 clocks + - "renesas,sh73a0-div6-clock" for SH73A0 (SH-MobileAG5) DIV6 clocks - "renesas,cpg-div6-clock" for generic DIV6 clocks - reg: Base address and length of the memory resource used by the DIV6 clock - - clocks: Reference to the parent clock + - clocks: Reference to the parent clock(s) - #clock-cells: Must be 0 - clock-output-names: The name of the clock as a free-form string +Optional Properties: + + - renesas,src-shift: Bit position of the input clock selector (default: + fixed input clock; requires renesas,src-width) + - renesas,src-width: Bit width of the input clock selector (default: fixed + input clock; requires renesas,src-shift) + Example ------- diff --git a/drivers/clk/shmobile/clk-div6.c b/drivers/clk/shmobile/clk-div6.c index f065f69..f8b57bf 100644 --- a/drivers/clk/shmobile/clk-div6.c +++ b/drivers/clk/shmobile/clk-div6.c @@ -39,8 +39,11 @@ struct div6_clock { static int cpg_div6_clock_enable(struct clk_hw *hw) { struct div6_clock *clock = to_div6_clock(hw); + u32 val; - clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg); + val = (clk_readl(clock->reg) & ~(CPG_DIV6_DIV_MASK | CPG_DIV6_CKSTP)) + | CPG_DIV6_DIV(clock->div - 1); + clk_writel(val, clock->reg); return 0; } @@ -52,7 +55,7 @@ static void cpg_div6_clock_disable(struct clk_hw *hw) /* DIV6 clocks require the divisor field to be non-zero when stopping * the clock. */ - clk_writel(CPG_DIV6_CKSTP | CPG_DIV6_DIV(CPG_DIV6_DIV_MASK), + clk_writel(clk_readl(clock->reg) | CPG_DIV6_CKSTP | CPG_DIV6_DIV_MASK, clock->reg); } @@ -94,12 +97,14 @@ static int cpg_div6_clock_set_rate(struct clk_hw *hw, unsigned long rate, { struct div6_clock *clock = to_div6_clock(hw); unsigned int div = cpg_div6_clock_calc_div(rate, parent_rate); + u32 val; clock->div = div; + val = clk_readl(clock->reg) & ~CPG_DIV6_DIV_MASK; /* Only program the new divisor if the clock isn't stopped. */ - if (!(clk_readl(clock->reg) & CPG_DIV6_CKSTP)) - clk_writel(CPG_DIV6_DIV(clock->div - 1), clock->reg); + if (!(val & CPG_DIV6_CKSTP)) + clk_writel(val | CPG_DIV6_DIV(clock->div - 1), clock->reg); return 0; } @@ -120,6 +125,8 @@ static void __init cpg_div6_clock_init(struct device_node *np) const char *parent_name; const char *name; struct clk *clk; + u32 src_shift; + u32 src_width; int ret; clock = kzalloc(sizeof(*clock), GFP_KERNEL); @@ -150,7 +157,22 @@ static void __init cpg_div6_clock_init(struct device_node *np) goto error; } - parent_name = of_clk_get_parent_name(np, 0); + if (!of_property_read_u32(np, "renesas,src-shift", &src_shift)) { + if (!of_property_read_u32(np, "renesas,src-width", + &src_width)) { + unsigned int parent_idx = + (clk_readl(clock->reg) >> src_shift) & + (BIT(src_width) - 1); + parent_name = of_clk_get_parent_name(np, parent_idx); + } else { + pr_err("%s: renesas,src-shift without renesas,src-width in %s\n", + __func__, np->name); + goto error; + } + } else { + parent_name = of_clk_get_parent_name(np, 0); + } + if (parent_name == NULL) { pr_err("%s: failed to get %s DIV6 clock parent name\n", __func__, np->name);