Hi Stephen, > -----Original Message----- > From: Stephen Warren [mailto:swar...@wwwdotorg.org] > Sent: 2016年9月3日 1:38 > To: Wenyou Yang - A41535 <wenyou.y...@microchip.com> > Cc: swar...@nvidia.com; u-boot@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.y...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
