devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Add Armada8K reset controller support
@ 2025-02-14  6:58 Wilson Ding
  2025-02-14  6:58 ` Wilson Ding
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Wilson Ding @ 2025-02-14  6:58 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel
  Cc: andrew, gregory.clement, sebastian.hesselbarth, robh, krzk+dt,
	conor+dt, p.zabel, salee, gakula, Wilson Ding

Armada8K has one simple register for unit soft reset, which is part of
the system controller register area. The simple reset code doesn't
support register access via regmap for the syscon devices. This patch
series added new ops for reset line operation to make the simple reset
code compatible for syscon device. And add Armada8K support then.

Wilson Ding (4):
  reset: simple: Add syscon device compatible
  reset: simple: Add support for Armada8K reset controller
  dt-bindings: cp110: Document the reset controller
  arm64: dts: marvell: cp11x: Add reset controller node

 .../arm/marvell/cp110-system-controller.txt   |  32 +++++
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi |   6 +
 drivers/reset/reset-simple.c                  | 126 +++++++++++++++---
 include/linux/reset/reset-simple.h            |  11 ++
 4 files changed, 154 insertions(+), 21 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 0/4] Add Armada8K reset controller support
  2025-02-14  6:58 [PATCH 0/4] Add Armada8K reset controller support Wilson Ding
@ 2025-02-14  6:58 ` Wilson Ding
  2025-02-14  6:58 ` [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible Wilson Ding
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Wilson Ding @ 2025-02-14  6:58 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel
  Cc: andrew, gregory.clement, sebastian.hesselbarth, robh, krzk+dt,
	conor+dt, p.zabel, salee, gakula, Wilson Ding

Armada8K has one simple register for unit soft reset, which is part of
the system controller register area. The simple reset code doesn't
support register access via regmap for the syscon devices. This patch
series added new ops for reset line operation to make the simple reset
code compatible for syscon device. And add Armada8K support then.

Wilson Ding (4):
  reset: simple: Add syscon device compatible
  reset: simple: Add support for Armada8K reset controller
  dt-bindings: cp110: Document the reset controller
  arm64: dts: marvell: cp11x: Add reset controller node

 .../arm/marvell/cp110-system-controller.txt   |  32 +++++
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi |   6 +
 drivers/reset/reset-simple.c                  | 126 +++++++++++++++---
 include/linux/reset/reset-simple.h            |  11 ++
 4 files changed, 154 insertions(+), 21 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible
  2025-02-14  6:58 [PATCH 0/4] Add Armada8K reset controller support Wilson Ding
  2025-02-14  6:58 ` Wilson Ding
@ 2025-02-14  6:58 ` Wilson Ding
  2025-02-14  8:48   ` Krzysztof Kozlowski
  2025-02-14 11:54   ` Philipp Zabel
  2025-02-14  6:58 ` [PATCH 2/4] [PATCH 2/4] reset: simple: Add support for Armada8K reset controller Wilson Ding
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 15+ messages in thread
From: Wilson Ding @ 2025-02-14  6:58 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel
  Cc: andrew, gregory.clement, sebastian.hesselbarth, robh, krzk+dt,
	conor+dt, p.zabel, salee, gakula, Wilson Ding

Introduce the new ops for updating reset line and getting status. Thus,
the reset controller can be accessed through either direct I/O or regmap
interfaces. It enables the support of the syscon devices with the simple
reset code. To adapt the DT binding of the syscon device, the number of
reset lines must be specified in device data.

Signed-off-by: Wilson Ding <dingwei@marvell.com>
---
 drivers/reset/reset-simple.c       | 117 +++++++++++++++++++++++------
 include/linux/reset/reset-simple.h |  11 +++
 2 files changed, 107 insertions(+), 21 deletions(-)

diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
index 276067839830..e4e777d40a79 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -15,8 +15,10 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/reset-controller.h>
 #include <linux/reset/reset-simple.h>
 #include <linux/spinlock.h>
@@ -27,10 +29,9 @@ to_reset_simple_data(struct reset_controller_dev *rcdev)
 	return container_of(rcdev, struct reset_simple_data, rcdev);
 }
 
-static int reset_simple_update(struct reset_controller_dev *rcdev,
+static int reset_simple_update_mmio(struct reset_simple_data *data,
 			       unsigned long id, bool assert)
 {
-	struct reset_simple_data *data = to_reset_simple_data(rcdev);
 	int reg_width = sizeof(u32);
 	int bank = id / (reg_width * BITS_PER_BYTE);
 	int offset = id % (reg_width * BITS_PER_BYTE);
@@ -51,16 +52,40 @@ static int reset_simple_update(struct reset_controller_dev *rcdev,
 	return 0;
 }
 
+static int reset_simple_update_regmap(struct reset_simple_data *data,
+				      unsigned long id, bool assert)
+{
+	int reg_width = sizeof(u32);
+	int bank = id / (reg_width * BITS_PER_BYTE);
+	int offset = id % (reg_width * BITS_PER_BYTE);
+	u32 mask, val;
+
+	mask = BIT(offset);
+
+	if (assert ^ data->active_low)
+		val = mask;
+	else
+		val = 0;
+
+	return regmap_write_bits(data->regmap,
+				 data->reg_offset + (bank * reg_width),
+				 mask, val);
+}
+
 static int reset_simple_assert(struct reset_controller_dev *rcdev,
 			       unsigned long id)
 {
-	return reset_simple_update(rcdev, id, true);
+	struct reset_simple_data *data = to_reset_simple_data(rcdev);
+
+	return data->ops.update(data, id, true);
 }
 
 static int reset_simple_deassert(struct reset_controller_dev *rcdev,
 				 unsigned long id)
 {
-	return reset_simple_update(rcdev, id, false);
+	struct reset_simple_data *data = to_reset_simple_data(rcdev);
+
+	return data->ops.update(data, id, false);
 }
 
 static int reset_simple_reset(struct reset_controller_dev *rcdev,
@@ -81,10 +106,9 @@ static int reset_simple_reset(struct reset_controller_dev *rcdev,
 	return reset_simple_deassert(rcdev, id);
 }
 
-static int reset_simple_status(struct reset_controller_dev *rcdev,
-			       unsigned long id)
+static int reset_simple_status_mmio(struct reset_simple_data *data,
+			     unsigned long id)
 {
-	struct reset_simple_data *data = to_reset_simple_data(rcdev);
 	int reg_width = sizeof(u32);
 	int bank = id / (reg_width * BITS_PER_BYTE);
 	int offset = id % (reg_width * BITS_PER_BYTE);
@@ -95,6 +119,31 @@ static int reset_simple_status(struct reset_controller_dev *rcdev,
 	return !(reg & BIT(offset)) ^ !data->status_active_low;
 }
 
+static int reset_simple_status_regmap(struct reset_simple_data *data,
+				    unsigned long id)
+{
+	int reg_width = sizeof(u32);
+	int bank = id / (reg_width * BITS_PER_BYTE);
+	int offset = id % (reg_width * BITS_PER_BYTE);
+	u32 reg;
+	int ret;
+
+	ret = regmap_read(data->regmap, data->reg_offset + (bank * reg_width),
+			  &reg);
+	if (ret)
+		return ret;
+
+	return !(reg & BIT(offset)) ^ !data->status_active_low;
+}
+
+static int reset_simple_status(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct reset_simple_data *data = to_reset_simple_data(rcdev);
+
+	return data->ops.status(data, id);
+}
+
 const struct reset_control_ops reset_simple_ops = {
 	.assert		= reset_simple_assert,
 	.deassert	= reset_simple_deassert,
@@ -118,6 +167,7 @@ struct reset_simple_devdata {
 	u32 nr_resets;
 	bool active_low;
 	bool status_active_low;
+	bool syscon_dev;
 };
 
 #define SOCFPGA_NR_BANKS	8
@@ -171,26 +221,51 @@ static int reset_simple_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
-	membase = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
-	if (IS_ERR(membase))
-		return PTR_ERR(membase);
+	if (devdata && devdata->syscon_dev) {
+		data->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
+		if (IS_ERR(data->regmap))
+			return PTR_ERR(data->regmap);
 
-	spin_lock_init(&data->lock);
-	data->membase = membase;
-	data->rcdev.owner = THIS_MODULE;
-	data->rcdev.nr_resets = resource_size(res) * BITS_PER_BYTE;
-	data->rcdev.ops = &reset_simple_ops;
-	data->rcdev.of_node = dev->of_node;
+		if (device_property_read_u32(&pdev->dev, "offset",
+					     &data->reg_offset))
+			data->reg_offset = devdata->reg_offset;
 
-	if (devdata) {
-		reg_offset = devdata->reg_offset;
-		if (devdata->nr_resets)
-			data->rcdev.nr_resets = devdata->nr_resets;
+		if (devdata->nr_resets == 0) {
+			dev_err(dev, "no reset line\n");
+			return -EINVAL;
+		}
+
+		data->rcdev.nr_resets = devdata->nr_resets;
 		data->active_low = devdata->active_low;
 		data->status_active_low = devdata->status_active_low;
+
+		data->ops.update = reset_simple_update_regmap;
+		data->ops.status = reset_simple_status_regmap;
+	} else {
+		membase = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+		if (IS_ERR(membase))
+			return PTR_ERR(membase);
+		data->membase = membase;
+		data->rcdev.nr_resets = resource_size(res) * BITS_PER_BYTE;
+
+		if (devdata) {
+			reg_offset = devdata->reg_offset;
+			if (devdata->nr_resets)
+				data->rcdev.nr_resets = devdata->nr_resets;
+			data->active_low = devdata->active_low;
+			data->status_active_low = devdata->status_active_low;
+		}
+
+		data->membase += reg_offset;
+
+		data->ops.update = reset_simple_update_mmio;
+		data->ops.status = reset_simple_status_mmio;
 	}
 
-	data->membase += reg_offset;
+	spin_lock_init(&data->lock);
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.ops = &reset_simple_ops;
+	data->rcdev.of_node = dev->of_node;
 
 	return devm_reset_controller_register(dev, &data->rcdev);
 }
diff --git a/include/linux/reset/reset-simple.h b/include/linux/reset/reset-simple.h
index c3e44f45b0f7..cbcf9e364dd4 100644
--- a/include/linux/reset/reset-simple.h
+++ b/include/linux/reset/reset-simple.h
@@ -13,9 +13,17 @@
 #define __RESET_SIMPLE_H__
 
 #include <linux/io.h>
+#include <linux/regmap.h>
 #include <linux/reset-controller.h>
 #include <linux/spinlock.h>
 
+struct reset_simple_data;
+
+struct reset_simple_line_ops {
+	int (*update)(struct reset_simple_data *data, unsigned long id, bool assert);
+	int (*status)(struct reset_simple_data *data, unsigned long id);
+};
+
 /**
  * struct reset_simple_data - driver data for simple reset controllers
  * @lock: spinlock to protect registers during read-modify-write cycles
@@ -37,6 +45,9 @@
 struct reset_simple_data {
 	spinlock_t			lock;
 	void __iomem			*membase;
+	struct regmap			*regmap;
+	u32				reg_offset;
+	struct reset_simple_line_ops	ops;
 	struct reset_controller_dev	rcdev;
 	bool				active_low;
 	bool				status_active_low;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/4] [PATCH 2/4] reset: simple: Add support for Armada8K reset controller
  2025-02-14  6:58 [PATCH 0/4] Add Armada8K reset controller support Wilson Ding
  2025-02-14  6:58 ` Wilson Ding
  2025-02-14  6:58 ` [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible Wilson Ding
@ 2025-02-14  6:58 ` Wilson Ding
  2025-02-14  8:48   ` Krzysztof Kozlowski
  2025-02-14  6:58 ` [PATCH 3/4] [PATCH 3/4] dt-bindings: cp110: Document the " Wilson Ding
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Wilson Ding @ 2025-02-14  6:58 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel
  Cc: andrew, gregory.clement, sebastian.hesselbarth, robh, krzk+dt,
	conor+dt, p.zabel, salee, gakula, Wilson Ding

Armada8k has one register for unit soft-reset configuration within the
system controller register area.

Signed-off-by: Wilson Ding <dingwei@marvell.com>
---
 drivers/reset/reset-simple.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
index e4e777d40a79..a792b913e1b1 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -183,6 +183,13 @@ static const struct reset_simple_devdata reset_simple_active_low = {
 	.status_active_low = true,
 };
 
+static const struct reset_simple_devdata reset_simple_armada8k = {
+	.nr_resets = 32,
+	.active_low = true,
+	.status_active_low = true,
+	.syscon_dev = true,
+};
+
 static const struct of_device_id reset_simple_dt_ids[] = {
 	{ .compatible = "altr,stratix10-rst-mgr",
 		.data = &reset_simple_socfpga },
@@ -203,6 +210,8 @@ static const struct of_device_id reset_simple_dt_ids[] = {
 		.data = &reset_simple_active_low },
 	{ .compatible = "sophgo,sg2042-reset",
 		.data = &reset_simple_active_low },
+	{ .compatible = "marvell,armada8k-reset",
+		.data = &reset_simple_armada8k },
 	{ /* sentinel */ },
 };
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/4] [PATCH 3/4] dt-bindings: cp110: Document the reset controller
  2025-02-14  6:58 [PATCH 0/4] Add Armada8K reset controller support Wilson Ding
                   ` (2 preceding siblings ...)
  2025-02-14  6:58 ` [PATCH 2/4] [PATCH 2/4] reset: simple: Add support for Armada8K reset controller Wilson Ding
@ 2025-02-14  6:58 ` Wilson Ding
  2025-02-14  8:45   ` Krzysztof Kozlowski
  2025-02-14  6:58 ` [PATCH 4/4] [PATCH 4/4] arm64: dts: marvell: cp11x: Add reset controller node Wilson Ding
  2025-02-19  0:42 ` [PATCH 0/4] Add Armada8K reset controller support Rob Herring (Arm)
  5 siblings, 1 reply; 15+ messages in thread
From: Wilson Ding @ 2025-02-14  6:58 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel
  Cc: andrew, gregory.clement, sebastian.hesselbarth, robh, krzk+dt,
	conor+dt, p.zabel, salee, gakula, Wilson Ding

Add new compatible to be used for CP110's reset controller, and document
the supported reset lines.

Signed-off-by: Wilson Ding <dingwei@marvell.com>
---
 .../arm/marvell/cp110-system-controller.txt   | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt b/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt
index 9d5d70c98058..a5cc1360969c 100644
--- a/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt
+++ b/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt
@@ -190,6 +190,38 @@ CP110_LABEL(syscon0): system-controller@440000 {
 
 };
 
+Reset:
+------
+
+The Device Tree node representing this System Controller 0 provides a
+number of reset lines:
+
+The following reset lines are available:
+
+-  0: Audio Software RESETn
+-  1: TDM Software RESETn
+-  2: Interrupt controller unit Software RESETn
+-  3: Packet processor Software RESETn
+-  4: SDIO Software RESETn
+-  7: XOR-1 engine Software RESETn
+-  8: XOR-0 engine Software RESETn
+- 11: PCIe-0 Gen.3 x1 Software RESETn
+- 12: PCIe-1 Gen.3 x1 Software RESETn
+- 13: PCIe Gen.3 x4 Software RESETn
+- 15: SATA port 0 and port 1 Software RESETn
+- 22: USB3 Host 0 Software RESETn
+- 23: USB3 Host 1 Software RESETn
+- 24: USB3 Device Software RESETn
+- 25: EIP150F Software RESETn
+- 26: EIP197 Software RESETn
+- 29: MSS Software RESETn
+
+Required properties:
+
+ - compatible: must be:
+     "marvell,armada8k-reset"
+ - #reset-cells: must be set to 1
+
 SYSTEM CONTROLLER 1
 ===================
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/4] [PATCH 4/4] arm64: dts: marvell: cp11x: Add reset controller node
  2025-02-14  6:58 [PATCH 0/4] Add Armada8K reset controller support Wilson Ding
                   ` (3 preceding siblings ...)
  2025-02-14  6:58 ` [PATCH 3/4] [PATCH 3/4] dt-bindings: cp110: Document the " Wilson Ding
@ 2025-02-14  6:58 ` Wilson Ding
  2025-02-14  8:48   ` Krzysztof Kozlowski
  2025-02-19  0:42 ` [PATCH 0/4] Add Armada8K reset controller support Rob Herring (Arm)
  5 siblings, 1 reply; 15+ messages in thread
From: Wilson Ding @ 2025-02-14  6:58 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel
  Cc: andrew, gregory.clement, sebastian.hesselbarth, robh, krzk+dt,
	conor+dt, p.zabel, salee, gakula, Wilson Ding

The unit soft-reset configuration register is part of the system
controller register.

Signed-off-by: Wilson Ding <dingwei@marvell.com>
---
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
index 161beec0b6b0..b82003df15e0 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
@@ -273,6 +273,12 @@ CP11X_LABEL(gpio2): gpio@140 {
 					 <&CP11X_LABEL(clk) 1 17>;
 				status = "disabled";
 			};
+
+			CP11X_LABEL(soft_reset): soft-reset@268 {
+				compatible = "marvell,armada8k-reset";
+				offset = <0x268>;
+				#reset-cells = <1>;
+			};
 		};
 
 		CP11X_LABEL(syscon1): system-controller@400000 {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/4] [PATCH 3/4] dt-bindings: cp110: Document the reset controller
  2025-02-14  6:58 ` [PATCH 3/4] [PATCH 3/4] dt-bindings: cp110: Document the " Wilson Ding
@ 2025-02-14  8:45   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2025-02-14  8:45 UTC (permalink / raw)
  To: Wilson Ding
  Cc: linux-kernel, devicetree, linux-arm-kernel, andrew,
	gregory.clement, sebastian.hesselbarth, robh, krzk+dt, conor+dt,
	p.zabel, salee, gakula

On Thu, Feb 13, 2025 at 10:58:32PM -0800, Wilson Ding wrote:
> Add new compatible to be used for CP110's reset controller, and document
> the supported reset lines.
> 
> Signed-off-by: Wilson Ding <dingwei@marvell.com>
> ---

Subject: only one prefix

>  .../arm/marvell/cp110-system-controller.txt   | 32 +++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt b/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt
> index 9d5d70c98058..a5cc1360969c 100644
> --- a/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt
> +++ b/Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt
> @@ -190,6 +190,38 @@ CP110_LABEL(syscon0): system-controller@440000 {
>  
>  };
>  
> +Reset:
> +------
> +
> +The Device Tree node representing this System Controller 0 provides a
> +number of reset lines:
> +

No new bindings in TXT. Only DT schema.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible
  2025-02-14  6:58 ` [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible Wilson Ding
@ 2025-02-14  8:48   ` Krzysztof Kozlowski
  2025-02-14 11:54   ` Philipp Zabel
  1 sibling, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2025-02-14  8:48 UTC (permalink / raw)
  To: Wilson Ding
  Cc: linux-kernel, devicetree, linux-arm-kernel, andrew,
	gregory.clement, sebastian.hesselbarth, robh, krzk+dt, conor+dt,
	p.zabel, salee, gakula

On Thu, Feb 13, 2025 at 10:58:30PM -0800, Wilson Ding wrote:
>  #define SOCFPGA_NR_BANKS	8
> @@ -171,26 +221,51 @@ static int reset_simple_probe(struct platform_device *pdev)
>  	if (!data)
>  		return -ENOMEM;
>  
> -	membase = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> -	if (IS_ERR(membase))
> -		return PTR_ERR(membase);
> +	if (devdata && devdata->syscon_dev) {
> +		data->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
> +		if (IS_ERR(data->regmap))
> +			return PTR_ERR(data->regmap);
>  
> -	spin_lock_init(&data->lock);
> -	data->membase = membase;
> -	data->rcdev.owner = THIS_MODULE;
> -	data->rcdev.nr_resets = resource_size(res) * BITS_PER_BYTE;
> -	data->rcdev.ops = &reset_simple_ops;
> -	data->rcdev.of_node = dev->of_node;
> +		if (device_property_read_u32(&pdev->dev, "offset",

Where is this binding documented? This is patch #1, so something anywy
is wrong here (see submitting patches for bindings).

> +					     &data->reg_offset))
> +			data->reg_offset = devdata->reg_offset;
>  
> -	if (devdata) {
> -		reg_offset = devdata->reg_offset;
> -		if (devdata->nr_resets)
> -			data->rcdev.nr_resets = devdata->nr_resets;
> +		if (devdata->nr_resets == 0) {
> +			dev_err(dev, "no reset line\n");
> +			return -EINVAL;
> +		}

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/4] [PATCH 2/4] reset: simple: Add support for Armada8K reset controller
  2025-02-14  6:58 ` [PATCH 2/4] [PATCH 2/4] reset: simple: Add support for Armada8K reset controller Wilson Ding
@ 2025-02-14  8:48   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2025-02-14  8:48 UTC (permalink / raw)
  To: Wilson Ding
  Cc: linux-kernel, devicetree, linux-arm-kernel, andrew,
	gregory.clement, sebastian.hesselbarth, robh, krzk+dt, conor+dt,
	p.zabel, salee, gakula

On Thu, Feb 13, 2025 at 10:58:31PM -0800, Wilson Ding wrote:
> Armada8k has one register for unit soft-reset configuration within the
> system controller register area.
> 
> Signed-off-by: Wilson Ding <dingwei@marvell.com>
> ---
>  drivers/reset/reset-simple.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index e4e777d40a79..a792b913e1b1 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -183,6 +183,13 @@ static const struct reset_simple_devdata reset_simple_active_low = {
>  	.status_active_low = true,
>  };
>  
> +static const struct reset_simple_devdata reset_simple_armada8k = {
> +	.nr_resets = 32,
> +	.active_low = true,
> +	.status_active_low = true,
> +	.syscon_dev = true,
> +};
> +
>  static const struct of_device_id reset_simple_dt_ids[] = {
>  	{ .compatible = "altr,stratix10-rst-mgr",
>  		.data = &reset_simple_socfpga },
> @@ -203,6 +210,8 @@ static const struct of_device_id reset_simple_dt_ids[] = {
>  		.data = &reset_simple_active_low },
>  	{ .compatible = "sophgo,sg2042-reset",
>  		.data = &reset_simple_active_low },
> +	{ .compatible = "marvell,armada8k-reset",

Undocumented compatible.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/4] [PATCH 4/4] arm64: dts: marvell: cp11x: Add reset controller node
  2025-02-14  6:58 ` [PATCH 4/4] [PATCH 4/4] arm64: dts: marvell: cp11x: Add reset controller node Wilson Ding
@ 2025-02-14  8:48   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2025-02-14  8:48 UTC (permalink / raw)
  To: Wilson Ding
  Cc: linux-kernel, devicetree, linux-arm-kernel, andrew,
	gregory.clement, sebastian.hesselbarth, robh, krzk+dt, conor+dt,
	p.zabel, salee, gakula

On Thu, Feb 13, 2025 at 10:58:33PM -0800, Wilson Ding wrote:
> The unit soft-reset configuration register is part of the system
> controller register.
> 
> Signed-off-by: Wilson Ding <dingwei@marvell.com>
> ---
>  arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
> index 161beec0b6b0..b82003df15e0 100644
> --- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
> @@ -273,6 +273,12 @@ CP11X_LABEL(gpio2): gpio@140 {
>  					 <&CP11X_LABEL(clk) 1 17>;
>  				status = "disabled";
>  			};
> +
> +			CP11X_LABEL(soft_reset): soft-reset@268 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Usually: reset-controller

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible
  2025-02-14  6:58 ` [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible Wilson Ding
  2025-02-14  8:48   ` Krzysztof Kozlowski
@ 2025-02-14 11:54   ` Philipp Zabel
  2025-02-14 17:13     ` Wilson Ding
  1 sibling, 1 reply; 15+ messages in thread
From: Philipp Zabel @ 2025-02-14 11:54 UTC (permalink / raw)
  To: Wilson Ding, linux-kernel, devicetree, linux-arm-kernel
  Cc: andrew, gregory.clement, sebastian.hesselbarth, robh, krzk+dt,
	conor+dt, salee, gakula

On Do, 2025-02-13 at 22:58 -0800, Wilson Ding wrote:
> Introduce the new ops for updating reset line and getting status. Thus,
> the reset controller can be accessed through either direct I/O or regmap
> interfaces.

Please don't add a new layer of function pointer indirection, just add
a new struct reset_control_ops for the regmap variant.

> It enables the support of the syscon devices with the simple
> reset code. To adapt the DT binding of the syscon device, the number of
> reset lines must be specified in device data.

If the DT node had a reg property, number of reset lines could be
determined from its size, like for MMIO.

> Signed-off-by: Wilson Ding <dingwei@marvell.com>
> ---
>  drivers/reset/reset-simple.c       | 117 +++++++++++++++++++++++------
>  include/linux/reset/reset-simple.h |  11 +++
>  2 files changed, 107 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index 276067839830..e4e777d40a79 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -15,8 +15,10 @@
>  #include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/regmap.h>
>  #include <linux/reset-controller.h>
>  #include <linux/reset/reset-simple.h>
>  #include <linux/spinlock.h>
> @@ -27,10 +29,9 @@ to_reset_simple_data(struct reset_controller_dev *rcdev)
>  	return container_of(rcdev, struct reset_simple_data, rcdev);
>  }
>  
> -static int reset_simple_update(struct reset_controller_dev *rcdev,
> +static int reset_simple_update_mmio(struct reset_simple_data *data,
>  			       unsigned long id, bool assert)

No need to rename or change the function prototype.

>  {
> -	struct reset_simple_data *data = to_reset_simple_data(rcdev);
>  	int reg_width = sizeof(u32);
>  	int bank = id / (reg_width * BITS_PER_BYTE);
>  	int offset = id % (reg_width * BITS_PER_BYTE);
> @@ -51,16 +52,40 @@ static int reset_simple_update(struct reset_controller_dev *rcdev,
>  	return 0;
>  }
>  
> +static int reset_simple_update_regmap(struct reset_simple_data *data,
> +				      unsigned long id, bool assert)

I'd call this reset_simple_regmap_update().

> +{
> +	int reg_width = sizeof(u32);
> +	int bank = id / (reg_width * BITS_PER_BYTE);
> +	int offset = id % (reg_width * BITS_PER_BYTE);
> +	u32 mask, val;
> +
> +	mask = BIT(offset);
> +
> +	if (assert ^ data->active_low)
> +		val = mask;
> +	else
> +		val = 0;
> +
> +	return regmap_write_bits(data->regmap,
> +				 data->reg_offset + (bank * reg_width),
> +				 mask, val);
> +}
> +
>  static int reset_simple_assert(struct reset_controller_dev *rcdev,
>  			       unsigned long id)
>  {
> -	return reset_simple_update(rcdev, id, true);
> +	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> +
> +	return data->ops.update(data, id, true);
>  }
>  
>  static int reset_simple_deassert(struct reset_controller_dev *rcdev,
>  				 unsigned long id)
>  {
> -	return reset_simple_update(rcdev, id, false);
> +	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> +
> +	return data->ops.update(data, id, false);
>  }

No need for indirection. Better to just add separate
reset_simple_regmap_assert/deassert() functions.

>  static int reset_simple_reset(struct reset_controller_dev *rcdev,
> @@ -81,10 +106,9 @@ static int reset_simple_reset(struct reset_controller_dev *rcdev,
>  	return reset_simple_deassert(rcdev, id);
>  }
>  
> -static int reset_simple_status(struct reset_controller_dev *rcdev,
> -			       unsigned long id)
> +static int reset_simple_status_mmio(struct reset_simple_data *data,
> +			     unsigned long id)
>  {
> -	struct reset_simple_data *data = to_reset_simple_data(rcdev);
>  	int reg_width = sizeof(u32);
>  	int bank = id / (reg_width * BITS_PER_BYTE);
>  	int offset = id % (reg_width * BITS_PER_BYTE);
> @@ -95,6 +119,31 @@ static int reset_simple_status(struct reset_controller_dev *rcdev,
>  	return !(reg & BIT(offset)) ^ !data->status_active_low;
>  }
>  
> +static int reset_simple_status_regmap(struct reset_simple_data *data,
> +				    unsigned long id)
> +{
> +	int reg_width = sizeof(u32);
> +	int bank = id / (reg_width * BITS_PER_BYTE);
> +	int offset = id % (reg_width * BITS_PER_BYTE);
> +	u32 reg;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, data->reg_offset + (bank * reg_width),
> +			  &reg);
> +	if (ret)
> +		return ret;
> +
> +	return !(reg & BIT(offset)) ^ !data->status_active_low;
> +}
> +
> +static int reset_simple_status(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> +
> +	return data->ops.status(data, id);
> +}

Same as above, no need for indirection.
Just add separate reset_simple_regmap_assert/deassert() functions ...

> +
>  const struct reset_control_ops reset_simple_ops = {
>  	.assert		= reset_simple_assert,
>  	.deassert	= reset_simple_deassert,

... and add a const struct reset_control_ops reset_simple_regmap_ops.

regards
Philipp

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible
  2025-02-14 11:54   ` Philipp Zabel
@ 2025-02-14 17:13     ` Wilson Ding
  2025-02-14 18:03       ` Philipp Zabel
  0 siblings, 1 reply; 15+ messages in thread
From: Wilson Ding @ 2025-02-14 17:13 UTC (permalink / raw)
  To: Philipp Zabel, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
  Cc: andrew@lunn.ch, gregory.clement@bootlin.com,
	sebastian.hesselbarth@gmail.com, robh@kernel.org,
	krzk+dt@kernel.org, conor+dt@kernel.org, Sanghoon Lee,
	Geethasowjanya Akula



> -----Original Message-----
> From: Philipp Zabel <p.zabel@pengutronix.de>
> Sent: Friday, February 14, 2025 3:54 AM
> To: Wilson Ding <dingwei@marvell.com>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Cc: andrew@lunn.ch; gregory.clement@bootlin.com;
> sebastian.hesselbarth@gmail.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Sanghoon Lee <salee@marvell.com>; Geethasowjanya
> Akula <gakula@marvell.com>
> Subject: [EXTERNAL] Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon
> device compatible
> 
> On Do, 2025-02-13 at 22:58 -0800, Wilson Ding wrote:
> > Introduce the new ops for updating reset line and getting status.
> > Thus, the reset controller can be accessed through either direct I/O
> > or regmap interfaces.
> 
> Please don't add a new layer of function pointer indirection, just add a new
> struct reset_control_ops for the regmap variant.
> 

If just adding a new struct reset_control_ops for the regmap variant, almost
all the functions will be duplicated for regmap variant. 
Besides reset_simple_regmap_assert/deassert(), we also need to have the
regmap version of reset_simple_update(). Since reset_simple_reset() invokes
reset_simple_regmap_assert/deassert(), it also needs to be duplicated.
In this case, there will be too many redundant codes in this file. I doubt if
it is worth to use the reset simple code. Maybe it's better to fork a new file
for the syscon device, such as 'reset-simple-syscon.c'. What do you say?

> > It enables the support of the syscon devices with the simple reset
> > code. To adapt the DT binding of the syscon device, the number of
> > reset lines must be specified in device data.
> 
> If the DT node had a reg property, number of reset lines could be determined
> from its size, like for MMIO.
> 
> > Signed-off-by: Wilson Ding <dingwei@marvell.com>
> > ---
> >  drivers/reset/reset-simple.c       | 117 +++++++++++++++++++++++------
> >  include/linux/reset/reset-simple.h |  11 +++
> >  2 files changed, 107 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/reset/reset-simple.c
> > b/drivers/reset/reset-simple.c index 276067839830..e4e777d40a79
> 100644
> > --- a/drivers/reset/reset-simple.c
> > +++ b/drivers/reset/reset-simple.c
> > @@ -15,8 +15,10 @@
> >  #include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> > +#include <linux/mfd/syscon.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> >  #include <linux/reset-controller.h>
> >  #include <linux/reset/reset-simple.h>  #include <linux/spinlock.h> @@
> > -27,10 +29,9 @@ to_reset_simple_data(struct reset_controller_dev *rcdev)
> >  	return container_of(rcdev, struct reset_simple_data, rcdev);  }
> >
> > -static int reset_simple_update(struct reset_controller_dev *rcdev,
> > +static int reset_simple_update_mmio(struct reset_simple_data *data,
> >  			       unsigned long id, bool assert)
> 
> No need to rename or change the function prototype.
> 
> >  {
> > -	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> >  	int reg_width = sizeof(u32);
> >  	int bank = id / (reg_width * BITS_PER_BYTE);
> >  	int offset = id % (reg_width * BITS_PER_BYTE); @@ -51,16 +52,40
> @@
> > static int reset_simple_update(struct reset_controller_dev *rcdev,
> >  	return 0;
> >  }
> >
> > +static int reset_simple_update_regmap(struct reset_simple_data *data,
> > +				      unsigned long id, bool assert)
> 
> I'd call this reset_simple_regmap_update().
> 
> > +{
> > +	int reg_width = sizeof(u32);
> > +	int bank = id / (reg_width * BITS_PER_BYTE);
> > +	int offset = id % (reg_width * BITS_PER_BYTE);
> > +	u32 mask, val;
> > +
> > +	mask = BIT(offset);
> > +
> > +	if (assert ^ data->active_low)
> > +		val = mask;
> > +	else
> > +		val = 0;
> > +
> > +	return regmap_write_bits(data->regmap,
> > +				 data->reg_offset + (bank * reg_width),
> > +				 mask, val);
> > +}
> > +
> >  static int reset_simple_assert(struct reset_controller_dev *rcdev,
> >  			       unsigned long id)
> >  {
> > -	return reset_simple_update(rcdev, id, true);
> > +	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> > +
> > +	return data->ops.update(data, id, true);
> >  }
> >
> >  static int reset_simple_deassert(struct reset_controller_dev *rcdev,
> >  				 unsigned long id)
> >  {
> > -	return reset_simple_update(rcdev, id, false);
> > +	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> > +
> > +	return data->ops.update(data, id, false);
> >  }
> 
> No need for indirection. Better to just add separate
> reset_simple_regmap_assert/deassert() functions.
> 

See my reply to the first comment.

> >  static int reset_simple_reset(struct reset_controller_dev *rcdev, @@
> > -81,10 +106,9 @@ static int reset_simple_reset(struct reset_controller_dev
> *rcdev,
> >  	return reset_simple_deassert(rcdev, id);  }
> >
> > -static int reset_simple_status(struct reset_controller_dev *rcdev,
> > -			       unsigned long id)
> > +static int reset_simple_status_mmio(struct reset_simple_data *data,
> > +			     unsigned long id)
> >  {
> > -	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> >  	int reg_width = sizeof(u32);
> >  	int bank = id / (reg_width * BITS_PER_BYTE);
> >  	int offset = id % (reg_width * BITS_PER_BYTE); @@ -95,6 +119,31
> @@
> > static int reset_simple_status(struct reset_controller_dev *rcdev,
> >  	return !(reg & BIT(offset)) ^ !data->status_active_low;  }
> >
> > +static int reset_simple_status_regmap(struct reset_simple_data *data,
> > +				    unsigned long id)
> > +{
> > +	int reg_width = sizeof(u32);
> > +	int bank = id / (reg_width * BITS_PER_BYTE);
> > +	int offset = id % (reg_width * BITS_PER_BYTE);
> > +	u32 reg;
> > +	int ret;
> > +
> > +	ret = regmap_read(data->regmap, data->reg_offset + (bank *
> reg_width),
> > +			  &reg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return !(reg & BIT(offset)) ^ !data->status_active_low; }
> > +
> > +static int reset_simple_status(struct reset_controller_dev *rcdev,
> > +			       unsigned long id)
> > +{
> > +	struct reset_simple_data *data = to_reset_simple_data(rcdev);
> > +
> > +	return data->ops.status(data, id);
> > +}
> 
> Same as above, no need for indirection.
> Just add separate reset_simple_regmap_assert/deassert() functions ...
> 

See my reply to the first comment.

> > +
> >  const struct reset_control_ops reset_simple_ops = {
> >  	.assert		= reset_simple_assert,
> >  	.deassert	= reset_simple_deassert,
> 
> ... and add a const struct reset_control_ops reset_simple_regmap_ops.
> 

See my reply to the first comment.

> regards
> Philipp

- Wilson

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible
  2025-02-14 17:13     ` Wilson Ding
@ 2025-02-14 18:03       ` Philipp Zabel
  2025-02-14 18:40         ` [EXTERNAL] " Wilson Ding
  0 siblings, 1 reply; 15+ messages in thread
From: Philipp Zabel @ 2025-02-14 18:03 UTC (permalink / raw)
  To: Wilson Ding, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
  Cc: andrew@lunn.ch, gregory.clement@bootlin.com,
	sebastian.hesselbarth@gmail.com, robh@kernel.org,
	krzk+dt@kernel.org, conor+dt@kernel.org, Sanghoon Lee,
	Geethasowjanya Akula

On Fr, 2025-02-14 at 17:13 +0000, Wilson Ding wrote:
> 
> > -----Original Message-----
> > From: Philipp Zabel <p.zabel@pengutronix.de>
> > Sent: Friday, February 14, 2025 3:54 AM
> > To: Wilson Ding <dingwei@marvell.com>; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > Cc: andrew@lunn.ch; gregory.clement@bootlin.com;
> > sebastian.hesselbarth@gmail.com; robh@kernel.org; krzk+dt@kernel.org;
> > conor+dt@kernel.org; Sanghoon Lee <salee@marvell.com>; Geethasowjanya
> > Akula <gakula@marvell.com>
> > Subject: [EXTERNAL] Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon
> > device compatible
> > 
> > On Do, 2025-02-13 at 22:58 -0800, Wilson Ding wrote:
> > > Introduce the new ops for updating reset line and getting status.
> > > Thus, the reset controller can be accessed through either direct I/O
> > > or regmap interfaces.
> > 
> > Please don't add a new layer of function pointer indirection, just add a new
> > struct reset_control_ops for the regmap variant.
> > 
> 
> If just adding a new struct reset_control_ops for the regmap variant, almost
> all the functions will be duplicated for regmap variant. 
> Besides reset_simple_regmap_assert/deassert(), we also need to have the
> regmap version of reset_simple_update().

Yes. You could also duplicate/fold update() into assert/deassert().
It is trivial enough and the compiler will do that anyway.

> Since reset_simple_reset() invokes
> reset_simple_regmap_assert/deassert(), it also needs to be
> duplicated.

That one could go through the data->rcdev.ops->assert/deassert function
pointers and be reused. But I wonder if that one function is worth the
added complexity.

> In this case, there will be too many redundant codes in this file. I doubt if
> it is worth to use the reset simple code. Maybe it's better to fork a new file
> for the syscon device, such as 'reset-simple-syscon.c'. What do you say?

That sounds sensible to me.

regards
Philipp

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [EXTERNAL] Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible
  2025-02-14 18:03       ` Philipp Zabel
@ 2025-02-14 18:40         ` Wilson Ding
  0 siblings, 0 replies; 15+ messages in thread
From: Wilson Ding @ 2025-02-14 18:40 UTC (permalink / raw)
  To: Philipp Zabel, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
  Cc: andrew@lunn.ch, gregory.clement@bootlin.com,
	sebastian.hesselbarth@gmail.com, robh@kernel.org,
	krzk+dt@kernel.org, conor+dt@kernel.org, Sanghoon Lee,
	Geethasowjanya Akula



> -----Original Message-----
> From: Philipp Zabel <p.zabel@pengutronix.de>
> Sent: Friday, February 14, 2025 10:03 AM
> To: Wilson Ding <dingwei@marvell.com>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Cc: andrew@lunn.ch; gregory.clement@bootlin.com;
> sebastian.hesselbarth@gmail.com; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; Sanghoon Lee <salee@marvell.com>; Geethasowjanya
> Akula <gakula@marvell.com>
> Subject: [EXTERNAL] Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon
> device compatible
> 
> On Fr, 2025-02-14 at 17: 13 +0000, Wilson Ding wrote: > > > -----Original
> Message----- > > From: Philipp Zabel <p. zabel@ pengutronix. de> > > Sent:
> Friday, February 14, 2025 3: 54 AM > > To: Wilson Ding
> <dingwei@ marvell. com>; 
> On Fr, 2025-02-14 at 17:13 +0000, Wilson Ding wrote:
> >
> > > -----Original Message-----
> > > From: Philipp Zabel <p.zabel@pengutronix.de>
> > > Sent: Friday, February 14, 2025 3:54 AM
> > > To: Wilson Ding <dingwei@marvell.com>; linux-kernel@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > > Cc: andrew@lunn.ch; gregory.clement@bootlin.com;
> > > sebastian.hesselbarth@gmail.com; robh@kernel.org;
> > > krzk+dt@kernel.org;
> > > conor+dt@kernel.org; Sanghoon Lee <salee@marvell.com>;
> > > conor+Geethasowjanya
> > > Akula <gakula@marvell.com>
> > > Subject: [EXTERNAL] Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add
> > > syscon device compatible
> > >
> > > On Do, 2025-02-13 at 22:58 -0800, Wilson Ding wrote:
> > > > Introduce the new ops for updating reset line and getting status.
> > > > Thus, the reset controller can be accessed through either direct
> > > > I/O or regmap interfaces.
> > >
> > > Please don't add a new layer of function pointer indirection, just
> > > add a new struct reset_control_ops for the regmap variant.
> > >
> >
> > If just adding a new struct reset_control_ops for the regmap variant,
> > almost all the functions will be duplicated for regmap variant.
> > Besides reset_simple_regmap_assert/deassert(), we also need to have
> > the regmap version of reset_simple_update().
> 
> Yes. You could also duplicate/fold update() into assert/deassert().
> It is trivial enough and the compiler will do that anyway.
> 
> > Since reset_simple_reset() invokes
> > reset_simple_regmap_assert/deassert(), it also needs to be duplicated.
> 
> That one could go through the data->rcdev.ops->assert/deassert function
> pointers and be reused. But I wonder if that one function is worth the added
> complexity.
> 
> > In this case, there will be too many redundant codes in this file. I
> > doubt if it is worth to use the reset simple code. Maybe it's better
> > to fork a new file for the syscon device, such as 'reset-simple-syscon.c'. What
> do you say?
> 
> That sounds sensible to me.
> 

Well. I will go with the approach of a new driver, which avoids all the
unnecessary complexity and redundance. Thank!

> regards
> Philipp

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/4] Add Armada8K reset controller support
  2025-02-14  6:58 [PATCH 0/4] Add Armada8K reset controller support Wilson Ding
                   ` (4 preceding siblings ...)
  2025-02-14  6:58 ` [PATCH 4/4] [PATCH 4/4] arm64: dts: marvell: cp11x: Add reset controller node Wilson Ding
@ 2025-02-19  0:42 ` Rob Herring (Arm)
  5 siblings, 0 replies; 15+ messages in thread
From: Rob Herring (Arm) @ 2025-02-19  0:42 UTC (permalink / raw)
  To: Wilson Ding
  Cc: gakula, sebastian.hesselbarth, andrew, linux-arm-kernel,
	gregory.clement, salee, devicetree, krzk+dt, linux-kernel,
	conor+dt, p.zabel


On Thu, 13 Feb 2025 22:58:28 -0800, Wilson Ding wrote:
> Armada8K has one simple register for unit soft reset, which is part of
> the system controller register area. The simple reset code doesn't
> support register access via regmap for the syscon devices. This patch
> series added new ops for reset line operation to make the simple reset
> code compatible for syscon device. And add Armada8K support then.
> 
> Wilson Ding (4):
>   reset: simple: Add syscon device compatible
>   reset: simple: Add support for Armada8K reset controller
>   dt-bindings: cp110: Document the reset controller
>   arm64: dts: marvell: cp11x: Add reset controller node
> 
>  .../arm/marvell/cp110-system-controller.txt   |  32 +++++
>  arch/arm64/boot/dts/marvell/armada-cp11x.dtsi |   6 +
>  drivers/reset/reset-simple.c                  | 126 +++++++++++++++---
>  include/linux/reset/reset-simple.h            |  11 ++
>  4 files changed, 154 insertions(+), 21 deletions(-)
> 
> --
> 2.43.0
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/marvell/' for 20250214065833.530276-1-dingwei@marvell.com:

arch/arm64/boot/dts/marvell/cn9131-db.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9130-db.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-8040-puzzle-m801.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-7040-db.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9130-cf-pro.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9130-db-B.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9130-cf-base.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9131-db-B.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-7040-mochabin.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-8040-puzzle-m801.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9131-db.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9132-clearfog.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9131-db-B.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9132-clearfog.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9132-clearfog.dtb: /cp2/config-space@f6000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9131-cf-solidwan.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dtb: /cp0/config-space@f2000000/spi@700680: failed to match any schema with compatible: ['marvell,armada-380-spi']
arch/arm64/boot/dts/marvell/cn9132-db-B.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9130-crb-B.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/ac5x-rd-carrier-cn9131.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9132-db.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9130-crb-A.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9131-cf-solidwan.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9132-db-B.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/ac5x-rd-carrier-cn9131.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9132-db.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9132-db-B.dtb: /cp2/config-space@f6000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/cn9132-db.dtb: /cp2/config-space@f6000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-8040-db.dtb: /cp0/config-space@f2000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']
arch/arm64/boot/dts/marvell/armada-8040-db.dtb: /cp1/config-space@f4000000/system-controller@440000/soft-reset@268: failed to match any schema with compatible: ['marvell,armada8k-reset']






^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2025-02-19  0:42 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-14  6:58 [PATCH 0/4] Add Armada8K reset controller support Wilson Ding
2025-02-14  6:58 ` Wilson Ding
2025-02-14  6:58 ` [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device compatible Wilson Ding
2025-02-14  8:48   ` Krzysztof Kozlowski
2025-02-14 11:54   ` Philipp Zabel
2025-02-14 17:13     ` Wilson Ding
2025-02-14 18:03       ` Philipp Zabel
2025-02-14 18:40         ` [EXTERNAL] " Wilson Ding
2025-02-14  6:58 ` [PATCH 2/4] [PATCH 2/4] reset: simple: Add support for Armada8K reset controller Wilson Ding
2025-02-14  8:48   ` Krzysztof Kozlowski
2025-02-14  6:58 ` [PATCH 3/4] [PATCH 3/4] dt-bindings: cp110: Document the " Wilson Ding
2025-02-14  8:45   ` Krzysztof Kozlowski
2025-02-14  6:58 ` [PATCH 4/4] [PATCH 4/4] arm64: dts: marvell: cp11x: Add reset controller node Wilson Ding
2025-02-14  8:48   ` Krzysztof Kozlowski
2025-02-19  0:42 ` [PATCH 0/4] Add Armada8K reset controller support Rob Herring (Arm)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).