From patchwork Thu Jul 25 19:11:06 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nicolas Pitre X-Patchwork-Id: 18594 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-gh0-f198.google.com (mail-gh0-f198.google.com [209.85.160.198]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id CE67A25DF6 for ; Thu, 25 Jul 2013 19:11:10 +0000 (UTC) Received: by mail-gh0-f198.google.com with SMTP id r13sf1870527ghr.9 for ; Thu, 25 Jul 2013 12:11:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-beenthere:x-forwarded-to:x-forwarded-for:delivered-to:date:from :to:cc:subject:in-reply-to:message-id:references:user-agent :mime-version:x-gm-message-state:x-original-sender :x-original-authentication-results:precedence:mailing-list:list-id :x-google-group-id:list-post:list-help:list-archive:list-unsubscribe :content-type; bh=XeI+Jwt6xBri3ljUFszTj0JruveW8MYwpqmfbhOdgb0=; b=YLXSDqwYQlcaI436/C4zwAj0sHq0fE4FdYuAuR7PqZwVHWimkx1UigsBtBun2LJKB5 sPR8FDvaB8E1avj8ikvFr+dnj79MngGS7MLiWpE10TgNDJv6KxGvCeLSspborbyIoTwZ 0ayOJFOjuX5BA2NKMuTJa0wavOjTNAuYD1Bl/XfuhJutAAiOq+8j9OeLpdtMU96bGvNq brspA7gNzCC+IjtKuKpLUlTMAsSTvVujeVtzRByiLuwtKtrUo3P3k1cPA0UX4z+A00+7 VBolRrgYd3zCZeDPkGrrUhRSXk9S1jTAGa30wlzuUJYno1YurqrNFt/LXH6OHffZ0jAw AVjA== X-Received: by 10.236.109.199 with SMTP id s47mr1224999yhg.2.1374779469543; Thu, 25 Jul 2013 12:11:09 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.49.81.200 with SMTP id c8ls37391qey.85.gmail; Thu, 25 Jul 2013 12:11:09 -0700 (PDT) X-Received: by 10.52.27.116 with SMTP id s20mr15610365vdg.42.1374779469445; Thu, 25 Jul 2013 12:11:09 -0700 (PDT) Received: from mail-ve0-f182.google.com (mail-ve0-f182.google.com [209.85.128.182]) by mx.google.com with ESMTPS id x12si799309vcl.67.2013.07.25.12.11.09 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 25 Jul 2013 12:11:09 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.128.182 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) client-ip=209.85.128.182; Received: by mail-ve0-f182.google.com with SMTP id m1so291575ves.13 for ; Thu, 25 Jul 2013 12:11:09 -0700 (PDT) X-Received: by 10.58.251.144 with SMTP id zk16mr18074484vec.37.1374779469244; Thu, 25 Jul 2013 12:11:09 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patches@linaro.org Received: by 10.58.165.8 with SMTP id yu8csp92653veb; Thu, 25 Jul 2013 12:11:08 -0700 (PDT) X-Received: by 10.224.51.134 with SMTP id d6mr11529292qag.39.1374779468654; Thu, 25 Jul 2013 12:11:08 -0700 (PDT) Received: from mail-qa0-f47.google.com (mail-qa0-f47.google.com [209.85.216.47]) by mx.google.com with ESMTPS id b8si12426329qea.33.2013.07.25.12.11.08 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 25 Jul 2013 12:11:08 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.216.47 is neither permitted nor denied by best guess record for domain of nicolas.pitre@linaro.org) client-ip=209.85.216.47; Received: by mail-qa0-f47.google.com with SMTP id o19so2351411qap.13 for ; Thu, 25 Jul 2013 12:11:08 -0700 (PDT) X-Received: by 10.224.60.133 with SMTP id p5mr50855988qah.101.1374779468320; Thu, 25 Jul 2013 12:11:08 -0700 (PDT) Received: from xanadu.home (modemcable044.209-83-70.mc.videotron.ca. [70.83.209.44]) by mx.google.com with ESMTPSA id nh4sm482662qeb.6.2013.07.25.12.11.06 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Thu, 25 Jul 2013 12:11:07 -0700 (PDT) Date: Thu, 25 Jul 2013 15:11:06 -0400 (EDT) From: Nicolas Pitre To: Jonathan Austin cc: "linux-arm-kernel@lists.infradead.org" , "dave.martin@linaro.org" , Lorenzo Pieralisi , "patches@linaro.org" Subject: Re: [PATCH 02/13] ARM: gic: add CPU migration support In-Reply-To: <51F10F85.7090508@arm.com> Message-ID: References: <1374550289-25305-1-git-send-email-nicolas.pitre@linaro.org> <1374550289-25305-3-git-send-email-nicolas.pitre@linaro.org> <51F10F85.7090508@arm.com> User-Agent: Alpine 2.03 (LFD 1266 2009-07-14) MIME-Version: 1.0 X-Gm-Message-State: ALoCoQlXOQ4w5I4vCJHqifpcTl5oavpDdh9e1yieSJpUE7t9SiCZswpkcsTknr98PSxwZXYWxXnT X-Original-Sender: nicolas.pitre@linaro.org X-Original-Authentication-Results: mx.google.com; spf=neutral (google.com: 209.85.128.182 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Precedence: list Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org List-ID: X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , On Thu, 25 Jul 2013, Jonathan Austin wrote: > Hi Nico, > > I've got a couple of minor questions/comments about this one... > > On 23/07/13 04:31, Nicolas Pitre wrote: > > This is required by the big.LITTLE switcher code. > > > > The gic_migrate_target() changes the CPU interface mapping for the > > current CPU to redirect SGIs to the specified interface, and it also > > updates the target CPU for each interrupts to that CPU interface > > if they were targeting the current interface. Finally, pending > > SGIs for the current CPU are forwarded to the new interface. > > > > Because Linux does not use it, the SGI source information for the > > forwarded SGIs is not preserved. Neither is the source information > > for the SGIs sent by the current CPU to other CPUs adjusted to match > > the new CPU interface mapping. The required registers are banked so > > only the target CPU could do it. > > > > Signed-off-by: Nicolas Pitre > > --- > > drivers/irqchip/irq-gic.c | 81 > > +++++++++++++++++++++++++++++++++++++++-- > > include/linux/irqchip/arm-gic.h | 4 ++ > > 2 files changed, 82 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > > index ee7c503120..6bd5a8c1aa 100644 > > --- a/drivers/irqchip/irq-gic.c > > +++ b/drivers/irqchip/irq-gic.c > > @@ -253,10 +253,9 @@ static int gic_set_affinity(struct irq_data *d, const > > struct cpumask *mask_val, > > if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids) > > return -EINVAL; > > > > + raw_spin_lock(&irq_controller_lock); > > mask = 0xff << shift; > > bit = gic_cpu_map[cpu] << shift; > > - > > - raw_spin_lock(&irq_controller_lock); > > val = readl_relaxed(reg) & ~mask; > > writel_relaxed(val | bit, reg); > > raw_spin_unlock(&irq_controller_lock); > > @@ -646,7 +645,9 @@ static void __init gic_pm_init(struct gic_chip_data > > *gic) > > void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) > > { > > int cpu; > > - unsigned long map = 0; > > + unsigned long flags, map = 0; > > + > > + raw_spin_lock_irqsave(&irq_controller_lock, flags); > > > > /* Convert our logical CPU mask into a physical one. */ > > for_each_cpu(cpu, mask) > > @@ -660,6 +661,80 @@ void gic_raise_softirq(const struct cpumask *mask, > > unsigned int irq) > > > > /* this always happens on GIC0 */ > > writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + > > GIC_DIST_SOFTINT); > > + > > + raw_spin_unlock_irqrestore(&irq_controller_lock, flags); > > +} > > +#endif > > + > > +#ifdef CONFIG_BL_SWITCHER > > +/* > > + * gic_migrate_target - migrate IRQs to another PU interface > > (nit) Should that be _C_PU? Obviously. > > + * > > + * @new_cpu_id: the CPU target ID to migrate IRQs to > > + * > > + * Migrate all peripheral interrupts with a target matching the current CPU > > + * to the interface corresponding to @new_cpu_id. The CPU interface > > mapping > > + * is also updated. Targets to other CPU interfaces are unchanged. > > + * This must be called with IRQs locally disabled. > > + */ > > +void gic_migrate_target(unsigned int new_cpu_id) > > +{ > > + unsigned int old_cpu_id, gic_irqs, gic_nr = 0; > > + void __iomem *dist_base; > > + int i, ror_val, cpu = smp_processor_id(); > > + u32 val, old_mask, active_mask; > > + > > + if (gic_nr >= MAX_GIC_NR) > > + BUG(); > > + > > + dist_base = gic_data_dist_base(&gic_data[gic_nr]); > > + if (!dist_base) > > + return; > > + gic_irqs = gic_data[gic_nr].gic_irqs; > > + > > + old_cpu_id = __ffs(gic_cpu_map[cpu]); > > + old_mask = 0x01010101 << old_cpu_id; > > I don't think this is very clear, though section 4.3.12 of the GIC spec helps > a lot! A little pointer or some more self-evident name for the mask might be > nice... > > old_mask = GIC_ITARGETSR_MASK << old_cpu_id > > or similar? This at least points one to the right bit of documentation? I've renamed old_cpu_id to cur_cpu_id and old_mask to cur_target_mask. Also added a comment later on. > > + ror_val = (old_cpu_id - new_cpu_id) & 31; > > + > > + raw_spin_lock(&irq_controller_lock); > > + > > + gic_cpu_map[cpu] = 1 << new_cpu_id; > > + > > + for (i = 8; i < DIV_ROUND_UP(gic_irqs, 4); i++) { > > Does this '8' here warrant a #define? GIC_RO_INTR_REGS? > > Perhaps everyone looking at the code will be familiar enough with the GIC to > see immediately why the first 8 entries are being skipped...? I've added a comment right before this loop explaining what is being done, mentioning why it starts at 8. > I'm curious as to why we don't need to do anything for PPIs here - could there > be any pending PPIs that don't get handled (I guess retargetting doesn't make > sense for these?) PPIs are strictly processor private and I don't think there is any way for one processor to set them on another processor. So the only way to cope with those is for the outbound CPU to save and disable its PPI state, and let the inbound CPU enable them. That should be up to each PPI-using driver to do via the cpu_pm_enter()/cpu_pm_exit() notifiers. I've amended the patch with the following: diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 6bd5a8c1aa..268874ac75 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -668,7 +668,7 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) #ifdef CONFIG_BL_SWITCHER /* - * gic_migrate_target - migrate IRQs to another PU interface + * gic_migrate_target - migrate IRQs to another CPU interface * * @new_cpu_id: the CPU target ID to migrate IRQs to * @@ -679,10 +679,10 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) */ void gic_migrate_target(unsigned int new_cpu_id) { - unsigned int old_cpu_id, gic_irqs, gic_nr = 0; + unsigned int cur_cpu_id, gic_irqs, gic_nr = 0; void __iomem *dist_base; int i, ror_val, cpu = smp_processor_id(); - u32 val, old_mask, active_mask; + u32 val, cur_target_mask, active_mask; if (gic_nr >= MAX_GIC_NR) BUG(); @@ -692,17 +692,23 @@ void gic_migrate_target(unsigned int new_cpu_id) return; gic_irqs = gic_data[gic_nr].gic_irqs; - old_cpu_id = __ffs(gic_cpu_map[cpu]); - old_mask = 0x01010101 << old_cpu_id; - ror_val = (old_cpu_id - new_cpu_id) & 31; + cur_cpu_id = __ffs(gic_cpu_map[cpu]); + cur_target_mask = 0x01010101 << cur_cpu_id; + ror_val = (cur_cpu_id - new_cpu_id) & 31; raw_spin_lock(&irq_controller_lock); + /* Update the target interface for this logical CPU */ gic_cpu_map[cpu] = 1 << new_cpu_id; + /* + * Find all the peripheral interrupts targetting the current + * CPU interface and migrate them to the new CPU interface. + * We skip DIST_TARGET 0 to 7 as they are read-only. + */ for (i = 8; i < DIV_ROUND_UP(gic_irqs, 4); i++) { val = readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4); - active_mask = val & old_mask; + active_mask = val & cur_target_mask; if (active_mask) { val &= ~active_mask; val |= ror32(active_mask, ror_val); @@ -714,7 +720,7 @@ void gic_migrate_target(unsigned int new_cpu_id) /* * Now let's migrate and clear any potential SGIs that might be - * pending for us (old_cpu_id). Since GIC_DIST_SGI_PENDING_SET + * pending for us (cur_cpu_id). Since GIC_DIST_SGI_PENDING_SET * is a banked register, we can only forward the SGI using * GIC_DIST_SOFTINT. The original SGI source is lost but Linux * doesn't use that information anyway.