LinuxLists.cc - [PATCH v4 0/5] ARM: Add GPIO support

2023-06-21 21:37:44

Subject: [PATCH v4 0/5] ARM: Add GPIO support

2023-06-21 21:38:28

Subject: [PATCH v4 5/5] MAINTAINERS: hpe: Add GPIO

From: Nick Hawkins <[email protected]>

List the files added for GPIO.

Signed-off-by: Nick Hawkins <[email protected]>

---

v4:
*No change
v3:
*No change
v2:
*Removed reference to PSU changes as they have been discarded.
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a3b14ec33830..6157d9466a58 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2239,6 +2239,7 @@ M: Nick Hawkins <[email protected]>
S: Maintained
F: Documentation/hwmon/gxp-fan-ctrl.rst
F: Documentation/devicetree/bindings/arm/hpe,gxp.yaml
+F: Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
F: Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
F: Documentation/devicetree/bindings/i2c/hpe,gxp-i2c.yaml
F: Documentation/devicetree/bindings/spi/hpe,gxp-spifi.yaml
@@ -2247,6 +2248,7 @@ 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/gpio/gpio-gxp.c
F: drivers/hwmon/gxp-fan-ctrl.c
F: drivers/i2c/busses/i2c-gxp.c
F: drivers/spi/spi-gxp.c
--
2.17.1

2023-06-21 21:46:52

by Hawkins, Nick

[permalink] [raw]

Subject: [PATCH v4 1/5] gpio: gxp: Add HPE GXP GPIO

From: Nick Hawkins <[email protected]>

The GXP SoC supports GPIO on multiple interfaces. The interfaces are
CPLD and Host. The GPIOs is a combination of both physical and virtual
I/O across the interfaces. The gpio-gxp driver specifically covers the
CSM(physical), FN2(virtual), and VUHC(virtual) which are the host. The
driver supports interrupts from the host.

Signed-off-by: Nick Hawkins <[email protected]>

---

v4:
*Moved gpio-gxp-pl.c to a separate commit.
*Moved regmap_config out of function and made it static const
*Removed unnecessary variables
*Removed redundant conditional
*Modified regmap_read switch statements to calculate offset and mask
then read at end.
*Removed use of -EOPNOTSUPP
*Removed redundant casting
*Switched generic_handle_irq -> generic_handle_domain_irq
*Used GENMASK where applicable
*Used bitmap_xor and for_each_bit_set
*Made GPIO chip const and marked as a template (in the name)
*Made irq_chip const and immutable
*Removed casting in one case
*Corrected check on devm_gpiochip_add_data
*Remove dev_err_probe on platform_get_irq
*Changed return 0 to devm_request_irq
v3:
*Remove shared variables with gxp-fan-ctrl
v2:
*Separated code into two files to keep size down: gpio-gxp.c and
gpio-gxp-pl.c
*Fixed Kconfig indentation as well as add new entry for gpio-gxp-pl
*Removed use of linux/of.h and linux/of_device.h
*Added mod_devicetable.h and property.h
*Fixed indentation of defines and uses consistent number of digits
*Corrected defines with improper GPIO_ namespace.
*For masks now use BIT()
*Added comment for PLREG offsets
*Move gpio_chip to be first in structure
*Calculate offset for high and low byte GPIO reads instead of having
H(High) and L(Low) letters added to the variables.
*Removed repeditive use of "? 1 : 0"
*Switched to handle_bad_irq()
*Removed improper bailout on gpiochip_add_data
*Used GENMASK to arm interrupts
*Removed use of of_match_device
*fixed sizeof in devm_kzalloc
*Added COMPILE_TEST to Kconfig
*Added dev_err_probe
*Removed unecessary parent and compatible checks
---
drivers/gpio/Kconfig | 9 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-gxp.c | 573 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 583 insertions(+)
create mode 100644 drivers/gpio/gpio-gxp.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 13be729710f2..fa0c9fdbb50c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1235,6 +1235,15 @@ config HTC_EGPIO
several HTC phones. It provides basic support for input
pins, output pins, and IRQs.

+config GPIO_GXP
+ tristate "GXP GPIO support"
+ depends on ARCH_HPE_GXP || COMPILE_TEST
+ select GPIOLIB_IRQCHIP
+ help
+ Say Y here to support GXP GPIO controllers. It provides
+ support for the multiple GPIO interfaces available to be
+ available to the Host.
+
config GPIO_JANZ_TTL
tristate "Janz VMOD-TTL Digital IO Module"
depends on MFD_JANZ_CMODIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c048ba003367..a7ce0ab097aa 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_GPIO_FTGPIO010) += gpio-ftgpio010.o
obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
obj-$(CONFIG_GPIO_GPIO_MM) += gpio-gpio-mm.o
obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
+obj-$(CONFIG_GPIO_GXP) += gpio-gxp.o
obj-$(CONFIG_GPIO_GW_PLD) += gpio-gw-pld.o
obj-$(CONFIG_GPIO_HISI) += gpio-hisi.o
obj-$(CONFIG_GPIO_HLWD) += gpio-hlwd.o
diff --git a/drivers/gpio/gpio-gxp.c b/drivers/gpio/gpio-gxp.c
new file mode 100644
index 000000000000..4fe086137e86
--- /dev/null
+++ b/drivers/gpio/gpio-gxp.c
@@ -0,0 +1,573 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */
+
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+#define GPIDAT 0x040
+#define GPODAT 0x0b0
+#define GPODAT2 0x0f8
+#define GPOOWN 0x110
+#define GPOOWN2 0x118
+#define ASR_OFS 0x05c
+#define VUHC_OFS 0x064
+
+#define GXP_GPIO_DIR_OUT 0
+#define GXP_GPIO_DIR_IN 1
+
+#define PGOOD_MASK BIT(0)
+
+struct gxp_gpio_drvdata {
+ struct gpio_chip chip;
+ struct regmap *csm_map;
+ void __iomem *fn2_vbtn;
+ struct regmap *fn2_stat;
+ struct regmap *vuhc0_map;
+ int irq;
+};
+
+/*
+ * Note: Instead of definining all PINs here are the select few that
+ * are specifically defined in DTS and offsets are used here.
+ */
+enum gxp_gpio_pn {
+ RESET = 192,
+ VPBTN = 210, /* aka POWER_OK */
+ PGOOD = 211, /* aka PS_PWROK */
+ PERST = 212, /* aka PCIERST */
+ POST_COMPLETE = 213,
+};
+
+static int gxp_gpio_csm_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+ int ret = 0;
+ unsigned int reg_offset;
+ unsigned int reg_mask;
+
+ switch (offset) {
+ case 0 ... 31:
+ reg_offset = GPIDAT;
+ reg_mask = BIT(offset);
+ break;
+ case 32 ... 63:
+ reg_offset = GPIDAT + 0x20;
+ reg_mask = BIT(offset - 32);
+ break;
+ case 64 ... 95:
+ reg_offset = GPODAT;
+ reg_mask = BIT(offset - 64);
+ break;
+ case 96 ... 127:
+ reg_offset = GPODAT + 0x04;
+ reg_mask = BIT(offset - 96);
+ break;
+ case 128 ... 159:
+ reg_offset = GPODAT2;
+ reg_mask = BIT(offset - 128);
+ break;
+ case 160 ... 191:
+ reg_offset = GPODAT2 + 0x04;
+ reg_mask = BIT(offset - 160);
+ break;
+ case RESET:
+ /* SW_RESET */
+ reg_offset = ASR_OFS;
+ reg_mask = BIT(15);
+ break;
+ default:
+ break;
+ }
+
+ regmap_read(drvdata->csm_map, reg_offset, &ret);
+ ret = (ret & reg_mask) ? 1 : 0;
+
+ return ret;
+}
+
+static void gxp_gpio_csm_set(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+ u32 tmp;
+
+ switch (offset) {
+ case 64 ... 95:
+ /* Keep ownership setting */
+ regmap_read(drvdata->csm_map, GPOOWN, &tmp);
+ tmp = (tmp & BIT(offset - 64)) ? 1 : 0;
+
+ regmap_update_bits(drvdata->csm_map, GPOOWN,
+ BIT(offset - 64), BIT(offset - 64));
+ regmap_update_bits(drvdata->csm_map, GPODAT,
+ BIT(offset - 64), value ? BIT(offset - 64) : 0);
+
+ /* Restore ownership setting */
+ regmap_update_bits(drvdata->csm_map, GPOOWN,
+ BIT(offset - 64), tmp ? BIT(offset - 64) : 0);
+ break;
+ case 96 ... 127:
+ /* Keep ownership setting */
+ regmap_read(drvdata->csm_map, GPOOWN + 0x04, &tmp);
+ tmp = (tmp & BIT(offset - 96)) ? 1 : 0;
+
+ regmap_update_bits(drvdata->csm_map, GPOOWN + 0x04,
+ BIT(offset - 96), BIT(offset - 96));
+ regmap_update_bits(drvdata->csm_map, GPODAT + 0x04,
+ BIT(offset - 96), value ? BIT(offset - 96) : 0);
+
+ /* Restore ownership setting */
+ regmap_update_bits(drvdata->csm_map, GPOOWN + 0x04,
+ BIT(offset - 96), tmp ? BIT(offset - 96) : 0);
+ break;
+ case 128 ... 159:
+ /* Keep ownership setting */
+ regmap_read(drvdata->csm_map, GPOOWN2, &tmp);
+ tmp = (tmp & BIT(offset - 128)) ? 1 : 0;
+
+ regmap_update_bits(drvdata->csm_map, GPOOWN2,
+ BIT(offset - 128), BIT(offset - 128));
+ regmap_update_bits(drvdata->csm_map, GPODAT2,
+ BIT(offset - 128), value ? BIT(offset - 128) : 0);
+
+ /* Restore ownership setting */
+ regmap_update_bits(drvdata->csm_map, GPOOWN2,
+ BIT(offset - 128), tmp ? BIT(offset - 128) : 0);
+ break;
+ case 160 ... 191:
+ /* Keep ownership setting */
+ regmap_read(drvdata->csm_map, GPOOWN2 + 0x04, &tmp);
+ tmp = (tmp & BIT(offset - 160)) ? 1 : 0;
+
+ regmap_update_bits(drvdata->csm_map, GPOOWN2 + 0x04,
+ BIT(offset - 160), BIT(offset - 160));
+ regmap_update_bits(drvdata->csm_map, GPODAT2 + 0x04,
+ BIT(offset - 160), value ? BIT(offset - 160) : 0);
+
+ /* Restore ownership setting */
+ regmap_update_bits(drvdata->csm_map, GPOOWN2 + 0x04,
+ BIT(offset - 160), tmp ? BIT(offset - 160) : 0);
+ break;
+ case 192:
+ if (value) {
+ regmap_update_bits(drvdata->csm_map, ASR_OFS,
+ BIT(0), BIT(0));
+ regmap_update_bits(drvdata->csm_map, ASR_OFS,
+ BIT(15), BIT(15));
+ } else {
+ regmap_update_bits(drvdata->csm_map, ASR_OFS,
+ BIT(15), 0);
+ }
+ break;
+ default:
+ break;
+ }
+}
+
+static int gxp_gpio_csm_get_direction(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ switch (offset) {
+ case 0 ... 63:
+ return GXP_GPIO_DIR_IN;
+ case 64 ... 191:
+ return GXP_GPIO_DIR_OUT;
+ case 192 ... 193:
+ return GXP_GPIO_DIR_OUT;
+ case 194:
+ return GXP_GPIO_DIR_IN;
+ default:
+ return -ENOTSUPP;
+ }
+}
+
+static int gxp_gpio_csm_direction_input(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ switch (offset) {
+ case 0 ... 63:
+ return 0;
+ case 194:
+ return 0;
+ default:
+ return -ENOTSUPP;
+ }
+}
+
+static int gxp_gpio_csm_direction_output(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ switch (offset) {
+ case 64 ... 191:
+ case 192 ... 193:
+ gxp_gpio_csm_set(chip, offset, value);
+ return 0;
+ default:
+ return -ENOTSUPP;
+ }
+}
+
+static int gxp_gpio_vuhc_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+ unsigned int val;
+ int ret = 0;
+
+ if (offset < 8) {
+ regmap_read(drvdata->vuhc0_map, VUHC_OFS + 4 * offset, &val);
+ ret = (val & BIT(13)) ? 1 : 0;
+ }
+
+ return ret;
+}
+
+static void gxp_gpio_vuhc_set(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ /* Currently we are not supporting setting of these values yet */
+ switch (offset) {
+ default:
+ break;
+ }
+}
+
+static int gxp_gpio_vuhc_get_direction(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ switch (offset) {
+ case 0:
+ case 1:
+ case 2:
+ return GXP_GPIO_DIR_IN;
+ default:
+ return -ENOTSUPP;
+ }
+}
+
+static int gxp_gpio_vuhc_direction_input(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ switch (offset) {
+ case 0:
+ case 1:
+ case 2:
+ return 0;
+ default:
+ return -ENOTSUPP;
+ }
+}
+
+static int gxp_gpio_vuhc_direction_output(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ switch (offset) {
+ default:
+ return -ENOTSUPP;
+ }
+}
+
+static int gxp_gpio_fn2_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+ unsigned int val;
+ int ret = 0;
+ unsigned int reg_mask;
+
+ switch (offset) {
+ case PGOOD:
+ regmap_read(drvdata->fn2_stat, 0, &val);
+ reg_mask = BIT(24);
+
+ break;
+ case PERST:
+ regmap_read(drvdata->fn2_stat, 0, &val);
+ reg_mask = BIT(25);
+
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ regmap_read(drvdata->fn2_stat, 0, &val);
+ ret = (val & reg_mask);
+ /* Return either 1 or 0 */
+ return ret ? 1 : 0;
+}
+
+static void gxp_gpio_fn2_set(struct gpio_chip *chip, unsigned int offset,
+ int value)
+{
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+
+ switch (offset) {
+ case VPBTN:
+ writeb(1, drvdata->fn2_vbtn);
+ break;
+ default:
+ break;
+ }
+}
+
+static int gxp_gpio_fn2_get_direction(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ switch (offset) {
+ case VPBTN:
+ return GXP_GPIO_DIR_OUT;
+ default:
+ return GXP_GPIO_DIR_IN;
+ }
+}
+
+static int gxp_gpio_fn2_direction_input(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ switch (offset) {
+ case PGOOD:
+ case PERST:
+ case POST_COMPLETE:
+ return 0;
+ default:
+ return -ENOTSUPP;
+ }
+}
+
+static int gxp_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+ if (offset < 200)
+ return gxp_gpio_csm_get(chip, offset);
+ else if (offset >= 200 && offset < 210)
+ return gxp_gpio_vuhc_get(chip, offset - 200);
+ else if (offset >= 210)
+ return gxp_gpio_fn2_get(chip, offset);
+
+ return 0;
+}
+
+static void gxp_gpio_set(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ if (offset < 200)
+ gxp_gpio_csm_set(chip, offset, value);
+ else if (offset >= 200 && offset < 210)
+ gxp_gpio_vuhc_set(chip, offset - 200, value);
+ else if (offset >= 210)
+ gxp_gpio_fn2_set(chip, offset, value);
+}
+
+static int gxp_gpio_get_direction(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ if (offset < 200)
+ return gxp_gpio_csm_get_direction(chip, offset);
+ else if (offset >= 200 && offset < 210)
+ return gxp_gpio_vuhc_get_direction(chip, offset - 200);
+ else if (offset >= 210)
+ return gxp_gpio_fn2_get_direction(chip, offset);
+
+ return 0;
+}
+
+static int gxp_gpio_direction_input(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ if (offset < 200)
+ return gxp_gpio_csm_direction_input(chip, offset);
+ else if (offset >= 200 && offset < 210)
+ return gxp_gpio_vuhc_direction_input(chip, offset - 200);
+ else if (offset >= 210)
+ return gxp_gpio_fn2_direction_input(chip, offset);
+
+ return 0;
+}
+
+static int gxp_gpio_direction_output(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ if (offset < 200)
+ return gxp_gpio_csm_direction_output(chip, offset, value);
+ else if (offset >= 200 && offset < 210)
+ return gxp_gpio_vuhc_direction_output(chip, offset - 200, value);
+
+ return 0;
+}
+
+static const struct regmap_config gxp_regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .name = "gxp",
+};
+
+static struct regmap *gxp_gpio_init_regmap(struct platform_device *pdev,
+ char *reg_name)
+{
+ void __iomem *base;
+
+ base = devm_platform_ioremap_resource_byname(pdev, reg_name);
+ if (IS_ERR(base))
+ return ERR_CAST(base);
+
+ return devm_regmap_init_mmio(&pdev->dev, base, &gxp_regmap_config);
+}
+
+static void gxp_gpio_fn2_irq_ack(struct irq_data *d)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+ unsigned int val;
+
+ /* Read latched interrupt */
+ regmap_read(drvdata->fn2_stat, 0, &val);
+ /* Clear latched interrupt */
+ regmap_update_bits(drvdata->fn2_stat, 0,
+ GENMASK(15, 0), GENMASK(15, 0));
+}
+
+#define FN2_SEVMASK BIT(2)
+static void gxp_gpio_fn2_irq_set_mask(struct irq_data *d, bool set)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+
+ regmap_update_bits(drvdata->fn2_stat, FN2_SEVMASK,
+ BIT(0), set ? BIT(0) : 0);
+}
+
+static void gxp_gpio_fn2_irq_mask(struct irq_data *d)
+{
+ gxp_gpio_fn2_irq_set_mask(d, false);
+}
+
+static void gxp_gpio_fn2_irq_unmask(struct irq_data *d)
+{
+ gxp_gpio_fn2_irq_set_mask(d, true);
+}
+
+static int gxp_gpio_fn2_set_type(struct irq_data *d, unsigned int type)
+{
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ irq_set_handler_locked(d, handle_level_irq);
+ else
+ irq_set_handler_locked(d, handle_edge_irq);
+
+ return 0;
+}
+
+static irqreturn_t gxp_gpio_fn2_irq_handle(int irq, void *_drvdata)
+{
+ struct gxp_gpio_drvdata *drvdata = (struct gxp_gpio_drvdata *)_drvdata;
+ unsigned int val;
+
+ regmap_read(drvdata->fn2_stat, 0, &val);
+
+ if (val & PGOOD_MASK)
+ generic_handle_domain_irq(drvdata->chip.irq.domain, PGOOD);
+
+ return IRQ_HANDLED;
+}
+
+static const struct irq_chip gxp_gpio_irqchip = {
+ .name = "gxp_fn2",
+ .irq_ack = gxp_gpio_fn2_irq_ack,
+ .irq_mask = gxp_gpio_fn2_irq_mask,
+ .irq_unmask = gxp_gpio_fn2_irq_unmask,
+ .irq_set_type = gxp_gpio_fn2_set_type,
+ .flags = IRQCHIP_IMMUTABLE,
+};
+
+static const struct gpio_chip common_chip_template = {
+ .label = "gxp_gpio",
+ .owner = THIS_MODULE,
+ .get = gxp_gpio_get,
+ .set = gxp_gpio_set,
+ .get_direction = gxp_gpio_get_direction,
+ .direction_input = gxp_gpio_direction_input,
+ .direction_output = gxp_gpio_direction_output,
+ .base = 0,
+};
+
+static const struct of_device_id gxp_gpio_of_match[] = {
+ { .compatible = "hpe,gxp-gpio" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, gxp_gpio_of_match);
+
+static int gxp_gpio_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct gxp_gpio_drvdata *drvdata;
+ struct gpio_irq_chip *girq;
+
+ drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, drvdata);
+
+ drvdata->csm_map = gxp_gpio_init_regmap(pdev, "csm");
+ if (IS_ERR(drvdata->csm_map))
+ return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->csm_map),
+ "failed to map csm_handle\n");
+
+ drvdata->fn2_vbtn = devm_platform_ioremap_resource_byname(pdev, "fn2-vbtn");
+ if (IS_ERR(drvdata->fn2_vbtn))
+ return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->fn2_vbtn),
+ "failed to map fn2_vbtn\n");
+
+ drvdata->fn2_stat = gxp_gpio_init_regmap(pdev, "fn2-stat");
+ if (IS_ERR(drvdata->fn2_stat))
+ return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->fn2_stat),
+ "failed to map fn2_stat\n");
+
+ drvdata->vuhc0_map = gxp_gpio_init_regmap(pdev, "vuhc");
+ if (IS_ERR(drvdata->vuhc0_map))
+ return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->vuhc0_map),
+ "failed to map vuhc0_map\n");
+
+ girq = &drvdata->chip.irq;
+ gpio_irq_chip_set_chip(girq, &gxp_gpio_irqchip);
+ girq->parent_handler = NULL;
+ girq->num_parents = 0;
+ girq->parents = NULL;
+ girq->default_type = IRQ_TYPE_NONE;
+ girq->handler = handle_bad_irq;
+
+ ret = platform_get_irq(pdev, 0);
+ if (ret < 0)
+ return ret;
+
+ drvdata->irq = ret;
+
+ ret = devm_request_irq(&pdev->dev, drvdata->irq, gxp_gpio_fn2_irq_handle,
+ IRQF_SHARED, "gxp-fn2", drvdata);
+ if (ret < 0)
+ return ret;
+
+ drvdata->chip = common_chip_template;
+ drvdata->chip.ngpio = 220;
+
+ drvdata->chip.parent = &pdev->dev;
+
+ return devm_gpiochip_add_data(&pdev->dev, &drvdata->chip, NULL);
+}
+
+static struct platform_driver gxp_gpio_driver = {
+ .driver = {
+ .name = "gxp-gpio",
+ .of_match_table = gxp_gpio_of_match,
+ },
+ .probe = gxp_gpio_probe,
+};
+module_platform_driver(gxp_gpio_driver);
+
+MODULE_AUTHOR("Nick Hawkins <[email protected]>");
+MODULE_DESCRIPTION("GPIO interface for GXP");
+MODULE_LICENSE("GPL");
--
2.17.1

2023-06-21 21:55:29

by Hawkins, Nick

[permalink] [raw]

Subject: [PATCH v4 3/5] dt-bindings: hwmon: hpe,gxp-fan-ctrl: remove fn2 and pl registers

From: Nick Hawkins <[email protected]>

Reduce the hpe,gxp-fan-ctrl register references from 3 to 1. The
function2 (fn2) and programmable logic (pl) references are removed.
The purpose of removal being their functionality will be consumed by a
new GPIO driver.

Signed-off-by: Nick Hawkins <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>

---
v4:
*No change, added reviewed by
v3:
*Modify the subject.
*Remove mention of fan driver receiving data from GPIO as it is no
longer applicable
v2:
*Added more detailed subject and patch description
---
.../bindings/hwmon/hpe,gxp-fan-ctrl.yaml | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml b/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
index 4a52aac6be72..963aa640dc05 100644
--- a/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
+++ b/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
@@ -18,21 +18,12 @@ properties:
const: hpe,gxp-fan-ctrl

reg:
- items:
- - description: Fan controller PWM
- - description: Programmable logic
- - description: Function 2
-
- reg-names:
- items:
- - const: base
- - const: pl
- - const: fn2
+ description: Fan controller PWM
+ maxItems: 1

required:
- compatible
- reg
- - reg-names

additionalProperties: false

@@ -40,6 +31,5 @@ examples:
- |
fan-controller@1000c00 {
compatible = "hpe,gxp-fan-ctrl";
- reg = <0x1000c00 0x200>, <0xd1000000 0xff>, <0x80200000 0x100000>;
- reg-names = "base", "pl", "fn2";
+ reg = <0x1000c00 0x200>;
};
--
2.17.1

2023-06-21 21:57:51

by Hawkins, Nick

[permalink] [raw]

Subject: [PATCH v4 4/5] hwmon: (gxp_fan_ctrl) Provide fan info via gpio

From: Nick Hawkins <[email protected]>

The fan driver now is independent of the fan plreg GPIO information.
Therefore there will no longer be presence or fail information available
from the driver. Part of the changes includes removing a system power check
as the GPIO driver needs it to report power state to host.

Signed-off-by: Nick Hawkins <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>

---

v4:
*No change, Added Reviewed-by:
v3:
*Removed shared variable
*Removed GPIO dependency on Kconfig
*Removed present and failure checks surrounding Fans sysfs
v2:
*Removed use of shared functions to GPIO in favor of a shared variable
*Added build dependency on GXP GPIO driver.
---
drivers/hwmon/Kconfig | 2 +-
drivers/hwmon/gxp-fan-ctrl.c | 108 +----------------------------------
2 files changed, 4 insertions(+), 106 deletions(-)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 5b3b76477b0e..196ce88d2db9 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -721,7 +721,7 @@ config SENSORS_GXP_FAN_CTRL
If you say yes here you get support for GXP fan control functionality.

The GXP controls fan function via the CPLD through the use of PWM
- registers. This driver reports status and pwm setting of the fans.
+ registers. This driver enables pwm setting of the fans.

config SENSORS_HIH6130
tristate "Honeywell Humidicon HIH-6130 humidity/temperature sensor"
diff --git a/drivers/hwmon/gxp-fan-ctrl.c b/drivers/hwmon/gxp-fan-ctrl.c
index 0014b8b0fd41..55a10c7fc9d6 100644
--- a/drivers/hwmon/gxp-fan-ctrl.c
+++ b/drivers/hwmon/gxp-fan-ctrl.c
@@ -1,7 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */
+/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */

-#include <linux/bits.h>
#include <linux/err.h>
#include <linux/hwmon.h>
#include <linux/io.h>
@@ -9,52 +8,10 @@
#include <linux/of_device.h>
#include <linux/platform_device.h>

-#define OFS_FAN_INST 0 /* Is 0 because plreg base will be set at INST */
-#define OFS_FAN_FAIL 2 /* Is 2 bytes after base */
-#define OFS_SEVSTAT 0 /* Is 0 because fn2 base will be set at SEVSTAT */
-#define POWER_BIT 24
-
struct gxp_fan_ctrl_drvdata {
- void __iomem *base;
- void __iomem *plreg;
- void __iomem *fn2;
+ void __iomem *base;
};

-static bool fan_installed(struct device *dev, int fan)
-{
- struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
- u8 val;
-
- val = readb(drvdata->plreg + OFS_FAN_INST);
-
- return !!(val & BIT(fan));
-}
-
-static long fan_failed(struct device *dev, int fan)
-{
- struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
- u8 val;
-
- val = readb(drvdata->plreg + OFS_FAN_FAIL);
-
- return !!(val & BIT(fan));
-}
-
-static long fan_enabled(struct device *dev, int fan)
-{
- struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
- u32 val;
-
- /*
- * Check the power status as if the platform is off the value
- * reported for the PWM will be incorrect. Report fan as
- * disabled.
- */
- val = readl(drvdata->fn2 + OFS_SEVSTAT);
-
- return !!((val & BIT(POWER_BIT)) && fan_installed(dev, fan));
-}
-
static int gxp_pwm_write(struct device *dev, u32 attr, int channel, long val)
{
struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
@@ -81,37 +38,11 @@ static int gxp_fan_ctrl_write(struct device *dev, enum hwmon_sensor_types type,
}
}

-static int gxp_fan_read(struct device *dev, u32 attr, int channel, long *val)
-{
- switch (attr) {
- case hwmon_fan_enable:
- *val = fan_enabled(dev, channel);
- return 0;
- case hwmon_fan_fault:
- *val = fan_failed(dev, channel);
- return 0;
- default:
- return -EOPNOTSUPP;
- }
-}
-
static int gxp_pwm_read(struct device *dev, u32 attr, int channel, long *val)
{
struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
- u32 reg;

- /*
- * Check the power status of the platform. If the platform is off
- * the value reported for the PWM will be incorrect. In this case
- * report a PWM of zero.
- */
-
- reg = readl(drvdata->fn2 + OFS_SEVSTAT);
-
- if (reg & BIT(POWER_BIT))
- *val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0;
- else
- *val = 0;
+ *val = readb(drvdata->base + channel);

return 0;
}
@@ -120,8 +51,6 @@ static int gxp_fan_ctrl_read(struct device *dev, enum hwmon_sensor_types type,
u32 attr, int channel, long *val)
{
switch (type) {
- case hwmon_fan:
- return gxp_fan_read(dev, attr, channel, val);
case hwmon_pwm:
return gxp_pwm_read(dev, attr, channel, val);
default:
@@ -136,16 +65,6 @@ static umode_t gxp_fan_ctrl_is_visible(const void *_data,
umode_t mode = 0;

switch (type) {
- case hwmon_fan:
- switch (attr) {
- case hwmon_fan_enable:
- case hwmon_fan_fault:
- mode = 0444;
- break;
- default:
- break;
- }
- break;
case hwmon_pwm:
switch (attr) {
case hwmon_pwm_input:
@@ -169,15 +88,6 @@ static const struct hwmon_ops gxp_fan_ctrl_ops = {
};

static const struct hwmon_channel_info *gxp_fan_ctrl_info[] = {
- HWMON_CHANNEL_INFO(fan,
- HWMON_F_FAULT | HWMON_F_ENABLE,
- HWMON_F_FAULT | HWMON_F_ENABLE,
- HWMON_F_FAULT | HWMON_F_ENABLE,
- HWMON_F_FAULT | HWMON_F_ENABLE,
- HWMON_F_FAULT | HWMON_F_ENABLE,
- HWMON_F_FAULT | HWMON_F_ENABLE,
- HWMON_F_FAULT | HWMON_F_ENABLE,
- HWMON_F_FAULT | HWMON_F_ENABLE),
HWMON_CHANNEL_INFO(pwm,
HWMON_PWM_INPUT,
HWMON_PWM_INPUT,
@@ -212,18 +122,6 @@ static int gxp_fan_ctrl_probe(struct platform_device *pdev)
return dev_err_probe(dev, PTR_ERR(drvdata->base),
"failed to map base\n");

- drvdata->plreg = devm_platform_ioremap_resource_byname(pdev,
- "pl");
- if (IS_ERR(drvdata->plreg))
- return dev_err_probe(dev, PTR_ERR(drvdata->plreg),
- "failed to map plreg\n");
-
- drvdata->fn2 = devm_platform_ioremap_resource_byname(pdev,
- "fn2");
- if (IS_ERR(drvdata->fn2))
- return dev_err_probe(dev, PTR_ERR(drvdata->fn2),
- "failed to map fn2\n");
-
hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
"hpe_gxp_fan_ctrl",
drvdata,
--
2.17.1

2023-06-21 21:59:44

by Hawkins, Nick

[permalink] [raw]

Subject: [PATCH v4 2/5] gpio: gxp: Add HPE GXP GPIO PL

From: Nick Hawkins <[email protected]>

The GXP SoC supports GPIO on multiple interfaces. The interfaces are
CPLD and Host. The gpio-gxp-pl driver covers the CPLD which takes
physical I/O from the board and shares it with GXP via a proprietary
interface that maps the I/O onto a specific register area of the GXP.
This driver supports interrupts from the CPLD.

Signed-off-by: Nick Hawkins <[email protected]>

---
v4:
*New commit: Broke GPIO into two different commits one for each driver
*Explain psu_presence global
*Moved gpio_chip to first in gxp_gpio_drvdata
*Moved regmap_config out of function and made it static const
*Removed unnecessary variables
*Removed redundant conditional
*Modified regmap_read switch statements to calculate offset and mask
then read at end.
*Removed use of -EOPNOTSUPP
*Removed redundant casting
*Switched generic_handle_irq -> generic_handle_domain_irq
*Used GENMASK where applicable
*Used bitmap_xor and for_each_bit_set
*Made GPIO chip const and marked as a template (in the name)
*Made irq_chip const and immutable
*Removed casting in one case
*Corrected check on devm_gpiochip_add_data
*Remove dev_err_probe on platform_get_irq
*Changed return 0 to devm_request_irq
*Added debug FS call to enable reading of the server_id
v3:
*Did not exist
v:2
*Did not exist
v1:
*Did not exist
---
drivers/gpio/Kconfig | 9 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-gxp-pl.c | 582 +++++++++++++++++++++++++++++++++++++
3 files changed, 592 insertions(+)
create mode 100644 drivers/gpio/gpio-gxp-pl.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index fa0c9fdbb50c..b0a24ef18392 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1244,6 +1244,15 @@ config GPIO_GXP
support for the multiple GPIO interfaces available to be
available to the Host.

+config GPIO_GXP_PL
+ tristate "GXP GPIO PL support"
+ depends on ARCH_HPE_GXP || COMPILE_TEST
+ select GPIOLIB_IRQCHIP
+ help
+ Say Y here to support GXP GPIO PL controller. It provides
+ support for the GPIO PL interface available to be
+ available to the Host.
+
config GPIO_JANZ_TTL
tristate "Janz VMOD-TTL Digital IO Module"
depends on MFD_JANZ_CMODIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a7ce0ab097aa..1f285c630e15 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
obj-$(CONFIG_GPIO_GPIO_MM) += gpio-gpio-mm.o
obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
obj-$(CONFIG_GPIO_GXP) += gpio-gxp.o
+obj-$(CONFIG_GPIO_GXP_PL) += gpio-gxp-pl.o
obj-$(CONFIG_GPIO_GW_PLD) += gpio-gw-pld.o
obj-$(CONFIG_GPIO_HISI) += gpio-hisi.o
obj-$(CONFIG_GPIO_HLWD) += gpio-hlwd.o
diff --git a/drivers/gpio/gpio-gxp-pl.c b/drivers/gpio/gpio-gxp-pl.c
new file mode 100644
index 000000000000..8506e2a96da4
--- /dev/null
+++ b/drivers/gpio/gpio-gxp-pl.c
@@ -0,0 +1,582 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */
+
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+/* Specific offsets in CPLD registers for interrupts */
+#define PLREG_INT_GRP_STAT_MASK 0x08
+#define PLREG_INT_HI_PRI_EN 0x0C
+#define PLREG_INT_GRP5_BASE 0x31
+#define PLREG_INT_GRP6_BASE 0x35
+#define PLREG_INT_GRP5_FLAG 0x30
+#define PLREG_INT_GRP5_STATE 0x32
+#define PLREG_INT_GRP6_FLAG 0x34
+
+/* Specific bits to enable Group 4 and Group 5 interrupts */
+#define PLREG_GRP4_GRP5_MASK GENMASK(5, 4)
+
+/* Specific offsets in CPLD registers */
+#define PLREG_SERVER_ID 0x01 /* 2 Bytes */
+#define PLREG_IOP_LED 0x04
+#define PLREG_IDENT_LED 0x05
+#define PLREG_HEALTH_LED 0x0D
+#define PLREG_PSU_INST 0x19
+#define PLREG_PSU_AC 0x1B
+#define PLREG_PSU_DC 0x1C
+#define PLREG_FAN_INST 0x27
+#define PLREG_FAN_FAIL 0x29
+#define PLREG_SIDEBAND 0x40
+#define GXP_GPIO_DIR_OUT 0x00
+#define GXP_GPIO_DIR_IN 0x01
+
+enum pl_gpio_pn {
+ IOP_LED1 = 0,
+ IOP_LED2 = 1,
+ IOP_LED3 = 2,
+ IOP_LED4 = 3,
+ IOP_LED5 = 4,
+ IOP_LED6 = 5,
+ IOP_LED7 = 6,
+ IOP_LED8 = 7,
+ FAN1_INST = 8,
+ FAN2_INST = 9,
+ FAN3_INST = 10,
+ FAN4_INST = 11,
+ FAN5_INST = 12,
+ FAN6_INST = 13,
+ FAN7_INST = 14,
+ FAN8_INST = 15,
+ FAN1_FAIL = 16,
+ FAN2_FAIL = 17,
+ FAN3_FAIL = 18,
+ FAN4_FAIL = 19,
+ FAN5_FAIL = 20,
+ FAN6_FAIL = 21,
+ FAN7_FAIL = 22,
+ FAN8_FAIL = 23,
+ LED_IDENTIFY = 24,
+ LED_HEALTH_RED = 25,
+ LED_HEALTH_AMBER = 26,
+ PWR_BTN_INT = 27,
+ UID_PRESS_INT = 28,
+ SLP_INT = 29,
+ ACM_FORCE_OFF = 30,
+ ACM_REMOVED = 31,
+ ACM_REQ_N = 32,
+ PSU1_INST = 33,
+ PSU2_INST = 34,
+ PSU3_INST = 35,
+ PSU4_INST = 36,
+ PSU5_INST = 37,
+ PSU6_INST = 38,
+ PSU7_INST = 39,
+ PSU8_INST = 40,
+ PSU1_AC = 41,
+ PSU2_AC = 42,
+ PSU3_AC = 43,
+ PSU4_AC = 44,
+ PSU5_AC = 45,
+ PSU6_AC = 46,
+ PSU7_AC = 47,
+ PSU8_AC = 48,
+ PSU1_DC = 49,
+ PSU2_DC = 50,
+ PSU3_DC = 51,
+ PSU4_DC = 52,
+ PSU5_DC = 53,
+ PSU6_DC = 54,
+ PSU7_DC = 55,
+ PSU8_DC = 56,
+ RESET = 57,
+ NMI_OUT = 58,
+ VPBTN = 59,
+ PGOOD = 60,
+ PERST = 61,
+ POST_COMPLETE = 62,
+ SIDEBAND_SEL_L = 63,
+ SIDEBAND_SEL_H = 64
+};
+
+/*
+ * When an interrupt fires for a PSU config change
+ * there is a need to know the previous PSU configuration
+ * so that the appropriate gpio line is interrupted for
+ * the correct PSU. In order to keep this variable up to
+ * date it is global so that it can be set at init and
+ * each time the interrupt fires.
+ */
+u8 psu_presence;
+
+struct gxp_gpio_drvdata {
+ struct gpio_chip chip;
+ struct regmap *base;
+ struct regmap *interrupt;
+ int irq;
+};
+
+static const struct regmap_config gxp_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = 0x80,
+ .name = "gxp-gpio-pl",
+};
+
+static const struct regmap_config gxp_int_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = 0x7f,
+ .name = "gxp-gpio-pl-int",
+};
+
+static struct regmap *gxp_gpio_init_regmap(struct platform_device *pdev,
+ char *reg_name, bool is_interrupt)
+{
+ void __iomem *base;
+
+ base = devm_platform_ioremap_resource_byname(pdev, reg_name);
+ if (IS_ERR(base))
+ return ERR_CAST(base);
+
+ if (is_interrupt)
+ return devm_regmap_init_mmio(&pdev->dev, base, &gxp_int_regmap_config);
+ else
+ return devm_regmap_init_mmio(&pdev->dev, base, &gxp_regmap_config);
+}
+
+#ifdef CONFIG_DEBUG_FS
+
+#include <linux/debugfs.h>
+
+static int gxp_gpio_serverid_show(struct seq_file *s, void *unused)
+{
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(s->private);
+ unsigned int server_id_l;
+ unsigned int server_id_h;
+
+ regmap_read(drvdata->base, PLREG_SERVER_ID, &server_id_l);
+ regmap_read(drvdata->base, PLREG_SERVER_ID + 0x01, &server_id_h);
+
+ seq_printf(s, "%02x %02x", server_id_h, server_id_l);
+
+ return 0;
+}
+
+static void gxp_gpio_debuginit(struct platform_device *pdev)
+{
+ debugfs_create_devm_seqfile(&pdev->dev, "gxp_gpio_serverid", NULL,
+ gxp_gpio_serverid_show);
+}
+
+#else
+
+static inline void gxp_gpio_debuginit(struct platform_device *pdev)
+{
+}
+
+#endif
+
+static int gxp_gpio_pl_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+ unsigned int val;
+ unsigned int reg_offset;
+ u8 reg_mask;
+ bool is_active_low = false;
+
+ switch (offset) {
+ case IOP_LED1 ... IOP_LED8:
+ reg_offset = PLREG_IOP_LED;
+ reg_mask = BIT(offset);
+ break;
+ case FAN1_INST ...FAN8_INST:
+ regmap_read(drvdata->base, PLREG_FAN_INST, &val);
+ reg_mask = BIT(offset - FAN1_INST);
+ break;
+ case FAN1_FAIL ... FAN8_FAIL:
+ regmap_read(drvdata->base, PLREG_FAN_FAIL, &val);
+ reg_mask = BIT(offset - FAN1_FAIL);
+ break;
+ case PWR_BTN_INT ... SLP_INT:
+ /* Note this is active low */
+ reg_offset = PLREG_INT_GRP5_STATE;
+ reg_mask = BIT(offset - PWR_BTN_INT);
+ is_active_low = true;
+ break;
+ case PSU1_INST ... PSU8_INST:
+ reg_offset = PLREG_PSU_INST;
+ reg_mask = BIT(offset - PSU1_INST);
+ break;
+ case PSU1_AC ... PSU8_AC:
+ reg_offset = PLREG_PSU_AC;
+ reg_mask = BIT(offset - PSU1_AC);
+ break;
+ case PSU1_DC ... PSU8_DC:
+ reg_offset = PLREG_PSU_DC;
+ reg_mask = BIT(offset - PSU1_DC);
+ break;
+ case LED_IDENTIFY:
+ reg_offset = PLREG_IDENT_LED;
+ reg_mask = BIT(1);
+ break;
+ case LED_HEALTH_RED:
+ reg_offset = PLREG_HEALTH_LED;
+ reg_mask = GENMASK(5, 4); /* Bit 5 set, bit 4 clear */
+ break;
+ case LED_HEALTH_AMBER:
+ reg_offset = PLREG_HEALTH_LED;
+ reg_mask = GENMASK(5, 4); /* Bit 5, bit 4 set */
+ break;
+ case SIDEBAND_SEL_L:
+ reg_offset = PLREG_SIDEBAND;
+ reg_mask = BIT(0);
+ break;
+ case SIDEBAND_SEL_H:
+ reg_offset = PLREG_SIDEBAND;
+ reg_mask = BIT(1);
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ regmap_read(drvdata->base, reg_offset, &val);
+
+ /* Special case: Check two bits for Health LED */
+ if (offset == LED_HEALTH_RED)
+ /* Bit 5 set, bit 4 not set */
+ return ((val & reg_mask) == BIT(5) ? 1 : 0);
+ else if (offset == LED_HEALTH_AMBER)
+ /* Bit 5 and 4 set */
+ return ((val & reg_mask) == reg_mask ? 1 : 0);
+
+ val = val & reg_mask;
+
+ if (is_active_low)
+ return (val ? 0 : 1);
+ else
+ return (val ? 1 : 0);
+}
+
+static void gxp_gpio_pl_set(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+
+ switch (offset) {
+ case IOP_LED1 ... IOP_LED8:
+ regmap_update_bits(drvdata->base,
+ PLREG_IOP_LED,
+ BIT(offset),
+ value == 0 ? 0 : BIT(offset));
+ break;
+ case LED_IDENTIFY:
+ regmap_update_bits(drvdata->base,
+ PLREG_IDENT_LED,
+ GENMASK(7, 6),
+ value == 0 ? BIT(7) : GENMASK(7, 6));
+ break;
+ case LED_HEALTH_RED:
+ regmap_update_bits(drvdata->base,
+ PLREG_HEALTH_LED,
+ GENMASK(7, 6),
+ value == 0 ? 0 : BIT(7));
+ break;
+ case LED_HEALTH_AMBER:
+ regmap_update_bits(drvdata->base,
+ PLREG_HEALTH_LED,
+ GENMASK(7, 6),
+ value == 0 ? 0 : BIT(6));
+ break;
+ case SIDEBAND_SEL_L ... SIDEBAND_SEL_H:
+ regmap_update_bits(drvdata->base,
+ PLREG_SIDEBAND,
+ BIT(offset - SIDEBAND_SEL_L),
+ value == 0 ? 0 : BIT(offset - SIDEBAND_SEL_L));
+ break;
+ default:
+ break;
+ }
+}
+
+static int gxp_gpio_pl_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+ switch (offset) {
+ case IOP_LED1 ... IOP_LED8:
+ case LED_IDENTIFY ... LED_HEALTH_AMBER:
+ case ACM_FORCE_OFF:
+ case ACM_REQ_N:
+ case SIDEBAND_SEL_L ... SIDEBAND_SEL_H:
+ return GXP_GPIO_DIR_OUT;
+ default:
+ return -ENOTSUPP;
+ }
+}
+
+static int gxp_gpio_pl_direction_input(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ switch (offset) {
+ case FAN1_INST ... FAN8_FAIL:
+ return GXP_GPIO_DIR_OUT;
+ case PWR_BTN_INT ... SLP_INT:
+ return GXP_GPIO_DIR_OUT;
+ default:
+ return -ENOTSUPP;
+ }
+}
+
+static int gxp_gpio_pl_direction_output(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ switch (offset) {
+ case IOP_LED1 ... IOP_LED8:
+ case LED_IDENTIFY ... LED_HEALTH_AMBER:
+ case ACM_FORCE_OFF:
+ case ACM_REQ_N:
+ case SIDEBAND_SEL_L ... SIDEBAND_SEL_H:
+ gxp_gpio_pl_set(chip, offset, value);
+ return GXP_GPIO_DIR_OUT;
+ default:
+ return -ENOTSUPP;
+ }
+}
+
+static void gxp_gpio_pl_irq_ack(struct irq_data *d)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+ unsigned int val;
+
+ /* Read latched interrupt for group 5 */
+ regmap_read(drvdata->interrupt, PLREG_INT_GRP5_FLAG, &val);
+ /* Clear latched interrupt */
+ regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP5_FLAG,
+ 0xFF, 0xFF);
+
+ /* Read latched interrupt for group 6 */
+ regmap_read(drvdata->interrupt, PLREG_INT_GRP6_FLAG, &val);
+ /* Clear latched interrupt */
+ regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP6_FLAG,
+ 0xFF, 0xFF);
+}
+
+static void gxp_gpio_pl_irq_set_mask(struct irq_data *d, bool set)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+
+ regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP5_BASE,
+ BIT(0) | BIT(2), set ? 0 : BIT(0) | BIT(2));
+
+ regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP6_BASE,
+ BIT(2), set ? 0 : BIT(2));
+}
+
+static void gxp_gpio_pl_irq_mask(struct irq_data *d)
+{
+ gxp_gpio_pl_irq_set_mask(d, false);
+}
+
+static void gxp_gpio_pl_irq_unmask(struct irq_data *d)
+{
+ gxp_gpio_pl_irq_set_mask(d, true);
+}
+
+static int gxp_gpio_irq_init_hw(struct gpio_chip *chip)
+{
+ struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
+
+ regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP5_BASE,
+ BIT(0) | BIT(2), 0);
+
+ regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP6_BASE,
+ BIT(2), 0);
+
+ return 0;
+}
+
+static int gxp_gpio_pl_set_type(struct irq_data *d, unsigned int type)
+{
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ irq_set_handler_locked(d, handle_level_irq);
+ else
+ irq_set_handler_locked(d, handle_edge_irq);
+
+ return 0;
+}
+
+static irqreturn_t gxp_gpio_pl_irq_handle(int irq, void *_drvdata)
+{
+ struct gxp_gpio_drvdata *drvdata = _drvdata;
+ unsigned int val, i;
+ unsigned long temp;
+
+ /* Check group 5 interrupts */
+
+ regmap_read(drvdata->base, PLREG_INT_GRP5_FLAG, &val);
+
+ temp = (unsigned long)val;
+ for_each_set_bit(i, &temp, 3) {
+ generic_handle_domain_irq(drvdata->chip.irq.domain,
+ i + PWR_BTN_INT);
+ }
+
+ /* Clear latched interrupt */
+ regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP5_FLAG,
+ GENMASK(7, 0), GENMASK(7, 0));
+ regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP5_BASE,
+ BIT(0) | BIT(2), 0);
+
+ /* Check group 6 interrupts */
+
+ regmap_read(drvdata->base, PLREG_INT_GRP6_FLAG, &val);
+
+ if (val & BIT(2)) {
+ u8 old_psu = psu_presence;
+
+ regmap_read(drvdata->base, PLREG_PSU_INST, &val);
+ psu_presence = val;
+
+ if (old_psu != psu_presence) {
+ /* Identify all bits which differs */
+ unsigned long current_val = psu_presence;
+ unsigned long old_val = old_psu;
+ unsigned long changed_bits;
+
+ bitmap_xor(&changed_bits, &current_val, &old_val, 8);
+
+ for_each_set_bit(i, &changed_bits, 8) {
+ /* PSU state has changed */
+ generic_handle_domain_irq(drvdata->chip.irq.domain,
+ i + PSU1_INST);
+ }
+ }
+ }
+
+ /* Clear latched interrupt */
+ regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP6_FLAG,
+ GENMASK(7, 0), GENMASK(7, 0));
+ regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP6_BASE,
+ BIT(2), 0);
+
+ return IRQ_HANDLED;
+}
+
+static const struct gpio_chip template_chip = {
+ .label = "gxp_gpio_plreg",
+ .owner = THIS_MODULE,
+ .get = gxp_gpio_pl_get,
+ .set = gxp_gpio_pl_set,
+ .get_direction = gxp_gpio_pl_get_direction,
+ .direction_input = gxp_gpio_pl_direction_input,
+ .direction_output = gxp_gpio_pl_direction_output,
+ .base = -1,
+};
+
+static const struct irq_chip gxp_plreg_irqchip = {
+ .name = "gxp_plreg",
+ .irq_ack = gxp_gpio_pl_irq_ack,
+ .irq_mask = gxp_gpio_pl_irq_mask,
+ .irq_unmask = gxp_gpio_pl_irq_unmask,
+ .irq_set_type = gxp_gpio_pl_set_type,
+ .flags = IRQCHIP_IMMUTABLE,
+};
+
+static const struct of_device_id gxp_gpio_of_match[] = {
+ { .compatible = "hpe,gxp-gpio-pl" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, gxp_gpio_of_match);
+
+static int gxp_gpio_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct gxp_gpio_drvdata *drvdata;
+ struct gpio_irq_chip *girq;
+ unsigned int val;
+
+ /* Initialize global vars */
+ psu_presence = 0;
+
+ drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, drvdata);
+
+ drvdata->base = gxp_gpio_init_regmap(pdev, "base", false);
+ if (IS_ERR(drvdata->base))
+ return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->base),
+ "failed to map base\n");
+
+ drvdata->interrupt = gxp_gpio_init_regmap(pdev, "interrupt", true);
+ if (IS_ERR(drvdata->interrupt))
+ return dev_err_probe(&pdev->dev, PTR_ERR(drvdata->interrupt),
+ "failed to map interrupt base\n");
+
+ /* Necessary to read the server id */
+ gxp_gpio_debuginit(pdev);
+
+ /* Initialize psu_presence variable */
+ regmap_read(drvdata->base, PLREG_PSU_INST, &val);
+ psu_presence = val;
+
+ drvdata->chip = template_chip;
+ drvdata->chip.ngpio = 80;
+ drvdata->chip.parent = &pdev->dev;
+
+ girq = &drvdata->chip.irq;
+ gpio_irq_chip_set_chip(girq, &gxp_plreg_irqchip);
+ girq->parent_handler = NULL;
+ girq->num_parents = 0;
+ girq->parents = NULL;
+ girq->default_type = IRQ_TYPE_NONE;
+ girq->handler = handle_bad_irq;
+
+ girq->init_hw = gxp_gpio_irq_init_hw;
+
+ ret = devm_gpiochip_add_data(&pdev->dev, &drvdata->chip, drvdata);
+ if (ret < 0)
+ return dev_err_probe(&pdev->dev, ret, "Could not register gpiochip for plreg\n");
+
+ regmap_update_bits(drvdata->interrupt,
+ PLREG_INT_HI_PRI_EN,
+ PLREG_GRP4_GRP5_MASK,
+ PLREG_GRP4_GRP5_MASK);
+ regmap_update_bits(drvdata->interrupt,
+ PLREG_INT_GRP_STAT_MASK,
+ PLREG_GRP4_GRP5_MASK,
+ 0x00);
+
+ regmap_read(drvdata->interrupt, PLREG_INT_HI_PRI_EN, &val);
+ regmap_read(drvdata->interrupt, PLREG_INT_GRP_STAT_MASK, &val);
+
+ ret = platform_get_irq(pdev, 0);
+ if (ret < 0)
+ return ret;
+
+ drvdata->irq = ret;
+
+ return devm_request_irq(&pdev->dev, drvdata->irq, gxp_gpio_pl_irq_handle,
+ IRQF_SHARED, "gxp-pl", drvdata);
+}
+
+static struct platform_driver gxp_gpio_driver = {
+ .driver = {
+ .name = "gxp-gpio-pl",
+ .of_match_table = gxp_gpio_of_match,
+ },
+ .probe = gxp_gpio_probe,
+};
+module_platform_driver(gxp_gpio_driver);
+
+MODULE_AUTHOR("Nick Hawkins <[email protected]>");
+MODULE_DESCRIPTION("GPIO PL interface for GXP");
+MODULE_LICENSE("GPL");
--
2.17.1

2023-06-22 07:32:17

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH v4 0/5] ARM: Add GPIO support

On Wed, Jun 21, 2023 at 11:35 PM <[email protected]> wrote:

> The gxp-fan-ctrl driver in HWMON no longer will report fan presence
> or fan failure states as these GPIOs providing this information will be
> consumed by the host. It will be the hosts function to keep track of
> fan presence and status.

I understand the approach such that you have also constructed a
userspace cooling daemon that will consume the fan and GPIO
information to drive the hardware monitoring and that is what you
mean when you say "the host" will do it.

This is a *bad idea*.

While I can't stop you since these are indeed userspace interfaces we
provide, I urge you to look into my earlier proposal to use a thermal
zone to manage the cooling inside the kernel and get rid of all that
custom userspace.

The kernel has all that is needed to regulate the thermal zone with
PID and on/off regulation. It will work even if the userspace crashes
completely, which is what you want. The code is reviewed by a large
community and very well tested.

I think I showed this example before from
arch/arm/boot/dts/gemini-dlink-dns-313.dts:

thermal-zones {
chassis-thermal {
/* Poll every 20 seconds */
polling-delay = <20000>;
/* Poll every 2nd second when cooling */
polling-delay-passive = <2000>;

thermal-sensors = <&g751>;

/* Tripping points from the fan.script in the rootfs */
trips {
chassis_alert0: chassis-alert0 {
/* At 43 degrees turn on low speed */
temperature = <43000>;
hysteresis = <3000>;
type = "active";
};
chassis_alert1: chassis-alert1 {
/* At 47 degrees turn on high speed */
temperature = <47000>;
hysteresis = <3000>;
type = "active";
};
chassis_crit: chassis-crit {
/* Just shut down at 60 degrees */
temperature = <60000>;
hysteresis = <2000>;
type = "critical";
};
};

cooling-maps {
map0 {
trip = <&chassis_alert0>;
cooling-device = <&fan0 1 1>;
};
map1 {
trip = <&chassis_alert1>;
cooling-device = <&fan0 2 2>;
};
};
};
};

This uses a thermal sensor and a fan with two speeds.

Adding a "presence" GPIO to the thermal zone core to enable and
disable it which is what your use case needs should be pretty trivial.

Yours,
Linus Walleij

2023-06-22 07:35:19

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH v4 2/5] gpio: gxp: Add HPE GXP GPIO PL

Hi Nick,

thanks for your patch!

This is looking pretty good, I have some minor questions.

On Wed, Jun 21, 2023 at 11:35 PM <[email protected]> wrote:

> From: Nick Hawkins <[email protected]>
>
> The GXP SoC supports GPIO on multiple interfaces. The interfaces are
> CPLD and Host. The gpio-gxp-pl driver covers the CPLD which takes
> physical I/O from the board and shares it with GXP via a proprietary
> interface that maps the I/O onto a specific register area of the GXP.
> This driver supports interrupts from the CPLD.
>
> Signed-off-by: Nick Hawkins <[email protected]>

(...)
> +enum pl_gpio_pn {
> + IOP_LED1 = 0,
> + IOP_LED2 = 1,
> + IOP_LED3 = 2,
> + IOP_LED4 = 3,
(...)

The confusing bit here is that GPIO means
*generic purpose input/output*
and these use cases are hardcoded into the driver and
do not look very generic purpose at all.

But I understand that it is convenient. I would add some
comment saying that if there is a new version with a
different layout of the pins, we need to make this kind
of stuff go away and just use the numbers.

> +static const struct gpio_chip template_chip = {
> + .label = "gxp_gpio_plreg",
> + .owner = THIS_MODULE,
> + .get = gxp_gpio_pl_get,
> + .set = gxp_gpio_pl_set,
> + .get_direction = gxp_gpio_pl_get_direction,
> + .direction_input = gxp_gpio_pl_direction_input,
> + .direction_output = gxp_gpio_pl_direction_output,
> + .base = -1,
> +};

Neat! Since you so explicitly have assigned a meaning to each
GPIO line, you can go ahead and assign the .names property as
well. Check in the kernel tree for other drivers doing this.

> + drvdata->chip = template_chip;
> + drvdata->chip.ngpio = 80;

If you're always assigning 80 to this you can just put that in the
template as well.

Other than that I think it looks good!

Yours,
Linus Walleij

2023-06-22 14:24:04

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] gpio: gxp: Add HPE GXP GPIO

2023-06-27 13:52:03

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] gpio: gxp: Add HPE GXP GPIO

On Thu, Jun 22, 2023 at 12:35 AM <[email protected]> wrote:
>
> From: Nick Hawkins <[email protected]>
>
> The GXP SoC supports GPIO on multiple interfaces. The interfaces are
> CPLD and Host. The GPIOs is a combination of both physical and virtual

are a

> I/O across the interfaces. The gpio-gxp driver specifically covers the
> CSM(physical), FN2(virtual), and VUHC(virtual) which are the host. The

A bit of elaboration what the Host interface means and what is the
difference to the CPLD. Perhaps spell it here as "Host interface", so
it will be clear that above you mentioned it already.

> driver supports interrupts from the host.

...

> +#define GPIDAT 0x040
> +#define GPODAT 0x0b0
> +#define GPODAT2 0x0f8
> +#define GPOOWN 0x110
> +#define GPOOWN2 0x118
> +#define ASR_OFS 0x05c
> +#define VUHC_OFS 0x064

Hmm... No GPIDAT2? I'm wondering if you can drop all these *2
definitions. Let see below...

...

> +struct gxp_gpio_drvdata {
> + struct gpio_chip chip;
> + struct regmap *csm_map;

> + void __iomem *fn2_vbtn;

Looking into the code I have no clue why this is in this driver. You
have regmaps and a separate resource for this. Why?! Is it in the
window of GPIO MMIO?

> + struct regmap *fn2_stat;
> + struct regmap *vuhc0_map;
> + int irq;
> +};

...

> +/*
> + * Note: Instead of definining all PINs here are the select few that

defining (I have a déjà vu of already showing you typos in the commit
message and comments and it looks like you ignored all of that. If so,
then why?)

> + * are specifically defined in DTS and offsets are used here.
> + */
> +enum gxp_gpio_pn {
> + RESET = 192,
> + VPBTN = 210, /* aka POWER_OK */
> + PGOOD = 211, /* aka PS_PWROK */
> + PERST = 212, /* aka PCIERST */
> + POST_COMPLETE = 213,

So, vbtn is a GPIO? Why does it need a special treatment?

> +};

...

> +static int gxp_gpio_csm_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);

> + int ret = 0;

Seems like a weird assignment.

> + unsigned int reg_offset;
> + unsigned int reg_mask;
> +
> + switch (offset) {
> + case 0 ... 31:
> + reg_offset = GPIDAT;
> + reg_mask = BIT(offset);
> + break;
> + case 32 ... 63:
> + reg_offset = GPIDAT + 0x20;
> + reg_mask = BIT(offset - 32);
> + break;

So, here is the 0x20 offset shift...

> + case 64 ... 95:
> + reg_offset = GPODAT;
> + reg_mask = BIT(offset - 64);
> + break;
> + case 96 ... 127:
> + reg_offset = GPODAT + 0x04;
> + reg_mask = BIT(offset - 96);
> + break;

...and here (between two groups of GPO) is 0x48. Looks a bit weird.
Does this GPIO have more functions than simply being a GPIO? To me
looks like a PMIC-ish one. Is there any datasheet available?

> + case 128 ... 159:
> + reg_offset = GPODAT2;
> + reg_mask = BIT(offset - 128);
> + break;
> + case 160 ... 191:
> + reg_offset = GPODAT2 + 0x04;
> + reg_mask = BIT(offset - 160);
> + break;

These (64-192) are two groups of the sequential bits in the address
space. Why do you do this instead of the simplest calculus with bit
and offset?

> + case RESET:
> + /* SW_RESET */
> + reg_offset = ASR_OFS;
> + reg_mask = BIT(15);
> + break;

Does it really belong to this driver? Maybe it should be an MFD with
GPIO and special functions with valid_mask properly assigned?

> + default:
> + break;
> + }

> + regmap_read(drvdata->csm_map, reg_offset, &ret);
> + ret = (ret & reg_mask) ? 1 : 0;
> +
> + return ret;

ret = regmap_read(, &value);
if (ret)
return ret;

return !!(value & mask);

> +}
> +
> +static void gxp_gpio_csm_set(struct gpio_chip *chip, unsigned int offset,
> + int value)
> +{
> + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
> + u32 tmp;

> + switch (offset) {

You definitely don't need this. All bits are in the sequential addresses.

> + case 64 ... 95:
> + /* Keep ownership setting */
> + regmap_read(drvdata->csm_map, GPOOWN, &tmp);
> + tmp = (tmp & BIT(offset - 64)) ? 1 : 0;
> +
> + regmap_update_bits(drvdata->csm_map, GPOOWN,
> + BIT(offset - 64), BIT(offset - 64));
> + regmap_update_bits(drvdata->csm_map, GPODAT,
> + BIT(offset - 64), value ? BIT(offset - 64) : 0);
> +
> + /* Restore ownership setting */
> + regmap_update_bits(drvdata->csm_map, GPOOWN,
> + BIT(offset - 64), tmp ? BIT(offset - 64) : 0);
> + break;
> + case 96 ... 127:
> + /* Keep ownership setting */
> + regmap_read(drvdata->csm_map, GPOOWN + 0x04, &tmp);
> + tmp = (tmp & BIT(offset - 96)) ? 1 : 0;
> +
> + regmap_update_bits(drvdata->csm_map, GPOOWN + 0x04,
> + BIT(offset - 96), BIT(offset - 96));
> + regmap_update_bits(drvdata->csm_map, GPODAT + 0x04,
> + BIT(offset - 96), value ? BIT(offset - 96) : 0);
> +
> + /* Restore ownership setting */
> + regmap_update_bits(drvdata->csm_map, GPOOWN + 0x04,
> + BIT(offset - 96), tmp ? BIT(offset - 96) : 0);
> + break;
> + case 128 ... 159:
> + /* Keep ownership setting */
> + regmap_read(drvdata->csm_map, GPOOWN2, &tmp);
> + tmp = (tmp & BIT(offset - 128)) ? 1 : 0;
> +
> + regmap_update_bits(drvdata->csm_map, GPOOWN2,
> + BIT(offset - 128), BIT(offset - 128));
> + regmap_update_bits(drvdata->csm_map, GPODAT2,
> + BIT(offset - 128), value ? BIT(offset - 128) : 0);
> +
> + /* Restore ownership setting */
> + regmap_update_bits(drvdata->csm_map, GPOOWN2,
> + BIT(offset - 128), tmp ? BIT(offset - 128) : 0);
> + break;
> + case 160 ... 191:
> + /* Keep ownership setting */
> + regmap_read(drvdata->csm_map, GPOOWN2 + 0x04, &tmp);
> + tmp = (tmp & BIT(offset - 160)) ? 1 : 0;
> +
> + regmap_update_bits(drvdata->csm_map, GPOOWN2 + 0x04,
> + BIT(offset - 160), BIT(offset - 160));
> + regmap_update_bits(drvdata->csm_map, GPODAT2 + 0x04,
> + BIT(offset - 160), value ? BIT(offset - 160) : 0);
> +
> + /* Restore ownership setting */
> + regmap_update_bits(drvdata->csm_map, GPOOWN2 + 0x04,
> + BIT(offset - 160), tmp ? BIT(offset - 160) : 0);
> + break;

> + case 192:
> + if (value) {
> + regmap_update_bits(drvdata->csm_map, ASR_OFS,
> + BIT(0), BIT(0));
> + regmap_update_bits(drvdata->csm_map, ASR_OFS,
> + BIT(15), BIT(15));
> + } else {
> + regmap_update_bits(drvdata->csm_map, ASR_OFS,
> + BIT(15), 0);
> + }
> + break;

Again, seems like a special function of GPIO that should probably have
another driver that shares regmap with GPIO and GPIO marks this one is
not valid for the GPIO operations.

> + default:
> + break;
> + }
> +}
> +
> +static int gxp_gpio_csm_get_direction(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + switch (offset) {

Why do you use your custom definitions for the direction? We already
have the generic ones for this. Please use them.

> + case 0 ... 63:
> + return GXP_GPIO_DIR_IN;
> + case 64 ... 191:
> + return GXP_GPIO_DIR_OUT;

> + case 192 ... 193:
> + return GXP_GPIO_DIR_OUT;
> + case 194:
> + return GXP_GPIO_DIR_IN;

These are special cases. Not sure if it's for the GPIO, but basically
you can check them separately and reduce switch-case to simple

type = offset / 64;
if (type)
return OUT;
return IN;

Something similar to the rest of the functions.

Note, that range operator in switch-case is non-standard.

> + default:
> + return -ENOTSUPP;
> + }
> +}

...

> +static int gxp_gpio_vuhc_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
> + unsigned int val;
> + int ret = 0;
> +
> + if (offset < 8) {
> + regmap_read(drvdata->vuhc0_map, VUHC_OFS + 4 * offset, &val);
> + ret = (val & BIT(13)) ? 1 : 0;
> + }
> +
> + return ret;
> +}
> +
> +static void gxp_gpio_vuhc_set(struct gpio_chip *chip, unsigned int offset,
> + int value)
> +{
> + /* Currently we are not supporting setting of these values yet */
> + switch (offset) {
> + default:
> + break;
> + }
> +}
> +
> +static int gxp_gpio_vuhc_get_direction(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + switch (offset) {
> + case 0:
> + case 1:
> + case 2:
> + return GXP_GPIO_DIR_IN;
> + default:
> + return -ENOTSUPP;
> + }
> +}
> +
> +static int gxp_gpio_vuhc_direction_input(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + switch (offset) {
> + case 0:
> + case 1:
> + case 2:
> + return 0;
> + default:
> + return -ENOTSUPP;
> + }
> +}
> +
> +static int gxp_gpio_vuhc_direction_output(struct gpio_chip *chip,
> + unsigned int offset, int value)
> +{
> + switch (offset) {
> + default:
> + return -ENOTSUPP;
> + }
> +}

I'm not sure this belongs to the GPIO driver.

> +static int gxp_gpio_fn2_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
> + unsigned int val;
> + int ret = 0;
> + unsigned int reg_mask;
> +
> + switch (offset) {
> + case PGOOD:
> + regmap_read(drvdata->fn2_stat, 0, &val);
> + reg_mask = BIT(24);
> +
> + break;
> + case PERST:
> + regmap_read(drvdata->fn2_stat, 0, &val);
> + reg_mask = BIT(25);
> +
> + break;
> + default:
> + return -ENOTSUPP;
> + }
> +
> + regmap_read(drvdata->fn2_stat, 0, &val);
> + ret = (val & reg_mask);
> + /* Return either 1 or 0 */
> + return ret ? 1 : 0;
> +}
> +
> +static void gxp_gpio_fn2_set(struct gpio_chip *chip, unsigned int offset,
> + int value)
> +{
> + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
> +
> + switch (offset) {
> + case VPBTN:
> + writeb(1, drvdata->fn2_vbtn);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +static int gxp_gpio_fn2_get_direction(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + switch (offset) {
> + case VPBTN:
> + return GXP_GPIO_DIR_OUT;
> + default:
> + return GXP_GPIO_DIR_IN;
> + }
> +}
> +
> +static int gxp_gpio_fn2_direction_input(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + switch (offset) {
> + case PGOOD:
> + case PERST:
> + case POST_COMPLETE:
> + return 0;
> + default:
> + return -ENOTSUPP;
> + }
> +}
> +
> +static int gxp_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + if (offset < 200)
> + return gxp_gpio_csm_get(chip, offset);
> + else if (offset >= 200 && offset < 210)
> + return gxp_gpio_vuhc_get(chip, offset - 200);
> + else if (offset >= 210)
> + return gxp_gpio_fn2_get(chip, offset);
> +
> + return 0;
> +}
> +
> +static void gxp_gpio_set(struct gpio_chip *chip,
> + unsigned int offset, int value)
> +{
> + if (offset < 200)
> + gxp_gpio_csm_set(chip, offset, value);
> + else if (offset >= 200 && offset < 210)
> + gxp_gpio_vuhc_set(chip, offset - 200, value);
> + else if (offset >= 210)
> + gxp_gpio_fn2_set(chip, offset, value);
> +}
> +
> +static int gxp_gpio_get_direction(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + if (offset < 200)
> + return gxp_gpio_csm_get_direction(chip, offset);
> + else if (offset >= 200 && offset < 210)
> + return gxp_gpio_vuhc_get_direction(chip, offset - 200);
> + else if (offset >= 210)
> + return gxp_gpio_fn2_get_direction(chip, offset);
> +
> + return 0;
> +}
> +
> +static int gxp_gpio_direction_input(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + if (offset < 200)
> + return gxp_gpio_csm_direction_input(chip, offset);
> + else if (offset >= 200 && offset < 210)
> + return gxp_gpio_vuhc_direction_input(chip, offset - 200);
> + else if (offset >= 210)
> + return gxp_gpio_fn2_direction_input(chip, offset);
> +
> + return 0;
> +}
> +
> +static int gxp_gpio_direction_output(struct gpio_chip *chip,
> + unsigned int offset, int value)
> +{
> + if (offset < 200)
> + return gxp_gpio_csm_direction_output(chip, offset, value);
> + else if (offset >= 200 && offset < 210)
> + return gxp_gpio_vuhc_direction_output(chip, offset - 200, value);
> +
> + return 0;
> +}

...

> + /* Clear latched interrupt */
> + regmap_update_bits(drvdata->fn2_stat, 0,
> + GENMASK(15, 0), GENMASK(15, 0));

unsigned int mask = GENMASK(...);
unsigned int value = mask;
regmap_update_bits(..., mask, value);

...

> + regmap_update_bits(drvdata->fn2_stat, FN2_SEVMASK,
> + BIT(0), set ? BIT(0) : 0);

Ditto.

unsigned int mask = BIT(0);
unsigned int value = set ? mask : 0;

...

So, overall it looks to me like an MFD device which should be split to
GPIO, GPIO with IRQ (fh2), special cases and designated
functionalities (somelike ~5 drivers all together). Without having a
datasheet it's hard to say.

--
With Best Regards,
Andy Shevchenko

2023-06-27 19:11:00

by Hawkins, Nick

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] gpio: gxp: Add HPE GXP GPIO

> defining (I have a déjà vu of already showing you typos in the commit
> message and comments and it looks like you ignored all of that. If so,
> then why?)

Apologies Andy,

I somehow completely missed / lost your mention of a typo in both the
commit and comments. I have no intention of ignoring it. I will correct
this. Thank you for the input you have provided. I have several
questions and comments below.

> > + * are specifically defined in DTS and offsets are used here.
> > + */
> > +enum gxp_gpio_pn {
> > + RESET = 192,
> > + VPBTN = 210, /* aka POWER_OK */
> > + PGOOD = 211, /* aka PS_PWROK */
> > + PERST = 212, /* aka PCIERST */
> > + POST_COMPLETE = 213,

> So, vbtn is a GPIO? Why does it need a special treatment?

I was specifically grabbing the areas of memory that I needed instead of
mapping the entire fn2 area of memory. I believe I can map the entire
area instead.

...

> > + case 64 ... 95:
> > + reg_offset = GPODAT;
> > + reg_mask = BIT(offset - 64);
> > + break;
> > + case 96 ... 127:
> > + reg_offset = GPODAT + 0x04;
> > + reg_mask = BIT(offset - 96);
> > + break;

> ...and here (between two groups of GPO) is 0x48. Looks a bit weird.
> Does this GPIO have more functions than simply being a GPIO? To me
> looks like a PMIC-ish one. Is there any datasheet available?

Unfortunately, there is no public datasheet available currently. There
are however some special functions others than being a simple GPIO.
There are ownership bits as the same area is accessible VIA PCI.

> > + case RESET:
> > + /* SW_RESET */
> > + reg_offset = ASR_OFS;
> > + reg_mask = BIT(15);
> > + break;

> Does it really belong to this driver? Maybe it should be an MFD with
> GPIO and special functions with valid_mask properly assigned?

Unlike your suggestion I quote directly below are you implying that
My accesses to the CSM area of memory can be its own separate
driver that is MFD and provides GPIO lines to read?

> ...

> > + case 192:
> > + if (value) {
> > + regmap_update_bits(drvdata->csm_map, ASR_OFS,
> > + BIT(0), BIT(0));
> > + regmap_update_bits(drvdata->csm_map, ASR_OFS,
> > + BIT(15), BIT(15));
> > + } else {
> > + regmap_update_bits(drvdata->csm_map, ASR_OFS,
> > + BIT(15), 0);
> > + }
> > + break;

> Again, seems like a special function of GPIO that should probably have
> another driver that shares regmap with GPIO and GPIO marks this one is
> not valid for the GPIO operations.

What do you mean by GPIO marking this one as not valid for GPIO
operations?

...

> > +static int gxp_gpio_vuhc_get(struct gpio_chip *chip, unsigned int offset)
> > +{
> > + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent);
> > + unsigned int val;
> > + int ret = 0;
> > +
> > + if (offset < 8) {
> > + regmap_read(drvdata->vuhc0_map, VUHC_OFS + 4 * offset, &val);
> > + ret = (val & BIT(13)) ? 1 : 0;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static void gxp_gpio_vuhc_set(struct gpio_chip *chip, unsigned int offset,
> > + int value)
> > +{
> > + /* Currently we are not supporting setting of these values yet */
> > + switch (offset) {
> > + default:
> > + break;
> > + }
> > +}
> > +
> > +static int gxp_gpio_vuhc_get_direction(struct gpio_chip *chip,
> > + unsigned int offset)
> > +{
> > + switch (offset) {
> > + case 0:
> > + case 1:
> > + case 2:
> > + return GXP_GPIO_DIR_IN;
> > + default:
> > + return -ENOTSUPP;
> > + }
> > +}
> > +
> > +static int gxp_gpio_vuhc_direction_input(struct gpio_chip *chip,
> > + unsigned int offset)
> > +{
> > + switch (offset) {
> > + case 0:
> > + case 1:
> > + case 2:
> > + return 0;
> > + default:
> > + return -ENOTSUPP;
> > + }
> > +}
> > +
> > +static int gxp_gpio_vuhc_direction_output(struct gpio_chip *chip,
> > + unsigned int offset, int value)
> > +{
> > + switch (offset) {
> > + default:
> > + return -ENOTSUPP;
> > + }
> > +}

> I'm not sure this belongs to the GPIO driver.

By this do you mean that it needs to be in a separate non GPIO driver
that shares a regmap as suggested above?

...

> So, overall it looks to me like an MFD device which should be split to
> GPIO, GPIO with IRQ (fh2), special cases and designated
> functionalities (somelike ~5 drivers all together). Without having a
> datasheet it's hard to say.

Yes that sounds like a good plan to me I will see what I can work up.
I might end up removing thils file entirely and just sticking with
gpio-gxp-pl.c As for the gpio-gxp-pl.c are you okay with it?

Thank you for the assistance and review,

-Nick Hawkins

2023-07-03 15:53:08

by Hawkins, Nick

[permalink] [raw]

Subject: Re: [PATCH v4 0/5] ARM: Add GPIO support

> I understand the approach such that you have also constructed a
> userspace cooling daemon that will consume the fan and GPIO
> information to drive the hardware monitoring and that is what you
> mean when you say "the host" will do it.

> This is a *bad idea*.

> While I can't stop you since these are indeed userspace interfaces we
> provide, I urge you to look into my earlier proposal to use a thermal
> zone to manage the cooling inside the kernel and get rid of all that
> custom userspace.

> The kernel has all that is needed to regulate the thermal zone with
> PID and on/off regulation. It will work even if the userspace crashes
> completely, which is what you want. The code is reviewed by a large
> community and very well tested.

> I think I showed this example before from
> arch/arm/boot/dts/gemini-dlink-dns-313.dts:

> thermal-zones {
> chassis-thermal {
> /* Poll every 20 seconds */
> polling-delay = <20000>;
> /* Poll every 2nd second when cooling */
> polling-delay-passive = <2000>;

> thermal-sensors = <&g751>;

> /* Tripping points from the fan.script in the rootfs */
> trips {
> chassis_alert0: chassis-alert0 {
> /* At 43 degrees turn on low speed */
> temperature = <43000>;
> hysteresis = <3000>;
> type = "active";
> };
> chassis_alert1: chassis-alert1 {
> /* At 47 degrees turn on high speed */
> temperature = <47000>;
> hysteresis = <3000>;
> type = "active";
> };
> chassis_crit: chassis-crit {
> /* Just shut down at 60 degrees */
> temperature = <60000>;
> hysteresis = <2000>;
> type = "critical";
> };
> };

> cooling-maps {
> map0 {
> trip = <&chassis_alert0>;
> cooling-device = <&fan0 1 1>;
> };
> map1 {
> trip = <&chassis_alert1>;
> cooling-device = <&fan0 2 2>;
> };
> };
> };
> };

> This uses a thermal sensor and a fan with two speeds.

> Adding a "presence" GPIO to the thermal zone core to enable and
> disable it which is what your use case needs should be pretty trivial.

Greetings Linus,

As always thank you for your feedback and suggestions. Sorry for the
delayed response. I will bring this concept to my team to discuss.
A possible issue with this could be how our cooling profile varies
based on options present such as extra DIMMS, CPU, storage,
network ... etc.

Thanks,

-Nick Hawkins

2023-07-03 22:49:18

by Linus Walleij

[permalink] [raw]

Subject: Re: [PATCH v4 0/5] ARM: Add GPIO support

On Mon, Jul 3, 2023 at 5:31 PM Hawkins, Nick <[email protected]> wrote:

> As always thank you for your feedback and suggestions. Sorry for the
> delayed response. I will bring this concept to my team to discuss.

Thanks!

> A possible issue with this could be how our cooling profile varies
> based on options present such as extra DIMMS, CPU, storage,
> network ... etc.

The thermal subsystem has plenty of tunables in sysfs, see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-thermal
I don't think it's a problem to add more, if there are technical
requirements for it.

Yours,
Linus Walleij