Re: [U-Boot] [PATCH 07/10] clk: imx8: add i.MX8QM clk driver
Hi Stefano > -Original Message- > From: Peng Fan > Sent: 2019年1月29日 9:15 > To: 'Stefano Babic' > Cc: Fabio Estevam ; dl-uboot-imx > ; 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: > sba...@denx.de > > > == > > === ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 07/10] clk: imx8: add i.MX8QM clk driver
> -Original Message- > From: Fabio Estevam [mailto:feste...@gmail.com] > Sent: 2019年1月28日 22:24 > To: Stefano Babic > Cc: Peng Fan ; Fabio Estevam > ; u-boot@lists.denx.de; dl-uboot-imx > ; Lukasz Majewski > 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 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.cdata=02%7C01%7Cpeng.fan%40nxp.com%7C124a7ae228794 > 2551ea208d6852c3c0a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C > 0%7C636842822364000869sdata=6xwmlVrr2MGKkRCNUFZTBcec9KP > RNoID%2FBWTU5HFKcg%3Dreserved=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.cdata=02%7C01%7Cpeng.fan%40nxp.com%7C124a7ae22879425 > 51ea208d6852c3c0a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0% > 7C636842822364000869sdata=m6%2FRa8HY9M1aPXy45%2Bp0aa9Xit > 6cMUVqBw%2F4UVTSOz0%3Dreserved=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. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [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. 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: sba...@denx.de > == > === ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 07/10] clk: imx8: add i.MX8QM clk driver
Hi Fabio, On 28/01/19 15:23, Fabio Estevam wrote: > Hi Stefano, > > On Mon, Jan 28, 2019 at 12:04 PM Stefano Babic 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 -- = 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: sba...@denx.de = ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
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 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. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 07/10] clk: imx8: add i.MX8QM clk driver
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
> ---
> 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 00..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
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#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);
> +
[U-Boot] [PATCH 07/10] clk: imx8: add i.MX8QM clk driver
Add i.MX8QM clk driver, SDHC/FEC/UART/I2C supported.
Signed-off-by: Peng Fan
---
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 00..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
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#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,
+
