| Message ID | 20210824104041.708945-4-tanureal@opensource.cirrus.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve support for AMD SPI controllers | expand |
On Tue, Aug 24, 2021 at 11:40:35AM +0100, Lucas Tanure wrote: > regmap-spi will split data and address between two transfers in the > same message so use addr_affects_max_raw_rw to flag that the number > bytes to read or write should be a little less (address + padding size), > so that the SPI controller can merge the entire message into a single > CS period This should be handled by the SPI core, it's already relying on being able to do multiple transfers to handle message size limits and in any case this is a super standard thing to do so many clients would require special code. The core should transparently coalesce things where it can, or error out if it can't, like it currently does when splitting transfers up.
On 8/24/21 5:37 PM, Mark Brown wrote: > On Tue, Aug 24, 2021 at 11:40:35AM +0100, Lucas Tanure wrote: >> regmap-spi will split data and address between two transfers in the >> same message so use addr_affects_max_raw_rw to flag that the number >> bytes to read or write should be a little less (address + padding size), >> so that the SPI controller can merge the entire message into a single >> CS period > > This should be handled by the SPI core, it's already relying on being > able to do multiple transfers to handle message size limits and in any > case this is a super standard thing to do so many clients would require > special code. The core should transparently coalesce things where it > can, or error out if it can't, like it currently does when splitting > transfers up. > __spi_validate seems a good candidate, but I don't think spi have enough information to merge two transfers into a single one. For a message with N transfers how can spi core decide what to merge or what not merge. If mergers everything and is less than max_transfer_size success, but if bigger will need to stop merging and add an address in front of the next not merged transfer, but spi core is not aware of addresses And in the case of multiple addresses and data transfers, how it will know doesn't need to be merged? For me seems more reasonable for the regmap-spi stop splitting address and data. Or at least if the controller has some flag change the bus for one where it uses different functions for gather_write, async_write etc Can you point which way you think the code should go? Investigate more spi core to coalesce transfers or change regmap-spi to not split address and data anymore? Thanks Lucas
On Wed, Aug 25, 2021 at 06:13:01PM +0100, Lucas tanure wrote: > On 8/24/21 5:37 PM, Mark Brown wrote: > > This should be handled by the SPI core, it's already relying on being > > able to do multiple transfers to handle message size limits and in any > > case this is a super standard thing to do so many clients would require > For a message with N transfers how can spi core decide what to merge or what > not merge. If mergers everything and is less than max_transfer_size success, In the same way it does for transfers that are too long. If the controller has a property saying that it can't handle more than one transfer then the core needs to either combine multiple transfers in a single message into a single transfer or return an error to the caller (modulo handling of cs_change). If the controller can handle the message it should just get passed straight through. > but if bigger will need to stop merging and add an address in front of the > next not merged transfer, but spi core is not aware of addresses > And in the case of multiple addresses and data transfers, how it will know > doesn't need to be merged? The spi_message says what the message should look like on the bus. The semantics of what's in the message don't matter. > For me seems more reasonable for the regmap-spi stop splitting address > and data. Or at least if the controller has some flag change the bus for > one where it uses different functions for gather_write, async_write etc This would force us to marshall the data in memory prior to sending which adds overhead. > Can you point which way you think the code should go? Investigate more spi > core to coalesce transfers or change regmap-spi to not split address and > data anymore? Like I said in reply to your driver patch it looks like this fundamentally doesn't do what you want in the first place.
diff --git a/drivers/base/regmap/regmap-spi.c b/drivers/base/regmap/regmap-spi.c index 603a4c1c2066..0c1f2e51c0c7 100644 --- a/drivers/base/regmap/regmap-spi.c +++ b/drivers/base/regmap/regmap-spi.c @@ -115,12 +115,22 @@ static const struct regmap_bus *regmap_get_spi_bus(struct spi_device *spi, struct spi_master *master = spi->master; struct regmap_bus *bus = NULL; - if (master->max_transfer_size) { + if (master->max_transfer_size || (master->flags & SPI_CONTROLLER_CS_PER_TRANSFER)) { bus = kmemdup(®map_spi, sizeof(*bus), GFP_KERNEL); if (!bus) return ERR_PTR(-ENOMEM); bus->free_on_exit = true; - bus->max_raw_read = bus->max_raw_write = master->max_transfer_size(spi); + + /* regmap-spi will split data and address between two transfers in the same message + * so use addr_affects_max_raw_rw to flag that the number bytes to read or write + * should be a little less (address + padding size), so the controller can + * fit both transfers in a single CS period + */ + bus->addr_affects_max_raw_rw = master->flags & SPI_CONTROLLER_CS_PER_TRANSFER; + + if (master->max_transfer_size) + bus->max_raw_read = bus->max_raw_write = master->max_transfer_size(spi); + return bus; } diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 6ad41d0720ba..31d0949b6c2f 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -815,6 +815,15 @@ struct regmap *__regmap_init(struct device *dev, if (bus) { map->max_raw_read = bus->max_raw_read; map->max_raw_write = bus->max_raw_write; + if (bus->addr_affects_max_raw_rw) { + if (map->max_raw_read < map->format.buf_size || + map->max_raw_write < map->format.buf_size) { + ret = -EINVAL; + goto err_name; + } + map->max_raw_read -= (map->format.reg_bytes + map->format.pad_bytes); + map->max_raw_write -= (map->format.reg_bytes + map->format.pad_bytes); + } } map->dev = dev; map->bus = bus; diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 77755196277c..a90d1e270b1f 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -504,6 +504,7 @@ typedef void (*regmap_hw_free_context)(void *context); * @max_raw_read: Max raw read size that can be used on the bus. * @max_raw_write: Max raw write size that can be used on the bus. * @free_on_exit: kfree this on exit of regmap + * @addr_affects_max_raw_rw: max_raw_[read|write] must include the address and padding preamble */ struct regmap_bus { bool fast_io; @@ -522,6 +523,7 @@ struct regmap_bus { size_t max_raw_read; size_t max_raw_write; bool free_on_exit; + bool addr_affects_max_raw_rw; }; /*
regmap-spi will split data and address between two transfers in the same message so use addr_affects_max_raw_rw to flag that the number bytes to read or write should be a little less (address + padding size), so that the SPI controller can merge the entire message into a single CS period Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com> --- drivers/base/regmap/regmap-spi.c | 14 ++++++++++++-- drivers/base/regmap/regmap.c | 9 +++++++++ include/linux/regmap.h | 2 ++ 3 files changed, 23 insertions(+), 2 deletions(-)