Messages in this thread | | | Date | Tue, 24 Feb 2015 17:48:54 +0200 | From | Georgi Djakov <> | Subject | Re: [PATCH v1] clk: qcom: Add MSM8916 Global Clock Controller support |
| |
On 02/24/2015 12:46 AM, Stephen Boyd wrote: > On 02/06/15 10:58, Georgi Djakov wrote: >> [...] [..] >> + >> + /* Vote for GPLL0 to turn on */ >> + regmap_read(regmap, 0x45000, &val); >> + val |= BIT(0); >> + regmap_write(regmap, 0x45000, val); > > Hm.. I guess this is for the CPU to stay on, otherwise GPLL0 might turn > off? But if we expose this register and the bit as gpll0_vote then we're > going to turn it off once the last user turns off GPLL0. So I'm not sure > how to handle this, but it certainly seems like we can just remove this > bit of code and not be any worse off than we are right now. We need to > figure out some way to make this work though. >
Ok, agree.
> Here's the problem. GPLL0 is used by the CPU running this code. It's > also used by random other clocks in the system. If the code for the CPU > clock is not compiled in (i.e. clock-a53.c or whatever we call it), then > it is very possible that we'll disable the last clock that's using the > PLL according to what software knows, that will in turn disable the PLL > and then the CPU will die. > > Of course, it's very possible that we'll never actually turn GPLL0 off > because it's used for quite a few clocks in the system. So the chances > of running into this problem are almost entirely theoretical.
Yes, I am investigating this scenario. Thanks for your comment.
> >> + >> + return qcom_cc_really_probe(pdev, &gcc_msm8916_desc, regmap); >> +} >> + >> +static int gcc_msm8916_remove(struct platform_device *pdev) >> +{ >> + qcom_cc_remove(pdev); >> + return 0; >> +} >> + >> +static struct platform_driver gcc_msm8916_driver = { >> + .probe = gcc_msm8916_probe, >> + .remove = gcc_msm8916_remove, >> + .driver = { >> + .name = "gcc-msm8916", > > We need owner = THIS_MODULE here because the platform_driver_module > macro isn't being used. >
But below i use the platform_driver_register macro and therefore it seems not needed.
>> + .of_match_table = gcc_msm8916_match_table, >> + }, >> +}; >> + >> +static int __init gcc_msm8916_init(void) >> +{ >> + return platform_driver_register(&gcc_msm8916_driver); >> +} >> +core_initcall(gcc_msm8916_init); >> + >> +static void __exit gcc_msm8916_exit(void) >> +{ >> + platform_driver_unregister(&gcc_msm8916_driver); >> +} >> +module_exit(gcc_msm8916_exit); >>
Thank you for taking a look at this preliminary stuff. I'll send in the next few days updated and tested version.
BR, Georgi
| |