[PATCH 0/2] soundwire: Remove redundant zeroing of page registers
Writing zero to the page registers after each message transaction can add up to a lot of overhead for codecs that need to transfer large amount of data - for example a firmware download.
There's no spec reason I can see for this zeroing. The page registers are only used for a paged address. The bus code uses a non-paged address for registers in page 0. It always writes the page registers at the start of a paged transaction.
If this zeroing was a workaround for anything, let me know and I will re-implement the zeroing as a quirk that can be enabled only when it is necessary.
Richard Fitzgerald (2): soundwire: bus: Don't zero page registers after every transaction soundwire: bus: Remove unused reset_page_addr() callback
drivers/soundwire/bus.c | 23 ----------------------- drivers/soundwire/cadence_master.c | 14 -------------- drivers/soundwire/cadence_master.h | 3 --- drivers/soundwire/intel_auxdevice.c | 1 - include/linux/soundwire/sdw.h | 3 --- 5 files changed, 44 deletions(-)
Zeroing the page registers at the end of every paged transaction is just overhead (40% overhead on a 1-register access, 25% on a 4-register transaction). According to the spec the peripheral should only use the values in the page registers if the address is paged, and since the page registers are always overwritten at the start of a paged transaction there will never be a transaction that uses the stale values from a previous paged transaction.
For peripherals that need large amounts of data to be transferred, for example firmware or filter coefficients, the overhead of page register zeroing can become quite significant.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- drivers/soundwire/bus.c | 23 ----------------------- 1 file changed, 23 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 76515c33e639..a02edcbfc282 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -247,23 +247,6 @@ static inline int do_transfer_defer(struct sdw_bus *bus, return ret; }
-static int sdw_reset_page(struct sdw_bus *bus, u16 dev_num) -{ - int retry = bus->prop.err_threshold; - enum sdw_command_response resp; - int ret = 0, i; - - for (i = 0; i <= retry; i++) { - resp = bus->ops->reset_page_addr(bus, dev_num); - ret = find_response_code(resp); - /* if cmd is ok or ignored return */ - if (ret == 0 || ret == -ENODATA) - return ret; - } - - return ret; -} - static int sdw_transfer_unlocked(struct sdw_bus *bus, struct sdw_msg *msg) { int ret; @@ -275,9 +258,6 @@ static int sdw_transfer_unlocked(struct sdw_bus *bus, struct sdw_msg *msg) (msg->flags & SDW_MSG_FLAG_WRITE) ? "write" : "read", msg->addr, msg->len);
- if (msg->page) - sdw_reset_page(bus, msg->dev_num); - return ret; }
@@ -352,9 +332,6 @@ int sdw_transfer_defer(struct sdw_bus *bus, struct sdw_msg *msg, dev_err(bus->dev, "Defer trf on Slave %d failed:%d\n", msg->dev_num, ret);
- if (msg->page) - sdw_reset_page(bus, msg->dev_num); - return ret; }
A previous patch removed unnecessary zeroing of the page registers after a paged transaction, so now the reset_page_addr callback is unused and can be removed.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- drivers/soundwire/cadence_master.c | 14 -------------- drivers/soundwire/cadence_master.h | 3 --- drivers/soundwire/intel_auxdevice.c | 1 - include/linux/soundwire/sdw.h | 3 --- 4 files changed, 21 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 6bffecf3d61a..1c4f8dea57f2 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -769,20 +769,6 @@ cdns_xfer_msg_defer(struct sdw_bus *bus, } EXPORT_SYMBOL(cdns_xfer_msg_defer);
-enum sdw_command_response -cdns_reset_page_addr(struct sdw_bus *bus, unsigned int dev_num) -{ - struct sdw_cdns *cdns = bus_to_cdns(bus); - struct sdw_msg msg; - - /* Create dummy message with valid device number */ - memset(&msg, 0, sizeof(msg)); - msg.dev_num = dev_num; - - return cdns_program_scp_addr(cdns, &msg); -} -EXPORT_SYMBOL(cdns_reset_page_addr); - u32 cdns_read_ping_status(struct sdw_bus *bus) { struct sdw_cdns *cdns = bus_to_cdns(bus); diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index c2d817e8e22a..064fe38fe7f0 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -172,9 +172,6 @@ struct sdw_cdns_pdi *sdw_cdns_alloc_pdi(struct sdw_cdns *cdns, void sdw_cdns_config_stream(struct sdw_cdns *cdns, u32 ch, u32 dir, struct sdw_cdns_pdi *pdi);
-enum sdw_command_response -cdns_reset_page_addr(struct sdw_bus *bus, unsigned int dev_num); - enum sdw_command_response cdns_xfer_msg(struct sdw_bus *bus, struct sdw_msg *msg);
diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c index 96c6b2112feb..5021be0f4158 100644 --- a/drivers/soundwire/intel_auxdevice.c +++ b/drivers/soundwire/intel_auxdevice.c @@ -113,7 +113,6 @@ static struct sdw_master_ops sdw_intel_ops = { .override_adr = sdw_dmi_override_adr, .xfer_msg = cdns_xfer_msg, .xfer_msg_defer = cdns_xfer_msg_defer, - .reset_page_addr = cdns_reset_page_addr, .set_bus_conf = cdns_bus_conf, .pre_bank_switch = generic_pre_bank_switch, .post_bank_switch = generic_post_bank_switch, diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index 9e4537f409c2..208faf3c886a 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -835,7 +835,6 @@ struct sdw_defer { * @override_adr: Override value read from firmware (quirk for buggy firmware) * @xfer_msg: Transfer message callback * @xfer_msg_defer: Defer version of transfer message callback - * @reset_page_addr: Reset the SCP page address registers * @set_bus_conf: Set the bus configuration * @pre_bank_switch: Callback for pre bank switch * @post_bank_switch: Callback for post bank switch @@ -851,8 +850,6 @@ struct sdw_master_ops { enum sdw_command_response (*xfer_msg_defer) (struct sdw_bus *bus, struct sdw_msg *msg, struct sdw_defer *defer); - enum sdw_command_response (*reset_page_addr) - (struct sdw_bus *bus, unsigned int dev_num); int (*set_bus_conf)(struct sdw_bus *bus, struct sdw_bus_params *params); int (*pre_bank_switch)(struct sdw_bus *bus);
On 12/1/22 08:08, Richard Fitzgerald wrote:
Writing zero to the page registers after each message transaction can add up to a lot of overhead for codecs that need to transfer large amount of data - for example a firmware download.
There's no spec reason I can see for this zeroing. The page registers are only used for a paged address. The bus code uses a non-paged address for registers in page 0. It always writes the page registers at the start of a paged transaction.
If this zeroing was a workaround for anything, let me know and I will re-implement the zeroing as a quirk that can be enabled only when it is necessary.
It's a feature, not a bug :-)
The page registers have to be zeroed out so that any bus-management command hits the page0 instead of using a value that was set by codec driver for vendor-specific configurations.
The implementation is far from optimal though, and indeed if we have long transactions that are not interrupted by anything else we could avoid resetting the page registers.
I tried to implement a 'lazy approach' some time back, but at the time I didn't see any benefits due to the limited number of configurations.
I can't remember where the code is, but the initial enhancement was listed here: https://github.com/thesofproject/linux/issues/2881
Richard Fitzgerald (2): soundwire: bus: Don't zero page registers after every transaction soundwire: bus: Remove unused reset_page_addr() callback
drivers/soundwire/bus.c | 23 ----------------------- drivers/soundwire/cadence_master.c | 14 -------------- drivers/soundwire/cadence_master.h | 3 --- drivers/soundwire/intel_auxdevice.c | 1 - include/linux/soundwire/sdw.h | 3 --- 5 files changed, 44 deletions(-)
On 01/12/2022 18:31, Pierre-Louis Bossart wrote:
On 12/1/22 08:08, Richard Fitzgerald wrote:
Writing zero to the page registers after each message transaction can add up to a lot of overhead for codecs that need to transfer large amount of data - for example a firmware download.
There's no spec reason I can see for this zeroing. The page registers are only used for a paged address. The bus code uses a non-paged address for registers in page 0. It always writes the page registers at the start of a paged transaction.
If this zeroing was a workaround for anything, let me know and I will re-implement the zeroing as a quirk that can be enabled only when it is necessary.
It's a feature, not a bug :-)
The page registers have to be zeroed out so that any bus-management command hits the page0 instead of using a value that was set by codec driver for vendor-specific configurations.
Why would these bus management commands set bit 15 to indicate a paged access? If they don't set bit 15 the page registers are not used and bits 15..31 of the register address must be 0. Table 78 in the Soundwire 1.2 spec. Table 71 in the 1.0 spec. Table 43 in the 0.6 draft spec.
The implementation is far from optimal though, and indeed if we have long transactions that are not interrupted by anything else we could avoid resetting the page registers.
I tried to implement a 'lazy approach' some time back, but at the time I didn't see any benefits due to the limited number of configurations.
I can't remember where the code is, but the initial enhancement was listed here: https://github.com/thesofproject/linux/issues/2881
Richard Fitzgerald (2): soundwire: bus: Don't zero page registers after every transaction soundwire: bus: Remove unused reset_page_addr() callback
drivers/soundwire/bus.c | 23 ----------------------- drivers/soundwire/cadence_master.c | 14 -------------- drivers/soundwire/cadence_master.h | 3 --- drivers/soundwire/intel_auxdevice.c | 1 - include/linux/soundwire/sdw.h | 3 --- 5 files changed, 44 deletions(-)
On 12/2/22 05:26, Richard Fitzgerald wrote:
On 01/12/2022 18:31, Pierre-Louis Bossart wrote:
On 12/1/22 08:08, Richard Fitzgerald wrote:
Writing zero to the page registers after each message transaction can add up to a lot of overhead for codecs that need to transfer large amount of data - for example a firmware download.
There's no spec reason I can see for this zeroing. The page registers are only used for a paged address. The bus code uses a non-paged address for registers in page 0. It always writes the page registers at the start of a paged transaction.
If this zeroing was a workaround for anything, let me know and I will re-implement the zeroing as a quirk that can be enabled only when it is necessary.
It's a feature, not a bug :-)
The page registers have to be zeroed out so that any bus-management command hits the page0 instead of using a value that was set by codec driver for vendor-specific configurations.
Why would these bus management commands set bit 15 to indicate a paged access? If they don't set bit 15 the page registers are not used and bits 15..31 of the register address must be 0. Table 78 in the Soundwire 1.2 spec. Table 71 in the 1.0 spec. Table 43 in the 0.6 draft spec.
I forgot about this magic BIT(15) and indeed the Addr_page1/2 values are ignored when issuing non-paged register access. There's really no need to zero-out the page registers, it's completely unnecessary. Nice catch!
For the series:
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
The implementation is far from optimal though, and indeed if we have long transactions that are not interrupted by anything else we could avoid resetting the page registers.
I tried to implement a 'lazy approach' some time back, but at the time I didn't see any benefits due to the limited number of configurations.
I can't remember where the code is, but the initial enhancement was listed here: https://github.com/thesofproject/linux/issues/2881
Richard Fitzgerald (2): soundwire: bus: Don't zero page registers after every transaction soundwire: bus: Remove unused reset_page_addr() callback
drivers/soundwire/bus.c | 23 ----------------------- drivers/soundwire/cadence_master.c | 14 -------------- drivers/soundwire/cadence_master.h | 3 --- drivers/soundwire/intel_auxdevice.c | 1 - include/linux/soundwire/sdw.h | 3 --- 5 files changed, 44 deletions(-)
participants (2)
-
Pierre-Louis Bossart -
Richard Fitzgerald