| Message ID | 1580886097-6312-3-git-send-email-smasetty@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sc7180: Add A618 GPU bindings | expand |
Hi, On Tue, Feb 4, 2020 at 11:02 PM Sharat Masetty <smasetty@codeaurora.org> wrote: > > From: Taniya Das <tdas@codeaurora.org> > > 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 Really weird word-wrapping? You've also indented your whole commit message? > headswitch off. Add a dummy enable function for the GX gdsc to simulate > success so that the pm_runtime reference counting is correct. Overall the commit message sounds a lot like the message in commit 85a3d920d30a ("clk: qcom: Add a dummy enable function for GX gdsc"). That's fine for the most part, but it makes it sound like you're _only_ adding the dummy enable. In this case you're adding support for the GX domain and _also_ adding a dummy enable. Maybe try: 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@codeaurora.org> Since you are re-posting Taniya's patch you need to add your own Signed-off-by as per kernel policy. > --- > 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 ec61194..3b29f19 100644 > --- a/drivers/clk/qcom/gpucc-sc7180.c > +++ b/drivers/clk/qcom/gpucc-sc7180.c > @@ -172,8 +172,45 @@ enum { > .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 = "gpu_gx_gdsc", nit: technically name could be "gx_gdsc" to match the name of the struct and #define. Your name is copied from sdm845 and matches the name of the struct and #define from there. > + .power_on = gx_gdsc_enable, > + }, > + .pwrsts = PWRSTS_OFF_ON, > + .flags = CLAMP_IO, Compared to sdm845, you have different flags. There we have: .flags = CLAMP_IO | AON_RESET | POLL_CFG_GDSCR, I'm not sure I have enough background knowledge about the hardare to figure this out. Can you confirm that you're different than sdm845 on purpose? Bonus points if you can confirm whether sdm845 is also correct as it is today or should be changed to match what you have? > +}; > + > static struct gdsc *gpu_cc_sc7180_gdscs[] = { > [CX_GDSC] = &cx_gdsc, > + [GX_GDSC] = &gx_gdsc, > }; Assuming that the question on flags is resolved and the commit message updated, feel free to add my Reviewed-by tag. -Doug
diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c index ec61194..3b29f19 100644 --- a/drivers/clk/qcom/gpucc-sc7180.c +++ b/drivers/clk/qcom/gpucc-sc7180.c @@ -172,8 +172,45 @@ enum { .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 = "gpu_gx_gdsc", + .power_on = gx_gdsc_enable, + }, + .pwrsts = PWRSTS_OFF_ON, + .flags = CLAMP_IO, +}; + static struct gdsc *gpu_cc_sc7180_gdscs[] = { [CX_GDSC] = &cx_gdsc, + [GX_GDSC] = &gx_gdsc, }; static struct clk_regmap *gpu_cc_sc7180_clocks[] = {