Re: [PATCH v1] clk: qcom: Add MSM8916 Global Clock Controller support

[Stephen Boyd <sboyd@codeaurora.org>]

On 02/06/15 10:58, Georgi Djakov wrote:
> [...]
> +
> +static const struct of_device_id gcc_msm8916_match_table[] = {
> +	{ .compatible = "qcom,gcc-msm8916" },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, gcc_msm8916_match_table);

Nitpick: Please remove newline between the MODULE_DEVICE_TABLE and match
table.

> +
> +static int gcc_msm8916_probe(struct platform_device *pdev)
> +{
> +	struct clk *clk;
> +	struct device *dev = &pdev->dev;
> +	struct regmap *regmap;
> +	u32 val;
> +
> +	/* Temporary until RPM clocks supported */
> +	clk = clk_register_fixed_rate(dev, "xo", NULL, CLK_IS_ROOT, 19200000);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	clk = clk_register_fixed_rate(dev, "sleep_clk_src", NULL,
> +				      CLK_IS_ROOT, 32768);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	regmap = qcom_cc_map(pdev, &gcc_msm8916_desc);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	/* 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.

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.

> +
> +	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.

> +		.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);
>
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project