| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMRc=MeZvHV-iOSTcaki=+8_j2Uqm_qpY3b1V15o9K0zefy+hw@mail.gmail.com>
Date: Fri, 5 Jul 2024 15:46:24 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: yang.li@...ogic.com
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH 2/3] power: sequenceing: Add power sequence for Amlogic
WCN chips
On Fri, Jul 5, 2024 at 1:13 PM Yang Li via B4 Relay
<devnull+yang.li.amlogic.com@...nel.org> wrote:
>
> From: Yang Li <yang.li@...ogic.com>
>
> 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@...ogic.com>
> ---
> 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.
> +#include <linux/of_gpio.h>
You don't need this either.
> +#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?
> +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.
> + 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.
> +
> + 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.
> + 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.
> + 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
> +
> + 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@...ogic.com>");
> +MODULE_DESCRIPTION("Amlogic WCN PMU power sequencing driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.42.0
>
>
Bart
Powered by blists - more mailing lists