Re: [PATCH 1/3] dt-bindings: iio: adc: Add Xilinx AMS binding documentation
On Thu, 6 Sep 2018 13:27:34 + Manish Narani wrote: > Hi Jonathan, > > Thanks for the review! > > > -Original Message- > > From: Jonathan Cameron [mailto:ji...@kernel.org] > > Sent: Sunday, September 2, 2018 11:45 PM > > To: Manish Narani > > Cc: knaac...@gmx.de; l...@metafoo.de; pme...@pmeerw.net; > > robh...@kernel.org; mark.rutl...@arm.com; Michal Simek > > ; leoyang...@nxp.com; sudeep.ho...@arm.com; > > amit.kuche...@linaro.org; broo...@kernel.org; arnaud.pouliq...@st.com; > > ge...@linux-m68k.org; eugen.hris...@microchip.com; rdun...@infradead.org; > > lu...@wunner.de; freeman@spreadtrum.com; vilhelm.g...@gmail.com; > > t...@linutronix.de; baolin.w...@linaro.org; gre...@linuxfoundation.org; > > Srinivas Goud ; Anirudha Sarangi ; > > linux-...@vger.kernel.org; devicet...@vger.kernel.org; linux-arm- > > ker...@lists.infradead.org; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH 1/3] dt-bindings: iio: adc: Add Xilinx AMS binding > > documentation > > > > On Thu, 30 Aug 2018 15:52:17 +0530 > > Manish Narani wrote: > > > > > Xilinx AMS have several ADC channels that can be used for measurement > > > of different voltages and temperatures. Document the same in the bindings. > > > > > > Signed-off-by: Manish Narani > > > --- > > > .../devicetree/bindings/iio/adc/xilinx-ams.txt | 159 > > + > > > 1 file changed, 159 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt > > > b/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt > > > new file mode 100644 > > > index 000..8cc96f0 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt > > > @@ -0,0 +1,159 @@ > > > +Xilinx AMS controller > > > + > > > +The AMS includes an ADC as well as on-chip sensors that can be used > > > +to sample external voltages and monitor on-die operating conditions, > > > +such as temperature and supply voltage levels. The AMS has two SYSMON > > blocks. > > > +PL-SYSMON block is capable of monitoring off chip voltage and > > temperature. > > > +PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring > > > +from external master. Out of this interface currently only DRP is > > > supported. > > > +Other block PS-SYSMON is memory mapped to PS. Both of block has > > > +built-in alarm generation logic that is used to interrupt the > > > +processor based on condition set. > > > > I guess anyone reading this because they have the hardware would know what > > PS and PL are, but it would still be nice to define those acronyms! > > Okay, I will rectify this in v2. > > > > > Google suggest, Programmable Logic (FPGA bit I guess) and Process Space > > (Arm core bit?) As I read this, from a driver point of view it doesn't > > really > > matter - these are just blocks of channels that are there? > > Yes, these are just blocks of channels, which are enabled/disabled > via devicetree child nodes "xlnx,zynqmp-ams-ps" and > "xlnx,zynqmp-ams-pl". Great, put please expand in here on the naming. This may not matter for people using this particular driver, but having clear human readable descriptions is useful when we are looking for similarities between this an future drivers etc. > > > > > This binding is complex enough I definitely want some DT binding > > review. > > Sure! > > > > > > > > > > + > > > +All designs should have AMS registers, but PS and PL are > > > optional. +The AMS controller can work with only PS, only PL and > > > both PS and PL +configurations. Please specify registers > > > according to your design. +Devicetree should always have AMS > > > module property. Providing PS & PL > > module is optional. > > > + > > > +Required properties: > > > + - compatible: Should be "xlnx,zynqmp-ams" > > > + - reg: Should specify AMS register space > > > + - interrupts: Interrupt number for the AMS control > > > interface > > > + - interrupt-names: Interrupt name, must be "ams-irq" > > > + - clocks: Should contain a clock specifier for the device > > > + - ranges: keep the property empty to map child address > > > space > > > + (for PS
RE: [PATCH 1/3] dt-bindings: iio: adc: Add Xilinx AMS binding documentation
Hi Jonathan, Thanks for the review! > -Original Message- > From: Jonathan Cameron [mailto:ji...@kernel.org] > Sent: Sunday, September 2, 2018 11:45 PM > To: Manish Narani > Cc: knaac...@gmx.de; l...@metafoo.de; pme...@pmeerw.net; > robh...@kernel.org; mark.rutl...@arm.com; Michal Simek > ; leoyang...@nxp.com; sudeep.ho...@arm.com; > amit.kuche...@linaro.org; broo...@kernel.org; arnaud.pouliq...@st.com; > ge...@linux-m68k.org; eugen.hris...@microchip.com; rdun...@infradead.org; > lu...@wunner.de; freeman@spreadtrum.com; vilhelm.g...@gmail.com; > t...@linutronix.de; baolin.w...@linaro.org; gre...@linuxfoundation.org; > Srinivas Goud ; Anirudha Sarangi ; > linux-...@vger.kernel.org; devicet...@vger.kernel.org; linux-arm- > ker...@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/3] dt-bindings: iio: adc: Add Xilinx AMS binding > documentation > > On Thu, 30 Aug 2018 15:52:17 +0530 > Manish Narani wrote: > > > Xilinx AMS have several ADC channels that can be used for measurement > > of different voltages and temperatures. Document the same in the bindings. > > > > Signed-off-by: Manish Narani > > --- > > .../devicetree/bindings/iio/adc/xilinx-ams.txt | 159 > + > > 1 file changed, 159 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt > > b/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt > > new file mode 100644 > > index 000..8cc96f0 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt > > @@ -0,0 +1,159 @@ > > +Xilinx AMS controller > > + > > +The AMS includes an ADC as well as on-chip sensors that can be used > > +to sample external voltages and monitor on-die operating conditions, > > +such as temperature and supply voltage levels. The AMS has two SYSMON > blocks. > > +PL-SYSMON block is capable of monitoring off chip voltage and > temperature. > > +PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring > > +from external master. Out of this interface currently only DRP is > > supported. > > +Other block PS-SYSMON is memory mapped to PS. Both of block has > > +built-in alarm generation logic that is used to interrupt the > > +processor based on condition set. > > I guess anyone reading this because they have the hardware would know what > PS and PL are, but it would still be nice to define those acronyms! Okay, I will rectify this in v2. > > Google suggest, Programmable Logic (FPGA bit I guess) and Process Space > (Arm core bit?) As I read this, from a driver point of view it doesn't really > matter - these are just blocks of channels that are there? Yes, these are just blocks of channels, which are enabled/disabled via devicetree child nodes "xlnx,zynqmp-ams-ps" and "xlnx,zynqmp-ams-pl". > > This binding is complex enough I definitely want some DT binding review. Sure! > > > > > + > > +All designs should have AMS registers, but PS and PL are optional. > > +The AMS controller can work with only PS, only PL and both PS and PL > > +configurations. Please specify registers according to your design. > > +Devicetree should always have AMS module property. Providing PS & PL > module is optional. > > + > > +Required properties: > > + - compatible: Should be "xlnx,zynqmp-ams" > > + - reg: Should specify AMS register space > > + - interrupts: Interrupt number for the AMS control interface > > + - interrupt-names: Interrupt name, must be "ams-irq" > > + - clocks: Should contain a clock specifier for the device > > + - ranges: keep the property empty to map child address space > > + (for PS and/or PL) nodes 1:1 onto the parent address > > + space > > + > > +AMS device tree subnode: > > + - compatible: Should be "xlnx,zynqmp-ams-ps" or "xlnx,zynqmp-ams-pl" > > + - reg: Register space for PS or PL > > + > > +Optional properties: > > + > > +Following optional property only valid for PL. > > + - xlnx,ext-channels: List of external channels that are connected to the > > +AMS PL module. > > + > > + The child nodes of this node represent the external channels which > are > > + connected to the AMS Module. If the property is not present > > + no external channels will be assumed to be connected. > > + > > + Each child node represents one channel a
RE: [PATCH 1/3] dt-bindings: iio: adc: Add Xilinx AMS binding documentation
Hi Rob,
Thanks for the review. Please see my comments inline.
> From: Rob Herring [mailto:r...@kernel.org]
> Sent: Tuesday, September 4, 2018 6:48 AM
> To: Manish Narani
>
> On Thu, Aug 30, 2018 at 03:52:17PM +0530, Manish Narani wrote:
> > Xilinx AMS have several ADC channels that can be used for measurement
> > of different voltages and temperatures. Document the same in the bindings.
> >
> > Signed-off-by: Manish Narani
> > ---
> > .../devicetree/bindings/iio/adc/xilinx-ams.txt | 159
> +
> > 1 file changed, 159 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
> > b/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
> > new file mode 100644
> > index 000..8cc96f0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
> > +AMS device tree subnode:
> > + - compatible: Should be "xlnx,zynqmp-ams-ps" or "xlnx,zynqmp-ams-pl"
> > + - reg: Register space for PS or PL
>
> Please split each child node into its own section.
Okay. I will update this in v2.
>
> > +
> > +Optional properties:
> > +
> > +Following optional property only valid for PL.
>
> property or...
It should be node. I will update this in v2.
>
> > + - xlnx,ext-channels: List of external channels that are connected to the
> > +AMS PL module.
> > +
> > + The child nodes of this node represent the external channels which
> > +are
>
> node?
>
> > + connected to the AMS Module. If the property is not present
> > + no external channels will be assumed to be connected.
> > +
> > + Each child node represents one channel and has the following
> > + properties:
> > + Required properties:
> > + * reg: Pair of pins the channel is connected to.
> > + 0: VP/VN
> > + 1: VUSER0
> > + 2: VUSER1
> > + 3: VUSER3
> > + 4: VUSER4
> > + 5: VAUXP[0]/VAUXN[0]
> > + 6: VAUXP[1]/VAUXN[1]
> > + ...
> > + 20: VAUXP[15]/VAUXN[15]
> > + Note each channel number should only be used at
> most
> > + once.
> > + Optional properties:
> > + * xlnx,bipolar: If set the channel is used in bipolar
> > + mode.
> > +
> > +
> > +Example:
> > + xilinx_ams: ams@ffa5 {
> > + compatible = "xlnx,zynqmp-ams";
> > + interrupt-parent = <&gic>;
> > + interrupts = <0 56 4>;
> > + interrupt-names = "ams-irq";
> > + clocks = <&clkc 70>;
> > + reg = <0x0 0xffa5 0x0 0x800>;
> > + reg-names = "ams-base";
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges;
>
> There's no need for 64-bits of addresses and sizes here. Use ranges.
Okay.
> > +
> > + ams_ps: ams_ps@ffa50800 {
> > + compatible = "xlnx,zynqmp-ams-ps";
> > + reg = <0x0 0xffa50800 0x0 0x400>;
> > + };
> > +
> > + ams_pl: ams_pl@ffa50c00 {
> > + compatible = "xlnx,zynqmp-ams-pl";
> > + reg = <0x0 0xffa50c00 0x0 0x400>;
> > + xlnx,ext-channels {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + channel@0 {
> > + reg = <0>;
> > + xlnx,bipolar;
> > + };
> > + channel@1 {
> > + reg = <1>;
> > + };
> > + channel@8 {
> > + reg = <8>;
> > + xlnx,bipolar;
> > + };
> > + };
> > + };
> > + };
> > +
> > +AMS Channels Details:
> > +
> > +Sysmon Block |Channel| Details
> |Measurement
> > +Number
>Type
> > +---
> --
> > +AMS CTRL |0 |System PLLs voltage measurement, VCC_PSPLL.
> |Voltage
> > + |1 |Battery voltage measurement, VCC_PSBATT.
> |Voltage
> > + |2 |PL Internal voltage measurement, VCCINT.
> |Voltage
> > + |3 |Block RAM voltage measurement, VCCBRAM.
> |Voltage
> > + |4 |PL Aux voltage measurement, VCCAUX.
> |Voltage
> > + |5 |Voltage measurement for six DDR I/O PLLs,
> V
Re: [PATCH 1/3] dt-bindings: iio: adc: Add Xilinx AMS binding documentation
On Thu, 30 Aug 2018 15:52:17 +0530
Manish Narani wrote:
> Xilinx AMS have several ADC channels that can be used for measurement of
> different voltages and temperatures. Document the same in the bindings.
>
> Signed-off-by: Manish Narani
> ---
> .../devicetree/bindings/iio/adc/xilinx-ams.txt | 159
> +
> 1 file changed, 159 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
> b/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
> new file mode 100644
> index 000..8cc96f0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
> @@ -0,0 +1,159 @@
> +Xilinx AMS controller
> +
> +The AMS includes an ADC as well as on-chip sensors that can be used to
> +sample external voltages and monitor on-die operating conditions, such as
> +temperature and supply voltage levels. The AMS has two SYSMON blocks.
> +PL-SYSMON block is capable of monitoring off chip voltage and temperature.
> +PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring from
> +external master. Out of this interface currently only DRP is supported.
> +Other block PS-SYSMON is memory mapped to PS. Both of block has built-in
> +alarm generation logic that is used to interrupt the processor based on
> +condition set.
I guess anyone reading this because they have the hardware would know what
PS and PL are, but it would still be nice to define those acronyms!
Google suggest, Programmable Logic (FPGA bit I guess) and Process Space
(Arm core bit?) As I read this, from a driver point of view it doesn't
really matter - these are just blocks of channels that are there?
This binding is complex enough I definitely want some DT binding
review.
> +
> +All designs should have AMS registers, but PS and PL are optional. The
> +AMS controller can work with only PS, only PL and both PS and PL
> +configurations. Please specify registers according to your design. Devicetree
> +should always have AMS module property. Providing PS & PL module is optional.
> +
> +Required properties:
> + - compatible: Should be "xlnx,zynqmp-ams"
> + - reg: Should specify AMS register space
> + - interrupts: Interrupt number for the AMS control interface
> + - interrupt-names: Interrupt name, must be "ams-irq"
> + - clocks: Should contain a clock specifier for the device
> + - ranges: keep the property empty to map child address space
> + (for PS and/or PL) nodes 1:1 onto the parent address
> + space
> +
> +AMS device tree subnode:
> + - compatible: Should be "xlnx,zynqmp-ams-ps" or "xlnx,zynqmp-ams-pl"
> + - reg: Register space for PS or PL
> +
> +Optional properties:
> +
> +Following optional property only valid for PL.
> + - xlnx,ext-channels: List of external channels that are connected to the
> + AMS PL module.
> +
> + The child nodes of this node represent the external channels which are
> + connected to the AMS Module. If the property is not present
> + no external channels will be assumed to be connected.
> +
> + Each child node represents one channel and has the following
> + properties:
> + Required properties:
> + * reg: Pair of pins the channel is connected to.
> + 0: VP/VN
Hmm. So we have this table here of the ones that may or may not be connected
with different numbering from the overall table. Given these indexes
are arbitrary can we unify them?
I.e. use the index from below but state here which values can be used?
> + 1: VUSER0
> + 2: VUSER1
> + 3: VUSER3
> + 4: VUSER4
> + 5: VAUXP[0]/VAUXN[0]
> + 6: VAUXP[1]/VAUXN[1]
> + ...
> + 20: VAUXP[15]/VAUXN[15]
> + Note each channel number should only be used at most
> + once.
> + Optional properties:
> + * xlnx,bipolar: If set the channel is used in bipolar
> + mode.
> +
> +
> +Example:
> + xilinx_ams: ams@ffa5 {
> + compatible = "xlnx,zynqmp-ams";
> + interrupt-parent = <&gic>;
> + interrupts = <0 56 4>;
> + interrupt-names = "ams-irq";
> + clocks = <&clkc 70>;
> + reg = <0x0 0xffa5 0x0 0x800>;
> + reg-names = "ams-base";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> + ams_ps: ams_ps@ffa50800 {
> + compatible = "xlnx,zynqmp-ams-ps";
> + reg = <0x0 0xffa50800 0x0 0x400>;
> + };
> +
> + ams_pl: ams_pl@ffa50c00
[PATCH 1/3] dt-bindings: iio: adc: Add Xilinx AMS binding documentation
Xilinx AMS have several ADC channels that can be used for measurement of
different voltages and temperatures. Document the same in the bindings.
Signed-off-by: Manish Narani
---
.../devicetree/bindings/iio/adc/xilinx-ams.txt | 159 +
1 file changed, 159 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
diff --git a/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
b/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
new file mode 100644
index 000..8cc96f0
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/xilinx-ams.txt
@@ -0,0 +1,159 @@
+Xilinx AMS controller
+
+The AMS includes an ADC as well as on-chip sensors that can be used to
+sample external voltages and monitor on-die operating conditions, such as
+temperature and supply voltage levels. The AMS has two SYSMON blocks.
+PL-SYSMON block is capable of monitoring off chip voltage and temperature.
+PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring from
+external master. Out of this interface currently only DRP is supported.
+Other block PS-SYSMON is memory mapped to PS. Both of block has built-in
+alarm generation logic that is used to interrupt the processor based on
+condition set.
+
+All designs should have AMS registers, but PS and PL are optional. The
+AMS controller can work with only PS, only PL and both PS and PL
+configurations. Please specify registers according to your design. Devicetree
+should always have AMS module property. Providing PS & PL module is optional.
+
+Required properties:
+ - compatible: Should be "xlnx,zynqmp-ams"
+ - reg: Should specify AMS register space
+ - interrupts: Interrupt number for the AMS control interface
+ - interrupt-names: Interrupt name, must be "ams-irq"
+ - clocks: Should contain a clock specifier for the device
+ - ranges: keep the property empty to map child address space
+ (for PS and/or PL) nodes 1:1 onto the parent address
+ space
+
+AMS device tree subnode:
+ - compatible: Should be "xlnx,zynqmp-ams-ps" or "xlnx,zynqmp-ams-pl"
+ - reg: Register space for PS or PL
+
+Optional properties:
+
+Following optional property only valid for PL.
+ - xlnx,ext-channels: List of external channels that are connected to the
+AMS PL module.
+
+ The child nodes of this node represent the external channels which are
+ connected to the AMS Module. If the property is not present
+ no external channels will be assumed to be connected.
+
+ Each child node represents one channel and has the following
+ properties:
+ Required properties:
+ * reg: Pair of pins the channel is connected to.
+ 0: VP/VN
+ 1: VUSER0
+ 2: VUSER1
+ 3: VUSER3
+ 4: VUSER4
+ 5: VAUXP[0]/VAUXN[0]
+ 6: VAUXP[1]/VAUXN[1]
+ ...
+ 20: VAUXP[15]/VAUXN[15]
+ Note each channel number should only be used at most
+ once.
+ Optional properties:
+ * xlnx,bipolar: If set the channel is used in bipolar
+ mode.
+
+
+Example:
+ xilinx_ams: ams@ffa5 {
+ compatible = "xlnx,zynqmp-ams";
+ interrupt-parent = <&gic>;
+ interrupts = <0 56 4>;
+ interrupt-names = "ams-irq";
+ clocks = <&clkc 70>;
+ reg = <0x0 0xffa5 0x0 0x800>;
+ reg-names = "ams-base";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ ams_ps: ams_ps@ffa50800 {
+ compatible = "xlnx,zynqmp-ams-ps";
+ reg = <0x0 0xffa50800 0x0 0x400>;
+ };
+
+ ams_pl: ams_pl@ffa50c00 {
+ compatible = "xlnx,zynqmp-ams-pl";
+ reg = <0x0 0xffa50c00 0x0 0x400>;
+ xlnx,ext-channels {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ channel@0 {
+ reg = <0>;
+ xlnx,bipolar;
+ };
+ channel@1 {
+ reg = <1>;
+ };
+ channel@8 {
+ reg = <8>;
+ xlnx,bipolar;
+ };
+ };
+ };
+ };
+
+AMS Channels D
