by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH v1 3/4] mfd: tps6594-esm: Add driver for TI TPS6594 ESM

On Thu, 16 Feb 2023, Julien Panis wrote:

> This patch adds support for TPS6594 ESM (Error Signal Monitor).
> This device monitors the SoC error output signal at its nERR_SOC input pin.
> In error condition, ESM toggles its nRSTOUT_SOC pin to reset the SoC.

This is not an MFD driver.

Please move it somewhere more appropriate.

> Signed-off-by: Julien Panis <[email protected]>
> ---
> drivers/mfd/Kconfig | 10 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/tps6594-esm.c | 132 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 143 insertions(+)
> create mode 100644 drivers/mfd/tps6594-esm.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3d2f5a10f54b..cbf7cfb70879 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1722,6 +1722,16 @@ config MFD_TPS6594_SPI
> This driver can also be built as a module. If so, the module
> will be called tps6594-spi.
>
> +config MFD_TPS6594_ESM
> + tristate "TI TPS6594 Error Signal Monitor support"
> + depends on MFD_TPS6594
> + help
> + Support ESM (Error Signal Monitor) on TPS6594 PMIC devices.
> + ESM is used typically to reboot the board in error condition.
> +
> + This driver can also be built as a module. If so, the module
> + will be called tps6594-esm.
> +
> config TWL4030_CORE
> bool "TI TWL4030/TWL5030/TWL6030/TPS659x0 Support"
> depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index d2cba7326b07..ae2d404cd6b8 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -105,6 +105,7 @@ obj-$(CONFIG_MFD_TPS65912_SPI) += tps65912-spi.o
> obj-$(CONFIG_MFD_TPS6594) += tps6594-core.o
> obj-$(CONFIG_MFD_TPS6594_I2C) += tps6594-i2c.o
> obj-$(CONFIG_MFD_TPS6594_SPI) += tps6594-spi.o
> +obj-$(CONFIG_MFD_TPS6594_ESM) += tps6594-esm.o
> obj-$(CONFIG_MENELAUS) += menelaus.o
>
> obj-$(CONFIG_TWL4030_CORE) += twl-core.o twl4030-irq.o twl6030-irq.o
> diff --git a/drivers/mfd/tps6594-esm.c b/drivers/mfd/tps6594-esm.c
> new file mode 100644
> index 000000000000..db242b50846f
> --- /dev/null
> +++ b/drivers/mfd/tps6594-esm.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * ESM (Error Signal Monitor) driver for TI TPS6594/TPS6593/LP8764X PMICs
> + *
> + * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/tps6594.h>
> +
> +static irqreturn_t tps6594_esm_isr(int irq, void *dev_id)
> +{
> + struct platform_device *pdev = dev_id;
> + int i;
> +
> + for (i = 0 ; i < pdev->num_resources ; i++) {
> + if (irq == platform_get_irq_byname(pdev, pdev->resource[i].name)) {
> + dev_err(pdev->dev.parent, "%s error detected\n", pdev->resource[i].name);
> + return IRQ_HANDLED;
> + }
> + }
> +
> + return IRQ_NONE;
> +}
> +
> +static int tps6594_esm_probe(struct platform_device *pdev)
> +{
> + struct tps6594 *tps = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + int irq;
> + int ret;
> + int i;
> +
> + for (i = 0 ; i < pdev->num_resources ; i++) {
> + irq = platform_get_irq_byname(pdev, pdev->resource[i].name);
> + if (irq < 0)
> + return dev_err_probe(dev, irq, "Failed to get %s irq\n",
> + pdev->resource[i].name);
> +
> + ret = devm_request_threaded_irq(dev, irq, NULL,
> + tps6594_esm_isr, IRQF_ONESHOT,
> + pdev->resource[i].name, pdev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to request irq\n");
> + }
> +
> + ret = regmap_set_bits(tps->regmap, TPS6594_REG_ESM_SOC_MODE_CFG,
> + TPS6594_BIT_ESM_SOC_EN | TPS6594_BIT_ESM_SOC_ENDRV);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to configure ESM\n");
> +
> + ret = regmap_set_bits(tps->regmap, TPS6594_REG_ESM_SOC_START_REG,
> + TPS6594_BIT_ESM_SOC_START);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to start ESM\n");
> +
> + pm_runtime_enable(dev);
> + pm_runtime_get_sync(dev);
> +
> + return 0;
> +}
> +
> +static int tps6594_esm_remove(struct platform_device *pdev)
> +{
> + struct tps6594 *tps = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + ret = regmap_clear_bits(tps->regmap, TPS6594_REG_ESM_SOC_START_REG,
> + TPS6594_BIT_ESM_SOC_START);
> + if (ret) {
> + dev_err(dev, "Failed to stop ESM\n");
> + goto out;
> + }
> +
> + ret = regmap_clear_bits(tps->regmap, TPS6594_REG_ESM_SOC_MODE_CFG,
> + TPS6594_BIT_ESM_SOC_EN | TPS6594_BIT_ESM_SOC_ENDRV);
> + if (ret)
> + dev_err(dev, "Failed to unconfigure ESM\n");
> +
> +out:
> + pm_runtime_put_sync(dev);
> + pm_runtime_disable(dev);
> +
> + return ret;
> +}
> +
> +static int tps6594_esm_suspend(struct device *dev)
> +{
> + struct tps6594 *tps = dev_get_drvdata(dev->parent);
> + int ret;
> +
> + ret = regmap_clear_bits(tps->regmap, TPS6594_REG_ESM_SOC_START_REG,
> + TPS6594_BIT_ESM_SOC_START);
> +
> + pm_runtime_put_sync(dev);
> +
> + return ret;
> +}
> +
> +static int tps6594_esm_resume(struct device *dev)
> +{
> + struct tps6594 *tps = dev_get_drvdata(dev->parent);
> +
> + pm_runtime_get_sync(dev);
> +
> + return regmap_set_bits(tps->regmap, TPS6594_REG_ESM_SOC_START_REG,
> + TPS6594_BIT_ESM_SOC_START);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(tps6594_esm_pm_ops, tps6594_esm_suspend, tps6594_esm_resume);
> +
> +static struct platform_driver tps6594_esm_driver = {
> + .driver = {
> + .name = "tps6594-esm",
> + .pm = pm_sleep_ptr(&tps6594_esm_pm_ops),
> + },
> + .probe = tps6594_esm_probe,
> + .remove = tps6594_esm_remove,
> +};
> +
> +module_platform_driver(tps6594_esm_driver);
> +
> +MODULE_ALIAS("platform:tps6594-esm");
> +MODULE_AUTHOR("Julien Panis <[email protected]>");
> +MODULE_DESCRIPTION("TPS6594 Error Signal Monitor Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.37.3
>

--
Lee Jones [李琼斯]

2023-03-06 16:43:30

by Julien Panis

[permalink] [raw]

Subject: Re: [PATCH v1 2/4] mfd: tps6594: Add driver for TI TPS6594 PMIC

On 3/3/23 16:03, Lee Jones wrote:
> On Thu, 16 Feb 2023, Julien Panis wrote:
>
>> This patch adds support for TPS6594 PMIC MFD core. It provides
>> communication through the I2C and SPI interfaces, and supports
>> protocols with embedded CRC data fields for safety applications.
>>
>> Signed-off-by: Julien Panis <[email protected]>
>> ---

(...)

>> +
>> +static int tps6594_check_crc_mode(struct tps6594 *tps, bool primary_pmic)
>> +{
>> + int ret;
>> +
>> + /*
>> + * Ensure that CRC is enabled.
>> + * Once CRC is enabled, it can't be disabled until next power cycle.
>> + */
>> + tps->use_crc = true;
>> + ret = regmap_test_bits(tps->regmap, TPS6594_REG_SERIAL_IF_CONFIG,
>> + TPS6594_BIT_I2C1_SPI_CRC_EN);
>> + if (ret < 0) {
>> + tps->use_crc = false;
>> + } else if (ret == 0) {
>> + tps->use_crc = false;
> Will this value be used again after you return an error?

No, it is not used any more. I will remove this line in v2.

>
>> + ret = -EIO;
>> + } else {
>> + dev_info(tps->dev, "CRC feature enabled on %s PMIC",
>> + primary_pmic ? "primary" : "secondary");
>> + ret = 0;
> I would consider reversing the logic of the if()s, default to 'false'
> then set 'true' in here before the print.

Do you speak about 'tps->use_crc' value ?
'tps->use_crc' is used in regmap read/write callbacks, so it
must be set 'true' before calling 'regmap_test_bits()' function.
In other words, CRC_EN bit must be read with 'tps->use_crc = true'.

>
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int tps6594_set_crc_feature(struct tps6594 *tps)
>> +{
>> + int ret;
>> +
>> + /* Force PFSM I2C_2 trigger to enable CRC on primary PMIC */
>> + ret = regmap_write_bits(tps->regmap, TPS6594_REG_FSM_I2C_TRIGGERS,
>> + TPS6594_BIT_TRIGGER_I2C(2), TPS6594_BIT_TRIGGER_I2C(2));
>> + if (ret)
>> + return ret;
>> +
>> + /* Wait for PFSM to process trigger */
>> + msleep(20);
> Is this the time specified in the datasheet?

I checked with the customer after your review and the datasheet
specifies 2 ms.
The clock specification is +/-5%. The customer recommends
using 4ms, which is a simple number providing sufficient margin.
As a consequence, I will adjust this delay in v2.

>
>> + return tps6594_check_crc_mode(tps, true);
>> +}
>> +
>> +int tps6594_device_init(struct tps6594 *tps)
>> +{
>> + struct device *dev = tps->dev;
>> + unsigned int prop;
> Since this only has a single use, better to rename it to something specific.
>
>> + unsigned long timeout = msecs_to_jiffies(TPS6594_CRC_SYNC_TIMEOUT_MS);
>> + int n_dev = ARRAY_SIZE(tps6594_cells);
>> + int ret;
>> +
>> + /* Keep PMIC in ACTIVE state */
>> + ret = regmap_set_bits(tps->regmap, TPS6594_REG_FSM_NSLEEP_TRIGGERS,
>> + TPS6594_BIT_NSLEEP1B | TPS6594_BIT_NSLEEP2B);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to set PMIC state\n");
>> +
>> + /*
>> + * CRC mode can be used with I2C or SPI protocols.
>> + * If this mode is specified for primary PMIC, it will also be applied to secondary PMICs
>> + * through SPMI serial interface.
>> + * In this multi-PMIC synchronization scheme, the primary PMIC is the controller device
>> + * on the SPMI bus, and the secondary PMICs are the target devices on the SPMI bus.
>> + */
>> + prop = of_property_read_bool(dev->of_node, "ti,use-crc");

As discussed with Krzysztof for dt-bindings, this 'ti,use-crc'
property will be removed from the device tree, in v2.
Instead, a property will be used to identify the primary PMIC.
Moreover, since using CRC applies either to all the PMICs or
to none of them, it is a global feature. That's why a driver
parameter will be added to enable CRC feature at initialization
(something like a 'enable_crc' bool).

(...)

>> diff --git a/include/linux/mfd/tps6594.h b/include/linux/mfd/tps6594.h
>> new file mode 100644
>> index 000000000000..e2ffd4dc034d
>> --- /dev/null
>> +++ b/include/linux/mfd/tps6594.h
>> @@ -0,0 +1,1018 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Functions to access TPS6594 Power Management IC
>> + *
>> + * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
>> + */
>> +
>> +#ifndef __LINUX_MFD_TPS6594_H
>> +#define __LINUX_MFD_TPS6594_H
>> +
>> +#include <linux/device.h>
>> +#include <linux/regmap.h>
>> +
>> +struct regmap_irq_chip_data;
>> +
>> +/* Chip id list */
>> +#define TPS6594 0
>> +#define TPS6593 1
>> +#define LP8764X 2
> enum?

Yes indeed, I will fix that in v2.

(...)

Your others suggestions will also be implemented in v2.

Thank you Lee for your time and feedback.

Julien