ad400x: Support for pulsar HDL and STM32

ahaslam2 commented 5 months ago

Pull Request Description

This PR adds:

support for stm32 with IIO
changes for new HDL that contains:
- fixes for simple spi,
- pwm
- clkgen

HDL is not merged yet on main repo, but code was tested with: https://github.com/adi-ses/sea-misc/tree/main/AD4000-PRJ/debug_single_conversion

PR Type

[ ] Bug fix (change that fixes an issue)
[x] New feature (change that adds new functionality)
[ ] Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

[x] I have followed the Coding style guidelines
[x] I have performed a self-review of the changes
[x] I have commented my code, at least hard-to-understand parts
[x] I have build all projects affected by the changes in this PR
[ ] I have tested in hardware affected projects, at the relevant boards
[x] I have signed off all commits from this PR
[ ] I have updated the documentation (wiki pages, ReadMe etc), if applies

ahaslam2 commented 4 months ago

I still have to think a little bit about the spi chip select changes you have submitted, until then here are my remarks.

There is onother option with these chips is leave the CS go low, but this would require SDI to be high. this could be done by setting buf=0xFFFFFFFF before the spi operation.

ahaslam2 commented 4 months ago

changes in v2:

voltage ref patch squashed into iio/project patches. changed licence agreement text in iio driver expanded iio scan type table, and removed global scan_type variable fixed scale factor removed alloc from capture loop, used single 32 bit variable add driver remove on iio remove other minor fixes.

rbolboac commented 4 months ago

I still have to think a little bit about the spi chip select changes you have submitted, until then here are my remarks.

There is onother option with these chips is leave the CS go low, but this would require SDI to be high. this could be done by setting buf=0xFFFFFFFF before the spi operation.

So please correct me if I am wrong: all you actually need for this driver is to have chip-select active high instead of active low, is that right? If this is the case, I think it would be better to have an cs_active_high flag in the spi driver. the chip select is actual part of the SPI protocol and I find strange to make it optional in a spi driver. however, I think there are cases in which the polarity is inverted... this would ma a more acceptable approach.

ahaslam2 commented 4 months ago

I still have to think a little bit about the spi chip select changes you have submitted, until then here are my remarks.

There is onother option with these chips is leave the CS go low, but this would require SDI to be high. this could be done by setting buf=0xFFFFFFFF before the spi operation.

So please correct me if I am wrong: all you actually need for this driver is to have chip-select active high instead of active low, is that right? If this is the case, I think it would be better to have an cs_active_high flag in the spi driver. the chip select is actual part of the SPI protocol and I find strange to make it optional in a spi driver. however, I think there are cases in which the polarity is inverted... this would ma a more acceptable approach.

this is not exactly right,he chip does not have a CS pin. its a conversion pin.

the issue with just inverting the polarity of the CS pin on the spi driver, is that in fact to read/write the control register the CNV pin has to be low :( (just realized need to add a toggle in register read/write too)

if you want to avoid handling the gpio on the driver: the other option: setting buf to 0xFFFFFFFFF so SDI is high would allow for CS pin to be low and read samples. the issue with this is that the conversion starts on a rising edge of CNV, so if i read 1 sample for example: the sample would be from past: from when the last SPI read finished and CS toggled from low to high. would this be acceptable?

the above is what i see on the datasheet and from experiments: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf

ahaslam2 commented 4 months ago

changes v4:

toggle cnv gpio on register read/write
remove ref_voltage from driver init param
extend sign in case of signed channel

rbolboac commented 4 months ago

I still have to think a little bit about the spi chip select changes you have submitted, until then here are my remarks.

There is onother option with these chips is leave the CS go low, but this would require SDI to be high. this could be done by setting buf=0xFFFFFFFF before the spi operation.

So please correct me if I am wrong: all you actually need for this driver is to have chip-select active high instead of active low, is that right? If this is the case, I think it would be better to have an cs_active_high flag in the spi driver. the chip select is actual part of the SPI protocol and I find strange to make it optional in a spi driver. however, I think there are cases in which the polarity is inverted... this would ma a more acceptable approach.

this is not exactly right,he chip does not have a CS pin. its a conversion pin.

the issue with just inverting the polarity of the CS pin on the spi driver, is that in fact to read/write the control register the CNV pin has to be low :( (just realized need to add a toggle in register read/write too)

if you want to avoid handling the gpio on the driver: the other option: setting buf to 0xFFFFFFFFF so SDI is high would allow for CS pin to be low and read samples. the issue with this is that the conversion starts on a rising edge of CNV, so if i read 1 sample for example: the sample would be from past: from when the last SPI read finished and CS toggled from low to high. would this be acceptable?

the above is what i see on the datasheet and from experiments: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf

So we would like to avoid changing the SPI driver like that (seeing how for other platforms we will have to adapt this kind of behavior and it might not be possible to not have a chip select configured)... A workaround would be to simply have a dummy chip select in the project (which will not be connected to the chip) and then gpio_cnv pin can be used however is needed directly in the driver.

ahaslam2 commented 4 months ago

I still have to think a little bit about the spi chip select changes you have submitted, until then here are my remarks.

There is onother option with these chips is leave the CS go low, but this would require SDI to be high. this could be done by setting buf=0xFFFFFFFF before the spi operation.

So please correct me if I am wrong: all you actually need for this driver is to have chip-select active high instead of active low, is that right? If this is the case, I think it would be better to have an cs_active_high flag in the spi driver. the chip select is actual part of the SPI protocol and I find strange to make it optional in a spi driver. however, I think there are cases in which the polarity is inverted... this would ma a more acceptable approach.

this is not exactly right,he chip does not have a CS pin. its a conversion pin. the issue with just inverting the polarity of the CS pin on the spi driver, is that in fact to read/write the control register the CNV pin has to be low :( (just realized need to add a toggle in register read/write too) if you want to avoid handling the gpio on the driver: the other option: setting buf to 0xFFFFFFFFF so SDI is high would allow for CS pin to be low and read samples. the issue with this is that the conversion starts on a rising edge of CNV, so if i read 1 sample for example: the sample would be from past: from when the last SPI read finished and CS toggled from low to high. would this be acceptable? the above is what i see on the datasheet and from experiments: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf

So we would like to avoid changing the SPI driver like that (seeing how for other platforms we will have to adapt this kind of behavior and it might not be possible to not have a chip select configured)... A workaround would be to simply have a dummy chip select in the project (which will not be connected to the chip) and then gpio_cnv pin can be used however is needed directly in the driver.

ok, ill do that.

ahaslam2 commented 4 months ago

changes in v5:

dont mask high nibble of buffer (new)
fix sign of read_raw
return EINVAL instead of -1
fix parameter documentation.
fix ad4000 realbit size

ahaslam2 commented 4 months ago

it looks like there are still some things which are not clear. some things I want to point out:

the driver and the iio driver should be platform independent -> in this particular case it means that the ad400x_spi_single_conversion API should return exactly the same data format in both cases so the IIO driver can handle it correctly.

do you have the means to test this code? it should be tested and we should make sure the behavior is the same on both platforms. iio_info command can be used to compare the behavior

Currently there is no stand-alone HDL with iio project. its either xilinx HDL no IIO or stm32 standard SPI with IIO.

Since HDL and standard SPI are not returning the same byte order, we will need to do bit operation in ad400x_spi_single_conversion just for the get_raw attribute, which i wanted to avoid for the buffer data case. perhaps we need to change HDL to return the same as standard SPI? or create a new driver function for standard spi? perhaps ill do that but in the iio dirver there would still have to be a way to make the difference between the platforms

why not just add "#if defined(USE_STANDARD_SPI)" for these bit operations in the iio driver?

here is what the xilinx based platfrom is doing:

                while(1) {
                        ad400x_spi_single_conversion(dev, &adc_data);
                        xil_printf("ADC: %d\n\r", adc_data);
                }

i did test on the k1 (stm32) with the ad400x boards. for the bare metal xilinx platform (with HDL) we see from the code that they print the value as it comes so it must be little endiann. but i did not actually run this.

rbolboac commented 4 months ago

it looks like there are still some things which are not clear. some things I want to point out:

the driver and the iio driver should be platform independent -> in this particular case it means that the ad400x_spi_single_conversion API should return exactly the same data format in both cases so the IIO driver can handle it correctly.

do you have the means to test this code? it should be tested and we should make sure the behavior is the same on both platforms. iio_info command can be used to compare the behavior

Currently there is no stand-alone HDL with iio project. its either xilinx HDL no IIO or stm32 standard SPI with IIO.

Since HDL and standard SPI are not returning the same byte order, we will need to do bit operation in ad400x_spi_single_conversion just for the get_raw attribute, which i wanted to avoid for the buffer data case. perhaps we need to change HDL to return the same as standard SPI? or create a new driver function for standard spi? perhaps ill do that but in the iio dirver there would still have to be a way to make the difference between the platforms

why not just add "#if defined(USE_STANDARD_SPI)" for these bit operations in the iio driver?

here is what the xilinx based platfrom is doing:
                while(1) {
                        ad400x_spi_single_conversion(dev, &adc_data);
                        xil_printf("ADC: %d\n\r", adc_data);
                }
i did test on the k1 (stm32) with the ad400x boards. for the bare metal xilinx platform (with HDL) we see from the code that they print the value as it comes so it must be little endiann. but i did not actually run this.

the IIO driver should be platform independent... I understand why the driver is not (because SPI transactions are handled differently in HDL), but the result should be the same.

it does not really matter that there is no IIO project with HDL, if there would be one done, still the same iio driver would be used. so let's make ad400x_spi_single_conversion API return the same result, regardless of the platform, and handle it accordingly in IIO driver, even if the buffered data will take longer. then we do not need to make a difference between platform in the iio driver.

ahaslam2 commented 4 months ago

it looks like there are still some things which are not clear. some things I want to point out:

the driver and the iio driver should be platform independent -> in this particular case it means that the ad400x_spi_single_conversion API should return exactly the same data format in both cases so the IIO driver can handle it correctly.

do you have the means to test this code? it should be tested and we should make sure the behavior is the same on both platforms. iio_info command can be used to compare the behavior

Currently there is no stand-alone HDL with iio project. its either xilinx HDL no IIO or stm32 standard SPI with IIO. Since HDL and standard SPI are not returning the same byte order, we will need to do bit operation in ad400x_spi_single_conversion just for the get_raw attribute, which i wanted to avoid for the buffer data case. perhaps we need to change HDL to return the same as standard SPI? or create a new driver function for standard spi? perhaps ill do that but in the iio dirver there would still have to be a way to make the difference between the platforms why not just add "#if defined(USE_STANDARD_SPI)" for these bit operations in the iio driver? here is what the xilinx based platfrom is doing:
                while(1) {
                        ad400x_spi_single_conversion(dev, &adc_data);
                        xil_printf("ADC: %d\n\r", adc_data);
                }
i did test on the k1 (stm32) with the ad400x boards. for the bare metal xilinx platform (with HDL) we see from the code that they print the value as it comes so it must be little endiann. but i did not actually run this.
the IIO driver should be platform independent... I understand why the driver is not (because SPI transactions are handled differently in HDL), but the result should be the same.

it does not really matter that there is no IIO project with HDL, if there would be one done, still the same iio driver would be used. so let's make ad400x_spi_single_conversion API return the same result, regardless of the platform, and handle it accordingly in IIO driver, even if the buffered data will take longer. then we do not need to make a difference between platform in the iio driver.

Ok.. will do that, we are paying a performance impact per sample because HDL is flipping bytes.

ahaslam2 commented 3 months ago

it looks like there are still some things which are not clear. some things I want to point out:

the driver and the iio driver should be platform independent -> in this particular case it means that the ad400x_spi_single_conversion API should return exactly the same data format in both cases so the IIO driver can handle it correctly.

do you have the means to test this code? it should be tested and we should make sure the behavior is the same on both platforms. iio_info command can be used to compare the behavior

Currently there is no stand-alone HDL with iio project. its either xilinx HDL no IIO or stm32 standard SPI with IIO. Since HDL and standard SPI are not returning the same byte order, we will need to do bit operation in ad400x_spi_single_conversion just for the get_raw attribute, which i wanted to avoid for the buffer data case. perhaps we need to change HDL to return the same as standard SPI? or create a new driver function for standard spi? perhaps ill do that but in the iio dirver there would still have to be a way to make the difference between the platforms why not just add "#if defined(USE_STANDARD_SPI)" for these bit operations in the iio driver? here is what the xilinx based platfrom is doing:
                while(1) {
                        ad400x_spi_single_conversion(dev, &adc_data);
                        xil_printf("ADC: %d\n\r", adc_data);
                }
i did test on the k1 (stm32) with the ad400x boards. for the bare metal xilinx platform (with HDL) we see from the code that they print the value as it comes so it must be little endiann. but i did not actually run this.
the IIO driver should be platform independent... I understand why the driver is not (because SPI transactions are handled differently in HDL), but the result should be the same. it does not really matter that there is no IIO project with HDL, if there would be one done, still the same iio driver would be used. so let's make ad400x_spi_single_conversion API return the same result, regardless of the platform, and handle it accordingly in IIO driver, even if the buffered data will take longer. then we do not need to make a difference between platform in the iio driver.
Ok.. will do that, we are paying a performance impact per sample because HDL is flipping bytes.

For info, the HDL and driver is being worked on after the issue was raised: https://github.com/analogdevicesinc/no-OS/commit/5a56faeab4731b2a0df855b5e7be201af42b00a3

once those fixes are fixes are finalized and merged.

ahaslam2 commented 3 months ago

@amiclaus @rbolboac

The HDL was fixed for simple spi case, and this comes with mandatory support of PWM and CLKGEN. the HDL dev branch was: https://github.com/analogdevicesinc/no-OS/compare/update_ad400x_noos_spi_engine the HDL dev binary is https://github.com/adi-ses/sea-misc/tree/main/AD4000-PRJ/debug_single_conversion

This series now integrates the changes for this, so single conversion function works for both HDL and simple SPI.

the "new patches" are: projects: ad400x: Compile fix for new HDL drivers: ad400x: Fix xfer with when offload disabled drivers: ad400x: Fix single conversion transfer drivers: ad400x: Add clkgen and pwm

i tested in the following cases: zed + spi offload,
zed + spi no offload, k1 + simple spi

these parts: ad4000 (16bit unsigned) , ad4001 (16bit signed) ,
ad4002 (18bit unsigned) , ad4020 (20bit signed)

of those the only test that is failing right now is: ad4020 in the offload case.

I need to figure out this case (probably with the HDL team), while this goes on im updating this PR to perhaps get some feedback on the new changes,

ahaslam2 commented 2 months ago

@rbolboac ok, about your your suggestion to moving to the new format, ist it ok we first merge the changes as they are and then do the conversion in a separate PR?

rbolboac commented 2 months ago

@rbolboac ok, about your your suggestion to moving to the new format, ist it ok we first merge the changes as they are and then do the conversion in a separate PR?

I would say it is alright if it makes things easier for you... However, the CI build should be green before merging these changes.

ahaslam2 commented 2 months ago

@rbolboac

Projects were merged using the new directory structure.

Testing done: ad4020, ad4000, ad4001, ad4002.

zedboard: (uses the pulsar HDL that needs 400x support merged )

iio_example and basic_example
spi engine offload enabled and disabled

-stm32

iio_example using standard spi.
basic_example: not done, no print on uart console.

ahaslam2 commented 2 months ago

I dont know what this error is. Could someone explain why its failing:

-> Building iio_demo (iio_zed) -- xilinx -- ad40xx_fmc_zed -> Same hardware from last build, use existing bsp -> make update -> make clean -> make all -> DONE -> test -d latest_build/iio_demo/iio_demo_xilinx_iio_zed_ad40xx_fmc_zed || mkdir -p latest_build/iio_demo/iio_demo_xilinx_iio_zed_ad40xx_fmc_zed test -d latest_build/iio_demo/iio_demo_xilinx_iio_zed_ad40xx_fmc_zed -> cp /no-OS_builds/builds_main/build_xilinx_ad40xx_fmc_zed/bootgen_sysfiles.tar.gz latest_build/iio_demo/iio_demo_xilinx_iio_zed_ad40xx_fmc_zed -> cp /no-OS_builds/builds_main/build_xilinx_ad40xx_fmc_zed/output_boot_bin/BOOT.BIN latest_build/iio_demo/iio_demo_xilinx_iio_zed_ad40xx_fmc_zed -> rm -r latest_build/iio_demo Error occured: 1

[error]Bash exited with code '1'.

Finishing: Run project build

ahaslam2 commented 2 months ago

Fould out that the hardware field of the json file is what ties the CI to a particular HDL project. The error was due because the builds.json hardware was set to ad40xx. This needed to be changed to pulsar_adc that now has support for ad400x

ahaslam2 commented 2 months ago

The STM32 CI builds are failing. i dont know why. They build ok on my machine. is there any way we can see the log files to see why the fail happens

CI mentions that some more info might be at: See log logs/ad400x-fmcz_stm32_basic.txt See log logs/ad400x-fmcz_stm32_iio.txt

test -d latest_build/ad400x-fmcz
 -> Building ad400x-fmcz (basic) -- stm32 
 -> make reset
 -> make all
 -> Project first failed, rebuild to check if the error persists
 -> make reset
 -> make all
 -> Error persits, not a random fail, please check!
 -> ERROR
 -> See log logs/ad400x-fmcz_stm32_basic.txt -- Use cat (linux) or type (windows) to see colored output
 -> test -d latest_build/ad400x-fmcz || mkdir -p latest_build/ad400x-fmcz
test -d latest_build/ad400x-fmcz
 -> Building ad400x-fmcz (iio) -- stm32 
 -> make reset
 -> make all
 -> Project first failed, rebuild to check if the error persists
 -> make reset
 -> make all
 -> Error persits, not a random fail, please check!
 -> ERROR
 -> See log logs/ad400x-fmcz_stm32_iio.txt -- Use cat (linux) or type (windows) to see colored output

ahaslam2 commented 2 months ago

after discussion with @rbolboac The issue was due to recent push for spi_dma that requires dma includes for sm32 projects.

ahaslam2 commented 2 months ago

changes:

remove the pointer cast on spi read, this means a per sample copy, so return data same format as offload. (offload returns LE, chip data is BE)
add dev_info struct to hold per device sign and resolution properties.

ahaslam2 commented 1 month ago

Change log:

fix 2 commit message typos.
use 72 line indent for commit message.

ahaslam2 commented 1 month ago

changelog:

fixed white line in iio driver patch.

rbolboac commented 1 month ago

seeing how there is no ad400x driver documentation, do you plan on adding that here?

ahaslam2 commented 1 month ago

seeing how there is no ad400x driver documentation, do you plan on adding that here?

I added the driver README.

analogdevicesinc / no-OS