How can bad driver (and DeviceTree) design can make your life extremely difficult

I will give you an example of what a bad driver design (and consequent DeviceTree and bindings) could be, and how it can seriously impact a developer's life and good intentions.

Here is a snippet of a DT node that appears quite harmless :

power-domain@MT8186_POWER_DOMAIN_IMG {
	reg = <MT8186_POWER_DOMAIN_IMG>;
	clocks = <&topckgen CLK_TOP_IMG1>,
		 <&imgsys1 CLK_IMG1_GALS_IMG1>;
	clock-names = "img-top", "gals";
	mediatek,infracfg = <&infracfg_ao>;
	#address-cells = <1>;
	#size-cells = <0>;
	#power-domain-cells = <1>;
        [...]
};        

We have a node who basically has two clocks as requirement.

And then we have such code written:

num_clks = of_clk_get_parent_count(node);
if (num_clks > 0) {
	/* Calculate number of subsys_clks */
	of_property_for_each_string(node, "clock-names", prop, clk_name) {
		char *subsys;

		subsys = strchr(clk_name, '-');
		if (subsys)
			pd->num_subsys_clks++;
		else
			pd->num_clks++;
	}
}
        

The surprise is that the clocks having a dash in their name appear to be saved differently. And then, the next snippet:

for (i = 0; i < pd->num_clks; i++) {
	clk = of_clk_get(node, i);
        [...]
	pd->clks[clk_ind++].clk = clk;
}

for (i = 0; i < pd->num_subsys_clks; i++) {
	clk = of_clk_get(node, i + clk_ind);
	[...]
	pd->subsys_clks[i].clk = clk;
}        

So, the driver asks for subsys_clks by index, starting from the number of the other clks (clk_ind which is computed first, and then of_clk_get will ask for it )

In our case, the img-top is treated as clks, and gals as subsys_clks, even if the intention was different, as img-top contains the dash.

So, enforcing an order inside a list of clocks, that is totally implementation specific, a bit random, and not complying with any kind of dedicated specification, except a small text which is not formalized inside the DT binding schema copied below, can lead to a total disaster as you can see in the example above.

clock-names:                                                               
        description: |                                                           
          List of names of clocks, in order to match the power-up
 sequencing for each power domain we need to group the clocks by
 name. BASIC clocks need to be enabled before enabling the 
corresponding power domain, and should not have a '-' in their name
 (i.e mm, mfg, venc).   
          SUSBYS clocks need to be enabled before releasing the bus
 protection, and should contain a '-' in their name (i.e mm-0,
 isp-0, cam-0).                                                                     
          In order to follow properly the power-up sequencing, the 
clocks must be specified by order, adding first the BASIC clocks
 followed by the SUSBSYS clocks.         

I would definitely say this is not a way of implementing a list of clocks, which can very easily lead to unexpected behavior that would be extremely hard to debug.

#ARM #devicetree #clocks #linux #driver #embedded #embeddedlinux