Messages in this thread | | | Subject | Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC | From | Dilip Kota <> | Date | Fri, 23 Aug 2019 17:47:19 +0800 |
| |
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, ®bit, ®off); >> + >> + 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, ®bit, ®off); >> + 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
| |