On 9/10/20 9:42 PM, Crystal Guo wrote:

On Wed, 2020-09-09 at 23:39 +0800, Suman Anna wrote:

quoted

On 9/8/20 9:57 PM, Crystal Guo wrote:

quoted

On Thu, 2020-09-03 at 07:40 +0800, Suman Anna wrote:

quoted

Hi Crystal,

On 8/16/20 10:03 PM, Crystal Guo wrote:

quoted

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

There are multiple changes in this same patch. I think you should split this
functionality away from the change for the regmap_update_bits() to
regmap_write_bits(), similar to what you have done in your v2 Patch 4.

Thanks for your suggestion.
I will split this patch in the next version.

quoted

quoted

Such as reset-qcom-aoss.c, it integrates assert and deassert together
by 'reset' method. MTK Socs also need this method to perform reset.

Signed-off-by: Crystal Guo <redacted>
---
 drivers/reset/reset-ti-syscon.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
index a2635c21db7f..08289342f9af 100644
--- a/drivers/reset/reset-ti-syscon.c
+++ b/drivers/reset/reset-ti-syscon.c

@@ -15,6 +15,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/delay.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>

@@ -56,6 +57,7 @@ struct ti_syscon_reset_data {
 	struct regmap *regmap;
 	struct ti_syscon_reset_control *controls;
 	unsigned int nr_controls;
+	unsigned int reset_duration_us;
 };
 
 #define to_ti_syscon_reset_data(rcdev)	\

@@ -89,7 +91,7 @@ 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);
+	return regmap_write_bits(data->regmap, control->assert_offset, mask, value);
 }
 
 /**

@@ -120,7 +122,7 @@ 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);
+	return regmap_write_bits(data->regmap, control->deassert_offset, mask, value);
 }
 
 /**

@@ -158,9 +160,26 @@ 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);
+	int ret;
+
+	ret = ti_syscon_reset_assert(rcdev, id);
+	if (ret)
+		return ret;
+
+	if (data->reset_duration_us)
+		usleep_range(data->reset_duration_us, data->reset_duration_us * 2);
+
+	return ti_syscon_reset_deassert(rcdev, id);

I echo Philipp's comments [1] from your original v1 series about this. We don't
need a property to distinguish this, but you could add a flag using match data
and Mediatek compatible, and use that within this function, or optionally set
this ops based on compatible (whatever is preferred by Philipp).

regards
Suman

[1] https://patchwork.kernel.org/comment/23519193/

Hi Suman, Philipp

Which method would you recommend more?
1. like v2 patch, but assign the flag "data->assert_deassert_together"
directly (maybe rename "assert_deassert_together" to
"reset_op_available")

2. use Mediatek compatible to decide the reset handler available or not.

I would go with this option. Anyway, I think you might have to add the reset SoC
data as well, based on Rob's comment on the binding.

regards
Suman

Thanks for your suggestions
I will add the following changes in the next version, 
please correct me if there is any misunderstanding.
1). revert ti-syscon-reset.txt add a new mediatek reset binding doc.
2). split the patch [v4,3/4] with the change for the
regmap_update_bits() to regmap_write_bits() and the change to integrate
assert and deassert together.
3). add the reset SoC data, which contains the flag "reset_op_available"
to decide the reset handler available or not.

You would also need to add your SoC-specific data equivalent of the current
ti,reset-bits in your infra node. Please see Rob's comments on patch 2. Also,
your new Mediatek binding should be in YAML format.

regards
Suman

4). separate the dts patch from this patch sets

quoted

quoted

Thanks
Crystal

quoted

quoted

+}
+
 static const struct reset_control_ops ti_syscon_reset_ops = {
 	.assert		= ti_syscon_reset_assert,
 	.deassert	= ti_syscon_reset_deassert,
+	.reset		= ti_syscon_reset,
 	.status		= ti_syscon_reset_status,
 };
 

@@ -204,6 +223,9 @@ static int ti_syscon_reset_probe(struct platform_device *pdev)
 		controls[i].flags = be32_to_cpup(list++);
 	}
 
+	of_property_read_u32(pdev->dev.of_node,	"reset-duration-us",
+				&data->reset_duration_us);
+
 	data->rcdev.ops = &ti_syscon_reset_ops;
 	data->rcdev.owner = THIS_MODULE;
 	data->rcdev.of_node = np;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel