MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Thu, 06 Jul 2017 16:18:40 +0530
From: kgunda@codeaurora.org
To: Stephen Boyd <sboyd@codeaurora.org>
Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>,
        David Collins <collinsd@codeaurora.org>, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, linux-arm-msm-owner@vger.kernel.org
Subject: Re: [PATCH V1 3/4] spmi: pmic-arb: add support for HW version 5
In-Reply-To: <20170706065930.GG22780@codeaurora.org>
References: <1499086338-19498-1-git-send-email-kgunda@codeaurora.org>
 <1499086338-19498-4-git-send-email-kgunda@codeaurora.org>
 <20170706065930.GG22780@codeaurora.org>
Message-ID: <83b580a6ff40515cd4e99412228829ee@codeaurora.org>
User-Agent: Roundcube Webmail/1.2.5
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4076
Lines: 133

On 2017-07-06 12:29, Stephen Boyd wrote:
> On 07/03, Kiran Gunda wrote:
>> From: David Collins <collinsd@codeaurora.org>
>> 
>> Add support for version 5 of the SPMI PMIC arbiter.  It utilizes
>> different offsets for registers than those found on version 3.
>> Also, the procedure to determine if writing and IRQ access is
>> allowed for a given PPID changes for version 5.
>> 
> 
> David's sign-off is missing?
> 
Looks like mistakenly removed along with change-id. Will add it in the 
next patch.
>> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> ---
>>  drivers/spmi/spmi-pmic-arb.c | 233 
>> +++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 214 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/spmi/spmi-pmic-arb.c 
>> b/drivers/spmi/spmi-pmic-arb.c
>> index 86affb0..bc88c19 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -1,5 +1,5 @@
>>  /*
>> - * Copyright (c) 2012-2015, The Linux Foundation. All rights 
>> reserved.
>> + * Copyright (c) 2012-2017, The Linux Foundation. All rights 
>> reserved.
>>   *
>>   * This program is free software; you can redistribute it and/or 
>> modify
>>   * it under the terms of the GNU General Public License version 2 and
>> @@ -29,6 +29,7 @@
>>  #define PMIC_ARB_VERSION		0x0000
>>  #define PMIC_ARB_VERSION_V2_MIN		0x20010000
>>  #define PMIC_ARB_VERSION_V3_MIN		0x30000000
>> +#define PMIC_ARB_VERSION_V5_MIN		0x50000000
>>  #define PMIC_ARB_INT_EN			0x0004
>> 
>>  /* PMIC Arbiter channel registers offsets */
>> @@ -39,7 +40,6 @@
>>  #define PMIC_ARB_WDATA1			0x14
>>  #define PMIC_ARB_RDATA0			0x18
>>  #define PMIC_ARB_RDATA1			0x1C
>> -#define PMIC_ARB_REG_CHNL(N)		(0x800 + 0x4 * (N))
>> 
>>  /* Mapping Table */
>>  #define SPMI_MAPPING_TABLE_REG(N)	(0x0B00 + (4 * (N)))
>> @@ -52,6 +52,8 @@
>>  #define SPMI_MAPPING_TABLE_TREE_DEPTH	16	/* Maximum of 16-bits */
>>  #define PMIC_ARB_MAX_PPID		BIT(12) /* PPID is 12bit */
>>  #define PMIC_ARB_APID_VALID		BIT(15)
>> +#define PMIC_ARB_CHAN_IS_IRQ_OWNER(reg)	((reg) & BIT(24))
>> +#define INVALID				(-1)
> 
> INVALID_EE? And then define it to be 0xff?
> 
ok... will modify it.
>> 
>>  /* Ownership Table */
>>  #define SPMI_OWNERSHIP_TABLE_REG(N)	(0x0700 + (4 * (N)))
>> @@ -86,6 +88,15 @@ enum pmic_arb_cmd_op_code {
>>  	PMIC_ARB_OP_ZERO_WRITE = 16,
>>  };
>> 
>> +/*
>> + * PMIC arbiter version 5 uses different register offsets for 
>> read/write vs
>> + * observer channels.
>> + */
>> +enum pmic_arb_channel {
>> +	PMIC_ARB_CHANNEL_RW,
>> +	PMIC_ARB_CHANNEL_OBS,
>> +};
>> +
>>  /* Maximum number of support PMIC peripherals */
>>  #define PMIC_ARB_MAX_PERIPHS		512
> [...]
>> @@ -112,7 +123,8 @@ enum pmic_arb_cmd_op_code {
>> 
>> +
>>  /* v2 offset per ppid and per ee */
>>  static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid, 
>> u16 addr,
>> -				u32 *offset)
>> +			   enum pmic_arb_channel ch_type, u32 *offset)
>>  {
>>  	u16 apid;
>>  	u16 ppid;
>> @@ -841,6 +947,34 @@ static int pmic_arb_offset_v2(struct 
>> spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
>>  	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 *pmic_arb, u8 sid, 
>> u16 addr,
>> +			   enum pmic_arb_channel ch_type, u32 *offset)
>> +{
>> +	u16 apid;
>> +	int rc;
>> +	u16 ppid = (sid << 8) | (addr >> 8);
>> +
>> +	rc = pmic_arb_ppid_to_apid_v5(pmic_arb, ppid);
>> +	if (rc < 0)
>> +		return rc;
>> +
>> +	apid = rc;
>> +	switch (ch_type) {
>> +	case PMIC_ARB_CHANNEL_OBS:
>> +		*offset = 0x10000 * pmic_arb->ee + 0x80 * apid;
>> +		break;
>> +	case PMIC_ARB_CHANNEL_RW:
>> +		*offset = 0x10000 * apid;
>> +		break;
>> +	}
>> +
>> +	return 0;
> 
> Why doesn't this also return an iomem pointer?
Actually the purpose of this function is to find the offset of the APID.
That's why we are not returning the iomem pointer. But we can return the 
offset
from this function, instead of a pass by pointer. Will change it to 
return the offset
in next patch.