[v5,0/2] intel: Add a new driver for a new clock controller IP

Hi,

This series adds clock driver for Clock Generation Unit(CGU) of
Lightning Mountain(LGM) SoC.

Patch 1 adds bindings document & include file for CGU.
Patch 2 adds common clock framework based clock driver for CGU.

These patches are baselined upon Linux 5.6-rc1 at below Git link:
git git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git

v5:
- Address review concerns - mainly below mentioned. (Stephen Boyd)
- Improve commit message, add COMPILE_TEST in KConfig dependency.
- Remove unused header include files, drop unnecessary casts.
- Switch to using readl_poll_timeout() instead of implementing timeout routine.
- Avoid using small functions which are called just once. Inline them or
  remove them.
- const static --> static const
- Fix coding style/convention related review concerns.
- Use __iomem for all IO addresses variables.
- Consolidate clk_enable & clk_disable ops into a common clk_enable_disable
  routine to avoid redundant code.
- Remove unnecessary dev pointers for clk data structures.
- Redesign code to use new way of specifying clk_parents i.e. use
  clk_parent_data.fw_name instead of older parent_name strings.
- Switch from raw_spin_locks() to normal spin_locks() and realign locking.
- Drop __initconst, __init, __refdata.
- Reorder patch series - make dt-binding patch as first patch.
- Add pointer to include file in dt-bindings document.
- Remove CLK_IS_CRITICAL flag for clks for which IGNORE_UNUSED flag is enough.
  Add comments for clks which are marked as CRITICAL.
- Fix $id path in dt-bindings - drop bindings. (Rob Herring).
- Add Reviewed-by tag from Rob Herring. Thanks Rob.

v4:
- Add drivers/clk/x86/Kconfig file which got missed in v3 by mistake.

v3:
- Address review concerns:
  Add Kconfig entry in x86 folder instead of modifying clk/Kconfig. (Andy Shevchenko)
  Fix coding style/convention related concerns. (Andy Shevchenko)
  Improve description, licensing info, rename node name correctly in dt bindings
  document & remove CLK_NR_CLKS from dt-bindings header file. (Stephen Boyd)
  Fix a build warning reported by kbuild test robot & Nathan Chancellor
- Add few new clocks & rename few existing clocks.
- Add more ops for ddiv & divider clk_ops.
- Fix few minor bugs.
- Use CLK_IS_CRITICAL flag for clocks which shall never be disabled.

v2:
- Move the driver to x86 folder.
- Remove syscon usage.
- Remove regmap based access. Use direct readl()/write() instead. Add spinlocks.
- Change all enum values to capitals.
- Rename all data structures & functions from intel_* to lgm_*.
- Remove multiple header files. Keep only one header file.
- Make probe fail when any of the clk/pll registration fails.
- Fix few bugs with clk_init_data assignement.
- Address review concerns for code quality/style/convention.

v1:
- Initial version.

Rahul Tanwar (1):
  dt-bindings: clk: intel: Add bindings document & header file for CGU

rtanwar (1):
  clk: intel: Add CGU clock driver for a new SoC

 .../devicetree/bindings/clock/intel,cgu-lgm.yaml   |  44 ++
 drivers/clk/Kconfig                                |   1 +
 drivers/clk/x86/Kconfig                            |   8 +
 drivers/clk/x86/Makefile                           |   1 +
 drivers/clk/x86/clk-cgu-pll.c                      | 156 +++++
 drivers/clk/x86/clk-cgu.c                          | 636 +++++++++++++++++++++
 drivers/clk/x86/clk-cgu.h                          | 335 +++++++++++
 drivers/clk/x86/clk-lgm.c                          | 492 ++++++++++++++++
 include/dt-bindings/clock/intel,lgm-clk.h          | 165 ++++++
 9 files changed, 1838 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/intel,cgu-lgm.yaml
 create mode 100644 drivers/clk/x86/Kconfig
 create mode 100644 drivers/clk/x86/clk-cgu-pll.c
 create mode 100644 drivers/clk/x86/clk-cgu.c
 create mode 100644 drivers/clk/x86/clk-cgu.h
 create mode 100644 drivers/clk/x86/clk-lgm.c
 create mode 100644 include/dt-bindings/clock/intel,lgm-clk.h

On 2/18/20 11:40 PM, Rahul Tanwar wrote:
> From: rtanwar <rahul.tanwar@intel.com>
> 
> Clock Generation Unit(CGU) is a new clock controller IP of a forthcoming
> Intel network processor SoC named Lightning Mountain(LGM). It provides
> programming interfaces to control & configure all CPU & peripheral clocks.
> Add common clock framework based clock controller driver for CGU.
> 
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
>  drivers/clk/Kconfig           |   1 +
>  drivers/clk/x86/Kconfig       |   8 +
>  drivers/clk/x86/Makefile      |   1 +
>  drivers/clk/x86/clk-cgu-pll.c | 156 +++++++++++
>  drivers/clk/x86/clk-cgu.c     | 636 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/x86/clk-cgu.h     | 335 ++++++++++++++++++++++
>  drivers/clk/x86/clk-lgm.c     | 492 ++++++++++++++++++++++++++++++++
>  7 files changed, 1629 insertions(+)
>  create mode 100644 drivers/clk/x86/Kconfig
>  create mode 100644 drivers/clk/x86/clk-cgu-pll.c
>  create mode 100644 drivers/clk/x86/clk-cgu.c
>  create mode 100644 drivers/clk/x86/clk-cgu.h
>  create mode 100644 drivers/clk/x86/clk-lgm.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index bcb257baed06..43dab257e7aa 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -360,6 +360,7 @@ source "drivers/clk/sunxi-ng/Kconfig"
>  source "drivers/clk/tegra/Kconfig"
>  source "drivers/clk/ti/Kconfig"
>  source "drivers/clk/uniphier/Kconfig"
> +source "drivers/clk/x86/Kconfig"
>  source "drivers/clk/zynqmp/Kconfig"
>  
>  endmenu

Hi,

> diff --git a/drivers/clk/x86/Kconfig b/drivers/clk/x86/Kconfig
> new file mode 100644
> index 000000000000..2e2b9730541f
> --- /dev/null
> +++ b/drivers/clk/x86/Kconfig
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config CLK_LGM_CGU
> +	depends on (OF && HAS_IOMEM) || COMPILE_TEST

This "depends on" looks problematic to me. I guess we shall see when
all the build bots get to it.

> +	select OF_EARLY_FLATTREE

If OF is not set and HAS_IOMEM is not set, but COMPILE_TEST is set,
I expect that this should not be attempting to select OF_EARLY_FLATTREE.

Have you tried such a config combination?

> +	bool "Clock driver for Lightning Mountain(LGM) platform"
> +	help
> +	  Clock Generation Unit(CGU) driver for Intel Lightning Mountain(LGM)
> +	  network processor SoC.
thanks.

Hi Randy,

On 19/2/2020 3:59 PM, Randy Dunlap wrote:
> On 2/18/20 11:40 PM, Rahul Tanwar wrote:
>> From: rtanwar <rahul.tanwar@intel.com>
>>
>> Clock Generation Unit(CGU) is a new clock controller IP of a forthcoming
>> Intel network processor SoC named Lightning Mountain(LGM). It provides
>> programming interfaces to control & configure all CPU & peripheral clocks.
>> Add common clock framework based clock controller driver for CGU.
>>
>> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
>> ---
>>  drivers/clk/Kconfig           |   1 +
>>  drivers/clk/x86/Kconfig       |   8 +
>>  drivers/clk/x86/Makefile      |   1 +
>>  drivers/clk/x86/clk-cgu-pll.c | 156 +++++++++++
>>  drivers/clk/x86/clk-cgu.c     | 636 ++++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/x86/clk-cgu.h     | 335 ++++++++++++++++++++++
>>  drivers/clk/x86/clk-lgm.c     | 492 ++++++++++++++++++++++++++++++++
>>  7 files changed, 1629 insertions(+)
>>  create mode 100644 drivers/clk/x86/Kconfig
>>  create mode 100644 drivers/clk/x86/clk-cgu-pll.c
>>  create mode 100644 drivers/clk/x86/clk-cgu.c
>>  create mode 100644 drivers/clk/x86/clk-cgu.h
>>  create mode 100644 drivers/clk/x86/clk-lgm.c
>>
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index bcb257baed06..43dab257e7aa 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -360,6 +360,7 @@ source "drivers/clk/sunxi-ng/Kconfig"
>>  source "drivers/clk/tegra/Kconfig"
>>  source "drivers/clk/ti/Kconfig"
>>  source "drivers/clk/uniphier/Kconfig"
>> +source "drivers/clk/x86/Kconfig"
>>  source "drivers/clk/zynqmp/Kconfig"
>>  
>>  endmenu
> Hi,
>
>> diff --git a/drivers/clk/x86/Kconfig b/drivers/clk/x86/Kconfig
>> new file mode 100644
>> index 000000000000..2e2b9730541f
>> --- /dev/null
>> +++ b/drivers/clk/x86/Kconfig
>> @@ -0,0 +1,8 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +config CLK_LGM_CGU
>> +	depends on (OF && HAS_IOMEM) || COMPILE_TEST
> This "depends on" looks problematic to me. I guess we shall see when
> all the build bots get to it.

At the moment, i am not able to figure out possible problems in this..

>> +	select OF_EARLY_FLATTREE
> If OF is not set and HAS_IOMEM is not set, but COMPILE_TEST is set,
> I expect that this should not be attempting to select OF_EARLY_FLATTREE.
>
> Have you tried such a config combination?

Agree, that would be a problem. I will change it to

select OF_EARLY_FLATTREE if OF

Thanks.

Regards,
Rahul

On Thu, Feb 27, 2020 at 03:19:26PM +0800, Tanwar, Rahul wrote:
> On 19/2/2020 3:59 PM, Randy Dunlap wrote:
> > On 2/18/20 11:40 PM, Rahul Tanwar wrote:

> >> +config CLK_LGM_CGU
> >> +	depends on (OF && HAS_IOMEM) || COMPILE_TEST
> > This "depends on" looks problematic to me. I guess we shall see when
> > all the build bots get to it.
> 
> At the moment, i am not able to figure out possible problems in this..

COMPILE_TEST should be accompanied by non-generic dependency.
There is none.

So, I quite agree with Randy.

> >> +	select OF_EARLY_FLATTREE
> > If OF is not set and HAS_IOMEM is not set, but COMPILE_TEST is set,
> > I expect that this should not be attempting to select OF_EARLY_FLATTREE.
> >
> > Have you tried such a config combination?
> 
> Agree, that would be a problem. I will change it to
> 
> select OF_EARLY_FLATTREE if OF

Nope, I think this is wrong work around.
See above.

On 27/2/2020 6:02 PM, Andy Shevchenko wrote:
> On Thu, Feb 27, 2020 at 03:19:26PM +0800, Tanwar, Rahul wrote:
>> On 19/2/2020 3:59 PM, Randy Dunlap wrote:
>>> On 2/18/20 11:40 PM, Rahul Tanwar wrote:
>>>
>>>> +config CLK_LGM_CGU
>>>> +	depends on (OF && HAS_IOMEM) || COMPILE_TEST
>>> This "depends on" looks problematic to me. I guess we shall see when
>>> all the build bots get to it.
>> At the moment, i am not able to figure out possible problems in this..
> COMPILE_TEST should be accompanied by non-generic dependency.
> There is none.
>
> So, I quite agree with Randy.

I see COMPILE_TEST is mostly ORed with ARCH_xx. How about below?

depends on OF && HAS_IOMEM && (CONFIG_X86 || COMPILE_TEST)

>>>> +	select OF_EARLY_FLATTREE
>>> If OF is not set and HAS_IOMEM is not set, but COMPILE_TEST is set,
>>> I expect that this should not be attempting to select OF_EARLY_FLATTREE.
>>>
>>> Have you tried such a config combination?
>> Agree, that would be a problem. I will change it to
>>
>> select OF_EARLY_FLATTREE if OF
> Nope, I think this is wrong work around.
> See above.

With above proposed change, i can simply switch to
select OF_EARLY_FLATTREE since all dependencies are already
in place..

Thanks.

Regards,
Rahul

On Tue, Mar 03, 2020 at 11:37:23AM +0800, Tanwar, Rahul wrote:
> On 27/2/2020 6:02 PM, Andy Shevchenko wrote:
> > On Thu, Feb 27, 2020 at 03:19:26PM +0800, Tanwar, Rahul wrote:
> >> On 19/2/2020 3:59 PM, Randy Dunlap wrote:
> >>> On 2/18/20 11:40 PM, Rahul Tanwar wrote:
> >>>
> >>>> +config CLK_LGM_CGU
> >>>> +	depends on (OF && HAS_IOMEM) || COMPILE_TEST
> >>> This "depends on" looks problematic to me. I guess we shall see when
> >>> all the build bots get to it.
> >> At the moment, i am not able to figure out possible problems in this..
> > COMPILE_TEST should be accompanied by non-generic dependency.
> > There is none.
> >
> > So, I quite agree with Randy.
> 
> I see COMPILE_TEST is mostly ORed with ARCH_xx. How about below?
> 
> depends on OF && HAS_IOMEM && (CONFIG_X86 || COMPILE_TEST)

How about to leave logical parts separately?
How is OF related to architecture?

On top of that, is this code only for x86 for sure?

> >>>> +	select OF_EARLY_FLATTREE
> >>> If OF is not set and HAS_IOMEM is not set, but COMPILE_TEST is set,
> >>> I expect that this should not be attempting to select OF_EARLY_FLATTREE.
> >>>
> >>> Have you tried such a config combination?
> >> Agree, that would be a problem. I will change it to
> >>
> >> select OF_EARLY_FLATTREE if OF
> > Nope, I think this is wrong work around.
> > See above.
> 
> With above proposed change, i can simply switch to
> select OF_EARLY_FLATTREE since all dependencies are already
> in place..

Right.

On 3/3/2020 6:09 PM, Andy Shevchenko wrote:
> On Tue, Mar 03, 2020 at 11:37:23AM +0800, Tanwar, Rahul wrote:
>> On 27/2/2020 6:02 PM, Andy Shevchenko wrote:
>>> On Thu, Feb 27, 2020 at 03:19:26PM +0800, Tanwar, Rahul wrote:
>>>> On 19/2/2020 3:59 PM, Randy Dunlap wrote:
>>>>> On 2/18/20 11:40 PM, Rahul Tanwar wrote:
>>>>>
>>>>>> +config CLK_LGM_CGU
>>>>>> +	depends on (OF && HAS_IOMEM) || COMPILE_TEST
>>>>> This "depends on" looks problematic to me. I guess we shall see when
>>>>> all the build bots get to it.
>>>> At the moment, i am not able to figure out possible problems in this..
>>> COMPILE_TEST should be accompanied by non-generic dependency.
>>> There is none.
>>>
>>> So, I quite agree with Randy.
>> I see COMPILE_TEST is mostly ORed with ARCH_xx. How about below?
>>
>> depends on OF && HAS_IOMEM && (CONFIG_X86 || COMPILE_TEST)
> How about to leave logical parts separately?
> How is OF related to architecture?

OF is not related to architecture. Driver depends on OF.
In the past, many build/linker issues were reported due to OF
& HAS_IOMEM dependencies in the code. Please see [1] & [2]. So
to be safe this time, i am adding these dependencies here.

> On top of that, is this code only for x86 for sure?
>

Yes, this is a totally new IP for x86 based SoC. No plans
of using same IP/driver for any other arch based SoCs.


[1] https://lkml.org/lkml/2019/12/3/613
[2] https://lkml.org/lkml/2019/12/11/1733