Re: [PATCH] clk: ti: clkctrl: Fix hidden dependency to node name with reg-names

[Tero Kristo <t-kristo@ti.com>]

On 06/09/2019 00:55, Tony Lindgren wrote:
> We currently have a hidden dependency to the device tree node name for
> the clkctrl clocks. Instead of using standard node name like "clock", we
> must use "l4-per-clkctrl" naming so the clock driver can find the
> associated clock domain. Further, if "clk" is specified for a clock node
> name, the driver sets TI_CLK_CLKCTRL_COMPAT flag that uses different
> logic with earlier naming for the clock node name.
> 
> If the clock node naming dependency is not understood, the related
> clockdomain is not found, or a wrong one can get used if a clock manager
> instance has multiple domains.
> 
> As each clkctrl instance represents a single clock domain with it's
> reg property describing the clocks available in that clock domain,
> we can simply use "reg-names" property for the clock domain.
> 
> This simplifies things and removes the hidden dependency to the node
> name. And then later on, we should be able to drop the related code
> for parsing the node names.
> 
> Let's also update the binding to use standard "clock" node naming
> instead of "clk".
> 
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   Documentation/devicetree/bindings/clock/ti-clkctrl.txt |  6 +++++-
>   drivers/clk/ti/clkctrl.c                               | 10 ++++++++--
>   2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> --- a/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> +++ b/Documentation/devicetree/bindings/clock/ti-clkctrl.txt
> @@ -20,15 +20,19 @@ Required properties :
>   - #clock-cells : shall contain 2 with the first entry being the instance
>   		 offset from the clock domain base and the second being the
>   		 clock index
> +- reg : clock registers
> +- reg-names : clock register names for the clock, should be same as the
> +	      domain name

Hmm, I think using the reg-names property like this is kind of wrong. 
Basically, reg and reg-names have pretty much nothing in common. 
Shouldn't you instead use something like ti,clkdm-name? This also breaks 
with SoCs like am3, which have mutant clkctrl entries like the one here:

                 l4ls_clkctrl: l4ls-clkctrl@38 {
                         compatible = "ti,clkctrl";
                         reg = <0x38 0x2c>, <0x6c 0x28>, <0xac 0xc>, 
<0xc0 0x1c>, <0xec 0xc>, <0x10c 0x8>, <0x130 0x4>;
                         #clock-cells = <2>;
                 };

What would you think single entry in reg-names would mean in this case?

Other than that, I think the approach taken in this patch looks fine.

-Tero

>   
>   Example: Clock controller node on omap 4430:
>   
>   &cm2 {
>   	l4per: cm@1400 {
>   		cm_l4per@0 {
> -			cm_l4per_clkctrl: clk@20 {
> +			cm_l4per_clkctrl: clock@20 {
>   				compatible = "ti,clkctrl";
>   				reg = <0x20 0x1b0>;
> +				reg-names = "l4_per";
>   				#clock-cells = <2>;
>   			};
>   		};
> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
> --- a/drivers/clk/ti/clkctrl.c
> +++ b/drivers/clk/ti/clkctrl.c
> @@ -446,6 +446,7 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
>   	struct clk_hw_omap *hw;
>   	struct clk *clk;
>   	struct omap_clkctrl_clk *clkctrl_clk;
> +	const char *clkdm_name;
>   	const __be32 *addrp;
>   	u32 addr;
>   	int ret;
> @@ -534,7 +535,12 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
>   
>   	provider->base = of_iomap(node, 0);
>   
> -	if (ti_clk_get_features()->flags & TI_CLK_CLKCTRL_COMPAT) {
> +	ret = of_property_read_string_index(node, "reg-names", 0, &clkdm_name);
> +	if (!ret) {
> +		provider->clkdm_name = kasprintf(GFP_KERNEL, "%s_clkdm",
> +						 clkdm_name);
> +		goto clkdm_found;
> +	} else if (ti_clk_get_features()->flags & TI_CLK_CLKCTRL_COMPAT) {
>   		provider->clkdm_name = kasprintf(GFP_KERNEL, "%pOFnxxx", node->parent);
>   		if (!provider->clkdm_name) {
>   			kfree(provider);
> @@ -570,7 +576,7 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
>   			*c = '_';
>   		c++;
>   	}
> -
> +clkdm_found:
>   	INIT_LIST_HEAD(&provider->clocks);
>   
>   	/* Generate clocks */
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki