Re: [PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Amit Nischal]

On 2018-05-05 08:44, Stephen Boyd wrote:

Quoting Amit Nischal (2018-05-04 03:45:12)

On 2018-05-02 12:53, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-04-30 09:20:10)
>> +
>> +static struct clk_branch gcc_disp_gpll0_clk_src = {
>> +   .halt_reg = 0x52004,
>> +   .halt_check = BRANCH_HALT_DELAY,
>
> What about this one? It's not a phy so I'm confused again why we're
> unable to check the halt bit. To be clear(er), I don't see why we ever
> want to have HALT_DELAY used. Hopefully we can remove that flag.
>
> From what I recall, the flag is there for clks that don't toggle their
> status bit at all, but that we know take a few cycles to ungate the
> upstream clk. So we threw a delay into the code to make sure that when
> clk_enable() returned, a driver wouldn't try to use hardware before the
> clk was actually on. But these cases should pretty much never happen,
> hence all the pushback against this flag.
>

For these "*gpll0_clk_src" and "*gpll0_div_clk" clocks, there is no 
halt
bit to check the status and it is required to have delay for few 
cycles
so that clock gets turned on before a client driver to use the 
hardware.


Ok.. but then why is there a 'halt_reg' configured for the clk?


Thanks for the review.
I will remove the halt_reg for the clocks where we are using the 
'HALT_DELAY'
flag and there is no need to poll the status bit as we are returning 
early

from the 'clk_branch_wait()' function.




>> +
>> +static struct clk_branch gcc_ufs_card_rx_symbol_0_clk = {
>> +   .halt_reg = 0x75018,
>> +   .halt_check = BRANCH_HALT_DELAY,
>
> There are still HALT_DELAY flags for UFS though? Why?

For ufs_card tx/rx symbol clocks, we don't poll the status bit as
per the recommendation from the HW team. We can change the halt_check
type to newly implemented flag "BRANCH_HALT_SKIP". Please update us 
with

your thoughts to change the flag to "BRANCH_HALT_SKIP".


Yes use HALT_SKIP please.


Thanks for confirming. I will do the changes in the next patch series.





>
> Also, are you going to send DFS support for the QUP clks? I would like
> to see that code merged soon.

Taniya has sent the patches for DFS support for QUP clocks.
https://patchwork.kernel.org/patch/10376951/



I'll take a look.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Amit Nischal]

On 2018-05-05 08:44, Stephen Boyd wrote:

Quoting Amit Nischal (2018-05-04 03:45:12)

On 2018-05-02 12:53, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-04-30 09:20:10)
>> +
>> +static struct clk_branch gcc_disp_gpll0_clk_src = {
>> +   .halt_reg = 0x52004,
>> +   .halt_check = BRANCH_HALT_DELAY,
>
> What about this one? It's not a phy so I'm confused again why we're
> unable to check the halt bit. To be clear(er), I don't see why we ever
> want to have HALT_DELAY used. Hopefully we can remove that flag.
>
> From what I recall, the flag is there for clks that don't toggle their
> status bit at all, but that we know take a few cycles to ungate the
> upstream clk. So we threw a delay into the code to make sure that when
> clk_enable() returned, a driver wouldn't try to use hardware before the
> clk was actually on. But these cases should pretty much never happen,
> hence all the pushback against this flag.
>

For these "*gpll0_clk_src" and "*gpll0_div_clk" clocks, there is no 
halt
bit to check the status and it is required to have delay for few 
cycles
so that clock gets turned on before a client driver to use the 
hardware.


Ok.. but then why is there a 'halt_reg' configured for the clk?


Thanks for the review.
I will remove the halt_reg for the clocks where we are using the 
'HALT_DELAY'
flag and there is no need to poll the status bit as we are returning 
early

from the 'clk_branch_wait()' function.




>> +
>> +static struct clk_branch gcc_ufs_card_rx_symbol_0_clk = {
>> +   .halt_reg = 0x75018,
>> +   .halt_check = BRANCH_HALT_DELAY,
>
> There are still HALT_DELAY flags for UFS though? Why?

For ufs_card tx/rx symbol clocks, we don't poll the status bit as
per the recommendation from the HW team. We can change the halt_check
type to newly implemented flag "BRANCH_HALT_SKIP". Please update us 
with

your thoughts to change the flag to "BRANCH_HALT_SKIP".


Yes use HALT_SKIP please.


Thanks for confirming. I will do the changes in the next patch series.





>
> Also, are you going to send DFS support for the QUP clks? I would like
> to see that code merged soon.

Taniya has sent the patches for DFS support for QUP clocks.
https://patchwork.kernel.org/patch/10376951/



I'll take a look.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Stephen Boyd]

Quoting Amit Nischal (2018-05-04 03:45:12)
> On 2018-05-02 12:53, Stephen Boyd wrote:
> > Quoting Amit Nischal (2018-04-30 09:20:10)
> >> +
> >> +static struct clk_branch gcc_disp_gpll0_clk_src = {
> >> +   .halt_reg = 0x52004,
> >> +   .halt_check = BRANCH_HALT_DELAY,
> > 
> > What about this one? It's not a phy so I'm confused again why we're
> > unable to check the halt bit. To be clear(er), I don't see why we ever
> > want to have HALT_DELAY used. Hopefully we can remove that flag.
> > 
> > From what I recall, the flag is there for clks that don't toggle their
> > status bit at all, but that we know take a few cycles to ungate the
> > upstream clk. So we threw a delay into the code to make sure that when
> > clk_enable() returned, a driver wouldn't try to use hardware before the
> > clk was actually on. But these cases should pretty much never happen,
> > hence all the pushback against this flag.
> > 
> 
> For these "*gpll0_clk_src" and "*gpll0_div_clk" clocks, there is no halt
> bit to check the status and it is required to have delay for few cycles
> so that clock gets turned on before a client driver to use the hardware.

Ok.. but then why is there a 'halt_reg' configured for the clk?

> >> +
> >> +static struct clk_branch gcc_ufs_card_rx_symbol_0_clk = {
> >> +   .halt_reg = 0x75018,
> >> +   .halt_check = BRANCH_HALT_DELAY,
> > 
> > There are still HALT_DELAY flags for UFS though? Why?
> 
> For ufs_card tx/rx symbol clocks, we don't poll the status bit as
> per the recommendation from the HW team. We can change the halt_check
> type to newly implemented flag "BRANCH_HALT_SKIP". Please update us with
> your thoughts to change the flag to "BRANCH_HALT_SKIP".

Yes use HALT_SKIP please.

> 
> > 
> > Also, are you going to send DFS support for the QUP clks? I would like
> > to see that code merged soon.
> 
> Taniya has sent the patches for DFS support for QUP clocks.
> https://patchwork.kernel.org/patch/10376951/
> 

I'll take a look.

Re: [PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Stephen Boyd]

Quoting Amit Nischal (2018-05-04 03:45:12)
> On 2018-05-02 12:53, Stephen Boyd wrote:
> > Quoting Amit Nischal (2018-04-30 09:20:10)
> >> +
> >> +static struct clk_branch gcc_disp_gpll0_clk_src = {
> >> +   .halt_reg = 0x52004,
> >> +   .halt_check = BRANCH_HALT_DELAY,
> > 
> > What about this one? It's not a phy so I'm confused again why we're
> > unable to check the halt bit. To be clear(er), I don't see why we ever
> > want to have HALT_DELAY used. Hopefully we can remove that flag.
> > 
> > From what I recall, the flag is there for clks that don't toggle their
> > status bit at all, but that we know take a few cycles to ungate the
> > upstream clk. So we threw a delay into the code to make sure that when
> > clk_enable() returned, a driver wouldn't try to use hardware before the
> > clk was actually on. But these cases should pretty much never happen,
> > hence all the pushback against this flag.
> > 
> 
> For these "*gpll0_clk_src" and "*gpll0_div_clk" clocks, there is no halt
> bit to check the status and it is required to have delay for few cycles
> so that clock gets turned on before a client driver to use the hardware.

Ok.. but then why is there a 'halt_reg' configured for the clk?

> >> +
> >> +static struct clk_branch gcc_ufs_card_rx_symbol_0_clk = {
> >> +   .halt_reg = 0x75018,
> >> +   .halt_check = BRANCH_HALT_DELAY,
> > 
> > There are still HALT_DELAY flags for UFS though? Why?
> 
> For ufs_card tx/rx symbol clocks, we don't poll the status bit as
> per the recommendation from the HW team. We can change the halt_check
> type to newly implemented flag "BRANCH_HALT_SKIP". Please update us with
> your thoughts to change the flag to "BRANCH_HALT_SKIP".

Yes use HALT_SKIP please.

> 
> > 
> > Also, are you going to send DFS support for the QUP clks? I would like
> > to see that code merged soon.
> 
> Taniya has sent the patches for DFS support for QUP clocks.
> https://patchwork.kernel.org/patch/10376951/
> 

I'll take a look.

Re: [PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Amit Nischal]

On 2018-05-02 12:53, Stephen Boyd wrote:

Quoting Amit Nischal (2018-04-30 09:20:10)

---
 .../devicetree/bindings/clock/qcom,gcc.txt |1 +
 drivers/clk/qcom/Kconfig   |   10 +-
 drivers/clk/qcom/Makefile  |1 +
 drivers/clk/qcom/gcc-sdm845.c  | 3480 


 include/dt-bindings/clock/qcom,gcc-sdm845.h|  239 ++


Do the split that Rob suggests please, given that you're resending. And
also include his reviewed-by tag.


Thanks for the review. Sure I will split the dt-binding into
separate patch in next series.




 5 files changed, 3727 insertions(+), 4 deletions(-)
 create mode 100644 drivers/clk/qcom/gcc-sdm845.c
 create mode 100644 include/dt-bindings/clock/qcom,gcc-sdm845.h

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index e42e1af..3298beb 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -218,13 +218,15 @@ config MSM_MMCC_8996
  Say Y if you want to support multimedia devices such as 
display,

  graphics, video encode/decode, camera, etc.

-config MSM_GCC_8998
-   tristate "MSM8998 Global Clock Controller"
+config SDM_GCC_845
+   tristate "SDM845 Global Clock Controller"
+   select QCOM_GDSC
depends on COMMON_CLK_QCOM
help
- Support for the global clock controller on msm8998 devices.
+ Support for the global clock controller on Qualcomm 
Technologies, Inc

+ sdm845 devices.
  Say Y if you want to use peripheral devices such as UART, 
SPI,

- i2c, USB, UFS, SD/eMMC, PCIe, etc.
+ I2C, USB, UFS, SDDC, PCIe, etc.


This is all wrong.



My bad. I did by mistake. Will fix this in next series.



 config SPMI_PMIC_CLKDIV
tristate "SPMI PMIC clkdiv Support"
diff --git a/drivers/clk/qcom/gcc-sdm845.c 
b/drivers/clk/qcom/gcc-sdm845.c

new file mode 100644
index 000..6484cba
--- /dev/null
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -0,0 +1,3480 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+

[...]

+   .name = "gcc_disp_axi_clk",
+   .ops = _branch2_ops,
+   },
+   },
+};
+
+static struct clk_branch gcc_disp_gpll0_clk_src = {
+   .halt_reg = 0x52004,
+   .halt_check = BRANCH_HALT_DELAY,


What about this one? It's not a phy so I'm confused again why we're
unable to check the halt bit. To be clear(er), I don't see why we ever
want to have HALT_DELAY used. Hopefully we can remove that flag.

From what I recall, the flag is there for clks that don't toggle their
status bit at all, but that we know take a few cycles to ungate the
upstream clk. So we threw a delay into the code to make sure that when
clk_enable() returned, a driver wouldn't try to use hardware before the
clk was actually on. But these cases should pretty much never happen,
hence all the pushback against this flag.



For these "*gpll0_clk_src" and "*gpll0_div_clk" clocks, there is no halt
bit to check the status and it is required to have delay for few cycles
so that clock gets turned on before a client driver to use the hardware.


+   .clkr = {
+   .enable_reg = 0x52004,
+   .enable_mask = BIT(18),
+   .hw.init = &(struct clk_init_data){
+   .name = "gcc_disp_gpll0_clk_src",
+   .parent_names = (const char *[]){
+   "gpll0",
+   },
+   .num_parents = 1,

[...]

+   .enable_reg = 0x7508c,
+   .enable_mask = BIT(0),
+   .hw.init = &(struct clk_init_data){
+   .name = "gcc_ufs_card_phy_aux_clk",
+   .parent_names = (const char *[]){
+   "gcc_ufs_card_phy_aux_clk_src",
+   },
+   .num_parents = 1,
+   .flags = CLK_SET_RATE_PARENT,
+   .ops = _branch2_ops,
+   },
+   },
+};
+
+static struct clk_branch gcc_ufs_card_rx_symbol_0_clk = {
+   .halt_reg = 0x75018,
+   .halt_check = BRANCH_HALT_DELAY,


There are still HALT_DELAY flags for UFS though? Why?


For ufs_card tx/rx symbol clocks, we don't poll the status bit as
per the recommendation from the HW team. We can change the halt_check
type to newly implemented flag "BRANCH_HALT_SKIP". Please update us with
your thoughts to change the flag to "BRANCH_HALT_SKIP".



Also, are you going to send DFS support for the QUP clks? I would like
to see that code merged soon.


Taniya has sent the patches for DFS support for QUP clocks.
https://patchwork.kernel.org/patch/10376951/

Re: [PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Amit Nischal]

On 2018-05-02 12:53, Stephen Boyd wrote:

Quoting Amit Nischal (2018-04-30 09:20:10)

---
 .../devicetree/bindings/clock/qcom,gcc.txt |1 +
 drivers/clk/qcom/Kconfig   |   10 +-
 drivers/clk/qcom/Makefile  |1 +
 drivers/clk/qcom/gcc-sdm845.c  | 3480 


 include/dt-bindings/clock/qcom,gcc-sdm845.h|  239 ++


Do the split that Rob suggests please, given that you're resending. And
also include his reviewed-by tag.


Thanks for the review. Sure I will split the dt-binding into
separate patch in next series.




 5 files changed, 3727 insertions(+), 4 deletions(-)
 create mode 100644 drivers/clk/qcom/gcc-sdm845.c
 create mode 100644 include/dt-bindings/clock/qcom,gcc-sdm845.h

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index e42e1af..3298beb 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -218,13 +218,15 @@ config MSM_MMCC_8996
  Say Y if you want to support multimedia devices such as 
display,

  graphics, video encode/decode, camera, etc.

-config MSM_GCC_8998
-   tristate "MSM8998 Global Clock Controller"
+config SDM_GCC_845
+   tristate "SDM845 Global Clock Controller"
+   select QCOM_GDSC
depends on COMMON_CLK_QCOM
help
- Support for the global clock controller on msm8998 devices.
+ Support for the global clock controller on Qualcomm 
Technologies, Inc

+ sdm845 devices.
  Say Y if you want to use peripheral devices such as UART, 
SPI,

- i2c, USB, UFS, SD/eMMC, PCIe, etc.
+ I2C, USB, UFS, SDDC, PCIe, etc.


This is all wrong.



My bad. I did by mistake. Will fix this in next series.



 config SPMI_PMIC_CLKDIV
tristate "SPMI PMIC clkdiv Support"
diff --git a/drivers/clk/qcom/gcc-sdm845.c 
b/drivers/clk/qcom/gcc-sdm845.c

new file mode 100644
index 000..6484cba
--- /dev/null
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -0,0 +1,3480 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+

[...]

+   .name = "gcc_disp_axi_clk",
+   .ops = _branch2_ops,
+   },
+   },
+};
+
+static struct clk_branch gcc_disp_gpll0_clk_src = {
+   .halt_reg = 0x52004,
+   .halt_check = BRANCH_HALT_DELAY,


What about this one? It's not a phy so I'm confused again why we're
unable to check the halt bit. To be clear(er), I don't see why we ever
want to have HALT_DELAY used. Hopefully we can remove that flag.

From what I recall, the flag is there for clks that don't toggle their
status bit at all, but that we know take a few cycles to ungate the
upstream clk. So we threw a delay into the code to make sure that when
clk_enable() returned, a driver wouldn't try to use hardware before the
clk was actually on. But these cases should pretty much never happen,
hence all the pushback against this flag.



For these "*gpll0_clk_src" and "*gpll0_div_clk" clocks, there is no halt
bit to check the status and it is required to have delay for few cycles
so that clock gets turned on before a client driver to use the hardware.


+   .clkr = {
+   .enable_reg = 0x52004,
+   .enable_mask = BIT(18),
+   .hw.init = &(struct clk_init_data){
+   .name = "gcc_disp_gpll0_clk_src",
+   .parent_names = (const char *[]){
+   "gpll0",
+   },
+   .num_parents = 1,

[...]

+   .enable_reg = 0x7508c,
+   .enable_mask = BIT(0),
+   .hw.init = &(struct clk_init_data){
+   .name = "gcc_ufs_card_phy_aux_clk",
+   .parent_names = (const char *[]){
+   "gcc_ufs_card_phy_aux_clk_src",
+   },
+   .num_parents = 1,
+   .flags = CLK_SET_RATE_PARENT,
+   .ops = _branch2_ops,
+   },
+   },
+};
+
+static struct clk_branch gcc_ufs_card_rx_symbol_0_clk = {
+   .halt_reg = 0x75018,
+   .halt_check = BRANCH_HALT_DELAY,


There are still HALT_DELAY flags for UFS though? Why?


For ufs_card tx/rx symbol clocks, we don't poll the status bit as
per the recommendation from the HW team. We can change the halt_check
type to newly implemented flag "BRANCH_HALT_SKIP". Please update us with
your thoughts to change the flag to "BRANCH_HALT_SKIP".



Also, are you going to send DFS support for the QUP clks? I would like
to see that code merged soon.


Taniya has sent the patches for DFS support for QUP clocks.
https://patchwork.kernel.org/patch/10376951/

Re: [PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Stephen Boyd]

Quoting Amit Nischal (2018-04-30 09:20:10)
> ---
>  .../devicetree/bindings/clock/qcom,gcc.txt |1 +
>  drivers/clk/qcom/Kconfig   |   10 +-
>  drivers/clk/qcom/Makefile  |1 +
>  drivers/clk/qcom/gcc-sdm845.c  | 3480 
> 
>  include/dt-bindings/clock/qcom,gcc-sdm845.h|  239 ++

Do the split that Rob suggests please, given that you're resending. And
also include his reviewed-by tag.

>  5 files changed, 3727 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/clk/qcom/gcc-sdm845.c
>  create mode 100644 include/dt-bindings/clock/qcom,gcc-sdm845.h
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index e42e1af..3298beb 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -218,13 +218,15 @@ config MSM_MMCC_8996
>   Say Y if you want to support multimedia devices such as display,
>   graphics, video encode/decode, camera, etc.
> 
> -config MSM_GCC_8998
> -   tristate "MSM8998 Global Clock Controller"
> +config SDM_GCC_845
> +   tristate "SDM845 Global Clock Controller"
> +   select QCOM_GDSC
> depends on COMMON_CLK_QCOM
> help
> - Support for the global clock controller on msm8998 devices.
> + Support for the global clock controller on Qualcomm Technologies, 
> Inc
> + sdm845 devices.
>   Say Y if you want to use peripheral devices such as UART, SPI,
> - i2c, USB, UFS, SD/eMMC, PCIe, etc.
> + I2C, USB, UFS, SDDC, PCIe, etc.

This is all wrong.

> 
>  config SPMI_PMIC_CLKDIV
> tristate "SPMI PMIC clkdiv Support"
> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
> new file mode 100644
> index 000..6484cba
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-sdm845.c
> @@ -0,0 +1,3480 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
[...]
> +   .name = "gcc_disp_axi_clk",
> +   .ops = _branch2_ops,
> +   },
> +   },
> +};
> +
> +static struct clk_branch gcc_disp_gpll0_clk_src = {
> +   .halt_reg = 0x52004,
> +   .halt_check = BRANCH_HALT_DELAY,

What about this one? It's not a phy so I'm confused again why we're
unable to check the halt bit. To be clear(er), I don't see why we ever
want to have HALT_DELAY used. Hopefully we can remove that flag.

From what I recall, the flag is there for clks that don't toggle their
status bit at all, but that we know take a few cycles to ungate the
upstream clk. So we threw a delay into the code to make sure that when
clk_enable() returned, a driver wouldn't try to use hardware before the
clk was actually on. But these cases should pretty much never happen,
hence all the pushback against this flag.

> +   .clkr = {
> +   .enable_reg = 0x52004,
> +   .enable_mask = BIT(18),
> +   .hw.init = &(struct clk_init_data){
> +   .name = "gcc_disp_gpll0_clk_src",
> +   .parent_names = (const char *[]){
> +   "gpll0",
> +   },
> +   .num_parents = 1,
[...]
> +   .enable_reg = 0x7508c,
> +   .enable_mask = BIT(0),
> +   .hw.init = &(struct clk_init_data){
> +   .name = "gcc_ufs_card_phy_aux_clk",
> +   .parent_names = (const char *[]){
> +   "gcc_ufs_card_phy_aux_clk_src",
> +   },
> +   .num_parents = 1,
> +   .flags = CLK_SET_RATE_PARENT,
> +   .ops = _branch2_ops,
> +   },
> +   },
> +};
> +
> +static struct clk_branch gcc_ufs_card_rx_symbol_0_clk = {
> +   .halt_reg = 0x75018,
> +   .halt_check = BRANCH_HALT_DELAY,

There are still HALT_DELAY flags for UFS though? Why?

Also, are you going to send DFS support for the QUP clks? I would like
to see that code merged soon.

Re: [PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Stephen Boyd]

Quoting Amit Nischal (2018-04-30 09:20:10)
> ---
>  .../devicetree/bindings/clock/qcom,gcc.txt |1 +
>  drivers/clk/qcom/Kconfig   |   10 +-
>  drivers/clk/qcom/Makefile  |1 +
>  drivers/clk/qcom/gcc-sdm845.c  | 3480 
> 
>  include/dt-bindings/clock/qcom,gcc-sdm845.h|  239 ++

Do the split that Rob suggests please, given that you're resending. And
also include his reviewed-by tag.

>  5 files changed, 3727 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/clk/qcom/gcc-sdm845.c
>  create mode 100644 include/dt-bindings/clock/qcom,gcc-sdm845.h
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index e42e1af..3298beb 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -218,13 +218,15 @@ config MSM_MMCC_8996
>   Say Y if you want to support multimedia devices such as display,
>   graphics, video encode/decode, camera, etc.
> 
> -config MSM_GCC_8998
> -   tristate "MSM8998 Global Clock Controller"
> +config SDM_GCC_845
> +   tristate "SDM845 Global Clock Controller"
> +   select QCOM_GDSC
> depends on COMMON_CLK_QCOM
> help
> - Support for the global clock controller on msm8998 devices.
> + Support for the global clock controller on Qualcomm Technologies, 
> Inc
> + sdm845 devices.
>   Say Y if you want to use peripheral devices such as UART, SPI,
> - i2c, USB, UFS, SD/eMMC, PCIe, etc.
> + I2C, USB, UFS, SDDC, PCIe, etc.

This is all wrong.

> 
>  config SPMI_PMIC_CLKDIV
> tristate "SPMI PMIC clkdiv Support"
> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
> new file mode 100644
> index 000..6484cba
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-sdm845.c
> @@ -0,0 +1,3480 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
[...]
> +   .name = "gcc_disp_axi_clk",
> +   .ops = _branch2_ops,
> +   },
> +   },
> +};
> +
> +static struct clk_branch gcc_disp_gpll0_clk_src = {
> +   .halt_reg = 0x52004,
> +   .halt_check = BRANCH_HALT_DELAY,

What about this one? It's not a phy so I'm confused again why we're
unable to check the halt bit. To be clear(er), I don't see why we ever
want to have HALT_DELAY used. Hopefully we can remove that flag.

From what I recall, the flag is there for clks that don't toggle their
status bit at all, but that we know take a few cycles to ungate the
upstream clk. So we threw a delay into the code to make sure that when
clk_enable() returned, a driver wouldn't try to use hardware before the
clk was actually on. But these cases should pretty much never happen,
hence all the pushback against this flag.

> +   .clkr = {
> +   .enable_reg = 0x52004,
> +   .enable_mask = BIT(18),
> +   .hw.init = &(struct clk_init_data){
> +   .name = "gcc_disp_gpll0_clk_src",
> +   .parent_names = (const char *[]){
> +   "gpll0",
> +   },
> +   .num_parents = 1,
[...]
> +   .enable_reg = 0x7508c,
> +   .enable_mask = BIT(0),
> +   .hw.init = &(struct clk_init_data){
> +   .name = "gcc_ufs_card_phy_aux_clk",
> +   .parent_names = (const char *[]){
> +   "gcc_ufs_card_phy_aux_clk_src",
> +   },
> +   .num_parents = 1,
> +   .flags = CLK_SET_RATE_PARENT,
> +   .ops = _branch2_ops,
> +   },
> +   },
> +};
> +
> +static struct clk_branch gcc_ufs_card_rx_symbol_0_clk = {
> +   .halt_reg = 0x75018,
> +   .halt_check = BRANCH_HALT_DELAY,

There are still HALT_DELAY flags for UFS though? Why?

Also, are you going to send DFS support for the QUP clks? I would like
to see that code merged soon.

Re: [PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Rob Herring]

On Mon, Apr 30, 2018 at 09:50:10PM +0530, Amit Nischal wrote:
> From: Taniya Das 
> 
> Add support for the global clock controller found on SDM845
> based devices. This should allow most non-multimedia device
> drivers to probe and control their clocks.
> 
> Signed-off-by: Taniya Das 
> Signed-off-by: Amit Nischal 
> ---
>  .../devicetree/bindings/clock/qcom,gcc.txt |1 +
>  drivers/clk/qcom/Kconfig   |   10 +-
>  drivers/clk/qcom/Makefile  |1 +
>  drivers/clk/qcom/gcc-sdm845.c  | 3480 
> 
>  include/dt-bindings/clock/qcom,gcc-sdm845.h|  239 ++
>  5 files changed, 3727 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/clk/qcom/gcc-sdm845.c
>  create mode 100644 include/dt-bindings/clock/qcom,gcc-sdm845.h

For the DT bits:

Reviewed-by: Rob Herring 

In the future please split bindings and binding headers to separate 
patch.

Rob

Re: [PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Rob Herring]

On Mon, Apr 30, 2018 at 09:50:10PM +0530, Amit Nischal wrote:
> From: Taniya Das 
> 
> Add support for the global clock controller found on SDM845
> based devices. This should allow most non-multimedia device
> drivers to probe and control their clocks.
> 
> Signed-off-by: Taniya Das 
> Signed-off-by: Amit Nischal 
> ---
>  .../devicetree/bindings/clock/qcom,gcc.txt |1 +
>  drivers/clk/qcom/Kconfig   |   10 +-
>  drivers/clk/qcom/Makefile  |1 +
>  drivers/clk/qcom/gcc-sdm845.c  | 3480 
> 
>  include/dt-bindings/clock/qcom,gcc-sdm845.h|  239 ++
>  5 files changed, 3727 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/clk/qcom/gcc-sdm845.c
>  create mode 100644 include/dt-bindings/clock/qcom,gcc-sdm845.h

For the DT bits:

Reviewed-by: Rob Herring 

In the future please split bindings and binding headers to separate 
patch.

Rob

[PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Amit Nischal]

From: Taniya Das 

Add support for the global clock controller found on SDM845
based devices. This should allow most non-multimedia device
drivers to probe and control their clocks.

Signed-off-by: Taniya Das 
Signed-off-by: Amit Nischal 
---
 .../devicetree/bindings/clock/qcom,gcc.txt |1 +
 drivers/clk/qcom/Kconfig   |   10 +-
 drivers/clk/qcom/Makefile  |1 +
 drivers/clk/qcom/gcc-sdm845.c  | 3480 
 include/dt-bindings/clock/qcom,gcc-sdm845.h|  239 ++
 5 files changed, 3727 insertions(+), 4 deletions(-)
 create mode 100644 drivers/clk/qcom/gcc-sdm845.c
 create mode 100644 include/dt-bindings/clock/qcom,gcc-sdm845.h

diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt 
b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
index d1fb8b2..664ea1f 100644
--- a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
+++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
@@ -19,6 +19,7 @@ Required properties :
"qcom,gcc-msm8996"
"qcom,gcc-msm8998"
"qcom,gcc-mdm9615"
+   "qcom,gcc-sdm845"

 - reg : shall contain base register location and length
 - #clock-cells : shall contain 1
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index e42e1af..3298beb 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -218,13 +218,15 @@ config MSM_MMCC_8996
  Say Y if you want to support multimedia devices such as display,
  graphics, video encode/decode, camera, etc.

-config MSM_GCC_8998
-   tristate "MSM8998 Global Clock Controller"
+config SDM_GCC_845
+   tristate "SDM845 Global Clock Controller"
+   select QCOM_GDSC
depends on COMMON_CLK_QCOM
help
- Support for the global clock controller on msm8998 devices.
+ Support for the global clock controller on Qualcomm Technologies, Inc
+ sdm845 devices.
  Say Y if you want to use peripheral devices such as UART, SPI,
- i2c, USB, UFS, SD/eMMC, PCIe, etc.
+ I2C, USB, UFS, SDDC, PCIe, etc.

 config SPMI_PMIC_CLKDIV
tristate "SPMI PMIC clkdiv Support"
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 7c09ab1..bf8fb25 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -38,4 +38,5 @@ obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
 obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
 obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
+obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
 obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
new file mode 100644
index 000..6484cba
--- /dev/null
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -0,0 +1,3480 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include "common.h"
+#include "clk-regmap.h"
+#include "clk-pll.h"
+#include "clk-rcg.h"
+#include "clk-branch.h"
+#include "clk-alpha-pll.h"
+#include "gdsc.h"
+#include "reset.h"
+
+#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }
+
+enum {
+   P_BI_TCXO,
+   P_AUD_REF_CLK,
+   P_CORE_BI_PLL_TEST_SE,
+   P_GPLL0_OUT_EVEN,
+   P_GPLL0_OUT_MAIN,
+   P_GPLL4_OUT_MAIN,
+   P_SLEEP_CLK,
+};
+
+static const struct parent_map gcc_parent_map_0[] = {
+   { P_BI_TCXO, 0 },
+   { P_GPLL0_OUT_MAIN, 1 },
+   { P_GPLL0_OUT_EVEN, 6 },
+   { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_0[] = {
+   "bi_tcxo",
+   "gpll0",
+   "gpll0_out_even",
+   "core_bi_pll_test_se",
+};
+
+static const struct parent_map gcc_parent_map_1[] = {
+   { P_BI_TCXO, 0 },
+   { P_GPLL0_OUT_MAIN, 1 },
+   { P_SLEEP_CLK, 5 },
+   { P_GPLL0_OUT_EVEN, 6 },
+   { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_1[] = {
+   "bi_tcxo",
+   "gpll0",
+   "core_pi_sleep_clk",
+   "gpll0_out_even",
+   "core_bi_pll_test_se",
+};
+
+static const struct parent_map gcc_parent_map_2[] = {
+   { P_BI_TCXO, 0 },
+   { P_SLEEP_CLK, 5 },
+   { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_2[] = {
+   "bi_tcxo",
+   "core_pi_sleep_clk",
+   "core_bi_pll_test_se",
+};
+
+static const struct parent_map gcc_parent_map_3[] = {
+   { P_BI_TCXO, 0 },
+   { P_GPLL0_OUT_MAIN, 1 },
+   { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_3[] = {
+   "bi_tcxo",
+   "gpll0",
+   "core_bi_pll_test_se",
+};
+
+static const struct parent_map 

[PATCH v6 3/3] clk: qcom: Add Global Clock controller (GCC) driver for SDM845

[Amit Nischal]

From: Taniya Das 

Add support for the global clock controller found on SDM845
based devices. This should allow most non-multimedia device
drivers to probe and control their clocks.

Signed-off-by: Taniya Das 
Signed-off-by: Amit Nischal 
---
 .../devicetree/bindings/clock/qcom,gcc.txt |1 +
 drivers/clk/qcom/Kconfig   |   10 +-
 drivers/clk/qcom/Makefile  |1 +
 drivers/clk/qcom/gcc-sdm845.c  | 3480 
 include/dt-bindings/clock/qcom,gcc-sdm845.h|  239 ++
 5 files changed, 3727 insertions(+), 4 deletions(-)
 create mode 100644 drivers/clk/qcom/gcc-sdm845.c
 create mode 100644 include/dt-bindings/clock/qcom,gcc-sdm845.h

diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.txt 
b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
index d1fb8b2..664ea1f 100644
--- a/Documentation/devicetree/bindings/clock/qcom,gcc.txt
+++ b/Documentation/devicetree/bindings/clock/qcom,gcc.txt
@@ -19,6 +19,7 @@ Required properties :
"qcom,gcc-msm8996"
"qcom,gcc-msm8998"
"qcom,gcc-mdm9615"
+   "qcom,gcc-sdm845"

 - reg : shall contain base register location and length
 - #clock-cells : shall contain 1
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index e42e1af..3298beb 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -218,13 +218,15 @@ config MSM_MMCC_8996
  Say Y if you want to support multimedia devices such as display,
  graphics, video encode/decode, camera, etc.

-config MSM_GCC_8998
-   tristate "MSM8998 Global Clock Controller"
+config SDM_GCC_845
+   tristate "SDM845 Global Clock Controller"
+   select QCOM_GDSC
depends on COMMON_CLK_QCOM
help
- Support for the global clock controller on msm8998 devices.
+ Support for the global clock controller on Qualcomm Technologies, Inc
+ sdm845 devices.
  Say Y if you want to use peripheral devices such as UART, SPI,
- i2c, USB, UFS, SD/eMMC, PCIe, etc.
+ I2C, USB, UFS, SDDC, PCIe, etc.

 config SPMI_PMIC_CLKDIV
tristate "SPMI PMIC clkdiv Support"
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 7c09ab1..bf8fb25 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -38,4 +38,5 @@ obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
 obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
 obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
+obj-$(CONFIG_SDM_GCC_845) += gcc-sdm845.o
 obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c
new file mode 100644
index 000..6484cba
--- /dev/null
+++ b/drivers/clk/qcom/gcc-sdm845.c
@@ -0,0 +1,3480 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include "common.h"
+#include "clk-regmap.h"
+#include "clk-pll.h"
+#include "clk-rcg.h"
+#include "clk-branch.h"
+#include "clk-alpha-pll.h"
+#include "gdsc.h"
+#include "reset.h"
+
+#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) }
+
+enum {
+   P_BI_TCXO,
+   P_AUD_REF_CLK,
+   P_CORE_BI_PLL_TEST_SE,
+   P_GPLL0_OUT_EVEN,
+   P_GPLL0_OUT_MAIN,
+   P_GPLL4_OUT_MAIN,
+   P_SLEEP_CLK,
+};
+
+static const struct parent_map gcc_parent_map_0[] = {
+   { P_BI_TCXO, 0 },
+   { P_GPLL0_OUT_MAIN, 1 },
+   { P_GPLL0_OUT_EVEN, 6 },
+   { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_0[] = {
+   "bi_tcxo",
+   "gpll0",
+   "gpll0_out_even",
+   "core_bi_pll_test_se",
+};
+
+static const struct parent_map gcc_parent_map_1[] = {
+   { P_BI_TCXO, 0 },
+   { P_GPLL0_OUT_MAIN, 1 },
+   { P_SLEEP_CLK, 5 },
+   { P_GPLL0_OUT_EVEN, 6 },
+   { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_1[] = {
+   "bi_tcxo",
+   "gpll0",
+   "core_pi_sleep_clk",
+   "gpll0_out_even",
+   "core_bi_pll_test_se",
+};
+
+static const struct parent_map gcc_parent_map_2[] = {
+   { P_BI_TCXO, 0 },
+   { P_SLEEP_CLK, 5 },
+   { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_2[] = {
+   "bi_tcxo",
+   "core_pi_sleep_clk",
+   "core_bi_pll_test_se",
+};
+
+static const struct parent_map gcc_parent_map_3[] = {
+   { P_BI_TCXO, 0 },
+   { P_GPLL0_OUT_MAIN, 1 },
+   { P_CORE_BI_PLL_TEST_SE, 7 },
+};
+
+static const char * const gcc_parent_names_3[] = {
+   "bi_tcxo",
+   "gpll0",
+   "core_bi_pll_test_se",
+};
+
+static const struct parent_map gcc_parent_map_4[] = {
+   { P_BI_TCXO, 0 },
+   {