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&data=02%7C01%7Cabel.vesa%40nxp.com%7C50f2d9cf92ae4c41db1308d76103e468%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637084541652623937&sdata=eY8TR3bpvYBWGZ7Xd58%2BK8Ig0qJ3ZqTWO8fNS5X0tj8%3D&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
