| Message ID | 20210220155618.176559-5-konrad.dybcio@somainline.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] clk: qcom: gcc-sdm660: Fix hmss_gpll0_clk_src parent_map | expand |
Quoting Konrad Dybcio (2021-02-20 07:56:16) > Downstream kernel executes a bunch of commands, such as keeping > GPU/MMSS interface clocks alive to make sure all subsystems can > work properly. Add these to make sure they do. > > Fixes: f2a76a2955c0 ("clk: qcom: Add Global Clock controller (GCC) driver for SDM660") > Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > --- > drivers/clk/qcom/gcc-sdm660.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/gcc-sdm660.c b/drivers/clk/qcom/gcc-sdm660.c > index bc8dfcd6d629..db2185c88b77 100644 > --- a/drivers/clk/qcom/gcc-sdm660.c > +++ b/drivers/clk/qcom/gcc-sdm660.c > @@ -11,6 +11,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > +#include <linux/clk.h> > #include <linux/clk-provider.h> > #include <linux/regmap.h> > #include <linux/reset-controller.h> > @@ -2622,7 +2623,27 @@ static int gcc_sdm660_probe(struct platform_device *pdev) > if (ret) > return ret; > > - return qcom_cc_really_probe(pdev, &gcc_sdm660_desc, regmap); > + ret = qcom_cc_really_probe(pdev, &gcc_sdm660_desc, regmap); > + if (ret) > + return ret; > + > + /* Disable the GPLL0 active input to MMSS and GPU via MISC registers */ > + regmap_update_bits(regmap, 0x0902c, 0x3, 0x3); > + regmap_update_bits(regmap, 0x71028, 0x3, 0x3); > + > + /* This clock is used for all MMSSCC register access */ > + clk_prepare_enable(gcc_mmss_noc_cfg_ahb_clk.clkr.hw.clk); > + > + /* This clock is used for all GPUCC register access */ > + clk_prepare_enable(gcc_gpu_cfg_ahb_clk.clkr.hw.clk); > + > + /* Keep bimc gfx clock port on all the time */ > + clk_prepare_enable(gcc_bimc_gfx_clk.clkr.hw.clk); > + Preferably just set these various bits with regmap_update_bits() during probe. Also, please do it before regsitering the clks, not after. > + /* Set the HMSS_GPLL0_SRC for 300MHz to CPU subsystem */ > + clk_set_rate(hmss_gpll0_clk_src.clkr.hw.clk, 300000000); Is this not already the case? > + > + return ret; > } >
Hi and sorry for the late reply, >> + >> + /* Keep bimc gfx clock port on all the time */ >> + clk_prepare_enable(gcc_bimc_gfx_clk.clkr.hw.clk); >> + > Preferably just set these various bits with regmap_update_bits() during > probe. Also, please do it before regsitering the clks, not after. To be fair, now I think that simply adding CLK_IS_CRITICAL flag to the clocks in question is the smartest thing to do. Magic writes don't tell a whole lot. >> + /* Set the HMSS_GPLL0_SRC for 300MHz to CPU subsystem */ >> + clk_set_rate(hmss_gpll0_clk_src.clkr.hw.clk, 300000000); > Is this not already the case? This is a mission-critical clock and we cannot trust the bootloader with setting it. Otherwise dragons might appear. Konrad
Quoting Konrad Dybcio (2021-02-25 11:09:14) > Hi and sorry for the late reply, > I'm sorry too. This fell off my review queue for some time. > > >> + > >> + /* Keep bimc gfx clock port on all the time */ > >> + clk_prepare_enable(gcc_bimc_gfx_clk.clkr.hw.clk); > >> + > > Preferably just set these various bits with regmap_update_bits() during > > probe. Also, please do it before regsitering the clks, not after. > > To be fair, now I think that simply adding CLK_IS_CRITICAL flag to the clocks in question is the smartest thing to do. Magic writes don't tell a whole lot. This is how it's been done in various other qcom clk drivers. Usually there is a comment about what is enabled, but really it's just setting random bits that sadly aren't already set by default. > > > >> + /* Set the HMSS_GPLL0_SRC for 300MHz to CPU subsystem */ > >> + clk_set_rate(hmss_gpll0_clk_src.clkr.hw.clk, 300000000); > > Is this not already the case? > > > This is a mission-critical clock and we cannot trust the bootloader with setting it. Otherwise dragons might appear. > What does the bootloader set it to?
>>>> + >>>> + /* Keep bimc gfx clock port on all the time */ >>>> + clk_prepare_enable(gcc_bimc_gfx_clk.clkr.hw.clk); >>>> + >>> Preferably just set these various bits with regmap_update_bits() during >>> probe. Also, please do it before regsitering the clks, not after. >> To be fair, now I think that simply adding CLK_IS_CRITICAL flag to the clocks in question is the smartest thing to do. Magic writes don't tell a whole lot. > This is how it's been done in various other qcom clk drivers. Usually > there is a comment about what is enabled, but really it's just setting > random bits that sadly aren't already set by default. But why.. why should we give Linux less information about the hardware it's running on? It's a kernel after all, I know some parties would prefer to keep the hardware away from its users, but cmon, it's OSSLand out there! Allocating a few bytes more in memory is proooobably a good trade-off for keeping an eye on the state of various clocks instead of simply setting some seemingly random bits and hoping nothing bad happens under the hood.. This isn't only a case for this clock, but for all ones that are effectively pinky-promise-trusted to function properly with no way of checking if they're even still alive other than poking the registers manually.. As of v5.12-rc2, there are *46* such trust credits.. It's NOT easy to track down issues in big monolithic kernels, especially when people submitting changes to common code seem to think that only their hardware uses it (no hard feelings, but drm/msm broke on !a6xx *at least* two times within the last year) and since making sure the clocks are ticking properly is one of the most crucial things when looking into more complex issues, which may or may not randomly happen on a platform that is just being brought up for various reasons (e.g. half of mdm9607 hardware doesn't wanna play along without a ICC driver), I *really* think we should use CLK_IS_CRITICAL instead and give developers a way to check if everything's in tact with the clock, while still keeping the "never turn it off, don't touch it!" aspect. >>>> + /* Set the HMSS_GPLL0_SRC for 300MHz to CPU subsystem */ >>>> + clk_set_rate(hmss_gpll0_clk_src.clkr.hw.clk, 300000000); >>> Is this not already the case? >> >> This is a mission-critical clock and we cannot trust the bootloader with setting it. Otherwise dragons might appear. >> > What does the bootloader set it to? Sorry but I can't check, nor do I remember right now. But we still shouldn't add a variable that might come from a lazy OEM not incorporating the fix into their release, especially since this one is used for clocking the AP. Konrad
Hi, I am aware that the comments I left are highly controversial and a bit emotional, but I'd still like to get a response. It's been almost two months and I have phones on the desk waiting for things to be merged, so that I can develop more things. P.S. I still stand by my stance that the more info we can get about what's going on in the black boxes that we are not allowed to get docs for, the better for us, developers. Konrad
Quoting Konrad Dybcio (2021-05-26 12:11:13) > Hi, > > > I am aware that the comments I left are highly controversial and a bit emotional, but I'd still like to get a response. It's been almost two months and I have phones on the desk waiting for things to be merged, so that I can develop more things. > > > P.S. I still stand by my stance that the more info we can get about what's going on in the black boxes that we are not allowed to get docs for, the better for us, developers. > I marked this patch as "changes requested". Please make the changes requested.
diff --git a/drivers/clk/qcom/gcc-sdm660.c b/drivers/clk/qcom/gcc-sdm660.c index bc8dfcd6d629..db2185c88b77 100644 --- a/drivers/clk/qcom/gcc-sdm660.c +++ b/drivers/clk/qcom/gcc-sdm660.c @@ -11,6 +11,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/clk.h> #include <linux/clk-provider.h> #include <linux/regmap.h> #include <linux/reset-controller.h> @@ -2622,7 +2623,27 @@ static int gcc_sdm660_probe(struct platform_device *pdev) if (ret) return ret; - return qcom_cc_really_probe(pdev, &gcc_sdm660_desc, regmap); + ret = qcom_cc_really_probe(pdev, &gcc_sdm660_desc, regmap); + if (ret) + return ret; + + /* Disable the GPLL0 active input to MMSS and GPU via MISC registers */ + regmap_update_bits(regmap, 0x0902c, 0x3, 0x3); + regmap_update_bits(regmap, 0x71028, 0x3, 0x3); + + /* This clock is used for all MMSSCC register access */ + clk_prepare_enable(gcc_mmss_noc_cfg_ahb_clk.clkr.hw.clk); + + /* This clock is used for all GPUCC register access */ + clk_prepare_enable(gcc_gpu_cfg_ahb_clk.clkr.hw.clk); + + /* Keep bimc gfx clock port on all the time */ + clk_prepare_enable(gcc_bimc_gfx_clk.clkr.hw.clk); + + /* Set the HMSS_GPLL0_SRC for 300MHz to CPU subsystem */ + clk_set_rate(hmss_gpll0_clk_src.clkr.hw.clk, 300000000); + + return ret; } static struct platform_driver gcc_sdm660_driver = {
Downstream kernel executes a bunch of commands, such as keeping GPU/MMSS interface clocks alive to make sure all subsystems can work properly. Add these to make sure they do. Fixes: f2a76a2955c0 ("clk: qcom: Add Global Clock controller (GCC) driver for SDM660") Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> --- drivers/clk/qcom/gcc-sdm660.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)