[PATCH v3 0/7] Add support for Qualcomm QCA639x chips family

[PATCH v3 0/7] Add support for Qualcomm QCA639x chips family

[Dmitry Baryshkov]

Qualcomm QCA639x is a family of WiFi + Bluetooth chips, with BT part
being controlled through the UART and WiFi being present on PCIe
bus. Both blocks use common power management unit wich should be turned
on before either of devices can be probed. Add common 'qcom-qca6390'
driver providing regulator to be used by both BT and PCIe parts.

Changes since v2:
 - Rebase on top of linux-next to get wcn6750 changes.
 - Switch from misc device providing power domains into regulator code.
 - Use vddpe-3v3-supply as expected by pcie-qcom driver.
 - Use enable_gpio functionality in hci_qca driver.

Changes since v1:
 - Stopped using wildcard in the dts binding, stick to qcom,qca6390.
 - Stopped using pcie0_phy for qca639x power domain.
 - Describe root PCIe bridge in the dts and bind power domain to the
   bridge.
 - Add pci quirk to power up power domains connected to this bridge.

----------------------------------------------------------------
Dmitry Baryshkov (7):
      dt-bindings: regulator: qcom,qca6390: add binding for QCA6390 device
      regulator: qca6390: add support for QCA639x powerup sequence
      Bluetooth: hci_qca: provide default device data
      Bluetooth: hci_qca: merge qca_power into qca_serdev
      Bluetooth: hci_qca: merge wcn & non-wcn code paths
      Bluetooth: hci_qca: add power sequencer support to qca6390
      arm64: dts: qcom: qrb5165-rb5: add QCA6391 WiFi+BT SoC

 .../bindings/regulator/qcom,qca6390.yaml           |  70 +++++++
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts           |  56 +++++
 drivers/bluetooth/hci_qca.c                        | 225 ++++++++++-----------
 drivers/regulator/Kconfig                          |  13 ++
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/qcom-qca639x.c                   | 157 ++++++++++++++
 6 files changed, 401 insertions(+), 121 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom,qca6390.yaml
 create mode 100644 drivers/regulator/qcom-qca639x.c

[PATCH v3 1/7] dt-bindings: regulator: qcom,qca6390: add binding for QCA6390 device

[Dmitry Baryshkov]

Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
being controlled through the UART and WiFi being present on PCIe bus.
Both blocks share common power sources. Add binding to describe power
sequencing required to power up this device.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../bindings/regulator/qcom,qca6390.yaml      | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/qcom,qca6390.yaml

diff --git a/Documentation/devicetree/bindings/regulator/qcom,qca6390.yaml b/Documentation/devicetree/bindings/regulator/qcom,qca6390.yaml
new file mode 100644
index 000000000000..35315c521041
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/qcom,qca6390.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/regulator/qcom,qca6390.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Qualcomm QCA6390 WiFi + Bluetoot SoC bindings
+
+maintainers:
+  - Andy Gross <agross@kernel.org>
+  - Bjorn Andersson <bjorn.andersson@linaro.org>
+
+description: |
+  This binding describes thes Qualcomm QCA6390 or QCA6391 power supplies and
+  enablement pins.
+
+properties:
+  compatible:
+    const: qcom,qca6390
+
+  vddaon-supply:
+    description:
+      0.95V always-on LDO power input
+
+  vddpmu-supply:
+    description:
+      0.95V LDO power input to PMU
+
+  vddrfa1-supply:
+    description:
+      0.95V LDO power input to RFA
+
+  vddrfa2-supply:
+    description:
+      1.25V LDO power input to RFA
+
+  vddrfa3-supply:
+    description:
+      2V LDO power input to RFA
+
+  vddpcie1-supply:
+    description:
+      1.25V LDO power input to PCIe part
+
+  vddpcie2-supply:
+    description:
+      2V LDO power input to PCIe part
+
+  vddio-supply:
+    description:
+      1.8V VIO input
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    qca6390: qca6390 {
+      compatible = "qcom,qca6390";
+
+      vddaon-supply = <&vreg_s6a_0p95>;
+      vddpmu-supply = <&vreg_s2f_0p95>;
+      vddrfa1-supply = <&vreg_s2f_0p95>;
+      vddrfa2-supply = <&vreg_s8c_1p3>;
+      vddrfa3-supply = <&vreg_s5a_1p9>;
+      vddpcie1-supply = <&vreg_s8c_1p3>;
+      vddpcie2-supply = <&vreg_s5a_1p9>;
+      vddio-supply = <&vreg_s4a_1p8>;
+    };
+...
-- 
2.30.2

[PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Dmitry Baryshkov]

Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
being controlled through the UART and WiFi being present on PCIe
bus. Both blocks share common power sources. Add device driver handling
power sequencing of QCA6390/1.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/regulator/Kconfig        |  13 +++
 drivers/regulator/Makefile       |   1 +
 drivers/regulator/qcom-qca639x.c | 157 +++++++++++++++++++++++++++++++
 3 files changed, 171 insertions(+)
 create mode 100644 drivers/regulator/qcom-qca639x.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 3e7a38525cb3..7a560cddea7a 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -909,6 +909,19 @@ config REGULATOR_PWM
 	  This driver supports PWM controlled voltage regulators. PWM
 	  duty cycle can increase or decrease the voltage.
 
+config REGULATOR_QCOM_QCA639X
+	tristate "Qualcomm QCA639x WiFi/Bluetooth module support"
+	help
+	  If you say yes to this option, support will be included for Qualcomm
+	  QCA639x family of WiFi and Bluetooth SoCs. Note, this driver supports
+	  only power control for this SoC, you still have to enable individual
+	  Bluetooth and WiFi drivers. This driver is only necessary on ARM
+	  platforms with this chip. PCIe cards handle power sequencing on their
+	  own.
+
+	  Say M here if you want to include support for QCA639x chips as a
+	  module. This will build a module called "qcom-qca639x".
+
 config REGULATOR_QCOM_RPM
 	tristate "Qualcomm RPM regulator driver"
 	depends on MFD_QCOM_RPM
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 580b015296ea..129c2110b78d 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -99,6 +99,7 @@ obj-$(CONFIG_REGULATOR_MT6380)	+= mt6380-regulator.o
 obj-$(CONFIG_REGULATOR_MT6397)	+= mt6397-regulator.o
 obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
+obj-$(CONFIG_REGULATOR_QCOM_QCA639X) += qcom-qca639x.o
 obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
 obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
diff --git a/drivers/regulator/qcom-qca639x.c b/drivers/regulator/qcom-qca639x.c
new file mode 100644
index 000000000000..a2c78c0f8baa
--- /dev/null
+++ b/drivers/regulator/qcom-qca639x.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, Linaro Limited
+ */
+#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/slab.h>
+
+#define MAX_NUM_REGULATORS	8
+
+static struct vreg {
+	const char *name;
+	unsigned int load_uA;
+} vregs[MAX_NUM_REGULATORS] = {
+	/* 2.0 V */
+	{ "vddpcie2", 15000 },
+	{ "vddrfa3", 400000 },
+
+	/* 0.95 V */
+	{ "vddaon", 100000 },
+	{ "vddpmu", 1250000 },
+	{ "vddrfa1", 200000 },
+
+	/* 1.35 V */
+	{ "vddrfa2", 400000 },
+	{ "vddpcie1", 35000 },
+
+	/* 1.8 V */
+	{ "vddio", 20000 },
+};
+
+struct qca6390_data {
+	struct device *dev;
+	struct regulator_bulk_data regulators[MAX_NUM_REGULATORS];
+	size_t num_vregs;
+
+	struct regulator_desc desc;
+	struct regulator_dev *regulator_dev;
+	unsigned int enable_counter;
+};
+
+#define domain_to_data(domain) container_of(domain, struct qca6390_data, pd)
+
+static int qca6390_enable(struct regulator_dev *rdev)
+{
+	struct qca6390_data *data = rdev_get_drvdata(rdev);
+	int ret;
+
+	ret = regulator_bulk_enable(data->num_vregs, data->regulators);
+	if (ret) {
+		dev_err(data->dev, "Failed to enable regulators");
+		return ret;
+	}
+
+	/* Wait for 1ms before toggling enable pins. */
+	usleep_range(1000, 2000);
+
+	data->enable_counter++;
+
+	return 0;
+}
+
+static int qca6390_disable(struct regulator_dev *rdev)
+{
+	struct qca6390_data *data = rdev_get_drvdata(rdev);
+
+	regulator_bulk_disable(data->num_vregs, data->regulators);
+
+	data->enable_counter--;
+
+	return 0;
+}
+
+static int qca6390_is_enabled(struct regulator_dev *rdev)
+{
+	struct qca6390_data *data = rdev_get_drvdata(rdev);
+
+	return data->enable_counter > 0;
+}
+
+static const struct regulator_ops qca6390_ops = {
+	.enable = qca6390_enable,
+	.disable = qca6390_disable,
+	.is_enabled = qca6390_is_enabled,
+};
+
+static int qca6390_probe(struct platform_device *pdev)
+{
+	struct qca6390_data *data;
+	struct device *dev = &pdev->dev;
+	struct regulator_config cfg = { };
+	int i, ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->dev = dev;
+	data->num_vregs = ARRAY_SIZE(vregs);
+
+	for (i = 0; i < data->num_vregs; i++)
+		data->regulators[i].supply = vregs[i].name;
+
+	ret = devm_regulator_bulk_get(dev, data->num_vregs, data->regulators);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < data->num_vregs; i++) {
+		ret = regulator_set_load(data->regulators[i].consumer, vregs[i].load_uA);
+		if (ret)
+			return ret;
+	}
+
+	data->desc.name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	if (!data->desc.name)
+		return -ENOMEM;
+
+	data->desc.type = REGULATOR_VOLTAGE;
+	data->desc.owner = THIS_MODULE;
+	data->desc.ops = &qca6390_ops;
+
+	cfg.dev = dev;
+	cfg.of_node = dev->of_node;
+	cfg.driver_data = data;
+	cfg.init_data = of_get_regulator_init_data(dev, dev->of_node, &data->desc);
+
+	data->regulator_dev = devm_regulator_register(dev, &data->desc, &cfg);
+	if (IS_ERR(data->regulator_dev)) {
+		ret = PTR_ERR(data->regulator_dev);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, data);
+
+	return 0;
+}
+
+static const struct of_device_id qca6390_of_match[] = {
+	{ .compatible = "qcom,qca6390" },
+};
+
+static struct platform_driver qca6390_driver = {
+	.probe = qca6390_probe,
+	.driver = {
+		.name = "qca6390",
+		.of_match_table = qca6390_of_match,
+	},
+};
+
+module_platform_driver(qca6390_driver);
+MODULE_AUTHOR("Dmitry Baryshkov <dmitry.baryshkov@linaro.org>");
+MODULE_DESCRIPTION("Power control for Qualcomm QCA6390/1 BT/WiFi chip");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2

[PATCH v3 3/7] Bluetooth: hci_qca: provide default device data

[Dmitry Baryshkov]

In order to simplify probe function provide default device data. This
removes the rest of if (data) checks.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/bluetooth/hci_qca.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 53deea2eb7b4..3704dbadba1d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1874,6 +1874,11 @@ static const struct qca_device_data qca_soc_data_wcn6750 = {
 	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
 };
 
+static const struct qca_device_data qca_soc_data_default = {
+	.soc_type = QCA_ROME,
+	.num_vregs = 0,
+};
+
 static void qca_power_shutdown(struct hci_uart *hu)
 {
 	struct qca_serdev *qcadev;
@@ -2019,12 +2024,15 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	int err;
 	bool power_ctrl_enabled = true;
 
+	data = device_get_match_data(&serdev->dev);
+	if (!data)
+		return -EINVAL;
+
 	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
 	if (!qcadev)
 		return -ENOMEM;
 
 	qcadev->serdev_hu.serdev = serdev;
-	data = device_get_match_data(&serdev->dev);
 	serdev_device_set_drvdata(serdev, qcadev);
 	device_property_read_string(&serdev->dev, "firmware-name",
 					 &qcadev->firmware_name);
@@ -2033,9 +2041,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	if (!qcadev->oper_speed)
 		BT_DBG("UART will pick default operating speed");
 
-	if (data &&
-	    (qca_is_wcn399x(data->soc_type) ||
-	    qca_is_wcn6750(data->soc_type))) {
+	if ((qca_is_wcn399x(data->soc_type) ||
+	     qca_is_wcn6750(data->soc_type))) {
 		qcadev->btsoc_type = data->soc_type;
 		qcadev->bt_power = devm_kzalloc(&serdev->dev,
 						sizeof(struct qca_power),
@@ -2077,10 +2084,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 			return err;
 		}
 	} else {
-		if (data)
-			qcadev->btsoc_type = data->soc_type;
-		else
-			qcadev->btsoc_type = QCA_ROME;
+		qcadev->btsoc_type = data->soc_type;
 
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
 					       GPIOD_OUT_LOW);
@@ -2309,9 +2313,9 @@ static SIMPLE_DEV_PM_OPS(qca_pm_ops, qca_suspend, qca_resume);
 
 #ifdef CONFIG_OF
 static const struct of_device_id qca_bluetooth_of_match[] = {
-	{ .compatible = "qcom,qca6174-bt" },
+	{ .compatible = "qcom,qca6174-bt", .data = &qca_soc_data_default},
 	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
-	{ .compatible = "qcom,qca9377-bt" },
+	{ .compatible = "qcom,qca9377-bt", .data = &qca_soc_data_default},
 	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
 	{ .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991},
 	{ .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998},
-- 
2.30.2

[PATCH v3 4/7] Bluetooth: hci_qca: merge qca_power into qca_serdev

[Dmitry Baryshkov]

There is no need to allocate separate structure for handling regulators
used by QCA chips, we gain nothing from it. Move all used data fields
directly to struct qca_serdev.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/bluetooth/hci_qca.c | 58 ++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 3704dbadba1d..9cc8a9153d76 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -208,20 +208,15 @@ struct qca_device_data {
 /*
  * Platform data for the QCA Bluetooth power driver.
  */
-struct qca_power {
-	struct device *dev;
-	struct regulator_bulk_data *vreg_bulk;
-	int num_vregs;
-	bool vregs_on;
-};
-
 struct qca_serdev {
 	struct hci_uart	 serdev_hu;
 	struct gpio_desc *bt_en;
 	struct gpio_desc *sw_ctrl;
 	struct clk	 *susclk;
 	enum qca_btsoc_type btsoc_type;
-	struct qca_power *bt_power;
+	struct regulator_bulk_data *vreg_bulk;
+	int num_vregs;
+	bool vregs_on;
 	u32 init_speed;
 	u32 oper_speed;
 	const char *firmware_name;
@@ -1602,7 +1597,7 @@ static int qca_regulator_init(struct hci_uart *hu)
 	 * off the voltage regulator.
 	 */
 	qcadev = serdev_device_get_drvdata(hu->serdev);
-	if (!qcadev->bt_power->vregs_on) {
+	if (!qcadev->vregs_on) {
 		serdev_device_close(hu->serdev);
 		ret = qca_regulator_enable(qcadev);
 		if (ret)
@@ -1945,20 +1940,19 @@ static int qca_power_off(struct hci_dev *hdev)
 
 static int qca_regulator_enable(struct qca_serdev *qcadev)
 {
-	struct qca_power *power = qcadev->bt_power;
 	int ret;
 
 	/* Already enabled */
-	if (power->vregs_on)
+	if (qcadev->vregs_on)
 		return 0;
 
-	BT_DBG("enabling %d regulators)", power->num_vregs);
+	BT_DBG("enabling %d regulators)", qcadev->num_vregs);
 
-	ret = regulator_bulk_enable(power->num_vregs, power->vreg_bulk);
+	ret = regulator_bulk_enable(qcadev->num_vregs, qcadev->vreg_bulk);
 	if (ret)
 		return ret;
 
-	power->vregs_on = true;
+	qcadev->vregs_on = true;
 
 	ret = clk_prepare_enable(qcadev->susclk);
 	if (ret)
@@ -1969,38 +1963,37 @@ static int qca_regulator_enable(struct qca_serdev *qcadev)
 
 static void qca_regulator_disable(struct qca_serdev *qcadev)
 {
-	struct qca_power *power;
-
 	if (!qcadev)
 		return;
 
-	power = qcadev->bt_power;
-
 	/* Already disabled? */
-	if (!power->vregs_on)
+	if (!qcadev->vregs_on)
 		return;
 
-	regulator_bulk_disable(power->num_vregs, power->vreg_bulk);
-	power->vregs_on = false;
+	regulator_bulk_disable(qcadev->num_vregs, qcadev->vreg_bulk);
+	qcadev->vregs_on = false;
 
 	clk_disable_unprepare(qcadev->susclk);
 }
 
-static int qca_init_regulators(struct qca_power *qca,
-				const struct qca_vreg *vregs, size_t num_vregs)
+static int qca_init_regulators(struct device *dev, struct qca_serdev *qca,
+			       const struct qca_vreg *vregs, size_t num_vregs)
 {
 	struct regulator_bulk_data *bulk;
 	int ret;
 	int i;
 
-	bulk = devm_kcalloc(qca->dev, num_vregs, sizeof(*bulk), GFP_KERNEL);
+	if (!num_vregs)
+		return 0;
+
+	bulk = devm_kcalloc(dev, num_vregs, sizeof(*bulk), GFP_KERNEL);
 	if (!bulk)
 		return -ENOMEM;
 
 	for (i = 0; i < num_vregs; i++)
 		bulk[i].supply = vregs[i].name;
 
-	ret = devm_regulator_bulk_get(qca->dev, num_vregs, bulk);
+	ret = devm_regulator_bulk_get(dev, num_vregs, bulk);
 	if (ret < 0)
 		return ret;
 
@@ -2044,21 +2037,15 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	if ((qca_is_wcn399x(data->soc_type) ||
 	     qca_is_wcn6750(data->soc_type))) {
 		qcadev->btsoc_type = data->soc_type;
-		qcadev->bt_power = devm_kzalloc(&serdev->dev,
-						sizeof(struct qca_power),
-						GFP_KERNEL);
-		if (!qcadev->bt_power)
-			return -ENOMEM;
-
-		qcadev->bt_power->dev = &serdev->dev;
-		err = qca_init_regulators(qcadev->bt_power, data->vregs,
+
+		err = qca_init_regulators(&serdev->dev, qcadev, data->vregs,
 					  data->num_vregs);
 		if (err) {
 			BT_ERR("Failed to init regulators:%d", err);
 			return err;
 		}
 
-		qcadev->bt_power->vregs_on = false;
+		qcadev->vregs_on = false;
 
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
 					       GPIOD_OUT_LOW);
@@ -2139,11 +2126,10 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 static void qca_serdev_remove(struct serdev_device *serdev)
 {
 	struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
-	struct qca_power *power = qcadev->bt_power;
 
 	if ((qca_is_wcn399x(qcadev->btsoc_type) ||
 	     qca_is_wcn6750(qcadev->btsoc_type)) &&
-	     power->vregs_on)
+	     qcadev->vregs_on)
 		qca_power_shutdown(&qcadev->serdev_hu);
 	else if (qcadev->susclk)
 		clk_disable_unprepare(qcadev->susclk);
-- 
2.30.2

[PATCH v3 5/7] Bluetooth: hci_qca: merge wcn & non-wcn code paths

[Dmitry Baryshkov]

In preparation to add power sequencer support to qca6390, merge wcnxxxx
and non-wcn codepaths.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/bluetooth/hci_qca.c | 152 +++++++++++++++++-------------------
 1 file changed, 71 insertions(+), 81 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 9cc8a9153d76..bb04da08468a 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -84,6 +84,7 @@ enum qca_flags {
 enum qca_capabilities {
 	QCA_CAP_WIDEBAND_SPEECH = BIT(0),
 	QCA_CAP_VALID_LE_STATES = BIT(1),
+	QCA_CAP_NEEDS_BT_ENABLE = BIT(2),
 };
 
 /* HCI_IBS transmit side sleep protocol states */
@@ -203,6 +204,7 @@ struct qca_device_data {
 	struct qca_vreg *vregs;
 	size_t num_vregs;
 	uint32_t capabilities;
+	const char *name;
 };
 
 /*
@@ -220,6 +222,7 @@ struct qca_serdev {
 	u32 init_speed;
 	u32 oper_speed;
 	const char *firmware_name;
+	const char *name;
 };
 
 static int qca_regulator_enable(struct qca_serdev *qcadev);
@@ -254,6 +257,17 @@ static const char *qca_get_firmware_name(struct hci_uart *hu)
 	}
 }
 
+static const char *qca_soc_name(struct hci_uart *hu)
+{
+	if (hu->serdev) {
+		struct qca_serdev *qsd = serdev_device_get_drvdata(hu->serdev);
+
+		return qsd->name;
+	} else {
+		return "ROME";
+	}
+}
+
 static void __serial_clock_on(struct tty_struct *tty)
 {
 	/* TODO: Some chipset requires to enable UART clock on client
@@ -1623,14 +1637,16 @@ static int qca_regulator_init(struct hci_uart *hu)
 		gpiod_set_value_cansleep(qcadev->bt_en, 0);
 		msleep(50);
 		gpiod_set_value_cansleep(qcadev->bt_en, 1);
-		msleep(50);
+		msleep(150);
 		if (qcadev->sw_ctrl) {
 			sw_ctrl_state = gpiod_get_value_cansleep(qcadev->sw_ctrl);
 			bt_dev_dbg(hu->hdev, "SW_CTRL is %d", sw_ctrl_state);
 		}
 	}
 
-	qca_set_speed(hu, QCA_INIT_SPEED);
+	if (qca_is_wcn399x(soc_type) ||
+	    qca_is_wcn6750(soc_type))
+		qca_set_speed(hu, QCA_INIT_SPEED);
 
 	if (qca_is_wcn399x(soc_type)) {
 		ret = qca_send_power_pulse(hu, true);
@@ -1650,7 +1666,9 @@ static int qca_regulator_init(struct hci_uart *hu)
 		return ret;
 	}
 
-	hci_uart_set_flow_control(hu, false);
+	if (qca_is_wcn399x(soc_type) ||
+	    qca_is_wcn6750(soc_type))
+		hci_uart_set_flow_control(hu, false);
 
 	return 0;
 }
@@ -1658,8 +1676,6 @@ static int qca_regulator_init(struct hci_uart *hu)
 static int qca_power_on(struct hci_dev *hdev)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
-	enum qca_btsoc_type soc_type = qca_soc_type(hu);
-	struct qca_serdev *qcadev;
 	struct qca_data *qca = hu->priv;
 	int ret = 0;
 
@@ -1669,17 +1685,7 @@ static int qca_power_on(struct hci_dev *hdev)
 	if (!hu->serdev)
 		return 0;
 
-	if (qca_is_wcn399x(soc_type) ||
-	    qca_is_wcn6750(soc_type)) {
-		ret = qca_regulator_init(hu);
-	} else {
-		qcadev = serdev_device_get_drvdata(hu->serdev);
-		if (qcadev->bt_en) {
-			gpiod_set_value_cansleep(qcadev->bt_en, 1);
-			/* Controller needs time to bootup. */
-			msleep(150);
-		}
-	}
+	ret = qca_regulator_init(hu);
 
 	clear_bit(QCA_BT_OFF, &qca->flags);
 	return ret;
@@ -1709,9 +1715,7 @@ static int qca_setup(struct hci_uart *hu)
 	 */
 	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 
-	bt_dev_info(hdev, "setting up %s",
-		qca_is_wcn399x(soc_type) ? "wcn399x" :
-		(soc_type == QCA_WCN6750) ? "wcn6750" : "ROME/QCA6390");
+	bt_dev_info(hdev, "setting up %s", qca_soc_name(hu));
 
 	qca->memdump_state = QCA_MEMDUMP_IDLE;
 
@@ -1822,6 +1826,7 @@ static const struct qca_device_data qca_soc_data_wcn3990 = {
 		{ "vddch0", 450000 },
 	},
 	.num_vregs = 4,
+	.name = "wcn3990",
 };
 
 static const struct qca_device_data qca_soc_data_wcn3991 = {
@@ -1834,6 +1839,7 @@ static const struct qca_device_data qca_soc_data_wcn3991 = {
 	},
 	.num_vregs = 4,
 	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
+	.name = "wcn3991",
 };
 
 static const struct qca_device_data qca_soc_data_wcn3998 = {
@@ -1845,11 +1851,14 @@ static const struct qca_device_data qca_soc_data_wcn3998 = {
 		{ "vddch0", 450000 },
 	},
 	.num_vregs = 4,
+	.name = "wcn3998",
 };
 
 static const struct qca_device_data qca_soc_data_qca6390 = {
 	.soc_type = QCA_QCA6390,
 	.num_vregs = 0,
+	.capabilities = QCA_CAP_NEEDS_BT_ENABLE,
+	.name = "qca6390",
 };
 
 static const struct qca_device_data qca_soc_data_wcn6750 = {
@@ -1866,12 +1875,15 @@ static const struct qca_device_data qca_soc_data_wcn6750 = {
 		{ "vddasd", 200 },
 	},
 	.num_vregs = 9,
-	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
+	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES | QCA_CAP_NEEDS_BT_ENABLE,
+	.name = "wcn6750",
 };
 
 static const struct qca_device_data qca_soc_data_default = {
 	.soc_type = QCA_ROME,
 	.num_vregs = 0,
+	.capabilities = QCA_CAP_NEEDS_BT_ENABLE,
+	.name = "ROME",
 };
 
 static void qca_power_shutdown(struct hci_uart *hu)
@@ -1903,7 +1915,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
 		host_set_baudrate(hu, 2400);
 		qca_send_power_pulse(hu, false);
 		qca_regulator_disable(qcadev);
-	} else if (soc_type == QCA_WCN6750) {
+	} else if (qcadev->bt_en) {
 		gpiod_set_value_cansleep(qcadev->bt_en, 0);
 		msleep(100);
 		qca_regulator_disable(qcadev);
@@ -1911,8 +1923,6 @@ static void qca_power_shutdown(struct hci_uart *hu)
 			sw_ctrl_state = gpiod_get_value_cansleep(qcadev->sw_ctrl);
 			bt_dev_dbg(hu->hdev, "SW_CTRL is %d", sw_ctrl_state);
 		}
-	} else if (qcadev->bt_en) {
-		gpiod_set_value_cansleep(qcadev->bt_en, 0);
 	}
 
 	set_bit(QCA_BT_OFF, &qca->flags);
@@ -2034,71 +2044,51 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	if (!qcadev->oper_speed)
 		BT_DBG("UART will pick default operating speed");
 
-	if ((qca_is_wcn399x(data->soc_type) ||
-	     qca_is_wcn6750(data->soc_type))) {
-		qcadev->btsoc_type = data->soc_type;
-
-		err = qca_init_regulators(&serdev->dev, qcadev, data->vregs,
-					  data->num_vregs);
-		if (err) {
-			BT_ERR("Failed to init regulators:%d", err);
-			return err;
-		}
+	qcadev->name = data->name;
+	qcadev->btsoc_type = data->soc_type;
 
-		qcadev->vregs_on = false;
-
-		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
-					       GPIOD_OUT_LOW);
-		if (!qcadev->bt_en && data->soc_type == QCA_WCN6750) {
-			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
-			power_ctrl_enabled = false;
-		}
+	err = qca_init_regulators(&serdev->dev, qcadev, data->vregs,
+				  data->num_vregs);
+	if (err) {
+		BT_ERR("Failed to init regulators:%d", err);
+		return err;
+	}
 
-		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
-					       GPIOD_IN);
-		if (!qcadev->sw_ctrl && data->soc_type == QCA_WCN6750)
-			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+	qcadev->vregs_on = false;
 
-		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
-		if (IS_ERR(qcadev->susclk)) {
-			dev_err(&serdev->dev, "failed to acquire clk\n");
-			return PTR_ERR(qcadev->susclk);
-		}
+	qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
+						GPIOD_OUT_LOW);
+	if (!qcadev->bt_en && (data->capabilities & QCA_CAP_NEEDS_BT_ENABLE)) {
+		dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
+		power_ctrl_enabled = false;
+	}
 
-		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
-		if (err) {
-			BT_ERR("wcn3990 serdev registration failed");
-			return err;
-		}
-	} else {
-		qcadev->btsoc_type = data->soc_type;
+	qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
+						  GPIOD_IN);
+	if (!qcadev->sw_ctrl && data->soc_type == QCA_WCN6750)
+		dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
 
-		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
-					       GPIOD_OUT_LOW);
-		if (!qcadev->bt_en) {
-			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-			power_ctrl_enabled = false;
-		}
+	qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
+	if (IS_ERR(qcadev->susclk)) {
+		dev_err(&serdev->dev, "failed to acquire clk\n");
+		return PTR_ERR(qcadev->susclk);
+	}
 
-		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
-		if (IS_ERR(qcadev->susclk)) {
-			dev_warn(&serdev->dev, "failed to acquire clk\n");
-			return PTR_ERR(qcadev->susclk);
-		}
-		err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
-		if (err)
-			return err;
+	err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
+	if (err)
+		return err;
 
+	if (!qca_is_wcn399x(qcadev->btsoc_type) &&
+	    !qca_is_wcn6750(qcadev->btsoc_type)) {
 		err = clk_prepare_enable(qcadev->susclk);
 		if (err)
 			return err;
+	}
 
-		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
-		if (err) {
-			BT_ERR("Rome serdev registration failed");
-			clk_disable_unprepare(qcadev->susclk);
-			return err;
-		}
+	err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+	if (err) {
+		BT_ERR("%s serdev registration failed", qcadev->name);
+		return err;
 	}
 
 	hdev = qcadev->serdev_hu.hdev;
@@ -2127,11 +2117,11 @@ static void qca_serdev_remove(struct serdev_device *serdev)
 {
 	struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
 
-	if ((qca_is_wcn399x(qcadev->btsoc_type) ||
-	     qca_is_wcn6750(qcadev->btsoc_type)) &&
-	     qcadev->vregs_on)
+	if (qcadev->vregs_on)
 		qca_power_shutdown(&qcadev->serdev_hu);
-	else if (qcadev->susclk)
+
+	if (!qca_is_wcn399x(qcadev->btsoc_type) &&
+	    !qca_is_wcn6750(qcadev->btsoc_type))
 		clk_disable_unprepare(qcadev->susclk);
 
 	hci_uart_unregister_device(&qcadev->serdev_hu);
-- 
2.30.2

[PATCH v3 6/7] Bluetooth: hci_qca: add power sequencer support to qca6390

[Dmitry Baryshkov]

QCA6390 uses on-chip power sequencer (and power management unit) to
supply power to both WiFi and BT parts. Since this sequencer is
supported by a separate driver, use it as a single "vin" regulator.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/bluetooth/hci_qca.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index bb04da08468a..deea38033248 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1856,7 +1856,10 @@ static const struct qca_device_data qca_soc_data_wcn3998 = {
 
 static const struct qca_device_data qca_soc_data_qca6390 = {
 	.soc_type = QCA_QCA6390,
-	.num_vregs = 0,
+	.vregs = (struct qca_vreg []) {
+		{ "vin", 1000 },
+	},
+	.num_vregs = 1,
 	.capabilities = QCA_CAP_NEEDS_BT_ENABLE,
 	.name = "qca6390",
 };
-- 
2.30.2

[PATCH v3 7/7] arm64: dts: qcom: qrb5165-rb5: add QCA6391 WiFi+BT SoC

[Dmitry Baryshkov]

Describe the onboard WiFi+BT SoC QCA6391. It is connected to uart6 to
provide Bluetooth functionality (QCA) and to PCIe to provide WiFi
(ath11k). A separate power sequencer device node is used to describe
on-SoC power management unit common to both WiFi and BT parts.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 56 ++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
index 5f41de20aa22..f968208b1947 100644
--- a/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
+++ b/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts
@@ -20,6 +20,7 @@ / {
 
 	aliases {
 		serial0 = &uart12;
+		serial1 = &uart6;
 		sdhc2 = &sdhc_2;
 	};
 
@@ -223,6 +224,28 @@ vreg_s4a_1p8: vreg-s4a-1p8 {
 		regulator-max-microvolt = <1800000>;
 		regulator-always-on;
 	};
+
+	qca6391: qca6391 {
+		compatible = "qcom,qca6390";
+
+		vddaon-supply = <&vreg_s6a_0p95>;
+		vddpmu-supply = <&vreg_s2f_0p95>;
+		vddrfa1-supply = <&vreg_s2f_0p95>;
+		vddrfa2-supply = <&vreg_s8c_1p3>;
+		vddrfa3-supply = <&vreg_s5a_1p9>;
+		vddpcie1-supply = <&vreg_s8c_1p3>;
+		vddpcie2-supply = <&vreg_s5a_1p9>;
+		vddio-supply = <&vreg_s4a_1p8>;
+	};
+
+	vreg_wlan: vreg-wlan {
+		compatible = "regulator-fixed";
+		regulator-name = "vreg-wlan";
+
+		vin-supply = <&qca6391>;
+		gpio = <&tlmm 20 GPIO_ACTIVE_LOW>;
+		enable-active-high;
+	};
 };
 
 &adsp {
@@ -668,6 +691,8 @@ &pcie0 {
 	wake-gpio = <&tlmm 81 GPIO_ACTIVE_HIGH>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie0_default_state>;
+
+	vddpe-3v3-supply = <&vreg_wlan>;
 };
 
 &pcie0_phy {
@@ -811,6 +836,26 @@ lt9611_rst_pin: lt9611-rst-pin {
 	};
 };
 
+&qup_uart6_default {
+	ctsrx {
+		pins = "gpio16", "gpio19";
+		drive-strength = <2>;
+		bias-disable;
+	};
+
+	rts {
+		pins = "gpio17";
+		drive-strength = <2>;
+		bias-disable;
+	};
+
+	tx {
+		pins = "gpio18";
+		drive-strength = <2>;
+		bias-pull-up;
+	};
+};
+
 &qupv3_id_0 {
 	status = "okay";
 };
@@ -1275,6 +1320,17 @@ sdc2_card_det_n: sd-card-det-n {
 	};
 };
 
+&uart6 {
+	status = "okay";
+	bluetooth {
+		compatible = "qcom,qca6390-bt";
+		vin-supply = <&qca6391>;
+		clocks = <&sleep_clk>;
+
+		enable-gpios = <&tlmm 21 GPIO_ACTIVE_HIGH>;
+	};
+};
+
 &uart12 {
 	status = "okay";
 };
-- 
2.30.2

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

On Tue, Jun 22, 2021 at 01:31:36AM +0300, Dmitry Baryshkov wrote:

> Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> being controlled through the UART and WiFi being present on PCIe
> bus. Both blocks share common power sources. Add device driver handling
> power sequencing of QCA6390/1.

Are you sure this is a regulator and not a MFD?  It appears to be a
consumer driver that turns on and off a bunch of regulators en masse
which for some reason exposes that on/off control as a single supply.
This looks like it'd be much more appropriate to implement as a MFD or
possibly power domain with the subdevices using runtime PM, it's clearly
not a regulator.

> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + */

Please make the entire comment a C++ one so things look more
intentional.

> +static int qca6390_enable(struct regulator_dev *rdev)
> +{
> +	struct qca6390_data *data = rdev_get_drvdata(rdev);
> +	int ret;
> +
> +	ret = regulator_bulk_enable(data->num_vregs, data->regulators);
> +	if (ret) {
> +		dev_err(data->dev, "Failed to enable regulators");
> +		return ret;
> +	}

The regulator API is *not* recursive, I am astonished this works.

> +	/* Wait for 1ms before toggling enable pins. */
> +	usleep_range(1000, 2000);

There's core support for delays after power on, better to use it.

> +	data->enable_counter++;

You shouldn't assume that enable and disable calls are matched.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Dmitry Baryshkov]

On Tue, 22 Jun 2021 at 14:29, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jun 22, 2021 at 01:31:36AM +0300, Dmitry Baryshkov wrote:
>
> > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > being controlled through the UART and WiFi being present on PCIe
> > bus. Both blocks share common power sources. Add device driver handling
> > power sequencing of QCA6390/1.
>
> Are you sure this is a regulator and not a MFD?  It appears to be a
> consumer driver that turns on and off a bunch of regulators en masse
> which for some reason exposes that on/off control as a single supply.
> This looks like it'd be much more appropriate to implement as a MFD or
> possibly power domain with the subdevices using runtime PM, it's clearly
> not a regulator.

First attempt was designed to be an MFD. And Lee clearly stated that
this is wrong:
"This is not an MFD, since it utilised neither the MFD API nor
of_platform_populate() to register child devices." [1]

I've attempted implementing that as a genpd (in previous iterations),
but it results in worse design. PCIe controllers are not expected to
handle power domains for EP devices, especially in cases when the PD
must come up before the controller does link training and bus probe.
I've tried following Rob's suggestions on implementing things clearly,
but doing so results in too big restructure just for a single device.

> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2021, Linaro Limited
> > + */
>
> Please make the entire comment a C++ one so things look more
> intentional.

Ack.

>
> > +static int qca6390_enable(struct regulator_dev *rdev)
> > +{
> > +     struct qca6390_data *data = rdev_get_drvdata(rdev);
> > +     int ret;
> > +
> > +     ret = regulator_bulk_enable(data->num_vregs, data->regulators);
> > +     if (ret) {
> > +             dev_err(data->dev, "Failed to enable regulators");
> > +             return ret;
> > +     }
>
> The regulator API is *not* recursive, I am astonished this works.

It does, even with lockdep enabled. Moreover BT regularly does disable
and enable this regulator, so both enable and disable paths were well
tested.
Should I change this into some internal call to remove API recursiveness?

>
> > +     /* Wait for 1ms before toggling enable pins. */
> > +     usleep_range(1000, 2000);
>
> There's core support for delays after power on, better to use it.

Ack.

>
> > +     data->enable_counter++;
>
> You shouldn't assume that enable and disable calls are matched.

Ack.

--
With best wishes
Dmitry

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2549 bytes --]

On Tue, Jun 22, 2021 at 05:17:28PM +0300, Dmitry Baryshkov wrote:
> On Tue, 22 Jun 2021 at 14:29, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Jun 22, 2021 at 01:31:36AM +0300, Dmitry Baryshkov wrote:

> > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > being controlled through the UART and WiFi being present on PCIe
> > > bus. Both blocks share common power sources. Add device driver handling
> > > power sequencing of QCA6390/1.

> > Are you sure this is a regulator and not a MFD?  It appears to be a
> > consumer driver that turns on and off a bunch of regulators en masse
> > which for some reason exposes that on/off control as a single supply.
> > This looks like it'd be much more appropriate to implement as a MFD or
> > possibly power domain with the subdevices using runtime PM, it's clearly
> > not a regulator.

> First attempt was designed to be an MFD. And Lee clearly stated that
> this is wrong:
> "This is not an MFD, since it utilised neither the MFD API nor
> of_platform_populate() to register child devices." [1]

Well, perhaps it should do one of those things then?  Like I say this is
very clearly not a regulator, it looks like a consumer of some kind.
The regulator API isn't there just to absorb things that need reference
counting, it's there to represent things providing supplies.  This seems
to be very clearly not a supply given that it's grouping together a
bunch of other supplies and switching them on and off together without
providing a clear output supply.

> I've tried following Rob's suggestions on implementing things clearly,
> but doing so results in too big restructure just for a single device.

I don't know what that suggestion was?  If there's only one device that
uses this why is it not implemented as part of that device?

> > > +static int qca6390_enable(struct regulator_dev *rdev)
> > > +{
> > > +     struct qca6390_data *data = rdev_get_drvdata(rdev);
> > > +     int ret;

> > > +     ret = regulator_bulk_enable(data->num_vregs, data->regulators);
> > > +     if (ret) {
> > > +             dev_err(data->dev, "Failed to enable regulators");
> > > +             return ret;
> > > +     }

> > The regulator API is *not* recursive, I am astonished this works.

> It does, even with lockdep enabled. Moreover BT regularly does disable
> and enable this regulator, so both enable and disable paths were well
> tested.
> Should I change this into some internal call to remove API recursiveness?

You should not be implementing this as a regulator at all.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Dmitry Baryshkov]

On Tue, 22 Jun 2021 at 17:38, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jun 22, 2021 at 05:17:28PM +0300, Dmitry Baryshkov wrote:
> > On Tue, 22 Jun 2021 at 14:29, Mark Brown <broonie@kernel.org> wrote:
> > > On Tue, Jun 22, 2021 at 01:31:36AM +0300, Dmitry Baryshkov wrote:
>
> > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > being controlled through the UART and WiFi being present on PCIe
> > > > bus. Both blocks share common power sources. Add device driver handling
> > > > power sequencing of QCA6390/1.
>
> > > Are you sure this is a regulator and not a MFD?  It appears to be a
> > > consumer driver that turns on and off a bunch of regulators en masse
> > > which for some reason exposes that on/off control as a single supply.
> > > This looks like it'd be much more appropriate to implement as a MFD or
> > > possibly power domain with the subdevices using runtime PM, it's clearly
> > > not a regulator.
>
> > First attempt was designed to be an MFD. And Lee clearly stated that
> > this is wrong:
> > "This is not an MFD, since it utilised neither the MFD API nor
> > of_platform_populate() to register child devices." [1]
>
> Well, perhaps it should do one of those things then?

I don't think so. BT part is just a serdev sitting on top of UART,
WiFi is PCIe device (for qca6390). So using MFD API (which primarily
targets platform devices) does not seem logical and feasible.

> Like I say this is
> very clearly not a regulator, it looks like a consumer of some kind.
> The regulator API isn't there just to absorb things that need reference
> counting, it's there to represent things providing supplies.  This seems
> to be very clearly not a supply given that it's grouping together a
> bunch of other supplies and switching them on and off together without
> providing a clear output supply.

Ack.

> > I've tried following Rob's suggestions on implementing things clearly,
> > but doing so results in too big restructure just for a single device.
>
> I don't know what that suggestion was?  If there's only one device that
> uses this why is it not implemented as part of that device?

One device = qca6390 (or 91). Because it is still a device sitting on
a PCI bus which is typically discoverable, while adding power
sequencer to the device driver would demand making the bus fully
descibiable (like PCI bus is on Spark).

> > > > +static int qca6390_enable(struct regulator_dev *rdev)
> > > > +{
> > > > +     struct qca6390_data *data = rdev_get_drvdata(rdev);
> > > > +     int ret;
>
> > > > +     ret = regulator_bulk_enable(data->num_vregs, data->regulators);
> > > > +     if (ret) {
> > > > +             dev_err(data->dev, "Failed to enable regulators");
> > > > +             return ret;
> > > > +     }
>
> > > The regulator API is *not* recursive, I am astonished this works.
>
> > It does, even with lockdep enabled. Moreover BT regularly does disable
> > and enable this regulator, so both enable and disable paths were well
> > tested.
> > Should I change this into some internal call to remove API recursiveness?
>
> You should not be implementing this as a regulator at all.

Ack

-- 
With best wishes
Dmitry

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

On Tue, Jun 22, 2021 at 07:46:08PM +0300, Dmitry Baryshkov wrote:
> On Tue, 22 Jun 2021 at 17:38, Mark Brown <broonie@kernel.org> wrote:

> > Well, perhaps it should do one of those things then?

> I don't think so. BT part is just a serdev sitting on top of UART,
> WiFi is PCIe device (for qca6390). So using MFD API (which primarily
> targets platform devices) does not seem logical and feasible.

That really does sound like a MFD - AIUI it's a single chip with
multiple interfaces that needs some glue logic to hold it together.  It
doesn't fit well with the current framework that MFD offers but it's
definitely the same sort of one chip in multiple Linux frameworks sort
of thing.  The only other thing I can think might fit is handling it
like a plug in module for a development board (eg, RPi hats) but we've
not been doing so great at getting them supported upstream.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Ulf Hansson]

+ Peter

On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> being controlled through the UART and WiFi being present on PCIe
> bus. Both blocks share common power sources. Add device driver handling
> power sequencing of QCA6390/1.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Power sequencing of discoverable buses have been discussed several
times before at LKML. The last attempt [1] I am aware of, was in 2017
from Peter Chen. I don't think there is a common solution, yet.

Note that, this isn't specific to the PCIe bus, but rather to all
buses that may have discoverable devices attached and which need some
kind of pre-power-sequence before they can be discovered/probed.  USB,
PCIe, SDIO, etc.

Long time ago, we fixed the problem for SDIO (that also can have WiFi,
UART, bluetooth chips attached), but unfortunately through an MMC
subsystem specific implementation, that can't be re-used in a generic
way.

In any case, I have looped in Peter Chen, maybe he can provide us with
a better update on how things have moved forward, if at all.

Kind regards
Uffe

[1]
https://www.spinics.net/lists/linux-usb/msg158451.html

> ---
>  drivers/regulator/Kconfig        |  13 +++
>  drivers/regulator/Makefile       |   1 +
>  drivers/regulator/qcom-qca639x.c | 157 +++++++++++++++++++++++++++++++
>  3 files changed, 171 insertions(+)
>  create mode 100644 drivers/regulator/qcom-qca639x.c
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 3e7a38525cb3..7a560cddea7a 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -909,6 +909,19 @@ config REGULATOR_PWM
>           This driver supports PWM controlled voltage regulators. PWM
>           duty cycle can increase or decrease the voltage.
>
> +config REGULATOR_QCOM_QCA639X
> +       tristate "Qualcomm QCA639x WiFi/Bluetooth module support"
> +       help
> +         If you say yes to this option, support will be included for Qualcomm
> +         QCA639x family of WiFi and Bluetooth SoCs. Note, this driver supports
> +         only power control for this SoC, you still have to enable individual
> +         Bluetooth and WiFi drivers. This driver is only necessary on ARM
> +         platforms with this chip. PCIe cards handle power sequencing on their
> +         own.
> +
> +         Say M here if you want to include support for QCA639x chips as a
> +         module. This will build a module called "qcom-qca639x".
> +
>  config REGULATOR_QCOM_RPM
>         tristate "Qualcomm RPM regulator driver"
>         depends on MFD_QCOM_RPM
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 580b015296ea..129c2110b78d 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_REGULATOR_MT6380)        += mt6380-regulator.o
>  obj-$(CONFIG_REGULATOR_MT6397) += mt6397-regulator.o
>  obj-$(CONFIG_REGULATOR_MTK_DVFSRC) += mtk-dvfsrc-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_LABIBB) += qcom-labibb-regulator.o
> +obj-$(CONFIG_REGULATOR_QCOM_QCA639X) += qcom-qca639x.o
>  obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_RPMH) += qcom-rpmh-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
> diff --git a/drivers/regulator/qcom-qca639x.c b/drivers/regulator/qcom-qca639x.c
> new file mode 100644
> index 000000000000..a2c78c0f8baa
> --- /dev/null
> +++ b/drivers/regulator/qcom-qca639x.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + */
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/slab.h>
> +
> +#define MAX_NUM_REGULATORS     8
> +
> +static struct vreg {
> +       const char *name;
> +       unsigned int load_uA;
> +} vregs[MAX_NUM_REGULATORS] = {
> +       /* 2.0 V */
> +       { "vddpcie2", 15000 },
> +       { "vddrfa3", 400000 },
> +
> +       /* 0.95 V */
> +       { "vddaon", 100000 },
> +       { "vddpmu", 1250000 },
> +       { "vddrfa1", 200000 },
> +
> +       /* 1.35 V */
> +       { "vddrfa2", 400000 },
> +       { "vddpcie1", 35000 },
> +
> +       /* 1.8 V */
> +       { "vddio", 20000 },
> +};
> +
> +struct qca6390_data {
> +       struct device *dev;
> +       struct regulator_bulk_data regulators[MAX_NUM_REGULATORS];
> +       size_t num_vregs;
> +
> +       struct regulator_desc desc;
> +       struct regulator_dev *regulator_dev;
> +       unsigned int enable_counter;
> +};
> +
> +#define domain_to_data(domain) container_of(domain, struct qca6390_data, pd)
> +
> +static int qca6390_enable(struct regulator_dev *rdev)
> +{
> +       struct qca6390_data *data = rdev_get_drvdata(rdev);
> +       int ret;
> +
> +       ret = regulator_bulk_enable(data->num_vregs, data->regulators);
> +       if (ret) {
> +               dev_err(data->dev, "Failed to enable regulators");
> +               return ret;
> +       }
> +
> +       /* Wait for 1ms before toggling enable pins. */
> +       usleep_range(1000, 2000);
> +
> +       data->enable_counter++;
> +
> +       return 0;
> +}
> +
> +static int qca6390_disable(struct regulator_dev *rdev)
> +{
> +       struct qca6390_data *data = rdev_get_drvdata(rdev);
> +
> +       regulator_bulk_disable(data->num_vregs, data->regulators);
> +
> +       data->enable_counter--;
> +
> +       return 0;
> +}
> +
> +static int qca6390_is_enabled(struct regulator_dev *rdev)
> +{
> +       struct qca6390_data *data = rdev_get_drvdata(rdev);
> +
> +       return data->enable_counter > 0;
> +}
> +
> +static const struct regulator_ops qca6390_ops = {
> +       .enable = qca6390_enable,
> +       .disable = qca6390_disable,
> +       .is_enabled = qca6390_is_enabled,
> +};
> +
> +static int qca6390_probe(struct platform_device *pdev)
> +{
> +       struct qca6390_data *data;
> +       struct device *dev = &pdev->dev;
> +       struct regulator_config cfg = { };
> +       int i, ret;
> +
> +       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->dev = dev;
> +       data->num_vregs = ARRAY_SIZE(vregs);
> +
> +       for (i = 0; i < data->num_vregs; i++)
> +               data->regulators[i].supply = vregs[i].name;
> +
> +       ret = devm_regulator_bulk_get(dev, data->num_vregs, data->regulators);
> +       if (ret < 0)
> +               return ret;
> +
> +       for (i = 0; i < data->num_vregs; i++) {
> +               ret = regulator_set_load(data->regulators[i].consumer, vregs[i].load_uA);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       data->desc.name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
> +       if (!data->desc.name)
> +               return -ENOMEM;
> +
> +       data->desc.type = REGULATOR_VOLTAGE;
> +       data->desc.owner = THIS_MODULE;
> +       data->desc.ops = &qca6390_ops;
> +
> +       cfg.dev = dev;
> +       cfg.of_node = dev->of_node;
> +       cfg.driver_data = data;
> +       cfg.init_data = of_get_regulator_init_data(dev, dev->of_node, &data->desc);
> +
> +       data->regulator_dev = devm_regulator_register(dev, &data->desc, &cfg);
> +       if (IS_ERR(data->regulator_dev)) {
> +               ret = PTR_ERR(data->regulator_dev);
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, data);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id qca6390_of_match[] = {
> +       { .compatible = "qcom,qca6390" },
> +};
> +
> +static struct platform_driver qca6390_driver = {
> +       .probe = qca6390_probe,
> +       .driver = {
> +               .name = "qca6390",
> +               .of_match_table = qca6390_of_match,
> +       },
> +};
> +
> +module_platform_driver(qca6390_driver);
> +MODULE_AUTHOR("Dmitry Baryshkov <dmitry.baryshkov@linaro.org>");
> +MODULE_DESCRIPTION("Power control for Qualcomm QCA6390/1 BT/WiFi chip");
> +MODULE_LICENSE("GPL v2");
> --
> 2.30.2
>

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov

> > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > being controlled through the UART and WiFi being present on PCIe
> > bus. Both blocks share common power sources. Add device driver handling
> > power sequencing of QCA6390/1.

> Power sequencing of discoverable buses have been discussed several
> times before at LKML. The last attempt [1] I am aware of, was in 2017
> from Peter Chen. I don't think there is a common solution, yet.

This feels a bit different to the power sequencing problem - it's not
exposing the individual inputs to the device but rather is a block that
manages everything but needs a bit of a kick to get things going (I'd
guess that with ACPI it'd be triggered via AML).  It's in the same space
but it's not quite the same issue I think, something that can handle
control of the individual resources might still struggle with this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Ulf Hansson]

- Peter (the email was bouncing)

On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
>
> > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > being controlled through the UART and WiFi being present on PCIe
> > > bus. Both blocks share common power sources. Add device driver handling
> > > power sequencing of QCA6390/1.
>
> > Power sequencing of discoverable buses have been discussed several
> > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > from Peter Chen. I don't think there is a common solution, yet.
>
> This feels a bit different to the power sequencing problem - it's not
> exposing the individual inputs to the device but rather is a block that
> manages everything but needs a bit of a kick to get things going (I'd
> guess that with ACPI it'd be triggered via AML).  It's in the same space
> but it's not quite the same issue I think, something that can handle
> control of the individual resources might still struggle with this.

Well, to me it looks very similar to those resouses we could manage
with the mmc pwrseq, for SDIO. It's also typically the same kind of
combo-chips that moved from supporting SDIO to PCIe, for improved
performance I guess. More importantly, the same constraint to
pre-power on the device is needed to allow it to be discovered/probed.

Therefore, I think it would be worth having a common solution for
this, rather than a solution per subsystem or even worse, per device.

Unfortunately, it looks like Peter's email is bouncing so we can't get
an update from him.

Kind regards
Uffe

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Dmitry Baryshkov]

Hi,

On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> - Peter (the email was bouncing)

+ Peter's kernel.org address

>
> On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> >
> > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> >
> > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > being controlled through the UART and WiFi being present on PCIe
> > > > bus. Both blocks share common power sources. Add device driver handling
> > > > power sequencing of QCA6390/1.
> >
> > > Power sequencing of discoverable buses have been discussed several
> > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > from Peter Chen. I don't think there is a common solution, yet.
> >
> > This feels a bit different to the power sequencing problem - it's not
> > exposing the individual inputs to the device but rather is a block that
> > manages everything but needs a bit of a kick to get things going (I'd
> > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > but it's not quite the same issue I think, something that can handle
> > control of the individual resources might still struggle with this.
>
> Well, to me it looks very similar to those resouses we could manage
> with the mmc pwrseq, for SDIO. It's also typically the same kind of
> combo-chips that moved from supporting SDIO to PCIe, for improved
> performance I guess. More importantly, the same constraint to
> pre-power on the device is needed to allow it to be discovered/probed.

In our case we'd definitely use pwrseq for PCIe bus and we can also
benefit from using pwrseq for serdev and for platform busses also (for
the same story of WiFi+BT chips).

I can take a look at rewriting pwrseq code to also handle the PCIe
bus. Rewriting it to be a generic lib seems like an easy task,
plugging it into PCIe code would be more fun.

Platform and serdev... Definitely even more fun.

> Therefore, I think it would be worth having a common solution for
> this, rather than a solution per subsystem or even worse, per device.
>
> Unfortunately, it looks like Peter's email is bouncing so we can't get
> an update from him.

Let's see if the kernel.org email will get to him.

-- 
With best wishes
Dmitry

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Rob Herring]

On Thu, Jul 08, 2021 at 02:37:44PM +0300, Dmitry Baryshkov wrote:
> Hi,
> 
> On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > - Peter (the email was bouncing)
> 
> + Peter's kernel.org address
> 
> >
> > On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> > >
> > > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> > >
> > > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > > being controlled through the UART and WiFi being present on PCIe
> > > > > bus. Both blocks share common power sources. Add device driver handling
> > > > > power sequencing of QCA6390/1.
> > >
> > > > Power sequencing of discoverable buses have been discussed several
> > > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > > from Peter Chen. I don't think there is a common solution, yet.
> > >
> > > This feels a bit different to the power sequencing problem - it's not
> > > exposing the individual inputs to the device but rather is a block that
> > > manages everything but needs a bit of a kick to get things going (I'd
> > > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > > but it's not quite the same issue I think, something that can handle
> > > control of the individual resources might still struggle with this.
> >
> > Well, to me it looks very similar to those resouses we could manage
> > with the mmc pwrseq, for SDIO. It's also typically the same kind of
> > combo-chips that moved from supporting SDIO to PCIe, for improved
> > performance I guess. More importantly, the same constraint to
> > pre-power on the device is needed to allow it to be discovered/probed.
> 
> In our case we'd definitely use pwrseq for PCIe bus and we can also
> benefit from using pwrseq for serdev and for platform busses also (for
> the same story of WiFi+BT chips).
> 
> I can take a look at rewriting pwrseq code to also handle the PCIe
> bus. Rewriting it to be a generic lib seems like an easy task,
> plugging it into PCIe code would be more fun.
> 
> Platform and serdev... Definitely even more fun.

I don't want to see pwrseq (the binding) expanded to other buses. If 
that was the answer, we wouldn't be having this discussion. It was a 
mistake for MMC IMO. 

If pwrseq works as a kernel library/api, then I have no issue with that.

> 
> > Therefore, I think it would be worth having a common solution for
> > this, rather than a solution per subsystem or even worse, per device.

Power sequencing requirements are inheritently per device unless we're 
talking about standard connectors. 

This is a solved problem on MDIO. It's quite simple. If there's a DT 
node for a device you haven't discovered, then probe it anyways.

Rob

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Bjorn Andersson]

On Wed 14 Jul 11:47 CDT 2021, Rob Herring wrote:

> On Thu, Jul 08, 2021 at 02:37:44PM +0300, Dmitry Baryshkov wrote:
> > Hi,
> > 
> > On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > - Peter (the email was bouncing)
> > 
> > + Peter's kernel.org address
> > 
> > >
> > > On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> > > >
> > > > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> > > >
> > > > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > > > being controlled through the UART and WiFi being present on PCIe
> > > > > > bus. Both blocks share common power sources. Add device driver handling
> > > > > > power sequencing of QCA6390/1.
> > > >
> > > > > Power sequencing of discoverable buses have been discussed several
> > > > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > > > from Peter Chen. I don't think there is a common solution, yet.
> > > >
> > > > This feels a bit different to the power sequencing problem - it's not
> > > > exposing the individual inputs to the device but rather is a block that
> > > > manages everything but needs a bit of a kick to get things going (I'd
> > > > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > > > but it's not quite the same issue I think, something that can handle
> > > > control of the individual resources might still struggle with this.
> > >
> > > Well, to me it looks very similar to those resouses we could manage
> > > with the mmc pwrseq, for SDIO. It's also typically the same kind of
> > > combo-chips that moved from supporting SDIO to PCIe, for improved
> > > performance I guess. More importantly, the same constraint to
> > > pre-power on the device is needed to allow it to be discovered/probed.
> > 
> > In our case we'd definitely use pwrseq for PCIe bus and we can also
> > benefit from using pwrseq for serdev and for platform busses also (for
> > the same story of WiFi+BT chips).
> > 
> > I can take a look at rewriting pwrseq code to also handle the PCIe
> > bus. Rewriting it to be a generic lib seems like an easy task,
> > plugging it into PCIe code would be more fun.
> > 
> > Platform and serdev... Definitely even more fun.
> 
> I don't want to see pwrseq (the binding) expanded to other buses. If 
> that was the answer, we wouldn't be having this discussion. It was a 
> mistake for MMC IMO. 
> 

But what do you want to see?

We have a single piece of hardware that needs a specific power sequence,
which is interacted with using both UART and PCIe.

> If pwrseq works as a kernel library/api, then I have no issue with that.
> 
> > 
> > > Therefore, I think it would be worth having a common solution for
> > > this, rather than a solution per subsystem or even worse, per device.
> 
> Power sequencing requirements are inheritently per device unless we're 
> talking about standard connectors. 
> 

Do you mean "device" as in the IC or device as in struct device? Because
we do have one physical IC, that has a need for a specific power on
sequence, but we have two struct device, on two different busses in
Linux interacting with this thing.

> This is a solved problem on MDIO. It's quite simple. If there's a DT 
> node for a device you haven't discovered, then probe it anyways.
> 

Okay, so DT tells us that there's actually a WiFi thing on the PCIe bus,
even though we can't find it, so we probe something...then what?

Regards,
Bjorn

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Bjorn Andersson]

On Thu 08 Jul 05:09 CDT 2021, Ulf Hansson wrote:

> - Peter (the email was bouncing)
> 
> On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> >
> > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> >
> > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > being controlled through the UART and WiFi being present on PCIe
> > > > bus. Both blocks share common power sources. Add device driver handling
> > > > power sequencing of QCA6390/1.
> >
> > > Power sequencing of discoverable buses have been discussed several
> > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > from Peter Chen. I don't think there is a common solution, yet.
> >
> > This feels a bit different to the power sequencing problem - it's not
> > exposing the individual inputs to the device but rather is a block that
> > manages everything but needs a bit of a kick to get things going (I'd
> > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > but it's not quite the same issue I think, something that can handle
> > control of the individual resources might still struggle with this.
> 
> Well, to me it looks very similar to those resouses we could manage
> with the mmc pwrseq, for SDIO. It's also typically the same kind of
> combo-chips that moved from supporting SDIO to PCIe, for improved
> performance I guess. More importantly, the same constraint to
> pre-power on the device is needed to allow it to be discovered/probed.
> 
> Therefore, I think it would be worth having a common solution for
> this, rather than a solution per subsystem or even worse, per device.
> 

Representing the chip and its power needs, separate from the busses does
seem reasonable. It's pretty much what Dmitry suggested originally, but
his attempts to use either power-domain or regulator references to
ensure ordering has been objected.


Beyond this, there is a similar case (that you and I have talked about
earlier) in supporting the SDX55 PCIe modem found in some devices.
Where in addition to ensuring that the power rails are configured, a
couple of gpios needs to be controlled and there's an incoming gpio line
indicating that the firmware of the device has locked up and the power
needs to be toggled and the device re-enumerated.

> Unfortunately, it looks like Peter's email is bouncing so we can't get
> an update from him.
> 

And for this second part, where we need some additional logic it seems
to go beyond what the power sequence discussions has touched upon so
far.

Regards,
Bjorn

Re: [PATCH v3 4/7] Bluetooth: hci_qca: merge qca_power into qca_serdev

[Bjorn Andersson]

On Mon 21 Jun 17:31 CDT 2021, Dmitry Baryshkov wrote:

> There is no need to allocate separate structure for handling regulators
> used by QCA chips, we gain nothing from it. Move all used data fields
> directly to struct qca_serdev.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/bluetooth/hci_qca.c | 58 ++++++++++++++-----------------------
>  1 file changed, 22 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 3704dbadba1d..9cc8a9153d76 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -208,20 +208,15 @@ struct qca_device_data {
>  /*
>   * Platform data for the QCA Bluetooth power driver.
>   */
> -struct qca_power {
> -	struct device *dev;
> -	struct regulator_bulk_data *vreg_bulk;
> -	int num_vregs;
> -	bool vregs_on;
> -};
> -
>  struct qca_serdev {
>  	struct hci_uart	 serdev_hu;
>  	struct gpio_desc *bt_en;
>  	struct gpio_desc *sw_ctrl;
>  	struct clk	 *susclk;
>  	enum qca_btsoc_type btsoc_type;
> -	struct qca_power *bt_power;
> +	struct regulator_bulk_data *vreg_bulk;
> +	int num_vregs;
> +	bool vregs_on;
>  	u32 init_speed;
>  	u32 oper_speed;
>  	const char *firmware_name;
> @@ -1602,7 +1597,7 @@ static int qca_regulator_init(struct hci_uart *hu)
>  	 * off the voltage regulator.
>  	 */
>  	qcadev = serdev_device_get_drvdata(hu->serdev);
> -	if (!qcadev->bt_power->vregs_on) {
> +	if (!qcadev->vregs_on) {
>  		serdev_device_close(hu->serdev);
>  		ret = qca_regulator_enable(qcadev);
>  		if (ret)
> @@ -1945,20 +1940,19 @@ static int qca_power_off(struct hci_dev *hdev)
>  
>  static int qca_regulator_enable(struct qca_serdev *qcadev)
>  {
> -	struct qca_power *power = qcadev->bt_power;
>  	int ret;
>  
>  	/* Already enabled */
> -	if (power->vregs_on)
> +	if (qcadev->vregs_on)
>  		return 0;
>  
> -	BT_DBG("enabling %d regulators)", power->num_vregs);
> +	BT_DBG("enabling %d regulators)", qcadev->num_vregs);
>  
> -	ret = regulator_bulk_enable(power->num_vregs, power->vreg_bulk);
> +	ret = regulator_bulk_enable(qcadev->num_vregs, qcadev->vreg_bulk);
>  	if (ret)
>  		return ret;
>  
> -	power->vregs_on = true;
> +	qcadev->vregs_on = true;
>  
>  	ret = clk_prepare_enable(qcadev->susclk);
>  	if (ret)
> @@ -1969,38 +1963,37 @@ static int qca_regulator_enable(struct qca_serdev *qcadev)
>  
>  static void qca_regulator_disable(struct qca_serdev *qcadev)
>  {
> -	struct qca_power *power;
> -
>  	if (!qcadev)
>  		return;
>  
> -	power = qcadev->bt_power;
> -
>  	/* Already disabled? */
> -	if (!power->vregs_on)
> +	if (!qcadev->vregs_on)
>  		return;
>  
> -	regulator_bulk_disable(power->num_vregs, power->vreg_bulk);
> -	power->vregs_on = false;
> +	regulator_bulk_disable(qcadev->num_vregs, qcadev->vreg_bulk);
> +	qcadev->vregs_on = false;
>  
>  	clk_disable_unprepare(qcadev->susclk);
>  }
>  
> -static int qca_init_regulators(struct qca_power *qca,
> -				const struct qca_vreg *vregs, size_t num_vregs)
> +static int qca_init_regulators(struct device *dev, struct qca_serdev *qca,
> +			       const struct qca_vreg *vregs, size_t num_vregs)
>  {
>  	struct regulator_bulk_data *bulk;
>  	int ret;
>  	int i;
>  
> -	bulk = devm_kcalloc(qca->dev, num_vregs, sizeof(*bulk), GFP_KERNEL);
> +	if (!num_vregs)
> +		return 0;
> +
> +	bulk = devm_kcalloc(dev, num_vregs, sizeof(*bulk), GFP_KERNEL);
>  	if (!bulk)
>  		return -ENOMEM;
>  
>  	for (i = 0; i < num_vregs; i++)
>  		bulk[i].supply = vregs[i].name;
>  
> -	ret = devm_regulator_bulk_get(qca->dev, num_vregs, bulk);
> +	ret = devm_regulator_bulk_get(dev, num_vregs, bulk);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -2044,21 +2037,15 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	if ((qca_is_wcn399x(data->soc_type) ||
>  	     qca_is_wcn6750(data->soc_type))) {
>  		qcadev->btsoc_type = data->soc_type;
> -		qcadev->bt_power = devm_kzalloc(&serdev->dev,
> -						sizeof(struct qca_power),
> -						GFP_KERNEL);
> -		if (!qcadev->bt_power)
> -			return -ENOMEM;
> -
> -		qcadev->bt_power->dev = &serdev->dev;
> -		err = qca_init_regulators(qcadev->bt_power, data->vregs,
> +
> +		err = qca_init_regulators(&serdev->dev, qcadev, data->vregs,
>  					  data->num_vregs);
>  		if (err) {
>  			BT_ERR("Failed to init regulators:%d", err);
>  			return err;
>  		}
>  
> -		qcadev->bt_power->vregs_on = false;
> +		qcadev->vregs_on = false;
>  
>  		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>  					       GPIOD_OUT_LOW);
> @@ -2139,11 +2126,10 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  static void qca_serdev_remove(struct serdev_device *serdev)
>  {
>  	struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
> -	struct qca_power *power = qcadev->bt_power;
>  
>  	if ((qca_is_wcn399x(qcadev->btsoc_type) ||
>  	     qca_is_wcn6750(qcadev->btsoc_type)) &&
> -	     power->vregs_on)
> +	     qcadev->vregs_on)
>  		qca_power_shutdown(&qcadev->serdev_hu);
>  	else if (qcadev->susclk)
>  		clk_disable_unprepare(qcadev->susclk);
> -- 
> 2.30.2
> 

Re: [PATCH v3 3/7] Bluetooth: hci_qca: provide default device data

[Bjorn Andersson]

On Mon 21 Jun 17:31 CDT 2021, Dmitry Baryshkov wrote:

> In order to simplify probe function provide default device data. This
> removes the rest of if (data) checks.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/bluetooth/hci_qca.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 53deea2eb7b4..3704dbadba1d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1874,6 +1874,11 @@ static const struct qca_device_data qca_soc_data_wcn6750 = {
>  	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
>  };
>  
> +static const struct qca_device_data qca_soc_data_default = {
> +	.soc_type = QCA_ROME,
> +	.num_vregs = 0,
> +};
> +
>  static void qca_power_shutdown(struct hci_uart *hu)
>  {
>  	struct qca_serdev *qcadev;
> @@ -2019,12 +2024,15 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	int err;
>  	bool power_ctrl_enabled = true;
>  
> +	data = device_get_match_data(&serdev->dev);
> +	if (!data)
> +		return -EINVAL;
> +
>  	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
>  	if (!qcadev)
>  		return -ENOMEM;
>  
>  	qcadev->serdev_hu.serdev = serdev;
> -	data = device_get_match_data(&serdev->dev);
>  	serdev_device_set_drvdata(serdev, qcadev);
>  	device_property_read_string(&serdev->dev, "firmware-name",
>  					 &qcadev->firmware_name);
> @@ -2033,9 +2041,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	if (!qcadev->oper_speed)
>  		BT_DBG("UART will pick default operating speed");
>  
> -	if (data &&
> -	    (qca_is_wcn399x(data->soc_type) ||
> -	    qca_is_wcn6750(data->soc_type))) {
> +	if ((qca_is_wcn399x(data->soc_type) ||
> +	     qca_is_wcn6750(data->soc_type))) {
>  		qcadev->btsoc_type = data->soc_type;
>  		qcadev->bt_power = devm_kzalloc(&serdev->dev,
>  						sizeof(struct qca_power),
> @@ -2077,10 +2084,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  			return err;
>  		}
>  	} else {
> -		if (data)
> -			qcadev->btsoc_type = data->soc_type;
> -		else
> -			qcadev->btsoc_type = QCA_ROME;
> +		qcadev->btsoc_type = data->soc_type;
>  
>  		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>  					       GPIOD_OUT_LOW);
> @@ -2309,9 +2313,9 @@ static SIMPLE_DEV_PM_OPS(qca_pm_ops, qca_suspend, qca_resume);
>  
>  #ifdef CONFIG_OF
>  static const struct of_device_id qca_bluetooth_of_match[] = {
> -	{ .compatible = "qcom,qca6174-bt" },
> +	{ .compatible = "qcom,qca6174-bt", .data = &qca_soc_data_default},
>  	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
> -	{ .compatible = "qcom,qca9377-bt" },
> +	{ .compatible = "qcom,qca9377-bt", .data = &qca_soc_data_default},
>  	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
>  	{ .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991},
>  	{ .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998},
> -- 
> 2.30.2
> 

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Ulf Hansson]

On Wed, 14 Jul 2021 at 18:47, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Jul 08, 2021 at 02:37:44PM +0300, Dmitry Baryshkov wrote:
> > Hi,
> >
> > On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > - Peter (the email was bouncing)
> >
> > + Peter's kernel.org address
> >
> > >
> > > On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> > > >
> > > > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> > > >
> > > > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > > > being controlled through the UART and WiFi being present on PCIe
> > > > > > bus. Both blocks share common power sources. Add device driver handling
> > > > > > power sequencing of QCA6390/1.
> > > >
> > > > > Power sequencing of discoverable buses have been discussed several
> > > > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > > > from Peter Chen. I don't think there is a common solution, yet.
> > > >
> > > > This feels a bit different to the power sequencing problem - it's not
> > > > exposing the individual inputs to the device but rather is a block that
> > > > manages everything but needs a bit of a kick to get things going (I'd
> > > > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > > > but it's not quite the same issue I think, something that can handle
> > > > control of the individual resources might still struggle with this.
> > >
> > > Well, to me it looks very similar to those resouses we could manage
> > > with the mmc pwrseq, for SDIO. It's also typically the same kind of
> > > combo-chips that moved from supporting SDIO to PCIe, for improved
> > > performance I guess. More importantly, the same constraint to
> > > pre-power on the device is needed to allow it to be discovered/probed.
> >
> > In our case we'd definitely use pwrseq for PCIe bus and we can also
> > benefit from using pwrseq for serdev and for platform busses also (for
> > the same story of WiFi+BT chips).
> >
> > I can take a look at rewriting pwrseq code to also handle the PCIe
> > bus. Rewriting it to be a generic lib seems like an easy task,
> > plugging it into PCIe code would be more fun.
> >
> > Platform and serdev... Definitely even more fun.
>
> I don't want to see pwrseq (the binding) expanded to other buses. If
> that was the answer, we wouldn't be having this discussion. It was a
> mistake for MMC IMO.

Let's make sure we get your point correctly. I think we have discussed
this in the past, but let's refresh our memories.

If I recall correctly, you are against the mmc pwrseq DT bindings
because we are using a separate pwrseq OF node, that we point to via a
"mmc-pwrseq" property that contains a phandle from the mmc controller
device node. Is that correct?

If we would have encoded the power sequence specific properties, from
within a child node for the mmc controller node, that would have been
okay for you, right?

>
> If pwrseq works as a kernel library/api, then I have no issue with that.

That's what Peter Chen was trying to do. A generic interface, flexible
enough so it can be used for many similar configurations (but not
exactly the same).

Perhaps it was too generic though.

>
> >
> > > Therefore, I think it would be worth having a common solution for
> > > this, rather than a solution per subsystem or even worse, per device.
>
> Power sequencing requirements are inheritently per device unless we're
> talking about standard connectors.

The requirements are certainly per device, but the way to manage them
doesn't have to be.

As you said above, a generic library that subsystems/drivers can call
to power on/off a discoverable device, before trying to probe it would
be a good start.

>
> This is a solved problem on MDIO. It's quite simple. If there's a DT
> node for a device you haven't discovered, then probe it anyways.

A child OF node?

Then what do you think about some common power sequence properties
that we can use in such node?

>
> Rob

Kind regards
Uffe

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Bjorn Andersson]

On Tue 10 Aug 06:55 CDT 2021, Ulf Hansson wrote:

> On Wed, 14 Jul 2021 at 18:47, Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Jul 08, 2021 at 02:37:44PM +0300, Dmitry Baryshkov wrote:
> > > Hi,
> > >
> > > On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > - Peter (the email was bouncing)
> > >
> > > + Peter's kernel.org address
> > >
> > > >
> > > > On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> > > > >
> > > > > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > > > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> > > > >
> > > > > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > > > > being controlled through the UART and WiFi being present on PCIe
> > > > > > > bus. Both blocks share common power sources. Add device driver handling
> > > > > > > power sequencing of QCA6390/1.
> > > > >
> > > > > > Power sequencing of discoverable buses have been discussed several
> > > > > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > > > > from Peter Chen. I don't think there is a common solution, yet.
> > > > >
> > > > > This feels a bit different to the power sequencing problem - it's not
> > > > > exposing the individual inputs to the device but rather is a block that
> > > > > manages everything but needs a bit of a kick to get things going (I'd
> > > > > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > > > > but it's not quite the same issue I think, something that can handle
> > > > > control of the individual resources might still struggle with this.
> > > >
> > > > Well, to me it looks very similar to those resouses we could manage
> > > > with the mmc pwrseq, for SDIO. It's also typically the same kind of
> > > > combo-chips that moved from supporting SDIO to PCIe, for improved
> > > > performance I guess. More importantly, the same constraint to
> > > > pre-power on the device is needed to allow it to be discovered/probed.
> > >
> > > In our case we'd definitely use pwrseq for PCIe bus and we can also
> > > benefit from using pwrseq for serdev and for platform busses also (for
> > > the same story of WiFi+BT chips).
> > >
> > > I can take a look at rewriting pwrseq code to also handle the PCIe
> > > bus. Rewriting it to be a generic lib seems like an easy task,
> > > plugging it into PCIe code would be more fun.
> > >
> > > Platform and serdev... Definitely even more fun.
> >
> > I don't want to see pwrseq (the binding) expanded to other buses. If
> > that was the answer, we wouldn't be having this discussion. It was a
> > mistake for MMC IMO.
> 
> Let's make sure we get your point correctly. I think we have discussed
> this in the past, but let's refresh our memories.
> 
> If I recall correctly, you are against the mmc pwrseq DT bindings
> because we are using a separate pwrseq OF node, that we point to via a
> "mmc-pwrseq" property that contains a phandle from the mmc controller
> device node. Is that correct?
> 
> If we would have encoded the power sequence specific properties, from
> within a child node for the mmc controller node, that would have been
> okay for you, right?
> 

In Dmitry's case, we have an external chip with that needs to be powered
on per a specific sequence, at which point the WiFi driver on PCIe and
BT driver on serdev will be able to communicate with the device.

The extended case of this is where we have an SDX55 modem soldered onto
the pcb next to the SoC, in which case the power sequencing is even more
complex and additionally there are incoming gpios used to detect things
such as the firmware of the modem has crashed and Linux needs to toggle
power and rescan the PCIe bus.

In both of these cases it seems quite reasonable to represent that
external chip (and it's power needs) as a separate DT node. But we need
a way to link the functional devices to that thing.

Regards,
Bjorn

> >
> > If pwrseq works as a kernel library/api, then I have no issue with that.
> 
> That's what Peter Chen was trying to do. A generic interface, flexible
> enough so it can be used for many similar configurations (but not
> exactly the same).
> 
> Perhaps it was too generic though.
> 
> >
> > >
> > > > Therefore, I think it would be worth having a common solution for
> > > > this, rather than a solution per subsystem or even worse, per device.
> >
> > Power sequencing requirements are inheritently per device unless we're
> > talking about standard connectors.
> 
> The requirements are certainly per device, but the way to manage them
> doesn't have to be.
> 
> As you said above, a generic library that subsystems/drivers can call
> to power on/off a discoverable device, before trying to probe it would
> be a good start.
> 
> >
> > This is a solved problem on MDIO. It's quite simple. If there's a DT
> > node for a device you haven't discovered, then probe it anyways.
> 
> A child OF node?
> 
> Then what do you think about some common power sequence properties
> that we can use in such node?
> 
> >
> > Rob
> 
> Kind regards
> Uffe

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Ulf Hansson]

On Tue, 10 Aug 2021 at 18:03, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Tue 10 Aug 06:55 CDT 2021, Ulf Hansson wrote:
>
> > On Wed, 14 Jul 2021 at 18:47, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Thu, Jul 08, 2021 at 02:37:44PM +0300, Dmitry Baryshkov wrote:
> > > > Hi,
> > > >
> > > > On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > - Peter (the email was bouncing)
> > > >
> > > > + Peter's kernel.org address
> > > >
> > > > >
> > > > > On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
> > > > > >
> > > > > > On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
> > > > > > > On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
> > > > > >
> > > > > > > > Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
> > > > > > > > being controlled through the UART and WiFi being present on PCIe
> > > > > > > > bus. Both blocks share common power sources. Add device driver handling
> > > > > > > > power sequencing of QCA6390/1.
> > > > > >
> > > > > > > Power sequencing of discoverable buses have been discussed several
> > > > > > > times before at LKML. The last attempt [1] I am aware of, was in 2017
> > > > > > > from Peter Chen. I don't think there is a common solution, yet.
> > > > > >
> > > > > > This feels a bit different to the power sequencing problem - it's not
> > > > > > exposing the individual inputs to the device but rather is a block that
> > > > > > manages everything but needs a bit of a kick to get things going (I'd
> > > > > > guess that with ACPI it'd be triggered via AML).  It's in the same space
> > > > > > but it's not quite the same issue I think, something that can handle
> > > > > > control of the individual resources might still struggle with this.
> > > > >
> > > > > Well, to me it looks very similar to those resouses we could manage
> > > > > with the mmc pwrseq, for SDIO. It's also typically the same kind of
> > > > > combo-chips that moved from supporting SDIO to PCIe, for improved
> > > > > performance I guess. More importantly, the same constraint to
> > > > > pre-power on the device is needed to allow it to be discovered/probed.
> > > >
> > > > In our case we'd definitely use pwrseq for PCIe bus and we can also
> > > > benefit from using pwrseq for serdev and for platform busses also (for
> > > > the same story of WiFi+BT chips).
> > > >
> > > > I can take a look at rewriting pwrseq code to also handle the PCIe
> > > > bus. Rewriting it to be a generic lib seems like an easy task,
> > > > plugging it into PCIe code would be more fun.
> > > >
> > > > Platform and serdev... Definitely even more fun.
> > >
> > > I don't want to see pwrseq (the binding) expanded to other buses. If
> > > that was the answer, we wouldn't be having this discussion. It was a
> > > mistake for MMC IMO.
> >
> > Let's make sure we get your point correctly. I think we have discussed
> > this in the past, but let's refresh our memories.
> >
> > If I recall correctly, you are against the mmc pwrseq DT bindings
> > because we are using a separate pwrseq OF node, that we point to via a
> > "mmc-pwrseq" property that contains a phandle from the mmc controller
> > device node. Is that correct?
> >
> > If we would have encoded the power sequence specific properties, from
> > within a child node for the mmc controller node, that would have been
> > okay for you, right?
> >
>
> In Dmitry's case, we have an external chip with that needs to be powered
> on per a specific sequence, at which point the WiFi driver on PCIe and
> BT driver on serdev will be able to communicate with the device.

Thanks for sharing more details.

So, not only do we have a discoverable device that needs to be powered
on in a device specific way before probing, but in fact we have two
consumers of that "combo chip", one (PCIe) for Wifi and one (serdev)
for Bluetooth.

>
> The extended case of this is where we have an SDX55 modem soldered onto
> the pcb next to the SoC, in which case the power sequencing is even more
> complex and additionally there are incoming gpios used to detect things
> such as the firmware of the modem has crashed and Linux needs to toggle
> power and rescan the PCIe bus.

That sounds very similar to what we manage for the SDIO bus already.

We have a mmc pwrseq node to describe what resources that are needed
to power on/off the external chip. The driver for the functional
device (Wifi chip for example) may then call SDIO APIs provided by the
mmc core to power on/off the device, in case some kind of reset would
be needed.

Additionally, we have a child node below the mmc controller node,
allowing us to describe device specific things for the SDIO functional
device, like an out-of-band IRQ line for example.

Overall, this seems to work fine, even if the DT bindings may be questionable.

>
> In both of these cases it seems quite reasonable to represent that
> external chip (and it's power needs) as a separate DT node. But we need
> a way to link the functional devices to that thing.

Don't get me wrong, I am not suggesting we should re-use the
mmc-pwrseq DT bindings - but just trying to share our experience
around them.

In the cases you describe, it certainly sounds like we need some kind
of minimal description in DT for these functional external devices.
For GPIO pins, for example.

How to describe this in DT is one thing, let's see if Rob can help to
point us in some direction of what could make sense.

When it comes to implementing a library/interface to manage these
functional devices, I guess we just have to continue to explore
various options. Perhaps just start simple with another subsystem,
like PCIe and see where this brings us.

>
> Regards,
> Bjorn
>

[...]

Kind regards
Uffe

Re: [PATCH v3 2/7] regulator: qca6390: add support for QCA639x powerup sequence

[Dmitry Baryshkov]

On 12/08/2021 12:48, Ulf Hansson wrote:
> On Tue, 10 Aug 2021 at 18:03, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
>>
>> On Tue 10 Aug 06:55 CDT 2021, Ulf Hansson wrote:
>>
>>> On Wed, 14 Jul 2021 at 18:47, Rob Herring <robh@kernel.org> wrote:
>>>>
>>>> On Thu, Jul 08, 2021 at 02:37:44PM +0300, Dmitry Baryshkov wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, 8 Jul 2021 at 13:10, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>>
>>>>>> - Peter (the email was bouncing)
>>>>>
>>>>> + Peter's kernel.org address
>>>>>
>>>>>>
>>>>>> On Tue, 6 Jul 2021 at 13:55, Mark Brown <broonie@kernel.org> wrote:
>>>>>>>
>>>>>>> On Tue, Jul 06, 2021 at 09:54:03AM +0200, Ulf Hansson wrote:
>>>>>>>> On Tue, 22 Jun 2021 at 00:32, Dmitry Baryshkov
>>>>>>>
>>>>>>>>> Qualcomm QCA6390/1 is a family of WiFi + Bluetooth SoCs, with BT part
>>>>>>>>> being controlled through the UART and WiFi being present on PCIe
>>>>>>>>> bus. Both blocks share common power sources. Add device driver handling
>>>>>>>>> power sequencing of QCA6390/1.
>>>>>>>
>>>>>>>> Power sequencing of discoverable buses have been discussed several
>>>>>>>> times before at LKML. The last attempt [1] I am aware of, was in 2017
>>>>>>>> from Peter Chen. I don't think there is a common solution, yet.
>>>>>>>
>>>>>>> This feels a bit different to the power sequencing problem - it's not
>>>>>>> exposing the individual inputs to the device but rather is a block that
>>>>>>> manages everything but needs a bit of a kick to get things going (I'd
>>>>>>> guess that with ACPI it'd be triggered via AML).  It's in the same space
>>>>>>> but it's not quite the same issue I think, something that can handle
>>>>>>> control of the individual resources might still struggle with this.
>>>>>>
>>>>>> Well, to me it looks very similar to those resouses we could manage
>>>>>> with the mmc pwrseq, for SDIO. It's also typically the same kind of
>>>>>> combo-chips that moved from supporting SDIO to PCIe, for improved
>>>>>> performance I guess. More importantly, the same constraint to
>>>>>> pre-power on the device is needed to allow it to be discovered/probed.
>>>>>
>>>>> In our case we'd definitely use pwrseq for PCIe bus and we can also
>>>>> benefit from using pwrseq for serdev and for platform busses also (for
>>>>> the same story of WiFi+BT chips).
>>>>>
>>>>> I can take a look at rewriting pwrseq code to also handle the PCIe
>>>>> bus. Rewriting it to be a generic lib seems like an easy task,
>>>>> plugging it into PCIe code would be more fun.
>>>>>
>>>>> Platform and serdev... Definitely even more fun.
>>>>
>>>> I don't want to see pwrseq (the binding) expanded to other buses. If
>>>> that was the answer, we wouldn't be having this discussion. It was a
>>>> mistake for MMC IMO.
>>>
>>> Let's make sure we get your point correctly. I think we have discussed
>>> this in the past, but let's refresh our memories.
>>>
>>> If I recall correctly, you are against the mmc pwrseq DT bindings
>>> because we are using a separate pwrseq OF node, that we point to via a
>>> "mmc-pwrseq" property that contains a phandle from the mmc controller
>>> device node. Is that correct?
>>>
>>> If we would have encoded the power sequence specific properties, from
>>> within a child node for the mmc controller node, that would have been
>>> okay for you, right?
>>>
>>
>> In Dmitry's case, we have an external chip with that needs to be powered
>> on per a specific sequence, at which point the WiFi driver on PCIe and
>> BT driver on serdev will be able to communicate with the device.
> 
> Thanks for sharing more details.
> 
> So, not only do we have a discoverable device that needs to be powered
> on in a device specific way before probing, but in fact we have two
> consumers of that "combo chip", one (PCIe) for Wifi and one (serdev)
> for Bluetooth.
> 
>>
>> The extended case of this is where we have an SDX55 modem soldered onto
>> the pcb next to the SoC, in which case the power sequencing is even more
>> complex and additionally there are incoming gpios used to detect things
>> such as the firmware of the modem has crashed and Linux needs to toggle
>> power and rescan the PCIe bus.
> 
> That sounds very similar to what we manage for the SDIO bus already.
> 
> We have a mmc pwrseq node to describe what resources that are needed
> to power on/off the external chip. The driver for the functional
> device (Wifi chip for example) may then call SDIO APIs provided by the
> mmc core to power on/off the device, in case some kind of reset would
> be needed.
> 
> Additionally, we have a child node below the mmc controller node,
> allowing us to describe device specific things for the SDIO functional
> device, like an out-of-band IRQ line for example.
> 
> Overall, this seems to work fine, even if the DT bindings may be questionable.
> 
>>
>> In both of these cases it seems quite reasonable to represent that
>> external chip (and it's power needs) as a separate DT node. But we need
>> a way to link the functional devices to that thing.
> 
> Don't get me wrong, I am not suggesting we should re-use the
> mmc-pwrseq DT bindings - but just trying to share our experience
> around them.
> 
> In the cases you describe, it certainly sounds like we need some kind
> of minimal description in DT for these functional external devices.
> For GPIO pins, for example.
> 
> How to describe this in DT is one thing, let's see if Rob can help to
> point us in some direction of what could make sense.
> 
> When it comes to implementing a library/interface to manage these
> functional devices, I guess we just have to continue to explore
> various options. Perhaps just start simple with another subsystem,
> like PCIe and see where this brings us.

Thank you for your opinion and suggestions. In fact I'm probably going 
to start working on non-discoverable busses first (by chaning support 
for few other BT+WiFi Qualcomm chips), later shifting the attention to 
the PCIe part. While this may seem like a longer path, I'd like to 
narrow pwrseq subsystem first, before going into PCIe details.


-- 
With best wishes
Dmitry