MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Tue, 18 Jul 2017 10:39:48 +0530
From: kgunda@codeaurora.org
To: Stephen Boyd <sboyd@codeaurora.org>
Cc: gregkh@linuxfoundation.org,
        Abhijeet Dharmapurikar <adharmap@codeaurora.org>,
        David Collins <collinsd@codeaurora.org>, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH V3 3/4] spmi: pmic-arb: add support for HW version 5
In-Reply-To: <20170714173028.GI22780@codeaurora.org>
References: <1499737497-25649-1-git-send-email-kgunda@codeaurora.org>
 <1499737497-25649-4-git-send-email-kgunda@codeaurora.org>
 <20170714173028.GI22780@codeaurora.org>
Message-ID: <4a6e9ec838d6afc1ca300e6aee7b8999@codeaurora.org>
User-Agent: Roundcube Webmail/1.2.5
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5287
Lines: 152

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.