Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1190933 > unrolled thread

Re: [PATCH v2 05/11] mfd: devicetree: bindings: Add Qualcomm SMD based RPM DT binding

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 05/11] mfd: devicetree: bindings: Add Qualcomm SMD  based RPM DT binding Lee Jones <lee.jones@linaro.org> - 2015-07-23 15:40 +0200
    Re: [PATCH v2 05/11] mfd: devicetree: bindings: Add Qualcomm SMD  based RPM DT binding Mark Brown <broonie@kernel.org> - 2015-07-23 19:20 +0200
      Re: [PATCH v2 05/11] mfd: devicetree: bindings: Add Qualcomm SMD  based RPM DT binding Lee Jones <lee.jones@linaro.org> - 2015-07-24 12:00 +0200
        Re: [PATCH v2 05/11] mfd: devicetree: bindings: Add Qualcomm SMD  based RPM DT binding Mark Brown <broonie@kernel.org> - 2015-07-24 12:30 +0200
          Re: [PATCH v2 05/11] mfd: devicetree: bindings: Add Qualcomm SMD  based RPM DT binding Mark Brown <broonie@kernel.org> - 2015-07-27 12:00 +0200
            Re: [PATCH v2 05/11] mfd: devicetree: bindings: Add Qualcomm SMD  based RPM DT binding Lee Jones <lee.jones@linaro.org> - 2015-07-27 13:00 +0200

#1190933 — Re: [PATCH v2 05/11] mfd: devicetree: bindings: Add Qualcomm SMD based RPM DT binding

On Mon, 13 Jul 2015, Bjorn Andersson wrote:

> On Tue 07 Jul 05:16 PDT 2015, Lee Jones wrote:
> 
> > FAO Mark and DT chaps,
> > 
> > > From: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > > 
> > > Add binding documentation for the Qualcomm Resource Power Manager (RPM)
> > > using shared memory (Qualcomm SMD) as transport mechanism. This is found
> > > in 8974 and newer based devices.
> > > 
> > > The binding currently describes the rpm itself and the regulator
> > > subnodes.
> > > 
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> > > ---
> > >  .../devicetree/bindings/mfd/qcom-rpm-smd.txt       | 117 +++++++++++++++++++++
> > >  include/dt-bindings/mfd/qcom-smd-rpm.h             |  28 +++++
> > >  2 files changed, 145 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
> > >  create mode 100644 include/dt-bindings/mfd/qcom-smd-rpm.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt b/Documentation/devicetree/bindings/mfd/qcom-rpm-smd.txt
> 
> [..]
> 
> > > +- qcom,smd-channels:
> > > +	Usage: required
> > > +	Value type: <stringlist>
> > > +	Definition: Shared Memory channel used for communication with the RPM
> > 
> > This is going to require a DT Ack.
> > 
> > Also, I don't see it being used anywhere.
> 
> It's a common property of all smd devices, defining the smd channel this
> driver should bind to.

Well it's not in the kernel and I can't find the patch that uses it,
so my points still stand.

> > > += EXAMPLE
> > > +
> > > +	smd {
> > > +		compatible = "qcom,smd";
> > 
> > Is an SMD (Shared Memory Device?) real hardware?
> > 
> 
> SMD is a mechanism for using shared memory for point-to-point
> communication channels with remote processors in all Qualcomm platforms.
> 
> So it's not hardware, it's the control mechanism for communicating with
> real hardware.

Then you can't have a node for it.  Virtual nodes which do not
represent real h/w are not allowed in Device Tree.

> > > +		rpm {
> > > +			interrupts = <0 168 1>;
> > > +			qcom,ipc = <&apcs 8 0>;
> > > +			qcom,smd-edge = <15>;
> > 
> > The child node won't probe without a compatible string.  Shouldn't
> > "qcom,rpm-msm8974" be in here instead?
> > 
> 
> These sub-nodes represents a logical grouping of the various channels
> that exist to this remote processor. For the rpm there is only the
> "rpm_requests" channel - used for sending regulator & clock requests.

Again, if it's not real h/w and don't have a proper driver, there
should be no reason for this node to exist.

> > > +			rpm_requests {
> > 
> > This node appears to be undocumented.
> 
> This is the actual rpm device node, the smd & rpm nodes above are
> included for completeness of the example.
> 
> They should perhaps be dropped to make this clearer.
> 
> > Does it represent real h/w?
> > 
> 
> The other end of this smd channel is a micro controller that handles
> regulator and clock requests for the platform - so this is hardware.
> 
> This is equivalent to the qcom_rpm driver, but instead of a hardware
> like register window this uses the same packet based messaging mechanism
> that's used for other remote peripherals in the Qualcomm platform.

This needs a good review by the DT guys.

> > > +				compatible = "qcom,rpm-msm8974";
> > > +				qcom,smd-channels = "rpm_requests";
> > > +
> > > +				pm8941-regulators {
> > > +					compatible = "qcom,rpm-pm8941-regulators";
> > > +					vdd_l13_l20_l23_l24-supply = <&pm8941_boost>;
> > 
> > I'd like Mark to glance at this.
> > 
> 
> Right.
> 
> > > +					pm8941_s3: s3 {
> > > +						regulator-min-microvolt = <1800000>;
> > > +						regulator-max-microvolt = <1800000>;
> > 
> > Aren't these fixed regulators?
> > 
> 
> In this system configuration most of the regulators have fixed values,
> but the regulators (hw) are not fixed.

I'm not sure that's how it works.  I believe 'max' and 'min' should
describe the upper and lower constraints of the regulator.  The actual
value it runs it is selected elsewhere.

We still need Mark to look at this.

> > > +					};
> > > +
> > > +					pm8941_boost: s4 {
> > > +						regulator-min-microvolt = <5000000>;
> > > +						regulator-max-microvolt = <5000000>;
> > > +					};
> > > +
> > > +					pm8941_l20: l20 {
> > > +						regulator-min-microvolt = <2950000>;
> > > +						regulator-max-microvolt = <2950000>;
> > > +					};
> > > +				};
> > > +			};
> > > +		};
> > > +	};
> > > +
> 
> Thanks,
> Bjorn

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1191153

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 23, 2015 at 09:41:28AM -0700, Bjorn Andersson wrote:

> > We still need Mark to look at this.

> Mark, would you mind giving us a statement on the regulator subnode of
> this binding?

I have no idea what's going on here, sorry.  I've not been reading this
thread.

[toc] | [prev] | [next] | [standalone]

#1191651

On Thu, 23 Jul 2015, Mark Brown wrote:

> On Thu, Jul 23, 2015 at 09:41:28AM -0700, Bjorn Andersson wrote:
> 
> > > We still need Mark to look at this.
> 
> > Mark, would you mind giving us a statement on the regulator subnode of
> > this binding?
> 
> I have no idea what's going on here, sorry.  I've not been reading this
> thread.

All of the information you need is in the email you replied to.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191687

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jul 24, 2015 at 10:58:47AM +0100, Lee Jones wrote:
> On Thu, 23 Jul 2015, Mark Brown wrote:
> > On Thu, Jul 23, 2015 at 09:41:28AM -0700, Bjorn Andersson wrote:

> > > > We still need Mark to look at this.

> > > Mark, would you mind giving us a statement on the regulator subnode of
> > > this binding?

> > I have no idea what's going on here, sorry.  I've not been reading this
> > thread.

> All of the information you need is in the email you replied to.

The mail appears to have an edited down section of what looks like an
example rather than the actual binding.  I haven't seen the binding so I
can't really comment on it.

[toc] | [prev] | [next] | [standalone]

#1192910

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jul 27, 2015 at 08:29:18AM +0100, Lee Jones wrote:
> On Fri, 24 Jul 2015, Mark Brown wrote:
> > On Fri, Jul 24, 2015 at 11:24:34AM +0100, Mark Brown wrote:

> From here:

> > > > > > +                pm8941-regulators {
> > > > > > +                        compatible = "qcom,rpm-pm8941-regulators";
> > > > > > +                        vdd_l13_l20_l23_l24-supply = <&pm8941_boost>;

> > > > > I'd like Mark to glance at this.

> Mark: Is this new property okay?

As far as I can see that looks like a standard supply property, assuming
the supply is actually called that why would it be an issue?

> > The specified range of the regulator is 1.75-1.85V and this is handled
> > by the implementation, however the board designers have stated that it
> > is only allowed to be configured to 1.8V.

> > So DT is used to narrow the capabilities of the individual component to
> > something that's suitable for this particular system.

> > > We still need Mark to look at this.

> Is it okay for the regulator-{min,max}-microvolt to be artificially
> restricted to the required value, despite knowing that the regulator
> is capable of supply {more,less} voltage?

Yes, that's the entire purpose of those properties - to set the limits
the board designers have which will typically be more restrictive than
those that the regulator itself is capable of imposing.

[toc] | [prev] | [next] | [standalone]

#1192962

On Mon, 27 Jul 2015, Mark Brown wrote:

> On Mon, Jul 27, 2015 at 08:29:18AM +0100, Lee Jones wrote:
> > On Fri, 24 Jul 2015, Mark Brown wrote:
> > > On Fri, Jul 24, 2015 at 11:24:34AM +0100, Mark Brown wrote:
> 
> > From here:
> 
> > > > > > > +                pm8941-regulators {
> > > > > > > +                        compatible = "qcom,rpm-pm8941-regulators";
> > > > > > > +                        vdd_l13_l20_l23_l24-supply = <&pm8941_boost>;
> 
> > > > > > I'd like Mark to glance at this.
> 
> > Mark: Is this new property okay?
> 
> As far as I can see that looks like a standard supply property, assuming
> the supply is actually called that why would it be an issue?
> 
> > > The specified range of the regulator is 1.75-1.85V and this is handled
> > > by the implementation, however the board designers have stated that it
> > > is only allowed to be configured to 1.8V.
> 
> > > So DT is used to narrow the capabilities of the individual component to
> > > something that's suitable for this particular system.
> 
> > > > We still need Mark to look at this.
> 
> > Is it okay for the regulator-{min,max}-microvolt to be artificially
> > restricted to the required value, despite knowing that the regulator
> > is capable of supply {more,less} voltage?
> 
> Yes, that's the entire purpose of those properties - to set the limits
> the board designers have which will typically be more restrictive than
> those that the regulator itself is capable of imposing.

All fine then.

Please re-submit with the changes we discussed.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web