From: Clay Chang <[email protected]>
The GXP SROM control register can be used to configure LPC related
legacy I/O registers. Currently only the SROM RAM Offset Register
(vromoff) is exported.
Clay Chang (5):
soc: hpe: Add GXP SROM Control Register Driver
dt-bindings: soc: hpe: hpe,gxp-srom.yaml
ARM: dts: hpe: Add SROM Driver
ARM: multi_v7_defconfig: Add GXP SROM Driver
MAINTAINERS: Add maintainer of GXP SROM support
.../bindings/soc/hpe/hpe,gxp-srom.yaml | 36 +++++
MAINTAINERS | 8 +
arch/arm/boot/dts/hpe-gxp.dtsi | 41 ++---
arch/arm/configs/multi_v7_defconfig | 2 +
drivers/soc/Kconfig | 1 +
drivers/soc/Makefile | 1 +
drivers/soc/hpe/Kconfig | 29 ++++
drivers/soc/hpe/Makefile | 2 +
drivers/soc/hpe/gxp-soclib.c | 17 +++
drivers/soc/hpe/gxp-soclib.h | 9 ++
drivers/soc/hpe/gxp-srom.c | 141 ++++++++++++++++++
11 files changed, 269 insertions(+), 18 deletions(-)
create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml
create mode 100644 drivers/soc/hpe/Kconfig
create mode 100644 drivers/soc/hpe/Makefile
create mode 100644 drivers/soc/hpe/gxp-soclib.c
create mode 100644 drivers/soc/hpe/gxp-soclib.h
create mode 100644 drivers/soc/hpe/gxp-srom.c
--
2.17.1
From: Clay Chang <[email protected]>
Add SROM driver to the device tree. Also re-organize the AHB base
address to accommodate the SROM driver register requirements. Add the
hpe,gxp-srom compatible.
Signed-off-by: Clay Chang <[email protected]>
---
arch/arm/boot/dts/hpe-gxp.dtsi | 41 +++++++++++++++++++---------------
1 file changed, 23 insertions(+), 18 deletions(-)
diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
index cf735b3c4f35..3e96941721d5 100644
--- a/arch/arm/boot/dts/hpe-gxp.dtsi
+++ b/arch/arm/boot/dts/hpe-gxp.dtsi
@@ -52,76 +52,81 @@
cache-level = <2>;
};
- ahb@c0000000 {
+ ahb@80000000 {
compatible = "simple-bus";
#address-cells = <1>;
#size-cells = <1>;
- ranges = <0x0 0xc0000000 0x30000000>;
+ ranges = <0x0 0x80000000 0x7FFFFFFF>;
dma-ranges;
- vic0: interrupt-controller@eff0000 {
+ vic0: interrupt-controller@4eff0000 {
compatible = "arm,pl192-vic";
- reg = <0xeff0000 0x1000>;
+ reg = <0x4eff0000 0x1000>;
interrupt-controller;
#interrupt-cells = <1>;
};
- vic1: interrupt-controller@80f00000 {
+ vic1: interrupt-controller@f00000 {
compatible = "arm,pl192-vic";
- reg = <0x80f00000 0x1000>;
+ reg = <0xf00000 0x1000>;
interrupt-controller;
#interrupt-cells = <1>;
};
- uarta: serial@e0 {
+ uarta: serial@400000e0 {
compatible = "ns16550a";
- reg = <0xe0 0x8>;
+ reg = <0x400000e0 0x8>;
interrupts = <17>;
interrupt-parent = <&vic0>;
clock-frequency = <1846153>;
reg-shift = <0>;
};
- uartb: serial@e8 {
+ uartb: serial@400000e8 {
compatible = "ns16550a";
- reg = <0xe8 0x8>;
+ reg = <0x400000e8 0x8>;
interrupts = <18>;
interrupt-parent = <&vic0>;
clock-frequency = <1846153>;
reg-shift = <0>;
};
- uartc: serial@f0 {
+ uartc: serial@400000f0 {
compatible = "ns16550a";
- reg = <0xf0 0x8>;
+ reg = <0x400000f0 0x8>;
interrupts = <19>;
interrupt-parent = <&vic0>;
clock-frequency = <1846153>;
reg-shift = <0>;
};
- usb0: usb@efe0000 {
+ usb0: usb@4efe0000 {
compatible = "hpe,gxp-ehci", "generic-ehci";
- reg = <0xefe0000 0x100>;
+ reg = <0x4efe0000 0x100>;
interrupts = <7>;
interrupt-parent = <&vic0>;
};
- st: timer@80 {
+ st: timer@40000080 {
compatible = "hpe,gxp-timer";
- reg = <0x80 0x16>;
+ reg = <0x40000080 0x16>;
interrupts = <0>;
interrupt-parent = <&vic0>;
clocks = <&iopclk>;
clock-names = "iop";
};
- usb1: usb@efe0100 {
+ usb1: usb@4efe0100 {
compatible = "hpe,gxp-ohci", "generic-ohci";
- reg = <0xefe0100 0x110>;
+ reg = <0x4efe0100 0x110>;
interrupts = <6>;
interrupt-parent = <&vic0>;
};
+
+ srom: srom@fc0000 {
+ compatible = "hpe,gxp-srom";
+ reg = <0xfc0000 0x100>;
+ };
};
};
};
--
2.17.1
From: Clay Chang <[email protected]>
The GXP SROM control register can be used to configure LPC related
legacy I/O registers. Currently only the SROM RAM Offset Register
(vromoff) is exported.
The GXP SOCLIB is a common library used for creating the common
"soc" class in the kernel.
Signed-off-by: Clay Chang <[email protected]>
---
drivers/soc/Kconfig | 1 +
drivers/soc/Makefile | 1 +
drivers/soc/hpe/Kconfig | 29 +++++++
drivers/soc/hpe/Makefile | 2 +
drivers/soc/hpe/gxp-soclib.c | 17 +++++
drivers/soc/hpe/gxp-soclib.h | 9 +++
drivers/soc/hpe/gxp-srom.c | 141 +++++++++++++++++++++++++++++++++++
7 files changed, 200 insertions(+)
create mode 100644 drivers/soc/hpe/Kconfig
create mode 100644 drivers/soc/hpe/Makefile
create mode 100644 drivers/soc/hpe/gxp-soclib.c
create mode 100644 drivers/soc/hpe/gxp-soclib.h
create mode 100644 drivers/soc/hpe/gxp-srom.c
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 5dbb09f843f7..faff0f036b61 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -10,6 +10,7 @@ source "drivers/soc/bcm/Kconfig"
source "drivers/soc/canaan/Kconfig"
source "drivers/soc/fsl/Kconfig"
source "drivers/soc/fujitsu/Kconfig"
+source "drivers/soc/hpe/Kconfig"
source "drivers/soc/imx/Kconfig"
source "drivers/soc/ixp4xx/Kconfig"
source "drivers/soc/litex/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index fff513bd522d..d257b9d654b3 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_MACH_DOVE) += dove/
obj-y += fsl/
obj-y += fujitsu/
obj-$(CONFIG_ARCH_GEMINI) += gemini/
+obj-$(CONFIG_ARCH_HPE) += hpe/
obj-y += imx/
obj-y += ixp4xx/
obj-$(CONFIG_SOC_XWAY) += lantiq/
diff --git a/drivers/soc/hpe/Kconfig b/drivers/soc/hpe/Kconfig
new file mode 100644
index 000000000000..88f5d46b06b6
--- /dev/null
+++ b/drivers/soc/hpe/Kconfig
@@ -0,0 +1,29 @@
+#
+# HPE GXP SoC drivers
+#
+menu "HPE GXP SoC drivers"
+ depends on ARCH_HPE || COMPILE_TEST
+
+config HPE_GXP_SOCLIB
+ bool "GXP Common SoC Library"
+ default y
+ depends on ARCH_HPE_GXP || COMPILE_TEST
+ help
+ This is for the common library for all HPE SoC drivers. It
+ creates the root soc class (/sys/class/soc) for all GXP SoC
+ drivers. It must be yes if any one of the GXP SoC drivers were
+ added, so the config of all GXP SoC drivers must select this.
+
+
+config HPE_GXP_SROM
+ bool "GXP SROM Configuration Driver"
+ default y
+ depends on ARCH_HPE_GXP || COMPILE_TEST
+ select HPE_GXP_SOCLIB
+ help
+ Say yes here to add support for SROM Configuration. The GXP SROM
+ control register can be used to configure LPC related legacy I/O
+ registers. Currently only the SROM RAM Offset Register (vromoff)
+ is exported.
+
+endmenu
diff --git a/drivers/soc/hpe/Makefile b/drivers/soc/hpe/Makefile
new file mode 100644
index 000000000000..78de24ecb606
--- /dev/null
+++ b/drivers/soc/hpe/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_HPE_GXP_SOCLIB) += gxp-soclib.o
+obj-$(CONFIG_HPE_GXP_SROM) += gxp-srom.o
diff --git a/drivers/soc/hpe/gxp-soclib.c b/drivers/soc/hpe/gxp-soclib.c
new file mode 100644
index 000000000000..11b0afe09070
--- /dev/null
+++ b/drivers/soc/hpe/gxp-soclib.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2023 Hewlett Packard Enteprise Development Company, L.P. */
+
+#include <linux/device.h>
+#include <linux/module.h>
+
+struct class *soc_class;
+
+static int __init gxp_soclib_init(void)
+{
+ soc_class = class_create(THIS_MODULE, "soc");
+ if (IS_ERR(soc_class))
+ return PTR_ERR(soc_class);
+ return 0;
+}
+
+module_init(gxp_soclib_init);
diff --git a/drivers/soc/hpe/gxp-soclib.h b/drivers/soc/hpe/gxp-soclib.h
new file mode 100644
index 000000000000..eb0e72b67aee
--- /dev/null
+++ b/drivers/soc/hpe/gxp-soclib.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/* Copyright (C) 2023 Hewlett Packard Enterprise Development Company, L.P. */
+
+#ifndef __GXP_SOCLIB_H__
+#define __GXP_SOCLIB_H__
+
+extern struct class *soc_class;
+
+#endif
diff --git a/drivers/soc/hpe/gxp-srom.c b/drivers/soc/hpe/gxp-srom.c
new file mode 100644
index 000000000000..5a8f005055cf
--- /dev/null
+++ b/drivers/soc/hpe/gxp-srom.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2023 Hewlett Packard Enterprise Development Company, L.P. */
+
+#include <linux/device.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/sysfs.h>
+
+#include "gxp-soclib.h"
+
+#define SROM_VROMOFF 0xf4 // 80fc_00f4
+
+struct gxp_srom_drvdata {
+ struct platform_device *pdev;
+ struct device *dev;
+ void __iomem *base;
+ struct mutex mutex;
+};
+
+static ssize_t vromoff_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct gxp_srom_drvdata *drvdata = dev_get_drvdata(dev);
+ unsigned int value;
+ ssize_t ret;
+
+ mutex_lock(&drvdata->mutex);
+ value = readl(drvdata->base + SROM_VROMOFF);
+ ret = sprintf(buf, "0x%08x", value);
+ mutex_unlock(&drvdata->mutex);
+
+ return ret;
+}
+
+static ssize_t vromoff_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct gxp_srom_drvdata *drvdata = dev_get_drvdata(dev);
+ unsigned int value;
+ int rc;
+
+ rc = kstrtouint(buf, 0, &value);
+ if (rc < 0)
+ return -EINVAL;
+
+ mutex_lock(&drvdata->mutex);
+ writel(value, drvdata->base + SROM_VROMOFF);
+ mutex_unlock(&drvdata->mutex);
+
+ return count;
+}
+static DEVICE_ATTR_RW(vromoff);
+
+static struct attribute *srom_attrs[] = {
+ &dev_attr_vromoff.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(srom);
+
+static int sysfs_register(struct device *parent,
+ struct gxp_srom_drvdata *drvdata)
+{
+ struct device *dev;
+
+ dev = device_create_with_groups(soc_class, parent, 0,
+ drvdata, srom_groups, "srom");
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
+ drvdata->dev = dev;
+ return 0;
+}
+
+static int gxp_srom_probe(struct platform_device *pdev)
+{
+ struct gxp_srom_drvdata *drvdata;
+ struct resource *res;
+ int ret;
+
+ drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_srom_drvdata),
+ GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+
+ drvdata->pdev = pdev;
+ platform_set_drvdata(pdev, drvdata);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "no srom resource defined\n");
+ ret = -ENODEV;
+ goto out;
+ }
+
+ drvdata->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(drvdata->base)) {
+ dev_err(&pdev->dev, "ioremap_resource error\n");
+ ret = PTR_ERR(drvdata->base);
+ goto ioremap_err;
+ }
+
+ mutex_init(&drvdata->mutex);
+
+ ret = sysfs_register(&pdev->dev, drvdata);
+ if (ret != 0) {
+ dev_err(&pdev->dev, "sysfs error\n");
+ goto sysfs_err;
+ }
+
+ dev_info(&pdev->dev, "initialized\n");
+ return 0;
+
+sysfs_err:
+ioremap_err:
+ platform_set_drvdata(pdev, NULL);
+out:
+ if (drvdata)
+ devm_kfree(&pdev->dev, (void *)drvdata);
+ return ret;
+}
+
+static const struct of_device_id gxp_srom_of_match[] = {
+ { .compatible = "hpe,gxp-srom" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, gxp_srom_of_match);
+
+static struct platform_driver gxp_srom_driver = {
+ .probe = gxp_srom_probe,
+ .driver = {
+ .name = "gxp-srom",
+ .of_match_table = of_match_ptr(gxp_srom_of_match),
+ },
+};
+module_platform_driver(gxp_srom_driver);
+
+MODULE_AUTHOR("Clay Chang <[email protected]>");
+MODULE_DESCRIPTION("HPE GXP SROM Configuration Driver");
+MODULE_LICENSE("GPL");
--
2.17.1
From: Clay Chang <[email protected]>
Add Clay Chang as the maintainer of GXP SROM support.
Signed-off-by: Clay Chang <[email protected]>
---
MAINTAINERS | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index ea941dc469fa..164571ac1cc5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2279,14 +2279,22 @@ F: arch/arm/mach-sa1100/jornada720.c
ARM/HPE GXP ARCHITECTURE
M: Jean-Marie Verdun <[email protected]>
M: Nick Hawkins <[email protected]>
+M: Clay Chang <[email protected]>
S: Maintained
F: Documentation/devicetree/bindings/arm/hpe,gxp.yaml
+F: Documentation/devicetree/bindings/soc/hpe/
+F: Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml
F: Documentation/devicetree/bindings/spi/hpe,gxp-spifi.yaml
F: Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
F: arch/arm/boot/dts/hpe-bmc*
F: arch/arm/boot/dts/hpe-gxp*
F: arch/arm/mach-hpe/
F: drivers/clocksource/timer-gxp.c
+F: drivers/soc/hpe/
+F: drivers/soc/hpe/Kconfig
+F: drivers/soc/hpe/Makefile
+F: drivers/soc/hpe/gxp-soclib.[ch]
+F: drivers/soc/hpe/gxp-srom.c
F: drivers/spi/spi-gxp.c
F: drivers/watchdog/gxp-wdt.c
--
2.17.1
From: Clay Chang <[email protected]>
Document binding to support SROM driver in GXP.
Signed-off-by: Clay Chang <[email protected]>
---
.../bindings/soc/hpe/hpe,gxp-srom.yaml | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml
diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml
new file mode 100644
index 000000000000..14ad97d595c8
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml
@@ -0,0 +1,36 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/hpe/hpe,gxp-srom.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE GXP SoC SROM Control Register
+
+maintainers:
+ - Clay Chang <[email protected]>
+
+description: |+
+ The SROM control register can be used to configure LPC related legacy
+ I/O registers.
+
+properties:
+ compatible:
+ items:
+ - const: hpe,gxp-srom
+
+ reg:
+ items:
+ - description: SROM LPC Configuration Registers
+
+required:
+ - compatible
+ - reg
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ srom: srom@80fc0000 {
+ compatible = "hpe,gxp-srom";
+ reg = <0x80fc0000 0x100>;
+ };
--
2.17.1
On 10/01/2023 05:25, [email protected] wrote:
> From: Clay Chang <[email protected]>
>
> Add Clay Chang as the maintainer of GXP SROM support.
Your commit is not doing it. Nope. Either make proper entry matching
this commit msg or make commit msg reflecting truth.
>
> Signed-off-by: Clay Chang <[email protected]>
> ---
> MAINTAINERS | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ea941dc469fa..164571ac1cc5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2279,14 +2279,22 @@ F: arch/arm/mach-sa1100/jornada720.c
> ARM/HPE GXP ARCHITECTURE
> M: Jean-Marie Verdun <[email protected]>
> M: Nick Hawkins <[email protected]>
> +M: Clay Chang <[email protected]>
Best regards,
Krzysztof
On 10/01/2023 05:25, [email protected] wrote:
> From: Clay Chang <[email protected]>
>
> The GXP SROM control register can be used to configure LPC related
> legacy I/O registers. Currently only the SROM RAM Offset Register
> (vromoff) is exported.
>
> The GXP SOCLIB is a common library used for creating the common
> "soc" class in the kernel.
>
> Signed-off-by: Clay Chang <[email protected]>
> ---
> drivers/soc/Kconfig | 1 +
> drivers/soc/Makefile | 1 +
> drivers/soc/hpe/Kconfig | 29 +++++++
> drivers/soc/hpe/Makefile | 2 +
> drivers/soc/hpe/gxp-soclib.c | 17 +++++
> drivers/soc/hpe/gxp-soclib.h | 9 +++
> drivers/soc/hpe/gxp-srom.c | 141 +++++++++++++++++++++++++++++++++++
> 7 files changed, 200 insertions(+)
> create mode 100644 drivers/soc/hpe/Kconfig
> create mode 100644 drivers/soc/hpe/Makefile
> create mode 100644 drivers/soc/hpe/gxp-soclib.c
> create mode 100644 drivers/soc/hpe/gxp-soclib.h
> create mode 100644 drivers/soc/hpe/gxp-srom.c
>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 5dbb09f843f7..faff0f036b61 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -10,6 +10,7 @@ source "drivers/soc/bcm/Kconfig"
> source "drivers/soc/canaan/Kconfig"
> source "drivers/soc/fsl/Kconfig"
> source "drivers/soc/fujitsu/Kconfig"
> +source "drivers/soc/hpe/Kconfig"
> source "drivers/soc/imx/Kconfig"
> source "drivers/soc/ixp4xx/Kconfig"
> source "drivers/soc/litex/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index fff513bd522d..d257b9d654b3 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_MACH_DOVE) += dove/
> obj-y += fsl/
> obj-y += fujitsu/
> obj-$(CONFIG_ARCH_GEMINI) += gemini/
> +obj-$(CONFIG_ARCH_HPE) += hpe/
> obj-y += imx/
> obj-y += ixp4xx/
> obj-$(CONFIG_SOC_XWAY) += lantiq/
> diff --git a/drivers/soc/hpe/Kconfig b/drivers/soc/hpe/Kconfig
> new file mode 100644
> index 000000000000..88f5d46b06b6
> --- /dev/null
> +++ b/drivers/soc/hpe/Kconfig
> @@ -0,0 +1,29 @@
> +#
> +# HPE GXP SoC drivers
> +#
> +menu "HPE GXP SoC drivers"
> + depends on ARCH_HPE || COMPILE_TEST
> +
> +config HPE_GXP_SOCLIB
> + bool "GXP Common SoC Library"
> + default y
> + depends on ARCH_HPE_GXP || COMPILE_TEST
> + help
> + This is for the common library for all HPE SoC drivers. It
> + creates the root soc class (/sys/class/soc) for all GXP SoC
> + drivers. It must be yes if any one of the GXP SoC drivers were
> + added, so the config of all GXP SoC drivers must select this.
Don't open-code Kconfig dependencies in free-form text. IOW, drop last
sentence.
> +
> +
> +config HPE_GXP_SROM
> + bool "GXP SROM Configuration Driver"
> + default y
> + depends on ARCH_HPE_GXP || COMPILE_TEST
> + select HPE_GXP_SOCLIB
> + help
> + Say yes here to add support for SROM Configuration. The GXP SROM
> + control register can be used to configure LPC related legacy I/O
> + registers. Currently only the SROM RAM Offset Register (vromoff)
> + is exported.
> +
> +endmenu
> diff --git a/drivers/soc/hpe/Makefile b/drivers/soc/hpe/Makefile
> new file mode 100644
> index 000000000000..78de24ecb606
> --- /dev/null
> +++ b/drivers/soc/hpe/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_HPE_GXP_SOCLIB) += gxp-soclib.o
> +obj-$(CONFIG_HPE_GXP_SROM) += gxp-srom.o
> diff --git a/drivers/soc/hpe/gxp-soclib.c b/drivers/soc/hpe/gxp-soclib.c
> new file mode 100644
> index 000000000000..11b0afe09070
> --- /dev/null
> +++ b/drivers/soc/hpe/gxp-soclib.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (C) 2023 Hewlett Packard Enteprise Development Company, L.P. */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +
> +struct class *soc_class;
> +
> +static int __init gxp_soclib_init(void)
> +{
> + soc_class = class_create(THIS_MODULE, "soc");
> + if (IS_ERR(soc_class))
> + return PTR_ERR(soc_class);
> + return 0;
> +}
> +
> +module_init(gxp_soclib_init);
I don't see a point of making it a shared object and a "kernel library".
Module inits are not libraries. Drop entire file.
> diff --git a/drivers/soc/hpe/gxp-soclib.h b/drivers/soc/hpe/gxp-soclib.h
> new file mode 100644
> index 000000000000..eb0e72b67aee
> --- /dev/null
> +++ b/drivers/soc/hpe/gxp-soclib.h
> @@ -0,0 +1,9 @@
> +
> +static int sysfs_register(struct device *parent,
> + struct gxp_srom_drvdata *drvdata)
> +{
> + struct device *dev;
> +
> + dev = device_create_with_groups(soc_class, parent, 0,
> + drvdata, srom_groups, "srom");
> + if (IS_ERR(dev))
> + return PTR_ERR(dev);
> + drvdata->dev = dev;
> + return 0;
> +}
> +
> +static int gxp_srom_probe(struct platform_device *pdev)
> +{
> + struct gxp_srom_drvdata *drvdata;
> + struct resource *res;
> + int ret;
> +
> + drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_srom_drvdata),
sizeof(*)
> + GFP_KERNEL);
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->pdev = pdev;
> + platform_set_drvdata(pdev, drvdata);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "no srom resource defined\n");
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + drvdata->base = devm_ioremap_resource(&pdev->dev, res);
Combine these two. There is a helper for this.
> + if (IS_ERR(drvdata->base)) {
> + dev_err(&pdev->dev, "ioremap_resource error\n");
> + ret = PTR_ERR(drvdata->base);
return dev_err_probe().
> + goto ioremap_err;
> + }
> +
> + mutex_init(&drvdata->mutex);
> +
> + ret = sysfs_register(&pdev->dev, drvdata);
Missing ABI documentation.
> + if (ret != 0) {
> + dev_err(&pdev->dev, "sysfs error\n");
return dev_err_probe
> + goto sysfs_err;
> + }
> +
> + dev_info(&pdev->dev, "initialized\n");
Drop silly probe successes.
> + return 0;
> +
> +sysfs_err:
> +ioremap_err:
> + platform_set_drvdata(pdev, NULL);
> +out:
> + if (drvdata)
> + devm_kfree(&pdev->dev, (void *)drvdata);
1. Why? if it is here, it must be in remove() callback. If it is invalid
(because it is really not correct) for remove(), it cannot be here, right?
2. Why cast?
> + return ret;
> +}
> +
> +static const struct of_device_id gxp_srom_of_match[] = {
> + { .compatible = "hpe,gxp-srom" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, gxp_srom_of_match);
> +
> +static struct platform_driver gxp_srom_driver = {
> + .probe = gxp_srom_probe,
> + .driver = {
> + .name = "gxp-srom",
> + .of_match_table = of_match_ptr(gxp_srom_of_match),
That will cause a warning. Drop of_match_ptr.
> + },
> +};
> +module_platform_driver(gxp_srom_driver);
> +
> +MODULE_AUTHOR("Clay Chang <[email protected]>");
> +MODULE_DESCRIPTION("HPE GXP SROM Configuration Driver");
> +MODULE_LICENSE("GPL");
Best regards,
Krzysztof
On 10/01/2023 05:25, [email protected] wrote:
> From: Clay Chang <[email protected]>
>
> Document binding to support SROM driver in GXP.
>
> Signed-off-by: Clay Chang <[email protected]>
> ---
> .../bindings/soc/hpe/hpe,gxp-srom.yaml | 36 +++++++++++++++++++
> 1 file changed, 36 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml
>
> diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml
> new file mode 100644
> index 000000000000..14ad97d595c8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml
Don't drop stuff to soc. Put it in respective directories. This also
applies to your driver.
SROM controllers go to memory-controllers. What is this, I have no clue.
"SROM Control Register" is not helping me.
> @@ -0,0 +1,36 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/hpe/hpe,gxp-srom.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP SoC SROM Control Register
> +
> +maintainers:
> + - Clay Chang <[email protected]>
> +
> +description: |+
> + The SROM control register can be used to configure LPC related legacy
> + I/O registers.
And why this is a hardware? No, you now add fake devices to be able to
write some stuff from user-space... Otherwise this needs proper hardware
description.
> +
> +properties:
> + compatible:
> + items:
Drop items, you have only one item.
> + - const: hpe,gxp-srom
> +
> + reg:
> + items:
> + - description: SROM LPC Configuration Registers
Drop items and description. Just maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + srom: srom@80fc0000 {
> + compatible = "hpe,gxp-srom";
> + reg = <0x80fc0000 0x100>;
> + };
Best regards,
Krzysztof
Hi Krzysztof,
Thank you for the time to review my code.
On Tue, Jan 10, 2023 at 10:46:57AM +0100, Krzysztof Kozlowski wrote:
> On 10/01/2023 05:25, [email protected] wrote:
> > From: Clay Chang <[email protected]>
> >
> > The GXP SROM control register can be used to configure LPC related
> > legacy I/O registers. Currently only the SROM RAM Offset Register
> > (vromoff) is exported.
> >
> > The GXP SOCLIB is a common library used for creating the common
> > "soc" class in the kernel.
> >
> > Signed-off-by: Clay Chang <[email protected]>
> > ---
> > drivers/soc/Kconfig | 1 +
> > drivers/soc/Makefile | 1 +
> > drivers/soc/hpe/Kconfig | 29 +++++++
> > drivers/soc/hpe/Makefile | 2 +
> > drivers/soc/hpe/gxp-soclib.c | 17 +++++
> > drivers/soc/hpe/gxp-soclib.h | 9 +++
> > drivers/soc/hpe/gxp-srom.c | 141 +++++++++++++++++++++++++++++++++++
> > 7 files changed, 200 insertions(+)
> > create mode 100644 drivers/soc/hpe/Kconfig
> > create mode 100644 drivers/soc/hpe/Makefile
> > create mode 100644 drivers/soc/hpe/gxp-soclib.c
> > create mode 100644 drivers/soc/hpe/gxp-soclib.h
> > create mode 100644 drivers/soc/hpe/gxp-srom.c
> >
> > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > index 5dbb09f843f7..faff0f036b61 100644
> > --- a/drivers/soc/Kconfig
> > +++ b/drivers/soc/Kconfig
> > @@ -10,6 +10,7 @@ source "drivers/soc/bcm/Kconfig"
> > source "drivers/soc/canaan/Kconfig"
> > source "drivers/soc/fsl/Kconfig"
> > source "drivers/soc/fujitsu/Kconfig"
> > +source "drivers/soc/hpe/Kconfig"
> > source "drivers/soc/imx/Kconfig"
> > source "drivers/soc/ixp4xx/Kconfig"
> > source "drivers/soc/litex/Kconfig"
> > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > index fff513bd522d..d257b9d654b3 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_MACH_DOVE) += dove/
> > obj-y += fsl/
> > obj-y += fujitsu/
> > obj-$(CONFIG_ARCH_GEMINI) += gemini/
> > +obj-$(CONFIG_ARCH_HPE) += hpe/
> > obj-y += imx/
> > obj-y += ixp4xx/
> > obj-$(CONFIG_SOC_XWAY) += lantiq/
> > diff --git a/drivers/soc/hpe/Kconfig b/drivers/soc/hpe/Kconfig
> > new file mode 100644
> > index 000000000000..88f5d46b06b6
> > --- /dev/null
> > +++ b/drivers/soc/hpe/Kconfig
> > @@ -0,0 +1,29 @@
> > +#
> > +# HPE GXP SoC drivers
> > +#
> > +menu "HPE GXP SoC drivers"
> > + depends on ARCH_HPE || COMPILE_TEST
> > +
> > +config HPE_GXP_SOCLIB
> > + bool "GXP Common SoC Library"
> > + default y
> > + depends on ARCH_HPE_GXP || COMPILE_TEST
> > + help
> > + This is for the common library for all HPE SoC drivers. It
> > + creates the root soc class (/sys/class/soc) for all GXP SoC
> > + drivers. It must be yes if any one of the GXP SoC drivers were
> > + added, so the config of all GXP SoC drivers must select this.
>
> Don't open-code Kconfig dependencies in free-form text. IOW, drop last
> sentence.
Understood, I will correct this part in the next revision.
>
> > +
> > +
> > +config HPE_GXP_SROM
> > + bool "GXP SROM Configuration Driver"
> > + default y
> > + depends on ARCH_HPE_GXP || COMPILE_TEST
> > + select HPE_GXP_SOCLIB
> > + help
> > + Say yes here to add support for SROM Configuration. The GXP SROM
> > + control register can be used to configure LPC related legacy I/O
> > + registers. Currently only the SROM RAM Offset Register (vromoff)
> > + is exported.
> > +
> > +endmenu
> > diff --git a/drivers/soc/hpe/Makefile b/drivers/soc/hpe/Makefile
> > new file mode 100644
> > index 000000000000..78de24ecb606
> > --- /dev/null
> > +++ b/drivers/soc/hpe/Makefile
> > @@ -0,0 +1,2 @@
> > +obj-$(CONFIG_HPE_GXP_SOCLIB) += gxp-soclib.o
> > +obj-$(CONFIG_HPE_GXP_SROM) += gxp-srom.o
> > diff --git a/drivers/soc/hpe/gxp-soclib.c b/drivers/soc/hpe/gxp-soclib.c
> > new file mode 100644
> > index 000000000000..11b0afe09070
> > --- /dev/null
> > +++ b/drivers/soc/hpe/gxp-soclib.c
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (C) 2023 Hewlett Packard Enteprise Development Company, L.P. */
> > +
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +
> > +struct class *soc_class;
> > +
> > +static int __init gxp_soclib_init(void)
> > +{
> > + soc_class = class_create(THIS_MODULE, "soc");
> > + if (IS_ERR(soc_class))
> > + return PTR_ERR(soc_class);
> > + return 0;
> > +}
> > +
> > +module_init(gxp_soclib_init);
>
> I don't see a point of making it a shared object and a "kernel library".
> Module inits are not libraries. Drop entire file.
Sure, I will re-write this part in the next revision.
>
> > diff --git a/drivers/soc/hpe/gxp-soclib.h b/drivers/soc/hpe/gxp-soclib.h
> > new file mode 100644
> > index 000000000000..eb0e72b67aee
> > --- /dev/null
> > +++ b/drivers/soc/hpe/gxp-soclib.h
> > @@ -0,0 +1,9 @@
>
>
> > +
> > +static int sysfs_register(struct device *parent,
> > + struct gxp_srom_drvdata *drvdata)
> > +{
> > + struct device *dev;
> > +
> > + dev = device_create_with_groups(soc_class, parent, 0,
> > + drvdata, srom_groups, "srom");
> > + if (IS_ERR(dev))
> > + return PTR_ERR(dev);
> > + drvdata->dev = dev;
> > + return 0;
> > +}
> > +
> > +static int gxp_srom_probe(struct platform_device *pdev)
> > +{
> > + struct gxp_srom_drvdata *drvdata;
> > + struct resource *res;
> > + int ret;
> > +
> > + drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_srom_drvdata),
>
> sizeof(*)
Ok, I will correct this in the next.
>
> > + GFP_KERNEL);
> > + if (!drvdata)
> > + return -ENOMEM;
> > +
> > + drvdata->pdev = pdev;
> > + platform_set_drvdata(pdev, drvdata);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res) {
> > + dev_err(&pdev->dev, "no srom resource defined\n");
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + drvdata->base = devm_ioremap_resource(&pdev->dev, res);
>
> Combine these two. There is a helper for this.
>
Sure, I will combine these two with devm_platform_ioremap_resource.
> > + if (IS_ERR(drvdata->base)) {
> > + dev_err(&pdev->dev, "ioremap_resource error\n");
> > + ret = PTR_ERR(drvdata->base);
>
> return dev_err_probe().
> > + goto ioremap_err;
> > + }
> > +
> > + mutex_init(&drvdata->mutex);
> > +
> > + ret = sysfs_register(&pdev->dev, drvdata);
>
> Missing ABI documentation.
Ok, I will provide with the proper ABI documentation in the next revision.
>
> > + if (ret != 0) {
> > + dev_err(&pdev->dev, "sysfs error\n");
>
> return dev_err_probe
Ok, will correct this in the next.
>
> > + goto sysfs_err;
> > + }
> > +
> > + dev_info(&pdev->dev, "initialized\n");
>
> Drop silly probe successes.
Sure, will drop this in the next revision.
>
> > + return 0;
> > +
> > +sysfs_err:
> > +ioremap_err:
> > + platform_set_drvdata(pdev, NULL);
> > +out:
> > + if (drvdata)
> > + devm_kfree(&pdev->dev, (void *)drvdata);
>
> 1. Why? if it is here, it must be in remove() callback. If it is invalid
> (because it is really not correct) for remove(), it cannot be here, right?
> 2. Why cast?
Ok, sorry for my mis-understanding about the error handling path in
device probing. I will re-write this part in the next revision.
>
> > + return ret;
> > +}
> > +
> > +static const struct of_device_id gxp_srom_of_match[] = {
> > + { .compatible = "hpe,gxp-srom" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, gxp_srom_of_match);
> > +
> > +static struct platform_driver gxp_srom_driver = {
> > + .probe = gxp_srom_probe,
> > + .driver = {
> > + .name = "gxp-srom",
> > + .of_match_table = of_match_ptr(gxp_srom_of_match),
>
> That will cause a warning. Drop of_match_ptr.
Got it!
>
> > + },
> > +};
> > +module_platform_driver(gxp_srom_driver);
> > +
> > +MODULE_AUTHOR("Clay Chang <[email protected]>");
> > +MODULE_DESCRIPTION("HPE GXP SROM Configuration Driver");
> > +MODULE_LICENSE("GPL");
>
> Best regards,
> Krzysztof
>
Thanks,
Clay
On Thu, Jan 12, 2023, at 14:16, Clay Chang wrote:
> On Tue, Jan 10, 2023 at 10:49:36AM +0100, Krzysztof Kozlowski wrote:
>> On 10/01/2023 05:25, [email protected] wrote:
>> > @@ -0,0 +1,36 @@
>> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> > +%YAML 1.2
>> > +---
>> > +$id: http://devicetree.org/schemas/soc/hpe/hpe,gxp-srom.yaml#
>> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> > +
>> > +title: HPE GXP SoC SROM Control Register
>> > +
>> > +maintainers:
>> > + - Clay Chang <[email protected]>
>> > +
>> > +description: |+
>> > + The SROM control register can be used to configure LPC related legacy
>> > + I/O registers.
>>
>> And why this is a hardware? No, you now add fake devices to be able to
>> write some stuff from user-space... Otherwise this needs proper hardware
>> description.
>
> Thank you for commenting on this. You are right, this is not a real
> hardware device, but simply exposes MMIO regions to the user-space.
> Maybe we should rewrite this as a syscon driver. Is writing a syscon
> driver a right direction?
There are two completely separate questions about the DT binding
and about the user-visible interface.
The binding needs to properly identify what this device is. I don't
think anyone without the datasheet can tell you the right answer
here, it really depends what the other registers do. If there are
lots of unrelated registers in a small area, a syscon might be
the right answer, but if they are all related to an external
memory bus, then categorizing it as a memory controller may
be more appropriate.
For the user interface side, I don't really like the idea of
having a hardware register directly exposed as driver in
drivers/soc, this generally makes it impossible to have portable
userspace that works across implementations of multiple SoC
vendors, and it makes it too easy to come up with an ad-hoc
interface to make a chip work for a particular use case when
a more general solution would be better.
Again, it's hard for me to tell why this even needs to be runtime
configurable, please try to describe what type of application
would access the sysfs interface here, and why this can't just
be set to a fixed value by bootloader or kernel without user
interaction.
Arnd
On Tue, Jan 10, 2023 at 10:49:36AM +0100, Krzysztof Kozlowski wrote:
> On 10/01/2023 05:25, [email protected] wrote:
> > From: Clay Chang <[email protected]>
> >
> > Document binding to support SROM driver in GXP.
> >
> > Signed-off-by: Clay Chang <[email protected]>
> > ---
> > .../bindings/soc/hpe/hpe,gxp-srom.yaml | 36 +++++++++++++++++++
> > 1 file changed, 36 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml
> > new file mode 100644
> > index 000000000000..14ad97d595c8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/hpe/hpe,gxp-srom.yaml
>
> Don't drop stuff to soc. Put it in respective directories. This also
> applies to your driver.
>
> SROM controllers go to memory-controllers. What is this, I have no clue.
> "SROM Control Register" is not helping me.
>
> > @@ -0,0 +1,36 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/hpe/hpe,gxp-srom.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: HPE GXP SoC SROM Control Register
> > +
> > +maintainers:
> > + - Clay Chang <[email protected]>
> > +
> > +description: |+
> > + The SROM control register can be used to configure LPC related legacy
> > + I/O registers.
>
> And why this is a hardware? No, you now add fake devices to be able to
> write some stuff from user-space... Otherwise this needs proper hardware
> description.
Thank you for commenting on this. You are right, this is not a real
hardware device, but simply exposes MMIO regions to the user-space.
Maybe we should rewrite this as a syscon driver. Is writing a syscon
driver a right direction?
>
> > +
> > +properties:
> > + compatible:
> > + items:
>
> Drop items, you have only one item.
>
> > + - const: hpe,gxp-srom
> > +
> > + reg:
> > + items:
> > + - description: SROM LPC Configuration Registers
>
> Drop items and description. Just maxItems: 1
>
> > +
> > +required:
> > + - compatible
> > + - reg
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + srom: srom@80fc0000 {
> > + compatible = "hpe,gxp-srom";
> > + reg = <0x80fc0000 0x100>;
> > + };
>
> Best regards,
> Krzysztof
>
Thanks,
Clay
On Tue, Jan 10, 2023 at 10:51:00AM +0100, Krzysztof Kozlowski wrote:
> On 10/01/2023 05:25, [email protected] wrote:
> > From: Clay Chang <[email protected]>
> >
> > Add Clay Chang as the maintainer of GXP SROM support.
>
> Your commit is not doing it. Nope. Either make proper entry matching
> this commit msg or make commit msg reflecting truth.
Thank you for the comment. I will address this correctly in the next
revision.
> >
> > Signed-off-by: Clay Chang <[email protected]>
> > ---
> > MAINTAINERS | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ea941dc469fa..164571ac1cc5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2279,14 +2279,22 @@ F: arch/arm/mach-sa1100/jornada720.c
> > ARM/HPE GXP ARCHITECTURE
> > M: Jean-Marie Verdun <[email protected]>
> > M: Nick Hawkins <[email protected]>
> > +M: Clay Chang <[email protected]>
>
>
> Best regards,
> Krzysztof
>
Thanks,
Clay
Hi Arnd,
Thank you for taking time to answer my questions.
On Thu, Jan 12, 2023 at 02:37:53PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 12, 2023, at 14:16, Clay Chang wrote:
> > On Tue, Jan 10, 2023 at 10:49:36AM +0100, Krzysztof Kozlowski wrote:
> >> On 10/01/2023 05:25, [email protected] wrote:
>
> >> > @@ -0,0 +1,36 @@
> >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> > +%YAML 1.2
> >> > +---
> >> > +$id: http://devicetree.org/schemas/soc/hpe/hpe,gxp-srom.yaml#
> >> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> > +
> >> > +title: HPE GXP SoC SROM Control Register
> >> > +
> >> > +maintainers:
> >> > + - Clay Chang <[email protected]>
> >> > +
> >> > +description: |+
> >> > + The SROM control register can be used to configure LPC related legacy
> >> > + I/O registers.
> >>
> >> And why this is a hardware? No, you now add fake devices to be able to
> >> write some stuff from user-space... Otherwise this needs proper hardware
> >> description.
> >
> > Thank you for commenting on this. You are right, this is not a real
> > hardware device, but simply exposes MMIO regions to the user-space.
> > Maybe we should rewrite this as a syscon driver. Is writing a syscon
> > driver a right direction?
>
> There are two completely separate questions about the DT binding
> and about the user-visible interface.
>
> The binding needs to properly identify what this device is. I don't
> think anyone without the datasheet can tell you the right answer
> here, it really depends what the other registers do. If there are
> lots of unrelated registers in a small area, a syscon might be
> the right answer, but if they are all related to an external
> memory bus, then categorizing it as a memory controller may
> be more appropriate.
Our use-cases are more like some register accesses not related to an
external memory bus, so syscon might be a better fit.
>
> For the user interface side, I don't really like the idea of
> having a hardware register directly exposed as driver in
> drivers/soc, this generally makes it impossible to have portable
> userspace that works across implementations of multiple SoC
> vendors, and it makes it too easy to come up with an ad-hoc
> interface to make a chip work for a particular use case when
> a more general solution would be better.
>
I agree with you. I have one question though: if we create a 'hpe'
directory under drivers/soc, and put all HPE BMC specific drivers there,
do you think this proper?
> Again, it's hard for me to tell why this even needs to be runtime
> configurable, please try to describe what type of application
> would access the sysfs interface here, and why this can't just
> be set to a fixed value by bootloader or kernel without user
> interaction.
The register is used for communication and synchronization between the
BMC and the host. During runtime, user-space daemons configures the
value of the register for interactions.
>
> Arnd
Thanks,
Clay
On Mon, Jan 16, 2023, at 14:42, Clay Chang wrote:
> On Thu, Jan 12, 2023 at 02:37:53PM +0100, Arnd Bergmann wrote:
>> On Thu, Jan 12, 2023, at 14:16, Clay Chang wrote:
>> For the user interface side, I don't really like the idea of
>> having a hardware register directly exposed as driver in
>> drivers/soc, this generally makes it impossible to have portable
>> userspace that works across implementations of multiple SoC
>> vendors, and it makes it too easy to come up with an ad-hoc
>> interface to make a chip work for a particular use case when
>> a more general solution would be better.
>>
>
> I agree with you. I have one question though: if we create a 'hpe'
> directory under drivers/soc, and put all HPE BMC specific drivers there,
> do you think this proper?
It certainly wouldn't be right to put "all HPE BMC specific drivers"
in there. Most drivers will fit into some existing subsystem, and
should be moved there instead. drivers/soc is used primarily for
drivers using soc_device_register() to provide information about the
soc, and we also use it as a place for drivers that just export
soc-specific helper functions that can be used by other drivers.
>> Again, it's hard for me to tell why this even needs to be runtime
>> configurable, please try to describe what type of application
>> would access the sysfs interface here, and why this can't just
>> be set to a fixed value by bootloader or kernel without user
>> interaction.
>
> The register is used for communication and synchronization between the
> BMC and the host. During runtime, user-space daemons configures the
> value of the register for interactions.
That does not sound very specific. What is the subsystem on the
host that this communicates with? Can you put the driver into the
same subsystem?
Arnd
On Mon, Jan 16, 2023 at 04:18:59PM +0100, Arnd Bergmann wrote:
> On Mon, Jan 16, 2023, at 14:42, Clay Chang wrote:
> > On Thu, Jan 12, 2023 at 02:37:53PM +0100, Arnd Bergmann wrote:
> >> On Thu, Jan 12, 2023, at 14:16, Clay Chang wrote:
> >> For the user interface side, I don't really like the idea of
> >> having a hardware register directly exposed as driver in
> >> drivers/soc, this generally makes it impossible to have portable
> >> userspace that works across implementations of multiple SoC
> >> vendors, and it makes it too easy to come up with an ad-hoc
> >> interface to make a chip work for a particular use case when
> >> a more general solution would be better.
> >>
> >
> > I agree with you. I have one question though: if we create a 'hpe'
> > directory under drivers/soc, and put all HPE BMC specific drivers there,
> > do you think this proper?
>
> It certainly wouldn't be right to put "all HPE BMC specific drivers"
> in there. Most drivers will fit into some existing subsystem, and
> should be moved there instead. drivers/soc is used primarily for
> drivers using soc_device_register() to provide information about the
> soc, and we also use it as a place for drivers that just export
> soc-specific helper functions that can be used by other drivers.
>
Sorry for not saying it clearly. I meant to put those HPE BMC related
drivers that are "not specific" to a particular subsystem in
drivers/soc/hpe. For those fit into some existing subsystems go to their
designated places.
> >> Again, it's hard for me to tell why this even needs to be runtime
> >> configurable, please try to describe what type of application
> >> would access the sysfs interface here, and why this can't just
> >> be set to a fixed value by bootloader or kernel without user
> >> interaction.
> >
> > The register is used for communication and synchronization between the
> > BMC and the host. During runtime, user-space daemons configures the
> > value of the register for interactions.
>
> That does not sound very specific. What is the subsystem on the
> host that this communicates with? Can you put the driver into the
> same subsystem?
>
> Arnd
This is a control register in the BMC chip that partially controls host
boot behaviors. When writing to the register, privileged mode is
required. That's why we rely on a kernel driver for writing to the
control register. And, there is no corresponding subsystem in the host
OS. For this case, is it acceptable to put this driver under
drivers/soc/hpe?
Thanks,
Clay
On Thu, Jan 19, 2023, at 08:39, Clay Chang wrote:
> On Mon, Jan 16, 2023 at 04:18:59PM +0100, Arnd Bergmann wrote:
>
> Sorry for not saying it clearly. I meant to put those HPE BMC related
> drivers that are "not specific" to a particular subsystem in
> drivers/soc/hpe. For those fit into some existing subsystems go to their
> designated places.
Ok, that makes sense. I'm just trying to reduce the number
of drivers that fit into this category.
>> >> Again, it's hard for me to tell why this even needs to be runtime
>> >> configurable, please try to describe what type of application
>> >> would access the sysfs interface here, and why this can't just
>> >> be set to a fixed value by bootloader or kernel without user
>> >> interaction.
>> >
>> > The register is used for communication and synchronization between the
>> > BMC and the host. During runtime, user-space daemons configures the
>> > value of the register for interactions.
>>
>> That does not sound very specific. What is the subsystem on the
>> host that this communicates with? Can you put the driver into the
>> same subsystem?
>
> This is a control register in the BMC chip that partially controls host
> boot behaviors. When writing to the register, privileged mode is
> required. That's why we rely on a kernel driver for writing to the
> control register. And, there is no corresponding subsystem in the host
> OS. For this case, is it acceptable to put this driver under
> drivers/soc/hpe?
I see. So this sounds like it might be generic BMC feature, which would
be nice to handle consistently for all BMC families. We had some
discussions about other BMC features in the past, but don't remember
what the overall consensus was, so I'm adding the openbmc list and
as well as aspeed and npcm maintainers to Cc.
The part I'm still missing is what type of userspace makes this
decision on the BMC side, and whether that might already have a
generalized interface. If there is, maybe part of that interface
can be abstracted in the kernel.
Arnd
On Tue, 10 Jan 2023, at 14:55, [email protected] wrote:
> From: Clay Chang <[email protected]>
>
> The GXP SROM control register can be used to configure LPC related
> legacy I/O registers. Currently only the SROM RAM Offset Register
> (vromoff) is exported.
What exact behaviour does vromoff influence? You mention I/O registers,
but RAM offset feels like it may be related to MEM or FWH LPC cycles
instead?
I'm trying to understand whether we can find some common ground with
controlling e.g. Aspeed's BMCs LPC peripherals based on Arnd's query[1],
but the description is a bit too vague right now for me to be able to do
that.
[1] https://lore.kernel.org/all/[email protected]/
Andrew
Hi Andrew,
Thank you for taking time, and sorry for my late response.
On Fri, Jan 20, 2023 at 12:51:54PM +1030, Andrew Jeffery wrote:
>
>
> On Tue, 10 Jan 2023, at 14:55, [email protected] wrote:
> > From: Clay Chang <[email protected]>
> >
> > The GXP SROM control register can be used to configure LPC related
> > legacy I/O registers. Currently only the SROM RAM Offset Register
> > (vromoff) is exported.
>
> What exact behaviour does vromoff influence? You mention I/O registers,
> but RAM offset feels like it may be related to MEM or FWH LPC cycles
> instead?
>
Sorry for my previous inaccurate description about the vromoff register.
You are right, it is not related to I/O but memory LPC cycles. This
register defines the offset and size to BMC's memory for the system rom.
BMC uses it for system rom related operations. One way to access this is
through the memory LPC cycles.
> I'm trying to understand whether we can find some common ground with
> controlling e.g. Aspeed's BMCs LPC peripherals based on Arnd's query[1],
> but the description is a bit too vague right now for me to be able to do
> that.
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> Andrew
Thanks,
Clay
Hi Andrew,
On Tue, Jan 31, 2023 at 09:46:42PM +0800, Clay Chang wrote:
> > I'm trying to understand whether we can find some common ground with
> > controlling e.g. Aspeed's BMCs LPC peripherals based on Arnd's query[1],
> > but the description is a bit too vague right now for me to be able to do
> > that.
> >
> > [1] https://lore.kernel.org/all/[email protected]/
> >
> > Andrew
I briefly studied driver/soc/aspeed/aspeed-lpc-ctrl.c, and IMO the
similarity between HPE GXP driver and Aspeed's could be that we both
expose the LPC memory addresses or registers for configuration purposes.
However, the functions to be configured could vary. There are a few sets
of registers that HPE wants to expose for configuration in the future.
So, do you think there could be some common grounds for genralization?
Please let me know if you need more information.
Thanks,
Clay
On Wed, 1 Feb 2023, at 23:58, Clay Chang wrote:
> Hi Andrew,
>
> On Tue, Jan 31, 2023 at 09:46:42PM +0800, Clay Chang wrote:
>> > I'm trying to understand whether we can find some common ground with
>> > controlling e.g. Aspeed's BMCs LPC peripherals based on Arnd's query[1],
>> > but the description is a bit too vague right now for me to be able to do
>> > that.
>> >
>> > [1] https://lore.kernel.org/all/[email protected]/
>> >
>> > Andrew
>
> I briefly studied driver/soc/aspeed/aspeed-lpc-ctrl.c, and IMO the
> similarity between HPE GXP driver and Aspeed's could be that we both
> expose the LPC memory addresses or registers for configuration purposes.
> However, the functions to be configured could vary. There are a few sets
> of registers that HPE wants to expose for configuration in the future.
The talk of "exposing registers" feels concerning - we're trying not to
do that directly. Instead we want to lift out an API that constrains
the behaviour a bit but works for both of us if there's overlap in
functionality.
Can you point to any documentation of the behaviour of your hardware?
It's still a little vague to me.
Andrew
On Thu, Feb 02, 2023 at 11:42:49AM +1030, Andrew Jeffery wrote:
>
>
> On Wed, 1 Feb 2023, at 23:58, Clay Chang wrote:
> > Hi Andrew,
> >
> > On Tue, Jan 31, 2023 at 09:46:42PM +0800, Clay Chang wrote:
> >> > I'm trying to understand whether we can find some common ground with
> >> > controlling e.g. Aspeed's BMCs LPC peripherals based on Arnd's query[1],
> >> > but the description is a bit too vague right now for me to be able to do
> >> > that.
> >> >
> >> > [1] https://lore.kernel.org/all/[email protected]/
> >> >
> >> > Andrew
> >
> > I briefly studied driver/soc/aspeed/aspeed-lpc-ctrl.c, and IMO the
> > similarity between HPE GXP driver and Aspeed's could be that we both
> > expose the LPC memory addresses or registers for configuration purposes.
> > However, the functions to be configured could vary. There are a few sets
> > of registers that HPE wants to expose for configuration in the future.
>
> The talk of "exposing registers" feels concerning - we're trying not to
> do that directly. Instead we want to lift out an API that constrains
> the behaviour a bit but works for both of us if there's overlap in
> functionality.
>
Let me describe it more clearly. We don't expose the registers directly
to the user. We describe those registers in the device tree, and then
the driver exposes them though sysfs interfaces. Users access the
registers through the device attributes defined under the sysfs
structure (e.g. /sys/class/soc/srom/vromoff) exposed by the driver. In
the show/store function pair, we get or set the regmap or memory,
validate input, do sanity check, synchronization, and so on.
I learned that in the drivers/soc/aspeed/aspeed-lpc-ctrl.c, both mmap
and ioctl were used, and revelant ioctl commands were defined in
include/uapi/linux/aspeed-lpc-ctrl.h. Is this what you mean an API for
aspeed? And we are trying to see if there are commonalities among us?
If yes, yeah I think it is good to see a common abstraction for BMC
chips.
Accroding to [1], I'd comment that, in terms of flash update, what we
want to do is similar to what was described in [1]. The SROM driver I
am working on partially layouts the registers for flash update for HPE
GXP hardware.
BTW, the way for user to access the registers is flexible - ioctl should
work as well.
> Can you point to any documentation of the behaviour of your hardware?
> It's still a little vague to me.
>
> Andrew
I wish I could, but there is not yet a technical document or datasheet
available to the public. Sorry.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?h=v6.2-rc4&id=6c4e976785011dfbe461821d0bfc58cfd60eac56
Thanks,
Clay