[PATCH 0/2] bus: brcmstb_gisb: misc fixes

[PATCH 0/2] bus: brcmstb_gisb: misc fixes

[Florian Fainelli]

Hi Arnd,

This patch set contains two small fixes for the Broadcom GISB bus
arbiter code. First patch makes sure we do register a fault code for
ARM platforms, second patch is a fix when the code is used on a PM
capable system.

Thank you!

Florian Fainelli (2):
  bus: brcmstb_gisb: register the fault code hook
  bus: brcmstb_gisb: save and restore GISB timeout

 drivers/bus/brcmstb_gisb.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH 1/2] bus: brcmstb_gisb: register the fault code hook

[Florian Fainelli]

Commit 44127b771d9c31 ("bus: add Broadcom GISB bus arbiter timeout/error
handler") added everything that is required to register an ARM fault
handler for imprecise external aborts, except that there is nothing
calling this currently.

We do not need to export that specific function and have to update
arch/arm/mach-bcm/brcmstb.c to call it, simply, register the fault
handler with an arch_initcall.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/bus/brcmstb_gisb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
index f2cd6a2d40b4..daae4f77f34a 100644
--- a/drivers/bus/brcmstb_gisb.c
+++ b/drivers/bus/brcmstb_gisb.c
@@ -48,6 +48,7 @@ struct brcmstb_gisb_arb_device {
 	struct list_head next;
 	u32 valid_mask;
 	const char *master_names[sizeof(u32) * BITS_PER_BYTE];
+	u32 saved_timeout;
 };
 
 static LIST_HEAD(brcmstb_gisb_arb_device_list);
@@ -160,11 +161,13 @@ static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 	return ret;
 }
 
-void __init brcmstb_hook_fault_code(void)
+static int __init brcmstb_hook_fault_code(void)
 {
 	hook_fault_code(22, brcmstb_bus_error_handler, SIGBUS, 0,
 			"imprecise external abort");
+	return 0;
 }
+arch_initcall(brcmstb_hook_fault_code)
 
 static irqreturn_t brcmstb_gisb_timeout_handler(int irq, void *dev_id)
 {
-- 
1.9.1

[PATCH 2/2] bus: brcmstb_gisb: save and restore GISB timeout

[Florian Fainelli]

When the system enters S3, we will lose the GISB timeout value we have
configured, make sure that we do save this timeout value, and restore
this timeout value prior to re-enabling interrupts such that the GISB
timeout interrupt will fire with the expected timeout.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/bus/brcmstb_gisb.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
index daae4f77f34a..c202c97493de 100644
--- a/drivers/bus/brcmstb_gisb.c
+++ b/drivers/bus/brcmstb_gisb.c
@@ -23,6 +23,7 @@
 #include <linux/list.h>
 #include <linux/of.h>
 #include <linux/bitops.h>
+#include <linux/pm.h>
 
 #include <asm/bug.h>
 #include <asm/signal.h>
@@ -270,6 +271,39 @@ static int brcmstb_gisb_arb_probe(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int brcmstb_gisb_arb_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct brcmstb_gisb_arb_device *gdev = platform_get_drvdata(pdev);
+
+	gdev->saved_timeout = ioread32(gdev->base + ARB_TIMER);
+
+	return 0;
+}
+
+/* Make sure we provide the same timeout value that was configured before, and
+ * do this before the GISB timeout interrupt handler has any chance to run.
+ */
+static int brcmstb_gisb_arb_resume_noirq(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct brcmstb_gisb_arb_device *gdev = platform_get_drvdata(pdev);
+
+	iowrite32(gdev->saved_timeout, gdev->base + ARB_TIMER);
+
+	return 0;
+}
+#else
+#define brcmstb_gisb_arb_suspend       NULL
+#define brcmstb_gisb_arb_resume_noirq  NULL
+#endif
+
+static const struct dev_pm_ops brcmstb_gisb_arb_pm_ops = {
+	.suspend	= brcmstb_gisb_arb_suspend,
+	.resume_noirq	= brcmstb_gisb_arb_resume_noirq,
+};
+
 static const struct of_device_id brcmstb_gisb_arb_of_match[] = {
 	{ .compatible = "brcm,gisb-arb" },
 	{ },
@@ -281,6 +315,7 @@ static struct platform_driver brcmstb_gisb_arb_driver = {
 		.name	= "brcm-gisb-arb",
 		.owner	= THIS_MODULE,
 		.of_match_table = brcmstb_gisb_arb_of_match,
+		.pm	= &brcmstb_gisb_arb_pm_ops,
 	},
 };
 
-- 
1.9.1

Re: [PATCH 1/2] bus: brcmstb_gisb: register the fault code hook

[Florian Fainelli]

On 09/19/2014 12:44 PM, Florian Fainelli wrote:
> Commit 44127b771d9c31 ("bus: add Broadcom GISB bus arbiter timeout/error
> handler") added everything that is required to register an ARM fault
> handler for imprecise external aborts, except that there is nothing
> calling this currently.
> 
> We do not need to export that specific function and have to update
> arch/arm/mach-bcm/brcmstb.c to call it, simply, register the fault
> handler with an arch_initcall.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/bus/brcmstb_gisb.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/brcmstb_gisb.c b/drivers/bus/brcmstb_gisb.c
> index f2cd6a2d40b4..daae4f77f34a 100644
> --- a/drivers/bus/brcmstb_gisb.c
> +++ b/drivers/bus/brcmstb_gisb.c
> @@ -48,6 +48,7 @@ struct brcmstb_gisb_arb_device {
>  	struct list_head next;
>  	u32 valid_mask;
>  	const char *master_names[sizeof(u32) * BITS_PER_BYTE];
> +	u32 saved_timeout;

Meh, this change is unrelated, I will resubmit a v2 shortly, sorry for
the noise.

>  };
>  
>  static LIST_HEAD(brcmstb_gisb_arb_device_list);
> @@ -160,11 +161,13 @@ static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
>  	return ret;
>  }
>  
> -void __init brcmstb_hook_fault_code(void)
> +static int __init brcmstb_hook_fault_code(void)
>  {
>  	hook_fault_code(22, brcmstb_bus_error_handler, SIGBUS, 0,
>  			"imprecise external abort");
> +	return 0;
>  }
> +arch_initcall(brcmstb_hook_fault_code)
>  
>  static irqreturn_t brcmstb_gisb_timeout_handler(int irq, void *dev_id)
>  {
> 

Re: [PATCH 1/2] bus: brcmstb_gisb: register the fault code hook

[Thomas Petazzoni]

Dear Florian Fainelli,

On Fri, 19 Sep 2014 12:44:39 -0700, Florian Fainelli wrote:

> -void __init brcmstb_hook_fault_code(void)
> +static int __init brcmstb_hook_fault_code(void)
>  {
>  	hook_fault_code(22, brcmstb_bus_error_handler, SIGBUS, 0,
>  			"imprecise external abort");
> +	return 0;
>  }
> +arch_initcall(brcmstb_hook_fault_code)

I don't have the entire context, but if your platform is part of
multi-platform, it means that regardless of which platform is actually
booting your system, you will register your fault hook. Is this the
intended behavior?

Generally speaking, in a multiplatform context, many arch_initcall()
should have a conditional to check whether we're really on a platform
where the initcall is needed.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/2] bus: brcmstb_gisb: register the fault code hook

[Brian Norris]

Hi Thomas,

On Sat, Sep 20, 2014 at 03:19:50PM +0200, Thomas Petazzoni wrote:
> On Fri, 19 Sep 2014 12:44:39 -0700, Florian Fainelli wrote:
> 
> > -void __init brcmstb_hook_fault_code(void)
> > +static int __init brcmstb_hook_fault_code(void)
> >  {
> >  	hook_fault_code(22, brcmstb_bus_error_handler, SIGBUS, 0,
> >  			"imprecise external abort");
> > +	return 0;
> >  }
> > +arch_initcall(brcmstb_hook_fault_code)
> 
> I don't have the entire context, but if your platform is part of
> multi-platform, it means that regardless of which platform is actually
> booting your system, you will register your fault hook. Is this the
> intended behavior?
> 
> Generally speaking, in a multiplatform context, many arch_initcall()
> should have a conditional to check whether we're really on a platform
> where the initcall is needed.

Florian already sent v2, about which I already made a similar comment:

  https://lkml.org/lkml/2014/9/19/516

I believe Florian plans to move this into the platform device probe
function instead.

Brian

Re: [PATCH 1/2] bus: brcmstb_gisb: register the fault code hook

[Thomas Petazzoni]

Brian,

On Sun, 21 Sep 2014 16:10:10 -0700, Brian Norris wrote:

> > I don't have the entire context, but if your platform is part of
> > multi-platform, it means that regardless of which platform is actually
> > booting your system, you will register your fault hook. Is this the
> > intended behavior?
> > 
> > Generally speaking, in a multiplatform context, many arch_initcall()
> > should have a conditional to check whether we're really on a platform
> > where the initcall is needed.
> 
> Florian already sent v2, about which I already made a similar comment:
> 
>   https://lkml.org/lkml/2014/9/19/516

Yes, sorry. I saw your review after I sent my e-mail :)

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com