Re: [PATCH v2 1/2] clk: qcom: gcc-sc7180: Keep the USB GDSC always on

From: Stephen Boyd <sboyd@kernel.org>
To: Bjorn Andersson <andersson@kernel.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	Rajendra Nayak <quic_rjendra@quicinc.com>
Cc: Andy Gross <agross@kernel.org>,
	Konrad Dybcio <konrad.dybcio@somainline.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Johan Hovold <johan+linaro@kernel.org>,
	linux-clk@vger.kernel.org,
	Krishna Kurapati <quic_kriskura@quicinc.com>,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Douglas Anderson <dianders@chromium.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>
Subject: Re: [PATCH v2 1/2] clk: qcom: gcc-sc7180: Keep the USB GDSC always on
Date: Tue, 30 Aug 2022 14:35:31 -0700	[thread overview]
Message-ID: <20220830213533.7C4FCC433C1@smtp.kernel.org> (raw)
In-Reply-To: <5ff21b1e-3af9-36ef-e13e-fa33f526d0e3@quicinc.com>

Quoting Rajendra Nayak (2022-08-29 01:12:02)
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index d3244006c661..0fe017ba901b 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -368,6 +368,10 @@ static int _gdsc_disable(struct gdsc *sc)
>          if (sc->pwrsts & PWRSTS_OFF)
>                  gdsc_clear_mem_on(sc);
> 
> +       /* If the GDSC supports RET, do not explicitly power it off */
> +       if (sc->pwrsts & PWRSTS_RET)
> +               return 0;
> +
>          ret = gdsc_toggle_logic(sc, GDSC_OFF);
>          if (ret)
>                  return ret;
> 
> 
> So with that change, we would then not need the ALWAYS_ON flag set for usb gdsc,
> instead we would update the .pwrsts to PWRSTS_RET_ON instead of PWRSTS_OFF_ON,
> and that should make both usb wake-ups to work and we can still have the usb_gdsc as
> a subdomain of CX for performance state voting.

To clarify, usb_gdsc is not setup as a subdomain of CX so far, right?
Just that eventually we'll make usb_gdsc a subdomain of CX so that
dev_pm_opp_set_rate() can target the CX domain instead of the usb one?

> Does that sounds like a reasonable solution?

It sounds good to me. What about the existing users of PWRSTS_RET
though? If I understand correctly this flag means the domain will never
be turned off, instead it will hit retention during low power modes.

While you're crafting this patch can you also document the PWRSTS_*
defines so that we know what they mean? I can guess that PWRSTS_RET
means "retention" but I don't know what it really means. I guess it
means "Deepest power off state is retention of memory cells".