Messages in this thread | | | Date | Sat, 8 Oct 2022 22:07:37 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH] Revert "mfd: syscon: Remove repetition of the regmap_get_val_endian()" |
| |
On Sat, Oct 08, 2022 at 09:45:16AM -0700, Linus Torvalds wrote: > On Sat, Oct 8, 2022 at 8:47 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > This reverts commit 72a95859728a7866522e6633818bebc1c2519b17. It broke > > reboots on big-endian MIPS and MIPS64 malta QEMU instances, which use > > the syscon driver. Little-endian is not effected, which means likely > > it's important to handle regmap_get_val_endian() in this function after > > all. > > Hmm. The revert may indeed be the right thing to do, but we're still > early in the release process, so let's go through the channels. > > I do note that commit 72a95859728a points to commit 0dbdb76c0ca8 > ("regmap: mmio: Parse endianness definitions from DT") as the reason > why it's not necessary any more, but that commit > > (a) doesn't seem to set config->val_format_endian (which somebody may > care about). It does set the operation pointers etc, but doesn't set > that field.
It should.
of_syscon_register() calls to regmap_init_mmio() with syscon_config data structure as a parameter.
Before 72a95859728a the of_syscon_register() fills the val_format_endian with something it parses from DT. After that commit the default value (0) is REGMAP_ENDIAN_DEFAULT. Now when __regmap_init_mmio_clk() is called it creates a context base on DT since the field is 0.
> (b) it uses regmap_get_val_endian(), which doesn't actually even look > at the OF properties unless config->val_format_endian is > REGMAP_ENDIAN_DEFAULT
Which is 0!
> so the code that commit 72a95859728a removed was actually quite a bit > different from the code in commit 0dbdb76c0ca8. > > Maybe the problem is related to those semantic differences, and is > easy to fix for somebody who knows what the heck that stuff is doing.
But while looking into this, I think I know what is going on, of_syscon_register() calls regmap API with dev == NULL, hence fwnode == NULL, hence nothing to read from DT.
But default (via regmap bus configuration) is LE and LE works fine.
> And if not, please just send me the revert through the normal channels. Ok?
Yeah, revert is a good move here.
For real deduplication we need to either add a regmap_get_val_endian() kind that takes fwnode as a parameter and call it explicitly or propagate fwnode to __regmap_init_mmio_clk() somehow.
-- With Best Regards, Andy Shevchenko
| |