Re: [PATCH 0/2] arm64: dts: imx8mp: Add GPT blocks

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Uwe,

Am Dienstag, 28. März 2023, 13:03:07 CEST schrieb Uwe Kleine-König:
> * PGP Signed by an unknown key
> 
> On Mon, Mar 27, 2023 at 07:35:24PM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > according to the dt binding we have the following major GPT variants:
> > 	imx1
> > 	imx21
> > 	imx31
> > 	imx6dl
> > 
> > I compared the GPT documentation for i.MX8MP, i.MX6DL and i.MX6Q (which
> > is a member of the imx31 variant) and didn't spot a difference. So
> > either these are actually identical, or I missed a detail. In the latter
> > case i.MX8MP might be a member of the imx31 variant, too, and not as
> > advertised in the commit logs of the imx6dl.
> 
> TL;DR: I did it right, i.MX8MP has a GPT of the i.MX6DL type.
> 
> > Does someone know (or spot) the relevant difference?
> 
> I found it. The relevant difference is that the i.MX6DL variant has a
> bit EN_24M in the CR register which is missing on i.MX31. Finding that
> was a bit complicated by the fact that i.MX6Q in fact has a GPT of the
> i.MX6DL type starting with rev1.1.
> 
> The only difference betweeen these two types in the gpt driver is:
> 
> static void imx31_gpt_setup_tctl(struct imx_timer *imxtm)
> {
>         u32 tctl_val;
> 
>         tctl_val = V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
>         if (clk_get_rate(imxtm->clk_per) == V2_TIMER_RATE_OSC_DIV8)
>                 tctl_val |= V2_TCTL_CLK_OSC_DIV8;
>         else
>                 tctl_val |= V2_TCTL_CLK_PER;
> 
>         writel_relaxed(tctl_val, imxtm->base + MXC_TCTL);
> }
> 
> static void imx6dl_gpt_setup_tctl(struct imx_timer *imxtm)
> {
>         u32 tctl_val;
> 
>         tctl_val = V2_TCTL_FRR | V2_TCTL_WAITEN | MXC_TCTL_TEN;
>         if (clk_get_rate(imxtm->clk_per) == V2_TIMER_RATE_OSC_DIV8) {
>                 tctl_val |= V2_TCTL_CLK_OSC_DIV8;
>                 /* 24 / 8 = 3 MHz */
>                 writel_relaxed(7 << V2_TPRER_PRE24M, imxtm->base +
> MXC_TPRER); tctl_val |= V2_TCTL_24MEN;
>         } else {
>                 tctl_val |= V2_TCTL_CLK_PER;
>         }
> 
>         writel_relaxed(tctl_val, imxtm->base + MXC_TCTL);
> }
> 
> I wonder about a few things:
> 
>  - Does setting the V2_TCTL_24MEN flag has an effect on the i.MX31
>    variant? I assume the 24M clk is on unconditionally there?
>    OTOH in the RM of the i.MX31 (MCIMX31RM Rev. 2.4 12/2008) the value
>    V2_TCTL_CLK_OSC_DIV8 (i.e. b101) is reserved for the CLKSRC field. So
>    maybe the 24M clk cannot be used as a clksrc here?
> 
>  - The check
> 
>  	clk_get_rate(imxtm->clk_per) == V2_TIMER_RATE_OSC_DIV8
> 
>    looks strange. If the per clk runs at V2_TIMER_RATE_OSC_DIV8
>    (i.e. 3000000) Hz it's not the 24M clk, is it? So using
>    V2_TCTL_CLK_OSC_DIV8 has no effect?!
> 
>    If the check is always false, we can handle the i.MX6DL and the
>    i.MX31 type GPT identically.

I think this is related to commit 2b2244a3e7c3 ("ARM: dts: imx6: make gpt per 
clock can be from OSC") where clk_per can be set to 3MHz for imx6qdl. The 
commit message also mentions i.mx6q > rev 1.0.
This 'osc_per' is also currently causing a lot of dtbs_check warnings for 
imx6qdl boards.

Best regards,
Alexander

>  - Should we change i.MX6Q to use the i.MX6DL type GPT? Is rev1.0 still
>    relevant?
> 
> Best regards
> Uwe


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/