From: Nick Hawkins <[email protected]>
The HPE SoC is new to linux. This patch
creates the basic device tree layout with minimum required
for linux to boot. This includes timer and watchdog
support.
Signed-off-by: Nick Hawkins <[email protected]>
---
arch/arm/boot/dts/Makefile | 2 +
arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 27 +++++
arch/arm/boot/dts/hpe-gxp.dtsi | 148 +++++++++++++++++++++++
3 files changed, 177 insertions(+)
create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index e41eca79c950..2823b359d373 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
aspeed-bmc-vegman-n110.dtb \
aspeed-bmc-vegman-rx20.dtb \
aspeed-bmc-vegman-sx20.dtb
+dtb-$(CONFIG_ARCH_HPE_GXP) += \
+ hpe-bmc-dl360gen10.dtb
diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
new file mode 100644
index 000000000000..da5eac1213a8
--- /dev/null
+++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree file for HPE DL360Gen10
+ */
+
+/include/ "hpe-gxp.dtsi"
+
+/ {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "hpe,gxp";
+ model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
+
+ chosen {
+ bootargs = "earlyprintk console=ttyS2,115200";
+ };
+
+ memory@40000000 {
+ device_type = "memory";
+ reg = <0x40000000 0x20000000>;
+ };
+
+ ahb {
+
+ };
+
+};
diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
new file mode 100644
index 000000000000..dfaf8df829fe
--- /dev/null
+++ b/arch/arm/boot/dts/hpe-gxp.dtsi
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree file for HPE GXP
+ */
+
+/dts-v1/;
+/ {
+ model = "Hewlett Packard Enterprise GXP BMC";
+ compatible = "hpe,gxp";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ cpus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ cpu@0 {
+ compatible = "arm,cortex-a9";
+ device_type = "cpu";
+ reg = <0>;
+ };
+ };
+
+ gxp-init@cefe0010 {
+ compatible = "hpe,gxp-cpu-init";
+ reg = <0xcefe0010 0x04>;
+ };
+
+ memory@40000000 {
+ device_type = "memory";
+ reg = <0x40000000 0x20000000>;
+ };
+
+ ahb {
+ compatible = "simple-bus";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ device_type = "soc";
+ ranges;
+
+ vic0: interrupt-controller@ceff0000 {
+ compatible = "arm,pl192-vic";
+ interrupt-controller;
+ reg = <0xceff0000 0x1000>;
+ #interrupt-cells = <1>;
+ };
+
+ vic1: interrupt-controller@80f00000 {
+ compatible = "arm,pl192-vic";
+ interrupt-controller;
+ reg = <0x80f00000 0x1000>;
+ #interrupt-cells = <1>;
+ };
+
+ timer0: timer@c0000080 {
+ compatible = "hpe,gxp-timer";
+ reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
+ interrupts = <0>;
+ interrupt-parent = <&vic0>;
+ clock-frequency = <400000000>;
+ };
+
+ uarta: serial@c00000e0 {
+ compatible = "ns16550a";
+ reg = <0xc00000e0 0x8>;
+ interrupts = <17>;
+ interrupt-parent = <&vic0>;
+ clock-frequency = <1846153>;
+ reg-shift = <0>;
+ };
+
+ uartb: serial@c00000e8 {
+ compatible = "ns16550a";
+ reg = <0xc00000e8 0x8>;
+ interrupts = <18>;
+ interrupt-parent = <&vic0>;
+ clock-frequency = <1846153>;
+ reg-shift = <0>;
+ };
+
+ uartc: serial@c00000f0 {
+ compatible = "ns16550a";
+ reg = <0xc00000f0 0x8>;
+ interrupts = <19>;
+ interrupt-parent = <&vic0>;
+ clock-frequency = <1846153>;
+ reg-shift = <0>;
+ };
+
+ usb0: usb@cefe0000 {
+ compatible = "generic-ehci";
+ reg = <0xcefe0000 0x100>;
+ interrupts = <7>;
+ interrupt-parent = <&vic0>;
+ };
+
+ usb1: usb@cefe0100 {
+ compatible = "generic-ohci";
+ reg = <0xcefe0100 0x110>;
+ interrupts = <6>;
+ interrupt-parent = <&vic0>;
+ };
+
+ vrom@58000000 {
+ compatible = "mtd-ram";
+ bank-width = <4>;
+ reg = <0x58000000 0x4000000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ partition@0 {
+ label = "vrom-prime";
+ reg = <0x0 0x2000000>;
+ };
+ partition@2000000 {
+ label = "vrom-second";
+ reg = <0x2000000 0x2000000>;
+ };
+ };
+
+ i2cg: syscon@c00000f8 {
+ compatible = "simple-mfd", "syscon";
+ reg = <0xc00000f8 0x08>;
+ };
+ };
+
+ clocks {
+ osc: osc {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-output-names = "osc";
+ clock-frequency = <33333333>;
+ };
+
+ iopclk: iopclk {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-output-names = "iopclk";
+ clock-frequency = <400000000>;
+ };
+
+ memclk: memclk {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-output-names = "memclk";
+ clock-frequency = <800000000>;
+ };
+ };
+};
--
2.17.1
On Thu, Mar 10, 2022 at 8:52 PM <[email protected]> wrote:
>
> From: Nick Hawkins <[email protected]>
>
> The HPE SoC is new to linux. This patch
> creates the basic device tree layout with minimum required
> for linux to boot. This includes timer and watchdog
> support.
>
> Signed-off-by: Nick Hawkins <[email protected]>
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "hpe,gxp";
> + model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
> +
> + chosen {
> + bootargs = "earlyprintk console=ttyS2,115200";
> + };
Please drop the bootargs here, you definitely should not have 'earlyprintk'
in the bootargs because that is incompatible with cross-platform kernels.
Instead of passing the console in the bootargs, use the "stdout-path"
property.
The "compatible" property should be a list that contains at least the specific
SoC variant and an identifier for the board. "hpe,gxp" is way too generic
to be the only property here.
> + gxp-init@cefe0010 {
> + compatible = "hpe,gxp-cpu-init";
> + reg = <0xcefe0010 0x04>;
> + };
> +
> + memory@40000000 {
> + device_type = "memory";
> + reg = <0x40000000 0x20000000>;
> + };
> +
> + ahb {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + device_type = "soc";
> + ranges;
> +
> + vic0: interrupt-controller@ceff0000 {
> + compatible = "arm,pl192-vic";
> + interrupt-controller;
> + reg = <0xceff0000 0x1000>;
> + #interrupt-cells = <1>;
> + };
I don't think this represents the actual hierarchy of the devices:
the register range of the "vic0" and the "gxp-init" is very close
together, which usually indicates that they are also on the same
bus.
> + vic1: interrupt-controller@80f00000 {
> + compatible = "arm,pl192-vic";
> + interrupt-controller;
> + reg = <0x80f00000 0x1000>;
> + #interrupt-cells = <1>;
> + };
> +
> + timer0: timer@c0000080 {
> + compatible = "hpe,gxp-timer";
> + reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
> + interrupts = <0>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <400000000>;
> + };
> +
> + uarta: serial@c00000e0 {
> + compatible = "ns16550a";
> + reg = <0xc00000e0 0x8>;
> + interrupts = <17>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <1846153>;
> + reg-shift = <0>;
> + };
In turn, you seem to have a lot of other devices on low addresses
of the 0xc0000000 range, which would be an indication that these
are on a different bus from the others.
Please see if you can find an internal datasheet that describes the
actual device hierarchy, and then try to model this in the device tree.
Use non-empty "ranges" properties to describe the address ranges
and how they get translated between these buses, and add
"dma-ranges" for any high-speed buses that have DMA master
capable devices like USB on them.
> + i2cg: syscon@c00000f8 {
> + compatible = "simple-mfd", "syscon";
> + reg = <0xc00000f8 0x08>;
> + };
> + };
It's odd to have a "syscon" device that only has 8 bytes worth of registers.
What are the contents of this? Is it possible to have a proper abstraction
for it as something that has a specific purpose rather than being a
random collection of individual bits?
Arnd
On 10/03/2022 20:52, [email protected] wrote:
> From: Nick Hawkins <[email protected]>
>
> The HPE SoC is new to linux. This patch
> creates the basic device tree layout with minimum required
> for linux to boot. This includes timer and watchdog
> support.
>
> Signed-off-by: Nick Hawkins <[email protected]>
> ---
> arch/arm/boot/dts/Makefile | 2 +
> arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 27 +++++
> arch/arm/boot/dts/hpe-gxp.dtsi | 148 +++++++++++++++++++++++
> 3 files changed, 177 insertions(+)
> create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index e41eca79c950..2823b359d373 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
> aspeed-bmc-vegman-n110.dtb \
> aspeed-bmc-vegman-rx20.dtb \
> aspeed-bmc-vegman-sx20.dtb
> +dtb-$(CONFIG_ARCH_HPE_GXP) += \
> + hpe-bmc-dl360gen10.dtb
Alphabetically, also in respect to other architectures, so before
CONFIG_ARCH_INTEGRATOR.
> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> new file mode 100644
> index 000000000000..da5eac1213a8
> --- /dev/null
> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for HPE DL360Gen10
> + */
> +
> +/include/ "hpe-gxp.dtsi"
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "hpe,gxp";
Missing board compatible.
> + model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
> +
> + chosen {
> + bootargs = "earlyprintk console=ttyS2,115200";
I have impression we talked about it...
> + };
> +
> + memory@40000000 {
> + device_type = "memory";
> + reg = <0x40000000 0x20000000>;
> + };
> +
> + ahb {
Why do you need empty node?
> +
> + };
> +
> +};
> diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
> new file mode 100644
> index 000000000000..dfaf8df829fe
> --- /dev/null
> +++ b/arch/arm/boot/dts/hpe-gxp.dtsi
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for HPE GXP
> + */
> +
> +/dts-v1/;
> +/ {
> + model = "Hewlett Packard Enterprise GXP BMC";
> + compatible = "hpe,gxp";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu@0 {
> + compatible = "arm,cortex-a9";
> + device_type = "cpu";
> + reg = <0>;
> + };
> + };
> +
> + gxp-init@cefe0010 {
Need a generic node name. gpx-init is specific.
> + compatible = "hpe,gxp-cpu-init";
> + reg = <0xcefe0010 0x04>;
> + };
> +
> + memory@40000000 {
> + device_type = "memory";
> + reg = <0x40000000 0x20000000>;
> + };
> +
> + ahb {
By convention we call it soc.
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + device_type = "soc";
> + ranges;
> +
> + vic0: interrupt-controller@ceff0000 {
> + compatible = "arm,pl192-vic";
> + interrupt-controller;
> + reg = <0xceff0000 0x1000>;
Please put reg after compatible, everywhere.
> + #interrupt-cells = <1>;
> + };
> +
> + vic1: interrupt-controller@80f00000 {
> + compatible = "arm,pl192-vic";
> + interrupt-controller;
> + reg = <0x80f00000 0x1000>;
> + #interrupt-cells = <1>;
> + };
> +
> + timer0: timer@c0000080 {
> + compatible = "hpe,gxp-timer";
> + reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
> + interrupts = <0>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <400000000>;
> + };
> +
> + uarta: serial@c00000e0 {
> + compatible = "ns16550a";
> + reg = <0xc00000e0 0x8>;
> + interrupts = <17>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <1846153>;
> + reg-shift = <0>;
> + };
> +
> + uartb: serial@c00000e8 {
> + compatible = "ns16550a";
> + reg = <0xc00000e8 0x8>;
> + interrupts = <18>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <1846153>;
> + reg-shift = <0>;
> + };
> +
> + uartc: serial@c00000f0 {
> + compatible = "ns16550a";
> + reg = <0xc00000f0 0x8>;
> + interrupts = <19>;
> + interrupt-parent = <&vic0>;
> + clock-frequency = <1846153>;
> + reg-shift = <0>;
> + };
> +
> + usb0: usb@cefe0000 {
> + compatible = "generic-ehci";
I think one of previous comments was that you cannot have "generic-ehci"
only, right?
> + reg = <0xcefe0000 0x100>;
> + interrupts = <7>;
> + interrupt-parent = <&vic0>;
> + };
> +
> + usb1: usb@cefe0100 {
> + compatible = "generic-ohci";
> + reg = <0xcefe0100 0x110>;
> + interrupts = <6>;
> + interrupt-parent = <&vic0>;
> + };
> +
> + vrom@58000000 {
> + compatible = "mtd-ram";
> + bank-width = <4>;
> + reg = <0x58000000 0x4000000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + partition@0 {
> + label = "vrom-prime";
> + reg = <0x0 0x2000000>;
> + };
> + partition@2000000 {
> + label = "vrom-second";
> + reg = <0x2000000 0x2000000>;
> + };
> + };
> +
> + i2cg: syscon@c00000f8 {
> + compatible = "simple-mfd", "syscon";
> + reg = <0xc00000f8 0x08>;
> + };
> + };
> +
> + clocks {
> + osc: osc {
Keep node naming consistent, so just "clk"... but it's also very generic
comparing to others, so I wonder what is this clock?
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-output-names = "osc";
> + clock-frequency = <33333333>;
> + };
> +
> + iopclk: iopclk {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-output-names = "iopclk";
> + clock-frequency = <400000000>;
> + };
> +
> + memclk: memclk {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-output-names = "memclk";
> + clock-frequency = <800000000>;
> + };
What are these clocks? If external to the SoC, then where are they? On
the board?
> + };
> +};
Best regards,
Krzysztof
On 16/03/2022 16:41, Hawkins, Nick wrote:
> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:[email protected]]
> Sent: Friday, March 11, 2022 4:30 AM
> To: Hawkins, Nick <[email protected]>>; Verdun, Jean-Marie <[email protected]>>
> Cc: Arnd Bergmann <[email protected]>>; Olof Johansson <[email protected]>>; [email protected]; Rob Herring <[email protected]>>; [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
>
> On 10/03/2022 20:52, [email protected] wrote:
>>> From: Nick Hawkins <[email protected]>>
>>>
>>> The HPE SoC is new to linux. This patch creates the basic device tree
>>> layout with minimum required for linux to boot. This includes timer
>>> and watchdog support.
>>>
>>> Signed-off-by: Nick Hawkins <[email protected]>>
>>> ---
>>> arch/arm/boot/dts/Makefile | 2 +
>>> arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 27 +++++
>>> arch/arm/boot/dts/hpe-gxp.dtsi | 148 +++++++++++++++++++++++
>>> 3 files changed, 177 insertions(+)
>>> create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>> create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi
>>>
>>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>>> index e41eca79c950..2823b359d373 100644
>>> --- a/arch/arm/boot/dts/Makefile
>>> +++ b/arch/arm/boot/dts/Makefile
>>> @@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>> aspeed-bmc-vegman-n110.dtb \
>>> aspeed-bmc-vegman-rx20.dtb \
>>> aspeed-bmc-vegman-sx20.dtb
>>> +dtb-$(CONFIG_ARCH_HPE_GXP) += \
>>> + hpe-bmc-dl360gen10.dtb
>
>> Alphabetically, also in respect to other architectures, so before CONFIG_ARCH_INTEGRATOR.
>
> Done
>
>>> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>> b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>> new file mode 100644
>>> index 000000000000..da5eac1213a8
>>> --- /dev/null
>>> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>> @@ -0,0 +1,27 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Device Tree file for HPE DL360Gen10 */
>>> +
>>> +/include/ "hpe-gxp.dtsi"
>>> +
>>> +/ {
>>> + #address-cells = <1>>;
>>> + #size-cells = <1>>;
>>> + compatible = "hpe,gxp";
>
>> Missing board compatible.
>
> Will become compatible = "hpe,gxp","hpe,bmc-dl360gen10"; If that seems okay to you.
Yes, except hpe,gxp goes at the end.
>
>>> + model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
>>> +
(...)
>>> +
>>> + usb0: usb@cefe0000 {
>>> + compatible = "generic-ehci";
>
>> I think one of previous comments was that you cannot have "generic-ehci"
>> only, right?
>
> Yes there was, I removed the usb0: ehci@cefe0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?
Yes,, see other cases in generic-ehci.yaml bindings. Your current choice
would be pointed out by dtbs_check, that it's invalid according to
current bindings.
>
>>> + reg = <0xcefe0000 0x100>>;
>>> + interrupts = <7>>;
>>> + interrupt-parent = <&vic0>>;
>>> + };
>>> +
>>> + usb1: usb@cefe0100 {
>>> + compatible = "generic-ohci";
>>> + reg = <0xcefe0100 0x110>>;
>>> + interrupts = <6>>;
>>> + interrupt-parent = <&vic0>>;
>>> + };
>>> +
>>> + vrom@58000000 {
>>> + compatible = "mtd-ram";
>>> + bank-width = <4>>;
>>> + reg = <0x58000000 0x4000000>>;
>>> + #address-cells = <1>>;
>>> + #size-cells = <1>>;
>>> + partition@0 {
>>> + label = "vrom-prime";
>>> + reg = <0x0 0x2000000>>;
>>> + };
>>> + partition@2000000 {
>>> + label = "vrom-second";
>>> + reg = <0x2000000 0x2000000>>;
>>> + };
>>> + };
>>> +
>>> + i2cg: syscon@c00000f8 {
>
>
>>> + compatible = "simple-mfd", "syscon";
>>> + reg = <0xc00000f8 0x08>>;
>>> + };
>>> + };
>>> +
>>> + clocks {
>>> + osc: osc {
>
>> Keep node naming consistent, so just "clk"... but it's also very generic comparing to others, so I wonder what is this clock?
>
> We are in the process of redoing the clocks. This was the oscillator but no longer needed for the minimum boot config.
>
>>> + compatible = "fixed-clock";
>>> + #clock-cells = <0>>;
>>> + clock-output-names = "osc";
>>> + clock-frequency = <33333333>>;
>>> + };
>>> +
>>> + iopclk: iopclk {
>>> + compatible = "fixed-clock";
>>> + #clock-cells = <0>>;
>>> + clock-output-names = "iopclk";
>>> + clock-frequency = <400000000>>;
>>> + };
>>> +
>>> + memclk: memclk {
>>> + compatible = "fixed-clock";
>>> + #clock-cells = <0>>;
>>> + clock-output-names = "memclk";
>>> + clock-frequency = <800000000>>;
>>> + };
>
>> What are these clocks? If external to the SoC, then where are they? On the board?
>
> This was the internal iopclk and memclk they were both internal to the chip.
> For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.
You should rather have a clock controller driver which defines this (and
many others). They can stay as a temporary work-around, if you really
need them for some other nodes.
Best regards,
Krzysztof
-----Original Message-----
From: Krzysztof Kozlowski [mailto:[email protected]]
Sent: Friday, March 11, 2022 4:30 AM
To: Hawkins, Nick <[email protected]>>; Verdun, Jean-Marie <[email protected]>>
Cc: Arnd Bergmann <[email protected]>>; Olof Johansson <[email protected]>>; [email protected]; Rob Herring <[email protected]>>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
On 10/03/2022 20:52, [email protected] wrote:
>> From: Nick Hawkins <[email protected]>>
>>
>> The HPE SoC is new to linux. This patch creates the basic device tree
>> layout with minimum required for linux to boot. This includes timer
>> and watchdog support.
>>
>> Signed-off-by: Nick Hawkins <[email protected]>>
>> ---
>> arch/arm/boot/dts/Makefile | 2 +
>> arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 27 +++++
>> arch/arm/boot/dts/hpe-gxp.dtsi | 148 +++++++++++++++++++++++
>> 3 files changed, 177 insertions(+)
>> create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>> create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index e41eca79c950..2823b359d373 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>> aspeed-bmc-vegman-n110.dtb \
>> aspeed-bmc-vegman-rx20.dtb \
>> aspeed-bmc-vegman-sx20.dtb
>> +dtb-$(CONFIG_ARCH_HPE_GXP) += \
>> + hpe-bmc-dl360gen10.dtb
> Alphabetically, also in respect to other architectures, so before CONFIG_ARCH_INTEGRATOR.
Done
>> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>> b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>> new file mode 100644
>> index 000000000000..da5eac1213a8
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>> @@ -0,0 +1,27 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device Tree file for HPE DL360Gen10 */
>> +
>> +/include/ "hpe-gxp.dtsi"
>> +
>> +/ {
>> + #address-cells = <1>>;
>> + #size-cells = <1>>;
>> + compatible = "hpe,gxp";
> Missing board compatible.
Will become compatible = "hpe,gxp","hpe,bmc-dl360gen10"; If that seems okay to you.
>> + model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
>> +
>> + chosen {
>> + bootargs = "earlyprintk console=ttyS2,115200";
> I have impression we talked about it...
Removed!
>> + };
>> +
>> + memory@40000000 {
>> + device_type = "memory";
>> + reg = <0x40000000 0x20000000>>;
>> + };
>> +
>> + ahb {
> Why do you need empty node?
I do not, this was a placeholder for after the initial checkin where I would put all the board specific stuff. This has been removed.
>> +
>> + };
>> +
>> +};
>> diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi
>> b/arch/arm/boot/dts/hpe-gxp.dtsi new file mode 100644 index
>> 000000000000..dfaf8df829fe
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hpe-gxp.dtsi
>> @@ -0,0 +1,148 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device Tree file for HPE GXP
>> + */
>> +
>> +/dts-v1/;
>> +/ {
>> + model = "Hewlett Packard Enterprise GXP BMC";
>> + compatible = "hpe,gxp";
>> + #address-cells = <1>>;
>> + #size-cells = <1>>;
>> +
>> + cpus {
>> + #address-cells = <1>>;
>> + #size-cells = <0>>;
>> +
>> + cpu@0 {
>> + compatible = "arm,cortex-a9";
>> + device_type = "cpu";
>> + reg = <0>>;
>> + };
>> + };
>> +
>> + gxp-init@cefe0010 {
> Need a generic node name. gpx-init is specific.
Will do. But more than likely this is going to get removed as I try to push this option into the bootloader.
>> + compatible = "hpe,gxp-cpu-init";
>> + reg = <0xcefe0010 0x04>>;
>> + };
>> +
>> + memory@40000000 {
>> + device_type = "memory";
>> + reg = <0x40000000 0x20000000>>;
>> + };
>> +
>> + ahb {
> By convention we call it soc.
>> + compatible = "simple-bus";
>> + #address-cells = <1>>;
>> + #size-cells = <1>>;
>> + device_type = "soc";
>> + ranges;
>> +
>> + vic0: interrupt-controller@ceff0000 {
>> + compatible = "arm,pl192-vic";
>> + interrupt-controller;
>> + reg = <0xceff0000 0x1000>>;
> Please put reg after compatible, everywhere.
Done
>> + #interrupt-cells = <1>>;
>> + };
>> +
>> + vic1: interrupt-controller@80f00000 {
>> + compatible = "arm,pl192-vic";
>> + interrupt-controller;
>> + reg = <0x80f00000 0x1000>>;
>> + #interrupt-cells = <1>>;
>> + };
>> +
>> + timer0: timer@c0000080 {
>> + compatible = "hpe,gxp-timer";
>> + reg = <0xc0000080 0x1>>, <0xc0000094 0x01>>, <0xc0000088 0x08>>;
>> + interrupts = <0>>;
>> + interrupt-parent = <&vic0>>;
>> + clock-frequency = <400000000>>;
>> + };
>> +
>> + uarta: serial@c00000e0 {
>> + compatible = "ns16550a";
>> + reg = <0xc00000e0 0x8>>;
>> + interrupts = <17>>;
>> + interrupt-parent = <&vic0>>;
>> + clock-frequency = <1846153>>;
>> + reg-shift = <0>>;
>> + };
>> +
>> + uartb: serial@c00000e8 {
>> + compatible = "ns16550a";
>> + reg = <0xc00000e8 0x8>>;
>> + interrupts = <18>>;
>> + interrupt-parent = <&vic0>>;
>> + clock-frequency = <1846153>>;
>> + reg-shift = <0>>;
>> + };
>> +
>> + uartc: serial@c00000f0 {
>> + compatible = "ns16550a";
>> + reg = <0xc00000f0 0x8>>;
>> + interrupts = <19>>;
>> + interrupt-parent = <&vic0>>;
>> + clock-frequency = <1846153>>;
>> + reg-shift = <0>>;
>> + };
>> +
>> + usb0: usb@cefe0000 {
>> + compatible = "generic-ehci";
> I think one of previous comments was that you cannot have "generic-ehci"
> only, right?
Yes there was, I removed the usb0: ehci@cefe0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?
>> + reg = <0xcefe0000 0x100>>;
>> + interrupts = <7>>;
>> + interrupt-parent = <&vic0>>;
>> + };
>> +
>> + usb1: usb@cefe0100 {
>> + compatible = "generic-ohci";
>> + reg = <0xcefe0100 0x110>>;
>> + interrupts = <6>>;
>> + interrupt-parent = <&vic0>>;
>> + };
>> +
>> + vrom@58000000 {
>> + compatible = "mtd-ram";
>> + bank-width = <4>>;
>> + reg = <0x58000000 0x4000000>>;
>> + #address-cells = <1>>;
>> + #size-cells = <1>>;
>> + partition@0 {
>> + label = "vrom-prime";
>> + reg = <0x0 0x2000000>>;
>> + };
>> + partition@2000000 {
>> + label = "vrom-second";
>> + reg = <0x2000000 0x2000000>>;
>> + };
>> + };
>> +
>> + i2cg: syscon@c00000f8 {
>> + compatible = "simple-mfd", "syscon";
>> + reg = <0xc00000f8 0x08>>;
>> + };
>> + };
>> +
>> + clocks {
>> + osc: osc {
> Keep node naming consistent, so just "clk"... but it's also very generic comparing to others, so I wonder what is this clock?
We are in the process of redoing the clocks. This was the oscillator but no longer needed for the minimum boot config.
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>>;
>> + clock-output-names = "osc";
>> + clock-frequency = <33333333>>;
>> + };
>> +
>> + iopclk: iopclk {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>>;
>> + clock-output-names = "iopclk";
>> + clock-frequency = <400000000>>;
>> + };
>> +
>> + memclk: memclk {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>>;
>> + clock-output-names = "memclk";
>> + clock-frequency = <800000000>>;
>> + };
> What are these clocks? If external to the SoC, then where are they? On the board?
This was the internal iopclk and memclk they were both internal to the chip.
For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.
>> + };
>> +};
Thanks,
-Nick
>> -----Original Message-----
>> From: Krzysztof Kozlowski [mailto:[email protected]]
>> Sent: Friday, March 11, 2022 4:30 AM
>> To: Hawkins, Nick <[email protected]>>>>; Verdun, Jean-Marie
>> <[email protected]>>>>
>> Cc: Arnd Bergmann <[email protected]>>>>; Olof Johansson <[email protected]>>>>;
>> [email protected]; Rob Herring <[email protected]>>>>;
>> [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP
>> Device tree
>>
>> On 10/03/2022 20:52, [email protected] wrote:
>>>>>> From: Nick Hawkins <[email protected]>>>>
>>>>>>
>>>>>> The HPE SoC is new to linux. This patch creates the basic device
>>>>>> tree layout with minimum required for linux to boot. This includes
>>>>>> timer and watchdog support.
>>>>>>
>>>>>> Signed-off-by: Nick Hawkins <[email protected]>>>>
>>>>>> ---
>>>>>> arch/arm/boot/dts/Makefile | 2 +
>>>>>> arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 27 +++++
>>>>>> arch/arm/boot/dts/hpe-gxp.dtsi | 148 +++++++++++++++++++++++
>>>>>> 3 files changed, 177 insertions(+)
>>>>>> create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>> create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>>>>>> index e41eca79c950..2823b359d373 100644
>>>>>> --- a/arch/arm/boot/dts/Makefile
>>>>>> +++ b/arch/arm/boot/dts/Makefile
>>>>>> @@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>>>>> aspeed-bmc-vegman-n110.dtb \
>>>>>> aspeed-bmc-vegman-rx20.dtb \
>>>>>> aspeed-bmc-vegman-sx20.dtb
>>>>>> +dtb-$(CONFIG_ARCH_HPE_GXP) += \
>>>>>> + hpe-bmc-dl360gen10.dtb
>>
>>>> Alphabetically, also in respect to other architectures, so before CONFIG_ARCH_INTEGRATOR.
>>
>> Done
>>
>>>>>> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>> b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>> new file mode 100644
>>>>>> index 000000000000..da5eac1213a8
>>>>>> --- /dev/null
>>>>>> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>> @@ -0,0 +1,27 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +/*
>>>>>> + * Device Tree file for HPE DL360Gen10 */
>>>>>> +
>>>>>> +/include/ "hpe-gxp.dtsi"
>>>>>> +
>>>>>> +/ {
>>>>>> + #address-cells = <1>>>>;
>>>>>> + #size-cells = <1>>>>;
>>>>>> + compatible = "hpe,gxp";
>>
>>>> Missing board compatible.
>>
>> Will become compatible = "hpe,gxp","hpe,bmc-dl360gen10"; If that seems okay to you.
> Yes, except hpe,gxp goes at the end.
Done
>>
>>>>>> + model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
>>>>>> +
(...)
>>>>>> +
>>>>>> + usb0: usb@cefe0000 {
>>>>>> + compatible = "generic-ehci";
>>
>>>> I think one of previous comments was that you cannot have "generic-ehci"
>>>> only, right?
>>
>> Yes there was, I removed the usb0: ehci@cefe0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?
> Yes,, see other cases in generic-ehci.yaml bindings. Your current choice would be pointed out by dtbs_check, that it's invalid according to current bindings.
For some reason when I compile I am not seeing a warning for that file. I have been using "make dtbs_check" and "make dtbs W=1". Perhaps I am missing an important flag?
In the case of creating a hpe,gxp-ehci binding would I need to add that to the generic-ehci.yaml?
>>
>>>>>> + reg = <0xcefe0000 0x100>>>>;
>>>>>> + interrupts = <7>>>>;
>>>>>> + interrupt-parent = <&vic0>>>>;
>>>>>> + };
>>>>>> +
>>>>>> + usb1: usb@cefe0100 {
>>>>>> + compatible = "generic-ohci";
>>>>>> + reg = <0xcefe0100 0x110>>>>;
>>>>>> + interrupts = <6>>>>;
>>>>>> + interrupt-parent = <&vic0>>>>;
>>>>>> + };
>>>>>> +
>>>>>> + vrom@58000000 {
>>>>>> + compatible = "mtd-ram";
>>>>>> + bank-width = <4>>>>;
>>>>>> + reg = <0x58000000 0x4000000>>>>;
>>>>>> + #address-cells = <1>>>>;
>>>>>> + #size-cells = <1>>>>;
>>>>>> + partition@0 {
>>>>>> + label = "vrom-prime";
>>>>>> + reg = <0x0 0x2000000>>>>;
>>>>>> + };
>>>>>> + partition@2000000 {
>>>>>> + label = "vrom-second";
>>>>>> + reg = <0x2000000 0x2000000>>>>;
>>>>>> + };
>>>>>> + };
>>>>>> +
>>>>>> + i2cg: syscon@c00000f8 {
>>
>>
>>>>>> + compatible = "simple-mfd", "syscon";
>>>>>> + reg = <0xc00000f8 0x08>>>>;
>>>>>> + };
>>>>>> + };
>>>>>> +
>>>>>> + clocks {
>>>>>> + osc: osc {
>>
>>>> Keep node naming consistent, so just "clk"... but it's also very generic comparing to others, so I wonder what is this clock?
>>
>> We are in the process of redoing the clocks. This was the oscillator but no longer needed for the minimum boot config.
>>
>>>>>> + compatible = "fixed-clock";
>>>>>> + #clock-cells = <0>>>>;
>>>>>> + clock-output-names = "osc";
>>>>>> + clock-frequency = <33333333>>>>;
>>>>>> + };
>>>>>> +
>>>>>> + iopclk: iopclk {
>>>>>> + compatible = "fixed-clock";
>>>>>> + #clock-cells = <0>>>>;
>>>>>> + clock-output-names = "iopclk";
>>>>>> + clock-frequency = <400000000>>>>;
>>>>>> + };
>>>>>> +
>>>>>> + memclk: memclk {
>>>>>> + compatible = "fixed-clock";
>>>>>> + #clock-cells = <0>>>>;
>>>>>> + clock-output-names = "memclk";
>>>>>> + clock-frequency = <800000000>>>>;
>>>>>> + };
>>
>>>> What are these clocks? If external to the SoC, then where are they? On the board?
>>
>> This was the internal iopclk and memclk they were both internal to the chip.
>> For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.
> You should rather have a clock controller driver which defines this (and many others). They can stay as a temporary work-around, if you really need them for some other nodes.
I am trying to picture what you are saying but I am unsure, I know that on a separate review you mentioned that the gxp-timer needed to have clocks, and clock-names inside the node. Would it be improper for the gxp-timer to reference iopclk?
Thanks!
-Nick Hawkins
On 16/03/2022 21:10, Hawkins, Nick wrote:
(...)
>>>>> I think one of previous comments was that you cannot have "generic-ehci"
>>>>> only, right?
>>>
>>> Yes there was, I removed the usb0: ehci@cefe0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?
>
>> Yes,, see other cases in generic-ehci.yaml bindings. Your current choice would be pointed out by dtbs_check, that it's invalid according to current bindings.
>
> For some reason when I compile I am not seeing a warning for that file. I have been using "make dtbs_check" and "make dtbs W=1". Perhaps I am missing an important flag?
My bad, I misread the generic-ehci binding, so your compatible is
actually correct from bindings point of view. Still common practice is
to add own compatible which allows later customization.
> In the case of creating a hpe,gxp-ehci binding would I need to add that to the generic-ehci.yaml?
Yes, add your compatible to that big enum with list of many implementations.
(...)
>>>>>>> +
>>>>>>> + memclk: memclk {
>>>>>>> + compatible = "fixed-clock";
>>>>>>> + #clock-cells = <0>>>>;
>>>>>>> + clock-output-names = "memclk";
>>>>>>> + clock-frequency = <800000000>>>>;
>>>>>>> + };
>>>
>>>>> What are these clocks? If external to the SoC, then where are they? On the board?
>>>
>>> This was the internal iopclk and memclk they were both internal to the chip.
>>> For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.
>
>> You should rather have a clock controller driver which defines this (and many others). They can stay as a temporary work-around, if you really need them for some other nodes.
>
> I am trying to picture what you are saying but I am unsure, I know that on a separate review you mentioned that the gxp-timer needed to have clocks, and clock-names inside the node. Would it be improper for the gxp-timer to reference iopclk?
It all depends on the architecture of your SoC. I could imagine such one:
1. Few external (from SoC point of view) oscillators, usually provided
on the board. It could be 24 MHz, could be 32767 Hz for Bluetooth etc.
2. One or several clock controllers inside the SoC which take as input
these external clocks. The clock controller provides clocks for several
other components/blocks. Allows also gating clocks, reparenting and
changing dividers (rate).
3. Other blocks within your SoC, e.g. gxp-timer, use these clocks.
The true question is where is that memclk or iopclk generated? Is it
controllable (gate/mux/rate)? Even some fixed-frequency clocks, coming
out of that clock controller (point 2.), are defined in the clock
controller because that's the logical place for them.
Best regards,
Krzysztof
On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <[email protected]> wrote:
> I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls in the same area.
>
> For instance with our watchdog controls we have:
>
> @90 the countdown value
> @96 the configuration
>
> And for our timer we have:
> @80 the countdown value
> @94 the configuration
> @88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.
>
> What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers.
I think this is most commonly done using a 'syscon' node, have a look at the
files listed by
$ git grep syscon drivers/watchdog/ drivers/clocksource/
You may also want to look at those drivers to find if any of them can
be directly reused,
this is perhaps a licensed IP block that others are using as well.
Arnd
-----Original Message-----
From: Krzysztof Kozlowski [mailto:[email protected]]
Sent: Thursday, March 17, 2022 3:37 AM
To: Hawkins, Nick <[email protected]>; Verdun, Jean-Marie <[email protected]>
Cc: Arnd Bergmann <[email protected]>; Olof Johansson <[email protected]>; [email protected]; Rob Herring <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
On 16/03/2022 21:10, Hawkins, Nick wrote:
(...)
>>>>>> I think one of previous comments was that you cannot have "generic-ehci"
>>>>>> only, right?
>>>
>>>> Yes there was, I removed the usb0: ehci@cefe0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?
>>
>>> Yes,, see other cases in generic-ehci.yaml bindings. Your current choice would be pointed out by dtbs_check, that it's invalid according to current bindings.
>>
>> For some reason when I compile I am not seeing a warning for that file. I have been using "make dtbs_check" and "make dtbs W=1". Perhaps I am missing an important flag?
> My bad, I misread the generic-ehci binding, so your compatible is actually correct from bindings point of view. Still common practice is to add own compatible which allows later customization.
>> In the case of creating a hpe,gxp-ehci binding would I need to add that to the generic-ehci.yaml?
> Yes, add your compatible to that big enum with list of many implementations.
Done.
> (...)
>>>>>>>> +
>>>>>>>> + memclk: memclk {
>>>>>>>> + compatible = "fixed-clock";
>>>>>>>> + #clock-cells = <0>>>>;
>>>>>>>> + clock-output-names = "memclk";
>>>>>>>> + clock-frequency = <800000000>>>>;
>>>>>>>> + };
>>>
>>>>>> What are these clocks? If external to the SoC, then where are they? On the board?
>>>
>>>> This was the internal iopclk and memclk they were both internal to the chip.
>>>> For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.
>>
>>> You should rather have a clock controller driver which defines this (and many others). They can stay as a temporary work-around, if you really need them for some other nodes.
>>
>> I am trying to picture what you are saying but I am unsure, I know that on a separate review you mentioned that the gxp-timer needed to have clocks, and clock-names inside the node. Would it be improper for the gxp-timer to reference iopclk?
> It all depends on the architecture of your SoC. I could imagine such one:
> 1. Few external (from SoC point of view) oscillators, usually provided on the board. It could be 24 MHz, could be 32767 Hz for Bluetooth etc.
> 2. One or several clock controllers inside the SoC which take as input these external clocks. The clock controller provides clocks for several other components/blocks. Allows also gating clocks, reparenting and changing dividers (rate).
> 3. Other blocks within your SoC, e.g. gxp-timer, use these clocks.
> The true question is where is that memclk or iopclk generated? Is it controllable (gate/mux/rate)? Even some fixed-frequency clocks, coming out of that clock controller (point 2.), are defined in the clock controller because that's the logical place for >them.
From the perspective of our SoC there is a PPUCLK that generates the clk signals for our watchdog and timer. This happens inside the SoC. I am trying to represent this below.
axi {
compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
ranges;
dma-ranges;
ahb@c0000000 {
compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0xc0000000 0x30000000>;
...
timer0: timer@80 {
compatible = "hpe,gxp-timer";
reg = <0x80 0x1>, <0x94 0x01>, <0x88 0x08>;
interrupts = <0>;
interrupt-parent = <&vic0>;
clocks = <&ppuclk>;
clock-names = "ppuclk";
clock-frequency = <400000000>;
};
watchdog0: watchdog@90 {
compatible = "hpe,gxp-wdt";
reg = <0x90 0x02>, <0x96 0x01>;
};
...
};
clocks {
ppuclk: ppuclk {
compatible = "fixed-clock";
#clock-cells = <0>;
clock-output-names = "ppuclk";
clock-frequency = <400000000>;
};
};
I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls in the same area.
For instance with our watchdog controls we have:
@90 the countdown value
@96 the configuration
And for our timer we have:
@80 the countdown value
@94 the configuration
@88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.
What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers.
Thanks,
-Nick Hawkins
-----Original Message-----
From: Arnd Bergmann [mailto:[email protected]]
Sent: Tuesday, March 29, 2022 4:13 PM
To: Hawkins, Nick <[email protected]>
Cc: Krzysztof Kozlowski <[email protected]>; Verdun, Jean-Marie <[email protected]>; Arnd Bergmann <[email protected]>; Olof Johansson <[email protected]>; [email protected]; Rob Herring <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <[email protected]>> wrote:
>> I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls in the same area.
>
>> For instance with our watchdog controls we have:
>
>> @90 the countdown value
>> @96 the configuration
>
>> And for our timer we have:
>> @80 the countdown value
>> @94 the configuration
>> @88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.
>
>> What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers.
> I think this is most commonly done using a 'syscon' node, have a look at the files listed by
> $ git grep syscon drivers/watchdog/ drivers/clocksource/
Is this a good example of what you were thinking of? I found this in arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi
sysctrl@61840000 {
compatible = "socionext,uniphier-ld11-sysctrl",
"simple-mfd", "syscon";
reg = <0x61840000 0x10000>;
sys_clk: clock {
compatible = "socionext,uniphier-ld11-clock";
#clock-cells = <1>;
};
sys_rst: reset {
compatible = "socionext,uniphier-ld11-reset";
#reset-cells = <1>;
};
watchdog {
compatible = "socionext,uniphier-wdt";
};
};
If that is the case..
timectrl@80 {
compatible = "hpe,gxp-timectrl","syscon";
reg = <0x80 0x16>;
watchdog0: watchdog {
compatible = "hpe,gxp-wdt";
}
timer0: timer {
compatible = "hpe,gxp-timer";
}
}
> You may also want to look at those drivers to find if any of them can be directly reused, this is perhaps a licensed IP block that others are using as well.
I will look and see if any of them have the same register sets and functionality.
Thanks,
-Nick Hawkins
-----Original Message-----
From: Arnd Bergmann [mailto:[email protected]]
Sent: Tuesday, March 29, 2022 4:13 PM
To: Hawkins, Nick <[email protected]>
Cc: Krzysztof Kozlowski <[email protected]>; Verdun, Jean-Marie <[email protected]>; Arnd Bergmann <[email protected]>; Olof Johansson <[email protected]>; [email protected]; Rob Herring <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <[email protected]>> wrote:
>> I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls in the same area.
>
>> For instance with our watchdog controls we have:
>
>> @90 the countdown value
>> @96 the configuration
>
>> And for our timer we have:
>> @80 the countdown value
>> @94 the configuration
>> @88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.
>
>> What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers.
> I think this is most commonly done using a 'syscon' node, have a look at the files listed by
I found an example and copied it although I have a couple questions when it comes to actually coding it. Can that be here or should I post these questions in the patch that actually concern the file?
st: timer@80 {
compatible = "hpe,gxp-timer","syscon","simple-mfd";
reg = <0x80 0x16>;
interrupts = <0>;
interrupt-parent = <&vic0>;
clocks = <&ppuclk>;
clock-names = "ppuclk";
clock-frequency = <400000000>;
watchdog {
compatible = "hpe,gxp-wdt";
};
};
Thanks,
-Nick Hawkins
On Thu, Mar 31, 2022 at 12:27 AM Hawkins, Nick <[email protected]> wrote:
> On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <[email protected]>> wrote:
>
> >> I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls in the same area.
> >
> >> For instance with our watchdog controls we have:
> >
> >> @90 the countdown value
> >> @96 the configuration
> >
> >> And for our timer we have:
> >> @80 the countdown value
> >> @94 the configuration
> >> @88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.
> >
> >> What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers.
>
> > I think this is most commonly done using a 'syscon' node, have a look at the files listed by
>
> I found an example and copied it although I have a couple questions when it comes to actually coding it. Can that be here or should I post these questions in the patch that actually concern the file?
>
> st: timer@80 {
> compatible = "hpe,gxp-timer","syscon","simple-mfd";
> reg = <0x80 0x16>;
> interrupts = <0>;
> interrupt-parent = <&vic0>;
> clocks = <&ppuclk>;
> clock-names = "ppuclk";
> clock-frequency = <400000000>;
>
> watchdog {
> compatible = "hpe,gxp-wdt";
> };
> };
I'd have to study the other examples myself to see what is most common.
My feeling would be that it's better to either have a "hpe,gxp-timer" parent
device with a watchdog child but no syscon, or to have a syscon/simple-mfd
parent with both the timer and the watchdog as children.
Arnd
On Thu, Mar 31, 2022 at 11:09 PM Hawkins, Nick <[email protected]> wrote:
> On Thu, Mar 31, 2022 at 12:27 AM Hawkins, Nick <[email protected]> wrote:
> > On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <[email protected]>> wrote:
> > I'd have to study the other examples myself to see what is most common.
>
> > My feeling would be that it's better to either have a "hpe,gxp-timer" parent device with a watchdog child but no syscon, or to have a syscon/simple-mfd parent with both the timer and the watchdog as children.
>
> Arnd, thanks for the feedback. I am trying to use the approach you recommend where you have a syscon/simple-mfd parent with watchdog and timer as children.
>
> st: chip-controller@80 {
> compatible = "hpe,gxp-ctrl-st","syscon","simple-mfd";
> reg = <0x80 0x16>;
>
> timer0: timer {
> compatible = "hpe,gxp-timer";
> interrupts = <0>;
> interrupt-parent = <&vic0>;
> clocks = <&ppuclk>;
> clock-names = "ppuclk";
> };
>
> watchdog {
> compatible = "hpe,gxp-wdt";
> };
> };
>
> This compiles without any errors but I do have some questions about accessing the regmap in both drivers, specifically the timer code. How do you use a regmap with clocksource_mmio_init? I tried searching through the codebase and could not find the answer. I assume I am missing some vital step.
I don't think you can do this, if you are using the syscon regmap, you
go through the
regmap indirection rather than accessing the mmio register by virtual address,
and this may result in some extra code in your driver, and a little
runtime overhead.
If you prefer to avoid that, you can go back to having the timer node as the
parent, but without being a syscon. In this case, the watchdog would be handled
in one of these ways:
a) a child device gets created from the clocksource driver and bound to the
watchdog driver, which then uses a private interface between the clocksource
and the watchdog to access the registers
b) the clocksource driver itself registers as a watchdog driver, without
having a separate driver module
One thing to consider is whether the register range here contains any
registers that may be used in another driver, e.g. a second timer,
a PWM, or a clk controller. If not, you are fairly free to pick any of these
approaches.
Arnd
On Fri, Apr 1, 2022 at 6:05 PM Hawkins, Nick <[email protected]> wrote:
> > I don't think you can do this, if you are using the syscon regmap, you go through the regmap indirection rather than accessing the mmio register by virtual address, and this may result in some extra code in your driver, and a little runtime overhead.
>
> > If you prefer to avoid that, you can go back to having the timer node as the parent, but without being a syscon. In this case, the watchdog would be handled in one of these ways:
>
> > a) a child device gets created from the clocksource driver and bound to the
> watchdog driver, which then uses a private interface between the clocksource
> and the watchdog to access the registers
>
> > b) the clocksource driver itself registers as a watchdog driver, without
> having a separate driver module
>
> > One thing to consider is whether the register range here contains any registers that may be used in another driver, e.g. a second timer, a PWM, or a clk controller. If not, you are fairly free to pick any of these approaches.
>
> I will try to use the b) approach everything in that range is timer or watchdog related. There is a second timer however there are no plans on using that. Should the combined code still live inside the driver/timer directory or should it be moved to mfd?
I would put it into drivers/clocksource/, I don't think drivers/mtd
would be any better,
but there is a chance that the clocksource maintainers don't want to
have the watchdog
code in their tree.
Arnd
On Mon, Apr 4, 2022 at 10:22 PM Hawkins, Nick <[email protected]> wrote:
> > > I would put it into drivers/clocksource/, I don't think drivers/mtd would be any better, but there is a chance that the clocksource maintainers don't want to have the watchdog code in their tree.
>
> While trying to discover how to creating two devices in one driver I ran across an interesting .dtsi and I was wondering if this would be a valid approach for my situation as well. The pertinent files are:
> 1) drivers/clocksource/timer-digicolor.c
> 2) arch/arm/boot/dts/cx92755.dtsi
> 3) drivers/watchdog/digicolor_wdt.c
>
> Here they are just sharing the same register area:
>
> timer@f0000fc0 {
> compatible = "cnxt,cx92755-timer";
> reg = <0xf0000fc0 0x40>;
> interrupts = <19>, <31>, <34>, <35>, <52>, <53>, <54>, <55>;
> clocks = <&main_clk>;
> };
>
> rtc@f0000c30 {
> compatible = "cnxt,cx92755-rtc";
> reg = <0xf0000c30 0x18>;
> interrupts = <25>;
> };
>
> watchdog@f0000fc0 {
> compatible = "cnxt,cx92755-wdt";
> reg = <0xf0000fc0 0x8>;
> clocks = <&main_clk>;
> timeout-sec = <15>;
> };
Right, it is possible to make this work, but it's not recommended, and you have
to work around the sanity checks in the code that try to keep you from doing it
wrong, as well as any tooling that tries to check for these in the DT.
Arnd
-----Original Message-----
From: Arnd Bergmann [mailto:[email protected]]
Sent: Friday, April 1, 2022 11:30 AM
To: Hawkins, Nick <[email protected]>
Cc: Arnd Bergmann <[email protected]>; Verdun, Jean-Marie <[email protected]>; Olof Johansson <[email protected]>; [email protected]; Rob Herring <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
On Fri, Apr 1, 2022 at 6:05 PM Hawkins, Nick <[email protected]> wrote:
> > > I don't think you can do this, if you are using the syscon regmap, you go through the regmap indirection rather than accessing the mmio register by virtual address, and this may result in some extra code in your driver, and a little runtime overhead.
> >
> > > If you prefer to avoid that, you can go back to having the timer node as the parent, but without being a syscon. In this case, the watchdog would be handled in one of these ways:
> >
> > > a) a child device gets created from the clocksource driver and bound
> > > to the
> > watchdog driver, which then uses a private interface between the clocksource
> > and the watchdog to access the registers
> >
> > > b) the clocksource driver itself registers as a watchdog driver,
> > > without
> > having a separate driver module
> >
> > > One thing to consider is whether the register range here contains any registers that may be used in another driver, e.g. a second timer, a PWM, or a clk controller. If not, you are fairly free to pick any of these approaches.
> >
> > I will try to use the b) approach everything in that range is timer or watchdog related. There is a second timer however there are no plans on using that. Should the combined code still live inside the driver/timer directory or should it be moved to mfd?
> > I would put it into drivers/clocksource/, I don't think drivers/mtd would be any better, but there is a chance that the clocksource maintainers don't want to have the watchdog code in their tree.
While trying to discover how to creating two devices in one driver I ran across an interesting .dtsi and I was wondering if this would be a valid approach for my situation as well. The pertinent files are:
1) drivers/clocksource/timer-digicolor.c
2) arch/arm/boot/dts/cx92755.dtsi
3) drivers/watchdog/digicolor_wdt.c
Here they are just sharing the same register area:
timer@f0000fc0 {
compatible = "cnxt,cx92755-timer";
reg = <0xf0000fc0 0x40>;
interrupts = <19>, <31>, <34>, <35>, <52>, <53>, <54>, <55>;
clocks = <&main_clk>;
};
rtc@f0000c30 {
compatible = "cnxt,cx92755-rtc";
reg = <0xf0000c30 0x18>;
interrupts = <25>;
};
watchdog@f0000fc0 {
compatible = "cnxt,cx92755-wdt";
reg = <0xf0000fc0 0x8>;
clocks = <&main_clk>;
timeout-sec = <15>;
};
Thanks,
-Nick Hawkins
-----Original Message-----
From: Arnd Bergmann [mailto:[email protected]]
Sent: Thursday, March 31, 2022 4:53 PM
To: Hawkins, Nick <[email protected]>
Cc: Arnd Bergmann <[email protected]>; Verdun, Jean-Marie <[email protected]>; Olof Johansson <[email protected]>; [email protected]; Rob Herring <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
> I don't think you can do this, if you are using the syscon regmap, you go through the regmap indirection rather than accessing the mmio register by virtual address, and this may result in some extra code in your driver, and a little runtime overhead.
> If you prefer to avoid that, you can go back to having the timer node as the parent, but without being a syscon. In this case, the watchdog would be handled in one of these ways:
> a) a child device gets created from the clocksource driver and bound to the
watchdog driver, which then uses a private interface between the clocksource
and the watchdog to access the registers
> b) the clocksource driver itself registers as a watchdog driver, without
having a separate driver module
> One thing to consider is whether the register range here contains any registers that may be used in another driver, e.g. a second timer, a PWM, or a clk controller. If not, you are fairly free to pick any of these approaches.
I will try to use the b) approach everything in that range is timer or watchdog related. There is a second timer however there are no plans on using that. Should the combined code still live inside the driver/timer directory or should it be moved to mfd?
Thanks,
-Nick
-----Original Message-----
From: Arnd Bergmann [mailto:[email protected]]
Sent: Thursday, March 31, 2022 4:30 AM
To: Hawkins, Nick <[email protected]>
Cc: Arnd Bergmann <[email protected]>; Verdun, Jean-Marie <[email protected]>; Olof Johansson <[email protected]>; [email protected]; Rob Herring <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
On Thu, Mar 31, 2022 at 12:27 AM Hawkins, Nick <[email protected]> wrote:
> On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <[email protected]>> wrote:
>
> >> I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls in the same area.
> >
> >> For instance with our watchdog controls we have:
> >
> >> @90 the countdown value
> >> @96 the configuration
> >
> >> And for our timer we have:
> >> @80 the countdown value
> >> @94 the configuration
> >> @88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.
> >
> >> What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers.
>>
> > I think this is most commonly done using a 'syscon' node, have a
> > look at the files listed by
>>
>> I found an example and copied it although I have a couple questions when it comes to actually coding it. Can that be here or should I post these questions in the patch that actually concern the file?
>>
>> st: timer@80 {
>> compatible = "hpe,gxp-timer","syscon","simple-mfd";
>> reg = <0x80 0x16>;
>> interrupts = <0>;
>> interrupt-parent = <&vic0>;
>> clocks = <&ppuclk>;
>> clock-names = "ppuclk";
>> clock-frequency = <400000000>;
>>
>> watchdog {
>> compatible = "hpe,gxp-wdt";
>> };
>> };
> I'd have to study the other examples myself to see what is most common.
> My feeling would be that it's better to either have a "hpe,gxp-timer" parent device with a watchdog child but no syscon, or to have a syscon/simple-mfd parent with both the timer and the watchdog as children.
Arnd, thanks for the feedback. I am trying to use the approach you recommend where you have a syscon/simple-mfd parent with watchdog and timer as children.
st: chip-controller@80 {
compatible = "hpe,gxp-ctrl-st","syscon","simple-mfd";
reg = <0x80 0x16>;
timer0: timer {
compatible = "hpe,gxp-timer";
interrupts = <0>;
interrupt-parent = <&vic0>;
clocks = <&ppuclk>;
clock-names = "ppuclk";
};
watchdog {
compatible = "hpe,gxp-wdt";
};
};
This compiles without any errors but I do have some questions about accessing the regmap in both drivers, specifically the timer code. How do you use a regmap with clocksource_mmio_init? I tried searching through the codebase and could not find the answer. I assume I am missing some vital step.
Thanks,
-Nick
-----Original Message-----
From: Arnd Bergmann [mailto:[email protected]]
Sent: Monday, April 4, 2022 5:02 PM
To: Hawkins, Nick <[email protected]>>
Cc: Arnd Bergmann <[email protected]>>; Verdun, Jean-Marie <[email protected]>>; Olof Johansson <[email protected]>>; [email protected]; Rob Herring <[email protected]>>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
On Mon, Apr 4, 2022 at 10:22 PM Hawkins, Nick <[email protected]>> wrote:
>>>> I would put it into drivers/clocksource/, I don't think drivers/mtd would be any better, but there is a chance that the clocksource maintainers don't want to have the watchdog code in their tree.
>>
>> While trying to discover how to creating two devices in one driver I ran across an interesting .dtsi and I was wondering if this would be a valid approach for my situation as well. The pertinent files are:
>> 1) drivers/clocksource/timer-digicolor.c
>> 2) arch/arm/boot/dts/cx92755.dtsi
>> 3) drivers/watchdog/digicolor_wdt.c
>>
>> Here they are just sharing the same register area:
>>
>> timer@f0000fc0 {
>> compatible = "cnxt,cx92755-timer";
>> reg = <0xf0000fc0 0x40>>;
>> interrupts = <19>>, <31>>, <34>>, <35>>, <52>>, <53>>, <54>>, <55>>;
>> clocks = <&main_clk>>;
>> };
>>
>> rtc@f0000c30 {
>> compatible = "cnxt,cx92755-rtc";
>> reg = <0xf0000c30 0x18>>;
>> interrupts = <25>>;
>> };
>>
>> watchdog@f0000fc0 {
>> compatible = "cnxt,cx92755-wdt";
>> reg = <0xf0000fc0 0x8>>;
>> clocks = <&main_clk>>;
>> timeout-sec = <15>>;
>> };
> Right, it is possible to make this work, but it's not recommended, and you have to work around the sanity checks in the code that try to keep you from doing it wrong, as well as any tooling that tries to check for these in the DT.
I found an example in the kernel where the timer creates a child watchdog device and passes it the base address when creating it. I used this to model the gxp-timer and gxp-wdt. The following files were what I have referenced:
drivers/watchdog/ixp4xx_wdt.c
drivers/clocksource/timer-ixp4xx.c
This seems very similar to what you suggested previously except I do not see a private interface in there between the parent and the child device. Is it mandatory to have the private interface between the two? If it is, what would you recommend that interface be? So far without the private interface I am not seeing any issues accessing the registers.
Thanks,
-Nick
On Tue, Apr 5, 2022 at 11:21 PM Hawkins, Nick <[email protected]> wrote:
>
> > Right, it is possible to make this work, but it's not recommended, and you have to work around the sanity checks in the code that try to keep you from doing it wrong, as well as any tooling that tries to check for these in the DT.
>
> I found an example in the kernel where the timer creates a child watchdog device and passes it the base address when creating it. I used this to model the gxp-timer and gxp-wdt. The following files were what I have referenced:
> drivers/watchdog/ixp4xx_wdt.c
> drivers/clocksource/timer-ixp4xx.c
Yes, I think that is a good example.
> This seems very similar to what you suggested previously except I do not see a private interface in there between the parent and the child device. Is it mandatory to have the private interface between the two? If it is, what would you recommend that interface be? So far without the private interface I am not seeing any issues accessing the registers.
I would count passing a register address to the child device as a
private interface.
It's a minimalistic one, but that is not a bad thing here.
Arnd
-----Original Message-----
From: Arnd Bergmann [mailto:[email protected]]
Sent: Wednesday, April 6, 2022 2:25 AM
To: Hawkins, Nick <[email protected]>
Cc: Arnd Bergmann <[email protected]>; Verdun, Jean-Marie <[email protected]>; Olof Johansson <[email protected]>; [email protected]; Rob Herring <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
> I would count passing a register address to the child device as a private interface.
> It's a minimalistic one, but that is not a bad thing here.
Thank you. Now that the parent/child issue with timer/watchdog is resolved I am now having an issue trying to access "iopclk" from "gxp-timer". I have tried use of of_clk_get_by_name and of_clk_get which both fail to find the clock.
Is it because clocks is outside of the axi -> ahb hierarchy?
clocks {
pll: pll {
compatible = "fixed-clock";
#clock-cells = <0>;
clock-frequency = <1600000000>;
};
iopclk: iopclk {
compatible = "fixed-factor-clock";
#clock-cells = <0>;
clock-div = <4>;
clock-mult = <4>;
clocks = <&pll>;
};
};
axi {
compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
ranges;
dma-ranges;
ahb@c0000000 {
compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0xc0000000 0x30000000>;
...
st: timer@80 {
compatible = "hpe,gxp-timer","simple-mfd";
reg = <0x80 0x16>;
interrupts = <0>;
interrupt-parent = <&vic0>;
clocks = <&iopclk>;
clock-names = "iopclk";
watchdog {
compatible = "hpe,gxp-wdt";
};
};
};
};
Thanks,
-Nick Hawkins
On Wed, Apr 13, 2022 at 6:48 PM Hawkins, Nick <[email protected]> wrote:
> > I would count passing a register address to the child device as a private interface.
> > It's a minimalistic one, but that is not a bad thing here.
>
> Thank you. Now that the parent/child issue with timer/watchdog is resolved I am now
> having an issue trying to access "iopclk" from "gxp-timer". I have tried use of
> of_clk_get_by_name and of_clk_get which both fail to find the clock.
> Is it because clocks is outside of the axi -> ahb hierarchy?
No, that should work, but you have to pass the right device node, which would
correspond to the parent device. You should also be able to pass the parent
device pointer (i.e. linux device, not device_node) and use clk_get().
Arnd