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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ce70cee90b5d9ecd7b52f6785159553732b9ae06.camel@pengutronix.de>
Date:   Tue, 04 Aug 2020 09:16:02 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Crystal Guo <crystal.guo@...iatek.com>, robh+dt@...nel.org,
        matthias.bgg@...il.com
Cc:     srv_heupstream@...iatek.com, linux-mediatek@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        seiya.wang@...iatek.com, stanley.chu@...iatek.com,
        yingjoe.chen@...iatek.com, fan.chen@...iatek.com,
        yong.liang@...iatek.com, Suman Anna <s-anna@...com>,
        "Andrew F. Davis" <afd@...com>
Subject: 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.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