Date:   Fri, 23 Aug 2019 17:47:19 +0800
From:   Dilip Kota <eswara.kota@...ux.intel.com>
To:     Philipp Zabel <p.zabel@...gutronix.de>, robh@...nel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     cheol.yong.kim@...el.com, chuanhua.lei@...ux.intel.com,
        qi-ming.wu@...el.com
Subject: Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC

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@...ux.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@...el.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

Powered by blists - more mailing lists