Re: [RFC Patch 1/6] Prepare the PowerPC platform for HW Breakpoint infrastructure

["K.Prasad" <prasad@linux.vnet.ibm.com>]

On Mon, May 18, 2009 at 01:35:32PM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2009-05-14 at 19:13 +0530, K.Prasad wrote:
> > plain text document attachment (ppc64_prepare_code_01)
> > Prepare the PowerPC code for HW Breakpoint infrastructure patches by including
> > relevant constant definitions and function declarations.
> 
> Hi !
> 
> Some comments below...
> 
> > +#define   HBP_NUM	1	/* Number of physical HW breakpoint registers */
> 
> Can you use a more verbose constant ? reg.h is included everywhere so
> the risk of collision is high.
>

This constant is used by the generic HW Breakpoint code in
kernel/hw_breakpoint.c too and renaming it here will require changes
there too, while I don't see any existing name-space clashes.

Instead the definition can be moved into arch/powerpc/include/asm/hw_breakpoint.h
and in arch/power/include/asm/processor.h. What do you think?
 
> >  #define   DABR_TRANSLATION	(1UL << 2)
> >  #define   DABR_DATA_WRITE	(1UL << 1)
> >  #define   DABR_DATA_READ	(1UL << 0)
> > +#define   DABR_DATA_RW		(3UL << 0)
> 
> Do you really need that ? It's just DABR_DATA_WRITE | DABR_DATA_READ :-)
> 

Yes, it looks silly. I will remove it. The HW_BREAKPOINT_RW defined in
arch/powerpc/include/asm/hw_breakpoint.h will instead be defined as:

#define HW_BREAKPOINT_RW (DABR_DATA_READ | DABR_DATA_WRITE)


> Cheers,
> Ben.
>

Thanks,
K.Prasad