[DRAFT] Update MBQ regmap to handle more sizes and defers #5087
Conversation
charleskeepax
requested review from
lgirdwood,
plbossart,
ranj063,
kv2019i,
dbaluta,
bardliao and
ujfalusi
as code owners
drivers/base/regmap/regmap-sdw-mbq.c
Outdated
| break; | ||
| default: | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
I have multiple objections to this patch
a) what makes you think the COMMAND_IGNORED will only happen for MBQs?
b) there's no concept of retry time, only a worst-case property, and the best solution is really to poll on the FunctionBusy bit. even if you only use the sleep and retry, for the final timeout case you'd want to log what the function busy bit value is.
c) DisCo has the concept of mipi-sdca-control-deferrable, so we should only use the deferred mechanism for the controls (and thus address ranges) that require this mechanism.
I would use another callback in addition to mbq_size to check if a control requires this check or not.
There was a problem hiding this comment.
Using the function busy would perhaps be slightly more accurate, but it adds a fair amount of complexity without really much in the way of advantages. You still have all the same questions of how often to poll and how long to poll for, I wouldn't object to changing to a total poll time if that is preferred rather than a delay between polls but fairly close to equivalent.
I could perhaps add a callback to trigger this only for deferrable controls, the decision I made the first time through was that it wasn't really worth it. There are really two situations you could get a command ignored for a non-deferrable control, the control doesn't exist or something went wrong. In either case retrying didn't really feel like much of an issue. It will either fail eventually or succeed on the retry if it was an intermittent fault.
There was a problem hiding this comment.
if you have an intermittent fault then something is really very wrong -> the device lost sync, interrupts are disabled and the audio transfers were stopped. I don't see much merit in trying to conceal such problems, fail big and fail early is better IMHO. I really think we should only have a different behavior when the spec requires us to do so...
There was a problem hiding this comment.
Yeah that is a fair point, I do like to fail early and big :-) will add a callback for the deferrabililty of the control.
|
Finally managed to get some more time to look at this, updated version that includes polling the function busy bit rather than just retrying and a few minor tidy ups. |
There was a problem hiding this comment.
nice code @charleskeepax, I added a set of comments on possible conceptual issues (which could be a misunderstanding on my side as well).
drivers/base/regmap/regmap-sdw-mbq.c
Outdated
| return ret; | ||
| fallthrough; | ||
| case 1: | ||
| return sdw_write_no_pm(slave, reg, val & 0xff); |
There was a problem hiding this comment.
Maybe I need more coffee while reading this post-lunch, but now I wonder if we actually need two regmaps for 'regular' 8 bits registers and extended MBQ for more that one byte.
Could we unify the two existing regmap handling?
There was a problem hiding this comment.
Oops missed this first comment, yeah my intention is that this patch unifies them and allows the MBQ register map to access 8-bit registers.
There was a problem hiding this comment.
so there would be an additional step of conversion of all existing regmap-sdw.c users.
Or the other way around, maybe the easiest path would be to move all the code from regmap-sdw-mbq.c into regmap-sdw.c, and then transition the small set of users of the MBQ stuff?
There was a problem hiding this comment.
I don't think we should get rid of the standard sdw regmap stuff, they serve different purposes. The regular map provides access to larger registers without MBQ, that isn't possible through the MBQ map, see all the 32-bit non-soundwire/SDCA registers on the cs42l43 or cs35l56.
Nothing should really "need" converted here, these changes should be entirely backwards compatible. So all existing code will work as is, although I would probably advise that the few codecs using multiple register maps for SDCA do get converted just because of the complexity and error prone-ness of implementing things that way, but that is up to the maintainers of those drivers.
There was a problem hiding this comment.
not following the point about serving different purposes. I don't see much of a difference between a vendor-specific and a class-defined register, and I don't like giving people the option of hurting themselves. I don't also understand the "32-bit non-soundwire/SDCA registers on the cs42l43 or cs35l56", the only thing the regular regmap will support is 8-bit registers.
There was a problem hiding this comment.
Loosely the way I see this is the MBQ register map is the SDCA regmap and the plain sdw one is just for plain soundwire devices.
There was a problem hiding this comment.
that's not my interpretation at all. the plain sdw regmap was for 8 bit registers and larger registers had to use the mbq stuff.
by removing the size restrictions on all sides, Cirrus Logic created an interesting question of what regmap should be used when....
If both implementations of regmaps for soundwire support multiple sizes, I am not sure why we need 2.
The only thing the MBQ does is write the 8-bit values in a specific order to be standard, I can't think how this would impact access to non-SDCA registers?
There was a problem hiding this comment.
The two register maps access data different for >1 bytes data. The sdw register maps just accesses the bytes sequentially, the mbq register maps uses the mbq protocol whilst accessing them. The mbq map also processes the whole thing as single byte calls to the soundwire core, whereas the regular map passes the whole transaction to the core allowing for slight efficiency improvements that are not possible in the mbq case.
The regular map is more of a direct mapping of the Linux soundwire API to a regmap, whereas the mbq one layers protocol on top. I am not sure I see that it creates any confusion other than arguably in the single byte case without defers where you could legitimately use either map but in that case it just doesn't really matter which you use. Although the standard would seem logical since you are using no mbq features. For higher sized registers only one of the two maps will actually work for a given device. But the differentiation between them seems pretty clear cut to me if you just want raw bytes over soundwire normal map, if you want mbq protocol on top of that mbq.
There was a problem hiding this comment.
I am probably missing something or not up-to-speed on all this.
Do you have any examples where the regmap stuff updates multiple registers with "slight efficiency improvements that are not possible in the mbq case".
Also if you have an example of non-mbq registers with more than 8 bits, that would be useful.
There was a problem hiding this comment.
For examples of non-MBQ registers with more than 8-bits you can look at any of cs42l43 registers (they are all 32-bit, non-mbq) the regmap_config is defined in drivers/mfd/cs42l43.c. I would be up for an actual chat if you want to go through the details of cs42l43s hardware more closely, that is probably best done offline. But I guess there are few reasons for this, but mostly Cirrus has been making soundwire chips a lot longer than MBQ has been a thing and our chips tend to offer multiple control buses (i2c/spi).
For the discussions of efficiency. A lot of it is really that the MBQ regmap is forced to use the reg_read/reg_write interface to regmap which processes a single transaction. This forces regmap to do additional processing/copies, particularly for larger bulk/raw read/writes, although those are less likely in an MBQ context anyway.
Secondly, the regmap-sdw-mbq is forced to use sdw_read, also because of the address changing between bytes of a single transaction. But regmap-sdw can use sdw_nwrite to pass the whole tranaction to the soundwire master at once letting it fill its whole internal FIFO and let rip. Using nwrite may also have some benefits if we were to look at integrating BRA into the system for larger transactions, I know you are currently favouring a separate API for BRA and I haven't spent enough time working through it to really form a solid opinion there yet, but it would be nice to not close doors earlier than we need to.
But fundamentally I think those things are fringe benefits and it really comes down to this, the two register maps access multiple byte quantities in different ways, so they should be different register maps. A chip either does things one way or the other way so its pretty easy to pick which map to use, and even if you get it wrong changing between costs a few minutes. Allowing them both to support 8-bit registers, even though those are handled identically in both, also makes sense because it avoid having to write drivers that have multiple regmaps which is error prone and really balloons code size when interfacing with ASoC.
drivers/base/regmap/regmap-sdw-mbq.c
Outdated
| return ret; | ||
| fallthrough; | ||
| case 1: | ||
| return sdw_write_no_pm(slave, reg, val & 0xff); |
There was a problem hiding this comment.
the other point that makes me tick is that in theory regular 8-bits SDCA registers could also be deferred. But this PR adds the deferrable property only for MBQs, so not sure if we are going in the right direction if we end-up having deferrable support in two places?
There was a problem hiding this comment.
Not sure I totally follow this one, you can access regular 8-bit registers through this regmap there is no need to differentiate them the behaviour is identical.
include/linux/regmap.h
Outdated
| @@ -503,11 +503,22 @@ struct regmap_range_cfg { | |||
| * struct regmap_sdw_mbq_cfg - Configuration for Multi-Byte Quantities | |||
| * | |||
| * @mbq_size: Callback returning the actual size of the given register. | |||
| * @deferrable: Callback returning true if the hardware can defer | |||
| * transactions to the given register. | |||
There was a problem hiding this comment.
@charleskeepax we should add a comment that only SDCA controls are allowed to be deferred, and for SDCA devices the DisCo spec provides a list of all SDCA controls and if each control can be deferred with a _DSD property. In other words this callback could be implemented either with a hard-coded switch table in the driver or by collecting information from platform firmware.
There was a problem hiding this comment.
Sure, seems sensible will add that.
Compliment the existing macro to construct an SDCA control address with additional macros to extract the constituent parts of such an address. Also add defines for the FUNCTION_STATUS register. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
SoundWire MBQ register maps typically contain a variety of register sizes, which doesn't map ideally to the regmap abstraction which expects register maps to have a consistent size. Currently the MBQ register map only allows 16-bit registers to be defined, however this leads to complex CODEC driver implementations with an 8-bit register map and a 16-bit MBQ, every control will then have a custom get and put handler that allows them to access different register maps. Further more 32-bit MBQ quantities are not currently supported. Add support for additional MBQ sizes and to avoid the complexity of multiple register maps treat the val_size as a maximum size for the register map. Within the regmap use an ancillary callback to determine how many bytes to actually read/write to the hardware for a specific register. In the case that no callback is defined the behaviour defaults back to the existing behaviour of a fixed size register map. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
The SDCA specification allows for controls to be deferred. In the case of a deferred control the device will return COMMAND_IGNORED to the 8-bit operation that would cause the value to commit. Which is the final 8-bits on a write, or the first 8-bits on a read. In the case of receiving a defer, the regmap will poll the SDCA function busy bit, after which the transaction will be retried, returning an error if the function busy does not clear within a chip specific timeout. Since this is common SDCA functionality which is the 99% use-case for MBQs it makes sense to incorporate this functionality into the register map. If no MBQ configuration is specified, the behaviour will default to the existing behaviour. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
|
@charleskeepax Is this PR ready to merge? |
|
I was thinking probably best if I send this one directly upstream, nothing that really depends on it in the sof tree yet, and in case it gets any major changes, but mostly happy either way. |
Yes, it would be appreciated if you could send it directly upstream. :) |
|
Going to close this for now, will re-open if there are any significant changes, otherwise will get around to sending upstream for comments soon. |
Add support to the existing MBQ regmap for 8-bit and 32-bit reads, as well SDCA style deferred reads that require retrying once the device side has had a chance to get the result ready. Probably makes more sense to send this directly upstream rather than merging through the SOF repo, but thought I would push a version here so you guys can have a chance to express any strong opinions before we get upstream involved.