Message ID | 20250108-wip-bl-ad3552r-axi-v0-iio-testing-carlos-v2-8-2dac02f04638@baylibre.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | iio: ad3552r-hs: add support for ad3541/42r | expand |
On 1/8/25 11:29 AM, Angelo Dureghello wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > A new fpga HDL has been developed from ADI to support ad354xr s/fpga/FPGA/ > devices. > > Add support for ad3541r and ad3542r with following additions: > > - use common device_info structures for hs and non hs drivers, > - DMA buffering, use DSPI mode for ad354xr and QSPI for ad355xr, > - change samplerate to respect number of lanes. s/samplerate/sample rate/ > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > --- > drivers/iio/dac/ad3552r-common.c | 4 + > drivers/iio/dac/ad3552r-hs.c | 240 ++++++++++++++++++++++++++++++++------- > drivers/iio/dac/ad3552r.h | 3 + > 3 files changed, 206 insertions(+), 41 deletions(-) > > diff --git a/drivers/iio/dac/ad3552r-common.c b/drivers/iio/dac/ad3552r-common.c > index 36920ec95836..fa3050dc2efa 100644 > --- a/drivers/iio/dac/ad3552r-common.c > +++ b/drivers/iio/dac/ad3552r-common.c > @@ -44,6 +44,7 @@ const struct ad3552r_model_data ad3541r_model_data = { > .ranges_table = ad3542r_ch_ranges, > .num_ranges = ARRAY_SIZE(ad3542r_ch_ranges), > .requires_output_range = true, > + .num_spi_data_lanes = 2, > }; > EXPORT_SYMBOL_NS_GPL(ad3541r_model_data, "IIO_AD3552R"); > > @@ -54,6 +55,7 @@ const struct ad3552r_model_data ad3542r_model_data = { > .ranges_table = ad3542r_ch_ranges, > .num_ranges = ARRAY_SIZE(ad3542r_ch_ranges), > .requires_output_range = true, > + .num_spi_data_lanes = 2, > }; > EXPORT_SYMBOL_NS_GPL(ad3542r_model_data, "IIO_AD3552R"); > > @@ -64,6 +66,7 @@ const struct ad3552r_model_data ad3551r_model_data = { > .ranges_table = ad3552r_ch_ranges, > .num_ranges = ARRAY_SIZE(ad3552r_ch_ranges), > .requires_output_range = false, > + .num_spi_data_lanes = 4, > }; > EXPORT_SYMBOL_NS_GPL(ad3551r_model_data, "IIO_AD3552R"); > > @@ -74,6 +77,7 @@ const struct ad3552r_model_data ad3552r_model_data = { > .ranges_table = ad3552r_ch_ranges, > .num_ranges = ARRAY_SIZE(ad3552r_ch_ranges), > .requires_output_range = false, > + .num_spi_data_lanes = 4, > }; > EXPORT_SYMBOL_NS_GPL(ad3552r_model_data, "IIO_AD3552R"); > > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c > index bfb6228c9b9b..5995bab6a9b1 100644 > --- a/drivers/iio/dac/ad3552r-hs.c > +++ b/drivers/iio/dac/ad3552r-hs.c > @@ -19,6 +19,31 @@ > #include "ad3552r.h" > #include "ad3552r-hs.h" > > +/* > + * Important notes for register map access: > + * ======================================== > + * > + * Register address space is divided in 2 regions, primary (config) and > + * secondary (DAC). Primary region can only be accessed in simple SPI mode, > + * with exception for ad355x models where setting QSPI pin high allows QSPI > + * access to both the regions. > + * > + * Due to the fact that ad3541/2r do not implement QSPI, for proper device > + * detection, HDL keeps "QSPI" pin level low at boot (see ad3552r manual, rev B > + * table 7, pin 31, digital input). For this reason, actually the working mode > + * between SPI, DSPI and QSPI must be set via software, configuring the target > + * DAC appropriately, together with the backend api to configure the bus mode s/api/API/ > + * accordingly. > + * > + * Also, important to note that none of the three modes allow to read in DDR. > + * > + * In non-buffering operations, mode is set to simple SPI SDR for all primary > + * and secondary region r/w accesses, to avoid to switch the mode each time DAC > + * register is accessed (raw accesses, r/w), and to be able to dump registers > + * content (possible as non DDR only). > + * In buffering mode, driver sets best possible mode, D/QSPI and DDR. > + */ > + > struct ad3552r_hs_state { > const struct ad3552r_model_data *model_data; > struct gpio_desc *reset_gpio; > @@ -27,6 +52,8 @@ struct ad3552r_hs_state { > bool single_channel; > struct ad3552r_ch_data ch_data[AD3552R_MAX_CH]; > struct ad3552r_hs_platform_data *data; > + /* INTERFACE_CONFIG_D register cache, in DDR we cannot read values. */ > + u32 config_d; > }; > > static int ad3552r_qspi_update_reg_bits(struct ad3552r_hs_state *st, > @@ -56,15 +83,19 @@ static int ad3552r_hs_read_raw(struct iio_dev *indio_dev, > switch (mask) { > case IIO_CHAN_INFO_SAMP_FREQ: > /* > - * Using 4 lanes (QSPI), then using 2 as DDR mode is > - * considered always on (considering buffering mode always). > + * Using a "num_spi_data_lanes" variable since ad3541/2 have > + * only DSPI interface, while ad355x is QSPI. Then using 2 as > + * DDR mode is considered always on (considering buffering > + * mode always). > */ > *val = DIV_ROUND_CLOSEST(st->data->bus_sample_data_clock_hz * > - 4 * 2, chan->scan_type.realbits); > + st->model_data->num_spi_data_lanes * 2, > + chan->scan_type.realbits); > > return IIO_VAL_INT; > > case IIO_CHAN_INFO_RAW: > + /* For RAW accesses, stay always in simple-spi. */ > ret = st->data->bus_reg_read(st->back, > AD3552R_REG_ADDR_CH_DAC_16B(chan->channel), > val, 2); > @@ -93,6 +124,7 @@ static int ad3552r_hs_write_raw(struct iio_dev *indio_dev, > > switch (mask) { > case IIO_CHAN_INFO_RAW: > + /* For RAW accesses, stay always in simple-spi. */ > iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > return st->data->bus_reg_write(st->back, > AD3552R_REG_ADDR_CH_DAC_16B(chan->channel), > @@ -104,6 +136,42 @@ static int ad3552r_hs_write_raw(struct iio_dev *indio_dev, > } > } > > +static int ad3552r_hs_set_bus_io_mode_hs(struct ad3552r_hs_state *st) > +{ > + int bus_mode; > + > + if (st->model_data->num_spi_data_lanes == 4) > + bus_mode = AD3552R_IO_MODE_QSPI; > + else > + bus_mode = AD3552R_IO_MODE_DSPI; > + > + return st->data->bus_set_io_mode(st->back, bus_mode); > +} > + > +static int ad3552r_hs_set_target_io_mode_hs(struct ad3552r_hs_state *st) > +{ > + int mode_target; > + > + /* > + * Best access for secondary reg area, QSPI where possible, > + * else as DSPI. > + */ > + if (st->model_data->num_spi_data_lanes == 4) > + mode_target = AD3552R_QUAD_SPI; > + else > + mode_target = AD3552R_DUAL_SPI; > + > + /* > + * Better to not use update here, since generally it is already > + * set as DDR mode, and it's not possible to read in DDR mode. > + */ > + return st->data->bus_reg_write(st->back, > + AD3552R_REG_ADDR_TRANSFER_REGISTER, > + FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE, > + mode_target) | > + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); > +} > + > static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) > { > struct ad3552r_hs_state *st = iio_priv(indio_dev); > @@ -132,6 +200,11 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) > return -EINVAL; > } > > + /* > + * With ad3541/2r supoport, QSPI pin is held low at reset from HDL, s/supoport/support/ > + * streaming start sequence must respect strictly the order below. > + */ > + > /* Primary region access, set streaming mode (now in SPI + SDR). */ > ret = ad3552r_qspi_update_reg_bits(st, > AD3552R_REG_ADDR_INTERFACE_CONFIG_B, > @@ -139,48 +212,114 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) > if (ret) > return ret; > > - ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, > + /* > + * Set target loop len, 0x2c 0r 0x2a, descending loop, > + * and keeping loop len value so it's not cleared hereafter when > + * enabling streaming mode (cleared by CS_ up). rewrap to be closer to 80 chars/line > + */ > + ret = ad3552r_qspi_update_reg_bits(st, > + AD3552R_REG_ADDR_TRANSFER_REGISTER, > + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, > + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); > + if (ret) > + goto exit_err_streaming; > + > + ret = st->data->bus_reg_write(st->back, > + AD3552R_REG_ADDR_STREAM_MODE, > loop_len, 1); > if (ret) > - return ret; > + goto exit_err_streaming; > > - /* Inform DAC chip to switch into DDR mode */ > - ret = ad3552r_qspi_update_reg_bits(st, > - AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > - AD3552R_MASK_SPI_CONFIG_DDR, > - AD3552R_MASK_SPI_CONFIG_DDR, 1); > + /* > + * Registers dump for debug purposes is only possible until here, > + * read in primary region must be SPI SDR (DDR read is never possible, > + * D/QSPI SDR read in primary region is also not possible). > + */ It doesn't look like we have debugfs implemented in this driver, so this comment doesn't seem relent. And if we did, we could guard it with iio_device_claim_direct_mode() to avoid the issue, so we shouldn't need this comment in any case. > + > + /* Setting DDR now, caching current config_d. */ > + ret = st->data->bus_reg_read(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + &st->config_d, 1); > if (ret) > - return ret; > + goto exit_err_streaming; > + > + st->config_d |= AD3552R_MASK_SPI_CONFIG_DDR; > + ret = st->data->bus_reg_write(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + st->config_d, 1); > + > + if (ret) > + goto exit_err_streaming; > > - /* Inform DAC IP to go for DDR mode from now on */ > ret = iio_backend_ddr_enable(st->back); > - if (ret) { > - dev_err(st->dev, "could not set DDR mode, not streaming"); > - goto exit_err; > - } > + if (ret) > + goto exit_err_ddr_mode_target; > > + /* > + * From here onward mode is DDR, so reading any register is not > + * possible anymore, including calling "ad3552r_qspi_update_reg_bits" > + * function. > + */ > + > + /* Set target to best high speed mode (D or QSPI). */ > + ret = ad3552r_hs_set_target_io_mode_hs(st); > + if (ret) > + goto exit_err_ddr_mode; > + > + /* Set bus to best high speed mode (D or QSPI). */ > + ret = ad3552r_hs_set_bus_io_mode_hs(st); > + if (ret) > + goto exit_err_bus_mode_target; > + > + /* > + * Backend setup must be done now only, or related register values > + * will be disrupted by previous bus accesses. > + */ > ret = iio_backend_data_transfer_addr(st->back, val); > if (ret) > - goto exit_err; > + goto exit_err_bus_mode_target; > > ret = iio_backend_data_format_set(st->back, 0, &fmt); > if (ret) > - goto exit_err; > + goto exit_err_bus_mode_target; > > ret = iio_backend_data_stream_enable(st->back); > if (ret) > - goto exit_err; > + goto exit_err_bus_mode_target; > > return 0; > > -exit_err: > - ad3552r_qspi_update_reg_bits(st, > - AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > - AD3552R_MASK_SPI_CONFIG_DDR, > - 0, 1); > +exit_err_bus_mode_target: > + /* Back to simple SPI, not using update to avoid read. */ > + st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_TRANSFER_REGISTER, > + FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE, > + AD3552R_SPI) | > + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); > > + /* > + * Back bus to simple SPI, this must be executed together with above > + * target mode unwind, and can be done only after it. > + */ > + st->data->bus_set_io_mode(st->back, AD3552R_IO_MODE_SPI); > + > +exit_err_ddr_mode: > iio_backend_ddr_disable(st->back); > > +exit_err_ddr_mode_target: > + /* > + * Back to SDR. Odd place for line break. > + * In DDR we cannot read, whatever the mode is, so not using update. > + */ > + st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + FIELD_PREP(AD3552R_MASK_SDO_DRIVE_STRENGTH, 1), > + 1); > + > +exit_err_streaming: > + /* Back to single instruction mode, disabling loop. */ > + st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_INTERFACE_CONFIG_B, > + AD3552R_MASK_SINGLE_INST | > + AD3552R_MASK_SHORT_INSTRUCTION, 1); > + > return ret; > } > > @@ -193,11 +332,22 @@ static int ad3552r_hs_buffer_predisable(struct iio_dev *indio_dev) > if (ret) > return ret; > > - /* Inform DAC to set in SDR mode */ > - ret = ad3552r_qspi_update_reg_bits(st, > - AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > - AD3552R_MASK_SPI_CONFIG_DDR, > - 0, 1); > + /* > + * Set us to simple SPI, even if still in ddr, so to be able > + * to write in primary region. > + */ > + ret = st->data->bus_set_io_mode(st->back, AD3552R_IO_MODE_SPI); > + if (ret) > + return ret; > + > + /* > + * Back to SDR > + * (in DDR we cannot read, whatever the mode is, so not using update). > + */ > + st->config_d &= ~AD3552R_MASK_SPI_CONFIG_DDR; > + ret = st->data->bus_reg_write(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + st->config_d, 1); > if (ret) > return ret; > > @@ -205,11 +355,21 @@ static int ad3552r_hs_buffer_predisable(struct iio_dev *indio_dev) > if (ret) > return ret; > > - /* Back to single instruction mode, disabling loop. */ > + /* > + * Back to simple SPI for secondary region too now, > + * so to be able to dump/read registers there too if needed. > + */ > ret = ad3552r_qspi_update_reg_bits(st, > - AD3552R_REG_ADDR_INTERFACE_CONFIG_B, > - AD3552R_MASK_SINGLE_INST, > - AD3552R_MASK_SINGLE_INST, 1); > + AD3552R_REG_ADDR_TRANSFER_REGISTER, > + AD3552R_MASK_MULTI_IO_MODE, > + AD3552R_SPI, 1); > + if (ret) > + return ret; > + > + /* Back to single instruction mode, disabling loop. */ > + ret = ad3552r_update_reg_bits(st, AD3552R_REG_ADDR_INTERFACE_CONFIG_B, I think this commit will fail to compile with this rename. ad3552r_qspi_update_reg_bits -> ad3552r_update_reg_bits This functinon isn't renamed until the next patch. > + AD3552R_MASK_SINGLE_INST, > + AD3552R_MASK_SINGLE_INST, 1); > if (ret) > return ret; > > @@ -319,6 +479,7 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) > if (ret) > return ret; > > + /* HDL starts with DDR enabled, disabling it. */ > ret = iio_backend_ddr_disable(st->back); > if (ret) > return ret; > @@ -352,6 +513,8 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) > "Chip ID mismatch, detected 0x%x but expected 0x%x\n", > id, st->model_data->chip_id); > > + dev_info(st->dev, "chip id %s detected", st->model_data->model_name); IIRC, Jonathan suggested dev_dbg() here. > + > /* Clear reset error flag, see ad3552r manual, rev B table 38. */ > ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_ERR_STATUS, > AD3552R_MASK_RESET_STATUS, 1); > @@ -364,14 +527,6 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) > if (ret) > return ret; > > - ret = st->data->bus_reg_write(st->back, > - AD3552R_REG_ADDR_TRANSFER_REGISTER, > - FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE, > - AD3552R_QUAD_SPI) | > - AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); > - if (ret) > - return ret; > - > ret = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL); > if (ret) > return ret; > @@ -528,6 +683,9 @@ static int ad3552r_hs_probe(struct platform_device *pdev) > } > > static const struct of_device_id ad3552r_hs_of_id[] = { > + { .compatible = "adi,ad3541r", .data = &ad3541r_model_data }, > + { .compatible = "adi,ad3542r", .data = &ad3542r_model_data }, > + { .compatible = "adi,ad3551r", .data = &ad3551r_model_data }, > { .compatible = "adi,ad3552r", .data = &ad3552r_model_data }, > { } > }; > diff --git a/drivers/iio/dac/ad3552r.h b/drivers/iio/dac/ad3552r.h > index 23f038464a0c..9d450019ece9 100644 > --- a/drivers/iio/dac/ad3552r.h > +++ b/drivers/iio/dac/ad3552r.h > @@ -132,6 +132,8 @@ > > #define AD3552R_MAX_RANGES 5 > #define AD3542R_MAX_RANGES 5 > +#define AD3552R_SPI 0 > +#define AD3552R_DUAL_SPI 1 > #define AD3552R_QUAD_SPI 2 > > extern const s32 ad3552r_ch_ranges[AD3552R_MAX_RANGES][2]; > @@ -156,6 +158,7 @@ struct ad3552r_model_data { > const s32 (*ranges_table)[2]; > int num_ranges; > bool requires_output_range; > + int num_spi_data_lanes; > }; > > struct ad3552r_ch_data { >
On Wed, 08 Jan 2025 18:29:22 +0100 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > A new fpga HDL has been developed from ADI to support ad354xr > devices. > > Add support for ad3541r and ad3542r with following additions: > > - use common device_info structures for hs and non hs drivers, > - DMA buffering, use DSPI mode for ad354xr and QSPI for ad355xr, > - change samplerate to respect number of lanes. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> One question inline. I also wonder if you can easily add checks that mean any spurious (bug) read in DDR mode would get an error print to tell who ever triggered it what went wrong. Jonathan > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c > index bfb6228c9b9b..5995bab6a9b1 100644 > --- a/drivers/iio/dac/ad3552r-hs.c > +++ b/drivers/iio/dac/ad3552r-hs.c > + > +static int ad3552r_hs_set_target_io_mode_hs(struct ad3552r_hs_state *st) > +{ > + int mode_target; > + > + /* > + * Best access for secondary reg area, QSPI where possible, > + * else as DSPI. > + */ > + if (st->model_data->num_spi_data_lanes == 4) > + mode_target = AD3552R_QUAD_SPI; > + else > + mode_target = AD3552R_DUAL_SPI; > + > + /* > + * Better to not use update here, since generally it is already > + * set as DDR mode, and it's not possible to read in DDR mode. > + */ > + return st->data->bus_reg_write(st->back, > + AD3552R_REG_ADDR_TRANSFER_REGISTER, > + FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE, > + mode_target) | > + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); > +} > @@ -319,6 +479,7 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) > if (ret) > return ret; > > + /* HDL starts with DDR enabled, disabling it. */ > ret = iio_backend_ddr_disable(st->back); > if (ret) > return ret; > @@ -352,6 +513,8 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) > "Chip ID mismatch, detected 0x%x but expected 0x%x\n", > id, st->model_data->chip_id); > > + dev_info(st->dev, "chip id %s detected", st->model_data->model_name); > + > /* Clear reset error flag, see ad3552r manual, rev B table 38. */ > ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_ERR_STATUS, > AD3552R_MASK_RESET_STATUS, 1); > @@ -364,14 +527,6 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) > if (ret) > return ret; > > - ret = st->data->bus_reg_write(st->back, > - AD3552R_REG_ADDR_TRANSFER_REGISTER, > - FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE, > - AD3552R_QUAD_SPI) | > - AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); > - if (ret) > - return ret; > - This is the call you just added to ensure we end up in instruction mode. I'm not seeing another place it is now called so is this an accidental revert? If it is intentional then break out a patch that deals with that change before the addition of new parts. I was expecting to see a call to _set_target_io_mode_hs. > ret = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL); > if (ret) > return ret; > @@ -528,6 +683,9 @@ static int ad3552r_hs_probe(struct platform_device *pdev) > }
On 12.01.2025 14:36, Jonathan Cameron wrote: > On Wed, 08 Jan 2025 18:29:22 +0100 > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > A new fpga HDL has been developed from ADI to support ad354xr > > devices. > > > > Add support for ad3541r and ad3542r with following additions: > > > > - use common device_info structures for hs and non hs drivers, > > - DMA buffering, use DSPI mode for ad354xr and QSPI for ad355xr, > > - change samplerate to respect number of lanes. > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > One question inline. I also wonder if you can easily add checks > that mean any spurious (bug) read in DDR mode would get an error > print to tell who ever triggered it what went wrong. > > Jonathan > > > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c > > index bfb6228c9b9b..5995bab6a9b1 100644 > > --- a/drivers/iio/dac/ad3552r-hs.c > > +++ b/drivers/iio/dac/ad3552r-hs.c > > > + > > +static int ad3552r_hs_set_target_io_mode_hs(struct ad3552r_hs_state *st) > > +{ > > + int mode_target; > > + > > + /* > > + * Best access for secondary reg area, QSPI where possible, > > + * else as DSPI. > > + */ > > + if (st->model_data->num_spi_data_lanes == 4) > > + mode_target = AD3552R_QUAD_SPI; > > + else > > + mode_target = AD3552R_DUAL_SPI; > > + > > + /* > > + * Better to not use update here, since generally it is already > > + * set as DDR mode, and it's not possible to read in DDR mode. > > + */ > > + return st->data->bus_reg_write(st->back, > > + AD3552R_REG_ADDR_TRANSFER_REGISTER, > > + FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE, > > + mode_target) | > > + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); > > +} > > > @@ -319,6 +479,7 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) > > if (ret) > > return ret; > > > > + /* HDL starts with DDR enabled, disabling it. */ > > ret = iio_backend_ddr_disable(st->back); > > if (ret) > > return ret; > > @@ -352,6 +513,8 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) > > "Chip ID mismatch, detected 0x%x but expected 0x%x\n", > > id, st->model_data->chip_id); > > > > + dev_info(st->dev, "chip id %s detected", st->model_data->model_name); > > + > > /* Clear reset error flag, see ad3552r manual, rev B table 38. */ > > ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_ERR_STATUS, > > AD3552R_MASK_RESET_STATUS, 1); > > @@ -364,14 +527,6 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) > > if (ret) > > return ret; > > > > - ret = st->data->bus_reg_write(st->back, > > - AD3552R_REG_ADDR_TRANSFER_REGISTER, > > - FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE, > > - AD3552R_QUAD_SPI) | > > - AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); > > - if (ret) > > - return ret; > > - > This is the call you just added to ensure we end up in instruction mode. > I'm not seeing another place it is now called so is this an accidental revert? > If it is intentional then break out a patch that deals with that change > before the addition of new parts. > > I was expecting to see a call to _set_target_io_mode_hs. > > The above was only related the IO mode, it was set for QSPI. Now the mode is kept as simple SPI for all the configurations, moving to DSPI or QSPI later for streaming. > > > ret = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL); > > if (ret) > > return ret; > > @@ -528,6 +683,9 @@ static int ad3552r_hs_probe(struct platform_device *pdev) > > }
diff --git a/drivers/iio/dac/ad3552r-common.c b/drivers/iio/dac/ad3552r-common.c index 36920ec95836..fa3050dc2efa 100644 --- a/drivers/iio/dac/ad3552r-common.c +++ b/drivers/iio/dac/ad3552r-common.c @@ -44,6 +44,7 @@ const struct ad3552r_model_data ad3541r_model_data = { .ranges_table = ad3542r_ch_ranges, .num_ranges = ARRAY_SIZE(ad3542r_ch_ranges), .requires_output_range = true, + .num_spi_data_lanes = 2, }; EXPORT_SYMBOL_NS_GPL(ad3541r_model_data, "IIO_AD3552R"); @@ -54,6 +55,7 @@ const struct ad3552r_model_data ad3542r_model_data = { .ranges_table = ad3542r_ch_ranges, .num_ranges = ARRAY_SIZE(ad3542r_ch_ranges), .requires_output_range = true, + .num_spi_data_lanes = 2, }; EXPORT_SYMBOL_NS_GPL(ad3542r_model_data, "IIO_AD3552R"); @@ -64,6 +66,7 @@ const struct ad3552r_model_data ad3551r_model_data = { .ranges_table = ad3552r_ch_ranges, .num_ranges = ARRAY_SIZE(ad3552r_ch_ranges), .requires_output_range = false, + .num_spi_data_lanes = 4, }; EXPORT_SYMBOL_NS_GPL(ad3551r_model_data, "IIO_AD3552R"); @@ -74,6 +77,7 @@ const struct ad3552r_model_data ad3552r_model_data = { .ranges_table = ad3552r_ch_ranges, .num_ranges = ARRAY_SIZE(ad3552r_ch_ranges), .requires_output_range = false, + .num_spi_data_lanes = 4, }; EXPORT_SYMBOL_NS_GPL(ad3552r_model_data, "IIO_AD3552R"); diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c index bfb6228c9b9b..5995bab6a9b1 100644 --- a/drivers/iio/dac/ad3552r-hs.c +++ b/drivers/iio/dac/ad3552r-hs.c @@ -19,6 +19,31 @@ #include "ad3552r.h" #include "ad3552r-hs.h" +/* + * Important notes for register map access: + * ======================================== + * + * Register address space is divided in 2 regions, primary (config) and + * secondary (DAC). Primary region can only be accessed in simple SPI mode, + * with exception for ad355x models where setting QSPI pin high allows QSPI + * access to both the regions. + * + * Due to the fact that ad3541/2r do not implement QSPI, for proper device + * detection, HDL keeps "QSPI" pin level low at boot (see ad3552r manual, rev B + * table 7, pin 31, digital input). For this reason, actually the working mode + * between SPI, DSPI and QSPI must be set via software, configuring the target + * DAC appropriately, together with the backend api to configure the bus mode + * accordingly. + * + * Also, important to note that none of the three modes allow to read in DDR. + * + * In non-buffering operations, mode is set to simple SPI SDR for all primary + * and secondary region r/w accesses, to avoid to switch the mode each time DAC + * register is accessed (raw accesses, r/w), and to be able to dump registers + * content (possible as non DDR only). + * In buffering mode, driver sets best possible mode, D/QSPI and DDR. + */ + struct ad3552r_hs_state { const struct ad3552r_model_data *model_data; struct gpio_desc *reset_gpio; @@ -27,6 +52,8 @@ struct ad3552r_hs_state { bool single_channel; struct ad3552r_ch_data ch_data[AD3552R_MAX_CH]; struct ad3552r_hs_platform_data *data; + /* INTERFACE_CONFIG_D register cache, in DDR we cannot read values. */ + u32 config_d; }; static int ad3552r_qspi_update_reg_bits(struct ad3552r_hs_state *st, @@ -56,15 +83,19 @@ static int ad3552r_hs_read_raw(struct iio_dev *indio_dev, switch (mask) { case IIO_CHAN_INFO_SAMP_FREQ: /* - * Using 4 lanes (QSPI), then using 2 as DDR mode is - * considered always on (considering buffering mode always). + * Using a "num_spi_data_lanes" variable since ad3541/2 have + * only DSPI interface, while ad355x is QSPI. Then using 2 as + * DDR mode is considered always on (considering buffering + * mode always). */ *val = DIV_ROUND_CLOSEST(st->data->bus_sample_data_clock_hz * - 4 * 2, chan->scan_type.realbits); + st->model_data->num_spi_data_lanes * 2, + chan->scan_type.realbits); return IIO_VAL_INT; case IIO_CHAN_INFO_RAW: + /* For RAW accesses, stay always in simple-spi. */ ret = st->data->bus_reg_read(st->back, AD3552R_REG_ADDR_CH_DAC_16B(chan->channel), val, 2); @@ -93,6 +124,7 @@ static int ad3552r_hs_write_raw(struct iio_dev *indio_dev, switch (mask) { case IIO_CHAN_INFO_RAW: + /* For RAW accesses, stay always in simple-spi. */ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { return st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_CH_DAC_16B(chan->channel), @@ -104,6 +136,42 @@ static int ad3552r_hs_write_raw(struct iio_dev *indio_dev, } } +static int ad3552r_hs_set_bus_io_mode_hs(struct ad3552r_hs_state *st) +{ + int bus_mode; + + if (st->model_data->num_spi_data_lanes == 4) + bus_mode = AD3552R_IO_MODE_QSPI; + else + bus_mode = AD3552R_IO_MODE_DSPI; + + return st->data->bus_set_io_mode(st->back, bus_mode); +} + +static int ad3552r_hs_set_target_io_mode_hs(struct ad3552r_hs_state *st) +{ + int mode_target; + + /* + * Best access for secondary reg area, QSPI where possible, + * else as DSPI. + */ + if (st->model_data->num_spi_data_lanes == 4) + mode_target = AD3552R_QUAD_SPI; + else + mode_target = AD3552R_DUAL_SPI; + + /* + * Better to not use update here, since generally it is already + * set as DDR mode, and it's not possible to read in DDR mode. + */ + return st->data->bus_reg_write(st->back, + AD3552R_REG_ADDR_TRANSFER_REGISTER, + FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE, + mode_target) | + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); +} + static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) { struct ad3552r_hs_state *st = iio_priv(indio_dev); @@ -132,6 +200,11 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) return -EINVAL; } + /* + * With ad3541/2r supoport, QSPI pin is held low at reset from HDL, + * streaming start sequence must respect strictly the order below. + */ + /* Primary region access, set streaming mode (now in SPI + SDR). */ ret = ad3552r_qspi_update_reg_bits(st, AD3552R_REG_ADDR_INTERFACE_CONFIG_B, @@ -139,48 +212,114 @@ static int ad3552r_hs_buffer_postenable(struct iio_dev *indio_dev) if (ret) return ret; - ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, + /* + * Set target loop len, 0x2c 0r 0x2a, descending loop, + * and keeping loop len value so it's not cleared hereafter when + * enabling streaming mode (cleared by CS_ up). + */ + ret = ad3552r_qspi_update_reg_bits(st, + AD3552R_REG_ADDR_TRANSFER_REGISTER, + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); + if (ret) + goto exit_err_streaming; + + ret = st->data->bus_reg_write(st->back, + AD3552R_REG_ADDR_STREAM_MODE, loop_len, 1); if (ret) - return ret; + goto exit_err_streaming; - /* Inform DAC chip to switch into DDR mode */ - ret = ad3552r_qspi_update_reg_bits(st, - AD3552R_REG_ADDR_INTERFACE_CONFIG_D, - AD3552R_MASK_SPI_CONFIG_DDR, - AD3552R_MASK_SPI_CONFIG_DDR, 1); + /* + * Registers dump for debug purposes is only possible until here, + * read in primary region must be SPI SDR (DDR read is never possible, + * D/QSPI SDR read in primary region is also not possible). + */ + + /* Setting DDR now, caching current config_d. */ + ret = st->data->bus_reg_read(st->back, + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + &st->config_d, 1); if (ret) - return ret; + goto exit_err_streaming; + + st->config_d |= AD3552R_MASK_SPI_CONFIG_DDR; + ret = st->data->bus_reg_write(st->back, + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + st->config_d, 1); + + if (ret) + goto exit_err_streaming; - /* Inform DAC IP to go for DDR mode from now on */ ret = iio_backend_ddr_enable(st->back); - if (ret) { - dev_err(st->dev, "could not set DDR mode, not streaming"); - goto exit_err; - } + if (ret) + goto exit_err_ddr_mode_target; + /* + * From here onward mode is DDR, so reading any register is not + * possible anymore, including calling "ad3552r_qspi_update_reg_bits" + * function. + */ + + /* Set target to best high speed mode (D or QSPI). */ + ret = ad3552r_hs_set_target_io_mode_hs(st); + if (ret) + goto exit_err_ddr_mode; + + /* Set bus to best high speed mode (D or QSPI). */ + ret = ad3552r_hs_set_bus_io_mode_hs(st); + if (ret) + goto exit_err_bus_mode_target; + + /* + * Backend setup must be done now only, or related register values + * will be disrupted by previous bus accesses. + */ ret = iio_backend_data_transfer_addr(st->back, val); if (ret) - goto exit_err; + goto exit_err_bus_mode_target; ret = iio_backend_data_format_set(st->back, 0, &fmt); if (ret) - goto exit_err; + goto exit_err_bus_mode_target; ret = iio_backend_data_stream_enable(st->back); if (ret) - goto exit_err; + goto exit_err_bus_mode_target; return 0; -exit_err: - ad3552r_qspi_update_reg_bits(st, - AD3552R_REG_ADDR_INTERFACE_CONFIG_D, - AD3552R_MASK_SPI_CONFIG_DDR, - 0, 1); +exit_err_bus_mode_target: + /* Back to simple SPI, not using update to avoid read. */ + st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_TRANSFER_REGISTER, + FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE, + AD3552R_SPI) | + AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); + /* + * Back bus to simple SPI, this must be executed together with above + * target mode unwind, and can be done only after it. + */ + st->data->bus_set_io_mode(st->back, AD3552R_IO_MODE_SPI); + +exit_err_ddr_mode: iio_backend_ddr_disable(st->back); +exit_err_ddr_mode_target: + /* + * Back to SDR. + * In DDR we cannot read, whatever the mode is, so not using update. + */ + st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + FIELD_PREP(AD3552R_MASK_SDO_DRIVE_STRENGTH, 1), + 1); + +exit_err_streaming: + /* Back to single instruction mode, disabling loop. */ + st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_INTERFACE_CONFIG_B, + AD3552R_MASK_SINGLE_INST | + AD3552R_MASK_SHORT_INSTRUCTION, 1); + return ret; } @@ -193,11 +332,22 @@ static int ad3552r_hs_buffer_predisable(struct iio_dev *indio_dev) if (ret) return ret; - /* Inform DAC to set in SDR mode */ - ret = ad3552r_qspi_update_reg_bits(st, - AD3552R_REG_ADDR_INTERFACE_CONFIG_D, - AD3552R_MASK_SPI_CONFIG_DDR, - 0, 1); + /* + * Set us to simple SPI, even if still in ddr, so to be able + * to write in primary region. + */ + ret = st->data->bus_set_io_mode(st->back, AD3552R_IO_MODE_SPI); + if (ret) + return ret; + + /* + * Back to SDR + * (in DDR we cannot read, whatever the mode is, so not using update). + */ + st->config_d &= ~AD3552R_MASK_SPI_CONFIG_DDR; + ret = st->data->bus_reg_write(st->back, + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + st->config_d, 1); if (ret) return ret; @@ -205,11 +355,21 @@ static int ad3552r_hs_buffer_predisable(struct iio_dev *indio_dev) if (ret) return ret; - /* Back to single instruction mode, disabling loop. */ + /* + * Back to simple SPI for secondary region too now, + * so to be able to dump/read registers there too if needed. + */ ret = ad3552r_qspi_update_reg_bits(st, - AD3552R_REG_ADDR_INTERFACE_CONFIG_B, - AD3552R_MASK_SINGLE_INST, - AD3552R_MASK_SINGLE_INST, 1); + AD3552R_REG_ADDR_TRANSFER_REGISTER, + AD3552R_MASK_MULTI_IO_MODE, + AD3552R_SPI, 1); + if (ret) + return ret; + + /* Back to single instruction mode, disabling loop. */ + ret = ad3552r_update_reg_bits(st, AD3552R_REG_ADDR_INTERFACE_CONFIG_B, + AD3552R_MASK_SINGLE_INST, + AD3552R_MASK_SINGLE_INST, 1); if (ret) return ret; @@ -319,6 +479,7 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) if (ret) return ret; + /* HDL starts with DDR enabled, disabling it. */ ret = iio_backend_ddr_disable(st->back); if (ret) return ret; @@ -352,6 +513,8 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) "Chip ID mismatch, detected 0x%x but expected 0x%x\n", id, st->model_data->chip_id); + dev_info(st->dev, "chip id %s detected", st->model_data->model_name); + /* Clear reset error flag, see ad3552r manual, rev B table 38. */ ret = st->data->bus_reg_write(st->back, AD3552R_REG_ADDR_ERR_STATUS, AD3552R_MASK_RESET_STATUS, 1); @@ -364,14 +527,6 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st) if (ret) return ret; - ret = st->data->bus_reg_write(st->back, - AD3552R_REG_ADDR_TRANSFER_REGISTER, - FIELD_PREP(AD3552R_MASK_MULTI_IO_MODE, - AD3552R_QUAD_SPI) | - AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE, 1); - if (ret) - return ret; - ret = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL); if (ret) return ret; @@ -528,6 +683,9 @@ static int ad3552r_hs_probe(struct platform_device *pdev) } static const struct of_device_id ad3552r_hs_of_id[] = { + { .compatible = "adi,ad3541r", .data = &ad3541r_model_data }, + { .compatible = "adi,ad3542r", .data = &ad3542r_model_data }, + { .compatible = "adi,ad3551r", .data = &ad3551r_model_data }, { .compatible = "adi,ad3552r", .data = &ad3552r_model_data }, { } }; diff --git a/drivers/iio/dac/ad3552r.h b/drivers/iio/dac/ad3552r.h index 23f038464a0c..9d450019ece9 100644 --- a/drivers/iio/dac/ad3552r.h +++ b/drivers/iio/dac/ad3552r.h @@ -132,6 +132,8 @@ #define AD3552R_MAX_RANGES 5 #define AD3542R_MAX_RANGES 5 +#define AD3552R_SPI 0 +#define AD3552R_DUAL_SPI 1 #define AD3552R_QUAD_SPI 2 extern const s32 ad3552r_ch_ranges[AD3552R_MAX_RANGES][2]; @@ -156,6 +158,7 @@ struct ad3552r_model_data { const s32 (*ranges_table)[2]; int num_ranges; bool requires_output_range; + int num_spi_data_lanes; }; struct ad3552r_ch_data {