regmap: Modeling a shared page selection register

[Christian Eggers <ceggers@arri.de>]

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


#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, 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').


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

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


regards,
Christian