Re: [U-Boot] [PATCH v1 2/2] clk: at91: Add .ops callback for clk_generic
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:
- The phandle in the clocks property is parsed. That is the node ID of &spi0_clk
is found.
- All devices of UCLASS_CLK are search to find the device with .of_offset that
matches the phandle parsed in (1) above.
- 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:
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.
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:
- The phandle in the clocks property is parsed. That is the node ID of &spi0_clk
is found.
- All devices of UCLASS_CLK are search to find the device with .of_offset that
matches the phandle parsed in (1) above.
- 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:
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.
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
participants (2)
-
Stephen Warren -
Wenyou.Yang＠microchip.com