[RFC,01/16] Documentation: driver: add SoundWire BRA description

The Bulk Register Access protocol was left as a TODO topic since
2018. It's time to document this protocol and the design of its Linux
support.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 Documentation/driver-api/soundwire/bra.rst    | 478 ++++++++++++++++++
 Documentation/driver-api/soundwire/index.rst  |   1 +
 .../driver-api/soundwire/summary.rst          |   5 +-
 3 files changed, 480 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/driver-api/soundwire/bra.rst

On Thu, Dec 07, 2023 at 04:29:29PM -0600, Pierre-Louis Bossart wrote:

> +Addressing restrictions
> +-----------------------
> +
> +The Device Number specified in the Header follows the SoundWire
> +definitions, and broadcast and group addressing are
> +permitted. However, in reality it is very unlikely that the exact same
> +binary data needs to be provided to the two different Peripheral
> +devices. The Linux implementation only allows for transfers to a
> +single device at a time.

For the firmware download case it seems likely that this won't always be
the case, but it's definitely a thing that could be done incrementally.

> +Regmap use
> +~~~~~~~~~~

> +Existing codec drivers rely on regmap to download firmware to
> +Peripherals, so at a high-level it would seem natural to combine BRA
> +and regmap. The regmap layer could check if BRA is available or not,
> +and use a regular read-write command channel in the latter case.

> +However, the regmap layer does not have information on latency or how
> +critical a BRA transfer is. It would make more sense to let the codec
> +driver make decisions (wait, timeout, fallback to regular
> +read/writes).

These don't seem like insurmountable obstacles - they feel more like a
copy break kind of situation where we can infer things from the pattern
of transactions, and there's always the possibility of adding some calls
to give regmap more high level information about the overall state of
the device.  One of the expected usage patterns with cache only mode is
to build up a final register state then let the cache code work out the
optimal pattern to actually write that out.

> +In addition, the hardware may lose context and ask for the firmware to
> +be downloaded again. The firmware is not however a fixed definition,
> +the SDCA definition allows the hardware to request an updated firmware
> +or a different coefficient table to deal with specific environment
> +conditions. In other words, traditional regmap cache management is not
> +good enough, the Peripheral driver is required track hardware
> +notifications and react accordingly.

I might be missing something but those requests for redownload sound
like things that would occur regardless of the mechanism used to perform
the I/O?

> +Concurrency between BRA and regular read/write
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> +The existing 'nread/nwrite' API already relies on a notion of start
> +address and number of bytes, so it would be possible to extend this
> +API with a 'hint' requesting BPT/BRA be used.

> +However BRA transfers could be quite long, and the use of a single
> +mutex for regular read/write and BRA is a show-stopper. Independent
> +operation of the control/command and BRA transfers is a fundamental
> +requirement, e.g. to change the volume level with the existing regmap
> +interface while downloading firmware.

> +This places the burden on the codec driver to verify that there is no
> +concurrent access to the same address with the command/control
> +protocol and the BRA protocol.

This makes me feel very nervous about robustness.  I do note that regmap
has an async interface which is currently only used for SPI and really
only by the Cirrus DSPs (plus opportunisticly by rbtree cache sync), the
original usage was to fill the pipleine of SPI controllers so we can
ideally push data entirely from interrupt.  TBH that was done before SMP
became standard and it's a lot less clear in this day and age that this
is actually helpful, the overhead of cross core synchronisation likely
obliterates any benefit.  There's sufficently few users that we could
make API changes readily to fit better.

> +One possible strategy to speed-up all initialization tasks would be to
> +start a BRA transfer for firmware download, then deal with all the
> +"regular" read/writes in parallel with the command channel, and last
> +to wait for the BRA transfers to complete. This would allow for a
> +degree of overlap instead of a purely sequential solution. As a
> +results, the BRA API must support async transfers and expose a
> +separate wait function.

This feels a lot like it could map onto the regmap async interface, it
would need a bit of a rework (mainly that currently they provide
ordering guarantees with respect to both each other and sync I/O) but
those could be removed with some care) but it's got the "here's a list
of I/O, here's another call to ensure it's all actually happened" thing.
It sounds very much like how I was thinking of the async API when I was
writing it and the initial users.

It's this bit that really got me thinking it could fit well into regmap.

> +Error handling
> +~~~~~~~~~~~~~~
> +
> +The expected response to a 'bra_message' and follow-up behavior may
> +vary:
> +
> +   (1) A Peripheral driver may want to receive an immediate -EBUSY
> +       response if the BRA protocol is not available at a given time.
> +
> +   (2) A Peripheral driver may want to wait until a timeout for the
> +       on-going transfer to be handled
> +
> +   (3) A Peripheral driver may want to wait until existing BRA
> +       transfers complete or deal with BRA as a background task when
> +       audio transfers stop. In this case, there would be no timeout,
> +       and the operation may not happen if the platform is suspended.

Option 3 would be a jump for regmap.

Thanks for the comments Mark, much appreciated.

>> +Addressing restrictions
>> +-----------------------
>> +
>> +The Device Number specified in the Header follows the SoundWire
>> +definitions, and broadcast and group addressing are
>> +permitted. However, in reality it is very unlikely that the exact same
>> +binary data needs to be provided to the two different Peripheral
>> +devices. The Linux implementation only allows for transfers to a
>> +single device at a time.
> 
> For the firmware download case it seems likely that this won't always be
> the case, but it's definitely a thing that could be done incrementally.

One example discussed this week on the mailing list is the Cirrus Logic
case, where the firmware contains information on which speakers a given
amplifier should be doing, and each firmware is named as AMP-n.

I have really not found any practical case where the same data needs to
be sent to N devices, and I don't have a burning desire to tie the codec
initialization together with all the asynchronous nature of
enumeration/probe.

>> +Regmap use
>> +~~~~~~~~~~
> 
>> +Existing codec drivers rely on regmap to download firmware to
>> +Peripherals, so at a high-level it would seem natural to combine BRA
>> +and regmap. The regmap layer could check if BRA is available or not,
>> +and use a regular read-write command channel in the latter case.
> 
>> +However, the regmap layer does not have information on latency or how
>> +critical a BRA transfer is. It would make more sense to let the codec
>> +driver make decisions (wait, timeout, fallback to regular
>> +read/writes).
> 
> These don't seem like insurmountable obstacles - they feel more like a
> copy break kind of situation where we can infer things from the pattern
> of transactions, and there's always the possibility of adding some calls
> to give regmap more high level information about the overall state of
> the device.  One of the expected usage patterns with cache only mode is
> to build up a final register state then let the cache code work out the
> optimal pattern to actually write that out.

I did expect some pushback on regmap :-)

The point is really that the main use for this download is a set of
write-once memory areas which happen to be mapped as registers. There is
no real need to declare or access each memory address individually.

In addition in terms of error handling, the expectation is that the set
of writes are handled in a pass/fail manner. There's no real way to know
which of the individual writes failed.

>> +In addition, the hardware may lose context and ask for the firmware to
>> +be downloaded again. The firmware is not however a fixed definition,
>> +the SDCA definition allows the hardware to request an updated firmware
>> +or a different coefficient table to deal with specific environment
>> +conditions. In other words, traditional regmap cache management is not
>> +good enough, the Peripheral driver is required track hardware
>> +notifications and react accordingly.
> 
> I might be missing something but those requests for redownload sound
> like things that would occur regardless of the mechanism used to perform
> the I/O?

What I meant is that the codec driver would e.g. need to fetch a
different firmware table and download it. There's no real need to
maintain a cache on the host side since the entire table will be downloaded.

>> +Concurrency between BRA and regular read/write
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>> +The existing 'nread/nwrite' API already relies on a notion of start
>> +address and number of bytes, so it would be possible to extend this
>> +API with a 'hint' requesting BPT/BRA be used.
> 
>> +However BRA transfers could be quite long, and the use of a single
>> +mutex for regular read/write and BRA is a show-stopper. Independent
>> +operation of the control/command and BRA transfers is a fundamental
>> +requirement, e.g. to change the volume level with the existing regmap
>> +interface while downloading firmware.
> 
>> +This places the burden on the codec driver to verify that there is no
>> +concurrent access to the same address with the command/control
>> +protocol and the BRA protocol.
> 
> This makes me feel very nervous about robustness.  I do note that regmap
> has an async interface which is currently only used for SPI and really
> only by the Cirrus DSPs (plus opportunisticly by rbtree cache sync), the
> original usage was to fill the pipleine of SPI controllers so we can
> ideally push data entirely from interrupt.  TBH that was done before SMP
> became standard and it's a lot less clear in this day and age that this
> is actually helpful, the overhead of cross core synchronisation likely
> obliterates any benefit.  There's sufficently few users that we could
> make API changes readily to fit better.

I am not that nervous, it's a known hardware issue and the software
drivers SHALL make sure that the two methods of accessing a register are
not used concurrently. We could add some sort of mutex on specific
memory areas.

>> +One possible strategy to speed-up all initialization tasks would be to
>> +start a BRA transfer for firmware download, then deal with all the
>> +"regular" read/writes in parallel with the command channel, and last
>> +to wait for the BRA transfers to complete. This would allow for a
>> +degree of overlap instead of a purely sequential solution. As a
>> +results, the BRA API must support async transfers and expose a
>> +separate wait function.
> 
> This feels a lot like it could map onto the regmap async interface, it
> would need a bit of a rework (mainly that currently they provide
> ordering guarantees with respect to both each other and sync I/O) but
> those could be removed with some care) but it's got the "here's a list
> of I/O, here's another call to ensure it's all actually happened" thing.
> It sounds very much like how I was thinking of the async API when I was
> writing it and the initial users.
> 
> It's this bit that really got me thinking it could fit well into regmap.

I really don't know anything about this async interface, if you have
pointers on existing examples I am all ears....I am not aware of any
Intel platform or codec used on an Intel platform making use of this API.

At any rate the low-level behavior is to
a) allocate and configure all the SoundWire resources
b) allocate and configure all the DMA resources
c) trigger DMA and enable SoundWire transfers
d) wait for the DMA to complete

The codec API can be modified as needed, as long as there are provisions
for these 4 steps.

>> +Error handling
>> +~~~~~~~~~~~~~~
>> +
>> +The expected response to a 'bra_message' and follow-up behavior may
>> +vary:
>> +
>> +   (1) A Peripheral driver may want to receive an immediate -EBUSY
>> +       response if the BRA protocol is not available at a given time.
>> +
>> +   (2) A Peripheral driver may want to wait until a timeout for the
>> +       on-going transfer to be handled
>> +
>> +   (3) A Peripheral driver may want to wait until existing BRA
>> +       transfers complete or deal with BRA as a background task when
>> +       audio transfers stop. In this case, there would be no timeout,
>> +       and the operation may not happen if the platform is suspended.
> 
> Option 3 would be a jump for regmap.

Sorry, I don't get what a 'jump' means in this context.

On Thu, Dec 07, 2023 at 04:29:29PM -0600, Pierre-Louis Bossart wrote:
> The Bulk Register Access protocol was left as a TODO topic since
> 2018. It's time to document this protocol and the design of its Linux
> support.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
> +Concurrency between BRA and regular read/write
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The existing 'nread/nwrite' API already relies on a notion of start
> +address and number of bytes, so it would be possible to extend this
> +API with a 'hint' requesting BPT/BRA be used.
> +
> +However BRA transfers could be quite long, and the use of a single
> +mutex for regular read/write and BRA is a show-stopper. Independent
> +operation of the control/command and BRA transfers is a fundamental
> +requirement, e.g. to change the volume level with the existing regmap
> +interface while downloading firmware.

Is this definitely a show stopper? Not saying that it wouldn't be
desirable to do both from a speed perspective, but current
systems that download firmware (I2C/SPI) typically will block the
bus for some amount of time. There are also some desirable properties
to a single lock such as not needing to worry about accessing the
same register in the bulk transfer and a normal command transfer.

> +Audio DMA support
> +-----------------
> +
> +Some DMAs, such as HDaudio, require an audio format field to be
> +set. This format is in turn used to define acceptable bursts. BPT/BRA
> +support is not fully compatible with these definitions in that the
> +format may vary between read and write commands.
> +
> +In addition, on Intel HDaudio Intel platforms the DMAs need to be
> +programmed with a PCM format matching the bandwidth of the BPT/BRA
> +transfer. The format is based on 48kHz 32-bit samples, and the number
> +of channels varies to adjust the bandwidth. The notion of channel is
> +completely notional since the data is not typical audio
> +PCM. Programming channels helps reserve enough bandwidth and adjust
> +FIFO sizes to avoid xruns. Note that the quality of service comes as a
> +cost. Since all channels need to be present as a sample block, data
> +sizes not aligned to 128-bytes are not supported.

Apologies but could you elaborate a litte on this? I am not sure
I follow the reasoning, how does the 48k 32bit DMA implementation
result in 128-byte limitation? I would have thought 1 channel would
be 4-bytes and you are varying the channels so I would have expected
4-byte aligned maybe 8-byte if the DMA expects stereo pairs.

And what exactly do we mean by aligned, are we saying the length
all transfers needs to be a multiple of 128-bytes?

I think we might have some annoying restrictions on the block
size on our hardware as well I will go dig into that and report
back.

Thanks,
Charles

On Thu, Dec 07, 2023 at 06:56:55PM -0600, Pierre-Louis Bossart wrote:

> >> +The Device Number specified in the Header follows the SoundWire
> >> +definitions, and broadcast and group addressing are
> >> +permitted. However, in reality it is very unlikely that the exact same
> >> +binary data needs to be provided to the two different Peripheral
> >> +devices. The Linux implementation only allows for transfers to a
> >> +single device at a time.

> > For the firmware download case it seems likely that this won't always be
> > the case, but it's definitely a thing that could be done incrementally.

> One example discussed this week on the mailing list is the Cirrus Logic
> case, where the firmware contains information on which speakers a given
> amplifier should be doing, and each firmware is named as AMP-n.

I can imagine a vendor structuring things so they've got separate
firmware and coefficent/configuration images, the firmware images could
be shared.

> I have really not found any practical case where the same data needs to
> be sent to N devices, and I don't have a burning desire to tie the codec
> initialization together with all the asynchronous nature of
> enumeration/probe.

Like I say it does seem like something that could be done incrementally.

> > These don't seem like insurmountable obstacles - they feel more like a
> > copy break kind of situation where we can infer things from the pattern
> > of transactions, and there's always the possibility of adding some calls
> > to give regmap more high level information about the overall state of
> > the device.  One of the expected usage patterns with cache only mode is
> > to build up a final register state then let the cache code work out the
> > optimal pattern to actually write that out.

> I did expect some pushback on regmap :-)

> The point is really that the main use for this download is a set of
> write-once memory areas which happen to be mapped as registers. There is
> no real need to declare or access each memory address individually.

Yeah, normally it's just write only stuff but I've seen things like
controls being added in the DSP memory before - the 

> In addition in terms of error handling, the expectation is that the set
> of writes are handled in a pass/fail manner. There's no real way to know
> which of the individual writes failed.

That's the case for any block writes.

> > I might be missing something but those requests for redownload sound
> > like things that would occur regardless of the mechanism used to perform
> > the I/O?

> What I meant is that the codec driver would e.g. need to fetch a
> different firmware table and download it. There's no real need to
> maintain a cache on the host side since the entire table will be downloaded.

I mean, if that's the usage pattern surely things would just be marked
volatile anyway?  A cache is entirely optional.

> > This feels a lot like it could map onto the regmap async interface, it
> > would need a bit of a rework (mainly that currently they provide
> > ordering guarantees with respect to both each other and sync I/O) but
> > those could be removed with some care) but it's got the "here's a list
> > of I/O, here's another call to ensure it's all actually happened" thing.
> > It sounds very much like how I was thinking of the async API when I was
> > writing it and the initial users.

> > It's this bit that really got me thinking it could fit well into regmap.

> I really don't know anything about this async interface, if you have
> pointers on existing examples I am all ears....I am not aware of any
> Intel platform or codec used on an Intel platform making use of this API.

grep for regmap_.*async - cs_dsp.c is the upstream example in a driver,
or there's the rbtree cache sync code which uses a back door to go into
an async mode.  Basically just variants of all the normal regmap I/O
calls with a _complete() call you can use to wait for everything to
happen.  The implementation is a bit heavyweight since it was written to
work with fairly slow buses.

> At any rate the low-level behavior is to
> a) allocate and configure all the SoundWire resources
> b) allocate and configure all the DMA resources
> c) trigger DMA and enable SoundWire transfers
> d) wait for the DMA to complete

> The codec API can be modified as needed, as long as there are provisions
> for these 4 steps.

That's not quite how the current API works, but it feels like it's close
enough to the intent and there's literally one user of the actual API.

> >> +   (3) A Peripheral driver may want to wait until existing BRA
> >> +       transfers complete or deal with BRA as a background task when
> >> +       audio transfers stop. In this case, there would be no timeout,
> >> +       and the operation may not happen if the platform is suspended.

> > Option 3 would be a jump for regmap.

> Sorry, I don't get what a 'jump' means in this context.

Big change.

Hi Pierre,

On 07-12-23, 16:29, Pierre-Louis Bossart wrote:
> The Bulk Register Access protocol was left as a TODO topic since
> 2018. It's time to document this protocol and the design of its Linux
> support.

Thanks for letting me know about this thread. At least now with B4 we
can grab threads we are not copied.

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  Documentation/driver-api/soundwire/bra.rst    | 478 ++++++++++++++++++

Can we split the cadence parts of this to bra-cadence.rst that way this
file documents the core parts only

> +Concurrency between BRA and regular read/write
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The existing 'nread/nwrite' API already relies on a notion of start
> +address and number of bytes, so it would be possible to extend this
> +API with a 'hint' requesting BPT/BRA be used.
> +
> +However BRA transfers could be quite long, and the use of a single
> +mutex for regular read/write and BRA is a show-stopper. Independent
> +operation of the control/command and BRA transfers is a fundamental
> +requirement, e.g. to change the volume level with the existing regmap
> +interface while downloading firmware.
> +
> +This places the burden on the codec driver to verify that there is no
> +concurrent access to the same address with the command/control
> +protocol and the BRA protocol.
> +
> +In addition, the 'sdw_msg' structure hard-codes support for 16-bit
> +addresses and paging registers which are irrelevant for BPT/BRA
> +support based on native 32-bit addresses. A separate API with
> +'sdw_bpt_msg' makes more sense.
> +
> +One possible strategy to speed-up all initialization tasks would be to
> +start a BRA transfer for firmware download, then deal with all the
> +"regular" read/writes in parallel with the command channel, and last
> +to wait for the BRA transfers to complete. This would allow for a
> +degree of overlap instead of a purely sequential solution. As a
> +results, the BRA API must support async transfers and expose a
> +separate wait function.

That sounds logical to me

> +
> +Error handling
> +~~~~~~~~~~~~~~
> +
> +The expected response to a 'bra_message' and follow-up behavior may
> +vary:
> +
> +   (1) A Peripheral driver may want to receive an immediate -EBUSY
> +       response if the BRA protocol is not available at a given time.
> +
> +   (2) A Peripheral driver may want to wait until a timeout for the
> +       on-going transfer to be handled
> +
> +   (3) A Peripheral driver may want to wait until existing BRA
> +       transfers complete or deal with BRA as a background task when
> +       audio transfers stop. In this case, there would be no timeout,
> +       and the operation may not happen if the platform is suspended.

Is this runtime suspend or S3/S4 case?

> +BTP/BRA API available to peripheral drivers
> +-------------------------------------------
> +
> +ASoC Peripheral drivers may use
> +
> +   - sdw_bpt_stream_open(mode)
> +
> +      This function verifies that the BPT protocol with the
> +      'mode'. For now only BRA is accepted as a mode. This function
> +      allocates a work buffer internally. This buffer is not exposed
> +      to the caller.
> +
> +     errors:
> +         -ENODEV: BPT/BRA is not supported by the Manager.
> +
> +         -EBUSY: another agent is already using the audio payload for
> +          audio transfers. There is no way to predict when the audio
> +          streams might stop, this will require the Peripheral driver
> +          to fall back to the regular (slow) command channel.
> +
> +         -EAGAIN: another agent is already transferring data using the
> +          BPT/BRA protocol. Since the transfers will typically last
> +          10s or 100s of ms, the Peripheral driver may wait and retry
> +          later.
> +
> +    - sdw_bpt_message_send_async(bpt_message)

why not have a single API that does both? First check if it is supported
and then allocate buffers and do the transfer.. What are the advantages
of using this two step process

>>  Documentation/driver-api/soundwire/bra.rst    | 478 ++++++++++++++++++
> 
> Can we split the cadence parts of this to bra-cadence.rst that way this
> file documents the core parts only

Yes, we can split the Cadence parts out.


>> +Error handling
>> +~~~~~~~~~~~~~~
>> +
>> +The expected response to a 'bra_message' and follow-up behavior may
>> +vary:
>> +
>> +   (1) A Peripheral driver may want to receive an immediate -EBUSY
>> +       response if the BRA protocol is not available at a given time.
>> +
>> +   (2) A Peripheral driver may want to wait until a timeout for the
>> +       on-going transfer to be handled
>> +
>> +   (3) A Peripheral driver may want to wait until existing BRA
>> +       transfers complete or deal with BRA as a background task when
>> +       audio transfers stop. In this case, there would be no timeout,
>> +       and the operation may not happen if the platform is suspended.
> 
> Is this runtime suspend or S3/S4 case?

System suspend (which can also mean S0i1).

I don't think we can have a case where a peripheral driver waits on
something without having done a pm_runtime_get_sync() to prevent
runtime_pm suspend.

> 
>> +BTP/BRA API available to peripheral drivers
>> +-------------------------------------------
>> +
>> +ASoC Peripheral drivers may use
>> +
>> +   - sdw_bpt_stream_open(mode)
>> +
>> +      This function verifies that the BPT protocol with the
>> +      'mode'. For now only BRA is accepted as a mode. This function
>> +      allocates a work buffer internally. This buffer is not exposed
>> +      to the caller.
>> +
>> +     errors:
>> +         -ENODEV: BPT/BRA is not supported by the Manager.
>> +
>> +         -EBUSY: another agent is already using the audio payload for
>> +          audio transfers. There is no way to predict when the audio
>> +          streams might stop, this will require the Peripheral driver
>> +          to fall back to the regular (slow) command channel.
>> +
>> +         -EAGAIN: another agent is already transferring data using the
>> +          BPT/BRA protocol. Since the transfers will typically last
>> +          10s or 100s of ms, the Peripheral driver may wait and retry
>> +          later.
>> +
>> +    - sdw_bpt_message_send_async(bpt_message)
> 
> why not have a single API that does both? First check if it is supported
> and then allocate buffers and do the transfer.. What are the advantages
> of using this two step process

Symmetry is the only thing that comes to my mind. Open - close and send
- wait are natural matches, aren't they?

We do need a wait(), so bundling open() and send() would be odd.

But you have a point that the open() is not generic in that it also
prepares the DMA buffers for transmission. Maybe it's more natural to
follow the traditional open(), hw_params(), hw_free, close() from ALSA.

On Mon, Dec 18, 2023 at 01:58:47PM +0100, Pierre-Louis Bossart wrote:
> > why not have a single API that does both? First check if it is supported
> > and then allocate buffers and do the transfer.. What are the advantages
> > of using this two step process
> 
> Symmetry is the only thing that comes to my mind. Open - close and send
> - wait are natural matches, aren't they?
> 
> We do need a wait(), so bundling open() and send() would be odd.
> 

I agree send->wait->close would be odd, But you just bundle close
into wait. So the API becomes just send->wait, which seems pretty
logical.

> But you have a point that the open() is not generic in that it also
> prepares the DMA buffers for transmission. Maybe it's more natural to
> follow the traditional open(), hw_params(), hw_free, close() from ALSA.

I think this just makes it worse, you are now adding even more
calls. The problem I see here is that, open and close (at least to
me) strongly implies that you can do multiple operations between
them and unless I have misunderstood something here you can't.

Thanks,
Charles

On 12/18/23 08:29, Charles Keepax wrote:
> On Mon, Dec 18, 2023 at 01:58:47PM +0100, Pierre-Louis Bossart wrote:
>>> why not have a single API that does both? First check if it is supported
>>> and then allocate buffers and do the transfer.. What are the advantages
>>> of using this two step process
>>
>> Symmetry is the only thing that comes to my mind. Open - close and send
>> - wait are natural matches, aren't they?
>>
>> We do need a wait(), so bundling open() and send() would be odd.
>>
> 
> I agree send->wait->close would be odd, But you just bundle close
> into wait. So the API becomes just send->wait, which seems pretty
> logical.

Fair enough, send()/wait() would work indeed.

I guess I wanted to keep the callbacks reasonably small (already 200
lines for the open), but we can split the 'send' callback into smaller
helpers to keep the code readable. There's no good reason to expose
these smaller helpers to codec drivers.

>> But you have a point that the open() is not generic in that it also
>> prepares the DMA buffers for transmission. Maybe it's more natural to
>> follow the traditional open(), hw_params(), hw_free, close() from ALSA.
> 
> I think this just makes it worse, you are now adding even more
> calls. The problem I see here is that, open and close (at least to
> me) strongly implies that you can do multiple operations between
> them and unless I have misunderstood something here you can't.

That's right, the open was not compatible with multiple operations.
Collapsing open/send and wait/close sounds more logical, thanks for the
feedback.

On 18-12-23, 13:58, Pierre-Louis Bossart wrote:
> 
> >>  Documentation/driver-api/soundwire/bra.rst    | 478 ++++++++++++++++++
> > 
> > Can we split the cadence parts of this to bra-cadence.rst that way this
> > file documents the core parts only
> 
> Yes, we can split the Cadence parts out.

Great

> 
> 
> >> +Error handling
> >> +~~~~~~~~~~~~~~
> >> +
> >> +The expected response to a 'bra_message' and follow-up behavior may
> >> +vary:
> >> +
> >> +   (1) A Peripheral driver may want to receive an immediate -EBUSY
> >> +       response if the BRA protocol is not available at a given time.
> >> +
> >> +   (2) A Peripheral driver may want to wait until a timeout for the
> >> +       on-going transfer to be handled
> >> +
> >> +   (3) A Peripheral driver may want to wait until existing BRA
> >> +       transfers complete or deal with BRA as a background task when
> >> +       audio transfers stop. In this case, there would be no timeout,
> >> +       and the operation may not happen if the platform is suspended.
> > 
> > Is this runtime suspend or S3/S4 case?
> 
> System suspend (which can also mean S0i1).
> 
> I don't think we can have a case where a peripheral driver waits on
> something without having done a pm_runtime_get_sync() to prevent
> runtime_pm suspend.
> 
> > 
> >> +BTP/BRA API available to peripheral drivers
> >> +-------------------------------------------
> >> +
> >> +ASoC Peripheral drivers may use
> >> +
> >> +   - sdw_bpt_stream_open(mode)
> >> +
> >> +      This function verifies that the BPT protocol with the
> >> +      'mode'. For now only BRA is accepted as a mode. This function
> >> +      allocates a work buffer internally. This buffer is not exposed
> >> +      to the caller.
> >> +
> >> +     errors:
> >> +         -ENODEV: BPT/BRA is not supported by the Manager.
> >> +
> >> +         -EBUSY: another agent is already using the audio payload for
> >> +          audio transfers. There is no way to predict when the audio
> >> +          streams might stop, this will require the Peripheral driver
> >> +          to fall back to the regular (slow) command channel.
> >> +
> >> +         -EAGAIN: another agent is already transferring data using the
> >> +          BPT/BRA protocol. Since the transfers will typically last
> >> +          10s or 100s of ms, the Peripheral driver may wait and retry
> >> +          later.
> >> +
> >> +    - sdw_bpt_message_send_async(bpt_message)
> > 
> > why not have a single API that does both? First check if it is supported
> > and then allocate buffers and do the transfer.. What are the advantages
> > of using this two step process
> 
> Symmetry is the only thing that comes to my mind. Open - close and send
> - wait are natural matches, aren't they?

Why have symmetry to DAI apis, why not symmetry to regmap write APIs..?
This is data transfer, so I am not sure why would we model it as a DAI.
(Internal implementation may rely on that but from API design, i dont
think that should be a concern)

> 
> We do need a wait(), so bundling open() and send() would be odd.
> 
> But you have a point that the open() is not generic in that it also
> prepares the DMA buffers for transmission. Maybe it's more natural to
> follow the traditional open(), hw_params(), hw_free, close() from ALSA.

On 18-12-23, 17:33, Pierre-Louis Bossart wrote:
> 
> 
> On 12/18/23 08:29, Charles Keepax wrote:
> > On Mon, Dec 18, 2023 at 01:58:47PM +0100, Pierre-Louis Bossart wrote:
> >>> why not have a single API that does both? First check if it is supported
> >>> and then allocate buffers and do the transfer.. What are the advantages
> >>> of using this two step process
> >>
> >> Symmetry is the only thing that comes to my mind. Open - close and send
> >> - wait are natural matches, aren't they?
> >>
> >> We do need a wait(), so bundling open() and send() would be odd.
> >>
> > 
> > I agree send->wait->close would be odd, But you just bundle close
> > into wait. So the API becomes just send->wait, which seems pretty
> > logical.
> 
> Fair enough, send()/wait() would work indeed.
> 
> I guess I wanted to keep the callbacks reasonably small (already 200
> lines for the open), but we can split the 'send' callback into smaller
> helpers to keep the code readable. There's no good reason to expose
> these smaller helpers to codec drivers.

Yes! that would be a better design IMO

> 
> >> But you have a point that the open() is not generic in that it also
> >> prepares the DMA buffers for transmission. Maybe it's more natural to
> >> follow the traditional open(), hw_params(), hw_free, close() from ALSA.
> > 
> > I think this just makes it worse, you are now adding even more
> > calls. The problem I see here is that, open and close (at least to
> > me) strongly implies that you can do multiple operations between
> > them and unless I have misunderstood something here you can't.
> 
> That's right, the open was not compatible with multiple operations.
> Collapsing open/send and wait/close sounds more logical, thanks for the
> feedback.

Sure

Hi Mark,

>> +One possible strategy to speed-up all initialization tasks would be to
>> +start a BRA transfer for firmware download, then deal with all the
>> +"regular" read/writes in parallel with the command channel, and last
>> +to wait for the BRA transfers to complete. This would allow for a
>> +degree of overlap instead of a purely sequential solution. As a
>> +results, the BRA API must support async transfers and expose a
>> +separate wait function.
> 
> This feels a lot like it could map onto the regmap async interface, it
> would need a bit of a rework (mainly that currently they provide
> ordering guarantees with respect to both each other and sync I/O) but
> those could be removed with some care) but it's got the "here's a list
> of I/O, here's another call to ensure it's all actually happened" thing.
> It sounds very much like how I was thinking of the async API when I was
> writing it and the initial users.
> 
> It's this bit that really got me thinking it could fit well into regmap.

I am now revisiting this entire patchset to try to enable firmware
downloads with this SoundWire BPT/BRA protocol. I re-looked at
regmap_raw_write_async()/regmap_async_complete() and they seem to map
well with my initial write/wait_async proposal.

Do I get this right that these async capabilities could be used to
enable this faster SoundWire protocol, but the regular regmap_write()
could still happen in parallel with these async transfers? This could be
useful if e.g. there's a jack detection while downloading firmware for
another function.

I don't see any locking that would prevent such a dual operation - with
the caveat that it would be up to the codec driver to make sure they
don't try to access the same register with the two modes of operation.

The only thing that would need to be extended is that we'd need
additional callbacks to check if the protocol is supported on a given
hardware/firmware platform, and if there's no audio stream. In these
cases the async writes would be demoted to regular writes. Otherwise the
bus would be locked to make sure no reconfiguration takes place while
the firmware download happens, and unlocked when the transfer ends.

Thanks for your help on all this,
-Pierre

On Tue, Aug 20, 2024 at 09:48:05AM +0200, Pierre-Louis Bossart wrote:

> > This feels a lot like it could map onto the regmap async interface, it
> > would need a bit of a rework (mainly that currently they provide
> > ordering guarantees with respect to both each other and sync I/O) but
> > those could be removed with some care) but it's got the "here's a list
> > of I/O, here's another call to ensure it's all actually happened" thing.
> > It sounds very much like how I was thinking of the async API when I was
> > writing it and the initial users.

> > It's this bit that really got me thinking it could fit well into regmap.

> Do I get this right that these async capabilities could be used to
> enable this faster SoundWire protocol, but the regular regmap_write()
> could still happen in parallel with these async transfers? This could be
> useful if e.g. there's a jack detection while downloading firmware for
> another function.

As far as the regmap core is concerned, yes - with SPI we do wind up
with ordering but that's because the SPI sync API is a thin wrapper
around the SPI async API rather than anything regmap does.

> The only thing that would need to be extended is that we'd need
> additional callbacks to check if the protocol is supported on a given
> hardware/firmware platform, and if there's no audio stream. In these
> cases the async writes would be demoted to regular writes. Otherwise the
> bus would be locked to make sure no reconfiguration takes place while
> the firmware download happens, and unlocked when the transfer ends.

Those callbacks seem reasonable.  We already do the demotion to sync for
buses that don't have async, it'd just need to be made dynamic.

On 8/20/24 13:53, Mark Brown wrote:
> On Tue, Aug 20, 2024 at 09:48:05AM +0200, Pierre-Louis Bossart wrote:
> 
>>> This feels a lot like it could map onto the regmap async interface, it
>>> would need a bit of a rework (mainly that currently they provide
>>> ordering guarantees with respect to both each other and sync I/O) but
>>> those could be removed with some care) but it's got the "here's a list
>>> of I/O, here's another call to ensure it's all actually happened" thing.
>>> It sounds very much like how I was thinking of the async API when I was
>>> writing it and the initial users.
> 
>>> It's this bit that really got me thinking it could fit well into regmap.
> 
>> Do I get this right that these async capabilities could be used to
>> enable this faster SoundWire protocol, but the regular regmap_write()
>> could still happen in parallel with these async transfers? This could be
>> useful if e.g. there's a jack detection while downloading firmware for
>> another function.
> 
> As far as the regmap core is concerned, yes - with SPI we do wind up
> with ordering but that's because the SPI sync API is a thin wrapper
> around the SPI async API rather than anything regmap does.

ok. I am struggling a bit to adjust the plumbing that was obviously
defined for SPI.

The regmap async_write() doesn't have any wait, but there's a notifier
that needs to be called by *something* that waits. I think the SPI layer
has a set of kthreads but in our case we could just rely on a a
workqueue after 1ms or something and do the wait then for the DMAs to
complete and finally call the notifier to wake-up the regmap stuff.

>> The only thing that would need to be extended is that we'd need
>> additional callbacks to check if the protocol is supported on a given
>> hardware/firmware platform, and if there's no audio stream. In these
>> cases the async writes would be demoted to regular writes. Otherwise the
>> bus would be locked to make sure no reconfiguration takes place while
>> the firmware download happens, and unlocked when the transfer ends.
> 
> Those callbacks seem reasonable.  We already do the demotion to sync for
> buses that don't have async, it'd just need to be made dynamic.

sounds good.

On Tue, Aug 20, 2024 at 04:58:30PM +0200, Pierre-Louis Bossart wrote:
> On 8/20/24 13:53, Mark Brown wrote:

> > As far as the regmap core is concerned, yes - with SPI we do wind up
> > with ordering but that's because the SPI sync API is a thin wrapper
> > around the SPI async API rather than anything regmap does.

> ok. I am struggling a bit to adjust the plumbing that was obviously
> defined for SPI.

> The regmap async_write() doesn't have any wait, but there's a notifier
> that needs to be called by *something* that waits. I think the SPI layer
> has a set of kthreads but in our case we could just rely on a a
> workqueue after 1ms or something and do the wait then for the DMAs to
> complete and finally call the notifier to wake-up the regmap stuff.

From the regmap point of view the expectation is that when the caller
wants to know that all the I/Os have actually happened it should call
regmap_async_complete() which will do the actual blocking.  regmap
itself won't autonomously wait for anything, it's up to the caller to do
that (presumably it will at some point care).