[Skiboot] [PATCH 1/7] phb4/capp: Update and re-factor phb4_set_capi_mode()

[Andrew Donnellan]

On 16/9/18 12:39 am, Vaibhav Jain wrote:
> Presently phb4_set_capi_mode() performs certain CAPP checks
> like, checking of CAPP ucode loaded or checks if CAPP is still in
> recovery, even when the requested mode is to switch to PCI mode.
> 
> Hence this patch updates and re-factors phb4_set_capi_mode() to make
> sure CAPP related checks are only performed when request to enable
> CAPP is made by mode==OPAL_PHB_CAPI_MODE_CAPI/DMA_TVT1. We also update
> other possible modes requests to return a more appropriate status code
> based on if CAPP is activated or not.
> 
> Signed-off-by: Vaibhav Jain <vaibhav at linux.ibm.com>
> ---
>   hw/phb4.c | 93 +++++++++++++++++++++++++++++++++++--------------------
>   1 file changed, 59 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/phb4.c b/hw/phb4.c
> index 94e741e0..4284c467 100644
> --- a/hw/phb4.c
> +++ b/hw/phb4.c
> @@ -4322,54 +4322,79 @@ static int64_t phb4_set_capi_mode(struct phb *phb, uint64_t mode,
>   {
>   	struct phb4 *p = phb_to_phb4(phb);
>   	struct proc_chip *chip = get_chip(p->chip_id);
> +	uint32_t offset = PHB4_CAPP_REG_OFFSET(p);
>   	uint64_t reg, ret;
> -	uint32_t offset;
> -
> -
> -	if (!capp_ucode_loaded(chip, p->index)) {
> -		PHBERR(p, "CAPP: ucode not loaded\n");
> -		return OPAL_RESOURCE;
> -	}
> +	bool attached;
>   
> +	/* No one else fiddle with capp while we modify its state */
>   	lock(&capi_lock);
> -	chip->capp_phb4_attached_mask |= 1 << p->index;
> -	unlock(&capi_lock);

Is there any way to release the lock earlier, like we currently do here? 
As is, we could be holding capi_lock for quite a while (some of the 
timeouts in enable_capi_mode() are up to 5 seconds) and we only need to 
hold capi_lock while we're touching the attached_mask, AIUI.

>   
> -	offset = PHB4_CAPP_REG_OFFSET(p);
> -	xscom_read(p->chip_id, CAPP_ERR_STATUS_CTRL + offset, &reg);
> -	if ((reg & PPC_BIT(5))) {
> -		PHBERR(p, "CAPP: recovery failed (%016llx)\n", reg);
> -		return OPAL_HARDWARE;
> -	} else if ((reg & PPC_BIT(0)) && (!(reg & PPC_BIT(1)))) {
> -		PHBDBG(p, "CAPP: recovery in progress\n");
> -		return OPAL_BUSY;
> -	}
> +	/* Check if CAPP are already attached to PHB */
> +	attached = chip->capp_phb4_attached_mask & (1 << p->index);
>   
>   	switch (mode) {
> -	case OPAL_PHB_CAPI_MODE_CAPI:
> -		ret = enable_capi_mode(p, pe_number,
> -					CAPP_MAX_STQ_ENGINES |
> -					CAPP_MIN_DMA_READ_ENGINES);
> -		disable_fast_reboot("CAPP being enabled");
> +
> +	case OPAL_PHB_CAPI_MODE_DMA: /* Enabled by default on p9 */
> +	case OPAL_PHB_CAPI_MODE_SNOOP_ON:
> +		/* nothing to do on P9 if CAPP is alreay enabled */
> +		ret = attached ? OPAL_SUCCESS : OPAL_UNSUPPORTED;
>   		break;
> +
> +	case OPAL_PHB_CAPI_MODE_SNOOP_OFF:
> +	case OPAL_PHB_CAPI_MODE_PCIE: /* Not supported at the moment */
> +		ret = !attached ? OPAL_SUCCESS : OPAL_UNSUPPORTED;
> +		break;
> +
> +	case OPAL_PHB_CAPI_MODE_CAPI: /* Fall Through */
>   	case OPAL_PHB_CAPI_MODE_DMA_TVT1:
> +		/* Check if ucode is available */
> +		if (!capp_ucode_loaded(chip, p->index)) {
> +			PHBERR(p, "CAPP: ucode not loaded\n");
> +			ret = OPAL_RESOURCE;
> +			break;
> +		}
> +
> +		xscom_read(p->chip_id, CAPP_ERR_STATUS_CTRL + offset, &reg);
> +		if ((reg & PPC_BIT(5))) {
> +			PHBERR(p, "CAPP: recovery failed (%016llx)\n", reg);
> +			ret = OPAL_HARDWARE;
> +			break;
> +		} else if ((reg & PPC_BIT(0)) && (!(reg & PPC_BIT(1)))) {
> +			PHBDBG(p, "CAPP: recovery in progress\n");
> +			ret = OPAL_BUSY;
> +			break;
> +		}
> +
> +		/*
> +		 * Mark the CAPP attached to the PHB right away so that
> +		 * if a MCE happens during CAPP init we can handle it.
> +		 * In case of an error in CAPP init we remove the PHB
> +		 * from the attached_mask later.
> +		 */
> +		chip->capp_phb4_attached_mask |= 1 << p->index;
>   		ret = enable_capi_mode(p, pe_number,
> -					CAPP_MIN_STQ_ENGINES |
> -					CAPP_MAX_DMA_READ_ENGINES);
> -		disable_fast_reboot("CAPP being enabled");
> -		break;
> -	case OPAL_PHB_CAPI_MODE_SNOOP_ON:
> -		/* nothing to do P9 if CAPP is alreay enabled */
> -		ret = OPAL_SUCCESS;
> +				       (mode == OPAL_PHB_CAPI_MODE_DMA_TVT1 ?
> +					/* Max perf for DMA */
> +					(CAPP_MIN_STQ_ENGINES |
> +					 CAPP_MAX_DMA_READ_ENGINES) :
> +					/* Max perf for CAPP STQ */
> +					(CAPP_MAX_STQ_ENGINES |
> +					 CAPP_MIN_DMA_READ_ENGINES)));

It would be easier to understand if you split this out into a separate 
variable rather than use the ternary operator inside the function call.

> +
> +		if (ret == OPAL_SUCCESS)
> +			/* Disable fast reboot for CAPP */
> +			disable_fast_reboot("CAPP being enabled");
> +		else
> +			/* In case of an error mark the PHB detached */
> +			chip->capp_phb4_attached_mask ^= 1 << p->index;
>   		break;
>   
> -	case OPAL_PHB_CAPI_MODE_PCIE: /* shouldn't be called on p9*/
> -	case OPAL_PHB_CAPI_MODE_DMA: /* Enabled by default on p9 */
> -	case OPAL_PHB_CAPI_MODE_SNOOP_OFF: /* shouldn't be called on p9*/
>   	default:
>   		ret = OPAL_UNSUPPORTED;
> -	}
> +		break;
> +	};
>   
> +	unlock(&capi_lock);
>   	return ret;
>   }
>   
> 

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com  IBM Australia Limited