Re: [PATCH] tty: max310x: work around regmap->regcache data corruption

[Hugo Villeneuve]

On Mon, 4 Dec 2023 17:19:25 +0000
Mark Brown <broonie@xxxxxxxxxx> wrote:

> On Mon, Dec 04, 2023 at 12:01:51PM -0500, Hugo Villeneuve wrote:
> > Mark Brown <broonie@xxxxxxxxxx> wrote:
> 
> > > I don't understand what you mean here - you say that the addresses both
> > > have addresses 0x1 and 0x81 but map to address 0x1.  What does the 0x81
> > > refer to?  The comments in the driver seemed to indicate that there was
> > > a single address which mapped to multiple underlying registers...
> 
> > I was referring to an example in da9063-i2c.c where they have
> > these two registers:
> 
> > #define	DA9063_REG_STATUS_A		0x01
> > #define	DA9063_REG_SEQ			0x81
> 
> > To access one or the other, you must select page 0 or 1 in page config
> > selection register at address 0x00. It makes sense to me for this case.
> 
> That appears to be a bit confused in that they've mapped the window
> through which you view the paged registers on top of the physical
> register map - I suppose that will work but more through luck than
> design.  The window is the physical address range through which the
> actual registers can be accessed, the range is the virtual register
> numbers through which users access the regmap.  It'll make things
> clearer if they don't overlap.

Ok, that was probably not a good example, let's forget about it, for
me at least :)

I found a better example maybe with tas2770.c.

> > But for the sc16is7xx, for example you have these two
> > independent registers, sharing the exact same address:
> 
> > #define SC16IS7XX_IIR_REG		(0x02) /* Interrupt Identification */
> > #define SC16IS7XX_FCR_REG		(0x02) /* FIFO control */
> 
> > I am not sure if regmap range can be used with this configuration.
> > Assuming regmap range would be properly setup, when we call
> > regmap_read(regmap, SC16IS7XX_IIR_REG, &val), how does regmap would
> > know that we want to access SC16IS7XX_IIR_REG and not SC16IS7XX_FCR_REG?
> 
> This is the exact situation this feature is supposed to handle, your
> window is address 2 and then you should pick some random non-physical
> numbers to map the two registers to for access by users.  The core will
> then do appropriate physical accesses transparently to manage the
> window.  The whole point here is to assign new, virtual addresses to the
> two registers you're trying to access.

Ok, I see.

Based on tas2770.c, I am assuming I could redefine the code to look
like this:

/* SC16IS7XX register definitions */
#define SC16IS7XX_REG(page, reg)        ((page * 128) + reg)

#define SC16IS7XX_RHR_REG	SC16IS7XX_REG(0, 0x00) /* RX FIFO */
#define SC16IS7XX_THR_REG	SC16IS7XX_REG(0, 0x00) /* TX FIFO */
#define SC16IS7XX_IER_REG	SC16IS7XX_REG(0, 0x01) /* Interrupt enable */
#define SC16IS7XX_IIR_REG	SC16IS7XX_REG(0, 0x02) /* Interrupt Identification (read) */
#define SC16IS7XX_FCR_REG	SC16IS7XX_REG(0, 0x02) /* FIFO control (write) */
#define SC16IS7XX_MCR_REG	SC16IS7XX_REG(0, 0x04) /* Modem Control */
#define SC16IS7XX_LSR_REG	SC16IS7XX_REG(0, 0x05) /* Line Status */
#define SC16IS7XX_MSR_REG	SC16IS7XX_REG(0, 0x06) /* Modem Status */
#define SC16IS7XX_SPR_REG	SC16IS7XX_REG(0, 0x07) /* Scratch Pad */
...
#define SC16IS7XX_EFR_REG	SC16IS7XX_REG(1, 0x02) /* Enhanced Features */
#define SC16IS7XX_XON1_REG	SC16IS7XX_REG(1, 0x04) /* Xon1 word */
#define SC16IS7XX_XON2_REG	SC16IS7XX_REG(1, 0x05) /* Xon2 word */
#define SC16IS7XX_XOFF1_REG	SC16IS7XX_REG(1, 0x06) /* Xoff1 word */
#define SC16IS7XX_XOFF2_REG	SC16IS7XX_REG(1, 0x07) /* Xoff2 word */
...

static const struct regmap_range_cfg sc16is7xx_regmap_ranges[] = {
	{
		.range_min = 0,
		.range_max = 1 * 128,
		.selector_reg = SC16IS7XX_LCR_REG,
		.selector_mask = 0xff,
		.selector_shift = 0,
		.window_start = 0,
		.window_len = 128,
	},
};

But here, selecting the proper "page" is not obvious,
because to select page 1, we need to write a fixed value of 0xBF to
the LCR register.

And when selecting page 0, we must write the previous value that was
in LCR _before_ we made the switch to page 1...

How do we do that?

Hugo.

 
> > > Searching for struct regmap_range_cfg should show a lot of users in
> > > mainline.
> 
> > Yes, I am trying to find a good example but I must download and read the
> > datasheet for each one. If you could point to an IC/driver that uses
> > regmap_range similar to IC sc16is7xx, it would really help.
> 
> Essentially all of them should be fine.