Re: [edk2] [PATCH edk2-platforms 14/27] Silicon/NXP: Add i.MX6 GPT and EPIT timer headers
On Tue, Dec 04, 2018 at 02:06:04AM +, Chris Co wrote: > > > diff --git a/Silicon/NXP/iMX6Pkg/Include/common_epit.h > > > b/Silicon/NXP/iMX6Pkg/Include/common_epit.h > > > new file mode 100644 > > > index ..485d6ccbc51e > > > --- /dev/null > > > +++ b/Silicon/NXP/iMX6Pkg/Include/common_epit.h > > > > Rename to CamelCase? > > Can do. Would you prefer the iMX prefix to be in CamelCase too? (i.e. > Imx6Epit.h) I'm honestly not too fussed. My preference (which contradicts the coding style, mind you) is to keep names looking like they do in normal reading. So I would aesthetically prefer the former, whereas the coding style says the latter. You choose :) / Leif ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH edk2-platforms 14/27] Silicon/NXP: Add i.MX6 GPT and EPIT timer headers
Hi Leif,
> -Original Message-
> From: Leif Lindholm
> Sent: Thursday, November 8, 2018 10:14 AM
> To: Chris Co
> Cc: edk2-devel@lists.01.org; Ard Biesheuvel ;
> Michael D Kinney
> Subject: Re: [PATCH edk2-platforms 14/27] Silicon/NXP: Add i.MX6 GPT and
> EPIT timer headers
>
> On Fri, Sep 21, 2018 at 08:26:05AM +, Chris Co wrote:
> > This adds the definitions for the NXP i.MX6 General Purpose Timer and
> > the Enhanced Periodic Interrupt Timer modules.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Christopher Co
> > Cc: Ard Biesheuvel
> > Cc: Leif Lindholm
> > Cc: Michael D Kinney
> > ---
> > Silicon/NXP/iMX6Pkg/Include/common_epit.h | 118 +
> > Silicon/NXP/iMX6Pkg/Include/common_gpt.h | 271
>
> > 2 files changed, 389 insertions(+)
> >
> > diff --git a/Silicon/NXP/iMX6Pkg/Include/common_epit.h
> > b/Silicon/NXP/iMX6Pkg/Include/common_epit.h
> > new file mode 100644
> > index ..485d6ccbc51e
> > --- /dev/null
> > +++ b/Silicon/NXP/iMX6Pkg/Include/common_epit.h
>
> Rename to CamelCase?
>
Can do. Would you prefer the iMX prefix to be in CamelCase too? (i.e.
Imx6Epit.h)
> > @@ -0,0 +1,118 @@
> > +/** @file
> > +*
> > +* Provides definitions for the EPIT (Enhanced Periodic Interrupt
> > +Timer)
> > +* module that are common to Freescale SoCs.
> > +*
> > +* Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> > +* Copyright (c) 2004-2010, Freescale Semiconductor, Inc. All Rights
> Reserved.
> > +*
> > +* This program and the accompanying materials
> > +* are licensed and made available under the terms and conditions of
> > +the BSD License
> > +* which accompanies this distribution. The full text of the license
> > +may be found at
> > +*
> > +https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopens
> > +ource.org%2Flicenses%2Fbsd-
> license.phpdata=02%7C01%7CChristopher
> >
> +.Co%40microsoft.com%7C5da90dbbf4b5452a4c8e08d645a605cc%7C72f988
> bf86f1
> >
> +41af91ab2d7cd011db47%7C1%7C0%7C636772976703090578sdata=
> g3OOywqu7
> > +VDuq39F3PVs8SaXZVbA7h77F7riMUXb0lE%3Dreserved=0
> > +*
> > +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > +BASIS,
> > +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> > +*
> > +**/
> > +
> > +#ifndef __COMMON_EPIT_H
> > +#define __COMMON_EPIT_H
>
> COMMON is a bit too common. Can we at least stick an IMX on the front?
>
> This is really also a problem for all of the structs and #defines below as
> well.
>
> > +
> > +typedef struct {
> > + UINT32 CR;
> > + UINT32 SR;
> > + UINT32 LR;
> > + UINT32 CMPR;
> > + UINT32 CNT;
> > +} CSP_EPIT_REG, *PCSP_EPIT_REG;
>
> I'm really not a fan of typedef pointers. Please drop the *PSCP one.
>
> > +
> > +#define EPIT_CR_OFFSET 0x
> > +#define EPIT_SR_OFFSET 0x0004
> > +#define EPIT_LR_OFFSET 0x0008
> > +#define EPIT_CMPR_OFFSET0x000C
> > +#define EPIT_CNR_OFFSET 0x0010
> > +
> > +#define EPIT_CR_EN_LSH 0
>
> What is LSH?
>
> > +#define EPIT_CR_ENMOD_LSH 1
> > +#define EPIT_CR_OCIEN_LSH 2
> > +#define EPIT_CR_RLD_LSH 3
> > +#define EPIT_CR_PRESCALAR_LSH 4
> > +#define EPIT_CR_SWR_LSH 16
> > +#define EPIT_CR_IOVW_LSH17
> > +#define EPIT_CR_DBGEN_LSH 18
> > +#define EPIT_CR_WAITEN_LSH 19
> > +#define EPIT_CR_DOZEN_LSH 20
> > +#define EPIT_CR_STOPEN_LSH 21
> > +#define EPIT_CR_OM_LSH 22
> > +#define EPIT_CR_CLKSRC_LSH 24
> > +
> > +#define EPIT_SR_OCIF_LSH0
> > +#define EPIT_LR_LOAD_LSH0
> > +#define EPIT_CMPR_COMPARE_LSH 0
> > +#define EPIT_CNT_COUNT_LSH 0
> > +
> > +#define EPIT_CR_EN_WID 1
>
> What is WID?
>
LSH is the number of bits to left shift to get to the specified bitfield in the
register. WID is the length of the specified bitfield.
I don't like this register access pattern at all. Will define structs of the
registers instead...
Thanks,
Chris
> > +#define EPIT_CR_ENMOD_WID 1
> > +#define EPIT_CR_OCIEN_WID 2
> > +#define EPIT_CR_RLD_WID 1
> > +#define EPIT_CR_PRESCALAR_WID 12
> > +#define EPIT_CR_SWR_WID 1
> > +#define EPIT_CR_IOVW_WID1
> > +#define EPIT_CR_DBGEN_WID 1
&
Re: [edk2] [PATCH edk2-platforms 14/27] Silicon/NXP: Add i.MX6 GPT and EPIT timer headers
On Fri, Sep 21, 2018 at 08:26:05AM +, Chris Co wrote:
> This adds the definitions for the NXP i.MX6 General Purpose Timer
> and the Enhanced Periodic Interrupt Timer modules.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Christopher Co
> Cc: Ard Biesheuvel
> Cc: Leif Lindholm
> Cc: Michael D Kinney
> ---
> Silicon/NXP/iMX6Pkg/Include/common_epit.h | 118 +
> Silicon/NXP/iMX6Pkg/Include/common_gpt.h | 271
> 2 files changed, 389 insertions(+)
>
> diff --git a/Silicon/NXP/iMX6Pkg/Include/common_epit.h
> b/Silicon/NXP/iMX6Pkg/Include/common_epit.h
> new file mode 100644
> index ..485d6ccbc51e
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Include/common_epit.h
Rename to CamelCase?
> @@ -0,0 +1,118 @@
> +/** @file
> +*
> +* Provides definitions for the EPIT (Enhanced Periodic Interrupt Timer)
> +* module that are common to Freescale SoCs.
> +*
> +* Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> +* Copyright (c) 2004-2010, Freescale Semiconductor, Inc. All Rights
> Reserved.
> +*
> +* This program and the accompanying materials
> +* are licensed and made available under the terms and conditions of the BSD
> License
> +* which accompanies this distribution. The full text of the license may be
> found at
> +* http://opensource.org/licenses/bsd-license.php
> +*
> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
> IMPLIED.
> +*
> +**/
> +
> +#ifndef __COMMON_EPIT_H
> +#define __COMMON_EPIT_H
COMMON is a bit too common. Can we at least stick an IMX on the front?
This is really also a problem for all of the structs and #defines
below as well.
> +
> +typedef struct {
> + UINT32 CR;
> + UINT32 SR;
> + UINT32 LR;
> + UINT32 CMPR;
> + UINT32 CNT;
> +} CSP_EPIT_REG, *PCSP_EPIT_REG;
I'm really not a fan of typedef pointers. Please drop the *PSCP one.
> +
> +#define EPIT_CR_OFFSET 0x
> +#define EPIT_SR_OFFSET 0x0004
> +#define EPIT_LR_OFFSET 0x0008
> +#define EPIT_CMPR_OFFSET0x000C
> +#define EPIT_CNR_OFFSET 0x0010
> +
> +#define EPIT_CR_EN_LSH 0
What is LSH?
> +#define EPIT_CR_ENMOD_LSH 1
> +#define EPIT_CR_OCIEN_LSH 2
> +#define EPIT_CR_RLD_LSH 3
> +#define EPIT_CR_PRESCALAR_LSH 4
> +#define EPIT_CR_SWR_LSH 16
> +#define EPIT_CR_IOVW_LSH17
> +#define EPIT_CR_DBGEN_LSH 18
> +#define EPIT_CR_WAITEN_LSH 19
> +#define EPIT_CR_DOZEN_LSH 20
> +#define EPIT_CR_STOPEN_LSH 21
> +#define EPIT_CR_OM_LSH 22
> +#define EPIT_CR_CLKSRC_LSH 24
> +
> +#define EPIT_SR_OCIF_LSH0
> +#define EPIT_LR_LOAD_LSH0
> +#define EPIT_CMPR_COMPARE_LSH 0
> +#define EPIT_CNT_COUNT_LSH 0
> +
> +#define EPIT_CR_EN_WID 1
What is WID?
> +#define EPIT_CR_ENMOD_WID 1
> +#define EPIT_CR_OCIEN_WID 2
> +#define EPIT_CR_RLD_WID 1
> +#define EPIT_CR_PRESCALAR_WID 12
> +#define EPIT_CR_SWR_WID 1
> +#define EPIT_CR_IOVW_WID1
> +#define EPIT_CR_DBGEN_WID 1
> +#define EPIT_CR_WAITEN_WID 1
> +#define EPIT_CR_DOZEN_WID 1
> +#define EPIT_CR_STOPEN_WID 1
> +#define EPIT_CR_OM_WID 2
> +#define EPIT_CR_CLKSRC_WID 2
> +
> +#define EPIT_SR_OCIF_WID1
> +#define EPIT_LR_LOAD_WID32
> +#define EPIT_CMPR_COMPARE_WID 32
> +#define EPIT_CNT_COUNT_WID 32
> +
> +// CR
> +#define EPIT_CR_EN_DISABLE 0
> +#define EPIT_CR_EN_ENABLE 1
> +
> +#define EPIT_CR_ENMOD_RESUME0
> +#define EPIT_CR_ENMOD_LOAD 1
> +
> +#define EPIT_CR_OCIEN_DISABLE 0
> +#define EPIT_CR_OCIEN_ENABLE1
> +
> +#define EPIT_CR_RLD_ROLLOVER0
> +#define EPIT_CR_RLD_RELOAD 1
> +
> +#define EPIT_CR_SWR_NORESET 0
> +#define EPIT_CR_SWR_RESET 1
> +
> +#define EPIT_CR_IOVW_NOOVR 0
> +#define EPIT_CR_IOVW_OVR1
> +
> +#define EPIT_CR_DBGEN_INACTIVE 0
> +#define EPIT_CR_DBGEN_ACTIVE1
> +
> +#define EPIT_CR_WAITEN_DISABLE 0
> +#define EPIT_CR_WAITEN_ENABLE 1
> +
> +#define EPIT_CR_DOZEN_DISABLE 0
> +#define EPIT_CR_DOZEN_ENABLE1
> +
> +#define EPIT_CR_STOPEN_DISABLE 0
> +#define EPIT_CR_STOPEN_ENABLE 1
> +
> +#define EPIT_CR_OM_DICONNECT0
> +#define EPIT_CR_OM_TOGGLE 1
> +#define EPIT_CR_OM_CLEAR2
> +#define EPIT_CR_OM_SET 3
> +
> +#define EPIT_CR_CLKSRC_OFF 0
> +#define EPIT_CR_CLKSRC_IPGCLK 1
> +#define EPIT_CR_CLKSRC_HIGHFREQ 2 // High freq is sourcing from PERCLK
> +#define EPIT_CR_CLKSRC_CKIL 3
> +
> +// CNT
> +#define EPIT_CNT_COUNT_MAX 0x
MAX_UINT32?
> +
> +#endif // __COMMON_EPIT_H
> diff --git a/Silicon/NXP/iMX6Pkg/Include/common_gpt.h
> b/Silicon/NXP/iMX6Pkg/Include/common_gpt.h
> new file mode 100644
[edk2] [PATCH edk2-platforms 14/27] Silicon/NXP: Add i.MX6 GPT and EPIT timer headers
This adds the definitions for the NXP i.MX6 General Purpose Timer
and the Enhanced Periodic Interrupt Timer modules.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Christopher Co
Cc: Ard Biesheuvel
Cc: Leif Lindholm
Cc: Michael D Kinney
---
Silicon/NXP/iMX6Pkg/Include/common_epit.h | 118 +
Silicon/NXP/iMX6Pkg/Include/common_gpt.h | 271
2 files changed, 389 insertions(+)
diff --git a/Silicon/NXP/iMX6Pkg/Include/common_epit.h
b/Silicon/NXP/iMX6Pkg/Include/common_epit.h
new file mode 100644
index ..485d6ccbc51e
--- /dev/null
+++ b/Silicon/NXP/iMX6Pkg/Include/common_epit.h
@@ -0,0 +1,118 @@
+/** @file
+*
+* Provides definitions for the EPIT (Enhanced Periodic Interrupt Timer)
+* module that are common to Freescale SoCs.
+*
+* Copyright (c) 2018 Microsoft Corporation. All rights reserved.
+* Copyright (c) 2004-2010, Freescale Semiconductor, Inc. All Rights Reserved.
+*
+* This program and the accompanying materials
+* are licensed and made available under the terms and conditions of the BSD
License
+* which accompanies this distribution. The full text of the license may be
found at
+* http://opensource.org/licenses/bsd-license.php
+*
+* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR
IMPLIED.
+*
+**/
+
+#ifndef __COMMON_EPIT_H
+#define __COMMON_EPIT_H
+
+typedef struct {
+ UINT32 CR;
+ UINT32 SR;
+ UINT32 LR;
+ UINT32 CMPR;
+ UINT32 CNT;
+} CSP_EPIT_REG, *PCSP_EPIT_REG;
+
+#define EPIT_CR_OFFSET 0x
+#define EPIT_SR_OFFSET 0x0004
+#define EPIT_LR_OFFSET 0x0008
+#define EPIT_CMPR_OFFSET0x000C
+#define EPIT_CNR_OFFSET 0x0010
+
+#define EPIT_CR_EN_LSH 0
+#define EPIT_CR_ENMOD_LSH 1
+#define EPIT_CR_OCIEN_LSH 2
+#define EPIT_CR_RLD_LSH 3
+#define EPIT_CR_PRESCALAR_LSH 4
+#define EPIT_CR_SWR_LSH 16
+#define EPIT_CR_IOVW_LSH17
+#define EPIT_CR_DBGEN_LSH 18
+#define EPIT_CR_WAITEN_LSH 19
+#define EPIT_CR_DOZEN_LSH 20
+#define EPIT_CR_STOPEN_LSH 21
+#define EPIT_CR_OM_LSH 22
+#define EPIT_CR_CLKSRC_LSH 24
+
+#define EPIT_SR_OCIF_LSH0
+#define EPIT_LR_LOAD_LSH0
+#define EPIT_CMPR_COMPARE_LSH 0
+#define EPIT_CNT_COUNT_LSH 0
+
+#define EPIT_CR_EN_WID 1
+#define EPIT_CR_ENMOD_WID 1
+#define EPIT_CR_OCIEN_WID 2
+#define EPIT_CR_RLD_WID 1
+#define EPIT_CR_PRESCALAR_WID 12
+#define EPIT_CR_SWR_WID 1
+#define EPIT_CR_IOVW_WID1
+#define EPIT_CR_DBGEN_WID 1
+#define EPIT_CR_WAITEN_WID 1
+#define EPIT_CR_DOZEN_WID 1
+#define EPIT_CR_STOPEN_WID 1
+#define EPIT_CR_OM_WID 2
+#define EPIT_CR_CLKSRC_WID 2
+
+#define EPIT_SR_OCIF_WID1
+#define EPIT_LR_LOAD_WID32
+#define EPIT_CMPR_COMPARE_WID 32
+#define EPIT_CNT_COUNT_WID 32
+
+// CR
+#define EPIT_CR_EN_DISABLE 0
+#define EPIT_CR_EN_ENABLE 1
+
+#define EPIT_CR_ENMOD_RESUME0
+#define EPIT_CR_ENMOD_LOAD 1
+
+#define EPIT_CR_OCIEN_DISABLE 0
+#define EPIT_CR_OCIEN_ENABLE1
+
+#define EPIT_CR_RLD_ROLLOVER0
+#define EPIT_CR_RLD_RELOAD 1
+
+#define EPIT_CR_SWR_NORESET 0
+#define EPIT_CR_SWR_RESET 1
+
+#define EPIT_CR_IOVW_NOOVR 0
+#define EPIT_CR_IOVW_OVR1
+
+#define EPIT_CR_DBGEN_INACTIVE 0
+#define EPIT_CR_DBGEN_ACTIVE1
+
+#define EPIT_CR_WAITEN_DISABLE 0
+#define EPIT_CR_WAITEN_ENABLE 1
+
+#define EPIT_CR_DOZEN_DISABLE 0
+#define EPIT_CR_DOZEN_ENABLE1
+
+#define EPIT_CR_STOPEN_DISABLE 0
+#define EPIT_CR_STOPEN_ENABLE 1
+
+#define EPIT_CR_OM_DICONNECT0
+#define EPIT_CR_OM_TOGGLE 1
+#define EPIT_CR_OM_CLEAR2
+#define EPIT_CR_OM_SET 3
+
+#define EPIT_CR_CLKSRC_OFF 0
+#define EPIT_CR_CLKSRC_IPGCLK 1
+#define EPIT_CR_CLKSRC_HIGHFREQ 2 // High freq is sourcing from PERCLK
+#define EPIT_CR_CLKSRC_CKIL 3
+
+// CNT
+#define EPIT_CNT_COUNT_MAX 0x
+
+#endif // __COMMON_EPIT_H
diff --git a/Silicon/NXP/iMX6Pkg/Include/common_gpt.h
b/Silicon/NXP/iMX6Pkg/Include/common_gpt.h
new file mode 100644
index ..7fdfc25d819f
--- /dev/null
+++ b/Silicon/NXP/iMX6Pkg/Include/common_gpt.h
@@ -0,0 +1,271 @@
+/** @file
+*
+* Provides definitions for the GPT (General Purpose Timer) module
+* that are common to Freescale SoCs.
+*
+* Copyright (c) 2018 Microsoft Corporation. All rights reserved.
+* Copyright (c) 2004-2010, Freescale Semiconductor, Inc. All Rights Reserved.
+*
+* This program and the accompanying materials
+* are licensed and made available under the terms and conditions of the BSD
License
+* which accompanies this distribution. The full text of the license may be
found at
+*
