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

[Frederic Barrat]

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 at 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