Add support for the System Power Management Interface (SPMI)

drivers/spmi/spmi-dbgfs.c new file +34 -0

+static char dbgfs_help[] =
+	"SPMI Debug-FS support\n"
+	"\n"
+	"Hierarchy schema:\n"
+	"/sys/kernel/debug/spmi\n"
+	"       /help                -- Static help text\n"
+	"       /spmi-0              -- Directory for SPMI bus 0\n"
+	"       /spmi-0/0-1          -- Directory for SPMI device '0-1'\n"
+	"       /spmi-0/0-1/address  -- Starting register for reads or writes\n"
+	"       /spmi-0/0-1/count    -- Number of registers to read (only used for reads)\n"
+	"       /spmi-0/0-1/data     -- Initiates the SPMI read (formatted output)\n"
+	"       /spmi-0/0-1/data_raw -- Initiates the SPMI raw read or write\n"
+	"       /spmi-n              -- Directory for SPMI bus n\n"
+	"\n"
+	"To perform SPMI read or write transactions, you need to first write the\n"
+	"address of the slave device register to the 'address' file.  For read\n"
+	"transactions, the number of bytes to be read needs to be written to the\n"
+	"'count' file.\n"
+	"\n"
+	"The 'address' file specifies the 20-bit address of a slave device register.\n"
+	"The upper 4 bits 'address[19..16]' specify the slave identifier (SID) for\n"
+	"the slave device.  The lower 16 bits specify the slave register address.\n"
+	"\n"
+	"Reading from the 'data' file will initiate a SPMI read transaction starting\n"
+	"from slave register 'address' for 'count' number of bytes.\n"
+	"\n"
+	"Writing to the 'data' file will initiate a SPMI write transaction starting\n"
+	"from slave register 'address'.  The number of registers written to will\n"
+	"match the number of bytes written to the 'data' file.\n"
+	"\n"
+	"Example: Read 4 bytes starting at register address 0x1234 for SID 2\n"
+	"\n"
+	"echo 0x21234 > address\n"
+	"echo 4 > count\n"

Quoted 34 lines · Josh Cartwright

drivers/spmi/spmi-dbgfs.c new file +34 -0

+static char dbgfs_help[] =
+	"SPMI Debug-FS support\n"
+	"\n"
+	"Hierarchy schema:\n"
+	"/sys/kernel/debug/spmi\n"
+	"       /help                -- Static help text\n"
+	"       /spmi-0              -- Directory for SPMI bus 0\n"
+	"       /spmi-0/0-1          -- Directory for SPMI device '0-1'\n"
+	"       /spmi-0/0-1/address  -- Starting register for reads or writes\n"
+	"       /spmi-0/0-1/count    -- Number of registers to read (only used for reads)\n"
+	"       /spmi-0/0-1/data     -- Initiates the SPMI read (formatted output)\n"
+	"       /spmi-0/0-1/data_raw -- Initiates the SPMI raw read or write\n"
+	"       /spmi-n              -- Directory for SPMI bus n\n"
+	"\n"
+	"To perform SPMI read or write transactions, you need to first write the\n"
+	"address of the slave device register to the 'address' file.  For read\n"
+	"transactions, the number of bytes to be read needs to be written to the\n"
+	"'count' file.\n"
+	"\n"
+	"The 'address' file specifies the 20-bit address of a slave device register.\n"
+	"The upper 4 bits 'address[19..16]' specify the slave identifier (SID) for\n"
+	"the slave device.  The lower 16 bits specify the slave register address.\n"
+	"\n"
+	"Reading from the 'data' file will initiate a SPMI read transaction starting\n"
+	"from slave register 'address' for 'count' number of bytes.\n"
+	"\n"
+	"Writing to the 'data' file will initiate a SPMI write transaction starting\n"
+	"from slave register 'address'.  The number of registers written to will\n"
+	"match the number of bytes written to the 'data' file.\n"
+	"\n"
+	"Example: Read 4 bytes starting at register address 0x1234 for SID 2\n"
+	"\n"
+	"echo 0x21234 > address\n"
+	"echo 4 > count\n"

drivers/spmi/spmi-dbgfs.c line 94

Quoted 10 lines

drivers/spmi/spmi-dbgfs.c new file +10 -0

+	"cat data\n"
+	"\n"
+	"Example: Write 3 bytes starting at register address 0x1008 for SID 1\n"
+	"\n"
+	"echo 0x11008 > address\n"
+	"echo 0x01 0x02 0x03 > data\n"
+	"\n"
+	"Note that the count file is not used for writes.  Since 3 bytes are\n"
+	"written to the 'data' file, then 3 bytes will be written across the\n"
+	"SPMI bus.\n\n";

The help file within the kernel is a nice touch :)

I do know the only rule for debugfs is "There are no rules", but this looks like you are going to have the way to interact to this bus and devices as debugfs, is that correct?

Or is this only for "debugging"? If so, please document it as such.

drivers/spmi/spmi-dbgfs.c line 508

Quoted 5 lines

drivers/spmi/spmi-dbgfs.c new file +5 -0

+void spmi_dfs_controller_add(struct spmi_controller *ctrl)
+{
+	ctrl->dfs_dir = debugfs_create_dir(dev_name(&ctrl->dev),
+					   spmi_debug_root);
+	WARN_ON(!ctrl->dfs_dir);

Why? What is a user going to be able to do with something like this? You do this in a number of places, please provide "valid" error messages instead of just kernel stack tracebacks, failing to show the device for which the error occured (hint, use dev_err()).

Again, never use WARN_ON() as error handling, it's lazy, and wrong.

Quoted 18 lines · Greg Kroah-Hartman

Quoted 4 lines · Josh Cartwright

drivers/spmi/spmi-dbgfs.c new file +4 -0

+	"Example: Read 4 bytes starting at register address 0x1234 for SID 2\n"
+	"\n"
+	"echo 0x21234 > address\n"
+	"echo 4 > count\n"

drivers/spmi/spmi-dbgfs.c line 94

Quoted 10 lines

drivers/spmi/spmi-dbgfs.c new file +10 -0

+	"cat data\n"
+	"\n"
+	"Example: Write 3 bytes starting at register address 0x1008 for SID 1\n"
+	"\n"
+	"echo 0x11008 > address\n"
+	"echo 0x01 0x02 0x03 > data\n"
+	"\n"
+	"Note that the count file is not used for writes.  Since 3 bytes are\n"
+	"written to the 'data' file, then 3 bytes will be written across the\n"
+	"SPMI bus.\n\n";

The help file within the kernel is a nice touch :)

Quoted 1 line

Or is this only for "debugging"? If so, please document it as such.

It's there because it provides a useful interface for debugging of the controller code, and for simple peek/poke of the slave registers without having a full driver in place. Will document this.

Quoted 20 lines · Josh Cartwright

Quoted 18 lines · Greg Kroah-Hartman

Quoted 4 lines · Josh Cartwright

drivers/spmi/spmi-dbgfs.c new file +4 -0

+	"Example: Read 4 bytes starting at register address 0x1234 for SID 2\n"
+	"\n"
+	"echo 0x21234 > address\n"
+	"echo 4 > count\n"

drivers/spmi/spmi-dbgfs.c line 94

Quoted 10 lines

drivers/spmi/spmi-dbgfs.c new file +10 -0

+	"cat data\n"
+	"\n"
+	"Example: Write 3 bytes starting at register address 0x1008 for SID 1\n"
+	"\n"
+	"echo 0x11008 > address\n"
+	"echo 0x01 0x02 0x03 > data\n"
+	"\n"
+	"Note that the count file is not used for writes.  Since 3 bytes are\n"
+	"written to the 'data' file, then 3 bytes will be written across the\n"
+	"SPMI bus.\n\n";

The help file within the kernel is a nice touch :)

Quoted 5 lines

Quoted 1 line

Or is this only for "debugging"? If so, please document it as such.

It's there because it provides a useful interface for debugging of the controller code, and for simple peek/poke of the slave registers without having a full driver in place. Will document this.

This looks awfully like a version of the debugfs interfaces that regmap provides, and indeed the entire bus sounds like something that could idiomatically be supported via regmap. Have you considered doing that? This would give access to standard tracepoints as well, plus the cache infrastructure.

Signed-off-by Josh Cartwright <[email protected]>

---
I'm introducing this as an RFC, because there are set of assumptions made in this binding spec, that currently hold true for the supported controller/addressing scheme for the Snapdragon 800 series, but don't necessarily hold true for SPMI in general.

1. No use of Group Slave Identifiers (GSIDs)
(SPMI allows for a slave to belong to zero or more groups specified by GSID, however this feature isn't currently implemented)

2. No specification of Master Identifier (MID)
(A "system integrator" allocates to each master a 2-bit MID, this currently isn't being specified, as it isn't needed by software for the PMIC Arb; not sure if this is of use to other SPMI controllers)

3. Single SPMI master per controller

Effectively, only a subset of possible SPMI configurations are specified in this document.

So, if it's considered necessary to provide a generic SPMI binding specification, is it acceptable to only define a subset at this time, expanding only when necessary, or shall I expand the definition to at least address 1 & 2, even though they are of no use in the current implementation?

Quoted 18 lines · Josh Cartwright

Signed-off-by Josh Cartwright <[email protected]>

---
I'm introducing this as an RFC, because there are set of assumptions made in this binding spec, that currently hold true for the supported controller/addressing scheme for the Snapdragon 800 series, but don't necessarily hold true for SPMI in general.

1. No use of Group Slave Identifiers (GSIDs)
(SPMI allows for a slave to belong to zero or more groups specified by GSID, however this feature isn't currently implemented)

2. No specification of Master Identifier (MID)
(A "system integrator" allocates to each master a 2-bit MID, this currently isn't being specified, as it isn't needed by software for the PMIC Arb; not sure if this is of use to other SPMI controllers)

3. Single SPMI master per controller

Quoted 8 lines

Effectively, only a subset of possible SPMI configurations are specified in this document.

So, if it's considered necessary to provide a generic SPMI binding specification, is it acceptable to only define a subset at this time, expanding only when necessary, or shall I expand the definition to at least address 1 & 2, even though they are of no use in the current implementation?

It's best to define the whole thing from the start if possible. It's easier to ensure the whole binding is consistent, and nothing has been left out.

However, it's probably OK to define a subset binding initially and then expand it later, as long as some thought it put into how it can be expanded in a way that is 100% compatible: old DTs will still operate with new kernels and perhaps even new DTs will still operate with old kernels.

That said, if the thought is put in to ensure that's possible, it's probably just as easy to define the whole binding from the start.

If we want to ensure for the generic bindings that we are fulling characterizing/describing the SPMI bus, then we'll additionally need to tackle an additional identified assumption:

4. One master per SPMI bus. (The SPMI spec allows for up to 4 masters)

On the Snapdragon 800 series, there exists only one software-controlled master, but it is conceivably possible to have a setup with two software-controlled masters on the same SPMI bus.

This necessarily means that the description of the slaves and the masters will need to be decoupled; I'm imagining a generic binding supporting multiple masters would look something like this:

	master0: master at 0 {
		compatible = "...";
		#spmi-master-cells = <0>;
		spmi-mid = <0>;

		...
	};

	master2: master at 2 {
		compatible = "...";
		#spmi-master-cells = <0>;
		spmi-mid = <2>;

		...
	};

	spmi_bus {
		compatible = "...";

		spmi-masters = <&master0 &master2>;

		foo at 0 {
			compatible = "...";
			reg = <0 ...>;
		};

		foo at 8 {
			compatible = "...";
			reg = <8 ...>;
		};
	};

(This will also necessitate a change in the underlying SPMI driver model, in the current implementation, a SPMI master 'owns' a particular device. This is not a valid assumption to make.)

Would this property-containing-phandle-vector be considered the canonical way of representing nodes with multiple parents in the device tree?

On 08/27/2013 11:01 AM, Josh Cartwright wrote:
...

Quoted 7 lines

If we want to ensure for the generic bindings that we are fulling characterizing/describing the SPMI bus, then we'll additionally need to tackle an additional identified assumption:

4. One master per SPMI bus. (The SPMI spec allows for up to 4 masters)

Quoted 7 lines

On the Snapdragon 800 series, there exists only one software-controlled master, but it is conceivably possible to have a setup with two software-controlled masters on the same SPMI bus.

This necessarily means that the description of the slaves and the masters will need to be decoupled; I'm imagining a generic binding supporting multiple masters would look something like this:

Is there a need to represent the other masters in the DT? Sure they're there in HW, but if there's no specific way for the CPU-to-which-the-DT-applies to actually interact with those other masters (except perhaps by experiencing some arbitration delays) then presumably there's no need to represent the other masters in DT?

Quoted 32 lines

	master0: master at 0 {
		compatible = "...";
		#spmi-master-cells = <0>;
		spmi-mid = <0>;

		...
	};

	master2: master at 2 {
		compatible = "...";
		#spmi-master-cells = <0>;
		spmi-mid = <2>;

		...
	};

	spmi_bus {
		compatible = "...";

		spmi-masters = <&master0 &master2>;

		foo at 0 {
			compatible = "...";
			reg = <0 ...>;
		};

		foo at 8 {
			compatible = "...";
			reg = <8 ...>;
		};
	};

Quoted 7 lines

(This will also necessitate a change in the underlying SPMI driver model, in the current implementation, a SPMI master 'owns' a particular device. This is not a valid assumption to make.)

Would this property-containing-phandle-vector be considered the canonical way of representing nodes with multiple parents in the device tree?

I don't think I've seen anything like this before, although that in-and-of-itself doesn't make it wrong.

Another approach might be to encode master-vs-slave into a cell in the reg property? Something like:

cell 0 - address type (0: master, 1: unique ID, 2: group ID, ...) cell 1 - address value

I haven't thought much about that; perhaps there are disadvantages doing that.

Quoted 17 lines · Stephen Warren

On 08/27/2013 11:01 AM, Josh Cartwright wrote:
...

Quoted 7 lines

If we want to ensure for the generic bindings that we are fulling characterizing/describing the SPMI bus, then we'll additionally need to tackle an additional identified assumption:

4. One master per SPMI bus. (The SPMI spec allows for up to 4 masters)

Quoted 7 lines

On the Snapdragon 800 series, there exists only one software-controlled master, but it is conceivably possible to have a setup with two software-controlled masters on the same SPMI bus.

This necessarily means that the description of the slaves and the masters will need to be decoupled; I'm imagining a generic binding supporting multiple masters would look something like this:

Quoted 5 lines

Is there a need to represent the other masters in the DT? Sure they're there in HW, but if there's no specific way for the CPU-to-which-the-DT-applies to actually interact with those other masters (except perhaps by experiencing some arbitration delays) then presumably there's no need to represent the other masters in DT?

My example is contrived, but there is nothing in the SPMI spec preventing two masters from being controllable by the same CPU-to-which-the-DT-applies, sharing the same underlying bus.

I would also expect this configuration to be uncommon, I'm checking with some folks with more SPMI experience to make sure they agree.

Interestingly, i2c as far as I can tell has also made the same assumption. There doesn't appear to be any way to express a multi-master i2c setup where both masters are controlled by the same OS.

On 08/27/2013 11:01 AM, Josh Cartwright wrote:
...
cell 0 - address type (0: master, 1: unique ID, 2: group ID, ...)

cell 1 - address value

Quoted 3 lines · Stephen Warren

On 08/27/2013 11:01 AM, Josh Cartwright wrote:
...
cell 0 - address type (0: master, 1: unique ID, 2: group ID, ...)

I think it would make sense to have the master id as a property of the bus,

as you could consider this to indicate different buses and then usid, gsid and base being part of the reg.

Quoted 1 line

cell 1 - address value

I did hack up Josh patchset to read a reg touple of <usid, base> instead of just usid. I stored the second value in the spmi_device struct for easy access, but maybe it should be done like on codeaurora; in a resource?
I believe this looks nice, but as I haven't read the mipi spec I wonder, will there be a case where you don't have an offset/base? Should it just be made optional?

Can we make the address <usid, [base]> and have the code populate a resource based on a reg-names property? That way it would be possible to extend it to support gsid in case we want to (would require reg-names though).

With the hack to Josh's patchset I quickly ported qpnp-revision and qpnp-vibrator, and it seems to work quite nicely.

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

+	qcom,spmi at fc4c0000 {