| 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: <fbc089d4-8be9-4ebf-b84d-04fbd3d5a253@amlogic.com>
Date: Wed, 10 Jul 2024 11:00:13 +0800
From: Yang Li <yang.li@...ogic.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
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 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@...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.
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@...ogic.com>");
>> +MODULE_DESCRIPTION("Amlogic WCN PMU power sequencing driver");
>> +MODULE_LICENSE("GPL");
>>
>> --
>> 2.42.0
>>
>>
> Bart
Powered by blists - more mailing lists