Message-ID: <aP8xPOG68qL2IQ-A@smile.fi.intel.com>
Date: Mon, 27 Oct 2025 10:45:48 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Christian Eggers <ceggers@...i.de>
Cc: Javier Carrasco <javier.carrasco.cruz@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Krystian Garbaciak <krystian.garbaciak@...semi.com>,
	linux-kernel@...r.kernel.org
Subject: Re: regmap: Modeling a shared page selection register

On Sat, Oct 25, 2025 at 06:22:26PM +0200, Christian Eggers wrote:
> Hi,
> 
> I try to develop a driver for the JLSemi JL5104 Ethernet switch (no public
> documentation  available, sorry).  The device is attached via the MDIO bus 
> and offers different register accesses (it occupies 8 PHY addresses):
> 
> 1. Direct access (on each PHY address for the first 16 registers)
> 2. Paged access (only on PHY address 0, last 16 registers are paged, 
>    register 31 is used for page selection)
> 
> There are also 'indirectly' accessed registers, but that is not part of
> this question.
> 
> I would like to build regmap for this like the following:
> 
> ---------------------------------------8<---------------------------------------------------
> /* Regmap addresses are built from the phy address, the page number and the register address */
> #define JL51XX_PHY_PHY_MASK		GENMASK(25, 21)  /* PHY address, 5 bit */
> #define JL51XX_PHY_PAGE_MASK		GENMASK(20,  5)  /* Page address within PHY, 16 bit */
> #define JL51XX_PHY_REG_MASK		GENMASK( 4,  0)  /* Register within page, 5 bit */
> 
> 
> #define JL51XX_PHY0_BASE		0x00000000U
> #define JL51XX_PHY_BLOCK_SIZE		0x00200000U
> #define JL51XX_PHY_BASE(PHY)		(JL51XX_PHY0_BASE + (PHY) * JL51XX_PHY_BLOCK_SIZE)
> #define JL51XX_PHY_END(PHY)		(JL51XX_PHY_BASE(PHY) + JL51XX_PHY_BLOCK_SIZE - 1)
> 
> /* Page selection register, shared between all PHYs !!! */
> #define JL51XX_PHY_PAGE_SEL			31

Ranges are sets of virtual (subject to page select) registers. If you have a
common subset of the register with paging available, it should be a _single_
range. I.o.w. _a_ range per set of the registers with a unique selector.

See, for example, drivers/pinctrl/pinctrl-cy8c95x0.c how it does it.
The HW has more selectors (i.e. PWM settings) than what is currently
implemented, but you can get an idea.

> #define JL51XX_PAGED_REGMAP_RANGE(PHY) \
> {									\
> 	.name = "JL51xx PHY" __stringify(PHY) " paged registers", 	\
> 	.range_min	= JL51XX_PHY_BASE(PHY), 			\
> 	.range_max	= JL51XX_PHY_END(PHY), 				\
> 	/*.selector_reg	= JL51XX_PHY_BASE(PHY) + JL51XX_PHY_PAGE_SEL, */				\
> 	.selector_reg	= JL51XX_PHY_PAGE_SEL, 				\
> 	.selector_mask	= GENMASK(15, 0), 				\
> 	.selector_shift	= 0,						\
> 	.window_start	= JL51XX_PHY_BASE(PHY),				\
> 	.window_len	= 32,						\
> }

> static const struct regmap_range_cfg jl51xx_paged_regmap_ranges[] = {
> 	JL51XX_PAGED_REGMAP_RANGE(0),
> 	JL51XX_PAGED_REGMAP_RANGE(1),
> 	JL51XX_PAGED_REGMAP_RANGE(2),
> 	JL51XX_PAGED_REGMAP_RANGE(3),
> 	JL51XX_PAGED_REGMAP_RANGE(4),
> 	JL51XX_PAGED_REGMAP_RANGE(5),
> 	JL51XX_PAGED_REGMAP_RANGE(6),
> 	JL51XX_PAGED_REGMAP_RANGE(7),
> };
> 
> static const struct regmap_config jl51xx_paged_regmap_config = {
> 	.name = "JL51xx direct accessible (paged) registers",
> 
> 	.reg_bits	= 32,  // roundup(phy[5] + page[8] + reg[5], 8)
> 	.reg_stride	= 1,
> 	.val_bits	= 16,
> 	.readable_reg	= jl51xx_paged_regmap_readable_reg,
> 	.volatile_reg	= jl51xx_paged_regmap_volatile_reg,
> 	.lock 		= jl51xx_regmap_lock,
> 	.unlock		= jl51xx_regmap_unlock,
> 	.max_register	= JL51XX_PHY0_BASE
> 			  + JL51XX_NUM_PHYS * JL51XX_PHY_BLOCK_SIZE - 1,
> 	.cache_type	= REGCACHE_MAPLE,
> 	.ranges 	= jl51xx_paged_regmap_ranges,
> 	.num_ranges 	= ARRAY_SIZE(jl51xx_paged_regmap_ranges),
> };
> 
> ...
> 	jl51xx->regmap_paged = devm_regmap_init(dev,
> 						&jl51xx_regmap_mdio_c22_bus,
> 						jl51xx, &jl51xx_paged_regmap_config);
> 
> --------------------------------------->8---------------------------------------------------
> 
> The problem is, that devm_regmap_init() prints the following error:
> 
> > jl51xx 2188000.ethernet-1:00: Range 0: selector for 1 in window
> 
> So, regmap complains because the selector_reg (JL51XX_PHY_PAGE_SEL) of one
> range appears within the range_min..rage_max area of another/all ranges 
> (PHYs in my case). But this is how the hardware actually works.

Of course, the idea of paging is that you have a single address space which
maps on per-page base. Also consider leaving the default page address be
untouched (meaning that most likely you will have direct access to the
registers _and_ via virtual address space to the "default" page).

> Of course, I could access the JL51XX_PHY_PAGE_SEL also via unique aliases
> which lay inside the individual ranges (see the line which is commented out). 
> But this would break caching (as all aliases of the PAGE_SEL register share
> the same resource in hardware). Caching this particular register is the 
> most significant speedup I can imagine for this device, as it has to be 
> accessed for almost every other register access.
> 
> On the other hand, the PAGE_SEL register is not meaningful for the first
> 16 registers of each PHY.  So, coding individual cached page selection logic
> (within the driver, without using regmap ranges at all) could avoid
> writing the PAGE_SEL register when accessing registers which are
> not dependent on the page selection logic.
> 
> I have also tried to model this using only a single regmap_range (for
> all PHYs). But then, the 'PHY address' part of the regmap_adress gets
> lost within regmap's page selection logic (the PHY address becomes
> part of the page address, until it is by the masked out by the 
> 'selector_mask').

Perhaps it's due to the detail I just described in the previous paragraph
(of my answer).

> After commenting out the "selector / range" checking logic in __regmap_init(),
> everything seems to work fine:
> 
> #if 0
> 			if (range_cfg->range_min <= sel_reg &&
> 			    sel_reg <= range_cfg->range_max) {
> 				dev_err(map->dev,
> 					"Range %d: selector for %d in window\n",
> 					i, j);
> 				goto err_range;
> 			}
> #endif
> 
> This check dates back since 2012, so it feels wrong to me to simply remove it:
> 6863ca622759 ("regmap: Add support for register indirect addressing.")

Right, the logic behind paging implementation in regmap is a bit tricky
(and has some known non-critical bugs, FWIW). There is no documentation
and it appears that most of the drivers using it, do it wrongly.

I am a bit lazy to spend time for googling, but you may try to exercise the
lore.kernel.org search with my name and things related to the regmap paging /
selector and find some discussions in the past.

> So should I better open coding the page selection stuff into my driver?

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists