Messages in this thread | | | From | Michael Ellerman <> | Subject | Re: [PATCH v4 09/10] powerpc/watchpoint: Return available watchpoints dynamically | Date | Tue, 21 Jul 2020 21:36:54 +1000 |
| |
Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes: > On 7/20/20 9:12 AM, Jordan Niethe wrote: >> On Fri, Jul 17, 2020 at 2:11 PM Ravi Bangoria >> <ravi.bangoria@linux.ibm.com> wrote: >>> >>> So far Book3S Powerpc supported only one watchpoint. Power10 is >>> introducing 2nd DAWR. Enable 2nd DAWR support for Power10. >>> Availability of 2nd DAWR will depend on CPU_FTR_DAWR1. >>> >>> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> >>> --- >>> arch/powerpc/include/asm/cputable.h | 4 +++- >>> arch/powerpc/include/asm/hw_breakpoint.h | 5 +++-- >>> 2 files changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h >>> index 3445c86e1f6f..36a0851a7a9b 100644 >>> --- a/arch/powerpc/include/asm/cputable.h >>> +++ b/arch/powerpc/include/asm/cputable.h >>> @@ -633,7 +633,9 @@ enum { >>> * Maximum number of hw breakpoint supported on powerpc. Number of >>> * breakpoints supported by actual hw might be less than this. >>> */ >>> -#define HBP_NUM_MAX 1 >>> +#define HBP_NUM_MAX 2 >>> +#define HBP_NUM_ONE 1 >>> +#define HBP_NUM_TWO 2
>> I wonder if these defines are necessary - has it any advantage over >> just using the literal? > > No, not really. Initially I had something like: > > #define HBP_NUM_MAX 2 > #define HBP_NUM_P8_P9 1 > #define HBP_NUM_P10 2 > > But then I thought it's also not right. So I made it _ONE and _TWO. > Now the function that decides nr watchpoints dynamically (nr_wp_slots) > is in different file, I thought to keep it like this so it would be > easier to figure out why _MAX is 2.
I don't think it makes anything clearer.
I had to stare at it thinking there was some sort of mapping or indirection going on, before I realised it's just literally the number of breakpoints.
So please just do:
static inline int nr_wp_slots(void) { return cpu_has_feature(CPU_FTR_DAWR1) ? 2 : 1; }
If you think HBP_NUM_MAX needs explanation then do that with a comment, it can refer to nr_wp_slots() if that's helpful.
cheers
| |