[PATCH 5/8] ARM: zynq: add COMMON_CLK support

[Lars-Peter Clausen]

On 11/02/2012 02:38 PM, Josh Cartwright wrote:
> Thanks for the review.
> 
> On Fri, Nov 02, 2012 at 10:33:44AM +0100, Lars-Peter Clausen wrote:
>> On 10/31/2012 07:58 PM, Josh Cartwright wrote:
>>> [...]
>>> +#define PERIPH_CLK_CTRL_SRC(x)	(periph_clk_parent_map[((x)&3)>>4])
>>> +#define PERIPH_CLK_CTRL_DIV(x)	(((x)&0x3F00)>>8)
>>
>> A few more spaces wouldn't hurt ;)
> 
> Okay, sure.
> 
>>> [...]
>>> +static void __init zynq_periph_clk_setup(struct device_node *np)
>>> +{
>>> +	struct zynq_periph_clk *periph;
>>> +	const char *parent_names[3];
>>> +	struct clk_init_data init;
>>> +	struct clk *clk;
>>> +	int err;
>>> +	u32 reg;
>>> +	int i;
>>> +
>>> +	err = of_property_read_u32(np, "reg", &reg);
>>> +	WARN_ON(err);
>>
>> Shouldn't the function abort if a error happens somewhere? Continuing here
>> will lead to undefined behavior. Same is probably true for the other WARN_ONs.
> 
> The way I see it is: the kernel is will be left in a bad state in the
> case of any failure, regardless of if we bail out or continue.  AFAICT,
> there is no clean way to recover from a failure this early.
> 
> Given that, it seems simpler (albeit marginally so) just to continue; so
> that's what I chose to do.  I'm not opposed to bailing out, just not
> convinced it does anything for us.
> 

The issue with this approach is that, while you get a warning, unexpected
seemingly unrelated side-effects may happen later on. E.g. if no reg
property for the clock is specified the reg variable will be uninitialized
and contain whatever was on the stack before. The clock will be registered
nonetheless and the boot process continues. Now if the clock is enabled a
bit in a random register will be modified, which could result in strange and
abnormal behavior, which can be very hard to track down.

Also if for example just one clock has its reg property missing the system
will continue to boot if we bail out here. It is just that the peripherals
using that clock won't work. Which will certainly be easier to diagnose than
random abnormal behavior.

>>> +
>>> +	periph = kzalloc(sizeof(*periph), GFP_KERNEL);
>>> +	WARN_ON(!periph);
>>> +
>>> +	periph->clk_ctrl = slcr_base + reg;
>>> +	spin_lock_init(&periph->clkact_lock);
>>> +
>>> +	init.name = np->name;
>>> +	init.ops = &zynq_periph_clk_ops;
>>> +	for (i = 0; i < ARRAY_SIZE(parent_names); i++)
>>> +		parent_names[i] = of_clk_get_parent_name(np, i);
>>> +	init.parent_names = parent_names;
>>> +	init.num_parents = ARRAY_SIZE(parent_names);
>>> +
>>> +	periph->hw.init = &init;
>>> +
>>> +	clk = clk_register(NULL, &periph->hw);
>>> +	WARN_ON(IS_ERR(clk));
>>> +
>>> +	err = of_clk_add_provider(np, of_clk_src_simple_get, clk);
>>> +	WARN_ON(err);
>>> +
>>> +	for (i = 0; i < 2; i++) {
>>
>> Not all of the peripheral clock generators have two output clocks. I think
>> it makes sense to use the number entries in clock-output-names here.
> 
> Yes, I agree.  I'll also update the bindings documentation.
> 
> Thanks again,
>   Josh