On 04/20, Jonathan Cameron wrote:
> On Tue, 16 Apr 2024 18:46:11 -0300
> Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx> wrote:
>
> > So, I have been trying to make this work, though I haven't been successful so
> > far, and I don't really think using pinctrl is a good solution for this either.
> >
> > Comments inline.
> >
> > On 04/14, Jonathan Cameron wrote:
> > > On Sat, 13 Apr 2024 12:33:54 -0500
> > > David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> > >
> > > > On 4/13/24 11:14 AM, Jonathan Cameron wrote:
> > > > > On Tue, 9 Apr 2024 12:30:09 -0300
> > > > > Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx> wrote:
> > > > >
> > > > >> On 04/08, David Lechner wrote:
> > > > >>> On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> > > > >>> <marcelo.schmitt@xxxxxxxxxx> wrote:
> > > > >>>>
> > > >
> > > > ...
> > > >
> >
> > ...
> >
...
> >
> > The pinctrl configuration for this ADC would not be meant to change once after
> > boot as it looks to be the usual use case for pinctrl (including mediatek-bluetooth.txt).
> >
> > Also, no suitable mux for the "3-wire" mode in
> > Documentation/devicetree/bindings/pinctrl/xlnx,pinctrl-zynq.yaml
> > to do it like Documentation/devicetree/bindings/net/mediatek-bluetooth.txt.
> > The zynq pinctrl driver (drivers/pinctrl/pinctrl-zynq.c) would have to be
> > updated to add the new mux function in
> > static const struct zynq_pinmux_function zynq_pmux_functions[] = {
> > DEFINE_ZYNQ_PINMUX_FUNCTION(ethernet0, 1),
> > ...
> > DEFINE_ZYNQ_PINMUX_FUNCTION(axi_spi_single, 1),
> > DEFINE_ZYNQ_PINMUX_FUNCTION(axi_spi_multi, 2),
> > though this is not really a thing that's on zynq, but one that is related to
> > these ADCs so I'm not sure it should go there.
>
>
> I'd argue we are after a specific SPI controller setup for this.
> A controller driver would need modifying to make it work.
Ack, makes sense to me.
>
> >
> > > > For example, if we wanted to use 3-wire mode for reading
> > > > data, we would set the pinctrl to "default" to write the
> > > > register to configure the chip during driver probe. Then
> > > > to read data, we would change the pinctrl to "single" before
> > > > doing the SPI xfer to ensure that the ADC SDI pin is pulled
> > > > high independent of what the SDO line of the SPI controller
> > > > is currently doing.
> >
> > No, the pin configuration for this ADCs would be expected to change unrestricted
> > amount of times. The pin configuration would have to change every time a sample
> > read is made after a register access transfers and vice-versa. If we decide
> > to support span compression, every change to _scale would lead to pinctrl
> > configuration change.
> >
> > At pin level, we would want to rise SPI controller SDO line to VIO but then
> > the new SDO pin config would conflict with SPI pin group config.
> >
> > I included pinctrl properties in my test dts to make use of pinctrl framework.
> > Yet, that doesn't really alternate SPI line configuration we are using because
> > the SPI function that is in the PS (FPGA's Processing System) is not what we are
> > interfacing when using spi-engine. Copy of my test dts at end of email.
> >
> > Currently, the SPI controller we are using to test with these exotic ADCs
> > is the spi-engine which is implemented in the FPGA as an IP block which
> > owns control of the bus lines (SPI, SDO, CS, ...). To alternate the
> > configuration of SPI lines (pull SDO (ADC SDI) up to VIO, connect controller CS
> > to ADC SDI, etc.) I think it should be done in the HDL project. I don't think
> > it's a good idea to hijack spi-engine lines through pinctrl.
>
> Such functionality would need to be pushed to the spi controller driver
> which could know if there was any need to do anything like this, or if there
> was simply a register to set.
>
Ack.
> >
> > >
> > > Ah ok. This is implying that we are switching to a controllable
> > > mode to pull that pin high (or I guess one where it is always
> > > high). I'm not sure if that's more common than an SPI controller
> > > where you can set the 'don't' care state to high or low.
> > > I assume we can't get away with just setting the output buffer
> > > to all 1s because it won't hold that state between transfers?
> >
> > I tried sending the tx buffer filled with 1s when testing this with a RPi4 but
> > it brought the controller SDO (ADC SDI) line low between each 8 bits of transfer
> > (attaching an image (yellow line is SCLK, green lines is controller SDO)).
>
> Pity - thought that was overly optimistic.
>
> > Anyway, can we have any guaranties with respect to controller SDO line behaviour
> > when its not clocking out data?
>
>
> >
> > >
> > > Feels like that could be rolled into the SPI subsystem with
> > > any necessary transitions handled there (maybe)
> >
> > To me, this sounds more reasonable than pinctrl.
> > Yeah, if we can set a don't' care state for the SDO line then that should be
> > enough for these ADCs.
> > Otherwise, can we really do anything if the controller can't keep SDO high?
>
> There is one similar (ish) entry already.
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/spi/spi.h#L29
> #define SPI_3WIRE_HIZ _BITUL(15) /* high impedance turnaround */
> in that it is controlling state in what I think would normally be a don't care state.
>
> I think we could have an
> SPI_SDO_DONT_CARE_HIGH (naming to be improved upon ;)
> that a driver could advertise support for and the spi device could request.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-tpo-tpg110.c#L429
>
> Implement that in the spi-gpio driver as a PoC probably and in your SPI copntoller
> driver.
>
> Ultimately if the controller really isn't capable (including dances through pin
> mode changes if necessary) then the ADC won't work in this wiring with that host
> controller.
>
> I'd propose something along these lines and see whether Mark + any other active
> SPI folk think it is reasonable or not?
Sounds promising. Will try implement something like that.
>
> >
> > >
> > > Just to check, this isn't just a case of a missing pull up
> > > resistor to drag that line correctly when it isn't actively
> > > driven (combined with all 1s in the write buffer) etc?
> >
> > When using spi-engine, the controller SDO is connected to ADC SDI, controller
> > CS to ADC CNV and, for reg access, it works as conventional SPI.
> > spi-engine leaves the SDO line the state it was after last transfer so it we
> > can make it idle high during sample read. No hardware pull-up needed.
>
> Fair enough. No multi master support I guess (that is a bit obscure for
> SPI). A little ugly that it's dependent on the last access - so you would need
> to do a dummy access if the normal last bit was wrong level?
Seems I've been lucky with it but yes, we would need a dummy transfer to put
controller SDO line in the desired state. I'm thinking it should not be
difficult to make spi-engine SDO line always idle high (or idle in a
pre-configured state). I'll talk with the guys in the HDL team and what can be
done about it.
Thanks,
Marcelo
>
> Jonathan
>
> >
> > Marcelo
> >
> > >
> > > Jonathan
> > >
> > >
> >
> > The device tree source file I was using for testing with the ADC with the
> > changes for using pinctrl. Didn't really work.
> >
> > // SPDX-License-Identifier: GPL-2.0
> > /*
> > * Analog Devices ADAQ4003
> > * https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad400x
> > * https://wiki.analog.com/resources/eval/user-guides/ad400x
> > *
> > * hdl_project: <pulsar_adc_pmdz/zed>
> > * board_revision: <>
> > *
> > * Copyright (C) 2016-2023 Analog Devices Inc.
> > */
> > /dts-v1/;
> >
> > #include "zynq-zed.dtsi"
> > #include "zynq-zed-adv7511.dtsi"
> > #include <dt-bindings/pinctrl/pinctrl-zynq.h>
> >
> > / {
> > adc_vref: regulator-vref {
> > compatible = "regulator-fixed";
> > regulator-name = "EVAL 5V Vref";
> > regulator-min-microvolt = <5000000>;
> > regulator-max-microvolt = <5000000>;
> > regulator-always-on;
> > };
> >
> > adc_vdd: regulator-vdd {
> > compatible = "regulator-fixed";
> > regulator-name = "Eval VDD supply";
> > regulator-min-microvolt = <1800000>;
> > regulator-max-microvolt = <1800000>;
> > regulator-always-on;
> > };
> >
> > adc_vio: regulator-vio {
> > compatible = "regulator-fixed";
> > regulator-name = "Eval VIO supply";
> > regulator-min-microvolt = <3300000>;
> > regulator-max-microvolt = <3300000>;
> > regulator-always-on;
> > };
> > };
> >
> > &pinctrl0 {
> > /* Restore conventional SPI pin configuration */
> > pinctrl_spi_default: default_config {
> > mux {
> > /* Are these the ones used by spi-engine? */
> > groups = "spi0_0_grp";
> > function = "spi0";
> > };
> > conf {
> > groups = "spi0_0_grp";
> > power-source = <IO_STANDARD_LVCMOS33>;
> > };
> > conf-spi-sdo {
> > pins = "MIO17"; /* SPI0 SDO? */
> > bias-disable;
> > };
> > };
> >
> > /* Pull-up SPI SDO (ADC SDI) to VIO */
> > pinctrl_spi_single: single_config {
> > conf-spi-sdo {
> > pins = "MIO17"; /* Conflicts with SPI0 pin group */
> > bias-pull-up;
> > };
> > };
> > };
> >
> > &fpga_axi {
> > rx_dma: rx-dmac@44a30000 {
> > compatible = "adi,axi-dmac-1.00.a";
> > reg = <0x44a30000 0x1000>;
> > #dma-cells = <1>;
> > interrupts = <0 57 IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&clkc 17>;
> >
> > adi,channels {
> > #size-cells = <0>;
> > #address-cells = <1>;
> >
> > dma-channel@0 {
> > reg = <0>;
> > adi,source-bus-width = <32>;
> > adi,source-bus-type = <1>;
> > adi,destination-bus-width = <64>;
> > adi,destination-bus-type = <0>;
> > };
> > };
> > };
> >
> > axi_pwm_gen: axi-pwm-gen@ {
> > compatible = "adi,axi-pwmgen";
> > reg = <0x44b00000 0x1000>;
> > label = "cnv";
> > #pwm-cells = <2>;
> > clocks = <&spi_clk>;
> > };
> >
> > spi_clk: axi-clkgen@44a70000 {
> > compatible = "adi,axi-clkgen-2.00.a";
> > reg = <0x44a70000 0x10000>;
> > #clock-cells = <0>;
> > clocks = <&clkc 15>, <&clkc 15>;
> > clock-names = "s_axi_aclk", "clkin1";
> > clock-output-names = "spi_clk";
> > };
> >
> > axi_spi_engine_0: spi@44a00000 {
> > compatible = "adi,axi-spi-engine-1.00.a";
> > reg = <0x44a00000 0x1000>;
> > interrupt-parent = <&intc>;
> > interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&clkc 15 &spi_clk>;
> > clock-names = "s_axi_aclk", "spi_clk";
> > num-cs = <1>;
> >
> > #address-cells = <0x1>;
> > #size-cells = <0x0>;
> >
> > adaq4003: adc@0 {
> > compatible = "adi,adaq4003";
> > reg = <0>;
> > spi-max-frequency = <102000000>;
> > spi-cpha;
> > pinctrl-names = "default", "single";
> > pinctrl-0 = <&pinctrl_spi_default>;
> > pinctrl-1 = <&pinctrl_spi_single>;
> > vdd-supply = <&adc_vdd>;
> > vio-supply = <&adc_vio>;
> > ref-supply = <&adc_vref>;
> > adi,high-z-input;
> > adi,gain-milli = <454>;
> > };
> > };
> > };
>
