add support for pmic_arb v2 and correct framework

PATCH Superseded Gilad Avidov 3 participants 12 messages Feb 9, 2015 – Feb 19, 2015

Subsystems (3)

Device tree bindings Arm Qualcomm mailing list SPMI

lore

Share & tools

lore

Gilad Avidov

author

2015-02-09 22:58 UTC #0 lore raw

v3 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.

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                       | 312 +++++++++++++++++----
 drivers/spmi/spmi.c                                |   9 +-
 3 files changed, 263 insertions(+), 64 deletions(-)

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Gilad Avidov

author

2015-02-09 22:51 UTC #1 lore raw

PATCH v3 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.

This is a bug since spmi master controllers, such as spmi-pmic-arb, which have no support for wakeup command return an error on that command and thus fail before reaching a slave driver probe.

Cc [email protected]

Reviewed-by Sagar Dharia <[email protected]>

Signed-off-by Gilad Avidov <[email protected]>

---
 drivers/spmi/spmi.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

drivers/spmi/spmi.c +2 -7

index 1d92f51..9493843 100644
@@ -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

Josh Cartwright

Acked-by on v3 1/2

2015-02-18 15:45 UTC #2 lore raw

Re: [PATCH V3 1/2] spmi: remove wakeup command before slave probe

Quoted 8 lines · Gilad Avidov

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.
This is a bug since spmi master controllers, such as spmi-pmic-arb, which have no support for wakeup command return an error on that command and thus fail before reaching a slave driver probe.

If masters are required by the spec to support all commands as Stephen mentions, then I'd argue this is not a bug in the core code at all, but in the spmi-pmic-arb driver. But, unfortunately, having lost access to the spec, I'll defer.

Regardless, I think this is useful as an optimization, just with dubious justification.

Therefore,

Acked-by Josh Cartwright <[email protected]>

Josh

Stephen Boyd

Reviewed-by

2015-02-18 19:17 UTC #3 lore raw

Re: [PATCH V3 1/2] spmi: remove wakeup command before slave probe

Quoted 14 lines · Josh Cartwright

Quoted 8 lines · Gilad Avidov
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.
This is a bug since spmi master controllers, such as spmi-pmic-arb, which have no support for wakeup command return an error on that command and thus fail before reaching a slave driver probe.
If masters are required by the spec to support all commands as Stephen mentions, then I'd argue this is not a bug in the core code at all, but in the spmi-pmic-arb driver. But, unfortunately, having lost access to the spec, I'll defer.

Quoted 8 lines

Regardless, I think this is useful as an optimization, just with dubious justification.
Therefore,
Acked-by Josh Cartwright <[email protected]>

Agreed, it's mostly an optimization and aligns the code more with the spec. How about we drop the "This is a bug" part?

With that done,

Reviewed-by Stephen Boyd <[email protected]>

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Gilad Avidov

author

2015-02-19 19:23 UTC #4 lore raw

Re: [PATCH V3 1/2] spmi: remove wakeup command before slave probe

Quoted 19 lines · On Wed, 18 Feb 2015 11:17:13 -0800 Stephen Boyd <[email protected]> wrote:

Quoted 18 lines · Josh Cartwright
Quoted 9 lines · Gilad Avidov
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.
This is a bug since spmi master controllers, such as spmi-pmic-arb, which have no support for wakeup command return an error on that command and thus fail before reaching a slave driver probe.
If masters are required by the spec to support all commands as Stephen mentions, then I'd argue this is not a bug in the core code at all, but in the spmi-pmic-arb driver. But, unfortunately, having lost access to the spec, I'll defer.
Regardless, I think this is useful as an optimization, just with dubious justification.

Quoted 9 lines

Quoted 5 lines
Therefore,
Acked-by Josh Cartwright <[email protected]>
Agreed, it's mostly an optimization and aligns the code more with the spec. How about we drop the "This is a bug" part?

As Stephen suggested, I'll drop the bug part.

Thank you for the reviews,
Gilad

Quoted 4 lines

With that done,
Reviewed-by Stephen Boyd <[email protected]>

Gilad Avidov

author

2015-02-09 22:51 UTC #5 lore raw

PATCH v3 2/2 spmi: pmic_arb: add support for hw version 2

In reply to 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]>

---
 .../bindings/spmi/qcom,spmi-pmic-arb.txt           |   6 +-
 drivers/spmi/spmi-pmic-arb.c                       | 312 +++++++++++++++++----
 2 files changed, 261 insertions(+), 57 deletions(-)

Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt +5 -1

index 715d099..e16b9b5 100644
@@ -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 +256 ~~-56~~

502 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

Josh Cartwright

2015-02-18 15:34 UTC #6 lore raw

Re: [PATCH V3 2/2] spmi: pmic_arb: add support for hw version 2

Hey Gilad-

Quoted 10 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]>

[..]

Quoted 1 line

+++ b/drivers/spmi/spmi-pmic-arb.c

[..]

drivers/spmi/spmi-pmic-arb.c static int spmi_pmic_arb_probe(struct platform_device *pdev) · line 798

Quoted 8 lines

drivers/spmi/spmi-pmic-arb.c +1 -3

@@ -645,12 +795,65 @@ 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);
+	pa->rd_base = devm_ioremap_resource(&ctrl->dev, res);

This seems like an awkward way to do this, especially if you end up remapping it...

Quoted 34 lines

drivers/spmi/spmi-pmic-arb.c +31 -0

+	if (IS_ERR(pa->rd_base)) {
+		err = PTR_ERR(pa->rd_base);
 		goto err_put_ctrl;
 	}
 
+	hw_ver = readl_relaxed(pa->rd_base + 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 = pa->rd_base;
+	} else {
+		u8  chan;
+		u16 ppid;
+		u32 regval;
+
+		pa->ver_ops = &pmic_arb_v2;
+
+		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 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 +
+					       PMIC_ARB_REG_CHNL(chan));

drivers/spmi/spmi-pmic-arb.c static int spmi_pmic_arb_probe(struct platform_device *pdev) · line 842

Quoted 10 lines

drivers/spmi/spmi-pmic-arb.c +10 -0

+			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);

...here. Especially because now you have some loose mapping hanging around for the lifetime of the device. I'd suggest splitting the v1 and v2 probe routines out into their own functions.

Josh

Gilad Avidov

author

2015-02-19 17:29 UTC #7 lore raw

Re: [PATCH V3 2/2] spmi: pmic_arb: add support for hw version 2

Hi Josh,

Thank you for the review.

Quoted 19 lines · On Wed, 18 Feb 2015 09:34:46 -0600 Josh Cartwright <[email protected]> wrote:

Hey Gilad-
Quoted 10 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]>
[..]
Quoted 1 line
+++ b/drivers/spmi/spmi-pmic-arb.c
[..]
Quoted 3 lines
(quoted diff) +0 -0
@@ -645,12 +795,65 @@ static int spmi_pmic_arb_probe(struct
 platform_device *pdev) pa->spmic = ctrl;
 

Quoted 9 lines

drivers/spmi/spmi-pmic-arb.c static int spmi_pmic_arb_probe(struct platform_device *pdev) · line 798
Quoted 6 lines
drivers/spmi/spmi-pmic-arb.c +1 -3
 	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);
+	pa->rd_base = devm_ioremap_resource(&ctrl->dev, res);
This seems like an awkward way to do this, especially if you end up remapping it...

Agree. I think the following will fix it:
1. Assign the core reg base to a local variable.
2. For v1: assign that local to rd/wr base.
3. For v2: devm_unmap() that core.

Quoted 48 lines

drivers/spmi/spmi-pmic-arb.c +26 -0

+	if (IS_ERR(pa->rd_base)) {
+		err = PTR_ERR(pa->rd_base);
 		goto err_put_ctrl;
 	}
 
+	hw_ver = readl_relaxed(pa->rd_base + 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 = pa->rd_base;
+	} else {
+		u8  chan;
+		u16 ppid;
+		u32 regval;
+
+		pa->ver_ops = &pmic_arb_v2;
+
+		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 mapping

channel to ppid.

(quoted diff) +1 -0

+		 * ppid_to_chan is an in-memory invert of that

table.

drivers/spmi/spmi-pmic-arb.c +4 -0

+		 */
+		for (chan = 0; chan < PMIC_ARB_MAX_CHNL; ++chan) {
+			regval = readl_relaxed(pa->rd_base +
+

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 9 lines

Quoted 4 lines
IORESOURCE_MEM,
(quoted diff) +2 -0
+						   "obsrvr");
+		pa->rd_base = devm_ioremap_resource(&ctrl->dev,
res);
...here. Especially because now you have some loose mapping hanging around for the lifetime of the device. I'd suggest splitting the v1 and v2 probe routines out into their own functions.

75% of the probe code is common. By splitting to two probes that code will be duplicated. Of course we can factor out the commonality, but then will end up with a similar situation as here.

Splitting the probes will not rid us of needing the core reg space for either version, since we need the core to access the version register.

Quoted 5 lines

Josh
--
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

Thank you,
Gilad

Stephen Boyd

2015-02-09 23:08 UTC #8 lore raw

In reply to Gilad Avidov

Quoted 6 lines · Gilad Avidov

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.

The spec also seems to stipulate that SPMI masters shall support all SPMI command sequences.

Quoted 9 lines

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                       | 312 +++++++++++++++++----
 drivers/spmi/spmi.c                                |   9 +-
 3 files changed, 263 insertions(+), 64 deletions(-)

Can you please include a patchset changelog here? What changed between v2 and v3?

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Gilad Avidov

author

2015-02-11 18:05 UTC #9 lore raw

Quoted 10 lines · On Mon, 09 Feb 2015 15:08:58 -0800 Stephen Boyd <[email protected]> wrote:

Quoted 6 lines · Gilad Avidov
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.
The spec also seems to stipulate that SPMI masters shall support all SPMI command sequences.

Regardless of the master's support, a wakeup call before prob is not required.

Quoted 12 lines

Quoted 11 lines
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                       | 312
+++++++++++++++++----
drivers/spmi/spmi.c | 9 +- 3 files
changed, 263 insertions(+), 64 deletions(-)

Quoted 3 lines

Can you please include a patchset changelog here? What changed between v2 and v3?

I will.

Thanks,
Gilad

Stephen Boyd

2015-02-12 00:53 UTC #10 lore raw

Quoted 11 lines · Gilad Avidov

Quoted 8 lines · On Mon, 09 Feb 2015 15:08:58 -0800 Stephen Boyd <[email protected]> wrote:
Quoted 6 lines · Gilad Avidov
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.

Quoted 5 lines

Quoted 2 lines
The spec also seems to stipulate that SPMI masters shall support all SPMI command sequences.
Regardless of the master's support, a wakeup call before prob is not required.

Yes that seems to match what the spec says.

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Gilad Avidov

author

2015-02-13 19:50 UTC #11 lore raw

In reply to Stephen Boyd

Hi Stephen,

Quoted 22 lines · On Mon, 09 Feb 2015 15:08:58 -0800 Stephen Boyd <[email protected]> wrote:

Quoted 6 lines · Gilad Avidov
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.
The spec also seems to stipulate that SPMI masters shall support all SPMI command sequences.
Quoted 10 lines
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                       | 312
+++++++++++++++++----
drivers/spmi/spmi.c | 9 +- 3 files
changed, 263 insertions(+), 64 deletions(-)

Quoted 3 lines

Can you please include a patchset changelog here? What changed between v2 and v3?

Changelog from v2 to v3:

spmi-pmic-arb.c
1. Correct multiline comment style of the copyright at the top.
2. Remove redundant parenthesis around #define values and around m,n parameters of the version specific callbacks. 3. Remove useless comment.
4. pr_debug -> dev_dbg
5. corrected few indentations.
6. phys_addr_t offset -> u32 offset

spmi.c
1. Correct multiline comment style of the copyright at the top.

Thanks,
Gilad