| 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
| ||
|
Date: Fri, 23 Jan 2015 12:36:58 -0700
From: Gilad Avidov <gavidov@...eaurora.org>
To: Stephen Boyd <sboyd@...eaurora.org>, sdharia@...eaurora.org,
mlocke@...eaurora.org, linux-arm-msm@...r.kernel.org,
gregkh@...uxfoundation.org
CC: linux-kernel@...r.kernel.org, iivanov@...sol.com,
galak@...eaurora.org, agross@...eaurora.org
Subject: Re: [PATCH] spmi: pmic_arb: add support for hw version 2
Hi Stephen,
On 1/21/2015 11:56 AM, Stephen Boyd wrote:
> On 01/19/2015 05:10 PM, Gilad Avidov wrote:
>> Qualcomm PMIC Arbiter version-2 changes from version-1 are:
>>
>> - Some diffrent register offsets.
> s/diffrent/different/
will correct the spelling. Thank you for pointing them out.
>
>
>
>> - New channel register space, one per PMIC peripheral (ppid).
>> All tx tarffic uses these channels.
> s/tarffic/traffic/
>
>> - New observer register space. All rx trafic uses this space.
> s/trafic/traffic/
>
>> - Diffrent command format for spmi command registers.
> s/Diffrent/Different/
>
> Please run spell check.
>
>> Signed-off-by: Gilad Avidov <gavidov@...eaurora.org>
>> Acked-by: Sagar Dharia <sdharia@...eaurora.org>
>> ---
>> .../bindings/spmi/qcom,spmi-pmic-arb.txt | 11 +-
>> drivers/spmi/spmi-pmic-arb.c | 295 ++++++++++++++++++---
>> 2 files changed, 263 insertions(+), 43 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
>> index 715d099..827bd21 100644
>> --- a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
>> +++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
>> @@ -1,11 +1,11 @@
>> Qualcomm SPMI Controller (PMIC Arbiter)
>>
>> -The SPMI PMIC Arbiter is found on the Snapdragon 800 Series. It is an SPMI
>> +The SPMI PMIC Arbiter is found on Snapdragon chipsets. It is an SPMI
>> controller with wrapping arbitration logic to allow for multiple on-chip
>> devices to control a single SPMI master.
>>
>> -The PMIC Arbiter can also act as an interrupt controller, providing interrupts
>> -to slave devices.
>> +The PMIC Arbiter is also an interrupt controller, interrupting the Snapdragon
>> +on dtection of a sequence initiated by a request-capable-slave to the master.
> s/dtection/detection/
>
> Honestly I don't see why this part needs to change either. Please drop
> this hunk.
will drop this.
>
>>
>> See spmi.txt for the generic SPMI controller binding requirements for child
>> nodes.
>> @@ -38,6 +38,11 @@ Required properties:
>> cell 4: interrupt flags indicating level-sense information, as defined in
>> dt-bindings/interrupt-controller/irq.h
>>
>> +Optional properties:
>> +- reg-names : may have "chnls", "obsrvr"
>> + "chnls" - tx-channel per virtual slave registers.
>> + "obsrvr" - rx-channel (called observer) per virtual slave registers.
>> +
> There's already a reg-names in this document and it's not optional.
> Please merge the two.
The existing regs are required for both v1 and v2, while the new ones
are required for v2 only.
Will combine with the existing reg-name as follows:
- reg-names : must contain:
"core" - core registers
...
v2 pmic arbiter registers:
"chnls" - tx-channel per virtual slave registers.
"obsrvr" - rx-channel (called observer) per virtual slave registers.
>
>> Example:
>>
>> spmi {
>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
>> index 246e03a..d12979a 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -25,16 +25,18 @@
>>
>> /* PMIC Arbiter configuration registers */
>> #define PMIC_ARB_VERSION 0x0000
>> +#define PMIC_ARB_VERSION_V2_MIN (0x20010000)
>> #define PMIC_ARB_INT_EN 0x0004
>>
>> -/* PMIC Arbiter channel registers */
>> -#define PMIC_ARB_CMD(N) (0x0800 + (0x80 * (N)))
>> -#define PMIC_ARB_CONFIG(N) (0x0804 + (0x80 * (N)))
>> -#define PMIC_ARB_STATUS(N) (0x0808 + (0x80 * (N)))
>> -#define PMIC_ARB_WDATA0(N) (0x0810 + (0x80 * (N)))
>> -#define PMIC_ARB_WDATA1(N) (0x0814 + (0x80 * (N)))
>> -#define PMIC_ARB_RDATA0(N) (0x0818 + (0x80 * (N)))
>> -#define PMIC_ARB_RDATA1(N) (0x081C + (0x80 * (N)))
>> +/* PMIC Arbiter channel registers offsets */
>> +#define PMIC_ARB_CMD (0x00)
>> +#define PMIC_ARB_CONFIG (0x04)
>> +#define PMIC_ARB_STATUS (0x08)
>> +#define PMIC_ARB_WDATA0 (0x10)
>> +#define PMIC_ARB_WDATA1 (0x14)
>> +#define PMIC_ARB_RDATA0 (0x18)
>> +#define PMIC_ARB_RDATA1 (0x1C)
>> +#define PMIC_ARB_REG_CHNL(N) (0x800 + 0x4 * (N))
>>
>> /* Interrupt Controller */
>> #define SPMI_PIC_OWNER_ACC_STATUS(M, N) (0x0000 + ((32 * (M)) + (4 * (N))))
> It looks like these macros would change too, but nothing has been done
> here. Interrupts haven't been tested?
>
>> @@ -52,6 +54,7 @@
>>
>> #define SPMI_MAPPING_TABLE_LEN 255
>> #define SPMI_MAPPING_TABLE_TREE_DEPTH 16 /* Maximum of 16-bits */
>> +#define PPID_TO_CHAN_TABLE_SZ BIT(12) /* PPID is 12bit chan is 1byte*/
>>
>> /* Ownership Table */
>> #define SPMI_OWNERSHIP_TABLE_REG(N) (0x0700 + (4 * (N)))
>> @@ -88,6 +91,7 @@ enum pmic_arb_cmd_op_code {
>>
>> /* Maximum number of support PMIC peripherals */
>> #define PMIC_ARB_MAX_PERIPHS 256
>> +#define PMIC_ARB_MAX_CHNL 128
>> #define PMIC_ARB_PERIPH_ID_VALID (1 << 15)
>> #define PMIC_ARB_TIMEOUT_US 100
>> #define PMIC_ARB_MAX_TRANS_BYTES (8)
>> @@ -98,14 +102,17 @@ enum pmic_arb_cmd_op_code {
>> /* interrupt enable bit */
>> #define SPMI_PIC_ACC_ENABLE_BIT BIT(0)
>>
>> +struct pmic_arb_ver;
>> +
>> /**
>> * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
>> *
>> - * @base: address of the PMIC Arbiter core registers.
>> + * @rd_base: on v1 "core", on v2 "observer" register base off DT.
>> + * @rd_base: on v1 "core", on v2 "chnls" register base off DT.
>> * @intr: address of the SPMI interrupt control registers.
>> * @cnfg: address of the PMIC Arbiter configuration registers.
>> * @lock: lock to synchronize accesses.
>> - * @channel: which channel to use for accesses.
>> + * @channel: which ee channel to use for accesses.
> Looks like an unnecessary change.
Added the "ee" to the channel here, to differentiate it from the new
channel registers.
It should decrease confusion between the two unrelated concept named
channel.
>
>> * @irq: PMIC ARB interrupt.
>> * @ee: the current Execution Environment
>> * @min_apid: minimum APID (used for bounding IRQ search)
>> @@ -114,9 +121,11 @@ enum pmic_arb_cmd_op_code {
>> * @domain: irq domain object for PMIC IRQ domain
>> * @spmic: SPMI controller object
>> * @apid_to_ppid: cached mapping from APID to PPID
>> + * @ppid_to_chan cached mapping from APID to channel number, v2 only.
>> */
>> struct spmi_pmic_arb_dev {
>> - void __iomem *base;
>> + void __iomem *rd_base;
>> + void __iomem *wr_base;
>> void __iomem *intr;
>> void __iomem *cnfg;
>> raw_spinlock_t lock;
>> @@ -129,17 +138,61 @@ struct spmi_pmic_arb_dev {
>> struct irq_domain *domain;
>> struct spmi_controller *spmic;
>> u16 apid_to_ppid[256];
>> + const struct pmic_arb_ver *ver;
>> + u8 *ppid_to_chan;
>> +};
>> +
>> +/**
>> + * pmic_arb_ver: version dependent functionality and values.
>> + *
>> + * @non_data_cmd: on v1 issues an spmi non-data command.
>> + * on v2 no HW support, returns -EOPNOTSUPP.
>> + * @offset: on v1 offset of per-ee channel.
>> + * on v2 offset of per-ee and per-ppid channel.
>> + * @fmt_cmd: formats a GENI/SPMI command.
>> + * @owner_acc_status: on v1 offset of PMIC_ARB_SPMI_PIC_OWNERm_ACC_STATUSn
>> + * on v2 offset of SPMI_PIC_OWNERm_ACC_STATUSn.
>> + * @acc_enable: on v1 offset of PMIC_ARB_SPMI_PIC_ACC_ENABLEn
>> + * on v2 offset of SPMI_PIC_ACC_ENABLEn.
>> + * @irq_status: on v1 offset of PMIC_ARB_SPMI_PIC_IRQ_STATUSn
>> + * on v2 offset of SPMI_PIC_IRQ_STATUSn.
>> + * @irq_clear: on v1 offset of PMIC_ARB_SPMI_PIC_IRQ_CLEARn
>> + * on v2 offset of SPMI_PIC_IRQ_CLEARn.
>> + * @geni_ctrl: PMIC_ARB_GENI_CTRL offset.
>> + * @geni_status: PMIC_ARB_GENI_STATUS offset.
>> + * @protocol_irq_status: PMIC_ARB_PROTOCOL_IRQ_STATUS offset.
>> + */
>> +struct pmic_arb_ver {
>> + int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
>> + /* SPMI commands (including data) related functionality */
>> + off_t (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr);
> Isn't off_t for file offsets? It doesn't seem right to use it for
> register offsets. Please use u32 or something similar.
>
Agree.
Thanks,
Gilad
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists