* Re: [PATCH 1/4] clk: shmobile: div6: support selectable-input clocks
2014-04-22 13:19 [PATCH 1/4] clk: shmobile: div6: support selectable-input clocks Ulrich Hecht
@ 2014-04-30 23:55 ` Laurent Pinchart
0 siblings, 0 replies; 2+ messages in thread
From: Laurent Pinchart @ 2014-04-30 23:55 UTC (permalink / raw)
To: linux-sh
Hi Ulrich,
Thank you for the patch.
On Tuesday 22 April 2014 15:19:09 Ulrich Hecht wrote:
> From: Ulrich Hecht <ulrich.hecht@gmail.com>
>
> Adds support for DIV6 clocks with selectable parents as found in the r8a7740
> and other SoCs.
>
> Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com>
> ---
> .../bindings/clock/renesas,cpg-div6-clocks.txt | 8 ++++++++
> drivers/clk/shmobile/clk-div6.c | 19 +++++++++++++++-
> 2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git
> a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt index
> 952e373..f8302f5 100644
> --- a/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-div6-clocks.txt
> @@ -7,6 +7,7 @@ to 64.
> Required Properties:
>
> - compatible: Must be one of the following
> + - "renesas,r8a7740-div6-clock" for R8A7740 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,cpg-div6-clock" for generic DIV6 clocks
> @@ -15,6 +16,13 @@ Required Properties:
> - #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)
> + - renesas,src-width: Bit width of the input clock selector (default:
> fixed
> + input clock)
Now that the driver supports multiple input clocks, you also need to update
the documentation of the clocks field which currently reads
"- clocks: Reference to the parent clock"
We also need to think about what to do when some values of the source field
are reserved. For instance, if the source field is two bits large, and only
value 00 and 10 are valid, the clocks field should probably contain something
like
clocks = <&clk_src_0 3>, <0>, <&clk_src_3 2>, <0>;
I'm not sure whether "NULL" clock entries are supported though. They're not
documented in Documentation/devicetree/bindings/clock/clock-bindings.txt but
could be supported by the code. At least the common clock bindings
documentation should be fixed. I think a patch has been posted some time ago
on LAKML.
One possible issue with this implementation is that large source fields with
only a couple of valid values will require lots of NULL clock entries. Another
option would be to use clock-names with the index number set as the name. The
above example would become
clocks = <&clk_src_0 3>, <&clk_src_3 2>;
clock-names = "0", "2";
> Example
> -------
> diff --git a/drivers/clk/shmobile/clk-div6.c
> b/drivers/clk/shmobile/clk-div6.c index f065f69..5be8161 100644
> --- a/drivers/clk/shmobile/clk-div6.c
> +++ b/drivers/clk/shmobile/clk-div6.c
> @@ -121,6 +121,7 @@ static void __init cpg_div6_clock_init(struct
> device_node *np) const char *name;
> struct clk *clk;
> int ret;
> + u32 src_shift, src_width;
>
> clock = kzalloc(sizeof(*clock), GFP_KERNEL);
> if (!clock) {
> @@ -150,7 +151,23 @@ 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);
> +
I'd move that to a separate cpg_div6_get_parent_index() function to make the
code cleaner. Something like
static int *cpg_div6_get_parent_index(struct div6_clock *clock,
struct device_node *np)
{
u32 shift, width;
int ret;
ret = of_property_read_u32(np, "renesas,src-shift", &shift);
if (ret < 0)
return ret;
ret = of_property_read_u32(np, "renesas,src-width", &width);
if (ret < 0)
pr_err("%s: renesas,src-shift without renesas,src-width in %s\n",
__func__, np->name);
return ret;
}
return clk_readl(clock->reg) >> shift) & (BIT(width) - 1);
}
This only supports reading the current source. Implementing support for
modifying the source shouldn't be too difficult. I don't think we need to
implement it right now, but I believe we at least need to give it a thought.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 2+ messages in thread