[PATCH v2 1/1] regmap: Synchronize cache for the page selector

[PATCH v2 1/1] regmap: Synchronize cache for the page selector

[Andy Shevchenko]

If the selector register is represented in each page, its value
in accordance to the debugfs is stale because it gets synchronized
only after the real page switch happens. Synchronize cache for
the page selector.

Before (offset followed by hexdump, the first byte is selector):

    // Real registers
    18: 05 ff 00 00 ff 0f 00 00 f0 00 00 00
    ...
    // Virtual (per port)
    40: 05 ff 00 00 e0 e0 00 00 00 00 00 1f
    50: 00 ff 00 00 e0 e0 00 00 00 00 00 1f
    60: 01 ff 00 00 ff ff 00 00 00 00 00 00
    70: 02 ff 00 00 cf f3 00 00 00 00 00 0c
    80: 03 ff 00 00 00 00 00 00 00 00 00 ff
    90: 04 ff 00 00 ff 0f 00 00 f0 00 00 00

After:

    // Real registers
    18: 05 ff 00 00 ff 0f 00 00 f0 00 00 00
    ...
    // Virtual (per port)
    40: 00 ff 00 00 e0 e0 00 00 00 00 00 1f
    50: 01 ff 00 00 e0 e0 00 00 00 00 00 1f
    60: 02 ff 00 00 ff ff 00 00 00 00 00 00
    70: 03 ff 00 00 cf f3 00 00 00 00 00 0c
    80: 04 ff 00 00 00 00 00 00 00 00 00 ff
    90: 05 ff 00 00 ff 0f 00 00 f0 00 00 00

Fixes: 6863ca622759 ("regmap: Add support for register indirect addressing.")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: improved test for selector to be in the range window
 drivers/base/regmap/regmap.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 4ded93687c1f..6d3c69c68cad 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1545,24 +1545,40 @@ static int _regmap_select_page(struct regmap *map, unsigned int *reg,
 			return -EINVAL;
 	}
 
-	/* It is possible to have selector register inside data window.
-	   In that case, selector register is located on every page and
-	   it needs no page switching, when accessed alone. */
+	/*
+	 * It is possible to have selector register inside data window.
+	 * In that case, selector register is located on every page and it
+	 * needs no page switching, when accessed alone.
+	 *
+	 * Nevertheless we should synchronize the cache values for it.
+	 */
 	if (val_num > 1 ||
 	    range->window_start + win_offset != range->selector_reg) {
+		unsigned int page_off = win_page * range->window_len;
+		unsigned int sel_offset = range->selector_reg - range->window_start;
+		unsigned int sel_register = range->range_min + page_off + sel_offset;
+		unsigned int val = win_page << range->selector_shift;
+		unsigned int mask = range->selector_mask;
+
 		/* Use separate work_buf during page switching */
 		orig_work_buf = map->work_buf;
 		map->work_buf = map->selector_work_buf;
 
-		ret = _regmap_update_bits(map, range->selector_reg,
-					  range->selector_mask,
-					  win_page << range->selector_shift,
+		ret = _regmap_update_bits(map, range->selector_reg, mask, val,
 					  &page_chg, false);
 
 		map->work_buf = orig_work_buf;
 
 		if (ret != 0)
 			return ret;
+
+		/*
+		 * If selector register has been just updated, update the respective
+		 * virtual copy as well.
+		 */
+		if (page_chg &&
+		    in_range(range->selector_reg, range->window_start, range->window_len))
+			_regmap_update_bits(map, sel_register, mask, val, NULL, false);
 	}
 
 	*reg = range->window_start + win_offset;
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 1/1] regmap: Synchronize cache for the page selector

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 04:03:20PM +0200, Andy Shevchenko wrote:
> If the selector register is represented in each page, its value
> in accordance to the debugfs is stale because it gets synchronized
> only after the real page switch happens. Synchronize cache for
> the page selector.
> 
> Before (offset followed by hexdump, the first byte is selector):
> 
>     // Real registers
>     18: 05 ff 00 00 ff 0f 00 00 f0 00 00 00
>     ...
>     // Virtual (per port)
>     40: 05 ff 00 00 e0 e0 00 00 00 00 00 1f
>     50: 00 ff 00 00 e0 e0 00 00 00 00 00 1f
>     60: 01 ff 00 00 ff ff 00 00 00 00 00 00
>     70: 02 ff 00 00 cf f3 00 00 00 00 00 0c
>     80: 03 ff 00 00 00 00 00 00 00 00 00 ff
>     90: 04 ff 00 00 ff 0f 00 00 f0 00 00 00
> 
> After:
> 
>     // Real registers
>     18: 05 ff 00 00 ff 0f 00 00 f0 00 00 00
>     ...
>     // Virtual (per port)
>     40: 00 ff 00 00 e0 e0 00 00 00 00 00 1f
>     50: 01 ff 00 00 e0 e0 00 00 00 00 00 1f
>     60: 02 ff 00 00 ff ff 00 00 00 00 00 00
>     70: 03 ff 00 00 cf f3 00 00 00 00 00 0c
>     80: 04 ff 00 00 00 00 00 00 00 00 00 ff
>     90: 05 ff 00 00 ff 0f 00 00 f0 00 00 00

Anything should I do to move this fix forward?

> Fixes: 6863ca622759 ("regmap: Add support for register indirect addressing.")

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/1] regmap: Synchronize cache for the page selector

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

On Thu, Dec 12, 2024 at 04:28:46PM +0200, Andy Shevchenko wrote:

> Anything should I do to move this fix forward?

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]