Re: [PATCH V3 3/4] spmi: pmic-arb: add support for HW version 5

[Stephen Boyd <sboyd@codeaurora.org>]

On 07/11, Kiran Gunda wrote:
> @@ -420,7 +440,8 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  

Mostly style nitpicks!

>  	/* Start the transaction */
>  	pmic_arb_base_write(pmic_arb, offset + PMIC_ARB_CMD, cmd);
> -	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr);
> +	rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr,
> +				    PMIC_ARB_CHANNEL_RW);
>  	raw_spin_unlock_irqrestore(&pmic_arb->lock, flags);
>  
>  	return rc;
> @@ -681,12 +702,19 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
>  	ppid = intspec[0] << 8 | intspec[1];
>  	rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
>  	if (rc < 0) {
> -		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = 0x%x, periph = 0x%x, irq = %x rc = %d\n",
> +		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n",
>  		intspec[0], intspec[1], intspec[2], rc);

Unrelated change?

>  		return rc;
>  	}
>  
>  	apid = rc;
> +	if (pmic_arb->apid_data[apid].irq_ee != pmic_arb->ee) {
> +		dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u: ee=%u but owner=%u\n",
> +			intspec[0], intspec[1], intspec[2], pmic_arb->ee,
> +			pmic_arb->apid_data[apid].irq_ee);
> +		return -ENODEV;
> +	}
> +
>  	/* Keep track of {max,min}_apid for bounding search during interrupt */
>  	if (apid > pmic_arb->max_apid)
>  		pmic_arb->max_apid = apid;
>  	return apid_valid & ~PMIC_ARB_APID_VALID;
>  }
>  
> +static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
> +{
> +	struct apid_data *apid_info = pmic_arb->apid_data;
> +	struct apid_data *prev_apid_info;
> +	u16 i, j, ppid;
> +	bool valid, is_irq_ee;
> +	u32 regval, offset;
> +
> +	/*
> +	 * PMIC_ARB_REG_CHNL is a table in HW mapping APID (channel) to PPID.

Is this comment stale? PMIC_ARB_REG_CHNL macro was deleted?

> +	 * ppid_to_apid is an in-memory invert of that table.  In order to allow
> +	 * multiple EEs to write to a single PPID in arbiter version 5, there
> +	 * is more than one APID mapped to each PPID.  The owner field for each
> +	 * of these mappings specifies the EE which is allowed to write to the
> +	 * APID.  The owner of the last (highest) APID for a given PPID will
> +	 * receive interrupts from the PPID.
> +	 */
> +	for (i = 0; ; i++, apid_info++) {
> +		offset = pmic_arb->ver_ops->apid_map_offset(i);
> +		if (offset >= pmic_arb->core_size)
> +			break;
> +
> +		regval = readl_relaxed(pmic_arb->core + offset);
> +		if (!regval)
> +			continue;
> +		ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
> +		is_irq_ee = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval);
> +
> +		regval = readl_relaxed(pmic_arb->cnfg +
> +				      SPMI_OWNERSHIP_TABLE_REG(i));
> +		apid_info->write_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
> +
> +		apid_info->irq_ee = is_irq_ee ?
> +					apid_info->write_ee : INVALID_EE;

Perhaps apid_info can be renamed to apidd (for apid descriptor)
or apidi (for apid info) and then this line is short enough to
fit on one line?

> +
> +		valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
> +		j = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;

Maybe j can be 'apid'. Slightly more informative and usually 'j'
is reserved for iterating, which in this case we're not doing.
We're just directly indexing an apid into a table.

> +		prev_apid_info = &pmic_arb->apid_data[j];
> +
> +		if (valid && is_irq_ee &&
> +				prev_apid_info->write_ee == pmic_arb->ee) {
> +			/*
> +			 * Duplicate PPID mapping after the one for this EE;
> +			 * override the irq owner
> +			 */
> +			prev_apid_info->irq_ee = apid_info->irq_ee;
> +		} else if (!valid || is_irq_ee) {
> +			/* First PPID mapping or duplicate for another EE */
> +			pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
> +		}
> +
> +		apid_info->ppid = ppid;
> +		pmic_arb->last_apid = i;
> +	}
> +
> +	/* Dump the mapping table for debug purposes. */
> +	dev_dbg(&pmic_arb->spmic->dev, "PPID APID Write-EE IRQ-EE\n");
> +	for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
> +		valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
> +		i = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
> +		if (valid) {
> +			apid_info = &pmic_arb->apid_data[i];
> +			dev_dbg(&pmic_arb->spmic->dev, "%#03X %3u %2u %2u\n",
> +			      ppid, i, apid_info->write_ee, apid_info->irq_ee);
> +		}

Could be 

	for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
		apid = pmic_arb->ppid_to_apid[ppid];
		if (apid & PMIC_ARB_APID_VALID) {
			apid &= ~PMIC_ARB_VALID;
			apidd = &pmic_arb->apid_data[apid];
			dev_dbg(&pmic_arb->spmic->dev, "%#03X %3u %2u %2u\n",
			      ppid, apid, apidd->write_ee, apidd->irq_ee);
		}
	}

Which maybe is clearer because it uses less local variables that
don't get used more than once.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project