Re: Some arm cleanups suggested by clang

[Mark Kettenis]

> Date: Sat, 24 Sep 2016 14:07:00 +1000
> From: Jonathan Gray 
> 
> On Fri, Sep 23, 2016 at 01:43:04PM +0200, Mark Kettenis wrote:
> > Index: arch/arm/arm/cpu.c
> > ===
> > RCS file: /cvs/src/sys/arch/arm/arm/cpu.c,v
> > retrieving revision 1.32
> > diff -u -p -r1.32 cpu.c
> > --- arch/arm/arm/cpu.c  14 Aug 2016 11:30:54 -  1.32
> > +++ arch/arm/arm/cpu.c  23 Sep 2016 11:32:51 -
> > @@ -103,16 +103,6 @@ static const char * const pxa2x0_steppin
> > "rev 12",   "rev 13",   "rev 14",   "rev 15"
> >  };
> >  
> > -/* Steppings for PXA255/26x.
> > - * rev 5: PXA26x B0, rev 6: PXA255 A0
> > - */
> > -static const char * const pxa255_steppings[16] = {
> > -   "rev 0","rev 1","rev 2","step A-0",
> > -   "rev 4","step B-0", "step A-0", "rev 7",
> > -   "rev 8","rev 9","rev 10",   "rev 11",
> > -   "rev 12",   "rev 13",   "rev 14",   "rev 15"
> > -};
> 
> Why not just remove all the pxa/xscale bits from cpu/cpufunc?

I committed the previous diff.  So here is a diff that removes the
pxa/xscale bits.  As a bonus I added an ARMv8 CPU class, such that we
don't misidentify those as ARMv7.

ok?


Index: arch/arm/arm/cpu.c
===
RCS file: /cvs/src/sys/arch/arm/arm/cpu.c,v
retrieving revision 1.33
diff -u -p -r1.33 cpu.c
--- arch/arm/arm/cpu.c  24 Sep 2016 13:03:47 -  1.33
+++ arch/arm/arm/cpu.c  24 Sep 2016 13:27:05 -
@@ -84,8 +84,8 @@ cpu_attach(struct device *dv)
 
 enum cpu_class {
CPU_CLASS_NONE,
-   CPU_CLASS_XSCALE,
-   CPU_CLASS_ARMv7
+   CPU_CLASS_ARMv7,
+   CPU_CLASS_ARMv8
 };
 
 static const char * const generic_steppings[16] = {
@@ -95,22 +95,6 @@ static const char * const generic_steppi
"rev 12",   "rev 13",   "rev 14",   "rev 15"
 };
 
-/* Steppings for PXA2[15]0 */
-static const char * const pxa2x0_steppings[16] = {
-   "step A-0", "step A-1", "step B-0", "step B-1",
-   "step B-2", "step C-0", "rev 6","rev 7",
-   "rev 8","rev 9","rev 10",   "rev 11",
-   "rev 12",   "rev 13",   "rev 14",   "rev 15"
-};
-
-/* Steppings for PXA270 */
-static const char * const pxa27x_steppings[16] = {
-   "step A-0", "step A-1", "step B-0", "step B-1",
-   "step C-0", "step ?",   "step ?",   "step C-5",
-   "rev 8","rev 9","rev 10",   "rev 11",
-   "rev 12",   "rev 13",   "rev 14",   "rev 15"
-};
-
 struct cpuidtab {
u_int32_t   cpuid;
enumcpu_class cpu_class;
@@ -119,21 +103,6 @@ struct cpuidtab {
 };
 
 const struct cpuidtab cpuids[] = {
-   { CPU_ID_PXA250A,   CPU_CLASS_XSCALE,   "PXA250",
- pxa2x0_steppings },
-   { CPU_ID_PXA210A,   CPU_CLASS_XSCALE,   "PXA210",
- pxa2x0_steppings },
-   { CPU_ID_PXA250B,   CPU_CLASS_XSCALE,   "PXA250",
- pxa2x0_steppings },
-   { CPU_ID_PXA210B,   CPU_CLASS_XSCALE,   "PXA210",
- pxa2x0_steppings },
-   { CPU_ID_PXA250C,   CPU_CLASS_XSCALE,   "PXA250",
- pxa2x0_steppings },
-   { CPU_ID_PXA27X,CPU_CLASS_XSCALE,   "PXA27x",
- pxa27x_steppings },
-   { CPU_ID_PXA210C,   CPU_CLASS_XSCALE,   "PXA210",
- pxa2x0_steppings },
-
{ CPU_ID_CORTEX_A5, CPU_CLASS_ARMv7,"ARM Cortex A5",
  generic_steppings },
{ CPU_ID_CORTEX_A7, CPU_CLASS_ARMv7,"ARM Cortex A7",
@@ -171,21 +140,21 @@ const struct cpuidtab cpuids[] = {
{ CPU_ID_CORTEX_A17_R1, CPU_CLASS_ARMv7,"ARM Cortex A17 R1",
  generic_steppings },
 
-   { CPU_ID_CORTEX_A35,CPU_CLASS_ARMv7,"ARM Cortex A35",
+   { CPU_ID_CORTEX_A35,CPU_CLASS_ARMv8,"ARM Cortex A35",
  generic_steppings },
-   { CPU_ID_CORTEX_A53,CPU_CLASS_ARMv7,"ARM Cortex A53",
+   { CPU_ID_CORTEX_A53,CPU_CLASS_ARMv8,"ARM Cortex A53",
  generic_steppings },
-   { CPU_ID_CORTEX_A53_R1, CPU_CLASS_ARMv7,"ARM Cortex A53 R1",
+   { CPU_ID_CORTEX_A53_R1, CPU_CLASS_ARMv8,"ARM Cortex A53 R1",
  generic_steppings },
-   { CPU_ID_CORTEX_A57,CPU_CLASS_ARMv7,"ARM Cortex A57",
+   { CPU_ID_CORTEX_A57,CPU_CLASS_ARMv8,"ARM Cortex A57",
  generic_steppings },
-   { CPU_ID_CORTEX_A57_R1, CPU_CLASS_ARMv7,"ARM Cortex A57 R1",
+   { CPU_ID_CORTEX_A57_R1, CPU_CLASS_ARMv8,"ARM Cortex A57 R1",
  generic_steppings },
-   { CPU_ID_CORTEX_A72,CPU_CLASS_ARMv7,"ARM Cortex A72",
+   { CPU_ID_CORTEX_A72,CPU_CLASS_ARMv8,"ARM Cortex A72",
  generic_steppings },
-   { CPU_ID_CORTEX_A72_R1, 

Re: Some arm cleanups suggested by clang

[Jonathan Gray]

On Fri, Sep 23, 2016 at 01:43:04PM +0200, Mark Kettenis wrote:
> Most of these are warnings about static symbols that aren't used.  The
> pmap_get_pde_pte() bit fixes:
> 
> ../../../../arch/arm/arm/pmap7.c:2220:10: warning: comparison of array
>   'pm->pm_l2' equal to a null pointer is always false
>   [-Wtautological-pointer-compare]
> if (pm->pm_l2 == NULL)
> 
> ok?
> 
> 
> Index: arch/arm/arm/cpu.c
> ===
> RCS file: /cvs/src/sys/arch/arm/arm/cpu.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 cpu.c
> --- arch/arm/arm/cpu.c14 Aug 2016 11:30:54 -  1.32
> +++ arch/arm/arm/cpu.c23 Sep 2016 11:32:51 -
> @@ -103,16 +103,6 @@ static const char * const pxa2x0_steppin
>   "rev 12",   "rev 13",   "rev 14",   "rev 15"
>  };
>  
> -/* Steppings for PXA255/26x.
> - * rev 5: PXA26x B0, rev 6: PXA255 A0
> - */
> -static const char * const pxa255_steppings[16] = {
> - "rev 0","rev 1","rev 2","step A-0",
> - "rev 4","step B-0", "step A-0", "rev 7",
> - "rev 8","rev 9","rev 10",   "rev 11",
> - "rev 12",   "rev 13",   "rev 14",   "rev 15"
> -};

Why not just remove all the pxa/xscale bits from cpu/cpufunc?

Anyway removing this and the making the other changes is ok jsg@

> -
>  /* Steppings for PXA270 */
>  static const char * const pxa27x_steppings[16] = {
>   "step A-0", "step A-1", "step B-0", "step B-1",
> Index: arch/arm/arm/pmap7.c
> ===
> RCS file: /cvs/src/sys/arch/arm/arm/pmap7.c,v
> retrieving revision 1.52
> diff -u -p -r1.52 pmap7.c
> --- arch/arm/arm/pmap7.c  15 Sep 2016 02:00:17 -  1.52
> +++ arch/arm/arm/pmap7.c  23 Sep 2016 11:32:51 -
> @@ -446,26 +446,12 @@ pmap_tlb_flushID_SE(pmap_t pm, vaddr_t v
>  }
>  
>  static __inline void
> -pmap_tlb_flushD_SE(pmap_t pm, vaddr_t va)
> -{
> - if (pmap_is_current(pm))
> - cpu_tlb_flushD_SE(va);
> -}
> -
> -static __inline void
>  pmap_tlb_flushID(pmap_t pm)
>  {
>   if (pmap_is_current(pm))
>   cpu_tlb_flushID();
>  }
>  
> -static __inline void
> -pmap_tlb_flushD(pmap_t pm)
> -{
> - if (pmap_is_current(pm))
> - cpu_tlb_flushD();
> -}
> -
>  /*
>   * Returns a pointer to the L2 bucket associated with the specified pmap
>   * and VA, or NULL if no L2 bucket exists for the address.
> @@ -2217,11 +2203,7 @@ pmap_get_pde_pte(pmap_t pm, vaddr_t va, 
>   return (TRUE);
>   }
>  
> - if (pm->pm_l2 == NULL)
> - return (FALSE);
> -
>   l2 = pm->pm_l2[L2_IDX(l1idx)];
> -
>   if (l2 == NULL ||
>   (ptep = l2->l2_bucket[L2_BUCKET(l1idx)].l2b_kva) == NULL) {
>   return (FALSE);
> Index: arch/arm/cortex/agtimer.c
> ===
> RCS file: /cvs/src/sys/arch/arm/cortex/agtimer.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 agtimer.c
> --- arch/arm/cortex/agtimer.c 10 Aug 2016 06:51:57 -  1.7
> +++ arch/arm/cortex/agtimer.c 23 Sep 2016 11:32:51 -
> @@ -126,16 +126,6 @@ agtimer_set_ctrl(uint32_t val)
>  }
>  
>  static inline int
> -agtimer_get_tval(void)
> -{
> - uint32_t val;
> -
> - __asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
> -
> - return (val);
> -}
> -
> -static inline int
>  agtimer_set_tval(uint32_t val)
>  {
>   __asm volatile("mcr p15, 0, %[val], c14, c2, 0" : :
> 

Some arm cleanups suggested by clang

[Mark Kettenis]

Most of these are warnings about static symbols that aren't used.  The
pmap_get_pde_pte() bit fixes:

../../../../arch/arm/arm/pmap7.c:2220:10: warning: comparison of array
  'pm->pm_l2' equal to a null pointer is always false
  [-Wtautological-pointer-compare]
if (pm->pm_l2 == NULL)

ok?


Index: arch/arm/arm/cpu.c
===
RCS file: /cvs/src/sys/arch/arm/arm/cpu.c,v
retrieving revision 1.32
diff -u -p -r1.32 cpu.c
--- arch/arm/arm/cpu.c  14 Aug 2016 11:30:54 -  1.32
+++ arch/arm/arm/cpu.c  23 Sep 2016 11:32:51 -
@@ -103,16 +103,6 @@ static const char * const pxa2x0_steppin
"rev 12",   "rev 13",   "rev 14",   "rev 15"
 };
 
-/* Steppings for PXA255/26x.
- * rev 5: PXA26x B0, rev 6: PXA255 A0
- */
-static const char * const pxa255_steppings[16] = {
-   "rev 0","rev 1","rev 2","step A-0",
-   "rev 4","step B-0", "step A-0", "rev 7",
-   "rev 8","rev 9","rev 10",   "rev 11",
-   "rev 12",   "rev 13",   "rev 14",   "rev 15"
-};
-
 /* Steppings for PXA270 */
 static const char * const pxa27x_steppings[16] = {
"step A-0", "step A-1", "step B-0", "step B-1",
Index: arch/arm/arm/pmap7.c
===
RCS file: /cvs/src/sys/arch/arm/arm/pmap7.c,v
retrieving revision 1.52
diff -u -p -r1.52 pmap7.c
--- arch/arm/arm/pmap7.c15 Sep 2016 02:00:17 -  1.52
+++ arch/arm/arm/pmap7.c23 Sep 2016 11:32:51 -
@@ -446,26 +446,12 @@ pmap_tlb_flushID_SE(pmap_t pm, vaddr_t v
 }
 
 static __inline void
-pmap_tlb_flushD_SE(pmap_t pm, vaddr_t va)
-{
-   if (pmap_is_current(pm))
-   cpu_tlb_flushD_SE(va);
-}
-
-static __inline void
 pmap_tlb_flushID(pmap_t pm)
 {
if (pmap_is_current(pm))
cpu_tlb_flushID();
 }
 
-static __inline void
-pmap_tlb_flushD(pmap_t pm)
-{
-   if (pmap_is_current(pm))
-   cpu_tlb_flushD();
-}
-
 /*
  * Returns a pointer to the L2 bucket associated with the specified pmap
  * and VA, or NULL if no L2 bucket exists for the address.
@@ -2217,11 +2203,7 @@ pmap_get_pde_pte(pmap_t pm, vaddr_t va, 
return (TRUE);
}
 
-   if (pm->pm_l2 == NULL)
-   return (FALSE);
-
l2 = pm->pm_l2[L2_IDX(l1idx)];
-
if (l2 == NULL ||
(ptep = l2->l2_bucket[L2_BUCKET(l1idx)].l2b_kva) == NULL) {
return (FALSE);
Index: arch/arm/cortex/agtimer.c
===
RCS file: /cvs/src/sys/arch/arm/cortex/agtimer.c,v
retrieving revision 1.7
diff -u -p -r1.7 agtimer.c
--- arch/arm/cortex/agtimer.c   10 Aug 2016 06:51:57 -  1.7
+++ arch/arm/cortex/agtimer.c   23 Sep 2016 11:32:51 -
@@ -126,16 +126,6 @@ agtimer_set_ctrl(uint32_t val)
 }
 
 static inline int
-agtimer_get_tval(void)
-{
-   uint32_t val;
-
-   __asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
-
-   return (val);
-}
-
-static inline int
 agtimer_set_tval(uint32_t val)
 {
__asm volatile("mcr p15, 0, %[val], c14, c2, 0" : :