[Skiboot] [PATCH v3 3/8] capp/phb: Introduce 'struct capp' to hold capp related info in 'struct phb'

[Vaibhav Jain]

Hi Fred,

Thanks for reviewing this patch.

Frederic Barrat <fbarrat at linux.ibm.com> writes:
>> +	capp->attached_pe = phb4_get_reserved_pe_number(&p->phb);

> It is my understanding that we just want to invalidate the stored 
> attached_pe and that phb4_get_reserved_pe_number() is used, but it could 
> as well be -1. It's not used anyway.
Want to use this member in future to set/remove the CAPP's IODA entries
that we populate.

> More fundamentally, why do we bother to store the attached_pe number? It 
> seems it's only used when calling set_capi_mode() to go back to PCI 
> mode, in which case, we don't care about the PE value.
Agreed, but sending the wrong value to the set_capi_mode() function
feels wrong and may catch some off gaurd in future.

>
>> +	capp->chip_id = p->chip_id;
>> +
>> +	/* Load capp microcode into the capp unit */
>> +	rc = load_capp_ucode(p);
>> +
>> +	if (rc == OPAL_SUCCESS)
>> +		p->capp = capp;
>> +
>> +	return rc;
>
> Shouldn't we free the capp memory structure if we coulnd't load the 
> microcode?
Good catch. Will get it fixed.

-- 
Vaibhav Jain <vaibhav at linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.