[v4,0/2] Add Qualcomm MSM8939 GCC binding and driver

Message

Bryan O'Donoghue May 12, 2020, 11:50 a.m. UTC

V4:
- Moves headers from 1/1 to 0/1 - patch squashing error - Rob
- Identifies licensing as GPL v2.0-only, thanks for pointing this out. - Rob
- Adds Tested-by: Vincent Knecht <[e-mail]>, thanks for
  testing this. - Vincent
- https://github.com/bryanodonoghue/linux/pull/new/clk-next+msm8939-v2.1
- https://github.com/bryanodonoghue/linux/pull/new/clk-next+msm8939-v4

V3:
This update removes the old clock name arrays which I forgot to prune in
the previous V2.

git diff bod/clk-next+msm8939 bod/clk-next+msm8939-v2.1

V2:
This update does the following

1. Drops code in the probe routine to add xo and sleep_clk. Instead
   the DTS for the GCC will need to declare both of those clocks for the
   GCC controller.

2. Supplants parent_names for parent_data for all clocks.

3. Squashes down the previous three patches into two.

4. Drops the git log of copying files. The git log makes clear the silicon
   is highly similar, so, you can just as easily read the log and do a
   diff.

5. Doesn't update the MSM8916 with parent_data.
   Happy to do this at a later date but, don't have the time to validate
   this properly at the moment. This set focuses on the MSM8939 alone.

6. Dropped comment and boilerplate license text as indicated.

7. Dropped dependency on COMMON_CLK_QCOM seems to not be needed.

8. Easily view the changes here:
   git add bod https://github.com/bryanodonoghue/linux.git
   git fetch bod
   git diff bod/clk-next+msm8939 bod/clk-next+msm8939-v2   

V1:
These three patches add support for the MSM8939 Global Clock Controller.
The MSM8939 is a derivation of the MSM8916 sharing the large majority of
its clock settings with MSM8916, however, there are enough changes, in some
cases mutually incompatible changes that necessitate a separate driver.

I thought it was both important and useful to show in the git log the
differences between MSM8916 and MSM8939 so, one patch copies the MSM8916
driver while another patch applies the entire gamut of MSM8939 changes,
squashing down from a git log of approximately 31 separate commits.

For reference that log is here:
https://github.com/bryanodonoghue/linux/pull/new/msm8939-clk-next-reference-log

Generally speaking MSM8939 differes from MSM8916 in two key ways.

- New and higher clock frequencies for existing IP blocks.
- New PLLs to drive those higher frequencies

Bryan O'Donoghue (2):
  clk: qcom: Add DT bindings for MSM8939 GCC
  clk: qcom: gcc-msm8939: Add MSM8939 Generic Clock Controller

 .../devicetree/bindings/clock/qcom,gcc.yaml   |    3 +
 drivers/clk/qcom/Kconfig                      |    8 +
 drivers/clk/qcom/Makefile                     |    1 +
 drivers/clk/qcom/gcc-msm8939.c                | 3999 +++++++++++++++++
 include/dt-bindings/clock/qcom,gcc-msm8939.h  |  206 +
 include/dt-bindings/reset/qcom,gcc-msm8939.h  |  110 +
 6 files changed, 4327 insertions(+)
 create mode 100644 drivers/clk/qcom/gcc-msm8939.c
 create mode 100644 include/dt-bindings/clock/qcom,gcc-msm8939.h
 create mode 100644 include/dt-bindings/reset/qcom,gcc-msm8939.h

Comments

Stephen Boyd May 14, 2020, 9:31 p.m. UTC | #1

Quoting Bryan O'Donoghue (2020-05-12 04:50:23)
> This patch adds support for the MSM8939 GCC. The MSM8939 is based on the
> MSM8916. MSM8939 is compatible in several ways with MSM8916 but, has
> additional functional blocks added which require additional PLL sources. In
> some cases functional blocks from the MSM8916 have different clock sources
> or different supported frequencies.
> 
> Cc: Andy Gross <[e-mail]>
> Cc: Bjorn Andersson <[e-mail]>
> Cc: Michael Turquette <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Philipp Zabel <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Shawn Guo <[e-mail]>

Is this a co-developed-by tag?

> Tested-by: Vincent Knecht <[e-mail]>
> Signed-off-by: Bryan O'Donoghue <[e-mail]>
> ---
> diff --git a/drivers/clk/qcom/gcc-msm8939.c b/drivers/clk/qcom/gcc-msm8939.c
> new file mode 100644
> index 000000000000..71081eb23081
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-msm8939.c
> @@ -0,0 +1,3999 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 Linaro Limited
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> +
> +#include <dt-bindings/clock/qcom,gcc-msm8939.h>
> +#include <dt-bindings/reset/qcom,gcc-msm8939.h>
> +
> +#include "common.h"
> +#include "clk-regmap.h"
> +#include "clk-pll.h"
> +#include "clk-rcg.h"
> +#include "clk-branch.h"
> +#include "reset.h"
> +#include "gdsc.h"
> +
> +enum {
> +       P_XO,
> +       P_GPLL0,
> +       P_GPLL0_AUX,
> +       P_BIMC,
> +       P_GPLL1,
> +       P_GPLL1_AUX,
> +       P_GPLL2,
> +       P_GPLL2_AUX,
> +       P_GPLL3,
> +       P_GPLL3_AUX,
> +       P_GPLL4,
> +       P_GPLL5,
> +       P_GPLL5_AUX,
> +       P_GPLL5_EARLY,
> +       P_GPLL6,
> +       P_GPLL6_AUX,
> +       P_SLEEP_CLK,
> +       P_DSI0_PHYPLL_BYTE,
> +       P_DSI0_PHYPLL_DSI,
> +       P_EXT_PRI_I2S,
> +       P_EXT_SEC_I2S,
> +       P_EXT_MCLK,
> +};
> +
> +static struct clk_pll gpll0 = {
> +       .l_reg = 0x21004,
> +       .m_reg = 0x21008,
> +       .n_reg = 0x2100c,
> +       .config_reg = 0x21010,
> +       .mode_reg = 0x21000,
> +       .status_reg = 0x2101c,
> +       .status_bit = 17,
> +       .clkr.hw.init = &(struct clk_init_data){
> +               .name = "gpll0",
> +               .parent_data = &(const struct clk_parent_data) {
> +                       .fw_name = "xo",
> +                       .name = "xo",

Please only have .fw_name. The .name field is a fallback, but given that
this is new code there shouldn't be a need for a fallback to the old way
of doing things.

> +               },
> +               .num_parents = 1,
> +               .ops = &clk_pll_ops,
> +       },
> +};
> +
> +static struct clk_regmap gpll0_vote = {
> +       .enable_reg = 0x45000,
> +       .enable_mask = BIT(0),
> +       .hw.init = &(struct clk_init_data){
> +               .name = "gpll0_vote",
> +               .parent_data = &(const struct clk_parent_data) {
> +                       .hw = &gpll0.clkr.hw,
> +               },
> +               .num_parents = 1,
> +               .ops = &clk_pll_vote_ops,
> +       },
> +};
> +
> +static struct clk_pll gpll1 = {
> +       .l_reg = 0x20004,
> +       .m_reg = 0x20008,
> +       .n_reg = 0x2000c,
> +       .config_reg = 0x20010,
> +       .mode_reg = 0x20000,
> +       .status_reg = 0x2001c,
> +       .status_bit = 17,
> +       .clkr.hw.init = &(struct clk_init_data){
> +               .name = "gpll1",
> +               .parent_data = &(const struct clk_parent_data) {
> +                       .fw_name = "xo",
> +                       .name = "xo",
> +               },
> +               .num_parents = 1,
> +               .ops = &clk_pll_ops,
> +       },
> +};
> +
> +static struct clk_regmap gpll1_vote = {
> +       .enable_reg = 0x45000,
> +       .enable_mask = BIT(1),
> +       .hw.init = &(struct clk_init_data){
> +               .name = "gpll1_vote",
> +               .parent_data = &(const struct clk_parent_data) {
> +                       .hw = &gpll1.clkr.hw,
> +               },
> +               .num_parents = 1,
> +               .ops = &clk_pll_vote_ops,
> +       },
> +};
> +
> +static struct clk_pll gpll2 = {
> +       .l_reg = 0x4a004,
> +       .m_reg = 0x4a008,
> +       .n_reg = 0x4a00c,
> +       .config_reg = 0x4a010,
> +       .mode_reg = 0x4a000,
> +       .status_reg = 0x4a01c,
> +       .status_bit = 17,
> +       .clkr.hw.init = &(struct clk_init_data){
> +               .name = "gpll2",
[...]
> +
> +static int gcc_msm8939_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       struct regmap *regmap;
> +
> +       ret = qcom_cc_probe(pdev, &gcc_msm8939_desc);
> +       if (ret)
> +               return ret;
> +
> +       regmap = dev_get_regmap(&pdev->dev, NULL);
> +       clk_pll_configure_sr_hpm_lp(&gpll3, regmap, &gpll3_config, true);
> +       clk_pll_configure_sr_hpm_lp(&gpll4, regmap, &gpll4_config, true);

We should probably configure these before registering the clks. Can you
do the usual, map registers, configure stuff, and then
qcom_cc_really_probe()?

> +
> +       return 0;

And then this can fail so return an error in case it does.

> +}
> +
> +static struct platform_driver gcc_msm8939_driver = {
> +       .probe          = gcc_msm8939_probe,
> +       .driver         = {
> +               .name   = "gcc-msm8939",
> +               .of_match_table = gcc_msm8939_match_table,
> +       },
> +};
> +
> +static int __init gcc_msm8939_init(void)
> +{
> +       return platform_driver_register(&gcc_msm8939_driver);
> +}
> +core_initcall(gcc_msm8939_init);
> +
> +static void __exit gcc_msm8939_exit(void)
> +{
> +       platform_driver_unregister(&gcc_msm8939_driver);
> +}
> +module_exit(gcc_msm8939_exit);
> +
> +MODULE_DESCRIPTION("Qualcomm GCC MSM8939 Driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:gcc-msm8939");

The module alias isn't needed right?

Bryan O'Donoghue May 14, 2020, 9:42 p.m. UTC | #2

On 14/05/2020 22:31, Stephen Boyd wrote:
> Quoting Bryan O'Donoghue (2020-05-12 04:50:23)
>> This patch adds support for the MSM8939 GCC. The MSM8939 is based on the
>> MSM8916. MSM8939 is compatible in several ways with MSM8916 but, has
>> additional functional blocks added which require additional PLL sources. In
>> some cases functional blocks from the MSM8916 have different clock sources
>> or different supported frequencies.
>>
>> Cc: Andy Gross <[e-mail]>
>> Cc: Bjorn Andersson <[e-mail]>
>> Cc: Michael Turquette <[e-mail]>
>> Cc: Stephen Boyd <[e-mail]>
>> Cc: Rob Herring <[e-mail]>
>> Cc: Philipp Zabel <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Signed-off-by: Shawn Guo <[e-mail]>
> 
> Is this a co-developed-by tag?

Yep. I'm squashing down about 30-some internal patches to this one patch 
here including one or two from Shawn in this set.

I wasn't quite sure what the etiquette on Co-developed was i.e. it 
wasn't something git allowed me to specify with a "git commit -s 
--co-developed="xyz"" so I just retained the SOB.

Looking through git logs I see an example

I'll apply a
Co-developed-by: Shawn Guo <[e-mail]>

for v5.

>> +static int gcc_msm8939_probe(struct platform_device *pdev)
>> +{
>> +       int ret;
>> +       struct regmap *regmap;
>> +
>> +       ret = qcom_cc_probe(pdev, &gcc_msm8939_desc);
>> +       if (ret)
>> +               return ret;
>> +
>> +       regmap = dev_get_regmap(&pdev->dev, NULL);
>> +       clk_pll_configure_sr_hpm_lp(&gpll3, regmap, &gpll3_config, true);
>> +       clk_pll_configure_sr_hpm_lp(&gpll4, regmap, &gpll4_config, true);
> 
> We should probably configure these before registering the clks. Can you
> do the usual, map registers, configure stuff, and then
> qcom_cc_really_probe()?

I think so. If there was a good reason to configure the plls after the 
registration, I can't recall what that was, maybe the original flow from 
downstream ...

>> +
>> +MODULE_DESCRIPTION("Qualcomm GCC MSM8939 Driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:gcc-msm8939");
> 
> The module alias isn't needed right?

Nope g/msm8916/s//msm8939/g - I can zap that.

Thanks for the review.

---
bod