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.

Cc: Lee Jones <lee@kernel.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Fixes: 72a95859728a ("mfd: syscon: Remove repetition of the regmap_get_val_endian()")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/mfd/syscon.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 9489e80e905a..bdb2ce7ff03b 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -66,6 +66,14 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 		goto err_map;
 	}
 
+	/* Parse the device's DT node for an endianness specification */
+	if (of_property_read_bool(np, "big-endian"))
+		syscon_config.val_format_endian = REGMAP_ENDIAN_BIG;
+	else if (of_property_read_bool(np, "little-endian"))
+		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
+	else if (of_property_read_bool(np, "native-endian"))
+		syscon_config.val_format_endian = REGMAP_ENDIAN_NATIVE;
+
 	/*
 	 * search for reg-io-width property in DT. If it is not provided,
 	 * default to 4 bytes. regmap_init_mmio will return an error if values
-- 
2.37.3

On Sat, 08 Oct 2022, Jason A. Donenfeld 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.
> 
> Cc: Lee Jones <lee@kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Fixes: 72a95859728a ("mfd: syscon: Remove repetition of the regmap_get_val_endian()")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/mfd/syscon.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Applied, thanks.

-- 
Lee Jones [李琼斯]

On Sat, Oct 08, 2022 at 09:47:00AM -0600, Jason A. Donenfeld 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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

as per discussion and analysis that shows that syscon needs that code because
it calls regmap_init_mmio() with dev == NULL.

> Cc: Lee Jones <lee@kernel.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Fixes: 72a95859728a ("mfd: syscon: Remove repetition of the regmap_get_val_endian()")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/mfd/syscon.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 9489e80e905a..bdb2ce7ff03b 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -66,6 +66,14 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
>  		goto err_map;
>  	}
>  
> +	/* Parse the device's DT node for an endianness specification */
> +	if (of_property_read_bool(np, "big-endian"))
> +		syscon_config.val_format_endian = REGMAP_ENDIAN_BIG;
> +	else if (of_property_read_bool(np, "little-endian"))
> +		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
> +	else if (of_property_read_bool(np, "native-endian"))
> +		syscon_config.val_format_endian = REGMAP_ENDIAN_NATIVE;
> +
>  	/*
>  	 * search for reg-io-width property in DT. If it is not provided,
>  	 * default to 4 bytes. regmap_init_mmio will return an error if values
> -- 
> 2.37.3
> 
> 

-- 
With Best Regards,
Andy Shevchenko

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.

 (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

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.

And if not, please just send me the revert through the normal channels. Ok?

                    Linus

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

On Sat, 08 Oct 2022, Andy Shevchenko wrote:

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

Could you review and provide a tag for the revert patch please?

-- 
Lee Jones [李琼斯]

On Tue, Oct 11, 2022 at 08:39:23AM +0100, Lee Jones wrote:
> On Sat, 08 Oct 2022, Andy Shevchenko wrote:
> > On Sat, Oct 08, 2022 at 09:45:16AM -0700, Linus Torvalds wrote:

...

> > > And if not, please just send me the revert through the normal channels. Ok?
> > 
> > Yeah, revert is a good move here.
> 
> Could you review and provide a tag for the revert patch please?

Done.

-- 
With Best Regards,
Andy Shevchenko

On Sat, 08 Oct 2022, Andy Shevchenko wrote:
> > And if not, please just send me the revert through the normal channels. Ok?
> 
> Yeah, revert is a good move here.

Thanks for the clarification Andy.

Linus, by normal channels, do you mean you'd like a follow-up PR?

-- 
Lee Jones [李琼斯]

On Mon, Oct 10, 2022 at 08:48:02AM +0100, Lee Jones wrote:
> On Sat, 08 Oct 2022, Andy Shevchenko wrote:
> > > And if not, please just send me the revert through the normal channels. Ok?
> > 
> > Yeah, revert is a good move here.
> 
> Thanks for the clarification Andy.
> 
> Linus, by normal channels, do you mean you'd like a follow-up PR?

I would assume so.

What happened here is that I sent you (Lee) and Andy this patch, but
also had Linus in the recipient list because I sent this as a reply to
my initial message on the pull request thread, creating some ambiguity.
Sometimes when Linus is on the CC like that, it's an indication that
he's to take it directly. That's not the plan here. Rather, I think the
idea is that you can queue it up in your tree, and send it out with the
next PR of fixes for 6.1.

Jason