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

[Hugo Villeneuve <hugo@hugovil.com>]

On Mon, 4 Dec 2023 19:16:57 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Mon, Dec 04, 2023 at 01:59:22PM -0500, Hugo Villeneuve wrote:
> 
> > 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,
> > 	},
> > };
> 
> That looks plausible - I'd tend to make the range just completely
> non-physical (eg, start at some unrepresentable value) so there's no
> possible ambiguity with a physical address.  I'm also not clear why
> you've made the window be 128 registers wide if it's just this range

I simply took what was in tas2770.c as a starting point.

> from 2-7 that gets switched around, I'd do something more like
> 
> #define SC16IS7XX_RANGE_BASE 0x1000
> #define SC16IS7XX_WINDOW_LEN 6
> #define SC16IS7XX_PAGED_REG(page, reg) \
> 	(SC16IS7XX_RANGE_BASE + (SC16IS7XX_WINDOW_LEN * page) + reg)
> 
> 	.range_min = SC16IS7XX_RANGE_BASE,
> 	.range_max = SC16IS7XX_RANGE_BASE + (2 * SC16IS7XX_WINDOW_LEN),
> 	.window_start = 2,
> 	.window_len = SC16IS7XX_WINDOW_LEN,
> 
> You could apply a - 2 offset to reg as well to make the defines for the
> registers clearer.  The above means that any untranslated register you
> see in I/O traces should be immediately obvious (and more likely to trip
> up error handling and tell you about it if it goes wrong) and hopefully
> also makes it easier to reason about what's going on.

Ok.

> > 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?
> 
> That's one reason why having the range cover all the registers gets
> confusing.  That said the code does have special casing for a selector
> register in the middle of the range since there were some devices that
> just put the paging register in the middle of the range it controls for
> some innovative region, the core will notice that this is a selector
> register and not do any paging for that register.

You are talking about the selector being inside the range, and I
understand that because I have looked at _regmap_select_page()
comments and logic.

But that is not was my question was about. Here a pseudo code
example to select "page" 1:

1. save original value of LCR register.
2. write 0xBF to LCR register
3. access desired register in page 1
4. restore original LCR value saved in step 1

How do you do that with regmap range?

Hugo.