[PATCH 0/2] soundwire: Remove redundant zeroing of page registers

[PATCH 0/2] soundwire: Remove redundant zeroing of page registers

[Richard Fitzgerald]

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(-)

-- 
2.30.2

[PATCH 1/2] soundwire: bus: Don't zero page registers after every transaction

[Richard Fitzgerald]

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;
 }
 
-- 
2.30.2

[PATCH 2/2] soundwire: bus: Remove unused reset_page_addr() callback

[Richard Fitzgerald]

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);
-- 
2.30.2

Re: [PATCH 0/2] soundwire: Remove redundant zeroing of page registers

[Pierre-Louis Bossart]

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(-)
> 

Re: [PATCH 0/2] soundwire: Remove redundant zeroing of page registers

[Richard Fitzgerald]

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(-)
>>

Re: [PATCH 0/2] soundwire: Remove redundant zeroing of page registers

[Pierre-Louis Bossart]

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(-)
>>>