clk: keystone: add sci clock support
Subsystems (4)
cover clk: keystone: add sci clock support
Hi,
Version 3 has following changes compared to v2 [1]:
- rebased on top of 4.9-rc1
- dropped some of the controversial DT properties like ti,ssc-clocks etc.
- driver now registers all the SCI clocks during probe time, for this, there is an array built-in to the driver with knowledge of valid clocks for the device
- xlate only picks up the clocks registered during probe
This series depends on the base SCI protocol support series [2] and the TI-SCI generic PM domain support series for the device ID include file [3].
Quick boot test seems to be fine.
-Tero
[1] http://www.spinics.net/lists/devicetree/msg141319.html
[2] http://www.spinics.net/lists/devicetree/msg146621.html
[3] http://www.spinics.net/lists/arm-kernel/msg536851.html
Tero Kristo (3):
Documentation: dt: Add TI SCI clock driver
dt-binding: clock: Add k2g clock definitions
clk: keystone: Add sci-clk driver support
.../devicetree/bindings/clock/ti,sci-clk.txt | 37 ++ MAINTAINERS | 3 + drivers/clk/Kconfig | 9 + drivers/clk/keystone/Makefile | 1 + drivers/clk/keystone/sci-clk.c | 589 +++++++++++++++++++++ include/dt-bindings/clock/k2g.h | 234 ++++++++ 6 files changed, 873 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/ti,sci-clk.txt create mode 100644 drivers/clk/keystone/sci-clk.c create mode 100644 include/dt-bindings/clock/k2g.h
Signature · 1 line
1.9.1
PATCH 1/3 Documentation: dt: Add TI SCI clock driver
Add a clock implementation, TI SCI clock, that will hook to the common clock framework, and allow each clock to be controlled via TI SCI protocol.
--- .../devicetree/bindings/clock/ti,sci-clk.txt | 37 ++++++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 38 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/ti,sci-clk.txt
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +37 -0
@@ -0,0 +1,37 @@ +Texas Instruments TI-SCI Clocks +=============================== + +All clocks on Texas Instruments' SoCs that contain a System Controller, +are only controlled by this entity. Communication between a host processor +running an OS and the System Controller happens through a protocol known +as TI-SCI[1]. This clock implementation plugs into the common clock +framework and makes use of the TI-SCI protocol on clock API requests. + +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt + +Required properties: +------------------- +- compatible: Must be "ti,k2g-sci-clk" +- #clock-cells: Shall be 2. + In clock consumers, this cell represents the device ID and clock ID + exposed by the PM firmware. The assignments can be found in the header + files <dt-bindings/genpd/<soc>.h> (which covers the device IDs) and + <dt-bindings/clock/<soc>.h> (which covers the clock IDs), where <soc> + is the SoC involved, for example 'k2g'. + +Examples: +-------- + +pmmc: pmmc { + compatible = "ti,k2g-sci"; + + k2g_clks: k2g_clks { + compatible = "ti,k2g-sci-clk"; + #clock-cells = <2>; + }; +}; + +uart0: serial at 2530c00 { + compatible = "ns16550a"; + clocks = <&k2g_clks K2G_DEV_UART0 0>; +};
MAINTAINERS +1 -0
@@ -11895,6 +11895,7 @@ F: include/linux/soc/ti/ti_sci_protocol.h F: Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt F: include/dt-bindings/genpd/k2g.h F: drivers/soc/ti/ti_sci_pm_domains.c +F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER M: Hans Verkuil <[email protected]>
Signature · 1 line
1.9.1
Rob Herring
maintainerPATCH 1/3 Documentation: dt: Add TI SCI clock driver
Quoted 35 lines · Tero Kristo
Add a clock implementation, TI SCI clock, that will hook to the common clock framework, and allow each clock to be controlled via TI SCI protocol.
Signed-off-by Tero Kristo <[email protected]>--- .../devicetree/bindings/clock/ti,sci-clk.txt | 37 ++++++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 38 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/ti,sci-clk.txtDocumentation/devicetree/bindings/clock/ti,sci-clk.txt new file +18
-0@@ -0,0 +1,37 @@ +Texas Instruments TI-SCI Clocks +=============================== + +All clocks on Texas Instruments' SoCs that contain a System Controller, +are only controlled by this entity. Communication between a host processor +running an OS and the System Controller happens through a protocol known +as TI-SCI[1]. This clock implementation plugs into the common clock +framework and makes use of the TI-SCI protocol on clock API requests. + +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt + +Required properties: +------------------- +- compatible: Must be "ti,k2g-sci-clk" +- #clock-cells: Shall be 2. + In clock consumers, this cell represents the device ID and clock ID + exposed by the PM firmware. The assignments can be found in the header + files <dt-bindings/genpd/<soc>.h> (which covers the device IDs) and
Documentation/devicetree/bindings/clock/ti,sci-clk.txt line 28
Quoted 10 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +10
-0+ <dt-bindings/clock/<soc>.h> (which covers the clock IDs), where <soc> + is the SoC involved, for example 'k2g'. + +Examples: +-------- + +pmmc: pmmc { + compatible = "ti,k2g-sci"; + + k2g_clks: k2g_clks {
Use "clocks" for node name instead.
Documentation/devicetree/bindings/clock/ti,sci-clk.txt line 29
Quoted 1 line
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +1
-0+ compatible = "ti,k2g-sci-clk";
I'm starting to think all these child nodes for SCI are pointless. Is there any reason why the parent node can't be the clock provider (along with all the other providers it acks as)?
Quoted 8 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +8
-0+ #clock-cells = <2>; + }; +}; + +uart0: serial at 2530c00 { + compatible = "ns16550a"; + clocks = <&k2g_clks K2G_DEV_UART0 0>; +};
PATCH 1/3 Documentation: dt: Add TI SCI clock driver
Quoted 49 lines · Rob Herring
Quoted 35 lines · Tero Kristo
Add a clock implementation, TI SCI clock, that will hook to the common clock framework, and allow each clock to be controlled via TI SCI protocol.
Signed-off-by Tero Kristo <[email protected]>--- .../devicetree/bindings/clock/ti,sci-clk.txt | 37 ++++++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 38 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/ti,sci-clk.txtDocumentation/devicetree/bindings/clock/ti,sci-clk.txt new file +18
-0@@ -0,0 +1,37 @@ +Texas Instruments TI-SCI Clocks +=============================== + +All clocks on Texas Instruments' SoCs that contain a System Controller, +are only controlled by this entity. Communication between a host processor +running an OS and the System Controller happens through a protocol known +as TI-SCI[1]. This clock implementation plugs into the common clock +framework and makes use of the TI-SCI protocol on clock API requests. + +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt + +Required properties: +------------------- +- compatible: Must be "ti,k2g-sci-clk" +- #clock-cells: Shall be 2. + In clock consumers, this cell represents the device ID and clock ID + exposed by the PM firmware. The assignments can be found in the header + files <dt-bindings/genpd/<soc>.h> (which covers the device IDs) andDocumentation/devicetree/bindings/clock/ti,sci-clk.txt line 28
Quoted 10 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +10
-0+ <dt-bindings/clock/<soc>.h> (which covers the clock IDs), where <soc> + is the SoC involved, for example 'k2g'. + +Examples: +-------- + +pmmc: pmmc { + compatible = "ti,k2g-sci"; + + k2g_clks: k2g_clks {Use "clocks" for node name instead.
Quoted 5 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt line 29
Quoted 1 line
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +1
-0+ compatible = "ti,k2g-sci-clk";I'm starting to think all these child nodes for SCI are pointless. Is there any reason why the parent node can't be the clock provider (along with all the other providers it acks as)?
I believe the only reason to keep them separate is to have kernel side of things modular. If we have separate nodes, the drivers can be probed separately.
If not, we need to build one huge blob with all the features in it, so the main driver can probe everything in one go, with annoying back-and-forth callbacks in place (assuming we still want to keep stuff somehow modular.)
-Tero
Quoted 9 lines
Quoted 9 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +8
-0+ #clock-cells = <2>; + }; +}; + +uart0: serial at 2530c00 { + compatible = "ns16550a"; + clocks = <&k2g_clks K2G_DEV_UART0 0>; +};
Nishanth Menon
maintainerPATCH 1/3 Documentation: dt: Add TI SCI clock driver
On 10/31/2016 07:50 AM, Tero Kristo wrote:
[...]
Quoted 13 lines
Quoted 7 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt line 28
Quoted 4 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +4
-0+pmmc: pmmc { + compatible = "ti,k2g-sci"; + + k2g_clks: k2g_clks {Use "clocks" for node name instead.
Quoted 5 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt line 29
Quoted 1 line
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +1
-0+ compatible = "ti,k2g-sci-clk";I'm starting to think all these child nodes for SCI are pointless. Is there any reason why the parent node can't be the clock provider (along with all the other providers it acks as)?
Quoted 8 lines
I believe the only reason to keep them separate is to have kernel side of things modular. If we have separate nodes, the drivers can be probed separately.
If not, we need to build one huge blob with all the features in it, so the main driver can probe everything in one go, with annoying back-and-forth callbacks in place (assuming we still want to keep stuff somehow modular.)
Documentation/devicetree/bindings/arm/arm,scpi.txt follows the same solution as well, right? There is indeed additional nodes coming in - such as reset, pd etc.. I cant see why it is different for sci clk.. not to mention the driver mess it results in.
Signature · 2 lines
Regards, Nishanth Menon
Rob Herring
maintainerPATCH 1/3 Documentation: dt: Add TI SCI clock driver
In reply to Tero KristoQuoted 65 lines · Tero Kristo
Quoted 56 lines · Rob Herring
Quoted 40 lines · Tero Kristo
Add a clock implementation, TI SCI clock, that will hook to the common clock framework, and allow each clock to be controlled via TI SCI protocol.
Signed-off-by Tero Kristo <[email protected]>--- .../devicetree/bindings/clock/ti,sci-clk.txt | 37 ++++++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 38 insertions(+) create mode 100644
Documentation/devicetree/bindings/clock/ti,sci-clk.txtDocumentation/devicetree/bindings/clock/ti,sci-clk.txt new file +5
-0@@ -0,0 +1,37 @@ +Texas Instruments TI-SCI Clocks +=============================== + +All clocks on Texas Instruments' SoCs that contain a System Controller, +are only controlled by this entity. Communication between a hostprocessor
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +13
-0+running an OS and the System Controller happens through a protocol known +as TI-SCI[1]. This clock implementation plugs into the common clock +framework and makes use of the TI-SCI protocol on clock API requests. + +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt + +Required properties: +------------------- +- compatible: Must be "ti,k2g-sci-clk" +- #clock-cells: Shall be 2. + In clock consumers, this cell represents the device ID and clock ID + exposed by the PM firmware. The assignments can be found in the header + files <dt-bindings/genpd/<soc>.h> (which covers the device IDs) andDocumentation/devicetree/bindings/clock/ti,sci-clk.txt line 28
Quoted 10 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +10
-0+ <dt-bindings/clock/<soc>.h> (which covers the clock IDs), where <soc> + is the SoC involved, for example 'k2g'. + +Examples: +-------- + +pmmc: pmmc { + compatible = "ti,k2g-sci"; + + k2g_clks: k2g_clks {Use "clocks" for node name instead.
Quoted 6 lines
Quoted 1 line
+ compatible = "ti,k2g-sci-clk";
I'm starting to think all these child nodes for SCI are pointless. Is there any reason why the parent node can't be the clock provider (along with all the other providers it acks as)?
Quoted 7 lines
I believe the only reason to keep them separate is to have kernel side of things modular. If we have separate nodes, the drivers can be probed separately.
If not, we need to build one huge blob with all the features in it, so the main driver can probe everything in one go, with annoying back-and-forth callbacks in place (assuming we still want to keep stuff somehow modular.)
Since when is DT the only way to create a device? The main driver can create devices for all the sub-functions like clocks. This is the same as MFDs which have been done both ways.
Rob
PATCH 1/3 Documentation: dt: Add TI SCI clock driver
Quoted 70 lines · Rob Herring
Quoted 69 lines · Tero Kristo
Quoted 56 lines · Rob Herring
Quoted 40 lines · Tero Kristo
Add a clock implementation, TI SCI clock, that will hook to the common clock framework, and allow each clock to be controlled via TI SCI protocol.
Signed-off-by Tero Kristo <[email protected]>--- .../devicetree/bindings/clock/ti,sci-clk.txt | 37 ++++++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 38 insertions(+) create mode 100644
Documentation/devicetree/bindings/clock/ti,sci-clk.txtDocumentation/devicetree/bindings/clock/ti,sci-clk.txt new file +5
-0@@ -0,0 +1,37 @@ +Texas Instruments TI-SCI Clocks +=============================== + +All clocks on Texas Instruments' SoCs that contain a System Controller, +are only controlled by this entity. Communication between a hostprocessor
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +13
-0+running an OS and the System Controller happens through a protocol known +as TI-SCI[1]. This clock implementation plugs into the common clock +framework and makes use of the TI-SCI protocol on clock API requests. + +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt + +Required properties: +------------------- +- compatible: Must be "ti,k2g-sci-clk" +- #clock-cells: Shall be 2. + In clock consumers, this cell represents the device ID and clock ID + exposed by the PM firmware. The assignments can be found in the header + files <dt-bindings/genpd/<soc>.h> (which covers the device IDs) andDocumentation/devicetree/bindings/clock/ti,sci-clk.txt line 28
Quoted 10 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +10
-0+ <dt-bindings/clock/<soc>.h> (which covers the clock IDs), where <soc> + is the SoC involved, for example 'k2g'. + +Examples: +-------- + +pmmc: pmmc { + compatible = "ti,k2g-sci"; + + k2g_clks: k2g_clks {Use "clocks" for node name instead.
Quoted 6 lines
Quoted 1 line
+ compatible = "ti,k2g-sci-clk";
I'm starting to think all these child nodes for SCI are pointless. Is there any reason why the parent node can't be the clock provider (along with all the other providers it acks as)?
I believe the only reason to keep them separate is to have kernel side of things modular. If we have separate nodes, the drivers can be probed separately.
Quoted 7 lines
Quoted 3 lines
If not, we need to build one huge blob with all the features in it, so the main driver can probe everything in one go, with annoying back-and-forth callbacks in place (assuming we still want to keep stuff somehow modular.)
Since when is DT the only way to create a device? The main driver can create devices for all the sub-functions like clocks. This is the same as MFDs which have been done both ways.
Yes obviously this can be done, my main point was that it will require building some sort of infra within the driver to handle this. With separate nodes, none of this is going to be needed. Also, we will lose any kind of configurability via DT if we don't have separate nodes; now we can select the available clocks / genpds via the compatible string of the clocks/genpd nodes themselves (this isn't clearly evident as of now as we only support a grand total of one device, which is k2g-evm.) Otherwise we need to probe against the main node and add a separate compatible string for every device, and carry this information to the sibling devices also somehow. It is just so much simpler if we can just keep separate nodes for them.
Also, plenty of things are doing this kind of stuff already in DT/kernel, having a parent node in place and sub-functions added separately for ease of use, with apparently no visible point for having the nodes within the DT.
-Tero
PATCH 1/3 Documentation: dt: Add TI SCI clock driver
Quoted 98 lines · Tero Kristo
Quoted 81 lines · Rob Herring
Quoted 74 lines · Tero Kristo
Quoted 60 lines · Rob Herring
Quoted 45 lines · Tero Kristo
Add a clock implementation, TI SCI clock, that will hook to the common clock framework, and allow each clock to be controlled via TI SCI protocol.
Signed-off-by Tero Kristo <[email protected]>--- .../devicetree/bindings/clock/ti,sci-clk.txt | 37 ++++++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 38 insertions(+) create mode 100644
Documentation/devicetree/bindings/clock/ti,sci-clk.txtDocumentation/devicetree/bindings/clock/ti,sci-clk.txt new file +4
-0@@ -0,0 +1,37 @@ +Texas Instruments TI-SCI Clocks +=============================== + +All clocks on Texas Instruments' SoCs that contain a SystemController,
+are only controlled by this entity. Communication between a host processor
+running an OS and the System Controller happens through a protocol knownDocumentation/devicetree/bindings/clock/ti,sci-clk.txt new file +11
-0+as TI-SCI[1]. This clock implementation plugs into the common clock +framework and makes use of the TI-SCI protocol on clock API requests. + +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt + +Required properties: +------------------- +- compatible: Must be "ti,k2g-sci-clk" +- #clock-cells: Shall be 2. + In clock consumers, this cell represents the device ID and clock ID + exposed by the PM firmware. The assignments can be found in theheader
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +2
-0+ files <dt-bindings/genpd/<soc>.h> (which covers the device IDs) and + <dt-bindings/clock/<soc>.h> (which covers the clock IDs), where<soc>
Documentation/devicetree/bindings/clock/ti,sci-clk.txt line 28
Quoted 9 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +9
-0+ is the SoC involved, for example 'k2g'. + +Examples: +-------- + +pmmc: pmmc { + compatible = "ti,k2g-sci"; + + k2g_clks: k2g_clks {Use "clocks" for node name instead.
Quoted 6 lines
Quoted 1 line
+ compatible = "ti,k2g-sci-clk";
I'm starting to think all these child nodes for SCI are pointless. Is there any reason why the parent node can't be the clock provider (along with all the other providers it acks as)?
I believe the only reason to keep them separate is to have kernel side of
things modular. If we have separate nodes, the drivers can be probed separately.Quoted 5 lines
If not, we need to build one huge blob with all the features in it, so the
main driver can probe everything in one go, with annoying back-and-forth callbacks in place (assuming we still want to keep stuff somehow modular.)Quoted 3 lines
Since when is DT the only way to create a device? The main driver can create devices for all the sub-functions like clocks. This is the same as MFDs which have been done both ways.
Yes obviously this can be done, my main point was that it will require building some sort of infra within the driver to handle this. With separate nodes, none of this is going to be needed. Also, we will lose any kind of configurability via DT if we don't have separate nodes; now we can select the available clocks / genpds via the compatible string of the clocks/genpd nodes themselves (this isn't clearly evident as of now as we only support a grand total of one device, which is k2g-evm.) Otherwise we need to probe against the main node and add a separate compatible string for every device, and carry this information to the sibling devices also somehow. It is just so much simpler if we can just keep separate nodes for them.
Quoted 4 lines
Also, plenty of things are doing this kind of stuff already in DT/kernel, having a parent node in place and sub-functions added separately for ease of use, with apparently no visible point for having the nodes within the DT.
Rob, any response on this one? I see you have acked the reset part of the bindings which is doing pretty much the same thing as the clock part is doing here, namely adding child node under the main SCI node. Is it okay to do this same for other parts of the TI SCI?
-Tero
Rob Herring
maintainerPATCH 1/3 Documentation: dt: Add TI SCI clock driver
Quoted 115 lines · Tero Kristo
Quoted 108 lines · Tero Kristo
Quoted 89 lines · Rob Herring
Quoted 80 lines · Tero Kristo
Quoted 63 lines · Rob Herring
Quoted 46 lines · Tero Kristo
Add a clock implementation, TI SCI clock, that will hook to the common clock framework, and allow each clock to be controlled via TI SCI protocol.
Signed-off-by Tero Kristo <[email protected]>--- .../devicetree/bindings/clock/ti,sci-clk.txt | 37 ++++++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 38 insertions(+) create mode 100644
Documentation/devicetree/bindings/clock/ti,sci-clk.txtDocumentation/devicetree/bindings/clock/ti,sci-clk.txt new file +4
-0@@ -0,0 +1,37 @@ +Texas Instruments TI-SCI Clocks +=============================== + +All clocks on Texas Instruments' SoCs that contain a SystemController,
+are only controlled by this entity. Communication between a host processor
+running an OS and the System Controller happens through a protocol knownDocumentation/devicetree/bindings/clock/ti,sci-clk.txt new file +11
-0+as TI-SCI[1]. This clock implementation plugs into the common clock +framework and makes use of the TI-SCI protocol on clock API requests. + +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt + +Required properties: +------------------- +- compatible: Must be "ti,k2g-sci-clk" +- #clock-cells: Shall be 2. + In clock consumers, this cell represents the device ID and clock ID + exposed by the PM firmware. The assignments can be found in theheader
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +2
-0+ files <dt-bindings/genpd/<soc>.h> (which covers the device IDs) and + <dt-bindings/clock/<soc>.h> (which covers the clock IDs), where<soc>
Documentation/devicetree/bindings/clock/ti,sci-clk.txt line 28
Quoted 9 lines
Documentation/devicetree/bindings/clock/ti,sci-clk.txt new file +9
-0+ is the SoC involved, for example 'k2g'. + +Examples: +-------- + +pmmc: pmmc { + compatible = "ti,k2g-sci"; + + k2g_clks: k2g_clks {Use "clocks" for node name instead.
Quoted 7 lines
Quoted 1 line
+ compatible = "ti,k2g-sci-clk";
I'm starting to think all these child nodes for SCI are pointless. Is there any reason why the parent node can't be the clock provider (along with all the other providers it acks as)?
I believe the only reason to keep them separate is to have kernel side of
things modular. If we have separate nodes, the drivers can be probed separately.Quoted 5 lines
If not, we need to build one huge blob with all the features in it, so the
main driver can probe everything in one go, with annoying back-and-forth callbacks in place (assuming we still want to keep stuff somehow modular.)Quoted 3 lines
Since when is DT the only way to create a device? The main driver can create devices for all the sub-functions like clocks. This is the same as MFDs which have been done both ways.
Yes obviously this can be done, my main point was that it will require building some sort of infra within the driver to handle this. With separate nodes, none of this is going to be needed. Also, we will lose any kind of configurability via DT if we don't have separate nodes; now we can select the available clocks / genpds via the compatible string of the clocks/genpd nodes themselves (this isn't clearly evident as of now as we only support a grand total of one device, which is k2g-evm.) Otherwise we need to probe against the main node and add a separate compatible string for every device, and carry this information to the sibling devices also somehow. It is just so much simpler if we can just keep separate nodes for them.
Quoted 4 lines
Also, plenty of things are doing this kind of stuff already in DT/kernel, having a parent node in place and sub-functions added separately for ease of use, with apparently no visible point for having the nodes within the DT.
Quoted 4 lines
Rob, any response on this one? I see you have acked the reset part of the bindings which is doing pretty much the same thing as the clock part is doing here, namely adding child node under the main SCI node. Is it okay to do this same for other parts of the TI SCI?
Yes. It would be silly to allow for one and not others...
Rob
PATCH 1/3 Documentation: dt: Add TI SCI clock driver
Quoted 8 lines · Rob Herring
Quoted 5 lines · Tero Kristo
Rob, any response on this one? I see you have acked the reset part of the bindings which is doing pretty much the same thing as the clock part is doing here, namely adding child node under the main SCI node. Is it okay to do this same for other parts of the TI SCI?
Yes. It would be silly to allow for one and not others...
I'm expecting a respin for the node name (clocks or clock-controller). I'll also make a review pass on patch 3 today so please don't respin until after that.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH 1/3 Documentation: dt: Add TI SCI clock driver
Quoted 7 lines · Stephen Boyd
Quoted 6 lines · Rob Herring
Quoted 5 lines · Tero Kristo
Rob, any response on this one? I see you have acked the reset part of the bindings which is doing pretty much the same thing as the clock part is doing here, namely adding child node under the main SCI node. Is it okay to do this same for other parts of the TI SCI?
Quoted 6 lines
Quoted 2 lines
Yes. It would be silly to allow for one and not others...
I'm expecting a respin for the node name (clocks or clock-controller). I'll also make a review pass on patch 3 today so please don't respin until after that.
Yeah, I need to fix that and re-send. This series will be most likely delayed until 4.11 though seeing its very late in 4.9-rc already (and the genpd part dependency is still not ready either) so we are not in a rush right now.
-Tero
PATCH 2/3 dt-binding: clock: Add k2g clock definitions
In reply to Tero KristoAdd identifiers for the K2G clocks managed by the PMMC.
--- MAINTAINERS | 1 + include/dt-bindings/clock/k2g.h | 234 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 235 insertions(+) create mode 100644 include/dt-bindings/clock/k2g.h
MAINTAINERS +1 -0
@@ -11896,6 +11896,7 @@ F: Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt F: include/dt-bindings/genpd/k2g.h F: drivers/soc/ti/ti_sci_pm_domains.c F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt +F: include/dt-bindings/clock/k2g.h THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER M: Hans Verkuil <[email protected]>
include/dt-bindings/clock/k2g.h new file +234 -0
234 lines not on this page · raw message
Signature · 1 line
1.9.1
PATCH 2/3 dt-binding: clock: Add k2g clock definitions
Quoted 5 lines · Tero Kristo
Add identifiers for the K2G clocks managed by the PMMC.
Signed-off-by Tero Kristo <[email protected]>Tested-by Dave Gerlach <[email protected]>Signed-off-by Nishanth Menon <[email protected]>
Digged this out of archives, hopefully the reply has the correct message id...
Anyway, now that TI SCI genpd is apparently getting its include/dt-bindings/genpd/k2g.h file being dropped, should the clock driver also drop this file included in this patch?
This will change all the clock handles for k2g clocks under DT to be of cryptic format like:
clocks = <&k2g_clks 0x18 10>
... as an example for NSS RGMII 5 MHz clock.
Arnd, any comments seeing the original request to drop the genpd device ids from the include came from you?
-Tero
Quoted 259 lines
--- MAINTAINERS | 1 + include/dt-bindings/clock/k2g.h | 234 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 235 insertions(+) create mode 100644 include/dt-bindings/clock/k2g.hMAINTAINERS +1
-0@@ -11896,6 +11896,7 @@ F: Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt F: include/dt-bindings/genpd/k2g.h F: drivers/soc/ti/ti_sci_pm_domains.c F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt +F: include/dt-bindings/clock/k2g.h THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER M: Hans Verkuil <[email protected]>include/dt-bindings/clock/k2g.h new file +234
-0@@ -0,0 +1,234 @@ +/* +* TI K2G SoC clock definitions +* +* Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/ +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ +#ifndef __DT_BINDINGS_CLOCK_K2G_H__ +#define __DT_BINDINGS_CLOCK_K2G_H__ + +/* +* The clock IDs listed in this file are describing the clocks at IP's +* boundaries. The firmware is not exposing any of the generic clocks +* from the system, those are handled internally by the firmware. +*/ +#define K2G_DEV_PMMC_MPM_VBUS_CLK 0 +#define K2G_DEV_PMMC_MPM_FUNC_32K_CLK 1 +#define K2G_DEV_PMMC_MPM_FUNC_OSC_CLK 2 +#define K2G_DEV_PMMC_MPM_DAP_CLK 3 + +#define K2G_DEV_MLB_MLB_SYS_CLK 0 +#define K2G_DEV_MLB_MLB_SHB_OCP_CLK 1 +#define K2G_DEV_MLB_MLB_SPB_OCP_CLK 2 +#define K2G_DEV_MLB_MLB_IO_CLK 3 +#define K2G_DEV_MLB_MLBP_IO_CLK 4 + +#define K2G_DEV_DSS_PI_DSS_OCP_CLK 0 +#define K2G_DEV_DSS_PI_DSS_VP_CLK 1 + +#define K2G_DEV_MCBSP_VBUS_CLK 0 +#define K2G_DEV_MCBSP_CLKS 1 +#define K2G_DEV_MCBSP_CLKS_PARENT_AUDIO_OSC 2 +#define K2G_DEV_MCBSP_CLKS_PARENT_MLB_IO_CLK 3 +#define K2G_DEV_MCBSP_CLKS_PARENT_MLBP_IO_CLK 4 +#define K2G_DEV_MCBSP_CLKS_PARENT_SYS_OSCCLK 5 +#define K2G_DEV_MCBSP_CLKS_PARENT_XREFCLK 6 +#define K2G_DEV_MCBSP_CLKS_PARENT_UART_PLL 7 + +#define K2G_DEV_MCASP_VBUS_CLK 0 +#define K2G_DEV_MCASP_AUX_CLK 1 +#define K2G_DEV_MCASP_AUX_CLK_PARENT_AUDIO_OSC 2 +#define K2G_DEV_MCASP_AUX_CLK_PARENT_MLB_IO_CLK 3 +#define K2G_DEV_MCASP_AUX_CLK_PARENT_MLBP_IO_CLK 4 +#define K2G_DEV_MCASP_AUX_CLK_PARENT_SYS_OSCCLK 5 +#define K2G_DEV_MCASP_AUX_CLK_PARENT_XREFCLK 6 +#define K2G_DEV_MCASP_AUX_CLK_PARENT_UART_PLL 7 + +#define K2G_DEV_DCAN_VBUS_CLK 0 +#define K2G_DEV_DCAN_CAN_CLK 1 + +#define K2G_DEV_EMIF_V_CLK 0 +#define K2G_DEV_EMIF_M_CLK 1 +#define K2G_DEV_EMIF_DFT_LOCAL_CLK 2 +#define K2G_DEV_EMIF_PUB_CTL_CLK 3 +#define K2G_DEV_EMIF_PHY_CTL_CLK 4 +#define K2G_DEV_EMIF_VBUSP_CLK 5 + +#define K2G_DEV_MMCHS_VBUS_CLK 0 +#define K2G_DEV_MMCHS_CLK_ADPI 1 +#define K2G_DEV_MMCHS_CLK32K 2 + +#define K2G_DEV_GPMC_GPMC_FCLK 0 + +#define K2G_DEV_ELM_CLK 0 + +#define K2G_DEV_SPI_VBUSP_CLK 0 + +#define K2G_DEV_ICSS_VCLK_CLK 0 +#define K2G_DEV_ICSS_CORE_CLK 1 +#define K2G_DEV_ICSS_CORE_CLK_PARENT_ICSS_PLL 2 +#define K2G_DEV_ICSS_CORE_CLK_PARENT_NSS_PLL 3 +#define K2G_DEV_ICSS_UCLK_CLK 4 +#define K2G_DEV_ICSS_IEPCLK_CLK 5 + +#define K2G_DEV_USB_BUS_CLK 0 +#define K2G_DEV_USB_PHYMMR_CLK 1 +#define K2G_DEV_USB_SUSP_CLK 2 +#define K2G_DEV_USB_REF_CLK 3 +#define K2G_DEV_USB_DFT_ULPI_CLK 4 +#define K2G_DEV_USB_DFT_UTMI_CLK 5 +#define K2G_DEV_USB_CLKCORE 6 + +#define K2G_DEV_NSS_VCLK 0 +#define K2G_DEV_NSS_SA_UL_CLK 1 +#define K2G_DEV_NSS_SA_UL_X1_CLK 2 +#define K2G_DEV_NSS_ESW_CLK 3 +/* +* Mux register is internal to the CPTS, so we must allow NSS to control it. +* CPTS_REFCLK_P/N is a direct input to this mux, so ignore it is ignored +* in the SoC clock tree. +*/ +#define K2G_DEV_NSS_CPTS_CHIP_CLK1_2 4 +#define K2G_DEV_NSS_CPTS_CHIP_CLK1_3 5 +#define K2G_DEV_NSS_CPTS_TIMI0 6 +#define K2G_DEV_NSS_CPTS_TIMI1 7 +#define K2G_DEV_NSS_CPTS_NSS_PLL 8 +#define K2G_DEV_NSS_GMII_RFTCLK 9 +#define K2G_DEV_NSS_RGMII_MHZ_5_CLK 10 +#define K2G_DEV_NSS_RGMII_MHZ_50_CLK 11 +#define K2G_DEV_NSS_RGMII_MHZ_250_CLK 12 +#define K2G_DEV_NSS_RMII_MHZ_50_CLK 13 + +#define K2G_DEV_PCIE_VBUS_CLK 0 + +#define K2G_DEV_OTP_VBUS_CLK 0 + +#define K2G_DEV_GPIO_VBUS_CLK 0 + +#define K2G_DEV_TIMER64_VBUS_CLK 0 +#define K2G_DEV_TIMER64_TINL 1 +#define K2G_DEV_TIMER64_TINL_PARENT_TIMI0 2 +#define K2G_DEV_TIMER64_TINL_PARENT_TIMI1 3 +#define K2G_DEV_TIMER64_TINH 4 +#define K2G_DEV_TIMER64_TINH_PARENT_TIMI0 5 +#define K2G_DEV_TIMER64_TINH_PARENT_TIMI1 6 +#define K2G_DEV_TIMER64_TOUTL 7 +#define K2G_DEV_TIMER64_TOUTH 8 + +#define K2G_DEV_SEC_MGR_SEC_CLK_PI 0 + +#define K2G_DEV_MSGMGR_VBUS_CLK 0 + +#define K2G_DEV_BOOTCFG_VBUS_CLK 0 + +#define K2G_DEV_ARM_BOOTROM_VBUS_CLK 0 + +#define K2G_DEV_DSP_BOOTROM_VBUS_CLK 0 + +#define K2G_DEV_DEBUGSS_VBUSP_CTTBRCLK_CLK 0 +#define K2G_DEV_DEBUGSS_VBUSP_STMD0_CLK 1 +#define K2G_DEV_DEBUGSS_VBUSP_SLAVE_CLK 2 +#define K2G_DEV_DEBUGSS_VBUSP_MASTER_CLK 3 +#define K2G_DEV_DEBUGSS_TCK 4 +#define K2G_DEV_DEBUGSS_CS_TRCEXPT_CLK 5 +#define K2G_DEV_DEBUGSS_DSP_TRACECLK 6 +#define K2G_DEV_DEBUGSS_STMXPT_CLK 7 + +#define K2G_DEV_UART_CBA_CLK_PI 0 + +#define K2G_DEV_EHRPWM_VBUS_CLK 0 + +#define K2G_DEV_EQEP_VBUS_CLK 0 + +#define K2G_DEV_ECAP_VBUS_CLK 0 + +#define K2G_DEV_I2C_VBUS_CLK 0 + +#define K2G_DEV_CP_TRACER_CP_TRACER_CLK 0 + +#define K2G_DEV_EDMA_TPTC_CLK 0 +#define K2G_DEV_EDMA_TPCC_CLK 1 + +#define K2G_DEV_SEMAPHORE_VBUS_CLK 0 + +#define K2G_DEV_INTC_VBUS_CLK 0 + +#define K2G_DEV_GIC_VBUS_CLK 0 + +#define K2G_DEV_QSPI_QSPI_CLK 0 +#define K2G_DEV_QSPI_DATA_BUS_CLK 1 +#define K2G_DEV_QSPI_CFG_BUS_CLK 2 +#define K2G_DEV_QSPI_QSPI_CLK_O 3 +#define K2G_DEV_QSPI_QSPI_CLK_I 4 + +#define K2G_DEV_ARM_64B_COUNTER_CLK_INPUT 0 +#define K2G_DEV_ARM_64B_COUNTER_VBUSP_CLK 1 + +#define K2G_DEV_TETRIS_CORE_CLK 0 +#define K2G_DEV_TETRIS_SUBSYS_CLK 1 + +#define K2G_DEV_CGEM_CORE_CLK 0 +#define K2G_DEV_CGEM_TRACE_CLK 1 + +#define K2G_DEV_MSMC_VBUS_CLK 0 + +#define K2G_DEV_DFT_SS_VBUS_CLK 0 +#define K2G_DEV_DFT_SS_TCK 1 + +#define K2G_DEV_CBASS_VBUS_CLK 0 + +#define K2G_DEV_SMARTREFLEX_SCLK_CLK 0 +#define K2G_DEV_SMARTREFLEX_REFCLK1_CLK 1 +#define K2G_DEV_SMARTREFLEX_TEMPMCLK_CLK 2 + +#define K2G_DEV_EFUSE_VBUS_CLK 0 + +/* Outputs from board (inputs to SoC) */ +#define K2G_DEV_BOARD_SYS_OSCIN 0 +#define K2G_DEV_BOARD_SYS_CLK 1 +#define K2G_DEV_BOARD_AUDIO_OSCIN 2 +#define K2G_DEV_BOARD_DDR 3 +#define K2G_DEV_BOARD_MLBCLK 4 +#define K2G_DEV_BOARD_MLBPCLK 5 +#define K2G_DEV_BOARD_XREFCLK 6 +#define K2G_DEV_BOARD_TIMI0 7 +#define K2G_DEV_BOARD_TIMI1 8 + +/* Inputs to board (outputs from SoC) */ +#define K2G_DEV_BOARD_SYSCLKOUT 10 +#define K2G_DEV_BOARD_OBSCLK 11 +#define K2G_DEV_BOARD_OBSCLK_PARENT_MAIN_PLL 12 +#define K2G_DEV_BOARD_OBSCLK_PARENT_DSS_PLL 13 +#define K2G_DEV_BOARD_OBSCLK_PARENT_ARM_PLL 14 +#define K2G_DEV_BOARD_OBSCLK_PARENT_UART_PLL 15 +#define K2G_DEV_BOARD_OBSCLK_PARENT_ICSS_PLL 16 +#define K2G_DEV_BOARD_OBSCLK_PARENT_DDR_PLL 17 +#define K2G_DEV_BOARD_OBSCLK_PARENT_PLL_CTRL 18 +#define K2G_DEV_BOARD_OBSCLK_PARENT_NSS_PLL 19 +#define K2G_DEV_BOARD_OBSCLK_PARENT_SYSOSC 20 +#define K2G_DEV_BOARD_MII_CLKOUT 21 +#define K2G_DEV_BOARD_TIMO0 22 +#define K2G_DEV_BOARD_TIMO1 23 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_0L 24 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_0H 25 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_1L 26 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_1H 27 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_2L 28 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_2H 29 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_3L 30 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_3H 31 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_4L 32 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_4H 33 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_5L 34 +#define K2G_DEV_BOARD_TIMO_PARENT_TIMER64_5H 35 + +#endif
PATCH 3/3 clk: keystone: Add sci-clk driver support
In reply to Tero KristoIn K2G, the clock handling is done through firmware executing on a separate core. Linux kernel needs to communicate to the firmware through TI system control interface to access any power management related resources, including clocks.
The keystone sci-clk driver does this, by communicating to the firmware through the TI SCI driver. The driver adds support for registering clocks through DT, and basic required clock operations like prepare/get_rate, etc.
--- MAINTAINERS | 1 + drivers/clk/Kconfig | 9 + drivers/clk/keystone/Makefile | 1 + drivers/clk/keystone/sci-clk.c | 589 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 600 insertions(+) create mode 100644 drivers/clk/keystone/sci-clk.c
MAINTAINERS +1 -0
@@ -11897,6 +11897,7 @@ F: include/dt-bindings/genpd/k2g.h F: drivers/soc/ti/ti_sci_pm_domains.c F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt F: include/dt-bindings/clock/k2g.h +F: drivers/clk/keystone/sci-clk.c THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER M: Hans Verkuil <[email protected]>
drivers/clk/Kconfig +9 -0
@@ -169,6 +169,15 @@ config COMMON_CLK_NXP ---help--- Support for clock providers on NXP platforms. +config TI_SCI_CLK + tristate "TI System Control Interface clock drivers" + depends on (TI_SCI_PROTOCOL && COMMON_CLK_KEYSTONE) || COMPILE_TEST + default TI_SCI_PROTOCOL + ---help--- + This adds the clock driver support over TI System Control Interface. + If you wish to use clock resources from the PMMC firmware, say Y. + Otherwise, say N. + config COMMON_CLK_PALMAS tristate "Clock driver for TI Palmas devices" depends on MFD_PALMAS
drivers/clk/keystone/Makefile +1 -0
@@ -1 +1,2 @@ obj-y += pll.o gate.o +obj-$(CONFIG_TI_SCI_CLK) += sci-clk.o
drivers/clk/keystone/sci-clk.c new file +589 -0
589 lines not on this page · raw message
Signature · 1 line
1.9.1
PATCH 3/3 clk: keystone: Add sci-clk driver support
drivers/clk/Kconfig config COMMON_CLK_NXP · line 174
Quoted 11 lines · Tero Kristo
drivers/clk/Kconfig +3
-0@@ -169,6 +169,15 @@ config COMMON_CLK_NXP ---help--- Support for clock providers on NXP platforms. +config TI_SCI_CLK + tristate "TI System Control Interface clock drivers" + depends on (TI_SCI_PROTOCOL && COMMON_CLK_KEYSTONE) || COMPILE_TEST
Given that we depend on COMMON_CLK_KEYSTONE (just for the Makefile dependency?) this should be moved to right below the COMMON_CLK_KEYSTONE config. And we should consider making a Kconfig file in drivers/clk/keystone/ to hold both those configs instead of having them at the toplevel.
Quoted 22 lines
drivers/clk/Kconfig +6
-0+ default TI_SCI_PROTOCOL + ---help--- + This adds the clock driver support over TI System Control Interface. + If you wish to use clock resources from the PMMC firmware, say Y. + Otherwise, say N. + config COMMON_CLK_PALMAS tristate "Clock driver for TI Palmas devices" depends on MFD_PALMASdrivers/clk/keystone/Makefile +1
-0@@ -1 +1,2 @@ obj-y += pll.o gate.o +obj-$(CONFIG_TI_SCI_CLK) += sci-clk.odrivers/clk/keystone/sci-clk.c new file +0
-0@@ -0,0 +1,589 @@
[...]
Quoted 16 lines
drivers/clk/keystone/sci-clk.c new file +16
-0+ +/** +* sci_clk_recalc_rate - Get clock rate for a TI SCI clock +* @hw: clock to get rate for +* @parent_rate: parent rate provided by common clock framework, not used +* +* Gets the current clock rate of a TI SCI clock. Returns the current +* clock rate, or zero in failure. +*/ +static unsigned long sci_clk_recalc_rate (struct clk_hw *hw, +unsigned long parent_rate) +{ +struct sci_clk *clk =to_sci_clk (hw); +u64 freq; +int ret; +
drivers/clk/keystone/sci-clk.c line 166
Quoted 10 lines
drivers/clk/keystone/sci-clk.c new file +10
-0+ ret = clk->provider->ops->get_freq (clk->provider->sci, clk->dev_id, + clk->clk_id, &freq); +if (ret) { +dev_err (clk->provider->dev, +"recalc-rate failed for dev=%d, clk=%d, ret=%d\n" , + clk->dev_id, clk->clk_id, ret); +return 0 ; + } + +return (u32 )freq;
Do we need the cast? sizeof(u32) doesn't always equal sizeof(unsigned long).
Quoted 34 lines
drivers/clk/keystone/sci-clk.c new file +34
-0+ +/** +* _sci_clk_get - Gets a handle for an SCI clock +* @provider: Handle to SCI clock provider +* @dev_id: device ID for the clock to register +* @clk_id: clock ID for the clock to register +* +* Gets a handle to an existing TI SCI hw clock, or builds a new clock +* entry and registers it with the common clock framework. Called from +* the common clock framework, when a corresponding of_clk_get call is +* executed, or recursively from itself when parsing parent clocks. +* Returns a pointer to the hw clock struct, or ERR_PTR value in failure. +*/ +static struct clk_hw *_sci_clk_build (struct sci_clk_provider *provider, +u16 dev_id,u8 clk_id) +{ +struct clk_init_data init = {NULL }; +struct sci_clk *sci_clk =NULL ; +char *name =NULL ; +char **parent_names =NULL ; +int i; +int ret; + + sci_clk =devm_kzalloc (provider->dev,sizeof (*sci_clk),GFP_KERNEL ); +if (!sci_clk) +return ERR_PTR (-ENOMEM ); + + sci_clk->dev_id = dev_id; + sci_clk->clk_id = clk_id; + sci_clk->provider = provider; + + ret = provider->ops->get_num_parents (provider->sci, dev_id, + clk_id, + &init.num_parents);
drivers/clk/keystone/sci-clk.c line 316
Quoted 10 lines
drivers/clk/keystone/sci-clk.c new file +10
-0+if (ret) + goto err; + + name =kasprintf (GFP_KERNEL ,"%s:%d:%d" ,dev_name (provider->dev), + sci_clk->dev_id, sci_clk->clk_id); + + init.name = name; + +if (init.num_parents <2 ) + init.num_parents =0 ;
This deserves a comment. Why is num_parents == 1 the same as num_parents == 0?
Quoted 14 lines
drivers/clk/keystone/sci-clk.c new file +14
-0+ +if (init.num_parents) { + parent_names =devm_kcalloc (provider->dev, init.num_parents, +sizeof (char *),GFP_KERNEL ); + +if (!parent_names) { + ret = -ENOMEM ; + goto err; + } + +for (i =0 ; i < init.num_parents; i++) { +char *parent_name; + + parent_name = kasprintf(GFP_KERNEL, "%s:%d:%d",
drivers/clk/keystone/sci-clk.c line 340
Quoted 10 lines
drivers/clk/keystone/sci-clk.c new file +10
-0+dev_name (provider->dev), + sci_clk->dev_id, + sci_clk->clk_id + 1 + i); +if (!parent_name) { + ret = -ENOMEM ; + goto err; + } + parent_names[i] = parent_name; + } + init.parent_names = (const char *const *)parent_names;
Does that really need a cast?
Quoted 10 lines
drivers/clk/keystone/sci-clk.c new file +10
-0+ } + + init.ops = &sci_clk_ops; + sci_clk->hw.init = &init; + + ret =devm_clk_hw_register (provider->dev, &sci_clk->hw); +if (ret) { +dev_err (provider->dev,"failed clk register with %d\n" , ret); + goto err; + }
drivers/clk/keystone/sci-clk.c line 360
Quoted 10 lines
drivers/clk/keystone/sci-clk.c new file +10
-0+kfree (name); + +return &sci_clk->hw; + +err: +if (parent_names) { +for (i =0 ; i < init.num_parents; i++) +devm_kfree (provider->dev, parent_names[i]); + +devm_kfree (provider->dev, parent_names);
Shouldn't we be freeing the parent names all the time? It should be deep copied in the framework.
Quoted 8 lines
drivers/clk/keystone/sci-clk.c new file +8
-0+ } + +devm_kfree (provider->dev, sci_clk); + +kfree (name); + +return ERR_PTR (ret); +}
[..]
Quoted 13 lines
drivers/clk/keystone/sci-clk.c new file +13
-0+ +static int ti_sci_init_clocks (struct sci_clk_provider *p) +{ +struct sci_clk_data *data = p->clocks; +struct clk_hw *hw; +int i; + +while (data->num_clks) { + data->clocks =devm_kcalloc (p->dev, data->num_clks, +sizeof (struct sci_clk ), +GFP_KERNEL ); +if (!data->clocks) +return -ENOMEM ;
drivers/clk/keystone/sci-clk.c line 428
Quoted 10 lines
drivers/clk/keystone/sci-clk.c new file +10
-0+ +for (i =0 ; i < data->num_clks; i++) { + hw =_sci_clk_build (p, data->dev, i); +if (!IS_ERR (hw)) { + data->clocks[i] = hw; +continue ; + } + +/* Skip any holes in the clock lists */ +if (PTR_ERR (hw) == -ENODEV )
Does this happen? I don't see where _sci_clk_build() returns -ENODEV.
Quoted 23 lines
drivers/clk/keystone/sci-clk.c new file +22
-0+continue ; + +return PTR_ERR (hw); + } + data++; + } + +return 0 ; +} + + +/** +* ti_sci_clk_probe - Probe function for the TI SCI clock driver +* @pdev: platform device pointer to be probed +* +* Probes the TI SCI clock device. Allocates a new clock provider +* and registers this to the common clock framework. Also applies +* any required flags to the identified clocks via clock lists +* supplied from DT. Returns 0 for success, negative error value +* for failure. +*/ +static int ti_sci_clk_probe (struct platform_device *pdev)
drivers/clk/keystone/sci-clk.c line 536
Quoted 10 lines
drivers/clk/keystone/sci-clk.c new file +10
-0+{ +struct device *dev = &pdev->dev; +struct device_node *np = dev->of_node; +struct sci_clk_provider *provider; +const struct ti_sci_handle *handle; +struct sci_clk_data *data; +int ret; + + data = (struct sci_clk_data *) +of_match_node (ti_sci_clk_of_match, np)->data;
Just use of_device_get_match_data() instead.
Quoted 6 lines
drivers/clk/keystone/sci-clk.c new file +6
-0+ + handle =devm_ti_sci_get_handle (dev); +if (IS_ERR (handle)) +return PTR_ERR (handle); + + provider =devm_kzalloc (dev,sizeof (*provider),GFP_KERNEL );
drivers/clk/keystone/sci-clk.c line 552
Quoted 10 lines
drivers/clk/keystone/sci-clk.c new file +10
-0+if (!provider) +return -ENOMEM ; + + provider->clocks = data; + + provider->sci = handle; + provider->ops = &handle->ops.clk_ops; + provider->dev = dev; + +ti_sci_init_clocks (provider);
And if this fails?
drivers/clk/keystone/sci-clk.c line 558
Quoted 6 lines
drivers/clk/keystone/sci-clk.c new file +6
-0+ + ret =of_clk_add_hw_provider (np, sci_clk_get, provider); +if (ret) +return ret; + +return 0 ;
Just "return of_clk_add_hw_provider()" please.
Quoted 1 line
+}
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH 3/3 clk: keystone: Add sci-clk driver support
Quoted 9 lines · Stephen Boyd
Quoted 8 lines · Tero Kristo
drivers/clk/Kconfig +0
-0@@ -169,6 +169,15 @@ config COMMON_CLK_NXP ---help--- Support for clock providers on NXP platforms.
Quoted 9 lines
drivers/clk/Kconfig config COMMON_CLK_NXP · line 174
Quoted 3 lines
drivers/clk/Kconfig +3
-0+config TI_SCI_CLK + tristate "TI System Control Interface clock drivers" + depends on (TI_SCI_PROTOCOL && COMMON_CLK_KEYSTONE) || COMPILE_TESTGiven that we depend on
COMMON_CLK_KEYSTONE(just for theMakefiledependency?) this should be moved to right below theCOMMON_CLK_KEYSTONEconfig. And we should consider making aKconfigfile indrivers/clk/keystone/to hold both those configs instead of having them at the toplevel.
Its a makefile dependency only right now. I'll have a look at how to handle this properly.
Quoted 45 lines
Quoted 23 lines
drivers/clk/Kconfig +6
-0+ default TI_SCI_PROTOCOL + ---help--- + This adds the clock driver support over TI System Control Interface. + If you wish to use clock resources from the PMMC firmware, say Y. + Otherwise, say N. + config COMMON_CLK_PALMAS tristate "Clock driver for TI Palmas devices" depends on MFD_PALMASdrivers/clk/keystone/Makefile +1
-0@@ -1 +1,2 @@ obj-y += pll.o gate.o +obj-$(CONFIG_TI_SCI_CLK) += sci-clk.odrivers/clk/keystone/sci-clk.c new file +0
-0@@ -0,0 +1,589 @@[...]
Quoted 21 lines
drivers/clk/keystone/sci-clk.c new file +21
-0+ +/** +* sci_clk_recalc_rate - Get clock rate for a TI SCI clock +* @hw: clock to get rate for +* @parent_rate: parent rate provided by common clock framework, not used +* +* Gets the current clock rate of a TI SCI clock. Returns the current +* clock rate, or zero in failure. +*/ +static unsigned long sci_clk_recalc_rate (struct clk_hw *hw, +unsigned long parent_rate) +{ +struct sci_clk *clk =to_sci_clk (hw); +u64 freq; +int ret; + + ret = clk->provider->ops->get_freq (clk->provider->sci, clk->dev_id, + clk->clk_id, &freq); +if (ret) { + dev_err(clk->provider->dev, + "recalc-rate failed for dev=%d, clk=%d, ret=%d\n",
Quoted 8 lines
drivers/clk/keystone/sci-clk.c line 166
Quoted 5 lines
drivers/clk/keystone/sci-clk.c new file +5
-0+ clk->dev_id, clk->clk_id, ret); +return 0 ; + } + +return (u32 )freq;Do we need the cast? sizeof(u32) doesn't always equal sizeof(unsigned long).
Hmm, not sure where that came from. But yea, can be dropped.
Quoted 40 lines
Quoted 40 lines
drivers/clk/keystone/sci-clk.c new file +39
-0+ +/** +* _sci_clk_get - Gets a handle for an SCI clock +* @provider: Handle to SCI clock provider +* @dev_id: device ID for the clock to register +* @clk_id: clock ID for the clock to register +* +* Gets a handle to an existing TI SCI hw clock, or builds a new clock +* entry and registers it with the common clock framework. Called from +* the common clock framework, when a corresponding of_clk_get call is +* executed, or recursively from itself when parsing parent clocks. +* Returns a pointer to the hw clock struct, or ERR_PTR value in failure. +*/ +static struct clk_hw *_sci_clk_build (struct sci_clk_provider *provider, +u16 dev_id,u8 clk_id) +{ +struct clk_init_data init = {NULL }; +struct sci_clk *sci_clk =NULL ; +char *name =NULL ; +char **parent_names =NULL ; +int i; +int ret; + + sci_clk =devm_kzalloc (provider->dev,sizeof (*sci_clk),GFP_KERNEL ); +if (!sci_clk) +return ERR_PTR (-ENOMEM ); + + sci_clk->dev_id = dev_id; + sci_clk->clk_id = clk_id; + sci_clk->provider = provider; + + ret = provider->ops->get_num_parents (provider->sci, dev_id, + clk_id, + &init.num_parents); +if (ret) + goto err; + + name =kasprintf (GFP_KERNEL ,"%s:%d:%d" ,dev_name (provider->dev), + sci_clk->dev_id, sci_clk->clk_id);
Quoted 8 lines
drivers/clk/keystone/sci-clk.c line 316
Quoted 5 lines
drivers/clk/keystone/sci-clk.c new file +5
-0+ + init.name = name; + +if (init.num_parents <2 ) + init.num_parents =0 ;This deserves a comment. Why is
num_parents== 1 the same asnum_parents== 0?
I'll add a comment on this. Basically some clocks can be root clocks which don't have parents, and we only want to have parent control for clocks that can switch their parent. This is kind of a quirk of the firmware.
Quoted 19 lines
Quoted 19 lines
drivers/clk/keystone/sci-clk.c new file +18
-0+ +if (init.num_parents) { + parent_names =devm_kcalloc (provider->dev, init.num_parents, +sizeof (char *),GFP_KERNEL ); + +if (!parent_names) { + ret = -ENOMEM ; + goto err; + } + +for (i =0 ; i < init.num_parents; i++) { +char *parent_name; + + parent_name =kasprintf (GFP_KERNEL ,"%s:%d:%d" , +dev_name (provider->dev), + sci_clk->dev_id, + sci_clk->clk_id +1 + i); +if (!parent_name) {
Quoted 8 lines
drivers/clk/keystone/sci-clk.c line 340
Quoted 6 lines
drivers/clk/keystone/sci-clk.c new file +6
-0+ ret = -ENOMEM ; + goto err; + } + parent_names[i] = parent_name; + } + init.parent_names = (const char *const *)parent_names;Does that really need a cast?
Doesn't seem like so... I think without this it was generating some checkpatch issue sometime back, but doesn't seem to be the case anymore.
Quoted 16 lines
Quoted 16 lines
drivers/clk/keystone/sci-clk.c new file +15
-0+ } + + init.ops = &sci_clk_ops; + sci_clk->hw.init = &init; + + ret =devm_clk_hw_register (provider->dev, &sci_clk->hw); +if (ret) { +dev_err (provider->dev,"failed clk register with %d\n" , ret); + goto err; + } +kfree (name); + +return &sci_clk->hw; + +err:
Quoted 8 lines
drivers/clk/keystone/sci-clk.c line 360
Quoted 5 lines
drivers/clk/keystone/sci-clk.c new file +5
-0+if (parent_names) { +for (i =0 ; i < init.num_parents; i++) +devm_kfree (provider->dev, parent_names[i]); + +devm_kfree (provider->dev, parent_names);Shouldn't we be freeing the parent names all the time? It should be deep copied in the framework.
I'll check this.
Quoted 28 lines
Quoted 9 lines
drivers/clk/keystone/sci-clk.c new file +8
-0+ } + +devm_kfree (provider->dev, sci_clk); + +kfree (name); + +return ERR_PTR (ret); +}[..]
Quoted 18 lines
drivers/clk/keystone/sci-clk.c new file +18
-0+ +static int ti_sci_init_clocks (struct sci_clk_provider *p) +{ +struct sci_clk_data *data = p->clocks; +struct clk_hw *hw; +int i; + +while (data->num_clks) { + data->clocks =devm_kcalloc (p->dev, data->num_clks, +sizeof (struct sci_clk ), +GFP_KERNEL ); +if (!data->clocks) +return -ENOMEM ; + +for (i =0 ; i < data->num_clks; i++) { + hw =_sci_clk_build (p, data->dev, i); +if (!IS_ERR (hw)) { + data->clocks[i] = hw;
Quoted 8 lines
drivers/clk/keystone/sci-clk.c line 428
Quoted 5 lines
drivers/clk/keystone/sci-clk.c new file +5
-0+continue ; + } + +/* Skip any holes in the clock lists */ +if (PTR_ERR (hw) == -ENODEV )Does this happen? I don't see where
_sci_clk_build()returns-ENODEV.
Yes, it can and will happen. get_num_parents() called by sci_clk_build will return ENODEV for device/clock pairs that don't exist.
Quoted 28 lines
Quoted 28 lines
drivers/clk/keystone/sci-clk.c new file +26
-0+continue ; + +return PTR_ERR (hw); + } + data++; + } + +return 0 ; +} + + +/** +* ti_sci_clk_probe - Probe function for the TI SCI clock driver +* @pdev: platform device pointer to be probed +* +* Probes the TI SCI clock device. Allocates a new clock provider +* and registers this to the common clock framework. Also applies +* any required flags to the identified clocks via clock lists +* supplied from DT. Returns 0 for success, negative error value +* for failure. +*/ +static int ti_sci_clk_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct device_node *np = dev->of_node; +struct sci_clk_provider *provider;
Quoted 8 lines
drivers/clk/keystone/sci-clk.c line 536
Quoted 6 lines
drivers/clk/keystone/sci-clk.c new file +6
-0+const struct ti_sci_handle *handle; +struct sci_clk_data *data; +int ret; + + data = (struct sci_clk_data *) +of_match_node (ti_sci_clk_of_match, np)->data;Just use
of_device_get_match_data()instead.
All righty.
Quoted 11 lines
Quoted 11 lines
drivers/clk/keystone/sci-clk.c new file +10
-0+ + handle =devm_ti_sci_get_handle (dev); +if (IS_ERR (handle)) +return PTR_ERR (handle); + + provider =devm_kzalloc (dev,sizeof (*provider),GFP_KERNEL ); +if (!provider) +return -ENOMEM ; + + provider->clocks = data;
Quoted 8 lines
drivers/clk/keystone/sci-clk.c line 552
Quoted 6 lines
drivers/clk/keystone/sci-clk.c new file +6
-0+ + provider->sci = handle; + provider->ops = &handle->ops.clk_ops; + provider->dev = dev; + +ti_sci_init_clocks (provider);And if this fails?
Yea this is kind of controversial. ti_sci_init_clocks() can fail if any of the clocks registered will fail. I decided to have it this way so that at least some clocks might work in failure cause, and you might have a booting device instead of total lock-up.
Obviously it could be done so that if any clock fails, we would de-register all clocks at that point, but personally I think this is a worse option.
ti_sci_init_clocks could probably be modified to continue registering clocks when a single clock fails though. Currently it aborts at first failure.
Thoughts on that?
Quoted 9 lines
drivers/clk/keystone/sci-clk.c line 558
Quoted 7 lines
drivers/clk/keystone/sci-clk.c new file +6
-0+ + ret =of_clk_add_hw_provider (np, sci_clk_get, provider); +if (ret) +return ret; + +return 0 ;Just "return
of_clk_add_hw_provider()" please.
True that, will fix.
-Tero
PATCH 3/3 clk: keystone: Add sci-clk driver support
Quoted 33 lines · Tero Kristo
Quoted 18 lines · Stephen Boyd
Quoted 17 lines · Tero Kristo
drivers/clk/keystone/sci-clk.c new file +10
-0+ + handle =devm_ti_sci_get_handle (dev); +if (IS_ERR (handle)) +return PTR_ERR (handle); + + provider =devm_kzalloc (dev,sizeof (*provider),GFP_KERNEL ); +if (!provider) +return -ENOMEM ; + + provider->clocks = data;Quoted 8 lines
drivers/clk/keystone/sci-clk.c line 552
Quoted 6 lines
drivers/clk/keystone/sci-clk.c new file +6
-0+ + provider->sci = handle; + provider->ops = &handle->ops.clk_ops; + provider->dev = dev; + +ti_sci_init_clocks (provider);And if this fails?
Yea this is kind of controversial.
ti_sci_init_clocks()can fail if any of the clocks registered will fail. I decided to have it this way so that at least some clocks might work in failure cause, and you might have a booting device instead of total lock-up.
Quoted 8 lines
Obviously it could be done so that if any clock fails, we would de-register all clocks at that point, but personally I think this is a worse option.
ti_sci_init_clockscould probably be modified to continue registering clocks when a single clock fails though. Currently it aborts at first failure.
That sounds like a better approach if we don't care about failures to register a clock. Returning a value from a function and not using it isn't really a great design.
I worry that if we start returning errors from clk_hw_register() that something will go wrong though, so really I don't know why we want to ignore errors at all. Just for debugging a boot hang? Can't we use early console to at least see that this driver is failing to probe and debug that way?
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH 3/3 clk: keystone: Add sci-clk driver support
Quoted 47 lines · Stephen Boyd
Quoted 33 lines · Tero Kristo
Quoted 18 lines · Stephen Boyd
Quoted 17 lines · Tero Kristo
drivers/clk/keystone/sci-clk.c new file +10
-0+ + handle =devm_ti_sci_get_handle (dev); +if (IS_ERR (handle)) +return PTR_ERR (handle); + + provider =devm_kzalloc (dev,sizeof (*provider),GFP_KERNEL ); +if (!provider) +return -ENOMEM ; + + provider->clocks = data;Quoted 8 lines
drivers/clk/keystone/sci-clk.c line 552
Quoted 6 lines
drivers/clk/keystone/sci-clk.c new file +6
-0+ + provider->sci = handle; + provider->ops = &handle->ops.clk_ops; + provider->dev = dev; + +ti_sci_init_clocks (provider);And if this fails?
Yea this is kind of controversial.
ti_sci_init_clocks()can fail if any of the clocks registered will fail. I decided to have it this way so that at least some clocks might work in failure cause, and you might have a booting device instead of total lock-up.Quoted 8 lines
Obviously it could be done so that if any clock fails, we would de-register all clocks at that point, but personally I think this is a worse option.
ti_sci_init_clockscould probably be modified to continue registering clocks when a single clock fails though. Currently it aborts at first failure.That sounds like a better approach if we don't care about failures to register a clock. Returning a value from a function and not using it isn't really a great design.
Quoted 5 lines
I worry that if we start returning errors from
clk_hw_register()that something will go wrong though, so really I don't know why we want to ignore errors at all. Just for debugging a boot hang? Can't we use early console to at least see that this driver is failing to probe and debug that way?
Early console can be used to debug that, but it is kind of annoying to recompile most of the kernel when you suddenly need to use it.
How about modifying the ti_sci_init_clocks func to print an error for each failed clock?
If you insist on aborting the probe though if a single clock fails, I can do that also.
-Tero
PATCH 3/3 clk: keystone: Add sci-clk driver support
Quoted 48 lines · Tero Kristo
Quoted 47 lines · Stephen Boyd
Quoted 33 lines · Tero Kristo
Quoted 18 lines · Stephen Boyd
Quoted 17 lines · Tero Kristo
drivers/clk/keystone/sci-clk.c new file +10
-0+ + handle =devm_ti_sci_get_handle (dev); +if (IS_ERR (handle)) +return PTR_ERR (handle); + + provider =devm_kzalloc (dev,sizeof (*provider),GFP_KERNEL ); +if (!provider) +return -ENOMEM ; + + provider->clocks = data;Quoted 8 lines
drivers/clk/keystone/sci-clk.c line 552
Quoted 6 lines
drivers/clk/keystone/sci-clk.c new file +6
-0+ + provider->sci = handle; + provider->ops = &handle->ops.clk_ops; + provider->dev = dev; + +ti_sci_init_clocks (provider);And if this fails?
Yea this is kind of controversial.
ti_sci_init_clocks()can fail if any of the clocks registered will fail. I decided to have it this way so that at least some clocks might work in failure cause, and you might have a booting device instead of total lock-up.Quoted 8 lines
Obviously it could be done so that if any clock fails, we would de-register all clocks at that point, but personally I think this is a worse option.
ti_sci_init_clockscould probably be modified to continue registering clocks when a single clock fails though. Currently it aborts at first failure.That sounds like a better approach if we don't care about failures to register a clock. Returning a value from a function and not using it isn't really a great design.
Quoted 8 lines
Quoted 5 lines
I worry that if we start returning errors from
clk_hw_register()that something will go wrong though, so really I don't know why we want to ignore errors at all. Just for debugging a boot hang? Can't we use early console to at least see that this driver is failing to probe and debug that way?Early console can be used to debug that, but it is kind of annoying to recompile most of the kernel when you suddenly need to use it.
I thought SERIAL_EARLYCON was selected by drivers that support it? So there shouldn't be any rebuilding required.
Quoted 3 lines
How about modifying the
ti_sci_init_clocksfunc to print an error for each failed clock?
Ok that's fine too. I'd prefer the function had a return type of void if we're not planning on using the return value, that's all.
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH 3/3 clk: keystone: Add sci-clk driver support
Quoted 55 lines · Stephen Boyd
Quoted 54 lines · Tero Kristo
Quoted 52 lines · Stephen Boyd
Quoted 33 lines · Tero Kristo
Quoted 18 lines · Stephen Boyd
Quoted 17 lines · Tero Kristo
drivers/clk/keystone/sci-clk.c new file +10
-0+ + handle =devm_ti_sci_get_handle (dev); +if (IS_ERR (handle)) +return PTR_ERR (handle); + + provider =devm_kzalloc (dev,sizeof (*provider),GFP_KERNEL ); +if (!provider) +return -ENOMEM ; + + provider->clocks = data;Quoted 8 lines
drivers/clk/keystone/sci-clk.c line 552
Quoted 6 lines
drivers/clk/keystone/sci-clk.c new file +6
-0+ + provider->sci = handle; + provider->ops = &handle->ops.clk_ops; + provider->dev = dev; + +ti_sci_init_clocks (provider);And if this fails?
Yea this is kind of controversial.
ti_sci_init_clocks()can fail if any of the clocks registered will fail. I decided to have it this way so that at least some clocks might work in failure cause, and you might have a booting device instead of total lock-up.Quoted 8 lines
Obviously it could be done so that if any clock fails, we would de-register all clocks at that point, but personally I think this is a worse option.
ti_sci_init_clockscould probably be modified to continue registering clocks when a single clock fails though. Currently it aborts at first failure.That sounds like a better approach if we don't care about failures to register a clock. Returning a value from a function and not using it isn't really a great design.
I worry that if we start returning errors from
clk_hw_register()that something will go wrong though, so really I don't know why we want to ignore errors at all. Just for debugging a boot hang? Can't we use early console to at least see that this driver is failing to probe and debug that way?
Quoted 5 lines
Quoted 2 lines
Early console can be used to debug that, but it is kind of annoying to recompile most of the kernel when you suddenly need to use it.
I thought
SERIAL_EARLYCONwas selected by drivers that support it? So there shouldn't be any rebuilding required.
Actually you can probably ignore my comment, I was just speaking out of OMAP experience where the clocks are initialized very early, but this doesn't apply to keystone. Sci-clock is a proper driver now with proper probe etc. in place so my comment here is invalid.
Quoted 7 lines
Quoted 4 lines
How about modifying the
ti_sci_init_clocksfunc to print an error for each failed clock?Ok that's fine too. I'd prefer the function had a return type of void if we're not planning on using the return value, that's all.
Ok, I'll see which way I go in v2 of the series, but seems I can pick either your original proposal or mine.
-Tero