[PATCH 0/5] Qualcomm PMIC fixes

[PATCH 0/5] Qualcomm PMIC fixes

[Caleb Connolly]

This series addresses some long-standing issues with the SPMI arb
driver, the PMIC, and the PMIC GPIO. It fixes compatibility with
upstream Linux devicetrees, and simplifies pwrkey/resin support by
rewriting the pon driver to be a button driver rather than a GPIO
driver.

Existing users (the db410c and 820c) are adjusted to use the new button
driver in their board init code.

This series is based on the pinctrl [1] and clock [2] cleanup series.
There may be some DTS conflicts applying it standalone.

[1]: https://lore.kernel.org/u-boot/20231106-b4-qcom-pinctrl-v2-0-406e8d8689ca@linaro.org/
[2]: https://lore.kernel.org/u-boot/20231103-b4-qcom-clk-v3-0-8d2d460ece84@linaro.org/

---
Caleb Connolly (5):
      gpio: qcom_pmic: fix silent dev_read_addr downcast
      gpio: qcom_pmic: rework pwrkey driver into a button driver
      gpio: qcom_pmic: fix support for upstream DT
      spmi: msm: fix register range names
      pmic: qcom: dont use dev_read_addr to get USID

 arch/arm/dts/dragonboard410c-uboot.dtsi          |  11 +-
 arch/arm/dts/dragonboard820c-uboot.dtsi          |   9 +-
 arch/arm/dts/dragonboard820c.dts                 |   3 -
 arch/arm/dts/sdm845.dtsi                         |   2 +-
 board/qualcomm/dragonboard410c/dragonboard410c.c |  29 ++--
 board/qualcomm/dragonboard820c/dragonboard820c.c |  29 ++--
 drivers/gpio/Kconfig                             |   3 +-
 drivers/gpio/qcom_pmic_gpio.c                    | 193 ++++++++++++++++-------
 drivers/power/pmic/pmic_qcom.c                   |  13 +-
 drivers/spmi/spmi-msm.c                          |  46 +++---
 10 files changed, 200 insertions(+), 138 deletions(-)
---
base-commit: 13995976aba2b9a63917f3c43b3cd3eaeccc4606

// Caleb (they/them)

[PATCH 1/5] gpio: qcom_pmic: fix silent dev_read_addr downcast

[Caleb Connolly]

priv->pid is uint32_t, but dev_read_addr() returns a uint64_t on arm64,
with the upper bits being used for error codes. Do error checking before
downcasting to u32 to prevent errors being silently ignored.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/gpio/qcom_pmic_gpio.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c
index 65feb453ebc3..e5841f502953 100644
--- a/drivers/gpio/qcom_pmic_gpio.c
+++ b/drivers/gpio/qcom_pmic_gpio.c
@@ -221,11 +221,14 @@ static int qcom_gpio_probe(struct udevice *dev)
 {
 	struct qcom_gpio_bank *priv = dev_get_priv(dev);
 	int reg;
+	u64 pid;
 
-	priv->pid = dev_read_addr(dev);
-	if (priv->pid == FDT_ADDR_T_NONE)
+	pid = dev_read_addr(dev);
+	if (pid == FDT_ADDR_T_NONE)
 		return log_msg_ret("bad address", -EINVAL);
 
+	priv->pid = pid;
+
 	/* Do a sanity check */
 	reg = pmic_reg_read(dev->parent, priv->pid + REG_TYPE);
 	if (reg != REG_TYPE_VAL)
@@ -328,11 +331,14 @@ static int qcom_pwrkey_probe(struct udevice *dev)
 {
 	struct qcom_gpio_bank *priv = dev_get_priv(dev);
 	int reg;
+	u64 pid;
 
-	priv->pid = dev_read_addr(dev);
-	if (priv->pid == FDT_ADDR_T_NONE)
+	pid = dev_read_addr(dev);
+	if (pid == FDT_ADDR_T_NONE)
 		return log_msg_ret("bad address", -EINVAL);
 
+	priv->pid = pid;
+
 	/* Do a sanity check */
 	reg = pmic_reg_read(dev->parent, priv->pid + REG_TYPE);
 	if (reg != 0x1)

-- 
2.42.0

[PATCH 2/5] gpio: qcom_pmic: rework pwrkey driver into a button driver

[Caleb Connolly]

The power and resin keys were implemented as GPIOs here, but their only
use would be as buttons. Avoid the additional layer of introspection and
rework this driver into a button driver.

While we're here, replace the "qcom,pm8998-pwrkey" compatible with
"qcom,pm8941-pwrkey" to match upstream (Linux).

The dragonboard410c and 820c boards are adjusted to benefit from this
change too, simplify their custom board init code.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 arch/arm/dts/dragonboard410c-uboot.dtsi          |  11 +-
 arch/arm/dts/dragonboard820c-uboot.dtsi          |   9 +-
 arch/arm/dts/dragonboard820c.dts                 |   3 -
 board/qualcomm/dragonboard410c/dragonboard410c.c |  29 ++--
 board/qualcomm/dragonboard820c/dragonboard820c.c |  29 ++--
 drivers/gpio/Kconfig                             |   3 +-
 drivers/gpio/qcom_pmic_gpio.c                    | 161 +++++++++++++++--------
 7 files changed, 139 insertions(+), 106 deletions(-)

diff --git a/arch/arm/dts/dragonboard410c-uboot.dtsi b/arch/arm/dts/dragonboard410c-uboot.dtsi
index 3b0bd0ed0a1b..c96f1fcc8930 100644
--- a/arch/arm/dts/dragonboard410c-uboot.dtsi
+++ b/arch/arm/dts/dragonboard410c-uboot.dtsi
@@ -5,6 +5,9 @@
  * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
  */
 
+#include <dt-bindings/input/linux-event-codes.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+
 / {
 
 	smem {
@@ -46,10 +49,14 @@
 
 &pm8916_pon {
 	key_vol_down {
-		gpios = <&pm8916_pon 1 0>;
+		interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>;
+		linux,code = <KEY_DOWN>;
+		label = "key_vol_down";
 	};
 
 	key_power {
-		gpios = <&pm8916_pon 0 0>;
+		interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
+		linux,code = <KEY_ENTER>;
+		label = "key_power";
 	};
 };
diff --git a/arch/arm/dts/dragonboard820c-uboot.dtsi b/arch/arm/dts/dragonboard820c-uboot.dtsi
index 457728a43ecb..ed8ac0e5cf1a 100644
--- a/arch/arm/dts/dragonboard820c-uboot.dtsi
+++ b/arch/arm/dts/dragonboard820c-uboot.dtsi
@@ -5,6 +5,9 @@
  * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
  */
 
+#include <dt-bindings/input/linux-event-codes.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+
 / {
 	smem {
 		bootph-all;
@@ -33,12 +36,14 @@
 
 &pm8994_pon {
 	key_vol_down {
-		gpios = <&pm8994_pon 1 0>;
+		interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>;
+		linux,code = <KEY_DOWN>;
 		label = "key_vol_down";
 	};
 
 	key_power {
-		gpios = <&pm8994_pon 0 0>;
+		interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
+		linux,code = <KEY_ENTER>;
 		label = "key_power";
 	};
 };
diff --git a/arch/arm/dts/dragonboard820c.dts b/arch/arm/dts/dragonboard820c.dts
index ad201d48749c..7db0cc9d64cc 100644
--- a/arch/arm/dts/dragonboard820c.dts
+++ b/arch/arm/dts/dragonboard820c.dts
@@ -112,9 +112,6 @@
 				pm8994_pon: pm8994_pon@800 {
 					compatible = "qcom,pm8994-pwrkey";
 					reg = <0x800 0x96>;
-					#gpio-cells = <2>;
-					gpio-controller;
-					gpio-bank-name="pm8994_key.";
 				};
 
 				pm8994_gpios: pm8994_gpios@c000 {
diff --git a/board/qualcomm/dragonboard410c/dragonboard410c.c b/board/qualcomm/dragonboard410c/dragonboard410c.c
index 371b3262f8c5..1d6cabfb9c41 100644
--- a/board/qualcomm/dragonboard410c/dragonboard410c.c
+++ b/board/qualcomm/dragonboard410c/dragonboard410c.c
@@ -5,6 +5,7 @@
  * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
  */
 
+#include <button.h>
 #include <common.h>
 #include <cpu_func.h>
 #include <dm.h>
@@ -108,30 +109,18 @@ int board_usb_init(int index, enum usb_init_type init)
 /* Check for vol- button - if pressed - stop autoboot */
 int misc_init_r(void)
 {
-	struct udevice *pon;
-	struct gpio_desc resin;
-	int node, ret;
+	struct udevice *btn;
+	int ret;
+	enum button_state_t state;
 
-	ret = uclass_get_device_by_name(UCLASS_GPIO, "pm8916_pon@800", &pon);
+	ret = button_get_by_label("key_vol_down", &btn);
 	if (ret < 0) {
-		printf("Failed to find PMIC pon node. Check device tree\n");
-		return 0;
+		printf("Couldn't find power button!\n");
+		return ret;
 	}
 
-	node = fdt_subnode_offset(gd->fdt_blob, dev_of_offset(pon),
-				  "key_vol_down");
-	if (node < 0) {
-		printf("Failed to find key_vol_down node. Check device tree\n");
-		return 0;
-	}
-
-	if (gpio_request_by_name_nodev(offset_to_ofnode(node), "gpios", 0,
-				       &resin, 0)) {
-		printf("Failed to request key_vol_down button.\n");
-		return 0;
-	}
-
-	if (dm_gpio_get_value(&resin)) {
+	state = button_get_state(btn);
+	if (state == BUTTON_ON) {
 		env_set("preboot", "setenv preboot; fastboot 0");
 		printf("key_vol_down pressed - Starting fastboot.\n");
 	}
diff --git a/board/qualcomm/dragonboard820c/dragonboard820c.c b/board/qualcomm/dragonboard820c/dragonboard820c.c
index 6785bf58e949..789b17a48636 100644
--- a/board/qualcomm/dragonboard820c/dragonboard820c.c
+++ b/board/qualcomm/dragonboard820c/dragonboard820c.c
@@ -5,6 +5,7 @@
  * (C) Copyright 2017 Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
  */
 
+#include <button.h>
 #include <cpu_func.h>
 #include <init.h>
 #include <env.h>
@@ -139,30 +140,18 @@ void reset_cpu(void)
 /* Check for vol- button - if pressed - stop autoboot */
 int misc_init_r(void)
 {
-	struct udevice *pon;
-	struct gpio_desc resin;
-	int node, ret;
+	struct udevice *btn;
+	int ret;
+	enum button_state_t state;
 
-	ret = uclass_get_device_by_name(UCLASS_GPIO, "pm8994_pon@800", &pon);
+	ret = button_get_by_label("key_power", &btn);
 	if (ret < 0) {
-		printf("Failed to find PMIC pon node. Check device tree\n");
-		return 0;
+		printf("Couldn't find power button!\n");
+		return ret;
 	}
 
-	node = fdt_subnode_offset(gd->fdt_blob, dev_of_offset(pon),
-				  "key_vol_down");
-	if (node < 0) {
-		printf("Failed to find key_vol_down node. Check device tree\n");
-		return 0;
-	}
-
-	if (gpio_request_by_name_nodev(offset_to_ofnode(node), "gpios", 0,
-				       &resin, 0)) {
-		printf("Failed to request key_vol_down button.\n");
-		return 0;
-	}
-
-	if (dm_gpio_get_value(&resin)) {
+	state = button_get_state(btn);
+	if (state == BUTTON_ON) {
 		env_set("bootdelay", "-1");
 		printf("Power button pressed - dropping to console.\n");
 	}
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ba42b0768e12..fbf77673c5e0 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -309,12 +309,13 @@ config CMD_PCA953X
 config QCOM_PMIC_GPIO
 	bool "Qualcomm generic PMIC GPIO/keypad driver"
 	depends on DM_GPIO && PMIC_QCOM
+	select BUTTON
 	help
 	  Support for GPIO pins and power/reset buttons found on
 	  Qualcomm SoCs PMIC.
 	  Default name for GPIO bank is "pm8916".
 	  Power and reset buttons are placed in "pwkey_qcom" bank and
-          have gpio numbers 0 and 1 respectively.
+	  have gpio numbers 0 and 1 respectively.
 
 config PCF8575_GPIO
 	bool "PCF8575 I2C GPIO Expander driver"
diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c
index e5841f502953..3dbc02d83198 100644
--- a/drivers/gpio/qcom_pmic_gpio.c
+++ b/drivers/gpio/qcom_pmic_gpio.c
@@ -5,8 +5,10 @@
  * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
  */
 
+#include <button.h>
 #include <common.h>
 #include <dm.h>
+#include <dm/lists.h>
 #include <log.h>
 #include <power/pmic.h>
 #include <spmi/spmi.h>
@@ -275,107 +277,150 @@ U_BOOT_DRIVER(qcom_pmic_gpio) = {
 	.priv_auto	= sizeof(struct qcom_gpio_bank),
 };
 
+struct qcom_pmic_btn_priv {
+	u32 base;
+	u32 status_bit;
+	int code;
+	struct udevice *pmic;
+};
 
 /* Add pmic buttons as GPIO as well - there is no generic way for now */
 #define PON_INT_RT_STS                        0x10
 #define KPDPWR_ON_INT_BIT                     0
 #define RESIN_ON_INT_BIT                      1
 
-static int qcom_pwrkey_get_function(struct udevice *dev, unsigned offset)
+static enum button_state_t qcom_pwrkey_get_state(struct udevice *dev)
 {
-	return GPIOF_INPUT;
-}
+	struct qcom_pmic_btn_priv *priv = dev_get_priv(dev);
 
-static int qcom_pwrkey_get_value(struct udevice *dev, unsigned offset)
-{
-	struct qcom_gpio_bank *priv = dev_get_priv(dev);
-
-	int reg = pmic_reg_read(dev->parent, priv->pid + PON_INT_RT_STS);
+	int reg = pmic_reg_read(priv->pmic, priv->base + PON_INT_RT_STS);
 
 	if (reg < 0)
 		return 0;
 
-	switch (offset) {
-	case 0: /* Power button */
-		return (reg & BIT(KPDPWR_ON_INT_BIT)) != 0;
-		break;
-	case 1: /* Reset button */
-	default:
-		return (reg & BIT(RESIN_ON_INT_BIT)) != 0;
-		break;
-	}
+	return (reg & BIT(priv->status_bit)) != 0;
 }
 
-/*
- * Since pmic buttons modelled as GPIO, we need empty direction functions
- * to trick u-boot button driver
- */
-static int qcom_pwrkey_direction_input(struct udevice *dev, unsigned int offset)
+static int qcom_pwrkey_get_code(struct udevice *dev)
 {
-	return 0;
-}
+	struct qcom_pmic_btn_priv *priv = dev_get_priv(dev);
 
-static int qcom_pwrkey_direction_output(struct udevice *dev, unsigned int offset, int value)
-{
-	return -EOPNOTSUPP;
+	return priv->code;
 }
 
-static const struct dm_gpio_ops qcom_pwrkey_ops = {
-	.get_value		= qcom_pwrkey_get_value,
-	.get_function		= qcom_pwrkey_get_function,
-	.direction_input	= qcom_pwrkey_direction_input,
-	.direction_output	= qcom_pwrkey_direction_output,
-};
-
 static int qcom_pwrkey_probe(struct udevice *dev)
 {
-	struct qcom_gpio_bank *priv = dev_get_priv(dev);
-	int reg;
-	u64 pid;
+	struct button_uc_plat *uc_plat = dev_get_uclass_plat(dev);
+	struct qcom_pmic_btn_priv *priv = dev_get_priv(dev);
+	int ret;
+	u64 base;
 
-	pid = dev_read_addr(dev);
-	if (pid == FDT_ADDR_T_NONE)
-		return log_msg_ret("bad address", -EINVAL);
+	/* Ignore the top-level button node */
+	if (!uc_plat->label)
+		return 0;
 
-	priv->pid = pid;
+	/* the pwrkey and resin nodes are children of the "pon" node, get the
+	 * PMIC device to use in pmic_reg_* calls.
+	 */
+	priv->pmic = dev->parent->parent;
+
+	base = dev_read_addr(dev);
+	if (!base || base == FDT_ADDR_T_NONE) {
+		/* Linux devicetrees don't specify an address in the pwrkey node */
+		base = dev_read_addr(dev->parent);
+		if (base == FDT_ADDR_T_NONE) {
+			printf("%s: Can't find address\n", dev->name);
+			return -EINVAL;
+		}
+	}
+
+	priv->base = base;
 
 	/* Do a sanity check */
-	reg = pmic_reg_read(dev->parent, priv->pid + REG_TYPE);
-	if (reg != 0x1)
-		return log_msg_ret("bad type", -ENXIO);
+	ret = pmic_reg_read(priv->pmic, priv->base + REG_TYPE);
+	if (ret != 0x1 && ret != 0xb) {
+		printf("%s: unexpected PMIC function type %d\n", dev->name, ret);
+		return -ENXIO;
+	}
 
-	reg = pmic_reg_read(dev->parent, priv->pid + REG_SUBTYPE);
-	if ((reg & 0x5) == 0)
-		return log_msg_ret("bad subtype", -ENXIO);
+	ret = pmic_reg_read(priv->pmic, priv->base + REG_SUBTYPE);
+	if ((ret & 0x7) == 0) {
+		printf("%s: unexpected PMCI function subtype %d\n", dev->name, ret);
+		//return -ENXIO;
+	}
+
+	/* Bit of a hack, we use the interrupt number to derive if this is the pwrkey or resin
+	 * node, it just so happens to line up with the bit numbers in the interrupt status register
+	 */
+	ret = ofnode_read_u32_index(dev_ofnode(dev), "interrupts", 2, &priv->status_bit);
+	if (ret < 0) {
+		printf("%s: Couldn't read interrupts: %d\n", __func__, ret);
+		return ret;
+	}
+
+	ret = ofnode_read_u32(dev_ofnode(dev), "linux,code", &priv->code);
+	if (ret < 0) {
+		printf("%s: Couldn't read interrupts: %d\n", __func__, ret);
+		return ret;
+	}
 
 	return 0;
 }
 
-static int qcom_pwrkey_of_to_plat(struct udevice *dev)
+static int button_qcom_pmic_bind(struct udevice *parent)
 {
-	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+	struct udevice *dev;
+	ofnode node;
+	int ret;
 
-	uc_priv->gpio_count = 2;
-	uc_priv->bank_name = dev_read_string(dev, "gpio-bank-name");
-	if (uc_priv->bank_name == NULL)
-		uc_priv->bank_name = "pwkey_qcom";
+	dev_for_each_subnode(node, parent) {
+		struct button_uc_plat *uc_plat;
+		const char *label;
+
+		if (!ofnode_is_enabled(node))
+			continue;
+
+		label = ofnode_read_string(node, "label");
+		if (!label) {
+			printf("%s: node %s has no label\n", __func__,
+			       ofnode_get_name(node));
+			/* Don't break booting, just print a warning and continue */
+			continue;
+		}
+		/* We need the PMIC device to be the parent, so flatten it out here */
+		ret = device_bind_driver_to_node(parent, "pwrkey_qcom",
+						 ofnode_get_name(node),
+						 node, &dev);
+		if (ret) {
+			printf("Failed to bind %s! %d\n", label, ret);
+			return ret;
+		}
+		uc_plat = dev_get_uclass_plat(dev);
+		uc_plat->label = label;
+	}
 
 	return 0;
 }
 
+static const struct button_ops button_qcom_pmic_ops = {
+	.get_state	= qcom_pwrkey_get_state,
+	.get_code	= qcom_pwrkey_get_code,
+};
+
 static const struct udevice_id qcom_pwrkey_ids[] = {
 	{ .compatible = "qcom,pm8916-pwrkey" },
 	{ .compatible = "qcom,pm8994-pwrkey" },
-	{ .compatible = "qcom,pm8998-pwrkey" },
+	{ .compatible = "qcom,pm8941-pwrkey" },
+	{ .compatible = "qcom,pm8998-pon" },
 	{ }
 };
 
 U_BOOT_DRIVER(pwrkey_qcom) = {
 	.name	= "pwrkey_qcom",
-	.id	= UCLASS_GPIO,
+	.id	= UCLASS_BUTTON,
 	.of_match = qcom_pwrkey_ids,
-	.of_to_plat = qcom_pwrkey_of_to_plat,
+	.bind = button_qcom_pmic_bind,
 	.probe	= qcom_pwrkey_probe,
-	.ops	= &qcom_pwrkey_ops,
-	.priv_auto	= sizeof(struct qcom_gpio_bank),
+	.ops	= &button_qcom_pmic_ops,
+	.priv_auto	= sizeof(struct qcom_pmic_btn_priv),
 };

-- 
2.42.0

[PATCH 3/5] gpio: qcom_pmic: fix support for upstream DT

[Caleb Connolly]

Linux devicetrees use the "gpio-ranges" property, add support for
parsing it instead of "gpio-count" so that upstream DTs can be used with
U-Boot.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/gpio/qcom_pmic_gpio.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c
index 3dbc02d83198..e6405881c637 100644
--- a/drivers/gpio/qcom_pmic_gpio.c
+++ b/drivers/gpio/qcom_pmic_gpio.c
@@ -247,11 +247,37 @@ static int qcom_gpio_probe(struct udevice *dev)
 	return 0;
 }
 
+/*
+ * Parse basic GPIO count specified via the gpio-ranges property
+ * as specified in Linux devicetrees
+ * Returns < 0 on error, otherwise gpio count
+ */
+static int qcom_gpio_of_parse_ranges(struct udevice *dev)
+{
+	int ret;
+	struct ofnode_phandle_args args;
+
+	ret = ofnode_parse_phandle_with_args(dev_ofnode(dev), "gpio-ranges",
+					     NULL, 3, 0, &args);
+	if (ret)
+		return log_msg_ret("gpio-ranges", ret);
+
+	return args.args[2];
+}
+
 static int qcom_gpio_of_to_plat(struct udevice *dev)
 {
 	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+	int ret;
 
 	uc_priv->gpio_count = dev_read_u32_default(dev, "gpio-count", 0);
+	if (!uc_priv->gpio_count) {
+		ret = qcom_gpio_of_parse_ranges(dev);
+		if (ret > 0)
+			uc_priv->gpio_count = ret;
+		else
+			printf("gpio-ranges error: %d\n", ret);
+	}
 	uc_priv->bank_name = dev_read_string(dev, "gpio-bank-name");
 	if (uc_priv->bank_name == NULL)
 		uc_priv->bank_name = "qcom_pmic";

-- 
2.42.0

[PATCH 4/5] spmi: msm: fix register range names

[Caleb Connolly]

The core and chnl register ranges were swapped on SDM845. Fix it, and
fetch the register ranges by name instead of by index.

Drop the cosmetic "version" variable and clean up the debug logging.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 arch/arm/dts/sdm845.dtsi |  2 +-
 drivers/spmi/spmi-msm.c  | 46 ++++++++++++++++++----------------------------
 2 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi
index 4798ace0ff8b..98f3744027bb 100644
--- a/arch/arm/dts/sdm845.dtsi
+++ b/arch/arm/dts/sdm845.dtsi
@@ -63,7 +63,7 @@
 			reg = <0xc440000 0x1100>,
 			      <0xc600000 0x2000000>,
 			      <0xe600000 0x100000>;
-			reg-names = "cnfg", "core", "obsrvr";
+			reg-names = "core", "chnls", "obsrvr";
 			#address-cells = <0x1>;
 			#size-cells = <0x1>;
 
diff --git a/drivers/spmi/spmi-msm.c b/drivers/spmi/spmi-msm.c
index 27a035c0a595..f8834e60c266 100644
--- a/drivers/spmi/spmi-msm.c
+++ b/drivers/spmi/spmi-msm.c
@@ -70,7 +70,7 @@ enum pmic_arb_channel {
 
 struct msm_spmi_priv {
 	phys_addr_t arb_chnl;  /* ARB channel mapping base */
-	phys_addr_t spmi_core; /* SPMI core */
+	phys_addr_t spmi_chnls; /* SPMI core */
 	phys_addr_t spmi_obs;  /* SPMI observer */
 	/* SPMI channel map */
 	uint8_t channel_map[SPMI_MAX_SLAVES][SPMI_MAX_PERIPH];
@@ -95,10 +95,10 @@ static int msm_spmi_write(struct udevice *dev, int usid, int pid, int off,
 
 	/* Disable IRQ mode for the current channel*/
 	writel(0x0,
-	       priv->spmi_core + SPMI_CH_OFFSET(channel) + SPMI_REG_CONFIG);
+	       priv->spmi_chnls + SPMI_CH_OFFSET(channel) + SPMI_REG_CONFIG);
 
 	/* Write single byte */
-	writel(val, priv->spmi_core + SPMI_CH_OFFSET(channel) + SPMI_REG_WDATA);
+	writel(val, priv->spmi_chnls + SPMI_CH_OFFSET(channel) + SPMI_REG_WDATA);
 
 	/* Prepare write command */
 	reg |= SPMI_CMD_EXT_REG_WRITE_LONG << SPMI_CMD_OPCODE_SHIFT;
@@ -113,12 +113,12 @@ static int msm_spmi_write(struct udevice *dev, int usid, int pid, int off,
 		ch_offset = SPMI_CH_OFFSET(channel);
 
 	/* Send write command */
-	writel(reg, priv->spmi_core + SPMI_CH_OFFSET(channel) + SPMI_REG_CMD0);
+	writel(reg, priv->spmi_chnls + SPMI_CH_OFFSET(channel) + SPMI_REG_CMD0);
 
 	/* Wait till CMD DONE status */
 	reg = 0;
 	while (!reg) {
-		reg = readl(priv->spmi_core + SPMI_CH_OFFSET(channel) +
+		reg = readl(priv->spmi_chnls + SPMI_CH_OFFSET(channel) +
 			    SPMI_REG_STATUS);
 	}
 
@@ -186,47 +186,37 @@ static struct dm_spmi_ops msm_spmi_ops = {
 static int msm_spmi_probe(struct udevice *dev)
 {
 	struct msm_spmi_priv *priv = dev_get_priv(dev);
-	phys_addr_t config_addr;
+	phys_addr_t core_addr;
 	u32 hw_ver;
-	u32 version;
 	int i;
-	int err;
 
-	config_addr = dev_read_addr_index(dev, 0);
-	priv->spmi_core = dev_read_addr_index(dev, 1);
-	priv->spmi_obs = dev_read_addr_index(dev, 2);
+	core_addr = dev_read_addr_name(dev, "core");
+	priv->spmi_chnls = dev_read_addr_name(dev, "chnls");
+	priv->spmi_obs = dev_read_addr_name(dev, "obsrvr");
 
-	hw_ver = readl(config_addr + PMIC_ARB_VERSION);
+	hw_ver = readl(core_addr + PMIC_ARB_VERSION);
 
 	if (hw_ver < PMIC_ARB_VERSION_V3_MIN) {
 		priv->arb_ver = V2;
-		version = 2;
-		priv->arb_chnl = config_addr + APID_MAP_OFFSET_V1_V2_V3;
+		priv->arb_chnl = core_addr + APID_MAP_OFFSET_V1_V2_V3;
 	} else if (hw_ver < PMIC_ARB_VERSION_V5_MIN) {
 		priv->arb_ver = V3;
-		version = 3;
-		priv->arb_chnl = config_addr + APID_MAP_OFFSET_V1_V2_V3;
+		priv->arb_chnl = core_addr + APID_MAP_OFFSET_V1_V2_V3;
 	} else {
 		priv->arb_ver = V5;
-		version = 5;
-		priv->arb_chnl = config_addr + APID_MAP_OFFSET_V5;
-
-		if (err) {
-			dev_err(dev, "could not read APID->PPID mapping table, rc= %d\n", err);
-			return -1;
-		}
+		priv->arb_chnl = core_addr + APID_MAP_OFFSET_V5;
 	}
 
-	dev_dbg(dev, "PMIC Arb Version-%d (0x%x)\n", version, hw_ver);
+	dev_dbg(dev, "PMIC Arb Version-%d (%#x)\n", hw_ver >> 28, hw_ver);
 
 	if (priv->arb_chnl == FDT_ADDR_T_NONE ||
-	    priv->spmi_core == FDT_ADDR_T_NONE ||
+	    priv->spmi_chnls == FDT_ADDR_T_NONE ||
 	    priv->spmi_obs == FDT_ADDR_T_NONE)
 		return -EINVAL;
 
-	dev_dbg(dev, "priv->arb_chnl address (%llu)\n", priv->arb_chnl);
-	dev_dbg(dev, "priv->spmi_core address (%llu)\n", priv->spmi_core);
-	dev_dbg(dev, "priv->spmi_obs address (%llu)\n", priv->spmi_obs);
+	dev_dbg(dev, "priv->arb_chnl address (%#08llx)\n", priv->arb_chnl);
+	dev_dbg(dev, "priv->spmi_chnls address (%#08llx)\n", priv->spmi_chnls);
+	dev_dbg(dev, "priv->spmi_obs address (%#08llx)\n", priv->spmi_obs);
 	/* Scan peripherals connected to each SPMI channel */
 	for (i = 0; i < SPMI_MAX_PERIPH; i++) {
 		uint32_t periph = readl(priv->arb_chnl + ARB_CHANNEL_OFFSET(i));

-- 
2.42.0

[PATCH 5/5] pmic: qcom: dont use dev_read_addr to get USID

[Caleb Connolly]

Linux DTs stuff a value indicating if the USID is a USID or a GSID in the
reg property, the Linux SPMI driver then reads the two address cells
separately. U-boot's dev_read_addr() doesn't know how to handle this, so
use ofnode_read_u32_index() to get just the USID.

The Qcom pmic driver doesn't have support for GSID handling, so just
ignore the second value for now.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/power/pmic/pmic_qcom.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/power/pmic/pmic_qcom.c b/drivers/power/pmic/pmic_qcom.c
index ad8daf43f06f..f2ac6494811d 100644
--- a/drivers/power/pmic/pmic_qcom.c
+++ b/drivers/power/pmic/pmic_qcom.c
@@ -66,12 +66,19 @@ static const struct udevice_id pmic_qcom_ids[] = {
 static int pmic_qcom_probe(struct udevice *dev)
 {
 	struct pmic_qcom_priv *priv = dev_get_priv(dev);
+	int ret;
 
-	priv->usid = dev_read_addr(dev);
-
-	if (priv->usid == FDT_ADDR_T_NONE)
+	/*
+	 * dev_read_addr() can't be used here because the reg property actually
+	 * contains two discrete values, not a single 64-bit address.
+	 * The address is the first value.
+	 */
+	ret = ofnode_read_u32_index(dev_ofnode(dev), "reg", 0, &priv->usid);
+	if (ret < 0)
 		return -EINVAL;
 
+	debug("usid: %d\n", priv->usid);
+
 	return 0;
 }
 

-- 
2.42.0

Re: [PATCH 2/5] gpio: qcom_pmic: rework pwrkey driver into a button driver

[Stephan Gerhold]

On Mon, Nov 06, 2023 at 08:57:30PM +0000, Caleb Connolly wrote:
> The power and resin keys were implemented as GPIOs here, but their only
> use would be as buttons. Avoid the additional layer of introspection and
> rework this driver into a button driver.
> 
> While we're here, replace the "qcom,pm8998-pwrkey" compatible with
> "qcom,pm8941-pwrkey" to match upstream (Linux).
> 
> The dragonboard410c and 820c boards are adjusted to benefit from this
> change too, simplify their custom board init code.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>

Thanks a lot for working on bringing the Qualcomm DTs in U-Boot closer
to Linux upstream! I agree that modelling the pwr/resin keys is better
than exposing tham as GPIOs.

I'm a bit confused about the actual diff in this patch series though.
Did you perhaps forget to make some changes you had planned or sent the
wrong version?

In particular:

 - You talk about replacing the custom "qcom,pm8998-pwrkey" compatible
   with "qcom,pm8941-pwrkey" to match upstream, but don't seem to adjust
   the users (sdm845.dtsi)?

 - sdm845.dtsi also uses GPIOs for PON, but you only update DB410c and
   DB820c. Isn't SDM845 the platform you're testing on?

Some more comments below.

> ---
>  arch/arm/dts/dragonboard410c-uboot.dtsi          |  11 +-
>  arch/arm/dts/dragonboard820c-uboot.dtsi          |   9 +-
>  arch/arm/dts/dragonboard820c.dts                 |   3 -
>  board/qualcomm/dragonboard410c/dragonboard410c.c |  29 ++--
>  board/qualcomm/dragonboard820c/dragonboard820c.c |  29 ++--
>  drivers/gpio/Kconfig                             |   3 +-
>  drivers/gpio/qcom_pmic_gpio.c                    | 161 +++++++++++++++--------
>  7 files changed, 139 insertions(+), 106 deletions(-)
> 
> diff --git a/arch/arm/dts/dragonboard410c-uboot.dtsi b/arch/arm/dts/dragonboard410c-uboot.dtsi
> index 3b0bd0ed0a1b..c96f1fcc8930 100644
> --- a/arch/arm/dts/dragonboard410c-uboot.dtsi
> +++ b/arch/arm/dts/dragonboard410c-uboot.dtsi
> @@ -5,6 +5,9 @@
>   * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
>   */
>  
> +#include <dt-bindings/input/linux-event-codes.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +
>  / {
>  
>  	smem {
> @@ -46,10 +49,14 @@
>  
>  &pm8916_pon {
>  	key_vol_down {
> -		gpios = <&pm8916_pon 1 0>;
> +		interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>;
> +		linux,code = <KEY_DOWN>;
> +		label = "key_vol_down";
>  	};
>  
>  	key_power {
> -		gpios = <&pm8916_pon 0 0>;
> +		interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
> +		linux,code = <KEY_ENTER>;
> +		label = "key_power";
>  	};
>  };

The upstream Linux DT looks like this:

		pon@800 {
			compatible = "qcom,pm8916-pon";
			reg = <0x800>;
			mode-bootloader = <0x2>;
			mode-recovery = <0x1>;

			pwrkey {
				compatible = "qcom,pm8941-pwrkey";
				interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
				debounce = <15625>;
				bias-pull-up;
				linux,code = <KEY_POWER>;
			};

			pm8916_resin: resin {
				compatible = "qcom,pm8941-resin";
				interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>;
				debounce = <15625>;
				bias-pull-up;
				linux,code = <KEY_VOLUME_DOWN>;
			};
		};

The new version you add is closer to upstream, but you also add a new
custom property called "label". You could just derive a unique label
from the node name ("pwrkey" vs "resin").

For looking up the buttons in the DB410c/DB820c couldn't you just loop
over all buttons and find a suitable one based on button_get_code()?

I think having different *linux*,code values (KEY_POWER vs KEY_ENTER and
KEY_DOWN vs KEY_VOLUME_DOWN) is also a bit strange. If U-Boot wants
different key codes it's kind of not the Linux code anymore, might as
well call it "u-boot,code" then. :-)

If KEY_POWER => KEY_ENTER and KEY_VOLUME_DOWN => KEY_DOWN is more useful
for U-Boot maybe that mapping could be done automatically in the code,
without having to change the real hardware description in the DT.

> diff --git a/arch/arm/dts/dragonboard820c.dts b/arch/arm/dts/dragonboard820c.dts
> index ad201d48749c..7db0cc9d64cc 100644
> --- a/arch/arm/dts/dragonboard820c.dts
> +++ b/arch/arm/dts/dragonboard820c.dts
> @@ -112,9 +112,6 @@
>  				pm8994_pon: pm8994_pon@800 {
>  					compatible = "qcom,pm8994-pwrkey";
>  					reg = <0x800 0x96>;
> -					#gpio-cells = <2>;
> -					gpio-controller;
> -					gpio-bank-name="pm8994_key.";
>  				};

Shouldn't we do the same change for pm8916_pon in db410c.dts?

> diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c
> index e5841f502953..3dbc02d83198 100644
> --- a/drivers/gpio/qcom_pmic_gpio.c
> +++ b/drivers/gpio/qcom_pmic_gpio.c
> @@ -5,8 +5,10 @@
>   * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
>   */
>  
> +#include <button.h>
>  #include <common.h>
>  #include <dm.h>
> +#include <dm/lists.h>
>  #include <log.h>
>  #include <power/pmic.h>
>  #include <spmi/spmi.h>
> @@ -275,107 +277,150 @@ U_BOOT_DRIVER(qcom_pmic_gpio) = {
>  	.priv_auto	= sizeof(struct qcom_gpio_bank),
>  };
>  
> +struct qcom_pmic_btn_priv {
> +	u32 base;
> +	u32 status_bit;
> +	int code;
> +	struct udevice *pmic;
> +};
>  
>  /* Add pmic buttons as GPIO as well - there is no generic way for now */
>  #define PON_INT_RT_STS                        0x10
>  #define KPDPWR_ON_INT_BIT                     0
>  #define RESIN_ON_INT_BIT                      1
>  
> -static int qcom_pwrkey_get_function(struct udevice *dev, unsigned offset)
> +static enum button_state_t qcom_pwrkey_get_state(struct udevice *dev)
>  {
> -	return GPIOF_INPUT;
> -}
> +	struct qcom_pmic_btn_priv *priv = dev_get_priv(dev);
>  
> -static int qcom_pwrkey_get_value(struct udevice *dev, unsigned offset)
> -{
> -	struct qcom_gpio_bank *priv = dev_get_priv(dev);
> -
> -	int reg = pmic_reg_read(dev->parent, priv->pid + PON_INT_RT_STS);
> +	int reg = pmic_reg_read(priv->pmic, priv->base + PON_INT_RT_STS);
>  
>  	if (reg < 0)
>  		return 0;
>  
> -	switch (offset) {
> -	case 0: /* Power button */
> -		return (reg & BIT(KPDPWR_ON_INT_BIT)) != 0;
> -		break;
> -	case 1: /* Reset button */
> -	default:
> -		return (reg & BIT(RESIN_ON_INT_BIT)) != 0;
> -		break;
> -	}
> +	return (reg & BIT(priv->status_bit)) != 0;
>  }
>  
> -/*
> - * Since pmic buttons modelled as GPIO, we need empty direction functions
> - * to trick u-boot button driver
> - */
> -static int qcom_pwrkey_direction_input(struct udevice *dev, unsigned int offset)
> +static int qcom_pwrkey_get_code(struct udevice *dev)
>  {
> -	return 0;
> -}
> +	struct qcom_pmic_btn_priv *priv = dev_get_priv(dev);
>  
> -static int qcom_pwrkey_direction_output(struct udevice *dev, unsigned int offset, int value)
> -{
> -	return -EOPNOTSUPP;
> +	return priv->code;
>  }
>  
> -static const struct dm_gpio_ops qcom_pwrkey_ops = {
> -	.get_value		= qcom_pwrkey_get_value,
> -	.get_function		= qcom_pwrkey_get_function,
> -	.direction_input	= qcom_pwrkey_direction_input,
> -	.direction_output	= qcom_pwrkey_direction_output,
> -};
> -
>  static int qcom_pwrkey_probe(struct udevice *dev)
>  {
> -	struct qcom_gpio_bank *priv = dev_get_priv(dev);
> -	int reg;
> -	u64 pid;
> +	struct button_uc_plat *uc_plat = dev_get_uclass_plat(dev);
> +	struct qcom_pmic_btn_priv *priv = dev_get_priv(dev);
> +	int ret;
> +	u64 base;
>  
> -	pid = dev_read_addr(dev);
> -	if (pid == FDT_ADDR_T_NONE)
> -		return log_msg_ret("bad address", -EINVAL);
> +	/* Ignore the top-level button node */
> +	if (!uc_plat->label)
> +		return 0;
>  
> -	priv->pid = pid;
> +	/* the pwrkey and resin nodes are children of the "pon" node, get the
> +	 * PMIC device to use in pmic_reg_* calls.
> +	 */
> +	priv->pmic = dev->parent->parent;
> +
> +	base = dev_read_addr(dev);
> +	if (!base || base == FDT_ADDR_T_NONE) {
> +		/* Linux devicetrees don't specify an address in the pwrkey node */
> +		base = dev_read_addr(dev->parent);
> +		if (base == FDT_ADDR_T_NONE) {
> +			printf("%s: Can't find address\n", dev->name);
> +			return -EINVAL;
> +		}
> +	}

Is it worth introducing new code that supports non-standard Linux DTs?
Or do we need to stay compatible with old U-Boot DTs too? Would expect
those are always bundled together with U-Boot.

> +
> +	priv->base = base;
>  
>  	/* Do a sanity check */
> -	reg = pmic_reg_read(dev->parent, priv->pid + REG_TYPE);
> -	if (reg != 0x1)
> -		return log_msg_ret("bad type", -ENXIO);
> +	ret = pmic_reg_read(priv->pmic, priv->base + REG_TYPE);
> +	if (ret != 0x1 && ret != 0xb) {
> +		printf("%s: unexpected PMIC function type %d\n", dev->name, ret);
> +		return -ENXIO;
> +	}
>  
> -	reg = pmic_reg_read(dev->parent, priv->pid + REG_SUBTYPE);
> -	if ((reg & 0x5) == 0)
> -		return log_msg_ret("bad subtype", -ENXIO);
> +	ret = pmic_reg_read(priv->pmic, priv->base + REG_SUBTYPE);
> +	if ((ret & 0x7) == 0) {
> +		printf("%s: unexpected PMCI function subtype %d\n", dev->name, ret);
> +		//return -ENXIO;

I guess this shouldn't be commented out? :D

> +	}
> +
> +	/* Bit of a hack, we use the interrupt number to derive if this is the pwrkey or resin
> +	 * node, it just so happens to line up with the bit numbers in the interrupt status register
> +	 */
> +	ret = ofnode_read_u32_index(dev_ofnode(dev), "interrupts", 2, &priv->status_bit);
> +	if (ret < 0) {
> +		printf("%s: Couldn't read interrupts: %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	ret = ofnode_read_u32(dev_ofnode(dev), "linux,code", &priv->code);
> +	if (ret < 0) {
> +		printf("%s: Couldn't read interrupts: %d\n", __func__, ret);
> +		return ret;
> +	}
>  
>  	return 0;
>  }
>  
> -static int qcom_pwrkey_of_to_plat(struct udevice *dev)
> +static int button_qcom_pmic_bind(struct udevice *parent)
>  {
> -	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +	struct udevice *dev;
> +	ofnode node;
> +	int ret;
>  
> -	uc_priv->gpio_count = 2;
> -	uc_priv->bank_name = dev_read_string(dev, "gpio-bank-name");
> -	if (uc_priv->bank_name == NULL)
> -		uc_priv->bank_name = "pwkey_qcom";
> +	dev_for_each_subnode(node, parent) {
> +		struct button_uc_plat *uc_plat;
> +		const char *label;
> +
> +		if (!ofnode_is_enabled(node))
> +			continue;
> +
> +		label = ofnode_read_string(node, "label");
> +		if (!label) {
> +			printf("%s: node %s has no label\n", __func__,
> +			       ofnode_get_name(node));
> +			/* Don't break booting, just print a warning and continue */
> +			continue;
> +		}
> +		/* We need the PMIC device to be the parent, so flatten it out here */
> +		ret = device_bind_driver_to_node(parent, "pwrkey_qcom",
> +						 ofnode_get_name(node),
> +						 node, &dev);
> +		if (ret) {
> +			printf("Failed to bind %s! %d\n", label, ret);
> +			return ret;
> +		}
> +		uc_plat = dev_get_uclass_plat(dev);
> +		uc_plat->label = label;
> +	}
>  
>  	return 0;
>  }
>  
> +static const struct button_ops button_qcom_pmic_ops = {
> +	.get_state	= qcom_pwrkey_get_state,
> +	.get_code	= qcom_pwrkey_get_code,
> +};
> +
>  static const struct udevice_id qcom_pwrkey_ids[] = {
>  	{ .compatible = "qcom,pm8916-pwrkey" },
>  	{ .compatible = "qcom,pm8994-pwrkey" },

These are also qcom,pm8941-pwrkey upstream.

> -	{ .compatible = "qcom,pm8998-pwrkey" },
> +	{ .compatible = "qcom,pm8941-pwrkey" },
> +	{ .compatible = "qcom,pm8998-pon" },

"qcom,pm8998-pon" is the outer container node for pwrkey+resin, while
"qcom,pm8941-pwrkey" is the actual power button. Why are both here next
to each other?

>  	{ }
>  };
>  
>  U_BOOT_DRIVER(pwrkey_qcom) = {
>  	.name	= "pwrkey_qcom",
> -	.id	= UCLASS_GPIO,
> +	.id	= UCLASS_BUTTON,
>  	.of_match = qcom_pwrkey_ids,
> -	.of_to_plat = qcom_pwrkey_of_to_plat,
> +	.bind = button_qcom_pmic_bind,
>  	.probe	= qcom_pwrkey_probe,
> -	.ops	= &qcom_pwrkey_ops,
> -	.priv_auto	= sizeof(struct qcom_gpio_bank),
> +	.ops	= &button_qcom_pmic_ops,
> +	.priv_auto	= sizeof(struct qcom_pmic_btn_priv),
>  };
> 

Can we move this out of the drivers/gpio into drivers/button? Seems like
there are now two quite different drivers in the same file. :-)

Thanks,
Stephan

Re: [PATCH 2/5] gpio: qcom_pmic: rework pwrkey driver into a button driver

[Caleb Connolly]

On 07/11/2023 13:27, Stephan Gerhold wrote:
> On Mon, Nov 06, 2023 at 08:57:30PM +0000, Caleb Connolly wrote:
>> The power and resin keys were implemented as GPIOs here, but their only
>> use would be as buttons. Avoid the additional layer of introspection and
>> rework this driver into a button driver.
>>
>> While we're here, replace the "qcom,pm8998-pwrkey" compatible with
>> "qcom,pm8941-pwrkey" to match upstream (Linux).
>>
>> The dragonboard410c and 820c boards are adjusted to benefit from this
>> change too, simplify their custom board init code.
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> 
> Thanks a lot for working on bringing the Qualcomm DTs in U-Boot closer
> to Linux upstream! I agree that modelling the pwr/resin keys is better
> than exposing tham as GPIOs.
> 
> I'm a bit confused about the actual diff in this patch series though.
> Did you perhaps forget to make some changes you had planned or sent the
> wrong version?
> 
> In particular:
> 
>  - You talk about replacing the custom "qcom,pm8998-pwrkey" compatible
>    with "qcom,pm8941-pwrkey" to match upstream, but don't seem to adjust
>    the users (sdm845.dtsi)?
> 
>  - sdm845.dtsi also uses GPIOs for PON, but you only update DB410c and
>    DB820c. Isn't SDM845 the platform you're testing on?

Argh, yeah you're totally right, I have been testing in the context of
upstream sdm845 DT and I forgot to pull in the individual changes for
this series.
> 
> Some more comments below.
> 
>> ---
>>  arch/arm/dts/dragonboard410c-uboot.dtsi          |  11 +-
>>  arch/arm/dts/dragonboard820c-uboot.dtsi          |   9 +-
>>  arch/arm/dts/dragonboard820c.dts                 |   3 -
>>  board/qualcomm/dragonboard410c/dragonboard410c.c |  29 ++--
>>  board/qualcomm/dragonboard820c/dragonboard820c.c |  29 ++--
>>  drivers/gpio/Kconfig                             |   3 +-
>>  drivers/gpio/qcom_pmic_gpio.c                    | 161 +++++++++++++++--------
>>  7 files changed, 139 insertions(+), 106 deletions(-)
>>
>> diff --git a/arch/arm/dts/dragonboard410c-uboot.dtsi b/arch/arm/dts/dragonboard410c-uboot.dtsi
>> index 3b0bd0ed0a1b..c96f1fcc8930 100644
>> --- a/arch/arm/dts/dragonboard410c-uboot.dtsi
>> +++ b/arch/arm/dts/dragonboard410c-uboot.dtsi
>> @@ -5,6 +5,9 @@
>>   * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
>>   */
>>  
>> +#include <dt-bindings/input/linux-event-codes.h>
>> +#include <dt-bindings/interrupt-controller/irq.h>
>> +
>>  / {
>>  
>>  	smem {
>> @@ -46,10 +49,14 @@
>>  
>>  &pm8916_pon {
>>  	key_vol_down {
>> -		gpios = <&pm8916_pon 1 0>;
>> +		interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>;
>> +		linux,code = <KEY_DOWN>;
>> +		label = "key_vol_down";
>>  	};
>>  
>>  	key_power {
>> -		gpios = <&pm8916_pon 0 0>;
>> +		interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
>> +		linux,code = <KEY_ENTER>;
>> +		label = "key_power";
>>  	};
>>  };
> 
> The upstream Linux DT looks like this:
> 
> 		pon@800 {
> 			compatible = "qcom,pm8916-pon";
> 			reg = <0x800>;
> 			mode-bootloader = <0x2>;
> 			mode-recovery = <0x1>;
> 
> 			pwrkey {
> 				compatible = "qcom,pm8941-pwrkey";
> 				interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
> 				debounce = <15625>;
> 				bias-pull-up;
> 				linux,code = <KEY_POWER>;
> 			};
> 
> 			pm8916_resin: resin {
> 				compatible = "qcom,pm8941-resin";
> 				interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>;
> 				debounce = <15625>;
> 				bias-pull-up;
> 				linux,code = <KEY_VOLUME_DOWN>;
> 			};
> 		};
> 
> The new version you add is closer to upstream, but you also add a new
> custom property called "label". You could just derive a unique label
> from the node name ("pwrkey" vs "resin").

I just kinda followed what gpio-button does. I agree though this should
be dropped and we can just use the node name.
> 
> For looking up the buttons in the DB410c/DB820c couldn't you just loop
> over all buttons and find a suitable one based on button_get_code()?
> 
> I think having different *linux*,code values (KEY_POWER vs KEY_ENTER and
> KEY_DOWN vs KEY_VOLUME_DOWN) is also a bit strange. If U-Boot wants
> different key codes it's kind of not the Linux code anymore, might as
> well call it "u-boot,code" then. :-)

Yeah, I don't really know what the right solution is here, in U-Boot we
want to use these buttons as controls - like you would in ABL fastboot.

> 
> If KEY_POWER => KEY_ENTER and KEY_VOLUME_DOWN => KEY_DOWN is more useful
> for U-Boot maybe that mapping could be done automatically in the code,
> without having to change the real hardware description in the DT.

That sounds reasonable.
> 
>> diff --git a/arch/arm/dts/dragonboard820c.dts b/arch/arm/dts/dragonboard820c.dts
>> index ad201d48749c..7db0cc9d64cc 100644
>> --- a/arch/arm/dts/dragonboard820c.dts
>> +++ b/arch/arm/dts/dragonboard820c.dts
>> @@ -112,9 +112,6 @@
>>  				pm8994_pon: pm8994_pon@800 {
>>  					compatible = "qcom,pm8994-pwrkey";
>>  					reg = <0x800 0x96>;
>> -					#gpio-cells = <2>;
>> -					gpio-controller;
>> -					gpio-bank-name="pm8994_key.";
>>  				};
> 
> Shouldn't we do the same change for pm8916_pon in db410c.dts?

Yes
> 
>> diff --git a/drivers/gpio/qcom_pmic_gpio.c b/drivers/gpio/qcom_pmic_gpio.c
>> index e5841f502953..3dbc02d83198 100644
>> --- a/drivers/gpio/qcom_pmic_gpio.c
>> +++ b/drivers/gpio/qcom_pmic_gpio.c
>> @@ -5,8 +5,10 @@
>>   * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
>>   */
>>  
>> +#include <button.h>
>>  #include <common.h>
>>  #include <dm.h>
>> +#include <dm/lists.h>
>>  #include <log.h>
>>  #include <power/pmic.h>
>>  #include <spmi/spmi.h>
>> @@ -275,107 +277,150 @@ U_BOOT_DRIVER(qcom_pmic_gpio) = {
>>  	.priv_auto	= sizeof(struct qcom_gpio_bank),
>>  };
>>  
>> +struct qcom_pmic_btn_priv {
>> +	u32 base;
>> +	u32 status_bit;
>> +	int code;
>> +	struct udevice *pmic;
>> +};
>>  
>>  /* Add pmic buttons as GPIO as well - there is no generic way for now */
>>  #define PON_INT_RT_STS                        0x10
>>  #define KPDPWR_ON_INT_BIT                     0
>>  #define RESIN_ON_INT_BIT                      1
>>  
>> -static int qcom_pwrkey_get_function(struct udevice *dev, unsigned offset)
>> +static enum button_state_t qcom_pwrkey_get_state(struct udevice *dev)
>>  {
>> -	return GPIOF_INPUT;
>> -}
>> +	struct qcom_pmic_btn_priv *priv = dev_get_priv(dev);
>>  
>> -static int qcom_pwrkey_get_value(struct udevice *dev, unsigned offset)
>> -{
>> -	struct qcom_gpio_bank *priv = dev_get_priv(dev);
>> -
>> -	int reg = pmic_reg_read(dev->parent, priv->pid + PON_INT_RT_STS);
>> +	int reg = pmic_reg_read(priv->pmic, priv->base + PON_INT_RT_STS);
>>  
>>  	if (reg < 0)
>>  		return 0;
>>  
>> -	switch (offset) {
>> -	case 0: /* Power button */
>> -		return (reg & BIT(KPDPWR_ON_INT_BIT)) != 0;
>> -		break;
>> -	case 1: /* Reset button */
>> -	default:
>> -		return (reg & BIT(RESIN_ON_INT_BIT)) != 0;
>> -		break;
>> -	}
>> +	return (reg & BIT(priv->status_bit)) != 0;
>>  }
>>  
>> -/*
>> - * Since pmic buttons modelled as GPIO, we need empty direction functions
>> - * to trick u-boot button driver
>> - */
>> -static int qcom_pwrkey_direction_input(struct udevice *dev, unsigned int offset)
>> +static int qcom_pwrkey_get_code(struct udevice *dev)
>>  {
>> -	return 0;
>> -}
>> +	struct qcom_pmic_btn_priv *priv = dev_get_priv(dev);
>>  
>> -static int qcom_pwrkey_direction_output(struct udevice *dev, unsigned int offset, int value)
>> -{
>> -	return -EOPNOTSUPP;
>> +	return priv->code;
>>  }
>>  
>> -static const struct dm_gpio_ops qcom_pwrkey_ops = {
>> -	.get_value		= qcom_pwrkey_get_value,
>> -	.get_function		= qcom_pwrkey_get_function,
>> -	.direction_input	= qcom_pwrkey_direction_input,
>> -	.direction_output	= qcom_pwrkey_direction_output,
>> -};
>> -
>>  static int qcom_pwrkey_probe(struct udevice *dev)
>>  {
>> -	struct qcom_gpio_bank *priv = dev_get_priv(dev);
>> -	int reg;
>> -	u64 pid;
>> +	struct button_uc_plat *uc_plat = dev_get_uclass_plat(dev);
>> +	struct qcom_pmic_btn_priv *priv = dev_get_priv(dev);
>> +	int ret;
>> +	u64 base;
>>  
>> -	pid = dev_read_addr(dev);
>> -	if (pid == FDT_ADDR_T_NONE)
>> -		return log_msg_ret("bad address", -EINVAL);
>> +	/* Ignore the top-level button node */
>> +	if (!uc_plat->label)
>> +		return 0;
>>  
>> -	priv->pid = pid;
>> +	/* the pwrkey and resin nodes are children of the "pon" node, get the
>> +	 * PMIC device to use in pmic_reg_* calls.
>> +	 */
>> +	priv->pmic = dev->parent->parent;
>> +
>> +	base = dev_read_addr(dev);
>> +	if (!base || base == FDT_ADDR_T_NONE) {
>> +		/* Linux devicetrees don't specify an address in the pwrkey node */
>> +		base = dev_read_addr(dev->parent);
>> +		if (base == FDT_ADDR_T_NONE) {
>> +			printf("%s: Can't find address\n", dev->name);
>> +			return -EINVAL;
>> +		}
>> +	}
> 
> Is it worth introducing new code that supports non-standard Linux DTs?
> Or do we need to stay compatible with old U-Boot DTs too? Would expect
> those are always bundled together with U-Boot.

I've been aiming to minimise the number of changes made to db410c and
db820c as I can't easily test on these boards... I don't think we need
to worry about backwards compatibility - the DTB is usually built-in to
the U-Boot image.
> 
>> +
>> +	priv->base = base;
>>  
>>  	/* Do a sanity check */
>> -	reg = pmic_reg_read(dev->parent, priv->pid + REG_TYPE);
>> -	if (reg != 0x1)
>> -		return log_msg_ret("bad type", -ENXIO);
>> +	ret = pmic_reg_read(priv->pmic, priv->base + REG_TYPE);
>> +	if (ret != 0x1 && ret != 0xb) {
>> +		printf("%s: unexpected PMIC function type %d\n", dev->name, ret);
>> +		return -ENXIO;
>> +	}
>>  
>> -	reg = pmic_reg_read(dev->parent, priv->pid + REG_SUBTYPE);
>> -	if ((reg & 0x5) == 0)
>> -		return log_msg_ret("bad subtype", -ENXIO);
>> +	ret = pmic_reg_read(priv->pmic, priv->base + REG_SUBTYPE);
>> +	if ((ret & 0x7) == 0) {
>> +		printf("%s: unexpected PMCI function subtype %d\n", dev->name, ret);
>> +		//return -ENXIO;
> 
> I guess this shouldn't be commented out? :D

Nope! Thanks for catching that
> 
>> +	}
>> +
>> +	/* Bit of a hack, we use the interrupt number to derive if this is the pwrkey or resin
>> +	 * node, it just so happens to line up with the bit numbers in the interrupt status register
>> +	 */
>> +	ret = ofnode_read_u32_index(dev_ofnode(dev), "interrupts", 2, &priv->status_bit);
>> +	if (ret < 0) {
>> +		printf("%s: Couldn't read interrupts: %d\n", __func__, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = ofnode_read_u32(dev_ofnode(dev), "linux,code", &priv->code);
>> +	if (ret < 0) {
>> +		printf("%s: Couldn't read interrupts: %d\n", __func__, ret);
>> +		return ret;
>> +	}
>>  
>>  	return 0;
>>  }
>>  
>> -static int qcom_pwrkey_of_to_plat(struct udevice *dev)
>> +static int button_qcom_pmic_bind(struct udevice *parent)
>>  {
>> -	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>> +	struct udevice *dev;
>> +	ofnode node;
>> +	int ret;
>>  
>> -	uc_priv->gpio_count = 2;
>> -	uc_priv->bank_name = dev_read_string(dev, "gpio-bank-name");
>> -	if (uc_priv->bank_name == NULL)
>> -		uc_priv->bank_name = "pwkey_qcom";
>> +	dev_for_each_subnode(node, parent) {
>> +		struct button_uc_plat *uc_plat;
>> +		const char *label;
>> +
>> +		if (!ofnode_is_enabled(node))
>> +			continue;
>> +
>> +		label = ofnode_read_string(node, "label");
>> +		if (!label) {
>> +			printf("%s: node %s has no label\n", __func__,
>> +			       ofnode_get_name(node));
>> +			/* Don't break booting, just print a warning and continue */
>> +			continue;
>> +		}
>> +		/* We need the PMIC device to be the parent, so flatten it out here */
>> +		ret = device_bind_driver_to_node(parent, "pwrkey_qcom",
>> +						 ofnode_get_name(node),
>> +						 node, &dev);
>> +		if (ret) {
>> +			printf("Failed to bind %s! %d\n", label, ret);
>> +			return ret;
>> +		}
>> +		uc_plat = dev_get_uclass_plat(dev);
>> +		uc_plat->label = label;
>> +	}
>>  
>>  	return 0;
>>  }
>>  
>> +static const struct button_ops button_qcom_pmic_ops = {
>> +	.get_state	= qcom_pwrkey_get_state,
>> +	.get_code	= qcom_pwrkey_get_code,
>> +};
>> +
>>  static const struct udevice_id qcom_pwrkey_ids[] = {
>>  	{ .compatible = "qcom,pm8916-pwrkey" },
>>  	{ .compatible = "qcom,pm8994-pwrkey" },
> 
> These are also qcom,pm8941-pwrkey upstream.
> 
>> -	{ .compatible = "qcom,pm8998-pwrkey" },
>> +	{ .compatible = "qcom,pm8941-pwrkey" },
>> +	{ .compatible = "qcom,pm8998-pon" },
> 
> "qcom,pm8998-pon" is the outer container node for pwrkey+resin, while
> "qcom,pm8941-pwrkey" is the actual power button. Why are both here next
> to each other?

This is a bit of a U-Boot trick, if you look in the probe function
you'll see the comment "Ignore the top-level button node". Basically the
same Driver is used to handle the NOP parent node and the children.
Otherwise I would have to add a new driver with UCLASS_NOP just to
handle the pon node and bind the children.
> 
>>  	{ }
>>  };
>>  
>>  U_BOOT_DRIVER(pwrkey_qcom) = {
>>  	.name	= "pwrkey_qcom",
>> -	.id	= UCLASS_GPIO,
>> +	.id	= UCLASS_BUTTON,
>>  	.of_match = qcom_pwrkey_ids,
>> -	.of_to_plat = qcom_pwrkey_of_to_plat,
>> +	.bind = button_qcom_pmic_bind,
>>  	.probe	= qcom_pwrkey_probe,
>> -	.ops	= &qcom_pwrkey_ops,
>> -	.priv_auto	= sizeof(struct qcom_gpio_bank),
>> +	.ops	= &button_qcom_pmic_ops,
>> +	.priv_auto	= sizeof(struct qcom_pmic_btn_priv),
>>  };
>>
> 
> Can we move this out of the drivers/gpio into drivers/button? Seems like
> there are now two quite different drivers in the same file. :-)

Yeah, fair enough. Will do.
> 
> Thanks,
> Stephan

-- 
// Caleb (they/them)