| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <badd4dbcb1088e7436d582fc5b3ab70e@codeaurora.org>
Date: Thu, 01 Jun 2017 22:07:52 +0530
From: kgunda@...eaurora.org
To: Stephen Boyd <sboyd@...eaurora.org>
Cc: Abhijeet Dharmapurikar <adharmap@...eaurora.org>,
Subbaraman Narayanamurthy <subbaram@...eaurora.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
David Collins <collinsd@...eaurora.org>,
Christophe JAILLET <christophe.jaillet@...adoo.fr>,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
adharmap@...cinc.com, aghayal@....qualcomm.com,
linux-arm-msm-owner@...r.kernel.org
Subject: Re: [PATCH V1 03/15] spmi: pmic-arb: fix inconsistent use of apid and
chan
On 2017-05-31 07:01, Stephen Boyd wrote:
> On 05/30, Kiran Gunda wrote:
>> From: Abhijeet Dharmapurikar <adharmap@...eaurora.org>
>>
>> The driver currently uses "apid" and "chan" to mean apid. Remove
>> the use of chan and use only apid.
>
> I'm not so sure. It currently uses "chan" to mean the offset to
> add to the "PMIC Arbiter channel registers" so that we can access
> the appropriate peripheral via the arbiter registers. I actually
> can't remember what APID or PPID stand for, so perhaps describing
> that as well would be helpful so we can navigate this acronym
> soup.
>
Yes. You are correct.
Will describe the "apid" and "ppid" in the next version patch.
>>
>> On a SPMI bus there is allocation to manage up to 4K peripherals.
>> However, in practice only few peripherals are instantiated
>> and only few among the instantiated ones actually interrupt.
>>
>> APID is CPU's way of keeping track of peripherals that could
>> interrupt.
>> There is a table that maps the 256 interrupting peripherals to
>> a number between 0 and 255. This number is called APID. Information
>> about
>> that interrupting peripheral is stored in registers offset by its
>> corresponding apid.
>
> That's all fine, but perhaps we shouldn't worry about "apid"
> being attached to interrupts? I mean, I can imagine some
> peripheral that doesn't interrupt, but we want to read/write it
> and that must be done with the "channel" or "apid" or really the
> "magic offset from the base of the channel registers" to do so.
> Probably APID is fine, as long as APID means "application
> processor peripheral id" or something along those lines.
>
Yes. you are right. APID means "Application peripheral id".
>>
>> Signed-off-by: Abhijeet Dharmapurikar <adharmap@...eaurora.org>
>> Signed-off-by: Kiran Gunda <kgunda@...eaurora.org>
>> ---
>> drivers/spmi/spmi-pmic-arb.c | 68
>> ++++++++++++++++++++++----------------------
>> 1 file changed, 34 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/spmi/spmi-pmic-arb.c
>> b/drivers/spmi/spmi-pmic-arb.c
>> index 7f918ea..7201611 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -117,7 +117,7 @@ enum pmic_arb_cmd_op_code {
>> * @spmic: SPMI controller object
>> * @apid_to_ppid: in-memory copy of APID -> PPID mapping table.
>> * @ver_ops: version dependent operations.
>> - * @ppid_to_chan in-memory copy of PPID -> channel (APID) mapping
>> table.
>> + * @ppid_to_apid in-memory copy of PPID -> channel (APID) mapping
>> table.
>
> PPID->APID? No channel?
Sure. Will change it the next patch.
>
>> * v2 only.
>> */
>> struct spmi_pmic_arb {
>> @@ -140,9 +140,9 @@ struct spmi_pmic_arb {
>> struct spmi_controller *spmic;
>> u16 *apid_to_ppid;
>> const struct pmic_arb_ver_ops *ver_ops;
>> - u16 *ppid_to_chan;
>> - u16 last_channel;
>> - u8 *chan_to_owner;
>> + u16 *ppid_to_apid;
>> + u16 last_apid;
>> + u8 *apid_to_owner;
>> };
>>
>> /**
>> @@ -772,22 +772,22 @@ static int qpnpint_irq_domain_map(struct
>> irq_domain *d,
>> return 0;
>> }
>>
>> -static u16 pmic_arb_find_chan(struct spmi_pmic_arb *pa, u16 ppid)
>> +static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid)
>> {
>> u32 regval, offset;
>> - u16 chan;
>> + u16 apid;
>> u16 id;
>>
>> /*
>> * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
>
> Is this comment still relevant?
We will change channel to apid in the next patch.
>
>> - * ppid_to_chan is an in-memory invert of that table.
>> + * ppid_to_apid is an in-memory invert of that table.
>> */
>> - for (chan = pa->last_channel; chan < pa->max_periph; chan++) {
>> + for (apid = pa->last_apid; apid < pa->max_periph; apid++) {
>> regval = readl_relaxed(pa->cnfg +
>> - SPMI_OWNERSHIP_TABLE_REG(chan));
>> - pa->chan_to_owner[chan] = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
>> + SPMI_OWNERSHIP_TABLE_REG(apid));
>> + pa->apid_to_owner[apid] = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
>>
>> - offset = PMIC_ARB_REG_CHNL(chan);
>> + offset = PMIC_ARB_REG_CHNL(apid);
>> if (offset >= pa->core_size)
>> break;
>>
>> @@ -796,15 +796,15 @@ static u16 pmic_arb_find_chan(struct
>> spmi_pmic_arb *pa, u16 ppid)
>> continue;
>>
>> id = (regval >> 8) & PMIC_ARB_PPID_MASK;
>> - pa->ppid_to_chan[id] = chan | PMIC_ARB_CHAN_VALID;
>> + pa->ppid_to_apid[id] = apid | PMIC_ARB_CHAN_VALID;
>
> Why do we still call the flag PMIC_ARB_CHAN_VALID then? Shouldn't
> it be PMIC_ARB_APID_VALID?
>
Yes. Agree. Will change it to PMIC_ARB_APID_VALID in the next patch.
>> if (id == ppid) {
>> - chan |= PMIC_ARB_CHAN_VALID;
>> + apid |= PMIC_ARB_CHAN_VALID;
>> break;
>> }
>> }
>> - pa->last_channel = chan & ~PMIC_ARB_CHAN_VALID;
>> + pa->last_apid = apid & ~PMIC_ARB_CHAN_VALID;
>>
>> - return chan;
>> + return apid;
>> }
>>
>>
Powered by blists - more mailing lists