Messages in this thread | | | From | Chen-Yu Tsai <> | Date | Fri, 21 Feb 2020 11:04:14 +0800 | Subject | Re: [linux-sunxi] Re: [PATCH] bus: sunxi-rsb: Return correct data when mixing 16-bit and 8-bit reads |
| |
On Wed, Feb 19, 2020 at 8:14 PM Ondřej Jirman <megous@megous.com> wrote: > > On Wed, Feb 19, 2020 at 10:49:18AM +0800, Chen-Yu Tsai wrote: > > On Wed, Feb 19, 2020 at 9:10 AM Ondrej Jirman <megous@megous.com> wrote: > > > > > > When doing a 16-bit read that returns data in the MSB byte, the > > > RSB_DATA register will keep the MSB byte unchanged when doing > > > the following 8-bit read. sunxi_rsb_read() will then return > > > a result that contains high byte from 16-bit read mixed with > > > the 8-bit result. > > > > > > The consequence is that after this happens the PMIC's regmap will > > > look like this: (0x33 is the high byte from the 16-bit read) > > > > > > % cat /sys/kernel/debug/regmap/sunxi-rsb-3a3/registers > > > 00: 33 > > > 01: 33 > > > 02: 33 > > > 03: 33 > > > 04: 33 > > > 05: 33 > > > 06: 33 > > > 07: 33 > > > 08: 33 > > > 09: 33 > > > 0a: 33 > > > 0b: 33 > > > 0c: 33 > > > 0d: 33 > > > 0e: 33 > > > [snip] > > > > > > Fix this by masking the result of the read with the correct mask > > > based on the size of the read. There are no 16-bit users in the > > > mainline kernel, so this doesn't need to get into the stable tree. > > > > > > Signed-off-by: Ondrej Jirman <megous@megous.com> > > > > Acked-by: Chen-Yu Tsai <wens@csie.org> > > > > for the fix, however it's not entirely clear to me how the MSB 0x33 > > value got into the regmap. Looks like I expected the regmap core to > > handle any overflows, or didn't expect the lingering MSB from 16-bit > > reads, as I didn't have any 16-bit register devices back when I wrote > > this. > > Now I feel like I masked some other bug. Though explanation may be quite simple. > > For example when AXP core does regmap_read on some values for showing charging > current or battery voltage, because regmap_read works with unsigned int, it > will simply get a number that's too big. And that was the major symptom > I observed. I got readings from sysfs that my tablet is consuming 600A or 200A, > etc. And this value was jumping around based on AC100 activity (as the MSB > byte got changed). > > In other places where the driver does regmap_update_bits, I think nothing bad > happened. The write after the read would simply discard the MSB byte. > > And for the debugfs/regmap/*/registers, those are printed such: > > https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-debugfs.c#L256 > > snprintf(buf + buf_pos, count - buf_pos, > "%.*x", map->debugfs_val_len, val); > > This doesn't truncate values, so the larger number gets printed to (for example): > > 33fe > > But regmap debugfs code truncates this by cutting off the formatted string to > this length: > > https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-debugfs.c#L189 > > So in the end, only: > > 00: 33 > > remains, instead of the correct value of: > > 00: fe > > So in the case of debugfs this is just a cosmetic/formatting issue, that didn't > affect anything else. > > I think regmap_bus->reg_read API simply expects the returned value to not exceed > the sensible range.
Sounds good. Thanks for digging through this. My ack still stands.
ChenYu
> regards, > o. > > > > ChenYu > > > > > > > --- > > > drivers/bus/sunxi-rsb.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c > > > index b8043b58568ac..8ab6a3865f569 100644 > > > --- a/drivers/bus/sunxi-rsb.c > > > +++ b/drivers/bus/sunxi-rsb.c > > > @@ -316,6 +316,7 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr, > > > { > > > u32 cmd; > > > int ret; > > > + u32 mask; > > > > > > if (!buf) > > > return -EINVAL; > > > @@ -323,12 +324,15 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr, > > > switch (len) { > > > case 1: > > > cmd = RSB_CMD_RD8; > > > + mask = 0xffu; > > > break; > > > case 2: > > > cmd = RSB_CMD_RD16; > > > + mask = 0xffffu; > > > break; > > > case 4: > > > cmd = RSB_CMD_RD32; > > > + mask = 0xffffffffu; > > > break; > > > default: > > > dev_err(rsb->dev, "Invalid access width: %zd\n", len); > > > @@ -345,7 +349,7 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr, > > > if (ret) > > > goto unlock; > > > > > > - *buf = readl(rsb->regs + RSB_DATA); > > > + *buf = readl(rsb->regs + RSB_DATA) & mask; > > > > > > unlock: > > > mutex_unlock(&rsb->lock); > > > -- > > > 2.25.1 > > > > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20200219121424.dfvrwfcavupmwbvw%40core.my.home.
| |