Messages in this thread | | | Subject | Re: [PATCH v4 09/10] powerpc/watchpoint: Return available watchpoints dynamically | From | Ravi Bangoria <> | Date | Tue, 21 Jul 2020 19:03:04 +0530 |
| |
On 7/21/20 5:06 PM, Michael Ellerman wrote: > 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.
Agreed. By adding a comment, we can remove those macros. Will change it.
Thanks, Ravi
| |