GPIO regression in Linux next caused by syscon change

GPIO regression in Linux next caused by syscon change

[Tony Lindgren]

Hi,

Looks like commit effe02710335 ("mfd: syscon: Set regmap max_register
in of_syscon_register") somehow breaks smsc911x GPIO interrupt for
me at least on omap3:

irq 241: nobody cared (try booting with the "irqpoll" option)
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.5.0-rc3-next-20160212 #864
Hardware name: Generic OMAP36xx (Flattened Device Tree)
[<c010fd44>] (unwind_backtrace) from [<c010c094>] (show_stack+0x10/0x14)
[<c010c094>] (show_stack) from [<c045f24c>] (dump_stack+0xb0/0xe4)
[<c045f24c>] (dump_stack) from [<c01998e8>] (__report_bad_irq+0x24/0xc0)
[<c01998e8>] (__report_bad_irq) from [<c0199ce4>] (note_interrupt+0x27c/0x2dc)
[<c0199ce4>] (note_interrupt) from [<c0197220>] (handle_irq_event_percpu+0x184/0x200)
[<c0197220>] (handle_irq_event_percpu) from [<c01972d4>] (handle_irq_event+0x38/0x5c)
[<c01972d4>] (handle_irq_event) from [<c019a3d8>] (handle_level_irq+0xc0/0x154)
[<c019a3d8>] (handle_level_irq) from [<c0196784>] (generic_handle_irq+0x20/0x34)
[<c0196784>] (generic_handle_irq) from [<c049b690>] (omap_gpio_irq_handler+0x120/0x160)
[<c049b690>] (omap_gpio_irq_handler) from [<c01970f0>] (handle_irq_event_percpu+0x54/0x200)
[<c01970f0>] (handle_irq_event_percpu) from [<c01972d4>] (handle_irq_event+0x38/0x5c)
[<c01972d4>] (handle_irq_event) from [<c019a3d8>] (handle_level_irq+0xc0/0x154)
[<c019a3d8>] (handle_level_irq) from [<c0196784>] (generic_handle_irq+0x20/0x34)
[<c0196784>] (generic_handle_irq) from [<c0196a78>] (__handle_domain_irq+0x64/0xe0)
[<c0196a78>] (__handle_domain_irq) from [<c076f038>] (__irq_svc+0x58/0x78)
[<c076f038>] (__irq_svc) from [<c076e93c>] (_raw_spin_unlock_irqrestore+0x34/0x44)
[<c076e93c>] (_raw_spin_unlock_irqrestore) from [<c019900c>] (__setup_irq+0x40c/0x5e0)
[<c019900c>] (__setup_irq) from [<c0199330>] (request_threaded_irq+0xc0/0x158)
[<c0199330>] (request_threaded_irq) from [<c05a4038>] (smsc911x_drv_probe+0x588/0xfec)
[<c05a4038>] (smsc911x_drv_probe) from [<c04fc1b4>] (platform_drv_probe+0x4c/0xb0)
[<c04fc1b4>] (platform_drv_probe) from [<c04fa89c>] (driver_probe_device+0x208/0x2c0)
[<c04fa89c>] (driver_probe_device) from [<c04fa9e8>] (__driver_attach+0x94/0x98)
[<c04fa9e8>] (__driver_attach) from [<c04f8bc4>] (bus_for_each_dev+0x6c/0xa0)
[<c04f8bc4>] (bus_for_each_dev) from [<c04f9d28>] (bus_add_driver+0x18c/0x214)
[<c04f9d28>] (bus_add_driver) from [<c04fb22c>] (driver_register+0x78/0xf8)
[<c04fb22c>] (driver_register) from [<c010180c>] (do_one_initcall+0x80/0x1dc)
[<c010180c>] (do_one_initcall) from [<c0b00ef0>] (kernel_init_freeable+0x218/0x2ec)
[<c0b00ef0>] (kernel_init_freeable) from [<c0768360>] (kernel_init+0x8/0xf4)
[<c0768360>] (kernel_init) from [<c01078d0>] (ret_from_fork+0x14/0x24)
handlers:
[<c05a26d8>] smsc911x_irqhandler
Disabling IRQ #241
libphy: smsc911x-mdio: probed

I'm not seeing the connection between regmap and  GPIO, so no
idea what goes wrong here.

Regards,

Tony

Re: GPIO regression in Linux next caused by syscon change

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [160212 16:31]:
> Hi,
> 
> Looks like commit effe02710335 ("mfd: syscon: Set regmap max_register
> in of_syscon_register") somehow breaks smsc911x GPIO interrupt for
> me at least on omap3:
> 
> irq 241: nobody cared (try booting with the "irqpoll" option)
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.5.0-rc3-next-20160212 #864
> Hardware name: Generic OMAP36xx (Flattened Device Tree)
> [<c010fd44>] (unwind_backtrace) from [<c010c094>] (show_stack+0x10/0x14)
> [<c010c094>] (show_stack) from [<c045f24c>] (dump_stack+0xb0/0xe4)
> [<c045f24c>] (dump_stack) from [<c01998e8>] (__report_bad_irq+0x24/0xc0)
> [<c01998e8>] (__report_bad_irq) from [<c0199ce4>] (note_interrupt+0x27c/0x2dc)
> [<c0199ce4>] (note_interrupt) from [<c0197220>] (handle_irq_event_percpu+0x184/0x200)
> [<c0197220>] (handle_irq_event_percpu) from [<c01972d4>] (handle_irq_event+0x38/0x5c)
> [<c01972d4>] (handle_irq_event) from [<c019a3d8>] (handle_level_irq+0xc0/0x154)
> [<c019a3d8>] (handle_level_irq) from [<c0196784>] (generic_handle_irq+0x20/0x34)
> [<c0196784>] (generic_handle_irq) from [<c049b690>] (omap_gpio_irq_handler+0x120/0x160)
> [<c049b690>] (omap_gpio_irq_handler) from [<c01970f0>] (handle_irq_event_percpu+0x54/0x200)
> [<c01970f0>] (handle_irq_event_percpu) from [<c01972d4>] (handle_irq_event+0x38/0x5c)
> [<c01972d4>] (handle_irq_event) from [<c019a3d8>] (handle_level_irq+0xc0/0x154)
> [<c019a3d8>] (handle_level_irq) from [<c0196784>] (generic_handle_irq+0x20/0x34)
> [<c0196784>] (generic_handle_irq) from [<c0196a78>] (__handle_domain_irq+0x64/0xe0)
> [<c0196a78>] (__handle_domain_irq) from [<c076f038>] (__irq_svc+0x58/0x78)
> [<c076f038>] (__irq_svc) from [<c076e93c>] (_raw_spin_unlock_irqrestore+0x34/0x44)
> [<c076e93c>] (_raw_spin_unlock_irqrestore) from [<c019900c>] (__setup_irq+0x40c/0x5e0)
> [<c019900c>] (__setup_irq) from [<c0199330>] (request_threaded_irq+0xc0/0x158)
> [<c0199330>] (request_threaded_irq) from [<c05a4038>] (smsc911x_drv_probe+0x588/0xfec)
> [<c05a4038>] (smsc911x_drv_probe) from [<c04fc1b4>] (platform_drv_probe+0x4c/0xb0)
> [<c04fc1b4>] (platform_drv_probe) from [<c04fa89c>] (driver_probe_device+0x208/0x2c0)
> [<c04fa89c>] (driver_probe_device) from [<c04fa9e8>] (__driver_attach+0x94/0x98)
> [<c04fa9e8>] (__driver_attach) from [<c04f8bc4>] (bus_for_each_dev+0x6c/0xa0)
> [<c04f8bc4>] (bus_for_each_dev) from [<c04f9d28>] (bus_add_driver+0x18c/0x214)
> [<c04f9d28>] (bus_add_driver) from [<c04fb22c>] (driver_register+0x78/0xf8)
> [<c04fb22c>] (driver_register) from [<c010180c>] (do_one_initcall+0x80/0x1dc)
> [<c010180c>] (do_one_initcall) from [<c0b00ef0>] (kernel_init_freeable+0x218/0x2ec)
> [<c0b00ef0>] (kernel_init_freeable) from [<c0768360>] (kernel_init+0x8/0xf4)
> [<c0768360>] (kernel_init) from [<c01078d0>] (ret_from_fork+0x14/0x24)
> handlers:
> [<c05a26d8>] smsc911x_irqhandler
> Disabling IRQ #241
> libphy: smsc911x-mdio: probed
> 
> I'm not seeing the connection between regmap and  GPIO, so no
> idea what goes wrong here.

The only regmap here is the scm_conf in omap3.dtsi. Probably some
resource size calculation issue causing an overlap?

Tony

Re: GPIO regression in Linux next caused by syscon change

[Philipp Zabel]

I've just replaced the of_iomap() call with an open coded version,
calling of_address_to_resource() and ioremap() directly. That was
needed so I can use the struct resource returned by
of_address_to_resource() to set the syscon_config.max_register. I
don't see where this could cause resource overlap. Does just setting
syscon_config.max_register to zero again make the problem disappear?

regards
Philipp

On Sat, Feb 13, 2016 at 1:49 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Tony Lindgren <tony@atomide.com> [160212 16:31]:
>> Hi,
>>
>> Looks like commit effe02710335 ("mfd: syscon: Set regmap max_register
>> in of_syscon_register") somehow breaks smsc911x GPIO interrupt for
>> me at least on omap3:
>>
>> irq 241: nobody cared (try booting with the "irqpoll" option)
>> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.5.0-rc3-next-20160212 #864
>> Hardware name: Generic OMAP36xx (Flattened Device Tree)
>> [<c010fd44>] (unwind_backtrace) from [<c010c094>] (show_stack+0x10/0x14)
>> [<c010c094>] (show_stack) from [<c045f24c>] (dump_stack+0xb0/0xe4)
>> [<c045f24c>] (dump_stack) from [<c01998e8>] (__report_bad_irq+0x24/0xc0)
>> [<c01998e8>] (__report_bad_irq) from [<c0199ce4>] (note_interrupt+0x27c/0x2dc)
>> [<c0199ce4>] (note_interrupt) from [<c0197220>] (handle_irq_event_percpu+0x184/0x200)
>> [<c0197220>] (handle_irq_event_percpu) from [<c01972d4>] (handle_irq_event+0x38/0x5c)
>> [<c01972d4>] (handle_irq_event) from [<c019a3d8>] (handle_level_irq+0xc0/0x154)
>> [<c019a3d8>] (handle_level_irq) from [<c0196784>] (generic_handle_irq+0x20/0x34)
>> [<c0196784>] (generic_handle_irq) from [<c049b690>] (omap_gpio_irq_handler+0x120/0x160)
>> [<c049b690>] (omap_gpio_irq_handler) from [<c01970f0>] (handle_irq_event_percpu+0x54/0x200)
>> [<c01970f0>] (handle_irq_event_percpu) from [<c01972d4>] (handle_irq_event+0x38/0x5c)
>> [<c01972d4>] (handle_irq_event) from [<c019a3d8>] (handle_level_irq+0xc0/0x154)
>> [<c019a3d8>] (handle_level_irq) from [<c0196784>] (generic_handle_irq+0x20/0x34)
>> [<c0196784>] (generic_handle_irq) from [<c0196a78>] (__handle_domain_irq+0x64/0xe0)
>> [<c0196a78>] (__handle_domain_irq) from [<c076f038>] (__irq_svc+0x58/0x78)
>> [<c076f038>] (__irq_svc) from [<c076e93c>] (_raw_spin_unlock_irqrestore+0x34/0x44)
>> [<c076e93c>] (_raw_spin_unlock_irqrestore) from [<c019900c>] (__setup_irq+0x40c/0x5e0)
>> [<c019900c>] (__setup_irq) from [<c0199330>] (request_threaded_irq+0xc0/0x158)
>> [<c0199330>] (request_threaded_irq) from [<c05a4038>] (smsc911x_drv_probe+0x588/0xfec)
>> [<c05a4038>] (smsc911x_drv_probe) from [<c04fc1b4>] (platform_drv_probe+0x4c/0xb0)
>> [<c04fc1b4>] (platform_drv_probe) from [<c04fa89c>] (driver_probe_device+0x208/0x2c0)
>> [<c04fa89c>] (driver_probe_device) from [<c04fa9e8>] (__driver_attach+0x94/0x98)
>> [<c04fa9e8>] (__driver_attach) from [<c04f8bc4>] (bus_for_each_dev+0x6c/0xa0)
>> [<c04f8bc4>] (bus_for_each_dev) from [<c04f9d28>] (bus_add_driver+0x18c/0x214)
>> [<c04f9d28>] (bus_add_driver) from [<c04fb22c>] (driver_register+0x78/0xf8)
>> [<c04fb22c>] (driver_register) from [<c010180c>] (do_one_initcall+0x80/0x1dc)
>> [<c010180c>] (do_one_initcall) from [<c0b00ef0>] (kernel_init_freeable+0x218/0x2ec)
>> [<c0b00ef0>] (kernel_init_freeable) from [<c0768360>] (kernel_init+0x8/0xf4)
>> [<c0768360>] (kernel_init) from [<c01078d0>] (ret_from_fork+0x14/0x24)
>> handlers:
>> [<c05a26d8>] smsc911x_irqhandler
>> Disabling IRQ #241
>> libphy: smsc911x-mdio: probed
>>
>> I'm not seeing the connection between regmap and  GPIO, so no
>> idea what goes wrong here.
>
> The only regmap here is the scm_conf in omap3.dtsi. Probably some
> resource size calculation issue causing an overlap?
>
> Tony
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: GPIO regression in Linux next caused by syscon change

[Tony Lindgren]

* Philipp Zabel <philipp.zabel@gmail.com> [160214 11:24]:
> I've just replaced the of_iomap() call with an open coded version,
> calling of_address_to_resource() and ioremap() directly. That was
> needed so I can use the struct resource returned by
> of_address_to_resource() to set the syscon_config.max_register. I
> don't see where this could cause resource overlap. Does just setting
> syscon_config.max_register to zero again make the problem disappear?

Yes commenting out the syscon_config.max_register line in your patch
makes things work again.

So what does that tell us about the problem?

Regards,

Tony

Re: GPIO regression in Linux next caused by syscon change

[Philipp Zabel]

Am Montag, den 15.02.2016, 08:01 -0800 schrieb Tony Lindgren:
> * Philipp Zabel <philipp.zabel@gmail.com> [160214 11:24]:
> > I've just replaced the of_iomap() call with an open coded version,
> > calling of_address_to_resource() and ioremap() directly. That was
> > needed so I can use the struct resource returned by
> > of_address_to_resource() to set the syscon_config.max_register. I
> > don't see where this could cause resource overlap. Does just setting
> > syscon_config.max_register to zero again make the problem disappear?
> 
> Yes commenting out the syscon_config.max_register line in your patch
> makes things work again.
> 
> So what does that tell us about the problem?

Maybe some out of bounds writes that previously worked are now catched
by the max_register check in regmap_writable and regmap_write returns
-EIO instead of the write being executed.
Is there any omap_ctrl_write?() call with an offset > 0x32c into
scm_conf?

regards
Philipp

Re: GPIO regression in Linux next caused by syscon change

[Tony Lindgren]

* Philipp Zabel <p.zabel@pengutronix.de> [160215 08:17]:
> Am Montag, den 15.02.2016, 08:01 -0800 schrieb Tony Lindgren:
> > * Philipp Zabel <philipp.zabel@gmail.com> [160214 11:24]:
> > > I've just replaced the of_iomap() call with an open coded version,
> > > calling of_address_to_resource() and ioremap() directly. That was
> > > needed so I can use the struct resource returned by
> > > of_address_to_resource() to set the syscon_config.max_register. I
> > > don't see where this could cause resource overlap. Does just setting
> > > syscon_config.max_register to zero again make the problem disappear?
> > 
> > Yes commenting out the syscon_config.max_register line in your patch
> > makes things work again.
> > 
> > So what does that tell us about the problem?
> 
> Maybe some out of bounds writes that previously worked are now catched
> by the max_register check in regmap_writable and regmap_write returns
> -EIO instead of the write being executed.

Hmm weird that something like that would not produce errors?

> Is there any omap_ctrl_write?() call with an offset > 0x32c into
> scm_conf?

Indeed, that's where things go wrong. Adding Tero to Cc, something
is wrong there.

Regards,

Tony

Re: GPIO regression in Linux next caused by syscon change

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [160215 08:49]:
> * Philipp Zabel <p.zabel@pengutronix.de> [160215 08:17]:
> > Am Montag, den 15.02.2016, 08:01 -0800 schrieb Tony Lindgren:
> > > * Philipp Zabel <philipp.zabel@gmail.com> [160214 11:24]:
> > > > I've just replaced the of_iomap() call with an open coded version,
> > > > calling of_address_to_resource() and ioremap() directly. That was
> > > > needed so I can use the struct resource returned by
> > > > of_address_to_resource() to set the syscon_config.max_register. I
> > > > don't see where this could cause resource overlap. Does just setting
> > > > syscon_config.max_register to zero again make the problem disappear?
> > > 
> > > Yes commenting out the syscon_config.max_register line in your patch
> > > makes things work again.
> > > 
> > > So what does that tell us about the problem?
> > 
> > Maybe some out of bounds writes that previously worked are now catched
> > by the max_register check in regmap_writable and regmap_write returns
> > -EIO instead of the write being executed.
> 
> Hmm weird that something like that would not produce errors?
> 
> > Is there any omap_ctrl_write?() call with an offset > 0x32c into
> > scm_conf?
> 
> Indeed, that's where things go wrong. Adding Tero to Cc, something
> is wrong there.

Something like this might fix it? Needs to be tested to see if it
happens on other omaps.

Tero, got any better ideas?

Regards,

Tony

8< -------------------
--- a/arch/arm/mach-omap2/control.c
+++ b/arch/arm/mach-omap2/control.c
@@ -167,15 +167,23 @@ u16 omap_ctrl_readw(u16 offset)
 u32 omap_ctrl_readl(u16 offset)
 {
 	u32 val;
+	int err;
 
 	offset &= 0xfffc;
-	if (!omap2_ctrl_syscon)
-		val = readl_relaxed(omap2_ctrl_base + offset);
-	else
-		regmap_read(omap2_ctrl_syscon, omap2_ctrl_offset + offset,
-			    &val);
+	if (omap2_ctrl_syscon) {
+		err = regmap_read(omap2_ctrl_syscon,
+				  omap2_ctrl_offset + offset, &val);
+		if (!err)
+			return val;
+	}
+
+	if (WARN(!omap2_ctrl_base,
+		 "syscon out of range for offset 0x%x, no omap2_ctrl_base?\n",
+		    offset)) {
+		return 0;
+	}
 
-	return val;
+	return readl_relaxed(omap2_ctrl_base + offset);
 }
 
 void omap_ctrl_writeb(u8 val, u16 offset)
@@ -206,12 +214,23 @@ void omap_ctrl_writew(u16 val, u16 offset)
 
 void omap_ctrl_writel(u32 val, u16 offset)
 {
+	int err;
+
 	offset &= 0xfffc;
-	if (!omap2_ctrl_syscon)
-		writel_relaxed(val, omap2_ctrl_base + offset);
-	else
-		regmap_write(omap2_ctrl_syscon, omap2_ctrl_offset + offset,
-			     val);
+	if (omap2_ctrl_syscon) {
+		err = regmap_write(omap2_ctrl_syscon,
+				   omap2_ctrl_offset + offset, val);
+		if (!err)
+			return;
+	}
+
+	if (WARN(!omap2_ctrl_base,
+		 "syscon out of range for offset 0x%x, no omap2_ctrl_base?\n",
+		    offset)) {
+		return;
+	}
+
+	writel_relaxed(val, omap2_ctrl_base + offset);
 }
 
 #ifdef CONFIG_ARCH_OMAP3
@@ -724,9 +743,6 @@ int __init omap_control_init(void)
 				if (ret)
 					return ret;
 			}
-
-			iounmap(omap2_ctrl_base);
-			omap2_ctrl_base = NULL;
 		} else {
 			/* No scm_conf found, direct access */
 			ret = omap2_clk_provider_init(np, data->index, NULL,

Re: GPIO regression in Linux next caused by syscon change

[Tero Kristo]

On 02/15/2016 08:06 PM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [160215 08:49]:
>> * Philipp Zabel <p.zabel@pengutronix.de> [160215 08:17]:
>>> Am Montag, den 15.02.2016, 08:01 -0800 schrieb Tony Lindgren:
>>>> * Philipp Zabel <philipp.zabel@gmail.com> [160214 11:24]:
>>>>> I've just replaced the of_iomap() call with an open coded version,
>>>>> calling of_address_to_resource() and ioremap() directly. That was
>>>>> needed so I can use the struct resource returned by
>>>>> of_address_to_resource() to set the syscon_config.max_register. I
>>>>> don't see where this could cause resource overlap. Does just setting
>>>>> syscon_config.max_register to zero again make the problem disappear?
>>>>
>>>> Yes commenting out the syscon_config.max_register line in your patch
>>>> makes things work again.
>>>>
>>>> So what does that tell us about the problem?
>>>
>>> Maybe some out of bounds writes that previously worked are now catched
>>> by the max_register check in regmap_writable and regmap_write returns
>>> -EIO instead of the write being executed.
>>
>> Hmm weird that something like that would not produce errors?
>>
>>> Is there any omap_ctrl_write?() call with an offset > 0x32c into
>>> scm_conf?
>>
>> Indeed, that's where things go wrong. Adding Tero to Cc, something
>> is wrong there.
>
> Something like this might fix it? Needs to be tested to see if it
> happens on other omaps.
>
> Tero, got any better ideas?

The underlying problem is the fact that OMAP3 control module register 
map is split into multiple regions, which in certain cases have 
overlapping functionality, and need to be accessed over boundaries. And 
the fact that we decided to split it in such way in the first place, for 
example to have pinmux regions separate.

If you are going to bypass the usage of regmap, you could just drop it 
completely from the omap_ctrl_readl / writel APIs, and use the iomap 
directly. That simplifies the code and is slightly faster also.

Eventually this should probably be fixed in such way that any 
out-of-bounds accesses on the omap_ctrl region should use the actual 
driver APIs for that. The padconf accesses I have no clue how to fix (as 
pinctrl driver doesn't provide direct register read/write API), but for 
others we should introduce new regions under the DTS files and use a 
separate syscon mapping or something similar.

It looks like we have following accesses in kernel that should be converted:

under omap3_ctrl_init:
OMAP2_CONTROL_SYSCONFIG
OMAP3_PADCONF_SAD2D_MSTANDBY
OMAP3_PADCONF_SAD2D_IDLEACK

under omap3_core_save_context:

         omap_ctrl_writel(omap_ctrl_readl(OMAP343X_PADCONF_ETK_D14),
                 OMAP343X_CONTROL_MEM_WKUP + 0x2a0);


under omap3_control_save / restore:

OMAP2_CONTROL_SYSCONFIG
OMAP34XX_CONTROL_WKUP_CTRL
OMAP343X_CONTROL_PADCONF_SYSNIRQ

-Tero

>
> Regards,
>
> Tony
>
> 8< -------------------
> --- a/arch/arm/mach-omap2/control.c
> +++ b/arch/arm/mach-omap2/control.c
> @@ -167,15 +167,23 @@ u16 omap_ctrl_readw(u16 offset)
>   u32 omap_ctrl_readl(u16 offset)
>   {
>   	u32 val;
> +	int err;
>
>   	offset &= 0xfffc;
> -	if (!omap2_ctrl_syscon)
> -		val = readl_relaxed(omap2_ctrl_base + offset);
> -	else
> -		regmap_read(omap2_ctrl_syscon, omap2_ctrl_offset + offset,
> -			    &val);
> +	if (omap2_ctrl_syscon) {
> +		err = regmap_read(omap2_ctrl_syscon,
> +				  omap2_ctrl_offset + offset, &val);
> +		if (!err)
> +			return val;
> +	}
> +
> +	if (WARN(!omap2_ctrl_base,
> +		 "syscon out of range for offset 0x%x, no omap2_ctrl_base?\n",
> +		    offset)) {
> +		return 0;
> +	}
>
> -	return val;
> +	return readl_relaxed(omap2_ctrl_base + offset);
>   }
>
>   void omap_ctrl_writeb(u8 val, u16 offset)
> @@ -206,12 +214,23 @@ void omap_ctrl_writew(u16 val, u16 offset)
>
>   void omap_ctrl_writel(u32 val, u16 offset)
>   {
> +	int err;
> +
>   	offset &= 0xfffc;
> -	if (!omap2_ctrl_syscon)
> -		writel_relaxed(val, omap2_ctrl_base + offset);
> -	else
> -		regmap_write(omap2_ctrl_syscon, omap2_ctrl_offset + offset,
> -			     val);
> +	if (omap2_ctrl_syscon) {
> +		err = regmap_write(omap2_ctrl_syscon,
> +				   omap2_ctrl_offset + offset, val);
> +		if (!err)
> +			return;
> +	}
> +
> +	if (WARN(!omap2_ctrl_base,
> +		 "syscon out of range for offset 0x%x, no omap2_ctrl_base?\n",
> +		    offset)) {
> +		return;
> +	}
> +
> +	writel_relaxed(val, omap2_ctrl_base + offset);
>   }
>
>   #ifdef CONFIG_ARCH_OMAP3
> @@ -724,9 +743,6 @@ int __init omap_control_init(void)
>   				if (ret)
>   					return ret;
>   			}
> -
> -			iounmap(omap2_ctrl_base);
> -			omap2_ctrl_base = NULL;
>   		} else {
>   			/* No scm_conf found, direct access */
>   			ret = omap2_clk_provider_init(np, data->index, NULL,
>

Re: GPIO regression in Linux next caused by syscon change

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [160220 02:59]:
> On 02/15/2016 08:06 PM, Tony Lindgren wrote:
> >Tero, got any better ideas?
> 
> The underlying problem is the fact that OMAP3 control module register map is
> split into multiple regions, which in certain cases have overlapping
> functionality, and need to be accessed over boundaries. And the fact that we
> decided to split it in such way in the first place, for example to have
> pinmux regions separate.

Well we should only use regmap for the scm_conf area as that contains
tens of random registers that may need to be shared between various
device drivers.

For other SCM regions, we should just do standard Linux device drivers,
like we're doing with the pinmux regions.

> If you are going to bypass the usage of regmap, you could just drop it
> completely from the omap_ctrl_readl / writel APIs, and use the iomap
> directly. That simplifies the code and is slightly faster also.

OK that's a good idea. I'll update the patch to drop regmap for
omap_ctrl_read/write.

> Eventually this should probably be fixed in such way that any out-of-bounds
> accesses on the omap_ctrl region should use the actual driver APIs for that.
> The padconf accesses I have no clue how to fix (as pinctrl driver doesn't
> provide direct register read/write API), but for others we should introduce
> new regions under the DTS files and use a separate syscon mapping or
> something similar.

Best to keep regmap out of it for ranges that can be clearly ioremapped
for a dedicated device drivers.

Anyways, updated patch below, please take a look and ack if that
works for you and I'll apply it into omap-for-v4.6/fixes-not-urgent.

Regards,

Tony

8< ----------------------
From: Tony Lindgren <tony@atomide.com>
Date: Wed, 17 Feb 2016 10:36:31 -0800
Subject: [PATCH] ARM: OMAP2+: Fix out of range register access with
 syscon_config.max_register

If syscon_config.max_register is initialized like it should be, we have
omap_ctrl_read/write() fail with out of range register access at least
for omap3.

We have omap3.dtsi setting up a regmap range for scm_conf, but we now
have omap_ctrl_read/write() also attempt to use the regmap. However,
omap_ctrl_read/write() is also used for other register ranges in the
system control module (SCM).

Let's fix the issue by just removing the regmap_read/write() usage for
control module as suggested by Tero Kristo <t-kristo@ti.com>.

Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/arch/arm/mach-omap2/control.c
+++ b/arch/arm/mach-omap2/control.c
@@ -36,7 +36,6 @@
 
 static void __iomem *omap2_ctrl_base;
 static s16 omap2_ctrl_offset;
-static struct regmap *omap2_ctrl_syscon;
 
 #if defined(CONFIG_ARCH_OMAP3) && defined(CONFIG_PM)
 struct omap3_scratchpad {
@@ -166,16 +165,9 @@ u16 omap_ctrl_readw(u16 offset)
 
 u32 omap_ctrl_readl(u16 offset)
 {
-	u32 val;
-
 	offset &= 0xfffc;
-	if (!omap2_ctrl_syscon)
-		val = readl_relaxed(omap2_ctrl_base + offset);
-	else
-		regmap_read(omap2_ctrl_syscon, omap2_ctrl_offset + offset,
-			    &val);
 
-	return val;
+	return readl_relaxed(omap2_ctrl_base + offset);
 }
 
 void omap_ctrl_writeb(u8 val, u16 offset)
@@ -207,11 +199,7 @@ void omap_ctrl_writew(u16 val, u16 offset)
 void omap_ctrl_writel(u32 val, u16 offset)
 {
 	offset &= 0xfffc;
-	if (!omap2_ctrl_syscon)
-		writel_relaxed(val, omap2_ctrl_base + offset);
-	else
-		regmap_write(omap2_ctrl_syscon, omap2_ctrl_offset + offset,
-			     val);
+	writel_relaxed(val, omap2_ctrl_base + offset);
 }
 
 #ifdef CONFIG_ARCH_OMAP3
@@ -715,8 +703,6 @@ int __init omap_control_init(void)
 			if (IS_ERR(syscon))
 				return PTR_ERR(syscon);
 
-			omap2_ctrl_syscon = syscon;
-
 			if (of_get_child_by_name(scm_conf, "clocks")) {
 				ret = omap2_clk_provider_init(scm_conf,
 							      data->index,
@@ -724,9 +710,6 @@ int __init omap_control_init(void)
 				if (ret)
 					return ret;
 			}
-
-			iounmap(omap2_ctrl_base);
-			omap2_ctrl_base = NULL;
 		} else {
 			/* No scm_conf found, direct access */
 			ret = omap2_clk_provider_init(np, data->index, NULL,