| Message ID | 1472544698-11711-3-git-send-email-wenyou.yang@atmel.com |
|---|---|
| State | Superseded |
| Delegated to: | Andreas Bießmann |
| Headers | show |
On 08/30/2016 02:11 AM, Wenyou Yang wrote: > 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.
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. periph32ck { compatible = "atmel,at91sam9x5-clk-peripheral"; #address-cells = <1>; #size-cells = <0>; clocks = <&h32ck>; [snip] twi0_clk: twi0_clk { reg = <29>; #clock-cells = <0>; atmel,clk-output-range = <0 83000000>; }; [snip] spi0_clk: spi0_clk { #clock-cells = <0>; reg = <33>; atmel,clk-output-range = <0 83000000>; }; [snip] }; [snip] spi0: spi@f8000000 { compatible = "atmel,at91rm9200-spi"; reg = <0xf8000000 0x100>; clocks = <&spi0_clk>; clock-names = "spi_clk"; }; Thanks. Best Regards, Wenyou Yang
On 09/01/2016 07:46 PM, Wenyou.Yang@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.
Hi Stephen, > -----Original Message----- > From: Stephen Warren [mailto:swarren@wwwdotorg.org] > Sent: 2016年9月3日 1:38 > To: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com> > Cc: swarren@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.Yang@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. Thank you very much for your direction. Best Regards, Wenyou Yang
Hi Stephen, > -----Original Message----- > From: Stephen Warren [mailto:swarren@wwwdotorg.org] > Sent: 2016年9月3日 1:38 > To: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com> > Cc: swarren@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.Yang@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
diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c index 76ff387..1043148 100644 --- a/drivers/clk/at91/pmc.c +++ b/drivers/clk/at91/pmc.c @@ -59,7 +59,10 @@ int at91_pmc_clk_node_bind(struct udevice *dev) return 0; } +static struct clk_ops generic_clk_ops; + U_BOOT_DRIVER(clk_generic) = { .id = UCLASS_CLK, .name = "clk", + .ops = &generic_clk_ops, };
To avoid the wild pointer as NULL->of_xlate, add an empty .ops callback for the clk_generic driver. Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com> --- drivers/clk/at91/pmc.c | 3 +++ 1 file changed, 3 insertions(+)