Re: [v2,4/6] reset-controller: ti: introduce a new reset handler
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.max...@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,4/6] reset-controller: ti: introduce a new reset handler
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 = _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
[v2,4/6] reset-controller: ti: introduce a new reset handler
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 = _syscon_reset_ops;
data->rcdev.owner = THIS_MODULE;
data->rcdev.of_node = np;
--
2.18.0
