Re: [PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
Quoting Duje Mihanović (2024-04-20 06:32:56) > On 4/20/24 00:24, Stephen Boyd wrote: > > Quoting Duje Mihanović (2024-04-19 07:31:14) > >> On Friday, April 12, 2024 4:57:09 AM GMT+2 Stephen Boyd wrote: > >>> Quoting Duje Mihanović (2024-04-11 03:15:34) > >>> > On 4/11/2024 10:00 AM, Stephen Boyd wrote: > > Is there a reason this file can't be a platform driver? > > Not that I know of, I did it like this only because the other in-tree > MMP clk drivers do so. I guess the initialization should look like any > of the qcom GCC drivers then? > >>> > >>> Yes. > >> > >> With the entire clock driver code in one file this is quite messy as I also > >> needed to add module_init and module_exit functions to (un)register each > >> platform driver, presumably because the module_platform_driver macro > >> doesn't > >> work with multiple platform drivers in one module. If I split up the driver > >> code for each clock controller block into its own file (such as clk-of- > >> pxa1908-apbc.c) as I believe is the best option, should the commits be > >> split > >> up accordingly as well? > > > > Sure. Why is 'of' in the name? Maybe that is unnecessary? > > That seems to be a historical leftover from when Marvell was just adding > DT support to the ARM32 MMP SoCs which Rob followed along with in the > PXA1928 clk driver and so have I. Should I drop it then as Marvell has > in the PXA1908 vendor kernel? > Sounds good to me.
Re: [PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
On 4/20/24 00:24, Stephen Boyd wrote: Quoting Duje Mihanović (2024-04-19 07:31:14) On Friday, April 12, 2024 4:57:09 AM GMT+2 Stephen Boyd wrote: Quoting Duje Mihanović (2024-04-11 03:15:34) On 4/11/2024 10:00 AM, Stephen Boyd wrote: Is there a reason this file can't be a platform driver? Not that I know of, I did it like this only because the other in-tree MMP clk drivers do so. I guess the initialization should look like any of the qcom GCC drivers then? Yes. With the entire clock driver code in one file this is quite messy as I also needed to add module_init and module_exit functions to (un)register each platform driver, presumably because the module_platform_driver macro doesn't work with multiple platform drivers in one module. If I split up the driver code for each clock controller block into its own file (such as clk-of- pxa1908-apbc.c) as I believe is the best option, should the commits be split up accordingly as well? Sure. Why is 'of' in the name? Maybe that is unnecessary? That seems to be a historical leftover from when Marvell was just adding DT support to the ARM32 MMP SoCs which Rob followed along with in the PXA1928 clk driver and so have I. Should I drop it then as Marvell has in the PXA1908 vendor kernel? Regards, -- Duje
Re: [PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
Quoting Duje Mihanović (2024-04-19 07:31:14) > On Friday, April 12, 2024 4:57:09 AM GMT+2 Stephen Boyd wrote: > > Quoting Duje Mihanović (2024-04-11 03:15:34) > > > > > On 4/11/2024 10:00 AM, Stephen Boyd wrote: > > > > Is there a reason this file can't be a platform driver? > > > > > > Not that I know of, I did it like this only because the other in-tree > > > MMP clk drivers do so. I guess the initialization should look like any > > > of the qcom GCC drivers then? > > > > Yes. > > With the entire clock driver code in one file this is quite messy as I also > needed to add module_init and module_exit functions to (un)register each > platform driver, presumably because the module_platform_driver macro doesn't > work with multiple platform drivers in one module. If I split up the driver > code for each clock controller block into its own file (such as clk-of- > pxa1908-apbc.c) as I believe is the best option, should the commits be split > up accordingly as well? Sure. Why is 'of' in the name? Maybe that is unnecessary? > > > > While at it, do you think the other MMP clk drivers could use a > conversion? > > > > I'm a little wary if the conversion cannot be tested though. > > I'd rather leave it to someone with the hardware then, especially since the > only reason I found out about the above is that the board I'm working on > failed to boot completely without the module_init function. > Ok, sounds fine.
Re: [PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
On Friday, April 12, 2024 4:57:09 AM GMT+2 Stephen Boyd wrote: > Quoting Duje Mihanović (2024-04-11 03:15:34) > > > On 4/11/2024 10:00 AM, Stephen Boyd wrote: > > > Is there a reason this file can't be a platform driver? > > > > Not that I know of, I did it like this only because the other in-tree > > MMP clk drivers do so. I guess the initialization should look like any > > of the qcom GCC drivers then? > > Yes. With the entire clock driver code in one file this is quite messy as I also needed to add module_init and module_exit functions to (un)register each platform driver, presumably because the module_platform_driver macro doesn't work with multiple platform drivers in one module. If I split up the driver code for each clock controller block into its own file (such as clk-of- pxa1908-apbc.c) as I believe is the best option, should the commits be split up accordingly as well? > > While at it, do you think the other MMP clk drivers could use a conversion? > > I'm a little wary if the conversion cannot be tested though. I'd rather leave it to someone with the hardware then, especially since the only reason I found out about the above is that the board I'm working on failed to boot completely without the module_init function. Regards, -- Duje
Re: [PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
Quoting Duje Mihanović (2024-04-11 03:15:34) > On 4/11/2024 10:00 AM, Stephen Boyd wrote: > > > > Is there a reason this file can't be a platform driver? > > Not that I know of, I did it like this only because the other in-tree > MMP clk drivers do so. I guess the initialization should look like any > of the qcom GCC drivers then? Yes. > > While at it, do you think the other MMP clk drivers could use a conversion? > Sure, go for it. I'm a little wary if the conversion cannot be tested though. It doesn't hurt that other drivers haven't been converted.
Re: [PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
On 4/11/2024 10:00 AM, Stephen Boyd wrote:
Quoting Duje Mihanović (2024-04-02 13:55:41)
diff --git a/drivers/clk/mmp/clk-of-pxa1908.c b/drivers/clk/mmp/clk-of-pxa1908.c
new file mode 100644
index ..6f1f6e25a718
--- /dev/null
+++ b/drivers/clk/mmp/clk-of-pxa1908.c
@@ -0,0 +1,328 @@
+// SPDX-License-Identifier: GPL-2.0-only
[...]
+static void __init pxa1908_apbc_clk_init(struct device_node *np)
+{
+ struct pxa1908_clk_unit *pxa_unit;
+
+ pxa_unit = kzalloc(sizeof(*pxa_unit), GFP_KERNEL);
+ if (!pxa_unit)
+ return;
+
+ pxa_unit->apbc_base = of_iomap(np, 0);
+ if (!pxa_unit->apbc_base) {
+ pr_err("failed to map apbc registers\n");
+ kfree(pxa_unit);
+ return;
+ }
+
+ mmp_clk_init(np, _unit->unit, APBC_NR_CLKS);
+
+ pxa1908_apb_periph_clk_init(pxa_unit);
+}
+CLK_OF_DECLARE(pxa1908_apbc, "marvell,pxa1908-apbc", pxa1908_apbc_clk_init);
Is there a reason this file can't be a platform driver?
Not that I know of, I did it like this only because the other in-tree
MMP clk drivers do so. I guess the initialization should look like any
of the qcom GCC drivers then?
While at it, do you think the other MMP clk drivers could use a conversion?
Regards,
--
Duje
Re: [PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
Quoting Duje Mihanović (2024-04-02 13:55:41)
> diff --git a/drivers/clk/mmp/clk-of-pxa1908.c
> b/drivers/clk/mmp/clk-of-pxa1908.c
> new file mode 100644
> index ..6f1f6e25a718
> --- /dev/null
> +++ b/drivers/clk/mmp/clk-of-pxa1908.c
> @@ -0,0 +1,328 @@
> +// SPDX-License-Identifier: GPL-2.0-only
[...]
> +static void __init pxa1908_apbc_clk_init(struct device_node *np)
> +{
> + struct pxa1908_clk_unit *pxa_unit;
> +
> + pxa_unit = kzalloc(sizeof(*pxa_unit), GFP_KERNEL);
> + if (!pxa_unit)
> + return;
> +
> + pxa_unit->apbc_base = of_iomap(np, 0);
> + if (!pxa_unit->apbc_base) {
> + pr_err("failed to map apbc registers\n");
> + kfree(pxa_unit);
> + return;
> + }
> +
> + mmp_clk_init(np, _unit->unit, APBC_NR_CLKS);
> +
> + pxa1908_apb_periph_clk_init(pxa_unit);
> +}
> +CLK_OF_DECLARE(pxa1908_apbc, "marvell,pxa1908-apbc", pxa1908_apbc_clk_init);
Is there a reason this file can't be a platform driver?
[PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
Add driver for Marvell PXA1908 clock controller blocks. The SoC has
numerous clock controller blocks, currently supporting APBC, APBCP, MPMU
and APMU.
Signed-off-by: Duje Mihanović
---
drivers/clk/mmp/Makefile | 2 +-
drivers/clk/mmp/clk-of-pxa1908.c | 328 +++
2 files changed, 329 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/mmp/Makefile b/drivers/clk/mmp/Makefile
index 441bf83080a1..69f9c3afde83 100644
--- a/drivers/clk/mmp/Makefile
+++ b/drivers/clk/mmp/Makefile
@@ -11,4 +11,4 @@ obj-$(CONFIG_MACH_MMP_DT) += clk-of-pxa168.o clk-of-pxa910.o
obj-$(CONFIG_COMMON_CLK_MMP2) += clk-of-mmp2.o clk-pll.o pwr-island.o
obj-$(CONFIG_COMMON_CLK_MMP2_AUDIO) += clk-audio.o
-obj-y += clk-of-pxa1928.o
+obj-$(CONFIG_ARCH_MMP) += clk-of-pxa1928.o clk-of-pxa1908.o
diff --git a/drivers/clk/mmp/clk-of-pxa1908.c b/drivers/clk/mmp/clk-of-pxa1908.c
new file mode 100644
index ..6f1f6e25a718
--- /dev/null
+++ b/drivers/clk/mmp/clk-of-pxa1908.c
@@ -0,0 +1,328 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#include "clk.h"
+
+#define APMU_CLK_GATE_CTRL 0x40
+#define MPMU_UART_PLL 0x14
+
+#define APBC_UART0 0x0
+#define APBC_UART1 0x4
+#define APBC_GPIO 0x8
+#define APBC_PWM0 0xc
+#define APBC_PWM1 0x10
+#define APBC_PWM2 0x14
+#define APBC_PWM3 0x18
+#define APBC_SSP0 0x1c
+#define APBC_SSP1 0x20
+#define APBC_IPC_RST 0x24
+#define APBC_RTC 0x28
+#define APBC_TWSI0 0x2c
+#define APBC_KPC 0x30
+#define APBC_SWJTAG0x40
+#define APBC_SSP2 0x4c
+#define APBC_TWSI1 0x60
+#define APBC_THERMAL 0x6c
+#define APBC_TWSI3 0x70
+
+#define APBCP_UART20x1c
+#define APBCP_TWSI20x28
+#define APBCP_AICER0x38
+
+#define APMU_CCIC1 0x24
+#define APMU_ISP 0x38
+#define APMU_DSI1 0x44
+#define APMU_DISP1 0x4c
+#define APMU_CCIC0 0x50
+#define APMU_SDH0 0x54
+#define APMU_SDH1 0x58
+#define APMU_USB 0x5c
+#define APMU_NF0x60
+#define APMU_VPU 0xa4
+#define APMU_GC0xcc
+#define APMU_SDH2 0xe0
+#define APMU_GC2D 0xf4
+#define APMU_TRACE 0x108
+#define APMU_DVC_DFC_DEBUG 0x140
+
+#define MPMU_NR_CLKS 39
+#define APBC_NR_CLKS 19
+#define APBCP_NR_CLKS 4
+#define APMU_NR_CLKS 17
+
+struct pxa1908_clk_unit {
+ struct mmp_clk_unit unit;
+ void __iomem *mpmu_base;
+ void __iomem *apmu_base;
+ void __iomem *apbc_base;
+ void __iomem *apbcp_base;
+ void __iomem *apbs_base;
+ void __iomem *ciu_base;
+};
+
+static struct mmp_param_fixed_rate_clk fixed_rate_clks[] = {
+ {PXA1908_CLK_CLK32, "clk32", NULL, 0, 32768},
+ {PXA1908_CLK_VCTCXO, "vctcxo", NULL, 0, 26 * HZ_PER_MHZ},
+ {PXA1908_CLK_PLL1_624, "pll1_624", NULL, 0, 624 * HZ_PER_MHZ},
+ {PXA1908_CLK_PLL1_416, "pll1_416", NULL, 0, 416 * HZ_PER_MHZ},
+ {PXA1908_CLK_PLL1_499, "pll1_499", NULL, 0, 499 * HZ_PER_MHZ},
+ {PXA1908_CLK_PLL1_832, "pll1_832", NULL, 0, 832 * HZ_PER_MHZ},
+ {PXA1908_CLK_PLL1_1248, "pll1_1248", NULL, 0, 1248 * HZ_PER_MHZ},
+};
+
+static struct mmp_param_fixed_factor_clk fixed_factor_clks[] = {
+ {PXA1908_CLK_PLL1_D2, "pll1_d2", "pll1_624", 1, 2, 0},
+ {PXA1908_CLK_PLL1_D4, "pll1_d4", "pll1_d2", 1, 2, 0},
+ {PXA1908_CLK_PLL1_D6, "pll1_d6", "pll1_d2", 1, 3, 0},
+ {PXA1908_CLK_PLL1_D8, "pll1_d8", "pll1_d4", 1, 2, 0},
+ {PXA1908_CLK_PLL1_D12, "pll1_d12", "pll1_d6", 1, 2, 0},
+ {PXA1908_CLK_PLL1_D13, "pll1_d13", "pll1_624", 1, 13, 0},
+ {PXA1908_CLK_PLL1_D16, "pll1_d16", "pll1_d8", 1, 2, 0},
+ {PXA1908_CLK_PLL1_D24, "pll1_d24", "pll1_d12", 1, 2, 0},
+ {PXA1908_CLK_PLL1_D48, "pll1_d48", "pll1_d24", 1, 2, 0},
+ {PXA1908_CLK_PLL1_D96, "pll1_d96", "pll1_d48", 1, 2, 0},
+ {PXA1908_CLK_PLL1_32, "pll1_32", "pll1_d13", 2, 3, 0},
+ {PXA1908_CLK_PLL1_208, "pll1_208", "pll1_d2", 2, 3, 0},
+ {PXA1908_CLK_PLL1_117, "pll1_117", "pll1_624", 3, 16, 0},
+};
+
+static struct mmp_clk_factor_masks uart_factor_masks = {
+ .factor = 2,
+ .num_mask = GENMASK(12, 0),
+ .den_mask = GENMASK(12, 0),
+ .num_shift = 16,
+ .den_shift = 0,
+};
+
+static struct u32_fract uart_factor_tbl[] = {
+ {.numerator = 8125, .denominator = 1536}, /* 14.745MHz */
+};
+
+static DEFINE_SPINLOCK(pll1_lock);
+static struct mmp_param_general_gate_clk pll1_gate_clks[] = {
+ {PXA1908_CLK_PLL1_D2_GATE, "pll1_d2_gate", "pll1_d2", 0,
APMU_CLK_GATE_CTRL, 29, 0, _lock},
+
