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

[Wenyou.Yang at microchip.com]

Hi Stephen,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> Sent: 2016年9月3日 1:38
> To: Wenyou Yang - A41535 <Wenyou.Yang at microchip.com>
> Cc: swarren at nvidia.com; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH v1 2/2] clk: at91: Add .ops callback for clk_generic
> 
> 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.

I sent a patch set to mail-list to solve this issue,
http://lists.denx.de/pipermail/u-boot/2016-September/266398.html

Could you help me review them, thank you very much.


Best Regards,
Wenyou Yang