[PATCH v3 01/10] arch: arm: mach-hpe: Introduce the HPE GXP architecture

[PATCH v3 01/10] arch: arm: mach-hpe: Introduce the HPE GXP architecture

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

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 <nick.hawkins@hpe.com>
---
 arch/arm/Kconfig           |  2 ++
 arch/arm/Makefile          |  1 +
 arch/arm/mach-hpe/Kconfig  | 20 +++++++++++++
 arch/arm/mach-hpe/Makefile |  1 +
 arch/arm/mach-hpe/gxp.c    | 61 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 85 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 4c97cb40eebb..6998b5b5f59e 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -618,6 +618,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 77172d555c7e..1cc0523d0a0a 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -178,6 +178,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..9b27f97c6536
--- /dev/null
+++ b/arch/arm/mach-hpe/Kconfig
@@ -0,0 +1,20 @@
+menuconfig ARCH_HPE
+	bool "HPE SoC support"
+	help
+	  This enables support for HPE ARM based SoC chips
+if ARCH_HPE
+
+config ARCH_HPE_GXP
+	bool "HPE GXP SoC"
+	select ARM_VIC
+	select PINCTRL
+	select IRQ_DOMAIN
+	select GENERIC_IRQ_CHIP
+	select MULTI_IRQ_HANDLER
+	select SPARSE_IRQ
+	select CLKSRC_MMIO
+	depends on ARCH_MULTI_V7
+	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..44f6d719a346
--- /dev/null
+++ b/arch/arm/mach-hpe/gxp.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P.*/
+
+
+#include <linux/init.h>
+#include <linux/of_address.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <asm/mach/arch.h>
+#include <asm/mach/map.h>
+
+#define IOP_REGS_PHYS_BASE 0xc0000000
+#define IOP_REGS_VIRT_BASE 0xf0000000
+#define IOP_REGS_SIZE (240*SZ_1M)
+#define RESET_CMD 0x00080002
+
+static struct map_desc gxp_io_desc[] __initdata = {
+	{
+		.virtual	= (unsigned long)IOP_REGS_VIRT_BASE,
+		.pfn		= __phys_to_pfn(IOP_REGS_PHYS_BASE),
+		.length		= IOP_REGS_SIZE,
+		.type		= MT_DEVICE,
+	},
+};
+
+void __init gxp_map_io(void)
+{
+	iotable_init(gxp_io_desc, ARRAY_SIZE(gxp_io_desc));
+}
+
+static void __init gxp_dt_init(void)
+{
+	void __iomem *gxp_init_regs;
+	struct device_node *np;
+
+	np = of_find_compatible_node(NULL, NULL, "hpe,gxp-cpu-init");
+	gxp_init_regs = of_iomap(np, 0);
+
+	/*it is necessary for our SOC to reset ECHI through this*/
+	/* register due to a hardware limitation*/
+	__raw_writel(RESET_CMD,
+		(gxp_init_regs));
+
+}
+
+static void gxp_restart(enum reboot_mode mode, const char *cmd)
+{
+	__raw_writel(1, (void __iomem *) IOP_REGS_VIRT_BASE);
+}
+
+static const char * const gxp_board_dt_compat[] = {
+	"hpe,gxp",
+	NULL,
+};
+
+DT_MACHINE_START(GXP_DT, "HPE GXP")
+	.init_machine	= gxp_dt_init,
+	.map_io		= gxp_map_io,
+	.restart	= gxp_restart,
+	.dt_compat	= gxp_board_dt_compat,
+MACHINE_END
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 02/10] arch: arm: configs: multi_v7_defconfig

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

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 <nick.hawkins@hpe.com>
---
 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 8863fa969ede..b93d213b7e60 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -1006,6 +1006,8 @@ CONFIG_QCOM_GSBI=y
 CONFIG_QCOM_SMD_RPM=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
@@ -1169,3 +1171,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[nick.hawkins]

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
diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
new file mode 100644
index 000000000000..da5eac1213a8
--- /dev/null
+++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree file for HPE DL360Gen10
+ */
+
+/include/ "hpe-gxp.dtsi"
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	compatible = "hpe,gxp";
+	model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
+
+	chosen {
+		bootargs = "earlyprintk console=ttyS2,115200";
+	};
+
+	memory@40000000 {
+		device_type = "memory";
+		reg = <0x40000000 0x20000000>;
+	};
+
+	ahb {
+
+	};
+
+};
diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
new file mode 100644
index 000000000000..dfaf8df829fe
--- /dev/null
+++ b/arch/arm/boot/dts/hpe-gxp.dtsi
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree file for HPE GXP
+ */
+
+/dts-v1/;
+/ {
+	model = "Hewlett Packard Enterprise GXP BMC";
+	compatible = "hpe,gxp";
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0>;
+		};
+	};
+
+	gxp-init@cefe0010 {
+		compatible = "hpe,gxp-cpu-init";
+		reg = <0xcefe0010 0x04>;
+	};
+
+	memory@40000000 {
+		device_type = "memory";
+		reg = <0x40000000 0x20000000>;
+	};
+
+	ahb {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		device_type = "soc";
+		ranges;
+
+		vic0: interrupt-controller@ceff0000 {
+			compatible = "arm,pl192-vic";
+			interrupt-controller;
+			reg = <0xceff0000 0x1000>;
+			#interrupt-cells = <1>;
+		};
+
+		vic1: interrupt-controller@80f00000 {
+			compatible = "arm,pl192-vic";
+			interrupt-controller;
+			reg = <0x80f00000 0x1000>;
+			#interrupt-cells = <1>;
+		};
+
+		timer0: timer@c0000080 {
+			compatible = "hpe,gxp-timer";
+			reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
+			interrupts = <0>;
+			interrupt-parent = <&vic0>;
+			clock-frequency = <400000000>;
+		};
+
+		uarta: serial@c00000e0 {
+			compatible = "ns16550a";
+			reg = <0xc00000e0 0x8>;
+			interrupts = <17>;
+			interrupt-parent = <&vic0>;
+			clock-frequency = <1846153>;
+			reg-shift = <0>;
+		};
+
+		uartb: serial@c00000e8 {
+			compatible = "ns16550a";
+			reg = <0xc00000e8 0x8>;
+			interrupts = <18>;
+			interrupt-parent = <&vic0>;
+			clock-frequency = <1846153>;
+			reg-shift = <0>;
+		};
+
+		uartc: serial@c00000f0 {
+			compatible = "ns16550a";
+			reg = <0xc00000f0 0x8>;
+			interrupts = <19>;
+			interrupt-parent = <&vic0>;
+			clock-frequency = <1846153>;
+			reg-shift = <0>;
+		};
+
+		usb0: usb@cefe0000 {
+			compatible = "generic-ehci";
+			reg = <0xcefe0000 0x100>;
+			interrupts = <7>;
+			interrupt-parent = <&vic0>;
+		};
+
+		usb1: usb@cefe0100 {
+			compatible = "generic-ohci";
+			reg = <0xcefe0100 0x110>;
+			interrupts = <6>;
+			interrupt-parent = <&vic0>;
+		};
+
+		vrom@58000000 {
+			compatible = "mtd-ram";
+			bank-width = <4>;
+			reg = <0x58000000 0x4000000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			partition@0 {
+				label = "vrom-prime";
+				reg = <0x0 0x2000000>;
+			};
+			partition@2000000 {
+				label = "vrom-second";
+				reg = <0x2000000 0x2000000>;
+			};
+		};
+
+		i2cg: syscon@c00000f8 {
+			compatible = "simple-mfd", "syscon";
+			reg = <0xc00000f8 0x08>;
+		};
+	};
+
+	clocks {
+		osc: osc {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-output-names = "osc";
+			clock-frequency = <33333333>;
+		};
+
+		iopclk: iopclk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-output-names = "iopclk";
+			clock-frequency = <400000000>;
+		};
+
+		memclk: memclk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-output-names = "memclk";
+			clock-frequency = <800000000>;
+		};
+	};
+};
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 01/10] arch: arm: mach-hpe: Introduce the HPE GXP architecture

[kernel test robot]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/timers/core]
[also build test WARNING on soc/for-next robh/for-next linus/master v5.17-rc7 next-20220310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/nick-hawkins-hpe-com/arch-arm-mach-hpe-Introduce-the-HPE-GXP-architecture/20220311-035513
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 58dedf0a4782ce42b4d31f1f62e5ad80a1b73d72
config: arm-defconfig (https://download.01.org/0day-ci/archive/20220311/202203111516.oSsKrRqX-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 276ca87382b8f16a65bddac700202924228982f6)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/9fbfc32473a65e025764e0a1456c421b4706fe8e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review nick-hawkins-hpe-com/arch-arm-mach-hpe-Introduce-the-HPE-GXP-architecture/20220311-035513
        git checkout 9fbfc32473a65e025764e0a1456c421b4706fe8e
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> arch/arm/mach-hpe/gxp.c:26:13: warning: no previous prototype for function 'gxp_map_io' [-Wmissing-prototypes]
   void __init gxp_map_io(void)
               ^
   arch/arm/mach-hpe/gxp.c:26:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void __init gxp_map_io(void)
   ^
   static 
   1 warning generated.


vim +/gxp_map_io +26 arch/arm/mach-hpe/gxp.c

    25	
  > 26	void __init gxp_map_io(void)
    27	{
    28		iotable_init(gxp_io_desc, ARRAY_SIZE(gxp_io_desc));
    29	}
    30	

---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 01/10] arch: arm: mach-hpe: Introduce the HPE GXP architecture

[Arnd Bergmann]

On Thu, Mar 10, 2022 at 8:52 PM <nick.hawkins@hpe.com> wrote:
>
> From: Nick Hawkins <nick.hawkins@hpe.com>
>
> 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 <nick.hawkins@hpe.com>

Please add me to Cc for the entire series when resending.


> +config ARCH_HPE_GXP
> +       bool "HPE GXP SoC"
> +       select ARM_VIC
> +       select PINCTRL
> +       select IRQ_DOMAIN
> +       select GENERIC_IRQ_CHIP
> +       select MULTI_IRQ_HANDLER
> +       select SPARSE_IRQ
> +       select CLKSRC_MMIO
> +       depends on ARCH_MULTI_V7

By convention, the 'depends on' usually comes first here.

Please drop the 'select' statements for things that are already selected
by ARCH_MULTIPLATFORM or ARCH_MULTI_V7.


> +
> +#define IOP_REGS_PHYS_BASE 0xc0000000
> +#define IOP_REGS_VIRT_BASE 0xf0000000
> +#define IOP_REGS_SIZE (240*SZ_1M)
> +#define RESET_CMD 0x00080002
> +
> +static struct map_desc gxp_io_desc[] __initdata = {
> +       {
> +               .virtual        = (unsigned long)IOP_REGS_VIRT_BASE,
> +               .pfn            = __phys_to_pfn(IOP_REGS_PHYS_BASE),
> +               .length         = IOP_REGS_SIZE,
> +               .type           = MT_DEVICE,
> +       },
> +};

It looks like this is only used for the pxf_restart() function below.
In this case, you should get rid of the static mapping entirely and
use an ioremap() in the gxp_dt_init() function instead, ideally getting
the address from an appropriate device node rather than hardcoding
it here.

If there are other drivers using the static mapping, either explain
here why this is required, or try to change them to dynamic mappings as well.

> +static void __init gxp_dt_init(void)
> +{
> +       void __iomem *gxp_init_regs;
> +       struct device_node *np;
> +
> +       np = of_find_compatible_node(NULL, NULL, "hpe,gxp-cpu-init");
> +       gxp_init_regs = of_iomap(np, 0);
> +
> +       /*it is necessary for our SOC to reset ECHI through this*/
> +       /* register due to a hardware limitation*/
> +       __raw_writel(RESET_CMD,
> +               (gxp_init_regs));

My feeling is still that this should be done in the platform specific EHCI
driver front-end. I think I commented on this before but don't remember
getting an explanation why you can't have it there.

> +static void gxp_restart(enum reboot_mode mode, const char *cmd)
> +{
> +       __raw_writel(1, (void __iomem *) IOP_REGS_VIRT_BASE);
> +}

With both of these, you should use writel() instead of __raw_write().
Using the __raw accessors breaks big-endian kernels (which you
probably don't need, but shouldn't break for no reason anyway), and
lacks serialization and atomicity of the access.

A better place for the restart logic may be a separate driver in
drivers/power/reset/, at least if this otherwise ends up being the only
platform specific code you need.

          Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 01/10] arch: arm: mach-hpe: Introduce the HPE GXP architecture

[kernel test robot]

Hi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/timers/core]
[also build test WARNING on soc/for-next robh/for-next linus/master v5.17-rc7 next-20220310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/nick-hawkins-hpe-com/arch-arm-mach-hpe-Introduce-the-HPE-GXP-architecture/20220311-035513
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 58dedf0a4782ce42b4d31f1f62e5ad80a1b73d72
config: arm-defconfig (https://download.01.org/0day-ci/archive/20220311/202203112000.DIc9zr62-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/9fbfc32473a65e025764e0a1456c421b4706fe8e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review nick-hawkins-hpe-com/arch-arm-mach-hpe-Introduce-the-HPE-GXP-architecture/20220311-035513
        git checkout 9fbfc32473a65e025764e0a1456c421b4706fe8e
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> arch/arm/mach-hpe/gxp.c:26:13: warning: no previous prototype for 'gxp_map_io' [-Wmissing-prototypes]
      26 | void __init gxp_map_io(void)
         |             ^~~~~~~~~~


vim +/gxp_map_io +26 arch/arm/mach-hpe/gxp.c

    25	
  > 26	void __init gxp_map_io(void)
    27	{
    28		iotable_init(gxp_io_desc, ARRAY_SIZE(gxp_io_desc));
    29	}
    30	

---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 01/10] arch: arm: mach-hpe: Introduce the HPE GXP architecture

[kernel test robot]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/timers/core]
[also build test ERROR on soc/for-next robh/for-next linus/master v5.17-rc7 next-20220310]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/nick-hawkins-hpe-com/arch-arm-mach-hpe-Introduce-the-HPE-GXP-architecture/20220311-035513
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 58dedf0a4782ce42b4d31f1f62e5ad80a1b73d72
config: arm-randconfig-c002-20220312 (https://download.01.org/0day-ci/archive/20220312/202203122119.szjgiy49-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/9fbfc32473a65e025764e0a1456c421b4706fe8e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review nick-hawkins-hpe-com/arch-arm-mach-hpe-Introduce-the-HPE-GXP-architecture/20220311-035513
        git checkout 9fbfc32473a65e025764e0a1456c421b4706fe8e
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]
--
>> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]
   drivers/memstick/host/r592.c:47:13: warning: no previous prototype for 'memstick_debug_get_tpc_name' [-Wmissing-prototypes]
      47 | const char *memstick_debug_get_tpc_name(int tpc)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
--
>> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]
   init/main.c:768:20: warning: no previous prototype for 'arch_post_acpi_subsys_init' [-Wmissing-prototypes]
     768 | void __init __weak arch_post_acpi_subsys_init(void) { }
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
   init/main.c:780:20: warning: no previous prototype for 'mem_encrypt_init' [-Wmissing-prototypes]
     780 | void __init __weak mem_encrypt_init(void) { }
         |                    ^~~~~~~~~~~~~~~~
   init/main.c:782:20: warning: no previous prototype for 'poking_init' [-Wmissing-prototypes]
     782 | void __init __weak poking_init(void) { }
         |                    ^~~~~~~~~~~
--
>> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]
   init/calibrate.c:261:37: warning: no previous prototype for 'calibrate_delay_is_known' [-Wmissing-prototypes]
     261 | unsigned long __attribute__((weak)) calibrate_delay_is_known(void)
         |                                     ^~~~~~~~~~~~~~~~~~~~~~~~
--
>> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]
   arch/arm/kernel/ptrace.c:854:16: warning: no previous prototype for 'syscall_trace_enter' [-Wmissing-prototypes]
     854 | asmlinkage int syscall_trace_enter(struct pt_regs *regs)
         |                ^~~~~~~~~~~~~~~~~~~
   arch/arm/kernel/ptrace.c:882:17: warning: no previous prototype for 'syscall_trace_exit' [-Wmissing-prototypes]
     882 | asmlinkage void syscall_trace_exit(struct pt_regs *regs)
         |                 ^~~~~~~~~~~~~~~~~~
--
>> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]
   arch/arm/kernel/reboot.c:78:6: warning: no previous prototype for 'soft_restart' [-Wmissing-prototypes]
      78 | void soft_restart(unsigned long addr)
         |      ^~~~~~~~~~~~
--
>> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]
   arch/arm/kernel/signal.c:186:16: warning: no previous prototype for 'sys_sigreturn' [-Wmissing-prototypes]
     186 | asmlinkage int sys_sigreturn(struct pt_regs *regs)
         |                ^~~~~~~~~~~~~
   arch/arm/kernel/signal.c:216:16: warning: no previous prototype for 'sys_rt_sigreturn' [-Wmissing-prototypes]
     216 | asmlinkage int sys_rt_sigreturn(struct pt_regs *regs)
         |                ^~~~~~~~~~~~~~~~
   arch/arm/kernel/signal.c:601:1: warning: no previous prototype for 'do_work_pending' [-Wmissing-prototypes]
     601 | do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
         | ^~~~~~~~~~~~~~~
--
>> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]
   arch/arm/kernel/sys_arm.c:32:17: warning: no previous prototype for 'sys_arm_fadvise64_64' [-Wmissing-prototypes]
      32 | asmlinkage long sys_arm_fadvise64_64(int fd, int advice,
         |                 ^~~~~~~~~~~~~~~~~~~~
--
>> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]
   arch/arm/kernel/time.c:88:13: warning: no previous prototype for 'time_init' [-Wmissing-prototypes]
      88 | void __init time_init(void)
         |             ^~~~~~~~~
--
>> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]
   arch/arm/kernel/traps.c:84:6: warning: no previous prototype for 'dump_backtrace_stm' [-Wmissing-prototypes]
      84 | void dump_backtrace_stm(u32 *stack, u32 instruction, const char *loglvl)
         |      ^~~~~~~~~~~~~~~~~~
   arch/arm/kernel/traps.c:433:17: warning: no previous prototype for 'do_undefinstr' [-Wmissing-prototypes]
     433 | asmlinkage void do_undefinstr(struct pt_regs *regs)
         |                 ^~~~~~~~~~~~~
   arch/arm/kernel/traps.c:498:39: warning: no previous prototype for 'handle_fiq_as_nmi' [-Wmissing-prototypes]
     498 | asmlinkage void __exception_irq_entry handle_fiq_as_nmi(struct pt_regs *regs)
         |                                       ^~~~~~~~~~~~~~~~~
   arch/arm/kernel/traps.c:517:17: warning: no previous prototype for 'bad_mode' [-Wmissing-prototypes]
     517 | asmlinkage void bad_mode(struct pt_regs *regs, int reason)
         |                 ^~~~~~~~
   arch/arm/kernel/traps.c:590:16: warning: no previous prototype for 'arm_syscall' [-Wmissing-prototypes]
     590 | asmlinkage int arm_syscall(int no, struct pt_regs *regs)
         |                ^~~~~~~~~~~
   arch/arm/kernel/traps.c:716:1: warning: no previous prototype for 'baddataabort' [-Wmissing-prototypes]
     716 | baddataabort(int code, unsigned long instr, struct pt_regs *regs)
         | ^~~~~~~~~~~~
   arch/arm/kernel/traps.c:756:17: warning: no previous prototype for '__div0' [-Wmissing-prototypes]
     756 | asmlinkage void __div0(void)
         |                 ^~~~~~
   arch/arm/kernel/traps.c:763:6: warning: no previous prototype for 'abort' [-Wmissing-prototypes]
     763 | void abort(void)
         |      ^~~~~
--
>> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]
   arch/arm/kernel/suspend.c:75:6: warning: no previous prototype for '__cpu_suspend_save' [-Wmissing-prototypes]
      75 | void __cpu_suspend_save(u32 *ptr, u32 ptrsz, u32 sp, u32 *save_ptr)
         |      ^~~~~~~~~~~~~~~~~~
..

---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 01/10] arch: arm: mach-hpe: Introduce the HPE GXP architecture

[Arnd Bergmann]

On Sat, Mar 12, 2022 at 2:27 PM kernel test robot <lkp@intel.com> wrote:

>
> All error/warnings (new ones prefixed by >>):
>
> >> cc1: warning: arch/arm/mach-hpe/include: No such file or directory [-Wmissing-include-dirs]

You need to make CONFIG_ARCH_HPE depend on CONFIG_ARCH_MULTI_V7, otherwise
it becomes possible to select this for non-multiplatform configs that expect
an include directory below the platform.

       Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

>> -----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Krzysztof Kozlowski]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----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




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel