Re: [PATCH v2 2/2] clk: qcom: gpucc: Add support for GX GDSC for SC7180

[Taniya Das]

Hi Doug,

On 2/10/2020 11:19 PM, Doug Anderson wrote:
> Hi,
>
> On Sun, Feb 9, 2020 at 8:01 PM Taniya Das <tdas@xxxxxxxxxxxxxx> wrote:
> > Most of the time the CPU should not be touching the GX domain on the
> > GPU except for a very special use case when the CPU needs to force the
> > GX headswitch off. Add the GX domain for that use case.  As part of
> > this add a dummy enable function for the GX gdsc to simulate success
> > so that the pm_runtime reference counting is correct.  This matches
> > what was done in sdm845 in commit 85a3d920d30a ("clk: qcom: Add a
> > dummy enable function for GX gdsc").
> >
> > Signed-off-by: Taniya Das <tdas@xxxxxxxxxxxxxx>
> > Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
>
> For future reference, if you have someone's tag in your commit message
> it's nice to CC them on the email.

My bad my miss.

> > ---
> >   drivers/clk/qcom/gpucc-sc7180.c | 37 +++++++++++++++++++++++++++++++++++++
> >   1 file changed, 37 insertions(+)
> >
> > diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c
> > index a96c0b9..7b656b6 100644
> > --- a/drivers/clk/qcom/gpucc-sc7180.c
> > +++ b/drivers/clk/qcom/gpucc-sc7180.c
> > @@ -170,8 +170,45 @@ static struct gdsc cx_gdsc = {
> >          .flags = VOTABLE,
> >   };
> >
> > +/*
> > + * On SC7180 the GPU GX domain is *almost* entirely controlled by the GMU
> > + * running in the CX domain so the CPU doesn't need to know anything about the
> > + * GX domain EXCEPT....
> > + *
> > + * Hardware constraints dictate that the GX be powered down before the CX. If
> > + * the GMU crashes it could leave the GX on. In order to successfully bring back
> > + * the device the CPU needs to disable the GX headswitch. There being no sane
> > + * way to reach in and touch that register from deep inside the GPU driver we
> > + * need to set up the infrastructure to be able to ensure that the GPU can
> > + * ensure that the GX is off during this super special case. We do this by
> > + * defining a GX gdsc with a dummy enable function and a "default" disable
> > + * function.
> > + *
> > + * This allows us to attach with genpd_dev_pm_attach_by_name() in the GPU
> > + * driver. During power up, nothing will happen from the CPU (and the GMU will
> > + * power up normally but during power down this will ensure that the GX domain
> > + * is *really* off - this gives us a semi standard way of doing what we need.
> > + */
> > +static int gx_gdsc_enable(struct generic_pm_domain *domain)
> > +{
> > +       /* Do nothing but give genpd the impression that we were successful */
> > +       return 0;
> > +}
> > +
> > +static struct gdsc gx_gdsc = {
> > +       .gdscr = 0x100c,
> > +       .clamp_io_ctrl = 0x1508,
> > +       .pd = {
> > +               .name = "gx_gdsc",
> > +               .power_on = gx_gdsc_enable,
> > +       },
> > +       .pwrsts = PWRSTS_OFF_ON,
> > +       .flags = CLAMP_IO,
>
> In my previous reply [1], I asked about these flags and if it was
> intentional that they were different from sdm845.  I did see a private
> response, but no public one.  In the future note that it's good to
> reply publicly so everyone understands what happened.  In this case, I
> was told "the GDSC's on 845 and SC7180 are different and hence the
> change in flags is expected".  That answers my question and thus I'm
> fine with my tag being here.  It also looks like you took my other
> review feedback on v1, which is nice.
>
>
> -Doug

I am unable to respond to the other thread, thus we put out the reply.

> [1] https://lore.kernel.org/r/CAD=FV=V6yM7UJwu0ZLPCqmDgV9FS4=g+wcLg0TV51b72zvWT9Q@xxxxxxxxxxxxxx

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--