Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754379AbdGNRac (ORCPT ); Fri, 14 Jul 2017 13:30:32 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:55476 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753382AbdGNRaa (ORCPT ); Fri, 14 Jul 2017 13:30:30 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 3A8E460234 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=sboyd@codeaurora.org Date: Fri, 14 Jul 2017 10:30:28 -0700 From: Stephen Boyd To: Kiran Gunda Cc: gregkh@linuxfoundation.org, Abhijeet Dharmapurikar , David Collins , 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 Message-ID: <20170714173028.GI22780@codeaurora.org> References: <1499737497-25649-1-git-send-email-kgunda@codeaurora.org> <1499737497-25649-4-git-send-email-kgunda@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1499737497-25649-4-git-send-email-kgunda@codeaurora.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4911 Lines: 138 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