From: Nick Hawkins <[email protected]>
The GXP is the HPE BMC SoC that is used in the majority
of HPE Generation 10 servers. Traditionally the asic will
last multiple generations of server before being replaced.
In gxp.c we reset the EHCI controller early to boot the asic.
Info about SoC:
HPE GXP is the name of the HPE Soc. This SoC is used to implement
many BMC features at HPE. It supports ARMv7 architecture based on
the Cortex A9 core. It is capable of using an AXI bus to which
a memory controller is attached. It has multiple SPI interfaces
to connect boot flash and BIOS flash. It uses a 10/100/1000 MAC
for network connectivity. It has multiple i2c engines to drive
connectivity with a host infrastructure. The initial patches
enable the watchdog and timer enabling the host to be able to
boot.
Signed-off-by: Nick Hawkins <[email protected]>
---
v5:
* Fixed version log
v4:
* Removed unecessary code: restart, iomap, init_machine
* Reordered Kconfig depends
* Removed SPARSE_IRQ, MULTI_IRQ_HANDLER, IRQ_DOMAIN, PINCTL from
Kconfig
v3:
* Put into proper patchset format
v2:
* No change
---
arch/arm/Kconfig | 2 ++
arch/arm/Makefile | 1 +
arch/arm/mach-hpe/Kconfig | 17 +++++++++++++++++
arch/arm/mach-hpe/Makefile | 1 +
arch/arm/mach-hpe/gxp.c | 16 ++++++++++++++++
5 files changed, 37 insertions(+)
create mode 100644 arch/arm/mach-hpe/Kconfig
create mode 100644 arch/arm/mach-hpe/Makefile
create mode 100644 arch/arm/mach-hpe/gxp.c
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 2e8091e2d8a8..13f77eec7c40 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -620,6 +620,8 @@ source "arch/arm/mach-highbank/Kconfig"
source "arch/arm/mach-hisi/Kconfig"
+source "arch/arm/mach-hpe/Kconfig"
+
source "arch/arm/mach-imx/Kconfig"
source "arch/arm/mach-integrator/Kconfig"
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index a2391b8de5a5..97a89023c10f 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -179,6 +179,7 @@ machine-$(CONFIG_ARCH_FOOTBRIDGE) += footbridge
machine-$(CONFIG_ARCH_GEMINI) += gemini
machine-$(CONFIG_ARCH_HIGHBANK) += highbank
machine-$(CONFIG_ARCH_HISI) += hisi
+machine-$(CONFIG_ARCH_HPE) += hpe
machine-$(CONFIG_ARCH_INTEGRATOR) += integrator
machine-$(CONFIG_ARCH_IOP32X) += iop32x
machine-$(CONFIG_ARCH_IXP4XX) += ixp4xx
diff --git a/arch/arm/mach-hpe/Kconfig b/arch/arm/mach-hpe/Kconfig
new file mode 100644
index 000000000000..c075248b259e
--- /dev/null
+++ b/arch/arm/mach-hpe/Kconfig
@@ -0,0 +1,17 @@
+menuconfig ARCH_HPE
+ bool "HPE SoC support"
+ depends on ARCH_MULTI_V7
+ help
+ This enables support for HPE ARM based SoC chips
+if ARCH_HPE
+
+config ARCH_HPE_GXP
+ bool "HPE GXP SoC"
+ depends on ARCH_MULTI_V7
+ select ARM_VIC
+ select GENERIC_IRQ_CHIP
+ select CLKSRC_MMIO
+ help
+ Support for GXP SoCs
+
+endif
diff --git a/arch/arm/mach-hpe/Makefile b/arch/arm/mach-hpe/Makefile
new file mode 100644
index 000000000000..8b0a91234df4
--- /dev/null
+++ b/arch/arm/mach-hpe/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_ARCH_HPE_GXP) += gxp.o
diff --git a/arch/arm/mach-hpe/gxp.c b/arch/arm/mach-hpe/gxp.c
new file mode 100644
index 000000000000..e2f0c3ae6bd8
--- /dev/null
+++ b/arch/arm/mach-hpe/gxp.c
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P.*/
+
+#include <linux/of_platform.h>
+#include <asm/mach/arch.h>
+
+static const char * const gxp_board_dt_compat[] = {
+ "hpe,gxp",
+ NULL,
+};
+
+DT_MACHINE_START(GXP_DT, "HPE GXP")
+ .dt_compat = gxp_board_dt_compat,
+ .l2c_aux_val = 0,
+ .l2c_aux_mask = 0,
+MACHINE_END
--
2.17.1
From: Nick Hawkins <[email protected]>
This adds support for the hpe,gxp binding. The GXP is based on
the cortex a9 processor and supports arm7.
Signed-off-by: Nick Hawkins <[email protected]>
---
v5:
* Fix version log
v4:
* Removed gxp.yaml
* Created hpe,gxp.yaml based on reviewer input
v3:
* Created gxp.yaml
v2:
* No change
---
.../devicetree/bindings/arm/hpe,gxp.yaml | 22 +++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/hpe,gxp.yaml
diff --git a/Documentation/devicetree/bindings/arm/hpe,gxp.yaml b/Documentation/devicetree/bindings/arm/hpe,gxp.yaml
new file mode 100644
index 000000000000..cd86b67ea207
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/hpe,gxp.yaml
@@ -0,0 +1,22 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/hpe,gxp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE BMC GXP SoC driver
+
+maintainers:
+ - Nick Hawkins <[email protected]>
+ - Jean-Marie Verdun <[email protected]>
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - hpe,gxp-dl360gen10
+ - const: hpe,gxp
+
+additionalProperties: true
+
+...
--
2.17.1
From: Nick Hawkins <[email protected]>
Add support for the HPE GXP SOC timer. The GXP supports several
different kinds of timers but for the purpose of this driver there
is only support for the General Timer. The timer has a 1us
resolution and is 32 bits. The timer also creates a child watchdog
device as the register region is the same based on previous review
feedback.
Signed-off-by: Nick Hawkins <[email protected]>
---
v5:
* Corrected version log
* Removed uncessary include file
v4:
* Made watchdog a child of timer as they share the same register
region
* Fixed watchdog init timeout call
* Fixed variable usage u32/u64
* Removed Read Once
* fixed error that should have been debug
v3:
* Put into proper patchset form
v2:
* No change
---
drivers/clocksource/Kconfig | 8 ++
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-gxp.c | 182 ++++++++++++++++++++++++++++++++
3 files changed, 191 insertions(+)
create mode 100644 drivers/clocksource/timer-gxp.c
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 1589ae7d5abb..110dd10b32f2 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -617,6 +617,14 @@ config CLKSRC_ST_LPC
Enable this option to use the Low Power controller timer
as clocksource.
+config GXP_TIMER
+ bool "GXP timer driver" if COMPILE_TEST
+ depends on ARCH_HPE
+ default y
+ help
+ Provides a driver for the timer control found on HPE
+ GXP SOCs. This is required for all GXP SOCs.
+
config RISCV_TIMER
bool "Timer for the RISC-V platform" if COMPILE_TEST
depends on GENERIC_SCHED_CLOCK && RISCV && RISCV_SBI
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 9c85ee2bb373..98017abf6c03 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -88,3 +88,4 @@ obj-$(CONFIG_GX6605S_TIMER) += timer-gx6605s.o
obj-$(CONFIG_HYPERV_TIMER) += hyperv_timer.o
obj-$(CONFIG_MICROCHIP_PIT64B) += timer-microchip-pit64b.o
obj-$(CONFIG_MSC313E_TIMER) += timer-msc313e.o
+obj-$(CONFIG_GXP_TIMER) += timer-gxp.o
diff --git a/drivers/clocksource/timer-gxp.c b/drivers/clocksource/timer-gxp.c
new file mode 100644
index 000000000000..c9290fb8131c
--- /dev/null
+++ b/drivers/clocksource/timer-gxp.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P.*/
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/sched_clock.h>
+
+#define TIMER0_FREQ 1000000
+#define GXP_TIMER_CNT_OFS 0x00
+#define GXP_TIMESTAMP_OFS 0x08
+#define GXP_TIMER_CTRL_OFS 0x14
+
+/*TCS Stands for Timer Control/Status: these are masks to be used in*/
+/* the Timer Count Registers */
+#define MASK_TCS_ENABLE 0x01
+#define MASK_TCS_PERIOD 0x02
+#define MASK_TCS_RELOAD 0x04
+#define MASK_TCS_TC 0x80
+
+struct gxp_timer {
+ void __iomem *counter;
+ void __iomem *control;
+ struct clock_event_device evt;
+};
+
+static struct gxp_timer *local_gxp_timer;
+
+static void __iomem *system_clock __read_mostly;
+
+static inline struct gxp_timer *to_gxp_timer(struct clock_event_device *evt_dev)
+{
+ return container_of(evt_dev, struct gxp_timer, evt);
+}
+
+static u64 notrace gxp_sched_read(void)
+{
+ return readl_relaxed(system_clock);
+}
+
+static int gxp_time_set_next_event(unsigned long event, struct clock_event_device *evt_dev)
+{
+ struct gxp_timer *timer = to_gxp_timer(evt_dev);
+
+ /* Stop counting and disable interrupt before updating */
+ writeb_relaxed(MASK_TCS_TC, timer->control);
+ writel_relaxed(event, timer->counter);
+ writeb_relaxed(MASK_TCS_TC | MASK_TCS_ENABLE, timer->control);
+
+ return 0;
+}
+
+static irqreturn_t gxp_timer_interrupt(int irq, void *dev_id)
+{
+ struct gxp_timer *timer = (struct gxp_timer *)dev_id;
+
+ if (!(readb_relaxed(timer->control) & MASK_TCS_TC))
+ return IRQ_NONE;
+
+ writeb_relaxed(MASK_TCS_TC, timer->control);
+
+ timer->evt.event_handler(&timer->evt);
+
+ return IRQ_HANDLED;
+}
+
+static int __init gxp_timer_init(struct device_node *node)
+{
+ void __iomem *base;
+ struct clk *clk;
+ u32 freq;
+ int ret, irq;
+ struct gxp_timer *gxp_timer;
+
+ base = of_iomap(node, 0);
+ if (!base) {
+ pr_err("Can't remap timer base register");
+ ret = -ENXIO;
+ return ret;
+ }
+
+ /*Set the offset to the clock register*/
+ system_clock = base + GXP_TIMESTAMP_OFS;
+
+ clk = of_clk_get(node, 0);
+ if (IS_ERR(clk)) {
+ pr_err("%pOFn clock not found: %d\n", node, (int)PTR_ERR(clk));
+ ret = -EIO;
+ goto err_iounmap;
+ }
+
+ ret = clk_prepare_enable(clk);
+
+ freq = clk_get_rate(clk);
+
+ sched_clock_register(gxp_sched_read, 32, freq);
+ clocksource_mmio_init(system_clock, node->name, freq,
+ 300, 32, clocksource_mmio_readl_up);
+
+ irq = irq_of_parse_and_map(node, 0);
+ if (irq <= 0) {
+ ret = -EINVAL;
+ pr_err("GXP Timer Can't parse IRQ %d", irq);
+ goto err_iounmap;
+ }
+
+ gxp_timer = kzalloc(sizeof(*gxp_timer), GFP_KERNEL);
+ if (!gxp_timer) {
+ ret = -ENOMEM;
+ goto err_iounmap;
+ }
+
+ gxp_timer->counter = base + GXP_TIMER_CNT_OFS;
+ gxp_timer->control = base + GXP_TIMER_CTRL_OFS;
+ gxp_timer->evt.name = node->name;
+ gxp_timer->evt.rating = 300;
+ gxp_timer->evt.features = CLOCK_EVT_FEAT_ONESHOT;
+ gxp_timer->evt.set_next_event = gxp_time_set_next_event;
+ gxp_timer->evt.cpumask = cpumask_of(0);
+
+ local_gxp_timer = gxp_timer;
+
+ ret = request_irq(irq, gxp_timer_interrupt, IRQF_TIMER | IRQF_SHARED,
+ node->name, gxp_timer);
+ if (ret) {
+ pr_err("%s: request_irq() failed %pe\n", "GXP Timer Tick", ERR_PTR(ret));
+ goto err_iounmap;
+ }
+
+ clockevents_config_and_register(&gxp_timer->evt, TIMER0_FREQ,
+ 0xf, 0xffffffff);
+
+ pr_debug("gxp: system timer (irq = %d)\n", irq);
+ return 0;
+
+err_iounmap:
+ iounmap(system_clock);
+ iounmap(base);
+ return ret;
+}
+
+static struct platform_device gxp_watchdog_device = {
+ .name = "gxp-wdt",
+ .id = -1,
+};
+
+/*
+ * This probe gets called after the timer is already up and running. This will create
+ * the watchdog device as a child since the registers are shared.
+ */
+
+static int gxp_timer_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+
+ /* Pass the base address (counter) as platform data and nothing else */
+ gxp_watchdog_device.dev.platform_data = local_gxp_timer->counter;
+ gxp_watchdog_device.dev.parent = dev;
+ return platform_device_register(&gxp_watchdog_device);
+}
+
+static const struct of_device_id gxp_timer_of_match[] = {
+ { .compatible = "hpe,gxp-timer", },
+ {},
+};
+
+static struct platform_driver gxp_timer_driver = {
+ .probe = gxp_timer_probe,
+ .driver = {
+ .name = "gxp-timer",
+ .of_match_table = gxp_timer_of_match,
+ .suppress_bind_attrs = true,
+ },
+};
+
+builtin_platform_driver(gxp_timer_driver);
+
+TIMER_OF_DECLARE(gxp, "hpe,gxp-timer", gxp_timer_init);
--
2.17.1
From: Nick Hawkins <[email protected]>
Create a section in MAINTAINERS for the GXP HPE
architecture.
Signed-off-by: Nick Hawkins <[email protected]>
---
v5:
* Fixed commit message to list all previous changes
v4:
* Added ARM/ before HPE Title
* Changed MAINTAINED to Maintained
* Renamed gxp-timer.c to timer-gxp.c
* Renamed gxp.yaml to hpe,gxp.yaml
v3:
* Removed uncessary files
* Used proper patch-set format
v2:
* Fixed email address
* Removed multiple entries in favor of one
---
MAINTAINERS | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 40fa1955ca3f..b3fdedc07791 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2130,6 +2130,19 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/kristoffer/linux-hpc.git
F: arch/arm/mach-sa1100/include/mach/jornada720.h
F: arch/arm/mach-sa1100/jornada720.c
+ARM/HPE GXP ARCHITECTURE
+M: Jean-Marie Verdun <[email protected]>
+M: Nick Hawkins <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/arm/hpe,gxp.yaml
+F: Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
+F: Documentation/devicetree/bindings/wdt/hpe,gxp-wdt.yaml
+F: arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
+F: arch/arm/boot/dts/hpe-gxp.dtsi
+F: arch/arm/mach-hpe/gxp.c
+F: drivers/clocksource/timer-gxp.c
+F: drivers/watchdog/gxp-wdt.c
+
ARM/IGEP MACHINE SUPPORT
M: Enric Balletbo i Serra <[email protected]>
M: Javier Martinez Canillas <[email protected]>
--
2.17.1
From: Nick Hawkins <[email protected]>
Add the hpe gxp watchdog timer binding hpe,gxp-wdt.
This will enable support for the HPE GXP Watchdog.
Signed-off-by: Nick Hawkins <[email protected]>
---
v5:
* Fixed version log
v4:
* Made watchdog a child of timer because of same register
area based on review feedback
* Simplified the watchdog yaml as it will get information
from parent device
v3:
* Used proper patchset format.
v2:
* Converted from txt to yaml
---
.../bindings/watchdog/hpe,gxp-wdt.yaml | 30 +++++++++++++++++++
1 file changed, 30 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
diff --git a/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
new file mode 100644
index 000000000000..c20da146352f
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
@@ -0,0 +1,30 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/hpe,gxp-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE GXP Controlled Watchdog
+
+allOf:
+ - $ref: "watchdog.yaml#"
+
+maintainers:
+ - Nick Hawkins <[email protected]>
+ - Jean-Marie Verdun <[email protected]>
+
+properties:
+ compatible:
+ const: hpe,gxp-wdt
+
+required:
+ - compatible
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ watchdog0: watchdog {
+ compatible = "hpe,gxp-wdt";
+ };
+
--
2.17.1
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.
The dts file is empty at this point but will be
updated in subsequent updates as board specific features
are enabled.
Signed-off-by: Nick Hawkins <[email protected]>
---
v5:
* Fixed commit message to show previous changes
* Fixed typo ehci -> echi
v4:
* Removed hpe,gxp-cpu-init as it was no longer necessary
* Removed bootargs as requested
* Removed empty ahb node
* Moved reg after compatible, everywhere
* Removed osc and memclk
* Removed syscon@c00000f8 as it was not necessary for boot
* Fixed Alphabetical issue in dts/Makefile
* Added specific board binding for dl360gen10
* Removed empty node
* Added Accurate Clock Architecture
* Fixed generic-echi and generic-ochi issues
* Removed i2cg
v3:
* Fixed issues with warnings
* Used proper patchset format
v2:
* Reduced size of dtsi to essential components
* Followed the proper format for having a dtsi and
dts
---
arch/arm/boot/dts/Makefile | 2 +
arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 13 +++
arch/arm/boot/dts/hpe-gxp.dtsi | 128 +++++++++++++++++++++++
3 files changed, 143 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 7c16f8a2b738..293717719c70 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -255,6 +255,8 @@ dtb-$(CONFIG_ARCH_HISI) += \
hi3519-demb.dtb
dtb-$(CONFIG_ARCH_HIX5HD2) += \
hisi-x5hd2-dkb.dtb
+dtb-$(CONFIG_ARCH_HPE_GXP) += \
+ hpe-bmc-dl360gen10.dtb
dtb-$(CONFIG_ARCH_INTEGRATOR) += \
integratorap.dtb \
integratorap-im-pd1.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..69e9c6672ea8
--- /dev/null
+++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
@@ -0,0 +1,13 @@
+// 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-dl360gen10","hpe,gxp";
+ model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
+};
diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
new file mode 100644
index 000000000000..a3a082d21101
--- /dev/null
+++ b/arch/arm/boot/dts/hpe-gxp.dtsi
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree file for HPE GXP
+ */
+
+/dts-v1/;
+/ {
+ model = "Hewlett Packard Enterprise GXP BMC";
+ compatible = "hpe,gxp","hpe,gxp-dl360gen10";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ cpus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ cpu@0 {
+ compatible = "arm,cortex-a9";
+ reg = <0>;
+ device_type = "cpu";
+ };
+ };
+
+ 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 = <1>;
+ clocks = <&pll>;
+ };
+ };
+
+ memory@40000000 {
+ device_type = "memory";
+ reg = <0x40000000 0x20000000>;
+ };
+
+ 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>;
+
+ vic0: interrupt-controller@eff0000 {
+ compatible = "arm,pl192-vic";
+ reg = <0xeff0000 0x1000>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ };
+
+ vic1: interrupt-controller@80f00000 {
+ compatible = "arm,pl192-vic";
+ reg = <0x80f00000 0x1000>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ };
+
+ uarta: serial@e0 {
+ compatible = "ns16550a";
+ reg = <0xe0 0x8>;
+ interrupts = <17>;
+ interrupt-parent = <&vic0>;
+ clock-frequency = <1846153>;
+ reg-shift = <0>;
+ };
+
+ uartb: serial@e8 {
+ compatible = "ns16550a";
+ reg = <0xe8 0x8>;
+ interrupts = <18>;
+ interrupt-parent = <&vic0>;
+ clock-frequency = <1846153>;
+ reg-shift = <0>;
+ };
+
+ uartc: serial@f0 {
+ compatible = "ns16550a";
+ reg = <0xf0 0x8>;
+ interrupts = <19>;
+ interrupt-parent = <&vic0>;
+ clock-frequency = <1846153>;
+ reg-shift = <0>;
+ };
+
+ usb0: usb@efe0000 {
+ compatible = "hpe,gxp-ehci","generic-ehci";
+ reg = <0xefe0000 0x100>;
+ interrupts = <7>;
+ interrupt-parent = <&vic0>;
+ };
+
+ 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";
+ };
+ };
+
+ usb1: usb@efe0100 {
+ compatible = "hpe,gxp-ohci","generic-ohci";
+ reg = <0xefe0100 0x110>;
+ interrupts = <6>;
+ interrupt-parent = <&vic0>;
+ };
+ };
+ };
+
+};
--
2.17.1
From: Nick Hawkins <[email protected]>
Creating binding for gxp timer in device tree hpe,gxp-timer
Although there are multiple timers on the SoC we are only
enabling one at this time.
Signed-off-by: Nick Hawkins <[email protected]>
---
v5:
* Fix versioning
* Fixed typo time -> timer
v4:
* Made watchdog a child of timer
* Added reference clock
v3:
* Removed maintainer change from patch
* Verified there was no compilation errors
* Added reference code in separate patch of patchset
v2:
* Converted from txt to yaml
---
.../bindings/timer/hpe,gxp-timer.yaml | 49 +++++++++++++++++++
1 file changed, 49 insertions(+)
create mode 100644 Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
diff --git a/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
new file mode 100644
index 000000000000..a4572be8d89a
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/hpe,gxp-timer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE GXP TIMER
+
+maintainers:
+ - Nick Hawkins <[email protected]>
+ - Jean-Marie Verdun <[email protected]>
+
+properties:
+ compatible:
+ items:
+ - const: hpe,gxp-timer
+ - const: simple-mfd
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ clock-names:
+ const: iopclk
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+
+additionalProperties: true
+
+examples:
+ - |
+ timer0: timer@c0000000 {
+ compatible = "hpe,gxp-timer","simple-mfd";
+ reg = <0x80 0x16>;
+ interrupts = <0>;
+ interrupt-parent = <&vic0>;
+ clocks = <&iopclk>;
+ clock-names = "iopclk";
+ };
--
2.17.1
From: Nick Hawkins <[email protected]>
Add hpe,gxp-ohci to the generic-ohci list. This is to
enable the device tree support.
Signed-off-by: Nick Hawkins <[email protected]>
---
v5:
* Fixed version log
* Fixed typo ochi -> ohci
v4:
* Based on previous feedback the hpe,gxp-ohci is added
to the list
v3:
* No change
v2:
* No change
---
Documentation/devicetree/bindings/usb/generic-ohci.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/usb/generic-ohci.yaml b/Documentation/devicetree/bindings/usb/generic-ohci.yaml
index acbf94fa5f74..e2ac84665316 100644
--- a/Documentation/devicetree/bindings/usb/generic-ohci.yaml
+++ b/Documentation/devicetree/bindings/usb/generic-ohci.yaml
@@ -42,6 +42,7 @@ properties:
- brcm,bcm7420-ohci
- brcm,bcm7425-ohci
- brcm,bcm7435-ohci
+ - hpe,gxp-ohci
- ibm,476gtr-ohci
- ingenic,jz4740-ohci
- snps,hsdk-v1.0-ohci
--
2.17.1
From: Nick Hawkins <[email protected]>
Add support for the HPE GXP Processor by modifing the
multi_v7_defconfig instead. This adds basic HPE and GXP
architecture as well as enables the watchdog which is part
of this patch set.
Signed-off-by: Nick Hawkins <[email protected]>
---
v5:
* Fix version log
v4:
* No change
v3:
* Put into proper patch format
* Modified multi_v7_defconfig instead of created gxp_defconfig
v2:
* Created gxp_defconfig
---
arch/arm/configs/multi_v7_defconfig | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 6e0c8c19b35c..d44988a708a1 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -1056,6 +1056,8 @@ CONFIG_QCOM_SOCINFO=m
CONFIG_QCOM_STATS=m
CONFIG_QCOM_WCNSS_CTRL=m
CONFIG_ARCH_EMEV2=y
+CONFIG_ARCH_HPE=y
+CONFIG_ARCH_HPE_GXP=y
CONFIG_ARCH_R8A7794=y
CONFIG_ARCH_R8A7779=y
CONFIG_ARCH_R8A7790=y
@@ -1228,3 +1230,4 @@ CONFIG_CMA_SIZE_MBYTES=64
CONFIG_PRINTK_TIME=y
CONFIG_MAGIC_SYSRQ=y
CONFIG_DEBUG_FS=y
+CONFIG_GXP_WATCHDOG=y
--
2.17.1
On Thu, Apr 21, 2022 at 9:21 PM <[email protected]> wrote:
> +
> +static struct platform_device gxp_watchdog_device = {
> + .name = "gxp-wdt",
> + .id = -1,
> +};
> +/*
> + * This probe gets called after the timer is already up and running. This will create
> + * the watchdog device as a child since the registers are shared.
> + */
> +
> +static int gxp_timer_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> +
> + /* Pass the base address (counter) as platform data and nothing else */
> + gxp_watchdog_device.dev.platform_data = local_gxp_timer->counter;
> + gxp_watchdog_device.dev.parent = dev;
> + return platform_device_register(&gxp_watchdog_device);
> +}
I don't understand what this is about: the device should be created from
DT, not defined statically in the code. There are multiple ways of creating
a platform_device from a DT node, or you can allocate one here, but static
definitions are generally a mistake.
I see that you copied this from the ixp4xx driver, so I think we should fix this
there as well.
Arnd
On Thu, Apr 21 2022 at 14:21, nick hawkins wrote:
> +
> +static struct gxp_timer *local_gxp_timer;
What is local about that time?
> +static void __iomem *system_clock __read_mostly;
__ro_after_init perhaps?
> +static inline struct gxp_timer *to_gxp_timer(struct clock_event_device *evt_dev)
> +{
> + return container_of(evt_dev, struct gxp_timer, evt);
> +}
> +
> +static u64 notrace gxp_sched_read(void)
> +{
> + return readl_relaxed(system_clock);
> +}
> +
> +static int gxp_time_set_next_event(unsigned long event, struct clock_event_device *evt_dev)
Stray TAB in arguments
> +static int __init gxp_timer_init(struct device_node *node)
> +{
> + void __iomem *base;
> + struct clk *clk;
> + u32 freq;
> + int ret, irq;
> + struct gxp_timer *gxp_timer;
> +
> + base = of_iomap(node, 0);
> + if (!base) {
> + pr_err("Can't remap timer base register");
> + ret = -ENXIO;
> + return ret;
> + }
> +
> + /*Set the offset to the clock register*/
> + system_clock = base + GXP_TIMESTAMP_OFS;
> +
> + clk = of_clk_get(node, 0);
> + if (IS_ERR(clk)) {
> + pr_err("%pOFn clock not found: %d\n", node, (int)PTR_ERR(clk));
> + ret = -EIO;
> + goto err_iounmap;
> + }
> +
> + ret = clk_prepare_enable(clk);
> +
> + freq = clk_get_rate(clk);
> +
> + sched_clock_register(gxp_sched_read, 32, freq);
> + clocksource_mmio_init(system_clock, node->name, freq,
> + 300, 32, clocksource_mmio_readl_up);
So this registers sched clock and clocksource and if one of the
following fails....
> + irq = irq_of_parse_and_map(node, 0);
> + if (irq <= 0) {
> + ret = -EINVAL;
> + pr_err("GXP Timer Can't parse IRQ %d", irq);
> + goto err_iounmap;
... the error path unmaps 'system_clock'
> + }
> +
> + gxp_timer = kzalloc(sizeof(*gxp_timer), GFP_KERNEL);
> + if (!gxp_timer) {
> + ret = -ENOMEM;
> + goto err_iounmap;
> + }
> +
> + gxp_timer->counter = base + GXP_TIMER_CNT_OFS;
> + gxp_timer->control = base + GXP_TIMER_CTRL_OFS;
> + gxp_timer->evt.name = node->name;
> + gxp_timer->evt.rating = 300;
> + gxp_timer->evt.features = CLOCK_EVT_FEAT_ONESHOT;
> + gxp_timer->evt.set_next_event = gxp_time_set_next_event;
> + gxp_timer->evt.cpumask = cpumask_of(0);
> +
> + local_gxp_timer = gxp_timer;
> +
> + ret = request_irq(irq, gxp_timer_interrupt, IRQF_TIMER | IRQF_SHARED,
> + node->name, gxp_timer);
> + if (ret) {
> + pr_err("%s: request_irq() failed %pe\n", "GXP Timer Tick", ERR_PTR(ret));
> + goto err_iounmap;
... and here as a bonus leaks gxp_timer.
> + }
> +
> + clockevents_config_and_register(&gxp_timer->evt, TIMER0_FREQ,
> + 0xf, 0xffffffff);
> +
> + pr_debug("gxp: system timer (irq = %d)\n", irq);
> + return 0;
> +
> +err_iounmap:
> + iounmap(system_clock);
> + iounmap(base);
> + return ret;
> +}
Thanks,
tglx
On 21/04/2022 21:21, [email protected] wrote:
> diff --git a/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> new file mode 100644
> index 000000000000..a4572be8d89a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/hpe,gxp-timer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP TIMER
s/TIMER/Timer/
> +
> +maintainers:
> + - Nick Hawkins <[email protected]>
> + - Jean-Marie Verdun <[email protected]>
> +
> +properties:
> + compatible:
> + items:
> + - const: hpe,gxp-timer
> + - const: simple-mfd
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> +
> + clock-names:
> + const: iopclk
s/iopclk/iop/
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> +
> +additionalProperties: true
This has to be false and you need to define the children (since you use
simple-mfd).
> +
> +examples:
> + - |
> + timer0: timer@c0000000 {
> + compatible = "hpe,gxp-timer","simple-mfd";
Missing space after ','.
> + reg = <0x80 0x16>;
> + interrupts = <0>;
> + interrupt-parent = <&vic0>;
> + clocks = <&iopclk>;
> + clock-names = "iopclk";
> + };
Best regards,
Krzysztof
On 21/04/2022 21:21, [email protected] wrote:
> From: Nick Hawkins <[email protected]>
>
Thank you for your patch. There is something to discuss/improve.
> +/include/ "hpe-gxp.dtsi"
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "hpe,gxp-dl360gen10","hpe,gxp";
Missing space after ','.
> + model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
> +};
> diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
> new file mode 100644
> index 000000000000..a3a082d21101
> --- /dev/null
> +++ b/arch/arm/boot/dts/hpe-gxp.dtsi
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for HPE GXP
> + */
> +
> +/dts-v1/;
> +/ {
> + model = "Hewlett Packard Enterprise GXP BMC";
> + compatible = "hpe,gxp","hpe,gxp-dl360gen10";
The same.
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + cpus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cpu@0 {
> + compatible = "arm,cortex-a9";
> + reg = <0>;
> + device_type = "cpu";
> + };
> + };
> +
> + clocks {
> +
No need for blank line.
> + pll: pll {
Generic node names, so either "clock-0" or "pll-clock"
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <1600000000>;
> + };
> +
> + iopclk: iopclk {
"clock-1" or "iop-clock"
> + compatible = "fixed-factor-clock";
> + #clock-cells = <0>;
> + clock-div = <4>;
> + clock-mult = <1>;
> + clocks = <&pll>;
> + };
> + };
(...)
> +
> + usb0: usb@efe0000 {
> + compatible = "hpe,gxp-ehci","generic-ehci";
Here and in other places - always missing a space.
> + reg = <0xefe0000 0x100>;
> + interrupts = <7>;
> + interrupt-parent = <&vic0>;
> + };
> +
> + 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";
> + };
> + };
> +
> + usb1: usb@efe0100 {
> + compatible = "hpe,gxp-ohci","generic-ohci";
> + reg = <0xefe0100 0x110>;
> + interrupts = <6>;
> + interrupt-parent = <&vic0>;
> + };
> + };
> + };
> +
No need for blank line.
> +};
Best regards,
Krzysztof
On 21/04/2022 21:21, [email protected] wrote:
> @@ -1228,3 +1230,4 @@ CONFIG_CMA_SIZE_MBYTES=64
> CONFIG_PRINTK_TIME=y
> CONFIG_MAGIC_SYSRQ=y
> CONFIG_DEBUG_FS=y
> +CONFIG_GXP_WATCHDOG=y
This does not look like in correct place. You need to add entries how
the savedefconfig would do it.
Best regards,
Krzysztof
On 21/04/2022 21:21, [email protected] wrote:
> ---
> .../bindings/watchdog/hpe,gxp-wdt.yaml | 30 +++++++++++++++++++
> 1 file changed, 30 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
>
> diff --git a/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> new file mode 100644
> index 000000000000..c20da146352f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> @@ -0,0 +1,30 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/hpe,gxp-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP Controlled Watchdog
> +
> +allOf:
> + - $ref: "watchdog.yaml#"
allOf goes after maintainers, before properties.
> +
> +maintainers:
> + - Nick Hawkins <[email protected]>
> + - Jean-Marie Verdun <[email protected]>
> +
> +properties:
> + compatible:
> + const: hpe,gxp-wdt
> +
> +required:
> + - compatible
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + watchdog0: watchdog {
Doubled space.
> + compatible = "hpe,gxp-wdt";
> + };
> +
Best regards,
Krzysztof
On 21/04/2022 21:21, [email protected] wrote:
> From: Nick Hawkins <[email protected]>
>
> The GXP is the HPE BMC SoC that is used in the majority
> of HPE Generation 10 servers. Traditionally the asic will
> last multiple generations of server before being replaced.
> In gxp.c we reset the EHCI controller early to boot the asic.
>
> Info about SoC:
Half of your patches did not make it to the lists. For example
linux-arm-kernel has only 1, 2 and 10.
Where is the rest? All your patches must be sent to linux-arm-kernel and
linux-kernel. Unless the list of people To/Cc is too big (~ 10 people),
entire patchset should be send to same addresses.
Best regards,
Krzysztof
On 21/04/2022 21:21, [email protected] wrote:
> From: Nick Hawkins <[email protected]>
>
> This adds support for the hpe,gxp binding.
Just "Add support for HPE GXP".
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> The GXP is based on
> the cortex a9 processor and supports arm7.
>
> Signed-off-by: Nick Hawkins <[email protected]>
>
> ---
> v5:
> * Fix version log
> v4:
> * Removed gxp.yaml
> * Created hpe,gxp.yaml based on reviewer input
> v3:
> * Created gxp.yaml
> v2:
> * No change
> ---
> .../devicetree/bindings/arm/hpe,gxp.yaml | 22 +++++++++++++++++++
> 1 file changed, 22 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/hpe,gxp.yaml
>
> diff --git a/Documentation/devicetree/bindings/arm/hpe,gxp.yaml b/Documentation/devicetree/bindings/arm/hpe,gxp.yaml
> new file mode 100644
> index 000000000000..cd86b67ea207
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/hpe,gxp.yaml
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/hpe,gxp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE BMC GXP SoC driver
This is not a SoC driver anymore, so instead maybe:
"HPE BMC GXP SoC platforms"
or
"HPE BMC GXP platforms"
> +
> +maintainers:
> + - Nick Hawkins <[email protected]>
> + - Jean-Marie Verdun <[email protected]>
> +
> +properties:
> + compatible:
You do not allow any extension of this (no oneOf), which is fine if you
do not plan any other SoCs or SoMs. This is okay, but if there is a
chance list will grow, then you should have here oneOf, like other bindings.
> + items:
> + - enum:
> + - hpe,gxp-dl360gen10
> + - const: hpe,gxp
> +
> +additionalProperties: true
> +
> +...
Best regards,
Krzysztof
On 21/04/2022 21:21, [email protected] wrote:
> From: Nick Hawkins <[email protected]>
>
> Add hpe,gxp-ohci to the generic-ohci list. This is to
> enable the device tree support.
The last sentence is not needed, it's kind of obvious.
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 21/04/2022 21:21, [email protected] wrote:
> From: Nick Hawkins <[email protected]>
>
> Add support for the HPE GXP Processor by modifing the
s/modifing/modifying/ ?
> multi_v7_defconfig instead. This adds basic HPE and GXP
> architecture as well as enables the watchdog which is part
> of this patch set.
The commit lands in the Git and there is no relation to "patchset", so just:
"Enable HPE GXP Architecture and its watchdog for basic support for HPE
GXP SoCs."
Your subject also does not look correct at all. Please use "git log
--oneline -- " to get the idea for correct title.
Best regards,
Krzysztof
-----Original Message-----
From: Krzysztof Kozlowski [mailto:[email protected]]
Sent: Saturday, April 23, 2022 6:05 AM
To: Hawkins, Nick <[email protected]>; Verdun, Jean-Marie <[email protected]>; [email protected]; [email protected]; [email protected]
Cc: Russell King <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH v5 01/11] aach: arm: mach-hpe: Introduce the HPE GXP architecture
On 21/04/2022 21:21, [email protected] wrote:
> > From: Nick Hawkins <[email protected]>
> >
> > The GXP is the HPE BMC SoC that is used in the majority of HPE
> > Generation 10 servers. Traditionally the asic will last multiple
> > generations of server before being replaced.
> > In gxp.c we reset the EHCI controller early to boot the asic.
> >
> > Info about SoC:
> Half of your patches did not make it to the lists. For example linux-arm-kernel has only 1, 2 and 10.
> Where is the rest? All your patches must be sent to linux-arm-kernel and linux-kernel. Unless the list of people To/Cc is too big (~ 10 people), entire patchset should be send to same addresses.
Apologies, I was relying on the ".scripts/get_maintainer.pl" in a script to grab and send emails. I will ensure on the next patchset that linux-arm-kernel and linux-kernel are included in all of the emails.
Thanks for the feedback,
-Nick Hawkins
On Fri, Apr 22, 2022 at 3:16 PM Arnd Bergmann <[email protected]> wrote:
> On Thu, Apr 21, 2022 at 9:21 PM <[email protected]> wrote:
>
> > +
> > +static struct platform_device gxp_watchdog_device = {
> > + .name = "gxp-wdt",
> > + .id = -1,
> > +};
> > +/*
> > + * This probe gets called after the timer is already up and running. This will create
> > + * the watchdog device as a child since the registers are shared.
> > + */
> > +
> > +static int gxp_timer_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > +
> > + /* Pass the base address (counter) as platform data and nothing else */
> > + gxp_watchdog_device.dev.platform_data = local_gxp_timer->counter;
> > + gxp_watchdog_device.dev.parent = dev;
> > + return platform_device_register(&gxp_watchdog_device);
> > +}
>
> I don't understand what this is about: the device should be created from
> DT, not defined statically in the code. There are multiple ways of creating
> a platform_device from a DT node, or you can allocate one here, but static
> definitions are generally a mistake.
>
> I see that you copied this from the ixp4xx driver, so I think we should fix this
> there as well.
The ixp4xx driver looks like that because the register range used for
the timer and the watchdog is combined, i.e. it is a single IP block:
timer@c8005000 {
compatible = "intel,ixp4xx-timer";
reg = <0xc8005000 0x100>;
interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
};
Device tree probing does not allow two devices to probe from the same
DT node, so this was solved by letting the (less important) watchdog
be spawn as a platform device from the timer.
I don't know if double-probing for the same register range can be fixed,
but I was assuming that the one-compatible-to-one-driver assumption
was pretty hard-coded into the abstractions. Maybe it isn't?
Another way is of course to introduce an MFD. That becomes
problematic in another way: MFD abstractions are supposed to
be inbetween the resource and the devices it spawns, and with
timers/clocksources this creates a horrible special-casing since the
MFD bus (the parent may be providing e.g. an MMIO regmap)
then need to be early-populated and searched by the timer core
from TIMER_OF_DECLARE() early in boot.
So this solution was the lesser evil that I could think about.
Yours,
Linus Walleij
On Mon, Apr 25, 2022 at 10:38:08PM +0200, Linus Walleij wrote:
> On Fri, Apr 22, 2022 at 3:16 PM Arnd Bergmann <[email protected]> wrote:
[...]
> The ixp4xx driver looks like that because the register range used for
> the timer and the watchdog is combined, i.e. it is a single IP block:
>
> timer@c8005000 {
> compatible = "intel,ixp4xx-timer";
> reg = <0xc8005000 0x100>;
> interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> Device tree probing does not allow two devices to probe from the same
> DT node, so this was solved by letting the (less important) watchdog
> be spawn as a platform device from the timer.
>
> I don't know if double-probing for the same register range can be fixed,
> but I was assuming that the one-compatible-to-one-driver assumption
> was pretty hard-coded into the abstractions. Maybe it isn't?
>
> Another way is of course to introduce an MFD. That becomes
> problematic in another way: MFD abstractions are supposed to
> be inbetween the resource and the devices it spawns, and with
> timers/clocksources this creates a horrible special-casing since the
> MFD bus (the parent may be providing e.g. an MMIO regmap)
> then need to be early-populated and searched by the timer core
> from TIMER_OF_DECLARE() early in boot.
>
> So this solution was the lesser evil that I could think about.
Nuvoton NPCM platforms use yet another approach:
timer0: timer@8000 {
compatible = "nuvoton,npcm750-timer";
interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
reg = <0x8000 0x1C>;
clocks = <&clk NPCM7XX_CLK_TIMER>;
};
watchdog0: watchdog@801C {
compatible = "nuvoton,npcm750-wdt";
interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
reg = <0x801C 0x4>;
status = "disabled";
clocks = <&clk NPCM7XX_CLK_TIMER>;
};
The watchdog control register is in the same register block, but
represented by a separate DT node.
(not necessarily a recommendation, but it is another existing approach.)
Jonathan
On 21/04/2022 21:21, [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.
>
> The dts file is empty at this point but will be
> updated in subsequent updates as board specific features
> are enabled.
>
> Signed-off-by: Nick Hawkins <[email protected]>
>
> ---
> v5:
> * Fixed commit message to show previous changes
> * Fixed typo ehci -> echi
One more thought, but I am pretty sure we talked about this already:
please fix your commit subject. git log --oneline.
Best regards,
Krzysztof
On Thu, Apr 21, 2022 at 02:21:27PM -0500, [email protected] wrote:
> From: Nick Hawkins <[email protected]>
>
> Add the hpe gxp watchdog timer binding hpe,gxp-wdt.
> This will enable support for the HPE GXP Watchdog.
>
> Signed-off-by: Nick Hawkins <[email protected]>
>
> ---
> v5:
> * Fixed version log
> v4:
> * Made watchdog a child of timer because of same register
> area based on review feedback
> * Simplified the watchdog yaml as it will get information
> from parent device
> v3:
> * Used proper patchset format.
> v2:
> * Converted from txt to yaml
> ---
> .../bindings/watchdog/hpe,gxp-wdt.yaml | 30 +++++++++++++++++++
> 1 file changed, 30 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
>
> diff --git a/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> new file mode 100644
> index 000000000000..c20da146352f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> @@ -0,0 +1,30 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/hpe,gxp-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP Controlled Watchdog
> +
> +allOf:
> + - $ref: "watchdog.yaml#"
> +
> +maintainers:
> + - Nick Hawkins <[email protected]>
> + - Jean-Marie Verdun <[email protected]>
> +
> +properties:
> + compatible:
> + const: hpe,gxp-wdt
> +
> +required:
> + - compatible
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + watchdog0: watchdog {
> + compatible = "hpe,gxp-wdt";
How is this h/w controlled? I'm guessing it's part of the timer? If so,
you don't need this node. A single node can implement multiple
functions.
Rob
On Mon, Apr 25, 2022 at 10:38 PM Linus Walleij <[email protected]> wrote:
> On Fri, Apr 22, 2022 at 3:16 PM Arnd Bergmann <[email protected]> wrote:
> > On Thu, Apr 21, 2022 at 9:21 PM <[email protected]> wrote:
> >
> > > +
> > > +static struct platform_device gxp_watchdog_device = {
> > > + .name = "gxp-wdt",
> > > + .id = -1,
> > > +};
> > > +/*
> > > + * This probe gets called after the timer is already up and running. This will create
> > > + * the watchdog device as a child since the registers are shared.
> > > + */
> > > +
> > > +static int gxp_timer_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > +
> > > + /* Pass the base address (counter) as platform data and nothing else */
> > > + gxp_watchdog_device.dev.platform_data = local_gxp_timer->counter;
> > > + gxp_watchdog_device.dev.parent = dev;
> > > + return platform_device_register(&gxp_watchdog_device);
> > > +}
> >
> > I don't understand what this is about: the device should be created from
> > DT, not defined statically in the code. There are multiple ways of creating
> > a platform_device from a DT node, or you can allocate one here, but static
> > definitions are generally a mistake.
> >
> > I see that you copied this from the ixp4xx driver, so I think we should fix this
> > there as well.
>
> The ixp4xx driver looks like that because the register range used for
> the timer and the watchdog is combined, i.e. it is a single IP block:
>
> timer@c8005000 {
> compatible = "intel,ixp4xx-timer";
> reg = <0xc8005000 0x100>;
> interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> Device tree probing does not allow two devices to probe from the same
> DT node, so this was solved by letting the (less important) watchdog
> be spawn as a platform device from the timer.
>
> I don't know if double-probing for the same register range can be fixed,
> but I was assuming that the one-compatible-to-one-driver assumption
> was pretty hard-coded into the abstractions. Maybe it isn't?
Having a child device is fine, my objection was about the way
the device is created from a 'static platform_device ...' definition
rather than having the device structure allocated at probe time.
> Another way is of course to introduce an MFD. That becomes
> problematic in another way: MFD abstractions are supposed to
> be inbetween the resource and the devices it spawns, and with
> timers/clocksources this creates a horrible special-casing since the
> MFD bus (the parent may be providing e.g. an MMIO regmap)
> then need to be early-populated and searched by the timer core
> from TIMER_OF_DECLARE() early in boot.
>
> So this solution was the lesser evil that I could think about.
There are multiple ways of doing this that we already discussed
in the thread. The easiest is probably to have a child node without
custom registers in the DT and then use the DT helpers to
populate the linux devices with the correct data.
Arnd
On 26/04/2022 15:21, Hawkins, Nick wrote:
>> How is this h/w controlled? I'm guessing it's part of the timer? If so, you don't need this node. A single node can implement multiple functions.
>
> It is associated with the timer because of the shared register set. Based on feedback from Krzysztof I need to create a child node for gxp-timer. I therefore will remove this file and move gxp-wdt to the hpe,gxp-timer.yaml as a child node.
I have impression my feedback was about mapping entire address space,
not few registers of watchdog:
https://lore.kernel.org/all/[email protected]/
However later during talks it turned out that the address space is
heavily shared.
Nick,
BTW, do you see my comments in the email I linked above? This v5 makes
the same mistake. We talked about this already, so please make note of
comments you receive and implement them.
Best regards,
Krzysztof
Dear Nick,
Thank you for the patches.
Am 21.04.22 um 21:21 schrieb [email protected]:
> From: Nick Hawkins <[email protected]>
Type in the prefix: s/aach/arch/. Looking at `git log --oneline
arch/arm`, *ARM* or *arm* seems to be commonly used though.
> The GXP is the HPE BMC SoC that is used in the majority
> of HPE Generation 10 servers. Traditionally the asic will
> last multiple generations of server before being replaced.
Please mention what kind of documentation (datasheets, …) are available.
> In gxp.c we reset the EHCI controller early to boot the asic.
Why does the EHCI controller need to be reset?
> Info about SoC:
>
> HPE GXP is the name of the HPE Soc. This SoC is used to implement
> many BMC features at HPE. It supports ARMv7 architecture based on
> the Cortex A9 core. It is capable of using an AXI bus to which
> a memory controller is attached. It has multiple SPI interfaces
> to connect boot flash and BIOS flash. It uses a 10/100/1000 MAC
> for network connectivity. It has multiple i2c engines to drive
> connectivity with a host infrastructure. The initial patches
> enable the watchdog and timer enabling the host to be able to
> boot.
Maybe doe that in separate commits?
Please reflow the commit message for 75 characters per line.
> Signed-off-by: Nick Hawkins <[email protected]>
>
> ---
> v5:
> * Fixed version log
> v4:
> * Removed unecessary code: restart, iomap, init_machine
unnecessary
> * Reordered Kconfig depends
> * Removed SPARSE_IRQ, MULTI_IRQ_HANDLER, IRQ_DOMAIN, PINCTL from
> Kconfig
> v3:
> * Put into proper patchset format
> v2:
> * No change
> ---
> arch/arm/Kconfig | 2 ++
> arch/arm/Makefile | 1 +
> arch/arm/mach-hpe/Kconfig | 17 +++++++++++++++++
> arch/arm/mach-hpe/Makefile | 1 +
> arch/arm/mach-hpe/gxp.c | 16 ++++++++++++++++
> 5 files changed, 37 insertions(+)
> create mode 100644 arch/arm/mach-hpe/Kconfig
> create mode 100644 arch/arm/mach-hpe/Makefile
> create mode 100644 arch/arm/mach-hpe/gxp.c
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 2e8091e2d8a8..13f77eec7c40 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -620,6 +620,8 @@ source "arch/arm/mach-highbank/Kconfig"
>
> source "arch/arm/mach-hisi/Kconfig"
>
> +source "arch/arm/mach-hpe/Kconfig"
> +
> source "arch/arm/mach-imx/Kconfig"
>
> source "arch/arm/mach-integrator/Kconfig"
> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> index a2391b8de5a5..97a89023c10f 100644
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -179,6 +179,7 @@ machine-$(CONFIG_ARCH_FOOTBRIDGE) += footbridge
> machine-$(CONFIG_ARCH_GEMINI) += gemini
> machine-$(CONFIG_ARCH_HIGHBANK) += highbank
> machine-$(CONFIG_ARCH_HISI) += hisi
> +machine-$(CONFIG_ARCH_HPE) += hpe
> machine-$(CONFIG_ARCH_INTEGRATOR) += integrator
> machine-$(CONFIG_ARCH_IOP32X) += iop32x
> machine-$(CONFIG_ARCH_IXP4XX) += ixp4xx
> diff --git a/arch/arm/mach-hpe/Kconfig b/arch/arm/mach-hpe/Kconfig
> new file mode 100644
> index 000000000000..c075248b259e
> --- /dev/null
> +++ b/arch/arm/mach-hpe/Kconfig
> @@ -0,0 +1,17 @@
> +menuconfig ARCH_HPE
> + bool "HPE SoC support"
> + depends on ARCH_MULTI_V7
> + help
> + This enables support for HPE ARM based SoC chips
Add a dot/period at the end?
> +if ARCH_HPE
> +
> +config ARCH_HPE_GXP
> + bool "HPE GXP SoC"
> + depends on ARCH_MULTI_V7
> + select ARM_VIC
> + select GENERIC_IRQ_CHIP
> + select CLKSRC_MMIO
> + help
> + Support for GXP SoCs
Please elaborate here, maybe copying parts of the commit message, in
what servers it is used.
> +
> +endif
> diff --git a/arch/arm/mach-hpe/Makefile b/arch/arm/mach-hpe/Makefile
> new file mode 100644
> index 000000000000..8b0a91234df4
> --- /dev/null
> +++ b/arch/arm/mach-hpe/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_ARCH_HPE_GXP) += gxp.o
> diff --git a/arch/arm/mach-hpe/gxp.c b/arch/arm/mach-hpe/gxp.c
> new file mode 100644
> index 000000000000..e2f0c3ae6bd8
> --- /dev/null
> +++ b/arch/arm/mach-hpe/gxp.c
> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P.*/
Space before closing comment delimiter.
> +
> +#include <linux/of_platform.h>
> +#include <asm/mach/arch.h>
> +
> +static const char * const gxp_board_dt_compat[] = {
> + "hpe,gxp",
> + NULL,
> +};
> +
> +DT_MACHINE_START(GXP_DT, "HPE GXP")
> + .dt_compat = gxp_board_dt_compat,
> + .l2c_aux_val = 0,
> + .l2c_aux_mask = 0,
> +MACHINE_END
Where is the EHCI controller reset?
Kind regards,
Paul
-----Original Message-----
From: Krzysztof Kozlowski [mailto:[email protected]]
Sent: Tuesday, April 26, 2022 8:35 AM
To: Hawkins, Nick <[email protected]>; Rob Herring <[email protected]>
Cc: Verdun, Jean-Marie <[email protected]>; [email protected]; [email protected]; [email protected]; Wim Van Sebroeck <[email protected]>; Guenter Roeck <[email protected]>; Krzysztof Kozlowski <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5 06/11] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding
On 26/04/2022 15:21, Hawkins, Nick wrote:
>>> How is this h/w controlled? I'm guessing it's part of the timer? If so, you don't need this node. A single node can implement multiple functions.
>>
>> It is associated with the timer because of the shared register set. Based on feedback from Krzysztof I need to create a child node for gxp-timer. I therefore will remove this file and move gxp-wdt to the hpe,gxp-timer.yaml as a child node.
>I have impression my feedback was about mapping entire address space, not few registers of watchdog:
>https://lore.kernel.org/all/[email protected]/
>However later during talks it turned out that the address space is heavily shared.
>Nick,
>BTW, do you see my comments in the email I linked above? This v5 makes the same mistake. We talked about this already, so please make note of comments you receive and implement them.
Krzysztof,
Apologies, I did miss the comment about the double spacing around the label and the label not being necessary. I will not make this mistake again. I became focused about the comment of mapping an entire register space which indirectly lead me on to the path which I am now having the gxp-timer have the gxp-wdt as a child. To be specific the feedback I was speaking of above was about the gxp-timer which is here: https://lore.kernel.org/all/[email protected]/ That is the children must be defined for a simple-mfd device. Hence the plan I have now is to remove the hpe,gxp-wdt.yaml entirely and include it in the hpe,gxp-timer.yaml. I assume that is the correct thing to do?
Thank you for your time and feedback,
-Nick
-----Original Message-----
From: Rob Herring [mailto:[email protected]]
Sent: Monday, April 25, 2022 5:05 PM
To: Hawkins, Nick <[email protected]>
Cc: Verdun, Jean-Marie <[email protected]>; [email protected]; [email protected]; [email protected]; Wim Van Sebroeck <[email protected]>; Guenter Roeck <[email protected]>; Krzysztof Kozlowski <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5 06/11] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding
(...)
> How is this h/w controlled? I'm guessing it's part of the timer? If so, you don't need this node. A single node can implement multiple functions.
It is associated with the timer because of the shared register set. Based on feedback from Krzysztof I need to create a child node for gxp-timer. I therefore will remove this file and move gxp-wdt to the hpe,gxp-timer.yaml as a child node.
Thank you for the feedback.
-Nick Hawkins
On Tue, Apr 26, 2022 at 11:38 PM Rob Herring <[email protected]> wrote:
> On Tue, Apr 26, 2022 at 08:00:20AM +0200, Arnd Bergmann wrote:
> > On Mon, Apr 25, 2022 at 10:38 PM Linus Walleij <[email protected]> wrote:
> > There are multiple ways of doing this that we already discussed
> > in the thread. The easiest is probably to have a child node without
> > custom registers in the DT and then use the DT helpers to
> > populate the linux devices with the correct data.
>
> I think that's what the wdt binding is doing, but I don't like that.
> Maybe it's not a child node, I can't tell.
>
> Bindings should not be decided on the *current* driver split on one
> particular OS. This looks like 1 block, so 1 node.
Fair enough.
> If that doesn't work well or easy for Linux, then we should fix Linux.
Doing a simple platform_device_create_pdata() should work fine here,
the only problem that might exist is if the wdt driver needs access to
DT properties, as we can't have both devices refer to the same of_node
pointer, which would cause them to be picked up by the timer driver
again.
Arnd
On 26/04/2022 15:52, Hawkins, Nick wrote:
> Apologies, I did miss the comment about the double spacing around the label and the label not being necessary. I will not make this mistake again. I became focused about the comment of mapping an entire register space which indirectly lead me on to the path which I am now having the gxp-timer have the gxp-wdt as a child. To be specific the feedback I was speaking of above was about the gxp-timer which is here: https://lore.kernel.org/all/[email protected]/ That is the children must be defined for a simple-mfd device.
This was comment for this v5, not for previous patches. In this v5, you
have a child of timer, so it has to be defined in timer schema.
This was not a comment whether a child should exist or should not. It
was made under the assumption that you want to have a child node.
> Hence the plan I have now is to remove the hpe,gxp-wdt.yaml entirely and include it in the hpe,gxp-timer.yaml. I assume that is the correct thing to do?
I would follow here the advice from Rob, so since the blocks are mixed
significantly (same address space), then let's assume it's actually one
device with two functions. In such case Rob pointed out that child node
is not necessary.
The implementation might differ, depending how the features are mixed-up
with each other. It might be one driver having timer and watchdog, or
several drivers (usually bound together with a MFD driver which serves
as parents and binds to the OF node).
Best regards,
Krzysztof
-----Original Message-----
From: Paul Menzel [mailto:[email protected]]
Sent: Tuesday, April 26, 2022 3:26 AM
To: Hawkins, Nick <[email protected]>
Cc: Verdun, Jean-Marie <[email protected]>; [email protected]; [email protected]; [email protected]; Russell King <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH v5 01/11] aach: arm: mach-hpe: Introduce the HPE GXP architecture
> Type in the prefix: s/aach/arch/. Looking at `git log --oneline arch/arm`, *ARM* or *arm* seems to be commonly used though.
I will correct this, I will also look in all other patches to make sure appropriate titles are being used.
> > The GXP is the HPE BMC SoC that is used in the majority of HPE
> > Generation 10 servers. Traditionally the asic will last multiple
> > generations of server before being replaced.
> Please mention what kind of documentation (datasheets, …) are available.
Currently there are none available. The only reference I can provide will be arm documentation.
> > In gxp.c we reset the EHCI controller early to boot the asic.
> Why does the EHCI controller need to be reset?
This functionality was moved into the boot loader. This message is stale and needs to be removed. It was necessary for the chip to boot.
> > Info about SoC:
> >
> > HPE GXP is the name of the HPE Soc. This SoC is used to implement many
> > BMC features at HPE. It supports ARMv7 architecture based on the
> > Cortex A9 core. It is capable of using an AXI bus to which a memory
> > controller is attached. It has multiple SPI interfaces to connect boot
> > flash and BIOS flash. It uses a 10/100/1000 MAC for network
> > connectivity. It has multiple i2c engines to drive connectivity with a
> > host infrastructure. The initial patches enable the watchdog and timer
> > enabling the host to be able to boot.
> Maybe doe that in separate commits?
Are you asking for me to have this paragraph in the other commits? Or perhaps not mention the other patches in this paragraph?
> Please reflow the commit message for 75 characters per line.
I will verify all the lines are under 75 characters.
Thanks for the feedback,
-Nick Hawkins
On Wed, Apr 27, 2022 at 12:04 AM Rob Herring <[email protected]> wrote:
>
> On Tue, Apr 26, 2022 at 4:55 PM Arnd Bergmann <[email protected]> wrote:
> >
> > On Tue, Apr 26, 2022 at 11:38 PM Rob Herring <[email protected]> wrote:
> > > On Tue, Apr 26, 2022 at 08:00:20AM +0200, Arnd Bergmann wrote:
> > > > On Mon, Apr 25, 2022 at 10:38 PM Linus Walleij <[email protected]> wrote:
> >
> > > > There are multiple ways of doing this that we already discussed
> > > > in the thread. The easiest is probably to have a child node without
> > > > custom registers in the DT and then use the DT helpers to
> > > > populate the linux devices with the correct data.
> > >
> > > I think that's what the wdt binding is doing, but I don't like that.
> > > Maybe it's not a child node, I can't tell.
> > >
> > > Bindings should not be decided on the *current* driver split on one
> > > particular OS. This looks like 1 block, so 1 node.
> >
> > Fair enough.
> >
> > > If that doesn't work well or easy for Linux, then we should fix Linux.
> >
> > Doing a simple platform_device_create_pdata() should work fine here,
> > the only problem that might exist is if the wdt driver needs access to
> > DT properties, as we can't have both devices refer to the same of_node
> > pointer,
>
> Why not? There's even a struct device flag for that.
Ah, I forgot about ->of_node_reused, that should work then if
it needs access to properties.
Arnd
On Tue, Apr 26, 2022 at 08:00:20AM +0200, Arnd Bergmann wrote:
> On Mon, Apr 25, 2022 at 10:38 PM Linus Walleij <[email protected]> wrote:
> > On Fri, Apr 22, 2022 at 3:16 PM Arnd Bergmann <[email protected]> wrote:
> > > On Thu, Apr 21, 2022 at 9:21 PM <[email protected]> wrote:
> > >
> > > > +
> > > > +static struct platform_device gxp_watchdog_device = {
> > > > + .name = "gxp-wdt",
> > > > + .id = -1,
> > > > +};
> > > > +/*
> > > > + * This probe gets called after the timer is already up and running. This will create
> > > > + * the watchdog device as a child since the registers are shared.
> > > > + */
> > > > +
> > > > +static int gxp_timer_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct device *dev = &pdev->dev;
> > > > +
> > > > + /* Pass the base address (counter) as platform data and nothing else */
> > > > + gxp_watchdog_device.dev.platform_data = local_gxp_timer->counter;
> > > > + gxp_watchdog_device.dev.parent = dev;
> > > > + return platform_device_register(&gxp_watchdog_device);
> > > > +}
> > >
> > > I don't understand what this is about: the device should be created from
> > > DT, not defined statically in the code. There are multiple ways of creating
> > > a platform_device from a DT node, or you can allocate one here, but static
> > > definitions are generally a mistake.
> > >
> > > I see that you copied this from the ixp4xx driver, so I think we should fix this
> > > there as well.
> >
> > The ixp4xx driver looks like that because the register range used for
> > the timer and the watchdog is combined, i.e. it is a single IP block:
> >
> > timer@c8005000 {
> > compatible = "intel,ixp4xx-timer";
> > reg = <0xc8005000 0x100>;
> > interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
> > };
> >
> > Device tree probing does not allow two devices to probe from the same
> > DT node, so this was solved by letting the (less important) watchdog
> > be spawn as a platform device from the timer.
> >
> > I don't know if double-probing for the same register range can be fixed,
> > but I was assuming that the one-compatible-to-one-driver assumption
> > was pretty hard-coded into the abstractions. Maybe it isn't?
>
> Having a child device is fine, my objection was about the way
> the device is created from a 'static platform_device ...' definition
> rather than having the device structure allocated at probe time.
>
> > Another way is of course to introduce an MFD. That becomes
> > problematic in another way: MFD abstractions are supposed to
> > be inbetween the resource and the devices it spawns, and with
> > timers/clocksources this creates a horrible special-casing since the
> > MFD bus (the parent may be providing e.g. an MMIO regmap)
> > then need to be early-populated and searched by the timer core
> > from TIMER_OF_DECLARE() early in boot.
> >
> > So this solution was the lesser evil that I could think about.
>
> There are multiple ways of doing this that we already discussed
> in the thread. The easiest is probably to have a child node without
> custom registers in the DT and then use the DT helpers to
> populate the linux devices with the correct data.
I think that's what the wdt binding is doing, but I don't like that.
Maybe it's not a child node, I can't tell.
Bindings should not be decided on the *current* driver split on one
particular OS. This looks like 1 block, so 1 node. If that doesn't work
well or easy for Linux, then we should fix Linux.
Rob
Dear Nick,
Am 26.04.22 um 19:28 schrieb Hawkins, Nick:
>
>
> -----Original Message-----
> From: Paul Menzel [mailto:[email protected]]
> Sent: Tuesday, April 26, 2022 3:26 AM
> To: Hawkins, Nick <[email protected]>
> Cc: Verdun, Jean-Marie <[email protected]>; [email protected]; [email protected]; [email protected]; Russell King <[email protected]>; [email protected]; [email protected]
> Subject: Re: [PATCH v5 01/11] aach: arm: mach-hpe: Introduce the HPE GXP architecture
[OT: Maybe use an email program, that does not add an unnecessary header.]
[…]
>>> The GXP is the HPE BMC SoC that is used in the majority of HPE
>>> Generation 10 servers. Traditionally the asic will last multiple
>>> generations of server before being replaced.
>
>> Please mention what kind of documentation (datasheets, …) are available.
>
> Currently there are none available. The only reference I can provide
> will be arm documentation.
Too bad.
>>> In gxp.c we reset the EHCI controller early to boot the asic.
>
>> Why does the EHCI controller need to be reset?
> This functionality was moved into the boot loader. This message is
> stale and needs to be removed. It was necessary for the chip to
> boot.
Understood. Please mention somewhere, what bootloader is used.
>>> Info about SoC:
>>>
>>> HPE GXP is the name of the HPE Soc. This SoC is used to implement many
>>> BMC features at HPE. It supports ARMv7 architecture based on the
>>> Cortex A9 core. It is capable of using an AXI bus to which a memory
>>> controller is attached. It has multiple SPI interfaces to connect boot
>>> flash and BIOS flash. It uses a 10/100/1000 MAC for network
>>> connectivity. It has multiple i2c engines to drive connectivity with a
>>> host infrastructure. The initial patches enable the watchdog and timer
>>> enabling the host to be able to boot.
>
>> Maybe doe that in separate commits?
> Are you asking for me to have this paragraph in the other commits?
> Or perhaps not mention the other patches in this paragraph?
Yes, please move:
> The initial patches enable the watchdog and timer enabling the host
> to be able to boot.
in a cover letter for example.
>> Please reflow the commit message for 75 characters per line.
> I will verify all the lines are under 75 characters.
Please make sure the lines are as long as possible, while being at most
75 characters long.
Kind regards,
Paul
On Tue, Apr 26, 2022 at 4:55 PM Arnd Bergmann <[email protected]> wrote:
>
> On Tue, Apr 26, 2022 at 11:38 PM Rob Herring <[email protected]> wrote:
> > On Tue, Apr 26, 2022 at 08:00:20AM +0200, Arnd Bergmann wrote:
> > > On Mon, Apr 25, 2022 at 10:38 PM Linus Walleij <[email protected]> wrote:
>
> > > There are multiple ways of doing this that we already discussed
> > > in the thread. The easiest is probably to have a child node without
> > > custom registers in the DT and then use the DT helpers to
> > > populate the linux devices with the correct data.
> >
> > I think that's what the wdt binding is doing, but I don't like that.
> > Maybe it's not a child node, I can't tell.
> >
> > Bindings should not be decided on the *current* driver split on one
> > particular OS. This looks like 1 block, so 1 node.
>
> Fair enough.
>
> > If that doesn't work well or easy for Linux, then we should fix Linux.
>
> Doing a simple platform_device_create_pdata() should work fine here,
> the only problem that might exist is if the wdt driver needs access to
> DT properties, as we can't have both devices refer to the same of_node
> pointer,
Why not? There's even a struct device flag for that.
> which would cause them to be picked up by the timer driver
> again.
Huh?
That's not to say there might be some gotchas. The musb driver didn't
like sharing. But those are issues we should fix rather than
work-around in the binding.
Rob
On 21/04/2022 21:21, [email protected] wrote:
> > @@ -1228,3 +1230,4 @@ CONFIG_CMA_SIZE_MBYTES=64 CONFIG_PRINTK_TIME=y
> > CONFIG_MAGIC_SYSRQ=y CONFIG_DEBUG_FS=y
> > +CONFIG_GXP_WATCHDOG=y
> This does not look like in correct place. You need to add entries how the savedefconfig would do it.
Hello Krzysztof,
I have moved CONFIG_GXP_WATCHDOG between CONFIG_PM8916_WATCHDOG=m and CONFIG_BCM47XX_WDT=y. How do I run savedefconfig to make sure it is in the right place for this file?
Thanks,
-Nick Hawkins
On 29/04/2022 22:34, Hawkins, Nick wrote:
> I have moved CONFIG_GXP_WATCHDOG between CONFIG_PM8916_WATCHDOG=m and CONFIG_BCM47XX_WDT=y. How do I run savedefconfig to make sure it is in the right place for this file?
make savedefconfig
Best regards,
Krzysztof