[PATCH v4] clk: shmobile: div6: support selectable-input clocks

[PATCH v4] clk: shmobile: div6: support selectable-input clocks

[Ulrich Hecht]

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)
+
 
 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

Re: [PATCH v4] clk: shmobile: div6: support selectable-input clocks

[Mark Rutland]

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
> 

Re: [PATCH v4] clk: shmobile: div6: support selectable-input clocks

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4] clk: shmobile: div6: support selectable-input clocks

[Ulrich Hecht]

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

Re: [PATCH v4] clk: shmobile: div6: support selectable-input clocks

[Mike Turquette]

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