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
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
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
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
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
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
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
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
[-- Attachment #1: Type: text/plain, Size: 3811 bytes --] This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=504629 ---Test result--- Test Summary: CheckPatch FAIL 1.51 seconds GitLint PASS 0.87 seconds BuildKernel PASS 618.69 seconds TestRunner: Setup PASS 403.43 seconds TestRunner: l2cap-tester PASS 2.93 seconds TestRunner: bnep-tester PASS 2.06 seconds TestRunner: mgmt-tester PASS 29.40 seconds TestRunner: rfcomm-tester PASS 2.31 seconds TestRunner: sco-tester PASS 2.22 seconds TestRunner: smp-tester PASS 2.30 seconds TestRunner: userchan-tester PASS 2.09 seconds Details ############################## Test: CheckPatch - FAIL - 1.51 seconds Run checkpatch.pl script with rule in .checkpatch.conf dt-bindings: regulator: qcom,qca6390: add binding for QCA6390 device WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #15: new file mode 100644 total: 0 errors, 1 warnings, 70 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. "[PATCH] dt-bindings: regulator: qcom,qca6390: add binding for QCA6390" has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. regulator: qca6390: add support for QCA639x powerup sequence WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #50: new file mode 100644 total: 0 errors, 1 warnings, 183 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. "[PATCH] regulator: qca6390: add support for QCA639x powerup sequence" has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. ############################## Test: GitLint - PASS - 0.87 seconds Run gitlint with rule in .gitlint ############################## Test: BuildKernel - PASS - 618.69 seconds Build Kernel with minimal configuration supports Bluetooth ############################## Test: TestRunner: Setup - PASS - 403.43 seconds Setup environment for running Test Runner ############################## Test: TestRunner: l2cap-tester - PASS - 2.93 seconds Run test-runner with l2cap-tester Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: bnep-tester - PASS - 2.06 seconds Run test-runner with bnep-tester Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: mgmt-tester - PASS - 29.40 seconds Run test-runner with mgmt-tester Total: 446, Passed: 433 (97.1%), Failed: 0, Not Run: 13 ############################## Test: TestRunner: rfcomm-tester - PASS - 2.31 seconds Run test-runner with rfcomm-tester Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: sco-tester - PASS - 2.22 seconds Run test-runner with sco-tester Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: smp-tester - PASS - 2.30 seconds Run test-runner with smp-tester Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: userchan-tester - PASS - 2.09 seconds Run test-runner with userchan-tester Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0 --- Regards, Linux Bluetooth [-- Attachment #2: l2cap-tester.log --] [-- Type: application/octet-stream, Size: 44350 bytes --] [-- Attachment #3: bnep-tester.log --] [-- Type: application/octet-stream, Size: 3556 bytes --] [-- Attachment #4: mgmt-tester.log --] [-- Type: application/octet-stream, Size: 608513 bytes --] [-- Attachment #5: rfcomm-tester.log --] [-- Type: application/octet-stream, Size: 11677 bytes --] [-- Attachment #6: sco-tester.log --] [-- Type: application/octet-stream, Size: 9912 bytes --] [-- Attachment #7: smp-tester.log --] [-- Type: application/octet-stream, Size: 11823 bytes --] [-- Attachment #8: userchan-tester.log --] [-- Type: application/octet-stream, Size: 5453 bytes --]
[-- 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 --]
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
[-- 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 --]
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
[-- 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 --]
+ 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 >
[-- 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 --]
- 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
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
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
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
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
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 >
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 >
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
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
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
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