On 2024/7/5 21:46, Bartosz Golaszewski wrote:
On Fri, Jul 5, 2024 at 1:13 PM Yang Li via B4 Relay
<devnull+yang.li.amlogic.com@xxxxxxxxxx> wrote:
From: Yang Li<yang.li@xxxxxxxxxxx>
Add power sequence for Bluetooth and Wi-Fi respectively, including chip_en
pull-up and bt_en pull-up, and generation of the 32.768 clock.
Signed-off-by: Yang Li<yang.li@xxxxxxxxxxx>
---
drivers/power/sequencing/Kconfig | 7 +
drivers/power/sequencing/Makefile | 1 +
drivers/power/sequencing/pwrseq-aml-wcn.c | 209 ++++++++++++++++++++++++++++++
3 files changed, 217 insertions(+)
diff --git a/drivers/power/sequencing/Kconfig b/drivers/power/sequencing/Kconfig
index c9f1cdb66524..65d3b2c20bfb 100644
--- a/drivers/power/sequencing/Kconfig
+++ b/drivers/power/sequencing/Kconfig
@@ -26,4 +26,11 @@ config POWER_SEQUENCING_QCOM_WCN
this driver is needed for correct power control or else we'd risk not
respecting the required delays between enabling Bluetooth and WLAN.
+config POWER_SEQUENCING_AML_WCN
+ tristate "Amlogic WCN family PMU driver"
+ default m if ARCH_MESON
+ help
+ Say Y here to enable the power sequencing driver for Amlogic
+ WCN Bluetooth/WLAN chipsets.
+
endif
diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
index 2eec2df7912d..32706daf8f0f 100644
--- a/drivers/power/sequencing/Makefile
+++ b/drivers/power/sequencing/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_POWER_SEQUENCING) += pwrseq-core.o
pwrseq-core-y := core.o
obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN) += pwrseq-qcom-wcn.o
+obj-$(CONFIG_POWER_SEQUENCING_AML_WCN) += pwrseq-aml-wcn.o
diff --git a/drivers/power/sequencing/pwrseq-aml-wcn.c b/drivers/power/sequencing/pwrseq-aml-wcn.c
new file mode 100644
index 000000000000..6f5bfcf60b9c
--- /dev/null
+++ b/drivers/power/sequencing/pwrseq-aml-wcn.c
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
+/*
+ * Copyright (C) 2024 Amlogic, Inc. All rights reserved
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/gpio.h>
Please see line 5 in this file.
I got it, I will remove this line, and include linux/gpio/consumer.h.
+#include <linux/of_gpio.h>
You don't need this either.
Yes, I will remove it.
+#include <linux/platform_device.h>
+#include <linux/pwrseq/provider.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+struct pwrseq_aml_wcn_ctx {
+ struct pwrseq_device *pwrseq;
+ int bt_enable_gpio;
+ int chip_enable_gpio;
+ struct clk *lpo_clk;
+ unsigned int pwr_count;
+};
+
+static DEFINE_MUTEX(pwrseq_lock);
+
Why is this global?
Okay, I will add it to structure of pwrseq_aml_wcn_ctx .
+static int pwrseq_aml_wcn_chip_enable(struct pwrseq_device *pwrseq)
+{
+ struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+ int err;
+
+ mutex_lock(&pwrseq_lock);
Please use guard() from linux/cleanup.h.
Well, I will use guard(mutex)(&pwrse_lock) to replace
mutex_lock(&pwrseq_lock).
+ if (ctx->pwr_count == 0) {
+ gpio_request(ctx->chip_enable_gpio, "chip-enable-gpios");
+ gpio_direction_output(ctx->chip_enable_gpio, 1);
+ gpio_free(ctx->chip_enable_gpio);
Not only are these legacy APIs but they are also used wrong. You
almost never want to release the GPIO after setting the direction as
someone else may grab it and use it.
Okay, I will use consumer API of devm_gpiod_get() to replace them.
+
+ if (!IS_ERR(ctx->lpo_clk)) {
+ err = clk_prepare_enable(ctx->lpo_clk);
+ if (err) {
+ mutex_unlock(&pwrseq_lock);
+ return err;
+ }
+ }
+ }
+
+ ctx->pwr_count++;
+ mutex_unlock(&pwrseq_lock);
+ return 0;
+}
+
+static int pwrseq_aml_wcn_chip_disable(struct pwrseq_device *pwrseq)
+{
+ struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+
+ mutex_lock(&pwrseq_lock);
+ if (--ctx->pwr_count == 0) {
+ gpio_request(ctx->chip_enable_gpio, "chip-enable-gpios");
+ gpio_direction_output(ctx->chip_enable_gpio, 0);
+ gpio_free(ctx->chip_enable_gpio);
+
+ if (!IS_ERR(ctx->lpo_clk))
+ clk_disable_unprepare(ctx->lpo_clk);
+ }
+
+ mutex_unlock(&pwrseq_lock);
+ return 0;
+}
+
+static const struct pwrseq_unit_data pwrseq_aml_wcn_chip_power_unit_data = {
+ .name = "chip-enable",
+ .enable = pwrseq_aml_wcn_chip_enable,
+ .disable = pwrseq_aml_wcn_chip_disable,
+};
+
+static const struct pwrseq_unit_data *pwrseq_aml_wcn_unit_deps[] = {
+ &pwrseq_aml_wcn_chip_power_unit_data,
+ NULL
+};
+
+static int pwrseq_aml_wcn_bt_enable(struct pwrseq_device *pwrseq)
+{
+ struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+
+ gpio_request(ctx->bt_enable_gpio, "bt-enable-gpios");
+ gpio_direction_output(ctx->bt_enable_gpio, 1);
+ gpio_free(ctx->bt_enable_gpio);
+
+ /* wait 100ms for bluetooth controller power on */
+ msleep(100);
+
+ return 0;
+}
+
+static int pwrseq_aml_wcn_bt_disable(struct pwrseq_device *pwrseq)
+{
+ struct pwrseq_aml_wcn_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
+
+ gpio_request(ctx->bt_enable_gpio, "bt-enable-gpios");
+ gpio_direction_output(ctx->bt_enable_gpio, 0);
+ gpio_free(ctx->bt_enable_gpio);
+
+ return 0;
+}
+
+static const struct pwrseq_unit_data pwrseq_aml_wcn_bt_unit_data = {
+ .name = "bluetooth-enable",
+ .deps = pwrseq_aml_wcn_unit_deps,
+ .enable = pwrseq_aml_wcn_bt_enable,
+ .disable = pwrseq_aml_wcn_bt_disable,
+};
+
+static const struct pwrseq_unit_data pwrseq_aml_wcn_wlan_unit_data = {
+ .name = "wlan-enable",
+ .deps = pwrseq_aml_wcn_unit_deps,
+};
+
+static const struct pwrseq_target_data pwrseq_aml_wcn_bt_target_data = {
+ .name = "bluetooth",
+ .unit = &pwrseq_aml_wcn_bt_unit_data,
+};
+
+static const struct pwrseq_target_data pwrseq_aml_wcn_wlan_target_data = {
+ .name = "wlan",
+ .unit = &pwrseq_aml_wcn_wlan_unit_data,
+};
+
+static const struct pwrseq_target_data *pwrseq_aml_wcn_targets[] = {
+ &pwrseq_aml_wcn_bt_target_data,
+ &pwrseq_aml_wcn_wlan_target_data,
+ NULL
+};
+
+static int pwrseq_aml_wcn_match(struct pwrseq_device *pwrseq,
+ struct device *dev)
+{
+ struct device_node *dev_node = dev->of_node;
+
+ if (!of_property_present(dev_node, "amlogic,wcn-pwrseq"))
+ return 0;
+
You must never reference the notion of power sequencing in the DT.
Please take a look at the pwrseq-qcom-wcn driver where we model the
PMU with its regulators and then parse them in match() to figure out
if we have the right thing or not.
There is some different between pwrseq-aml-wcn and pwrseq-qcom-wcn,
pwrseq-aml-wcn device is abstracted to manage the chip-en pin, bt-en
pin, and 32.768KHz clock. The drivers/power/sequence/core.c requirements
need to be defined match () function is used to determine whether a
potential consumers actually related to the sequencer. So, I need to add
a meaningless node "amlogic,wcn-pwrseq" to both the consumer dt-binding
and the provider dt-binding.
Right now, I add "amlogic,wcn-pwrseq" in binding file of
"amlogic,w155s2-bt.yaml" only, may I need to add this properties
("amlogic,wcn-pwrseq") in the binding file of "amlogic,w155s2-pwrseq.yaml"?
Or there are any others way to fixed this issue please let me know.
+ return 1;
+}
+
+static int pwrseq_aml_wcn_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct pwrseq_aml_wcn_ctx *ctx;
+ struct pwrseq_config config;
+
+ ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ ctx->bt_enable_gpio = of_get_named_gpio(dev->of_node,
+ "amlogic,bt-enable-gpios", 0);
+ if (!gpio_is_valid(ctx->bt_enable_gpio))
+ return dev_err_probe(dev, ctx->bt_enable_gpio,
+ "Failed to get the bt enable GPIO");
+
+ ctx->chip_enable_gpio = of_get_named_gpio(dev->of_node,
+ "amlogic,chip-enable-gpios", 0);
You don't need the OF variant. Use the regular devm_gpiod_get(). You
also forgot to release it but the devres variant will take care of it.
Well, I will do it.
+ if (!gpio_is_valid(ctx->chip_enable_gpio))
+ return dev_err_probe(dev, ctx->bt_enable_gpio,
+ "Failed to get the chip enable GPIO");
Wat
I got it, and I will fix it.
+
+ ctx->lpo_clk = devm_clk_get_optional(dev, NULL);
+ if (IS_ERR(ctx->lpo_clk))
+ return dev_err_probe(dev, PTR_ERR(ctx->lpo_clk),
+ "Failed to get the clock source");
+
+ memset(&config, 0, sizeof(config));
+
+ config.parent = dev;
+ config.owner = THIS_MODULE;
+ config.drvdata = ctx;
+ config.match = pwrseq_aml_wcn_match;
+ config.targets = pwrseq_aml_wcn_targets;
+
+ ctx->pwr_count = 0;
+ ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
+ if (IS_ERR(ctx->pwrseq))
+ return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
+ "Failed to register the power sequencer\n");
+
+ return 0;
+}
+
+static const struct of_device_id pwrseq_aml_wcn_of_match[] = {
+ { .compatible = "amlogic,w155s2-pwrseq" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, pwrseq_aml_wcn_of_match);
+
+static struct platform_driver pwrseq_aml_wcn_driver = {
+ .driver = {
+ .name = "pwrseq-aml_wcn",
+ .of_match_table = pwrseq_aml_wcn_of_match,
+ },
+ .probe = pwrseq_aml_wcn_probe,
+};
+module_platform_driver(pwrseq_aml_wcn_driver);
+
+MODULE_AUTHOR("Yang Li<yang.li@xxxxxxxxxxx>");
+MODULE_DESCRIPTION("Amlogic WCN PMU power sequencing driver");
+MODULE_LICENSE("GPL");
--
2.42.0
Bart
