[PATCH v4 0/4] irqchip/gic-v3: Add support to DT based quirk for msm8996

[PATCH v4 0/4] irqchip/gic-v3: Add support to DT based quirk for msm8996

[Srinivas Kandagatla]

Access to GICR_WAKER is restricted on msm8996 SoC in Hypervisor.
There are many devices out there with this restriction in place
and there has been no update to this firmware since last few years,
making those devices totally unusable for upstream development.

My previous attempts to add quirk based on IIDR register value seems to be
flawed by the fact that the value conflicted with other SoCs.
Last Suggestion by Marc Z using compatible seems to be the only way to apply
quirks required for msm8996 based SoCs.

Here is the patchset which add new compatible for msm8996 gicv3 and add
support gic_enable_of_quirks() followed by the actual quirk required for msm8996.

Without this quirk many qcom SoCs (atleast 3 that I know) are
unable to boot mainline.

Changes since v3:
- removed __maybe unused annotation
- moved enabling quirks to gic_of_init() as suggested by Marc
- fixed quick comment as suggest by Marc

Thanks,
Srini

Srinivas Kandagatla (4):
  dt-bindings/gic-v3: Add msm8996 compatible string
  irqchip/gic: common: add support to device tree based quirks
  irqchip: gic-v3: Add quirk for msm8996 secured registers
  arm64: dts: add msm8996 compatible to gicv3

 .../interrupt-controller/arm,gic-v3.txt       |  4 ++-
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |  2 +-
 drivers/irqchip/irq-gic-common.c              | 12 +++++++++
 drivers/irqchip/irq-gic-common.h              |  3 +++
 drivers/irqchip/irq-gic-v3.c                  | 25 +++++++++++++++++++
 5 files changed, 44 insertions(+), 2 deletions(-)

-- 
2.19.2

[PATCH v4 1/4] dt-bindings/gic-v3: Add msm8996 compatible string

[Srinivas Kandagatla]

Access to GICR_WAKER is restricted on msm8996 SoC in Hypervisor.
There are many devices out there with this restriction in place
and there has been no update to this firmware since last few years,
making those devices totally unusable for upstream development.

IIDR register value conflicts with other SoCs, using compatible seems
to be the only way to apply quirks required for msm8996 based SoCs.

Without this quirk many qcom SoCs (atleast 3 that I know) are
unable to boot mainline.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 .../devicetree/bindings/interrupt-controller/arm,gic-v3.txt   | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
index 3ea78c4ef887..b83bb8249074 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
@@ -7,7 +7,9 @@ Interrupts (LPI).
 
 Main node required properties:
 
-- compatible : should at least contain  "arm,gic-v3".
+- compatible : should at least contain  "arm,gic-v3" or either
+		"qcom,msm8996-gic-v3", "arm,gic-v3" for msm8996 SoCs
+		to address SoC specific bugs/quirks
 - interrupt-controller : Identifies the node as an interrupt controller
 - #interrupt-cells : Specifies the number of cells needed to encode an
   interrupt source. Must be a single cell with a value of at least 3.
-- 
2.19.2

[PATCH v4 2/4] irqchip/gic: common: add support to device tree based quirks

[Srinivas Kandagatla]

This patch adds support to device tree based quirks based on
device tree compatible string.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/irqchip/irq-gic-common.c | 12 ++++++++++++
 drivers/irqchip/irq-gic-common.h |  3 +++
 drivers/irqchip/irq-gic-v3.c     |  5 +++++
 3 files changed, 20 insertions(+)

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index 01e673c680cd..3c93c6f4d1f1 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -36,6 +36,18 @@ void gic_set_kvm_info(const struct gic_kvm_info *info)
 	gic_kvm_info = info;
 }
 
+void gic_enable_of_quirks(const struct device_node *np,
+			  const struct gic_quirk *quirks, void *data)
+{
+	for (; quirks->desc; quirks++) {
+		if (!of_device_is_compatible(np, quirks->compatible))
+			continue;
+		if (quirks->init(data))
+			pr_info("GIC: enabling workaround for %s\n",
+				quirks->desc);
+	}
+}
+
 void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
 		void *data)
 {
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index 3919cd7c5285..97e58fb6c232 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -23,6 +23,7 @@
 
 struct gic_quirk {
 	const char *desc;
+	const char *compatible;
 	bool (*init)(void *data);
 	u32 iidr;
 	u32 mask;
@@ -35,6 +36,8 @@ void gic_dist_config(void __iomem *base, int gic_irqs,
 void gic_cpu_config(void __iomem *base, void (*sync_access)(void));
 void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
 		void *data);
+void gic_enable_of_quirks(const struct device_node *np,
+			  const struct gic_quirk *quirks, void *data);
 
 void gic_set_kvm_info(const struct gic_kvm_info *info);
 
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 29e9d47be97d..c95796fa4de6 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1271,6 +1271,9 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
 	gic_set_kvm_info(&gic_v3_kvm_info);
 }
 
+static const struct gic_quirk gic_quirks[] = {
+};
+
 static int __init gic_of_init(struct device_node *node, struct device_node *parent)
 {
 	void __iomem *dist_base;
@@ -1318,6 +1321,8 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
 	if (of_property_read_u64(node, "redistributor-stride", &redist_stride))
 		redist_stride = 0;
 
+	gic_enable_of_quirks(node, gic_quirks, &gic_data);
+
 	err = gic_init_bases(dist_base, rdist_regs, nr_redist_regions,
 			     redist_stride, &node->fwnode);
 	if (err)
-- 
2.19.2

[PATCH v4 3/4] irqchip: gic-v3: Add quirk for msm8996 secured registers

[Srinivas Kandagatla]

Access to GICR_WAKER is restricted on msm8996 SoC in Hypervisor.
Its been more than 2+ years of wait for this to be fixed, which has
no hopes to be fixed. This change was introduced for the "lead device"
on msm8996 platform. It looks like all publicly available msm8996 and
other Qualcomm SoCs have this implementation.

So add a quirk to not access this register on msm8996.

With this quirk MSM8996 can at least boot out of mainline,
which can help community to work with boards based on MSM8996 and other
SoCs with have this restrictions. This Quirk is based on device tree
compatible string.

Without this patch Qualcomm DB820c board reboots when GICR_WAKER
is accessed.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/irqchip/irq-gic-v3.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index c95796fa4de6..1a2fa62a52f0 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -41,6 +41,8 @@
 
 #include "irq-gic-common.h"
 
+#define FLAGS_WORKAROUND_GICR_WAKER_MSM8996	(1ULL << 0)
+
 struct redist_region {
 	void __iomem		*redist_base;
 	phys_addr_t		phys_base;
@@ -55,6 +57,7 @@ struct gic_chip_data {
 	struct irq_domain	*domain;
 	u64			redist_stride;
 	u32			nr_redist_regions;
+	u64			flags;
 	bool			has_rss;
 	unsigned int		irq_nr;
 	struct partition_desc	*ppi_descs[16];
@@ -139,6 +142,9 @@ static void gic_enable_redist(bool enable)
 	u32 count = 1000000;	/* 1s! */
 	u32 val;
 
+	if (gic_data.flags & FLAGS_WORKAROUND_GICR_WAKER_MSM8996)
+		return;
+
 	rbase = gic_data_rdist_rd_base();
 
 	val = readl_relaxed(rbase + GICR_WAKER);
@@ -1067,6 +1073,15 @@ static const struct irq_domain_ops partition_domain_ops = {
 	.select = gic_irq_domain_select,
 };
 
+static bool gic_enable_quirk_msm8996(void *data)
+{
+	struct gic_chip_data *d = data;
+
+	d->flags |= FLAGS_WORKAROUND_GICR_WAKER_MSM8996;
+
+	return true;
+}
+
 static int __init gic_init_bases(void __iomem *dist_base,
 				 struct redist_region *rdist_regs,
 				 u32 nr_redist_regions,
@@ -1272,6 +1287,11 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
 }
 
 static const struct gic_quirk gic_quirks[] = {
+	{
+		.desc	= "GICv3: Qualcomm MSM8996 broken firmware",
+		.compatible = "qcom,msm8996-gic-v3",
+		.init	= gic_enable_quirk_msm8996,
+	}
 };
 
 static int __init gic_of_init(struct device_node *node, struct device_node *parent)
-- 
2.19.2

[PATCH v4 4/4] arm64: dts: add msm8996 compatible to gicv3

[Srinivas Kandagatla]

Add compatible to gicv3 node to enable quirk required to restrict writing
to GICR_WAKER register which is restricted on msm8996 SoC in Hypervisor.

With this quirk MSM8996 can at least boot out of mainline, which can help
community to work with boards based on MSM8996.

Without this patch Qualcomm DB820c board reboots on mainline.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 8585c61e32ef..cd9650cea05d 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -404,7 +404,7 @@
 		};
 
 		intc: interrupt-controller@9bc0000 {
-			compatible = "arm,gic-v3";
+			compatible = "qcom,msm8996-gic-v3", "arm,gic-v3";
 			#interrupt-cells = <3>;
 			interrupt-controller;
 			#redistributor-regions = <1>;
-- 
2.19.2

Re: [PATCH v4 2/4] irqchip/gic: common: add support to device tree based quirks

[Marc Zyngier]

On 10/12/2018 10:23, Srinivas Kandagatla wrote:
> This patch adds support to device tree based quirks based on
> device tree compatible string.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/irqchip/irq-gic-common.c | 12 ++++++++++++
>  drivers/irqchip/irq-gic-common.h |  3 +++
>  drivers/irqchip/irq-gic-v3.c     |  5 +++++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
> index 01e673c680cd..3c93c6f4d1f1 100644
> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -36,6 +36,18 @@ void gic_set_kvm_info(const struct gic_kvm_info *info)
>  	gic_kvm_info = info;
>  }
>  
> +void gic_enable_of_quirks(const struct device_node *np,
> +			  const struct gic_quirk *quirks, void *data)
> +{
> +	for (; quirks->desc; quirks++) {

So you expect quirks->desc to be NULL at some point, just like for any
other quirk table we have.

> +		if (!of_device_is_compatible(np, quirks->compatible))
> +			continue;
> +		if (quirks->init(data))
> +			pr_info("GIC: enabling workaround for %s\n",
> +				quirks->desc);
> +	}
> +}
> +
>  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>  		void *data)
>  {
> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> index 3919cd7c5285..97e58fb6c232 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -23,6 +23,7 @@
>  
>  struct gic_quirk {
>  	const char *desc;
> +	const char *compatible;
>  	bool (*init)(void *data);
>  	u32 iidr;
>  	u32 mask;
> @@ -35,6 +36,8 @@ void gic_dist_config(void __iomem *base, int gic_irqs,
>  void gic_cpu_config(void __iomem *base, void (*sync_access)(void));
>  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>  		void *data);
> +void gic_enable_of_quirks(const struct device_node *np,
> +			  const struct gic_quirk *quirks, void *data);
>  
>  void gic_set_kvm_info(const struct gic_kvm_info *info);
>  
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 29e9d47be97d..c95796fa4de6 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1271,6 +1271,9 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
>  	gic_set_kvm_info(&gic_v3_kvm_info);
>  }
>  
> +static const struct gic_quirk gic_quirks[] = {
> +};

... and yet here you provide an empty table. That's not going to work
very well. You definitely need to have an empty entry at the end of the
array, always.

I guess you want to test your code on a non affected platform, and I'm
pretty sure you'll see it exploding.

> +
>  static int __init gic_of_init(struct device_node *node, struct device_node *parent)
>  {
>  	void __iomem *dist_base;
> @@ -1318,6 +1321,8 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>  	if (of_property_read_u64(node, "redistributor-stride", &redist_stride))
>  		redist_stride = 0;
>  
> +	gic_enable_of_quirks(node, gic_quirks, &gic_data);
> +
>  	err = gic_init_bases(dist_base, rdist_regs, nr_redist_regions,
>  			     redist_stride, &node->fwnode);
>  	if (err)
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/4] irqchip/gic: common: add support to device tree based quirks

[Srinivas Kandagatla]

On 10/12/18 10:55, Marc Zyngier wrote:
>>   
>> +void gic_enable_of_quirks(const struct device_node *np,
>> +			  const struct gic_quirk *quirks, void *data)
>> +{
>> +	for (; quirks->desc; quirks++) {
> So you expect quirks->desc to be NULL at some point, just like for any
> other quirk table we have.
> 
>> +		if (!of_device_is_compatible(np, quirks->compatible))
>> +			continue;
>> +		if (quirks->init(data))
>> +			pr_info("GIC: enabling workaround for %s\n",
>> +				quirks->desc);
>> +	}
>> +}
>> +
>>   void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>>   		void *data)
>>   {
>> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
>> index 3919cd7c5285..97e58fb6c232 100644
>> --- a/drivers/irqchip/irq-gic-common.h
>> +++ b/drivers/irqchip/irq-gic-common.h
>> @@ -23,6 +23,7 @@
>>   
>>   struct gic_quirk {
>>   	const char *desc;
>> +	const char *compatible;
>>   	bool (*init)(void *data);
>>   	u32 iidr;
>>   	u32 mask;
>> @@ -35,6 +36,8 @@ void gic_dist_config(void __iomem *base, int gic_irqs,
>>   void gic_cpu_config(void __iomem *base, void (*sync_access)(void));
>>   void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>>   		void *data);
>> +void gic_enable_of_quirks(const struct device_node *np,
>> +			  const struct gic_quirk *quirks, void *data);
>>   
>>   void gic_set_kvm_info(const struct gic_kvm_info *info);
>>   
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 29e9d47be97d..c95796fa4de6 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -1271,6 +1271,9 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
>>   	gic_set_kvm_info(&gic_v3_kvm_info);
>>   }
>>   
>> +static const struct gic_quirk gic_quirks[] = {
>> +};
> ... and yet here you provide an empty table. That's not going to work
> very well. You definitely need to have an empty entry at the end of the
> array, always.
> 
> I guess you want to test your code on a non affected platform, and I'm
> pretty sure you'll see it exploding.

Yes, Should have carefully looked at this!!
We need an empty entry at the end!
I will fix it and send a new version!

Thanks,
srini
>