| Message ID | 42039170811f798b8edc66bf85166aefe7dbc903.1566531960.git.eswara.kota@linux.intel.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: reset: Add YAML schemas for the Intel Reset controller | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
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. > + > 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. > + 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. > + > +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). > +} > + > +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;". > +} > + > +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
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
On Fri, 2019-08-23 at 17:47 +0800, Dilip Kota wrote: [...] > Thanks for pointing it out. > Reset is not supported on LGM platform. > I will update the reset_device() definition to "return -EOPNOTSUPP" In that case you can just drop intel_reset_device() completely, the core checks whether ops->reset is set before trying to call it. regards Philipp
On Fri, Aug 23, 2019 at 12:28 AM Dilip Kota <eswara.kota@linux.intel.com> wrote: > > Add YAML schemas for the reset controller on Intel > Lightening Mountain (LGM) SoC. > > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > --- > Changes on v2: > Address review comments > Update the compatible property definition > Add description for reset-cells > Add 'additionalProperties: false' property > > .../bindings/reset/intel,syscon-reset.yaml | 53 ++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml > > diff --git a/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml > new file mode 100644 > index 000000000000..3403a967190a > --- /dev/null > +++ b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml > @@ -0,0 +1,53 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/reset/intel,syscon-reset.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Intel Lightening Mountain SoC System Reset Controller > + > +maintainers: > + - Dilip Kota <eswara.kota@linux.intel.com> > + > +properties: > + compatible: > + items: > + - const: intel,rcu-lgm > + - const: syscon > + > + reg: > + description: Reset controller register base address and size > + > + intel,global-reset: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: Global reset register offset and bit offset. > + > + "#reset-cells": > + const: 2 > + description: | > + The 1st cell is the register offset. > + The 2nd cell is the bit offset in the register. > + > +required: > + - compatible > + - reg > + - intel,global-reset > + - "#reset-cells" > + > +additionalProperties: false > + > +examples: > + - | > + rcu0: reset-controller@00000000 { > + compatible = "intel,rcu-lgm", "syscon"; > + reg = <0x000000 0x80000>; > + intel,global-reset = <0x10 30>; > + #reset-cells = <2>; > + }; > + > + pcie_phy0: pciephy@... { > + ... You need to run 'make dt_binding_check' and fix the warnings. The example has to be buildable and it is not. > + /* address offset: 0x10, bit offset: 12 */ > + resets = <&rcu0 0x10 12>; > + ... > + }; > -- > 2.11.0 >
On 8/23/2019 6:09 PM, Philipp Zabel wrote: > On Fri, 2019-08-23 at 17:47 +0800, Dilip Kota wrote: > [...] >> Thanks for pointing it out. >> Reset is not supported on LGM platform. >> I will update the reset_device() definition to "return -EOPNOTSUPP" > In that case you can just drop intel_reset_device() completely, > the core checks whether ops->reset is set before trying to call it. Agree, will do the same. Regards, Dilip > > regards > Philipp
Hi Rob, On 8/23/2019 8:25 PM, Rob Herring wrote: > On Fri, Aug 23, 2019 at 12:28 AM Dilip Kota <eswara.kota@linux.intel.com> wrote: >> Add YAML schemas for the reset controller on Intel >> Lightening Mountain (LGM) SoC. >> >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >> --- >> Changes on v2: >> Address review comments >> Update the compatible property definition >> Add description for reset-cells >> Add 'additionalProperties: false' property >> >> .../bindings/reset/intel,syscon-reset.yaml | 53 ++++++++++++++++++++++ >> 1 file changed, 53 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >> >> diff --git a/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >> new file mode 100644 >> index 000000000000..3403a967190a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >> @@ -0,0 +1,53 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/reset/intel,syscon-reset.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Intel Lightening Mountain SoC System Reset Controller >> + >> +maintainers: >> + - Dilip Kota <eswara.kota@linux.intel.com> >> + >> +properties: >> + compatible: >> + items: >> + - const: intel,rcu-lgm >> + - const: syscon >> + >> + reg: >> + description: Reset controller register base address and size >> + >> + intel,global-reset: >> + $ref: /schemas/types.yaml#/definitions/uint32-array >> + description: Global reset register offset and bit offset. >> + >> + "#reset-cells": >> + const: 2 >> + description: | >> + The 1st cell is the register offset. >> + The 2nd cell is the bit offset in the register. >> + >> +required: >> + - compatible >> + - reg >> + - intel,global-reset >> + - "#reset-cells" >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + rcu0: reset-controller@00000000 { >> + compatible = "intel,rcu-lgm", "syscon"; >> + reg = <0x000000 0x80000>; >> + intel,global-reset = <0x10 30>; >> + #reset-cells = <2>; >> + }; >> + >> + pcie_phy0: pciephy@... { >> + ... > You need to run 'make dt_binding_check' and fix the warnings. The > example has to be buildable and it is not. Sure, i will correct this pcie_phy0 node. But i didn't get any warnings for make dt_binding_check CHKDT Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml DTC Documentation/devicetree/bindings/arm/renesas.example.dt.yaml FATAL ERROR: Unknown output format "yaml" Will DTC report about the example node errors? But, DTC is failing with FATAL_ERROR. I tried it even after installing libyaml and headers in my local directory and export the path, but no luck.(ref: https://lkml.org/lkml/2018/12/3/951) Could you please let me know if i miss anything and help me to proceed further. Regards, Dilip > >> + /* address offset: 0x10, bit offset: 12 */ >> + resets = <&rcu0 0x10 12>; >> + ... >> + }; >> -- >> 2.11.0 >>
On Mon, Aug 26, 2019 at 4:52 AM Dilip Kota <eswara.kota@linux.intel.com> wrote: > > Hi Rob, > > On 8/23/2019 8:25 PM, Rob Herring wrote: > > On Fri, Aug 23, 2019 at 12:28 AM Dilip Kota <eswara.kota@linux.intel.com> wrote: > >> Add YAML schemas for the reset controller on Intel > >> Lightening Mountain (LGM) SoC. > >> > >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > >> --- > >> Changes on v2: > >> Address review comments > >> Update the compatible property definition > >> Add description for reset-cells > >> Add 'additionalProperties: false' property > >> > >> .../bindings/reset/intel,syscon-reset.yaml | 53 ++++++++++++++++++++++ > >> 1 file changed, 53 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml > >> > >> diff --git a/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml > >> new file mode 100644 > >> index 000000000000..3403a967190a > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml > >> @@ -0,0 +1,53 @@ > >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > >> +%YAML 1.2 > >> +--- > >> +$id: http://devicetree.org/schemas/reset/intel,syscon-reset.yaml# > >> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >> + > >> +title: Intel Lightening Mountain SoC System Reset Controller > >> + > >> +maintainers: > >> + - Dilip Kota <eswara.kota@linux.intel.com> > >> + > >> +properties: > >> + compatible: > >> + items: > >> + - const: intel,rcu-lgm > >> + - const: syscon > >> + > >> + reg: > >> + description: Reset controller register base address and size > >> + > >> + intel,global-reset: > >> + $ref: /schemas/types.yaml#/definitions/uint32-array > >> + description: Global reset register offset and bit offset. > >> + > >> + "#reset-cells": > >> + const: 2 > >> + description: | > >> + The 1st cell is the register offset. > >> + The 2nd cell is the bit offset in the register. > >> + > >> +required: > >> + - compatible > >> + - reg > >> + - intel,global-reset > >> + - "#reset-cells" > >> + > >> +additionalProperties: false > >> + > >> +examples: > >> + - | > >> + rcu0: reset-controller@00000000 { > >> + compatible = "intel,rcu-lgm", "syscon"; > >> + reg = <0x000000 0x80000>; > >> + intel,global-reset = <0x10 30>; > >> + #reset-cells = <2>; > >> + }; > >> + > >> + pcie_phy0: pciephy@... { > >> + ... > > You need to run 'make dt_binding_check' and fix the warnings. The > > example has to be buildable and it is not. > > Sure, i will correct this pcie_phy0 node. But i didn't get any warnings > for make dt_binding_check > > CHKDT Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml > DTC Documentation/devicetree/bindings/arm/renesas.example.dt.yaml > FATAL ERROR: Unknown output format "yaml" > > Will DTC report about the example node errors? But, DTC is failing with > FATAL_ERROR. > I tried it even after installing libyaml and headers in my local > directory and export the path, but no luck.(ref: > https://lkml.org/lkml/2018/12/3/951) > Could you please let me know if i miss anything and help me to proceed > further. See Documentation/devicetree/writing-schema.md Rob
Hi Rob, On 8/26/2019 7:23 PM, Rob Herring wrote: > On Mon, Aug 26, 2019 at 4:52 AM Dilip Kota <eswara.kota@linux.intel.com> wrote: >> Hi Rob, >> >> On 8/23/2019 8:25 PM, Rob Herring wrote: >>> On Fri, Aug 23, 2019 at 12:28 AM Dilip Kota <eswara.kota@linux.intel.com> wrote: >>>> Add YAML schemas for the reset controller on Intel >>>> Lightening Mountain (LGM) SoC. >>>> >>>> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >>>> --- >>>> Changes on v2: >>>> Address review comments >>>> Update the compatible property definition >>>> Add description for reset-cells >>>> Add 'additionalProperties: false' property >>>> >>>> .../bindings/reset/intel,syscon-reset.yaml | 53 ++++++++++++++++++++++ >>>> 1 file changed, 53 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >>>> new file mode 100644 >>>> index 000000000000..3403a967190a >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >>>> @@ -0,0 +1,53 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/reset/intel,syscon-reset.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: Intel Lightening Mountain SoC System Reset Controller >>>> + >>>> +maintainers: >>>> + - Dilip Kota <eswara.kota@linux.intel.com> >>>> + >>>> +properties: >>>> + compatible: >>>> + items: >>>> + - const: intel,rcu-lgm >>>> + - const: syscon >>>> + >>>> + reg: >>>> + description: Reset controller register base address and size >>>> + >>>> + intel,global-reset: >>>> + $ref: /schemas/types.yaml#/definitions/uint32-array >>>> + description: Global reset register offset and bit offset. >>>> + >>>> + "#reset-cells": >>>> + const: 2 >>>> + description: | >>>> + The 1st cell is the register offset. >>>> + The 2nd cell is the bit offset in the register. >>>> + >>>> +required: >>>> + - compatible >>>> + - reg >>>> + - intel,global-reset >>>> + - "#reset-cells" >>>> + >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + rcu0: reset-controller@00000000 { >>>> + compatible = "intel,rcu-lgm", "syscon"; >>>> + reg = <0x000000 0x80000>; >>>> + intel,global-reset = <0x10 30>; >>>> + #reset-cells = <2>; >>>> + }; >>>> + >>>> + pcie_phy0: pciephy@... { >>>> + ... >>> You need to run 'make dt_binding_check' and fix the warnings. The >>> example has to be buildable and it is not. >> Sure, i will correct this pcie_phy0 node. But i didn't get any warnings >> for make dt_binding_check >> >> CHKDT Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >> DTC Documentation/devicetree/bindings/arm/renesas.example.dt.yaml >> FATAL ERROR: Unknown output format "yaml" >> >> Will DTC report about the example node errors? But, DTC is failing with >> FATAL_ERROR. >> I tried it even after installing libyaml and headers in my local >> directory and export the path, but no luck.(ref: >> https://lkml.org/lkml/2018/12/3/951) >> Could you please let me know if i miss anything and help me to proceed >> further. > See Documentation/devicetree/writing-schema.md I have followed all the steps mentioned in the document before keeping the mail itself. Does the dtc script looks for libyaml and its header files at any default or specific location? Regards, Dilip > > Rob
Hi Rob, On 8/27/2019 10:04 PM, Dilip Kota wrote: > Hi Rob, > > On 8/26/2019 7:23 PM, Rob Herring wrote: >> On Mon, Aug 26, 2019 at 4:52 AM Dilip Kota >> <eswara.kota@linux.intel.com> wrote: >>> Hi Rob, >>> >>> On 8/23/2019 8:25 PM, Rob Herring wrote: >>>> On Fri, Aug 23, 2019 at 12:28 AM Dilip Kota >>>> <eswara.kota@linux.intel.com> wrote: >>>>> Add YAML schemas for the reset controller on Intel >>>>> Lightening Mountain (LGM) SoC. >>>>> >>>>> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >>>>> --- >>>>> Changes on v2: >>>>> Address review comments >>>>> Update the compatible property definition >>>>> Add description for reset-cells >>>>> Add 'additionalProperties: false' property >>>>> >>>>> .../bindings/reset/intel,syscon-reset.yaml | 53 >>>>> ++++++++++++++++++++++ >>>>> 1 file changed, 53 insertions(+) >>>>> create mode 100644 >>>>> Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >>>>> >>>>> diff --git >>>>> a/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >>>>> b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >>>>> new file mode 100644 >>>>> index 000000000000..3403a967190a >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >>>>> @@ -0,0 +1,53 @@ >>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>> +%YAML 1.2 >>>>> +--- >>>>> +$id: http://devicetree.org/schemas/reset/intel,syscon-reset.yaml# >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>> + >>>>> +title: Intel Lightening Mountain SoC System Reset Controller >>>>> + >>>>> +maintainers: >>>>> + - Dilip Kota <eswara.kota@linux.intel.com> >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + items: >>>>> + - const: intel,rcu-lgm >>>>> + - const: syscon >>>>> + >>>>> + reg: >>>>> + description: Reset controller register base address and size >>>>> + >>>>> + intel,global-reset: >>>>> + $ref: /schemas/types.yaml#/definitions/uint32-array >>>>> + description: Global reset register offset and bit offset. >>>>> + >>>>> + "#reset-cells": >>>>> + const: 2 >>>>> + description: | >>>>> + The 1st cell is the register offset. >>>>> + The 2nd cell is the bit offset in the register. >>>>> + >>>>> +required: >>>>> + - compatible >>>>> + - reg >>>>> + - intel,global-reset >>>>> + - "#reset-cells" >>>>> + >>>>> +additionalProperties: false >>>>> + >>>>> +examples: >>>>> + - | >>>>> + rcu0: reset-controller@00000000 { >>>>> + compatible = "intel,rcu-lgm", "syscon"; >>>>> + reg = <0x000000 0x80000>; >>>>> + intel,global-reset = <0x10 30>; >>>>> + #reset-cells = <2>; >>>>> + }; >>>>> + >>>>> + pcie_phy0: pciephy@... { >>>>> + ... >>>> You need to run 'make dt_binding_check' and fix the warnings. The >>>> example has to be buildable and it is not. >>> Sure, i will correct this pcie_phy0 node. But i didn't get any >>> warnings >>> for make dt_binding_check >>> >>> CHKDT >>> Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml >>> DTC Documentation/devicetree/bindings/arm/renesas.example.dt.yaml >>> FATAL ERROR: Unknown output format "yaml" >>> >>> Will DTC report about the example node errors? But, DTC is failing with >>> FATAL_ERROR. >>> I tried it even after installing libyaml and headers in my local >>> directory and export the path, but no luck.(ref: >>> https://lkml.org/lkml/2018/12/3/951) >>> Could you please let me know if i miss anything and help me to proceed >>> further. >> See Documentation/devicetree/writing-schema.md > > I have followed all the steps mentioned in the document before keeping > the mail itself. > Does the dtc script looks for libyaml and its header files at any > default or specific location? DTC is working for me now. It is working for me after updating the libyaml and header paths in scripts/dtc/Makefile and yamltree.c (since i have installed libyaml and header files in my local directories) I have fixed all the warnings and DTC checks are successful. I will push the changes in the next patch version. DTC Documentation/devicetree/bindings/reset/intel,syscon-reset.example.dt.yaml CHECK Documentation/devicetree/bindings/reset/intel,syscon-reset.example.dt.yaml Regards, Dilip
diff --git a/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml new file mode 100644 index 000000000000..3403a967190a --- /dev/null +++ b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/reset/intel,syscon-reset.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel Lightening Mountain SoC System Reset Controller + +maintainers: + - Dilip Kota <eswara.kota@linux.intel.com> + +properties: + compatible: + items: + - const: intel,rcu-lgm + - const: syscon + + reg: + description: Reset controller register base address and size + + intel,global-reset: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: Global reset register offset and bit offset. + + "#reset-cells": + const: 2 + description: | + The 1st cell is the register offset. + The 2nd cell is the bit offset in the register. + +required: + - compatible + - reg + - intel,global-reset + - "#reset-cells" + +additionalProperties: false + +examples: + - | + rcu0: reset-controller@00000000 { + compatible = "intel,rcu-lgm", "syscon"; + reg = <0x000000 0x80000>; + intel,global-reset = <0x10 30>; + #reset-cells = <2>; + }; + + pcie_phy0: pciephy@... { + ... + /* address offset: 0x10, bit offset: 12 */ + resets = <&rcu0 0x10 12>; + ... + };
Add YAML schemas for the reset controller on Intel Lightening Mountain (LGM) SoC. Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> --- Changes on v2: Address review comments Update the compatible property definition Add description for reset-cells Add 'additionalProperties: false' property .../bindings/reset/intel,syscon-reset.yaml | 53 ++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml