diff mbox series

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

Message ID 20190108095902.24718-4-vaibhav@linux.ibm.com
State Superseded
Headers show
Series Enable fast-reboot support for CAPI-2 | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success master/apply_patch Successfully applied
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master

Commit Message

Vaibhav Jain Jan. 8, 2019, 9:58 a.m. UTC
Previously struct proc_chip member 'capp_phb3_attached_mask' was used
for Power-8 to keep track of PHB attached to the single CAPP on the
chip. CAPP on that chip supported a flexible PHB assignment
scheme. However since then new chips only support a static assignment
i.e a CAPP can only be attached to a specific PEC.

Hence instead of using 'proc_chip.capp_phb4_attached_mask' to manage
CAPP <-> PEC assignments which needs a global lock (capi_lock) to be
updated, we introduce a new struct named 'capp' a pointer to which
resides inside struct 'phb4'. Since updates to struct 'phb4' already
happen in context of phb_lock; this eliminates the
need to use mutex 'capi_lock' while updating
'capp_phb4_attached_mask'.

This struct is also used to hold CAPP specific variables such as
pointer to the 'struct phb' to which the CAPP is attached,
'capp_xscom_offset' which is the xscom offset to be added to CAPP
registers in case there are more than 1 on the chip, 'capp_index'
which is the index of the CAPP on the chip, and attached_pe' which is
the process endpoint index to which CAPP is attached. Finally member
'chip_id' holds the chip-id thats used for performing xscom
read/writes.

Also new helpers named capp_xscom_read()/write() are introduced to
make access to CAPP xscom registers easier.

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Change-log

v3:	Moved the 'struct capp *' from struct 'phb' to struct 'phb4' [ Christophe ]

	Introduced 'chip_id' as a member of struct 'capp'.

	Introduced helpers capp_xscom_read()/write().
---
 hw/capp.c      | 12 ++++++++++++
 hw/phb4.c      | 52 +++++++++++++++++++++++++++++++++++++++++++-------
 include/capp.h | 12 ++++++++++++
 include/chip.h |  1 -
 include/phb4.h |  3 +++
 5 files changed, 72 insertions(+), 8 deletions(-)

Comments

Christophe Lombard Jan. 8, 2019, 4:20 p.m. UTC | #1
Le 08/01/2019 à 10:58, Vaibhav Jain a écrit :
> Previously struct proc_chip member 'capp_phb3_attached_mask' was used
> for Power-8 to keep track of PHB attached to the single CAPP on the
> chip. CAPP on that chip supported a flexible PHB assignment
> scheme. However since then new chips only support a static assignment
> i.e a CAPP can only be attached to a specific PEC.
> 
> Hence instead of using 'proc_chip.capp_phb4_attached_mask' to manage
> CAPP <-> PEC assignments which needs a global lock (capi_lock) to be
> updated, we introduce a new struct named 'capp' a pointer to which
> resides inside struct 'phb4'. Since updates to struct 'phb4' already
> happen in context of phb_lock; this eliminates the
> need to use mutex 'capi_lock' while updating
> 'capp_phb4_attached_mask'.
> 
> This struct is also used to hold CAPP specific variables such as
> pointer to the 'struct phb' to which the CAPP is attached,
> 'capp_xscom_offset' which is the xscom offset to be added to CAPP
> registers in case there are more than 1 on the chip, 'capp_index'
> which is the index of the CAPP on the chip, and attached_pe' which is
> the process endpoint index to which CAPP is attached. Finally member
> 'chip_id' holds the chip-id thats used for performing xscom
> read/writes.
> 
> Also new helpers named capp_xscom_read()/write() are introduced to
> make access to CAPP xscom registers easier.
> 
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Change-log
> 
> v3:	Moved the 'struct capp *' from struct 'phb' to struct 'phb4' [ Christophe ]
> 
> 	Introduced 'chip_id' as a member of struct 'capp'.
> 
> 	Introduced helpers capp_xscom_read()/write().


Reviewed-by: Christophe Lombard <clombard@linux.vnet.ibm.com>
Andrew Donnellan Jan. 9, 2019, 4:18 a.m. UTC | #2
On 8/1/19 8:58 pm, Vaibhav Jain wrote:
> Previously struct proc_chip member 'capp_phb3_attached_mask' was used
> for Power-8 to keep track of PHB attached to the single CAPP on the
> chip. CAPP on that chip supported a flexible PHB assignment
> scheme. However since then new chips only support a static assignment
> i.e a CAPP can only be attached to a specific PEC.
> 
> Hence instead of using 'proc_chip.capp_phb4_attached_mask' to manage
> CAPP <-> PEC assignments which needs a global lock (capi_lock) to be
> updated, we introduce a new struct named 'capp' a pointer to which
> resides inside struct 'phb4'. Since updates to struct 'phb4' already
> happen in context of phb_lock; this eliminates the
> need to use mutex 'capi_lock' while updating
> 'capp_phb4_attached_mask'.
> 
> This struct is also used to hold CAPP specific variables such as
> pointer to the 'struct phb' to which the CAPP is attached,
> 'capp_xscom_offset' which is the xscom offset to be added to CAPP
> registers in case there are more than 1 on the chip, 'capp_index'
> which is the index of the CAPP on the chip, and attached_pe' which is
> the process endpoint index to which CAPP is attached. Finally member
> 'chip_id' holds the chip-id thats used for performing xscom
> read/writes.
> 
> Also new helpers named capp_xscom_read()/write() are introduced to
> make access to CAPP xscom registers easier.
> 
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>

Doing the analogous thing for phb3 would be a nice future cleanup.

Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

> ---
> Change-log
> 
> v3:	Moved the 'struct capp *' from struct 'phb' to struct 'phb4' [ Christophe ]
> 
> 	Introduced 'chip_id' as a member of struct 'capp'.
> 
> 	Introduced helpers capp_xscom_read()/write().
> ---
>   hw/capp.c      | 12 ++++++++++++
>   hw/phb4.c      | 52 +++++++++++++++++++++++++++++++++++++++++++-------
>   include/capp.h | 12 ++++++++++++
>   include/chip.h |  1 -
>   include/phb4.h |  3 +++
>   5 files changed, 72 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/capp.c b/hw/capp.c
> index eeaa4ac4..b3984ea0 100644
> --- a/hw/capp.c
> +++ b/hw/capp.c
> @@ -240,3 +240,15 @@ int64_t capp_get_info(int chip_id, struct phb *phb, struct capp_info *info)
>   
>   	return OPAL_PARAMETER;
>   }
> +
> +int64_t capp_xscom_read(struct capp *capp, int64_t off, uint64_t *val)
> +{
> +	return capp == NULL ? OPAL_UNSUPPORTED :
> +		xscom_read(capp->chip_id, off + capp->capp_xscom_offset, val);
> +}
> +
> +int64_t capp_xscom_write(struct capp *capp, int64_t off, uint64_t val)
> +{
> +	return capp == NULL ? OPAL_UNSUPPORTED :
> +		xscom_write(capp->chip_id, off + capp->capp_xscom_offset, val);
> +}
> diff --git a/hw/phb4.c b/hw/phb4.c
> index c0797647..59a96680 100644
> --- a/hw/phb4.c
> +++ b/hw/phb4.c
> @@ -3880,13 +3880,13 @@ static int64_t phb4_get_capp_info(int chip_id, struct phb *phb,
>   				  struct capp_info *info)
>   {
>   	struct phb4 *p = phb_to_phb4(phb);
> -	struct proc_chip *chip = get_chip(p->chip_id);
>   	uint32_t offset;
>   
>   	if (chip_id != p->chip_id)
>   		return OPAL_PARAMETER;
>   
> -	if (!((1 << p->index) & chip->capp_phb4_attached_mask))
> +	/* Check is CAPP is attached to the PHB */
> +	if (p->capp == NULL || p->capp->phb != phb)
>   		return OPAL_PARAMETER;
>   
>   	offset = PHB4_CAPP_REG_OFFSET(p);
> @@ -4397,23 +4397,61 @@ static int64_t enable_capi_mode(struct phb4 *p, uint64_t pe_number,
>   	return OPAL_SUCCESS;
>   }
>   
> +
> +static int64_t phb4_init_capp(struct phb4 *p)
> +{
> +	struct capp *capp;
> +	int rc;
> +
> +	if (p->index != CAPP0_PHB_INDEX &&
> +	    p->index != CAPP1_PHB_INDEX)
> +		return OPAL_UNSUPPORTED;
> +
> +	capp = zalloc(sizeof(struct capp));
> +	if (capp == NULL)
> +		return OPAL_NO_MEM;
> +
> +	if (p->index == CAPP0_PHB_INDEX) {
> +		capp->capp_index = 0;
> +		capp->capp_xscom_offset = 0;
> +
> +	} else if (p->index == CAPP1_PHB_INDEX) {
> +		capp->capp_index = 1;
> +		capp->capp_xscom_offset = CAPP1_REG_OFFSET;
> +	}
> +
> +	capp->attached_pe = phb4_get_reserved_pe_number(&p->phb);
> +	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;
> +}
> +
>   static int64_t phb4_set_capi_mode(struct phb *phb, uint64_t mode,
>   				  uint64_t pe_number)
>   {
>   	struct phb4 *p = phb_to_phb4(phb);
>   	struct proc_chip *chip = get_chip(p->chip_id);
> +	struct capp *capp = p->capp;
>   	uint64_t reg, ret;
>   	uint32_t offset;
>   
> +	if (capp == NULL)
> +		return OPAL_UNSUPPORTED;
>   
>   	if (!capp_ucode_loaded(chip, p->index)) {
>   		PHBERR(p, "CAPP: ucode not loaded\n");
>   		return OPAL_RESOURCE;
>   	}
>   
> -	lock(&capi_lock);
> -	chip->capp_phb4_attached_mask |= 1 << p->index;
> -	unlock(&capi_lock);
> +	/* mark the capp attached to the phb */
> +	capp->phb = phb;
> +	capp->attached_pe = pe_number;
>   
>   	offset = PHB4_CAPP_REG_OFFSET(p);
>   	xscom_read(p->chip_id, CAPP_ERR_STATUS_CTRL + offset, &reg);
> @@ -5595,8 +5633,8 @@ static void phb4_create(struct dt_node *np)
>   	/* Get the HW up and running */
>   	phb4_init_hw(p);
>   
> -	/* Load capp microcode into capp unit */
> -	load_capp_ucode(p);
> +	/* init capp that might get attached to the phb */
> +	phb4_init_capp(p);
>   
>   	/* Compute XIVE source flags depending on PHB revision */
>   	irq_flags = 0;
> diff --git a/include/capp.h b/include/capp.h
> index 6ec3f7fe..cc70e443 100644
> --- a/include/capp.h
> +++ b/include/capp.h
> @@ -79,6 +79,14 @@ struct capp_ops {
>   	int64_t (*get_capp_info)(int, struct phb *, struct capp_info *);
>   };
>   
> +struct capp {
> +	struct phb *phb;
> +	unsigned int capp_index;
> +	uint64_t capp_xscom_offset;
> +	uint64_t attached_pe;
> +	uint64_t chip_id;
> +};
> +
>   struct proc_chip;
>   extern struct lock capi_lock;
>   extern struct capp_ops capi_ops;
> @@ -96,4 +104,8 @@ extern int64_t capp_load_ucode(unsigned int chip_id, uint32_t opal_id,
>   extern int64_t capp_get_info(int chip_id, struct phb *phb,
>   			     struct capp_info *info);
>   
> +
> +/* Helpers to read/write capp registers */
> +extern int64_t capp_xscom_read(struct capp *capp, int64_t off, uint64_t *val);
> +extern int64_t capp_xscom_write(struct capp *capp, int64_t off, uint64_t val);
>   #endif /* __CAPP_H */
> diff --git a/include/chip.h b/include/chip.h
> index 2fb8126d..c759d0a0 100644
> --- a/include/chip.h
> +++ b/include/chip.h
> @@ -197,7 +197,6 @@ struct proc_chip {
>   
>   	/* Must hold capi_lock to change */
>   	uint8_t			capp_phb3_attached_mask;
> -	uint8_t			capp_phb4_attached_mask;
>   	uint8_t			capp_ucode_loaded;
>   
>   	/* Used by hw/centaur.c */
> diff --git a/include/phb4.h b/include/phb4.h
> index 43819d57..60c1735d 100644
> --- a/include/phb4.h
> +++ b/include/phb4.h
> @@ -232,6 +232,9 @@ struct phb4 {
>   	/* Current NPU2 relaxed ordering state */
>   	bool			ro_state;
>   
> +	/* Any capp instance attached to the PHB4 */
> +	struct capp		*capp;
> +
>   	struct phb		phb;
>   };
>   
>
Frederic Barrat Jan. 10, 2019, 1:43 p.m. UTC | #3
Le 08/01/2019 à 10:58, Vaibhav Jain a écrit :
> Previously struct proc_chip member 'capp_phb3_attached_mask' was used
> for Power-8 to keep track of PHB attached to the single CAPP on the
> chip. CAPP on that chip supported a flexible PHB assignment
> scheme. However since then new chips only support a static assignment
> i.e a CAPP can only be attached to a specific PEC.
> 
> Hence instead of using 'proc_chip.capp_phb4_attached_mask' to manage
> CAPP <-> PEC assignments which needs a global lock (capi_lock) to be
> updated, we introduce a new struct named 'capp' a pointer to which
> resides inside struct 'phb4'. Since updates to struct 'phb4' already
> happen in context of phb_lock; this eliminates the
> need to use mutex 'capi_lock' while updating
> 'capp_phb4_attached_mask'.
> 
> This struct is also used to hold CAPP specific variables such as
> pointer to the 'struct phb' to which the CAPP is attached,
> 'capp_xscom_offset' which is the xscom offset to be added to CAPP
> registers in case there are more than 1 on the chip, 'capp_index'
> which is the index of the CAPP on the chip, and attached_pe' which is
> the process endpoint index to which CAPP is attached. Finally member
> 'chip_id' holds the chip-id thats used for performing xscom
> read/writes.
> 
> Also new helpers named capp_xscom_read()/write() are introduced to
> make access to CAPP xscom registers easier.
> 
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Change-log
> 
> v3:	Moved the 'struct capp *' from struct 'phb' to struct 'phb4' [ Christophe ]
> 
> 	Introduced 'chip_id' as a member of struct 'capp'.
> 
> 	Introduced helpers capp_xscom_read()/write().
> ---
>   hw/capp.c      | 12 ++++++++++++
>   hw/phb4.c      | 52 +++++++++++++++++++++++++++++++++++++++++++-------
>   include/capp.h | 12 ++++++++++++
>   include/chip.h |  1 -
>   include/phb4.h |  3 +++
>   5 files changed, 72 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/capp.c b/hw/capp.c
> index eeaa4ac4..b3984ea0 100644
> --- a/hw/capp.c
> +++ b/hw/capp.c
> @@ -240,3 +240,15 @@ int64_t capp_get_info(int chip_id, struct phb *phb, struct capp_info *info)
>   
>   	return OPAL_PARAMETER;
>   }
> +
> +int64_t capp_xscom_read(struct capp *capp, int64_t off, uint64_t *val)
> +{
> +	return capp == NULL ? OPAL_UNSUPPORTED :
> +		xscom_read(capp->chip_id, off + capp->capp_xscom_offset, val);
> +}
> +
> +int64_t capp_xscom_write(struct capp *capp, int64_t off, uint64_t val)
> +{
> +	return capp == NULL ? OPAL_UNSUPPORTED :
> +		xscom_write(capp->chip_id, off + capp->capp_xscom_offset, val);
> +}
> diff --git a/hw/phb4.c b/hw/phb4.c
> index c0797647..59a96680 100644
> --- a/hw/phb4.c
> +++ b/hw/phb4.c
> @@ -3880,13 +3880,13 @@ static int64_t phb4_get_capp_info(int chip_id, struct phb *phb,
>   				  struct capp_info *info)
>   {
>   	struct phb4 *p = phb_to_phb4(phb);
> -	struct proc_chip *chip = get_chip(p->chip_id);
>   	uint32_t offset;
>   
>   	if (chip_id != p->chip_id)
>   		return OPAL_PARAMETER;
>   
> -	if (!((1 << p->index) & chip->capp_phb4_attached_mask))
> +	/* Check is CAPP is attached to the PHB */
> +	if (p->capp == NULL || p->capp->phb != phb)
>   		return OPAL_PARAMETER;
>   
>   	offset = PHB4_CAPP_REG_OFFSET(p);
> @@ -4397,23 +4397,61 @@ static int64_t enable_capi_mode(struct phb4 *p, uint64_t pe_number,
>   	return OPAL_SUCCESS;
>   }
>   
> +
> +static int64_t phb4_init_capp(struct phb4 *p)
> +{
> +	struct capp *capp;
> +	int rc;
> +
> +	if (p->index != CAPP0_PHB_INDEX &&
> +	    p->index != CAPP1_PHB_INDEX)
> +		return OPAL_UNSUPPORTED;
> +
> +	capp = zalloc(sizeof(struct capp));
> +	if (capp == NULL)
> +		return OPAL_NO_MEM;
> +
> +	if (p->index == CAPP0_PHB_INDEX) {
> +		capp->capp_index = 0;
> +		capp->capp_xscom_offset = 0;
> +
> +	} else if (p->index == CAPP1_PHB_INDEX) {
> +		capp->capp_index = 1;
> +		capp->capp_xscom_offset = CAPP1_REG_OFFSET;
> +	}
> +
> +	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.

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.

> +	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?

  Fred
Vaibhav Jain Jan. 10, 2019, 2:13 p.m. UTC | #4
Hi Fred,

Thanks for reviewing this patch.

Frederic Barrat <fbarrat@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.
Frederic Barrat Jan. 10, 2019, 4:19 p.m. UTC | #5
Le 10/01/2019 à 15:13, Vaibhav Jain a écrit :
> Hi Fred,
> 
> Thanks for reviewing this patch.
> 
> Frederic Barrat <fbarrat@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.

I would argue that it also feels wrong to use the reserved value from 
phb4_get_reserved_pe_number(), I was really wondering where that was 
coming from when reading the patch. And I don't think it's that wrong to 
pass an invalid value like -1 when switching to pci mode. It's unused in 
that case anyway.
Anyway, it's not that important, and the code in the patch works.

   Fred


>>
>>> +	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.
>
diff mbox series

Patch

diff --git a/hw/capp.c b/hw/capp.c
index eeaa4ac4..b3984ea0 100644
--- a/hw/capp.c
+++ b/hw/capp.c
@@ -240,3 +240,15 @@  int64_t capp_get_info(int chip_id, struct phb *phb, struct capp_info *info)
 
 	return OPAL_PARAMETER;
 }
+
+int64_t capp_xscom_read(struct capp *capp, int64_t off, uint64_t *val)
+{
+	return capp == NULL ? OPAL_UNSUPPORTED :
+		xscom_read(capp->chip_id, off + capp->capp_xscom_offset, val);
+}
+
+int64_t capp_xscom_write(struct capp *capp, int64_t off, uint64_t val)
+{
+	return capp == NULL ? OPAL_UNSUPPORTED :
+		xscom_write(capp->chip_id, off + capp->capp_xscom_offset, val);
+}
diff --git a/hw/phb4.c b/hw/phb4.c
index c0797647..59a96680 100644
--- a/hw/phb4.c
+++ b/hw/phb4.c
@@ -3880,13 +3880,13 @@  static int64_t phb4_get_capp_info(int chip_id, struct phb *phb,
 				  struct capp_info *info)
 {
 	struct phb4 *p = phb_to_phb4(phb);
-	struct proc_chip *chip = get_chip(p->chip_id);
 	uint32_t offset;
 
 	if (chip_id != p->chip_id)
 		return OPAL_PARAMETER;
 
-	if (!((1 << p->index) & chip->capp_phb4_attached_mask))
+	/* Check is CAPP is attached to the PHB */
+	if (p->capp == NULL || p->capp->phb != phb)
 		return OPAL_PARAMETER;
 
 	offset = PHB4_CAPP_REG_OFFSET(p);
@@ -4397,23 +4397,61 @@  static int64_t enable_capi_mode(struct phb4 *p, uint64_t pe_number,
 	return OPAL_SUCCESS;
 }
 
+
+static int64_t phb4_init_capp(struct phb4 *p)
+{
+	struct capp *capp;
+	int rc;
+
+	if (p->index != CAPP0_PHB_INDEX &&
+	    p->index != CAPP1_PHB_INDEX)
+		return OPAL_UNSUPPORTED;
+
+	capp = zalloc(sizeof(struct capp));
+	if (capp == NULL)
+		return OPAL_NO_MEM;
+
+	if (p->index == CAPP0_PHB_INDEX) {
+		capp->capp_index = 0;
+		capp->capp_xscom_offset = 0;
+
+	} else if (p->index == CAPP1_PHB_INDEX) {
+		capp->capp_index = 1;
+		capp->capp_xscom_offset = CAPP1_REG_OFFSET;
+	}
+
+	capp->attached_pe = phb4_get_reserved_pe_number(&p->phb);
+	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;
+}
+
 static int64_t phb4_set_capi_mode(struct phb *phb, uint64_t mode,
 				  uint64_t pe_number)
 {
 	struct phb4 *p = phb_to_phb4(phb);
 	struct proc_chip *chip = get_chip(p->chip_id);
+	struct capp *capp = p->capp;
 	uint64_t reg, ret;
 	uint32_t offset;
 
+	if (capp == NULL)
+		return OPAL_UNSUPPORTED;
 
 	if (!capp_ucode_loaded(chip, p->index)) {
 		PHBERR(p, "CAPP: ucode not loaded\n");
 		return OPAL_RESOURCE;
 	}
 
-	lock(&capi_lock);
-	chip->capp_phb4_attached_mask |= 1 << p->index;
-	unlock(&capi_lock);
+	/* mark the capp attached to the phb */
+	capp->phb = phb;
+	capp->attached_pe = pe_number;
 
 	offset = PHB4_CAPP_REG_OFFSET(p);
 	xscom_read(p->chip_id, CAPP_ERR_STATUS_CTRL + offset, &reg);
@@ -5595,8 +5633,8 @@  static void phb4_create(struct dt_node *np)
 	/* Get the HW up and running */
 	phb4_init_hw(p);
 
-	/* Load capp microcode into capp unit */
-	load_capp_ucode(p);
+	/* init capp that might get attached to the phb */
+	phb4_init_capp(p);
 
 	/* Compute XIVE source flags depending on PHB revision */
 	irq_flags = 0;
diff --git a/include/capp.h b/include/capp.h
index 6ec3f7fe..cc70e443 100644
--- a/include/capp.h
+++ b/include/capp.h
@@ -79,6 +79,14 @@  struct capp_ops {
 	int64_t (*get_capp_info)(int, struct phb *, struct capp_info *);
 };
 
+struct capp {
+	struct phb *phb;
+	unsigned int capp_index;
+	uint64_t capp_xscom_offset;
+	uint64_t attached_pe;
+	uint64_t chip_id;
+};
+
 struct proc_chip;
 extern struct lock capi_lock;
 extern struct capp_ops capi_ops;
@@ -96,4 +104,8 @@  extern int64_t capp_load_ucode(unsigned int chip_id, uint32_t opal_id,
 extern int64_t capp_get_info(int chip_id, struct phb *phb,
 			     struct capp_info *info);
 
+
+/* Helpers to read/write capp registers */
+extern int64_t capp_xscom_read(struct capp *capp, int64_t off, uint64_t *val);
+extern int64_t capp_xscom_write(struct capp *capp, int64_t off, uint64_t val);
 #endif /* __CAPP_H */
diff --git a/include/chip.h b/include/chip.h
index 2fb8126d..c759d0a0 100644
--- a/include/chip.h
+++ b/include/chip.h
@@ -197,7 +197,6 @@  struct proc_chip {
 
 	/* Must hold capi_lock to change */
 	uint8_t			capp_phb3_attached_mask;
-	uint8_t			capp_phb4_attached_mask;
 	uint8_t			capp_ucode_loaded;
 
 	/* Used by hw/centaur.c */
diff --git a/include/phb4.h b/include/phb4.h
index 43819d57..60c1735d 100644
--- a/include/phb4.h
+++ b/include/phb4.h
@@ -232,6 +232,9 @@  struct phb4 {
 	/* Current NPU2 relaxed ordering state */
 	bool			ro_state;
 
+	/* Any capp instance attached to the PHB4 */
+	struct capp		*capp;
+
 	struct phb		phb;
 };