Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC

Dilip Kota
2019-08-23T09:47:25Z

Hi Philipp,

On 8/23/2019 4:43 PM, Philipp Zabel wrote:
> Hi Dilip,
>
> On Fri, 2019-08-23 at 13:28 +0800, Dilip Kota wrote:
>> Add driver for the reset controller present on Intel
>> Lightening Mountain (LGM) SoC for performing reset
>> management of the devices present on the SoC. Driver also
>> registers a reset handler to peform the entire device reset.
>>
>> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
> thank you for the patch, I have a few questions/suggestions below:
>
>> ---
>> Changes on v2:
>> No changes
>>
>> drivers/reset/Kconfig | 10 ++
>> drivers/reset/Makefile | 1 +
>> drivers/reset/reset-intel-syscon.c | 215 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 226 insertions(+)
>> create mode 100644 drivers/reset/reset-intel-syscon.c
>>
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index 6d5d76db55b0..e0fd14cb4cf5 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -64,6 +64,16 @@ config RESET_IMX7
>> help
>> This enables the reset controller driver for i.MX7 SoCs.
>>
>> +config RESET_INTEL_SYSCON
>> + bool "Intel SYSCON Reset Driver"
>> + depends on HAS_IOMEM
>> + select MFD_SYSCON
>> + help
>> + This enables the reset driver support for Intel SoC devices with
>> + memory-mapped reset registers as part of a syscon device node. If
>> + you wish to use the reset framework for such memory-mapped devices,
>> + say Y here. Otherwise, say N.
> Is this driver really as generic as this description makes it sound,
> or is it limited to LGM?
>
> Do you expect this to be reused by other platforms? The timeouts,
> status register offsets, and readback mechanism might be platform
> specific.
Yes you are correct, this is platform specific limited to LGM.
I will update the config description.
>
>> +
>> config RESET_LANTIQ
>> bool "Lantiq XWAY Reset Driver" if COMPILE_TEST
>> default SOC_TYPE_XWAY
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index 61456b8f659c..6d68c50c7e89 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -10,6 +10,7 @@ obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
>> obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
>> obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
>> obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
>> +obj-$(CONFIG_RESET_INTEL_SYSCON) += reset-intel-syscon.o
>> obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
>> obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
>> obj-$(CONFIG_RESET_MESON) += reset-meson.o
>> diff --git a/drivers/reset/reset-intel-syscon.c b/drivers/reset/reset-intel-syscon.c
>> new file mode 100644
>> index 000000000000..6377a0cac1e7
>> --- /dev/null
>> +++ b/drivers/reset/reset-intel-syscon.c
>> @@ -0,0 +1,215 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2019 Intel Corporation.
>> + * Lei Chuanhua <Chuanhua.lei@intel.com>
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/io.h>
>> +#include <linux/init.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset-controller.h>
>> +
>> +struct intel_reset_data {
>> + struct reset_controller_dev rcdev;
>> + struct notifier_block restart_nb;
>> + struct device *dev;
>> + struct regmap *regmap;
>> + u32 reboot_id;
>> +};
>> +
>> +/* reset platform data */
>> +#define to_reset_data(x) container_of(x, struct intel_reset_data, rcdev)
>> +
>> +/*
>> + * Reset status register offset relative to
>> + * the reset control register(X) is X + 4
>> + */
>> +static inline u32 id_to_reg_bit_and_offset(unsigned long id,
>> + u32 *regbit, u32 *regoff)
>> +{
>> + *regoff = id >> 8;
>> + *regbit = id & 0x1f;
>> + return *regoff + 0x4;
>> +}
>> +
>> +static int intel_set_clr_bits(struct intel_reset_data *data,
>> + unsigned long id, bool set, u64 timeout)
>> +{
>> + u32 regoff, regbit;
>> + u32 stat_off;
>> + u32 val;
>> + int ret;
>> +
>> + stat_off = id_to_reg_bit_and_offset(id, &regbit, &regoff);
>> +
>> + val = set ? BIT(regbit) : 0;
>> + ret = regmap_update_bits(data->regmap, regoff, BIT(regbit), val);
>> + if (ret)
>> + return ret;
>> +
>> + return regmap_read_poll_timeout(data->regmap, stat_off, val,
>> + set == !!(val & BIT(regbit)),
>> + 20, timeout);
>> +}
>> +
>> +static int intel_assert_device(struct reset_controller_dev *rcdev,
>> + unsigned long id)
>> +{
>> + struct intel_reset_data *data = to_reset_data(rcdev);
>> + int ret;
>> +
>> + ret = intel_set_clr_bits(data, id, 1, 200);
> Since set is of type bool, I'd use true instead of 1.
Agree, will update it to true/false at all the places.
Good Catch!
>
>> + if (ret)
>> + dev_err(data->dev, "Failed to set reset assert bit %d\n", ret);
>> + return ret;
>> +}
>> +
> [...]
>> +
>> +static int intel_reset_device(struct reset_controller_dev *rcdev,
>> + unsigned long id)
>> +{
>> + struct intel_reset_data *data = to_reset_data(rcdev);
>> + int ret;
>> +
>> + ret = intel_set_clr_bits(data, id, 1, 20000);
>> + if (ret)
>> + dev_err(data->dev, "Failed to reset device %d\n", ret);
>> + return ret;
>> +}
> This doesn't seem right. _assert and _reset are doing exactly the same
> thing, except for allowing a different timeout. Depending on whether any
> individual reset control bit is a (possibly self-clearing) trigger or
> directly controls the reset signal, either one or the other doesn't do
> the expected thing.
>
> Do you have self-clearing and direct-control reset bits mixed in the
> same register space? If so, _reset should either return -EOPNOTSUPP for
> the direct-control bits or implement an assert-delay-deassert sequence.
> _assert and _deassert should return -EOPNOTSUPP for self-clearing reset
> bits.

Thanks for pointing it out.
Reset is not supported on LGM platform.
I will update the reset_device() definition to "return -EOPNOTSUPP"

>
>> +
>> +static int intel_reset_status(struct reset_controller_dev *rcdev,
>> + unsigned long id)
>> +{
>> + struct intel_reset_data *data = to_reset_data(rcdev);
>> + u32 regoff, regbit;
>> + u32 stat_off;
>> + u32 val;
>> + int ret;
>> +
>> + stat_off = id_to_reg_bit_and_offset(id, &regbit, &regoff);
>> + ret = regmap_read(data->regmap, stat_off, &val);
>> + if (ret)
>> + return ret;
>> +
>> + return !!(val & BIT(regbit));
>> +}
>> +
>> +static const struct reset_control_ops intel_reset_ops = {
>> + .reset = intel_reset_device,
>> + .assert = intel_assert_device,
>> + .deassert = intel_deassert_device,
>> + .status = intel_reset_status,
>> +};
>> +
>> +static int intel_reset_xlate(struct reset_controller_dev *rcdev,
>> + const struct of_phandle_args *spec)
>> +{
>> + u32 offset, bit;
>> +
>> + offset = spec->args[0];
>> + bit = spec->args[1];
>> +
>> + return (offset << 8) | (bit & 0x1f);
> Instead of wrapping around for invalid bit offsets, better return
> -EINVAL if (offset >= rcdev->nr_resets || bit > 31).
Agree with you. I will update it.
>
>> +}
>> +
>> +static int intel_reset_restart_handler(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct intel_reset_data *reset_data =
>> + container_of(nb, struct intel_reset_data, restart_nb);
>> +
>> + intel_assert_device(&reset_data->rcdev, reset_data->reboot_id);
>> +
>> + return NOTIFY_DONE;
>> +}
>> +
>> +static int intel_reset_probe(struct platform_device *pdev)
>> +{
>> + int ret;
>> + struct device_node *np = pdev->dev.of_node;
>> + struct device *dev = &pdev->dev;
>> + struct intel_reset_data *data;
>> + struct regmap *regmap;
>> + u32 rb_id[2];
>> +
>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + regmap = syscon_node_to_regmap(np);
>> + if (IS_ERR(regmap)) {
>> + dev_err(dev, "Failed to get reset controller regmap\n");
>> + return PTR_ERR(regmap);
>> + }
>> +
>> + ret = device_property_read_u32_array(dev, "intel,global-reset",
>> + rb_id, ARRAY_SIZE(rb_id));
>> + if (ret) {
>> + dev_err(dev, "Failed to get global reset offset!\n");
>> + return ret;
>> + }
>> +
>> + data->dev = dev;
>> + data->reboot_id = (rb_id[0] << 8) | rb_id[1];
>> + data->regmap = regmap;
>> + data->rcdev.of_node = np;
>> + data->rcdev.owner = dev->driver->owner;
>> + data->rcdev.ops = &intel_reset_ops;
>> + data->rcdev.of_xlate = intel_reset_xlate;
>> + data->rcdev.of_reset_n_cells = 2;
>> +
>> + ret = devm_reset_controller_register(&pdev->dev, &data->rcdev);
>> + if (ret)
>> + return ret;
>> +
>> + data->restart_nb.notifier_call = intel_reset_restart_handler;
>> + data->restart_nb.priority = 128;
>> +
>> + register_restart_handler(&data->restart_nb);
>> +
>> + return ret;
> Could be "return 0;".
Agree, will correct it.
>
>> +}
>> +
>> +static const struct of_device_id intel_reset_match[] = {
>> + { .compatible = "intel,rcu-lgm" },
>> + {}
>> +};
>> +
>> +static struct platform_driver intel_reset_driver = {
>> + .probe = intel_reset_probe,
>> + .driver = {
>> + .name = "intel-reset-syscon",
>> + .of_match_table = intel_reset_match,
>> + },
>> +};
>> +
>> +static int __init intel_reset_init(void)
>> +{
>> + return platform_driver_register(&intel_reset_driver);
>> +}
>> +
>> +/*
>> + * RCU is system core entity which is in Always On Domain whose clocks
>> + * or resource initialization happens in system core initialization.
>> + * Also, it is required for most of the platform or architecture
>> + * specific devices to perform reset operation as part of initialization.
>> + * So perform RCU as post core initialization.
>> + */
>> +postcore_initcall(intel_reset_init);
> regards
> Philipp

Thanks for the review comments!

Regards,
Dilip