[V9,0/7] clk: imx: add imx8qxp clock support

This patch series adds i.MX8QXP clock support which is based
on the clock service provided by SCU firmware.

Note: It depends on SCU driver which has already been merged by Shawn.
So this patch series could go through Shawn's tree as well.

ChangeLog:
v8->v9:
 * add __le32 and __le16 for SCU message structure members as it's little endian
 * use readl_relaxed() as it does not need insert barrier
 * a small code logic improvement suggested by Stephen
v7->v8:
 * add more kernel doc for lpcg and scu clock structures
 * remove one unneccessry reg checking
v6->v7:
 * use struct_size()
 * remove MODULE_ macros
 * add more kernel docs/code comments
 * other small improvements
 * update reviewed-by tags
 * add the missing PATCH 1 in V6
v5->v6:
 * simply the driver a lot by re-orgnizing the driver into a few clock types:
   scu clock (merge scu divider/gate/mux) and scu gpr lock which accessing is
   through SCU protocol and LPCG clock which is directly accessible by CPU.
 * LPCG clock is separate from SCU clock, gpr clock is still not used
   and will be added later.
 * remove old year license as the code is totally rewritten
 * scu mux support will be added later as it's also still not used.
v4->v5:
 Address all Stephen and Sascha's review comments, see details in each patch
v3->v4:
 * scu headfile path update
 * no functionality change
v2->v3:
 * structures/enums name update with imx_sc prefix
 * no functionality change
v1->v2:
 * structure and enums name update
 * api usage update due to api change
 * no functionality change

Dong Aisheng (7):
  clk: imx: add configuration option for mmio clks
  clk: imx: add scu clock common part
  dt-bindings: clock: imx8qxp: add SCU clock IDs
  clk: imx: add imx8qxp clk driver
  dt-bindings: clock: add imx8qxp lpcg clock binding
  clk: imx: add lpcg clock support
  clk: imx: add imx8qxp lpcg driver

 .../devicetree/bindings/clock/imx8qxp-lpcg.txt     |  51 ++++
 arch/arm/mach-imx/Kconfig                          |  11 +
 drivers/clk/Kconfig                                |   1 +
 drivers/clk/imx/Kconfig                            |   9 +
 drivers/clk/imx/Makefile                           |   8 +-
 drivers/clk/imx/clk-imx8qxp-lpcg.c                 | 216 +++++++++++++++
 drivers/clk/imx/clk-imx8qxp-lpcg.h                 | 102 ++++++++
 drivers/clk/imx/clk-imx8qxp.c                      | 153 +++++++++++
 drivers/clk/imx/clk-lpcg-scu.c                     | 114 ++++++++
 drivers/clk/imx/clk-scu.c                          | 265 +++++++++++++++++++
 drivers/clk/imx/clk-scu.h                          |  24 ++
 include/dt-bindings/clock/imx8qxp-clock.h          | 289 +++++++++++++++++++++
 12 files changed, 1242 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
 create mode 100644 drivers/clk/imx/Kconfig
 create mode 100644 drivers/clk/imx/clk-imx8qxp-lpcg.c
 create mode 100644 drivers/clk/imx/clk-imx8qxp-lpcg.h
 create mode 100644 drivers/clk/imx/clk-imx8qxp.c
 create mode 100644 drivers/clk/imx/clk-lpcg-scu.c
 create mode 100644 drivers/clk/imx/clk-scu.c
 create mode 100644 drivers/clk/imx/clk-scu.h
 create mode 100644 include/dt-bindings/clock/imx8qxp-clock.h

Quoting Aisheng DONG (2018-12-04 06:39:18)
> This patch series adds i.MX8QXP clock support which is based
> on the clock service provided by SCU firmware.
> 
> Note: It depends on SCU driver which has already been merged by Shawn.
> So this patch series could go through Shawn's tree as well.

Which patch series?

I'm having trouble applying the first patch, so I suspect something is
wrong.

Quoting Aisheng DONG (2018-12-04 06:39:25)
> diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
> index 43a3ecc..63e7b01 100644
> --- a/drivers/clk/imx/Kconfig
> +++ b/drivers/clk/imx/Kconfig
> @@ -3,3 +3,7 @@
>  config MXC_CLK
>         bool
>         depends on ARCH_MXC
> +
> +config MXC_CLK_SCU

Is there any reason to make this a hidden option instead of making it a
selectable option? It can still depend on ARCH_MXC and ARM64, but
otherwise it should be compilable as long as CONFIG_IMX_SCU is defined
(this should also be a config we depend on here).

> +       bool
> +       depends on ARCH_MXC && ARM64
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index f850424..4abed37 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -20,6 +20,9 @@ obj-$(CONFIG_MXC_CLK) += \
>         clk-pllv4.o \
>         clk-sccg-pll.o
>  
> +obj-$(CONFIG_MXC_CLK_SCU) += \
> +       clk-scu.o
> +
>  obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
>  obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
>  obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o
> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> new file mode 100644
> index 0000000..ec8a471
> --- /dev/null
> +++ b/drivers/clk/imx/clk-scu.c
> @@ -0,0 +1,265 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018 NXP
> + *   Dong Aisheng <aisheng.dong@nxp.com>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +
> +#include "clk-scu.h"
> +
> +struct imx_sc_ipc *ccm_ipc_handle;

Why does this need to be a global? Can it be in each clk_scu instance
instead?

> +
> diff --git a/drivers/clk/imx/clk-scu.h b/drivers/clk/imx/clk-scu.h
> new file mode 100644
> index 0000000..09f381b
> --- /dev/null
> +++ b/drivers/clk/imx/clk-scu.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2018 NXP
> + *   Dong Aisheng <aisheng.dong@nxp.com>
> + */
> +
> +#ifndef __IMX_CLK_SCU_H
> +#define __IMX_CLK_SCU_H
> +
> +#include <linux/firmware/imx/sci.h>
> +
> +extern struct imx_sc_ipc *ccm_ipc_handle;
> +
> +static inline int imx_clk_scu_init(void)
> +{
> +       return imx_scu_get_handle(&ccm_ipc_handle);

And then this can be implemented in the C driver so that ccm_ipc_handle
doesn't need to be exported into the header file?

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Sent: Tuesday, December 11, 2018 4:47 AM
> To: linux-clk@vger.kernel.org; Aisheng Dong <aisheng.dong@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org; mturquette@baylibre.com;
> shawnguo@kernel.org; Fabio Estevam <fabio.estevam@nxp.com>; dl-linux-imx
> <linux-imx@nxp.com>; kernel@pengutronix.de; Aisheng Dong
> <aisheng.dong@nxp.com>
> Subject: Re: [PATCH V9 0/7] clk: imx: add imx8qxp clock support
> 
> Quoting Aisheng DONG (2018-12-04 06:39:18)
> > This patch series adds i.MX8QXP clock support which is based on the
> > clock service provided by SCU firmware.
> >
> > Note: It depends on SCU driver which has already been merged by Shawn.
> > So this patch series could go through Shawn's tree as well.
> 
> Which patch series?
> 
> I'm having trouble applying the first patch, so I suspect something is wrong.

Which branch would you like me to make the patch against?
I made the patch against clk-next branch and just re-tested it was ok.

BTW, it depends on the following SCU firmware patch which already in 4.20-RC
In your tree. So it's safe to apply.

commit edbee095fafb4b727b51032bdc41e345f95bbc20
Author: Dong Aisheng <aisheng.dong@nxp.com>
Date:   Sun Oct 7 21:04:42 2018 +0800

    firmware: imx: add SCU firmware driver support

    The System Controller Firmware (SCFW) is a low-level system function
    which runs on a dedicated Cortex-M core to provide power, clock, and
    resource management. It exists on some i.MX8 processors. e.g. i.MX8QM
    (QM, QP), and i.MX8QX (QXP, DX).

    This patch implements the SCU firmware IPC function and the common
    message sending API sc_call_rpc.

    Cc: Shawn Guo <shawnguo@kernel.org>
    Cc: Fabio Estevam <fabio.estevam@nxp.com>
    Cc: Jassi Brar <jassisinghbrar@gmail.com>
    Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
    Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
    Signed-off-by: Shawn Guo <shawnguo@kernel.org>

Regards
Dong Aisheng

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Quoting Aisheng DONG (2018-12-04 06:39:25)
> > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index
> > 43a3ecc..63e7b01 100644
> > --- a/drivers/clk/imx/Kconfig
> > +++ b/drivers/clk/imx/Kconfig
> > @@ -3,3 +3,7 @@
> >  config MXC_CLK
> >         bool
> >         depends on ARCH_MXC
> > +
> > +config MXC_CLK_SCU
> 
> Is there any reason to make this a hidden option instead of making it a
> selectable option? It can still depend on ARCH_MXC and ARM64, but otherwise
> it should be compilable as long as CONFIG_IMX_SCU is defined (this should
> also be a config we depend on here).
> 

This is mostly following the exist using that CLK is selected by SoC config option.
https://patchwork.kernel.org/patch/10677309/

As CLK usually is required for platform to run well, so we did not make it selectable.

> > +       bool
> > +       depends on ARCH_MXC && ARM64
> > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index
> > f850424..4abed37 100644
> > --- a/drivers/clk/imx/Makefile
> > +++ b/drivers/clk/imx/Makefile
> > @@ -20,6 +20,9 @@ obj-$(CONFIG_MXC_CLK) += \
> >         clk-pllv4.o \
> >         clk-sccg-pll.o
> >
> > +obj-$(CONFIG_MXC_CLK_SCU) += \
> > +       clk-scu.o
> > +
> >  obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> >  obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
> >  obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o diff --git
> > a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c new file mode
> > 100644 index 0000000..ec8a471
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-scu.c
> > @@ -0,0 +1,265 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2018 NXP
> > + *   Dong Aisheng <aisheng.dong@nxp.com>
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +
> > +#include "clk-scu.h"
> > +
> > +struct imx_sc_ipc *ccm_ipc_handle;
> 
> Why does this need to be a global? Can it be in each clk_scu instance instead?
> 

No, no need to be in each clk_scu instance.
There's only one handler.

> > +
> > diff --git a/drivers/clk/imx/clk-scu.h b/drivers/clk/imx/clk-scu.h new
> > file mode 100644 index 0000000..09f381b
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-scu.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2018 NXP
> > + *   Dong Aisheng <aisheng.dong@nxp.com>
> > + */
> > +
> > +#ifndef __IMX_CLK_SCU_H
> > +#define __IMX_CLK_SCU_H
> > +
> > +#include <linux/firmware/imx/sci.h>
> > +
> > +extern struct imx_sc_ipc *ccm_ipc_handle;
> > +
> > +static inline int imx_clk_scu_init(void) {
> > +       return imx_scu_get_handle(&ccm_ipc_handle);
> 
> And then this can be implemented in the C driver so that ccm_ipc_handle
> doesn't need to be exported into the header file?

Looks like a good suggestion.
I will take this as there's no other users of ccm_ipc_handle now, so no need in headfile.

Thanks

Regards
Dong Aisheng

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Quoting Aisheng DONG (2018-12-04 06:39:18)
> > This patch series adds i.MX8QXP clock support which is based on the
> > clock service provided by SCU firmware.
> >
> > Note: It depends on SCU driver which has already been merged by Shawn.
> > So this patch series could go through Shawn's tree as well.
> 
> Which patch series?
> 
> I'm having trouble applying the first patch, so I suspect something is wrong.

As I replied in last mail, patches were made against clk-next branch.
I also tried apply against clk-master branch and met the same failure.

$ git am *.patch
Patch is empty. Was it split wrong?
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
$ git am --skip
Applying: clk: imx: add configuration option for mmio clks
error: patch failed: drivers/clk/imx/Makefile:1
error: drivers/clk/imx/Makefile: patch does not apply
Patch failed at 0002 clk: imx: add configuration option for mmio clks
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

If manually apply, it can work.
$ patch -p1 < 0001-clk-imx-add-configuration-option-for-mmio-clks.patch
patching file arch/arm/mach-imx/Kconfig
Hunk #7 succeeded at 571 (offset -9 lines).
patching file drivers/clk/Kconfig
patching file drivers/clk/imx/Kconfig
patching file drivers/clk/imx/Makefile
Hunk #1 succeeded at 1 with fuzz 1.

I guess it might because clk-next merged clk-imx7ulp.c and clk-imx8mq.c which
Changed the drivers/clk/imx/Makefile a bit.

If you want me to generate the patch set against clk-master, please let me know.

Regards
Dong Aisheng

Hi,

On Tue, 11 Dec 2018 03:35:48 +0000 Aisheng Dong wrote:
> > -----Original Message-----
> > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > Quoting Aisheng DONG (2018-12-04 06:39:25)  
> > > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index
> > > 43a3ecc..63e7b01 100644
> > > --- a/drivers/clk/imx/Kconfig
> > > +++ b/drivers/clk/imx/Kconfig
> > > @@ -3,3 +3,7 @@
> > >  config MXC_CLK
> > >         bool
> > >         depends on ARCH_MXC
> > > +
> > > +config MXC_CLK_SCU  
> > 
> > Is there any reason to make this a hidden option instead of making it a
> > selectable option? It can still depend on ARCH_MXC and ARM64, but otherwise
> > it should be compilable as long as CONFIG_IMX_SCU is defined (this should
> > also be a config we depend on here).
> >   
> 
> This is mostly following the exist using that CLK is selected by SoC config option.
> https://patchwork.kernel.org/patch/10677309/
> 
> As CLK usually is required for platform to run well, so we did not make it selectable.
> 
> > > +       bool
> > > +       depends on ARCH_MXC && ARM64
> > > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index
> > > f850424..4abed37 100644
> > > --- a/drivers/clk/imx/Makefile
> > > +++ b/drivers/clk/imx/Makefile
> > > @@ -20,6 +20,9 @@ obj-$(CONFIG_MXC_CLK) += \
> > >         clk-pllv4.o \
> > >         clk-sccg-pll.o
> > >
> > > +obj-$(CONFIG_MXC_CLK_SCU) += \
> > > +       clk-scu.o
> > > +
> > >  obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> > >  obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
> > >  obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o diff --git
> > > a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c new file mode
> > > 100644 index 0000000..ec8a471
> > > --- /dev/null
> > > +++ b/drivers/clk/imx/clk-scu.c
> > > @@ -0,0 +1,265 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2018 NXP
> > > + *   Dong Aisheng <aisheng.dong@nxp.com>
> > > + */
> > > +
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/err.h>
> > > +#include <linux/slab.h>
> > > +
> > > +#include "clk-scu.h"
> > > +
> > > +struct imx_sc_ipc *ccm_ipc_handle;  
> > 
> > Why does this need to be a global? Can it be in each clk_scu instance instead?
> >   
> 
> No, no need to be in each clk_scu instance.
> There's only one handler.
> 
Shouldn't it be 'static'?


Lothar Waßmann

[...]
> > > > +#include "clk-scu.h"
> > > > +
> > > > +struct imx_sc_ipc *ccm_ipc_handle;
> > >
> > > Why does this need to be a global? Can it be in each clk_scu instance
> instead?
> > >
> >
> > No, no need to be in each clk_scu instance.
> > There's only one handler.
> >
> Shouldn't it be 'static'?
> 

This has been fixed in CLK tree.

Regards
Dong Aisheng

> 
> Lothar Waßmann