[PATCH 0/4] Xilinx AMS fixes

[PATCH 0/4] Xilinx AMS fixes

[Robert Hancock]

Various fixes for the Xilinx AMS driver, and addition of the required
references in the ZynqMP device tree files to use it.

Robert Hancock (4):
  arm64: dts: zynqmp: add AMS driver to device tree
  iio: adc: xilinx-ams: Fixed missing PS channels
  iio: adc: xilinx-ams: Fixed wrong sequencer register settings
  iio: adc: xilinx-ams: Fix single channel switching sequence

 .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi |  4 +++
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi        | 26 +++++++++++++++++++
 drivers/iio/adc/xilinx-ams.c                  | 15 ++++++++---
 3 files changed, 41 insertions(+), 4 deletions(-)

-- 
2.31.1

[PATCH 1/4] arm64: dts: zynqmp: add AMS driver to device tree

[Robert Hancock]

Add an entry to the ZynqMP device tree to support the AMS device which
now has a driver in mainline.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi |  4 +++
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi        | 26 +++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
index 1e0b1bca7c94..108592104a1b 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
@@ -239,6 +239,10 @@ &lpd_watchdog {
 	clocks = <&zynqmp_clk LPD_WDT>;
 };
 
+&xilinx_ams {
+	clocks = <&zynqmp_clk AMS_REF>;
+};
+
 &zynqmp_dpdma {
 	clocks = <&zynqmp_clk DPDMA_REF>;
 };
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 74e66443e4ce..d1fe1e5b46c1 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -878,6 +878,32 @@ lpd_watchdog: watchdog@ff150000 {
 			timeout-sec = <10>;
 		};
 
+		xilinx_ams: ams@ffa50000 {
+			compatible = "xlnx,zynqmp-ams";
+			status = "disabled";
+			interrupt-parent = <&gic>;
+			interrupts = <0 56 4>;
+			reg = <0x0 0xffa50000 0x0 0x800>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			#io-channel-cells = <1>;
+			ranges = <0 0 0xffa50800 0x800>;
+
+			ams_ps: ams_ps@0 {
+				compatible = "xlnx,zynqmp-ams-ps";
+				status = "disabled";
+				reg = <0x0 0x400>;
+			};
+
+			ams_pl: ams_pl@400 {
+				compatible = "xlnx,zynqmp-ams-pl";
+				status = "disabled";
+				reg = <0x400 0x400>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+
 		zynqmp_dpdma: dma-controller@fd4c0000 {
 			compatible = "xlnx,zynqmp-dpdma";
 			status = "disabled";
-- 
2.31.1

Re: [PATCH 1/4] arm64: dts: zynqmp: add AMS driver to device tree

[Michael Tretter]

On Wed, 19 Jan 2022 19:02:43 -0600, Robert Hancock wrote:
> Add an entry to the ZynqMP device tree to support the AMS device which
> now has a driver in mainline.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>

Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>

> ---
>  .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi |  4 +++
>  arch/arm64/boot/dts/xilinx/zynqmp.dtsi        | 26 +++++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
> index 1e0b1bca7c94..108592104a1b 100644
> --- a/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
> +++ b/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
> @@ -239,6 +239,10 @@ &lpd_watchdog {
>  	clocks = <&zynqmp_clk LPD_WDT>;
>  };
>  
> +&xilinx_ams {
> +	clocks = <&zynqmp_clk AMS_REF>;
> +};
> +
>  &zynqmp_dpdma {
>  	clocks = <&zynqmp_clk DPDMA_REF>;
>  };
> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> index 74e66443e4ce..d1fe1e5b46c1 100644
> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> @@ -878,6 +878,32 @@ lpd_watchdog: watchdog@ff150000 {
>  			timeout-sec = <10>;
>  		};
>  
> +		xilinx_ams: ams@ffa50000 {
> +			compatible = "xlnx,zynqmp-ams";
> +			status = "disabled";
> +			interrupt-parent = <&gic>;
> +			interrupts = <0 56 4>;
> +			reg = <0x0 0xffa50000 0x0 0x800>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#io-channel-cells = <1>;
> +			ranges = <0 0 0xffa50800 0x800>;
> +
> +			ams_ps: ams_ps@0 {
> +				compatible = "xlnx,zynqmp-ams-ps";
> +				status = "disabled";
> +				reg = <0x0 0x400>;
> +			};
> +
> +			ams_pl: ams_pl@400 {
> +				compatible = "xlnx,zynqmp-ams-pl";
> +				status = "disabled";
> +				reg = <0x400 0x400>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};
> +
>  		zynqmp_dpdma: dma-controller@fd4c0000 {
>  			compatible = "xlnx,zynqmp-dpdma";
>  			status = "disabled";
> -- 
> 2.31.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 1/4] arm64: dts: zynqmp: add AMS driver to device tree

[Michal Simek]

On 1/20/22 02:02, Robert Hancock wrote:
> Add an entry to the ZynqMP device tree to support the AMS device which
> now has a driver in mainline.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>   .../arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi |  4 +++
>   arch/arm64/boot/dts/xilinx/zynqmp.dtsi        | 26 +++++++++++++++++++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
> index 1e0b1bca7c94..108592104a1b 100644
> --- a/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
> +++ b/arch/arm64/boot/dts/xilinx/zynqmp-clk-ccf.dtsi
> @@ -239,6 +239,10 @@ &lpd_watchdog {
>   	clocks = <&zynqmp_clk LPD_WDT>;
>   };
>   
> +&xilinx_ams {
> +	clocks = <&zynqmp_clk AMS_REF>;
> +};

Please send this patch out of the series. It should go via soc tree not via iio 
tree.
And unfortunately clock is not listed in DT binding document and needs to be 
added there.
Can you please send that patch too? dtbs_check reports it as issue.

> +
>   &zynqmp_dpdma {
>   	clocks = <&zynqmp_clk DPDMA_REF>;
>   };
> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> index 74e66443e4ce..d1fe1e5b46c1 100644
> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> @@ -878,6 +878,32 @@ lpd_watchdog: watchdog@ff150000 {
>   			timeout-sec = <10>;
>   		};
>   
> +		xilinx_ams: ams@ffa50000 {
> +			compatible = "xlnx,zynqmp-ams";
> +			status = "disabled";
> +			interrupt-parent = <&gic>;
> +			interrupts = <0 56 4>;
> +			reg = <0x0 0xffa50000 0x0 0x800>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			#io-channel-cells = <1>;
> +			ranges = <0 0 0xffa50800 0x800>;
> +
> +			ams_ps: ams_ps@0 {
> +				compatible = "xlnx,zynqmp-ams-ps";
> +				status = "disabled";
> +				reg = <0x0 0x400>;
> +			};
> +
> +			ams_pl: ams_pl@400 {
> +				compatible = "xlnx,zynqmp-ams-pl";
> +				status = "disabled";
> +				reg = <0x400 0x400>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};
> +
>   		zynqmp_dpdma: dma-controller@fd4c0000 {
>   			compatible = "xlnx,zynqmp-dpdma";
>   			status = "disabled";

And this second piece is completely aligned with dt binding that's why it is fine.

Thanks,
Michal

[PATCH 2/4] iio: adc: xilinx-ams: Fixed missing PS channels

[Robert Hancock]

The code forgot to increment num_channels for the PS channel inputs,
resulting in them not being enabled as they should.

Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/iio/adc/xilinx-ams.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
index 8343c5f74121..b93864362dac 100644
--- a/drivers/iio/adc/xilinx-ams.c
+++ b/drivers/iio/adc/xilinx-ams.c
@@ -1224,6 +1224,7 @@ static int ams_init_module(struct iio_dev *indio_dev,
 
 		/* add PS channels to iio device channels */
 		memcpy(channels, ams_ps_channels, sizeof(ams_ps_channels));
+		num_channels = ARRAY_SIZE(ams_ps_channels);
 	} else if (fwnode_property_match_string(fwnode, "compatible",
 						"xlnx,zynqmp-ams-pl") == 0) {
 		ams->pl_base = fwnode_iomap(fwnode, 0);
-- 
2.31.1

Re: [PATCH 2/4] iio: adc: xilinx-ams: Fixed missing PS channels

[Robert Hancock]

On Wed, 2022-01-19 at 19:02 -0600, Robert Hancock wrote:
> The code forgot to increment num_channels for the PS channel inputs,
> resulting in them not being enabled as they should.
> 
> Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>  drivers/iio/adc/xilinx-ams.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> index 8343c5f74121..b93864362dac 100644
> --- a/drivers/iio/adc/xilinx-ams.c
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -1224,6 +1224,7 @@ static int ams_init_module(struct iio_dev *indio_dev,
>  
>  		/* add PS channels to iio device channels */
>  		memcpy(channels, ams_ps_channels, sizeof(ams_ps_channels));
> +		num_channels = ARRAY_SIZE(ams_ps_channels);
>  	} else if (fwnode_property_match_string(fwnode, "compatible",
>  						"xlnx,zynqmp-ams-pl") == 0) {
>  		ams->pl_base = fwnode_iomap(fwnode, 0);

Looks like this is the same change just submitted by Michael Tretter ("iio:
adc: xilinx-ams: Fix num_channels for PS channels").

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: [PATCH 2/4] iio: adc: xilinx-ams: Fixed missing PS channels

[Michael Tretter]

On Thu, 20 Jan 2022 01:09:39 +0000, Robert Hancock wrote:
> On Wed, 2022-01-19 at 19:02 -0600, Robert Hancock wrote:
> > The code forgot to increment num_channels for the PS channel inputs,
> > resulting in them not being enabled as they should.
> > 
> > Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > ---
> >  drivers/iio/adc/xilinx-ams.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> > index 8343c5f74121..b93864362dac 100644
> > --- a/drivers/iio/adc/xilinx-ams.c
> > +++ b/drivers/iio/adc/xilinx-ams.c
> > @@ -1224,6 +1224,7 @@ static int ams_init_module(struct iio_dev *indio_dev,
> >  
> >  		/* add PS channels to iio device channels */
> >  		memcpy(channels, ams_ps_channels, sizeof(ams_ps_channels));
> > +		num_channels = ARRAY_SIZE(ams_ps_channels);
> >  	} else if (fwnode_property_match_string(fwnode, "compatible",
> >  						"xlnx,zynqmp-ams-pl") == 0) {
> >  		ams->pl_base = fwnode_iomap(fwnode, 0);
> 
> Looks like this is the same change just submitted by Michael Tretter ("iio:
> adc: xilinx-ams: Fix num_channels for PS channels").

Thanks for pointing me here.

Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>

Michael

Re: [PATCH 2/4] iio: adc: xilinx-ams: Fixed missing PS channels

[Jonathan Cameron]

On Tue, 25 Jan 2022 09:06:34 +0100
Michael Tretter <m.tretter@pengutronix.de> wrote:

> On Thu, 20 Jan 2022 01:09:39 +0000, Robert Hancock wrote:
> > On Wed, 2022-01-19 at 19:02 -0600, Robert Hancock wrote:  
> > > The code forgot to increment num_channels for the PS channel inputs,
> > > resulting in them not being enabled as they should.
> > > 
> > > Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> > > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > > ---
> > >  drivers/iio/adc/xilinx-ams.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> > > index 8343c5f74121..b93864362dac 100644
> > > --- a/drivers/iio/adc/xilinx-ams.c
> > > +++ b/drivers/iio/adc/xilinx-ams.c
> > > @@ -1224,6 +1224,7 @@ static int ams_init_module(struct iio_dev *indio_dev,
> > >  
> > >  		/* add PS channels to iio device channels */
> > >  		memcpy(channels, ams_ps_channels, sizeof(ams_ps_channels));
> > > +		num_channels = ARRAY_SIZE(ams_ps_channels);
> > >  	} else if (fwnode_property_match_string(fwnode, "compatible",
> > >  						"xlnx,zynqmp-ams-pl") == 0) {
> > >  		ams->pl_base = fwnode_iomap(fwnode, 0);  
> > 
> > Looks like this is the same change just submitted by Michael Tretter ("iio:
> > adc: xilinx-ams: Fix num_channels for PS channels").  
> 
> Thanks for pointing me here.
> 
> Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>

I picked up Michael's patch and will drop this one...
(based on random email reading order. I'd argue his was first in time, but
then someone would point out next time I pick a patch up that is later than
another one :)

Thanks,

> 
> Michael

[PATCH 3/4] iio: adc: xilinx-ams: Fixed wrong sequencer register settings

[Robert Hancock]

Register settings used for the sequencer configuration register
were incorrect, causing some inputs to not be read properly.

Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/iio/adc/xilinx-ams.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
index b93864362dac..199027c93cdc 100644
--- a/drivers/iio/adc/xilinx-ams.c
+++ b/drivers/iio/adc/xilinx-ams.c
@@ -91,8 +91,8 @@
 
 #define AMS_CONF1_SEQ_MASK		GENMASK(15, 12)
 #define AMS_CONF1_SEQ_DEFAULT		FIELD_PREP(AMS_CONF1_SEQ_MASK, 0)
-#define AMS_CONF1_SEQ_CONTINUOUS	FIELD_PREP(AMS_CONF1_SEQ_MASK, 1)
-#define AMS_CONF1_SEQ_SINGLE_CHANNEL	FIELD_PREP(AMS_CONF1_SEQ_MASK, 2)
+#define AMS_CONF1_SEQ_CONTINUOUS	FIELD_PREP(AMS_CONF1_SEQ_MASK, 2)
+#define AMS_CONF1_SEQ_SINGLE_CHANNEL	FIELD_PREP(AMS_CONF1_SEQ_MASK, 3)
 
 #define AMS_REG_SEQ0_MASK		GENMASK(15, 0)
 #define AMS_REG_SEQ2_MASK		GENMASK(21, 16)
-- 
2.31.1

Re: [PATCH 3/4] iio: adc: xilinx-ams: Fixed wrong sequencer register settings

[Michael Tretter]

On Wed, 19 Jan 2022 19:02:45 -0600, Robert Hancock wrote:
> Register settings used for the sequencer configuration register
> were incorrect, causing some inputs to not be read properly.
> 
> Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>  drivers/iio/adc/xilinx-ams.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> index b93864362dac..199027c93cdc 100644
> --- a/drivers/iio/adc/xilinx-ams.c
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -91,8 +91,8 @@
>  
>  #define AMS_CONF1_SEQ_MASK		GENMASK(15, 12)
>  #define AMS_CONF1_SEQ_DEFAULT		FIELD_PREP(AMS_CONF1_SEQ_MASK, 0)
> -#define AMS_CONF1_SEQ_CONTINUOUS	FIELD_PREP(AMS_CONF1_SEQ_MASK, 1)
> -#define AMS_CONF1_SEQ_SINGLE_CHANNEL	FIELD_PREP(AMS_CONF1_SEQ_MASK, 2)
> +#define AMS_CONF1_SEQ_CONTINUOUS	FIELD_PREP(AMS_CONF1_SEQ_MASK, 2)
> +#define AMS_CONF1_SEQ_SINGLE_CHANNEL	FIELD_PREP(AMS_CONF1_SEQ_MASK, 3)

The TRM states that Continuous Loop Mode is 2, but Single Pass Sequence Mode
is 1, not 3. Is there a reason, why you need to set both bits?

Michael

>  
>  #define AMS_REG_SEQ0_MASK		GENMASK(15, 0)
>  #define AMS_REG_SEQ2_MASK		GENMASK(21, 16)
> -- 
> 2.31.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 3/4] iio: adc: xilinx-ams: Fixed wrong sequencer register settings

[Robert Hancock]

On Tue, 2022-01-25 at 09:21 +0100, Michael Tretter wrote:
> On Wed, 19 Jan 2022 19:02:45 -0600, Robert Hancock wrote:
> > Register settings used for the sequencer configuration register
> > were incorrect, causing some inputs to not be read properly.
> > 
> > Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > ---
> >  drivers/iio/adc/xilinx-ams.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> > index b93864362dac..199027c93cdc 100644
> > --- a/drivers/iio/adc/xilinx-ams.c
> > +++ b/drivers/iio/adc/xilinx-ams.c
> > @@ -91,8 +91,8 @@
> >  
> >  #define AMS_CONF1_SEQ_MASK		GENMASK(15, 12)
> >  #define AMS_CONF1_SEQ_DEFAULT		FIELD_PREP(AMS_CONF1_SEQ_MASK,
> > 0)
> > -#define AMS_CONF1_SEQ_CONTINUOUS	FIELD_PREP(AMS_CONF1_SEQ_MASK, 1)
> > -#define AMS_CONF1_SEQ_SINGLE_CHANNEL	FIELD_PREP(AMS_CONF1_SEQ_MASK,
> > 2)
> > +#define AMS_CONF1_SEQ_CONTINUOUS	FIELD_PREP(AMS_CONF1_SEQ_MASK, 2)
> > +#define AMS_CONF1_SEQ_SINGLE_CHANNEL	FIELD_PREP(AMS_CONF1_SEQ_MASK,
> > 3)
> 
> The TRM states that Continuous Loop Mode is 2, but Single Pass Sequence Mode
> is 1, not 3. Is there a reason, why you need to set both bits?

Single pass sequence mode (1) just runs the same sequence only once. To read
these values it needs to switch to single channel mode (3).

The register bits are defined in Table 3-8 of 
https://www.xilinx.com/support/documentation/user_guides/ug580-ultrascale-sysmon.pdf
 .

> 
> Michael
> 
> >  
> >  #define AMS_REG_SEQ0_MASK		GENMASK(15, 0)
> >  #define AMS_REG_SEQ2_MASK		GENMASK(21, 16)
> > -- 
> > 2.31.1
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!IOGos0k!yGFEjSC1BL20lwurby914len0HCLXyzarwxKJP9Jx30qv_qrERSkRJUiVo_2MdusMVA$ 
> > 
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: [PATCH 3/4] iio: adc: xilinx-ams: Fixed wrong sequencer register settings

[Michael Tretter]

On Tue, 25 Jan 2022 16:15:05 +0000, Robert Hancock wrote:
> On Tue, 2022-01-25 at 09:21 +0100, Michael Tretter wrote:
> > On Wed, 19 Jan 2022 19:02:45 -0600, Robert Hancock wrote:
> > > Register settings used for the sequencer configuration register
> > > were incorrect, causing some inputs to not be read properly.
> > > 
> > > Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> > > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > > ---
> > >  drivers/iio/adc/xilinx-ams.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> > > index b93864362dac..199027c93cdc 100644
> > > --- a/drivers/iio/adc/xilinx-ams.c
> > > +++ b/drivers/iio/adc/xilinx-ams.c
> > > @@ -91,8 +91,8 @@
> > >  
> > >  #define AMS_CONF1_SEQ_MASK		GENMASK(15, 12)
> > >  #define AMS_CONF1_SEQ_DEFAULT		FIELD_PREP(AMS_CONF1_SEQ_MASK,
> > > 0)
> > > -#define AMS_CONF1_SEQ_CONTINUOUS	FIELD_PREP(AMS_CONF1_SEQ_MASK, 1)
> > > -#define AMS_CONF1_SEQ_SINGLE_CHANNEL	FIELD_PREP(AMS_CONF1_SEQ_MASK,
> > > 2)
> > > +#define AMS_CONF1_SEQ_CONTINUOUS	FIELD_PREP(AMS_CONF1_SEQ_MASK, 2)
> > > +#define AMS_CONF1_SEQ_SINGLE_CHANNEL	FIELD_PREP(AMS_CONF1_SEQ_MASK,
> > > 3)
> > 
> > The TRM states that Continuous Loop Mode is 2, but Single Pass Sequence Mode
> > is 1, not 3. Is there a reason, why you need to set both bits?
> 
> Single pass sequence mode (1) just runs the same sequence only once. To read
> these values it needs to switch to single channel mode (3).
> 
> The register bits are defined in Table 3-8 of 
> https://www.xilinx.com/support/documentation/user_guides/ug580-ultrascale-sysmon.pdf
>  .

Thanks for the clarification.

Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>

> 
> > 
> > Michael
> > 
> > >  
> > >  #define AMS_REG_SEQ0_MASK		GENMASK(15, 0)
> > >  #define AMS_REG_SEQ2_MASK		GENMASK(21, 16)
> > > -- 
> > > 2.31.1
> > > 
> > > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > https://urldefense.com/v3/__http://lists.infradead.org/mailman/listinfo/linux-arm-kernel__;!!IOGos0k!yGFEjSC1BL20lwurby914len0HCLXyzarwxKJP9Jx30qv_qrERSkRJUiVo_2MdusMVA$ 
> > > 
> -- 
> Robert Hancock
> Senior Hardware Designer, Calian Advanced Technologies
> www.calian.com

Re: [PATCH 3/4] iio: adc: xilinx-ams: Fixed wrong sequencer register settings

[Jonathan Cameron]

On Wed, 26 Jan 2022 10:12:50 +0100
Michael Tretter <m.tretter@pengutronix.de> wrote:

> On Tue, 25 Jan 2022 16:15:05 +0000, Robert Hancock wrote:
> > On Tue, 2022-01-25 at 09:21 +0100, Michael Tretter wrote:  
> > > On Wed, 19 Jan 2022 19:02:45 -0600, Robert Hancock wrote:  
> > > > Register settings used for the sequencer configuration register
> > > > were incorrect, causing some inputs to not be read properly.
> > > > 
> > > > Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > > > ---
> > > >  drivers/iio/adc/xilinx-ams.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> > > > index b93864362dac..199027c93cdc 100644
> > > > --- a/drivers/iio/adc/xilinx-ams.c
> > > > +++ b/drivers/iio/adc/xilinx-ams.c
> > > > @@ -91,8 +91,8 @@
> > > >  
> > > >  #define AMS_CONF1_SEQ_MASK		GENMASK(15, 12)
> > > >  #define AMS_CONF1_SEQ_DEFAULT		FIELD_PREP(AMS_CONF1_SEQ_MASK,
> > > > 0)
> > > > -#define AMS_CONF1_SEQ_CONTINUOUS	FIELD_PREP(AMS_CONF1_SEQ_MASK, 1)
> > > > -#define AMS_CONF1_SEQ_SINGLE_CHANNEL	FIELD_PREP(AMS_CONF1_SEQ_MASK,
> > > > 2)
> > > > +#define AMS_CONF1_SEQ_CONTINUOUS	FIELD_PREP(AMS_CONF1_SEQ_MASK, 2)
> > > > +#define AMS_CONF1_SEQ_SINGLE_CHANNEL	FIELD_PREP(AMS_CONF1_SEQ_MASK,
> > > > 3)  
> > > 
> > > The TRM states that Continuous Loop Mode is 2, but Single Pass Sequence Mode
> > > is 1, not 3. Is there a reason, why you need to set both bits?  
> > 
> > Single pass sequence mode (1) just runs the same sequence only once. To read
> > these values it needs to switch to single channel mode (3).
> > 
> > The register bits are defined in Table 3-8 of 
> > https://www.xilinx.com/support/documentation/user_guides/ug580-ultrascale-sysmon.pdf
> >  .  
> 
> Thanks for the clarification.
> 
> Reviewed-by: Michael Tretter <m.tretter@pengutronix.de>

Applied to the fixes-togreg branch of iio.git

Thanks,

Jonathan

[PATCH 4/4] iio: adc: xilinx-ams: Fix single channel switching sequence

[Robert Hancock]

Some of the AMS channels need to be read by switching into single-channel
mode from the normal polling sequence. There was a logic issue in this
switching code that could cause the first read of these channels to read
back as zero.

It appears that the sequencer should be set back to default mode before
changing the channel selection, and the channel should be set before
switching the sequencer back into single-channel mode.

Also, write 1 to the EOC bit in the status register to clear it before
waiting for it to become set, so that we actually wait for a new
conversion to complete, and don't proceed based on a previous conversion
completing.

Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/iio/adc/xilinx-ams.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
index 199027c93cdc..7bf097fa10cb 100644
--- a/drivers/iio/adc/xilinx-ams.c
+++ b/drivers/iio/adc/xilinx-ams.c
@@ -530,14 +530,18 @@ static int ams_enable_single_channel(struct ams *ams, unsigned int offset)
 		return -EINVAL;
 	}
 
-	/* set single channel, sequencer off mode */
+	/* put sysmon in a soft reset to change the sequence */
 	ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK,
-			  AMS_CONF1_SEQ_SINGLE_CHANNEL);
+			  AMS_CONF1_SEQ_DEFAULT);
 
 	/* write the channel number */
 	ams_ps_update_reg(ams, AMS_REG_CONFIG0, AMS_CONF0_CHANNEL_NUM_MASK,
 			  channel_num);
 
+	/* set single channel, sequencer off mode */
+	ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK,
+			  AMS_CONF1_SEQ_SINGLE_CHANNEL);
+
 	return 0;
 }
 
@@ -551,6 +555,8 @@ static int ams_read_vcc_reg(struct ams *ams, unsigned int offset, u32 *data)
 	if (ret)
 		return ret;
 
+	/* clear end-of-conversion flag, wait for next conversion to complete */
+	writel(expect, ams->base + AMS_ISR_1);
 	ret = readl_poll_timeout(ams->base + AMS_ISR_1, reg, (reg & expect),
 				 AMS_INIT_POLL_TIME_US, AMS_INIT_TIMEOUT_US);
 	if (ret)
-- 
2.31.1

Re: [PATCH 4/4] iio: adc: xilinx-ams: Fix single channel switching sequence

[Jonathan Cameron]

On Wed, 19 Jan 2022 19:02:46 -0600
Robert Hancock <robert.hancock@calian.com> wrote:

> Some of the AMS channels need to be read by switching into single-channel
> mode from the normal polling sequence. There was a logic issue in this
> switching code that could cause the first read of these channels to read
> back as zero.
> 
> It appears that the sequencer should be set back to default mode before
> changing the channel selection, and the channel should be set before
> switching the sequencer back into single-channel mode.
> 
> Also, write 1 to the EOC bit in the status register to clear it before
> waiting for it to become set, so that we actually wait for a new
> conversion to complete, and don't proceed based on a previous conversion
> completing.
> 
> Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>

Looking for an Ack from Anand or someone else familiar with this device.

Thanks,

Jonathan


> ---
>  drivers/iio/adc/xilinx-ams.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> index 199027c93cdc..7bf097fa10cb 100644
> --- a/drivers/iio/adc/xilinx-ams.c
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -530,14 +530,18 @@ static int ams_enable_single_channel(struct ams *ams, unsigned int offset)
>  		return -EINVAL;
>  	}
>  
> -	/* set single channel, sequencer off mode */
> +	/* put sysmon in a soft reset to change the sequence */
>  	ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK,
> -			  AMS_CONF1_SEQ_SINGLE_CHANNEL);
> +			  AMS_CONF1_SEQ_DEFAULT);
>  
>  	/* write the channel number */
>  	ams_ps_update_reg(ams, AMS_REG_CONFIG0, AMS_CONF0_CHANNEL_NUM_MASK,
>  			  channel_num);
>  
> +	/* set single channel, sequencer off mode */
> +	ams_ps_update_reg(ams, AMS_REG_CONFIG1, AMS_CONF1_SEQ_MASK,
> +			  AMS_CONF1_SEQ_SINGLE_CHANNEL);
> +
>  	return 0;
>  }
>  
> @@ -551,6 +555,8 @@ static int ams_read_vcc_reg(struct ams *ams, unsigned int offset, u32 *data)
>  	if (ret)
>  		return ret;
>  
> +	/* clear end-of-conversion flag, wait for next conversion to complete */
> +	writel(expect, ams->base + AMS_ISR_1);
>  	ret = readl_poll_timeout(ams->base + AMS_ISR_1, reg, (reg & expect),
>  				 AMS_INIT_POLL_TIME_US, AMS_INIT_TIMEOUT_US);
>  	if (ret)