| Message ID | 20190610121346.15779-2-abel.vesa@nxp.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Add workaround for core wake-up on IPI for i.MX8MQ | expand |
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
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.
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&data=02%7C01%7Cabel.vesa%40nxp.com%7Ce23f69dbe37c4e83d7ab08d6eda6f062%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636957698629664098&sdata=tAFuqTJBWiSbeoUv8gqA9vQfeWAklCv3t4qk0RLJQKM%3D&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...
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&data=02%7C01%7Cabel.vesa%40nxp.com%7Ce23f69dbe37c4e83d7ab08d6eda6f062%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C636957698629664098&sdata=tAFuqTJBWiSbeoUv8gqA9vQfeWAklCv3t4qk0RLJQKM%3D&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.
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&data=02%7C01%7Cabel.vesa%40nxp.com%7Cf74b196c8beb46599f8408d6edaace09%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636957715230445874&sdata=ruP3qm1NTLTdoLC5XDu0uN5yNKLb4%2F2iP9kF5vdr1OI%3D&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...
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&data=02%7C01%7Cabel.vesa%40nxp.com%7Cf74b196c8beb46599f8408d6edaace09%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636957715230445874&sdata=ruP3qm1NTLTdoLC5XDu0uN5yNKLb4%2F2iP9kF5vdr1OI%3D&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.
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;
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(+)