[v6,0/6] clk/qcom: Support gdsc collapse polling using 'reset' interface

Some clients like adreno gpu driver would like to ensure that its gdsc
is collapsed at hardware during a gpu reset sequence. This is because it
has a votable gdsc which could be ON due to a vote from another subsystem
like tz, hyp etc or due to an internal hardware signal. To allow
this, gpucc driver can expose an interface to the client driver using
reset framework. Using this the client driver can trigger a polling within
the gdsc driver.

This series is rebased on top of linus's master branch.

Related discussion: https://patchwork.freedesktop.org/patch/493144/

Changes in v6:
- No code changes in this version. Just captured the Acked-by tags

Changes in v5:
- Nit: Remove a duplicate blank line (Krzysztof)

Changes in v4:
- Update gpu dt-binding schema
- Typo fix in commit text

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)

Akhil P Oommen (6):
  dt-bindings: clk: qcom: Support gpu cx gdsc reset
  clk: qcom: Allow custom reset ops
  clk: qcom: gdsc: Add a reset op to poll gdsc collapse
  clk: qcom: gpucc-sc7280: Add cx collapse reset support
  dt-bindings: drm/msm/gpu: Add optional resets
  arm64: dts: qcom: sc7280: Add Reset support for gpu

 .../devicetree/bindings/display/msm/gpu.yaml       |  6 +++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi               |  3 +++
 drivers/clk/qcom/gdsc.c                            | 23 ++++++++++++++----
 drivers/clk/qcom/gdsc.h                            |  7 ++++++
 drivers/clk/qcom/gpucc-sc7280.c                    | 10 ++++++++
 drivers/clk/qcom/reset.c                           | 27 ++++++++++++++++++++++
 drivers/clk/qcom/reset.h                           |  8 +++++++
 include/dt-bindings/clock/qcom,gpucc-sc7280.h      |  3 +++
 8 files changed, 83 insertions(+), 4 deletions(-)

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

On Wed, Aug 31, 2022 at 10:48:25AM +0530, Akhil P Oommen wrote:
> Allow a consumer driver to poll for cx gdsc collapse through Reset
> framework.
> 
> 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:
> - Convert 'struct qcom_reset_ops cx_gdsc_reset' to 'static const' (Krzysztof)
> 
> Changes in v2:
> - Minor update to use the updated custom reset ops implementation
> 
>  drivers/clk/qcom/gpucc-sc7280.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gpucc-sc7280.c b/drivers/clk/qcom/gpucc-sc7280.c
> index 9a832f2..fece3f4 100644
> --- a/drivers/clk/qcom/gpucc-sc7280.c
> +++ b/drivers/clk/qcom/gpucc-sc7280.c
> @@ -433,12 +433,22 @@ static const struct regmap_config gpu_cc_sc7280_regmap_config = {
>  	.fast_io = true,
>  };
>  
> +static const struct qcom_reset_ops cx_gdsc_reset = {
> +	.reset = gdsc_wait_for_collapse,

This should be accompanied by a comment explaining the not-quite-reset
nature of this workaround, i.e. what is the prerequisite for this to
actually work as expected?

> +};
> +
> +static const struct qcom_reset_map gpucc_sc7280_resets[] = {
> +	[GPU_CX_COLLAPSE] = { .ops = &cx_gdsc_reset, .priv = &cx_gdsc },
> +};
> +
>  static const struct qcom_cc_desc gpu_cc_sc7280_desc = {
>  	.config = &gpu_cc_sc7280_regmap_config,
>  	.clks = gpu_cc_sc7280_clocks,
>  	.num_clks = ARRAY_SIZE(gpu_cc_sc7280_clocks),
>  	.gdscs = gpu_cc_sc7180_gdscs,
>  	.num_gdscs = ARRAY_SIZE(gpu_cc_sc7180_gdscs),
> +	.resets = gpucc_sc7280_resets,
> +	.num_resets = ARRAY_SIZE(gpucc_sc7280_resets),

See my comment on patch 2. I think instead of adding a const struct
qcom_reset_ops * to gpucc_sc7280_resets, this should just add a const
struct reset_control * to gpu_cc_sc7280_desc.

regards
Philipp

On 9/1/2022 3:47 PM, Philipp Zabel wrote:
> 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.
I had a more minimal implementation in v1. But this makes it more 
complete and make it less prone to trip up ourselves in future.
>
>> +};
>> +
>>   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.
Yes, like I responded in patch 4, we need different ops for the same 
reset controller in some gpucc implementations.
For eg: 
https://elixir.bootlin.com/linux/v6.0-rc3/source/drivers/clk/qcom/gpucc-sdm660.c

-Akhil
>
> regards
> Philipp