From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v6a 05/17] xen/arm: use ioremap to map gic-v2 registers Date: Thu, 26 Jun 2014 13:07:37 +0100 Message-ID: <53AC0D09.4050405@linaro.org> References: <1403760849-14627-1-git-send-email-vijay.kilari@gmail.com> <1403760849-14627-6-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1403760849-14627-6-git-send-email-vijay.kilari@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: vijay.kilari@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org Cc: Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hi Vijay, On 06/26/2014 06:33 AM, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > gic-v2 driver uses fixmap to map the registers. > Instead use ioremap to access mmio registers. > > With this patch, gic-v2 register definitions are updated > to use absolute offset address instead of dividing the > register offset by 4. I remembered I requested to made some changes in this patch. I saw you did it but you didn't specify in the changelog. People may not be agree with your new changes and you should notifying about any change in the code! [..] > @@ -229,8 +232,7 @@ static void gic_set_irq_properties(struct irq_desc *desc, > const cpumask_t *cpu_mask, > unsigned int priority) > { > - volatile unsigned char *bytereg; > - uint32_t cfg, edgebit; > + uint32_t cfg, edgebit, itarget, ipriority; > unsigned int mask; > unsigned int irq = desc->irq; > unsigned int type = desc->arch.type; > @@ -243,21 +245,28 @@ static void gic_set_irq_properties(struct irq_desc *desc, > mask = gic_cpu_mask(cpu_mask); > > /* Set edge / level */ > - cfg = GICD[GICD_ICFGR + irq / 16]; > + cfg = readl_relaxed(GICD + GICD_ICFGR + (irq / 16) * 4); > edgebit = 2u << (2 * (irq % 16)); > if ( type & DT_IRQ_TYPE_LEVEL_MASK ) > cfg &= ~edgebit; > else if ( type & DT_IRQ_TYPE_EDGE_BOTH ) > cfg |= edgebit; > - GICD[GICD_ICFGR + irq / 16] = cfg; > + writel_relaxed(cfg, GICD + GICD_ICFGR + (irq / 16) * 4); > > /* Set target CPU mask (RAZ/WI on uniprocessor) */ > - bytereg = (unsigned char *) (GICD + GICD_ITARGETSR); > - bytereg[irq] = mask; > - > + itarget = readl_relaxed(GICD + GICD_ITARGETSR + (irq / 4) * 4); > + /* Clear mask */ > + itarget &= ~(0xffu << (8 * (irq % 4))); > + /* Set mask */ > + itarget |= mask << (8 * (irq % 4)); > + writel_relaxed(itarget, GICD + GICD_ITARGETSR + (irq / 4) * 4); Why didn't you use writeb_relaxed? This change is complex for nothing. writeb_relaxed(mask, GICD + GICD_ITARGETSR + irq); > + > + ipriority = readl_relaxed(GICD + GICD_IPRIORITYR + (irq / 4) * 4); > + /* Clear priority */ > + ipriority &= ~(0xffu << (8 * (irq % 4))); > /* Set priority */ > - bytereg = (unsigned char *) (GICD + GICD_IPRIORITYR); > - bytereg[irq] = priority; > + ipriority |= priority << (8 * (irq % 4)); > + writel_relaxed(ipriority, GICD + GICD_IPRIORITYR + (irq / 4) * 4); writeb_relaxed(priority, GICD + GICD_IPRIORITYR + irq); Regards, -- Julien Grall