[U-Boot] [PATCH 0/4] arm: mxs: mxs_set_gpmiclk

[U-Boot] [PATCH 0/4] arm: mxs: mxs_set_gpmiclk

[Rasmus Villemoes]

While trying to implement an mxs_set_gpmiclk() I stumbled on a few minor things.

Rasmus Villemoes (4):
  arm: mxs: fix register definitions for clkctrl_gpmi and clkctrl_sspX
  arm: mxs: fix comments in arch_cpu_init to match the code
  arm: mxs: be more careful when enabling gpmi_clk
  arm: mxs: implement mxs_set_gpmiclk

 arch/arm/cpu/arm926ejs/mxs/clock.c            | 41 +++++++++++++++++++
 arch/arm/cpu/arm926ejs/mxs/mxs.c              |  9 ++--
 arch/arm/include/asm/arch-mxs/clock.h         |  1 +
 .../include/asm/arch-mxs/regs-clkctrl-mx23.h  |  6 ++-
 .../include/asm/arch-mxs/regs-clkctrl-mx28.h  | 15 ++++---
 5 files changed, 62 insertions(+), 10 deletions(-)

-- 
2.20.1

[U-Boot] [PATCH 1/4] arm: mxs: fix register definitions for clkctrl_gpmi and clkctrl_sspX

[Rasmus Villemoes]

I tried clearing a bit by writing to hw_clkctrl_gpmi_clr, then
busy-waiting for it to actually clear. My board hung. The data sheet
agrees, these registers do not have _set, _clr, _tog, so fix up the
definitions. git grep -E 'clkctrl_(gpmi|ssp[0-9])_' says that nobody
uses those non-existing ops registers.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/include/asm/arch-mxs/regs-clkctrl-mx23.h |  6 ++++--
 arch/arm/include/asm/arch-mxs/regs-clkctrl-mx28.h | 15 ++++++++++-----
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx23.h b/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx23.h
index 6e9ffeb6d5..50fdc9cd03 100644
--- a/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx23.h
+++ b/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx23.h
@@ -24,8 +24,10 @@ struct mxs_clkctrl_regs {
 	mxs_reg_32(hw_clkctrl_xbus)		/* 0x40 */
 	mxs_reg_32(hw_clkctrl_xtal)		/* 0x50 */
 	mxs_reg_32(hw_clkctrl_pix)		/* 0x60 */
-	mxs_reg_32(hw_clkctrl_ssp0)		/* 0x70 */
-	mxs_reg_32(hw_clkctrl_gpmi)		/* 0x80 */
+	uint32_t	hw_clkctrl_ssp0;	/* 0x70 */
+	uint32_t	reserved_ssp0[3];	/* 0x74-0x7c */
+	uint32_t	hw_clkctrl_gpmi;	/* 0x80 */
+	uint32_t	reserved_gpmi[3];	/* 0x84-0x8c */
 	mxs_reg_32(hw_clkctrl_spdif)		/* 0x90 */
 	mxs_reg_32(hw_clkctrl_emi)		/* 0xa0 */
 
diff --git a/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx28.h b/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx28.h
index 01e0a7a053..caef9e4b1f 100644
--- a/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx28.h
+++ b/arch/arm/include/asm/arch-mxs/regs-clkctrl-mx28.h
@@ -27,11 +27,16 @@ struct mxs_clkctrl_regs {
 	mxs_reg_32(hw_clkctrl_hbus)		/* 0x60 */
 	mxs_reg_32(hw_clkctrl_xbus)		/* 0x70 */
 	mxs_reg_32(hw_clkctrl_xtal)		/* 0x80 */
-	mxs_reg_32(hw_clkctrl_ssp0)		/* 0x90 */
-	mxs_reg_32(hw_clkctrl_ssp1)		/* 0xa0 */
-	mxs_reg_32(hw_clkctrl_ssp2)		/* 0xb0 */
-	mxs_reg_32(hw_clkctrl_ssp3)		/* 0xc0 */
-	mxs_reg_32(hw_clkctrl_gpmi)		/* 0xd0 */
+	uint32_t	hw_clkctrl_ssp0;	/* 0x90 */
+	uint32_t	reserved_ssp0[3];	/* 0x94-0x9c */
+	uint32_t	hw_clkctrl_ssp1;	/* 0xa0 */
+	uint32_t	reserved_ssp1[3];	/* 0xa4-0xac */
+	uint32_t	hw_clkctrl_ssp2;	/* 0xb0 */
+	uint32_t	reserved_ssp2[3];	/* 0xb4-0xbc */
+	uint32_t	hw_clkctrl_ssp3;	/* 0xc0 */
+	uint32_t	reserved_ssp3[3];	/* 0xc4-0xcc */
+	uint32_t	hw_clkctrl_gpmi;	/* 0xd0 */
+	uint32_t	reserved_gpmi[3];	/* 0xd4-0xdc */
 	mxs_reg_32(hw_clkctrl_spdif)		/* 0xe0 */
 	mxs_reg_32(hw_clkctrl_emi)		/* 0xf0 */
 	mxs_reg_32(hw_clkctrl_saif0)		/* 0x100 */
-- 
2.20.1

[U-Boot] [PATCH 2/4] arm: mxs: fix comments in arch_cpu_init to match the code

[Rasmus Villemoes]

The comment says to clear the bypass bit, but in fact it sets it, thus
selecting ref_xtal. And the next line of code does not set the divider
to 12, but to (the reset value of) 1.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/cpu/arm926ejs/mxs/mxs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/mxs/mxs.c b/arch/arm/cpu/arm926ejs/mxs/mxs.c
index 85c65dcb44..585c53baf6 100644
--- a/arch/arm/cpu/arm926ejs/mxs/mxs.c
+++ b/arch/arm/cpu/arm926ejs/mxs/mxs.c
@@ -98,11 +98,11 @@ int arch_cpu_init(void)
 	/*
 	 * Enable NAND clock
 	 */
-	/* Clear bypass bit */
+	/* Set bypass bit */
 	writel(CLKCTRL_CLKSEQ_BYPASS_GPMI,
 		&clkctrl_regs->hw_clkctrl_clkseq_set);
 
-	/* Set GPMI clock to ref_gpmi / 12 */
+	/* Set GPMI clock to ref_xtal / 1 */
 	clrsetbits_le32(&clkctrl_regs->hw_clkctrl_gpmi,
 		CLKCTRL_GPMI_CLKGATE | CLKCTRL_GPMI_DIV_MASK, 1);
 
-- 
2.20.1

[U-Boot] [PATCH 3/4] arm: mxs: be more careful when enabling gpmi_clk

[Rasmus Villemoes]

The data sheet says that the DIV field cannot change while the CLKGATE
bit is set or modified. So do it a little more carefully, by first
clearing the bit, waiting for that to appear, then setting the DIV
field.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/cpu/arm926ejs/mxs/mxs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/cpu/arm926ejs/mxs/mxs.c b/arch/arm/cpu/arm926ejs/mxs/mxs.c
index 585c53baf6..183aa40b6d 100644
--- a/arch/arm/cpu/arm926ejs/mxs/mxs.c
+++ b/arch/arm/cpu/arm926ejs/mxs/mxs.c
@@ -103,8 +103,11 @@ int arch_cpu_init(void)
 		&clkctrl_regs->hw_clkctrl_clkseq_set);
 
 	/* Set GPMI clock to ref_xtal / 1 */
+	clrbits_le32(&clkctrl_regs->hw_clkctrl_gpmi, CLKCTRL_GPMI_CLKGATE);
+	while (readl(&clkctrl_regs->hw_clkctrl_gpmi) & CLKCTRL_GPMI_CLKGATE)
+		;
 	clrsetbits_le32(&clkctrl_regs->hw_clkctrl_gpmi,
-		CLKCTRL_GPMI_CLKGATE | CLKCTRL_GPMI_DIV_MASK, 1);
+		CLKCTRL_GPMI_DIV_MASK, 1);
 
 	udelay(1000);
 
-- 
2.20.1

[U-Boot] [PATCH 4/4] arm: mxs: implement mxs_set_gpmiclk

[Rasmus Villemoes]

This allows a board file to set the gpmiclk to something other than
the default 24 MHz based on ref_xtal.

I don't have an mx23-based board, but I believe there's a bug in the
current mxs_get_gpmiclk: According to the data sheet, the gpmiclk can
be derived from ref_io, not ref_cpu. Since other clocks are also
derived from ref_io, it seems most sensible to require the board file
to set that appropriately first, then derive the divider from its
current setting.

For mx28 boards, OTOH, there's a separate ref_gpmi only used for
clk_gpmi. For simplicity, if !xtal, simply enable that at its maximum
frequency.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 arch/arm/cpu/arm926ejs/mxs/clock.c    | 41 +++++++++++++++++++++++++++
 arch/arm/include/asm/arch-mxs/clock.h |  1 +
 2 files changed, 42 insertions(+)

diff --git a/arch/arm/cpu/arm926ejs/mxs/clock.c b/arch/arm/cpu/arm926ejs/mxs/clock.c
index 43d044d917..d247da56fe 100644
--- a/arch/arm/cpu/arm926ejs/mxs/clock.c
+++ b/arch/arm/cpu/arm926ejs/mxs/clock.c
@@ -138,6 +138,47 @@ static uint32_t mxs_get_gpmiclk(void)
 	return (PLL_FREQ_MHZ * PLL_FREQ_COEF / frac) / div;
 }
 
+void mxs_set_gpmiclk(uint32_t freq, int xtal)
+{
+	struct mxs_clkctrl_regs *clkctrl_regs =
+		(struct mxs_clkctrl_regs *)MXS_CLKCTRL_BASE;
+	uint32_t clk, div;
+
+	if (xtal) {
+		clk = XTAL_FREQ_KHZ;
+	} else {
+#if defined(CONFIG_MX23)
+		clk = mxs_get_ioclk(MXC_IOCLK0);
+#elif defined(CONFIG_MX28)
+		/* enable ref_gpmi at 480 MHz. */
+		writeb(CLKCTRL_FRAC_CLKGATE,
+			&clkctrl_regs->hw_clkctrl_frac1_set[CLKCTRL_FRAC1_GPMI]);
+		writeb(CLKCTRL_FRAC_CLKGATE | PLL_FREQ_COEF,
+			&clkctrl_regs->hw_clkctrl_frac1[CLKCTRL_FRAC1_GPMI]);
+		writeb(CLKCTRL_FRAC_CLKGATE,
+			&clkctrl_regs->hw_clkctrl_frac1_clr[CLKCTRL_FRAC1_GPMI]);
+		clk = PLL_FREQ_KHZ;
+#endif
+	}
+	if (freq > clk)
+		return;
+	div = clk / freq;
+	if (div > CLKCTRL_GPMI_DIV_MASK)
+		div = CLKCTRL_GPMI_DIV_MASK;
+
+	clrbits_le32(&clkctrl_regs->hw_clkctrl_gpmi, CLKCTRL_GPMI_CLKGATE);
+	while (readl(&clkctrl_regs->hw_clkctrl_gpmi) & CLKCTRL_GPMI_CLKGATE)
+		;
+	clrsetbits_le32(&clkctrl_regs->hw_clkctrl_gpmi, CLKCTRL_GPMI_DIV_MASK, div);
+	while (readl(&clkctrl_regs->hw_clkctrl_gpmi) & CLKCTRL_GPMI_BUSY)
+		;
+
+	if (xtal)
+		writel(CLKCTRL_CLKSEQ_BYPASS_GPMI, &clkctrl_regs->hw_clkctrl_clkseq_set);
+	else
+		writel(CLKCTRL_CLKSEQ_BYPASS_GPMI, &clkctrl_regs->hw_clkctrl_clkseq_clr);
+}
+
 /*
  * Set IO clock frequency, in kHz
  */
diff --git a/arch/arm/include/asm/arch-mxs/clock.h b/arch/arm/include/asm/arch-mxs/clock.h
index ee56d10fec..0a6625eb90 100644
--- a/arch/arm/include/asm/arch-mxs/clock.h
+++ b/arch/arm/include/asm/arch-mxs/clock.h
@@ -44,6 +44,7 @@ uint32_t mxc_get_clock(enum mxc_clock clk);
 
 void mxs_set_ioclk(enum mxs_ioclock io, uint32_t freq);
 void mxs_set_sspclk(enum mxs_sspclock ssp, uint32_t freq, int xtal);
+void mxs_set_gpmiclk(uint32_t freq, int xtal);
 void mxs_set_ssp_busclock(unsigned int bus, uint32_t freq);
 void mxs_set_lcdclk(uint32_t __maybe_unused lcd_base, uint32_t freq);
 
-- 
2.20.1

[U-Boot] [PATCH 0/4] arm: mxs: mxs_set_gpmiclk

[Rasmus Villemoes]

On 12/09/2019 11.17, Rasmus Villemoes wrote:
> While trying to implement an mxs_set_gpmiclk() I stumbled on a few minor things.
> 
> Rasmus Villemoes (4):
>   arm: mxs: fix register definitions for clkctrl_gpmi and clkctrl_sspX
>   arm: mxs: fix comments in arch_cpu_init to match the code
>   arm: mxs: be more careful when enabling gpmi_clk
>   arm: mxs: implement mxs_set_gpmiclk

ping

[PATCH 0/4] arm: mxs: mxs_set_gpmiclk

[Stefano Babic]

Hi Rasmus,

On 27/09/19 10:33, Rasmus Villemoes wrote:
> On 12/09/2019 11.17, Rasmus Villemoes wrote:
>> While trying to implement an mxs_set_gpmiclk() I stumbled on a few minor things.
>>
>> Rasmus Villemoes (4):
>>   arm: mxs: fix register definitions for clkctrl_gpmi and clkctrl_sspX
>>   arm: mxs: fix comments in arch_cpu_init to match the code
>>   arm: mxs: be more careful when enabling gpmi_clk
>>   arm: mxs: implement mxs_set_gpmiclk
> 
> ping
> 

You're right, patch was lost (..just because the list of patches for
i.MX was very long and I have not seen..), I found the series again.

I applied the first 3 patches, they are improvements / clarifications.
The fourth patch has no user, so I ask who is suing it. Nevertheless,
patch 4/4 breaks some boards (I found at least "xfi3") because it is
compiled even for not i.MX boards and then prototype is missing:


       arm:  +   xfi3
+arch/arm/cpu/arm926ejs/mxs/clock.c: In function 'mxs_set_gpmiclk':
+arch/arm/cpu/arm926ejs/mxs/clock.c:151:9: error: implicit declaration
of function 'mxs_get_ioclk'; did you mean 'mxs_set_ioclk'?
[-Werror=implicit-function-declaration]
+   clk = mxs_get_ioclk(MXC_IOCLK0);
+         ^~~~~~~~~~~~~
+         mxs_set_ioclk
+arch/arm/cpu/arm926ejs/mxs/clock.c: At top level:
+arch/arm/cpu/arm926ejs/mxs/clock.c:218:17: error: conflicting types for
'mxs_get_ioclk'
+ static uint32_t mxs_get_ioclk(enum mxs_ioclock io)
+                 ^~~~~~~~~~~~~
+arch/arm/cpu/arm926ejs/mxs/clock.c:151:9: note: previous implicit
declaration of 'mxs_get_ioclk' was here
+cc1: all warnings being treated as errors
+make[3]: *** [arch/arm/cpu/arm926ejs/mxs/clock.o] Error 1
+make[2]: *** [arch/arm/cpu/arm926ejs/mxs] Error 2
+make[1]: *** [arch/arm/cpu/arm926ejs] Error 2
+make: *** [sub-make] Error 2

I have nothing against to merge this, too, but the issue above must be
fixed - thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
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 at denx.de
=====================================================================

[PATCH 0/4] arm: mxs: mxs_set_gpmiclk

[Rasmus Villemoes]

On 03/01/2020 13.42, Stefano Babic wrote:
> Hi Rasmus,
> 
> On 27/09/19 10:33, Rasmus Villemoes wrote:
>> On 12/09/2019 11.17, Rasmus Villemoes wrote:
>>> While trying to implement an mxs_set_gpmiclk() I stumbled on a few minor things.
>>>
>>> Rasmus Villemoes (4):
>>>   arm: mxs: fix register definitions for clkctrl_gpmi and clkctrl_sspX
>>>   arm: mxs: fix comments in arch_cpu_init to match the code
>>>   arm: mxs: be more careful when enabling gpmi_clk
>>>   arm: mxs: implement mxs_set_gpmiclk
>>
>> ping
>>
> 
> You're right, patch was lost (..just because the list of patches for
> i.MX was very long and I have not seen..), I found the series again.
> 
> I applied the first 3 patches, they are improvements / clarifications.

Thanks.

> The fourth patch has no user, so I ask who is suing it. 

I had a user in the branch I was working on, from which I tried to carve
out some upstreamable chunks. Unfortunately, the focus internally has
now shifted away from that mx28 board, so I don't know if and when I'll
be working on that again. But I'll try to remember to fix the break you
report the next time around.

Thanks,
Rasmus