Re: [PATCH v6 2/6] clk: qcom: Allow custom reset ops

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Akhil,

On Wed, Aug 31, 2022 at 10:48:23AM +0530, Akhil P Oommen wrote:
> Allow soc specific clk drivers to specify a custom reset operation. We
> will use this in an upcoming patch to allow gpucc driver to specify a
> differet reset operation for cx_gdsc.
> 
> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> 
> (no changes since v3)
> 
> Changes in v3:
> - Use pointer to const for "struct qcom_reset_ops" in qcom_reset_map (Krzysztof)
> 
> Changes in v2:
> - Return error when a particular custom reset op is not implemented. (Dmitry)
> 
>  drivers/clk/qcom/reset.c | 27 +++++++++++++++++++++++++++
>  drivers/clk/qcom/reset.h |  8 ++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/clk/qcom/reset.c b/drivers/clk/qcom/reset.c
> index 819d194..b7ae4a3 100644
> --- a/drivers/clk/qcom/reset.c
> +++ b/drivers/clk/qcom/reset.c
> @@ -13,6 +13,21 @@
>  
>  static int qcom_reset(struct reset_controller_dev *rcdev, unsigned long id)
>  {
> +	struct qcom_reset_controller *rst;
> +	const struct qcom_reset_map *map;
> +
> +	rst = to_qcom_reset_controller(rcdev);
> +	map = &rst->reset_map[id];
> +
> +	if (map->ops && map->ops->reset)
> +		return map->ops->reset(map->priv);
> +	/*
> +	 * If custom ops is implemented but just not this callback, return
> +	 * error
> +	 */
> +	else if (map->ops)
> +		return -EOPNOTSUPP;
> +

It doesn't seem necessary to stack reset_ops -> qcom_reset_ops for what
you need here.
Just add an optional const struct reset_ops * to qcom_cc_desc and feed
that into qcom_cc_really_probe to replace &qcom_reset_ops.

[...]
> +struct qcom_reset_ops {
> +	int (*reset)(void *priv);
> +	int (*assert)(void *priv);
> +	int (*deassert)(void *priv);

Why add assert and deassert ops? There doesn't seem to be any user.

> +};
> +
>  struct qcom_reset_map {
>  	unsigned int reg;
>  	u8 bit;
> +	const struct qcom_reset_ops *ops;
> +	void *priv;

Adding per-reset ops + priv counters seems excessive to me.

Are you expecting different reset controls in the same reset controller
to have completely different ops at some point? If so, I would wonder
whether it wouldn't be better to split the reset controller in that
case. Either way, for the GDSC collapse workaround this does not seem
to be required at all.

regards
Philipp