Re: [PATCH RFC v2 3/5] spmi: add generic SPMI controller binding documentation

Re: [PATCH RFC v2 3/5] spmi: add generic SPMI controller binding documentation

[Josh Cartwright]

Hey Stephen-

Thanks for the comments.

On Fri, Aug 23, 2013 at 03:58:36PM -0600, Stephen Warren wrote:
> On 08/22/2013 01:59 PM, Josh Cartwright wrote:
> > Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> > ---
> > 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?
> 
> 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.

That makes sense.  I think I'll go down this route for v3 of this
patchset:

For #1 above, extend the 'reg' property of a slave node to include the
group slave ID's in which the slave is a member.  The first 'reg' entry
will remain the slave's Unique Slave Identifier (USID) as before.

For #2, add additional required 'spmi-mid' property in the
controller/master node that describes the 2-bit Master Identifier (MID).

For #3, rename the SPMI API's s/controller/master/.  The current
controller/master terminology difference is confusing and unnecessary.

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

That all makes sense.

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@0 {
		compatible = "...";
		#spmi-master-cells = <0>;
		spmi-mid = <0>;

		...
	};

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

		...
	};

	spmi_bus {
		compatible = "...";

		spmi-masters = <&master0 &master2>;

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

		foo@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?

Thanks,
  Josh

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

Re: [PATCH RFC v2 3/5] spmi: add generic SPMI controller binding documentation

[Stephen Warren]

On 08/27/2013 11:01 AM, Josh Cartwright wrote:
...
> 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:

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?

> 	master0: master@0 {
> 		compatible = "...";
> 		#spmi-master-cells = <0>;
> 		spmi-mid = <0>;
> 
> 		...
> 	};
> 
> 	master2: master@2 {
> 		compatible = "...";
> 		#spmi-master-cells = <0>;
> 		spmi-mid = <2>;
> 
> 		...
> 	};
> 
> 	spmi_bus {
> 		compatible = "...";
> 
> 		spmi-masters = <&master0 &master2>;
> 
> 		foo@0 {
> 			compatible = "...";
> 			reg = <0 ...>;
> 		};
> 
> 		foo@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?

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.

Re: [PATCH RFC v2 3/5] spmi: add generic SPMI controller binding documentation

[Josh Cartwright]

On Tue, Aug 27, 2013 at 03:55:19PM -0600, Stephen Warren wrote:
> On 08/27/2013 11:01 AM, Josh Cartwright wrote:
> ...
> > 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:
>
> 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.

> > 	master0: master@0 {
> > 		compatible = "...";
> > 		#spmi-master-cells = <0>;
> > 		spmi-mid = <0>;
> > 
> > 		...
> > 	};
> > 
> > 	master2: master@2 {
> > 		compatible = "...";
> > 		#spmi-master-cells = <0>;
> > 		spmi-mid = <2>;
> > 
> > 		...
> > 	};
> > 
> > 	spmi_bus {
> > 		compatible = "...";
> > 
> > 		spmi-masters = <&master0 &master2>;
> > 
> > 		foo@0 {
> > 			compatible = "...";
> > 			reg = <0 ...>;
> > 		};
> > 
> > 		foo@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?
> 
> 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.

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

Re: [PATCH RFC v2 3/5] spmi: add generic SPMI controller binding documentation

[Stephen Warren]

On 08/28/2013 12:00 PM, Josh Cartwright wrote:
> On Tue, Aug 27, 2013 at 03:55:19PM -0600, Stephen Warren wrote:
>> On 08/27/2013 11:01 AM, Josh Cartwright wrote:
>> ...
>>> 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:
>>
>> 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.

That's true.

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

Yes, I think it's a fair assumption that we don't need to represent
that; I immediately thought about the I2C counter-example after reading
your first paragraph.

Re: [PATCH RFC v2 3/5] spmi: add generic SPMI controller binding documentation

[Bjorn Andersson]

On Tue, Aug 27, 2013 at 2:55 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 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.

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

Regards,
Bjorn

Re: [PATCH RFC v2 3/5] spmi: add generic SPMI controller binding documentation

[Josh Cartwright]

Hey Bjorn-

On Sat, Oct 05, 2013 at 11:11:36PM -0700, Bjorn Andersson wrote:
> On Tue, Aug 27, 2013 at 2:55 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> > 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,

Except that SPMI supports bus configurations with multiple masters.
Unless by 'bus' you meant 'bus controller' here?

> as you could consider this to indicate different buses and then
> usid, gsid and base being part of the reg.
>
> > 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?

The SPMI spec says nothing about partitioning up the slave address to
support multiple functions.  AFAICT, this is a Qualcomm-created
construct (QPNP) for the 8x41 PMICs.  It's difficult to tell at this
point whether or not other vendors might implement a similarly
partitioned scheme.

I suspect the intent is that implementations make use of logical slave
ID for each function in a multi-function device.

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

It is certainly possible, and, as you've seen, is how the current
codeaurora.org tree implements SPMI.  But, I'm actually actively trying
to avoid doing so, as it conflates Qualcomm-implementation details and
what is actually in the spec (and not just the address space
partitioning, but also the of_spmi.c[2] parsing must know about
interrupts, which are _also_ completely outside the SPMI spec).

Instead what I hope to do for v3 is either:
  A) Make QPNP its own bus type (for which I have a prototyped
     implementation).  A PMIC driver sits on the SPMI bus and registers
     itself as a QPNP controller.  QPNP controllers have very
     simple 8-bit register read/write operations used by QPNP devices.
  B) Effectively the same as A, but gets rid of a special QPNP bus type
     and uses mfd/platform devices, similar to other in-tree PMIC drivers
     (currently working on prototyping this approach)

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

Great! Thanks for testing.

  Josh

1: https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/spmi
2: https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.10/tree/drivers/of/of_spmi.c

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html