[v2,0/6] introduce TI reset controller for MT8192 SoC

[v2,0/6] introduce TI reset controller for MT8192 SoC

[Crystal Guo]

v1 changes:
https://patchwork.kernel.org/patch/11690523/
https://patchwork.kernel.org/patch/11690527/

Crystal Guo (6):
  dt-binding: reset-controller: ti: add assert-deassert-together
    property
  dt-binding: reset-controller: ti: add update-force property
  dt-binding: reset-controller: ti: add generic-reset to compatible
  reset-controller: ti: introduce a new reset handler
  reset-controller: ti: Introduce force-update method
  arm64: dts: mt8192: add infracfg_rst node

 .../bindings/reset/ti-syscon-reset.txt        |  5 +++
 arch/arm64/boot/dts/mediatek/mt8192.dtsi      | 13 ++++++-
 drivers/reset/reset-ti-syscon.c               | 35 +++++++++++++++++--
 3 files changed, 50 insertions(+), 3 deletions(-)

[v2,1/6] dt-binding: reset-controller: ti: add assert-deassert-together property

[Crystal Guo]

add assert-deassert-together property to allow device to do serialized
assert and deassert operations in a single step by 'reset' method.

Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
---
 Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
index 86945502ccb5..903d1979942f 100644
--- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
+++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
@@ -59,6 +59,9 @@ Required properties:
 Please also refer to Documentation/devicetree/bindings/reset/reset.txt for
 common reset controller usage by consumers.
 
+Optional properties:
+- assert-deassert-together: Allow device to do serialized assert and deassert operations in a single step by 'reset' method.
+
 Example:
 --------
 The following example demonstrates a syscon node, the reset controller node
-- 
2.18.0

[v2,2/6] dt-binding: reset-controller: ti: add update-force property

[Crystal Guo]

add update-force property to force the write operation
in case the read already happens to return the correct value.

Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
---
 Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
index 903d1979942f..d551161ae785 100644
--- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
+++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
@@ -61,6 +61,7 @@ common reset controller usage by consumers.
 
 Optional properties:
 - assert-deassert-together: Allow device to do serialized assert and deassert operations in a single step by 'reset' method.
+- update-force: Force the write operation in case the read already happens to return the correct value.
 
 Example:
 --------
-- 
2.18.0

[v2,3/6] dt-binding: reset-controller: ti: add generic-reset to compatible

[Crystal Guo]

The TI syscon reset controller provides a common reset management,
and should be suitable for other SOCs. Add compatible "generic-reset",
which denotes to use a common reset-controller driver.

Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
---
 Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
index d551161ae785..e36d3631eab2 100644
--- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
+++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
@@ -25,6 +25,7 @@ Required properties:
 			    "ti,k2l-pscrst"
 			    "ti,k2hk-pscrst"
 			    "ti,syscon-reset"
+			    "generic-reset", "ti,syscon-reset"
  - #reset-cells		: Should be 1. Please see the reset consumer node below
 			  for usage details
  - ti,reset-bits	: Contains the reset control register information
-- 
2.18.0

[v2,4/6] reset-controller: ti: introduce a new reset handler

[Crystal Guo]

Add ti_syscon_reset() to integrate assert and deassert together.
If some modules need do serialized assert and deassert operations
to reset itself, reset_control_reset can be called for convenience.

Change-Id: I9046992b115a46f3594de57fa89c6a2de9957d49
---
 drivers/reset/reset-ti-syscon.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
index a2635c21db7f..1c74bcb9a6c3 100644
--- a/drivers/reset/reset-ti-syscon.c
+++ b/drivers/reset/reset-ti-syscon.c
@@ -56,6 +56,7 @@ struct ti_syscon_reset_data {
 	struct regmap *regmap;
 	struct ti_syscon_reset_control *controls;
 	unsigned int nr_controls;
+	bool assert_deassert_together;
 };
 
 #define to_ti_syscon_reset_data(rcdev)	\
@@ -158,10 +159,24 @@ static int ti_syscon_reset_status(struct reset_controller_dev *rcdev,
 		!(control->flags & STATUS_SET);
 }
 
+static int ti_syscon_reset(struct reset_controller_dev *rcdev,
+			  unsigned long id)
+{
+	struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
+
+	if (data->assert_deassert_together) {
+		ti_syscon_reset_assert(rcdev, id);
+		return ti_syscon_reset_deassert(rcdev, id);
+	} else {
+		return -ENOTSUPP;
+	}
+}
+
 static const struct reset_control_ops ti_syscon_reset_ops = {
 	.assert		= ti_syscon_reset_assert,
 	.deassert	= ti_syscon_reset_deassert,
 	.status		= ti_syscon_reset_status,
+	.reset		= ti_syscon_reset,
 };
 
 static int ti_syscon_reset_probe(struct platform_device *pdev)
@@ -204,6 +219,11 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
 		controls[i].flags = be32_to_cpup(list++);
 	}
 
+	if (of_property_read_bool(np, "assert-deassert-together"))
+		data->assert_deassert_together = true;
+	else
+		data->assert_deassert_together = false;
+
 	data->rcdev.ops = &ti_syscon_reset_ops;
 	data->rcdev.owner = THIS_MODULE;
 	data->rcdev.of_node = np;
-- 
2.18.0

[v2,5/6] reset-controller: ti: Introduce force-update method

[Crystal Guo]

Introduce force-update method for assert and deassert interface,
which force the write operation in case the read already happens
to return the correct value.

Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
---
 drivers/reset/reset-ti-syscon.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
index 1c74bcb9a6c3..f4baf78afd14 100644
--- a/drivers/reset/reset-ti-syscon.c
+++ b/drivers/reset/reset-ti-syscon.c
@@ -57,6 +57,7 @@ struct ti_syscon_reset_data {
 	struct ti_syscon_reset_control *controls;
 	unsigned int nr_controls;
 	bool assert_deassert_together;
+	bool update_force;
 };
 
 #define to_ti_syscon_reset_data(rcdev)	\
@@ -90,7 +91,10 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
 	mask = BIT(control->assert_bit);
 	value = (control->flags & ASSERT_SET) ? mask : 0x0;
 
-	return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
+	if (data->update_force)
+		return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
+	else
+		return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
 }
 
 /**
@@ -121,7 +125,10 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
 	mask = BIT(control->deassert_bit);
 	value = (control->flags & DEASSERT_SET) ? mask : 0x0;
 
-	return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
+	if (data->update_force)
+		return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
+	else
+		return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
 }
 
 /**
@@ -223,6 +230,10 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
 		data->assert_deassert_together = true;
 	else
 		data->assert_deassert_together = false;
+	if (of_property_read_bool(np, "update-force"))
+		data->update_force = true;
+	else
+		data->update_force = false;
 
 	data->rcdev.ops = &ti_syscon_reset_ops;
 	data->rcdev.owner = THIS_MODULE;
-- 
2.18.0

[v2,6/6] arm64: dts: mt8192: add infracfg_rst node

[Crystal Guo]

add infracfg_rst node which is for MT8192 platform

Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8192.dtsi | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
index 931e1ca17220..c97eff3aa48d 100644
--- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
@@ -10,6 +10,7 @@
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/pinctrl/mt8192-pinfunc.h>
 #include <dt-bindings/power/mt8192-power.h>
+#include <dt-bindings/reset/ti-syscon.h>
 
 / {
 	compatible = "mediatek,mt8192";
@@ -219,9 +220,19 @@
 		};
 
 		infracfg: infracfg@10001000 {
-			compatible = "mediatek,mt8192-infracfg", "syscon";
+			compatible = "mediatek,mt8192-infracfg", "syscon", "simple-mfd";
 			reg = <0 0x10001000 0 0x1000>;
 			#clock-cells = <1>;
+
+			infracfg_rst: reset-controller {
+				compatible = "generic-reset", "ti,syscon-reset";
+				#reset-cells = <1>;
+				ti,reset-bits = <
+					0x140 15 0x144 15 0 0 (ASSERT_SET | DEASSERT_SET | STATUS_NONE) /* 0: pcie */
+				>;
+				assert-deassert-together;
+				update-force;
+			};
 		};
 
 		pericfg: pericfg@10003000 {
-- 
2.18.0

Re: [v2,5/6] reset-controller: ti: Introduce force-update method

[Stanley Chu]

On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> Introduce force-update method for assert and deassert interface,
> which force the write operation in case the read already happens
> to return the correct value.
> 
> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> ---
>  drivers/reset/reset-ti-syscon.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index 1c74bcb9a6c3..f4baf78afd14 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -57,6 +57,7 @@ struct ti_syscon_reset_data {
>  	struct ti_syscon_reset_control *controls;
>  	unsigned int nr_controls;
>  	bool assert_deassert_together;
> +	bool update_force;
>  };
>  
>  #define to_ti_syscon_reset_data(rcdev)	\
> @@ -90,7 +91,10 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
>  	mask = BIT(control->assert_bit);
>  	value = (control->flags & ASSERT_SET) ? mask : 0x0;
>  
> -	return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
> +	if (data->update_force)
> +		return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
> +	else
> +		return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
>  }
>  
>  /**
> @@ -121,7 +125,10 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
>  	mask = BIT(control->deassert_bit);
>  	value = (control->flags & DEASSERT_SET) ? mask : 0x0;
>  
> -	return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
> +	if (data->update_force)
> +		return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
> +	else
> +		return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
>  }
>  
>  /**
> @@ -223,6 +230,10 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
>  		data->assert_deassert_together = true;
>  	else
>  		data->assert_deassert_together = false;
> +	if (of_property_read_bool(np, "update-force"))
> +		data->update_force = true;
> +	else
> +		data->update_force = false;
>  
>  	data->rcdev.ops = &ti_syscon_reset_ops;
>  	data->rcdev.owner = THIS_MODULE;

This patch is good to help MediaTek drivers' transition to use
ti-syscon-reset-controller, also not have side effect to existed users.

Acked-by: Stanley Chu <stanley.chu@mediatek.com>

Re: [v2,4/6] reset-controller: ti: introduce a new reset handler

[Stanley Chu]

On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> Add ti_syscon_reset() to integrate assert and deassert together.
> If some modules need do serialized assert and deassert operations
> to reset itself, reset_control_reset can be called for convenience.
> 
> Change-Id: I9046992b115a46f3594de57fa89c6a2de9957d49

Please drop "Change-Id" tags.

> ---
>  drivers/reset/reset-ti-syscon.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index a2635c21db7f..1c74bcb9a6c3 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -56,6 +56,7 @@ struct ti_syscon_reset_data {
>  	struct regmap *regmap;
>  	struct ti_syscon_reset_control *controls;
>  	unsigned int nr_controls;
> +	bool assert_deassert_together;
>  };
>  
>  #define to_ti_syscon_reset_data(rcdev)	\
> @@ -158,10 +159,24 @@ static int ti_syscon_reset_status(struct reset_controller_dev *rcdev,
>  		!(control->flags & STATUS_SET);
>  }
>  
> +static int ti_syscon_reset(struct reset_controller_dev *rcdev,
> +			  unsigned long id)
> +{
> +	struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
> +
> +	if (data->assert_deassert_together) {
> +		ti_syscon_reset_assert(rcdev, id);
> +		return ti_syscon_reset_deassert(rcdev, id);
> +	} else {
> +		return -ENOTSUPP;
> +	}
> +}
> +
>  static const struct reset_control_ops ti_syscon_reset_ops = {
>  	.assert		= ti_syscon_reset_assert,
>  	.deassert	= ti_syscon_reset_deassert,
>  	.status		= ti_syscon_reset_status,
> +	.reset		= ti_syscon_reset,
>  };
>  
>  static int ti_syscon_reset_probe(struct platform_device *pdev)
> @@ -204,6 +219,11 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
>  		controls[i].flags = be32_to_cpup(list++);
>  	}
>  
> +	if (of_property_read_bool(np, "assert-deassert-together"))
> +		data->assert_deassert_together = true;
> +	else
> +		data->assert_deassert_together = false;
> +
>  	data->rcdev.ops = &ti_syscon_reset_ops;
>  	data->rcdev.owner = THIS_MODULE;
>  	data->rcdev.of_node = np;

Perhaps please provide the reason why you shall add this new method? Any
existed or upcoming users?

Thanks,

Stanley Chu

Re: [v2,5/6] reset-controller: ti: Introduce force-update method

[Philipp Zabel]

Hi Crystal,

On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> Introduce force-update method for assert and deassert interface,
> which force the write operation in case the read already happens
> to return the correct value.
> 
> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>

Added Suman and Andrew for confirmation: I think writing unconditionally
can't break any existing user. Just changing to regmap_write_bits()
instead of adding the update-force property as in v1 should be fine.

regards
Philipp

> ---
>  drivers/reset/reset-ti-syscon.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index 1c74bcb9a6c3..f4baf78afd14 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -57,6 +57,7 @@ struct ti_syscon_reset_data {
>  	struct ti_syscon_reset_control *controls;
>  	unsigned int nr_controls;
>  	bool assert_deassert_together;
> +	bool update_force;
>  };
>  
>  #define to_ti_syscon_reset_data(rcdev)	\
> @@ -90,7 +91,10 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
>  	mask = BIT(control->assert_bit);
>  	value = (control->flags & ASSERT_SET) ? mask : 0x0;
>  
> -	return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
> +	if (data->update_force)
> +		return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
> +	else
> +		return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
>  }
>  
>  /**
> @@ -121,7 +125,10 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
>  	mask = BIT(control->deassert_bit);
>  	value = (control->flags & DEASSERT_SET) ? mask : 0x0;
>  
> -	return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
> +	if (data->update_force)
> +		return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
> +	else
> +		return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
>  }
>  
>  /**
> @@ -223,6 +230,10 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
>  		data->assert_deassert_together = true;
>  	else
>  		data->assert_deassert_together = false;
> +	if (of_property_read_bool(np, "update-force"))
> +		data->update_force = true;
> +	else
> +		data->update_force = false;
>  
>  	data->rcdev.ops = &ti_syscon_reset_ops;
>  	data->rcdev.owner = THIS_MODULE;
> -- 
> 2.18.0

Re: [v2,4/6] reset-controller: ti: introduce a new reset handler

[Philipp Zabel]

On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> Add ti_syscon_reset() to integrate assert and deassert together.
> If some modules need do serialized assert and deassert operations
> to reset itself, reset_control_reset can be called for convenience.
> 
> Change-Id: I9046992b115a46f3594de57fa89c6a2de9957d49
> ---
>  drivers/reset/reset-ti-syscon.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index a2635c21db7f..1c74bcb9a6c3 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -56,6 +56,7 @@ struct ti_syscon_reset_data {
>  	struct regmap *regmap;
>  	struct ti_syscon_reset_control *controls;
>  	unsigned int nr_controls;
> +	bool assert_deassert_together;
>  };
>  
>  #define to_ti_syscon_reset_data(rcdev)	\
> @@ -158,10 +159,24 @@ static int ti_syscon_reset_status(struct reset_controller_dev *rcdev,
>  		!(control->flags & STATUS_SET);
>  }
>  
> +static int ti_syscon_reset(struct reset_controller_dev *rcdev,
> +			  unsigned long id)
> +{
> +	struct ti_syscon_reset_data *data = to_ti_syscon_reset_data(rcdev);
> +
> +	if (data->assert_deassert_together) {
> +		ti_syscon_reset_assert(rcdev, id);
> +		return ti_syscon_reset_deassert(rcdev, id);

Even if your hardware engineers guarantee that no delays are necessary
between assert and deassert for any peripheral used with your reset
controller that may use this function, this is not generic.

I wonder if it would be better to define a minimum delay similarly to
reset-simple [1].

[1] https://lore.kernel.org/lkml/be2cecb2654e68385561a15df7967c7723d5531d.1590594512.git-series.maxime@cerno.tech/

> +	} else {
> +		return -ENOTSUPP;
> +	}
> +}
> +
>  static const struct reset_control_ops ti_syscon_reset_ops = {
>  	.assert		= ti_syscon_reset_assert,
>  	.deassert	= ti_syscon_reset_deassert,
>  	.status		= ti_syscon_reset_status,
> +	.reset		= ti_syscon_reset,
>  };
>  
>  static int ti_syscon_reset_probe(struct platform_device *pdev)
> @@ -204,6 +219,11 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
>  		controls[i].flags = be32_to_cpup(list++);
>  	}
>  
> +	if (of_property_read_bool(np, "assert-deassert-together"))
> +		data->assert_deassert_together = true;
> +	else
> +		data->assert_deassert_together = false;
> +

This could just be assigned directly, but as said above, I think it
might be better to have a reset-duration-us property or similar.

regards
Philipp

Re: [v2,3/6] dt-binding: reset-controller: ti: add generic-reset to compatible

[Philipp Zabel]

On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> The TI syscon reset controller provides a common reset management,
> and should be suitable for other SOCs. Add compatible "generic-reset",
> which denotes to use a common reset-controller driver.
> 
> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> ---
>  Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> index d551161ae785..e36d3631eab2 100644
> --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> @@ -25,6 +25,7 @@ Required properties:
>  			    "ti,k2l-pscrst"
>  			    "ti,k2hk-pscrst"
>  			    "ti,syscon-reset"
> +			    "generic-reset", "ti,syscon-reset"
>   - #reset-cells		: Should be 1. Please see the reset consumer node below
>  			  for usage details
>   - ti,reset-bits	: Contains the reset control register information
> -- 
> 2.18.0

My understanding is that it would be better to add a mtk specific
compatible instead of adding this "generic-reset", especially since we
can't guarantee this binding will be considered generic in the future.
I think there is nothing wrong with specifying
	compatible = "mtk,your-compatible", "ti,syscon-reset";
in your device tree if your hardware is indeed compatible with the
specified "ti,syscon-reset" binding, but I may be wrong: Therefore,
please add devicetree@vger.kernel.org to Cc: for binding changes.

regards
Philipp

Re: [v2,3/6] dt-binding: reset-controller: ti: add generic-reset to compatible

[Yingjoe Chen]

On Tue, 2020-08-04 at 10:15 +0200, Philipp Zabel wrote:
> On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> > The TI syscon reset controller provides a common reset management,
> > and should be suitable for other SOCs. Add compatible "generic-reset",
> > which denotes to use a common reset-controller driver.
> > 
> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> > ---
> >  Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > index d551161ae785..e36d3631eab2 100644
> > --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > @@ -25,6 +25,7 @@ Required properties:
> >  			    "ti,k2l-pscrst"
> >  			    "ti,k2hk-pscrst"
> >  			    "ti,syscon-reset"
> > +			    "generic-reset", "ti,syscon-reset"
> >   - #reset-cells		: Should be 1. Please see the reset consumer node below
> >  			  for usage details
> >   - ti,reset-bits	: Contains the reset control register information
> > -- 
> > 2.18.0
> 
> My understanding is that it would be better to add a mtk specific
> compatible instead of adding this "generic-reset", especially since we
> can't guarantee this binding will be considered generic in the future.
> I think there is nothing wrong with specifying
> 	compatible = "mtk,your-compatible", "ti,syscon-reset";
> in your device tree if your hardware is indeed compatible with the
> specified "ti,syscon-reset" binding, but I may be wrong: Therefore,
> please add devicetree@vger.kernel.org to Cc: for binding changes.
> 

Hi Philipp,

This would work.
But having "ti" in mtk dts raise alarm for some people inside and
outside of MTK. It would save us some explanation if we could use a more
generic name.

Joe.C

Re: [v2,5/6] reset-controller: ti: Introduce force-update method

[Yingjoe Chen]

On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> Introduce force-update method for assert and deassert interface,
> which force the write operation in case the read already happens
> to return the correct value.
> 
> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> ---
>  drivers/reset/reset-ti-syscon.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index 1c74bcb9a6c3..f4baf78afd14 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -57,6 +57,7 @@ struct ti_syscon_reset_data {
>  	struct ti_syscon_reset_control *controls;
>  	unsigned int nr_controls;
>  	bool assert_deassert_together;
> +	bool update_force;
>  };
>  
>  #define to_ti_syscon_reset_data(rcdev)	\
> @@ -90,7 +91,10 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
>  	mask = BIT(control->assert_bit);
>  	value = (control->flags & ASSERT_SET) ? mask : 0x0;
>  
> -	return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
> +	if (data->update_force)
> +		return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
> +	else
> +		return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
>  }
>  
>  /**
> @@ -121,7 +125,10 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
>  	mask = BIT(control->deassert_bit);
>  	value = (control->flags & DEASSERT_SET) ? mask : 0x0;
>  
> -	return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
> +	if (data->update_force)
> +		return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
> +	else
> +		return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
>  }
>  
>  /**
> @@ -223,6 +230,10 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
>  		data->assert_deassert_together = true;
>  	else
>  		data->assert_deassert_together = false;
> +	if (of_property_read_bool(np, "update-force"))
> +		data->update_force = true;
> +	else
> +		data->update_force = false;

You are using 'force-update' in commit message, and I think that a
better one.
Please change it if we still need this one

Joe.C

Re: [v2,5/6] reset-controller: ti: Introduce force-update method

[Crystal Guo]

On Tue, 2020-08-04 at 15:03 +0800, Philipp Zabel wrote:
> Hi Crystal,
> 
> On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> > Introduce force-update method for assert and deassert interface,
> > which force the write operation in case the read already happens
> > to return the correct value.
> > 
> > Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
> 
> Added Suman and Andrew for confirmation: I think writing unconditionally
> can't break any existing user. Just changing to regmap_write_bits()
> instead of adding the update-force property as in v1 should be fine.
> 
> regards
> Philipp
> 
Hi Suman, Andrew,

Can you help to give some suggestions about this change.
Is this can be changed to write unconditionally, or should I add a
update-force property to force the write operation.

Best regards
Crystal.

> > ---
> >  drivers/reset/reset-ti-syscon.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> > index 1c74bcb9a6c3..f4baf78afd14 100644
> > --- a/drivers/reset/reset-ti-syscon.c
> > +++ b/drivers/reset/reset-ti-syscon.c
> > @@ -57,6 +57,7 @@ struct ti_syscon_reset_data {
> >  	struct ti_syscon_reset_control *controls;
> >  	unsigned int nr_controls;
> >  	bool assert_deassert_together;
> > +	bool update_force;
> >  };
> >  
> >  #define to_ti_syscon_reset_data(rcdev)	\
> > @@ -90,7 +91,10 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
> >  	mask = BIT(control->assert_bit);
> >  	value = (control->flags & ASSERT_SET) ? mask : 0x0;
> >  
> > -	return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
> > +	if (data->update_force)
> > +		return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
> > +	else
> > +		return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
> >  }
> >  
> >  /**
> > @@ -121,7 +125,10 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
> >  	mask = BIT(control->deassert_bit);
> >  	value = (control->flags & DEASSERT_SET) ? mask : 0x0;
> >  
> > -	return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
> > +	if (data->update_force)
> > +		return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
> > +	else
> > +		return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
> >  }
> >  
> >  /**
> > @@ -223,6 +230,10 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
> >  		data->assert_deassert_together = true;
> >  	else
> >  		data->assert_deassert_together = false;
> > +	if (of_property_read_bool(np, "update-force"))
> > +		data->update_force = true;
> > +	else
> > +		data->update_force = false;
> >  
> >  	data->rcdev.ops = &ti_syscon_reset_ops;
> >  	data->rcdev.owner = THIS_MODULE;
> > -- 
> > 2.18.0

Re: [v2,5/6] reset-controller: ti: Introduce force-update method

[Suman Anna]

Hi Crystal,

On 8/10/20 1:57 AM, Crystal Guo wrote:
> On Tue, 2020-08-04 at 15:03 +0800, Philipp Zabel wrote:
>> Hi Crystal,
>>
>> On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
>>> Introduce force-update method for assert and deassert interface,
>>> which force the write operation in case the read already happens
>>> to return the correct value.
>>>
>>> Signed-off-by: Crystal Guo <crystal.guo@mediatek.com>
>>
>> Added Suman and Andrew for confirmation: I think writing unconditionally
>> can't break any existing user. Just changing to regmap_write_bits()
>> instead of adding the update-force property as in v1 should be fine.
>>
>> regards
>> Philipp
>>
> Hi Suman, Andrew,
> 
> Can you help to give some suggestions about this change.
> Is this can be changed to write unconditionally, or should I add a
> update-force property to force the write operation.

Sorry for the delay on this one, I have tested your latest v4, and everything is
functional on the TI SoCs. We do use the same register for asserting and
deasserting and is not self-clearing, and have additional non-reset related bits
in the register, so this change doesn't impact us. I have some minor
comments/questions that I will post on your v4.

Removing Andrew to not have the emails bounce.

regards
Suman

> 
> Best regards
> Crystal.
> 
>>> ---
>>>  drivers/reset/reset-ti-syscon.c | 15 +++++++++++++--
>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
>>> index 1c74bcb9a6c3..f4baf78afd14 100644
>>> --- a/drivers/reset/reset-ti-syscon.c
>>> +++ b/drivers/reset/reset-ti-syscon.c
>>> @@ -57,6 +57,7 @@ struct ti_syscon_reset_data {
>>>  	struct ti_syscon_reset_control *controls;
>>>  	unsigned int nr_controls;
>>>  	bool assert_deassert_together;
>>> +	bool update_force;
>>>  };
>>>  
>>>  #define to_ti_syscon_reset_data(rcdev)	\
>>> @@ -90,7 +91,10 @@ static int ti_syscon_reset_assert(struct reset_controller_dev *rcdev,
>>>  	mask = BIT(control->assert_bit);
>>>  	value = (control->flags & ASSERT_SET) ? mask : 0x0;
>>>  
>>> -	return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
>>> +	if (data->update_force)
>>> +		return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
>>> +	else
>>> +		return regmap_update_bits(data->regmap, control->assert_offset, mask, value);
>>>  }
>>>  
>>>  /**
>>> @@ -121,7 +125,10 @@ static int ti_syscon_reset_deassert(struct reset_controller_dev *rcdev,
>>>  	mask = BIT(control->deassert_bit);
>>>  	value = (control->flags & DEASSERT_SET) ? mask : 0x0;
>>>  
>>> -	return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
>>> +	if (data->update_force)
>>> +		return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
>>> +	else
>>> +		return regmap_update_bits(data->regmap, control->deassert_offset, mask, value);
>>>  }
>>>  
>>>  /**
>>> @@ -223,6 +230,10 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
>>>  		data->assert_deassert_together = true;
>>>  	else
>>>  		data->assert_deassert_together = false;
>>> +	if (of_property_read_bool(np, "update-force"))
>>> +		data->update_force = true;
>>> +	else
>>> +		data->update_force = false;
>>>  
>>>  	data->rcdev.ops = &ti_syscon_reset_ops;
>>>  	data->rcdev.owner = THIS_MODULE;
>>> -- 
>>> 2.18.0
>