[PATCH v2 1/2] dt-bindings: reset: Add YAML schemas for the Intel Reset controller

[PATCH v2 1/2] dt-bindings: reset: Add YAML schemas for the Intel Reset controller

[Dilip Kota]

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@... {
+        ...
+        /* address offset: 0x10, bit offset: 12 */
+        resets = <&rcu0 0x10 12>;
+        ...
+    };
-- 
2.11.0

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

[Dilip Kota]

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>
---
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.
+
 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);
+	if (ret)
+		dev_err(data->dev, "Failed to set reset assert bit %d\n", ret);
+	return ret;
+}
+
+static int intel_deassert_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, 0, 200);
+	if (ret)
+		dev_err(data->dev,
+			"Failed to set reset deassert 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;
+}
+
+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);
+}
+
+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;
+}
+
+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);
-- 
2.11.0

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

[Philipp Zabel]

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, &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.

> +	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, &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).

> +}
> +
> +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

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

[Dilip Kota]

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

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

[Philipp Zabel]

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

Re: [PATCH v2 1/2] dt-bindings: reset: Add YAML schemas for the Intel Reset controller

[Rob Herring]

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
>

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

[Martin Blumenstingl]

Hi Dilip,

> 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.

[...]
> +static const struct of_device_id intel_reset_match[] = {
> +	{ .compatible = "intel,rcu-lgm" },
> +	{}
> +};
how is this IP block differnet from the one used in many Lantiq SoCs?
there is already an upstream driver for the RCU IP block on the Lantiq
SoCs: drivers/reset/reset-lantiq.c

some background:
Lantiq was started as a spinoff from Infineon in 2009. Intel then
acquired Lantiq in 2015. source: [0]
Intel is re-using some of the IP blocks from the MIPS Lantiq SoCs
(Intel even has some own MIPS SoCs as part of the Lantiq acquisition,
typically used for PON/GPON/ADSL/VDSL capable network devices).
Thus I think it is likely that the new "Lightening Mountain" SoCs use
an updated version of the Lantiq RCU IP.


Martin


[0] https://wikidevi.com/wiki/Lantiq

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

[Chuan Hua, Lei]

Hi Martin,

Thanks for your comment.

On 8/25/2019 5:11 AM, Martin Blumenstingl wrote:
> Hi Dilip,
>
>> 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.
> [...]
>> +static const struct of_device_id intel_reset_match[] = {
>> +	{ .compatible = "intel,rcu-lgm" },
>> +	{}
>> +};
> how is this IP block differnet from the one used in many Lantiq SoCs?
> there is already an upstream driver for the RCU IP block on the Lantiq
> SoCs: drivers/reset/reset-lantiq.c
>
> some background:
> Lantiq was started as a spinoff from Infineon in 2009. Intel then
> acquired Lantiq in 2015. source: [0]
> Intel is re-using some of the IP blocks from the MIPS Lantiq SoCs
> (Intel even has some own MIPS SoCs as part of the Lantiq acquisition,
> typically used for PON/GPON/ADSL/VDSL capable network devices).
> Thus I think it is likely that the new "Lightening Mountain" SoCs use
> an updated version of the Lantiq RCU IP.

I would not say there is a fundamental difference since reset is a 
really simple

stuff from all reset drivers.  However, it did have some difference

from existing reset-lantiq.c since SoC becomes more and more complex.

1. reset-lantiq.c use index instead of register offset + bit position.

index reset is good for a small system (< 64). However, it will become very

difficult to use if you have  > 100 reset. So we use register offset + 
bit position

2. reset-lantiq.c does not support device restart which is part of the 
reset in

old lantiq SoC. It moved this part into arch/mips/lantiq directory.

3. reset-lantiqc reset callback doesn't implement what hardware implemented

function. In old SoCs, some bits in the same register can be hardware 
reset clear.

It just call assert + assert. For these SoCs, we should only call 
assert, hardware

will auto deassert.

4. Code not optimized and intel internal review not assessed.

Based on the above findings, I would suggest reset-lantiq.c to move to 
reset-intel-syscon.c

What is your opinion?


Chuanhua

>
>
> Martin
>
>
> [0] https://wikidevi.com/wiki/Lantiq

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

[Dilip Kota]

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

Re: [PATCH v2 1/2] dt-bindings: reset: Add YAML schemas for the Intel Reset controller

[Dilip Kota]

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
>>

Re: [PATCH v2 1/2] dt-bindings: reset: Add YAML schemas for the Intel Reset controller

[Rob Herring]

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

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

[Martin Blumenstingl]

Hi,

On Mon, Aug 26, 2019 at 6:01 AM Chuan Hua, Lei
<chuanhua.lei@linux.intel.com> wrote:
>
> Hi Martin,
>
> Thanks for your comment.
thank you for the quick reply

> On 8/25/2019 5:11 AM, Martin Blumenstingl wrote:
> > Hi Dilip,
> >
> >> 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.
> > [...]
> >> +static const struct of_device_id intel_reset_match[] = {
> >> +    { .compatible = "intel,rcu-lgm" },
> >> +    {}
> >> +};
> > how is this IP block differnet from the one used in many Lantiq SoCs?
> > there is already an upstream driver for the RCU IP block on the Lantiq
> > SoCs: drivers/reset/reset-lantiq.c
> >
> > some background:
> > Lantiq was started as a spinoff from Infineon in 2009. Intel then
> > acquired Lantiq in 2015. source: [0]
> > Intel is re-using some of the IP blocks from the MIPS Lantiq SoCs
> > (Intel even has some own MIPS SoCs as part of the Lantiq acquisition,
> > typically used for PON/GPON/ADSL/VDSL capable network devices).
> > Thus I think it is likely that the new "Lightening Mountain" SoCs use
> > an updated version of the Lantiq RCU IP.
>
> I would not say there is a fundamental difference since reset is a
> really simple
> stuff from all reset drivers.  However, it did have some difference
> from existing reset-lantiq.c since SoC becomes more and more complex.
OK, let me go through your detailed list

> 1. reset-lantiq.c use index instead of register offset + bit position.
> index reset is good for a small system (< 64). However, it will become very
> difficult to use if you have  > 100 reset. So we use register offset +
> bit position
reset-lantiq uses bit bit positions for specifying the reset line.
for example this is from OpenWrt's vr9.dtsi:
  reset0: reset-controller@10 {
    ...
    reg = <0x10 4>, <0x14 4>;
    #reset-cells = <2>;
  };

  gphy0: gphy@20 {
    ...
    resets = <&reset0 31 30>, <&reset1 7 7>;
    reset-names = "gphy", "gphy2";
  };

in my own words this means:
- all reset0 reset bits are at offset 0x10 (parent is RCU)
- all reset0 status bits are at offset 0x14 (parent is RCU)
- the first reset line uses reset bit 31 and status bit 30
- the second reset line uses reset bit 7 and status bit 7
- there can be multiple reset-controller instances, each taking the
reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
reset controller "reset1" with reset offset 0x48 and status offset
0x24)

> 2. reset-lantiq.c does not support device restart which is part of the
> reset in
> old lantiq SoC. It moved this part into arch/mips/lantiq directory.
it was moved to the .dts instead of the arch code. again from
OpenWrt's vr9.dtsi [0]:
  reboot {
    compatible = "syscon-reboot";
    regmap = <&rcu0>;
    offset = <0x10>;
    mask = <0xe0000000>;
  };

this sets the reset0 reset bits 31, 30 and 29 at reboot

> 3. reset-lantiqc reset callback doesn't implement what hardware implemented
> function. In old SoCs, some bits in the same register can be hardware
> reset clear.
>
> It just call assert + assert. For these SoCs, we should only call
> assert, hardware will auto deassert.
I didn't know that. so to confirm I understand this correctly:
- some reset lines must be asserted and de-asserted manually (setting
and clearing the bit manually). the _assert and _deassert functions
should be used in this case
- other reset lines only support reset pulses. the _reset function
should be used in this case
- the _reset function should only assert the reset line, then wait
until the hardware automatically de-asserts it (without any further
write)

is this the same for all, old and new SoCs?

only two mainline Lantiq drivers are using reset lines - both are
using the _assert and _deassert variants:
- drivers/net/dsa/lantiq_gswip.c
- drivers/phy/lantiq/phy-lantiq-rcu-usb2.c

> 4. Code not optimized and intel internal review not assessed.
insights from you (like the issue with the reset callback) are very
valuable - this shows that we should focus on having one driver.

> Based on the above findings, I would suggest reset-lantiq.c to move to
> reset-intel-syscon.c
my concern with having two separate drivers is that it will be hard to
migrate from reset-lantiq to the "optimized" reset-intel-syscon
driver.
I don't have access to the datasheets for the any Lantiq/Intel SoC
(VRX200 and even older).
so debugging issues after switching from one driver to another is
tedious because I cannot tell which part of the driver is causing a
problem (it's either "all code from driver A" vs "all code from driver
B", meaning it's hard to narrow it down).
with separate commits/patches that are improving the reset-lantiq
driver I can do git bisect to find the cause of a problem on the older
SoCs (VRX200 for example)

> What is your opinion?
I explained why I would like to avoid having two separate drivers
(even just for a limited amount of time)


Martin


[0] https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/lantiq/files/arch/mips/boot/dts/vr9.dtsi;h=e8b87dbcc7de2fb928a4e602f1a650030d2f7c35;hb=c3a78955f34a61d402044f357f54f21c75a19ff9#l103

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

[Chuan Hua, Lei]

Hi Martin,

Please check the reply below.

On 8/27/2019 5:49 AM, Martin Blumenstingl wrote:
> Hi,
>
> On Mon, Aug 26, 2019 at 6:01 AM Chuan Hua, Lei
> <chuanhua.lei@linux.intel.com> wrote:
>> Hi Martin,
>>
>> Thanks for your comment.
> thank you for the quick reply
>
>> On 8/25/2019 5:11 AM, Martin Blumenstingl wrote:
>>> Hi Dilip,
>>>
>>>> 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.
>>> [...]
>>>> +static const struct of_device_id intel_reset_match[] = {
>>>> +    { .compatible = "intel,rcu-lgm" },
>>>> +    {}
>>>> +};
>>> how is this IP block differnet from the one used in many Lantiq SoCs?
>>> there is already an upstream driver for the RCU IP block on the Lantiq
>>> SoCs: drivers/reset/reset-lantiq.c
>>>
>>> some background:
>>> Lantiq was started as a spinoff from Infineon in 2009. Intel then
>>> acquired Lantiq in 2015. source: [0]
>>> Intel is re-using some of the IP blocks from the MIPS Lantiq SoCs
>>> (Intel even has some own MIPS SoCs as part of the Lantiq acquisition,
>>> typically used for PON/GPON/ADSL/VDSL capable network devices).
>>> Thus I think it is likely that the new "Lightening Mountain" SoCs use
>>> an updated version of the Lantiq RCU IP.
>> I would not say there is a fundamental difference since reset is a
>> really simple
>> stuff from all reset drivers.  However, it did have some difference
>> from existing reset-lantiq.c since SoC becomes more and more complex.
> OK, let me go through your detailed list
>
>> 1. reset-lantiq.c use index instead of register offset + bit position.
>> index reset is good for a small system (< 64). However, it will become very
>> difficult to use if you have  > 100 reset. So we use register offset +
>> bit position
> reset-lantiq uses bit bit positions for specifying the reset line.
> for example this is from OpenWrt's vr9.dtsi:
>    reset0: reset-controller@10 {
>      ...
>      reg = <0x10 4>, <0x14 4>;
>      #reset-cells = <2>;
>    };
>
>    gphy0: gphy@20 {
>      ...
>      resets = <&reset0 31 30>, <&reset1 7 7>;
>      reset-names = "gphy", "gphy2";
>    };
>
> in my own words this means:
> - all reset0 reset bits are at offset 0x10 (parent is RCU)
> - all reset0 status bits are at offset 0x14 (parent is RCU)
> - the first reset line uses reset bit 31 and status bit 30
> - the second reset line uses reset bit 7 and status bit 7
> - there can be multiple reset-controller instances, each taking the
> reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
> reset controller "reset1" with reset offset 0x48 and status offset
> 0x24)

in reset-lantiq.c, we split each reset request /status pair into one 
reset controller.

Each reset controller handles up to 32 resets. It will create up to 9 
even more

reset controllers in the new SoCs. In reality, there is only one RCU 
controller for all

SoCs. These designs worked but did not follow what hardware implemented.

After checking the existing code and referring to other implementation, 
we decided to

use register offset + bit position method. It can support all SoCs with 
this methods

without code change(device tree change only).

>
>> 2. reset-lantiq.c does not support device restart which is part of the
>> reset in
>> old lantiq SoC. It moved this part into arch/mips/lantiq directory.
> it was moved to the .dts instead of the arch code. again from
> OpenWrt's vr9.dtsi [0]:
>    reboot {
>      compatible = "syscon-reboot";
>      regmap = <&rcu0>;
>      offset = <0x10>;
>      mask = <0xe0000000>;
>    };
>
> this sets the reset0 reset bits 31, 30 and 29 at reboot
ok. but not sure why we need to reset bit 31 and 29. global softwre 
reset is bit 30.
>
>> 3. reset-lantiqc reset callback doesn't implement what hardware implemented
>> function. In old SoCs, some bits in the same register can be hardware
>> reset clear.
>>
>> It just call assert + assert. For these SoCs, we should only call
>> assert, hardware will auto deassert.
> I didn't know that. so to confirm I understand this correctly:
> - some reset lines must be asserted and de-asserted manually (setting
> and clearing the bit manually). the _assert and _deassert functions
> should be used in this case

Yes. We call software managed reset. callers call assert, deassert and delay

duration according to their specific requirement.

> - other reset lines only support reset pulses. the _reset function
> should be used in this case
> - the _reset function should only assert the reset line, then wait
> until the hardware automatically de-asserts it (without any further
> write)
Yes, this is called hardware reset. We can't control reset duration.
> is this the same for all, old and new SoCs?

New SoCs have removed support for hardware reset after software's feedback.

Old SoCs such as VRX200/ARX300 has both software/hardware resets

>
> only two mainline Lantiq drivers are using reset lines - both are
> using the _assert and _deassert variants:
> - drivers/net/dsa/lantiq_gswip.c
> - drivers/phy/lantiq/phy-lantiq-rcu-usb2.c
It means migration will be very easy:)
>
>> 4. Code not optimized and intel internal review not assessed.
> insights from you (like the issue with the reset callback) are very
> valuable - this shows that we should focus on having one driver.
>
>> Based on the above findings, I would suggest reset-lantiq.c to move to
>> reset-intel-syscon.c
> my concern with having two separate drivers is that it will be hard to
> migrate from reset-lantiq to the "optimized" reset-intel-syscon
> driver.
> I don't have access to the datasheets for the any Lantiq/Intel SoC
> (VRX200 and even older).
> so debugging issues after switching from one driver to another is
> tedious because I cannot tell which part of the driver is causing a
> problem (it's either "all code from driver A" vs "all code from driver
> B", meaning it's hard to narrow it down).
> with separate commits/patches that are improving the reset-lantiq
> driver I can do git bisect to find the cause of a problem on the older
> SoCs (VRX200 for example)

Our internal version supports XRX350/XRX500/PRX300(MIPS based) and

latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c

should be straight forward.

>
>> What is your opinion?
> I explained why I would like to avoid having two separate drivers
> (even just for a limited amount of time)
>
>
> Martin
>
>
> [0] https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/lantiq/files/arch/mips/boot/dts/vr9.dtsi;h=e8b87dbcc7de2fb928a4e602f1a650030d2f7c35;hb=c3a78955f34a61d402044f357f54f21c75a19ff9#l103

Re: [PATCH v2 1/2] dt-bindings: reset: Add YAML schemas for the Intel Reset controller

[Dilip Kota]

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

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

[Martin Blumenstingl]

Hi,

On Tue, Aug 27, 2019 at 4:23 AM Chuan Hua, Lei
<chuanhua.lei@linux.intel.com> wrote:
[...]
> >> 1. reset-lantiq.c use index instead of register offset + bit position.
> >> index reset is good for a small system (< 64). However, it will become very
> >> difficult to use if you have  > 100 reset. So we use register offset +
> >> bit position
> > reset-lantiq uses bit bit positions for specifying the reset line.
> > for example this is from OpenWrt's vr9.dtsi:
> >    reset0: reset-controller@10 {
> >      ...
> >      reg = <0x10 4>, <0x14 4>;
> >      #reset-cells = <2>;
> >    };
> >
> >    gphy0: gphy@20 {
> >      ...
> >      resets = <&reset0 31 30>, <&reset1 7 7>;
> >      reset-names = "gphy", "gphy2";
> >    };
> >
> > in my own words this means:
> > - all reset0 reset bits are at offset 0x10 (parent is RCU)
> > - all reset0 status bits are at offset 0x14 (parent is RCU)
> > - the first reset line uses reset bit 31 and status bit 30
> > - the second reset line uses reset bit 7 and status bit 7
> > - there can be multiple reset-controller instances, each taking the
> > reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
> > reset controller "reset1" with reset offset 0x48 and status offset
> > 0x24)
>
> in reset-lantiq.c, we split each reset request /status pair into one
> reset controller.
>
> Each reset controller handles up to 32 resets. It will create up to 9
> even more
> reset controllers in the new SoCs. In reality, there is only one RCU
> controller for all
> SoCs. These designs worked but did not follow what hardware implemented.
>
> After checking the existing code and referring to other implementation,
> we decided to
> use register offset + bit position method. It can support all SoCs with
> this methods
> without code change(device tree change only).
maybe I have a different interpretation of what "RCU" does.
let me explain it in my own words based on my knowledge about VRX200:
- in my own words it is a multi function device with the following
functionality:
- it contains two reset controllers (reset at 0x10, status 0x14 and
reset at 0x48, status at 0x24)
- it contains two USB2 PHYs (PHY registers at 0x18, ANA cfg at 0x38
and PHY registers at 0x34, ANA cfg at 0x3c)
- it contains the configuration for the two GPHY IP blocks (at 0x20 and 0x68)
- it contains endianness configuration registers (for PCI, PCIe, ...)
- it contains the watchdog boot status (whether the SoC was previously
reset by the WDT)
- maybe more, but I don't know anything else about it

we tried our best to document this in
Documentation/devicetree/bindings/mips/lantiq/rcu.txt

I'm not sure about the details of the RCU on the LGM SoCs:
if it contains more than just reset controllers then please let Rob
Herring (dt-bindings maintainer) know about this.
we may only have one chance to do it right, if we start with a
"broken" binding then devices with incompatible bootloaders etc. may
have already shipped
(in general: that is why the devicetree maintainers want to have all
device properties documented in the binding, even if the driver does
not support all of them yet)

> >> 2. reset-lantiq.c does not support device restart which is part of the
> >> reset in
> >> old lantiq SoC. It moved this part into arch/mips/lantiq directory.
> > it was moved to the .dts instead of the arch code. again from
> > OpenWrt's vr9.dtsi [0]:
> >    reboot {
> >      compatible = "syscon-reboot";
> >      regmap = <&rcu0>;
> >      offset = <0x10>;
> >      mask = <0xe0000000>;
> >    };
> >
> > this sets the reset0 reset bits 31, 30 and 29 at reboot
> ok. but not sure why we need to reset bit 31 and 29. global softwre
> reset is bit 30.
I don't know either. depending on what the LGM SoCs need you can
change the "mask" property to the value that fits that SoC best

[...]
> > - other reset lines only support reset pulses. the _reset function
> > should be used in this case
> > - the _reset function should only assert the reset line, then wait
> > until the hardware automatically de-asserts it (without any further
> > write)
> Yes, this is called hardware reset. We can't control reset duration.
> > is this the same for all, old and new SoCs?
>
> New SoCs have removed support for hardware reset after software's feedback.
>
> Old SoCs such as VRX200/ARX300 has both software/hardware resets
nice, it's good to see teamwork between hardware and software teams!

[...]
> >> 4. Code not optimized and intel internal review not assessed.
> > insights from you (like the issue with the reset callback) are very
> > valuable - this shows that we should focus on having one driver.
> >
> >> Based on the above findings, I would suggest reset-lantiq.c to move to
> >> reset-intel-syscon.c
> > my concern with having two separate drivers is that it will be hard to
> > migrate from reset-lantiq to the "optimized" reset-intel-syscon
> > driver.
> > I don't have access to the datasheets for the any Lantiq/Intel SoC
> > (VRX200 and even older).
> > so debugging issues after switching from one driver to another is
> > tedious because I cannot tell which part of the driver is causing a
> > problem (it's either "all code from driver A" vs "all code from driver
> > B", meaning it's hard to narrow it down).
> > with separate commits/patches that are improving the reset-lantiq
> > driver I can do git bisect to find the cause of a problem on the older
> > SoCs (VRX200 for example)
>
> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
> should be straight forward.
what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
they only use level resets (_assert and _deassert) or are some reset
lines using reset pulses (_reset)?

when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
we still had to add support for the _reset callback as this is missing
in reset-intel-syscon.c currently


Martin

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

[Chuan Hua, Lei]

Hi Martin,

On 8/28/2019 5:15 AM, Martin Blumenstingl wrote:
> Hi,
>
> On Tue, Aug 27, 2019 at 4:23 AM Chuan Hua, Lei
> <chuanhua.lei@linux.intel.com> wrote:
> [...]
>>>> 1. reset-lantiq.c use index instead of register offset + bit position.
>>>> index reset is good for a small system (< 64). However, it will become very
>>>> difficult to use if you have  > 100 reset. So we use register offset +
>>>> bit position
>>> reset-lantiq uses bit bit positions for specifying the reset line.
>>> for example this is from OpenWrt's vr9.dtsi:
>>>     reset0: reset-controller@10 {
>>>       ...
>>>       reg = <0x10 4>, <0x14 4>;
>>>       #reset-cells = <2>;
>>>     };
>>>
>>>     gphy0: gphy@20 {
>>>       ...
>>>       resets = <&reset0 31 30>, <&reset1 7 7>;
>>>       reset-names = "gphy", "gphy2";
>>>     };
>>>
>>> in my own words this means:
>>> - all reset0 reset bits are at offset 0x10 (parent is RCU)
>>> - all reset0 status bits are at offset 0x14 (parent is RCU)
>>> - the first reset line uses reset bit 31 and status bit 30
>>> - the second reset line uses reset bit 7 and status bit 7
>>> - there can be multiple reset-controller instances, each taking the
>>> reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
>>> reset controller "reset1" with reset offset 0x48 and status offset
>>> 0x24)
>> in reset-lantiq.c, we split each reset request /status pair into one
>> reset controller.
>>
>> Each reset controller handles up to 32 resets. It will create up to 9
>> even more
>> reset controllers in the new SoCs. In reality, there is only one RCU
>> controller for all
>> SoCs. These designs worked but did not follow what hardware implemented.
>>
>> After checking the existing code and referring to other implementation,
>> we decided to
>> use register offset + bit position method. It can support all SoCs with
>> this methods
>> without code change(device tree change only).
> maybe I have a different interpretation of what "RCU" does.
> let me explain it in my own words based on my knowledge about VRX200:
> - in my own words it is a multi function device with the following
> functionality:
> - it contains two reset controllers (reset at 0x10, status 0x14 and
> reset at 0x48, status at 0x24)
> - it contains two USB2 PHYs (PHY registers at 0x18, ANA cfg at 0x38
> and PHY registers at 0x34, ANA cfg at 0x3c)
> - it contains the configuration for the two GPHY IP blocks (at 0x20 and 0x68)
> - it contains endianness configuration registers (for PCI, PCIe, ...)
> - it contains the watchdog boot status (whether the SoC was previously
> reset by the WDT)
> - maybe more, but I don't know anything else about it
In fact, there is only one reset controller for all SoCs even it doesn't 
prevent software from virtualizing multiple reset controllers. Reset 
control does include some misc stuff which has been moved to chiptop in 
new SoCs so that RCU has a clean job.
> we tried our best to document this in
> Documentation/devicetree/bindings/mips/lantiq/rcu.txt
>
> I'm not sure about the details of the RCU on the LGM SoCs:
> if it contains more than just reset controllers then please let Rob
> Herring (dt-bindings maintainer) know about this.
> we may only have one chance to do it right, if we start with a
> "broken" binding then devices with incompatible bootloaders etc. may
> have already shipped
> (in general: that is why the devicetree maintainers want to have all
> device properties documented in the binding, even if the driver does
> not support all of them yet)

>
>>>> 2. reset-lantiq.c does not support device restart which is part of the
>>>> reset in
>>>> old lantiq SoC. It moved this part into arch/mips/lantiq directory.
>>> it was moved to the .dts instead of the arch code. again from
>>> OpenWrt's vr9.dtsi [0]:
>>>     reboot {
>>>       compatible = "syscon-reboot";
>>>       regmap = <&rcu0>;
>>>       offset = <0x10>;
>>>       mask = <0xe0000000>;
>>>     };
>>>
>>> this sets the reset0 reset bits 31, 30 and 29 at reboot
>> ok. but not sure why we need to reset bit 31 and 29. global softwre
>> reset is bit 30.
> I don't know either. depending on what the LGM SoCs need you can
> change the "mask" property to the value that fits that SoC best
>
> [...]
All SoCs have only one global software reset bit.
>>> - other reset lines only support reset pulses. the _reset function
>>> should be used in this case
>>> - the _reset function should only assert the reset line, then wait
>>> until the hardware automatically de-asserts it (without any further
>>> write)
>> Yes, this is called hardware reset. We can't control reset duration.
>>> is this the same for all, old and new SoCs?
>> New SoCs have removed support for hardware reset after software's feedback.
>>
>> Old SoCs such as VRX200/ARX300 has both software/hardware resets
> nice, it's good to see teamwork between hardware and software teams!
>
> [...]
>>>> 4. Code not optimized and intel internal review not assessed.
>>> insights from you (like the issue with the reset callback) are very
>>> valuable - this shows that we should focus on having one driver.
>>>
>>>> Based on the above findings, I would suggest reset-lantiq.c to move to
>>>> reset-intel-syscon.c
>>> my concern with having two separate drivers is that it will be hard to
>>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
>>> driver.
>>> I don't have access to the datasheets for the any Lantiq/Intel SoC
>>> (VRX200 and even older).
>>> so debugging issues after switching from one driver to another is
>>> tedious because I cannot tell which part of the driver is causing a
>>> problem (it's either "all code from driver A" vs "all code from driver
>>> B", meaning it's hard to narrow it down).
>>> with separate commits/patches that are improving the reset-lantiq
>>> driver I can do git bisect to find the cause of a problem on the older
>>> SoCs (VRX200 for example)
>> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
>> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
>> should be straight forward.
> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
> they only use level resets (_assert and _deassert) or are some reset
> lines using reset pulses (_reset)?
>
> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
> we still had to add support for the _reset callback as this is missing
> in reset-intel-syscon.c currently
Yes. We have reset pulse(assert, then check the reset status).
>
>
> Martin

Re: [PATCH v2 1/2] dt-bindings: reset: Add YAML schemas for the Intel Reset controller

[Dilip Kota]

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

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

[Martin Blumenstingl]

Hi,

On Wed, Aug 28, 2019 at 3:53 AM Chuan Hua, Lei
<chuanhua.lei@linux.intel.com> wrote:
[...]
> >>>> 1. reset-lantiq.c use index instead of register offset + bit position.
> >>>> index reset is good for a small system (< 64). However, it will become very
> >>>> difficult to use if you have  > 100 reset. So we use register offset +
> >>>> bit position
> >>> reset-lantiq uses bit bit positions for specifying the reset line.
> >>> for example this is from OpenWrt's vr9.dtsi:
> >>>     reset0: reset-controller@10 {
> >>>       ...
> >>>       reg = <0x10 4>, <0x14 4>;
> >>>       #reset-cells = <2>;
> >>>     };
> >>>
> >>>     gphy0: gphy@20 {
> >>>       ...
> >>>       resets = <&reset0 31 30>, <&reset1 7 7>;
> >>>       reset-names = "gphy", "gphy2";
> >>>     };
> >>>
> >>> in my own words this means:
> >>> - all reset0 reset bits are at offset 0x10 (parent is RCU)
> >>> - all reset0 status bits are at offset 0x14 (parent is RCU)
> >>> - the first reset line uses reset bit 31 and status bit 30
> >>> - the second reset line uses reset bit 7 and status bit 7
> >>> - there can be multiple reset-controller instances, each taking the
> >>> reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
> >>> reset controller "reset1" with reset offset 0x48 and status offset
> >>> 0x24)
> >> in reset-lantiq.c, we split each reset request /status pair into one
> >> reset controller.
> >>
> >> Each reset controller handles up to 32 resets. It will create up to 9
> >> even more
> >> reset controllers in the new SoCs. In reality, there is only one RCU
> >> controller for all
> >> SoCs. These designs worked but did not follow what hardware implemented.
> >>
> >> After checking the existing code and referring to other implementation,
> >> we decided to
> >> use register offset + bit position method. It can support all SoCs with
> >> this methods
> >> without code change(device tree change only).
> > maybe I have a different interpretation of what "RCU" does.
> > let me explain it in my own words based on my knowledge about VRX200:
> > - in my own words it is a multi function device with the following
> > functionality:
> > - it contains two reset controllers (reset at 0x10, status 0x14 and
> > reset at 0x48, status at 0x24)
> > - it contains two USB2 PHYs (PHY registers at 0x18, ANA cfg at 0x38
> > and PHY registers at 0x34, ANA cfg at 0x3c)
> > - it contains the configuration for the two GPHY IP blocks (at 0x20 and 0x68)
> > - it contains endianness configuration registers (for PCI, PCIe, ...)
> > - it contains the watchdog boot status (whether the SoC was previously
> > reset by the WDT)
> > - maybe more, but I don't know anything else about it
> In fact, there is only one reset controller for all SoCs even it doesn't
> prevent software from virtualizing multiple reset controllers. Reset
> control does include some misc stuff which has been moved to chiptop in
> new SoCs so that RCU has a clean job.
just to confirm that I understand this correctly:
even the VRX200 SoC only has one physical reset controller?
instead of a contiguous register area (let's say: 0x10 to 0x1c) it
uses four separate registers:
- 0x10 for asserting/deasserting/pulsing the first 32 reset lines
- 0x14 for the status of the first 32 reset lines
- 0x48 for asserting/deasserting/pulsing the second 32 reset lines
- 0x28 for the status of the second 32 reset lines

I'm not surprised that we got some of the IP block layout for the
VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
(BSP).
with proper documentation (as in a "public datasheet for the SoC") it
would be easy to spot these mistakes (at least I assume that the
quality of the Infineon / Lantiq datasheets is excellent).

back to reset-intel-syscon:
assigning only one job to the RCU hardware is a good idea (in my opinion).
that brings up a question: why do we need the "syscon" compatible for
the RCU node?
this is typically used when registers are accessed by another IP block
and the other driver has to access these registers as well. does this
mean that there's more hidden in the RCU registers?

> >>>> 2. reset-lantiq.c does not support device restart which is part of the
> >>>> reset in
> >>>> old lantiq SoC. It moved this part into arch/mips/lantiq directory.
> >>> it was moved to the .dts instead of the arch code. again from
> >>> OpenWrt's vr9.dtsi [0]:
> >>>     reboot {
> >>>       compatible = "syscon-reboot";
> >>>       regmap = <&rcu0>;
> >>>       offset = <0x10>;
> >>>       mask = <0xe0000000>;
> >>>     };
> >>>
> >>> this sets the reset0 reset bits 31, 30 and 29 at reboot
> >> ok. but not sure why we need to reset bit 31 and 29. global softwre
> >> reset is bit 30.
> > I don't know either. depending on what the LGM SoCs need you can
> > change the "mask" property to the value that fits that SoC best
> >
> > [...]
> All SoCs have only one global software reset bit.
OK
you can still use syscon-reboot to set the soft reset bit if Rob
(dt-binding maintainer) doesn't like the "intel,global-reset" property

[...]
> >>>> 4. Code not optimized and intel internal review not assessed.
> >>> insights from you (like the issue with the reset callback) are very
> >>> valuable - this shows that we should focus on having one driver.
> >>>
> >>>> Based on the above findings, I would suggest reset-lantiq.c to move to
> >>>> reset-intel-syscon.c
> >>> my concern with having two separate drivers is that it will be hard to
> >>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
> >>> driver.
> >>> I don't have access to the datasheets for the any Lantiq/Intel SoC
> >>> (VRX200 and even older).
> >>> so debugging issues after switching from one driver to another is
> >>> tedious because I cannot tell which part of the driver is causing a
> >>> problem (it's either "all code from driver A" vs "all code from driver
> >>> B", meaning it's hard to narrow it down).
> >>> with separate commits/patches that are improving the reset-lantiq
> >>> driver I can do git bisect to find the cause of a problem on the older
> >>> SoCs (VRX200 for example)
> >> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
> >> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
> >> should be straight forward.
> > what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
> > they only use level resets (_assert and _deassert) or are some reset
> > lines using reset pulses (_reset)?
> >
> > when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
> > we still had to add support for the _reset callback as this is missing
> > in reset-intel-syscon.c currently
> Yes. We have reset pulse(assert, then check the reset status).
only now I realized that the reset-intel-syscon driver does not seem
to use the status registers (instead it's looking at the reset
registers when checking the status).
what happened to the status registers - do they still exist in newer
SoCs (like LGM)? why are they not used?

on VRX200 for example there seem to be some cases where the bits in
the reset and status registers are different (for example: the first
GPHY seems to use reset bit 31 but status bit 30)
this is currently not supported in reset-intel-syscon


Martin

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

[Chuan Hua, Lei]

On 8/29/2019 4:01 AM, Martin Blumenstingl wrote:
> Hi,
>
> On Wed, Aug 28, 2019 at 3:53 AM Chuan Hua, Lei
> <chuanhua.lei@linux.intel.com> wrote:
> [...]
>>>>>> 1. reset-lantiq.c use index instead of register offset + bit position.
>>>>>> index reset is good for a small system (< 64). However, it will become very
>>>>>> difficult to use if you have  > 100 reset. So we use register offset +
>>>>>> bit position
>>>>> reset-lantiq uses bit bit positions for specifying the reset line.
>>>>> for example this is from OpenWrt's vr9.dtsi:
>>>>>      reset0: reset-controller@10 {
>>>>>        ...
>>>>>        reg = <0x10 4>, <0x14 4>;
>>>>>        #reset-cells = <2>;
>>>>>      };
>>>>>
>>>>>      gphy0: gphy@20 {
>>>>>        ...
>>>>>        resets = <&reset0 31 30>, <&reset1 7 7>;
>>>>>        reset-names = "gphy", "gphy2";
>>>>>      };
>>>>>
>>>>> in my own words this means:
>>>>> - all reset0 reset bits are at offset 0x10 (parent is RCU)
>>>>> - all reset0 status bits are at offset 0x14 (parent is RCU)
>>>>> - the first reset line uses reset bit 31 and status bit 30
>>>>> - the second reset line uses reset bit 7 and status bit 7
>>>>> - there can be multiple reset-controller instances, each taking the
>>>>> reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
>>>>> reset controller "reset1" with reset offset 0x48 and status offset
>>>>> 0x24)
>>>> in reset-lantiq.c, we split each reset request /status pair into one
>>>> reset controller.
>>>>
>>>> Each reset controller handles up to 32 resets. It will create up to 9
>>>> even more
>>>> reset controllers in the new SoCs. In reality, there is only one RCU
>>>> controller for all
>>>> SoCs. These designs worked but did not follow what hardware implemented.
>>>>
>>>> After checking the existing code and referring to other implementation,
>>>> we decided to
>>>> use register offset + bit position method. It can support all SoCs with
>>>> this methods
>>>> without code change(device tree change only).
>>> maybe I have a different interpretation of what "RCU" does.
>>> let me explain it in my own words based on my knowledge about VRX200:
>>> - in my own words it is a multi function device with the following
>>> functionality:
>>> - it contains two reset controllers (reset at 0x10, status 0x14 and
>>> reset at 0x48, status at 0x24)
>>> - it contains two USB2 PHYs (PHY registers at 0x18, ANA cfg at 0x38
>>> and PHY registers at 0x34, ANA cfg at 0x3c)
>>> - it contains the configuration for the two GPHY IP blocks (at 0x20 and 0x68)
>>> - it contains endianness configuration registers (for PCI, PCIe, ...)
>>> - it contains the watchdog boot status (whether the SoC was previously
>>> reset by the WDT)
>>> - maybe more, but I don't know anything else about it
>> In fact, there is only one reset controller for all SoCs even it doesn't
>> prevent software from virtualizing multiple reset controllers. Reset
>> control does include some misc stuff which has been moved to chiptop in
>> new SoCs so that RCU has a clean job.
> just to confirm that I understand this correctly:
> even the VRX200 SoC only has one physical reset controller?
> instead of a contiguous register area (let's say: 0x10 to 0x1c) it
> uses four separate registers:
> - 0x10 for asserting/deasserting/pulsing the first 32 reset lines
> - 0x14 for the status of the first 32 reset lines
> - 0x48 for asserting/deasserting/pulsing the second 32 reset lines
> - 0x28 for the status of the second 32 reset lines

Yes, but for VRX200, reset controller registers include some other misc 
registers. At that time,

hardware doesn't use chiptop concept, they put some misc registers into 
CGU/RCU which makes it quite messy.

We also prefer to have 0x10~0x1c. However, when developing VRX200, 0x18, 
0x20 and other address had been used by other registers. system becomes 
more complex, need more reset bits for new modules, then hardware just 
added them to any available place. From another angle, hardware people 
also tried to keep backward compatible with old products like Danube.

>
> I'm not surprised that we got some of the IP block layout for the
> VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
> (BSP).
> with proper documentation (as in a "public datasheet for the SoC") it
> would be easy to spot these mistakes (at least I assume that the
> quality of the Infineon / Lantiq datasheets is excellent).
>
> back to reset-intel-syscon:
> assigning only one job to the RCU hardware is a good idea (in my opinion).
> that brings up a question: why do we need the "syscon" compatible for
> the RCU node?
> this is typically used when registers are accessed by another IP block
> and the other driver has to access these registers as well. does this
> mean that there's more hidden in the RCU registers?
As I mentioned, some other misc registers are put into RCU even they 
don't belong to reset functions. In MIPS, global software reset handled 
in arch/mips/, only recently, this situation changed. This means we have 
at least two places to access this module.
>
>>>>>> 2. reset-lantiq.c does not support device restart which is part of the
>>>>>> reset in
>>>>>> old lantiq SoC. It moved this part into arch/mips/lantiq directory.
>>>>> it was moved to the .dts instead of the arch code. again from
>>>>> OpenWrt's vr9.dtsi [0]:
>>>>>      reboot {
>>>>>        compatible = "syscon-reboot";
>>>>>        regmap = <&rcu0>;
>>>>>        offset = <0x10>;
>>>>>        mask = <0xe0000000>;
>>>>>      };
>>>>>
>>>>> this sets the reset0 reset bits 31, 30 and 29 at reboot
>>>> ok. but not sure why we need to reset bit 31 and 29. global softwre
>>>> reset is bit 30.
>>> I don't know either. depending on what the LGM SoCs need you can
>>> change the "mask" property to the value that fits that SoC best
>>>
>>> [...]
>> All SoCs have only one global software reset bit.
> OK
> you can still use syscon-reboot to set the soft reset bit if Rob
> (dt-binding maintainer) doesn't like the "intel,global-reset" property
Dilip should check and do the necessary change.
> [...]
>>>>>> 4. Code not optimized and intel internal review not assessed.
>>>>> insights from you (like the issue with the reset callback) are very
>>>>> valuable - this shows that we should focus on having one driver.
>>>>>
>>>>>> Based on the above findings, I would suggest reset-lantiq.c to move to
>>>>>> reset-intel-syscon.c
>>>>> my concern with having two separate drivers is that it will be hard to
>>>>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
>>>>> driver.
>>>>> I don't have access to the datasheets for the any Lantiq/Intel SoC
>>>>> (VRX200 and even older).
>>>>> so debugging issues after switching from one driver to another is
>>>>> tedious because I cannot tell which part of the driver is causing a
>>>>> problem (it's either "all code from driver A" vs "all code from driver
>>>>> B", meaning it's hard to narrow it down).
>>>>> with separate commits/patches that are improving the reset-lantiq
>>>>> driver I can do git bisect to find the cause of a problem on the older
>>>>> SoCs (VRX200 for example)
>>>> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
>>>> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
>>>> should be straight forward.
>>> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
>>> they only use level resets (_assert and _deassert) or are some reset
>>> lines using reset pulses (_reset)?
>>>
>>> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
>>> we still had to add support for the _reset callback as this is missing
>>> in reset-intel-syscon.c currently
>> Yes. We have reset pulse(assert, then check the reset status).
> only now I realized that the reset-intel-syscon driver does not seem
> to use the status registers (instead it's looking at the reset
> registers when checking the status).
> what happened to the status registers - do they still exist in newer
> SoCs (like LGM)? why are they not used?
Reset status check is there. regmap_read_poll_timeout to check status 
big. Status register offset <4) from request register. For legacy, there 
is one exception, we can add soc specific data to handle it.
> on VRX200 for example there seem to be some cases where the bits in
> the reset and status registers are different (for example: the first
> GPHY seems to use reset bit 31 but status bit 30)
> this is currently not supported in reset-intel-syscon
This is most tricky and ugly part for VRX200/Danube. Do you have any 
idea to handle this nicely?
>
>
> Martin

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

[Martin Blumenstingl]

Hi,

On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei
<chuanhua.lei@linux.intel.com> wrote:

> >
> > I'm not surprised that we got some of the IP block layout for the
> > VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
> > (BSP).
> > with proper documentation (as in a "public datasheet for the SoC") it
> > would be easy to spot these mistakes (at least I assume that the
> > quality of the Infineon / Lantiq datasheets is excellent).
> >
> > back to reset-intel-syscon:
> > assigning only one job to the RCU hardware is a good idea (in my opinion).
> > that brings up a question: why do we need the "syscon" compatible for
> > the RCU node?
> > this is typically used when registers are accessed by another IP block
> > and the other driver has to access these registers as well. does this
> > mean that there's more hidden in the RCU registers?
> As I mentioned, some other misc registers are put into RCU even they
> don't belong to reset functions.
OK, just be aware that there are also rules for syscon compatible
drivers, see for example: [0]
if Rob (dt-bindings maintainer) is happy with the documentation in
patch 1 then I'm fine with it as well.
for my own education I would appreciate if you could describe these
"other misc registers" with a few sentences (I assume that this can
also help Rob)

[...]
> >>>>>> 4. Code not optimized and intel internal review not assessed.
> >>>>> insights from you (like the issue with the reset callback) are very
> >>>>> valuable - this shows that we should focus on having one driver.
> >>>>>
> >>>>>> Based on the above findings, I would suggest reset-lantiq.c to move to
> >>>>>> reset-intel-syscon.c
> >>>>> my concern with having two separate drivers is that it will be hard to
> >>>>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
> >>>>> driver.
> >>>>> I don't have access to the datasheets for the any Lantiq/Intel SoC
> >>>>> (VRX200 and even older).
> >>>>> so debugging issues after switching from one driver to another is
> >>>>> tedious because I cannot tell which part of the driver is causing a
> >>>>> problem (it's either "all code from driver A" vs "all code from driver
> >>>>> B", meaning it's hard to narrow it down).
> >>>>> with separate commits/patches that are improving the reset-lantiq
> >>>>> driver I can do git bisect to find the cause of a problem on the older
> >>>>> SoCs (VRX200 for example)
> >>>> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
> >>>> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
> >>>> should be straight forward.
> >>> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
> >>> they only use level resets (_assert and _deassert) or are some reset
> >>> lines using reset pulses (_reset)?
> >>>
> >>> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
> >>> we still had to add support for the _reset callback as this is missing
> >>> in reset-intel-syscon.c currently
> >> Yes. We have reset pulse(assert, then check the reset status).
> > only now I realized that the reset-intel-syscon driver does not seem
> > to use the status registers (instead it's looking at the reset
> > registers when checking the status).
> > what happened to the status registers - do they still exist in newer
> > SoCs (like LGM)? why are they not used?
> Reset status check is there. regmap_read_poll_timeout to check status
> big. Status register offset <4) from request register. For legacy, there
> is one exception, we can add soc specific data to handle it.
I see, thank you for the explanation
this won't work on VRX200 for example because the status register is
not always at (reset register - 0x4)

> > on VRX200 for example there seem to be some cases where the bits in
> > the reset and status registers are different (for example: the first
> > GPHY seems to use reset bit 31 but status bit 30)
> > this is currently not supported in reset-intel-syscon
> This is most tricky and ugly part for VRX200/Danube. Do you have any
> idea to handle this nicely?
with reset-lantiq we have the following register information:
a) reset offset: first reg property
b) status offset: second reg property
c) reset bit: first #reset-cell
d) status bit: second #reset-cell

reset-intel-syscon derives half of this information from the two #reset-cells:
a) reset offset: first #reset-cell
b) status offset: reset offset - 0x4
c) reset bit: second #reset-cell
d) status bit: same as reset bit

I cannot make any suggestion (yet) how to handle VRX200 and LGM in one
driver because I don't know enough about LGM (yet).
on VRX200 my understanding is that we have 64 reset bits (2x 32bit
registers) and 64 status bits (also 2x 32bit registers). each reset
bit has a corresponding status bit but the numbering may be different
it's not clear to me how many resets LGM supports and how they are
organized. for example: I think it makes a difference if "there are 64
registers with each one reset bit" versus "there are two registers
with 32 bits each"
please share some details how it's organized internally, then I can
try to come up with a suggestion.

... after writing this I noticed that we are discussing the binding.
we definitely need to share a summary with Rob on patch #1 and check
with him if he sees any problems with the approach that we may come up
with


Martin


[0] https://lkml.org/lkml/2019/8/27/849

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

[Chuan Hua, Lei]

Hi Martin,

On 8/30/2019 5:40 AM, Martin Blumenstingl wrote:
> Hi,
>
> On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei
> <chuanhua.lei@linux.intel.com> wrote:
>
>>> I'm not surprised that we got some of the IP block layout for the
>>> VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
>>> (BSP).
>>> with proper documentation (as in a "public datasheet for the SoC") it
>>> would be easy to spot these mistakes (at least I assume that the
>>> quality of the Infineon / Lantiq datasheets is excellent).
>>>
>>> back to reset-intel-syscon:
>>> assigning only one job to the RCU hardware is a good idea (in my opinion).
>>> that brings up a question: why do we need the "syscon" compatible for
>>> the RCU node?
>>> this is typically used when registers are accessed by another IP block
>>> and the other driver has to access these registers as well. does this
>>> mean that there's more hidden in the RCU registers?
>> As I mentioned, some other misc registers are put into RCU even they
>> don't belong to reset functions.
> OK, just be aware that there are also rules for syscon compatible
> drivers, see for example: [0]
> if Rob (dt-bindings maintainer) is happy with the documentation in
> patch 1 then I'm fine with it as well.
> for my own education I would appreciate if you could describe these
> "other misc registers" with a few sentences (I assume that this can
> also help Rob)
For LGM, RCU is clean. There would be no MISC register after software's 
feedback. These misc registers will be moved to chiptop/misc 
groups(implemented by syscon). For legacy SoC, we do have a lot MISC 
registers for different SoCs.
>
> [...]
>>>>>>>> 4. Code not optimized and intel internal review not assessed.
>>>>>>> insights from you (like the issue with the reset callback) are very
>>>>>>> valuable - this shows that we should focus on having one driver.
>>>>>>>
>>>>>>>> Based on the above findings, I would suggest reset-lantiq.c to move to
>>>>>>>> reset-intel-syscon.c
>>>>>>> my concern with having two separate drivers is that it will be hard to
>>>>>>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
>>>>>>> driver.
>>>>>>> I don't have access to the datasheets for the any Lantiq/Intel SoC
>>>>>>> (VRX200 and even older).
>>>>>>> so debugging issues after switching from one driver to another is
>>>>>>> tedious because I cannot tell which part of the driver is causing a
>>>>>>> problem (it's either "all code from driver A" vs "all code from driver
>>>>>>> B", meaning it's hard to narrow it down).
>>>>>>> with separate commits/patches that are improving the reset-lantiq
>>>>>>> driver I can do git bisect to find the cause of a problem on the older
>>>>>>> SoCs (VRX200 for example)
>>>>>> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
>>>>>> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
>>>>>> should be straight forward.
>>>>> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
>>>>> they only use level resets (_assert and _deassert) or are some reset
>>>>> lines using reset pulses (_reset)?
>>>>>
>>>>> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
>>>>> we still had to add support for the _reset callback as this is missing
>>>>> in reset-intel-syscon.c currently
>>>> Yes. We have reset pulse(assert, then check the reset status).
>>> only now I realized that the reset-intel-syscon driver does not seem
>>> to use the status registers (instead it's looking at the reset
>>> registers when checking the status).
>>> what happened to the status registers - do they still exist in newer
>>> SoCs (like LGM)? why are they not used?
>> Reset status check is there. regmap_read_poll_timeout to check status
>> big. Status register offset <4) from request register. For legacy, there
>> is one exception, we can add soc specific data to handle it.
> I see, thank you for the explanation
> this won't work on VRX200 for example because the status register is
> not always at (reset register - 0x4)

As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved 
with one soc data in the compatible array.

For example(not same as upstream, but idea is similar)

static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
{
     if (data->soc_data->legacy && req_off == RCU_RST_REQ)
         return RCU_RST_STAT;
     else
         return req_off + 0x4;
}

>
>>> on VRX200 for example there seem to be some cases where the bits in
>>> the reset and status registers are different (for example: the first
>>> GPHY seems to use reset bit 31 but status bit 30)
>>> this is currently not supported in reset-intel-syscon
>> This is most tricky and ugly part for VRX200/Danube. Do you have any
>> idea to handle this nicely?
> with reset-lantiq we have the following register information:
> a) reset offset: first reg property
> b) status offset: second reg property
> c) reset bit: first #reset-cell
> d) status bit: second #reset-cell
>
> reset-intel-syscon derives half of this information from the two #reset-cells:
> a) reset offset: first #reset-cell
> b) status offset: reset offset - 0x4
> c) reset bit: second #reset-cell
> d) status bit: same as reset bit
>
> I cannot make any suggestion (yet) how to handle VRX200 and LGM in one
> driver because I don't know enough about LGM (yet).
> on VRX200 my understanding is that we have 64 reset bits (2x 32bit
> registers) and 64 status bits (also 2x 32bit registers). each reset
> bit has a corresponding status bit but the numbering may be different
> it's not clear to me how many resets LGM supports and how they are
> organized. for example: I think it makes a difference if "there are 64
> registers with each one reset bit" versus "there are two registers
> with 32 bits each"
> please share some details how it's organized internally, then I can
> try to come up with a suggestion.
LGM reset organization is more clean compared with legacy SoCs. We have 
8 x 32bit reset and status registers(more modules need to be reset, 
overall ideas are similar without big change). Their request and status 
bit is at the same register bit position.  Hope this will help you.
>
> ... after writing this I noticed that we are discussing the binding.
> we definitely need to share a summary with Rob on patch #1 and check
> with him if he sees any problems with the approach that we may come up
> with
>
>
> Martin
>
>
> [0] https://lkml.org/lkml/2019/8/27/849

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

[Martin Blumenstingl]

Hi,

On Fri, Aug 30, 2019 at 5:02 AM Chuan Hua, Lei
<chuanhua.lei@linux.intel.com> wrote:
>
> Hi Martin,
>
> On 8/30/2019 5:40 AM, Martin Blumenstingl wrote:
> > Hi,
> >
> > On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei
> > <chuanhua.lei@linux.intel.com> wrote:
> >
> >>> I'm not surprised that we got some of the IP block layout for the
> >>> VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
> >>> (BSP).
> >>> with proper documentation (as in a "public datasheet for the SoC") it
> >>> would be easy to spot these mistakes (at least I assume that the
> >>> quality of the Infineon / Lantiq datasheets is excellent).
> >>>
> >>> back to reset-intel-syscon:
> >>> assigning only one job to the RCU hardware is a good idea (in my opinion).
> >>> that brings up a question: why do we need the "syscon" compatible for
> >>> the RCU node?
> >>> this is typically used when registers are accessed by another IP block
> >>> and the other driver has to access these registers as well. does this
> >>> mean that there's more hidden in the RCU registers?
> >> As I mentioned, some other misc registers are put into RCU even they
> >> don't belong to reset functions.
> > OK, just be aware that there are also rules for syscon compatible
> > drivers, see for example: [0]
> > if Rob (dt-bindings maintainer) is happy with the documentation in
> > patch 1 then I'm fine with it as well.
> > for my own education I would appreciate if you could describe these
> > "other misc registers" with a few sentences (I assume that this can
> > also help Rob)
> For LGM, RCU is clean. There would be no MISC register after software's
> feedback. These misc registers will be moved to chiptop/misc
> groups(implemented by syscon). For legacy SoC, we do have a lot MISC
> registers for different SoCs.
OK, I think I understand now: chiptop != RCU
so RCU really only has one purpose: handling resets
while chiptop manages all the random bits

does this means we don't need RCU to match "syscon"?

> >
> > [...]
> >>>>>>>> 4. Code not optimized and intel internal review not assessed.
> >>>>>>> insights from you (like the issue with the reset callback) are very
> >>>>>>> valuable - this shows that we should focus on having one driver.
> >>>>>>>
> >>>>>>>> Based on the above findings, I would suggest reset-lantiq.c to move to
> >>>>>>>> reset-intel-syscon.c
> >>>>>>> my concern with having two separate drivers is that it will be hard to
> >>>>>>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
> >>>>>>> driver.
> >>>>>>> I don't have access to the datasheets for the any Lantiq/Intel SoC
> >>>>>>> (VRX200 and even older).
> >>>>>>> so debugging issues after switching from one driver to another is
> >>>>>>> tedious because I cannot tell which part of the driver is causing a
> >>>>>>> problem (it's either "all code from driver A" vs "all code from driver
> >>>>>>> B", meaning it's hard to narrow it down).
> >>>>>>> with separate commits/patches that are improving the reset-lantiq
> >>>>>>> driver I can do git bisect to find the cause of a problem on the older
> >>>>>>> SoCs (VRX200 for example)
> >>>>>> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
> >>>>>> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
> >>>>>> should be straight forward.
> >>>>> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
> >>>>> they only use level resets (_assert and _deassert) or are some reset
> >>>>> lines using reset pulses (_reset)?
> >>>>>
> >>>>> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
> >>>>> we still had to add support for the _reset callback as this is missing
> >>>>> in reset-intel-syscon.c currently
> >>>> Yes. We have reset pulse(assert, then check the reset status).
> >>> only now I realized that the reset-intel-syscon driver does not seem
> >>> to use the status registers (instead it's looking at the reset
> >>> registers when checking the status).
> >>> what happened to the status registers - do they still exist in newer
> >>> SoCs (like LGM)? why are they not used?
> >> Reset status check is there. regmap_read_poll_timeout to check status
> >> big. Status register offset <4) from request register. For legacy, there
> >> is one exception, we can add soc specific data to handle it.
> > I see, thank you for the explanation
> > this won't work on VRX200 for example because the status register is
> > not always at (reset register - 0x4)
>
> As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
> with one soc data in the compatible array.
>
> For example(not same as upstream, but idea is similar)
>
> static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
> {
>      if (data->soc_data->legacy && req_off == RCU_RST_REQ)
>          return RCU_RST_STAT;
>      else
>          return req_off + 0x4;
> }
>
> >
> >>> on VRX200 for example there seem to be some cases where the bits in
> >>> the reset and status registers are different (for example: the first
> >>> GPHY seems to use reset bit 31 but status bit 30)
> >>> this is currently not supported in reset-intel-syscon
> >> This is most tricky and ugly part for VRX200/Danube. Do you have any
> >> idea to handle this nicely?
> > with reset-lantiq we have the following register information:
> > a) reset offset: first reg property
> > b) status offset: second reg property
> > c) reset bit: first #reset-cell
> > d) status bit: second #reset-cell
> >
> > reset-intel-syscon derives half of this information from the two #reset-cells:
> > a) reset offset: first #reset-cell
> > b) status offset: reset offset - 0x4
> > c) reset bit: second #reset-cell
> > d) status bit: same as reset bit
> >
> > I cannot make any suggestion (yet) how to handle VRX200 and LGM in one
> > driver because I don't know enough about LGM (yet).
> > on VRX200 my understanding is that we have 64 reset bits (2x 32bit
> > registers) and 64 status bits (also 2x 32bit registers). each reset
> > bit has a corresponding status bit but the numbering may be different
> > it's not clear to me how many resets LGM supports and how they are
> > organized. for example: I think it makes a difference if "there are 64
> > registers with each one reset bit" versus "there are two registers
> > with 32 bits each"
> > please share some details how it's organized internally, then I can
> > try to come up with a suggestion.
> LGM reset organization is more clean compared with legacy SoCs. We have
> 8 x 32bit reset and status registers(more modules need to be reset,
> overall ideas are similar without big change). Their request and status
> bit is at the same register bit position.  Hope this will help you.
have you already discussed using only one reset cell?
if there's only one big reset controller in RCU then why not let the
reset controller driver do it's job of translating a reset line? also
this represents the hardware best (dt-bindings should describe the
hardware, drivers then translate that into the various subsystems
offered by the kernel).

we have to translate it into:
- status register and bit
- reset register and bit

for LGM the implementation seems to be the easiest because the reset
line can be mapped easily to the registers and bit offsets (for
example like reset-meson.c does it, which also supports 256 reset
lines together with for example
include/dt-bindings/reset/amlogic,meson-g12a-reset.h. the latter is
nice to have but optional)
we can then implement special translation logic (in other words: a
separate of_xlate callback) for VRX200 which then has to do more
"magic" (like you have shown in your example code above: "if the reset
line belongs to the second set of 32 reset lines then use reset offset
X and status offset Y" - or even use a translation table as
reset-imx7.c does)

the current binding is a mix of specifying reset register and bit in
.dts but calculating the status register.
I missed the calculation of the status register until you pointed it out earlier


Martin

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

[Chuan Hua, Lei]

Hi Martin,


On 9/2/2019 5:38 AM, Martin Blumenstingl wrote:
> Hi,
>
> On Fri, Aug 30, 2019 at 5:02 AM Chuan Hua, Lei
> <chuanhua.lei@linux.intel.com> wrote:
>> Hi Martin,
>>
>> On 8/30/2019 5:40 AM, Martin Blumenstingl wrote:
>>> Hi,
>>>
>>> On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei
>>> <chuanhua.lei@linux.intel.com> wrote:
>>>
>>>>> I'm not surprised that we got some of the IP block layout for the
>>>>> VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
>>>>> (BSP).
>>>>> with proper documentation (as in a "public datasheet for the SoC") it
>>>>> would be easy to spot these mistakes (at least I assume that the
>>>>> quality of the Infineon / Lantiq datasheets is excellent).
>>>>>
>>>>> back to reset-intel-syscon:
>>>>> assigning only one job to the RCU hardware is a good idea (in my opinion).
>>>>> that brings up a question: why do we need the "syscon" compatible for
>>>>> the RCU node?
>>>>> this is typically used when registers are accessed by another IP block
>>>>> and the other driver has to access these registers as well. does this
>>>>> mean that there's more hidden in the RCU registers?
>>>> As I mentioned, some other misc registers are put into RCU even they
>>>> don't belong to reset functions.
>>> OK, just be aware that there are also rules for syscon compatible
>>> drivers, see for example: [0]
>>> if Rob (dt-bindings maintainer) is happy with the documentation in
>>> patch 1 then I'm fine with it as well.
>>> for my own education I would appreciate if you could describe these
>>> "other misc registers" with a few sentences (I assume that this can
>>> also help Rob)
>> For LGM, RCU is clean. There would be no MISC register after software's
>> feedback. These misc registers will be moved to chiptop/misc
>> groups(implemented by syscon). For legacy SoC, we do have a lot MISC
>> registers for different SoCs.
> OK, I think I understand now: chiptop != RCU
> so RCU really only has one purpose: handling resets
> while chiptop manages all the random bits
>
> does this means we don't need RCU to match "syscon"?

If we don't support legacy SoC with the same driver, we don't need 
syscon, just regmap. Regmap is a must for us since we will use regmap 
proxy to implement secure rest via secure processor.

>
>>> [...]
>>>>>>>>>> 4. Code not optimized and intel internal review not assessed.
>>>>>>>>> insights from you (like the issue with the reset callback) are very
>>>>>>>>> valuable - this shows that we should focus on having one driver.
>>>>>>>>>
>>>>>>>>>> Based on the above findings, I would suggest reset-lantiq.c to move to
>>>>>>>>>> reset-intel-syscon.c
>>>>>>>>> my concern with having two separate drivers is that it will be hard to
>>>>>>>>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
>>>>>>>>> driver.
>>>>>>>>> I don't have access to the datasheets for the any Lantiq/Intel SoC
>>>>>>>>> (VRX200 and even older).
>>>>>>>>> so debugging issues after switching from one driver to another is
>>>>>>>>> tedious because I cannot tell which part of the driver is causing a
>>>>>>>>> problem (it's either "all code from driver A" vs "all code from driver
>>>>>>>>> B", meaning it's hard to narrow it down).
>>>>>>>>> with separate commits/patches that are improving the reset-lantiq
>>>>>>>>> driver I can do git bisect to find the cause of a problem on the older
>>>>>>>>> SoCs (VRX200 for example)
>>>>>>>> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
>>>>>>>> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
>>>>>>>> should be straight forward.
>>>>>>> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
>>>>>>> they only use level resets (_assert and _deassert) or are some reset
>>>>>>> lines using reset pulses (_reset)?
>>>>>>>
>>>>>>> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
>>>>>>> we still had to add support for the _reset callback as this is missing
>>>>>>> in reset-intel-syscon.c currently
>>>>>> Yes. We have reset pulse(assert, then check the reset status).
>>>>> only now I realized that the reset-intel-syscon driver does not seem
>>>>> to use the status registers (instead it's looking at the reset
>>>>> registers when checking the status).
>>>>> what happened to the status registers - do they still exist in newer
>>>>> SoCs (like LGM)? why are they not used?
>>>> Reset status check is there. regmap_read_poll_timeout to check status
>>>> big. Status register offset <4) from request register. For legacy, there
>>>> is one exception, we can add soc specific data to handle it.
>>> I see, thank you for the explanation
>>> this won't work on VRX200 for example because the status register is
>>> not always at (reset register - 0x4)
>> As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
>> with one soc data in the compatible array.
>>
>> For example(not same as upstream, but idea is similar)
>>
>> static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
>> {
>>       if (data->soc_data->legacy && req_off == RCU_RST_REQ)
>>           return RCU_RST_STAT;
>>       else
>>           return req_off + 0x4;
>> }
>>
>>>>> on VRX200 for example there seem to be some cases where the bits in
>>>>> the reset and status registers are different (for example: the first
>>>>> GPHY seems to use reset bit 31 but status bit 30)
>>>>> this is currently not supported in reset-intel-syscon
>>>> This is most tricky and ugly part for VRX200/Danube. Do you have any
>>>> idea to handle this nicely?
>>> with reset-lantiq we have the following register information:
>>> a) reset offset: first reg property
>>> b) status offset: second reg property
>>> c) reset bit: first #reset-cell
>>> d) status bit: second #reset-cell
>>>
>>> reset-intel-syscon derives half of this information from the two #reset-cells:
>>> a) reset offset: first #reset-cell
>>> b) status offset: reset offset - 0x4
>>> c) reset bit: second #reset-cell
>>> d) status bit: same as reset bit
>>>
>>> I cannot make any suggestion (yet) how to handle VRX200 and LGM in one
>>> driver because I don't know enough about LGM (yet).
>>> on VRX200 my understanding is that we have 64 reset bits (2x 32bit
>>> registers) and 64 status bits (also 2x 32bit registers). each reset
>>> bit has a corresponding status bit but the numbering may be different
>>> it's not clear to me how many resets LGM supports and how they are
>>> organized. for example: I think it makes a difference if "there are 64
>>> registers with each one reset bit" versus "there are two registers
>>> with 32 bits each"
>>> please share some details how it's organized internally, then I can
>>> try to come up with a suggestion.
>> LGM reset organization is more clean compared with legacy SoCs. We have
>> 8 x 32bit reset and status registers(more modules need to be reset,
>> overall ideas are similar without big change). Their request and status
>> bit is at the same register bit position.  Hope this will help you.
> have you already discussed using only one reset cell?
> if there's only one big reset controller in RCU then why not let the
> reset controller driver do it's job of translating a reset line? also
> this represents the hardware best (dt-bindings should describe the
> hardware, drivers then translate that into the various subsystems
> offered by the kernel).
>
> we have to translate it into:
> - status register and bit
> - reset register and bit
>
> for LGM the implementation seems to be the easiest because the reset
> line can be mapped easily to the registers and bit offsets (for
> example like reset-meson.c does it, which also supports 256 reset
> lines together with for example
> include/dt-bindings/reset/amlogic,meson-g12a-reset.h. the latter is
> nice to have but optional)
When we implement this driver, we checked other drivers(hisilicon/*, 
reset-berlin.c and etc). After evaluation, we think register offset and 
register bit are easier for users to understand and use if they follow 
the hardware spec.
> we can then implement special translation logic (in other words: a
> separate of_xlate callback) for VRX200 which then has to do more
> "magic" (like you have shown in your example code above: "if the reset
> line belongs to the second set of 32 reset lines then use reset offset
> X and status offset Y" - or even use a translation table as
> reset-imx7.c does)
>
> the current binding is a mix of specifying reset register and bit in
> .dts but calculating the status register.
> I missed the calculation of the status register until you pointed it out earlier
But we still don't have a good solution for VRX200 status bit issues. 
Before we solve this issue, it is very difficult to use one driver for 
all SoCs.
>
> Martin

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

[Martin Blumenstingl]

Hi,

On Mon, Sep 2, 2019 at 11:45 AM Chuan Hua, Lei
<chuanhua.lei@linux.intel.com> wrote:
>
> Hi Martin,
>
>
> On 9/2/2019 5:38 AM, Martin Blumenstingl wrote:
> > Hi,
> >
> > On Fri, Aug 30, 2019 at 5:02 AM Chuan Hua, Lei
> > <chuanhua.lei@linux.intel.com> wrote:
> >> Hi Martin,
> >>
> >> On 8/30/2019 5:40 AM, Martin Blumenstingl wrote:
> >>> Hi,
> >>>
> >>> On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei
> >>> <chuanhua.lei@linux.intel.com> wrote:
> >>>
> >>>>> I'm not surprised that we got some of the IP block layout for the
> >>>>> VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
> >>>>> (BSP).
> >>>>> with proper documentation (as in a "public datasheet for the SoC") it
> >>>>> would be easy to spot these mistakes (at least I assume that the
> >>>>> quality of the Infineon / Lantiq datasheets is excellent).
> >>>>>
> >>>>> back to reset-intel-syscon:
> >>>>> assigning only one job to the RCU hardware is a good idea (in my opinion).
> >>>>> that brings up a question: why do we need the "syscon" compatible for
> >>>>> the RCU node?
> >>>>> this is typically used when registers are accessed by another IP block
> >>>>> and the other driver has to access these registers as well. does this
> >>>>> mean that there's more hidden in the RCU registers?
> >>>> As I mentioned, some other misc registers are put into RCU even they
> >>>> don't belong to reset functions.
> >>> OK, just be aware that there are also rules for syscon compatible
> >>> drivers, see for example: [0]
> >>> if Rob (dt-bindings maintainer) is happy with the documentation in
> >>> patch 1 then I'm fine with it as well.
> >>> for my own education I would appreciate if you could describe these
> >>> "other misc registers" with a few sentences (I assume that this can
> >>> also help Rob)
> >> For LGM, RCU is clean. There would be no MISC register after software's
> >> feedback. These misc registers will be moved to chiptop/misc
> >> groups(implemented by syscon). For legacy SoC, we do have a lot MISC
> >> registers for different SoCs.
> > OK, I think I understand now: chiptop != RCU
> > so RCU really only has one purpose: handling resets
> > while chiptop manages all the random bits
> >
> > does this means we don't need RCU to match "syscon"?
>
> If we don't support legacy SoC with the same driver, we don't need
> syscon, just regmap. Regmap is a must for us since we will use regmap
> proxy to implement secure rest via secure processor.
I think we should drop the syscon compatible for LGM then
even for the legacy SoCs the reset controller should not have a syscon
compatible: instead it should have a syscon parent (as the current
"lantiq,xrx200-reset" binding requires and as suggested by Rob for
another IP block: [0])

keeping regmap is great in my opinion because it's a nice API and gets
rid of some boilerplate
even better if it makes things easier for accessing the secure processor

> >
> >>> [...]
> >>>>>>>>>> 4. Code not optimized and intel internal review not assessed.
> >>>>>>>>> insights from you (like the issue with the reset callback) are very
> >>>>>>>>> valuable - this shows that we should focus on having one driver.
> >>>>>>>>>
> >>>>>>>>>> Based on the above findings, I would suggest reset-lantiq.c to move to
> >>>>>>>>>> reset-intel-syscon.c
> >>>>>>>>> my concern with having two separate drivers is that it will be hard to
> >>>>>>>>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
> >>>>>>>>> driver.
> >>>>>>>>> I don't have access to the datasheets for the any Lantiq/Intel SoC
> >>>>>>>>> (VRX200 and even older).
> >>>>>>>>> so debugging issues after switching from one driver to another is
> >>>>>>>>> tedious because I cannot tell which part of the driver is causing a
> >>>>>>>>> problem (it's either "all code from driver A" vs "all code from driver
> >>>>>>>>> B", meaning it's hard to narrow it down).
> >>>>>>>>> with separate commits/patches that are improving the reset-lantiq
> >>>>>>>>> driver I can do git bisect to find the cause of a problem on the older
> >>>>>>>>> SoCs (VRX200 for example)
> >>>>>>>> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
> >>>>>>>> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
> >>>>>>>> should be straight forward.
> >>>>>>> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
> >>>>>>> they only use level resets (_assert and _deassert) or are some reset
> >>>>>>> lines using reset pulses (_reset)?
> >>>>>>>
> >>>>>>> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
> >>>>>>> we still had to add support for the _reset callback as this is missing
> >>>>>>> in reset-intel-syscon.c currently
> >>>>>> Yes. We have reset pulse(assert, then check the reset status).
> >>>>> only now I realized that the reset-intel-syscon driver does not seem
> >>>>> to use the status registers (instead it's looking at the reset
> >>>>> registers when checking the status).
> >>>>> what happened to the status registers - do they still exist in newer
> >>>>> SoCs (like LGM)? why are they not used?
> >>>> Reset status check is there. regmap_read_poll_timeout to check status
> >>>> big. Status register offset <4) from request register. For legacy, there
> >>>> is one exception, we can add soc specific data to handle it.
> >>> I see, thank you for the explanation
> >>> this won't work on VRX200 for example because the status register is
> >>> not always at (reset register - 0x4)
> >> As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
> >> with one soc data in the compatible array.
> >>
> >> For example(not same as upstream, but idea is similar)
> >>
> >> static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
> >> {
> >>       if (data->soc_data->legacy && req_off == RCU_RST_REQ)
> >>           return RCU_RST_STAT;
> >>       else
> >>           return req_off + 0x4;
> >> }
> >>
> >>>>> on VRX200 for example there seem to be some cases where the bits in
> >>>>> the reset and status registers are different (for example: the first
> >>>>> GPHY seems to use reset bit 31 but status bit 30)
> >>>>> this is currently not supported in reset-intel-syscon
> >>>> This is most tricky and ugly part for VRX200/Danube. Do you have any
> >>>> idea to handle this nicely?
> >>> with reset-lantiq we have the following register information:
> >>> a) reset offset: first reg property
> >>> b) status offset: second reg property
> >>> c) reset bit: first #reset-cell
> >>> d) status bit: second #reset-cell
> >>>
> >>> reset-intel-syscon derives half of this information from the two #reset-cells:
> >>> a) reset offset: first #reset-cell
> >>> b) status offset: reset offset - 0x4
> >>> c) reset bit: second #reset-cell
> >>> d) status bit: same as reset bit
> >>>
> >>> I cannot make any suggestion (yet) how to handle VRX200 and LGM in one
> >>> driver because I don't know enough about LGM (yet).
> >>> on VRX200 my understanding is that we have 64 reset bits (2x 32bit
> >>> registers) and 64 status bits (also 2x 32bit registers). each reset
> >>> bit has a corresponding status bit but the numbering may be different
> >>> it's not clear to me how many resets LGM supports and how they are
> >>> organized. for example: I think it makes a difference if "there are 64
> >>> registers with each one reset bit" versus "there are two registers
> >>> with 32 bits each"
> >>> please share some details how it's organized internally, then I can
> >>> try to come up with a suggestion.
> >> LGM reset organization is more clean compared with legacy SoCs. We have
> >> 8 x 32bit reset and status registers(more modules need to be reset,
> >> overall ideas are similar without big change). Their request and status
> >> bit is at the same register bit position.  Hope this will help you.
> > have you already discussed using only one reset cell?
> > if there's only one big reset controller in RCU then why not let the
> > reset controller driver do it's job of translating a reset line? also
> > this represents the hardware best (dt-bindings should describe the
> > hardware, drivers then translate that into the various subsystems
> > offered by the kernel).
> >
> > we have to translate it into:
> > - status register and bit
> > - reset register and bit
> >
> > for LGM the implementation seems to be the easiest because the reset
> > line can be mapped easily to the registers and bit offsets (for
> > example like reset-meson.c does it, which also supports 256 reset
> > lines together with for example
> > include/dt-bindings/reset/amlogic,meson-g12a-reset.h. the latter is
> > nice to have but optional)
> When we implement this driver, we checked other drivers(hisilicon/*,
> reset-berlin.c and etc). After evaluation, we think register offset and
> register bit are easier for users to understand and use if they follow
> the hardware spec.
just so I know how the documentation looks like:
does the hardware spec document 8 registers, each with (up to) the 32
reset lines in it?

reset-meson.c does it like that, but the difference there is that the
reset registers are continuous because there's no status register in
between
so your existing way of describing the reset line seems fine if Rob is
happy with it as well

> > we can then implement special translation logic (in other words: a
> > separate of_xlate callback) for VRX200 which then has to do more
> > "magic" (like you have shown in your example code above: "if the reset
> > line belongs to the second set of 32 reset lines then use reset offset
> > X and status offset Y" - or even use a translation table as
> > reset-imx7.c does)
> >
> > the current binding is a mix of specifying reset register and bit in
> > .dts but calculating the status register.
> > I missed the calculation of the status register until you pointed it out earlier
> But we still don't have a good solution for VRX200 status bit issues.
> Before we solve this issue, it is very difficult to use one driver for
OK, let me summarize what we have so far.

all SoC have the following "shared" logic so far:
- all reset_control_ops callbacks are the same on VRX200 and LGM
(assuming we fix the issues you found in the reset-lantiq.c
implementation)
- internally we should use regmap (LGM for accessing the secure
processor, earlier SoCs because the parent is a syscon)
- each reset line consists of a reset register offset and bit as well
as a status register offset and bit

however, we have differences in:
- how to map the registers (LGM maps the RCU registers directly while
earlier SoCs fetch the parent syscon)
- calculation of the status register
- calculation of the status bit

I see two ways to use one common driver for LGM and the earlier SoCs:

1) use a reset line mapping table as for example reset-imx7.c does.
this would include reset register, reset bit, status register and status bit.
LGM can use a macro to get rid of the duplication between status bit
and reset bit (and the status register offset if you prefer)
this case would use #reset-cells = <1> and we wouldn't need to
implement the of_xlate callback

2) on VRX200 (and probably the older SoCs as well) we can encode the
following information in one 32-bit value:
- reset register (max value: 0x48)
- status register (max value: 0x24)
- reset bit (max value: 32)
- status bit (max value: 32)

if this also works for LGM we can determine all required information
for a reset line in the of_xlate callback and translate it to one
32-bit value.
LGM and earlier SoCs would each use it's own of_xlate implementation.
the reset_control_ops callback would then unpack the 32-bit value
("unsigned long id") into the reset register and bit as well as status
register and bit (as needed)

both ways can work, but it depends on what the dt-bindings maintainers
(like Rob) think of the binding itself.
(dt-bindings follow what the hardware implements, the driver only does
the translation between a vendor specific binding and a given
subsystem)
so we first need Rob's ACK on the binding, then we can figure out the
best driver implementation for that binding


Martin


[0] https://lkml.org/lkml/2019/8/27/849

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

[Chuan Hua, Lei]

Hi Martin,

On 9/3/2019 6:04 AM, Martin Blumenstingl wrote:
> Hi,
>
> On Mon, Sep 2, 2019 at 11:45 AM Chuan Hua, Lei
> <chuanhua.lei@linux.intel.com> wrote:
>> Hi Martin,
>>
>>
>> On 9/2/2019 5:38 AM, Martin Blumenstingl wrote:
>>> Hi,
>>>
>>> On Fri, Aug 30, 2019 at 5:02 AM Chuan Hua, Lei
>>> <chuanhua.lei@linux.intel.com> wrote:
>>>> Hi Martin,
>>>>
>>>> On 8/30/2019 5:40 AM, Martin Blumenstingl wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei
>>>>> <chuanhua.lei@linux.intel.com> wrote:
>>>>>
>>>>>>> I'm not surprised that we got some of the IP block layout for the
>>>>>>> VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
>>>>>>> (BSP).
>>>>>>> with proper documentation (as in a "public datasheet for the SoC") it
>>>>>>> would be easy to spot these mistakes (at least I assume that the
>>>>>>> quality of the Infineon / Lantiq datasheets is excellent).
>>>>>>>
>>>>>>> back to reset-intel-syscon:
>>>>>>> assigning only one job to the RCU hardware is a good idea (in my opinion).
>>>>>>> that brings up a question: why do we need the "syscon" compatible for
>>>>>>> the RCU node?
>>>>>>> this is typically used when registers are accessed by another IP block
>>>>>>> and the other driver has to access these registers as well. does this
>>>>>>> mean that there's more hidden in the RCU registers?
>>>>>> As I mentioned, some other misc registers are put into RCU even they
>>>>>> don't belong to reset functions.
>>>>> OK, just be aware that there are also rules for syscon compatible
>>>>> drivers, see for example: [0]
>>>>> if Rob (dt-bindings maintainer) is happy with the documentation in
>>>>> patch 1 then I'm fine with it as well.
>>>>> for my own education I would appreciate if you could describe these
>>>>> "other misc registers" with a few sentences (I assume that this can
>>>>> also help Rob)
>>>> For LGM, RCU is clean. There would be no MISC register after software's
>>>> feedback. These misc registers will be moved to chiptop/misc
>>>> groups(implemented by syscon). For legacy SoC, we do have a lot MISC
>>>> registers for different SoCs.
>>> OK, I think I understand now: chiptop != RCU
>>> so RCU really only has one purpose: handling resets
>>> while chiptop manages all the random bits
>>>
>>> does this means we don't need RCU to match "syscon"?
>> If we don't support legacy SoC with the same driver, we don't need
>> syscon, just regmap. Regmap is a must for us since we will use regmap
>> proxy to implement secure rest via secure processor.
> I think we should drop the syscon compatible for LGM then
> even for the legacy SoCs the reset controller should not have a syscon
> compatible: instead it should have a syscon parent (as the current
> "lantiq,xrx200-reset" binding requires and as suggested by Rob for
> another IP block: [0])
I am not sure if syscon parent really matches hardware implementation. 
In all our Networking SoCs, chiptop is kind of misc register collection. 
Some registers can't belong to any particular group, or they need to 
work together with other modules(therefore, these misc registers would 
be accessed by two or more modules). However, chiptop is not a hardware 
module.
>
> keeping regmap is great in my opinion because it's a nice API and gets
> rid of some boilerplate
> even better if it makes things easier for accessing the secure processor
>
>>>>> [...]
>>>>>>>>>>>> 4. Code not optimized and intel internal review not assessed.
>>>>>>>>>>> insights from you (like the issue with the reset callback) are very
>>>>>>>>>>> valuable - this shows that we should focus on having one driver.
>>>>>>>>>>>
>>>>>>>>>>>> Based on the above findings, I would suggest reset-lantiq.c to move to
>>>>>>>>>>>> reset-intel-syscon.c
>>>>>>>>>>> my concern with having two separate drivers is that it will be hard to
>>>>>>>>>>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
>>>>>>>>>>> driver.
>>>>>>>>>>> I don't have access to the datasheets for the any Lantiq/Intel SoC
>>>>>>>>>>> (VRX200 and even older).
>>>>>>>>>>> so debugging issues after switching from one driver to another is
>>>>>>>>>>> tedious because I cannot tell which part of the driver is causing a
>>>>>>>>>>> problem (it's either "all code from driver A" vs "all code from driver
>>>>>>>>>>> B", meaning it's hard to narrow it down).
>>>>>>>>>>> with separate commits/patches that are improving the reset-lantiq
>>>>>>>>>>> driver I can do git bisect to find the cause of a problem on the older
>>>>>>>>>>> SoCs (VRX200 for example)
>>>>>>>>>> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
>>>>>>>>>> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
>>>>>>>>>> should be straight forward.
>>>>>>>>> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
>>>>>>>>> they only use level resets (_assert and _deassert) or are some reset
>>>>>>>>> lines using reset pulses (_reset)?
>>>>>>>>>
>>>>>>>>> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
>>>>>>>>> we still had to add support for the _reset callback as this is missing
>>>>>>>>> in reset-intel-syscon.c currently
>>>>>>>> Yes. We have reset pulse(assert, then check the reset status).
>>>>>>> only now I realized that the reset-intel-syscon driver does not seem
>>>>>>> to use the status registers (instead it's looking at the reset
>>>>>>> registers when checking the status).
>>>>>>> what happened to the status registers - do they still exist in newer
>>>>>>> SoCs (like LGM)? why are they not used?
>>>>>> Reset status check is there. regmap_read_poll_timeout to check status
>>>>>> big. Status register offset <4) from request register. For legacy, there
>>>>>> is one exception, we can add soc specific data to handle it.
>>>>> I see, thank you for the explanation
>>>>> this won't work on VRX200 for example because the status register is
>>>>> not always at (reset register - 0x4)
>>>> As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
>>>> with one soc data in the compatible array.
>>>>
>>>> For example(not same as upstream, but idea is similar)
>>>>
>>>> static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
>>>> {
>>>>        if (data->soc_data->legacy && req_off == RCU_RST_REQ)
>>>>            return RCU_RST_STAT;
>>>>        else
>>>>            return req_off + 0x4;
>>>> }
>>>>
>>>>>>> on VRX200 for example there seem to be some cases where the bits in
>>>>>>> the reset and status registers are different (for example: the first
>>>>>>> GPHY seems to use reset bit 31 but status bit 30)
>>>>>>> this is currently not supported in reset-intel-syscon
>>>>>> This is most tricky and ugly part for VRX200/Danube. Do you have any
>>>>>> idea to handle this nicely?
>>>>> with reset-lantiq we have the following register information:
>>>>> a) reset offset: first reg property
>>>>> b) status offset: second reg property
>>>>> c) reset bit: first #reset-cell
>>>>> d) status bit: second #reset-cell
>>>>>
>>>>> reset-intel-syscon derives half of this information from the two #reset-cells:
>>>>> a) reset offset: first #reset-cell
>>>>> b) status offset: reset offset - 0x4
>>>>> c) reset bit: second #reset-cell
>>>>> d) status bit: same as reset bit
>>>>>
>>>>> I cannot make any suggestion (yet) how to handle VRX200 and LGM in one
>>>>> driver because I don't know enough about LGM (yet).
>>>>> on VRX200 my understanding is that we have 64 reset bits (2x 32bit
>>>>> registers) and 64 status bits (also 2x 32bit registers). each reset
>>>>> bit has a corresponding status bit but the numbering may be different
>>>>> it's not clear to me how many resets LGM supports and how they are
>>>>> organized. for example: I think it makes a difference if "there are 64
>>>>> registers with each one reset bit" versus "there are two registers
>>>>> with 32 bits each"
>>>>> please share some details how it's organized internally, then I can
>>>>> try to come up with a suggestion.
>>>> LGM reset organization is more clean compared with legacy SoCs. We have
>>>> 8 x 32bit reset and status registers(more modules need to be reset,
>>>> overall ideas are similar without big change). Their request and status
>>>> bit is at the same register bit position.  Hope this will help you.
>>> have you already discussed using only one reset cell?
>>> if there's only one big reset controller in RCU then why not let the
>>> reset controller driver do it's job of translating a reset line? also
>>> this represents the hardware best (dt-bindings should describe the
>>> hardware, drivers then translate that into the various subsystems
>>> offered by the kernel).
>>>
>>> we have to translate it into:
>>> - status register and bit
>>> - reset register and bit
>>>
>>> for LGM the implementation seems to be the easiest because the reset
>>> line can be mapped easily to the registers and bit offsets (for
>>> example like reset-meson.c does it, which also supports 256 reset
>>> lines together with for example
>>> include/dt-bindings/reset/amlogic,meson-g12a-reset.h. the latter is
>>> nice to have but optional)
>> When we implement this driver, we checked other drivers(hisilicon/*,
>> reset-berlin.c and etc). After evaluation, we think register offset and
>> register bit are easier for users to understand and use if they follow
>> the hardware spec.
> just so I know how the documentation looks like:
> does the hardware spec document 8 registers, each with (up to) the 32
> reset lines in it?
>
> reset-meson.c does it like that, but the difference there is that the
> reset registers are continuous because there's no status register in
> between
> so your existing way of describing the reset line seems fine if Rob is
> happy with it as well
>
>>> we can then implement special translation logic (in other words: a
>>> separate of_xlate callback) for VRX200 which then has to do more
>>> "magic" (like you have shown in your example code above: "if the reset
>>> line belongs to the second set of 32 reset lines then use reset offset
>>> X and status offset Y" - or even use a translation table as
>>> reset-imx7.c does)
>>>
>>> the current binding is a mix of specifying reset register and bit in
>>> .dts but calculating the status register.
>>> I missed the calculation of the status register until you pointed it out earlier
>> But we still don't have a good solution for VRX200 status bit issues.
>> Before we solve this issue, it is very difficult to use one driver for
> OK, let me summarize what we have so far.
>
> all SoC have the following "shared" logic so far:
> - all reset_control_ops callbacks are the same on VRX200 and LGM
> (assuming we fix the issues you found in the reset-lantiq.c
> implementation)
> - internally we should use regmap (LGM for accessing the secure
> processor, earlier SoCs because the parent is a syscon)
> - each reset line consists of a reset register offset and bit as well
> as a status register offset and bit
>
> however, we have differences in:
> - how to map the registers (LGM maps the RCU registers directly while
> earlier SoCs fetch the parent syscon)
> - calculation of the status register
> - calculation of the status bit
>
> I see two ways to use one common driver for LGM and the earlier SoCs:
>
> 1) use a reset line mapping table as for example reset-imx7.c does.
> this would include reset register, reset bit, status register and status bit.
> LGM can use a macro to get rid of the duplication between status bit
> and reset bit (and the status register offset if you prefer)
> this case would use #reset-cells = <1> and we wouldn't need to
> implement the of_xlate callback
>
> 2) on VRX200 (and probably the older SoCs as well) we can encode the
> following information in one 32-bit value:
> - reset register (max value: 0x48)
> - status register (max value: 0x24)
> - reset bit (max value: 32)
> - status bit (max value: 32)
>
> if this also works for LGM we can determine all required information
> for a reset line in the of_xlate callback and translate it to one
> 32-bit value.
> LGM and earlier SoCs would each use it's own of_xlate implementation.
> the reset_control_ops callback would then unpack the 32-bit value
> ("unsigned long id") into the reset register and bit as well as status
> register and bit (as needed)
>
> both ways can work, but it depends on what the dt-bindings maintainers
> (like Rob) think of the binding itself.
> (dt-bindings follow what the hardware implements, the driver only does
> the translation between a vendor specific binding and a given
> subsystem)
> so we first need Rob's ACK on the binding, then we can figure out the
> best driver implementation for that binding
I will check with Dilip about how we should move forward. syscon parent 
is one issue that we have to solve first.
>
>
> Martin
>
>
> [0] https://lkml.org/lkml/2019/8/27/849

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

[Martin Blumenstingl]

Hi,

On Thu, Sep 5, 2019 at 4:38 AM Chuan Hua, Lei
<chuanhua.lei@linux.intel.com> wrote:
[...]
> >>>>>>> I'm not surprised that we got some of the IP block layout for the
> >>>>>>> VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
> >>>>>>> (BSP).
> >>>>>>> with proper documentation (as in a "public datasheet for the SoC") it
> >>>>>>> would be easy to spot these mistakes (at least I assume that the
> >>>>>>> quality of the Infineon / Lantiq datasheets is excellent).
> >>>>>>>
> >>>>>>> back to reset-intel-syscon:
> >>>>>>> assigning only one job to the RCU hardware is a good idea (in my opinion).
> >>>>>>> that brings up a question: why do we need the "syscon" compatible for
> >>>>>>> the RCU node?
> >>>>>>> this is typically used when registers are accessed by another IP block
> >>>>>>> and the other driver has to access these registers as well. does this
> >>>>>>> mean that there's more hidden in the RCU registers?
> >>>>>> As I mentioned, some other misc registers are put into RCU even they
> >>>>>> don't belong to reset functions.
> >>>>> OK, just be aware that there are also rules for syscon compatible
> >>>>> drivers, see for example: [0]
> >>>>> if Rob (dt-bindings maintainer) is happy with the documentation in
> >>>>> patch 1 then I'm fine with it as well.
> >>>>> for my own education I would appreciate if you could describe these
> >>>>> "other misc registers" with a few sentences (I assume that this can
> >>>>> also help Rob)
> >>>> For LGM, RCU is clean. There would be no MISC register after software's
> >>>> feedback. These misc registers will be moved to chiptop/misc
> >>>> groups(implemented by syscon). For legacy SoC, we do have a lot MISC
> >>>> registers for different SoCs.
> >>> OK, I think I understand now: chiptop != RCU
> >>> so RCU really only has one purpose: handling resets
> >>> while chiptop manages all the random bits
> >>>
> >>> does this means we don't need RCU to match "syscon"?
> >> If we don't support legacy SoC with the same driver, we don't need
> >> syscon, just regmap. Regmap is a must for us since we will use regmap
> >> proxy to implement secure rest via secure processor.
> > I think we should drop the syscon compatible for LGM then
> > even for the legacy SoCs the reset controller should not have a syscon
> > compatible: instead it should have a syscon parent (as the current
> > "lantiq,xrx200-reset" binding requires and as suggested by Rob for
> > another IP block: [0])
> I am not sure if syscon parent really matches hardware implementation.
> In all our Networking SoCs, chiptop is kind of misc register collection.
> Some registers can't belong to any particular group, or they need to
> work together with other modules(therefore, these misc registers would
> be accessed by two or more modules). However, chiptop is not a hardware
> module.
indeed, chiptop should not have any child nodes (based on your explanation).
I was referring to VRX200 where the RCU syscon has various children
(one child node for each hardware module that's part of RCU: reset
controller, 2x USB PHY, ...)

back to LGM:
you said that the LGM RCU registers only contain the reset controller.
thus I see no need for the syscon compatible

> >
> > keeping regmap is great in my opinion because it's a nice API and gets
> > rid of some boilerplate
> > even better if it makes things easier for accessing the secure processor
> >
> >>>>> [...]
> >>>>>>>>>>>> 4. Code not optimized and intel internal review not assessed.
> >>>>>>>>>>> insights from you (like the issue with the reset callback) are very
> >>>>>>>>>>> valuable - this shows that we should focus on having one driver.
> >>>>>>>>>>>
> >>>>>>>>>>>> Based on the above findings, I would suggest reset-lantiq.c to move to
> >>>>>>>>>>>> reset-intel-syscon.c
> >>>>>>>>>>> my concern with having two separate drivers is that it will be hard to
> >>>>>>>>>>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
> >>>>>>>>>>> driver.
> >>>>>>>>>>> I don't have access to the datasheets for the any Lantiq/Intel SoC
> >>>>>>>>>>> (VRX200 and even older).
> >>>>>>>>>>> so debugging issues after switching from one driver to another is
> >>>>>>>>>>> tedious because I cannot tell which part of the driver is causing a
> >>>>>>>>>>> problem (it's either "all code from driver A" vs "all code from driver
> >>>>>>>>>>> B", meaning it's hard to narrow it down).
> >>>>>>>>>>> with separate commits/patches that are improving the reset-lantiq
> >>>>>>>>>>> driver I can do git bisect to find the cause of a problem on the older
> >>>>>>>>>>> SoCs (VRX200 for example)
> >>>>>>>>>> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
> >>>>>>>>>> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
> >>>>>>>>>> should be straight forward.
> >>>>>>>>> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
> >>>>>>>>> they only use level resets (_assert and _deassert) or are some reset
> >>>>>>>>> lines using reset pulses (_reset)?
> >>>>>>>>>
> >>>>>>>>> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
> >>>>>>>>> we still had to add support for the _reset callback as this is missing
> >>>>>>>>> in reset-intel-syscon.c currently
> >>>>>>>> Yes. We have reset pulse(assert, then check the reset status).
> >>>>>>> only now I realized that the reset-intel-syscon driver does not seem
> >>>>>>> to use the status registers (instead it's looking at the reset
> >>>>>>> registers when checking the status).
> >>>>>>> what happened to the status registers - do they still exist in newer
> >>>>>>> SoCs (like LGM)? why are they not used?
> >>>>>> Reset status check is there. regmap_read_poll_timeout to check status
> >>>>>> big. Status register offset <4) from request register. For legacy, there
> >>>>>> is one exception, we can add soc specific data to handle it.
> >>>>> I see, thank you for the explanation
> >>>>> this won't work on VRX200 for example because the status register is
> >>>>> not always at (reset register - 0x4)
> >>>> As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
> >>>> with one soc data in the compatible array.
> >>>>
> >>>> For example(not same as upstream, but idea is similar)
> >>>>
> >>>> static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
> >>>> {
> >>>>        if (data->soc_data->legacy && req_off == RCU_RST_REQ)
> >>>>            return RCU_RST_STAT;
> >>>>        else
> >>>>            return req_off + 0x4;
> >>>> }
> >>>>
> >>>>>>> on VRX200 for example there seem to be some cases where the bits in
> >>>>>>> the reset and status registers are different (for example: the first
> >>>>>>> GPHY seems to use reset bit 31 but status bit 30)
> >>>>>>> this is currently not supported in reset-intel-syscon
> >>>>>> This is most tricky and ugly part for VRX200/Danube. Do you have any
> >>>>>> idea to handle this nicely?
> >>>>> with reset-lantiq we have the following register information:
> >>>>> a) reset offset: first reg property
> >>>>> b) status offset: second reg property
> >>>>> c) reset bit: first #reset-cell
> >>>>> d) status bit: second #reset-cell
> >>>>>
> >>>>> reset-intel-syscon derives half of this information from the two #reset-cells:
> >>>>> a) reset offset: first #reset-cell
> >>>>> b) status offset: reset offset - 0x4
> >>>>> c) reset bit: second #reset-cell
> >>>>> d) status bit: same as reset bit
> >>>>>
> >>>>> I cannot make any suggestion (yet) how to handle VRX200 and LGM in one
> >>>>> driver because I don't know enough about LGM (yet).
> >>>>> on VRX200 my understanding is that we have 64 reset bits (2x 32bit
> >>>>> registers) and 64 status bits (also 2x 32bit registers). each reset
> >>>>> bit has a corresponding status bit but the numbering may be different
> >>>>> it's not clear to me how many resets LGM supports and how they are
> >>>>> organized. for example: I think it makes a difference if "there are 64
> >>>>> registers with each one reset bit" versus "there are two registers
> >>>>> with 32 bits each"
> >>>>> please share some details how it's organized internally, then I can
> >>>>> try to come up with a suggestion.
> >>>> LGM reset organization is more clean compared with legacy SoCs. We have
> >>>> 8 x 32bit reset and status registers(more modules need to be reset,
> >>>> overall ideas are similar without big change). Their request and status
> >>>> bit is at the same register bit position.  Hope this will help you.
> >>> have you already discussed using only one reset cell?
> >>> if there's only one big reset controller in RCU then why not let the
> >>> reset controller driver do it's job of translating a reset line? also
> >>> this represents the hardware best (dt-bindings should describe the
> >>> hardware, drivers then translate that into the various subsystems
> >>> offered by the kernel).
> >>>
> >>> we have to translate it into:
> >>> - status register and bit
> >>> - reset register and bit
> >>>
> >>> for LGM the implementation seems to be the easiest because the reset
> >>> line can be mapped easily to the registers and bit offsets (for
> >>> example like reset-meson.c does it, which also supports 256 reset
> >>> lines together with for example
> >>> include/dt-bindings/reset/amlogic,meson-g12a-reset.h. the latter is
> >>> nice to have but optional)
> >> When we implement this driver, we checked other drivers(hisilicon/*,
> >> reset-berlin.c and etc). After evaluation, we think register offset and
> >> register bit are easier for users to understand and use if they follow
> >> the hardware spec.
> > just so I know how the documentation looks like:
> > does the hardware spec document 8 registers, each with (up to) the 32
> > reset lines in it?
> >
> > reset-meson.c does it like that, but the difference there is that the
> > reset registers are continuous because there's no status register in
> > between
> > so your existing way of describing the reset line seems fine if Rob is
> > happy with it as well
> >
> >>> we can then implement special translation logic (in other words: a
> >>> separate of_xlate callback) for VRX200 which then has to do more
> >>> "magic" (like you have shown in your example code above: "if the reset
> >>> line belongs to the second set of 32 reset lines then use reset offset
> >>> X and status offset Y" - or even use a translation table as
> >>> reset-imx7.c does)
> >>>
> >>> the current binding is a mix of specifying reset register and bit in
> >>> .dts but calculating the status register.
> >>> I missed the calculation of the status register until you pointed it out earlier
> >> But we still don't have a good solution for VRX200 status bit issues.
> >> Before we solve this issue, it is very difficult to use one driver for
> > OK, let me summarize what we have so far.
> >
> > all SoC have the following "shared" logic so far:
> > - all reset_control_ops callbacks are the same on VRX200 and LGM
> > (assuming we fix the issues you found in the reset-lantiq.c
> > implementation)
> > - internally we should use regmap (LGM for accessing the secure
> > processor, earlier SoCs because the parent is a syscon)
> > - each reset line consists of a reset register offset and bit as well
> > as a status register offset and bit
> >
> > however, we have differences in:
> > - how to map the registers (LGM maps the RCU registers directly while
> > earlier SoCs fetch the parent syscon)
> > - calculation of the status register
> > - calculation of the status bit
> >
> > I see two ways to use one common driver for LGM and the earlier SoCs:
> >
> > 1) use a reset line mapping table as for example reset-imx7.c does.
> > this would include reset register, reset bit, status register and status bit.
> > LGM can use a macro to get rid of the duplication between status bit
> > and reset bit (and the status register offset if you prefer)
> > this case would use #reset-cells = <1> and we wouldn't need to
> > implement the of_xlate callback
> >
> > 2) on VRX200 (and probably the older SoCs as well) we can encode the
> > following information in one 32-bit value:
> > - reset register (max value: 0x48)
> > - status register (max value: 0x24)
> > - reset bit (max value: 32)
> > - status bit (max value: 32)
> >
> > if this also works for LGM we can determine all required information
> > for a reset line in the of_xlate callback and translate it to one
> > 32-bit value.
> > LGM and earlier SoCs would each use it's own of_xlate implementation.
> > the reset_control_ops callback would then unpack the 32-bit value
> > ("unsigned long id") into the reset register and bit as well as status
> > register and bit (as needed)
> >
> > both ways can work, but it depends on what the dt-bindings maintainers
> > (like Rob) think of the binding itself.
> > (dt-bindings follow what the hardware implements, the driver only does
> > the translation between a vendor specific binding and a given
> > subsystem)
> > so we first need Rob's ACK on the binding, then we can figure out the
> > best driver implementation for that binding
> I will check with Dilip about how we should move forward. syscon parent
> is one issue that we have to solve first.
agreed, let's define the binding first


Martin

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

[Dilip Kota]

Re-sending the mail, because of delivery failure.
sorry for the spam.

Hi Martin,

On 9/6/2019 4:53 AM, Martin Blumenstingl wrote:
> Hi,
>
> On Thu, Sep 5, 2019 at 4:38 AM Chuan Hua, Lei
> <chuanhua.lei@linux.intel.com> wrote:
> [...]
>>>>>>>>> I'm not surprised that we got some of the IP block layout for the
>>>>>>>>> VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
>>>>>>>>> (BSP).
>>>>>>>>> with proper documentation (as in a "public datasheet for the SoC") it
>>>>>>>>> would be easy to spot these mistakes (at least I assume that the
>>>>>>>>> quality of the Infineon / Lantiq datasheets is excellent).
>>>>>>>>>
>>>>>>>>> back to reset-intel-syscon:
>>>>>>>>> assigning only one job to the RCU hardware is a good idea (in my opinion).
>>>>>>>>> that brings up a question: why do we need the "syscon" compatible for
>>>>>>>>> the RCU node?
>>>>>>>>> this is typically used when registers are accessed by another IP block
>>>>>>>>> and the other driver has to access these registers as well. does this
>>>>>>>>> mean that there's more hidden in the RCU registers?
>>>>>>>> As I mentioned, some other misc registers are put into RCU even they
>>>>>>>> don't belong to reset functions.
>>>>>>> OK, just be aware that there are also rules for syscon compatible
>>>>>>> drivers, see for example: [0]
>>>>>>> if Rob (dt-bindings maintainer) is happy with the documentation in
>>>>>>> patch 1 then I'm fine with it as well.
>>>>>>> for my own education I would appreciate if you could describe these
>>>>>>> "other misc registers" with a few sentences (I assume that this can
>>>>>>> also help Rob)
>>>>>> For LGM, RCU is clean. There would be no MISC register after software's
>>>>>> feedback. These misc registers will be moved to chiptop/misc
>>>>>> groups(implemented by syscon). For legacy SoC, we do have a lot MISC
>>>>>> registers for different SoCs.
>>>>> OK, I think I understand now: chiptop != RCU
>>>>> so RCU really only has one purpose: handling resets
>>>>> while chiptop manages all the random bits
>>>>>
>>>>> does this means we don't need RCU to match "syscon"?
>>>> If we don't support legacy SoC with the same driver, we don't need
>>>> syscon, just regmap. Regmap is a must for us since we will use regmap
>>>> proxy to implement secure rest via secure processor.
>>> I think we should drop the syscon compatible for LGM then
>>> even for the legacy SoCs the reset controller should not have a syscon
>>> compatible: instead it should have a syscon parent (as the current
>>> "lantiq,xrx200-reset" binding requires and as suggested by Rob for
>>> another IP block: [0])
>> I am not sure if syscon parent really matches hardware implementation.
>> In all our Networking SoCs, chiptop is kind of misc register collection.
>> Some registers can't belong to any particular group, or they need to
>> work together with other modules(therefore, these misc registers would
>> be accessed by two or more modules). However, chiptop is not a hardware
>> module.
> indeed, chiptop should not have any child nodes (based on your explanation).
> I was referring to VRX200 where the RCU syscon has various children
> (one child node for each hardware module that's part of RCU: reset
> controller, 2x USB PHY, ...)
>
> back to LGM:
> you said that the LGM RCU registers only contain the reset controller.
> thus I see no need for the syscon compatible
>
>>> keeping regmap is great in my opinion because it's a nice API and gets
>>> rid of some boilerplate
>>> even better if it makes things easier for accessing the secure processor
>>>
>>>>>>> [...]
>>>>>>>>>>>>>> 4. Code not optimized and intel internal review not assessed.
>>>>>>>>>>>>> insights from you (like the issue with the reset callback) are very
>>>>>>>>>>>>> valuable - this shows that we should focus on having one driver.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Based on the above findings, I would suggest reset-lantiq.c to move to
>>>>>>>>>>>>>> reset-intel-syscon.c
>>>>>>>>>>>>> my concern with having two separate drivers is that it will be hard to
>>>>>>>>>>>>> migrate from reset-lantiq to the "optimized" reset-intel-syscon
>>>>>>>>>>>>> driver.
>>>>>>>>>>>>> I don't have access to the datasheets for the any Lantiq/Intel SoC
>>>>>>>>>>>>> (VRX200 and even older).
>>>>>>>>>>>>> so debugging issues after switching from one driver to another is
>>>>>>>>>>>>> tedious because I cannot tell which part of the driver is causing a
>>>>>>>>>>>>> problem (it's either "all code from driver A" vs "all code from driver
>>>>>>>>>>>>> B", meaning it's hard to narrow it down).
>>>>>>>>>>>>> with separate commits/patches that are improving the reset-lantiq
>>>>>>>>>>>>> driver I can do git bisect to find the cause of a problem on the older
>>>>>>>>>>>>> SoCs (VRX200 for example)
>>>>>>>>>>>> Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
>>>>>>>>>>>> latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
>>>>>>>>>>>> should be straight forward.
>>>>>>>>>>> what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
>>>>>>>>>>> they only use level resets (_assert and _deassert) or are some reset
>>>>>>>>>>> lines using reset pulses (_reset)?
>>>>>>>>>>>
>>>>>>>>>>> when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
>>>>>>>>>>> we still had to add support for the _reset callback as this is missing
>>>>>>>>>>> in reset-intel-syscon.c currently
>>>>>>>>>> Yes. We have reset pulse(assert, then check the reset status).
>>>>>>>>> only now I realized that the reset-intel-syscon driver does not seem
>>>>>>>>> to use the status registers (instead it's looking at the reset
>>>>>>>>> registers when checking the status).
>>>>>>>>> what happened to the status registers - do they still exist in newer
>>>>>>>>> SoCs (like LGM)? why are they not used?
>>>>>>>> Reset status check is there. regmap_read_poll_timeout to check status
>>>>>>>> big. Status register offset <4) from request register. For legacy, there
>>>>>>>> is one exception, we can add soc specific data to handle it.
>>>>>>> I see, thank you for the explanation
>>>>>>> this won't work on VRX200 for example because the status register is
>>>>>>> not always at (reset register - 0x4)
>>>>>> As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
>>>>>> with one soc data in the compatible array.
>>>>>>
>>>>>> For example(not same as upstream, but idea is similar)
>>>>>>
>>>>>> static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
>>>>>> {
>>>>>>         if (data->soc_data->legacy && req_off == RCU_RST_REQ)
>>>>>>             return RCU_RST_STAT;
>>>>>>         else
>>>>>>             return req_off + 0x4;
>>>>>> }
>>>>>>
>>>>>>>>> on VRX200 for example there seem to be some cases where the bits in
>>>>>>>>> the reset and status registers are different (for example: the first
>>>>>>>>> GPHY seems to use reset bit 31 but status bit 30)
>>>>>>>>> this is currently not supported in reset-intel-syscon
>>>>>>>> This is most tricky and ugly part for VRX200/Danube. Do you have any
>>>>>>>> idea to handle this nicely?
>>>>>>> with reset-lantiq we have the following register information:
>>>>>>> a) reset offset: first reg property
>>>>>>> b) status offset: second reg property
>>>>>>> c) reset bit: first #reset-cell
>>>>>>> d) status bit: second #reset-cell
>>>>>>>
>>>>>>> reset-intel-syscon derives half of this information from the two #reset-cells:
>>>>>>> a) reset offset: first #reset-cell
>>>>>>> b) status offset: reset offset - 0x4
>>>>>>> c) reset bit: second #reset-cell
>>>>>>> d) status bit: same as reset bit
>>>>>>>
>>>>>>> I cannot make any suggestion (yet) how to handle VRX200 and LGM in one
>>>>>>> driver because I don't know enough about LGM (yet).
>>>>>>> on VRX200 my understanding is that we have 64 reset bits (2x 32bit
>>>>>>> registers) and 64 status bits (also 2x 32bit registers). each reset
>>>>>>> bit has a corresponding status bit but the numbering may be different
>>>>>>> it's not clear to me how many resets LGM supports and how they are
>>>>>>> organized. for example: I think it makes a difference if "there are 64
>>>>>>> registers with each one reset bit" versus "there are two registers
>>>>>>> with 32 bits each"
>>>>>>> please share some details how it's organized internally, then I can
>>>>>>> try to come up with a suggestion.
>>>>>> LGM reset organization is more clean compared with legacy SoCs. We have
>>>>>> 8 x 32bit reset and status registers(more modules need to be reset,
>>>>>> overall ideas are similar without big change). Their request and status
>>>>>> bit is at the same register bit position.  Hope this will help you.
>>>>> have you already discussed using only one reset cell?
>>>>> if there's only one big reset controller in RCU then why not let the
>>>>> reset controller driver do it's job of translating a reset line? also
>>>>> this represents the hardware best (dt-bindings should describe the
>>>>> hardware, drivers then translate that into the various subsystems
>>>>> offered by the kernel).
>>>>>
>>>>> we have to translate it into:
>>>>> - status register and bit
>>>>> - reset register and bit
>>>>>
>>>>> for LGM the implementation seems to be the easiest because the reset
>>>>> line can be mapped easily to the registers and bit offsets (for
>>>>> example like reset-meson.c does it, which also supports 256 reset
>>>>> lines together with for example
>>>>> include/dt-bindings/reset/amlogic,meson-g12a-reset.h. the latter is
>>>>> nice to have but optional)
>>>> When we implement this driver, we checked other drivers(hisilicon/*,
>>>> reset-berlin.c and etc). After evaluation, we think register offset and
>>>> register bit are easier for users to understand and use if they follow
>>>> the hardware spec.
>>> just so I know how the documentation looks like:
>>> does the hardware spec document 8 registers, each with (up to) the 32
>>> reset lines in it?
>>>
>>> reset-meson.c does it like that, but the difference there is that the
>>> reset registers are continuous because there's no status register in
>>> between
>>> so your existing way of describing the reset line seems fine if Rob is
>>> happy with it as well
>>>
>>>>> we can then implement special translation logic (in other words: a
>>>>> separate of_xlate callback) for VRX200 which then has to do more
>>>>> "magic" (like you have shown in your example code above: "if the reset
>>>>> line belongs to the second set of 32 reset lines then use reset offset
>>>>> X and status offset Y" - or even use a translation table as
>>>>> reset-imx7.c does)
>>>>>
>>>>> the current binding is a mix of specifying reset register and bit in
>>>>> .dts but calculating the status register.
>>>>> I missed the calculation of the status register until you pointed it out earlier
>>>> But we still don't have a good solution for VRX200 status bit issues.
>>>> Before we solve this issue, it is very difficult to use one driver for
>>> OK, let me summarize what we have so far.
>>>
>>> all SoC have the following "shared" logic so far:
>>> - all reset_control_ops callbacks are the same on VRX200 and LGM
>>> (assuming we fix the issues you found in the reset-lantiq.c
>>> implementation)
>>> - internally we should use regmap (LGM for accessing the secure
>>> processor, earlier SoCs because the parent is a syscon)
>>> - each reset line consists of a reset register offset and bit as well
>>> as a status register offset and bit
>>>
>>> however, we have differences in:
>>> - how to map the registers (LGM maps the RCU registers directly while
>>> earlier SoCs fetch the parent syscon)
>>> - calculation of the status register
>>> - calculation of the status bit
>>>
>>> I see two ways to use one common driver for LGM and the earlier SoCs:
>>>
>>> 1) use a reset line mapping table as for example reset-imx7.c does.
>>> this would include reset register, reset bit, status register and status bit.
>>> LGM can use a macro to get rid of the duplication between status bit
>>> and reset bit (and the status register offset if you prefer)
>>> this case would use #reset-cells = <1> and we wouldn't need to
>>> implement the of_xlate callback
>>>
>>> 2) on VRX200 (and probably the older SoCs as well) we can encode the
>>> following information in one 32-bit value:
>>> - reset register (max value: 0x48)
>>> - status register (max value: 0x24)
>>> - reset bit (max value: 32)
>>> - status bit (max value: 32)
>>>
>>> if this also works for LGM we can determine all required information
>>> for a reset line in the of_xlate callback and translate it to one
>>> 32-bit value.
>>> LGM and earlier SoCs would each use it's own of_xlate implementation.
>>> the reset_control_ops callback would then unpack the 32-bit value
>>> ("unsigned long id") into the reset register and bit as well as status
>>> register and bit (as needed)
>>>
>>> both ways can work, but it depends on what the dt-bindings maintainers
>>> (like Rob) think of the binding itself.
>>> (dt-bindings follow what the hardware implements, the driver only does
>>> the translation between a vendor specific binding and a given
>>> subsystem)
>>> so we first need Rob's ACK on the binding, then we can figure out the
>>> best driver implementation for that binding
>> I will check with Dilip about how we should move forward. syscon parent
>> is one issue that we have to solve first.
> agreed, let's define the binding first

Lantiq reset driver and dtsi bindings are designed with an understanding 
of 2 reset controllers and they are children of the multifunction device 
RCU(reset controller unit)[0], which is not the case. Hardware wise 
there is only one reset controller and it is RCU which is not a 
multifunction device. intel-reset-syscon.c is defined as per the hardware.

The major difference between the vrx200 and lgm is:
1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm 
has one single register region.
2.) Register offsets and bit offsets are different.

So enhancing the intel-reset-syscon.c to provide compatibility/support 
for vrx200.
Please check the below dtsi binding proposal and let me know your view.

rcu0:reset-controller@00000000 {
     compatible= "intel,rcu-lgm";
     reg = <0x0000000 0x80000>, <reg_set2 size>, <reg_set3 size>, 
<reg_set4 size>;
    intel,global-reset = <0x10 30>;
    #reset-cells = <3>;
};

"#reset-cells":
   const:3
   description: |
     The 1st cell is the reset register offset.
     The 2nd cell is the reset set bit offset.
     The 3rd cell is the reset status bit offset.

Reset driver takes care of parsing the register address "reg" as per the 
".data" structure in struct of_device_id.
Reset driver takes care of traversing the status register offset.

Regards,
Dilip

[0]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mips/lantiq/rcu.txt?h=v5.3-rc8

>
>
> Martin

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

[Dilip Kota]

Hi Martin,

On 9/12/2019 2:38 PM, Dilip Kota wrote:
> Re-sending the mail, because of delivery failure.
> sorry for the spam.
>
> Hi Martin,
>
> On 9/6/2019 4:53 AM, Martin Blumenstingl wrote:
>> Hi,
>>
>> On Thu, Sep 5, 2019 at 4:38 AM Chuan Hua, Lei
>> <chuanhua.lei@linux.intel.com> wrote:
>> [...]
>>>>>>>>>> I'm not surprised that we got some of the IP block layout for 
>>>>>>>>>> the
>>>>>>>>>> VRX200 RCU "wrong" - all "documentation" we have is the old 
>>>>>>>>>> Lantiq UGW
>>>>>>>>>> (BSP).
>>>>>>>>>> with proper documentation (as in a "public datasheet for the 
>>>>>>>>>> SoC") it
>>>>>>>>>> would be easy to spot these mistakes (at least I assume that the
>>>>>>>>>> quality of the Infineon / Lantiq datasheets is excellent).
>>>>>>>>>>
>>>>>>>>>> back to reset-intel-syscon:
>>>>>>>>>> assigning only one job to the RCU hardware is a good idea (in 
>>>>>>>>>> my opinion).
>>>>>>>>>> that brings up a question: why do we need the "syscon" 
>>>>>>>>>> compatible for
>>>>>>>>>> the RCU node?
>>>>>>>>>> this is typically used when registers are accessed by another 
>>>>>>>>>> IP block
>>>>>>>>>> and the other driver has to access these registers as well. 
>>>>>>>>>> does this
>>>>>>>>>> mean that there's more hidden in the RCU registers?
>>>>>>>>> As I mentioned, some other misc registers are put into RCU 
>>>>>>>>> even they
>>>>>>>>> don't belong to reset functions.
>>>>>>>> OK, just be aware that there are also rules for syscon compatible
>>>>>>>> drivers, see for example: [0]
>>>>>>>> if Rob (dt-bindings maintainer) is happy with the documentation in
>>>>>>>> patch 1 then I'm fine with it as well.
>>>>>>>> for my own education I would appreciate if you could describe 
>>>>>>>> these
>>>>>>>> "other misc registers" with a few sentences (I assume that this 
>>>>>>>> can
>>>>>>>> also help Rob)
>>>>>>> For LGM, RCU is clean. There would be no MISC register after 
>>>>>>> software's
>>>>>>> feedback. These misc registers will be moved to chiptop/misc
>>>>>>> groups(implemented by syscon). For legacy SoC, we do have a lot 
>>>>>>> MISC
>>>>>>> registers for different SoCs.
>>>>>> OK, I think I understand now: chiptop != RCU
>>>>>> so RCU really only has one purpose: handling resets
>>>>>> while chiptop manages all the random bits
>>>>>>
>>>>>> does this means we don't need RCU to match "syscon"?
>>>>> If we don't support legacy SoC with the same driver, we don't need
>>>>> syscon, just regmap. Regmap is a must for us since we will use regmap
>>>>> proxy to implement secure rest via secure processor.
>>>> I think we should drop the syscon compatible for LGM then
>>>> even for the legacy SoCs the reset controller should not have a syscon
>>>> compatible: instead it should have a syscon parent (as the current
>>>> "lantiq,xrx200-reset" binding requires and as suggested by Rob for
>>>> another IP block: [0])
>>> I am not sure if syscon parent really matches hardware implementation.
>>> In all our Networking SoCs, chiptop is kind of misc register 
>>> collection.
>>> Some registers can't belong to any particular group, or they need to
>>> work together with other modules(therefore, these misc registers would
>>> be accessed by two or more modules). However, chiptop is not a hardware
>>> module.
>> indeed, chiptop should not have any child nodes (based on your 
>> explanation).
>> I was referring to VRX200 where the RCU syscon has various children
>> (one child node for each hardware module that's part of RCU: reset
>> controller, 2x USB PHY, ...)
>>
>> back to LGM:
>> you said that the LGM RCU registers only contain the reset controller.
>> thus I see no need for the syscon compatible
>>
>>>> keeping regmap is great in my opinion because it's a nice API and gets
>>>> rid of some boilerplate
>>>> even better if it makes things easier for accessing the secure 
>>>> processor
>>>>
>>>>>>>> [...]
>>>>>>>>>>>>>>> 4. Code not optimized and intel internal review not 
>>>>>>>>>>>>>>> assessed.
>>>>>>>>>>>>>> insights from you (like the issue with the reset 
>>>>>>>>>>>>>> callback) are very
>>>>>>>>>>>>>> valuable - this shows that we should focus on having one 
>>>>>>>>>>>>>> driver.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Based on the above findings, I would suggest 
>>>>>>>>>>>>>>> reset-lantiq.c to move to
>>>>>>>>>>>>>>> reset-intel-syscon.c
>>>>>>>>>>>>>> my concern with having two separate drivers is that it 
>>>>>>>>>>>>>> will be hard to
>>>>>>>>>>>>>> migrate from reset-lantiq to the "optimized" 
>>>>>>>>>>>>>> reset-intel-syscon
>>>>>>>>>>>>>> driver.
>>>>>>>>>>>>>> I don't have access to the datasheets for the any 
>>>>>>>>>>>>>> Lantiq/Intel SoC
>>>>>>>>>>>>>> (VRX200 and even older).
>>>>>>>>>>>>>> so debugging issues after switching from one driver to 
>>>>>>>>>>>>>> another is
>>>>>>>>>>>>>> tedious because I cannot tell which part of the driver is 
>>>>>>>>>>>>>> causing a
>>>>>>>>>>>>>> problem (it's either "all code from driver A" vs "all 
>>>>>>>>>>>>>> code from driver
>>>>>>>>>>>>>> B", meaning it's hard to narrow it down).
>>>>>>>>>>>>>> with separate commits/patches that are improving the 
>>>>>>>>>>>>>> reset-lantiq
>>>>>>>>>>>>>> driver I can do git bisect to find the cause of a problem 
>>>>>>>>>>>>>> on the older
>>>>>>>>>>>>>> SoCs (VRX200 for example)
>>>>>>>>>>>>> Our internal version supports XRX350/XRX500/PRX300(MIPS 
>>>>>>>>>>>>> based) and
>>>>>>>>>>>>> latest Lighting Mountain(X86 based). Migration to 
>>>>>>>>>>>>> reset-intel-syscon.c
>>>>>>>>>>>>> should be straight forward.
>>>>>>>>>>>> what about the _reset callback on the XRX350/XRX500/PRX300 
>>>>>>>>>>>> SoCs - do
>>>>>>>>>>>> they only use level resets (_assert and _deassert) or are 
>>>>>>>>>>>> some reset
>>>>>>>>>>>> lines using reset pulses (_reset)?
>>>>>>>>>>>>
>>>>>>>>>>>> when we wanted to switch from reset-lantiq.c to 
>>>>>>>>>>>> reset-intel-syscon.c
>>>>>>>>>>>> we still had to add support for the _reset callback as this 
>>>>>>>>>>>> is missing
>>>>>>>>>>>> in reset-intel-syscon.c currently
>>>>>>>>>>> Yes. We have reset pulse(assert, then check the reset status).
>>>>>>>>>> only now I realized that the reset-intel-syscon driver does 
>>>>>>>>>> not seem
>>>>>>>>>> to use the status registers (instead it's looking at the reset
>>>>>>>>>> registers when checking the status).
>>>>>>>>>> what happened to the status registers - do they still exist 
>>>>>>>>>> in newer
>>>>>>>>>> SoCs (like LGM)? why are they not used?
>>>>>>>>> Reset status check is there. regmap_read_poll_timeout to check 
>>>>>>>>> status
>>>>>>>>> big. Status register offset <4) from request register. For 
>>>>>>>>> legacy, there
>>>>>>>>> is one exception, we can add soc specific data to handle it.
>>>>>>>> I see, thank you for the explanation
>>>>>>>> this won't work on VRX200 for example because the status 
>>>>>>>> register is
>>>>>>>> not always at (reset register - 0x4)
>>>>>>> As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be 
>>>>>>> solved
>>>>>>> with one soc data in the compatible array.
>>>>>>>
>>>>>>> For example(not same as upstream, but idea is similar)
>>>>>>>
>>>>>>> static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 
>>>>>>> req_off)
>>>>>>> {
>>>>>>>         if (data->soc_data->legacy && req_off == RCU_RST_REQ)
>>>>>>>             return RCU_RST_STAT;
>>>>>>>         else
>>>>>>>             return req_off + 0x4;
>>>>>>> }
>>>>>>>
>>>>>>>>>> on VRX200 for example there seem to be some cases where the 
>>>>>>>>>> bits in
>>>>>>>>>> the reset and status registers are different (for example: 
>>>>>>>>>> the first
>>>>>>>>>> GPHY seems to use reset bit 31 but status bit 30)
>>>>>>>>>> this is currently not supported in reset-intel-syscon
>>>>>>>>> This is most tricky and ugly part for VRX200/Danube. Do you 
>>>>>>>>> have any
>>>>>>>>> idea to handle this nicely?
>>>>>>>> with reset-lantiq we have the following register information:
>>>>>>>> a) reset offset: first reg property
>>>>>>>> b) status offset: second reg property
>>>>>>>> c) reset bit: first #reset-cell
>>>>>>>> d) status bit: second #reset-cell
>>>>>>>>
>>>>>>>> reset-intel-syscon derives half of this information from the 
>>>>>>>> two #reset-cells:
>>>>>>>> a) reset offset: first #reset-cell
>>>>>>>> b) status offset: reset offset - 0x4
>>>>>>>> c) reset bit: second #reset-cell
>>>>>>>> d) status bit: same as reset bit
>>>>>>>>
>>>>>>>> I cannot make any suggestion (yet) how to handle VRX200 and LGM 
>>>>>>>> in one
>>>>>>>> driver because I don't know enough about LGM (yet).
>>>>>>>> on VRX200 my understanding is that we have 64 reset bits (2x 32bit
>>>>>>>> registers) and 64 status bits (also 2x 32bit registers). each 
>>>>>>>> reset
>>>>>>>> bit has a corresponding status bit but the numbering may be 
>>>>>>>> different
>>>>>>>> it's not clear to me how many resets LGM supports and how they are
>>>>>>>> organized. for example: I think it makes a difference if "there 
>>>>>>>> are 64
>>>>>>>> registers with each one reset bit" versus "there are two registers
>>>>>>>> with 32 bits each"
>>>>>>>> please share some details how it's organized internally, then I 
>>>>>>>> can
>>>>>>>> try to come up with a suggestion.
>>>>>>> LGM reset organization is more clean compared with legacy SoCs. 
>>>>>>> We have
>>>>>>> 8 x 32bit reset and status registers(more modules need to be reset,
>>>>>>> overall ideas are similar without big change). Their request and 
>>>>>>> status
>>>>>>> bit is at the same register bit position.  Hope this will help you.
>>>>>> have you already discussed using only one reset cell?
>>>>>> if there's only one big reset controller in RCU then why not let the
>>>>>> reset controller driver do it's job of translating a reset line? 
>>>>>> also
>>>>>> this represents the hardware best (dt-bindings should describe the
>>>>>> hardware, drivers then translate that into the various subsystems
>>>>>> offered by the kernel).
>>>>>>
>>>>>> we have to translate it into:
>>>>>> - status register and bit
>>>>>> - reset register and bit
>>>>>>
>>>>>> for LGM the implementation seems to be the easiest because the reset
>>>>>> line can be mapped easily to the registers and bit offsets (for
>>>>>> example like reset-meson.c does it, which also supports 256 reset
>>>>>> lines together with for example
>>>>>> include/dt-bindings/reset/amlogic,meson-g12a-reset.h. the latter is
>>>>>> nice to have but optional)
>>>>> When we implement this driver, we checked other drivers(hisilicon/*,
>>>>> reset-berlin.c and etc). After evaluation, we think register 
>>>>> offset and
>>>>> register bit are easier for users to understand and use if they 
>>>>> follow
>>>>> the hardware spec.
>>>> just so I know how the documentation looks like:
>>>> does the hardware spec document 8 registers, each with (up to) the 32
>>>> reset lines in it?
>>>>
>>>> reset-meson.c does it like that, but the difference there is that the
>>>> reset registers are continuous because there's no status register in
>>>> between
>>>> so your existing way of describing the reset line seems fine if Rob is
>>>> happy with it as well
>>>>
>>>>>> we can then implement special translation logic (in other words: a
>>>>>> separate of_xlate callback) for VRX200 which then has to do more
>>>>>> "magic" (like you have shown in your example code above: "if the 
>>>>>> reset
>>>>>> line belongs to the second set of 32 reset lines then use reset 
>>>>>> offset
>>>>>> X and status offset Y" - or even use a translation table as
>>>>>> reset-imx7.c does)
>>>>>>
>>>>>> the current binding is a mix of specifying reset register and bit in
>>>>>> .dts but calculating the status register.
>>>>>> I missed the calculation of the status register until you pointed 
>>>>>> it out earlier
>>>>> But we still don't have a good solution for VRX200 status bit issues.
>>>>> Before we solve this issue, it is very difficult to use one driver 
>>>>> for
>>>> OK, let me summarize what we have so far.
>>>>
>>>> all SoC have the following "shared" logic so far:
>>>> - all reset_control_ops callbacks are the same on VRX200 and LGM
>>>> (assuming we fix the issues you found in the reset-lantiq.c
>>>> implementation)
>>>> - internally we should use regmap (LGM for accessing the secure
>>>> processor, earlier SoCs because the parent is a syscon)
>>>> - each reset line consists of a reset register offset and bit as well
>>>> as a status register offset and bit
>>>>
>>>> however, we have differences in:
>>>> - how to map the registers (LGM maps the RCU registers directly while
>>>> earlier SoCs fetch the parent syscon)
>>>> - calculation of the status register
>>>> - calculation of the status bit
>>>>
>>>> I see two ways to use one common driver for LGM and the earlier SoCs:
>>>>
>>>> 1) use a reset line mapping table as for example reset-imx7.c does.
>>>> this would include reset register, reset bit, status register and 
>>>> status bit.
>>>> LGM can use a macro to get rid of the duplication between status bit
>>>> and reset bit (and the status register offset if you prefer)
>>>> this case would use #reset-cells = <1> and we wouldn't need to
>>>> implement the of_xlate callback
>>>>
>>>> 2) on VRX200 (and probably the older SoCs as well) we can encode the
>>>> following information in one 32-bit value:
>>>> - reset register (max value: 0x48)
>>>> - status register (max value: 0x24)
>>>> - reset bit (max value: 32)
>>>> - status bit (max value: 32)
>>>>
>>>> if this also works for LGM we can determine all required information
>>>> for a reset line in the of_xlate callback and translate it to one
>>>> 32-bit value.
>>>> LGM and earlier SoCs would each use it's own of_xlate implementation.
>>>> the reset_control_ops callback would then unpack the 32-bit value
>>>> ("unsigned long id") into the reset register and bit as well as status
>>>> register and bit (as needed)
>>>>
>>>> both ways can work, but it depends on what the dt-bindings maintainers
>>>> (like Rob) think of the binding itself.
>>>> (dt-bindings follow what the hardware implements, the driver only does
>>>> the translation between a vendor specific binding and a given
>>>> subsystem)
>>>> so we first need Rob's ACK on the binding, then we can figure out the
>>>> best driver implementation for that binding
>>> I will check with Dilip about how we should move forward. syscon parent
>>> is one issue that we have to solve first.
>> agreed, let's define the binding first
>
> Lantiq reset driver and dtsi bindings are designed with an 
> understanding of 2 reset controllers and they are children of the 
> multifunction device RCU(reset controller unit)[0], which is not the 
> case. Hardware wise there is only one reset controller and it is RCU 
> which is not a multifunction device. intel-reset-syscon.c is defined 
> as per the hardware.
>
> The major difference between the vrx200 and lgm is:
> 1.) RCU in vrx200 is having multiple register regions wheres RCU in 
> lgm has one single register region.
> 2.) Register offsets and bit offsets are different.
>
> So enhancing the intel-reset-syscon.c to provide compatibility/support 
> for vrx200.
> Please check the below dtsi binding proposal and let me know your view.
>
> rcu0:reset-controller@00000000 {
>     compatible= "intel,rcu-lgm";
>     reg = <0x0000000 0x80000>, <reg_set2 size>, <reg_set3 size>, 
> <reg_set4 size>;
>    intel,global-reset = <0x10 30>;
>    #reset-cells = <3>;
> };
>
> "#reset-cells":
>   const:3
>   description: |
>     The 1st cell is the reset register offset.
>     The 2nd cell is the reset set bit offset.
>     The 3rd cell is the reset status bit offset.
>
> Reset driver takes care of parsing the register address "reg" as per 
> the ".data" structure in struct of_device_id.
> Reset driver takes care of traversing the status register offset.

I will go ahead to implement this design and submit for review in the 
next patch version. Please let me know if you have any opens or queries 
on the design.

Thanks and Regards,
Dilip

>
> Regards,
> Dilip
>
> [0]: 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mips/lantiq/rcu.txt?h=v5.3-rc8
>
>>
>>
>> Martin

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

[Langer, Thomas]

Hi Dilip,

> -----Original Message-----
> From: devicetree-owner@vger.kernel.org <devicetree-
> owner@vger.kernel.org> On Behalf Of Dilip Kota
> Sent: Donnerstag, 19. September 2019 10:06
> To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: Chuan Hua, Lei <chuanhua.lei@linux.intel.com>; Kim, Cheol Yong
> <cheol.yong.kim@intel.com>; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; p.zabel@pengutronix.de; Wu, Qiming <qi-
> ming.wu@intel.com>; robh@kernel.org; Hauke Mehrtens <hauke@hauke-m.de>
> Subject: Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM
> SoC
> 
> Hi Martin,
> 
> On 9/12/2019 2:38 PM, Dilip Kota wrote:
> > Re-sending the mail, because of delivery failure.
> > sorry for the spam.
> >
> > Hi Martin,
> >
> > On 9/6/2019 4:53 AM, Martin Blumenstingl wrote:
> >> Hi,
> >>
> >> On Thu, Sep 5, 2019 at 4:38 AM Chuan Hua, Lei
> >> <chuanhua.lei@linux.intel.com> wrote:
> >> [...]
> >>>>>>>>>> I'm not surprised that we got some of the IP block layout for
> >>>>>>>>>> the
> >>>>>>>>>> VRX200 RCU "wrong" - all "documentation" we have is the old
> >>>>>>>>>> Lantiq UGW
> >>>>>>>>>> (BSP).
> >>>>>>>>>> with proper documentation (as in a "public datasheet for the
> >>>>>>>>>> SoC") it
> >>>>>>>>>> would be easy to spot these mistakes (at least I assume that
> the
> >>>>>>>>>> quality of the Infineon / Lantiq datasheets is excellent).
> >>>>>>>>>>
> >>>>>>>>>> back to reset-intel-syscon:
> >>>>>>>>>> assigning only one job to the RCU hardware is a good idea (in
> >>>>>>>>>> my opinion).
> >>>>>>>>>> that brings up a question: why do we need the "syscon"
> >>>>>>>>>> compatible for
> >>>>>>>>>> the RCU node?
> >>>>>>>>>> this is typically used when registers are accessed by another
> >>>>>>>>>> IP block
> >>>>>>>>>> and the other driver has to access these registers as well.
> >>>>>>>>>> does this
> >>>>>>>>>> mean that there's more hidden in the RCU registers?
> >>>>>>>>> As I mentioned, some other misc registers are put into RCU
> >>>>>>>>> even they
> >>>>>>>>> don't belong to reset functions.
> >>>>>>>> OK, just be aware that there are also rules for syscon
> compatible
> >>>>>>>> drivers, see for example: [0]
> >>>>>>>> if Rob (dt-bindings maintainer) is happy with the documentation
> in
> >>>>>>>> patch 1 then I'm fine with it as well.
> >>>>>>>> for my own education I would appreciate if you could describe
> >>>>>>>> these
> >>>>>>>> "other misc registers" with a few sentences (I assume that this
> >>>>>>>> can
> >>>>>>>> also help Rob)
> >>>>>>> For LGM, RCU is clean. There would be no MISC register after
> >>>>>>> software's
> >>>>>>> feedback. These misc registers will be moved to chiptop/misc
> >>>>>>> groups(implemented by syscon). For legacy SoC, we do have a lot
> >>>>>>> MISC
> >>>>>>> registers for different SoCs.
> >>>>>> OK, I think I understand now: chiptop != RCU
> >>>>>> so RCU really only has one purpose: handling resets
> >>>>>> while chiptop manages all the random bits
> >>>>>>
> >>>>>> does this means we don't need RCU to match "syscon"?
> >>>>> If we don't support legacy SoC with the same driver, we don't need
> >>>>> syscon, just regmap. Regmap is a must for us since we will use
> regmap
> >>>>> proxy to implement secure rest via secure processor.
> >>>> I think we should drop the syscon compatible for LGM then
> >>>> even for the legacy SoCs the reset controller should not have a
> syscon
> >>>> compatible: instead it should have a syscon parent (as the current
> >>>> "lantiq,xrx200-reset" binding requires and as suggested by Rob for
> >>>> another IP block: [0])
> >>> I am not sure if syscon parent really matches hardware
> implementation.
> >>> In all our Networking SoCs, chiptop is kind of misc register
> >>> collection.
> >>> Some registers can't belong to any particular group, or they need to
> >>> work together with other modules(therefore, these misc registers
> would
> >>> be accessed by two or more modules). However, chiptop is not a
> hardware
> >>> module.
> >> indeed, chiptop should not have any child nodes (based on your
> >> explanation).
> >> I was referring to VRX200 where the RCU syscon has various children
> >> (one child node for each hardware module that's part of RCU: reset
> >> controller, 2x USB PHY, ...)
> >>
> >> back to LGM:
> >> you said that the LGM RCU registers only contain the reset
> controller.
> >> thus I see no need for the syscon compatible
> >>
> >>>> keeping regmap is great in my opinion because it's a nice API and
> gets
> >>>> rid of some boilerplate
> >>>> even better if it makes things easier for accessing the secure
> >>>> processor
> >>>>
> >>>>>>>> [...]
> >>>>>>>>>>>>>>> 4. Code not optimized and intel internal review not
> >>>>>>>>>>>>>>> assessed.
> >>>>>>>>>>>>>> insights from you (like the issue with the reset
> >>>>>>>>>>>>>> callback) are very
> >>>>>>>>>>>>>> valuable - this shows that we should focus on having one
> >>>>>>>>>>>>>> driver.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Based on the above findings, I would suggest
> >>>>>>>>>>>>>>> reset-lantiq.c to move to
> >>>>>>>>>>>>>>> reset-intel-syscon.c
> >>>>>>>>>>>>>> my concern with having two separate drivers is that it
> >>>>>>>>>>>>>> will be hard to
> >>>>>>>>>>>>>> migrate from reset-lantiq to the "optimized"
> >>>>>>>>>>>>>> reset-intel-syscon
> >>>>>>>>>>>>>> driver.
> >>>>>>>>>>>>>> I don't have access to the datasheets for the any
> >>>>>>>>>>>>>> Lantiq/Intel SoC
> >>>>>>>>>>>>>> (VRX200 and even older).
> >>>>>>>>>>>>>> so debugging issues after switching from one driver to
> >>>>>>>>>>>>>> another is
> >>>>>>>>>>>>>> tedious because I cannot tell which part of the driver is
> >>>>>>>>>>>>>> causing a
> >>>>>>>>>>>>>> problem (it's either "all code from driver A" vs "all
> >>>>>>>>>>>>>> code from driver
> >>>>>>>>>>>>>> B", meaning it's hard to narrow it down).
> >>>>>>>>>>>>>> with separate commits/patches that are improving the
> >>>>>>>>>>>>>> reset-lantiq
> >>>>>>>>>>>>>> driver I can do git bisect to find the cause of a problem
> >>>>>>>>>>>>>> on the older
> >>>>>>>>>>>>>> SoCs (VRX200 for example)
> >>>>>>>>>>>>> Our internal version supports XRX350/XRX500/PRX300(MIPS
> >>>>>>>>>>>>> based) and
> >>>>>>>>>>>>> latest Lighting Mountain(X86 based). Migration to
> >>>>>>>>>>>>> reset-intel-syscon.c
> >>>>>>>>>>>>> should be straight forward.
> >>>>>>>>>>>> what about the _reset callback on the XRX350/XRX500/PRX300
> >>>>>>>>>>>> SoCs - do
> >>>>>>>>>>>> they only use level resets (_assert and _deassert) or are
> >>>>>>>>>>>> some reset
> >>>>>>>>>>>> lines using reset pulses (_reset)?
> >>>>>>>>>>>>
> >>>>>>>>>>>> when we wanted to switch from reset-lantiq.c to
> >>>>>>>>>>>> reset-intel-syscon.c
> >>>>>>>>>>>> we still had to add support for the _reset callback as this
> >>>>>>>>>>>> is missing
> >>>>>>>>>>>> in reset-intel-syscon.c currently
> >>>>>>>>>>> Yes. We have reset pulse(assert, then check the reset
> status).
> >>>>>>>>>> only now I realized that the reset-intel-syscon driver does
> >>>>>>>>>> not seem
> >>>>>>>>>> to use the status registers (instead it's looking at the
> reset
> >>>>>>>>>> registers when checking the status).
> >>>>>>>>>> what happened to the status registers - do they still exist
> >>>>>>>>>> in newer
> >>>>>>>>>> SoCs (like LGM)? why are they not used?
> >>>>>>>>> Reset status check is there. regmap_read_poll_timeout to check
> >>>>>>>>> status
> >>>>>>>>> big. Status register offset <4) from request register. For
> >>>>>>>>> legacy, there
> >>>>>>>>> is one exception, we can add soc specific data to handle it.
> >>>>>>>> I see, thank you for the explanation
> >>>>>>>> this won't work on VRX200 for example because the status
> >>>>>>>> register is
> >>>>>>>> not always at (reset register - 0x4)
> >>>>>>> As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be
> >>>>>>> solved
> >>>>>>> with one soc data in the compatible array.
> >>>>>>>
> >>>>>>> For example(not same as upstream, but idea is similar)
> >>>>>>>
> >>>>>>> static u32 intel_stat_reg_off(struct intel_reset_data *data, u32
> >>>>>>> req_off)
> >>>>>>> {
> >>>>>>>         if (data->soc_data->legacy && req_off == RCU_RST_REQ)
> >>>>>>>             return RCU_RST_STAT;
> >>>>>>>         else
> >>>>>>>             return req_off + 0x4;
> >>>>>>> }
> >>>>>>>
> >>>>>>>>>> on VRX200 for example there seem to be some cases where the
> >>>>>>>>>> bits in
> >>>>>>>>>> the reset and status registers are different (for example:
> >>>>>>>>>> the first
> >>>>>>>>>> GPHY seems to use reset bit 31 but status bit 30)
> >>>>>>>>>> this is currently not supported in reset-intel-syscon
> >>>>>>>>> This is most tricky and ugly part for VRX200/Danube. Do you
> >>>>>>>>> have any
> >>>>>>>>> idea to handle this nicely?
> >>>>>>>> with reset-lantiq we have the following register information:
> >>>>>>>> a) reset offset: first reg property
> >>>>>>>> b) status offset: second reg property
> >>>>>>>> c) reset bit: first #reset-cell
> >>>>>>>> d) status bit: second #reset-cell
> >>>>>>>>
> >>>>>>>> reset-intel-syscon derives half of this information from the
> >>>>>>>> two #reset-cells:
> >>>>>>>> a) reset offset: first #reset-cell
> >>>>>>>> b) status offset: reset offset - 0x4
> >>>>>>>> c) reset bit: second #reset-cell
> >>>>>>>> d) status bit: same as reset bit
> >>>>>>>>
> >>>>>>>> I cannot make any suggestion (yet) how to handle VRX200 and LGM
> >>>>>>>> in one
> >>>>>>>> driver because I don't know enough about LGM (yet).
> >>>>>>>> on VRX200 my understanding is that we have 64 reset bits (2x
> 32bit
> >>>>>>>> registers) and 64 status bits (also 2x 32bit registers). each
> >>>>>>>> reset
> >>>>>>>> bit has a corresponding status bit but the numbering may be
> >>>>>>>> different
> >>>>>>>> it's not clear to me how many resets LGM supports and how they
> are
> >>>>>>>> organized. for example: I think it makes a difference if "there
> >>>>>>>> are 64
> >>>>>>>> registers with each one reset bit" versus "there are two
> registers
> >>>>>>>> with 32 bits each"
> >>>>>>>> please share some details how it's organized internally, then I
> >>>>>>>> can
> >>>>>>>> try to come up with a suggestion.
> >>>>>>> LGM reset organization is more clean compared with legacy SoCs.
> >>>>>>> We have
> >>>>>>> 8 x 32bit reset and status registers(more modules need to be
> reset,
> >>>>>>> overall ideas are similar without big change). Their request and
> >>>>>>> status
> >>>>>>> bit is at the same register bit position.  Hope this will help
> you.
> >>>>>> have you already discussed using only one reset cell?
> >>>>>> if there's only one big reset controller in RCU then why not let
> the
> >>>>>> reset controller driver do it's job of translating a reset line?
> >>>>>> also
> >>>>>> this represents the hardware best (dt-bindings should describe
> the
> >>>>>> hardware, drivers then translate that into the various subsystems
> >>>>>> offered by the kernel).
> >>>>>>
> >>>>>> we have to translate it into:
> >>>>>> - status register and bit
> >>>>>> - reset register and bit
> >>>>>>
> >>>>>> for LGM the implementation seems to be the easiest because the
> reset
> >>>>>> line can be mapped easily to the registers and bit offsets (for
> >>>>>> example like reset-meson.c does it, which also supports 256 reset
> >>>>>> lines together with for example
> >>>>>> include/dt-bindings/reset/amlogic,meson-g12a-reset.h. the latter
> is
> >>>>>> nice to have but optional)
> >>>>> When we implement this driver, we checked other
> drivers(hisilicon/*,
> >>>>> reset-berlin.c and etc). After evaluation, we think register
> >>>>> offset and
> >>>>> register bit are easier for users to understand and use if they
> >>>>> follow
> >>>>> the hardware spec.
> >>>> just so I know how the documentation looks like:
> >>>> does the hardware spec document 8 registers, each with (up to) the
> 32
> >>>> reset lines in it?
> >>>>
> >>>> reset-meson.c does it like that, but the difference there is that
> the
> >>>> reset registers are continuous because there's no status register
> in
> >>>> between
> >>>> so your existing way of describing the reset line seems fine if Rob
> is
> >>>> happy with it as well
> >>>>
> >>>>>> we can then implement special translation logic (in other words:
> a
> >>>>>> separate of_xlate callback) for VRX200 which then has to do more
> >>>>>> "magic" (like you have shown in your example code above: "if the
> >>>>>> reset
> >>>>>> line belongs to the second set of 32 reset lines then use reset
> >>>>>> offset
> >>>>>> X and status offset Y" - or even use a translation table as
> >>>>>> reset-imx7.c does)
> >>>>>>
> >>>>>> the current binding is a mix of specifying reset register and bit
> in
> >>>>>> .dts but calculating the status register.
> >>>>>> I missed the calculation of the status register until you pointed
> >>>>>> it out earlier
> >>>>> But we still don't have a good solution for VRX200 status bit
> issues.
> >>>>> Before we solve this issue, it is very difficult to use one driver
> >>>>> for
> >>>> OK, let me summarize what we have so far.
> >>>>
> >>>> all SoC have the following "shared" logic so far:
> >>>> - all reset_control_ops callbacks are the same on VRX200 and LGM
> >>>> (assuming we fix the issues you found in the reset-lantiq.c
> >>>> implementation)
> >>>> - internally we should use regmap (LGM for accessing the secure
> >>>> processor, earlier SoCs because the parent is a syscon)
> >>>> - each reset line consists of a reset register offset and bit as
> well
> >>>> as a status register offset and bit
> >>>>
> >>>> however, we have differences in:
> >>>> - how to map the registers (LGM maps the RCU registers directly
> while
> >>>> earlier SoCs fetch the parent syscon)
> >>>> - calculation of the status register
> >>>> - calculation of the status bit
> >>>>
> >>>> I see two ways to use one common driver for LGM and the earlier
> SoCs:
> >>>>
> >>>> 1) use a reset line mapping table as for example reset-imx7.c does.
> >>>> this would include reset register, reset bit, status register and
> >>>> status bit.
> >>>> LGM can use a macro to get rid of the duplication between status
> bit
> >>>> and reset bit (and the status register offset if you prefer)
> >>>> this case would use #reset-cells = <1> and we wouldn't need to
> >>>> implement the of_xlate callback
> >>>>
> >>>> 2) on VRX200 (and probably the older SoCs as well) we can encode
> the
> >>>> following information in one 32-bit value:
> >>>> - reset register (max value: 0x48)
> >>>> - status register (max value: 0x24)
> >>>> - reset bit (max value: 32)
> >>>> - status bit (max value: 32)
> >>>>
> >>>> if this also works for LGM we can determine all required
> information
> >>>> for a reset line in the of_xlate callback and translate it to one
> >>>> 32-bit value.
> >>>> LGM and earlier SoCs would each use it's own of_xlate
> implementation.
> >>>> the reset_control_ops callback would then unpack the 32-bit value
> >>>> ("unsigned long id") into the reset register and bit as well as
> status
> >>>> register and bit (as needed)
> >>>>
> >>>> both ways can work, but it depends on what the dt-bindings
> maintainers
> >>>> (like Rob) think of the binding itself.
> >>>> (dt-bindings follow what the hardware implements, the driver only
> does
> >>>> the translation between a vendor specific binding and a given
> >>>> subsystem)
> >>>> so we first need Rob's ACK on the binding, then we can figure out
> the
> >>>> best driver implementation for that binding
> >>> I will check with Dilip about how we should move forward. syscon
> parent
> >>> is one issue that we have to solve first.
> >> agreed, let's define the binding first
> >
> > Lantiq reset driver and dtsi bindings are designed with an
> > understanding of 2 reset controllers and they are children of the
> > multifunction device RCU(reset controller unit)[0], which is not the
> > case. Hardware wise there is only one reset controller and it is RCU
> > which is not a multifunction device. intel-reset-syscon.c is defined
> > as per the hardware.
> >
> > The major difference between the vrx200 and lgm is:
> > 1.) RCU in vrx200 is having multiple register regions wheres RCU in
> > lgm has one single register region.
> > 2.) Register offsets and bit offsets are different.
> >
> > So enhancing the intel-reset-syscon.c to provide compatibility/support
> > for vrx200.
> > Please check the below dtsi binding proposal and let me know your
> view.
> >
> > rcu0:reset-controller@00000000 {
> >     compatible= "intel,rcu-lgm";
> >     reg = <0x0000000 0x80000>, <reg_set2 size>, <reg_set3 size>,
> > <reg_set4 size>;
> >    intel,global-reset = <0x10 30>;
> >    #reset-cells = <3>;
> > };
> >
> > "#reset-cells":
> >   const:3
> >   description: |
> >     The 1st cell is the reset register offset.
> >     The 2nd cell is the reset set bit offset.
> >     The 3rd cell is the reset status bit offset.
> >
> > Reset driver takes care of parsing the register address "reg" as per
> > the ".data" structure in struct of_device_id.
> > Reset driver takes care of traversing the status register offset.
> 
> I will go ahead to implement this design and submit for review in the
> next patch version. Please let me know if you have any opens or queries
> on the design.

Why do you define a binding for LGM with a 3rd cell for the status bit,
when you don't need it?
The problem is not the LGM binding. It is important that the binding
for the already supported SoC is still supported by the driver!
Adding a new cell does not help, as the existing DTBs don't have it.

Thomas

> 
> Thanks and Regards,
> Dilip
> 
> >
> > Regards,
> > Dilip
> >
> > [0]:
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/
> Documentation/devicetree/bindings/mips/lantiq/rcu.txt?h=v5.3-rc8
> >
> >>
> >>
> >> Martin

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

[Dilip Kota]

Hi Thomas,

On 9/19/2019 4:36 PM, Langer, Thomas wrote:
> Hi Dilip,
>
>> -----Original Message-----
>> From: devicetree-owner@vger.kernel.org <devicetree-
>> owner@vger.kernel.org> On Behalf Of Dilip Kota
>> Sent: Donnerstag, 19. September 2019 10:06
>> To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Cc: Chuan Hua, Lei <chuanhua.lei@linux.intel.com>; Kim, Cheol Yong
>> <cheol.yong.kim@intel.com>; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; p.zabel@pengutronix.de; Wu, Qiming <qi-
>> ming.wu@intel.com>; robh@kernel.org; Hauke Mehrtens <hauke@hauke-m.de>
>> Subject: Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM
>> SoC
>>
>> Hi Martin,
>>
>> On 9/12/2019 2:38 PM, Dilip Kota wrote:
>>> Re-sending the mail, because of delivery failure.
>>> sorry for the spam.
>>>
>>> Hi Martin,
>>>
>>> On 9/6/2019 4:53 AM, Martin Blumenstingl wrote:
>>>> Hi,
>>>>
>>>> On Thu, Sep 5, 2019 at 4:38 AM Chuan Hua, Lei
>>>> <chuanhua.lei@linux.intel.com> wrote:
>>>> [...]
>>>>>>>>>>>> I'm not surprised that we got some of the IP block layout for
>>>>>>>>>>>> the
>>>>>>>>>>>> VRX200 RCU "wrong" - all "documentation" we have is the old
>>>>>>>>>>>> Lantiq UGW
>>>>>>>>>>>> (BSP).
>>>>>>>>>>>> with proper documentation (as in a "public datasheet for the
>>>>>>>>>>>> SoC") it
>>>>>>>>>>>> would be easy to spot these mistakes (at least I assume that
>> the
>>>>>>>>>>>> quality of the Infineon / Lantiq datasheets is excellent).
>>>>>>>>>>>>
>>>>>>>>>>>> back to reset-intel-syscon:
>>>>>>>>>>>> assigning only one job to the RCU hardware is a good idea (in
>>>>>>>>>>>> my opinion).
>>>>>>>>>>>> that brings up a question: why do we need the "syscon"
>>>>>>>>>>>> compatible for
>>>>>>>>>>>> the RCU node?
>>>>>>>>>>>> this is typically used when registers are accessed by another
>>>>>>>>>>>> IP block
>>>>>>>>>>>> and the other driver has to access these registers as well.
>>>>>>>>>>>> does this
>>>>>>>>>>>> mean that there's more hidden in the RCU registers?
>>>>>>>>>>> As I mentioned, some other misc registers are put into RCU
>>>>>>>>>>> even they
>>>>>>>>>>> don't belong to reset functions.
>>>>>>>>>> OK, just be aware that there are also rules for syscon
>> compatible
>>>>>>>>>> drivers, see for example: [0]
>>>>>>>>>> if Rob (dt-bindings maintainer) is happy with the documentation
>> in
>>>>>>>>>> patch 1 then I'm fine with it as well.
>>>>>>>>>> for my own education I would appreciate if you could describe
>>>>>>>>>> these
>>>>>>>>>> "other misc registers" with a few sentences (I assume that this
>>>>>>>>>> can
>>>>>>>>>> also help Rob)
>>>>>>>>> For LGM, RCU is clean. There would be no MISC register after
>>>>>>>>> software's
>>>>>>>>> feedback. These misc registers will be moved to chiptop/misc
>>>>>>>>> groups(implemented by syscon). For legacy SoC, we do have a lot
>>>>>>>>> MISC
>>>>>>>>> registers for different SoCs.
>>>>>>>> OK, I think I understand now: chiptop != RCU
>>>>>>>> so RCU really only has one purpose: handling resets
>>>>>>>> while chiptop manages all the random bits
>>>>>>>>
>>>>>>>> does this means we don't need RCU to match "syscon"?
>>>>>>> If we don't support legacy SoC with the same driver, we don't need
>>>>>>> syscon, just regmap. Regmap is a must for us since we will use
>> regmap
>>>>>>> proxy to implement secure rest via secure processor.
>>>>>> I think we should drop the syscon compatible for LGM then
>>>>>> even for the legacy SoCs the reset controller should not have a
>> syscon
>>>>>> compatible: instead it should have a syscon parent (as the current
>>>>>> "lantiq,xrx200-reset" binding requires and as suggested by Rob for
>>>>>> another IP block: [0])
>>>>> I am not sure if syscon parent really matches hardware
>> implementation.
>>>>> In all our Networking SoCs, chiptop is kind of misc register
>>>>> collection.
>>>>> Some registers can't belong to any particular group, or they need to
>>>>> work together with other modules(therefore, these misc registers
>> would
>>>>> be accessed by two or more modules). However, chiptop is not a
>> hardware
>>>>> module.
>>>> indeed, chiptop should not have any child nodes (based on your
>>>> explanation).
>>>> I was referring to VRX200 where the RCU syscon has various children
>>>> (one child node for each hardware module that's part of RCU: reset
>>>> controller, 2x USB PHY, ...)
>>>>
>>>> back to LGM:
>>>> you said that the LGM RCU registers only contain the reset
>> controller.
>>>> thus I see no need for the syscon compatible
>>>>
>>>>>> keeping regmap is great in my opinion because it's a nice API and
>> gets
>>>>>> rid of some boilerplate
>>>>>> even better if it makes things easier for accessing the secure
>>>>>> processor
>>>>>>
>>>>>>>>>> [...]
>>>>>>>>>>>>>>>>> 4. Code not optimized and intel internal review not
>>>>>>>>>>>>>>>>> assessed.
>>>>>>>>>>>>>>>> insights from you (like the issue with the reset
>>>>>>>>>>>>>>>> callback) are very
>>>>>>>>>>>>>>>> valuable - this shows that we should focus on having one
>>>>>>>>>>>>>>>> driver.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Based on the above findings, I would suggest
>>>>>>>>>>>>>>>>> reset-lantiq.c to move to
>>>>>>>>>>>>>>>>> reset-intel-syscon.c
>>>>>>>>>>>>>>>> my concern with having two separate drivers is that it
>>>>>>>>>>>>>>>> will be hard to
>>>>>>>>>>>>>>>> migrate from reset-lantiq to the "optimized"
>>>>>>>>>>>>>>>> reset-intel-syscon
>>>>>>>>>>>>>>>> driver.
>>>>>>>>>>>>>>>> I don't have access to the datasheets for the any
>>>>>>>>>>>>>>>> Lantiq/Intel SoC
>>>>>>>>>>>>>>>> (VRX200 and even older).
>>>>>>>>>>>>>>>> so debugging issues after switching from one driver to
>>>>>>>>>>>>>>>> another is
>>>>>>>>>>>>>>>> tedious because I cannot tell which part of the driver is
>>>>>>>>>>>>>>>> causing a
>>>>>>>>>>>>>>>> problem (it's either "all code from driver A" vs "all
>>>>>>>>>>>>>>>> code from driver
>>>>>>>>>>>>>>>> B", meaning it's hard to narrow it down).
>>>>>>>>>>>>>>>> with separate commits/patches that are improving the
>>>>>>>>>>>>>>>> reset-lantiq
>>>>>>>>>>>>>>>> driver I can do git bisect to find the cause of a problem
>>>>>>>>>>>>>>>> on the older
>>>>>>>>>>>>>>>> SoCs (VRX200 for example)
>>>>>>>>>>>>>>> Our internal version supports XRX350/XRX500/PRX300(MIPS
>>>>>>>>>>>>>>> based) and
>>>>>>>>>>>>>>> latest Lighting Mountain(X86 based). Migration to
>>>>>>>>>>>>>>> reset-intel-syscon.c
>>>>>>>>>>>>>>> should be straight forward.
>>>>>>>>>>>>>> what about the _reset callback on the XRX350/XRX500/PRX300
>>>>>>>>>>>>>> SoCs - do
>>>>>>>>>>>>>> they only use level resets (_assert and _deassert) or are
>>>>>>>>>>>>>> some reset
>>>>>>>>>>>>>> lines using reset pulses (_reset)?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> when we wanted to switch from reset-lantiq.c to
>>>>>>>>>>>>>> reset-intel-syscon.c
>>>>>>>>>>>>>> we still had to add support for the _reset callback as this
>>>>>>>>>>>>>> is missing
>>>>>>>>>>>>>> in reset-intel-syscon.c currently
>>>>>>>>>>>>> Yes. We have reset pulse(assert, then check the reset
>> status).
>>>>>>>>>>>> only now I realized that the reset-intel-syscon driver does
>>>>>>>>>>>> not seem
>>>>>>>>>>>> to use the status registers (instead it's looking at the
>> reset
>>>>>>>>>>>> registers when checking the status).
>>>>>>>>>>>> what happened to the status registers - do they still exist
>>>>>>>>>>>> in newer
>>>>>>>>>>>> SoCs (like LGM)? why are they not used?
>>>>>>>>>>> Reset status check is there. regmap_read_poll_timeout to check
>>>>>>>>>>> status
>>>>>>>>>>> big. Status register offset <4) from request register. For
>>>>>>>>>>> legacy, there
>>>>>>>>>>> is one exception, we can add soc specific data to handle it.
>>>>>>>>>> I see, thank you for the explanation
>>>>>>>>>> this won't work on VRX200 for example because the status
>>>>>>>>>> register is
>>>>>>>>>> not always at (reset register - 0x4)
>>>>>>>>> As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be
>>>>>>>>> solved
>>>>>>>>> with one soc data in the compatible array.
>>>>>>>>>
>>>>>>>>> For example(not same as upstream, but idea is similar)
>>>>>>>>>
>>>>>>>>> static u32 intel_stat_reg_off(struct intel_reset_data *data, u32
>>>>>>>>> req_off)
>>>>>>>>> {
>>>>>>>>>          if (data->soc_data->legacy && req_off == RCU_RST_REQ)
>>>>>>>>>              return RCU_RST_STAT;
>>>>>>>>>          else
>>>>>>>>>              return req_off + 0x4;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>>>> on VRX200 for example there seem to be some cases where the
>>>>>>>>>>>> bits in
>>>>>>>>>>>> the reset and status registers are different (for example:
>>>>>>>>>>>> the first
>>>>>>>>>>>> GPHY seems to use reset bit 31 but status bit 30)
>>>>>>>>>>>> this is currently not supported in reset-intel-syscon
>>>>>>>>>>> This is most tricky and ugly part for VRX200/Danube. Do you
>>>>>>>>>>> have any
>>>>>>>>>>> idea to handle this nicely?
>>>>>>>>>> with reset-lantiq we have the following register information:
>>>>>>>>>> a) reset offset: first reg property
>>>>>>>>>> b) status offset: second reg property
>>>>>>>>>> c) reset bit: first #reset-cell
>>>>>>>>>> d) status bit: second #reset-cell
>>>>>>>>>>
>>>>>>>>>> reset-intel-syscon derives half of this information from the
>>>>>>>>>> two #reset-cells:
>>>>>>>>>> a) reset offset: first #reset-cell
>>>>>>>>>> b) status offset: reset offset - 0x4
>>>>>>>>>> c) reset bit: second #reset-cell
>>>>>>>>>> d) status bit: same as reset bit
>>>>>>>>>>
>>>>>>>>>> I cannot make any suggestion (yet) how to handle VRX200 and LGM
>>>>>>>>>> in one
>>>>>>>>>> driver because I don't know enough about LGM (yet).
>>>>>>>>>> on VRX200 my understanding is that we have 64 reset bits (2x
>> 32bit
>>>>>>>>>> registers) and 64 status bits (also 2x 32bit registers). each
>>>>>>>>>> reset
>>>>>>>>>> bit has a corresponding status bit but the numbering may be
>>>>>>>>>> different
>>>>>>>>>> it's not clear to me how many resets LGM supports and how they
>> are
>>>>>>>>>> organized. for example: I think it makes a difference if "there
>>>>>>>>>> are 64
>>>>>>>>>> registers with each one reset bit" versus "there are two
>> registers
>>>>>>>>>> with 32 bits each"
>>>>>>>>>> please share some details how it's organized internally, then I
>>>>>>>>>> can
>>>>>>>>>> try to come up with a suggestion.
>>>>>>>>> LGM reset organization is more clean compared with legacy SoCs.
>>>>>>>>> We have
>>>>>>>>> 8 x 32bit reset and status registers(more modules need to be
>> reset,
>>>>>>>>> overall ideas are similar without big change). Their request and
>>>>>>>>> status
>>>>>>>>> bit is at the same register bit position.  Hope this will help
>> you.
>>>>>>>> have you already discussed using only one reset cell?
>>>>>>>> if there's only one big reset controller in RCU then why not let
>> the
>>>>>>>> reset controller driver do it's job of translating a reset line?
>>>>>>>> also
>>>>>>>> this represents the hardware best (dt-bindings should describe
>> the
>>>>>>>> hardware, drivers then translate that into the various subsystems
>>>>>>>> offered by the kernel).
>>>>>>>>
>>>>>>>> we have to translate it into:
>>>>>>>> - status register and bit
>>>>>>>> - reset register and bit
>>>>>>>>
>>>>>>>> for LGM the implementation seems to be the easiest because the
>> reset
>>>>>>>> line can be mapped easily to the registers and bit offsets (for
>>>>>>>> example like reset-meson.c does it, which also supports 256 reset
>>>>>>>> lines together with for example
>>>>>>>> include/dt-bindings/reset/amlogic,meson-g12a-reset.h. the latter
>> is
>>>>>>>> nice to have but optional)
>>>>>>> When we implement this driver, we checked other
>> drivers(hisilicon/*,
>>>>>>> reset-berlin.c and etc). After evaluation, we think register
>>>>>>> offset and
>>>>>>> register bit are easier for users to understand and use if they
>>>>>>> follow
>>>>>>> the hardware spec.
>>>>>> just so I know how the documentation looks like:
>>>>>> does the hardware spec document 8 registers, each with (up to) the
>> 32
>>>>>> reset lines in it?
>>>>>>
>>>>>> reset-meson.c does it like that, but the difference there is that
>> the
>>>>>> reset registers are continuous because there's no status register
>> in
>>>>>> between
>>>>>> so your existing way of describing the reset line seems fine if Rob
>> is
>>>>>> happy with it as well
>>>>>>
>>>>>>>> we can then implement special translation logic (in other words:
>> a
>>>>>>>> separate of_xlate callback) for VRX200 which then has to do more
>>>>>>>> "magic" (like you have shown in your example code above: "if the
>>>>>>>> reset
>>>>>>>> line belongs to the second set of 32 reset lines then use reset
>>>>>>>> offset
>>>>>>>> X and status offset Y" - or even use a translation table as
>>>>>>>> reset-imx7.c does)
>>>>>>>>
>>>>>>>> the current binding is a mix of specifying reset register and bit
>> in
>>>>>>>> .dts but calculating the status register.
>>>>>>>> I missed the calculation of the status register until you pointed
>>>>>>>> it out earlier
>>>>>>> But we still don't have a good solution for VRX200 status bit
>> issues.
>>>>>>> Before we solve this issue, it is very difficult to use one driver
>>>>>>> for
>>>>>> OK, let me summarize what we have so far.
>>>>>>
>>>>>> all SoC have the following "shared" logic so far:
>>>>>> - all reset_control_ops callbacks are the same on VRX200 and LGM
>>>>>> (assuming we fix the issues you found in the reset-lantiq.c
>>>>>> implementation)
>>>>>> - internally we should use regmap (LGM for accessing the secure
>>>>>> processor, earlier SoCs because the parent is a syscon)
>>>>>> - each reset line consists of a reset register offset and bit as
>> well
>>>>>> as a status register offset and bit
>>>>>>
>>>>>> however, we have differences in:
>>>>>> - how to map the registers (LGM maps the RCU registers directly
>> while
>>>>>> earlier SoCs fetch the parent syscon)
>>>>>> - calculation of the status register
>>>>>> - calculation of the status bit
>>>>>>
>>>>>> I see two ways to use one common driver for LGM and the earlier
>> SoCs:
>>>>>> 1) use a reset line mapping table as for example reset-imx7.c does.
>>>>>> this would include reset register, reset bit, status register and
>>>>>> status bit.
>>>>>> LGM can use a macro to get rid of the duplication between status
>> bit
>>>>>> and reset bit (and the status register offset if you prefer)
>>>>>> this case would use #reset-cells = <1> and we wouldn't need to
>>>>>> implement the of_xlate callback
>>>>>>
>>>>>> 2) on VRX200 (and probably the older SoCs as well) we can encode
>> the
>>>>>> following information in one 32-bit value:
>>>>>> - reset register (max value: 0x48)
>>>>>> - status register (max value: 0x24)
>>>>>> - reset bit (max value: 32)
>>>>>> - status bit (max value: 32)
>>>>>>
>>>>>> if this also works for LGM we can determine all required
>> information
>>>>>> for a reset line in the of_xlate callback and translate it to one
>>>>>> 32-bit value.
>>>>>> LGM and earlier SoCs would each use it's own of_xlate
>> implementation.
>>>>>> the reset_control_ops callback would then unpack the 32-bit value
>>>>>> ("unsigned long id") into the reset register and bit as well as
>> status
>>>>>> register and bit (as needed)
>>>>>>
>>>>>> both ways can work, but it depends on what the dt-bindings
>> maintainers
>>>>>> (like Rob) think of the binding itself.
>>>>>> (dt-bindings follow what the hardware implements, the driver only
>> does
>>>>>> the translation between a vendor specific binding and a given
>>>>>> subsystem)
>>>>>> so we first need Rob's ACK on the binding, then we can figure out
>> the
>>>>>> best driver implementation for that binding
>>>>> I will check with Dilip about how we should move forward. syscon
>> parent
>>>>> is one issue that we have to solve first.
>>>> agreed, let's define the binding first
>>> Lantiq reset driver and dtsi bindings are designed with an
>>> understanding of 2 reset controllers and they are children of the
>>> multifunction device RCU(reset controller unit)[0], which is not the
>>> case. Hardware wise there is only one reset controller and it is RCU
>>> which is not a multifunction device. intel-reset-syscon.c is defined
>>> as per the hardware.
>>>
>>> The major difference between the vrx200 and lgm is:
>>> 1.) RCU in vrx200 is having multiple register regions wheres RCU in
>>> lgm has one single register region.
>>> 2.) Register offsets and bit offsets are different.
>>>
>>> So enhancing the intel-reset-syscon.c to provide compatibility/support
>>> for vrx200.
>>> Please check the below dtsi binding proposal and let me know your
>> view.
>>> rcu0:reset-controller@00000000 {
>>>      compatible= "intel,rcu-lgm";
>>>      reg = <0x0000000 0x80000>, <reg_set2 size>, <reg_set3 size>,
>>> <reg_set4 size>;
>>>     intel,global-reset = <0x10 30>;
>>>     #reset-cells = <3>;
>>> };
>>>
>>> "#reset-cells":
>>>    const:3
>>>    description: |
>>>      The 1st cell is the reset register offset.
>>>      The 2nd cell is the reset set bit offset.
>>>      The 3rd cell is the reset status bit offset.
>>>
>>> Reset driver takes care of parsing the register address "reg" as per
>>> the ".data" structure in struct of_device_id.
>>> Reset driver takes care of traversing the status register offset.
>> I will go ahead to implement this design and submit for review in the
>> next patch version. Please let me know if you have any opens or queries
>> on the design.
> Why do you define a binding for LGM with a 3rd cell for the status bit,
> when you don't need it?
> The problem is not the LGM binding. It is important that the binding
> for the already supported SoC is still supported by the driver!
> Adding a new cell does not help, as the existing DTBs don't have it.
3rd cell is added to provide compatibility for older SoCs (Vrx200) so 
that they can switch to the new driver.
Existing driver and DTB design are not according to the hardware and 
doesn't work for LGM.

Regards,
Dilip
>
> Thomas
>
>> Thanks and Regards,
>> Dilip
>>
>>> Regards,
>>> Dilip
>>>
>>> [0]:
>>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/
>> Documentation/devicetree/bindings/mips/lantiq/rcu.txt?h=v5.3-rc8
>>>>
>>>> Martin

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

[Martin Blumenstingl]

Hi Dilip,

(sorry for the late reply)

On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota <eswara.kota@linux.intel.com> wrote:
[...]
> The major difference between the vrx200 and lgm is:
> 1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm
> has one single register region.
> 2.) Register offsets and bit offsets are different.
>
> So enhancing the intel-reset-syscon.c to provide compatibility/support
> for vrx200.
> Please check the below dtsi binding proposal and let me know your view.
>
> rcu0:reset-controller@00000000 {
>      compatible= "intel,rcu-lgm";
>      reg = <0x0000000 0x80000>, <reg_set2 size>, <reg_set3 size>,
> <reg_set4 size>;
I'm not sure that I understand what are reg_set2/3/4 for
the first resource (0x80000 at 0x0) already covers the whole LGM RCU,
so what is the purpose of the other register resources

>     intel,global-reset = <0x10 30>;
>     #reset-cells = <3>;
> };
>
> "#reset-cells":
>    const:3
>    description: |
>      The 1st cell is the reset register offset.
>      The 2nd cell is the reset set bit offset.
>      The 3rd cell is the reset status bit offset.
I think this will work fine for VRX200 (and even older SoCs)
as you have described in your previous emails we can determine the
status offset from the reset offset using a simple if/else

for LGM I like your initial suggestion with #reset-cells = <2> because
it's easier to read and write.

> Reset driver takes care of parsing the register address "reg" as per the
> ".data" structure in struct of_device_id.
> Reset driver takes care of traversing the status register offset.
the differentiation between two and three #reset-cells can also happen
based on the struct of_device_id:
- the LGM implementation would simply also use the reset bit as status
bit (only two cells are needed)
- the implementation for earlier SoCs would parse the third cell and
use that as status bit

Philipp, can you please share your opinion on how to move forward with
the reset-intel driver from this series?
The reset_control_ops from the reset-intel driver are (in my opinion)
a bug-fixed and improved version of what we already have in
drivers/reset/reset-lantiq.c. The driver is NOT simply copy and paste
because the register layout was greatly simplified for the newer SoCs
(for which there is reset-intel) compared to the older ones
(reset-lantiq).
Dilip's suggestion (in my own words) is that you take his new
reset-intel driver, then we will work on porting reset-lantiq over to
that so in the end we can drop the reset-lantiq driver. This approach
means more work for me (as I am probably the one who then has to do
the work to port reset-lantiq over to reset-intel). I'm happy to do
that work if you think that it's worth following this approach.
So I want your opinion on this before I spend any effort on porting
reset-lantiq over to reset-intel.


Martin

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

[Dilip Kota]

Hi Martin,

On 9/20/2019 3:51 AM, Martin Blumenstingl wrote:
> Hi Dilip,
>
> (sorry for the late reply)
>
> On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota <eswara.kota@linux.intel.com> wrote:
> [...]
>> The major difference between the vrx200 and lgm is:
>> 1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm
>> has one single register region.
>> 2.) Register offsets and bit offsets are different.
>>
>> So enhancing the intel-reset-syscon.c to provide compatibility/support
>> for vrx200.
>> Please check the below dtsi binding proposal and let me know your view.
>>
>> rcu0:reset-controller@00000000 {
>>       compatible= "intel,rcu-lgm";
>>       reg = <0x0000000 0x80000>, <reg_set2 size>, <reg_set3 size>,
>> <reg_set4 size>;
> I'm not sure that I understand what are reg_set2/3/4 for
> the first resource (0x80000 at 0x0) already covers the whole LGM RCU,
> so what is the purpose of the other register resources
Yes, as you said the first register resource is enough for LGM RCU as 
registers are at one single region. Whereas in older SoCs RCU registers 
are at different regions, so for that reason reg_set2/3/4 are used.

Driver will decide in reading the no. of register resources based on the 
"struct of_device_id".

Regards,
Dilip
>
>>      intel,global-reset = <0x10 30>;
>>      #reset-cells = <3>;
>> };
>>
>> "#reset-cells":
>>     const:3
>>     description: |
>>       The 1st cell is the reset register offset.
>>       The 2nd cell is the reset set bit offset.
>>       The 3rd cell is the reset status bit offset.
> I think this will work fine for VRX200 (and even older SoCs)
> as you have described in your previous emails we can determine the
> status offset from the reset offset using a simple if/else
>
> for LGM I like your initial suggestion with #reset-cells = <2> because
> it's easier to read and write.
>
>> Reset driver takes care of parsing the register address "reg" as per the
>> ".data" structure in struct of_device_id.
>> Reset driver takes care of traversing the status register offset.
> the differentiation between two and three #reset-cells can also happen
> based on the struct of_device_id:
> - the LGM implementation would simply also use the reset bit as status
> bit (only two cells are needed)
> - the implementation for earlier SoCs would parse the third cell and
> use that as status bit
>
> Philipp, can you please share your opinion on how to move forward with
> the reset-intel driver from this series?
> The reset_control_ops from the reset-intel driver are (in my opinion)
> a bug-fixed and improved version of what we already have in
> drivers/reset/reset-lantiq.c. The driver is NOT simply copy and paste
> because the register layout was greatly simplified for the newer SoCs
> (for which there is reset-intel) compared to the older ones
> (reset-lantiq).
> Dilip's suggestion (in my own words) is that you take his new
> reset-intel driver, then we will work on porting reset-lantiq over to
> that so in the end we can drop the reset-lantiq driver. This approach
> means more work for me (as I am probably the one who then has to do
> the work to port reset-lantiq over to reset-intel). I'm happy to do
> that work if you think that it's worth following this approach.
> So I want your opinion on this before I spend any effort on porting
> reset-lantiq over to reset-intel.
>
>
> Martin

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

[Dilip Kota]

Hi Martin and Philipp,


On 20/9/2019 10:47 AM, Dilip Kota wrote:
> Hi Martin,
>
> On 9/20/2019 3:51 AM, Martin Blumenstingl wrote:
>> Hi Dilip,
>>
>> (sorry for the late reply)
>>
>> On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota 
>> <eswara.kota@linux.intel.com> wrote:
>> [...]
>>> The major difference between the vrx200 and lgm is:
>>> 1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm
>>> has one single register region.
>>> 2.) Register offsets and bit offsets are different.
>>>
>>> So enhancing the intel-reset-syscon.c to provide compatibility/support
>>> for vrx200.
>>> Please check the below dtsi binding proposal and let me know your view.
>>>
>>> rcu0:reset-controller@00000000 {
>>> compatible= "intel,rcu-lgm";
>>> reg = <0x0000000 0x80000>, <reg_set2 size>, <reg_set3 size>,
>>> <reg_set4 size>;
>> I'm not sure that I understand what are reg_set2/3/4 for
>> the first resource (0x80000 at 0x0) already covers the whole LGM RCU,
>> so what is the purpose of the other register resources
> Yes, as you said the first register resource is enough for LGM RCU as 
> registers are at one single region. Whereas in older SoCs RCU 
> registers are at different regions, so for that reason reg_set2/3/4 
> are used.
>
> Driver will decide in reading the no. of register resources based on 
> the "struct of_device_id".
>
> Regards,
> Dilip
>>
>>> intel,global-reset = <0x10 30>;
>>> #reset-cells = <3>;
>>> };
>>>
>>> "#reset-cells":
>>> const:3
>>> description: |
>>> The 1st cell is the reset register offset.
>>> The 2nd cell is the reset set bit offset.
>>> The 3rd cell is the reset status bit offset.
>> I think this will work fine for VRX200 (and even older SoCs)
>> as you have described in your previous emails we can determine the
>> status offset from the reset offset using a simple if/else
>>
>> for LGM I like your initial suggestion with #reset-cells = <2> because
>> it's easier to read and write.
>>
>>> Reset driver takes care of parsing the register address "reg" as per the
>>> ".data" structure in struct of_device_id.
>>> Reset driver takes care of traversing the status register offset.
>> the differentiation between two and three #reset-cells can also happen
>> based on the struct of_device_id:
>> - the LGM implementation would simply also use the reset bit as status
>> bit (only two cells are needed)
>> - the implementation for earlier SoCs would parse the third cell and
>> use that as status bit
>>
>> Philipp, can you please share your opinion on how to move forward with
>> the reset-intel driver from this series?
>> The reset_control_ops from the reset-intel driver are (in my opinion)
>> a bug-fixed and improved version of what we already have in
>> drivers/reset/reset-lantiq.c. The driver is NOT simply copy and paste
>> because the register layout was greatly simplified for the newer SoCs
>> (for which there is reset-intel) compared to the older ones
>> (reset-lantiq).
>> Dilip's suggestion (in my own words) is that you take his new
>> reset-intel driver, then we will work on porting reset-lantiq over to
>> that so in the end we can drop the reset-lantiq driver. This approach
>> means more work for me (as I am probably the one who then has to do
>> the work to port reset-lantiq over to reset-intel). I'm happy to do
>> that work if you think that it's worth following this approach.
>> So I want your opinion on this before I spend any effort on porting
>> reset-lantiq over to reset-intel.

I will start implementing this design in the next patch version along 
with the other changes suggested in this patch review, please let me 
know if you have other thoughts in this design.

Regards,
Dilip

>>
>>
>> Martin

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

[Philipp Zabel]

Hi Martin, Dilip,

On Thu, Sep 19, 2019 at 09:51:48PM +0200, Martin Blumenstingl wrote:
> Hi Dilip,
> 
> (sorry for the late reply)

Same, sorry for the delay.

> On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota <eswara.kota@linux.intel.com> wrote:
> [...]
> > The major difference between the vrx200 and lgm is:
> > 1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm
> > has one single register region.
> > 2.) Register offsets and bit offsets are different.
> >
> > So enhancing the intel-reset-syscon.c to provide compatibility/support
> > for vrx200.
> > Please check the below dtsi binding proposal and let me know your view.
> >
> > rcu0:reset-controller@00000000 {
> >      compatible= "intel,rcu-lgm";
> >      reg = <0x0000000 0x80000>, <reg_set2 size>, <reg_set3 size>,
> > <reg_set4 size>;
> I'm not sure that I understand what are reg_set2/3/4 for
> the first resource (0x80000 at 0x0) already covers the whole LGM RCU,
> so what is the purpose of the other register resources
> 
> >     intel,global-reset = <0x10 30>;
> >     #reset-cells = <3>;
> > };
> >
> > "#reset-cells":
> >    const:3
> >    description: |
> >      The 1st cell is the reset register offset.
> >      The 2nd cell is the reset set bit offset.
> >      The 3rd cell is the reset status bit offset.
> I think this will work fine for VRX200 (and even older SoCs)
> as you have described in your previous emails we can determine the
> status offset from the reset offset using a simple if/else
> 
> for LGM I like your initial suggestion with #reset-cells = <2> because
> it's easier to read and write.
>
> > Reset driver takes care of parsing the register address "reg" as per the
> > ".data" structure in struct of_device_id.
> > Reset driver takes care of traversing the status register offset.
> the differentiation between two and three #reset-cells can also happen
> based on the struct of_device_id:
> - the LGM implementation would simply also use the reset bit as status
> bit (only two cells are needed)
> - the implementation for earlier SoCs would parse the third cell and
> use that as status bit
> 
> Philipp, can you please share your opinion on how to move forward with
> the reset-intel driver from this series?

That all sounds reasonable for VRX200/LGM to me.

> because the register layout was greatly simplified for the newer SoCs
> (for which there is reset-intel) compared to the older ones
> (reset-lantiq).
> Dilip's suggestion (in my own words) is that you take his new
> reset-intel driver, then we will work on porting reset-lantiq over to
> that so in the end we can drop the reset-lantiq driver.

Just to be sure, you are suggesting to add support for the current
lantiq,reset binding to the reset-intel driver at a later point? I
see no reason not to do that, but I'm also not quite sure what the
benefit will be over just keeping reset-lantiq as is?

> This approach means more work for me (as I am probably the one who
> then has to do the work to port reset-lantiq over to reset-intel).

More work than what alternative?

> I'm happy to do that work if you think that it's worth following this
> approach.  So I want your opinion on this before I spend any effort on
> porting reset-lantiq over to reset-intel.

Reset drivers are typically so simple, I'm not quite sure whether it is
worth to integrate multiple drivers if it complicates matters too much.
In this case though I expect it would just be adding support for a
custom .of_xlate and lantiq specific register property parsing?

regards
Philipp

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

[Martin Blumenstingl]

Hi Philipp,

On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel <pza@pengutronix.de> wrote:
[...]
> > because the register layout was greatly simplified for the newer SoCs
> > (for which there is reset-intel) compared to the older ones
> > (reset-lantiq).
> > Dilip's suggestion (in my own words) is that you take his new
> > reset-intel driver, then we will work on porting reset-lantiq over to
> > that so in the end we can drop the reset-lantiq driver.
>
> Just to be sure, you are suggesting to add support for the current
> lantiq,reset binding to the reset-intel driver at a later point? I
> see no reason not to do that, but I'm also not quite sure what the
> benefit will be over just keeping reset-lantiq as is?
according to Chuan and Dilip the current reset-lantiq implementation
is wrong [0].
my understanding is that the Lantiq and Intel LGM reset controllers
are identical except:
- the Lantiq variant uses a weird register layout (reset and status
registers not at consecutive offsets)
- the bits of the reset and status registers sometimes don't match on
the Lantiq variant
- the Intel variant has a dedicated registers area for the reset
controller registers, while the Lantiq variant mixes them with various
other functionality (for example: USB2 PHYs)

> > This approach means more work for me (as I am probably the one who
> > then has to do the work to port reset-lantiq over to reset-intel).
>
> More work than what alternative?
compared to "fixing" the existing reset-lantiq driver (reset callback)
and then (instead of adding a new driver) integrating Intel LGM
support into reset-lantiq

> > I'm happy to do that work if you think that it's worth following this
> > approach.  So I want your opinion on this before I spend any effort on
> > porting reset-lantiq over to reset-intel.
>
> Reset drivers are typically so simple, I'm not quite sure whether it is
> worth to integrate multiple drivers if it complicates matters too much.
> In this case though I expect it would just be adding support for a
> custom .of_xlate and lantiq specific register property parsing?
yes, that's how I understand the Lantiq and Intel reset controllers:
- reset/status/assert/deassert callbacks would be shared across all variants
- register parsing and of_xlate are SoC specific


Martin


[0] https://www.spinics.net/lists/devicetree/msg305951.html

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

[Dilip Kota]

Hi Martin,Philipp,

On 10/8/2019 3:53 AM, Martin Blumenstingl wrote:
> Hi Philipp,
>
> On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel <pza@pengutronix.de> wrote:
> [...]
>>> because the register layout was greatly simplified for the newer SoCs
>>> (for which there is reset-intel) compared to the older ones
>>> (reset-lantiq).
>>> Dilip's suggestion (in my own words) is that you take his new
>>> reset-intel driver, then we will work on porting reset-lantiq over to
>>> that so in the end we can drop the reset-lantiq driver.
>> Just to be sure, you are suggesting to add support for the current
>> lantiq,reset binding to the reset-intel driver at a later point? I
>> see no reason not to do that, but I'm also not quite sure what the
>> benefit will be over just keeping reset-lantiq as is?
> according to Chuan and Dilip the current reset-lantiq implementation
> is wrong [0].
> my understanding is that the Lantiq and Intel LGM reset controllers
> are identical except:
> - the Lantiq variant uses a weird register layout (reset and status
> registers not at consecutive offsets)
> - the bits of the reset and status registers sometimes don't match on
> the Lantiq variant
> - the Intel variant has a dedicated registers area for the reset
> controller registers, while the Lantiq variant mixes them with various
> other functionality (for example: USB2 PHYs)
>
>>> This approach means more work for me (as I am probably the one who
>>> then has to do the work to port reset-lantiq over to reset-intel).
>> More work than what alternative?
> compared to "fixing" the existing reset-lantiq driver (reset callback)
> and then (instead of adding a new driver) integrating Intel LGM
> support into reset-lantiq

Integrating Intel LGM support into reset-lantiq boils down to re-writing 
reset-lantiq driver as intel-reset driver and adding Lantiq variant 
support. Why because reset-lantiq driver is not according to hardware 
design[1].

I see the final best solution is to integrate Lantiq variant driver to 
intel-reset driver.[1]
I hope you guys are ok with it. Please let me know your view.


Regards,
Dilip

[1]: https://www.spinics.net/lists/devicetree/msg308930.html

>
>>> I'm happy to do that work if you think that it's worth following this
>>> approach.  So I want your opinion on this before I spend any effort on
>>> porting reset-lantiq over to reset-intel.
>> Reset drivers are typically so simple, I'm not quite sure whether it is
>> worth to integrate multiple drivers if it complicates matters too much.
>> In this case though I expect it would just be adding support for a
>> custom .of_xlate and lantiq specific register property parsing?
> yes, that's how I understand the Lantiq and Intel reset controllers:
> - reset/status/assert/deassert callbacks would be shared across all variants
> - register parsing and of_xlate are SoC specific
>
>
> Martin
>
>
> [0] https://www.spinics.net/lists/devicetree/msg305951.html

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

[Philipp Zabel]

Hi Martin,

On Mon, 2019-10-07 at 21:53 +0200, Martin Blumenstingl wrote:
> Hi Philipp,
> 
> On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel <pza@pengutronix.de> wrote:
> [...]
> > > because the register layout was greatly simplified for the newer SoCs
> > > (for which there is reset-intel) compared to the older ones
> > > (reset-lantiq).
> > > Dilip's suggestion (in my own words) is that you take his new
> > > reset-intel driver, then we will work on porting reset-lantiq over to
> > > that so in the end we can drop the reset-lantiq driver.
> > 
> > Just to be sure, you are suggesting to add support for the current
> > lantiq,reset binding to the reset-intel driver at a later point? I
> > see no reason not to do that, but I'm also not quite sure what the
> > benefit will be over just keeping reset-lantiq as is?
> 
> according to Chuan and Dilip the current reset-lantiq implementation
> is wrong [0].

The only issue seems to be the .reset callback, which doesn't have any
users anway.

> my understanding is that the Lantiq and Intel LGM reset controllers
> are identical except:
> - the Lantiq variant uses a weird register layout (reset and status
> registers not at consecutive offsets)
> - the bits of the reset and status registers sometimes don't match on
> the Lantiq variant

Thank you, so these are a good explanation for why the DT bindings
should be different.

> - the Intel variant has a dedicated registers area for the reset
> controller registers, while the Lantiq variant mixes them with various
> other functionality (for example: USB2 PHYs)

I'm not quite sure I understand why the intel driver is using syscon,
then. Either way, it shouldn't make a big difference if regmap is used
anyway. 

> > > This approach means more work for me (as I am probably the one who
> > > then has to do the work to port reset-lantiq over to reset-intel).
> > 
> > More work than what alternative?
> 
> compared to "fixing" the existing reset-lantiq driver (reset callback)

That is still something you could do, or just drop the .reset callback
because there are no reset consumers using it anyway.

One correct thing to do would be to identify those self-clearing reset
bits and to disallow calling assert/deassert on them.

> and then (instead of adding a new driver) integrating Intel LGM
> support into reset-lantiq

Since at this point I'm not even sure whether merging the two at all is
better than keeping them separate, I have no opinion on whether merging
intel support into the lantiq driver or the other way around is
preferable.

> > > I'm happy to do that work if you think that it's worth following this
> > > approach.  So I want your opinion on this before I spend any effort on
> > > porting reset-lantiq over to reset-intel.
> > 
> > Reset drivers are typically so simple, I'm not quite sure whether it is
> > worth to integrate multiple drivers if it complicates matters too much.
> > In this case though I expect it would just be adding support for a
> > custom .of_xlate and lantiq specific register property parsing?
> 
> yes, that's how I understand the Lantiq and Intel reset controllers:
> - reset/status/assert/deassert callbacks would be shared across all variants
> - register parsing and of_xlate are SoC specific

Ok. If that turns out to be less rather than more boilerplate than two
separate drivers, that should be fine.

regards
Philipp

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

[Dilip Kota]

Hi Philipp,

On 10/8/2019 11:56 PM, Philipp Zabel wrote:
> Hi Martin,
>
> On Mon, 2019-10-07 at 21:53 +0200, Martin Blumenstingl wrote:
>> Hi Philipp,
>>
>> On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel <pza@pengutronix.de> wrote:
>> [...]
>>>> because the register layout was greatly simplified for the newer SoCs
>>>> (for which there is reset-intel) compared to the older ones
>>>> (reset-lantiq).
>>>> Dilip's suggestion (in my own words) is that you take his new
>>>> reset-intel driver, then we will work on porting reset-lantiq over to
>>>> that so in the end we can drop the reset-lantiq driver.
>>> Just to be sure, you are suggesting to add support for the current
>>> lantiq,reset binding to the reset-intel driver at a later point? I
>>> see no reason not to do that, but I'm also not quite sure what the
>>> benefit will be over just keeping reset-lantiq as is?
>> according to Chuan and Dilip the current reset-lantiq implementation
>> is wrong [0].
> The only issue seems to be the .reset callback, which doesn't have any
> users anway.
The DT binding of reset-lantiq driver is also having issue. I have 
explained here [1].
>
>> my understanding is that the Lantiq and Intel LGM reset controllers
>> are identical except:
>> - the Lantiq variant uses a weird register layout (reset and status
>> registers not at consecutive offsets)
>> - the bits of the reset and status registers sometimes don't match on
>> the Lantiq variant
> Thank you, so these are a good explanation for why the DT bindings
> should be different.
>
>> - the Intel variant has a dedicated registers area for the reset
>> controller registers, while the Lantiq variant mixes them with various
>> other functionality (for example: USB2 PHYs)
> I'm not quite sure I understand why the intel driver is using syscon,
> then. Either way, it shouldn't make a big difference if regmap is used
> anyway.
Yes, we decided to remove the syscon and use the regmap.[2]
>
>>>> This approach means more work for me (as I am probably the one who
>>>> then has to do the work to port reset-lantiq over to reset-intel).
>>> More work than what alternative?
>> compared to "fixing" the existing reset-lantiq driver (reset callback)
> That is still something you could do, or just drop the .reset callback
> because there are no reset consumers using it anyway.
>
> One correct thing to do would be to identify those self-clearing reset
> bits and to disallow calling assert/deassert on them.
>
>> and then (instead of adding a new driver) integrating Intel LGM
>> support into reset-lantiq
> Since at this point I'm not even sure whether merging the two at all is
> better than keeping them separate, I have no opinion on whether merging
> intel support into the lantiq driver or the other way around is
> preferable.
>
>>>> I'm happy to do that work if you think that it's worth following this
>>>> approach.  So I want your opinion on this before I spend any effort on
>>>> porting reset-lantiq over to reset-intel.
>>> Reset drivers are typically so simple, I'm not quite sure whether it is
>>> worth to integrate multiple drivers if it complicates matters too much.
>>> In this case though I expect it would just be adding support for a
>>> custom .of_xlate and lantiq specific register property parsing?
>> yes, that's how I understand the Lantiq and Intel reset controllers:
>> - reset/status/assert/deassert callbacks would be shared across all variants
>> - register parsing and of_xlate are SoC specific
> Ok. If that turns out to be less rather than more boilerplate than two
> separate drivers, that should be fine.

Thanks Philipp for your time and briefly explaining your view.

Regards,
Dilip

[1]: https://www.spinics.net/lists/devicetree/msg308930.html
[2]: https://lkml.org/lkml/2019/9/2/289

> regards
> Philipp