Message-ID: <2765821.lGaqSPkdTl@n9w6sw14>
Date: Mon, 27 Oct 2025 10:38:20 +0100
From: Christian Eggers <ceggers@...i.de>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
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

Hi Andy,

On Monday, 27 October 2025, 09:45:48 CET, Andy Shevchenko wrote:
> 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.

Thanks for clarifying this. Actually this is how I started. I'll try to do
it this way (see next paragraph).

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

I guess that my problem arises due to the way I built my virtual address map
(I took it from the vendor's SDK):

> #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 */

As you can see, the PAGE part is in the middle of the address, which
causes the PHY part becoming part of the value calculated for the PAGESEL registers
(which does overflow in turn). I haven't tried yet, but I guess there are two options:

1. Manually splitting the PHY and PAGE parts from the PAGESEL value (when writing
to the PAGESEL reg). Then I would need to record the current PHY value outside of the
regmap and add it again in my read/write callbacks (sounds weird).

2. Rearranging the virtual address space, so the the PAGE bits are in the
uppermost bits. So the PAGESEL value should be calculated correctly, but I'll
need to split the PHY and REG parts within my read/write callbacks. This looks
more straight to me, but differs from how it's implemented in the vendor SDK.


> > 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.

Thanks for providing a pointer.

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

regards,
Christian

Powered by blists - more mailing lists