diff mbox series

[U-Boot,07/10] clk: imx8: add i.MX8QM clk driver

Message ID 20181224041309.15225-7-peng.fan@nxp.com
State Superseded
Delegated to: Stefano Babic
Headers show
Series [U-Boot,01/10] pinctrl: imx8: add i.MX8QM compatible | expand

Commit Message

Peng Fan Dec. 24, 2018, 4:04 a.m. UTC
Add i.MX8QM clk driver, SDHC/FEC/UART/I2C supported.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/imx/Makefile     |   1 +
 drivers/clk/imx/clk-imx8.c   |   1 +
 drivers/clk/imx/clk-imx8qm.c | 307 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 309 insertions(+)
 create mode 100644 drivers/clk/imx/clk-imx8qm.c

Comments

Stefano Babic Jan. 28, 2019, 2:04 p.m. UTC | #1
Hi Peng,

On 24/12/18 05:04, Peng Fan wrote:
> Add i.MX8QM clk driver, SDHC/FEC/UART/I2C supported.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/clk/imx/Makefile     |   1 +
>  drivers/clk/imx/clk-imx8.c   |   1 +
>  drivers/clk/imx/clk-imx8qm.c | 307 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 309 insertions(+)
>  create mode 100644 drivers/clk/imx/clk-imx8qm.c
> 
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index d07d91b88f..eb379c188a 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -6,4 +6,5 @@ obj-$(CONFIG_CLK_IMX8) += clk-imx8.o
>  
>  ifdef CONFIG_CLK_IMX8
>  obj-$(CONFIG_IMX8QXP) += clk-imx8qxp.o
> +obj-$(CONFIG_IMX8QM) += clk-imx8qm.o
>  endif
> diff --git a/drivers/clk/imx/clk-imx8.c b/drivers/clk/imx/clk-imx8.c
> index c69a9ed867..a755e26501 100644
> --- a/drivers/clk/imx/clk-imx8.c
> +++ b/drivers/clk/imx/clk-imx8.c
> @@ -101,6 +101,7 @@ static int imx8_clk_probe(struct udevice *dev)
>  
>  static const struct udevice_id imx8_clk_ids[] = {
>  	{ .compatible = "fsl,imx8qxp-clk" },
> +	{ .compatible = "fsl,imx8qm-clk" },
>  	{ },
>  };
>  
> diff --git a/drivers/clk/imx/clk-imx8qm.c b/drivers/clk/imx/clk-imx8qm.c
> new file mode 100644
> index 0000000000..6b5561e178
> --- /dev/null
> +++ b/drivers/clk/imx/clk-imx8qm.c
> @@ -0,0 +1,307 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018 NXP
> + * Peng Fan <peng.fan@nxp.com>
> + */
> +
> +#include <common.h>
> +#include <clk-uclass.h>
> +#include <dm.h>
> +#include <asm/arch/sci/sci.h>
> +#include <asm/arch/clock.h>
> +#include <dt-bindings/clock/imx8qm-clock.h>
> +#include <dt-bindings/soc/imx_rsrc.h>
> +#include <misc.h>
> +
> +#include "clk-imx8.h"
> +
> +#if CONFIG_IS_ENABLED(CMD_CLK)
> +struct imx8_clks imx8_clk_names[] = {
> +	{ IMX8QM_A53_DIV, "A53_DIV" },
> +	{ IMX8QM_UART0_CLK, "UART0" },
> +	{ IMX8QM_UART1_CLK, "UART1" },
> +	{ IMX8QM_UART2_CLK, "UART2" },
> +	{ IMX8QM_UART3_CLK, "UART3" },
> +	{ IMX8QM_SDHC0_CLK, "SDHC0" },
> +	{ IMX8QM_SDHC1_CLK, "SDHC1" },
> +	{ IMX8QM_SDHC2_CLK, "SDHC2" },
> +	{ IMX8QM_ENET0_AHB_CLK, "ENET0_AHB" },
> +	{ IMX8QM_ENET0_IPG_CLK, "ENET0_IPG" },
> +	{ IMX8QM_ENET0_REF_DIV, "ENET0_REF" },
> +	{ IMX8QM_ENET0_PTP_CLK, "ENET0_PTP" },
> +	{ IMX8QM_ENET1_AHB_CLK, "ENET1_AHB" },
> +	{ IMX8QM_ENET1_IPG_CLK, "ENET1_IPG" },
> +	{ IMX8QM_ENET1_REF_DIV, "ENET1_REF" },
> +	{ IMX8QM_ENET1_PTP_CLK, "ENET1_PTP" },
> +};
> +
> +int num_clks = ARRAY_SIZE(imx8_clk_names);
> +#endif
> +
> +ulong imx8_clk_get_rate(struct clk *clk)
> +{
> +	sc_pm_clk_t pm_clk;
> +	ulong rate;
> +	u16 resource;
> +	int ret;
> +
> +	debug("%s(#%lu)\n", __func__, clk->id);
> +
> +	switch (clk->id) {
> +	case IMX8QM_A53_DIV:
> +		resource = SC_R_A53;
> +		pm_clk = SC_PM_CLK_CPU;
> +		break;
> +	case IMX8QM_I2C0_CLK:
> +		resource = SC_R_I2C_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_I2C1_CLK:
> +		resource = SC_R_I2C_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_I2C2_CLK:
> +		resource = SC_R_I2C_2;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_I2C3_CLK:
> +		resource = SC_R_I2C_3;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_SDHC0_IPG_CLK:
> +	case IMX8QM_SDHC0_CLK:
> +	case IMX8QM_SDHC0_DIV:
> +		resource = SC_R_SDHC_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_SDHC1_IPG_CLK:
> +	case IMX8QM_SDHC1_CLK:
> +	case IMX8QM_SDHC1_DIV:
> +		resource = SC_R_SDHC_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART0_IPG_CLK:
> +	case IMX8QM_UART0_CLK:
> +		resource = SC_R_UART_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART1_CLK:
> +		resource = SC_R_UART_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART2_CLK:
> +		resource = SC_R_UART_2;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART3_CLK:
> +		resource = SC_R_UART_3;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_ENET0_IPG_CLK:
> +	case IMX8QM_ENET0_AHB_CLK:
> +	case IMX8QM_ENET0_REF_DIV:
> +	case IMX8QM_ENET0_PTP_CLK:
> +		resource = SC_R_ENET_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_ENET1_IPG_CLK:
> +	case IMX8QM_ENET1_AHB_CLK:
> +	case IMX8QM_ENET1_REF_DIV:
> +	case IMX8QM_ENET1_PTP_CLK:
> +		resource = SC_R_ENET_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	default:
> +		if (clk->id < IMX8QM_UART0_IPG_CLK ||
> +		    clk->id >= IMX8QM_CLK_END) {
> +			printf("%s(Invalid clk ID #%lu)\n",
> +			       __func__, clk->id);
> +			return -EINVAL;
> +		}
> +		return -ENOTSUPP;
> +	};
> +
> +	ret = sc_pm_get_clock_rate(-1, resource, pm_clk,
> +				   (sc_pm_clock_rate_t *)&rate);
> +	if (ret) {
> +		printf("%s err %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	return rate;
> +}
> +
> +ulong imx8_clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	sc_pm_clk_t pm_clk;
> +	u32 new_rate = rate;
> +	u16 resource;
> +	int ret;
> +
> +	debug("%s(#%lu), rate: %lu\n", __func__, clk->id, rate);
> +
> +	switch (clk->id) {
> +	case IMX8QM_I2C0_CLK:
> +		resource = SC_R_I2C_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_I2C1_CLK:
> +		resource = SC_R_I2C_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_I2C2_CLK:
> +		resource = SC_R_I2C_2;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_I2C3_CLK:
> +		resource = SC_R_I2C_3;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART0_CLK:
> +		resource = SC_R_UART_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART1_CLK:
> +		resource = SC_R_UART_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART2_CLK:
> +		resource = SC_R_UART_2;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART3_CLK:
> +		resource = SC_R_UART_3;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_SDHC0_IPG_CLK:
> +	case IMX8QM_SDHC0_CLK:
> +	case IMX8QM_SDHC0_DIV:
> +		resource = SC_R_SDHC_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_SDHC1_IPG_CLK:
> +	case IMX8QM_SDHC1_CLK:
> +	case IMX8QM_SDHC1_DIV:
> +		resource = SC_R_SDHC_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_ENET0_IPG_CLK:
> +	case IMX8QM_ENET0_AHB_CLK:
> +	case IMX8QM_ENET0_REF_DIV:
> +	case IMX8QM_ENET0_PTP_CLK:
> +	case IMX8QM_ENET0_ROOT_DIV:
> +		resource = SC_R_ENET_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_ENET1_IPG_CLK:
> +	case IMX8QM_ENET1_AHB_CLK:
> +	case IMX8QM_ENET1_REF_DIV:
> +	case IMX8QM_ENET1_PTP_CLK:
> +	case IMX8QM_ENET1_ROOT_DIV:
> +		resource = SC_R_ENET_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	default:
> +		if (clk->id < IMX8QM_UART0_IPG_CLK ||
> +		    clk->id >= IMX8QM_CLK_END) {
> +			printf("%s(Invalid clk ID #%lu)\n",
> +			       __func__, clk->id);
> +			return -EINVAL;
> +		}
> +		return -ENOTSUPP;
> +	};
> +
> +	ret = sc_pm_set_clock_rate(-1, resource, pm_clk, &new_rate);
> +	if (ret) {
> +		printf("%s err %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	return new_rate;
> +}
> +
> +int __imx8_clk_enable(struct clk *clk, bool enable)
> +{
> +	sc_pm_clk_t pm_clk;
> +	u16 resource;
> +	int ret;
> +
> +	debug("%s(#%lu)\n", __func__, clk->id);
> +
> +	switch (clk->id) {
> +	case IMX8QM_I2C0_CLK:
> +		resource = SC_R_I2C_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_I2C1_CLK:
> +		resource = SC_R_I2C_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_I2C2_CLK:
> +		resource = SC_R_I2C_2;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_I2C3_CLK:
> +		resource = SC_R_I2C_3;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART0_CLK:
> +		resource = SC_R_UART_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART1_CLK:
> +		resource = SC_R_UART_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART2_CLK:
> +		resource = SC_R_UART_2;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_UART3_CLK:
> +		resource = SC_R_UART_3;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_SDHC0_IPG_CLK:
> +	case IMX8QM_SDHC0_CLK:
> +	case IMX8QM_SDHC0_DIV:
> +		resource = SC_R_SDHC_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_SDHC1_IPG_CLK:
> +	case IMX8QM_SDHC1_CLK:
> +	case IMX8QM_SDHC1_DIV:
> +		resource = SC_R_SDHC_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_ENET0_IPG_CLK:
> +	case IMX8QM_ENET0_AHB_CLK:
> +	case IMX8QM_ENET0_REF_DIV:
> +	case IMX8QM_ENET0_PTP_CLK:
> +		resource = SC_R_ENET_0;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	case IMX8QM_ENET1_IPG_CLK:
> +	case IMX8QM_ENET1_AHB_CLK:
> +	case IMX8QM_ENET1_REF_DIV:
> +	case IMX8QM_ENET1_PTP_CLK:
> +		resource = SC_R_ENET_1;
> +		pm_clk = SC_PM_CLK_PER;
> +		break;
> +	default:
> +		if (clk->id < IMX8QM_UART0_IPG_CLK ||
> +		    clk->id >= IMX8QM_CLK_END) {
> +			printf("%s(Invalid clk ID #%lu)\n",
> +			       __func__, clk->id);
> +			return -EINVAL;
> +		}
> +		return -ENOTSUPP;
> +	}
> +
> +	ret = sc_pm_clock_enable(-1, resource, pm_clk, enable, 0);
> +	if (ret) {
> +		printf("%s err %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> 

Ok - I see you decide to split the code on depend of the SOC type.
Nevertheless, even if defines are different, it seems to me that there
is still a lot of common code (_imx8_clk_enable is very similar). Do we
need really a separate file for each MX8 variant ? Which are the
blocking points that avoid to do this ? I am looking at the patch, but I
do not find such as issue. Can you explain it ?

Regards,
Stefano
Fabio Estevam Jan. 28, 2019, 2:23 p.m. UTC | #2
Hi Stefano,

On Mon, Jan 28, 2019 at 12:04 PM Stefano Babic <sbabic@denx.de> wrote:

> Ok - I see you decide to split the code on depend of the SOC type.
> Nevertheless, even if defines are different, it seems to me that there
> is still a lot of common code (_imx8_clk_enable is very similar). Do we
> need really a separate file for each MX8 variant ? Which are the
> blocking points that avoid to do this ? I am looking at the patch, but I
> do not find such as issue. Can you explain it ?

I am also concerned about the current i.MX clock driver status in
U-Boot and I expressed this during my review of Lukasz's patch that
added a imx6 clock driver.

It does not scale well as we can see by the huge switch statement in
this patch. Of course this would need to grow when new peripherals
clocks are added.

I think a better approach would be to use the same scheme as done in
the kernel with common clock framework.

Barebox folows this same approach. For example:

i.MX8M clock driver:
https://git.pengutronix.de/cgit/barebox/tree/drivers/clk/imx/clk-imx8mq.c

i.MX6Q clock driver:
https://git.pengutronix.de/cgit/barebox/tree/drivers/clk/imx/clk-imx6.c

It allows syncing with the kernel clock driver more easily and it
makes it easier to maintain several i.MX SoCs in the long term.
Stefano Babic Jan. 28, 2019, 2:35 p.m. UTC | #3
Hi Fabio,

On 28/01/19 15:23, Fabio Estevam wrote:
> Hi Stefano,
> 
> On Mon, Jan 28, 2019 at 12:04 PM Stefano Babic <sbabic@denx.de> wrote:
> 
>> Ok - I see you decide to split the code on depend of the SOC type.
>> Nevertheless, even if defines are different, it seems to me that there
>> is still a lot of common code (_imx8_clk_enable is very similar). Do we
>> need really a separate file for each MX8 variant ? Which are the
>> blocking points that avoid to do this ? I am looking at the patch, but I
>> do not find such as issue. Can you explain it ?
> 
> I am also concerned about the current i.MX clock driver status in
> U-Boot and I expressed this during my review of Lukasz's patch that
> added a imx6 clock driver.
> 

Nice we are sharing the sa me opinion. I am currently checking all
patches from oldest to neweset, I have not yet catched Lukasz', but they
are on my sight, too.

> It does not scale well as we can see by the huge switch statement in
> this patch. Of course this would need to grow when new peripherals
> clocks are added.

I fully agree. Yes, it is simple now to do in this way, but it does not
scale very well.

> 
> I think a better approach would be to use the same scheme as done in
> the kernel with common clock framework.
> 
> Barebox folows this same approach. For example:
> 
> i.MX8M clock driver:
> https://git.pengutronix.de/cgit/barebox/tree/drivers/clk/imx/clk-imx8mq.c
> 
> i.MX6Q clock driver:
> https://git.pengutronix.de/cgit/barebox/tree/drivers/clk/imx/clk-imx6.c
> 
> It allows syncing with the kernel clock driver more easily 

This is also a great benefit.

> and it
> makes it easier to maintain several i.MX SoCs in the long term.
> 

Best regards,
Stefano
Peng Fan Jan. 29, 2019, 1:14 a.m. UTC | #4
Hi Stefano

> > +
> >
> 
> Ok - I see you decide to split the code on depend of the SOC type.
> Nevertheless, even if defines are different, it seems to me that there is still a
> lot of common code (_imx8_clk_enable is very similar). Do we need really a
> separate file for each MX8 variant ? Which are the blocking points that avoid
> to do this ? I am looking at the patch, but I do not find such as issue. Can you
> explain it ?

I do not want to mix i.MX8QXP and QM currently, although it looks most same logic except the macros.
Linux side code still not merged by upstream, there will be some change there I think. Then I think
uboot side code also need some restructure following Linux change.
Another one is there are many variants, not only QXP/QM, add all the macros in one file will make
it not elegant and end up of more and more ifdef in one file.

Thanks,
Peng.

> 
> Regards,
> Stefano
> 
> 
> --
> ==============================================================
> =======
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> ==============================================================
> =======
Peng Fan Jan. 29, 2019, 1:21 a.m. UTC | #5
> -----Original Message-----
> From: Fabio Estevam [mailto:festevam@gmail.com]
> Sent: 2019年1月28日 22:24
> To: Stefano Babic <sbabic@denx.de>
> Cc: Peng Fan <peng.fan@nxp.com>; Fabio Estevam
> <fabio.estevam@nxp.com>; u-boot@lists.denx.de; dl-uboot-imx
> <uboot-imx@nxp.com>; Lukasz Majewski <lukma@denx.de>
> Subject: Re: [U-Boot] [PATCH 07/10] clk: imx8: add i.MX8QM clk driver
> 
> Hi Stefano,
> 
> On Mon, Jan 28, 2019 at 12:04 PM Stefano Babic <sbabic@denx.de> wrote:
> 
> > Ok - I see you decide to split the code on depend of the SOC type.
> > Nevertheless, even if defines are different, it seems to me that there
> > is still a lot of common code (_imx8_clk_enable is very similar). Do
> > we need really a separate file for each MX8 variant ? Which are the
> > blocking points that avoid to do this ? I am looking at the patch, but
> > I do not find such as issue. Can you explain it ?
> 
> I am also concerned about the current i.MX clock driver status in U-Boot and I
> expressed this during my review of Lukasz's patch that added a imx6 clock
> driver.
> 
> It does not scale well as we can see by the huge switch statement in this
> patch. Of course this would need to grow when new peripherals clocks are
> added.
> 
> I think a better approach would be to use the same scheme as done in the
> kernel with common clock framework.
> 
> Barebox folows this same approach. For example:
> 
> i.MX8M clock driver:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.p
> engutronix.de%2Fcgit%2Fbarebox%2Ftree%2Fdrivers%2Fclk%2Fimx%2Fclk-i
> mx8mq.c&amp;data=02%7C01%7Cpeng.fan%40nxp.com%7C124a7ae228794
> 2551ea208d6852c3c0a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C636842822364000869&amp;sdata=6xwmlVrr2MGKkRCNUFZTBcec9KP
> RNoID%2FBWTU5HFKcg%3D&amp;reserved=0
> 
> i.MX6Q clock driver:
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.p
> engutronix.de%2Fcgit%2Fbarebox%2Ftree%2Fdrivers%2Fclk%2Fimx%2Fclk-i
> mx6.c&amp;data=02%7C01%7Cpeng.fan%40nxp.com%7C124a7ae22879425
> 51ea208d6852c3c0a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C636842822364000869&amp;sdata=m6%2FRa8HY9M1aPXy45%2Bp0aa9Xit
> 6cMUVqBw%2F4UVTSOz0%3D&amp;reserved=0
> 
> It allows syncing with the kernel clock driver more easily and it makes it
> easier to maintain several i.MX SoCs in the long term.

I thought about this before, but this will increase SPL code side a lot when
SPL DM clk used.

Regards,
Peng.
Peng Fan Feb. 24, 2019, 5:53 a.m. UTC | #6
Hi Stefano

> -----Original Message-----
> From: Peng Fan
> Sent: 2019年1月29日 9:15
> To: 'Stefano Babic' <sbabic@denx.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>; dl-uboot-imx
> <uboot-imx@nxp.com>; u-boot@lists.denx.de
> Subject: RE: [PATCH 07/10] clk: imx8: add i.MX8QM clk driver
> 
> Hi Stefano
> 
> > > +
> > >
> >
> > Ok - I see you decide to split the code on depend of the SOC type.
> > Nevertheless, even if defines are different, it seems to me that there
> > is still a lot of common code (_imx8_clk_enable is very similar). Do
> > we need really a separate file for each MX8 variant ? Which are the
> > blocking points that avoid to do this ? I am looking at the patch, but
> > I do not find such as issue. Can you explain it ?
> 
> I do not want to mix i.MX8QXP and QM currently, although it looks most same
> logic except the macros.
> Linux side code still not merged by upstream, there will be some change there
> I think. Then I think uboot side code also need some restructure following
> Linux change.
> Another one is there are many variants, not only QXP/QM, add all the macros
> in one file will make it not elegant and end up of more and more ifdef in one
> file.

I just have time and go through my patches again, because we use clk id from imx8qxp-clock.h
and imx8qm-clock.h and some ids has same value but with different name, saying
QM/QXP. If we use the QM/QXP macros in clk-imx8.c with case, such as:
        case IMX8QM_UART0_IPG_CLK:
        case IMX8QM_UART0_CLK:
        case IMX8QXP_UART0_IPG_CLK:
        case IMX8QXP_UART0_CLK:
                resource = SC_R_UART_0;
                pm_clk = SC_PM_CLK_PER;
                break;

gcc will complains error:
drivers/clk/imx/clk-imx8.c: In function ‘imx8_clk_get_rate’:
drivers/clk/imx/clk-imx8.c:124:2: error: duplicate case value
  case IMX8QXP_UART0_IPG_CLK:
  ^~~~

Because some QM/QXP macros has same value currently.

However we could not let the imx8qxp-clock.h and imx8qm-clock.h be mixed
into one header file, we reuse Linux headers.

So I need to have clk-imx8qm.c and clk-imx8qxp.c separately.

Do you have more concerns about the QM patch set?

Thanks,
Peng.

> 
> Thanks,
> Peng.
> 
> >
> > Regards,
> > Stefano
> >
> >
> > --
> >
> ==============================================================
> > =======
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> Denk
> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email:
> sbabic@denx.de
> >
> ==============================================================
> > =======
diff mbox series

Patch

diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
index d07d91b88f..eb379c188a 100644
--- a/drivers/clk/imx/Makefile
+++ b/drivers/clk/imx/Makefile
@@ -6,4 +6,5 @@  obj-$(CONFIG_CLK_IMX8) += clk-imx8.o
 
 ifdef CONFIG_CLK_IMX8
 obj-$(CONFIG_IMX8QXP) += clk-imx8qxp.o
+obj-$(CONFIG_IMX8QM) += clk-imx8qm.o
 endif
diff --git a/drivers/clk/imx/clk-imx8.c b/drivers/clk/imx/clk-imx8.c
index c69a9ed867..a755e26501 100644
--- a/drivers/clk/imx/clk-imx8.c
+++ b/drivers/clk/imx/clk-imx8.c
@@ -101,6 +101,7 @@  static int imx8_clk_probe(struct udevice *dev)
 
 static const struct udevice_id imx8_clk_ids[] = {
 	{ .compatible = "fsl,imx8qxp-clk" },
+	{ .compatible = "fsl,imx8qm-clk" },
 	{ },
 };
 
diff --git a/drivers/clk/imx/clk-imx8qm.c b/drivers/clk/imx/clk-imx8qm.c
new file mode 100644
index 0000000000..6b5561e178
--- /dev/null
+++ b/drivers/clk/imx/clk-imx8qm.c
@@ -0,0 +1,307 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2018 NXP
+ * Peng Fan <peng.fan@nxp.com>
+ */
+
+#include <common.h>
+#include <clk-uclass.h>
+#include <dm.h>
+#include <asm/arch/sci/sci.h>
+#include <asm/arch/clock.h>
+#include <dt-bindings/clock/imx8qm-clock.h>
+#include <dt-bindings/soc/imx_rsrc.h>
+#include <misc.h>
+
+#include "clk-imx8.h"
+
+#if CONFIG_IS_ENABLED(CMD_CLK)
+struct imx8_clks imx8_clk_names[] = {
+	{ IMX8QM_A53_DIV, "A53_DIV" },
+	{ IMX8QM_UART0_CLK, "UART0" },
+	{ IMX8QM_UART1_CLK, "UART1" },
+	{ IMX8QM_UART2_CLK, "UART2" },
+	{ IMX8QM_UART3_CLK, "UART3" },
+	{ IMX8QM_SDHC0_CLK, "SDHC0" },
+	{ IMX8QM_SDHC1_CLK, "SDHC1" },
+	{ IMX8QM_SDHC2_CLK, "SDHC2" },
+	{ IMX8QM_ENET0_AHB_CLK, "ENET0_AHB" },
+	{ IMX8QM_ENET0_IPG_CLK, "ENET0_IPG" },
+	{ IMX8QM_ENET0_REF_DIV, "ENET0_REF" },
+	{ IMX8QM_ENET0_PTP_CLK, "ENET0_PTP" },
+	{ IMX8QM_ENET1_AHB_CLK, "ENET1_AHB" },
+	{ IMX8QM_ENET1_IPG_CLK, "ENET1_IPG" },
+	{ IMX8QM_ENET1_REF_DIV, "ENET1_REF" },
+	{ IMX8QM_ENET1_PTP_CLK, "ENET1_PTP" },
+};
+
+int num_clks = ARRAY_SIZE(imx8_clk_names);
+#endif
+
+ulong imx8_clk_get_rate(struct clk *clk)
+{
+	sc_pm_clk_t pm_clk;
+	ulong rate;
+	u16 resource;
+	int ret;
+
+	debug("%s(#%lu)\n", __func__, clk->id);
+
+	switch (clk->id) {
+	case IMX8QM_A53_DIV:
+		resource = SC_R_A53;
+		pm_clk = SC_PM_CLK_CPU;
+		break;
+	case IMX8QM_I2C0_CLK:
+		resource = SC_R_I2C_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_I2C1_CLK:
+		resource = SC_R_I2C_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_I2C2_CLK:
+		resource = SC_R_I2C_2;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_I2C3_CLK:
+		resource = SC_R_I2C_3;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_SDHC0_IPG_CLK:
+	case IMX8QM_SDHC0_CLK:
+	case IMX8QM_SDHC0_DIV:
+		resource = SC_R_SDHC_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_SDHC1_IPG_CLK:
+	case IMX8QM_SDHC1_CLK:
+	case IMX8QM_SDHC1_DIV:
+		resource = SC_R_SDHC_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART0_IPG_CLK:
+	case IMX8QM_UART0_CLK:
+		resource = SC_R_UART_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART1_CLK:
+		resource = SC_R_UART_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART2_CLK:
+		resource = SC_R_UART_2;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART3_CLK:
+		resource = SC_R_UART_3;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_ENET0_IPG_CLK:
+	case IMX8QM_ENET0_AHB_CLK:
+	case IMX8QM_ENET0_REF_DIV:
+	case IMX8QM_ENET0_PTP_CLK:
+		resource = SC_R_ENET_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_ENET1_IPG_CLK:
+	case IMX8QM_ENET1_AHB_CLK:
+	case IMX8QM_ENET1_REF_DIV:
+	case IMX8QM_ENET1_PTP_CLK:
+		resource = SC_R_ENET_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	default:
+		if (clk->id < IMX8QM_UART0_IPG_CLK ||
+		    clk->id >= IMX8QM_CLK_END) {
+			printf("%s(Invalid clk ID #%lu)\n",
+			       __func__, clk->id);
+			return -EINVAL;
+		}
+		return -ENOTSUPP;
+	};
+
+	ret = sc_pm_get_clock_rate(-1, resource, pm_clk,
+				   (sc_pm_clock_rate_t *)&rate);
+	if (ret) {
+		printf("%s err %d\n", __func__, ret);
+		return ret;
+	}
+
+	return rate;
+}
+
+ulong imx8_clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	sc_pm_clk_t pm_clk;
+	u32 new_rate = rate;
+	u16 resource;
+	int ret;
+
+	debug("%s(#%lu), rate: %lu\n", __func__, clk->id, rate);
+
+	switch (clk->id) {
+	case IMX8QM_I2C0_CLK:
+		resource = SC_R_I2C_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_I2C1_CLK:
+		resource = SC_R_I2C_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_I2C2_CLK:
+		resource = SC_R_I2C_2;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_I2C3_CLK:
+		resource = SC_R_I2C_3;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART0_CLK:
+		resource = SC_R_UART_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART1_CLK:
+		resource = SC_R_UART_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART2_CLK:
+		resource = SC_R_UART_2;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART3_CLK:
+		resource = SC_R_UART_3;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_SDHC0_IPG_CLK:
+	case IMX8QM_SDHC0_CLK:
+	case IMX8QM_SDHC0_DIV:
+		resource = SC_R_SDHC_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_SDHC1_IPG_CLK:
+	case IMX8QM_SDHC1_CLK:
+	case IMX8QM_SDHC1_DIV:
+		resource = SC_R_SDHC_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_ENET0_IPG_CLK:
+	case IMX8QM_ENET0_AHB_CLK:
+	case IMX8QM_ENET0_REF_DIV:
+	case IMX8QM_ENET0_PTP_CLK:
+	case IMX8QM_ENET0_ROOT_DIV:
+		resource = SC_R_ENET_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_ENET1_IPG_CLK:
+	case IMX8QM_ENET1_AHB_CLK:
+	case IMX8QM_ENET1_REF_DIV:
+	case IMX8QM_ENET1_PTP_CLK:
+	case IMX8QM_ENET1_ROOT_DIV:
+		resource = SC_R_ENET_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	default:
+		if (clk->id < IMX8QM_UART0_IPG_CLK ||
+		    clk->id >= IMX8QM_CLK_END) {
+			printf("%s(Invalid clk ID #%lu)\n",
+			       __func__, clk->id);
+			return -EINVAL;
+		}
+		return -ENOTSUPP;
+	};
+
+	ret = sc_pm_set_clock_rate(-1, resource, pm_clk, &new_rate);
+	if (ret) {
+		printf("%s err %d\n", __func__, ret);
+		return ret;
+	}
+
+	return new_rate;
+}
+
+int __imx8_clk_enable(struct clk *clk, bool enable)
+{
+	sc_pm_clk_t pm_clk;
+	u16 resource;
+	int ret;
+
+	debug("%s(#%lu)\n", __func__, clk->id);
+
+	switch (clk->id) {
+	case IMX8QM_I2C0_CLK:
+		resource = SC_R_I2C_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_I2C1_CLK:
+		resource = SC_R_I2C_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_I2C2_CLK:
+		resource = SC_R_I2C_2;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_I2C3_CLK:
+		resource = SC_R_I2C_3;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART0_CLK:
+		resource = SC_R_UART_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART1_CLK:
+		resource = SC_R_UART_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART2_CLK:
+		resource = SC_R_UART_2;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_UART3_CLK:
+		resource = SC_R_UART_3;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_SDHC0_IPG_CLK:
+	case IMX8QM_SDHC0_CLK:
+	case IMX8QM_SDHC0_DIV:
+		resource = SC_R_SDHC_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_SDHC1_IPG_CLK:
+	case IMX8QM_SDHC1_CLK:
+	case IMX8QM_SDHC1_DIV:
+		resource = SC_R_SDHC_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_ENET0_IPG_CLK:
+	case IMX8QM_ENET0_AHB_CLK:
+	case IMX8QM_ENET0_REF_DIV:
+	case IMX8QM_ENET0_PTP_CLK:
+		resource = SC_R_ENET_0;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	case IMX8QM_ENET1_IPG_CLK:
+	case IMX8QM_ENET1_AHB_CLK:
+	case IMX8QM_ENET1_REF_DIV:
+	case IMX8QM_ENET1_PTP_CLK:
+		resource = SC_R_ENET_1;
+		pm_clk = SC_PM_CLK_PER;
+		break;
+	default:
+		if (clk->id < IMX8QM_UART0_IPG_CLK ||
+		    clk->id >= IMX8QM_CLK_END) {
+			printf("%s(Invalid clk ID #%lu)\n",
+			       __func__, clk->id);
+			return -EINVAL;
+		}
+		return -ENOTSUPP;
+	}
+
+	ret = sc_pm_clock_enable(-1, resource, pm_clk, enable, 0);
+	if (ret) {
+		printf("%s err %d\n", __func__, ret);
+		return ret;
+	}
+
+	return 0;
+}