[1/3] irqchip: gic/realview: support more RealView DCC variants

Message ID 1455803172-22747-1-git-send-email-linus.walleij@linaro.org
State New
Headers show

Commit Message

Linus Walleij Feb. 18, 2016, 1:46 p.m.
In the add-on file for the GIC dealing with the RealView family
we currently only handle the PB11MPCore, let's extend this to
manage the RealView EB ARM11MPCore as well. The Revision B of the
ARM11MPCore core tile is a bit special and needs special handling
as it moves a system control register around at random.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
This can be applied in isolation from the other patches so Marc
one you're happy with it, please take it into the IRQchip tree.

There are two compatible strings getting added to the device tree
bindings so CC to the DT list. No biggie though, just figures out
exactly what ARM custom GIC flavor it is.
---
 .../bindings/interrupt-controller/arm,gic.txt      |  2 ++
 drivers/irqchip/irq-gic-realview.c                 | 35 ++++++++++++++++++----
 2 files changed, 32 insertions(+), 5 deletions(-)

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Arnd Bergmann Feb. 18, 2016, 2:16 p.m. | #1
On Thursday 18 February 2016 14:46:12 Linus Walleij wrote:
> In the add-on file for the GIC dealing with the RealView family

> we currently only handle the PB11MPCore, let's extend this to

> manage the RealView EB ARM11MPCore as well. The Revision B of the

> ARM11MPCore core tile is a bit special and needs special handling

> as it moves a system control register around at random.

> 

> Cc: Arnd Bergmann <arnd@arndb.de>

> Cc: Marc Zyngier <marc.zyngier@arm.com>

> Cc: devicetree@vger.kernel.org

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> This can be applied in isolation from the other patches so Marc

> one you're happy with it, please take it into the IRQchip tree.

> 

> There are two compatible strings getting added to the device tree

> bindings so CC to the DT list. No biggie though, just figures out

> exactly what ARM custom GIC flavor it is.

> 


Looks good to me.

All three patches:

Acked-by: Arnd Bergmann <arnd@arndb.de>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier Feb. 18, 2016, 2:19 p.m. | #2
Hi Linus,

On 18/02/16 13:46, Linus Walleij wrote:
> In the add-on file for the GIC dealing with the RealView family

> we currently only handle the PB11MPCore, let's extend this to

> manage the RealView EB ARM11MPCore as well. The Revision B of the

> ARM11MPCore core tile is a bit special and needs special handling

> as it moves a system control register around at random.


Ah, 11MPCore RevB. That thing. Tried to locate one in the magic
cupboard, and failed. Oh well...

> 

> Cc: Arnd Bergmann <arnd@arndb.de>

> Cc: Marc Zyngier <marc.zyngier@arm.com>

> Cc: devicetree@vger.kernel.org

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> This can be applied in isolation from the other patches so Marc

> one you're happy with it, please take it into the IRQchip tree.

> 

> There are two compatible strings getting added to the device tree

> bindings so CC to the DT list. No biggie though, just figures out

> exactly what ARM custom GIC flavor it is.

> ---

>  .../bindings/interrupt-controller/arm,gic.txt      |  2 ++

>  drivers/irqchip/irq-gic-realview.c                 | 35 ++++++++++++++++++----

>  2 files changed, 32 insertions(+), 5 deletions(-)

> 

> diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt

> index 5a1cb4bc3dfe..0c80e6870645 100644

> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt

> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt

> @@ -16,6 +16,8 @@ Main node required properties:

>  	"arm,cortex-a15-gic"

>  	"arm,cortex-a7-gic"

>  	"arm,cortex-a9-gic"

> +	"arm,eb11mp-gic"

> +	"arm,eb11mp-revb-gic"

>  	"arm,gic-400"

>  	"arm,pl390"

>  	"arm,tc11mp-gic"

> diff --git a/drivers/irqchip/irq-gic-realview.c b/drivers/irqchip/irq-gic-realview.c

> index aa46eb280a7f..224e83d5c056 100644

> --- a/drivers/irqchip/irq-gic-realview.c

> +++ b/drivers/irqchip/irq-gic-realview.c

> @@ -10,7 +10,8 @@

>  #include <linux/irqchip/arm-gic.h>

>  

>  #define REALVIEW_SYS_LOCK_OFFSET	0x20

> -#define REALVIEW_PB11MP_SYS_PLD_CTRL1	0x74

> +#define REALVIEW_SYS_PLD_CTRL1		0x74

> +#define REALVIEW_EB_REVB_SYS_PLD_CTRL1	0xD8

>  #define VERSATILE_LOCK_VAL		0xA05F

>  #define PLD_INTMODE_MASK		BIT(22)|BIT(23)|BIT(24)

>  #define PLD_INTMODE_LEGACY		0x0

> @@ -18,26 +19,50 @@

>  #define PLD_INTMODE_NEW_NO_DCC		BIT(23)

>  #define PLD_INTMODE_FIQ_ENABLE		BIT(24)

>  

> +static const struct of_device_id syscon_pldset_of_match[] = {

> +	{

> +		.compatible = "arm,realview-eb-syscon",

> +	},

> +	{

> +		.compatible = "arm,realview-pb11mp-syscon",

> +	},

> +	{},

> +};

> +

>  static int __init

>  realview_gic_of_init(struct device_node *node, struct device_node *parent)

>  {

>  	static struct regmap *map;

> +	struct device_node *np;

> +	struct pld_setting *pldset;

> +	u32 pld1_ctrl = REALVIEW_SYS_PLD_CTRL1;

> +

> +	np = of_find_matching_node_and_match(NULL, syscon_pldset_of_match,

> +					     (void *)&pldset);

> +	if (!np)

> +		return -ENODEV;

> +

> +	/* For some reason RealView EB Rev B moved this register */

> +	if (of_device_is_compatible(np, "arm,eb11mp-revb-gic"))

> +		pld1_ctrl = REALVIEW_EB_REVB_SYS_PLD_CTRL1;


Associating the PLD control register with the GIC compatible string is a
bit odd, as they are conceptually two different devices. Why not storing
the register address as the data field in the syscon_pldset_of_match
array? One less check, and you get it for free (sort of).

>  

>  	/* The PB11MPCore GIC needs to be configured in the syscon */

> -	map = syscon_regmap_lookup_by_compatible("arm,realview-pb11mp-syscon");

> +	map = syscon_node_to_regmap(np);

>  	if (!IS_ERR(map)) {

>  		/* new irq mode with no DCC */

>  		regmap_write(map, REALVIEW_SYS_LOCK_OFFSET,

>  			     VERSATILE_LOCK_VAL);

> -		regmap_update_bits(map, REALVIEW_PB11MP_SYS_PLD_CTRL1,

> +		regmap_update_bits(map, pld1_ctrl,

>  				   PLD_INTMODE_NEW_NO_DCC,

>  				   PLD_INTMODE_MASK);

>  		regmap_write(map, REALVIEW_SYS_LOCK_OFFSET, 0x0000);

> -		pr_info("TC11MP GIC: set up interrupt controller to NEW mode, no DCC\n");

> +		pr_info("RealView GIC: set up interrupt controller to NEW mode, no DCC\n");

>  	} else {

> -		pr_err("TC11MP GIC setup: could not find syscon\n");

> +		pr_err("RealView GIC setup: could not find syscon\n");

>  		return -ENXIO;


Is that even reachable now that you check for the syscon early in the
function? Also, you are now returning -ENODEV while you were returning
-ENXIO before. Which one is the most correct (I have no idea)?

>  	}

>  	return gic_of_init(node, parent);

>  }

>  IRQCHIP_DECLARE(armtc11mp_gic, "arm,tc11mp-gic", realview_gic_of_init);

> +IRQCHIP_DECLARE(armeb11mp_gic, "arm,eb11mp-gic", realview_gic_of_init);

> +IRQCHIP_DECLARE(armeb11mp_revb_gic, "arm,eb11mp-revb-gic", realview_gic_of_init);

> 


Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Feb. 18, 2016, 2:40 p.m. | #3
On Thursday 18 February 2016 14:19:48 Marc Zyngier wrote:
> On 18/02/16 13:46, Linus Walleij wrote:

> > In the add-on file for the GIC dealing with the RealView family

> > we currently only handle the PB11MPCore, let's extend this to

> > manage the RealView EB ARM11MPCore as well. The Revision B of the

> > ARM11MPCore core tile is a bit special and needs special handling

> > as it moves a system control register around at random.

> 

> Ah, 11MPCore RevB. That thing. Tried to locate one in the magic

> cupboard, and failed. Oh well...


What surprised me though is that it's enabled by default:

$ grep REVB arch/arm/configs/realview*
arch/arm/configs/realview_defconfig:CONFIG_REALVIEW_EB_ARM11MP_REVB=y
arch/arm/configs/realview-smp_defconfig:CONFIG_REALVIEW_EB_ARM11MP_REVB=y

With that option set, the defconfig kernel should not be able
to boot on RevC machines. Do you have a RevC machine that works
with the defconfig?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier Feb. 18, 2016, 2:47 p.m. | #4
On 18/02/16 14:40, Arnd Bergmann wrote:
> On Thursday 18 February 2016 14:19:48 Marc Zyngier wrote:

>> On 18/02/16 13:46, Linus Walleij wrote:

>>> In the add-on file for the GIC dealing with the RealView family

>>> we currently only handle the PB11MPCore, let's extend this to

>>> manage the RealView EB ARM11MPCore as well. The Revision B of the

>>> ARM11MPCore core tile is a bit special and needs special handling

>>> as it moves a system control register around at random.

>>

>> Ah, 11MPCore RevB. That thing. Tried to locate one in the magic

>> cupboard, and failed. Oh well...

> 

> What surprised me though is that it's enabled by default:

> 

> $ grep REVB arch/arm/configs/realview*

> arch/arm/configs/realview_defconfig:CONFIG_REALVIEW_EB_ARM11MP_REVB=y

> arch/arm/configs/realview-smp_defconfig:CONFIG_REALVIEW_EB_ARM11MP_REVB=y

> 

> With that option set, the defconfig kernel should not be able

> to boot on RevC machines. Do you have a RevC machine that works

> with the defconfig?


No, it looks like someone helped themselves to *all* of the EB-11MPs - I
have a bunch of empty baseboards, and no tile to slap on. I also have a
box of unsoldered 11MPs, but that a different story... ;-)

I guess I should plan for another dumpster diving session.

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Feb. 18, 2016, 6:30 p.m. | #5
On Thu, Feb 18, 2016 at 3:40 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 18 February 2016 14:19:48 Marc Zyngier wrote:

>> On 18/02/16 13:46, Linus Walleij wrote:

>> > In the add-on file for the GIC dealing with the RealView family

>> > we currently only handle the PB11MPCore, let's extend this to

>> > manage the RealView EB ARM11MPCore as well. The Revision B of the

>> > ARM11MPCore core tile is a bit special and needs special handling

>> > as it moves a system control register around at random.

>>

>> Ah, 11MPCore RevB. That thing. Tried to locate one in the magic

>> cupboard, and failed. Oh well...

>

> What surprised me though is that it's enabled by default:


Looks like my fault according to git-blame...

I think this happened when making cover-all defconfigs for
multiplatform. I think I just turned everything on... not realizing
that turning on RevB disabled RevA+RevC.

However since it is the *only* version that works with QEMU
it actually makes some kind of weird sense to have it enabled
since the QEMU version is probably more common than
RevA+RevC...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Feb. 19, 2016, 11:14 a.m. | #6
On Thursday 18 February 2016 19:30:27 Linus Walleij wrote:
> On Thu, Feb 18, 2016 at 3:40 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> > On Thursday 18 February 2016 14:19:48 Marc Zyngier wrote:

> >> On 18/02/16 13:46, Linus Walleij wrote:

> >> > In the add-on file for the GIC dealing with the RealView family

> >> > we currently only handle the PB11MPCore, let's extend this to

> >> > manage the RealView EB ARM11MPCore as well. The Revision B of the

> >> > ARM11MPCore core tile is a bit special and needs special handling

> >> > as it moves a system control register around at random.

> >>

> >> Ah, 11MPCore RevB. That thing. Tried to locate one in the magic

> >> cupboard, and failed. Oh well...

> >

> > What surprised me though is that it's enabled by default:

> 

> Looks like my fault according to git-blame...

> 

> I think this happened when making cover-all defconfigs for

> multiplatform. I think I just turned everything on... not realizing

> that turning on RevB disabled RevA+RevC.

> 

> However since it is the *only* version that works with QEMU

> it actually makes some kind of weird sense to have it enabled

> since the QEMU version is probably more common than

> RevA+RevC...


Do we have reason to believe that any RevA exists?

My understanding from reading the source was that RevB was everything
there was until support for RevC got added and RevB gained a
separate option, or possibly that RevA was the same as RevB but
it was never common enough to even get mentioned in the source.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Feb. 22, 2016, 2:54 a.m. | #7
On Thu, Feb 18, 2016 at 02:46:12PM +0100, Linus Walleij wrote:
> In the add-on file for the GIC dealing with the RealView family

> we currently only handle the PB11MPCore, let's extend this to

> manage the RealView EB ARM11MPCore as well. The Revision B of the

> ARM11MPCore core tile is a bit special and needs special handling

> as it moves a system control register around at random.

> 

> Cc: Arnd Bergmann <arnd@arndb.de>

> Cc: Marc Zyngier <marc.zyngier@arm.com>

> Cc: devicetree@vger.kernel.org

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> This can be applied in isolation from the other patches so Marc

> one you're happy with it, please take it into the IRQchip tree.

> 

> There are two compatible strings getting added to the device tree

> bindings so CC to the DT list. No biggie though, just figures out

> exactly what ARM custom GIC flavor it is.

> ---

>  .../bindings/interrupt-controller/arm,gic.txt      |  2 ++


Acked-by: Rob Herring <robh@kernel.org>


>  drivers/irqchip/irq-gic-realview.c                 | 35 ++++++++++++++++++----

>  2 files changed, 32 insertions(+), 5 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
index 5a1cb4bc3dfe..0c80e6870645 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
@@ -16,6 +16,8 @@  Main node required properties:
 	"arm,cortex-a15-gic"
 	"arm,cortex-a7-gic"
 	"arm,cortex-a9-gic"
+	"arm,eb11mp-gic"
+	"arm,eb11mp-revb-gic"
 	"arm,gic-400"
 	"arm,pl390"
 	"arm,tc11mp-gic"
diff --git a/drivers/irqchip/irq-gic-realview.c b/drivers/irqchip/irq-gic-realview.c
index aa46eb280a7f..224e83d5c056 100644
--- a/drivers/irqchip/irq-gic-realview.c
+++ b/drivers/irqchip/irq-gic-realview.c
@@ -10,7 +10,8 @@ 
 #include <linux/irqchip/arm-gic.h>
 
 #define REALVIEW_SYS_LOCK_OFFSET	0x20
-#define REALVIEW_PB11MP_SYS_PLD_CTRL1	0x74
+#define REALVIEW_SYS_PLD_CTRL1		0x74
+#define REALVIEW_EB_REVB_SYS_PLD_CTRL1	0xD8
 #define VERSATILE_LOCK_VAL		0xA05F
 #define PLD_INTMODE_MASK		BIT(22)|BIT(23)|BIT(24)
 #define PLD_INTMODE_LEGACY		0x0
@@ -18,26 +19,50 @@ 
 #define PLD_INTMODE_NEW_NO_DCC		BIT(23)
 #define PLD_INTMODE_FIQ_ENABLE		BIT(24)
 
+static const struct of_device_id syscon_pldset_of_match[] = {
+	{
+		.compatible = "arm,realview-eb-syscon",
+	},
+	{
+		.compatible = "arm,realview-pb11mp-syscon",
+	},
+	{},
+};
+
 static int __init
 realview_gic_of_init(struct device_node *node, struct device_node *parent)
 {
 	static struct regmap *map;
+	struct device_node *np;
+	struct pld_setting *pldset;
+	u32 pld1_ctrl = REALVIEW_SYS_PLD_CTRL1;
+
+	np = of_find_matching_node_and_match(NULL, syscon_pldset_of_match,
+					     (void *)&pldset);
+	if (!np)
+		return -ENODEV;
+
+	/* For some reason RealView EB Rev B moved this register */
+	if (of_device_is_compatible(np, "arm,eb11mp-revb-gic"))
+		pld1_ctrl = REALVIEW_EB_REVB_SYS_PLD_CTRL1;
 
 	/* The PB11MPCore GIC needs to be configured in the syscon */
-	map = syscon_regmap_lookup_by_compatible("arm,realview-pb11mp-syscon");
+	map = syscon_node_to_regmap(np);
 	if (!IS_ERR(map)) {
 		/* new irq mode with no DCC */
 		regmap_write(map, REALVIEW_SYS_LOCK_OFFSET,
 			     VERSATILE_LOCK_VAL);
-		regmap_update_bits(map, REALVIEW_PB11MP_SYS_PLD_CTRL1,
+		regmap_update_bits(map, pld1_ctrl,
 				   PLD_INTMODE_NEW_NO_DCC,
 				   PLD_INTMODE_MASK);
 		regmap_write(map, REALVIEW_SYS_LOCK_OFFSET, 0x0000);
-		pr_info("TC11MP GIC: set up interrupt controller to NEW mode, no DCC\n");
+		pr_info("RealView GIC: set up interrupt controller to NEW mode, no DCC\n");
 	} else {
-		pr_err("TC11MP GIC setup: could not find syscon\n");
+		pr_err("RealView GIC setup: could not find syscon\n");
 		return -ENXIO;
 	}
 	return gic_of_init(node, parent);
 }
 IRQCHIP_DECLARE(armtc11mp_gic, "arm,tc11mp-gic", realview_gic_of_init);
+IRQCHIP_DECLARE(armeb11mp_gic, "arm,eb11mp-gic", realview_gic_of_init);
+IRQCHIP_DECLARE(armeb11mp_revb_gic, "arm,eb11mp-revb-gic", realview_gic_of_init);