clk: introduce TC9564 clock and reset
Subsystems (5)
cover clk: introduce TC9564 clock and reset
This series adds support for the clock and reset controller present within the TC9564 SoC.
This SoC attaches to a host system via PCIe. It includes a PCIe switch with three downstream ports. To one of these ports an embedded PCIe endpoint is attached, and it implements two PCI functions. BAR 4 for both of these PCI functions provides access to the registers that enable or disable a number of clock signals, and assert or deassert a set of reset lines.
The TC9564 PCI functions provide PCI endpoint buses via devicetree. This allows a PCI function to use devicetree to define subordinate devices accessible using one or more of the function's BARs. The clock/reset driver binds with a device defined this way. Only one of the two PCI functions is permitted to define the clock/reset device node.
-Alex
Note: to work correctly, this series depends on this:
https://lore.kernel.org/lkml/[email protected]/
Alex Elder (4):
dt-bindings: mfd: introduce the TC9564 config syscon
dt-bindings: clock: introduce toshiba,tc9564-clock.yaml
clk: toshiba: introduce a TC9564 SoC clock and reset driver
arm64: dts: qcom: qcs6490-rb3gen2: add the clock controller
.../bindings/clock/toshiba,tc9564-clock.yaml | 64 +++ .../bindings/mfd/toshiba,tc9564.yaml | 56 +++ MAINTAINERS | 9 + arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 22 +- drivers/clk/Kconfig | 11 + drivers/clk/Makefile | 1 + drivers/clk/clk-tc9564.c | 366 ++++++++++++++++++ include/dt-bindings/clock/toshiba,tc9564.h | 54 +++ 8 files changed, 582 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml create mode 100644 Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml create mode 100644 drivers/clk/clk-tc9564.c create mode 100644 include/dt-bindings/clock/toshiba,tc9564.h
base-commit: 79d34e34e9ab629be19e588ec2eb7e1855d6dc46
Signature · 1 line
2.53.0
PATCH 1/4 dt-bindings: mfd: introduce the TC9564 config syscon
Define the binding for a system controller used in the Toshiba TC9564 SoC.
--- .../bindings/mfd/toshiba,tc9564.yaml | 56 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 57 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +56 -0
@@ -0,0 +1,56 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/toshiba,tc9564.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Toshiba TC9564 System Controller + +maintainers: + - Alex Elder <[email protected]> + - Daniel Thompson <[email protected]> + +description: | + The Toshiba TC9564 is an SoC accessed by a host system through the + upstream PCIe port on the PCIe switch it implements. The switch includes + an embedded PCIe endpoint on one of its downstream ports that provides + access to various SoC peripherals (including a clock and reset controller) + via one of its BARs. A system controller provides managed access to the + first two pages of this memory region to ensure accesses made by these + peripherals produce well-defined results. + +properties: + compatible: + items: + - const: toshiba,tc9564-config + - const: syscon + - const: simple-mfd + + reg: + maxItems: 1 + + ranges: true + + '#address-cells': + const: 1 + + '#size-cells': + const: 1 + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + syscon@0 { + compatible = "toshiba,tc9564-config", + "syscon", + "simple-mfd"; + reg = <0x0 0x2000>; + #address-cells = <1>; + #size-cells = <1>; + ranges; + };
MAINTAINERS +1 -0
@@ -27674,6 +27674,7 @@ TOSHIBA TC9564 PCI DRIVER M: Alex Elder <[email protected]> M: Daniel Thompson <[email protected]> S: Maintained +F: Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml F: Documentation/devicetree/bindings/misc/pci1179,0220.yaml F: drivers/misc/tc9564-pci.c
Signature · 1 line
2.53.0
Re: [PATCH 1/4] dt-bindings: mfd: introduce the TC9564 config syscon
Quoted 21 lines · Alex Elder
Define the binding for a system controller used in the Toshiba TC9564 SoC.
Co-developed-by Daniel Thompson <[email protected]>Signed-off-by Daniel Thompson <[email protected]>Signed-off-by Alex Elder <[email protected]>--- .../bindings/mfd/toshiba,tc9564.yaml | 56 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 57 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/toshiba,tc9564.yamlDocumentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +3
-0@@ -0,0 +1,56 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +---
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml line 13
Quoted 10 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +10
-0+$id: http://devicetree.org/schemas/mfd/toshiba,tc9564.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Toshiba TC9564 System Controller + +maintainers: + - Alex Elder <[email protected]> + - Daniel Thompson <[email protected]> + +description: |
Do not need '|' unless you need to preserve formatting.
Quoted 15 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +15
-0+ The Toshiba TC9564 is an SoC accessed by a host system through the + upstream PCIe port on the PCIe switch it implements. The switch includes + an embedded PCIe endpoint on one of its downstream ports that provides + access to various SoC peripherals (including a clock and reset controller) + via one of its BARs. A system controller provides managed access to the + first two pages of this memory region to ensure accesses made by these + peripherals produce well-defined results. + +properties: + compatible: + items: + - const: toshiba,tc9564-config + - const: syscon + - const: simple-mfd +
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml line 38
Quoted 10 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +10
-0+ reg: + maxItems: 1 + + ranges: true + + '#address-cells': + const: 1 + + '#size-cells': + const: 1
Both properties and simple-mfd are redundant. You do not have children.
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml line 44
Quoted 6 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +6
-0+ +required: + - compatible + - reg + +unevaluatedProperties: false
And this should be additionalProperties instead
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml line 55
Quoted 11 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +11
-0+ +examples: + - | + syscon@0 { + compatible = "toshiba,tc9564-config", + "syscon", + "simple-mfd"; + reg = <0x0 0x2000>; + #address-cells = <1>; + #size-cells = <1>; + ranges;
As you can see here - no children.
I also have doubts that this is needed - I see no updates to the misc binding, which would be referencing it. But then another point would be, that you do not need separate child node, which has no properties.
Well, has one - address space.
Lack of full picture is not helping here.
Best regards,
Krzysztof
Re: [PATCH 1/4] dt-bindings: mfd: introduce the TC9564 config syscon
Quoted 7 lines · Krzysztof Kozlowski
Quoted 6 lines · Alex Elder
Define the binding for a system controller used in the Toshiba TC9564 SoC.
Co-developed-by Daniel Thompson <[email protected]>Signed-off-by Daniel Thompson <[email protected]>Signed-off-by Alex Elder <[email protected]>
Thank you for your feedback, Krzysztof.
Quoted 19 lines
Quoted 19 lines
--- .../bindings/mfd/toshiba,tc9564.yaml | 56 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 57 insertions(+)create mode 100644
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yamlDocumentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +7
-0@@ -0,0 +1,56 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/toshiba,tc9564.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Toshiba TC9564 System Controller
Quoted 8 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml line 13
Quoted 6 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +6
-0+ +maintainers: + - Alex Elder <[email protected]> + - Daniel Thompson <[email protected]> + +description: |Do not need '|' unless you need to preserve formatting.
OK.
Quoted 19 lines
Quoted 19 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +19
-0+ The Toshiba TC9564 is an SoC accessed by a host system through the + upstream PCIe port on the PCIe switch it implements. The switch includes + an embedded PCIe endpoint on one of its downstream ports that provides + access to various SoC peripherals (including a clock and reset controller) + via one of its BARs. A system controller provides managed access to the + first two pages of this memory region to ensure accesses made by these + peripherals produce well-defined results. + +properties: + compatible: + items: + - const: toshiba,tc9564-config + - const: syscon + - const: simple-mfd + + reg: + maxItems: 1 + + ranges: true
Quoted 8 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml line 38
Quoted 6 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +6
-0+ + '#address-cells': + const: 1 + + '#size-cells': + const: 1Both properties and simple-mfd are redundant. You do not have children.
I think ranges, #address-cells, and #size-cells might have been left here by mistake, and I hadn't noticed. (They are required to be included within a pci-ep-bus sub-node, but even if that's why they're here, they're in the wrong place.)
I will remove these three properties, as well as the "simple-mfd" compatible string in version 2.
Quoted 8 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml line 44
Quoted 6 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +6
-0+ +required: + - compatible + - reg + +unevaluatedProperties: falseAnd this should be additionalProperties instead
OK.
Quoted 11 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml line 55
Quoted 10 lines
Documentation/devicetree/bindings/mfd/toshiba,tc9564.yaml new file +10
-0+examples: + - | + syscon@0 { + compatible = "toshiba,tc9564-config", + "syscon", + "simple-mfd"; + reg = <0x0 0x2000>; + #address-cells = <1>; + #size-cells = <1>; + ranges;
Quoted 1 line
As you can see here - no children.
I have seen some examples of syscon nodes that incorporate sub-devices, while others do not.
Our purpose for defining a syscon here is to coordinate access for multiple devices to several registers located within the same page of memory.
I think this device was originally defined in a sub-node because the only memory accesses it requires are within the range covered by the syscon.
Should we instead define the syscon to be a fairly trivial standalone thing, and then refer to it in the clock/reset device node by phandle? (Or have the driver look it up by compatible string?)
tc9564_config_syscon0: syscon@0 {
compatible = "toshiba,tc9564-config",
"syscon";
reg = <0x0 0x2000>;
};
clock@1004 {
compatible = "toshiba,tc9564-clock";
toshiba,config-syscon = <&tc9564_config_syscon0 0x1004>;
#clock-cells = <1>;
#reset-cells = <1>;
};Quoted 3 lines
I also have doubts that this is needed - I see no updates to the misc binding, which would be referencing it. But then another point would be, that you do not need separate child node, which has no properties.
I'm sorry if I'm missing something. I saw other examples of syscon nodes being defined, and tried to copy what I saw, but obviously got it wrong.
Quoted 3 lines
Well, has one - address space.
Lack of full picture is not helping here.
I will gladly provide a better picture, but I also want to stay focused on what's necessary for the binding.
I have more in my next message.
-Alex
Quoted 2 lines
Best regards,
Krzysztof
Re: [PATCH 1/4] dt-bindings: mfd: introduce the TC9564 config syscon
Quoted 3 lines · Alex Elder
Quoted 3 lines
I also have doubts that this is needed - I see no updates to the misc binding, which would be referencing it. But then another point would be, that you do not need separate child node, which has no properties.
Krzysztof, I've done some additional work on this based on your feedback.
I think you are correct that there is no need to define this as a special syscon with its own compatible string.
Instead, I can simply define a simple syscon with a label:
tc9564_config_syscon0: syscon@0 {
compatible = "syscon";
reg = <0x0 0x2000>;
};And then have the devices that need to use it, refer to its label using a phandle:
tc9564_clock0: clock@1004 {
compatible = "toshiba,tc9564-clock";
toshiba,config-syscon = <&tc9564_config_syscon0>;
#clock-cells = <1>;
#reset-cells = <1>;
};
tc9564_emac0: ethernet@40000 {
compatible = "toshiba,tc9564-xgmac";
reg = <0x40000 0x8000>;
toshiba,config-syscon = <&tc9564_config_syscon0 0x1070>;
. . .
};
tc9564_emac1: ethernet@48000 {
compatible = "toshiba,tc9564-xgmac";
reg = <0x48000 0x8000>;
toshiba,config-syscon = <&tc9564_config_syscon0 0x1074>;
. . .
};That simplifies things a bit, and requires no devicetree binding.
If you think this is incorrect, please let me know, otherwise it is the direction I plan to go with the next version of this series.
Thank you again for your review.
-Alex
Re: [PATCH 1/4] dt-bindings: mfd: introduce the TC9564 config syscon
Quoted 8 lines · Alex Elder
Quoted 3 lines · Alex Elder
Quoted 3 lines
I also have doubts that this is needed - I see no updates to the misc binding, which would be referencing it. But then another point would be, that you do not need separate child node, which has no properties.
Krzysztof, I've done some additional work on this based on your feedback.
Quoted 7 lines
I think you are correct that there is no need to define this as a special syscon with its own compatible string.
Instead, I can simply define a simple syscon with a label:
tc9564_config_syscon0: syscon@0 { compatible = "syscon";
I never proposed anything like that. I asked to drop simple-mfd.
Quoted 27 lines
reg = <0x0 0x2000>; };And then have the devices that need to use it, refer to its label using a phandle:
tc9564_clock0: clock@1004 { compatible = "toshiba,tc9564-clock"; toshiba,config-syscon = <&tc9564_config_syscon0>; #clock-cells = <1>; #reset-cells = <1>; }; tc9564_emac0: ethernet@40000 { compatible = "toshiba,tc9564-xgmac"; reg = <0x40000 0x8000>; toshiba,config-syscon = <&tc9564_config_syscon0 0x1070>; . . . }; tc9564_emac1: ethernet@48000 { compatible = "toshiba,tc9564-xgmac"; reg = <0x48000 0x8000>; toshiba,config-syscon = <&tc9564_config_syscon0 0x1074>; . . . };
Quoted 6 lines
That simplifies things a bit, and requires no devicetree binding.
If you think this is incorrect, please let me know, otherwise it is the direction I plan to go with the next version of this series.
I think nothing behind PCI EP should be described in DT, because everything is internal to the device and implied by compatible (or vid/pid). We do describe internals of devices in DT usually for complex devices (like some display blocks with multiple subblocks) or when they are used outside of that device. That's why I asked how are all these clocks and resets exposed to the host. If the syscon is used only by the device itself, I don't quite get the benefits of describing it in DT.
Best regards,
Krzysztof
Re: [PATCH 1/4] dt-bindings: mfd: introduce the TC9564 config syscon
Quoted 7 lines · Krzysztof Kozlowski
I think nothing behind PCI EP should be described in DT, because everything is internal to the device and implied by compatible (or vid/pid). We do describe internals of devices in DT usually for complex devices (like some display blocks with multiple subblocks) or when they are used outside of that device. That's why I asked how are all these clocks and resets exposed to the host. If the syscon is used only by the device itself, I don't quite get the benefits of describing it in DT.
You say you think "nothing behind PCI EP should be described in DT". However the mere existence of the PCI endpoint bus as an option contradicts that statement, by providing a means of doing *exactly*
that: using devicetree to describe things behind a PCI endpoint. This is the model we are using.
In June we switched over to using the pci-ep-bus model based on a suggestion from Rob Herring (and Mani Sadhasivam). Rob felt our previous auxiliary device approach wasn't right for the peripherals found within the SoC, given we were accessing them via PCI BARs.
https://lore.kernel.org/lkml/[email protected]/
We found that using pci-ep-bus provided a very natural and familiar way of representing the organization of the TC9564 SoC, and used it to represent all of the peripherals (including XGMAC). It nicely partitions the hardware design, as well as providing a well-understood connection with the software.
The switch involved quite a bit of work, and the result is a set of basically independent platform drivers that are modular and easily reviewed. I really would rather not go back to to what we had before.
Do you have criteria for what in a chip/SoC is sufficiently "complex" to warrant allowing devicetree to describe some of its internal details?
I believe the TC9564 *is* such a "complex" SoC. And in any case, anywhere pci-ep-bus is used, it implies that some chip internals would be described using devicetree.
Would incorporating the chip internals in a devicetree *overlay* have any effect on your opinion on this? I.e., the "main" devicetree nodes would simply define the PCI device hierarchy, but anything behind an endpoint would be described in an overlay. This way, pci-ep-bus on a PCI endpoint just provides an external interface onto which a very complex device could be "mounted" (and described in an overlay).
I'd like to continue to move ahead with upstreaming support for this SoC, but to me it seems you have basic doubts about pci-ep-bus and I'd like to get past that.
Thanks.
-Alex
Re: [PATCH 1/4] dt-bindings: mfd: introduce the TC9564 config syscon
Quoted 9 lines · Alex Elder
Quoted 7 lines · Krzysztof Kozlowski
I think nothing behind PCI EP should be described in DT, because everything is internal to the device and implied by compatible (or vid/pid). We do describe internals of devices in DT usually for complex devices (like some display blocks with multiple subblocks) or when they are used outside of that device. That's why I asked how are all these clocks and resets exposed to the host. If the syscon is used only by the device itself, I don't quite get the benefits of describing it in DT.
Quoted 5 lines
You say you think "nothing behind PCI EP should be described in DT". However the mere existence of the PCI endpoint bus as an option contradicts that statement, by providing a means of doing *exactly*
that: using devicetree to describe things behind a PCI endpoint. This is the model we are using.
No, the existence does not contradict. We allow further description as long as things are not autodiscoverable and are necessary for the host to deal with the device, like power sequencing. Same we did for "onboard" USB hubs.
You are however describing internals of the device like syscon, which is way beyond what is necessary for the host to instantiate that device.
Quoted 6 lines
In June we switched over to using the pci-ep-bus model based on a suggestion from Rob Herring (and Mani Sadhasivam). Rob felt our previous auxiliary device approach wasn't right for the peripherals found within the SoC, given we were accessing them via PCI BARs.
https://lore.kernel.org/lkml/[email protected]/
Oh, that's good, so let's quote:
"The ethernet nodes should be just these PCI function nodes."
"And really, a DT node for them should be completely optional (unless there's some power on ctrl needed)."
"Everything else like SPI, GPIO, UART, etc. should be under the PCIe switch upstream node in a pci-ep-bus."
Sure, but you add syscon. That's completely internal stuff.
Quoted 6 lines
We found that using pci-ep-bus provided a very natural and familiar way of representing the organization of the TC9564 SoC, and used it to represent all of the peripherals (including XGMAC). It nicely partitions the hardware design, as well as providing a well-understood connection with the software.
Quoted 8 lines
The switch involved quite a bit of work, and the result is a set of basically independent platform drivers that are modular and easily reviewed. I really would rather not go back to to what we had before.
Do you have criteria for what in a chip/SoC is sufficiently "complex" to warrant allowing devicetree to describe some of its internal details?
For me criteria are the resources from the host. I think I said it already, we do not describe internals of I2C or SPI devices: their internal clocks, internal power supplies. We only describe the inputs from the host. You can map it into the pins.
Same here, IMO.
Quoted 13 lines
I believe the TC9564 *is* such a "complex" SoC. And in any case, anywhere pci-ep-bus is used, it implies that some chip internals would be described using devicetree.
Would incorporating the chip internals in a devicetree *overlay* have any effect on your opinion on this? I.e., the "main" devicetree nodes would simply define the PCI device hierarchy, but anything behind an endpoint would be described in an overlay. This way, pci-ep-bus on a PCI endpoint just provides an external interface onto which a very complex device could be "mounted" (and described in an overlay).
Quoted 3 lines
I'd like to continue to move ahead with upstreaming support for this SoC, but to me it seems you have basic doubts about pci-ep-bus and I'd like to get past that.
Well, Rob might understand this better, so maybe review from him will solve these questions.
Best regards,
Krzysztof
Re: [PATCH 1/4] dt-bindings: mfd: introduce the TC9564 config syscon
Quoted 14 lines · Krzysztof Kozlowski
Quoted 13 lines · Alex Elder
On 9/23/26 2:25 AM, Krzysztof Kozlowski wrote:
I believe the TC9564 *is* such a "complex" SoC. And in any case, anywhere pci-ep-bus is used, it implies that some chip internals would be described using devicetree.Would incorporating the chip internals in a devicetree *overlay* have any effect on your opinion on this? I.e., the "main" devicetree nodes would simply define the PCI device hierarchy, but anything behind an endpoint would be described in an overlay. This way, pci-ep-bus on a PCI endpoint just provides an external interface onto which a very complex device could be "mounted" (and described in an overlay).
Quoted 6 lines
Quoted 3 lines
I'd like to continue to move ahead with upstreaming support for this SoC, but to me it seems you have basic doubts about pci-ep-bus and I'd like to get past that.
Well, Rob might understand this better, so maybe review from him will solve these questions.
To aid whichever of the DT maintainers can help with review, I'd like to offer a little more background on the "TC9564 is a complex device" thread.
Hopefully it doesn't contradict what you've been discussing with Alex, but it will perhaps give a different perspective.
My thoughts neatly divide into three parts:
1. The TC956x-series form a complex enough family of multi-function devices. We will need some way to describe it's structure (both external *and* internal).
2. There are multiple ways to describe a device's connectivity but devicetree is great for this job making it an excellent technical solution.
3. We have done sufficient thought experiments (and real-world experiments) to be confident that our solution is not in conflict with treating the QPS615 as a regular PCIe card.
Having offered the summary above because I now need to spend a lot of words to back up the above. I'm afraid I have to go a long way past the syscon bindings perimeter!
## Why I think the QPS615 is "complex"
I hope I don't have to rehash the multi-function part too much. The TC9564 included comms peripherals (UART, I2C, SPI, GPIO), some very common SoC-ish management peripherals (reset, clock, interrupt controller), a Cortex-M3 controller (with access to pretty much everything) and some licensed peripherals with well established devicetree bindings (2x Designware XGMAC).
It should be noted that TC9564 does also include a PCIe switch. However the switch is *upstream* of the PCIe endpoint we use to control most of the other TC9564 peripherals so you won't see it any any bindings that have come from Alex and me.
Another factor that makes describing TC9564 complex is the presence or absence of side channels that give the host SoC mechanisms to communicate with the TC9564 without using PCIe.
* On RB3gen2 there are host GPIOs that control TC9564 peripherals such as the ethernet phys (used for power control). Additionally there is an I2C side channel that is used to control the TC9564 GPIOs. This is needed to solve a chicken-and-egg problem related to the PCIe #PERST signals in the PCIe switch but also ends up being used to reset the ethernet phys.
* On a real PCIe card there will be no side channels, which means the TC9564 GPIO peripheral would suddenly become status = "okay" on the PCIe bus instead of the I2C bus (in this case we know that there is no chicken-and-egg... if there were then the card won't work electrically).
Thus, even with one part number, we already have some fairly significant connectivity difference depending on the board design.
Finally, the TC956x is a family and we'd like to reuse as much as possible across the part numbers that will come. Having a convenient way to reuse common subsystems is is very valuable.
## Why devicetree is a great technical solution
Clearly we *could* rely on a big pile of what is essentially platform bus code in the PCIe driver to describe the connectivity and familial difference. Renaming it to auxiliary bus code doesn't stop the results from feeling like fifteen-year old platform bus code.
For that reason we view devicetree as a better solution to model the hardware. DT already has idiomatic ways to describe the connectivity of the device.
Most of the non-discoverable baseboard <-> TC9564 connections are for the MAC and using devicetree simplifies the binding there since we add only a few essentials over the existing stmmac bindings (there are existing stmmac PCIe cards but I believe many are for MACs with a smaller feature set than TC9564 which makes things easier).
You are correct that for several other peripherals we only describe internal connectivity. Ultimately we are copying prior art from the Raspberry Pi RP1. RP1 is also a complex MFD that exposes its functions in a similar way to what we propose for TC9564 (and on Raspberry Pi 5 is it soldered down just like it is on RB3gen2). FWIW I'd note that I think the RP1 bindings describe peripherals with no external connectivity.
So, it's not that the card can't come alive if we use open-coded C to describe the connectivity. It can. We've been there and done it. Instead the argument is that *not* using DT to describe TC956x family parts in their various baseboards is a missed opportunity!
## Using TC9564 as a PCIe card
Currently our work only supports QPS615 soldered onto an RB3gen2 with all it's sidebands. Here we settled on a static devicetree (no overlay) similar to Raspberry Pi RP1. We tried overlays, the resulting DTs were fragmented and hard to read. Ultimately we felt embedded developers would find working with a static DT easier.
If you feel super strongly that the syscon bindings (which is what this thread is about) should not appear in a static device tree, we maybe we could discuss overlaying the discoverable-from-PID-VID parts as an "embedded TC9564" overlay. Personally I really dislike this approach because it's hard to read. Also it wouldn't actually impact the DT bindings so perhaps isn't in scope yet anyway!
For real PCIe cards I definitely think DT overlay is the answer. Here there are no sidebands meaning there will be no phandles linking the overlay to the base DT (if there is one). That resolved the concerns about readability of the overlay. If we actually had a PCIe card today then I'm confident we'd be able to reuse almost all the driver work Alex has posted so far and that we'd be able to run it on a system using ACPI as its primary hardware model.
There are still some warts related to PCIe card identification but they are unrelated to the DT bindings. TC9564 has a fixed VID/PID so, when there is no static DT to help us, we must dynamically interrogate the card from the pcie driver to discover what it actually is and figure out *which* overlay to load. This is another reason I view devicetree as
an excellent technical solution. Having an easy way to describe each PCIe card form its schematic is a great property to have.
In short, the TC9564 on a real removable PCIe card has been an important thought experiment for me throughout this work. It's also been a real experiment: we did implement dynamic overlay for RB3gen2 and have it working. The thing that moved us back to static DT for this board was the sidebands and these don't exist for a real PCIe card.
## That's all folks!
In short, for zany embedded designs with nasty (and mandatory) side channels, we'd like to use the DT bindings we have proposed from static DT. This should be comfortable and ergonomic for embedded developers who are accustomed to slinging DTs around.
For sensible grown up designs that need to cooperate with both DT and ACPI, we'd like to use DT overlay. This essentially makes the use of DT an implementation detail that reduces maintenance by avoiding reinvention.
Daniel.
PATCH 2/4 dt-bindings: clock: introduce toshiba,tc9564-clock.yaml
In reply to Alex ElderDefine the binding for the clock controller functionality present in the Toshiba TC9564 SoC.
--- .../bindings/clock/toshiba,tc9564-clock.yaml | 64 +++++++++++++++++++ MAINTAINERS | 7 ++ include/dt-bindings/clock/toshiba,tc9564.h | 54 ++++++++++++++++ 3 files changed, 125 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml create mode 100644 include/dt-bindings/clock/toshiba,tc9564.h
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml new file +64 -0
@@ -0,0 +1,64 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/clock/toshiba,tc9564-clock.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Toshiba TC9564 Clock and Reset Controller + +maintainers: + - Alex Elder <[email protected]> + - Daniel Thompson <[email protected]> + +description: | + The Toshiba TC9564 is an SoC accessed by a host system through the + upstream PCIe port on the PCIe switch it implements. The switch + includes an embedded PCIe endpoint that provides access to various + SoC peripherals (including a clock/reset controller) via its BARs. + + The SoC is represented using a devicetree PCI endpoint bus, so the + clock controller is bound to its driver using a platform driver + compatible string. All of the clocks are simple gate clocks. + + A total of 21 clocks are implemented, though two of these are not + controllable. Access to the clock controller relies on PCIe being + functional, so the PCIe clock is assumed to be always on. Similarly, + the PCIe controller relies on I2C, so the I2C clock is also assumed + to be always on. + + In addition, 13 resets are implemented. Similar to clocks, the PCIe + and I2C resets are assumed to be deasserted and cannot be controlled. + + Clock and reset ids are defined in <dt-bindings/clock/toshiba,tc9564.h>. + +properties: + compatible: + const: toshiba,tc9564-clock + + reg: + maxItems: 1 + + "#clock-cells": + const: 1 + + "#reset-cells": + const: 1 + +required: + - compatible + - reg + - "#clock-cells" + - "#reset-cells" + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/toshiba,tc9564.h> + + clock@1004 { + compatible = "toshiba,tc9564-clock"; + reg = <0x1004 0x20>; + #clock-cells = <1>; + #reset-cells = <1>; + };
MAINTAINERS +7 -0
@@ -27670,6 +27670,13 @@ F: Documentation/devicetree/bindings/media/i2c/toshiba,tc358743.txt F: drivers/media/i2c/tc358743* F: include/media/i2c/tc358743.h +TOSHIBA TC9564 CLOCK DRIVER +M: Alex Elder <[email protected]> +M: Daniel Thompson <[email protected]> +S: Maintained +F: Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml +F: include/dt-bindings/clock/toshiba,tc9564.h + TOSHIBA TC9564 PCI DRIVER M: Alex Elder <[email protected]> M: Daniel Thompson <[email protected]>
include/dt-bindings/clock/toshiba,tc9564.h new file +54 -0
54 lines not on this page · raw message
Signature · 1 line
2.53.0
Re: [PATCH 2/4] dt-bindings: clock: introduce toshiba,tc9564-clock.yaml
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml line 20
Quoted 8 lines · Alex Elder
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml new file +8
-0+description: | + The Toshiba TC9564 is an SoC accessed by a host system through the + upstream PCIe port on the PCIe switch it implements. The switch + includes an embedded PCIe endpoint that provides access to various + SoC peripherals (including a clock/reset controller) via its BARs. + + The SoC is represented using a devicetree PCI endpoint bus, so the + clock controller is bound to its driver using a platform driver
Do not mention drivers here. Just say that host can access clocks etc via PCI.
But then more questions. This is PCI, yes? So how are the clocks from PCI routed BACK to the main soc, which would justify having this in DTS in the first place?
Quoted 33 lines
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml new file +33
-0+ compatible string. All of the clocks are simple gate clocks. + + A total of 21 clocks are implemented, though two of these are not + controllable. Access to the clock controller relies on PCIe being + functional, so the PCIe clock is assumed to be always on. Similarly, + the PCIe controller relies on I2C, so the I2C clock is also assumed + to be always on. + + In addition, 13 resets are implemented. Similar to clocks, the PCIe + and I2C resets are assumed to be deasserted and cannot be controlled. + + Clock and reset ids are defined in <dt-bindings/clock/toshiba,tc9564.h>. + +properties: + compatible: + const: toshiba,tc9564-clock + + reg: + maxItems: 1 + + "#clock-cells": + const: 1 + + "#reset-cells": + const: 1 + +required: + - compatible + - reg + - "#clock-cells" + - "#reset-cells" + +unevaluatedProperties: false
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml line 63
Quoted 10 lines
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml new file +10
-0+ +examples: + - | + #include <dt-bindings/clock/toshiba,tc9564.h> + + clock@1004 { + compatible = "toshiba,tc9564-clock"; + reg = <0x1004 0x20>; + #clock-cells = <1>; + #reset-cells = <1>;
No resources except small address space, so does not look like a separate device and should be folded into the parent node.
Quoted 30 lines
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml new file +1
-0+ };MAINTAINERS +7
-0@@ -27670,6 +27670,13 @@ F: Documentation/devicetree/bindings/media/i2c/toshiba,tc358743.txt F: drivers/media/i2c/tc358743* F: include/media/i2c/tc358743.h +TOSHIBA TC9564 CLOCK DRIVER +M: Alex Elder <[email protected]> +M: Daniel Thompson <[email protected]> +S: Maintained +F: Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml +F: include/dt-bindings/clock/toshiba,tc9564.h + TOSHIBA TC9564 PCI DRIVER M: Alex Elder <[email protected]> M: Daniel Thompson <[email protected]>include/dt-bindings/clock/toshiba,tc9564.h new file +5
-0@@ -0,0 +1,54 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ + +/* +* Copyright (C) 2026 by RISCstar Solutions Corporation. All rights reserved. +*/
include/dt-bindings/clock/toshiba,tc9564.h line 15
Quoted 10 lines
include/dt-bindings/clock/toshiba,tc9564.h new file +10
-0+ +#ifndef __ClOCK_TOSHIBA_TC9564_H__ +#define __ClOCK_TOSHIBA_TC9564_H__ + +/* Clock IDs */ + +#define CLOCK_MCU 0 +#define CLOCK_INTC 1 +/* #define CLOCK_PCIE 2 */ +/* #define CLOCK_I2C 3 */
Drop both, and SRAM is number 2. Or enable and implement them.
Best regards,
Krzysztof
Re: [PATCH 2/4] dt-bindings: clock: introduce toshiba,tc9564-clock.yaml
Quoted 2 lines · Krzysztof Kozlowski
Quoted 1 line · Alex Elder
+description: |
I suppose I don't need the '|' here either.
Quoted 10 lines
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml line 20
Quoted 7 lines
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml new file +7
-0+ The Toshiba TC9564 is an SoC accessed by a host system through the + upstream PCIe port on the PCIe switch it implements. The switch + includes an embedded PCIe endpoint that provides access to various + SoC peripherals (including a clock/reset controller) via its BARs. + + The SoC is represented using a devicetree PCI endpoint bus, so the + clock controller is bound to its driver using a platform driverDo not mention drivers here. Just say that host can access clocks etc via PCI.
Daniel and I discussed this this morning, and in the next version I'll explain things from a little different perspective, and I think it will help keep this focused on the hardware.
This particular binding should not even mention anything about PCI. Using the PCI endpoint bus means that the devices can be bound based on compatible string (i.e. with platform drivers), with no knowledge of access being provided by PCI.
Quoted 3 lines
But then more questions. This is PCI, yes? So how are the clocks from PCI routed BACK to the main soc, which would justify having this in DTS in the first place?
This clock+reset controller is one of several IP blocks within an SoC (Toshiba TC9564). Most of these clock and reset signals control things internal to this SoC, but at least one clock signal and a few reset lines are exposed externally, allowing them to be used by the platform that incorporates the SoC.
Ultimately the TC9564 SoC has a single 25 MHz input clock, and internal to the SoC there is an internal PLL that then drives other clocks at rates derived from that. We can only enable/disable the clocks (we have no rate control).
But I think you have more and deeper questions too.
We are using the *PCI endpoint bus* model to represent this SoC. To me it seems that there is not full agreement on some aspects of this model.
It is a weird situation, where the use of this *software* model provides a means of more completely describing (using devicetree) details of the hardware "behind" a PCI endpoint.
Despite PCI doing its own enumeration, PCI_DYNAMIC_OF_NODES allows the PCI hierarchy to *also* have devicetree nodes representing bridges and endpoints.
Building on that, a PCI function with a devicetree node can include a "pci-ep-bus" sub-node. That sub-node provides a place to describe IP blocks (devices) that can be accessed via the endpoint bus.
In our case, in addition to this clock+reset device, the TC9564 SoC implements a UART, a GPIO controller, a network interface, and so on. All of these are accessed via the endpoint bus associated with the PCI function's BAR 4 address space.
So this model differs from "normal" PCI devices in that a single PCI endpoint (function) can expose more than just one capability (in contrast with, for example, a PCI function that implements only an Ethernet interface).
Quoted 38 lines
Quoted 38 lines
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml new file +38
-0+ compatible string. All of the clocks are simple gate clocks. + + A total of 21 clocks are implemented, though two of these are not + controllable. Access to the clock controller relies on PCIe being + functional, so the PCIe clock is assumed to be always on. Similarly, + the PCIe controller relies on I2C, so the I2C clock is also assumed + to be always on. + + In addition, 13 resets are implemented. Similar to clocks, the PCIe + and I2C resets are assumed to be deasserted and cannot be controlled. + + Clock and reset ids are defined in <dt-bindings/clock/toshiba,tc9564.h>. + +properties: + compatible: + const: toshiba,tc9564-clock + + reg: + maxItems: 1 + + "#clock-cells": + const: 1 + + "#reset-cells": + const: 1 + +required: + - compatible + - reg + - "#clock-cells" + - "#reset-cells" + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/toshiba,tc9564.h> +
Quoted 8 lines
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml line 63
Quoted 5 lines
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml new file +5
-0+ clock@1004 { + compatible = "toshiba,tc9564-clock"; + reg = <0x1004 0x20>; + #clock-cells = <1>; + #reset-cells = <1>;No resources except small address space, so does not look like a separate device and should be folded into the parent node.
Modeling the TC9564 as an SoC using pci-ep-bus, this *is* sensibly a separate device.
We actually started with everything rolled into the stmmac networking driver, but after some discussion we concluded that the pci-ep-bus model would be simpler (and other than some challenges related to it being a relatively new thing, it *is* quite a bit simpler and better partitioned).
-Alex
Quoted 46 lines
Quoted 31 lines
Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml new file +1
-0+ };MAINTAINERS +7
-0@@ -27670,6 +27670,13 @@ F: Documentation/devicetree/bindings/media/i2c/toshiba,tc358743.txt F: drivers/media/i2c/tc358743* F: include/media/i2c/tc358743.h +TOSHIBA TC9564 CLOCK DRIVER +M: Alex Elder <[email protected]> +M: Daniel Thompson <[email protected]> +S: Maintained +F: Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml +F: include/dt-bindings/clock/toshiba,tc9564.h + TOSHIBA TC9564 PCI DRIVER M: Alex Elder <[email protected]> M: Daniel Thompson <[email protected]>include/dt-bindings/clock/toshiba,tc9564.h new file +5
-0@@ -0,0 +1,54 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ + +/* +* Copyright (C) 2026 by RISCstar Solutions Corporation. All rights reserved. +*/ include/dt-bindings/clock/toshiba,tc9564.h line 15
Quoted 10 lines
include/dt-bindings/clock/toshiba,tc9564.h new file +10
-0+ +#ifndef __ClOCK_TOSHIBA_TC9564_H__ +#define __ClOCK_TOSHIBA_TC9564_H__ + +/* Clock IDs */ + +#define CLOCK_MCU 0 +#define CLOCK_INTC 1 +/* #define CLOCK_PCIE 2 */ +/* #define CLOCK_I2C 3 */ Drop both, and SRAM is number 2. Or enable and implement them.
Best regards,
Krzysztof
Re: [PATCH 2/4] dt-bindings: clock: introduce toshiba,tc9564-clock.yaml
In reply to Krzysztof KozlowskiQuoted 14 lines · Krzysztof Kozlowski
Quoted 14 lines
include/dt-bindings/clock/toshiba,tc9564.h new file +8
-0@@ -0,0 +1,54 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ + +/* +* Copyright (C) 2026 by RISCstar Solutions Corporation. All rights reserved. +*/ + +#ifndef __ClOCK_TOSHIBA_TC9564_H__ +#define __ClOCK_TOSHIBA_TC9564_H__
Quoted 8 lines
include/dt-bindings/clock/toshiba,tc9564.h line 15
Quoted 7 lines
include/dt-bindings/clock/toshiba,tc9564.h new file +7
-0+ +/* Clock IDs */ + +#define CLOCK_MCU 0 +#define CLOCK_INTC 1 +/* #define CLOCK_PCIE 2 */ +/* #define CLOCK_I2C 3 */ Drop both, and SRAM is number 2. Or enable and implement them.
I forgot to respond to this. I think you are confused.
SRAM is BAR number 2. This is defining clock IDs.
And the reason these are commented out is that, while they do exist, we explicitly do not want anyone to disable either of these clocks because PCIe uses them. This clock (and reset) control is accessed via PCIe.
I do accept that someone might want me to simply omit these commented-out definitions, and I'd be fine with that.
But your suggestion wasn't sensible so I wanted to clear that up before deleting these.
-Alex
Re: [PATCH 2/4] dt-bindings: clock: introduce toshiba,tc9564-clock.yaml
Quoted 33 lines · Alex Elder
Quoted 14 lines · Krzysztof Kozlowski
Quoted 14 lines
include/dt-bindings/clock/toshiba,tc9564.h new file +8
-0@@ -0,0 +1,54 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ + +/* +* Copyright (C) 2026 by RISCstar Solutions Corporation. All rights reserved. +*/ + +#ifndef __ClOCK_TOSHIBA_TC9564_H__ +#define __ClOCK_TOSHIBA_TC9564_H__Quoted 8 lines
include/dt-bindings/clock/toshiba,tc9564.h line 15
Quoted 7 lines
include/dt-bindings/clock/toshiba,tc9564.h new file +7
-0+ +/* Clock IDs */ + +#define CLOCK_MCU 0 +#define CLOCK_INTC 1 +/* #define CLOCK_PCIE 2 */ +/* #define CLOCK_I2C 3 */ Drop both, and SRAM is number 2. Or enable and implement them.
I forgot to respond to this. I think you are confused.
SRAM is BAR number 2. This is defining clock IDs.
And the reason these are commented out is that, while they do exist, we explicitly do not want anyone to disable either of these clocks because PCIe uses them. This clock (and reset) control is accessed via PCIe.
Quoted 7 lines
I do accept that someone might want me to simply omit these commented-out definitions, and I'd be fine with that.
But your suggestion wasn't sensible so I wanted to clear that up before deleting these.
Indices should be continuous, there is no point in having holes in numbering.
Best regards,
Krzysztof
PATCH 3/4 clk: toshiba: introduce a TC9564 SoC clock and reset driver
In reply to Alex ElderDefine a new platform driver that manages clock and reset signals within the TC9564 SoC. There are 21 clocks, which can only be enabled and disabled, as well as 13 reset signals.
Two registers manage the state of the clocks and two others manage the state of the resets. The registers are accessed via a regmap supplied by a system controller, which coordinates access to a region of memory that will be shared with another driver.
Access to the memory region is provided via a BAR on a PCIe endpoint function embedded in the TC9564 SoC. For that reason, neither the PCIe clock nor PCIe reset can be manipulated by this driver (they are assumed always on and deasserted, respectively).
Similarly, control is not available for the I2C clock and reset, because the PCIe subsystem on the TC9564 relies on I2C
--- MAINTAINERS | 1 + drivers/clk/Kconfig | 11 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-tc9564.c | 366 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 379 insertions(+) create mode 100644 drivers/clk/clk-tc9564.c
MAINTAINERS +1 -0
@@ -27675,6 +27675,7 @@ M: Alex Elder <[email protected]> M: Daniel Thompson <[email protected]> S: Maintained F: Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml +F: drivers/clk/clk-tc9564.c F: include/dt-bindings/clock/toshiba,tc9564.h TOSHIBA TC9564 PCI DRIVER
drivers/clk/Kconfig +11 -0
@@ -292,6 +292,17 @@ config COMMON_CLK_S2MPS11 clock. These multi-function devices have two (S2MPS14) or three (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each. +config COMMON_CLK_TC9564 + tristate "Toshiba TC9564 clock support" + depends on TC9564_PCI + default m + help + This enables support for the clock and reset controller embedded + in the Toshiba TC9564 (and Qualcomm QPS615) SoC. The state of + clock and reset lines is controlled by MMIO to a region managed + by a system controller; this ensures access to the region is + coordinated between this and other drivers. + config CLK_TWL tristate "Clock driver for the TWL PMIC family" depends on TWL4030_CORE
drivers/clk/Makefile +1 -0
@@ -110,6 +110,7 @@ obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o obj-$(CONFIG_COMMON_CLK_SP7021) += clk-sp7021.o obj-$(CONFIG_COMMON_CLK_STM32F) += clk-stm32f4.o obj-$(CONFIG_COMMON_CLK_STM32H7) += clk-stm32h7.o +obj-$(CONFIG_COMMON_CLK_TC9564) += clk-tc9564.o obj-$(CONFIG_COMMON_CLK_TPS68470) += clk-tps68470.o obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o obj-$(CONFIG_CLK_TWL) += clk-twl.o
drivers/clk/clk-tc9564.c new file +366 -0
366 lines not on this page · raw message
Signature · 1 line
2.53.0
Re: [PATCH 3/4] clk: toshiba: introduce a TC9564 SoC clock and reset driver
Hello Alex,
drivers/clk/clk-tc9564.c line 14
Quoted 8 lines · Alex Elder
drivers/clk/clk-tc9564.c new file +8
-0+#include <linux/bits.h> +#include <linux/clk-provider.h> +#include <linux/mfd/syscon.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/reset-controller.h>
Please rely on linux/platform_device.h to provide of_device_id and drop including <linux/mod_devicetable.h>. (If you want to go full iwyu, include <linux/device-id/of.h> instead.)
drivers/clk/clk-tc9564.c line 351
Quoted 4 lines
drivers/clk/clk-tc9564.c new file +4
-0+static const struct of_device_id tc9564_clk_ids[] = { + { .compatible ="toshiba,tc9564-clock" }, + { }, +};
Drop the trailing comma after the list terminator please.
Best regards
Uwe
Re: [PATCH 3/4] clk: toshiba: introduce a TC9564 SoC clock and reset driver
Quoted 15 lines · Uwe Kleine-König
Hello Alex,
drivers/clk/clk-tc9564.c line 14
Quoted 8 lines · Alex Elder
drivers/clk/clk-tc9564.c new file +8
-0+#include <linux/bits.h> +#include <linux/clk-provider.h> +#include <linux/mfd/syscon.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/reset-controller.h> Please rely on
linux/platform_device.hto provideof_device_idand drop including <linux/mod_devicetable.h>. (If you want to go full iwyu, include <linux/device-id/of.h> instead.)
In general I would like to "go full iwyu" but have never heard it expressed that way. Can you recommend a tool available that will help me do that?
It looks like "mod_devicetable.h" doesn't help much unless I need struct_cpu_feature (which I do not), so I'll include "of.h".
Quoted 6 lines
drivers/clk/clk-tc9564.c line 351
Quoted 4 lines
drivers/clk/clk-tc9564.c new file +4
-0+static const struct of_device_id tc9564_clk_ids[] = { + { .compatible ="toshiba,tc9564-clock" }, + { }, +};Drop the trailing comma after the list terminator please.
OK. Only for the list terminator, right, because nothing will ever follow it? Non-empty final initializer is OK to have a comma as far as you're concerned?
Thanks for your review.
-Alex
Quoted 2 lines
Best regards
Uwe
Re: [PATCH 3/4] clk: toshiba: introduce a TC9564 SoC clock and reset driver
Hello Alex,
Quoted 15 lines · Alex Elder
Quoted 13 lines · Uwe Kleine-König
drivers/clk/clk-tc9564.c line 14
Quoted 8 lines · Alex Elder
drivers/clk/clk-tc9564.c new file +8
-0+#include <linux/bits.h> +#include <linux/clk-provider.h> +#include <linux/mfd/syscon.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/reset-controller.h> Please rely on
linux/platform_device.hto provideof_device_idand drop including <linux/mod_devicetable.h>. (If you want to go full iwyu, include <linux/device-id/of.h> instead.)
Quoted 3 lines
In general I would like to "go full iwyu" but have never heard it expressed that way. Can you recommend a tool available that will help me do that?
There is apt install iwyu, but I only tried that shortly and didn't manage to make it work nicely with-in the kernel.
Quoted 2 lines
It looks like "
mod_devicetable.h" doesn't help much unless I needstruct_cpu_feature(which I do not), so I'll include "of.h".
Yeah, and for struct cpu_feature there is
https://lore.kernel.org/all/ef103a4b9e1cd7477fa52ee347c4ec301354fba9.1789988540.git.u.kleine-koenig@baylibre.com/
.
Quoted 10 lines
Quoted 6 lines
drivers/clk/clk-tc9564.c line 351
Quoted 4 lines
drivers/clk/clk-tc9564.c new file +4
-0+static const struct of_device_id tc9564_clk_ids[] = { + { .compatible ="toshiba,tc9564-clock" }, + { }, +};Drop the trailing comma after the list terminator please.
OK. Only for the list terminator, right, because nothing will ever follow it? Non-empty final initializer is OK to have a comma as far as you're concerned?
correct. A non-empty last item even should have a comma.
Best regards
Uwe
Re: [PATCH 3/4] clk: toshiba: introduce a TC9564 SoC clock and reset driver
In reply to Alex ElderHi Alex,
Quoted 45 lines · Alex Elder
Define a new platform driver that manages clock and reset signals within the TC9564 SoC. There are 21 clocks, which can only be enabled and disabled, as well as 13 reset signals.
Two registers manage the state of the clocks and two others manage the state of the resets. The registers are accessed via a regmap supplied by a system controller, which coordinates access to a region of memory that will be shared with another driver.
Access to the memory region is provided via a BAR on a PCIe endpoint function embedded in the TC9564 SoC. For that reason, neither the PCIe clock nor PCIe reset can be manipulated by this driver (they are assumed always on and deasserted, respectively).
Similarly, control is not available for the I2C clock and reset, because the PCIe subsystem on the TC9564 relies on I2C
Co-developed-by Daniel Thompson <[email protected]>Signed-off-by Daniel Thompson <[email protected]>Signed-off-by Alex Elder <[email protected]>--- MAINTAINERS | 1 + drivers/clk/Kconfig | 11 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-tc9564.c | 366 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 379 insertions(+) create mode 100644 drivers/clk/clk-tc9564.cMAINTAINERS +1
-0@@ -27675,6 +27675,7 @@ M: Alex Elder <[email protected]> M: Daniel Thompson <[email protected]> S: Maintained F: Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml +F: drivers/clk/clk-tc9564.c F: include/dt-bindings/clock/toshiba,tc9564.h TOSHIBA TC9564 PCI DRIVERdrivers/clk/Kconfig +0
-0
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 297
Quoted 6 lines
drivers/clk/Kconfig +3
-0@@ -292,6 +292,17 @@ config COMMON_CLK_S2MPS11 clock. These multi-function devices have two (S2MPS14) or three (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each. +config COMMON_CLK_TC9564 + tristate "Toshiba TC9564 clock support" + depends on TC9564_PCI
select RESET_CONTROLLER
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 298
Quoted 1 line
drivers/clk/Kconfig +1
-0+ default m
default n
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 304
Quoted 6 lines
drivers/clk/Kconfig +6
-0+ help + This enables support for the clock and reset controller embedded + in the Toshiba TC9564 (and Qualcomm QPS615) SoC. The state of + clock and reset lines is controlled by MMIO to a region managed + by a system controller; this ensures access to the region is + coordinated between this and other drivers.
Rather than 'other drivers', outline specifically which other drivers.
Quoted 22 lines
drivers/clk/Kconfig +1
-0+ config CLK_TWL tristate "Clock driver for the TWL PMIC family" depends on TWL4030_COREdrivers/clk/Makefile +1
-0@@ -110,6 +110,7 @@ obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o obj-$(CONFIG_COMMON_CLK_SP7021) += clk-sp7021.o obj-$(CONFIG_COMMON_CLK_STM32F) += clk-stm32f4.o obj-$(CONFIG_COMMON_CLK_STM32H7) += clk-stm32h7.o +obj-$(CONFIG_COMMON_CLK_TC9564) += clk-tc9564.o obj-$(CONFIG_COMMON_CLK_TPS68470) += clk-tps68470.o obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o obj-$(CONFIG_CLK_TWL) += clk-twl.odrivers/clk/clk-tc9564.c new file +0
-0
drivers/clk/clk-tc9564.c line 10
Quoted 10 lines
drivers/clk/clk-tc9564.c new file +10
-0@@ -0,0 +1,366 @@ +// SPDX-License-Identifier: GPL-2.0-only + +/* +* Copyright (C) 2026 by RISCstar Solutions Corporation. All rights reserved. +*/ + +#include <linux/bits.h> +#include <linux/clk-provider.h> +#include <linux/mfd/syscon.h> +#include <linux/mod_devicetable.h>
Uwe already pointed out removing this.
Quoted 52 lines
drivers/clk/clk-tc9564.c new file +52
-0+#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/reset-controller.h> + +#include <dt-bindings/clock/toshiba,tc9564.h> + +#define CLK_CTRL0_OFFSET 0x1004 +#define RST_CTRL0_OFFSET 0x1008 +#define CLK_CTRL1_OFFSET 0x100c +#define RST_CTRL1_OFFSET 0x1010 + +struct tc9564_clock_init { +const char *name;/* NULL means unused entry */ +u32 offset; +u32 mask; +}; + +struct tc9564_clock { +struct clk_hw hw; +u32 which; +u32 offset;/* CLK_CTRL0_OFFSET or CLK_CTRL1_OFFSET */ +u32 mask;/* Zero means undefined clock */ +}; + +struct tc9564_reset { +u32 offset;/* RST_CTRL0_OFFSET or RST_CTRL1_OFFSET */ +u32 mask;/* Zero means undefined reset */ +}; + +struct tc9564_clocks { +struct device *dev; +struct regmap *regmap; +struct reset_controller_dev rcdev; +size_t clock_count; + struct tc9564_clock clocks[] __counted_by(clock_count); +}; + +#define TC9564_CLOCK_INIT0 (_name, _bit) __TC9564_CLOCK_INIT(_name, 0, _bit) +#define TC9564_CLOCK_INIT1 (_name, _bit) __TC9564_CLOCK_INIT(_name, 1, _bit) + +#define __TC9564_CLOCK_INIT (_name, _reg, _bit) \ + [CLOCK_##_name] = { \ + .name = #_name, \ + .offset = CLK_CTRL ## _reg ## _OFFSET, \ + .mask = BIT(_bit), \ + } + +#define TC9564_RESET_INIT0 (_name, _bit) __TC9564_RESET_INIT(_name, 0, _bit) +#define TC9564_RESET_INIT1 (_name, _bit) __TC9564_RESET_INIT(_name, 1, _bit) + +#define __TC9564_RESET_INIT (_name, _reg, _bit) \
drivers/clk/clk-tc9564.c line 72
Quoted 10 lines
drivers/clk/clk-tc9564.c new file +10
-0+ [RESET_##_name] = { \ + .offset = RST_CTRL ## _reg ## _OFFSET, \ + .mask = BIT(_bit), \ + } + +static const struct tc9564_clock_init tc9564_clock_init[] = { +TC9564_CLOCK_INIT0 (MCU ,0 ), +TC9564_CLOCK_INIT0 (INTC ,4 ), +/* TC9564_CLOCK_INIT0(PCIE, 9), */ +/* TC9564_CLOCK_INIT0(I2C, 12), */
A comment would be useful here to outline why these are commented out. Is the PCIE one comment out because of what's outlined in the commit message?
Quoted 203 lines
drivers/clk/clk-tc9564.c new file +203
-0+TC9564_CLOCK_INIT0 (SRAM ,13 ), +TC9564_CLOCK_INIT0 (UART ,16 ), +TC9564_CLOCK_INIT0 (MSIGEN ,18 ), +TC9564_CLOCK_INIT0 (PLL ,24 ), +TC9564_CLOCK_INIT0 (SGMII ,25 ), +TC9564_CLOCK_INIT0 (REFCLKO ,26 ), + +TC9564_CLOCK_INIT0 (MAC0_TX ,7 ), +TC9564_CLOCK_INIT0 (MAC0_RX ,14 ), +TC9564_CLOCK_INIT0 (MAC0_125M ,29 ), +TC9564_CLOCK_INIT0 (MAC0_312_5M ,30 ), +TC9564_CLOCK_INIT0 (MAC0_ALL ,31 ), + +TC9564_CLOCK_INIT1 (MAC1_TX ,7 ), +TC9564_CLOCK_INIT1 (MAC1_RX ,14 ), +TC9564_CLOCK_INIT1 (MAC1_RMII ,15 ), +TC9564_CLOCK_INIT1 (MAC1_125M ,29 ), +TC9564_CLOCK_INIT1 (MAC1_312_5M ,30 ), + TC9564_CLOCK_INIT1(MAC1_ALL, 31), +}; +#define TC9564_CLOCK_COUNT ARRAY_SIZE(tc9564_clock_init) + +static const struct tc9564_reset tc9564_reset[] = { +TC9564_RESET_INIT0 (MCU ,0 ), +TC9564_RESET_INIT0 (MCU1 ,1 ), +TC9564_RESET_INIT0 (INTC ,4 ), +/* TC9564_RESET_INIT0(PCIE, 9), */ +/* TC9564_RESET_INIT0(I2C, 12), */ +TC9564_RESET_INIT0 (UART ,16 ), +TC9564_RESET_INIT0 (MSIGEN ,18 ), + +TC9564_RESET_INIT0 (MAC0_MAC ,7 ), +TC9564_RESET_INIT0 (MAC0_PMA ,30 ), +TC9564_RESET_INIT0 (MAC0_XPCS ,31 ), + +TC9564_RESET_INIT1 (MAC1_MAC ,7 ), +TC9564_RESET_INIT1 (MAC1_PMA ,30 ), +TC9564_RESET_INIT1 (MAC1_XPCS ,31 ), +}; +#define TC9564_RESET_COUNT ARRAY_SIZE(tc9564_reset) + +static const struct tc9564_clock *hw_to_tc9564_clock (struct clk_hw *hw) +{ + return container_of_const(hw, struct tc9564_clock, hw); +} + +static const struct tc9564_clocks * +tc9564_clock_to_clocks (const struct tc9564_clock *clock) +{ +u32 which = clock->which; + +if (which >=TC9564_CLOCK_COUNT ) +return ERR_PTR (-ENXIO ); + + return container_of_const(clock, struct tc9564_clocks, clocks[which]); +} + +static int tc9564_clk_manage (struct clk_hw *hw,bool enable) +{ +const struct tc9564_clock *clock =hw_to_tc9564_clock (hw); +const struct tc9564_clocks *clocks; +u32 offset = clock->offset; +u32 mask = clock->mask; + + clocks =tc9564_clock_to_clocks (clock); +if (IS_ERR (clocks) || !mask) { +dev_err (clk_hw_get_dev (hw),"invalid clock (%s id %u)\n" , + enable ?"enable" :"disable" , clock->which); +return -ENXIO ; + } + +return regmap_update_bits (clocks->regmap, offset, mask, + enable ? mask :0 ); +} + +static int tc9564_clk_enable (struct clk_hw *hw) +{ +return tc9564_clk_manage (hw, true); +} + +static void tc9564_clk_disable (struct clk_hw *hw) +{ + (void )tc9564_clk_manage (hw, false); +} + +static const struct clk_ops tc9564_clk_ops = { + .enable = tc9564_clk_enable, + .disable = tc9564_clk_disable, +}; + +static void tc9564_clock_disable_all (struct tc9564_clocks *clocks) +{ +for (int i =0 ; i < clocks->clock_count; i++) { +const struct tc9564_clock *clock = &clocks->clocks[i]; + +if (clock->mask) +regmap_update_bits (clocks->regmap, clock->offset, + clock->mask,0 ); + } +} + +static struct clk_hw *tc9564_clk_hw_get (struct of_phandle_args *clkspec, +void *data) +{ +struct tc9564_clocks *clocks = data; +unsigned int i = clkspec->args[0 ]; + +if (i < clocks->clock_count) +return &clocks->clocks[i].hw; + +dev_err (clocks->dev,"invalid index %u\n" , i); + +return ERR_PTR (-EINVAL ); +} + +static struct tc9564_clocks *tc9564_clk_init (struct device *dev) +{ +struct tc9564_clocks *clocks; +size_t clocks_size; +int ret; + + clocks_size =struct_size (clocks, clocks,TC9564_CLOCK_COUNT ); + clocks =devm_kzalloc (dev, clocks_size,GFP_KERNEL ); +if (!clocks) +return ERR_PTR (-ENOMEM ); + + clocks->dev = dev; + clocks->clock_count =TC9564_CLOCK_COUNT ; + + clocks->regmap =syscon_node_to_regmap (dev_of_node (dev->parent)); +if (IS_ERR (clocks->regmap)) { +dev_err (dev,"failed to get config regmap\n" ); +return ERR_CAST (clocks->regmap); + } + +for (u32 i =0 ; i <TC9564_CLOCK_COUNT ; i++) { +const struct tc9564_clock_init *clock_init; +struct clk_init_data init = { }; +struct tc9564_clock *clock; + + clock_init = &tc9564_clock_init[i]; +if (!clock_init->name) +continue ; + + init.name = clock_init->name; + init.ops = &tc9564_clk_ops; + + clock = &clocks->clocks[i]; + clock->hw.init = &init; + + ret =devm_clk_hw_register (dev, &clock->hw); +if (ret) { +dev_err (dev,"failed to register clock \"%s\"\n" , + init.name); +return ERR_PTR (ret); + } + + clock->which = i; + clock->offset = clock_init->offset; + clock->mask = clock_init->mask; + } + + ret =devm_of_clk_add_hw_provider (dev, tc9564_clk_hw_get, clocks); +if (ret) { +dev_err (dev,"failed to add clock hardware provider\n" ); +return ERR_PTR (ret); + } + +return clocks; +} + +static const struct tc9564_clocks * +rcdev_to_tc9564_clocks (struct reset_controller_dev *rcdev) +{ + return container_of_const(rcdev, struct tc9564_clocks, rcdev); +} + +static int tc9564_reset_manage (struct reset_controller_dev *rcdev, +unsigned long id,bool assert) +{ +const struct tc9564_clocks *clocks =rcdev_to_tc9564_clocks (rcdev); + +if (id < rcdev->nr_resets) { +const struct tc9564_reset *reset = &tc9564_reset[id]; +u32 mask = reset->mask; + +if (mask) +return regmap_update_bits (clocks->regmap, + reset->offset, mask, + assert ? mask :0 ); + } + +dev_err (clocks->dev,"invalid reset (%sassert id %lu)\n" , + assert ?"" :"de" , id); + +return -ENXIO ; +} + +static int tc9564_reset_assert (struct reset_controller_dev *rcdev, +unsigned long id) +{ +return tc9564_reset_manage (rcdev, id, true); +}
drivers/clk/clk-tc9564.c line 285
Quoted 10 lines
drivers/clk/clk-tc9564.c new file +10
-0+ +static int tc9564_reset_deassert (struct reset_controller_dev *rcdev, +unsigned long id) +{ +return tc9564_reset_manage (rcdev, id, false); +} + +static const struct reset_control_ops tc9564_reset_control_ops = { + .assert = tc9564_reset_assert, + .deassert = tc9564_reset_deassert,
I'd rather not have the extra spaces because sometimes these get out of line with new members that need more space. Also tc9564_clk_ops above doesn't have the extra spaces.
Quoted 47 lines
drivers/clk/clk-tc9564.c new file +47
-0+}; + +static void tc9564_reset_assert_all (struct tc9564_clocks *clocks) +{ +for (u32 id =0 ; id <TC9564_RESET_COUNT ; id++) +if (tc9564_reset[id].mask) +tc9564_reset_manage (&clocks->rcdev, id, true); +} + +static int tc9564_reset_init (struct tc9564_clocks *clocks) +{ +struct reset_controller_dev *rcdev = &clocks->rcdev; + + rcdev->ops = &tc9564_reset_control_ops; + rcdev->owner =THIS_MODULE ; + rcdev->dev = clocks->dev; + rcdev->of_node =dev_of_node (clocks->dev); + rcdev->nr_resets =TC9564_RESET_COUNT ; + +return devm_reset_controller_register (clocks->dev, rcdev); +} + +static int tc9564_clk_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct tc9564_clocks *clocks; +int ret; + +if (!dev_of_node (dev)) +return dev_err_probe (dev, -EINVAL ,"no devicetree node\n" ); + + clocks =tc9564_clk_init (dev); +if (IS_ERR (clocks)) +return dev_err_probe (dev,PTR_ERR (clocks), +"failed to initialize clocks\n" ); + + ret =tc9564_reset_init (clocks); +if (ret) +return dev_err_probe (dev, ret,"failed to initialize resets\n" ); + +/* Force all resets to be initially asserted */ +tc9564_reset_assert_all (clocks); + +/* Force all clocks to be initially disabled */ +tc9564_clock_disable_all (clocks); + +platform_set_drvdata (pdev, clocks);
drivers/clk/clk-tc9564.c line 342
Quoted 10 lines
drivers/clk/clk-tc9564.c new file +10
-0+ +return 0 ; +} + +static void tc9564_clk_remove (struct platform_device *pdev) +{ +struct tc9564_clocks *clocks =platform_get_drvdata (pdev); + +/* Leave all resets to be deasserted when done */ +tc9564_reset_assert_all (clocks);
The comment says deasserted but the code calls assert_all.
Brian
Quoted 27 lines
drivers/clk/clk-tc9564.c new file +24
-0+ +/* Leave all clocks disabled when done */ +tc9564_clock_disable_all (clocks); +} + +static const struct of_device_id tc9564_clk_ids[] = { + { .compatible ="toshiba,tc9564-clock" }, + { }, +}; +MODULE_DEVICE_TABLE (of, tc9564_clk_ids); + +static struct platform_driver tc9564_clk_driver = { + .probe = tc9564_clk_probe, + .remove = tc9564_clk_remove, + .driver = { + .name =KBUILD_MODNAME , + .of_match_table = tc9564_clk_ids, + .probe_type =PROBE_PREFER_ASYNCHRONOUS , + }, +}; +module_platform_driver (tc9564_clk_driver); + +MODULE_DESCRIPTION ("Toshiba TC9564 Clock and Reset Driver" ); +MODULE_LICENSE ("GPL" );Signature · 1 line
2.53.0
Re: [PATCH 3/4] clk: toshiba: introduce a TC9564 SoC clock and reset driver
Quoted 51 lines · Brian Masney
Hi Alex,
Quoted 48 lines · Alex Elder
Define a new platform driver that manages clock and reset signals within the TC9564 SoC. There are 21 clocks, which can only be enabled and disabled, as well as 13 reset signals.
Two registers manage the state of the clocks and two others manage the state of the resets. The registers are accessed via a regmap supplied by a system controller, which coordinates access to a region of memory that will be shared with another driver.
Access to the memory region is provided via a BAR on a PCIe endpoint function embedded in the TC9564 SoC. For that reason, neither the PCIe clock nor PCIe reset can be manipulated by this driver (they are assumed always on and deasserted, respectively).
Similarly, control is not available for the I2C clock and reset, because the PCIe subsystem on the TC9564 relies on I2C
Co-developed-by Daniel Thompson <[email protected]>Signed-off-by Daniel Thompson <[email protected]>Signed-off-by Alex Elder <[email protected]>--- MAINTAINERS | 1 + drivers/clk/Kconfig | 11 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-tc9564.c | 366 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 379 insertions(+)create mode 100644
drivers/clk/clk-tc9564.cMAINTAINERS +1
-0@@ -27675,6 +27675,7 @@ M: Alex Elder <[email protected]> M: Daniel Thompson <[email protected]> S: Maintained F: Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml +F: drivers/clk/clk-tc9564.c F: include/dt-bindings/clock/toshiba,tc9564.h TOSHIBA TC9564 PCI DRIVERdrivers/clk/Kconfig +0
-0@@ -292,6 +292,17 @@ config COMMON_CLK_S2MPS11 clock. These multi-function devices have two (S2MPS14) or three (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each.
Quoted 5 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 297
Quoted 3 lines
drivers/clk/Kconfig +3
-0+config COMMON_CLK_TC9564 + tristate "Toshiba TC9564 clock support" + depends on TC9564_PCIselect
RESET_CONTROLLER
Thank you. The reset and clock drivers were previously separate and the reset only became available if RESET_CONTROLLER was enabled. Combining them means I need this. I will add it.
I also need to select MFD_SYSCON to get syscon_node_to_regmap() (or perhaps something similar, depending on the answers to my questions about the proper way to define the syscon).
Quoted 4 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 298
Quoted 2 lines
drivers/clk/Kconfig +1
-0+ default mdefault n
This depends on TC9564_PCI, and if TC9564_PCI is enabled I would like this to (automatically) be available (at least as a module).
Why do you recommend "n"?
Quoted 8 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 304
Quoted 6 lines
drivers/clk/Kconfig +6
-0+ help + This enables support for the clock and reset controller embedded + in the Toshiba TC9564 (and Qualcomm QPS615) SoC. The state of + clock and reset lines is controlled by MMIO to a region managed + by a system controller; this ensures access to the region is + coordinated between this and other drivers.Rather than 'other drivers', outline specifically which other drivers.
I was intentionally vague at this point because the one other driver has not yet gone out for upstream review for this iteration of the code. But I do agree with you, so I'll change this to say:
... between this and the XGMAC (stmmac) driver.
Then it will be correct without modification once that driver goes out for review. Is that better, or do you suggest something else?
Quoted 26 lines
Quoted 26 lines
drivers/clk/Kconfig +1
-0+ config CLK_TWL tristate "Clock driver for the TWL PMIC family" depends on TWL4030_COREdrivers/clk/Makefile +1
-0@@ -110,6 +110,7 @@ obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o obj-$(CONFIG_COMMON_CLK_SP7021) += clk-sp7021.o obj-$(CONFIG_COMMON_CLK_STM32F) += clk-stm32f4.o obj-$(CONFIG_COMMON_CLK_STM32H7) += clk-stm32h7.o +obj-$(CONFIG_COMMON_CLK_TC9564) += clk-tc9564.o obj-$(CONFIG_COMMON_CLK_TPS68470) += clk-tps68470.o obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o obj-$(CONFIG_CLK_TWL) += clk-twl.odrivers/clk/clk-tc9564.c new file +4
-0@@ -0,0 +1,366 @@ +// SPDX-License-Identifier: GPL-2.0-only + +/* +* Copyright (C) 2026 by RISCstar Solutions Corporation. All rights reserved.
Quoted 8 lines
drivers/clk/clk-tc9564.c line 10
Quoted 6 lines
drivers/clk/clk-tc9564.c new file +6
-0+*/ + +#include <linux/bits.h> +#include <linux/clk-provider.h> +#include <linux/mfd/syscon.h> +#include <linux/mod_devicetable.h> Uwe already pointed out removing this.
Yes, done.
Quoted 59 lines
Quoted 59 lines
drivers/clk/clk-tc9564.c new file +58
-0+#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/reset-controller.h> + +#include <dt-bindings/clock/toshiba,tc9564.h> + +#define CLK_CTRL0_OFFSET 0x1004 +#define RST_CTRL0_OFFSET 0x1008 +#define CLK_CTRL1_OFFSET 0x100c +#define RST_CTRL1_OFFSET 0x1010 + +struct tc9564_clock_init { +const char *name;/* NULL means unused entry */ +u32 offset; +u32 mask; +}; + +struct tc9564_clock { +struct clk_hw hw; +u32 which; +u32 offset;/* CLK_CTRL0_OFFSET or CLK_CTRL1_OFFSET */ +u32 mask;/* Zero means undefined clock */ +}; + +struct tc9564_reset { +u32 offset;/* RST_CTRL0_OFFSET or RST_CTRL1_OFFSET */ +u32 mask;/* Zero means undefined reset */ +}; + +struct tc9564_clocks { +struct device *dev; +struct regmap *regmap; +struct reset_controller_dev rcdev; +size_t clock_count; + struct tc9564_clock clocks[] __counted_by(clock_count); +}; + +#define TC9564_CLOCK_INIT0 (_name, _bit) __TC9564_CLOCK_INIT(_name, 0, _bit) +#define TC9564_CLOCK_INIT1 (_name, _bit) __TC9564_CLOCK_INIT(_name, 1, _bit) + +#define __TC9564_CLOCK_INIT (_name, _reg, _bit) \ + [CLOCK_##_name] = { \ + .name = #_name, \ + .offset = CLK_CTRL ## _reg ## _OFFSET, \ + .mask = BIT(_bit), \ + } + +#define TC9564_RESET_INIT0 (_name, _bit) __TC9564_RESET_INIT(_name, 0, _bit) +#define TC9564_RESET_INIT1 (_name, _bit) __TC9564_RESET_INIT(_name, 1, _bit) + +#define __TC9564_RESET_INIT (_name, _reg, _bit) \ + [RESET_##_name] = { \ + .offset = RST_CTRL ## _reg ## _OFFSET, \ + .mask = BIT(_bit), \ + } + +static const struct tc9564_clock_init tc9564_clock_init[] = {
Quoted 8 lines
drivers/clk/clk-tc9564.c line 72
Quoted 4 lines
drivers/clk/clk-tc9564.c new file +4
-0+TC9564_CLOCK_INIT0 (MCU ,0 ), + TC9564_CLOCK_INIT0(INTC, 4), +/* TC9564_CLOCK_INIT0(PCIE, 9), */ +/* TC9564_CLOCK_INIT0(I2C, 12), */ A comment would be useful here to outline why these are commented out. Is the PCIE one comment out because of what's outlined in the commit message?
Yes, that is why. We are downstream of PCIe, and PCIe in this case depends on I2C. So we don't want to mess with these two clocks or their reset counterparts.
I only provide this for the benefit of documentation. If someone happens to see bit 9 set in this register, it means the PCIe clock is enabled.
Anyway, others have suggested simply removing these comments. So I'll do what you suggest, but I'm interested to know whether you would favor just deleting them instead.
Quoted 7 lines
drivers/clk/clk-tc9564.c line 78
Quoted 7 lines
drivers/clk/clk-tc9564.c new file +6
-0+TC9564_CLOCK_INIT0 (SRAM ,13 ), +TC9564_CLOCK_INIT0 (UART ,16 ), +TC9564_CLOCK_INIT0 (MSIGEN ,18 ), +TC9564_CLOCK_INIT0 (PLL ,24 ), +TC9564_CLOCK_INIT0 (SGMII ,25 ), + TC9564_CLOCK_INIT0(REFCLKO, 26),
. . .
Quoted 7 lines
drivers/clk/clk-tc9564.c line 285
Quoted 3 lines
drivers/clk/clk-tc9564.c new file +3
-0+static const struct reset_control_ops tc9564_reset_control_ops = { + .assert = tc9564_reset_assert, + .deassert = tc9564_reset_deassert,I'd rather not have the extra spaces because sometimes these get out of line with new members that need more space. Also
tc9564_clk_opsabove doesn't have the extra spaces.
That's OK with me. I normally align assignments like this for readability, but it's obviously an arbitrary stylistic choice. I'll just use one space, no alignment (just as the clk_ops do above). I'll do the same in the platform driver structure assignments.
Quoted 52 lines
Quoted 52 lines
drivers/clk/clk-tc9564.c new file +51
-0+}; + +static void tc9564_reset_assert_all (struct tc9564_clocks *clocks) +{ +for (u32 id =0 ; id <TC9564_RESET_COUNT ; id++) +if (tc9564_reset[id].mask) +tc9564_reset_manage (&clocks->rcdev, id, true); +} + +static int tc9564_reset_init (struct tc9564_clocks *clocks) +{ +struct reset_controller_dev *rcdev = &clocks->rcdev; + + rcdev->ops = &tc9564_reset_control_ops; + rcdev->owner =THIS_MODULE ; + rcdev->dev = clocks->dev; + rcdev->of_node =dev_of_node (clocks->dev); + rcdev->nr_resets =TC9564_RESET_COUNT ; + +return devm_reset_controller_register (clocks->dev, rcdev); +} + +static int tc9564_clk_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct tc9564_clocks *clocks; +int ret; + +if (!dev_of_node (dev)) +return dev_err_probe (dev, -EINVAL ,"no devicetree node\n" ); + + clocks =tc9564_clk_init (dev); +if (IS_ERR (clocks)) +return dev_err_probe (dev,PTR_ERR (clocks), +"failed to initialize clocks\n" ); + + ret =tc9564_reset_init (clocks); +if (ret) +return dev_err_probe (dev, ret,"failed to initialize resets\n" ); + +/* Force all resets to be initially asserted */ +tc9564_reset_assert_all (clocks); + +/* Force all clocks to be initially disabled */ +tc9564_clock_disable_all (clocks); + +platform_set_drvdata (pdev, clocks); + +return 0 ; +} +
Quoted 8 lines
drivers/clk/clk-tc9564.c line 342
Quoted 6 lines
drivers/clk/clk-tc9564.c new file +6
-0+static void tc9564_clk_remove (struct platform_device *pdev) +{ +struct tc9564_clocks *clocks =platform_get_drvdata (pdev); + +/* Leave all resets to be deasserted when done */ +tc9564_reset_assert_all (clocks);The comment says deasserted but the code calls
assert_all.
Oops. Will fix. Thank you very much for your review Brian.
-Alex
Quoted 32 lines
Brian
Quoted 27 lines
drivers/clk/clk-tc9564.c new file +24
-0+ +/* Leave all clocks disabled when done */ +tc9564_clock_disable_all (clocks); +} + +static const struct of_device_id tc9564_clk_ids[] = { + { .compatible ="toshiba,tc9564-clock" }, + { }, +}; +MODULE_DEVICE_TABLE (of, tc9564_clk_ids); + +static struct platform_driver tc9564_clk_driver = { + .probe = tc9564_clk_probe, + .remove = tc9564_clk_remove, + .driver = { + .name =KBUILD_MODNAME , + .of_match_table = tc9564_clk_ids, + .probe_type =PROBE_PREFER_ASYNCHRONOUS , + }, +}; +module_platform_driver (tc9564_clk_driver); + +MODULE_DESCRIPTION ("Toshiba TC9564 Clock and Reset Driver" ); +MODULE_LICENSE ("GPL" );Signature · 1 line
2.53.0
Re: [PATCH 3/4] clk: toshiba: introduce a TC9564 SoC clock and reset driver
Quoted 58 lines · Alex Elder
Quoted 51 lines · Brian Masney
Hi Alex,
Quoted 48 lines · Alex Elder
Define a new platform driver that manages clock and reset signals within the TC9564 SoC. There are 21 clocks, which can only be enabled and disabled, as well as 13 reset signals.
Two registers manage the state of the clocks and two others manage the state of the resets. The registers are accessed via a regmap supplied by a system controller, which coordinates access to a region of memory that will be shared with another driver.
Access to the memory region is provided via a BAR on a PCIe endpoint function embedded in the TC9564 SoC. For that reason, neither the PCIe clock nor PCIe reset can be manipulated by this driver (they are assumed always on and deasserted, respectively).
Similarly, control is not available for the I2C clock and reset, because the PCIe subsystem on the TC9564 relies on I2C
Co-developed-by Daniel Thompson <[email protected]>Signed-off-by Daniel Thompson <[email protected]>Signed-off-by Alex Elder <[email protected]>--- MAINTAINERS | 1 + drivers/clk/Kconfig | 11 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-tc9564.c | 366 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 379 insertions(+)create mode 100644
drivers/clk/clk-tc9564.cMAINTAINERS +1
-0@@ -27675,6 +27675,7 @@ M: Alex Elder <[email protected]> M: Daniel Thompson <[email protected]> S: Maintained F: Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml +F: drivers/clk/clk-tc9564.c F: include/dt-bindings/clock/toshiba,tc9564.h TOSHIBA TC9564 PCI DRIVERdrivers/clk/Kconfig +0
-0@@ -292,6 +292,17 @@ config COMMON_CLK_S2MPS11 clock. These multi-function devices have two (S2MPS14) or three (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each.Quoted 5 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 297
Quoted 3 lines
drivers/clk/Kconfig +3
-0+config COMMON_CLK_TC9564 + tristate "Toshiba TC9564 clock support" + depends on TC9564_PCIselect
RESET_CONTROLLER
Quoted 3 lines
Thank you. The reset and clock drivers were previously separate and the reset only became available if
RESET_CONTROLLERwas enabled. Combining them means I need this. I will add it.
Why did you combine them ?
it would be a lot better if the reset were handled in drivers/reset rather than in clock. There has already been some work to move reset from clock back to reset. This often involve auxiliary drivers.
Quoted 269 lines
I also need to select
MFD_SYSCONto getsyscon_node_to_regmap()(or perhaps something similar, depending on the answers to my questions about the proper way to define the syscon).Quoted 4 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 298
Quoted 2 lines
drivers/clk/Kconfig +1
-0+ default mdefault n
This depends on
TC9564_PCI, and ifTC9564_PCIis enabled I would like this to (automatically) be available (at least as a module).Why do you recommend "n"?
Quoted 8 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 304
Quoted 6 lines
drivers/clk/Kconfig +6
-0+ help + This enables support for the clock and reset controller embedded + in the Toshiba TC9564 (and Qualcomm QPS615) SoC. The state of + clock and reset lines is controlled by MMIO to a region managed + by a system controller; this ensures access to the region is + coordinated between this and other drivers.Rather than 'other drivers', outline specifically which other drivers.
I was intentionally vague at this point because the one other driver has not yet gone out for upstream review for this iteration of the code. But I do agree with you, so I'll change this to say:
... between this and the XGMAC (stmmac) driver.
Then it will be correct without modification once that driver goes out for review. Is that better, or do you suggest something else?
Quoted 26 lines
Quoted 26 lines
drivers/clk/Kconfig +1
-0+ config CLK_TWL tristate "Clock driver for the TWL PMIC family" depends on TWL4030_COREdrivers/clk/Makefile +1
-0@@ -110,6 +110,7 @@ obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o obj-$(CONFIG_COMMON_CLK_SP7021) += clk-sp7021.o obj-$(CONFIG_COMMON_CLK_STM32F) += clk-stm32f4.o obj-$(CONFIG_COMMON_CLK_STM32H7) += clk-stm32h7.o +obj-$(CONFIG_COMMON_CLK_TC9564) += clk-tc9564.o obj-$(CONFIG_COMMON_CLK_TPS68470) += clk-tps68470.o obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o obj-$(CONFIG_CLK_TWL) += clk-twl.odrivers/clk/clk-tc9564.c new file +4
-0@@ -0,0 +1,366 @@ +// SPDX-License-Identifier: GPL-2.0-only + +/* +* Copyright (C) 2026 by RISCstar Solutions Corporation. All rights reserved. Quoted 8 lines
drivers/clk/clk-tc9564.c line 10
Quoted 6 lines
drivers/clk/clk-tc9564.c new file +6
-0+*/ + +#include <linux/bits.h> +#include <linux/clk-provider.h> +#include <linux/mfd/syscon.h> +#include <linux/mod_devicetable.h> Uwe already pointed out removing this.
Yes, done.
Quoted 59 lines
Quoted 59 lines
drivers/clk/clk-tc9564.c new file +58
-0+#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/reset-controller.h> + +#include <dt-bindings/clock/toshiba,tc9564.h> + +#define CLK_CTRL0_OFFSET 0x1004 +#define RST_CTRL0_OFFSET 0x1008 +#define CLK_CTRL1_OFFSET 0x100c +#define RST_CTRL1_OFFSET 0x1010 + +struct tc9564_clock_init { +const char *name;/* NULL means unused entry */ +u32 offset; +u32 mask; +}; + +struct tc9564_clock { +struct clk_hw hw; +u32 which; +u32 offset;/* CLK_CTRL0_OFFSET or CLK_CTRL1_OFFSET */ +u32 mask;/* Zero means undefined clock */ +}; + +struct tc9564_reset { +u32 offset;/* RST_CTRL0_OFFSET or RST_CTRL1_OFFSET */ +u32 mask;/* Zero means undefined reset */ +}; + +struct tc9564_clocks { +struct device *dev; +struct regmap *regmap; +struct reset_controller_dev rcdev; +size_t clock_count; + struct tc9564_clock clocks[] __counted_by(clock_count); +}; + +#define TC9564_CLOCK_INIT0 (_name, _bit) __TC9564_CLOCK_INIT(_name, 0, _bit) +#define TC9564_CLOCK_INIT1 (_name, _bit) __TC9564_CLOCK_INIT(_name, 1, _bit) + +#define __TC9564_CLOCK_INIT (_name, _reg, _bit) \ + [CLOCK_##_name] = { \ + .name = #_name, \ + .offset = CLK_CTRL ## _reg ## _OFFSET, \ + .mask = BIT(_bit), \ + } + +#define TC9564_RESET_INIT0 (_name, _bit) __TC9564_RESET_INIT(_name, 0, _bit) +#define TC9564_RESET_INIT1 (_name, _bit) __TC9564_RESET_INIT(_name, 1, _bit) + +#define __TC9564_RESET_INIT (_name, _reg, _bit) \ + [RESET_##_name] = { \ + .offset = RST_CTRL ## _reg ## _OFFSET, \ + .mask = BIT(_bit), \ + } + +static const struct tc9564_clock_init tc9564_clock_init[] = {Quoted 8 lines
drivers/clk/clk-tc9564.c line 72
Quoted 4 lines
drivers/clk/clk-tc9564.c new file +4
-0+TC9564_CLOCK_INIT0 (MCU ,0 ), + TC9564_CLOCK_INIT0(INTC, 4), +/* TC9564_CLOCK_INIT0(PCIE, 9), */ +/* TC9564_CLOCK_INIT0(I2C, 12), */ A comment would be useful here to outline why these are commented out. Is the PCIE one comment out because of what's outlined in the commit message?
Yes, that is why. We are downstream of PCIe, and PCIe in this case depends on I2C. So we don't want to mess with these two clocks or their reset counterparts.
I only provide this for the benefit of documentation. If someone happens to see bit 9 set in this register, it means the PCIe clock is enabled.
Anyway, others have suggested simply removing these comments. So I'll do what you suggest, but I'm interested to know whether you would favor just deleting them instead.
Quoted 7 lines
drivers/clk/clk-tc9564.c line 78
Quoted 7 lines
drivers/clk/clk-tc9564.c new file +6
-0+TC9564_CLOCK_INIT0 (SRAM ,13 ), +TC9564_CLOCK_INIT0 (UART ,16 ), +TC9564_CLOCK_INIT0 (MSIGEN ,18 ), +TC9564_CLOCK_INIT0 (PLL ,24 ), +TC9564_CLOCK_INIT0 (SGMII ,25 ), + TC9564_CLOCK_INIT0(REFCLKO, 26),. . .
Quoted 7 lines
drivers/clk/clk-tc9564.c line 285
Quoted 3 lines
drivers/clk/clk-tc9564.c new file +3
-0+static const struct reset_control_ops tc9564_reset_control_ops = { + .assert = tc9564_reset_assert, + .deassert = tc9564_reset_deassert,I'd rather not have the extra spaces because sometimes these get out of line with new members that need more space. Also
tc9564_clk_opsabove doesn't have the extra spaces.That's OK with me. I normally align assignments like this for readability, but it's obviously an arbitrary stylistic choice. I'll just use one space, no alignment (just as the
clk_opsdo above). I'll do the same in the platform driver structure assignments.Quoted 52 lines
Quoted 52 lines
drivers/clk/clk-tc9564.c new file +51
-0+}; + +static void tc9564_reset_assert_all (struct tc9564_clocks *clocks) +{ +for (u32 id =0 ; id <TC9564_RESET_COUNT ; id++) +if (tc9564_reset[id].mask) +tc9564_reset_manage (&clocks->rcdev, id, true); +} + +static int tc9564_reset_init (struct tc9564_clocks *clocks) +{ +struct reset_controller_dev *rcdev = &clocks->rcdev; + + rcdev->ops = &tc9564_reset_control_ops; + rcdev->owner =THIS_MODULE ; + rcdev->dev = clocks->dev; + rcdev->of_node =dev_of_node (clocks->dev); + rcdev->nr_resets =TC9564_RESET_COUNT ; + +return devm_reset_controller_register (clocks->dev, rcdev); +} + +static int tc9564_clk_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct tc9564_clocks *clocks; +int ret; + +if (!dev_of_node (dev)) +return dev_err_probe (dev, -EINVAL ,"no devicetree node\n" ); + + clocks =tc9564_clk_init (dev); +if (IS_ERR (clocks)) +return dev_err_probe (dev,PTR_ERR (clocks), +"failed to initialize clocks\n" ); + + ret =tc9564_reset_init (clocks); +if (ret) +return dev_err_probe (dev, ret,"failed to initialize resets\n" ); + +/* Force all resets to be initially asserted */ +tc9564_reset_assert_all (clocks); + +/* Force all clocks to be initially disabled */ +tc9564_clock_disable_all (clocks); + +platform_set_drvdata (pdev, clocks); + +return 0 ; +} +Quoted 8 lines
drivers/clk/clk-tc9564.c line 342
Quoted 6 lines
drivers/clk/clk-tc9564.c new file +6
-0+static void tc9564_clk_remove (struct platform_device *pdev) +{ +struct tc9564_clocks *clocks =platform_get_drvdata (pdev); + +/* Leave all resets to be deasserted when done */ +tc9564_reset_assert_all (clocks);The comment says deasserted but the code calls
assert_all.Oops. Will fix. Thank you very much for your review Brian.
-Alex
Quoted 32 lines
Brian
Quoted 27 lines
drivers/clk/clk-tc9564.c new file +24
-0+ +/* Leave all clocks disabled when done */ +tc9564_clock_disable_all (clocks); +} + +static const struct of_device_id tc9564_clk_ids[] = { + { .compatible ="toshiba,tc9564-clock" }, + { }, +}; +MODULE_DEVICE_TABLE (of, tc9564_clk_ids); + +static struct platform_driver tc9564_clk_driver = { + .probe = tc9564_clk_probe, + .remove = tc9564_clk_remove, + .driver = { + .name =KBUILD_MODNAME , + .of_match_table = tc9564_clk_ids, + .probe_type =PROBE_PREFER_ASYNCHRONOUS , + }, +}; +module_platform_driver (tc9564_clk_driver); + +MODULE_DESCRIPTION ("Toshiba TC9564 Clock and Reset Driver" ); +MODULE_LICENSE ("GPL" );Signature · 1 line
2.53.0
Signature · 1 line
Jerome
Re: [PATCH 3/4] clk: toshiba: introduce a TC9564 SoC clock and reset driver
Quoted 12 lines · Jerome Brunet
Quoted 12 lines
Quoted 12 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 297
Quoted 11 lines
drivers/clk/Kconfig +3
-0@@ -292,6 +292,17 @@ config COMMON_CLK_S2MPS11 clock. These multi-function devices have two (S2MPS14) or three (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each. +config COMMON_CLK_TC9564 + tristate "Toshiba TC9564 clock support" + depends on TC9564_PCIselect
RESET_CONTROLLER
Quoted 8 lines
Quoted 3 lines
Thank you. The reset and clock drivers were previously separate and the reset only became available if
RESET_CONTROLLERwas enabled. Combining them means I need this. I will add it.Why did you combine them ?
it would be a lot better if the reset were handled in
drivers/resetrather than in clock. There has already been some work to move reset from clock back to reset. This often involve auxiliary drivers.
To be honest, I got a suggestion from someone to combine them, but I said "no, I got feedback last year that separating them was better." But then a second person told me that these are often combined, so I took that as "two against one" and decided to merge them.
The other reason is that four registers are used:
#define CLK_CTRL0_OFFSET 0x1004
#define RST_CTRL0_OFFSET 0x1008
#define CLK_CTRL1_OFFSET 0x100c
define RST_CTRL1_OFFSET 0x1010
They alternate between clock and reset. The previous separated drivers specified the two offsets for each driver in the reg property. But somehow these interleaved registers seemed a little off, so I thought the combined driver cleaned that up a little.
I don't have any strong preference one way or another. If you think they're better separate, I don't mind separating them again. The two drivers are extremely simple.
-Alex
Re: [PATCH 3/4] clk: toshiba: introduce a TC9564 SoC clock and reset driver
Quoted 39 lines · Alex Elder
Quoted 12 lines · Jerome Brunet
Quoted 12 lines
Quoted 12 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 297
Quoted 11 lines
drivers/clk/Kconfig +3
-0@@ -292,6 +292,17 @@ config COMMON_CLK_S2MPS11 clock. These multi-function devices have two (S2MPS14) or three (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each. +config COMMON_CLK_TC9564 + tristate "Toshiba TC9564 clock support" + depends on TC9564_PCIselect
RESET_CONTROLLERQuoted 8 lines
Quoted 3 lines
Thank you. The reset and clock drivers were previously separate and the reset only became available if
RESET_CONTROLLERwas enabled. Combining them means I need this. I will add it.Why did you combine them ?
it would be a lot better if the reset were handled in
drivers/resetrather than in clock. There has already been some work to move reset from clock back to reset. This often involve auxiliary drivers.To be honest, I got a suggestion from someone to combine them, but I said "no, I got feedback last year that separating them was better." But then a second person told me that these are often combined, so I took that as "two against one" and decided to merge them.
The other reason is that four registers are used:
#define CLK_CTRL0_OFFSET 0x1004 #define RST_CTRL0_OFFSET 0x1008 #define CLK_CTRL1_OFFSET 0x100c define RST_CTRL1_OFFSET 0x1010They alternate between clock and reset. The previous separated drivers specified the two offsets for each driver in the reg property. But somehow these interleaved registers seemed a little off, so I thought the combined driver cleaned that up a little.
Quoted 3 lines
I don't have any strong preference one way or another. If you think they're better separate, I don't mind separating them again. The two drivers are extremely simple.
Yes, SoC manufacturer tend to mix the 2, it is fairly usual and we used to have many drivers in clock providing reset too. We have been slowly cleaning that up over the past few years.
To be clear it is not strictly forbbiden to mix ressources but putting things where they belong help review and maintainance.
There are several examples you can look at with auxiliary device support, ./tenstorrent/atlantis-prcm.c is a good example.
Quoted 2 lines
-Alex
Signature · 1 line
Jerome
Re: [PATCH 3/4] clk: toshiba: introduce a TC9564 SoC clock and reset driver
In reply to Alex ElderHi Alex,
Quoted 62 lines · Alex Elder
Quoted 44 lines · Brian Masney
Quoted 43 lines · Alex Elder
Define a new platform driver that manages clock and reset signals within the TC9564 SoC. There are 21 clocks, which can only be enabled and disabled, as well as 13 reset signals.
Two registers manage the state of the clocks and two others manage the state of the resets. The registers are accessed via a regmap supplied by a system controller, which coordinates access to a region of memory that will be shared with another driver.
Access to the memory region is provided via a BAR on a PCIe endpoint function embedded in the TC9564 SoC. For that reason, neither the PCIe clock nor PCIe reset can be manipulated by this driver (they are assumed always on and deasserted, respectively).
Similarly, control is not available for the I2C clock and reset, because the PCIe subsystem on the TC9564 relies on I2C
Co-developed-by Daniel Thompson <[email protected]>Signed-off-by Daniel Thompson <[email protected]>Signed-off-by Alex Elder <[email protected]>--- MAINTAINERS | 1 + drivers/clk/Kconfig | 11 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-tc9564.c | 366 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 379 insertions(+)create mode 100644
drivers/clk/clk-tc9564.cMAINTAINERS +1
-0@@ -27675,6 +27675,7 @@ M: Alex Elder <[email protected]> M: Daniel Thompson <[email protected]> S: Maintained F: Documentation/devicetree/bindings/clock/toshiba,tc9564-clock.yaml +F: drivers/clk/clk-tc9564.c F: include/dt-bindings/clock/toshiba,tc9564.h TOSHIBA TC9564 PCI DRIVERdrivers/clk/Kconfig +0
-0Quoted 8 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 296
Quoted 6 lines
drivers/clk/Kconfig +3
-0@@ -292,6 +292,17 @@ config COMMON_CLK_S2MPS11 clock. These multi-function devices have two (S2MPS14) or three (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each. +config COMMON_CLK_TC9564 + tristate "Toshiba TC9564 clock support" + depends on TC9564_PCIselect
RESET_CONTROLLERThank you. The reset and clock drivers were previously separate and the reset only became available if
RESET_CONTROLLERwas enabled. Combining them means I need this. I will add it.I also need to select
MFD_SYSCONto getsyscon_node_to_regmap()(or perhaps something similar, depending on the answers to my questions about the proper way to define the syscon).Quoted 1 line
Quoted 8 lines
Quoted 3 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 298
Quoted 1 line
drivers/clk/Kconfig +1
-0+ default mdefault n
This depends on
TC9564_PCI, and ifTC9564_PCIis enabled I would like this to (automatically) be available (at least as a module).Why do you recommend "n"?
There is no existing 'default m' or 'default y' in the toplevel clk Kconfig. How about this instead?
depends on TC9564_PCI || COMPILE_TEST
default TC9564_PCIQuoted 9 lines
Quoted 8 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 304
Quoted 6 lines
drivers/clk/Kconfig +6
-0+ help + This enables support for the clock and reset controller embedded + in the Toshiba TC9564 (and Qualcomm QPS615) SoC. The state of + clock and reset lines is controlled by MMIO to a region managed + by a system controller; this ensures access to the region is + coordinated between this and other drivers.Rather than 'other drivers', outline specifically which other drivers.
Quoted 8 lines
I was intentionally vague at this point because the one other driver has not yet gone out for upstream review for this iteration of the code. But I do agree with you, so I'll change this to say:
... between this and the XGMAC (stmmac) driver.
Then it will be correct without modification once that driver goes out for review. Is that better, or do you suggest something else?
Yes that sounds good.
Quoted 14 lines
Quoted 9 lines
drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 308
Quoted 5 lines
drivers/clk/Kconfig +5
-0+static const struct tc9564_clock_init tc9564_clock_init[] = { + TC9564_CLOCK_INIT0(MCU, 0), + TC9564_CLOCK_INIT0(INTC, 4), + /* TC9564_CLOCK_INIT0(PCIE, 9), */ + /* TC9564_CLOCK_INIT0(I2C, 12), */A comment would be useful here to outline why these are commented out. Is the PCIE one comment out because of what's outlined in the commit message?
Yes, that is why. We are downstream of PCIe, and PCIe in this case depends on I2C. So we don't want to mess with these two clocks or their reset counterparts.
Quoted 7 lines
I only provide this for the benefit of documentation. If someone happens to see bit 9 set in this register, it means the PCIe clock is enabled.
Anyway, others have suggested simply removing these comments. So I'll do what you suggest, but I'm interested to know whether you would favor just deleting them instead.
I also think to delete these comments. Would it make sense to include the comment in just include/dt-bindings/clock/toshiba,tc9564.h ?
Brian
PATCH 4/4 arm64: dts: qcom: qcs6490-rb3gen2: add the clock controller
In reply to Alex ElderThe clock (and reset) controller within the TC9564 SoC is accessed via a PCI endpoint bus through BAR 4 of the first embedded PCIe function. A syscon node is created to provide access to a range of memory that is used by the clock/reset controller and shared by other devices.
--- arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 22 +++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts +21 -1
@@ -9,6 +9,7 @@ #define PM7250B_SID 8 #define PM7250B_SID1 9 +#include <dt-bindings/clock/toshiba,tc9564.h> #include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h> #include <dt-bindings/iio/qcom,spmi-adc7-pm7325.h> #include <dt-bindings/leds/common.h>
@@ -983,7 +984,7 @@ dev@0,1 { <0x2 0x0 0x0 0x83050100 0x0 0x4000 0x0 0x80000>, <0x4 0x0 0x0 0x83050100 0x0 0x84000 0x0 0x200000>; - pci-ep-bus@4 { + pcie_s1b5d0f0_bar4_bus: pci-ep-bus@4 { compatible = "simple-bus"; #address-cells = <1>; #size-cells = <1>;
@@ -995,6 +996,25 @@ pci-ep-bus@4 { }; }; +&pcie_s1b5d0f0_bar4_bus { + syscon@0 { + compatible = "toshiba,tc9564-config", + "syscon", + "simple-mfd"; + reg = <0x0 0x2000>; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + tc9564_clock0: clock@1004 { + compatible = "toshiba,tc9564-clock"; + reg = <0x1004 0x10>; + #clock-cells = <1>; + #reset-cells = <1>; + }; + }; +}; + &pm7325_gpios { kypd_vol_up_n: kypd-vol-up-n-state { pins = "gpio6";
Signature · 1 line
2.53.0