[Skiboot] [PATCH V2 2/6] phb3: Load CAPP ucode to both CAPP units on Naples

[Michael Neuling]

On Thu, 2016-02-25 at 15:10 +1100, Stewart Smith wrote:
> Philippe Bergheaud <felix at linux.vnet.ibm.com> writes:
> > diff --git a/hw/phb3.c b/hw/phb3.c
> > index adff5bc..087f3e0 100644
> > --- a/hw/phb3.c
> > +++ b/hw/phb3.c
> > @@ -2314,6 +2314,10 @@ static struct {
> > 
> >  #define CAPP_UCODE_MAX_SIZE 0x20000
> > 
> > +#define CAPP_UCODE_LOADED(chip, p) \
> > +	((p->rev < PHB3_REV_NAPLES_DD10) ? chip->capp_ucode_loaded
> > : \
> > +	 (chip->capp_ucode_loaded & (1 << p->index)))
> > +

p and chip need some parenthesis too.

> Why not just treat it as a bitmap for everything? Sure, for not Naples,
> it'll just be one bit, but that's okay and probably simpler to
> understand.

Agreed.

> Also, are we *sure* that rev < PHB3_REV_NAPLES_DD10 is going to always
> be true? Do we have docs somewhere saying how the revision numbers are
> allocated? I couldn't find it from a quick look at docs...

Agreed, That is a bit fragile.  Probably best to wrap the test with a
PHB3_IS_NAPLES macro

#define PHB3_IS_NAPLES(p) ((p)->rev == PHB3_REV_NAPLES_DD10)

> 
> > @@ -2346,12 +2350,18 @@ static int64_t capp_load_ucode(struct phb3
> > *p)
> >  	struct capp_ucode_data *data;
> >  	struct capp_lid_hdr *lid;
> >  	uint64_t rc, val, addr;
> > -	uint32_t chunk_count, offset;
> > +	uint32_t chunk_count, offset, reg_offset;
> >  	int i;
> > 
> > -	if (chip->capp_ucode_loaded)
> > +	if (CAPP_UCODE_LOADED(chip, p))
> >  		return OPAL_SUCCESS;
> > 
> > +	/* Return if PHB not attached to a CAPP unit */
> > +	if (p->rev == PHB3_REV_NAPLES_DD10 && p->index > 1)
> > +		return OPAL_HARDWARE;
> > +	if (p->index > 2)
> > +		return OPAL_HARDWARE;
> 
> Should we have a MAX_PHBs define somewhere?

I tend to agree, although it's max phbs allowed to attach to capp unit
in this case.  Naples has 4 phbs, but only the first 2 can attach to
the CAPP units.

Mikey