linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
@ 2019-06-10 12:13 Abel Vesa
  2019-06-10 12:13 ` [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171 Abel Vesa
                   ` (3 more replies)
  0 siblings, 4 replies; 38+ messages in thread
From: Abel Vesa @ 2019-06-10 12:13 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, Thomas Gleixner,
	Marc Zyngier, Lucas Stach, Bai Ping, Lorenzo Pieralisi,
	Leonard Crestez
  Cc: devicetree, Carlo Caione, NXP Linux Team, linux-arm-kernel, linux-kernel

This is another alternative for the RFC:
https://lkml.org/lkml/2019/3/27/545

This new workaround proposal is a little bit more hacky but more contained
since everything is done within the irq-imx-gpcv2 driver.

Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
handler and registers instead a wrapper which calls in the 'hijacked' 
handler, after that calling into EL3 which will take care of the actual
wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.

I also have the patches ready for TF-A but I'll hold on to them until I see if
this has a chance of getting in.

Abel Vesa (2):
  irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171
  arm64: dts: imx8mq: Add idle states and gpcv2 wake_request broken
    property

 arch/arm64/boot/dts/freescale/imx8mq.dtsi | 20 +++++++++++++++
 drivers/irqchip/irq-imx-gpcv2.c           | 42 +++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171
  2019-06-10 12:13 [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ Abel Vesa
@ 2019-06-10 12:13 ` Abel Vesa
  2019-06-10 12:38   ` Leonard Crestez
  2019-06-10 13:24   ` Marc Zyngier
  2019-06-10 12:13 ` [RFC 2/2] arm64: dts: imx8mq: Add idle states and gpcv2 wake_request broken property Abel Vesa
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 38+ messages in thread
From: Abel Vesa @ 2019-06-10 12:13 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, Thomas Gleixner,
	Marc Zyngier, Lucas Stach, Bai Ping, Lorenzo Pieralisi,
	Leonard Crestez
  Cc: devicetree, Carlo Caione, NXP Linux Team, linux-arm-kernel, linux-kernel

i.MX8MQ is missing the wake_request signals from GIC to GPCv2. This indirectly
breaks cpuidle support due to inability to wake target cores on IPIs.

Here is the link to the errata (see e11171):

https://www.nxp.com/docs/en/errata/IMX8MDQLQ_0N14W.pdf

Now, in order to fix this, we can trigger IRQ 32 (hwirq 0) to all the cores by
setting 12th bit in IOMUX_GPR1 register. In order to control the target cores
only, that is, not waking up all the cores every time, we can unmask/mask the
IRQ 32 in the first GPC IMR register. So basically we can leave the IOMUX_GPR1
12th bit always set and just play with the masking and unmasking the IRO 32 for
each independent core.

Since EL3 is the one that deals with powering down/up the cores, and since the
cores wake up in EL3, EL3 should be the one to control the IMRs in this case.
This implies we need to get into EL3 on every IPI to do the unmasking, leaving
the masking to be done on the power-up sequence by the core itself.

In order to be able to get into EL3 on each IPI, we 'hijack' the registered smp
cross call handler, in this case the gic_raise_softirq which is registered by
the irq-gic-v3 driver and register our own handler instead. This new handler is
basically a wrapper over the hijacked handler plus the call into EL3.

To get into EL3, we use a custom vendor SIP id added just for this purpose.

All of this is conditional for i.MX8MQ only.

Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
---
 drivers/irqchip/irq-imx-gpcv2.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
index 66501ea..b921105 100644
--- a/drivers/irqchip/irq-imx-gpcv2.c
+++ b/drivers/irqchip/irq-imx-gpcv2.c
@@ -6,11 +6,19 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/arm-smccc.h>
+#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/irqchip.h>
 #include <linux/syscore_ops.h>
+#include <linux/smp.h>
+
+#define IMX_SIP_GPC		0xC2000004
+#define IMX_SIP_GPC_CORE_WAKE	0x00
 
 #define IMR_NUM			4
 #define GPC_MAX_IRQS            (IMR_NUM * 32)
@@ -73,6 +81,37 @@ static struct syscore_ops imx_gpcv2_syscore_ops = {
 	.resume		= gpcv2_wakeup_source_restore,
 };
 
+static void (*__gic_v3_smp_cross_call)(const struct cpumask *, unsigned int);
+
+static void imx_gpcv2_raise_softirq(const struct cpumask *mask,
+					  unsigned int irq)
+{
+	struct arm_smccc_res res;
+
+	/* call the hijacked smp cross call handler */
+	__gic_v3_smp_cross_call(mask, irq);
+
+	/* now call into EL3 and take care of the wakeup */
+	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_GPC_CORE_WAKE,
+			*cpumask_bits(mask), 0, 0, 0, 0, 0, &res);
+}
+
+static void imx_gpcv2_wake_request_fixup(void)
+{
+	struct regmap *iomux_gpr;
+
+	/* hijack the already registered smp cross call handler */
+	__gic_v3_smp_cross_call = __smp_cross_call;
+
+	/* register our workaround handler for smp cross call */
+	set_smp_cross_call(imx_gpcv2_raise_softirq);
+
+	iomux_gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
+	if (!IS_ERR(iomux_gpr))
+		regmap_update_bits(iomux_gpr, IOMUXC_GPR1, IMX6Q_GPR1_GINT,
+					IMX6Q_GPR1_GINT);
+}
+
 static int imx_gpcv2_irq_set_wake(struct irq_data *d, unsigned int on)
 {
 	struct gpcv2_irqchip_data *cd = d->chip_data;
@@ -269,6 +308,9 @@ static int __init imx_gpcv2_irqchip_init(struct device_node *node,
 		cd->wakeup_sources[i] = ~0;
 	}
 
+	if (of_property_read_bool(node, "broken-wake-request-signals"))
+		imx_gpcv2_wake_request_fixup();
+
 	/* Let CORE0 as the default CPU to wake up by GPC */
 	cd->cpu2wakeup = GPC_IMR1_CORE0;
 
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [RFC 2/2] arm64: dts: imx8mq: Add idle states and gpcv2 wake_request broken property
  2019-06-10 12:13 [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ Abel Vesa
  2019-06-10 12:13 ` [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171 Abel Vesa
@ 2019-06-10 12:13 ` Abel Vesa
  2019-06-10 13:19 ` [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ Mark Rutland
  2019-06-23 11:47 ` Martin Kepplinger
  3 siblings, 0 replies; 38+ messages in thread
From: Abel Vesa @ 2019-06-10 12:13 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, Thomas Gleixner,
	Marc Zyngier, Lucas Stach, Bai Ping, Lorenzo Pieralisi,
	Leonard Crestez
  Cc: devicetree, Carlo Caione, NXP Linux Team, linux-arm-kernel, linux-kernel

Add the cpu-sleep idle state with all the necessary parameters and also add
the cpu-idle-states to the cpu nodes.

The 'broken-wake-request-signals' property is used to let the irq-imx-gpcv2
driver know that the wake request signals from GIC are not linked to the
GPC at all and, therefore, the driver should  make use of the dedicated
workaround to explicitly wake up the target core on every IPI.

Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mq.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index d09b808..7217138 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -103,6 +103,7 @@
 			#cooling-cells = <2>;
 			nvmem-cells = <&cpu_speed_grade>;
 			nvmem-cell-names = "speed_grade";
+			cpu-idle-states = <&CPU_SLEEP>;
 		};
 
 		A53_1: cpu@1 {
@@ -115,6 +116,7 @@
 			next-level-cache = <&A53_L2>;
 			operating-points-v2 = <&a53_opp_table>;
 			#cooling-cells = <2>;
+			cpu-idle-states = <&CPU_SLEEP>;
 		};
 
 		A53_2: cpu@2 {
@@ -127,6 +129,7 @@
 			next-level-cache = <&A53_L2>;
 			operating-points-v2 = <&a53_opp_table>;
 			#cooling-cells = <2>;
+			cpu-idle-states = <&CPU_SLEEP>;
 		};
 
 		A53_3: cpu@3 {
@@ -139,11 +142,27 @@
 			next-level-cache = <&A53_L2>;
 			operating-points-v2 = <&a53_opp_table>;
 			#cooling-cells = <2>;
+			cpu-idle-states = <&CPU_SLEEP>;
 		};
 
 		A53_L2: l2-cache0 {
 			compatible = "cache";
 		};
+
+		idle-states {
+			entry-method = "psci";
+
+			CPU_SLEEP: cpu-sleep {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x0010033>;
+				local-timer-stop;
+				entry-latency-us = <1000>;
+				exit-latency-us = <700>;
+				min-residency-us = <2700>;
+				wakeup-latency-us = <1500>;
+			};
+		};
+
 	};
 
 	a53_opp_table: opp-table {
@@ -502,6 +521,7 @@
 				reg = <0x303a0000 0x10000>;
 				interrupt-parent = <&gic>;
 				interrupt-controller;
+				broken-wake-request-signals;
 				#interrupt-cells = <3>;
 
 				pgc {
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171
  2019-06-10 12:13 ` [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171 Abel Vesa
@ 2019-06-10 12:38   ` Leonard Crestez
  2019-06-10 13:24   ` Marc Zyngier
  1 sibling, 0 replies; 38+ messages in thread
From: Leonard Crestez @ 2019-06-10 12:38 UTC (permalink / raw)
  To: Abel Vesa, Marc Zyngier, Lucas Stach
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring,
	dl-linux-imx, Pengutronix Kernel Team, Thomas Gleixner,
	Fabio Estevam, linux-arm-kernel

On 6/10/2019 3:15 PM, Abel Vesa wrote:
> i.MX8MQ is missing the wake_request signals from GIC to GPCv2. This indirectly
> breaks cpuidle support due to inability to wake target cores on IPIs.
> 
> Now, in order to fix this, we can trigger IRQ 32 (hwirq 0) to all the cores by
> setting 12th bit in IOMUX_GPR1 register. In order to control the target cores
> only, that is, not waking up all the cores every time, we can unmask/mask the
> IRQ 32 in the first GPC IMR register.
> 
> Since EL3 is the one that deals with powering down/up the cores, and since the
> cores wake up in EL3, EL3 should be the one to control the IMRs in this case.
> This implies we need to get into EL3 on every IPI to do the unmasking, leaving
> the masking to be done on the power-up sequence by the core itself.

Manipulating same IMR registers in TF-A and Linux is racy so all IMR 
manipulation (set_wake etc) needs to be done through SIP calls with 
locking inside TF-A.

It would make sense to have an entirely separate SIP-based 
irq-imx8mq-gpc.c driver based on what is used in NXP tree.

> +	iomux_gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> +	if (!IS_ERR(iomux_gpr))
> +		regmap_update_bits(iomux_gpr, IOMUXC_GPR1, IMX6Q_GPR1_GINT,
> +					IMX6Q_GPR1_GINT);

Doesn't this initialization belong in TF-A? On boot enable the irq and 
keep it masked until somebody calls "wake".

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-10 12:13 [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ Abel Vesa
  2019-06-10 12:13 ` [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171 Abel Vesa
  2019-06-10 12:13 ` [RFC 2/2] arm64: dts: imx8mq: Add idle states and gpcv2 wake_request broken property Abel Vesa
@ 2019-06-10 13:19 ` Mark Rutland
  2019-06-10 13:29   ` Abel Vesa
  2019-06-23 11:47 ` Martin Kepplinger
  3 siblings, 1 reply; 38+ messages in thread
From: Mark Rutland @ 2019-06-10 13:19 UTC (permalink / raw)
  To: Abel Vesa
  Cc: devicetree, Lorenzo Pieralisi, Bai Ping, Carlo Caione,
	Marc Zyngier, Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring,
	NXP Linux Team, Pengutronix Kernel Team, Thomas Gleixner,
	Leonard Crestez, Fabio Estevam, linux-arm-kernel, Lucas Stach

On Mon, Jun 10, 2019 at 03:13:44PM +0300, Abel Vesa wrote:
> This is another alternative for the RFC:
> https://lkml.org/lkml/2019/3/27/545
> 
> This new workaround proposal is a little bit more hacky but more contained
> since everything is done within the irq-imx-gpcv2 driver.
> 
> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
> handler and registers instead a wrapper which calls in the 'hijacked' 
> handler, after that calling into EL3 which will take care of the actual
> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.

IIUC from last time [1,2], this erratum affects all interrupts
targetting teh idle CPU, not just IPIs, so even if the bodge is more
self-contained, it doesn't really solve the issue, and there are still
cases where a CPU will not be woken from idle when it should be (e.g.
upon receipt of an LPI).

IIUC, Marc, Lorenzo, and Rafael [1,2,3] all thought that that this was
not worthwhile. What's changed?

Thanks,
Mark.

[1] https://lkml.org/lkml/2019/3/28/197
[2] https://lkml.org/lkml/2019/3/28/203
[3] https://lkml.org/lkml/2019/3/28/198

> 
> I also have the patches ready for TF-A but I'll hold on to them until I see if
> this has a chance of getting in.
> 
> Abel Vesa (2):
>   irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171
>   arm64: dts: imx8mq: Add idle states and gpcv2 wake_request broken
>     property
> 
>  arch/arm64/boot/dts/freescale/imx8mq.dtsi | 20 +++++++++++++++
>  drivers/irqchip/irq-imx-gpcv2.c           | 42 +++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> -- 
> 2.7.4
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171
  2019-06-10 12:13 ` [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171 Abel Vesa
  2019-06-10 12:38   ` Leonard Crestez
@ 2019-06-10 13:24   ` Marc Zyngier
  2019-06-10 13:38     ` Abel Vesa
  1 sibling, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2019-06-10 13:24 UTC (permalink / raw)
  To: Abel Vesa, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, Thomas Gleixner,
	Lucas Stach, Bai Ping, Lorenzo Pieralisi, Leonard Crestez
  Cc: devicetree, Carlo Caione, NXP Linux Team, linux-arm-kernel, linux-kernel

Abel,

On 10/06/2019 13:13, Abel Vesa wrote:
> i.MX8MQ is missing the wake_request signals from GIC to GPCv2. This indirectly
> breaks cpuidle support due to inability to wake target cores on IPIs.
> 
> Here is the link to the errata (see e11171):
> 
> https://www.nxp.com/docs/en/errata/IMX8MDQLQ_0N14W.pdf
> 
> Now, in order to fix this, we can trigger IRQ 32 (hwirq 0) to all the cores by
> setting 12th bit in IOMUX_GPR1 register. In order to control the target cores
> only, that is, not waking up all the cores every time, we can unmask/mask the
> IRQ 32 in the first GPC IMR register. So basically we can leave the IOMUX_GPR1
> 12th bit always set and just play with the masking and unmasking the IRO 32 for
> each independent core.
> 
> Since EL3 is the one that deals with powering down/up the cores, and since the
> cores wake up in EL3, EL3 should be the one to control the IMRs in this case.
> This implies we need to get into EL3 on every IPI to do the unmasking, leaving
> the masking to be done on the power-up sequence by the core itself.
> 
> In order to be able to get into EL3 on each IPI, we 'hijack' the registered smp
> cross call handler, in this case the gic_raise_softirq which is registered by
> the irq-gic-v3 driver and register our own handler instead. This new handler is
> basically a wrapper over the hijacked handler plus the call into EL3.
> 
> To get into EL3, we use a custom vendor SIP id added just for this purpose.
> 
> All of this is conditional for i.MX8MQ only.
> 
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> ---
>  drivers/irqchip/irq-imx-gpcv2.c | 42 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> index 66501ea..b921105 100644
> --- a/drivers/irqchip/irq-imx-gpcv2.c
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -6,11 +6,19 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/arm-smccc.h>
> +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> +#include <linux/regmap.h>
>  #include <linux/slab.h>
>  #include <linux/irqchip.h>
>  #include <linux/syscore_ops.h>
> +#include <linux/smp.h>
> +
> +#define IMX_SIP_GPC		0xC2000004
> +#define IMX_SIP_GPC_CORE_WAKE	0x00
>  
>  #define IMR_NUM			4
>  #define GPC_MAX_IRQS            (IMR_NUM * 32)
> @@ -73,6 +81,37 @@ static struct syscore_ops imx_gpcv2_syscore_ops = {
>  	.resume		= gpcv2_wakeup_source_restore,
>  };
>  
> +static void (*__gic_v3_smp_cross_call)(const struct cpumask *, unsigned int);
> +
> +static void imx_gpcv2_raise_softirq(const struct cpumask *mask,
> +					  unsigned int irq)
> +{
> +	struct arm_smccc_res res;
> +
> +	/* call the hijacked smp cross call handler */
> +	__gic_v3_smp_cross_call(mask, irq);

I'm feeling a bit sick... :-(

> +
> +	/* now call into EL3 and take care of the wakeup */
> +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_GPC_CORE_WAKE,
> +			*cpumask_bits(mask), 0, 0, 0, 0, 0, &res);

There is a number of things that look wrong here:

- What guarantees that this SMC call actually exists? The DT itself just
says that this is broken, and not much about EL3.

- What guarantees that the cpumask matches the physical layout? I could
have booted via kexec, and logical CPU0 is now physical CPU3.

- What if you have more than 64 CPUs? Probably not a big deal for this
particular instance, but I fully expect people to get it wrong again on
the next iteration if we "fix" it for them.

- How does it work on a 32bit kernel, when firmware advertises a SMC64 call?

And also:

- IMX_SIP_GMC doesn't strike me as a very distinctive name. It certainly
doesn't say *which* SiP is responsible for this wonderful thing. I
understand that they would like to stay anonymous, but still...

- It isn't clear what you gain from relying on the kernel to send the
SGI, while you could do the whole thing at EL3.

> +}
> +
> +static void imx_gpcv2_wake_request_fixup(void)
> +{
> +	struct regmap *iomux_gpr;
> +
> +	/* hijack the already registered smp cross call handler */
> +	__gic_v3_smp_cross_call = __smp_cross_call;
> +
> +	/* register our workaround handler for smp cross call */
> +	set_smp_cross_call(imx_gpcv2_raise_softirq);
> +
> +	iomux_gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> +	if (!IS_ERR(iomux_gpr))
> +		regmap_update_bits(iomux_gpr, IOMUXC_GPR1, IMX6Q_GPR1_GINT,
> +					IMX6Q_GPR1_GINT);
> +}
> +
>  static int imx_gpcv2_irq_set_wake(struct irq_data *d, unsigned int on)
>  {
>  	struct gpcv2_irqchip_data *cd = d->chip_data;
> @@ -269,6 +308,9 @@ static int __init imx_gpcv2_irqchip_init(struct device_node *node,
>  		cd->wakeup_sources[i] = ~0;
>  	}
>  
> +	if (of_property_read_bool(node, "broken-wake-request-signals"))
> +		imx_gpcv2_wake_request_fixup();
> +
>  	/* Let CORE0 as the default CPU to wake up by GPC */
>  	cd->cpu2wakeup = GPC_IMR1_CORE0;
>  
> 

Overall, I find this horrible, and pretty pointless:

- Supporting this in mainline is likely to be a loosing battle (let's
get real here, nobody will get the updated firmware)

- It doesn't solve some other problems such as the lack of LPI and PPI
wakeup, as it was outlined in the previous iteration of such workaround.

Given this, I'm drawing the same conclusion as last time: this isn't fit
for mainline. The real mainline fix is to prevent the use of cpuidle.
This workaround is best kept in a vendor-specific tree.

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-10 13:19 ` [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ Mark Rutland
@ 2019-06-10 13:29   ` Abel Vesa
  2019-06-10 13:39     ` Marc Zyngier
  0 siblings, 1 reply; 38+ messages in thread
From: Abel Vesa @ 2019-06-10 13:29 UTC (permalink / raw)
  To: Mark Rutland
  Cc: devicetree, Lorenzo Pieralisi, Jacky Bai, Carlo Caione,
	Marc Zyngier, Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring,
	dl-linux-imx, Pengutronix Kernel Team, Abel Vesa,
	Thomas Gleixner, Leonard Crestez, Fabio Estevam,
	linux-arm-kernel, Lucas Stach

On 19-06-10 14:19:21, Mark Rutland wrote:
> On Mon, Jun 10, 2019 at 03:13:44PM +0300, Abel Vesa wrote:
> > This is another alternative for the RFC:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C05d512f83dfa4d4f52d908d6eda64321%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636957695741584637&amp;sdata=d3X0xyWiaotq4VPNW306wdRhsY4TI%2BBjRSABk6vzf%2B8%3D&amp;reserved=0
> > 
> > This new workaround proposal is a little bit more hacky but more contained
> > since everything is done within the irq-imx-gpcv2 driver.
> > 
> > Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
> > handler and registers instead a wrapper which calls in the 'hijacked' 
> > handler, after that calling into EL3 which will take care of the actual
> > wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
> 
> IIUC from last time [1,2], this erratum affects all interrupts
> targetting teh idle CPU, not just IPIs, so even if the bodge is more
> self-contained, it doesn't really solve the issue, and there are still
> cases where a CPU will not be woken from idle when it should be (e.g.
> upon receipt of an LPI).
> 

Wrong, this erratum does not affect any other type of interrupts, other
than IPIs. That is because all the other interrupts go through GPC,
which means the cores will wake up on any other type (again, other than IPI).

> IIUC, Marc, Lorenzo, and Rafael [1,2,3] all thought that that this was
> not worthwhile. What's changed?

The fact that this is done in the imx-gpcv2 driver and it's not spread
around like the old RFC. Yes, I agree that fixing something like this
from the core subsystems (like cpuidle) or irq-gic-v3 driver is a bad
idea, but this is not the case anymore with this new RFC.

> 
> Thanks,
> Mark.
> 
> [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F28%2F197&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C05d512f83dfa4d4f52d908d6eda64321%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636957695741584637&amp;sdata=cA5UKbFuZHHnk1599lJi2QXCMTKxCJmPPzoBaRhbdCE%3D&amp;reserved=0
> [2] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F28%2F203&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C05d512f83dfa4d4f52d908d6eda64321%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636957695741584637&amp;sdata=TrWSY3eozWSd0KwZgIprmPazdDno979NqGnVjpdzi50%3D&amp;reserved=0
> [3] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F28%2F198&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C05d512f83dfa4d4f52d908d6eda64321%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636957695741584637&amp;sdata=ge%2FOXE40T6GSb0x1SmYFXtwLIdyVy1W0Yl0EItKyXNU%3D&amp;reserved=0
> 
> > 
> > I also have the patches ready for TF-A but I'll hold on to them until I see if
> > this has a chance of getting in.
> > 
> > Abel Vesa (2):
> >   irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171
> >   arm64: dts: imx8mq: Add idle states and gpcv2 wake_request broken
> >     property
> > 
> >  arch/arm64/boot/dts/freescale/imx8mq.dtsi | 20 +++++++++++++++
> >  drivers/irqchip/irq-imx-gpcv2.c           | 42 +++++++++++++++++++++++++++++++
> >  2 files changed, 62 insertions(+)
> > 
> > -- 
> > 2.7.4
> > 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171
  2019-06-10 13:24   ` Marc Zyngier
@ 2019-06-10 13:38     ` Abel Vesa
  2019-06-10 13:51       ` Marc Zyngier
  0 siblings, 1 reply; 38+ messages in thread
From: Abel Vesa @ 2019-06-10 13:38 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Fabio Estevam, Sascha Hauer, linux-kernel,
	Rob Herring, dl-linux-imx, Pengutronix Kernel Team, Abel Vesa,
	Thomas Gleixner, Leonard Crestez, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On 19-06-10 14:24:11, Marc Zyngier wrote:
> Abel,
> 
> On 10/06/2019 13:13, Abel Vesa wrote:
> > i.MX8MQ is missing the wake_request signals from GIC to GPCv2. This indirectly
> > breaks cpuidle support due to inability to wake target cores on IPIs.
> > 
> > Here is the link to the errata (see e11171):
> > 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.nxp.com%2Fdocs%2Fen%2Ferrata%2FIMX8MDQLQ_0N14W.pdf&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Ce23f69dbe37c4e83d7ab08d6eda6f062%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636957698629664098&amp;sdata=tAFuqTJBWiSbeoUv8gqA9vQfeWAklCv3t4qk0RLJQKM%3D&amp;reserved=0
> > 
> > Now, in order to fix this, we can trigger IRQ 32 (hwirq 0) to all the cores by
> > setting 12th bit in IOMUX_GPR1 register. In order to control the target cores
> > only, that is, not waking up all the cores every time, we can unmask/mask the
> > IRQ 32 in the first GPC IMR register. So basically we can leave the IOMUX_GPR1
> > 12th bit always set and just play with the masking and unmasking the IRO 32 for
> > each independent core.
> > 
> > Since EL3 is the one that deals with powering down/up the cores, and since the
> > cores wake up in EL3, EL3 should be the one to control the IMRs in this case.
> > This implies we need to get into EL3 on every IPI to do the unmasking, leaving
> > the masking to be done on the power-up sequence by the core itself.
> > 
> > In order to be able to get into EL3 on each IPI, we 'hijack' the registered smp
> > cross call handler, in this case the gic_raise_softirq which is registered by
> > the irq-gic-v3 driver and register our own handler instead. This new handler is
> > basically a wrapper over the hijacked handler plus the call into EL3.
> > 
> > To get into EL3, we use a custom vendor SIP id added just for this purpose.
> > 
> > All of this is conditional for i.MX8MQ only.
> > 
> > Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> > ---
> >  drivers/irqchip/irq-imx-gpcv2.c | 42 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> > index 66501ea..b921105 100644
> > --- a/drivers/irqchip/irq-imx-gpcv2.c
> > +++ b/drivers/irqchip/irq-imx-gpcv2.c
> > @@ -6,11 +6,19 @@
> >   * published by the Free Software Foundation.
> >   */
> >  
> > +#include <linux/arm-smccc.h>
> > +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> > +#include <linux/mfd/syscon.h>
> >  #include <linux/of_address.h>
> >  #include <linux/of_irq.h>
> > +#include <linux/regmap.h>
> >  #include <linux/slab.h>
> >  #include <linux/irqchip.h>
> >  #include <linux/syscore_ops.h>
> > +#include <linux/smp.h>
> > +
> > +#define IMX_SIP_GPC		0xC2000004
> > +#define IMX_SIP_GPC_CORE_WAKE	0x00
> >  
> >  #define IMR_NUM			4
> >  #define GPC_MAX_IRQS            (IMR_NUM * 32)
> > @@ -73,6 +81,37 @@ static struct syscore_ops imx_gpcv2_syscore_ops = {
> >  	.resume		= gpcv2_wakeup_source_restore,
> >  };
> >  
> > +static void (*__gic_v3_smp_cross_call)(const struct cpumask *, unsigned int);
> > +
> > +static void imx_gpcv2_raise_softirq(const struct cpumask *mask,
> > +					  unsigned int irq)
> > +{
> > +	struct arm_smccc_res res;
> > +
> > +	/* call the hijacked smp cross call handler */
> > +	__gic_v3_smp_cross_call(mask, irq);
> 
> I'm feeling a bit sick... :-(
> 
> > +
> > +	/* now call into EL3 and take care of the wakeup */
> > +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_GPC_CORE_WAKE,
> > +			*cpumask_bits(mask), 0, 0, 0, 0, 0, &res);
> 
> There is a number of things that look wrong here:
> 
> - What guarantees that this SMC call actually exists? The DT itself just
> says that this is broken, and not much about EL3.

OK, that's easy to fix.

> 
> - What guarantees that the cpumask matches the physical layout? I could
> have booted via kexec, and logical CPU0 is now physical CPU3.
> 

Fair point. I didn't think of that. Will have to put some thought into it.

> - What if you have more than 64 CPUs? Probably not a big deal for this
> particular instance, but I fully expect people to get it wrong again on
> the next iteration if we "fix" it for them.

That will never be the case. This is done in the irq-imx-gpcv2, so it
won't be used by any other platform. It's just a workaround for the 
gpcv2.

> 
> - How does it work on a 32bit kernel, when firmware advertises a SMC64 call?
> 

This is also easy to fix.

> And also:
> 
> - IMX_SIP_GMC doesn't strike me as a very distinctive name. It certainly
> doesn't say *which* SiP is responsible for this wonderful thing. I
> understand that they would like to stay anonymous, but still...
> 

Fair point. The idea is to have a class of SIPs just for the GPC needed actions.
One thing that will come in the near future is the move of all the IMR 
(Interrupt Mask Register) control (which is part of the GPC) to TF-A.
This IMX_SIP_GPC will be extended then to every action required by the IMR
and so on. Remember, GPC is more than a power controller. It's an irqchip
too.

> - It isn't clear what you gain from relying on the kernel to send the
> SGI, while you could do the whole thing at EL3.

OK, how would you suggest to wake a core on an IPI from EL3 ?

> 
> > +}
> > +
> > +static void imx_gpcv2_wake_request_fixup(void)
> > +{
> > +	struct regmap *iomux_gpr;
> > +
> > +	/* hijack the already registered smp cross call handler */
> > +	__gic_v3_smp_cross_call = __smp_cross_call;
> > +
> > +	/* register our workaround handler for smp cross call */
> > +	set_smp_cross_call(imx_gpcv2_raise_softirq);
> > +
> > +	iomux_gpr = syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
> > +	if (!IS_ERR(iomux_gpr))
> > +		regmap_update_bits(iomux_gpr, IOMUXC_GPR1, IMX6Q_GPR1_GINT,
> > +					IMX6Q_GPR1_GINT);
> > +}
> > +
> >  static int imx_gpcv2_irq_set_wake(struct irq_data *d, unsigned int on)
> >  {
> >  	struct gpcv2_irqchip_data *cd = d->chip_data;
> > @@ -269,6 +308,9 @@ static int __init imx_gpcv2_irqchip_init(struct device_node *node,
> >  		cd->wakeup_sources[i] = ~0;
> >  	}
> >  
> > +	if (of_property_read_bool(node, "broken-wake-request-signals"))
> > +		imx_gpcv2_wake_request_fixup();
> > +
> >  	/* Let CORE0 as the default CPU to wake up by GPC */
> >  	cd->cpu2wakeup = GPC_IMR1_CORE0;
> >  
> > 
> 
> Overall, I find this horrible, and pretty pointless:
> 
> - Supporting this in mainline is likely to be a loosing battle (let's
> get real here, nobody will get the updated firmware)
> 
> - It doesn't solve some other problems such as the lack of LPI and PPI
> wakeup, as it was outlined in the previous iteration of such workaround.
> 
> Given this, I'm drawing the same conclusion as last time: this isn't fit
> for mainline. The real mainline fix is to prevent the use of cpuidle.
> This workaround is best kept in a vendor-specific tree.
> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-10 13:29   ` Abel Vesa
@ 2019-06-10 13:39     ` Marc Zyngier
  2019-06-10 13:55       ` Abel Vesa
  0 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2019-06-10 13:39 UTC (permalink / raw)
  To: Abel Vesa, Mark Rutland
  Cc: devicetree, Lorenzo Pieralisi, Jacky Bai, Carlo Caione,
	Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring, dl-linux-imx,
	Pengutronix Kernel Team, Abel Vesa, Thomas Gleixner,
	Leonard Crestez, Fabio Estevam, linux-arm-kernel, Lucas Stach

On 10/06/2019 14:29, Abel Vesa wrote:
> On 19-06-10 14:19:21, Mark Rutland wrote:
>> On Mon, Jun 10, 2019 at 03:13:44PM +0300, Abel Vesa wrote:
>>> This is another alternative for the RFC:
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C05d512f83dfa4d4f52d908d6eda64321%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636957695741584637&amp;sdata=d3X0xyWiaotq4VPNW306wdRhsY4TI%2BBjRSABk6vzf%2B8%3D&amp;reserved=0
>>>
>>> This new workaround proposal is a little bit more hacky but more contained
>>> since everything is done within the irq-imx-gpcv2 driver.
>>>
>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>> handler and registers instead a wrapper which calls in the 'hijacked' 
>>> handler, after that calling into EL3 which will take care of the actual
>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>
>> IIUC from last time [1,2], this erratum affects all interrupts
>> targetting teh idle CPU, not just IPIs, so even if the bodge is more
>> self-contained, it doesn't really solve the issue, and there are still
>> cases where a CPU will not be woken from idle when it should be (e.g.
>> upon receipt of an LPI).
>>
> 
> Wrong, this erratum does not affect any other type of interrupts, other
> than IPIs. That is because all the other interrupts go through GPC,
> which means the cores will wake up on any other type (again, other than IPI).

Huh... Are you saying that LPIs and PPIs are going through the GPC, and
will trigger the wake-up of the core? That's not the conclusion we
reached last time.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171
  2019-06-10 13:38     ` Abel Vesa
@ 2019-06-10 13:51       ` Marc Zyngier
  2019-06-10 14:12         ` Abel Vesa
  0 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2019-06-10 13:51 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Fabio Estevam, Sascha Hauer, linux-kernel,
	Rob Herring, dl-linux-imx, Pengutronix Kernel Team, Abel Vesa,
	Thomas Gleixner, Leonard Crestez, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On 10/06/2019 14:38, Abel Vesa wrote:
> On 19-06-10 14:24:11, Marc Zyngier wrote:
>> Abel,
>>
>> On 10/06/2019 13:13, Abel Vesa wrote:
>>> i.MX8MQ is missing the wake_request signals from GIC to GPCv2. This indirectly
>>> breaks cpuidle support due to inability to wake target cores on IPIs.
>>>
>>> Here is the link to the errata (see e11171):
>>>
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.nxp.com%2Fdocs%2Fen%2Ferrata%2FIMX8MDQLQ_0N14W.pdf&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Ce23f69dbe37c4e83d7ab08d6eda6f062%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636957698629664098&amp;sdata=tAFuqTJBWiSbeoUv8gqA9vQfeWAklCv3t4qk0RLJQKM%3D&amp;reserved=0
>>>
>>> Now, in order to fix this, we can trigger IRQ 32 (hwirq 0) to all the cores by
>>> setting 12th bit in IOMUX_GPR1 register. In order to control the target cores
>>> only, that is, not waking up all the cores every time, we can unmask/mask the
>>> IRQ 32 in the first GPC IMR register. So basically we can leave the IOMUX_GPR1
>>> 12th bit always set and just play with the masking and unmasking the IRO 32 for
>>> each independent core.
>>>
>>> Since EL3 is the one that deals with powering down/up the cores, and since the
>>> cores wake up in EL3, EL3 should be the one to control the IMRs in this case.
>>> This implies we need to get into EL3 on every IPI to do the unmasking, leaving
>>> the masking to be done on the power-up sequence by the core itself.
>>>
>>> In order to be able to get into EL3 on each IPI, we 'hijack' the registered smp
>>> cross call handler, in this case the gic_raise_softirq which is registered by
>>> the irq-gic-v3 driver and register our own handler instead. This new handler is
>>> basically a wrapper over the hijacked handler plus the call into EL3.
>>>
>>> To get into EL3, we use a custom vendor SIP id added just for this purpose.
>>>
>>> All of this is conditional for i.MX8MQ only.
>>>
>>> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
>>> ---
>>>  drivers/irqchip/irq-imx-gpcv2.c | 42 +++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
>>> index 66501ea..b921105 100644
>>> --- a/drivers/irqchip/irq-imx-gpcv2.c
>>> +++ b/drivers/irqchip/irq-imx-gpcv2.c
>>> @@ -6,11 +6,19 @@
>>>   * published by the Free Software Foundation.
>>>   */
>>>  
>>> +#include <linux/arm-smccc.h>
>>> +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
>>> +#include <linux/mfd/syscon.h>
>>>  #include <linux/of_address.h>
>>>  #include <linux/of_irq.h>
>>> +#include <linux/regmap.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/irqchip.h>
>>>  #include <linux/syscore_ops.h>
>>> +#include <linux/smp.h>
>>> +
>>> +#define IMX_SIP_GPC		0xC2000004
>>> +#define IMX_SIP_GPC_CORE_WAKE	0x00
>>>  
>>>  #define IMR_NUM			4
>>>  #define GPC_MAX_IRQS            (IMR_NUM * 32)
>>> @@ -73,6 +81,37 @@ static struct syscore_ops imx_gpcv2_syscore_ops = {
>>>  	.resume		= gpcv2_wakeup_source_restore,
>>>  };
>>>  
>>> +static void (*__gic_v3_smp_cross_call)(const struct cpumask *, unsigned int);
>>> +
>>> +static void imx_gpcv2_raise_softirq(const struct cpumask *mask,
>>> +					  unsigned int irq)
>>> +{
>>> +	struct arm_smccc_res res;
>>> +
>>> +	/* call the hijacked smp cross call handler */
>>> +	__gic_v3_smp_cross_call(mask, irq);
>>
>> I'm feeling a bit sick... :-(
>>
>>> +
>>> +	/* now call into EL3 and take care of the wakeup */
>>> +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_GPC_CORE_WAKE,
>>> +			*cpumask_bits(mask), 0, 0, 0, 0, 0, &res);
>>
>> There is a number of things that look wrong here:
>>
>> - What guarantees that this SMC call actually exists? The DT itself just
>> says that this is broken, and not much about EL3.
> 
> OK, that's easy to fix.

Sure. How?

> 
>>
>> - What guarantees that the cpumask matches the physical layout? I could
>> have booted via kexec, and logical CPU0 is now physical CPU3.
>>
> 
> Fair point. I didn't think of that. Will have to put some thought into it.
> 
>> - What if you have more than 64 CPUs? Probably not a big deal for this
>> particular instance, but I fully expect people to get it wrong again on
>> the next iteration if we "fix" it for them.
> 
> That will never be the case. This is done in the irq-imx-gpcv2, so it
> won't be used by any other platform. It's just a workaround for the 
> gpcv2.

"never"? That's a pretty strong statement. Given that the same IP has
been carried across a number of implementations, I fully expect imx9 (or
whatever the next generation is labeled) to use the same stuff.

> 
>>
>> - How does it work on a 32bit kernel, when firmware advertises a SMC64 call?
>>
> 
> This is also easy to fix.
> 
>> And also:
>>
>> - IMX_SIP_GMC doesn't strike me as a very distinctive name. It certainly
>> doesn't say *which* SiP is responsible for this wonderful thing. I
>> understand that they would like to stay anonymous, but still...
>>
> 
> Fair point. The idea is to have a class of SIPs just for the GPC needed actions.

I don't know what meaning you give to the "SIP" acronym, but the SMCCC
documentation clearly has a different definition:

"SiP	: Silicon Partner. In this document, the silicon manufacturer."

What I'm asking for is that the silicon vendor's name to be clearly
spoken out.

> One thing that will come in the near future is the move of all the IMR 
> (Interrupt Mask Register) control (which is part of the GPC) to TF-A.
> This IMX_SIP_GPC will be extended then to every action required by the IMR
> and so on. Remember, GPC is more than a power controller. It's an irqchip
> too.
> 
>> - It isn't clear what you gain from relying on the kernel to send the
>> SGI, while you could do the whole thing at EL3.
> 
> OK, how would you suggest to wake a core on an IPI from EL3 ?

Erm... By writing to the ICC_SGI1R_EL1 system register, directly from
EL3, just before you apply your workaround?

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-10 13:39     ` Marc Zyngier
@ 2019-06-10 13:55       ` Abel Vesa
  2019-06-10 14:07         ` Marc Zyngier
  0 siblings, 1 reply; 38+ messages in thread
From: Abel Vesa @ 2019-06-10 13:55 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Fabio Estevam, Sascha Hauer, linux-kernel,
	Rob Herring, dl-linux-imx, Pengutronix Kernel Team, Abel Vesa,
	Thomas Gleixner, Leonard Crestez, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On 19-06-10 14:39:02, Marc Zyngier wrote:
> On 10/06/2019 14:29, Abel Vesa wrote:
> > On 19-06-10 14:19:21, Mark Rutland wrote:
> >> On Mon, Jun 10, 2019 at 03:13:44PM +0300, Abel Vesa wrote:
> >>> This is another alternative for the RFC:
> >>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C7cb2bda286214541bd1e08d6eda903e0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636957707535314247&amp;sdata=guYqyq5ND6HzW6doFyWrR1Ry4ffWpGwtm0xDZ2ufFSg%3D&amp;reserved=0
> >>>
> >>> This new workaround proposal is a little bit more hacky but more contained
> >>> since everything is done within the irq-imx-gpcv2 driver.
> >>>
> >>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
> >>> handler and registers instead a wrapper which calls in the 'hijacked' 
> >>> handler, after that calling into EL3 which will take care of the actual
> >>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
> >>
> >> IIUC from last time [1,2], this erratum affects all interrupts
> >> targetting teh idle CPU, not just IPIs, so even if the bodge is more
> >> self-contained, it doesn't really solve the issue, and there are still
> >> cases where a CPU will not be woken from idle when it should be (e.g.
> >> upon receipt of an LPI).
> >>
> > 
> > Wrong, this erratum does not affect any other type of interrupts, other
> > than IPIs. That is because all the other interrupts go through GPC,
> > which means the cores will wake up on any other type (again, other than IPI).
> 
> Huh... Are you saying that LPIs and PPIs are going through the GPC, and
> will trigger the wake-up of the core? That's not the conclusion we
> reached last time.
> 

Hmm, I don't think that was the conclusion. Yes, Lucas was saying (IIRC)
that if you terminate the IRQs at GIC then all the other interrupts will be
in the same situation. But the performance improvement given by terminating
them at GIC might not be worth it when compared to the cpuidle support.

> 	M.
> -- 
> Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-10 13:55       ` Abel Vesa
@ 2019-06-10 14:07         ` Marc Zyngier
  2019-06-10 14:32           ` Leonard Crestez
  0 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2019-06-10 14:07 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Fabio Estevam, Sascha Hauer, linux-kernel,
	Rob Herring, dl-linux-imx, Pengutronix Kernel Team, Abel Vesa,
	Thomas Gleixner, Leonard Crestez, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On 10/06/2019 14:55, Abel Vesa wrote:
> On 19-06-10 14:39:02, Marc Zyngier wrote:
>> On 10/06/2019 14:29, Abel Vesa wrote:
>>> On 19-06-10 14:19:21, Mark Rutland wrote:
>>>> On Mon, Jun 10, 2019 at 03:13:44PM +0300, Abel Vesa wrote:
>>>>> This is another alternative for the RFC:
>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C7cb2bda286214541bd1e08d6eda903e0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636957707535314247&amp;sdata=guYqyq5ND6HzW6doFyWrR1Ry4ffWpGwtm0xDZ2ufFSg%3D&amp;reserved=0
>>>>>
>>>>> This new workaround proposal is a little bit more hacky but more contained
>>>>> since everything is done within the irq-imx-gpcv2 driver.
>>>>>
>>>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>>>> handler and registers instead a wrapper which calls in the 'hijacked' 
>>>>> handler, after that calling into EL3 which will take care of the actual
>>>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>>
>>>> IIUC from last time [1,2], this erratum affects all interrupts
>>>> targetting teh idle CPU, not just IPIs, so even if the bodge is more
>>>> self-contained, it doesn't really solve the issue, and there are still
>>>> cases where a CPU will not be woken from idle when it should be (e.g.
>>>> upon receipt of an LPI).
>>>>
>>>
>>> Wrong, this erratum does not affect any other type of interrupts, other
>>> than IPIs. That is because all the other interrupts go through GPC,
>>> which means the cores will wake up on any other type (again, other than IPI).
>>
>> Huh... Are you saying that LPIs and PPIs are going through the GPC, and
>> will trigger the wake-up of the core? That's not the conclusion we
>> reached last time.
>>
> 
> Hmm, I don't think that was the conclusion. Yes, Lucas was saying (IIRC)
> that if you terminate the IRQs at GIC then all the other interrupts will be
> in the same situation. But the performance improvement given by terminating
> them at GIC might not be worth it when compared to the cpuidle support.

https://lkml.org/lkml/2019/3/27/1124 clearly says that PPIs are broken,
relying on some other terrible hack for the timer (and only the timer,
leaving other PPIs dead as a nail). It also implies that LPIs have never
been looked into, and given that they aren't routed through the GPC, the
conclusion is pretty easy to draw.

Nobody is talking about performance here. It is strictly about
correctness, and what I read about this system is that it cannot
reliably use cpuidle.

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171
  2019-06-10 13:51       ` Marc Zyngier
@ 2019-06-10 14:12         ` Abel Vesa
  2019-06-10 14:28           ` Marc Zyngier
  0 siblings, 1 reply; 38+ messages in thread
From: Abel Vesa @ 2019-06-10 14:12 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Fabio Estevam, Sascha Hauer, linux-kernel,
	Rob Herring, dl-linux-imx, Pengutronix Kernel Team, Abel Vesa,
	Thomas Gleixner, Leonard Crestez, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On 19-06-10 14:51:48, Marc Zyngier wrote:
> On 10/06/2019 14:38, Abel Vesa wrote:
> > On 19-06-10 14:24:11, Marc Zyngier wrote:
> >> Abel,
> >>
> >> On 10/06/2019 13:13, Abel Vesa wrote:
> >>> i.MX8MQ is missing the wake_request signals from GIC to GPCv2. This indirectly
> >>> breaks cpuidle support due to inability to wake target cores on IPIs.
> >>>
> >>> Here is the link to the errata (see e11171):
> >>>
> >>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.nxp.com%2Fdocs%2Fen%2Ferrata%2FIMX8MDQLQ_0N14W.pdf&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Cf74b196c8beb46599f8408d6edaace09%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636957715230445874&amp;sdata=ruP3qm1NTLTdoLC5XDu0uN5yNKLb4%2F2iP9kF5vdr1OI%3D&amp;reserved=0
> >>>
> >>> Now, in order to fix this, we can trigger IRQ 32 (hwirq 0) to all the cores by
> >>> setting 12th bit in IOMUX_GPR1 register. In order to control the target cores
> >>> only, that is, not waking up all the cores every time, we can unmask/mask the
> >>> IRQ 32 in the first GPC IMR register. So basically we can leave the IOMUX_GPR1
> >>> 12th bit always set and just play with the masking and unmasking the IRO 32 for
> >>> each independent core.
> >>>
> >>> Since EL3 is the one that deals with powering down/up the cores, and since the
> >>> cores wake up in EL3, EL3 should be the one to control the IMRs in this case.
> >>> This implies we need to get into EL3 on every IPI to do the unmasking, leaving
> >>> the masking to be done on the power-up sequence by the core itself.
> >>>
> >>> In order to be able to get into EL3 on each IPI, we 'hijack' the registered smp
> >>> cross call handler, in this case the gic_raise_softirq which is registered by
> >>> the irq-gic-v3 driver and register our own handler instead. This new handler is
> >>> basically a wrapper over the hijacked handler plus the call into EL3.
> >>>
> >>> To get into EL3, we use a custom vendor SIP id added just for this purpose.
> >>>
> >>> All of this is conditional for i.MX8MQ only.
> >>>
> >>> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> >>> ---
> >>>  drivers/irqchip/irq-imx-gpcv2.c | 42 +++++++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 42 insertions(+)
> >>>
> >>> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> >>> index 66501ea..b921105 100644
> >>> --- a/drivers/irqchip/irq-imx-gpcv2.c
> >>> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> >>> @@ -6,11 +6,19 @@
> >>>   * published by the Free Software Foundation.
> >>>   */
> >>>  
> >>> +#include <linux/arm-smccc.h>
> >>> +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> >>> +#include <linux/mfd/syscon.h>
> >>>  #include <linux/of_address.h>
> >>>  #include <linux/of_irq.h>
> >>> +#include <linux/regmap.h>
> >>>  #include <linux/slab.h>
> >>>  #include <linux/irqchip.h>
> >>>  #include <linux/syscore_ops.h>
> >>> +#include <linux/smp.h>
> >>> +
> >>> +#define IMX_SIP_GPC		0xC2000004
> >>> +#define IMX_SIP_GPC_CORE_WAKE	0x00
> >>>  
> >>>  #define IMR_NUM			4
> >>>  #define GPC_MAX_IRQS            (IMR_NUM * 32)
> >>> @@ -73,6 +81,37 @@ static struct syscore_ops imx_gpcv2_syscore_ops = {
> >>>  	.resume		= gpcv2_wakeup_source_restore,
> >>>  };
> >>>  
> >>> +static void (*__gic_v3_smp_cross_call)(const struct cpumask *, unsigned int);
> >>> +
> >>> +static void imx_gpcv2_raise_softirq(const struct cpumask *mask,
> >>> +					  unsigned int irq)
> >>> +{
> >>> +	struct arm_smccc_res res;
> >>> +
> >>> +	/* call the hijacked smp cross call handler */
> >>> +	__gic_v3_smp_cross_call(mask, irq);
> >>
> >> I'm feeling a bit sick... :-(
> >>
> >>> +
> >>> +	/* now call into EL3 and take care of the wakeup */
> >>> +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_GPC_CORE_WAKE,
> >>> +			*cpumask_bits(mask), 0, 0, 0, 0, 0, &res);
> >>
> >> There is a number of things that look wrong here:
> >>
> >> - What guarantees that this SMC call actually exists? The DT itself just
> >> says that this is broken, and not much about EL3.
> > 
> > OK, that's easy to fix.
> 
> Sure. How?
> 

If the SMC_UNK is returned, then we keep the IOMUX_GPR1 bit 12 set and the IMR1 bit 0
for that core unset. That would always wake up the cores and therefore no the
cpuidle will not have any effect.

> > 
> >>
> >> - What guarantees that the cpumask matches the physical layout? I could
> >> have booted via kexec, and logical CPU0 is now physical CPU3.
> >>
> > 
> > Fair point. I didn't think of that. Will have to put some thought into it.
> > 
> >> - What if you have more than 64 CPUs? Probably not a big deal for this
> >> particular instance, but I fully expect people to get it wrong again on
> >> the next iteration if we "fix" it for them.
> > 
> > That will never be the case. This is done in the irq-imx-gpcv2, so it
> > won't be used by any other platform. It's just a workaround for the 
> > gpcv2.
> 
> "never"? That's a pretty strong statement. Given that the same IP has
> been carried across a number of implementations, I fully expect imx9 (or
> whatever the next generation is labeled) to use the same stuff.
> 

Again, this workaround will only apply to i.MX8MQ. IIRC, the gic500 was the
one that added the wake_request signals, gic400 didn't gave them.
And i.MX8MQ is the first NXP SoC to use the gic500. All the newer i.MX SoC
which use GPCv2 don't have this issue. So it's obviously related to
the switch from gic400 to gic500 when interfacing with GPCv2.

> > 
> >>
> >> - How does it work on a 32bit kernel, when firmware advertises a SMC64 call?
> >>
> > 
> > This is also easy to fix.
> > 
> >> And also:
> >>
> >> - IMX_SIP_GMC doesn't strike me as a very distinctive name. It certainly
> >> doesn't say *which* SiP is responsible for this wonderful thing. I
> >> understand that they would like to stay anonymous, but still...
> >>
> > 
> > Fair point. The idea is to have a class of SIPs just for the GPC needed actions.
> 
> I don't know what meaning you give to the "SIP" acronym, but the SMCCC
> documentation clearly has a different definition:
> 
> "SiP	: Silicon Partner. In this document, the silicon manufacturer."
> 
> What I'm asking for is that the silicon vendor's name to be clearly
> spoken out.

Fair point. TBH, I used the same naming I found in some other subsystems upstream.
If you grep the tree for IMX_SIP you will find IMX_SIP_TIMER, IMX_SIP_SRTC and
IMX_SIP_CPUFREQ.

So I only followed the pattern here.

> 
> > One thing that will come in the near future is the move of all the IMR 
> > (Interrupt Mask Register) control (which is part of the GPC) to TF-A.
> > This IMX_SIP_GPC will be extended then to every action required by the IMR
> > and so on. Remember, GPC is more than a power controller. It's an irqchip
> > too.
> > 
> >> - It isn't clear what you gain from relying on the kernel to send the
> >> SGI, while you could do the whole thing at EL3.
> > 
> > OK, how would you suggest to wake a core on an IPI from EL3 ?
> 
> Erm... By writing to the ICC_SGI1R_EL1 system register, directly from
> EL3, just before you apply your workaround?

Right, but how will you know in EL3 that an IPI has been raised ?

> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171
  2019-06-10 14:12         ` Abel Vesa
@ 2019-06-10 14:28           ` Marc Zyngier
  0 siblings, 0 replies; 38+ messages in thread
From: Marc Zyngier @ 2019-06-10 14:28 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Fabio Estevam, Sascha Hauer, linux-kernel,
	Rob Herring, dl-linux-imx, Pengutronix Kernel Team, Abel Vesa,
	Thomas Gleixner, Leonard Crestez, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On 10/06/2019 15:12, Abel Vesa wrote:
> On 19-06-10 14:51:48, Marc Zyngier wrote:
>> On 10/06/2019 14:38, Abel Vesa wrote:
>>> On 19-06-10 14:24:11, Marc Zyngier wrote:
>>>> Abel,
>>>>
>>>> On 10/06/2019 13:13, Abel Vesa wrote:
>>>>> i.MX8MQ is missing the wake_request signals from GIC to GPCv2. This indirectly
>>>>> breaks cpuidle support due to inability to wake target cores on IPIs.
>>>>>
>>>>> Here is the link to the errata (see e11171):
>>>>>
>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.nxp.com%2Fdocs%2Fen%2Ferrata%2FIMX8MDQLQ_0N14W.pdf&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Cf74b196c8beb46599f8408d6edaace09%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636957715230445874&amp;sdata=ruP3qm1NTLTdoLC5XDu0uN5yNKLb4%2F2iP9kF5vdr1OI%3D&amp;reserved=0
>>>>>
>>>>> Now, in order to fix this, we can trigger IRQ 32 (hwirq 0) to all the cores by
>>>>> setting 12th bit in IOMUX_GPR1 register. In order to control the target cores
>>>>> only, that is, not waking up all the cores every time, we can unmask/mask the
>>>>> IRQ 32 in the first GPC IMR register. So basically we can leave the IOMUX_GPR1
>>>>> 12th bit always set and just play with the masking and unmasking the IRO 32 for
>>>>> each independent core.
>>>>>
>>>>> Since EL3 is the one that deals with powering down/up the cores, and since the
>>>>> cores wake up in EL3, EL3 should be the one to control the IMRs in this case.
>>>>> This implies we need to get into EL3 on every IPI to do the unmasking, leaving
>>>>> the masking to be done on the power-up sequence by the core itself.
>>>>>
>>>>> In order to be able to get into EL3 on each IPI, we 'hijack' the registered smp
>>>>> cross call handler, in this case the gic_raise_softirq which is registered by
>>>>> the irq-gic-v3 driver and register our own handler instead. This new handler is
>>>>> basically a wrapper over the hijacked handler plus the call into EL3.
>>>>>
>>>>> To get into EL3, we use a custom vendor SIP id added just for this purpose.
>>>>>
>>>>> All of this is conditional for i.MX8MQ only.
>>>>>
>>>>> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
>>>>> ---
>>>>>  drivers/irqchip/irq-imx-gpcv2.c | 42 +++++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
>>>>> index 66501ea..b921105 100644
>>>>> --- a/drivers/irqchip/irq-imx-gpcv2.c
>>>>> +++ b/drivers/irqchip/irq-imx-gpcv2.c
>>>>> @@ -6,11 +6,19 @@
>>>>>   * published by the Free Software Foundation.
>>>>>   */
>>>>>  
>>>>> +#include <linux/arm-smccc.h>
>>>>> +#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
>>>>> +#include <linux/mfd/syscon.h>
>>>>>  #include <linux/of_address.h>
>>>>>  #include <linux/of_irq.h>
>>>>> +#include <linux/regmap.h>
>>>>>  #include <linux/slab.h>
>>>>>  #include <linux/irqchip.h>
>>>>>  #include <linux/syscore_ops.h>
>>>>> +#include <linux/smp.h>
>>>>> +
>>>>> +#define IMX_SIP_GPC		0xC2000004
>>>>> +#define IMX_SIP_GPC_CORE_WAKE	0x00
>>>>>  
>>>>>  #define IMR_NUM			4
>>>>>  #define GPC_MAX_IRQS            (IMR_NUM * 32)
>>>>> @@ -73,6 +81,37 @@ static struct syscore_ops imx_gpcv2_syscore_ops = {
>>>>>  	.resume		= gpcv2_wakeup_source_restore,
>>>>>  };
>>>>>  
>>>>> +static void (*__gic_v3_smp_cross_call)(const struct cpumask *, unsigned int);
>>>>> +
>>>>> +static void imx_gpcv2_raise_softirq(const struct cpumask *mask,
>>>>> +					  unsigned int irq)
>>>>> +{
>>>>> +	struct arm_smccc_res res;
>>>>> +
>>>>> +	/* call the hijacked smp cross call handler */
>>>>> +	__gic_v3_smp_cross_call(mask, irq);
>>>>
>>>> I'm feeling a bit sick... :-(
>>>>
>>>>> +
>>>>> +	/* now call into EL3 and take care of the wakeup */
>>>>> +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_GPC_CORE_WAKE,
>>>>> +			*cpumask_bits(mask), 0, 0, 0, 0, 0, &res);
>>>>
>>>> There is a number of things that look wrong here:
>>>>
>>>> - What guarantees that this SMC call actually exists? The DT itself just
>>>> says that this is broken, and not much about EL3.
>>>
>>> OK, that's easy to fix.
>>
>> Sure. How?
>>
> 
> If the SMC_UNK is returned, then we keep the IOMUX_GPR1 bit 12 set and the IMR1 bit 0
> for that core unset. That would always wake up the cores and therefore no the
> cpuidle will not have any effect.
> 
>>>
>>>>
>>>> - What guarantees that the cpumask matches the physical layout? I could
>>>> have booted via kexec, and logical CPU0 is now physical CPU3.
>>>>
>>>
>>> Fair point. I didn't think of that. Will have to put some thought into it.
>>>
>>>> - What if you have more than 64 CPUs? Probably not a big deal for this
>>>> particular instance, but I fully expect people to get it wrong again on
>>>> the next iteration if we "fix" it for them.
>>>
>>> That will never be the case. This is done in the irq-imx-gpcv2, so it
>>> won't be used by any other platform. It's just a workaround for the 
>>> gpcv2.
>>
>> "never"? That's a pretty strong statement. Given that the same IP has
>> been carried across a number of implementations, I fully expect imx9 (or
>> whatever the next generation is labeled) to use the same stuff.
>>
> 
> Again, this workaround will only apply to i.MX8MQ. IIRC, the gic500 was the
> one that added the wake_request signals, gic400 didn't gave them.
> And i.MX8MQ is the first NXP SoC to use the gic500. All the newer i.MX SoC
> which use GPCv2 don't have this issue. So it's obviously related to
> the switch from gic400 to gic500 when interfacing with GPCv2.

I can only admire your optimism.

> 
>>>
>>>>
>>>> - How does it work on a 32bit kernel, when firmware advertises a SMC64 call?
>>>>
>>>
>>> This is also easy to fix.
>>>
>>>> And also:
>>>>
>>>> - IMX_SIP_GMC doesn't strike me as a very distinctive name. It certainly
>>>> doesn't say *which* SiP is responsible for this wonderful thing. I
>>>> understand that they would like to stay anonymous, but still...
>>>>
>>>
>>> Fair point. The idea is to have a class of SIPs just for the GPC needed actions.
>>
>> I don't know what meaning you give to the "SIP" acronym, but the SMCCC
>> documentation clearly has a different definition:
>>
>> "SiP	: Silicon Partner. In this document, the silicon manufacturer."
>>
>> What I'm asking for is that the silicon vendor's name to be clearly
>> spoken out.
> 
> Fair point. TBH, I used the same naming I found in some other subsystems upstream.
> If you grep the tree for IMX_SIP you will find IMX_SIP_TIMER, IMX_SIP_SRTC and
> IMX_SIP_CPUFREQ.
> 
> So I only followed the pattern here.
> 
>>
>>> One thing that will come in the near future is the move of all the IMR 
>>> (Interrupt Mask Register) control (which is part of the GPC) to TF-A.
>>> This IMX_SIP_GPC will be extended then to every action required by the IMR
>>> and so on. Remember, GPC is more than a power controller. It's an irqchip
>>> too.
>>>
>>>> - It isn't clear what you gain from relying on the kernel to send the
>>>> SGI, while you could do the whole thing at EL3.
>>>
>>> OK, how would you suggest to wake a core on an IPI from EL3 ?
>>
>> Erm... By writing to the ICC_SGI1R_EL1 system register, directly from
>> EL3, just before you apply your workaround?
> 
> Right, but how will you know in EL3 that an IPI has been raised ?

Because that's what you do at EL3. Don't call into the GIC driver, but
just deal with IPIs entirely at EL3.

But that's a pretty moot point, as this workaround only addresses part
of the overall issue.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-10 14:07         ` Marc Zyngier
@ 2019-06-10 14:32           ` Leonard Crestez
  2019-06-10 14:52             ` Marc Zyngier
  2019-06-12  7:14             ` Thomas Gleixner
  0 siblings, 2 replies; 38+ messages in thread
From: Leonard Crestez @ 2019-06-10 14:32 UTC (permalink / raw)
  To: Marc Zyngier, Abel Vesa, Lucas Stach
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Fabio Estevam, Sascha Hauer, linux-kernel,
	Rob Herring, dl-linux-imx, Pengutronix Kernel Team, Abel Vesa,
	Thomas Gleixner, Shawn Guo, linux-arm-kernel

On 6/10/2019 5:08 PM, Marc Zyngier wrote:
> On 10/06/2019 14:55, Abel Vesa wrote:
>> On 19-06-10 14:39:02, Marc Zyngier wrote:
>>> On 10/06/2019 14:29, Abel Vesa wrote:
>>>> On 19-06-10 14:19:21, Mark Rutland wrote:
>>>>> On Mon, Jun 10, 2019 at 03:13:44PM +0300, Abel Vesa wrote:

>>>>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>>>>> handler and registers instead a wrapper which calls in the 'hijacked'
>>>>>> handler, after that calling into EL3 which will take care of the actual
>>>>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>>>
>>>>> IIUC from last time [1,2], this erratum affects all interrupts
>>>>> targetting teh idle CPU, not just IPIs, so even if the bodge is more
>>>>> self-contained, it doesn't really solve the issue, and there are still
>>>>> cases where a CPU will not be woken from idle when it should be (e.g.
>>>>> upon receipt of an LPI).
>>>>
>>>> Wrong, this erratum does not affect any other type of interrupts, other
>>>> than IPIs. That is because all the other interrupts go through GPC,
>>>> which means the cores will wake up on any other type (again, other than IPI).
>>>
>>> Huh... Are you saying that LPIs and PPIs are going through the GPC, and
>>> will trigger the wake-up of the core? That's not the conclusion we
>>> reached last time.
>>
>> Hmm, I don't think that was the conclusion. Yes, Lucas was saying (IIRC)
>> that if you terminate the IRQs at GIC then all the other interrupts will be
>> in the same situation. But the performance improvement given by terminating
>> them at GIC might not be worth it when compared to the cpuidle support.
> 
> PPIs are broken,
> relying on some other terrible hack for the timer (and only the timer,
> leaving other PPIs dead as a nail). It also implies that LPIs have never
> been looked into, and given that they aren't routed through the GPC, the
> conclusion is pretty easy to draw.
> 
> Nobody is talking about performance here. It is strictly about
> correctness, and what I read about this system is that it cannot
> reliably use cpuidle.
My argument was that it's fine if PPIs and LPIs are broken as long as 
they're not used:

  * PPIs are only used for local timer which is not used for wakeup.
  * LPIs on imx are not currently implemented.

This workaround is only targeted at a very specific SOC with specific 
usecases and in that context it behaves correctly, as far as I can tell.

As mentioned in another thread the HW issue was already solved in newer 
chips of the same family (like imx8mm). If there is a need for PPIs and 
LPIs on imx8mq in the future then maybe we can detect that scenario and 
disable cpuidle?

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-10 14:32           ` Leonard Crestez
@ 2019-06-10 14:52             ` Marc Zyngier
  2019-06-12  7:14             ` Thomas Gleixner
  1 sibling, 0 replies; 38+ messages in thread
From: Marc Zyngier @ 2019-06-10 14:52 UTC (permalink / raw)
  To: Leonard Crestez, Abel Vesa, Lucas Stach
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Fabio Estevam, Sascha Hauer, linux-kernel,
	Rob Herring, dl-linux-imx, Pengutronix Kernel Team, Abel Vesa,
	Thomas Gleixner, Shawn Guo, linux-arm-kernel

On 10/06/2019 15:32, Leonard Crestez wrote:
> On 6/10/2019 5:08 PM, Marc Zyngier wrote:
>> On 10/06/2019 14:55, Abel Vesa wrote:
>>> On 19-06-10 14:39:02, Marc Zyngier wrote:
>>>> On 10/06/2019 14:29, Abel Vesa wrote:
>>>>> On 19-06-10 14:19:21, Mark Rutland wrote:
>>>>>> On Mon, Jun 10, 2019 at 03:13:44PM +0300, Abel Vesa wrote:
> 
>>>>>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>>>>>> handler and registers instead a wrapper which calls in the 'hijacked'
>>>>>>> handler, after that calling into EL3 which will take care of the actual
>>>>>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>>>>
>>>>>> IIUC from last time [1,2], this erratum affects all interrupts
>>>>>> targetting teh idle CPU, not just IPIs, so even if the bodge is more
>>>>>> self-contained, it doesn't really solve the issue, and there are still
>>>>>> cases where a CPU will not be woken from idle when it should be (e.g.
>>>>>> upon receipt of an LPI).
>>>>>
>>>>> Wrong, this erratum does not affect any other type of interrupts, other
>>>>> than IPIs. That is because all the other interrupts go through GPC,
>>>>> which means the cores will wake up on any other type (again, other than IPI).
>>>>
>>>> Huh... Are you saying that LPIs and PPIs are going through the GPC, and
>>>> will trigger the wake-up of the core? That's not the conclusion we
>>>> reached last time.
>>>
>>> Hmm, I don't think that was the conclusion. Yes, Lucas was saying (IIRC)
>>> that if you terminate the IRQs at GIC then all the other interrupts will be
>>> in the same situation. But the performance improvement given by terminating
>>> them at GIC might not be worth it when compared to the cpuidle support.
>>
>> PPIs are broken,
>> relying on some other terrible hack for the timer (and only the timer,
>> leaving other PPIs dead as a nail). It also implies that LPIs have never
>> been looked into, and given that they aren't routed through the GPC, the
>> conclusion is pretty easy to draw.
>>
>> Nobody is talking about performance here. It is strictly about
>> correctness, and what I read about this system is that it cannot
>> reliably use cpuidle.
> My argument was that it's fine if PPIs and LPIs are broken as long as 
> they're not used:
> 
>   * PPIs are only used for local timer which is not used for wakeup.

How about the PMU and GIC maintenance interrupts? Any interrupt should
get you out of idle.

>   * LPIs on imx are not currently implemented.

Define "implemented". You don't have an ITS at all? Or is it that you
currently don't expose the ITS in your firmware?

> This workaround is only targeted at a very specific SOC with specific 
> usecases and in that context it behaves correctly, as far as I can tell.

And I still maintain that such specific use cases should be kept
specific, and that the mainline kernel should be reliable in all
circumstances.

> As mentioned in another thread the HW issue was already solved in newer 
> chips of the same family (like imx8mm). If there is a need for PPIs and 
> LPIs on imx8mq in the future then maybe we can detect that scenario and 
> disable cpuidle?

I'd suggest it the other way around. No cpuidle unless you absolutely
force it, tainting the kernel in the process.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-10 14:32           ` Leonard Crestez
  2019-06-10 14:52             ` Marc Zyngier
@ 2019-06-12  7:14             ` Thomas Gleixner
  2019-06-12  7:35               ` Marc Zyngier
  1 sibling, 1 reply; 38+ messages in thread
From: Thomas Gleixner @ 2019-06-12  7:14 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Abel Vesa,
	Carlo Caione, Marc Zyngier, Fabio Estevam, Sascha Hauer,
	linux-kernel, Rob Herring, Jacky Bai, dl-linux-imx,
	Pengutronix Kernel Team, Abel Vesa, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On Mon, 10 Jun 2019, Leonard Crestez wrote:
> On 6/10/2019 5:08 PM, Marc Zyngier wrote:
> > Nobody is talking about performance here. It is strictly about
> > correctness, and what I read about this system is that it cannot
> > reliably use cpuidle.
> My argument was that it's fine if PPIs and LPIs are broken as long as 
> they're not used:
> 
>   * PPIs are only used for local timer which is not used for wakeup.

Huch? The timer has to bring the CPU out of idle as any other interrupt. 

Thanks,

	tglx

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-12  7:14             ` Thomas Gleixner
@ 2019-06-12  7:35               ` Marc Zyngier
  2019-06-12  7:37                 ` Thomas Gleixner
  0 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2019-06-12  7:35 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Abel Vesa,
	Carlo Caione, Fabio Estevam, Sascha Hauer, linux-kernel,
	Rob Herring, Jacky Bai, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Leonard Crestez, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On Wed, 12 Jun 2019 08:14:16 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 10 Jun 2019, Leonard Crestez wrote:
> > On 6/10/2019 5:08 PM, Marc Zyngier wrote:
> > > Nobody is talking about performance here. It is strictly about
> > > correctness, and what I read about this system is that it cannot
> > > reliably use cpuidle.
> > My argument was that it's fine if PPIs and LPIs are broken as long as 
> > they're not used:
> > 
> >   * PPIs are only used for local timer which is not used for wakeup.
> 
> Huch? The timer has to bring the CPU out of idle as any other
> interrupt.

They use a separate hack for that, pretending that the timer is
stopped during idle (it isn't), and setup a broadcast timer when
entering idle. That timer uses an interrupt that can wake-up the
target CPU, and all is well in the world. Sort of.

Of course, this breaks as PPIs are not only used by the timer, but
also by a number of other HW bits (PMU, GIC, guest and hypervisor
timers), and they don't have corresponding hacks to back them up.

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-12  7:35               ` Marc Zyngier
@ 2019-06-12  7:37                 ` Thomas Gleixner
  0 siblings, 0 replies; 38+ messages in thread
From: Thomas Gleixner @ 2019-06-12  7:37 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Abel Vesa,
	Carlo Caione, Fabio Estevam, Sascha Hauer, linux-kernel,
	Rob Herring, Jacky Bai, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Leonard Crestez, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On Wed, 12 Jun 2019, Marc Zyngier wrote:
> On Wed, 12 Jun 2019 08:14:16 +0100,
> Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Mon, 10 Jun 2019, Leonard Crestez wrote:
> > > On 6/10/2019 5:08 PM, Marc Zyngier wrote:
> > > > Nobody is talking about performance here. It is strictly about
> > > > correctness, and what I read about this system is that it cannot
> > > > reliably use cpuidle.
> > > My argument was that it's fine if PPIs and LPIs are broken as long as 
> > > they're not used:
> > > 
> > >   * PPIs are only used for local timer which is not used for wakeup.
> > 
> > Huch? The timer has to bring the CPU out of idle as any other
> > interrupt.
> 
> They use a separate hack for that, pretending that the timer is
> stopped during idle (it isn't), and setup a broadcast timer when
> entering idle. That timer uses an interrupt that can wake-up the
> target CPU, and all is well in the world. Sort of.
> 
> Of course, this breaks as PPIs are not only used by the timer, but
> also by a number of other HW bits (PMU, GIC, guest and hypervisor
> timers), and they don't have corresponding hacks to back them up.

Eew.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-10 12:13 [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ Abel Vesa
                   ` (2 preceding siblings ...)
  2019-06-10 13:19 ` [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ Mark Rutland
@ 2019-06-23 11:47 ` Martin Kepplinger
  2019-06-28  8:54   ` Abel Vesa
  2019-10-30  6:11   ` Martin Kepplinger
  3 siblings, 2 replies; 38+ messages in thread
From: Martin Kepplinger @ 2019-06-23 11:47 UTC (permalink / raw)
  To: Abel Vesa, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, Thomas Gleixner,
	Marc Zyngier, Lucas Stach, Bai Ping, Lorenzo Pieralisi,
	Leonard Crestez
  Cc: devicetree, linux-kernel, NXP Linux Team, linux-arm-kernel, Carlo Caione

On 10.06.19 14:13, Abel Vesa wrote:
> This is another alternative for the RFC:
> https://lkml.org/lkml/2019/3/27/545
> 
> This new workaround proposal is a little bit more hacky but more contained
> since everything is done within the irq-imx-gpcv2 driver.
> 
> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
> handler and registers instead a wrapper which calls in the 'hijacked' 
> handler, after that calling into EL3 which will take care of the actual
> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
> 
> I also have the patches ready for TF-A but I'll hold on to them until I see if
> this has a chance of getting in.

Let's leave out of the picture for now, how generally applicable and
mergable your changes are. I'd like to reproduce what you do and test
cpuidle on imx8mq:

When applying your changes here and the corresponding ATF changes (
https://github.com/abelvesa/arm-trusted-firmware/tree/imx8mq-err11171 if
I got that right) I don't yet see any difference in the SoC heating up
under zero load. __cpu_do_idle() is called about every 1ms (without your
changes, that was even more often but I'm not yet sure if that means
anything).

What I also see is that I get about 10x more "arch_timer" (int.3, GICv3)
interrupts than without your changes.

What am I doing wrong? I'd be happy to test, again, regardless of how
acceptable the workaround is in the end.

thanks,

                                  martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-23 11:47 ` Martin Kepplinger
@ 2019-06-28  8:54   ` Abel Vesa
  2019-07-02  6:47     ` Martin Kepplinger
  2019-10-30  6:11   ` Martin Kepplinger
  1 sibling, 1 reply; 38+ messages in thread
From: Abel Vesa @ 2019-06-28  8:54 UTC (permalink / raw)
  To: Martin Kepplinger
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Fabio Estevam, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Thomas Gleixner, Leonard Crestez, Shawn Guo,
	linux-arm-kernel, Lucas Stach

On 19-06-23 13:47:26, Martin Kepplinger wrote:
> On 10.06.19 14:13, Abel Vesa wrote:
> > This is another alternative for the RFC:
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C6c9d12c1017745750e3908d6f7d0935a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636968872531886931&amp;sdata=DAN3TVPD%2FaQzseYUYAjsnfQM6odM1x8qzsVVslFXAnY%3D&amp;reserved=0
> > 
> > This new workaround proposal is a little bit more hacky but more contained
> > since everything is done within the irq-imx-gpcv2 driver.
> > 
> > Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
> > handler and registers instead a wrapper which calls in the 'hijacked' 
> > handler, after that calling into EL3 which will take care of the actual
> > wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
> > 
> > I also have the patches ready for TF-A but I'll hold on to them until I see if
> > this has a chance of getting in.
> 
> Let's leave out of the picture for now, how generally applicable and
> mergable your changes are. I'd like to reproduce what you do and test
> cpuidle on imx8mq:
> 
> When applying your changes here and the corresponding ATF changes (
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fabelvesa%2Farm-trusted-firmware%2Ftree%2Fimx8mq-err11171&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C6c9d12c1017745750e3908d6f7d0935a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636968872531886931&amp;sdata=nB%2FYGkuRrJYwoBJ1afTjIhoadn9Pn3c2QqRFnShWS0c%3D&amp;reserved=0 if
> I got that right) I don't yet see any difference in the SoC heating up
> under zero load. __cpu_do_idle() is called about every 1ms (without your
> changes, that was even more often but I'm not yet sure if that means
> anything).

You will most probably not see any change in the SoC temp since the cpuidle
only touches the A53s. There are way many more IPs in the SoC that could
heat it up. If you want some real numbers you'll have to measure the power
consumtion on VDD_ARM rail. If you don't want to go through that much trouble
you can use the idlestat tool to measure the times each A53 speends in cpu-sleep
state.

> 
> What I also see is that I get about 10x more "arch_timer" (int.3, GICv3)
> interrupts than without your changes.
> 
> What am I doing wrong? I'd be happy to test, again, regardless of how
> acceptable the workaround is in the end.
> 
> thanks,
> 
>                                   martin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-28  8:54   ` Abel Vesa
@ 2019-07-02  6:47     ` Martin Kepplinger
  2019-07-02 11:33       ` Abel Vesa
  0 siblings, 1 reply; 38+ messages in thread
From: Martin Kepplinger @ 2019-07-02  6:47 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Shawn Guo, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Thomas Gleixner, Leonard Crestez, Fabio Estevam,
	linux-arm-kernel, Lucas Stach

On 28.06.19 10:54, Abel Vesa wrote:
> On 19-06-23 13:47:26, Martin Kepplinger wrote:
>> On 10.06.19 14:13, Abel Vesa wrote:
>>> This is another alternative for the RFC:
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C6c9d12c1017745750e3908d6f7d0935a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636968872531886931&amp;sdata=DAN3TVPD%2FaQzseYUYAjsnfQM6odM1x8qzsVVslFXAnY%3D&amp;reserved=0
>>>
>>> This new workaround proposal is a little bit more hacky but more contained
>>> since everything is done within the irq-imx-gpcv2 driver.
>>>
>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>> handler and registers instead a wrapper which calls in the 'hijacked' 
>>> handler, after that calling into EL3 which will take care of the actual
>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>
>>> I also have the patches ready for TF-A but I'll hold on to them until I see if
>>> this has a chance of getting in.
>>
>> Let's leave out of the picture for now, how generally applicable and
>> mergable your changes are. I'd like to reproduce what you do and test
>> cpuidle on imx8mq:
>>
>> When applying your changes here and the corresponding ATF changes (
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fabelvesa%2Farm-trusted-firmware%2Ftree%2Fimx8mq-err11171&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C6c9d12c1017745750e3908d6f7d0935a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636968872531886931&amp;sdata=nB%2FYGkuRrJYwoBJ1afTjIhoadn9Pn3c2QqRFnShWS0c%3D&amp;reserved=0 if
>> I got that right) I don't yet see any difference in the SoC heating up
>> under zero load. __cpu_do_idle() is called about every 1ms (without your
>> changes, that was even more often but I'm not yet sure if that means
>> anything).
> 
> You will most probably not see any change in the SoC temp since the cpuidle
> only touches the A53s. There are way many more IPs in the SoC that could
> heat it up. If you want some real numbers you'll have to measure the power
> consumtion on VDD_ARM rail. If you don't want to go through that much trouble
> you can use the idlestat tool to measure the times each A53 speends in cpu-sleep
> state.
> 
>>
>> What I also see is that I get about 10x more "arch_timer" (int.3, GICv3)
>> interrupts than without your changes.


thanks for getting back at me here. This is run on the imx8mq
librem5-devkit with your wakeup-workaround applied. Typical measurements
under zero load look like this:

sudo idlestat --trace -f /tmp/mytrace -t 10 -p -c -w
Log is 10.000395 secs long with 31194 events
------------------------------------------------------------------------
| C-state  |  min   |  max    |  avg    |  total | hits | over | under |
------------------------------------------------------------------------
| clusterA                                                             |
------------------------------------------------------------------------
|     WFI |   14us |  3.99ms |  3.90ms |   9.93s | 2543 |    0 |     0 |
------------------------------------------------------------------------
|          cpu0                                                        |
------------------------------------------------------------------------
|     WFI |   14us |  3.99ms |  3.89ms |   9.96s | 2561 |    0 |     0 |
------------------------------------------------------------------------
...


with IRQs coming in:

-------------------------------------------------------
| IRQ |       Name      |  Count  |  early  |  late   |
-------------------------------------------------------
|             cpu0                                    |
-------------------------------------------------------
| IPI | Reschedulin     |      11 |       0 |       0 |
| 3   | arch_timer      |    2505 |       0 |       0 |
| 41  | 30be0000.ethern |      11 |       0 |       0 |
| 36  | mmc0            |       6 |       0 |       0 |
| 33  | 30a20000.i2c    |      12 |       0 |       0 |
| 40  | 30be0000.ethern |       1 |       0 |       0 |
| 43  | 38000000.gpu    |       2 |       0 |       0 |
| 208 | dcss_drm        |      12 |       0 |       0 |
| 207 | dcss_ctxld      |       2 |       0 |       0 |
-------------------------------------------------------
|             cpu1                                    |
-------------------------------------------------------
| IPI | Reschedulin     |      13 |       0 |       0 |
| 3   | arch_timer      |    2500 |       0 |       0 |
| IPI | Functio         |       1 |       0 |       0 |
...


So we seem to spend most of the time in C1/WFI. As mentioned,
"arch_timer" wakes up the cpu often.

Why is that? Do these measurements look like what you would expect them
to be?

(I'm not sure how much sense it makes to come up with something to
compare these to)

thanks a lot,

                                martin



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-07-02  6:47     ` Martin Kepplinger
@ 2019-07-02 11:33       ` Abel Vesa
  2019-07-08  7:54         ` Martin Kepplinger
  0 siblings, 1 reply; 38+ messages in thread
From: Abel Vesa @ 2019-07-02 11:33 UTC (permalink / raw)
  To: Martin Kepplinger
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Shawn Guo, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Thomas Gleixner, Leonard Crestez, Fabio Estevam,
	linux-arm-kernel, Lucas Stach

On 19-07-02 08:47:19, Martin Kepplinger wrote:
> On 28.06.19 10:54, Abel Vesa wrote:
> > On 19-06-23 13:47:26, Martin Kepplinger wrote:
> >> On 10.06.19 14:13, Abel Vesa wrote:
> >>> This is another alternative for the RFC:
> >>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Ccfc582f9977d479b7dda08d6feb9258a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636976468485275045&amp;sdata=L%2Byn29%2FBS3KMjm9eCPBTZBTl30PmZywSjIj11bMQw5c%3D&amp;reserved=0
> >>>
> >>> This new workaround proposal is a little bit more hacky but more contained
> >>> since everything is done within the irq-imx-gpcv2 driver.
> >>>
> >>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
> >>> handler and registers instead a wrapper which calls in the 'hijacked' 
> >>> handler, after that calling into EL3 which will take care of the actual
> >>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
> >>>
> >>> I also have the patches ready for TF-A but I'll hold on to them until I see if
> >>> this has a chance of getting in.
> >>
> >> Let's leave out of the picture for now, how generally applicable and
> >> mergable your changes are. I'd like to reproduce what you do and test
> >> cpuidle on imx8mq:
> >>
> >> When applying your changes here and the corresponding ATF changes (
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fabelvesa%2Farm-trusted-firmware%2Ftree%2Fimx8mq-err11171&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Ccfc582f9977d479b7dda08d6feb9258a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636976468485275045&amp;sdata=VT3duSl70DNxcY8Ev4FFrHlWoOjkcckeM8BgxrSkr8A%3D&amp;reserved=0 if
> >> I got that right) I don't yet see any difference in the SoC heating up
> >> under zero load. __cpu_do_idle() is called about every 1ms (without your
> >> changes, that was even more often but I'm not yet sure if that means
> >> anything).
> > 
> > You will most probably not see any change in the SoC temp since the cpuidle
> > only touches the A53s. There are way many more IPs in the SoC that could
> > heat it up. If you want some real numbers you'll have to measure the power
> > consumtion on VDD_ARM rail. If you don't want to go through that much trouble
> > you can use the idlestat tool to measure the times each A53 speends in cpu-sleep
> > state.
> > 
> >>
> >> What I also see is that I get about 10x more "arch_timer" (int.3, GICv3)
> >> interrupts than without your changes.
> 
> 
> thanks for getting back at me here. This is run on the imx8mq
> librem5-devkit with your wakeup-workaround applied. Typical measurements
> under zero load look like this:
> 
> sudo idlestat --trace -f /tmp/mytrace -t 10 -p -c -w
> Log is 10.000395 secs long with 31194 events
> ------------------------------------------------------------------------
> | C-state  |  min   |  max    |  avg    |  total | hits | over | under |
> ------------------------------------------------------------------------
> | clusterA                                                             |
> ------------------------------------------------------------------------
> |     WFI |   14us |  3.99ms |  3.90ms |   9.93s | 2543 |    0 |     0 |
> ------------------------------------------------------------------------
> |          cpu0                                                        |
> ------------------------------------------------------------------------
> |     WFI |   14us |  3.99ms |  3.89ms |   9.96s | 2561 |    0 |     0 |
> ------------------------------------------------------------------------
> ...
> 

I don't see the cpu-sleep state at all in your idlestat log. Maybe the cpuidle
isn't enabled. Or probably the workaround itself is not applied. You'll have
to look into that.

Here is how it looks like with the workaround enabled:

Log is 10.001685 secs long with 1175 events
--------------------------------------------------------------------------------
| C-state  |   min    |   max    |   avg    |   total  | hits  |  over | under |
--------------------------------------------------------------------------------
| clusterA                                                                     |
--------------------------------------------------------------------------------
|      WFI |      2us |  50.04ms |  29.63ms |    9.99s |   337 |     0 |     0 |
--------------------------------------------------------------------------------
|             cpu0                                                             |
--------------------------------------------------------------------------------
|      WFI |     11us |  50.04ms |  40.44ms |    9.62s |   238 |     0 |   219 |
| cpu-sleep |    537us |  50.58ms |  14.11ms | 366.94ms |    26 |     7 |     0 |
--------------------------------------------------------------------------------
|             cpu1                                                             |
--------------------------------------------------------------------------------
|      WFI |     11us | 539.04ms |  93.20ms |    5.78s |    62 |     0 |    38 |
| cpu-sleep |    536us | 607.90ms | 183.38ms |    4.22s |    23 |    12 |     0 |
--------------------------------------------------------------------------------
|             cpu2                                                             |
--------------------------------------------------------------------------------
|      WFI |     41us | 265.99ms |  17.51ms | 332.66ms |    19 |     0 |    11 |
| cpu-sleep |    568us |    6.56s |    1.38s |    9.67s |     7 |     2 |     0 |
--------------------------------------------------------------------------------
|             cpu3                                                             |
--------------------------------------------------------------------------------
|      WFI |   7.94ms | 881.50ms | 367.81ms |    1.10s |     3 |     0 |     3 |
| cpu-sleep |    549us |    2.02s | 808.72ms |    8.90s |    11 |     1 |     0 |
--------------------------------------------------------------------------------

You can see that the cpu2 was once for 6.56 seconds (out of 10s) in cpu-sleep.

> 
> with IRQs coming in:
> 
> -------------------------------------------------------
> | IRQ |       Name      |  Count  |  early  |  late   |
> -------------------------------------------------------
> |             cpu0                                    |
> -------------------------------------------------------
> | IPI | Reschedulin     |      11 |       0 |       0 |
> | 3   | arch_timer      |    2505 |       0 |       0 |
> | 41  | 30be0000.ethern |      11 |       0 |       0 |
> | 36  | mmc0            |       6 |       0 |       0 |
> | 33  | 30a20000.i2c    |      12 |       0 |       0 |
> | 40  | 30be0000.ethern |       1 |       0 |       0 |
> | 43  | 38000000.gpu    |       2 |       0 |       0 |
> | 208 | dcss_drm        |      12 |       0 |       0 |
> | 207 | dcss_ctxld      |       2 |       0 |       0 |
> -------------------------------------------------------
> |             cpu1                                    |
> -------------------------------------------------------
> | IPI | Reschedulin     |      13 |       0 |       0 |
> | 3   | arch_timer      |    2500 |       0 |       0 |
> | IPI | Functio         |       1 |       0 |       0 |
> ...
> 
> 
> So we seem to spend most of the time in C1/WFI. As mentioned,
> "arch_timer" wakes up the cpu often.
> 
> Why is that? Do these measurements look like what you would expect them
> to be?
> 
> (I'm not sure how much sense it makes to come up with something to
> compare these to)
> 
> thanks a lot,
> 
>                                 martin
> 
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-07-02 11:33       ` Abel Vesa
@ 2019-07-08  7:54         ` Martin Kepplinger
  2019-07-08 12:20           ` Martin Kepplinger
  0 siblings, 1 reply; 38+ messages in thread
From: Martin Kepplinger @ 2019-07-08  7:54 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Shawn Guo, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Thomas Gleixner, Leonard Crestez, Fabio Estevam,
	linux-arm-kernel, Lucas Stach

On 02.07.19 13:33, Abel Vesa wrote:
> On 19-07-02 08:47:19, Martin Kepplinger wrote:
>> On 28.06.19 10:54, Abel Vesa wrote:
>>> On 19-06-23 13:47:26, Martin Kepplinger wrote:
>>>> On 10.06.19 14:13, Abel Vesa wrote:
>>>>> This is another alternative for the RFC:
>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Ccfc582f9977d479b7dda08d6feb9258a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636976468485275045&amp;sdata=L%2Byn29%2FBS3KMjm9eCPBTZBTl30PmZywSjIj11bMQw5c%3D&amp;reserved=0
>>>>>
>>>>> This new workaround proposal is a little bit more hacky but more contained
>>>>> since everything is done within the irq-imx-gpcv2 driver.
>>>>>
>>>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>>>> handler and registers instead a wrapper which calls in the 'hijacked' 
>>>>> handler, after that calling into EL3 which will take care of the actual
>>>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>>>
>>>>> I also have the patches ready for TF-A but I'll hold on to them until I see if
>>>>> this has a chance of getting in.
>>>>
>>>> Let's leave out of the picture for now, how generally applicable and
>>>> mergable your changes are. I'd like to reproduce what you do and test
>>>> cpuidle on imx8mq:
>>>>
>>>> When applying your changes here and the corresponding ATF changes (
>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fabelvesa%2Farm-trusted-firmware%2Ftree%2Fimx8mq-err11171&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Ccfc582f9977d479b7dda08d6feb9258a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636976468485275045&amp;sdata=VT3duSl70DNxcY8Ev4FFrHlWoOjkcckeM8BgxrSkr8A%3D&amp;reserved=0 if
>>>> I got that right) I don't yet see any difference in the SoC heating up
>>>> under zero load. __cpu_do_idle() is called about every 1ms (without your
>>>> changes, that was even more often but I'm not yet sure if that means
>>>> anything).
>>>
>>> You will most probably not see any change in the SoC temp since the cpuidle
>>> only touches the A53s. There are way many more IPs in the SoC that could
>>> heat it up. If you want some real numbers you'll have to measure the power
>>> consumtion on VDD_ARM rail. If you don't want to go through that much trouble
>>> you can use the idlestat tool to measure the times each A53 speends in cpu-sleep
>>> state.
>>>
>>>>
>>>> What I also see is that I get about 10x more "arch_timer" (int.3, GICv3)
>>>> interrupts than without your changes.
>>
>>
>> thanks for getting back at me here. This is run on the imx8mq
>> librem5-devkit with your wakeup-workaround applied. Typical measurements
>> under zero load look like this:
>>
>> sudo idlestat --trace -f /tmp/mytrace -t 10 -p -c -w
>> Log is 10.000395 secs long with 31194 events
>> ------------------------------------------------------------------------
>> | C-state  |  min   |  max    |  avg    |  total | hits | over | under |
>> ------------------------------------------------------------------------
>> | clusterA                                                             |
>> ------------------------------------------------------------------------
>> |     WFI |   14us |  3.99ms |  3.90ms |   9.93s | 2543 |    0 |     0 |
>> ------------------------------------------------------------------------
>> |          cpu0                                                        |
>> ------------------------------------------------------------------------
>> |     WFI |   14us |  3.99ms |  3.89ms |   9.96s | 2561 |    0 |     0 |
>> ------------------------------------------------------------------------
>> ...
>>
> 
> I don't see the cpu-sleep state at all in your idlestat log. Maybe the cpuidle
> isn't enabled. Or probably the workaround itself is not applied. You'll have
> to look into that.
> 
> Here is how it looks like with the workaround enabled:
> 
> Log is 10.001685 secs long with 1175 events
> --------------------------------------------------------------------------------
> | C-state  |   min    |   max    |   avg    |   total  | hits  |  over | under |
> --------------------------------------------------------------------------------
> | clusterA                                                                     |
> --------------------------------------------------------------------------------
> |      WFI |      2us |  50.04ms |  29.63ms |    9.99s |   337 |     0 |     0 |
> --------------------------------------------------------------------------------
> |             cpu0                                                             |
> --------------------------------------------------------------------------------
> |      WFI |     11us |  50.04ms |  40.44ms |    9.62s |   238 |     0 |   219 |
> | cpu-sleep |    537us |  50.58ms |  14.11ms | 366.94ms |    26 |     7 |     0 |
> --------------------------------------------------------------------------------
> |             cpu1                                                             |
> --------------------------------------------------------------------------------
> |      WFI |     11us | 539.04ms |  93.20ms |    5.78s |    62 |     0 |    38 |
> | cpu-sleep |    536us | 607.90ms | 183.38ms |    4.22s |    23 |    12 |     0 |
> --------------------------------------------------------------------------------
> |             cpu2                                                             |
> --------------------------------------------------------------------------------
> |      WFI |     41us | 265.99ms |  17.51ms | 332.66ms |    19 |     0 |    11 |
> | cpu-sleep |    568us |    6.56s |    1.38s |    9.67s |     7 |     2 |     0 |
> --------------------------------------------------------------------------------
> |             cpu3                                                             |
> --------------------------------------------------------------------------------
> |      WFI |   7.94ms | 881.50ms | 367.81ms |    1.10s |     3 |     0 |     3 |
> | cpu-sleep |    549us |    2.02s | 808.72ms |    8.90s |    11 |     1 |     0 |
> --------------------------------------------------------------------------------
> 
> You can see that the cpu2 was once for 6.56 seconds (out of 10s) in cpu-sleep.
> 

So I run this ATF tree
https://github.com/abelvesa/arm-trusted-firmware/tree/imx8mq-err11171
and, on top of "v5.2-rc7" I have your commits
("irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171") and
("arm64: dts: imx8mq: Add idle states and gpcv2 wake_request broken
property") applied.

Then simply enabled CONFIG_ARM_CPUIDLE.

(I also use the "imx-cpufreq-dt" driver, but this should be unrelated here).

I do see the possible cpuidle states:
/sys/devices/system/cpu/cpu0/cpuidle$ cat state*/name

WFI

cpu-sleep

but idlestat doesn't see it or it is (thus) never used. Do you know a
needed change I might be missing?

thanks again,
                                martin



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-07-08  7:54         ` Martin Kepplinger
@ 2019-07-08 12:20           ` Martin Kepplinger
  0 siblings, 0 replies; 38+ messages in thread
From: Martin Kepplinger @ 2019-07-08 12:20 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Fabio Estevam, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Thomas Gleixner, Leonard Crestez, Shawn Guo,
	linux-arm-kernel, Lucas Stach

On 08.07.19 09:54, Martin Kepplinger wrote:
> On 02.07.19 13:33, Abel Vesa wrote:
>> On 19-07-02 08:47:19, Martin Kepplinger wrote:
>>> On 28.06.19 10:54, Abel Vesa wrote:
>>>> On 19-06-23 13:47:26, Martin Kepplinger wrote:
>>>>> On 10.06.19 14:13, Abel Vesa wrote:
>>>>>> This is another alternative for the RFC:
>>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Ccfc582f9977d479b7dda08d6feb9258a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636976468485275045&amp;sdata=L%2Byn29%2FBS3KMjm9eCPBTZBTl30PmZywSjIj11bMQw5c%3D&amp;reserved=0
>>>>>>
>>>>>> This new workaround proposal is a little bit more hacky but more contained
>>>>>> since everything is done within the irq-imx-gpcv2 driver.
>>>>>>
>>>>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>>>>> handler and registers instead a wrapper which calls in the 'hijacked' 
>>>>>> handler, after that calling into EL3 which will take care of the actual
>>>>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>>>>
>>>>>> I also have the patches ready for TF-A but I'll hold on to them until I see if
>>>>>> this has a chance of getting in.
>>>>>
>>>>> Let's leave out of the picture for now, how generally applicable and
>>>>> mergable your changes are. I'd like to reproduce what you do and test
>>>>> cpuidle on imx8mq:
>>>>>
>>>>> When applying your changes here and the corresponding ATF changes (
>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fabelvesa%2Farm-trusted-firmware%2Ftree%2Fimx8mq-err11171&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Ccfc582f9977d479b7dda08d6feb9258a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636976468485275045&amp;sdata=VT3duSl70DNxcY8Ev4FFrHlWoOjkcckeM8BgxrSkr8A%3D&amp;reserved=0 if
>>>>> I got that right) I don't yet see any difference in the SoC heating up
>>>>> under zero load. __cpu_do_idle() is called about every 1ms (without your
>>>>> changes, that was even more often but I'm not yet sure if that means
>>>>> anything).
>>>>
>>>> You will most probably not see any change in the SoC temp since the cpuidle
>>>> only touches the A53s. There are way many more IPs in the SoC that could
>>>> heat it up. If you want some real numbers you'll have to measure the power
>>>> consumtion on VDD_ARM rail. If you don't want to go through that much trouble
>>>> you can use the idlestat tool to measure the times each A53 speends in cpu-sleep
>>>> state.
>>>>
>>>>>
>>>>> What I also see is that I get about 10x more "arch_timer" (int.3, GICv3)
>>>>> interrupts than without your changes.
>>>
>>>
>>> thanks for getting back at me here. This is run on the imx8mq
>>> librem5-devkit with your wakeup-workaround applied. Typical measurements
>>> under zero load look like this:
>>>
>>> sudo idlestat --trace -f /tmp/mytrace -t 10 -p -c -w
>>> Log is 10.000395 secs long with 31194 events
>>> ------------------------------------------------------------------------
>>> | C-state  |  min   |  max    |  avg    |  total | hits | over | under |
>>> ------------------------------------------------------------------------
>>> | clusterA                                                             |
>>> ------------------------------------------------------------------------
>>> |     WFI |   14us |  3.99ms |  3.90ms |   9.93s | 2543 |    0 |     0 |
>>> ------------------------------------------------------------------------
>>> |          cpu0                                                        |
>>> ------------------------------------------------------------------------
>>> |     WFI |   14us |  3.99ms |  3.89ms |   9.96s | 2561 |    0 |     0 |
>>> ------------------------------------------------------------------------
>>> ...
>>>
>>
>> I don't see the cpu-sleep state at all in your idlestat log. Maybe the cpuidle
>> isn't enabled. Or probably the workaround itself is not applied. You'll have
>> to look into that.
>>
>> Here is how it looks like with the workaround enabled:
>>
>> Log is 10.001685 secs long with 1175 events
>> --------------------------------------------------------------------------------
>> | C-state  |   min    |   max    |   avg    |   total  | hits  |  over | under |
>> --------------------------------------------------------------------------------
>> | clusterA                                                                     |
>> --------------------------------------------------------------------------------
>> |      WFI |      2us |  50.04ms |  29.63ms |    9.99s |   337 |     0 |     0 |
>> --------------------------------------------------------------------------------
>> |             cpu0                                                             |
>> --------------------------------------------------------------------------------
>> |      WFI |     11us |  50.04ms |  40.44ms |    9.62s |   238 |     0 |   219 |
>> | cpu-sleep |    537us |  50.58ms |  14.11ms | 366.94ms |    26 |     7 |     0 |
>> --------------------------------------------------------------------------------
>> |             cpu1                                                             |
>> --------------------------------------------------------------------------------
>> |      WFI |     11us | 539.04ms |  93.20ms |    5.78s |    62 |     0 |    38 |
>> | cpu-sleep |    536us | 607.90ms | 183.38ms |    4.22s |    23 |    12 |     0 |
>> --------------------------------------------------------------------------------
>> |             cpu2                                                             |
>> --------------------------------------------------------------------------------
>> |      WFI |     41us | 265.99ms |  17.51ms | 332.66ms |    19 |     0 |    11 |
>> | cpu-sleep |    568us |    6.56s |    1.38s |    9.67s |     7 |     2 |     0 |
>> --------------------------------------------------------------------------------
>> |             cpu3                                                             |
>> --------------------------------------------------------------------------------
>> |      WFI |   7.94ms | 881.50ms | 367.81ms |    1.10s |     3 |     0 |     3 |
>> | cpu-sleep |    549us |    2.02s | 808.72ms |    8.90s |    11 |     1 |     0 |
>> --------------------------------------------------------------------------------
>>
>> You can see that the cpu2 was once for 6.56 seconds (out of 10s) in cpu-sleep.
>>
> 
> So I run this ATF tree
> https://github.com/abelvesa/arm-trusted-firmware/tree/imx8mq-err11171
> and, on top of "v5.2-rc7" I have your commits
> ("irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171") and
> ("arm64: dts: imx8mq: Add idle states and gpcv2 wake_request broken
> property") applied.
> 
> Then simply enabled CONFIG_ARM_CPUIDLE.
> 
> (I also use the "imx-cpufreq-dt" driver, but this should be unrelated here).
> 
> I do see the possible cpuidle states:
> /sys/devices/system/cpu/cpu0/cpuidle$ cat state*/name
> 
> WFI
> 
> cpu-sleep
> 
> but idlestat doesn't see it or it is (thus) never used. Do you know a
> needed change I might be missing?
> 

looks like I mess things up myself here and somehow prevent
cpu_pm_enter() from being called...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-06-23 11:47 ` Martin Kepplinger
  2019-06-28  8:54   ` Abel Vesa
@ 2019-10-30  6:11   ` Martin Kepplinger
  2019-10-30  7:33     ` Martin Kepplinger
  2019-10-30  8:08     ` Abel Vesa
  1 sibling, 2 replies; 38+ messages in thread
From: Martin Kepplinger @ 2019-10-30  6:11 UTC (permalink / raw)
  To: Abel Vesa, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, Thomas Gleixner,
	Marc Zyngier, Lucas Stach, Bai Ping, Lorenzo Pieralisi,
	Leonard Crestez
  Cc: devicetree, linux-kernel, NXP Linux Team, linux-arm-kernel, Carlo Caione

On 23.06.19 13:47, Martin Kepplinger wrote:
> On 10.06.19 14:13, Abel Vesa wrote:
>> This is another alternative for the RFC:
>> https://lkml.org/lkml/2019/3/27/545
>>
>> This new workaround proposal is a little bit more hacky but more contained
>> since everything is done within the irq-imx-gpcv2 driver.
>>
>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>> handler and registers instead a wrapper which calls in the 'hijacked' 
>> handler, after that calling into EL3 which will take care of the actual
>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>
>> I also have the patches ready for TF-A but I'll hold on to them until I see if
>> this has a chance of getting in.
> 

Hi Abel,

Running this workaround doesn't seem to work anymore on 5.4-rcX. Linux
doesn't boot, with ATF unchanged (includes your workaround changes). I
can try to add more details to this...

Have you tested this for 5.4? Could you update this workaround? Please
let me know if I missed any earlier update on this (having a cpu-sleep
idle state).

thanks!

                              martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-10-30  6:11   ` Martin Kepplinger
@ 2019-10-30  7:33     ` Martin Kepplinger
  2019-10-30  8:08     ` Abel Vesa
  1 sibling, 0 replies; 38+ messages in thread
From: Martin Kepplinger @ 2019-10-30  7:33 UTC (permalink / raw)
  To: Abel Vesa, Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, Thomas Gleixner,
	Marc Zyngier, Lucas Stach, Bai Ping, Lorenzo Pieralisi,
	Leonard Crestez
  Cc: devicetree, linux-kernel, NXP Linux Team, linux-arm-kernel, Carlo Caione

On 30.10.19 07:11, Martin Kepplinger wrote:
> On 23.06.19 13:47, Martin Kepplinger wrote:
>> On 10.06.19 14:13, Abel Vesa wrote:
>>> This is another alternative for the RFC:
>>> https://lkml.org/lkml/2019/3/27/545
>>>
>>> This new workaround proposal is a little bit more hacky but more contained
>>> since everything is done within the irq-imx-gpcv2 driver.
>>>
>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>> handler and registers instead a wrapper which calls in the 'hijacked' 
>>> handler, after that calling into EL3 which will take care of the actual
>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>
>>> I also have the patches ready for TF-A but I'll hold on to them until I see if
>>> this has a chance of getting in.
>>
> 
> Hi Abel,
> 
> Running this workaround doesn't seem to work anymore on 5.4-rcX. Linux
> doesn't boot, with ATF unchanged (includes your workaround changes). I
> can try to add more details to this...
> 

When I run this change instead:
https://source.puri.sm/Librem5/linux-next/commit/e59807ae0e236512761b751abc84a9b129d7fcda
(again, with the same ATF) Linux 5.4 boots, but simply doesn't load a
cpuidle driver (cpu-sleep; and cpuidle in /sys/devices/system/cpu/cpu0).

Still, since this is starting with 5.4-rcX, my previous questions stay
the same :)

thanks,
                                      martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-10-30  6:11   ` Martin Kepplinger
  2019-10-30  7:33     ` Martin Kepplinger
@ 2019-10-30  8:08     ` Abel Vesa
  2019-10-30  8:14       ` Martin Kepplinger
  2019-11-04  8:49       ` Martin Kepplinger
  1 sibling, 2 replies; 38+ messages in thread
From: Abel Vesa @ 2019-10-30  8:08 UTC (permalink / raw)
  To: Martin Kepplinger
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Fabio Estevam, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Thomas Gleixner, Leonard Crestez, Shawn Guo,
	linux-arm-kernel, Lucas Stach

On 19-10-30 07:11:37, Martin Kepplinger wrote:
> On 23.06.19 13:47, Martin Kepplinger wrote:
> > On 10.06.19 14:13, Abel Vesa wrote:
> >> This is another alternative for the RFC:
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Cf5f8d8dd37974234fcb108d75d000944%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637080127051582184&amp;sdata=qZEo1fY1lkTjqZWuuQftYJ5euEsSxjEAqGILCY8ChnU%3D&amp;reserved=0
> >>
> >> This new workaround proposal is a little bit more hacky but more contained
> >> since everything is done within the irq-imx-gpcv2 driver.
> >>
> >> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
> >> handler and registers instead a wrapper which calls in the 'hijacked' 
> >> handler, after that calling into EL3 which will take care of the actual
> >> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
> >>
> >> I also have the patches ready for TF-A but I'll hold on to them until I see if
> >> this has a chance of getting in.
> > 
> 
> Hi Abel,
> 
> Running this workaround doesn't seem to work anymore on 5.4-rcX. Linux
> doesn't boot, with ATF unchanged (includes your workaround changes). I
> can try to add more details to this...
> 

This is happening because the system counter is now enabled on 8mq.
And since the irq-imx-gpcv2 is using as irq_set_affinity the 
irq_chip_set_affinity_parent. This is because the actual implementation
of the driver relies on GIC to set the right affinity. On a SoC
that has the wake_request signales linked to the power controller this
works fine. Since the system counter is actually the tick broadcast
device and the set affinity relies only on GIC, the cores can't be
woken up by the broadcast interrupt.

> Have you tested this for 5.4? Could you update this workaround? Please
> let me know if I missed any earlier update on this (having a cpu-sleep
> idle state).
> 

The solution is to implement the set affinity in the irq-imx-gpcv2 driver
which would allow the gpc to wake up the target core when the broadcast
irq arrives.

I have a patch for this. I just need to clean it up a little bit.
Unfortunately, it won't go upstream since everuone thinks the gic
should be the one to control the affinity. This obviously doesn't work
on 8mq.

Currently, I'm at ELCE in Lyon. Will get back at the office tomorrow
and sned you what I have.

> thanks!
> 
>                               martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-10-30  8:08     ` Abel Vesa
@ 2019-10-30  8:14       ` Martin Kepplinger
  2019-11-04  8:49       ` Martin Kepplinger
  1 sibling, 0 replies; 38+ messages in thread
From: Martin Kepplinger @ 2019-10-30  8:14 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Fabio Estevam, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Thomas Gleixner, Leonard Crestez, Shawn Guo,
	linux-arm-kernel, Lucas Stach

On 30.10.19 09:08, Abel Vesa wrote:
> On 19-10-30 07:11:37, Martin Kepplinger wrote:
>> On 23.06.19 13:47, Martin Kepplinger wrote:
>>> On 10.06.19 14:13, Abel Vesa wrote:
>>>> This is another alternative for the RFC:
>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Cf5f8d8dd37974234fcb108d75d000944%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637080127051582184&amp;sdata=qZEo1fY1lkTjqZWuuQftYJ5euEsSxjEAqGILCY8ChnU%3D&amp;reserved=0
>>>>
>>>> This new workaround proposal is a little bit more hacky but more contained
>>>> since everything is done within the irq-imx-gpcv2 driver.
>>>>
>>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>>> handler and registers instead a wrapper which calls in the 'hijacked' 
>>>> handler, after that calling into EL3 which will take care of the actual
>>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>>
>>>> I also have the patches ready for TF-A but I'll hold on to them until I see if
>>>> this has a chance of getting in.
>>>
>>
>> Hi Abel,
>>
>> Running this workaround doesn't seem to work anymore on 5.4-rcX. Linux
>> doesn't boot, with ATF unchanged (includes your workaround changes). I
>> can try to add more details to this...
>>
> 
> This is happening because the system counter is now enabled on 8mq.
> And since the irq-imx-gpcv2 is using as irq_set_affinity the 
> irq_chip_set_affinity_parent. This is because the actual implementation
> of the driver relies on GIC to set the right affinity. On a SoC
> that has the wake_request signales linked to the power controller this
> works fine. Since the system counter is actually the tick broadcast
> device and the set affinity relies only on GIC, the cores can't be
> woken up by the broadcast interrupt.
> 
>> Have you tested this for 5.4? Could you update this workaround? Please
>> let me know if I missed any earlier update on this (having a cpu-sleep
>> idle state).
>>
> 
> The solution is to implement the set affinity in the irq-imx-gpcv2 driver
> which would allow the gpc to wake up the target core when the broadcast
> irq arrives.
> 
> I have a patch for this. I just need to clean it up a little bit.
> Unfortunately, it won't go upstream since everuone thinks the gic
> should be the one to control the affinity. This obviously doesn't work
> on 8mq.

I see and that's fine for the moment.

> 
> Currently, I'm at ELCE in Lyon. Will get back at the office tomorrow
> and sned you what I have.
> 

Thanks for answering so quickly. I'll test as soon as you can send that
update.

thanks for this work,

                                martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-10-30  8:08     ` Abel Vesa
  2019-10-30  8:14       ` Martin Kepplinger
@ 2019-11-04  8:49       ` Martin Kepplinger
  2019-11-04 10:35         ` Abel Vesa
  1 sibling, 1 reply; 38+ messages in thread
From: Martin Kepplinger @ 2019-11-04  8:49 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Fabio Estevam, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Thomas Gleixner, Leonard Crestez, Shawn Guo,
	linux-arm-kernel, Lucas Stach

On 30.10.19 09:08, Abel Vesa wrote:
> On 19-10-30 07:11:37, Martin Kepplinger wrote:
>> On 23.06.19 13:47, Martin Kepplinger wrote:
>>> On 10.06.19 14:13, Abel Vesa wrote:
>>>> This is another alternative for the RFC:
>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7Cf5f8d8dd37974234fcb108d75d000944%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637080127051582184&amp;sdata=qZEo1fY1lkTjqZWuuQftYJ5euEsSxjEAqGILCY8ChnU%3D&amp;reserved=0
>>>>
>>>> This new workaround proposal is a little bit more hacky but more contained
>>>> since everything is done within the irq-imx-gpcv2 driver.
>>>>
>>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>>> handler and registers instead a wrapper which calls in the 'hijacked' 
>>>> handler, after that calling into EL3 which will take care of the actual
>>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>>
>>>> I also have the patches ready for TF-A but I'll hold on to them until I see if
>>>> this has a chance of getting in.
>>>
>>
>> Hi Abel,
>>
>> Running this workaround doesn't seem to work anymore on 5.4-rcX. Linux
>> doesn't boot, with ATF unchanged (includes your workaround changes). I
>> can try to add more details to this...
>>
> 
> This is happening because the system counter is now enabled on 8mq.
> And since the irq-imx-gpcv2 is using as irq_set_affinity the 
> irq_chip_set_affinity_parent. This is because the actual implementation
> of the driver relies on GIC to set the right affinity. On a SoC
> that has the wake_request signales linked to the power controller this
> works fine. Since the system counter is actually the tick broadcast
> device and the set affinity relies only on GIC, the cores can't be
> woken up by the broadcast interrupt.
> 
>> Have you tested this for 5.4? Could you update this workaround? Please
>> let me know if I missed any earlier update on this (having a cpu-sleep
>> idle state).
>>
> 
> The solution is to implement the set affinity in the irq-imx-gpcv2 driver
> which would allow the gpc to wake up the target core when the broadcast
> irq arrives.
> 
> I have a patch for this. I just need to clean it up a little bit.
> Unfortunately, it won't go upstream since everuone thinks the gic
> should be the one to control the affinity. This obviously doesn't work
> on 8mq.
> 
> Currently, I'm at ELCE in Lyon. Will get back at the office tomorrow
> and sned you what I have.
> 

Hi Abel,

Do you have any news on said patch for testing? That'd be great for my
plannings.

thanks a lot,

                                 martin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-11-04  8:49       ` Martin Kepplinger
@ 2019-11-04 10:35         ` Abel Vesa
  2019-11-06 11:59           ` Martin Kepplinger
  0 siblings, 1 reply; 38+ messages in thread
From: Abel Vesa @ 2019-11-04 10:35 UTC (permalink / raw)
  To: Martin Kepplinger
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Fabio Estevam, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Thomas Gleixner, Leonard Crestez, Shawn Guo,
	linux-arm-kernel, Lucas Stach

On 19-11-04 09:49:18, Martin Kepplinger wrote:
> On 30.10.19 09:08, Abel Vesa wrote:
> > On 19-10-30 07:11:37, Martin Kepplinger wrote:
> >> On 23.06.19 13:47, Martin Kepplinger wrote:
> >>> On 10.06.19 14:13, Abel Vesa wrote:
> >>>> This is another alternative for the RFC:
> >>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C50f2d9cf92ae4c41db1308d76103e468%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637084541652623937&amp;sdata=eY8TR3bpvYBWGZ7Xd58%2BK8Ig0qJ3ZqTWO8fNS5X0tj8%3D&amp;reserved=0
> >>>>
> >>>> This new workaround proposal is a little bit more hacky but more contained
> >>>> since everything is done within the irq-imx-gpcv2 driver.
> >>>>
> >>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
> >>>> handler and registers instead a wrapper which calls in the 'hijacked' 
> >>>> handler, after that calling into EL3 which will take care of the actual
> >>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
> >>>>
> >>>> I also have the patches ready for TF-A but I'll hold on to them until I see if
> >>>> this has a chance of getting in.
> >>>
> >>
> >> Hi Abel,
> >>
> >> Running this workaround doesn't seem to work anymore on 5.4-rcX. Linux
> >> doesn't boot, with ATF unchanged (includes your workaround changes). I
> >> can try to add more details to this...
> >>
> > 
> > This is happening because the system counter is now enabled on 8mq.
> > And since the irq-imx-gpcv2 is using as irq_set_affinity the 
> > irq_chip_set_affinity_parent. This is because the actual implementation
> > of the driver relies on GIC to set the right affinity. On a SoC
> > that has the wake_request signales linked to the power controller this
> > works fine. Since the system counter is actually the tick broadcast
> > device and the set affinity relies only on GIC, the cores can't be
> > woken up by the broadcast interrupt.
> > 
> >> Have you tested this for 5.4? Could you update this workaround? Please
> >> let me know if I missed any earlier update on this (having a cpu-sleep
> >> idle state).
> >>
> > 
> > The solution is to implement the set affinity in the irq-imx-gpcv2 driver
> > which would allow the gpc to wake up the target core when the broadcast
> > irq arrives.
> > 
> > I have a patch for this. I just need to clean it up a little bit.
> > Unfortunately, it won't go upstream since everuone thinks the gic
> > should be the one to control the affinity. This obviously doesn't work
> > on 8mq.
> > 
> > Currently, I'm at ELCE in Lyon. Will get back at the office tomorrow
> > and sned you what I have.
> > 
> 
> Hi Abel,
> 
> Do you have any news on said patch for testing? That'd be great for my
> plannings.
> 

Sorry for the late answer.

I'm dropping here the diff.

Please keep in mind that this is _not_ an official solution.

---
 drivers/irqchip/irq-imx-gpcv2.c | 42 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
index 01ce6f4..3150588 100644
--- a/drivers/irqchip/irq-imx-gpcv2.c
+++ b/drivers/irqchip/irq-imx-gpcv2.c
@@ -41,6 +41,24 @@ static void __iomem *gpcv2_idx_to_reg(struct gpcv2_irqchip_data *cd, int i)
 	return cd->gpc_base + cd->cpu2wakeup + i * 4;
 }
 
+static void __iomem *gpcv2_idx_to_reg_cpu(struct gpcv2_irqchip_data *cd,
+					int i, int cpu)
+{
+	u32 offset =  GPC_IMR1_CORE0;
+	switch(cpu) {
+	case 1:
+		offset = GPC_IMR1_CORE1;
+		break;
+	case 2:
+		offset = GPC_IMR1_CORE2;
+		break;
+	case 3:
+		offset = GPC_IMR1_CORE3;
+		break;
+	}
+	return cd->gpc_base + offset + i * 4;
+}
+
 static int gpcv2_wakeup_source_save(void)
 {
 	struct gpcv2_irqchip_data *cd;
@@ -163,6 +181,28 @@ static void imx_gpcv2_irq_mask(struct irq_data *d)
 	irq_chip_mask_parent(d);
 }
 
+static int imx_gpcv2_irq_set_affinity(struct irq_data *d,
+				 const struct cpumask *dest, bool force)
+{
+	struct gpcv2_irqchip_data *cd = d->chip_data;
+	void __iomem *reg;
+	u32 val;
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		raw_spin_lock(&cd->rlock);
+		reg = gpcv2_idx_to_reg_cpu(cd, d->hwirq / 32, cpu);
+		val = readl_relaxed(reg);
+		val |= BIT(d->hwirq % 32);
+		if (cpumask_test_cpu(cpu, dest))
+			val &= ~BIT(d->hwirq % 32);
+		writel_relaxed(val, reg);
+		raw_spin_unlock(&cd->rlock);
+	}
+
+	return irq_chip_set_affinity_parent(d, dest, force);
+}
+
 static struct irq_chip gpcv2_irqchip_data_chip = {
 	.name			= "GPCv2",
 	.irq_eoi		= irq_chip_eoi_parent,
@@ -172,7 +212,7 @@ static struct irq_chip gpcv2_irqchip_data_chip = {
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_set_type		= irq_chip_set_type_parent,
 #ifdef CONFIG_SMP
-	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_set_affinity	= imx_gpcv2_irq_set_affinity,
 #endif
 };
 
-- 


> thanks a lot,
> 
>                                  martin
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-11-04 10:35         ` Abel Vesa
@ 2019-11-06 11:59           ` Martin Kepplinger
  2019-11-06 22:36             ` Leonard Crestez
  0 siblings, 1 reply; 38+ messages in thread
From: Martin Kepplinger @ 2019-11-06 11:59 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Fabio Estevam, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Abel Vesa, Thomas Gleixner, Leonard Crestez, Shawn Guo,
	linux-arm-kernel, Lucas Stach

On 04.11.19 11:35, Abel Vesa wrote:
> On 19-11-04 09:49:18, Martin Kepplinger wrote:
>> On 30.10.19 09:08, Abel Vesa wrote:
>>> On 19-10-30 07:11:37, Martin Kepplinger wrote:
>>>> On 23.06.19 13:47, Martin Kepplinger wrote:
>>>>> On 10.06.19 14:13, Abel Vesa wrote:
>>>>>> This is another alternative for the RFC:
>>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C50f2d9cf92ae4c41db1308d76103e468%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637084541652623937&amp;sdata=eY8TR3bpvYBWGZ7Xd58%2BK8Ig0qJ3ZqTWO8fNS5X0tj8%3D&amp;reserved=0
>>>>>>
>>>>>> This new workaround proposal is a little bit more hacky but more contained
>>>>>> since everything is done within the irq-imx-gpcv2 driver.
>>>>>>
>>>>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>>>>> handler and registers instead a wrapper which calls in the 'hijacked' 
>>>>>> handler, after that calling into EL3 which will take care of the actual
>>>>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>>>>
>>>>>> I also have the patches ready for TF-A but I'll hold on to them until I see if
>>>>>> this has a chance of getting in.
>>>>>
>>>>
>>>> Hi Abel,
>>>>
>>>> Running this workaround doesn't seem to work anymore on 5.4-rcX. Linux
>>>> doesn't boot, with ATF unchanged (includes your workaround changes). I
>>>> can try to add more details to this...
>>>>
>>>
>>> This is happening because the system counter is now enabled on 8mq.
>>> And since the irq-imx-gpcv2 is using as irq_set_affinity the 
>>> irq_chip_set_affinity_parent. This is because the actual implementation
>>> of the driver relies on GIC to set the right affinity. On a SoC
>>> that has the wake_request signales linked to the power controller this
>>> works fine. Since the system counter is actually the tick broadcast
>>> device and the set affinity relies only on GIC, the cores can't be
>>> woken up by the broadcast interrupt.
>>>
>>>> Have you tested this for 5.4? Could you update this workaround? Please
>>>> let me know if I missed any earlier update on this (having a cpu-sleep
>>>> idle state).
>>>>
>>>
>>> The solution is to implement the set affinity in the irq-imx-gpcv2 driver
>>> which would allow the gpc to wake up the target core when the broadcast
>>> irq arrives.
>>>
>>> I have a patch for this. I just need to clean it up a little bit.
>>> Unfortunately, it won't go upstream since everuone thinks the gic
>>> should be the one to control the affinity. This obviously doesn't work
>>> on 8mq.
>>>
>>> Currently, I'm at ELCE in Lyon. Will get back at the office tomorrow
>>> and sned you what I have.
>>>
>>
>> Hi Abel,
>>
>> Do you have any news on said patch for testing? That'd be great for my
>> plannings.
>>
> 
> Sorry for the late answer.
> 
> I'm dropping here the diff.
> 
> Please keep in mind that this is _not_ an official solution.
> 
> ---
>  drivers/irqchip/irq-imx-gpcv2.c | 42 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> index 01ce6f4..3150588 100644
> --- a/drivers/irqchip/irq-imx-gpcv2.c
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -41,6 +41,24 @@ static void __iomem *gpcv2_idx_to_reg(struct gpcv2_irqchip_data *cd, int i)
>  	return cd->gpc_base + cd->cpu2wakeup + i * 4;
>  }
>  
> +static void __iomem *gpcv2_idx_to_reg_cpu(struct gpcv2_irqchip_data *cd,
> +					int i, int cpu)
> +{
> +	u32 offset =  GPC_IMR1_CORE0;
> +	switch(cpu) {
> +	case 1:
> +		offset = GPC_IMR1_CORE1;
> +		break;
> +	case 2:
> +		offset = GPC_IMR1_CORE2;
> +		break;
> +	case 3:
> +		offset = GPC_IMR1_CORE3;
> +		break;
> +	}
> +	return cd->gpc_base + offset + i * 4;
> +}
> +
>  static int gpcv2_wakeup_source_save(void)
>  {
>  	struct gpcv2_irqchip_data *cd;
> @@ -163,6 +181,28 @@ static void imx_gpcv2_irq_mask(struct irq_data *d)
>  	irq_chip_mask_parent(d);
>  }
>  
> +static int imx_gpcv2_irq_set_affinity(struct irq_data *d,
> +				 const struct cpumask *dest, bool force)
> +{
> +	struct gpcv2_irqchip_data *cd = d->chip_data;
> +	void __iomem *reg;
> +	u32 val;
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		raw_spin_lock(&cd->rlock);
> +		reg = gpcv2_idx_to_reg_cpu(cd, d->hwirq / 32, cpu);
> +		val = readl_relaxed(reg);
> +		val |= BIT(d->hwirq % 32);
> +		if (cpumask_test_cpu(cpu, dest))
> +			val &= ~BIT(d->hwirq % 32);
> +		writel_relaxed(val, reg);
> +		raw_spin_unlock(&cd->rlock);
> +	}
> +
> +	return irq_chip_set_affinity_parent(d, dest, force);
> +}
> +
>  static struct irq_chip gpcv2_irqchip_data_chip = {
>  	.name			= "GPCv2",
>  	.irq_eoi		= irq_chip_eoi_parent,
> @@ -172,7 +212,7 @@ static struct irq_chip gpcv2_irqchip_data_chip = {
>  	.irq_retrigger		= irq_chip_retrigger_hierarchy,
>  	.irq_set_type		= irq_chip_set_type_parent,
>  #ifdef CONFIG_SMP
> -	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +	.irq_set_affinity	= imx_gpcv2_irq_set_affinity,
>  #endif
>  };
>  
> 


hi Abel,

I guess this diff does not apply when using this reworked change:
https://source.puri.sm/Librem5/linux-next/commit/e59807ae0e236512761b751abc84a9b129d7fcda
which has worked for me when running 5.3.

At least on 5.4-rc5, using your change, I still get

cat /sys/devices/system/cpu/cpuidle/current_driver
none

But also when trying to rewrite your patch against irq-gic-v3.c at least
nothing changes for me (I might have done that wrong as well though).

What needs to change (in order to have the cpu-sleep state / idle
driver) based on the above "reworked" workaround?

Could the config have changed? CONFIG_ARM_CPUIDLE should be the only
needed path, or did things change there in 5.4?

I know all this is no real solution, but currently the only way to have
said sleep state on top of mainline. so be it for now.

thanks for your help!

                            martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-11-06 11:59           ` Martin Kepplinger
@ 2019-11-06 22:36             ` Leonard Crestez
  2019-11-08 11:21               ` Martin Kepplinger
  2019-11-25 17:23               ` Martin Kepplinger
  0 siblings, 2 replies; 38+ messages in thread
From: Leonard Crestez @ 2019-11-06 22:36 UTC (permalink / raw)
  To: Martin Kepplinger, Abel Vesa, Lorenzo Pieralisi
  Cc: Mark Rutland, devicetree, Jacky Bai, Carlo Caione, Marc Zyngier,
	Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring, dl-linux-imx,
	Pengutronix Kernel Team, Thomas Gleixner, Fabio Estevam,
	linux-arm-kernel, Lucas Stach

On 06.11.2019 13:59, Martin Kepplinger wrote:
> On 04.11.19 11:35, Abel Vesa wrote:
>> On 19-11-04 09:49:18, Martin Kepplinger wrote:
>>> On 30.10.19 09:08, Abel Vesa wrote:
>>>> On 19-10-30 07:11:37, Martin Kepplinger wrote:
>>>>> On 23.06.19 13:47, Martin Kepplinger wrote:
>>>>>> On 10.06.19 14:13, Abel Vesa wrote:
>>>>>>> This is another alternative for the RFC:
>>>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C6ca438b3b9e44d70ac7608d762b0c030%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637086383589318475&amp;sdata=NyFLkQ8PUfC7PGejDK7NBJoQu36ZfaYvg9yuJvHedzo%3D&amp;reserved=0
>>>>>>>
>>>>>>> This new workaround proposal is a little bit more hacky but more contained
>>>>>>> since everything is done within the irq-imx-gpcv2 driver.
>>>>>>>
>>>>>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>>>>>> handler and registers instead a wrapper which calls in the 'hijacked'
>>>>>>> handler, after that calling into EL3 which will take care of the actual
>>>>>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>>>>>
>>>>>>> I also have the patches ready for TF-A but I'll hold on to them until I see if
>>>>>>> this has a chance of getting in.
>>>>>>
>>>>>
>>>>> Hi Abel,
>>>>>
>>>>> Running this workaround doesn't seem to work anymore on 5.4-rcX. Linux
>>>>> doesn't boot, with ATF unchanged (includes your workaround changes). I
>>>>> can try to add more details to this...
>>>>>
>>>>
>>>> This is happening because the system counter is now enabled on 8mq.
>>>> And since the irq-imx-gpcv2 is using as irq_set_affinity the
>>>> irq_chip_set_affinity_parent. This is because the actual implementation
>>>> of the driver relies on GIC to set the right affinity. On a SoC
>>>> that has the wake_request signales linked to the power controller this
>>>> works fine. Since the system counter is actually the tick broadcast
>>>> device and the set affinity relies only on GIC, the cores can't be
>>>> woken up by the broadcast interrupt.
>>>>
>>>>> Have you tested this for 5.4? Could you update this workaround? Please
>>>>> let me know if I missed any earlier update on this (having a cpu-sleep
>>>>> idle state).
>>>>>
>>>>
>>>> The solution is to implement the set affinity in the irq-imx-gpcv2 driver
>>>> which would allow the gpc to wake up the target core when the broadcast
>>>> irq arrives.
>>>>
>>>> I have a patch for this. I just need to clean it up a little bit.
>>>> Unfortunately, it won't go upstream since everuone thinks the gic
>>>> should be the one to control the affinity. This obviously doesn't work
>>>> on 8mq.
>>>>
>>>> Currently, I'm at ELCE in Lyon. Will get back at the office tomorrow
>>>> and sned you what I have.
>>>>
>>>
>>> Hi Abel,
>>>
>>> Do you have any news on said patch for testing? That'd be great for my
>>> plannings.
>>>
>>
>> Sorry for the late answer.
>>
>> I'm dropping here the diff.
>>
>> Please keep in mind that this is _not_ an official solution.
>>
>> ---
>>   drivers/irqchip/irq-imx-gpcv2.c | 42 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
>> index 01ce6f4..3150588 100644
>> --- a/drivers/irqchip/irq-imx-gpcv2.c
>> +++ b/drivers/irqchip/irq-imx-gpcv2.c
>> @@ -41,6 +41,24 @@ static void __iomem *gpcv2_idx_to_reg(struct gpcv2_irqchip_data *cd, int i)
>>   	return cd->gpc_base + cd->cpu2wakeup + i * 4;
>>   }
>>   
>> +static void __iomem *gpcv2_idx_to_reg_cpu(struct gpcv2_irqchip_data *cd,
>> +					int i, int cpu)
>> +{
>> +	u32 offset =  GPC_IMR1_CORE0;
>> +	switch(cpu) {
>> +	case 1:
>> +		offset = GPC_IMR1_CORE1;
>> +		break;
>> +	case 2:
>> +		offset = GPC_IMR1_CORE2;
>> +		break;
>> +	case 3:
>> +		offset = GPC_IMR1_CORE3;
>> +		break;
>> +	}
>> +	return cd->gpc_base + offset + i * 4;
>> +}
>> +
>>   static int gpcv2_wakeup_source_save(void)
>>   {
>>   	struct gpcv2_irqchip_data *cd;
>> @@ -163,6 +181,28 @@ static void imx_gpcv2_irq_mask(struct irq_data *d)
>>   	irq_chip_mask_parent(d);
>>   }
>>   
>> +static int imx_gpcv2_irq_set_affinity(struct irq_data *d,
>> +				 const struct cpumask *dest, bool force)
>> +{
>> +	struct gpcv2_irqchip_data *cd = d->chip_data;
>> +	void __iomem *reg;
>> +	u32 val;
>> +	int cpu;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		raw_spin_lock(&cd->rlock);
>> +		reg = gpcv2_idx_to_reg_cpu(cd, d->hwirq / 32, cpu);
>> +		val = readl_relaxed(reg);
>> +		val |= BIT(d->hwirq % 32);
>> +		if (cpumask_test_cpu(cpu, dest))
>> +			val &= ~BIT(d->hwirq % 32);
>> +		writel_relaxed(val, reg);
>> +		raw_spin_unlock(&cd->rlock);
>> +	}
>> +
>> +	return irq_chip_set_affinity_parent(d, dest, force);
>> +}
>> +
>>   static struct irq_chip gpcv2_irqchip_data_chip = {
>>   	.name			= "GPCv2",
>>   	.irq_eoi		= irq_chip_eoi_parent,
>> @@ -172,7 +212,7 @@ static struct irq_chip gpcv2_irqchip_data_chip = {
>>   	.irq_retrigger		= irq_chip_retrigger_hierarchy,
>>   	.irq_set_type		= irq_chip_set_type_parent,
>>   #ifdef CONFIG_SMP
>> -	.irq_set_affinity	= irq_chip_set_affinity_parent,
>> +	.irq_set_affinity	= imx_gpcv2_irq_set_affinity,
>>   #endif
>>   };

This is prone to race conditions.

In NXP tree there is different gpcv2 irqchip driver which does all GPC 
IMR register manipulation in TF-A through SMC calls. The cpuidle 
workaround also manipulates the same registers and does so safely under 
a lock.

If OS also writes to same IMR register then set_affinity for SPIs 1-31 
can potentially race with one those cores being woken up. This is very 
unlikely (set_affinity calls are rare) but in the worst case the system 
could still hang on lost IPI.

> I guess this diff does not apply when using this reworked change:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2FLibrem5%2Flinux-next%2Fcommit%2Fe59807ae0e236512761b751abc84a9b129d7fcda&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C6ca438b3b9e44d70ac7608d762b0c030%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637086383589318475&amp;sdata=Mf%2BFtqFSG4xHL3IGPrD%2FOweR8qoJHV0IKuziPIUK%2Bsw%3D&amp;reserved=0
> which has worked for me when running 5.3.
> 
> At least on 5.4-rc5, using your change, I still get
> 
> cat /sys/devices/system/cpu/cpuidle/current_driver
> none

This reads "psci_idle" for me in linux-next on imx8mm. Your problem 
seems to be related to probing the cpuidle driver, not related to any 
hardware workarounds.

> But also when trying to rewrite your patch against irq-gic-v3.c at least
> nothing changes for me (I might have done that wrong as well though).
> 
> What needs to change (in order to have the cpu-sleep state / idle
> driver) based on the above "reworked" workaround?
> 
> Could the config have changed? CONFIG_ARM_CPUIDLE should be the only
> needed path, or did things change there in 5.4?

It seems there were some recent cleanups in the cpuidle psci core code, 
maybe you need config updates?

https://patchwork.kernel.org/cover/11052723/

> I know all this is no real solution, but currently the only way to have
> said sleep state on top of mainline. so be it for now.
Can you use the gpcv2 driver from NXP tree?

--
Regards,
Leonard

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-11-06 22:36             ` Leonard Crestez
@ 2019-11-08 11:21               ` Martin Kepplinger
  2019-11-08 11:50                 ` Abel Vesa
  2019-11-25 17:23               ` Martin Kepplinger
  1 sibling, 1 reply; 38+ messages in thread
From: Martin Kepplinger @ 2019-11-08 11:21 UTC (permalink / raw)
  To: Leonard Crestez, Abel Vesa, Lorenzo Pieralisi
  Cc: Mark Rutland, devicetree, Jacky Bai, Carlo Caione, Marc Zyngier,
	Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring, dl-linux-imx,
	Pengutronix Kernel Team, Thomas Gleixner, Fabio Estevam,
	linux-arm-kernel, Lucas Stach

On 06.11.19 23:36, Leonard Crestez wrote:
> On 06.11.2019 13:59, Martin Kepplinger wrote:
>> On 04.11.19 11:35, Abel Vesa wrote:
>>> On 19-11-04 09:49:18, Martin Kepplinger wrote:
>>>> On 30.10.19 09:08, Abel Vesa wrote:
>>>>> On 19-10-30 07:11:37, Martin Kepplinger wrote:
>>>>>> On 23.06.19 13:47, Martin Kepplinger wrote:
>>>>>>> On 10.06.19 14:13, Abel Vesa wrote:
>>>>>>>> This is another alternative for the RFC:
>>>>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C6ca438b3b9e44d70ac7608d762b0c030%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637086383589318475&amp;sdata=NyFLkQ8PUfC7PGejDK7NBJoQu36ZfaYvg9yuJvHedzo%3D&amp;reserved=0
>>>>>>>>
>>>>>>>> This new workaround proposal is a little bit more hacky but more contained
>>>>>>>> since everything is done within the irq-imx-gpcv2 driver.
>>>>>>>>
>>>>>>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>>>>>>> handler and registers instead a wrapper which calls in the 'hijacked'
>>>>>>>> handler, after that calling into EL3 which will take care of the actual
>>>>>>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>>>>>>
>>>>>>>> I also have the patches ready for TF-A but I'll hold on to them until I see if
>>>>>>>> this has a chance of getting in.
>>>>>>>
>>>>>>
>>>>>> Hi Abel,
>>>>>>
>>>>>> Running this workaround doesn't seem to work anymore on 5.4-rcX. Linux
>>>>>> doesn't boot, with ATF unchanged (includes your workaround changes). I
>>>>>> can try to add more details to this...
>>>>>>
>>>>>
>>>>> This is happening because the system counter is now enabled on 8mq.
>>>>> And since the irq-imx-gpcv2 is using as irq_set_affinity the
>>>>> irq_chip_set_affinity_parent. This is because the actual implementation
>>>>> of the driver relies on GIC to set the right affinity. On a SoC
>>>>> that has the wake_request signales linked to the power controller this
>>>>> works fine. Since the system counter is actually the tick broadcast
>>>>> device and the set affinity relies only on GIC, the cores can't be
>>>>> woken up by the broadcast interrupt.
>>>>>
>>>>>> Have you tested this for 5.4? Could you update this workaround? Please
>>>>>> let me know if I missed any earlier update on this (having a cpu-sleep
>>>>>> idle state).
>>>>>>
>>>>>
>>>>> The solution is to implement the set affinity in the irq-imx-gpcv2 driver
>>>>> which would allow the gpc to wake up the target core when the broadcast
>>>>> irq arrives.
>>>>>
>>>>> I have a patch for this. I just need to clean it up a little bit.
>>>>> Unfortunately, it won't go upstream since everuone thinks the gic
>>>>> should be the one to control the affinity. This obviously doesn't work
>>>>> on 8mq.
>>>>>
>>>>> Currently, I'm at ELCE in Lyon. Will get back at the office tomorrow
>>>>> and sned you what I have.
>>>>>
>>>>
>>>> Hi Abel,
>>>>
>>>> Do you have any news on said patch for testing? That'd be great for my
>>>> plannings.
>>>>
>>>
>>> Sorry for the late answer.
>>>
>>> I'm dropping here the diff.
>>>
>>> Please keep in mind that this is _not_ an official solution.
>>>
>>> ---
>>>   drivers/irqchip/irq-imx-gpcv2.c | 42 ++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 41 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
>>> index 01ce6f4..3150588 100644
>>> --- a/drivers/irqchip/irq-imx-gpcv2.c
>>> +++ b/drivers/irqchip/irq-imx-gpcv2.c
>>> @@ -41,6 +41,24 @@ static void __iomem *gpcv2_idx_to_reg(struct gpcv2_irqchip_data *cd, int i)
>>>   	return cd->gpc_base + cd->cpu2wakeup + i * 4;
>>>   }
>>>   
>>> +static void __iomem *gpcv2_idx_to_reg_cpu(struct gpcv2_irqchip_data *cd,
>>> +					int i, int cpu)
>>> +{
>>> +	u32 offset =  GPC_IMR1_CORE0;
>>> +	switch(cpu) {
>>> +	case 1:
>>> +		offset = GPC_IMR1_CORE1;
>>> +		break;
>>> +	case 2:
>>> +		offset = GPC_IMR1_CORE2;
>>> +		break;
>>> +	case 3:
>>> +		offset = GPC_IMR1_CORE3;
>>> +		break;
>>> +	}
>>> +	return cd->gpc_base + offset + i * 4;
>>> +}
>>> +
>>>   static int gpcv2_wakeup_source_save(void)
>>>   {
>>>   	struct gpcv2_irqchip_data *cd;
>>> @@ -163,6 +181,28 @@ static void imx_gpcv2_irq_mask(struct irq_data *d)
>>>   	irq_chip_mask_parent(d);
>>>   }
>>>   
>>> +static int imx_gpcv2_irq_set_affinity(struct irq_data *d,
>>> +				 const struct cpumask *dest, bool force)
>>> +{
>>> +	struct gpcv2_irqchip_data *cd = d->chip_data;
>>> +	void __iomem *reg;
>>> +	u32 val;
>>> +	int cpu;
>>> +
>>> +	for_each_possible_cpu(cpu) {
>>> +		raw_spin_lock(&cd->rlock);
>>> +		reg = gpcv2_idx_to_reg_cpu(cd, d->hwirq / 32, cpu);
>>> +		val = readl_relaxed(reg);
>>> +		val |= BIT(d->hwirq % 32);
>>> +		if (cpumask_test_cpu(cpu, dest))
>>> +			val &= ~BIT(d->hwirq % 32);
>>> +		writel_relaxed(val, reg);
>>> +		raw_spin_unlock(&cd->rlock);
>>> +	}
>>> +
>>> +	return irq_chip_set_affinity_parent(d, dest, force);
>>> +}
>>> +
>>>   static struct irq_chip gpcv2_irqchip_data_chip = {
>>>   	.name			= "GPCv2",
>>>   	.irq_eoi		= irq_chip_eoi_parent,
>>> @@ -172,7 +212,7 @@ static struct irq_chip gpcv2_irqchip_data_chip = {
>>>   	.irq_retrigger		= irq_chip_retrigger_hierarchy,
>>>   	.irq_set_type		= irq_chip_set_type_parent,
>>>   #ifdef CONFIG_SMP
>>> -	.irq_set_affinity	= irq_chip_set_affinity_parent,
>>> +	.irq_set_affinity	= imx_gpcv2_irq_set_affinity,
>>>   #endif
>>>   };
> 
> This is prone to race conditions.
> 
> In NXP tree there is different gpcv2 irqchip driver which does all GPC 
> IMR register manipulation in TF-A through SMC calls. The cpuidle 
> workaround also manipulates the same registers and does so safely under 
> a lock.>
> If OS also writes to same IMR register then set_affinity for SPIs 1-31 
> can potentially race with one those cores being woken up. This is very 
> unlikely (set_affinity calls are rare) but in the worst case the system 
> could still hang on lost IPI.
> 
>> I guess this diff does not apply when using this reworked change:
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2FLibrem5%2Flinux-next%2Fcommit%2Fe59807ae0e236512761b751abc84a9b129d7fcda&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C6ca438b3b9e44d70ac7608d762b0c030%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637086383589318475&amp;sdata=Mf%2BFtqFSG4xHL3IGPrD%2FOweR8qoJHV0IKuziPIUK%2Bsw%3D&amp;reserved=0
>> which has worked for me when running 5.3.
>>
>> At least on 5.4-rc5, using your change, I still get
>>
>> cat /sys/devices/system/cpu/cpuidle/current_driver
>> none
> 
> This reads "psci_idle" for me in linux-next on imx8mm. Your problem 
> seems to be related to probing the cpuidle driver, not related to any 
> hardware workarounds.
> 
>> But also when trying to rewrite your patch against irq-gic-v3.c at least
>> nothing changes for me (I might have done that wrong as well though).
>>
>> What needs to change (in order to have the cpu-sleep state / idle
>> driver) based on the above "reworked" workaround?
>>
>> Could the config have changed? CONFIG_ARM_CPUIDLE should be the only
>> needed path, or did things change there in 5.4?
> 
> It seems there were some recent cleanups in the cpuidle psci core code, 
> maybe you need config updates?
> 
> https://patchwork.kernel.org/cover/11052723/
> 
>> I know all this is no real solution, but currently the only way to have
>> said sleep state on top of mainline. so be it for now.
> Can you use the gpcv2 driver from NXP tree?

hm. what driver do you mean? at least
https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/irqchip/irq-imx-gpcv2.c?h=imx_4.19.35_1.0.0
seems not support imx8mq yet.

> 
> --
> Regards,
> Leonard
> 

Hi Leonard, hi Abel,

Thanks for having a look! To sum up this problem and not to get confused:

We have the workaround that changes irq-imx-gpcv2 from this very email
thread, to be used with mainline ATF. when applying Abel's recent diff,
Linux 5.4 boots but I still don't have a cpuidle driver.

When I enable CONFIG_ARM_PSCI_CPUIDLE, the kernel hangs during boot
(after probing mmc, but that doesn't tell much)

What do I miss?


Then (in parallel) we have NXP's ATF:
https://source.codeaurora.org/external/imx/imx-atf/log/?h=imx_4.19.35_1.0.0
that I test in parallel (and will actually want to have cpuidle right
now too). The workaround in Linux in that case looks like so:
https://source.codeaurora.org/external/imx/linux-imx/commit/?h=imx_4.19.35_1.0.0&id=26a59057f88997dfe48ab7f81898ddd6b6d3903e
which changes irq-gic-v3 only.

Since 5.4, also no cpu-sleep state anymore. What would need to change in
that "NXP" case, for 5.4 to have cpuidle again?

When I enable ARM_PSCI_CPUIDLE here, right now Linux hangs during boot
(during probing sdhci but again that seems random).

I'm still happy for hints :)

Thanks,

                              martin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-11-08 11:21               ` Martin Kepplinger
@ 2019-11-08 11:50                 ` Abel Vesa
  2019-11-08 14:17                   ` Martin Kepplinger
  0 siblings, 1 reply; 38+ messages in thread
From: Abel Vesa @ 2019-11-08 11:50 UTC (permalink / raw)
  To: Martin Kepplinger
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Fabio Estevam, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Thomas Gleixner, Leonard Crestez, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On 19-11-08 12:21:21, Martin Kepplinger wrote:

> Hi Leonard, hi Abel,
> 
> Thanks for having a look! To sum up this problem and not to get confused:
> 
> We have the workaround that changes irq-imx-gpcv2 from this very email
> thread, to be used with mainline ATF. when applying Abel's recent diff,
> Linux 5.4 boots but I still don't have a cpuidle driver.
> 
> When I enable CONFIG_ARM_PSCI_CPUIDLE, the kernel hangs during boot
> (after probing mmc, but that doesn't tell much)
> 
> What do I miss?
> 

OK, please fetch the branches called "imx8mq-err11171" from both following github repos and give it a try:

https://github.com/abelvesa/linux.git

and

https://github.com/abelvesa/arm-trusted-firmware.git

I just tested it. Works with defconfig.

> 
> Then (in parallel) we have NXP's ATF:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Fimx-atf%2Flog%2F%3Fh%3Dimx_4.19.35_1.0.0&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C0a5a09f616c84932e06d08d7643dccaf%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637088088896134121&amp;sdata=tq0aUGawG%2FhRRXZB9jdIi2xSNGHINhbWM1ZpDKPFrqU%3D&amp;reserved=0
> that I test in parallel (and will actually want to have cpuidle right
> now too). The workaround in Linux in that case looks like so:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2F%3Fh%3Dimx_4.19.35_1.0.0%26id%3D26a59057f88997dfe48ab7f81898ddd6b6d3903e&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C0a5a09f616c84932e06d08d7643dccaf%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637088088896134121&amp;sdata=GqmtzpS8fB4bxmOXxTNQZrWNCV18lNu7XpX5dmcAEaY%3D&amp;reserved=0
> which changes irq-gic-v3 only.
> 

Forget about the NXP arm-trusted-firmware for now. It will never work with mainline + the workaround patches.

> Since 5.4, also no cpu-sleep state anymore. What would need to change in
> that "NXP" case, for 5.4 to have cpuidle again?
> 
> When I enable ARM_PSCI_CPUIDLE here, right now Linux hangs during boot
> (during probing sdhci but again that seems random).
> 
> I'm still happy for hints :)
> 
> Thanks,
> 
>                               martin
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-11-08 11:50                 ` Abel Vesa
@ 2019-11-08 14:17                   ` Martin Kepplinger
  2019-11-11  7:54                     ` Abel Vesa
  0 siblings, 1 reply; 38+ messages in thread
From: Martin Kepplinger @ 2019-11-08 14:17 UTC (permalink / raw)
  To: Abel Vesa
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Fabio Estevam, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Thomas Gleixner, Leonard Crestez, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On 08.11.19 12:50, Abel Vesa wrote:
> On 19-11-08 12:21:21, Martin Kepplinger wrote:
> 
>> Hi Leonard, hi Abel,
>>
>> Thanks for having a look! To sum up this problem and not to get confused:
>>
>> We have the workaround that changes irq-imx-gpcv2 from this very email
>> thread, to be used with mainline ATF. when applying Abel's recent diff,
>> Linux 5.4 boots but I still don't have a cpuidle driver.
>>
>> When I enable CONFIG_ARM_PSCI_CPUIDLE, the kernel hangs during boot
>> (after probing mmc, but that doesn't tell much)
>>
>> What do I miss?
>>
> 
> OK, please fetch the branches called "imx8mq-err11171" from both following github repos and give it a try:
> 
> https://github.com/abelvesa/linux.git
> 
> and
> 
> https://github.com/abelvesa/arm-trusted-firmware.git
> 
> I just tested it. Works with defconfig.
> 

thanks for the reminder. I was missing IMX_SCU_PD appearently.

thanks for the effort, I hope this is useful for others too.

                         martin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-11-08 14:17                   ` Martin Kepplinger
@ 2019-11-11  7:54                     ` Abel Vesa
  0 siblings, 0 replies; 38+ messages in thread
From: Abel Vesa @ 2019-11-11  7:54 UTC (permalink / raw)
  To: Martin Kepplinger
  Cc: Mark Rutland, devicetree, Lorenzo Pieralisi, Jacky Bai,
	Carlo Caione, Marc Zyngier, Fabio Estevam, Sascha Hauer,
	linux-kernel, Rob Herring, dl-linux-imx, Pengutronix Kernel Team,
	Thomas Gleixner, Leonard Crestez, Shawn Guo, linux-arm-kernel,
	Lucas Stach

On 19-11-08 15:17:12, Martin Kepplinger wrote:
> On 08.11.19 12:50, Abel Vesa wrote:
> > On 19-11-08 12:21:21, Martin Kepplinger wrote:
> > 
> >> Hi Leonard, hi Abel,
> >>
> >> Thanks for having a look! To sum up this problem and not to get confused:
> >>
> >> We have the workaround that changes irq-imx-gpcv2 from this very email
> >> thread, to be used with mainline ATF. when applying Abel's recent diff,
> >> Linux 5.4 boots but I still don't have a cpuidle driver.
> >>
> >> When I enable CONFIG_ARM_PSCI_CPUIDLE, the kernel hangs during boot
> >> (after probing mmc, but that doesn't tell much)
> >>
> >> What do I miss?
> >>
> > 
> > OK, please fetch the branches called "imx8mq-err11171" from both following github repos and give it a try:
> > 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fabelvesa%2Flinux.git&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C663191bd4af6489d08f808d764565ca2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637088194388256459&amp;sdata=HydsZLtPExbRx7qDByTI%2FCYFGYS67fU9FDsLmHy7x7o%3D&amp;reserved=0
> > 
> > and
> > 
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fabelvesa%2Farm-trusted-firmware.git&amp;data=02%7C01%7Cabel.vesa%40nxp.com%7C663191bd4af6489d08f808d764565ca2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637088194388256459&amp;sdata=zkCpOrEvoiNGj7BBuVuHzTWmBFaZk0SCuePOKZA3T2o%3D&amp;reserved=0
> > 
> > I just tested it. Works with defconfig.
> > 
> 
> thanks for the reminder. I was missing IMX_SCU_PD appearently.
> 

There is no SCU on 8MQ so I don't think that was the problem.

Maybe something else.

> thanks for the effort, I hope this is useful for others too.

I'll try to keep those up-to-date.

> 
>                          martin
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ
  2019-11-06 22:36             ` Leonard Crestez
  2019-11-08 11:21               ` Martin Kepplinger
@ 2019-11-25 17:23               ` Martin Kepplinger
  1 sibling, 0 replies; 38+ messages in thread
From: Martin Kepplinger @ 2019-11-25 17:23 UTC (permalink / raw)
  To: Leonard Crestez, Abel Vesa, Lorenzo Pieralisi
  Cc: Mark Rutland, devicetree, Jacky Bai, Carlo Caione, Marc Zyngier,
	Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring, dl-linux-imx,
	Pengutronix Kernel Team, Thomas Gleixner, Fabio Estevam,
	linux-arm-kernel, Lucas Stach

On 06.11.19 23:36, Leonard Crestez wrote:
> On 06.11.2019 13:59, Martin Kepplinger wrote:
>> On 04.11.19 11:35, Abel Vesa wrote:
>>> On 19-11-04 09:49:18, Martin Kepplinger wrote:
>>>> On 30.10.19 09:08, Abel Vesa wrote:
>>>>> On 19-10-30 07:11:37, Martin Kepplinger wrote:
>>>>>> On 23.06.19 13:47, Martin Kepplinger wrote:
>>>>>>> On 10.06.19 14:13, Abel Vesa wrote:
>>>>>>>> This is another alternative for the RFC:
>>>>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F3%2F27%2F545&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C6ca438b3b9e44d70ac7608d762b0c030%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637086383589318475&amp;sdata=NyFLkQ8PUfC7PGejDK7NBJoQu36ZfaYvg9yuJvHedzo%3D&amp;reserved=0
>>>>>>>>
>>>>>>>> This new workaround proposal is a little bit more hacky but more contained
>>>>>>>> since everything is done within the irq-imx-gpcv2 driver.
>>>>>>>>
>>>>>>>> Basically, it 'hijacks' the registered gic_raise_softirq __smp_cross_call
>>>>>>>> handler and registers instead a wrapper which calls in the 'hijacked'
>>>>>>>> handler, after that calling into EL3 which will take care of the actual
>>>>>>>> wake up. This time, instead of expanding the PSCI ABI, we use a new vendor SIP.
>>>>>>>>
>>>>>>>> I also have the patches ready for TF-A but I'll hold on to them until I see if
>>>>>>>> this has a chance of getting in.
>>>>>>>
>>>>>>
>>>>>> Hi Abel,
>>>>>>
>>>>>> Running this workaround doesn't seem to work anymore on 5.4-rcX. Linux
>>>>>> doesn't boot, with ATF unchanged (includes your workaround changes). I
>>>>>> can try to add more details to this...
>>>>>>
>>>>>
>>>>> This is happening because the system counter is now enabled on 8mq.
>>>>> And since the irq-imx-gpcv2 is using as irq_set_affinity the
>>>>> irq_chip_set_affinity_parent. This is because the actual implementation
>>>>> of the driver relies on GIC to set the right affinity. On a SoC
>>>>> that has the wake_request signales linked to the power controller this
>>>>> works fine. Since the system counter is actually the tick broadcast
>>>>> device and the set affinity relies only on GIC, the cores can't be
>>>>> woken up by the broadcast interrupt.
>>>>>
>>>>>> Have you tested this for 5.4? Could you update this workaround? Please
>>>>>> let me know if I missed any earlier update on this (having a cpu-sleep
>>>>>> idle state).
>>>>>>
>>>>>
>>>>> The solution is to implement the set affinity in the irq-imx-gpcv2 driver
>>>>> which would allow the gpc to wake up the target core when the broadcast
>>>>> irq arrives.
>>>>>
>>>>> I have a patch for this. I just need to clean it up a little bit.
>>>>> Unfortunately, it won't go upstream since everuone thinks the gic
>>>>> should be the one to control the affinity. This obviously doesn't work
>>>>> on 8mq.
>>>>>
>>>>> Currently, I'm at ELCE in Lyon. Will get back at the office tomorrow
>>>>> and sned you what I have.
>>>>>
>>>>
>>>> Hi Abel,
>>>>
>>>> Do you have any news on said patch for testing? That'd be great for my
>>>> plannings.
>>>>
>>>
>>> Sorry for the late answer.
>>>
>>> I'm dropping here the diff.
>>>
>>> Please keep in mind that this is _not_ an official solution.
>>>
>>> ---
>>>   drivers/irqchip/irq-imx-gpcv2.c | 42 ++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 41 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
>>> index 01ce6f4..3150588 100644
>>> --- a/drivers/irqchip/irq-imx-gpcv2.c
>>> +++ b/drivers/irqchip/irq-imx-gpcv2.c
>>> @@ -41,6 +41,24 @@ static void __iomem *gpcv2_idx_to_reg(struct gpcv2_irqchip_data *cd, int i)
>>>   	return cd->gpc_base + cd->cpu2wakeup + i * 4;
>>>   }
>>>   
>>> +static void __iomem *gpcv2_idx_to_reg_cpu(struct gpcv2_irqchip_data *cd,
>>> +					int i, int cpu)
>>> +{
>>> +	u32 offset =  GPC_IMR1_CORE0;
>>> +	switch(cpu) {
>>> +	case 1:
>>> +		offset = GPC_IMR1_CORE1;
>>> +		break;
>>> +	case 2:
>>> +		offset = GPC_IMR1_CORE2;
>>> +		break;
>>> +	case 3:
>>> +		offset = GPC_IMR1_CORE3;
>>> +		break;
>>> +	}
>>> +	return cd->gpc_base + offset + i * 4;
>>> +}
>>> +
>>>   static int gpcv2_wakeup_source_save(void)
>>>   {
>>>   	struct gpcv2_irqchip_data *cd;
>>> @@ -163,6 +181,28 @@ static void imx_gpcv2_irq_mask(struct irq_data *d)
>>>   	irq_chip_mask_parent(d);
>>>   }
>>>   
>>> +static int imx_gpcv2_irq_set_affinity(struct irq_data *d,
>>> +				 const struct cpumask *dest, bool force)
>>> +{
>>> +	struct gpcv2_irqchip_data *cd = d->chip_data;
>>> +	void __iomem *reg;
>>> +	u32 val;
>>> +	int cpu;
>>> +
>>> +	for_each_possible_cpu(cpu) {
>>> +		raw_spin_lock(&cd->rlock);
>>> +		reg = gpcv2_idx_to_reg_cpu(cd, d->hwirq / 32, cpu);
>>> +		val = readl_relaxed(reg);
>>> +		val |= BIT(d->hwirq % 32);
>>> +		if (cpumask_test_cpu(cpu, dest))
>>> +			val &= ~BIT(d->hwirq % 32);
>>> +		writel_relaxed(val, reg);
>>> +		raw_spin_unlock(&cd->rlock);
>>> +	}
>>> +
>>> +	return irq_chip_set_affinity_parent(d, dest, force);
>>> +}
>>> +
>>>   static struct irq_chip gpcv2_irqchip_data_chip = {
>>>   	.name			= "GPCv2",
>>>   	.irq_eoi		= irq_chip_eoi_parent,
>>> @@ -172,7 +212,7 @@ static struct irq_chip gpcv2_irqchip_data_chip = {
>>>   	.irq_retrigger		= irq_chip_retrigger_hierarchy,
>>>   	.irq_set_type		= irq_chip_set_type_parent,
>>>   #ifdef CONFIG_SMP
>>> -	.irq_set_affinity	= irq_chip_set_affinity_parent,
>>> +	.irq_set_affinity	= imx_gpcv2_irq_set_affinity,
>>>   #endif
>>>   };
> 
> This is prone to race conditions.
> 
> In NXP tree there is different gpcv2 irqchip driver which does all GPC 
> IMR register manipulation in TF-A through SMC calls. The cpuidle 
> workaround also manipulates the same registers and does so safely under 
> a lock.
> 
> If OS also writes to same IMR register then set_affinity for SPIs 1-31 
> can potentially race with one those cores being woken up. This is very 
> unlikely (set_affinity calls are rare) but in the worst case the system 
> could still hang on lost IPI.
> 
>> I guess this diff does not apply when using this reworked change:
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.puri.sm%2FLibrem5%2Flinux-next%2Fcommit%2Fe59807ae0e236512761b751abc84a9b129d7fcda&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C6ca438b3b9e44d70ac7608d762b0c030%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637086383589318475&amp;sdata=Mf%2BFtqFSG4xHL3IGPrD%2FOweR8qoJHV0IKuziPIUK%2Bsw%3D&amp;reserved=0
>> which has worked for me when running 5.3.
>>
>> At least on 5.4-rc5, using your change, I still get
>>
>> cat /sys/devices/system/cpu/cpuidle/current_driver
>> none
> 
> This reads "psci_idle" for me in linux-next on imx8mm. Your problem 
> seems to be related to probing the cpuidle driver, not related to any 
> hardware workarounds.

thanks,

I see the "psci_idle" driver too, but I'm not able to boot from flashed
emmc when having `ARM_PSCI_CPUIDLE` enabled!

The logs below are both the last logs that get printed when startup hangs:

```
[    1.638207] imx-cpufreq-dt imx-cpufreq-dt: cpu speed grade 3 mkt
segment 0 supported-hw 0x8 0x1
[    1.683487] mmc1: SDHCI controller on 30b50000.mmc [30b50000.mmc]
using ADMA
[    1.695528] input: gpio-keys as /devices/platform/gpio-keys/input/input0
[    1.708037] input: bd718xx-pwrkey as
/devices/platform/soc@0/soc@0:bus@30800000/30a20000.i2c/i2c-0/0-004b/gpio-keys.0.auto/input/input1
[    1.721939] snvs_rtc 30370000.snvs:snvs-rtc-lp: setting system clock
to 1970-01-01T00:00:00 UTC (0)
[    1.723543] mmc1: new high speed SDIO card at address fffd
```

but the psci checker (when configured-in) seems to be ok:

```
[    1.717281] imx-cpufreq-dt imx-cpufreq-dt: cpu speed grade 3 mkt
segment 0 supported-hw 0x8 0x1
[    1.763172] mmc1: SDHCI controller on 30b50000.mmc [30b50000.mmc]
using ADMA
[    1.775368] input: gpio-keys as /devices/platform/gpio-keys/input/input1
[    1.784397] input: bd718xx-pwrkey as
/devices/platform/soc@0/soc@0:bus@30800000/30a20000.i2c/i2c-0/0-004b/gpio-keys.0.auto/input/input2
[    1.798160] snvs_rtc 30370000.snvs:snvs-rtc-lp: setting system clock
to 1970-01-01T00:00:00 UTC (0)
[    1.807668] psci_checker: PSCI checker started using 4 CPUs
[    1.813500] psci_checker: Starting hotplug tests
[    1.818351] psci_checker: Trying to turn off and on again all CPUs
[    1.826388] IRQ 6: no longer affine to CPU0
[    1.826805] CPU0: shutdown
[    1.834060] psci: CPU0 killed.
[    1.840096] CPU1: shutdown
[    1.842938] psci: CPU1 killed.
[    1.848633] CPU2: shutdown
[    1.851500] psci: CPU2 killed.
[    1.856376] Detected VIPT I-cache on CPU0
[    1.856407] GICv3: CPU0: found redistributor 0 region
0:0x0000000038880000
[    1.856459] CPU0: Booted secondary processor 0x0000000000 [0x410fd034]
[    1.862897] mmc1: new high speed SDIO card at address fffd
[    1.882136] Detected VIPT I-cache on CPU1
[    1.882155] GICv3: CPU1: found redistributor 1 region
0:0x00000000388a0000
[    1.882186] CPU1: Booted secondary processor 0x0000000001 [0x410fd034]
[    1.902604] Detected VIPT I-cache on CPU2
[    1.902624] GICv3: CPU2: found redistributor 2 region
0:0x00000000388c0000
[    1.902653] CPU2: Booted secondary processor 0x0000000002 [0x410fd034]
[    1.921604] psci_checker: Trying to turn off and on again group 0
(CPUs 0-3)
[    1.930565] IRQ 6: no longer affine to CPU0
[    1.930691] CPU0: shutdown
[    1.937961] psci: CPU0 killed.
[    1.942402] IRQ 6: no longer affine to CPU1
[    1.942518] CPU1: shutdown
[    1.949759] psci: CPU1 killed.
[    1.954370] CPU2: shutdown
[    1.957249] psci: CPU2 killed.
[    1.961582] Detected VIPT I-cache on CPU0
[    1.961600] GICv3: CPU0: found redistributor 0 region
0:0x0000000038880000
[    1.961632] CPU0: Booted secondary processor 0x0000000000 [0x410fd034]
[    1.981892] Detected VIPT I-cache on CPU1
[    1.981910] GICv3: CPU1: found redistributor 1 region
0:0x00000000388a0000
[    1.981941] CPU1: Booted secondary processor 0x0000000001 [0x410fd034]
[    2.002301] Detected VIPT I-cache on CPU2
[    2.002319] GICv3: CPU2: found redistributor 2 region
0:0x00000000388c0000
[    2.002348] CPU2: Booted secondary processor 0x0000000002 [0x410fd034]
[    2.021288] psci_checker: Hotplug tests passed OK
[    2.026241] psci_checker: Starting suspend tests (10 cycles per state)
[    2.033683] psci_checker: CPU 1 entering suspend cycles, states 1
through 1
[    2.033685] psci_checker: CPU 3 entering suspend cycles, states 1
through 1
[    2.033687] psci_checker: CPU 0 entering suspend cycles, states 1
through 1
[    2.033689] psci_checker: CPU 2 entering suspend cycles, states 1
through 1
[    2.091607] psci_checker: CPU 0 suspend test results: success 10,
shallow states 0, errors 0
[    2.100497] psci_checker: CPU 1 suspend test results: success 10,
shallow states 0, errors 0
[    2.109361] psci_checker: CPU 2 suspend test results: success 10,
shallow states 0, errors 0
[    2.118227] psci_checker: CPU 3 suspend test results: success 10,
shallow states 0, errors 0
[    2.127106] psci_checker: Suspend tests passed OK
[    2.132030] psci_checker: PSCI checker completed
```

(also when booted (via SDP) , I can't wake up from S3 or reboot.)

All the above worked with v5.3. Do you know what I could be doing wrong
on 5.4?

thanks!

                                martin


> 
>> But also when trying to rewrite your patch against irq-gic-v3.c at least
>> nothing changes for me (I might have done that wrong as well though).
>>
>> What needs to change (in order to have the cpu-sleep state / idle
>> driver) based on the above "reworked" workaround?
>>
>> Could the config have changed? CONFIG_ARM_CPUIDLE should be the only
>> needed path, or did things change there in 5.4?
> 
> It seems there were some recent cleanups in the cpuidle psci core code, 
> maybe you need config updates?
> 
> https://patchwork.kernel.org/cover/11052723/

ARM_CPUIDLE is basically replaced with ARM_PSCI_CPUIDLE

> 
>> I know all this is no real solution, but currently the only way to have
>> said sleep state on top of mainline. so be it for now.
> Can you use the gpcv2 driver from NXP tree?
> 
> --
> Regards,
> Leonard
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2019-11-25 17:23 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-10 12:13 [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ Abel Vesa
2019-06-10 12:13 ` [RFC 1/2] irqchip: irq-imx-gpcv2: Add workaround for i.MX8MQ ERR11171 Abel Vesa
2019-06-10 12:38   ` Leonard Crestez
2019-06-10 13:24   ` Marc Zyngier
2019-06-10 13:38     ` Abel Vesa
2019-06-10 13:51       ` Marc Zyngier
2019-06-10 14:12         ` Abel Vesa
2019-06-10 14:28           ` Marc Zyngier
2019-06-10 12:13 ` [RFC 2/2] arm64: dts: imx8mq: Add idle states and gpcv2 wake_request broken property Abel Vesa
2019-06-10 13:19 ` [RFC 0/2] Add workaround for core wake-up on IPI for i.MX8MQ Mark Rutland
2019-06-10 13:29   ` Abel Vesa
2019-06-10 13:39     ` Marc Zyngier
2019-06-10 13:55       ` Abel Vesa
2019-06-10 14:07         ` Marc Zyngier
2019-06-10 14:32           ` Leonard Crestez
2019-06-10 14:52             ` Marc Zyngier
2019-06-12  7:14             ` Thomas Gleixner
2019-06-12  7:35               ` Marc Zyngier
2019-06-12  7:37                 ` Thomas Gleixner
2019-06-23 11:47 ` Martin Kepplinger
2019-06-28  8:54   ` Abel Vesa
2019-07-02  6:47     ` Martin Kepplinger
2019-07-02 11:33       ` Abel Vesa
2019-07-08  7:54         ` Martin Kepplinger
2019-07-08 12:20           ` Martin Kepplinger
2019-10-30  6:11   ` Martin Kepplinger
2019-10-30  7:33     ` Martin Kepplinger
2019-10-30  8:08     ` Abel Vesa
2019-10-30  8:14       ` Martin Kepplinger
2019-11-04  8:49       ` Martin Kepplinger
2019-11-04 10:35         ` Abel Vesa
2019-11-06 11:59           ` Martin Kepplinger
2019-11-06 22:36             ` Leonard Crestez
2019-11-08 11:21               ` Martin Kepplinger
2019-11-08 11:50                 ` Abel Vesa
2019-11-08 14:17                   ` Martin Kepplinger
2019-11-11  7:54                     ` Abel Vesa
2019-11-25 17:23               ` Martin Kepplinger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).