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

[kgunda]

On 2017-07-14 23:00, Stephen Boyd wrote:
> 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!
Will check and address in the next patch.
> >  	/* 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?
This just to print the irq number in decimal format.
> >  		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?
will remove this line in the next patch.
> > +	 * 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?
ok. will change it in next patch.
> > +
> > +		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.
Will change in the next patch.
> > +		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.
yes. Will change it in the next patch.