[U-Boot] [PATCH v1 2/2] clk: at91: Add .ops callback for clk_generic

[Stephen Warren]

On 09/01/2016 07:46 PM, Wenyou.Yang at microchip.com wrote:
> Hi Stephen,
>
>>> Subject: [PATCH v1 2/2] clk: at91: Add .ops callback for clk_generic
>>>
>>> To avoid the wild pointer as NULL->of_xlate, add an empty .ops
>>> callback for the clk_generic driver.
>>
>> This shouldn't be needed. If this driver isn't a clock provider, and it cannot be a
>> clock provider without implementing and clock ops, then nothing should be calling
>> of_xlate through its ops pointer, and the driver shouldn't be a UCLASS_CLK.
>
> The Atmel clock tree structure has some difference, take the following spi0 node as an example.
> The 'spi0' node use the 'clocks' phandle to refer to the 'spi0_clk' node to enable its clock.
>
> The 'spi0_clk' doesn't provide .ops->enable(), only its peripheral ID via 'reg' property.
> It uses its parent node (i.e. periph32ck) .ops->enable(), shared with other sibling nodes.
>
> These nodes as 'spi0_clk' don't have .compatible, which is bound with the 'clk_generic' driver.
> If not provide .ops for the 'clk_generic' driver, it will produce the wild pointer as NULL->of_xlate
> when call clk_get_by_index() in the spi driver.

The way clocks are looked in in DT currently requires that:

1) The phandle in the clocks property is parsed. That is the node ID of 
&spi0_clk is found.

2) All devices of UCLASS_CLK are search to find the device with 
.of_offset that matches the phandle parsed in (1) above.

3) That device's driver's uclass ops of_xlate is called to translate the 
rest of the client node's clock specifier (in your case, there are 0 
cells, so no data is extracted, but the U-Boot core clock node doesn't 
know that, since this is binding-specific).

Thus, there /must/ be a struct udevice for the spi0_clk node, and it 
must have an ops pointer, and either a working of_xlate pointer or NULL 
to use the default xlate function.

This is all simply how the code works; your driver must fit into this 
mechanism.

Now, the one way your driver would be able to defer all clock operations 
to the driver for the periph32ck node is if the of_xlate for the 
spi0_clk node were to edit the struct clk's .dev field to point it back 
at the driver for the periph32ck node, and set a value in struct clk's 
.id field that is hard-coded rather than derived from the client's DT 
clock specifier. However, I know you aren't using that technique, since 
your patch relies on the default of_xlate function for the spi0_clk node 
(since you provide an ops pointer, but don't fill in any of the function 
pointers within it, and hence the of_xlate pointer defaults to NULL, and 
hence clk_get_by_index() uses clk_of_xlate_default().

So here's how you need to make it work:

1)

A driver should exist for the periph32ck node. This driver's uclass is 
likely *not* UCLASS_CLK since it doesn't provide any clocks; there is no 
#clock-cells property in ths node in DT.

This driver needs to somehow create a device for each child node, so 
that the clock core can find those devices.

2)

The device for the spi0_clk node needs to be UCLASS_CLK, needs to 
provide an ops pointer, and can either use the default of_xlate or 
provide a custom one if needed. request and free ops are also required.

In particular, the ops pointer for this device *must* provide some other 
clock ops so that clients can do something useful with the clock. At 
least one of enable, disable, get_rate, set_rate are required.

Now, the *implementation* of this driver can call functions in the 
parent driver if you need, so that all the code is in one place. There's 
nothing preventing that.