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, &pxa_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, &pxa_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, &pll1_lock}, + {P