| Message ID | 20220310195229.109477-9-nick.hawkins@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: Introduce HPE GXP Architecture | expand |
On Thu, Mar 10, 2022 at 8:52 PM <nick.hawkins@hpe.com> wrote: > > From: Nick Hawkins <nick.hawkins@hpe.com> > > 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 <nick.hawkins@hpe.com> > +/ { > + #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, nick.hawkins@hpe.com wrote: > From: Nick Hawkins <nick.hawkins@hpe.com> > > 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 <nick.hawkins@hpe.com> > --- > 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
-----Original Message----- From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com] Sent: Friday, March 11, 2022 4:30 AM To: Hawkins, Nick <nick.hawkins@hpe.com>>; Verdun, Jean-Marie <verdun@hpe.com>> Cc: Arnd Bergmann <arnd@arndb.de>>; Olof Johansson <olof@lixom.net>>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree On 10/03/2022 20:52, nick.hawkins@hpe.com wrote: >> From: Nick Hawkins <nick.hawkins@hpe.com>> >> >> 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 <nick.hawkins@hpe.com>> >> --- >> 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
On 16/03/2022 16:41, Hawkins, Nick wrote: > -----Original Message----- > From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com] > Sent: Friday, March 11, 2022 4:30 AM > To: Hawkins, Nick <nick.hawkins@hpe.com>>; Verdun, Jean-Marie <verdun@hpe.com>> > Cc: Arnd Bergmann <arnd@arndb.de>>; Olof Johansson <olof@lixom.net>>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree > > On 10/03/2022 20:52, nick.hawkins@hpe.com wrote: >>> From: Nick Hawkins <nick.hawkins@hpe.com>> >>> >>> 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 <nick.hawkins@hpe.com>> >>> --- >>> 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:krzysztof.kozlowski@canonical.com] >> Sent: Friday, March 11, 2022 4:30 AM >> To: Hawkins, Nick <nick.hawkins@hpe.com>>>>; Verdun, Jean-Marie >> <verdun@hpe.com>>>> >> Cc: Arnd Bergmann <arnd@arndb.de>>>>; Olof Johansson <olof@lixom.net>>>>; >> soc@kernel.org; Rob Herring <robh+dt@kernel.org>>>>; >> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; >> linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP >> Device tree >> >> On 10/03/2022 20:52, nick.hawkins@hpe.com wrote: >>>>>> From: Nick Hawkins <nick.hawkins@hpe.com>>>> >>>>>> >>>>>> 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 <nick.hawkins@hpe.com>>>> >>>>>> --- >>>>>> 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
-----Original Message----- From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com] Sent: Thursday, March 17, 2022 3:37 AM To: Hawkins, Nick <nick.hawkins@hpe.com>; Verdun, Jean-Marie <verdun@hpe.com> Cc: Arnd Bergmann <arnd@arndb.de>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org 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
On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <nick.hawkins@hpe.com> 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: Arnd Bergmann [mailto:arnd@arndb.de] Sent: Tuesday, March 29, 2022 4:13 PM To: Hawkins, Nick <nick.hawkins@hpe.com> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>; Verdun, Jean-Marie <verdun@hpe.com>; Arnd Bergmann <arnd@arndb.de>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org 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 <nick.hawkins@hpe.com>> 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:arnd@arndb.de] Sent: Tuesday, March 29, 2022 4:13 PM To: Hawkins, Nick <nick.hawkins@hpe.com> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>; Verdun, Jean-Marie <verdun@hpe.com>; Arnd Bergmann <arnd@arndb.de>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org 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 <nick.hawkins@hpe.com>> 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 <nick.hawkins@hpe.com> wrote: > On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <nick.hawkins@hpe.com>> 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
-----Original Message----- From: Arnd Bergmann [mailto:arnd@arndb.de] Sent: Thursday, March 31, 2022 4:30 AM To: Hawkins, Nick <nick.hawkins@hpe.com> Cc: Arnd Bergmann <arnd@arndb.de>; Verdun, Jean-Marie <verdun@hpe.com>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org 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 <nick.hawkins@hpe.com> wrote: > On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <nick.hawkins@hpe.com>> 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
On Thu, Mar 31, 2022 at 11:09 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote: > On Thu, Mar 31, 2022 at 12:27 AM Hawkins, Nick <nick.hawkins@hpe.com> wrote: > > On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <nick.hawkins@hpe.com>> 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
-----Original Message----- From: Arnd Bergmann [mailto:arnd@arndb.de] Sent: Thursday, March 31, 2022 4:53 PM To: Hawkins, Nick <nick.hawkins@hpe.com> Cc: Arnd Bergmann <arnd@arndb.de>; Verdun, Jean-Marie <verdun@hpe.com>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org 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
On Fri, Apr 1, 2022 at 6:05 PM Hawkins, Nick <nick.hawkins@hpe.com> 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
-----Original Message----- From: Arnd Bergmann [mailto:arnd@arndb.de] Sent: Friday, April 1, 2022 11:30 AM To: Hawkins, Nick <nick.hawkins@hpe.com> Cc: Arnd Bergmann <arnd@arndb.de>; Verdun, Jean-Marie <verdun@hpe.com>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org 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 <nick.hawkins@hpe.com> 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
On Mon, Apr 4, 2022 at 10:22 PM Hawkins, Nick <nick.hawkins@hpe.com> 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:arnd@arndb.de] Sent: Monday, April 4, 2022 5:02 PM To: Hawkins, Nick <nick.hawkins@hpe.com>> Cc: Arnd Bergmann <arnd@arndb.de>>; Verdun, Jean-Marie <verdun@hpe.com>>; Olof Johansson <olof@lixom.net>>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org 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 <nick.hawkins@hpe.com>> 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 <nick.hawkins@hpe.com> 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:arnd@arndb.de] Sent: Wednesday, April 6, 2022 2:25 AM To: Hawkins, Nick <nick.hawkins@hpe.com> Cc: Arnd Bergmann <arnd@arndb.de>; Verdun, Jean-Marie <verdun@hpe.com>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org 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 <nick.hawkins@hpe.com> 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
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>; + }; + }; +};