| Message ID | 20220831104741.v6.4.I5e64ff4b77bb9079eb2edeea8a02585c9e76778f@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clk/qcom: Support gdsc collapse polling using 'reset' interface | expand |
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 01/09/2022 13:34, Philipp Zabel wrote: > 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. While this will work for the sc7280, the platform that Akhil was developing, this will not work for other platforms (like sm8250), where the dispcc also provides traditional BCR resets.
On 9/1/2022 4:16 PM, Dmitry Baryshkov wrote: > On 01/09/2022 13:34, Philipp Zabel wrote: >> 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. > > While this will work for the sc7280, the platform that Akhil was > developing, this will not work for other platforms (like sm8250), > where the dispcc also provides traditional BCR resets. > Like Dimtry mentioned, we should eventually implement this feature on all gpucc drivers and some of them already use the existing reset ops. The current implementation creates the least code churn and duplication's. -Akhil
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, +}; + +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), }; static const struct of_device_id gpu_cc_sc7280_match_table[] = {