Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SoundWire: start adding BPT/BRA support #5266

Open
wants to merge 16 commits into
base: topic/sof-dev
Choose a base branch
from

Conversation

bardliao
Copy link
Collaborator

@bardliao bardliao commented Dec 6, 2024

This is a rebase version of #4679
I removed the removal of select' statements at a higher level. in the SND_SOC_SOF_HDA_LINK_BASELINE config in af779a3 which I think it is removed by accident.

@bardliao bardliao force-pushed the sdw/start-BRA-support branch from 5d16674 to 1b0b781 Compare December 6, 2024 05:33
drivers/soundwire/stream.c Outdated Show resolved Hide resolved
@bardliao bardliao force-pushed the sdw/start-BRA-support branch from 1b0b781 to 336a2f4 Compare December 24, 2024 07:44
@bardliao bardliao force-pushed the sdw/start-BRA-support branch 4 times, most recently from 4291dd7 to 1eb379b Compare January 6, 2025 12:32
@bardliao bardliao marked this pull request as ready for review January 6, 2025 12:55
sound/soc/sof/intel/hda-sdw-bpt.c Outdated Show resolved Hide resolved
sound/soc/sof/intel/hda-sdw-bpt.c Outdated Show resolved Hide resolved
sound/soc/sof/intel/hda-sdw-bpt.c Show resolved Hide resolved
sound/soc/sof/intel/hda-sdw-bpt.c Show resolved Hide resolved
drivers/soundwire/intel.h Show resolved Hide resolved
sound/soc/sof/intel/hda-sdw-bpt.c Outdated Show resolved Hide resolved
sound/soc/sof/intel/hda-sdw-bpt.c Show resolved Hide resolved
sound/soc/sof/intel/hda-sdw-bpt.c Show resolved Hide resolved
sound/soc/sof/intel/hda-sdw-bpt.c Show resolved Hide resolved
@bardliao bardliao force-pushed the sdw/start-BRA-support branch from 1eb379b to 8debac1 Compare January 7, 2025 07:47
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice update @bardliao, keep up the good work.

Don't forget to add your own Signed-off-by tag.



(2) Multiple data lane support.
(1) Multiple data lane support.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't you provide multi-lane support already?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, that is great.

drivers/soundwire/cadence_master.c Show resolved Hide resolved
drivers/soundwire/cadence_master.c Show resolved Hide resolved
val);

val = pdi->num;
/* The DataPort0 needs to be mapped to both PDI and PDI1 ! */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: PDI0 and PDI1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

drivers/soundwire/cadence_master.c Show resolved Hide resolved
// This file is provided under a dual BSD/GPLv2 license. When using or
// redistributing this file, you may do so under either license.
//
// Copyright(c) 2024 Intel Corporation. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also remove "All rights reserved", not aligned with Intel recommendations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

#include "../sof-priv.h"
#include "hda.h"

#define BPT_FREQUENCY 192000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need explanation of this value, I think it's 9.6MHz * Double data rate factor.
but if you can vary the clock now you'd need to use the max clock before using BRA/BPT...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart I think it is the sampling rate of the HDA stream. But I am not sure how does it related to the SoundWire bus rate. The SDW bus clock and the HDA sampling rate seems to be independent to me.
I guess why 192000 is because that is the max value of rate_bits[].

false);
msleep(1);
i++;
} while (tx_position && i < HDA_BPT_IOC_TIMEOUT_MS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably need an error message if the DMA position was never cleared and the timeout happens?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

sound/soc/sof/intel/hda-sdw-bpt.c Show resolved Hide resolved

ret1 = hda_sdw_bpt_dma_disable(dev, bpt_tx_stream);
if (!ret)
ret = ret1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember if there's any reason to disable RX first?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is because we enable TX first. So to be symmetrical, we disable RX first.

@bardliao bardliao force-pushed the sdw/start-BRA-support branch 2 times, most recently from 470fcc9 to fa54048 Compare January 8, 2025 08:51
@lgirdwood
Copy link
Member

@ujfalusi can you give this a review too. Thanks !

@bardliao bardliao force-pushed the sdw/start-BRA-support branch from fa54048 to dc995f8 Compare January 9, 2025 03:11
ranj063
ranj063 previously approved these changes Jan 9, 2025
Copy link
Collaborator

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bardliao, I think you should add your signed-off-by and if your changes tot he original code warrants it and if @plbossart agrees, probably a Co-developed as well?

@@ -1815,12 +1815,14 @@ static int set_stream(struct snd_pcm_substream *substream,
* sdw_alloc_stream() - Allocate and return stream runtime
*
* @stream_name: SoundWire stream name
* @type: stream type (could be PCM, PDM or BPT)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SDW_STREAM_BPT is yet to be added, we only have definition for PCM and PDM

Copy link
Collaborator Author

@bardliao bardliao Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swapped the commits order with the next commit.

include/linux/soundwire/sdw.h Show resolved Hide resolved
*/
int sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime *stream)
int sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime *stream, bool bpt_stream)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we pass the stream->type as a separate parameter via bpt_stream?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Removed.

@@ -620,6 +697,11 @@ int sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime *stream)
if (ret < 0)
return ret;

if (bpt_stream) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (stream->type == SDW_STREAM_BPT) {

?

drivers/soundwire/stream.c Show resolved Hide resolved

ret = sdw_bpt_send_async(slave->bus,
slave,
&msg);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (ret < 0)
dev_err(&slave->dev, "%s: bpt_wait failed: %d\n", __func__, ret);

dev_dbg(&slave->dev, "wait done\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would check a log and drop debug prints to make it less noisy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, removed

sound/soc/codecs/rt711-sdca-sdw.c Show resolved Hide resolved
sound/soc/sof/intel/hda-sdw-bpt.c Show resolved Hide resolved
sound/soc/sof/intel/hda-sdw-bpt.c Show resolved Hide resolved
@bardliao bardliao force-pushed the sdw/start-BRA-support branch from 9e853e0 to 4e2e71f Compare January 13, 2025 15:03
SDW_PORT_DATA_MODE_NORMAL);
}

sdw_compute_slave_dp0_ports(m_rt, &t_data);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not planning to address this?

if (ret < 0)
return ret;
} else {
if (port_config[i].num)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be

} else if (port_config[i].num) {


int sdw_bpt_wait(struct sdw_bus *bus,
struct sdw_slave *slave,
struct sdw_bpt_msg *msg)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: lots of things can be fit to use less lines, this for example can be in two lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

drivers/soundwire/stream.c Show resolved Hide resolved
include/linux/soundwire/sdw.h Show resolved Hide resolved
include/linux/soundwire/sdw_intel.h Show resolved Hide resolved
*bpt_tx_stream,
dmab_tx_bdl,
*bpt_rx_stream,
dmab_rx_bdl);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks a bit funny when all parameters are in new line...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

return ret;
}

return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can drop the return ret from line 225 and use it here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

struct sdw_cdns_pdi *pdi0;
struct sdw_cdns_pdi *pdi1;
const int ch = 1;
const int num_pdi = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ch and num_pdi is const, why do you need a variable for them?

sound/soc/sof/intel/hda-sdw-bpt.c Outdated Show resolved Hide resolved
@bardliao bardliao force-pushed the sdw/start-BRA-support branch from 4e2e71f to 1811ffb Compare January 14, 2025 14:18
Copy link
Collaborator

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bardliao, not much to comment on the functionality and flow, but can you check the patches for lines that can be folded?

@@ -2038,3 +2038,37 @@ void sdw_clear_slave_status(struct sdw_bus *bus, u32 request)
}
}
EXPORT_SYMBOL(sdw_clear_slave_status);

int sdw_bpt_send_async_and_wait(struct sdw_bus *bus, struct sdw_slave *slave,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iow, sdw_bpt_send_sync() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading the document in the first commit again. I would keep the send async and wait APIs separately.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but adding a helper to do a sync transfer does not mean that the async and wait cannot be used as described while helping users to avoid duplicated code.
The do_bpt_sequence() could be as simple as:

static int do_bpt_sequence(struct sdw_slave *slave, bool write, u8 *buffer)
{
	struct sdw_bpt_msg msg = {0};

	msg.addr = start_addr;
	msg.len = num_bytes;
	msg.dev_num = slave->dev_num;
	if (write)
		msg.flags = SDW_MSG_FLAG_WRITE;
	else
		msg.flags = SDW_MSG_FLAG_READ;
	msg.buf = buffer;

	return sdw_bpt_send_sync(slave->bus, slave, &msg); /* Errors printed in the helper */
}

include/linux/soundwire/sdw.h Show resolved Hide resolved
include/linux/soundwire/sdw.h Show resolved Hide resolved
struct hdac_ext_stream *bpt_tx_stream,
struct hdac_ext_stream *bpt_rx_stream)
{
WARN_ONCE(1, "SoundWire BPT is disabled");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARN will dump stack?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will dump stack.


ret1 = hda_sdw_bpt_dma_deprepare(dev,
bpt_tx_stream,
dmab_tx_bdl);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you still have these spread around, they can fit to one line.

sound/soc/sof/intel/hda-sdw-bpt.c Show resolved Hide resolved
sound/soc/sof/intel/hda-sdw-bpt.c Show resolved Hide resolved
@@ -154,13 +155,32 @@ static int set_command(void *data, u64 value)
add_taint(TAINT_USER, LOCKDEP_STILL_OK);

dev_dbg(&slave->dev, "command: %s\n", value ? "read" : "write");
cmd = value;
cmd = (int)value;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a sparate patch?

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 <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
The register definitions are missing a BULK_ENABLE bitfield which must
be set for DP0.

In addition, the existing mapping from PDI to Data Port is 1:1. That's
fine for PCM streams which are by construction in one direction
only. The BTP/BRA protocol is bidirectional and relies on DP0 only,
which breaks the 1:1 mapping. DP0 MUST be mapped to both PDI0 and
PDI1, with PDI0 taking care of the TX direction and PDI1 of the RX
direction.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
BPT/BRA need to be special cased, i.e. there's no point in using the
bandwidth allocation since the entire frame can be used.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
In the existing definition of sdw_stream_runtime, the 'type' member is
never set and defaults to PCM. To prepare for the BPT/BRA support, we
need to special-case streams and make use of the 'type'.

No functional change for now, the implicit PCM type is now explicit.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
For BPT support, we want to allocate the entire audio payload and
bypass the allocation based on PCM/PDM parameters.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
@bardliao bardliao force-pushed the sdw/start-BRA-support branch from 1811ffb to e297ac6 Compare January 16, 2025 01:43
DP0 (Data Port 0) is very similar to regular data ports, with minor
tweaks we can reuse the same code.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
@bardliao bardliao force-pushed the sdw/start-BRA-support branch 2 times, most recently from c292254 to 6efefed Compare January 16, 2025 03:20
@bardliao
Copy link
Collaborator Author

@bardliao, not much to comment on the functionality and flow, but can you check the patches for lines that can be folded?

I checked and folded the lines. Hope I didn't miss any.

Copy link
Collaborator

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bardliao, I only really have few small comments left..

struct hdac_ext_stream *bpt_rx_stream,
struct snd_dma_buffer *dmab_rx_bdl)
{
WARN_ONCE(1, "SoundWire BPT is disabled");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a single WARN from hda_sdw_bpt_open() is enough to catch the attention?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it is harmless to have WARN in every function.

* Copyright(c) 2025 Intel Corporation.
*/

#include <linux/device.h>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a good practice to protect the header with

#ifndef __HDA_SDW_BPT_H
#define __HDA_SDW_BPT_H
...
#endif /* __HDA_SDW_BPT_H */

@@ -2038,3 +2038,37 @@ void sdw_clear_slave_status(struct sdw_bus *bus, u32 request)
}
}
EXPORT_SYMBOL(sdw_clear_slave_status);

int sdw_bpt_send_async_and_wait(struct sdw_bus *bus, struct sdw_slave *slave,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but adding a helper to do a sync transfer does not mean that the async and wait cannot be used as described while helping users to avoid duplicated code.
The do_bpt_sequence() could be as simple as:

static int do_bpt_sequence(struct sdw_slave *slave, bool write, u8 *buffer)
{
	struct sdw_bpt_msg msg = {0};

	msg.addr = start_addr;
	msg.len = num_bytes;
	msg.dev_num = slave->dev_num;
	if (write)
		msg.flags = SDW_MSG_FLAG_WRITE;
	else
		msg.flags = SDW_MSG_FLAG_READ;
	msg.buf = buffer;

	return sdw_bpt_send_sync(slave->bus, slave, &msg); /* Errors printed in the helper */
}

@@ -196,9 +216,40 @@ static int set_num_bytes(void *data, u64 value)
DEFINE_DEBUGFS_ATTRIBUTE(set_num_bytes_fops, NULL,
set_num_bytes, "%llu\n");

static int do_bpt_sequence(struct sdw_slave *slave, bool write,
u8 *buffer)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can fit to one line ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

plbossart and others added 10 commits January 16, 2025 16:21
Add definitions and helpers for the BPT/BRA protocol. Peripheral
drivers (aka ASoC codec drivers) can use this API to send bulk data
such as firmware or tables. The design intent is however NOT to
directly use this API but to rely on an intermediate regmap layer.

The API is only available when no other audio streams have been
allocated, and only one BTP/BRA stream is allowed per link. To avoid
the addition of yet another lock, the refcount tests are handled in
the stream master_runtime alloc/free routines where the bus_lock is
already held. Another benefit of this approach is that the same
bus_lock is used to handle runtime and port linked lists, which
reduces the potential for misaligned configurations.

In addition to exclusion with audio streams, BPT transfers have a lot
of overhead, specifically registers writes are needed to enable
transport in DP0. Most DMAs don't handle too well very small data sets
and they may have alignment limitations.

The size and alignment requirements are for now not handled by the
core but must be checked by platform-specific drivers.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
Add a convenience pointer to the 'sdw_bus' structure. BPT is a
dedicated stream which will typically not be handled by DAIs or
dailinks. Since there's only one BPT stream per link, storing the
pointer at the link level seems rather natural.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
The Cadence IP expects a specific format (detailed in the
Documentation). Add helpers to copy the data into the DMA buffer.

The crc8 table is for now only used by the Cadence driver. This table
might be moved to a common module at a later point if needed by other
controller implementations.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
Mirror abstraction added for master ops.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
Add SoundWire BPT DMA helpers as a separate module to avoid circular
dependencies.

For now this assumes no link DMA, only coupled mode.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
This is needed to be shared between open/send_async/close.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
Add support for BTP API using Cadence and hda-sdw-bpt helpers.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
When the firmware is involved, the data can be transferred with a
CHAIN_DMA on LNL+.

The CHAIN_DMA needs to be programmed before the DMAs per the
documentation. The states are not exactly symmetrical, on stop we must
do a PAUSE and RESET.

The FIFO size of 10ms was determined experimentally. With the minimum
of 2ms, errors were reported by the codec, likely because of xruns.

The code flow deals with the two TX and RX CHAIN_DMAs in symmetrical
ways, i.e.
alloc TX
alloc RX
enable TX
enable RX
disable RX
disable TX
free RX
free TX

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
Add code to show what codec drivers will need to do to enable BPT/BRA
transfers. The only difference is to set the 'command_type' file to
'1'. A zero-value will rely on regular read/write commands in Column0.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
DP0 is required for BPT/BRA transport.

Signed-off-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
@bardliao bardliao force-pushed the sdw/start-BRA-support branch from 6efefed to 1f40c2e Compare January 16, 2025 08:31
Copy link
Collaborator

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bardliao (and @plbossart ) thank you I think this looks pretty nice!

@bardliao bardliao requested review from plbossart and ranj063 January 16, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants