by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v1 11/17] iio: chemical: bme680: Use bulk reads for calibration data

On Mon, 3 Jun 2024 22:30:03 +0200
Vasileios Amoiridis <[email protected]> wrote:

> On Mon, Jun 03, 2024 at 08:25:37PM +0100, Jonathan Cameron wrote:
> > On Sun, 2 Jun 2024 21:30:23 +0200
> > Vasileios Amoiridis <[email protected]> wrote:
> >
> > > On Sun, Jun 02, 2024 at 01:57:26PM +0100, Jonathan Cameron wrote:
> > > > On Mon, 27 May 2024 20:37:59 +0200
> > > > Vasileios Amoiridis <[email protected]> wrote:
> > > >
> > > > > Calibration data are located in contiguous-ish registers
> > > > > inside the chip. For that reason we can use bulk reads as is
> > > > > done as well in the BME68x Sensor API [1].
> > > > >
> > > > > The arrays that are used for reading the data out of the sensor
> > > > > are located inside DMA safe buffer.
> > > >
> > > > See below. I think in this case that isn't necessary.
> > > > However it's a quirk of how the custom regmap works. Whilst
> > > > we can't rely on regmap core spi implementations continuing to
> > > > bounce buffer, we can rely on one local to our particular driver.
> > > >
> > >
> > > What about the I2C implementation though? I watched recently a video
> > > from Wolfram Sang [1] and as far as I understood, the buffers are not
> > > provided by the I2C API, but you have to provide them. In any case, I
> > > should maybe check both SPI and I2C reads to understand the internals.
> > >
> > > [1]: https://www.youtube.com/watch?v=JDwaMClvV-s
> > >
> >
> > I'm not sure Wolfram got far with his desire for generally avoiding the
> > bounce buffers for i2c. I think it's strictly opt in only so don't opt in
> > unless your code is safe for it and regmap never will by default as too
> > many drivers will be subtly broken.
> >
>
> The things that I found about DMA "safety" in I2C are [1] and [2] so I think
> that the IIO_DMA_MINALIGN should remain because in the future, in case it's
> needed for triggered buffers to do buffer reads from the volatile registers
> of the device, then it might be a problem for I2C.
>
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L2627
> [2]: https://elixir.bootlin.com/linux/latest/source/include/linux/i2c.h#L92
>

Those will remain opt in for a very long time if not for ever.

I suspect they will actually go away as a result of Catalin's
https://lore.kernel.org/linux-arm-kernel/[email protected]/
which bounces small or unaligned buffers and will apply to an annoying number of
IIO buffers even though they are actually safe because it's a heuristic
on the size part.

Today only a few architectures that do non coherent DMA use it though.
Will take the others opting in to the point where the config variable
goes away before we can stop this dance.

I don't know if anyone has yet looked at the impact on performance of that
change on the many drivers that have to work whether that is in place
or not and do small dma mappings. Maybe we need to teach bus drivers
when they need to map padding around an actually safe buffer that
doesn't look like it.

Jonathan

> >
> > > > >
> > > > > [1]: https://github.com/boschsensortec/BME68x_SensorAPI/blob/v4.4.8/bme68x.c#L1769
> > > > > Signed-off-by: Vasileios Amoiridis <[email protected]>
> > > >
> > > >
> > > > > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> > > > > index 681f271f9b06..ed4cdb4d64af 100644
> > > > > --- a/drivers/iio/chemical/bme680_core.c
> > > > > +++ b/drivers/iio/chemical/bme680_core.c
> > > >
> > > > > +
> > > > > struct bme680_calib {
> > > > > u16 par_t1;
> > > > > s16 par_t2;
> > > > > @@ -64,6 +109,16 @@ struct bme680_data {
> > > > > * and humidity compensation calculations.
> > > > > */
> > > > > s32 t_fine;
> > > > > +
> > > > > + /*
> > > > > + * DMA (thus cache coherency maintenance) may require the
> > > > > + * transfer buffers to live in their own cache lines.
> > > > > + */
> > > > > + union {
> > > > > + u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];
> > > > > + u8 bme680_cal_buf_2[BME680_CALIB_RANGE_2_LEN];
> > > > > + u8 bme680_cal_buf_3[BME680_CALIB_RANGE_3_LEN];
> > > > > + } __aligned(IIO_DMA_MINALIGN);
> > > > Ah! I should have read ahead. I don't think you need this alignment forcing
> > > > because bme680_regmap_spi_read uses spi_write_then_read() which always
> > > > bounces the data.
> > > >
> > >
> > > Same comment. What about I2C?
> > >
> > > > > };
> > > > >
> > > > > static const struct regmap_range bme680_volatile_ranges[] = {
> > > > > @@ -112,217 +167,73 @@ static int bme680_read_calib(struct bme680_data *data,
> > > > > struct bme680_calib *calib)
> > > > > {
> > > >
> > > >
> > > > > + calib->par_h3 = data->bme680_cal_buf_2[H3];
> > > > > + calib->par_h4 = data->bme680_cal_buf_2[H4];
> > > > > + calib->par_h5 = data->bme680_cal_buf_2[H5];
> > > > > + calib->par_h6 = data->bme680_cal_buf_2[H6];
> > > > > + calib->par_h7 = data->bme680_cal_buf_2[H7];
> > > > > + calib->par_t1 = get_unaligned_le16(&data->bme680_cal_buf_2[T1_LSB]);
> > > > > + calib->par_gh2 = get_unaligned_le16(&data->bme680_cal_buf_2[GH2_LSB]);
> > > > > + calib->par_gh1 = data->bme680_cal_buf_2[GH1];
> > > > > + calib->par_gh3 = data->bme680_cal_buf_2[GH3];
> > > > >
> > > > > - ret = regmap_read(data->regmap, BME680_H7_REG, &tmp);
> > > > > + ret = regmap_bulk_read(data->regmap, BME680_REG_RES_HEAT_VAL,
> > > > > + &data->bme680_cal_buf_3[0],
> > > > This one is always debated, but personally I'd prefer
> > > > data->bme680_cal_buf_3,
> > > >
> > >
> > > For me it's the same, I could change it to what you proposed, no problem!
> > >
> > > Cheers,
> > > Vasilis
> > >
> > > > for cases like this. Up to you though.
> > > > > + sizeof(data->bme680_cal_buf_3));
> > > > > if (ret < 0) {
> > > > > - dev_err(dev, "failed to read BME680_H7_REG\n");
> > > > > + dev_err(dev, "failed to read 3rd set of calib data;\n");
> > > > > return ret;
> > > > > }
> > > >
> >