From mboxrd@z Thu Jan 1 00:00:00 1970 From: kgunda@codeaurora.org Subject: Re: [PATCH V1 13/15] spmi: pmic-arb: add support for HW version 5 Date: Thu, 08 Jun 2017 16:58:29 +0530 Message-ID: <24975b3ca3a92d42e4ce7693abe4bd2f@codeaurora.org> References: <1496147943-25822-1-git-send-email-kgunda@codeaurora.org> <1496147943-25822-14-git-send-email-kgunda@codeaurora.org> <20170601060859.GK20170@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:47028 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750788AbdFHL2b (ORCPT ); Thu, 8 Jun 2017 07:28:31 -0400 In-Reply-To: <20170601060859.GK20170@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Stephen Boyd Cc: Abhijeet Dharmapurikar , Greg Kroah-Hartman , Subbaraman Narayanamurthy , David Collins , Christophe JAILLET , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, adharmap@quicinc.com, aghayal@qti.qualcomm.com, linux-arm-msm-owner@vger.kernel.org On 2017-06-01 11:38, Stephen Boyd wrote: > On 05/30, Kiran Gunda wrote: >> diff --git a/drivers/spmi/spmi-pmic-arb.c >> b/drivers/spmi/spmi-pmic-arb.c >> index 412481d..b755c24 100644 >> --- a/drivers/spmi/spmi-pmic-arb.c >> +++ b/drivers/spmi/spmi-pmic-arb.c >> @@ -112,7 +123,8 @@ enum pmic_arb_cmd_op_code { >> >> struct apid_data { >> u16 ppid; >> - u8 owner; >> + u8 write_owner; >> + u8 irq_owner; > > How about irq_ee and write_ee instead? > Sure. will change it in the subsequent patch. >> }; >> >> /** >> static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa, >> @@ -705,11 +724,18 @@ static int >> qpnpint_irq_domain_dt_translate(struct irq_domain *d, >> (intspec[1] << 8), &apid); >> if (rc < 0) { >> dev_err(&pa->spmic->dev, >> - "failed to xlate sid = 0x%x, periph = 0x%x, irq = %x rc = %d\n", >> + "failed to xlate sid = 0x%x, periph = 0x%x, irq = %u rc = %d\n", > > What is this change? Also, use %#x instead of 0x%x and the " > should line up with the ( on the previous line. > Sure. will change it in the subsequent patch. >> intspec[0], intspec[1], intspec[2], rc); >> return rc; >> } >> >> + if (pa->apid_data[apid].irq_owner != pa->ee) { >> + dev_err(&pa->spmic->dev, "failed to xlate sid = 0x%x, periph = >> 0x%x, irq = %u: ee=%u but owner=%u\n", >> + intspec[0], intspec[1], intspec[2], pa->ee, >> + pa->apid_data[apid].irq_owner); >> + return -ENODEV; >> + } >> + >> /* Keep track of {max,min}_apid for bounding search during interrupt >> */ >> if (apid > pa->max_apid) >> pa->max_apid = apid; >> @@ -814,9 +841,11 @@ static u16 pmic_arb_find_apid(struct >> spmi_pmic_arb *pa, u16 ppid) >> for (apid = pa->last_apid; apid < pa->max_periph; apid++) { >> regval = readl_relaxed(pa->cnfg + >> SPMI_OWNERSHIP_TABLE_REG(apid)); >> - pa->apid_data[apid].owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval); >> + pa->apid_data[apid].irq_owner >> + = SPMI_OWNERSHIP_PERIPH2OWNER(regval); >> + pa->apid_data[apid].write_owner = pa->apid_data[apid].irq_owner; > > Please use a local variable pointer *apid. > Sure. will change it in the subsequent patch. >> >> - offset = PMIC_ARB_REG_CHNL(apid); >> + offset = pa->ver_ops->channel_map_offset(apid); >> if (offset >= pa->core_size) >> break; >> >> @@ -854,27 +883,110 @@ static u16 pmic_arb_find_apid(struct >> spmi_pmic_arb *pa, u16 ppid) >> return 0; >> } >> >> +static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pa) >> +{ >> + u32 regval, offset; >> + u16 apid, prev_apid, ppid; >> + bool valid, is_irq_owner; >> + >> + /* >> + * PMIC_ARB_REG_CHNL is a table in HW mapping APID (channel) to >> PPID. >> + * ppid_to_apid is an in-memory invert of that table. In order to >> allow >> + * multiple EE's to write to a single PPID in arbiter version 5, >> there > > Drop the apostrophe ^ > Ok. will change it in the subsequent patch. >> + * 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 (apid = 0; apid < pa->max_periph; apid++) { >> + offset = pa->ver_ops->channel_map_offset(apid); >> + if (offset >= pa->core_size) >> + break; >> + >> + regval = readl_relaxed(pa->core + offset); >> + if (!regval) >> + continue; >> + ppid = (regval >> 8) & PMIC_ARB_PPID_MASK; >> + is_irq_owner = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval); >> + >> + regval = readl_relaxed(pa->cnfg + >> + SPMI_OWNERSHIP_TABLE_REG(apid)); >> + pa->apid_data[apid].write_owner >> + = SPMI_OWNERSHIP_PERIPH2OWNER(regval); > > Please use a pointer like *apid and *prev where *apid is > incremented during the for-loop. That way we have shorter lines > of code: > > apid->write_owner = SPMI_OWNERSHIP_TABLE_REG(regval); > > and > > if (valid && is_irq_owner && prev->write_owner == pa->ee) { > > obviously the existing apid will need to be renamed to i, but > that's ok because it's a counter. > Sure. will do it in the subsequent patch.. >> + >> + pa->apid_data[apid].irq_owner = is_irq_owner ? >> + pa->apid_data[apid].write_owner : INVALID_EE; >> + >> + valid = pa->ppid_to_apid[ppid] & PMIC_ARB_CHAN_VALID; >> + prev_apid = pa->ppid_to_apid[ppid] & ~PMIC_ARB_CHAN_VALID; >> + >> + if (valid && is_irq_owner && >> + pa->apid_data[prev_apid].write_owner == pa->ee) { >> + /* >> + * Duplicate PPID mapping after the one for this EE; >> + * override the irq owner >> + */ >> + pa->apid_data[prev_apid].irq_owner >> + = pa->apid_data[apid].irq_owner; >> + } else if (!valid || is_irq_owner) { >> + /* First PPID mapping or duplicate for another EE */ >> + pa->ppid_to_apid[ppid] = apid | PMIC_ARB_CHAN_VALID; >> + } >> + >> + pa->apid_data[apid].ppid = ppid; >> + pa->last_apid = apid; >> + } >> + >> + /* Dump the mapping table for debug purposes. */ >> + dev_dbg(&pa->spmic->dev, "PPID APID Write-EE IRQ-EE\n"); >> + for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) { >> + valid = pa->ppid_to_apid[ppid] & PMIC_ARB_CHAN_VALID; >> + apid = pa->ppid_to_apid[ppid] & ~PMIC_ARB_CHAN_VALID; >> + >> + if (valid) >> + dev_dbg(&pa->spmic->dev, "0x%03X %3u %2u %2u\n", > > Same %#x story here. > Sure. will change it in the subsequent patch. >> + ppid, apid, pa->apid_data[apid].write_owner, >> + pa->apid_data[apid].irq_owner); >> + } >> + >> + return 0; >> +} >> + >> >> @@ -887,6 +999,27 @@ static u16 pmic_arb_find_apid(struct >> spmi_pmic_arb *pa, u16 ppid) >> return 0; >> } >> >> +/* >> + * v5 offset per ee and per apid for observer channels and per apid >> for >> + * read/write channels. >> + */ >> +static int >> +pmic_arb_offset_v5(struct spmi_pmic_arb *pa, u8 sid, u16 addr, >> + enum pmic_arb_channel ch_type, u32 *offset) >> +{ >> + u16 apid; >> + int rc; >> + >> + rc = pmic_arb_ppid_to_apid_v5(pa, sid, addr, &apid); >> + if (rc < 0) >> + return rc; >> + >> + *offset = (ch_type == PMIC_ARB_CHANNEL_OBS) >> + ? 0x10000 * pa->ee + 0x80 * apid >> + : 0x10000 * apid; > > Please use a switch statement to handle the enum here. That way > we can get compiler checking to make sure all enumerations are > handled. > Sure. will change it in the subsequent patch. >> + return 0; >> +} >> + >> static u32 pmic_arb_fmt_cmd_v1(u8 opc, u8 sid, u16 addr, u8 bc) >> { >> return (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7); >> @@ -1033,11 +1213,14 @@ static int spmi_pmic_arb_probe(struct >> platform_device *pdev) >> >> if (hw_ver < PMIC_ARB_VERSION_V3_MIN) >> pa->ver_ops = &pmic_arb_v2; >> - else >> + else if (hw_ver < PMIC_ARB_VERSION_V5_MIN) >> pa->ver_ops = &pmic_arb_v3; >> + else >> + pa->ver_ops = &pmic_arb_v5; >> >> - /* the apid to ppid table starts at PMIC_ARB_REG_CHNL(0) */ >> - pa->max_periph = (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4; >> + /* the apid to ppid table starts at PMIC_ARB_REG_CHNL0 */ >> + pa->max_periph >> + = (pa->core_size - pa->ver_ops->channel_map_offset(0)) / 4; > > This is really ugly. Please grow a local variable so we can keep > things on one line. > Ok. will change it in the subsequent patch. >> >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> "obsrvr"); >> @@ -1074,6 +1257,14 @@ static int spmi_pmic_arb_probe(struct >> platform_device *pdev) >> err = PTR_ERR(pa->intr); >> goto err_put_ctrl; >> } >> + pa->acc_status = pa->intr; >> + >> + /* >> + * PMIC arbiter v5 groups the IRQ control registers in the same >> hardware >> + * module as the read/write channels. >> + */ >> + if (hw_ver >= PMIC_ARB_VERSION_V5_MIN) >> + pa->intr = pa->wr_base; > > There's some weird things going on here. How about we make the > version ops return an __iomem pointer to the address instead of > an offset? That way we don't need to care what pa->intr is or add > pa->acc_status? That could be a patch early in the series that > adjusts the ops to return an iomem pointer, and perhaps also > update the 'n' variable to be 16 instead of 8 bits. We can also > express errors through iomem pointers with IS_ERR() checks, so it > nicely removes the need to retrieve offsets through references > sometimes too. Yes. This looks good. Will change it in the subsequent patch series.