[1/2] dt-bindings: clock: am33xx: Update GPIO number as per datasheet
diff mbox series

Message ID 20190912014849.10684-1-ankur.tyagi@gallagher.com
State Needs Review / ACK
Headers show
Series
  • [1/2] dt-bindings: clock: am33xx: Update GPIO number as per datasheet
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Ankur Tyagi Sept. 12, 2019, 1:48 a.m. UTC
Sitara technical reference manual numbers GPIO from 0-3 whereas
in code GPIO are numbered from 1-4.

Signed-off-by: Ankur Tyagi <ankur.tyagi@gallagher.com>
---
 include/dt-bindings/clock/am3.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Tero Kristo Sept. 12, 2019, 7:35 a.m. UTC | #1
On 12/09/2019 04:48, Ankur Tyagi wrote:
> Sitara technical reference manual numbers GPIO from 0-3 whereas
> in code GPIO are numbered from 1-4.
> 

You are right that TRM states these are 0-3, but just to change the 
numbering at this point seems unnecessary churn for the minor 
inconvenience it causes. Also, I think we have similar indexing issues 
all over the place, should we fix them all? At least am4 has exact same 
indexing issue for gpios. They have been historically called 1-4 since 
introduction to linux kernel some 6 years already (as hwmod data). :)

Also, your patches cause bisect break points, try to apply this patch 
only to kernel and try to compile and see what happens.... and you don't 
touch the DT side for these either which causes another compile breakage.

So, from my side, NAK for both patches, I can't see why we should do 
such a small change and cause so many problems... not to mention the 
compile issues.

Do you have some specific reason why you want these changed?

-Tero

> Signed-off-by: Ankur Tyagi <ankur.tyagi@gallagher.com>
> ---
>   include/dt-bindings/clock/am3.h | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/dt-bindings/clock/am3.h b/include/dt-bindings/clock/am3.h
> index 894951541276..980fdc05c3d0 100644
> --- a/include/dt-bindings/clock/am3.h
> +++ b/include/dt-bindings/clock/am3.h
> @@ -41,9 +41,9 @@
>   #define AM3_RNG_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0x90)
>   #define AM3_AES_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0x94)
>   #define AM3_SHAM_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xa0)
> -#define AM3_GPIO2_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xac)
> -#define AM3_GPIO3_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xb0)
> -#define AM3_GPIO4_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xb4)
> +#define AM3_GPIO1_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xac)
> +#define AM3_GPIO2_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xb0)
> +#define AM3_GPIO3_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xb4)
>   #define AM3_TPCC_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xbc)
>   #define AM3_D_CAN0_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xc0)
>   #define AM3_D_CAN1_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xc4)
> @@ -69,7 +69,7 @@
>   #define AM3_L4_WKUP_CLKCTRL_OFFSET	0x4
>   #define AM3_L4_WKUP_CLKCTRL_INDEX(offset)	((offset) - AM3_L4_WKUP_CLKCTRL_OFFSET)
>   #define AM3_CONTROL_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0x4)
> -#define AM3_GPIO1_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0x8)
> +#define AM3_GPIO0_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0x8)
>   #define AM3_L4_WKUP_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0xc)
>   #define AM3_DEBUGSS_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0x14)
>   #define AM3_WKUP_M3_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0xb0)
> @@ -121,9 +121,9 @@
>   #define AM3_L4LS_TIMER3_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0x84)
>   #define AM3_L4LS_TIMER4_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0x88)
>   #define AM3_L4LS_RNG_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0x90)
> -#define AM3_L4LS_GPIO2_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xac)
> -#define AM3_L4LS_GPIO3_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xb0)
> -#define AM3_L4LS_GPIO4_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xb4)
> +#define AM3_L4LS_GPIO1_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xac)
> +#define AM3_L4LS_GPIO2_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xb0)
> +#define AM3_L4LS_GPIO3_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xb4)
>   #define AM3_L4LS_D_CAN0_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xc0)
>   #define AM3_L4LS_D_CAN1_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xc4)
>   #define AM3_L4LS_EPWMSS1_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xcc)
> @@ -184,7 +184,7 @@
>   
>   /* l4_wkup clocks */
>   #define AM3_L4_WKUP_CONTROL_CLKCTRL	AM3_CLKCTRL_INDEX(0x4)
> -#define AM3_L4_WKUP_GPIO1_CLKCTRL	AM3_CLKCTRL_INDEX(0x8)
> +#define AM3_L4_WKUP_GPIO0_CLKCTRL	AM3_CLKCTRL_INDEX(0x8)
>   #define AM3_L4_WKUP_L4_WKUP_CLKCTRL	AM3_CLKCTRL_INDEX(0xc)
>   #define AM3_L4_WKUP_UART1_CLKCTRL	AM3_CLKCTRL_INDEX(0xb4)
>   #define AM3_L4_WKUP_I2C1_CLKCTRL	AM3_CLKCTRL_INDEX(0xb8)
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Ankur Tyagi Sept. 12, 2019, 11:57 p.m. UTC | #2
> -----Original Message-----
> From: Tero Kristo <t-kristo@ti.com>
> Sent: Thursday, 12 September 2019 7:36 PM
> To: Ankur Tyagi <Ankur.Tyagi@gallagher.com>; mark.rutland@arm.com;
> robh+dt@kernel.org
> Cc: devicetree@vger.kernel.org
> Subject: Re: [PATCH 1/2] dt-bindings: clock: am33xx: Update GPIO number as
> per datasheet
>
> On 12/09/2019 04:48, Ankur Tyagi wrote:
> > Sitara technical reference manual numbers GPIO from 0-3 whereas
> > in code GPIO are numbered from 1-4.
> >
>
> You are right that TRM states these are 0-3, but just to change the
> numbering at this point seems unnecessary churn for the minor
> inconvenience it causes. Also, I think we have similar indexing issues
> all over the place, should we fix them all? At least am4 has exact same
> indexing issue for gpios. They have been historically called 1-4 since
> introduction to linux kernel some 6 years already (as hwmod data). :)

I know it is a minor inconvenience once you figure it out 😊
One of our product is based upon am335x and I was writing a driver
which needed to read GPIO2 memory and was causing kernel panic as GPIO2 OCP clock
and fclk is not enabled by default. I looked inside am33xx-clocks.dtsi and found
"gpio0_dbclk_mux_ck" and my brain wired itself for numbering 0-3 and grabbed
AM3_GPIO2_CLKCTRL but clock was not getting enabled. Then after some debugging found
out numbering issue. And this can happen with anyone that's why thought of fixing it 😊

> Also, your patches cause bisect break points, try to apply this patch
> only to kernel and try to compile and see what happens.... and you don't
> touch the DT side for these either which causes another compile breakage.

I actually sent DT fix in separate mailing list, will resend everything in same list
to keep things simple. I will recheck the compilation issue, it worked in my setup

> So, from my side, NAK for both patches, I can't see why we should do
> such a small change and cause so many problems... not to mention the
> compile issues.
>
> Do you have some specific reason why you want these changed?
>
> -Tero

Regards
Ankur
>
> > Signed-off-by: Ankur Tyagi <ankur.tyagi@gallagher.com>
> > ---
> >   include/dt-bindings/clock/am3.h | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/dt-bindings/clock/am3.h b/include/dt-
> bindings/clock/am3.h
> > index 894951541276..980fdc05c3d0 100644
> > --- a/include/dt-bindings/clock/am3.h
> > +++ b/include/dt-bindings/clock/am3.h
> > @@ -41,9 +41,9 @@
> >   #define AM3_RNG_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0x90)
> >   #define AM3_AES_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0x94)
> >   #define AM3_SHAM_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0xa0)
> > -#define AM3_GPIO2_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0xac)
> > -#define AM3_GPIO3_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0xb0)
> > -#define AM3_GPIO4_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0xb4)
> > +#define AM3_GPIO1_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0xac)
> > +#define AM3_GPIO2_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0xb0)
> > +#define AM3_GPIO3_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0xb4)
> >   #define AM3_TPCC_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0xbc)
> >   #define AM3_D_CAN0_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0xc0)
> >   #define AM3_D_CAN1_CLKCTRLAM3_L4_PER_CLKCTRL_INDEX(0xc4)
> > @@ -69,7 +69,7 @@
> >   #define AM3_L4_WKUP_CLKCTRL_OFFSET0x4
> >   #define AM3_L4_WKUP_CLKCTRL_INDEX(offset)((offset) -
> AM3_L4_WKUP_CLKCTRL_OFFSET)
> >   #define AM3_CONTROL_CLKCTRL
> AM3_L4_WKUP_CLKCTRL_INDEX(0x4)
> > -#define AM3_GPIO1_CLKCTRL
> AM3_L4_WKUP_CLKCTRL_INDEX(0x8)
> > +#define AM3_GPIO0_CLKCTRL
> AM3_L4_WKUP_CLKCTRL_INDEX(0x8)
> >   #define AM3_L4_WKUP_CLKCTRL
> AM3_L4_WKUP_CLKCTRL_INDEX(0xc)
> >   #define AM3_DEBUGSS_CLKCTRL
> AM3_L4_WKUP_CLKCTRL_INDEX(0x14)
> >   #define AM3_WKUP_M3_CLKCTRL
> AM3_L4_WKUP_CLKCTRL_INDEX(0xb0)
> > @@ -121,9 +121,9 @@
> >   #define AM3_L4LS_TIMER3_CLKCTRL
> AM3_L4LS_CLKCTRL_INDEX(0x84)
> >   #define AM3_L4LS_TIMER4_CLKCTRL
> AM3_L4LS_CLKCTRL_INDEX(0x88)
> >   #define AM3_L4LS_RNG_CLKCTRLAM3_L4LS_CLKCTRL_INDEX(0x90)
> > -#define AM3_L4LS_GPIO2_CLKCTRLAM3_L4LS_CLKCTRL_INDEX(0xac)
> > -#define AM3_L4LS_GPIO3_CLKCTRLAM3_L4LS_CLKCTRL_INDEX(0xb0)
> > -#define AM3_L4LS_GPIO4_CLKCTRLAM3_L4LS_CLKCTRL_INDEX(0xb4)
> > +#define AM3_L4LS_GPIO1_CLKCTRL
> AM3_L4LS_CLKCTRL_INDEX(0xac)
> > +#define AM3_L4LS_GPIO2_CLKCTRL
> AM3_L4LS_CLKCTRL_INDEX(0xb0)
> > +#define AM3_L4LS_GPIO3_CLKCTRL
> AM3_L4LS_CLKCTRL_INDEX(0xb4)
> >   #define AM3_L4LS_D_CAN0_CLKCTRL
> AM3_L4LS_CLKCTRL_INDEX(0xc0)
> >   #define AM3_L4LS_D_CAN1_CLKCTRL
> AM3_L4LS_CLKCTRL_INDEX(0xc4)
> >   #define AM3_L4LS_EPWMSS1_CLKCTRL
> AM3_L4LS_CLKCTRL_INDEX(0xcc)
> > @@ -184,7 +184,7 @@
> >
> >   /* l4_wkup clocks */
> >   #define AM3_L4_WKUP_CONTROL_CLKCTRL
> AM3_CLKCTRL_INDEX(0x4)
> > -#define AM3_L4_WKUP_GPIO1_CLKCTRLAM3_CLKCTRL_INDEX(0x8)
> > +#define AM3_L4_WKUP_GPIO0_CLKCTRLAM3_CLKCTRL_INDEX(0x8)
> >   #define AM3_L4_WKUP_L4_WKUP_CLKCTRL
> AM3_CLKCTRL_INDEX(0xc)
> >   #define AM3_L4_WKUP_UART1_CLKCTRLAM3_CLKCTRL_INDEX(0xb4)
> >   #define AM3_L4_WKUP_I2C1_CLKCTRLAM3_CLKCTRL_INDEX(0xb8)
> >
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-
> tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Patch
diff mbox series

diff --git a/include/dt-bindings/clock/am3.h b/include/dt-bindings/clock/am3.h
index 894951541276..980fdc05c3d0 100644
--- a/include/dt-bindings/clock/am3.h
+++ b/include/dt-bindings/clock/am3.h
@@ -41,9 +41,9 @@ 
 #define AM3_RNG_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0x90)
 #define AM3_AES_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0x94)
 #define AM3_SHAM_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xa0)
-#define AM3_GPIO2_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xac)
-#define AM3_GPIO3_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xb0)
-#define AM3_GPIO4_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xb4)
+#define AM3_GPIO1_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xac)
+#define AM3_GPIO2_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xb0)
+#define AM3_GPIO3_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xb4)
 #define AM3_TPCC_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xbc)
 #define AM3_D_CAN0_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xc0)
 #define AM3_D_CAN1_CLKCTRL	AM3_L4_PER_CLKCTRL_INDEX(0xc4)
@@ -69,7 +69,7 @@ 
 #define AM3_L4_WKUP_CLKCTRL_OFFSET	0x4
 #define AM3_L4_WKUP_CLKCTRL_INDEX(offset)	((offset) - AM3_L4_WKUP_CLKCTRL_OFFSET)
 #define AM3_CONTROL_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0x4)
-#define AM3_GPIO1_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0x8)
+#define AM3_GPIO0_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0x8)
 #define AM3_L4_WKUP_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0xc)
 #define AM3_DEBUGSS_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0x14)
 #define AM3_WKUP_M3_CLKCTRL	AM3_L4_WKUP_CLKCTRL_INDEX(0xb0)
@@ -121,9 +121,9 @@ 
 #define AM3_L4LS_TIMER3_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0x84)
 #define AM3_L4LS_TIMER4_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0x88)
 #define AM3_L4LS_RNG_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0x90)
-#define AM3_L4LS_GPIO2_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xac)
-#define AM3_L4LS_GPIO3_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xb0)
-#define AM3_L4LS_GPIO4_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xb4)
+#define AM3_L4LS_GPIO1_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xac)
+#define AM3_L4LS_GPIO2_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xb0)
+#define AM3_L4LS_GPIO3_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xb4)
 #define AM3_L4LS_D_CAN0_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xc0)
 #define AM3_L4LS_D_CAN1_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xc4)
 #define AM3_L4LS_EPWMSS1_CLKCTRL	AM3_L4LS_CLKCTRL_INDEX(0xcc)
@@ -184,7 +184,7 @@ 
 
 /* l4_wkup clocks */
 #define AM3_L4_WKUP_CONTROL_CLKCTRL	AM3_CLKCTRL_INDEX(0x4)
-#define AM3_L4_WKUP_GPIO1_CLKCTRL	AM3_CLKCTRL_INDEX(0x8)
+#define AM3_L4_WKUP_GPIO0_CLKCTRL	AM3_CLKCTRL_INDEX(0x8)
 #define AM3_L4_WKUP_L4_WKUP_CLKCTRL	AM3_CLKCTRL_INDEX(0xc)
 #define AM3_L4_WKUP_UART1_CLKCTRL	AM3_CLKCTRL_INDEX(0xb4)
 #define AM3_L4_WKUP_I2C1_CLKCTRL	AM3_CLKCTRL_INDEX(0xb8)