mbox series

[v10,0/6] Add support for Qualcomm A53 CPU clock

Message ID 20171201170224.25053-1-georgi.djakov@linaro.org
Headers show
Series Add support for Qualcomm A53 CPU clock | expand

Message

Georgi Djakov Dec. 1, 2017, 5:02 p.m. UTC 
This patchset adds support for the A53 CPU clock on MSM8916 platforms
and allows scaling of the CPU frequency on msm8916 based platforms.

Changes since v9 (https://lkml.org/lkml/2017/9/21/511)
* Added the clock properties to the APCS DT node, instead of adding a subnode
and also replaced patch "mailbox: qcom: Populate APCS child platform devices"
with "mailbox: qcom: Create APCS child device for clock controller".
* Dropped patch "mailbox: qcom: Move the apcs struct into a separate header",
and use dev_get_regmap(dev->parent) in the child driver.
* Addressed Bjorn's comments on a53-pll and apcs-clk drivers.
* Added SPDX copyright identifiers.

Changes since v8 (https://lkml.org/lkml/2017/6/23/476)
 * Converted APCS mailbox driver to use regmap and to populate child
 platform devices that will handle the rest of the functionality
 provided by APCS block.
 * Picked Rob's Ack for the PLL binding.
 * Changed the APCS binding and put it into a separate patch.
 * Addressed review comments.
 * Minor changes.

Changes since v7 (https://lkml.org/lkml/2016/10/31/296)
 * Add the APCS clock controller to the APCS driver to expose both the
 mailbox and clock controller functionality as discussed earlier:
 https://lkml.org/lkml/2016/11/14/860
 * Changed the a53pll compatible string as suggested by Rob.

Changes since v6 (https://lkml.org/lkml/2016/9/7/347)
 * Addressed various comments from Stephen Boyd

Changes since v5 (https://lkml.org/lkml/2016/2/1/407)
 * Rebase to clk-next and update according to the recent API changes.

Changes since v4 (https://lkml.org/lkml/2015/12/14/367)
 * Convert to builtin drivers as now __clk_lookup() is used

Changes since v3 (https://lkml.org/lkml/2015/8/12/585)
 * Split driver into two parts - and separate A53 PLL and
   A53 clock controller drivers.
 * Drop the safe switch hook patch. Add a clock notifier in
   the clock provider to handle switching via safe mux and
   divider configuration.

Changes since v2 (https://lkml.org/lkml/2015/7/24/526)
 * Drop gpll0_vote patch.
 * Switch to the new clk_hw_* APIs.
 * Rebase to the current clk-next.

Changes since v1 (https://lkml.org/lkml/2015/6/12/193)
 * Drop SR2 PLL patch, as it is already applied.
 * Add gpll0_vote rate propagation patch.
 * Update/rebase patches to the current clk-next.


Georgi Djakov (6):
  mailbox: qcom: Convert APCS IPC driver to use regmap
  mailbox: qcom: Create APCS child device for clock controller
  clk: qcom: Add A53 PLL support
  clk: qcom: Add regmap mux-div clocks support
  dt-bindings: mailbox: qcom: Document the APCS clock binding
  clk: qcom: Add APCS clock controller support

 .../devicetree/bindings/clock/qcom,a53pll.txt      |  22 ++
 .../bindings/mailbox/qcom,apcs-kpss-global.txt     |  18 ++
 drivers/clk/qcom/Kconfig                           |  21 ++
 drivers/clk/qcom/Makefile                          |   3 +
 drivers/clk/qcom/a53-pll.c                         | 109 ++++++++++
 drivers/clk/qcom/apcs-msm8916.c                    | 149 +++++++++++++
 drivers/clk/qcom/clk-regmap-mux-div.c              | 230 +++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-mux-div.h              |  47 +++++
 drivers/mailbox/qcom-apcs-ipc-mailbox.c            |  35 +++-
 9 files changed, 629 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53pll.txt
 create mode 100644 drivers/clk/qcom/a53-pll.c
 create mode 100644 drivers/clk/qcom/apcs-msm8916.c
 create mode 100644 drivers/clk/qcom/clk-regmap-mux-div.c
 create mode 100644 drivers/clk/qcom/clk-regmap-mux-div.h

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bjorn Andersson Dec. 5, 2017, 6:01 a.m. UTC | #1 
On Fri 01 Dec 09:02 PST 2017, Georgi Djakov wrote:
[..]
> diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
> new file mode 100644
> index 000000000000..f71039ff2347
> --- /dev/null
> +++ b/drivers/clk/qcom/apcs-msm8916.c
> @@ -0,0 +1,149 @@
> +/*
> + * Qualcomm APCS clock controller driver
> + *
> + * Copyright (c) 2017, Linaro Limited
> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
> + *
> + * SPDX-License-Identifier: GPL-2.0

The SPDX-License-Identifier should be on the first line in the file,
commented by //

> + */
> +
[..]
> +static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = pdev->dev.parent;

Call this "parent" instead.

> +	struct device_node *np = dev->of_node;
> +	struct clk_regmap_mux_div *a53cc;
> +	struct regmap *regmap;
> +	struct clk_init_data init = { };
> +	int ret;
> +
> +	regmap = dev_get_regmap(dev, NULL);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		dev_err(dev, "failed to get regmap: %d\n", ret);

dev_* prints should be on &pdev->dev and not on parent device.

> +		return ret;
> +	}
> +
> +	a53cc = devm_kzalloc(dev, sizeof(*a53cc), GFP_KERNEL);

Perform this allocation on behalf of this device (i.e. &pdev->dev and
not parent)

> +	if (!a53cc)
> +		return -ENOMEM;
> +
> +	init.name = "a53mux";
> +	init.parent_names = gpll0_a53cc;
> +	init.num_parents = ARRAY_SIZE(gpll0_a53cc);
> +	init.ops = &clk_regmap_mux_div_ops;
> +	init.flags = CLK_SET_RATE_PARENT;
> +
> +	a53cc->clkr.hw.init = &init;
> +	a53cc->clkr.regmap = regmap;
> +	a53cc->reg_offset = 0x50;
> +	a53cc->hid_width = 5;
> +	a53cc->hid_shift = 0;
> +	a53cc->src_width = 3;
> +	a53cc->src_shift = 8;
> +	a53cc->parent_map = gpll0_a53cc_map;
> +
> +	a53cc->pclk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(a53cc->pclk)) {
> +		ret = PTR_ERR(a53cc->pclk);
> +		dev_err(dev, "failed to get clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	a53cc->clk_nb.notifier_call = a53cc_notifier_cb;
> +	ret = clk_notifier_register(a53cc->pclk, &a53cc->clk_nb);
> +	if (ret) {
> +		dev_err(dev, "failed to register clock notifier: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_clk_register_regmap(dev, &a53cc->clkr);

This you can do on the &pdev->dev, it won't find a regmap on this node
and will try the parent.

> +	if (ret) {
> +		dev_err(dev, "failed to register regmap clock: %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get,

Be explicit here and do parent->of_node.

> +				     &a53cc->clkr.hw);
> +	if (ret) {
> +		dev_err(dev, "failed to add clock provider: %d\n", ret);
> +		goto err;
> +	}
> +
> +	platform_set_drvdata(pdev, a53cc);
> +
> +	return 0;
> +
> +err:
> +	clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb);
> +	return ret;
> +}
> +
> +static int qcom_apcs_msm8916_clk_remove(struct platform_device *pdev)
> +{
> +	struct clk_regmap_mux_div *a53cc = platform_get_drvdata(pdev);
> +
> +	clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb);
> +	of_clk_del_provider(pdev->dev.of_node);

You registered the provider on pdev->dev->parent.of_node.

> +
> +	return 0;
> +}
> +

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Andersson Dec. 5, 2017, 6:01 a.m. UTC | #2 
On Fri 01 Dec 09:02 PST 2017, Georgi Djakov wrote:

> There is a clock controller functionality provided by the APCS hardware
> block of msm8916 devices. The device-tree would represent an APCS node
> with both mailbox and clock provider properties.
> Create a platform child device for the clock controller functionality so
> the driver can probe and use APCS as parent.
> 
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/mailbox/qcom-apcs-ipc-mailbox.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> index ab344bc6fa63..57bde0dfd12f 100644
> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> @@ -29,6 +29,7 @@ struct qcom_apcs_ipc {
>  
>  	struct regmap *regmap;
>  	unsigned long offset;
> +	struct platform_device *clk;
>  };
>  
>  static const struct regmap_config apcs_regmap_config = {
> @@ -96,6 +97,14 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global")) {
> +		apcs->clk = platform_device_register_data(&pdev->dev,
> +							  "qcom-apcs-msm8916-clk",
> +							  -1, NULL, 0);
> +		if (IS_ERR(apcs->clk))
> +			dev_err(&pdev->dev, "failed to register APCS clk\n");
> +	}
> +
>  	platform_set_drvdata(pdev, apcs);
>  
>  	return 0;
> @@ -104,8 +113,10 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
>  static int qcom_apcs_ipc_remove(struct platform_device *pdev)
>  {
>  	struct qcom_apcs_ipc *apcs = platform_get_drvdata(pdev);
> +	struct platform_device *clk = apcs->clk;
>  
>  	mbox_controller_unregister(&apcs->mbox);
> +	platform_device_unregister(clk);
>  
>  	return 0;
>  }
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Georgi Djakov Dec. 5, 2017, 3:33 p.m. UTC | #3 
On 12/05/2017 08:01 AM, Bjorn Andersson wrote:
> On Fri 01 Dec 09:02 PST 2017, Georgi Djakov wrote:
> [..]
>> diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
>> new file mode 100644
>> index 000000000000..f71039ff2347
>> --- /dev/null
>> +++ b/drivers/clk/qcom/apcs-msm8916.c
>> @@ -0,0 +1,149 @@
>> +/*
>> + * Qualcomm APCS clock controller driver
>> + *
>> + * Copyright (c) 2017, Linaro Limited
>> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0
> 
> The SPDX-License-Identifier should be on the first line in the file,
> commented by //
> 
>> + */
>> +
> [..]
>> +static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = pdev->dev.parent;
> 
> Call this "parent" instead.
> 
>> +	struct device_node *np = dev->of_node;
>> +	struct clk_regmap_mux_div *a53cc;
>> +	struct regmap *regmap;
>> +	struct clk_init_data init = { };
>> +	int ret;
>> +
>> +	regmap = dev_get_regmap(dev, NULL);
>> +	if (IS_ERR(regmap)) {
>> +		ret = PTR_ERR(regmap);
>> +		dev_err(dev, "failed to get regmap: %d\n", ret);
> 
> dev_* prints should be on &pdev->dev and not on parent device.
> 
>> +		return ret;
>> +	}
>> +
>> +	a53cc = devm_kzalloc(dev, sizeof(*a53cc), GFP_KERNEL);
> 
> Perform this allocation on behalf of this device (i.e. &pdev->dev and
> not parent)
> 
>> +	if (!a53cc)
>> +		return -ENOMEM;
>> +
>> +	init.name = "a53mux";
>> +	init.parent_names = gpll0_a53cc;
>> +	init.num_parents = ARRAY_SIZE(gpll0_a53cc);
>> +	init.ops = &clk_regmap_mux_div_ops;
>> +	init.flags = CLK_SET_RATE_PARENT;
>> +
>> +	a53cc->clkr.hw.init = &init;
>> +	a53cc->clkr.regmap = regmap;
>> +	a53cc->reg_offset = 0x50;
>> +	a53cc->hid_width = 5;
>> +	a53cc->hid_shift = 0;
>> +	a53cc->src_width = 3;
>> +	a53cc->src_shift = 8;
>> +	a53cc->parent_map = gpll0_a53cc_map;
>> +
>> +	a53cc->pclk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(a53cc->pclk)) {
>> +		ret = PTR_ERR(a53cc->pclk);
>> +		dev_err(dev, "failed to get clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	a53cc->clk_nb.notifier_call = a53cc_notifier_cb;
>> +	ret = clk_notifier_register(a53cc->pclk, &a53cc->clk_nb);
>> +	if (ret) {
>> +		dev_err(dev, "failed to register clock notifier: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_clk_register_regmap(dev, &a53cc->clkr);
> 
> This you can do on the &pdev->dev, it won't find a regmap on this node
> and will try the parent.
> 
>> +	if (ret) {
>> +		dev_err(dev, "failed to register regmap clock: %d\n", ret);
>> +		goto err;
>> +	}
>> +
>> +	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get,
> 
> Be explicit here and do parent->of_node.
> 
>> +				     &a53cc->clkr.hw);
>> +	if (ret) {
>> +		dev_err(dev, "failed to add clock provider: %d\n", ret);
>> +		goto err;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, a53cc);
>> +
>> +	return 0;
>> +
>> +err:
>> +	clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb);
>> +	return ret;
>> +}
>> +
>> +static int qcom_apcs_msm8916_clk_remove(struct platform_device *pdev)
>> +{
>> +	struct clk_regmap_mux_div *a53cc = platform_get_drvdata(pdev);
>> +
>> +	clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb);
>> +	of_clk_del_provider(pdev->dev.of_node);
> 
> You registered the provider on pdev->dev->parent.of_node.
> 
>> +
>> +	return 0;
>> +}
>> +

Hi Bjorn,

Thanks for reviewing! I agree with your comments and will send a new
version shortly.

BR,
Georgi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html