[alsa-devel] [PATCH 0/8] soundwire: corrections to ACPI and DisCo properties
Now that we are done with cleanups, we can start fixing the code with actual semantic or functional changes.
This patchset applies on top of everything Vinod and I contributed this week.
The fist patch correct a simplifying assumption made earlier. With the IceLake BIOS the existing code is fooled by the presence of a second child device and the namespace walk needs to be filtered. This was not needed on previous generations.
The second patch fixes a long-standing misalignment between code and DisCo specification, preventing MIPI DisCo properties from being parsed successfully.
The third and fourth patch remove restrictions preventing codec drivers from reading DisCo properties.
The fifth patch adds definitions from the SoundWire spec that were missed somehow, but will be very much needed for dynamic bandwidth allocation.
The last 3 patches realign the code with the MIPI specification. The existing code exposes properties that don't exist, or exposes them with ambiguous wording. Sticking to the specification helps avoid interpretation issues for integrators, and will make sure the follow-up sysfs support is self-explanatory.
Parts of this code was initially written by my Intel colleagues Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah, who are either no longer with Intel or no longer involved in SoundWire development. When relevant, I explictly added a note in commit messages to give them credit for their hard work, but I removed their signed-off-by tags to avoid email bounces and avoid spamming them forever with SoundWire patches.
Pierre-Louis Bossart (8): soundwire: intel: filter SoundWire controller device search soundwire: mipi_disco: fix master/link error soundwire: mipi_disco: expose sdw_slave_read_dpn as symbol soundwire: mipi_disco: expose sdw_slave_read_dp0 as symbol soundwire: add port-related definitions soundwire: remove master data port properties soundwire: fix master properties soundwire: rename/clarify MIPI DisCo properties
drivers/soundwire/bus.c | 6 +-- drivers/soundwire/intel.c | 11 ++-- drivers/soundwire/intel_init.c | 19 ++++++- drivers/soundwire/mipi_disco.c | 49 +++++++++--------- drivers/soundwire/stream.c | 8 +-- include/linux/soundwire/sdw.h | 94 +++++++++++++++++++++++++++------- 6 files changed, 132 insertions(+), 55 deletions(-)
The convention is that the SoundWire controller device is a child of the HDAudio controller. However there can be more than one child exposed in the DSDT table, and the current namespace walk returns the last device.
Add a filter and terminate early when a valid _ADR is provided, otherwise keep iterating to find the next child.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/intel_init.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index d3d6b54c5791..f85db67d05f0 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -150,6 +150,12 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, { struct sdw_intel_res *res = cdata; struct acpi_device *adev; + acpi_status status; + u64 adr; + + status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr); + if (ACPI_FAILURE(status)) + return AE_OK; /* keep going */
if (acpi_bus_get_device(handle, &adev)) { pr_err("%s: Couldn't find ACPI handle\n", __func__); @@ -157,7 +163,18 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, }
res->handle = handle; - return AE_OK; + + /* + * On some Intel platforms, multiple children of the HDAS + * device can be found, but only one of them is the SoundWire + * controller. The SNDW device is always exposed with + * Name(_ADR, 0x40000000) so filter accordingly + */ + if (adr != 0x40000000) + return AE_OK; /* keep going */ + + /* device found, stop namespace walk */ + return AE_CTRL_TERMINATE; }
/**
On 03-05-19, 19:29, Pierre-Louis Bossart wrote:
The convention is that the SoundWire controller device is a child of the HDAudio controller. However there can be more than one child exposed in the DSDT table, and the current namespace walk returns the last device.
Add a filter and terminate early when a valid _ADR is provided, otherwise keep iterating to find the next child.
So what are the other devices in DSDT here..
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/intel_init.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c index d3d6b54c5791..f85db67d05f0 100644 --- a/drivers/soundwire/intel_init.c +++ b/drivers/soundwire/intel_init.c @@ -150,6 +150,12 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, { struct sdw_intel_res *res = cdata; struct acpi_device *adev;
acpi_status status;
u64 adr;
status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr);
if (ACPI_FAILURE(status))
return AE_OK; /* keep going */if (acpi_bus_get_device(handle, &adev)) { pr_err("%s: Couldn't find ACPI handle\n", __func__);
@@ -157,7 +163,18 @@ static acpi_status sdw_intel_acpi_cb(acpi_handle handle, u32 level, }
res->handle = handle;
- return AE_OK;
- /*
* On some Intel platforms, multiple children of the HDAS* device can be found, but only one of them is the SoundWire* controller. The SNDW device is always exposed with* Name(_ADR, 0x40000000) so filter accordingly*/- if (adr != 0x40000000)
I do not recall if 4 corresponds to the links you have or soundwire device type, is this number documented somewhere is HDA specs?
Also it might good to create a define for this
return AE_OK; /* keep going */- /* device found, stop namespace walk */
- return AE_CTRL_TERMINATE;
}
/**
2.17.1
On 5/7/19 7:26 AM, Vinod Koul wrote:
On 03-05-19, 19:29, Pierre-Louis Bossart wrote:
The convention is that the SoundWire controller device is a child of the HDAudio controller. However there can be more than one child exposed in the DSDT table, and the current namespace walk returns the last device.
Add a filter and terminate early when a valid _ADR is provided, otherwise keep iterating to find the next child.
So what are the other devices in DSDT here..
this is what I see:
Scope (HDAS) { Device (IDA) { Name (_ADR, 0x00020001) // _ADR: Address } }
I thought this was nonsense but your question triggered me to look into the Intel SST ACPI specs (not public I am afraid but shared with the OS who shall not be named). Using the same source of information as below, I *believe* this is HDaudio related, bits 31..16 mean HDaudio with codec SDI 2, and NodeId 1 for the function group. This would make sense as I believe there are two codecs on the board that can be pin-strapped to boot either in HDaudio or SoundWire mode- but this is a conjecture only.
At any rate, we need a hardware rework and mutual exclusion between HDaudio and SoundWire, so we have to ignore this one when SoundWire is enabled.
- /*
* On some Intel platforms, multiple children of the HDAS* device can be found, but only one of them is the SoundWire* controller. The SNDW device is always exposed with* Name(_ADR, 0x40000000) so filter accordingly*/- if (adr != 0x40000000)
I do not recall if 4 corresponds to the links you have or soundwire device type, is this number documented somewhere is HDA specs?
I thought it was a magic number, but I did check and for once it's documented and the values match the spec :-) I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type and bits 3..0 indicate the SoundWire controller instance, the rest is reserved to zero.
Also it might good to create a define for this
I will respin this one to add the documentation above, and only filter on the 4 ms-bits. Thanks for forcing me to RTFM :-)
On 07-05-19, 09:43, Pierre-Louis Bossart wrote:
On 5/7/19 7:26 AM, Vinod Koul wrote:
On 03-05-19, 19:29, Pierre-Louis Bossart wrote:
The convention is that the SoundWire controller device is a child of the HDAudio controller. However there can be more than one child exposed in the DSDT table, and the current namespace walk returns the last device.
Add a filter and terminate early when a valid _ADR is provided, otherwise keep iterating to find the next child.
So what are the other devices in DSDT here..
this is what I see:
Scope (HDAS) { Device (IDA) { Name (_ADR, 0x00020001) // _ADR: Address } }
I thought this was nonsense but your question triggered me to look into the Intel SST ACPI specs (not public I am afraid but shared with the OS who shall not be named). Using the same source of information as below, I *believe* this is HDaudio related, bits 31..16 mean HDaudio with codec SDI 2, and NodeId 1 for the function group. This would make sense as I believe there are two codecs on the board that can be pin-strapped to boot either in HDaudio or SoundWire mode- but this is a conjecture only.
At any rate, we need a hardware rework and mutual exclusion between HDaudio and SoundWire, so we have to ignore this one when SoundWire is enabled.
That is how I was expecting it to be...
- /*
* On some Intel platforms, multiple children of the HDAS* device can be found, but only one of them is the SoundWire* controller. The SNDW device is always exposed with* Name(_ADR, 0x40000000) so filter accordingly*/- if (adr != 0x40000000)
I do not recall if 4 corresponds to the links you have or soundwire device type, is this number documented somewhere is HDA specs?
I thought it was a magic number, but I did check and for once it's documented and the values match the spec :-) I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type and bits 3..0 indicate the SoundWire controller instance, the rest is reserved to zero.
So in that case we should mask with bits 31..28 and match, who knows you may have multiple controller instances in future I had a vague recollection that this was documented in the spec, glad that in turned out to be the case.
Btw was the update to HDA spec made public?
Also it might good to create a define for this
I will respin this one to add the documentation above, and only filter on the 4 ms-bits. Thanks for forcing me to RTFM :-)
- /*
* On some Intel platforms, multiple children of the HDAS* device can be found, but only one of them is the SoundWire* controller. The SNDW device is always exposed with* Name(_ADR, 0x40000000) so filter accordingly*/- if (adr != 0x40000000)
I do not recall if 4 corresponds to the links you have or soundwire device type, is this number documented somewhere is HDA specs?
I thought it was a magic number, but I did check and for once it's documented and the values match the spec :-) I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type and bits 3..0 indicate the SoundWire controller instance, the rest is reserved to zero.
So in that case we should mask with bits 31..28 and match, who knows you may have multiple controller instances in future
yes, I was planning on only using the link type.
I had a vague recollection that this was documented in the spec, glad that in turned out to be the case.
Btw was the update to HDA spec made public?
Not that I know of. The previous NHLT public doc has actually disappeared from the Intel site and I can't find it any longer, so currently the amount of public documentation is trending to zero :-(
Also it might good to create a define for this
I will respin this one to add the documentation above, and only filter on the 4 ms-bits. Thanks for forcing me to RTFM :-)
On 08-05-19, 11:20, Pierre-Louis Bossart wrote:
- /*
* On some Intel platforms, multiple children of the HDAS* device can be found, but only one of them is the SoundWire* controller. The SNDW device is always exposed with* Name(_ADR, 0x40000000) so filter accordingly*/- if (adr != 0x40000000)
I do not recall if 4 corresponds to the links you have or soundwire device type, is this number documented somewhere is HDA specs?
I thought it was a magic number, but I did check and for once it's documented and the values match the spec :-) I see in the ACPI docs bits 31..28 set to 4 indicate a SoundWire Link Type and bits 3..0 indicate the SoundWire controller instance, the rest is reserved to zero.
So in that case we should mask with bits 31..28 and match, who knows you may have multiple controller instances in future
yes, I was planning on only using the link type.
I had a vague recollection that this was documented in the spec, glad that in turned out to be the case.
Btw was the update to HDA spec made public?
Not that I know of. The previous NHLT public doc has actually disappeared from the Intel site and I can't find it any longer, so currently the amount of public documentation is trending to zero :-(
Also it might good to create a define for this
I will respin this one to add the documentation above, and only filter on the 4 ms-bits. Thanks for forcing me to RTFM :-)
Yeah about that someone was indeed complaining about that on IRC, it is shame that link is valid but doc is gone... check with Rakesh or someone they might have a copy...
The MIPI DisCo specification for SoundWire defines the "mipi-sdw-link-N-subproperties", not the master-N properties. Fix to parse firmware information.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/mipi_disco.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c index c1f51d6a23d2..6df68584c963 100644 --- a/drivers/soundwire/mipi_disco.c +++ b/drivers/soundwire/mipi_disco.c @@ -40,7 +40,7 @@ int sdw_master_read_prop(struct sdw_bus *bus)
/* Find master handle */ snprintf(name, sizeof(name), - "mipi-sdw-master-%d-subproperties", bus->link_id); + "mipi-sdw-link-%d-subproperties", bus->link_id);
link = device_get_named_child_node(bus->dev, name); if (!link) {
On 5/3/19 7:29 PM, Pierre-Louis Bossart wrote:
The MIPI DisCo specification for SoundWire defines the "mipi-sdw-link-N-subproperties", not the master-N properties. Fix to parse firmware information.
Please ignore this patch for now, there is a confusion in the spec itself that needs to be addressed by MIPI... Either there will be an errata issued, or we'll have to try both master- and link-N-properties to reconcile spec and actual implementations.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/mipi_disco.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c index c1f51d6a23d2..6df68584c963 100644 --- a/drivers/soundwire/mipi_disco.c +++ b/drivers/soundwire/mipi_disco.c @@ -40,7 +40,7 @@ int sdw_master_read_prop(struct sdw_bus *bus)
/* Find master handle */ snprintf(name, sizeof(name),
"mipi-sdw-master-%d-subproperties", bus->link_id);
"mipi-sdw-link-%d-subproperties", bus->link_id);link = device_get_named_child_node(bus->dev, name); if (!link) {
sdw_slave_read_dpn was so far a static function, which prevented codec drivers from using it. Expose as non-static function and add symbol
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/mipi_disco.c | 7 ++++--- include/linux/soundwire/sdw.h | 3 +++ 2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c index 6df68584c963..4995554bd6b6 100644 --- a/drivers/soundwire/mipi_disco.c +++ b/drivers/soundwire/mipi_disco.c @@ -161,9 +161,9 @@ static int sdw_slave_read_dp0(struct sdw_slave *slave, return 0; }
-static int sdw_slave_read_dpn(struct sdw_slave *slave, - struct sdw_dpn_prop *dpn, int count, int ports, - char *type) +int sdw_slave_read_dpn(struct sdw_slave *slave, + struct sdw_dpn_prop *dpn, int count, int ports, + char *type) { struct fwnode_handle *node; u32 bit, i = 0; @@ -283,6 +283,7 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
return 0; } +EXPORT_SYMBOL(sdw_slave_read_dpn);
/** * sdw_slave_read_prop() - Read Slave properties diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 35662d9c2c62..04a45225e248 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -345,6 +345,9 @@ struct sdw_master_prop {
int sdw_master_read_prop(struct sdw_bus *bus); int sdw_slave_read_prop(struct sdw_slave *slave); +int sdw_slave_read_dpn(struct sdw_slave *slave, + struct sdw_dpn_prop *dpn, int count, int ports, + char *type);
/* * SDW Slave Structures and APIs
On 03-05-19, 19:29, Pierre-Louis Bossart wrote:
sdw_slave_read_dpn was so far a static function, which prevented codec drivers from using it. Expose as non-static function and add symbol
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/mipi_disco.c | 7 ++++--- include/linux/soundwire/sdw.h | 3 +++ 2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c index 6df68584c963..4995554bd6b6 100644 --- a/drivers/soundwire/mipi_disco.c +++ b/drivers/soundwire/mipi_disco.c @@ -161,9 +161,9 @@ static int sdw_slave_read_dp0(struct sdw_slave *slave, return 0; }
-static int sdw_slave_read_dpn(struct sdw_slave *slave,
struct sdw_dpn_prop *dpn, int count, int ports,char *type)+int sdw_slave_read_dpn(struct sdw_slave *slave,
struct sdw_dpn_prop *dpn, int count, int ports,char *type){ struct fwnode_handle *node; u32 bit, i = 0; @@ -283,6 +283,7 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
return 0; } +EXPORT_SYMBOL(sdw_slave_read_dpn);
Fine to export but would be great to accompany user with it. In general do not add a API without user.
sdw_slave_read_dp0 was so far a static function, which prevented codec drivers from using it. Expose as non-static function and add symbol
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/mipi_disco.c | 7 ++++--- include/linux/soundwire/sdw.h | 3 +++ 2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c index 4995554bd6b6..f6b1be920a19 100644 --- a/drivers/soundwire/mipi_disco.c +++ b/drivers/soundwire/mipi_disco.c @@ -121,9 +121,9 @@ int sdw_master_read_prop(struct sdw_bus *bus) } EXPORT_SYMBOL(sdw_master_read_prop);
-static int sdw_slave_read_dp0(struct sdw_slave *slave, - struct fwnode_handle *port, - struct sdw_dp0_prop *dp0) +int sdw_slave_read_dp0(struct sdw_slave *slave, + struct fwnode_handle *port, + struct sdw_dp0_prop *dp0) { int nval;
@@ -160,6 +160,7 @@ static int sdw_slave_read_dp0(struct sdw_slave *slave,
return 0; } +EXPORT_SYMBOL(sdw_slave_read_dp0);
int sdw_slave_read_dpn(struct sdw_slave *slave, struct sdw_dpn_prop *dpn, int count, int ports, diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 04a45225e248..594c17ed8593 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -348,6 +348,9 @@ int sdw_slave_read_prop(struct sdw_slave *slave); int sdw_slave_read_dpn(struct sdw_slave *slave, struct sdw_dpn_prop *dpn, int count, int ports, char *type); +int sdw_slave_read_dp0(struct sdw_slave *slave, + struct fwnode_handle *port, + struct sdw_dp0_prop *dp0);
/* * SDW Slave Structures and APIs
Somehow previous header files did not include definition for sink/source, flow and grouping.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- include/linux/soundwire/sdw.h | 53 +++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 594c17ed8593..bd05a85d345c 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -41,6 +41,31 @@ struct sdw_slave; #define SDW_DAI_ID_RANGE_START 100 #define SDW_DAI_ID_RANGE_END 200
+enum { + SDW_PORT_DIRN_SINK = 0, + SDW_PORT_DIRN_SOURCE, + SDW_PORT_DIRN_MAX, +}; + +/* + * constants for flow control, ports and transport + * + * these are bit masks as devices can have multiple capabilities + */ + +/* + * flow modes for SDW port. These can be isochronous, tx controlled, + * rx controlled or async + */ +#define SDW_PORT_FLOW_MODE_ISOCH 0 +#define SDW_PORT_FLOW_MODE_TX_CNTRL BIT(0) +#define SDW_PORT_FLOW_MODE_RX_CNTRL BIT(1) +#define SDW_PORT_FLOW_MODE_ASYNC GENMASK(1, 0) + +/* sample packaging for block. It can be per port or per channel */ +#define SDW_BLOCK_PACKG_PER_PORT BIT(0) +#define SDW_BLOCK_PACKG_PER_CH BIT(1) + /** * enum sdw_slave_status - Slave status * @SDW_SLAVE_UNATTACHED: Slave is not attached with the bus. @@ -76,6 +101,14 @@ enum sdw_command_response { SDW_CMD_FAIL_OTHER = 4, };
+/* block group count enum */ +enum sdw_dpn_grouping { + SDW_BLK_GRP_CNT_1 = 0, + SDW_BLK_GRP_CNT_2 = 1, + SDW_BLK_GRP_CNT_3 = 2, + SDW_BLK_GRP_CNT_4 = 3, +}; + /** * enum sdw_stream_type: data stream type * @@ -100,6 +133,26 @@ enum sdw_data_direction { SDW_DATA_DIR_TX = 1, };
+/** + * enum sdw_port_data_mode: Data Port mode + * + * @SDW_PORT_DATA_MODE_NORMAL: Normal data mode where audio data is received + * and transmitted. + * @SDW_PORT_DATA_MODE_STATIC_1: Simple test mode which uses static value of + * logic 1. The encoding will result in signal transitions at every bitslot + * owned by this Port + * @SDW_PORT_DATA_MODE_STATIC_0: Simple test mode which uses static value of + * logic 0. The encoding will result in no signal transitions + * @SDW_PORT_DATA_MODE_PRBS: Test mode which uses a PRBS generator to produce + * a pseudo random data pattern that is transferred + */ +enum sdw_port_data_mode { + SDW_PORT_DATA_MODE_NORMAL = 0, + SDW_PORT_DATA_MODE_STATIC_1 = 1, + SDW_PORT_DATA_MODE_STATIC_0 = 2, + SDW_PORT_DATA_MODE_PRBS = 3, +}; + /* * SDW properties, defined in MIPI DisCo spec v1.0 */
The SoundWire and DisCo specifications do not define Master data ports or related properties. Data ports are only defined for Slave devices, so remove the unused member in properties.
Credits: this patch is based on an earlier internal contribution by Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- include/linux/soundwire/sdw.h | 2 -- 1 file changed, 2 deletions(-)
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index bd05a85d345c..80584e9d5970 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -377,7 +377,6 @@ struct sdw_slave_prop { * @dynamic_frame: Dynamic frame supported * @err_threshold: Number of times that software may retry sending a single * command - * @dpn_prop: Data Port N properties */ struct sdw_master_prop { u32 revision; @@ -393,7 +392,6 @@ struct sdw_master_prop { u32 default_col; bool dynamic_frame; u32 err_threshold; - struct sdw_dpn_prop *dpn_prop; };
int sdw_master_read_prop(struct sdw_bus *bus);
The master-count is only defined for a controller or a Slave in the MIPI DisCo for SoundWire document.
rename all fields with 'freq' as 'clk_freq' to follow the MIPI specification and avoid confusion between bus clock and audio clocks.
fix support for clock_stop_mode0 and 1. The existing code uses a bitmask between enums, one of which being zero. Or'ing with zero is not very useful in general...Fix by or-ing with a BIT dependent on the enum value.
Fix additional comments to align with MIPI spec
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/bus.c | 4 ++-- drivers/soundwire/intel.c | 11 ++++++----- drivers/soundwire/mipi_disco.c | 27 ++++++++++++++------------- drivers/soundwire/stream.c | 2 +- include/linux/soundwire/sdw.h | 20 +++++++++----------- 5 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index aac35fc3cf22..96e42df8f458 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -87,7 +87,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
/* * Initialize clock values based on Master properties. The max - * frequency is read from max_freq property. Current assumption + * frequency is read from max_clk_freq property. Current assumption * is that the bus will start at highest clock frequency when * powered on. * @@ -95,7 +95,7 @@ int sdw_add_bus_master(struct sdw_bus *bus) * to start with bank 0 (Table 40 of Spec) */ prop = &bus->prop; - bus->params.max_dr_freq = prop->max_freq * SDW_DOUBLE_RATE_FACTOR; + bus->params.max_dr_freq = prop->max_clk_freq * SDW_DOUBLE_RATE_FACTOR; bus->params.curr_dr_freq = bus->params.max_dr_freq; bus->params.curr_bank = SDW_BANK0; bus->params.next_bank = SDW_BANK1; diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 31336b0271b0..4ac141730b13 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -796,13 +796,14 @@ static int intel_prop_read(struct sdw_bus *bus) sdw_master_read_prop(bus);
/* BIOS is not giving some values correctly. So, lets override them */ - bus->prop.num_freq = 1; - bus->prop.freq = devm_kcalloc(bus->dev, bus->prop.num_freq, - sizeof(*bus->prop.freq), GFP_KERNEL); - if (!bus->prop.freq) + bus->prop.num_clk_freq = 1; + bus->prop.clk_freq = devm_kcalloc(bus->dev, bus->prop.num_clk_freq, + sizeof(*bus->prop.clk_freq), + GFP_KERNEL); + if (!bus->prop.clk_freq) return -ENOMEM;
- bus->prop.freq[0] = bus->prop.max_freq; + bus->prop.clk_freq[0] = bus->prop.max_clk_freq; bus->prop.err_threshold = 5;
return 0; diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c index f6b1be920a19..7db816691393 100644 --- a/drivers/soundwire/mipi_disco.c +++ b/drivers/soundwire/mipi_disco.c @@ -50,39 +50,40 @@ int sdw_master_read_prop(struct sdw_bus *bus)
if (fwnode_property_read_bool(link, "mipi-sdw-clock-stop-mode0-supported")) - prop->clk_stop_mode = SDW_CLK_STOP_MODE0; + prop->clk_stop_modes |= BIT(SDW_CLK_STOP_MODE0);
if (fwnode_property_read_bool(link, "mipi-sdw-clock-stop-mode1-supported")) - prop->clk_stop_mode |= SDW_CLK_STOP_MODE1; + prop->clk_stop_modes |= BIT(SDW_CLK_STOP_MODE1);
fwnode_property_read_u32(link, "mipi-sdw-max-clock-frequency", - &prop->max_freq); + &prop->max_clk_freq);
nval = fwnode_property_read_u32_array(link, "mipi-sdw-clock-frequencies-supported", NULL, 0); if (nval > 0) { - prop->num_freq = nval; - prop->freq = devm_kcalloc(bus->dev, prop->num_freq, - sizeof(*prop->freq), GFP_KERNEL); - if (!prop->freq) + prop->num_clk_freq = nval; + prop->clk_freq = devm_kcalloc(bus->dev, prop->num_clk_freq, + sizeof(*prop->clk_freq), + GFP_KERNEL); + if (!prop->clk_freq) return -ENOMEM;
fwnode_property_read_u32_array(link, "mipi-sdw-clock-frequencies-supported", - prop->freq, prop->num_freq); + prop->clk_freq, prop->num_clk_freq); }
/* * Check the frequencies supported. If FW doesn't provide max * freq, then populate here by checking values. */ - if (!prop->max_freq && prop->freq) { - prop->max_freq = prop->freq[0]; - for (i = 1; i < prop->num_freq; i++) { - if (prop->freq[i] > prop->max_freq) - prop->max_freq = prop->freq[i]; + if (!prop->max_clk_freq && prop->clk_freq) { + prop->max_clk_freq = prop->clk_freq[0]; + for (i = 1; i < prop->num_clk_freq; i++) { + if (prop->clk_freq[i] > prop->max_clk_freq) + prop->max_clk_freq = prop->clk_freq[i]; } }
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index d01060dbee96..89edc897b8eb 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1474,7 +1474,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) memcpy(¶ms, &bus->params, sizeof(params));
/* TODO: Support Asynchronous mode */ - if ((prop->max_freq % stream->params.rate) != 0) { + if ((prop->max_clk_freq % stream->params.rate) != 0) { dev_err(bus->dev, "Async mode not supported\n"); return -EINVAL; } diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 80584e9d5970..89c51838b7ec 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -364,29 +364,27 @@ struct sdw_slave_prop { /** * struct sdw_master_prop - Master properties * @revision: MIPI spec version of the implementation - * @master_count: Number of masters - * @clk_stop_mode: Bitmap for Clock Stop modes supported - * @max_freq: Maximum Bus clock frequency, in Hz + * @clk_stop_modes: Bitmap, bit N set when clock-stop-modeN supported + * @max_clk_freq: Maximum Bus clock frequency, in Hz * @num_clk_gears: Number of clock gears supported * @clk_gears: Clock gears supported - * @num_freq: Number of clock frequencies supported, in Hz - * @freq: Clock frequencies supported, in Hz + * @num_clk_freq: Number of clock frequencies supported, in Hz + * @clk_freq: Clock frequencies supported, in Hz * @default_frame_rate: Controller default Frame rate, in Hz * @default_row: Number of rows * @default_col: Number of columns - * @dynamic_frame: Dynamic frame supported + * @dynamic_frame: Dynamic frame shape supported * @err_threshold: Number of times that software may retry sending a single * command */ struct sdw_master_prop { u32 revision; - u32 master_count; - enum sdw_clk_stop_mode clk_stop_mode; - u32 max_freq; + u32 clk_stop_modes; + u32 max_clk_freq; u32 num_clk_gears; u32 *clk_gears; - u32 num_freq; - u32 *freq; + u32 num_clk_freq; + u32 *clk_freq; u32 default_frame_rate; u32 default_row; u32 default_col;
On 03-05-19, 19:29, Pierre-Louis Bossart wrote:
The master-count is only defined for a controller or a Slave in the MIPI DisCo for SoundWire document.
... so remove it
rename all fields with 'freq' as 'clk_freq' to follow the MIPI specification and avoid confusion between bus clock and audio clocks.
That sounds good to me.
fix support for clock_stop_mode0 and 1. The existing code uses a bitmask between enums, one of which being zero. Or'ing with zero is not very useful in general...Fix by or-ing with a BIT dependent on the enum value.
Fix additional comments to align with MIPI spec
Ideally these should be different patches...
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
drivers/soundwire/bus.c | 4 ++-- drivers/soundwire/intel.c | 11 ++++++----- drivers/soundwire/mipi_disco.c | 27 ++++++++++++++------------- drivers/soundwire/stream.c | 2 +- include/linux/soundwire/sdw.h | 20 +++++++++----------- 5 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index aac35fc3cf22..96e42df8f458 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -87,7 +87,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
/* * Initialize clock values based on Master properties. The max
* frequency is read from max_freq property. Current assumption
* frequency is read from max_clk_freq property. Current assumption
- is that the bus will start at highest clock frequency when
- powered on.
@@ -95,7 +95,7 @@ int sdw_add_bus_master(struct sdw_bus *bus) * to start with bank 0 (Table 40 of Spec) */ prop = &bus->prop;
- bus->params.max_dr_freq = prop->max_freq * SDW_DOUBLE_RATE_FACTOR;
- bus->params.max_dr_freq = prop->max_clk_freq * SDW_DOUBLE_RATE_FACTOR; bus->params.curr_dr_freq = bus->params.max_dr_freq; bus->params.curr_bank = SDW_BANK0; bus->params.next_bank = SDW_BANK1;
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c index 31336b0271b0..4ac141730b13 100644 --- a/drivers/soundwire/intel.c +++ b/drivers/soundwire/intel.c @@ -796,13 +796,14 @@ static int intel_prop_read(struct sdw_bus *bus) sdw_master_read_prop(bus);
/* BIOS is not giving some values correctly. So, lets override them */
- bus->prop.num_freq = 1;
- bus->prop.freq = devm_kcalloc(bus->dev, bus->prop.num_freq,
sizeof(*bus->prop.freq), GFP_KERNEL);- if (!bus->prop.freq)
- bus->prop.num_clk_freq = 1;
- bus->prop.clk_freq = devm_kcalloc(bus->dev, bus->prop.num_clk_freq,
sizeof(*bus->prop.clk_freq),GFP_KERNEL);- if (!bus->prop.clk_freq) return -ENOMEM;
- bus->prop.freq[0] = bus->prop.max_freq;
bus->prop.clk_freq[0] = bus->prop.max_clk_freq; bus->prop.err_threshold = 5;
return 0;
diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c index f6b1be920a19..7db816691393 100644 --- a/drivers/soundwire/mipi_disco.c +++ b/drivers/soundwire/mipi_disco.c @@ -50,39 +50,40 @@ int sdw_master_read_prop(struct sdw_bus *bus)
if (fwnode_property_read_bool(link, "mipi-sdw-clock-stop-mode0-supported"))
prop->clk_stop_mode = SDW_CLK_STOP_MODE0;
prop->clk_stop_modes |= BIT(SDW_CLK_STOP_MODE0);if (fwnode_property_read_bool(link, "mipi-sdw-clock-stop-mode1-supported"))
prop->clk_stop_mode |= SDW_CLK_STOP_MODE1;
prop->clk_stop_modes |= BIT(SDW_CLK_STOP_MODE1);fwnode_property_read_u32(link, "mipi-sdw-max-clock-frequency",
&prop->max_freq);
&prop->max_clk_freq);nval = fwnode_property_read_u32_array(link, "mipi-sdw-clock-frequencies-supported", NULL, 0); if (nval > 0) {
prop->num_freq = nval;prop->freq = devm_kcalloc(bus->dev, prop->num_freq,sizeof(*prop->freq), GFP_KERNEL);if (!prop->freq)
prop->num_clk_freq = nval;prop->clk_freq = devm_kcalloc(bus->dev, prop->num_clk_freq,sizeof(*prop->clk_freq),GFP_KERNEL);if (!prop->clk_freq) return -ENOMEM;fwnode_property_read_u32_array(link, "mipi-sdw-clock-frequencies-supported",
prop->freq, prop->num_freq);
prop->clk_freq, prop->num_clk_freq);}
/*
- Check the frequencies supported. If FW doesn't provide max
- freq, then populate here by checking values.
*/
- if (!prop->max_freq && prop->freq) {
prop->max_freq = prop->freq[0];for (i = 1; i < prop->num_freq; i++) {if (prop->freq[i] > prop->max_freq)prop->max_freq = prop->freq[i];
- if (!prop->max_clk_freq && prop->clk_freq) {
prop->max_clk_freq = prop->clk_freq[0];for (i = 1; i < prop->num_clk_freq; i++) {if (prop->clk_freq[i] > prop->max_clk_freq)prop->max_clk_freq = prop->clk_freq[i];diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index d01060dbee96..89edc897b8eb 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -1474,7 +1474,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream) memcpy(¶ms, &bus->params, sizeof(params));
/* TODO: Support Asynchronous mode */
if ((prop->max_freq % stream->params.rate) != 0) {
if ((prop->max_clk_freq % stream->params.rate) != 0) { dev_err(bus->dev, "Async mode not supported\n"); return -EINVAL;diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 80584e9d5970..89c51838b7ec 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -364,29 +364,27 @@ struct sdw_slave_prop { /**
- struct sdw_master_prop - Master properties
- @revision: MIPI spec version of the implementation
- @master_count: Number of masters
- @clk_stop_mode: Bitmap for Clock Stop modes supported
- @max_freq: Maximum Bus clock frequency, in Hz
- @clk_stop_modes: Bitmap, bit N set when clock-stop-modeN supported
- @max_clk_freq: Maximum Bus clock frequency, in Hz
- @num_clk_gears: Number of clock gears supported
- @clk_gears: Clock gears supported
- @num_freq: Number of clock frequencies supported, in Hz
- @freq: Clock frequencies supported, in Hz
- @num_clk_freq: Number of clock frequencies supported, in Hz
- @clk_freq: Clock frequencies supported, in Hz
- @default_frame_rate: Controller default Frame rate, in Hz
- @default_row: Number of rows
- @default_col: Number of columns
- @dynamic_frame: Dynamic frame supported
*/
- @dynamic_frame: Dynamic frame shape supported
- @err_threshold: Number of times that software may retry sending a single
- command
struct sdw_master_prop { u32 revision;
- u32 master_count;
- enum sdw_clk_stop_mode clk_stop_mode;
- u32 max_freq;
- u32 clk_stop_modes;
- u32 max_clk_freq; u32 num_clk_gears; u32 *clk_gears;
- u32 num_freq;
- u32 *freq;
- u32 num_clk_freq;
- u32 *clk_freq; u32 default_frame_rate; u32 default_row; u32 default_col;
-- 2.17.1
The existing definitions are ambiguous and possibly misleading.
For DP0, 'flow-control' is only relevant for the BRA protocol and should not be confused with async modes explicitly not supported for DP0, add prefix to follow MIPI DisCo definition
The use of 'device_interrupts' is also questionable. The MIPI SoundWire spec defines Slave-, DP0- and DPN-level implementation-defined interrupts. Using the 'device' prefix in the last two cases is misleading, not only is the term 'device' overloaded but these properties are only valid at the DP0 and DPn levels. Rename to follow the MIPI definitions, no need to be creative here.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/soundwire/bus.c | 2 +- drivers/soundwire/mipi_disco.c | 6 +++--- drivers/soundwire/stream.c | 6 +++--- include/linux/soundwire/sdw.h | 13 +++++++------ 4 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 96e42df8f458..fe745830a261 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -648,7 +648,7 @@ static int sdw_initialize_slave(struct sdw_slave *slave) return 0;
/* Enable DP0 interrupts */ - val = prop->dp0_prop->device_interrupts; + val = prop->dp0_prop->imp_def_interrupts; val |= SDW_DP0_INT_PORT_READY | SDW_DP0_INT_BRA_FAILURE;
ret = sdw_update(slave, SDW_DP0_INTMASK, val, val); diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c index 7db816691393..a065cba8c2c5 100644 --- a/drivers/soundwire/mipi_disco.c +++ b/drivers/soundwire/mipi_disco.c @@ -150,13 +150,13 @@ int sdw_slave_read_dp0(struct sdw_slave *slave, dp0->words, dp0->num_words); }
- dp0->flow_controlled = fwnode_property_read_bool(port, + dp0->BRA_flow_controlled = fwnode_property_read_bool(port, "mipi-sdw-bra-flow-controlled");
dp0->simple_ch_prep_sm = fwnode_property_read_bool(port, "mipi-sdw-simplified-channel-prepare-sm");
- dp0->device_interrupts = fwnode_property_read_bool(port, + dp0->imp_def_interrupts = fwnode_property_read_bool(port, "mipi-sdw-imp-def-dp0-interrupts-supported");
return 0; @@ -226,7 +226,7 @@ int sdw_slave_read_dpn(struct sdw_slave *slave,
fwnode_property_read_u32(node, "mipi-sdw-imp-def-dpn-interrupts-supported", - &dpn[i].device_interrupts); + &dpn[i].imp_def_interrupts);
fwnode_property_read_u32(node, "mipi-sdw-min-channel-number", &dpn[i].min_ch); diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 89edc897b8eb..ce9cb7fa4724 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -439,7 +439,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
prep_ch.bank = bus->params.next_bank;
- if (dpn_prop->device_interrupts || !dpn_prop->simple_ch_prep_sm) + if (dpn_prop->imp_def_interrupts || !dpn_prop->simple_ch_prep_sm) intr = true;
/* @@ -449,7 +449,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus, */ if (prep && intr) { ret = sdw_configure_dpn_intr(s_rt->slave, p_rt->num, prep, - dpn_prop->device_interrupts); + dpn_prop->imp_def_interrupts); if (ret < 0) return ret; } @@ -493,7 +493,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus, /* Disable interrupt after Port de-prepare */ if (!prep && intr) ret = sdw_configure_dpn_intr(s_rt->slave, p_rt->num, prep, - dpn_prop->device_interrupts); + dpn_prop->imp_def_interrupts);
return ret; } diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 89c51838b7ec..3b231472464a 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -206,10 +206,11 @@ enum sdw_clk_stop_mode { * (inclusive) * @num_words: number of wordlengths supported * @words: wordlengths supported - * @flow_controlled: Slave implementation results in an OK_NotReady + * @BRA_flow_controlled: Slave implementation results in an OK_NotReady * response * @simple_ch_prep_sm: If channel prepare sequence is required - * @device_interrupts: If implementation-defined interrupts are supported + * @imp_def_interrupts: If set, each bit corresponds to support for + * implementation-defined interrupts * * The wordlengths are specified by Spec as max, min AND number of * discrete values, implementation can define based on the wordlengths they @@ -220,9 +221,9 @@ struct sdw_dp0_prop { u32 min_word; u32 num_words; u32 *words; - bool flow_controlled; + bool BRA_flow_controlled; bool simple_ch_prep_sm; - bool device_interrupts; + bool imp_def_interrupts; };
/** @@ -272,7 +273,7 @@ struct sdw_dpn_audio_mode { * @simple_ch_prep_sm: If the port supports simplified channel prepare state * machine * @ch_prep_timeout: Port-specific timeout value, in milliseconds - * @device_interrupts: If set, each bit corresponds to support for + * @imp_def_interrupts: If set, each bit corresponds to support for * implementation-defined interrupts * @max_ch: Maximum channels supported * @min_ch: Minimum channels supported @@ -297,7 +298,7 @@ struct sdw_dpn_prop { u32 max_grouping; bool simple_ch_prep_sm; u32 ch_prep_timeout; - u32 device_interrupts; + u32 imp_def_interrupts; u32 max_ch; u32 min_ch; u32 num_ch;
participants (2)
-
Pierre-Louis Bossart -
Vinod Koul