add support for pmic_arb v2 and correct framework
Subsystems (3)
v4 cover add support for pmic_arb v2 and correct framework
pmic_arb v2 has no support for spmi non-data commands and thus returns -EOPNOTSUPP on .cmd callback. This causes a failure in spmi_drv_probe() which sends a wakeup command to the slave before probing its driver. This patchset removes the wakeup from spmi_drv_probe() since the spmi spec stipulates that a slaves default state is active and doesn't need a wakeup.
Changelog from v3 to v4:
spmi: remove wakeup command before slave probe:
1. Remove the claim that this is a bug fix off the commit text.
spmi: pmic_arb: add support for hw version 2
1. Unmap the core register space as soon as we know that it will not be used (on v2 it is used only to read the hw version in probe).
3. Assign the core reg space to a local until we know if it appropriate to use it to configure the controller fields (on v1) or unmap it (on v2).
Gilad Avidov (2):
spmi: remove wakeup command before slave probe
spmi: pmic_arb: add support for hw version 2
.../bindings/spmi/qcom,spmi-pmic-arb.txt | 6 +- drivers/spmi/spmi-pmic-arb.c | 315 +++++++++++++++++---- drivers/spmi/spmi.c | 9 +- 3 files changed, 266 insertions(+), 64 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
PATCH v4 1/2 spmi: remove wakeup command before slave probe
According to spmi spec a slave powers up into startup state and then transitions into active state. Thus, the wakeup command is not required before calling the slave's probe. The wakeup command is only needed for slaves that are in sleep state after receiving the sleep command.
--- drivers/spmi/spmi.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
drivers/spmi/spmi.c +2 -7
@@ -1,4 +1,5 @@ -/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. +/* +* Copyright (c) 2012-2015, 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
@@ -316,11 +317,6 @@ static int spmi_drv_probe(struct device *dev)struct spmi_device *sdev =to_spmi_device (dev);int err; -/* Ensure the slave is in ACTIVE state */ - err =spmi_command_wakeup (sdev); -if (err) - goto fail_wakeup; -pm_runtime_get_noresume (dev);pm_runtime_set_active (dev);pm_runtime_enable (dev);
@@ -335,7 +331,6 @@ fail_probe:pm_runtime_disable (dev);pm_runtime_set_suspended (dev);pm_runtime_put_noidle (dev); -fail_wakeup:return err; }
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
PATCH v4 2/2 spmi: pmic_arb: add support for hw version 2
In reply to Gilad AvidovQualcomm PMIC Arbiter version-2 changes from version-1 are:
- Some different register offsets.
- New channel register space, one per PMIC peripheral (ppid). All tx traffic uses these channels.
- New observer register space. All rx trafic uses this space.
- Different command format for spmi command registers.
--- .../bindings/spmi/qcom,spmi-pmic-arb.txt | 6 +- drivers/spmi/spmi-pmic-arb.c | 315 +++++++++++++++++---- 2 files changed, 264 insertions(+), 57 deletions(-)
Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt +5 -1
@@ -1,6 +1,6 @@ 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.
@@ -19,6 +19,10 @@ Required properties: "core" - core registers "intr" - interrupt controller registers "cnfg" - configuration registers + Registers used only for V2 PMIC Arbiter: + "chnls" - tx-channel per virtual slave registers. + "obsrvr" - rx-channel (called observer) per virtual slave registers. + - reg : address + size pairs describing the PMIC arb register sets; order must correspond with the order of entries in reg-names - #address-cells : must be set to 2
drivers/spmi/spmi-pmic-arb.c +259 -56
505 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH V4 2/2] spmi: pmic_arb: add support for hw version 2
Hi Gilad,
Quoted 11 lines · Gilad Avidov
Qualcomm PMIC Arbiter version-2 changes from version-1 are:
- Some different register offsets.
- New channel register space, one per PMIC peripheral (ppid). All tx traffic uses these channels.
- New observer register space. All rx trafic uses this space.
- Different command format for spmi command registers.Acked-by Sagar Dharia <[email protected]>Signed-off-by Gilad Avidov <[email protected]
<snip>
Quoted 33 lines
drivers/spmi/spmi-pmic-arb.c +23
-3@@ -645,12 +796,67 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) pa->spmic = ctrl; res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"core" ); - pa->base =devm_ioremap_resource (&ctrl->dev, res); -if (IS_ERR (pa->base)) { - err =PTR_ERR (pa->base); + core =devm_ioremap_resource (&ctrl->dev, res); +if (IS_ERR (core)) { + err =PTR_ERR (core); goto err_put_ctrl; } + hw_ver =readl_relaxed (core +PMIC_ARB_VERSION ); + is_v1 = (hw_ver <PMIC_ARB_VERSION_V2_MIN ); + +dev_info (&ctrl->dev,"PMIC Arb Version-%d (0x%x)\n" , (is_v1 ?1 :2 ), + hw_ver); + +if (is_v1) { + pa->ver_ops = &pmic_arb_v1; + pa->wr_base = core; + pa->rd_base = core; + }else { +u8 chan; +u16 ppid; +u32 regval; + + pa->ver_ops = &pmic_arb_v2; +devm_iounmap (&ctrl->dev, core); + + pa->ppid_to_chan =devm_kzalloc (&ctrl->dev, +PPID_TO_CHAN_TABLE_SZ ,GFP_KERNEL );
drivers/spmi/spmi-pmic-arb.c static int spmi_pmic_arb_probe(struct platform_device *pdev) · line 834
Quoted 10 lines
drivers/spmi/spmi-pmic-arb.c +10
-0+if (!pa->ppid_to_chan) { + err = -ENOMEM ; + goto err_put_ctrl; + } +/* +* PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid. +* ppid_to_chan is an in-memory invert of that table. +*/ +for (chan =0 ; chan <PMIC_ARB_MAX_CHNL ; ++chan) { + regval = readl_relaxed(pa->rd_base +
rd_base is not initialized at this point.
drivers/spmi/spmi-pmic-arb.c static int spmi_pmic_arb_probe(struct platform_device *pdev) · line 845
Quoted 12 lines
drivers/spmi/spmi-pmic-arb.c +11
-0+ PMIC_ARB_REG_CHNL(chan)); +if (!regval) +continue ; + + ppid = (regval >>8 ) &0xFFF ; + pa->ppid_to_chan[ppid] = chan; + } + + res =platform_get_resource_byname (pdev,IORESOURCE_MEM , +"obsrvr" ); + pa->rd_base =devm_ioremap_resource (&ctrl->dev, res);
and they are compile warnings:
drivers/spmi/spmi-pmic-arb.c: In function ‘pmic_arb_read_cmd’:drivers/spmi/spmi-pmic-arb.c:310:4: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘size_t’ [-Wformat=]
PMIC_ARB_MAX_TRANS_BYTES, len);
^
drivers/spmi/spmi-pmic-arb.c: In function ‘pmic_arb_write_cmd’:
drivers/spmi/spmi-pmic-arb.c:357:4: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘size_t’ [-Wformat=]
PMIC_ARB_MAX_TRANS_BYTES, len);
^Ivan
Re: [PATCH V4 2/2] spmi: pmic_arb: add support for hw version 2
Ping.
Quoted 91 lines · Ivan T. Ivanov
Hi Gilad,
Quoted 11 lines · Gilad Avidov
Qualcomm PMIC Arbiter version-2 changes from version-1 are:
- Some different register offsets.
- New channel register space, one per PMIC peripheral (ppid). All tx traffic uses these channels.
- New observer register space. All rx trafic uses this space.
- Different command format for spmi command registers.Acked-by Sagar Dharia <[email protected]>Signed-off-by Gilad Avidov <[email protected]<snip>
Quoted 33 lines
drivers/spmi/spmi-pmic-arb.c +23
-3@@ -645,12 +796,67 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) pa->spmic = ctrl; res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"core" ); - pa->base =devm_ioremap_resource (&ctrl->dev, res); -if (IS_ERR (pa->base)) { - err =PTR_ERR (pa->base); + core =devm_ioremap_resource (&ctrl->dev, res); +if (IS_ERR (core)) { + err =PTR_ERR (core); goto err_put_ctrl; } + hw_ver =readl_relaxed (core +PMIC_ARB_VERSION ); + is_v1 = (hw_ver <PMIC_ARB_VERSION_V2_MIN ); + +dev_info (&ctrl->dev,"PMIC Arb Version-%d (0x%x)\n" , (is_v1 ?1 :2 ), + hw_ver); + +if (is_v1) { + pa->ver_ops = &pmic_arb_v1; + pa->wr_base = core; + pa->rd_base = core; + }else { +u8 chan; +u16 ppid; +u32 regval; + + pa->ver_ops = &pmic_arb_v2; +devm_iounmap (&ctrl->dev, core); + + pa->ppid_to_chan =devm_kzalloc (&ctrl->dev, +PPID_TO_CHAN_TABLE_SZ ,GFP_KERNEL );drivers/spmi/spmi-pmic-arb.c static int spmi_pmic_arb_probe(struct platform_device *pdev) · line 834
Quoted 10 lines
drivers/spmi/spmi-pmic-arb.c +10
-0+if (!pa->ppid_to_chan) { + err = -ENOMEM ; + goto err_put_ctrl; + } +/* +* PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid. +* ppid_to_chan is an in-memory invert of that table. +*/ +for (chan =0 ; chan <PMIC_ARB_MAX_CHNL ; ++chan) { + regval = readl_relaxed(pa->rd_base +
rd_baseis not initialized at this point.drivers/spmi/spmi-pmic-arb.c static int spmi_pmic_arb_probe(struct platform_device *pdev) · line 845
Quoted 12 lines
drivers/spmi/spmi-pmic-arb.c +11
-0+ PMIC_ARB_REG_CHNL(chan)); +if (!regval) +continue ; + + ppid = (regval >>8 ) &0xFFF ; + pa->ppid_to_chan[ppid] = chan; + } + + res =platform_get_resource_byname (pdev,IORESOURCE_MEM , +"obsrvr" ); + pa->rd_base =devm_ioremap_resource (&ctrl->dev, res);and they are compile warnings:
drivers/spmi/spmi-pmic-arb.c: In function ‘pmic_arb_read_cmd’:drivers/spmi/spmi-pmic-arb.c:310:4: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘size_t’ [-Wformat=]PMIC_ARB_MAX_TRANS_BYTES, len); ^ drivers/spmi/spmi-pmic-arb.c: In function ‘pmic_arb_write_cmd’: drivers/spmi/spmi-pmic-arb.c:357:4: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘size_t’ [-Wformat=] PMIC_ARB_MAX_TRANS_BYTES, len); ^Ivan
Re: [PATCH V4 2/2] spmi: pmic_arb: add support for hw version 2
In reply to Ivan T. IvanovHi Ivan,
Quoted 59 lines · On Mon, 23 Feb 2015 11:59:14 +0200 "Ivan T. Ivanov" <[email protected]> wrote:
Hi Gilad,
Quoted 11 lines · Gilad Avidov
Qualcomm PMIC Arbiter version-2 changes from version-1 are:
- Some different register offsets.
- New channel register space, one per PMIC peripheral (ppid). All tx traffic uses these channels.
- New observer register space. All rx trafic uses this space.
- Different command format for spmi command registers.Acked-by Sagar Dharia <[email protected]>Signed-off-by Gilad Avidov <[email protected]<snip>
Quoted 42 lines
drivers/spmi/spmi-pmic-arb.c +29
-3@@ -645,12 +796,67 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) pa->spmic = ctrl; res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"core" ); - pa->base =devm_ioremap_resource (&ctrl->dev, res); -if (IS_ERR (pa->base)) { - err =PTR_ERR (pa->base); + core =devm_ioremap_resource (&ctrl->dev, res); +if (IS_ERR (core)) { + err =PTR_ERR (core); goto err_put_ctrl; } + hw_ver =readl_relaxed (core +PMIC_ARB_VERSION ); + is_v1 = (hw_ver <PMIC_ARB_VERSION_V2_MIN ); + +dev_info (&ctrl->dev,"PMIC Arb Version-%d (0x%x)\n" , (is_v1 ?1 :2 ), + hw_ver); + +if (is_v1) { + pa->ver_ops = &pmic_arb_v1; + pa->wr_base = core; + pa->rd_base = core; + }else { +u8 chan; +u16 ppid; +u32 regval; + + pa->ver_ops = &pmic_arb_v2; +devm_iounmap (&ctrl->dev, core); + + pa->ppid_to_chan =devm_kzalloc (&ctrl->dev, +PPID_TO_CHAN_TABLE_SZ ,GFP_KERNEL ); +if (!pa->ppid_to_chan) { + err = -ENOMEM ; + goto err_put_ctrl; + } +/* +* PMIC_ARB_REG_CHNL is a table in HW
Quoted 8 lines
drivers/spmi/spmi-pmic-arb.c static int spmi_pmic_arb_probe(struct platform_device *pdev) · line 834
Quoted 6 lines
mapping channel to ppid.
+ *ppid_to_chanis an in-memory invert of
that table.drivers/spmi/spmi-pmic-arb.c +3
-0+*/ +for (chan =0 ; chan <PMIC_ARB_MAX_CHNL ; ++chan) { + regval = readl_relaxed(pa->rd_base +
rd_baseis not initialized at this point.
correct.
Quoted 28 lines
Quoted 11 lines
+
PMIC_ARB_REG_CHNL(chan));drivers/spmi/spmi-pmic-arb.c +8
-0+if (!regval) +continue ; + + ppid = (regval >>8 ) &0xFFF ; + pa->ppid_to_chan[ppid] = chan; + } + + res = platform_get_resource_byname(pdev,Quoted 5 lines
IORESOURCE_MEM,
+
"obsrvr");
+pa->rd_base= devm_ioremap_resource(&ctrl->dev,
res);and they are compile warnings:
drivers/spmi/spmi-pmic-arb.c: In function ‘pmic_arb_read_cmd’:drivers/spmi/spmi-pmic-arb.c:310:4: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘size_t’ [-Wformat=]PMIC_ARB_MAX_TRANS_BYTES, len); ^drivers/spmi/spmi-pmic-arb.c: In function ‘pmic_arb_write_cmd’:drivers/spmi/spmi-pmic-arb.c:357:4: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘size_t’ [-Wformat=]PMIC_ARB_MAX_TRANS_BYTES, len); ^
Quoted 1 line
Ivan
Sorry about that. I'll fix it.
Thanks,
Gilad
Re: [PATCH V4 2/2] spmi: pmic_arb: add support for hw version 2
In reply to Gilad AvidovHi Gilad,
drivers/spmi/spmi-pmic-arb.c static int qpnpint_irq_domain_map(struct irq_domain *d, · line 713
Quoted 10 lines · Gilad Avidov
drivers/spmi/spmi-pmic-arb.c +9
-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 ); +} + +static u32 pmic_arb_fmt_cmd_v2 (u8 opc,u8 sid,u16 addr,u8 bc) +{ +return (opc <<27 ) | ((addr &0xff ) <<4 ) | (bc &0x7 );
This one is looking suspicious. Address could be only 8 bits? Slave ID is not used?
Is this correct?
Ivan
Re: [PATCH V4 2/2] spmi: pmic_arb: add support for hw version 2
Hi Ivan,
Quoted 19 lines · On Mon, 23 Feb 2015 18:06:10 +0200 "Ivan T. Ivanov" <[email protected]> wrote:
Hi Gilad,
drivers/spmi/spmi-pmic-arb.c static int qpnpint_irq_domain_map(struct irq_domain *d, · line 713
Quoted 10 lines · Gilad Avidov
drivers/spmi/spmi-pmic-arb.c +4
-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); +}drivers/spmi/spmi-pmic-arb.c +4
-0+ +static u32 pmic_arb_fmt_cmd_v2 (u8 opc,u8 sid,u16 addr,u8 bc) +{ +return (opc <<27 ) | ((addr &0xff ) <<4 ) | (bc &0x7 );This one is looking suspicious. Address could be only 8 bits? Slave ID is not used?
Quoted 1 line
Is this correct?
Yes it is correct.
On HW v2 we are writing the command directly to a channel which is specific (already configured) to an SID and the upper 8 bits of the address.
This is how we get the channel:
u32 offset = pmic_arb->ver_ops->offset(pmic_arb, sid, addr);
The lower 8 bits of the address is all that we need to have the entire 20bits of SPMI addressing.
Thanks,
Gilad
Quoted 6 lines
Ivan
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 2/2] spmi: pmic_arb: add support for hw version 2
In reply to Gilad AvidovHi Gilad,
One more comment :-).
On Thu, 2015-02-19 at 15:54 -0700, Gilad Avidov wrote:
<snip>
-static int pmic_arb_wait_for_done(struct spmi_controller *ctrl) +static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
+ void __iomem *base, u8 sid, u16 addr)
{
struct spmi_pmic_arb_dev *dev = spmi_controller_get_drvdata(ctrl);
u32 status = 0;
u32 timeout = PMIC_ARB_TIMEOUT_US;- u32 offset = PMIC_ARB_STATUS(dev->channel);
+ u32 offset = dev->ver_ops->offset(dev, sid, addr) + PMIC_ARB_STATUS;
while (timeout--) {
status = pmic_arb_base_read(dev, offset);I see that downstream driver is using read or write base address based on operation for which we are waiting (read_cmd/write_cmd).
Should this be reflected here?
Ivan.
Re: [PATCH V4 2/2] spmi: pmic_arb: add support for hw version 2
Hi Ivan,
Quoted 22 lines · On Tue, 24 Feb 2015 13:01:08 +0200 "Ivan T. Ivanov" <[email protected]> wrote:
Hi Gilad,
One more comment :-).
On Thu, 2015-02-19 at 15:54 -0700, Gilad Avidov wrote:
<snip>
drivers/spmi/spmi-pmic-arb.c +2
-1-static int pmic_arb_wait_for_done (struct spmi_controller *ctrl) +static int pmic_arb_wait_for_done(struct spmi_controller *ctrl, + void __iomem *base,u8 sid, u16 addr) {
structspmi_pmic_arb_dev*dev =spmi_controller_get_drvdata(ctrl); u32 status = 0; u32 timeout = PMIC_ARB_TIMEOUT_US;(quoted diff) +1
-1- u32 offset = PMIC_ARB_STATUS(dev->channel); + u32 offset = dev->ver_ops->offset(dev, sid, addr) +
PMIC_ARB_STATUS;
Quoted 7 lines
while (timeout--) { status = pmic_arb_base_read(dev, offset);I see that downstream driver is using read or write base address based on operation for which we are waiting (read_cmd/write_cmd).
Should this be reflected here?
Good catch, it should (although it still works fine). I'll fix that.
Gilad.
Quoted 3 lines
Ivan.