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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 23 Jul 2015 14:31:33 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Bjorn Andersson <bjorn.andersson@...ymobile.com>
Cc:	bjorn@...o.se, Andy Gross <agross@...eaurora.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	devicetree@...r.kernel.org, Mark Brown <broonie@...nel.org>,
	linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: 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@...ymobile.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@...ymobile.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/